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

claude-bot commented

2026-09-01 20:20:51 +00:00

Collaborator

Found while implementing #351, and deliberately not fixed there — see the discussion on PR #483.

The pattern

Every generation path takes the LLM target as four separate parameters —
endpoint_url, api_key, model, context_tokens — destructured from
LLMConfig at the boundary and re-threaded by hand through every helper. As of
#351 it is five, because llm_provider had to travel the same way.

Concretely, adding one field to LLMConfig cost:

19 signatures gaining a parameter
25 pass-through call sites
14 origin sites (llm_provider=llm_cfg.provider)
~20 test doubles updated to match

That is ~58 edits and four rounds of test-double breakage for a field that
already existed on an object every one of those call sites started from.

Why it matters beyond the churn

The repo already knows this does not hold. From DEFAULT_CONTEXT_TOKENS
(llm_service.py:79-82):

Of the eighteen generate_structured_text call sites, one passed a window.
Threading it through the other seventeen fixes those seventeen and leaves the
eighteenth, written next month, unprotected again, with no test able to
notice.

That comment was written about context_tokens and it is now equally true of
llm_provider. The mitigation both times was to make the absence benign — a
conservative default window, a fallback to URL sniffing — which works, but it
means the safety comes from the default rather than from the structure, and the
next field may not have a benign default.

