diff --git a/hive-subagent-mcp/src/mcp.rs b/hive-subagent-mcp/src/mcp.rs index f0f3be86..e8d5491e 100644 --- a/hive-subagent-mcp/src/mcp.rs +++ b/hive-subagent-mcp/src/mcp.rs @@ -37,17 +37,6 @@ 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 { @@ -64,24 +53,12 @@ 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)] @@ -118,7 +95,6 @@ impl SubagentMcp { args.model, &args.prompt_file, args.trigger, - args.dir.as_deref(), ) { Ok(msg) => msg, Err(e) => format!("start error: {e:#}"), @@ -134,13 +110,7 @@ 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, - args.dir.as_deref(), - ) { + match session::continue_(&self.state, &args.name, args.prompt, args.model) { Ok(msg) => msg, Err(e) => format!("continue error: {e:#}"), } @@ -168,7 +138,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, args.dir.as_deref()) { + match session::status(&self.state, &args.name) { 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 79263a4b..8b2c5e90 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -3,17 +3,13 @@ //! //! **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) 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 +//! the `None`/`Some` split is for), 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, and the -//! remembered `dir` is gone too. The durable +//! (`tokio`'s own child-process drop semantics), not adopted. 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" — after a restart, re-supply -//! `dir` once if the session isn't in the daemon's own default directory. +//! restarted and I want to pick this back up." //! //! **No mid-turn compaction.** Building on `hive_claude::Claude::spawn` + //! `RunningClaude::wait` directly (not `InfiniteSession::run`) is what makes @@ -45,17 +41,8 @@ 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, } @@ -64,7 +51,6 @@ impl State { pub fn new(socket: PathBuf) -> Self { Self { running: Mutex::new(HashMap::new()), - dirs: Mutex::new(HashMap::new()), socket, } } @@ -106,56 +92,6 @@ 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 @@ -182,17 +118,9 @@ 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. `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 { +/// 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 { 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()); @@ -200,7 +128,6 @@ fn build_config( } Config { model, - cwd: dir.map(PathBuf::from), strict_mcp_config: true, extra_args, env: vec![( @@ -240,16 +167,12 @@ 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`"); } - // 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()); + let result = start_reserved(state, name, model, prompt_file, trigger); if result.is_err() { state.release_reservation(name); } @@ -266,9 +189,8 @@ fn start_reserved( model: Option, prompt_file: &str, trigger: String, - dir: Option<&str>, ) -> anyhow::Result { - let config = build_config(name, model, Some(prompt_file), dir); + let config = build_config(name, model, Some(prompt_file)); let store = build_store(&config)?; if store.find_by_title(name).is_some() { tracing::info!( @@ -303,7 +225,6 @@ pub fn continue_( name: &str, prompt: String, model: Option, - dir: Option<&str>, ) -> anyhow::Result { validate_name(name)?; if !state.reserve(name) { @@ -311,10 +232,7 @@ pub fn continue_( "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()); + let result = continue_reserved(state, name, prompt, model); if result.is_err() { state.release_reservation(name); } @@ -329,9 +247,8 @@ fn continue_reserved( name: &str, prompt: String, model: Option, - dir: Option<&str>, ) -> anyhow::Result { - let config = build_config(name, model, None, dir); + let config = build_config(name, model, None); let store = build_store(&config)?; if store.find_by_title(name).is_none() { anyhow::bail!( @@ -404,12 +321,8 @@ fn spawn_and_track( /// # Errors /// /// An invalid name, or no session — running or on disk — under `name`. -pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result { +pub fn status(state: &State, name: &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) => { @@ -419,7 +332,7 @@ pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result {} } - let config = build_config(name, None, None, dir.as_deref()); + let config = build_config(name, None, None); let store = build_store(&config)?; if store.find_by_title(name).is_some() { Ok(format!( @@ -559,117 +472,18 @@ mod tests { #[test] fn build_config_only_appends_system_prompt_when_given() { - let with = build_config("n", None, Some("/tmp/p.md"), None); + let with = build_config("n", None, Some("/tmp/p.md")); 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, None); + let without = build_config("n", 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); - } }