diff --git a/README.md b/README.md index 18acb7ee..eb66e4c6 100644 --- a/README.md +++ b/README.md @@ -305,11 +305,12 @@ leave you answering one call blind. One question, one answer, then the process ends. `sandbx tui` takes the same flags and draws that turn on a screen instead of streaming it, where ctrl-c or -escape ends it mid-flight — `--session` keeping the rounds it finished but not -the one it stopped in, and a running `bash` command killed, where any other tool -call finishes first. When the turn ends the screen goes and the transcript is -printed in its place, so the answer stays in your scrollback. It needs a -terminal on stdout, so it refuses a +escape ends it mid-flight — `--session` keeping the rounds it finished, and the +one it stopped in if it was running tools, with the output of the call it +stopped on withheld, and a running `bash` command killed and a call that only +reads left running, where a `write` or `edit` finishes first. When the turn +ends the screen goes and the transcript is printed in its place, so the answer +stays in your scrollback. It needs a terminal on stdout, so it refuses a piped run. `--approve call` asks on the screen, in rows under the transcript, and one key answers: `y`, `n` or `a`, escape or ctrl-c refusing the call and ending the turn ([#225](https://github.com/danczw/sandbx/issues/225)): @@ -335,7 +336,7 @@ resuming one another user can write is refused — [SECURITY.md](SECURITY.md). |---|---| | `0` | the model finished its answer | | `2` | a bound cut the turn short, named on stderr — `--max-tokens` or `--max-rounds`. Out of rounds, one more request answers the turn with no tool, so stdout usually holds a summary a blank line below what arrived before the cap; `--no-wrap-up` skips it, leaving nothing at all if the model opened with a tool call ([#204](https://github.com/danczw/sandbx/pull/204)). Under `tui` it also means you interrupted the turn | -| `3` | the terminal went away, so the turn ended there and nobody was watching it. Under `--approve call` that is the terminal it asks on: that call and the ones behind it were refused and no further request was sent. What the turn did before is on stdout and in `--session`. Under `tui` it is the screen or the keyboard hanging up mid-turn, which ends the turn where it stood: `--session` keeps the rounds it finished and loses the one it stopped in, so a hangup before the first round finishes stores nothing, the prompt included ([#271](https://github.com/danczw/sandbx/issues/271)). In the wrap-up round after `--max-rounds` it loses only the wrap-up. The audit trail of what it touched is released to stderr rather than dropped unwritten, so `2>` a file to keep it: where stderr is the terminal that died, the write fails and it goes with it | +| `3` | the terminal went away, so the turn ended there and nobody was watching it. Under `--approve call` that is the terminal it asks on: that call and the ones behind it were refused and no further request was sent. What the turn did before is on stdout and in `--session`. Under `tui` it is the screen or the keyboard hanging up mid-turn, which ends the turn where it stood: `--session` keeps the rounds it finished and the one it stopped in if it was running tools, and loses it if the model was still writing, so a hangup before the first round finishes streaming stores nothing, the prompt included ([#271](https://github.com/danczw/sandbx/issues/271)). In the wrap-up round after `--max-rounds` it loses only the wrap-up. The audit trail of what it touched is released to stderr rather than dropped unwritten, so `2>` a file to keep it: where stderr is the terminal that died, the write fails and it goes with it | | `64` | sandbx would not take the command line, so no turn was started and nothing was spawned. The complaint is on stderr, as clap wrote it. `EX_USAGE`, and the same code under every subcommand | | anything else | it failed before or during the turn, with the reason on stderr | diff --git a/SECURITY.md b/SECURITY.md index 4995879f..11700226 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -329,15 +329,21 @@ architecture the filter gates on. Whatever a tool read is in it: a token in a config, a `.env` under `--allow-read`, a key a `bash` printed. No encryption, no redaction, no expiry, no deletion. A turn stopped on `tui`'s screen stores the prompt and every - round it finished, under a stop naming an interrupt or a hung-up terminal, and - loses the round it stopped in whole: whatever it streamed, including the part - of the answer you read, and every tool call it asked for. The calls it had - already run did run, and one still running is killed if it is a `bash` and - finishes if it is not, but the transcript records the effect of neither. A - stop before the first round finishes stores - nothing, the prompt included, and leaves no trace in the session that a call - ran ([#271](https://github.com/danczw/sandbx/issues/271)). Where a round was - stored, resuming says so on stderr; the model is told nothing of it. What the + round it finished, under a stop naming an interrupt or a hung-up terminal. A + stop while the model is still writing loses that round whole: whatever it + streamed, including the part of the answer you read, and every tool call it + asked for. A stop while its tools run keeps the round with every call + answered: a call that had settled when the stop was raised keeps its output; + one still running then, or settling as it was raised, is killed if it is a + `bash`, left running if it only reads and finishes if it writes, and is + stored with its output withheld, since you stopped before reading it and a + resume would send it to the provider; one behind it is stored as not run. + What a withheld call did still stands, and a killed or abandoned one is + stored as cancelled with anything it had done possibly remaining. A stop + before the first round finishes streaming stores nothing, the prompt + included ([#271](https://github.com/danczw/sandbx/issues/271)). Where a round + was stored, resuming says so on stderr, and the model sees those results and + nothing more of the stop. What the turn showed is printed back to the terminal once the screen is gone, and the call a stop stood on is named there as it ended, run or cancelled, but never with its output. Where the terminal is @@ -374,17 +380,23 @@ architecture the filter gates on. instead — a cap on files visited and bytes read for `grep` and `find`, the single file or directory for the rest ([`ToolLimits`](crates/sandbx-tools/src/tool_limits.rs)) — on a thread that - cannot be cancelled, so one read on a stalled filesystem hangs indefinitely. - Nothing else is capped: no CPU bound, no memory bound, no limit on processes - spawned. A fork bomb is unbounded while the call lasts; bounded is that it does - not outlive it. -- **Only a spawned command can be interrupted.** A stop on `tui` (ctrl-c, - escape, or a hung-up terminal) kills a running `bash` command with the kill a - timeout makes, and starts no call behind it. The six in-process tools cannot - be cancelled: one already running finishes, so a `write` under way lands, and - the stop waits for it — for as long as a stalled filesystem takes. Nothing a - killed command had already done is undone - ([#26](https://github.com/danczw/sandbx/issues/26)). + cannot be cancelled, so one read on a stalled filesystem hangs indefinitely, + holding that thread until the process exits; a stop stops waiting for it, + below. Nothing else is capped: no CPU bound, no memory bound, no limit on + processes spawned. A fork bomb is unbounded while the call lasts; bounded is + that it does not outlive it. +- **A `write` or `edit` cannot be interrupted, and a read is abandoned, not + stopped.** A stop on `tui` (ctrl-c, escape, or a hung-up terminal) kills a + running `bash` command with the kill a timeout makes, and stops waiting for a + running `read`, `ls`, `grep` or `find`: the call is left on its blocking + thread with its result unread, a `grep` also stopping before its next file + (not during the directory walk before it). It starts no call behind either. A + `write` or `edit` already running finishes, so a `write` under way lands, and + the stop waits for it. An abandoned read goes on until it finishes or the + process exits, so a stop is not a promise that nothing is still reading: its + access was recorded in the audit trail before it began, and a stalled one + holds a blocking thread until exit. Nothing a killed command had already done + is undone ([#26](https://github.com/danczw/sandbx/issues/26)). - **A bound on the harness's own loop is not a containment claim.** A turn is bounded — `TurnLimits::max_rounds` caps the requests one turn may make of the model, `TurnLimits::stream_timeout` how long one of them may spend streaming diff --git a/context/decision-approval-gate.md b/context/decision-approval-gate.md index d66c5995..c19a3945 100644 --- a/context/decision-approval-gate.md +++ b/context/decision-approval-gate.md @@ -16,7 +16,9 @@ that ask it and implement it. In `answer_calls`, between resolving the name and `spawn_blocking`. A blocking task cannot be cancelled (#26): dropping the `JoinHandle` leaves the -task running to completion, and only a `bash` has a kill to reach for besides. +task running to completion. A stop abandons that handle only for a call that +reads, which leaves nothing behind it; a `write` or `edit` is waited for, and +that is the case this position guards. So a gate consulted *concurrently* with the call — `select!` on a decision and a `JoinHandle` — would answer "denied" about a `write` that had already landed. Before the spawn is the only position where a refusal means anything. @@ -201,10 +203,12 @@ case without `--approve call`; with it, a hangup lands on a question and is the It also keeps less. The abort is a `TurnStop`, so the turn returns and its messages, usage and `withheld` are stored. A `tui` stop ends the turn through -`run_turn_until`, which keeps the rounds before it and drops the one it lands -in, so the round the operator was watching is stored nowhere, for a hangup as -for an interrupt (#271). A tool call already running is killed if it is a `bash` -and finishes if it is not (#26), on both surfaces alike. +`run_turn_until`, which keeps the rounds before it, and the one it lands in if +that was running tools, every call answered and the one under way with its +output withheld. A round the model was still writing is stored nowhere, for a +hangup as for an interrupt (#271). A tool call already running is killed if it +is a `bash`, left running if it only reads, and finishes if it writes (#26), on +both surfaces alike. ## Deny by default, and the honest claim @@ -364,8 +368,8 @@ descriptor no task feeds, and nothing else is in flight during `approve`, since - **A stop given earlier outranks an `a`.** Before the blanket is read, the gate asks whether the turn was interrupted or the terminal has gone, and refuses or aborts at once. An `a` was the operator's word about a tool, not a way past a - stop they gave since: a covered call would otherwise run, and an in-process - call cannot be cancelled (#26). + stop they gave since: a covered call would otherwise run, and a `write` or + `edit` cannot be cancelled (#26). - **Refusal outranks an answer already pressed.** An interrupt beats a queued `y`, and a hangup beats both. Each leaves the call refused, the direction a doubtful reading must lean. diff --git a/crates/sandbx-cli/src/agent_gate.rs b/crates/sandbx-cli/src/agent_gate.rs index 53881bd8..1c495a1c 100644 --- a/crates/sandbx-cli/src/agent_gate.rs +++ b/crates/sandbx-cli/src/agent_gate.rs @@ -194,7 +194,7 @@ fn report(call: Settled<'_>) -> String { ToolError::TimedOut { after, .. } => { format!("{head} — timed out after {after:?} and was killed") } - ToolError::Cancelled { .. } => format!("{head} — cancelled and killed"), + ToolError::Cancelled { .. } => format!("{head} — cancelled"), }, } } diff --git a/crates/sandbx-cli/src/agent_gate_tests.rs b/crates/sandbx-cli/src/agent_gate_tests.rs index 680b0051..53673b0f 100644 --- a/crates/sandbx-cli/src/agent_gate_tests.rs +++ b/crates/sandbx-cli/src/agent_gate_tests.rs @@ -359,17 +359,17 @@ fn a_policy_refusal_is_not_reported_as_a_call_that_ran() { ); } -/// The operator who pressed ctrl-C reads this line, so it must say the command was killed -/// and not that it ran or that a limit was hit. +/// The operator who pressed ctrl-C reads this line, so it must say the call was cancelled +/// and not that it ran or that a limit was hit. Not "killed": a `grep` is stopped, not killed. #[test] -fn a_cancelled_call_is_reported_as_killed() { +fn a_cancelled_call_is_reported_as_cancelled() { let error = ToolError::Cancelled { subject: "run `sleep 80`".to_string(), }; assert_eq!( reported_bash("sleep 80", Outcome::Errored(&error)), - "bash sleep 80 — cancelled and killed" + "bash sleep 80 — cancelled" ); } diff --git a/crates/sandbx-cli/src/cli.rs b/crates/sandbx-cli/src/cli.rs index c76cff57..d11dba0f 100644 --- a/crates/sandbx-cli/src/cli.rs +++ b/crates/sandbx-cli/src/cli.rs @@ -96,10 +96,11 @@ pub enum Command { /// screen goes, and what it showed is printed in its place. /// /// Interrupting costs two things, which sandbx prints again once the screen goes: - /// `--session` keeps the rounds the turn finished but not the one it stopped - /// in, and a tool already running is dealt with first: a `bash` command is - /// killed, any other tool call finishes, and the process waits for it before - /// it exits. A turn interrupted while it is being summarised after + /// `--session` keeps the rounds the turn finished, and the one it stopped in + /// if it was running tools, and a tool already running is dealt with first: + /// a `bash` command is killed, a call that only reads is left running, and a + /// `write` or `edit` finishes, the process waiting for it before it exits. + /// A turn interrupted while it is being summarised after /// `--max-rounds` loses only the summary. /// /// `--show-thinking` reads differently here: it draws the reasoning in the diff --git a/crates/sandbx-cli/src/main.rs b/crates/sandbx-cli/src/main.rs index 088a0e1b..066a0614 100644 --- a/crates/sandbx-cli/src/main.rs +++ b/crates/sandbx-cli/src/main.rs @@ -122,13 +122,19 @@ fn report(result: Result, failure: u8) -> ExitCode { fn block_on( future: impl Future>, ) -> Result { - tokio::runtime::Builder::new_current_thread() + let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() // Named at the one call site that can produce it rather than by a blanket // `From`, which would label any later io error as this one. - .map_err(sandbx_cli::AgentError::Runtime)? - .block_on(future) + .map_err(sandbx_cli::AgentError::Runtime)?; + let result = runtime.block_on(future); + + // Dropping a runtime waits for every blocking task, and a read-only tool call a stop + // abandoned may be parked in one for as long as a stalled filesystem takes (#26). Only + // those are left by now, every other call having been awaited. + runtime.shutdown_background(); + result } #[cfg(test)] diff --git a/crates/sandbx-cli/src/main_tests.rs b/crates/sandbx-cli/src/main_tests.rs index 7d02df1f..85991123 100644 --- a/crates/sandbx-cli/src/main_tests.rs +++ b/crates/sandbx-cli/src/main_tests.rs @@ -49,3 +49,22 @@ fn an_argv_sandbx_accepts_is_not_a_usage_error() { Cli::try_parse_from(["sandbx", "sandbox-run", "--", "true"]) .expect("a well-formed sandbox-run"); } + +/// A read abandoned on a stop is still parked on its blocking thread, and dropping the +/// runtime waits for every such thread. The process must not (#26). +#[test] +fn the_runtime_does_not_wait_for_a_blocking_task_left_running() { + let started = std::time::Instant::now(); + + let result = super::block_on(async { + tokio::task::spawn_blocking(|| std::thread::sleep(std::time::Duration::from_secs(3))); + Ok(0) + }); + + assert!(matches!(result, Ok(0))); + assert!( + started.elapsed() < std::time::Duration::from_secs(1), + "the runtime waited {:?} for a task nobody awaits", + started.elapsed() + ); +} diff --git a/crates/sandbx-cli/src/tui.rs b/crates/sandbx-cli/src/tui.rs index d2a982bd..25ad08d7 100644 --- a/crates/sandbx-cli/src/tui.rs +++ b/crates/sandbx-cli/src/tui.rs @@ -203,8 +203,9 @@ impl Tui { matches!(self.run.approve, Approve::Call).then(|| Modal::new(&pane, keys.answers())); // Inside the loop rather than raced against it, so the rounds before the stop come - // back to be stored. The stop kills a running `bash` and waits for an in-process - // tool, so the call it stood on settles, and is drawn, before the turn returns (#26). + // back to be stored. The stop kills a running `bash`, leaves a read-only call running + // and waits for a `write` or `edit`, so the call it stood on settles, and is drawn, + // before the turn returns (#26). let mut seen = None; let first = run_turn_until( &mut open, @@ -336,7 +337,7 @@ fn held(stderr: Device) -> Option { /// What the turn's futures came to, before any of it is accounted for. enum Driven { /// Stopped before the turn had finished, which keeps the rounds before the one it - /// stopped in. + /// stopped in, and that one too if it was running tools. Stopped(Stopped, TurnOutcome), /// Stopped in the wrap-up round of a turn that ran out after `usize` rounds, which is /// kept as the cap left it. @@ -404,7 +405,7 @@ fn stored(stopped: Stopped) -> StoredStop { /// The code a stop exits with, and its line. `wrapping` is whether it found the turn in /// its wrap-up round, which keeps every round before it, where any other stop loses the -/// round it lands in. +/// round it lands in if the model was still writing it. /// /// A hangup has its own line and not the interrupt's: the operator did not stop this turn, /// and telling them they did is the one reading of a hangup that is certainly wrong (#264). @@ -427,9 +428,11 @@ fn stop(stopped: Stopped, wrapping: bool) -> Ending { ) } else { format!( - "{how}. Under `--session` the rounds it finished are kept and the one it stopped \ - in is lost. A `bash` command already running is killed; any other tool call \ - already running finishes first, and what it did stays (#26)" + "{how}. Under `--session` the rounds it finished are kept, and the one it stopped \ + in is kept if it was running tools and is lost if the model was still writing. A \ + `bash` command already running is killed, and a call that only reads is left \ + running and not waited for; a `write` or `edit` already running finishes first, \ + and what it did stays (#26)" ) }; diff --git a/crates/sandbx-cli/src/tui_modal.rs b/crates/sandbx-cli/src/tui_modal.rs index 9f588abe..d2a46907 100644 --- a/crates/sandbx-cli/src/tui_modal.rs +++ b/crates/sandbx-cli/src/tui_modal.rs @@ -74,7 +74,7 @@ impl Operator for Modal { fn ask(&mut self, call: ToolCall<'_>) -> ApprovalDecision { // Before the `a` that covers a tool: that was the operator's word about the tool, // not a way past a stop they gave since, or a terminal that has gone. A covered - // call would otherwise run, and an in-process call cannot be cancelled (#26). + // call would otherwise run, and a `write` or `edit` cannot be cancelled (#26). match self.replies.stopped() { Some(Reply::Interrupted) => return deny(INTERRUPTED), Some(Reply::Closed) => return abort(CLOSED), diff --git a/crates/sandbx-cli/src/tui_tests.rs b/crates/sandbx-cli/src/tui_tests.rs index 66160ee5..5078c1d0 100644 --- a/crates/sandbx-cli/src/tui_tests.rs +++ b/crates/sandbx-cli/src/tui_tests.rs @@ -371,8 +371,13 @@ fn every_stop_keeps_the_rounds_it_finished() { let (_, stop) = turn.expect("the finished rounds are kept"); assert_eq!(stop, stored, "{stopped:?}"); assert_eq!(mid_turn, code, "{stopped:?}"); + // Both stops are named: one in the stream loses its round, one in a tool call keeps it. assert!( - said[0].contains("the one it stopped in is lost"), + said[0].contains("is lost if the model was still writing"), + "{said:?}" + ); + assert!( + said[0].contains("is kept if it was running tools"), "{said:?}" ); // What the operator is told about a call under way: the mechanism's whole claim. @@ -380,6 +385,14 @@ fn every_stop_keeps_the_rounds_it_finished() { said[0].contains("`bash` command already running is killed"), "{said:?}" ); + assert!( + said[0].contains("a call that only reads is left running and not waited for"), + "{said:?}" + ); + assert!( + said[0].contains("a `write` or `edit` already running finishes first"), + "{said:?}" + ); } } diff --git a/crates/sandbx-session-bridge/src/lib.rs b/crates/sandbx-session-bridge/src/lib.rs index 1f83ec08..9da452a3 100644 --- a/crates/sandbx-session-bridge/src/lib.rs +++ b/crates/sandbx-session-bridge/src/lib.rs @@ -97,17 +97,17 @@ fn resume_note(pending: bool, stop: Option) -> Option<&'static str> { "its last turn ended with no one left to approve a call; this prompt goes to \ the model beside a tool call it never answered" } - // The dropped round is named because its tool calls may have run (#26), and - // nothing the model is sent says so. + // The round is kept with every call answered (#26): a call running at the stop shows + // its output withheld, so the operator is told what that record leaves out. Some(TurnStop::Interrupted) => { - "its last turn was interrupted, and the round it stopped in is not stored, nor \ - what any tool call in it did; this prompt goes to the model beside a tool call it \ - never answered" + "its last turn was interrupted; the round it stopped in is stored, a call running \ + at the stop with its output withheld and none behind it run; this prompt goes to \ + the model beside those results" } Some(TurnStop::HungUp) => { - "its last turn stopped when its terminal went away, and the round it stopped in \ - is not stored, nor what any tool call in it did; this prompt goes to the model \ - beside a tool call it never answered" + "its last turn stopped when its terminal went away; the round it stopped in is \ + stored, a call running at the stop with its output withheld and none behind it \ + run; this prompt goes to the model beside those results" } // An answer beside an unanswered call is a hand edit, so it says no more than no // record does. diff --git a/crates/sandbx-session-bridge/src/session_bridge_tests.rs b/crates/sandbx-session-bridge/src/session_bridge_tests.rs index 70087c21..392d823b 100644 --- a/crates/sandbx-session-bridge/src/session_bridge_tests.rs +++ b/crates/sandbx-session-bridge/src/session_bridge_tests.rs @@ -207,9 +207,10 @@ fn an_unanswered_call_is_explained_by_the_stop_recorded() { assert!(!note(None).contains("rounds")); } -/// A hangup is not reported as a keypress, and both name the round they lost (#271). +/// A hangup is not reported as a keypress, and both say the round was kept and what the +/// call the stop landed on left out (#271, #26). #[test] -fn a_stopped_turn_names_its_stop_and_what_it_lost() { +fn a_stopped_turn_names_its_stop_and_what_it_kept() { let pressed = resume_note(true, Some(TurnStop::Interrupted)).expect("a note"); let gone = resume_note(true, Some(TurnStop::HungUp)).expect("a note"); @@ -217,7 +218,8 @@ fn a_stopped_turn_names_its_stop_and_what_it_lost() { assert!(gone.contains("terminal went away"), "{gone}"); assert!(!gone.contains("interrupted"), "{gone}"); for note in [pressed, gone] { - assert!(note.contains("is not stored"), "{note}"); + assert!(note.contains("output withheld"), "{note}"); + assert!(!note.contains("not stored"), "{note}"); assert!(!note.contains("rounds"), "{note}"); } } diff --git a/crates/sandbx-session-message/src/lib.rs b/crates/sandbx-session-message/src/lib.rs index 05e79904..b9094e63 100644 --- a/crates/sandbx-session-message/src/lib.rs +++ b/crates/sandbx-session-message/src/lib.rs @@ -93,7 +93,8 @@ pub enum TurnStop { Summarised, /// The gate ended the turn, having lost whatever it decides with. GateAborted, - /// The operator stopped the turn, and the round it was in is not stored. + /// The operator stopped the turn. A round it stopped mid-stream is not stored; one it + /// stopped mid-call is, every call answered. Interrupted, /// The terminal went away, which stopped the turn as [`Interrupted`](Self::Interrupted) /// does. diff --git a/crates/sandbx-tools/src/execution_context.rs b/crates/sandbx-tools/src/execution_context.rs index 048eb0d3..47ede407 100644 --- a/crates/sandbx-tools/src/execution_context.rs +++ b/crates/sandbx-tools/src/execution_context.rs @@ -100,8 +100,10 @@ impl ExecutionContext { /// Stop what this context spawns once `cancel` is raised, instead of a token nobody holds. /// /// Every clone shares the token, so one raised stops a command started through any of - /// them. Only a spawned command can be stopped: the in-process tools are bounded by - /// work and finish what they started. + /// them. A spawned command is killed, and `grep` stops between files; the other + /// in-process tools never read the token, and `write` and `edit` finish what they + /// started (#26). A caller may stop waiting for a read-only one instead, as + /// `sandbx-turn` does. #[must_use] pub fn with_cancel(mut self, cancel: Cancel) -> Self { self.cancel = cancel; diff --git a/crates/sandbx-tools/src/tool_error.rs b/crates/sandbx-tools/src/tool_error.rs index 354d4824..60636fd0 100644 --- a/crates/sandbx-tools/src/tool_error.rs +++ b/crates/sandbx-tools/src/tool_error.rs @@ -44,10 +44,11 @@ pub enum ToolError { after: Duration, }, - /// The operation was killed because the caller stopped waiting for it. + /// The operation was stopped because the caller stopped waiting for it: a command is + /// killed, a `grep` returns before its next file. /// /// Separate from [`TimedOut`](Self::TimedOut): no limit was hit, so there is nothing to - /// narrow or lengthen, and the operation may have changed state before it died. + /// narrow or lengthen, and a command may have changed state before it died. Cancelled { /// What was attempted. subject: String, @@ -66,7 +67,7 @@ impl fmt::Display for ToolError { write!(f, "{subject} timed out after {after:?} and was killed") } Self::Cancelled { subject } => { - write!(f, "{subject} was cancelled and killed before it finished") + write!(f, "{subject} was cancelled before it finished") } } } diff --git a/crates/sandbx-tools/src/tools_grep.rs b/crates/sandbx-tools/src/tools_grep.rs index ab7d0855..22a65d4c 100644 --- a/crates/sandbx-tools/src/tools_grep.rs +++ b/crates/sandbx-tools/src/tools_grep.rs @@ -39,6 +39,15 @@ pub(crate) fn execute(input: GrepInput, ctx: &ExecutionContext) -> Result= ctx.limits().max_bytes_scanned { diff --git a/crates/sandbx-tools/tests/search.rs b/crates/sandbx-tools/tests/search.rs index 1fac77c0..14127bc5 100644 --- a/crates/sandbx-tools/tests/search.rs +++ b/crates/sandbx-tools/tests/search.rs @@ -9,7 +9,7 @@ #![allow(clippy::disallowed_methods)] use sandbx_core::{SandboxPolicy, VettedPath}; -use sandbx_tools::{BuiltinTool, ExecutionContext, ToolError}; +use sandbx_tools::{BuiltinTool, Cancel, ExecutionContext, ToolError}; use serde_json::json; use std::fs; use std::os::unix::fs as unix_fs; @@ -83,6 +83,50 @@ fn grep_reports_no_matches_rather_than_failing() { assert_eq!(out.content(), "no matches"); } +/// A stop is an error and not a short listing: a search that gave up must not read as one +/// that found nothing. +#[test] +fn grep_stops_between_files_once_cancelled() { + let root = tempfile::tempdir().unwrap(); + fs::write(root.path().join("a.txt"), "needle\n").unwrap(); + fs::write(root.path().join("b.txt"), "needle\n").unwrap(); + + let cancel = Cancel::new(); + let ctx = context(SandboxPolicy::default().allow_read(vetted(root.path()))) + .with_cancel(cancel.clone()); + cancel.raise(); + + let result = BuiltinTool::Grep.execute( + json!({ "path": root.path().to_str().unwrap(), "pattern": "needle" }), + &ctx, + ); + + assert!( + matches!(result, Err(ToolError::Cancelled { .. })), + "expected a cancellation, got: {result:?}" + ); + // The model and the operator both read this, and nothing was killed. + let message = result.unwrap_err().to_string(); + assert!(!message.contains("killed"), "got: {message}"); +} + +#[test] +fn grep_is_unchanged_by_a_token_nobody_raised() { + let root = tempfile::tempdir().unwrap(); + fs::write(root.path().join("a.txt"), "needle\n").unwrap(); + + let ctx = context(SandboxPolicy::default().allow_read(vetted(root.path()))) + .with_cancel(Cancel::new()); + let out = BuiltinTool::Grep + .execute( + json!({ "path": root.path().to_str().unwrap(), "pattern": "needle" }), + &ctx, + ) + .unwrap(); + + assert!(out.content().contains("a.txt:1"), "got: {}", out.content()); +} + #[cfg(unix)] #[test] fn grep_does_not_follow_a_symlink_out_of_the_root() { diff --git a/crates/sandbx-turn/src/tools.rs b/crates/sandbx-turn/src/tools.rs index 0ccad249..5d385c87 100644 --- a/crates/sandbx-turn/src/tools.rs +++ b/crates/sandbx-turn/src/tools.rs @@ -3,9 +3,13 @@ //! The crate's only `spawn_blocking` site. Tools are synchronous, so the sync/async //! boundary lives here rather than spread through seven tool bodies. +use futures_util::future::{Either, select}; use sandbx_approval::{ApprovalDecision, CallGate, Outcome, Settled, ToolCall}; use sandbx_prompt::{ContentBlock, ToolDefinition}; -use sandbx_tools::{BuiltinTool, ExecutionContext}; +use sandbx_tools::{BuiltinTool, ExecutionContext, RiskLevel, ToolError}; +use std::pin::pin; +use std::time::Duration; +use tokio::task::{JoinError, JoinHandle}; use crate::TurnError; @@ -15,12 +19,15 @@ use crate::TurnError; /// `ExecutionContext` settled first (#242). `BuiltinTool::execute` may sit in a `write`, a /// directory walk or a 90-second command, which on a current-thread runtime would freeze /// every other task — hence `spawn_blocking`, whose uncancellability `run_turn` documents. -/// Every answered block reports to [`CallGate::settled`], including the two refusals above -/// the gate, which the gate's own verdict cannot account for. +/// A stop abandons a read-only call there and still waits for a `write` or `edit`. +/// Every call that was put to the gate, or refused above it, reports to +/// [`CallGate::settled`]. A call behind a stop is answered as not run and reports nothing: +/// it never reached the gate, so there is no verdict for the account to close. /// -/// A cancelled `ctx` ends the loop early, leaving a `tool_use` with no `tool_result`: the -/// caller must drop the round, which `run_turn_until` does and is the only caller to hold -/// the token. +/// A cancelled `ctx` ends the loop early. Every call still gets a result, so the round stays +/// legal to store: a call behind the stop says it did not run, and one that settled once +/// the token was raised says its output was withheld, whatever it returned. Only +/// `run_turn_until` holds the token. pub(crate) async fn answer_calls( blocks: &[ContentBlock], ctx: &ExecutionContext, @@ -68,7 +75,7 @@ where continue; } - // Before `spawn_blocking`, never racing it: an in-process tool cannot be cancelled, + // Before `spawn_blocking`, never racing it: a `write` or `edit` cannot be cancelled, // so a late decision would not stop the call it refused (#26). let verdict = match &aborted { Some(reason) => ApprovalDecision::Deny { @@ -99,34 +106,107 @@ where // consumes it; an `Arc` would only pay off past a few path lists. let arguments = input.clone(); let context = ctx.clone(); - let outcome = tokio::task::spawn_blocking(move || tool.execute(arguments, &context)) - .await - .map_err(|_| TurnError::ToolPanicked { - name: name.to_string(), - })?; + let mut running = tokio::task::spawn_blocking(move || tool.execute(arguments, &context)); + let joined = match tool.risk() { + RiskLevel::ReadOnly => unless_stopped(&mut running, ctx).await, + RiskLevel::Writes | RiskLevel::Executes => Some(running.await), + }; + let outcome = match joined { + Some(Ok(outcome)) => outcome, + // Answered and not propagated: the calls before it in this round have landed, and + // a round dropped for one panic would leave a resume knowing nothing of them. + Some(Err(_)) if ctx.cancelled() => Err(ToolError::Failed { + subject: name.to_string(), + detail: "the tool panicked".to_string(), + }), + Some(Err(_)) => { + return Err(TurnError::ToolPanicked { + name: name.to_string(), + }); + } + // Left running on its blocking thread, its result never read. Safe only because it + // is read-only: abandoning a `write` is what the gate's position ahead of the + // spawn exists to rule out. Exhaustive above, so a fourth level must pick a side. + None => Err(ToolError::Cancelled { + subject: name.to_string(), + }), + }; + // Read after the join, never before the spawn: a call that finished a moment before + // the stop is withheld too, which errs toward saying less of what the operator stopped. + let stopped = ctx.cancelled(); results.push(match outcome { Ok(output) => { gate.settled(settle(Some(tool), Outcome::Ran)); - ContentBlock::ToolResult { - tool_use_id: id.clone(), - content: output.into_content(), - is_error: None, + if stopped { + refused(id, FINISHED_AFTER_STOP.to_string()) + } else { + ContentBlock::ToolResult { + tool_use_id: id.clone(), + content: output.into_content(), + is_error: None, + } } } Err(error) => { gate.settled(settle(Some(tool), Outcome::Errored(&error))); - refused(id, error.to_string()) + match (stopped, &error) { + (false, _) => refused(id, error.to_string()), + (true, ToolError::Cancelled { .. }) => refused(id, CANCELLED.to_string()), + (true, _) => refused(id, ERRORED_AFTER_STOP.to_string()), + } } }); } + // A stop ends the loop above early. Answered here, in the order asked, because a + // `tool_use` with no `tool_result` is a transcript no provider takes back, and a + // stopped round is stored and resumed. + // By position and not by id: each call leaves exactly one result, in order, so the + // unanswered are the suffix. + let behind: Vec<&String> = blocks + .iter() + .filter_map(|block| match block { + ContentBlock::ToolUse { id, .. } => Some(id), + _ => None, + }) + .skip(results.len()) + .collect(); + results.extend( + behind + .into_iter() + .map(|id| refused(id, NOT_RUN.to_string())), + ); + Ok(Answers { results, aborted: aborted.is_some(), }) } +/// How often a running read-only call is checked for a stop. +/// +/// A poll and not a wake-up because [`Cancel`](sandbx_tools::Cancel) is a polled flag by +/// design. Bounds how long a stop waits on a call that cannot finish. +const STOP_POLL: Duration = Duration::from_millis(50); + +/// `call`'s result, or `None` once `ctx` is cancelled while it is still running. +/// +/// `call` is borrowed and so left running, not aborted: nothing can interrupt a blocking +/// thread, only stop waiting for it. +async fn unless_stopped( + call: &mut JoinHandle, + ctx: &ExecutionContext, +) -> Option> { + loop { + match select(&mut *call, pin!(tokio::time::sleep(STOP_POLL))).await { + Either::Left((joined, _)) => return Some(joined), + Either::Right(((), _)) if ctx.cancelled() => return None, + Either::Right(((), _)) => {} + } + } +} + /// One round's answers, and whether the gate ended the turn giving them. pub(crate) struct Answers { /// One `tool_result` per `tool_use` block, in the order the model asked. @@ -136,6 +216,29 @@ pub(crate) struct Answers { pub(crate) aborted: bool, } +// What a stopped round stores in place of a call's own result. Fixed text and never a +// path, a command or a tool's output: a resume sends these to the provider, and the +// operator stopped before reading any of it. + +/// A call behind the one the stop landed on, never put to the gate or run. +const NOT_RUN: &str = "not run: the turn was stopped"; + +/// A call the stop killed or abandoned before it finished. Says it may have changed +/// something, because a `bash` killed halfway recorded as plainly cancelled would be rerun +/// on top of what it had done. +const CANCELLED: &str = "cancelled: the turn was stopped before it finished; anything it \ + had already done may remain"; + +/// A call that finished once the stop was raised. Says its effect stands, because a `write` +/// that landed and was recorded as not run would be done again. +const FINISHED_AFTER_STOP: &str = "was running when the turn was stopped; it finished, its \ + effect stands, its output was withheld"; + +/// A call that failed once the stop was raised. Claims no effect either way: a failed +/// `write` changed nothing, a timed-out `bash` may have. +const ERRORED_AFTER_STOP: &str = "was running when the turn was stopped; it did not succeed, \ + and its error was withheld"; + fn refused(id: &str, content: String) -> ContentBlock { ContentBlock::ToolResult { tool_use_id: id.to_string(), diff --git a/crates/sandbx-turn/src/turn.rs b/crates/sandbx-turn/src/turn.rs index ae304097..ccfadf5c 100644 --- a/crates/sandbx-turn/src/turn.rs +++ b/crates/sandbx-turn/src/turn.rs @@ -137,12 +137,16 @@ pub enum TurnStop { /// `tool_result`. GateAborted, - /// [`run_turn_until`]'s `stop` resolved, and the round it found running was dropped. + /// [`run_turn_until`]'s `stop` resolved. /// - /// The transcript is every round before it, as legal to send again as - /// [`RoundLimit`](Self::RoundLimit)'s, or empty. Not in it: anything of the dropped - /// round, the calls it had already run included. A `bash` it had started is killed; any - /// other call it had started finishes, and its effect stays (#26). + /// A stop that lands while the model is streaming drops that round: the transcript is + /// every round before it, or empty. One that lands in a round's tool calls keeps the + /// round, every call answered, so it ends on a user turn of results, as legal to send + /// again as [`RoundLimit`](Self::RoundLimit)'s. A call that had settled when the stop + /// was raised says what it found; one running then is killed if it is a `bash`, left + /// running if it only reads, and waited for if it writes, its effect standing (#26), and + /// says only that its output was withheld or that it was cancelled; one behind it was + /// never put to the gate or run, and says so. Interrupted, } @@ -150,7 +154,7 @@ pub enum TurnStop { /// /// None of them bounds a turn's total wall-clock time, and no product of them does: the /// in-process tools are bounded by work, not time, so a single `read` on a stalled filesystem -/// is bounded by nothing here. +/// is bounded by nothing here, though a stop no longer waits for it. #[derive(Debug, Clone, Copy)] pub struct TurnLimits { /// How many times the model may be asked within one turn. @@ -217,7 +221,8 @@ impl Default for TurnLimits { /// Tools run on `spawn_blocking`, uncancellable: dropping this future still lets the /// blocking task finish, so a turn abandoned mid-tool applies the `write` anyway, or lets a /// `bash` run out its timeout, recorded only in the audit trail (#26). [`run_turn_until`] -/// stops a turn instead: it keeps the rounds before the stop and kills a running `bash`. +/// stops a turn instead: it keeps the rounds before the stop, the one it landed in among +/// them, and kills a running `bash`. /// /// Needs a tokio runtime with the time driver enabled: the per-round bound is /// `tokio::time::timeout`, which panics with "there is no timer running" otherwise. @@ -245,12 +250,18 @@ where /// the only way to cancel a call from here. /// /// Raced against each round's request and its tool calls, `stop` first, so a stop that -/// has already resolved opens no further request. The round it lands in is dropped whole, -/// calls already answered in it included. A call already running is waited for rather than -/// abandoned, and reported to [`CallGate::settled`] as it ends: a `bash` is killed, but an -/// in-process tool cannot be cancelled (#26), so it finishes and its effect stays. No call +/// has already resolved opens no further request. A round it lands in while streaming is +/// dropped whole; one it lands in during its calls is kept, [`TurnStop::Interrupted`] says +/// how. A call already running is reported to [`CallGate::settled`] as it ends: a `bash` is +/// killed, and a read-only call (`read`, `ls`, `grep`, `find`) is abandoned on its blocking +/// thread with its result unread, a `grep` also stopping at its next file. A `write` or +/// `edit` cannot be cancelled (#26), so it is waited for and its effect stays. No call /// behind it is put to the gate or run. /// +/// An abandoned call keeps its blocking thread, and dropping a runtime waits for every one: +/// a caller that must exit promptly after a stop ends its runtime with +/// `shutdown_background`, as `sandbx` does. +/// /// [`TurnOutcome::usage`] keeps the dropped round's figure when it reported one, its request /// being exactly the messages kept. [`TurnOutcome::withheld`] is the cut of the last request /// that finished streaming, the carried floor before one did: a cut planned for a round @@ -348,13 +359,7 @@ where })? }; let Some(round) = unless(stop.as_mut(), streamed).await else { - return Ok(outcome( - produced, - usage, - streamed_cut, - TurnStop::Interrupted, - last_stop, - )); + return Ok(interrupted(produced, usage, streamed_cut, last_stop)); }; let round = round?; streamed_cut = cut; @@ -380,17 +385,17 @@ where // Before pushing: answering borrows `round.blocks`, pushing moves them. let answered = answer_calls(&round.blocks, ctx, turn.tools, &mut gate); - let Some(answers) = unless_cancelling(stop.as_mut(), &cancel, answered).await else { - return Ok(outcome( - produced, - usage, - streamed_cut, - TurnStop::Interrupted, - last_stop, - )); - }; + let (answers, stopped) = unless_cancelling(stop.as_mut(), &cancel, answered).await; + // A tool that panics after the stop is answered as a failure, so an error here is one + // that came before it. let Answers { results, aborted } = answers?; + // No result means no call in the round, so nothing a stop could have left unsaid: it + // is dropped as it was before calls were kept, not stored as a bare assistant turn. + if stopped && results.is_empty() { + return Ok(interrupted(produced, usage, streamed_cut, last_stop)); + } + produced.push(RequestMessage { role: Role::Assistant, content: round.blocks, @@ -413,7 +418,12 @@ where }); // After the push, so the transcript ends on this round's results, as `RoundLimit` - // already hands back. + // already hands back. Before `aborted`: a stop that lands in the round the gate + // ended is the stop, not the abort. + if stopped { + return Ok(interrupted(produced, usage, streamed_cut, last_stop)); + } + if aborted { return Ok(outcome( produced, @@ -453,23 +463,38 @@ async fn unless( /// wait for `work` to wind down, rather than drop it with a call still running on a thread /// nothing can reach. /// -/// What `work` returns then is discarded, the round it belongs to being dropped whole. +/// `work`'s output and whether it was stopped. A stopped `work` has answered every call it was +/// asked, so the round it belongs to is kept. async fn unless_cancelling( stop: Pin<&mut impl Future>, cancel: &Cancel, work: impl Future, -) -> Option { +) -> (T, bool) { let mut work = pin!(work); match select(stop, work.as_mut()).await { Either::Left(((), _)) => { cancel.raise(); - work.await; - None + (work.await, true) } - Either::Right((done, _)) => Some(done), + Either::Right((done, _)) => (done, false), } } +fn interrupted( + produced: Vec, + usage: Option, + streamed_cut: usize, + last_stop: Option, +) -> TurnOutcome { + outcome( + produced, + usage, + streamed_cut, + TurnStop::Interrupted, + last_stop, + ) +} + /// The one exit from [`run_turn_until`], so no path can return reasoning to a caller. fn outcome( mut produced: Vec, diff --git a/crates/sandbx-turn/tests/turn_stop.rs b/crates/sandbx-turn/tests/turn_stop.rs index 1f1c191f..10d671fe 100644 --- a/crates/sandbx-turn/tests/turn_stop.rs +++ b/crates/sandbx-turn/tests/turn_stop.rs @@ -1,11 +1,16 @@ //! Public contract of `run_turn_until`: what a stopped turn keeps. +// `mkfifo` is spawned to build a fixture: std cannot create a named pipe. Not code +// escaping the sandbox, which is what the ban on `Command::new` exists to stop. +#![allow(clippy::disallowed_methods)] use std::future::{self, Future}; use std::path::Path; +use std::process::Command; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Mutex, mpsc}; use std::task::Poll; -use std::time::Duration; +use std::time::{Duration, Instant}; +use std::{fs, thread}; use futures_util::task::AtomicWaker; use futures_util::{StreamExt, stream}; @@ -29,6 +34,41 @@ fn listing(root: &tempfile::TempDir) -> Vec { ] } +/// Opens `fifo` for writing once `delay` has passed, which wakes a reader parked on it, and +/// joins that thread when dropped. +struct Release(Option>); + +impl Release { + fn after(delay: Duration, fifo: std::path::PathBuf) -> Self { + Self(Some(thread::spawn(move || { + thread::sleep(delay); + let _ = fs::OpenOptions::new().write(true).open(fifo); + }))) + } +} + +impl Drop for Release { + fn drop(&mut self) { + if let Some(thread) = self.0.take() { + let _ = thread.join(); + } + } +} + +/// The `(content, is_error)` of each `tool_result` in a user turn. +fn answers(message: &RequestMessage) -> Vec<(&str, Option)> { + message + .content + .iter() + .map(|block| match block { + ContentBlock::ToolResult { + content, is_error, .. + } => (content.as_str(), *is_error), + other => panic!("not a tool_result: {other:?}"), + }) + .collect() +} + /// A stop raised inside a tool call, landing before that call can finish (#497). /// /// Raising a flag alone races the call: an `ls` that completes before its `JoinHandle` is @@ -191,11 +231,18 @@ async fn a_stop_already_resolved_sends_nothing() { } /// The stop reaches a round that has more calls behind the stopped one: the stopped call -/// is waited for and settles, and nothing behind it is put to the gate or run (#26). A -/// `write` behind it, because an `ls` leaves no mark to look for. +/// is waited for and settles, and nothing behind it is put to the gate or run (#26). The +/// stopped call is a `write`, which is always waited for and lands; a read-only call is +/// abandoned instead, so it could settle either way. +/// +/// The round is kept with every call answered: the one that finished before the stop says +/// what it found, the stopped one says only that it finished, and the one behind it says it +/// did not run. #[test] fn a_stop_mid_call_starts_no_call_behind_it() { let root = tempfile::tempdir().unwrap(); + fs::write(root.path().join("seen.txt"), "").unwrap(); + let landed = root.path().join("landed.txt"); let behind = root.path().join("behind.txt"); let ctx = ctx(SandboxPolicy::default() .allow_read(vetted(root.path())) @@ -203,8 +250,11 @@ fn a_stop_mid_call_starts_no_call_behind_it() { let listed = serde_json::json!({ "path": root.path().to_str().unwrap() }); let mut script = Script::new([ vec![ - call("ls", listed.clone()), call("ls", listed), + call( + "write", + serde_json::json!({ "path": landed.to_str().unwrap(), "content": "landed" }), + ), call( "write", serde_json::json!({ "path": behind.to_str().unwrap(), "content": "behind it" }), @@ -223,16 +273,77 @@ fn a_stop_mid_call_starts_no_call_behind_it() { assert_eq!(stopped.outcome.stop, TurnStop::Interrupted); assert_eq!(stopped.settled, 2, "the stopped call was not waited for"); assert_eq!(stopped.asked, 2, "a call behind the stop reached the gate"); + assert_eq!(fs::read_to_string(&landed).unwrap(), "landed"); assert!(!behind.exists(), "a call behind the stop ran"); + + let [assistant, user] = &stopped.outcome.messages[..] else { + panic!( + "expected the stopped round, got {:?}", + stopped.outcome.messages + ); + }; + assert_eq!(assistant.role, Role::Assistant); + let [before, during, after] = &answers(user)[..] else { + panic!("a result for each of three calls, got {user:?}"); + }; + assert!(before.0.contains("seen.txt"), "got {before:?}"); + assert_eq!(before.1, None); + // What the operator stopped before reading, and a resume would send on. + assert!(during.0.contains("withheld"), "got {during:?}"); assert!( - stopped.outcome.messages.is_empty(), - "the stopped round was kept: {:?}", - stopped.outcome.messages + !during.0.contains("landed"), + "the write's own report got through: {during:?}" + ); + assert_eq!(during.1, Some(true)); + assert!(after.0.contains("not run"), "got {after:?}"); + assert_eq!(after.1, Some(true)); +} + +/// A `write` the stop landed on that then fails changed nothing, and is not stored as a +/// call whose effect stands: a resume would take it for done, and never redo it. +#[test] +fn a_call_that_fails_after_the_stop_is_not_stored_as_done() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let refused = outside.path().join("refused.txt"); + let ctx = ctx(SandboxPolicy::default() + .allow_read(vetted(root.path())) + .allow_write(vetted(root.path()))); + let listed = serde_json::json!({ "path": root.path().to_str().unwrap() }); + let mut script = Script::new([ + vec![ + call("ls", listed), + call( + "write", + serde_json::json!({ "path": refused.to_str().unwrap(), "content": "no" }), + ), + stop(StopReason::ToolUse), + ], + vec![text("never asked"), stop(StopReason::EndTurn)], + ]); + + let stopped = stopped_in_second_call_under( + &mut script, + turn(&[], &[BuiltinTool::Ls, BuiltinTool::Write]), + &ctx, ); + + assert!(!refused.exists(), "the write was outside the policy"); + let [_, user] = &stopped.outcome.messages[..] else { + panic!( + "expected the stopped round, got {:?}", + stopped.outcome.messages + ); + }; + let [_, write] = &answers(user)[..] else { + panic!("a result for each of two calls, got {user:?}"); + }; + assert!(!write.0.contains("effect stands"), "got {write:?}"); + assert!(write.0.contains("did not succeed"), "got {write:?}"); + assert_eq!(write.1, Some(true)); } -/// Round one alone: the stop drops the stopped round's answers, the call it found running -/// having been waited for first (#26). +/// Round one answered, the stop in round two's only call: both rounds are kept (#26). #[test] fn a_stop_mid_call_keeps_the_rounds_before_it() { let root = tempfile::tempdir().unwrap(); @@ -246,18 +357,84 @@ fn a_stop_mid_call_keeps_the_rounds_before_it() { assert_eq!(outcome.stop, TurnStop::Interrupted); let roles: Vec = outcome.messages.iter().map(|m| m.role).collect(); - assert_eq!(roles, [Role::Assistant, Role::User], "round one alone"); + assert_eq!( + roles, + [Role::Assistant, Role::User, Role::Assistant, Role::User] + ); + // An `ls` is abandoned or finished, whichever the poll finds first: either says nothing of + // what it listed. + let last = answers(&outcome.messages[3]); + assert_eq!(last.len(), 1, "got {:?}", outcome.messages); + assert_eq!(last[0].1, Some(true), "got {last:?}"); assert!( - matches!( - outcome.messages[1].content[..], - [ContentBlock::ToolResult { .. }] - ), - "got {:?}", - outcome.messages + last[0].0.contains("withheld") || last[0].0.contains("cancelled"), + "got {last:?}" ); assert_eq!(script.sent.len(), 2, "no third request after the stop"); } +/// A `read` of a FIFO with no writer waits in `open` for as long as it takes someone to +/// write, which no stop can interrupt. It is read-only and writes nothing, so the stop +/// abandons it instead of waiting (#26). +#[tokio::test] +async fn a_stop_does_not_wait_for_a_read_that_cannot_finish() { + let root = tempfile::tempdir().unwrap(); + let fifo = root.path().join("pipe"); + let made = Command::new("mkfifo") + .arg(&fifo) + .status() + .expect("mkfifo should run"); + assert!(made.success()); + let mut script = Script::new([ + vec![ + call( + "read", + serde_json::json!({ "path": fifo.to_str().unwrap() }), + ), + stop(StopReason::ToolUse), + ], + vec![text("never asked"), stop(StopReason::EndTurn)], + ]); + // Frees the thread the read is parked on after two seconds, a stop that waits for it + // then failing the bound below instead of hanging the runtime's drop. Declared after + // `root`, so it is joined before the directory goes: with the FIFO unlinked the open + // fails, and the read stays parked. + let _release = Release::after(Duration::from_secs(2), fifo.clone()); + let started = Instant::now(); + + let outcome = tokio::time::timeout( + Duration::from_secs(10), + run_turn_until( + async |r| script.open(r).await, + turn(&[], &[BuiltinTool::Read]), + &ctx(SandboxPolicy::default().allow_read(vetted(root.path()))), + |_| {}, + AllowAll, + tokio::time::sleep(Duration::from_millis(200)), + ), + ) + .await + .expect("the stop ended the turn") + .unwrap(); + let elapsed = started.elapsed(); + + assert_eq!(outcome.stop, TurnStop::Interrupted); + assert!( + elapsed < Duration::from_secs(1), + "the stop waited {elapsed:?} for a read" + ); + let [_, user] = &outcome.messages[..] else { + panic!("expected the stopped round, got {:?}", outcome.messages); + }; + let [result] = &answers(user)[..] else { + panic!("a result for the one call, got {user:?}"); + }; + assert!(result.0.contains("cancelled"), "got {result:?}"); + // A killed `bash` shares this text, and may have changed state before it died. + assert!(result.0.contains("already done"), "got {result:?}"); + assert_eq!(result.1, Some(true)); +} + /// A round that streams forever: only the stop can end it under a 300-second bound. fn stalled() -> EventStream { Box::pin(stream::pending()) @@ -311,7 +488,7 @@ fn a_stopped_turn_returns_no_reasoning() { let outcome = stopped_in_second_call(&mut script, turn(&[], &[BuiltinTool::Ls]), root.path()); assert_eq!(outcome.stop, TurnStop::Interrupted); - assert_eq!(outcome.messages.len(), 2, "got {:?}", outcome.messages); + assert_eq!(outcome.messages.len(), 4, "got {:?}", outcome.messages); assert!( !outcome .messages