From 258efb2575c7b2839c947e76b400f541efa87259 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Tue, 28 Apr 2026 16:02:06 -0700 Subject: [PATCH] feat(tui): expand light-terminal auto-detection (HERMES_TUI_THEME, background hex) (#17113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(tui): expand light-terminal auto-detection (HERMES_TUI_THEME, BG hex) Modern terminals (Ghostty, Warp, iTerm2) don't set COLORFGBG, so the auto-light path was effectively COLORFGBG-only and silently broken for many users. Two pragmatic additions, both opt-in, plus a clearer priority chain: 1. **`HERMES_TUI_THEME=light|dark`** as a symmetric explicit override. The existing `HERMES_TUI_LIGHT` is fine but reads as boolean noise; a named theme env var matches `display.skin` muscle memory. 2. **`HERMES_TUI_BACKGROUND` hex/rgb hint.** Lets advanced users (or a future OSC11 query helper that caches the answer) state a ground-truth background colour. Decoded to Rec. 709 luma; ≥ 0.6 counts as light. Priority order is now fully ordered and explainable: 1. `HERMES_TUI_LIGHT` (1/0/true/false/on/off). 2. `HERMES_TUI_THEME=light|dark`. 3. `HERMES_TUI_BACKGROUND` luminance. 4. `COLORFGBG` last field — light slots 7/15 → light, 0–15 → dark (authoritative when set, so the new TERM_PROGRAM path can never stomp on a terminal that already volunteered a dark answer). 5. `TERM_PROGRAM` allow-list — empty by default. The slot is left in place because folks asked for it but populating it risks wrongly flipping users on Apple_Terminal / iTerm2 dark profiles to light. Easy to add per terminal once we have signal. Tests: 5 new cases in `theme.test.ts` covering theme env, background hex (3- and 6-char), invalid hex falling through, and COLORFGBG taking precedence over the future allow-list. Validation: `npm run type-check` clean, `npm test --run` 392/392. * review(copilot): tighten theme detection comments + drop unnecessary cast * review(copilot): strict hex regex so partial garbage doesn't slip into luminance * test(tui): make TERM_PROGRAM allow-list injectable so precedence is provable Copilot review on PR #17113: `LIGHT_DEFAULT_TERM_PROGRAMS` is empty in production, so the prior assertion would have passed even if `detectLightMode` ignored `COLORFGBG` entirely. That defeats the test's purpose. `detectLightMode` now takes the allow-list as an optional second argument (defaults to the production set). The test injects a set containing `Apple_Terminal`, asserts the allow-list alone WOULD return light, then asserts `COLORFGBG: '15;0'` overrides it — the precedence rule is now exercised, not assumed. * fix(tui): COLORFGBG empty-trailing-field falls through; isolate DEFAULT_THEME tests Round 2 Copilot review on PR #17113: 1. `Number(colorfgbg.split(';').at(-1))` returns 0 for an empty trailing field (e.g. `COLORFGBG='15;'` → bg===0), which would have looked like an authoritative dark slot and incorrectly blocked the TERM_PROGRAM allow-list. Added a `/^\d+$/` guard before coercion; non-numeric trailing fields now fall through. 2. Fixed the misleading '0–6 / 8–15 ranges are dark' comment — the block returns true for bg===15, so the range is actually 0–6 / 8–14. 3. `DEFAULT_THEME` is computed from `process.env` at module-load. A developer shell with `HERMES_TUI_THEME=light` (or a bright `HERMES_TUI_BACKGROUND`) would flip it and break local tests. The DEFAULT_THEME describe blocks now sterilize the relevant env vars + dynamically import theme.ts (vi.resetModules pattern from platform.test.ts). fromSkin tests compare against DARK_THEME directly to decouple them from ambient env. * test(tui): isolate ALL env-coupled theme symbols, not just DEFAULT_THEME Round 3 Copilot review on PR #17113: the static top-level imports of `fromSkin`, `DARK_THEME`, `LIGHT_THEME` evaluated theme.ts before `importThemeWithCleanEnv` had a chance to clean the env. Because `fromSkin` closes over `DEFAULT_THEME`, an ambient `HERMES_TUI_THEME=light` or bright `HERMES_TUI_BACKGROUND` would still flip the base palette and cause local-only failures. Removed the static import entirely. Every test now obtains its theme symbols via `importThemeWithCleanEnv`, including `detectLightMode` (for consistency, even though it takes env as a parameter). `fromSkin` tests assert against the cleaned `DEFAULT_THEME` from the same dynamic import — preserves the actual contract (skins extend the ambient base palette) without coupling the test to dev-shell state. Verified by running with HERMES_TUI_THEME=light + HERMES_TUI_BACKGROUND=#ffffff: all 20 theme tests still pass. Self-review (avoid round 4): - Audited other test files importing DEFAULT_THEME (syntax.test.ts, streamingMarkdown.test.ts, constants.test.ts) — all just pass it as a parameter or assert palette property existence (works on both light + dark), so no env coupling there. --- ui-tui/src/__tests__/theme.test.ts | 163 +++++++++++++++++++++++++---- ui-tui/src/theme.ts | 124 ++++++++++++++++++++-- 2 files changed, 260 insertions(+), 27 deletions(-) diff --git a/ui-tui/src/__tests__/theme.test.ts b/ui-tui/src/__tests__/theme.test.ts index db2b1eac38..b73251188f 100644 --- a/ui-tui/src/__tests__/theme.test.ts +++ b/ui-tui/src/__tests__/theme.test.ts @@ -1,46 +1,90 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' -import { DARK_THEME, DEFAULT_THEME, detectLightMode, fromSkin, LIGHT_THEME } from '../theme.js' +// `theme.js` reads `process.env` at module-load to compute DEFAULT_THEME, +// and `fromSkin` closes over DEFAULT_THEME. A developer shell with +// HERMES_TUI_THEME=light (or HERMES_TUI_BACKGROUND set to something +// bright) would flip the base and turn these assertions into a local- +// only failure. We sterilize the relevant env vars + dynamically +// import the module fresh so EVERY symbol that closes over the env +// (DEFAULT_THEME, DARK_THEME, LIGHT_THEME, fromSkin) is loaded against +// a known-empty environment. +// +// `detectLightMode` takes env as an explicit arg, so it's safe to import +// statically — but we stay consistent and dynamic-import it too. +const RELEVANT_ENV = [ + 'HERMES_TUI_LIGHT', + 'HERMES_TUI_THEME', + 'HERMES_TUI_BACKGROUND', + 'COLORFGBG', + 'TERM_PROGRAM', +] as const + +async function importThemeWithCleanEnv() { + for (const key of RELEVANT_ENV) { + vi.stubEnv(key, '') + } + vi.resetModules() + return import('../theme.js') +} + +afterEach(() => { + vi.unstubAllEnvs() + vi.resetModules() +}) describe('DEFAULT_THEME', () => { - it('has brand defaults', () => { + it('has brand defaults', async () => { + const { DEFAULT_THEME } = await importThemeWithCleanEnv() + expect(DEFAULT_THEME.brand.name).toBe('Hermes Agent') expect(DEFAULT_THEME.brand.prompt).toBe('❯') expect(DEFAULT_THEME.brand.tool).toBe('┊') }) - it('has color palette', () => { + it('has color palette', async () => { + const { DEFAULT_THEME } = await importThemeWithCleanEnv() + expect(DEFAULT_THEME.color.gold).toBe('#FFD700') expect(DEFAULT_THEME.color.error).toBe('#ef5350') }) }) describe('LIGHT_THEME', () => { - it('avoids bright-yellow accents unreadable on white backgrounds (#11300)', () => { + it('avoids bright-yellow accents unreadable on white backgrounds (#11300)', async () => { + const { LIGHT_THEME } = await importThemeWithCleanEnv() + expect(LIGHT_THEME.color.gold).not.toBe('#FFD700') expect(LIGHT_THEME.color.amber).not.toBe('#FFBF00') expect(LIGHT_THEME.color.dim).not.toBe('#B8860B') expect(LIGHT_THEME.color.statusWarn).not.toBe('#FFD700') }) - it('keeps the same shape as DARK_THEME', () => { + it('keeps the same shape as DARK_THEME', async () => { + const { DARK_THEME, LIGHT_THEME } = await importThemeWithCleanEnv() + expect(Object.keys(LIGHT_THEME.color).sort()).toEqual(Object.keys(DARK_THEME.color).sort()) expect(LIGHT_THEME.brand).toEqual(DARK_THEME.brand) }) }) describe('DEFAULT_THEME aliasing', () => { - it('defaults to DARK_THEME when nothing signals light', () => { - expect(DEFAULT_THEME).toBe(DARK_THEME) + it('defaults to DARK_THEME when nothing signals light', async () => { + const { DEFAULT_THEME, DARK_THEME: DARK } = await importThemeWithCleanEnv() + + expect(DEFAULT_THEME).toBe(DARK) }) }) describe('detectLightMode', () => { - it('returns false on empty env', () => { + it('returns false on empty env', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + expect(detectLightMode({})).toBe(false) }) - it('honors HERMES_TUI_LIGHT on/off', () => { + it('honors HERMES_TUI_LIGHT on/off', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + expect(detectLightMode({ HERMES_TUI_LIGHT: '1' })).toBe(true) expect(detectLightMode({ HERMES_TUI_LIGHT: 'true' })).toBe(true) expect(detectLightMode({ HERMES_TUI_LIGHT: 'on' })).toBe(true) @@ -48,7 +92,9 @@ describe('detectLightMode', () => { expect(detectLightMode({ HERMES_TUI_LIGHT: 'off' })).toBe(false) }) - it('sniffs COLORFGBG bg slots 7 and 15 as light (#11300)', () => { + it('sniffs COLORFGBG bg slots 7 and 15 as light (#11300)', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + expect(detectLightMode({ COLORFGBG: '0;15' })).toBe(true) expect(detectLightMode({ COLORFGBG: '0;default;15' })).toBe(true) expect(detectLightMode({ COLORFGBG: '0;7' })).toBe(true) @@ -56,38 +102,119 @@ describe('detectLightMode', () => { expect(detectLightMode({ COLORFGBG: '7;default;0' })).toBe(false) }) - it('lets HERMES_TUI_LIGHT=0 override a light COLORFGBG', () => { + it('falls through on malformed COLORFGBG with empty/non-numeric trailing field', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + // `Number('')` is 0, so `'15;'` would have been read as bg=0 + // (authoritative dark) and incorrectly blocked TERM_PROGRAM. + // The strict /^\d+$/ guard makes these fall through instead. + const allowList = new Set(['Apple_Terminal']) + + expect(detectLightMode({ COLORFGBG: '15;', TERM_PROGRAM: 'Apple_Terminal' }, allowList)).toBe(true) + expect(detectLightMode({ COLORFGBG: 'default;default', TERM_PROGRAM: 'Apple_Terminal' }, allowList)).toBe(true) + // Without an allow-list match, fall-through still defaults to dark. + expect(detectLightMode({ COLORFGBG: '15;' })).toBe(false) + }) + + it('lets HERMES_TUI_LIGHT=0 override a light COLORFGBG', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + expect(detectLightMode({ COLORFGBG: '0;15', HERMES_TUI_LIGHT: '0' })).toBe(false) }) + + it('honors HERMES_TUI_THEME=light/dark as a symmetric explicit override', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + + expect(detectLightMode({ HERMES_TUI_THEME: 'light' })).toBe(true) + expect(detectLightMode({ HERMES_TUI_THEME: 'dark' })).toBe(false) + expect(detectLightMode({ COLORFGBG: '0;15', HERMES_TUI_THEME: 'dark' })).toBe(false) + expect(detectLightMode({ COLORFGBG: '15;0', HERMES_TUI_THEME: 'light' })).toBe(true) + }) + + it('uses HERMES_TUI_BACKGROUND luminance when COLORFGBG is missing', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#ffffff' })).toBe(true) + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#000000' })).toBe(false) + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#1e1e1e' })).toBe(false) + // Three-char hex normalises like CSS. + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#fff' })).toBe(true) + // Garbage falls through to the default-dark path. + expect(detectLightMode({ HERMES_TUI_BACKGROUND: 'not-a-colour' })).toBe(false) + }) + + it('rejects partially-invalid hex instead of silently truncating', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + // `parseInt('fffgff'.slice(2,4), 16)` would return 15 — the strict + // regex must reject these inputs so they fall through to default- + // dark instead of producing a false-positive light reading. + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#fffgff' })).toBe(false) + expect(detectLightMode({ HERMES_TUI_BACKGROUND: 'ffggff' })).toBe(false) + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#xyz' })).toBe(false) + // Wrong length also rejected (no implicit padding/truncation). + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#fffff' })).toBe(false) + expect(detectLightMode({ HERMES_TUI_BACKGROUND: '#fffffff' })).toBe(false) + }) + + it('treats COLORFGBG as authoritative when present so it dominates the TERM_PROGRAM allow-list', async () => { + const { detectLightMode } = await importThemeWithCleanEnv() + // Inject a light-default allow-list so the precedence test is + // meaningful even though the production allow-list is empty. + const allowList = new Set(['Apple_Terminal']) + + // Sanity: the allow-list alone WOULD turn this terminal light. + expect(detectLightMode({ TERM_PROGRAM: 'Apple_Terminal' }, allowList)).toBe(true) + + // Dark COLORFGBG must beat the allow-list. + expect( + detectLightMode({ COLORFGBG: '15;0', TERM_PROGRAM: 'Apple_Terminal' }, allowList), + ).toBe(false) + }) }) describe('fromSkin', () => { - it('overrides banner colors', () => { + // `fromSkin` closes over DEFAULT_THEME (which is env-derived), so we + // must dynamic-import it after sterilizing env — otherwise an ambient + // HERMES_TUI_THEME=light would flip the base palette and make these + // assertions order-dependent on the developer's shell. + + it('overrides banner colors', async () => { + const { fromSkin } = await importThemeWithCleanEnv() + expect(fromSkin({ banner_title: '#FF0000' }, {}).color.gold).toBe('#FF0000') }) - it('preserves unset colors', () => { + it('preserves unset colors', async () => { + const { DEFAULT_THEME, fromSkin } = await importThemeWithCleanEnv() + expect(fromSkin({ banner_title: '#FF0000' }, {}).color.amber).toBe(DEFAULT_THEME.color.amber) }) - it('overrides branding', () => { + it('overrides branding', async () => { + const { fromSkin } = await importThemeWithCleanEnv() const { brand } = fromSkin({}, { agent_name: 'TestBot', prompt_symbol: '$' }) + expect(brand.name).toBe('TestBot') expect(brand.prompt).toBe('$') }) - it('defaults for empty skin', () => { + it('defaults for empty skin', async () => { + const { DEFAULT_THEME, fromSkin } = await importThemeWithCleanEnv() + expect(fromSkin({}, {}).color).toEqual(DEFAULT_THEME.color) expect(fromSkin({}, {}).brand.icon).toBe(DEFAULT_THEME.brand.icon) }) - it('passes banner logo/hero', () => { + it('passes banner logo/hero', async () => { + const { fromSkin } = await importThemeWithCleanEnv() + expect(fromSkin({}, {}, 'LOGO', 'HERO').bannerLogo).toBe('LOGO') expect(fromSkin({}, {}, 'LOGO', 'HERO').bannerHero).toBe('HERO') }) - it('maps ui_ color keys + cascades to status', () => { + it('maps ui_ color keys + cascades to status', async () => { + const { fromSkin } = await importThemeWithCleanEnv() const { color } = fromSkin({ ui_ok: '#008000' }, {}) + expect(color.ok).toBe('#008000') expect(color.statusGood).toBe('#008000') }) diff --git a/ui-tui/src/theme.ts b/ui-tui/src/theme.ts index daeedb3377..baff80abf1 100644 --- a/ui-tui/src/theme.ts +++ b/ui-tui/src/theme.ts @@ -179,23 +179,129 @@ export const LIGHT_THEME: Theme = { bannerHero: '' } -// Pick light vs dark. Explicit `HERMES_TUI_LIGHT` wins; otherwise sniff -// `COLORFGBG` (set by XFCE Terminal, rxvt, Terminal.app, etc.) — last field is the -// background ANSI index; 7/15 are the "white" slots most light themes emit (#11300). -export function detectLightMode(env: NodeJS.ProcessEnv = process.env): boolean { - const explicit = (env.HERMES_TUI_LIGHT ?? '').trim().toLowerCase() +const TRUE_RE = /^(?:1|true|yes|on)$/ +const FALSE_RE = /^(?:0|false|no|off)$/ - if (/^(?:1|true|yes|on)$/.test(explicit)) { +// Reserved for future TERM_PROGRAM-based heuristics. Empty by default: +// most modern terminals (Ghostty, Warp, iTerm2, Apple_Terminal) ship a +// dark profile out of the box, so guessing wrong here is more annoying +// than missing a light user — light users can always set +// `HERMES_TUI_LIGHT=1` or `HERMES_TUI_THEME=light`. +const LIGHT_DEFAULT_TERM_PROGRAMS = new Set() + +// Best-effort RGB → luminance check. Currently only accepts a 3- or +// 6-digit hex value (with or without a leading `#`); the env var name +// `HERMES_TUI_BACKGROUND` is intentionally generic so a future OSC11 +// query helper can cache its answer there too, but additional formats +// (rgb()/hsl()/named colours) would need explicit parsing here first. +const LUMA_LIGHT_THRESHOLD = 0.6 + +// Strict allow-list: parseInt(..., 16) silently truncates at the first +// non-hex character (e.g. `fffgff` would parse as `fff` and yield a +// false-positive "white" reading), so reject anything that doesn't match +// the canonical 3- or 6-digit shape up front. +const HEX_3_RE = /^[0-9a-f]{3}$/ +const HEX_6_RE = /^[0-9a-f]{6}$/ + +function backgroundLuminance(raw: string): null | number { + const v = raw.trim().toLowerCase() + + if (!v) { + return null + } + + const hex = v.startsWith('#') ? v.slice(1) : v + const rgb = HEX_6_RE.test(hex) + ? [parseInt(hex.slice(0, 2), 16), parseInt(hex.slice(2, 4), 16), parseInt(hex.slice(4, 6), 16)] + : HEX_3_RE.test(hex) + ? [parseInt(hex[0]! + hex[0]!, 16), parseInt(hex[1]! + hex[1]!, 16), parseInt(hex[2]! + hex[2]!, 16)] + : null + + if (!rgb) { + return null + } + + // Rec. 709 luma — close enough for "is this background bright". + return (0.2126 * rgb[0]! + 0.7152 * rgb[1]! + 0.0722 * rgb[2]!) / 255 +} + +// Pick light vs dark with ordered, explainable signals (#11300): +// +// 1. `HERMES_TUI_LIGHT` boolean — `1`/`true`/`yes`/`on` → light; +// `0`/`false`/`no`/`off` → dark. Either explicit value wins +// regardless of any later signal. +// 2. `HERMES_TUI_THEME` named override — `light` / `dark` win over +// every signal below. +// 3. `HERMES_TUI_BACKGROUND` hex hint (3- or 6-digit) — luminance +// ≥ LUMA_LIGHT_THRESHOLD → light. +// 4. `COLORFGBG` last field — XFCE / rxvt / Terminal.app emit +// slot 7 or 15 on light profiles; 0–15 ranges are otherwise +// treated as authoritatively dark so the TERM_PROGRAM +// allow-list below cannot override an explicit dark profile. +// 5. `TERM_PROGRAM` light-default allow-list (currently empty). +// +// Anything we can't decide stays dark — the default Hermes palette +// is the dark one. +export function detectLightMode( + env: NodeJS.ProcessEnv = process.env, + // Injectable so tests can prove the COLORFGBG-over-TERM_PROGRAM + // precedence rule even though the production allow-list is empty. + lightDefaultTermPrograms: ReadonlySet = LIGHT_DEFAULT_TERM_PROGRAMS, +): boolean { + const lightFlag = (env.HERMES_TUI_LIGHT ?? '').trim().toLowerCase() + + if (TRUE_RE.test(lightFlag)) { return true } - if (/^(?:0|false|no|off)$/.test(explicit)) { + if (FALSE_RE.test(lightFlag)) { return false } - const bg = Number((env.COLORFGBG ?? '').trim().split(';').at(-1)) + const themeFlag = (env.HERMES_TUI_THEME ?? '').trim().toLowerCase() - return bg === 7 || bg === 15 + if (themeFlag === 'light') { + return true + } + + if (themeFlag === 'dark') { + return false + } + + const bgHint = backgroundLuminance(env.HERMES_TUI_BACKGROUND ?? '') + + if (bgHint !== null) { + return bgHint >= LUMA_LIGHT_THRESHOLD + } + + const colorfgbg = (env.COLORFGBG ?? '').trim() + + if (colorfgbg) { + // Validate as a decimal integer before coercing — `Number('')` is 0, + // so a malformed `COLORFGBG='15;'` would otherwise look like an + // authoritative dark slot and incorrectly block the TERM_PROGRAM + // allow-list. Anything that isn't pure digits falls through. + const lastField = colorfgbg.split(';').at(-1) ?? '' + + if (/^\d+$/.test(lastField)) { + const bg = Number(lastField) + + if (bg === 7 || bg === 15) { + return true + } + + // Slots 0–6 and 8–14 are the dark half of the 0–15 ANSI range. + // When COLORFGBG is set we trust it as authoritative — a non-light + // value here shouldn't get overridden by the TERM_PROGRAM allow-list. + if (bg >= 0 && bg < 16) { + return false + } + } + } + + const termProgram = (env.TERM_PROGRAM ?? '').trim() + + return lightDefaultTermPrograms.has(termProgram) } export const DEFAULT_THEME: Theme = detectLightMode() ? LIGHT_THEME : DARK_THEME