From 0e015197b61a6d387ec1ffc8b04b372a8a6ec42e Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 7 Oct 2026 11:55:34 +0200 Subject: [PATCH] hive-subagent-mcp: refuse a continue whose session is not in the dir searched resume_target no longer falls back to the bare name when the project dir holds no session titled it: continue (and a goal run's next turn) refuses before anything spawns, naming the directory searched. A store that cannot be resolved refuses too, with the io error. The fallback notice is gone. --- docs/tools/subagent.md | 41 +++--- hive-subagent-mcp/src/mcp.rs | 11 +- hive-subagent-mcp/src/session.rs | 218 ++++++++++++------------------- 3 files changed, 114 insertions(+), 156 deletions(-) diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index aca7d314..e53a465c 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -71,10 +71,8 @@ project directory, read off disk at every resume. claude looks a bare name up across every git worktree of the repo and refuses when two sessions match, and `start` archives a prior session only in its own directory, so a name reused under a sibling worktree would otherwise stop resuming. When -`dir` holds no session under the name, the name goes to claude as-is, -the daemon logs it, and `continue`'s reply — or its error, when claude refuses -— carries a note naming the directory searched; claude's answer stands, see -below. +`dir` holds no session under the name, the daemon refuses and never hands +claude the bare name — see below. Because the remembered directory goes with the rest of it, a `continue` after a restart has to re-supply `dir` when the session lives anywhere @@ -213,20 +211,33 @@ no progress left to describe. ## A `continue` that finds no session - +`continue` looks for the session before it spawns anything. The lookup is +the live session titled the name in the project directory of the `dir` in +effect: the one passed, or else the one the daemon remembers for the name. +No such session refuses the call — nothing spawns and the name stays free +for a retry — and the error names the directory searched: -`continue` doesn't check for the session before spawning. claude's own -`--resume` is the authority, and it exits non-zero rather than quietly -starting a fresh session, so the check could only duplicate the lookup -the driver was about to do — while answering as though the session were -gone. The usual truth is that the session exists somewhere else. +``` +continue error: no session titled `n` under /home/agent/.claude/projects/-home-agent-work; pass the `dir` the session was started in +``` - +A name is never searched for in any other directory, because claude would +resolve a bare title across every worktree of the repo and could pick, or +refuse, the wrong session. A session started in another directory +needs that directory passed as `dir` — and after a daemon restart, which +forgets the remembered one, a session outside the daemon's own working +directory needs it again. A store location the daemon can't resolve +refuses the same way, with the io error. -`continue` waits for that answer instead. Where `start` returns the -instant the process exists — it creates its session, so the spawn -succeeding is the whole story — a resumed turn can fail a moment _after_ -a pid exists, and a pid is no proof that turn began. `continue` holds the +A goal run's next turn does the same lookup; a miss there ends the run as +a failed turn, with the same message in its end-of-turn todo. + +The refusal covers a session missing from the directory searched. +It can't cover one that vanishes between the lookup and the spawn, so +`continue` still waits for claude's answer instead of returning the +instant the process exists. Where `start` returns at the spawn — it +creates its session, so the spawn succeeding is the whole story — a +resumed turn can fail a moment _after_ a pid exists. `continue` holds the tool call open until the turn is underway or the resume has come back missed, and reports a miss as the call's own error, carrying claude's message plus the location this daemon searched: diff --git a/hive-subagent-mcp/src/mcp.rs b/hive-subagent-mcp/src/mcp.rs index 67f0825a..97f8159b 100644 --- a/hive-subagent-mcp/src/mcp.rs +++ b/hive-subagent-mcp/src/mcp.rs @@ -164,8 +164,8 @@ struct ContinueArgs { /// is exactly when you're most likely to be reaching for `continue`. So /// pass it when pointing the session at a *different* directory than /// last time, and pass it again after a restart if the session lives - /// anywhere other than the daemon's own working directory. A resume that - /// finds nothing says which directory it searched. + /// anywhere other than the daemon's own working directory. A name with no + /// session in the directory searched is refused, naming it. #[serde(default)] dir: Option, } @@ -245,9 +245,10 @@ impl SubagentMcp { of the daemon that spawned it). Returns once the turn is underway rather than the \ instant the process exists — a second or so, not the length of the turn — so a \ reply saying the turn started means it started; a name already running is refused. \ - A name with no session to resume is not refused up front, because claude's own \ - `--resume` decides that: a miss comes back as this call's own error, naming the \ - directory that was searched, so check `dir` before concluding the session is gone. \ + A name with no session in the directory searched is refused up front, before \ + anything spawns, and the error names that directory — the session is only found \ + under the `dir` it was started in (or the one this daemon remembers for the name, \ + which a restart forgets), so pass `dir` before concluding the session is gone. \ Resuming a session whose last turn was killed is allowed — the reply says so, since \ that turn's work stopped wherever it had got to." )] diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index e97468fb..c263e470 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -23,11 +23,11 @@ //! age — the timestamp says the child is alive, never what it said. What it //! said goes to the swarm as the subagent's terminal (`crate::swarm_term`). //! -//! **`continue` doesn't pre-check the session's existence — it waits for -//! the answer instead.** claude's own `--resume` is the authority, so -//! `continue` holds its tool call open for up to `RESUME_GRACE` and reports -//! a missed resume as its own error, naming the directory searched -//! (`classify_end`). See `docs/tools/subagent.md`. +//! **`continue` refuses a name with no session in the `dir` in effect +//! (`resume_target`), and waits for the driver's answer on the rest.** A +//! turn that still misses is the call's own error, naming the directory +//! searched (`classify_end`), within `RESUME_GRACE`. 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 @@ -1120,53 +1120,32 @@ fn build_store(config: &Config) -> std::io::Result { )) } -/// What a resume hands `--resume`, and whether that was a fallback the -/// caller should be told about. -struct ResumeTarget { - /// The `--resume` value. - value: String, - /// Set when `value` is the bare name because the project dir held no - /// session titled it: what the caller needs to know to recover. - fallback_notice: Option, -} - -/// What a resume of `name` hands `--resume`: the id of the live session -/// titled `name` in `config`'s own project dir, or `name` itself when that -/// dir has none, so a miss is still claude's to report. The by-name fallback -/// is logged and carries a notice naming the dir searched. +/// The session id a resume of `name` hands `--resume`: the live session +/// titled `name` in `config`'s own project dir. /// /// claude resolves a *title* across the project dirs of every git worktree /// of the repo, and refuses when more than one matches; `start` archives only /// in its own cwd, so a name reused under a sibling worktree's `dir` stays -/// live there. An id names one file. -fn resume_target(config: &Config, name: &str) -> ResumeTarget { - let store = build_store(config).ok(); - let found = store - .as_ref() - .and_then(|store| store.find_by_title(name)) - .and_then(|path| path.file_stem().map(|id| id.to_string_lossy().into_owned())); - if let Some(id) = found { - return ResumeTarget { - value: id, - fallback_notice: None, - }; - } - let searched = store.map_or_else( - || "a location that could not be resolved".to_owned(), - |store| store.project_dir().display().to_string(), - ); - tracing::warn!( - name = %name, - searched = %searched, - "subagent: no session found to resume by id; resuming by name" - ); - ResumeTarget { - value: name.to_owned(), - fallback_notice: Some(format!( - "no session titled `{name}` was found under {searched}, so it was resumed by name — \ - if it was started in another directory, pass the `dir` it was started in" - )), - } +/// live there. An id names one file, and a name is never handed over in its +/// place: a session that is not in this dir is refused, not looked for +/// elsewhere. +/// +/// # Errors +/// +/// A store that cannot be resolved, or a project dir with no session titled +/// `name`; either message names what was searched. +fn resume_target(config: &Config, name: &str) -> anyhow::Result { + let store = build_store(config) + .map_err(|e| anyhow::anyhow!("no session titled `{name}` can be looked for: {e}"))?; + store + .find_by_title(name) + .and_then(|path| path.file_stem().map(|id| id.to_string_lossy().into_owned())) + .ok_or_else(|| { + anyhow::anyhow!( + "no session titled `{name}` under {}; pass the `dir` the session was started in", + store.project_dir().display() + ) + }) } /// Everything a `start` needs, as one struct rather than a parameter list: @@ -1368,18 +1347,15 @@ fn start_on_runtime( /// means "the previous turn finished, here's the next instruction" or "the /// daemon restarted, reattaching." Refuses a name already running. /// -/// A name with **no** session to resume is still not *pre-checked* here: -/// claude's own `--resume` answers that, and the pre-check that used to live -/// here could only repeat the lookup the driver was about to do anyway, -/// while telling the caller a session didn't exist when the true answer was -/// almost always that it exists somewhere else. What changed is that -/// `continue` now waits for the driver's answer instead of returning ahead -/// of it: a missed resume comes back as this call's own `Err`, carrying -/// claude's message and the directory that was searched (`classify_end`), -/// rather than only as an end-of-turn todo the caller had already stopped -/// looking for. A `continue` that reports "started" therefore means the turn -/// started. See `await_resume` for the bound, and `spawn_and_track`'s doc for -/// why `start` keeps the older, unconditional contract. +/// A name with no session titled it in the project dir of the `dir` in +/// effect is refused before anything spawns, naming the directory searched +/// (`resume_target`): the session is only ever resumed by its id, from that +/// dir. A turn that still misses — the session vanished between the lookup +/// and the spawn — comes back as this call's own `Err` too, because +/// `continue` waits for the driver's answer instead of returning ahead of it +/// (`classify_end`). A `continue` that reports "started" therefore means the +/// turn started. See `await_resume` for the bound, and `spawn_and_track`'s +/// doc for why `start` keeps the older, unconditional contract. /// /// Resuming a session whose last turn was *killed* is allowed — that is /// often exactly what the caller wants — but never silent: the reply says @@ -1392,8 +1368,9 @@ fn start_on_runtime( /// # Errors /// /// An invalid name, one already running, a `model` the operator hasn't made -/// available (see `check_model`), `Claude::spawn` failing, or the resumed -/// turn failing within `RESUME_GRACE` — in practice a missed resume. +/// available (see `check_model`), no session titled `name` in the project +/// dir searched, `Claude::spawn` failing, or the resumed turn failing within +/// `RESUME_GRACE`. pub async fn continue_( state: &Arc, name: &str, @@ -1421,7 +1398,7 @@ pub async fn continue_( state.restart_turns(name); let (tx, rx) = oneshot::channel(); let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); - let (started, notice) = + let started = match continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict) { Ok(spawned) => spawned, Err(e) => { @@ -1433,15 +1410,8 @@ pub async fn continue_( // 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`. - // The notice rides on the error too: a by-name resume is where claude - // answers "matches 2 sessions", and the caller needs to know to pass `dir`. - if let Err(e) = await_resume(rx).await { - anyhow::bail!("{}", with_resume_notice(&e.to_string(), notice.as_deref())); - } - Ok(with_resume_notice( - ¬e_resumed_after_kill(&started, killed), - notice.as_deref(), - )) + await_resume(rx).await?; + Ok(note_resumed_after_kill(&started, killed)) } /// Hold a `continue` open until its resume is known to have landed — or @@ -1495,15 +1465,6 @@ fn note_resumed_after_kill(msg: &str, killed: Option) -> String { } } -/// Append the by-name-resume notice (see `ResumeTarget`) to a `continue`'s -/// reply or error; pass `msg` through unchanged when there is none. -fn with_resume_notice(msg: &str, notice: Option<&str>) -> String { - match notice { - None => msg.to_owned(), - Some(notice) => format!("{msg} — note: {notice}"), - } -} - /// The slow, fallible part of `continue_`, run only after `reserve` has /// already closed the TOCTOU window — same split rationale as /// `start_reserved`. @@ -1515,7 +1476,7 @@ fn continue_reserved( effort: Option, dir: Option<&str>, verdict: &VerdictTx, -) -> anyhow::Result<(String, Option)> { +) -> anyhow::Result { // A fresh URL for the resumed run, not the one the last run was handed: // a token is per run, and this is a new one. let signal_url = state.mint_signal_url(name); @@ -1525,20 +1486,19 @@ fn continue_reserved( /// Resume `name`'s session on the daemon's runtime for one more run. /// -/// Returns the reply and, when the session could not be found by id, the -/// notice that goes with it. -/// /// # Errors /// -/// On ACP, a name with no recorded session: the ACP runtime would start a -/// new one rather than fail, so the check has to happen here. +/// On claude, a name with no session in `config`'s project dir (see +/// `resume_target`). On ACP, a name with no recorded session: the ACP +/// runtime would start a new one rather than fail, so the check has to +/// happen here. fn continue_on_runtime( state: &Arc, name: &str, config: Config, prompt: String, verdict: &VerdictTx, -) -> anyhow::Result<(String, Option)> { +) -> anyhow::Result { if let SubagentRuntime::Acp { command, sessions } = &state.runtime { let session = acp_session_file(sessions, name); if !session.exists() { @@ -1556,23 +1516,21 @@ fn continue_on_runtime( config, prompt, Some(Arc::clone(verdict)), - ) - .map(|reply| (reply, None)); + ); } - // 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 - // caller in time to be its error. See this module's doc. - let target = resume_target(&config, name); - let reply = spawn_and_track( + // The lookup in `resume_target` refuses before anything spawns. `verdict` + // still carries claude's own answer back in time to be the call's error, + // for a session that vanishes between the lookup and the spawn. See this + // module's doc. + let id = resume_target(&config, name)?; + spawn_and_track( state, name, &config, - &Attach::Resume(target.value), + &Attach::Resume(id), prompt, Some(Arc::clone(verdict)), - )?; - Ok((reply, target.fallback_notice)) + ) } /// Bumps `name`'s liveness clock on every line of the turn's output, and @@ -1714,10 +1672,9 @@ fn spawn_and_track( let Some(next) = after_turn(&state, &task_name, end, verdict.as_ref()).await else { return; }; - match Claude::spawn( - &config, - &Attach::Resume(resume_target(&config, &task_name).value), - ) { + match resume_target(&config, &task_name) + .and_then(|id| Ok(Claude::spawn(&config, &Attach::Resume(id))?)) + { Ok(next_running) => { state.track(&task_name, Stopper::Claude(next_running.cancel_handle())); state.note_event(&task_name); @@ -4439,7 +4396,7 @@ echo '{"type":"result","subtype":"success","is_error":false}' assert!(state.reserve("n")); let (tx, rx) = oneshot::channel(); let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); - let (_, notice) = continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) + continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) .expect("the fake claude spawns"); await_resume(rx) .await @@ -4459,16 +4416,14 @@ echo '{"type":"result","subtype":"success","is_error":false}' ], "the continue must add its own resume of the session, not of the title" ); - assert_eq!(notice, None, "an id was found, so nothing was a fallback"); std::fs::remove_dir_all(&root).ok(); } #[tokio::test] - async fn resuming_where_no_session_is_on_disk_falls_back_to_the_name_and_says_so() { - // `n` lives only in the sibling worktree; resuming from `here` finds - // no id and hands claude the bare name, which claude resolves across - // worktrees. - let root = scratch_dir("resume-fallback"); + async fn resuming_where_no_session_is_on_disk_is_refused_without_spawning() { + // `n` lives only in the sibling worktree, where claude would resolve + // the bare title; resuming from `here` must refuse instead. + let root = scratch_dir("resume-refused"); let (here, sibling) = (root.join("here"), root.join("sibling")); let home = root.join("claude-home"); for dir in [&here, &sibling, &home] { @@ -4489,40 +4444,31 @@ echo '{"type":"result","subtype":"success","is_error":false}' .expect("the fake claude spawns"); until("the run ends", || state.last_event_age("n").is_none()).await; - let target = resume_target(&config(&here), "n"); - assert_eq!(target.value, "n", "no session here, so the bare name"); let searched = SessionStore::new(&home, &here).project_dir(); - let notice = target.fallback_notice.expect("a fallback says so"); - assert!( - notice.contains("no session titled `n`") - && notice.contains(&searched.display().to_string()) - && notice.contains("resumed by name") - && notice.contains("pass the `dir`"), - "{notice}" - ); - assert!(state.reserve("n")); let (tx, rx) = oneshot::channel(); let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); - let (_, carried) = - continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) - .expect("the fake claude spawns"); - await_resume(rx).await.expect("one title match resumes"); - until("the continue's turn ends", || { - state.last_event_age("n").is_none() - }) - .await; + let refused = continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) + .expect_err("no session titled `n` under `here`"); + assert_eq!( + refused.to_string(), + format!( + "no session titled `n` under {}; pass the `dir` the session was started in", + searched.display() + ) + ); + drop(rx); assert_eq!( lines(&home.join("attaches")), - ["create session-0", "resume session-0"], - "the name was handed to claude, which found the sibling's session" + ["create session-0"], + "claude was not spawned, so nothing attached" ); - assert_eq!(carried.as_deref(), Some(notice.as_str())); - assert_eq!( - with_resume_notice("subagent `n` started", carried.as_deref()), - format!("subagent `n` started — note: {notice}") + assert!(state.last_event_age("n").is_none(), "no turn is tracked"); + + assert!( + resume_target(&config(&sibling), "n").is_ok(), + "the directory it was started in still resumes" ); - assert_eq!(with_resume_notice("m", None), "m"); std::fs::remove_dir_all(&root).ok(); }