feat(tui): tool output keeps its colours

[Hmbown]

No-Issue: internal 0.9.12 shell wave slice.

Rendering wave R4 (0.9.12 runlog): preserve ANSI colour in tool output instead of stripping it.

What changed for the user. When a shell tool's output carries colour — cargo build lines, git/gh with colour forced on, anything run through a PTY — the transcript now paints it as the tool emitted it: the green bold Compiling, red error, yellow warnings. Plain output looks exactly as before.

Why. Every coloured line was flattened to one ink, so the reader lost the tool's own emphasis and had to re-read output the terminal would have shown at a glance.

How. Each OutputRow now also carries styled segments parsed with ansi-to-tui (MIT, 8.0.1; depends on ratatui-core 0.1 already in the lockfile; default features off). Only the painted cell keeps the colour — the model, session store, pager, clipboard and exports still see stripped text; rows without an escape take the old path with no extra allocation. A tool's reset (ESC[0m, Color::Reset) is normalised to "nothing set" before being patched over the cell's value style, so the cell's own dim/state/file:line ink (#5799) shows through wherever the tool set nothing. Wrapped rows are split along the same boundaries wrap_text chose. OSC 8 wrappers are still stripped; links keep resolving from the visible text (osc8::strip_ansi_keep_sgr_into is the new helper).

Evidence.

• ansi_colour_tests (plain stays plain; cargo green bold verb; OSC 8 + SGR mix; wrap boundaries; span patched over the cell style; reset-only stays plain): Summary 6 tests run: 6 passed
• strip_keep_sgr_keeps_colour_and_drops_everything_else: Summary 1 test run: 1 passed
• scripts/dev-test.sh tui: Summary 11858 tests run: 11858 passed, 13 skipped
• cargo clippy --workspace --all-targets with CI flags: clean. cargo fmt: clean.

Not in this PR. Live PTY panes (R3) and word-level diff emphasis (R5) are separate slices.

---

Note

Low Risk
Display-only TUI rendering change with fallback to the plain path when parsing fails; no auth, persistence, or model-facing text changes.

Overview
Coloured shell tool output (cargo, git, PTY runs, etc.) is now painted in the transcript with the tool’s SGR emphasis instead of a single flat ink. Plain lines behave as before with no extra allocation.

