Mid-session wake = barge-in, not a reconnect - #10
Conversation
"Hey Mycroft" during an open session used to stop and restart the
websocket: the stop fired on_stopped's end cue (goodbye sound on every
re-wake), the reconnect ate the first words of the command, and a wake
with no follow-up left the in-flight reply playing (server_vad never
hears the user while the mic gate feeds it silence).
Firmware: the wake handler now calls the (previously unused)
voice_assistant_websocket.interrupt action when a session is running —
cuts local playback, keeps the socket, notifies the broker with the
existing {"type":"interrupt"} text frame. No new C++.
Broker: device->broker text frames become control messages. On
"interrupt": explicitly cancel any in-flight response (server_vad can't
auto-cancel on speech it never heard; the benign cancel/response.done
race is swallowed), flush the pipeline, rewind the playback clock past
the VAD release margin so the mic opens for the command immediately,
clear stale VAD state, and grant a fresh hygiene window.
Harness: new --wake set (cut mid-reply, wake while idle, bare wake with
no follow-up). Wake 3/3, legacy 10/10, hygiene 3/3 against a dev broker.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds JSON device control frames for wake interrupts. The broker cancels active responses, resets playback and VAD state, and preserves the websocket session. Firmware and harness scenarios support and validate the new flow. ChangesWake interruption flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Device
participant RawPCMSerializer
participant VoicePERealtimeService
participant Pipeline
Device->>RawPCMSerializer: Send {"type":"interrupt"}
RawPCMSerializer->>VoicePERealtimeService: Dispatch parsed control data
VoicePERealtimeService->>Pipeline: Cancel active response
Pipeline-->>VoicePERealtimeService: Finish interruption processing
VoicePERealtimeService->>VoicePERealtimeService: Reset playback and VAD state
VoicePERealtimeService-->>Device: Keep websocket session active
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
broker/tools/harness.py (1)
534-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the scenario-set selection to one helper.
Line 534 and Line 580 repeat the same precedence rule. If you add a fourth mode and update only one site,
--onlyvalidates against one set whilerunexecutes another. The failure is silent.The rule also drops
--hygienewithout warning when a user passes--wake --hygienetogether.Define the selection once and reject conflicting mode flags.
♻️ Proposed refactor
Add a helper near the registries:
def _scenario_set(hygiene: bool, wake: bool) -> dict: if wake and hygiene: raise SystemExit("--wake and --hygiene are mutually exclusive") if wake: return WAKE_SCENARIOS if hygiene: return HYGIENE_SCENARIOS return SCENARIOSUse it in
run:- scenario_set = WAKE_SCENARIOS if wake else HYGIENE_SCENARIOS if hygiene else SCENARIOS + scenario_set = _scenario_set(hygiene, wake)Use it in
main:if "--only" in sys.argv: only = sys.argv[sys.argv.index("--only") + 1] - valid = WAKE_SCENARIOS if wake else HYGIENE_SCENARIOS if hygiene else SCENARIOS + valid = _scenario_set(hygiene, wake) if only not in valid: raise SystemExit(f"unknown scenario {only!r}; one of: {', '.join(valid)}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@broker/tools/harness.py` at line 534, Add a shared _scenario_set(hygiene, wake) helper near the scenario registries that rejects both flags together and returns WAKE_SCENARIOS, HYGIENE_SCENARIOS, or SCENARIOS in the existing precedence order. Replace the duplicated selection logic in run and main so validation and execution always use the same scenario set.broker/realtime_broker/serializer.py (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWiden
deserializeto allow the passedstrpath.
deserializeaccepts bothstrcontrol frames andbytesaudio frames, butRawPCMSerializer.deserializeis annotated withmessage: bytes; use the basestr | bytesspan so type checkers can see the control-frame path.
[maintainability và_code_quality]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@broker/realtime_broker/serializer.py` at line 36, Update RawPCMSerializer.deserialize to annotate its message parameter as str | bytes, matching the base deserialize contract and supporting both control-frame strings and audio-frame bytes. Preserve the existing return type and deserialization behavior.broker/realtime_broker/agent.py (1)
50-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfirm the private upstream hook against the pinned Pipecat installation.
_handle_evt_erroris a private method with no import/attribute guard; changing the base class to remove or rename it makes this override a no-op. Add a startup guard that either checksOpenAIRealtimeLLMService._handle_evt_erroror imports the exact class from the installed Pipecat wheel.The patched code already guards the
evt.error.codechain safely; the remaining work is catching future upstream method renames explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@broker/realtime_broker/agent.py` around lines 50 - 58, Add startup-time validation for the private upstream hook used by _handle_evt_error, confirming that OpenAIRealtimeLLMService exposes _handle_evt_error in the pinned Pipecat installation. Fail explicitly with a clear error if the method is absent or renamed, while preserving the existing guarded event handling and superclass delegation.broker/realtime_broker/server.py (1)
672-678: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid relying on private Pipecat response state.
_current_assistant_responseis a private implementation detail; use the public task-state APIs if available, or usegetattr(service, "_current_assistant_response", None)so an internal rename does not crash every mid-session wake.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@broker/realtime_broker/server.py` around lines 672 - 678, Update the response-cancellation guard in the wake handling flow to avoid direct access to private _current_assistant_response state. Prefer the service’s public task-state API if available; otherwise retrieve the private attribute safely with getattr and a None default, preserving cancellation and interruption behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@broker/realtime_broker/serializer.py`:
- Around line 36-45: Update deserialize to dispatch control frames to on_control
only when json.loads returns a dictionary; treat all other valid JSON values as
malformed and return None. Wrap the awaited on_control call in exception
handling so handler failures are logged and contained within deserialize,
preventing them from propagating into the transport receive loop.
---
Nitpick comments:
In `@broker/realtime_broker/agent.py`:
- Around line 50-58: Add startup-time validation for the private upstream hook
used by _handle_evt_error, confirming that OpenAIRealtimeLLMService exposes
_handle_evt_error in the pinned Pipecat installation. Fail explicitly with a
clear error if the method is absent or renamed, while preserving the existing
guarded event handling and superclass delegation.
In `@broker/realtime_broker/serializer.py`:
- Line 36: Update RawPCMSerializer.deserialize to annotate its message parameter
as str | bytes, matching the base deserialize contract and supporting both
control-frame strings and audio-frame bytes. Preserve the existing return type
and deserialization behavior.
In `@broker/realtime_broker/server.py`:
- Around line 672-678: Update the response-cancellation guard in the wake
handling flow to avoid direct access to private _current_assistant_response
state. Prefer the service’s public task-state API if available; otherwise
retrieve the private attribute safely with getattr and a None default,
preserving cancellation and interruption behavior.
In `@broker/tools/harness.py`:
- Line 534: Add a shared _scenario_set(hygiene, wake) helper near the scenario
registries that rejects both flags together and returns WAKE_SCENARIOS,
HYGIENE_SCENARIOS, or SCENARIOS in the existing precedence order. Replace the
duplicated selection logic in run and main so validation and execution always
use the same scenario set.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a6f2a1e-179f-410f-8da8-979398219aaf
📒 Files selected for processing (5)
broker/realtime_broker/agent.pybroker/realtime_broker/serializer.pybroker/realtime_broker/server.pybroker/tools/harness.pyfirmware/voice_pe_dual.yaml
A text frame of 123/null/[] parses fine but isn't a dict, so the control handler's msg.get() would raise inside the transport receive loop and kill the session's audio input. Drop non-object frames and contain handler exceptions instead. Addresses CodeRabbit review on #10. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
On the nitpicks: |
CI installs latest ruff unpinned; 0.16.2 flags directives that 0.12-era ruff required. All changes are lint-mechanical, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
"Hey Mycroft" during an open session used to stop/start the websocket: goodbye cue on every re-wake, reconnect ate the start of the command, and a bare wake left the in-flight reply playing (the mic gate feeds server_vad silence during playback, so it can never auto-cancel).
Firmware (
voice_pe_dual.yaml): mid-session wake now calls the previously unusedvoice_assistant_websocket.interruptaction — cuts local playback, keeps the socket, notifies the broker via the existing{"type":"interrupt"}text frame. Zero new C++.on_stopped's end cue now only fires on real session ends.Broker: device→broker text frames become control messages (
RawPCMSerializer.on_control). Oninterrupt: explicitresponse.cancel(guarded; the benign race withresponse.doneis swallowed inVoicePERealtimeService), pipeline interruption flushes the ~2s send-ahead, playback clock rewinds past the VAD release margin so the mic opens immediately,reset_vadclears stale state, hygiene grants a fresh window/budget/grace.Tests (dev broker on 8766): new
--wakesuite 3/3 — mid-reply cut leaves 0.2s tail then answers a follow-up; wake while idle answers next turn; bare wake with no follow-up stays silent. Legacy 10/10,--hygiene3/3.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests