diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index 9a729d83..678d7d72 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -86,17 +86,25 @@ directory and has the answer. Per-agent rather than hive-wide: the clauses a role carries come out of that one agent's own prompt files, and the daemon only ever spawns on that agent's behalf. -**The role is the system prompt; the task is the turn — never the other -way round.** The daemon writes the named role's text, alone, into a -per-session file under the harness directory and points -`--append-system-prompt-file` at it — nothing about the task reaches that -file. It reads the task instructions (`prompt_file`) and folds them ahead -of the turn's own prompt instead, the same channel that carries them to -the subagent without a role. A task baked into the system prompt would -re-assert itself as an instruction on every later turn of the session, not +**The task is the turn, never a standing system prompt — with or without a +role.** A task baked into the system prompt re-asserts itself as an +instruction on every later turn of the session (`continue`, `goal`), not just the one the caller wrote it for — the system prompt is the -subagent's standing identity, not a one-shot channel. The caller's own -task file stays untouched either way. +subagent's standing identity, not a one-shot channel. So the daemon always +reads the task instructions (`prompt_file`) and folds them ahead of the +turn's own prompt instead, whether or not the session has a role. The +caller's own task file stays untouched either way. + +A role additionally names a standing identity: its text, alone, becomes a +per-session file under the harness directory, and `--append-system-prompt-file` +points at that — nothing about the task ever reaches it. Without a role +there is no such identity to hold a file open for, so `--append-system-prompt-file` +is omitted altogether rather than pointed at anything task-shaped. + +⚠️ This applies to every dispatch, not just role-bearing ones — no agent +ships roles yet, so today it's the only path in real use. Before this fix +the no-role path put the task straight into the system prompt, same as +every `start` before roles existed at all. ### A role with no file refuses the call diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 29fac636..8f0337fd 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -945,44 +945,42 @@ fn build_config( } } -/// Which file `--append-system-prompt-file` is pointed at, and the prompt -/// this turn actually opens with, for one spawn. +/// Which file `--append-system-prompt-file` is pointed at, if any, and the +/// prompt this turn actually opens with, for one spawn. /// -/// **The role is the system prompt; the task is the turn — never the -/// other way round.** No role: `prompt_file`, passed through untouched, is -/// the system prompt, and `trigger` is returned as given — the path every -/// `start` took before roles existed, unchanged. A role: its text, alone, -/// becomes the system-prompt file under [`crate::paths::harness_dir`] — -/// nothing about the task reaches it — and the task instructions are read -/// and folded ahead of `trigger` instead, so they still reach the subagent, -/// on the same channel they always have: the turn's own prompt. Per -/// session, like the generated `--mcp-config` beside it, so two concurrent -/// `start`s never share a system-prompt file. +/// **The task is always the turn, never a standing system prompt — with or +/// without a role.** A task baked into the system prompt re-asserts itself +/// as an instruction on every later turn of the session (`continue`, +/// `goal`), not just the one the caller wrote it for; the system prompt is +/// the subagent's standing identity, not a one-shot channel. So the task +/// instructions (`prompt_file`) are always read and folded ahead of +/// `trigger` instead, the one channel that has always carried them. +/// +/// No role: that folded trigger is the whole story — `None` comes back for +/// the system-prompt file, since there is no role text to put there. +/// A role: its text, alone, becomes the system-prompt file under +/// [`crate::paths::harness_dir`] — nothing about the task reaches it. +/// Per session, like the generated `--mcp-config` beside it, so two +/// concurrent `start`s never share a system-prompt file. /// /// # Errors /// -/// A role naming unreadable task instructions, or a harness dir that +/// Unreadable task instructions, or (with a role) a harness dir that /// cannot be written. Both refuse the spawn rather than dropping either /// half — see `docs/tools/subagent.md`. fn system_prompt_and_trigger( name: &str, role: Option<&str>, prompt_file: &str, - trigger: String, -) -> anyhow::Result<(PathBuf, String)> { + trigger: &str, +) -> anyhow::Result<(Option, String)> { // `harness_dir` panics outside a container (`HYPERHIVE_HARNESS_DIR` - // unset) — read lazily, only once a role means it's actually needed, so - // the no-role path never touches it, in tests or otherwise. - let Some(role) = role else { - return Ok((PathBuf::from(prompt_file), trigger)); - }; - compose_prompt( - &crate::paths::harness_dir(), - name, - Some(role), - prompt_file, - trigger, - ) + // unset) — read lazily, only when a role means a system-prompt file + // actually needs somewhere to land. `compose_prompt` only touches `dir` + // inside its `Some(role)` branch, so a no-role call never forces this, + // in tests or otherwise. + let dir = role.map_or_else(PathBuf::new, |_| crate::paths::harness_dir()); + compose_prompt(&dir, name, role, prompt_file, trigger) } /// [`system_prompt_and_trigger`] with the harness dir as an argument rather @@ -997,16 +995,17 @@ fn compose_prompt( name: &str, role: Option<&str>, prompt_file: &str, - trigger: String, -) -> anyhow::Result<(PathBuf, String)> { - let Some(role) = role else { - return Ok((PathBuf::from(prompt_file), trigger)); - }; + trigger: &str, +) -> anyhow::Result<(Option, String)> { let task = std::fs::read_to_string(prompt_file).map_err(|e| { anyhow::anyhow!("reading the task instructions at {prompt_file} failed: {e}") })?; + let trigger = format!("{}\n\n{trigger}", task.trim_end()); + let Some(role) = role else { + return Ok((None, trigger)); + }; let path = render_role_prompt(dir, name, role)?; - Ok((path, format!("{}\n\n{trigger}", task.trim_end()))) + Ok((Some(path), trigger)) } /// Write `name`'s system-prompt file — `role`'s text, alone — into `dir`, @@ -1123,7 +1122,7 @@ pub fn start(state: &Arc, req: StartRequest) -> anyhow::Result { prompt_file: req.prompt_file, role, }, - trigger, + &trigger, dir.as_deref(), ); if result.is_err() { @@ -1155,17 +1154,18 @@ fn start_reserved( state: &Arc, name: &str, spec: SpawnSpec, - trigger: String, + trigger: &str, dir: Option<&str>, ) -> anyhow::Result { let signal_url = state.mint_signal_url(name); let (system_prompt, trigger) = system_prompt_and_trigger(name, spec.role.as_deref(), &spec.prompt_file, trigger)?; + let system_prompt = system_prompt.map(|p| p.to_string_lossy().into_owned()); let config = build_config( name, spec.model, spec.effort, - Some(&system_prompt.to_string_lossy()), + system_prompt.as_deref(), dir, Some(&signal_url), ); @@ -2150,27 +2150,35 @@ mod tests { } #[test] - fn without_a_role_the_task_file_is_what_claude_is_pointed_at() { - // The no-role path, pinned: the caller's own path reaches - // `--append-system-prompt-file` unchanged, nothing is rendered on - // the way, and the trigger passes through untouched too — no - // system-prompt file, no extra flag beyond the one every `start` - // written before roles existed already sent. "Unchanged" is the - // whole assertion. - let (path, trigger) = system_prompt_and_trigger("n", None, "/tmp/p.md", "go".to_owned()) - .expect("passing the task file through cannot fail"); - assert_eq!(path, PathBuf::from("/tmp/p.md")); - assert_eq!(trigger, "go", "no role means the trigger is untouched too"); - - let config = build_config("n", None, None, Some(&path.to_string_lossy()), None, None); - let flag = config - .extra_args - .iter() - .position(|a| a == "--append-system-prompt-file") - .expect("the task file is still passed as the appended system prompt"); + fn without_a_role_the_task_still_moves_out_of_the_system_prompt() { + // The principle the role fix was made on is general: a task baked + // into the system prompt re-asserts itself on every later turn, not + // just the one it was written for, whether or not the session has a + // role. So the no-role path folds the task into the trigger too — + // it must be ABSENT from whatever reaches + // `--append-system-prompt-file` and PRESENT in the trigger, the + // same shape the role-path tests below pin for the role case. + let task = scratch_file("no-role-task", "rebase the branch and report"); + let (path, trigger) = system_prompt_and_trigger("n", None, &task.to_string_lossy(), "go") + .expect("a readable task file composes even with no role"); assert_eq!( - config.extra_args.get(flag + 1), - Some(&"/tmp/p.md".to_owned()) + path, None, + "no role means no system-prompt file at all — there is no role \ + text to put in one, and the task must not go in one either" + ); + assert!( + trigger.contains("rebase the branch"), + "the task must reach the subagent as the turn's own prompt: {trigger:?}" + ); + + let system_prompt = path.map(|p| p.to_string_lossy().into_owned()); + let config = build_config("n", None, None, system_prompt.as_deref(), None, None); + assert!( + !config + .extra_args + .contains(&"--append-system-prompt-file".to_owned()), + "the task must not reach claude as a standing system prompt: {:?}", + config.extra_args ); } @@ -2190,9 +2198,10 @@ mod tests { "role-only-run", Some("you are a reviewer; block on anything unsafe"), &task.to_string_lossy(), - "go".to_owned(), + "go", ) .expect("a role and a readable task file compose"); + let path = path.expect("a role always renders a system-prompt file"); let body = std::fs::read_to_string(&path).expect("the system-prompt file is readable"); assert!( body.contains("you are a reviewer"), @@ -2217,7 +2226,7 @@ mod tests { "turn-prompt-run", Some("you are a reviewer"), &task.to_string_lossy(), - "Carry out the task described in your instructions.".to_owned(), + "Carry out the task described in your instructions.", ) .expect("a role and a readable task file compose"); assert!( @@ -2234,7 +2243,7 @@ mod tests { "n", Some("a role"), "/tmp/no-such-task-file-here.md", - "go".to_owned(), + "go", ) .expect_err("an unreadable task file cannot silently become a role-only prompt"); assert!(err.to_string().contains("task instructions"), "{err}");