From 1a4d990d6b7ad4c3907a8cbe98dee86372a40075 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 4 May 2026 13:32:08 -0500 Subject: [PATCH] fix(tui): address Copilot round-4 review on #19835 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four final corners of the voice.record_key surface: * **Bare-char configs silently coerced to ``ctrl+``.** A config like ``voice.record_key: o`` / ``space`` / ``escape`` fell through to the default ``mod = 'ctrl'`` and silently bound Ctrl+O, while the classic CLI's prompt_toolkit would bind the raw key (no rewrite) — so the two runtimes silently disagreed on what "o" means. Require an explicit modifier; bare-char configs fall back to the documented Ctrl+B default. * **Reserved ctrl+ bindings would never fire.** ``useInputHandlers()`` intercepts ``ctrl+c`` (interrupt), ``ctrl+d`` (quit), and ``ctrl+l`` (clear screen) before the voice check runs, so those configs would be advertised in /voice status but the advertised shortcut never actually triggers push-to-talk. Added ``_RESERVED_CTRL_CHARS`` at parse time so the user gets the documented default instead of a dead shortcut. (``alt+c``, ``cmd+l``, etc. are not intercepted and stay usable.) * **``_load_cfg()`` root itself may be a non-dict.** ``_voice_record_key()`` isinstance-guarded the ``voice`` subkey but not the root — a malformed config.yaml that collapsed to a scalar/list at the top level (``config.yaml: true`` or ``[]``) would still raise on ``.get("voice")``. Added the top-level guard too so every malformed shape falls back to ``ctrl+b``. * **Stale header comment on ``isVoiceToggleKey``.** The doc-comment still claimed "On macOS we additionally accept the platform action modifier (Cmd) for the configured letter" even though the implementation gates the Cmd fallback to the documented default only. Rewrote to match. Coverage added: * ``parseVoiceRecordKey`` fallback on bare chars (``o``, ``b``, ``space``, ``escape``). * ``parseVoiceRecordKey`` fallback on ``ctrl+c`` / ``ctrl+d`` / ``ctrl+l``; positive case for ``alt+c`` / ``cmd+l`` still usable. * Backend ``test_voice_toggle_handles_non_dict_voice_cfg`` now exercises 5 non-dict shapes at the YAML root too. Suite: 583/583 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. --- tests/test_tui_gateway_server.py | 15 +++++++++ tui_gateway/server.py | 18 +++++++---- ui-tui/src/__tests__/platform.test.ts | 28 ++++++++++++++++ ui-tui/src/lib/platform.ts | 46 +++++++++++++++++++-------- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index d22691fd5ee2..3b2915a7dafa 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -134,6 +134,21 @@ def test_voice_toggle_handles_non_dict_voice_cfg(monkeypatch): f"voice.record_key fell back to default for voice={bad!r}" ) + # Round-4 follow-up: the YAML root itself may be a non-dict. A + # hand-edit that collapses config.yaml to a scalar / list would + # otherwise crash ``.get("voice")`` before the inner isinstance + # guard gets a chance to run. + for bad_root in (True, None, [], "ctrl+b", 42): + monkeypatch.setattr(server, "_load_cfg", lambda r=bad_root: r) + + status_resp = server.dispatch( + {"id": "voice-status-root", "method": "voice.toggle", "params": {"action": "status"}} + ) + + assert status_resp["result"]["record_key"] == "ctrl+b", ( + f"voice.record_key fell back to default for root={bad_root!r}" + ) + def test_voice_toggle_tts_branch_also_carries_record_key(monkeypatch): """Round-2 Copilot review regression on #19835. diff --git a/tui_gateway/server.py b/tui_gateway/server.py index e54af11a39d6..50de17d5c193 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -5281,14 +5281,18 @@ def _voice_tts_enabled() -> bool: def _voice_record_key() -> str: """Current ``voice.record_key`` value, documented default on error. - ``_load_cfg()`` returns raw ``yaml.safe_load()`` output, so ``voice`` - may be any scalar — a hand-edited ``voice: true`` or ``voice: cmd+b`` - (string where a dict is expected) would break ``.get("record_key")`` - and take every ``voice.toggle`` branch down with it (Copilot round-3 - review on #19835). Coerce through ``isinstance`` so malformed config - falls back to the documented default instead of crashing /voice. + ``_load_cfg()`` returns raw ``yaml.safe_load()`` output, so both the + root AND ``voice`` may be any YAML scalar / list / None. A hand-edit + like ``voice: true`` or ``voice: cmd+b`` (string where a dict is + expected) or even a malformed top-level config that parses to a + scalar would otherwise break ``.get("record_key")`` and take every + ``voice.toggle`` branch down with it (Copilot round-3/round-4 + review on #19835). Coerce through ``isinstance`` at every level so + malformed config falls back to the documented default instead of + crashing /voice. """ - voice_cfg = _load_cfg().get("voice") + cfg = _load_cfg() + voice_cfg = cfg.get("voice") if isinstance(cfg, dict) else None record_key = voice_cfg.get("record_key") if isinstance(voice_cfg, dict) else None return str(record_key) if isinstance(record_key, str) and record_key else "ctrl+b" diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index 709906811a92..557dd23e0fef 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -179,6 +179,34 @@ describe('parseVoiceRecordKey (#18994)', () => { expect(parseVoiceRecordKey('cmd+ctrl+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) expect(parseVoiceRecordKey('alt+ctrl+space')).toEqual(DEFAULT_VOICE_RECORD_KEY) }) + + // Round-4 Copilot review regressions on #19835. + it('rejects bare-char configs without an explicit modifier', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // The classic CLI's prompt_toolkit binds raw-char configs to the key + // itself (``c-o`` requires an explicit modifier); rewriting ``o`` + // → ``ctrl+o`` would silently diverge the two runtimes. Refuse. + expect(parseVoiceRecordKey('o')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('space')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('escape')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('rejects ctrl+c / ctrl+d / ctrl+l — reserved by the TUI input handler', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``useInputHandlers()`` intercepts these before the voice check, + // so a binding like ``ctrl+c`` would be advertised but never fire. + // Fall back to the documented default instead of lying to the user. + expect(parseVoiceRecordKey('ctrl+c')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('ctrl+d')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('ctrl+l')).toEqual(DEFAULT_VOICE_RECORD_KEY) + // 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') + }) }) describe('formatVoiceRecordKey (#18994)', () => { diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index e37771e8f6dc..bdfd3a84b92f 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -58,9 +58,9 @@ export const isCopyShortcut = ( * config.yaml default. The TUI honours the same config knob (#18994); * when ``voice.record_key`` is e.g. ``ctrl+o`` the TUI binds Ctrl+O. * - * On macOS we additionally accept the platform action modifier (Cmd) for - * the configured letter so existing macOS muscle memory keeps working - * alongside the documented Ctrl+ shortcut. + * Only the documented default (``ctrl+b``) additionally accepts the + * macOS action modifier (Cmd+B) — custom bindings like ``ctrl+o`` + * require the literal Ctrl bit so Cmd+O can't steal the shortcut. */ export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'super' @@ -124,6 +124,14 @@ const _NAMED_KEY_ALIASES: Record = { tab: 'tab' } +/** ``useInputHandlers()`` intercepts these before the voice check runs, + * so a binding like ``ctrl+c`` (interrupt), ``ctrl+d`` (quit), or + * ``ctrl+l`` (clear screen) would be advertised in /voice status but + * never actually fire push-to-talk. Reject at parse time so the user + * gets the documented Ctrl+B instead of a dead shortcut (Copilot + * round-4 review on #19835). */ +const _RESERVED_CTRL_CHARS = new Set(['c', 'd', 'l']) + interface RuntimeKeyEvent { alt?: boolean backspace?: boolean @@ -205,19 +213,31 @@ export const parseVoiceRecordKey = (raw: unknown): ParsedVoiceRecordKey => { return DEFAULT_VOICE_RECORD_KEY } - let mod: VoiceRecordKeyMod = 'ctrl' + // Require an explicit modifier. A bare ``o`` / ``space`` / ``escape`` + // has no sensible mapping: the CLI's prompt_toolkit binds the raw + // key (no rewrite) so bare-char configs would silently diverge + // between the two runtimes (Copilot round-4 review on #19835). + // Fall back to the documented default. + if (modCandidates.length === 0) { + return DEFAULT_VOICE_RECORD_KEY + } - if (modCandidates.length === 1) { - const norm = _MOD_ALIASES[modCandidates[0]] + const norm = _MOD_ALIASES[modCandidates[0]] - // Unknown modifier token (e.g. bare ``meta+b`` which is ambiguous on - // the wire) falls back to the documented default rather than - // silently coercing to Ctrl and producing a misleading bind. - if (!norm) { - return DEFAULT_VOICE_RECORD_KEY - } + // Unknown modifier token (e.g. bare ``meta+b`` which is ambiguous on + // the wire) falls back to the documented default rather than + // silently coercing to Ctrl and producing a misleading bind. + if (!norm) { + return DEFAULT_VOICE_RECORD_KEY + } - mod = norm + const mod = norm + + // Block bindings the TUI input handler intercepts before the voice + // check — ``ctrl+c`` / ``ctrl+d`` / ``ctrl+l`` would never actually + // fire push-to-talk, so advertising them in /voice status is a lie. + if (mod === 'ctrl' && last.length === 1 && _RESERVED_CTRL_CHARS.has(last)) { + return DEFAULT_VOICE_RECORD_KEY } if (last.length === 1) {