diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index e53a465c..b4b7dda5 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -214,8 +214,10 @@ no progress left to describe. `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: +No such session refuses the call — nothing spawns, the name stays free +for a retry, and the session keeps its `status` (stop reason and turn +budget) and its remembered `dir` — and the error names the directory +searched: ``` continue error: no session titled `n` under /home/agent/.claude/projects/-home-agent-work; pass the `dir` the session was started in diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index c263e470..8466f8a0 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -678,13 +678,11 @@ impl State { /// could. /// /// ⚠️ **Callers must only call this once the call is known to proceed** - /// (i.e. after `reserve` has already succeeded in `start`/`continue_`) — - /// see `peek_dir` for the non-committing variant a call that might still - /// be refused (or that shouldn't persist its `dir` at all, like - /// `status`) needs instead. This one argus caught in review: calling it - /// unconditionally before `reserve`'s check meant a *rejected* concurrent - /// call still overwrote the remembered `dir` for the name it was refused - /// against. + /// — after `reserve` in `start`, after the session lookup in a + /// `continue` (`begin_continue`) — so a refused call never changes what + /// a later bare `continue` resolves to. `peek_dir` is the non-committing + /// variant for a call that might still be refused, or that shouldn't + /// persist its `dir` at all, like `status`. fn resolve_dir(&self, name: &str, dir: Option<&str>) -> Option { let mut dirs = self.dirs.lock().unwrap_or_else(PoisonError::into_inner); match dir { @@ -778,6 +776,16 @@ impl State { } } + /// Commit a `continue` whose session has been found: remember its `dir` + /// (see `resolve_dir`), drop the stop reason that described the run + /// before it, and give a goal session its allowance back (see + /// `restart_turns`). + fn begin_continue(&self, name: &str, dir: Option<&str>) { + self.resolve_dir(name, dir); + self.clear_stop(name); + self.restart_turns(name); + } + /// `(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. @@ -1389,23 +1397,15 @@ pub async fn continue_( // Read before the spawn, which clears the record as soon as the new turn // is confirmed — this reply is the last chance to mention it. let killed = state.killed_by(name); - // 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 = - match continue_reserved(state, name, prompt, model, effort, dir.as_deref(), &verdict) { - Ok(spawned) => spawned, - Err(e) => { - state.release_reservation(name); - return Err(e); - } - }; + let started = match continue_reserved(state, name, prompt, model, effort, dir, &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 @@ -1467,7 +1467,8 @@ fn note_resumed_after_kill(msg: &str, killed: Option) -> String { /// The slow, fallible part of `continue_`, run only after `reserve` has /// already closed the TOCTOU window — same split rationale as -/// `start_reserved`. +/// `start_reserved`. `dir` is the caller's own, not yet resolved against the +/// remembered one. fn continue_reserved( state: &Arc, name: &str, @@ -1480,12 +1481,24 @@ fn continue_reserved( // 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); - let config = build_config(name, model, effort, None, dir, Some(&signal_url)); - continue_on_runtime(state, name, config, prompt, verdict) + let resolved = state.peek_dir(name, dir); + let config = build_config( + name, + model, + effort, + None, + resolved.as_deref(), + Some(&signal_url), + ); + continue_on_runtime(state, name, config, dir, prompt, verdict) } /// Resume `name`'s session on the daemon's runtime for one more run. /// +/// The session is looked up before any of `name`'s records change, so a +/// refused call leaves `status` and the remembered `dir` as they were; only +/// a session that is there gets `State::begin_continue`. +/// /// # Errors /// /// On claude, a name with no session in `config`'s project dir (see @@ -1496,6 +1509,7 @@ fn continue_on_runtime( state: &Arc, name: &str, config: Config, + dir: Option<&str>, prompt: String, verdict: &VerdictTx, ) -> anyhow::Result { @@ -1508,6 +1522,7 @@ fn continue_on_runtime( session.display() ); } + state.begin_continue(name, dir); let runtime = acp_runtime(command.clone(), sessions, name); return spawn_and_track_acp( state, @@ -1523,6 +1538,7 @@ fn continue_on_runtime( // for a session that vanishes between the lookup and the spawn. See this // module's doc. let id = resume_target(&config, name)?; + state.begin_continue(name, dir); spawn_and_track( state, name, @@ -4396,7 +4412,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) + continue_on_runtime(&state, "n", config(&here), None, "again".into(), &verdict) .expect("the fake claude spawns"); await_resume(rx) .await @@ -4448,8 +4464,9 @@ 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 refused = continue_on_runtime(&state, "n", config(&here), "again".into(), &verdict) - .expect_err("no session titled `n` under `here`"); + let refused = + continue_on_runtime(&state, "n", config(&here), None, "again".into(), &verdict) + .expect_err("no session titled `n` under `here`"); assert_eq!( refused.to_string(), format!( @@ -4472,6 +4489,46 @@ echo '{"type":"result","subtype":"success","is_error":false}' std::fs::remove_dir_all(&root).ok(); } + #[test] + fn a_refused_continue_leaves_the_sessions_records_as_they_were() { + // A capped goal session on its last turn, stopped on a signal, with a + // remembered `dir`; the `continue` names a dir holding no session. + let root = scratch_dir("refused-keeps-records"); + let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url())); + state.set_goal("n", Some("g".into()), 3); + state.plan_after_turn("n"); + state.plan_after_turn("n"); + state.record_stop("n", StopReason::NeedHelp("blocked on X".into())); + state.resolve_dir("n", Some("/remembered")); + assert_eq!(state.turns("n"), Some((3, 3))); + + assert!(state.reserve("n")); + let (tx, _rx) = oneshot::channel(); + let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); + continue_on_runtime( + &state, + "n", + claude_config(&root), + root.to_str(), + "again".into(), + &verdict, + ) + .expect_err("no session under the scratch dir"); + + assert_eq!( + state.stop_reason("n"), + Some(StopReason::NeedHelp("blocked on X".into())), + "`status` still says why the session stopped" + ); + assert_eq!(state.turns("n"), Some((3, 3)), "the budget is not reset"); + assert_eq!( + state.peek_dir("n", None), + Some("/remembered".to_owned()), + "the refused call's dir is not remembered" + ); + std::fs::remove_dir_all(&root).ok(); + } + #[tokio::test] async fn an_acp_parent_runs_its_subagents_on_its_acp_agent() { let dir = scratch_dir("runtime-acp"); @@ -4509,6 +4566,7 @@ echo '{"type":"result","subtype":"success","is_error":false}' &state, "n", claude_config(&dir), + None, "TRIGGER-TWO".into(), &verdict, ) @@ -4542,8 +4600,15 @@ echo '{"type":"result","subtype":"success","is_error":false}' let (tx, _rx) = oneshot::channel(); let verdict: VerdictTx = Arc::new(Mutex::new(Some(tx))); - let err = continue_on_runtime(&state, "n", claude_config(&dir), "go".into(), &verdict) - .expect_err("nothing to resume"); + let err = continue_on_runtime( + &state, + "n", + claude_config(&dir), + None, + "go".into(), + &verdict, + ) + .expect_err("nothing to resume"); assert!(err.to_string().contains("no ACP session"), "{err}"); assert!( !dir.join("prompts.methods").exists(),