From d6787ccc04fa2f606230f10bae5d21295b8da22c Mon Sep 17 00:00:00 2001 From: asto18089 Date: Wed, 4 Nov 2026 18:44:08 +0800 Subject: [PATCH 1/7] fix(shell): stdin EOF on payload-less runs First slice of the #126 split: the foreground/sync stdin lifecycle. - Foreground and sync shell commands without a stdin payload no longer hold the write end open for the process lifetime: foreground closes it right after spawn, the sync lane spawns with `Stdio::null()`, so a stdin-reading probe (`cat`, a bare `$SHELL`) gets end-of-file and exits instead of idling until the 120 s watchdog. - A foreground command later detached to the background reports its closed stdin on `interact` instead of an opaque refusal. - stdin writes are bounded everywhere (64 KiB inline cap, owned-writer fallback for Windows/ohos, 5 s deadline with a stall flag), a PTY writer no longer injects EOF on drop (dup'd master fd), and the verifier's background gate jobs are handed EOF at spawn instead of idling on the held-open pipe. - bash guidance and the schema describe the foreground stdin contract; the model-facing description carries the same sentence. Tests: 11 new pins (foreground EOF, sync-lane EOF, detached-foreground interact receipt, PTY interact surface, stalled-tty bound, interact/ inline caps, bounded owned write) + the verifier gate-job EOF pin. Targeted: `tools::shell::` 152 passed / 0 failed / 1 ignored (pre-existing), `tests::verifier::` green; fmt clean; fork-CI clippy flags clean; upstream all-targets clippy reports only the pre-existing core/engine/turn_loop.rs:5499 collapsible_if (base code). Split of Pinvou/CodeWhale#126; full content preserved on archive/p126-pre-split-8705ccd85. Stacks: none (base f1eb10efc). Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 443 ++++++++++++-- crates/tui/src/tools/shell/guidance.rs | 15 +- crates/tui/src/tools/shell/tests.rs | 763 +++++++++++++++++++------ crates/tui/src/tools/verifier.rs | 72 ++- 4 files changed, 1093 insertions(+), 200 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 62aa201e55..9d7e94a8d3 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -873,7 +873,21 @@ impl ShellChild { enum StdinWriter { Pipe(ChildStdin), - #[cfg(not(target_env = "ohos"))] + /// Unix PTY lane: a dup of the portable-pty master fd, so interact + /// 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. + /// 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 + /// never-constructed `dead_code` variant there, which `deny(warnings)` + /// makes fatal, and every referencing site carries the same predicate. + #[cfg(all(unix, not(target_env = "ohos")))] + PtyFile(std::fs::File), + #[cfg(all(not(target_env = "ohos"), not(unix)))] Pty(Box), } @@ -881,7 +895,9 @@ impl StdinWriter { fn write_all(&mut self, data: &[u8]) -> std::io::Result<()> { match self { StdinWriter::Pipe(stdin) => stdin.write_all(data), - #[cfg(not(target_env = "ohos"))] + #[cfg(all(unix, not(target_env = "ohos")))] + StdinWriter::PtyFile(file) => file.write_all(data), + #[cfg(all(not(target_env = "ohos"), not(unix)))] StdinWriter::Pty(writer) => writer.write_all(data), } } @@ -889,12 +905,49 @@ impl StdinWriter { fn flush(&mut self) -> std::io::Result<()> { match self { StdinWriter::Pipe(stdin) => stdin.flush(), - #[cfg(not(target_env = "ohos"))] + #[cfg(all(unix, not(target_env = "ohos")))] + StdinWriter::PtyFile(file) => file.flush(), + #[cfg(all(not(target_env = "ohos"), not(unix)))] StdinWriter::Pty(writer) => writer.flush(), } } } +/// 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)`; +/// a deadline timeout returns `(None, TimedOut)` — the writer stays with +/// the abandoned worker, the bytes still land if the command resumes +/// reading, and the caller marks the stall. 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))] +fn bounded_stdin_write_owned( + writer: StdinWriter, + payload: Vec, +) -> (Option, std::io::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)); + }); + match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { + Ok((writer, result)) => (Some(writer), result), + Err(_) => ( + None, + Err(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "the stdin write did not complete within the deadline", + )), + ), + } +} + fn spawn_reader_thread( mut reader: R, buffer: SharedRawOutput, @@ -1079,6 +1132,19 @@ 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. + 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. + stdin_write_stalled: bool, child: Option, /// A stop was delivered but the direct child's exit was never observed: /// the bounded reap grace elapsed first (e.g. uninterruptible sleep), so @@ -1411,16 +1477,161 @@ impl BackgroundShell { } } + /// Close the stdin pipe of this task, keeping a PTY writer attached — + /// the foreground close: pipe readers get end-of-file at once, while a + /// terminal command keeps the interact surface a session member can + /// write to while it runs. Only a PIPE is claimed as end-of-file. + fn close_stdin_pipe(&mut self) { + if matches!(self.stdin, Some(StdinWriter::Pipe(_))) { + self.stdin_closed_at_eof = true; + self.stdin = None; + } + } + fn write_stdin(&mut self, input: &str, close: bool) -> Result<()> { - if let Some(stdin) = self.stdin.as_mut() { + // Taken owned so the Windows/ohos fallback can move the writer into + // its bounded worker and hand it back; every exit below restores the + // slot first, keeping the stall/eof refusal ordering identical with + // the pinned unix behavior. + let mut slot = self.stdin.take(); + if slot.is_some() { if !input.is_empty() { - stdin - .write_all(input.as_bytes()) - .context("Failed to write to stdin")?; - stdin.flush().ok(); + if self.stdin_write_stalled { + self.stdin = slot; + return Err(anyhow!( + "stdin writes to task {} are refused: an earlier write did not complete within {STDIN_WRITE_DEADLINE:?} because the command stopped consuming its stdin; the abandoned bytes still land if it resumes. Kill the task and start a new one instead of feeding it further.", + self.id + )); + } + // A blocking write under the shell-manager lock wedges the + // executor thread and every other shell in the session when + // the stdin handle's buffer is full and the command never + // reads (the exact stall the inline-payload cap exists to + // close on the run boundary — interact chunks and + // small-buffer platforms hit the same wall one chunk later). + // Every platform bounds the write now, by what its handle + // offers: on unix the write is bounded for BOTH handle kinds + // by fd clone — pipes clone the `ChildStdin` fd, and the PTY + // lane clones the dup'd master fd it spawned with — a worker + // thread owns the clone and finishes the payload in the + // background (it still lands if the child resumes), the + // caller learns about the stall at the deadline, and the + // stall flag refuses any further write so two writers can + // never interleave on one handle. + // The same predicate as the `PtyFile` variant. ohos strips + // this whole statement (the attribute gates the if-else), so + // ohos takes the owned-writer fallback below — that + // predicate is what keeps ohos compiling with a live write + // and no dead binding, field, or methods under the workspace + // deny(warnings). + #[cfg(all(unix, not(target_env = "ohos")))] + if matches!( + slot, + Some(StdinWriter::Pipe(_)) | Some(StdinWriter::PtyFile(_)) + ) { + use std::os::fd::AsFd; + let dup_fd = match slot.as_ref() { + Some(StdinWriter::Pipe(pipe)) => pipe.as_fd().try_clone_to_owned(), + Some(StdinWriter::PtyFile(file)) => file.as_fd().try_clone_to_owned(), + // The matches! above proves Some(Pipe | PtyFile); the + // None leg exists only for exhaustiveness (clippy + // flags an expect-after-is_some here, and an + // exhaustive match without it would be an + // unreachable-pattern warning). + _ => unreachable!("variant checked above"), + } + .context("Failed to clone the stdin handle for the bounded write")?; + 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(result) => result.context("Failed to write to stdin")?, + // A channel close without a send means the worker + // died before completing (a panic in the write + // 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) => { + 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 + )); + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + 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 + )); + } + } + } else { + // Armor for a future third variant on this cfg: the + // matches! above proved Some, and this arm never runs on + // Windows/ohos (stripped with the whole if-else). + if let Some(writer) = slot.as_mut() { + writer + .write_all(input.as_bytes()) + .context("Failed to write to stdin")?; + writer.flush().ok(); + } + } + // Windows and ohos cannot clone an fd out of their handle + // (Windows strips the statement above because `unix` is + // false, ohos by the `not(target_env = "ohos")` half — the + // portable-pty writer box exposes no cloneable handle at + // all). They bound by ownership instead: the worker takes + // the writer, finishes the payload, and hands the writer + // back through the channel; a deadline timeout leaves the + // writer with the abandoned worker (the bytes still land if + // the command resumes), marks the stall, and the caller + // returns with the session's shell lock released — cancel, + // kill, and wait all stay live. This is the same wedge the + // unix comment above describes, closed here too. + #[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()); + match result { + Ok(()) => { + slot = returned; + } + Err(error) if error.kind() == std::io::ErrorKind::TimedOut => { + self.stdin_write_stalled = true; + // `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 + )); + } + Err(error) => { + return Err(anyhow!(error).context("Failed to write to stdin")); + } + } + } } if close { + // Only a dropped pipe delivers end-of-file; dropping a PTY + // writer is invisible to the child (the reader holds the + // master), so it must not be reported as EOF later. 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. + self.stdin_closed_at_eof = + matches!(slot, Some(StdinWriter::Pipe(_))) && !self.stdin_write_stalled; self.stdin = None; + } else { + self.stdin = slot; } return Ok(()); } @@ -1429,7 +1640,23 @@ impl BackgroundShell { return Ok(()); } - Err(anyhow!("stdin is not available for task {}", self.id)) + if self.stdin_write_stalled { + return Err(anyhow!( + "stdin is not available for task {}: its last write stalled because the command stopped consuming stdin, and the abandoned writer may still hold the handle; kill it and start a new one.", + self.id + )); + } + + 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\"", + 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\"", + self.id + )) } fn full_output(&self) -> (String, String, usize, usize) { @@ -1986,6 +2213,8 @@ impl ShellManager { stderr_cursor: 0, completion_reported: false, stdin: None, + stdin_closed_at_eof: false, + stdin_write_stalled: false, child: None, stop_unconfirmed: false, #[cfg(windows)] @@ -2491,6 +2720,13 @@ impl ShellManager { if stdin_data.is_some() { cmd.stdin(Stdio::piped()); + } else { + // std defaults to `Stdio::inherit()`, which on a hosted (non-PTY) + // run hands the child the parent's TTY or an arbitrary pipe it can + // never see EOF on. This lane has no stdin payload and no operator: + // a deterministic null stdin gives stdin-readers an immediate + // end-of-file instead of a hang or a SIGTTIN stop. + cmd.stdin(Stdio::null()); } child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env)); @@ -2842,22 +3078,64 @@ impl ShellManager { return Err(err).context("Failed to clone PTY reader"); } }; - let writer = match pair.master.take_writer() { + let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); + + // 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 + // the writer sends EOF" is implemented there), which a + // terminal command that never receives an interact would read + // 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. + #[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"); + } + // SAFETY: `dup_fd` is a freshly duplicated fd owned + // by this call; `File::from_raw_fd` takes ownership. + Some(StdinWriter::PtyFile(unsafe { + std::fs::File::from_raw_fd(dup_fd) + })) + } + None => { + let _ = child.kill(); + let _ = child.wait(); + return Err(anyhow!( + "Failed to obtain the PTY master fd for the stdin writer" + )); + } + } + }; + // Windows has no dup'd master fd to hand out (the + // portable-pty writer box is not fd-exposed there), so the + // lane keeps the box and its blocking write, per the + // interact comment. + #[cfg(not(unix))] + let stdin_writer = Some(StdinWriter::Pty(match pair.master.take_writer() { Ok(writer) => writer, Err(err) => { let _ = child.kill(); let _ = child.wait(); return Err(err).context("Failed to take PTY writer"); } - }; - let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); + })); - ( - ShellChild::Pty(child), - Some(StdinWriter::Pty(writer)), - stdout_thread, - None, - ) + (ShellChild::Pty(child), stdin_writer, stdout_thread, None) } } else if persist_pending { let mut cmd = Command::new(program); @@ -2980,6 +3258,8 @@ impl ShellManager { stderr_cursor: 0, completion_reported: false, stdin, + stdin_closed_at_eof: false, + stdin_write_stalled: false, child: Some(child), stop_unconfirmed: false, #[cfg(windows)] @@ -3105,6 +3385,21 @@ impl ShellManager { Ok(()) } + /// Close a foreground command's stdin PIPE so pipe readers get their + /// end-of-file at once, keeping any PTY writer attached: dropping the + /// 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. + pub 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(()) + } + pub fn write_stdin_for_session( &mut self, active_session_id: &str, @@ -4573,20 +4868,50 @@ async fn execute_foreground_via_background( let task_id = spawned .task_id .ok_or_else(|| anyhow!("foreground shell did not return a process id"))?; - if let Some(permit) = heavy_permit { + // The stdin write end must close BEFORE any fallible step between spawn + // and close: a future step that can fail here (the heavy-permit attach + // below) would otherwise return early and leak an open write end, + // wedging stdin-reading commands until the foreground watchdog killed + // 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 `write_stdin` 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). + // + // Foreground has no operator at the keyboard, so whatever the command + // will ever read from stdin must be bounded now. With stdin data the + // payload was delivered at spawn and closing the write end turns the + // pipe into a normal end-of-file; with no data, closing immediately + // delivers that EOF at once. Leaving the write end open wedged any + // stdin-reading command (`cat`, a bare `$SHELL`) on a pipe that never + // EOFs until the foreground watchdog killed it. This is the foreground + // stdin contract the tool description states: a command detached to the + // background later (`request_foreground_background`) keeps running, but + // its stdin stays at EOF — `interact` reports the closed stdin and points + // at background=true, which is the lane for input-driven work. The close + // is deliberately pipe-only: a terminal (`combined_output`) foreground + // command's PTY writer must stay attached, because dropping it would + // destroy the interact surface a session member can still write to + // while the terminal command runs (the child sees no difference — the + // reader holds the master — and the terminal lane's blocking is bounded + // by the foreground watchdog, not by a never-EOFing pipe). The + // manager-side close is idempotent-safe after natural completion and on + // lanes where the handle is absent (persistent). + { let mut manager = context .shell_manager .lock() .map_err(|_| anyhow!("shell manager lock poisoned"))?; - manager.attach_heavy_permit(&task_id, permit)?; + manager.close_stdin_pipe(&task_id)?; } - - if stdin_data.is_some() { + if let Some(permit) = heavy_permit { let mut manager = context .shell_manager .lock() .map_err(|_| anyhow!("shell manager lock poisoned"))?; - manager.write_stdin(&task_id, "", true)?; + manager.attach_heavy_permit(&task_id, permit)?; } let deadline = timeout_ms.map(|timeout| Instant::now() + Duration::from_millis(timeout)); @@ -4657,6 +4982,27 @@ 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. +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. +const STDIN_WRITE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); + /// 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 @@ -4775,7 +5121,7 @@ impl ToolSpec for LowercaseBashTool { } fn description(&self) -> &'static str { - "Execute a shell command in the workspace and return stdout and stderr. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell); with an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." + "Execute a shell command in the workspace and return stdout and stderr. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell); This foreground surface runs without a terminal stdin: interactive programs and a bare `$SHELL` receive end-of-file and exit. With an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." } fn input_schema(&self) -> serde_json::Value { @@ -4990,14 +5336,17 @@ impl ToolSpec for BashTool { }, "stdin": { "type": "string", - "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one." + "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." }, "input": { "type": "string", + "maxLength": 65536, "description": "Alias for `stdin`." }, "data": { "type": "string", + "maxLength": 65536, "description": "Alias for `stdin`." }, "cwd": { @@ -5190,12 +5539,27 @@ impl ToolSpec for BashTool { // for misspelled names. A wrong type is an error, never a no-op. let stdin_data = match first_present_field(&input, &["stdin", "input", "data"]) { None => None, - Some((name, value)) => Some( - value + Some((name, value)) => { + let payload = value .as_str() .ok_or_else(|| type_mismatch(name, value, "a string"))? - .to_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. + 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.", + payload.len() + ))); + } + Some(payload) + } }; if interactive && background { @@ -6086,9 +6450,24 @@ impl BashTool { // place whichever spelling the model uses. let interaction_input = match first_present_field(input, &["stdin", "input", "data"]) { None => "", - Some((name, value)) => value - .as_str() - .ok_or_else(|| type_mismatch(name, value, "a string"))?, + Some((name, value)) => { + let payload = value + .as_str() + .ok_or_else(|| type_mismatch(name, value, "a string"))?; + // The same bound the inline spawn payload carries: one + // oversized `interact` payload would push megabytes through + // a bounded write for no benefit — feed ≥64 KiB of input as + // several interact calls (or generate it in the command). + // The deadline below is what actually bounds the stall; this + // keeps each write bounded too. + if payload.len() > MAX_INLINE_STDIN_BYTES { + return Ok(ToolResult::error(format!( + "The interact stdin payload is {} bytes; the shell tool accepts at most {MAX_INLINE_STDIN_BYTES} per interact call — split the feed across interact calls.", + payload.len() + ))); + } + payload + } }; { diff --git a/crates/tui/src/tools/shell/guidance.rs b/crates/tui/src/tools/shell/guidance.rs index 8a565a1205..258512032a 100644 --- a/crates/tui/src/tools/shell/guidance.rs +++ b/crates/tui/src/tools/shell/guidance.rs @@ -85,7 +85,20 @@ pub(super) fn description() -> &'static str { \"wait\" blocks for a background task until completion or timeout; \ \"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. \ + commands; use background=true for work expected to take >5 seconds. Foreground \ + commands without `interactive` 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 \ + writes to unless a session member interacts with the task, so a \ + stdin-reading command waits until the watchdog ends it. A foreground \ + run without `combined_output` may carry one inline stdin payload \ + (delivered to the command, then end-of-file); a `combined_output` \ + foreground run delivers the payload but its terminal lane stays \ + open, so no end-of-file follows it. For multi-write input-driven \ + workflows supply stdin on the background lane: \ + use background=true plus \ + action=\"interact\". \ Output is truncated per stream (~30KB: head/tail kept, middle summarized; \ see metadata flags).", runtime_command_guidance() diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index d73535648b..422e54f6c7 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -1,26 +1,20 @@ use super::*; - use crate::tools::spec::ToolContext; use serde_json::{Value, json}; use tempfile::tempdir; - #[cfg(windows)] use windows::Win32::Foundation::{DUPLICATE_HANDLE_OPTIONS, DuplicateHandle, HANDLE}; #[cfg(windows)] use windows::Win32::System::Threading::GetCurrentProcess; - // `env_lock` serializes tests that mutate the process environment. #[cfg(any(unix, windows))] use std::sync::{Mutex, OnceLock}; - #[cfg(any(unix, windows))] fn env_lock() -> &'static Mutex<()> { static LOCK: OnceLock> = OnceLock::new(); LOCK.get_or_init(|| Mutex::new(())) } - const BACKGROUND_COMPLETION_WAIT_MS: u64 = 30_000; - #[test] fn lowercase_bash_schema_is_small_contract() { let schema = LowercaseBashTool.input_schema(); @@ -40,7 +34,6 @@ fn lowercase_bash_schema_is_small_contract() { ); assert!(!BashTool::new("Bash").model_visible()); } - #[test] fn lowercase_bash_description_matches_the_timeout_it_actually_applies() { use super::{ @@ -89,7 +82,6 @@ fn lowercase_bash_description_matches_the_timeout_it_actually_applies() { "schema must name the default it applies: {timeout_doc}" ); } - #[test] fn contract_bash_foreground_without_a_timeout_is_bounded_not_endless() { use super::{ @@ -130,7 +122,6 @@ fn contract_bash_foreground_without_a_timeout_is_bounded_not_endless() { Some(120_000) ); } - #[cfg(all(unix, not(target_env = "ohos")))] #[test] fn inherited_interactive_terminal_fails_closed_before_spawn() { @@ -147,7 +138,6 @@ fn inherited_interactive_terminal_fails_closed_before_spawn() { assert!(message.contains("terminal/run"), "{message}"); assert!(message.contains("terminal/send"), "{message}"); } - #[cfg(all(unix, target_env = "ohos"))] #[test] fn inherited_interactive_terminal_offers_only_ohos_recovery_paths() { @@ -163,7 +153,6 @@ fn inherited_interactive_terminal_offers_only_ohos_recovery_paths() { assert!(!message.contains("background: true"), "{message}"); assert!(!message.contains("terminal/run"), "{message}"); } - #[test] fn contract_bash_nonzero_is_an_error_with_status_after_output() { let error = finish_contract_bash_result( @@ -194,7 +183,6 @@ fn contract_bash_nonzero_is_an_error_with_status_after_output() { .ends_with("before\n\nCommand exited with code 7") ); } - #[cfg(unix)] #[tokio::test] async fn lowercase_bash_returns_one_ordered_stream() { @@ -209,7 +197,6 @@ async fn lowercase_bash_returns_one_ordered_stream() { .expect("bash"); assert_eq!(result.content, "out-1err-2out-3"); } - #[cfg(windows)] fn windows_python_stdio_fixture() -> (tempfile::TempDir, String) { assert!( @@ -256,7 +243,6 @@ fn windows_python_stdio_fixture() -> (tempfile::TempDir, String) { ); (workspace, command) } - #[cfg(windows)] fn assert_windows_python_stdio(output: &str) { assert!( @@ -276,7 +262,6 @@ fn assert_windows_python_stdio(output: &str) { "output contains replacement characters" ); } - #[cfg(windows)] #[tokio::test] #[ignore = "requires native Windows Python on PATH and PowerShell"] @@ -293,7 +278,6 @@ async fn forkguard_windows_python_stdio_lowercase_bash() { assert!(result.success, "{}", result.content); assert_windows_python_stdio(&result.content); } - #[cfg(windows)] #[test] #[ignore = "requires native Windows Python on PATH and PowerShell"] @@ -316,7 +300,6 @@ fn forkguard_windows_python_stdio_background_shell() { assert_windows_python_stdio(&format!("{}\n{}", result.stdout, result.stderr)); } } - #[cfg(unix)] #[tokio::test] async fn lowercase_bash_keeps_raw_command_under_readonly_policy() { @@ -337,7 +320,6 @@ async fn lowercase_bash_keeps_raw_command_under_readonly_policy() { .to_string() ); } - #[tokio::test] async fn lowercase_bash_readonly_refusal_names_work_mode() { let workspace = tempdir().expect("workspace"); @@ -353,7 +335,6 @@ async fn lowercase_bash_readonly_refusal_names_work_mode() { assert!(!result.content.contains("Act mode")); assert!(!workspace.path().join("blocked-by-plan").exists()); } - /// Regression for the wedge that took out the owner's own session under swap /// exhaustion: the lowercase `bash` spill file could not be created (full temp /// volume), every call — including `echo ok` — failed with the harness-internal @@ -426,7 +407,6 @@ async fn lowercase_bash_survives_spill_file_failure_and_stays_usable() { "fail-soft must not create the dir" ); } - /// A spawn/stream failure caused by host exhaustion must reach the model as /// an actionable message (cause chain + likely reason + retry), never as a /// bare harness-internal context string. @@ -463,7 +443,6 @@ fn shell_execution_failure_names_resource_exhaustion_and_says_retry() { "Shell execution failed: working directory does not exist" ); } - #[tokio::test] async fn lowercase_bash_timeout_uses_seconds_and_fails() { let workspace = tempdir().expect("workspace"); @@ -482,7 +461,6 @@ async fn lowercase_bash_timeout_uses_seconds_and_fails() { "{error}" ); } - fn execute_shell( manager: &mut ShellManager, command: &str, @@ -502,7 +480,6 @@ fn execute_shell( "workspace", ) } - #[test] fn forkguard_shell_catalog_guidance_matches_execution() { let tool = BashTool::new("Bash"); @@ -552,7 +529,6 @@ fn forkguard_shell_catalog_guidance_matches_execution() { assert_eq!(catalog[0].name, LowercaseBashTool.name()); assert_eq!(catalog[0].description, LowercaseBashTool.description()); } - #[test] #[ignore = "Exports model-visible shell fixtures for opt-in live model evaluation"] fn export_shell_guidance_eval_fixture() { @@ -569,7 +545,6 @@ fn export_shell_guidance_eval_fixture() { }); std::fs::write(path, serde_json::to_vec_pretty(&fixture).unwrap()).unwrap(); } - #[test] fn deleted_saved_workspace_reports_path_and_recovery_before_spawn() { let workspace = tempdir().expect("workspace"); @@ -584,7 +559,6 @@ fn deleted_saved_workspace_reports_path_and_recovery_before_spawn() { assert!(message.contains("working_dir") || message.contains("cwd")); assert!(message.contains("resume/fork")); } - #[test] fn explicit_missing_working_dir_is_not_misreported_as_session_corruption() { let workspace = tempdir().expect("workspace"); @@ -604,7 +578,6 @@ fn explicit_missing_working_dir_is_not_misreported_as_session_corruption() { assert!(message.contains(&missing.display().to_string())); assert!(!message.contains("saved session workspace")); } - #[cfg(not(target_env = "ohos"))] #[test] fn pty_exit_status_preserves_high_windows_code_losslessly() { @@ -619,7 +592,6 @@ fn pty_exit_status_preserves_high_windows_code_losslessly() { ); assert_eq!(exit_code_hex(status.code).as_deref(), Some("0xC0000005")); } - #[cfg(not(target_env = "ohos"))] #[test] fn ordinary_pty_exit_status_keeps_concise_label() { @@ -629,16 +601,13 @@ fn ordinary_pty_exit_status_keeps_concise_label() { assert_eq!(exit_code_label(status.code), "exit code 127"); assert_eq!(exit_code_hex(status.code), None); } - #[cfg(windows)] #[test] fn std_windows_exit_status_reinterprets_signed_dword() { assert_eq!(std_exit_code_i64(0xC000_0005_u32 as i32), 0xC000_0005); } - #[cfg(windows)] const JOB_OBJECT_QUERY_ACCESS: u32 = 0x0004; - #[cfg(windows)] fn duplicate_job_without_terminate_access(job: WindowsJob) -> WindowsJob { let process = unsafe { GetCurrentProcess() }; @@ -662,11 +631,9 @@ fn duplicate_job_without_terminate_access(job: WindowsJob) -> WindowsJob { handle: limited_handle, } } - fn echo_command(message: &str) -> String { format!("echo {message}") } - fn sleep_command(seconds: u64) -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -682,7 +649,6 @@ fn sleep_command(seconds: u64) -> String { format!("sleep {seconds}") } } - fn sleep_then_echo_command(seconds: u64, message: &str) -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -698,7 +664,6 @@ fn sleep_then_echo_command(seconds: u64, message: &str) -> String { format!("sleep {seconds} && echo {message}") } } - fn echo_stdin_command() -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -713,7 +678,6 @@ fn echo_stdin_command() -> String { "cat".to_string() } } - fn network_restricted_context(tmp: &std::path::Path) -> ToolContext { ToolContext::new(tmp) .with_elevated_sandbox_policy(ExecutionSandboxPolicy::WorkspaceWrite { @@ -726,7 +690,6 @@ fn network_restricted_context(tmp: &std::path::Path) -> ToolContext { "Shell command blocked: Plan mode runs shell commands in a network-restricted sandbox.", ) } - fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -746,12 +709,10 @@ fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult { sandbox_denied: false, } } - #[cfg(unix)] const SHELL_DESCENDANT_HELPER_ENV: &str = "CODEWHALE_SHELL_DESCENDANT_HELPER"; #[cfg(unix)] const SHELL_DESCENDANT_PID_FILE_ENV: &str = "CODEWHALE_SHELL_DESCENDANT_PID_FILE"; - #[cfg(unix)] #[test] fn shell_descendant_helper_process() { @@ -768,7 +729,6 @@ fn shell_descendant_helper_process() { std::thread::sleep(Duration::from_secs(30)); let _ = child.wait(); } - #[cfg(unix)] fn wait_for_shell_pid_file(path: &Path) -> libc::pid_t { let deadline = Instant::now() + Duration::from_secs(5); @@ -785,7 +745,6 @@ fn wait_for_shell_pid_file(path: &Path) -> libc::pid_t { std::thread::sleep(Duration::from_millis(25)); } } - #[cfg(unix)] fn wait_for_shell_pid_exit(pid: libc::pid_t) -> bool { let deadline = Instant::now() + Duration::from_secs(2); @@ -801,7 +760,6 @@ fn wait_for_shell_pid_exit(pid: libc::pid_t) -> bool { std::thread::sleep(Duration::from_millis(25)); } } - fn wait_for_completed_shell(manager: &mut ShellManager, task_id: &str) -> ShellResult { let deadline = Instant::now() + Duration::from_millis(BACKGROUND_COMPLETION_WAIT_MS); @@ -815,7 +773,6 @@ fn wait_for_completed_shell(manager: &mut ShellManager, task_id: &str) -> ShellR std::thread::sleep(Duration::from_millis(50)); } } - #[test] fn shell_owner_registers_before_spawn_and_silent_work_stays_live() { let work = crate::work_graph::new_shared_work_runtime( @@ -878,7 +835,6 @@ fn shell_owner_registers_before_spawn_and_silent_work_stays_live() { Some(512) ); } - #[test] fn exec_shell_parallel_flags_are_input_aware() { let tool = BashTool::new("Bash"); @@ -971,7 +927,6 @@ fn exec_shell_parallel_flags_are_input_aware() { "interactive": true }))); } - #[tokio::test] async fn readonly_shell_refuses_raw_string_external_backend() { struct Backend(std::sync::atomic::AtomicBool); @@ -1004,7 +959,6 @@ async fn readonly_shell_refuses_raw_string_external_backend() { assert!(error.contains("raw command string"), "{error}"); assert!(!backend.0.load(std::sync::atomic::Ordering::SeqCst)); } - #[tokio::test] async fn lowercase_bash_refuses_non_streaming_external_backend() { struct Backend(std::sync::atomic::AtomicBool); @@ -1031,7 +985,6 @@ async fn lowercase_bash_refuses_non_streaming_external_backend() { assert!(error.to_string().contains("combined streaming output")); assert!(!backend.0.load(std::sync::atomic::Ordering::SeqCst)); } - #[test] fn readonly_argv_is_shell_free_and_disables_git_helpers() { let (program, args) = hardened_readonly_argv("git show HEAD").expect("argv"); @@ -1118,7 +1071,6 @@ fn readonly_program_resolution_ignores_workspace_shadow_executables() { assert!(resolved.is_absolute() && !resolved.starts_with(workspace.path())); } } - /// B14-1: an attached root is agent-writable, so an executable planted inside /// it must shadow nothing and its PATH entries must be stripped — exactly /// like the primary root. Judging only the primary here fails open. @@ -1161,7 +1113,6 @@ fn forkguard_workspace_roots_readonly_shell_distrusts_attached_root_programs() { let kept: Vec<_> = std::env::split_paths(&sanitized).collect(); assert_eq!(kept, vec![trusted.path().canonicalize().unwrap()]); } - #[test] fn readonly_child_env_removes_git_and_github_redirects() { let mut command = std::process::Command::new("unused"); @@ -1194,7 +1145,6 @@ fn readonly_child_env_removes_git_and_github_redirects() { .any(|(name, value)| name == READONLY_ENV_MARKER && value.is_none()) ); } - #[test] fn readonly_operands_are_workspace_bounded_and_symlink_aware() { let workspace = tempdir().expect("workspace"); @@ -1269,7 +1219,6 @@ fn readonly_operands_are_workspace_bounded_and_symlink_aware() { .expect_err("operands must resolve relative to the effective cwd"); } } - /// B14-1: the operand/cwd gate judges against the full declared root set. /// Operands and working directories under an attached root were already /// admitted by the roots-aware `resolve_path`; refusing them here would @@ -1324,7 +1273,6 @@ fn forkguard_workspace_roots_readonly_shell_operands_span_attached_roots() { enforce_readonly_workspace_operands("cat shared.txt", &roots, outside.path()) .expect_err("cwd outside every root must fail"); } - #[test] fn windows_verbatim_and_drive_operands_survive_posix_split() { // `shlex` splits with POSIX backslash-escaping, which silently eats @@ -1348,7 +1296,6 @@ fn windows_verbatim_and_drive_operands_survive_posix_split() { ); } } - #[test] fn windows_path_protection_leaves_other_words_untouched() { // POSIX escapes, drive-relative spellings, and plain relative operands @@ -1366,7 +1313,6 @@ fn windows_path_protection_leaves_other_words_untouched() { "cat inside.txt" ); } - #[test] fn readonly_github_shell_calls_obey_the_host_network_policy_before_spawn() { let tmp = tempdir().expect("tempdir"); @@ -1400,7 +1346,6 @@ fn readonly_github_shell_calls_obey_the_host_network_policy_before_spawn() { .to_string(); assert!(prompted.contains("requires network approval")); } - #[test] fn exec_shell_interact_requires_approval() { let tool = BashTool::alias("exec_shell_interact", "interact"); @@ -1410,7 +1355,6 @@ fn exec_shell_interact_requires_approval() { .contains(&ToolCapability::RequiresApproval) ); } - #[tokio::test] async fn read_only_shell_policy_blocks_non_readonly_commands() { let tmp = tempdir().expect("tempdir"); @@ -1451,7 +1395,6 @@ async fn read_only_shell_policy_blocks_non_readonly_commands() { ); } } - #[cfg(unix)] #[tokio::test] async fn read_only_shell_resolves_operands_from_the_effective_cwd() { @@ -1474,7 +1417,6 @@ async fn read_only_shell_resolves_operands_from_the_effective_cwd() { .to_string(); assert!(error.contains("resolves outside"), "{error}"); } - #[cfg(unix)] #[tokio::test] async fn read_only_shell_skips_shell_env_hooks() { @@ -1503,7 +1445,6 @@ async fn read_only_shell_skips_shell_env_hooks() { assert!(result.success, "{}", result.content); assert!(!marker.exists(), "shell_env hook must not run for ReadOnly"); } - #[tokio::test] async fn read_only_shell_policy_allows_readonly_inspection() { let tmp = tempdir().expect("tempdir"); @@ -1529,7 +1470,6 @@ async fn read_only_shell_policy_allows_readonly_inspection() { Some("Completed") ); } - #[tokio::test] async fn exec_shell_multiline_block_explains_allow_shell_boundary() { let tmp = tempdir().expect("tempdir"); @@ -1565,7 +1505,6 @@ async fn exec_shell_multiline_block_explains_allow_shell_boundary() { result.content ); } - #[test] fn exec_shell_wait_schema_defaults_to_blocking() { let schema = BashTool::alias("exec_shell_wait", "wait").input_schema(); @@ -1580,7 +1519,6 @@ fn exec_shell_wait_schema_defaults_to_blocking() { .contains("wait") ); } - #[tokio::test] async fn exec_shell_wait_without_wait_arg_blocks_until_completion() { let tmp = tempdir().expect("tempdir"); @@ -1617,7 +1555,6 @@ async fn exec_shell_wait_without_wait_arg_blocks_until_completion() { Some("Completed") ); } - #[tokio::test] async fn exec_shell_wait_false_returns_nonblocking_snapshot() { let tmp = tempdir().expect("tempdir"); @@ -1659,7 +1596,6 @@ async fn exec_shell_wait_false_returns_nonblocking_snapshot() { Some("Running") ); } - #[tokio::test] async fn exec_shell_wait_without_wait_arg_returns_running_at_timeout() { let tmp = tempdir().expect("tempdir"); @@ -1703,7 +1639,6 @@ async fn exec_shell_wait_without_wait_arg_returns_running_at_timeout() { .await .expect("cancel background"); } - #[tokio::test] async fn exec_shell_wait_many_until_any_returns_when_the_first_task_settles() { let tmp = tempdir().expect("tempdir"); @@ -1772,7 +1707,6 @@ async fn exec_shell_wait_many_until_any_returns_when_the_first_task_settles() { Some(&json!(false)) ); } - #[tokio::test] async fn exec_shell_wait_many_all_reports_timeout_and_still_running() { let tmp = tempdir().expect("tempdir"); @@ -1827,7 +1761,6 @@ async fn exec_shell_wait_many_all_reports_timeout_and_still_running() { assert_eq!(statuses[first_id.as_str()], "Running"); assert_eq!(statuses[second_id.as_str()], "Running"); } - #[tokio::test] async fn exec_shell_wait_many_rejects_an_unknown_task_id() { let tmp = tempdir().expect("tempdir"); @@ -1841,7 +1774,6 @@ async fn exec_shell_wait_many_rejects_an_unknown_task_id() { .expect_err("unknown ids must fail the same way as the single-task path"); assert!(err.to_string().contains("nope-1"), "{err}"); } - #[tokio::test] async fn background_start_advertises_task_status_completion() { let tmp = tempdir().expect("tempdir"); @@ -1871,7 +1803,6 @@ async fn background_start_advertises_task_status_completion() { Some("nonblocking") ); } - #[tokio::test] async fn background_shell_job_preserves_origin_identity() { let tmp = tempdir().expect("tempdir"); @@ -1948,7 +1879,6 @@ async fn background_shell_job_preserves_origin_identity() { .await .expect("cancel owned background shell"); } - #[tokio::test] async fn drain_finished_jobs_reports_once() { let tmp = tempdir().expect("tempdir"); @@ -2004,7 +1934,6 @@ async fn drain_finished_jobs_reports_once() { assert!(second.is_empty(), "completion should be reported only once"); assert!(!manager.may_have_undelivered_completion()); } - #[tokio::test] async fn background_job_is_hidden_from_replacement_session_and_resumes_once_for_owner() { let tmp = tempdir().expect("tempdir"); @@ -2065,7 +1994,6 @@ async fn background_job_is_hidden_from_replacement_session_and_resumes_once_for_ "A completion is delivered exactly once" ); } - #[test] fn completion_evidence_preserves_arbitrary_stream_bytes() { use base64::Engine as _; @@ -2112,7 +2040,6 @@ fn completion_evidence_preserves_arbitrary_stream_bytes() { assert_eq!(decoded_stdout, stdout); assert_eq!(decoded_stderr, stderr); } - #[test] #[cfg(unix)] fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() { @@ -2155,7 +2082,6 @@ fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() { assert_eq!(result.status, ShellStatus::Completed); assert_eq!(result.stdout, "unset\nexplicit-value\n"); } - #[test] #[cfg(windows)] fn shell_execution_preserves_custom_windows_sdk_root_env() { @@ -2205,7 +2131,6 @@ fn shell_execution_preserves_custom_windows_sdk_root_env() { result ); } - #[test] fn test_sync_execution() { let tmp = tempdir().expect("tempdir"); @@ -2218,7 +2143,6 @@ fn test_sync_execution() { assert!(result.stdout.contains("hello")); assert!(result.task_id.is_none()); } - #[test] fn test_background_execution() { let tmp = tempdir().expect("tempdir"); @@ -2245,7 +2169,6 @@ fn test_background_execution() { assert_eq!(final_result.status, ShellStatus::Completed); assert!(final_result.stdout.contains("done")); } - #[test] fn test_timeout() { let tmp = tempdir().expect("tempdir"); @@ -2256,7 +2179,6 @@ fn test_timeout() { assert_eq!(result.status, ShellStatus::TimedOut); } - #[test] fn test_kill() { let tmp = tempdir().expect("tempdir"); @@ -2273,7 +2195,6 @@ fn test_kill() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } - #[test] fn test_write_stdin_streams_output() { let tmp = tempdir().expect("tempdir"); @@ -2311,7 +2232,6 @@ fn test_write_stdin_streams_output() { .expect("get_output_delta"); assert!(delta2.result.stdout.is_empty()); } - #[test] #[cfg(all(unix, not(target_env = "ohos")))] fn background_tty_command_has_controlling_terminal() { @@ -2346,7 +2266,6 @@ fn background_tty_command_has_controlling_terminal() { "tty output should confirm /dev/tty opened; got {done:?}" ); } - #[test] fn test_job_list_poll_cancel_and_stale_snapshot() { let tmp = tempdir().expect("tempdir"); @@ -2399,7 +2318,6 @@ fn test_job_list_poll_cancel_and_stale_snapshot() { assert!(stale.stale); assert_eq!(stale.linked_task_id.as_deref(), Some("task_old")); } - #[test] fn running_job_snapshot_marks_no_output_stale_after_threshold() { let tmp = tempdir().expect("tempdir"); @@ -2428,7 +2346,6 @@ fn running_job_snapshot_marks_no_output_stale_after_threshold() { "elapsed no-output time should be exposed: {job:?}" ); } - #[test] fn running_job_snapshot_keeps_recent_no_output_fresh() { let tmp = tempdir().expect("tempdir"); @@ -2448,7 +2365,6 @@ fn running_job_snapshot_keeps_recent_no_output_fresh() { assert!(!job.stale, "fresh running job should not start stale"); assert!(job.elapsed_since_output_ms.is_some()); } - #[test] fn test_job_cancel_updates_completion_state() { let tmp = tempdir().expect("tempdir"); @@ -2464,7 +2380,6 @@ fn test_job_cancel_updates_completion_state() { assert_eq!(job.snapshot.status, ShellStatus::Killed); assert!(!job.snapshot.stdin_available); } - #[test] fn test_output_truncation() { let long_output = "x".repeat(50_000); @@ -2473,7 +2388,6 @@ fn test_output_truncation() { assert!(truncated.len() < long_output.len()); assert!(truncated.contains("truncated")); } - #[test] fn test_truncate_with_meta_reports_omission_counts() { let long_output = format!("line1\nline2\n{}", "x".repeat(60_000)); @@ -2484,7 +2398,6 @@ fn test_truncate_with_meta_reports_omission_counts() { assert!(meta.omitted > 0); assert!(truncated.contains("bytes omitted")); } - #[test] fn network_restricted_hint_detects_silent_curl_failure() { let tmp = tempdir().expect("tempdir"); @@ -2500,7 +2413,6 @@ fn network_restricted_hint_detects_silent_curl_failure() { assert!(hint.contains("Plan mode")); } - #[test] fn sandbox_denied_hint_names_the_effective_posture() { // DGF-02: an approved write blocked by a read-only sandbox must come @@ -2522,7 +2434,6 @@ fn sandbox_denied_hint_names_the_effective_posture() { ); assert!(hint.contains("justification"), "{hint}"); } - #[test] fn contract_bash_denial_surfaces_the_escalation_shape() { let tmp = tempdir().expect("tempdir"); @@ -2541,7 +2452,6 @@ fn contract_bash_denial_surfaces_the_escalation_shape() { "{error}" ); } - #[test] fn sandbox_denied_hint_absent_without_denial_or_policy() { let tmp = tempdir().expect("tempdir"); @@ -2555,7 +2465,6 @@ fn sandbox_denied_hint_absent_without_denial_or_policy() { let no_policy_ctx = ToolContext::new(tmp.path()); assert!(shell_sandbox_denied_hint(&no_policy_ctx, &denied).is_none()); } - #[test] fn shell_delta_result_surfaces_sandbox_denied_hint() { let tmp = tempdir().expect("tempdir"); @@ -2584,7 +2493,6 @@ fn shell_delta_result_surfaces_sandbox_denied_hint() { let metadata = tool_result.metadata.expect("metadata"); assert!(metadata.get("sandbox_denied_hint").is_some()); } - #[test] fn network_restricted_hint_ignores_local_failures() { let tmp = tempdir().expect("tempdir"); @@ -2593,7 +2501,6 @@ fn network_restricted_hint_ignores_local_failures() { assert!(shell_network_restricted_hint(&ctx, "cat missing.txt", &result).is_none()); } - #[test] fn shell_delta_result_surfaces_network_restricted_hint() { let tmp = tempdir().expect("tempdir"); @@ -2620,7 +2527,6 @@ fn shell_delta_result_surfaces_network_restricted_hint() { Some(true) ); } - #[test] fn shell_delta_result_exposes_lossless_high_exit_code_and_hex() { let tmp = tempdir().expect("tempdir"); @@ -2649,7 +2555,6 @@ fn shell_delta_result_exposes_lossless_high_exit_code_and_hex() { assert_eq!(metadata["exit_code"], json!(3221225477_i64)); assert_eq!(metadata["exit_code_hex"], json!("0xC0000005")); } - #[test] fn shell_delta_result_surfaces_elapsed_time_in_content() { let tmp = tempdir().expect("tempdir"); @@ -2677,7 +2582,6 @@ fn shell_delta_result_surfaces_elapsed_time_in_content() { tool_result.content ); } - #[test] fn shell_delta_timing_line_omits_task_id_when_unknown() { let tmp = tempdir().expect("tempdir"); @@ -2701,7 +2605,6 @@ fn shell_delta_timing_line_omits_task_id_when_unknown() { tool_result.content ); } - #[test] fn shell_delta_timing_line_phrases_cover_terminal_statuses() { let tmp = tempdir().expect("tempdir"); @@ -2732,7 +2635,6 @@ fn shell_delta_timing_line_phrases_cover_terminal_statuses() { ); } } - #[test] fn shell_delta_timing_line_handles_zero_duration() { let tmp = tempdir().expect("tempdir"); @@ -2757,7 +2659,6 @@ fn shell_delta_timing_line_handles_zero_duration() { tool_result.content ); } - #[test] fn shell_delta_timing_line_sits_below_network_hint() { let tmp = tempdir().expect("tempdir"); @@ -2782,7 +2683,6 @@ fn shell_delta_timing_line_sits_below_network_hint() { "hint must precede timing line: {content}" ); } - #[test] fn shell_delta_result_includes_cargo_failure_summary() { let tmp = tempdir().expect("tempdir"); @@ -2833,7 +2733,6 @@ fn shell_delta_result_includes_cargo_failure_summary() { .contains("error: test failed") ); } - #[test] fn shell_delta_result_keeps_existing_summary_for_generic_cargo_failure() { let tmp = tempdir().expect("tempdir"); @@ -2873,7 +2772,6 @@ fn shell_delta_result_keeps_existing_summary_for_generic_cargo_failure() { json!("command failed without structured cargo diagnostics") ); } - #[test] fn shell_delta_result_surfaces_python_build_dependency_hint() { let tmp = tempdir().expect("tempdir"); @@ -2925,7 +2823,6 @@ fn shell_delta_result_surfaces_python_build_dependency_hint() { .contains("setuptools") ); } - #[test] fn test_summarize_output_strips_truncation_note() { let long_output = "x".repeat(60_000); @@ -2933,7 +2830,6 @@ fn test_summarize_output_strips_truncation_note() { let summary = summarize_output(&truncated); assert!(!summary.contains("Output truncated at")); } - #[tokio::test] async fn test_exec_shell_metadata_includes_summaries() { let tmp = tempdir().expect("tempdir"); @@ -2956,7 +2852,6 @@ async fn test_exec_shell_metadata_includes_summaries() { assert!(meta.get("stdout_len").is_some()); assert!(meta.get("stdout_truncated").is_some()); } - #[cfg(not(windows))] #[tokio::test] async fn test_exec_shell_combined_output_uses_single_stream() { @@ -2979,7 +2874,6 @@ async fn test_exec_shell_combined_output_uses_single_stream() { Some(true) ); } - #[tokio::test] async fn test_exec_shell_foreground_timeout_guides_background_rerun() { let tmp = tempdir().expect("tempdir"); @@ -3039,7 +2933,6 @@ async fn test_exec_shell_foreground_timeout_guides_background_rerun() { let recommended = recovery.to_string(); assert!(!recommended.contains("exec_shell"), "{recommended}"); } - #[test] fn background_schema_distinguishes_temporary_jobs_from_persistent_services() { let schema = BashTool::new("Bash").input_schema(); @@ -3048,7 +2941,6 @@ fn background_schema_distinguishes_temporary_jobs_from_persistent_services() { .expect("background description"); assert!(d.contains("killed") && d.contains("background:true") && d.contains("persist:true")); } - #[tokio::test] async fn test_exec_shell_foreground_cancel_kills_process() { let tmp = tempdir().expect("tempdir"); @@ -3083,7 +2975,6 @@ async fn test_exec_shell_foreground_cancel_kills_process() { assert_eq!(meta.get("status").and_then(Value::as_str), Some("Killed")); assert_eq!(meta.get("canceled").and_then(Value::as_bool), Some(true)); } - #[tokio::test] async fn test_exec_shell_foreground_can_move_to_background() { let tmp = tempdir().expect("tempdir"); @@ -3150,7 +3041,6 @@ async fn test_exec_shell_foreground_can_move_to_background() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } - #[tokio::test] async fn lowercase_bash_foreground_detach_is_a_successful_running_receipt() { let tmp = tempdir().expect("tempdir"); @@ -3194,7 +3084,6 @@ async fn lowercase_bash_foreground_detach_is_a_successful_running_receipt() { assert_eq!(job.snapshot.status, ShellStatus::Running); manager.kill(task_id).expect("kill test job"); } - #[tokio::test] async fn test_exec_shell_wait_cancel_leaves_background_process_running() { let tmp = tempdir().expect("tempdir"); @@ -3250,7 +3139,6 @@ async fn test_exec_shell_wait_cancel_leaves_background_process_running() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } - #[tokio::test] async fn test_completed_background_shell_releases_process_handles() { let tmp = tempdir().expect("tempdir"); @@ -3290,7 +3178,6 @@ async fn test_completed_background_shell_releases_process_handles() { assert!(shell.stdout_thread.is_none()); assert!(shell.stderr_thread.is_none()); } - #[cfg(unix)] #[tokio::test] async fn exec_shell_cancel_kills_descendant_process_group() { @@ -3339,7 +3226,6 @@ async fn exec_shell_cancel_kills_descendant_process_group() { "descendant {descendant} survived shell process-group cancellation" ); } - #[tokio::test] async fn test_exec_shell_cancel_tool_kills_background_process() { let tmp = tempdir().expect("tempdir"); @@ -3373,7 +3259,6 @@ async fn test_exec_shell_cancel_tool_kills_background_process() { let job = manager.inspect_job(task_id).expect("inspect job"); assert_eq!(job.snapshot.status, ShellStatus::Killed); } - #[tokio::test] async fn test_exec_shell_cancel_tool_can_kill_all_running_processes() { let tmp = tempdir().expect("tempdir"); @@ -3416,7 +3301,6 @@ async fn test_exec_shell_cancel_tool_can_kill_all_running_processes() { assert_eq!(first_job.snapshot.status, ShellStatus::Killed); assert_eq!(second_job.snapshot.status, ShellStatus::Killed); } - fn make_failed_result(stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -3436,7 +3320,6 @@ fn make_failed_result(stderr: &str) -> ShellResult { stderr_truncated: false, } } - #[test] fn test_macos_provenance_detected_by_activity_time_message() { let result = make_failed_result( @@ -3445,7 +3328,6 @@ fn test_macos_provenance_detected_by_activity_time_message() { ); assert!(looks_like_macos_provenance_failure(&result)); } - #[test] fn test_macos_provenance_detected_by_activity_path_and_eperm() { let result = make_failed_result( @@ -3453,7 +3335,6 @@ fn test_macos_provenance_detected_by_activity_path_and_eperm() { ); assert!(looks_like_macos_provenance_failure(&result)); } - #[test] fn test_macos_provenance_not_triggered_on_success() { let mut result = make_failed_result( @@ -3464,13 +3345,11 @@ fn test_macos_provenance_not_triggered_on_success() { result.exit_code = Some(0); assert!(!looks_like_macos_provenance_failure(&result)); } - #[test] fn test_macos_provenance_not_triggered_on_unrelated_eperm() { let result = make_failed_result("open /some/other/path: operation not permitted"); assert!(!looks_like_macos_provenance_failure(&result)); } - // Regression test for #828: shell spawns an orphaned background subprocess // (simulating `nohup curl`) that keeps the pipe write-end open after the shell // exits. collect_output() must not block indefinitely — it kills the whole @@ -3493,7 +3372,6 @@ fn test_orphaned_subprocess_does_not_block_collect_output() { .expect("get_output must complete, not hang"); assert_eq!(done.status, ShellStatus::Completed); } - #[cfg(unix)] #[test] fn foreground_shell_does_not_block_on_orphaned_subprocess_pipe() { @@ -3510,7 +3388,6 @@ fn foreground_shell_does_not_block_on_orphaned_subprocess_pipe() { ); assert_eq!(result.status, ShellStatus::Completed); } - // Windows equivalent of the orphaned pipe-handle regression. `cmd /c start /b` // launches a descendant process that inherits stdout/stderr and outlives the // shell. Job-object cleanup must terminate that descendant before reader-thread @@ -3542,7 +3419,6 @@ fn background_collection_does_not_block_on_detached_descendant_pipe() { ); assert_eq!(done.status, ShellStatus::Completed); } - #[cfg(windows)] #[test] fn windows_job_terminate_denied_falls_back_to_child_kill() { @@ -3573,7 +3449,6 @@ fn windows_job_terminate_denied_falls_back_to_child_kill() { "fallback child kill should terminate child" ); } - #[cfg(windows)] #[test] fn windows_job_close_releases_foreground_reader_threads_when_terminate_denied() { @@ -3621,7 +3496,6 @@ fn windows_job_close_releases_foreground_reader_threads_when_terminate_denied() ); assert!(status.is_some(), "kill-on-close should terminate child"); } - #[cfg(windows)] #[test] fn windows_job_kill_on_close_releases_reader_threads_when_terminate_denied() { @@ -3663,7 +3537,6 @@ fn windows_job_kill_on_close_releases_reader_threads_when_terminate_denied() { ); assert_eq!(done.status, ShellStatus::Completed); } - #[cfg(windows)] #[test] fn killed_shell_does_not_wait_for_blocked_reader_threads() { @@ -3717,7 +3590,6 @@ fn killed_shell_does_not_wait_for_blocked_reader_threads() { ); release_tx.send(()).expect("release detached reader"); } - #[test] fn test_list_jobs_cleans_up_completed_old_processes() { let tmp = tempdir().expect("tempdir"); @@ -3738,7 +3610,6 @@ fn test_list_jobs_cleans_up_completed_old_processes() { "completed processes should be evicted by cleanup" ); } - /// Regression for #1691: a `git commit -m "feat: complete sub-pages"` shell /// command must reach the OS shell with its quoted message intact (one argv /// slot), never split into `feat:` / `complete` / `sub-pages"`. @@ -3783,7 +3654,6 @@ fn issue_1691_quoted_commit_message_round_trips() { .collect(); assert_eq!(got, spec.args); } - /// When no `cwd` is provided, the shell should run in `context.workspace`, /// not in the ShellManager's default_workspace. This ensures sub-agents in /// worktrees run commands in the worktree directory rather than the parent. @@ -3836,14 +3706,12 @@ async fn default_cwd_uses_context_workspace_not_shell_manager_default() { // escaped descendant wedged the blocking reader-thread join inside kill() // until the descendant exited on its own (observed: ~180s past a 120s // timeout in the wild). - #[cfg(unix)] const SHELL_SIGTERM_HELPER_ENV: &str = "CODEWHALE_SHELL_SIGTERM_HELPER"; #[cfg(unix)] const SHELL_ESCAPE_HELPER_ENV: &str = "CODEWHALE_SHELL_ESCAPE_HELPER"; #[cfg(unix)] const SHELL_ESCAPED_GRANDCHILD_ENV: &str = "CODEWHALE_SHELL_ESCAPED_GRANDCHILD"; - /// Helper role: ignore SIGTERM and idle. Runs as the shell's direct child /// (same process group), so only the SIGKILL escalation can stop it. #[cfg(unix)] @@ -3863,7 +3731,6 @@ fn shell_sigterm_ignoring_helper_process() { std::thread::sleep(Duration::from_millis(10)); } } - /// Helper role: spawn a grandchild in its OWN process group (escaping the /// shell's group) that inherits the output pipe, then exit immediately. The /// wrapper shell keeps running (`sleep` after `&`), so the job stays Running @@ -3892,7 +3759,6 @@ fn shell_group_escape_helper_process() { cmd.process_group(0); let _child = cmd.spawn().expect("spawn escaped grandchild"); } - /// Helper role: the escaped grandchild — ignores SIGTERM, reports its pid, /// then idles (holding the inherited output pipe open the whole time). #[cfg(unix)] @@ -3909,7 +3775,6 @@ fn shell_escaped_grandchild_helper_process() { std::fs::write(pid_file, std::process::id().to_string()).expect("write grandchild pid"); std::thread::sleep(Duration::from_secs(30)); } - /// Required regression: a foreground command that ignores SIGTERM must be /// dead and the tool must have returned within timeout + a small grace /// (2s timeout, assert wall < 10s). @@ -3949,7 +3814,6 @@ async fn foreground_timeout_kills_sigterm_ignoring_command_within_grace() { "SIGTERM-ignoring helper {helper_pid} survived the timeout kill" ); } - /// Regression for the ~180s kill-path overshoot: a descendant that escaped /// the process group keeps the output pipe open after the group is killed. /// kill() must still return within a bounded grace instead of blocking on @@ -3991,7 +3855,6 @@ async fn kill_returns_promptly_when_escaped_descendant_holds_pipe_open() { } assert!(wait_for_shell_pid_exit(grandchild)); } - /// `Bash` was the only action wrapper whose catch-all fell through to its most /// dangerous branch: an unrecognised action ran the command instead. #[tokio::test] @@ -4020,7 +3883,6 @@ async fn unknown_bash_action_is_refused_instead_of_running_the_command() { ); assert!(!marker.exists(), "the command must not have run"); } - /// The same hole one type down. `and_then(as_str).unwrap_or("run")` read a /// non-string `action` as absent and fell through to the branch that executes /// arbitrary code, so `Bash{action: 3, command: "…"}` ran the command. `File`, @@ -4056,7 +3918,6 @@ async fn non_string_bash_action_is_refused_instead_of_running_the_command() { assert!(!marker.exists(), "the command must not have run: {action}"); } } - /// The same hole for the data fields (2026-08-04 review). A non-string /// `stdin` was silently dropped — the command ran with NO stdin and reported /// success, the silent-drop failure this lane exists to close. A non-string @@ -4102,7 +3963,6 @@ async fn wrongly_typed_stdin_cwd_and_task_id_are_refused_not_dropped() { "a supplied-but-mistyped task_id must not read as missing: {message}" ); } - /// `null` is the wire spelling of absence, and `action` documents a `run` /// default — so the strictness above must not swallow the default. #[tokio::test] @@ -4122,7 +3982,6 @@ async fn absent_or_null_bash_action_still_defaults_to_run() { assert!(result.content.contains("defaulted"), "{}", result.content); } } - /// Negative case for the strictness above: every legitimate action still /// dispatches to its own handler rather than the action refusal. `wait`, /// `interact`, and `cancel` are checked by the error they raise *after* @@ -4165,7 +4024,6 @@ async fn every_valid_bash_action_still_dispatches() { .expect("action=cancel all=true must dispatch"); assert!(cancelled.success, "{}", cancelled.content); } - /// The stdin aliases were real but undocumented: a model that wrote `input` /// or `data` got them honoured with nothing in the schema saying so, and a /// maintainer reading the schema would have removed them as dead. Advertise @@ -4212,7 +4070,6 @@ async fn every_advertised_stdin_spelling_reaches_the_command() { "task_id must name its alias" ); } - /// The schema declared no `required` key at all, so `Bash{}` — no command, no /// task — was schema-valid for the tool that runs shell commands. What is /// required is per-action, so it is spelled as root `anyOf` required groups, @@ -4251,7 +4108,6 @@ fn bash_schema_declares_what_each_action_requires() { } } } - /// A root `anyOf` is not portable to every provider, so prove the fallback /// the sanitizer promises: Responses/xAI drop root composition, and the /// constraint has to survive as a description note rather than vanishing. @@ -4269,7 +4125,6 @@ fn bash_required_groups_survive_a_provider_that_drops_root_composition() { assert!(schema.get("anyOf").is_none(), "root anyOf must be removed"); assert!(schema["properties"]["command"].is_object()); } - /// Every hint in this file has to name a tool the model can actually call. /// `exec_shell` / `exec_shell_wait` were retired in v0.9.3. #[test] @@ -4278,7 +4133,6 @@ fn shell_recovery_hints_name_only_dispatchable_tools() { assert!(FOREGROUND_TIMEOUT_RECOVERY_HINT.contains("Bash")); assert!(FOREGROUND_TIMEOUT_RECOVERY_HINT.contains("action=\"wait\"")); } - /// One documented default hid three real ones: `wait` uses 30s and /// `interact` 1s, so a model omitting `timeout_ms` on `wait` got a quarter of /// the timeout the schema promised. @@ -4296,7 +4150,6 @@ fn timeout_ms_description_covers_every_action_default() { ); } } - #[cfg(unix)] fn authorized_persistent_service_context(workspace: &Path) -> ToolContext { let mut context = ToolContext::new(workspace.to_path_buf()) @@ -4307,13 +4160,11 @@ fn authorized_persistent_service_context(workspace: &Path) -> ToolContext { context.auto_approve = true; context } - #[cfg(unix)] fn persistent_service_test_lock() -> &'static tokio::sync::Mutex<()> { static LOCK: OnceLock> = OnceLock::new(); LOCK.get_or_init(|| tokio::sync::Mutex::new(())) } - #[cfg(unix)] #[tokio::test] async fn persistent_service_requires_explicit_headless_exec_authority() { @@ -4335,7 +4186,6 @@ async fn persistent_service_requires_explicit_headless_exec_authority() { assert!(error.to_string().contains("real headless `codewhale exec`")); } - #[cfg(unix)] #[tokio::test] async fn committed_persistent_service_survives_manager_drop_and_reports_identity() { @@ -4390,7 +4240,6 @@ async fn committed_persistent_service_survives_manager_drop_and_reports_identity "released service must survive Codewhale manager teardown" ); } - #[cfg(unix)] #[tokio::test] async fn signal_cleanup_kills_staged_persistent_service_group() { @@ -4446,13 +4295,11 @@ async fn signal_cleanup_kills_staged_persistent_service_group() { } // === #5472: in-memory retention must be bounded === - /// ~1.1 MB on stdout, fast: 30,000 lines of 37 bytes. #[cfg(unix)] fn chatty_command() -> String { "yes 0123456789abcdefghijklmnopqrstuvwxyz | head -n 30000".to_string() } - /// The finding-1 regression: before this bound, a single uppercase `Bash` call /// left its entire stdout resident in `ShellManager.processes` until the 1 h /// `cleanup`, and only if the user happened to open the jobs panel. @@ -4482,7 +4329,6 @@ async fn foreground_bash_releases_its_output_once_the_result_is_returned() { RAW_STREAM_SETTLED_TAIL_BYTES * 2 ); } - /// Releasing memory must not rewrite history: the job panel keeps reporting how /// much the command actually printed. #[cfg(unix)] @@ -4508,7 +4354,6 @@ async fn released_output_still_reports_the_real_stream_length() { "a diagnostic tail must survive the release" ); } - /// A background job's bytes become a durable session artifact at drain time; /// keeping a second copy in the manager afterwards is the pure-waste term. #[cfg(unix)] @@ -4558,7 +4403,6 @@ async fn draining_completion_evidence_releases_the_retained_copy() { "{retained} bytes still held after the evidence was published" ); } - /// Age was the only bound, and it only ran from `list_jobs()`. Hundreds of /// finished records inside one hour were all retained. #[test] @@ -4591,7 +4435,6 @@ fn cleanup_bounds_finished_records_by_count() { "the newest record must survive" ); } - /// #5478 nit: `/jobs` reported `2m 07s` for a 12-second command, because /// elapsed was `started_at.elapsed()` even after the job finished. A completed /// job reports the duration it finished with. @@ -4640,7 +4483,6 @@ fn a_finished_job_reports_its_duration_not_a_growing_elapsed() { "the frozen value must be the real duration, not the timeout: {second}ms" ); } - // The Windows execution-policy fallback: the temp `-File` script is refused // before any statement runs, so the tool re-runs the identical payload as an // inline `-EncodedCommand`. The predicate is what decides "refused by the @@ -4750,7 +4592,6 @@ fn forkguard_powershell_execution_policy_rejection_is_locale_independent() { )); assert!(!powershell_execution_policy_rejection("", "", script)); } - // End-to-end: the encoded form really runs the payload the temp `-File` form // was written for (multiline + non-ASCII), with no file on disk. #[cfg(windows)] @@ -4783,7 +4624,6 @@ fn forkguard_powershell_encoded_spec_runs_a_non_ascii_payload() { assert!(stdout.contains("编码-ok"), "stdout={stdout}"); assert!(stdout.contains("line2"), "stdout={stdout}"); } - fn failed_result_with(status: ShellStatus, stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -4803,7 +4643,6 @@ fn failed_result_with(status: ShellStatus, stderr: &str) -> ShellResult { sandbox_denied: false, } } - fn temp_file_spec(script: &std::path::Path, cwd: &std::path::Path) -> CommandSpec { CommandSpec::program( "pwsh", @@ -4820,7 +4659,6 @@ fn temp_file_spec(script: &std::path::Path, cwd: &std::path::Path) -> CommandSpe Duration::from_secs(5), ) } - // The retry gate: only a failed temp `-File` refusal may be retried, so a // cancellation or a timeout can never re-run a command the user stopped. #[test] @@ -4882,7 +4720,6 @@ fn forkguard_powershell_inline_retry_only_for_a_failed_file_refusal() { ) )); } - // The glue: the retry helper re-runs the identical payload inline, annotates // the fallback, and accounts for both attempts. #[cfg(windows)] @@ -4920,7 +4757,6 @@ fn forkguard_powershell_inline_retry_runs_the_command_and_notes_the_fallback() { retried.duration_ms ); } - // End-to-end: the normal temp `-File` form still carries the process-scoped // policy bypass, so a restricted machine runs the script this tool wrote. #[cfg(windows)] @@ -4951,7 +4787,6 @@ fn forkguard_powershell_temp_script_form_runs_a_non_ascii_payload() { assert!(stdout.contains("编码-ok"), "stdout={stdout}"); assert!(stdout.contains("line2"), "stdout={stdout}"); } - // A recovery path that cannot even start must not replace the refusal that // sent it there: the first attempt's evidence is what the user needs, so it is // kept and annotated with the retry failure. @@ -5019,6 +4854,604 @@ fn bounded_reap_reports_grace_expiry_while_the_child_lives() { assert!( !wait_child_bounded(&mut child, Duration::from_millis(50)), "a live child must report the exit as unconfirmed within the grace" + +/// An inline stdin payload past one pipe buffer is refused before spawn: +/// the payload is delivered with a blocking write while the shell-manager +/// lock is held, and a non-reading child would wedge the executor past any +/// watchdog. The boundary payload (exactly 64 KiB) still runs. The raw +/// lane is the one that carries payloads — the read-only grammar rejects +/// a `stdin` field outright. +// Unix-only: the boundary leg executes `wc -c`, which the sanitized child +// PATH does not resolve on Windows (and the deadline semantics the +// boundary payload rides are unix — see `write_stdin_…` above). +#[cfg(unix)] +#[tokio::test] +async fn inline_stdin_payload_over_the_cap_refuses() { + let workspace = tempdir().expect("workspace"); + let context = ToolContext::new(workspace.path().to_path_buf()); + let oversized = "x".repeat(super::MAX_INLINE_STDIN_BYTES + 1); + let output = BashTool::new("Bash") + .execute(json!({"command": "wc -c", "stdin": oversized}), &context) + .await + .expect("the refusal is a result, not an error"); + assert!(!output.success, "{}", output.content); + assert!( + output.content.contains("action=interact"), + "the refusal must point at the streaming alternative: {}", + output.content + ); + + let boundary = "y".repeat(super::MAX_INLINE_STDIN_BYTES); + let output = BashTool::new("Bash") + .execute(json!({"command": "wc -c", "stdin": boundary}), &context) + .await + .expect("the boundary payload dispatches"); + assert!(output.success, "{}", output.content); + // Exact equality: a `contains` anchor would stay green if the cap + // moved to a value whose spelling embeds this one (655360 contains + // "65536"). + assert_eq!( + output.content.trim(), + super::MAX_INLINE_STDIN_BYTES.to_string(), + "the boundary payload must reach the command whole: {}", + output.content + ); +} +/// The bounded stdin write: a command that never consumes its pipe fills +/// it, and past [`STDIN_WRITE_DEADLINE`] the write errors and the task's +/// stdin is marked stalled instead of holding the shell-manager lock (and +/// with it every other shell in the session) forever. The stall flag then +/// refuses further writes without another wait — two writers could +/// interleave on one pipe. +// Unix-only: the bounded write (worker + deadline) the test exercises is +// `#[cfg(unix)]`; on Windows the write is deliberately blocking under the +// per-call cap, so the non-reading child would wedge to this test's own +// 30 s guard instead of erroring. +#[cfg(unix)] +#[test] +fn write_stdin_to_a_child_that_stops_consuming_errors_instead_of_wedging() { + let (sender, receiver) = std::sync::mpsc::channel::>(); + let handle = std::thread::spawn(move || { + let run = || -> Result<(), String> { + let tmp = tempdir().map_err(|error| error.to_string())?; + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + let result = manager + .execute_with_options_env( + &sleep_command(60), + None, + 60_000, + true, + None, + false, + 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())?; + // Far past any pipe buffer on any platform: the write cannot + // complete unless the child reads, and it never does. + let payload = "x".repeat(200 * 1024); + let started = std::time::Instant::now(); + let error = manager + .write_stdin(&task_id, &payload, false) + .err() + .ok_or_else(|| "the stalled write must error at the deadline".to_string())? + .to_string(); + if !error.contains("did not complete within") { + return Err(error); + } + let elapsed = started.elapsed(); + if elapsed < STDIN_WRITE_DEADLINE { + return Err(format!( + "the write returned before the deadline: {elapsed:?}" + )); + } + // No upper bound on the elapsed time: the load-bearing assertions + // are that the deadline FIRED (elapsed ≥ deadline) and further + // writes are refused. A fixed upper bound is load-fragile at any + // multiple — under a saturated full-suite parallel run the + // deadline worker was observed starving past 30 s — while a + // broken-mechanism mutant (deadline removed) never returns and + // trips the outer hang guard instead. + // Further writes are refused by the stall flag without waiting. + let started = std::time::Instant::now(); + let again = manager.write_stdin(&task_id, "y", false); + if again.is_ok() { + return Err("the stalled task must refuse further writes".to_string()); + } + if started.elapsed() > std::time::Duration::from_millis(500) { + return Err("the stall refusal must not wait on another write".to_string()); + } + // A close after the stall must not claim the end-of-file the + // abandoned writer never delivered: the post-close refusal must + // still name the stall (the `stdin_closed_at_eof` stall conjunct + // and the stall-first check order together keep the EOF claim + // off; a reorder or conjunct drop that surfaced the EOF message + // reddens here). + let _closed = manager.write_stdin(&task_id, "", true); + let after_close = manager + .write_stdin(&task_id, "z", false) + .err() + .ok_or_else(|| "the post-stall close must leave writes refused".to_string())? + .to_string(); + if !after_close.contains("stalled") || after_close.contains("end-of-file") { + return Err(format!( + "a close after a stall must not claim an EOF the abandoned writer has not delivered: {after_close}" + )); + } + manager.kill(&task_id).map_err(|error| error.to_string())?; + Ok(()) + }; + let verdict = run(); + let _ = sender.send(verdict.clone()); + verdict + }); + let _ = receiver + .recv_timeout(std::time::Duration::from_secs(30)) + .expect("the bounded-write pin hung: the STDIN_WRITE_DEADLINE regression is live"); + handle + .join() + .expect("the bounded-write worker panicked") + .expect("bounded-write assertions"); +} +/// The `interact` payload carries the same per-call bound as the inline +/// spawn payload: one oversized call is refused with a pointer at chunking +/// before any write is attempted. +#[tokio::test] +async fn interact_payload_over_the_cap_refuses() { + let workspace = tempdir().expect("workspace"); + let context = ToolContext::new(workspace.path().to_path_buf()); + let oversized = "x".repeat(super::MAX_INLINE_STDIN_BYTES + 1); + let output = BashTool::new("Bash") + .execute( + json!({"action": "interact", "command": "pwd", "task_id": "no-such-task", "stdin": oversized}), + &context, + ) + .await + .expect("the refusal is a result, not an error"); + assert!(!output.success, "{}", output.content); + assert!( + output.content.contains("per interact call"), + "the refusal must name the per-call bound: {}", + output.content + ); + // The advertised bound stays in sync with the constant (the copy + // interpolates it; this asserts the value actually reaches the model). + assert!( + output + .content + .contains(&super::MAX_INLINE_STDIN_BYTES.to_string()), + "the refusal must carry the exact bound value: {}", + output.content + ); +} +/// The tool description states the foreground stdin contract; the sentence is +/// the model-facing contract for why stdin readers exit, so it is pinned. +#[test] +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"), + "guidance must state the bounded foreground stdin: {description}" + ); + assert!( + description.contains("`tty` always selects a background terminal task"), + "guidance must state where tty commands actually run — the run path forces them background: {description}" + ); + assert!( + description.contains( + "a terminal lane that nothing writes to unless a session member interacts with the task" + ), + "guidance must state the combined_output foreground stdin contract accurately: {description}" + ); + assert!( + description.contains("action=\"interact\""), + "guidance must point at background interact for input-driven work: {description}" + ); +} +/// A foreground command that reads stdin with NO stdin data used to wedge on +/// a pipe whose write end the tool kept open: the live repro was `$SHELL` +/// holding the turn until the foreground watchdog killed it at the full +/// timeout (`cat` behaves identically). The foreground lane must deliver EOF +/// at spawn instead. Background keeps the pipe open for deferred +/// `interact` streaming — that contract is pinned by +/// `test_write_stdin_streams_output` and must not regress. +#[tokio::test] +async fn foreground_without_stdin_data_gives_stdin_readers_eof() { + let workspace = tempdir().expect("workspace"); + let context = ToolContext::new(workspace.path().to_path_buf()); + // `cat` on Unix, `more` on cmd, `[Console]::In.ReadToEnd()` on PowerShell — + // each blocks until stdin reaches EOF. + let echo_stdin = echo_stdin_command(); + + let started = Instant::now(); + let result = BashTool::new("Bash") + .execute(json!({"command": echo_stdin, "timeout_ms": 3000}), &context) + .await + .expect("foreground stdin-reader must spawn"); + let elapsed = started.elapsed(); + + assert!( + result.success, + "a stdin-reading foreground command with no stdin data must see EOF \ + and exit promptly, not run to the timeout; took {elapsed:?}: {}", + result.content + ); + assert!( + elapsed < Duration::from_millis(2900), + "EOF must be delivered right after spawn, not discovered at the \ + timeout; took {elapsed:?}: {}", + result.content + ); +} +#[cfg(unix)] +const SYNC_STDIN_EOF_HELPER_ENV: &str = "CODEWHALE_SYNC_STDIN_EOF_HELPER"; +#[cfg(unix)] +const SYNC_STDIN_EOF_RESULT_FILE_ENV: &str = "CODEWHALE_SYNC_STDIN_EOF_RESULT_FILE"; +/// Helper half of +/// `sync_lane_without_stdin_data_gets_eof_not_inherited_stdin`: runs one +/// synchronous (background=false) stdin-reading command from a process whose +/// own stdin is a pipe the driver deliberately holds open, and records the +/// resulting status for the driver to judge. +#[cfg(unix)] +#[test] +fn sync_lane_stdin_eof_helper_process() { + if std::env::var(SYNC_STDIN_EOF_HELPER_ENV).ok().as_deref() != Some("1") { + return; + } + let result = ShellManager::new(std::env::temp_dir()) + .execute_with_options_env( + &echo_stdin_command(), + None, + 10_000, + false, + None, + false, + None, + HashMap::new(), + ) + .expect("sync execute in helper"); + std::fs::write( + PathBuf::from( + std::env::var(SYNC_STDIN_EOF_RESULT_FILE_ENV).expect("sync stdin result file"), + ), + format!("{:?}", result.status), + ) + .expect("write sync stdin result file"); +} +/// The sync lane's old default left the child inheriting the host's stdin +/// (std's `Stdio::inherit()`), so on a live TTY a stdin-reading command hung +/// or drew SIGTTIN. Reproduce deterministically: re-exec this test binary as +/// a helper whose own stdin is a pipe this process holds open, and run the +/// sync lane inside it. An inherited pipe can never EOF; `Stdio::null()` +/// gives the child an immediate end-of-file. +#[cfg(unix)] +#[test] +fn sync_lane_without_stdin_data_gets_eof_not_inherited_stdin() { + let tmp = tempdir().expect("tempdir"); + let result_file = tmp.path().join("sync-stdin-result"); + let test_binary = std::env::current_exe().expect("current test binary"); + let mut helper = Command::new(&test_binary) + .arg("--exact") + .arg("tools::shell::tests::sync_lane_stdin_eof_helper_process") + .arg("--nocapture") + .env(SYNC_STDIN_EOF_HELPER_ENV, "1") + .env(SYNC_STDIN_EOF_RESULT_FILE_ENV, &result_file) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sync stdin helper"); + // Hold the write end for the whole run: while it is open, an inherited + // stdin can never reach EOF, which is exactly the old hang. + let held_stdin = helper.stdin.take().expect("helper stdin pipe"); + let deadline = Instant::now() + Duration::from_secs(60); + let status = loop { + match helper.try_wait().expect("poll sync stdin helper") { + Some(status) => break status, + None => { + assert!( + Instant::now() < deadline, + "sync-lane helper never finished; the lane is inheriting \ + a never-EOF stdin" + ); + std::thread::sleep(Duration::from_millis(50)); + } + } + }; + drop(held_stdin); + assert!(status.success(), "sync stdin helper failed: {status}"); + let outcome = std::fs::read_to_string(&result_file).expect("sync lane result file"); + assert_eq!( + outcome, "Completed", + "a sync command reading stdin with no stdin data must get EOF, not \ + inherited host stdin" + ); +} +/// The foreground stdin contract survives a detach: the command keeps +/// running in the background, but its stdin was put at end-of-file at spawn +/// (so stdin readers exit instead of wedging), and a later `interact` says so +/// and points at the background lane instead of failing opaquely. +#[tokio::test] +async fn detached_foreground_command_reports_its_closed_stdin_on_interact() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path()); + let shell_manager = ctx.shell_manager.clone(); + let command = sleep_command(30); + let task_ctx = ctx.clone(); + + let task = tokio::spawn(async move { + BashTool::new("Bash") + .execute( + json!({"command": command, "timeout_ms": 600_000}), + &task_ctx, + ) + .await + .expect("execute") + }); + tokio::time::sleep(Duration::from_millis(150)).await; + shell_manager + .lock() + .expect("shell manager lock") + .request_foreground_background(); + let result = tokio::time::timeout(Duration::from_secs(5), task) + .await + .expect("foreground shell should detach") + .expect("task should not panic"); + let task_id = result + .metadata + .as_ref() + .and_then(|meta| meta.get("task_id")) + .and_then(Value::as_str) + .expect("task id") + .to_string(); + + let mut manager = shell_manager.lock().expect("shell manager lock"); + assert_eq!( + manager + .inspect_job(&task_id) + .expect("inspect job") + .snapshot + .status, + ShellStatus::Running, + "the detached command keeps running" + ); + let error = manager + .write_stdin(&task_id, "hello\n", false) + .expect_err("stdin was closed at spawn by the foreground contract") + .to_string(); + assert!(error.contains("end-of-file"), "{error}"); + assert!(error.contains("background=true"), "{error}"); + let killed = manager.kill(&task_id).expect("kill"); + assert_eq!(killed.status, ShellStatus::Killed); +} +/// The foreground stdin-EOF close is deliberately pipe-only: a terminal +/// (`combined_output`) foreground command keeps its PTY writer attached so +/// a session member can interact with it while it runs — the base behavior +/// on that lane, which an unconditional close silently destroyed (and the +/// guidance sentence describes). +#[tokio::test] +#[cfg(all(unix, not(target_env = "ohos")))] +async fn foreground_terminal_command_keeps_its_interact_surface() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path()); + let shell_manager = ctx.shell_manager.clone(); + let command = echo_stdin_command(); + let task_ctx = ctx.clone(); + + let task = tokio::spawn(async move { + BashTool::new("Bash") + .execute( + json!({"command": command, "combined_output": true, "timeout_ms": 600_000}), + &task_ctx, + ) + .await + .expect("execute") + }); + tokio::time::sleep(Duration::from_millis(300)).await; + shell_manager + .lock() + .expect("shell manager lock") + .request_foreground_background(); + let result = tokio::time::timeout(Duration::from_secs(5), task) + .await + .expect("foreground shell should detach") + .expect("task should not panic"); + let task_id = result + .metadata + .as_ref() + .and_then(|meta| meta.get("task_id")) + .and_then(Value::as_str) + .expect("task id") + .to_string(); + + let mut manager = shell_manager.lock().expect("shell manager lock"); + { + let snapshot = manager.inspect_job(&task_id).expect("inspect job").snapshot; + assert_eq!( + snapshot.status, + ShellStatus::Running, + "the terminal command keeps running (exit {:?}, out {:?}, err {:?})", + snapshot.exit_code, + snapshot.stdout_tail, + snapshot.stderr_tail + ); + } + // The interact surface must still be attached: a session member writes + // to the running terminal command and the input reaches it (the tty + // echoes, and `cat` reads the line and writes it back). + manager + .write_stdin(&task_id, "hello-round14\n", false) + .expect("the terminal lane keeps a writable stdin while the command runs"); + let mut delivered = 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("hello-round14") + || snapshot.stderr_tail.contains("hello-round14") + { + delivered = true; + break; + } + } + assert!( + delivered, + "the interact write must reach the terminal command: {:?}", + manager.inspect_job(&task_id).expect("inspect job").snapshot + ); + manager.kill(&task_id).expect("kill"); +} +/// The unix PTY interact write rides the same bounded worker as a pipe +/// write: a tty command that stops consuming input fills its line- +/// discipline buffer, the write hits the deadline, the payload is abandoned +/// to a background writer, and further writes are refused by the stall +/// flag — instead of blocking the session's shell-manager lock forever. +#[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 tmp = tempdir().map_err(|error| error.to_string())?; + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + let result = manager + // A RAW-mode tty is the shape whose full input queue + // backpressures the master write (canonical mode discards + // overflow instead): a raw `sleep` that never reads pins the + // writer until the deadline. + .execute_with_options_env( + // The marker prints only after `stty` succeeds, so the + // wait below also turns a stty failure into an honest + // setup error instead of a misleading write success. + "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())?; + // Raw mode must be established before the write lands: on a fast + // or loaded host the background shell may not have run `stty` + // yet, and a payload written in canonical mode drains (overflow + // is discarded there) instead of backpressuring — the write then + // completes and the deadline pin never gets to fire. That race + // failed the first CI run of this pin. + 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 + ); + // Far past any line-discipline input buffer: the write cannot + // complete unless the child reads, and it never does. + let payload = "x".repeat(200 * 1024); + let started = std::time::Instant::now(); + 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); + } + let elapsed = started.elapsed(); + if elapsed < STDIN_WRITE_DEADLINE { + return Err(format!( + "the write returned before the deadline: {elapsed:?}" + )); + } + // No upper bound on the elapsed time: the load-bearing assertions + // are that the deadline FIRED (elapsed ≥ deadline) and further + // writes are refused. A fixed upper bound is load-fragile at any + // multiple — under a saturated full-suite parallel run the + // deadline worker was observed starving past 30 s — while a + // broken-mechanism mutant (deadline removed) never returns and + // trips the outer hang guard instead. + let started = std::time::Instant::now(); + let again = manager.write_stdin(&task_id, "y", false); + if again.is_ok() { + return Err("the stalled tty task must refuse further writes".to_string()); + } + if started.elapsed() > std::time::Duration::from_millis(500) { + return Err("the stall refusal must not wait on another write".to_string()); + } + 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:?}"); +} +/// 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 +/// reports TimedOut with the writer left to the abandoned worker — the +/// caller's lock is never held across the deadline. Present on every +/// target; production unix keeps the fd-clone path, so this exercise is +/// test-only there. +#[cfg(unix)] +#[test] +fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { + use std::process::{Command, Stdio}; + + let mut child = Command::new("cat") + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .spawn() + .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()); + result.expect("a consuming child accepts the payload"); + assert!(returned.is_some(), "the writer is handed back on success"); + drop(returned); + child.wait().expect("cat exits after its stdin closes"); + + let mut child = Command::new("sleep") + .arg("120") + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .spawn() + .expect("sleep spawns"); + let stdin = child.stdin.take().expect("stdin piped"); + 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); + let elapsed = started.elapsed(); + let error = result.expect_err("a non-reading child must time the write out"); + assert_eq!(error.kind(), std::io::ErrorKind::TimedOut, "{error}"); + assert!( + returned.is_none(), + "a timed-out writer stays with the abandoned worker" + ); + assert!( + elapsed >= super::STDIN_WRITE_DEADLINE, + "the timeout must honor the deadline, not fire early: {elapsed:?}" ); let _ = child.kill(); let _ = child.wait(); diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index aba4d275b2..5632c32fea 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -568,7 +568,7 @@ fn start_background_gates( .shell_manager .lock() .map_err(|_| ToolError::execution_failed("shell manager lock poisoned"))?; - manager.execute_with_options_env_for_session( + let spawned = manager.execute_with_options_env_for_session( &command, Some(&cwd), BACKGROUND_GATE_TIMEOUT_MS, @@ -578,7 +578,20 @@ fn start_background_gates( context.elevated_sandbox_policy.clone(), env, &context.state_namespace, - ) + ); + // A gate job is machine-launched with nobody to feed its stdin, + // so hand the child end-of-file at once: a gate that reads stdin + // then exits instead of idling on an open pipe until the + // background timeout kills it. Idempotent-safe if the job already + // finished. + if let Some(task_id) = spawned + .as_ref() + .ok() + .and_then(|result| result.task_id.as_deref()) + { + manager.write_stdin(task_id, "", true).ok(); + } + spawned }; match spawn_result { @@ -1588,4 +1601,59 @@ mod tests { output.stdout ); } + + /// A machine-launched gate job has nobody to feed its stdin, so the + /// spawn closes it at once: a gate that reads stdin (`cat`) must + /// complete on the delivered end-of-file instead of idling on the + /// held-open pipe until the background gate timeout kills it. This + /// pins the `write_stdin(task_id, "", true)` close in the + /// background-gate spawn loop; without it this test reddens after the + /// completion wait finds the job still running. + #[cfg(unix)] + #[tokio::test] + async fn run_verifiers_background_gate_jobs_get_stdin_eof_at_spawn() { + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path()); + let tool = RunVerifiersTool; + let result = tool + .execute( + json!({ + "profile": "auto", + "background": true, + "commands": [ + {"name": "stdin-reader", "program": "cat", "args": []} + ] + }), + &ctx, + ) + .await + .expect("execute"); + + let parsed: RunVerifiersBackgroundOutput = + serde_json::from_str(&result.content).expect("background verifier output json"); + assert!(parsed.success, "result: {}", result.content); + assert_eq!(parsed.started, 1); + let task_id = parsed.jobs[0] + .task_id + .as_deref() + .expect("background task id"); + + let output = wait_for_completed_shell( + &mut ctx.shell_manager.lock().expect("shell manager"), + task_id, + ); + if output.status == ShellStatus::Running { + // Clean up the mutated behavior's wedged job before failing. + let _ = ctx + .shell_manager + .lock() + .expect("shell manager") + .kill(task_id); + } + assert_eq!( + output.status, + ShellStatus::Completed, + "a stdin-reading gate must exit on the end-of-file delivered at spawn" + ); + } } From b374d4aad17b0733b9db7b24a4fe38f5371db0a0 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Wed, 4 Nov 2026 20:53:11 +0800 Subject: [PATCH 2/7] fix(shell): address stdin review findings Round-1 review wave on the stdin EOF slice (9-lane fresh review pinned at e13a366fd): - scrub the tests.rs blank-line sweep (166 deletions were pure churn; the file keeps its blank-line-between-items style and the new pins) - drop the dead unbounded-write armor arm in `write_stdin`: the variant match is now exhaustive on its cfg, so a future third unix variant fails to compile instead of silently wedging under the session lock; the fd-clone failure and write-error paths restore the healthy handle instead of stripping a live task's interact surface - `bounded_stdin_write_owned` now returns `RecvTimeoutError` so the Windows/ohos path distinguishes a worker panic (writer failure, no stall flag) from a real deadline stall, matching the unix path - `close_stdin_pipe` applies the same stall conjunct as the `write_stdin` close (a stalled pipe must not claim EOF) and the manager wrapper drops its unneeded `pub` - the verifier's gate-job stdin close logs a warning instead of swallowing errors, so a future failure cannot silently revert gates to idling on the open pipe - copy: fix the semicolon splice in the lowercase bash description and the stale comment naming the removed write_stdin wrapper - pin the schema maxLength literals to the enforced MAX_INLINE_STDIN_BYTES byte cap Not in this wave: the unix PTY `close_stdin:true` semantics decision (explicit VEOF delivery vs honest refusal) is awaiting a call, and the sibling `run_bounded_child` inherit in tools/process.rs is follow-up material. PTY foreground lane: `tools::shell::` 153/0/1 green; fmt clean; fork-CI clippy flags green. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 143 ++++++++++++-------- crates/tui/src/tools/shell/tests.rs | 196 +++++++++++++++++++++++++++- crates/tui/src/tools/verifier.rs | 8 +- 3 files changed, 292 insertions(+), 55 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 9d7e94a8d3..365cde47cf 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -916,17 +916,24 @@ 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)`; -/// a deadline timeout returns `(None, TimedOut)` — the writer stays with -/// the abandoned worker, the bytes still land if the command resumes -/// reading, and the caller marks the stall. 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. +/// 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. +/// `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))] fn bounded_stdin_write_owned( writer: StdinWriter, payload: Vec, -) -> (Option, std::io::Result<()>) { +) -> ( + Option, + Result, std::sync::mpsc::RecvTimeoutError>, +) { let (sender, receiver) = std::sync::mpsc::channel(); std::thread::spawn(move || { let mut writer = writer; @@ -937,14 +944,8 @@ fn bounded_stdin_write_owned( let _ = sender.send((writer, result)); }); match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok((writer, result)) => (Some(writer), result), - Err(_) => ( - None, - Err(std::io::Error::new( - std::io::ErrorKind::TimedOut, - "the stdin write did not complete within the deadline", - )), - ), + Ok((writer, result)) => (Some(writer), Ok(result)), + Err(error) => (None, Err(error)), } } @@ -1480,10 +1481,14 @@ impl BackgroundShell { /// Close the stdin pipe of this task, keeping a PTY writer attached — /// the foreground close: pipe readers get end-of-file at once, while a /// terminal command keeps the interact surface a session member can - /// write to while it runs. Only a PIPE is claimed as end-of-file. + /// write to while it runs. Only an unstalled PIPE is claimed as + /// end-of-file: after a stalled write the abandoned worker still holds + /// a clone of the handle and the bytes have not finished landing, so + /// the flag stays down and refusals report the stall instead — the + /// same conjunct the `write_stdin` close applies. fn close_stdin_pipe(&mut self) { if matches!(self.stdin, Some(StdinWriter::Pipe(_))) { - self.stdin_closed_at_eof = true; + self.stdin_closed_at_eof = !self.stdin_write_stalled; self.stdin = None; } } @@ -1493,6 +1498,10 @@ impl BackgroundShell { // its bounded worker and hand it back; every exit below restores the // slot first, keeping the stall/eof refusal ordering identical with // the pinned unix behavior. + // `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`. + #[cfg_attr(all(unix, not(target_env = "ohos")), allow(unused_mut))] let mut slot = self.stdin.take(); if slot.is_some() { if !input.is_empty() { @@ -1525,22 +1534,33 @@ impl BackgroundShell { // and no dead binding, field, or methods under the workspace // deny(warnings). #[cfg(all(unix, not(target_env = "ohos")))] - if matches!( - slot, - Some(StdinWriter::Pipe(_)) | Some(StdinWriter::PtyFile(_)) - ) { + if let Some(writer) = slot.as_ref() { + // Exhaustive by construction: on this cfg `StdinWriter` + // has exactly `Pipe` and `PtyFile`, so a future third + // unix variant fails to COMPILE at the match below + // instead of silently falling into an unbounded blocking + // write under the session lock. The `else` arm only + // covers the `None` slot the take site already ruled + // out. use std::os::fd::AsFd; - let dup_fd = match slot.as_ref() { - Some(StdinWriter::Pipe(pipe)) => pipe.as_fd().try_clone_to_owned(), - Some(StdinWriter::PtyFile(file)) => file.as_fd().try_clone_to_owned(), - // The matches! above proves Some(Pipe | PtyFile); the - // None leg exists only for exhaustiveness (clippy - // flags an expect-after-is_some here, and an - // exhaustive match without it would be an - // unreachable-pattern warning). - _ => unreachable!("variant checked above"), - } - .context("Failed to clone the stdin handle for the bounded write")?; + let dup_fd = match writer { + StdinWriter::Pipe(pipe) => pipe.as_fd().try_clone_to_owned(), + StdinWriter::PtyFile(file) => file.as_fd().try_clone_to_owned(), + }; + let dup_fd = match dup_fd { + Ok(fd) => fd, + Err(error) => { + // The clone failed without consuming the + // handle: restore it so the task keeps its + // interact surface instead of later refusals + // misreporting a live task as "a finished + // command has released its stdin". + self.stdin = slot; + return Err(anyhow!(error).context( + "Failed to clone the stdin handle for the bounded write", + )); + } + }; let mut writer = std::process::ChildStdin::from(dup_fd); let payload = input.as_bytes().to_vec(); let (sender, receiver) = std::sync::mpsc::channel(); @@ -1549,7 +1569,16 @@ impl BackgroundShell { sender.send(writer.write_all(&payload).and_then(|_| writer.flush())); }); match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok(result) => result.context("Failed to write to stdin")?, + 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). + self.stdin = slot; + return Err(anyhow!(error).context("Failed to write to stdin")); + } // A channel close without a send means the worker // died before completing (a panic in the write // path): that is a writer failure, not a stall, so @@ -1572,15 +1601,8 @@ impl BackgroundShell { } } } else { - // Armor for a future third variant on this cfg: the - // matches! above proved Some, and this arm never runs on - // Windows/ohos (stripped with the whole if-else). - if let Some(writer) = slot.as_mut() { - writer - .write_all(input.as_bytes()) - .context("Failed to write to stdin")?; - writer.flush().ok(); - } + // The `None` slot the take site already ruled out: + // no handle to write through, nothing to restore. } // Windows and ohos cannot clone an fd out of their handle // (Windows strips the statement above because `unix` is @@ -1600,10 +1622,18 @@ impl BackgroundShell { let (returned, result) = bounded_stdin_write_owned(writer, input.as_bytes().to_vec()); match result { - Ok(()) => { + Ok(Ok(())) => { slot = returned; } - Err(error) if error.kind() == std::io::ErrorKind::TimedOut => { + Ok(Err(error)) => { + // The worker completed and handed the writer + // back: keep it attached so a write error does + // not silently strip the task's interact + // surface (same as the unix path above). + self.stdin = returned; + return Err(anyhow!(error).context("Failed to write to stdin")); + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { self.stdin_write_stalled = true; // `slot` stays None: the abandoned worker owns // the handle, so the close below cannot claim @@ -1613,8 +1643,17 @@ impl BackgroundShell { self.id )); } - Err(error) => { - return Err(anyhow!(error).context("Failed to write to stdin")); + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + // The worker died before completing (a panic in + // the write path): that is a writer failure, not + // a stall — the task is not marked stalled so a + // 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 + )); } } } @@ -3391,7 +3430,7 @@ impl ShellManager { /// but destroys the interact surface a session member can still use /// while a terminal command runs — the foreground terminal lane's /// contract. - pub fn close_stdin_pipe(&mut self, task_id: &str) -> Result<()> { + fn close_stdin_pipe(&mut self, task_id: &str) -> Result<()> { let shell = self .processes .get_mut(task_id) @@ -4875,10 +4914,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 `write_stdin` 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 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). // // Foreground has no operator at the keyboard, so whatever the command // will ever read from stdin must be bounded now. With stdin data the @@ -5121,7 +5160,7 @@ impl ToolSpec for LowercaseBashTool { } fn description(&self) -> &'static str { - "Execute a shell command in the workspace and return stdout and stderr. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell); This foreground surface runs without a terminal stdin: interactive programs and a bare `$SHELL` receive end-of-file and exit. With an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." + "Execute a shell command in the workspace and return stdout and stderr. The command runs in the detected platform shell — `$SHELL` on Unix (any shell it names, falling back to `/bin/sh`); on Windows, bash when the session provides one, otherwise PowerShell (pwsh 7 or Windows PowerShell 5.1) or cmd.exe. Windows PowerShell 5.1 lacks `&&`; prefer forms the detected shell supports. In read-only shell sessions a single approved command runs directly as argv instead (only pipelines still go through the shell). This foreground surface runs without a terminal stdin: interactive programs and a bare `$SHELL` receive end-of-file and exit. With an external sandbox backend configured this tool is unavailable (use the backend-specific shell tool instead). Output keeps the last 2000 lines or 50KB. An optional timeout is expressed in seconds; when omitted the command is killed after 120 seconds, so pass an explicit timeout for work expected to take longer. In Ask, after a sandbox denial, retry the exact command once with sandbox_permissions (the narrowest wider mode that suffices) and a one-sentence justification; the approval prompt asks the user." } fn input_schema(&self) -> serde_json::Value { diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 422e54f6c7..f2c9e51d86 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -1,20 +1,26 @@ use super::*; + use crate::tools::spec::ToolContext; use serde_json::{Value, json}; use tempfile::tempdir; + #[cfg(windows)] use windows::Win32::Foundation::{DUPLICATE_HANDLE_OPTIONS, DuplicateHandle, HANDLE}; #[cfg(windows)] use windows::Win32::System::Threading::GetCurrentProcess; + // `env_lock` serializes tests that mutate the process environment. #[cfg(any(unix, windows))] use std::sync::{Mutex, OnceLock}; + #[cfg(any(unix, windows))] fn env_lock() -> &'static Mutex<()> { static LOCK: OnceLock> = OnceLock::new(); LOCK.get_or_init(|| Mutex::new(())) } + const BACKGROUND_COMPLETION_WAIT_MS: u64 = 30_000; + #[test] fn lowercase_bash_schema_is_small_contract() { let schema = LowercaseBashTool.input_schema(); @@ -34,6 +40,7 @@ fn lowercase_bash_schema_is_small_contract() { ); assert!(!BashTool::new("Bash").model_visible()); } + #[test] fn lowercase_bash_description_matches_the_timeout_it_actually_applies() { use super::{ @@ -82,6 +89,7 @@ fn lowercase_bash_description_matches_the_timeout_it_actually_applies() { "schema must name the default it applies: {timeout_doc}" ); } + #[test] fn contract_bash_foreground_without_a_timeout_is_bounded_not_endless() { use super::{ @@ -122,6 +130,7 @@ fn contract_bash_foreground_without_a_timeout_is_bounded_not_endless() { Some(120_000) ); } + #[cfg(all(unix, not(target_env = "ohos")))] #[test] fn inherited_interactive_terminal_fails_closed_before_spawn() { @@ -138,6 +147,7 @@ fn inherited_interactive_terminal_fails_closed_before_spawn() { assert!(message.contains("terminal/run"), "{message}"); assert!(message.contains("terminal/send"), "{message}"); } + #[cfg(all(unix, target_env = "ohos"))] #[test] fn inherited_interactive_terminal_offers_only_ohos_recovery_paths() { @@ -153,6 +163,7 @@ fn inherited_interactive_terminal_offers_only_ohos_recovery_paths() { assert!(!message.contains("background: true"), "{message}"); assert!(!message.contains("terminal/run"), "{message}"); } + #[test] fn contract_bash_nonzero_is_an_error_with_status_after_output() { let error = finish_contract_bash_result( @@ -183,6 +194,7 @@ fn contract_bash_nonzero_is_an_error_with_status_after_output() { .ends_with("before\n\nCommand exited with code 7") ); } + #[cfg(unix)] #[tokio::test] async fn lowercase_bash_returns_one_ordered_stream() { @@ -197,6 +209,7 @@ async fn lowercase_bash_returns_one_ordered_stream() { .expect("bash"); assert_eq!(result.content, "out-1err-2out-3"); } + #[cfg(windows)] fn windows_python_stdio_fixture() -> (tempfile::TempDir, String) { assert!( @@ -243,6 +256,7 @@ fn windows_python_stdio_fixture() -> (tempfile::TempDir, String) { ); (workspace, command) } + #[cfg(windows)] fn assert_windows_python_stdio(output: &str) { assert!( @@ -262,6 +276,7 @@ fn assert_windows_python_stdio(output: &str) { "output contains replacement characters" ); } + #[cfg(windows)] #[tokio::test] #[ignore = "requires native Windows Python on PATH and PowerShell"] @@ -278,6 +293,7 @@ async fn forkguard_windows_python_stdio_lowercase_bash() { assert!(result.success, "{}", result.content); assert_windows_python_stdio(&result.content); } + #[cfg(windows)] #[test] #[ignore = "requires native Windows Python on PATH and PowerShell"] @@ -300,6 +316,7 @@ fn forkguard_windows_python_stdio_background_shell() { assert_windows_python_stdio(&format!("{}\n{}", result.stdout, result.stderr)); } } + #[cfg(unix)] #[tokio::test] async fn lowercase_bash_keeps_raw_command_under_readonly_policy() { @@ -320,6 +337,7 @@ async fn lowercase_bash_keeps_raw_command_under_readonly_policy() { .to_string() ); } + #[tokio::test] async fn lowercase_bash_readonly_refusal_names_work_mode() { let workspace = tempdir().expect("workspace"); @@ -335,6 +353,7 @@ async fn lowercase_bash_readonly_refusal_names_work_mode() { assert!(!result.content.contains("Act mode")); assert!(!workspace.path().join("blocked-by-plan").exists()); } + /// Regression for the wedge that took out the owner's own session under swap /// exhaustion: the lowercase `bash` spill file could not be created (full temp /// volume), every call — including `echo ok` — failed with the harness-internal @@ -407,6 +426,7 @@ async fn lowercase_bash_survives_spill_file_failure_and_stays_usable() { "fail-soft must not create the dir" ); } + /// A spawn/stream failure caused by host exhaustion must reach the model as /// an actionable message (cause chain + likely reason + retry), never as a /// bare harness-internal context string. @@ -443,6 +463,7 @@ fn shell_execution_failure_names_resource_exhaustion_and_says_retry() { "Shell execution failed: working directory does not exist" ); } + #[tokio::test] async fn lowercase_bash_timeout_uses_seconds_and_fails() { let workspace = tempdir().expect("workspace"); @@ -461,6 +482,7 @@ async fn lowercase_bash_timeout_uses_seconds_and_fails() { "{error}" ); } + fn execute_shell( manager: &mut ShellManager, command: &str, @@ -480,6 +502,7 @@ fn execute_shell( "workspace", ) } + #[test] fn forkguard_shell_catalog_guidance_matches_execution() { let tool = BashTool::new("Bash"); @@ -529,6 +552,7 @@ fn forkguard_shell_catalog_guidance_matches_execution() { assert_eq!(catalog[0].name, LowercaseBashTool.name()); assert_eq!(catalog[0].description, LowercaseBashTool.description()); } + #[test] #[ignore = "Exports model-visible shell fixtures for opt-in live model evaluation"] fn export_shell_guidance_eval_fixture() { @@ -545,6 +569,7 @@ fn export_shell_guidance_eval_fixture() { }); std::fs::write(path, serde_json::to_vec_pretty(&fixture).unwrap()).unwrap(); } + #[test] fn deleted_saved_workspace_reports_path_and_recovery_before_spawn() { let workspace = tempdir().expect("workspace"); @@ -559,6 +584,7 @@ fn deleted_saved_workspace_reports_path_and_recovery_before_spawn() { assert!(message.contains("working_dir") || message.contains("cwd")); assert!(message.contains("resume/fork")); } + #[test] fn explicit_missing_working_dir_is_not_misreported_as_session_corruption() { let workspace = tempdir().expect("workspace"); @@ -578,6 +604,7 @@ fn explicit_missing_working_dir_is_not_misreported_as_session_corruption() { assert!(message.contains(&missing.display().to_string())); assert!(!message.contains("saved session workspace")); } + #[cfg(not(target_env = "ohos"))] #[test] fn pty_exit_status_preserves_high_windows_code_losslessly() { @@ -592,6 +619,7 @@ fn pty_exit_status_preserves_high_windows_code_losslessly() { ); assert_eq!(exit_code_hex(status.code).as_deref(), Some("0xC0000005")); } + #[cfg(not(target_env = "ohos"))] #[test] fn ordinary_pty_exit_status_keeps_concise_label() { @@ -601,13 +629,16 @@ fn ordinary_pty_exit_status_keeps_concise_label() { assert_eq!(exit_code_label(status.code), "exit code 127"); assert_eq!(exit_code_hex(status.code), None); } + #[cfg(windows)] #[test] fn std_windows_exit_status_reinterprets_signed_dword() { assert_eq!(std_exit_code_i64(0xC000_0005_u32 as i32), 0xC000_0005); } + #[cfg(windows)] const JOB_OBJECT_QUERY_ACCESS: u32 = 0x0004; + #[cfg(windows)] fn duplicate_job_without_terminate_access(job: WindowsJob) -> WindowsJob { let process = unsafe { GetCurrentProcess() }; @@ -631,9 +662,11 @@ fn duplicate_job_without_terminate_access(job: WindowsJob) -> WindowsJob { handle: limited_handle, } } + fn echo_command(message: &str) -> String { format!("echo {message}") } + fn sleep_command(seconds: u64) -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -649,6 +682,7 @@ fn sleep_command(seconds: u64) -> String { format!("sleep {seconds}") } } + fn sleep_then_echo_command(seconds: u64, message: &str) -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -664,6 +698,7 @@ fn sleep_then_echo_command(seconds: u64, message: &str) -> String { format!("sleep {seconds} && echo {message}") } } + fn echo_stdin_command() -> String { let dispatcher = crate::shell_dispatcher::global_dispatcher(); if dispatcher.kind().is_powershell() { @@ -678,6 +713,7 @@ fn echo_stdin_command() -> String { "cat".to_string() } } + fn network_restricted_context(tmp: &std::path::Path) -> ToolContext { ToolContext::new(tmp) .with_elevated_sandbox_policy(ExecutionSandboxPolicy::WorkspaceWrite { @@ -690,6 +726,7 @@ fn network_restricted_context(tmp: &std::path::Path) -> ToolContext { "Shell command blocked: Plan mode runs shell commands in a network-restricted sandbox.", ) } + fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -709,10 +746,12 @@ fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult { sandbox_denied: false, } } + #[cfg(unix)] const SHELL_DESCENDANT_HELPER_ENV: &str = "CODEWHALE_SHELL_DESCENDANT_HELPER"; #[cfg(unix)] const SHELL_DESCENDANT_PID_FILE_ENV: &str = "CODEWHALE_SHELL_DESCENDANT_PID_FILE"; + #[cfg(unix)] #[test] fn shell_descendant_helper_process() { @@ -729,6 +768,7 @@ fn shell_descendant_helper_process() { std::thread::sleep(Duration::from_secs(30)); let _ = child.wait(); } + #[cfg(unix)] fn wait_for_shell_pid_file(path: &Path) -> libc::pid_t { let deadline = Instant::now() + Duration::from_secs(5); @@ -745,6 +785,7 @@ fn wait_for_shell_pid_file(path: &Path) -> libc::pid_t { std::thread::sleep(Duration::from_millis(25)); } } + #[cfg(unix)] fn wait_for_shell_pid_exit(pid: libc::pid_t) -> bool { let deadline = Instant::now() + Duration::from_secs(2); @@ -760,6 +801,7 @@ fn wait_for_shell_pid_exit(pid: libc::pid_t) -> bool { std::thread::sleep(Duration::from_millis(25)); } } + fn wait_for_completed_shell(manager: &mut ShellManager, task_id: &str) -> ShellResult { let deadline = Instant::now() + Duration::from_millis(BACKGROUND_COMPLETION_WAIT_MS); @@ -773,6 +815,7 @@ fn wait_for_completed_shell(manager: &mut ShellManager, task_id: &str) -> ShellR std::thread::sleep(Duration::from_millis(50)); } } + #[test] fn shell_owner_registers_before_spawn_and_silent_work_stays_live() { let work = crate::work_graph::new_shared_work_runtime( @@ -835,6 +878,7 @@ fn shell_owner_registers_before_spawn_and_silent_work_stays_live() { Some(512) ); } + #[test] fn exec_shell_parallel_flags_are_input_aware() { let tool = BashTool::new("Bash"); @@ -927,6 +971,7 @@ fn exec_shell_parallel_flags_are_input_aware() { "interactive": true }))); } + #[tokio::test] async fn readonly_shell_refuses_raw_string_external_backend() { struct Backend(std::sync::atomic::AtomicBool); @@ -959,6 +1004,7 @@ async fn readonly_shell_refuses_raw_string_external_backend() { assert!(error.contains("raw command string"), "{error}"); assert!(!backend.0.load(std::sync::atomic::Ordering::SeqCst)); } + #[tokio::test] async fn lowercase_bash_refuses_non_streaming_external_backend() { struct Backend(std::sync::atomic::AtomicBool); @@ -985,6 +1031,7 @@ async fn lowercase_bash_refuses_non_streaming_external_backend() { assert!(error.to_string().contains("combined streaming output")); assert!(!backend.0.load(std::sync::atomic::Ordering::SeqCst)); } + #[test] fn readonly_argv_is_shell_free_and_disables_git_helpers() { let (program, args) = hardened_readonly_argv("git show HEAD").expect("argv"); @@ -1071,6 +1118,7 @@ fn readonly_program_resolution_ignores_workspace_shadow_executables() { assert!(resolved.is_absolute() && !resolved.starts_with(workspace.path())); } } + /// B14-1: an attached root is agent-writable, so an executable planted inside /// it must shadow nothing and its PATH entries must be stripped — exactly /// like the primary root. Judging only the primary here fails open. @@ -1113,6 +1161,7 @@ fn forkguard_workspace_roots_readonly_shell_distrusts_attached_root_programs() { let kept: Vec<_> = std::env::split_paths(&sanitized).collect(); assert_eq!(kept, vec![trusted.path().canonicalize().unwrap()]); } + #[test] fn readonly_child_env_removes_git_and_github_redirects() { let mut command = std::process::Command::new("unused"); @@ -1145,6 +1194,7 @@ fn readonly_child_env_removes_git_and_github_redirects() { .any(|(name, value)| name == READONLY_ENV_MARKER && value.is_none()) ); } + #[test] fn readonly_operands_are_workspace_bounded_and_symlink_aware() { let workspace = tempdir().expect("workspace"); @@ -1219,6 +1269,7 @@ fn readonly_operands_are_workspace_bounded_and_symlink_aware() { .expect_err("operands must resolve relative to the effective cwd"); } } + /// B14-1: the operand/cwd gate judges against the full declared root set. /// Operands and working directories under an attached root were already /// admitted by the roots-aware `resolve_path`; refusing them here would @@ -1273,6 +1324,7 @@ fn forkguard_workspace_roots_readonly_shell_operands_span_attached_roots() { enforce_readonly_workspace_operands("cat shared.txt", &roots, outside.path()) .expect_err("cwd outside every root must fail"); } + #[test] fn windows_verbatim_and_drive_operands_survive_posix_split() { // `shlex` splits with POSIX backslash-escaping, which silently eats @@ -1296,6 +1348,7 @@ fn windows_verbatim_and_drive_operands_survive_posix_split() { ); } } + #[test] fn windows_path_protection_leaves_other_words_untouched() { // POSIX escapes, drive-relative spellings, and plain relative operands @@ -1313,6 +1366,7 @@ fn windows_path_protection_leaves_other_words_untouched() { "cat inside.txt" ); } + #[test] fn readonly_github_shell_calls_obey_the_host_network_policy_before_spawn() { let tmp = tempdir().expect("tempdir"); @@ -1346,6 +1400,7 @@ fn readonly_github_shell_calls_obey_the_host_network_policy_before_spawn() { .to_string(); assert!(prompted.contains("requires network approval")); } + #[test] fn exec_shell_interact_requires_approval() { let tool = BashTool::alias("exec_shell_interact", "interact"); @@ -1355,6 +1410,7 @@ fn exec_shell_interact_requires_approval() { .contains(&ToolCapability::RequiresApproval) ); } + #[tokio::test] async fn read_only_shell_policy_blocks_non_readonly_commands() { let tmp = tempdir().expect("tempdir"); @@ -1395,6 +1451,7 @@ async fn read_only_shell_policy_blocks_non_readonly_commands() { ); } } + #[cfg(unix)] #[tokio::test] async fn read_only_shell_resolves_operands_from_the_effective_cwd() { @@ -1417,6 +1474,7 @@ async fn read_only_shell_resolves_operands_from_the_effective_cwd() { .to_string(); assert!(error.contains("resolves outside"), "{error}"); } + #[cfg(unix)] #[tokio::test] async fn read_only_shell_skips_shell_env_hooks() { @@ -1445,6 +1503,7 @@ async fn read_only_shell_skips_shell_env_hooks() { assert!(result.success, "{}", result.content); assert!(!marker.exists(), "shell_env hook must not run for ReadOnly"); } + #[tokio::test] async fn read_only_shell_policy_allows_readonly_inspection() { let tmp = tempdir().expect("tempdir"); @@ -1470,6 +1529,7 @@ async fn read_only_shell_policy_allows_readonly_inspection() { Some("Completed") ); } + #[tokio::test] async fn exec_shell_multiline_block_explains_allow_shell_boundary() { let tmp = tempdir().expect("tempdir"); @@ -1505,6 +1565,7 @@ async fn exec_shell_multiline_block_explains_allow_shell_boundary() { result.content ); } + #[test] fn exec_shell_wait_schema_defaults_to_blocking() { let schema = BashTool::alias("exec_shell_wait", "wait").input_schema(); @@ -1519,6 +1580,7 @@ fn exec_shell_wait_schema_defaults_to_blocking() { .contains("wait") ); } + #[tokio::test] async fn exec_shell_wait_without_wait_arg_blocks_until_completion() { let tmp = tempdir().expect("tempdir"); @@ -1555,6 +1617,7 @@ async fn exec_shell_wait_without_wait_arg_blocks_until_completion() { Some("Completed") ); } + #[tokio::test] async fn exec_shell_wait_false_returns_nonblocking_snapshot() { let tmp = tempdir().expect("tempdir"); @@ -1596,6 +1659,7 @@ async fn exec_shell_wait_false_returns_nonblocking_snapshot() { Some("Running") ); } + #[tokio::test] async fn exec_shell_wait_without_wait_arg_returns_running_at_timeout() { let tmp = tempdir().expect("tempdir"); @@ -1639,6 +1703,7 @@ async fn exec_shell_wait_without_wait_arg_returns_running_at_timeout() { .await .expect("cancel background"); } + #[tokio::test] async fn exec_shell_wait_many_until_any_returns_when_the_first_task_settles() { let tmp = tempdir().expect("tempdir"); @@ -1707,6 +1772,7 @@ async fn exec_shell_wait_many_until_any_returns_when_the_first_task_settles() { Some(&json!(false)) ); } + #[tokio::test] async fn exec_shell_wait_many_all_reports_timeout_and_still_running() { let tmp = tempdir().expect("tempdir"); @@ -1761,6 +1827,7 @@ async fn exec_shell_wait_many_all_reports_timeout_and_still_running() { assert_eq!(statuses[first_id.as_str()], "Running"); assert_eq!(statuses[second_id.as_str()], "Running"); } + #[tokio::test] async fn exec_shell_wait_many_rejects_an_unknown_task_id() { let tmp = tempdir().expect("tempdir"); @@ -1774,6 +1841,7 @@ async fn exec_shell_wait_many_rejects_an_unknown_task_id() { .expect_err("unknown ids must fail the same way as the single-task path"); assert!(err.to_string().contains("nope-1"), "{err}"); } + #[tokio::test] async fn background_start_advertises_task_status_completion() { let tmp = tempdir().expect("tempdir"); @@ -1803,6 +1871,7 @@ async fn background_start_advertises_task_status_completion() { Some("nonblocking") ); } + #[tokio::test] async fn background_shell_job_preserves_origin_identity() { let tmp = tempdir().expect("tempdir"); @@ -1879,6 +1948,7 @@ async fn background_shell_job_preserves_origin_identity() { .await .expect("cancel owned background shell"); } + #[tokio::test] async fn drain_finished_jobs_reports_once() { let tmp = tempdir().expect("tempdir"); @@ -1934,6 +2004,7 @@ async fn drain_finished_jobs_reports_once() { assert!(second.is_empty(), "completion should be reported only once"); assert!(!manager.may_have_undelivered_completion()); } + #[tokio::test] async fn background_job_is_hidden_from_replacement_session_and_resumes_once_for_owner() { let tmp = tempdir().expect("tempdir"); @@ -1994,6 +2065,7 @@ async fn background_job_is_hidden_from_replacement_session_and_resumes_once_for_ "A completion is delivered exactly once" ); } + #[test] fn completion_evidence_preserves_arbitrary_stream_bytes() { use base64::Engine as _; @@ -2040,6 +2112,7 @@ fn completion_evidence_preserves_arbitrary_stream_bytes() { assert_eq!(decoded_stdout, stdout); assert_eq!(decoded_stderr, stderr); } + #[test] #[cfg(unix)] fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() { @@ -2082,6 +2155,7 @@ fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() { assert_eq!(result.status, ShellStatus::Completed); assert_eq!(result.stdout, "unset\nexplicit-value\n"); } + #[test] #[cfg(windows)] fn shell_execution_preserves_custom_windows_sdk_root_env() { @@ -2131,6 +2205,7 @@ fn shell_execution_preserves_custom_windows_sdk_root_env() { result ); } + #[test] fn test_sync_execution() { let tmp = tempdir().expect("tempdir"); @@ -2143,6 +2218,7 @@ fn test_sync_execution() { assert!(result.stdout.contains("hello")); assert!(result.task_id.is_none()); } + #[test] fn test_background_execution() { let tmp = tempdir().expect("tempdir"); @@ -2169,6 +2245,7 @@ fn test_background_execution() { assert_eq!(final_result.status, ShellStatus::Completed); assert!(final_result.stdout.contains("done")); } + #[test] fn test_timeout() { let tmp = tempdir().expect("tempdir"); @@ -2179,6 +2256,7 @@ fn test_timeout() { assert_eq!(result.status, ShellStatus::TimedOut); } + #[test] fn test_kill() { let tmp = tempdir().expect("tempdir"); @@ -2195,6 +2273,7 @@ fn test_kill() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } + #[test] fn test_write_stdin_streams_output() { let tmp = tempdir().expect("tempdir"); @@ -2232,6 +2311,7 @@ fn test_write_stdin_streams_output() { .expect("get_output_delta"); assert!(delta2.result.stdout.is_empty()); } + #[test] #[cfg(all(unix, not(target_env = "ohos")))] fn background_tty_command_has_controlling_terminal() { @@ -2266,6 +2346,7 @@ fn background_tty_command_has_controlling_terminal() { "tty output should confirm /dev/tty opened; got {done:?}" ); } + #[test] fn test_job_list_poll_cancel_and_stale_snapshot() { let tmp = tempdir().expect("tempdir"); @@ -2318,6 +2399,7 @@ fn test_job_list_poll_cancel_and_stale_snapshot() { assert!(stale.stale); assert_eq!(stale.linked_task_id.as_deref(), Some("task_old")); } + #[test] fn running_job_snapshot_marks_no_output_stale_after_threshold() { let tmp = tempdir().expect("tempdir"); @@ -2346,6 +2428,7 @@ fn running_job_snapshot_marks_no_output_stale_after_threshold() { "elapsed no-output time should be exposed: {job:?}" ); } + #[test] fn running_job_snapshot_keeps_recent_no_output_fresh() { let tmp = tempdir().expect("tempdir"); @@ -2365,6 +2448,7 @@ fn running_job_snapshot_keeps_recent_no_output_fresh() { assert!(!job.stale, "fresh running job should not start stale"); assert!(job.elapsed_since_output_ms.is_some()); } + #[test] fn test_job_cancel_updates_completion_state() { let tmp = tempdir().expect("tempdir"); @@ -2380,6 +2464,7 @@ fn test_job_cancel_updates_completion_state() { assert_eq!(job.snapshot.status, ShellStatus::Killed); assert!(!job.snapshot.stdin_available); } + #[test] fn test_output_truncation() { let long_output = "x".repeat(50_000); @@ -2388,6 +2473,7 @@ fn test_output_truncation() { assert!(truncated.len() < long_output.len()); assert!(truncated.contains("truncated")); } + #[test] fn test_truncate_with_meta_reports_omission_counts() { let long_output = format!("line1\nline2\n{}", "x".repeat(60_000)); @@ -2398,6 +2484,7 @@ fn test_truncate_with_meta_reports_omission_counts() { assert!(meta.omitted > 0); assert!(truncated.contains("bytes omitted")); } + #[test] fn network_restricted_hint_detects_silent_curl_failure() { let tmp = tempdir().expect("tempdir"); @@ -2413,6 +2500,7 @@ fn network_restricted_hint_detects_silent_curl_failure() { assert!(hint.contains("Plan mode")); } + #[test] fn sandbox_denied_hint_names_the_effective_posture() { // DGF-02: an approved write blocked by a read-only sandbox must come @@ -2434,6 +2522,7 @@ fn sandbox_denied_hint_names_the_effective_posture() { ); assert!(hint.contains("justification"), "{hint}"); } + #[test] fn contract_bash_denial_surfaces_the_escalation_shape() { let tmp = tempdir().expect("tempdir"); @@ -2452,6 +2541,7 @@ fn contract_bash_denial_surfaces_the_escalation_shape() { "{error}" ); } + #[test] fn sandbox_denied_hint_absent_without_denial_or_policy() { let tmp = tempdir().expect("tempdir"); @@ -2465,6 +2555,7 @@ fn sandbox_denied_hint_absent_without_denial_or_policy() { let no_policy_ctx = ToolContext::new(tmp.path()); assert!(shell_sandbox_denied_hint(&no_policy_ctx, &denied).is_none()); } + #[test] fn shell_delta_result_surfaces_sandbox_denied_hint() { let tmp = tempdir().expect("tempdir"); @@ -2493,6 +2584,7 @@ fn shell_delta_result_surfaces_sandbox_denied_hint() { let metadata = tool_result.metadata.expect("metadata"); assert!(metadata.get("sandbox_denied_hint").is_some()); } + #[test] fn network_restricted_hint_ignores_local_failures() { let tmp = tempdir().expect("tempdir"); @@ -2501,6 +2593,7 @@ fn network_restricted_hint_ignores_local_failures() { assert!(shell_network_restricted_hint(&ctx, "cat missing.txt", &result).is_none()); } + #[test] fn shell_delta_result_surfaces_network_restricted_hint() { let tmp = tempdir().expect("tempdir"); @@ -2527,6 +2620,7 @@ fn shell_delta_result_surfaces_network_restricted_hint() { Some(true) ); } + #[test] fn shell_delta_result_exposes_lossless_high_exit_code_and_hex() { let tmp = tempdir().expect("tempdir"); @@ -2555,6 +2649,7 @@ fn shell_delta_result_exposes_lossless_high_exit_code_and_hex() { assert_eq!(metadata["exit_code"], json!(3221225477_i64)); assert_eq!(metadata["exit_code_hex"], json!("0xC0000005")); } + #[test] fn shell_delta_result_surfaces_elapsed_time_in_content() { let tmp = tempdir().expect("tempdir"); @@ -2582,6 +2677,7 @@ fn shell_delta_result_surfaces_elapsed_time_in_content() { tool_result.content ); } + #[test] fn shell_delta_timing_line_omits_task_id_when_unknown() { let tmp = tempdir().expect("tempdir"); @@ -2605,6 +2701,7 @@ fn shell_delta_timing_line_omits_task_id_when_unknown() { tool_result.content ); } + #[test] fn shell_delta_timing_line_phrases_cover_terminal_statuses() { let tmp = tempdir().expect("tempdir"); @@ -2635,6 +2732,7 @@ fn shell_delta_timing_line_phrases_cover_terminal_statuses() { ); } } + #[test] fn shell_delta_timing_line_handles_zero_duration() { let tmp = tempdir().expect("tempdir"); @@ -2659,6 +2757,7 @@ fn shell_delta_timing_line_handles_zero_duration() { tool_result.content ); } + #[test] fn shell_delta_timing_line_sits_below_network_hint() { let tmp = tempdir().expect("tempdir"); @@ -2683,6 +2782,7 @@ fn shell_delta_timing_line_sits_below_network_hint() { "hint must precede timing line: {content}" ); } + #[test] fn shell_delta_result_includes_cargo_failure_summary() { let tmp = tempdir().expect("tempdir"); @@ -2733,6 +2833,7 @@ fn shell_delta_result_includes_cargo_failure_summary() { .contains("error: test failed") ); } + #[test] fn shell_delta_result_keeps_existing_summary_for_generic_cargo_failure() { let tmp = tempdir().expect("tempdir"); @@ -2772,6 +2873,7 @@ fn shell_delta_result_keeps_existing_summary_for_generic_cargo_failure() { json!("command failed without structured cargo diagnostics") ); } + #[test] fn shell_delta_result_surfaces_python_build_dependency_hint() { let tmp = tempdir().expect("tempdir"); @@ -2823,6 +2925,7 @@ fn shell_delta_result_surfaces_python_build_dependency_hint() { .contains("setuptools") ); } + #[test] fn test_summarize_output_strips_truncation_note() { let long_output = "x".repeat(60_000); @@ -2830,6 +2933,7 @@ fn test_summarize_output_strips_truncation_note() { let summary = summarize_output(&truncated); assert!(!summary.contains("Output truncated at")); } + #[tokio::test] async fn test_exec_shell_metadata_includes_summaries() { let tmp = tempdir().expect("tempdir"); @@ -2852,6 +2956,7 @@ async fn test_exec_shell_metadata_includes_summaries() { assert!(meta.get("stdout_len").is_some()); assert!(meta.get("stdout_truncated").is_some()); } + #[cfg(not(windows))] #[tokio::test] async fn test_exec_shell_combined_output_uses_single_stream() { @@ -2874,6 +2979,7 @@ async fn test_exec_shell_combined_output_uses_single_stream() { Some(true) ); } + #[tokio::test] async fn test_exec_shell_foreground_timeout_guides_background_rerun() { let tmp = tempdir().expect("tempdir"); @@ -2933,6 +3039,7 @@ async fn test_exec_shell_foreground_timeout_guides_background_rerun() { let recommended = recovery.to_string(); assert!(!recommended.contains("exec_shell"), "{recommended}"); } + #[test] fn background_schema_distinguishes_temporary_jobs_from_persistent_services() { let schema = BashTool::new("Bash").input_schema(); @@ -2941,6 +3048,7 @@ fn background_schema_distinguishes_temporary_jobs_from_persistent_services() { .expect("background description"); assert!(d.contains("killed") && d.contains("background:true") && d.contains("persist:true")); } + #[tokio::test] async fn test_exec_shell_foreground_cancel_kills_process() { let tmp = tempdir().expect("tempdir"); @@ -2975,6 +3083,7 @@ async fn test_exec_shell_foreground_cancel_kills_process() { assert_eq!(meta.get("status").and_then(Value::as_str), Some("Killed")); assert_eq!(meta.get("canceled").and_then(Value::as_bool), Some(true)); } + #[tokio::test] async fn test_exec_shell_foreground_can_move_to_background() { let tmp = tempdir().expect("tempdir"); @@ -3041,6 +3150,7 @@ async fn test_exec_shell_foreground_can_move_to_background() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } + #[tokio::test] async fn lowercase_bash_foreground_detach_is_a_successful_running_receipt() { let tmp = tempdir().expect("tempdir"); @@ -3084,6 +3194,7 @@ async fn lowercase_bash_foreground_detach_is_a_successful_running_receipt() { assert_eq!(job.snapshot.status, ShellStatus::Running); manager.kill(task_id).expect("kill test job"); } + #[tokio::test] async fn test_exec_shell_wait_cancel_leaves_background_process_running() { let tmp = tempdir().expect("tempdir"); @@ -3139,6 +3250,7 @@ async fn test_exec_shell_wait_cancel_leaves_background_process_running() { let killed = manager.kill(&task_id).expect("kill"); assert_eq!(killed.status, ShellStatus::Killed); } + #[tokio::test] async fn test_completed_background_shell_releases_process_handles() { let tmp = tempdir().expect("tempdir"); @@ -3178,6 +3290,7 @@ async fn test_completed_background_shell_releases_process_handles() { assert!(shell.stdout_thread.is_none()); assert!(shell.stderr_thread.is_none()); } + #[cfg(unix)] #[tokio::test] async fn exec_shell_cancel_kills_descendant_process_group() { @@ -3226,6 +3339,7 @@ async fn exec_shell_cancel_kills_descendant_process_group() { "descendant {descendant} survived shell process-group cancellation" ); } + #[tokio::test] async fn test_exec_shell_cancel_tool_kills_background_process() { let tmp = tempdir().expect("tempdir"); @@ -3259,6 +3373,7 @@ async fn test_exec_shell_cancel_tool_kills_background_process() { let job = manager.inspect_job(task_id).expect("inspect job"); assert_eq!(job.snapshot.status, ShellStatus::Killed); } + #[tokio::test] async fn test_exec_shell_cancel_tool_can_kill_all_running_processes() { let tmp = tempdir().expect("tempdir"); @@ -3301,6 +3416,7 @@ async fn test_exec_shell_cancel_tool_can_kill_all_running_processes() { assert_eq!(first_job.snapshot.status, ShellStatus::Killed); assert_eq!(second_job.snapshot.status, ShellStatus::Killed); } + fn make_failed_result(stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -3320,6 +3436,7 @@ fn make_failed_result(stderr: &str) -> ShellResult { stderr_truncated: false, } } + #[test] fn test_macos_provenance_detected_by_activity_time_message() { let result = make_failed_result( @@ -3328,6 +3445,7 @@ fn test_macos_provenance_detected_by_activity_time_message() { ); assert!(looks_like_macos_provenance_failure(&result)); } + #[test] fn test_macos_provenance_detected_by_activity_path_and_eperm() { let result = make_failed_result( @@ -3335,6 +3453,7 @@ fn test_macos_provenance_detected_by_activity_path_and_eperm() { ); assert!(looks_like_macos_provenance_failure(&result)); } + #[test] fn test_macos_provenance_not_triggered_on_success() { let mut result = make_failed_result( @@ -3345,11 +3464,13 @@ fn test_macos_provenance_not_triggered_on_success() { result.exit_code = Some(0); assert!(!looks_like_macos_provenance_failure(&result)); } + #[test] fn test_macos_provenance_not_triggered_on_unrelated_eperm() { let result = make_failed_result("open /some/other/path: operation not permitted"); assert!(!looks_like_macos_provenance_failure(&result)); } + // Regression test for #828: shell spawns an orphaned background subprocess // (simulating `nohup curl`) that keeps the pipe write-end open after the shell // exits. collect_output() must not block indefinitely — it kills the whole @@ -3372,6 +3493,7 @@ fn test_orphaned_subprocess_does_not_block_collect_output() { .expect("get_output must complete, not hang"); assert_eq!(done.status, ShellStatus::Completed); } + #[cfg(unix)] #[test] fn foreground_shell_does_not_block_on_orphaned_subprocess_pipe() { @@ -3388,6 +3510,7 @@ fn foreground_shell_does_not_block_on_orphaned_subprocess_pipe() { ); assert_eq!(result.status, ShellStatus::Completed); } + // Windows equivalent of the orphaned pipe-handle regression. `cmd /c start /b` // launches a descendant process that inherits stdout/stderr and outlives the // shell. Job-object cleanup must terminate that descendant before reader-thread @@ -3419,6 +3542,7 @@ fn background_collection_does_not_block_on_detached_descendant_pipe() { ); assert_eq!(done.status, ShellStatus::Completed); } + #[cfg(windows)] #[test] fn windows_job_terminate_denied_falls_back_to_child_kill() { @@ -3449,6 +3573,7 @@ fn windows_job_terminate_denied_falls_back_to_child_kill() { "fallback child kill should terminate child" ); } + #[cfg(windows)] #[test] fn windows_job_close_releases_foreground_reader_threads_when_terminate_denied() { @@ -3496,6 +3621,7 @@ fn windows_job_close_releases_foreground_reader_threads_when_terminate_denied() ); assert!(status.is_some(), "kill-on-close should terminate child"); } + #[cfg(windows)] #[test] fn windows_job_kill_on_close_releases_reader_threads_when_terminate_denied() { @@ -3537,6 +3663,7 @@ fn windows_job_kill_on_close_releases_reader_threads_when_terminate_denied() { ); assert_eq!(done.status, ShellStatus::Completed); } + #[cfg(windows)] #[test] fn killed_shell_does_not_wait_for_blocked_reader_threads() { @@ -3590,6 +3717,7 @@ fn killed_shell_does_not_wait_for_blocked_reader_threads() { ); release_tx.send(()).expect("release detached reader"); } + #[test] fn test_list_jobs_cleans_up_completed_old_processes() { let tmp = tempdir().expect("tempdir"); @@ -3610,6 +3738,7 @@ fn test_list_jobs_cleans_up_completed_old_processes() { "completed processes should be evicted by cleanup" ); } + /// Regression for #1691: a `git commit -m "feat: complete sub-pages"` shell /// command must reach the OS shell with its quoted message intact (one argv /// slot), never split into `feat:` / `complete` / `sub-pages"`. @@ -3654,6 +3783,7 @@ fn issue_1691_quoted_commit_message_round_trips() { .collect(); assert_eq!(got, spec.args); } + /// When no `cwd` is provided, the shell should run in `context.workspace`, /// not in the ShellManager's default_workspace. This ensures sub-agents in /// worktrees run commands in the worktree directory rather than the parent. @@ -3706,12 +3836,14 @@ async fn default_cwd_uses_context_workspace_not_shell_manager_default() { // escaped descendant wedged the blocking reader-thread join inside kill() // until the descendant exited on its own (observed: ~180s past a 120s // timeout in the wild). + #[cfg(unix)] const SHELL_SIGTERM_HELPER_ENV: &str = "CODEWHALE_SHELL_SIGTERM_HELPER"; #[cfg(unix)] const SHELL_ESCAPE_HELPER_ENV: &str = "CODEWHALE_SHELL_ESCAPE_HELPER"; #[cfg(unix)] const SHELL_ESCAPED_GRANDCHILD_ENV: &str = "CODEWHALE_SHELL_ESCAPED_GRANDCHILD"; + /// Helper role: ignore SIGTERM and idle. Runs as the shell's direct child /// (same process group), so only the SIGKILL escalation can stop it. #[cfg(unix)] @@ -3731,6 +3863,7 @@ fn shell_sigterm_ignoring_helper_process() { std::thread::sleep(Duration::from_millis(10)); } } + /// Helper role: spawn a grandchild in its OWN process group (escaping the /// shell's group) that inherits the output pipe, then exit immediately. The /// wrapper shell keeps running (`sleep` after `&`), so the job stays Running @@ -3759,6 +3892,7 @@ fn shell_group_escape_helper_process() { cmd.process_group(0); let _child = cmd.spawn().expect("spawn escaped grandchild"); } + /// Helper role: the escaped grandchild — ignores SIGTERM, reports its pid, /// then idles (holding the inherited output pipe open the whole time). #[cfg(unix)] @@ -3775,6 +3909,7 @@ fn shell_escaped_grandchild_helper_process() { std::fs::write(pid_file, std::process::id().to_string()).expect("write grandchild pid"); std::thread::sleep(Duration::from_secs(30)); } + /// Required regression: a foreground command that ignores SIGTERM must be /// dead and the tool must have returned within timeout + a small grace /// (2s timeout, assert wall < 10s). @@ -3814,6 +3949,7 @@ async fn foreground_timeout_kills_sigterm_ignoring_command_within_grace() { "SIGTERM-ignoring helper {helper_pid} survived the timeout kill" ); } + /// Regression for the ~180s kill-path overshoot: a descendant that escaped /// the process group keeps the output pipe open after the group is killed. /// kill() must still return within a bounded grace instead of blocking on @@ -3855,6 +3991,7 @@ async fn kill_returns_promptly_when_escaped_descendant_holds_pipe_open() { } assert!(wait_for_shell_pid_exit(grandchild)); } + /// `Bash` was the only action wrapper whose catch-all fell through to its most /// dangerous branch: an unrecognised action ran the command instead. #[tokio::test] @@ -3883,6 +4020,7 @@ async fn unknown_bash_action_is_refused_instead_of_running_the_command() { ); assert!(!marker.exists(), "the command must not have run"); } + /// The same hole one type down. `and_then(as_str).unwrap_or("run")` read a /// non-string `action` as absent and fell through to the branch that executes /// arbitrary code, so `Bash{action: 3, command: "…"}` ran the command. `File`, @@ -3918,6 +4056,7 @@ async fn non_string_bash_action_is_refused_instead_of_running_the_command() { assert!(!marker.exists(), "the command must not have run: {action}"); } } + /// The same hole for the data fields (2026-08-04 review). A non-string /// `stdin` was silently dropped — the command ran with NO stdin and reported /// success, the silent-drop failure this lane exists to close. A non-string @@ -3963,6 +4102,7 @@ async fn wrongly_typed_stdin_cwd_and_task_id_are_refused_not_dropped() { "a supplied-but-mistyped task_id must not read as missing: {message}" ); } + /// `null` is the wire spelling of absence, and `action` documents a `run` /// default — so the strictness above must not swallow the default. #[tokio::test] @@ -3982,6 +4122,7 @@ async fn absent_or_null_bash_action_still_defaults_to_run() { assert!(result.content.contains("defaulted"), "{}", result.content); } } + /// Negative case for the strictness above: every legitimate action still /// dispatches to its own handler rather than the action refusal. `wait`, /// `interact`, and `cancel` are checked by the error they raise *after* @@ -4024,6 +4165,7 @@ async fn every_valid_bash_action_still_dispatches() { .expect("action=cancel all=true must dispatch"); assert!(cancelled.success, "{}", cancelled.content); } + /// The stdin aliases were real but undocumented: a model that wrote `input` /// or `data` got them honoured with nothing in the schema saying so, and a /// maintainer reading the schema would have removed them as dead. Advertise @@ -4070,6 +4212,7 @@ async fn every_advertised_stdin_spelling_reaches_the_command() { "task_id must name its alias" ); } + /// The schema declared no `required` key at all, so `Bash{}` — no command, no /// task — was schema-valid for the tool that runs shell commands. What is /// required is per-action, so it is spelled as root `anyOf` required groups, @@ -4108,6 +4251,7 @@ fn bash_schema_declares_what_each_action_requires() { } } } + /// A root `anyOf` is not portable to every provider, so prove the fallback /// the sanitizer promises: Responses/xAI drop root composition, and the /// constraint has to survive as a description note rather than vanishing. @@ -4125,6 +4269,7 @@ fn bash_required_groups_survive_a_provider_that_drops_root_composition() { assert!(schema.get("anyOf").is_none(), "root anyOf must be removed"); assert!(schema["properties"]["command"].is_object()); } + /// Every hint in this file has to name a tool the model can actually call. /// `exec_shell` / `exec_shell_wait` were retired in v0.9.3. #[test] @@ -4133,6 +4278,7 @@ fn shell_recovery_hints_name_only_dispatchable_tools() { assert!(FOREGROUND_TIMEOUT_RECOVERY_HINT.contains("Bash")); assert!(FOREGROUND_TIMEOUT_RECOVERY_HINT.contains("action=\"wait\"")); } + /// One documented default hid three real ones: `wait` uses 30s and /// `interact` 1s, so a model omitting `timeout_ms` on `wait` got a quarter of /// the timeout the schema promised. @@ -4150,6 +4296,7 @@ fn timeout_ms_description_covers_every_action_default() { ); } } + #[cfg(unix)] fn authorized_persistent_service_context(workspace: &Path) -> ToolContext { let mut context = ToolContext::new(workspace.to_path_buf()) @@ -4160,11 +4307,13 @@ fn authorized_persistent_service_context(workspace: &Path) -> ToolContext { context.auto_approve = true; context } + #[cfg(unix)] fn persistent_service_test_lock() -> &'static tokio::sync::Mutex<()> { static LOCK: OnceLock> = OnceLock::new(); LOCK.get_or_init(|| tokio::sync::Mutex::new(())) } + #[cfg(unix)] #[tokio::test] async fn persistent_service_requires_explicit_headless_exec_authority() { @@ -4186,6 +4335,7 @@ async fn persistent_service_requires_explicit_headless_exec_authority() { assert!(error.to_string().contains("real headless `codewhale exec`")); } + #[cfg(unix)] #[tokio::test] async fn committed_persistent_service_survives_manager_drop_and_reports_identity() { @@ -4240,6 +4390,7 @@ async fn committed_persistent_service_survives_manager_drop_and_reports_identity "released service must survive Codewhale manager teardown" ); } + #[cfg(unix)] #[tokio::test] async fn signal_cleanup_kills_staged_persistent_service_group() { @@ -4295,11 +4446,13 @@ async fn signal_cleanup_kills_staged_persistent_service_group() { } // === #5472: in-memory retention must be bounded === + /// ~1.1 MB on stdout, fast: 30,000 lines of 37 bytes. #[cfg(unix)] fn chatty_command() -> String { "yes 0123456789abcdefghijklmnopqrstuvwxyz | head -n 30000".to_string() } + /// The finding-1 regression: before this bound, a single uppercase `Bash` call /// left its entire stdout resident in `ShellManager.processes` until the 1 h /// `cleanup`, and only if the user happened to open the jobs panel. @@ -4329,6 +4482,7 @@ async fn foreground_bash_releases_its_output_once_the_result_is_returned() { RAW_STREAM_SETTLED_TAIL_BYTES * 2 ); } + /// Releasing memory must not rewrite history: the job panel keeps reporting how /// much the command actually printed. #[cfg(unix)] @@ -4354,6 +4508,7 @@ async fn released_output_still_reports_the_real_stream_length() { "a diagnostic tail must survive the release" ); } + /// A background job's bytes become a durable session artifact at drain time; /// keeping a second copy in the manager afterwards is the pure-waste term. #[cfg(unix)] @@ -4403,6 +4558,7 @@ async fn draining_completion_evidence_releases_the_retained_copy() { "{retained} bytes still held after the evidence was published" ); } + /// Age was the only bound, and it only ran from `list_jobs()`. Hundreds of /// finished records inside one hour were all retained. #[test] @@ -4435,6 +4591,7 @@ fn cleanup_bounds_finished_records_by_count() { "the newest record must survive" ); } + /// #5478 nit: `/jobs` reported `2m 07s` for a 12-second command, because /// elapsed was `started_at.elapsed()` even after the job finished. A completed /// job reports the duration it finished with. @@ -4483,6 +4640,7 @@ fn a_finished_job_reports_its_duration_not_a_growing_elapsed() { "the frozen value must be the real duration, not the timeout: {second}ms" ); } + // The Windows execution-policy fallback: the temp `-File` script is refused // before any statement runs, so the tool re-runs the identical payload as an // inline `-EncodedCommand`. The predicate is what decides "refused by the @@ -4592,6 +4750,7 @@ fn forkguard_powershell_execution_policy_rejection_is_locale_independent() { )); assert!(!powershell_execution_policy_rejection("", "", script)); } + // End-to-end: the encoded form really runs the payload the temp `-File` form // was written for (multiline + non-ASCII), with no file on disk. #[cfg(windows)] @@ -4624,6 +4783,7 @@ fn forkguard_powershell_encoded_spec_runs_a_non_ascii_payload() { assert!(stdout.contains("编码-ok"), "stdout={stdout}"); assert!(stdout.contains("line2"), "stdout={stdout}"); } + fn failed_result_with(status: ShellStatus, stderr: &str) -> ShellResult { ShellResult { task_id: None, @@ -4643,6 +4803,7 @@ fn failed_result_with(status: ShellStatus, stderr: &str) -> ShellResult { sandbox_denied: false, } } + fn temp_file_spec(script: &std::path::Path, cwd: &std::path::Path) -> CommandSpec { CommandSpec::program( "pwsh", @@ -4659,6 +4820,7 @@ fn temp_file_spec(script: &std::path::Path, cwd: &std::path::Path) -> CommandSpe Duration::from_secs(5), ) } + // The retry gate: only a failed temp `-File` refusal may be retried, so a // cancellation or a timeout can never re-run a command the user stopped. #[test] @@ -4720,6 +4882,7 @@ fn forkguard_powershell_inline_retry_only_for_a_failed_file_refusal() { ) )); } + // The glue: the retry helper re-runs the identical payload inline, annotates // the fallback, and accounts for both attempts. #[cfg(windows)] @@ -4757,6 +4920,7 @@ fn forkguard_powershell_inline_retry_runs_the_command_and_notes_the_fallback() { retried.duration_ms ); } + // End-to-end: the normal temp `-File` form still carries the process-scoped // policy bypass, so a restricted machine runs the script this tool wrote. #[cfg(windows)] @@ -4787,6 +4951,7 @@ fn forkguard_powershell_temp_script_form_runs_a_non_ascii_payload() { assert!(stdout.contains("编码-ok"), "stdout={stdout}"); assert!(stdout.contains("line2"), "stdout={stdout}"); } + // A recovery path that cannot even start must not replace the refusal that // sent it there: the first attempt's evidence is what the user needs, so it is // kept and annotated with the retry failure. @@ -4854,6 +5019,10 @@ fn bounded_reap_reports_grace_expiry_while_the_child_lives() { assert!( !wait_child_bounded(&mut child, Duration::from_millis(50)), "a live child must report the exit as unconfirmed within the grace" + ); + let _ = child.kill(); + let _ = child.wait(); +} /// An inline stdin payload past one pipe buffer is refused before spawn: /// the payload is delivered with a blocking write while the shell-manager @@ -5426,6 +5595,9 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { 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()); + let Ok(result) = result else { + panic!("a consuming child must complete the write, not lose the worker: {result:?}"); + }; result.expect("a consuming child accepts the payload"); assert!(returned.is_some(), "the writer is handed back on success"); drop(returned); @@ -5443,8 +5615,10 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { let (returned, result) = super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), payload); let elapsed = started.elapsed(); - let error = result.expect_err("a non-reading child must time the write out"); - assert_eq!(error.kind(), std::io::ErrorKind::TimedOut, "{error}"); + assert!( + matches!(result, Err(std::sync::mpsc::RecvTimeoutError::Timeout)), + "a non-reading child must time the write out: {result:?}" + ); assert!( returned.is_none(), "a timed-out writer stays with the abandoned worker" @@ -5723,3 +5897,21 @@ fn dropping_the_manager_stops_a_child_with_an_unconfirmed_stop() { std::thread::sleep(Duration::from_millis(20)); } } + +#[test] +fn bash_schema_stdin_max_length_matches_the_enforced_inline_cap() { + // The schema literal and the enforced byte cap are one contract; this + // keeps the two hand-maintained numbers from drifting apart. + let schema = BashTool::new("Bash").input_schema(); + assert!( + !schema["properties"]["stdin"].is_null(), + "the full schema must declare stdin at its root properties" + ); + for key in ["stdin", "input", "data"] { + assert_eq!( + schema["properties"][key]["maxLength"], + json!(super::MAX_INLINE_STDIN_BYTES), + "schema {key} maxLength must equal the enforced inline byte cap" + ); + } +} diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index 5632c32fea..4abd1f3c40 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -589,7 +589,13 @@ fn start_background_gates( .ok() .and_then(|result| result.task_id.as_deref()) { - manager.write_stdin(task_id, "", true).ok(); + 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"); + } } spawned }; From 40c7fb670b0c9536a670f41eb23fff01a9b668f8 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Thu, 5 Nov 2026 00:33:48 +0800 Subject: [PATCH 3/7] fix(shell): interact close_stdin delivers EOF to unix PTY tasks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review decision (Option B): base delivered end-of-file to a terminal task through the ownership box's drop (`cat` on a pty exited 0 the instant it dropped); the dup'd-master rewrite made that drop invisible, so interact close_stdin:true on a unix terminal task became a silent no-op that returned success while the child waited forever (and Windows still EOFs via its box — the platforms diverged). The explicit close now writes the same \n + VEOF through the dup itself and records the EOF honestly; a stalled write still claims nothing, and the spawn-path close stays pipe-only so the documented "no end-of-file follows the payload" contract is untouched. Pinned by interact_close_stdin_delivers_eof_to_a_terminal_command. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 39 +++++++++++---- crates/tui/src/tools/shell/tests.rs | 74 +++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 365cde47cf..5b4c585cbf 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -1659,15 +1659,36 @@ impl BackgroundShell { } } if close { - // Only a dropped pipe delivers end-of-file; dropping a PTY - // writer is invisible to the child (the reader holds the - // master), so it must not be reported as EOF later. 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. - self.stdin_closed_at_eof = - matches!(slot, Some(StdinWriter::Pipe(_))) && !self.stdin_write_stalled; + // Only a dropped PIPE delivers end-of-file. An explicit + // close on a unix PTY writer must DELIVER it instead: the + // old ownership box injected `\n` + VEOF on drop (a `cat` + // 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(); + } + self.stdin_closed_at_eof = true; + } + _ => {} + } self.stdin = None; } else { self.stdin = slot; diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index f2c9e51d86..f0049aa762 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -5915,3 +5915,77 @@ fn bash_schema_stdin_max_length_matches_the_enforced_inline_cap() { ); } } + +#[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 + // box's drop (`cat` on a pty exited 0 the instant it dropped); the + // dup'd-master rewrite made that drop invisible, so an explicit + // `close_stdin: true` must write the ^D semantics itself. Without it + // the command below runs until the watchdog and this pin reddens. + let tmp = tempdir().expect("tempdir"); + let ctx = ToolContext::new(tmp.path()); + let shell_manager = ctx.shell_manager.clone(); + let command = echo_stdin_command(); + let task_ctx = ctx.clone(); + + let task = tokio::spawn(async move { + BashTool::new("Bash") + .execute( + json!({"command": command, "combined_output": true, "timeout_ms": 600_000}), + &task_ctx, + ) + .await + .expect("execute") + }); + tokio::time::sleep(Duration::from_millis(300)).await; + shell_manager + .lock() + .expect("shell manager lock") + .request_foreground_background(); + let result = tokio::time::timeout(Duration::from_secs(5), task) + .await + .expect("foreground shell should detach") + .expect("task should not panic"); + let task_id = result + .metadata + .as_ref() + .and_then(|meta| meta.get("task_id")) + .and_then(Value::as_str) + .expect("task id") + .to_string(); + + let mut manager = shell_manager.lock().expect("shell manager lock"); + // Feed one line first so the close proves itself against a command that + // is genuinely mid-read, then deliver end-of-file. + manager + .write_stdin(&task_id, "close-me\n", false) + .expect("the terminal lane keeps a writable stdin while the command runs"); + manager + .write_stdin(&task_id, "", true) + .expect("an explicit stdin close on the terminal lane must succeed"); + let mut exited = 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.exit_code.is_some() { + exited = true; + break; + } + } + assert!( + exited, + "closing stdin must end the terminal command (base parity with the \ + ownership-box drop): {:?}", + manager.inspect_job(&task_id).expect("inspect job").snapshot + ); + // The post-close refusal must tell the truth: stdin readers have really + // seen end-of-file now. + let error = manager + .write_stdin(&task_id, "again\n", false) + .expect_err("writes after the close must be refused"); + assert!( + error.to_string().contains("end-of-file"), + "the post-close refusal must cite end-of-file: {error}" + ); +} From 8c6c00dc0c821adb577e1f7a1f705d55ab2f2756 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Thu, 5 Nov 2026 01:41:07 +0800 Subject: [PATCH 4/7] fix(shell): seed the stdin state fields in the test helper initializer Main's #123/#124 added `seed_running_shell_for_test` while this slice was in flight; the rebased replay needs its `BackgroundShell` literal to carry the two stdin state fields this slice introduced. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 5b4c585cbf..00a7d4cb20 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -2330,6 +2330,8 @@ impl ShellManager { stderr_cursor: 0, completion_reported: false, stdin: None, + stdin_closed_at_eof: false, + stdin_write_stalled: false, child: None, stop_unconfirmed: false, #[cfg(windows)] From 7b754c7df3c788969e322fdf34eedd3e78dd4589 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Wed, 4 Nov 2026 18:44:51 +0800 Subject: [PATCH 5/7] fix(shell): compound read-only probes behind the operand gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second slice of the #126 split: the bounded read-only probe lane. - Sequences (`a; b`, `a && b`, `a || b`) admit iff every segment individually passes the read-only gate, via one shared splitter; `cd ` is admitted inside a sequence with its operand shape shared by classifier and execution seam, and routed to the shell lane (it is a builtin); read-only `od` dumps and clustered short text-filter options join the grammar. - Every admitted operand is judged at the execution seam: the cwd folds left-to-right through admitted `cd`s and connector conditionals, glob operands are expanded and judged match by match under shared caps and a wall-clock budget (dot/dot-dot synthesis, `**` refusal, absolute-spelling walks included), and `=cmd`, tilde, verbatim and post-dash wildcard spellings refuse. - Admitted git reads are hardened: repository `filter.` / textconv conversions are pinned to inert helpers via argv config, discovery enumerates registered submodules and linked worktrees byte-exactly like git (multiline/NUL/CRLF/BOM values, padded links, quoted gitlink spellings), under bounded scans, a watchdog, and a git-version floor for env-config pins. `core.worktree` values carrying quotes or backslashes refuse: git escape-decodes quoted values, so the raw spelling validated here is not the string git applies (round-19 review). - gh reads judge every stage (compound and `*`-bearing stages obey the host network policy and the worker envelope); the pipefail prefix keys on the shell kind, and cmd.exe/PowerShell delimiter walls are stated in refusals. - The `[shell.readonly.command]` refusal enumerates admitted families, the every-segment rule, and refused shapes; the worker catalog's read-only gh pre-filter uses the same per-stage predicate. Tests: 114 new pins across classifier, operand gate, glob walker, git discovery/parser differential (incl. the escape-decode worktree wall, mutation-verified 2/2 red), network guards, and lane routing; every listed escape was reproduced end-to-end against the unfixed code before fixing (18 review rounds). Targeted: `tools::shell::` + `command_safety::` + `tools::subagent::` + `tools::registry::` + `tools::verifier::` 1019 passed / 0 failed / 1 ignored (the ignored test pre-existing; `tools::verifier::` is the current filter name — the pre-split `tests::verifier::` spelling no longer matches); fmt clean; fork-CI clippy flags clean. Split of Pinvou/CodeWhale#126, rebased onto fix/shell-stdin-eof @6337b0e0a (the stdin-review round); full pre-split content preserved on archive/p126-pre-split-8705ccd85. Signed-off-by: asto18089 --- crates/tui/src/command_safety.rs | 1481 ++++- crates/tui/src/tools/registry.rs | 4 +- crates/tui/src/tools/shell.rs | 3485 +++++++++++- crates/tui/src/tools/shell/tests.rs | 7158 ++++++++++++++++++++++-- crates/tui/src/tools/subagent/mod.rs | 11 +- crates/tui/src/tools/subagent/tests.rs | 1105 +--- 6 files changed, 11312 insertions(+), 1932 deletions(-) diff --git a/crates/tui/src/command_safety.rs b/crates/tui/src/command_safety.rs index 902968af9a..160a2c145c 100644 --- a/crates/tui/src/command_safety.rs +++ b/crates/tui/src/command_safety.rs @@ -420,7 +420,25 @@ const GITHUB_READONLY_PREFIXES: &[&str] = &[ /// /// Words that do not look like Windows absolute paths are untouched, so POSIX /// escapes and unix hosts are unaffected. -pub(crate) fn normalize_windows_command_paths(command: &str) -> String { +/// +/// The transform is applied only on Windows hosts. A judged spelling and the +/// bytes a POSIX shell actually parses must be the same string: the strip +/// runs *before* the glob-charset gate, so on unix a `\\?\`-bearing word would +/// slip past the `?` refusal and reach the shell as a live one-character glob +/// (`cat a\\?\b` judged as `ab` while the shell expands `a\?b`), and a +/// doubled-backslash word would be judged under a spelling the program never +/// receives. On unix those spellings are plain (odd) POSIX file names and +/// fail closed at the charset and shape gates instead. The inner transform is +/// exposed unconditionally for tests. +pub(crate) fn normalize_windows_command_paths(command: &str) -> std::borrow::Cow<'_, str> { + if cfg!(windows) { + std::borrow::Cow::Owned(protected_windows_command_paths(command)) + } else { + std::borrow::Cow::Borrowed(command) + } +} + +pub(crate) fn protected_windows_command_paths(command: &str) -> String { let stripped = command.replace(r"\\?\", ""); let mut out = String::with_capacity(stripped.len()); let mut word_start = 0; @@ -473,7 +491,8 @@ pub fn is_parallel_readonly_command(command: &str) -> bool { if trimmed.chars().any(|ch| { matches!( ch, - '\n' | '\r' + '\0' | '\n' + | '\r' | ';' | '&' | '|' @@ -561,56 +580,264 @@ fn readonly_tokens_admitted(trimmed: &str) -> bool { /// untouched for the parent's parallel auto-approve chunks, where its /// tightness is load-bearing): /// +/// - compound sequences `a; b`, `a && b`, `a || b`, where **every** segment +/// must itself be an admitted read-only command (an empty segment — +/// including `;;` or a trailing `;` — rejects); a bare `&` still refuses; +/// at most `MAX_READONLY_SEQUENCE_SEGMENTS` segments; +/// - `cd ` as any whole, unpiped sequence segment +/// (standalone, leading, or later in the sequence; the execution seam +/// folds each one into the working directory of the segments after it) — +/// never a pipeline stage, where it would run in a subshell and persist +/// nowhere the operand gate could follow, and never a wildcard, because +/// the shell would enter the first expansion while the seam judged the +/// spelling — with the operand additionally resolved against the +/// workspace, and required to be a directory, at the execution seam; +/// - read-only `od` dump forms (display radices plus numeric `-j`/`-N`), +/// piped in either direction like any other segment on BSD and GNU hosts; /// - pipelines `a | b`, where **every** segment must itself be an admitted /// read-only command (an empty segment — including `||` — rejects); -/// - glob `*` arguments, expanded by the shell only against workspace paths -/// the operand gate already confines; +/// - glob `*` arguments, the only wildcard the charset admits; the operand +/// gate expands each one against the live working directory and judges +/// every match by its resolved location before any shell runs; /// - `git -C ` and `git --no-pager `, whose /// remainder re-enters the existing per-subcommand option tables; /// - `find` without any mutating primary (`-delete`, `-exec`, `-execdir`, -/// `-ok`, `-okdir`, `-fprintf`, `-fls`, `-fprint`, `-fprint0`); -/// - `sed -n 'p` — numeric line-range print only, no script verbs -/// (`w`/`r`/`e`/`s`) can appear in a two-token range script; +/// `-ok`, `-okdir`, `-truncate`, `-fprintf`, `-fls`, `-fprint`, `-fprint0`); +/// - `sed -n 'p ` — one numeric line-range print script (no +/// script verbs `w`/`r`/`e`/`s`), every later token a plain input file: a +/// later `-e`/`-f` script would escape the range-only contract; /// - `npm view|show|info ` — registry reads, matching the scout role's /// network-capable read-only posture; /// - pure text filters `sort`, `uniq`, `cut`, `tr`, `comm` as pipeline -/// stages. +/// stages or standalone segments; /// -/// Everything else keeps the parallel classifier's posture: no separators, -/// redirects, backgrounding, command/parameter expansion, subshells, or +/// Everything else keeps the parallel classifier's posture: no bare `&` +/// backgrounding, redirects, command/parameter expansion, subshells, or /// env-assignment prefixes. pub fn is_agent_readonly_shell_command(command: &str) -> bool { + // On Windows the normalize below copies the text, so the length bound + // fires before that allocation (a giant command is refused without + // paying for its copy). Backslash doubling can still grow the text past + // the bound, so the post-normalize check stays load-bearing. + #[cfg(windows)] + if command.len() > MAX_READONLY_COMMAND_LEN { + return false; + } let trimmed = normalize_windows_command_paths(command); let trimmed = trimmed.trim(); if trimmed.is_empty() { return false; } + if trimmed.len() > MAX_READONLY_COMMAND_LEN { + return false; + } if trimmed.chars().any(|ch| { matches!( ch, - '\n' | '\r' - | ';' - | '&' - | '>' - | '<' - | '`' - | '$' - | '?' - | '[' - | ']' - | '{' - | '}' - | '(' - | ')' + '\0' | '\n' | '\r' | '>' | '<' | '`' | '$' | '?' | '[' | ']' | '{' | '}' | '(' | ')' ) }) { return false; } - // A pipeline is admitted only when every segment is: `a | b` is two - // read-only commands, while `a | | b`, `a |`, and `||` all carry an empty - // segment and reject. Quoted pipes inside an argument mis-split here, - // which only ever makes a segment fail classification (fail closed). - trimmed.split('|').all(is_agent_readonly_segment) + // A sequence (`;` / `&&` / `||`) is admitted only when every segment is: + // the same every-segment rule the pipeline split applies, one level up. + // A bare `&` refuses. A quoted separator inside an argument can + // mis-split here into segments that still classify (the splitter falls + // back to quote-trimming whitespace splitting when the strict parse + // fails) — that is safe only because the execution seam re-parses every + // segment with the strict tokenizer and refuses what it cannot parse + // byte-for-byte; the fail-closed wall for this shape lives at the seam, + // not here. + let Some(segments) = readonly_sequence_segments_with_connectors(trimmed) else { + return false; + }; + // The execution seam resolves one `cd` destination per segment against + // the filesystem, so an unbounded sequence would let a single command + // buy O(segments × path depth) of gate work; real probes are a handful. + if segments.len() > MAX_READONLY_SEQUENCE_SEGMENTS { + return false; + } + segments.into_iter().all(|(segment, connector)| { + // A `cd` joined by `&&`/`||` runs only when the predecessor's exit + // status allows it — a runtime fact the gate cannot know — while the + // seam folds every admitted `cd` unconditionally. The fold would + // judge later operands in a directory the shell never entered, + // skipping whatever exists only at the real cwd (`cat nope && cd + // sub; cat link` reads a root symlink unjudged). Refuse the shape + // statically; the seam re-checks it at the fold. + if matches!( + connector, + ReadonlySequenceConnector::AndThen | ReadonlySequenceConnector::OrElse + ) && shell_words(segment) + .first() + .is_some_and(|word| word == "cd") + { + return false; + } + readonly_segment_stages_admitted(segment) + }) +} + +/// Upper bound on `;`/`&&`/`||` segments in one admitted read-only command. +pub(crate) const MAX_READONLY_SEQUENCE_SEGMENTS: usize = 32; + +/// Upper bound on the text length of one admitted read-only command. The +/// grammar's refusals are checked in linear passes that allocate transiently +/// (the segment split, the shell-lane hardening copy), so an uncapped command +/// would buy host memory and parse work at dispatch time for text no legitimate +/// bounded probe needs. The execution seam runs only after this admission, so +/// this one check bounds both layers. +const MAX_READONLY_COMMAND_LEN: usize = 64 * 1024; + +/// Admit one sequence segment: every pipeline stage must be an admitted +/// read-only command (`a | b` is two read-only commands, while `a | | b`, +/// `a |`, and a dangling pipe around a separator all carry an empty stage and +/// reject), and `cd` may only be the whole segment. A pipeline stage runs in +/// a subshell, so `cd sub | cat f` changes no working directory the operand +/// gate could follow — the gate folds `cd` into the cwd of later segments +/// precisely because an unpiped `cd` does persist — and a `cd` stage would +/// otherwise end the gate's token walk before the operands after the `|`. +fn readonly_segment_stages_admitted(segment: &str) -> bool { + let stages = readonly_pipeline_stages(segment); + let piped = stages.len() > 1; + stages.into_iter().all(|stage| { + if piped && shell_words(stage).first().is_some_and(|word| word == "cd") { + return false; + } + is_agent_readonly_segment(stage) + }) +} + +/// Split one sequence segment on the pipeline separator `|`. +/// +/// The one stage split every read-only judge uses — the classifier above, +/// the execution seam's operand gate, its network guard, and its git stage +/// hardening — so no two of them can ever disagree about where a stage +/// begins. The split is naive byte scanning like [`readonly_sequence_segments`] +/// (a quoted `|` mis-splits into a stage that fails classification, or fails +/// the seam's stricter `shlex` parse: fail closed either way). Empty stages +/// are returned, not dropped; refusing them is the caller's contract. +pub(crate) fn readonly_pipeline_stages(segment: &str) -> Vec<&str> { + segment.split('|').collect() +} + +/// Whether any pipeline stage of any sequence segment of `command` is an +/// admitted GitHub read ([`is_github_readonly_stage`]), i.e. whether running +/// the command reaches `api.github.com`. `pwd; gh repo view`, `gh repo view | +/// head`, and `gh pr list --search '*'` classify as gh reads exactly like a +/// bare `gh repo view`, so every network ceiling judges them with this one +/// predicate instead of the whole-command parallel classifier, whose charset +/// gate would see a compound or a `*` and answer "not a gh read". At the +/// execution seam the operand gate additionally refuses the wildcard-bearing +/// gh stages outright; this predicate over-recognizes them so the network +/// pre-checks (a child with no network, the fleet envelope) still fail +/// closed. A bare `&` never classifies and answers `false` here; the grammar +/// refuses it anyway. +#[must_use] +pub(crate) fn command_reaches_github_readonly(command: &str) -> bool { + readonly_sequence_segments(command).is_some_and(|segments| { + segments + .iter() + .flat_map(|segment| readonly_pipeline_stages(segment)) + .any(is_github_readonly_stage) + }) +} + +/// How a sequence segment is joined to its predecessor. The distinction is +/// load-bearing for the execution seam's `cd` fold: `;` runs the segment +/// whatever the predecessor returned, while `&&`/`||` run it only when the +/// predecessor succeeded/failed — a runtime fact no pre-spawn gate can +/// evaluate, so a `cd` joined by a conditional connector must never fold +/// the working directory (the shell may never enter it). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum ReadonlySequenceConnector { + /// The first segment: always runs. + Start, + /// Joined by `;`: runs whatever the predecessor returned. + Sequence, + /// Joined by `&&`: runs only when the predecessor succeeded. + AndThen, + /// Joined by `||`: runs only when the predecessor failed. + OrElse, +} + +/// Split a command on the sequence separators `;`, `&&`, and `||`, keeping +/// the connector each segment was joined with. +/// +/// Returns `None` when a bare `&` (backgrounding) appears: `&&` separates a +/// sequence, a lone `&` refuses. The split is deliberately naive — straight +/// byte scanning with no quote parsing, exactly like the `|` pipeline split +/// in `readonly_segment_stages_admitted`. Quotes and backslashes pass the +/// charset gate, so a quoted `;` mis-splits here; a mis-split that still +/// classifies is refused before any shell runs by the operand gate's stricter +/// `shlex::split` (`enforce_readonly_workspace_operands`), which rejects the +/// now-unbalanced quote, so the residual risk is a refusal, never an +/// execution. The destructive-command pass keeps its own +/// `split_command_segments`, which folds `|` and a bare `&` into the same +/// level and drops empty segments — the opposite contract from this gate. +pub(crate) fn readonly_sequence_segments_with_connectors( + command: &str, +) -> Option> { + let mut segments = Vec::new(); + let mut connector = ReadonlySequenceConnector::Start; + let bytes = command.as_bytes(); + let mut start = 0; + let mut index = 0; + while index < bytes.len() { + let byte = bytes[index]; + let doubled = index + 1 < bytes.len() && bytes[index + 1] == byte; + if byte == b';' { + segments.push((&command[start..index], connector)); + connector = ReadonlySequenceConnector::Sequence; + index += 1; + start = index; + } else if doubled && (byte == b'&' || byte == b'|') { + segments.push((&command[start..index], connector)); + connector = if byte == b'&' { + ReadonlySequenceConnector::AndThen + } else { + ReadonlySequenceConnector::OrElse + }; + index += 2; + start = index; + } else if byte == b'&' { + // A single `&` is backgrounding, never admitted: the sequence + // split is where the charset gate used to refuse it. + return None; + } else { + index += 1; + } + } + segments.push((&command[start..], connector)); + Some(segments) +} + +/// Split a command on the sequence separators, dropping the connectors (for +/// consumers that judge every segment the same way regardless of when the +/// shell runs it). The `cd` fold at the execution seam must use +/// [`readonly_sequence_segments_with_connectors`] instead. +pub(crate) fn readonly_sequence_segments(command: &str) -> Option> { + readonly_sequence_segments_with_connectors(command) + .map(|segments| segments.into_iter().map(|(segment, _)| segment).collect()) +} + +/// Whether an admitted read-only command must run under a shell because it +/// names the `cd` builtin. The direct-argv lane execs a real binary, and +/// `cd` exists as one only on some hosts (macOS ships `/usr/bin/cd`; most +/// Linux systems do not), so any command with a `cd`-leading segment — at +/// any position in the sequence — takes the shell lane pipelines already +/// use, where the builtin always resolves. Pipeline-bearing and +/// multi-segment commands route to the shell anyway; the term is +/// load-bearing for the standalone `cd`, whose admission would otherwise +/// end in a not-found exec error. +pub(crate) fn agent_readonly_command_needs_shell_for_cd(command: &str) -> bool { + readonly_sequence_segments(command).is_some_and(|segments| { + segments.iter().any(|segment| { + shell_words(segment) + .first() + .is_some_and(|word| word == "cd") + }) + }) } fn is_agent_readonly_segment(segment: &str) -> bool { @@ -627,10 +854,12 @@ fn is_agent_readonly_segment(segment: &str) -> bool { return false; } match program.as_str() { + "cd" => is_agent_readonly_cd(&tokens), "git" => is_agent_readonly_git(&tokens), "find" => is_agent_readonly_find(&tokens), "sed" => is_agent_readonly_sed(&tokens), "npm" => is_agent_readonly_npm(&tokens), + "od" => is_agent_readonly_od(&tokens), "sort" => agent_text_filter_options_match( &tokens, &[ @@ -754,14 +983,17 @@ fn is_agent_readonly_segment(segment: &str) -> bool { /// Several of these programs have write or helper-execution forms despite /// looking like harmless stdout transforms (`sort -o`, `sort /// --compress-program`, and uniq's second FILE operand). Keep their accepted -/// options exact, reject attached/unknown flags, and cap operands where the -/// command's positional grammar can name an output file. +/// options exact, reject unknown flags, and cap operands where the command's +/// positional grammar can name an output file. Clustered short spellings +/// (`-rn`, `-d,`, `-f1`) are expanded first so that only admitted letters +/// ever reach the exact match — see [`expand_clustered_short_options`]. fn agent_text_filter_options_match( tokens: &[String], switches: &[&str], value_options: &[&str], max_operands: usize, ) -> bool { + let tokens = expand_clustered_short_options(tokens, switches, value_options); let mut index = 1; let mut options = true; let mut operands = 0; @@ -791,6 +1023,57 @@ fn agent_text_filter_options_match( true } +/// Expand the clustered short-option spellings POSIX `getopt` accepts and +/// models reach for first: `-rn` becomes `-r -n`, `-d,` becomes `-d ,`, and +/// `-f1` becomes `-f 1`. A cluster expands only when its first letter is an +/// admitted value option (the remainder is its value) or when every letter is +/// an admitted switch; anything else, every long option, and every token +/// after `--` is left verbatim so the exact-match grammar refuses it. +fn expand_clustered_short_options( + tokens: &[String], + switches: &[&str], + value_options: &[&str], +) -> Vec { + let mut expanded = Vec::with_capacity(tokens.len()); + let mut options = true; + for (index, token) in tokens.iter().enumerate() { + if index == 0 || !options || !token.starts_with('-') || token.starts_with("--") { + if index > 0 && options && token == "--" { + options = false; + } + expanded.push(token.clone()); + continue; + } + let mut letters = token.char_indices().skip(1); + let Some((_, first_letter)) = letters.next() else { + expanded.push(token.clone()); + continue; + }; + let remainder_start = letters.next().map_or(token.len(), |(offset, _)| offset); + let remainder = &token[remainder_start..]; + if remainder.is_empty() { + // A single short option is already in exact-match form. + expanded.push(token.clone()); + continue; + } + let first = format!("-{first_letter}"); + if value_options.contains(&first.as_str()) { + expanded.push(first); + expanded.push(remainder.to_string()); + } else if switches.contains(&first.as_str()) + && remainder + .chars() + .all(|letter| switches.contains(&format!("-{letter}").as_str())) + { + expanded.push(first); + expanded.extend(remainder.chars().map(|letter| format!("-{letter}"))); + } else { + expanded.push(token.clone()); + } + } + expanded +} + fn is_agent_readonly_git(tokens: &[String]) -> bool { // Skip the two safe global preambles; anything else before the // subcommand (e.g. `--git-dir`, `-c`) leaves it unclassified and @@ -1004,13 +1287,27 @@ fn is_agent_readonly_find(tokens: &[String]) -> bool { } fn is_agent_readonly_sed(tokens: &[String]) -> bool { + // The script is pinned to the third token, and every later token must be + // a plain input file: GNU sed's option permutation parses a later + // `-e