mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-27 11:22:03 +00:00
fix(tui): address Copilot round-2 review on #19835
Three tightenings on the surface introduced in the round-1 fix: * **``/voice tts`` reset custom bindings to Ctrl+B.** The ``tts`` branch of ``voice.toggle`` omitted ``record_key`` from its response, so the frontend's ``r.record_key ?? 'ctrl+b'`` coerced a user's custom binding back to the default on every TTS toggle. Two-sided fix: the backend now includes ``record_key`` on the ``tts`` branch (parity with ``status``/``on``/``off``), and the slash handler only pushes frontend state when the response actually carries ``record_key`` — belt-and-suspenders against any future branch forgetting to include it. * **``super+b`` / ``win+b`` / ``cmd+b`` displayed "Cmd+B" on Linux and Windows.** ``formatVoiceRecordKey`` rendered ``mod === 'super'`` as ``Cmd`` universally, which told non-mac users the wrong modifier to press even though ``isVoiceToggleKey`` matched the right event bits. Gate the label to ``isMac`` so non-mac renders ``Super+B``. * **``control+b`` / ``ctrl + b`` lost the macOS Cmd+B fallback.** ``_isDefaultVoiceKey`` keyed off ``parsed.raw`` — so semantically-equal aliases of the documented default dropped into the strict branch even though they bind Ctrl+B. Compare on the parsed spec (mod + ch + named) instead. Coverage added: Linux ``Super+B`` rendering (and macOS ``Cmd+B``), ``control+b`` / ``ctrl + b`` accepting the Cmd+B fallback on darwin, ``/voice tts`` without ``record_key`` not clobbering cached binding, and a backend regression asserting every ``voice.toggle`` branch carries the configured key. Suite: 579/579 TUI vitest green, 2/2 backend voice tests green, tsc --noEmit clean.
This commit is contained in:
parent
fb82d132b3
commit
674b1030c1
6 changed files with 126 additions and 8 deletions
|
|
@ -106,6 +106,38 @@ def test_voice_toggle_returns_configured_record_key(monkeypatch):
|
|||
assert status_resp["result"]["record_key"] == "ctrl+o"
|
||||
|
||||
|
||||
def test_voice_toggle_tts_branch_also_carries_record_key(monkeypatch):
|
||||
"""Round-2 Copilot review regression on #19835.
|
||||
|
||||
The ``tts`` branch used to omit ``record_key`` from its response, so a
|
||||
TUI client would parse ``r.record_key ?? 'ctrl+b'`` and reset a
|
||||
custom binding to the default on every TTS toggle. Every branch of
|
||||
``voice.toggle`` now carries the configured key so frontend state
|
||||
stays authoritative.
|
||||
"""
|
||||
monkeypatch.setattr(
|
||||
server,
|
||||
"_load_cfg",
|
||||
lambda: {"voice": {"record_key": "ctrl+space"}},
|
||||
)
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"tools.voice_mode",
|
||||
types.SimpleNamespace(
|
||||
check_voice_requirements=lambda: {"available": True, "details": ""}
|
||||
),
|
||||
)
|
||||
monkeypatch.setenv("HERMES_VOICE", "1")
|
||||
monkeypatch.delenv("HERMES_VOICE_TTS", raising=False)
|
||||
|
||||
tts_resp = server.dispatch(
|
||||
{"id": "voice-tts", "method": "voice.toggle", "params": {"action": "tts"}}
|
||||
)
|
||||
|
||||
assert tts_resp["result"]["record_key"] == "ctrl+space"
|
||||
assert tts_resp["result"]["tts"] is True
|
||||
|
||||
|
||||
def test_load_enabled_toolsets_prefers_tui_env(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_TUI_TOOLSETS", "web, terminal, ,memory")
|
||||
|
||||
|
|
|
|||
|
|
@ -5360,7 +5360,20 @@ def _(rid, params: dict) -> dict:
|
|||
new_value = not _voice_tts_enabled()
|
||||
# Runtime-only flag (CLI parity) — see voice.toggle on/off above.
|
||||
os.environ["HERMES_VOICE_TTS"] = "1" if new_value else "0"
|
||||
return _ok(rid, {"enabled": True, "tts": new_value})
|
||||
# Include ``record_key`` on every branch so a /voice tts toggle
|
||||
# doesn't reset the TUI's cached shortcut to the default when a
|
||||
# user has a custom binding configured (Copilot review, round 2
|
||||
# on #19835). Keeps parity with the status/on/off branches above.
|
||||
return _ok(
|
||||
rid,
|
||||
{
|
||||
"enabled": True,
|
||||
"record_key": str(
|
||||
(_load_cfg().get("voice") or {}).get("record_key") or "ctrl+b"
|
||||
),
|
||||
"tts": new_value,
|
||||
},
|
||||
)
|
||||
|
||||
return _err(rid, 4013, f"unknown voice action: {action}")
|
||||
|
||||
|
|
|
|||
|
|
@ -215,6 +215,22 @@ describe('createSlashHandler', () => {
|
|||
})
|
||||
})
|
||||
|
||||
// Round-2 Copilot review on #19835: a response missing ``record_key``
|
||||
// (e.g. the old tts branch, or any future branch that forgets to
|
||||
// include it) MUST NOT clobber the user's cached binding back to
|
||||
// Ctrl+B. The label still renders the default for display; the
|
||||
// frontend state keeps whatever was last authoritatively set.
|
||||
it('/voice tts without record_key does not clobber cached frontend binding', async () => {
|
||||
const rpc = vi.fn(() => Promise.resolve({ enabled: true, tts: true }))
|
||||
const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } })
|
||||
|
||||
expect(createSlashHandler(ctx)('/voice tts')).toBe(true)
|
||||
await vi.waitFor(() => {
|
||||
expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice TTS enabled.')
|
||||
})
|
||||
expect(ctx.voice.setVoiceRecordKey).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('cycles details mode and persists it', async () => {
|
||||
const ctx = buildCtx()
|
||||
|
||||
|
|
|
|||
|
|
@ -164,7 +164,10 @@ describe('formatVoiceRecordKey (#18994)', () => {
|
|||
expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+b'))).toBe('Ctrl+B')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+o'))).toBe('Ctrl+O')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('alt+r'))).toBe('Alt+R')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Cmd+B')
|
||||
// ``cmd``/``command``/``super``/``win`` all collapse onto the super
|
||||
// modifier and render as ``Super`` on non-mac so the hint doesn't
|
||||
// tell Linux/Windows users to press a Cmd key they don't have.
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Super+B')
|
||||
})
|
||||
|
||||
it('renders named keys in title case (Ctrl+Space, Ctrl+Enter)', async () => {
|
||||
|
|
@ -289,6 +292,37 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => {
|
|||
// Ctrl held at the same time → reject (different chord).
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false)
|
||||
})
|
||||
|
||||
// Round-2 Copilot review regressions on #19835.
|
||||
it('super+b renders "Super+B" on Linux (not "Cmd+B")', async () => {
|
||||
const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('linux')
|
||||
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Super+B')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Super+B')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('cmd+b'))).toBe('Super+B')
|
||||
})
|
||||
|
||||
it('super+b still renders "Cmd+B" on macOS', async () => {
|
||||
const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Cmd+B')
|
||||
expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Cmd+B')
|
||||
})
|
||||
|
||||
it('ctrl+b aliases (control+b, "ctrl + b") still accept Cmd+B fallback on macOS', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const controlB = parseVoiceRecordKey('control+b')
|
||||
const spacedB = parseVoiceRecordKey('ctrl + b')
|
||||
|
||||
// Both parse to the documented default semantically; both must keep
|
||||
// the macOS Cmd+B muscle-memory fallback.
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', controlB)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', spacedB)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', controlB)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', spacedB)).toBe(true)
|
||||
// And still reject a ctrl bit on a different letter.
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', controlB)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('isMacActionFallback', () => {
|
||||
|
|
|
|||
|
|
@ -232,9 +232,19 @@ export const sessionCommands: SlashCommand[] = [
|
|||
// 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)
|
||||
//
|
||||
// Round-2 follow-up: only push state when the response actually
|
||||
// carries ``record_key`` — otherwise an older gateway (or a future
|
||||
// branch that forgets to include it) would clobber a custom user
|
||||
// binding back to the default on every /voice invocation. The
|
||||
// label still falls back to the documented default for display.
|
||||
const parsed = r.record_key ? parseVoiceRecordKey(r.record_key) : undefined
|
||||
|
||||
if (parsed) {
|
||||
ctx.voice.setVoiceRecordKey(parsed)
|
||||
}
|
||||
|
||||
const recordKeyLabel = formatVoiceRecordKey(parsed ?? parseVoiceRecordKey('ctrl+b'))
|
||||
|
||||
// Match CLI's _show_voice_status / _enable_voice_mode /
|
||||
// _toggle_voice_tts output shape so users don't have to learn
|
||||
|
|
|
|||
|
|
@ -218,9 +218,15 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => {
|
|||
return DEFAULT_VOICE_RECORD_KEY
|
||||
}
|
||||
|
||||
/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. */
|
||||
/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text.
|
||||
*
|
||||
* Platform-aware for the ``super`` modifier: renders ``Cmd`` on macOS and
|
||||
* ``Super`` elsewhere. Previously rendered ``Cmd`` universally, which told
|
||||
* Linux/Windows users the wrong modifier to press (Copilot review, round
|
||||
* 2 on #19835). */
|
||||
export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => {
|
||||
const modLabel = parsed.mod === 'super' ? 'Cmd' : parsed.mod[0].toUpperCase() + parsed.mod.slice(1)
|
||||
const modLabel =
|
||||
parsed.mod === 'super' ? (isMac ? 'Cmd' : 'Super') : 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
|
||||
|
|
@ -230,8 +236,15 @@ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => {
|
|||
return `${modLabel}+${keyLabel}`
|
||||
}
|
||||
|
||||
/** Whether the parsed binding is the documented default (ctrl+b).
|
||||
*
|
||||
* Compare on the parsed spec rather than ``raw`` so semantically-equal
|
||||
* aliases (``control+b``, ``ctrl + b``) still get the macOS Cmd+B
|
||||
* muscle-memory fallback (Copilot review, round 2 on #19835). */
|
||||
const _isDefaultVoiceKey = (parsed: ParsedVoiceRecordKey): boolean =>
|
||||
parsed.raw === DEFAULT_VOICE_RECORD_KEY.raw
|
||||
parsed.mod === DEFAULT_VOICE_RECORD_KEY.mod &&
|
||||
parsed.ch === DEFAULT_VOICE_RECORD_KEY.ch &&
|
||||
parsed.named === DEFAULT_VOICE_RECORD_KEY.named
|
||||
|
||||
export const isVoiceToggleKey = (
|
||||
key: RuntimeKeyEvent,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue