From 02374795a0750a74ad1dbb9f5cabe3a1640c993c Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 4 May 2026 14:13:18 -0500 Subject: [PATCH] fix(tui): address Copilot round-6 review on #19835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three classes of modifier-discipline tightening + one config-surface honesty fix: * **Default ``ctrl+b`` Cmd fallback leaked Alt+B.** The default's macOS Cmd+B muscle-memory path used ``isActionMod(key)``, which returns ``key.meta || key.super`` on darwin. hermes-ink also reports plain Alt as ``key.meta``, so Alt+B silently fired the default binding. Replaced with strict ``isMac && key.super === true`` — kitty-style Cmd+B still works, Alt+B correctly rejected. Legacy-terminal mac users (Terminal.app without CSI-u) now get raw Ctrl+B only; the documented default still works everywhere. * **ctrl / super branches accepted extra modifier bits.** The parser rejects multi-modifier configs like ``ctrl+alt+o``, but the runtime matcher was permissive — ``ctrl+o`` fired on Ctrl+Alt+O / Ctrl+Cmd+O, and ``super+b`` fired on Cmd+Alt+B / Ctrl+Cmd+B. Added strict ``!key.alt && !key.meta && key.super !== true`` on ctrl, and ``!key.ctrl && !key.alt && !key.meta`` on super, so the runtime only fires the exact chord the parser would let you configure. * **Dropped ``cmd`` / ``command`` aliases.** They parsed to ``super`` and rendered as ``Cmd+X``, but legacy macOS terminals report Cmd as ``key.meta`` (same signal as Alt), so a ``cmd+o`` config was advertised as working but never actually fired on Terminal.app-without-CSI-u. That recreated the "displayed shortcut does not work" problem this PR was meant to remove. Users who want the platform action modifier spell it ``super`` / ``win`` — that matches the unambiguous ``key.super`` bit, and kitty-style macOS terminals render it as ``Cmd+X`` via platform-aware formatter. Coverage updated: * Default ctrl+b no longer fires on Alt+B via ``key.meta`` leak; raw Ctrl+B and kitty-style Cmd+B still fire. * ``ctrl+o`` rejects Ctrl+Alt+O / Ctrl+Cmd+O / Ctrl+Meta+O chords. * ``super+b`` rejects Cmd+Alt+B / Cmd+Meta+B / Ctrl+Cmd+B chords. * ``cmd+b`` / ``command+b`` / ``meta+b`` all fall back to the documented default at parse time (joined the ambiguous-mac-mod rejection class). * Round-2 expectations that asserted ``cmd+b`` parsed as super and accepted ``key.meta`` on darwin updated to reflect the new stricter contract. Suite: 588/588 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. --- ui-tui/src/__tests__/platform.test.ts | 130 ++++++++++++++++++-------- ui-tui/src/lib/platform.ts | 59 +++++++----- 2 files changed, 127 insertions(+), 62 deletions(-) diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index e481e1a22f4..aa73de75a89 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -67,11 +67,15 @@ describe('isVoiceToggleKey', () => { expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'B')).toBe(true) }) - it('matches Cmd+B on macOS (preserve platform muscle memory)', async () => { + it('matches kitty-style Cmd+B on macOS via key.super', async () => { const { isVoiceToggleKey } = await importPlatform('darwin') - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b')).toBe(true) expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b')).toBe(true) + // ``key.meta`` is NOT accepted as Cmd — hermes-ink uses meta for + // Alt too, so accepting it leaked Alt+B into the default binding + // (Copilot round-6 review on #19835). Legacy-terminal mac users + // get strict Ctrl+B. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b')).toBe(false) }) it('matches Ctrl+B on non-macOS platforms', async () => { @@ -103,26 +107,27 @@ describe('parseVoiceRecordKey (#18994)', () => { expect(parseVoiceRecordKey('Ctrl+R')).toEqual({ ch: 'r', mod: 'ctrl', raw: 'ctrl+r' }) }) - it('parses alt/cmd/super aliases', async () => { + it('parses alt/super aliases', async () => { const { parseVoiceRecordKey } = await importPlatform('linux') expect(parseVoiceRecordKey('alt+b').mod).toBe('alt') expect(parseVoiceRecordKey('option+b').mod).toBe('alt') - // ``cmd``/``command`` collapse onto ``super`` — hermes-ink's ``key.meta`` - // means Alt on most terminals, so mapping ``cmd`` to a distinct ``meta`` - // mod would make the display lie about the actual match target. - expect(parseVoiceRecordKey('cmd+b').mod).toBe('super') - expect(parseVoiceRecordKey('command+b').mod).toBe('super') expect(parseVoiceRecordKey('super+b').mod).toBe('super') expect(parseVoiceRecordKey('win+b').mod).toBe('super') }) - it('treats a bare "meta+b" as unrecognised (falls back to Ctrl+B)', async () => { + it('treats ambiguous mac modifiers (meta / cmd / command) as unrecognised', async () => { const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') - // ``meta`` is ambiguous on the wire (Alt on xterm, Cmd on legacy - // macOS). Rather than pick the wrong one, refuse it. + // ``meta`` / ``cmd`` / ``command`` are ambiguous on the wire: + // hermes-ink sets ``key.meta`` for plain Alt on every platform AND + // for Cmd on legacy macOS terminals. Accepting any of them would + // produce a display/binding mismatch (Copilot round-6 review on + // #19835). Users on modern kitty-style terminals spell the + // platform action modifier ``super`` / ``win``. expect(parseVoiceRecordKey('meta+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('cmd+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('command+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) }) it('parses named keys (space, enter, tab, escape, backspace, delete)', async () => { @@ -205,24 +210,64 @@ describe('parseVoiceRecordKey (#18994)', () => { // Alt- and super-modifier versions of those letters are NOT // intercepted, so they remain usable. expect(parseVoiceRecordKey('alt+c').mod).toBe('alt') - expect(parseVoiceRecordKey('cmd+l').mod).toBe('super') + expect(parseVoiceRecordKey('super+l').mod).toBe('super') }) // Round-5 Copilot review regressions on #19835. - it('cmd+ does NOT fire on key.meta-only events (Alt+X false-fire guard)', async () => { + it('super+ does NOT fire on key.meta-only events (Alt+X false-fire guard)', async () => { const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') // hermes-ink sets ``key.meta`` for Alt/Option AND for bare Esc on // some macOS terminals. The super branch used to accept - // ``isMac && key.meta`` as a Cmd fallback, which made cmd+ + // ``isMac && key.meta`` as a Cmd fallback, which made super+ // bindings silently fire on Alt+ / bare Esc. - const cmdB = parseVoiceRecordKey('cmd+b') - const cmdSpace = parseVoiceRecordKey('cmd+space') - const cmdEscape = parseVoiceRecordKey('cmd+escape') + const superB = parseVoiceRecordKey('super+b') + const superSpace = parseVoiceRecordKey('super+space') + const superEscape = parseVoiceRecordKey('super+escape') - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(false) - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', cmdSpace)).toBe(false) - expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', cmdEscape)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', superSpace)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', superEscape)).toBe(false) + }) + + // Round-6 Copilot review regressions on #19835. + it('default ctrl+b does NOT fire on Alt+B via isActionMod meta leak', async () => { + const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin') + + // ``isActionMod(key)`` on darwin was accepting ``key.meta`` as the + // action modifier, so Alt+B (key.meta=true) fired the default + // ctrl+b binding. Now the Cmd-fallback path requires literal + // ``key.super`` on macOS and rejects ``key.meta``. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(false) + // Literal Ctrl+B and Cmd+B (kitty-style) still work on darwin. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + }) + + it('ctrl+ rejects chords with extra alt / meta / super bits', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + // ``ctrl+o`` must fire ONLY on literal Ctrl+O, not on + // Ctrl+Alt+O / Ctrl+Cmd+O / Ctrl+Meta+O — otherwise the runtime + // matches a different chord than the parser would let you + // configure. + expect(isVoiceToggleKey({ alt: true, ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: true, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'o', ctrlO)).toBe(false) + // Sanity: plain Ctrl+O still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) + }) + + it('super+ rejects chords with extra ctrl / alt / meta bits', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const superB = parseVoiceRecordKey('super+b') + + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, super: true }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: true }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', superB)).toBe(false) + // Sanity: plain Super+B still fires. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', superB)).toBe(true) }) it('rejects matches when Shift is held (different chord than configured)', async () => { @@ -252,10 +297,10 @@ describe('formatVoiceRecordKey (#18994)', () => { expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+b'))).toBe('Ctrl+B') expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+o'))).toBe('Ctrl+O') expect(formatVoiceRecordKey(parseVoiceRecordKey('alt+r'))).toBe('Alt+R') - // ``cmd``/``command``/``super``/``win`` all collapse onto the super - // modifier and render as ``Super`` on non-mac so the hint doesn't - // tell Linux/Windows users to press a Cmd key they don't have. - expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Super+B') + // ``super``/``win`` render as ``Super`` on non-mac so the hint + // doesn't tell Linux/Windows users to press a Cmd key they don't + // have. + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Super+B') }) it('renders named keys in title case (Ctrl+Space, Ctrl+Enter)', async () => { @@ -264,6 +309,7 @@ describe('formatVoiceRecordKey (#18994)', () => { expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+space'))).toBe('Ctrl+Space') expect(formatVoiceRecordKey(parseVoiceRecordKey('alt+enter'))).toBe('Alt+Enter') expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+esc'))).toBe('Ctrl+Escape') + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+space'))).toBe('Super+Space') }) }) @@ -349,12 +395,17 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, ' ', ctrlSpace)).toBe(true) }) - it('default ctrl+b still accepts Cmd+B on macOS (muscle-memory preserved)', async () => { + it('default ctrl+b accepts raw Ctrl+B and kitty-style Cmd+B on macOS', async () => { const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin') - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) - expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + // Raw Ctrl+B: always works. expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + // Cmd+B via kitty-style ``key.super``: still works. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + // Cmd+B via legacy ``key.meta`` NO LONGER works — ``key.meta`` is + // hermes-ink's Alt signal, so accepting it leaked Alt+B into the + // default binding (Copilot round-6 review on #19835). + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(false) }) it('custom ctrl+ does NOT accept Cmd fallback on macOS', async () => { @@ -368,19 +419,19 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) }) - it('cmd+b renders "Cmd+B" and requires the literal key.super bit', async () => { + it('super+b renders "Cmd+B" on darwin and requires the literal key.super bit', async () => { const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') - const cmdB = parseVoiceRecordKey('cmd+b') + const superB = parseVoiceRecordKey('super+b') - expect(formatVoiceRecordKey(cmdB)).toBe('Cmd+B') + expect(formatVoiceRecordKey(superB)).toBe('Cmd+B') // Kitty-style: key.super fires the binding. - expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', cmdB)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', superB)).toBe(true) // ``key.meta`` is NOT accepted — hermes-ink uses meta for Alt too, - // so accepting it here would make cmd+b silently fire on Alt+B + // so accepting it here would make super+b silently fire on Alt+B // (Copilot round-5 review on #19835). - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', superB)).toBe(false) // Ctrl held at the same time → reject (different chord). - expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', superB)).toBe(false) }) // Round-2 Copilot review regressions on #19835. @@ -389,7 +440,6 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Super+B') expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Super+B') - expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Super+B') }) it('super+b still renders "Cmd+B" on macOS', async () => { @@ -405,11 +455,15 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { const spacedB = parseVoiceRecordKey('ctrl + b') // Both parse to the documented default semantically; both must keep - // the macOS Cmd+B muscle-memory fallback. - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', controlB)).toBe(true) - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', spacedB)).toBe(true) + // the macOS Cmd+B muscle-memory fallback via kitty-style key.super. + // ``key.meta`` is NOT accepted — that's hermes-ink's Alt signal + // (round-6 review), so legacy-terminal users get strict Ctrl+B. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', controlB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', spacedB)).toBe(false) expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', controlB)).toBe(true) expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', spacedB)).toBe(true) + // Literal Ctrl+B still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', controlB)).toBe(true) // And still reject a ctrl bit on a different letter. expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', controlB)).toBe(false) }) diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index b2d0249b628..1bdbfd3ac2d 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -85,16 +85,22 @@ export const DEFAULT_VOICE_RECORD_KEY: ParsedVoiceRecordKey = { raw: 'ctrl+b' } -/** Modifier aliases. ``meta`` is intentionally absent: hermes-ink sets - * ``key.meta`` for plain Alt/Option (and on some legacy terminals for Cmd - * too), so accepting a literal ``meta+b`` config would display as ``Cmd+B`` - * but match Alt+B on the wire — the kind of lie this fix was meant to - * remove. Users who want the platform action modifier spell it ``cmd`` / - * ``command`` / ``super`` / ``win``. */ +/** Modifier aliases. + * + * ``meta`` / ``cmd`` / ``command`` are intentionally absent. + * hermes-ink sets ``key.meta`` for plain Alt/Option on every platform + * AND for Cmd on some legacy macOS terminals (Terminal.app without + * kitty-protocol passthrough). Accepting any of those as a literal + * modifier would produce a display/binding mismatch — a config like + * ``cmd+b`` would render as ``Cmd+B`` but silently fire on Alt+B, or + * never fire at all on legacy terminals even though the UI advertises + * it (Copilot round-6 review on #19835). Users on modern kitty-style + * terminals (iTerm2 CSI-u, Ghostty, Kitty, WezTerm, Alacritty) spell + * the platform action modifier ``super`` / ``win``, which match the + * unambiguous ``key.super`` bit. macOS users on Terminal.app stick + * with the documented ``ctrl+b``. */ const _MOD_ALIASES: Record = { alt: 'alt', - cmd: 'super', - command: 'super', control: 'ctrl', ctrl: 'ctrl', option: 'alt', @@ -318,22 +324,27 @@ export const isVoiceToggleKey = ( // alt binding. return (key.alt === true || key.meta) && !key.ctrl && key.super !== true case 'ctrl': - // Only the documented default (``ctrl+b``) gets the macOS - // Cmd-as-action-modifier fallback. For any user-configured binding - // we require the literal Ctrl bit — otherwise ``ctrl+escape`` would - // fire on bare Esc (hermes-ink sets ``key.meta`` for bare Esc on - // some macOS terminals) and ``ctrl+space`` would fire on Alt+Space. - return key.ctrl || (_isDefaultVoiceKey(configured) && isActionMod(key)) + // Require the Ctrl bit AND a clear Alt/Super so a chord like + // Ctrl+Alt+ / Ctrl+Cmd+ doesn't spuriously match + // ``ctrl+`` (Copilot round-6 review on #19835). + // + // The documented default (``ctrl+b``) additionally accepts the + // explicit ``key.super`` bit on macOS for Cmd+B muscle memory — + // but ONLY ``key.super`` (kitty-style), never ``key.meta``, since + // ``key.meta`` is hermes-ink's Alt signal and accepting it would + // fire the binding on Alt+B. + if (key.ctrl) { + return !key.alt && !key.meta && key.super !== true + } + + return _isDefaultVoiceKey(configured) && isMac && key.super === true && !key.alt && !key.meta case 'super': - // Require the explicit ``key.super`` bit (kitty-style protocol). - // The previous ``isMac && key.meta`` fallback was intended for - // legacy macOS terminals that report Cmd as ``meta``, but - // hermes-ink also sets ``key.meta`` for plain Alt/Option and even - // bare Escape — so a ``cmd+b`` config silently fired on Alt+B - // and ``cmd+escape`` on bare Esc (Copilot round-5 review on - // #19835). Legacy-terminal users who want the Cmd shortcut - // should upgrade to a kitty-protocol terminal or bind ``alt+X`` - // explicitly. - return key.super === true && !key.ctrl + // Require the explicit ``key.super`` bit (kitty-style protocol) + // AND clear Ctrl/Alt/Meta so Ctrl+Cmd+X or Alt+Cmd+X don't + // spuriously fire the super binding (Copilot round-6 review on + // #19835). Legacy-terminal users whose Cmd arrives as + // ``key.meta`` need a kitty-protocol terminal — see the + // _MOD_ALIASES doc-comment for the rationale. + return key.super === true && !key.ctrl && !key.alt && !key.meta } }