From b18348bc9ade64d2d20fd4d6a177001a06341e5f Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 14 Sep 2026 21:42:46 +0200 Subject: [PATCH] subagent: give a run a goal, turns toward it, and a reason it stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `start` takes an optional `goal`. With one set a session stops being a single turn: when a turn ends and nothing has said to stop, the daemon spawns another turn re-prompting the subagent toward that goal, up to `max_turns` (default 5, per-session). Without a goal nothing changes — one turn, one todo, same as before. Four things end a run, each recorded distinctly and reported by `status`: the turn ending with no goal, `goal_reached`, `need_help`, and the turn cap. The last says so out loud rather than stopping quietly — the todo states the harness limit was reached and the goal was never reported reached. Every stop extends the done message rather than replacing it, and lands in the session's report file when it has one. The path is never inferred: it comes from `start`'s `report_file` or from the subagent naming where it wrote. `goal_reached` and `need_help` are the subagent's own, served on a second route (`/signal/mcp`) that carries those two tools and nothing else, so reporting on a run can't become starting one. `goal_reached` is built as a label, never a gate: it is self-reported by a subagent that has just been re-prompted with "you haven't reached the goal", which is exactly the incentive to claim it — the same failure class as a build report asserting the tests pass. Every surface that renders it says so. `need_help` is the blocking signal, and shows in `status` as its own state so a parent polling it sees the block without reading a file. `status` also carries `turn N of M`: with 4330's last-event age, that separates working from wedged from out of turns off one answer. Two bugs the new tests caught: a `tokio::fs::File` was dropped without flushing, so the report line was written to nothing, and the plain idle answer dropped the turn counter. Also documents `await_resume`'s third case — a closed channel with no send, which fails open the same as `Underway` — per argus on #4411. Refs #4403 --- CLAUDE.md | 14 +- docs/tools/subagent.md | 83 +- hive-subagent-mcp/Cargo.toml | 4 +- hive-subagent-mcp/README.md | 21 +- hive-subagent-mcp/src/lib.rs | 19 +- hive-subagent-mcp/src/main.rs | 17 +- hive-subagent-mcp/src/mcp.rs | 181 ++- hive-subagent-mcp/src/mcp_config.rs | 42 +- hive-subagent-mcp/src/session.rs | 1619 +++++++++++++++++++++++---- nix/agent-modules/mcp.nix | 13 +- 10 files changed, 1759 insertions(+), 254 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 80e867cf..3351cb97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,12 +101,14 @@ hand-maintained per-file tree drifts out of sync with the code. - **`hive-subagent-mcp/`** — per-agent claude-subagent runner daemon (`hive-subagent-daemon`); spawns nested claude sessions on request and serves `start`/`continue`/`status`/`interrupt` directly over - streamable-http (no stdio bridge). Independent of `hive-bash-mcp` (a - subagent is a much heavier capability than a bash command). No task - files — the daemon's only state is an in-memory map of currently-running - processes, live only as long as the process is; the actual claude - session survives a daemon restart independently (see `session.rs`'s - module doc). + streamable-http (no stdio bridge), plus a second subagent-facing route + carrying `goal_reached`/`need_help`. Independent of `hive-bash-mcp` (a + subagent is a much heavier capability than a bash command). A `start` + with a `goal` is a multi-turn run: the daemon re-prompts the subagent + toward the goal each time a turn ends, up to a per-session turn cap. No + task files — the daemon's state is in-memory only, live only as long as + the process is; the actual claude session survives a daemon restart + independently (see `session.rs`'s module doc). - **`hive-sh4re/`** — shared wire types (Agent / Manager request + response, `Message`, `Approval`, `HelperEvent`) used across the unix sockets. Host-admin-socket and hive-priv-socket wire types have been diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index 1cd7184e..7d38a057 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -24,11 +24,21 @@ infrastructure, not the agent-facing API. Served under the `subagent` MCP server (`mcp__subagent__`): `start`, `continue`, `status`, `interrupt`. +A second route on the same port serves the two tools a **subagent** calls +about its own run — `goal_reached` and `need_help`. It isn't part of the +`subagent` server an agent's own config points at; the daemon writes it +into each subagent's `--mcp-config` itself, under `subagent_control`. Two +routes rather than six tools on one, so that being able to report on a run +never carries the ability to start one: there's no route a subagent holds +that `start` is reachable from. + ## State In-memory only: what's running now, where each name's session lives, how -each name's last turn ended, and when each running turn last produced -output. All of it lives only as long as the daemon process does. A daemon +each name's last turn ended, when each running turn last produced output, +what each session is working toward, how far through its turn budget it +is, why its run stopped, and where it writes its report. All of it lives +only as long as the daemon process does. A daemon restart stops whatever was running rather than adopting it. The durable record of a subagent's existence is claude's own on-disk session (`hive_claude::SessionStore`), which `continue` reattaches to independent @@ -40,14 +50,68 @@ after a restart has to re-supply `dir` when the session lives anywhere other than the daemon's own working directory — and a restart is the situation you reach for `continue` in most often. +## Goals, and turns toward them + +`start` takes an optional `goal`. Without one a session is a single turn, +exactly as it always was. With one, the daemon keeps the session going: +when a turn ends and nothing has said to stop, it starts another turn +re-prompting the subagent toward that goal, quoting it verbatim and saying +which turn of the budget this is. `max_turns` caps that, defaulting to +**5**. + +`status` reports `Turn N of M` for such a session in every state it +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, and each is recorded distinctly, reported by +`status`, and appended to the one todo the daemon pushes when the run ends: + +- **the turn ended and there was no goal** — the single-turn case; +- **`goal_reached`**, which the subagent calls itself; +- **`need_help`**, likewise; +- **the turn cap**, which says so rather than stopping quietly: the todo + states that the harness limit was reached and the goal was never + reported reached, so the work stopped where it had got to. + +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. + +When the session was told where its report goes — `start`'s `report_file`, +or the path the subagent names when it signals — the stop reason is +appended to that file as well, so the artifact you were going to read +anyway also says how the run ended. Nothing is inferred: with no path +given, no file is touched. + +## `goal_reached` is a label, not a gate + +Both signals stop the continuation and **extend** the done message. Extend, +not replace: the turn's observed end and the reason the run stopped are +different facts, and the second never stands in for the first. + +`goal_reached` is **self-reported**, by a subagent that has just been +re-prompted with "you haven't reached the goal" — which is precisely the +incentive to claim it. It's the same failure class as a build report +asserting "done, tests pass": a claim about an artifact, not the artifact. +Nothing in this daemon treats it as verification, and every surface that +renders it says so. Read the diff and the gate output regardless. + +`need_help` is the blocking signal. It stops the run and shows up in +`status` as its own state — blocked, with the subagent's reason — so a +parent that polls `status` sees the block without reading anything else. +`continue` is how you answer it. + ## Is it working, or is it wedged? `status` reporting **running** says a process is tracked, which a wedged subagent satisfies as fully as a busy one. A running answer therefore carries the age of that turn's last event too: seconds means it's working, -an age climbing into the minutes with no end-of-turn todo means it's -stuck. That one number replaces inferring the same thing from `ps` output -and CPU-time deltas. +an age climbing into the minutes means it's stuck. That one number +replaces inferring the same thing from `ps` output and CPU-time deltas. +It resets at each turn's spawn, so on a goal run it describes the turn in +flight rather than the run — which is what you want, since a run that's +making progress spends several perfectly healthy minutes. Every line the subagent's `claude` process writes bumps the timestamp — stream-json events, plain stdout chatter and stderr alike — and what the @@ -135,10 +199,11 @@ Own systemd unit, defined alongside the other per-agent MCP daemons in ## MCP servers available to a subagent -A subagent runs with `--strict-mcp-config` and no `--mcp-config` by -default — zero MCP servers, full stop; it falls back to claude's own -native tools (`Bash`, `WebFetch`, etc.), not the parent's `mcp__bash__*` / -`mcp__hyperhive__*` surface. Nothing implicit reaches it: the built-in +A subagent runs with `--strict-mcp-config` and, by default, exactly one +MCP server: the two-tool `subagent_control` route above. It otherwise +falls back to claude's own native tools (`Bash`, `WebFetch`, etc.), not +the parent's `mcp__bash__*` / `mcp__hyperhive__*` surface. Nothing +implicit reaches it: the built-in hyperhive surface (todos/messaging) isn't an `extraMcpServers` entry at all, and the automatically injected `bash`/`subagent` entries default to excluded too (a subagent can't spawn hive-bash tasks or its own nested subagents diff --git a/hive-subagent-mcp/Cargo.toml b/hive-subagent-mcp/Cargo.toml index 025735ea..ca7f915a 100644 --- a/hive-subagent-mcp/Cargo.toml +++ b/hive-subagent-mcp/Cargo.toml @@ -33,7 +33,9 @@ tokio = { workspace = true, features = ["test-util"] } # `hive-subagent-daemon` — long-running per-agent claude-subagent runner. # Independent of `hive-bash-mcp` (own crate, own binary, own MCP server) — # see lib.rs's module doc for why. Serves its MCP tools (`start`/ -# `continue`/`interrupt`) directly over streamable-http — no stdio bridge. +# `continue`/`status`/`interrupt`, plus the subagent-facing +# `goal_reached`/`need_help` route) directly over streamable-http — no +# stdio bridge. [[bin]] name = "hive-subagent-daemon" path = "src/main.rs" diff --git a/hive-subagent-mcp/README.md b/hive-subagent-mcp/README.md index 27ae605b..8f78f226 100644 --- a/hive-subagent-mcp/README.md +++ b/hive-subagent-mcp/README.md @@ -2,9 +2,10 @@ Per-agent daemon (`hive-subagent-daemon`) that spawns nested headless `claude` sessions on request and serves the tool surface -(`start`/`continue`/`interrupt`) directly over streamable-http. No -stdio bridge, no per-turn respawn — claude reconnects to the same -stable URL every turn. +(`start`/`continue`/`status`/`interrupt`, plus a separate +subagent-facing `goal_reached`/`need_help` route) directly over +streamable-http. No stdio bridge, no per-turn respawn — claude +reconnects to the same stable URL every turn. Independent of `hive-bash-mcp` — a subagent spawns a full nested `claude` session, a much heavier capability than a bash command, worth @@ -18,10 +19,12 @@ own lib (`src/lib.rs`): - **`session.rs`** — the actual claude-facing logic: `Claude::spawn` + `RunningClaude::wait`/`cancel_handle` (not `InfiniteSession::run`, which has no cancel handle to reach in — see the module doc for the - v1 scope this trades away), the in-memory `running` map that's the - _only_ state this daemon keeps (no task files — a restart stops - whatever's running; the actual claude session is the durable store, - found again by name via `hive_claude::SessionStore`). -- **`mcp.rs`** — the `rmcp` tool router (`start`/`continue`/`interrupt`) - - `serve_http`. + v1 scope this trades away), the turn-continuation loop a `goal` + switches on, and the in-memory maps that are the _only_ state this + daemon keeps (no task files — a restart stops whatever's running; + the actual claude session is the durable store, found again by name + via `hive_claude::SessionStore`). +- **`mcp.rs`** — the `rmcp` tool routers (the parent's + `start`/`continue`/`status`/`interrupt` on `/mcp`, the subagent's + `goal_reached`/`need_help` on `/signal/mcp`) + `serve_http`. - **`paths.rs`** — the in-agent todo-socket path. diff --git a/hive-subagent-mcp/src/lib.rs b/hive-subagent-mcp/src/lib.rs index 666088be..e317ec23 100644 --- a/hive-subagent-mcp/src/lib.rs +++ b/hive-subagent-mcp/src/lib.rs @@ -1,13 +1,18 @@ //! Library for `hive-subagent-daemon`: spawns nested claude sessions on -//! request and serves the `start`/`continue`/`interrupt` MCP tool surface -//! directly over streamable-http — no stdio bridge, no round-trip socket. -//! Independent of `hive-bash-mcp` — a subagent is a much heavier capability -//! than a bash command (a full nested `claude` process), worth its own -//! deployable/restartable unit rather than sharing one. +//! request and serves the `start`/`continue`/`status`/`interrupt` MCP tool +//! surface directly over streamable-http — no stdio bridge, no round-trip +//! socket. Independent of `hive-bash-mcp` — a subagent is a much heavier +//! capability than a bash command (a full nested `claude` process), worth +//! its own deployable/restartable unit rather than sharing one. +//! +//! A second route on the same listener serves `goal_reached`/`need_help` to +//! the *subagents*, which is how a run says it's done or stuck; see +//! [`mcp`]'s module doc for why that is a separate surface rather than two +//! more tools on the parent's. //! //! See [`session`]'s module doc for the actual design: no task files, no -//! restart recovery, no mid-turn compaction — the daemon's only state is an -//! in-memory `name -> Cancel` map, live only as long as the process is. +//! restart recovery, no mid-turn compaction — the daemon's only state is a +//! handful of in-memory maps, live only as long as the process is. pub mod mcp; pub mod mcp_config; diff --git a/hive-subagent-mcp/src/main.rs b/hive-subagent-mcp/src/main.rs index 40f5fdc3..e4de7422 100644 --- a/hive-subagent-mcp/src/main.rs +++ b/hive-subagent-mcp/src/main.rs @@ -1,7 +1,9 @@ //! `hive-subagent-daemon` binary — spawns nested claude sessions on -//! request and serves the `start`/`continue`/`interrupt` MCP tool surface -//! directly over streamable-http on `--http ` — no stdio bridge, no -//! separate bin claude has to respawn every turn. +//! request and serves the `start`/`continue`/`status`/`interrupt` MCP tool +//! surface directly over streamable-http on `--http ` — no stdio +//! bridge, no separate bin claude has to respawn every turn. The same +//! address also carries the subagent-facing `goal_reached`/`need_help` +//! route, which is the only place the daemon can learn its own URL from. use std::sync::Arc; @@ -42,7 +44,14 @@ async fn main() -> Result<()> { "hive-subagent-daemon starting" ); - let state = Arc::new(hive_subagent_mcp::session::State::new(todo_socket)); + // The one place the subagent-facing signal URL can come from: the + // address this process was told to listen on. Anything else would be a + // guess at the deployment's own port assignment. + let signal_url = format!("http://{}{}", cli.http, hive_subagent_mcp::mcp::SIGNAL_PATH); + let state = Arc::new(hive_subagent_mcp::session::State::new( + todo_socket, + signal_url, + )); // Serve the MCP tools over streamable-http forever. No background poll // loop to start — unlike the bash daemon's task-file queue, `start`/ diff --git a/hive-subagent-mcp/src/mcp.rs b/hive-subagent-mcp/src/mcp.rs index fd5b7bef..2edf0809 100644 --- a/hive-subagent-mcp/src/mcp.rs +++ b/hive-subagent-mcp/src/mcp.rs @@ -1,6 +1,12 @@ //! The MCP tool surface: `start` / `continue` / `status` / `interrupt`, //! served directly over streamable-http — no stdio bridge, no round-trip //! socket. +//! +//! Two surfaces, two routes. `/mcp` is the parent's: the four tools above. +//! `/signal/mcp` is the *subagent's*, and carries `goal_reached` and +//! `need_help` only — it is what a subagent is handed in its own +//! `--mcp-config` (see [`crate::mcp_config`]), so the ability to report on +//! its own run can't be the ability to spawn a nested one. use std::sync::Arc; @@ -57,12 +63,64 @@ struct StartArgs { /// this daemon tracks in memory. #[serde(default)] dir: Option, + /// What this subagent is working *toward*, in its own words — set it and + /// the daemon keeps giving it turns until it says it's done, says it's + /// stuck, or runs out. Omit it and the session is a single turn, exactly + /// as before. Written for the subagent to read: it's quoted back at it + /// verbatim at the start of every continued turn, so "get `cargo clippy + /// --workspace` to pass with no warnings" continues far better than "fix + /// the lints". + #[serde(default)] + goal: Option, + /// How many turns the continuation may spend before the harness stops it + /// itself. Default 5. Only meaningful alongside `goal` — without one + /// there's nothing to re-prompt toward, so nothing to cap. The cap + /// bounds *unattended* re-prompting: a `continue` you issue yourself + /// starts the allowance over. + #[serde(default)] + max_turns: Option, + /// Where the instructions in `prompt_file` told this subagent to write + /// its report. The daemon appends the run's stop reason to that file when + /// the run ends, so the artifact you were going to read anyway also says + /// how it stopped. Nothing is inferred: if you don't pass it (and the + /// subagent doesn't name it when it signals), no file is touched. + #[serde(default)] + report_file: Option, } fn default_trigger() -> String { "Carry out the task described in your instructions.".to_owned() } +#[derive(Debug, Deserialize, JsonSchema)] +struct GoalReachedArgs { + /// Your own session name — the one your brief and your continuation + /// prompts address you by. + name: String, + /// Optionally, what you did. It's shown to whoever spawned you. + #[serde(default)] + msg: Option, + /// Optionally, the path you wrote your report to, so the stop reason + /// gets appended to it. + #[serde(default)] + report_file: Option, +} + +#[derive(Debug, Deserialize, JsonSchema)] +struct NeedHelpArgs { + /// Your own session name — the one your brief and your continuation + /// prompts address you by. + name: String, + /// What is blocking you, specifically enough for someone else to act on + /// it. This is the whole content of the signal, which is why it's + /// required. + msg: String, + /// Optionally, the path you wrote your report to, so the stop reason + /// gets appended to it. + #[serde(default)] + report_file: Option, +} + #[derive(Debug, Deserialize, JsonSchema)] struct ContinueArgs { /// The existing session's name (from a prior `start`). @@ -127,17 +185,26 @@ impl SubagentMcp { first (real fresh start, not a silent resume); a *currently running* one is \ refused. Runs unattended — every tool-call permission prompt is pre-approved rather \ than interactively confirmed — with its MCP server set fixed to what this daemon \ - configures for it. See the `base:claude-subagents` skill for when to reach for this." + configures for it. Pass `goal` to make this a multi-turn run: the daemon re-prompts \ + the subagent toward that goal each time a turn ends, up to `max_turns` (default 5), \ + stopping early when the subagent reports the goal reached or asks for help. Whichever \ + way it stops, one todo is pushed at the end and `status` says which. See the \ + `base:claude-subagents` skill for when to reach for this." )] fn start(&self, Parameters(args): Parameters) -> String { match session::start( &self.state, - &args.name, - args.model, - args.effort, - &args.prompt_file, - args.trigger, - args.dir.as_deref(), + session::StartRequest { + name: args.name, + model: args.model, + effort: args.effort, + prompt_file: args.prompt_file, + trigger: args.trigger, + dir: args.dir, + goal: args.goal, + max_turns: args.max_turns, + report_file: args.report_file, + }, ) { Ok(msg) => msg, Err(e) => format!("start error: {e:#}"), @@ -193,7 +260,9 @@ impl SubagentMcp { never launches a process, unlike `continue`. The answer says what state it found \ and what to do about it. For a running one it also reports how long since that turn \ last produced any output, which is how you tell a subagent that's working from one \ - that has wedged without resorting to `ps`." + that has wedged without resorting to `ps`. For a session started with a `goal` it \ + reports which turn of the budget it is on, and — once the run has stopped — why it \ + stopped: goal reported reached, blocked and needing help, or out of turns." )] fn status(&self, Parameters(args): Parameters) -> String { match session::status(&self.state, &args.name, args.dir.as_deref()) { @@ -206,10 +275,76 @@ impl SubagentMcp { #[tool_handler] impl ServerHandler for SubagentMcp {} +/// The surface a *subagent* gets, served on its own route: two tools, both +/// of them ways for a running subagent to say how its own run should end. +/// Separate handler rather than two more tools on [`SubagentMcp`] so the +/// split is structural — there is no route a subagent holds that `start` is +/// reachable from. +#[derive(Clone)] +struct SubagentSignalMcp { + state: Arc, +} + +#[tool_router] +impl SubagentSignalMcp { + #[tool( + description = "Report that you have reached the goal you were given. Stops the harness \ + from starting another turn to re-prompt you toward it, and extends the \"subagent \ + done\" message your parent gets with what you say here. This records a claim, not a \ + result: whoever spawned you reads the diff and the gate output regardless, so \ + calling it does not make unfinished work finished. Call it when the goal is actually \ + met — otherwise keep working, or call `need_help` if you can't proceed." + )] + fn goal_reached(&self, Parameters(args): Parameters) -> String { + match session::goal_reached( + &self.state, + &args.name, + args.msg, + args.report_file.as_deref(), + ) { + Ok(msg) => msg, + Err(e) => format!("goal_reached error: {e:#}"), + } + } + + #[tool( + description = "Report that you cannot proceed, and why. Stops the harness from starting \ + another turn to re-prompt you toward your goal, marks your session as blocked so \ + whoever spawned you sees it in `status` without reading any file, and extends the \ + \"subagent done\" message with your reason. Use it for a genuine block — a missing \ + credential, a decision that isn't yours, an instruction that contradicts what you \ + found — not for work that is merely hard. Say enough that someone else can act on it." + )] + fn need_help(&self, Parameters(args): Parameters) -> String { + match session::need_help( + &self.state, + &args.name, + args.msg, + args.report_file.as_deref(), + ) { + Ok(msg) => msg, + Err(e) => format!("need_help error: {e:#}"), + } + } +} + +#[tool_handler] +impl ServerHandler for SubagentSignalMcp {} + +/// Path the subagent-facing signal surface is served at, and the tail of the +/// URL [`crate::session::State`] hands to every subagent it spawns. Kept +/// here, next to the route that answers it, so the two can't drift. +pub const SIGNAL_PATH: &str = "/signal/mcp"; + /// Run the MCP server over HTTP (rmcp streamable-http transport) on `addr`. /// Loopback-only bind, one long-lived session — same shape as the bash and /// matrix daemons' own `serve_http`. /// +/// Two routes off one listener: `/mcp` for the parent's four tools, and +/// [`SIGNAL_PATH`] for the subagent's two. Separate session managers because +/// they're separate MCP servers to separate clients — the parent's harness +/// on one, each subagent's own claude on the other. +/// /// # Errors /// /// Returns an error if the listener cannot bind `addr` or the HTTP server @@ -218,23 +353,41 @@ pub async fn serve_http(addr: std::net::SocketAddr, state: Arc) -> anyhow use rmcp::transport::streamable_http_server::{ StreamableHttpServerConfig, StreamableHttpService, session::local::LocalSessionManager, }; - let mut session_manager = LocalSessionManager::default(); // A subagent turn can run considerably longer than a bash command — // same 24h keep-alive rationale as the bash/matrix daemons. - session_manager.session_config.keep_alive = Some(std::time::Duration::from_hours(24)); - let session_manager = std::sync::Arc::new(session_manager); + let manager = || { + let mut session_manager = LocalSessionManager::default(); + session_manager.session_config.keep_alive = Some(std::time::Duration::from_hours(24)); + std::sync::Arc::new(session_manager) + }; + let parent_state = Arc::clone(&state); let service = StreamableHttpService::new( move || { Ok(SubagentMcp { + state: Arc::clone(&parent_state), + }) + }, + manager(), + StreamableHttpServerConfig::default(), + ); + let signal_service = StreamableHttpService::new( + move || { + Ok(SubagentSignalMcp { state: Arc::clone(&state), }) }, - session_manager, + manager(), StreamableHttpServerConfig::default(), ); - let app = axum::Router::new().nest_service("/mcp", service); + let app = axum::Router::new() + .nest_service("/mcp", service) + .nest_service(SIGNAL_PATH, signal_service); let listener = tokio::net::TcpListener::bind(addr).await?; - tracing::info!(%addr, "serving hive-subagent MCP over streamable-http at /mcp"); + tracing::info!( + %addr, + signal = SIGNAL_PATH, + "serving hive-subagent MCP over streamable-http at /mcp" + ); axum::serve(listener, app).await?; Ok(()) } diff --git a/hive-subagent-mcp/src/mcp_config.rs b/hive-subagent-mcp/src/mcp_config.rs index 74520314..51931709 100644 --- a/hive-subagent-mcp/src/mcp_config.rs +++ b/hive-subagent-mcp/src/mcp_config.rs @@ -1,35 +1,61 @@ //! Builds a subagent's own filtered `--mcp-config`: only //! `hyperhive.extraMcpServers` entries with `availableToSubagents = true` //! (see `hive_agent_sock::extra_mcp::ExtraMcpServer::available_to_subagents`) ever reach -//! a subagent's claude invocation. Everything else — the built-in hyperhive +//! a subagent's claude invocation, plus this daemon's own two-tool signal +//! surface. Everything else — the built-in hyperhive //! surface (todos/messaging), the automatically injected `bash` and `subagent` //! entries — stays unreachable by construction: none of those default to //! opted in, and the built-in surface isn't an `extraMcpServers` entry at //! all, so there's no name for an operator to opt it in under even if they //! wanted to. +//! +//! The signal surface is the one server a subagent always gets. It is a +//! *different route* on this daemon's listener from the one the parent +//! uses, serving `goal_reached` and `need_help` and nothing else — so +//! "a subagent can say it is done or stuck" never widens into "a subagent +//! can spawn subagents", which is what handing it the parent's route would +//! have meant. use std::path::PathBuf; /// Filename the rendered config lives at, under [`crate::paths::harness_dir`]. const CONFIG_FILE: &str = "subagent-mcp-config.json"; -/// Render the subagent-eligible extra-MCP servers to a `--mcp-config` file, -/// returning its path — or `None` when no entry opts in (or the render/write -/// fails), so [`hive_claude::Config::mcp_config`] stays unset and the -/// subagent gets literally zero MCP servers, the same default as before this -/// toggle existed. Re-rendered on every call (cheap: a filter plus a small +/// Name the signal surface appears under in a subagent's own MCP config, +/// and therefore the prefix its tools are called by +/// (`mcp__subagent_control__goal_reached`). +const SIGNAL_SERVER: &str = "subagent_control"; + +/// Render a subagent's `--mcp-config` file, returning its path — or `None` +/// when there is nothing to put in it (or the render/write fails), so +/// [`hive_claude::Config::mcp_config`] stays unset and the subagent gets +/// literally zero MCP servers. +/// +/// `signal_url` is this daemon's own `goal_reached`/`need_help` route; it is +/// always included when given, since a subagent that can't say it's done or +/// stuck is exactly the one the turn cap has to stop on its behalf. `None` +/// reproduces the pre-continuation shape — only the opted-in extras, and no +/// file at all when none opt in. +/// +/// Re-rendered on every call (cheap: a filter plus a small /// file write) rather than cached once at daemon startup, so a config change /// takes effect on this subagent's next `start`/`continue` without needing /// the daemon itself restarted. #[must_use] -pub fn build() -> Option { +pub fn build(signal_url: Option<&str>) -> Option { let state_dir = crate::paths::state_dir(); - let servers: serde_json::Map = + let mut servers: serde_json::Map = hive_agent_sock::extra_mcp::load_extra_mcp() .into_iter() .filter(|(_, spec)| spec.available_to_subagents()) .map(|(name, spec)| (name, spec.to_json_entry(&state_dir))) .collect(); + if let Some(url) = signal_url { + servers.insert( + SIGNAL_SERVER.to_owned(), + serde_json::json!({ "type": "http", "url": url }), + ); + } if servers.is_empty() { return None; } diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index e2747ae7..18868a2c 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -1,14 +1,14 @@ -//! The claude-facing half of this daemon: spawn a subagent turn, track it -//! only while it's alive, and push exactly one todo when it ends — saying -//! whether it finished or was killed. +//! 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. //! //! **No task files, no restart recovery.** The daemon's only state is an -//! in-memory `name -> Option` map (see `State`'s own doc for what -//! the `None`/`Some` split is for), a `name -> dir` memory so `start`'s -//! `dir` doesn't have to be repeated on every later `continue`/`status` -//! (`State::resolve_dir`), and a `name -> last_event_at` liveness clock -//! (`State::note_event`) — all of it living for exactly as long as the -//! process is. A daemon restart means whatever was running gets killed +//! in-memory `name -> Option` map (see `State`'s own doc for the +//! rest of the maps and for what the `None`/`Some` split is for) — all of +//! it living for exactly as long as the process is. A daemon restart means +//! whatever was running gets killed //! with it (`tokio`'s own child-process drop semantics), not adopted, and //! the remembered `dir` is gone too. The durable record of a subagent's //! existence is `hive_claude::SessionStore` — claude's own on-disk @@ -28,6 +28,22 @@ //! a missed resume as its own error, naming the directory searched //! (`classify_end`). See `docs/tools/subagent.md`. +//! **One `start` can be more than one turn.** With a `goal` set, a turn that +//! ends without a stop signal is followed by another re-prompting the +//! subagent toward it, up to `max_turns` (default five). The loop lives in +//! `spawn_and_track`'s background task, so both tool-call contracts are +//! unchanged and only one todo is pushed, when the *run* stops. No goal +//! means what it always did: one turn, one todo. +//! +//! **A subagent's own stop signals are labels, not gates.** `goal_reached` +//! and `need_help` stop the loop and extend the done message; neither +//! verifies anything. `goal_reached` is self-reported by a subagent that has +//! just been re-prompted with "you have not reached the goal", which is +//! exactly the incentive to claim it — the same failure class as a build +//! report asserting "done, tests pass". Everything here treats it as a claim +//! about the work, never as the work, and says so. See +//! `docs/tools/subagent.md`. + //! **A killed turn is not a finished turn.** A child that died on a signal //! arrives as a `hive_claude::Error::Exit` carrying its `ExitStatus`, so the //! "how" is there to be read: `classify_end` takes the signal out of it and @@ -72,6 +88,68 @@ use tokio::sync::oneshot; /// any of those runs, while still bounding the one case that reaches it. const RESUME_GRACE: Duration = Duration::from_secs(5); +/// How many turns a goal-continued session gets before the harness stops it +/// itself. Five is the number the feature was specified with, not one tuned +/// here: enough for a bounded batch to converge, short enough that a +/// subagent which has misunderstood its goal can't re-attempt it forever on +/// someone else's budget. `start`'s `max_turns` overrides it per session, +/// which is where a caller that genuinely needs a longer leash says so. +const DEFAULT_MAX_TURNS: u32 = 5; + +/// Why a session's turn continuation stopped — recorded per name, reported +/// 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. +#[derive(Debug, Clone, PartialEq, Eq)] +enum StopReason { + /// The turn ended and there was no goal to continue toward — the + /// single-turn shape every session had before continuation existed. + Done, + /// The subagent called `goal_reached`. **Self-reported.** It records + /// that the subagent claimed the goal, never that the goal was met. + GoalReached(Option), + /// The subagent called `need_help`: it can't proceed, and says why. + NeedHelp(String), + /// `turns` turns ran and the goal was never reported reached. + TurnCap { turns: u32 }, +} + +impl StopReason { + /// The one sentence that says why, shared by the todo extension and the + /// report-file line so a parent reading either sees the same words. + /// + /// `GoalReached` carries its "self-reported" caveat in the sentence + /// itself rather than leaving it to whichever surface renders it: the + /// caveat is the load-bearing half of that claim, and a surface that + /// forgot to add it would read as verification. + fn sentence(&self) -> String { + match self { + Self::Done => "its turn ended and there was no goal to continue toward".to_owned(), + Self::GoalReached(msg) => { + let said = msg + .as_deref() + .map_or_else(String::new, |m| format!(": {m}")); + format!( + "the subagent reported its goal reached{said} — self-reported, not verified, \ + so read what it actually changed before acting on it" + ) + } + Self::NeedHelp(msg) => { + format!("the subagent called `need_help` and can't proceed: {msg}") + } + Self::TurnCap { turns } => format!( + "the harness turn limit was reached ({turns} turns) without the goal ever being \ + reported reached, so the work stopped where it had got to" + ), + } + } +} + /// How a subagent's turn ended, as far as the daemon can tell from what /// [`hive_claude::RunningClaude::wait`] returned. /// @@ -185,10 +263,10 @@ fn searched_location(config: &Config) -> Option { )) } -/// This daemon's whole state: which names currently have a live process or -/// a reservation in flight, and where to push the completion todo. -/// `Arc`-wrapped so the background task that drives a turn to completion -/// can outlive the tool call that started it. +/// This daemon's whole state: which names have a live process or a +/// reservation in flight, what each is working toward and how it stopped, +/// and where to push the completion todo. `Arc`-wrapped so the background +/// task driving a run outlives the tool call that started it. /// /// The map value is `Option`: `None` means `name` is reserved for /// an in-flight `start`/`continue` that hasn't reached a confirmed @@ -220,24 +298,60 @@ pub struct State { dirs: Mutex>, killed: Mutex>, last_event: Mutex>, + /// Turn continuation's three records. Unlike the four maps above they + /// deliberately **outlive the turn** — a stop reason that vanished with + /// the process it described would be unreadable by the time anyone + /// asked — so they're cleared by the next `start` under the same name, + /// not by a turn ending. + goals: Mutex>, + stops: Mutex>, + reports: Mutex>, socket: PathBuf, + /// Where a subagent reaches this daemon's own `goal_reached`/`need_help` + /// surface. It lives here because the daemon can only learn it from its + /// own `--http` argument — deriving it from a convention would be the + /// same inference the report path is careful not to make. + signal_url: String, +} + +/// What a session is being continued toward, and how far through its turn +/// budget it is. Present only for a session `start`ed with a `goal`; its +/// absence is what makes a session single-turn. +struct GoalState { + /// Verbatim from `start` — re-prompted at the subagent each turn rather + /// than paraphrased, since the caller wrote it for the subagent to read. + goal: String, + /// The cap this session runs under (`start`'s `max_turns`). + max_turns: u32, + /// Turns started so far, counting the first — so the very first turn is + /// `1 of max_turns`, not `0`. + turn: u32, } impl State { + /// `signal_url` is the streamable-http endpoint a subagent's own claude + /// reaches `goal_reached`/`need_help` on — this daemon's `--http` + /// address with the signal route appended (see `crate::mcp::serve_http`). #[must_use] - pub fn new(socket: PathBuf) -> Self { + pub fn new(socket: PathBuf, signal_url: String) -> Self { Self { running: Mutex::new(HashMap::new()), dirs: Mutex::new(HashMap::new()), killed: Mutex::new(HashMap::new()), last_event: Mutex::new(HashMap::new()), + goals: Mutex::new(HashMap::new()), + stops: Mutex::new(HashMap::new()), + reports: Mutex::new(HashMap::new()), socket, + signal_url, } } /// `Some(true)` — a live process is tracked, interruptible. `Some(false)` - /// — reserved for an in-flight start/continue, not yet a confirmed - /// spawn. `None` — nothing tracked under `name` at all. + /// — the name is claimed but no process is confirmed under it: an + /// in-flight start/continue that hasn't spawned yet, or the gap between + /// one continued turn's child exiting and the next one spawning (see + /// `between_turns`). `None` — nothing tracked under `name` at all. fn occupancy(&self, name: &str) -> Option { self.running .lock() @@ -332,13 +446,14 @@ impl State { /// is an elapsed age, which a clock adjustment must not be able to /// distort into a stall that never happened. /// - /// Also called once by `spawn_and_track` the moment the child exists, so - /// the clock starts at the spawn rather than at the first line. Without - /// that seed a subagent that wedged before emitting anything at all - /// would report no age forever — the one case where an age is most worth - /// having. The value therefore reads as "how long since the daemon last - /// heard anything from this child", counting the spawn itself as the - /// first thing it heard. + /// Also called by `spawn_and_track` the moment each turn's child exists, + /// so the clock starts at the spawn rather than at the first line. + /// Without that seed a subagent that wedged before emitting anything at + /// all would report no age forever — the one case where an age is most + /// worth having. The value therefore reads as "how long since the daemon + /// last heard anything from this child", counting the spawn itself as + /// the first thing it heard. A continued run re-seeds it per turn, for + /// the same reason: the age describes the turn in flight, not the run. fn note_event(&self, name: &str) { self.last_event .lock() @@ -408,6 +523,180 @@ impl State { .cloned(), } } + + /// Upgrade `name`'s `None` reservation to a real, interruptible process. + /// Same key as the reservation, so there is no window in which `name` + /// reads as unoccupied between the two. + fn track(&self, name: &str, cancel: Cancel) { + self.running + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(name.to_owned(), Some(cancel)); + } + + /// Drop `name`'s cancel handle but keep the name claimed — the moment + /// between a continued turn's child exiting and its successor being + /// spawned. Without it the name would read as free mid-loop and a + /// concurrent `start` could take it out from under the continuation; + /// with it, `status` says "starting" and `interrupt` says "still + /// starting, retry shortly", both of which are true of the sub-second + /// gap it covers. + fn between_turns(&self, name: &str) { + self.running + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(name.to_owned(), None); + } + + /// Set (or clear) `name`'s goal and start its turn budget over at turn + /// one. Called by `start` only: the goal is a property of the session + /// being created, and a later `continue` re-prompts toward whatever + /// `start` set rather than redefining it. + fn set_goal(&self, name: &str, goal: Option, max_turns: u32) { + let mut goals = self.goals.lock().unwrap_or_else(PoisonError::into_inner); + match goal { + Some(goal) => { + goals.insert( + name.to_owned(), + GoalState { + goal, + max_turns, + turn: 1, + }, + ); + } + None => { + goals.remove(name); + } + } + } + + /// Put `name` back at turn one, keeping whatever goal `start` set. A + /// `continue` is the parent's own deliberate turn, and the cap exists to + /// bound *unattended* continuation — so its allowance starts over rather + /// than a capped session being permanently un-continuable. The parent + /// was always the authority on whether more turns are worth spending. + fn restart_turns(&self, name: &str) { + if let Some(goal) = self + .goals + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get_mut(name) + { + goal.turn = 1; + } + } + + /// `(turn, max_turns)` for `name`. `None` for a session with no goal, + /// which has no turn budget to be partway through — reporting `1 of 1` + /// there would invent a cap that isn't enforced. + fn turns(&self, name: &str) -> Option<(u32, u32)> { + self.goals + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(name) + .map(|g| (g.turn, g.max_turns)) + } + + /// Record why `name` stopped. Idempotent by design: the subagent's own + /// signal lands here mid-turn and the loop re-records the same reason at + /// the turn's end, so both paths can write without checking. + fn record_stop(&self, name: &str, stop: StopReason) { + self.stops + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(name.to_owned(), stop); + } + + fn stop_reason(&self, name: &str) -> Option { + self.stops + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(name) + .cloned() + } + + /// Forget why `name` last stopped — called when a new turn is starting, + /// since the record describes the run before it and would otherwise make + /// a session that has since been given another turn still read as + /// blocked or out of turns. + fn clear_stop(&self, name: &str) { + self.stops + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(name); + } + + /// Remember where `name` writes its report, so a stop reason can be + /// appended to the artifact a parent already reads rather than living + /// only in a todo. `None` leaves whatever was remembered alone, so a + /// signal tool that doesn't name a path doesn't erase the one `start` + /// gave. + /// + /// Never inferred: `start` carries what the brief named, and the signal + /// tools carry where the subagent says it actually wrote. A daemon that + /// derived this from a path convention would be guessing about someone + /// else's layout. + fn set_report_file(&self, name: &str, path: Option<&str>) { + if let Some(path) = path { + self.reports + .lock() + .unwrap_or_else(PoisonError::into_inner) + .insert(name.to_owned(), PathBuf::from(path)); + } + } + + /// Forget `name`'s report path — a fresh `start` under the same name is + /// a different piece of work, and inheriting the last one's artifact + /// path would append its stop reason to a file this run never wrote. + fn clear_report_file(&self, name: &str) { + self.reports + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(name); + } + + fn report_file(&self, name: &str) -> Option { + self.reports + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(name) + .cloned() + } + + /// What the continuation loop does after a turn that ran to completion: + /// stop with a reason, or spend another turn re-prompting toward the + /// goal. Advances the turn counter itself, since deciding to continue + /// and consuming a turn are the same act. + /// + /// A signal the subagent raised mid-turn is already in `stops` and wins + /// outright — that is what "both stop goal continues" means, and it's + /// checked before the goal so a `goal_reached` on the last allowed turn + /// reads as reached rather than as capped. + fn plan_after_turn(&self, name: &str) -> Continuation { + if let Some(stop) = self.stop_reason(name) { + return Continuation::Stop(stop); + } + let mut goals = self.goals.lock().unwrap_or_else(PoisonError::into_inner); + let Some(state) = goals.get_mut(name) else { + return Continuation::Stop(StopReason::Done); + }; + if state.turn >= state.max_turns { + return Continuation::Stop(StopReason::TurnCap { turns: state.turn }); + } + state.turn += 1; + Continuation::Continue { + prompt: continuation_prompt(name, &state.goal, state.turn, state.max_turns), + } + } +} + +/// `plan_after_turn`'s answer: the loop either stops with a reason to +/// report, or has the next turn's prompt ready to spawn against. +#[derive(Debug, PartialEq, Eq)] +enum Continuation { + Stop(StopReason), + Continue { prompt: String }, } /// A caller-chosen name, validated the same way `hive-bash-mcp`'s task ids @@ -448,16 +737,23 @@ fn subagent_otel_attrs(name: &str) -> String { /// property is `strict_mcp_config: true` with no ambient MCP discovery, not /// an unconditional absence of `--mcp-config`: a subagent gets exactly the /// `hyperhive.extraMcpServers` entries an operator has explicitly opted in -/// via `availableToSubagents = true` (`crate::mcp_config::build`), nothing -/// implicit and nothing more. With no entry opted in — the default — that -/// resolves to `None` and the invocation is unchanged from before this -/// toggle existed: zero MCP servers, full stop. +/// via `availableToSubagents = true` (`crate::mcp_config::build`), plus this +/// daemon's own two-tool signal surface when `signal_url` is given — nothing +/// implicit and nothing more. +/// +/// `signal_url` is what makes `goal_reached`/`need_help` callable at all: a +/// subagent reaches them over the same streamable-http listener its parent +/// uses, on a route that serves those two tools and nothing else, so being +/// able to say "I'm done" never carries the ability to spawn a subagent of +/// its own. `None` — which only `status` passes, building a config purely to +/// resolve the session store — leaves the surface out entirely. fn build_config( name: &str, model: Option, effort: Option, prompt_file: Option<&str>, dir: Option<&str>, + signal_url: Option<&str>, ) -> Config { let mut extra_args = vec!["--dangerously-skip-permissions".to_owned()]; if let Some(path) = prompt_file { @@ -468,7 +764,7 @@ fn build_config( model, effort: Some(effort.unwrap_or_else(|| "medium".to_owned())), cwd: dir.map(PathBuf::from), - mcp_config: crate::mcp_config::build(), + mcp_config: crate::mcp_config::build(signal_url), strict_mcp_config: true, extra_args, env: vec![( @@ -489,12 +785,44 @@ fn build_store(config: &Config) -> std::io::Result { )) } +/// Everything a `start` needs, as one struct rather than a parameter list: +/// the call already carried six mostly-optional values before goals were +/// added, and nine positional arguments is both unreadable at the call site +/// and a lint. +pub struct StartRequest { + /// Session name — the tracking key and claude's own session title. + pub name: String, + pub model: Option, + pub effort: Option, + /// File holding the subagent's task instructions. + pub prompt_file: String, + /// The first turn's prompt. A goal, when given, is appended to it. + pub trigger: String, + /// Working directory for the session; `None` inherits the daemon's. + pub dir: Option, + /// What this session is being continued *toward*. `None` keeps the + /// pre-continuation shape: one turn, one todo, no re-prompting. + pub goal: Option, + /// Turn cap for the continuation, defaulting to `DEFAULT_MAX_TURNS`. + /// Ignored without a `goal`, which is what continuation continues + /// toward — there is nothing to re-prompt against otherwise. + pub max_turns: Option, + /// Where this session's brief told it to write its report, so the stop + /// reason can be appended to that artifact. Never inferred — see + /// `State::set_report_file`. + pub report_file: Option, +} + /// Start a fresh subagent under `name`. A prior *finished* session under /// the same name is archived first (so this is a real fresh start, not a /// silent resume of old history) — a *currently running* one is refused /// outright, since `hive_claude::InfiniteSession`'s own docs warn that two /// concurrent runs against the same name corrupt both. /// +/// With a `goal` set this starts a whole *run*, not a single turn: see +/// `spawn_and_track` for the continuation loop. The return is unchanged +/// either way — it reports the first turn's spawn, not the run's outcome. +/// /// # Errors /// /// A name already running, an invalid name, an archive failure, or the @@ -502,28 +830,34 @@ fn build_store(config: &Config) -> std::io::Result { /// case is the only one that can happen *after* commit-to-run, and it's /// exactly why nothing is registered in `running` until spawn actually /// succeeds. -pub fn start( - state: &Arc, - name: &str, - model: Option, - effort: Option, - prompt_file: &str, - trigger: String, - dir: Option<&str>, -) -> anyhow::Result { +pub fn start(state: &Arc, req: StartRequest) -> anyhow::Result { + let name = req.name.as_str(); validate_name(name)?; if !state.reserve(name) { anyhow::bail!("subagent `{name}` is already running — use `continue` or `interrupt`"); } // Only commit the remembered `dir` now that `reserve` has actually - // claimed `name` — see `resolve_dir`'s doc for why the order matters. - let dir = state.resolve_dir(name, dir); + // claimed `name` — see `resolve_dir`'s doc for why the order matters, + // and the same reasoning governs the three records below it. + let dir = state.resolve_dir(name, req.dir.as_deref()); + // A fresh start owns none of the previous run's records: its goal, its + // turn budget, why it stopped and where it wrote are all about work this + // call is deliberately replacing. + state.clear_stop(name); + state.clear_report_file(name); + state.set_report_file(name, req.report_file.as_deref()); + let max_turns = req.max_turns.unwrap_or(DEFAULT_MAX_TURNS).max(1); + state.set_goal(name, req.goal.clone(), max_turns); + let trigger = match req.goal.as_deref() { + None => req.trigger, + Some(goal) => format!("{}{}", req.trigger, goal_briefing(name, goal, max_turns)), + }; let result = start_reserved( state, name, - model, - effort, - prompt_file, + req.model, + req.effort, + &req.prompt_file, trigger, dir.as_deref(), ); @@ -546,7 +880,14 @@ fn start_reserved( trigger: String, dir: Option<&str>, ) -> anyhow::Result { - let config = build_config(name, model, effort, Some(prompt_file), dir); + let config = build_config( + name, + model, + effort, + Some(prompt_file), + dir, + Some(&state.signal_url), + ); let store = build_store(&config)?; if store.find_by_title(name).is_some() { tracing::info!( @@ -593,6 +934,9 @@ fn start_reserved( /// so, since a caller that never ran `status` and missed the todo would /// otherwise carry on from cut-off work believing it was finished work. /// +/// A goal session re-enters its continuation loop here with a fresh turn +/// budget — the cap bounds *unattended* re-prompting, not the parent's own. +/// /// # Errors /// /// An invalid name, one already running, `Claude::spawn` failing, or the @@ -617,6 +961,10 @@ pub async fn continue_( // Only commit the remembered `dir` now that `reserve` has actually // claimed `name` — see `resolve_dir`'s doc for why the order matters. let dir = state.resolve_dir(name, dir); + // A new turn supersedes why the last run stopped, and gives a + // goal-continued session its allowance back — see `restart_turns`. + state.clear_stop(name); + state.restart_turns(name); let (tx, rx) = oneshot::channel(); let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); let started = continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict); @@ -624,9 +972,10 @@ pub async fn continue_( state.release_reservation(name); return started; } - // Nothing to release on this path: a turn that ended inside the grace - // has already been through `finish_turn`, which clears the tracking the - // spawn put there. + // Nothing to release on this path either way: a turn that ended inside + // the grace has already been through `finish_turn`, which clears the + // tracking the spawn put there, and one that a goal carried into another + // turn still owns the name via `between_turns`. await_resume(rx).await?; started.map(|msg| note_resumed_after_kill(&msg, killed)) } @@ -642,6 +991,14 @@ pub async fn continue_( /// reported as started — which it is, with the end-of-turn todo left to say /// how it goes. /// +/// The third case is the channel closing with nothing ever sent — both the +/// sink and the background task dropping their `Arc` without calling +/// `settle`, which only a panic in the task can produce. That reads here as +/// "started", the same as `Underway`, and the fail-open is deliberate: the +/// one thing already known is that `Claude::spawn` returned a live pid, so +/// answering "the resume missed" would be a claim about the session that +/// nothing observed. The turn's real end still reaches the caller as a todo. +/// /// # Errors /// /// The turn's own failure message, verbatim: the caller asked claude to @@ -653,7 +1010,9 @@ async fn await_resume(rx: oneshot::Receiver) -> anyhow::Result<() // Everything else is a turn that started: it spoke (`Underway`), or // it ended on its own terms within the grace — `Complete`, or a // `Killed` that some concurrent `interrupt` asked for and whose todo - // says so — or it is still going when the grace runs out. + // says so — or it is still going when the grace runs out, or nobody + // ever sent at all, which only a panicked task produces and which + // this deliberately fails open on. See the doc above. _ => Ok(()), } } @@ -684,7 +1043,7 @@ fn continue_reserved( dir: Option<&str>, verdict: &VerdictTx, ) -> anyhow::Result { - let config = build_config(name, model, effort, None, dir); + let config = build_config(name, model, effort, None, dir, Some(&state.signal_url)); // No existence pre-check: claude's own `--resume` is the authority on // whether the session is there, and it errors rather than quietly // starting a fresh one. `verdict` is how that answer gets back to the @@ -769,6 +1128,16 @@ impl hive_claude::Sink for LivenessSink { /// that is waiting for it (`await_resume`); `None` on a `start`, which keeps /// the immediate-return contract unchanged. /// +/// **The turn *continuation* loop lives in that background task, and only +/// there.** A session `start`ed with a goal runs turn after turn until +/// something stops it, and every one of those turns is a fresh +/// `Claude::spawn` against `Attach::Resume` — the driver's `wait` consumes +/// its child, so there is no other shape it could take. Keeping the loop +/// behind the same `tokio::spawn` is what leaves both tool-call contracts +/// untouched: `start` still returns at the first spawn, `continue` still +/// returns when its own turn is underway, and neither waits on turns two +/// through five. +/// /// Not `async` itself — `tokio::spawn` needs an active runtime to spawn /// *onto*, not an `async` caller to spawn *from*. fn spawn_and_track( @@ -781,14 +1150,7 @@ fn spawn_and_track( ) -> anyhow::Result { let running = Claude::spawn(config, attach) .map_err(|e| anyhow::anyhow!("starting the subagent process failed: {e}"))?; - // Upgrades the `None` reservation `reserve` already placed here to a - // real cancel handle — same key, so there's no window where `name` - // reads as unoccupied between the reservation and this insert. - state - .running - .lock() - .unwrap_or_else(PoisonError::into_inner) - .insert(name.to_owned(), Some(running.cancel_handle())); + state.track(name, running.cancel_handle()); // This turn supersedes whatever the previous one did, including having // been killed — the record is about the turn before this one. state.clear_kill(name); @@ -798,85 +1160,329 @@ fn spawn_and_track( // Resolved here, on the calling thread, while the config is still to // hand: only a resume can miss, and only `classify_end` finding a - // `SessionNotFound` ever uses it. - let searched = matches!(attach, Attach::Resume(_)) - .then(|| searched_location(config)) - .flatten(); + // `SessionNotFound` ever uses it. Every continuation turn is a resume, + // so this is worth having even when the *first* attach is a `Create`. + let resume_searched = searched_location(config); + let mut searched = if matches!(attach, Attach::Resume(_)) { + resume_searched.clone() + } else { + None + }; + let config = config.clone(); let state = Arc::clone(state); let task_name = name.to_owned(); tokio::spawn(async move { - let sink = LivenessSink { - state: Arc::clone(&state), - name: task_name.clone(), - verdict: verdict.clone(), - }; - let end = classify_end(running.wait(&prompt, &sink).await, searched.as_deref()); - match &end { - TurnEnd::Complete => {} - TurnEnd::Killed { signal } => { - tracing::warn!( - name = %task_name, - signal, - "subagent: turn killed — the child died on a signal, it did not finish" - ); + let mut running = running; + let mut prompt = prompt; + loop { + let sink = LivenessSink { + state: Arc::clone(&state), + name: task_name.clone(), + verdict: verdict.clone(), + }; + let end = classify_end(running.wait(&prompt, &sink).await, searched.as_deref()); + log_turn_end(&task_name, &end); + // The todo is how a turn's end reaches an agent that is no + // longer looking — so it is pushed for every end *except* the + // one the caller is being handed as a tool-call error right now. + // `settle` saying the verdict was delivered is what makes that + // certain: a `continue` whose grace had already run out gets + // `false` here and its todo, same as before. Only the first turn + // can ever win this — the sender is consumed — which is right, + // since only the first turn is one a caller is still waiting on. + let reported = settle(verdict.as_ref(), ResumeVerdict::Ended(end.clone())); + + if !matches!(end, TurnEnd::Complete) { + // A killed or failed turn ends the run, goal or not: there is + // nothing to re-prompt a child that isn't there any more, and + // these two ends already have records of their own. + state.finish_turn(&task_name, &end); + if !(matches!(end, TurnEnd::Failed(_)) && reported) { + push_turn_end_todo(&state.socket, &task_name, &end, None).await; + } + return; } - TurnEnd::Failed(e) => { - tracing::warn!(name = %task_name, error = %e, "subagent: turn failed"); + + match state.plan_after_turn(&task_name) { + Continuation::Stop(stop) => { + state.record_stop(&task_name, stop.clone()); + state.finish_turn(&task_name, &end); + write_stop_to_report(state.report_file(&task_name), &task_name, &stop).await; + push_turn_end_todo(&state.socket, &task_name, &end, Some(&stop)).await; + return; + } + Continuation::Continue { prompt: next } => { + // The name stays claimed across the gap — see + // `between_turns` for what a concurrent `start` would + // otherwise be able to do with it. + state.between_turns(&task_name); + match Claude::spawn(&config, &Attach::Resume(task_name.clone())) { + Ok(next_running) => { + state.track(&task_name, next_running.cancel_handle()); + state.note_event(&task_name); + running = next_running; + prompt = next; + searched = resume_searched.clone(); + } + Err(e) => { + let end = TurnEnd::Failed(format!( + "claude error: starting the next goal turn failed: {e}" + )); + log_turn_end(&task_name, &end); + state.finish_turn(&task_name, &end); + push_turn_end_todo(&state.socket, &task_name, &end, None).await; + return; + } + } + } } } - state.finish_turn(&task_name, &end); - let failed = matches!(end, TurnEnd::Failed(_)); - // The todo is how a turn's end reaches an agent that is no longer - // looking — so it is pushed for every end *except* the one the - // caller is being handed as a tool-call error right now. `settle` - // saying the verdict was delivered is what makes that certain: a - // `continue` whose grace had already run out gets `false` here and - // its todo, same as before. - let reported = settle(verdict.as_ref(), ResumeVerdict::Ended(end.clone())); - if !(failed && reported) { - push_turn_end_todo(&state.socket, &task_name, &end).await; - } }); Ok(format!("subagent `{name}` started")) } +/// Log a turn's end at the level its severity deserves: a completion is +/// unremarkable, the other two are not. +fn log_turn_end(name: &str, end: &TurnEnd) { + match end { + TurnEnd::Complete => {} + TurnEnd::Killed { signal } => { + tracing::warn!( + name = %name, + signal, + "subagent: turn killed — the child died on a signal, it did not finish" + ); + } + TurnEnd::Failed(e) => { + tracing::warn!(name = %name, error = %e, "subagent: turn failed"); + } + } +} + +/// Append the stop reason to the session's own report file, so the artifact +/// a parent already reads is where the run's ending is recorded too — rather +/// than the parent having to correlate a todo against a file. +/// +/// Best-effort and appended, never rewritten: the subagent wrote that file, +/// and this adds a line under what it wrote instead of taking a position on +/// the rest of it. A path this daemon can't write to is logged and dropped — +/// the todo still carries the same sentence, so nothing is only here. +async fn write_stop_to_report(path: Option, name: &str, stop: &StopReason) { + let Some(path) = path else { return }; + let line = format!("\n**Subagent `{name}` stopped:** {}\n", stop.sentence()); + let appended = tokio::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&path) + .await; + let result = match appended { + Ok(mut file) => { + use tokio::io::AsyncWriteExt as _; + // `flush`, not just `write_all`: a `tokio::fs::File` buffers, and + // dropping one discards whatever hasn't been handed to the + // blocking pool — so without this the line is written to nothing + // and the failure is silent. A unit test caught exactly that. + match file.write_all(line.as_bytes()).await { + Ok(()) => file.flush().await, + Err(e) => Err(e), + } + } + Err(e) => Err(e), + }; + if let Err(e) = result { + tracing::warn!( + name, + path = %path.display(), + error = ?e, + "subagent: could not record the stop reason in the session's report file", + ); + } +} + +/// The goal contract appended to a `start`'s first prompt when a goal was +/// given. Spelled out to the subagent rather than left implicit: it is about +/// to be re-prompted by something it can't see, and the two tools that stop +/// that are the only way it has to say "done" or "stuck". +fn goal_briefing(name: &str, goal: &str, max_turns: u32) -> String { + format!( + "\n\nYour goal for this session: {goal}\n\nYou have up to {max_turns} turns to reach it. \ + When a turn of yours ends and you haven't reported the goal reached, the harness starts \ + another turn re-prompting you toward it. Call the `goal_reached` tool (with `name: \ + \"{name}\"`) once you've genuinely reached it, or `need_help` (same `name`) with what is \ + blocking you if you can't proceed — either one stops the re-prompting. Running out of \ + turns stops it too, with the work left wherever it had got to." + ) +} + +/// The prompt a continuation turn opens with. It states the one fact the +/// subagent can't observe for itself — that its last turn ended without the +/// goal being reported reached — and says plainly that claiming the goal +/// isn't the same as reaching it, since being re-prompted is precisely the +/// pressure to claim it. +fn continuation_prompt(name: &str, goal: &str, turn: u32, max_turns: u32) -> String { + format!( + "Your previous turn ended and you have not reported the goal reached.\n\nGoal: \ + {goal}\n\nThis is turn {turn} of {max_turns}. Carry on toward the goal. If you have in \ + fact reached it, call `goal_reached` with `name: \"{name}\"`; if you are blocked, call \ + `need_help` with the same `name` and what is blocking you. Neither is a substitute for \ + the work: whoever spawned you reads what you actually changed, not what you claim about \ + it." + ) +} + +/// Record the subagent's own "I have reached the goal" signal and stop its +/// turn continuation. Called by the subagent, from inside its own turn, over +/// the signal route this daemon hands it (see `build_config`). +/// +/// **This verifies nothing**, and the answer it returns says so to the +/// subagent's face. It stops the loop and extends the done message; whether +/// the goal was actually reached is a question about the diff and the gate +/// output, which the parent reads for itself. +/// +/// `report_file` is the subagent saying where it wrote its report, which is +/// the only reason this daemon ever knows that path — see +/// `State::set_report_file`. +/// +/// # Errors +/// +/// An invalid name, or a name with nothing in flight: these are a running +/// subagent's signals about its own turn, and a name that isn't running is +/// either a typo or a signal aimed at somebody else's session. +pub fn goal_reached( + state: &State, + name: &str, + msg: Option, + report_file: Option<&str>, +) -> anyhow::Result { + signal_stop( + state, + name, + StopReason::GoalReached(msg), + report_file, + "goal_reached", + )?; + Ok(format!( + "noted — `{name}`'s goal is recorded as reported reached, so this turn finishes and no \ + further goal turn is started. It is recorded as your claim, not as verification: whoever \ + spawned you still reads what you changed." + )) +} + +/// Record that the subagent can't proceed, and stop its turn continuation. +/// The blocking signal the run has otherwise no way to raise: without it a +/// stuck subagent would be re-prompted toward a goal it has already told +/// nobody it can't reach, until the turn cap. +/// +/// `msg` is required, unlike `goal_reached`'s — "I'm stuck" with no reason +/// gives the parent nothing to act on, and acting on it is the entire point. +/// +/// # Errors +/// +/// Same as [`goal_reached`]. +pub fn need_help( + state: &State, + name: &str, + msg: String, + report_file: Option<&str>, +) -> anyhow::Result { + signal_stop( + state, + name, + StopReason::NeedHelp(msg), + report_file, + "need_help", + )?; + Ok(format!( + "noted — `{name}` is recorded as blocked, so this turn finishes and no further goal turn \ + is started. Write down what you have done so far where your brief told you to; whoever \ + spawned you sees the block in `status` and in this run's todo." + )) +} + +/// The half [`goal_reached`] and [`need_help`] share: check the signal is +/// coming from a session that's actually in flight, remember where the +/// subagent says it wrote, and record the stop. +/// +/// The in-flight check is what keeps a signal pointed at its own session. +/// It's a guard, not a boundary: two subagents running concurrently can each +/// reach the other's name, since the signal route carries no identity of its +/// own. Bounded on purpose — the subagents sharing that route are ones the +/// same parent spawned, and the cost of a misfire is a stopped continuation +/// the parent can restart with `continue`, not lost work. +fn signal_stop( + state: &State, + name: &str, + stop: StopReason, + report_file: Option<&str>, + tool: &str, +) -> anyhow::Result<()> { + validate_name(name)?; + if state.occupancy(name).is_none() { + anyhow::bail!( + "no subagent named `{name}` has a turn in flight — `{tool}` is a running subagent's \ + signal about its own session, so check the `name` you were given" + ); + } + state.set_report_file(name, report_file); + state.record_stop(name, stop); + Ok(()) +} + /// Report whether `name` is currently running — a zero-cost check that -/// never launches a process, unlike `continue`. Distinguishes five states: -/// running, starting (reserved, not yet a confirmed spawn — see `State`'s -/// doc), killed (its last turn died on a signal), idle (a session exists, -/// its last turn finished, nothing is in flight), and no such session at -/// all. +/// never launches a process, unlike `continue`. Distinguishes running, +/// starting (reserved, not yet a confirmed spawn — see `State`'s doc), +/// killed (its last turn died on a signal), each of the four ways a goal +/// run stops, idle (a session exists, its last turn finished, nothing is in +/// flight), and no such session at all. /// /// A *running* answer also carries how long since that turn last produced /// output, which is the part of this answer a caller can act on: "running" /// describes a wedged child and a busy one identically, and the age /// separates them (see `State`'s `last_event` doc). /// +/// A goal-continued session carries `turn N of M` alongside that, in every +/// state. With the age, it is what lets a caller tell *working* from +/// *wedged* from *out of turns* off one answer, without reaching for `ps` +/// or reading any file. +/// /// # Errors /// -/// An invalid name, or no session — running, killed or on disk — under -/// `name`. +/// An invalid name, or no session — running, killed, stopped or on disk — +/// under `name`. pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result { validate_name(name)?; // Read-only: an explicit `dir` here is a one-off "check this other // directory's session" per this fn's own doc, not a new remembered // default — `peek_dir` resolves the same way but never writes. let dir = state.peek_dir(name, dir); - let occupancy = state.occupancy(name); - let killed = state.killed_by(name); - let last_event_age = state.last_event_age(name); + let facts = StatusFacts { + occupancy: state.occupancy(name), + killed: state.killed_by(name), + last_event_age: state.last_event_age(name), + turns: state.turns(name), + stop: state.stop_reason(name), + ..StatusFacts::new(name) + }; // Nothing on disk to look for while something is tracked in memory — // those states answer on their own, and the store read is the only - // expensive part of this call. - let session_exists = if occupancy.is_none() && killed.is_none() { - let config = build_config(name, None, None, None, dir.as_deref()); - build_store(&config)?.find_by_title(name).is_some() - } else { - false - }; - describe_status(name, occupancy, killed, session_exists, last_event_age) + // expensive part of this call. A recorded stop reason counts: it is + // proof this daemon ran the session, which is what the lookup asks. + let session_exists = + if facts.occupancy.is_none() && facts.killed.is_none() && facts.stop.is_none() { + // No signal surface in this config: it exists only to resolve the + // session store, and rendering a subagent's MCP config for a + // read-only status check would be writing a file for nobody. + let config = build_config(name, None, None, None, dir.as_deref(), None); + build_store(&config)?.find_by_title(name).is_some() + } else { + false + }; + describe_status(&StatusFacts { + session_exists, + ..facts + }) } /// The liveness sentence appended to a *running* answer, and the whole point @@ -894,65 +1500,113 @@ fn describe_liveness(age: Option) -> String { Some(age) => format!( " Last event {}s ago — how long since this turn's claude process produced any output \ at all, whatever it was: a few seconds means it's working, an age that keeps \ - climbing into the minutes with no end-of-turn todo means it's wedged.", + climbing into the minutes means it's wedged. It resets at each turn's spawn, so on a \ + goal run it describes the turn in flight, not the run.", age.as_secs() ), } } -/// Render `status`'s answer from the facts it gathers. Split out from the -/// gathering so the killed-versus-idle distinction — and the liveness age on -/// a running turn — are exercisable without a real spawn, a real signal, a -/// real stream of events and a real on-disk claude session. +/// The progress sentence a goal-continued session carries in every state: +/// which turn of its budget it is on. Empty for a session with no goal, +/// which has no budget to be partway through. /// -/// A recorded kill outranks the on-disk session: the session file exists -/// either way, so "a session is there" is precisely the fact that cannot -/// tell the two apart. +/// On a *running* answer this is the turn in flight; on a stopped one it is +/// the turn it stopped on — the same number either way, since the counter +/// only advances when the loop decides to spend another turn. +fn describe_turns(turns: Option<(u32, u32)>) -> String { + match turns { + None => String::new(), + Some((turn, max_turns)) => format!(" Turn {turn} of {max_turns}."), + } +} + +/// Everything `status` gathers, as one value — seven separate parameters +/// read as noise at both the call site and the test sites, and every one of +/// them is a fact about the same session. +struct StatusFacts<'a> { + name: &'a str, + occupancy: Option, + killed: Option, + session_exists: bool, + last_event_age: Option, + turns: Option<(u32, u32)>, + stop: Option, +} + +impl<'a> StatusFacts<'a> { + /// The nothing-known baseline for `name`: no process, no records, no + /// session. Both the real gathering in `status` and the tests build on + /// it, so neither has to spell out the fields it isn't exercising. + fn new(name: &'a str) -> Self { + Self { + name, + occupancy: None, + killed: None, + session_exists: false, + last_event_age: None, + turns: None, + stop: None, + } + } +} + +/// Render `status`'s answer from the facts it gathers. Split out from the +/// gathering so the killed-versus-idle distinction — and the liveness age, +/// the turn counter and each stop reason — are exercisable without a real +/// spawn, a real signal, a real stream of events and a real on-disk claude +/// session. +/// +/// A recorded kill outranks both the stop reason and the on-disk session: +/// the session file exists either way and a stop reason may be left over +/// from the signal a subagent raised just before something killed it, so +/// neither can tell a killed turn from a finished one. The kill can. /// /// Every answer is self-contained — it names the one state the caller got /// and what to do next — because the tool description deliberately doesn't /// enumerate the state space (the operator's ruling on this surface: /// describe the tool, explain the state when returning it). Keep the split: /// a terser answer here has nowhere left to be explained from. -fn describe_status( - name: &str, - occupancy: Option, - killed: Option, - session_exists: bool, - last_event_age: Option, -) -> anyhow::Result { - match occupancy { +fn describe_status(facts: &StatusFacts<'_>) -> anyhow::Result { + let name = facts.name; + let turns = describe_turns(facts.turns); + match facts.occupancy { Some(true) => { return Ok(format!( "subagent `{name}` is running — its turn is still in flight, so there's nothing \ - to do but let it work: the daemon pushes a todo when the turn ends, or \ - `interrupt` it if you want it stopped early.{}", - describe_liveness(last_event_age) + to do but let it work: the daemon pushes a todo when the run ends, or \ + `interrupt` it if you want it stopped early.{turns}{}{}", + describe_liveness(facts.last_event_age), + describe_pending_signal(facts.stop.as_ref()), )); } Some(false) => { return Ok(format!( - "subagent `{name}` is starting — a `start`/`continue` has claimed the name but \ - its process isn't confirmed spawned yet, which is normally over in well under a \ - second: check again shortly rather than starting anything else under this name." + "subagent `{name}` is starting — the name is claimed but no process is confirmed \ + under it yet, either because a `start`/`continue` hasn't spawned one or because \ + a goal run is between turns. Normally over in well under a second: check again \ + shortly rather than starting anything else under this name.{turns}" )); } None => {} } - if let Some(signal) = killed { + if let Some(signal) = facts.killed { return Ok(format!( "subagent `{name}` was killed — its last turn died on {}, so its work stopped \ wherever it had got to rather than finishing. `continue` still resumes it, but \ whatever it was told to do is unfinished: check what it actually left behind before \ - trusting it.", + trusting it.{turns}", describe_signal(signal) )); } - if session_exists { + if let Some(stop) = &facts.stop { + return Ok(describe_stopped(name, stop, &turns)); + } + if facts.session_exists { Ok(format!( "subagent `{name}` is idle — its session exists, its last turn ended on its own \ rather than being cut off, and nothing is in flight: that turn's own todo says how \ - it went, and `continue` gives it another." + it went, and `continue` gives it another.{turns}" )) } else { anyhow::bail!( @@ -963,16 +1617,74 @@ fn describe_status( } } +/// The note a *running* answer carries when the subagent has already raised +/// a stop signal for the turn still in flight. Without it a parent polling +/// `status` would read plain "running" for the whole stretch between the +/// subagent saying it is blocked and its turn actually ending — the one +/// stretch where "it's working, leave it alone" is the wrong conclusion. +fn describe_pending_signal(stop: Option<&StopReason>) -> String { + match stop { + None | Some(StopReason::Done | StopReason::TurnCap { .. }) => String::new(), + Some(stop) => format!( + " It has already signalled how this run ends — {} — so this is its last turn.", + stop.sentence() + ), + } +} + +/// 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 +/// read as "finished successfully" when it means "said so". +fn describe_stopped(name: &str, stop: &StopReason, turns: &str) -> String { + match stop { + StopReason::Done => format!( + "subagent `{name}` is idle — its session exists, its last turn ended on its own \ + rather than being cut off, and nothing is in flight: that turn's own todo says how \ + it went, and `continue` gives it another.{turns}" + ), + StopReason::GoalReached(msg) => format!( + "subagent `{name}` stopped: it reported its goal reached{}. Nothing is in flight and \ + no further goal turn will start.{turns} That report is the subagent's own claim, not \ + a verification of anything — read the diff and whatever gate the work was supposed \ + to pass before you treat the goal as met, exactly as you would a build report saying \ + the tests passed.", + msg.as_deref() + .map_or_else(String::new, |m| format!(": {m}")) + ), + StopReason::NeedHelp(msg) => format!( + "subagent `{name}` is BLOCKED and needs help: {msg}. It called `need_help`, which \ + stopped its goal continuation, and nothing is in flight — it stays blocked until you \ + answer it.{turns} `continue` is how you answer: give it what it asked for as the \ + next turn's prompt." + ), + StopReason::TurnCap { turns: spent } => format!( + "subagent `{name}` ran out of turns — the harness limit of {spent} was reached and it \ + never reported its goal reached, so the work stopped wherever it had got to rather \ + than finishing.{turns} Check what it actually left behind; `continue` gives it a \ + fresh allowance if carrying on is worth it." + ), + } +} + /// Signal `name`'s running process — `force` picks SIGKILL over SIGINT (see /// `hive_claude::Cancel::cancel`). Refuses a name with nothing running: no -/// entry at all, or one still in the brief `reserve`d-but-not-yet-spawned -/// window (nothing to signal yet — the reservation is put back so a -/// concurrent `start`/`continue` for the same name still gets refused). +/// entry at all, or one still in the brief not-yet-spawned window (nothing +/// 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. /// /// # Errors /// -/// An invalid name, nothing tracked under `name`, or `name` is still -/// starting (reserved, not yet a confirmed spawn). +/// An invalid name, nothing tracked under `name`, or `name` has no confirmed +/// process right now (a spawn in flight, or a goal run between turns). pub fn interrupt(state: &State, name: &str, force: bool) -> anyhow::Result { validate_name(name)?; let mut running = state.running.lock().unwrap_or_else(PoisonError::into_inner); @@ -997,11 +1709,16 @@ pub fn interrupt(state: &State, name: &str, force: bool) -> anyhow::Result, +) { let req = hive_agent_sock::Request::UpsertTodo { subsystem: "subagent".to_owned(), key: Some(name.to_owned()), - summary: turn_end_summary(name, end), + summary: turn_end_summary(name, end, stop), source: None, reopen_if_acked: false, }; @@ -1010,12 +1727,20 @@ async fn push_turn_end_todo(socket: &std::path::Path, name: &str, end: &TurnEnd) } } -/// The todo text for a turn that has ended. A killed turn deliberately does +/// The todo text for a run that has ended. A killed turn deliberately does /// not use the "finished" wording the other two share: this todo is the only /// thing the owning agent is shown without asking, so it has to read as the /// interruption it is rather than as one more completed subagent. -fn turn_end_summary(name: &str, end: &TurnEnd) -> String { - match end { +/// +/// A `stop` **extends** that message, it never replaces it. Both halves are +/// load-bearing and neither substitutes for the other: the turn's own end is +/// what the daemon observed, the stop reason is why the run went no further +/// — and for `GoalReached` that second half is a claim, which would read as +/// a verdict if it were allowed to stand where the observed end belongs. +/// `StopReason::Done` adds nothing, since "there was no goal" is exactly +/// what the unextended message already describes. +fn turn_end_summary(name: &str, end: &TurnEnd, stop: Option<&StopReason>) -> String { + let base = match end { TurnEnd::Complete => format!("subagent `{name}` finished: turn complete"), TurnEnd::Failed(e) => format!("subagent `{name}` finished: {e}"), TurnEnd::Killed { signal } => format!( @@ -1024,6 +1749,10 @@ fn turn_end_summary(name: &str, end: &TurnEnd) -> String { resumes the session if you want it carried on.", describe_signal(*signal) ), + }; + match stop { + None | Some(StopReason::Done) => base, + Some(stop) => format!("{base} — and the run stopped there: {}", stop.sentence()), } } @@ -1036,9 +1765,32 @@ mod tests { // half of `State` directly — the actual TOCTOU-closure logic — rather // than the full start/spawn path. + /// A stand-in for the signal route a real daemon would hand its + /// subagents. Nothing in these tests dials it: what `State` does with it + /// is carry it into `build_config`, which is asserted on directly. + fn signal_url() -> String { + "http://127.0.0.1:1/signal/mcp".to_owned() + } + + /// A `StartRequest` with only the fields a test cares about set — the + /// other six are the same "nothing asked for" every time. + fn start_request(name: &str) -> StartRequest { + StartRequest { + name: name.to_owned(), + model: None, + effort: None, + prompt_file: "/tmp/prompt.md".to_owned(), + trigger: "trigger".to_owned(), + dir: None, + goal: None, + max_turns: None, + report_file: None, + } + } + #[test] fn reserve_is_exclusive_for_the_same_name() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert!(state.reserve("dup"), "first reservation should succeed"); assert!( !state.reserve("dup"), @@ -1049,7 +1801,7 @@ mod tests { #[test] fn reserve_does_not_cross_block_different_names() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert!(state.reserve("a")); assert!( state.reserve("b"), @@ -1059,7 +1811,7 @@ mod tests { #[test] fn release_reservation_frees_the_name_for_reuse() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert!(state.reserve("n")); state.release_reservation("n"); assert!( @@ -1070,7 +1822,7 @@ mod tests { #[test] fn occupancy_reflects_the_reserved_but_not_running_state() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert_eq!(state.occupancy("never-reserved"), None); state.reserve("n"); assert_eq!( @@ -1109,14 +1861,14 @@ mod tests { #[test] fn build_config_only_appends_system_prompt_when_given() { - let with = build_config("n", None, None, Some("/tmp/p.md"), None); + let with = build_config("n", None, None, Some("/tmp/p.md"), None, None); assert!( with.extra_args .contains(&"--append-system-prompt-file".to_owned()) ); assert!(with.extra_args.contains(&"/tmp/p.md".to_owned())); - let without = build_config("n", None, None, None, None); + let without = build_config("n", None, None, None, None, None); assert!( !without .extra_args @@ -1126,7 +1878,7 @@ mod tests { #[test] fn resolve_dir_remembers_an_explicit_dir_and_falls_back_to_it_when_omitted() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert_eq!( state.resolve_dir("n", None), None, @@ -1156,7 +1908,7 @@ mod tests { #[test] fn resolve_dir_does_not_cross_names() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); state.resolve_dir("a", Some("/tmp/a")); assert_eq!( state.resolve_dir("b", None), @@ -1167,7 +1919,7 @@ mod tests { #[test] fn peek_dir_resolves_like_resolve_dir_but_never_writes() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); state.resolve_dir("n", Some("/tmp/remembered")); assert_eq!( state.peek_dir("n", Some("/tmp/one-off")), @@ -1189,7 +1941,7 @@ mod tests { // to. Reordering `reserve` before `resolve_dir` in `start` closes it // — a name that's already reserved must never reach `resolve_dir` at // all, so `dirs` stays exactly as a caller left it. - let state = Arc::new(State::new(PathBuf::from("/dev/null"))); + let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())); state.resolve_dir("dup", Some("/tmp/original")); assert!( state.reserve("dup"), @@ -1197,12 +1949,10 @@ mod tests { ); let result = start( &state, - "dup", - None, - None, - "/tmp/prompt.md", - "trigger".to_owned(), - Some("/tmp/rejected"), + StartRequest { + dir: Some("/tmp/rejected".to_owned()), + ..start_request("dup") + }, ); assert!( result.is_err(), @@ -1251,9 +2001,17 @@ mod tests { fn status_reports_killed_not_idle_for_a_signalled_session() { // Both sessions exist on disk and neither is running: the only thing // that can tell them apart is the recorded signal. - let killed = - describe_status("n", None, Some(libc::SIGKILL), true, None).expect("killed status"); - let idle = describe_status("n", None, None, true, None).expect("idle status"); + let killed = describe_status(&StatusFacts { + killed: Some(libc::SIGKILL), + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("killed status"); + let idle = describe_status(&StatusFacts { + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("idle status"); assert!( killed.contains("killed") && killed.contains("SIGKILL (signal 9)"), "a killed session must say so, and name the signal: {killed}" @@ -1274,30 +2032,45 @@ mod tests { // The tool description no longer lists the states, so each answer // has to carry its own explanation — checked one state at a time, // which is all a caller ever gets back. - let running = describe_status("n", Some(true), None, false, None).expect("running status"); + let running = describe_status(&StatusFacts { + occupancy: Some(true), + ..StatusFacts::new("n") + }) + .expect("running status"); assert!( running.contains("running") && running.contains("`interrupt`"), "a running answer must say the turn is in flight and how to stop it: {running}" ); - let starting = - describe_status("n", Some(false), None, false, None).expect("starting status"); + let starting = describe_status(&StatusFacts { + occupancy: Some(false), + ..StatusFacts::new("n") + }) + .expect("starting status"); assert!( starting.contains("starting") && starting.contains("check again"), "a starting answer must say the spawn isn't confirmed yet and to retry: {starting}" ); - let idle = describe_status("n", None, None, true, None).expect("idle status"); + let idle = describe_status(&StatusFacts { + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("idle status"); assert!( idle.contains("idle") && idle.contains("`continue`"), "an idle answer must say the last turn ended on its own and how to give it another: \ {idle}" ); - let killed = - describe_status("n", None, Some(libc::SIGKILL), true, None).expect("killed status"); + let killed = describe_status(&StatusFacts { + killed: Some(libc::SIGKILL), + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("killed status"); assert!( killed.contains("killed") && killed.contains("`continue`"), "a killed answer must name the kill and say resuming is still possible: {killed}" ); - let missing = describe_status("n", None, None, false, None) + let missing = describe_status(&StatusFacts::new("n")) .expect_err("nothing tracked and nothing on disk is an error, not a state"); assert!( missing.to_string().contains("`start`"), @@ -1309,7 +2082,7 @@ mod tests { fn a_killed_turn_and_a_clean_one_do_not_land_in_the_same_state() { // The whole path a real turn takes, minus the process: what the // driver returned -> what the daemon records -> what `status` says. - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); for (name, outcome) in [ ("gone", Err(signalled_exit(libc::SIGKILL))), ("done", Ok(())), @@ -1325,17 +2098,25 @@ mod tests { assert_eq!(state.killed_by("gone"), Some(9)); assert_eq!(state.killed_by("done"), None); - let gone = - describe_status("gone", None, state.killed_by("gone"), true, None).expect("status"); - let done = - describe_status("done", None, state.killed_by("done"), true, None).expect("status"); + let gone = describe_status(&StatusFacts { + killed: state.killed_by("gone"), + session_exists: true, + ..StatusFacts::new("gone") + }) + .expect("status"); + let done = describe_status(&StatusFacts { + killed: state.killed_by("done"), + session_exists: true, + ..StatusFacts::new("done") + }) + .expect("status"); assert!(gone.contains("killed"), "{gone}"); assert!(done.contains("idle"), "{done}"); } #[test] fn a_new_turn_clears_the_previous_turn_s_kill() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); state.finish_turn("n", &TurnEnd::Killed { signal: 9 }); assert_eq!(state.killed_by("n"), Some(9)); // What `spawn_and_track` does once the next turn is confirmed @@ -1352,8 +2133,8 @@ mod tests { #[test] fn the_killed_todo_does_not_read_like_a_completion() { - let complete = turn_end_summary("n", &TurnEnd::Complete); - let killed = turn_end_summary("n", &TurnEnd::Killed { signal: 9 }); + let complete = turn_end_summary("n", &TurnEnd::Complete, None); + let killed = turn_end_summary("n", &TurnEnd::Killed { signal: 9 }, None); assert_eq!( complete, "subagent `n` finished: turn complete", "the ordinary completion todo is unchanged" @@ -1394,10 +2175,10 @@ mod tests { #[test] fn build_config_sets_cwd_only_when_a_dir_is_given() { - let with = build_config("n", None, None, None, Some("/tmp/some-worktree")); + let with = build_config("n", None, None, None, Some("/tmp/some-worktree"), None); assert_eq!(with.cwd, Some(PathBuf::from("/tmp/some-worktree"))); - let without = build_config("n", None, None, None, None); + let without = build_config("n", None, None, None, None, None); assert_eq!(without.cwd, None); } @@ -1409,6 +2190,7 @@ mod tests { Some("high".to_owned()), None, None, + None, ); assert_eq!(config.model, Some("opus".to_owned())); assert_eq!(config.effort, Some("high".to_owned())); @@ -1416,7 +2198,7 @@ mod tests { #[test] fn an_event_starts_the_liveness_clock_and_the_turn_ending_stops_it() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); assert_eq!( state.last_event_age("n"), None, @@ -1440,7 +2222,7 @@ mod tests { fn a_later_event_resets_the_age_rather_than_letting_it_climb() { // The distinction the whole record exists for: a child still // producing output must not accumulate the age of a wedged one. - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); state.note_event("n"); std::thread::sleep(Duration::from_millis(20)); let before = state.last_event_age("n").expect("clock started"); @@ -1454,7 +2236,7 @@ mod tests { #[test] fn the_liveness_clock_does_not_cross_names() { - let state = State::new(PathBuf::from("/dev/null")); + let state = State::new(PathBuf::from("/dev/null"), signal_url()); state.note_event("a"); assert_eq!( state.last_event_age("b"), @@ -1471,7 +2253,7 @@ mod tests { use hive_claude::Sink as _; fn fresh() -> (Arc, LivenessSink) { - let state = Arc::new(State::new(PathBuf::from("/dev/null"))); + let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())); let sink = LivenessSink { state: Arc::clone(&state), name: "n".to_owned(), @@ -1504,15 +2286,23 @@ mod tests { #[test] fn a_running_status_reports_the_age_and_an_idle_one_does_not() { - let running = describe_status("n", Some(true), None, false, Some(Duration::from_secs(4))) - .expect("running status"); + let running = describe_status(&StatusFacts { + occupancy: Some(true), + last_event_age: Some(Duration::from_secs(4)), + ..StatusFacts::new("n") + }) + .expect("running status"); assert!( running.contains("Last event 4s ago"), "a running answer must carry the age — the one part of it a caller can act on: \ {running}" ); - let wedged = describe_status("n", Some(true), None, false, Some(Duration::from_mins(15))) - .expect("running status"); + let wedged = describe_status(&StatusFacts { + occupancy: Some(true), + last_event_age: Some(Duration::from_mins(15)), + ..StatusFacts::new("n") + }) + .expect("running status"); assert!( wedged.contains("Last event 900s ago"), "the wedged case is the one this exists for: {wedged}" @@ -1523,7 +2313,11 @@ mod tests { the bug this reports" ); - let idle = describe_status("n", None, None, true, None).expect("idle status"); + let idle = describe_status(&StatusFacts { + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("idle status"); assert!( !idle.contains("Last event"), "an idle session has no in-flight turn whose progress an age would describe: {idle}" @@ -1535,7 +2329,11 @@ mod tests { // `status` reads `running` and the clock under separate locks, so a // turn can finish between the two. The answer drops the age rather // than inventing one. - let raced = describe_status("n", Some(true), None, false, None).expect("running status"); + let raced = describe_status(&StatusFacts { + occupancy: Some(true), + ..StatusFacts::new("n") + }) + .expect("running status"); assert!( raced.contains("is running") && !raced.contains("Last event"), "a missing age must cost the sentence, not the answer: {raced}" @@ -1598,7 +2396,7 @@ mod tests { // re-deriving them: the location reported must be the one actually // searched, so it can't drift from `build_store`/the driver. let cwd = std::env::current_dir().expect("a cwd"); - let config = build_config("n", None, None, None, Some(&cwd.to_string_lossy())); + let config = build_config("n", None, None, None, Some(&cwd.to_string_lossy()), None); let Some(located) = searched_location(&config) else { // No `HOME` in this environment — nothing to compare against. return; @@ -1747,7 +2545,7 @@ mod tests { fn fresh() -> (oneshot::Receiver, LivenessSink) { let (tx, rx) = oneshot::channel(); let sink = LivenessSink { - state: Arc::new(State::new(PathBuf::from("/dev/null"))), + state: Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())), name: "n".to_owned(), verdict: Some(Arc::new(Mutex::new(Some(tx)))), }; @@ -1786,7 +2584,446 @@ mod tests { // claude's own default (`high` on most models) — this daemon picks // `medium` itself, a deliberate cost-conscious choice for subagent // work. - let config = build_config("n", None, None, None, None); + let config = build_config("n", None, None, None, None, None); assert_eq!(config.effort, Some("medium".to_owned())); } + + // ---- turn continuation ------------------------------------------------- + + /// A state with `name` mid-run against `goal`, as `start` would have left + /// it: reserved, goal registered, on turn one of `max_turns`. + fn mid_run(name: &str, goal: &str, max_turns: u32) -> State { + let state = State::new(PathBuf::from("/dev/null"), signal_url()); + state.reserve(name); + state.set_goal(name, Some(goal.to_owned()), max_turns); + state + } + + #[test] + fn a_session_with_no_goal_stops_after_one_turn() { + // The pre-continuation shape, and the reason `goal` is what switches + // the loop on rather than a separate flag: nothing to continue + // toward is the same fact as nothing to continue. + let state = mid_run("n", "unused", 5); + state.set_goal("n", None, 5); + assert!( + matches!( + state.plan_after_turn("n"), + Continuation::Stop(StopReason::Done) + ), + "without a goal the first completed turn ends the run" + ); + assert_eq!( + state.turns("n"), + None, + "and there is no turn budget to report being partway through" + ); + } + + #[test] + fn a_goal_keeps_spending_turns_until_the_cap_and_then_stops() { + let state = mid_run("n", "make the gate pass", 3); + assert_eq!(state.turns("n"), Some((1, 3)), "the first turn is 1 of 3"); + + for expected in [2, 3] { + let Continuation::Continue { prompt } = state.plan_after_turn("n") else { + panic!("turn {expected} of 3 must still be spent"); + }; + assert!( + prompt.contains("make the gate pass"), + "the re-prompt continues toward the goal verbatim: {prompt}" + ); + assert!( + prompt.contains("have not reported the goal reached"), + "and says the one thing the subagent can't observe for itself: {prompt}" + ); + assert_eq!(state.turns("n"), Some((expected, 3))); + } + + assert_eq!( + state.plan_after_turn("n"), + Continuation::Stop(StopReason::TurnCap { turns: 3 }), + "the cap is the number of turns run, not one more" + ); + assert_eq!( + state.turns("n"), + Some((3, 3)), + "a capped run must not advance past its own cap" + ); + } + + #[test] + fn the_default_cap_is_five_turns() { + // The number is the feature's own, not a value tuned here — pinned so + // a later edit to `DEFAULT_MAX_TURNS` has to be deliberate. + assert_eq!(DEFAULT_MAX_TURNS, 5); + } + + #[test] + fn both_signals_stop_the_continuation_before_the_cap_is_reached() { + // The issue's own words: "both stop goal continues". Turn one of + // five, so only the signal can be what stopped it. + for stop in [ + StopReason::GoalReached(Some("wrote the fix".to_owned())), + StopReason::NeedHelp("no credential for the registry".to_owned()), + ] { + let state = mid_run("n", "a goal", 5); + state.record_stop("n", stop.clone()); + assert_eq!( + state.plan_after_turn("n"), + Continuation::Stop(stop.clone()), + "{stop:?} must end the run with turns still on the clock" + ); + assert_eq!( + state.turns("n"), + Some((1, 5)), + "and must not have spent one on the way out" + ); + } + } + + #[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 + // between "it says it finished" and "it ran out of road". + let state = mid_run("n", "a goal", 1); + state.record_stop("n", StopReason::GoalReached(None)); + assert_eq!( + state.plan_after_turn("n"), + Continuation::Stop(StopReason::GoalReached(None)) + ); + } + + #[test] + fn a_signal_needs_a_session_with_a_turn_in_flight() { + // The guard that keeps a signal pointed at its own session: these are + // a running subagent's report about its own turn. + let state = State::new(PathBuf::from("/dev/null"), signal_url()); + let err = need_help(&state, "ghost", "stuck".to_owned(), None) + .expect_err("nothing is running under that name"); + assert!( + err.to_string().contains("check the `name`"), + "the error must point at the likely cause: {err}" + ); + assert!( + state.stop_reason("ghost").is_none(), + "and must not have recorded a stop for a session that isn't there" + ); + + state.reserve("real"); + need_help(&state, "real", "no credential".to_owned(), None).expect("a running session"); + assert_eq!( + state.stop_reason("real"), + Some(StopReason::NeedHelp("no credential".to_owned())) + ); + } + + #[test] + fn need_help_is_a_state_a_parent_can_see_without_reading_anything() { + // Requirement in full: it stops the session *and* shows up in + // `status` as its own state, distinct from idle and from killed. + let blocked = describe_status(&StatusFacts { + stop: Some(StopReason::NeedHelp( + "the brief contradicts the code".to_owned(), + )), + turns: Some((2, 5)), + ..StatusFacts::new("n") + }) + .expect("a blocked session is a state, not an error"); + assert!( + blocked.contains("BLOCKED") && blocked.contains("the brief contradicts the code"), + "the block and its reason must both be in the answer: {blocked}" + ); + assert!( + blocked.contains("Turn 2 of 5"), + "with the progress that says how far it got: {blocked}" + ); + assert!( + !blocked.contains("idle"), + "a blocked subagent must not also read as idle: {blocked}" + ); + } + + #[test] + fn a_reported_goal_never_reads_as_a_verified_one() { + // The failure this is built against: `goal_reached` is self-reported + // by a subagent that has just been told it hasn't reached the goal. + // Every surface that renders it has to say so. + let status = describe_status(&StatusFacts { + stop: Some(StopReason::GoalReached(Some( + "refactored the parser".to_owned(), + ))), + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + status.contains("refactored the parser") && status.contains("claim"), + "status must carry both the report and the fact it's only a report: {status}" + ); + let todo = turn_end_summary( + "n", + &TurnEnd::Complete, + Some(&StopReason::GoalReached(Some( + "refactored the parser".to_owned(), + ))), + ); + assert!( + todo.contains("self-reported, not verified"), + "and so must the todo, which is the half a parent reads unprompted: {todo}" + ); + } + + #[test] + fn a_stop_reason_extends_the_done_message_rather_than_replacing_it() { + // "Extend", not "replace": the observed end of the turn and the + // reason the run stopped are different facts, and dropping the first + // would let a self-reported claim stand where an observation was. + let plain = turn_end_summary("n", &TurnEnd::Complete, None); + for stop in [ + StopReason::GoalReached(None), + StopReason::NeedHelp("blocked".to_owned()), + StopReason::TurnCap { turns: 5 }, + ] { + let extended = turn_end_summary("n", &TurnEnd::Complete, Some(&stop)); + assert!( + extended.starts_with(&plain), + "{stop:?} must extend the done message, not rewrite it: {extended}" + ); + assert!( + extended.len() > plain.len(), + "{stop:?} must actually add something: {extended}" + ); + } + assert_eq!( + turn_end_summary("n", &TurnEnd::Complete, Some(&StopReason::Done)), + plain, + "`Done` has nothing to add — the unextended message already says exactly that" + ); + } + + #[test] + fn the_turn_cap_todo_says_the_harness_limit_was_what_stopped_it() { + // Not a silent stop: the one notification a parent gets unprompted + // has to distinguish "it finished" from "we stopped asking". + let todo = turn_end_summary( + "n", + &TurnEnd::Complete, + Some(&StopReason::TurnCap { turns: 5 }), + ); + assert!( + todo.contains("harness turn limit was reached (5 turns)"), + "the todo must name the limit as the cause: {todo}" + ); + assert!( + todo.contains("without the goal ever being reported reached"), + "and say what that means for the work: {todo}" + ); + } + + #[test] + fn status_reports_the_turn_counter_in_every_state_a_goal_run_reaches() { + for occupancy in [Some(true), Some(false), None] { + let answer = describe_status(&StatusFacts { + occupancy, + session_exists: true, + turns: Some((3, 5)), + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + answer.contains("Turn 3 of 5"), + "progress is the point of the counter — it can't be absent from {occupancy:?}: \ + {answer}" + ); + } + let goalless = describe_status(&StatusFacts { + occupancy: Some(true), + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + !goalless.contains("Turn "), + "a session with no goal has no budget to be partway through: {goalless}" + ); + } + + #[test] + fn a_running_turn_that_has_already_signalled_says_so() { + // The window a parent would otherwise misread: the subagent has said + // it is blocked, its turn hasn't ended yet, and plain "running" would + // tell the parent to leave it alone. + let answer = describe_status(&StatusFacts { + occupancy: Some(true), + stop: Some(StopReason::NeedHelp("no credential".to_owned())), + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + answer.contains("is running") && answer.contains("no credential"), + "both facts are true at once and both have to be in the answer: {answer}" + ); + let quiet = describe_status(&StatusFacts { + occupancy: Some(true), + stop: Some(StopReason::Done), + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + !quiet.contains("already signalled"), + "`Done` is not a signal the subagent raised — nothing to announce: {quiet}" + ); + } + + #[test] + fn a_kill_outranks_a_stop_reason_the_subagent_had_already_raised() { + // Both records can be set at once — a subagent calls `goal_reached` + // and something SIGKILLs it before the turn ends. The observed kill + // is the one that can't be a claim. + let answer = describe_status(&StatusFacts { + killed: Some(libc::SIGKILL), + stop: Some(StopReason::GoalReached(None)), + session_exists: true, + ..StatusFacts::new("n") + }) + .expect("status"); + assert!( + answer.contains("was killed"), + "the kill is what happened: {answer}" + ); + assert!( + !answer.contains("goal reached"), + "a claim made just before being killed must not be the headline: {answer}" + ); + } + + #[test] + fn a_continue_gives_a_capped_session_its_allowance_back() { + // `TurnCap` stops the loop, it doesn't retire the session — and the + // parent spending a turn on purpose is not what the cap bounds. + let state = mid_run("n", "a goal", 2); + state.record_stop("n", StopReason::TurnCap { turns: 2 }); + state.clear_stop("n"); + state.restart_turns("n"); + assert_eq!(state.turns("n"), Some((1, 2))); + assert_eq!(state.stop_reason("n"), None); + assert!( + matches!(state.plan_after_turn("n"), Continuation::Continue { .. }), + "with the allowance back, the loop has a turn to spend again" + ); + } + + #[test] + fn a_fresh_start_inherits_nothing_from_the_run_before_it() { + // `start` archives the prior session precisely so this is a fresh + // start; a leftover stop reason or report path would make the new run + // report the old one's ending, into the old one's file. + let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())); + state.record_stop("n", StopReason::NeedHelp("old block".to_owned())); + state.set_report_file("n", Some("/tmp/old-report.md")); + state.reserve("n"); + let refused = start(&state, start_request("n")); + assert!(refused.is_err(), "the reserved name is refused as before"); + + // The same clearing `start` does once it owns the name. + state.clear_stop("n"); + state.clear_report_file("n"); + assert_eq!(state.stop_reason("n"), None); + assert_eq!(state.report_file("n"), None); + } + + #[test] + fn the_report_path_is_taken_from_the_session_and_never_guessed() { + // Both halves of where it can come from: the brief `start` carried, + // and the subagent saying where it actually wrote. A signal that + // names no path leaves the remembered one alone rather than erasing + // it. + let state = State::new(PathBuf::from("/dev/null"), signal_url()); + assert_eq!( + state.report_file("n"), + None, + "a session nobody told about a report file has none — nothing is inferred" + ); + state.set_report_file("n", Some("/tmp/brief-said.md")); + state.reserve("n"); + goal_reached(&state, "n", None, None).expect("a running session"); + assert_eq!( + state.report_file("n"), + Some(PathBuf::from("/tmp/brief-said.md")), + "a signal with no path must not erase what the brief named" + ); + goal_reached(&state, "n", None, Some("/tmp/actually-wrote.md")).expect("a running session"); + assert_eq!( + state.report_file("n"), + Some(PathBuf::from("/tmp/actually-wrote.md")), + "and the subagent saying where it wrote is what wins" + ); + } + + #[tokio::test] + async fn the_stop_reason_is_appended_to_the_report_file_the_session_named() { + let dir = std::env::temp_dir().join(format!("hive-subagent-report-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let path = dir.join("report.md"); + std::fs::write(&path, "# what the subagent wrote\n").expect("seed the report"); + + write_stop_to_report(Some(path.clone()), "n", &StopReason::TurnCap { turns: 5 }).await; + + let body = std::fs::read_to_string(&path).expect("report still readable"); + assert!( + body.starts_with("# what the subagent wrote\n"), + "appended, never rewritten — the subagent wrote that file: {body}" + ); + assert!( + body.contains("harness turn limit was reached (5 turns)"), + "and the stop reason lands in the artifact a parent already reads: {body}" + ); + std::fs::remove_dir_all(&dir).ok(); + } + + #[tokio::test] + async fn an_unwritable_report_path_costs_the_line_and_nothing_else() { + // Best-effort by design: the same sentence is in the todo, so a path + // this daemon can't write is a missing convenience, not a lost fact. + write_stop_to_report( + Some(PathBuf::from("/proc/definitely/not/writable/report.md")), + "n", + &StopReason::Done, + ) + .await; + write_stop_to_report(None, "n", &StopReason::Done).await; + } + + #[test] + fn a_goal_briefing_tells_the_subagent_what_it_cannot_otherwise_know() { + let briefing = goal_briefing("batch-1", "get the gate to pass", 5); + assert!( + briefing.contains("get the gate to pass") && briefing.contains("up to 5 turns"), + "the goal and the budget both have to reach the subagent: {briefing}" + ); + assert!( + briefing.contains("goal_reached") && briefing.contains("need_help"), + "as do the two ways it has to stop the re-prompting: {briefing}" + ); + assert!( + briefing.contains("batch-1"), + "and its own name, which is what those tools are called with: {briefing}" + ); + } + + #[test] + fn a_signal_url_reaches_the_subagent_and_a_status_check_renders_no_config() { + // The signal surface is how `goal_reached` is callable at all, so a + // spawned turn's config has to carry it; `status` builds a config + // purely to resolve the store and has no subagent to hand it to. + let turn = build_config("n", None, None, None, None, Some(&signal_url())); + let checked = build_config("n", None, None, None, None, None); + assert!( + turn.mcp_config.is_some(), + "a turn's config must carry an --mcp-config with the signal surface in it" + ); + assert!( + turn.strict_mcp_config && checked.strict_mcp_config, + "the safety property is unchanged: no ambient MCP discovery either way" + ); + } } diff --git a/nix/agent-modules/mcp.nix b/nix/agent-modules/mcp.nix index 70b6db16..92465d0d 100644 --- a/nix/agent-modules/mcp.nix +++ b/nix/agent-modules/mcp.nix @@ -349,11 +349,14 @@ in # Subagent task runner daemon — independent of `hive-bash-daemon` (own # crate, own process): spawns nested claude sessions on request, serves # the `start`/`continue`/`status`/`interrupt` MCP tools directly over - # streamable-http on `hyperhive.mcp.subagentHttpPort`. No task files — - # this daemon's only state is an in-memory map of currently-running - # processes, live only as long as the process is (see - # `hive-subagent-mcp/src/session.rs`'s module doc); a restart stops - # whatever's running, the actual claude session survives independently. + # streamable-http on `hyperhive.mcp.subagentHttpPort`. The same port also + # serves a second, subagent-facing route (`/signal/mcp`: + # `goal_reached`/`need_help`) that the daemon hands each subagent it + # spawns — not something an agent's own config points at. No task files — + # this daemon's only state is in-memory, live only as long as the process + # is (see `hive-subagent-mcp/src/session.rs`'s module doc); a restart + # stops whatever's running, the actual claude session survives + # independently. systemd.services.hive-subagent-daemon = { description = "subagent task runner + MCP daemon for hive-subagent"; wantedBy = [ "multi-user.target" ];