diff --git a/SUBAGENTS.md b/SUBAGENTS.md index b173aa7..0d8a1c7 100644 --- a/SUBAGENTS.md +++ b/SUBAGENTS.md @@ -50,17 +50,35 @@ There is no separate delete. 2. Every child line is translated by that subagent's own `Translator` (one per subagent: tool ids are unique but streaming deltas are by content-block index, and parallel subagents interleave). -3. When the parent's `tool_result` for the Task id arrives, the parent gets - its `ToolEnd` as before, and the subagent gets `Status Exited`. -4. When the parent session's process exits (`Status Exited` on the +3. **The parent's `tool_result` never finishes a subagent.** The Task tool + runs in the background by default: the `tool_result` -- "Async agent + launched..." -- arrives the moment it *starts*, while the subagent goes + on working for however long its own turn takes, sometimes minutes. What + ends it is its own turn ending: the raw API's `message_delta` on its + stream carrying `stop_reason: "end_turn"` (a `stop_reason` of `tool_use` + is the model about to call one, not an end), or a `result` line for its + own turn if a future CLI version ever sends one. Either maps to + `Status Exited`; the subagent's vocabulary has no `Idle`, so the + equivalent event `dispatch` produces for an ordinary session is dropped + rather than written. A shipped version of this finished on the + `tool_result` instead, which read a running background agent as + "finished" with its transcript truncated at the moment it launched. +4. **A child line for a subagent that already finished reopens it** + (`Status Running`) rather than being dropped: a background Task can be + sent another message long after its first turn ended, and that is + exactly what a further line for it means. Same transcript, same child + `Translator`, just picking back up. +5. When the parent session's process exits (`Status Exited` on the session), every subagent still `Running` gets `Status Exited` too: its process was the parent's. A subagent that was mid-flight when the backend restarted keeps working: the registry reopens the existing transcript on the next child line, and -the file continues its sequence. If its Task call finished while the backend -was down nothing ever closes it -- its last status stays `Running`, which -the list reports as **unknown** rather than as running (see the wire shape). +the file continues its sequence -- the same reopening #4 describes, whether +what closed it was a restart or its own `end_turn`. If its turn ended while +the backend was down nothing recorded that until the next line arrives, so +its last status stays `Running`, which the list reports as **unknown** +rather than as running (see the wire shape) until then. Title: the Task call's `description` input, then ` ()` when one is given; falling back to the tool's name when the child arrives before @@ -71,7 +89,7 @@ one is given; falling back to the tool's name when the child arrives before - `session/subagent.rs` -- the registry: `Subagents` (per session, in `Shared`), `Subagent` (its `Transcript` behind a mutex plus a `broadcast::Sender`), `record(id, event)`, `start(id, title, - prompt)`, `finish(id)`, `finish_all()`, `list()` from disk. Drivers get an + prompt)`, `finish(id)`, `reopen(id)`, `finish_all()`, `list()` from disk. Drivers get an `Arc` beside their `EventSink`; llama ignores it. - `session/claude/translate.rs` -- routes child lines by parent id, holds one child `Translator` per subagent, remembers pending Task calls' diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt index 425d26b..27faf3f 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt @@ -11,6 +11,7 @@ import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyColumn @@ -396,10 +397,15 @@ private fun SessionCard( // already on screen -- see UI_RULES on a control not displacing the text beside it. if (session.subagents > 0) { Spacer(Modifier.height(8.dp)) + // The platform's minimum touch height, not the chevron's own ten or so dp: + // at the chevron's height a tap meant for it landed on the first subcard + // beneath and opened a subagent instead. Row( horizontalArrangement = Arrangement.Center, + verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth() + .heightIn(min = 48.dp) .clickable(enabled = !deleting, onClick = onToggleSubagents) .semantics { contentDescription = diff --git a/server/src/session/claude/translate.rs b/server/src/session/claude/translate.rs index 6a4bd09..72964e4 100644 --- a/server/src/session/claude/translate.rs +++ b/server/src/session/claude/translate.rs @@ -151,10 +151,12 @@ impl Translator { fn translate_child(&mut self, id: &str, message: &Value) -> Vec { match self.subagents.get(id) { Some(subagent) if !subagent.is_open() => { - // The Task call already ended (or this line is stale from a - // resumed conversation) -- see `SUBAGENTS.md`'s lifecycle. - tracing::debug!("dropping a line for subagent {id}, which has already finished"); - return Vec::new(); + // Not stale: the Task tool runs in the background by + // default, so a finished subagent can still be sent another + // message later (SendMessage) and start working again. A + // line arriving after `finish` means exactly that, not a + // conversation that is over -- see `SUBAGENTS.md`. + self.subagents.reopen(id); } Some(_) => {} None => { @@ -178,7 +180,28 @@ impl Translator { .clone(); let events = child.lock().unwrap().dispatch(message); for event in events { - self.subagents.record(id, event); + // The subagent's own vocabulary is Running/Exited/Unknown, never + // Idle -- a background Task is either working or it has ended, + // never merely "between turns" the way a session is. Dropped + // here rather than never produced, so a `result` line's own + // `Idle` (dispatch's ordinary end-of-turn event, for a subagent + // dialect that ever sends one) is caught the same way a + // `message_delta` would be. + if !matches!( + event, + Event::Status { + state: SessionStatus::Idle + } + ) { + self.subagents.record(id, event); + } + } + // What actually ends a subagent's turn: not the parent's + // `tool_result`, which for a background Task arrives at launch + // ("Async agent launched...") long before the work is done -- see + // `SUBAGENTS.md`. + if ends_a_turn(message) { + self.subagents.finish(id); } Vec::new() } @@ -678,11 +701,11 @@ impl Translator { id: about.clone(), output: texts.join("\n"), }); - // A no-op unless `about` is a subagent's own id -- see - // `SUBAGENTS.md`'s lifecycle #3: the parent gets this `ToolEnd` - // like any other tool result, and the subagent it names (if it - // names one) gets its `Status::Exited`. - self.subagents.finish(&about); + // Deliberately does *not* finish a subagent `about` might name: + // the Task tool runs in the background by default, so this + // `tool_result` -- "Async agent launched..." -- arrives at + // launch, long before the subagent's own work is done. What + // ends it is its own turn ending, handled in `translate_child`. } events } @@ -705,6 +728,32 @@ fn fallback_title(message: &Value) -> String { .to_string() } +/// Whether this line is a subagent's *own* turn ending -- the only thing +/// that does, per `SUBAGENTS.md`: not the parent's `tool_result`, which for +/// a background Task arrives at launch rather than at completion. +/// +/// Checked on the raw line rather than on what `dispatch` returns, so this +/// never has to touch the shared `translate_stream_event`/`dispatch` code a +/// top-level session's own turn-ending also goes through -- a subagent's +/// idea of "ended" must not change when a real session's does. +/// +/// `message_delta` is the raw API's own signal, carrying the stop reason: +/// `end_turn` is genuinely done, `tool_use` means the model is about to call +/// one and there is more coming. A `result` line is the CLI's own shape for +/// a top-level turn; a subagent has not been observed to send one, but +/// SUBAGENTS.md counts it too in case a future CLI version does. +fn ends_a_turn(message: &Value) -> bool { + match message.get("type").and_then(Value::as_str) { + Some("stream_event") => { + let event = &message["event"]; + event.get("type").and_then(Value::as_str) == Some("message_delta") + && event["delta"].get("stop_reason").and_then(Value::as_str) == Some("end_turn") + } + Some("result") => true, + _ => false, + } +} + /// Whether a failed turn failed because the account is out of quota, and when /// the CLI said the limit lifts. /// @@ -1043,7 +1092,13 @@ mod tests { /// The parent's `tool_result` for the Task id is what ends the /// subagent -- SUBAGENTS.md's lifecycle #3 -- and nothing else does. #[test] - fn the_parents_tool_result_finishes_the_subagent() { + fn the_parents_tool_result_does_not_finish_the_subagent() { + // The Task tool runs in the background by default: this + // `tool_result` is "Async agent launched...", arriving the moment + // the subagent *starts*, while it goes on working for however long + // its own turn takes. Finishing it here was the bug -- a running + // background agent read as "finished" with its transcript truncated + // at launch. let dir = tempfile::tempdir().expect("tempdir"); let subagents = test_subagents(&dir); let mut translator = Translator::new(dir.path().to_path_buf(), Arc::clone(&subagents)); @@ -1058,10 +1113,115 @@ mod tests { translate_lines( &mut translator, &[ - r#"{"type":"user","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_task2","content":"done","is_error":false}]},"parent_tool_use_id":null}"#, + r#"{"type":"user","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_task2","content":"Async agent launched","is_error":false}]},"parent_tool_use_id":null}"#, ], ); + assert!(subagent.is_open()); + } + + /// What actually ends a subagent: the raw API's own `message_delta` + /// saying its turn stopped with `end_turn`. Never written into the + /// subagent's own transcript as `Idle` -- its vocabulary has no such + /// state. + #[test] + fn the_subagents_own_end_turn_finishes_it() { + let dir = tempfile::tempdir().expect("tempdir"); + let subagents = test_subagents(&dir); + let mut translator = Translator::new(dir.path().to_path_buf(), Arc::clone(&subagents)); + translate_lines( + &mut translator, + &[ + r#"{"type":"assistant","message":{"content":[{"type":"tool_use","id":"toolu_task3","name":"Task","input":{"description":"helper"}}]},"parent_tool_use_id":null}"#, + r#"{"type":"stream_event","event":{"type":"message_delta","delta":{"stop_reason":"end_turn"}},"parent_tool_use_id":"toolu_task3"}"#, + ], + ); + let subagent = subagents.get("toolu_task3").expect("subagent started"); assert!(!subagent.is_open()); + let lines = crate::session::transcript::read_after(&subagent.transcript_path(), 0) + .expect("read subagent transcript"); + assert!( + !lines + .iter() + .any(|entry| matches!(&entry.event, Event::Status { state } if *state == SessionStatus::Idle)), + "a subagent's transcript must never carry Idle: {lines:?}" + ); + assert_eq!( + lines.last().unwrap().event, + Event::Status { + state: SessionStatus::Exited + } + ); + } + + /// `stop_reason: "tool_use"` is the model about to call a tool, with + /// more of the turn still coming -- not an end. + #[test] + fn a_stop_reason_of_tool_use_does_not_finish_the_subagent() { + let dir = tempfile::tempdir().expect("tempdir"); + let subagents = test_subagents(&dir); + let mut translator = Translator::new(dir.path().to_path_buf(), Arc::clone(&subagents)); + translate_lines( + &mut translator, + &[ + r#"{"type":"assistant","message":{"content":[{"type":"tool_use","id":"toolu_task4","name":"Task","input":{"description":"helper"}}]},"parent_tool_use_id":null}"#, + r#"{"type":"stream_event","event":{"type":"message_delta","delta":{"stop_reason":"tool_use"}},"parent_tool_use_id":"toolu_task4"}"#, + ], + ); + assert!( + subagents + .get("toolu_task4") + .expect("subagent started") + .is_open() + ); + } + + /// A background Task can be sent another message long after its first + /// turn ended -- a further child line for it reopens rather than being + /// dropped, and the same transcript and child translator carry on. + #[test] + fn a_line_after_finish_reopens_the_subagent_rather_than_being_dropped() { + let dir = tempfile::tempdir().expect("tempdir"); + let subagents = test_subagents(&dir); + let mut translator = Translator::new(dir.path().to_path_buf(), Arc::clone(&subagents)); + translate_lines( + &mut translator, + &[ + r#"{"type":"assistant","message":{"content":[{"type":"tool_use","id":"toolu_task5","name":"Task","input":{"description":"helper"}}]},"parent_tool_use_id":null}"#, + r#"{"type":"stream_event","event":{"type":"message_delta","delta":{"stop_reason":"end_turn"}},"parent_tool_use_id":"toolu_task5"}"#, + ], + ); + let subagent = subagents.get("toolu_task5").expect("subagent started"); + assert!(!subagent.is_open()); + + translate_lines( + &mut translator, + &[ + r#"{"type":"assistant","message":{"content":[{"type":"tool_use","id":"toolu_more","name":"Bash","input":{}}]},"parent_tool_use_id":"toolu_task5"}"#, + ], + ); + assert!(subagent.is_open()); + let lines = crate::session::transcript::read_after(&subagent.transcript_path(), 0) + .expect("read subagent transcript"); + // Running, [prompt], Exited, Running (reopened), then the new line's + // own ToolStart -- the same transcript throughout, not a new one. + assert!( + lines.iter().any( + |entry| matches!(&entry.event, Event::ToolStart { tool, .. } if tool == "Bash") + ) + ); + assert_eq!( + lines + .iter() + .filter(|entry| matches!( + &entry.event, + Event::Status { + state: SessionStatus::Running + } + )) + .count(), + 2, + "expected one Running at creation and one at the reopen: {lines:?}" + ); } /// Two subagents running at once keep two separate transcripts: tool ids diff --git a/server/src/session/subagent.rs b/server/src/session/subagent.rs index c825e9f..a6a23e5 100644 --- a/server/src/session/subagent.rs +++ b/server/src/session/subagent.rs @@ -89,9 +89,9 @@ impl Subagent { } /// Whether this subagent's last recorded status is not `Exited` -- - /// what decides whether a further child line still belongs in its - /// transcript. See `SUBAGENTS.md`'s lifecycle: "a child line whose - /// subagent finished already... is ignored". + /// what decides whether a further child line reopens it (see + /// `Subagents::reopen`) rather than continuing straight through. See + /// `SUBAGENTS.md`'s lifecycle. pub fn is_open(&self) -> bool { *self.status.lock().unwrap() != SessionStatus::Exited } @@ -269,10 +269,12 @@ impl Subagents { } } - /// The parent's `tool_result` for this Task id arrived: the subagent's - /// own `Status::Exited`. A no-op for an id that is not a subagent's, so - /// callers can call this for every `tool_result` without first checking - /// whether it belongs to one. + /// The subagent's own turn ended: its `Status::Exited`. Called from + /// `translate_child` on the subagent's own `end_turn`, never on the + /// parent's `tool_result` -- a background Task's `tool_result` arrives + /// at launch, not at completion, so it says nothing about whether this + /// is over. A no-op for an id that is not a subagent's or is already + /// closed. pub fn finish(&self, id: &str) { if let Some(subagent) = self.live.lock().unwrap().get(id).cloned() && subagent.is_open() @@ -283,6 +285,21 @@ impl Subagents { } } + /// A line arrived for a subagent that had already finished: it is + /// working again, not stale -- a background Task can be sent another + /// message long after its first turn ended. Appends `Status::Running` + /// so the list stops reporting it as finished; a no-op if it was not + /// actually closed, so a caller need not check first. + pub fn reopen(&self, id: &str) { + if let Some(subagent) = self.live.lock().unwrap().get(id).cloned() + && !subagent.is_open() + { + subagent.append(Event::Status { + state: SessionStatus::Running, + }); + } + } + /// The parent session's process is gone, so nothing still open here has /// a process behind it either -- see `SUBAGENTS.md`'s lifecycle #4. pub fn finish_all(&self) {