fix(cli): pause wake-word listener before manual push-to-talk recording - #74152
fix(cli): pause wake-word listener before manual push-to-talk recording#74152pierrenode wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this to the classic CLI path. The premise remains valid on current main: cli.py:15705-15713 starts the manual recording thread and calls _voice_start_recording() without the wake handoff, while cli.py:12614-12623 already uses pause_listening() plus _wake_suspended for wake-triggered capture.
Problems
- The new test reads
cli.pyand asserts source ordering.AGENTS.md:1382-1416bans source-text tests because they validate implementation shape rather than runtime behavior. The test should not be carried forward as written.
Suggested changes
- Replace the source inspection with a behavioral seam: extract the handoff into a small testable helper, or invoke the registered keybinding with mocked
pause_listeningand_voice_start_recordingand assert their call order plus the suspension flag.
Current main substantially restructured tests/tools/test_voice_cli_integration.py, and GitHub reports this PR as dirty, so the test needs rework during salvage. This is an automated hermes-sweeper review.
|
|
||
| _on_wake_word already does this (it owns the mic when it fires), but | ||
| manual push-to-talk (Ctrl+B) can run while the wake detector's own | ||
| input stream is still open — two independent sd.InputStream opens on |
There was a problem hiding this comment.
Please replace this source-text assertion with a behavioral test. AGENTS.md:1382-1416 explicitly bans tests that read source files; this can pass while runtime wiring is wrong and fails under behavior-preserving refactors.
8c201ba to
3aec217
Compare
|
Rebased onto current `upstream/main`. This file (`tests/tools/test_voice_cli_integration.py`) was heavily restructured/pruned upstream in the meantime — the `TestKeyHandlerNeverBlocks` class (including this test) no longer exists there. The bug itself is still fully present on current `main` (`_start_recording`'s closure still has no `pause_listening` call), so re-applied the fix fresh against the current file rather than replaying the old diff through the rebase. Also rewrote the test to be behavioral, per AGENTS.md's ban on source-inspection tests ("A test that reads a source file's text is testing the shape of the source code, not its behavior... banned outright"). The original test read `cli.py`'s text and string-searched for `pause_listening`/`cli_ref._voice_start_recording()` ordering — exactly the banned pattern, and it would happily pass on a correct-looking-but-wrong reformatting while missing an actual behavioral regression. Fix: extracted the closure's pause-then-record logic into a standalone `HermesCLI._manual_ptt_start_recording()` method — mirroring `_on_wake_word`'s existing, already-real method pattern almost verbatim — instead of an inline nested function. `handle_voice_record`'s daemon thread now dispatches straight to it. This makes the logic independently callable in a test with real (mocked) collaborators, per AGENTS.md's own "Do write" example (extract into a small DI-testable function, call it for real). 3 new behavioral tests call `_manual_ptt_start_recording()` directly against a real `HermesCLI` instance (`_make_voice_cli()`, this file's existing helper) with a mocked `pause_listening`/`_voice_start_recording` pair, asserting on the actual call order and `_wake_suspended` side effect — not the source text. Mutation-verified: swapping the two try-blocks' order in `_manual_ptt_start_recording` breaks the ordering test; making `pause_listening`'s return value unconditional (dropping the `if`) breaks the "declines" test. Full `tests/tools/test_voice_cli_integration.py` (34 tests) passes. Ruff clean. `tests/tools/test_wake_word.py` has one pre-existing, unrelated failure (missing `ai-edge-litert` tflite runtime on this Mac) — confirmed identical against clean `upstream/main`. Textual-proximity note: open PR #79696 (wake-triggered continuous hands-free conversation) modifies the exact same `_on_wake_word`/`_start_wake_watchdog` boundary this PR inserts `_manual_ptt_start_recording` right after — different bug, will need a small rebase against whichever lands first, no semantic overlap. Squashed to a single commit on top of current `upstream/main`. |
The wake-triggered capture path (_on_wake_word) already releases the mic from the wake-word detector via pause_listening() before opening the recorder, because the detector owns the mic when it fires. Manual push-to-talk (Ctrl+B, handle_voice_record's _start_recording) never did this — it can run while the wake detector's own input stream is still open, so AudioRecorder._ensure_stream() opens a second, independent sd.InputStream on the same device. On platforms where the capture device is effectively single-owner (e.g. Windows, per the desktop frontend's own note when it hit the same contention for its button/hotkey path) this raises a PortAudio error surfaced to the user as "Voice recording failed: ...", while the wake listener keeps running untouched. Extracted the fixed pause-then-record logic into a standalone HermesCLI._manual_ptt_start_recording() method (mirroring _on_wake_word's existing pattern almost verbatim) rather than inlining it in handle_voice_record's nested closure, so it can be exercised directly in tests instead of reading cli.py's source text — AGENTS.md bans source-inspection tests outright. handle_voice_record's daemon thread now dispatches straight to this method. Regression tests call _manual_ptt_start_recording() directly with a mocked pause_listening/_voice_start_recording pair and assert on the real call order, covering: pause runs before recording starts and sets _wake_suspended (so the existing wake watchdog resumes the listener once idle); _wake_suspended stays False when pause_listening declines (nothing to resume); and a pause_listening failure doesn't block recording (the hand-off is best-effort). Mutation-verified: swapping the two try blocks' order breaks the first test.
3aec217 to
367e079
Compare
Summary
The classic CLI's manual push-to-talk hotkey (Ctrl+B) can open a second, independent microphone input stream while an active wake-word listener's own stream is still open on the same device — a mic-contention race, the same class the desktop frontend already fixed for its button/hotkey path.
Root cause
_on_wake_word(the wake-triggered capture path) already callspause_listening(owner=self)before starting the recorder, because the wake detector owns the mic when it fires.handle_voice_record's_start_recordingclosure, dispatched to a daemon thread so it never blocks the prompt_toolkit event loop) callscli_ref._voice_start_recording()directly with no such handoff — greppingcli.pyconfirmspause_listeningwas only ever called from_on_wake_word.wake_wordenabled (soWakeWordDetectorholds an opensd.InputStream) and also presses the manual record hotkey,AudioRecorder._ensure_stream()opens a second independentsd.InputStreamon the same device. On platforms where the capture device is effectively single-owner (Windows), this raises a PortAudio error surfaced to the user as "Voice recording failed: ...", while the wake listener silently keeps running untouched.Fix
Mirror
_on_wake_word's existing pattern inside the push-to-talk_start_recordingclosure: trypause_listening(owner=cli_ref)before starting the recorder and, on success, set_wake_suspended = True. The pre-existing wake watchdog (_start_wake_watchdog, already running whenever wake word is armed) picks this flag up and callsresume_listening()once recording goes idle — the exact same resume mechanism already used for the wake-triggered path, so no new resume logic is needed.Tests
tests/tools/test_voice_cli_integration.py::TestKeyHandlerNeverBlocks::test_start_recording_pauses_wake_word_first: source-order regression test (mirrors this file's existing static/AST-based tests for the same unblockable-closure code, since the handler is a keybinding closure defined inside__init__and isn't practically invokable in isolation — the file's own_make_voice_cli()helper exists specifically to bypass__init__).pause_idx == -1).test_voice_cli_integration.py,test_wake_word.py,test_voice_mode.py): 275 passed (up from 274), 15 pre-existing failures unrelated to this change (confirmed identical on unmodified code — real-audio-device/network-fetch/WSL tests that don't run in this sandbox).ruff check: clean.Checklist
push-to-talk wake microphone,wake word microphone contention,pause_listening cli,voice record wake— closest hit is feat(voice): chat UX polish — busy-aware silence, stop hint, thinking sounds, barge-in fix #74000, an unrelated Desktop/TypeScript voice-UX PR opened today that doesn't touchcli.py)