context_tokens is the cautionary case: its silent-absence bug (#337) shipped
because seventeen call sites did not opt in, and the fix was to make the
parameter impossible to omit meaningfully, not impossible to omit.

Proposed fix

Pass the config object (or a small frozen LlmTarget) instead of its fields.
A new call site then cannot forget a field of an object it is already holding,
and adding a sixth attribute is one edit rather than fifty-eight.

Rough shape:

LlmTarget(endpoint_url, api_key, model, context_tokens, provider) — or just
reuse LLMConfig, which is already exactly this.
generate_structured_text(prompt, target, *, system_prompt, ...), keeping the
loose-parameter form as a thin deprecated shim during migration so the change
can land in reviewable pieces rather than one 6-file commit.
Domain functions in audio_service, beat_service, lore_service,
generation_service, reminder_tasks and planning_tasks take target and
pass it on unchanged.

Scope notes

This is a refactor with no behaviour change, which is what makes it safe
to do and easy to verify: the existing suite should pass untouched apart from
the doubles.
Test doubles should become real LLMConfig instances rather than
SimpleNamespace, which is what let them drift from the dataclass in the
first place (#441's concern — a double that diverges is a test that passes
while production breaks).
The four _qa_* functions in audio_service build their own request bodies
and bypass the transport entirely (#426). They should be folded in at the same
time, or the count goes back up.

Milestone

Filed under v4.6.0 with the rest of the simplification work (#443), but it
gets cheaper the sooner it happens — v4.2.0 and v4.3.0 will both add
generation call sites, and each one is another hand-threaded copy. Worth pulling
forward if there is an appetite for it before then.

Found while implementing #351, and deliberately **not** fixed there — see the discussion on PR #483. ## The pattern Every generation path takes the LLM target as four separate parameters — `endpoint_url`, `api_key`, `model`, `context_tokens` — destructured from `LLMConfig` at the boundary and re-threaded by hand through every helper. As of #351 it is five, because `llm_provider` had to travel the same way. Concretely, adding one field to `LLMConfig` cost: - **19** signatures gaining a parameter - **25** pass-through call sites - **14** origin sites (`llm_provider=llm_cfg.provider`) - **~20** test doubles updated to match That is ~58 edits and four rounds of test-double breakage for a field that already existed on an object every one of those call sites started from. ## Why it matters beyond the churn The repo already knows this does not hold. From `DEFAULT_CONTEXT_TOKENS` ([llm_service.py:79-82](webapp/backend/app/services/llm_service.py#L79-L82)): > Of the eighteen `generate_structured_text` call sites, one passed a window. > Threading it through the other seventeen fixes those seventeen and leaves the > eighteenth, written next month, unprotected again, with no test able to > notice. That comment was written about `context_tokens` and it is now equally true of `llm_provider`. The mitigation both times was to make the *absence* benign — a conservative default window, a fallback to URL sniffing — which works, but it means the safety comes from the default rather than from the structure, and the next field may not have a benign default. `context_tokens` is the cautionary case: its silent-absence bug (#337) shipped because seventeen call sites did not opt in, and the fix was to make the parameter impossible to omit *meaningfully*, not impossible to omit. ## Proposed fix Pass the config object (or a small frozen `LlmTarget`) instead of its fields. A new call site then cannot forget a field of an object it is already holding, and adding a sixth attribute is one edit rather than fifty-eight. Rough shape: - `LlmTarget(endpoint_url, api_key, model, context_tokens, provider)` — or just reuse `LLMConfig`, which is already exactly this. - `generate_structured_text(prompt, target, *, system_prompt, ...)`, keeping the loose-parameter form as a thin deprecated shim during migration so the change can land in reviewable pieces rather than one 6-file commit. - Domain functions in `audio_service`, `beat_service`, `lore_service`, `generation_service`, `reminder_tasks` and `planning_tasks` take `target` and pass it on unchanged. ## Scope notes - **This is a refactor with no behaviour change**, which is what makes it safe to do and easy to verify: the existing suite should pass untouched apart from the doubles. - Test doubles should become real `LLMConfig` instances rather than `SimpleNamespace`, which is what let them drift from the dataclass in the first place (#441's concern — a double that diverges is a test that passes while production breaks). - The four `_qa_*` functions in `audio_service` build their own request bodies and bypass the transport entirely (#426). They should be folded in at the same time, or the count goes back up. ## Milestone Filed under v4.6.0 with the rest of the simplification work (#443), but **it gets cheaper the sooner it happens** — v4.2.0 and v4.3.0 will both add generation call sites, and each one is another hand-threaded copy. Worth pulling forward if there is an appetite for it before then.

claude-bot added this to the v4.7.0 - Browse the Rules milestone

2026-09-01 20:21:00 +00:00

claude-bot referenced this issue 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 issue

2026-09-01 20:29:59 +00:00

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

rbrooks referenced this issue from a commit

2026-09-01 21:00:26 +00:00

fix(ask): stop presenting a truncated /ask answer as a complete one (#426)

claude-bot referenced this issue

2026-09-01 21:00:44 +00:00

Stop presenting a truncated /ask answer as a complete one (#426) #486

claude-bot referenced this issue

2026-09-01 21:08:17 +00:00

[Backend] /ask is unprotected on every provider — no context budget, no num_ctx, no truncation check #426

claude-bot modified the milestone from v4.7.0 - Browse the Rules to v4.2.0 - AI Provider Abstraction & BYO-AI

2026-09-05 00:14:02 +00:00

claude-bot commented

2026-09-05 00:14:09 +00:00

Author

Collaborator

Picking this up now, ahead of the rest of v4.2.0, and moving it from v4.6.0 to v4.2.0. Reasoning: #356, #357 and #358 each need the provider object at the generation call sites, so every one of them done before this adds another hand-threaded parameter through the same signatures. Doing this first makes each of those a one-object change.

Measured on `main` at `233dc71` (post-#483)

	count
signatures carrying `endpoint_url: str` across `audio_service`, `beat_service`, `generation_service`, `lore_service`, `reminder_tasks`, `planning_tasks`	34
`generate_structured_text` call sites	20
`llm_provider=` occurrences	49
hand-rolled transports that bypass `generate_structured_text`	8 — the four `_qa_` this issue names and* the four `_summarise_*`
test files patching `get_llm_config` / `require_llm_config`	46

Approach

Reuse LLMConfig as the target. It is already exactly the five fields; a second type would be one more thing to keep in sync.
generate_structured_text(prompt, target, *, ...) with the loose form removed outright, not shimmed. The point of this issue is that a new call site cannot omit a field of an object it is holding; a deprecated shim keeps the omit-able path alive for exactly the "eighteenth call site written next month" the DEFAULT_CONTEXT_TOKENS comment warns about. All call sites are in this repo, so there is nothing for a shim to protect.
Domain functions take target: LLMConfig and pass it on unchanged. Origin sites stop destructuring.
The eight hand-rolled transports take target too, so the count cannot climb back up. Merging them into one transport is out of scope here — that is the simplification pass (#443) and it changes behaviour surface; this issue does not.
No behaviour change. The suite must pass with changes only to test doubles, which become real LLMConfig instances.

Will land as one PR, committed in reviewable pieces (transport → audio_service → other services → tasks/routers → tests).

Picking this up now, ahead of the rest of v4.2.0, and **moving it from v4.6.0 to v4.2.0**. Reasoning: #356, #357 and #358 each need the provider object at the generation call sites, so every one of them done before this adds another hand-threaded parameter through the same signatures. Doing this first makes each of those a one-object change. ## Measured on `main` at `233dc71` (post-#483) | | count | |---|---| | signatures carrying `endpoint_url: str` across `audio_service`, `beat_service`, `generation_service`, `lore_service`, `reminder_tasks`, `planning_tasks` | **34** | | `generate_structured_text` call sites | 20 | | `llm_provider=` occurrences | 49 | | hand-rolled transports that bypass `generate_structured_text` | **8** — the four `_qa_*` this issue names *and* the four `_summarise_*` | | test files patching `get_llm_config` / `require_llm_config` | 46 | ## Approach - **Reuse `LLMConfig` as the target.** It is already exactly the five fields; a second type would be one more thing to keep in sync. - **`generate_structured_text(prompt, target, *, ...)` with the loose form removed outright, not shimmed.** The point of this issue is that a new call site *cannot* omit a field of an object it is holding; a deprecated shim keeps the omit-able path alive for exactly the "eighteenth call site written next month" the `DEFAULT_CONTEXT_TOKENS` comment warns about. All call sites are in this repo, so there is nothing for a shim to protect. - Domain functions take `target: LLMConfig` and pass it on unchanged. Origin sites stop destructuring. - The eight hand-rolled transports take `target` too, so the count cannot climb back up. **Merging them into one transport is out of scope** here — that is the simplification pass (#443) and it changes behaviour surface; this issue does not. - No behaviour change. The suite must pass with changes only to test doubles, which become real `LLMConfig` instances. Will land as one PR, committed in reviewable pieces (transport → `audio_service` → other services → tasks/routers → tests).

~~claude-bot referenced this issue 2026-09-05 00:17:15 +00:00~~

[Backend] The prose summary path and the admin "test LLM" button ignore the explicit AI provider and still sniff the URL #488

claude-bot referenced this issue

2026-09-05 00:18:41 +00:00

[Backend] The prose summary path and the admin "test LLM" button ignore the explicit AI provider and still sniff the URL #488

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

refactor(llm): make LLMConfig the one parameter a generation path takes (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

refactor(audio): pass the LLM target through as one object (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

refactor(llm): take the target whole in the beat, generation and lore services (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

refactor(llm): stop destructuring the config at the origin sites (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

test(llm): give the tests a real LLMConfig, not a look-alike (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:09 +00:00

fix(llm): honour the explicit provider on the prose summary path and the admin test (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:10 +00:00

refactor(llm): take the target whole in the four structured transports too (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:10 +00:00

docs(audio): correct a forward reference #484 has now overtaken (#484)

rbrooks referenced this issue from a commit

2026-09-05 01:00:10 +00:00

refactor(evals): give the eval harness the same target the app uses (#484)

claude-bot referenced this issue from a pull request that will close it,

2026-09-05 01:00:32 +00:00

refactor(llm): pass LLMConfig through the generation paths as one object (#484), and honour the explicit provider on the prose path (#488) #489

claude-bot closed this issue

2026-09-05 01:09:35 +00:00

claude-bot referenced this issue from a commit

2026-09-05 01:09:36 +00:00

Merge PR #489: pass LLMConfig through the generation paths as one object (#484); honour the explicit provider on the prose path (#488)

claude-bot commented

2026-09-05 01:10:16 +00:00

Author

Collaborator

Landed in PR #489 (merged 2026-09-05, CI green). Nine commits, 45 files, 1716 tests passing.

What was done

LLMConfig now lives in app/providers/llm.py, frozen, next to LlmCapabilities/LlmProvider, with DEFAULT_CONTEXT_TOKENS and its rationale block. It had to move: settings_service imports from llm_service, so the transport could not take the config from settings without a cycle. settings_service re-exports the name.
registry.llm_provider_for(target) is the one-object resolve; resolve_llm_provider stays for the two callers that genuinely hold the fields loose.
generate_structured_text(prompt, target, *, ...) — the loose quintet is gone with no shim, and an inspect.signature guard test keeps it gone. All four _structured_*, four _summarise_*, four _qa_* and four _test_llm_* transports take target too, so the destructuring did not just move one frame down.
24 LLM-taking functions in audio_service plus beat_service, generation_service, lore_service, reminder_tasks, planning_tasks and two routers converted; origin sites pass llm_cfg whole. The eight Whisper ASR functions were deliberately left alone (#350). evals/LiveProvider was the one caller outside app/ and had no test cover; it now carries an LLMConfig and gained QB_EVAL_PROVIDER.
Test doubles are real LLMConfig instances everywhere; no SimpleNamespace look-alikes remain.

	before	after
`endpoint_url: str` signatures in `app/`	22	12 (8 ASR + the registry resolvers)
`llm_provider=` in `app/`	49	1 (a response-schema field name)

Verified no behaviour change

An AST walk over the pre-change tree found zero production call sites reaching the transport with context_tokens omitted (45 in tests, all migrated), so no production path's effective window moved. The three places the window is now read off provider.capabilities.context_tokens are identical to the old resolve_window(...) for any positive int, and get_llm_config always resolves one.

Found on the way

#488 — _dispatch_prose and test_llm accepted the explicit provider and ignored it. Fixed in the same PR as its own commit; see that issue.

Left open, on purpose

Merging the twelve hand-rolled transports into one is #443, not this — it changes behaviour surface.
LLMConfig being frozen means a variant is made with dataclasses.replace; evals/providers.py:with_window is the first user.

Landed in **PR #489** (merged 2026-09-05, CI green). Nine commits, 45 files, 1716 tests passing. ## What was done - `LLMConfig` now lives in `app/providers/llm.py`, frozen, next to `LlmCapabilities`/`LlmProvider`, with `DEFAULT_CONTEXT_TOKENS` and its rationale block. It had to move: `settings_service` imports from `llm_service`, so the transport could not take the config from settings without a cycle. `settings_service` re-exports the name. - `registry.llm_provider_for(target)` is the one-object resolve; `resolve_llm_provider` stays for the two callers that genuinely hold the fields loose. - `generate_structured_text(prompt, target, *, ...)` — the loose quintet is gone with **no shim**, and an `inspect.signature` guard test keeps it gone. All four `_structured_*`, four `_summarise_*`, four `_qa_*` and four `_test_llm_*` transports take `target` too, so the destructuring did not just move one frame down. - 24 LLM-taking functions in `audio_service` plus `beat_service`, `generation_service`, `lore_service`, `reminder_tasks`, `planning_tasks` and two routers converted; origin sites pass `llm_cfg` whole. The eight Whisper ASR functions were deliberately left alone (#350). `evals/LiveProvider` was the one caller outside `app/` and had no test cover; it now carries an `LLMConfig` and gained `QB_EVAL_PROVIDER`. - Test doubles are real `LLMConfig` instances everywhere; no `SimpleNamespace` look-alikes remain. | | before | after | |---|---|---| | `endpoint_url: str` signatures in `app/` | 22 | 12 (8 ASR + the registry resolvers) | | `llm_provider=` in `app/` | 49 | 1 (a response-schema field name) | ## Verified no behaviour change An AST walk over the pre-change tree found zero production call sites reaching the transport with `context_tokens` omitted (45 in tests, all migrated), so no production path's effective window moved. The three places the window is now read off `provider.capabilities.context_tokens` are identical to the old `resolve_window(...)` for any positive int, and `get_llm_config` always resolves one. ## Found on the way **#488** — `_dispatch_prose` and `test_llm` accepted the explicit provider and ignored it. Fixed in the same PR as its own commit; see that issue. ## Left open, on purpose - Merging the twelve hand-rolled transports into one is **#443**, not this — it changes behaviour surface. - `LLMConfig` being frozen means a variant is made with `dataclasses.replace`; `evals/providers.py:with_window` is the first user.

rbrooks referenced this issue from a commit

2026-09-05 01:17:05 +00:00

chore(release): v4.1.2

claude-bot referenced this issue

2026-09-05 01:17:20 +00:00

chore(release): v4.1.2 #490

rbrooks referenced this issue from a commit

2026-09-05 03:10:58 +00:00

feat(llm): count what every LLM call consumed, without threading a parameter (#357)

claude-bot referenced this issue

2026-09-05 03:48:21 +00:00

[Backlog] AI provider abstraction and per-campaign BYO-AI #128