diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index 3c1ae1db..53eeca0e 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -136,7 +136,7 @@ reaches. Read alongside the last-event age below, it's what separates a subagent that's working from one that's wedged from one that's out of turns — without `ps` and without opening a file. -Four things stop a run; the daemon records each distinctly, reports it via +Five things stop a run; the daemon records each distinctly, reports it via `status`, and appends it to the one todo it pushes when the run ends: - **the turn ended and there was no goal** — the single-turn case; @@ -144,11 +144,22 @@ Four things stop a run; the daemon records each distinctly, reports it via - **`need_help`**, likewise; - **the turn cap**, which says so rather than stopping quietly: the todo states that the run hit the harness limit and the goal was never - reported reached, so the work stopped where it had got to. + reported reached, so the work stopped where it had got to; +- **`interrupt`**, which cancels the whole run and not merely the turn + that was in flight. A killed or failed turn ends the run too, and keeps the records it already -had — see [A killed turn](#a-killed-turn). `interrupt` therefore stops a -whole goal run, not just the turn in flight. +had — see [A killed turn](#a-killed-turn). + +`interrupt` being on that list is load-bearing, not bookkeeping. Killing +the process isn't by itself a cancel: `interrupt` sends SIGINT by +default, and `claude --print` handles SIGINT by writing its terminal +result event and exiting **zero**, which is indistinguishable from a turn +that finished. `interrupt` therefore records the cancel before it +signals, and the continuation loop reads that record rather than guessing +from how the child exited. `continue` clears it and hands the session a +fresh turn allowance, which is what makes an interrupt a pause you can +undo rather than a session you have to abandon. When the session knows where its report goes — `start`'s `report_file`, or the path the subagent names when it signals — the daemon appends the diff --git a/hive-subagent-mcp/src/mcp.rs b/hive-subagent-mcp/src/mcp.rs index 21f4d7ee..67f0825a 100644 --- a/hive-subagent-mcp/src/mcp.rs +++ b/hive-subagent-mcp/src/mcp.rs @@ -268,11 +268,13 @@ impl SubagentMcp { } #[tool( - description = "Signal a currently-running subagent session to stop. Only works once \ - it's actually running — a `start`/`continue` still in its brief window before the \ - process is confirmed spawned refuses interrupt too (nothing to signal yet; retry \ - shortly), same as a name with nothing tracked at all. `force: true` for SIGKILL, \ - otherwise SIGINT." + description = "Cancel a currently-running subagent session. This stops the whole run, \ + not just the turn in flight: a session started with a `goal` will not be \ + re-prompted toward it afterwards, and `continue` is what restarts it. Only works \ + once it's actually running — a `start`/`continue` still in its brief window before \ + the process is confirmed spawned refuses interrupt too (nothing to signal yet; \ + retry shortly), same as a name with nothing tracked at all. `force: true` for \ + SIGKILL, otherwise SIGINT." )] fn interrupt(&self, Parameters(args): Parameters) -> String { match session::interrupt(&self.state, &args.name, args.force) { diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 8f0337fd..e01f6acc 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -1,8 +1,8 @@ //! The claude-facing half of this daemon: spawn a subagent turn, keep giving //! it turns until it says it's done or runs out of them, track it only while //! it's alive, and push exactly one todo when the whole run stops — saying -//! whether it finished, was killed, or stopped for one of the four reasons -//! the continuation loop records. +//! whether it finished, was killed, or stopped for one of the reasons the +//! continuation loop records. //! //! **No task files, no restart recovery.** The daemon's only state is an //! in-memory `name -> Option` map (see `State`'s own doc for the @@ -110,11 +110,11 @@ const DEFAULT_MAX_TURNS: u32 = 5; /// by `status`, appended to the end-of-turn todo, and written into the /// session's report file when it has one. /// -/// Only the four ends of the *continuation loop* live here. A turn that was -/// killed or that failed outright never reaches the loop's decision at all: -/// those keep the records they already had (`State::killed`, the todo's own -/// killed wording), and giving them a second home here would have handed -/// `status` two rival answers for one fact. +/// Only ends of the *continuation loop* live here. A turn that was killed or +/// that failed outright never reaches the loop's decision at all: those keep +/// the records they already had (`State::killed`, the todo's own killed +/// wording), and giving them a second home here would have handed `status` +/// two rival answers for one fact. #[derive(Debug, Clone, PartialEq, Eq)] enum StopReason { /// The turn ended and there was no goal to continue toward — the @@ -127,6 +127,19 @@ enum StopReason { NeedHelp(String), /// `turns` turns ran and the goal was never reported reached. TurnCap { turns: u32 }, + /// An operator called `interrupt`. Unlike the four above, this is + /// recorded from *outside* the run, by `interrupt` itself, before the + /// signal is sent. + /// + /// It has to be, and that is the whole of it: `interrupt`'s + /// default is SIGINT, and claude handles SIGINT by writing its terminal + /// `result` event and **exiting 0**. An exit of zero is + /// `TurnEnd::Complete`, so the killed-turn early return in + /// `spawn_and_track` never fires and the loop re-prompts a run the + /// operator had just asked to stop. Recording the reason makes the + /// cancel a fact about the *run* rather than an inference from how its + /// child happened to die. + Cancelled, } impl StopReason { @@ -156,6 +169,9 @@ impl StopReason { "the harness turn limit was reached ({turns} turns) without the goal ever being \ reported reached, so the work stopped where it had got to" ), + Self::Cancelled => "it was interrupted, which cancels the whole run and not just the \ + turn that was in flight — its work stopped wherever it had got to" + .to_owned(), } } } @@ -1889,7 +1905,7 @@ fn describe_pending_signal(stop: Option<&StopReason>) -> String { /// The answer for a session whose run has stopped, one per [`StopReason`]. /// Each names the state, why the continuation stopped, and what `continue` /// would do about it — and the `GoalReached` one is deliberately the least -/// reassuring of the four, because it is the one a caller is most likely to +/// reassuring of them, because it is the one a caller is most likely to /// read as "finished successfully" when it means "said so". fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String { match stop { @@ -1919,6 +1935,12 @@ fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String { than finishing.{turns} Check what it actually left behind; `continue` gives it a \ fresh allowance if carrying on is worth it." ), + StopReason::Cancelled => format!( + "subagent `{name}` was CANCELLED — someone called `interrupt` on it, which stopped \ + the run and not merely the turn that was in flight, so nothing further will start \ + on its own.{turns} Its work stopped wherever it had got to: check what it left \ + behind before acting on it. `continue` restarts it, with a fresh turn allowance." + ), } } @@ -1928,12 +1950,25 @@ fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String { /// to signal yet — the reservation is put back so a concurrent /// `start`/`continue` for the same name still gets refused). /// -/// This stops a goal run, not just the turn in it: the signalled child ends -/// as `TurnEnd::Killed`, which the continuation loop treats as the end of -/// the whole run rather than something to re-prompt past. That falls out of -/// there being no child left to continue, and it is the answer you want — -/// `interrupt` would be useless if the harness immediately started turn -/// three of five. +/// This stops a goal run, not just the turn in it — and it does so by +/// **recording `StopReason::Cancelled`**, not by relying on how the child +/// dies. +/// +/// That distinction is the entire fix. The signal alone is not enough: the +/// default is SIGINT, claude handles SIGINT by emitting its terminal +/// `result` event and exiting 0, and a zero exit is `TurnEnd::Complete` — +/// so `spawn_and_track`'s killed-turn early return never fires and the loop +/// goes straight on to spawn turn three of five. The recorded reason is what +/// `plan_after_turn` reads, and it is checked before the goal, so the run +/// stops whichever way the child ends up exiting. `force`'s SIGKILL does +/// produce a `TurnEnd::Killed`; recording the reason anyway costs nothing +/// and keeps one path, and `status` still prefers the kill record it +/// already had. +/// +/// The reason is written while the `running` lock is still held, so there is +/// no instant in which the name has lost its cancel handle but not yet +/// gained its stop reason — a turn ending of its own accord in that gap +/// would find nothing to stop it and start the next one. /// /// # Errors /// @@ -1952,9 +1987,14 @@ pub fn interrupt(state: &State, name: &str, force: bool) -> anyhow::Result { + state.record_stop(name, StopReason::Cancelled); drop(running); cancel.cancel(force); - Ok(format!("interrupt sent to subagent `{name}`")) + Ok(format!( + "interrupt sent to subagent `{name}` — the run is cancelled, not just the turn \ + that was in flight, so no further goal turn will start. `continue` is what \ + restarts it, with a fresh turn allowance." + )) } } } @@ -3181,6 +3221,146 @@ mod tests { } } + /// A stand-in for claude that reproduces the one behaviour the cancel + /// turns on: on SIGINT it exits **zero**. The real `claude --print` does + /// exactly this — it writes its terminal `result` event and shuts down + /// cleanly — which is why an interrupted turn is indistinguishable from + /// a completed one by exit status alone, and why `interrupt` cannot rely + /// on the child's death to stop the run. + /// + /// It appends a line to `log` as its very first act, so the file is a + /// count of how many turns the continuation loop actually spawned. Every + /// argument is ignored; the driver's own flags mean nothing here. + /// + /// The wait is `sleep &` + `wait` rather than a plain foreground + /// `sleep`: a POSIX shell runs a pending trap as soon as `wait` returns, + /// but defers it until a *foreground* child has finished — which for a + /// 30-second sleep is long after the test has given up. The background + /// sleep gets its own stdout/stderr for the mirror-image reason: it + /// would otherwise inherit and keep open the pipes the driver reads to + /// EOF, so `RunningClaude::wait` would not return until the orphaned + /// sleep expired, and the turn would look like it was still running. + fn fake_claude(dir: &Path, log: &Path) -> PathBuf { + use std::os::unix::fs::PermissionsExt as _; + let script = dir.join("fake-claude"); + std::fs::write( + &script, + format!( + "#!/bin/sh\necho turn >> '{}'\ntrap 'exit 0' INT\nsleep 30 >/dev/null 2>&1 &\nwait\n", + log.display() + ), + ) + .expect("write the fake claude"); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)) + .expect("make the fake claude executable"); + script + } + + /// How many turns the fake has been spawned for so far. + fn turns_spawned(log: &Path) -> usize { + std::fs::read_to_string(log).map_or(0, |body| body.lines().count()) + } + + #[tokio::test] + async fn an_interrupt_cancels_the_whole_goal_run_and_not_merely_its_turn() { + // The regression, end to end through the real + // continuation loop: a goal run with four turns left is interrupted, + // its child exits cleanly the way claude actually does, and the loop + // must not spawn turn two. + let dir = std::env::temp_dir().join(format!( + "hive-subagent-cancel-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let log = dir.join("turns"); + let program = fake_claude(&dir, &log); + + let state = Arc::new(mid_run("n", "carry on indefinitely", 5)); + let config = Config { + program: Some(program), + ..Config::default() + }; + spawn_and_track( + &state, + "n", + &config, + &Attach::Create("n".to_owned()), + "turn one".to_owned(), + None, + ) + .expect("the fake claude spawns"); + + // Wait for the first turn to be genuinely under way before + // interrupting: signalling a child that hasn't run yet would prove + // nothing about the loop. + for _ in 0..100 { + if turns_spawned(&log) == 1 { + break; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + assert_eq!(turns_spawned(&log), 1, "the first turn never started"); + + // An operator's plain `interrupt`, not `force` — SIGINT is the + // default and the case that was broken. + interrupt(&state, "n", false).expect("a running turn is interruptible"); + + // Generous: the fake logs its invocation as its first act, so a + // re-prompt shows up here almost immediately once the loop decides + // to make one. + tokio::time::sleep(Duration::from_millis(1500)).await; + assert_eq!( + turns_spawned(&log), + 1, + "an interrupted goal run must not be re-prompted — that is the whole of the cancel" + ); + assert_eq!( + state.stop_reason("n"), + Some(StopReason::Cancelled), + "and the run must carry a reason saying why it stopped" + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_cancelled_run_stops_with_turns_still_on_its_clock() { + // The loop half of the same fact, asserted directly: `Cancelled` is + // read where the other stop signals are, ahead of the goal, so it + // stops a run that has four turns left rather than being overridden + // by them. + let state = mid_run("n", "a goal", 5); + state.record_stop("n", StopReason::Cancelled); + assert_eq!( + state.plan_after_turn("n"), + Continuation::Stop(StopReason::Cancelled) + ); + assert_eq!( + state.turns("n"), + Some((1, 5)), + "and must not spend one on the way out" + ); + } + + #[test] + fn a_continue_after_a_cancel_starts_the_run_over() { + // The deliberate other half: a cancel is not a permanent + // state. `continue` clears the reason and hands back a fresh + // allowance, which is what makes `interrupt` a pause an operator can + // undo rather than a session they have to abandon. + let state = mid_run("n", "a goal", 5); + state.record_stop("n", StopReason::Cancelled); + state.clear_stop("n"); + state.restart_turns("n"); + assert_eq!( + state.plan_after_turn("n"), + Continuation::Continue { + prompt: continuation_prompt("n", "a goal", 2, 5) + }, + "a continued session resumes toward its goal" + ); + } + #[test] fn a_signal_on_the_last_allowed_turn_outranks_the_cap() { // Both are true at once, and which one is reported is the difference