Repository navigation
feat(voice-transport): fail-fast client hardening + canonical transport consolidation (voice plan WS1 Steps 15–18) - #2690
Conversation
|
Post-review addition 🤖 Generated with Claude Code |
|
Review follow-up pushed: 2ee1cd6. P1 — every terminal conclusion now exposes an awaitable close-handshake completion
New test vectors (8 rows):
🤖 Generated with Claude Code |
|
Merge position: Stage 3 — after #2689 (merge commit, not squash). Sibling of #2691 (either order between the two). After this merges, cinny #393 (already re-vendored verbatim at this exact head 🤖 Generated with Claude Code |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Fresh out of draft, zero prior reviews. Ran everything myself:
tests/web-voice-transport.test.ts: 70/70 pass (19 suites).tests/web-voice-transport-delivery.test.ts: 9/9 pass.tests/voice-connect-resolver.test.ts: 12/12 pass.
Specifically verified the claimed bug fix ("attempt concluded ⇒ generation invalidated") against its own repro test - server close (1006) while getUserMedia is parked: server drops the socket while the mic permission prompt is open, user grants late, and the test asserts directly against internal state (micStream === null, statsTimer === null, granted track stopped === true) that the dead attempt never resumes and doesn't leak the mic grant or a stats timer. Non-vacuous - it exercises the real race, not a mocked shortcut.
Didn't deep-read every line of the 26-file diff, but the transport's core invariants (generation fencing, terminal-state latching, close-code decoding for 4409/4410, mic failure classification) are all directly test-covered and green.
|
Review note (exact head |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approved current head 2ee1cd6 after exact-head validation: npm run typecheck PASS; voice-connect-resolver 12/12 PASS; web-voice-transport-delivery 9/9 PASS; web-voice-transport 70/70 PASS; voice-error-classifier 15/15 PASS. GitHub currently reports MERGEABLE but BEHIND; required CI is not present on this head, so do not merge until branch is updated and checks pass.
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
…on (WS1 Steps 15+18)
Canonical src/web-voice-transport.ts — plan WS1 Steps 15 and 18 as amended
by R10/R12/S6/W5/X6/Z6; design 1e + 1a-prime client rules.
Step 15 (consolidation i — cinny additions reconciled upstream):
- mute/deafen call controls merged verbatim from the cinny vendored copy:
micMuted/deafened fields, setMicMuted/setDeafened, the onaudioprocess
send gate and the playChunk playback gate. Canonical keeps its richer
two-arg classifyMicError, onDebug, VoiceCloseInfo, and debug counters
(the canonical module is the superset).
- new export classifyMicErrorCode(name): 'permission'|'device'|'unknown'
(machine-readable class for provider routing).
Step 18 (design 1e):
- 6s connect timeout (CONNECT_TIMEOUT_MS, constructor-overridable); armed
in connect(), cleared on open and every terminal path; fire = latched
terminal error.
- attempt-generation token: every socket callback and every post-await
continuation (connect's AudioContext.resume, startMic's getUserMedia and
resume, the post-startMic continuation) re-checks the generation (R10);
stale grants stop their own tracks — no replacement socket, no capture
leak.
- disconnect() terminalizes (S6): invalidates the attempt first, clears
timers, then synchronously emits exactly ONE 'closed' — never dependent
on the generation-fenced onclose; the terminal-error cleanup path stays
separate and preserves the latched 'error'.
- terminal-state latching: a timeout/error is terminal for the attempt;
the self-inflicted close tears audio down but never overwrites 'error'
with 'closed'.
- agent.state v1 client handling (1a-prime): mic/'Live' stay keyed on
onopen; 3s legacy window (no frame => behave exactly as today);
connecting/backoff are progress detail; idle->connecting->live is the
normal wake-up; upstream:'failed' is a terminal CLIENT transition —
invalidate, stop mic/stats/playback, close the socket, latch the
classified error, suppress the self-inflicted onclose. AgentStateV1 is
duplicated here (vendored standalone) and pinned to the v1 frame.
- exported failure union + onConnectFailure (R12+X6+Z6+W5):
'timeout'|'connect-error'|'mic-permission'|'mic-device'|'mic-other'|
'agent-failed'|'service-down'|'client-busy', each with classified detail
+ remediation hint. 'connect-error' is the browser-observable pre-open
failure ('refused' is NOT browser-observable — Z6); 'service-down' is a
supervisor-status conclusion produced by surfaces, never by the
transport; 'mic-other' remediation is Retry, never credential repair
(X6); close 4409 'client-busy' decodes to kind 'client-busy' (W5).
- close 4410 'superseded-by-takeover' decodes to its own terminal
VoiceStatus 'superseded' (call moved; not a connect failure).
- testability seam wsFactory + explicit fire-and-forget connect() doc;
additive sendTextInput/setPlaybackRate for the webUI page dedup.
Tests (tests/web-voice-transport.test.ts): node-driven state machine via
fake sockets + AudioContext/getUserMedia stand-ins — timeout latch, stale
onclose fencing, slow-startMic across attempts, disconnect during each
await, S6 single-closed vectors, legacy-window fallback, backoff/failed
frames, 4409/4410 decoding, mic-failure kind mapping, failure-union
remediation table, mute/deafen gates. 53 tests green; tsc --noEmit clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…WS1 Step 16) Transport consolidation (ii), plan WS1 Step 16 as amended by R11 — the webUI page (what the room-Call iframe executes) now inherits the canonical transport's timeout/latching/agent.state behavior instead of its older inline copy. - The page loads the existing served IIFE artifact via a classic <script src="/web-voice-transport.js"> (pipeline unchanged — no second build path) and instantiates SutandoVoice.VoiceTransport. - Inline connectWs/startMic/stopMic/playChunk and the PCM DSP copies are deleted. The page keeps ONLY surface concerns: transcript DOM, status text, stats panel via onStats, avatar analyser via onAnalyser, Chrome interim-STT, tool cues, and reconnect policy (clean-vs-drop ladder on 'closed', exactly as before). - New failure surface per the canonical union: terminal failures (timeout/connect-error/agent-failed/mic-*) reset the voice UI and print classified detail + remediation; 'client-busy' renders the W5 take-over affordance (reconnect with ?takeover=1); status 'superseded' (close 4410) renders "call moved" and never auto-reconnects. - mute button drives transport.setMicMuted; speech_speed frames drive setPlaybackRate; the text box sends through sendTextInput while live. Embedded page script remains plain JS (no TS-only syntax) — verified by classic-script parsing of the served page. - DSP drift-guard framing retired: the page uses the module directly, so equivalence-by-copy is no longer a thing to pin. Delivery test (R11): asserts the artifact is classic-script IIFE JS defining SutandoVoice.VoiceTransport — NOT ESM (vm.Script parse + global install), covers the Step-15/18 export surface, and adds a serve-path smoke that spawns web-client and asserts GET /web-voice-transport.js serves that JS and the page references it via a classic script tag. build:bundle green (28KB browser IIFE); full tsx suite 804/804; web-client python suites and tsc --noEmit clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eneration
Reviewer-reproduced blocker (voice plan WS1 Steps 15/17/18 fix round): the
attempt-generation token was only bumped by connect(), disconnect(), and the
upstream-failed transition. Every other concluding path — all handleClose
branches (ordinary drop, 4000 goodbye, 4409 client-busy, 4410 superseded,
terminal-latched return), onConnectTimeout, handleOpen's mic-error catch,
and handleSocketError's pre-open branch — latched terminal/attemptActive
only. The post-await fences check gen alone, so a server close arriving
while startMic's getUserMedia await was parked let the dead attempt resume:
status('live') overwrote the close-derived/latched status, the just-granted
mic stream stayed captured, and the statsTimer leaked (the leak literally
keeps the node:test runner alive without --test-force-exit).
Fix: the uniform invariant "attempt concluded => generation invalidated" —
every concluding path now bumps attemptGen exactly as upstream-failed and
disconnect() already did. handleClose bumps once at the top (its closure
fence guarantees it only runs for the current attempt); the terminal-latched
branch keeps teardown as defense in depth. Audit fallout fixed alongside:
the mic-error catch relied on the trailing self-inflicted onclose (now
fenced out) for teardown, so it tears its own audio graph down and clears
the legacy timer itself.
Also in this pass:
- connect()'s new-attempt reset flushes scheduled playback, so a direct
second connect() over a live session cannot keep speaking old audio into
the new attempt (nextPlayTime restarts on the new clock);
- onStatus doc: close info accompanies every 'closed' decoded from a WS
close frame; user-initiated disconnect() emits 'closed' without it.
Tests (fake-socket harness): server 1006 while getUserMedia is parked keeps
'closed' final (no live resurrection, tracks stopped, no statsTimer leak);
4409 while parked keeps the latched client-busy error; R10 vector for
startMic's INTERNAL audioCtx.resume() await (park via resumeHook,
disconnect while parked, newer attempt's micStream never clobbered, exactly
one 'closed'); connect()-over-live flushes playback. Verified the first
three fail against the unfixed transport (3 fail / 54 pass) and the full
suite is 812/812 green with the fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
voice plan WS1 Steps 15/17/18 fix round: Step 17 wants voice-connect-resolver.ts vendorable verbatim, but the cinny copy had grown local additions upstream never received — the 'tailnet' VoiceTier (mesh-VPN address, preferred over same-subnet-only lan), directWsUrl() (https -> wss/:443, http -> ws with the advertised port, bare host -> :proxyPort/ws), the localPort-vs-proxyPort split (off-localhost tiers dial the webUI /ws proxy since the voice-agent WS binds loopback-only), and agentEndpoint identity-routing (dial the room-agent's advertised endpoint, never the generic local host that would answer as the wrong agent). This makes the upstream file byte-identical to the cinny copy (verified with cmp; the cinny content is the newer superset — nothing upstream had is dropped, and the only behavioral change, lan candidates moving from ws://host:9900 to ws://host:proxyPort/ws, is the documented loopback-bind correction with no upstream dependents). New tests pin the reconciled surface: tailnet slots between local and lan, off-localhost candidates use the proxy port, directWsUrl's three mappings, and agentEndpoint routing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ake completion (T8) The transport's disconnect() was fully synchronous: it bumped the attempt generation, emitted the single synchronous 'closed' (S6), and called ws.close() — which only STARTS the async WebSocket close handshake; the socket's real 'close' event is generation-fenced and unobservable to consumers. Amendment T8 requires lease owners to await teardown before releasing the connection lease, which needs the handshake to be awaitable. disconnect() now returns Promise<void> resolving when the underlying socket's real 'close' event fires. Purely additive: - the synchronous behavior (gen bump, timer clears, teardownAudio, the single sync 'closed' emission) is byte-for-byte unchanged, and the sync 'closed' always fires before the promise resolves; - the close listener wraps the socket's `onclose` property — the wsFactory seam guarantees only the onopen/onmessage/onerror/onclose + send/close/ readyState surface (no addEventListener on fakes) — attached BEFORE ws.close() and BEFORE this.ws is cleared; the wrapped previous handler (connect()'s generation-fenced closure) still runs, a guaranteed no-op after the bump; - no socket, or readyState already CLOSED → immediate resolve; - a wedged handshake is bounded by new ctor option disconnectCloseTimeoutMs (default exported DISCONNECT_CLOSE_TIMEOUT_MS = 1500) so teardown can never hang lease release; - close() alias returns the same promise. Tests (T8 block): resolves only after the fake socket's delayed close event (sync 'closed' first, still exactly one 'closed'); close() alias waits the same way; no socket → immediate; already-CLOSED socket → immediate; injected short bound → fallback resolves a never-closing handshake. Full tsx suite 817/817, tsc --noEmit clean, build:bundle OK. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…close-handshake completion (P1) Transport-initiated TERMINAL failures (connect timeout, mic failure while the socket is open, pre-open socket error, upstream-failed teardown) used to latch + emit onConnectFailure and only then self-close the socket, with no way for consumers to know when the close handshake actually finished. A consumer releasing a single-client lease inside onConnectFailure raced the handshake: the server still counted the old client and rejected the next attempt with a spurious 4409. - Factor disconnect()'s close-completion tracking (onclose property wrap, immediate resolve for no-socket/CLOSED, bounded by disconnectCloseTimeoutMs) into a private trackCloseCompletion() helper and use it at EVERY self-initiated terminal close site: the mic-error catch, onConnectTimeout, the pre-open handleSocketError branch, the upstream-failed teardown, and disconnect() itself. - New public closeSettled(): the CURRENT attempt-conclusion close completion, latched BEFORE the terminal status/onConnectFailure emits so it can be read from inside the callbacks. Close-derived conclusions (handleClose: plain close, 4409, 4410, pre-open close) latch an already-resolved completion — the socket is already closed there. - disconnect() with no socket now returns the current conclusion's completion, so `await disconnect()` after a terminal latch still covers the in-flight handshake of the socket the latch already closed. - Synchronous observable ordering (S6/R10 latching, single closed emission, generation bumps) is unchanged. - Tests: closeSettled timing rows for mic-error / timeout / pre-open error / upstream-failed (settle only on the socket's real close event), already-settled rows for server-initiated closes, the bounded fallback, and disconnect()-after-latch coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
40eb8e6 to
a48577f
Compare
|
@cla-assistant check |
|
Stack rebuild complete at |
Part of the voice reliability & onboarding plan (desktop repo:
docs/design-voice-reliability-and-onboarding.md§1e,docs/impl-plan-voice-reliability-and-onboarding.mdWS1 Steps 15–18 + amendments R10/R11/R12/S6/W5/X6/Z6). #2689 is merged; this branch is rebuilt on its merge and currentmain, so the PR diff is transport-only.What changed, and why?
Seven commits, one concern: the web voice client fails fast and predictably, from one canonical transport.
4aa02b86—src/web-voice-transport.tsbecomes the single canonical client transport (design 1e):awaitboundary (AudioContext resume,getUserMedia, mic start) re-checks the attempt generation, so stale continuations can't resurrect a dead attempt.agent.statehandling: consumes the voice: agent.state v1 protocol emitter + lifecycle snapshot (phase 1, stacked on #2684) #2689 protocol (connecting/backoff/failedreflected in status detail;upstream: failedis a terminal client teardown), with a 3 s legacy fallback for pre-protocol agents.onConnectFailurereportstimeout | connect-error | mic-permission | mic-device | mic-other | agent-failed | service-down | client-busy; close codes 4409 (client-busy, W5) and 4410 (superseded-by-takeover → its own terminalsupersededstatus, never a failure callback) are decoded.classifyMicErrorCodemerged upstream so the vendored copies can be verbatim again.c7662cfb—web-client.ts's voice page drops its inline duplicate transport and runs on the servedSutandoVoiceIIFE (/web-voice-transport.js), Step 16/R11: one implementation, surface concerns only. Two additive public methods (sendTextInput,setPlaybackRate) keep the page at feature parity.710b75bd— fix round from adversarial review (blocker): attempt-concluding paths (handleCloseall branches, connect timeout, mic-error) now bump the attempt generation — uniform invariant attempt concluded ⇒ generation invalidated. Without it, a server close arriving while thegetUserMediaprompt was open let the dead attempt resume tolivewith the mic captured and the stats timer leaked (repro'd in tests, below). Also:connect()-over-live flushes stale scheduled playback; events-doc correction.8e3dedb3—src/voice-connect-resolver.tsreconciles cinny'stailnettier ladder upstream (byte-identical to the cinny copy), so both vendored voice files are now verbatim and drift-gated in cinny.d8947985—disconnect()returns an awaitable completion that settles after the WebSocket close handshake (bounded for wedged peers), so lease release cannot race transport teardown.370e2f58— every terminal conclusion exposes the same awaitablecloseSettled()contract, including timeout, mic failure, server close, 4409/4410, and upstream-failed paths.a48577fb— removes redundant probe initialization from the reconciled resolver.What files / sections should reviewers look at first?
src/web-voice-transport.ts: the class-header invariants comment, thenhandleClose/onConnectTimeout/ the mic-error catch (the generation-invalidation fix), thendecodeCloseInfo(4409/4410).tests/web-voice-transport.test.ts: the "fix round" describe block — the two blocker repros are the spec of the bug.src/web-client.ts: the voice page script block (now plain JS onSutandoVoice, per the CONTRIBUTING embedded-script rule).What user behavior or bug does this prove?
client-busy; a takeover supersedes the old client with its ownsupersededterminal state instead of a spurious error.Feature/documentation consistency
N/A for
docs/canonical documents — same rationale the reviewers accepted on #2689:docs/catalog.jsonhas no canonical doc for the web voice client; the canonical spec for this cross-repo feature is the desktop repo'sdocs/design-voice-reliability-and-onboarding.md(§1e), and module-level docs live in the source headers here.docs/voice-agent-test-framework.md(agent-side test framework) is unaffected.Before/after evidence
New discriminating vectors run against the parent commit (detached worktree at
387539a2, test file only overlaid from3eff47bf):(
--test-force-exitis required before the fix: the leaked statsTimer from the repro hangs the runner — the leak is itself part of the bug.)Same suite at HEAD:
What tests did you run?
npx --no-install tsx --test tests/web-voice-transport.test.ts tests/web-voice-transport-delivery.test.ts tests/voice-connect-resolver.test.ts— 91/91, 0 fail (transport 70, delivery 9, resolver 12).npx --no-install tsc --noEmit— clean.git diff origin/main...HEAD | bash scripts/review-checks.sh— PASS (hardcoded paths clean).npm run build:bundleOK (28 KB IIFE), and the affected Python web-client suites green.Touching a live path?
The served voice page and its transport are client-side; the delivery suite spawns the real
web-clientand exercises the serve path (above). The full user-visible round trip (mic click → typed failure / live call on a real agent) runs through the desktop app: it will be verified via the desktop repo'sscripts/verify-branch-build.shDMG pass with this branch pinned — same protocol as the Phase-1 PRs — and the transcript/console evidence will be appended here before this PR leaves draft.Stacked PR?
Parent #2689 merged as
ffc0e0af. This branch was rebuilt on that merge/currentmain; all seven transport patches are unchanged byrange-diff, and the PR diff is now child-only. Next: this PR; bodhi #27 provides the 4409/4410 server half (either order — the client tolerates it absent). The paired cinny PR must re-vendor the rebuilt source tree verbatim.Hardcoded-path check
git diff origin/main...HEAD | bash scripts/review-checks.sh→ PASS on rebuilt heada48577fb.Manual verification — packaged DMG pass (2026-08-06)
Local DMG with this branch as the engine transport (integration merge @
4bb2d9a9+ phase-3 preference; bodhi pinned at the dist-rebuilt PR-27 head9f027da; desktopvoice/phase3-control-plane; cinnyvoice/phase3-enable-flow— whose vendored copy is byte-identical to this branch's file, so every client scenario below exercised this transport verbatim).Live-path round trips (CONTRIBUTING evidence). Real conversation through the transport, from
conversation.sqlite(SELECT ts_unix,kind,substr(text,1,60) FROM voice ORDER BY ts_unix DESC):The
tasks/task-1786041727122.txt → results/file round trip completed in the same exchange — the voice→core bridge through this transport, not just the Gemini loop.Fail-fast + reconnect (incident class). Three
kill -9s of the agent mid-session: the client latched a typed error each time (no self-resurrecting "Live"), and each user-initiated reconnect attached to the replacement within seconds. The[VoiceFailure] classifier wired into transport.onCloseboot line confirms the classifier active in the packaged agent.W5/4409/4410 decode, end-to-end through a real bodhi server (first live proof of the takeover wire contract): with a call active in the app, the webUI second client showed "Voice is in use on another surface — Take over the call here" (4409 busy + affordance, no call stealing); taking over superseded the app client, which rendered its own calm terminal ("Call moved — your voice call was taken over on another surface"), i.e. the 4410 →
supersededstatus path, not an error. Reverse direction also checked: second client busy-rejected, incumbent call unaffected.Step-16 page on the canonical IIFE: the webUI surface used throughout S6 is the served
SutandoVoicetransport page from this branch — connect, converse, busy-reject, and take over all through/web-voice-transport.js.Verification/handoff timing through this transport:
stage=verify code=livethenenable-voice-handoff … ms=44/ms=41(same live attempt handed to the session, no second dial).🤖 Generated with Claude Code