From 98923e53ccd0481925cb26f7146e80a6a4c42405 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Wed, 4 Mar 2026 17:03:34 -0300 Subject: [PATCH] fix(tui): decode ANSI alpha-channel encoding in syntax themes (#13382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 | |---|---|---|---| | macos-dark | macos-light | ![windows-ansi](https://github.com/user-attachments/assets/d41029e6-ffd3-454e-ab72-6751607e5d5c) | ![windows-base16](https://github.com/user-attachments/assets/b48aafcc-0196-4977-8ee1-8f8eaddd1698) | ## 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 --- codex-rs/tui/src/render/highlight.rs | 300 +++++++++++++++--- ...tests__ansi_family_foreground_palette.snap | 21 ++ 2 files changed, 284 insertions(+), 37 deletions(-) create mode 100644 codex-rs/tui/src/render/snapshots/codex_tui__render__highlight__tests__ansi_family_foreground_palette.snap diff --git a/codex-rs/tui/src/render/highlight.rs b/codex-rs/tui/src/render/highlight.rs index 2fd0abbb94..b44fc0a742 100644 --- a/codex-rs/tui/src/render/highlight.rs +++ b/codex-rs/tui/src/render/highlight.rs @@ -31,6 +31,7 @@ use std::path::PathBuf; use std::sync::OnceLock; use std::sync::RwLock; use syntect::easy::HighlightLines; +use syntect::highlighting::Color as SyntectColor; use syntect::highlighting::FontStyle; use syntect::highlighting::Highlighter; use syntect::highlighting::Style as SyntectStyle; @@ -49,10 +50,23 @@ static THEME: OnceLock> = OnceLock::new(); static THEME_OVERRIDE: OnceLock> = OnceLock::new(); static CODEX_HOME: OnceLock> = OnceLock::new(); +// Syntect/bat encode ANSI palette semantics in alpha: +// `a=0` => indexed ANSI palette via RGB payload, `a=1` => terminal default. +const ANSI_ALPHA_INDEX: u8 = 0x00; +const ANSI_ALPHA_DEFAULT: u8 = 0x01; +const OPAQUE_ALPHA: u8 = 0xFF; + fn syntax_set() -> &'static SyntaxSet { SYNTAX_SET.get_or_init(two_face::syntax::extra_newlines) } +// NOTE: We intentionally do NOT emit a runtime diagnostic when an ANSI-family +// theme (ansi, base16, base16-256) lacks the expected alpha-channel marker +// encoding. If the upstream two_face/syntect theme format changes, the +// `ansi_themes_use_only_ansi_palette_colors` test will catch it at build +// time — long before it reaches users. A runtime warning would be +// unactionable noise since users can't fix upstream themes. + /// Set the user-configured syntax theme override and codex home path. /// /// Call this with the **final resolved config** (after onboarding, resume, and @@ -62,15 +76,13 @@ fn syntax_set() -> &'static SyntaxSet { /// Subsequent calls cannot change the persisted `OnceLock` values, but they /// still update the runtime theme immediately for live preview flows. /// -/// Returns a warning message when the configured theme name cannot be -/// resolved to a bundled theme or a custom `.tmTheme` file on disk. -/// The caller should surface this via `Config::startup_warnings` so it -/// appears as a `⚠` banner in the TUI. +/// Returns user-facing warnings for actionable configuration issues, such as +/// unknown/invalid theme names or duplicate override persistence. pub(crate) fn set_theme_override( name: Option, codex_home: Option, ) -> Option { - let mut warning = validate_theme_name(name.as_deref(), codex_home.as_deref()); + let warning = validate_theme_name(name.as_deref(), codex_home.as_deref()); let override_set_ok = THEME_OVERRIDE.set(name.clone()).is_ok(); let codex_home_set_ok = CODEX_HOME.set(codex_home.clone()).is_ok(); if THEME.get().is_some() { @@ -80,11 +92,10 @@ pub(crate) fn set_theme_override( )); } if !override_set_ok || !codex_home_set_ok { - let duplicate_msg = "Ignoring duplicate or late syntax theme override persistence; runtime theme was updated from the latest override, but persisted override config can only be initialized once."; - tracing::warn!("{duplicate_msg}"); - if warning.is_none() { - warning = Some(duplicate_msg.to_string()); - } + // This should never happen in practice — set_theme_override is only + // called once at startup. Keep as a debug breadcrumb in case a second + // call site is added in the future. + tracing::debug!("set_theme_override called more than once; OnceLock values unchanged"); } warning } @@ -109,15 +120,15 @@ pub(crate) fn validate_theme_name(name: Option<&str>, codex_home: Option<&Path>) return None; } return Some(format!( - "Syntax theme \"{name}\" was found at {custom_theme_path_display} \ - but could not be parsed. Falling back to auto-detection." + "Custom theme \"{name}\" at {custom_theme_path_display} could not \ + be loaded (invalid .tmTheme format). Falling back to the default theme." )); } } Some(format!( - "Unknown syntax theme \"{name}\", falling back to auto-detection. \ - Use a bundled name or place a .tmTheme file at \ - {custom_theme_path_display}" + "Theme \"{name}\" not found. Using the default theme. \ + To use a custom theme, place a .tmTheme file at \ + {custom_theme_path_display}." )) } @@ -189,7 +200,7 @@ pub(crate) fn adaptive_default_theme_name() -> &'static str { adaptive_default_theme_selection().1 } -/// Build the theme from current override/auto-detection settings. +/// Build the theme from current override/default-theme settings. /// Extracted from the old `theme()` init closure so it can be reused. fn resolve_theme_with_override(name: Option<&str>, codex_home: Option<&Path>) -> Theme { let ts = two_face::theme::extra(); @@ -206,13 +217,13 @@ fn resolve_theme_with_override(name: Option<&str>, codex_home: Option<&Path>) -> { return theme; } - tracing::warn!("unknown syntax theme \"{name}\", falling back to auto-detection"); + tracing::debug!("Theme \"{name}\" not recognized; using default theme"); } ts.get(adaptive_default_embedded_theme_name()).clone() } -/// Build the theme from current override/auto-detection settings. +/// Build the theme from current override/default-theme settings. /// Extracted from the old `theme()` init closure so it can be reused. fn build_default_theme() -> Theme { let name = THEME_OVERRIDE.get().and_then(|name| name.as_deref()); @@ -412,20 +423,71 @@ const BUILTIN_THEME_NAMES: &[&str] = &[ // -- Style conversion (syntect -> ratatui) ------------------------------------ +/// Map a low ANSI palette index (0–7) to ratatui's named color variants, +/// falling back to `Indexed(n)` for indices 8–255. +/// +/// Named variants are preferred over `Indexed(0)`…`Indexed(7)` because many +/// terminals apply bold/bright treatment differently for named vs indexed +/// colors, and ANSI themes expect the named behavior. +/// +/// `clippy::disallowed_methods` is explicitly allowed here because this helper +/// intentionally constructs `ratatui::style::Color::Indexed`. +#[allow(clippy::disallowed_methods)] +fn ansi_palette_color(index: u8) -> RtColor { + match index { + 0x00 => RtColor::Black, + 0x01 => RtColor::Red, + 0x02 => RtColor::Green, + 0x03 => RtColor::Yellow, + 0x04 => RtColor::Blue, + 0x05 => RtColor::Magenta, + 0x06 => RtColor::Cyan, + // ANSI code 37 is "white", represented as `Gray` in ratatui. + 0x07 => RtColor::Gray, + n => RtColor::Indexed(n), + } +} + +/// Decode a syntect foreground `Color` into a ratatui color, respecting the +/// alpha-channel encoding that bat's `ansi`, `base16`, and `base16-256` themes +/// use to signal ANSI palette semantics instead of true RGB. +/// +/// Returns `None` when the color signals "use the terminal's default +/// foreground", allowing the caller to omit the foreground attribute entirely. +/// +/// Passing a color from a standard RGB theme (alpha 0xFF) returns +/// `Some(Rgb(..))`, so this function is backward-compatible with non-ANSI +/// themes. Unexpected intermediate alpha values are treated as RGB. +/// +/// `clippy::disallowed_methods` is explicitly allowed here because this helper +/// intentionally constructs `ratatui::style::Color::Rgb`. +#[allow(clippy::disallowed_methods)] +fn convert_syntect_color(color: SyntectColor) -> Option { + match color.a { + // Bat-compatible encoding used by `ansi`, `base16`, and `base16-256`: + // alpha 0x00 means `r` stores an ANSI palette index, not RGB red. + ANSI_ALPHA_INDEX => Some(ansi_palette_color(color.r)), + // alpha 0x01 means "use terminal default foreground/background". + ANSI_ALPHA_DEFAULT => None, + OPAQUE_ALPHA => Some(RtColor::Rgb(color.r, color.g, color.b)), + // Non-ANSI alpha values appear in some bundled themes; treat as plain RGB. + _ => Some(RtColor::Rgb(color.r, color.g, color.b)), + } +} + /// Convert a syntect `Style` to a ratatui `Style`. /// -/// Syntax highlighting themes inherently produce RGB colors, so we allow -/// `Color::Rgb` here despite the project-wide preference for ANSI colors. -#[allow(clippy::disallowed_methods)] +/// Most themes produce RGB colors. The built-in `ansi`/`base16`/`base16-256` +/// themes encode ANSI palette semantics in the alpha channel, matching bat. fn convert_style(syn_style: SyntectStyle) -> Style { let mut rt_style = Style::default(); - // Map foreground color when visible. - let fg = syn_style.foreground; - if fg.a > 0 { - rt_style = rt_style.fg(RtColor::Rgb(fg.r, fg.g, fg.b)); + if let Some(fg) = convert_syntect_color(syn_style.foreground) { + rt_style = rt_style.fg(fg); } // Intentionally skip background to avoid overwriting terminal bg. + // If background support is added later, decode with `convert_syntect_color` + // to reuse the same alpha-marker semantics as foreground. if syn_style.font_style.contains(FontStyle::BOLD) { rt_style.add_modifier |= Modifier::BOLD; @@ -500,10 +562,16 @@ pub(crate) fn exceeds_highlight_limits(total_bytes: usize, total_lines: usize) - // -- Core highlighting -------------------------------------------------------- -/// Parse `code` using syntect for `lang` and return per-line styled spans. -/// Each inner Vec represents one source line. Returns None when the language -/// is not recognized or the input exceeds safety limits. -fn highlight_to_line_spans(code: &str, lang: &str) -> Option>>> { +/// Core highlighter that accepts an explicit theme reference. +/// +/// This keeps production behavior and test behavior on the same code path: +/// production callers pass the global theme lock, while tests can pass a +/// concrete theme without mutating process-global state. +fn highlight_to_line_spans_with_theme( + code: &str, + lang: &str, + theme: &Theme, +) -> Option>>> { // Empty input has nothing to highlight; fall back to the plain text path // which correctly produces a single empty Line. if code.is_empty() { @@ -518,11 +586,7 @@ fn highlight_to_line_spans(code: &str, lang: &str) -> Option theme_guard, - Err(poisoned) => poisoned.into_inner(), - }; - let mut h = HighlightLines::new(syntax, &theme_guard); + let mut h = HighlightLines::new(syntax, theme); let mut lines: Vec>> = Vec::new(); for line in LinesWithEndings::from(code) { @@ -546,6 +610,17 @@ fn highlight_to_line_spans(code: &str, lang: &str) -> Option Option>>> { + let theme_guard = match theme_lock().read() { + Ok(theme_guard) => theme_guard, + Err(poisoned) => poisoned.into_inner(), + }; + highlight_to_line_spans_with_theme(code, lang, &theme_guard) +} + // -- Public API --------------------------------------------------------------- /// Highlight code in any supported language, returning styled ratatui `Line`s. @@ -596,6 +671,7 @@ pub(crate) fn highlight_code_to_styled_spans( #[cfg(test)] mod tests { use super::*; + use insta::assert_snapshot; use pretty_assertions::assert_eq; use std::str::FromStr; use syntect::highlighting::Color as SyntectColor; @@ -673,6 +749,25 @@ mod tests { .join("\n") } + fn unique_foreground_colors_for_theme(theme_name: &str) -> Vec { + let theme = resolve_theme_by_name(theme_name, None) + .unwrap_or_else(|| panic!("expected built-in theme {theme_name} to resolve")); + let lines = highlight_to_line_spans_with_theme( + "fn main() { let answer = 42; println!(\"hello\"); }\n", + "rust", + &theme, + ) + .expect("expected highlighted spans"); + let mut colors: Vec = lines + .iter() + .flat_map(|line| line.iter().filter_map(|span| span.style.fg)) + .map(|fg| format!("{fg:?}")) + .collect(); + colors.sort(); + colors.dedup(); + colors + } + fn theme_item(scope: &str, background: Option<(u8, u8, u8)>) -> ThemeItem { ThemeItem { scope: ScopeSelectors::from_str(scope).expect("scope selector should parse"), @@ -791,7 +886,6 @@ mod tests { } #[test] - #[allow(clippy::disallowed_methods)] fn convert_style_suppresses_underline() { // Dracula (and other themes) set FontStyle::UNDERLINE on type scopes, // producing distracting underlines on type names in terminal output. @@ -807,7 +901,7 @@ mod tests { r: 0, g: 0, b: 0, - a: 0, + a: 0xFF, }, font_style: FontStyle::UNDERLINE, }; @@ -820,6 +914,138 @@ mod tests { ); } + #[test] + fn style_conversion_uses_ansi_named_color_when_alpha_is_zero_low_index() { + let syn = SyntectStyle { + foreground: syntect::highlighting::Color { + r: 0x02, + g: 0, + b: 0, + a: 0, + }, + background: syntect::highlighting::Color { + r: 0, + g: 0, + b: 0, + a: 0xFF, + }, + font_style: FontStyle::empty(), + }; + let rt = convert_style(syn); + assert_eq!(rt.fg, Some(RtColor::Green)); + } + + #[test] + fn style_conversion_uses_indexed_color_when_alpha_is_zero_high_index() { + let syn = SyntectStyle { + foreground: syntect::highlighting::Color { + r: 0x9a, + g: 0, + b: 0, + a: 0, + }, + background: syntect::highlighting::Color { + r: 0, + g: 0, + b: 0, + a: 0xFF, + }, + font_style: FontStyle::empty(), + }; + let rt = convert_style(syn); + assert!(matches!(rt.fg, Some(RtColor::Indexed(0x9a)))); + } + + #[test] + fn style_conversion_uses_terminal_default_when_alpha_is_one() { + let syn = SyntectStyle { + foreground: syntect::highlighting::Color { + r: 0, + g: 0, + b: 0, + a: 1, + }, + background: syntect::highlighting::Color { + r: 0, + g: 0, + b: 0, + a: 0xFF, + }, + font_style: FontStyle::empty(), + }; + let rt = convert_style(syn); + assert_eq!(rt.fg, None); + } + + #[test] + fn style_conversion_unexpected_alpha_falls_back_to_rgb() { + let syn = SyntectStyle { + foreground: syntect::highlighting::Color { + r: 10, + g: 20, + b: 30, + a: 0x80, + }, + background: syntect::highlighting::Color { + r: 0, + g: 0, + b: 0, + a: 0xFF, + }, + font_style: FontStyle::empty(), + }; + let rt = convert_style(syn); + assert!(matches!(rt.fg, Some(RtColor::Rgb(10, 20, 30)))); + } + + #[test] + fn ansi_palette_color_maps_ansi_white_to_gray() { + assert_eq!(ansi_palette_color(0x07), RtColor::Gray); + } + + #[test] + fn ansi_family_themes_use_terminal_palette_colors_not_rgb() { + for theme_name in ["ansi", "base16", "base16-256"] { + let theme = resolve_theme_by_name(theme_name, None) + .unwrap_or_else(|| panic!("expected built-in theme {theme_name} to resolve")); + let lines = highlight_to_line_spans_with_theme( + "fn main() { let answer = 42; println!(\"hello\"); }\n", + "rust", + &theme, + ) + .expect("expected highlighted spans"); + let mut has_non_default_fg = false; + for line in &lines { + for span in line { + match span.style.fg { + Some(RtColor::Rgb(..)) => { + panic!("theme {theme_name} produced RGB foreground: {span:?}") + } + Some(_) => has_non_default_fg = true, + None => {} + } + } + } + assert!( + has_non_default_fg, + "theme {theme_name} should produce at least one non-default foreground color" + ); + } + } + + #[test] + fn ansi_family_foreground_palette_snapshot() { + let mut out = String::new(); + for theme_name in ["ansi", "base16", "base16-256"] { + let colors = unique_foreground_colors_for_theme(theme_name); + out.push_str(&format!("{theme_name}:\n")); + for color in colors { + out.push_str(&format!(" {color}\n")); + } + } + assert_snapshot!("ansi_family_foreground_palette", out); + } + #[test] fn highlight_multiline_python() { let code = "def hello():\n print(\"hi\")\n return 42"; @@ -1152,7 +1378,7 @@ mod tests { assert!( warning .as_deref() - .is_some_and(|msg| msg.contains("could not be parsed")), + .is_some_and(|msg| msg.contains("could not be loaded")), "warning should explain that the theme file is invalid" ); } diff --git a/codex-rs/tui/src/render/snapshots/codex_tui__render__highlight__tests__ansi_family_foreground_palette.snap b/codex-rs/tui/src/render/snapshots/codex_tui__render__highlight__tests__ansi_family_foreground_palette.snap new file mode 100644 index 0000000000..79e130916c --- /dev/null +++ b/codex-rs/tui/src/render/snapshots/codex_tui__render__highlight__tests__ansi_family_foreground_palette.snap @@ -0,0 +1,21 @@ +--- +source: tui/src/render/highlight.rs +expression: out +--- +ansi: + Blue + Green + Magenta + Yellow +base16: + Blue + Gray + Green + Indexed(9) + Magenta +base16-256: + Blue + Gray + Green + Indexed(16) + Magenta