diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 16af398c533..d22691fd5ee 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -106,6 +106,35 @@ def test_voice_toggle_returns_configured_record_key(monkeypatch): assert status_resp["result"]["record_key"] == "ctrl+o" +def test_voice_toggle_handles_non_dict_voice_cfg(monkeypatch): + """Round-3 Copilot review regression on #19835. + + ``_load_cfg()`` is raw ``yaml.safe_load()`` output — a hand-edited + ``voice: true`` / ``voice: cmd+b`` / ``voice: null`` leaves ``voice`` + as a bool/str/None, not a dict. Previously ``.get("record_key")`` + on a non-dict broke every ``voice.toggle`` branch. Now it falls + back to the documented default. + """ + monkeypatch.setitem( + sys.modules, + "tools.voice_mode", + types.SimpleNamespace( + check_voice_requirements=lambda: {"available": True, "details": ""} + ), + ) + + for bad in (True, "cmd+b", None, 42, ["ctrl+b"]): + monkeypatch.setattr(server, "_load_cfg", lambda b=bad: {"voice": b}) + + status_resp = server.dispatch( + {"id": "voice-status", "method": "voice.toggle", "params": {"action": "status"}} + ) + + assert status_resp["result"]["record_key"] == "ctrl+b", ( + f"voice.record_key fell back to default for voice={bad!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 95ce8298278..e54af11a39d 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -5278,6 +5278,22 @@ def _voice_tts_enabled() -> bool: return os.environ.get("HERMES_VOICE_TTS", "").strip() == "1" +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. + """ + voice_cfg = _load_cfg().get("voice") + 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" + + @method("voice.toggle") def _(rid, params: dict) -> dict: """CLI parity for the ``/voice`` slash command. @@ -5304,9 +5320,7 @@ def _(rid, params: dict) -> dict: # ignored the config (#18994). payload: dict = { "enabled": _voice_mode_enabled(), - "record_key": str( - (_load_cfg().get("voice") or {}).get("record_key") or "ctrl+b" - ), + "record_key": _voice_record_key(), "tts": _voice_tts_enabled(), } try: @@ -5347,9 +5361,7 @@ def _(rid, params: dict) -> dict: rid, { "enabled": enabled, - "record_key": str( - (_load_cfg().get("voice") or {}).get("record_key") or "ctrl+b" - ), + "record_key": _voice_record_key(), "tts": _voice_tts_enabled(), }, ) @@ -5368,9 +5380,7 @@ def _(rid, params: dict) -> dict: rid, { "enabled": True, - "record_key": str( - (_load_cfg().get("voice") or {}).get("record_key") or "ctrl+b" - ), + "record_key": _voice_record_key(), "tts": new_value, }, ) diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index b2757b71bdb..709906811a9 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -155,6 +155,30 @@ describe('parseVoiceRecordKey (#18994)', () => { expect(parseVoiceRecordKey('ctrl+spcae')).toEqual(DEFAULT_VOICE_RECORD_KEY) expect(parseVoiceRecordKey('ctrl+f5')).toEqual(DEFAULT_VOICE_RECORD_KEY) }) + + // Round-3 Copilot review regressions on #19835. + it('does not throw on non-string YAML scalars — falls back instead', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``config.get full`` surfaces raw YAML values; ``voice.record_key: 1`` + // or ``voice.record_key: true`` would otherwise crash ``.trim()``. + expect(parseVoiceRecordKey(1 as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(true as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(null as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(undefined as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey({} as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('rejects multi-modifier chords rather than silently dropping extras', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // Previously ``ctrl+alt+r`` parsed as ``ctrl+r`` and ``cmd+ctrl+b`` as + // ``super+b`` — a typo silently bound a different shortcut. Now a + // multi-modifier spelling falls back to the documented default. + expect(parseVoiceRecordKey('ctrl+alt+r')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('cmd+ctrl+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('alt+ctrl+space')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) }) describe('formatVoiceRecordKey (#18994)', () => { diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index 8b77f7989a8..e37771e8f6d 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -168,12 +168,19 @@ const _matchesNamedKey = ( * ``delete``) — matching the keys prompt_toolkit accepts on the CLI * side via the ``c-`` rewrite in ``cli.py``. * - * Falls back to the documented Ctrl+B default for empty input or for - * unrecognised multi-character tokens so a typo never silently disables - * the shortcut. + * Accepts ``unknown`` because the source is raw YAML via + * ``config.get full`` — a hand-edited ``voice.record_key: 1`` or + * ``voice.record_key: true`` would otherwise crash ``.trim()`` on a + * non-string scalar (Copilot round-3 review on #19835). Non-string / + * empty / unrecognised values fall back to the documented Ctrl+B + * default so a typo never silently disables the shortcut. */ -export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => { - const lower = (raw ?? '').trim().toLowerCase() +export const parseVoiceRecordKey = (raw: unknown): ParsedVoiceRecordKey => { + if (typeof raw !== 'string') { + return DEFAULT_VOICE_RECORD_KEY + } + + const lower = raw.trim().toLowerCase() if (!lower) { return DEFAULT_VOICE_RECORD_KEY @@ -188,9 +195,19 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => { const last = parts[parts.length - 1] const modCandidates = parts.slice(0, -1) + // Reject multi-modifier chords (``ctrl+alt+r``, ``cmd+ctrl+b``) rather + // than silently dropping the extra modifier — the previous + // single-token validator made a typo bind a different shortcut than + // the user configured (Copilot round-3 review on #19835). The classic + // CLI only supports single-modifier bindings via prompt_toolkit's + // ``c-x`` / ``a-x`` rewrite in ``cli.py``, so this matches CLI parity. + if (modCandidates.length > 1) { + return DEFAULT_VOICE_RECORD_KEY + } + let mod: VoiceRecordKeyMod = 'ctrl' - if (modCandidates.length) { + if (modCandidates.length === 1) { const norm = _MOD_ALIASES[modCandidates[0]] // Unknown modifier token (e.g. bare ``meta+b`` which is ambiguous on