From 674b1030c1ac717c5a78c7699d0fc0f30b7a9dab Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 4 May 2026 12:56:25 -0500 Subject: [PATCH] fix(tui): address Copilot round-2 review on #19835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three tightenings on the surface introduced in the round-1 fix: * **``/voice tts`` reset custom bindings to Ctrl+B.** The ``tts`` branch of ``voice.toggle`` omitted ``record_key`` from its response, so the frontend's ``r.record_key ?? 'ctrl+b'`` coerced a user's custom binding back to the default on every TTS toggle. Two-sided fix: the backend now includes ``record_key`` on the ``tts`` branch (parity with ``status``/``on``/``off``), and the slash handler only pushes frontend state when the response actually carries ``record_key`` — belt-and-suspenders against any future branch forgetting to include it. * **``super+b`` / ``win+b`` / ``cmd+b`` displayed "Cmd+B" on Linux and Windows.** ``formatVoiceRecordKey`` rendered ``mod === 'super'`` as ``Cmd`` universally, which told non-mac users the wrong modifier to press even though ``isVoiceToggleKey`` matched the right event bits. Gate the label to ``isMac`` so non-mac renders ``Super+B``. * **``control+b`` / ``ctrl + b`` lost the macOS Cmd+B fallback.** ``_isDefaultVoiceKey`` keyed off ``parsed.raw`` — so semantically-equal aliases of the documented default dropped into the strict branch even though they bind Ctrl+B. Compare on the parsed spec (mod + ch + named) instead. Coverage added: Linux ``Super+B`` rendering (and macOS ``Cmd+B``), ``control+b`` / ``ctrl + b`` accepting the Cmd+B fallback on darwin, ``/voice tts`` without ``record_key`` not clobbering cached binding, and a backend regression asserting every ``voice.toggle`` branch carries the configured key. Suite: 579/579 TUI vitest green, 2/2 backend voice tests green, tsc --noEmit clean. --- tests/test_tui_gateway_server.py | 32 +++++++++++++++++ tui_gateway/server.py | 15 +++++++- .../src/__tests__/createSlashHandler.test.ts | 16 +++++++++ ui-tui/src/__tests__/platform.test.ts | 36 ++++++++++++++++++- ui-tui/src/app/slash/commands/session.ts | 16 +++++++-- ui-tui/src/lib/platform.ts | 19 ++++++++-- 6 files changed, 126 insertions(+), 8 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index b0cd9667643..16af398c533 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -106,6 +106,38 @@ def test_voice_toggle_returns_configured_record_key(monkeypatch): assert status_resp["result"]["record_key"] == "ctrl+o" +def test_voice_toggle_tts_branch_also_carries_record_key(monkeypatch): + """Round-2 Copilot review regression on #19835. + + The ``tts`` branch used to omit ``record_key`` from its response, so a + TUI client would parse ``r.record_key ?? 'ctrl+b'`` and reset a + custom binding to the default on every TTS toggle. Every branch of + ``voice.toggle`` now carries the configured key so frontend state + stays authoritative. + """ + monkeypatch.setattr( + server, + "_load_cfg", + lambda: {"voice": {"record_key": "ctrl+space"}}, + ) + monkeypatch.setitem( + sys.modules, + "tools.voice_mode", + types.SimpleNamespace( + check_voice_requirements=lambda: {"available": True, "details": ""} + ), + ) + monkeypatch.setenv("HERMES_VOICE", "1") + monkeypatch.delenv("HERMES_VOICE_TTS", raising=False) + + tts_resp = server.dispatch( + {"id": "voice-tts", "method": "voice.toggle", "params": {"action": "tts"}} + ) + + assert tts_resp["result"]["record_key"] == "ctrl+space" + assert tts_resp["result"]["tts"] is True + + def test_load_enabled_toolsets_prefers_tui_env(monkeypatch): monkeypatch.setenv("HERMES_TUI_TOOLSETS", "web, terminal, ,memory") diff --git a/tui_gateway/server.py b/tui_gateway/server.py index b5576649e30..95ce8298278 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -5360,7 +5360,20 @@ def _(rid, params: dict) -> dict: new_value = not _voice_tts_enabled() # Runtime-only flag (CLI parity) — see voice.toggle on/off above. os.environ["HERMES_VOICE_TTS"] = "1" if new_value else "0" - return _ok(rid, {"enabled": True, "tts": new_value}) + # Include ``record_key`` on every branch so a /voice tts toggle + # doesn't reset the TUI's cached shortcut to the default when a + # user has a custom binding configured (Copilot review, round 2 + # on #19835). Keeps parity with the status/on/off branches above. + return _ok( + rid, + { + "enabled": True, + "record_key": str( + (_load_cfg().get("voice") or {}).get("record_key") or "ctrl+b" + ), + "tts": new_value, + }, + ) return _err(rid, 4013, f"unknown voice action: {action}") diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index 18d4d2f16d6..c9447f16d81 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -215,6 +215,22 @@ describe('createSlashHandler', () => { }) }) + // Round-2 Copilot review on #19835: a response missing ``record_key`` + // (e.g. the old tts branch, or any future branch that forgets to + // include it) MUST NOT clobber the user's cached binding back to + // Ctrl+B. The label still renders the default for display; the + // frontend state keeps whatever was last authoritatively set. + it('/voice tts without record_key does not clobber cached frontend binding', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, tts: true })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice tts')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice TTS enabled.') + }) + expect(ctx.voice.setVoiceRecordKey).not.toHaveBeenCalled() + }) + it('cycles details mode and persists it', async () => { const ctx = buildCtx() diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index 872710405d1..b2757b71bdb 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -164,7 +164,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') - expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Cmd+B') + // ``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') }) it('renders named keys in title case (Ctrl+Space, Ctrl+Enter)', async () => { @@ -289,6 +292,37 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { // Ctrl held at the same time → reject (different chord). expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false) }) + + // Round-2 Copilot review regressions on #19835. + it('super+b renders "Super+B" on Linux (not "Cmd+B")', async () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('linux') + + 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 () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('darwin') + + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Cmd+B') + expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Cmd+B') + }) + + it('ctrl+b aliases (control+b, "ctrl + b") still accept Cmd+B fallback on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const controlB = parseVoiceRecordKey('control+b') + 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) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', controlB)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', spacedB)).toBe(true) + // And still reject a ctrl bit on a different letter. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', controlB)).toBe(false) + }) }) describe('isMacActionFallback', () => { diff --git a/ui-tui/src/app/slash/commands/session.ts b/ui-tui/src/app/slash/commands/session.ts index c371f383444..a75419c3b02 100644 --- a/ui-tui/src/app/slash/commands/session.ts +++ b/ui-tui/src/app/slash/commands/session.ts @@ -232,9 +232,19 @@ export const sessionCommands: SlashCommand[] = [ // state would skew display and binding between config-edit and // the next ``mtime`` poll (~5s). Parse once, push into state so // ``useInputHandlers()`` picks up the new binding immediately. - const parsed = parseVoiceRecordKey(r.record_key ?? 'ctrl+b') - ctx.voice.setVoiceRecordKey(parsed) - const recordKeyLabel = formatVoiceRecordKey(parsed) + // + // Round-2 follow-up: only push state when the response actually + // carries ``record_key`` — otherwise an older gateway (or a future + // branch that forgets to include it) would clobber a custom user + // binding back to the default on every /voice invocation. The + // label still falls back to the documented default for display. + const parsed = r.record_key ? parseVoiceRecordKey(r.record_key) : undefined + + if (parsed) { + ctx.voice.setVoiceRecordKey(parsed) + } + + const recordKeyLabel = formatVoiceRecordKey(parsed ?? parseVoiceRecordKey('ctrl+b')) // Match CLI's _show_voice_status / _enable_voice_mode / // _toggle_voice_tts output shape so users don't have to learn diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index 6da69ea10ff..8b77f7989a8 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -218,9 +218,15 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => { return DEFAULT_VOICE_RECORD_KEY } -/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. */ +/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. + * + * Platform-aware for the ``super`` modifier: renders ``Cmd`` on macOS and + * ``Super`` elsewhere. Previously rendered ``Cmd`` universally, which told + * Linux/Windows users the wrong modifier to press (Copilot review, round + * 2 on #19835). */ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => { - const modLabel = parsed.mod === 'super' ? 'Cmd' : parsed.mod[0].toUpperCase() + parsed.mod.slice(1) + const modLabel = + parsed.mod === 'super' ? (isMac ? 'Cmd' : 'Super') : parsed.mod[0].toUpperCase() + parsed.mod.slice(1) // Named tokens render in title case (Ctrl+Space, Ctrl+Enter); single // chars render upper-case to match the existing Ctrl+B convention. const keyLabel = parsed.named @@ -230,8 +236,15 @@ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => { return `${modLabel}+${keyLabel}` } +/** Whether the parsed binding is the documented default (ctrl+b). + * + * Compare on the parsed spec rather than ``raw`` so semantically-equal + * aliases (``control+b``, ``ctrl + b``) still get the macOS Cmd+B + * muscle-memory fallback (Copilot review, round 2 on #19835). */ const _isDefaultVoiceKey = (parsed: ParsedVoiceRecordKey): boolean => - parsed.raw === DEFAULT_VOICE_RECORD_KEY.raw + parsed.mod === DEFAULT_VOICE_RECORD_KEY.mod && + parsed.ch === DEFAULT_VOICE_RECORD_KEY.ch && + parsed.named === DEFAULT_VOICE_RECORD_KEY.named export const isVoiceToggleKey = ( key: RuntimeKeyEvent,