subagent daemon: explain the status when returning it, not up front
The `status` tool description enumerated all five states it can report, so every caller paid for four answers it didn't get and read the explanation in the wrong place. The description now states only what the tool is for and that it costs nothing to call; each answer it returns carries its own meaning and the caller's next move instead — running, starting and idle were terse, the not-found error terser still, and they had been leaning on the enumeration to be legible. The killed answer and the end-of-turn todo are unchanged. `continue`'s description keeps its killed-resume sentence: that describes what the tool does, not a state it might hand back. Refs #4326
This commit is contained in:
parent
0bdee751b9
commit
ae84174e3a
2 changed files with 60 additions and 10 deletions
|
|
@ -164,12 +164,8 @@ impl SubagentMcp {
|
||||||
|
|
||||||
#[tool(
|
#[tool(
|
||||||
description = "Report whether a subagent is currently running — a zero-cost check that \
|
description = "Report whether a subagent is currently running — a zero-cost check that \
|
||||||
never launches a process, unlike `continue`. Distinguishes running, starting (a \
|
never launches a process, unlike `continue`. The answer says what state it found \
|
||||||
`start`/`continue` is in flight but not yet a confirmed spawn — this is normally \
|
and what to do about it."
|
||||||
over in well under a second), killed (its last turn died on a signal — the kernel's \
|
|
||||||
OOM killer, a stopped unit, an `interrupt` — so its work was cut off mid-turn rather \
|
|
||||||
than finished), idle (a session exists, its last turn finished, nothing is in flight \
|
|
||||||
— `continue` to give it another turn), and no such session at all."
|
|
||||||
)]
|
)]
|
||||||
fn status(&self, Parameters(args): Parameters<StatusArgs>) -> String {
|
fn status(&self, Parameters(args): Parameters<StatusArgs>) -> String {
|
||||||
match session::status(&self.state, &args.name, args.dir.as_deref()) {
|
match session::status(&self.state, &args.name, args.dir.as_deref()) {
|
||||||
|
|
|
||||||
|
|
@ -576,6 +576,12 @@ pub fn status(state: &State, name: &str, dir: Option<&str>) -> anyhow::Result<St
|
||||||
/// A recorded kill outranks the on-disk session: the session file exists
|
/// A recorded kill outranks the on-disk session: the session file exists
|
||||||
/// either way, so "a session is there" is precisely the fact that cannot
|
/// either way, so "a session is there" is precisely the fact that cannot
|
||||||
/// tell the two apart.
|
/// tell the two apart.
|
||||||
|
///
|
||||||
|
/// Every answer is self-contained — it names the one state the caller got
|
||||||
|
/// and what to do next — because the tool description deliberately doesn't
|
||||||
|
/// enumerate the state space (the operator's ruling on this surface:
|
||||||
|
/// describe the tool, explain the state when returning it). Keep the split:
|
||||||
|
/// a terser answer here has nowhere left to be explained from.
|
||||||
fn describe_status(
|
fn describe_status(
|
||||||
name: &str,
|
name: &str,
|
||||||
occupancy: Option<bool>,
|
occupancy: Option<bool>,
|
||||||
|
|
@ -583,10 +589,18 @@ fn describe_status(
|
||||||
session_exists: bool,
|
session_exists: bool,
|
||||||
) -> anyhow::Result<String> {
|
) -> anyhow::Result<String> {
|
||||||
match occupancy {
|
match occupancy {
|
||||||
Some(true) => return Ok(format!("subagent `{name}` is running")),
|
Some(true) => {
|
||||||
|
return Ok(format!(
|
||||||
|
"subagent `{name}` is running — its turn is still in flight, so there's nothing \
|
||||||
|
to do but let it work: the daemon pushes a todo when the turn ends, or \
|
||||||
|
`interrupt` it if you want it stopped early."
|
||||||
|
));
|
||||||
|
}
|
||||||
Some(false) => {
|
Some(false) => {
|
||||||
return Ok(format!(
|
return Ok(format!(
|
||||||
"subagent `{name}` is starting — not yet confirmed running"
|
"subagent `{name}` is starting — a `start`/`continue` has claimed the name but \
|
||||||
|
its process isn't confirmed spawned yet, which is normally over in well under a \
|
||||||
|
second: check again shortly rather than starting anything else under this name."
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
None => {}
|
None => {}
|
||||||
|
|
@ -602,10 +616,16 @@ fn describe_status(
|
||||||
}
|
}
|
||||||
if session_exists {
|
if session_exists {
|
||||||
Ok(format!(
|
Ok(format!(
|
||||||
"subagent `{name}` is idle — its last turn finished; `continue` to give it another"
|
"subagent `{name}` is idle — its session exists, its last turn ended on its own \
|
||||||
|
rather than being cut off, and nothing is in flight: that turn's own todo says how \
|
||||||
|
it went, and `continue` gives it another."
|
||||||
))
|
))
|
||||||
} else {
|
} else {
|
||||||
anyhow::bail!("no subagent named `{name}` exists — `start` creates one")
|
anyhow::bail!(
|
||||||
|
"no subagent named `{name}` exists — nothing is running under that name and there's \
|
||||||
|
no session on disk to resume, so `start` is what creates one (check the name if you \
|
||||||
|
expected something here)."
|
||||||
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -913,6 +933,40 @@ mod tests {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn every_status_answer_names_its_state_and_the_next_move() {
|
||||||
|
// The tool description no longer lists the states, so each answer
|
||||||
|
// has to carry its own explanation — checked one state at a time,
|
||||||
|
// which is all a caller ever gets back.
|
||||||
|
let running = describe_status("n", Some(true), None, false).expect("running status");
|
||||||
|
assert!(
|
||||||
|
running.contains("running") && running.contains("`interrupt`"),
|
||||||
|
"a running answer must say the turn is in flight and how to stop it: {running}"
|
||||||
|
);
|
||||||
|
let starting = describe_status("n", Some(false), None, false).expect("starting status");
|
||||||
|
assert!(
|
||||||
|
starting.contains("starting") && starting.contains("check again"),
|
||||||
|
"a starting answer must say the spawn isn't confirmed yet and to retry: {starting}"
|
||||||
|
);
|
||||||
|
let idle = describe_status("n", None, None, true).expect("idle status");
|
||||||
|
assert!(
|
||||||
|
idle.contains("idle") && idle.contains("`continue`"),
|
||||||
|
"an idle answer must say the last turn ended on its own and how to give it another: \
|
||||||
|
{idle}"
|
||||||
|
);
|
||||||
|
let killed = describe_status("n", None, Some(libc::SIGKILL), true).expect("killed status");
|
||||||
|
assert!(
|
||||||
|
killed.contains("killed") && killed.contains("`continue`"),
|
||||||
|
"a killed answer must name the kill and say resuming is still possible: {killed}"
|
||||||
|
);
|
||||||
|
let missing = describe_status("n", None, None, false)
|
||||||
|
.expect_err("nothing tracked and nothing on disk is an error, not a state");
|
||||||
|
assert!(
|
||||||
|
missing.to_string().contains("`start`"),
|
||||||
|
"the no-such-session answer must point at what creates one: {missing}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_killed_turn_and_a_clean_one_do_not_land_in_the_same_state() {
|
fn a_killed_turn_and_a_clean_one_do_not_land_in_the_same_state() {
|
||||||
// The whole path a real turn takes, minus the process: what the
|
// The whole path a real turn takes, minus the process: what the
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue