fix(cli): disarm continuous voice on hotkey during STT/agent (#67545) - #67569
fix(cli): disarm continuous voice on hotkey during STT/agent (#67545)#67569Enough1122 wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused CLI fix. The current-main premise is confirmed: cli.py:13697 and cli.py:13704 return from the non-recording hotkey path without clearing _voice_continuous, while cli.py:14937 restarts recording after a turn when that flag remains armed. The added locked assignments address that state transition without changing the recording branch.
Problems
- The PR changes only
cli.py; it adds no regression coverage for the classic CLI binding.tests/hermes_cli/test_voice_wrapper.py:682tests the separatehermes_cli.voiceAPI, andtests/tools/test_voice_cli_integration.py:711currently uses source/AST checks rather than executinghandle_voice_record.
Suggested changes
- Add a runtime test for Ctrl+B/configured record-key handling with
_voice_continuous=Truein both_agent_runningand_voice_processingstates, asserting the flag is cleared and recording is not started.
Automated hermes-sweeper review.
| # transcribed, pressing the hotkey means "disarm continuous" | ||
| # rather than "start a new take". Without this, Ctrl+B during | ||
| # STT/agent is a silent no-op and the loop auto-restarts | ||
| # after the turn — the only way out is /voice off (#67545). | ||
| if cli_ref._agent_running: |
There was a problem hiding this comment.
Please add a runtime regression test for both new disarm branches. The existing voice-wrapper test exercises hermes_cli.voice.stop_continuous(), not this classic CLI keybinding, so it would not catch a future regression here.
|
cc @alt-glitch — re: the competing #67550 / #67573 note. Updated the PR body to make the boundary explicit. Summary: #67569 (this) is intentionally the narrowest possible fix. It only touches the two early-return paths the issue reporter actually hit:
No global hotkey-toggle semantic. #67550 and #67573 both propose putting a disarm check at the top of the
Those paths are not in the scope of #67545, and changing them is a policy call (some users may rely on Ctrl+B being inert during a confirmation prompt so they don't accidentally drop continuous mode mid-confirmation). I didn't want to make that call as part of a "fix the reported bug" PR. Recommendation:
I've added a "Scope vs. #67550 / #67573" comparison table to the PR body so the choice is obvious from the description without needing to read the other two diffs. Branch — written by Hermes Agent on behalf of @Enough1122 |
…earch#67545) - Add locked _voice_continuous = False assignments in both guard branches of handle_voice_record: when _agent_running is True and when _voice_processing is True. Previously the hotkey was a silent no-op in these states and the loop auto-restarted after the turn. - Add runtime regression tests that extract the real handle_voice_record closure from cli.py via AST and exec it against a real CLI instance, testing both disarm branches and a negative control (review feedback on NousResearch#67569).
7976bd9 to
a09b6e0
Compare
|
Review feedback addressed —
The handler is extracted from cli.py AST and |
|
Closing this stale PR for now: the current patch still has unresolved review/test-scope gaps, and it is unlikely to be merged in its present form. Reopen or submit a focused follow-up if the issue remains relevant. |
Fixes #67545.
Narrow fix for the two specific non-recording paths the bug report calls out:
Ctrl+Bduring STT (_voice_processing=True) or agent-busy (_agent_running=True) was a silent no-op — continuous stayed armed and the loop auto-restarted after the turn. The only workaround was/voice off.This PR does not introduce a full hotkey-toggle semantic. It only inserts a
_voice_continuous = Falsedisarm in the two existing early-return paths the issue identifies, leaving the other non-recording paths unchanged. See "Scope vs. #67550 / #67573" below for the deliberate boundary.What changed
cli.py::handle_voice_record,elsebranch, before the existing guards:Existing behavior on the recording branch (
if cli_ref._voice_recording) is unchanged — that path already disarms continuous today and continues to do so.Why narrow on purpose
The issue reporter asked specifically for the two paths they hit:
A broader "Ctrl+B is a global toggle" interpretation (#67550, #67573) changes semantics on paths the issue does not call out — e.g. TTS playback, interactive
clarify/sudo/approval/slash_confirmprompts — and risks surprising users who currently rely on those paths being inert. That's a policy decision the maintainers should make, not one I want to bake in via the narrowest possible fix.Scope vs. #67550 / #67573
elsebranch (toggle semantic)elsebranch (toggle semantic)Happy to close in favor of #67550 or #67573 if maintainers prefer the broader toggle. The diff in this PR is a strict subset of either. If they prefer the narrow fix, the diff here is the smallest correct change.
Test plan
tests/hermes_cli/test_voice_wrapper.py— 44/44 pass locally.handle_voice_record. The disarm is guarded bycli_ref._voice_lockto match the existing lock discipline around_voice_continuouswrites elsewhere in the same handler.Branch:
fix/67545-voice-continuous-hotkey-disarm@7976bd96.