Keep a subagent's words in its own transcript, and count the ones already running
Two corrections to the previous commit. A subagent's closing report belongs in the subagent's transcript, which is where it already is; drawing it as a card in the parent's put the same paragraph in two places for a reader who did not ask for it. The row is a divider now -- a boundary, which is what the transcript actually needed there -- closed, saying only what reported and how it went. Opening it shows the report anyway, since leaving the conversation to read one line has its own cost, and a backgrounded command has no transcript of its own so this is the only place its report exists at all: that one names itself from its summary and has nothing left to open. `TranscriptDivider` grew a `trailing` slot for the chevron rather than the row growing its own copy of the rules. And the status was wrong for a session that was already running before the update, which is every session when the backend is replaced under it. Adoption picks a session's stdout back up from a recorded offset, so the `task_started` lines for subagents launched earlier are behind it and the translator never saw them -- it started with an empty set and reported `idle` with a subagent plainly still working. `Subagents::any_open` reads the directory instead, which is a measurement rather than bookkeeping and is right for a session this process did not start. Both sources are kept and neither subsumes the other: the translator's own set is the only thing that knows about a backgrounded *command*, which has no subagent to be found. The same pair decides whether an ending has already been reported, so a task that began before the restart still gets its divider. Echo's helpers now record their report as their own subagent's closing text, the way the real driver does, so the fixture has the shape being tested. Verified on the emulator: three dividers closed, one opened to its report, and each reply drawn as its own message. 170 server tests, ktfmt, clippy, rustfmt, Android lint and the JVM unit tests all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
5711c2568a
commit
ef1aad8776
11 files changed
+342
-120
No files matched your search
@@ -9,7 +9,7 @@
|
||||
//! session directory; everything else is pure, which is what makes the mapping
|
||||
//! testable without a process.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::collections::{HashMap, HashSet};
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
@@ -108,19 +108,23 @@ pub(super) struct Translator {
|
||||
/// that only the change into that state is reported -- see
|
||||
/// [`Translator::translate_rate_limit`].
|
||||
rate_limited: bool,
|
||||
/// Which Task call each *unfinished* task belongs to: the CLI's `task_id`
|
||||
/// against the `tool_use_id` this side names a subagent by.
|
||||
/// Which Task call each task belongs to: the CLI's `task_id` against the
|
||||
/// `tool_use_id` this side names a subagent by.
|
||||
///
|
||||
/// Needed because the line that says a task ended comes in two shapes and
|
||||
/// only one of them carries the tool id -- see [`Translator::translate_task`].
|
||||
///
|
||||
/// Emptied entry by entry as tasks report back, which makes it the answer
|
||||
/// to two further questions: whether an ending has already been reported
|
||||
/// (the two shapes can both arrive for one task, and the row belongs in
|
||||
/// the transcript once), and whether the session still has work outstanding
|
||||
/// when its own turn ends, which is the difference between `Idle` and
|
||||
/// [`SessionStatus::Waiting`].
|
||||
tasks: HashMap<String, String>,
|
||||
/// The backgrounded tasks this translator has seen start and not seen
|
||||
/// finish, by `tool_use_id`.
|
||||
///
|
||||
/// Half of the answer to "does this session still have work outstanding",
|
||||
/// which is the difference between `Idle` and [`SessionStatus::Waiting`].
|
||||
/// The other half is `Subagents::any_open`, and both are needed: this one
|
||||
/// covers a backgrounded *command*, which has no subagent behind it at
|
||||
/// all, and the registry covers a subagent launched before this
|
||||
/// translator existed, which is every one of them after a backend
|
||||
/// restart adopts a running session.
|
||||
open_tasks: HashSet<String>,
|
||||
/// Whether a turn is open, judged from this translator's own output: the
|
||||
/// events that [`super::proves_a_turn`] accepts open one, and the status
|
||||
/// that ends a turn closes it.
|
||||
@@ -146,6 +150,7 @@ impl Translator {
|
||||
children: HashMap::new(),
|
||||
rate_limited: false,
|
||||
tasks: HashMap::new(),
|
||||
open_tasks: HashSet::new(),
|
||||
in_turn: false,
|
||||
}
|
||||
}
|
||||
@@ -374,10 +379,10 @@ impl Translator {
|
||||
// nobody having typed anything. Reported as what it is, so
|
||||
// that nothing tells the reader the work has finished.
|
||||
events.push(Event::Status {
|
||||
state: if self.tasks.is_empty() {
|
||||
SessionStatus::Idle
|
||||
} else {
|
||||
state: if self.work_outstanding() {
|
||||
SessionStatus::Waiting
|
||||
} else {
|
||||
SessionStatus::Idle
|
||||
},
|
||||
});
|
||||
events
|
||||
@@ -534,7 +539,6 @@ impl Translator {
|
||||
.or_else(|| task_id.and_then(|task| self.tasks.get(task).cloned()));
|
||||
match message.get("subtype").and_then(Value::as_str) {
|
||||
Some("task_notification") => self.task_ended(
|
||||
task_id,
|
||||
about,
|
||||
message.get("status").and_then(Value::as_str),
|
||||
text_field(message, "summary"),
|
||||
@@ -555,16 +559,39 @@ impl Translator {
|
||||
if status == Some("completed") {
|
||||
return Vec::new();
|
||||
}
|
||||
self.task_ended(task_id, about, status, None)
|
||||
self.task_ended(about, status, None)
|
||||
}
|
||||
// `task_started` and `task_progress`: the mapping above is the
|
||||
// whole of what they are for. The subagent itself is created by
|
||||
// the Task `tool_use` in the parent's own message, which arrives
|
||||
// first and carries the title this side shows.
|
||||
Some("task_started") => {
|
||||
// What makes the session `Waiting` when its turn ends. The
|
||||
// subagent itself is created by the Task `tool_use` in the
|
||||
// parent's own message, which arrives first and carries the
|
||||
// title this side shows.
|
||||
if let Some(about) = about {
|
||||
self.open_tasks.insert(about);
|
||||
}
|
||||
Vec::new()
|
||||
}
|
||||
// `task_progress`: the mapping above is the whole of what it is
|
||||
// for.
|
||||
_ => Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether the session has work of its own still running: the difference
|
||||
/// between `Idle` and [`SessionStatus::Waiting`].
|
||||
///
|
||||
/// Two sources because neither covers the other. `open_tasks` holds what
|
||||
/// this translator watched start, which is the only thing that knows
|
||||
/// about a backgrounded *command* -- it has no subagent. The registry
|
||||
/// holds what is on disk, which is the only thing that knows about a
|
||||
/// subagent that started before this translator did.
|
||||
///
|
||||
/// `session_running` is true by construction: this is only ever asked
|
||||
/// while translating a line the session's process just wrote.
|
||||
fn work_outstanding(&self) -> bool {
|
||||
!self.open_tasks.is_empty() || self.subagents.any_open(true)
|
||||
}
|
||||
|
||||
/// A task reporting back, from whichever of the two lines got here first.
|
||||
///
|
||||
/// Reported once. The two shapes can both arrive for one task, and the
|
||||
@@ -582,7 +609,6 @@ impl Translator {
|
||||
/// of it.
|
||||
fn task_ended(
|
||||
&mut self,
|
||||
task_id: Option<&str>,
|
||||
about: Option<String>,
|
||||
status: Option<&str>,
|
||||
summary: Option<String>,
|
||||
@@ -593,11 +619,16 @@ impl Translator {
|
||||
let Some(about) = about else {
|
||||
return Vec::new();
|
||||
};
|
||||
// Nothing under that id: either this task has already been reported,
|
||||
// or its `task_started` was never seen. Both are "say nothing"; the
|
||||
// first would be a duplicate row and the second a row for a task this
|
||||
// translator cannot say anything about.
|
||||
if task_id.is_none_or(|task| self.tasks.remove(task).is_none()) {
|
||||
// Reported once. The two lifecycle shapes can both arrive for one
|
||||
// task, and whichever gets here first is the one that finds it open.
|
||||
//
|
||||
// The registry is asked as well as this translator's own set, and
|
||||
// that is what makes an adopted session work: a subagent launched
|
||||
// before a backend restart has no entry here, because its
|
||||
// `task_started` is behind the offset its session's stdout is read
|
||||
// from. `finish` below closes it either way, so a second line for the
|
||||
// same task still finds nothing.
|
||||
if !self.open_tasks.remove(&about) && !self.subagents.is_open(&about) {
|
||||
return Vec::new();
|
||||
}
|
||||
if let Some(summary) = &summary {
|
||||
@@ -621,7 +652,7 @@ impl Translator {
|
||||
// over: it has stopped being `Waiting` and nothing else will say so.
|
||||
// Inside a turn there is nothing to announce -- the turn's own
|
||||
// `result` will decide between the two statuses when it lands.
|
||||
if self.tasks.is_empty() && !self.in_turn {
|
||||
if !self.work_outstanding() && !self.in_turn {
|
||||
events.push(Event::Status {
|
||||
state: SessionStatus::Idle,
|
||||
});
|
||||
@@ -1476,6 +1507,62 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The case a backend restart produces, which is every subagent a session
|
||||
/// has when the server is updated under it. Adoption picks the session's
|
||||
/// stdout back up from a recorded offset, so the `task_started` lines for
|
||||
/// anything already running are behind it and this translator never sees
|
||||
/// them: it starts empty, and asking only itself would report the session
|
||||
/// idle with a subagent plainly still working.
|
||||
#[test]
|
||||
fn a_subagent_that_started_before_this_translator_still_counts_as_outstanding() {
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
let subagents = test_subagents(&dir);
|
||||
// Started by somebody else, exactly as a previous run of the server
|
||||
// would have left it on disk.
|
||||
subagents.start("toolu_old", "the Dev Updater agent", None);
|
||||
|
||||
let mut translator = Translator::new(dir.path().to_path_buf(), Arc::clone(&subagents));
|
||||
let result = r#"{"type":"result","subtype":"success","is_error":false,"usage":{}}"#;
|
||||
assert_eq!(
|
||||
translate_lines(&mut translator, &[result]).last(),
|
||||
Some(&Event::Status {
|
||||
state: SessionStatus::Waiting
|
||||
}),
|
||||
"the registry knows about it even though this translator does not"
|
||||
);
|
||||
|
||||
// And its ending is reported, though nothing here saw it begin.
|
||||
assert_eq!(
|
||||
translate_lines(
|
||||
&mut translator,
|
||||
&[
|
||||
r#"{"type":"system","subtype":"task_notification","task_id":"old","tool_use_id":"toolu_old","status":"completed","summary":"pushed"}"#,
|
||||
],
|
||||
),
|
||||
vec![
|
||||
Event::TaskNote {
|
||||
about: "toolu_old".into(),
|
||||
title: Some("the Dev Updater agent".into()),
|
||||
status: "completed".into(),
|
||||
summary: Some("pushed".into()),
|
||||
},
|
||||
Event::Status {
|
||||
state: SessionStatus::Idle
|
||||
},
|
||||
]
|
||||
);
|
||||
// Once: `finish` closed it, so the second shape finds nothing.
|
||||
assert!(
|
||||
translate_lines(
|
||||
&mut translator,
|
||||
&[
|
||||
r#"{"type":"system","subtype":"task_updated","task_id":"old","patch":{"status":"failed"}}"#,
|
||||
],
|
||||
)
|
||||
.is_empty()
|
||||
);
|
||||
}
|
||||
|
||||
/// A task ending *inside* a turn says nothing about the session's status:
|
||||
/// the turn is still running, and its own `result` decides. Without the
|
||||
/// `in_turn` guard this reported the session idle in the middle of one,
|
||||
|
||||
Reference in new issue
Block a user