diff --git a/hive-subagent-mcp/src/mcp.rs b/hive-subagent-mcp/src/mcp.rs index e8d5491e..f0f3be86 100644 --- a/hive-subagent-mcp/src/mcp.rs +++ b/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, } 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, + /// 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, } #[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, } #[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) -> 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) -> 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:#}"), } diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 8b2c5e90..79263a4b 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/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` 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>>, + dirs: Mutex>, 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 { + 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 { + 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, 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, + 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, 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, prompt_file: &str, trigger: String, + dir: Option<&str>, ) -> anyhow::Result { 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, prompt_file: &str, trigger: String, + dir: Option<&str>, ) -> anyhow::Result { - 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, + dir: Option<&str>, ) -> anyhow::Result { 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, + dir: Option<&str>, ) -> anyhow::Result { - 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 { +pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result { 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 { } 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); + } }