feat(turns): let external-turn STT decide the turn start, retire provisional_vad - #767
Conversation
…isional_vad Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Qyj7V1wB6QVFRWjihZB8M
…table FastAPI derives a route's operation id once from list(route.methods)[0] and reuses it for every method on the route, so one route serving GET and POST emitted two operations sharing an id — and which verb named it flipped with the interpreter's hash seed, churning the generated clients between runs. Also bumps pipecat to drop the fork-only provisional VAD strategy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Qyj7V1wB6QVFRWjihZB8M
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
2 issues found across 17 files
Not reviewed (too large): ui/src/client/index.ts (~4 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/alembic/versions/c4e21b7f80a9_retire_provisional_vad_turn_start.py">
<violation number="1" location="api/alembic/versions/c4e21b7f80a9_retire_provisional_vad_turn_start.py:61">
P3: The migration calls `fetchall()` on both target tables and then issues a separate `UPDATE` per matched row, materializing every matching `workflow_configurations` payload in memory. With the versioned `workflow_definitions` table this can be thousands of payloads/statements during a locked migration window. Batch the rewrite into a single statement per table using a CASE/JSON rewrite lambda or `jsonb_set`-style expression (the columns are `json`, so a `->>`+reconstruct in one UPDATE is possible for these flat keys) instead of the fetchall + per-row UPDATE loop.</violation>
</file>
<file name="api/services/pipecat/run_pipeline.py">
<violation number="1" location="api/services/pipecat/run_pipeline.py:209">
P2: For external-turn STTs, an explicitly configured `turn_start_strategy="min_words"` (and `turn_start_min_words`) is now silently discarded by the new early return, yet the API schema and workflow-config UI still accept and persist it. Users who configured word-gated turns for an external-turn provider see the setting simply stop working, with only an info log exposing the override. Log a warning (or otherwise surface) when an external-turn STT overrides a requested `min_words` strategy so the no-op config isn't discovered only at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # Local VAD is deliberately kept out of these start strategies too: it would | ||
| # win the race on raw voice activity and start the turn before the STT | ||
| # confirms a real turn. | ||
| if uses_external_turns: |
There was a problem hiding this comment.
P2: For external-turn STTs, an explicitly configured turn_start_strategy="min_words" (and turn_start_min_words) is now silently discarded by the new early return, yet the API schema and workflow-config UI still accept and persist it. Users who configured word-gated turns for an external-turn provider see the setting simply stop working, with only an info log exposing the override. Log a warning (or otherwise surface) when an external-turn STT overrides a requested min_words strategy so the no-op config isn't discovered only at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/services/pipecat/run_pipeline.py, line 209:
<comment>For external-turn STTs, an explicitly configured `turn_start_strategy="min_words"` (and `turn_start_min_words`) is now silently discarded by the new early return, yet the API schema and workflow-config UI still accept and persist it. Users who configured word-gated turns for an external-turn provider see the setting simply stop working, with only an info log exposing the override. Log a warning (or otherwise surface) when an external-turn STT overrides a requested `min_words` strategy so the no-op config isn't discovered only at runtime.</comment>
<file context>
@@ -197,22 +195,20 @@ def _resolve_turn_start_min_words(run_configs: dict) -> int:
+ # Local VAD is deliberately kept out of these start strategies too: it would
+ # win the race on raw voice activity and start the turn before the STT
+ # confirms a real turn.
+ if uses_external_turns:
+ return [ExternalUserTurnStartStrategy(enable_interruptions=True)]
+
</file context>
| for table, column in _TARGETS: | ||
| # These are `json`, not `jsonb`, so key containment (`?`) is unavailable | ||
| # and a ::jsonb cast is avoided; `->>` works on both. | ||
| rows = conn.execute( |
There was a problem hiding this comment.
P3: The migration calls fetchall() on both target tables and then issues a separate UPDATE per matched row, materializing every matching workflow_configurations payload in memory. With the versioned workflow_definitions table this can be thousands of payloads/statements during a locked migration window. Batch the rewrite into a single statement per table using a CASE/JSON rewrite lambda or jsonb_set-style expression (the columns are json, so a ->>+reconstruct in one UPDATE is possible for these flat keys) instead of the fetchall + per-row UPDATE loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/alembic/versions/c4e21b7f80a9_retire_provisional_vad_turn_start.py, line 61:
<comment>The migration calls `fetchall()` on both target tables and then issues a separate `UPDATE` per matched row, materializing every matching `workflow_configurations` payload in memory. With the versioned `workflow_definitions` table this can be thousands of payloads/statements during a locked migration window. Batch the rewrite into a single statement per table using a CASE/JSON rewrite lambda or `jsonb_set`-style expression (the columns are `json`, so a `->>`+reconstruct in one UPDATE is possible for these flat keys) instead of the fetchall + per-row UPDATE loop.</comment>
<file context>
@@ -0,0 +1,85 @@
+ for table, column in _TARGETS:
+ # These are `json`, not `jsonb`, so key containment (`?`) is unavailable
+ # and a ::jsonb cast is avoided; `->>` works on both.
+ rows = conn.execute(
+ sa.text(
+ f"SELECT id, {column} FROM {table} "
</file context>
| function coerceTurnStartStrategy(value: string): TurnStartStrategy { | ||
| return TURN_START_STRATEGY_OPTIONS.some(o => o.value === value) | ||
| ? (value as TurnStartStrategy) | ||
| : 'default'; | ||
| } |
There was a problem hiding this comment.
If a cached UI receives a turn-start strategy added by a newer backend, this helper replaces it with default rather than preserving it. Saving an unrelated setting then sends default in the full configuration payload and silently discards the newer strategy. This is non-blocking, but it can unexpectedly change workflow behavior; convert only the retired provisional_vad value.
| function coerceTurnStartStrategy(value: string): TurnStartStrategy { | |
| return TURN_START_STRATEGY_OPTIONS.some(o => o.value === value) | |
| ? (value as TurnStartStrategy) | |
| : 'default'; | |
| } | |
| function coerceTurnStartStrategy(value: string): TurnStartStrategy { | |
| return value === 'provisional_vad' | |
| ? 'default' | |
| : (value as TurnStartStrategy); | |
| } |
Artifacts
▶ Future strategy preserved before coercion
- This recording shows the comparison case preserving the future backend strategy in the saved value.
Future strategy preserved before coercion
- This image captures the comparison output where the future backend strategy remains unchanged.
▶ Unknown strategy replaced on save
- This recording shows an unrelated settings edit saving `default` instead of the future backend strategy.
Unknown strategy replaced on save
- This image captures the executed result where the save payload replaces the future strategy with `default`.
- This output records the comparison case retaining `future_backend_strategy` through resolution and save.
Configuration dialog execution output
- This output records the focused dialog test resolving and saving `default` after an unrelated edit.
- This is the source for the comparison execution that preserves the future backend strategy.
Configuration dialog test source
- This is the focused test source that renders the dialog, changes one unrelated value, and checks the saved payload.
- This is the source used to capture the rendered execution evidence.
- This output confirms that the rendered evidence capture completed successfully.
`->>` renders a JSON null as SQL NULL, so `provisional_vad_pause_secs: null` read as an absent key and the row was skipped. Stored configs do carry explicit nulls for keys the user never configured, and 38 prod rows are in that shape. The key check now uses `->`, which is SQL NULL only when the key is absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Qyj7V1wB6QVFRWjihZB8M
Follow-up to the production incident where user turns on
provisional_vad+ Deepgram Flux never closed — runs 696485 and 696493 sat silent for 47s and 29s. The frame-level fix is already in production (aea310947). This removes the configuration that exposed it and closes the remaining paths.1. External-turn STT decides the turn start
_create_non_realtime_user_turn_start_strategiesnow checksuses_external_turnsbefore any transcript-gated strategy.An STT that reports its own turn boundaries should own the turn start. Gating it on transcript text instead means ignoring the provider's turn detection on the start side while still relying on it to end the turn — and it resolves the start from a queued frame, which is the shape that lets a turn's own interruption flush the stop proposal queued behind it.
Measured on Flux runs over 14 days,
min_wordswas doing nothing anyway: 89% of its decisions hadbot_speaking=False, where the threshold collapses to 1 word (min_words = self._min_words if self._bot_speaking else 1). It suppressed an interruption twice in 14 days. Three orgs haveturn_start_min_words = 1configured, making it a literal no-op.~194 answered telephony calls/30d across 4 orgs were in this state and get real turn detection back.
The log line now reports what was actually built, since
requested=alone is misleading:2.
provisional_vadretiredRemoved from the runtime, schema, both UI surfaces, and all generated artifacts. Also dropped from the pipecat fork — it was fork-only (PR dograh-hq/pipecat#47), and it dragged
BotOutputAudioPause/ResumeFrameplus ~72 lines ofbase_output.pytransport support with it, none of which had any other consumer.Fork divergence from upstream on the affected files: +819/-16 → +223/-13, with
base_output.pydown to +51/-4. That file conflicts on every upstream merge, so this is the part that matters.Submodule bumped to
dograh-hq/pipecat@b5d50eae5.Back-compat
workflow_definitionsrows are immutable versions and a run executes against the definition it points at, so one can outlive the migration (fresh restore, replica lagging a deploy). Three independent layers, so a missed row degrades rather than breaks:default— already true, now covered by testsprovisional_vad→defaultin afield_validator; an unknown value still raises, so this is not a blanket fallbackresolveWorkflowConfigurationsmaps it todefault, so the select never receives a value it has no option forMigration
c4e21b7f80a9rewritesworkflowsandworkflow_definitions, leaving run history alone. Note these arejson, notjsonb, so the?containment operator is unavailable and a::jsonbcast is avoided — the predicate uses->>.downgrade()is intentionally a no-op: restoring the value would leave rows the code cannot honour.3.
/inbound/rungets a route per methodUnrelated but surfaced by the regeneration. FastAPI derives a route's operation id once from
list(route.methods)[0]and reuses it for every method on the route, so one route serving GET and POST emitted two operations sharing one id. Which verb named it flipped with the interpreter's hash seed:That churned the generated clients between runs — the two artifacts in this branch initially disagreed with each other. Split into stacked
@router.get+@router.postover one handler: distinct ids, no duplicate warning, stable across 5 randomly-seeded runs. Providers genuinely differ on the verb (Exotel's inbound webhook is a GET, everyone else POSTs), so both stay.Why the frame patch stays
stt_uses_external_turns()covers 3 of the 10 services that emitProposedUserStoppedSpeakingFrame. Sarvam (51 orgs, 87 telephony/30d) and OpenAI STT (37 orgs, 56/30d) emit stop proposals while it returnsFalse, so they still get transcript-gated starts alongside a queued stop proposal — and their stop signal arrives on a separate server event, making the interleaving a race rather than an impossibility. The wiring gate and the frame patch cover disjoint sets.Testing
tsc --noEmit, eslintSame failure and error counts on both pipecat sides; the 10-test pass delta is exactly the 10 tests removed (7 strategy + 3 transport). The 16 errors are missing optional deps (
aws_sdk_sagemaker_runtime_http2), pre-existing.The
ProposedUserStoppedSpeakingFrameregression test was rebased off the deleted strategy ontoTranscriptionUserTurnStartStrategyand still discriminates — fails without theUninterruptibleFramemixin, passes with it. It now uses only upstream classes, so it is offerable to pipecat-ai/pipecat#5702, whose current fix coversTranscriptionFramebut not the stop proposal.Migration validated on the test DB: applied, downgrade/upgrade round-trip, and the predicate + rewrite exercised against a real
jsoncolumn across six config shapes.Deploy note
This carries a migration, so unlike a plain image bump
plan.shwill gate onapi/alembic/versions/— confirm a recent RDS snapshot before releasing.🤖 Generated with Claude Code
https://claude.ai/code/session_014Qyj7V1wB6QVFRWjihZB8M
Summary by cubic
Follow-up to the incident where calls sat silent for up to 47s: external-turn STTs now decide turn start, and the
provisional_vadstrategy that exposed the bug is retired. Also splits/inbound/runinto per-method routes so generated clients stop churning between runs, and bumps thepipecatsubmodule to drop the fork-only strategy.Bug Fixes
min_wordssuppressed interruptions only twice in 14 days and is a near no-op for external-turn STTs, so the behavior change is low-risk./inbound/runuses stacked GET and POST routes with distinct operation ids instead of two operations sharing one.Migration
provisional_vadandprovisional_vad_pause_secsare removed from runtime, schema, UI, and generated SDKs.defaultat runtime, API validation, and UI layers; the predicate uses->so explicit JSON nulls (38 prod rows) are matched instead of read as absent keys.default, so saving a workflow can silently drop a newer backend-only setting.workflowsandworkflow_definitions, leaving run history alone; downgrade is a no-op.Written for commit 0653c63. Summary will update on new commits.
Merge-safe: the remaining issue is non-blocking.
Findings
Summary
provisional_vad, uses external STT turn signals when available, updates the API and generated clients, and separates inbound webhook GET and POST operations. The change is merge-safe; one previously reported compatibility concern remains non-blocking.Reviews (3) · Last reviewed commit: "chore: update pipecat submodule"