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" ];