hive-subagent-mcp: fold the task out of the system prompt on the no-role path too
mara's ruling on this PR: the principle that landed for the role path is general, so the no-role path moves the task out of the system prompt as well. `system_prompt_and_trigger`'s no-role branch still passed `prompt_file` straight through as `--append-system-prompt-file` — byte-identical to main's pre-PR behaviour, and the exact bug this PR exists to fix. Since no agent ships roles yet, every real dispatch takes this path, so the standing-system-prompt bug (a task re-asserting itself as an instruction on turn 2/3/N under continue/goal) was still live for 100% of usage; only the unused role path had actually been fixed. `compose_prompt` now always folds the task into `trigger`, role or not. Without a role there is no role text to hold a system-prompt file open for, so none exists at all — `--append-system-prompt-file` is omitted from the spawn entirely, not pointed at anything task-shaped. Replaces `without_a_role_the_task_file_is_what_claude_is_pointed_at` (which pinned the bug as "unchanged") with a test asserting the task is absent from what reaches `--append-system-prompt-file` and present in the trigger. Role-path tests are untouched in behavior; only mechanical fallout from `compose_prompt`'s `Option<PathBuf>` return and `&str` trigger params (clippy needless_pass_by_value once both branches only borrowed it). Blast radius: this changes behaviour for every existing dispatch, since prompt_file's content has always gone into the system-prompt file before this fix.
This commit is contained in:
parent
657875b2fa
commit
85ac62968e
2 changed files with 86 additions and 69 deletions
|
|
@ -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
|
a role carries come out of that one agent's own prompt files, and the
|
||||||
daemon only ever spawns on that agent's behalf.
|
daemon only ever spawns on that agent's behalf.
|
||||||
|
|
||||||
**The role is the system prompt; the task is the turn — never the other
|
**The task is the turn, never a standing system prompt — with or without a
|
||||||
way round.** The daemon writes the named role's text, alone, into a
|
role.** A task baked into the system prompt re-asserts itself as an
|
||||||
per-session file under the harness directory and points
|
instruction on every later turn of the session (`continue`, `goal`), not
|
||||||
`--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
|
|
||||||
just the one the caller wrote it for — the system prompt is the
|
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
|
subagent's standing identity, not a one-shot channel. So the daemon always
|
||||||
task file stays untouched either way.
|
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
|
### A role with no file refuses the call
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -945,44 +945,42 @@ fn build_config(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Which file `--append-system-prompt-file` is pointed at, and the prompt
|
/// Which file `--append-system-prompt-file` is pointed at, if any, and the
|
||||||
/// this turn actually opens with, for one spawn.
|
/// prompt this turn actually opens with, for one spawn.
|
||||||
///
|
///
|
||||||
/// **The role is the system prompt; the task is the turn — never the
|
/// **The task is always the turn, never a standing system prompt — with or
|
||||||
/// other way round.** No role: `prompt_file`, passed through untouched, is
|
/// without a role.** A task baked into the system prompt re-asserts itself
|
||||||
/// the system prompt, and `trigger` is returned as given — the path every
|
/// as an instruction on every later turn of the session (`continue`,
|
||||||
/// `start` took before roles existed, unchanged. A role: its text, alone,
|
/// `goal`), not just the one the caller wrote it for; the system prompt is
|
||||||
/// becomes the system-prompt file under [`crate::paths::harness_dir`] —
|
/// the subagent's standing identity, not a one-shot channel. So the task
|
||||||
/// nothing about the task reaches it — and the task instructions are read
|
/// instructions (`prompt_file`) are always read and folded ahead of
|
||||||
/// and folded ahead of `trigger` instead, so they still reach the subagent,
|
/// `trigger` instead, the one channel that has always carried them.
|
||||||
/// on the same channel they always have: the turn's own prompt. Per
|
///
|
||||||
/// session, like the generated `--mcp-config` beside it, so two concurrent
|
/// No role: that folded trigger is the whole story — `None` comes back for
|
||||||
/// `start`s never share a system-prompt file.
|
/// 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
|
/// # 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
|
/// cannot be written. Both refuse the spawn rather than dropping either
|
||||||
/// half — see `docs/tools/subagent.md`.
|
/// half — see `docs/tools/subagent.md`.
|
||||||
fn system_prompt_and_trigger(
|
fn system_prompt_and_trigger(
|
||||||
name: &str,
|
name: &str,
|
||||||
role: Option<&str>,
|
role: Option<&str>,
|
||||||
prompt_file: &str,
|
prompt_file: &str,
|
||||||
trigger: String,
|
trigger: &str,
|
||||||
) -> anyhow::Result<(PathBuf, String)> {
|
) -> anyhow::Result<(Option<PathBuf>, String)> {
|
||||||
// `harness_dir` panics outside a container (`HYPERHIVE_HARNESS_DIR`
|
// `harness_dir` panics outside a container (`HYPERHIVE_HARNESS_DIR`
|
||||||
// unset) — read lazily, only once a role means it's actually needed, so
|
// unset) — read lazily, only when a role means a system-prompt file
|
||||||
// the no-role path never touches it, in tests or otherwise.
|
// actually needs somewhere to land. `compose_prompt` only touches `dir`
|
||||||
let Some(role) = role else {
|
// inside its `Some(role)` branch, so a no-role call never forces this,
|
||||||
return Ok((PathBuf::from(prompt_file), trigger));
|
// in tests or otherwise.
|
||||||
};
|
let dir = role.map_or_else(PathBuf::new, |_| crate::paths::harness_dir());
|
||||||
compose_prompt(
|
compose_prompt(&dir, name, role, prompt_file, trigger)
|
||||||
&crate::paths::harness_dir(),
|
|
||||||
name,
|
|
||||||
Some(role),
|
|
||||||
prompt_file,
|
|
||||||
trigger,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// [`system_prompt_and_trigger`] with the harness dir as an argument rather
|
/// [`system_prompt_and_trigger`] with the harness dir as an argument rather
|
||||||
|
|
@ -997,16 +995,17 @@ fn compose_prompt(
|
||||||
name: &str,
|
name: &str,
|
||||||
role: Option<&str>,
|
role: Option<&str>,
|
||||||
prompt_file: &str,
|
prompt_file: &str,
|
||||||
trigger: String,
|
trigger: &str,
|
||||||
) -> anyhow::Result<(PathBuf, String)> {
|
) -> anyhow::Result<(Option<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| {
|
let task = std::fs::read_to_string(prompt_file).map_err(|e| {
|
||||||
anyhow::anyhow!("reading the task instructions at {prompt_file} failed: {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)?;
|
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`,
|
/// Write `name`'s system-prompt file — `role`'s text, alone — into `dir`,
|
||||||
|
|
@ -1123,7 +1122,7 @@ pub fn start(state: &Arc<State>, req: StartRequest) -> anyhow::Result<String> {
|
||||||
prompt_file: req.prompt_file,
|
prompt_file: req.prompt_file,
|
||||||
role,
|
role,
|
||||||
},
|
},
|
||||||
trigger,
|
&trigger,
|
||||||
dir.as_deref(),
|
dir.as_deref(),
|
||||||
);
|
);
|
||||||
if result.is_err() {
|
if result.is_err() {
|
||||||
|
|
@ -1155,17 +1154,18 @@ fn start_reserved(
|
||||||
state: &Arc<State>,
|
state: &Arc<State>,
|
||||||
name: &str,
|
name: &str,
|
||||||
spec: SpawnSpec,
|
spec: SpawnSpec,
|
||||||
trigger: String,
|
trigger: &str,
|
||||||
dir: Option<&str>,
|
dir: Option<&str>,
|
||||||
) -> anyhow::Result<String> {
|
) -> anyhow::Result<String> {
|
||||||
let signal_url = state.mint_signal_url(name);
|
let signal_url = state.mint_signal_url(name);
|
||||||
let (system_prompt, trigger) =
|
let (system_prompt, trigger) =
|
||||||
system_prompt_and_trigger(name, spec.role.as_deref(), &spec.prompt_file, 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(
|
let config = build_config(
|
||||||
name,
|
name,
|
||||||
spec.model,
|
spec.model,
|
||||||
spec.effort,
|
spec.effort,
|
||||||
Some(&system_prompt.to_string_lossy()),
|
system_prompt.as_deref(),
|
||||||
dir,
|
dir,
|
||||||
Some(&signal_url),
|
Some(&signal_url),
|
||||||
);
|
);
|
||||||
|
|
@ -2150,27 +2150,35 @@ mod tests {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn without_a_role_the_task_file_is_what_claude_is_pointed_at() {
|
fn without_a_role_the_task_still_moves_out_of_the_system_prompt() {
|
||||||
// The no-role path, pinned: the caller's own path reaches
|
// The principle the role fix was made on is general: a task baked
|
||||||
// `--append-system-prompt-file` unchanged, nothing is rendered on
|
// into the system prompt re-asserts itself on every later turn, not
|
||||||
// the way, and the trigger passes through untouched too — no
|
// just the one it was written for, whether or not the session has a
|
||||||
// system-prompt file, no extra flag beyond the one every `start`
|
// role. So the no-role path folds the task into the trigger too —
|
||||||
// written before roles existed already sent. "Unchanged" is the
|
// it must be ABSENT from whatever reaches
|
||||||
// whole assertion.
|
// `--append-system-prompt-file` and PRESENT in the trigger, the
|
||||||
let (path, trigger) = system_prompt_and_trigger("n", None, "/tmp/p.md", "go".to_owned())
|
// same shape the role-path tests below pin for the role case.
|
||||||
.expect("passing the task file through cannot fail");
|
let task = scratch_file("no-role-task", "rebase the branch and report");
|
||||||
assert_eq!(path, PathBuf::from("/tmp/p.md"));
|
let (path, trigger) = system_prompt_and_trigger("n", None, &task.to_string_lossy(), "go")
|
||||||
assert_eq!(trigger, "go", "no role means the trigger is untouched too");
|
.expect("a readable task file composes even with no role");
|
||||||
|
|
||||||
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!(
|
assert_eq!(
|
||||||
config.extra_args.get(flag + 1),
|
path, None,
|
||||||
Some(&"/tmp/p.md".to_owned())
|
"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",
|
"role-only-run",
|
||||||
Some("you are a reviewer; block on anything unsafe"),
|
Some("you are a reviewer; block on anything unsafe"),
|
||||||
&task.to_string_lossy(),
|
&task.to_string_lossy(),
|
||||||
"go".to_owned(),
|
"go",
|
||||||
)
|
)
|
||||||
.expect("a role and a readable task file compose");
|
.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");
|
let body = std::fs::read_to_string(&path).expect("the system-prompt file is readable");
|
||||||
assert!(
|
assert!(
|
||||||
body.contains("you are a reviewer"),
|
body.contains("you are a reviewer"),
|
||||||
|
|
@ -2217,7 +2226,7 @@ mod tests {
|
||||||
"turn-prompt-run",
|
"turn-prompt-run",
|
||||||
Some("you are a reviewer"),
|
Some("you are a reviewer"),
|
||||||
&task.to_string_lossy(),
|
&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");
|
.expect("a role and a readable task file compose");
|
||||||
assert!(
|
assert!(
|
||||||
|
|
@ -2234,7 +2243,7 @@ mod tests {
|
||||||
"n",
|
"n",
|
||||||
Some("a role"),
|
Some("a role"),
|
||||||
"/tmp/no-such-task-file-here.md",
|
"/tmp/no-such-task-file-here.md",
|
||||||
"go".to_owned(),
|
"go",
|
||||||
)
|
)
|
||||||
.expect_err("an unreadable task file cannot silently become a role-only prompt");
|
.expect_err("an unreadable task file cannot silently become a role-only prompt");
|
||||||
assert!(err.to_string().contains("task instructions"), "{err}");
|
assert!(err.to_string().contains("task instructions"), "{err}");
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue