hive-subagent-mcp: name a role at dispatch, load it as the system prompt
A `start` may now name a role: `role: "reviewer"` loads the spawning agent's own `subagent_roles/reviewer.md` and renders it, alone, into one per-session file that `--append-system-prompt-file` points at. The role is the system prompt; the task is the turn, never the other way round — a task baked into the system prompt would re-assert itself as an instruction on every later turn of a continued session, not just the one it was written for. The task instructions (`prompt_file`) are read and folded ahead of the turn's own prompt instead, the same channel that carries them to the subagent without a role. The argument is optional, so every existing call is unchanged — pinned by a test that a pre-role payload still deserializes with `role` absent from the schema's required set, and another that the no-role path reaches claude with the caller's own file, unrendered, and the trigger untouched. With a role, one test pins the system-prompt file to the role's text and nothing of the task, and another pins the task still reaching the subagent as the turn's prompt. A role name with no file fails the call, before the session name is even reserved, and the error lists the roles the directory does hold. No agent ships roles yet, so named-but-missing is the ordinary first-run state; a fallback there would spawn a subagent under a prompt missing every clause the role existed to carry. An empty file and a name that is not a plain identifier refuse the same way.
This commit is contained in:
atlas
mara
parent
aaedff20a9
commit
657875b2fa
6 changed files with 545 additions and 11 deletions
|
|
@ -76,7 +76,7 @@
|
|||
|
||||
use std::collections::HashMap;
|
||||
use std::os::unix::process::ExitStatusExt as _;
|
||||
use std::path::PathBuf;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::{Arc, Mutex, PoisonError};
|
||||
use std::time::{Duration, Instant};
|
||||
|
||||
|
|
@ -945,6 +945,87 @@ fn build_config(
|
|||
}
|
||||
}
|
||||
|
||||
/// Which file `--append-system-prompt-file` is pointed at, 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.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// A role naming unreadable task instructions, or 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)> {
|
||||
// `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,
|
||||
)
|
||||
}
|
||||
|
||||
/// [`system_prompt_and_trigger`] with the harness dir as an argument rather
|
||||
/// than an ambient container-only environment read — the split a test uses
|
||||
/// to control where the role-only file lands.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// See [`system_prompt_and_trigger`].
|
||||
fn compose_prompt(
|
||||
dir: &Path,
|
||||
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));
|
||||
};
|
||||
let task = std::fs::read_to_string(prompt_file).map_err(|e| {
|
||||
anyhow::anyhow!("reading the task instructions at {prompt_file} failed: {e}")
|
||||
})?;
|
||||
let path = render_role_prompt(dir, name, role)?;
|
||||
Ok((path, format!("{}\n\n{trigger}", task.trim_end())))
|
||||
}
|
||||
|
||||
/// Write `name`'s system-prompt file — `role`'s text, alone — into `dir`,
|
||||
/// returning the file's path. Split from [`compose_prompt`] so the
|
||||
/// directory is an argument rather than an ambient container-only
|
||||
/// environment read.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// A `dir` that cannot be created or written.
|
||||
fn render_role_prompt(dir: &Path, name: &str, role: &str) -> anyhow::Result<PathBuf> {
|
||||
std::fs::create_dir_all(dir)
|
||||
.map_err(|e| anyhow::anyhow!("creating {} failed: {e}", dir.display()))?;
|
||||
let path = dir.join(format!("subagent-system-prompt-{name}.md"));
|
||||
std::fs::write(&path, role)
|
||||
.map_err(|e| anyhow::anyhow!("writing {} failed: {e}", path.display()))?;
|
||||
Ok(path)
|
||||
}
|
||||
|
||||
/// The [`SessionStore`] a subagent's turn actually runs against — same
|
||||
/// resolution `hive_claude::Claude` itself uses, so a lookup here can't
|
||||
/// disagree with what the driver does a moment later.
|
||||
|
|
@ -966,6 +1047,11 @@ pub struct StartRequest {
|
|||
pub effort: Option<String>,
|
||||
/// File holding the subagent's task instructions.
|
||||
pub prompt_file: String,
|
||||
/// Which named role this subagent runs as — a file in this agent's own
|
||||
/// role directory, named without its extension. `None` is the
|
||||
/// pre-role shape: the task instructions alone. A name with no file
|
||||
/// refuses the whole `start` (see [`crate::role`]).
|
||||
pub role: Option<String>,
|
||||
/// The first turn's prompt. A goal, when given, is appended to it.
|
||||
pub trigger: String,
|
||||
/// Working directory for the session; `None` inherits the daemon's.
|
||||
|
|
@ -1005,6 +1091,10 @@ pub fn start(state: &Arc<State>, req: StartRequest) -> anyhow::Result<String> {
|
|||
let name = req.name.as_str();
|
||||
validate_name(name)?;
|
||||
check_model(req.model.as_deref(), available_models().as_deref())?;
|
||||
// Before `reserve`, so a role this agent does not have costs the caller
|
||||
// an error and nothing else: no name claimed, no session archived, no
|
||||
// process spawned.
|
||||
let role = req.role.as_deref().map(crate::role::load).transpose()?;
|
||||
if !state.reserve(name) {
|
||||
anyhow::bail!("subagent `{name}` is already running — use `continue` or `interrupt`");
|
||||
}
|
||||
|
|
@ -1027,9 +1117,12 @@ pub fn start(state: &Arc<State>, req: StartRequest) -> anyhow::Result<String> {
|
|||
let result = start_reserved(
|
||||
state,
|
||||
name,
|
||||
req.model,
|
||||
req.effort,
|
||||
&req.prompt_file,
|
||||
SpawnSpec {
|
||||
model: req.model,
|
||||
effort: req.effort,
|
||||
prompt_file: req.prompt_file,
|
||||
role,
|
||||
},
|
||||
trigger,
|
||||
dir.as_deref(),
|
||||
);
|
||||
|
|
@ -1039,6 +1132,21 @@ pub fn start(state: &Arc<State>, req: StartRequest) -> anyhow::Result<String> {
|
|||
result
|
||||
}
|
||||
|
||||
/// How one spawn is shaped, as against which session it is for: the four
|
||||
/// values `start` has already resolved by the time it commits to running.
|
||||
/// A struct rather than four more parameters — the role made
|
||||
/// `start_reserved`'s list long enough to be both unreadable and a lint,
|
||||
/// the same reason [`StartRequest`] exists.
|
||||
struct SpawnSpec {
|
||||
model: Option<String>,
|
||||
effort: Option<String>,
|
||||
/// The caller's task-instruction file, as given.
|
||||
prompt_file: String,
|
||||
/// The named role's *text*, already loaded — `start` reads it before
|
||||
/// reserving the name, so an unknown role never gets this far.
|
||||
role: Option<String>,
|
||||
}
|
||||
|
||||
/// The slow, fallible part of `start`, run only after `reserve` has
|
||||
/// already closed the TOCTOU window — split out so `start` can release the
|
||||
/// reservation on any error path here without duplicating that logic per
|
||||
|
|
@ -1046,18 +1154,18 @@ pub fn start(state: &Arc<State>, req: StartRequest) -> anyhow::Result<String> {
|
|||
fn start_reserved(
|
||||
state: &Arc<State>,
|
||||
name: &str,
|
||||
model: Option<String>,
|
||||
effort: Option<String>,
|
||||
prompt_file: &str,
|
||||
spec: SpawnSpec,
|
||||
trigger: String,
|
||||
dir: Option<&str>,
|
||||
) -> anyhow::Result<String> {
|
||||
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 config = build_config(
|
||||
name,
|
||||
model,
|
||||
effort,
|
||||
Some(prompt_file),
|
||||
spec.model,
|
||||
spec.effort,
|
||||
Some(&system_prompt.to_string_lossy()),
|
||||
dir,
|
||||
Some(&signal_url),
|
||||
);
|
||||
|
|
@ -1936,6 +2044,7 @@ mod tests {
|
|||
model: None,
|
||||
effort: None,
|
||||
prompt_file: "/tmp/prompt.md".to_owned(),
|
||||
role: None,
|
||||
trigger: "trigger".to_owned(),
|
||||
dir: None,
|
||||
goal: None,
|
||||
|
|
@ -2032,6 +2141,135 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// A scratch file holding `body`, named after `tag` so parallel tests
|
||||
/// never share one.
|
||||
fn scratch_file(tag: &str, body: &str) -> PathBuf {
|
||||
let path = std::env::temp_dir().join(format!("hive-subagent-session-test-{tag}.md"));
|
||||
std::fs::write(&path, body).expect("a scratch file is writable");
|
||||
path
|
||||
}
|
||||
|
||||
#[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");
|
||||
assert_eq!(
|
||||
config.extra_args.get(flag + 1),
|
||||
Some(&"/tmp/p.md".to_owned())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_role_s_system_prompt_file_holds_the_role_and_never_the_task() {
|
||||
// A task rendered into the system prompt re-asserts itself as an
|
||||
// instruction on every later turn of the session, not just the one
|
||||
// it was written for — the system prompt is the subagent's
|
||||
// standing identity, not a one-shot channel. Asserting the role is
|
||||
// present is not enough to catch that regression; a merged file
|
||||
// would pass that check too. The task's absence from this file is
|
||||
// the assertion that actually pins the bug.
|
||||
let task = scratch_file("role-only-task", "rebase the branch and report");
|
||||
let dir = std::env::temp_dir().join("hive-subagent-session-test-role-only");
|
||||
let (path, _trigger) = compose_prompt(
|
||||
&dir,
|
||||
"role-only-run",
|
||||
Some("you are a reviewer; block on anything unsafe"),
|
||||
&task.to_string_lossy(),
|
||||
"go".to_owned(),
|
||||
)
|
||||
.expect("a role and a readable task file compose");
|
||||
let body = std::fs::read_to_string(&path).expect("the system-prompt file is readable");
|
||||
assert!(
|
||||
body.contains("you are a reviewer"),
|
||||
"the role must reach the system prompt: {body:?}"
|
||||
);
|
||||
assert!(
|
||||
!body.contains("rebase the branch"),
|
||||
"the task must NOT reach the system-prompt file: {body:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_role_still_delivers_the_task_as_the_turns_own_prompt() {
|
||||
// The other half of the same fix: dropping the task from the
|
||||
// system prompt must not drop it altogether — it goes back to
|
||||
// being the turn's prompt, the same channel it reached the
|
||||
// subagent by before roles existed.
|
||||
let task = scratch_file("turn-prompt-task", "rebase the branch and report");
|
||||
let dir = std::env::temp_dir().join("hive-subagent-session-test-turn-prompt");
|
||||
let (_path, trigger) = compose_prompt(
|
||||
&dir,
|
||||
"turn-prompt-run",
|
||||
Some("you are a reviewer"),
|
||||
&task.to_string_lossy(),
|
||||
"Carry out the task described in your instructions.".to_owned(),
|
||||
)
|
||||
.expect("a role and a readable task file compose");
|
||||
assert!(
|
||||
trigger.contains("rebase the branch"),
|
||||
"the task must reach the subagent as the turn's own prompt: {trigger:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_role_with_an_unreadable_task_file_refuses_rather_than_dropping_either() {
|
||||
let dir = std::env::temp_dir().join("hive-subagent-session-test-unreadable");
|
||||
let err = compose_prompt(
|
||||
&dir,
|
||||
"n",
|
||||
Some("a role"),
|
||||
"/tmp/no-such-task-file-here.md",
|
||||
"go".to_owned(),
|
||||
)
|
||||
.expect_err("an unreadable task file cannot silently become a role-only prompt");
|
||||
assert!(err.to_string().contains("task instructions"), "{err}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_start_naming_an_unknown_role_errors_and_reserves_nothing() {
|
||||
// The error path end to end: the refusal happens before anything is
|
||||
// claimed, so an unknown role costs a message and nothing else —
|
||||
// no reservation to release, no archived prior session, no spawn.
|
||||
let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url()));
|
||||
let mut req = start_request("unknown-role-run");
|
||||
req.role = Some("no-such-role-ships-anywhere".to_owned());
|
||||
let err = start(&state, req).expect_err("a role this agent lacks must refuse the start");
|
||||
assert!(
|
||||
err.to_string().contains("no-such-role-ships-anywhere"),
|
||||
"the refusal names the role that was asked for: {err}"
|
||||
);
|
||||
assert_eq!(
|
||||
state.occupancy("unknown-role-run"),
|
||||
None,
|
||||
"a refused start leaves the name free"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_start_naming_an_unusable_role_refuses_before_touching_the_filesystem() {
|
||||
let state = Arc::new(State::new(PathBuf::from("/dev/null"), signal_url()));
|
||||
let mut req = start_request("bad-role-name");
|
||||
req.role = Some("../../etc/passwd".to_owned());
|
||||
let err = start(&state, req).expect_err("a role name that is not an identifier refuses");
|
||||
assert!(err.to_string().contains("invalid role name"), "{err}");
|
||||
assert_eq!(state.occupancy("bad-role-name"), None);
|
||||
}
|
||||
|
||||
/// The `--tools` value as it reaches the spawned argv.
|
||||
fn spawned_tools(config: &Config) -> &str {
|
||||
let flag = config
|
||||
|
|
|
|||
Loading…
Reference in a new issue