diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 3b2915a7daf..7953022e6c2 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -94,6 +94,12 @@ def test_voice_toggle_returns_configured_record_key(monkeypatch): check_voice_requirements=lambda: {"available": True, "details": ""} ), ) + # ``voice.toggle`` action=on mutates ``os.environ["HERMES_VOICE"]`` + # directly (CLI parity, runtime-only flag). Take monkeypatch + # ownership of the var so the change is reverted at teardown and + # later tests don't inherit a stale ON state (Copilot round-5 + # review on #19835). + monkeypatch.setenv("HERMES_VOICE", "0") on_resp = server.dispatch( {"id": "voice-on", "method": "voice.toggle", "params": {"action": "on"}} diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index 557dd23e0fe..e481e1a22f4 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -207,6 +207,42 @@ describe('parseVoiceRecordKey (#18994)', () => { expect(parseVoiceRecordKey('alt+c').mod).toBe('alt') expect(parseVoiceRecordKey('cmd+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 () => { + 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+ + // bindings silently fire on Alt+ / bare Esc. + const cmdB = parseVoiceRecordKey('cmd+b') + const cmdSpace = parseVoiceRecordKey('cmd+space') + const cmdEscape = parseVoiceRecordKey('cmd+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) + }) + + it('rejects matches when Shift is held (different chord than configured)', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + + // Parser rejects multi-modifier configs like ``ctrl+shift+tab``, + // so the runtime matcher must also reject Shift-held events — + // otherwise ``ctrl+tab`` would fire on Ctrl+Shift+Tab. + const ctrlTab = parseVoiceRecordKey('ctrl+tab') + const altEnter = parseVoiceRecordKey('alt+enter') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false, tab: true }, '', ctrlTab)).toBe(false) + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, return: true, shift: true, super: false }, '', altEnter)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false }, 'o', ctrlO)).toBe(false) + + // Sanity: same events without Shift still fire. + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false, tab: true }, '', ctrlTab)).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false }, 'o', ctrlO)).toBe(true) + }) }) describe('formatVoiceRecordKey (#18994)', () => { @@ -332,15 +368,17 @@ 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 matches key.super OR (macOS) key.meta', async () => { + it('cmd+b renders "Cmd+B" and requires the literal key.super bit', async () => { const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') const cmdB = parseVoiceRecordKey('cmd+b') expect(formatVoiceRecordKey(cmdB)).toBe('Cmd+B') - // Kitty-style: key.super + // Kitty-style: key.super fires the binding. expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', cmdB)).toBe(true) - // Legacy macOS terminal: key.meta - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).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 + // (Copilot round-5 review on #19835). + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(false) // Ctrl held at the same time → reject (different chord). expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false) }) diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index bdfd3a84b92..b2d0249b628 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -140,6 +140,7 @@ interface RuntimeKeyEvent { escape?: boolean meta: boolean return?: boolean + shift?: boolean super?: boolean tab?: boolean } @@ -299,6 +300,15 @@ export const isVoiceToggleKey = ( return false } + // The parser rejects multi-modifier configs (``ctrl+shift+b`` etc.), + // so at match time Shift must always be clear — otherwise + // ``ctrl+tab`` would also fire on Ctrl+Shift+Tab and ``alt+enter`` + // on Alt+Shift+Enter, triggering a different chord than configured + // (Copilot round-5 review on #19835). + if (key.shift === true) { + return false + } + switch (configured.mod) { case 'alt': // Most terminals surface Alt as either ``alt`` or ``meta``; accept @@ -315,9 +325,15 @@ export const isVoiceToggleKey = ( // some macOS terminals) and ``ctrl+space`` would fire on Alt+Space. return key.ctrl || (_isDefaultVoiceKey(configured) && isActionMod(key)) case 'super': - // Kitty-style protocol surfaces Cmd as ``key.super``; legacy macOS - // terminals still surface it as ``key.meta``. Accept both but - // require the ctrl bit to be clear so Ctrl+Cmd+ doesn't match. - return (key.super === true || (isMac && key.meta)) && !key.ctrl + // 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 } }