Keep subagent delivery out of assistant text
This commit is contained in:
1 parent
83b113ef0f
commit
cad0cbcfbe
2 files changed
+78
-4
No files matched your search
+5
-1
@@ -28,7 +28,11 @@ items name the child thread and its lifecycle, and `collabAgentToolCall`
|
|||||||
items carry the spawn prompt. The Codex translator routes a non-root
|
items carry the spawn prompt. The Codex translator routes a non-root
|
||||||
`threadId` exactly as Claude routes a `parent_tool_use_id`. The child thread
|
`threadId` exactly as Claude routes a `parent_tool_use_id`. The child thread
|
||||||
id is the subagent id on disk. An asynchronously delivered `agentMessage` is
|
id is the subagent id on disk. An asynchronously delivered `agentMessage` is
|
||||||
a `PeerMessage`, not assistant text from the recipient.
|
a `PeerMessage`, not assistant text from the recipient. Its delta notification
|
||||||
|
does not repeat the completed item's `delivery` field, so the translator
|
||||||
|
remembers that field from `item/started` and suppresses those deltas. Letting
|
||||||
|
one into the recipient's provisional assistant row makes its next completed
|
||||||
|
message replace the combined row, visibly erasing text that Codex still has.
|
||||||
|
|
||||||
## Storage
|
## Storage
|
||||||
|
|
||||||
|
|||||||
@@ -4,7 +4,7 @@
|
|||||||
//! therefore match only the records that have a useful common equivalent and
|
//! therefore match only the records that have a useful common equivalent and
|
||||||
//! ignore the rest; an added Codex item must not make a live session go deaf.
|
//! ignore the rest; an added Codex item must not make a live session go deaf.
|
||||||
|
|
||||||
use std::collections::HashMap;
|
use std::collections::{HashMap, HashSet};
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use serde_json::{Value, json};
|
use serde_json::{Value, json};
|
||||||
@@ -21,6 +21,7 @@ pub(super) struct Translator {
|
|||||||
subagents: Option<Arc<Subagents>>,
|
subagents: Option<Arc<Subagents>>,
|
||||||
children: HashMap<String, Translator>,
|
children: HashMap<String, Translator>,
|
||||||
prompts: HashMap<String, String>,
|
prompts: HashMap<String, String>,
|
||||||
|
async_messages: HashSet<String>,
|
||||||
in_turn: bool,
|
in_turn: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -134,15 +135,43 @@ impl Translator {
|
|||||||
state: SessionStatus::Running,
|
state: SessionStatus::Running,
|
||||||
}]
|
}]
|
||||||
}
|
}
|
||||||
Some("item.started") | Some("item/started") => start_item(&body["item"]),
|
Some("item.started") | Some("item/started") => {
|
||||||
|
let item = &body["item"];
|
||||||
|
if let Some(id) = item.get("id").and_then(Value::as_str)
|
||||||
|
&& item.get("delivery").and_then(Value::as_str) == Some("async")
|
||||||
|
{
|
||||||
|
self.async_messages.insert(id.to_string());
|
||||||
|
}
|
||||||
|
start_item(item)
|
||||||
|
}
|
||||||
Some("item.updated") => update_item(&line["item"]),
|
Some("item.updated") => update_item(&line["item"]),
|
||||||
// The old `codex exec --json` dialect reports only the completed message. App-server
|
// The old `codex exec --json` dialect reports only the completed message. App-server
|
||||||
// also reports deltas, but they are provisional: safety buffering can revise their
|
// also reports deltas, but they are provisional: safety buffering can revise their
|
||||||
// text before completion. Keep its completed copy as an append-only correction rather
|
// text before completion. Keep its completed copy as an append-only correction rather
|
||||||
// than guessing that the two representations concatenate to the same answer.
|
// than guessing that the two representations concatenate to the same answer.
|
||||||
Some("item.completed") => complete_item(&body["item"], true),
|
Some("item.completed") => complete_item(&body["item"], true),
|
||||||
Some("item/completed") => final_item(&body["item"]),
|
Some("item/completed") => {
|
||||||
|
let item = &body["item"];
|
||||||
|
let events = final_item(item);
|
||||||
|
if let Some(id) = item.get("id").and_then(Value::as_str) {
|
||||||
|
self.async_messages.remove(id);
|
||||||
|
}
|
||||||
|
events
|
||||||
|
}
|
||||||
Some("item/agentMessage/delta") => {
|
Some("item/agentMessage/delta") => {
|
||||||
|
if body
|
||||||
|
.get("itemId")
|
||||||
|
.and_then(Value::as_str)
|
||||||
|
.is_some_and(|id| self.async_messages.contains(id))
|
||||||
|
{
|
||||||
|
// The delta notification does not repeat `delivery`.
|
||||||
|
// Appending an asynchronously delivered peer report as
|
||||||
|
// assistant text merges it into the recipient's current
|
||||||
|
// reply; the next completed assistant item then replaces
|
||||||
|
// that whole row. The item/started record did identify it,
|
||||||
|
// so leave its text to the PeerMessage emitted at completion.
|
||||||
|
return Vec::new();
|
||||||
|
}
|
||||||
let Some(delta) = body.get("delta").and_then(Value::as_str) else {
|
let Some(delta) = body.get("delta").and_then(Value::as_str) else {
|
||||||
return Vec::new();
|
return Vec::new();
|
||||||
};
|
};
|
||||||
@@ -1192,6 +1221,47 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn delivered_agent_message_deltas_do_not_replace_the_recipients_text() {
|
||||||
|
let mut translator = Translator::default();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
translator
|
||||||
|
.translate(&line(
|
||||||
|
r#"{"method":"item/started","params":{"threadId":"parent-thread","turnId":"turn-1","item":{"id":"delivery-1","type":"agentMessage","text":"","phase":null,"delivery":"async"}}}"#
|
||||||
|
))
|
||||||
|
.is_empty()
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
translator
|
||||||
|
.translate(&line(
|
||||||
|
r#"{"method":"item/agentMessage/delta","params":{"threadId":"parent-thread","turnId":"turn-1","itemId":"delivery-1","delta":"a child report"}}"#
|
||||||
|
))
|
||||||
|
.is_empty()
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
translator.translate(&line(
|
||||||
|
r#"{"method":"item/completed","params":{"threadId":"parent-thread","turnId":"turn-1","item":{"id":"delivery-1","type":"agentMessage","text":"a child report","phase":null,"delivery":"async"}}}"#
|
||||||
|
)),
|
||||||
|
vec![Event::PeerMessage {
|
||||||
|
from: "another agent".to_string(),
|
||||||
|
text: "a child report".to_string(),
|
||||||
|
turn_start: None
|
||||||
|
}]
|
||||||
|
);
|
||||||
|
|
||||||
|
// Removing the completed id is the path out for this state: a later
|
||||||
|
// item reusing it is not silently suppressed.
|
||||||
|
assert_eq!(
|
||||||
|
translator.translate(&line(
|
||||||
|
r#"{"method":"item/agentMessage/delta","params":{"threadId":"parent-thread","turnId":"turn-2","itemId":"delivery-1","delta":"ordinary text"}}"#
|
||||||
|
)),
|
||||||
|
vec![Event::AssistantText {
|
||||||
|
delta: "ordinary text".to_string()
|
||||||
|
}]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn an_adopted_turn_does_not_go_idle_when_its_last_child_finishes() {
|
fn an_adopted_turn_does_not_go_idle_when_its_last_child_finishes() {
|
||||||
let dir = tempfile::tempdir().expect("tempdir");
|
let dir = tempfile::tempdir().expect("tempdir");
|
||||||
|
|||||||
Reference in new issue
Block a user