From d03f10822cbf413d1e7ef8e288f1a6fb2cde3919 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 6 Oct 2026 15:33:20 +0200 Subject: [PATCH] hive-subagent-mcp: say so when a resume falls back to the bare name resume_target logs the fallback and returns a notice naming the project dir searched; continue appends it to its reply and to a failed resume's error, so a caller hitting claude's 'matches 2 sessions' knows to pass dir. The test's final assertion now pins the whole attach list so the continue site is covered on its own, and the fallback branch has a test. --- docs/tools/subagent.md | 6 +- hive-subagent-mcp/src/session.rs | 178 +++++++++++++++++++++++++++---- 2 files changed, 159 insertions(+), 25 deletions(-) diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index c79c5c4a..aca7d314 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -71,8 +71,10 @@ 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 and -claude's answer stands — see below. +`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. 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 diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 555ca2e4..e97468fb 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -1120,20 +1120,53 @@ 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. +/// 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. /// /// 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) -> String { - build_store(config) - .ok() +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())) - .unwrap_or_else(|| name.to_owned()) + .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" + )), + } } /// Everything a `start` needs, as one struct rather than a parameter list: @@ -1388,17 +1421,27 @@ pub async fn continue_( 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); - if started.is_err() { - state.release_reservation(name); - return started; - } + let (started, notice) = + match continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict) { + Ok(spawned) => spawned, + Err(e) => { + state.release_reservation(name); + return Err(e); + } + }; // 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)) + // 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(), + )) } /// Hold a `continue` open until its resume is known to have landed — or @@ -1452,6 +1495,15 @@ 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`. @@ -1463,7 +1515,7 @@ fn continue_reserved( effort: Option, dir: Option<&str>, verdict: &VerdictTx, -) -> anyhow::Result { +) -> anyhow::Result<(String, Option)> { // 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); @@ -1473,6 +1525,9 @@ 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 @@ -1483,7 +1538,7 @@ fn continue_on_runtime( config: Config, prompt: String, verdict: &VerdictTx, -) -> anyhow::Result { +) -> anyhow::Result<(String, Option)> { if let SubagentRuntime::Acp { command, sessions } = &state.runtime { let session = acp_session_file(sessions, name); if !session.exists() { @@ -1501,20 +1556,23 @@ 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. - spawn_and_track( + let target = resume_target(&config, name); + let reply = spawn_and_track( state, name, &config, - &Attach::Resume(resume_target(&config, name)), + &Attach::Resume(target.value), prompt, Some(Arc::clone(verdict)), - ) + )?; + Ok((reply, target.fallback_notice)) } /// Bumps `name`'s liveness clock on every line of the turn's output, and @@ -1656,7 +1714,10 @@ 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))) { + match Claude::spawn( + &config, + &Attach::Resume(resume_target(&config, &task_name).value), + ) { Ok(next_running) => { state.track(&task_name, Stopper::Claude(next_running.cancel_handle())); state.note_event(&task_name); @@ -4378,7 +4439,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))); - continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) + let (_, notice) = continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) .expect("the fake claude spawns"); await_resume(rx) .await @@ -4388,9 +4449,80 @@ echo '{"type":"result","subtype":"success","is_error":false}' }) .await; assert_eq!( - attaches().last().map(String::as_str), - Some("resume session-2") + attaches(), + [ + "create session-0", + "create session-1", + "create session-2", + "resume session-2", + "resume session-2" + ], + "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"); + let (here, sibling) = (root.join("here"), root.join("sibling")); + let home = root.join("claude-home"); + for dir in [&here, &sibling, &home] { + std::fs::create_dir_all(dir).expect("scratch subdir"); + } + let program = root.join("session-claude"); + write_executable(&program, SESSION_CLAUDE); + let config = |cwd: &Path| Config { + program: Some(program.clone()), + cwd: Some(cwd.to_path_buf()), + claude_config_dir: Some(home.clone()), + ..Config::default() + }; + let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())); + assert!(state.reserve("n")); + state.clear_stop("n"); + start_on_runtime(&state, "n", config(&sibling), None, "go".into()) + .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; + assert_eq!( + lines(&home.join("attaches")), + ["create session-0", "resume session-0"], + "the name was handed to claude, which found the sibling's session" + ); + 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_eq!(with_resume_notice("m", None), "m"); std::fs::remove_dir_all(&root).ok(); }