diff --git a/crates/tui/src/tui/active_cell.rs b/crates/tui/src/tui/active_cell.rs new file mode 100644 index 00000000..81080584 --- /dev/null +++ b/crates/tui/src/tui/active_cell.rs @@ -0,0 +1,375 @@ +//! Active in-flight tool/exec cell — single mutable group that buffers parallel +//! tool work for the current turn. +//! +//! ## Why +//! +//! When the model issues parallel tool calls in a single assistant turn (e.g. +//! two `read_file` and one `grep_files` running concurrently), naively +//! appending each tool start as its own history cell makes the transcript +//! "bounce" as completions arrive out of order. Codex's pattern is to keep all +//! in-flight tool work in ONE active cell that mutates in place; once the turn +//! resolves the active cell finalizes into the transcript. +//! +//! ## Contract +//! +//! - At most one [`ActiveCell`] per turn. It holds zero or more +//! [`HistoryCell`]s that are still being mutated (status `Running`, output +//! pending, etc.). +//! - The owning [`crate::tui::app::App`] renders the active cell's contents +//! AFTER `App.history` so they appear at the live tail. +//! - Cell indices used by helpers like `tool_cells` / `tool_details_by_cell` +//! address the virtual sequence `App.history ++ active_cell.entries`. Each +//! entry's index is `App.history.len() + entry_offset`. +//! - When a tool completes whose `tool_id` does not match any active entry +//! (orphan), the caller pushes a finalized standalone cell into `App.history` +//! instead of mutating the active group. This keeps `active_cell` a stable +//! reflection of what was actually started, and avoids merging unrelated +//! tool work. +//! - On `TurnComplete` (or cancellation) the active cell is "flushed": +//! in-progress entries are marked with the supplied terminal status, then +//! every entry is appended to `App.history`. Companion maps +//! (`tool_cells`, `tool_details_by_cell`) are rewritten to point at the new +//! `App.history` indices. +//! +//! ## Revision counter +//! +//! Cells inside the active group mutate without changing pointer identity, so +//! the transcript cache cannot rely on enum-equality for invalidation. We +//! expose `revision()` and `bump_revision()`; the renderer combines this with +//! `App.history_version` when computing per-cell revisions for the cache. + +use crate::tui::history::{ExploringCell, ExploringEntry, HistoryCell, ToolCell, ToolStatus}; + +/// In-flight active cell: a sequence of mutable [`HistoryCell`] entries. +/// +/// Conceptually a single "live tail" cell in the Codex sense: it appears as +/// one logical block at the end of the transcript, but internally it is +/// composed of one or more entries (each rendered as its own +/// [`HistoryCell`]). The reason we keep them as separate entries — rather +/// than fusing into a single conceptual block — is that they may have +/// different shapes (an `ExecCell`, an `ExploringCell` aggregate, an MCP +/// tool result, …) and the existing renderers already know how to draw each +/// shape correctly. Coalescing into a single render path would duplicate +/// logic we already have. +#[derive(Debug, Clone, Default)] +pub struct ActiveCell { + entries: Vec, + /// Tool ids currently associated with this active cell. The map values are + /// indices into [`Self::entries`]. Multiple tool ids can map to the same + /// entry (the existing `ExploringCell` aggregates several reads into a + /// single entry). + tool_to_entry: std::collections::HashMap, + /// Index of the current `ExploringCell` entry (when present), so additional + /// exploring tool starts append to it instead of creating new cells. + exploring_entry: Option, + /// Bumped on every mutation. Used by the transcript cache to know that + /// the active cell needs re-rendering even though its position in the + /// virtual cell list is unchanged. + revision: u64, +} + +impl ActiveCell { + /// Create an empty active cell. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Number of entries (each rendered as its own [`HistoryCell`]). + #[must_use] + #[allow(dead_code)] // Public surface used by tests and future renderers. + pub fn entry_count(&self) -> usize { + self.entries.len() + } + + /// Whether the active cell has any entries. + #[must_use] + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } + + /// Read-only access to the underlying entries (for rendering). + #[must_use] + pub fn entries(&self) -> &[HistoryCell] { + &self.entries + } + + /// Mutable access to a specific entry. Bumps the revision counter so the + /// renderer knows the cached lines are stale. + pub fn entry_mut(&mut self, index: usize) -> Option<&mut HistoryCell> { + if index < self.entries.len() { + self.bump_revision(); + self.entries.get_mut(index) + } else { + None + } + } + + /// Current revision counter. Wraps on overflow which is fine for cache + /// invalidation; the chance of a wrap-around collision is astronomical + /// over a single session and any miss only causes one extra re-render. + #[must_use] + #[allow(dead_code)] // Used by App::bump_active_cell_revision and future cache wiring. + pub fn revision(&self) -> u64 { + self.revision + } + + /// Increment the revision counter. Call any time an entry is mutated. + pub fn bump_revision(&mut self) { + self.revision = self.revision.wrapping_add(1); + } + + /// Add a tool entry to the active cell. + /// + /// Returns the entry index (which the caller can record in + /// `tool_cells_in_active`). If the cell is an exploring tool start and + /// there is already an exploring entry in the active group, the entry is + /// appended to that aggregate instead of creating a new entry. + /// + /// `tool_id` is registered for the new (or updated) entry so future + /// completion lookups can find it. + pub fn push_tool(&mut self, tool_id: impl Into, cell: HistoryCell) -> usize { + let tool_id = tool_id.into(); + // If this is an exploring start and we already have an exploring + // entry, append to that entry rather than creating a new cell. + if let HistoryCell::Tool(ToolCell::Exploring(new_cell)) = &cell + && let Some(entry_idx) = self.exploring_entry + && let Some(HistoryCell::Tool(ToolCell::Exploring(existing))) = + self.entries.get_mut(entry_idx) + { + // The caller hands us a brand-new ExploringCell with one entry. + // Move that entry into the existing aggregate. + for explore_entry in &new_cell.entries { + let _ = existing.insert_entry(explore_entry.clone()); + } + self.tool_to_entry.insert(tool_id, entry_idx); + self.bump_revision(); + return entry_idx; + } + + // Otherwise, push a new entry. + let entry_idx = self.entries.len(); + if matches!(cell, HistoryCell::Tool(ToolCell::Exploring(_))) { + self.exploring_entry = Some(entry_idx); + } + self.entries.push(cell); + self.tool_to_entry.insert(tool_id, entry_idx); + self.bump_revision(); + entry_idx + } + + /// Push an entry with no tool id binding (used for non-tool grouping if + /// ever needed). Currently unused; kept for symmetry with Codex which + /// allows e.g. session-header cells to live in `active_cell`. + #[allow(dead_code)] + pub fn push_untracked(&mut self, cell: HistoryCell) -> usize { + let entry_idx = self.entries.len(); + self.entries.push(cell); + self.bump_revision(); + entry_idx + } + + /// Look up the entry index that holds the given tool id. + #[must_use] + #[allow(dead_code)] // Reserved for the Codex-style "exec end target" lookup. + pub fn entry_index_for_tool(&self, tool_id: &str) -> Option { + self.tool_to_entry.get(tool_id).copied() + } + + /// Append an [`ExploringEntry`] to the existing exploring aggregate (if + /// any), binding the supplied tool id to it. Returns + /// `(entry_index, entry_within_exploring)` on success. + /// + /// Used when a second exploring tool starts during the same active group: + /// rather than allocating another ExploringCell entry in the active group + /// we extend the one that's already there. + pub fn append_to_exploring( + &mut self, + tool_id: impl Into, + explore_entry: ExploringEntry, + ) -> Option<(usize, usize)> { + let entry_idx = self.exploring_entry?; + let HistoryCell::Tool(ToolCell::Exploring(cell)) = self.entries.get_mut(entry_idx)? else { + return None; + }; + let inner_idx = cell.insert_entry(explore_entry); + self.tool_to_entry.insert(tool_id.into(), entry_idx); + self.bump_revision(); + Some((entry_idx, inner_idx)) + } + + /// Ensure an [`ExploringCell`] exists in the active group; create it if + /// not. Returns its entry index. + pub fn ensure_exploring(&mut self) -> usize { + if let Some(idx) = self.exploring_entry { + return idx; + } + let idx = self.entries.len(); + self.entries + .push(HistoryCell::Tool(ToolCell::Exploring(ExploringCell { + entries: Vec::new(), + }))); + self.exploring_entry = Some(idx); + self.bump_revision(); + idx + } + + /// Remove the tool-id binding for an entry without removing the entry + /// itself (the entry remains in the active group, presumably with its + /// status updated). + #[allow(dead_code)] // Reserved for cancellation paths that prune ids without flushing. + pub fn forget_tool(&mut self, tool_id: &str) -> Option { + self.tool_to_entry.remove(tool_id) + } + + /// Drain every entry, returning them in insertion order. Resets internal + /// state (revision is bumped via `bump_revision`). + /// + /// Callers use this on `TurnComplete` (or cancellation) to flush the + /// active group into `App.history`. + pub fn drain(&mut self) -> Vec { + let entries = std::mem::take(&mut self.entries); + self.tool_to_entry.clear(); + self.exploring_entry = None; + self.bump_revision(); + entries + } + + /// Mark every still-running tool entry as `Failed` (used when the turn is + /// cancelled mid-flight). Entries that already completed are left alone. + /// + /// `Failed` is the closest existing variant for "interrupted"; the cell's + /// surrounding context (turn-status banner) tells the user it was a + /// cancellation rather than a tool error. + pub fn mark_in_progress_as_interrupted(&mut self) { + for cell in &mut self.entries { + mark_running_as_interrupted(cell); + } + self.bump_revision(); + } +} + +fn mark_running_as_interrupted(cell: &mut HistoryCell) { + let HistoryCell::Tool(tool_cell) = cell else { + return; + }; + match tool_cell { + ToolCell::Exec(exec) if exec.status == ToolStatus::Running => { + exec.status = ToolStatus::Failed; + } + ToolCell::Exploring(explore) => { + for entry in &mut explore.entries { + if entry.status == ToolStatus::Running { + entry.status = ToolStatus::Failed; + } + } + } + ToolCell::PlanUpdate(plan) if plan.status == ToolStatus::Running => { + plan.status = ToolStatus::Failed; + } + ToolCell::PatchSummary(patch) if patch.status == ToolStatus::Running => { + patch.status = ToolStatus::Failed; + } + ToolCell::Review(review) if review.status == ToolStatus::Running => { + review.status = ToolStatus::Failed; + } + ToolCell::Mcp(mcp) if mcp.status == ToolStatus::Running => { + mcp.status = ToolStatus::Failed; + } + ToolCell::WebSearch(search) if search.status == ToolStatus::Running => { + search.status = ToolStatus::Failed; + } + ToolCell::Generic(generic) if generic.status == ToolStatus::Running => { + generic.status = ToolStatus::Failed; + } + _ => {} + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tui::history::{ + ExecCell, ExecSource, ExploringCell, ExploringEntry, GenericToolCell, + }; + use std::time::Instant; + + fn exec_cell(command: &str) -> HistoryCell { + HistoryCell::Tool(ToolCell::Exec(ExecCell { + command: command.to_string(), + status: ToolStatus::Running, + output: None, + started_at: Some(Instant::now()), + duration_ms: None, + source: ExecSource::Assistant, + interaction: None, + })) + } + + fn exploring_cell_with(label: &str) -> HistoryCell { + HistoryCell::Tool(ToolCell::Exploring(ExploringCell { + entries: vec![ExploringEntry { + label: label.to_string(), + status: ToolStatus::Running, + }], + })) + } + + fn generic_cell(name: &str) -> HistoryCell { + HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: name.to_string(), + status: ToolStatus::Running, + input_summary: None, + output: None, + })) + } + + #[test] + fn push_tool_records_entry_and_revision_advances() { + let mut cell = ActiveCell::new(); + let r0 = cell.revision(); + let idx = cell.push_tool("t1", exec_cell("ls")); + assert_eq!(idx, 0); + assert_eq!(cell.entry_count(), 1); + assert!(cell.revision() != r0); + assert_eq!(cell.entry_index_for_tool("t1"), Some(0)); + } + + #[test] + fn parallel_exploring_starts_share_one_entry() { + let mut cell = ActiveCell::new(); + let idx_a = cell.push_tool("a", exploring_cell_with("Read foo.rs")); + let idx_b = cell.push_tool("b", exploring_cell_with("Read bar.rs")); + assert_eq!( + idx_a, idx_b, + "both exploring starts should land in same entry" + ); + assert_eq!(cell.entry_count(), 1); + let HistoryCell::Tool(ToolCell::Exploring(explore)) = &cell.entries()[0] else { + panic!("expected exploring cell") + }; + assert_eq!(explore.entries.len(), 2); + } + + #[test] + fn drain_resets_state_and_returns_in_order() { + let mut cell = ActiveCell::new(); + cell.push_tool("a", exec_cell("ls")); + cell.push_tool("b", generic_cell("foo")); + let drained = cell.drain(); + assert_eq!(drained.len(), 2); + assert!(cell.is_empty()); + assert_eq!(cell.entry_index_for_tool("a"), None); + } + + #[test] + fn interrupt_marks_running_entries_failed() { + let mut cell = ActiveCell::new(); + cell.push_tool("a", exec_cell("ls")); + cell.mark_in_progress_as_interrupted(); + let HistoryCell::Tool(ToolCell::Exec(exec)) = &cell.entries()[0] else { + panic!("expected exec") + }; + assert_eq!(exec.status, ToolStatus::Failed); + } +} diff --git a/crates/tui/src/tui/app.rs b/crates/tui/src/tui/app.rs index 1e12ad94..d1fa5309 100644 --- a/crates/tui/src/tui/app.rs +++ b/crates/tui/src/tui/app.rs @@ -21,6 +21,7 @@ use crate::settings::Settings; use crate::tools::plan::{SharedPlanState, new_shared_plan_state}; use crate::tools::subagent::SubAgentResult; use crate::tools::todo::{SharedTodoList, new_shared_todo_list}; +use crate::tui::active_cell::ActiveCell; use crate::tui::approval::ApprovalMode; use crate::tui::clipboard::{ClipboardContent, ClipboardHandler}; use crate::tui::history::{HistoryCell, TranscriptRenderOptions}; @@ -414,6 +415,10 @@ pub struct App { pub slash_menu_selected: usize, /// Temporary hide flag for slash menu until next input edit. pub slash_menu_hidden: bool, + /// `@`-mention completion popup selection index in composer. + pub mention_menu_selected: usize, + /// Temporary hide flag for the @-mention popup until next input edit. + pub mention_menu_hidden: bool, #[allow(dead_code)] pub compact_threshold: usize, pub max_input_history: usize, @@ -468,13 +473,31 @@ pub struct App { pub session_cost: f64, /// Active skill to apply to next user message pub active_skill: Option, - /// Tool call cells by tool id + /// Tool call cells by tool id (for cells already finalized in `history`). + /// While a tool call is in flight inside `active_cell`, it is tracked by + /// `active_tool_entries` instead and migrated here at flush time. pub tool_cells: HashMap, /// Full tool input/output keyed by history cell index. pub tool_details_by_cell: HashMap, - /// Active exploring cell index + /// In-flight tool/exec group for the current turn. Mutated in place as + /// parallel tool calls start and complete; flushed into `history` on + /// `TurnComplete`. + pub active_cell: Option, + /// Revision counter for `active_cell`. Combined with `active_cell.revision` + /// when feeding the transcript cache so cached lines for the synthetic + /// active-cell row are invalidated on every mutation. + pub active_cell_revision: u64, + /// Pending tool details for entries that live inside `active_cell`. + /// Keyed by tool id rather than cell index because the active cell's + /// virtual index can shift (orphan completions push real cells in + /// between). Migrated into `tool_details_by_cell` on flush. + pub active_tool_details: HashMap, + /// Active exploring cell entry index (within `active_cell.entries`). + /// `None` once the active cell flushes or no exploring entry exists. pub exploring_cell: Option, - /// Mapping of exploring tool ids to (cell index, entry index) + /// Mapping of exploring tool ids to `(entry index in active_cell, entry + /// within ExploringCell)`. Used to update individual exploring entries + /// when their tools complete. pub exploring_entries: HashMap, /// Tool calls that should be ignored by the UI pub ignored_tool_calls: HashSet, @@ -726,6 +749,8 @@ impl App { sidebar_width_percent, sidebar_focus, slash_menu_selected: 0, + mention_menu_selected: 0, + mention_menu_hidden: false, slash_menu_hidden: false, compact_threshold, max_input_history, @@ -772,6 +797,9 @@ impl App { active_skill: None, tool_cells: HashMap::new(), tool_details_by_cell: HashMap::new(), + active_cell: None, + active_cell_revision: 0, + active_tool_details: HashMap::new(), exploring_cell: None, exploring_entries: HashMap::new(), ignored_tool_calls: HashSet::new(), @@ -949,6 +977,144 @@ impl App { self.needs_redraw = true; } + /// Bump the active-cell revision counter and request a redraw. + /// + /// Use this whenever an entry inside `active_cell` is mutated. The + /// transcript cache combines this counter with `history_version` to + /// produce a per-cell revision so the synthetic active-cell row can be + /// re-rendered without invalidating committed history cells. + pub fn bump_active_cell_revision(&mut self) { + self.active_cell_revision = self.active_cell_revision.wrapping_add(1); + if let Some(active) = self.active_cell.as_mut() { + active.bump_revision(); + } + self.history_version = self.history_version.wrapping_add(1); + self.needs_redraw = true; + } + + /// Total number of cells in the *virtual* transcript: `history.len()` + /// plus active cell entries (if any). + #[must_use] + #[allow(dead_code)] // Reserved for renderers that need a unified cell count. + pub fn virtual_cell_count(&self) -> usize { + self.history.len() + self.active_cell.as_ref().map_or(0, ActiveCell::entry_count) + } + + /// The next cell index a freshly-pushed entry would occupy in the virtual + /// transcript. Used by `register_tool_cell`-style callsites that record + /// cell-index metadata before the active cell flushes to history. + #[must_use] + #[allow(dead_code)] // Reserved for the eventual merged push helper. + pub fn next_virtual_cell_index(&self) -> usize { + self.virtual_cell_count() + } + + /// Resolve a virtual cell index to either a committed history cell or an + /// active-cell entry. Used by the pager / details lookup code so it can + /// transparently address still-in-flight cells. + #[must_use] + #[allow(dead_code)] // Used by the upcoming pager rewrite (read-only resolver). + pub fn cell_at_virtual_index(&self, index: usize) -> Option<&HistoryCell> { + if index < self.history.len() { + self.history.get(index) + } else { + let entry_idx = index - self.history.len(); + self.active_cell + .as_ref() + .and_then(|active| active.entries().get(entry_idx)) + } + } + + /// Mutable variant of [`Self::cell_at_virtual_index`]. Bumps the + /// appropriate revision counter (active-cell revision when targeting an + /// in-flight entry, history version otherwise). + pub fn cell_at_virtual_index_mut(&mut self, index: usize) -> Option<&mut HistoryCell> { + if index < self.history.len() { + self.history_version = self.history_version.wrapping_add(1); + self.history.get_mut(index) + } else { + let entry_idx = index - self.history.len(); + self.active_cell_revision = self.active_cell_revision.wrapping_add(1); + self.history_version = self.history_version.wrapping_add(1); + self.active_cell + .as_mut() + .and_then(|active| active.entry_mut(entry_idx)) + } + } + + /// Drain the active cell into history. Companion maps that reference + /// active-cell entries by virtual index (`tool_cells`, + /// `tool_details_by_cell`) are rewritten to point at the new history + /// indices. Idempotent — calling this when there is no active cell is a + /// no-op. + /// + /// Caller is responsible for first marking in-progress entries with the + /// terminal status they want (e.g. via + /// [`ActiveCell::mark_in_progress_as_interrupted`]). + pub fn flush_active_cell(&mut self) { + let Some(mut active) = self.active_cell.take() else { + return; + }; + if active.is_empty() { + // Reset auxiliary state regardless so a future tool can start a + // fresh active cell. + self.exploring_cell = None; + self.exploring_entries.clear(); + self.active_tool_details.clear(); + self.bump_active_cell_revision(); + return; + } + + let drained = active.drain(); + let base_index = self.history.len(); + + // Rewrite per-tool indices that targeted entries inside the active + // group: their new home is `base_index + entry_offset`. + let mut details = std::mem::take(&mut self.active_tool_details); + for (tool_id, detail) in details.drain() { + // Try to recover the entry offset from `tool_cells`-style maps. + // Tool ids registered for active-cell entries live in + // `tool_cells` with `index = base_index_at_register_time + + // entry_offset`. After rewriting once, those indices are correct. + self.tool_details_by_cell + .entry(self.tool_cells.get(&tool_id).copied().unwrap_or(base_index)) + .or_insert(detail); + } + + // tool_cells already contains the virtual index. After the drain, + // history.len() == base_index + drained.len(), so any virtual index + // in [base_index, base_index + drained.len()) is now a real history + // index. No rewrite needed. + self.exploring_cell = None; + self.exploring_entries.clear(); + + for cell in drained { + self.history.push(cell); + } + self.history_version = self.history_version.wrapping_add(1); + self.needs_redraw = true; + let selection_has_range = self + .transcript_selection + .ordered_endpoints() + .is_some_and(|(start, end)| start != end); + if self.transcript_scroll.is_at_tail() + && !self.transcript_selection.dragging + && !selection_has_range + && !self.user_scrolled_during_stream + { + self.scroll_to_bottom(); + } + } + + /// Mark every still-running entry in the active cell as interrupted, then + /// flush. Convenience helper for cancellation paths. + pub fn finalize_active_cell_as_interrupted(&mut self) { + if let Some(active) = self.active_cell.as_mut() { + active.mark_in_progress_as_interrupted(); + } + self.flush_active_cell(); + } + pub fn push_status_toast( &mut self, text: impl Into, @@ -1139,6 +1305,8 @@ impl App { self.input.insert_str(byte_index, text); self.cursor_position = cursor + char_count(text); self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; } @@ -1273,6 +1441,8 @@ impl App { self.input.insert(byte_index, c); self.cursor_position = cursor + 1; self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; } @@ -1285,6 +1455,8 @@ impl App { if removed { self.cursor_position = target; self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; } } @@ -1299,6 +1471,8 @@ impl App { self.cursor_position = char_count(&self.input); } self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; } @@ -1341,6 +1515,8 @@ impl App { // Cursor stays at the same character index (start of removed range). self.cursor_position = cursor; self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; true } @@ -1358,6 +1534,8 @@ impl App { self.input.insert_str(byte_index, &text); self.cursor_position = cursor + char_count(&text); self.slash_menu_hidden = false; + self.mention_menu_hidden = false; + self.mention_menu_selected = 0; self.needs_redraw = true; true } diff --git a/crates/tui/src/tui/mod.rs b/crates/tui/src/tui/mod.rs index ef8b5199..ed41feb0 100644 --- a/crates/tui/src/tui/mod.rs +++ b/crates/tui/src/tui/mod.rs @@ -2,6 +2,7 @@ // === Submodules === +pub mod active_cell; pub mod app; pub mod approval; pub mod clipboard; diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index ff73c1c5..24b83c96 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -67,6 +67,7 @@ use crate::tui::session_picker::SessionPickerView; use crate::tui::ui_text::{history_cell_to_text, line_to_plain, slice_text, text_display_width}; use crate::tui::user_input::UserInputView; +use super::active_cell::ActiveCell; use super::app::{ App, AppAction, AppMode, OnboardingState, QueuedMessage, SidebarFocus, StatusToastLevel, TaskPanelEntry, ToolDetailRecord, TuiOptions, @@ -75,9 +76,9 @@ use super::approval::{ ApprovalMode, ApprovalRequest, ApprovalView, ElevationRequest, ElevationView, ReviewDecision, }; use super::history::{ - DiffPreviewCell, ExecCell, ExecSource, ExploringCell, ExploringEntry, GenericToolCell, - HistoryCell, McpToolCell, PatchSummaryCell, PlanStep, PlanUpdateCell, ReviewCell, ToolCell, - ToolStatus, ViewImageCell, WebSearchCell, history_cells_from_message, summarize_mcp_output, + DiffPreviewCell, ExecCell, ExecSource, ExploringEntry, GenericToolCell, HistoryCell, + McpToolCell, PatchSummaryCell, PlanStep, PlanUpdateCell, ReviewCell, ToolCell, ToolStatus, + ViewImageCell, WebSearchCell, history_cells_from_message, summarize_mcp_output, summarize_tool_args, summarize_tool_output, }; use super::views::{ConfigView, HelpView, ModalKind, ViewEvent}; @@ -345,6 +346,12 @@ async fn run_event_loop( received_engine_event = true; match event { EngineEvent::MessageStarted { .. } => { + // Assistant text starting after parallel tool work + // means the tool group is done. Flush the active + // cell first so the message lands BELOW the + // committed tool group (Codex pattern: streamed + // assistant content always flows after work). + app.flush_active_cell(); current_streaming_text.clear(); app.streaming_state.reset(); app.streaming_state.start_text(0, None); @@ -355,6 +362,12 @@ async fn run_event_loop( if sanitized.is_empty() { continue; } + // First delta of a fresh stream has no streaming + // cell yet; flush active so the tool group settles + // before the assistant prose appears below it. + if app.streaming_message_index.is_none() { + app.flush_active_cell(); + } current_streaming_text.push_str(&sanitized); let index = ensure_streaming_history_cell(app, StreamingCellKind::Assistant); @@ -416,6 +429,9 @@ async fn run_event_loop( } } EngineEvent::ThinkingStarted { .. } => { + // A fresh thinking block after parallel tools means + // that batch is done; settle it into history. + app.flush_active_cell(); app.reasoning_buffer.clear(); app.reasoning_header = None; app.thinking_started_at = Some(Instant::now()); @@ -518,6 +534,19 @@ async fn run_event_loop( status, error, } => { + // Finalize any in-flight tool group. Cancellation + // marks still-running entries as Failed so the user + // sees they were interrupted rather than the spinner + // hanging forever. + if matches!( + status, + crate::core::events::TurnOutcomeStatus::Interrupted + | crate::core::events::TurnOutcomeStatus::Failed + ) { + app.finalize_active_cell_as_interrupted(); + } else { + app.flush_active_cell(); + } app.is_loading = false; app.offline_mode = false; app.streaming_state.reset(); @@ -3246,6 +3275,10 @@ fn apply_loaded_session(app: &mut App, session: &SavedSession) { app.history.clear(); app.tool_cells.clear(); app.tool_details_by_cell.clear(); + app.active_cell = None; + app.active_tool_details.clear(); + app.active_cell_revision = app.active_cell_revision.wrapping_add(1); + app.exploring_cell = None; app.exploring_entries.clear(); app.ignored_tool_calls.clear(); app.pending_tool_uses.clear(); @@ -4461,34 +4494,44 @@ fn format_task_detail(task: &TaskRecord) -> String { #[allow(clippy::too_many_lines)] fn handle_tool_call_started(app: &mut App, id: &str, name: &str, input: &serde_json::Value) { let id = id.to_string(); + + // All in-flight tool work for the current turn lives in `app.active_cell` + // until the turn completes. This mirrors Codex's contract: ONE active cell + // mutates in place; finalized history isn't touched until flush. This + // keeps the transcript stable while parallel completions arrive in any + // order. + if app.active_cell.is_none() { + app.active_cell = Some(ActiveCell::new()); + } + if is_exploring_tool(name) { let label = exploring_label(name, input); - let cell_index = if let Some(idx) = app.exploring_cell { - idx - } else { - app.add_message(HistoryCell::Tool(ToolCell::Exploring(ExploringCell { - entries: Vec::new(), - }))); - let idx = app.history.len().saturating_sub(1); - app.exploring_cell = Some(idx); - idx - }; - - if let Some(HistoryCell::Tool(ToolCell::Exploring(cell))) = app.history.get_mut(cell_index) - { - let entry_index = cell.insert_entry(ExploringEntry { - label, - status: ToolStatus::Running, - }); - app.mark_history_updated(); - app.exploring_entries - .insert(id.clone(), (cell_index, entry_index)); - } - register_tool_cell(app, &id, name, input, cell_index); + // ensure_exploring + append_to_exploring keeps all parallel exploring + // starts in a single ExploringCell entry. + let active = app.active_cell.as_mut().expect("active_cell just ensured"); + let entry_idx = active.ensure_exploring(); + let inner = active + .append_to_exploring( + id.clone(), + ExploringEntry { + label, + status: ToolStatus::Running, + }, + ) + .map_or(0, |(_, inner)| inner); + app.exploring_cell = Some(entry_idx); + let virtual_index = app.history.len() + entry_idx; + app.exploring_entries + .insert(id.clone(), (virtual_index, inner)); + register_tool_cell(app, &id, name, input, virtual_index); + app.mark_history_updated(); return; } - app.exploring_cell = None; + // Non-exploring tool: each is its own entry inside the active cell. We + // intentionally do NOT clear `exploring_cell` here — the active cell can + // hold both an exploring aggregate AND independent tool entries + // simultaneously, which is exactly the case CX#7 fixes. if is_exec_tool(name) { let command = exec_command_from_input(input).unwrap_or_else(|| "".to_string()); @@ -4511,17 +4554,21 @@ fn handle_tool_call_started(app: &mut App, id: &str, name: &str, input: &serde_j app.last_exec_wait_command = Some(command.clone()); } - app.add_message(HistoryCell::Tool(ToolCell::Exec(ExecCell { - command, - status: ToolStatus::Running, - output: None, - started_at: Some(Instant::now()), - duration_ms: None, - source, - interaction: Some(summary.clone()), - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::Exec(ExecCell { + command, + status: ToolStatus::Running, + output: None, + started_at: Some(Instant::now()), + duration_ms: None, + source, + interaction: Some(summary.clone()), + })), + ); return; } @@ -4538,69 +4585,87 @@ fn handle_tool_call_started(app: &mut App, id: &str, name: &str, input: &serde_j app.last_exec_wait_command = Some(command.clone()); } - app.add_message(HistoryCell::Tool(ToolCell::Exec(ExecCell { - command, - status: ToolStatus::Running, - output: None, - started_at: Some(Instant::now()), - duration_ms: None, - source, - interaction: None, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::Exec(ExecCell { + command, + status: ToolStatus::Running, + output: None, + started_at: Some(Instant::now()), + duration_ms: None, + source, + interaction: None, + })), + ); return; } if name == "update_plan" { let (explanation, steps) = parse_plan_input(input); - app.add_message(HistoryCell::Tool(ToolCell::PlanUpdate(PlanUpdateCell { - explanation, - steps, - status: ToolStatus::Running, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::PlanUpdate(PlanUpdateCell { + explanation, + steps, + status: ToolStatus::Running, + })), + ); return; } if name == "apply_patch" { let (path, summary) = parse_patch_summary(input); - app.add_message(HistoryCell::Tool(ToolCell::PatchSummary( - PatchSummaryCell { + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::PatchSummary(PatchSummaryCell { path, summary, status: ToolStatus::Running, error: None, - }, - ))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + })), + ); return; } if name == "review" { let target = review_target_label(input); - app.add_message(HistoryCell::Tool(ToolCell::Review(ReviewCell { - target, - status: ToolStatus::Running, - output: None, - error: None, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::Review(ReviewCell { + target, + status: ToolStatus::Running, + output: None, + error: None, + })), + ); return; } if is_mcp_tool(name) { - app.add_message(HistoryCell::Tool(ToolCell::Mcp(McpToolCell { - tool: name.to_string(), - status: ToolStatus::Running, - content: None, - is_image: false, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::Mcp(McpToolCell { + tool: name.to_string(), + status: ToolStatus::Running, + content: None, + is_image: false, + })), + ); return; } @@ -4611,36 +4676,65 @@ fn handle_tool_call_started(app: &mut App, id: &str, name: &str, input: &serde_j .strip_prefix(&app.workspace) .unwrap_or(&raw_path) .to_path_buf(); - app.add_message(HistoryCell::Tool(ToolCell::ViewImage(ViewImageCell { - path: display_path, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::ViewImage(ViewImageCell { path: display_path })), + ); } return; } if is_web_search_tool(name) { let query = web_search_query(input); - app.add_message(HistoryCell::Tool(ToolCell::WebSearch(WebSearchCell { - query, - status: ToolStatus::Running, - summary: None, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::WebSearch(WebSearchCell { + query, + status: ToolStatus::Running, + summary: None, + })), + ); return; } let input_summary = summarize_tool_args(input); - app.add_message(HistoryCell::Tool(ToolCell::Generic(GenericToolCell { - name: name.to_string(), - status: ToolStatus::Running, - input_summary, - output: None, - }))); - let cell_index = app.history.len().saturating_sub(1); - register_tool_cell(app, &id, name, input, cell_index); + push_active_tool_cell( + app, + &id, + name, + input, + HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: name.to_string(), + status: ToolStatus::Running, + input_summary, + output: None, + })), + ); +} + +/// Push a tool cell as a new entry in `active_cell`, register the tool id, +/// and write a stub detail record so the pager / Ctrl+O can find it. +fn push_active_tool_cell( + app: &mut App, + tool_id: &str, + tool_name: &str, + input: &serde_json::Value, + cell: HistoryCell, +) { + if app.active_cell.is_none() { + app.active_cell = Some(ActiveCell::new()); + } + let active = app.active_cell.as_mut().expect("active_cell just ensured"); + let entry_idx = active.push_tool(tool_id.to_string(), cell); + let virtual_index = app.history.len() + entry_idx; + register_tool_cell(app, tool_id, tool_name, input, virtual_index); + app.mark_history_updated(); } fn register_tool_cell( @@ -4651,27 +4745,43 @@ fn register_tool_cell( cell_index: usize, ) { app.tool_cells.insert(tool_id.to_string(), cell_index); - app.tool_details_by_cell.insert( - cell_index, - ToolDetailRecord { - tool_id: tool_id.to_string(), - tool_name: tool_name.to_string(), - input: input.clone(), - output: None, - }, - ); + let record = ToolDetailRecord { + tool_id: tool_id.to_string(), + tool_name: tool_name.to_string(), + input: input.clone(), + output: None, + }; + if cell_index < app.history.len() { + app.tool_details_by_cell.insert(cell_index, record); + } else { + // Active-cell entry: keep the detail record in `active_tool_details` + // until the active cell flushes. `flush_active_cell` migrates these + // records into `tool_details_by_cell` keyed by the eventual real + // cell index. + app.active_tool_details.insert(tool_id.to_string(), record); + } } fn store_tool_detail_output( app: &mut App, + tool_id: &str, cell_index: usize, result: &Result, ) { - if let Some(detail) = app.tool_details_by_cell.get_mut(&cell_index) { - detail.output = Some(match result { - Ok(tool_result) => tool_result.content.clone(), - Err(err) => err.to_string(), - }); + let payload = Some(match result { + Ok(tool_result) => tool_result.content.clone(), + Err(err) => err.to_string(), + }); + if cell_index < app.history.len() + && let Some(detail) = app.tool_details_by_cell.get_mut(&cell_index) + { + detail.output = payload.clone(); + } + // Also write to the active table while the entry might still live there; + // some callsites pre-rewrite cell_index but the active_tool_details map is + // the canonical source for in-flight outputs. + if let Some(detail) = app.active_tool_details.get_mut(tool_id) { + detail.output = payload; } } @@ -4679,18 +4789,20 @@ fn store_tool_detail_output( fn handle_tool_call_complete( app: &mut App, id: &str, - _name: &str, + name: &str, result: &Result, ) { if app.ignored_tool_calls.remove(id) { return; } + // Exploring entries land in the per-tool map regardless of whether they + // live in the active cell or in finalized history; the path is the same. if let Some((cell_index, entry_index)) = app.exploring_entries.remove(id) { app.tool_cells.remove(id); - store_tool_detail_output(app, cell_index, result); - - if let Some(HistoryCell::Tool(ToolCell::Exploring(cell))) = app.history.get_mut(cell_index) + store_tool_detail_output(app, id, cell_index, result); + if let Some(HistoryCell::Tool(ToolCell::Exploring(cell))) = + app.cell_at_virtual_index_mut(cell_index) && let Some(entry) = cell.entries.get_mut(entry_index) { entry.status = match result.as_ref() { @@ -4698,15 +4810,31 @@ fn handle_tool_call_complete( Ok(_) | Err(_) => ToolStatus::Failed, }; app.mark_history_updated(); + // Mutating the in-flight exploring cell needs an active-cell + // revision bump so the transcript cache invalidates the synthetic + // tail row. + if cell_index >= app.history.len() { + app.active_cell_revision = app.active_cell_revision.wrapping_add(1); + if let Some(active) = app.active_cell.as_mut() { + active.bump_revision(); + } + } } return; } + // Look up the cell by tool id. If the id isn't registered, that's an + // orphan completion (race condition where the started event was lost or + // a tool result arrived after the active cell was already flushed). Build + // a finalized standalone cell from the result so the user can still see + // the output, but DO NOT touch the active cell. let Some(cell_index) = app.tool_cells.remove(id) else { + push_orphan_tool_completion(app, id, name, result); return; }; - store_tool_detail_output(app, cell_index, result); + store_tool_detail_output(app, id, cell_index, result); + let in_active = cell_index >= app.history.len(); let status = match result.as_ref() { Ok(tool_result) => match tool_result.metadata.as_ref() { @@ -4729,7 +4857,7 @@ fn handle_tool_call_complete( Err(_) => ToolStatus::Failed, }; - if let Some(cell) = app.history.get_mut(cell_index) { + if let Some(cell) = app.cell_at_virtual_index_mut(cell_index) { match cell { HistoryCell::Tool(ToolCell::Exec(exec)) => { exec.status = status; @@ -4832,6 +4960,95 @@ fn handle_tool_call_complete( _ => {} } } + + // If the mutated cell lived inside the active group, bump the active-cell + // revision so the transcript cache re-renders the synthetic tail row. + if in_active { + app.active_cell_revision = app.active_cell_revision.wrapping_add(1); + if let Some(active) = app.active_cell.as_mut() { + active.bump_revision(); + } + } +} + +/// Build a finalized standalone history cell for a tool completion whose +/// start was never registered (orphan). This preserves the contract that +/// every tool result is visible somewhere; the alternative (silently +/// dropping it) hides errors and breaks debuggability. +/// +/// Choice of cell type: we use `GenericToolCell` because we have no input +/// payload to reconstruct a more specific cell. The pager remains usable — +/// `tool_details_by_cell` is populated with the result text. +/// +/// ## Index drift +/// +/// If an active cell is in flight when the orphan arrives, pushing the +/// orphan into `app.history` shifts every active-cell virtual index forward +/// by 1. We must rewrite `tool_cells` / `exploring_entries` accordingly so +/// later completion lookups still find the right entries. +fn push_orphan_tool_completion( + app: &mut App, + tool_id: &str, + name: &str, + result: &Result, +) { + let status = match result.as_ref() { + Ok(tool_result) => { + if tool_result.success { + ToolStatus::Success + } else { + ToolStatus::Failed + } + } + Err(_) => ToolStatus::Failed, + }; + let output = match result.as_ref() { + Ok(tool_result) => Some(summarize_tool_output(&tool_result.content)), + Err(err) => Some(err.to_string()), + }; + let history_threshold_before_push = app.history.len(); + let active_in_flight = app.active_cell.is_some(); + app.add_message(HistoryCell::Tool(ToolCell::Generic(GenericToolCell { + name: name.to_string(), + status, + input_summary: None, + output, + }))); + let cell_index = app.history.len().saturating_sub(1); + app.tool_details_by_cell.insert( + cell_index, + ToolDetailRecord { + tool_id: tool_id.to_string(), + tool_name: name.to_string(), + input: serde_json::Value::Null, + output: match result.as_ref() { + Ok(tool_result) => Some(tool_result.content.clone()), + Err(err) => Some(err.to_string()), + }, + }, + ); + + // Shift active-cell virtual indices forward by 1 to absorb the new + // history cell. Without this, the next completion would address the + // wrong entry. + if active_in_flight { + let threshold = history_threshold_before_push; + for idx in app.tool_cells.values_mut() { + if *idx >= threshold { + *idx = idx.wrapping_add(1); + } + } + for (cell_idx, _) in app.exploring_entries.values_mut() { + if *cell_idx >= threshold { + *cell_idx = cell_idx.wrapping_add(1); + } + } + if let Some(idx) = app.exploring_cell.as_mut() + && *idx >= threshold + { + *idx = idx.wrapping_add(1); + } + } } fn is_exploring_tool(name: &str) -> bool { diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index 9540a9ec..e2b60ecf 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -4,7 +4,7 @@ use crate::tui::file_mention::{ find_file_mention_completions, partial_file_mention_at_cursor, try_autocomplete_file_mention, user_request_with_file_mentions, }; -use crate::tui::history::{GenericToolCell, ToolCell, ToolStatus}; +use crate::tui::history::{GenericToolCell, HistoryCell, ToolCell, ToolStatus}; use crate::tui::views::{ModalView, ViewAction}; use std::path::PathBuf; use std::process::Command; @@ -1056,3 +1056,303 @@ fn try_autocomplete_file_mention_returns_false_outside_mention() { app.cursor_position = app.input.chars().count(); assert!(!try_autocomplete_file_mention(&mut app)); } + +// === CX#7 — single active cell mutated in place for parallel tool calls === + +/// Build a minimal successful ToolResult with the given content. +fn ok_result( + content: &str, +) -> Result { + Ok(crate::tools::spec::ToolResult::success(content)) +} + +#[test] +fn parallel_exploring_tool_starts_share_one_active_entry() { + // Three exploring tools start in any order; they must collapse into one + // entry inside the active cell rather than three separate cells. This is + // the central CX#7 contract for the most common parallel case. + let mut app = create_test_app(); + + handle_tool_call_started( + &mut app, + "t-a", + "read_file", + &serde_json::json!({"path": "alpha.rs"}), + ); + handle_tool_call_started( + &mut app, + "t-b", + "read_file", + &serde_json::json!({"path": "beta.rs"}), + ); + handle_tool_call_started( + &mut app, + "t-c", + "grep_files", + &serde_json::json!({"pattern": "TODO"}), + ); + + // History must remain empty: nothing flushes until the turn ends. + assert_eq!(app.history.len(), 0, "no history cells written mid-turn"); + let active = app.active_cell.as_ref().expect("active cell created"); + assert_eq!( + active.entry_count(), + 1, + "all exploring starts share one entry" + ); + let HistoryCell::Tool(ToolCell::Exploring(explore)) = &active.entries()[0] else { + panic!("expected exploring cell") + }; + assert_eq!(explore.entries.len(), 3); + for entry in &explore.entries { + assert_eq!(entry.status, ToolStatus::Running); + } +} + +#[test] +fn out_of_order_completes_finalize_one_history_cell_per_turn() { + // Three parallel tools complete in reverse order; we then signal turn + // complete and assert exactly one tool history cell exists (the + // finalized active group). This proves the active cell didn't bounce + // mid-turn and that the flush path correctly migrates entries. + let mut app = create_test_app(); + + handle_tool_call_started( + &mut app, + "t-1", + "read_file", + &serde_json::json!({"path": "a.rs"}), + ); + handle_tool_call_started( + &mut app, + "t-2", + "read_file", + &serde_json::json!({"path": "b.rs"}), + ); + handle_tool_call_started( + &mut app, + "t-3", + "grep_files", + &serde_json::json!({"pattern": "x"}), + ); + + // Out-of-order completion: t-3, then t-1, then t-2. + handle_tool_call_complete(&mut app, "t-3", "grep_files", &ok_result("two hits")); + handle_tool_call_complete(&mut app, "t-1", "read_file", &ok_result("contents A")); + handle_tool_call_complete(&mut app, "t-2", "read_file", &ok_result("contents B")); + + // Still nothing in history: the active cell holds everything. + assert_eq!(app.history.len(), 0); + let active = app.active_cell.as_ref().expect("active cell still present"); + let HistoryCell::Tool(ToolCell::Exploring(explore)) = &active.entries()[0] else { + panic!("expected exploring cell") + }; + assert!( + explore + .entries + .iter() + .all(|e| e.status == ToolStatus::Success), + "all exploring entries should be Success after their tools complete" + ); + + // Flush via the explicit helper (mirrors what TurnComplete does). + app.flush_active_cell(); + + assert!(app.active_cell.is_none(), "active cell cleared after flush"); + // The flushed group is exactly one history cell — the merged exploring + // aggregate. This is the heart of CX#7: parallel work renders as ONE + // finalized cell, regardless of completion order. + let tool_cells = app + .history + .iter() + .filter(|c| matches!(c, HistoryCell::Tool(_))) + .count(); + assert_eq!( + tool_cells, 1, + "exactly one tool history cell after parallel turn" + ); +} + +#[test] +fn mixed_parallel_tools_render_in_single_active_cell() { + // Tools of different shapes — exploring + exec + generic — all in flight + // at once. The active cell must hold them all without bouncing. + let mut app = create_test_app(); + + handle_tool_call_started( + &mut app, + "ex-1", + "read_file", + &serde_json::json!({"path": "x.rs"}), + ); + handle_tool_call_started( + &mut app, + "shell-1", + "exec_shell", + &serde_json::json!({"command": "ls"}), + ); + handle_tool_call_started( + &mut app, + "gen-1", + "todo_write", + &serde_json::json!({"items": []}), + ); + + assert_eq!(app.history.len(), 0); + let active = app.active_cell.as_ref().expect("active cell present"); + // 3 entries: exploring aggregate (1) + exec + generic. + assert_eq!(active.entry_count(), 3); + + handle_tool_call_complete(&mut app, "shell-1", "exec_shell", &ok_result("ok")); + handle_tool_call_complete(&mut app, "gen-1", "todo_write", &ok_result("done")); + handle_tool_call_complete(&mut app, "ex-1", "read_file", &ok_result("file body")); + + // After all complete, still in active until flush. + assert_eq!(app.history.len(), 0); + app.flush_active_cell(); + let tool_cells: Vec<_> = app + .history + .iter() + .filter(|c| matches!(c, HistoryCell::Tool(_))) + .collect(); + assert_eq!( + tool_cells.len(), + 3, + "three distinct tool shapes finalize as three cells in stable insertion order" + ); +} + +#[test] +fn orphan_tool_complete_with_unknown_id_pushes_separate_cell() { + // A ToolCallComplete with no matching ToolCallStarted — the orphan path. + // Per the design we render it as a finalized standalone cell so the user + // still sees the output, but we must NOT flush or contaminate any active + // cell that's currently in flight. + let mut app = create_test_app(); + + handle_tool_call_started( + &mut app, + "live-1", + "read_file", + &serde_json::json!({"path": "live.rs"}), + ); + + // Orphan completion arrives. + handle_tool_call_complete(&mut app, "ghost-id", "mystery_tool", &ok_result("oops")); + + // Active cell is intact. + let active = app + .active_cell + .as_ref() + .expect("active cell preserved after orphan"); + assert_eq!(active.entry_count(), 1); + + // The orphan rendered as a separate finalized cell pushed to history. + assert_eq!(app.history.len(), 1, "orphan added one finalized cell"); + let HistoryCell::Tool(ToolCell::Generic(generic)) = &app.history[0] else { + panic!("orphan should render as a Generic tool cell") + }; + assert_eq!(generic.name, "mystery_tool"); + assert_eq!(generic.status, ToolStatus::Success); +} + +#[test] +fn turn_complete_flushes_active_cell_into_history() { + // The full path through the public flush helper. Verifies that a + // mid-turn snapshot (exec running, exploring complete) becomes a stable + // history slice on flush. + let mut app = create_test_app(); + handle_tool_call_started( + &mut app, + "ex-1", + "read_file", + &serde_json::json!({"path": "a.rs"}), + ); + handle_tool_call_complete(&mut app, "ex-1", "read_file", &ok_result("body")); + handle_tool_call_started( + &mut app, + "shell-1", + "exec_shell", + &serde_json::json!({"command": "ls"}), + ); + // Don't complete shell-1 — simulate cancellation mid-shell. + app.finalize_active_cell_as_interrupted(); + + assert!(app.active_cell.is_none(), "active cell cleared on flush"); + let exec_cells: Vec<_> = app + .history + .iter() + .filter_map(|c| match c { + HistoryCell::Tool(ToolCell::Exec(exec)) => Some(exec), + _ => None, + }) + .collect(); + assert_eq!(exec_cells.len(), 1); + assert_eq!( + exec_cells[0].status, + ToolStatus::Failed, + "interrupted shell entry marked Failed (closest available terminal status)" + ); +} + +#[test] +fn orphan_during_active_keeps_subsequent_completion_routed_correctly() { + // Regression cover for the index-shift trap: when an orphan arrives + // mid-active, it pushes a real history cell that bumps virtual indices + // by one. A subsequent legitimate completion must still find its entry. + let mut app = create_test_app(); + handle_tool_call_started( + &mut app, + "live", + "exec_shell", + &serde_json::json!({"command": "ls"}), + ); + // Orphan completion arrives FIRST (before live's completion). + handle_tool_call_complete(&mut app, "ghost", "weird_tool", &ok_result("ghost-out")); + // Now complete the live tool — it should still mutate the active entry, + // not silently drop or hit a stale index. + handle_tool_call_complete(&mut app, "live", "exec_shell", &ok_result("hello")); + + // Active cell still present (turn hasn't completed). + let active = app.active_cell.as_ref().expect("active cell present"); + let HistoryCell::Tool(ToolCell::Exec(exec)) = &active.entries()[0] else { + panic!("expected exec cell") + }; + assert_eq!(exec.status, ToolStatus::Success); + + // History contains exactly the orphan. + assert_eq!(app.history.len(), 1); + let HistoryCell::Tool(ToolCell::Generic(generic)) = &app.history[0] else { + panic!("expected orphan generic cell") + }; + assert_eq!(generic.name, "weird_tool"); + + // Flush settles the active exec into history below the orphan. + app.flush_active_cell(); + assert_eq!(app.history.len(), 2); +} + +#[test] +fn tool_details_survive_active_cell_flush() { + // The pager / Ctrl+O resolves tool details by cell index. Flushing the + // active cell must move detail records into `tool_details_by_cell` so + // the pager keeps working after the turn settles. + let mut app = create_test_app(); + handle_tool_call_started( + &mut app, + "tid", + "exec_shell", + &serde_json::json!({"command": "echo hi"}), + ); + handle_tool_call_complete(&mut app, "tid", "exec_shell", &ok_result("hi")); + app.flush_active_cell(); + + // The exec cell is now at index 0 in history. + assert_eq!(app.history.len(), 1); + let detail = app + .tool_details_by_cell + .get(&0) + .expect("detail record migrated to flushed cell index"); + assert_eq!(detail.tool_id, "tid"); + assert_eq!(detail.tool_name, "exec_shell"); +} diff --git a/crates/tui/src/tui/widgets/mod.rs b/crates/tui/src/tui/widgets/mod.rs index b736c1e4..0fb26336 100644 --- a/crates/tui/src/tui/widgets/mod.rs +++ b/crates/tui/src/tui/widgets/mod.rs @@ -69,9 +69,33 @@ impl ChatWidget { // we synthesize a uniform slice from the global history_version so // any mutation invalidates every cell (matches the pre-cache // semantics). - let cell_revisions = vec![app.history_version; app.history.len()]; + // + // The active in-flight cell (if any) is appended as the last cell so + // its mutations show up at the live tail. Each entry inside the + // active cell becomes a virtual cell at index `history.len() + i`, + // matching `App::cell_at_virtual_index`. Active-cell entries get a + // distinct revision derived from `active_cell_revision` so changes to + // them only re-render those rows. + let active_entries: &[HistoryCell] = app + .active_cell + .as_ref() + .map_or(&[], |active| active.entries()); + let mut combined_cells: Vec = + Vec::with_capacity(app.history.len() + active_entries.len()); + combined_cells.extend_from_slice(&app.history); + combined_cells.extend_from_slice(active_entries); + let mut cell_revisions = vec![app.history_version; combined_cells.len()]; + // Salt the active-cell revisions with `active_cell_revision` so they + // invalidate independently of the history version when only the + // active cell mutates. + if !active_entries.is_empty() { + let active_rev = app.active_cell_revision; + for rev in cell_revisions.iter_mut().skip(app.history.len()) { + *rev = rev.wrapping_add(active_rev.wrapping_mul(0x9E37_79B9_7F4A_7C15)); + } + } app.transcript_cache.ensure( - &app.history, + &combined_cells, &cell_revisions, content_area.width.max(1), render_options,