fix(voice): ignore empty first chunk for speaking event - #850
Conversation
WalkthroughChangesVoice speaking-event handling
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
javascript/src/voice/__tests__/interrupt-truncation.test.ts (1)
127-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument how this test is run.
Add the required test-file documentation covering the command to run it, coverage expectations, test dependencies, and a concise usage example. As per coding guidelines, test files must document these requirements.
🤖 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 `@javascript/src/voice/__tests__/interrupt-truncation.test.ts` around lines 127 - 137, Document the test setup for the “does not set the speaking event when the first chunk is empty” case in the surrounding test file. Add the required command for running the file, coverage expectations, test dependencies, and a concise usage example, following the repository’s existing test-file documentation conventions.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@javascript/src/voice/__tests__/interrupt-truncation.test.ts`:
- Around line 127-137: Document the test setup for the “does not set the
speaking event when the first chunk is empty” case in the surrounding test file.
Add the required command for running the file, coverage expectations, test
dependencies, and a concise usage example, following the repository’s existing
test-file documentation conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17780c2e-4420-43f1-a6d0-d94b1a80cf47
📒 Files selected for processing (2)
javascript/src/voice/__tests__/interrupt-truncation.test.tsjavascript/src/voice/adapter.runtime.ts
langwatch-agent
left a comment
There was a problem hiding this comment.
Requesting changes: 1 P1 correctness issue. See the inline comment.
| ); | ||
| if (first.data.length > 0) { | ||
| onFirstChunk(); | ||
| speakingEvent.set(); |
There was a problem hiding this comment.
P1 — A leading empty chunk does not mean the response contains no playable audio. drainInner continues reading after this first chunk, so an input sequence such as [empty, non-empty, end] emits audio but never calls onFirstChunk() or sets speakingEvent. That leaves the speaking/interrupt state inconsistent for a real response. Keep discarding zero-length prefixes until the first playable chunk (or terminal end), then set the event at that point; add a regression test for an empty chunk followed by audio.
There was a problem hiding this comment.
Addressed in 3380f97.
drainInnernow marks the first playable chunk wherever it appears, so a leading empty chunk no longer suppressesonFirstChunk()orspeakingEvent.set().- Added the requested regression for
[empty, non-empty, end], while retaining the empty-only assertion. - Verified
interrupt-truncation.test.ts(12 passed), the Voice suites (436 passed, 1 skipped), and the full JavaScript suite (1036 passed, 4 skipped). Build, lint, typecheck, and CJS/ESM smoke checks also pass.
Could you please re-review?
langwatch-agent
left a comment
There was a problem hiding this comment.
Static review found no actionable voice-runtime or liveness issue in the current fork change. Empty initial chunks no longer signal speech, while the later playable chunk still starts the recorder and speaking event; interruption remains time-bounded elsewhere. I did not execute the untrusted head or its scripts.
langwatch-agent
left a comment
There was a problem hiding this comment.
Static review of external contribution at 3380f97992847641f75449b012c4eb74f9f1da03 (no PR-branch code or dependencies executed). No merge-blocking issue found. The speaking event is now set only after the first playable audio chunk, while an initially empty chunk no longer produces a false speaking transition; the expected empty-first-then-audio and empty-only behavior is covered. CI evidence was considered but not rerun under the external-contribution trust boundary.
langwatch-agent
left a comment
There was a problem hiding this comment.
External static review: no blocking concern found in the current diff. I did not execute branch code, install dependencies, or run contributor-provided scripts. Residual risk: runtime behavior remains covered by the repository CI.
LangWatch-Review: verdict=clean sha=3380f97992847641f75449b012c4eb74f9f1da03 p0=0 p1=0 p2=0 p3=0
langwatch-agent
left a comment
There was a problem hiding this comment.
External static review: no blocking concern found in the current diff. I did not execute branch code, install dependencies, or run contributor-provided scripts. Residual risk: runtime behavior remains covered by the repository CI.
LangWatch-Review: verdict=clean sha=3380f97992847641f75449b012c4eb74f9f1da03 p0=0 p1=0 p2=0 p3=0
Human Review BriefCaution This changes when Previously the TypeScript voice runtime resolved the event on the first audio chunk, empty or not. An empty first chunk does not prove the agent spoke, so a silent turn could be treated as active speech by interruption logic. The fix waits for the first playable chunk, whether that is the initial one or a later one, and leaves the event unset for a response that contains only empty terminal chunks. Both halves matter and the PR gets both. Gating naively on the first chunk would have missed real audio arriving after an empty prefix, which is the obvious wrong fix. The consequence to think about is the other direction: a turn that produces no playable audio now never resolves the event at all. Anything awaiting it needs to handle that, and the timing of playback start moves later by however long the empty prefix lasts.
The one useful checkFind the callers. If anything Playback timing also now starts later, at the first playable chunk rather than the first chunk. If any metric measures time-to-first-audio off that clock, its numbers move. Note The event should mean the agent actually spoke. It now does, and the case where it never fires is the one to trace. |
Why
An empty first audio chunk does not itself prove that the agent spoke, but the TypeScript voice runtime resolved
AgentSpeakingEventimmediately. That could make interruption logic treat a silent turn as active speech, while simply gating on the first chunk could miss real audio that follows an empty prefix.Closes #569
What changed
AgentSpeakingEventand start playback timing only when the drain observes its first playable audio chunk, whether that is the initial chunk or a later one.Test plan
does not set the speaking event when the first chunk is emptyfailed before the runtime fix (expected false, receivedtrue).sets the speaking event when audio follows a leading empty chunkfailed before the review fix (expected true, receivedfalse).pnpm exec vitest run src/voice/__tests__/interrupt-truncation.test.ts— 12 passed.pnpm exec vitest run src/voice/__tests__ src/voice/adapters/__tests__— 45 files passed, 1 skipped; 436 tests passed, 1 skipped.pnpm test:ci— 89 files passed, 1 skipped; 1036 tests passed, 4 skipped.pnpm lint:allpnpm typecheck:allpnpm build:allpnpm smoke:dist— CJS and ESM distributions load successfully.How I can prove I was successful
No playable artifact — this is an internal voice-runtime state fix. Run the named regression tests in the Test plan; they verify the event stays unset for an empty-only response and becomes set when playable audio follows an empty prefix.