How it works: OutputRow optionally carries parsed StyledSegments built with ansi-to-tui after strip_ansi_keep_sgr_into keeps only ESC [ … m sequences while still stripping OSC 8, cursor controls, and other escapes. Display-only rendering patches those styles over the existing cell ink (diff/file:line/dim); resets are normalized so they don’t wipe cell styling. Wrapped rows split colours along the same wrap_text boundaries; intact paths stay one line for unchanged click hitboxes.

Dependency: ansi-to-tui 8.0.1 (default features off) on codewhale-tui. Session store, model, pager, clipboard, and exports still use fully stripped text.

^{Reviewed by Cursor Bugbot for commit 661c877. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_b1ea3a93-7bf1-4f24-a600-cb68e03a123e)

[claude]

Claude finished @Hmbown's task in 3m 16s —— View job

---

Review complete ✅

• Gather context (read diff, changed files)
• Understand request (code review of PR feat(tui): tool output keeps its colours #5812)
• Review correctness/panics
• Review provider/model/route safety (N/A — no provider/model/routing code touched)
• Review reuse/simplification
• Review tests
• Review security
• Post final review

Summary

Solid, well-scoped feature. The reset-normalization design (tool_style, crates/tui/src/tui/history/tool_output.rs:495-513) is a genuinely good call — patching only positive SGR requests over value_style instead of a raw Style::reset() correctly avoids clobbering the cell's own dim/state/file:line ink. The round-trip guard in styled_segments (bails to the plain path whenever the parsed text disagrees with the stripped text) is a nice defensive touch, and the strip_ansi_keep_sgr_into OSC-8-vs-SGR split is clean. Test coverage for the happy paths (cargo bold-green, OSC8+SGR mix, wrap-boundary splitting, reset-only) is good.

One real bug found, plus a couple of minor notes.

Bug: styled rows bypass the intact (no-wrap) path for path/URL-like lines

crates/tui/src/tui/history/tool_output.rs:438-457 computes styled for a line before branching on intact, so an OutputRow can be both intact: true and styled: Some(...). But render_output_row (crates/tui/src/tui/history/tool_output.rs:693-706) checks row.styled first:

if let Some(segments) = &row.styled {
    lines.extend(render_card_detail_line_styled(label, segments, value_style, width));
} else if row.intact {
    lines.push(render_card_detail_line_single(label, &row.text, value_style));
} else { ... }

intact exists specifically to keep path/URL lines on one unwrapped line (is_path_or_url_like at tool_output.rs:626-636; it's the only consumer of .intact in the whole crate). render_card_detail_line_styled wraps via wrap_text(&full, content_width) regardless. So any colored line that also looks like a path/URL — e.g. a git diff --color +++ b/src/foo.rs / --- a/src/foo.rs header (bold, contains / + a ≤8-char extension → is_path_or_url_like = true), or a colored URL from gh — now gets wrapped across multiple Lines instead of staying intact, which is exactly the case this PR's own description calls out as a target scenario (git/gh with colour forced on). This is untested — none of the six new ansi_colour_tests combine intact with color.

Fix: gate the styled branch on !row.intact, or give the styled path a no-wrap single-line variant analogous to render_card_detail_line_single for the intact case.

Fix this →

Minor notes (non-blocking)

• render_card_detail_line_styled (crates/tui/src/tui/history.rs:2611) calls segments.to_vec() to hand ownership to split_segments, cloning every String segment on each render of a colored row. Given rows are frame-rendered from a cache, this is a per-frame allocation cost for any colored output — likely fine given the PR's own "no extra allocation for plain rows" framing only promises that for the uncolored path, but worth knowing if colored cargo/CI output turns out to dominate.
• Cargo.lock now carries both nom 7.1.3 and nom 8.0.0 (the new ansi-to-tui pulls in the latter). Not actionable here, just a minor build-graph note.

Everything else — the strip_ansi_keep_sgr_into CSI/SGR-only pass-through, split_segments' char-count-based re-splitting along wrap_text's boundaries, and the empty/all-default-style bailouts in styled_segments — checked out correct against the wrap_text hard-char-wrap implementation it depends on.

[cursor]

Stale comment

Left a non-blocking comment; Cursor Bugbot was present but skipped (usage limit), so this PR is not approved. Human review is needed, and reviewers were assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[Copilot]

🟡 Changes recommended

Styled path/URL-like output currently gets wrapped (because row.styled takes precedence over row.intact), which can break path:line detection for “Open file at line” and contradict the existing “keep intact” behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR preserves ANSI SGR colouring in TUI tool-output transcripts by parsing colour/style codes into styled segments and rendering them with ratatui styles, while keeping the stored/exported text fully ANSI-stripped.

Changes:

• Add styled: Option<Vec<StyledSegment>> to OutputRow and parse SGR sequences into styled segments (using ansi-to-tui) for colourized tool output.
• Introduce strip_ansi_keep_sgr_into to remove non-SGR escapes (including OSC 8) while retaining SGR for styling.
• Add a styled rendering path that patches tool-provided styles over the cell’s existing value style.

File summaries

File	Description
crates/tui/src/tui/history/tool_output.rs	Builds OutputRow styled segments from SGR and renders them via a new styled line renderer.
crates/tui/src/tui/history.rs	Adds render_card_detail_line_styled to paint tool-provided styles while preserving cell ink where unset.
crates/tui/src/tui/osc8.rs	Adds strip_ansi_keep_sgr_into and refactors ANSI stripping to optionally retain SGR.
crates/tui/src/tui/output_rows_cache.rs	Updates cached OutputRow test helpers for the new styled field.
crates/tui/Cargo.toml	Adds ansi-to-tui dependency (default features off).
Cargo.lock	Locks ansi-to-tui and transitive dependencies.

Review details

• Files reviewed: 5/6 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codewhale-agent]

Codewhale review

The PR adds ANSI colour preservation to TUI tool output by storing styled segments on OutputRow and rendering them with a new styled detail-line path. Text for non-display consumers remains stripped. The change is well tested and generally conservative, with two display-path concerns.

Findings

• [WARNING] Styled intact rows no longer use single-line rendering (crates/tui/src/tui/history.rs:693)
  When a row is intact (path/URL-like) and carries SGR, render_output_row now routes it through render_card_detail_line_styled, which wraps to width like non-intact rows. Previously intact rows were rendered as a single line via render_card_detail_line_single. Long coloured paths/URLs can now wrap across multiple lines, potentially changing link/path click regions and diverging from the plain intact path.
• [INFO] Styled render path clones segments and re-wraps every frame (crates/tui/src/tui/history.rs:2682)
  render_card_detail_line_styled builds a fresh full String and calls split_segments(segments.to_vec(), ...) for every visible coloured row on each frame. For long coloured tool output this is avoidable; split_segments could borrow from the already-owned row.styled Vec, and full could reuse row.text. Not a correctness bug, but worth optimizing.

Suggestions

• crates/tui/src/tui/history.rs:693 — Add a styled single-line renderer for row.intact (or extend render_card_detail_line_single to accept styled segments) so coloured paths/URLs stay on one line and keep the existing intact/link behaviour.
• crates/tui/src/tui/history.rs:2682 — Change split_segments to accept &[StyledSegment] and avoid creating full/cloning segments every frame; reuse the existing row.text or concatenate once.

Assessment

Solid feature with good test coverage and careful separation of display styling from stripped text. The main risk is the rendering path divergence for intact coloured rows; otherwise the implementation is clean and conservative.

---

Advisory review by Codewhale (codewhale review --pr 5812 --post, head 490608cdba309a3cbe6c91b89f7d3c25a73380f2). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[INFO] Styled render path clones segments and re-wraps every frame

render_card_detail_line_styled builds a fresh full String and calls split_segments(segments.to_vec(), ...) for every visible coloured row on each frame. For long coloured tool output this is avoidable; split_segments could borrow from the already-owned row.styled Vec, and full could reuse row.text. Not a correctness bug, but worth optimizing.

[codewhale-agent]

Change split_segments to accept &[StyledSegment] and avoid creating full/cloning segments every frame; reuse the existing row.text or concatenate once.

[Hmbown]

Review addressed in 661c87702: intact rows with colour now render through a single-line styled path (same rail+label prefix, never wrapped), so click regions are identical with or without colour; split_segments borrows and the wrapped renderer reuses row.text. New test coloured_intact_path_stays_on_one_line_with_the_plain_hitbox. Evidence: ansi_colour 7 passed; history 199 passed; full tui area 11859 passed, 13 skipped; clippy (CI flags) clean.

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_bc5c2133-312c-4a55-99f1-b298c5e003cd)

[claude]

Claude finished @Hmbown's task in 2m 49s —— View job

---

Review complete ✅

• Gather context (read diff 88ba3cd..HEAD, changed files, prior bot reviews)
• Understand request (code review of PR feat(tui): tool output keeps its colours #5812)
• Review correctness/panics
• Review provider/model/route safety (N/A — no provider/model/routing code touched)
• Review reuse/simplification
• Review tests
• Review security

Summary

Good news: the one real bug flagged by Cursor Bugbot, Copilot, and the earlier Codewhale review — styled intact rows (colourized path/URL lines) getting wrapped because row.styled took precedence over row.intact in render_output_row — is already fixed in this PR's latest commit, 661c877 ("a coloured intact path stays on one line"). It adds render_card_detail_line_single_styled (crates/tui/src/tui/history.rs:2721), which reuses render_card_detail_line_single's rail/label plumbing (build with an empty value, pop the placeholder span, push the real styled segments) rather than duplicating it — a clean, minimal reuse. render_output_row (crates/tui/src/tui/history/tool_output.rs:693) now branches on row.intact inside the styled arm, so a coloured git diff --color +++ b/... header or a coloured URL from gh stays on one unwrapped line with the same click region as the plain path. The new test coloured_intact_path_stays_on_one_line_with_the_plain_hitbox (tool_output.rs:830) covers exactly this case, including asserting the rail/label prefix spans are identical between the plain and coloured renders.

The same commit also resolves the earlier "clones every frame" note: render_card_detail_line_styled now takes text: &str instead of rebuilding a full string from segments, and split_segments now takes &[StyledSegment] instead of segments.to_vec() — no more per-frame allocation/clone beyond what's needed.

Re-reviewed the rest of the diff (crates/tui/src/tui/history/tool_output.rs, history.rs, osc8.rs) against 88ba3cd (the branch point, matching the PR's stated 374/-3 across 6 files):

• styled_segments (tool_output.rs:544) bails to None on a round-trip mismatch between the parsed segments and the fully-stripped text, and on an all-default-style parse — both correctly keep the plain path as the fallback truth rather than risking a mis-rendered line.
• tool_style (tool_output.rs:576) filters Color::Reset per-channel (fg/bg independently) and only copies add_modifier, never sub_modifier. That means an explicit "un-bold" mid-line SGR (ESC[22m) combined with a value_style that's already bold (diff/file:line ink) wouldn't un-bold when patched — Style::patch only clears bits present in the other style's sub_modifier, which tool_style never sets. This is a narrow edge case (needs an explicit un-bold code colliding with diff/file-line highlighting in the same cell) and not something cargo/git/gh output is likely to trigger — flagging as a minor note, not a blocker.
• split_segments (tool_output.rs:614) walks segments/parts by .chars(), consistent with wrap_text's char-by-char wrap boundary decisions (width-based break, char-based split) — the two stay in sync since parts are literal substrings of the same char sequence.
• No provider/model/prefix inference touched; no shell/exec/secret handling touched. ansi-to-tui is a well-scoped, default-features-off addition parsing already-trusted-for-display tool output (previously fully stripped, now parsed for style) — no new trust boundary.

Test coverage for the new behavior (plain stays plain, cargo bold-green, OSC8+SGR mix, wrap-boundary split, reset-only, patched cell style, and now the intact+colour case) is solid.

Not run: cargo test / cargo clippy / cargo fmt — not available under this job's allowed tools. The PR description's own evidence (ansi_colour_tests: 6 passed; strip_keep_sgr_keeps_colour_and_drops_everything_else: 1 passed; scripts/dev-test.sh tui: 11858 passed; clean clippy/fmt) is the evidence of record for this change.

[cursor]

Left a non-blocking comment; Cursor Bugbot was present but skipped (usage limit), so this PR is not approved. Human review is needed, and reviewers were assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[codewhale-agent]

Codewhale review

The PR preserves ANSI SGR colours in tool output by attaching optional styled segments to OutputRow and patching those styles over the cell's existing ink during rendering. The parsing and OSC8 handling have focused tests, and the fallback path keeps plain rows allocation-free.

Findings

• [WARNING] Styled single-line renderer relies on popping a dummy empty value span (crates/tui/src/tui/history.rs)
  render_card_detail_line_single_styled builds a plain line with an empty value, then calls line.spans.pop() under the assumption that the last span is the empty value span. This works with the current render_card_detail_line_single implementation, but if that function ever changes to omit empty value spans, the pop will remove the label/gap span and corrupt the prefix/hitbox. The styled path should construct its prefix directly or use a dedicated helper instead of depending on this implementation detail.
• [INFO] Missing coverage for labelled wrapped styled rows and Unicode wrap boundaries (crates/tui/src/tui/history/tool_output.rs)
  The new styled wrapper is exercised for unlabelled row splitting and for a labelled intact row, but not for the combination of a label plus a wrapped coloured row. The split_segments helper is also only tested with ASCII text. A regression in the label/indent logic for wrapped lines, or in splitting styled segments when wrap_text breaks on Unicode-width boundaries, would not be caught by the current tests.

Suggestions

• crates/tui/src/tui/history.rs — Construct the prefix spans directly in render_card_detail_line_single_styled rather than calling render_card_detail_line_single with an empty value and popping the last span. This removes the hidden coupling to the plain function's exact span layout.
• crates/tui/src/tui/history/tool_output.rs — Add a test that renders a labelled, wrapped coloured row and asserts the rail, label/gap indent, and styled segments on each line. Also add a wrapped-row test containing a multi-byte or wide character to verify split_segments stays aligned with wrap_text.

Assessment

Display-only change with a solid fallback and good targeted tests. No correctness issues were found in the diff, but the fragile pop-based prefix construction and the missing labelled wrapped-row coverage are worth addressing before merge.

---

Advisory review by Codewhale (codewhale review --pr 5812 --post, head 661c87702133b1943794c155d04c6bf9d83ce553). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[devin-ai-integration]

Landing check (read the review threads, not the rollup):

• Review threads: 3 threads, all outdated by 661c877 (styled intact-row wrapping, split_segments clone); no open findings.
• Required checks green (buildkite, Lint, Test windows, npm smoke).
• Test (macos-latest) fails only chatgpt_oauth::tests::callback_is_accepted_on_either_loopback_family (crates/tui/src/chatgpt_oauth.rs:1639). This PR does not touch that file (diff: Cargo.lock, crates/tui/Cargo.toml, history.rs, history/tool_output.rs, osc8.rs, output_rows_cache.rs).
• Version drift fails identically on main (run 33580750475, job 100094325067: crates/tui/CHANGELOG.md is out of date with the root CHANGELOG.md slice + missing receipts for feat(tui): the work surface sits under the composer by default #5809/Native ChatGPT/Codex subscription sign-in without the Codex CLI installed #5778/feat(providers): Concentrate as a first-class opt-in BYOK Responses gateway #5725/feat(cli): Daytona cloud-agent dispatch #5701/feat(cli): cloud-dispatch remote runner — sandbox to forge PR #5712/feat(compaction): publish survival contract and keep last round (#4394) #5782). Pre-existing main debt, tracked for the integration branch.