mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-30 11:52:04 +00:00
fix(tui): address Copilot review on #19835 voice.record_key wiring
Five tightenings on the parser + matcher + hydration surface, all caught by the Copilot review on the PR — each one turns a silent false-fire or display/binding skew into a deterministic behaviour. * **isVoiceToggleKey ctrl branch was too permissive for named keys.** The doc-default macOS Cmd+B muscle-memory fallback (``isActionMod(key)`` on top of ``key.ctrl``) fired for every configured key, so bare Esc — which hermes-ink reports with ``key.meta`` on some macOS terminals — triggered ``ctrl+escape``, and Alt+Space / Alt+Tab triggered ``ctrl+space`` / ``ctrl+tab``. Gate the fallback to the literal ``ctrl+b`` binding so any custom chord requires the real Ctrl bit. * **Alt branch guarded against Ctrl/Cmd co-press.** Without this, Ctrl+Alt+<letter> and Cmd+Alt+<letter> also fired ``alt+<letter>``. * **Dropped the ``meta`` modifier variant and its alias.** In hermes-ink ``key.meta`` is Alt on xterm-style terminals and Cmd on legacy macOS ones, so a literal ``meta+b`` config displayed as ``Cmd+B`` while matching Alt+B — exactly the kind of false shortcut the PR was meant to remove. ``cmd`` / ``command`` now collapse onto ``super`` (kitty-style ``key.super``, with a macOS ``key.meta`` fallback) and render as ``Cmd+B``. Unknown modifier tokens fall back to the documented Ctrl+B default rather than silently coercing to Ctrl. * **Slash-command display/binding skew.** ``/voice status`` and ``/voice on`` rendered from the fresh gateway ``record_key`` response, but ``useInputHandlers()`` still bound the old key until the next 5s mtime poll. Thread ``setVoiceRecordKey`` through ``SlashHandlerContext.voice`` and push the parsed spec into frontend state on every response so text and binding stay consistent. * **Test coverage for the two paths Copilot flagged.** Added vitest coverage for (a) the three-case ``/voice`` slash output in ``createSlashHandler.test.ts`` and (b) the ``applyDisplay → voice.record_key`` hydration + omit-setter back-compat paths in ``useConfigSync.test.ts``. Plus regression cases for every false-fire scenario above. Suite: 575/575 green, tsc --noEmit clean.
This commit is contained in:
parent
c4a693c86f
commit
fb82d132b3
7 changed files with 207 additions and 23 deletions
|
|
@ -173,6 +173,48 @@ describe('createSlashHandler', () => {
|
|||
expect(ctx.transcript.sys).toHaveBeenCalledWith(expect.stringContaining('usage: /skills'))
|
||||
})
|
||||
|
||||
// Regressions from Copilot review on #19835: /voice output + frontend
|
||||
// binding state must both track the gateway's fresh ``record_key`` on
|
||||
// every response, or a config edit shows the new shortcut in text
|
||||
// while push-to-talk still fires the old one until the next mtime
|
||||
// poll (~5s).
|
||||
it('/voice status renders the gateway record_key and pushes it into frontend state', async () => {
|
||||
const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'ctrl+space', tts: false }))
|
||||
const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } })
|
||||
|
||||
expect(createSlashHandler(ctx)('/voice status')).toBe(true)
|
||||
await vi.waitFor(() => {
|
||||
expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+Space')
|
||||
})
|
||||
expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space' })
|
||||
)
|
||||
})
|
||||
|
||||
it('/voice on renders the configured binding for the start/stop hint', async () => {
|
||||
const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'alt+r', tts: false }))
|
||||
const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } })
|
||||
|
||||
expect(createSlashHandler(ctx)('/voice on')).toBe(true)
|
||||
await vi.waitFor(() => {
|
||||
expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice mode enabled')
|
||||
expect(ctx.transcript.sys).toHaveBeenCalledWith(' Alt+R to start/stop recording')
|
||||
})
|
||||
expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ ch: 'r', mod: 'alt' })
|
||||
)
|
||||
})
|
||||
|
||||
it('/voice falls back to Ctrl+B when the gateway response omits record_key', async () => {
|
||||
const rpc = vi.fn(() => Promise.resolve({ enabled: false, tts: false }))
|
||||
const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } })
|
||||
|
||||
expect(createSlashHandler(ctx)('/voice status')).toBe(true)
|
||||
await vi.waitFor(() => {
|
||||
expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+B')
|
||||
})
|
||||
})
|
||||
|
||||
it('cycles details mode and persists it', async () => {
|
||||
const ctx = buildCtx()
|
||||
|
||||
|
|
@ -648,7 +690,8 @@ const buildTranscript = () => ({
|
|||
})
|
||||
|
||||
const buildVoice = () => ({
|
||||
setVoiceEnabled: vi.fn()
|
||||
setVoiceEnabled: vi.fn(),
|
||||
setVoiceRecordKey: vi.fn()
|
||||
})
|
||||
|
||||
interface Ctx {
|
||||
|
|
|
|||
|
|
@ -108,12 +108,23 @@ describe('parseVoiceRecordKey (#18994)', () => {
|
|||
|
||||
expect(parseVoiceRecordKey('alt+b').mod).toBe('alt')
|
||||
expect(parseVoiceRecordKey('option+b').mod).toBe('alt')
|
||||
expect(parseVoiceRecordKey('cmd+b').mod).toBe('meta')
|
||||
expect(parseVoiceRecordKey('command+b').mod).toBe('meta')
|
||||
// ``cmd``/``command`` collapse onto ``super`` — hermes-ink's ``key.meta``
|
||||
// means Alt on most terminals, so mapping ``cmd`` to a distinct ``meta``
|
||||
// mod would make the display lie about the actual match target.
|
||||
expect(parseVoiceRecordKey('cmd+b').mod).toBe('super')
|
||||
expect(parseVoiceRecordKey('command+b').mod).toBe('super')
|
||||
expect(parseVoiceRecordKey('super+b').mod).toBe('super')
|
||||
expect(parseVoiceRecordKey('win+b').mod).toBe('super')
|
||||
})
|
||||
|
||||
it('treats a bare "meta+b" as unrecognised (falls back to Ctrl+B)', async () => {
|
||||
const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux')
|
||||
|
||||
// ``meta`` is ambiguous on the wire (Alt on xterm, Cmd on legacy
|
||||
// macOS). Rather than pick the wrong one, refuse it.
|
||||
expect(parseVoiceRecordKey('meta+b')).toEqual(DEFAULT_VOICE_RECORD_KEY)
|
||||
})
|
||||
|
||||
it('parses named keys (space, enter, tab, escape, backspace, delete)', async () => {
|
||||
const { parseVoiceRecordKey } = await importPlatform('linux')
|
||||
|
||||
|
|
@ -220,6 +231,64 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => {
|
|||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b')).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o')).toBe(false)
|
||||
})
|
||||
|
||||
// Regressions from Copilot review on #19835: the previous implementation
|
||||
// accepted ``isActionMod(key)`` in the ``ctrl`` branch for every
|
||||
// configured key, so bare Esc (which hermes-ink reports with
|
||||
// ``key.meta`` on some macOS terminals) fired ``ctrl+escape``, and
|
||||
// Alt+Space / Alt+Tab fired ``ctrl+space`` / ``ctrl+tab``. The fallback
|
||||
// is now gated to the documented default (``ctrl+b``) only.
|
||||
it('ctrl+escape does NOT fire on bare Esc via key.meta on macOS', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const ctrlEscape = parseVoiceRecordKey('ctrl+escape')
|
||||
|
||||
// Bare Esc on a legacy macOS terminal: ``key.meta: true``, ``key.escape: true``, no ctrl.
|
||||
expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', ctrlEscape)).toBe(false)
|
||||
// Real Ctrl+Esc still fires.
|
||||
expect(isVoiceToggleKey({ ctrl: true, escape: true, meta: false, super: false }, '', ctrlEscape)).toBe(true)
|
||||
})
|
||||
|
||||
it('ctrl+space does NOT fire on Alt+Space on macOS', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const ctrlSpace = parseVoiceRecordKey('ctrl+space')
|
||||
|
||||
// Alt+Space surfaces as ``key.meta: true`` with space char.
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', ctrlSpace)).toBe(false)
|
||||
// Real Ctrl+Space still fires.
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, ' ', ctrlSpace)).toBe(true)
|
||||
})
|
||||
|
||||
it('default ctrl+b still accepts Cmd+B on macOS (muscle-memory preserved)', async () => {
|
||||
const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin')
|
||||
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true)
|
||||
})
|
||||
|
||||
it('custom ctrl+<letter> does NOT accept Cmd fallback on macOS', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const ctrlO = parseVoiceRecordKey('ctrl+o')
|
||||
|
||||
// Only ``ctrl+b`` gets the action-modifier fallback; ``ctrl+o`` must
|
||||
// be a literal Ctrl bit — otherwise Cmd+O would steal the shortcut.
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'o', ctrlO)).toBe(false)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'o', ctrlO)).toBe(false)
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true)
|
||||
})
|
||||
|
||||
it('cmd+b renders "Cmd+B" and matches key.super OR (macOS) key.meta', async () => {
|
||||
const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const cmdB = parseVoiceRecordKey('cmd+b')
|
||||
|
||||
expect(formatVoiceRecordKey(cmdB)).toBe('Cmd+B')
|
||||
// Kitty-style: key.super
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', cmdB)).toBe(true)
|
||||
// Legacy macOS terminal: key.meta
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(true)
|
||||
// Ctrl held at the same time → reject (different chord).
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('isMacActionFallback', () => {
|
||||
|
|
|
|||
|
|
@ -292,3 +292,48 @@ describe('applyDisplay → tui_status_indicator', () => {
|
|||
expect($uiState.get().indicatorStyle).toBe('kaomoji')
|
||||
})
|
||||
})
|
||||
|
||||
// Regressions from Copilot review on #19835: the config-hydration path
|
||||
// for voice.record_key was untested, so a future regression in the
|
||||
// hydration or mtime-reapply wiring would slip past the suite.
|
||||
describe('applyDisplay → voice.record_key (#18994)', () => {
|
||||
beforeEach(() => {
|
||||
resetUiState()
|
||||
})
|
||||
|
||||
it('parses voice.record_key and pushes it through the setter', () => {
|
||||
const setBell = vi.fn()
|
||||
const setVoiceRecordKey = vi.fn()
|
||||
|
||||
applyDisplay(
|
||||
{ config: { display: {}, voice: { record_key: 'ctrl+space' } } },
|
||||
setBell,
|
||||
setVoiceRecordKey
|
||||
)
|
||||
|
||||
expect(setVoiceRecordKey).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space', raw: 'ctrl+space' })
|
||||
)
|
||||
})
|
||||
|
||||
it('falls back to the documented default when voice.record_key is missing', () => {
|
||||
const setBell = vi.fn()
|
||||
const setVoiceRecordKey = vi.fn()
|
||||
|
||||
applyDisplay({ config: { display: {} } }, setBell, setVoiceRecordKey)
|
||||
|
||||
expect(setVoiceRecordKey).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ ch: 'b', mod: 'ctrl', raw: 'ctrl+b' })
|
||||
)
|
||||
})
|
||||
|
||||
it('is a no-op when the voice setter is not passed (back-compat)', () => {
|
||||
const setBell = vi.fn()
|
||||
|
||||
// applyDisplay is used in the setVoiceEnabled-less init path too;
|
||||
// omitting the third arg must not throw.
|
||||
expect(() =>
|
||||
applyDisplay({ config: { display: {}, voice: { record_key: 'alt+r' } } }, setBell)
|
||||
).not.toThrow()
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -293,6 +293,7 @@ export interface SlashHandlerContext {
|
|||
}
|
||||
voice: {
|
||||
setVoiceEnabled: StateSetter<boolean>
|
||||
setVoiceRecordKey: (v: ParsedVoiceRecordKey) => void
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -226,7 +226,15 @@ export const sessionCommands: SlashCommand[] = [
|
|||
// instead of hardcoded "Ctrl+B" — the gateway response carries the
|
||||
// current value so /voice status and /voice on stay in sync with
|
||||
// both the CLI and the TUI's actual binding (#18994).
|
||||
const recordKeyLabel = formatVoiceRecordKey(parseVoiceRecordKey(r.record_key ?? 'ctrl+b'))
|
||||
//
|
||||
// Copilot review on #19835 caught that rendering from the fresh
|
||||
// backend response WITHOUT updating the frontend ``voice.recordKey``
|
||||
// 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)
|
||||
|
||||
// Match CLI's _show_voice_status / _enable_voice_mode /
|
||||
// _toggle_voice_tts output shape so users don't have to learn
|
||||
|
|
|
|||
|
|
@ -644,7 +644,7 @@ export function useMainApp(gw: GatewayClient) {
|
|||
},
|
||||
slashFlightRef,
|
||||
transcript: { page, panel, send, setHistoryItems, sys, trimLastExchange: session.trimLastExchange },
|
||||
voice: { setVoiceEnabled }
|
||||
voice: { setVoiceEnabled, setVoiceRecordKey }
|
||||
}),
|
||||
[
|
||||
catalog,
|
||||
|
|
|
|||
|
|
@ -62,7 +62,7 @@ export const isCopyShortcut = (
|
|||
* the configured letter so existing macOS muscle memory keeps working
|
||||
* alongside the documented Ctrl+<letter> shortcut.
|
||||
*/
|
||||
export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'meta' | 'super'
|
||||
export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'super'
|
||||
|
||||
/** Named (multi-character) keys we support, matching the CLI's
|
||||
* prompt_toolkit binding shape (``c-space``, ``c-enter``, etc.) so a
|
||||
|
|
@ -85,13 +85,18 @@ export const DEFAULT_VOICE_RECORD_KEY: ParsedVoiceRecordKey = {
|
|||
raw: 'ctrl+b'
|
||||
}
|
||||
|
||||
/** Modifier aliases. ``meta`` is intentionally absent: hermes-ink sets
|
||||
* ``key.meta`` for plain Alt/Option (and on some legacy terminals for Cmd
|
||||
* too), so accepting a literal ``meta+b`` config would display as ``Cmd+B``
|
||||
* but match Alt+B on the wire — the kind of lie this fix was meant to
|
||||
* remove. Users who want the platform action modifier spell it ``cmd`` /
|
||||
* ``command`` / ``super`` / ``win``. */
|
||||
const _MOD_ALIASES: Record<string, VoiceRecordKeyMod> = {
|
||||
alt: 'alt',
|
||||
cmd: 'meta',
|
||||
command: 'meta',
|
||||
cmd: 'super',
|
||||
command: 'super',
|
||||
control: 'ctrl',
|
||||
ctrl: 'ctrl',
|
||||
meta: 'meta',
|
||||
option: 'alt',
|
||||
opt: 'alt',
|
||||
super: 'super',
|
||||
|
|
@ -185,13 +190,17 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => {
|
|||
|
||||
let mod: VoiceRecordKeyMod = 'ctrl'
|
||||
|
||||
for (const cand of modCandidates) {
|
||||
const norm = _MOD_ALIASES[cand]
|
||||
if (modCandidates.length) {
|
||||
const norm = _MOD_ALIASES[modCandidates[0]]
|
||||
|
||||
if (norm) {
|
||||
mod = norm
|
||||
break
|
||||
// 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
|
||||
}
|
||||
|
||||
if (last.length === 1) {
|
||||
|
|
@ -211,7 +220,7 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => {
|
|||
|
||||
/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. */
|
||||
export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => {
|
||||
const modLabel = parsed.mod === 'meta' ? 'Cmd' : parsed.mod[0].toUpperCase() + parsed.mod.slice(1)
|
||||
const modLabel = parsed.mod === 'super' ? 'Cmd' : 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
|
||||
|
|
@ -221,6 +230,9 @@ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => {
|
|||
return `${modLabel}+${keyLabel}`
|
||||
}
|
||||
|
||||
const _isDefaultVoiceKey = (parsed: ParsedVoiceRecordKey): boolean =>
|
||||
parsed.raw === DEFAULT_VOICE_RECORD_KEY.raw
|
||||
|
||||
export const isVoiceToggleKey = (
|
||||
key: RuntimeKeyEvent,
|
||||
ch: string,
|
||||
|
|
@ -241,15 +253,21 @@ export const isVoiceToggleKey = (
|
|||
case 'alt':
|
||||
// Most terminals surface Alt as either ``alt`` or ``meta``; accept
|
||||
// both so the binding works across xterm-style and kitty-style
|
||||
// protocols.
|
||||
return key.alt === true || key.meta
|
||||
// protocols. Guard against ctrl/super bits so a chord like
|
||||
// Ctrl+Alt+<key> or Cmd+Alt+<key> doesn't spuriously fire the
|
||||
// alt binding.
|
||||
return (key.alt === true || key.meta) && !key.ctrl && key.super !== true
|
||||
case 'ctrl':
|
||||
// Doc default — also accept the platform action modifier so macOS
|
||||
// Cmd+<letter> muscle memory keeps working alongside Ctrl+<letter>.
|
||||
return key.ctrl || isActionMod(key)
|
||||
case 'meta':
|
||||
return key.meta || key.super === true
|
||||
// Only the documented default (``ctrl+b``) gets the macOS
|
||||
// Cmd-as-action-modifier fallback. For any user-configured binding
|
||||
// we require the literal Ctrl bit — otherwise ``ctrl+escape`` would
|
||||
// fire on bare Esc (hermes-ink sets ``key.meta`` for bare Esc on
|
||||
// some macOS terminals) and ``ctrl+space`` would fire on Alt+Space.
|
||||
return key.ctrl || (_isDefaultVoiceKey(configured) && isActionMod(key))
|
||||
case 'super':
|
||||
return key.super === true
|
||||
// Kitty-style protocol surfaces Cmd as ``key.super``; legacy macOS
|
||||
// terminals still surface it as ``key.meta``. Accept both but
|
||||
// require the ctrl bit to be clear so Ctrl+Cmd+<key> doesn't match.
|
||||
return (key.super === true || (isMac && key.meta)) && !key.ctrl
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue