mirror of
https://github.com/openai/codex.git
synced 2026-10-31 19:28:49 +00:00
dev/kyleb/plugin-script-shell-lifecycle
19
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Felipe CouryandEric Traut
|
bede1d9e23 |
fix(tui): refresh footer on collaboration mode changes (#16026)
## Summary - Moves status surface refresh (`refresh_status_surfaces` / `refresh_status_line`) from `App` event handlers into `ChatWidget` setters via a new `refresh_model_dependent_surfaces()` method - Ensures model-dependent UI stays in sync whenever collaboration mode, model, or reasoning effort changes, including the footer and terminal title in both `tui` and `tui_app_server` - Applies the fix to both `tui` and `tui_app_server` widgets #15961 ## Test plan - [x] Added snapshot test `status_line_model_with_reasoning_plan_mode_footer` verifying footer renders correctly in plan mode - [x] Added `terminal_title_model_updates_on_model_change_without_manual_refresh` in `tui_app_server` - [ ] Verify switching collaboration modes updates the footer in real TUI - [ ] Verify model/reasoning effort changes reflect in the status bar and terminal title --------- Co-authored-by: Eric Traut <etraut@openai.com> |
||
|
Felipe CouryandEric Traut
|
2c54d4b160 |
feat(tui): add terminal title support to tui app server (#15860)
## TR;DR
Replicates the `/title` command from `tui` to `tui_app_server`.
## Problem
The classic `tui` crate supports customizing the terminal window/tab
title via `/title`, but the `tui_app_server` crate does not. Users on
the app-server path have no way to configure what their terminal title
shows (project name, status, spinner, thread, etc.), making it harder to
identify Codex sessions across tabs or windows.
## Mental model
The terminal title is a *status surface* -- conceptually parallel to the
footer status line. Both surfaces are configurable lists of items, both
share expensive inputs (git branch lookup, project root discovery), and
both must be refreshed at the same lifecycle points. This change ports
the classic `tui`'s design verbatim:
1. **`terminal_title.rs`** owns the low-level OSC write path and input
sanitization. It strips control characters and bidi/invisible codepoints
before placing untrusted text (model output, thread names, project
paths) inside an escape sequence.
2. **`title_setup.rs`** defines `TerminalTitleItem` (the 8 configurable
items) and `TerminalTitleSetupView` (the interactive picker that wraps
`MultiSelectPicker`).
3. **`status_surfaces.rs`** is the shared refresh pipeline. It parses
both surface configs once per refresh, warns about invalid items once
per session, synchronizes the git-branch cache, then renders each
surface from the same `StatusSurfaceSelections` snapshot.
4. **`chatwidget.rs`** sets `TerminalTitleStatusKind` at each state
transition (Working, Thinking, Undoing, WaitingForBackgroundTerminal)
and calls `refresh_terminal_title()` whenever relevant state changes.
5. **`app.rs`** handles the three setup events (confirm/preview/cancel),
persists config via `ConfigEditsBuilder`, and clears the managed title
on `Drop`.
## Non-goals
- **Restoring the previous terminal title on exit.** There is no
portable way to read the terminal's current title, so `Drop` clears the
managed title rather than restoring it.
- **Sharing code between `tui` and `tui_app_server`.** The
implementation is a parallel copy, matching the existing pattern for the
status-line feature. Extracting a shared crate is future work.
## Tradeoffs
- **Duplicate code across crates.** The three core files
(`terminal_title.rs`, `title_setup.rs`, `status_surfaces.rs`) are
byte-for-byte copies from the classic `tui`. This was chosen for
consistency with the existing status-line port and to avoid coupling the
two crates at the dependency level. Future changes must be applied in
both places.
- **`status_surfaces.rs` is large (~660 lines).** It absorbs logic that
previously lived inline in `chatwidget.rs` (status-line refresh, git
branch management, project root discovery) plus all new terminal-title
logic. This consolidation trades file size for a single place where both
surfaces are coordinated.
- **Spinner scheduling on every refresh.** The terminal title spinner
(when active) schedules a frame every 100ms. This is the same pattern
the status-indicator spinner already uses; the overhead is a timer
registration, not a redraw.
## Architecture
```
/title command
-> SlashCommand::Title
-> open_terminal_title_setup()
-> TerminalTitleSetupView (MultiSelectPicker)
-> on_change: AppEvent::TerminalTitleSetupPreview -> preview_terminal_title()
-> on_confirm: AppEvent::TerminalTitleSetup -> ConfigEditsBuilder + setup_terminal_title()
-> on_cancel: AppEvent::TerminalTitleSetupCancelled -> cancel_terminal_title_setup()
Runtime title refresh:
state change (turn start, reasoning, undo, plan update, thread rename, ...)
-> set terminal_title_status_kind
-> refresh_terminal_title()
-> status_surface_selections() (parse configs, collect invalids)
-> refresh_terminal_title_from_selections()
-> terminal_title_value_for_item() for each configured item
-> assemble title string with separators
-> skip if identical to last_terminal_title (dedup OSC writes)
-> set_terminal_title() (sanitize + OSC 0 write)
-> schedule spinner frame if animating
Widget replacement:
replace_chat_widget_with_app_server_thread()
-> transfer last_terminal_title from old widget to new
-> avoids redundant OSC clear+rewrite on session switch
```
## Observability
- Invalid terminal-title item IDs in config emit a one-per-session
warning via `on_warning()` (gated by
`terminal_title_invalid_items_warned` `AtomicBool`).
- OSC write failures are logged at `tracing::debug` level.
- Config persistence failures are logged at `tracing::error` and
surfaced to the user via `add_error_message()`.
## Tests
- `terminal_title.rs`: 4 unit tests covering sanitization (control
chars, bidi codepoints, truncation) and OSC output format.
- `title_setup.rs`: 3 tests covering setup view snapshot rendering,
parse order preservation, and invalid-ID rejection.
- `chatwidget/tests.rs`: Updated test helpers with new fields; existing
tests continue to pass.
---------
Co-authored-by: Eric Traut <etraut@openai.com>
|
||
|
Felipe CouryandEric Traut
|
e9996ec62a |
fix(tui_app_server): preserve transcript events under backpressure (#15759)
## TL;DR When running codex with `-c features.tui_app_server=true` we see corruption when streaming large amounts of data. This PR marks other event types as _critical_ by making them _must-deliver_. ## Problem When the TUI consumer falls behind the app-server event stream, the bounded `mpsc` channel fills up and the forwarding layer drops events via `try_send`. Previously only `TurnCompleted` was marked as must-deliver. Streamed assistant text (`AgentMessageDelta`) and the authoritative final item (`ItemCompleted`) were treated as droppable — the same as ephemeral command output deltas. Because the TUI renders markdown incrementally from these deltas, dropping any of them produces permanently corrupted or incomplete paragraphs that persist for the rest of the session. ## Mental model The app-server event stream has two tiers of importance: 1. **Lossless (transcript + terminal):** Events that form the authoritative record of what the assistant said or that signal turn lifecycle transitions. Losing any of these corrupts the visible output or leaves surfaces waiting forever. These are: `AgentMessageDelta`, `PlanDelta`, `ReasoningSummaryTextDelta`, `ReasoningTextDelta`, `ItemCompleted`, and `TurnCompleted`. 2. **Best-effort (everything else):** Ephemeral status events like `CommandExecutionOutputDelta` and progress notifications. Dropping these under load causes cosmetic gaps but no permanent corruption. The forwarding layer uses `try_send` for best-effort events (dropping on backpressure) and blocking `send().await` for lossless events (applying back-pressure to the producer until the consumer catches up). ## Non-goals - Eliminating backpressure entirely. The bounded queue is intentional; this change only widens the set of events that survive it. - Changing the event protocol or adding new notification types. - Addressing root causes of consumer slowness (e.g. TUI render cost). ## Tradeoffs Blocking on transcript events means a slow consumer can now stall the producer for the duration of those events. This is acceptable because: (a) the alternative is permanently broken output, which is worse; (b) the consumer already had to keep up with `TurnCompleted` blocking sends; and (c) transcript events arrive at model-output speed, not burst speed, so sustained saturation is unlikely in practice. ## Architecture Two parallel changes, one per transport: - **In-process path** (`lib.rs`): The inline forwarding logic was extracted into `forward_in_process_event`, a standalone async function that encapsulates the lag-marker / must-deliver / try-send decision tree. The worker loop now delegates to it. A new `server_notification_requires_delivery` function (shared `pub(crate)`) centralizes the notification classification. - **Remote path** (`remote.rs`): The local `event_requires_delivery` now delegates to the same shared `server_notification_requires_delivery`, keeping both transports in sync. ## Observability No new metrics or log lines. The existing `warn!` on event drops continues to fire for best-effort events. Lossless events that block will not produce a log line (they simply wait). ## Tests - `event_requires_delivery_marks_transcript_and_terminal_events`: unit test confirming the expanded classification covers `AgentMessageDelta`, `ItemCompleted`, `TurnCompleted`, and excludes `CommandExecutionOutputDelta` and `Lagged`. - `forward_in_process_event_preserves_transcript_notifications_under_backpressure`: integration-style test that fills a capacity-1 channel, verifies a best-effort event is dropped (skipped count increments), then sends lossless transcript events and confirms they all arrive in order with the correct lag marker preceding them. - `remote_backpressure_preserves_transcript_notifications`: end-to-end test over a real websocket that verifies the remote transport preserves transcript events under the same backpressure scenario. - `event_requires_delivery_marks_transcript_and_disconnect_events` (remote): unit test confirming the remote-side classification covers transcript events and `Disconnected`. --------- Co-authored-by: Eric Traut <etraut@openai.com> |
||
|
Felipe CouryandEric Traut
|
79359fb5e7 |
fix(tui_app_server): fix remote subagent switching and agent names (#15513)
## TL;DR This PR changes the `tui_app_server` _path_ in the following ways: - add missing feature to show agent names (shows only UUIDs today) - add `Cmd/Alt+Arrows` navigation between agent conversations ## Problem When the TUI connects to a remote app server, collab agent tool-call items (spawn, wait, delegate, etc.) render thread UUIDs instead of human-readable agent names because the `ChatWidget` never receives nickname/role metadata for receiver threads. Separately, keyboard next/previous agent navigation silently does nothing when the local `AgentNavigationState` cache has not yet been populated with subagent threads that the remote server already knows about. Both issues share a root cause: in the remote (app-server) code path the TUI never proactively fetches thread metadata. In the local code path this metadata arrives naturally via spawn events the TUI itself orchestrates, but in the remote path those events were processed by a different client and the TUI only sees the resulting collab tool-call notifications. ## Mental model Collab agent tool-call notifications reference receiver threads by id, but carry no nickname or role. The TUI needs that metadata in two places: 1. **Rendering** -- `ChatWidget` converts `CollabAgentToolCall` items into history cells. Without metadata, agent status lines show raw UUIDs. 2. **Navigation** -- `AgentNavigationState` tracks known threads for the `/agent` picker and keyboard cycling. Without entries for remote subagents, next/previous has nowhere to go. This change closes the gap with two complementary strategies: - **Eager hydration**: when any notification carries `receiver_thread_ids`, the TUI fetches metadata (`thread/read`) for threads it has not yet cached before the notification is rendered. - **Backfill on thread switch**: when the user resumes, forks, or starts a new app-server thread, the TUI fetches the full `thread/loaded/list`, walks the parent-child spawn tree, and registers every descendant subagent in both the navigation cache and the `ChatWidget` metadata map. A new `collab_agent_metadata` side-table in `ChatWidget` stores nickname/role keyed by `ThreadId`, kept in sync by `App` whenever it calls `upsert_agent_picker_thread`. The `replace_chat_widget` helper re-seeds this map from `AgentNavigationState` so that thread switches (which reconstruct the widget) do not lose previously discovered metadata. ## Non-goals - This change does not alter the local (non-app-server) collab code path. That path already receives metadata via spawn events and is unaffected. - No new protocol messages are introduced. The change uses existing `thread/read` and `thread/loaded/list` RPCs. - No changes to how `AgentNavigationState` orders or cycles through threads. The traversal logic is unchanged; only the population of entries is extended. ## Tradeoffs - **Extra RPCs on notification path**: `hydrate_collab_agent_metadata_for_notification` issues a `thread/read` for each unknown receiver thread before the notification is forwarded to rendering. This adds latency on the notification path but only fires once per thread (the result is cached). The alternative -- rendering first and backfilling names later -- would cause visible flicker as UUIDs are replaced with names. - **Backfill fetches all loaded threads**: `backfill_loaded_subagent_threads` fetches the full loaded-thread list and walks the spawn tree even when the user may only care about one subagent. This is simple and correct but O(loaded_threads) per thread switch. For typical session sizes this is negligible; it could become a concern for sessions with hundreds of subagents. - **Metadata duplication**: agent nickname/role is now stored in both `AgentNavigationState` (for picker/label) and `ChatWidget::collab_agent_metadata` (for rendering). The two are kept in sync through `upsert_agent_picker_thread` and `replace_chat_widget`, but there is no compile-time enforcement of this coupling. ## Architecture ### New module: `app::loaded_threads` Pure function `find_loaded_subagent_threads_for_primary` that takes a flat list of `Thread` objects and a primary thread id, then walks the `SessionSource::SubAgent` parent-child edges to collect all transitive descendants. Returns a sorted vec of `LoadedSubagentThread` (thread_id + nickname + role). No async, no side effects -- designed for unit testing. ### New methods on `App` | Method | Purpose | |--------|---------| | `collab_receiver_thread_ids` | Extracts `receiver_thread_ids` from `ItemStarted` / `ItemCompleted` collab notifications | | `hydrate_collab_agent_metadata_for_notification` | Fetches and caches metadata for unknown receiver threads before a notification is rendered | | `backfill_loaded_subagent_threads` | Bulk-fetches all loaded threads and registers descendants of the primary thread | | `adjacent_thread_id_with_backfill` | Attempts navigation, falls back to backfill if the cache has no adjacent entry | | `replace_chat_widget` | Replaces the widget and re-seeds its metadata map from `AgentNavigationState` | ### New state in `ChatWidget` `collab_agent_metadata: HashMap<ThreadId, CollabAgentMetadata>` -- a lookup table that rendering functions consult to attach human-readable names to collab tool-call items. Populated externally by `App` via `set_collab_agent_metadata`. ### New method on `AppServerSession` `thread_loaded_list` -- thin wrapper around `ClientRequest::ThreadLoadedList`. ## Observability - `tracing::warn` on invalid thread ids during hydration and backfill. - `tracing::warn` on failed `thread/read` or `thread/loaded/list` RPCs (with thread id and error). - No new metrics or feature flags. ## Tests - **`loaded_threads::tests::finds_loaded_subagent_tree_for_primary_thread`** -- unit test for the spawn-tree walk: verifies child and grandchild are included, unrelated threads are excluded, and metadata is carried through. - **`app::tests::replace_chat_widget_reseeds_collab_agent_metadata_for_replay`** -- integration test that creates a `ChatWidget`, replaces it via `replace_chat_widget`, replays a collab wait notification, and asserts the rendered history cell contains the agent name rather than a UUID. - **Updated snapshot** `app_server_collab_wait_items_render_history` -- the existing collab wait rendering test now sets metadata before sending notifications, so the snapshot shows `Robie [explorer]` / `Ada [reviewer]` instead of raw thread ids. --------- Co-authored-by: Eric Traut <etraut@openai.com> |
||
|
Felipe Coury
|
c6ffe9abab |
fix(tui): avoid duplicate live reasoning summaries (#15758)
## TL;DR Fix duplicated reasoning summaries in `tui_app_server`. <img width="1716" height="912" alt="image" src="https://github.com/user-attachments/assets/6362f25a-ab1c-4a01-bf10-b5616c9428c2" /> During live turns, reasoning text is already rendered incrementally from `ReasoningSummaryTextDelta`. When the same reasoning item later arrives via `ItemCompleted`, we should only finalize the reasoning block, not render the same summary again. ## What changed - only replay rendered reasoning summaries from completed `ThreadItem::Reasoning` items - kept live completed reasoning items as finalize-only - added a regression test covering the live streaming + completion path ## Why Without this, the first reasoning summary often appears twice in the transcript when `model_reasoning_summary = "detailed"` and `features.tui_app_server = true`. |
||
|
Felipe Coury
|
334164a6f7 |
feat(tui): restore composer history in app-server tui (#14945)
## Problem
The app-server TUI (`tui_app_server`) lacked composer history support.
Pressing Up/Down to recall previous prompts hit a stub that logged a
warning and displayed "Not available in app-server TUI yet." New
submissions were silently dropped from the shared history file, so
nothing persisted for future sessions.
## Mental model
Codex maintains a single, append-only history file
(`$CODEX_HOME/history.jsonl`) shared across all TUI processes on the
same machine. The legacy (in-process) TUI already reads/writes this file
through `codex_core::message_history`. The app-server TUI delegates most
operations to a separate process over RPC, but history is intentionally
*not* an RPC concern — it's a client-local file.
This PR makes the app-server TUI access the same history file directly,
bypassing the app-server process entirely. The composer's Up/Down
navigation and submit-time persistence now follow the same code paths as
the legacy TUI, with the only difference being *where* the call is
dispatched (locally in `App`, rather than inside `CodexThread`).
The branch is rebuilt directly on top of `upstream/main`, so it keeps
the
existing app-server restore architecture intact.
`AppServerStartedThread`
still restores transcript history from the server `Thread` snapshot via
`thread_snapshot_events`; this PR only adds composer-history support.
## Non-goals
- Adding history support to the app-server protocol. History remains
client-local.
- Changing the on-disk format or location of `history.jsonl`.
- Surfacing history I/O errors to the user (failures are logged and
silently swallowed, matching the legacy TUI).
## Tradeoffs
| Decision | Why | Risk |
|----------|-----|------|
| Widen `message_history` from `pub(crate)` to `pub` | Avoids
duplicating file I/O logic; the module already has a clean, minimal API
surface. | Other workspace crates can now call these functions — the
contract is no longer crate-private. However, this is consistent with
recent precedent: `590cfa617` exposed `mention_syntax` for TUI
consumption, `752402c4f` exposed plugin APIs (`PluginsManager`), and
`14fcb6645`/`edacbf7b6` widened internal core APIs for other crates.
These were all narrow, intentional exposures of specific APIs — not
broad "make internals public" moves. `1af2a37ad` even went the other
direction, reducing broad re-exports to tighten boundaries. This change
follows the same pattern: a small, deliberate API surface (3 functions)
rather than a wholesale visibility change. |
| Intercept `AddToHistory` / `GetHistoryEntryRequest` in `App` before
RPC fallback | Keeps history ops out of the "unsupported op" error path
without changing app-server protocol. | This now routes through a single
`submit_thread_op` entry point, which is safer than the original
duplicated dispatch. The remaining risk is organizational: future
thread-op submission paths need to keep using that shared entry point. |
| `session_configured_from_thread_response` is now `async` | Needs
`await` on `history_metadata()` to populate real `history_log_id` /
`history_entry_count`. | Adds an async file-stat + full-file newline
scan to the session bootstrap path. The scan is bounded by
`history.max_bytes` and matches the legacy TUI's cost profile, but
startup latency still scales with file size. |
## Architecture
```
User presses Up User submits a prompt
│ │
▼ ▼
ChatComposerHistory ChatWidget::do_submit_turn
navigate_up() encode_history_mentions()
│ │
▼ ▼
AppEvent::CodexOp Op::AddToHistory { text }
(GetHistoryEntryRequest) │
│ ▼
▼ App::try_handle_local_history_op
App::try_handle_local_history_op message_history::append_entry()
spawn_blocking { │
message_history::lookup() ▼
} $CODEX_HOME/history.jsonl
│
▼
AppEvent::ThreadEvent
(GetHistoryEntryResponse)
│
▼
ChatComposerHistory::on_entry_response()
```
## Observability
- `tracing::warn` on `append_entry` failure (includes thread ID).
- `tracing::warn` on `spawn_blocking` lookup join error.
- `tracing::warn` from `message_history` internals on file-open, lock,
or parse failures.
## Tests
- `chat_composer_history::tests::navigation_with_async_fetch` — verifies
that Up emits `Op::GetHistoryEntryRequest` (was: checked for stub error
cell).
- `app::tests::history_lookup_response_is_routed_to_requesting_thread` —
verifies multi-thread composer recall routes the lookup result back to
the originating thread.
-
`app_server_session::tests::resume_response_relies_on_snapshot_replay_not_initial_messages`
— verifies app-server session restore still uses the upstream
thread-snapshot path.
-
`app_server_session::tests::session_configured_populates_history_metadata`
— verifies bootstrap sets nonzero `history_log_id` /
`history_entry_count` from the shared local history file.
|
||
|
Felipe Coury
|
43ee72a9b9 |
fix(tui): implement /mcp inventory for tui_app_server (#14931)
## Problem
The `/mcp` command did not work in the app-server TUI (remote mode). On
`main`, `add_mcp_output()` called `McpManager::effective_servers()`
in-process, which only sees locally configured servers, and then emitted
a generic stub message for the app-server to handle. In remote usage,
that left `/mcp` without a real inventory view.
## Solution
Implement `/mcp` for the app-server TUI by fetching MCP server inventory
directly from the app-server via the paginated `mcpServerStatus/list`
RPC and rendering the results into chat history.
The command now follows a three-phase lifecycle:
1. Loading: `ChatWidget::add_mcp_output()` inserts a transient
`McpInventoryLoadingCell` and emits `AppEvent::FetchMcpInventory`. This
gives immediate feedback that the command registered.
2. Fetch: `App::fetch_mcp_inventory()` spawns a background task that
calls `fetch_all_mcp_server_statuses()` over an app-server request
handle. When the RPC completes, it sends `AppEvent::McpInventoryLoaded {
result }`.
3. Resolve: `App::handle_mcp_inventory_result()` clears the loading cell
and renders either `new_mcp_tools_output_from_statuses(...)` or an error
message.
This keeps the main app event loop responsive, so the TUI can repaint
before the remote RPC finishes.
## Notes
- No `app-server` changes were required.
- The rendered inventory includes auth, tools, resources, and resource
templates, plus transport details when they are available from local
config for display enrichment.
- The app-server RPC does not expose authoritative `enabled` or
`disabled_reason` state for MCP servers, so the remote `/mcp` view no
longer renders a `Status:` row rather than guessing from local config.
- RPC failures surface in history as `Failed to load MCP inventory:
...`.
## Tests
- `slash_mcp_requests_inventory_via_app_server`
- `mcp_inventory_maps_prefix_tool_names_by_server`
- `handle_mcp_inventory_result_clears_committed_loading_cell`
- `mcp_tools_output_from_statuses_renders_status_only_servers`
- `mcp_inventory_loading_snapshot`
|
||
|
Felipe Coury
|
78e8ee4591 |
fix(tui): restore remote resume and fork history (#14930)
## Problem
When the TUI connects to a **remote** app-server (via WebSocket), resume
and fork operations lost all conversation history.
`AppServerStartedThread` carried only the `SessionConfigured` event, not
the full `Thread` snapshot. After resume or fork, the chat transcript
was empty — prior turns were silently discarded.
A secondary issue: `primary_session_configured` was not cleared on
reset, causing stale session state after reconnection.
## Approach: TUI-side only, zero app-server changes
The app-server **already returns** the full `Thread` object (with
populated `turns: Vec<Turn>`) in its `ThreadStartResponse`,
`ThreadResumeResponse`, and `ThreadForkResponse`. The data was always
there — the TUI was simply throwing it away. The old
`AppServerStartedThread` struct only kept the `SessionConfiguredEvent`,
discarding the rich turn history that the server had already provided.
This PR fixes the problem entirely within `tui_app_server` (3 files
changed, 0 changes to `app-server`, `app-server-protocol`, or any other
crate). Rather than modifying the server to send history in a different
format or adding a new endpoint, the fix preserves the existing `Thread`
snapshot and replays it through the TUI's standard event pipeline —
making restored sessions indistinguishable from live ones.
## Solution
Add a **thread snapshot replay** path. When the server hands back a
`Thread` object (on start, resume, or fork),
`restore_started_app_server_thread` converts its historical turns into
the same core `Event` sequence the TUI already processes for live
interactions, then replays them into the event store so the chat widget
renders them.
Key changes:
- **`AppServerStartedThread` now carries the full `Thread`** —
`started_thread_from_{start,resume,fork}_response` clone the thread into
the struct alongside the existing `SessionConfiguredEvent`.
- **`thread_snapshot_events()`** walks the thread's turns and items,
producing `TurnStarted` → `ItemCompleted`* →
`TurnComplete`/`TurnAborted` event sequences that the TUI already knows
how to render.
- **`restore_started_app_server_thread()`** pushes the session event +
history events into the thread channel's store, activates the channel,
and replays the snapshot — used for initial startup, resume, and fork.
- **`primary_session_configured` cleared on reset** to prevent stale
session state after reconnection.
## Tradeoffs
- **`Thread` is cloned into `AppServerStartedThread`**: The full thread
snapshot (including all historical turns) is cloned at startup. For
long-lived threads this could be large, but it's a one-time cost and
avoids lifetime gymnastics with the response.
## Tests
- `restore_started_app_server_thread_replays_remote_history` —
end-to-end: constructs a `Thread` with one completed turn, restores it,
and asserts user/agent messages appear in the transcript.
- `bridges_thread_snapshot_turns_for_resume_restore` — unit: verifies
`thread_snapshot_events` produces the correct event sequence for
completed and interrupted turns.
## Test plan
- [ ] Verify `cargo check -p codex-tui-app-server` passes
- [ ] Verify `cargo test -p codex-tui-app-server` passes
- [ ] Manual: connect to a remote app-server, resume an existing thread,
confirm history renders in the chat widget
- [ ] Manual: fork a thread via remote, confirm prior turns appear
|
||
|
Felipe Coury
|
98923e53cc |
fix(tui): decode ANSI alpha-channel encoding in syntax themes (#13382)
## Problem The `ansi`, `base16`, and `base16-256` syntax themes are designed to emit ANSI palette colors so that highlighted code respects the user's terminal color scheme. Syntect encodes this intent in the alpha channel of its `Color` struct — a convention shared with `bat` — but `convert_style` was ignoring it entirely, treating every foreground color as raw RGB. This caused ANSI-family themes to produce hard-coded RGB values (e.g. `Rgb(0x02, 0, 0)` instead of `Green`), defeating their purpose and rendering them as near-invisible dark colors on most terminals. Reported in #12890. ## Mental model Syntect themes use a compact encoding in their `Color` struct: | `alpha` | Meaning of `r` | Mapped to | |---------|----------------|-----------| | `0x00` | ANSI palette index (0–255) | `RtColor::Black`…`Gray` for 0–7, `Indexed(n)` for 8–255 | | `0x01` | Unused (sentinel) | `None` — inherit terminal default fg/bg | | `0xFF` | True RGB red channel | `RtColor::Rgb(r, g, b)` | | other | Unexpected | `RtColor::Rgb(r, g, b)` (silent fallback) | This encoding is a bat convention that three bundled themes rely on. The new `convert_syntect_color` function decodes it; `ansi_palette_color` maps indices 0–7 to ratatui's named ANSI variants. | macOS - Dark | macOS - Light | Windows - ansi | Windows - base16 | |---|---|---|---| | <img width="1064" height="1205" alt="macos-dark" src="https://github.com/user-attachments/assets/f03d92fb-b44b-4939-b2b9-503fde133811" /> | <img width="1073" height="1227" alt="macos-light" src="https://github.com/user-attachments/assets/2ecb2089-73b5-4676-bed8-e4e6794250b4" /> |  |  | ## Non-goals - Background color decoding — we intentionally skip backgrounds to preserve the terminal's own background. The decoder supports it, but `convert_style` does not apply it. - Italic/underline changes — those remain suppressed as before. - Custom `.tmTheme` support for ANSI encoding — only the bundled themes use this convention. ## Tradeoffs - The alpha-channel encoding is an undocumented bat/syntect convention, not a formal spec. We match bat's behavior exactly, trading formality for ecosystem compatibility. - Indices 0–7 are mapped to ratatui's named variants (`Black`, `Red`, …, `Gray`) rather than `Indexed(0)`…`Indexed(7)`. This lets terminals apply bold/bright semantics to named colors, which is the expected behavior for ANSI themes, but means the two representations are not perfectly round-trippable. ## Architecture All changes are in `codex-rs/tui/src/render/highlight.rs`, within the style-conversion layer between syntect and ratatui: ``` syntect::highlighting::Color └─ convert_syntect_color(color) [NEW — alpha-dispatch] ├─ a=0x00 → ansi_palette_color() [NEW — index→named/indexed] ├─ a=0x01 → None (terminal default) ├─ a=0xFF → Rgb(r,g,b) (standard opaque path) └─ other → Rgb(r,g,b) (silent fallback) ``` `convert_style` delegates foreground mapping to `convert_syntect_color` instead of inlining the `Rgb(r,g,b)` conversion. The core highlighter is refactored into `highlight_to_line_spans_with_theme` (accepts an explicit theme reference) so tests can highlight against specific themes without mutating process-global state. ### ANSI-family theme contract The ANSI-family themes (`ansi`, `base16`, `base16-256`) rely on upstream alpha-channel encoding from two_face/syntect. We intentionally do **not** validate this contract at runtime — if the upstream format changes, the `ansi_themes_use_only_ansi_palette_colors` test catches it at build time, long before it reaches users. A runtime warning would be unactionable noise. ### Warning copy cleanup User-facing warning messages were rewritten for clarity: - Removed internal jargon ("alpha-encoded ANSI color markers", "RGB fallback semantics", "persisted override config") - Dropped "syntax" prefix from "syntax theme" — users just think "theme" - Downgraded developer-only diagnostics (duplicate override, resolve fallback) from `warn` to `debug` ## Observability - The `ansi_themes_use_only_ansi_palette_colors` test enforces the ANSI-family contract at build time. - The snapshot test provides a regression tripwire for palette color output. - User-facing warnings are limited to actionable issues: unknown theme names and invalid custom `.tmTheme` files. ## Tests - **Unit tests for each alpha branch:** `alpha=0x00` with low index (named color), `alpha=0x00` with high index (`Indexed`), `alpha=0x01` (terminal default), unexpected alpha (falls back to RGB), ANSI white → Gray mapping. - **Integration test:** `ansi_family_themes_use_terminal_palette_colors_not_rgb` — highlights a Rust snippet with each ANSI-family theme and asserts zero `Rgb` foreground colors appear. - **Snapshot test:** `ansi_family_foreground_palette_snapshot` — records the exact set of unique foreground colors each ANSI-family theme produces, guarding against regressions. - **Warning validation tests:** verify user-facing warnings for missing custom themes, invalid `.tmTheme` files, and bundled theme resolution. ## Test plan - [ ] `cargo test -p codex-tui` passes all new and existing tests - [ ] Select `ansi`, `base16`, or `base16-256` theme and verify code blocks render with terminal palette colors (not near-black RGB) - [ ] Select a standard RGB theme (e.g. `dracula`) and verify no regression in color output |
||
|
Felipe Coury
|
745c48b088 |
fix(core): scope file search gitignore to repository context (#13250)
Closes #3493
## Problem
When a user's home directory (or any ancestor) contains a broad
`.gitignore` (e.g. `*` + `!.gitignore`), the `@` file mention picker in
Codex silently hides valid repository files like `package.json`. The
picker returns `no matches` for searches that should succeed. This is
surprising because manually typed paths still work, making the failure
hard to diagnose.
## Mental model
Git itself never walks above the repository root to assemble its ignore
list. Its `.gitignore` resolution is strictly scoped: it reads
`.gitignore` files from the repo root downward, the per-repo
`.git/info/exclude`, and the user's global excludes file (via
`core.excludesFile`). A `.gitignore` sitting in a parent directory above
the repo root has no effect on `git status`, `git ls-files`, or any
other git operation. Our file search should replicate this contract
exactly.
The `ignore` crate's `WalkBuilder` has a `require_git` flag that
controls whether it follows this contract:
- `require_git(false)` (the previous setting): the walker reads
`.gitignore` files from _all_ ancestor directories, even those above or
outside the repository root. This is a deliberate divergence from git's
behavior in the `ignore` crate, intended for non-git use cases. It means
a `~/.gitignore` with `*` will suppress every file in the walk—something
git itself would never do.
- `require_git(true)` (this fix): the walker only applies `.gitignore`
semantics when it detects a `.git` directory, scoping ignore resolution
to the repository boundary. This matches git's own behavior: parent
`.gitignore` files above the repo root have no effect.
The fix is a one-line change: `require_git(false)` becomes
`require_git(true)`.
## How `require_git(false)` got here
The setting was introduced in
|
||
|
Felipe Coury
|
c3c75878e8 |
fix(tui): theme-aware diff backgrounds with fallback behavior (#13037)
## Problem The TUI diff renderer uses hardcoded background palettes for insert/delete lines that don't respect the user's chosen syntax theme. When a theme defines `markup.inserted` / `markup.deleted` scope backgrounds (the convention used by GitHub, Solarized, Monokai, and most VS Code themes), those colors are ignored — the diff always renders with the same green/red tints regardless of theme selection. Separately, ANSI-16 terminals (and Windows Terminal sessions misreported as ANSI-16) rendered diff backgrounds as full-saturation blocks that obliterated syntax token colors, making highlighted diffs unreadable. ## Mental model Diff backgrounds are resolved in three layers: 1. **Color level detection** — `diff_color_level_for_terminal()` maps the raw `supports-color` probe + Windows Terminal heuristics to a `DiffColorLevel` (TrueColor / Ansi256 / Ansi16). Windows Terminal gets promoted from Ansi16 to TrueColor when `WT_SESSION` is present. 2. **Background resolution** — `resolve_diff_backgrounds()` queries the active syntax theme for `markup.inserted`/`markup.deleted` (falling back to `diff.inserted`/`diff.deleted`), then overlays those on top of the hardcoded palette. For ANSI-256, theme RGB values are quantized to the nearest xterm-256 index. For ANSI-16, backgrounds are `None` (foreground-only). 3. **Style composition** — The resolved `ResolvedDiffBackgrounds` is threaded through every call to `style_add`, `style_del`, `style_sign_*`, and `style_line_bg_for`, which decide how to compose foreground+background for each line kind and theme variant. A new `RichDiffColorLevel` type (a subset of `DiffColorLevel` without Ansi16) encodes the invariant "we have enough depth for tinted backgrounds" at the type level, so background-producing functions have exhaustive matches without unreachable arms. ## Non-goals - No change to gutter (line number column) styling — gutter backgrounds still use the hardcoded palette. - No per-token scope background resolution — this is line-level background only; syntax token colors come from the existing `highlight_code_to_styled_spans` path. - No dark/light theme auto-switching from scope backgrounds — `DiffTheme` is still determined by querying the terminal's background color. ## Tradeoffs - **Theme trust vs. visual safety:** When a theme defines scope backgrounds, we trust them unconditionally for rich color levels. A badly authored theme could produce illegible combinations. The fallback for `None` backgrounds (foreground-only) is intentionally conservative. - **Quantization quality:** ANSI-256 quantization uses perceptual distance across indices 16–255, skipping system colors. The result is approximate — a subtle theme tint may land on a noticeably different xterm index. - **Single-query caching:** `resolve_diff_backgrounds` is called once per `render_change` invocation (i.e., once per file in a diff). If the theme changes mid-render (live preview), the next file picks up the new backgrounds. ## Architecture Files changed: | File | Role | |---|---| | `tui/src/render/highlight.rs` | New: `DiffScopeBackgroundRgbs`, `diff_scope_background_rgbs()`, scope extraction helpers | | `tui/src/diff_render.rs` | New: `RichDiffColorLevel`, `ResolvedDiffBackgrounds`, `resolve_diff_backgrounds*`, `quantize_rgb_to_ansi256`, Windows Terminal promotion; modified: all style helpers to accept/thread `ResolvedDiffBackgrounds` | The scope-extraction code lives in `highlight.rs` because it uses `syntect::highlighting::Highlighter` and the theme singleton. The resolution and quantization logic lives in `diff_render.rs` because it depends on diff-specific types (`DiffTheme`, `DiffColorLevel`, ratatui `Color`). ## Observability No runtime logging was added. The most useful debugging aid is the `diff_color_level_for_terminal` function, which is pure and fully unit-tested — to diagnose a color-depth mismatch, log its four inputs (`StdoutColorLevel`, `TerminalName`, `WT_SESSION` presence, `FORCE_COLOR` presence). Scope resolution can be tested by loading a custom `.tmTheme` with known `markup.inserted` / `markup.deleted` backgrounds and checking the diff output in a truecolor terminal. ## Tests - **Windows Terminal promotion:** 7 unit tests cover every branch of `diff_color_level_for_terminal` (ANSI-16 promotion, `WT_SESSION` unconditional promotion, `FORCE_COLOR` suppression, conservative `Unknown` level). - **ANSI-16 foreground-only:** Tests verify that `style_add`, `style_del`, `style_sign_*`, `style_line_bg_for`, and `style_gutter_for` all return `None` backgrounds on ANSI-16. - **Scope resolution:** Tests verify `markup.*` preference over `diff.*`, `None` when no scope matches, bundled theme resolution, and custom `.tmTheme` round-trip. - **Quantization:** Test verifies ANSI-256 quantization of a known RGB triple. - **Insta snapshots:** 2 new snapshot tests (`ansi16_insert_delete_no_background`, `theme_scope_background_resolution`) lock visual output. |
||
|
Felipe Coury
|
3b5996f988 |
fix(tui): promote windows terminal diff ansi16 to truecolor (#13016)
## Summary - Promote ANSI-16 to truecolor for diff rendering when running inside Windows Terminal - Respect explicit `FORCE_COLOR` override, skipping promotion when set - Extract a pure `diff_color_level_for_terminal` function for testability - Strip background tints from ANSI-16 diff output, rendering add/delete lines with foreground color only - Introduce `RichDiffColorLevel` to type-safely restrict background fills to truecolor and ansi256 ## Problem Windows Terminal fully supports 24-bit (truecolor) rendering but often does not provide the usual TERM metadata (`TERM`, `TERM_PROGRAM`, `COLORTERM`) in `cmd.exe`/PowerShell sessions. In those environments, `supports-color` can report only ANSI-16 support. The diff renderer therefore falls back to a 16-color palette, producing washed-out, hard-to-read diffs. The screenshots below demonstrate that both PowerShell and cmd.exe don't set any `*TERM*` environment variables. | PowerShell | cmd.exe | |---|---| | <img width="2032" height="1162" alt="SCR-20260226-nfvy" src="https://github.com/user-attachments/assets/59e968cc-4add-4c7b-a415-07163297e86a" /> | <img width="2032" height="1162" alt="SCR-20260226-nfyc" src="https://github.com/user-attachments/assets/d06b3e39-bf91-4ce3-9705-82bf9563a01b" /> | ## Mental model `StdoutColorLevel` (from `supports-color`) is the _detected_ capability. `DiffColorLevel` is the _intended_ capability for diff rendering. A new intermediary — `diff_color_level_for_terminal` — maps one to the other and is the single place where terminal-specific overrides live. Windows Terminal is detected two independent ways: the `TerminalName` parsed by `terminal_info()` and the raw presence of `WT_SESSION`. When `WT_SESSION` is present and `FORCE_COLOR` is not set, we promote unconditionally to truecolor. When `WT_SESSION` is absent but `TerminalName::WindowsTerminal` is detected, we promote only the ANSI-16 level (not `Unknown`). A single override helper — `has_force_color_override()` — checks whether `FORCE_COLOR` is set. When it is, both the `WT_SESSION` fast-path and the `TerminalName`-based promotion are suppressed, preserving explicit user intent. | PowerShell | cmd.exe | WSL | Bash for Windows | |---|---|---|---| |  |  |  |  | ## Non-goals - This does not change color detection for anything outside the diff renderer (e.g. the chat widget, markdown rendering). - This does not add a user-facing config knob; `FORCE_COLOR` already serves that role. ## Tradeoffs - The `has_wt_session` signal is intentionally kept separate from `TerminalName::WindowsTerminal`. `terminal_info()` is derived with `TERM_PROGRAM` precedence, so it can differ from raw `WT_SESSION`. - Real-world validation in this issue: in both `cmd.exe` and PowerShell, `TERM`/`TERM_PROGRAM`/`COLORTERM` were absent, so TERM-based capability hints were unavailable in those sessions. - Checking `FORCE_COLOR` for presence rather than parsing its value is a simplification. In practice `supports-color` has already parsed it, so our check is a coarse "did the user set _anything_?" gate. The effective color level still comes from `supports-color`. - When `WT_SESSION` is present without `FORCE_COLOR`, we promote to truecolor regardless of `stdout_level` (including `Unknown`). This is aggressive but correct: `WT_SESSION` is a strong signal that we're in Windows Terminal. - ANSI-16 add/delete backgrounds (bright green/red) overpower syntax-highlighted token colors, making diffs harder to read. Foreground-only cues (colored text, gutter signs) preserve readability on low-color terminals. ## Architecture ``` stdout_color_level() ──┐ terminal_info().name ──┤ WT_SESSION presence ──┼──▶ diff_color_level_for_terminal() ──▶ DiffColorLevel FORCE_COLOR presence ──┘ │ ▼ RichDiffColorLevel::from_diff_color_level() │ ┌──────────┴──────────┐ │ Some(TrueColor|256) │ → bg tints │ None (Ansi16) │ → fg only └─────────────────────┘ ``` `diff_color_level()` is the environment-reading entry point; it gathers the four runtime signals and delegates to the pure, testable `diff_color_level_for_terminal()`. ## Observability No new logs or metrics. Incorrect color selection is immediately visible as broken diff rendering; the test suite covers the decision matrix exhaustively. ## Tests Six new unit tests exercise every branch of `diff_color_level_for_terminal`: | Test | Inputs | Expected | |------|--------|----------| | `windows_terminal_promotes_ansi16_to_truecolor_for_diffs` | Ansi16 + WindowsTerminal name | TrueColor | | `wt_session_promotes_ansi16_to_truecolor_for_diffs` | Ansi16 + WT_SESSION only | TrueColor | | `non_windows_terminal_keeps_ansi16_diff_palette` | Ansi16 + WezTerm | Ansi16 | | `wt_session_promotes_unknown_color_level_to_truecolor` | Unknown + WT_SESSION | TrueColor | | `explicit_force_override_keeps_ansi16_on_windows_terminal` | Ansi16 + WindowsTerminal + FORCE_COLOR | Ansi16 | | `explicit_force_override_keeps_ansi256_on_windows_terminal` | Ansi256 + WT_SESSION + FORCE_COLOR | Ansi256 | | `ansi16_add_style_uses_foreground_only` | Dark + Ansi16 | fg=Green, bg=None | | (and any other new snapshot/assertion tests from commits |
||
|
Felipe Coury
|
061d1d3b5e |
feat(tui): add theme-aware diff backgrounds with capability-graded palettes (#12581)
## Problem Diff lines used only foreground colors (green/red) with no background tinting, making them hard to scan. The gutter (line numbers) also had no theme awareness — dimmed text was fine on dark terminals but unreadable on light ones. ## Mental model Each diff line now has four styled layers: **gutter** (line number), **sign** (`+`/`-`), **content** (text), and **line background** (full terminal width). A `DiffTheme` enum (`Dark` / `Light`) is selected once per render by probing the terminal's queried background via `default_bg()`. A companion `DiffColorLevel` enum (`TrueColor` / `Ansi256` / `Ansi16`) is derived from `stdout_color_level()` and gates which palette is used. All style helpers dispatch on `(theme, DiffLineType, color_level)` to pick the right colors. | Theme Picker Wide | Theme Picker Narrow | |---|---| | <img width="1552" height="1012" alt="image" src="https://github.com/user-attachments/assets/231b21b7-32d4-4727-80ed-7d01924954be" /> | <img width="795" height="1012" alt="image" src="https://github.com/user-attachments/assets/549cacdf-daec-43c9-ad64-2a28d16d140e" /> | | Dark BG - 16 colors | Dark BG - 256 colors | Dark BG - True Colors | |---|---|---| | <img width="1552" height="1012" alt="dark-16colors" src="https://github.com/user-attachments/assets/fba36de3-c101-47d4-9e63-88cdd00410d0" /> | <img width="1552" height="1012" alt="dark-256colors" src="https://github.com/user-attachments/assets/f39e4307-c6b0-49c4-b4fe-bd26d3d8e41c" /> | <img width="1552" height="1012" alt="dark-truecolor" src="https://github.com/user-attachments/assets/1af4ec57-04bf-4dfb-8a44-0ab5e5aaaf18" /> | | Light BG - 16 colors | Light BG - 256 colors | Light BG - True Colors | |---|---|---| | <img width="1552" height="1012" alt="light-16colors" src="https://github.com/user-attachments/assets/2b5423d1-74b4-4b1e-8123-7c2488ff436b" /> | <img width="1552" height="1012" alt="light-256colors" src="https://github.com/user-attachments/assets/c94cff9a-8d3e-42c9-bbe7-079da39953a8" /> | <img width="1552" height="1012" alt="light-truecolor" src="https://github.com/user-attachments/assets/f73da626-725f-4452-99ee-69ef706df2c6" /> | ## Non-goals - No runtime theme switching beyond what `default_bg()` already provides. - No change to syntax highlighting theme selection or the highlight module. ## Tradeoffs - Three fixed palettes (truecolor RGB, 256-color indexed, 16-color named) are maintained rather than using `best_color` nearest-match. This is deliberate: `supports_color::on_cached(Stream::Stdout)` can misreport capabilities once crossterm enters the alternate screen, so hand-picked palette entries give better visual results than automatic quantization. - Delete lines in the syntax-highlighted path get `Modifier::DIM` to visually recede compared to insert lines. This trades some readability of deleted code for scan-ability of additions. - The theme picker's diff preview sets `preserve_side_content_bg: true` on `ListSelectionView` so diff background tints survive into the side panel. Other popups keep the default (`false`) to preserve their reset-background look. ## Architecture - **Color constants** are module-level `const` items grouped by palette tier: `DARK_TC_*` / `LIGHT_TC_*` (truecolor RGB tuples), `DARK_256_*` / `LIGHT_256_*` (xterm indexed), with named `Color` variants used for the 16-color tier. - **`DiffTheme`** is a private enum; `diff_theme()` probes the terminal and `diff_theme_for_bg()` is the testable pure-function version. - **`DiffColorLevel`** is a private enum derived from `StdoutColorLevel` via `diff_color_level()`. - **Palette helpers** (`add_line_bg`, `del_line_bg`, `light_gutter_fg`, `light_add_num_bg`, `light_del_num_bg`) each take `(DiffTheme, DiffColorLevel)` or just `DiffColorLevel` and return a `Color`. - **Style helpers** (`style_line_bg_for`, `style_gutter_for`, `style_sign_add`, `style_sign_del`, `style_add`, `style_del`) each take `(DiffLineType, DiffTheme, DiffColorLevel)` or `(DiffTheme, DiffColorLevel)` and return a `Style`. - **`push_wrapped_diff_line_inner_with_theme_and_color_level`** is the innermost renderer, accepting both theme and color level so tests can exercise any combination without depending on the terminal. - **Line-level background** is applied via `RtLine::from(...).style(line_bg)` so the tint extends across the full terminal width, not just the text content. - **Theme picker integration**: `ListSelectionView` gained a `preserve_side_content_bg` flag. When `true`, the side panel skips `force_bg_to_terminal_bg`, letting diff preview backgrounds render faithfully. ## Observability No new logging. Theme selection is deterministic from `default_bg()`, which is already queried and cached at TUI startup. ## Tests 1. **`DiffTheme` is determined per `render_change` call** — if `default_bg()` changes mid-render (e.g. `requery_default_colors()` fires), different file chunks could render with different themes. Low risk in practice since re-query only happens on explicit user action. 2. **16-color tier uses named `Color` variants** (`Color::Green`, `Color::Red`, etc.) which the terminal maps to its own palette. On unusual terminal themes these could clash with the background. Acceptable since 16-color terminals already have unpredictable color rendering. 3. **Light-theme `style_add` / `style_del` set bg but no fg** — on light terminals, non-syntax-highlighted content uses the terminal's default foreground against a pastel background. If the terminal's default fg happens to be very light, contrast could suffer. This is an edge case since light-terminal users typically have dark default fg. 4. **`preserve_side_content_bg` is a general-purpose flag but only used by the theme picker** — if other popups start using side content with intentional backgrounds they'll need to opt in explicitly. Not a real risk today, just a note for future callers. |
||
|
Felipe Coury
|
48e08a1561 |
fix(tui): recover on owned wrap mapping mismatch (#12609)
## Summary - Replace the `panic!` in `map_owned_wrapped_line_to_range` with a recoverable flow that skips synthetic leading characters, logs a warning on mid-line mismatch, and returns the mapped prefix range instead of crashing - Fixes a crash when `textwrap` produces owned lines with synthetic indent prefixes (e.g. non-space indents via `initial_indent`/`subsequent_indent`) ## Test plan - [x] Added unit test for direct mismatch recovery (`map_owned_wrapped_line_to_range_recovers_on_non_prefix_mismatch`) - [x] Added end-to-end `wrap_ranges` test with non-space indents that forces owned wrapped lines and validates full source reconstruction - [x] Verify no regressions in existing `wrapping.rs` tests (`cargo test -p codex-tui`) |
||
|
Felipe Coury
|
c4f1af7a86 |
feat(tui): syntax highlighting via syntect with theme picker (#11447)
## Summary Adds syntax highlighting to the TUI for fenced code blocks in markdown responses and file diffs, plus a `/theme` command with live preview and persistent theme selection. Uses syntect (~250 grammars, 32 bundled themes, ~1 MB binary cost) — the same engine behind `bat`, `delta`, and `xi-editor`. Includes guardrails for large inputs, graceful fallback to plain text, and SSH-aware clipboard integration for the `/copy` command. <img width="1554" height="1014" alt="image" src="https://github.com/user-attachments/assets/38737a79-8717-4715-b857-94cf1ba59b85" /> <img width="2354" height="1374" alt="image" src="https://github.com/user-attachments/assets/25d30a00-c487-4af8-9cb6-63b0695a4be7" /> ## Problem Code blocks in the TUI (markdown responses and file diffs) render without syntax highlighting, making it hard to scan code at a glance. Users also have no way to pick a color theme that matches their terminal aesthetic. ## Mental model The highlighting system has three layers: 1. **Syntax engine** (`render::highlight`) -- a thin wrapper around syntect + two-face. It owns a process-global `SyntaxSet` (~250 grammars) and a `RwLock<Theme>` that can be swapped at runtime. All public entry points accept `(code, lang)` and return ratatui `Span`/`Line` vectors or `None` when the language is unrecognized or the input exceeds safety guardrails. 2. **Rendering consumers** -- `markdown_render` feeds fenced code blocks through the engine; `diff_render` highlights Add/Delete content as a whole file and Update hunks per-hunk (preserving parser state across hunk lines). Both callers fall back to plain unstyled text when the engine returns `None`. 3. **Theme lifecycle** -- at startup the config's `tui.theme` is resolved to a syntect `Theme` via `set_theme_override`. At runtime the `/theme` picker calls `set_syntax_theme` to swap themes live; on cancel it restores the snapshot taken at open. On confirm it persists `[tui] theme = "..."` to config.toml. ## Non-goals - Inline diff highlighting (word-level change detection within a line). - Semantic / LSP-backed highlighting. - Theme authoring tooling; users supply standard `.tmTheme` files. ## Tradeoffs | Decision | Upside | Downside | | ------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | | syntect over tree-sitter / arborium | ~1 MB binary increase for ~250 grammars + 32 themes; battle-tested crate powering widely-used tools (`bat`, `delta`, `xi-editor`). tree-sitter would add ~12 MB for 20-30 languages or ~35 MB for full coverage. | Regex-based; less structurally accurate than tree-sitter for some languages (e.g. language injections like JS-in-HTML). | | Global `RwLock<Theme>` | Enables live `/theme` preview without threading Theme through every call site | Lock contention risk (mitigated: reads vastly outnumber writes, single UI thread) | | Skip background / italic / underline from themes | Terminal BG preserved, avoids ugly rendering on some themes | Themes that rely on these properties lose fidelity | | Guardrails: 512 KB / 10k lines | Prevents pathological stalls on huge diffs or pastes | Very large files render without color | ## Architecture ``` config.toml ─[tui.theme]─> set_theme_override() ─> THEME (RwLock) │ ┌───────────────────────────────────────────┘ │ markdown_render ─── highlight_code_to_lines(code, lang) ─> Vec<Line> diff_render ─── highlight_code_to_styled_spans(code, lang) ─> Option<Vec<Vec<Span>>> │ │ (None ⇒ plain text fallback) │ /theme picker ─── set_syntax_theme(theme) // live preview swap ─── current_syntax_theme() // snapshot for cancel ─── resolve_theme_by_name(name) // lookup by kebab-case ``` Key files: - `tui/src/render/highlight.rs` -- engine, theme management, guardrails - `tui/src/diff_render.rs` -- syntax-aware diff line wrapping - `tui/src/theme_picker.rs` -- `/theme` command builder - `tui/src/bottom_pane/list_selection_view.rs` -- side content panel, callbacks - `core/src/config/types.rs` -- `Tui::theme` field - `core/src/config/edit.rs` -- `syntax_theme_edit()` helper ## Observability - `tracing::warn` when a configured theme name cannot be resolved. - `Config::startup_warnings` surfaces the same message as a TUI banner. - `tracing::error` when persisting theme selection fails. ## Tests - Unit tests in `highlight.rs`: language coverage, fallback behavior, CRLF stripping, style conversion, guardrail enforcement, theme name mapping exhaustiveness. - Unit tests in `diff_render.rs`: snapshot gallery at multiple terminal sizes (80x24, 94x35, 120x40), syntax-highlighted wrapping, large-diff guardrail, rename-to-different-extension highlighting, parser state preservation across hunk lines. - Unit tests in `theme_picker.rs`: preview rendering (wide + narrow), dim overlay on deletions, subtitle truncation, cancel-restore, fallback for unavailable configured theme. - Unit tests in `list_selection_view.rs`: side layout geometry, stacked fallback, buffer clearing, cancel/selection-changed callbacks. - Integration test in `lib.rs`: theme warning uses the final (post-resume) config. ## Cargo Deny: Unmaintained Dependency Exceptions This PR adds two `cargo deny` advisory exceptions for transitive dependencies pulled in by `syntect v5.3.0`: | Advisory | Crate | Status | |----------|-------|--------| | RUSTSEC-2024-0320 | `yaml-rust` | Unmaintained (maintainer unreachable) | | RUSTSEC-2025-0141 | `bincode` | Unmaintained (development ceased; v1.3.3 considered complete) | **Why this is safe in our usage:** - Neither advisory describes a known security vulnerability. Both are "unmaintained" notices only. - `bincode` is used by syntect to deserialize pre-compiled syntax sets. Again, these are **static vendored artifacts** baked into the binary at build time. No user-supplied bincode data is ever deserialized. - Attack surface is zero for both crates; exploitation would require a supply-chain compromise of our own build artifacts. - These exceptions can be removed when syntect migrates to `yaml-rust2` and drops `bincode`, or when alternative crates are available upstream. |
||
|
Felipe Coury
|
2ba2c57af4 |
fix(tui): preserve URL clickability across all TUI views (#12067)
## Problem Long URLs containing `/` and `-` characters are split across multiple terminal lines by `textwrap`'s default hyphenation rules. This breaks terminal link detection: emulators can no longer identify the URL as clickable, and copy-paste yields a truncated fragment. The issue affects every view that renders user or agent text — exec output, history cells, markdown, the app-link setup screen, and the VT100 scrollback path. A secondary bug compounds the first: `desired_height()` calculations count logical lines rather than viewport rows. When a URL overflows its line and wraps visually, the height budget is too small, causing content to clip or leave gaps. Here is how the complete URL is interpreted by the terminal before (first line only) and after (complete URL): | Before | After | |---|---| | <img width="777" height="1002" alt="Screenshot 2026-02-17 at 7 59 11 PM" src="https://github.com/user-attachments/assets/193a89a0-7e56-49c5-8b76-53499a76e7e3" /> | <img width="777" height="1002" alt="Screenshot 2026-02-17 at 7 58 40 PM" src="https://github.com/user-attachments/assets/0b9b4c14-aafb-439f-9ffe-f6bba556f95e" /> | ## Mental model The TUI now treats URL-like tokens as atomic units that must never be split by the wrapping engine. Every call site that previously used `word_wrap_*` has been migrated to `adaptive_wrap_*`, which inspects each line for URL-like tokens and switches wrapping strategy accordingly: - **Non-URL lines** follow the existing `textwrap` path unchanged (word boundaries, optional indentation, hyphenation). - **URL-only lines** (with at most decorative markers like `│`, `-`, `1.`) are emitted unwrapped so terminal link detection works; ratatui's `Wrap { trim: false }` handles the final character wrap at render time. - **Mixed lines** (URL + substantive non-URL prose) flow through `adaptive_wrap_line` so prose wraps naturally at word boundaries while URL tokens remain unsplit. Height measurement everywhere now delegates to `Paragraph::line_count(width)`, which accounts for the visual row cost of overflowed lines. This single source of truth replaces ad-hoc line counting in individual cells. For terminal scrollback (the VT100 path that prints history when the TUI exits), URL-only lines are emitted unwrapped so the terminal's own link detector can find them. Mixed URL+prose lines use adaptive wrapping so surrounding text wraps naturally. Continuation rows are pre-cleared to avoid stale content artifacts. ## Non-goals - Full RFC 3986 URL parsing. The detector is a conservative heuristic that covers `scheme://host`, bare domains (`example.com/path`), `localhost:port`, and IPv4 hosts. IPv6 (`[::1]:8080`) and exotic schemes are intentionally excluded from v1. - Changing wrapping behavior for non-URL content. - Reflowing or reformatting existing terminal scrollback on resize. ## Tradeoffs | Decision | Upside | Downside | |----------|--------|----------| | Heuristic URL detection vs. full parser | Fast, zero-alloc on the hot path; conservative enough to reject file paths like `src/main.rs` | False negatives on obscure URL formats (they get split as before) | | Adaptive (three-path) wrapping | Non-URL lines are untouched — no behavior change, no perf cost; mixed lines wrap prose naturally while preserving URLs | Three wrapping strategies to reason about when debugging layout | | Row-based truncation with line-unit ellipsis | Accurate viewport budget; stable "N lines omitted" count across terminal widths | `truncate_lines_middle` is more complex (must compute per-line row cost) | | Unwrapped URL-only lines in scrollback | Terminal emulators detect clickable links; copy-paste gets the full URL | TUI and scrollback formatting diverge for URL-only lines | | Default `desired_height` via `Paragraph::line_count` | DRY — most cells inherit correct measurement | Cells with custom layout must remember to override | ## Architecture ```mermaid flowchart TD A["adaptive_wrap_*()"] --> B{"line_contains_url_like?"} B -- No URL tokens --> C["word_wrap_line<br/>(textwrap default)"] B -- Has URL tokens --> D{"mixed URL + prose?"} D -- "URL-only<br/>(+ decorative markers)" --> E["emit unwrapped<br/>(terminal char-wraps)"] D -- "Mixed<br/>(URL + substantive text)" --> F["adaptive_wrap_line<br/>(AsciiSpace + custom WordSplitter)"] C --> G["Paragraph::line_count(w)<br/>(single height truth)"] E --> G F --> G ``` **Changed files:** | File | Role | |------|------| | `wrapping.rs` | URL detection heuristics, mixed-line detection, `adaptive_wrap_*` functions, custom `WordSplitter` | | `exec_cell/render.rs` | Row-aware `truncate_lines_middle`, adaptive wrapping for command/output display | | `history_cell.rs` | Migrate all cell types to `adaptive_wrap_*`; default `desired_height` via `Paragraph::line_count` | | `insert_history.rs` | Three-path scrollback wrapping (unwrapped URL-only, adaptive mixed, word-wrapped text); continuation row clearing | | `app_link_view.rs` | Adaptive wrapping for setup URL; `desired_height` via `Paragraph::line_count` | | `markdown_render.rs` | Adaptive wrapping in `finish_paragraph` | | `model_migration.rs` | Viewport-aware wrapping for narrow-pane markdown | | `pager_overlay.rs` | `Wrap { trim: false }` for transcript and streaming chunks | | `queued_user_messages.rs` | Migrate to `adaptive_wrap_lines` | | `status/card.rs` | Migrate to `adaptive_wrap_lines` | ## Observability - **Ellipsis message** in truncated exec output reports omitted count in logical lines (stable across resize) rather than viewport rows (fluctuates). - URL detection is deterministic and stateless — no hidden caching or memoization to go stale. - Height mismatch bugs surface immediately as visual clipping or gaps; the `Paragraph::line_count` path is the same code ratatui uses at render time, so measurement and rendering cannot diverge. ## Tests 26 new unit tests across 7 files, covering: - **URL integrity**: assert a URL-like token appears on exactly one rendered line (not split across two). - **Height accuracy**: compare `desired_height()` against `Paragraph::line_count()` for URL-containing content. - **Row-aware truncation**: verify ellipsis counts logical lines and output fits within the row budget. - **Scrollback rendering**: VT100 backend tests confirm prefix and URL land on the same row; continuation rows are cleared; mixed URL+prose lines wrap prose while preserving URL tokens. - **Mixed URL+prose detection**: `line_has_mixed_url_and_non_url_tokens` correctly distinguishes lines with substantive non-URL text from lines with only decorative markers alongside a URL. - **Heuristic correctness**: positive matches (`https://...`, `example.com/path`, `localhost:3000/api`, `192.168.1.1:8080/health`) and negative matches (`src/main.rs`, `foo/bar`, `hello-world`). ## Risks and open items 1. **URL-like tokens in code output** (e.g. `example.com/api` inside a JSON blob) will trigger URL-preserving wrap on that line. This is acceptable — the worst case is a slightly wider line, not broken output. 2. **Very long non-URL tokens on a URL line** can only break at character boundaries (the custom splitter emits all char indices for non-URL words). On extremely narrow terminals this could overflow, but narrow terminals already degrade gracefully. 3. **No IPv6 support** — `[::1]:8080/path` will be treated as a non-URL and may get split. Can be added later without API changes. Fixes #5457 |
||
|
Felipe Coury
|
a5d0757ed1 |
fix(tui): queued-message edit shortcut unreachable in some terminals (#12240)
## Problem The TUI's "edit queued message" shortcut (Alt+Up) is either silently swallowed or recognized as another key combination by Apple Terminal, Warp, and VSCode's integrated terminal on macOS. Users in those environments see the hint but pressing the keys does nothing. ## Mental model When a model turn is in progress the user can still type follow-up messages. These are queued and displayed below the composer with a hint line showing how to pop the most recent one back into the editor. The hint text and the actual key handler must agree on which shortcut is used, and that shortcut must actually reach the TUI—i.e. it must not be intercepted by the host terminal. Three terminals are known to intercept Alt+Up: Apple Terminal (remaps it to cursor movement), Warp (consumes it for its own command palette), and VSCode (maps it to "move line up"). For these we use Shift+Left instead. <p align="center"> <img width="283" height="182" alt="image" src="https://github.com/user-attachments/assets/4a9c5d13-6e47-4157-bb41-28b4ce96a914" /> </p> | macOS Native Terminal | Warp | VSCode Terminal | |---|---|---| | <img width="1557" height="1010" alt="SCR-20260219-kigi" src="https://github.com/user-attachments/assets/f4ff52f8-119e-407b-a3f3-52f564c36d70" /> | <img width="1479" height="1261" alt="SCR-20260219-krrf" src="https://github.com/user-attachments/assets/5807d7c4-17ae-4a2b-aa27-238fd49d90fd" /> | <img width="1612" height="1312" alt="SCR-20260219-ksbz" src="https://github.com/user-attachments/assets/1cedb895-6966-4d63-ac5f-0eea0f7057e8" /> | ## Non-goals - Making the binding user-configurable at runtime (deferred to a broader keybinding-config effort). - Remapping any other shortcuts that might be terminal-specific. ## Tradeoffs - **Exhaustive match instead of a wildcard default.** The `queued_message_edit_binding_for_terminal` function explicitly lists every `TerminalName` variant. This is intentional: adding a new terminal to the enum will produce a compile error, forcing the author to decide which binding that terminal should use. - **Binding lives on `ChatWidget`, hint lives on `QueuedUserMessages`.** The key event handler that actually acts on the press is in `ChatWidget`, but the rendered hint text is inside `QueuedUserMessages`. These are kept in sync by `ChatWidget` calling `bottom_pane.set_queued_message_edit_binding(self.queued_message_edit_binding)` during construction. A mismatch would show the wrong hint but would not lose data. ## Architecture ```mermaid graph TD TI["terminal_info().name"] --> FN["queued_message_edit_binding_for_terminal(name)"] FN --> KB["KeyBinding"] KB --> CW["ChatWidget.queued_message_edit_binding<br/><i>key event matching</i>"] KB --> BP["BottomPane.set_queued_message_edit_binding()"] BP --> QUM["QueuedUserMessages.edit_binding<br/><i>rendered in hint line</i>"] subgraph "Special terminals (Shift+Left)" AT["Apple Terminal"] WT["Warp"] VS["VSCode"] end subgraph "Default (Alt+Up)" GH["Ghostty"] IT["iTerm2"] OT["Others…"] end AT --> FN WT --> FN VS --> FN GH --> FN IT --> FN OT --> FN ``` No new crates or public API surface. The only cross-crate dependency added is `codex_core::terminal::{TerminalName, terminal_info}`, which already existed for telemetry. ## Observability No new logging. Terminal detection already emits a `tracing::debug!` log line at startup with the detected terminal name, which is sufficient to diagnose binding mismatches. ## Tests - Existing `alt_up_edits_most_recent_queued_message` test is preserved and explicitly sets the Alt+Up binding to isolate from the host terminal. - New parameterized async tests verify Shift+Left works for Apple Terminal, Warp, and VSCode. - A sync unit test asserts the mapping table covers the three special terminals (Shift+Left) and that iTerm2 still gets Alt+Up. Fixes #4490 |
||
|
Felipe Coury
|
22545bf206 |
feat(tui): add sortable resume picker with created/updated timestamp toggle (#10752)
## Summary
- Add sorting support to the resume session picker with Tab key toggle
- Sessions can now be sorted by either creation time or last updated
time
- Display the current sort mode in the picker header
- Default to sorting by creation time (most recent first)
## Changes
- Add `sort_key` field to `PickerState` to track current sort order
- Pass sort key to `RolloutRecorder::list_threads()` for proper backend
sorting
- Add Tab key handler to toggle between `CreatedAt` and `UpdatedAt`
sorting
- Show current sort mode ("Created at" / "Updated at") in header
- Add "Tab to toggle sort" keyboard hint
- Intelligently hide secondary date column when terminal is narrow
- Reload session list when sort order changes
## Test plan
- [x] Unit tests for sort key toggle functionality
- [x] Snapshot tests updated for new header format
- [x] Test that Tab key triggers reload with new sort key
- [x] Test column visibility adapts to narrow terminals
|
||
|
Felipe CouryandJosh McKinney
|
b0e5a6305b |
feat(tui): add /statusline command for interactive status line configuration (#10546)
## Summary - Adds a new `/statusline` command to configure TUI footer status line - Introduces reusable `MultiSelectPicker` component with keyboard navigation, optional ordering and toggle support - Implement status line setup modal that persist configuration to config.toml ## Status Line Items The following items can be displayed in the status line: - **Model**: Current model name (with optional reasoning level) - **Context**: Remaining/used context window percentage - **Rate Limits**: 5-day and weekly usage limits - **Git**: Current branch (with optimized lookups) - **Tokens**: Used tokens, input/output token counts - **Session**: Session ID (full or shortened prefix) - **Paths**: Current directory, project root - **Version**: Codex version ## Features - Live preview while configuring status line items - Fuzzy search filtering in the picker - Intelligent truncation when items don't fit - Items gracefully omit when data is unavailable - Configuration persists to `config.toml` - Validates and warns about invalid status line items ## Test plan - [x] Run `/statusline` and verify picker UI appears - [x] Toggle items on/off and verify live preview updates - [x] Confirm selection persists after restart - [x] Verify truncation behavior with many items selected - [x] Test git branch detection in and out of git repos --------- Co-authored-by: Josh McKinney <joshka@openai.com> |