mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-19 10:02:16 +00:00
fix(tui): address Copilot round-5 review on #19835
Three follow-ups on the voice matcher's modifier + shift discipline:
* **``super`` branch falsely fired on Alt+<key> / bare Esc on macOS.**
``isVoiceToggleKey`` accepted ``isMac && key.meta`` as a Cmd
fallback for the ``super`` modifier — but hermes-ink sets
``key.meta`` for plain Alt/Option AND for bare Escape on some
macOS terminals. A ``cmd+b`` config silently fired on Alt+B;
``cmd+space`` on Alt+Space; ``cmd+escape`` on bare Esc. Drop the
fallback and require the literal ``key.super`` bit. Legacy-
terminal users who need Cmd should upgrade to a kitty-protocol
terminal or bind ``alt+X`` explicitly.
* **Shift bit was never checked.** The parser rejects multi-
modifier configs like ``ctrl+shift+tab``, but the runtime
matcher didn't check ``key.shift`` — so ``ctrl+tab`` also fired
on Ctrl+Shift+Tab and ``alt+enter`` on Alt+Shift+Enter.
Early-return on ``key.shift === true`` so the runtime only fires
the exact chord the user configured.
* **Test leaked ``HERMES_VOICE=1`` into later tests.**
``voice.toggle`` action=on writes to ``os.environ`` directly
(CLI parity, runtime-only flag); ``test_voice_toggle_returns_
configured_record_key`` dispatched action=on without letting
monkeypatch take ownership of the var first. Any later test
that read voice mode in the same Python process could inherit a
stale enabled state. Added ``monkeypatch.setenv("HERMES_VOICE",
"0")`` up front so monkeypatch restores the original value at
teardown.
Coverage added:
* ``cmd+b`` / ``cmd+space`` / ``cmd+escape`` do NOT fire on
``key.meta``-only events on darwin.
* ``ctrl+tab`` / ``alt+enter`` / ``ctrl+o`` reject matches when
``key.shift`` is held; sanity cases without Shift still fire.
Suite: 585/585 TUI vitest green, 3/3 backend voice tests green,
tsc --noEmit clean.
This commit is contained in:
parent
1a4d990d6b
commit
0db73be157
3 changed files with 68 additions and 8 deletions
|
|
@ -94,6 +94,12 @@ def test_voice_toggle_returns_configured_record_key(monkeypatch):
|
|||
check_voice_requirements=lambda: {"available": True, "details": ""}
|
||||
),
|
||||
)
|
||||
# ``voice.toggle`` action=on mutates ``os.environ["HERMES_VOICE"]``
|
||||
# directly (CLI parity, runtime-only flag). Take monkeypatch
|
||||
# ownership of the var so the change is reverted at teardown and
|
||||
# later tests don't inherit a stale ON state (Copilot round-5
|
||||
# review on #19835).
|
||||
monkeypatch.setenv("HERMES_VOICE", "0")
|
||||
|
||||
on_resp = server.dispatch(
|
||||
{"id": "voice-on", "method": "voice.toggle", "params": {"action": "on"}}
|
||||
|
|
|
|||
|
|
@ -207,6 +207,42 @@ describe('parseVoiceRecordKey (#18994)', () => {
|
|||
expect(parseVoiceRecordKey('alt+c').mod).toBe('alt')
|
||||
expect(parseVoiceRecordKey('cmd+l').mod).toBe('super')
|
||||
})
|
||||
|
||||
// Round-5 Copilot review regressions on #19835.
|
||||
it('cmd+<key> does NOT fire on key.meta-only events (Alt+X false-fire guard)', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
|
||||
// hermes-ink sets ``key.meta`` for Alt/Option AND for bare Esc on
|
||||
// some macOS terminals. The super branch used to accept
|
||||
// ``isMac && key.meta`` as a Cmd fallback, which made cmd+<key>
|
||||
// bindings silently fire on Alt+<key> / bare Esc.
|
||||
const cmdB = parseVoiceRecordKey('cmd+b')
|
||||
const cmdSpace = parseVoiceRecordKey('cmd+space')
|
||||
const cmdEscape = parseVoiceRecordKey('cmd+escape')
|
||||
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(false)
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', cmdSpace)).toBe(false)
|
||||
expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', cmdEscape)).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects matches when Shift is held (different chord than configured)', async () => {
|
||||
const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux')
|
||||
|
||||
// Parser rejects multi-modifier configs like ``ctrl+shift+tab``,
|
||||
// so the runtime matcher must also reject Shift-held events —
|
||||
// otherwise ``ctrl+tab`` would fire on Ctrl+Shift+Tab.
|
||||
const ctrlTab = parseVoiceRecordKey('ctrl+tab')
|
||||
const altEnter = parseVoiceRecordKey('alt+enter')
|
||||
const ctrlO = parseVoiceRecordKey('ctrl+o')
|
||||
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false, tab: true }, '', ctrlTab)).toBe(false)
|
||||
expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, return: true, shift: true, super: false }, '', altEnter)).toBe(false)
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false }, 'o', ctrlO)).toBe(false)
|
||||
|
||||
// Sanity: same events without Shift still fire.
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false, tab: true }, '', ctrlTab)).toBe(true)
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false }, 'o', ctrlO)).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('formatVoiceRecordKey (#18994)', () => {
|
||||
|
|
@ -332,15 +368,17 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => {
|
|||
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 () => {
|
||||
it('cmd+b renders "Cmd+B" and requires the literal key.super bit', async () => {
|
||||
const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin')
|
||||
const cmdB = parseVoiceRecordKey('cmd+b')
|
||||
|
||||
expect(formatVoiceRecordKey(cmdB)).toBe('Cmd+B')
|
||||
// Kitty-style: key.super
|
||||
// Kitty-style: key.super fires the binding.
|
||||
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)
|
||||
// ``key.meta`` is NOT accepted — hermes-ink uses meta for Alt too,
|
||||
// so accepting it here would make cmd+b silently fire on Alt+B
|
||||
// (Copilot round-5 review on #19835).
|
||||
expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(false)
|
||||
// Ctrl held at the same time → reject (different chord).
|
||||
expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false)
|
||||
})
|
||||
|
|
|
|||
|
|
@ -140,6 +140,7 @@ interface RuntimeKeyEvent {
|
|||
escape?: boolean
|
||||
meta: boolean
|
||||
return?: boolean
|
||||
shift?: boolean
|
||||
super?: boolean
|
||||
tab?: boolean
|
||||
}
|
||||
|
|
@ -299,6 +300,15 @@ export const isVoiceToggleKey = (
|
|||
return false
|
||||
}
|
||||
|
||||
// The parser rejects multi-modifier configs (``ctrl+shift+b`` etc.),
|
||||
// so at match time Shift must always be clear — otherwise
|
||||
// ``ctrl+tab`` would also fire on Ctrl+Shift+Tab and ``alt+enter``
|
||||
// on Alt+Shift+Enter, triggering a different chord than configured
|
||||
// (Copilot round-5 review on #19835).
|
||||
if (key.shift === true) {
|
||||
return false
|
||||
}
|
||||
|
||||
switch (configured.mod) {
|
||||
case 'alt':
|
||||
// Most terminals surface Alt as either ``alt`` or ``meta``; accept
|
||||
|
|
@ -315,9 +325,15 @@ export const isVoiceToggleKey = (
|
|||
// some macOS terminals) and ``ctrl+space`` would fire on Alt+Space.
|
||||
return key.ctrl || (_isDefaultVoiceKey(configured) && isActionMod(key))
|
||||
case 'super':
|
||||
// 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
|
||||
// Require the explicit ``key.super`` bit (kitty-style protocol).
|
||||
// The previous ``isMac && key.meta`` fallback was intended for
|
||||
// legacy macOS terminals that report Cmd as ``meta``, but
|
||||
// hermes-ink also sets ``key.meta`` for plain Alt/Option and even
|
||||
// bare Escape — so a ``cmd+b`` config silently fired on Alt+B
|
||||
// and ``cmd+escape`` on bare Esc (Copilot round-5 review on
|
||||
// #19835). Legacy-terminal users who want the Cmd shortcut
|
||||
// should upgrade to a kitty-protocol terminal or bind ``alt+X``
|
||||
// explicitly.
|
||||
return key.super === true && !key.ctrl
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue