fix(tui): address Copilot round-4 review on #19835

Four final corners of the voice.record_key surface:

* **Bare-char configs silently coerced to ``ctrl+<key>``.** 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+<letter> 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.
This commit is contained in:
Brooklyn Nicholson 2026-05-04 13:32:08 -05:00
parent 14f61bbd63
commit 1a4d990d6b
4 changed files with 87 additions and 20 deletions

View file

@ -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.

View file

@ -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"

View file

@ -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)', () => {

View file

@ -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+<letter> 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<string, VoiceRecordKeyNamed> = {
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) {