From 71406edbd58e53f1aac95a11023e5addd5fdfbc3 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 18 Sep 2026 01:37:10 +0000 Subject: [PATCH] Keep TUI exploration grouped across reasoning and nonzero exits (#46487) ## Why Reasoning summaries and nonzero command exits split adjacent exploration into separate groups, while compact history omitted exit codes. Replayed commands also failed to retain the same grouping as live commands. ## What changed - Keep adjacent read, list, and search commands grouped across reasoning summaries and nonzero exits in both live and replayed history. - Preserve reasoning in chronological order in the expanded transcript while omitting it from compact and raw history. - Show nonzero exit codes in compact exploration entries and keep unsuccessful reads separate from successful read summaries. Render search exit code `1` without red failure styling, and label compound-command outcomes as `command exit`. ## Testing Add regression coverage for live/replay rendering parity, reasoning order, grouping boundaries, and nonzero exit labels and colors. Update the overlapping-command test to verify that exploration stays grouped after a failure. GitOrigin-RevId: 19c6ffec44c62c185fc6f79282c38e66468f7b80 --- .../tui/src/chatwidget/command_lifecycle.rs | 25 ++- codex-rs/tui/src/chatwidget/streaming.rs | 15 +- .../tui/src/chatwidget/tests/exec_flow.rs | 153 +++++++++++++++++- ...ps_across_reasoning_live_and_replayed.snap | 69 ++++++++ ...emain_visible_beside_successful_reads.snap | 12 ++ codex-rs/tui/src/exec_cell/mod.rs | 1 + codex-rs/tui/src/exec_cell/model.rs | 15 +- codex-rs/tui/src/exec_cell/render.rs | 92 +++++------ codex-rs/tui/src/exec_cell/transcript.rs | 76 +++++++++ 9 files changed, 387 insertions(+), 71 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__adjacent_exploration_groups_across_reasoning_live_and_replayed.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__exploration_nonzero_exits_remain_visible_beside_successful_reads.snap create mode 100644 codex-rs/tui/src/exec_cell/transcript.rs diff --git a/codex-rs/tui/src/chatwidget/command_lifecycle.rs b/codex-rs/tui/src/chatwidget/command_lifecycle.rs index 00e1ea2a6d..ddfeef26a2 100644 --- a/codex-rs/tui/src/chatwidget/command_lifecycle.rs +++ b/codex-rs/tui/src/chatwidget/command_lifecycle.rs @@ -431,7 +431,6 @@ impl ChatWidget { self.request_redraw(); } ExecEndTarget::NewCell => { - self.flush_active_cell(); let mut cell = new_active_exec_command( id.clone(), command, @@ -442,12 +441,28 @@ impl ChatWidget { ); let completed = cell.complete_call(&id, output, duration); debug_assert!(completed, "new exec cell should contain {id}"); - if cell.should_flush() { - self.add_to_history(cell); - } else { - self.transcript.active_cell = Some(Box::new(cell)); + if let Some(active) = self + .transcript + .active_cell + .as_mut() + .and_then(|cell| cell.as_any_mut().downcast_mut::()) + && !active.is_active() + && active.is_exploring_cell() + && cell.is_exploring_cell() + { + // Replayed commands have completion events without matching starts. + active.calls.extend(cell.calls); self.bump_active_cell_revision(); self.request_redraw(); + } else { + self.flush_active_cell(); + if cell.should_flush() { + self.add_to_history(cell); + } else { + self.transcript.active_cell = Some(Box::new(cell)); + self.bump_active_cell_revision(); + self.request_redraw(); + } } } } diff --git a/codex-rs/tui/src/chatwidget/streaming.rs b/codex-rs/tui/src/chatwidget/streaming.rs index 5c1cc08f56..f1deccd655 100644 --- a/codex-rs/tui/src/chatwidget/streaming.rs +++ b/codex-rs/tui/src/chatwidget/streaming.rs @@ -350,7 +350,20 @@ impl ChatWidget { if !self.reasoning_summary_parts.is_empty() { let reasoning_parts = std::mem::take(&mut self.reasoning_summary_parts); let cell = history_cell::new_reasoning_summary_block(reasoning_parts, &self.config.cwd); - self.add_boxed_history(cell); + if let Some(exec) = self + .transcript + .active_cell + .as_mut() + .and_then(|cell| cell.as_any_mut().downcast_mut::()) + && exec.is_exploring_cell() + && !exec.is_active() + { + // Keep adjacent exploration grouped while retaining reasoning in transcript order. + exec.reasoning.push((exec.calls.len(), cell)); + self.bump_active_cell_revision(); + } else { + self.add_boxed_history(cell); + } } self.reasoning_buffer.clear(); // Keep the last useful summary through tools and later empty items. diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index 8df2eae2c4..5c23db1702 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -141,12 +141,10 @@ async fn failed_exploration_keeps_overlapping_commands_active_until_all_finish() assert!(drain_insert_history(&mut rx).is_empty()); end_exec(&mut chat, followup, "followup\n", "", /*exit_code*/ 0); - let cells = drain_insert_history(&mut rx); - assert_eq!(cells.len(), 1); - let history = lines_to_single_string(&cells[0]); - insta::assert_snapshot!(history, @r" + assert!(drain_insert_history(&mut rx).is_empty()); + insta::assert_snapshot!(active_blob(&chat), @r" • Explored - └ List missing + └ List missing (exit 1) Read foo.txt, bar.txt "); @@ -154,10 +152,153 @@ async fn failed_exploration_keeps_overlapping_commands_active_until_all_finish() end_exec(&mut chat, later, "later\n", "", /*exit_code*/ 0); insta::assert_snapshot!(active_blob(&chat), @r" • Explored - └ Read later.txt + └ List missing (exit 1) + Read foo.txt, bar.txt, later.txt "); } +#[tokio::test] +async fn exploration_nonzero_exits_remain_visible_beside_successful_reads() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + for (id, command, exit_code) in [ + ("read", "cat first.txt", 0), + ("missing", "cat missing.txt", 1), + ("later", "cat later.txt", 0), + ("search", "rg absent .", 1), + ("error", "rg text missing.txt", 2), + ("compound", "cat existing.txt && rg absent .", 1), + ] { + let item = begin_exec(&mut chat, id, command); + end_exec(&mut chat, item, "", "", exit_code); + } + insta::assert_snapshot!(active_blob(&chat)); + let statuses = chat + .transcript + .active_cell + .as_ref() + .unwrap() + .display_lines(/*width*/ 80) + .into_iter() + .flat_map(|line| line.spans) + .filter(|span| { + span.content.starts_with(" (exit") || span.content.starts_with(" (command exit") + }) + .map(|span| (span.content.into_owned(), span.style.fg)) + .collect::>(); + assert_eq!( + statuses, + vec![ + (" (exit 1)".to_string(), Some(ratatui::style::Color::Red)), + (" (exit 1)".to_string(), None), + (" (exit 2)".to_string(), Some(ratatui::style::Color::Red)), + (" (command exit 1)".to_string(), None), + ] + ); +} + +#[tokio::test] +async fn adjacent_exploration_groups_across_reasoning_live_and_replayed() { + let mut renders = Vec::new(); + for replay in [false, true] { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.on_task_started(); + for (id, script, exit_code) in [ + ("list", "ls missing", 1), + ("read", "cat index.html", 0), + ("run", "echo boundary", 0), + ("later", "cat later.txt", 0), + ("after-message", "cat final.txt", 0), + ] { + let command = vec!["bash".to_string(), "-lc".to_string(), script.to_string()]; + let mut item = AppServerThreadItem::CommandExecution { + model_context: None, + id: id.to_string(), + command: codex_shell_command::parse_command::shlex_join(&command), + cwd: chat.config.cwd.clone().into(), + process_id: None, + plugin_id: None, + script_path: None, + source: ExecCommandSource::UnifiedExecStartup, + status: AppServerCommandExecutionStatus::InProgress, + command_actions: codex_shell_command::parse_command::parse_command(&command) + .into_iter() + .map(|parsed| { + AppServerCommandAction::from_core_with_cwd(parsed, &chat.config.cwd) + }) + .collect(), + aggregated_output: Some(format!("{id} output\n")), + exit_code: Some(exit_code), + duration_ms: Some(5), + }; + if !replay { + handle_exec_begin(&mut chat, item.clone()); + } + if let AppServerThreadItem::CommandExecution { status, .. } = &mut item { + *status = if exit_code == 0 { + AppServerCommandExecutionStatus::Completed + } else { + AppServerCommandExecutionStatus::Failed + }; + } + if replay { + chat.replay_thread_item(item, "turn-1".to_string(), ReplayKind::ThreadSnapshot); + } else { + handle_exec_end(&mut chat, item); + } + if id == "list" { + for summary in ["Inspecting the page", "Checking its contents"] { + if replay { + chat.replay_thread_item( + AppServerThreadItem::Reasoning { + id: summary.to_string(), + summary: vec![summary.to_string()], + content: Vec::new(), + }, + "turn-1".to_string(), + ReplayKind::ThreadSnapshot, + ); + } else { + chat.on_agent_reasoning_delta(summary.to_string()); + chat.on_agent_reasoning_final(); + } + } + } + if id == "later" { + complete_assistant_message( + &mut chat, + "message", + "Checking one more file.", + Some(MessagePhase::Commentary), + ); + } + } + chat.flush_active_cell(); + let cells = std::iter::from_fn(|| rx.try_recv().ok()) + .filter_map(|event| match event { + AppEvent::InsertHistoryCell(cell) => Some(cell), + _ => None, + }) + .collect::>(); + assert_eq!(cells.len(), 5); + let mut render = String::new(); + for mode in ["Compact", "Expanded", "Raw"] { + render.push_str(&format!("{mode}:\n")); + for cell in &cells { + let lines = match mode { + "Compact" => cell.display_lines(/*width*/ 80), + "Expanded" => cell.transcript_lines(/*width*/ 80), + _ => cell.raw_lines(), + }; + render.push_str(&lines_to_single_string(&lines)); + render.push('\n'); + } + } + renders.push(render); + } + assert_eq!(renders[0], renders[1]); + insta::assert_snapshot!(renders[0]); +} + #[tokio::test] async fn replayed_commands_preserve_individual_output_and_failure_status() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__adjacent_exploration_groups_across_reasoning_live_and_replayed.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__adjacent_exploration_groups_across_reasoning_live_and_replayed.snap new file mode 100644 index 0000000000..d4e4fe5997 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__adjacent_exploration_groups_across_reasoning_live_and_replayed.snap @@ -0,0 +1,69 @@ +--- +source: tui/src/chatwidget/tests/exec_flow.rs +expression: "renders[0]" +--- +Compact: +• Explored + └ List missing (exit 1) + Read index.html + +• Ran echo boundary + └ run output + +• Explored + └ Read later.txt + +• Checking one more file. + +• Explored + └ Read final.txt + +Expanded: +$ ls missing +list output +✗ (1) • 5ms + +• Inspecting the page + +• Checking its contents + +$ cat index.html +read output +✓ • 5ms + +$ echo boundary +run output +✓ • 5ms + +$ cat later.txt +later output +✓ • 5ms + +• Checking one more file. + +$ cat final.txt +after-message output +✓ • 5ms + +Raw: +$ ls missing +list output +✗ (1) • 5ms + +$ cat index.html +read output +✓ • 5ms + +$ echo boundary +run output +✓ • 5ms + +$ cat later.txt +later output +✓ • 5ms + +Checking one more file. + +$ cat final.txt +after-message output +✓ • 5ms diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__exploration_nonzero_exits_remain_visible_beside_successful_reads.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__exploration_nonzero_exits_remain_visible_beside_successful_reads.snap new file mode 100644 index 0000000000..9779d552ec --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__exec_flow__exploration_nonzero_exits_remain_visible_beside_successful_reads.snap @@ -0,0 +1,12 @@ +--- +source: tui/src/chatwidget/tests/exec_flow.rs +expression: active_blob(&chat) +--- +• Explored + └ Read first.txt + Read missing.txt (exit 1) + Read later.txt + Search absent in . (exit 1) + Search text in missing.txt (exit 2) + Read existing.txt + Search absent in . (command exit 1) diff --git a/codex-rs/tui/src/exec_cell/mod.rs b/codex-rs/tui/src/exec_cell/mod.rs index 40bef5d6b3..033a949a80 100644 --- a/codex-rs/tui/src/exec_cell/mod.rs +++ b/codex-rs/tui/src/exec_cell/mod.rs @@ -1,6 +1,7 @@ mod live_output; mod model; mod render; +mod transcript; pub(crate) use model::CommandOutput; #[cfg(test)] diff --git a/codex-rs/tui/src/exec_cell/model.rs b/codex-rs/tui/src/exec_cell/model.rs index e5fc309e33..3eb2a2431e 100644 --- a/codex-rs/tui/src/exec_cell/model.rs +++ b/codex-rs/tui/src/exec_cell/model.rs @@ -4,12 +4,15 @@ //! list/search commands. The chat widget relies on stable `call_id` matching to route progress and //! end events into the right cell, and it treats "call id not found" as a real signal (for //! example, an orphan end that should render as a separate history entry). +//! Transcript-only reasoning stays inside completed exploration groups so it does not split their +//! compact display, while the expanded transcript retains its position between commands. use std::borrow::Cow; use std::time::Duration; use std::time::Instant; use super::live_output::LiveCommandOutput; +use crate::history_cell::HistoryCell; use codex_app_server_protocol::CommandExecutionSource as ExecCommandSource; use codex_protocol::parse_command::ParsedCommand; use itertools::Either; @@ -75,6 +78,8 @@ pub(crate) struct ExecCall { #[derive(Debug)] pub(crate) struct ExecCell { pub(crate) calls: Vec, + /// Transcript-only reasoning, paired with the number of calls preceding it. + pub(crate) reasoning: Vec<(usize, Box)>, animations_enabled: bool, } @@ -82,6 +87,7 @@ impl ExecCell { pub(crate) fn new(call: ExecCall, animations_enabled: bool) -> Self { Self { calls: vec![call], + reasoning: Vec::new(), animations_enabled, } } @@ -133,14 +139,7 @@ impl ExecCell { } pub(crate) fn should_flush(&self) -> bool { - if self.calls.iter().any(|call| { - call.output - .as_ref() - .is_some_and(|output| output.exit_code != 0) - }) { - return !self.is_active(); - } - + // Exploration stays open for adjacent calls, including after a failed read/list/search. !self.is_exploring_cell() && self.calls.iter().all(|c| c.duration.is_some()) } diff --git a/codex-rs/tui/src/exec_cell/render.rs b/codex-rs/tui/src/exec_cell/render.rs index ed091515d3..13574fe7fa 100644 --- a/codex-rs/tui/src/exec_cell/render.rs +++ b/codex-rs/tui/src/exec_cell/render.rs @@ -5,6 +5,7 @@ use super::model::ExecCall; use super::model::ExecCell; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell::HistoryCell; +use crate::history_cell::HistoryRenderMode; use crate::history_cell::plain_lines; use crate::motion::MotionMode; use crate::motion::ReducedMotionIndicator; @@ -15,12 +16,10 @@ use crate::render::line_utils::push_owned_lines; use crate::ui_consts::TRANSCRIPT_HINT; use crate::wrapping::RtOptions; use crate::wrapping::adaptive_wrap_line; -use crate::wrapping::adaptive_wrap_lines; use codex_ansi_escape::ansi_escape_line; use codex_app_server_protocol::CommandExecutionSource as ExecCommandSource; use codex_protocol::parse_command::ParsedCommand; use codex_shell_command::bash::extract_bash_command; -use codex_utils_elapsed::format_duration; use itertools::Itertools; use ratatui::prelude::*; use ratatui::style::Modifier; @@ -193,53 +192,11 @@ impl HistoryCell for ExecCell { } fn transcript_lines(&self, width: u16) -> Vec> { - let mut lines: Vec> = vec![]; - for (i, call) in self.iter_calls().enumerate() { - if i > 0 { - lines.push("".into()); - } - let script = strip_bash_lc_and_escape(&call.command); - let highlighted_script = highlight_bash_to_lines(&script); - let cmd_display = adaptive_wrap_lines( - &highlighted_script, - RtOptions::new(width as usize) - .initial_indent("$ ".magenta().into()) - .subsequent_indent(" ".into()), - ); - lines.extend(cmd_display); - - if let Some(output) = call.output.as_ref() { - if !call.is_unified_exec_interaction() { - let wrap_width = width.max(1) as usize; - let wrap_opts = RtOptions::new(wrap_width); - for unwrapped in output - .transcript_lines() - .map(|line| ansi_escape_line(line.as_ref())) - { - let wrapped = adaptive_wrap_line(&unwrapped, wrap_opts.clone()); - push_owned_lines(&wrapped, &mut lines); - } - } - if let Some(duration) = call.duration { - let duration = format_duration(duration); - let mut result: Line = if output.exit_code == 0 { - Line::from("✓".green().bold()) - } else { - Line::from(vec![ - "✗".red().bold(), - format!(" ({})", output.exit_code).into(), - ]) - }; - result.push_span(format!(" • {duration}").dim()); - lines.push(result); - } - } - } - lines + self.detailed_lines(width, HistoryRenderMode::Rich) } fn raw_lines(&self) -> Vec> { - plain_lines(self.transcript_lines(u16::MAX)) + plain_lines(self.detailed_lines(u16::MAX, HistoryRenderMode::Raw)) } } @@ -270,18 +227,27 @@ impl ExecCell { let mut calls = self.calls.as_slice(); let mut out_indented = Vec::new(); + let nonzero_exit = |call: &ExecCall| { + call.duration + .and(call.output.as_ref()) + .map(|output| output.exit_code) + .filter(|code| *code != 0) + }; while let Some((call, remaining)) = calls.split_first() { + let exit_code = nonzero_exit(call); let reads_only = call .parsed .iter() .all(|parsed| matches!(parsed, ParsedCommand::Read { .. })); - let group_len = if reads_only { + let group_len = if reads_only && exit_code.is_none() { 1 + remaining .iter() .take_while(|next| { - next.parsed - .iter() - .all(|parsed| matches!(parsed, ParsedCommand::Read { .. })) + nonzero_exit(next).is_none() + && next + .parsed + .iter() + .all(|parsed| matches!(parsed, ParsedCommand::Read { .. })) }) .count() } else { @@ -331,7 +297,31 @@ impl ExecCell { lines }; - for (title, line) in call_lines { + let line_count = call_lines.len(); + for (index, (title, mut line)) in call_lines.into_iter().enumerate() { + if let Some(code) = exit_code + && index + 1 == line_count + { + // A compound command has one exit code, not an outcome for each parsed action. + let status = if call.parsed.len() > 1 { + format!(" (command exit {code})") + } else { + format!(" (exit {code})") + }; + // Search exit 1 can mean no matches; report the code without calling it a failure. + line.push( + if code == 1 + && call + .parsed + .iter() + .any(|p| matches!(p, ParsedCommand::Search { .. })) + { + status.dim() + } else { + status.red() + }, + ); + } let line = Line::from(line); let initial_indent = Line::from(vec![title.cyan(), " ".into()]); let subsequent_indent = " ".repeat(initial_indent.width()).into(); diff --git a/codex-rs/tui/src/exec_cell/transcript.rs b/codex-rs/tui/src/exec_cell/transcript.rs new file mode 100644 index 0000000000..f25e766904 --- /dev/null +++ b/codex-rs/tui/src/exec_cell/transcript.rs @@ -0,0 +1,76 @@ +//! Expanded command history, preserving reasoning between grouped exploration calls. +//! +//! Compact history groups adjacent exploration; expanded history retains chronological details, +//! and raw history continues to omit transcript-only reasoning. + +use super::model::ExecCell; +use crate::exec_command::strip_bash_lc_and_escape; +use crate::history_cell::HistoryRenderMode; +use crate::render::highlight::highlight_bash_to_lines; +use crate::render::line_utils::push_owned_lines; +use crate::wrapping::RtOptions; +use crate::wrapping::adaptive_wrap_line; +use crate::wrapping::adaptive_wrap_lines; +use codex_ansi_escape::ansi_escape_line; +use codex_utils_elapsed::format_duration; +use ratatui::prelude::*; + +impl ExecCell { + pub(super) fn detailed_lines(&self, width: u16, mode: HistoryRenderMode) -> Vec> { + let mut lines: Vec> = vec![]; + let mut reasoning = self.reasoning.iter().peekable(); + for (i, call) in self.iter_calls().enumerate() { + if i > 0 { + lines.push("".into()); + } + let script = strip_bash_lc_and_escape(&call.command); + let highlighted_script = highlight_bash_to_lines(&script); + let cmd_display = adaptive_wrap_lines( + &highlighted_script, + RtOptions::new(width as usize) + .initial_indent("$ ".magenta().into()) + .subsequent_indent(" ".into()), + ); + lines.extend(cmd_display); + + if let Some(output) = call.output.as_ref() { + if !call.is_unified_exec_interaction() { + let wrap_width = width.max(1) as usize; + let wrap_opts = RtOptions::new(wrap_width); + for unwrapped in output + .transcript_lines() + .map(|line| ansi_escape_line(line.as_ref())) + { + let wrapped = adaptive_wrap_line(&unwrapped, wrap_opts.clone()); + push_owned_lines(&wrapped, &mut lines); + } + } + if let Some(duration) = call.duration { + let duration = format_duration(duration); + let mut result: Line = if output.exit_code == 0 { + Line::from("✓".green().bold()) + } else { + Line::from(vec![ + "✗".red().bold(), + format!(" ({})", output.exit_code).into(), + ]) + }; + result.push_span(format!(" • {duration}").dim()); + lines.push(result); + } + } + if mode == HistoryRenderMode::Rich { + while let Some((_, cell)) = + reasoning.next_if(|(after_calls, _)| *after_calls == i + 1) + { + let reasoning_lines = cell.transcript_lines(width); + if !reasoning_lines.is_empty() { + lines.push("".into()); + lines.extend(reasoning_lines); + } + } + } + } + lines + } +}