hive-subagent-mcp: fix resolve_dir commit ordering + add non-committing peek_dir

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<String> {
         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<String> {
+        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<String> {
     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<String> {
     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<String> {
     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"));