diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index f9d0c55661..dd96bed078 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -5648,6 +5648,8 @@ impl Engine { // (plan-mode role override above is intentionally before this). rt.worker_profile.denied_tools = self.config.disallowed_tools.clone().unwrap_or_default(); + rt.worker_profile.explicit_denied_tools = + self.config.disallowed_tools.clone().unwrap_or_default(); if let Some(context) = fork_context_for_runtime.clone() { rt = rt.with_fork_context(context); } @@ -7364,6 +7366,8 @@ impl Engine { rt.worker_profile = WorkerRuntimeProfile::for_role(FleetRole::Planner); } rt.worker_profile.denied_tools = self.config.disallowed_tools.clone().unwrap_or_default(); + rt.worker_profile.explicit_denied_tools = + self.config.disallowed_tools.clone().unwrap_or_default(); Some(rt) } diff --git a/crates/tui/src/core/engine/tool_catalog.rs b/crates/tui/src/core/engine/tool_catalog.rs index 10dd8298eb..0ddf526693 100644 --- a/crates/tui/src/core/engine/tool_catalog.rs +++ b/crates/tui/src/core/engine/tool_catalog.rs @@ -805,22 +805,29 @@ pub(super) fn tool_denied(disallowed_tools: Option<&[String]>, tool_name: &str) } pub(crate) fn tool_matches_any_rule(rules: &[String], tool_name: &str) -> bool { + rules.iter().any(|rule| rule_matches_tool(rule, tool_name)) +} + +/// Whether one deny/allow rule matches `tool_name`, with the same +/// canonicalization [`tool_matches_any_rule`] applies across a list. Split +/// out so callers that must tell rules apart by provenance (an operator's +/// explicit `bash` rule vs the role clamp's automatic `Bash` rule — the +/// strings are identical) can classify each entry. +pub(crate) fn rule_matches_tool(rule: &str, tool_name: &str) -> bool { let tool_name = tool_name.to_ascii_lowercase(); - rules.iter().any(|rule| { - let rule = rule.to_ascii_lowercase(); - let (rule_body, is_prefix) = rule - .strip_suffix('*') - .map_or((rule.as_str(), false), |prefix| (prefix, true)); - std::iter::once(tool_name.as_str()) - .chain(policy_tool_aliases(&tool_name).iter().copied()) - .any(|candidate| { - if is_prefix { - candidate.starts_with(rule_body) - } else { - candidate == rule_body - } - }) - }) + let rule = rule.to_ascii_lowercase(); + let (rule_body, is_prefix) = rule + .strip_suffix('*') + .map_or((rule.as_str(), false), |prefix| (prefix, true)); + std::iter::once(tool_name.as_str()) + .chain(policy_tool_aliases(&tool_name).iter().copied()) + .any(|candidate| { + if is_prefix { + candidate.starts_with(rule_body) + } else { + candidate == rule_body + } + }) } /// Whether an explicit tool allowlist is provably limited to the native file diff --git a/crates/tui/src/fleet/executor.rs b/crates/tui/src/fleet/executor.rs index a508e8545e..3871f1a970 100644 --- a/crates/tui/src/fleet/executor.rs +++ b/crates/tui/src/fleet/executor.rs @@ -243,8 +243,14 @@ pub(crate) fn authority_envelope_for_worker( && spec.runtime_profile.shell.allows_shell() { // Scout, Reviewer, and Planner get one classifier-bounded - // foreground shell. Verifiers keep the dedicated Run surface, - // consultants remain shell-less, and writers retain the historical + // foreground shell. On this executor path the Verifier keeps only + // the dedicated Run surface below. (The classifier-bounded `bash` + // evidence lane named by + // `SubAgentToolRegistry::allows_bounded_readonly_bash` is an + // in-session delegation surface: a fleet member spawned here has no + // in-process subagent registry at all, so it never holds that lane — + // see the fleet mutual-exclusivity note in docs/SUBAGENTS.md.) + // Consultants remain shell-less, and writers retain the historical // subprocess contract. ToolShellAuthority::ReadOnly } else { diff --git a/crates/tui/src/fleet/manager.rs b/crates/tui/src/fleet/manager.rs index ec3257002e..8a2df63982 100644 --- a/crates/tui/src/fleet/manager.rs +++ b/crates/tui/src/fleet/manager.rs @@ -4884,7 +4884,16 @@ esac !permissions.write, "verifier receipt {key} must stay read-only" ); - assert_eq!(permissions.shell, "full"); + // The verifier joined the read-only-shell roles, so the + // receipt records the narrowed effective posture (the old + // `full` label overclaimed a raw shell the clamp always + // denied). On the fleet envelope the verifier holds the + // gates with no bash surface at all — the classifier + // probe lane is in-session only; in-session, its bash is + // the classifier-bounded lane. The built-in verification + // surface stays keyed on the shell ceiling, not on this + // label. + assert_eq!(permissions.shell, "read_only"); } role => panic!("unexpected receipt role for {key}: {role:?}"), } diff --git a/crates/tui/src/fleet/role.rs b/crates/tui/src/fleet/role.rs index fe729af6b7..7839988404 100644 --- a/crates/tui/src/fleet/role.rs +++ b/crates/tui/src/fleet/role.rs @@ -77,10 +77,12 @@ pub enum FleetRole { /// posture pushes hard on landing the change cleanly with the /// minimum surrounding edit (#404). Builder, - /// Verification — focused on running the test suite or other - /// validation gates and reporting pass/fail with evidence. - /// Distinct from `Reviewer` in that Reviewer reads code and grades it; - /// Verifier *runs* tests and reports the outcome (#404). + /// Verification — runs the built-in verification gates and reports + /// pass/fail with evidence; the gates are refused when a read-only-shell + /// parent clamps the child's shell authority. In-session delegations + /// also carry the bounded read-only probe lane. Distinct from `Reviewer` + /// in that Reviewer reads code and grades it; Verifier *runs* the gates + /// and reports the outcome (#404). Verifier, /// Advisory counsel — a strong-model second opinion the operator can ask /// for guidance, judgement calls, and design critique (#4752). @@ -193,7 +195,7 @@ impl FleetRole { "Lands focused code changes cleanly with the minimum surrounding edit." } Self::Verifier => { - "Runs the test suite and validation gates, reports pass/fail with evidence." + "Runs the built-in verification gates with test-selection arguments unless a read-only-shell parent clamped its shell authority (the clamped child keeps the bounded read-only probes and loses the gates), plus — for in-session delegations — a bounded read-only shell for evidence gathering; cannot execute custom commands — use a write-capable role for those." } Self::Consultant => { "Read-only high-reasoning counsel for judgement calls and design critique." @@ -350,9 +352,11 @@ pub(crate) const MUTATING_TOOL_DENYLIST: &[&str] = &[ /// receipt says `write=false`. Denying the raw shell entries and leaving the /// bounded verification surface (`Run` / `run_tests` / `run_verifiers`) intact /// keeps the verifier able to do its job under a contract that is true. -/// Scout/reviewer read-only inspection selectively removes only canonical `Bash` from this -/// deny list after the role is known; its input-specific read-only classifier -/// remains the authority for that narrow exception. +/// Scout/reviewer/planner read-only inspection selectively removes only canonical `Bash` from this +/// deny list after the role is known (`from_runtime_role`'s retain); a verifier's +/// classifier-bounded `bash` is admitted separately by the subagent registry's +/// probe lane (`allows_bounded_readonly_bash`), not by that retain; the +/// input-specific read-only classifier remains the authority for both narrow exceptions. /// /// That surface is bounded only in its **default** form, and the distinction is /// load-bearing: `run_verifiers` accepts a `commands` array of arbitrary @@ -514,8 +518,10 @@ impl ChildAuthority { // Raw shell requires the ceiling to *say* `shell = "full"`. Any narrower // shell posture — `none` or `read_only` — loses the raw command surface // outright. `from_runtime_role` may subsequently - // retain canonical `Bash` for a named scout/reviewer, whose concrete - // calls are bounded by the strict read-only classifier. + // retain canonical `Bash` for the Scout/Reviewer/Planner trio, whose concrete + // calls are bounded by the strict read-only classifier; a Verifier's probe + // `bash` is admitted by the registry lane instead, without touching this + // deny list. // // This is deliberately keyed on the shell field rather than only on // `write`, and that is the whole repair: the execution envelope reads @@ -644,9 +650,13 @@ impl ChildAuthority { FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner ) && authority.ceiling.shell != ShellCeiling::None { - // Runtime's Scout/Reviewer policy permits classifier-bounded Bash - // inspection. Keep the canonical entry while all other shell and - // execution aliases remain denied. + // Runtime's Scout/Reviewer/Planner policy permits classifier-bounded + // Bash inspection. Keep the canonical entry while all other shell and + // execution aliases remain denied. This literal is the one role set + // that intentionally differs from `role_requires_read_only_shell` + // (which adds Verifier): a Verifier's probe `bash` is admitted over + // the deny by the registry lane, not by this retain, so changing + // either side means changing the member fingerprint or dropping a lane. authority .disallowed_tools .retain(|name| !name.eq_ignore_ascii_case("Bash")); @@ -715,11 +725,20 @@ pub(crate) fn posture_role_for(ceiling: PermissionCeiling) -> &'static str { /// Whether a Fleet role is never allowed a mutating shell, whatever its /// requested runtime profile says. Spawn narrows the child to a read-only /// shell for these roles, and every receipt must report that same posture. +/// +/// The narrowing downgrades only the raw-command policy: canonical `bash` +/// becomes the classifier-bounded probe lane, while the bounded built-in +/// verification surface keeps keying on the role's shell *ceiling* and the +/// shell-authority sentinel — the `test` role keeps both, because running the +/// workspace's own gates is its entire purpose. "Keeps both" describes the +/// in-session registry path; the fleet executor envelope gives the Verifier +/// only the bounded verification surface (`shell = none`, no probe lane) by +/// design. #[must_use] pub(crate) fn role_requires_read_only_shell(role: &FleetRole) -> bool { matches!( role, - FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner + FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner | FleetRole::Verifier ) } diff --git a/crates/tui/src/fleet/roster.rs b/crates/tui/src/fleet/roster.rs index c9ac4a2c7d..fef749f24b 100644 --- a/crates/tui/src/fleet/roster.rs +++ b/crates/tui/src/fleet/roster.rs @@ -481,7 +481,7 @@ impl FleetRoster { "verifier", FleetSlot::Verifier, FleetLoadout::Inherit, - "Runs builds and tests to verify claims; reports evidence, does not patch.", + "Runs the built-in verification gates to verify claims; reports evidence, does not patch, holds no shell.", None, ), ( diff --git a/crates/tui/src/fleet/worker_runtime.rs b/crates/tui/src/fleet/worker_runtime.rs index 6d86a1c8e9..8d862dfb6b 100644 --- a/crates/tui/src/fleet/worker_runtime.rs +++ b/crates/tui/src/fleet/worker_runtime.rs @@ -1521,11 +1521,15 @@ pub(crate) fn network_posture_warning_for_task( return None; } + // Name the shell the role actually runs under (read-only roles run + // narrowed), not the requested default — the same truth the receipts + // report. + let effective = crate::fleet::role::effective_runtime_profile_for_role(&agent_type, &runtime); Some(format!( "Fleet task `{}` mentions network-backed verification, but role `{}` has network=off and shell={}. Dispatch a `worker` role with shell `read_only` for gh/curl evidence, or revise the brief.", task.id, role.as_deref().unwrap_or("worker"), - shell_policy_label(runtime.shell), + shell_policy_label(effective.shell), )) } @@ -1600,7 +1604,12 @@ mod tests { ..WorkerRuntimeProfile::default() }; - for role in [FleetRole::Scout, FleetRole::Reviewer, FleetRole::Planner] { + for role in [ + FleetRole::Scout, + FleetRole::Reviewer, + FleetRole::Planner, + FleetRole::Verifier, + ] { let effective = role::effective_runtime_profile_for_role(&role, &requested); assert_eq!(effective.shell, ShellPolicy::ReadOnly, "{role:?}"); assert_eq!( diff --git a/crates/tui/src/tools/registry.rs b/crates/tui/src/tools/registry.rs index df854b680c..17130b5066 100644 --- a/crates/tui/src/tools/registry.rs +++ b/crates/tui/src/tools/registry.rs @@ -263,7 +263,16 @@ impl ToolRegistry { .filter(|tool| tool.model_visible()) .filter(|tool| { read_only_authority.is_none_or(|authority| { - readonly_evidence_tool(tool.as_ref()) + // The classifier-bounded probe lane is an evidence tool, + // but a worker whose envelope grants no shell authority + // refuses every bash call at dispatch + // (`enforce_tool_authority` demands + // `ToolShellAuthority::ReadOnly`); listing it would + // advertise a surface the worker can never use. + let evidence = readonly_evidence_tool(tool.as_ref()) + && !(matches!(tool.name(), "bash" | "Bash") + && authority.shell != super::spec::ToolShellAuthority::ReadOnly); + evidence || (tool.name() == "Run" && authority.verification == super::spec::ToolVerificationAuthority::Bounded) diff --git a/crates/tui/src/tools/registry/tests.rs b/crates/tui/src/tools/registry/tests.rs index a56963ee52..abd677b868 100644 --- a/crates/tui/src/tools/registry/tests.rs +++ b/crates/tui/src/tools/registry/tests.rs @@ -711,6 +711,38 @@ fn readonly_verifier_context(workspace: &std::path::Path) -> ToolContext { .expect("bounded verifier authority") } +/// A worker whose envelope grants no shell authority refuses every bash call +/// at dispatch (`enforce_tool_authority` demands `ToolShellAuthority::ReadOnly` +/// for bash/Bash), so the evidence catalog must not advertise the lane it can +/// never use — while a worker whose envelope DOES grant read-only shell keeps +/// the classifier-bounded probe lane. +#[test] +fn machine_catalog_omits_bash_without_shell_authority() { + let tmp = tempdir().expect("tempdir"); + let shell_less = ToolRegistryBuilder::new() + .with_read_only_shell_tool() + .build(readonly_verifier_context(tmp.path())); + let shell_less_tools = shell_less.to_api_tools(); + let names: Vec<&str> = shell_less_tools + .iter() + .map(|tool| tool.name.as_str()) + .collect(); + assert!( + !names.contains(&"bash") && !names.contains(&"Bash"), + "a shell-less verifier catalog must not list the bash lane: {names:?}" + ); + // The twin: a read-only-shell envelope keeps the lane. + let scoped = ToolRegistryBuilder::new() + .with_read_only_shell_tool() + .build(readonly_scout_context(tmp.path(), true)); + let scoped_tools = scoped.to_api_tools(); + let names: Vec<&str> = scoped_tools.iter().map(|tool| tool.name.as_str()).collect(); + assert!( + names.contains(&"bash"), + "a read-only-shell scout catalog keeps the classified probe lane: {names:?}" + ); +} + #[test] fn machine_verifier_catalog_and_dispatch_add_only_bounded_run() { let tmp = tempdir().expect("tempdir"); @@ -917,6 +949,30 @@ fn fleet_authority_intersects_readonly_github_bash_with_network_ceiling() { .expect_err("network denial must win") .to_string(); assert!(error.contains("does not grant network access"), "{error}"); + // The envelope admits Bash through the parallel classifier, which refuses + // compounds and globs outright, so a gh read inside one fails closed + // before the per-stage network predicate is consulted — with or without + // a network grant. Compound probes are an in-session `agent` capability, + // not a fleet one. + for command in [ + "pwd; gh repo view", + "gh pr list --search '*'", + "gh repo view | head -n 3", + ] { + let compound = json!({"action": "run", "command": command}); + for context in [&offline, &networked] { + let error = enforce_tool_authority("Bash", &compound, &shell, context) + .expect_err("a compound gh read fails closed under the fleet envelope") + .to_string(); + // The wall that fires is the parallel classifier's (compounds + // are never parallel-admitted), not the network ceiling: name it + // so a refusal from any other wall cannot green this leg. + assert!( + error.contains("outside its machine-readable authority envelope"), + "{command}: {error}" + ); + } + } } #[tokio::test] diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index af7965ef2f..62640f8b59 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -877,9 +877,11 @@ enum StdinWriter { /// writes can run on the bounded worker thread exactly like pipe /// writes. The reader thread holds its own clone of the master fd for /// the task's whole lifetime, so this dup is NOT the last master-side - /// handle: dropping it delivers nothing to the slave (the child sees no - /// EOF from a close here), which is why PTY closes never claim EOF — - /// the same observable behavior the `take_writer` box had. + /// handle: dropping it delivers nothing to the slave on its own. An + /// explicit `close_stdin` therefore delivers the ^D bytes itself, + /// through the same bounded worker as a payload write (see + /// [`bounded_stdin_eof_delivery`]), and a delivery that fails or + /// stalls claims no end-of-file. /// The cfg mirrors the only constructor (the unix arm inside the /// `not(target_env = "ohos")` PTY block — ohos returns from tty mode /// before PTY setup): keeping the variant off ohos avoids a @@ -913,42 +915,222 @@ impl StdinWriter { } } -/// The bounded stdin write for the platforms that cannot clone an fd out of -/// their handle (Windows and ohos, see [`ShellTask::write_stdin`]): the -/// worker thread takes the writer by value, finishes the payload, and hands -/// the writer back through the channel. `Ok` returns `(Some(writer), -/// Ok(io_result))`. `Err(RecvTimeoutError::Timeout)` returns `(None, ..)`: -/// the writer stays with the abandoned worker, the bytes still land if the -/// command resumes reading, and the caller marks the stall. +/// Blocks SIGPIPE for the guarded region on unix, restoring it on drop. +/// The process deliberately runs with SIGPIPE at SIG_DFL (`run_with_args` +/// re-arms it so delegated children see conventional pipe semantics), which +/// arms it for in-process threads too. A bounded stdin write that times out +/// deliberately leaves its worker blocked mid-write on the handle, and the +/// documented stall recovery — killing the task, or the stalled command +/// simply exiting later — closes the read end: the resumed write would +/// otherwise kill the whole host process with SIGPIPE instead of failing +/// with EPIPE like every handled write error. +/// +/// Blocking alone is not sufficient: a SIGPIPE generated while blocked is +/// not discarded — it stays PENDING on the worker thread, and unblocking +/// delivers a pending unblocked signal before `pthread_sigmask` returns. +/// With SIGPIPE at SIG_DFL that would kill the process one instruction +/// after the "handled" EPIPE, exactly where the guard ends. The drop +/// therefore checks this thread's pending set and consumes a pending +/// SIGPIPE with `sigwait` BEFORE restoring the mask. A write-failure +/// SIGPIPE is thread-directed, so a pending SIGPIPE inside a guarded +/// region is one this region's write generated; `sigpending` also reports +/// process-directed signals, but nothing in this process raises SIGPIPE +/// that way, and consuming such a signal under SIG_DFL would only spare +/// the process. Test binaries run with std's SIG_IGN, under +/// which a generated SIGPIPE is discarded at generation and never becomes +/// pending — which is why the suite cannot catch a block-and-restore guard; +/// the consume path is pinned by re-arming SIG_DFL inside a helper process +/// (see `sigpipe_guard_survives_a_broken_pipe_restore`). +struct BlockedSigpipeGuard { + #[cfg(unix)] + previous: libc::sigset_t, +} + +impl BlockedSigpipeGuard { + fn new() -> Self { + #[cfg(unix)] + { + // SAFETY: `zeroed` is valid for `sigset_t` (an array of + // integer words) and both pointers below are local stack + // values. A failed `pthread_sigmask` is not expected with + // these local sets; if it did fail, the write would run with + // the process default disposition (the pre-guard behavior) + // and the drop's restore would install the zeroed `previous` + // mask — an empty set, leaving this thread's signals unblocked. + unsafe { + let mut blocked: libc::sigset_t = std::mem::zeroed(); + let mut previous: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut blocked); + libc::sigaddset(&mut blocked, libc::SIGPIPE); + // Best-effort: if the mask cannot be set, the write runs + // with the process default and the failure mode is simply + // the pre-guard one. + libc::pthread_sigmask(libc::SIG_BLOCK, &blocked, &mut previous); + Self { previous } + } + } + #[cfg(not(unix))] + Self {} + } +} + +#[cfg(unix)] +impl Drop for BlockedSigpipeGuard { + fn drop(&mut self) { + unsafe { + // A write that ended in EPIPE raised SIGPIPE while it was + // blocked here. Consume it before the restore — otherwise the + // unblock delivers the pending signal at the process + // disposition (SIG_DFL in production) and the guard's own Drop + // kills the process it exists to protect (see the struct doc). + let mut pending: libc::sigset_t = std::mem::zeroed(); + libc::sigpending(&mut pending); + if libc::sigismember(&pending, libc::SIGPIPE) == 1 { + let mut blocked: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut blocked); + libc::sigaddset(&mut blocked, libc::SIGPIPE); + let mut delivered: i32 = 0; + // SAFETY: the signal is pending on this thread, so the + // wait returns immediately; the sets are local stack + // values. + libc::sigwait(&blocked, &mut delivered); + } + libc::pthread_sigmask(libc::SIG_SETMASK, &self.previous, std::ptr::null_mut()); + } + } +} + +#[cfg(not(unix))] +impl Drop for BlockedSigpipeGuard { + fn drop(&mut self) {} +} + +/// Outcome of the bounded end-of-file delivery behind an explicit +/// `close_stdin` on a unix terminal task: `Delivered` means the ^D bytes +/// landed, `Failed` means the write errored without landing (typically the +/// child was already gone and the master write failed), and `Stalled` means +/// the tty input queue was still full at the deadline. +#[cfg(all(unix, not(target_env = "ohos")))] +#[derive(Debug)] +enum BoundedEofDelivery { + Delivered, + Failed, + Stalled, +} + +/// Deliver `\n` + VEOF through the given master fd on the same bounded +/// worker a payload write uses ([`bounded_stdin_write_owned`]). The leading +/// newline flushes any pending partial line, because canonical mode only +/// turns VEOF into a read of length zero from an EMPTY line buffer — the +/// same two bytes the old ownership box injected on drop. In raw mode both +/// bytes are ordinary input, exactly like that box drop; the caller records +/// a delivery only when the write really completed, so the flag never +/// claims an end-of-file that did not land. The EOF byte is written +/// literally (`0x04`) rather than read from `c_cc[VEOF]`: portable-pty's +/// unix writer reads the live byte at drop and writes nothing at all when +/// VEOF is disabled (`_POSIX_VDISABLE`), so the two behaviors differ in +/// that state — this close sends the two bytes as ordinary input, exactly +/// like raw mode, and no end-of-file ever arrives. The `close_stdin` +/// schema discloses that; the old box delivered nothing in the same +/// state, so the close still reports honestly what it did. +#[cfg(all(unix, not(target_env = "ohos")))] +fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { + use std::os::fd::AsFd; + let dup_fd = match file.as_fd().try_clone_to_owned() { + Ok(fd) => fd, + // No clone means no worker can attempt the delivery; report it as + // a failed delivery — a clone failure is a process-fd problem, not + // a queue-full condition, and the flag must stay down either way. + Err(_) => return BoundedEofDelivery::Failed, + }; + match bounded_stdin_write_owned( + StdinWriter::Pipe(std::process::ChildStdin::from(dup_fd)), + b"\n\x04".to_vec(), + ) { + // The clone is dropped on every arm: the dup this function was + // given stays with the task, and only the write's outcome decides + // what the close may claim. + Err(_) => BoundedEofDelivery::Failed, + Ok((_, Ok(Ok(())))) => BoundedEofDelivery::Delivered, + Ok((_, Ok(Err(_)))) => BoundedEofDelivery::Failed, + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Timeout))) => BoundedEofDelivery::Stalled, + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Disconnected))) => BoundedEofDelivery::Failed, + } +} + +/// The single bounded stdin write every platform uses: the worker thread +/// takes the writer by value, writes and flushes the payload, and hands the +/// writer back through the channel. Unix reaches it with a clone of the +/// task's handle — pipes clone the `ChildStdin` fd and the PTY lane clones +/// the dup'd master fd (see [`BackgroundShell::write_stdin`] and +/// [`bounded_stdin_eof_delivery`]); Windows and ohos cannot clone an fd out +/// of their handle, so the handoff moves the real writer. +/// `Ok((Some(writer), Ok(io_result)))` returns the writer to the caller. +/// `Ok((None, Err(RecvTimeoutError::Timeout)))` means the deadline hit +/// first: the writer stays with the abandoned worker, the bytes still land +/// if the command resumes reading, and the caller marks the stall. /// `Err(RecvTimeoutError::Disconnected)` means the worker died before /// completing (a panic in the write path): the writer is gone, and the /// caller must NOT mark a stall — a writer failure is not the command's -/// fault. Present on every target so the unix-host test suite exercises it -/// directly; production unix keeps the fd-clone path, which is why the -/// allow only covers the non-unix side. -#[cfg_attr(all(unix, not(target_env = "ohos")), allow(dead_code))] +/// fault. An `Err` from the function itself means the worker thread could +/// not be spawned at all: nothing was written, and the writer is gone with +/// the failed closure (ownership was already handed over), so the caller +/// must report the task as having no writable stdin. A plain +/// `thread::spawn` panic here would unwind through the shell-manager lock +/// and poison it for the rest of the process. +type OwnedBoundedWriteOutcome = ( + Option, + Result, std::sync::mpsc::RecvTimeoutError>, +); + fn bounded_stdin_write_owned( writer: StdinWriter, payload: Vec, -) -> ( - Option, - Result, std::sync::mpsc::RecvTimeoutError>, -) { +) -> anyhow::Result { let (sender, receiver) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let mut writer = writer; - let result = writer.write_all(&payload).and_then(|_| writer.flush()); - // The writer moves back out only on the send path; a send failure - // (receiver gone) drops it, which is fine — the caller already - // gave up on the handle at that point. - let _ = sender.send((writer, result)); - }); + let worker = std::thread::Builder::new() + .name("stdin-bounded-write".to_string()) + .spawn(move || { + let _sigpipe = BlockedSigpipeGuard::new(); + let mut writer = writer; + let result = writer.write_all(&payload).and_then(|_| writer.flush()); + // The writer moves back out only on the send path; a send failure + // (receiver gone) drops it, which is fine — the caller already + // gave up on the handle at that point. + let _ = sender.send((writer, result)); + }); + let receiver = match worker { + Ok(_) => receiver, + Err(error) => { + return Err(anyhow!(error).context("Failed to spawn the bounded stdin writer thread")); + } + }; match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok((writer, result)) => (Some(writer), Ok(result)), - Err(error) => (None, Err(error)), + Ok((writer, result)) => Ok((Some(writer), Ok(result))), + Err(error) => Ok((None, Err(error))), } } +/// Duplicate `fd` with the close-on-exec flag set (`F_DUPFD_CLOEXEC`). +/// +/// The dup'd PTY master stays in this process for a task's whole lifetime, +/// so it must behave like every other handle this crate holds for the task: +/// invisible to children spawned meanwhile. portable-pty opens the pty with +/// `FD_CLOEXEC` and its own reader/writer clones re-apply it +/// (`F_DUPFD_CLOEXEC`), but a bare `libc::dup` CLEARS the flag on the new +/// descriptor — every command spawned while a terminal task runs would +/// inherit a live master-side fd aliasing that task's terminal, so a child +/// could write into (inject input to) or read (capture output from) another +/// task's terminal and keep the master alive past the task's teardown. +#[cfg(all(unix, not(target_env = "ohos")))] +fn dup_cloexec(fd: std::os::fd::RawFd) -> std::io::Result { + let dup_fd = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 0) }; + if dup_fd < 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(dup_fd) +} + fn spawn_reader_thread( mut reader: R, buffer: SharedRawOutput, @@ -1133,18 +1315,21 @@ pub struct BackgroundShell { stderr_cursor: usize, completion_reported: bool, stdin: Option, - /// Set when the stdin *pipe* was closed through `write_stdin`, i.e. the - /// child has really been handed end-of-file. Stays false when the task - /// never exposed a writable stdin (persistent services spawn with a null - /// stdin, PTY lanes keep the slave open through the reader) or when the - /// handle was simply released after completion, so `interact` can state - /// the true reason stdin is unavailable. + /// Set when the child has really been handed end-of-file: either its + /// stdin pipe was closed through `write_stdin`, or an explicit unix + /// terminal close delivered the ^D bytes through the bounded worker. + /// Stays false when the task never exposed a writable stdin (persistent + /// services spawn with a null stdin, PTY lanes keep the slave open + /// through the reader), when the handle was simply released after + /// completion, and when a close delivery failed or stalled — so + /// `interact` can state the true reason stdin is unavailable. stdin_closed_at_eof: bool, /// Set when a stdin write outlived [`STDIN_WRITE_DEADLINE`] because the - /// command stopped consuming its pipe. The abandoned bytes keep flowing - /// from the background writer if the child resumes, so further writes - /// are refused (two writers could interleave) instead of queued behind - /// an unknown pipe state. + /// command stopped consuming its pipe, or when a terminal close's own + /// ^D delivery stalled the same way (same deadline, same refusal + /// family). The abandoned bytes keep flowing from the background writer + /// if the child resumes, so further writes are refused (two writers + /// could interleave) instead of queued behind an unknown pipe state. stdin_write_stalled: bool, child: Option, /// A stop was delivered but the direct child's exit was never observed: @@ -1495,9 +1680,12 @@ impl BackgroundShell { fn write_stdin(&mut self, input: &str, close: bool) -> Result<()> { // Taken owned so the Windows/ohos fallback can move the writer into - // its bounded worker and hand it back; every exit below restores the + // its bounded worker and hand it back. Every unix exit restores the // slot first, keeping the stall/eof refusal ordering identical with - // the pinned unix behavior. + // the pinned unix behavior; on the ownership-fallback cfgs the + // Timeout, Disconnected, and spawn-failure arms deliberately leave + // the slot empty — the writer is gone with the worker, and the stall + // flag or the returned refusal reports that. // `mut` is real only on the ownership-fallback cfgs (they re-take and // reassign the slot); the unix fd-clone path only moves it, so the // same predicate that strips the fallback also silences `unused_mut`. @@ -1561,21 +1749,24 @@ impl BackgroundShell { )); } }; - let mut writer = std::process::ChildStdin::from(dup_fd); - let payload = input.as_bytes().to_vec(); - let (sender, receiver) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let _ = - sender.send(writer.write_all(&payload).and_then(|_| writer.flush())); - }); - match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok(Ok(())) => {} - Ok(Err(error)) => { - // The worker completed and handed the handle - // back: restore it so a write error does not - // silently strip the task's interact surface - // (the base behavior kept the handle attached - // after a write error too). + // The bounded write is the shared helper (the same one + // the owned platforms and the ^D close delivery use): + // the clone rides it by value, so only the ORIGINAL + // slot decides what is restored below — a returned + // clone is dropped, never reattached. + match bounded_stdin_write_owned( + StdinWriter::Pipe(std::process::ChildStdin::from(dup_fd)), + input.as_bytes().to_vec(), + ) { + // The clone came back with a completed write: drop + // it and keep the original attached. + Ok((_, Ok(Ok(())))) => {} + Ok((_, Ok(Err(error)))) => { + // The worker completed: restore the ORIGINAL + // handle so a write error does not silently + // strip the task's interact surface (the base + // behavior kept the handle attached after a + // write error too). self.stdin = slot; return Err(anyhow!(error).context("Failed to write to stdin")); } @@ -1584,20 +1775,29 @@ impl BackgroundShell { // path): that is a writer failure, not a stall, so // the task is not marked stalled — a healthy write // after this must not be refused by the flag. - Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Disconnected))) => { self.stdin = slot; - return Err(anyhow!( - "the stdin writer for task {} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one.", - self.id - )); + return Err(anyhow!(stdin_writer_failed_refusal(&self.id))); } - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Timeout))) => { + // The clone stays with the abandoned worker; + // the original handle is restored, and the + // stall flag refuses any further write so two + // writers can never interleave on one handle. self.stdin_write_stalled = true; self.stdin = slot; - return Err(anyhow!( - "the write to task {} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one.", - self.id - )); + return Err(anyhow!(stdin_stalled_write_refusal(&self.id))); + } + Err(error) => { + // The spawn failed before any byte was written + // and the worker holds nothing: restore the + // handle and fail like a plain write error. A + // bare `thread::spawn` panic here would unwind + // through the shell-manager mutex and poison it + // for the rest of the process. + self.stdin = slot; + return Err(anyhow!(error) + .context("Failed to spawn the bounded stdin writer thread")); } } } else { @@ -1619,8 +1819,17 @@ impl BackgroundShell { #[cfg(any(target_env = "ohos", not(unix)))] { let writer = slot.take().expect("slot held above"); - let (returned, result) = - bounded_stdin_write_owned(writer, input.as_bytes().to_vec()); + let (returned, result) = bounded_stdin_write_owned( + writer, + input.as_bytes().to_vec(), + ) + .map_err(|error| { + // The worker could not be spawned and the writer + // went with the failed closure: `slot` stays None, + // so the task honestly reports no writable stdin + // from here on. + anyhow!(error).context("Failed to spawn the bounded stdin writer thread") + })?; match result { Ok(Ok(())) => { slot = returned; @@ -1638,10 +1847,7 @@ impl BackgroundShell { // `slot` stays None: the abandoned worker owns // the handle, so the close below cannot claim // EOF and further writes hit the stall refusal. - return Err(anyhow!( - "the write to task {} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one.", - self.id - )); + return Err(anyhow!(stdin_stalled_write_refusal(&self.id))); } Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { // The worker died before completing (a panic in @@ -1650,10 +1856,7 @@ impl BackgroundShell { // healthy write after this is not refused, and // the writer is gone with the worker (nothing // to restore). - return Err(anyhow!( - "the stdin writer for task {} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one.", - self.id - )); + return Err(anyhow!(stdin_writer_failed_refusal(&self.id))); } } } @@ -1665,29 +1868,78 @@ impl BackgroundShell { // on a pty exited 0 the instant it dropped), and the dup // the terminal lane now holds delivers nothing on its own // because the reader holds the master — so write the same - // ^D semantics through the dup, then drop it. The flag - // only follows a real delivery. A task whose last write - // STALLED must not claim EOF either: the abandoned worker - // still holds a clone of the handle and the bytes have not - // finished landing, so the flag stays down and the tail - // refusal reports the stall instead. - match slot { - Some(StdinWriter::Pipe(_)) => { - self.stdin_closed_at_eof = !self.stdin_write_stalled; - } - #[cfg(all(unix, not(target_env = "ohos")))] - Some(StdinWriter::PtyFile(mut file)) if !self.stdin_write_stalled => { - { - use std::io::Write as _; - // Ignored if the child is already gone; in raw - // mode VEOF reads as ordinary input, exactly - // like the box drop it replaces. - let _ = file.write_all(b"\n\x04"); - let _ = file.flush(); + // ^D semantics through the dup, then drop it. The delivery + // runs on the same kind of bounded worker as a payload + // write: two bytes into a tty whose input queue is already + // full (the earlier payload completed inside the deadline, + // so the stall flag is still down) block just as forever + // as a large payload would, and inline under the manager + // lock they wedge every shell in the session. + #[cfg(all(unix, not(target_env = "ohos")))] + if let Some(StdinWriter::PtyFile(file)) = slot.as_ref() { + if !self.stdin_write_stalled { + match bounded_stdin_eof_delivery(file) { + BoundedEofDelivery::Delivered => { + self.stdin = None; + self.stdin_closed_at_eof = true; + return Ok(()); + } + // The close consumed the handle as requested, + // but the bytes did not land (typically the + // child was already gone and the master write + // failed): keep the flag down so later refusals + // do not claim readers saw end-of-file. Ok is + // honest here — the release the caller asked + // for happened, and no delivery is possible + // once the child is gone. + BoundedEofDelivery::Failed => { + self.stdin = None; + return Ok(()); + } + // Queue still full at the deadline: restore the + // handle under the stall flag and report the + // close as FAILED. The schema's close promises a + // delivered end-of-file, so a close that neither + // delivered nor released must not read as + // success — the same disposition the + // payload-timeout arm applies. + BoundedEofDelivery::Stalled => { + self.stdin_write_stalled = true; + self.stdin = slot; + return Err(anyhow!(stdin_stalled_close_refusal(&self.id))); + } } - self.stdin_closed_at_eof = true; } - _ => {} + // A task whose last write stalled gets no delivery + // attempt at all: the queue that stalled the payload + // would stall the two bytes too, so the close is + // refused without waiting and without destroying the + // interact surface — the model is told the close was + // not applied instead of reading a success that + // delivered nothing. + self.stdin = slot; + return Err(anyhow!(stdin_stalled_close_not_applied_refusal(&self.id))); + } + // Windows cannot deliver end-of-file to a console child at + // all (portable-pty's ConPTY writer has no EOF-on-drop — + // that exists only in its unix writer), so an explicit + // close here only releases the handle. Say so instead of + // returning a silent success that the schema's "Close + // stdin" would read as a delivered end-of-file. + #[cfg(all(not(unix), not(target_env = "ohos")))] + if matches!(slot, Some(StdinWriter::Pty(_))) { + self.stdin = None; + return Err(anyhow!( + "stdin close for task {} released the terminal's stdin handle, but this platform cannot deliver end-of-file to a console child; the command keeps running until it finishes or its timeout ends it.", + self.id + )); + } + // A task whose last write STALLED must not claim EOF: the + // abandoned worker still holds a clone of the handle and + // the bytes have not finished landing, so the flag stays + // down and the tail refusal reports the stall instead. + if let Some(StdinWriter::Pipe(_)) = slot { + self.stdin_closed_at_eof = !self.stdin_write_stalled; } self.stdin = None; } else { @@ -1709,12 +1961,12 @@ impl BackgroundShell { if self.stdin_closed_at_eof { return Err(anyhow!( - "stdin is not available for task {}: its stdin pipe was closed, so stdin readers have seen end-of-file; start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file (a program that put its terminal into raw mode — or disabled or remapped VEOF — consumes those bytes as ordinary input instead); start input-driven work with background=true and feed it with action=\"interact\"", self.id )); } Err(anyhow!( - "stdin is not available for task {}: no writable stdin handle is attached to it (a finished command has released its stdin, a persistent command never exposes one, and a terminal command exposes one only while it runs and only until its stdin is closed); start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: no writable stdin handle is attached to it (a finished command has released its stdin, a persistent command never exposes one, a terminal command exposes one only while it runs and only until its stdin is closed, and a bounded stdin writer that failed to spawn or panicked takes its handle with it); start input-driven work with background=true and feed it with action=\"interact\"", self.id )) } @@ -2850,6 +3102,17 @@ impl ShellManager { if let Some(input) = stdin_data && let Some(mut stdin) = child.stdin.take() { + // Same SIGPIPE discipline as the bounded stdin workers (see + // [`BlockedSigpipeGuard`]): the child dying mid-write must + // surface as a handled write error, not kill a process that + // runs with SIGPIPE at its default disposition. Unlike the + // tool surface this manager-API lane has neither + // [`MAX_INLINE_STDIN_BYTES`] nor the bounded writer's + // deadline — every production payload goes through the + // background lane's bounded writer (see + // [`STDIN_WRITE_DEADLINE`]), so a direct caller passing a + // payload here must keep it within what the child reads. + let _sigpipe = BlockedSigpipeGuard::new(); stdin .write_all(input.as_bytes()) .context("Failed to write to stdin")?; @@ -3167,8 +3430,13 @@ impl ShellManager { return Err(err).context("Failed to clone PTY reader"); } }; - let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); - + // The interact writer is acquired BEFORE the reader thread + // spawns, so a writer-acquisition failure aborts with only + // the child to clean up (kill + wait) instead of orphaning + // an already-spawned reader thread. Nothing observes the + // order: `pair.master` is alive across the whole block, so + // the slave sees no end-of-file in either arrangement. + // // Unix: the interact writer is a dup of the master fd, and // `take_writer` is NOT called — its box's Drop injects a // newline plus `VEOF` into the master (that is how "dropping @@ -3177,24 +3445,31 @@ impl ShellManager { // as an immediate end-of-file (probe-pinned: `cat` on a pty // exited 0 the instant the box was dropped). The dup keeps // a master-side description open — no EOF for the slave - // while the reader's clone is open too. `close_stdin` on - // this lane drops the dup: with the reader's clone still - // open the child sees nothing (the PTY close never promised - // an EOF and `stdin_closed_at_eof` never claimed one); the - // final master handle closes at teardown, after the child - // can no longer observe it. + // while the reader's clone is open too. A plain drop of the + // dup (the spawn-path close never touches this lane, and a + // close whose delivery failed releases it) still shows the + // child nothing; + // an explicit `close_stdin` instead delivers the ^D bytes + // itself through the bounded worker and claims end-of-file + // only when that write really landed; the final master + // handle closes at teardown, after the child can no longer + // observe it. The dup is CLOEXEC (`dup_cloexec`): children + // spawned while the task runs must not inherit a master-side + // fd, matching the `take_writer` box it replaces and + // portable-pty's own cloexec'd descriptors. #[cfg(unix)] let stdin_writer = { use std::os::fd::FromRawFd; match pair.master.as_raw_fd() { Some(fd) => { - let dup_fd = unsafe { libc::dup(fd) }; - if dup_fd < 0 { - let _ = child.kill(); - let _ = child.wait(); - return Err(std::io::Error::last_os_error()) - .context("Failed to dup the PTY master for the stdin writer"); - } + let dup_fd = dup_cloexec(fd) + .inspect_err(|_| { + let _ = child.kill(); + let _ = child.wait(); + }) + .with_context( + || "Failed to dup the PTY master for the stdin writer", + )?; // SAFETY: `dup_fd` is a freshly duplicated fd owned // by this call; `File::from_raw_fd` takes ownership. Some(StdinWriter::PtyFile(unsafe { @@ -3223,6 +3498,7 @@ impl ShellManager { return Err(err).context("Failed to take PTY writer"); } })); + let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); (ShellChild::Pty(child), stdin_writer, stdout_thread, None) } @@ -3374,8 +3650,14 @@ impl ShellManager { if let Some(input) = stdin_data && let Err(err) = bg_shell.write_stdin(input, false) { + // No task id exists yet — the record is only registered below — + // so `write_stdin`'s "kill it and start a new one" recovery copy + // has no target here. The spawn aborts instead: the child is + // killed and the caller learns the run was ended at spawn. let _ = bg_shell.kill(); - return Err(err); + return Err(err.context( + "the run was killed at spawn: the process was created, but delivering the inline stdin payload failed, so the command was killed before it could do any work and NO task was created (the task id the error above mentions was never registered — do not try to cancel or kill it)", + )); } if let Err(err) = bg_shell.publish_lifecycle() { @@ -3479,14 +3761,15 @@ impl ShellManager { /// PTY writer is invisible to the child (the reader holds the master) /// but destroys the interact surface a session member can still use /// while a terminal command runs — the foreground terminal lane's - /// contract. - fn close_stdin_pipe(&mut self, task_id: &str) -> Result<()> { - let shell = self - .processes - .get_mut(task_id) - .ok_or_else(|| anyhow!("Task {task_id} not found"))?; - shell.close_stdin_pipe(); - Ok(()) + /// contract. A missing record is a no-op, not an error: only eviction + /// removes a record, and eviction drops the stdin handle with it, so + /// the pipe cannot still be open — and the close runs between spawn + /// and the first caller-visible step, where an error would abort a run + /// whose result is already determined. + fn close_stdin_pipe(&mut self, task_id: &str) { + if let Some(shell) = self.processes.get_mut(task_id) { + shell.close_stdin_pipe(); + } } pub fn write_stdin_for_session( @@ -7912,10 +8195,10 @@ async fn execute_foreground_via_background( // them. The task-id extraction above is the one step between spawn and // close, and `spawn_background_sandboxed` always returns an id on `Ok`, // so the close reaches the shell layer without a fallible step in - // between (the manager-level `close_stdin_pipe` wrapper can still fail - // when the record was already evicted — and eviction drops the stdin - // handle with it, so the invariant that matters holds: no later - // failure can leave the pipe open). + // between (the manager-level `close_stdin_pipe` wrapper treats an + // already-evicted record as success — eviction drops the stdin + // handle with the record, so the invariant that matters holds: no + // later failure can leave the pipe open). // // Foreground has no operator at the keyboard, so whatever the command // will ever read from stdin must be bounded now. With stdin data the @@ -7941,7 +8224,7 @@ async fn execute_foreground_via_background( .shell_manager .lock() .map_err(|_| anyhow!("shell manager lock poisoned"))?; - manager.close_stdin_pipe(&task_id)?; + manager.close_stdin_pipe(&task_id); } if let Some(permit) = heavy_permit { let mut manager = context @@ -8019,27 +8302,66 @@ async fn execute_foreground_via_background( const BASH_MAX_TIMEOUT_MS: u64 = i32::MAX as u64; -/// Upper bound for one inline `stdin` payload. The spawn delivers it with a -/// blocking `write_all` while the shell-manager lock is held; on Linux a -/// payload equal to one pipe buffer (exactly 64 KiB) always completes -/// without the child reading, so at this cap the write can never wedge the -/// executor thread there. Windows and macOS pipe buffers are -/// system-decided/best-effort and may be smaller — the cap is a strict -/// improvement over the uncapped base everywhere, and the never-wedge -/// guarantee as stated is a Linux fact. Anything larger is refused with a -/// pointer at background `interact` streaming instead of hanging the -/// session. +/// Upper bound for one inline `stdin` payload (counted in UTF-8 bytes). +/// Every payload the tool surface accepts is delivered through the bounded +/// stdin writer (see [`STDIN_WRITE_DEADLINE`]), so a command that stops +/// reading surfaces as a deadline stall rather than a wedged executor; the +/// cap additionally keeps oversized payloads from ever reaching the +/// writer, and on Linux's default 64 KiB pipe buffer a cap-sized payload +/// completes into an empty pipe without a reader, so the common +/// payload-then-close foreground shape delivers without stalling there. +/// Platforms with smaller default pipe buffers lean on the deadline +/// instead. Anything larger is refused with a pointer at background +/// `interact` streaming instead of hanging the session. const MAX_INLINE_STDIN_BYTES: usize = 64 * 1024; -/// Wall-clock bound on one stdin write. The same blocking `write_all` runs -/// for `interact` payloads (and for the spawn payload on platforms whose -/// pipe buffer is smaller than the cap), where a command that stops -/// consuming stdin fills its pipe and would hold the shell-manager lock -/// forever. At the bound the write is abandoned to a background worker (the -/// bytes still land if the child resumes reading) and the task's stdin is -/// marked stalled. +/// Wall-clock bound on one stdin write. At the bound an `interact` write is +/// abandoned to its background worker (the bytes still land if the child +/// resumes reading), the task's stdin is marked stalled, and the manager +/// lock is released — so each stalled write costs one bounded wait under +/// the lock, and n stalled tasks serialize into n such waits. A stalled +/// SPAWN payload is different: no task id exists yet, so the spawn aborts +/// and the child is killed instead of running task-less. The abandoned +/// worker is SIGPIPE-blocked (see [`BlockedSigpipeGuard`]): when the task +/// is later killed — the documented stall recovery — the worker observes +/// EPIPE instead of taking the process down. const STDIN_WRITE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); +/// One shared copy per stdin refusal shape so the unix fd-clone path and +/// the owned-writer path cannot drift apart — the pins assert substrings +/// of these. They are functions rather than consts so the deadline +/// interpolation stays a real format capture. +fn stdin_stalled_write_refusal(task_id: &str) -> String { + format!( + "the write to task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one." + ) +} + +fn stdin_writer_failed_refusal(task_id: &str) -> String { + format!( + "the stdin writer for task {task_id} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one." + ) +} + +/// Serves only the unix PTY close arm (the Windows/ohos close arm has its +/// own inline copy and never takes the unix delivery path). Same +/// predicate as the `PtyFile` variant, so `deny(warnings)`' fatal +/// `dead_code` cannot fire on platforms where the arm is compiled out. +#[cfg(all(unix, not(target_env = "ohos")))] +fn stdin_stalled_close_refusal(task_id: &str) -> String { + format!( + "the stdin close for task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the terminal's input queue stayed full because the command stopped consuming input, so the end-of-file bytes were not delivered; the close was not applied and the interact surface stays attached. Further stdin writes to this task are refused; kill it and start a new one." + ) +} + +/// See [`stdin_stalled_close_refusal`]: unix-PTY-only, same cfg predicate. +#[cfg(all(unix, not(target_env = "ohos")))] +fn stdin_stalled_close_not_applied_refusal(task_id: &str) -> String { + format!( + "the stdin close for task {task_id} was not applied: its last stdin write stalled because the command stopped consuming input, so the end-of-file was never delivered and no close can deliver it now; the interact surface stays attached and further stdin writes are refused; kill it and start a new one." + ) +} + /// Initial cadence for foreground-via-background completion polling. Fast /// commands dominate real agent traffic; detection latency on `true`-class /// commands drops from ~100ms to ~10ms, while long commands reach the @@ -8374,7 +8696,7 @@ impl ToolSpec for BashTool { "stdin": { "type": "string", "maxLength": 65536, - "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB: a larger write to a command that is not reading would block the executor; for bigger payloads run the command in the background and feed it through `action=interact` in chunks." + "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB, counted as UTF-8 bytes (maxLength mirrors the same number): a write to a command that is not reading ends as a bounded deadline-stall refusal instead of blocking the session; for bigger payloads run the command in the background and feed it through `action=interact` in chunks." }, "input": { "type": "string", @@ -8422,7 +8744,7 @@ impl ToolSpec for BashTool { }, "close_stdin": { "type": "boolean", - "description": "Close stdin after sending (action=interact)" + "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode, or disabled or remapped VEOF, receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" }, "all": { "type": "boolean", @@ -8592,14 +8914,17 @@ impl ToolSpec for BashTool { .as_str() .ok_or_else(|| type_mismatch(name, value, "a string"))? .to_string(); - // The spawn delivers the payload with a blocking write while - // the caller holds the shell-manager lock: past the pipe - // buffer a command that never reads would wedge the - // executor thread and every other shell in the session - // before any watchdog is reached. The cap equals the Linux - // pipe buffer, so there a write that passes it completes - // unconditionally; bigger payloads belong on the background - // lane, fed through `action=interact` in chunks. + // The tool surface's inline cap keeps an oversized payload + // from ever reaching the writer; the spawn delivers the + // payload through the bounded stdin writer, so a command + // that never reads surfaces as the writer's 5-second + // deadline stall refusal rather than a wedged executor. On + // Linux the cap equals the default pipe buffer, so a + // cap-sized write into an empty pipe completes without a + // reader there; smaller default buffers lean on the same + // deadline on the bounded lanes. Bigger payloads belong on + // the background lane, fed through `action=interact` in + // chunks. if payload.len() > MAX_INLINE_STDIN_BYTES { return Ok(ToolResult::error(format!( "The inline stdin payload is {} bytes; the shell tool accepts at most {MAX_INLINE_STDIN_BYTES} inline — run the command with background=true and feed it through action=interact in chunks.", diff --git a/crates/tui/src/tools/shell/guidance.rs b/crates/tui/src/tools/shell/guidance.rs index 258512032a..5ca9ff5aab 100644 --- a/crates/tui/src/tools/shell/guidance.rs +++ b/crates/tui/src/tools/shell/guidance.rs @@ -86,7 +86,7 @@ pub(super) fn description() -> &'static str { \"interact\" sends stdin to a background task; \"cancel\" kills a background task. \ Pass wait=false for a nonblocking task snapshot. Foreground mode is for bounded \ commands; use background=true for work expected to take >5 seconds. Foreground \ - commands without `interactive` or `combined_output` run without a \ + commands without `interactive`, `tty`, or `combined_output` run without a \ terminal stdin — interactive programs and a bare `$SHELL` receive EOF and \ exit; `tty` always selects a background terminal task, and a foreground \ command with `combined_output` runs on a terminal lane that nothing \ diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 5186e6de27..320c9ef2b1 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -5778,7 +5778,7 @@ async fn readonly_github_calls_refuse_at_the_policy_inside_the_tool_entry() { fn guidance_states_the_foreground_stdin_contract() { let description = super::guidance::description(); assert!( - description.contains("Foreground commands without `interactive` or `combined_output` run without a terminal stdin"), + description.contains("Foreground commands without `interactive`, `tty`, or `combined_output` run without a terminal stdin"), "guidance must state the bounded foreground stdin: {description}" ); assert!( @@ -8501,6 +8501,8 @@ fn killed_shell_does_not_wait_for_blocked_reader_threads() { completion_reported: false, bounded_output: None, stdin: None, + stdin_closed_at_eof: false, + stdin_write_stalled: false, child: None, stop_unconfirmed: false, windows_job: None, @@ -9870,7 +9872,319 @@ async fn inline_stdin_payload_over_the_cap_refuses() { output.content ); } +/// Runs a stdin scenario on a worker thread behind a 60 s outer guard. The +/// scenario's own deadline machinery is what should finish it; a mechanism +/// regression that never returns trips this guard instead (the join can +/// extend past the guard while the scenario's child still holds the blocked +/// write open — the child's own watchdog releases it, so the guard +/// terminates, loudly late). A worker panic surfaces as a failed verdict, +/// never as a hung test. +#[cfg(unix)] +fn run_stdin_scenario_with_hang_guard( + name: &'static str, + body: impl FnOnce() -> Result<(), String> + Send + 'static, +) -> Result<(), String> { + let (sender, receiver) = std::sync::mpsc::channel::>(); + let handle = std::thread::spawn(move || { + let verdict = body(); + let _ = sender.send(verdict.clone()); + verdict + }); + match receiver.recv_timeout(Duration::from_secs(60)) { + Ok(verdict) => { + // Reaps the worker; the verdict from the channel is the + // scenario outcome, so the join's own copy adds nothing. + let _ = handle.join().expect("the scenario worker panicked"); + verdict + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + let _ = handle.join(); + Err(format!("{name} hung past its 60 s guard")) + } + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + // The worker died before its verdict reached the channel. + let _ = handle.join().expect_err("the scenario worker panicked"); + Err(format!("{name} panicked before reporting a verdict")) + } + } +} + +#[cfg(unix)] +const SIGPIPE_GUARD_HELPER_ENV: &str = "CODEWHALE_SIGPIPE_GUARD_HELPER"; +/// Lets the driver verify the helper actually RAN: libtest exits 0 on an +/// unmatched `--exact` filter, so a future rename of the helper test would +/// otherwise leave the pin silently green while guarding nothing. +#[cfg(unix)] +const SIGPIPE_GUARD_HELPER_CANARY_ENV: &str = "CODEWHALE_SIGPIPE_GUARD_HELPER_CANARY"; + +/// Helper half of `sigpipe_guard_survives_a_broken_pipe_restore`: runs in a +/// process that re-armed SIGPIPE at SIG_DFL (what `run_with_args` does in +/// production), takes the guard, writes into a readerless pipe, checks that +/// the drop restored the prior mask, and only then writes the ran-canary — +/// so the driver fails on a helper that died at signal 13 (consume removed), +/// panicked on a left-blocked mask (restore removed), or never ran (rename +/// drift). +#[cfg(unix)] +#[test] +fn sigpipe_guard_helper_process() { + if std::env::var(SIGPIPE_GUARD_HELPER_ENV).ok().as_deref() != Some("1") { + return; + } + // The suite binary runs with std's SIG_IGN, under which a broken-pipe + // SIGPIPE is discarded at generation and never becomes pending — the + // bug cannot reproduce there, so the helper re-arms the production + // disposition first. + unsafe { + libc::signal(libc::SIGPIPE, libc::SIG_DFL); + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(libc::pipe(fds.as_mut_ptr()), 0, "pipe"); + libc::close(fds[0]); + { + let guard = super::BlockedSigpipeGuard::new(); + let buf = [b'x'; 4096]; + let written = libc::write(fds[1], buf.as_ptr().cast(), buf.len()); + assert_eq!(written, -1, "the readerless write must fail"); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPIPE), + "the readerless write must fail with EPIPE" + ); + // Dropping the guard restores the mask. A block-and-restore + // guard delivers the pending SIGPIPE here and this process + // dies at signal 13 before reaching the end of the test; the + // consume-before-restore fix reaches it and exits 0. + drop(guard); + } + // The restore half is pinned too: after the drop, SIGPIPE must no + // longer be blocked on this thread. A no-op restore would leave + // every later SIGPIPE pending on this thread forever — invisible + // to the exit-status assertion above, so query the mask directly. + let mut now_blocked: libc::sigset_t = std::mem::zeroed(); + assert_eq!( + libc::pthread_sigmask(libc::SIG_BLOCK, std::ptr::null(), &mut now_blocked), + 0, + "query the signal mask" + ); + assert_eq!( + libc::sigismember(&now_blocked, libc::SIGPIPE), + 0, + "the guard drop must restore the prior mask (SIGPIPE must not stay blocked)" + ); + libc::close(fds[1]); + } + // Ran-canary for the driver: written only after every assertion above + // passed, so an unmatched filter (helper never ran) is distinguishable + // from a green run. + if let Some(canary) = std::env::var_os(SIGPIPE_GUARD_HELPER_CANARY_ENV) { + std::fs::write(&canary, b"ran").expect("write the ran-canary"); + } +} + +/// The SIGPIPE guard must consume the signal it blocked before restoring +/// the mask: a bounded write that ends in EPIPE leaves SIGPIPE pending on +/// the worker thread, and a plain block-and-restore delivers it at the +/// process disposition (SIG_DFL in production, see `run_with_args`) — +/// killing the whole host one instruction after the "handled" EPIPE. The +/// suite cannot see this (test binaries run SIG_IGN, so no SIGPIPE ever +/// becomes pending), so the driver re-execs this binary as a helper that +/// re-arms SIG_DFL first: with the consume the helper exits 0; without it +/// the helper dies at signal 13 and the non-success status fails here. +#[cfg(unix)] +#[test] +fn sigpipe_guard_survives_a_broken_pipe_restore() { + let test_binary = std::env::current_exe().expect("current test binary"); + let canary = std::env::temp_dir().join(format!( + "codewhale-sigpipe-guard-canary-{}", + std::process::id() + )); + let _ = std::fs::remove_file(&canary); + let mut helper = Command::new(&test_binary) + .arg("--exact") + .arg("tools::shell::tests::sigpipe_guard_helper_process") + .env(SIGPIPE_GUARD_HELPER_ENV, "1") + .env(SIGPIPE_GUARD_HELPER_CANARY_ENV, &canary) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sigpipe guard helper"); + let status = helper.wait().expect("wait sigpipe guard helper"); + assert!( + status.success(), + "the guard's mask restore must not deliver the pending SIGPIPE (a broken guard kills the helper at signal 13): {status}" + ); + assert!( + canary.exists(), + "the helper must actually run — libtest exits 0 on an unmatched --exact filter, so a green status without the canary means this pin guarded nothing (helper renamed?)" + ); + let _ = std::fs::remove_file(&canary); +} + +/// The PTY interact writer dup must carry close-on-exec: it stays open in +/// this process for a terminal task's whole lifetime, so a bare `dup` +/// (which CLEARS `FD_CLOEXEC`) would leak a live master-side terminal fd +/// into every command spawned while the task runs — a child could inject +/// input into or capture output from another task's terminal, and hold the +/// master open past the task's teardown. portable-pty's own descriptors are +/// cloexec'd; this pin holds the dup to the same invariant, and to the +/// same-description write semantics the writer relies on. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn pty_writer_dup_carries_close_on_exec() { + use std::io::{Read, Write}; + use std::os::fd::{AsRawFd, FromRawFd}; + + let mut fds = [0 as libc::c_int; 2]; + assert_eq!( + unsafe { libc::pipe2(fds.as_mut_ptr(), libc::O_CLOEXEC) }, + 0, + "pipe2() failed" + ); + let mut reader = unsafe { std::fs::File::from_raw_fd(fds[0]) }; + let mut writer = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + let dup_fd = dup_cloexec(writer.as_raw_fd()).expect("dup_cloexec"); + let flags = unsafe { libc::fcntl(dup_fd, libc::F_GETFD) }; + assert!(flags >= 0, "F_GETFD on the dup failed: {flags}"); + assert!( + flags & libc::FD_CLOEXEC != 0, + "the PTY writer dup must set FD_CLOEXEC (a bare libc::dup clears it): flags={flags}" + ); + + // Same open description as the source: bytes written through the dup + // surface on the reader end, and the task keeps writing after the + // original handle is gone (both write ends must close before EOF). + let mut dup_file = unsafe { std::fs::File::from_raw_fd(dup_fd) }; + dup_file + .write_all(b"cloexec-") + .and_then(|_| writer.write_all(b"dup")) + .expect("write through the dup and the original"); + drop(writer); + drop(dup_file); + let mut rest = Vec::new(); + reader.read_to_end(&mut rest).expect("read through the dup"); + assert_eq!(rest, b"cloexec-dup", "bytes written via the dup must land"); +} + +/// Behavioral half of `pty_writer_dup_carries_close_on_exec`: a command +/// spawned after the dup must not see the duplicated descriptor in its fd +/// table (Linux `/proc` pin; the cloexec assert above holds the same +/// invariant on every unix target). +#[cfg(all(target_os = "linux", not(target_env = "ohos")))] +#[test] +fn spawned_child_does_not_inherit_the_pty_writer_dup() { + use std::os::fd::{AsRawFd, FromRawFd}; + + let mut fds = [0 as libc::c_int; 2]; + assert_eq!( + unsafe { libc::pipe2(fds.as_mut_ptr(), libc::O_CLOEXEC) }, + 0, + "pipe2() failed" + ); + let reader = unsafe { std::fs::File::from_raw_fd(fds[0]) }; + let writer = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + let dup_fd = dup_cloexec(writer.as_raw_fd()).expect("dup_cloexec"); + let dup_file = unsafe { std::fs::File::from_raw_fd(dup_fd) }; + + // Identify OUR pipe by kernel inode, not by fd number: a parallel test + // may legally hold (and leak) unrelated fds with colliding numbers. + let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + assert_eq!( + unsafe { libc::fstat(writer.as_raw_fd(), &mut stat) }, + 0, + "fstat on the pipe write end failed" + ); + let pipe_target = format!("pipe:[{}]", stat.st_ino); + + let output = Command::new("sh") + .arg("-c") + .arg("for f in /proc/self/fd/*; do readlink \"$f\"; done") + .stdin(Stdio::null()) + .output() + .expect("spawn fd-listing child"); + let listing = String::from_utf8_lossy(&output.stdout); + assert!( + !listing.lines().any(|line| line.trim() == pipe_target), + "spawned child must not inherit the dup'd descriptor ({pipe_target}); its fd targets:\n{listing}" + ); + drop(dup_file); + drop(writer); + drop(reader); +} + +/// The lowercase tool's own description carries the same foreground stdin +/// contract: its legacy translation accepts only command/timeout/sandbox +/// fields, so that sentence is the whole stdin story on that surface. +#[test] +fn lowercase_bash_description_states_the_foreground_stdin_contract() { + let description = LowercaseBashTool.description(); + assert!( + description.contains("This foreground surface runs without a terminal stdin"), + "the lowercase description must state the bounded foreground stdin: {description}" + ); + assert!( + description.contains("a bare `$SHELL` receive end-of-file"), + "the bare-shell probe must be named as exiting on EOF: {description}" + ); +} +/// When the inline payload cannot be delivered at spawn, the run must abort +/// with the run-never-started context: no task id exists yet, so the child +/// is killed and the error must say the mentioned task id was never +/// registered — the caller must not try to cancel or kill it. The +/// manager-level lane has no inline cap, so an over-pipe-buffer payload +/// against a non-reading child reaches the bounded writer's deadline here; +/// the tool surface cannot produce this stall on Linux (its 64 KiB cap +/// equals the default pipe buffer), which is why this pin drives the +/// manager API directly. +#[test] +#[cfg(unix)] +fn spawn_aborts_with_run_never_started_context_when_the_payload_stalls() { + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + // 2 MiB: far past any default pipe buffer, so the bounded writer's + // deadline is what ends the write. + let payload = "x".repeat(2 * 1024 * 1024); + + let started = Instant::now(); + let err = manager + .execute_with_options_env( + &sleep_command(60), + None, + 60_000, + true, + Some(payload.as_str()), + false, + None, + HashMap::new(), + ) + .expect_err("the spawn must abort when the inline payload stalls"); + let elapsed = started.elapsed(); + + let text = format!("{err:#}"); + assert!( + text.contains("the run was killed at spawn"), + "error must carry the killed-at-spawn context (the process WAS created, \ + so the copy must not claim the run never started); got: {text}" + ); + assert!( + text.contains("the command was killed before it could do any work"), + "error must state the process was created and then killed; got: {text}" + ); + assert!( + text.contains("never registered"), + "error must tell the caller the task id was never registered; got: {text}" + ); + assert!( + !text.contains("Kill the task"), + "the task-targeted recovery copy must not appear on the spawn-abort \ + path; got: {text}" + ); + assert!( + elapsed >= STDIN_WRITE_DEADLINE, + "the abort must come from the bounded writer's deadline, not an \ + instant failure; took {elapsed:?}: {text}" + ); +} #[cfg(unix)] #[test] fn readonly_operand_gate_refuses_cd_operands_the_shell_reinterprets() { @@ -10700,9 +11014,9 @@ async fn foreground_terminal_command_keeps_its_interact_surface() { #[test] #[cfg(all(unix, not(target_env = "ohos")))] fn interact_write_to_a_stalled_tty_command_is_bounded() { - let (sender, receiver) = std::sync::mpsc::channel::>(); - let handle = std::thread::spawn(move || { - let run = || -> Result<(), String> { + let verdict = run_stdin_scenario_with_hang_guard( + "the bounded tty-write pin", + || -> Result<(), String> { let tmp = tempdir().map_err(|error| error.to_string())?; let mut manager = ShellManager::new(tmp.path().to_path_buf()); let result = manager @@ -10782,17 +11096,8 @@ fn interact_write_to_a_stalled_tty_command_is_bounded() { } manager.kill(&task_id).map_err(|error| error.to_string())?; Ok(()) - }; - let verdict = run(); - let _ = sender.send(verdict.clone()); - verdict - }); - let verdict = receiver - .recv_timeout(Duration::from_secs(60)) - .unwrap_or_else(|_| { - let _ = handle.join(); - Err("the bounded tty-write test hung past its 60 s guard".to_string()) - }); + }, + ); assert!(verdict.is_ok(), "{verdict:?}"); } @@ -11488,6 +11793,103 @@ fn readonly_compound_refusal_names_the_cmd_comment_wall() { } } +/// An explicit `close_stdin` on a terminal task whose last write already +/// STALLED must stay bounded, must not claim an end-of-file the abandoned +/// worker never delivered, and must not read as success either: the stall +/// skip refuses the close outright ("was not applied") and keeps the +/// interact surface attached, and the tail refusal still names the stall. +/// A mutation that reports the stalled close as Ok reddens on the +/// expect_err; a close that wedges on the full input queue trips the outer +/// guard instead. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof() { + let verdict = run_stdin_scenario_with_hang_guard( + "the stalled-terminal-close pin", + || -> Result<(), String> { + let tmp = tempdir().map_err(|error| error.to_string())?; + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + let result = manager + // Same shape as the stalled-write pin above: a raw-mode + // tty whose command never reads backpressures the master + // write, and the marker only prints after `stty` succeeds. + .execute_with_options_env( + "stty raw -echo 2>/dev/null && printf RAW-READY; sleep 60", + None, + 60_000, + true, + None, + true, + None, + HashMap::new(), + ) + .map_err(|error| error.to_string())?; + let task_id = result + .task_id + .ok_or_else(|| "background execution should return task_id".to_string())?; + let mut raw_ready = false; + for _ in 0..40 { + std::thread::sleep(Duration::from_millis(150)); + let snapshot = manager.inspect_job(&task_id).expect("inspect job").snapshot; + if snapshot.stdout_tail.contains("RAW-READY") { + raw_ready = true; + break; + } + } + assert!( + raw_ready, + "raw mode must be established before the stalled write: {:?}", + manager.inspect_job(&task_id).expect("inspect job").snapshot + ); + let payload = "x".repeat(200 * 1024); + let error = manager + .write_stdin(&task_id, &payload, false) + .err() + .ok_or_else(|| "the stalled tty write must error at the deadline".to_string())? + .to_string(); + if !error.contains("did not complete within") { + return Err(error); + } + // The close itself must return promptly (no delivery attempt + // on a stalled queue) and must NOT report success: with the + // end-of-file undelivered, an Ok close would tell the model + // stdin is closed when it is not. + let started = std::time::Instant::now(); + let close_error = manager + .write_stdin(&task_id, "", true) + .expect_err("a close on a stalled terminal task must not claim success") + .to_string(); + if started.elapsed() >= STDIN_WRITE_DEADLINE { + return Err(format!( + "the close on a stalled terminal task must not wait on the queue: {:?}", + started.elapsed() + )); + } + if !close_error.contains("was not applied") { + return Err(format!( + "the stalled-close refusal must say the close was not applied: {close_error}" + )); + } + // And any further stdin traffic must still be refused without + // claiming an EOF: no delivery happened, so "end-of-file" would + // be a lie (the refusal here comes from the write-stall gate — + // the refused close left the handle attached). + let after_close = manager + .write_stdin(&task_id, "y", false) + .err() + .ok_or_else(|| "the post-close write must be refused".to_string())? + .to_string(); + if after_close.contains("end-of-file") { + return Err(format!( + "a terminal close after a stall must not claim an EOF that never landed: {after_close}" + )); + } + manager.kill(&task_id).map_err(|error| error.to_string())?; + Ok(()) + }, + ); + assert!(verdict.is_ok(), "{verdict:?}"); +} /// The owned-writer bounded stdin write (the Windows/ohos fallback path): /// a consuming child gets the payload and the writer handed back; a child /// that never reads fills its pipe buffer and past the deadline the helper @@ -11507,7 +11909,8 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { .expect("cat spawns"); let stdin = child.stdin.take().expect("stdin piped"); let (returned, result) = - super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), b"hello\n".to_vec()); + super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), b"hello\n".to_vec()) + .expect("the bounded-write worker thread must spawn"); let Ok(result) = result else { panic!("a consuming child must complete the write, not lose the worker: {result:?}"); }; @@ -11526,7 +11929,8 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { let payload = vec![b'x'; 256 * 1024]; // well past the 64 KiB pipe buffer let started = std::time::Instant::now(); let (returned, result) = - super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), payload); + super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), payload) + .expect("the bounded-write worker thread must spawn"); let elapsed = started.elapsed(); assert!( matches!(result, Err(std::sync::mpsc::RecvTimeoutError::Timeout)), @@ -11544,6 +11948,247 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { let _ = child.wait(); } +/// `bounded_stdin_eof_delivery` — the worker the explicit terminal close +/// rides — must classify its outcome honestly: a write that lands reports +/// `Delivered`, a write into a FULL pipe (nothing consuming) reports +/// `Stalled` only at the deadline, and a write to a readerless pipe reports +/// `Failed`. A Stalled that came back as Delivered would let `write_stdin` +/// claim an end-of-file that never landed. The call is deadline-bounded by +/// construction, so a broken deadline hangs this test into the nextest +/// slow-timeout instead of passing. +// Unix PTY-lane cfg, matching the function it pins. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered() { + use std::os::fd::FromRawFd; + + // Delivered: /dev/null accepts the two bytes unconditionally. + let null = std::fs::OpenOptions::new() + .write(true) + .open("/dev/null") + .expect("open /dev/null"); + assert!( + matches!( + super::bounded_stdin_eof_delivery(&null), + super::BoundedEofDelivery::Delivered + ), + "a write that lands must report Delivered" + ); + + // Failed: a readerless pipe errors the write without delivering (the + // suite runs with SIG_IGN, so the EPIPE surfaces as an error, not a + // signal — the process-death discipline is pinned by the sigpipe guard + // helper test). + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + unsafe { libc::close(fds[0]) }; + let orphan = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + assert!( + matches!( + super::bounded_stdin_eof_delivery(&orphan), + super::BoundedEofDelivery::Failed + ), + "a readerless pipe must report Failed" + ); + drop(orphan); + + // Stalled: fill a pipe to capacity without a consumer (O_NONBLOCK so + // the test thread never blocks), restore blocking mode for the worker, + // and the two-byte delivery cannot land until the deadline. + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + let read_fd = fds[0]; + let full = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + unsafe { libc::fcntl(fds[1], libc::F_SETFL, libc::O_NONBLOCK) }; + let chunk = [b'x'; 4096]; + loop { + let written = unsafe { libc::write(fds[1], chunk.as_ptr().cast(), chunk.len()) }; + if written <= 0 { + break; // EAGAIN: the pipe is full + } + } + unsafe { libc::fcntl(fds[1], libc::F_SETFL, 0) }; + let started = std::time::Instant::now(); + let outcome = super::bounded_stdin_eof_delivery(&full); + drop(full); + assert!( + matches!(outcome, super::BoundedEofDelivery::Stalled), + "a full pipe must stall the delivery: {outcome:?}" + ); + assert!( + started.elapsed() >= super::STDIN_WRITE_DEADLINE, + "the stall must honor the deadline: {:?}", + started.elapsed() + ); + // Unblock the abandoned worker: a reader consumes the queued bytes, the + // worker's write lands, its send fails on the dropped receiver, and the + // thread exits instead of parking for the process lifetime. The drain + // is bounded so the reader cannot hang once the pipe ends are closed. + let drainer = std::thread::spawn(move || { + let mut buf = [0u8; 4096]; + let mut total = 0usize; + while total < 512 * 1024 { + let n = unsafe { libc::read(read_fd, buf.as_mut_ptr().cast(), buf.len()) }; + if n <= 0 { + break; + } + total += n as usize; + } + unsafe { libc::close(read_fd) }; + }); + drainer.join().expect("drain the stalled pipe"); +} + +/// The FRESH-stall close arm — a terminal close whose own ^D delivery +/// stalls with the stall flag still down (no prior write set it) — must +/// wait the bounded deadline, return the close refusal that cites it, +/// restore the handle under the stall flag, and refuse the follow-up write +/// without another deadline wait. A mutant reporting this arm as Ok would +/// read as the schema's delivered end-of-file while the bytes never +/// landed; the not-applied pin next door only reaches the SKIP arm (its +/// scenario stalls a write first), so this record-level pin is the only +/// guard on the arm itself. Drives `write_stdin` on a record whose stdin +/// is a `PtyFile` over a full pipe — exactly what a real PTY task hands +/// the arm. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn close_stdin_that_stalls_on_fresh_delivery_refuses_and_stays_attached() { + use std::os::fd::FromRawFd; + + // Fill a pipe to capacity without a consumer (O_NONBLOCK so the test + // thread never blocks), restore blocking mode for the worker: the + // two-byte ^D delivery cannot land until the deadline. + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + let read_fd = fds[0]; + let full = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + unsafe { libc::fcntl(fds[1], libc::F_SETFL, libc::O_NONBLOCK) }; + let chunk = [b'x'; 4096]; + loop { + let written = unsafe { libc::write(fds[1], chunk.as_ptr().cast(), chunk.len()) }; + if written <= 0 { + break; // EAGAIN: the pipe is full + } + } + unsafe { libc::fcntl(fds[1], libc::F_SETFL, 0) }; + + let now = std::time::Instant::now(); + let mut shell = BackgroundShell { + id: "fresh-stall-close".to_string(), + owner_session_id: "stdin-test-session".to_string(), + command: "cat".to_string(), + working_dir: std::path::PathBuf::from("."), + status: ShellStatus::Running, + exit_code: None, + started_at: now, + finished_at: None, + last_output_at: now, + last_observed_output_len: 0, + sandbox_type: SandboxType::None, + ownership: ShellOwnership::Managed, + linked_task_id: None, + owner_agent: None, + origin_tool_call_id: None, + origin_turn_id: None, + stdout_buffer: super::new_shared_raw_output(), + stderr_buffer: None, + bounded_output: None, + heavy_permit: None, + stdout_cursor: 0, + stderr_cursor: 0, + completion_reported: false, + stdin: Some(StdinWriter::PtyFile(full)), + stdin_closed_at_eof: false, + stdin_write_stalled: false, + child: None, + stop_unconfirmed: false, + stdout_thread: None, + stderr_thread: None, + work_lifecycle: None, + lifecycle_seq: 0, + last_lifecycle_status: None, + last_lifecycle_bytes: 0, + }; + + let started = std::time::Instant::now(); + let close_err = shell + .write_stdin("", true) + .expect_err("a close whose own delivery stalls must be refused, not reported Ok"); + assert!( + started.elapsed() >= super::STDIN_WRITE_DEADLINE, + "the fresh-stall close must attempt the bounded delivery first: {:?}", + started.elapsed() + ); + let message = format!("{close_err:#}"); + assert!( + message.starts_with("the stdin close for task fresh-stall-close"), + "the refusal must be the close refusal: {message}" + ); + assert!( + message.contains("did not complete within"), + "the fresh-stall refusal must cite the deadline: {message}" + ); + assert!( + !message.contains("no close can deliver it now"), + "a fresh stall must not take the not-applied skip arm: {message}" + ); + assert!( + !message.contains("stdin writes to task"), + "the close refusal must not be the write-stall gate: {message}" + ); + assert!( + shell.stdin.is_some(), + "the refused close must keep the interact surface attached" + ); + assert!( + shell.stdin_write_stalled, + "the fresh-stall close must mark the stall for later refusals" + ); + assert!( + !shell.stdin_closed_at_eof, + "a stalled delivery must not claim an end-of-file" + ); + + // The follow-up write is refused by the stall gate without another + // bounded wait. + let started = std::time::Instant::now(); + let write_err = shell + .write_stdin("x", false) + .expect_err("a write after a stalled close must be refused"); + assert!( + started.elapsed() < std::time::Duration::from_millis(500), + "the stall flag must refuse the follow-up write without another deadline wait: {:?}", + started.elapsed() + ); + let message = format!("{write_err:#}"); + assert!( + message.contains("stdin writes to task fresh-stall-close are refused"), + "the follow-up refusal must be the stall gate: {message}" + ); + + // Unblock the abandoned worker exactly like the classification pin: a + // bounded reader consumes the queued bytes, the worker's write lands, + // its send fails on the dropped receiver, and the thread exits. The + // record must drop first — it still holds the ORIGINAL pipe write end, + // and until every write end closes the drainer's read never sees EOF, + // so joining before the drop would hang the harness (the classification + // pin avoids this by dropping `full` before its call). + drop(shell); + let drainer = std::thread::spawn(move || { + let mut buf = [0u8; 4096]; + let mut total = 0usize; + while total < 512 * 1024 { + let n = unsafe { libc::read(read_fd, buf.as_mut_ptr().cast(), buf.len()) }; + if n <= 0 { + break; + } + total += n as usize; + } + unsafe { libc::close(read_fd) }; + }); + drainer.join().expect("drain the stalled pipe"); +} + /// An unconfirmed stop keeps its write scope across every coordination /// query — until a later poll reaps the real child and clears the /// uncertainty. The flag models the un-reaped state; the confirmation cycle @@ -11829,6 +12474,11 @@ fn bash_schema_stdin_max_length_matches_the_enforced_inline_cap() { } } +// The ^D close delivery is compiled only for the unix PTY lane (the same +// predicate as the `PtyFile` variant): Windows has no console EOF to +// deliver and ohos has no tty lane at all, so both the exit and the +// end-of-file refusal assertions below are meaningless there. +#[cfg(all(unix, not(target_env = "ohos")))] #[tokio::test] async fn interact_close_stdin_delivers_eof_to_a_terminal_command() { // Base delivered end-of-file to a terminal task through the ownership diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 3abcf7e7db..67c33376a9 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -36,8 +36,8 @@ use crate::core::engine::tool_catalog::{ TOOL_SEARCH_NAME, active_tools_for_request, apply_native_tool_deferral, bounded_toolbox_eviction_note, ensure_advanced_tooling, execute_tool_search_with_cache, initial_active_tools, is_tool_search_tool, park_toolbox_eviction, remove_toolbox_drops, - render_toolbox_eviction_notes, tool_matches_any_rule, toolbox_oversize_reason, - touch_cached_tool_after_execution, + render_toolbox_eviction_notes, rule_matches_tool, tool_matches_any_rule, + toolbox_oversize_reason, touch_cached_tool_after_execution, }; use crate::core::events::{AgentProgressEventMeta, Event}; use crate::core::session::{PendingToolboxEviction, ToolActivationCache, ToolboxEvictionTrigger}; @@ -404,7 +404,7 @@ fn subagent_perf_enabled() -> bool { const VALID_SUBAGENT_TYPES: &str = "general, explore, planner, reviewer, implement, test, advisor, custom \ (legacy aliases remain accepted: worker, scout, builder, verifier, consultant, general-purpose, general_purpose, default, exploration/explorer, plan/planning/awaiter, review/code-review/code_review, implementer/implementation, verify/verification/validator/tester, oracle)"; -const SUBAGENT_TYPE_DESCRIPTION: &str = "Fleet role for this delegated worker. general: full tool access for multi-step tasks; write-capable, so it claims the parent workspace unless write_roots narrows it — parallel generals without disjoint write_roots contend, so prefer a read-only role for research fan-outs. explore: fast read-only exploration. planner: grounded strategy with read-only probes. reviewer: reads and grades code. implement: lands focused code changes; write-capable under the same workspace-claim rules as general, so parallel implements need disjoint write_roots or worktree=true. test: runs tests/validation gates and reports evidence. advisor: read-only high-reasoning counsel for judgement calls and design critique. custom: the tools listed in allowed_tools on the parent's posture; without an explicit write authority it starts read-only and never contends. Legacy aliases remain accepted at deserialization boundaries."; +const SUBAGENT_TYPE_DESCRIPTION: &str = "Fleet role for this delegated worker. general: full tool access for multi-step tasks; write-capable, so it claims the parent workspace unless write_roots narrows it — parallel generals without disjoint write_roots contend, so prefer a read-only role for research fan-outs. explore: fast read-only exploration. planner: grounded strategy with read-only probes. reviewer: reads and grades code. implement: lands focused code changes; write-capable under the same workspace-claim rules as general, so parallel implements need disjoint write_roots or worktree=true. test: runs the built-in verification gates and reports evidence, with a bounded read-only shell — for in-session delegations — for gathering it; a read-only-shell parent clamps the gates (probes remain), and custom commands are refused. advisor: read-only high-reasoning counsel for judgement calls and design critique. custom: the tools listed in allowed_tools on the parent's posture; without an explicit write authority it starts read-only and never contends. Legacy aliases remain accepted at deserialization boundaries."; // === Types === @@ -9513,7 +9513,7 @@ impl ToolSpec for AgentTool { concat!( "Start with action=start and prompt; returns a turn-owned agent_id immediately. A prompt-only start (no type/profile) launches read-only; pass a write-capable type (e.g. implement) or declare write_roots before write work. Read-only roles need no extra fields. Set detached=true only for work that must remain independently observable after the turn. ", "Use multiple starts for independent parallel tasks. ", - "type selects the Fleet role: general (full tool access), explore (fast read-only exploration), planner (grounded strategy, read-only probes), reviewer (reads and grades code), implement (lands focused code changes), test (runs tests and reports evidence), advisor (read-only design counsel), or custom (allowed_tools on the parent's posture); legacy aliases are still accepted. ", + "type selects the Fleet role: general (full tool access), explore (fast read-only exploration), planner (grounded strategy, read-only probes), reviewer (reads and grades code), implement (lands focused code changes), test (built-in verification gates; custom commands refused), advisor (read-only design counsel), or custom (allowed_tools on the parent's posture); legacy aliases are still accepted. ", "profile runs the child as a named Fleet role or an exact prompt-only profile explicitly presented by the embedding host — pass it only when the task needs that identity. The child follows the operator's per-role model configuration, else the parent's model; model or thinking overrides are not advertised (parse-accepted for compatibility). ", "Use action=roster to inspect Fleet roles and host-presented prompt-only profiles before choosing a type or profile. The host-profile listing is capped and unpaged — search a large pool with profile_query. ", "Child run budgets come from Fleet role defaults and operator [subagents] config; budget fields are not advertised (parse-accepted for compatibility). ", @@ -10622,6 +10622,11 @@ async fn spawn_subagent_from_input( .worker_profile .denied_tools .retain(|rule| is_posture_denial(rule)); + // The dropped preferences were the inherited operator rules; their + // explicit-bookkeeping twins go with them, so the child's explicit + // set holds exactly the caller's list below. An automatic clamp rule + // keeps its enforcement regardless. + child_runtime.worker_profile.explicit_denied_tools.clear(); } if let Some(ref caller_deny) = spawn_request.disallowed_tools { for tool in caller_deny { @@ -10633,6 +10638,21 @@ async fn spawn_subagent_from_input( { child_runtime.worker_profile.denied_tools.push(tool.clone()); } + // Bookkeep the caller's provenance even when the rule string was + // already present (e.g. the role clamp installed the same + // `Bash` rule): the carve-outs override the clamp's automatic + // denial, never the caller's. + if !child_runtime + .worker_profile + .explicit_denied_tools + .iter() + .any(|existing| existing == tool) + { + child_runtime + .worker_profile + .explicit_denied_tools + .push(tool.clone()); + } } } apply_spawn_write_authority(&mut child_runtime, &spawn_request); @@ -15560,12 +15580,14 @@ impl SubAgentToolSurface { /// Role-only approval posture; the registry allow/deny and runtime envelope /// remain the authoritative per-tool checks. /// -/// `Auto` is safe for every role. `Suggest` requires a write-capable role; -/// `Required` requires the full-shell posture. A Custom worker may describe -/// either posture, but its explicit allowlist and the runtime envelope still -/// narrow what it can execute. Read-only bash operations are classified Auto, -/// so Scouts and Reviewers do not gain general shell authority through this -/// branch. +/// Only the `Auto` and `Suggest` answers remain live: the posture gate's +/// `Required` branch keys on the held shell policy plus the verifier's +/// recorded parent-shell grant (`holds_shell_authority`), no longer on this +/// role-only table. `Suggest` still requires a write-capable role. A Custom +/// worker may describe either posture, but its explicit allowlist and the +/// runtime envelope still narrow what it can execute. Read-only bash +/// operations are classified Auto, so the read-only roles do not gain +/// general shell authority through this branch. fn role_posture_permits(agent_type: &FleetRole, approval: ApprovalRequirement) -> bool { if matches!(agent_type, FleetRole::Custom) { return true; @@ -15630,12 +15652,26 @@ struct SubAgentToolRegistry { // parent/child scope. Deny rules always win, including canonical prefixes. allowed_tools: Option>, disallowed_tools: Vec, + // The operator/caller subset of `disallowed_tools` (session + // `--disallowed-tools` preferences plus the spawn request's explicit + // `disallowed_tools`). The strings overlap the role clamp's automatic + // rules, so provenance is carried: the bounded read-only bash lane + // overrides the clamp's automatic denial, never one of these. + operator_denied_tools: Vec, // Approval posture is separate from authority. Auto approval can remove a // prompt, but cannot restore a tool removed by role, scope, or envelope. auto_approve: bool, accept_edits: bool, agent_type: FleetRole, runtime_profile: WorkerRuntimeProfile, + /// Whether the child's requested shell was a full shell *after the parent + /// clamp but before the role's own narrowing* — i.e. the read-only shell + /// it now runs under came from its role's own narrowing, not from a + /// parent that itself held no shell authority. Keys the Verifier's + /// verification-surface carve-out in [`Self::holds_shell_authority`] so a + /// read-only role parent cannot obtain gate execution it never had by + /// delegating to `test`. + parent_shell_authority: bool, // Depth is the only special authority governing nested `agent` calls. can_spawn_child: bool, // Every mutation is attributed to the child and checked against its live @@ -15706,11 +15742,17 @@ impl SubAgentToolRegistry { let coordination_manager = Arc::clone(&runtime.manager); let mut surface_options = runtime.agent_tool_surface_options.clone(); // Shell authority is an intersection: a parent cannot delegate more - // than it owns, and read-only inspection roles (Scout, Reviewer) are - // narrowed to the hardened read-only classifier even when the parent - // has a full shell. + // than it owns, and read-only roles (Scout, Reviewer, Planner, + // Verifier) are narrowed to the hardened read-only classifier even + // when the parent has a full shell. let parent_shell = ShellPolicy::from_legacy_allow_shell(runtime.allow_shell); let mut child_shell = runtime.worker_profile.shell.min_with(parent_shell); + // Capture the grant before the role narrowing below: a Full shell + // here means the parent itself could run processes, so the role's + // read-only downgrade is the child's own posture. A clamped + // (read-only-parent) child must not regain authority through a + // role carve-out it merely shares the name of. + let parent_shell_authority = matches!(child_shell, ShellPolicy::Full); if crate::fleet::role::role_requires_read_only_shell(&agent_type) && child_shell.allows_shell() { @@ -15752,10 +15794,12 @@ impl SubAgentToolRegistry { Self { allowed_tools, disallowed_tools: effective_profile.denied_tools.clone(), + operator_denied_tools: effective_profile.explicit_denied_tools.clone(), auto_approve: runtime.context.auto_approve, accept_edits: runtime.accept_edits, agent_type, runtime_profile: effective_profile, + parent_shell_authority, can_spawn_child, owner_agent_id, owner_agent_name, @@ -15774,10 +15818,11 @@ impl SubAgentToolRegistry { /// clears the role-based delegation rules on its own. fn delegation_refusal(&self, name: &str, input: &Value) -> Option { // #5595/#5438: session approval is transport, not a second shell - // classifier. Scout/Reviewer/Planner calls already proven by the exact - // predicate used at posture, envelope, and execute boundaries are - // delegated reads even though lowercase `bash` deliberately keeps the - // stricter parent-parallel classifier as its generic approval baseline. + // classifier. Scout/Reviewer/Planner/Verifier calls already proven by + // the exact predicate used at posture, envelope, and execute + // boundaries are delegated reads even though lowercase `bash` + // deliberately keeps the stricter parent-parallel classifier as its + // generic approval baseline. if self.bounded_readonly_bash_evidence(name, input) { return None; } @@ -16373,8 +16418,18 @@ impl SubAgentToolRegistry { { return true; } - matches!(self.runtime_profile.shell, ShellPolicy::Full) - && role_posture_permits(&self.agent_type, ApprovalRequirement::Required) + // Canonical `bash` on a lane role is deliberately NOT + // covered by the authority check below: the classifier + // carve-out above is its only door, so a command the + // grammar refuses keeps the `[shell.readonly.command]` + // refusal instead of being waved through to the + // envelope. The check is case-aware because legacy + // `Bash` is the raw-shell alias — the carve-out is + // exact-name `bash` only, and the alias must not + // re-enter through this branch. + let bash_must_use_classifier_lane = name.eq_ignore_ascii_case("bash") + && self.allows_bounded_readonly_bash("bash"); + !bash_must_use_classifier_lane && self.holds_shell_authority() } }, None => true, @@ -16391,9 +16446,11 @@ impl SubAgentToolRegistry { /// Whether this child may surface and dispatch the canonical lowercase /// `bash` tool under the read-only inspection posture. /// - /// Scout, Reviewer, and Planner keep exactly one shell entry point — - /// canonical `bash` — whose concrete calls the strict read-only - /// classifier bounds. + /// Scout, Reviewer, Planner, and Verifier keep exactly one shell entry + /// point — canonical `bash` — whose concrete calls the strict read-only + /// classifier bounds. For the Verifier this is the evidence lane: running + /// the built-in gates is only half the role, and "pass/fail with + /// evidence" needs probes like `wc -c` the raw surface can never allow. /// Catalog visibility and dispatch authorization consult this same /// predicate, so a tool that appears on the wire can always be called and /// one that is denied never appears. @@ -16411,13 +16468,20 @@ impl SubAgentToolRegistry { /// case-insensitive compare would hand these roles back the raw shell this /// carve-out exists to withhold. Only the canonical lowercase tool the /// first-turn catalog actually offers is bounded by the classifier. + /// + /// The lane overrides exactly ONE denial source: the role clamp's + /// automatic raw-shell rules. An operator or caller who denied `bash` (or + /// any rule matching it — `exec_shell*`, a wildcard) by name keeps that + /// restriction authoritative at catalog admission and dispatch alike, so + /// any operator/caller rule matching `bash` disables the lane outright. fn allows_bounded_readonly_bash(&self, name: &str) -> bool { name == "bash" - && matches!( - &self.agent_type, - FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner - ) + && crate::fleet::role::role_requires_read_only_shell(&self.agent_type) && self.runtime_profile.shell != crate::worker_profile::ShellPolicy::None + && !self + .operator_denied_tools + .iter() + .any(|rule| rule_matches_tool(rule, name)) } fn legacy_action_alias(family: &str, action: &str) -> Option<&'static str> { @@ -16474,8 +16538,10 @@ impl SubAgentToolRegistry { fn role_blocks_unhardened_process_tool(&self, name: &str) -> bool { // Catalog filtering is defense in depth. Scout/Reviewer see only the - // hardened evidence profile; Verifier may receive bounded Run but not - // any raw or session-oriented shell path. Dispatch repeats this check. + // hardened evidence profile; Verifier may receive bounded Run and the + // same classifier-bounded canonical `bash` lane as the inspection + // roles, but not any raw or session-oriented shell path. Dispatch + // repeats this check. let lower = name.to_ascii_lowercase(); let evidence_tool = crate::tools::registry::readonly_evidence_tool_name(name) || self @@ -16485,14 +16551,19 @@ impl SubAgentToolRegistry { let bounded_inspection = matches!(&self.agent_type, FleetRole::Scout | FleetRole::Reviewer) && lower != "agent" && !evidence_tool; - let raw_shell = lower == "bash" - || lower.starts_with("exec_shell") + let raw_shell = lower.starts_with("exec_shell") || matches!( lower.as_str(), "exec_wait" | "exec_interact" | "task_shell_start" | "task_shell_wait" ) || lower.starts_with("terminal/"); - bounded_inspection || matches!(&self.agent_type, FleetRole::Verifier) && raw_shell + // For a verifier, canonical `bash` is hardened exactly when the + // bounded read-only lane admits it (`allows_bounded_readonly_bash`); + // every other raw-shell spelling — and legacy `Bash`, which fails the + // lane's exact-name check — stays unhardened and blocked. + let verifier_shell = matches!(&self.agent_type, FleetRole::Verifier) + && (raw_shell || (lower == "bash" && !self.allows_bounded_readonly_bash(name))); + bounded_inspection || verifier_shell } fn network_is_denied(&self) -> bool { @@ -16505,12 +16576,33 @@ impl SubAgentToolRegistry { !self.runtime_profile.permissions.write } + /// Shell *authority* — the right to start the processes the built-in + /// verification surface runs. A full runtime shell holds it. The one + /// narrowed exception is the Verifier whose *parent granted a full + /// shell* (`parent_shell_authority`): `role_requires_read_only_shell` + /// downgrades the Verifier's raw-command policy to the classifier-bounded + /// probe lane, and running the workspace's own gates is the role's entire + /// purpose, so that role-own downgrade must not take the gates away + /// (`reject_unbounded_verification` and the envelope still bound every + /// concrete call). The grant cannot travel through delegation: a Verifier + /// child of a read-only role parent was clamped by that parent, holds no + /// authority of its own, and stays refused — delegation moves work, never + /// authority. No other role regains authority through a narrowed shell: a + /// Builder or Custom child of a read-only parent inherits + /// `ShellPolicy::ReadOnly` and must not start the test runner its parent + /// cannot — keying this on the role's *default* posture would hand + /// exactly that to every role whose default shell is full. + fn holds_shell_authority(&self) -> bool { + matches!(self.runtime_profile.shell, ShellPolicy::Full) + || (self.parent_shell_authority + && matches!(self.agent_type, FleetRole::Verifier) + && !matches!(self.runtime_profile.shell, ShellPolicy::None)) + } + fn shell_is_denied(&self) -> bool { - // Likewise, shell requires both a Full profile and no explicit shell - // sentinel. Read-only evidence commands are Auto-classified exceptions, - // not a Full-shell grant. - !matches!(self.runtime_profile.shell, ShellPolicy::Full) - || self.is_tool_denied(SHELL_AUTHORITY_SENTINEL) + // Read-only evidence commands are Auto-classified exceptions, not a + // shell-authority grant; the explicit shell sentinel always wins. + !self.holds_shell_authority() || self.is_tool_denied(SHELL_AUTHORITY_SENTINEL) } fn execution_envelope(&self) -> crate::tools::execution_envelope::ExecutionEnvelope { @@ -16609,10 +16701,9 @@ impl SubAgentToolRegistry { // Visibility and dispatch consult the same capability guard. These // representative calls let a read-only bash schema survive catalog // shaping without treating an empty input as arbitrary shell authority. - if !matches!( - &self.agent_type, - FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner - ) { + // The role set is the one `allows_bounded_readonly_bash` admits: the + // read-only-shell roles, including the verifier's evidence lane. + if !crate::fleet::role::role_requires_read_only_shell(&self.agent_type) { return None; } match name { @@ -17830,7 +17921,8 @@ const CONSULTANT_AGENT_INTRO: &str = concat!( ); const VERIFIER_AGENT_INTRO: &str = concat!( - "You are a trusted Fleet test agent (role: `test`). Your job is to run the requested gates with your bounded validation tools — the allowed test/check selections — and report results. You never write: patching the workspace is denied. Unbounded shell forms are refused; use the verification surface.\n", + "You are a trusted Fleet test agent (role: `test`). Your job is to run the requested gates with your bounded validation tools — the allowed test/check selections — and report results; the gates are yours on envelopes whose parent grants shell authority, while a child clamped by a read-only-shell parent keeps the read-only probes but has its gates refused. You never write: patching the workspace is denied. Unbounded shell forms are refused; use the verification surface.\n", + "When the session grants it — for in-session delegations, on envelopes that carry the probe lane — you also hold a bounded read-only shell, under the same classifier-gated grammar as the read-only roles, for gathering evidence with probes such as `wc`, `ls`, `cat`, and `grep` pipelines; envelopes without it omit the shell entirely and shell attempts refuse, and custom commands are refused.\n", "Report PASS/FAIL/FLAKY at the top of SUMMARY with exact command evidence.\n", "Capture failing assertion and file:line; put obvious fixes under RISKS.\n", "You may use more tool calls than quick exploration, but stop after decisive pass/fail evidence.\n", diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 659e8d9bab..f9bcff5ef7 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -9,6 +9,7 @@ use std::process::Command; use std::sync::atomic::{AtomicUsize, Ordering}; use tempfile::{Builder as TempDirBuilder, tempdir}; mod identity_selection; + mod launch_receipt; mod operation_identity; fn built_in_whale_name_that_cannot_be_generated_for(agent_id: &str) -> &'static str { @@ -2863,7 +2864,7 @@ fn agent_description_explains_background_child_and_transcript_handle() { "planner (grounded strategy", "reviewer (reads and grades code)", "implement (lands focused code changes)", - "test (runs tests and reports evidence)", + "test (built-in verification gates; custom commands refused)", "advisor (read-only design counsel)", "custom (allowed_tools", "legacy aliases are still accepted", @@ -3421,148 +3422,6 @@ fn declared_write_scope_is_normalized_and_rejects_traversal() { "{err}" ); } -/// A scoped child that spells its target as an absolute path inside its own -/// workspace is writing the same file the relative spelling names (live -/// session, 2026-10-24: `write` of `/fixture/calc/inside.txt` -/// was refused as "must be repo-relative" while `fixture/calc/inside.txt` -/// succeeded). Absolute paths outside the workspace and `..` traversal stay -/// refused. -#[test] -fn scoped_mutation_paths_accept_absolute_targets_inside_the_workspace() { - let tmp = tempdir().expect("tempdir"); - let workspace = tmp.path().join("workspace"); - std::fs::create_dir_all(workspace.join("fixture/calc")).expect("workspace tree"); - let canonical = workspace.canonicalize().expect("canonical workspace"); - - // Existing target, canonical spelling. - std::fs::write(workspace.join("fixture/calc/existing.txt"), "x").expect("seed"); - let paths = mutation_paths( - "write", - &json!({ "path": canonical.join("fixture/calc/existing.txt").to_string_lossy() }), - &workspace, - ) - .expect("absolute in-workspace path is admitted"); - assert_eq!(paths, vec!["fixture/calc/existing.txt".to_string()]); - - // Not-yet-existing target (the write creates it), spelled through the - // un-canonicalized workspace path. - let paths = mutation_paths( - "write", - &json!({ "path": workspace.join("fixture/calc/new.txt").to_string_lossy() }), - &workspace, - ) - .expect("absolute path to a file the write will create is admitted"); - assert_eq!(paths, vec!["fixture/calc/new.txt".to_string()]); - - // Relative spelling is unchanged. - let paths = mutation_paths( - "write", - &json!({ "path": "fixture/calc/new.txt" }), - &workspace, - ) - .expect("relative path"); - assert_eq!(paths, vec!["fixture/calc/new.txt".to_string()]); - - // Outside the workspace: refused as before. - let err = mutation_paths( - "write", - &json!({ "path": tmp.path().join("escape.txt").to_string_lossy() }), - &workspace, - ) - .expect_err("absolute path outside the workspace is refused") - .to_string(); - assert!(err.contains("repo-relative"), "{err}"); - - // Traversal: refused as before, relative or absolute. - for path in ["../escape.txt", "fixture/../../escape.txt"] { - let err = mutation_paths("write", &json!({ "path": path }), &workspace) - .expect_err("traversal is refused") - .to_string(); - assert!(err.contains("traversal"), "{path}: {err}"); - } - let err = mutation_paths( - "write", - &json!({ "path": workspace.join("fixture/../../escape.txt").to_string_lossy() }), - &workspace, - ) - .expect_err("absolute traversal out of the workspace is refused") - .to_string(); - assert!(err.contains("repo-relative"), "{err}"); - - // Whitespace-padded spellings are judged EXACTLY as the write tool - // resolves them — the raw string, never trimmed. A padded absolute is - // NOT absolute to the tool (it joins onto the workspace as - // `/␣␣//…`), so the claim lane validates that literal - // space-led spelling, never the trimmed absolute target the tool does - // not touch. (The pre-round-4 trim made this spelling validate as - // `fixture/calc/padded.txt` while the tool wrote the space-led path.) - let padded_relative_target = "fixture/calc/padded.txt"; - let padded_absolute = format!( - " {}", - canonical.join(padded_relative_target).to_string_lossy() - ); - let paths = mutation_paths("write", &json!({ "path": padded_absolute }), &workspace) - .expect("a padded spelling still normalizes"); - // The expected is the ledger's canonical frame of the raw spelling, built - // here independently of the code under test: backslashes fold to forward - // slashes and leading separators collapse into the single one the - // component join emits (a Windows verbatim `\\?\` prefix reduces to its - // literal `?` component), while the space-led first component survives - // verbatim — trimming would instead validate the unpadded absolute target - // the tool never touches. - let ledger_frame = canonical - .join(padded_relative_target) - .to_string_lossy() - .replace('\\', "/"); - assert_eq!( - paths, - vec![format!(" /{}", ledger_frame.trim_start_matches('/'))], - "the claim lane judges the raw spelling in the forward-slash ledger frame: \ - the padded path is not absolute, so it validates as the literal \ - space-led components the tool would join onto the workspace" - ); - - // A padded relative spelling keeps its literal form for the same - // reason: trimming here would validate one path and write another. - let paths = mutation_paths( - "write", - &json!({ "path": "fixture/calc/ tail.txt " }), - &workspace, - ) - .expect("a padded relative spelling still normalizes"); - assert_eq!(paths, vec!["fixture/calc/ tail.txt ".to_string()]); - - // Whitespace-only spellings refuse outright instead of normalizing to - // "." — the whole-workspace scope no write input ever meant. - for path in [" ", "\t"] { - let err = mutation_paths("write", &json!({ "path": path }), &workspace) - .expect_err("a whitespace-only path is refused") - .to_string(); - assert!( - err.contains("non-empty repo-relative path"), - "{path}: {err}" - ); - } -} - -/// The ledger frame's separator contract, pinned cross-platform: a -/// Windows-spelled relative path folds to forward slashes and a padded -/// spelling keeps its space-led first component. This is the contract the -/// padded-path assertion above relies on for native-Windows equality, where -/// a verbatim canonical path would otherwise never compare equal to the -/// raw backslash spelling. -#[test] -fn normalize_claim_path_folds_backslashes_into_the_ledger_frame() { - assert_eq!( - normalize_claim_path(r"fixture\calc\tail.txt").expect("windows-spelled relative path"), - "fixture/calc/tail.txt" - ); - assert_eq!( - normalize_claim_path(" fixture\\calc\\tail.txt").expect("padded windows spelling"), - " fixture/calc/tail.txt" - ); -} - #[test] fn shared_child_cwd_claims_use_one_root_namespace_for_collisions_and_mutations() { let repo = tempdir().expect("repo"); @@ -7154,6 +7013,7 @@ fn every_named_role_has_one_complete_capability_based_surface() { "Web", "agent", "automation", + "bash", "diagnostics", "file_search", "finance", @@ -8217,6 +8077,578 @@ async fn read_only_inspection_roles_cannot_write_through_text_filters() { // bounded read-only probes the inspection roles use — so "with evidence" // includes evidence the child gathered itself (`wc`, `ls`, `cat`, `grep`) — // while custom commands stay refused and the workspace stays read-only. +/// The roster description must promise exactly the surface the role has: +/// built-in gates plus a bounded read-only evidence shell, no custom commands. +#[test] +fn verifier_roster_description_states_the_bounded_contract() { + let description = FleetRole::Verifier.description(); + assert!( + description.contains("built-in verification gates"), + "roster description must name the built-in gates: {description}" + ); + assert!( + description.contains("bounded read-only shell"), + "roster description must promise the bounded evidence shell: {description}" + ); + assert!( + description.contains("cannot execute custom commands"), + "roster description must refuse custom commands: {description}" + ); + assert!( + description.contains("unless a read-only-shell parent clamped its shell authority"), + "roster description must condition the gates on shell authority — a clamped child keeps probes and loses gates: {description}" + ); + // The Agent-tool `type` description carries the same truth, briefly — + // including the two clauses the model needs before picking: the gates + // clamp under a read-only-shell parent, and custom commands are refused. + assert!( + SUBAGENT_TYPE_DESCRIPTION.contains("test: runs the built-in verification gates"), + "type description must match the roster: {SUBAGENT_TYPE_DESCRIPTION}" + ); + assert!( + SUBAGENT_TYPE_DESCRIPTION.contains("custom commands are refused"), + "type description must refuse custom commands: {SUBAGENT_TYPE_DESCRIPTION}" + ); + assert!( + SUBAGENT_TYPE_DESCRIPTION + .contains("a read-only-shell parent clamps the gates (probes remain)"), + "type description must condition the gates on the parent shell: {SUBAGENT_TYPE_DESCRIPTION}" + ); + // The fleet roster slot blurb is the third roster surface a model reads + // (built-in members feed `AgentProfile.description`); it must not + // advertise builds or a shell the fleet envelope never grants. + let fleet_blurb = FleetRoster::built_in_members() + .into_iter() + .find(|member| member.id == "verifier") + .unwrap_or_else(|| panic!("built-in roster must carry the verifier slot")) + .description + .unwrap_or_else(|| panic!("built-in verifier slot must carry a description")); + assert!( + fleet_blurb.contains("built-in verification gates"), + "fleet roster blurb must name the built-in gates: {fleet_blurb}" + ); + assert!( + fleet_blurb.contains("holds no shell"), + "fleet roster blurb must disclaim the shell the fleet envelope refuses: {fleet_blurb}" + ); + assert!( + !fleet_blurb.contains("builds and tests"), + "fleet roster blurb must not re-overclaim builds: {fleet_blurb}" + ); +} +/// Acceptance for the verifier lane, sibling of +/// `read_only_inspection_roles_execute_pwd_and_absolute_git_log`: a live +/// `test` child must execute a bounded read-only probe through canonical +/// `bash` — `wc -c` on a fixture — with the raw-shell deny list still +/// installed, reaching the lane *through* it via +/// `allows_bounded_readonly_bash`. +#[cfg_attr(not(unix), ignore = "wc and unix paths")] +#[tokio::test] +async fn verifier_child_executes_a_bounded_readonly_bash_probe_for_evidence() { + let (tmp, registry) = read_only_with_shell_registry(); + let fixture = tmp.path().join("evidence.txt"); + std::fs::write(&fixture, "countable\n").expect("fixture"); + + let command = format!("wc -c {}", fixture.display()); + assert!( + !registry.role_blocks_unhardened_process_tool("bash"), + "canonical bash must reach the bounded lane, not the unhardened wall" + ); + let output = registry + .execute( + "agent_verifier_evidence", + "bash", + json!({ "command": command }), + ) + .await + .unwrap_or_else(|error| panic!("verifier probe must run `{command}`: {error}")); + // The count is wc's FIRST whitespace-separated field (`10 /path`), not a + // substring: `contains("10")` would also match line noise like `210`. + let count = output.split_ascii_whitespace().next().unwrap_or_default(); + assert_eq!( + count, "10", + "verifier probe must report the fixture's byte count as wc's first field: {output}" + ); +} +/// The walls around the lane stay up: the legacy raw `Bash` spelling, a +/// custom `Run` command, and a workspace write are all refused for a +/// verifier — at dispatch, and with the `reject_unbounded_verification` +/// contract text for the custom command. +#[tokio::test] +async fn verifier_child_still_refuses_raw_shell_custom_commands_and_writes() { + let (_tmp, registry) = read_only_with_shell_registry(); + + let raw = registry + .execute( + "agent_verifier_walls", + "Bash", + json!({"action": "run", "command": "rm -rf src"}), + ) + .await + .expect_err("legacy raw Bash must stay refused for a verifier"); + let raw = raw.to_string(); + assert!( + raw.contains("not available") || raw.contains("not allowed"), + "raw Bash refusal did not identify the wall: {raw}" + ); + // The second legacy-`Bash` wall is the posture's classifier-lane rule: a + // command the classifier cannot prove read-only stays `Required`, the + // case-insensitive lane-door check refuses the alias name itself, and + // the call is refused at posture — pinned here on this Verifier-labeled + // registry (the sibling pin covers the same wall through a Scout + // fixture). A *proven*-read-only shape clears posture's Auto + // classification for any role, so the alias-at-any-provability refusal + // is pinned below at the wall that actually owns it: the role clamp. + assert!( + !registry.posture_permits_tool("Bash", Some(&json!({"command": "rm -rf src"}))), + "the posture wall must refuse unprovable commands through the legacy `Bash` alias for a Verifier" + ); + // The exact-name asymmetry at the dispatch wall: only canonical + // lowercase `bash` reaches the lane, so the alias is refused even for a + // provably read-only shape — a case-relaxed lane check cannot re-admit + // it at any provability. + registry + .execute( + "agent_verifier_walls", + "Bash", + json!({"command": "pwd"}), + ) + .await + .expect_err("the legacy `Bash` alias must stay refused even for a provably read-only command — the lane is exact-name"); + + let custom_command = json!({ + "action": "verifiers", + "commands": [{"name": "x", "program": "bash", "args": ["-lc", "id"]}] + }); + // The contract text, at the guard that owns it: a custom command is the + // unbounded form, refused for a write-denied child with or without shell. + assert!( + super::reject_unbounded_verification("Run", &custom_command, true).is_err() + && super::reject_unbounded_verification("Run", &custom_command, false).is_err(), + "custom Run commands must be refused by the unbounded-verification guard" + ); + // …and the refusal reaches the real dispatch path too, from one of the + // two walls that own it — the approval gate (a Required call with no + // session decision) or the unbounded-verification guard — rather than + // failing for some unrelated reason. + let custom = registry + .execute("agent_verifier_walls", "Run", custom_command) + .await + .expect_err("custom Run commands are refused by design"); + let custom = custom.to_string(); + assert!( + custom.contains("requires approval") || custom.contains("operator-supplied commands"), + "custom command refusal must come from the approval or unbounded wall: {custom}" + ); + + let write = registry + .execute( + "agent_verifier_walls", + "File", + json!({"action": "write", "path": "a.rs", "content": "b"}), + ) + .await + .expect_err("a verifier must not mutate the workspace"); + assert!( + write.to_string().contains("not allowed") || write.to_string().contains("read-only"), + "workspace-write refusal did not identify the denial: {write}" + ); +} +/// The bounded read-only bash lane overrides exactly ONE denial source: the +/// role clamp's AUTOMATIC raw-shell rules. An operator (session +/// `--disallowed-tools`) or spawn caller who denies `bash` — by exact name +/// or by a rule that matches it (`ba*`, `exec_shell*`) — keeps that +/// restriction authoritative at BOTH boundaries the lane serves: the tool +/// leaves the model-visible catalog, and a direct dispatch is refused by the +/// name gate instead of being judged by the lane's read-only classifier. +/// The strings of an operator rule and the clamp's rule are identical +/// (`Bash`), so the provenance must be carried (`explicit_denied_tools`), +/// not re-derived — this pin holds the registry side of that contract. +#[tokio::test] +async fn verifier_explicit_bash_deny_rules_close_the_lane_at_admission_and_dispatch() { + for rule in ["bash", "ba*", "exec_shell*"] { + let tmp = tempdir().expect("tempdir"); + let authority = crate::fleet::role::ChildAuthority::from_runtime_role( + "verifier", + codewhale_workflow::PermissionCeiling::preset("full").expect("preset"), + ); + let mut runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.allow_shell = true; + runtime.worker_profile = WorkerRuntimeProfile::for_role(FleetRole::Verifier); + runtime.worker_profile.denied_tools = authority.disallowed_tools.clone(); + // What the spawn merge does with the caller's `disallowed_tools` + // entry: installs the rule AND books its operator/caller provenance. + runtime.worker_profile.denied_tools.push(rule.to_string()); + runtime + .worker_profile + .explicit_denied_tools + .push(rule.to_string()); + let registry = SubAgentToolRegistry::new( + runtime, + FleetRole::Verifier, + authority.allowed_tools.clone(), + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + // Catalog admission: the model must not even see the tool. + let names: Vec = registry + .tools_for_model(&FleetRole::Verifier) + .iter() + .map(|tool| tool.name.clone()) + .collect(); + assert!( + !names.iter().any(|name| name == "bash"), + "explicit operator rule `{rule}` must keep bash out of the model-visible catalog: {names:?}" + ); + + // Dispatch: a hand-written call is refused before the lane's + // classifier could ever judge the command. With the lane closed, + // canonical bash is unhardened for this role, so the unhardened- + // process wall speaks first; the name gate is its backstop. Either + // refusal proves the operator rule outranks the lane's classifier. + let refused = registry + .execute( + "agent_verifier_explicit_deny", + "bash", + json!({"command": "wc -c src/main.rs"}), + ) + .await + .expect_err("explicit operator rule `{rule}` must refuse bash dispatch"); + let refused = refused.to_string(); + assert!( + refused.contains("not available to this read-only worker") + || refused.contains("not allowed for this sub-agent"), + "explicit operator rule `{rule}` refusal must come from a dispatch wall: {refused}" + ); + } + + // Control: the lane still exists to override the clamp's AUTOMATIC + // raw-shell denial — with no operator rule matching bash, the canonical + // tool is admitted (the probe execution itself is pinned by + // `verifier_child_executes_a_bounded_readonly_bash_probe_for_evidence`). + let (_tmp, control) = read_only_with_shell_registry(); + assert!( + control.is_tool_allowed("bash"), + "the bounded lane must keep overriding the automatic raw-shell denial when no operator rule matches bash" + ); +} +/// A registry for a write-capable role whose parent handed it a read-only +/// shell and no writes — what `derive_child` produces for a Builder, Worker, +/// or Custom child of a Scout or Planner on the plain `agent` nesting path, +/// where no Fleet deny list is installed. +fn read_only_shell_child_registry(role: FleetRole) -> (tempfile::TempDir, SubAgentToolRegistry) { + let tmp = tempdir().expect("tempdir"); + let mut runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.allow_shell = true; + let mut profile = WorkerRuntimeProfile::for_role(role.clone()); + profile.permissions = crate::worker_profile::PermissionSet::read_only_with_network(); + profile.shell = crate::worker_profile::ShellPolicy::ReadOnly; + runtime.worker_profile = profile; + let registry = SubAgentToolRegistry::new( + runtime, + role, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + (tmp, registry) +} +/// Shell authority is the Verifier's carve-out alone. Every other role whose +/// runtime shell was narrowed to read-only — the Builder, Worker, or Custom +/// child of a read-only parent — holds no process authority: the built-in +/// test runner stays refused at posture and envelope and absent from the +/// catalog, exactly as before the verifier lane existed, so delegation +/// cannot widen a read-only parent into a child that runs the suite. +#[tokio::test] +async fn read_only_shell_children_of_other_roles_hold_no_shell_authority() { + let gate = json!({"action": "tests", "args": "--lib fleet::exact"}); + for role in [FleetRole::Builder, FleetRole::Worker, FleetRole::Custom] { + let (_tmp, registry) = read_only_shell_child_registry(role.clone()); + assert!( + !registry.posture_permits_tool("Run", Some(&gate)), + "{role:?} with a read-only shell must not hold the Required surface" + ); + assert!( + registry.envelope_refusal("Run", &gate).is_some(), + "{role:?} envelope must report no shell authority" + ); + let names: Vec = registry + .tools_for_model(&role) + .iter() + .map(|tool| tool.name.clone()) + .collect(); + assert!( + !names.iter().any(|name| name == "Run"), + "{role:?} catalog must not offer the verification gate: {names:?}" + ); + // The probe lane is read-only-roles-only: a widened role set must not + // silently hand the classifier-bounded shell to write-capable + // children. The catalog refusal keys on the same + // `allows_bounded_readonly_bash` predicate the production dispatch + // walls (the raw-shell deny rules' role-gated rescue) key on, so a + // widened role set flips both — and this catalog pin reddens first. + assert!( + !names.iter().any(|name| name == "bash"), + "{role:?} catalog must not offer the classifier probe lane: {names:?}" + ); + let refused = registry + .execute("agent_read_only_child", "Run", gate.clone()) + .await + .expect_err("the test runner must stay refused for a read-only-shell child"); + assert!( + refused.to_string().contains("not permitted") || refused.to_string().contains("shell"), + "{role:?}: {refused}" + ); + } + // The carve-out itself: the same narrowed shell on the Verifier keeps the + // bounded gates (pinned in detail by + // `verifier_child_keeps_the_default_and_selection_bounded_run_surface`). + let (_tmp, verifier) = read_only_with_shell_registry(); + assert!(verifier.posture_permits_tool("Run", Some(&gate))); + assert!(verifier.envelope_refusal("Run", &gate).is_none()); +} +/// The purpose survives the narrowing: default gates and test-selection +/// arguments stay admitted for the verifier at posture and envelope, and the +/// catalog still offers `Run`. +#[test] +fn verifier_child_keeps_the_default_and_selection_bounded_run_surface() { + let (_tmp, registry) = read_only_with_shell_registry(); + + for input in [ + json!({}), + json!({"action": "tests"}), + json!({"action": "tests", "args": "-p codewhale-tui"}), + json!({"action": "tests", "args": "--lib fleet::exact"}), + ] { + assert!( + registry.posture_permits_tool("Run", Some(&input)), + "verifier posture must admit the bounded gate: {input}" + ); + assert!( + registry.envelope_refusal("Run", &input).is_none(), + "verifier envelope must admit the bounded gate: {input}" + ); + } + let tools = registry.tools_for_model(&FleetRole::Verifier); + let names: Vec<&str> = tools.iter().map(|tool| tool.name.as_str()).collect(); + assert!( + names.contains(&"Run"), + "catalog must keep offering the verification gate: {names:?}" + ); + // Catalog and dispatch must agree on the carve-out: the probe lane stays + // visible, not merely dispatchable. + assert!( + names.contains(&"bash"), + "catalog must keep offering the probe lane beside the gates: {names:?}" + ); +} +/// A Verifier child of a read-only role parent (its shell was clamped by the +/// parent, not narrowed by the Verifier role) regains nothing through the +/// carve-out: the built-in gate runner stays refused at posture and envelope +/// and off the catalog, while the classifier-bounded bash evidence lane — +/// which the read-only parent holds too — keeps working. Delegation moves +/// work, never authority. +#[tokio::test] +async fn clamped_verifier_child_of_read_only_parent_holds_no_run_surface() { + let (_tmp, registry) = read_only_shell_child_registry(FleetRole::Verifier); + for input in [ + json!({}), + json!({"action": "tests"}), + json!({"action": "tests", "args": "-p codewhale-tui"}), + ] { + assert!( + !registry.posture_permits_tool("Run", Some(&input)), + "a clamped verifier must not run gates: {input}" + ); + assert!( + registry.envelope_refusal("Run", &input).is_some(), + "the envelope must refuse the gate for a clamped verifier: {input}" + ); + } + let tools = registry.tools_for_model(&FleetRole::Verifier); + let names: Vec<&str> = tools.iter().map(|tool| tool.name.as_str()).collect(); + assert!( + !names.contains(&"Run"), + "the clamped verifier catalog must not offer the gate: {names:?}" + ); + assert!( + names.contains(&"bash"), + "the clamped verifier keeps the shared bash evidence lane: {names:?}" + ); + assert!( + registry.posture_permits_tool("bash", Some(&json!({"command": "pwd"}))), + "the shared evidence lane admits read-only probes" + ); + assert!( + !registry.posture_permits_tool("bash", Some(&json!({"command": "rm -rf src"}))), + "the evidence lane stays classifier-bounded" + ); +} +/// The sub-subagent chain: a carve-out Verifier parent (its effective shell +/// is the role's read-only narrowing; the Run grant is a registry surface, +/// not shell authority) spawning a Builder child through the REAL spawn +/// path. Hop 2 is not hand-derived: the test starts the Builder through the +/// verifier's own `agent` tool and reads the child's recorded +/// `runtime_profile` — the profile `run_subagent` derived from the runtime +/// the registry published (`worker_profile_for_spawn` -> +/// `derive_child`). That recorded profile is the spawn ceiling the +/// carve-out parent actually published; a Builder built under it keeps the +/// no-Run, lane-less shape. This guards the propagation side of the +/// round-3 fix: if the registry ever leaked a pre-narrowing profile to its +/// spawns (`child_runtime.worker_profile` regressed to the parent-runtime +/// profile), the recorded shell below would come out `full` and this test +/// would redden. +#[tokio::test] +async fn sub_subagent_chain_of_a_carveout_verifier_holds_no_extra_authority() { + // A carve-out Verifier in a full session: the coarse session flag stays + // `allow_shell = true` while its effective runtime shell is the + // read-only narrowing. The ceiling its spawns publish must read the + // effective shell, not the flag. + let mut runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + runtime.allow_shell = true; + runtime.worker_profile.shell = crate::worker_profile::ShellPolicy::ReadOnly; + let ceiling = session_permission_ceiling(&runtime); + assert_eq!( + ceiling.shell, + codewhale_workflow::ShellCeiling::ReadOnly, + "the carve-out must not widen the shell ceiling a verifier publishes for its own spawns" + ); + // Hop 2 through the real spawn path: the write-capable `agent` start + // needs an auto-approved surface, so the fixture runs in its + // auto-approving variant. Everything else about the carve-out shape is + // the fixture's own. + let (verifier_tmp, verifier) = read_only_with_shell_registry_auto_approving(true); + assert_eq!( + verifier.runtime_profile.shell, + crate::worker_profile::ShellPolicy::ReadOnly, + "the carve-out fixture must narrow the verifier's own profile" + ); + let started = verifier + .execute( + "agent_chain_probe", + "agent", + json!({ + "action": "start", + "type": "builder", + "prompt": "noop chain probe", + }), + ) + .await + .expect("the carve-out verifier must be able to start a chain Builder"); + let started: Value = + serde_json::from_str(&started).expect("the agent start must answer with a JSON payload"); + let agent_id = started + .get("agent_id") + .and_then(Value::as_str) + .expect("the agent start must answer with an agent_id") + .to_string(); + // The record appears once the spawned task registers the child; the + // recorded profile is what production derived from the published + // runtime, not a field this test set. + let manager = verifier.gate_runtime.manager.clone(); + let record = tokio::time::timeout(Duration::from_secs(5), async { + loop { + if let Some(record) = manager.read().await.get_worker_record(&agent_id) + && record.spec.runtime_profile != WorkerRuntimeProfile::default() + { + return record; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + }) + .await + .expect("the chain Builder worker record must appear"); + assert_eq!( + record.spec.agent_type, + FleetRole::Builder, + "the spawned hop must be the requested Builder" + ); + assert_eq!( + record.spec.runtime_profile.shell, + crate::worker_profile::ShellPolicy::ReadOnly, + "the profile the registry published to its spawn must derive the read-only shell; \ + a pre-narrowing leak redds here" + ); + let child_tmp = tempfile::tempdir().expect("tempdir"); + let mut child_runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + child_runtime.context = ToolContext::new(child_tmp.path().to_path_buf()); + child_runtime.allow_shell = true; + child_runtime.worker_profile = record.spec.runtime_profile.clone(); + let builder = SubAgentToolRegistry::new( + child_runtime, + FleetRole::Builder, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + // No gate surface at any wall, and no probe lane: the Builder two hops + // into a shell-granting chain holds exactly the read-only shell its + // ancestors owned. + let gate = json!({"action": "tests", "args": "--lib fleet::exact"}); + assert!( + !builder.posture_permits_tool("Run", Some(&gate)), + "a chain Builder must not hold the verification gate" + ); + assert!( + builder.envelope_refusal("Run", &gate).is_some(), + "a chain Builder envelope must refuse the verification gate" + ); + let names: Vec = builder + .tools_for_model(&FleetRole::Builder) + .iter() + .map(|tool| tool.name.clone()) + .collect(); + assert!( + !names.iter().any(|name| name == "Run"), + "a chain Builder catalog must not offer the verification gate: {names:?}" + ); + assert!( + !names.iter().any(|name| name == "bash"), + "a chain Builder catalog must not offer the classifier probe lane: {names:?}" + ); + let refused = builder + .execute("agent_chain_builder", "Run", gate.clone()) + .await + .expect_err("the test runner must stay refused one hop below the carve-out verifier"); + assert!( + refused.to_string().contains("not permitted") || refused.to_string().contains("shell"), + "chain Builder refusal did not identify the wall: {refused}" + ); + drop(verifier_tmp); +} +/// The verifier intro tells the child about its bounded read-only shell and +/// the custom-command refusal, scoped to in-session delegations the way +/// `FleetRole`'s roster description scopes the role. The gates claim is +/// conditional too: a child clamped by a read-only-shell parent keeps the +/// probes and loses the gates — the intro must not promise the gates +/// unconditionally. +#[test] +fn verifier_intro_states_the_evidence_contract() { + assert!( + VERIFIER_AGENT_INTRO.contains("bounded read-only shell"), + "verifier intro must name the bounded read-only evidence shell: {VERIFIER_AGENT_INTRO}" + ); + assert!( + VERIFIER_AGENT_INTRO.contains("custom commands are refused"), + "verifier intro must repeat the custom-command refusal: {VERIFIER_AGENT_INTRO}" + ); + assert!( + VERIFIER_AGENT_INTRO.contains("in-session delegations"), + "the verifier's bounded read-only shell must be scoped to in-session delegations: {VERIFIER_AGENT_INTRO}" + ); + assert!( + VERIFIER_AGENT_INTRO + .contains("a child clamped by a read-only-shell parent keeps the read-only probes but has its gates refused"), + "the intro's gates claim must carry the read-only-parent conditional: {VERIFIER_AGENT_INTRO}" + ); +} #[test] fn explore_catalog_inherits_web_but_hides_write_shell_and_fim_tools() { let tmp = tempdir().expect("tempdir"); @@ -8597,17 +9029,31 @@ async fn scout_shell_respects_parent_shell_and_network_ceilings() { crate::tools::todo::new_shared_todo_list(), crate::tools::plan::new_shared_plan_state(), ); - assert!( - !tool_names(registry.tools_for_model(&role)).contains("bash"), - "{role:?} must not inherit the read-only inspection bash catalog" - ); - if role == FleetRole::Verifier { + if role == FleetRole::Consultant { assert!( - registry - .execute("agent_verifier", "bash", json!({"command": "pwd"})) - .await - .is_err(), - "Verifier keeps bounded Run, never arbitrary Bash" + !tool_names(registry.tools_for_model(&role)).contains("bash"), + "{role:?} must not inherit the read-only inspection bash catalog" + ); + } else { + // The verifier shares the inspection lane: canonical lowercase + // `bash` is advertised and classifier-bounded — evidence probes + // run, arbitrary shell keeps the bounded-lane refusal. + assert!( + tool_names(registry.tools_for_model(&role)).contains("bash"), + "verifier must advertise the bounded read-only bash lane" + ); + let probe = registry + .execute("agent_verifier", "bash", json!({"command": "pwd"})) + .await + .expect("the verifier's bounded read-only probe runs"); + assert!(!probe.trim().is_empty(), "pwd must return evidence"); + let arbitrary = registry + .execute("agent_verifier", "bash", json!({"command": "rm -rf src"})) + .await + .expect_err("arbitrary shell stays refused behind the lane"); + assert!( + arbitrary.to_string().contains("[shell.readonly.command]"), + "unexpected refusal: {arbitrary}" ); } } @@ -9047,12 +9493,13 @@ fn every_fleet_role_catalog_advertises_one_executable_load_skill() { "read-only role {role:?} keeps load_skill without gaining {denied}" ); } - // Scout/reviewer/planner expose only canonical lowercase bash, - // whose concrete calls are reclassified. Verifier keeps its - // bounded Run surface but not raw bash. + // Inspection roles and the verifier expose only canonical + // lowercase bash, whose concrete calls are reclassified by the + // read-only classifier (the verifier's evidence lane). Consultant + // is shell-less and gains no bash at all. if matches!( &role, - FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner + FleetRole::Scout | FleetRole::Reviewer | FleetRole::Planner | FleetRole::Verifier ) { assert!(names.contains("bash"), "{role:?} keeps read-only bash"); for denied in ["exec_shell", "task_shell_start"] { @@ -11549,71 +11996,6 @@ fn create_isolated_worktree_rejects_ambiguous_nested_repos() { "expected ambiguity diagnostic, got: {message}" ); } -/// Discovery may adopt a nested checkout only at the anchor itself. A session -/// workspace that is not a repository used to walk up to `$HOME` and scan -/// *its* children (live session, 2026-10-24: "Multiple git repositories found -/// under /Users/asto. Specify cwd to disambiguate: /Users/asto/flutter, -/// /Users/asto/Spec2017-iOS") — leaking unrelated project names and, with a -/// single sibling, silently adopting a stranger's repository. -/// -/// The anchor probe itself (`git rev-parse` from the workspace) walks -/// unboundedly, so this test assumes the tempdir is not inside a checkout -/// (the base no-repo test nests six levels to dodge the same assumption). -#[test] -fn worktree_discovery_never_adopts_or_enumerates_an_ancestors_sibling_repos() { - let home = tempdir().expect("home"); - for name in ["flutter", "Spec2017-iOS"] { - let sibling = home.path().join(name); - std::fs::create_dir_all(&sibling).expect("sibling dir"); - init_git_repo_at(&sibling); - } - // `/.pinvou3/sessions//workspace`: not a repository, with the - // sibling repositories four hops up — home is exactly the fourth (last) - // probed ancestor, and the Tried-list assertion below fails if a fixture - // change pushes it out of the probe window and turns this test vacuous. - let workspace = home - .path() - .join(".pinvou3") - .join("sessions") - .join("sid") - .join("workspace"); - std::fs::create_dir_all(&workspace).expect("workspace"); - let worktree_home = tempdir().expect("worktree home"); - let request = SubAgentWorktreeRequest { - branch: Some("codex/agent-session".to_string()), - path: Some(worktree_home.path().join("isolated")), - base_ref: None, - }; - - let message = create_isolated_worktree(&workspace, &request, None, &FleetRole::Worker) - .expect_err("a workspace outside any repository must not borrow an ancestor's sibling") - .to_string(); - assert!( - message.contains("requires a git repository"), - "expected the no-repository diagnostic, got: {message}" - ); - assert!( - !message.contains("Multiple git repositories") - && !message.contains("flutter") - && !message.contains("Spec2017-iOS"), - "ancestor siblings must not be scanned or named: {message}" - ); - assert!( - message.contains(home.path().to_string_lossy().as_ref()), - "the walk must have actually probed home (last ancestor in the window): {message}" - ); - - // A single sibling must not be adopted silently either. - std::fs::remove_dir_all(home.path().join("Spec2017-iOS")).expect("drop one sibling"); - let message = create_isolated_worktree(&workspace, &request, None, &FleetRole::Worker) - .expect_err("one sibling repository is still not the workspace's repository") - .to_string(); - assert!( - message.contains("requires a git repository"), - "expected the no-repository diagnostic, got: {message}" - ); -} - #[test] fn build_subagent_system_prompt_appends_role_when_set() { let assignment = SubAgentAssignment::new("p".to_string(), Some("worker".to_string())); @@ -13101,236 +13483,6 @@ async fn contended_shared_writer_refusal_names_blocking_peer_and_remediation() { "the refused command must not have run" ); } -/// The isolated lane carries the same contention premise as the shared one: -/// the same-worktree admission chain lets several unrelated writers share ONE -/// checkout on disjoint claims (overlapping same-root scopes are refused at -/// admission, so a live peer here always holds a disjoint share), and an -/// unbounded shell command cannot prove it stays inside the owner's share — -/// so a live same-worktree peer blocks it, naming that peer. The old -/// assumption "isolated means sole writer of the worktree" no longer holds -/// once a worktree parent can fan children out into its own checkout. -#[tokio::test] -async fn same_worktree_disjoint_sibling_blocks_unbounded_shell() { - let tmp = tempdir().expect("tempdir"); - let worktree = tmp.path().join("wt-blocked"); - std::fs::create_dir_all(worktree.join("a-dir")).expect("worktree layout"); - let canonical_worktree = worktree.canonicalize().expect("canonical worktree"); - let manager = new_shared_subagent_manager(tmp.path().to_path_buf(), 4); - { - let mut guard = manager.write().await; - assert_eq!(guard.insert_test_running_agent("a", &worktree), "agent_a"); - assert_eq!(guard.insert_test_running_agent("b", &worktree), "agent_b"); - let live: HashSet = ["agent_a".to_string(), "agent_b".to_string()] - .into_iter() - .collect(); - for (owner, root) in [("agent_a", "a-dir"), ("agent_b", "b-dir")] { - guard - .coordination - .register_claim( - WriteScopeClaim { - owner: owner.into(), - roots: vec![root.into()], - exact_files: vec![], - contracts: vec![], - }, - true, - Some(canonical_worktree.to_string_lossy().into_owned()), - &[], - |candidate| live.contains(candidate), - ) - .expect("disjoint same-worktree claims admit both live writers"); - } - } - let registry = contended_builder_registry(&manager, &worktree, "agent_a"); - let err = registry - .execute( - "agent_a", - "Bash", - json!({"action": "run", "command": "touch a-dir/unbounded.txt"}), - ) - .await - .expect_err("unbounded shell against a live same-worktree peer must be refused") - .to_string(); - assert!( - err.contains("cannot prove a bounded file target"), - "refusal kind: {err}" - ); - assert!( - err.contains("this same worktree") && err.contains("agent_b"), - "refusal must name the same-worktree blocking peer: {err}" - ); - assert!( - !worktree.join("a-dir/unbounded.txt").exists(), - "the refused command must not have run" - ); -} - -/// The ancestor chain keeps its unbounded shell inside the shared worktree: -/// admission deliberately admits a descendant overlapping its ancestors (one -/// delegated work unit), so the parent's live claim must not gate the child -/// the way an unrelated sibling does. Dropping the exemption turns this into -/// a contention refusal and a delegation chain could never run ordinary -/// shell work at all. -#[tokio::test] -async fn same_worktree_ancestor_chain_keeps_unbounded_shell() { - let tmp = tempdir().expect("tempdir"); - let coordination_root = tmp.path().join("workspace"); - let worktree = tmp - .path() - .join(".codewhale-worktrees") - .join("workspace") - .join("codex-agent-parent"); - std::fs::create_dir_all(worktree.join("c-dir")).expect("worktree layout"); - let manager = new_shared_subagent_manager(coordination_root, 4); - let claim = |root: &str| WriteScopeClaim { - owner: String::new(), - roots: vec![root.into()], - exact_files: vec![], - contracts: vec![], - }; - - let mut guard = manager.write().await; - let mut parent_runtime = stub_runtime(); - parent_runtime.manager = Arc::clone(&manager); - parent_runtime.context = ToolContext::new(&worktree); - let parent = guard - .spawn_background_with_assignment_options( - Arc::clone(&manager), - parent_runtime, - FleetRole::Builder, - "work in the worktree".into(), - make_assignment(), - Some(vec![]), - SubAgentSpawnOptions { - name: Some("shell-worktree-parent".into()), - write_claim: Some(claim(".")), - isolated_worktree: true, - ..Default::default() - }, - ) - .expect("worktree parent admitted"); - // Live parent: otherwise the peer never exists and the test passes with - // the exemption removed (the round-4 mutation lesson). - if let Some(agent) = guard.agents.get_mut(&parent.agent_id) { - agent.status = SubAgentStatus::Running; - } - let child = { - let mut child_runtime = stub_runtime(); - child_runtime.manager = Arc::clone(&manager); - child_runtime.context = ToolContext::new(&worktree); - child_runtime.parent_agent_id = Some(parent.agent_id.clone()); - guard - .spawn_background_with_assignment_options( - Arc::clone(&manager), - child_runtime, - FleetRole::Builder, - "run shell in the delegated share".into(), - make_assignment(), - Some(vec![]), - SubAgentSpawnOptions { - name: Some("shell-child".into()), - write_claim: Some(claim("c-dir")), - ..Default::default() - }, - ) - .expect("a disjoint descendant of the live worktree parent is admitted") - }; - drop(guard); - - let registry = contended_builder_registry(&manager, &worktree, &child.agent_id); - registry - .execute( - &child.agent_id, - "Bash", - json!({"action": "run", "command": "touch c-dir/chain.txt"}), - ) - .await - .unwrap_or_else(|err| { - panic!( - "the parent's live claim must not gate the delegated child's unbounded shell: {err}" - ) - }); - assert!( - worktree.join("c-dir/chain.txt").exists(), - "the admitted command must actually have run" - ); -} - -/// Liveness and the worktree boundary stay load-bearing in the isolated gate: -/// a settled sibling's claim cannot block (a terminal owner cannot write), and -/// a peer keyed on a DIFFERENT worktree never shared this checkout. -#[tokio::test] -async fn same_worktree_gate_ignores_settled_and_foreign_worktree_peers() { - let tmp = tempdir().expect("tempdir"); - let worktree = tmp.path().join("wt-live"); - let other_worktree = tmp.path().join("wt-other"); - std::fs::create_dir_all(worktree.join("a-dir")).expect("worktree layout"); - std::fs::create_dir_all(&other_worktree).expect("other worktree layout"); - let canonical_worktree = worktree.canonicalize().expect("canonical worktree"); - let canonical_other = other_worktree.canonicalize().expect("canonical other"); - let manager = new_shared_subagent_manager(tmp.path().to_path_buf(), 4); - { - let mut guard = manager.write().await; - assert_eq!(guard.insert_test_running_agent("a", &worktree), "agent_a"); - assert_eq!(guard.insert_test_running_agent("b", &worktree), "agent_b"); - assert_eq!(guard.insert_test_running_agent("c", &worktree), "agent_c"); - let live: HashSet = ["agent_a".to_string(), "agent_b".to_string()] - .into_iter() - .collect(); - for (owner, scope, root) in [ - ( - "agent_a", - "a-dir", - canonical_worktree.to_string_lossy().into_owned(), - ), - ( - "agent_b", - "b-dir", - canonical_worktree.to_string_lossy().into_owned(), - ), - ( - "agent_c", - ".", - canonical_other.to_string_lossy().into_owned(), - ), - ] { - guard - .coordination - .register_claim( - WriteScopeClaim { - owner: owner.into(), - roots: vec![scope.into()], - exact_files: vec![], - contracts: vec![], - }, - true, - Some(root), - &[], - |candidate| live.contains(candidate), - ) - .expect("same-worktree disjoint-root claims and a foreign-worktree claim admit"); - } - if let Some(agent) = guard.agents.get_mut("agent_b") { - agent.status = SubAgentStatus::Completed; - } - } - let registry = contended_builder_registry(&manager, &worktree, "agent_a"); - registry - .execute( - "agent_a", - "Bash", - json!({"action": "run", "command": "touch a-dir/kept.txt"}), - ) - .await - .unwrap_or_else(|err| { - panic!("a settled sibling and a foreign-worktree peer must not gate the shell: {err}") - }); - assert!( - worktree.join("a-dir/kept.txt").exists(), - "the admitted command must actually have run" - ); -} - /// A child whose task body panics must still reach a terminal state. /// `spawn_supervised` catches the panic to keep the parent alive; without the /// guard committing a crash result the child stayed `Running` and its write @@ -21044,6 +21196,15 @@ fn the_session_ceiling_reflects_the_live_parent_posture() { // capability it will be refused for using. /// The child registry a Runtime verifier under a full parent actually runs with. fn read_only_with_shell_registry() -> (tempfile::TempDir, SubAgentToolRegistry) { + read_only_with_shell_registry_auto_approving(false) +} + +/// The carve-out fixture with approvals auto-accepted, for tests that drive +/// the real spawn path (a write-capable `agent` start is otherwise held for +/// approval and never reaches `run_subagent`). +fn read_only_with_shell_registry_auto_approving( + auto_approve: bool, +) -> (tempfile::TempDir, SubAgentToolRegistry) { let tmp = tempdir().expect("tempdir"); let authority = crate::fleet::role::ChildAuthority::from_runtime_role( "verifier", @@ -21062,6 +21223,7 @@ fn read_only_with_shell_registry() -> (tempfile::TempDir, SubAgentToolRegistry) let mut runtime = stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.context.auto_approve = auto_approve; runtime.allow_shell = true; // The posture reaches the child through the Runtime authority envelope. runtime.worker_profile = WorkerRuntimeProfile::for_role(FleetRole::Verifier); @@ -21160,9 +21322,11 @@ fn a_verifier_runs_bounded_test_selections_and_nothing_else_at_dispatch() { } } /// Catalog and dispatch must agree: the verification surface the guard admits -/// is offered, and the raw shell it refuses is absent. +/// is offered, and the legacy raw-shell spellings it refuses are absent (the +/// verifier's shell surface is the bounded canonical `bash` tool pinned by +/// the read-only-shell tests, not the legacy `Bash`/`exec_shell` names). #[test] -fn the_verifier_catalog_offers_the_verification_surface_and_no_shell() { +fn the_verifier_catalog_offers_run_without_the_legacy_raw_shell_spellings() { let (_tmp, registry) = read_only_with_shell_registry(); let tools = registry.tools_for_model(&FleetRole::Verifier); let names: Vec<&str> = tools.iter().map(|tool| tool.name.as_str()).collect(); diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index 4abd1f3c40..ed686eab87 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -588,14 +588,13 @@ fn start_background_gates( .as_ref() .ok() .and_then(|result| result.task_id.as_deref()) + && let Err(error) = manager.write_stdin(task_id, "", true) { - if let Err(error) = manager.write_stdin(task_id, "", true) { - // Unreachable today (the job was just inserted under the - // held lock and the close writes nothing), but a silent - // `.ok()` here would quietly revert gates to idling on - // the open pipe until the background gate timeout. - tracing::warn!(%task_id, %error, "closing the gate job's stdin failed; the gate may idle until its timeout"); - } + // Unreachable today (the job was just inserted under the + // held lock and the close writes nothing), but a silent + // `.ok()` here would quietly revert gates to idling on + // the open pipe until the background gate timeout. + tracing::warn!(%task_id, %error, "closing the gate job's stdin failed; the gate may idle until its timeout"); } spawned }; diff --git a/crates/tui/src/worker_profile.rs b/crates/tui/src/worker_profile.rs index 9a3b8ab203..2bf9f28f75 100644 --- a/crates/tui/src/worker_profile.rs +++ b/crates/tui/src/worker_profile.rs @@ -162,6 +162,19 @@ pub struct WorkerRuntimeProfile { /// which clears the cloned runtime's list before the registry reads it. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub denied_tools: Vec, + /// The subset of `denied_tools` that an **operator or spawn caller asked + /// for by name** (the session's `--disallowed-tools` preferences and the + /// spawn request's explicit `disallowed_tools`), as opposed to the rules a + /// role clamp installs automatically (the raw-shell, network, and + /// verification-surface lists). The strings of the two sources overlap, so + /// the distinction must be carried, not re-derived: the bounded read-only + /// bash lane exists to override the clamp's automatic denial, and must + /// never override one of these. + /// + /// A child can only ever *add* entries — `derive_child()` unions this list + /// the same way it unions `denied_tools`. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub explicit_denied_tools: Vec, /// Remaining nested-delegation budget. A worker may spawn children while /// `max_spawn_depth > 0`; each level decrements it. Clamped to the workspace /// ceiling. @@ -205,10 +218,11 @@ impl WorkerRuntimeProfile { // the network or from shell for no role reason cannot do its job. let (permissions, shell) = match role { // Read-only investigators: no workspace writes, but network reach - // and the bounded verification surface so a scout/reviewer lane - // can run git/gh/web inspection. Raw shell stays denied by the - // registry clamp (read-only classifier), so this widens capability - // without widening mutation authority. + // so a scout/reviewer lane can run git/gh/web inspection. Raw + // shell stays denied by the registry clamp (read-only + // classifier), so this widens capability without widening + // mutation authority; the bounded verification surface is the + // Verifier's own arm below, not a scout/reviewer tool. FleetRole::Scout | FleetRole::Reviewer => { (PermissionSet::read_only_with_network(), ShellPolicy::Full) } @@ -249,6 +263,7 @@ impl WorkerRuntimeProfile { // wins via `derive_child`, same as every other role. reasoning_effort: matches!(role, FleetRole::Consultant).then(|| "high".to_string()), denied_tools: Vec::new(), + explicit_denied_tools: Vec::new(), max_spawn_depth: codewhale_config::DEFAULT_SPAWN_DEPTH, max_steps: Self::default_max_steps(role.clone()), background: true, @@ -281,6 +296,15 @@ impl WorkerRuntimeProfile { denied_tools.push(rule.clone()); } } + // Same union for the operator/caller subset: a descendant cannot drop + // an explicit restriction an ancestor carries, and only + // `inherit_disallowed_tools: false` may clear the inherited half. + let mut explicit_denied_tools = self.explicit_denied_tools.clone(); + for rule in &requested.explicit_denied_tools { + if !explicit_denied_tools.contains(rule) { + explicit_denied_tools.push(rule.clone()); + } + } let tools = match (&self.tools, &requested.tools) { // Parent restricts to a set → the child can only narrow within it. (ToolScope::Explicit(parent), ToolScope::Explicit(child)) => ToolScope::Explicit( @@ -314,6 +338,7 @@ impl WorkerRuntimeProfile { .clone() .or_else(|| self.reasoning_effort.clone()), denied_tools, + explicit_denied_tools, max_spawn_depth, max_steps: requested.max_steps, background: requested.background, @@ -614,6 +639,34 @@ mod tests { assert!(child.denied_tools.contains(&"write_file".to_string())); } + /// The operator/caller subset of the deny list unions the same way: a + /// descendant cannot drop an explicit restriction an ancestor carries. + /// The registry reads this list to keep role carve-outs (the bounded + /// read-only bash lane) from overriding operator rules whose strings are + /// identical to the clamp's own. + #[test] + fn child_explicit_denied_tools_union_never_drops_parent_rule() { + let mut parent = WorkerRuntimeProfile::for_role(FleetRole::Worker); + parent.explicit_denied_tools = vec!["bash".into()]; + let requested = WorkerRuntimeProfile::for_role(FleetRole::Builder); + + let child = parent.derive_child(&requested); + assert_eq!(child.explicit_denied_tools, vec!["bash".to_string()]); + + let mut parent_with_both = parent.clone(); + parent_with_both.explicit_denied_tools = vec!["bash".into(), "mcp_*".into()]; + let mut requesting_child = WorkerRuntimeProfile::for_role(FleetRole::Builder); + requesting_child.explicit_denied_tools = vec!["write_file".into()]; + let merged = parent_with_both.derive_child(&requesting_child); + for rule in ["bash", "mcp_*", "write_file"] { + assert!( + merged.explicit_denied_tools.contains(&rule.to_string()), + "explicit operator rule `{rule}` must survive derive_child: {:?}", + merged.explicit_denied_tools + ); + } + } + /// Every built-in role keeps network reads by default: a worker cut off /// from the network for no role reason cannot do its job. Only workspace /// mutation is a role intent. diff --git a/docs/GUIDE.md b/docs/GUIDE.md index 977a403970..cf8b470e6f 100644 --- a/docs/GUIDE.md +++ b/docs/GUIDE.md @@ -471,7 +471,7 @@ Useful roles include: | `plan` | Design and migration planning | | `review` | Bug-focused review of an existing change | | `implementer` | A tightly specified edit | -| `verifier` | Running checks and reporting pass/fail evidence | +| `verifier` | Running the built-in verification gates and reporting pass/fail evidence | Sub-agents are most useful when work can be separated cleanly. Do not use them for tiny edits, and do not ask multiple agents to write the same files at the diff --git a/docs/SUBAGENTS.md b/docs/SUBAGENTS.md index 506c0347d9..96b7ca006a 100644 --- a/docs/SUBAGENTS.md +++ b/docs/SUBAGENTS.md @@ -67,11 +67,11 @@ stewardship. | Role | Stance | Writes? | Network? | Shell posture | Typical use | |---------------|----------------------------------------|---------|----------|---------------|----------------------------------------------| | `general` | flexible; do whatever the parent says | yes | yes | yes | the default; multi-step tasks | -| `explore` | read-only; map the relevant code fast | no | yes | read-only (net + bounded verify) | "find every call site of `Foo`; check the PR with gh" | +| `explore` | read-only; map the relevant code fast | no | yes | read-only (net) | "find every call site of `Foo`; check the PR with gh" | | `planner` | analyse and produce a strategy | no | yes | read-only probes | "design the migration; don't execute" | -| `reviewer` | read-and-grade with severity scores | no | yes | read-only (net + bounded verify) | "audit this PR for bugs" | +| `reviewer` | read-and-grade with severity scores | no | yes | read-only (net) | "audit this PR for bugs" | | `implement` | land a specific change with min edit | yes | yes | yes | "rewrite `bar.rs::Foo::bar` to do X" | -| `test` | run tests / validation, report outcome | no | yes | bounded verification (no writes) | "verify the diff with the bounded test checks; report PASS/FAIL" | +| `test` | run the built-in verification gates and report outcome | no | yes | built-in gates unless parent-clamped (then probes only) + read-only probes (no custom commands) | "verify the diff with the built-in checks; report PASS/FAIL" | | `advisor` | short-lived, high-reasoning counsel | no | yes | none | "what are we missing in this design?" | | `custom` | explicit narrow tool allowlist | inherits | inherits | inherits | hand-picked tools on the parent's posture | @@ -90,12 +90,17 @@ builder) is a supported escape hatch for *work capacity* — the child brings its own model, route, and step budget — but the child's authority is clamped against the delegating parent's live posture, not the operator's: a scout's builder child lands read-only with raw shell and mutating tools denied, and -canonical `Bash` is denied to it too (only bounded-inspection roles keep the +canonical `Bash` is denied to it too (only the bounded-inspection roles and the verifier keep the classified read-only shell). Delegating to obtain shell is therefore mechanically useless — the scout's own bounded shell (`git -C … log`, `find … | head`, `npm view …`, classifier-gated) is the only shell path a -read-only parent has. Read-only is transitive through any delegation chain: -the clamp (`ChildAuthority::clamp` in `fleet/exact.rs`) intersects every +read-only parent has. The bounded grammar also admits compound probes of +those families — segments joined by `;`, `&&`, `||`, or `|`, where every +segment must individually be admitted (e.g. `wc -c f; wc -l f`, +`cd sub && wc -c notes.md`, `cat x | od -c`) — and a workspace-confined +`cd ` in any sequence position its connector always runs (joined by `;`, or first — a `cd` after `&&`/`||` may never execute, so it refuses). +Read-only is transitive through any delegation chain: +the clamp (`ChildAuthority::clamp` in `fleet/role.rs`) intersects every field with the narrower side, the deny-list union means a descendant can never drop an ancestor's restriction, and `inherit_disallowed_tools: false` cannot drop a posture denial (`is_posture_denial`). This is pinned by @@ -252,8 +257,8 @@ OUTPUT: VERDICT, EVIDENCE, GAPS, NEXT ``` `scout` briefs default to quick, read-only investigation (no writes, but -network reach and the bounded verification surface are available for real -scouting). About 3-5 tool calls +network reach and the classifier-gated read-only probe lane are available +for real scouting — no verification gates). About 3-5 tool calls is enough for quick exploration: orient, search, read the decisive lines, and return. Do not repeat `ALREADY_KNOWN` work unless evidence contradicts it. Review and verifier briefs can spend more calls, but should stop after decisive @@ -314,9 +319,20 @@ OUTPUT: VERDICT, EVIDENCE, GAPS, NEXT. - **`test`** — when the parent needs an authoritative pass/fail on the test suite or other validation. Verifiers don't fix failures; they capture the failing assertion + stack and put fix - candidates under RISKS. The verifier posture never writes, and shell - is clamped to the bounded built-in verification surface: the write - ceiling is read-only and unbounded shell forms are refused (#5186). + candidates under RISKS. The verifier posture never writes; its shell + is the bounded built-in verification surface — refused for an + in-session child only when a read-only-shell parent clamps its + authority (the clamped child keeps the probe lane and loses the + gates) — plus, on in-session delegations whose envelope grants it, + the same classifier-gated read-only probe lane the inspection roles + hold (`wc`, `ls`, `cat`, `grep` pipelines) for gathering evidence; + envelopes without the lane omit it and shell attempts refuse. On + Fleet members the envelope is stricter still: bounded verification + and Bash are mutually exclusive there, so a fleet verifier holds the + gates without the probe lane. The + write ceiling is read-only, unbounded shell forms are refused + (#5186), and custom commands are refused — use a write-capable role + for those. - **`advisor`** — when the operator wants a high-leverage second opinion before cheaper execution continues. Consultants read enough to ground a recommendation, but cannot write or run shell commands. `oracle` and diff --git a/docs/zh_hans/FLEET.md b/docs/zh_hans/FLEET.md index df9652850c..32b10f3214 100644 --- a/docs/zh_hans/FLEET.md +++ b/docs/zh_hans/FLEET.md @@ -121,7 +121,7 @@ call_reasrning = "lrw" 在运行时，它只能为已经冻结的 worker route 选择 reasoning 级别。它不能改变 worker、provider、model、role、tools 或 permissions。Router 调用本身被限制在 `off` 或 `low`；更贵的值会被拒绝。手动选择的 worker reasoning 级别不会产生 Router 调用。Route 与 reasoning 回执会指名 worker model，并在使用 Router 时给出 Router 的确切 provider/model，让操作者看到哪个 model 干了哪份活。如果相同的裸 Router 或 Fleet 名字在两个根里都存在，请用 `workspace/` 或 `codewhale_home/` 限定它，而不是依赖遮蔽（shadowing）。 -每个成员的 `permissions` 预设是一个**上限**，绝不是授权：它会与会话的实时姿态取交集，因此只读会话里的 `read_write` 成员以只读方式运行。这个交集成为子任务真实的工具包——`permissions = "none"` 让它一个工具都没有，而没有网络工具的成员会失去每一个可触达的网络表面——`web.run`、`fetch_url`、`web_search`、`github`、`mcp*` 系列以及可触达的 `rlm` 动作——而不是仅仅在调用时被拒绝。唯一刻意的例外是规范 `Web` 家族的 `search`/`fetch` 动作：它们是只读成员有权使用的只读 web 表面（与普通 scout 对等），因此网络被禁的成员恰好保留 `Web {search, fetch}`。它在事实上保持只读，而不仅仅是名义上：按 URL 寻址的 `fetch` 在派发时被拒绝，表面上没有任何其他东西被授予。不能写入的成员会失去可变的文件工具，并且——当它保留了 `shell = "full"` 以便运行检查时——也会失去原始 shell，只保留它为之而存在的有界验证工具（`run_tests`、`run_verifiers`）：任意 shell 命令和 `write_file` 一样确定无疑地改变工作区，所以保留它会令 `write = false` 不成立。那个验证表面只在它的*默认*形式下有界，因此无界参数会随 shell 一起消失：被拒绝写入的成员可以运行内置 gate，但不能用显式 `commands` 数组运行 `run_verifiers`，也不能用原始 `args` 字符串运行 `run_tests`——这两者都会启动操作者提供的程序，是换了名字的原始 shell。 +每个成员的 `permissions` 预设是一个**上限**，绝不是授权：它会与会话的实时姿态取交集，因此只读会话里的 `read_write` 成员以只读方式运行。这个交集成为子任务真实的工具包——`permissions = "none"` 让它一个工具都没有，而没有网络工具的成员会失去每一个可触达的网络表面——`web.run`、`fetch_url`、`web_search`、`github`、`mcp*` 系列以及可触达的 `rlm` 动作——而不是仅仅在调用时被拒绝。唯一刻意的例外是规范 `Web` 家族的 `search`/`fetch` 动作：它们是只读成员有权使用的只读 web 表面（与普通 scout 对等），因此网络被禁的成员恰好保留 `Web {search, fetch}`。它在事实上保持只读，而不仅仅是名义上：按 URL 寻址的 `fetch` 在派发时被拒绝，表面上没有任何其他东西被授予。不能写入的成员会失去可变的文件工具，并且——当它保留了 `shell = "full"` 以便运行检查时——也会失去原始 shell，只保留它为之而存在的有界验证工具（`run_tests`、`run_verifiers`；经 `agent` 工具嵌套的 verifier 在授予该通道的会话内委派上额外持有分类器门控的只读取证探针通道，但 Fleet 成员信封把有界验证与 Bash 权限定义为互斥，因此不带该探针通道）：任意 shell 命令和 `write_file` 一样确定无疑地改变工作区，所以保留它会令 `write = false` 不成立。那个验证表面只在它的*默认*形式下有界，因此无界参数会随 shell 一起消失：被拒绝写入的成员可以运行内置 gate，但不能用显式 `commands` 数组运行 `run_verifiers`，也不能用原始 `args` 字符串运行 `run_tests`——这两者都会启动操作者提供的程序，是换了名字的原始 shell。 这些拒绝中有两项比工具名更窄，因为有两个工具触及了它们名字之外的地方。`rlm` 通过在进程*内部*、以它自己的名字调用 fetch 工具来加载 `url`，而 `rlm` 的 `eval` 动作针对活内核运行 Python——套接字和文件系统都触及。因此该家族**按动作**被拒绝，而不是整体拒绝： diff --git a/docs/zh_hans/GUIDE.md b/docs/zh_hans/GUIDE.md index f6ad23ae18..0d2d9634f2 100644 --- a/docs/zh_hans/GUIDE.md +++ b/docs/zh_hans/GUIDE.md @@ -325,7 +325,7 @@ Codewhale 的工具是结构化操作。模型不只是产出文字，还能调 | `plan` | 设计与迁移规划 | | `review` | 对已有改动的 bug 聚焦审查 | | `implementer` | 规格明确的编辑 | -| `verifier` | 运行检查并报告通过/失败证据 | +| `verifier` | 运行内置验证门禁并报告通过/失败证据 | 子智能体在可以干净切分工作的时候最有用。不要为微小编辑使用它们，也不要让多个智能体同时写入相同文件。 diff --git a/docs/zh_hans/SUBAGENTS.md b/docs/zh_hans/SUBAGENTS.md index 708cc14dda..f4b4584e51 100644 --- a/docs/zh_hans/SUBAGENTS.md +++ b/docs/zh_hans/SUBAGENTS.md @@ -27,17 +27,17 @@ Fleet 角色是面向用户的委派工作词汇：父代理通过 `agent` 启 | 角色 | 姿态 | 可写？ | 联网？ | Shell 姿态 | 典型用途 | |---|---|---|---|---|---| | `worker` | 灵活；父代理说什么就做什么 | 是 | 是 | 是 | 默认角色；多步任务 | -| `scout` | 只读；快速摸清相关代码 | 否 | 是 | 只读（网络 + 有界验证） | "找到 `Foo` 的每个调用点；用 gh 检查这个 PR" | +| `scout` | 只读；快速摸清相关代码 | 否 | 是 | 只读（网络） | "找到 `Foo` 的每个调用点；用 gh 检查这个 PR" | | `planner` | 分析并产出策略 | 否 | 是 | 只读探针 | "设计迁移方案；不要执行" | -| `reviewer` | 带严重度评分的阅读与评审 | 否 | 是 | 只读（网络 + 有界验证） | "审计这个 PR 的 bug" | +| `reviewer` | 带严重度评分的阅读与评审 | 否 | 是 | 只读（网络） | "审计这个 PR 的 bug" | | `builder` | 以最小改动落地某个具体变更 | 是 | 是 | 是 | "把 `bar.rs::Foo::bar` 重写为做 X" | -| `verifier` | 运行测试/验证并报告结果 | 否 | 是 | 测试导向 | "运行 cargo test --workspace，并报告" | +| `verifier` | 运行内置验证门禁并报告结果（只读探针取证） | 否 | 是 | 内置门禁（被父代理钳制时仅剩探针）+ 只读探针（无自定义命令） | "用内置检查验证这个 diff，并报告 PASS/FAIL" | | `consultant` | 短期、高推理密度的咨询 | 否 | 是 | 无 | "这个设计我们漏掉了什么？" | | `custom` | 显式的窄工具 allowlist | 继承 | 继承 | 继承 | 在父代理姿态上精选的工具 | 角色的默认值就是该角色*想要*的姿态，而父代理的有效姿态永远是天花板（子代理绝不会扩得比父代理更宽）。只读角色按意图扣留**工作区写入**；默认不拿走任何其他东西——每个角色都保留网络读取，`custom` 继承父代理的写入/网络/shell 姿态，并且只被它的显式工具列表或发起调用收窄。被聚焦的 worker 的头部会依据运行时自身的权限快照声明有效姿态（`scout · read-only · network · read-only shell`）。 -**委派移动的是工作，绝不是权限**（#5426 的遏制答案）。只读角色委派给可写角色（scout → builder）是*工作容量*受支持的逃生舱——子代理自带其模型、路由和步骤预算——但子代理的权限被钳制在委派父代理的实时姿态上，而不是操作者的姿态上：scout 的 builder 子代理以只读落地，raw shell 和可变工具被拒绝，规范的 `Bash` 对它也被拒绝（只有有界检查角色保留分类的只读 shell）。因此通过委派来获得 shell 在机制上是无用的——scout 自带的受限 shell（`git -C … log`、`find … | head`、`npm view …`，分类器门控）是只读父代理唯一的 shell 路径。只读通过任何委派链都是传递的：钳制（`fleet/exact.rs` 中的 `ChildAuthority::clamp`）把每个字段与更窄的一侧求交，拒绝列表的并集意味着后代永远无法去掉祖先的限制，而 `inherit_disallowed_tools: false` 无法去掉姿态拒绝（`is_posture_denial`）。这一点由 `crates/tui/src/fleet/exact.rs` 测试中的 `a_read_only_parents_delegation_never_widens_authority` 钉死。 +**委派移动的是工作，绝不是权限**（#5426 的遏制答案）。只读角色委派给可写角色（scout → builder）是*工作容量*受支持的逃生舱——子代理自带其模型、路由和步骤预算——但子代理的权限被钳制在委派父代理的实时姿态上，而不是操作者的姿态上：scout 的 builder 子代理以只读落地，raw shell 和可变工具被拒绝，规范的 `Bash` 对它也被拒绝（只有有界检查角色与 verifier 保留分类的只读 shell）。因此通过委派来获得 shell 在机制上是无用的——scout 自带的受限 shell（`git -C … log`、`find … | head`、`npm view …`，分类器门控）是只读父代理唯一的 shell 路径。该有界语法还允许把这些已准入的探针组合成复合序列——以 `;`、`&&`、`||` 或 `|` 连接的段，且每个段都必须单独准入（例如 `wc -c f; wc -l f`、`cd sub && wc -c notes.md`、`cat x | od -c`）——以及工作区受限的 `cd <单个相对路径>`（可单独使用，或出现在连接符必定执行它的序列位置——以 `;` 连接或位于序列首位；`&&`/`||` 之后的 `cd` 可能根本不会执行，因此会被拒绝）。只读通过任何委派链都是传递的：钳制（`fleet/role.rs` 中的 `ChildAuthority::clamp`）把每个字段与更窄的一侧求交，拒绝列表的并集意味着后代永远无法去掉祖先的限制，而 `inherit_disallowed_tools: false` 无法去掉姿态拒绝（`is_posture_denial`）。这一点由 `crates/tui/src/fleet/exact.rs` 测试中的 `a_read_only_parents_delegation_never_widens_authority` 钉死。 会话的**权限姿态**在每个子代理内部的应用方式与父代理回合完全一致：在 Auto-Review 下，同一个确定性底线和一次性模型守护者决定 worker 的被扣留调用（绝不是提示词；守护者不可用时拒绝，fail closed）；在 Ask 下，角色无法委派的被扣留调用会作为审批提示在父代理的 UI 中弹出，worker 可见地等待（`waiting for user`），或者在无法提示的主机上带着原因被拒绝；Full Access 仍然在不可绕过的安全底线上 fail closed。每一次没有人被提示的决策都是该 worker 转录中的一行备注（聚焦时可见）和一条审计日志记录。参见 `docs/MODES.md`。 @@ -84,7 +84,7 @@ STOP_CONDITION: OUTPUT: VERDICT, EVIDENCE, GAPS, NEXT ``` -`scout` 简报默认为快速的只读调查（不写，但网络触达和有界验证面可用于真正的侦察）。约 3-5 次工具调用足以完成快速探索：定位、搜索、读取决定性代码行，然后返回。除非证据与之矛盾，否则不要重复 `ALREADY_KNOWN` 的工作。review 和 verifier 简报可以花更多调用，但应在拿到决定性证据后停止。builder 和修复型简报应在扩展示范围之前或反复失败之后设置检查点，而不是设置一个很小的调用上限。 +`scout` 简报默认为快速的只读调查（不写，但网络触达和分类器门控的只读探针通道可用于真正的侦察——不含验证门禁）。约 3-5 次工具调用足以完成快速探索：定位、搜索、读取决定性代码行，然后返回。除非证据与之矛盾，否则不要重复 `ALREADY_KNOWN` 的工作。review 和 verifier 简报可以花更多调用，但应在拿到决定性证据后停止。builder 和修复型简报应在扩展示范围之前或反复失败之后设置检查点，而不是设置一个很小的调用上限。 好的委派提示词示例： @@ -122,7 +122,7 @@ OUTPUT: VERDICT、EVIDENCE、GAPS、NEXT。 - **`planner`** —— 当父代理有目标但没有可执行的分解。planner 写工件（`todo_write` 条目、响应体里的策略），但不执行它们。 - **`reviewer`** —— 当已经有一个变更，父代理想要它被评分。reviewer 不打补丁——他们在发现里描述修复方案，这样如果判定是"修它"，父代理可以派一个 builder。 - **`builder`** —— 当变更已经被明确指定、只需要落地。builder 保持严格的范围：最小改动，不做顺手重构，交回前跑一次快速验证。 -- **`verifier`** —— 当父代理需要测试套件或其他验证上的权威通过/失败结论。verifier 不修失败；他们记录失败的断言 + 栈，把修复候选放在 RISKS 下。 +- **`verifier`** —— 当父代理需要测试套件或其他验证上的权威通过/失败结论。verifier 不修失败；他们记录失败的断言 + 栈，把修复候选放在 RISKS 下。verifier 姿态从不写入；其 shell 是有界内置验证面——对会话内子代理，只有当只读 shell 父代理钳制了其权限时才会被拒绝，被钳制的子代理保留探针通道、失去验证门禁——加上，在授予该通道的会话内委派上，与检查角色相同的分类器门控只读探针通道（`wc`、`ls`、`cat`、`grep` 管道）用于取证；未授予该通道的信封会省略它，shell 尝试一律拒绝。Fleet 成员的信封更严格：在那里有界验证与 Bash 权限互斥，因此 fleet verifier 只持有门禁、不带探针通道。写入上限是只读，无界 shell 形式被拒绝（#5186），自定义命令也被拒绝——请改用具备写能力的角色。 - **`consultant`** —— 当操作者想在更便宜的执行继续之前得到一个高杠杆的第二意见。consultant 读足够的材料来支撑一条建议，但不能写，也不能运行 shell 命令。`oracle` 和 `advisor` 只在加载更老的请求或持久化记录时被接受；新的提示词、回执和 UI 使用 `consultant`。 - **`custom`** —— 只有当父代理需要显式约束工具集时。通过 legacy/internal 子代理记录上的 `allowed_tools` 字段传 allowlist；面向模型的 `agent` 工具刻意保持公共 schema 很小。