Slice 2: Python/FastAPI voice gateway - #22
Merged
Merged
Conversation
Nine documents covering product definition, verified current state, data architecture, auth, the composed Deepgram voice pipeline, AI pipelines and observability, the cleanup inventory, the build order, and the decision log. Key decisions: BC/Canada market (PIPEDA), Supabase in ca-central-1 with forced RLS, Supabase Auth, composed Deepgram STT -> GPT-5.6 -> Aura TTS, Langfuse under a no-PHI telemetry contract, vision shelved. Co-Authored-By: Claude <noreply@anthropic.com>
Ran the tooling 07 was originally written without. Findings: - No CI exists, and .husky/pre-commit is not executable, so nothing has ever gated typecheck, test, or build. - The test suite was 100% non-executing: @testing-library/dom is an undeclared peer dep of @testing-library/react v16. Declaring it takes the suite from 0 tests running to 192/203 passing. - 20 tsc errors, all in test files. Production code typechecks clean. - src/lib/date-utils.js is a committed tsc build artifact shadowing its own .ts source. - knip reports 116 unused files vs this doc's ~50; both tools have false positives, so verify per file. Revises the execution order: a new phase 0 fixes the build gate before any deletion, and Vapi/database work moves out of cleanup into 08. Co-Authored-By: Claude <noreply@anthropic.com>
Vision is kept. No vision code is deleted or refactored by the cleanup, explicitly including the ~1,099 LOC that currently has zero importers. 07 section 3 becomes a do-not-touch list rather than a deletion list. Cleanup shrinks from ~14,800 LOC to ~9,500 across 104 files (knip's 116 minus 12 vision files). The vision phase is gone; @mediapipe/tasks-vision, 1eurofilter and dynamic-time-warping stay in package.json. Surfaced while verifying: analyzeStandingLumbarFlexion never enters its flexion phase. It is defined twice — form-engine.ts:319 (live, dispatched at :1205/:1239) and exercises/lumbar.ts:14 (orphaned) — and the live one gates on hipAngle <= 90 while the test encodes a looser threshold. Under the old plan that test was going to be deleted; now it is a live bug. Also corrects two earlier claims: scripts/seed-pt-data.mjs is NOT wired to package.json, and src/lib/date-utils.ts is a knip false positive (its committed .js build artifact shadows it). Co-Authored-By: Claude <noreply@anthropic.com>
ADR-003 was reversed to "vision retained in full"; five docs still described it as shelved. - 00: decision table row -> "Retained in full, excluded from cleanup" - 01: header, deferred list, scope-conflict table. Keeps "AI-observed form correction" deferred as a *clinical claim* and adds a note separating that from the code decision — stage G is deliberately not renegotiated. - 02 §7: reframed from "blast radius of removal" to the data path vision feeds, which is now something the rebuild has to carry rather than drop. - 09: status table + superseded map. Old ADR-002 is now *partly* superseded (billing premise dies with CPT, vision does not), and old ADR-004 (server-side vision analysis) moves from Moot back to Open. Co-Authored-By: Claude <noreply@anthropic.com>
Residency had been framed as two moves: buy the region where a vendor sells it, or design around it by sending no PHI. Speech fit neither, so gate 2 was written as an open-ended wait on Deepgram's roadmap. There is a third move. Deepgram ships a self-hosted deployment, so residency is a question of where we run containers rather than where Deepgram operates. 05 §6 already documented this; 08 and 09 had not absorbed it. Cloud credits (GCP/Azure/AWS) are the budget for it. - ADR-013 (Proposed): self-host in AWS ca-central-1, co-located with Supabase; re-aim gate 2 from a residency request to an Enterprise sales call. Records the License Server outbound dependency, the new GPU ops surface, and the rule that credits buy capability, never discount. - ADR-010 amended: the self-hosted model question is now load-bearing. The bake-off has three arms, and if streaming nova-3-medical is not available self-hosted, residency and the medical model are mutually exclusive — a clinical-risk call, not an engineering one. - ADR-008 amended: LLM residency is a fork, not a checkbox. No GPT-5.x on standard deployment in Canada; PTU in Canada East is the only GPT-5-class route. Canada or GPT-5.6 tiering, probably not both. - ADR-009 amended: self-hosted Langfuse considered and rejected — five stateful services to hold PHI we have already decided not to emit. - ADR-011 amended: three residency responses, not two. - 08 §4a: which gates credits actually move, and the explicit do-not-replatform-off-Supabase rule. - 05 §6: the Canada-vs-US escape hatch is closed by ADR-011. - 00-README: stale 07 LOC figure corrected to ~9,300. Co-Authored-By: Claude <noreply@anthropic.com>
The exercise library, its metadata, prompt tuning, and the eval set are not four workstreams. They are one clinical-authoring bottleneck with three pieces of software hanging off it, and the bottleneck is the same physiotherapist named in gate 9. Key positions: - Two kinds of metadata. Descriptive (names, cues, images) is commodity and we already have the images. Clinical decision metadata (contraindication predicates, dosage bounds, progression edges) is what no library vendor sells, because selling it would make them a medical device. Buying a library solves the cheap half. - NASM rejected: free marketing resource with no commercial licence, and a US personal-training credential is the wrong provenance in front of a CPTBC-regulated signer. Clinical layer comes from JOSPT CPGs plus the clinical lead; AI drafts, physio signs, unsigned content cannot enter a plan. - Data model gaps against the current schema: no progression graph, no tenancy, no versioning, no provenance, and contraindications as free text cannot gate anything. Plans must reference an exercise *version*. - Templated plans collapse the AI's job from composition to selection plus bounded personalization — and the templates are simultaneously the gate-9 sign-off artifact and the eval reference. - Evaluation in four layers: deterministic gates carry most of the safety; golden cases split into classification vs plan fidelity so a failure says which half broke; red-flag cases are pass/fail and block release; clinician edit distance is a free continuous signal because the authority boundary already forces human approval. Instrument that in stage 8 or lose the pilot's best data. Also records an asset/scope mismatch: 01 scopes first release to non-acute knee, but ~half the 31 exercise images are lumbar-specific. Blocks the authoring list. Co-Authored-By: Claude <noreply@anthropic.com>
Ordering pathways by joint complexity — degrees of freedom, elbow before hip — is sound biomechanics and the wrong variable. The system is not modelling the joint; it is classifying a presentation, selecting a template, and not missing anything dangerous. The axes that decide difficulty here are red-flag burden, classification tractability, pose trackability, and clinic volume. Order: knee → shoulder → hip → low back → distal. - Knee first, confirming 01's existing scope: low red-flag burden when non-acute, distinct categories with established special tests, and the best joint on the board for MediaPipe. The live squat analyzer is already pathway 1. - Shoulder second on purpose. Hip is the easier second pathway, which is the argument against it — too similar to knee to reveal whether the content model generalizes. Better to break an abstraction at pathway two than pathway five. - Low back fourth despite being the highest-volume complaint: highest red-flag burden in MSK care, contested classification with no reliable routing target, and lumbar segmental motion is not observable in 2D pose landmarks — which is also why 07 §0f's flexion bug is a real bug and not a bad constant. - Distal joints last. Simple is not the same as valuable. Consequences recorded: ~half the exercise images and the orphaned lumbar vision analyzers are reserved for pathway 4, not dead — ADR-003's retain-everything call now reads as sequencing. 07 phase 3 amended so the two docs agree. Clinical claims here are the clinical lead's to confirm; routed with gate 9. Co-Authored-By: Claude <noreply@anthropic.com>
…y contract to real traffic Four inputs from the user changed the plan's shape: - The old app is a frozen reference with **no users**. 08 §0's claim that /progress "renders fabricated clinical history to real users" was wrong. It is a credibility bug, not a safety one, and cleanup carries no merge cost. - The near-term forcing function is a **funding milestone**, not a pilot. Every red gate blocks real PHI; a demo runs on synthetic data. So the gates are the critical path to the first patient and not to a raise. 08 §4's unqualified "these gates are the critical path" is now qualified. - Observability should be **full-content on synthetic, structure-only on real**. - The clinical lead is "kinda identified" — not committed. Changes: - 08 §1 rewritten: ten serial stages become three concurrent tracks (A foundation / B clinical content / C vendor gates). Stage numbers are unchanged so the rest of the doc set still resolves. - Track B added — the clinical-content workstream from doc 10 had no row in the build order at all despite being the longest pole. B0 is recruiting. - 08 §1a "Two deadlines, not one" — raise vs first real patient. - Stage 9 split: 9a (harness) moves from last to just after stage 6; 9b (golden cases) stays behind Track B. The harness is the only work item that is both durable architecture and legible to a non-engineer. - 08 §3a — two-tier observability, with the discriminator as two Langfuse projects rather than a runtime boolean, so production cannot reach the verbose project regardless of program state. - 06 §3 scoped to real patient traffic; ADR-009 amended; ADR-014 added. - 00-README: stale "nine documents" -> eleven (third count drift found). Co-Authored-By: Claude <noreply@anthropic.com>
Adds doc 11 — three mermaid diagrams: where components live relative to the ca-central-1 border, one turn of a voice intake, and what gets recorded where. Mermaid rather than image files so they diff like code. Scopes the first vertical slice to voice intake -> plan generation -> clinician approval (ADR-015). Vision is excluded from the slice and will be rebuilt from scratch rather than ported. Vision code is NOT deleted or refactored — ADR-003's retention stands exactly as written, and doc 08 stage 2 now says so explicitly, since "out of scope" reads like a licence to sweep and is not one. Side effect: 01 stage G's "never claims to observe or correct form" is now simply true for the first release, closing a tension the docs have carried since ADR-003 was reversed. Co-Authored-By: Claude <noreply@anthropic.com>
Two runtimes. Next.js owns the web app, auth, and every database access path; a Python/FastAPI service owns the voice gateway, extraction, plan composition, and evals. The driver is velocity — the engineer is materially faster in Python, and three of the four slice workstreams are in Python's strongest territory. The obstacle was that doc 03's db.rls/db.admin boundary is enforced by an ESLint rule, and ESLint does not run on Python. Resolved by not duplicating the boundary: the Python service holds no database credentials and returns validated Pydantic models for the TypeScript tier to persist. "All DB access goes through one boundary" stops being a lint rule and becomes a property of the deployment. Fifth choke point in the architecture, and the first one that was free. Rejects LangChain/LangGraph: the model routing is precisely what doc 01 forbids, and the question graph must be signable by a physiotherapist, which a data file is and a program is not. Langfuse is unaffected. Also flags that Langfuse's Python @observe() captures args and returns by default — a naively decorated extract(transcript) leaks PHI on the happy path. Defence is capture_input=False in production plus keeping PHI out of traced signatures, not masking. Amends 03, 05, 06, 08, 11 and the README; redraws both diagrams. Co-Authored-By: Claude <noreply@anthropic.com>
Browser WSS in, Deepgram WSS out, turn loop in the middle. Implements docs/architecture/05-voice-pipeline.md §§2–8 and ADR-007/013/016. The load-bearing pieces: - deepgram/urls.py is the only place a Deepgram URL is constructed, and it welds mip_opt_out=true onto every one. tests/test_url_chokepoint.py walks the AST of every other module and fails on a Deepgram host or path literal anywhere else. Without this a stale client silently enters PHI into a training corpus — the default posture is opt-IN. - The long-lived API key is used for exactly one request: minting 30s tokens at the auth-grant endpoint. It never enters the STT or TTS request path. - turn/detector.py is a direct port of §4: is_final accumulates, speech_final commits, UtteranceEnd is the word-timing safety net, and last_word_end == -1 returns before the one-shot suppression flag resets. - All five §5 failure modes, including KeepAlive as a TEXT frame (binary degrades transcription with no error) and the 1.25x send cap that only bites on reconnect drain. - Pre-rendered approved-question audio. Aura's 45 concurrent streams against STT's 150 makes TTS the binding constraint; serving approved wording from disk takes it off the concurrency path. The manifest records a digest of the rendered text, so wording edited without a version bump is a miss rather than a patient hearing last month's approved script. - No PHI in a traced function's signature: @Traced reads the signature at decoration time and refuses anything off the structural allowlist, so a violation fails at import in CI rather than depending on Langfuse's capture_input=False staying False. No database client and no database URL, by construction (ADR-016). Each turn goes to the TypeScript tier over HTTP; persistence/stub_server.py stands in until slice 1 ships the real endpoint. graph/mock_graph.py is a hardcoded three-question list for slice 3 to replace; the transition is a dictionary lookup with no model in it, and must stay that way. contracts/ is a temporary stand-in — slice/contracts is documentation-only at f0af7ba, so slice 0's Pydantic types do not exist yet. The package docstring names the three touch points to swap. 116 tests pass. Verified live in a browser: multi-turn exchange, a rejected fragment re-asked rather than persisted, a forced connection drop recovered with the question re-asked, five turns at the persistence stub, and spans carrying structure only. Deepgram itself is exercised against fakes — the credentialed path has not been run. Co-Authored-By: Claude <noreply@anthropic.com>
`_handle_turn` read the current question from session state at handling time,
but a turn is handled on a worker task behind a persistence round trip and a
stretch of audio playback. A patient who kept talking through that window
committed a second turn while the session was still on the first, and the
graph had moved on by the time it was handled — so the words were filed under
a question the patient had not yet been asked.
Reproduced against the stub: two utterances spoken while only `knee.onset` was
on the floor were persisted as `knee.onset: answered` and `knee.severity:
answered`. The second is a well-formed severity answer, which is what made the
mis-filing silent — it passes the answer gate and reads as a real reply.
`CommittedTurn` now carries the question id, stamped by the detector at commit
time. A turn whose question the session has already left is recorded as
LATE_UTTERANCE against the question it was actually spoken against: the patient
said it, so it stays in the record, but it does not drive a transition for a
question they have not heard.
Also in the turn loop, both found in the same review:
* `aclose()` tracked only the latest supervisor task, but `_recover()` runs
*inside* a supervisor and spawns its replacement — so closing during a
reconnect left the old one retrying against a channel that was gone. All
session-spawned tasks are now tracked and cancelled together, which also
keeps the fire-and-forget barge-in task from being collected mid-flight.
* The committed-turn queue was unbounded and fed from the socket reader,
which cannot block. Bounded, with shedding logged rather than silent.
Co-Authored-By: Claude <noreply@anthropic.com>
…d to have Three edges, each with a check that read as present and was not. **Anyone could open an intake session.** The WebSocket route called `accept()` unconditionally. `allowed_origins` was wired only into `CORSMiddleware`, which never sees this request — a browser sends no preflight for a WebSocket handshake and applies no same-origin rule to it, so any page on the internet could open a session. The handshake is now rejected before `accept()` for an origin that is not allowed. The Origin header cannot be forged from script, which is exactly the attacker this is for; it is not a substitute for the session auth that arrives with the real deployment. `localhost` joins `127.0.0.1` in the default allowlist. They are different origins to a browser and the demo page is reachable at either — a check that rejects half the URLs a developer types gets widened to `*` and stays there. **The persistence stub accepted any bearer token.** It tested that the header started with `Bearer ` and never compared it to the configured secret, while `persist_turn_token` sitting in config made it read as enforced. Now compared with `compare_digest`. It is a stub, but the real TypeScript route gets written from this file, and a stub that teaches the shape of a check without the substance of it teaches the wrong thing. **The ADR-013 self-hosted deployment was unreachable.** `allow_self_hosted_host` was threaded through every URL builder and passed by no call site, so pointing `deepgram_host` at a private host in ca-central-1 — the deployment the ADR specifies — raised `DeepgramUrlError` from every call. It now defaults from a new `deepgram_allow_self_hosted_host` setting, off by default so a typo in the host still fails loudly rather than quietly sending audio somewhere unintended. A flag only the tests can set is not a flag. The URL choke point itself is unchanged: `mip_opt_out=true` is still welded on after the caller's parameters, on every endpoint, including this path where it is inert. Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces the slice-2 Python/FastAPI “voice gateway” service that terminates a browser WebSocket, proxies to Deepgram (STT/TTS), runs the turn loop + deterministic graph transition in the middle, and POSTs one event per turn to the TypeScript persistence tier, aligning with the referenced architecture docs.
Changes:
- Added FastAPI WSS intake endpoint (
/v1/intake/stream) plus a demo client + AudioWorklet linear16@16k capture path. - Implemented Deepgram integration (ephemeral token minting, URL choke point with
mip_opt_out=true, STT/TTS socket transports, reconnect/failure-mode handling). - Implemented turn detection + answer plausibility gating, pre-rendered audio cache, persistence HTTP client, and extensive unit/integration tests enforcing the safety guarantees.
Reviewed changes
Copilot reviewed 47 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/voice-gateway/tests/test_url_chokepoint.py | AST-based enforcement that Deepgram URLs can’t be constructed outside the builder. |
| services/voice-gateway/tests/test_turn_detection.py | Pins 05 §4 turn-detection edge cases and ordering constraints. |
| services/voice-gateway/tests/test_telemetry_no_phi.py | Enforces no-PHI telemetry contract structurally at decoration/import time. |
| services/voice-gateway/tests/test_intake_session.py | End-to-end (no network) turn loop + graph transition + reconnect + persistence assertions. |
| services/voice-gateway/tests/test_deepgram_urls.py | Validates mip_opt_out welding + Deepgram URL parameter invariants. |
| services/voice-gateway/tests/test_connection_failure_modes.py | Tests 05 §5 socket failure modes, pacing, and close-code classification. |
| services/voice-gateway/tests/test_audio_cache.py | Tests pre-rendered audio cache correctness and traversal-safety properties. |
| services/voice-gateway/tests/conftest.py | Test fixtures (settings, offline render, fake channel, recording persistence). |
| services/voice-gateway/src/voice_gateway/turn/session.py | Core intake turn loop, gating, persistence, TTS cache usage, reconnect handling. |
| services/voice-gateway/src/voice_gateway/turn/detector.py | Turn detector state machine + provenance/timestamp offsets. |
| services/voice-gateway/src/voice_gateway/turn/answer_gate.py | Lightweight answer plausibility gate keyed by expected answer type. |
| services/voice-gateway/src/voice_gateway/turn/init.py | Turn package marker. |
| services/voice-gateway/src/voice_gateway/telemetry.py | Tracing decorator + structural allowlist to prevent PHI in span attrs. |
| services/voice-gateway/src/voice_gateway/static/worklet.js | AudioWorklet resampling to linear16@16k and framing for WS send. |
| services/voice-gateway/src/voice_gateway/static/demo.html | Browser demo client for WS intake, mic capture, and PCM playback. |
| services/voice-gateway/src/voice_gateway/persistence/stub_server.py | Dev-only stub HTTP server for turn persistence route shape. |
| services/voice-gateway/src/voice_gateway/persistence/client.py | HTTP client to POST turn events with retry/backoff and PHI-safe logging. |
| services/voice-gateway/src/voice_gateway/persistence/init.py | Persistence package marker. |
| services/voice-gateway/src/voice_gateway/graph/mock_graph.py | Hardcoded 3-question graph stand-in + fixed system utterances. |
| services/voice-gateway/src/voice_gateway/graph/init.py | Graph package marker. |
| services/voice-gateway/src/voice_gateway/deepgram/urls.py | Single Deepgram URL builder enforcing mip_opt_out=true and parameter discipline. |
| services/voice-gateway/src/voice_gateway/deepgram/transport.py | Frame-type discipline + realtime pacing for Deepgram sockets. |
| services/voice-gateway/src/voice_gateway/deepgram/speak.py | Aura streaming TTS client for cache-miss path (flush/window limits). |
| services/voice-gateway/src/voice_gateway/deepgram/messages.py | Typed parsing for Deepgram streaming message payloads. |
| services/voice-gateway/src/voice_gateway/deepgram/listen.py | Deepgram listen socket wrapper (keepalive, reader loop, close outcomes). |
| services/voice-gateway/src/voice_gateway/deepgram/auth.py | Ephemeral token minting via auth-grant with shallow per-process caching. |
| services/voice-gateway/src/voice_gateway/deepgram/init.py | Deepgram package marker. |
| services/voice-gateway/src/voice_gateway/contracts/models.py | Temporary Pydantic wire types for TS boundary (slice-0 stand-in). |
| services/voice-gateway/src/voice_gateway/contracts/init.py | Re-export stand-in contract types and documents intended deletion. |
| services/voice-gateway/src/voice_gateway/config.py | Pydantic Settings for Deepgram, audio, persistence hop, telemetry posture. |
| services/voice-gateway/src/voice_gateway/audio/prerender.py | Build-time renderer for approved-question audio (online/offline). |
| services/voice-gateway/src/voice_gateway/audio/cache.py | Read-only cache enforcing text-digest/prompt-version correctness. |
| services/voice-gateway/src/voice_gateway/audio/init.py | Audio package marker. |
| services/voice-gateway/src/voice_gateway/app.py | FastAPI app + WS intake endpoint + static demo routes. |
| services/voice-gateway/src/voice_gateway/main.py | CLI entry point for running the gateway under uvicorn. |
| services/voice-gateway/src/voice_gateway/init.py | Package metadata. |
| services/voice-gateway/README.md | Service overview, constraints, run instructions, and layout. |
| services/voice-gateway/pyproject.toml | Python project metadata, deps, scripts, and pytest/ruff config. |
| services/voice-gateway/.gitignore | Service-local ignores (venv, env, audio-cache, caches). |
| services/voice-gateway/.env.example | Documented environment variables and boundary constraints. |
| docs/architecture/11-diagrams.md | Architecture diagrams updated/added for boundaries and pipeline overview. |
| docs/architecture/04-auth-access-control.md | Auth/access-control spec updates (Supabase Auth constraints). |
| docs/architecture/03-data-architecture.md | Data architecture spec updates (RLS boundary, tenancy, residency notes). |
| docs/architecture/02-current-state.md | Current-state findings and rebuild justification updates. |
| docs/architecture/01-product-definition.md | Product definition/spec updates for BC/Canada scope and constraints. |
| docs/architecture/00-README.md | Architecture doc set index and consolidated decisions summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
+123
| resolved_host = host or get_settings().deepgram_host | ||
| if resolved_host not in DEEPGRAM_HOSTS and not allow_self_hosted_host: | ||
| raise DeepgramUrlError( | ||
| f"{resolved_host!r} is not a known Deepgram host. If this is the " | ||
| "self-hosted deployment, pass allow_self_hosted_host=True." | ||
| ) | ||
|
|
Comment on lines
+106
to
+110
| @app.websocket("/v1/intake/stream") | ||
| async def intake_stream(websocket: WebSocket) -> None: | ||
| await websocket.accept() | ||
| settings = app.state.settings | ||
|
|
Comment on lines
+37
to
+45
| while (this.buffer.length >= FRAME_SAMPLES) { | ||
| const frame = this.buffer.splice(0, FRAME_SAMPLES); | ||
| const pcm = new Int16Array(FRAME_SAMPLES); | ||
| for (let i = 0; i < FRAME_SAMPLES; i++) { | ||
| const s = Math.max(-1, Math.min(1, frame[i])); | ||
| pcm[i] = s < 0 ? s * 0x8000 : s * 0x7fff; | ||
| } | ||
| this.port.postMessage(pcm.buffer, [pcm.buffer]); | ||
| } |
|
| Filename | Overview |
|---|---|
| services/voice-gateway/src/voice_gateway/deepgram/speak.py | Live TTS socket client; is_open checks only for a non-None socket reference, failing to detect a closed-but-retained connection after an unexpected disconnect. |
| services/voice-gateway/src/voice_gateway/turn/session.py | Core intake session loop; _reprompts not reset in _recover(), plus previously flagged issues with bare exception handling and outcome overloading. |
| services/voice-gateway/src/voice_gateway/deepgram/urls.py | PHI-critical URL builder; mip_opt_out=true welded on unconditionally, validated by AST-walking test. |
| services/voice-gateway/src/voice_gateway/app.py | FastAPI entry point; origin-check WebSocket gate is well-implemented. simulate_drop reachable in all deployments (previously flagged). |
| services/voice-gateway/src/voice_gateway/deepgram/transport.py | Frame-type enforcement and send pacing; KeepAlive-as-text-frame invariant enforced at transport level. |
| services/voice-gateway/src/voice_gateway/deepgram/listen.py | STT socket with keepalive; _closed_event correctly tracks state, unlike the parallel TTS connection. |
| services/voice-gateway/src/voice_gateway/deepgram/auth.py | Ephemeral token minting with asyncio lock, safety margin, and invalidation on auth failure. |
| services/voice-gateway/src/voice_gateway/turn/detector.py | Turn detection ported verbatim from spec; subtle ordering of committed_recently reset preserved and commented. |
| services/voice-gateway/src/voice_gateway/audio/cache.py | Pre-rendered audio cache; text-digest check prevents stale audio. Sync read_bytes() previously flagged. |
| services/voice-gateway/src/voice_gateway/telemetry.py | Signature-level PHI guard enforced at decoration time via parameter allowlist. |
| services/voice-gateway/src/voice_gateway/persistence/client.py | Turn persistence with retry, 4xx fast-fail, and PHI excluded from drop logs. |
Sequence Diagram
sequenceDiagram
participant B as Browser (AudioWorklet)
participant GW as Voice Gateway (FastAPI)
participant DG_STT as Deepgram STT (listen)
participant DG_TTS as Deepgram TTS (speak)
participant TS as TypeScript Tier (persist)
B->>GW: WSS /v1/intake/stream (origin check)
GW->>GW: EphemeralTokenProvider.get_token()
GW->>DG_STT: "wss connect (Bearer token, mip_opt_out=true)"
GW-->>B: session_started + greeting audio
B-->>GW: binary linear16 audio
loop Turn Loop
DG_STT-->>GW: Results / UtteranceEnd
GW->>GW: TurnDetector commits turn
alt cache hit
GW-->>B: pre-rendered PCM
else cache miss
GW->>DG_TTS: "Speak + Flush (mip_opt_out=true)"
DG_TTS-->>GW: raw linear16
GW-->>B: live PCM
end
GW->>TS: POST TurnEvent
GW-->>B: listening event
end
alt STT drop
DG_STT--xGW: ConnectionClosed
GW->>GW: _recover(): discard partial
GW->>DG_STT: reconnect
GW-->>B: reconnected + re-ask
end
GW-->>B: session_complete
Prompt To Fix All With AI
### Issue 1
services/voice-gateway/src/voice_gateway/deepgram/speak.py:67-69
**`is_open` does not detect a closed-but-non-None socket**
After an unexpected `ConnectionClosed` is raised inside `synthesize()`, the cleanup path (`_speak`'s `finally` → `_turn_worker`'s `except Exception`) never calls `SpeakConnection.close()`, so `self._socket` is never set to `None`. On the next call to `_speak_live`, the guard in `session.py` (`not self._tts.is_open`) evaluates to `False` — the socket looks alive — and no reconnect is attempted. `synthesize()` then calls `_transport.send_control()` on the closed websockets connection, which immediately raises `ConnectionClosed` again. This cycle repeats for every subsequent turn that requires live TTS, permanently breaking the TTS path for the remainder of the session.
The concrete failure scenario is a momentary Aura socket reset occurring mid-synthesis on a cache-miss turn, after which every further cache-miss turn in that session silently fails.
`websockets.ClientConnection` exposes an `.open` property that correctly tracks the `OPEN` / `CLOSED` state — checking `self._socket is not None and self._socket.open` would catch this.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (3): Last reviewed commit: "Merge pull request #24 from obro79/slice..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+348
to
+354
| # question, which is true, instead of a wrong one. | ||
| logger.info("advancing past %s unanswered after %d re-prompts", node.id, MAX_REPROMPTS) | ||
| await self._record_turn( | ||
| node, outcome=TurnOutcome.REPROMPT, answer=None, | ||
| commit_reason=turn.reason.value, tts_cache_hit=self._last_tts_cache_hit, | ||
| ) | ||
| await self._advance(node) |
There was a problem hiding this comment.
TurnOutcome.REPROMPT used for two semantically different events
Both a normal re-prompt ("patient will be asked again") and the exhausted-budget advance ("patient skipped unanswered") record outcome=TurnOutcome.REPROMPT. The TypeScript tier cannot distinguish these from the event alone: it sees N consecutive REPROMPT/answer=None rows for the same question_id, then a row for a new question_id, and must infer that the last REPROMPT was a skip rather than a genuine re-ask. This inference breaks if any row is dropped in transit (dropped_turn_count > 0).
A TurnOutcome.SKIPPED (or similar) would let the persistence tier assert the invariant directly, rather than requiring fragile positional reasoning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/turn/session.py
Line: 348-354
Comment:
**`TurnOutcome.REPROMPT` used for two semantically different events**
Both a normal re-prompt ("patient will be asked again") and the exhausted-budget advance ("patient skipped unanswered") record `outcome=TurnOutcome.REPROMPT`. The TypeScript tier cannot distinguish these from the event alone: it sees N consecutive `REPROMPT`/`answer=None` rows for the same `question_id`, then a row for a new `question_id`, and must infer that the last `REPROMPT` was a skip rather than a genuine re-ask. This inference breaks if any row is dropped in transit (`dropped_turn_count > 0`).
A `TurnOutcome.SKIPPED` (or similar) would let the persistence tier assert the invariant directly, rather than requiring fragile positional reasoning.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+141
to
+142
| def read(self, cached: CachedAudio) -> bytes: | ||
| return cached.path.read_bytes() |
There was a problem hiding this comment.
read_bytes() inside the async turn loop
cache.read(cached) calls cached.path.read_bytes() — a blocking filesystem syscall — and its result is passed directly to await self._stream_pcm(...) in the session. For the 5–15 KB pre-rendered clips this is effectively instant, but it blocks the event loop during the read, preventing keepalive frames and other sessions from progressing for that slice of time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/audio/cache.py
Line: 141-142
Comment:
**Synchronous `read_bytes()` inside the async turn loop**
`cache.read(cached)` calls `cached.path.read_bytes()` — a blocking filesystem syscall — and its result is passed directly to `await self._stream_pcm(...)` in the session. For the 5–15 KB pre-rendered clips this is effectively instant, but it blocks the event loop during the read, preventing keepalive frames and other sessions from progressing for that slice of time.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Bind each turn to the question it was spoken against
Enforce the checks the service edges only appeared to have
Comment on lines
+290
to
+349
| async def _supervise(self, connection: ListenConnection) -> None: | ||
| outcome = await connection.wait_closed() | ||
| if self._stopping.is_set() or self._finished.is_set(): | ||
| return | ||
|
|
||
| if not outcome.is_recoverable: | ||
| # 1008 DATA-0000: undecodable audio, almost always a wrong | ||
| # encoding/sample_rate or a control message sent as a binary frame. | ||
| # Reconnecting sends the same bad audio again. | ||
| logger.error("STT closed unrecoverably (%s); ending session", outcome) | ||
| await self._channel.send_event( | ||
| {"type": "fatal", "reason": outcome.value} | ||
| ) | ||
| self._finished.set() | ||
| return | ||
|
|
||
| if outcome is CloseOutcome.NO_AUDIO_TIMEOUT: | ||
| # KeepAlive resets this one, so seeing it means the keepalive loop | ||
| # stalled rather than that the patient went quiet. | ||
| logger.warning("NET-0002 despite KeepAlive — the keepalive loop may have stalled") | ||
|
|
||
| await self._recover() | ||
|
|
||
| async def _recover(self) -> None: | ||
| # 05 §5: audio during a reconnect is lost unless buffered client-side. | ||
| # Whatever is in the buffer is a fragment with a hole in it, so it is | ||
| # discarded rather than persisted as a partial answer. | ||
| self._detector.discard() | ||
| self._connection_index += 1 | ||
|
|
||
| for attempt in range(1, self._settings.max_reconnect_attempts + 1): | ||
| if self._stopping.is_set(): | ||
| return | ||
| try: | ||
| if self._audio_only_mode: | ||
| await self._connect_stt() | ||
| break | ||
| except (DeepgramAuthError, OSError) as exc: | ||
| logger.warning( | ||
| "reconnect attempt %d/%d failed: %s", | ||
| attempt, self._settings.max_reconnect_attempts, exc, | ||
| ) | ||
| if attempt == self._settings.max_reconnect_attempts: | ||
| await self._channel.send_event({"type": "fatal", "reason": "reconnect_failed"}) | ||
| self._finished.set() | ||
| return | ||
| await asyncio.sleep(self._settings.reconnect_backoff_seconds * attempt) | ||
|
|
||
| await self._channel.send_event( | ||
| {"type": "reconnected", "connection_index": self._connection_index} | ||
| ) | ||
|
|
||
| # Re-ask. Do not guess at a partial answer. | ||
| node = self._current | ||
| if node is None: | ||
| return | ||
| await self._record_turn(node, outcome=TurnOutcome.RECONNECT_REASK, answer=None, | ||
| commit_reason="reconnect", tts_cache_hit=self._last_tts_cache_hit) | ||
| await self._speak(RECONNECT_NOTICE) | ||
| await self._ask(node) |
There was a problem hiding this comment.
BrowserChannel writes between _turn_worker and _supervise/_recover
_turn_worker and the _supervise task both call self._channel.send_event/send_audio with no coordination lock. A connection drop that arrives while the turn worker is inside _speak (awaiting send_audio chunks via live TTS or reconnect notice playback) lets _recover immediately start its own _channel.send_event({"type": "reconnected"…}) → _speak(RECONNECT_NOTICE) sequence on the same WebSocket. The browser can receive a second audio_begin before the first audio_end, which is an invalid event sequence the demo client does not handle. Because _recover re-asks the current question while the turn worker may simultaneously be advancing past it and asking the next question, both paths call _speak on different nodes concurrently, producing interleaved audio frames for two distinct questions.
The fix is either a send lock on _WebSocketChannel or — more structurally — routing all channel output through the turn worker queue so a single task owns the wire.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/turn/session.py
Line: 290-349
Comment:
**Concurrent `BrowserChannel` writes between `_turn_worker` and `_supervise`/`_recover`**
`_turn_worker` and the `_supervise` task both call `self._channel.send_event`/`send_audio` with no coordination lock. A connection drop that arrives while the turn worker is inside `_speak` (awaiting `send_audio` chunks via live TTS or reconnect notice playback) lets `_recover` immediately start its own `_channel.send_event({"type": "reconnected"…})` → `_speak(RECONNECT_NOTICE)` sequence on the same WebSocket. The browser can receive a second `audio_begin` before the first `audio_end`, which is an invalid event sequence the demo client does not handle. Because `_recover` re-asks the current question while the turn worker may simultaneously be advancing past it and asking the *next* question, both paths call `_speak` on different nodes concurrently, producing interleaved audio frames for two distinct questions.
The fix is either a send lock on `_WebSocketChannel` or — more structurally — routing all channel output through the turn worker queue so a single task owns the wire.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+67
to
+69
| @property | ||
| def is_open(self) -> bool: | ||
| return self._socket is not None |
There was a problem hiding this comment.
is_open
SpeakConnection.is_open returns True whenever self._socket is not None, but _socket is only set to None in close(). If Deepgram drops the connection unexpectedly (network blip, 60-minute cap hit slightly early, or any other server-side close), _socket remains non-None and is_open remains True. The next _speak_live call skips the reconnect branch and calls synthesize, which immediately raises ConnectionClosed on the first frame send. That exception propagates to _turn_worker and is swallowed by the bare except Exception, so the patient hears nothing and the question is not re-asked. Unlike the STT path, there is no supervisor task watching for spontaneous TTS close events.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/deepgram/speak.py
Line: 67-69
Comment:
**Spontaneously closed TTS socket is invisible to `is_open`**
`SpeakConnection.is_open` returns `True` whenever `self._socket is not None`, but `_socket` is only set to `None` in `close()`. If Deepgram drops the connection unexpectedly (network blip, 60-minute cap hit slightly early, or any other server-side close), `_socket` remains non-`None` and `is_open` remains `True`. The next `_speak_live` call skips the reconnect branch and calls `synthesize`, which immediately raises `ConnectionClosed` on the first frame send. That exception propagates to `_turn_worker` and is swallowed by the bare `except Exception`, so the patient hears nothing and the question is not re-asked. Unlike the STT path, there is no supervisor task watching for spontaneous TTS close events.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| # --- The TypeScript persistence hop (ADR-016) ------------------------- | ||
| persist_turn_url: str = "http://127.0.0.1:8787/api/internal/intake/turn" | ||
| persist_turn_token: str = "dev-shared-secret" |
There was a problem hiding this comment.
persist_turn_token will be accepted by the stub and any future TypeScript tier if not overridden
"dev-shared-secret" is now public in source code. Any deployment that reaches production without setting VOICE_GATEWAY_PERSIST_TURN_TOKEN will authenticate its turn POSTs with this well-known value, allowing any party that reads this repository to inject or replay fabricated TurnEvent payloads against the TypeScript persistence endpoint. The fix is to default to an empty string (just like deepgram_api_key) so a misconfigured deployment fails loudly rather than silently accepting the known default.
Suggested change
| persist_turn_token: str = "dev-shared-secret" | |
| persist_turn_token: str = "" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/config.py
Line: 85
Comment:
**Hard-coded default `persist_turn_token` will be accepted by the stub and any future TypeScript tier if not overridden**
`"dev-shared-secret"` is now public in source code. Any deployment that reaches production without setting `VOICE_GATEWAY_PERSIST_TURN_TOKEN` will authenticate its turn POSTs with this well-known value, allowing any party that reads this repository to inject or replay fabricated `TurnEvent` payloads against the TypeScript persistence endpoint. The fix is to default to an empty string (just like `deepgram_api_key`) so a misconfigured deployment fails loudly rather than silently accepting the known default.
```suggestion
persist_turn_token: str = ""
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+67
to
+69
| @property | ||
| def is_open(self) -> bool: | ||
| return self._socket is not None |
There was a problem hiding this comment.
is_open does not detect a closed-but-non-None socket
After an unexpected ConnectionClosed is raised inside synthesize(), the cleanup path (_speak's finally → _turn_worker's except Exception) never calls SpeakConnection.close(), so self._socket is never set to None. On the next call to _speak_live, the guard in session.py (not self._tts.is_open) evaluates to False — the socket looks alive — and no reconnect is attempted. synthesize() then calls _transport.send_control() on the closed websockets connection, which immediately raises ConnectionClosed again. This cycle repeats for every subsequent turn that requires live TTS, permanently breaking the TTS path for the remainder of the session.
The concrete failure scenario is a momentary Aura socket reset occurring mid-synthesis on a cache-miss turn, after which every further cache-miss turn in that session silently fails.
websockets.ClientConnection exposes an .open property that correctly tracks the OPEN / CLOSED state — checking self._socket is not None and self._socket.open would catch this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/deepgram/speak.py
Line: 67-69
Comment:
**`is_open` does not detect a closed-but-non-None socket**
After an unexpected `ConnectionClosed` is raised inside `synthesize()`, the cleanup path (`_speak`'s `finally` → `_turn_worker`'s `except Exception`) never calls `SpeakConnection.close()`, so `self._socket` is never set to `None`. On the next call to `_speak_live`, the guard in `session.py` (`not self._tts.is_open`) evaluates to `False` — the socket looks alive — and no reconnect is attempted. `synthesize()` then calls `_transport.send_control()` on the closed websockets connection, which immediately raises `ConnectionClosed` again. This cycle repeats for every subsequent turn that requires live TTS, permanently breaking the TTS path for the remainder of the session.
The concrete failure scenario is a momentary Aura socket reset occurring mid-synthesis on a cache-miss turn, after which every further cache-miss turn in that session silently fails.
`websockets.ClientConnection` exposes an `.open` property that correctly tracks the `OPEN` / `CLOSED` state — checking `self._socket is not None and self._socket.open` would catch this.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
obro79
added a commit
that referenced
this pull request
Aug 3, 2026
Brings in the two review fixes that had not yet landed: frozen wire collections with the extraction attribution hole closed, and the failure-recording and splice holes found in review. Every other slice (schema, intake, eval, voice, voice-boundaries, voice-turn-binding) was already contained in rebuild via #22-#24. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack — merge bottom to top
slice/voice→prod)Browser WebSocket in, Deepgram WebSocket out, turn loop in the middle. Implements
docs/architecture/05-voice-pipeline.md, which §4 asks to be implemented exactly.What's here
/v1/intake/streamaccepting linear16 @ 16 kHz.deepgram/urls.py), always settingmip_opt_out=true.tests/test_url_chokepoint.pywalks the AST of every other module and fails if any of them so much as names a Deepgram host — the flag is only as good as the guarantee that nothing routes around the builder. Deepgram's default posture is opt-in to training on your audio and the opt-out is per-request, so a single request missing the flag is a PHI disclosure./v1/auth/grant, so the long-lived key never enters the outbound STT/TTS request path.last_word_end == -1returns before thecommitted_recentlyreset, and that flag clears at the end of every non--1UtteranceEnd.What's deliberately absent
No database client and no database credentials, per ADR-016 and doc 03 §4 — the gateway is inside the PHI boundary and outside the persistence boundary. No LangChain or LangGraph; the transition is
next_idand the model is not in the decision. Tracing carries structure only —tests/test_telemetry_no_phi.pyfails at decoration time on a traced function whose signature names content.The question graph is a hardcoded three-question knee list behind the slice-0 contract types. Slice 3 replaces it.
Verification
129 tests pass. A live browser exchange covers a multi-turn conversation, a rejected fragment, a forced connection drop with recovery, turns arriving at the persistence stub, and audio through Web Audio.
Not yet verified: every Deepgram interaction is exercised against fakes. Nothing here has run against real Deepgram credentials — no live STT socket, no live Aura render. That is the outstanding half.
Note for review
slice/contractsis documentation-only at time of writing, so slice 0's Pydantic types do not exist yet.contracts/here is a marked stand-in naming the three touch points to swap.Doc 05 §5 contradicts itself on
NET-0001/NET-0002: the failure-mode table attributes the 10s no-data timeout toNET-0001and prescribes KeepAlive, while the close-code list says KeepAlive does not resetNET-0001and does resetNET-0002. Flagged inclassify_close's docstring; the doc was not edited.🤖 Generated with Claude Code