ASR and LLM provider contracts, and a truncation check that was only wired to one provider (#350, #351) #483

Merged

claude-bot merged 4 commits from feat/350-351-provider-contracts into main 2026-09-01 20:29:11 +00:00

Conversation 0 Commits 4 Files changed 40 +2242 -107

claude-bot commented 2026-09-01 20:19:28 +00:00

Contributor

Copy link

Opens v4.2.0. Four commits, reviewable in order — the contracts, the defect they exposed, the operator-facing field, and the ASR wiring.

The defect this turned up

_warn_if_prompt_truncated was called from exactly one site, the Ollama path. llama.cpp, Anthropic and OpenAI all parsed usage for completion tokens and never looked at the prompt side, so a front-truncated prompt on three of four providers produced no signal at all — the input-side half of the failure #331/#336 exist to prevent, live on most deployments.

Usage reading now goes through provider.read_usage, normalising four different shapes (prompt_eval_count, usage.prompt_tokens, usage.input_tokens), and one _check_usage runs both truncation checks so a new transport cannot forget one.

It is skipped rather than fired-and-ignored where a low count is genuinely ambiguous. Anthropic and OpenAI cache prompt prefixes and report only newly evaluated tokens, so "truncated" and "cache hit" are indistinguishable from here. Ollama and llama.cpp neither cache nor reject an over-long prompt, so there a low count has one meaning — and those are the deployments most likely to have the window misconfigured. That declaration is what the abstraction buys; the docstring had been asking for it since #331.

Provider identity stops being a guess

Dispatch was URL-substring matching — anthropic.com, openai.com, :11434/ollama, else llama.cpp — and the four _qa_* functions repeated it independently. A self-hoster behind a gateway on their own domain was read as llama.cpp regardless of what was behind it, and got that provider's schema spelling, truncation semantics and window defaults.

Provider is now configuration, with a validated select in Admin → Bot Settings. The old heuristic survives as a named, logged fallback so nothing changes on upgrade, and the log line names the field to set.

Two structural guarantees worth reviewing as such

TranscriptionResult has no speaker field. Not "the echo is ignored" — there is nowhere to put one. Identity comes from Discord track ownership, so an adapter that wanted to relabel a track has no channel through which to do it. #342 came back through that door once; this closes it in the type rather than by convention. There is a test asserting the field's absence.

Association is by track_id, never list position. Echoed through untouched.

Deliberate scope limits, flagged for review

• Word timestamps are declared, not measured. The bundled server is segment-only and replacing it is #352, out of repo. LocalWhisperProvider declares word_timestamps=False and interpolates spans across each segment by character length, marking every word interpolated=True and the result degraded. Consumers read words either way and none branch on provider; #358 surfaces the gap. Nothing downstream consumes words yet — on this provider they would add no precision, and pushing a new shape through the merge and segment rows is worth doing when #352 makes them real.
• The ASR wiring is one function's internals. Wire format byte-for-byte unchanged, returned dict shape unchanged, so the pipeline v3.11.5/v4.0.0/v4.1.0 stabilised is untouched. The VAD path comes along free because it delegates to transcribe_track.
• vocabulary maps for the local adapter only — #350 asks for one managed adapter too, which wants #360.
• preflight_prompt stays a warning, not a raise. #351 makes a hard check possible (a declared tokenizer, a declared front_truncates), but flipping it should wait for #359 proving the tokenizer.

One thing I want a second opinion on

llm_provider is threaded alongside context_tokens through every generation path — 19 signatures, 25 pass-throughs, 14 origin sites. Partial adoption would be worse than none (a self-hoster is misread on every path, so half-right routing is harder to diagnose than none), and omitting it falls back to the sniff, which is safe.

But this is the exact pattern DEFAULT_CONTEXT_TOKENS' comment warns about — "the eighteenth, written next month, unprotected again". It holds only because the fallback is benign. The real fix is to stop destructuring LLMConfig into four loose parameters at every boundary, which would kill both threading problems permanently. That is a refactor worth its own issue rather than one smuggled into this one — happy to file it.

Verification

• 1698 backend tests pass; 470 frontend tests pass; ruff check and format clean.
• Test doubles were updated to match the real dataclass rather than worked around with getattr — a double that silently diverges is exactly the test that passes while production breaks (#441).
• Mutation-checked that the new ASR path is actually exercised rather than passing vacuously: dropping the segment conversion fails 7 of 54 transcription tests. Only 1 asserts the URL, which is a thin spot in that suite worth knowing about.
• test_llm_transport caught a regression on the first run — a junk context window raising instead of falling back. Exactly what it was written for; fixed in the registry, since raising would trade #337's silent hole for a dead pipeline.

Opens v4.2.0. Four commits, reviewable in order — the contracts, the defect they exposed, the operator-facing field, and the ASR wiring. ## The defect this turned up `_warn_if_prompt_truncated` was called from **exactly one site**, the Ollama path. llama.cpp, Anthropic and OpenAI all parsed `usage` for *completion* tokens and never looked at the prompt side, so a front-truncated prompt on three of four providers produced no signal at all — the input-side half of the failure #331/#336 exist to prevent, live on most deployments. Usage reading now goes through `provider.read_usage`, normalising four different shapes (`prompt_eval_count`, `usage.prompt_tokens`, `usage.input_tokens`), and one `_check_usage` runs both truncation checks so a new transport cannot forget one. It is **skipped** rather than fired-and-ignored where a low count is genuinely ambiguous. Anthropic and OpenAI cache prompt prefixes and report only newly evaluated tokens, so "truncated" and "cache hit" are indistinguishable from here. Ollama and llama.cpp neither cache nor reject an over-long prompt, so there a low count has one meaning — and those are the deployments most likely to have the window misconfigured. That declaration is what the abstraction buys; the docstring had been asking for it since #331. ## Provider identity stops being a guess Dispatch was URL-substring matching — `anthropic.com`, `openai.com`, `:11434`/`ollama`, else llama.cpp — and the four `_qa_*` functions repeated it independently. A self-hoster behind a gateway on their own domain was read as llama.cpp regardless of what was behind it, and got that provider's schema spelling, truncation semantics and window defaults. Provider is now configuration, with a validated select in Admin → Bot Settings. The old heuristic survives as a named, logged fallback so **nothing changes on upgrade**, and the log line names the field to set. ## Two structural guarantees worth reviewing as such **`TranscriptionResult` has no speaker field.** Not "the echo is ignored" — there is nowhere to put one. Identity comes from Discord track ownership, so an adapter that wanted to relabel a track has no channel through which to do it. #342 came back through that door once; this closes it in the type rather than by convention. There is a test asserting the field's *absence*. **Association is by `track_id`, never list position.** Echoed through untouched. ## Deliberate scope limits, flagged for review - **Word timestamps are declared, not measured.** The bundled server is segment-only and replacing it is #352, out of repo. `LocalWhisperProvider` declares `word_timestamps=False` and interpolates spans across each segment by character length, marking every word `interpolated=True` and the result degraded. Consumers read words either way and none branch on provider; #358 surfaces the gap. Nothing downstream consumes words yet — on this provider they would add no precision, and pushing a new shape through the merge and segment rows is worth doing when #352 makes them real. - **The ASR wiring is one function's internals.** Wire format byte-for-byte unchanged, returned dict shape unchanged, so the pipeline v3.11.5/v4.0.0/v4.1.0 stabilised is untouched. The VAD path comes along free because it delegates to `transcribe_track`. - **`vocabulary` maps for the local adapter only** — #350 asks for one managed adapter too, which wants #360. - **`preflight_prompt` stays a warning, not a raise.** #351 makes a hard check *possible* (a declared tokenizer, a declared `front_truncates`), but flipping it should wait for #359 proving the tokenizer. ## One thing I want a second opinion on `llm_provider` is threaded alongside `context_tokens` through every generation path — 19 signatures, 25 pass-throughs, 14 origin sites. Partial adoption would be worse than none (a self-hoster is misread on *every* path, so half-right routing is harder to diagnose than none), and omitting it falls back to the sniff, which is safe. But this is the exact pattern `DEFAULT_CONTEXT_TOKENS`' comment warns about — "the eighteenth, written next month, unprotected again". It holds only because the fallback is benign. **The real fix is to stop destructuring `LLMConfig` into four loose parameters at every boundary**, which would kill both threading problems permanently. That is a refactor worth its own issue rather than one smuggled into this one — happy to file it. ## Verification - 1698 backend tests pass; 470 frontend tests pass; ruff check and format clean. - Test doubles were updated to match the real dataclass rather than worked around with `getattr` — a double that silently diverges is exactly the test that passes while production breaks (#441). - Mutation-checked that the new ASR path is actually exercised rather than passing vacuously: dropping the segment conversion fails 7 of 54 transcription tests. Only 1 asserts the URL, which is a thin spot in that suite worth knowing about. - `test_llm_transport` caught a regression on the first run — a junk context window raising instead of falling back. Exactly what it was written for; fixed in the registry, since raising would trade #337's silent hole for a dead pipeline.

claude-bot added 4 commits 2026-09-01 20:19:28 +00:00

feat(providers): define the ASR and LLM provider contracts (#350, #351) ... fda22738c9

The foundation of v4.2.0: a normalised contract with declared capabilities, so
a provider can be swapped without a rewrite and self-hosted stays a first-class
target rather than a fallback.

Nothing consumes this yet — the call-site migration follows in this branch.

ASR (#350). One job per track with caller-supplied track_id echoed back, so
association is never by list position. TranscriptionResult carries no speaker
field at all: identity comes from Discord track ownership, and an adapter that
wanted to relabel a track has no channel through which to do it. That makes
"no adapter can influence speaker attribution" a property of the types rather
than a rule someone has to remember — #342 came back through exactly that door
once already.

Words are the normalisation target. The bundled server is segment-only and
replacing it is #352, out of repo, so LocalWhisperProvider declares
word_timestamps=False and interpolates word spans across each segment by
character length, marking every word interpolated and the result degraded.
Consumers read words either way and none branch on provider; #358 renders the
gap. When #352 lands the flag flips and the interpolation stops being reached.

Chunk-and-remap deliberately stays in caller code, where compute_speech_spans
and _transcribe_span_resiliently already do it. An adapter chunking internally
would produce timestamps in its own private frame, which is #320 again.

LLM (#351). Much of this already existed unowned — context_tokens,
resolve_window, prompt_budget_tokens, and the four-provider _apply_json_schema
— so the substance is the declaration layer they hang off, plus replacing the
thing that was actually broken: provider identity was guessed from URL
substrings, so a self-hoster behind a proxy on their own domain was classified
as llama.cpp regardless of what was behind it. Provider is now configuration;
the old heuristic survives as a named, logged fallback so no existing
deployment breaks on upgrade, and the log line names the field to set.

read_usage normalises four different usage shapes. Today
_warn_if_prompt_truncated is called from exactly one site, the Ollama path, so
a front-truncated prompt on llama.cpp, Anthropic and OpenAI passes with no
signal at all. Normalising the read is what lets one check cover all four, and
it is the raw material for #357's cost telemetry.

Schema enforcement is three-state, not boolean: json_object is advisory and
demonstrably unenforced on the current deployment, and a provider that cannot
enforce a schema has to say so rather than be silently trusted.

app.providers imports nothing from app.services, so a provider builds in a test
with no database, settings or event loop.

Tests pin the four schema spellings to the existing _apply_json_schema, which
was learned the hard way against real endpoints (#281) — simplifying one of
them now fails here rather than at a self-hoster.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(llm): check prompt consumption on every provider, not just Ollama (#351) ... b8497c1aab

Wires the transports to the provider contract, and closes the gap that found
along the way.

`_warn_if_prompt_truncated` was called from exactly one site — the Ollama path.
llama.cpp, Anthropic and OpenAI all parsed `usage` for completion tokens and
never looked at the prompt side, so a front-truncated prompt on three of four
providers produced no signal at all. That is the input-side half of the failure
#331/#336 exist to prevent, live on the majority of deployments.

Usage reading now goes through `provider.read_usage`, which normalises four
different shapes (`prompt_eval_count`, `usage.prompt_tokens`,
`usage.input_tokens`), and `_check_usage` runs both truncation checks in one
place so a new transport cannot forget one.

The check is skipped where a low count is genuinely ambiguous rather than fired
and ignored. Anthropic and OpenAI both cache prompt prefixes and report only
newly evaluated tokens, so "truncated" and "cache hit" are indistinguishable
from here; Ollama and llama.cpp neither cache nor reject an over-long prompt,
so on those a low count has one meaning — and those are the deployments where
the window is most likely misconfigured in the first place. That declaration is
what the abstraction buys: the docstring had been asking for it since #331.

Provider selection moves from URL-substring sniffing to the registry. A
configured name wins; an unset one still sniffs, logging what it guessed and
naming the field to set, so no existing deployment changes behaviour on
upgrade.

One deliberate behaviour change: when a caller declares no window, the
provider's own declared window now applies instead of a universal 32k. For
Ollama and llama.cpp that is the same 32k, so local paths are unchanged; for
Anthropic and OpenAI it is the model's documented window, which is more
accurate, not more optimistic. An explicit context_tokens still wins over both.

A junk window (0, -1, None, "", "not-a-number") falls back rather than raising
— test_llm_transport caught that on the first run, which is exactly what it was
written for. Raising would have traded #337's silent hole for a dead pipeline.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

feat(settings): let an admin name the AI provider instead of guessing it (#351) ... da4d2ef266

Completes the operator-facing half of the provider contract. Detection matches
on the endpoint URL — anthropic.com, openai.com, :11434, else llama.cpp — which
is right for the three obvious hosted URLs and wrong for everything else. A
self-hoster behind a gateway or reverse proxy on their own domain was read as
llama.cpp regardless of what was behind it, and got that provider's schema
spelling, truncation semantics and window defaults.

Adds `provider` to WhisperConfig and LLMConfig, a validated select in
Admin → Bot Settings, and the plumbing to carry the choice to the transport.
Empty stays the default, so nothing changes on upgrade; the registry logs
whenever it has to guess, naming the field to set.

Two shapes worth recording, because both were decisions rather than defaults:

Blank means "preserve" everywhere on this endpoint, so an explicit "auto"
sentinel clears a pinned provider. Without it a selection could be set but
never undone, and saving any unrelated field would silently re-pin.

An unknown name is rejected by the form rather than stored. resolve_llm_provider
raises on it at generation time, so a typo would otherwise surface hours later
as every summary failing with no obvious cause. The 400 names the way out, not
just the mistake. The choice list comes from the registry rather than being
hardcoded in the front end, so adding an adapter does not need a matching UI
change.

`llm_provider` is threaded alongside `context_tokens` through every generation
path — 19 signatures, 25 pass-throughs, 14 origin sites — rather than only the
convenient ones. Partial adoption would be worse than none here: a self-hoster
is misread on *every* path, so a deployment where some generation routes
correctly and some does not is harder to diagnose than one where none does.
Omitting it falls back to the sniff, which is today's behaviour and safe.

That threading is the pattern DEFAULT_CONTEXT_TOKENS' comment warns about — the
eighteenth call site, written next month, quietly unprotected. It holds here
only because the fallback is benign. The real fix is to stop destructuring
LLMConfig into four loose parameters at every boundary, which is a refactor
worth its own issue rather than one smuggled into this one.

Test doubles were updated rather than worked around with getattr: a double that
silently diverges from the real dataclass is exactly the kind of test that
passes while production breaks (#441).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

refactor(asr): route transcription through the provider contract (#350) ... 5ec344564a

Makes the ASR abstraction load-bearing rather than dead code. transcribe_track
now resolves an adapter from the registry and submits through it, so adding a
managed provider is a registry entry instead of a branch in this function.

The blast radius is deliberately one function's internals. The wire format is
byte-for-byte what it was — the bundled adapter sends the same multipart
POST /transcribe with the same fields — and the returned dict shape is
unchanged, so transcribe_session, transcribe_track_vad,
_transcribe_span_resiliently, merge_attributed_transcript and the segment rows
are all untouched. The VAD path comes along for free because it delegates here.

Words are on the result and deliberately not consumed yet. On this provider
they are interpolated from segments rather than measured, so nothing downstream
would gain precision by reading them; pushing a new shape through the merge and
the segment rows is worth doing when #352 makes the words real, and not before.

Speaker identity is stamped from the Track we submitted, because it cannot come
from anywhere else — TranscriptionResult has no speaker field at all. That is
the structural version of the rule #342 had to enforce by convention.

Verified the tests actually reach the new path rather than passing vacuously
(#441's concern): dropping the segment conversion fails 7 of the 54
transcription tests, and pointing the adapter at a wrong path fails 1. Worth
noting only one asserts the URL — the contract tests cover the adapter directly,
but that is a thin spot in the pipeline suite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot referenced this pull request 2026-09-01 20:20:51 +00:00

[Backend] Pass LLMConfig through the generation paths instead of destructuring it into loose parameters #484

claude-bot merged commit 12daccb8e4 into main 2026-09-01 20:29:11 +00:00

claude-bot deleted branch feat/350-351-provider-contracts 2026-09-01 20:29:11 +00:00

claude-bot referenced this pull request from a commit 2026-09-01 20:29:13 +00:00

Merge PR #483: ASR and LLM provider contracts (#350, #351)

claude-bot referenced this pull request 2026-09-01 20:29:37 +00:00

[Backend] Define the ASR provider contract and normalise on word timestamps #350

claude-bot referenced this pull request 2026-09-01 20:29:58 +00:00

[Backend] Define the LLM provider contract with declared context window and schema output #351

claude-bot referenced this pull request 2026-09-05 00:11:57 +00:00

chore(release): v4.1.1 #487

claude-bot referenced this pull request 2026-09-05 03:48:16 +00:00

[Backend] Define the ASR provider contract and normalise on word timestamps #350

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backend

Issue related to the backend

  

bot

Issue related to the bot

  

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

frontend

Issue related to the frontend

  

help wanted

Need some help

  

invalid

Something is wrong

  

question

More information is needed

  

recording

Issue related to the recording system

  

security

Security hardening / vulnerability work

  

wontfix

This won't be fixed

No labels

backend

bot

bug

duplicate

enhancement

frontend

help wanted

invalid

question

recording

security

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rbrooks/Quest-Board!483

No description provided.