From d6787ccc04fa2f606230f10bae5d21295b8da22c Mon Sep 17 00:00:00 2001 From: asto18089 Date: Wed, 4 Nov 2026 18:44:08 +0800 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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 3eed997778cd17eb55980d6da89935b1c1184fd0 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Thu, 5 Nov 2026 14:35:06 +0800 Subject: [PATCH 5/9] fix(shell): bound stdin close delivery and SIGPIPE workers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fresh-review follow-ups on the stdin EOF slice (review 5779706794): - Host kill: a bounded stdin write that hits the deadline abandons a thread blocked mid-write on the handle; the documented stall recovery (killing the task, or the stalled command exiting later) closed the read end, and the resumed write raised SIGPIPE on a process that runs with SIGPIPE at SIG_DFL (run_with_args) — killing the whole host process. Every stdin write worker (unix fd-clone, owned fallback, the new close delivery) now blocks SIGPIPE for the write (BlockedSigpipeGuard) so the abandoned write observes EPIPE and the existing write-error arm handles it; the sync-lane inline payload write gets the same guard. Not directly testable on unix CI (test binaries keep SIG_IGN), so this is a mechanism fix pinned by code path, not a red/green test. - Session wedge: interact close_stdin on a unix terminal task delivered the ^D bytes inline under the shell-manager lock; two bytes into a tty whose input queue filled earlier (completed payload, no stall flag) block as forever as a large payload would. The delivery now runs on a bounded worker (bounded_stdin_eof_delivery): Delivered claims end-of-file, a failed write consumes the handle without claiming, and a deadline stall restores the handle under the stall flag. - The unix ^D pin is now cfg-gated like its sibling tty pins — the close semantics it asserts exist only under all(unix, not(target_env = "ohos")); on Windows it was deterministically red and on ohos it panicked. - The windows-only killed_shell test's BackgroundShell literal seeds the two stdin state fields it was missing (E0063 on windows test builds). - Worker spawns use thread::Builder and map spawn failure to a write error instead of panicking through the shell-manager lock (permanent poison); the owned fallback documents that a spawn failure releases the writer with the failed closure. - A spawn-time stdin write failure now says the run never started (no task id exists for the kill-and-restart copy), and the MAX_INLINE_STDIN_BYTES / STDIN_WRITE_DEADLINE comments state what the deadline actually does today (bounded waits, n x 5 s aggregate under the lock, spawn aborts). - Copy: the eof refusal says "stdin was closed" (terminal closes are not pipes); guidance names tty in the no-terminal-stdin clause; the schema's close_stdin describes the newline + EOF delivery; the stale PTY-close comments (variant doc, field doc, spawn path, test gate justification) now match the delivered behavior; Windows close_stdin on a terminal task refuses explicitly instead of returning a silent Ok (ConPTY has no EOF to deliver). New pin close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof is red against the stall-conjunct-removal mutant (verified: the mutant's close waits the full deadline and the pin reds at its elapsed bound) and green here. Targeted: tools::shell 169 passed / 0 failed / 1 ignored (pre-existing), tools::verifier 9 / 0, tools::registry 48 / 0; cargo fmt --all -- --check clean; fork-CI clippy flag set clean on codewhale-tui in an isolated target dir. The windows-gated edits are verified by construction only (field names copied from the declarations; platform-neutral syntax) — this host has no windows C cross toolchain for the dependency build scripts. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 359 +++++++++++++++++++------ crates/tui/src/tools/shell/guidance.rs | 2 +- crates/tui/src/tools/shell/tests.rs | 123 ++++++++- 3 files changed, 397 insertions(+), 87 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 00a7d4cb20..6868a39cd2 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -877,9 +877,11 @@ enum StdinWriter { /// writes can run on the bounded worker thread exactly like pipe /// writes. The reader thread holds its own clone of the master fd for /// the task's whole lifetime, so this dup is NOT the last master-side - /// handle: dropping it delivers nothing to the slave (the child sees no - /// EOF from a close here), which is why PTY closes never claim EOF — - /// the same observable behavior the `take_writer` box had. + /// handle: dropping it delivers nothing to the slave on its own. An + /// explicit `close_stdin` therefore delivers the ^D bytes itself, + /// through the same bounded worker as a payload write (see + /// [`bounded_stdin_eof_delivery`]), and a delivery that fails or + /// stalls claims no end-of-file. /// The cfg mirrors the only constructor (the unix arm inside the /// `not(target_env = "ohos")` PTY block — ohos returns from tty mode /// before PTY setup): keeping the variant off ohos avoids a @@ -913,6 +915,110 @@ impl StdinWriter { } } +/// Blocks SIGPIPE for the guarded region on unix, restoring it on drop. +/// The process deliberately runs with SIGPIPE at SIG_DFL (`run_with_args` +/// re-arms it so delegated children see conventional pipe semantics), which +/// arms it for in-process threads too. A bounded stdin write that times out +/// deliberately leaves its worker blocked mid-write on the handle, and the +/// documented stall recovery — killing the task, or the stalled command +/// simply exiting later — closes the read end: the resumed write would +/// otherwise kill the whole host process with SIGPIPE instead of failing +/// with EPIPE like every handled write error. +struct BlockedSigpipeGuard { + #[cfg(unix)] + previous: libc::sigset_t, +} + +impl BlockedSigpipeGuard { + fn new() -> Self { + #[cfg(unix)] + { + // SAFETY: `zeroed` is valid for `sigset_t` (an array of + // integer words) and both pointers below are local stack + // values; a failed `pthread_sigmask` simply leaves the + // process default disposition and the failure mode is the + // pre-guard one. + unsafe { + let mut blocked: libc::sigset_t = std::mem::zeroed(); + let mut previous: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut blocked); + libc::sigaddset(&mut blocked, libc::SIGPIPE); + // Best-effort: if the mask cannot be set, the write runs + // with the process default and the failure mode is simply + // the pre-guard one. + libc::pthread_sigmask(libc::SIG_BLOCK, &blocked, &mut previous); + Self { previous } + } + } + #[cfg(not(unix))] + Self {} + } +} + +#[cfg(unix)] +impl Drop for BlockedSigpipeGuard { + fn drop(&mut self) { + unsafe { + libc::pthread_sigmask(libc::SIG_SETMASK, &self.previous, std::ptr::null_mut()); + } + } +} + +#[cfg(not(unix))] +impl Drop for BlockedSigpipeGuard { + fn drop(&mut self) {} +} + +/// Outcome of the bounded end-of-file delivery behind an explicit +/// `close_stdin` on a unix terminal task: `Delivered` means the ^D bytes +/// landed, `Failed` means the write errored without landing (typically the +/// child was already gone and the master write failed), and `Stalled` means +/// the tty input queue was still full at the deadline. +#[cfg(all(unix, not(target_env = "ohos")))] +enum BoundedEofDelivery { + Delivered, + Failed, + Stalled, +} + +/// Deliver `\n` + VEOF through the given master fd on the same kind of +/// bounded worker a payload write uses. The leading newline flushes any +/// pending partial line, because canonical mode only turns VEOF into a +/// read of length zero from an EMPTY line buffer — the same two bytes the +/// old ownership box injected on drop. In raw mode both bytes are ordinary +/// input, exactly like that box drop; the caller records a delivery only +/// when the write really completed, so the flag never claims an end-of-file +/// that did not land. +#[cfg(all(unix, not(target_env = "ohos")))] +fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { + use std::os::fd::AsFd; + let dup_fd = match file.as_fd().try_clone_to_owned() { + Ok(fd) => fd, + // No clone means no worker can attempt the delivery; report it as + // a failed delivery — a clone failure is a process-fd problem, not + // a queue-full condition, and the flag must stay down either way. + Err(_) => return BoundedEofDelivery::Failed, + }; + let mut writer = std::process::ChildStdin::from(dup_fd); + let (sender, receiver) = std::sync::mpsc::channel(); + let worker = std::thread::Builder::new() + .name("stdin-eof-delivery".to_string()) + .spawn(move || { + let _sigpipe = BlockedSigpipeGuard::new(); + let _ = sender.send(writer.write_all(b"\n\x04").and_then(|_| writer.flush())); + }); + let receiver = match worker { + Ok(_) => receiver, + Err(_) => return BoundedEofDelivery::Failed, + }; + match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { + Ok(Ok(())) => BoundedEofDelivery::Delivered, + Ok(Err(_)) => BoundedEofDelivery::Failed, + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => BoundedEofDelivery::Stalled, + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => BoundedEofDelivery::Failed, + } +} + /// 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 @@ -923,29 +1029,46 @@ impl StdinWriter { /// `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. +/// fault. An `Err` from the function itself means the worker thread could +/// not be spawned at all: nothing was written, and the writer is gone with +/// the failed closure (ownership was already handed over), so the caller +/// must report the task as having no writable stdin. A plain +/// `thread::spawn` panic here would unwind through the shell-manager lock +/// and poison it for the rest of the process. 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))] +type OwnedBoundedWriteOutcome = ( + Option, + Result, std::sync::mpsc::RecvTimeoutError>, +); + #[cfg_attr(all(unix, not(target_env = "ohos")), allow(dead_code))] fn bounded_stdin_write_owned( writer: StdinWriter, payload: Vec, -) -> ( - Option, - Result, std::sync::mpsc::RecvTimeoutError>, -) { +) -> anyhow::Result { let (sender, receiver) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let mut writer = writer; - let result = writer.write_all(&payload).and_then(|_| writer.flush()); - // The writer moves back out only on the send path; a send failure - // (receiver gone) drops it, which is fine — the caller already - // gave up on the handle at that point. - let _ = sender.send((writer, result)); - }); + let worker = std::thread::Builder::new() + .name("stdin-bounded-write".to_string()) + .spawn(move || { + let _sigpipe = BlockedSigpipeGuard::new(); + let mut writer = writer; + let result = writer.write_all(&payload).and_then(|_| writer.flush()); + // The writer moves back out only on the send path; a send failure + // (receiver gone) drops it, which is fine — the caller already + // gave up on the handle at that point. + let _ = sender.send((writer, result)); + }); + let receiver = match worker { + Ok(_) => receiver, + Err(error) => { + return Err(anyhow!(error).context("Failed to spawn the bounded stdin writer thread")); + } + }; match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok((writer, result)) => (Some(writer), Ok(result)), - Err(error) => (None, Err(error)), + Ok((writer, result)) => Ok((Some(writer), Ok(result))), + Err(error) => Ok((None, Err(error))), } } @@ -1133,12 +1256,14 @@ pub struct BackgroundShell { stderr_cursor: usize, completion_reported: bool, stdin: Option, - /// Set when the stdin *pipe* was closed through `write_stdin`, i.e. the - /// child has really been handed end-of-file. Stays false when the task - /// never exposed a writable stdin (persistent services spawn with a null - /// stdin, PTY lanes keep the slave open through the reader) or when the - /// handle was simply released after completion, so `interact` can state - /// the true reason stdin is unavailable. + /// Set when the child has really been handed end-of-file: either its + /// stdin pipe was closed through `write_stdin`, or an explicit unix + /// terminal close delivered the ^D bytes through the bounded worker. + /// Stays false when the task never exposed a writable stdin (persistent + /// services spawn with a null stdin, PTY lanes keep the slave open + /// through the reader), when the handle was simply released after + /// completion, and when a close delivery failed or stalled — so + /// `interact` can state the true reason stdin is unavailable. stdin_closed_at_eof: bool, /// Set when a stdin write outlived [`STDIN_WRITE_DEADLINE`] because the /// command stopped consuming its pipe. The abandoned bytes keep flowing @@ -1564,10 +1689,27 @@ impl BackgroundShell { let mut writer = std::process::ChildStdin::from(dup_fd); let payload = input.as_bytes().to_vec(); let (sender, receiver) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let _ = - sender.send(writer.write_all(&payload).and_then(|_| writer.flush())); - }); + let worker = std::thread::Builder::new() + .name("stdin-bounded-write".to_string()) + .spawn(move || { + let _sigpipe = BlockedSigpipeGuard::new(); + let _ = sender + .send(writer.write_all(&payload).and_then(|_| writer.flush())); + }); + let receiver = match worker { + Ok(_) => receiver, + Err(error) => { + // The spawn failed before any byte was written + // and the worker holds nothing: restore the + // handle and fail like a plain write error. A + // bare `thread::spawn` panic here would unwind + // through the shell-manager mutex and poison it + // for the rest of the process. + self.stdin = slot; + return Err(anyhow!(error) + .context("Failed to spawn the bounded stdin writer thread")); + } + }; match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { Ok(Ok(())) => {} Ok(Err(error)) => { @@ -1619,8 +1761,17 @@ impl BackgroundShell { #[cfg(any(target_env = "ohos", not(unix)))] { let writer = slot.take().expect("slot held above"); - let (returned, result) = - bounded_stdin_write_owned(writer, input.as_bytes().to_vec()); + let (returned, result) = bounded_stdin_write_owned( + writer, + input.as_bytes().to_vec(), + ) + .map_err(|error| { + // The worker could not be spawned and the writer + // went with the failed closure: `slot` stays None, + // so the task honestly reports no writable stdin + // from here on. + anyhow!(error).context("Failed to spawn the bounded stdin writer thread") + })?; match result { Ok(Ok(())) => { slot = returned; @@ -1665,29 +1816,64 @@ impl BackgroundShell { // on a pty exited 0 the instant it dropped), and the dup // the terminal lane now holds delivers nothing on its own // because the reader holds the master — so write the same - // ^D semantics through the dup, then drop it. The flag - // only follows a real delivery. A task whose last write - // STALLED must not claim EOF either: the abandoned worker - // still holds a clone of the handle and the bytes have not - // finished landing, so the flag stays down and the tail - // refusal reports the stall instead. - match slot { - Some(StdinWriter::Pipe(_)) => { - self.stdin_closed_at_eof = !self.stdin_write_stalled; - } - #[cfg(all(unix, not(target_env = "ohos")))] - Some(StdinWriter::PtyFile(mut file)) if !self.stdin_write_stalled => { - { - use std::io::Write as _; - // Ignored if the child is already gone; in raw - // mode VEOF reads as ordinary input, exactly - // like the box drop it replaces. - let _ = file.write_all(b"\n\x04"); - let _ = file.flush(); + // ^D semantics through the dup, then drop it. The delivery + // runs on the same kind of bounded worker as a payload + // write: two bytes into a tty whose input queue is already + // full (the earlier payload completed inside the deadline, + // so the stall flag is still down) block just as forever + // as a large payload would, and inline under the manager + // lock they wedge every shell in the session. + #[cfg(all(unix, not(target_env = "ohos")))] + if let Some(StdinWriter::PtyFile(file)) = slot.as_ref() + && !self.stdin_write_stalled + { + match bounded_stdin_eof_delivery(file) { + BoundedEofDelivery::Delivered => { + self.stdin = None; + self.stdin_closed_at_eof = true; + return Ok(()); + } + // The close consumed the handle as requested, but + // the bytes did not land (typically the child was + // already gone and the master write failed): keep + // the flag down so later refusals do not claim + // readers saw end-of-file. + BoundedEofDelivery::Failed => { + self.stdin = None; + return Ok(()); + } + // Queue still full at the deadline: restore the + // handle under the stall flag, mirroring the + // payload-timeout disposition, so later writes + // cite the stall instead of a delivery that has + // not happened. + BoundedEofDelivery::Stalled => { + self.stdin_write_stalled = true; + self.stdin = slot; + return Ok(()); } - self.stdin_closed_at_eof = true; } - _ => {} + } + // Windows cannot deliver end-of-file to a console child at + // all (portable-pty's ConPTY writer has no EOF-on-drop — + // that exists only in its unix writer), so an explicit + // close here only releases the handle. Say so instead of + // returning a silent success that the schema's "Close + // stdin" would read as a delivered end-of-file. + #[cfg(all(not(unix), not(target_env = "ohos")))] + if matches!(slot, Some(StdinWriter::Pty(_))) { + self.stdin = None; + return Err(anyhow!( + "stdin close for task {} released the terminal's stdin handle, but this platform cannot deliver end-of-file to a console child; the command keeps running until it finishes or its timeout ends it.", + self.id + )); + } + // A task whose last write STALLED must not claim EOF: the + // abandoned worker still holds a clone of the handle and + // the bytes have not finished landing, so the flag stays + // down and the tail refusal reports the stall instead. + if let Some(StdinWriter::Pipe(_)) = slot { + self.stdin_closed_at_eof = !self.stdin_write_stalled; } self.stdin = None; } else { @@ -1709,7 +1895,7 @@ impl BackgroundShell { if self.stdin_closed_at_eof { return Err(anyhow!( - "stdin is not available for task {}: its stdin pipe was closed, so stdin readers have seen end-of-file; start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file; start input-driven work with background=true and feed it with action=\"interact\"", self.id )); } @@ -2823,6 +3009,11 @@ impl ShellManager { if let Some(input) = stdin_data && let Some(mut stdin) = child.stdin.take() { + // Same SIGPIPE discipline as the bounded stdin workers (see + // [`BlockedSigpipeGuard`]): the child dying mid-write must + // surface as a handled write error, not kill a process that + // runs with SIGPIPE at its default disposition. + let _sigpipe = BlockedSigpipeGuard::new(); stdin .write_all(input.as_bytes()) .context("Failed to write to stdin")?; @@ -3150,12 +3341,14 @@ impl ShellManager { // as an immediate end-of-file (probe-pinned: `cat` on a pty // exited 0 the instant the box was dropped). The dup keeps // a master-side description open — no EOF for the slave - // while the reader's clone is open too. `close_stdin` on - // this lane drops the dup: with the reader's clone still - // open the child sees nothing (the PTY close never promised - // an EOF and `stdin_closed_at_eof` never claimed one); the - // final master handle closes at teardown, after the child - // can no longer observe it. + // while the reader's clone is open too. A plain drop of the + // dup (the spawn-path close never touches this lane, and a + // stalled close releases it) still shows the child nothing; + // an explicit `close_stdin` instead delivers the ^D bytes + // itself through the bounded worker and claims end-of-file + // only when that write really landed; the final master + // handle closes at teardown, after the child can no longer + // observe it. #[cfg(unix)] let stdin_writer = { use std::os::fd::FromRawFd; @@ -3347,8 +3540,14 @@ impl ShellManager { if let Some(input) = stdin_data && let Err(err) = bg_shell.write_stdin(input, false) { + // No task id exists yet — the record is only registered below — + // so `write_stdin`'s "kill it and start a new one" recovery copy + // has no target here. The spawn aborts instead: the child is + // killed and the caller learns the run never started. let _ = bg_shell.kill(); - return Err(err); + return Err(err.context( + "the run never started: delivering the inline stdin payload failed, so the command was killed and no task was created", + )); } if let Err(err) = bg_shell.publish_lifecycle() { @@ -5044,25 +5243,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. +/// Upper bound for one inline `stdin` payload. Every accepted payload is +/// delivered through the bounded stdin writer (see +/// [`STDIN_WRITE_DEADLINE`]), so a command that stops reading surfaces as a +/// deadline stall rather than a wedged executor; the cap additionally keeps +/// oversized payloads from ever reaching the writer, and a 64 KiB payload +/// fits a Linux pipe buffer in one atomic write so the common +/// payload-then-close foreground shape delivers without stalling. Anything +/// larger is refused with a pointer at background `interact` streaming +/// instead of hanging the session. const MAX_INLINE_STDIN_BYTES: usize = 64 * 1024; -/// Wall-clock bound on one stdin write. The same blocking `write_all` runs -/// for `interact` payloads (and for the spawn payload on platforms whose -/// pipe buffer is smaller than the cap), where a command that stops -/// consuming stdin fills its pipe and would hold the shell-manager lock -/// forever. At the bound the write is abandoned to a background worker (the -/// bytes still land if the child resumes reading) and the task's stdin is -/// marked stalled. +/// Wall-clock bound on one stdin write. At the bound an `interact` write is +/// abandoned to its background worker (the bytes still land if the child +/// resumes reading), the task's stdin is marked stalled, and the manager +/// lock is released — so each stalled write costs one bounded wait under +/// the lock, and n stalled tasks serialize into n such waits. A stalled +/// SPAWN payload is different: no task id exists yet, so the spawn aborts +/// and the child is killed instead of running task-less. The abandoned +/// worker is SIGPIPE-blocked (see [`BlockedSigpipeGuard`]): when the task +/// is later killed — the documented stall recovery — the worker observes +/// EPIPE instead of taking the process down. const STDIN_WRITE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); /// Initial cadence for foreground-via-background completion polling. Fast @@ -5447,7 +5648,7 @@ impl ToolSpec for BashTool { }, "close_stdin": { "type": "boolean", - "description": "Close stdin after sending (action=interact)" + "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte" }, "all": { "type": "boolean", diff --git a/crates/tui/src/tools/shell/guidance.rs b/crates/tui/src/tools/shell/guidance.rs index 258512032a..5ca9ff5aab 100644 --- a/crates/tui/src/tools/shell/guidance.rs +++ b/crates/tui/src/tools/shell/guidance.rs @@ -86,7 +86,7 @@ pub(super) fn description() -> &'static str { \"interact\" sends stdin to a background task; \"cancel\" kills a background task. \ Pass wait=false for a nonblocking task snapshot. Foreground mode is for bounded \ commands; use background=true for work expected to take >5 seconds. Foreground \ - commands without `interactive` or `combined_output` run without a \ + commands without `interactive`, `tty`, or `combined_output` run without a \ terminal stdin — interactive programs and a bare `$SHELL` receive EOF and \ exit; `tty` always selects a background terminal task, and a foreground \ command with `combined_output` runs on a terminal lane that nothing \ diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index f0049aa762..4e08662002 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -3697,6 +3697,8 @@ fn killed_shell_does_not_wait_for_blocked_reader_threads() { completion_reported: false, bounded_output: None, stdin: None, + stdin_closed_at_eof: false, + stdin_write_stalled: false, child: None, stop_unconfirmed: false, windows_job: None, @@ -5073,9 +5075,9 @@ async fn inline_stdin_payload_over_the_cap_refuses() { /// 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)]`, as are the raw-mode tty semantics it pins; the +// Windows/ohos owned path is bounded by the same deadline through +// `bounded_stdin_write_owned` and covered by its own helper test. #[cfg(unix)] #[test] fn write_stdin_to_a_child_that_stops_consuming_errors_instead_of_wedging() { @@ -5158,7 +5160,7 @@ fn write_stdin_to_a_child_that_stops_consuming_errors_instead_of_wedging() { verdict }); let _ = receiver - .recv_timeout(std::time::Duration::from_secs(30)) + .recv_timeout(std::time::Duration::from_secs(60)) .expect("the bounded-write pin hung: the STDIN_WRITE_DEADLINE regression is live"); handle .join() @@ -5202,7 +5204,7 @@ async fn interact_payload_over_the_cap_refuses() { fn guidance_states_the_foreground_stdin_contract() { let description = super::guidance::description(); assert!( - description.contains("Foreground commands without `interactive` or `combined_output` run without a terminal stdin"), + description.contains("Foreground commands without `interactive`, `tty`, or `combined_output` run without a terminal stdin"), "guidance must state the bounded foreground stdin: {description}" ); assert!( @@ -5248,6 +5250,10 @@ async fn foreground_without_stdin_data_gives_stdin_readers_eof() { and exit promptly, not run to the timeout; took {elapsed:?}: {}", result.content ); + // The elapsed bound is a unix pin: on Windows a cold PowerShell startup + // can legitimately approach the 3000 ms watchdog under parallel load, + // while `success` above already fails if the watchdog killed the run. + #[cfg(unix)] assert!( elapsed < Duration::from_millis(2900), "EOF must be delivered right after spawn, not discovered at the \ @@ -5575,6 +5581,102 @@ fn interact_write_to_a_stalled_tty_command_is_bounded() { }); assert!(verdict.is_ok(), "{verdict:?}"); } +/// An explicit `close_stdin` on a terminal task whose last write already +/// STALLED must stay bounded and must not claim an end-of-file the +/// abandoned worker never delivered: the stall conjunct skips the ^D +/// delivery, the close releases the handle, and the tail refusal still +/// names the stall. A mutation that claims EOF unconditionally on the +/// terminal close (dropping the stall conjunct) reddens on the refusal +/// text; a close that wedges on the full input queue trips the outer +/// guard instead. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof() { + let (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 + // Same shape as the stalled-write pin above: a raw-mode + // tty whose command never reads backpressures the master + // write, and the marker only prints after `stty` succeeds. + .execute_with_options_env( + "stty raw -echo 2>/dev/null && printf RAW-READY; sleep 60", + None, + 60_000, + true, + None, + true, + None, + HashMap::new(), + ) + .map_err(|error| error.to_string())?; + let task_id = result + .task_id + .ok_or_else(|| "background execution should return task_id".to_string())?; + let mut raw_ready = false; + for _ in 0..40 { + std::thread::sleep(Duration::from_millis(150)); + let snapshot = manager.inspect_job(&task_id).expect("inspect job").snapshot; + if snapshot.stdout_tail.contains("RAW-READY") { + raw_ready = true; + break; + } + } + assert!( + raw_ready, + "raw mode must be established before the stalled write: {:?}", + manager.inspect_job(&task_id).expect("inspect job").snapshot + ); + let payload = "x".repeat(200 * 1024); + let error = manager + .write_stdin(&task_id, &payload, false) + .err() + .ok_or_else(|| "the stalled tty write must error at the deadline".to_string())? + .to_string(); + if !error.contains("did not complete within") { + return Err(error); + } + // The close itself must return promptly (bounded path, stall + // guard) instead of wedging on the full input queue. + let started = std::time::Instant::now(); + manager + .write_stdin(&task_id, "", true) + .map_err(|error| error.to_string())?; + if started.elapsed() > STDIN_WRITE_DEADLINE { + return Err(format!( + "the close on a stalled terminal task must not wait on the queue: {:?}", + started.elapsed() + )); + } + // And the tail refusal must still report the stall: no EOF was + // delivered, so claiming end-of-file here would be a lie. + let after_close = manager + .write_stdin(&task_id, "y", false) + .err() + .ok_or_else(|| "the post-close write must be refused".to_string())? + .to_string(); + if !after_close.contains("stalled") || after_close.contains("end-of-file") { + return Err(format!( + "a terminal close after a stall must not claim an EOF that never landed: {after_close}" + )); + } + manager.kill(&task_id).map_err(|error| error.to_string())?; + Ok(()) + }; + 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 stalled-terminal-close 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 @@ -5594,7 +5696,8 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { .expect("cat spawns"); let stdin = child.stdin.take().expect("stdin piped"); let (returned, result) = - super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), b"hello\n".to_vec()); + super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), b"hello\n".to_vec()) + .expect("the bounded-write worker thread must spawn"); let Ok(result) = result else { panic!("a consuming child must complete the write, not lose the worker: {result:?}"); }; @@ -5613,7 +5716,8 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { let payload = vec![b'x'; 256 * 1024]; // well past the 64 KiB pipe buffer let started = std::time::Instant::now(); let (returned, result) = - super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), payload); + super::bounded_stdin_write_owned(super::StdinWriter::Pipe(stdin), payload) + .expect("the bounded-write worker thread must spawn"); let elapsed = started.elapsed(); assert!( matches!(result, Err(std::sync::mpsc::RecvTimeoutError::Timeout)), @@ -5916,6 +6020,11 @@ fn bash_schema_stdin_max_length_matches_the_enforced_inline_cap() { } } +// The ^D close delivery is compiled only for the unix PTY lane (the same +// predicate as the `PtyFile` variant): Windows has no console EOF to +// deliver and ohos has no tty lane at all, so both the exit and the +// end-of-file refusal assertions below are meaningless there. +#[cfg(all(unix, not(target_env = "ohos")))] #[tokio::test] async fn interact_close_stdin_delivers_eof_to_a_terminal_command() { // Base delivered end-of-file to a terminal task through the ownership From f28a861c6600fb83b40e71c0042002c4294fc6cc Mon Sep 17 00:00:00 2001 From: asto18089 Date: Thu, 5 Nov 2026 18:34:02 +0800 Subject: [PATCH 6/9] fix(shell): consume pending SIGPIPE, honest stalled closes, one bounded writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 fresh-review fixes (comment 5781947688): - Host kill (BLOCKER): the SIGPIPE guard only deferred it. A SIGPIPE generated while blocked stays PENDING on the worker thread, and the guard's SIG_SETMASK restore delivers it before pthread_sigmask returns — with the process at SIG_DFL (run_with_args) the host died at signal 13 one instruction after the "handled" EPIPE, in exactly the documented stall-recovery shape (reproduced standalone: exit 141 for both the immediate-EPIPE and the blocked-mid-write recovery; a sigwait consume survives). The drop now checks this thread's pending set and consumes a pending SIGPIPE with sigwait before restoring the mask. Std's SIG_IGN makes test binaries blind to this (a generated SIGPIPE is discarded at generation and never becomes pending), so the new sigpipe_guard_survives_a_broken_pipe_restore pin re-execs the test binary as a helper that re-arms SIG_DFL first: block-and-restore kills the helper at signal 13, consume-before-restore exits 0. - Honest close results (MAJOR): a stalled close_stdin on a terminal task returned Ok while the schema promises a delivered end-of-file, and a second close silently destroyed the interact surface (also Ok). Both stalled close paths now return Err citing the stall (the delivery timed out / the close was not applied) and keep the handle attached; the Delivered/Failed arms keep their dispositions with the comments now describing them accurately. The stalled-terminal-close pin asserts the Err, and a new unit pin (bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered) classifies the close worker's outcomes against real pipes. - One bounded writer (MAJOR): the worker machinery existed three times (the eof-delivery worker, the owned fallback, the inline unix clone block), and the SIGPIPE fix had to touch all three. The eof delivery and the unix clone payload write now ride the single bounded_stdin_write_owned helper — a returned clone is dropped, never reattached, so the original slot keeps deciding what is restored — and the deadline-dependent refusal copy is shared in named functions (format captures need fns, not consts). The cfg_attr dead-code allows the duplication required are gone, and the helper doc now describes the real ownership split (clone vs handoff) without the ShellTask link that pointed at a type that does not exist. - Honesty and copy: the EOF refusal names the raw-mode exception; the generic refusal names the lost-writer case; close_stdin's schema discloses raw mode and the Windows handle-only release; the inline cap docs drop the "atomic" claim, scope the pipe-buffer rationale to Linux, and note the byte counting; the run-never-started context says the mentioned task id was never registered; the foreground close treats an evicted record as success instead of aborting a finished run; the PTY lane acquires the interact writer before the reader thread spawns so a dup/take_writer failure no longer orphans a spawned reader thread; the VEOF hardcode is acknowledged against portable-pty's live c_cc[VEOF] injection. - Test hygiene: the three hang-guard scaffolds collapse into run_stdin_scenario_with_hang_guard (worker panics surface as failed verdicts), the stalled-close guard bounds with >=, and the lowercase tool description's stdin sentence gets its own pin. Deferred unchanged, disclosed: the sync-lane inline payload write stays SIGPIPE-guarded but deadline-less (no production caller can reach it with a payload today; same family as the tracked run_bounded_child_observed/process.rs follow-up); Windows-lane CI coverage for the owned-path helper test. Targeted: tools::shell 167 passed / 0 failed / 1 ignored (pre-existing), tools::verifier 9 / 0; cargo fmt clean; fork-CI clippy flag set clean in an isolated target dir. The ohos/windows gated edits are verified by construction only — this host has no ohos/windows C cross toolchain for the dependency build scripts (rquickjs-sys/sqlite build scripts fail before codewhale-tui). Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 368 +++++++++++++++++----------- crates/tui/src/tools/shell/tests.rs | 312 ++++++++++++++++++----- 2 files changed, 476 insertions(+), 204 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index 6868a39cd2..d4df24c254 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -924,6 +924,20 @@ impl StdinWriter { /// simply exiting later — closes the read end: the resumed write would /// otherwise kill the whole host process with SIGPIPE instead of failing /// with EPIPE like every handled write error. +/// +/// Blocking alone is not sufficient: a SIGPIPE generated while blocked is +/// not discarded — it stays PENDING on the worker thread, and unblocking +/// delivers a pending unblocked signal before `pthread_sigmask` returns. +/// With SIGPIPE at SIG_DFL that would kill the process one instruction +/// after the "handled" EPIPE, exactly where the guard ends. The drop +/// therefore checks this thread's pending set and consumes a pending +/// SIGPIPE with `sigwait` BEFORE restoring the mask (`sigpending` only +/// sees thread-directed signals, so a pending SIGPIPE here is one this +/// guarded region generated). Test binaries run with std's SIG_IGN, under +/// which a generated SIGPIPE is discarded at generation and never becomes +/// pending — which is why the suite cannot catch a block-and-restore guard; +/// the consume path is pinned by re-arming SIG_DFL inside a helper process +/// (see `sigpipe_guard_survives_a_broken_pipe_restore`). struct BlockedSigpipeGuard { #[cfg(unix)] previous: libc::sigset_t, @@ -959,6 +973,23 @@ impl BlockedSigpipeGuard { impl Drop for BlockedSigpipeGuard { fn drop(&mut self) { unsafe { + // A write that ended in EPIPE raised SIGPIPE while it was + // blocked here. Consume it before the restore — otherwise the + // unblock delivers the pending signal at the process + // disposition (SIG_DFL in production) and the guard's own Drop + // kills the process it exists to protect (see the struct doc). + let mut pending: libc::sigset_t = std::mem::zeroed(); + libc::sigpending(&mut pending); + if libc::sigismember(&pending, libc::SIGPIPE) == 1 { + let mut blocked: libc::sigset_t = std::mem::zeroed(); + libc::sigemptyset(&mut blocked); + libc::sigaddset(&mut blocked, libc::SIGPIPE); + let mut delivered: i32 = 0; + // SAFETY: the signal is pending on this thread, so the + // wait returns immediately; the sets are local stack + // values. + libc::sigwait(&blocked, &mut delivered); + } libc::pthread_sigmask(libc::SIG_SETMASK, &self.previous, std::ptr::null_mut()); } } @@ -975,20 +1006,25 @@ impl Drop for BlockedSigpipeGuard { /// child was already gone and the master write failed), and `Stalled` means /// the tty input queue was still full at the deadline. #[cfg(all(unix, not(target_env = "ohos")))] +#[derive(Debug)] enum BoundedEofDelivery { Delivered, Failed, Stalled, } -/// Deliver `\n` + VEOF through the given master fd on the same kind of -/// bounded worker a payload write uses. The leading newline flushes any -/// pending partial line, because canonical mode only turns VEOF into a -/// read of length zero from an EMPTY line buffer — the same two bytes the -/// old ownership box injected on drop. In raw mode both bytes are ordinary -/// input, exactly like that box drop; the caller records a delivery only -/// when the write really completed, so the flag never claims an end-of-file -/// that did not land. +/// Deliver `\n` + VEOF through the given master fd on the same bounded +/// worker a payload write uses ([`bounded_stdin_write_owned`]). The leading +/// newline flushes any pending partial line, because canonical mode only +/// turns VEOF into a read of length zero from an EMPTY line buffer — the +/// same two bytes the old ownership box injected on drop. In raw mode both +/// bytes are ordinary input, exactly like that box drop; the caller records +/// a delivery only when the write really completed, so the flag never +/// claims an end-of-file that did not land. The EOF byte is written +/// literally (`0x04`) rather than read from `c_cc[VEOF]`: portable-pty's +/// unix writer injects the live byte at drop, so a terminal with VEOF +/// disabled received plain bytes from the box too — the parity holds in +/// every realistic termios state. #[cfg(all(unix, not(target_env = "ohos")))] fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { use std::os::fd::AsFd; @@ -999,33 +1035,32 @@ fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { // a queue-full condition, and the flag must stay down either way. Err(_) => return BoundedEofDelivery::Failed, }; - let mut writer = std::process::ChildStdin::from(dup_fd); - let (sender, receiver) = std::sync::mpsc::channel(); - let worker = std::thread::Builder::new() - .name("stdin-eof-delivery".to_string()) - .spawn(move || { - let _sigpipe = BlockedSigpipeGuard::new(); - let _ = sender.send(writer.write_all(b"\n\x04").and_then(|_| writer.flush())); - }); - let receiver = match worker { - Ok(_) => receiver, - Err(_) => return BoundedEofDelivery::Failed, - }; - match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok(Ok(())) => BoundedEofDelivery::Delivered, - Ok(Err(_)) => BoundedEofDelivery::Failed, - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => BoundedEofDelivery::Stalled, - Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => BoundedEofDelivery::Failed, - } -} - -/// The bounded stdin write for the platforms that cannot clone an fd out of -/// their handle (Windows and ohos, see [`ShellTask::write_stdin`]): the -/// worker thread takes the writer by value, finishes the payload, and hands -/// the writer back through the channel. `Ok` returns `(Some(writer), -/// Ok(io_result))`. `Err(RecvTimeoutError::Timeout)` returns `(None, ..)`: -/// the writer stays with the abandoned worker, the bytes still land if the -/// command resumes reading, and the caller marks the stall. + match bounded_stdin_write_owned( + StdinWriter::Pipe(std::process::ChildStdin::from(dup_fd)), + b"\n\x04".to_vec(), + ) { + // The clone is dropped on every arm: the dup this function was + // given stays with the task, and only the write's outcome decides + // what the close may claim. + Err(_) => BoundedEofDelivery::Failed, + Ok((_, Ok(Ok(())))) => BoundedEofDelivery::Delivered, + Ok((_, Ok(Err(_)))) => BoundedEofDelivery::Failed, + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Timeout))) => BoundedEofDelivery::Stalled, + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Disconnected))) => BoundedEofDelivery::Failed, + } +} + +/// The single bounded stdin write every platform uses: the worker thread +/// takes the writer by value, writes and flushes the payload, and hands the +/// writer back through the channel. Unix reaches it with a clone of the +/// task's handle — pipes clone the `ChildStdin` fd and the PTY lane clones +/// the dup'd master fd (see [`BackgroundShell::write_stdin`] and +/// [`bounded_stdin_eof_delivery`]); Windows and ohos cannot clone an fd out +/// of their handle, so the handoff moves the real writer. +/// `Ok((Some(writer), Ok(io_result)))` returns the writer to the caller. +/// `Ok((None, Err(RecvTimeoutError::Timeout)))` means the deadline hit +/// first: the writer stays with the abandoned worker, the bytes still land +/// if the command resumes reading, and the caller marks the stall. /// `Err(RecvTimeoutError::Disconnected)` means the worker died before /// completing (a panic in the write path): the writer is gone, and the /// caller must NOT mark a stall — a writer failure is not the command's @@ -1034,16 +1069,12 @@ fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { /// the failed closure (ownership was already handed over), so the caller /// must report the task as having no writable stdin. A plain /// `thread::spawn` panic here would unwind through the shell-manager lock -/// and poison it for the rest of the process. 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))] +/// and poison it for the rest of the process. type OwnedBoundedWriteOutcome = ( Option, Result, std::sync::mpsc::RecvTimeoutError>, ); -#[cfg_attr(all(unix, not(target_env = "ohos")), allow(dead_code))] fn bounded_stdin_write_owned( writer: StdinWriter, payload: Vec, @@ -1686,38 +1717,24 @@ impl BackgroundShell { )); } }; - let mut writer = std::process::ChildStdin::from(dup_fd); - let payload = input.as_bytes().to_vec(); - let (sender, receiver) = std::sync::mpsc::channel(); - let worker = std::thread::Builder::new() - .name("stdin-bounded-write".to_string()) - .spawn(move || { - let _sigpipe = BlockedSigpipeGuard::new(); - let _ = sender - .send(writer.write_all(&payload).and_then(|_| writer.flush())); - }); - let receiver = match worker { - Ok(_) => receiver, - Err(error) => { - // The spawn failed before any byte was written - // and the worker holds nothing: restore the - // handle and fail like a plain write error. A - // bare `thread::spawn` panic here would unwind - // through the shell-manager mutex and poison it - // for the rest of the process. - self.stdin = slot; - return Err(anyhow!(error) - .context("Failed to spawn the bounded stdin writer thread")); - } - }; - match receiver.recv_timeout(STDIN_WRITE_DEADLINE) { - Ok(Ok(())) => {} - Ok(Err(error)) => { - // The worker completed and handed the handle - // back: restore it so a write error does not - // silently strip the task's interact surface - // (the base behavior kept the handle attached - // after a write error too). + // The bounded write is the shared helper (the same one + // the owned platforms and the ^D close delivery use): + // the clone rides it by value, so only the ORIGINAL + // slot decides what is restored below — a returned + // clone is dropped, never reattached. + match bounded_stdin_write_owned( + StdinWriter::Pipe(std::process::ChildStdin::from(dup_fd)), + input.as_bytes().to_vec(), + ) { + // The clone came back with a completed write: drop + // it and keep the original attached. + Ok((_, Ok(Ok(())))) => {} + Ok((_, Ok(Err(error)))) => { + // The worker completed: restore the ORIGINAL + // handle so a write error does not silently + // strip the task's interact surface (the base + // behavior kept the handle attached after a + // write error too). self.stdin = slot; return Err(anyhow!(error).context("Failed to write to stdin")); } @@ -1726,20 +1743,29 @@ impl BackgroundShell { // path): that is a writer failure, not a stall, so // the task is not marked stalled — a healthy write // after this must not be refused by the flag. - Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Disconnected))) => { self.stdin = slot; - return Err(anyhow!( - "the stdin writer for task {} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one.", - self.id - )); + return Err(anyhow!(stdin_writer_failed_refusal(&self.id))); } - Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + Ok((_, Err(std::sync::mpsc::RecvTimeoutError::Timeout))) => { + // The clone stays with the abandoned worker; + // the original handle is restored, and the + // stall flag refuses any further write so two + // writers can never interleave on one handle. self.stdin_write_stalled = true; self.stdin = slot; - return Err(anyhow!( - "the write to task {} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one.", - self.id - )); + return Err(anyhow!(stdin_stalled_write_refusal(&self.id))); + } + Err(error) => { + // The spawn failed before any byte was written + // and the worker holds nothing: restore the + // handle and fail like a plain write error. A + // bare `thread::spawn` panic here would unwind + // through the shell-manager mutex and poison it + // for the rest of the process. + self.stdin = slot; + return Err(anyhow!(error) + .context("Failed to spawn the bounded stdin writer thread")); } } } else { @@ -1789,10 +1815,7 @@ impl BackgroundShell { // `slot` stays None: the abandoned worker owns // the handle, so the close below cannot claim // EOF and further writes hit the stall refusal. - return Err(anyhow!( - "the write to task {} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one.", - self.id - )); + return Err(anyhow!(stdin_stalled_write_refusal(&self.id))); } Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { // The worker died before completing (a panic in @@ -1801,10 +1824,7 @@ impl BackgroundShell { // healthy write after this is not refused, and // the writer is gone with the worker (nothing // to restore). - return Err(anyhow!( - "the stdin writer for task {} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one.", - self.id - )); + return Err(anyhow!(stdin_writer_failed_refusal(&self.id))); } } } @@ -1824,35 +1844,49 @@ impl BackgroundShell { // as a large payload would, and inline under the manager // lock they wedge every shell in the session. #[cfg(all(unix, not(target_env = "ohos")))] - if let Some(StdinWriter::PtyFile(file)) = slot.as_ref() - && !self.stdin_write_stalled - { - match bounded_stdin_eof_delivery(file) { - BoundedEofDelivery::Delivered => { - self.stdin = None; - self.stdin_closed_at_eof = true; - return Ok(()); - } - // The close consumed the handle as requested, but - // the bytes did not land (typically the child was - // already gone and the master write failed): keep - // the flag down so later refusals do not claim - // readers saw end-of-file. - BoundedEofDelivery::Failed => { - self.stdin = None; - return Ok(()); - } - // Queue still full at the deadline: restore the - // handle under the stall flag, mirroring the - // payload-timeout disposition, so later writes - // cite the stall instead of a delivery that has - // not happened. - BoundedEofDelivery::Stalled => { - self.stdin_write_stalled = true; - self.stdin = slot; - return Ok(()); + if let Some(StdinWriter::PtyFile(file)) = slot.as_ref() { + if !self.stdin_write_stalled { + match bounded_stdin_eof_delivery(file) { + BoundedEofDelivery::Delivered => { + self.stdin = None; + self.stdin_closed_at_eof = true; + return Ok(()); + } + // The close consumed the handle as requested, + // but the bytes did not land (typically the + // child was already gone and the master write + // failed): keep the flag down so later refusals + // do not claim readers saw end-of-file. Ok is + // honest here — the release the caller asked + // for happened, and no delivery is possible + // once the child is gone. + BoundedEofDelivery::Failed => { + self.stdin = None; + return Ok(()); + } + // Queue still full at the deadline: restore the + // handle under the stall flag and report the + // close as FAILED. The schema's close promises a + // delivered end-of-file, so a close that neither + // delivered nor released must not read as + // success — the same disposition the + // payload-timeout arm applies. + BoundedEofDelivery::Stalled => { + self.stdin_write_stalled = true; + self.stdin = slot; + return Err(anyhow!(stdin_stalled_close_refusal(&self.id))); + } } } + // A task whose last write stalled gets no delivery + // attempt at all: the queue that stalled the payload + // would stall the two bytes too, so the close is + // refused without waiting and without destroying the + // interact surface — the model is told the close was + // not applied instead of reading a success that + // delivered nothing. + self.stdin = slot; + return Err(anyhow!(stdin_stalled_close_not_applied_refusal(&self.id))); } // Windows cannot deliver end-of-file to a console child at // all (portable-pty's ConPTY writer has no EOF-on-drop — @@ -1895,12 +1929,12 @@ impl BackgroundShell { if self.stdin_closed_at_eof { return Err(anyhow!( - "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file; start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file (a program that put its terminal into raw mode consumes the EOF byte as ordinary input instead); start input-driven work with background=true and feed it with action=\"interact\"", self.id )); } Err(anyhow!( - "stdin is not available for task {}: no writable stdin handle is attached to it (a finished command has released its stdin, a persistent command never exposes one, and a terminal command exposes one only while it runs and only until its stdin is closed); start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: no writable stdin handle is attached to it (a finished command has released its stdin, a persistent command never exposes one, a terminal command exposes one only while it runs and only until its stdin is closed, and a bounded stdin writer that failed to spawn or panicked takes its handle with it); start input-driven work with background=true and feed it with action=\"interact\"", self.id )) } @@ -3331,8 +3365,13 @@ impl ShellManager { return Err(err).context("Failed to clone PTY reader"); } }; - let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); - + // The interact writer is acquired BEFORE the reader thread + // spawns, so a writer-acquisition failure aborts with only + // the child to clean up (kill + wait) instead of orphaning + // an already-spawned reader thread. Nothing observes the + // order: `pair.master` is alive across the whole block, so + // the slave sees no end-of-file in either arrangement. + // // Unix: the interact writer is a dup of the master fd, and // `take_writer` is NOT called — its box's Drop injects a // newline plus `VEOF` into the master (that is how "dropping @@ -3389,6 +3428,7 @@ impl ShellManager { return Err(err).context("Failed to take PTY writer"); } })); + let stdout_thread = Some(spawn_reader_thread(reader, Arc::clone(&stdout_buffer))); (ShellChild::Pty(child), stdin_writer, stdout_thread, None) } @@ -3546,7 +3586,7 @@ impl ShellManager { // killed and the caller learns the run never started. let _ = bg_shell.kill(); return Err(err.context( - "the run never started: delivering the inline stdin payload failed, so the command was killed and no task was created", + "the run never started: delivering the inline stdin payload failed, so the command was killed and NO task was created (the task id the error above mentions was never registered — do not try to cancel or kill it)", )); } @@ -3651,13 +3691,15 @@ impl ShellManager { /// PTY writer is invisible to the child (the reader holds the master) /// but destroys the interact surface a session member can still use /// while a terminal command runs — the foreground terminal lane's - /// contract. + /// contract. A missing record is success, not an error: only eviction + /// removes a record, and eviction drops the stdin handle with it, so + /// the pipe cannot still be open — and the close runs between spawn + /// and the first caller-visible step, where an error would abort a run + /// whose result is already determined. fn close_stdin_pipe(&mut self, task_id: &str) -> Result<()> { - let shell = self - .processes - .get_mut(task_id) - .ok_or_else(|| anyhow!("Task {task_id} not found"))?; - shell.close_stdin_pipe(); + if let Some(shell) = self.processes.get_mut(task_id) { + shell.close_stdin_pipe(); + } Ok(()) } @@ -5136,10 +5178,10 @@ async fn execute_foreground_via_background( // them. The task-id extraction above is the one step between spawn and // close, and `spawn_background_sandboxed` always returns an id on `Ok`, // so the close reaches the shell layer without a fallible step in - // between (the manager-level `close_stdin_pipe` wrapper can still fail - // when the record was already evicted — and eviction drops the stdin - // handle with it, so the invariant that matters holds: no later - // failure can leave the pipe open). + // between (the manager-level `close_stdin_pipe` wrapper treats an + // already-evicted record as success — eviction drops the stdin + // handle with the record, so the invariant that matters holds: no + // later failure can leave the pipe open). // // Foreground has no operator at the keyboard, so whatever the command // will ever read from stdin must be bounded now. With stdin data the @@ -5243,15 +5285,17 @@ async fn execute_foreground_via_background( const BASH_MAX_TIMEOUT_MS: u64 = i32::MAX as u64; -/// Upper bound for one inline `stdin` payload. Every accepted payload is -/// delivered through the bounded stdin writer (see -/// [`STDIN_WRITE_DEADLINE`]), so a command that stops reading surfaces as a -/// deadline stall rather than a wedged executor; the cap additionally keeps -/// oversized payloads from ever reaching the writer, and a 64 KiB payload -/// fits a Linux pipe buffer in one atomic write so the common -/// payload-then-close foreground shape delivers without stalling. Anything -/// larger is refused with a pointer at background `interact` streaming -/// instead of hanging the session. +/// Upper bound for one inline `stdin` payload (counted in UTF-8 bytes). +/// Every payload the tool surface accepts is delivered through the bounded +/// stdin writer (see [`STDIN_WRITE_DEADLINE`]), so a command that stops +/// reading surfaces as a deadline stall rather than a wedged executor; the +/// cap additionally keeps oversized payloads from ever reaching the +/// writer, and on Linux's default 64 KiB pipe buffer a cap-sized payload +/// completes into an empty pipe without a reader, so the common +/// payload-then-close foreground shape delivers without stalling there. +/// Platforms with smaller default pipe buffers lean on the deadline +/// instead. Anything larger is refused with a pointer at background +/// `interact` streaming instead of hanging the session. const MAX_INLINE_STDIN_BYTES: usize = 64 * 1024; /// Wall-clock bound on one stdin write. At the bound an `interact` write is @@ -5266,6 +5310,34 @@ const MAX_INLINE_STDIN_BYTES: usize = 64 * 1024; /// EPIPE instead of taking the process down. const STDIN_WRITE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); +/// One shared copy per stdin refusal shape so the unix fd-clone path and +/// the owned-writer path (and the close paths) cannot drift apart — the +/// pins assert substrings of these. They are functions rather than consts +/// so the deadline interpolation stays a real format capture. +fn stdin_stalled_write_refusal(task_id: &str) -> String { + format!( + "the write to task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one." + ) +} + +fn stdin_writer_failed_refusal(task_id: &str) -> String { + format!( + "the stdin writer for task {task_id} failed: the background write ended without completing or reporting (the writer task panicked); the payload's delivery is unknown. Kill the task and start a new one." + ) +} + +fn stdin_stalled_close_refusal(task_id: &str) -> String { + format!( + "the stdin close for task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the terminal's input queue stayed full because the command stopped consuming input, so the end-of-file bytes were not delivered; the close was not applied and the interact surface stays attached. Further stdin writes to this task are refused; kill it and start a new one." + ) +} + +fn stdin_stalled_close_not_applied_refusal(task_id: &str) -> String { + format!( + "the stdin close for task {task_id} was not applied: its last stdin write stalled because the command stopped consuming input, so the end-of-file was never delivered and no close can deliver it now; the interact surface stays attached and further stdin writes are refused; kill it and start a new one." + ) +} + /// Initial cadence for foreground-via-background completion polling. Fast /// commands dominate real agent traffic; detection latency on `true`-class /// commands drops from ~100ms to ~10ms, while long commands reach the @@ -5600,7 +5672,7 @@ impl ToolSpec for BashTool { "stdin": { "type": "string", "maxLength": 65536, - "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB: a larger write to a command that is not reading would block the executor; for bigger payloads run the command in the background and feed it through `action=interact` in chunks." + "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB, counted as UTF-8 bytes (maxLength mirrors the same number): a 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", @@ -5648,7 +5720,7 @@ impl ToolSpec for BashTool { }, "close_stdin": { "type": "boolean", - "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte" + "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" }, "all": { "type": "boolean", @@ -5811,10 +5883,12 @@ impl ToolSpec for BashTool { // 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. + // before any watchdog is reached. On Linux the cap equals + // the default pipe buffer, so a cap-sized write into an + // empty pipe completes without a reader there; smaller + // default buffers lean on the bounded writer's deadline. + // Bigger payloads belong on the background lane, fed + // through `action=interact` in chunks. if payload.len() > MAX_INLINE_STDIN_BYTES { return Ok(ToolResult::error(format!( "The inline stdin payload is {} bytes; the shell tool accepts at most {MAX_INLINE_STDIN_BYTES} inline — run the command with background=true and feed it through action=interact in chunks.", diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 4e08662002..39d862efad 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -5068,6 +5068,115 @@ async fn inline_stdin_payload_over_the_cap_refuses() { output.content ); } +/// Runs a stdin scenario on a worker thread behind a 60 s outer guard. The +/// scenario's own deadline machinery is what should finish it; a mechanism +/// regression that never returns trips this guard instead (the join can +/// extend past the guard while the scenario's child still holds the blocked +/// write open — the child's own watchdog releases it, so the guard +/// terminates, loudly late). A worker panic surfaces as a failed verdict, +/// never as a hung test. +#[cfg(unix)] +fn run_stdin_scenario_with_hang_guard( + name: &'static str, + body: impl FnOnce() -> Result<(), String> + Send + 'static, +) -> Result<(), String> { + let (sender, receiver) = std::sync::mpsc::channel::>(); + let handle = std::thread::spawn(move || { + let verdict = body(); + let _ = sender.send(verdict.clone()); + verdict + }); + match receiver.recv_timeout(Duration::from_secs(60)) { + Ok(verdict) => { + // Reaps the worker; the verdict from the channel is the + // scenario outcome, so the join's own copy adds nothing. + let _ = handle.join().expect("the scenario worker panicked"); + verdict + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + let _ = handle.join(); + Err(format!("{name} hung past its 60 s guard")) + } + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + // The worker died before its verdict reached the channel. + let _ = handle.join().expect_err("the scenario worker panicked"); + Err(format!("{name} panicked before reporting a verdict")) + } + } +} + +#[cfg(unix)] +const SIGPIPE_GUARD_HELPER_ENV: &str = "CODEWHALE_SIGPIPE_GUARD_HELPER"; + +/// Helper half of `sigpipe_guard_survives_a_broken_pipe_restore`: runs in a +/// process that re-armed SIGPIPE at SIG_DFL (what `run_with_args` does in +/// production), takes the guard, writes into a readerless pipe, and only +/// reaches the implicit test exit if the guard's mask restore did not +/// deliver the pending signal. +#[cfg(unix)] +#[test] +fn sigpipe_guard_helper_process() { + if std::env::var(SIGPIPE_GUARD_HELPER_ENV).ok().as_deref() != Some("1") { + return; + } + // The suite binary runs with std's SIG_IGN, under which a broken-pipe + // SIGPIPE is discarded at generation and never becomes pending — the + // bug cannot reproduce there, so the helper re-arms the production + // disposition first. + unsafe { + libc::signal(libc::SIGPIPE, libc::SIG_DFL); + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(libc::pipe(fds.as_mut_ptr()), 0, "pipe"); + libc::close(fds[0]); + { + let guard = super::BlockedSigpipeGuard::new(); + let buf = [b'x'; 4096]; + let written = libc::write(fds[1], buf.as_ptr().cast(), buf.len()); + assert_eq!(written, -1, "the readerless write must fail"); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPIPE), + "the readerless write must fail with EPIPE" + ); + // Dropping the guard restores the mask. A block-and-restore + // guard delivers the pending SIGPIPE here and this process + // dies at signal 13 before reaching the end of the test; the + // consume-before-restore fix reaches it and exits 0. + drop(guard); + } + libc::close(fds[1]); + } +} + +/// The SIGPIPE guard must consume the signal it blocked before restoring +/// the mask: a bounded write that ends in EPIPE leaves SIGPIPE pending on +/// the worker thread, and a plain block-and-restore delivers it at the +/// process disposition (SIG_DFL in production, see `run_with_args`) — +/// killing the whole host one instruction after the "handled" EPIPE. The +/// suite cannot see this (test binaries run SIG_IGN, so no SIGPIPE ever +/// becomes pending), so the driver re-execs this binary as a helper that +/// re-arms SIG_DFL first: with the consume the helper exits 0; without it +/// the helper dies at signal 13 and the non-success status fails here. +#[cfg(unix)] +#[test] +fn sigpipe_guard_survives_a_broken_pipe_restore() { + let test_binary = std::env::current_exe().expect("current test binary"); + let mut helper = Command::new(&test_binary) + .arg("--exact") + .arg("tools::shell::tests::sigpipe_guard_helper_process") + .env(SIGPIPE_GUARD_HELPER_ENV, "1") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sigpipe guard helper"); + let status = helper.wait().expect("wait sigpipe guard helper"); + assert!( + status.success(), + "the guard's mask restore must not deliver the pending SIGPIPE (a broken guard kills the helper at signal 13): {status}" + ); +} + /// 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 @@ -5081,9 +5190,9 @@ async fn inline_stdin_payload_over_the_cap_refuses() { #[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 verdict = run_stdin_scenario_with_hang_guard( + "the bounded-write pin", + || -> Result<(), String> { let tmp = tempdir().map_err(|error| error.to_string())?; let mut manager = ShellManager::new(tmp.path().to_path_buf()); let result = manager @@ -5154,18 +5263,9 @@ fn write_stdin_to_a_child_that_stops_consuming_errors_instead_of_wedging() { } 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(60)) - .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"); + }, + ); + verdict.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 @@ -5222,6 +5322,21 @@ fn guidance_states_the_foreground_stdin_contract() { "guidance must point at background interact for input-driven work: {description}" ); } +/// The lowercase tool's own description carries the same foreground stdin +/// contract: its legacy translation accepts only command/timeout/sandbox +/// fields, so that sentence is the whole stdin story on that surface. +#[test] +fn lowercase_bash_description_states_the_foreground_stdin_contract() { + let description = LowercaseBashTool.description(); + assert!( + description.contains("This foreground surface runs without a terminal stdin"), + "the lowercase description must state the bounded foreground stdin: {description}" + ); + assert!( + description.contains("a bare `$SHELL` receive end-of-file"), + "the bare-shell probe must be named as exiting on EOF: {description}" + ); +} /// 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 @@ -5486,9 +5601,9 @@ async fn foreground_terminal_command_keeps_its_interact_surface() { #[test] #[cfg(all(unix, not(target_env = "ohos")))] fn interact_write_to_a_stalled_tty_command_is_bounded() { - let (sender, receiver) = std::sync::mpsc::channel::>(); - let handle = std::thread::spawn(move || { - let run = || -> Result<(), String> { + let verdict = run_stdin_scenario_with_hang_guard( + "the bounded tty-write pin", + || -> Result<(), String> { let tmp = tempdir().map_err(|error| error.to_string())?; let mut manager = ShellManager::new(tmp.path().to_path_buf()); let result = manager @@ -5568,33 +5683,24 @@ fn interact_write_to_a_stalled_tty_command_is_bounded() { } manager.kill(&task_id).map_err(|error| error.to_string())?; Ok(()) - }; - let verdict = run(); - let _ = sender.send(verdict.clone()); - verdict - }); - let verdict = receiver - .recv_timeout(Duration::from_secs(60)) - .unwrap_or_else(|_| { - let _ = handle.join(); - Err("the bounded tty-write test hung past its 60 s guard".to_string()) - }); + }, + ); assert!(verdict.is_ok(), "{verdict:?}"); } /// An explicit `close_stdin` on a terminal task whose last write already -/// STALLED must stay bounded and must not claim an end-of-file the -/// abandoned worker never delivered: the stall conjunct skips the ^D -/// delivery, the close releases the handle, and the tail refusal still -/// names the stall. A mutation that claims EOF unconditionally on the -/// terminal close (dropping the stall conjunct) reddens on the refusal -/// text; a close that wedges on the full input queue trips the outer +/// STALLED must stay bounded, must not claim an end-of-file the abandoned +/// worker never delivered, and must not read as success either: the stall +/// skip refuses the close outright ("was not applied") and keeps the +/// interact surface attached, and the tail refusal still names the stall. +/// A mutation that reports the stalled close as Ok reddens on the +/// expect_err; a close that wedges on the full input queue trips the outer /// guard instead. #[cfg(all(unix, not(target_env = "ohos")))] #[test] fn close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof() { - let (sender, receiver) = std::sync::mpsc::channel::>(); - let handle = std::thread::spawn(move || { - let run = || -> Result<(), String> { + let verdict = run_stdin_scenario_with_hang_guard( + "the stalled-terminal-close pin", + || -> Result<(), String> { let tmp = tempdir().map_err(|error| error.to_string())?; let mut manager = ShellManager::new(tmp.path().to_path_buf()); let result = manager @@ -5638,43 +5744,44 @@ fn close_stdin_on_a_stalled_terminal_task_is_bounded_and_claims_no_eof() { if !error.contains("did not complete within") { return Err(error); } - // The close itself must return promptly (bounded path, stall - // guard) instead of wedging on the full input queue. + // The close itself must return promptly (no delivery attempt + // on a stalled queue) and must NOT report success: with the + // end-of-file undelivered, an Ok close would tell the model + // stdin is closed when it is not. let started = std::time::Instant::now(); - manager + let close_error = manager .write_stdin(&task_id, "", true) - .map_err(|error| error.to_string())?; - if started.elapsed() > STDIN_WRITE_DEADLINE { + .expect_err("a close on a stalled terminal task must not claim success") + .to_string(); + if started.elapsed() >= STDIN_WRITE_DEADLINE { return Err(format!( "the close on a stalled terminal task must not wait on the queue: {:?}", started.elapsed() )); } - // And the tail refusal must still report the stall: no EOF was - // delivered, so claiming end-of-file here would be a lie. + if !close_error.contains("was not applied") { + return Err(format!( + "the stalled-close refusal must say the close was not applied: {close_error}" + )); + } + // And any further stdin traffic must still be refused without + // claiming an EOF: no delivery happened, so "end-of-file" would + // be a lie (the refusal here comes from the write-stall gate — + // the refused close left the handle attached). let after_close = manager .write_stdin(&task_id, "y", false) .err() .ok_or_else(|| "the post-close write must be refused".to_string())? .to_string(); - if !after_close.contains("stalled") || after_close.contains("end-of-file") { + if after_close.contains("end-of-file") { return Err(format!( "a terminal close after a stall must not claim an EOF that never landed: {after_close}" )); } manager.kill(&task_id).map_err(|error| error.to_string())?; Ok(()) - }; - 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 stalled-terminal-close 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): @@ -5735,6 +5842,97 @@ fn bounded_stdin_write_owned_returns_the_writer_and_times_out_clean() { let _ = child.wait(); } +/// `bounded_stdin_eof_delivery` — the worker the explicit terminal close +/// rides — must classify its outcome honestly: a write that lands reports +/// `Delivered`, a write into a FULL pipe (nothing consuming) reports +/// `Stalled` only at the deadline, and a write to a readerless pipe reports +/// `Failed`. A Stalled that came back as Delivered would let `write_stdin` +/// claim an end-of-file that never landed. The call is deadline-bounded by +/// construction, so a broken deadline hangs this test into the nextest +/// slow-timeout instead of passing. +// Unix PTY-lane cfg, matching the function it pins. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered() { + use std::os::fd::FromRawFd; + + // Delivered: /dev/null accepts the two bytes unconditionally. + let null = std::fs::OpenOptions::new() + .write(true) + .open("/dev/null") + .expect("open /dev/null"); + assert!( + matches!( + super::bounded_stdin_eof_delivery(&null), + super::BoundedEofDelivery::Delivered + ), + "a write that lands must report Delivered" + ); + + // Failed: a readerless pipe errors the write without delivering (the + // suite runs with SIG_IGN, so the EPIPE surfaces as an error, not a + // signal — the process-death discipline is pinned by the sigpipe guard + // helper test). + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + unsafe { libc::close(fds[0]) }; + let orphan = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + assert!( + matches!( + super::bounded_stdin_eof_delivery(&orphan), + super::BoundedEofDelivery::Failed + ), + "a readerless pipe must report Failed" + ); + drop(orphan); + + // Stalled: fill a pipe to capacity without a consumer (O_NONBLOCK so + // the test thread never blocks), restore blocking mode for the worker, + // and the two-byte delivery cannot land until the deadline. + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + let read_fd = fds[0]; + let full = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + unsafe { libc::fcntl(fds[1], libc::F_SETFL, libc::O_NONBLOCK) }; + let chunk = [b'x'; 4096]; + loop { + let written = unsafe { libc::write(fds[1], chunk.as_ptr().cast(), chunk.len()) }; + if written <= 0 { + break; // EAGAIN: the pipe is full + } + } + unsafe { libc::fcntl(fds[1], libc::F_SETFL, 0) }; + let started = std::time::Instant::now(); + let outcome = super::bounded_stdin_eof_delivery(&full); + drop(full); + assert!( + matches!(outcome, super::BoundedEofDelivery::Stalled), + "a full pipe must stall the delivery: {outcome:?}" + ); + assert!( + started.elapsed() >= super::STDIN_WRITE_DEADLINE, + "the stall must honor the deadline: {:?}", + started.elapsed() + ); + // Unblock the abandoned worker: a reader consumes the queued bytes, the + // worker's write lands, its send fails on the dropped receiver, and the + // thread exits instead of parking for the process lifetime. The drain + // is bounded so the reader cannot hang once the pipe ends are closed. + let drainer = std::thread::spawn(move || { + let mut buf = [0u8; 4096]; + let mut total = 0usize; + while total < 512 * 1024 { + let n = unsafe { libc::read(read_fd, buf.as_mut_ptr().cast(), buf.len()) }; + if n <= 0 { + break; + } + total += n as usize; + } + unsafe { libc::close(read_fd) }; + }); + drainer.join().expect("drain the stalled pipe"); +} + /// An unconfirmed stop keeps its write scope across every coordination /// query — until a later poll reaps the real child and clears the /// uncertainty. The flag models the un-reaped state; the confirmation cycle From 8a390dee31b1a369ccf0333b875c10268abc5fdd Mon Sep 17 00:00:00 2001 From: asto18089 Date: Thu, 5 Nov 2026 20:40:37 +0800 Subject: [PATCH 7/9] fix(shell): gate unix-only close refusals, pin the fresh-stall close arm Round-3 wave from the fresh 10-lane re-review at f28a861c6: - BLOCKER: stdin_stalled_close_refusal and stdin_stalled_close_not_applied_refusal had no cfg, but their only references sit in the all(unix, not(target_env = "ohos")) PTY-close arm - on Windows and ohos both fns are dead code, fatal under -D warnings (ci.yml RUSTFLAGS, ohos.yml, fork-ci clippy). Gate them with the arm's predicate, mirroring the PtyFile variant. - The bounded_stdin_eof_delivery doc claimed a terminal with VEOF disabled "received plain bytes from the box too"; portable-pty 0.9.0 writes nothing at all in that state (unix.rs guards if eot != 0), so the comment now states the real divergence and the close_stdin schema discloses the disabled-VEOF state alongside raw mode. - Accuracy: the guard doc's "sigpending only sees thread-directed signals" parenthetical (sigpending also reports process-directed signals); write_stdin's lead comment (the owned-path Timeout, Disconnected, and spawn-failure arms deliberately leave the slot empty); the stdin_write_stalled field doc (a stalled close sets it). - New pin close_stdin_that_stalls_on_fresh_delivery_refuses_and_ stays_attached: the fresh-stall close arm (Stalled -> Err) had no guard; the record-level pin drives write_stdin("", true) against a full pipe wrapped in PtyFile and asserts the Err, the deadline wait, the restored handle + stall flag, the no-EOF flag, and the fast follow-up refusal. Mutation-verified red (Stalled -> Ok revert). - Harden sigpipe_guard_survives_a_broken_pipe_restore: the helper now also asserts SIGPIPE is unblocked after the drop (a no-op restore went undetected; mutation-verified red) and writes a ran-canary the driver asserts, so a renamed helper cannot leave the pin silently green via libtest's exit-0-on-unmatched-filter. Gates: tools::shell 174/0/1, tools::verifier 9/0, fmt clean, fork-CI clippy flags clean (isolated target dir). Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 49 ++++--- crates/tui/src/tools/shell/tests.rs | 196 +++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 19 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index d4df24c254..f9912472cf 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -931,9 +931,12 @@ impl StdinWriter { /// With SIGPIPE at SIG_DFL that would kill the process one instruction /// after the "handled" EPIPE, exactly where the guard ends. The drop /// therefore checks this thread's pending set and consumes a pending -/// SIGPIPE with `sigwait` BEFORE restoring the mask (`sigpending` only -/// sees thread-directed signals, so a pending SIGPIPE here is one this -/// guarded region generated). Test binaries run with std's SIG_IGN, under +/// SIGPIPE with `sigwait` BEFORE restoring the mask. A write-failure +/// SIGPIPE is thread-directed, so a pending SIGPIPE inside a guarded +/// region is one this region's write generated; `sigpending` also reports +/// process-directed signals, but nothing in this process raises SIGPIPE +/// that way, and consuming such a signal under SIG_DFL would only spare +/// the process. Test binaries run with std's SIG_IGN, under /// which a generated SIGPIPE is discarded at generation and never becomes /// pending — which is why the suite cannot catch a block-and-restore guard; /// the consume path is pinned by re-arming SIG_DFL inside a helper process @@ -1022,9 +1025,12 @@ enum BoundedEofDelivery { /// a delivery only when the write really completed, so the flag never /// claims an end-of-file that did not land. The EOF byte is written /// literally (`0x04`) rather than read from `c_cc[VEOF]`: portable-pty's -/// unix writer injects the live byte at drop, so a terminal with VEOF -/// disabled received plain bytes from the box too — the parity holds in -/// every realistic termios state. +/// unix writer reads the live byte at drop and writes nothing at all when +/// VEOF is disabled (`_POSIX_VDISABLE`), so the two behaviors differ in +/// that state — this close sends the two bytes as ordinary input, exactly +/// like raw mode, and no end-of-file ever arrives. The `close_stdin` +/// schema discloses that; the old box delivered nothing in the same +/// state, so the close still reports honestly what it did. #[cfg(all(unix, not(target_env = "ohos")))] fn bounded_stdin_eof_delivery(file: &std::fs::File) -> BoundedEofDelivery { use std::os::fd::AsFd; @@ -1297,10 +1303,11 @@ pub struct BackgroundShell { /// `interact` can state the true reason stdin is unavailable. stdin_closed_at_eof: bool, /// Set when a stdin write outlived [`STDIN_WRITE_DEADLINE`] because the - /// command stopped consuming its pipe. The abandoned bytes keep flowing - /// from the background writer if the child resumes, so further writes - /// are refused (two writers could interleave) instead of queued behind - /// an unknown pipe state. + /// command stopped consuming its pipe, or when a terminal close's own + /// ^D delivery stalled the same way (same deadline, same refusal + /// family). The abandoned bytes keep flowing from the background writer + /// if the child resumes, so further writes are refused (two writers + /// could interleave) instead of queued behind an unknown pipe state. stdin_write_stalled: bool, child: Option, /// A stop was delivered but the direct child's exit was never observed: @@ -1651,9 +1658,12 @@ impl BackgroundShell { fn write_stdin(&mut self, input: &str, close: bool) -> Result<()> { // Taken owned so the Windows/ohos fallback can move the writer into - // its bounded worker and hand it back; every exit below restores the + // its bounded worker and hand it back. Every unix exit restores the // slot first, keeping the stall/eof refusal ordering identical with - // the pinned unix behavior. + // the pinned unix behavior; on the ownership-fallback cfgs the + // Timeout, Disconnected, and spawn-failure arms deliberately leave + // the slot empty — the writer is gone with the worker, and the stall + // flag or the returned refusal reports that. // `mut` is real only on the ownership-fallback cfgs (they re-take and // reassign the slot); the unix fd-clone path only moves it, so the // same predicate that strips the fallback also silences `unused_mut`. @@ -5311,9 +5321,9 @@ const MAX_INLINE_STDIN_BYTES: usize = 64 * 1024; const STDIN_WRITE_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); /// One shared copy per stdin refusal shape so the unix fd-clone path and -/// the owned-writer path (and the close paths) cannot drift apart — the -/// pins assert substrings of these. They are functions rather than consts -/// so the deadline interpolation stays a real format capture. +/// the owned-writer path cannot drift apart — the pins assert substrings +/// of these. They are functions rather than consts so the deadline +/// interpolation stays a real format capture. fn stdin_stalled_write_refusal(task_id: &str) -> String { format!( "the write to task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the command stopped consuming its stdin, so its input buffer filled and the rest of the payload was abandoned to a background writer (it still lands if the command resumes reading). Further stdin writes to this task are refused; kill it and start a new one." @@ -5326,12 +5336,19 @@ fn stdin_writer_failed_refusal(task_id: &str) -> String { ) } +/// Serves only the unix PTY close arm (the Windows/ohos close arm has its +/// own inline copy and never takes the unix delivery path). Same +/// predicate as the `PtyFile` variant, so `deny(warnings)`' fatal +/// `dead_code` cannot fire on platforms where the arm is compiled out. +#[cfg(all(unix, not(target_env = "ohos")))] fn stdin_stalled_close_refusal(task_id: &str) -> String { format!( "the stdin close for task {task_id} did not complete within {STDIN_WRITE_DEADLINE:?}: the terminal's input queue stayed full because the command stopped consuming input, so the end-of-file bytes were not delivered; the close was not applied and the interact surface stays attached. Further stdin writes to this task are refused; kill it and start a new one." ) } +/// See [`stdin_stalled_close_refusal`]: unix-PTY-only, same cfg predicate. +#[cfg(all(unix, not(target_env = "ohos")))] fn stdin_stalled_close_not_applied_refusal(task_id: &str) -> String { format!( "the stdin close for task {task_id} was not applied: its last stdin write stalled because the command stopped consuming input, so the end-of-file was never delivered and no close can deliver it now; the interact surface stays attached and further stdin writes are refused; kill it and start a new one." @@ -5720,7 +5737,7 @@ impl ToolSpec for BashTool { }, "close_stdin": { "type": "boolean", - "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" + "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode, or disabled VEOF, receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" }, "all": { "type": "boolean", diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 39d862efad..c8f4a5ba4c 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -5108,11 +5108,19 @@ fn run_stdin_scenario_with_hang_guard( #[cfg(unix)] const SIGPIPE_GUARD_HELPER_ENV: &str = "CODEWHALE_SIGPIPE_GUARD_HELPER"; +/// Lets the driver verify the helper actually RAN: libtest exits 0 on an +/// unmatched `--exact` filter, so a future rename of the helper test would +/// otherwise leave the pin silently green while guarding nothing. +#[cfg(unix)] +const SIGPIPE_GUARD_HELPER_CANARY_ENV: &str = "CODEWHALE_SIGPIPE_GUARD_HELPER_CANARY"; + /// Helper half of `sigpipe_guard_survives_a_broken_pipe_restore`: runs in a /// process that re-armed SIGPIPE at SIG_DFL (what `run_with_args` does in -/// production), takes the guard, writes into a readerless pipe, and only -/// reaches the implicit test exit if the guard's mask restore did not -/// deliver the pending signal. +/// production), takes the guard, writes into a readerless pipe, checks that +/// the drop restored the prior mask, and only then writes the ran-canary — +/// so the driver fails on a helper that died at signal 13 (consume removed), +/// panicked on a left-blocked mask (restore removed), or never ran (rename +/// drift). #[cfg(unix)] #[test] fn sigpipe_guard_helper_process() { @@ -5144,8 +5152,29 @@ fn sigpipe_guard_helper_process() { // consume-before-restore fix reaches it and exits 0. drop(guard); } + // The restore half is pinned too: after the drop, SIGPIPE must no + // longer be blocked on this thread. A no-op restore would leave + // every later SIGPIPE pending on this thread forever — invisible + // to the exit-status assertion above, so query the mask directly. + let mut now_blocked: libc::sigset_t = std::mem::zeroed(); + assert_eq!( + libc::pthread_sigmask(libc::SIG_BLOCK, std::ptr::null(), &mut now_blocked), + 0, + "query the signal mask" + ); + assert_eq!( + libc::sigismember(&now_blocked, libc::SIGPIPE), + 0, + "the guard drop must restore the prior mask (SIGPIPE must not stay blocked)" + ); libc::close(fds[1]); } + // Ran-canary for the driver: written only after every assertion above + // passed, so an unmatched filter (helper never ran) is distinguishable + // from a green run. + if let Some(canary) = std::env::var_os(SIGPIPE_GUARD_HELPER_CANARY_ENV) { + std::fs::write(&canary, b"ran").expect("write the ran-canary"); + } } /// The SIGPIPE guard must consume the signal it blocked before restoring @@ -5161,10 +5190,16 @@ fn sigpipe_guard_helper_process() { #[test] fn sigpipe_guard_survives_a_broken_pipe_restore() { let test_binary = std::env::current_exe().expect("current test binary"); + let canary = std::env::temp_dir().join(format!( + "codewhale-sigpipe-guard-canary-{}", + std::process::id() + )); + let _ = std::fs::remove_file(&canary); let mut helper = Command::new(&test_binary) .arg("--exact") .arg("tools::shell::tests::sigpipe_guard_helper_process") .env(SIGPIPE_GUARD_HELPER_ENV, "1") + .env(SIGPIPE_GUARD_HELPER_CANARY_ENV, &canary) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -5175,6 +5210,11 @@ fn sigpipe_guard_survives_a_broken_pipe_restore() { status.success(), "the guard's mask restore must not deliver the pending SIGPIPE (a broken guard kills the helper at signal 13): {status}" ); + assert!( + canary.exists(), + "the helper must actually run — libtest exits 0 on an unmatched --exact filter, so a green status without the canary means this pin guarded nothing (helper renamed?)" + ); + let _ = std::fs::remove_file(&canary); } /// The bounded stdin write: a command that never consumes its pipe fills @@ -5933,6 +5973,156 @@ fn bounded_stdin_eof_delivery_reports_stalled_failed_and_delivered() { drainer.join().expect("drain the stalled pipe"); } +/// The FRESH-stall close arm — a terminal close whose own ^D delivery +/// stalls with the stall flag still down (no prior write set it) — must +/// wait the bounded deadline, return the close refusal that cites it, +/// restore the handle under the stall flag, and refuse the follow-up write +/// without another deadline wait. A mutant reporting this arm as Ok would +/// read as the schema's delivered end-of-file while the bytes never +/// landed; the not-applied pin next door only reaches the SKIP arm (its +/// scenario stalls a write first), so this record-level pin is the only +/// guard on the arm itself. Drives `write_stdin` on a record whose stdin +/// is a `PtyFile` over a full pipe — exactly what a real PTY task hands +/// the arm. +#[cfg(all(unix, not(target_env = "ohos")))] +#[test] +fn close_stdin_that_stalls_on_fresh_delivery_refuses_and_stays_attached() { + use std::os::fd::FromRawFd; + + // Fill a pipe to capacity without a consumer (O_NONBLOCK so the test + // thread never blocks), restore blocking mode for the worker: the + // two-byte ^D delivery cannot land until the deadline. + let mut fds = [0 as libc::c_int; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0, "pipe"); + let read_fd = fds[0]; + let full = unsafe { std::fs::File::from_raw_fd(fds[1]) }; + unsafe { libc::fcntl(fds[1], libc::F_SETFL, libc::O_NONBLOCK) }; + let chunk = [b'x'; 4096]; + loop { + let written = unsafe { libc::write(fds[1], chunk.as_ptr().cast(), chunk.len()) }; + if written <= 0 { + break; // EAGAIN: the pipe is full + } + } + unsafe { libc::fcntl(fds[1], libc::F_SETFL, 0) }; + + let now = std::time::Instant::now(); + let mut shell = BackgroundShell { + id: "fresh-stall-close".to_string(), + owner_session_id: "stdin-test-session".to_string(), + command: "cat".to_string(), + working_dir: std::path::PathBuf::from("."), + status: ShellStatus::Running, + exit_code: None, + started_at: now, + finished_at: None, + last_output_at: now, + last_observed_output_len: 0, + sandbox_type: SandboxType::None, + ownership: ShellOwnership::Managed, + linked_task_id: None, + owner_agent: None, + origin_tool_call_id: None, + origin_turn_id: None, + stdout_buffer: super::new_shared_raw_output(), + stderr_buffer: None, + bounded_output: None, + heavy_permit: None, + stdout_cursor: 0, + stderr_cursor: 0, + completion_reported: false, + stdin: Some(StdinWriter::PtyFile(full)), + stdin_closed_at_eof: false, + stdin_write_stalled: false, + child: None, + stop_unconfirmed: false, + stdout_thread: None, + stderr_thread: None, + work_lifecycle: None, + lifecycle_seq: 0, + last_lifecycle_status: None, + last_lifecycle_bytes: 0, + }; + + let started = std::time::Instant::now(); + let close_err = shell + .write_stdin("", true) + .expect_err("a close whose own delivery stalls must be refused, not reported Ok"); + assert!( + started.elapsed() >= super::STDIN_WRITE_DEADLINE, + "the fresh-stall close must attempt the bounded delivery first: {:?}", + started.elapsed() + ); + let message = format!("{close_err:#}"); + assert!( + message.starts_with("the stdin close for task fresh-stall-close"), + "the refusal must be the close refusal: {message}" + ); + assert!( + message.contains("did not complete within"), + "the fresh-stall refusal must cite the deadline: {message}" + ); + assert!( + !message.contains("no close can deliver it now"), + "a fresh stall must not take the not-applied skip arm: {message}" + ); + assert!( + !message.contains("stdin writes to task"), + "the close refusal must not be the write-stall gate: {message}" + ); + assert!( + shell.stdin.is_some(), + "the refused close must keep the interact surface attached" + ); + assert!( + shell.stdin_write_stalled, + "the fresh-stall close must mark the stall for later refusals" + ); + assert!( + !shell.stdin_closed_at_eof, + "a stalled delivery must not claim an end-of-file" + ); + + // The follow-up write is refused by the stall gate without another + // bounded wait. + let started = std::time::Instant::now(); + let write_err = shell + .write_stdin("x", false) + .expect_err("a write after a stalled close must be refused"); + assert!( + started.elapsed() < std::time::Duration::from_millis(500), + "the stall flag must refuse the follow-up write without another deadline wait: {:?}", + started.elapsed() + ); + let message = format!("{write_err:#}"); + assert!( + message.contains("stdin writes to task fresh-stall-close are refused"), + "the follow-up refusal must be the stall gate: {message}" + ); + + // Unblock the abandoned worker exactly like the classification pin: a + // bounded reader consumes the queued bytes, the worker's write lands, + // its send fails on the dropped receiver, and the thread exits. The + // record must drop first — it still holds the ORIGINAL pipe write end, + // and until every write end closes the drainer's read never sees EOF, + // so joining before the drop would hang the harness (the classification + // pin avoids this by dropping `full` before its call). + drop(shell); + let drainer = std::thread::spawn(move || { + let mut buf = [0u8; 4096]; + let mut total = 0usize; + while total < 512 * 1024 { + let n = unsafe { libc::read(read_fd, buf.as_mut_ptr().cast(), buf.len()) }; + if n <= 0 { + break; + } + total += n as usize; + } + unsafe { libc::close(read_fd) }; + }); + drainer.join().expect("drain the stalled pipe"); +} + /// An unconfirmed stop keeps its write scope across every coordination /// query — until a later poll reaps the real child and clears the /// uncertainty. The flag models the un-reaped state; the confirmation cycle From 5e32effde389e57b4ef4ad9197ecd51f3912a542 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Fri, 6 Nov 2026 03:15:03 +0800 Subject: [PATCH 8/9] fix(shell): collapse gate close if, honest stdin copy, spawn-abort pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review wave at 8a390dee: - collapse the verifier gate-close if-let pair into a let chain: the upstream all-targets clippy variant (-D warnings, collapsible_if not allowed) reported the verifier.rs block this branch adds as a third collapsible_if; the two remaining reports are the pre-existing base sites (core/engine/turn_loop.rs:5611, tools/subagent/mod.rs:8058) - the stdin schema and the run-cap comment claimed an oversized write "would block the executor"; since the bounded writer, every such write surfaces as the 5-second deadline-stall refusal - the sync-lane payload write now documents that this manager-API lane has neither the inline cap nor the bounded writer's deadline - the closed-stdin refusal and the close_stdin schema name disabled or remapped VEOF next to raw mode as states that consume the EOF bytes as ordinary input - manager close_stdin_pipe returns () — the close cannot fail, so the false Result contract and the ? at the foreground call site are gone - new pin: spawn_aborts_with_run_never_started_context_when_the_payload_ stalls drives the uncapped manager lane against a non-reading child and asserts the run-never-started / never-registered abort copy and the bounded-deadline elapsed bound Gates: tools::shell 175/0/1, tools::verifier 9/0, fmt clean, fork-CI clippy flags clean. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 42 +++++++++++++---------- crates/tui/src/tools/shell/tests.rs | 53 +++++++++++++++++++++++++++++ crates/tui/src/tools/verifier.rs | 13 ++++--- 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index f9912472cf..a8d85cef07 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -1939,7 +1939,7 @@ impl BackgroundShell { if self.stdin_closed_at_eof { return Err(anyhow!( - "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file (a program that put its terminal into raw mode consumes the EOF byte as ordinary input instead); start input-driven work with background=true and feed it with action=\"interact\"", + "stdin is not available for task {}: its stdin was closed, so stdin readers have seen end-of-file (a program that put its terminal into raw mode — or disabled or remapped VEOF — consumes those bytes as ordinary input instead); start input-driven work with background=true and feed it with action=\"interact\"", self.id )); } @@ -3056,7 +3056,13 @@ impl ShellManager { // Same SIGPIPE discipline as the bounded stdin workers (see // [`BlockedSigpipeGuard`]): the child dying mid-write must // surface as a handled write error, not kill a process that - // runs with SIGPIPE at its default disposition. + // runs with SIGPIPE at its default disposition. Unlike the + // tool surface this manager-API lane has neither + // [`MAX_INLINE_STDIN_BYTES`] nor the bounded writer's + // deadline — every production payload goes through the + // background lane's bounded writer (see + // [`STDIN_WRITE_DEADLINE`]), so a direct caller passing a + // payload here must keep it within what the child reads. let _sigpipe = BlockedSigpipeGuard::new(); stdin .write_all(input.as_bytes()) @@ -3701,16 +3707,15 @@ impl ShellManager { /// PTY writer is invisible to the child (the reader holds the master) /// but destroys the interact surface a session member can still use /// while a terminal command runs — the foreground terminal lane's - /// contract. A missing record is success, not an error: only eviction + /// contract. A missing record is a no-op, not an error: only eviction /// removes a record, and eviction drops the stdin handle with it, so /// the pipe cannot still be open — and the close runs between spawn /// and the first caller-visible step, where an error would abort a run /// whose result is already determined. - fn close_stdin_pipe(&mut self, task_id: &str) -> Result<()> { + fn close_stdin_pipe(&mut self, task_id: &str) { if let Some(shell) = self.processes.get_mut(task_id) { shell.close_stdin_pipe(); } - Ok(()) } pub fn write_stdin_for_session( @@ -5217,7 +5222,7 @@ async fn execute_foreground_via_background( .shell_manager .lock() .map_err(|_| anyhow!("shell manager lock poisoned"))?; - manager.close_stdin_pipe(&task_id)?; + manager.close_stdin_pipe(&task_id); } if let Some(permit) = heavy_permit { let mut manager = context @@ -5689,7 +5694,7 @@ impl ToolSpec for BashTool { "stdin": { "type": "string", "maxLength": 65536, - "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB, counted as UTF-8 bytes (maxLength mirrors the same number): a larger write to a command that is not reading would block the executor; for bigger payloads run the command in the background and feed it through `action=interact` in chunks." + "description": "Stdin data to send (action=run: before waiting; action=interact: to the background task). Also accepted as `input` or `data` — send only one. Inline payloads are capped at 64 KiB, counted as UTF-8 bytes (maxLength mirrors the same number): a write to a command that is not reading ends as a bounded deadline-stall refusal instead of blocking the session; for bigger payloads run the command in the background and feed it through `action=interact` in chunks." }, "input": { "type": "string", @@ -5737,7 +5742,7 @@ impl ToolSpec for BashTool { }, "close_stdin": { "type": "boolean", - "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode, or disabled VEOF, receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" + "description": "Close stdin after sending (action=interact); on a terminal task the close delivers end-of-file, sent as a literal newline followed by the EOF byte — a program that put its terminal into raw mode, or disabled or remapped VEOF, receives those bytes as ordinary input instead, and on Windows the close only releases the terminal handle (a console child has no end-of-file to deliver)" }, "all": { "type": "boolean", @@ -5896,16 +5901,17 @@ impl ToolSpec for BashTool { .as_str() .ok_or_else(|| type_mismatch(name, value, "a string"))? .to_string(); - // The spawn delivers the payload with a blocking write while - // the caller holds the shell-manager lock: past the pipe - // buffer a command that never reads would wedge the - // executor thread and every other shell in the session - // before any watchdog is reached. On Linux the cap equals - // the default pipe buffer, so a cap-sized write into an - // empty pipe completes without a reader there; smaller - // default buffers lean on the bounded writer's deadline. - // Bigger payloads belong on the background lane, fed - // through `action=interact` in chunks. + // The tool surface's inline cap keeps an oversized payload + // from ever reaching the writer; the spawn delivers the + // payload through the bounded stdin writer, so a command + // that never reads surfaces as the writer's 5-second + // deadline stall refusal rather than a wedged executor. On + // Linux the cap equals the default pipe buffer, so a + // cap-sized write into an empty pipe completes without a + // reader there; smaller default buffers lean on the same + // deadline on the bounded lanes. Bigger payloads belong on + // the background lane, fed through `action=interact` in + // chunks. if payload.len() > MAX_INLINE_STDIN_BYTES { return Ok(ToolResult::error(format!( "The inline stdin payload is {} bytes; the shell tool accepts at most {MAX_INLINE_STDIN_BYTES} inline — run the command with background=true and feed it through action=interact in chunks.", diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index c8f4a5ba4c..7a9f9e26e4 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -5416,6 +5416,59 @@ async fn foreground_without_stdin_data_gives_stdin_readers_eof() { result.content ); } +/// When the inline payload cannot be delivered at spawn, the run must abort +/// with the run-never-started context: no task id exists yet, so the child +/// is killed and the error must say the mentioned task id was never +/// registered — the caller must not try to cancel or kill it. The +/// manager-level lane has no inline cap, so an over-pipe-buffer payload +/// against a non-reading child reaches the bounded writer's deadline here; +/// the tool surface cannot produce this stall on Linux (its 64 KiB cap +/// equals the default pipe buffer), which is why this pin drives the +/// manager API directly. +#[test] +#[cfg(unix)] +fn spawn_aborts_with_run_never_started_context_when_the_payload_stalls() { + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + // 2 MiB: far past any default pipe buffer, so the bounded writer's + // deadline is what ends the write. + let payload = "x".repeat(2 * 1024 * 1024); + + let started = Instant::now(); + let err = manager + .execute_with_options_env( + &sleep_command(60), + None, + 60_000, + true, + Some(payload.as_str()), + false, + None, + HashMap::new(), + ) + .expect_err("the spawn must abort when the inline payload stalls"); + let elapsed = started.elapsed(); + + let text = format!("{err:#}"); + assert!( + text.contains("the run never started"), + "error must carry the run-never-started context; got: {text}" + ); + assert!( + text.contains("never registered"), + "error must tell the caller the task id was never registered; got: {text}" + ); + assert!( + !text.contains("Kill the task"), + "the task-targeted recovery copy must not appear on the spawn-abort \ + path; got: {text}" + ); + assert!( + elapsed >= STDIN_WRITE_DEADLINE, + "the abort must come from the bounded writer's deadline, not an \ + instant failure; took {elapsed:?}: {text}" + ); +} #[cfg(unix)] const SYNC_STDIN_EOF_HELPER_ENV: &str = "CODEWHALE_SYNC_STDIN_EOF_HELPER"; #[cfg(unix)] diff --git a/crates/tui/src/tools/verifier.rs b/crates/tui/src/tools/verifier.rs index 4abd1f3c40..ed686eab87 100644 --- a/crates/tui/src/tools/verifier.rs +++ b/crates/tui/src/tools/verifier.rs @@ -588,14 +588,13 @@ fn start_background_gates( .as_ref() .ok() .and_then(|result| result.task_id.as_deref()) + && let Err(error) = manager.write_stdin(task_id, "", true) { - if let Err(error) = manager.write_stdin(task_id, "", true) { - // Unreachable today (the job was just inserted under the - // held lock and the close writes nothing), but a silent - // `.ok()` here would quietly revert gates to idling on - // the open pipe until the background gate timeout. - tracing::warn!(%task_id, %error, "closing the gate job's stdin failed; the gate may idle until its timeout"); - } + // Unreachable today (the job was just inserted under the + // held lock and the close writes nothing), but a silent + // `.ok()` here would quietly revert gates to idling on + // the open pipe until the background gate timeout. + tracing::warn!(%task_id, %error, "closing the gate job's stdin failed; the gate may idle until its timeout"); } spawned }; From 7deeca87d1a0e884761ed8d322ac5531ec696059 Mon Sep 17 00:00:00 2001 From: asto18089 Date: Fri, 6 Nov 2026 19:44:29 +0800 Subject: [PATCH 9/9] fix(shell): correct two stale stdin comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PTY spawn comment said "a stalled close releases it" about the dup'd master fd: no stall path releases it — both stall arms restore the handle into the record (the fresh-stall arm under the stall flag, the not-applied arm verbatim), and it is the Failed delivery arm that releases. The comment now says that. The BlockedSigpipeGuard SAFETY note claimed a failed `pthread_sigmask` leaves "the failure mode ... the pre-guard one". The write-phase half of that claim holds (the write runs at the process default disposition), but the drop's restore would then install the zeroed `previous` mask — an empty `SIG_SETMASK`, leaving this thread's signals unblocked — which is not the pre-guard mode. The comment now describes the real consequence instead of a false reassurance. Comment-only; no behavior change. Gates at this tree: tools::shell 175/0/1, tools::verifier 9/0, `cargo fmt --all -- --check` clean, fork-CI clippy flags clean. Signed-off-by: asto18089 --- crates/tui/src/tools/shell.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index a8d85cef07..e4ec8f876b 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -952,9 +952,11 @@ impl BlockedSigpipeGuard { { // SAFETY: `zeroed` is valid for `sigset_t` (an array of // integer words) and both pointers below are local stack - // values; a failed `pthread_sigmask` simply leaves the - // process default disposition and the failure mode is the - // pre-guard one. + // values. A failed `pthread_sigmask` is not expected with + // these local sets; if it did fail, the write would run with + // the process default disposition (the pre-guard behavior) + // and the drop's restore would install the zeroed `previous` + // mask — an empty set, leaving this thread's signals unblocked. unsafe { let mut blocked: libc::sigset_t = std::mem::zeroed(); let mut previous: libc::sigset_t = std::mem::zeroed(); @@ -3398,7 +3400,8 @@ impl ShellManager { // a master-side description open — no EOF for the slave // while the reader's clone is open too. A plain drop of the // dup (the spawn-path close never touches this lane, and a - // stalled close releases it) still shows the child nothing; + // close whose delivery failed releases it) still shows the + // child nothing; // an explicit `close_stdin` instead delivers the ^D bytes // itself through the bounded worker and claims end-of-file // only when that write really landed; the final master