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

hive-subagent-mcp/src/mcp.rs

@@ -37,6 +37,17 @@ struct StartArgs {
     /// belongs in `prompt_file`, not here.
     #[serde(default = "default_trigger")]
     trigger: String,
+    /// Working directory for the subagent's session — e.g. a git worktree
+    /// you've already prepared for it, so a parallel batch of subagents
+    /// never race on the same working tree. Must exist. Omit to inherit
+    /// this daemon's own working directory (today's default). The daemon
+    /// remembers whichever `dir` you give here against `name`, so a later
+    /// `continue`/`status` for the same name doesn't need to repeat it —
+    /// only pass it there again if you want to point at a *different*
+    /// directory. Forgotten on a daemon restart, same as everything else
+    /// this daemon tracks in memory.
+    #[serde(default)]
+    dir: Option<String>,
 }
 
 fn default_trigger() -> String {
@@ -53,12 +64,24 @@ struct ContinueArgs {
     /// default — this does not have to match whatever model `start` used.
     #[serde(default)]
     model: Option<String>,
+    /// Omit to reuse whatever `dir` `start` (or a prior `continue`) used for
+    /// this name — the daemon remembers it. Only pass this to point the
+    /// session at a *different* directory than last time.
+    #[serde(default)]
+    dir: Option<String>,
 }
 
 #[derive(Debug, Deserialize, JsonSchema)]
 struct StatusArgs {
     /// The subagent name to check.
     name: String,
+    /// Omit to use whatever `dir` was last remembered for this name (see
+    /// `start`'s `dir` doc) — you only need this if nothing's running or
+    /// reserved for `name` right now (the common "is it running" case never
+    /// even looks at it) *and* you want to check a different directory's
+    /// session than the one last remembered.
+    #[serde(default)]
+    dir: Option<String>,
 }
 
 #[derive(Debug, Deserialize, JsonSchema)]
@@ -95,6 +118,7 @@ impl SubagentMcp {
             args.model,
             &args.prompt_file,
             args.trigger,
+            args.dir.as_deref(),
         ) {
             Ok(msg) => msg,
             Err(e) => format!("start error: {e:#}"),
@@ -110,7 +134,13 @@ impl SubagentMcp {
             `start`. Refuses a name with no session on disk at all, or one already running."
     )]
     fn r#continue(&self, Parameters(args): Parameters<ContinueArgs>) -> String {
-        match session::continue_(&self.state, &args.name, args.prompt, args.model) {
+        match session::continue_(
+            &self.state,
+            &args.name,
+            args.prompt,
+            args.model,
+            args.dir.as_deref(),
+        ) {
             Ok(msg) => msg,
             Err(e) => format!("continue error: {e:#}"),
         }
@@ -138,7 +168,7 @@ impl SubagentMcp {
             `continue` to give it another turn), and no such session at all."
     )]
     fn status(&self, Parameters(args): Parameters<StatusArgs>) -> String {
-        match session::status(&self.state, &args.name) {
+        match session::status(&self.state, &args.name, args.dir.as_deref()) {
             Ok(msg) => msg,
             Err(e) => format!("status error: {e:#}"),
         }

hive-subagent-mcp/src/session.rs

@@ -3,13 +3,17 @@
 //!
 //! **No task files, no restart recovery.** The daemon's only state is an
 //! in-memory `name -> Option<Cancel>` map (see `State`'s own doc for what
-//! the `None`/`Some` split is for), live for exactly as long as the process
+//! the `None`/`Some` split is for) plus a `name -> dir` memory so `start`'s
+//! `dir` doesn't have to be repeated on every later `continue`/`status`
+//! (`State::resolve_dir`) — both live for exactly as long as the process
 //! is — a daemon restart means whatever was running gets killed with it
-//! (`tokio`'s own child-process drop semantics), not adopted. The durable
+//! (`tokio`'s own child-process drop semantics), not adopted, and the
+//! remembered `dir` is gone too. The durable
 //! record of a subagent's existence is `hive_claude::SessionStore` — claude's
 //! own on-disk session, found again by name. `continue` is how a caller
 //! reattaches to it, whether that's "give it a new turn" or "the daemon
-//! restarted and I want to pick this back up."
+//! restarted and I want to pick this back up" — after a restart, re-supply
+//! `dir` once if the session isn't in the daemon's own default directory.
 //!
 //! **No mid-turn compaction.** Building on `hive_claude::Claude::spawn` +
 //! `RunningClaude::wait` directly (not `InfiniteSession::run`) is what makes
@@ -41,8 +45,17 @@ use hive_claude::{Attach, Cancel, Claude, Config, NoopSink, SessionStore};
 /// reviewer caught in the original check-then-insert version: checking "is
 /// `name` free" and committing to it are two different lock acquisitions
 /// unless the check *is* the reservation — see `reserve`.
+///
+/// ⚠️ This concurrency guard is keyed by `name` alone — a `start`/`continue`
+/// for `name` with a *different* `dir` than one already in flight under
+/// that name is refused as "already running," even though the two would
+/// resolve to entirely separate on-disk sessions. Deliberate: `name` is the
+/// caller's one chosen identity for a subagent, not `(name, dir)` — reuse a
+/// name across directories at your own risk, the tool doesn't disambiguate
+/// it (flagged in review when `dir` was added).
 pub struct State {
     running: Mutex<HashMap<String, Option<Cancel>>>,
+    dirs: Mutex<HashMap<String, String>>,
     socket: PathBuf,
 }
 
@@ -51,6 +64,7 @@ impl State {
     pub fn new(socket: PathBuf) -> Self {
         Self {
             running: Mutex::new(HashMap::new()),
+            dirs: Mutex::new(HashMap::new()),
             socket,
         }
     }
@@ -92,6 +106,56 @@ impl State {
             running.remove(name);
         }
     }
+
+    /// An explicit `dir` is remembered against `name` and returned as-is; an
+    /// omitted one (`None`) falls back to whatever was last remembered for
+    /// `name`, so a `start` that gave a `dir` doesn't force every later
+    /// `continue`/`status` against the same name to repeat it. Explicit
+    /// always wins and updates the memory — there's no way to say "forget
+    /// it, use the default" once something's been remembered short of
+    /// giving a genuinely different `dir`, a deliberate simplicity trade,
+    /// not an oversight. In-memory only, same durability envelope as
+    /// `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 {
+            Some(d) => {
+                dirs.insert(name.to_owned(), d.to_owned());
+                Some(d.to_owned())
+            }
+            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
@@ -118,9 +182,17 @@ fn subagent_otel_attrs(name: &str) -> String {
 
 /// Build the `Config` one subagent turn runs against. `prompt_file`, when
 /// given, becomes `--append-system-prompt-file` — the subagent's task
-/// instructions. Always `--dangerously-skip-permissions --strict-mcp-config`
-/// (no `--mcp-config` override — a safety property, not a knob).
-fn build_config(name: &str, model: Option<String>, prompt_file: Option<&str>) -> Config {
+/// instructions. `dir`, when given, becomes `Config::cwd` (e.g. a worktree
+/// the caller already prepared); `None` inherits this daemon's own working
+/// directory, same as before this field existed. Always
+/// `--dangerously-skip-permissions --strict-mcp-config` (no `--mcp-config`
+/// override — a safety property, not a knob).
+fn build_config(
+    name: &str,
+    model: Option<String>,
+    prompt_file: Option<&str>,
+    dir: Option<&str>,
+) -> Config {
     let mut extra_args = vec!["--dangerously-skip-permissions".to_owned()];
     if let Some(path) = prompt_file {
         extra_args.push("--append-system-prompt-file".to_owned());
@@ -128,6 +200,7 @@ fn build_config(name: &str, model: Option<String>, prompt_file: Option<&str>) ->
     }
     Config {
         model,
+        cwd: dir.map(PathBuf::from),
         strict_mcp_config: true,
         extra_args,
         env: vec![(
@@ -167,12 +240,16 @@ pub fn start(
     model: Option<String>,
     prompt_file: &str,
     trigger: String,
+    dir: Option<&str>,
 ) -> anyhow::Result<String> {
     validate_name(name)?;
     if !state.reserve(name) {
         anyhow::bail!("subagent `{name}` is already running — use `continue` or `interrupt`");
     }
-    let result = start_reserved(state, name, model, prompt_file, trigger);
+    // 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);
     }
@@ -189,8 +266,9 @@ fn start_reserved(
     model: Option<String>,
     prompt_file: &str,
     trigger: String,
+    dir: Option<&str>,
 ) -> anyhow::Result<String> {
-    let config = build_config(name, model, Some(prompt_file));
+    let config = build_config(name, model, Some(prompt_file), dir);
     let store = build_store(&config)?;
     if store.find_by_title(name).is_some() {
         tracing::info!(
@@ -225,6 +303,7 @@ pub fn continue_(
     name: &str,
     prompt: String,
     model: Option<String>,
+    dir: Option<&str>,
 ) -> anyhow::Result<String> {
     validate_name(name)?;
     if !state.reserve(name) {
@@ -232,7 +311,10 @@ pub fn continue_(
             "subagent `{name}` is already running — use `interrupt` first if you meant to redirect it"
         );
     }
-    let result = continue_reserved(state, name, prompt, model);
+    // 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);
     }
@@ -247,8 +329,9 @@ fn continue_reserved(
     name: &str,
     prompt: String,
     model: Option<String>,
+    dir: Option<&str>,
 ) -> anyhow::Result<String> {
-    let config = build_config(name, model, None);
+    let config = build_config(name, model, None, dir);
     let store = build_store(&config)?;
     if store.find_by_title(name).is_none() {
         anyhow::bail!(
@@ -321,8 +404,12 @@ fn spawn_and_track(
 /// # Errors
 ///
 /// An invalid name, or no session — running or on disk — under `name`.
-pub fn status(state: &State, name: &str) -> anyhow::Result<String> {
+pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result<String> {
     validate_name(name)?;
+    // 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) => {
@@ -332,7 +419,7 @@ pub fn status(state: &State, name: &str) -> anyhow::Result<String> {
         }
         None => {}
     }
-    let config = build_config(name, None, None);
+    let config = build_config(name, None, None, dir.as_deref());
     let store = build_store(&config)?;
     if store.find_by_title(name).is_some() {
         Ok(format!(
@@ -472,18 +559,117 @@ mod tests {
 
     #[test]
     fn build_config_only_appends_system_prompt_when_given() {
-        let with = build_config("n", None, Some("/tmp/p.md"));
+        let with = build_config("n", None, Some("/tmp/p.md"), None);
         assert!(
             with.extra_args
                 .contains(&"--append-system-prompt-file".to_owned())
         );
         assert!(with.extra_args.contains(&"/tmp/p.md".to_owned()));
 
-        let without = build_config("n", None, None);
+        let without = build_config("n", None, None, None);
         assert!(
             !without
                 .extra_args
                 .contains(&"--append-system-prompt-file".to_owned())
         );
     }
+
+    #[test]
+    fn resolve_dir_remembers_an_explicit_dir_and_falls_back_to_it_when_omitted() {
+        let state = State::new(PathBuf::from("/dev/null"));
+        assert_eq!(
+            state.resolve_dir("n", None),
+            None,
+            "nothing remembered yet — omitted dir has nothing to fall back to"
+        );
+        assert_eq!(
+            state.resolve_dir("n", Some("/tmp/worktree")),
+            Some("/tmp/worktree".to_owned()),
+            "an explicit dir is returned as-is"
+        );
+        assert_eq!(
+            state.resolve_dir("n", None),
+            Some("/tmp/worktree".to_owned()),
+            "a later omitted dir falls back to what was just remembered"
+        );
+        assert_eq!(
+            state.resolve_dir("n", Some("/tmp/other")),
+            Some("/tmp/other".to_owned()),
+            "a later explicit dir overrides the memory, not just reads it"
+        );
+        assert_eq!(
+            state.resolve_dir("n", None),
+            Some("/tmp/other".to_owned()),
+            "the fallback now reflects the override"
+        );
+    }
+
+    #[test]
+    fn resolve_dir_does_not_cross_names() {
+        let state = State::new(PathBuf::from("/dev/null"));
+        state.resolve_dir("a", Some("/tmp/a"));
+        assert_eq!(
+            state.resolve_dir("b", None),
+            None,
+            "an unrelated name's memory must stay empty"
+        );
+    }
+
+    #[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"));
+        assert_eq!(with.cwd, Some(PathBuf::from("/tmp/some-worktree")));
+
+        let without = build_config("n", None, None, None);
+        assert_eq!(without.cwd, None);
+    }
 }