mirror of
https://github.com/openai/codex.git
synced 2026-05-01 18:06:47 +00:00
32c4993c8a17cb0608d747f268e3d543745de962
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Michael Bolin
|
b77fe8fefe |
Apply argument comment lint across codex-rs (#14652)
## Why Once the repo-local lint exists, `codex-rs` needs to follow the checked-in convention and CI needs to keep it from drifting. This commit applies the fallback `/*param*/` style consistently across existing positional literal call sites without changing those APIs. The longer-term preference is still to avoid APIs that require comments by choosing clearer parameter types and call shapes. This PR is intentionally the mechanical follow-through for the places where the existing signatures stay in place. After rebasing onto newer `main`, the rollout also had to cover newly introduced `tui_app_server` call sites. That made it clear the first cut of the CI job was too expensive for the common path: it was spending almost as much time installing `cargo-dylint` and re-testing the lint crate as a representative test job spends running product tests. The CI update keeps the full workspace enforcement but trims that extra overhead from ordinary `codex-rs` PRs. ## What changed - keep a dedicated `argument_comment_lint` job in `rust-ci` - mechanically annotate remaining opaque positional literals across `codex-rs` with exact `/*param*/` comments, including the rebased `tui_app_server` call sites that now fall under the lint - keep the checked-in style aligned with the lint policy by using `/*param*/` and leaving string and char literals uncommented - cache `cargo-dylint`, `dylint-link`, and the relevant Cargo registry/git metadata in the lint job - split changed-path detection so the lint crate's own `cargo test` step runs only when `tools/argument-comment-lint/*` or `rust-ci.yml` changes - continue to run the repo wrapper over the `codex-rs` workspace, so product-code enforcement is unchanged Most of the code changes in this commit are intentionally mechanical comment rewrites or insertions driven by the lint itself. ## Verification - `./tools/argument-comment-lint/run.sh --workspace` - `cargo test -p codex-tui-app-server -p codex-tui` - parsed `.github/workflows/rust-ci.yml` locally with PyYAML --- * -> #14652 * #14651 |
||
|
Curtis 'Fjord' Hawthorne
|
ee2e3c415b | Fix codespell warning about pre-selects (#13605) | ||
|
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
|
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
|
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. |