From 4abcb8d1dacc1343c7695acf815b096089e5fe9e Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 19 Sep 2026 20:09:58 +0000 Subject: [PATCH] Restore rich tool details in persisted TUI transcripts (#46710) ## Why Loaded transcripts reduce several tool calls and file changes to basic status summaries, losing details available in live history cells. ## What changed - Reuse rich history cells for completed commands, MCP calls, patches, agent activity, image generation, and notices. - Add dynamic tool cells with bounded output previews while retaining arguments and full output in detailed and raw views. - Preserve last-known statuses for incomplete calls, unavailable results, and declined or failed operations. Keep legacy command text and terminal interaction output when rich reconstruction is unavailable. - Remove redundant move annotations from patch diffs and retain patch activity IDs. - Filter untrusted control characters before emitting terminal scrollback, while preserving semantic hyperlinks. ## Testing Add unit and snapshot coverage for compact and detailed tool rendering, status fallbacks, retained output, patch restoration, and control-character filtering with and without hyperlinks. GitOrigin-RevId: 73d754262768cf6d91f3ea4040116bf93cf87784 --- .../src/app_server_approval_conversions.rs | 19 +- codex-rs/tui/src/history_cell/dynamic.rs | 275 ++++++++++++++ .../tui/src/history_cell/dynamic_tests.rs | 116 ++++++ codex-rs/tui/src/history_cell/mod.rs | 2 + codex-rs/tui/src/history_cell/patches.rs | 7 + ..._hidden_lines_and_retains_full_output.snap | 30 ++ ..._output_match_persisted_presentations.snap | 31 ++ codex-rs/tui/src/insert_history.rs | 43 ++- codex-rs/tui/src/terminal_hyperlinks.rs | 24 +- codex-rs/tui/src/thread_transcript.rs | 333 ++++++----------- .../tui/src/thread_transcript/other_items.rs | 148 ++++++++ .../thread_transcript/other_items_tests.rs | 190 ++++++++++ ...tatus_without_duplicating_v2_activity.snap | 9 + ...__tests__completed_tool_presentations.snap | 34 ++ ...ve_status_output_and_group_boundaries.snap | 52 +++ ...ls__tests__pending_tool_presentations.snap | 21 ++ codex-rs/tui/src/thread_transcript/tools.rs | 248 +++++++++++++ .../tui/src/thread_transcript/tools_tests.rs | 338 ++++++++++++++++++ 18 files changed, 1689 insertions(+), 231 deletions(-) create mode 100644 codex-rs/tui/src/history_cell/dynamic.rs create mode 100644 codex-rs/tui/src/history_cell/dynamic_tests.rs create mode 100644 codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__dynamic__tests__dynamic_preview_reports_hidden_lines_and_retains_full_output.snap create mode 100644 codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__dynamic__tests__dynamic_status_and_output_match_persisted_presentations.snap create mode 100644 codex-rs/tui/src/thread_transcript/other_items.rs create mode 100644 codex-rs/tui/src/thread_transcript/other_items_tests.rs create mode 100644 codex-rs/tui/src/thread_transcript/snapshots/codex_tui__thread_transcript__tools__tests__agent_tool_fallbacks_preserve_status_without_duplicating_v2_activity.snap create mode 100644 codex-rs/tui/src/thread_transcript/snapshots/codex_tui__thread_transcript__tools__tests__completed_tool_presentations.snap create mode 100644 codex-rs/tui/src/thread_transcript/snapshots/codex_tui__thread_transcript__tools__tests__historical_command_fallbacks_preserve_status_output_and_group_boundaries.snap create mode 100644 codex-rs/tui/src/thread_transcript/snapshots/codex_tui__thread_transcript__tools__tests__pending_tool_presentations.snap create mode 100644 codex-rs/tui/src/thread_transcript/tools.rs create mode 100644 codex-rs/tui/src/thread_transcript/tools_tests.rs diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index 2208bcabbf..1f0153cd2d 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -30,7 +30,7 @@ pub(crate) fn file_update_changes_to_display( ) -> HashMap { changes .into_iter() - .map(|change| { + .map(|mut change| { let path = PathBuf::from(change.path); let file_change = match change.kind { PatchChangeKind::Add => FileChange::Add { @@ -39,10 +39,19 @@ pub(crate) fn file_update_changes_to_display( PatchChangeKind::Delete => FileChange::Delete { content: change.diff, }, - PatchChangeKind::Update { move_path } => FileChange::Update { - unified_diff: change.diff, - move_path, - }, + PatchChangeKind::Update { move_path } => { + if let Some(path) = &move_path + && let Some(diff) = change + .diff + .strip_suffix(&format!("\n\nMoved to: {}", path.display())) + { + change.diff.truncate(diff.len()); + } + FileChange::Update { + unified_diff: change.diff, + move_path, + } + } }; (path, file_change) }) diff --git a/codex-rs/tui/src/history_cell/dynamic.rs b/codex-rs/tui/src/history_cell/dynamic.rs new file mode 100644 index 0000000000..c74599747f --- /dev/null +++ b/codex-rs/tui/src/history_cell/dynamic.rs @@ -0,0 +1,275 @@ +//! Dynamic tool activity with retained arguments and output across live and loaded transcripts. + +use super::HistoryCell; +use super::activity_preview::DETAIL_PREVIEW_LINES; +use super::activity_preview::clipped_line; +use super::raw_lines_from_source; +use crate::style::accent_color; +use crate::terminal_hyperlinks::HyperlinkLine; +use crate::terminal_hyperlinks::adaptive_wrap_hyperlink_lines; +use crate::terminal_hyperlinks::plain_hyperlink_lines; +use crate::terminal_hyperlinks::visible_lines; +use crate::wrapping::RtOptions; +use codex_app_server_protocol::DynamicToolCallOutputContentItem; +use codex_app_server_protocol::DynamicToolCallStatus; +use codex_app_server_protocol::ThreadItem; +use ratatui::style::Stylize; +use ratatui::text::Line; +use serde_json::Value; +use std::sync::Arc; +use std::sync::RwLock; + +#[derive(Clone, Debug)] +pub(crate) struct DynamicToolCallCell { + #[allow(dead_code, reason = "Used by later layers of the TUI refresh stack.")] + call_id: String, + data: Arc>, +} + +#[derive(Debug)] +struct DynamicToolCallData { + name: String, + arguments: Value, + status: DynamicToolCallStatus, + interrupted: bool, + output: Option>, + duration_ms: Option, +} + +impl DynamicToolCallCell { + pub(crate) fn from_item(item: ThreadItem) -> Option { + let (call_id, data) = DynamicToolCallData::from_item(item)?; + Some(Self { + call_id, + data: Arc::new(RwLock::new(data)), + }) + } + + #[allow(dead_code, reason = "Used by later layers of the TUI refresh stack.")] + pub(crate) fn call_id(&self) -> &str { + &self.call_id + } + + #[allow(dead_code, reason = "Used by later layers of the TUI refresh stack.")] + pub(crate) fn is_active(&self) -> bool { + self.data + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .is_active() + } + + /// Updates the original retained row when concurrent calls complete in a different order. + /// Callers revising an already-terminal cell must rebuild its cached renderables. + #[allow(dead_code, reason = "Used by later layers of the TUI refresh stack.")] + pub(crate) fn update_from_item(&self, item: ThreadItem) -> bool { + let Some((call_id, data)) = DynamicToolCallData::from_item(item) else { + return false; + }; + if call_id != self.call_id { + return false; + } + *self + .data + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner) = data; + true + } + + #[allow(dead_code, reason = "Used by later layers of the TUI refresh stack.")] + pub(crate) fn mark_interrupted(&self) { + let mut data = self + .data + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if !data.is_active() { + return; + } + data.status = DynamicToolCallStatus::Failed; + data.interrupted = true; + data.output + .get_or_insert_with(Vec::new) + .push("Interrupted before this tool returned a result.".to_owned()); + } +} + +impl DynamicToolCallData { + fn from_item(item: ThreadItem) -> Option<(String, Self)> { + let ThreadItem::DynamicToolCall { + id, + namespace, + tool, + arguments, + status, + content_items, + success, + duration_ms, + } = item + else { + return None; + }; + Some(( + id, + Self { + name: namespace + .map_or_else(|| tool.clone(), |namespace| format!("{namespace}.{tool}")), + arguments, + status: if success == Some(false) { + DynamicToolCallStatus::Failed + } else { + status + }, + interrupted: false, + output: content_items.map(|items| { + items + .into_iter() + .map(|item| match item { + DynamicToolCallOutputContentItem::InputText { text } => text, + DynamicToolCallOutputContentItem::InputImage { .. } => { + "".to_owned() + } + DynamicToolCallOutputContentItem::InputAudio { .. } => { + "