Skip to content

Commit 3eed63b

Browse files
CSResselnori-agent
andauthored
fix(tui): route all ACP tools through ClientToolCell, fix rendering bugs (#420)
## Summary 🤖 Generated with [Nori](https://www.npmjs.com/package/nori-ai) - **Fix Read/Search rendering**: Auto-detect as "Explored" in `display_lines` via `is_exploring_snapshot()` instead of falling to generic "Tool [completed]" format - **Fix transcript formatting**: Execute tools use shell-style `$ command` in `transcript_lines`; exploring cells use "Explored" format (previously swapped) - **Simplify dispatch**: All ACP tool kinds route through `ClientToolCell` natively, removing the exec-like adaptation path that converted Read/Search into fake `ExecCommandBegin/End` events (-29 lines net) ## Test Plan - [x] 4 new unit tests covering the three bugs + regression guard - [x] 1198 unit tests pass - [x] 10 E2E ACP tool call tests pass - [x] Clippy and rustfmt clean Share Nori with your team: https://www.npmjs.com/package/nori-skillsets --------- Co-authored-by: Nori <contact@tilework.tech>
1 parent c532c6b commit 3eed63b

4 files changed

Lines changed: 284 additions & 297 deletions

File tree

codex-rs/tui/docs.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,19 @@ The chat interface is managed by the `chatwidget/` module (`chatwidget/mod.rs` +
5353
- File search integration (`file_search.rs`)
5454
- Pager overlay for reviewing long content (`pager_overlay.rs`)
5555

56-
The transcript pager overlay uses each history cell's transcript view rather than the live summary view. To keep reopened transcripts readable, the overlay caps non-patch cells at 20 lines and appends an omission marker, while patch cells keep their full diff output for review. In ACP sessions, file operation tool snapshots render through `ClientToolCell` with `render_edit_lines()`, which produces the same `transcript_lines()` as `display_lines()` (both dispatch to the edit rendering path).
56+
The transcript pager overlay uses each history cell's transcript view rather than the live summary view. To keep reopened transcripts readable, the overlay caps non-patch cells at 20 lines and appends an omission marker, while patch cells keep their full diff output for review. In ACP sessions, `ClientToolCell` provides differentiated `transcript_lines()` for Execute tools (shell-style `$ command` format via `render_execute_transcript_lines()`) while exploring and edit cells reuse their `display_lines()` rendering for transcripts.
5757

5858
**Approval Request Routing** (`chatwidget/event_handlers.rs`, `bottom_pane/approval_overlay.rs`): ACP approval requests arrive as `ClientEvent::ApprovalRequest` containing a `nori_protocol::ToolSnapshot`. The `approval_request_from_client_event()` function performs two-way routing: Execute tools with `Invocation::Command` map to `ApprovalRequest::Exec` (bash-highlighted overlay), and everything else (including Edit/Delete/Move) maps to `ApprovalRequest::AcpTool`. The `AcpTool` variant carries a boxed `ToolSnapshot`, a `cwd: PathBuf` (threaded from `self.config.cwd` in the chat widget), and dispatches decisions via `Op::ExecApproval`, which gives users the "always approve" option that `ApplyPatch` did not have. The `From<ApprovalRequest>` impl in `approval_overlay.rs` applies `relativize_paths_in_text` to the title before building the overlay prompt and `DiffSummary`, so users see relative paths instead of absolute ones. The fullscreen approval preview in `app/event_handling.rs` also uses the real `cwd` from the request for `DiffSummary` construction. `ApprovalRequest::ApplyPatch` is now only used by the legacy non-ACP codex backend. History cells for AcpTool decisions are produced by `history_cell::new_acp_approval_decision_cell()`, using `format_tool_kind()` for the kind label.
5959

6060
For edit-like tools (Edit/Delete/Move), both the approval overlay and the fullscreen preview extract diff data from the `ToolSnapshot` and render a `DiffSummary`. The diff extraction reuses two `pub(crate)` helpers from `client_tool_cell.rs`: `diff_changes_from_artifacts()` (checks `Artifact::Diff` entries) with fallback to `changes_from_invocation()` (handles `Invocation::FileChanges` and `Invocation::FileOperations`). When diff data is available, the overlay renders a `DiffSummary` via `ColumnRenderable` and the fullscreen preview renders a `DiffSummary` overlay titled "P A T C H". When no diff data is available, both paths fall back to text-only rendering of title, invocation, and artifacts.
6161

6262
**ClientToolCell Rendering** (`client_tool_cell.rs`):
6363

64-
`ClientToolCell` wraps a `nori_protocol::ToolSnapshot` (and a `cwd` path for path normalization) and implements `HistoryCell`. It selects between four rendering paths based on cell state: exploring cells use `render_exploring_lines(width)`, `ToolKind::Execute` uses `render_execute_lines(width)` for ExecCell-parity display, Edit/Delete/Move kinds use `render_edit_lines()` for semantic verb headers with diff content, and all remaining tool kinds use `render_generic_lines()` for the generic `"Tool [phase]: title (kind)"` format with invocation/artifact details.
64+
`ClientToolCell` wraps a `nori_protocol::ToolSnapshot` (and a `cwd` path for path normalization) and implements `HistoryCell`. All ACP tool kinds route through `ClientToolCell` via `handle_client_native_tool_snapshot`. The cell selects between four rendering paths based on cell state: exploring cells (Read/Search, auto-detected via `is_exploring_snapshot()` or merged via `exploring_snapshots`) use `render_exploring_lines(width)`, `ToolKind::Execute` uses `render_execute_lines(width)` for display and `render_execute_transcript_lines(width)` for shell-style transcripts, Edit/Delete/Move kinds use `render_edit_lines()` for semantic verb headers with diff content, and all remaining tool kinds use `render_generic_lines()` for the generic `"Tool [phase]: title (kind)"` format with invocation/artifact details.
6565

66-
**Exploring cell grouping**: When consecutive Read/Search/ListFiles snapshots arrive, they are merged into a single `ClientToolCell` with a grouped exploring rendering. The exploring display shows a compact `Explored`/`Exploring` header with tree-prefixed sub-items that group consecutive reads by filename (e.g., `Read file1.rs, file2.rs`) and show `Search`/`List` labels with compact arguments. Read output content is omitted from exploring cells since it is noise in history. The merge logic in `handle_client_native_tool_snapshot` checks whether the active cell is an exploring `ClientToolCell` and the new snapshot is also exploring; if so, it merges the snapshot via `merge_exploring()` rather than creating a new cell. A single exploring snapshot (not merged with others) still uses `render_exploring_lines` -- when `exploring_snapshots` is empty, the renderer falls back to the primary snapshot. The generic fallback sub-item renderer avoids duplicating the kind label when the title already starts with it (case-insensitive prefix check), e.g., `List /path` instead of `List List /path`.
66+
**Exploring cell grouping**: When consecutive Read/Search/ListFiles snapshots arrive, they are merged into a single `ClientToolCell` with a grouped exploring rendering. The exploring display shows a compact `Explored`/`Exploring` header with tree-prefixed sub-items that group consecutive reads by basename (e.g., `Read file1.rs, file2.rs`) and show `Search`/`List` labels with compact arguments. Read output content is omitted from exploring cells since it is noise in history. The merge logic in `handle_client_native_tool_snapshot` checks whether the active cell is an exploring `ClientToolCell` and the new snapshot is also exploring; if so, it merges the snapshot via `merge_exploring()` rather than creating a new cell. `merge_exploring()` deduplicates by `call_id` — if a snapshot with the same call_id already exists in the group, it is updated in place rather than appended. Merged call_ids are tracked in `completed_client_tool_calls` so completions arriving after the cell is flushed to history don't get re-merged into a later exploring cell. A standalone Read/Search snapshot (not merged with others) still uses `render_exploring_lines` — the auto-detection via `is_exploring_snapshot()` in `display_lines`/`transcript_lines` routes it there without requiring explicit `mark_exploring()`. The generic fallback sub-item renderer avoids duplicating the kind label when the title already starts with it (case-insensitive prefix check), e.g., `List /path` instead of `List List /path`.
6767

68-
**Tool title sanitization** (`client_event_format.rs`): The `sanitize_tool_title()` function cleans up noisy tool titles produced by some agents (notably Gemini). It strips `[current working directory ...]` bracket patterns and trailing `(description text)` parenthetical metadata, then trims whitespace. This is applied in `exec_begin_event_from_client_snapshot` and the helper functions `generic_tool_command_text` and `generic_execute_command_text` in `event_handlers.rs`, ensuring that Execute, Edit, and other tool kinds all display clean titles in the TUI.
68+
**Tool title sanitization** (`client_event_format.rs`): The `sanitize_tool_title()` function cleans up noisy tool titles produced by some agents (notably Gemini). It strips `[current working directory ...]` bracket patterns and trailing `(description text)` parenthetical metadata, then trims whitespace. This is applied in the approval request path and helper functions in `event_handlers.rs`, ensuring that tool kinds display clean titles in the TUI.
6969

7070
**Execute rendering**: The execute rendering path reuses shared utilities from `exec_cell/render.rs` (`truncate_lines_middle`, `limit_lines_from_start`, `output_lines`, `spinner`) and layout constants that match the `ExecCell` display layout. Output text is sourced preferentially from `raw_output["stdout"]`, falling back to `Artifact::Text` with code fence stripping only for completed/failed snapshots. During pending/in-progress phases, artifact text for execute tools contains the agent's description (e.g., "Print current UTC date/time"), not stdout, so the fallback is suppressed via `is_active_phase` gating in `execute_output_text()`. Exit code success is determined from `raw_output["exit_code"]` when present, otherwise inferred from `ToolPhase`.
7171

@@ -119,7 +119,7 @@ The trade-off: incomplete cells may appear in scrollback showing "Running"/"Expl
119119

120120
**Interrupt Queue & Tool Event Deferral** (`chatwidget/event_handlers.rs`):
121121

122-
When the agent streams text, ACP `ClientEvent::ToolSnapshot` updates can arrive concurrently with answer or reasoning deltas. The TUI adapts those normalized snapshots into the existing exec-cell and patch-cell machinery, and the relevant handlers call `flush_answer_stream_with_separator()` before deferring or rendering so tool cells appear in their correct interleaved position relative to text rather than being grouped after all text. The `InterruptManager` queues events via `defer_or_handle()` when the queue is already non-empty, preserving FIFO ordering for events that arrive while earlier deferred events are pending.
122+
When the agent streams text, ACP `ClientEvent::ToolSnapshot` updates can arrive concurrently with answer or reasoning deltas. All ACP tool kinds route directly through `ClientToolCell` via `handle_client_native_tool_snapshot`, and the handler calls `flush_answer_stream_with_separator()` before deferring or rendering so tool cells appear in their correct interleaved position relative to text rather than being grouped after all text. The `InterruptManager` queues events via `defer_or_handle()` when the queue is already non-empty, preserving FIFO ordering for events that arrive while earlier deferred events are pending.
123123

124124
One operation consumes the queue:
125125

0 commit comments

Comments
 (0)