feat(tui): expand light-terminal auto-detection (HERMES_TUI_THEME, background hex) (#17113)

* 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.
This commit is contained in:
brooklyn! 2026-04-28 16:02:06 -07:00 committed by GitHub
parent 1e326c686d
commit 258efb2575
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 260 additions and 27 deletions

View file

@ -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')
})

View file

@ -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<string>()
// 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; 015 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<string> = 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 06 and 814 are the dark half of the 015 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