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) {