From 73c66f4777fabbeb700808855f982266f9cd8a5e Mon Sep 17 00:00:00 2001 From: ymote <151983+ymote@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:03:41 -0700 Subject: [PATCH] fix(shell): a reload keeps the panel's notices in place and the tool rows' names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things changed when a transcript was loaded again from history (the "Ask " panel reopened, the system chat's pane reopened): - The panel's notices ("Stopped.", "Nothing of yours was running.", a turn's "turn interrupted by client", ...) were appended after the whole history, so on every reopen they moved below newer rows (instrument run, frames 21 and 22). Conversation::load_history now puts what history cannot know (notices, an open question, a running turn's rows) back where it stood: right after the row it followed, found again by its kind and turn (keep_in_place). - Reloaded tool rows read "⚙ tool · done" instead of the tool's name (frames 24 and 35). Both models read `name` or `tool_name`, which no history row has: octos's session/hydrate row (HydratedMessage at the pinned ae230ce) is seq, role, content, thread_id, persisted_at and the like, with neither the tool's name nor its call id (the name is only on the assistant's tool call, which hydrate does not return; octos-web and Octoscode skip these rows), and the broker's merged history adds none. So a reloaded tool row keeps the name the transcript already showed for the same call (system_chat::model::ToolNames): a turn's tool rows in order, matched by the turn every persisted row carries as `thread_id`, when the transcript showed as many for that turn. A row of a turn the transcript never followed in full (another run's) still says "tool", never another call's name; octos would have to return the name for those. The system chat now reads a row's turn from `thread_id` too, as the panel did. Tests: app_chat (the instrument run's notices, and tool names, across two reloads) and system_chat (tool names across reloads; a turn seen in part). Co-Authored-By: Claude Opus 5.5 --- crates/shell/src/app_chat/model.rs | 80 ++++++++++++++++--- crates/shell/src/app_chat/tests.rs | 110 ++++++++++++++++++++++++++ crates/shell/src/system_chat/model.rs | 79 ++++++++++++++++-- crates/shell/src/system_chat/tests.rs | 49 ++++++++++++ 4 files changed, 300 insertions(+), 18 deletions(-) diff --git a/crates/shell/src/app_chat/model.rs b/crates/shell/src/app_chat/model.rs index a696a0e4..6098390c 100644 --- a/crates/shell/src/app_chat/model.rs +++ b/crates/shell/src/app_chat/model.rs @@ -29,7 +29,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap}; use serde_json::Value; -use crate::system_chat::model::{ChatModel, Item, Phase, Role}; +use crate::system_chat::model::{history_turn, ChatModel, Item, Phase, Role, ToolNames}; /// The person's lane (this panel's context) and the system agent's. pub const LANE_PERSON: &str = "person"; @@ -232,12 +232,16 @@ impl Conversation { /// The conversation's history (both transcripts merged by time; rows /// `{role, content, lane, speaker?, display_text?, thread_id|turn_id?}`) - /// replaces what the panel showed. + /// replaces what the panel showed. A tool row keeps the name the panel + /// showed for it ([`ToolNames`]: history has none), and what history + /// cannot know stays where it stood ([`keep_in_place`]). pub fn load_history(&mut self, rows: &Value) { + let rows = rows.as_array().map(Vec::as_slice).unwrap_or(&[]); + let mut tools = ToolNames::new(&self.chat.items, rows); let mut items = Vec::new(); - for row in rows.as_array().into_iter().flatten() { + for row in rows { let lane = row["lane"].as_str().unwrap_or(LANE_PERSON); - let turn = row["turn_id"].as_str().or_else(|| row["thread_id"].as_str()).map(str::to_string); + let turn = history_turn(row); if let Some(t) = &turn { self.lanes.entry(t.clone()).or_insert_with(|| lane.to_string()); } @@ -259,7 +263,7 @@ impl Conversation { } "tool" => items.push(Item::Tool { call_id: row["tool_call_id"].as_str().unwrap_or("").to_string(), - name: row["name"].as_str().or_else(|| row["tool_name"].as_str()).unwrap_or("tool").to_string(), + name: tools.name(row, turn.as_deref()), status: crate::system_chat::model::ToolStatus::Done, detail: String::new(), turn, @@ -269,14 +273,11 @@ impl Conversation { } } // What history cannot know stays: notices, and anything still open. - for item in std::mem::take(&mut self.chat.items) { - let keep = matches!(item, Item::Question { answered: None, .. } | Item::Notice(_)) - || matches!(&item, Item::Message { turn: Some(t), .. } if self.running.contains(t)); - if keep { - items.push(item); - } - } - self.chat.items = items; + let running = &self.running; + let shown = std::mem::take(&mut self.chat.items); + self.chat.items = keep_in_place(items, shown, |item| { + matches!(item, Item::Question { answered: None, .. } | Item::Notice(_)) || matches!(item, Item::Message { turn: Some(t), .. } if running.contains(t)) + }); self.settle_phase(); } @@ -285,6 +286,59 @@ impl Conversation { } } +/// What a transcript row is, to find it again in a reloaded history: its +/// kind and its turn (history rows carry the turn, [`history_turn`]). +fn row_key(item: &Item) -> Option<(&'static str, &str)> { + match item { + Item::Message { role: Role::User, turn: Some(t), .. } => Some(("user", t)), + Item::Message { role: Role::Assistant, turn: Some(t), .. } => Some(("assistant", t)), + Item::Tool { turn: Some(t), .. } => Some(("tool", t)), + _ => None, + } +} + +/// `history`, with the items of `shown` (the transcript it replaces) that +/// history cannot know (`kept`: notices, an open question, a running turn's +/// rows) put back where they stood: right after the row they followed in +/// `shown`, found again in `history` by its kind and turn (the same +/// occurrence of that pair, or the last one there is). An item kept from +/// before any row comes first; one whose rows history has none of comes +/// last. Appending them all after the history moved "Stopped." and the +/// other notices below every newer row on each reload. +fn keep_in_place(history: Vec, shown: Vec, kept: impl Fn(&Item) -> bool) -> Vec { + let keys: Vec> = history.iter().map(|i| row_key(i).map(|(k, t)| (k, t.to_string()))).collect(); + let find = |key: &(&'static str, String), nth: usize| -> Option { + let at: Vec = keys.iter().enumerate().filter(|(_, k)| k.as_ref() == Some(key)).map(|(i, _)| i).collect(); + at.get(nth).or(at.last()).copied() + }; + let mut after: Vec> = (0..history.len()).map(|_| Vec::new()).collect(); + let (mut first, mut last) = (Vec::new(), Vec::new()); + // The rows `shown` had so far: kind, turn, and which occurrence. + let mut before: Vec<((&'static str, String), usize)> = Vec::new(); + let mut seen: HashMap<(&'static str, String), usize> = HashMap::new(); + for item in shown { + if kept(&item) { + match before.iter().rev().find_map(|(key, nth)| find(key, *nth)) { + Some(row) => after[row].push(item), + None if before.is_empty() => first.push(item), + None => last.push(item), + } + } else if let Some((kind, turn)) = row_key(&item) { + let key = (kind, turn.to_string()); + let nth = seen.entry(key.clone()).or_default(); + before.push((key, *nth)); + *nth += 1; + } + } + let mut items = first; + for (row, placed) in history.into_iter().zip(after) { + items.push(row); + items.extend(placed); + } + items.extend(last); + items +} + /// The text after the kernel's origin marker, if it carries one. fn strip_marker(text: &str) -> &str { crate::ai_host::app_peers::split_origin_marker(text).map(|(_, rest)| rest).unwrap_or(text) diff --git a/crates/shell/src/app_chat/tests.rs b/crates/shell/src/app_chat/tests.rs index 1dfb7fe2..6e698c6c 100644 --- a/crates/shell/src/app_chat/tests.rs +++ b/crates/shell/src/app_chat/tests.rs @@ -149,6 +149,116 @@ fn the_merged_history_names_its_speakers() { ); } +/// The transcript as the panel draws it, row by row: speakers, notices, +/// tool rows. +fn transcript(c: &Conversation) -> Vec { + c.chat + .items + .iter() + .filter_map(|i| match i { + Item::Message { speaker, text, .. } => Some(format!("{}: {text}", speaker.clone().unwrap_or_default())), + Item::Notice(n) => Some(format!("({n})")), + Item::Tool { name, .. } => Some(format!("[{name}]")), + _ => None, + }) + .collect() +} + +fn started(lane: &str, turn: &str, text: &str, kind: &str) -> Value { + json!({"method": "turn/started", "lane": lane, "speaker": {"kind": kind}, "request": {"text": text, "speaker": {"kind": kind}}, "params": {"turn_id": turn}}) +} + +/// A reload (the panel reopened) puts the notices back where they stood, +/// after the row they followed, not below every newer row. The instrument +/// run's order: the person's turn; the system agent's turn and the person's +/// second one; the person's Stop ("Stopped." and its end), Ctrl+. ("Nothing +/// of yours was running."), the row's Stop ("Stopped the system agent's +/// task." and its end); then, with the panel closed, the system agent's +/// next turn. After the reopen the notices stood below that last turn. +#[test] +fn a_reload_keeps_the_notices_where_they_stood() { + let mut c = Conversation::new("News"); + c.apply(&started(LANE_PERSON, "p1", "Hello News", "person")); + c.apply(&env(LANE_PERSON, "p1", 2, "assistant_persisted", json!({"assistant_segment_id": "p1:1", "text": "SLOW DONE after 15 s"}))); + c.apply(&env(LANE_PERSON, "p1", 3, "turn_terminal", json!({"outcome": "completed"}))); + c.apply(&started(LANE_SYSTEM_AGENT, "s1", "SLOW:45", "system_agent")); + c.apply(&started(LANE_PERSON, "p2", "SLOW:40", "person")); + c.notice("Stopped."); + c.apply(&env(LANE_PERSON, "p2", 5, "turn_terminal", json!({"outcome": "interrupted", "error": {"message": "turn interrupted by client"}}))); + c.notice("Nothing of yours was running."); + c.notice("Stopped the system agent's task."); + c.apply(&env(LANE_SYSTEM_AGENT, "s1", 900, "turn_terminal", json!({"outcome": "interrupted", "error": {"message": "turn interrupted by client"}}))); + // The panel is closed; its follower still hears the system agent's turn. + c.apply(&started(LANE_SYSTEM_AGENT, "s2", "SLOW:3", "system_agent")); + c.apply(&env(LANE_SYSTEM_AGENT, "s2", 902, "assistant_persisted", json!({"assistant_segment_id": "s2:1", "text": "SLOW DONE after 3 s"}))); + c.apply(&env(LANE_SYSTEM_AGENT, "s2", 903, "turn_terminal", json!({"outcome": "completed"}))); + let live = transcript(&c); + assert_eq!( + live, + [ + "You: Hello News", + "News's agent: SLOW DONE after 15 s", + "System agent: SLOW:45", + "You: SLOW:40", + "(Stopped.)", + "(turn interrupted by client)", + "(Nothing of yours was running.)", + "(Stopped the system agent's task.)", + "(turn interrupted by client)", + "System agent: SLOW:3", + "News's agent, to the system agent: SLOW DONE after 3 s", + ] + ); + // Reopened: the merged history as the broker sends it. Every row a turn + // persisted carries its turn as `thread_id`; the two stopped turns the + // kernel never recorded are the broker's rows, with `turn_id`. + let history = json!([ + {"role": "user", "content": "[from the person: News] Hello News", "lane": "person", "speaker": {"kind": "person"}, "display_text": "Hello News", "thread_id": "p1"}, + {"role": "assistant", "content": "SLOW DONE after 15 s", "lane": "person", "thread_id": "p1"}, + {"role": "user", "content": "SLOW:45", "display_text": "SLOW:45", "speaker": {"kind": "system_agent"}, "lane": "system_agent", "turn_id": "s1", "unrecorded": true}, + {"role": "user", "content": "SLOW:40", "display_text": "SLOW:40", "speaker": {"kind": "person"}, "lane": "person", "turn_id": "p2", "unrecorded": true}, + {"role": "user", "content": "[from the system agent] SLOW:3", "lane": "system_agent", "speaker": {"kind": "system_agent"}, "display_text": "SLOW:3", "thread_id": "s2"}, + {"role": "assistant", "content": "SLOW DONE after 3 s", "lane": "system_agent", "thread_id": "s2"}, + ]); + c.load_history(&history); + assert_eq!(transcript(&c), live, "the reload keeps every notice in its place"); + c.load_history(&history); + assert_eq!(transcript(&c), live, "and so does the next one"); + // A notice from before any row stays first; one after rows the history + // no longer has goes last. + let mut c = Conversation::new("News"); + c.notice("Could not load the conversation: closed"); + c.apply(&started(LANE_PERSON, "gone", "a turn the history lost", "person")); + c.apply(&env(LANE_PERSON, "gone", 2, "turn_terminal", json!({"outcome": "completed"}))); + c.notice("Stopped."); + c.load_history(&json!([{"role": "user", "content": "earlier", "lane": "person", "thread_id": "p0"}])); + assert_eq!(transcript(&c), ["(Could not load the conversation: closed)", "You: earlier", "(Stopped.)"]); +} + +/// The history's tool rows carry no tool name (octos's hydrate rows have +/// none), so a reload showed "⚙ tool · done". A row keeps the name the +/// panel showed for the same turn's call; a turn the panel never followed +/// stays "tool" (never another call's name). +#[test] +fn a_reload_keeps_the_tool_names_the_panel_showed() { + let mut c = Conversation::new("News"); + c.apply(&started(LANE_PERSON, "p1", "SCN_ASK", "person")); + c.apply(&env(LANE_PERSON, "p1", 2, "tool_start", json!({"tool_call_id": "c1", "name": "ask_user_question"}))); + c.apply(&env(LANE_PERSON, "p1", 3, "tool_end", json!({"tool_call_id": "c1", "status": "complete"}))); + c.apply(&env(LANE_PERSON, "p1", 4, "assistant_persisted", json!({"assistant_segment_id": "p1:1", "text": "SCN AUDIENCE Team"}))); + c.apply(&env(LANE_PERSON, "p1", 5, "turn_terminal", json!({"outcome": "completed"}))); + c.load_history(&json!([ + {"role": "user", "content": "[from the person: News] older", "lane": "person", "speaker": {"kind": "person"}, "display_text": "older", "thread_id": "p0"}, + {"role": "assistant", "content": "", "lane": "person", "thread_id": "p0"}, + {"role": "tool", "content": "{\"ok\":true}", "lane": "person", "thread_id": "p0"}, + {"role": "user", "content": "[from the person: News] SCN_ASK", "lane": "person", "speaker": {"kind": "person"}, "display_text": "SCN_ASK", "thread_id": "p1"}, + {"role": "assistant", "content": "", "lane": "person", "thread_id": "p1"}, + {"role": "tool", "content": "{\"ok\":true,\"kind\":\"user_question_answer\"}", "lane": "person", "thread_id": "p1"}, + {"role": "assistant", "content": "SCN AUDIENCE Team", "lane": "person", "thread_id": "p1"}, + ])); + assert_eq!(transcript(&c), ["You: older", "[tool]", "You: SCN_ASK", "[ask_user_question]", "News's agent: SCN AUDIENCE Team"]); +} + /// The live run's order (#184 follow-up): each lane counts its own /// `cursor.seq` (the peer's session is long, the person's context new), and /// the kernel sends a turn's user message when the turn ENDS. Replayed as diff --git a/crates/shell/src/system_chat/model.rs b/crates/shell/src/system_chat/model.rs index a92e11d7..250a937b 100644 --- a/crates/shell/src/system_chat/model.rs +++ b/crates/shell/src/system_chat/model.rs @@ -17,7 +17,10 @@ //! agent's on the person's behalf; an approval of any other turn on the //! session (an external client's, e.g. Talk to Octos) is marked //! [`ApprovalAsk::external`] and shown read-only: that client answers it. -//! - **History**: `session/hydrate`'s rows replace the transcript. +//! - **History**: `session/hydrate`'s rows replace the transcript; its tool +//! rows keep the names the transcript showed ([`ToolNames`]). + +use std::collections::HashMap; use serde_json::Value; @@ -565,19 +568,22 @@ impl ChatModel { } /// The transcript from `session/hydrate` (`result.messages`), replacing - /// what the pane showed. Rows are `{role, content, turn_id?}`. + /// what the pane showed. Rows are `{role, content, thread_id?, + /// turn_id?}` ([`history_turn`]); a tool row is named by [`ToolNames`]. pub fn load_history(&mut self, messages: &Value) { + let rows = messages.as_array().map(Vec::as_slice).unwrap_or(&[]); + let mut tools = ToolNames::new(&self.items, rows); let mut items = Vec::new(); - for row in messages.as_array().into_iter().flatten() { + for row in rows { // The shell's note to the system agent is not the person's words. let text = crate::agents::strip_note(row.get("content").and_then(Value::as_str).unwrap_or("")).to_string(); - let turn = row.get("turn_id").and_then(Value::as_str).map(str::to_string); + let turn = history_turn(row); match row.get("role").and_then(Value::as_str).unwrap_or("") { "user" => items.push(Item::Message { role: Role::User, text, turn, segment: None, seq: None, speaker: None }), "assistant" if !text.trim().is_empty() => items.push(Item::Message { role: Role::Assistant, text, turn, segment: None, seq: None, speaker: None }), "tool" => items.push(Item::Tool { call_id: row.get("tool_call_id").and_then(Value::as_str).unwrap_or("").to_string(), - name: row.get("name").or_else(|| row.get("tool_name")).and_then(Value::as_str).unwrap_or("tool").to_string(), + name: tools.name(row, turn.as_deref()), status: ToolStatus::Done, detail: String::new(), turn, @@ -610,6 +616,69 @@ impl ChatModel { } } +/// A history row's turn: its `turn_id`, else its `thread_id`. octos's +/// `session/hydrate` rows have no `turn_id` (its `HydratedMessage` leaves it +/// unset), but every row a turn persists carries `thread_id` = that turn's +/// id (`pre_stamp_turn_thread_id`); the broker's rows for a turn the kernel +/// never recorded carry `turn_id`. +pub fn history_turn(row: &Value) -> Option { + row.get("turn_id").and_then(Value::as_str).or_else(|| row.get("thread_id").and_then(Value::as_str)).map(str::to_string) +} + +/// What a reloaded history's tool rows are called. The rows carry no tool +/// name: octos's `HydratedMessage` (the pinned revision) is `seq`, `role`, +/// `content`, `thread_id`, `persisted_at` and the like, with no tool name or +/// call id (the name is only on the assistant's tool call, which hydrate +/// does not return), and the broker's merged history adds none. So a +/// reloaded tool row keeps the name the transcript already showed for it +/// (its live `tool/started`, or an earlier reload): a turn's tool rows, in +/// order, when the transcript showed as many for that turn. A row's own +/// `name` or `tool_name` comes first, should a kernel send one; a row the +/// transcript never showed in full (another run's, a turn followed only in +/// part) reads "tool". +pub struct ToolNames { + /// The names the replaced transcript showed, per turn, in order. + shown: HashMap>, + /// How many tool rows the history has per turn. + rows: HashMap, + /// How many of a turn's rows were named so far. + named: HashMap, +} + +impl ToolNames { + /// For `rows` (the history) replacing `items` (the transcript). + pub fn new(items: &[Item], rows: &[Value]) -> ToolNames { + let mut shown: HashMap> = HashMap::new(); + for item in items { + if let Item::Tool { name, turn: Some(turn), .. } = item { + shown.entry(turn.clone()).or_default().push(name.clone()); + } + } + let mut per_turn: HashMap = HashMap::new(); + for row in rows.iter().filter(|r| r.get("role").and_then(Value::as_str) == Some("tool")) { + if let Some(turn) = history_turn(row) { + *per_turn.entry(turn).or_default() += 1; + } + } + ToolNames { shown, rows: per_turn, named: HashMap::new() } + } + + /// The name of `row`, the next tool row of `turn`. + pub fn name(&mut self, row: &Value, turn: Option<&str>) -> String { + let at = turn.map(|turn| { + let next = self.named.entry(turn.to_string()).or_default(); + *next += 1; + *next - 1 + }); + let own = row.get("name").and_then(Value::as_str).or_else(|| row.get("tool_name").and_then(Value::as_str)).filter(|n| !n.is_empty()); + let shown = match (turn, at) { + (Some(turn), Some(at)) => self.shown.get(turn).filter(|names| names.len() == self.rows.get(turn).copied().unwrap_or(0)).and_then(|names| names.get(at)), + _ => None, + }; + own.or(shown.map(String::as_str)).unwrap_or("tool").to_string() + } +} + /// A provider's error as the person may see it: every word that carries a /// key, or a visible piece of one, becomes `[key]`. Providers quote a masked /// key back ("Your api key: ****fcb0 is invalid", "sk-proj-****abcd"), and diff --git a/crates/shell/src/system_chat/tests.rs b/crates/shell/src/system_chat/tests.rs index 38c82af7..11b35a41 100644 --- a/crates/shell/src/system_chat/tests.rs +++ b/crates/shell/src/system_chat/tests.rs @@ -295,6 +295,55 @@ fn history_replaces_the_transcript_and_keeps_what_is_still_open() { assert_eq!(m.open_question(), Some(("q1", 1)), "an open question survives a reload"); } +/// `session/hydrate`'s rows carry no tool name, so a reloaded tool row read +/// "⚙ tool · done" where the live one had said "peer_send_input". It keeps +/// the name the chat showed for the same turn's call (octos stamps each row +/// a turn persists with `thread_id` = the turn id), across reloads; a turn +/// the chat never saw in full stays "tool", never another call's name. +#[test] +fn a_reload_keeps_the_tool_names_the_chat_showed() { + let mut m = ChatModel::new(); + m.start_turn("t1", "MAIL_NOTIFY"); + m.apply("tool/started", &json!({"turn_id": "t1", "tool_call_id": "c1", "tool_name": "peer_send_input", "arguments": {"slug": "os-mail-1"}})); + m.apply("tool/completed", &json!({"turn_id": "t1", "tool_call_id": "c1", "tool_name": "peer_send_input", "success": true})); + m.apply("message/delta", &json!({"turn_id": "t1", "text": "DELEGATED"})); + m.apply("turn/completed", &json!({"turn_id": "t1"})); + // A turn the chat followed only in part: one of its two calls. + m.apply("tool/started", &json!({"turn_id": "t2", "tool_call_id": "c3", "tool_name": "web_fetch"})); + m.apply("tool/completed", &json!({"turn_id": "t2", "tool_call_id": "c3", "tool_name": "web_fetch", "success": true})); + // The rows octos's session/hydrate returns for them: no name, no call id. + let row = |seq: u64, role: &str, content: &str, thread: &str| json!({"seq": seq, "role": role, "content": content, "thread_id": thread, "persisted_at": "2026-10-02T05:56:00Z"}); + let history = json!([ + row(0, "user", "an earlier run's request", "t0"), + row(1, "assistant", "", "t0"), + row(2, "tool", "[]", "t0"), + row(3, "user", "MAIL_NOTIFY", "t1"), + row(4, "assistant", "", "t1"), + row(5, "tool", "message sent to peer os-mail-1", "t1"), + row(6, "assistant", "DELEGATED", "t1"), + row(7, "user", "read two pages", "t2"), + row(8, "tool", "page one", "t2"), + row(9, "tool", "page two", "t2"), + ]); + let tools = |m: &ChatModel| -> Vec<(String, Option)> { + m.items.iter().filter_map(|i| match i { + Item::Tool { name, turn, .. } => Some((name.clone(), turn.clone())), + _ => None, + }).collect() + }; + let expected = vec![ + ("tool".to_string(), Some("t0".to_string())), + ("peer_send_input".to_string(), Some("t1".to_string())), + ("tool".to_string(), Some("t2".to_string())), + ("tool".to_string(), Some("t2".to_string())), + ]; + m.load_history(&history); + assert_eq!(tools(&m), expected); + m.load_history(&history); + assert_eq!(tools(&m), expected, "a second reload keeps it"); + assert_eq!(text_of(&m, Role::Assistant), ["DELEGATED"], "an empty tool-call row shows nothing"); +} + #[test] fn an_approval_is_handed_on_never_answered_by_the_model() { let mut m = ChatModel::new();