diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 0699dc36..79263a4b 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -118,6 +118,15 @@ impl State { /// `running`: a daemon restart forgets it same as it forgets everything /// else here — the caller re-supplies `dir` once, same as it always /// 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. fn resolve_dir(&self, name: &str, dir: Option<&str>) -> Option { let mut dirs = self.dirs.lock().unwrap_or_else(PoisonError::into_inner); match dir { @@ -128,6 +137,25 @@ impl State { None => dirs.get(name).cloned(), } } + + /// Same resolution as `resolve_dir` (explicit wins, omitted falls back + /// to the remembered value) but never writes — for a call that must not + /// change what a later bare `continue`/`status` resolves to. `status`'s + /// own doc offers `dir` as a one-off "check a different directory" + /// knob; if that call committed the same way `start`/`continue_` do, the + /// one-off peek would silently become the new remembered default (the + /// second bug argus flagged in the same review). + fn peek_dir(&self, name: &str, dir: Option<&str>) -> Option { + match dir { + Some(d) => Some(d.to_owned()), + None => self + .dirs + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(name) + .cloned(), + } + } } /// A caller-chosen name, validated the same way `hive-bash-mcp`'s task ids @@ -215,10 +243,12 @@ pub fn start( dir: Option<&str>, ) -> anyhow::Result { validate_name(name)?; - let dir = state.resolve_dir(name, dir); 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); let result = start_reserved(state, name, model, prompt_file, trigger, dir.as_deref()); if result.is_err() { state.release_reservation(name); @@ -276,12 +306,14 @@ pub fn continue_( dir: Option<&str>, ) -> anyhow::Result { validate_name(name)?; - let dir = state.resolve_dir(name, dir); if !state.reserve(name) { anyhow::bail!( "subagent `{name}` is already running — use `interrupt` first if you meant to redirect it" ); } + // 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); let result = continue_reserved(state, name, prompt, model, dir.as_deref()); if result.is_err() { state.release_reservation(name); @@ -374,7 +406,10 @@ fn spawn_and_track( /// An invalid name, or no session — running or on disk — under `name`. pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result { validate_name(name)?; - let dir = state.resolve_dir(name, dir); + // 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); match state.occupancy(name) { Some(true) => return Ok(format!("subagent `{name}` is running")), Some(false) => { @@ -580,6 +615,55 @@ mod tests { ); } + #[test] + fn peek_dir_resolves_like_resolve_dir_but_never_writes() { + let state = State::new(PathBuf::from("/dev/null")); + state.resolve_dir("n", Some("/tmp/remembered")); + assert_eq!( + state.peek_dir("n", Some("/tmp/one-off")), + Some("/tmp/one-off".to_owned()), + "an explicit dir still resolves as given" + ); + assert_eq!( + state.resolve_dir("n", None), + Some("/tmp/remembered".to_owned()), + "but the one-off peek must not have overwritten the remembered value" + ); + } + + #[test] + fn start_does_not_corrupt_the_remembered_dir_when_the_name_is_already_running() { + // The exact scenario argus caught in review: a rejected concurrent + // `start` used to commit its `dir` before the reservation check + // refused it, corrupting what a later bare `continue` would resolve + // 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"))); + state.resolve_dir("dup", Some("/tmp/original")); + assert!( + state.reserve("dup"), + "simulate an in-flight start/continue already owning the name" + ); + let result = start( + &state, + "dup", + None, + "/tmp/prompt.md", + "trigger".to_owned(), + Some("/tmp/rejected"), + ); + assert!( + result.is_err(), + "a name already reserved must be refused, not spawned" + ); + assert_eq!( + state.resolve_dir("dup", None), + Some("/tmp/original".to_owned()), + "the rejected call's dir must not have overwritten the remembered one" + ); + } + #[test] fn build_config_sets_cwd_only_when_a_dir_is_given() { let with = build_config("n", None, None, Some("/tmp/some-worktree"));