rbrooks/Quest-Board

Watch

Fork 0

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

New issue

Closed

opened 2026-08-27 23:28:53 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-27 23:28:53 +00:00

Collaborator

Found by the v4.0.0 acceptance-criteria pass, while verifying #336, #337 and #338. Each of those three has a gap, and they turn out to be the same gap seen from three angles: the /ask question-answering path received none of the context hygiene the summarisation path did.

The four `/ask` provider functions

function	`max_tokens`	context declared	truncation checked
`_qa_anthropic` (`audio_service.py:3994`)	hardcoded 1024	—	no
`_qa_openai` (`audio_service.py:4020`)	hardcoded 1024	—	no
`_qa_llamacpp` (`audio_service.py:3970`)	absent	—	no
`_qa_ollama` (`audio_service.py:3955`)	—	no `options` key at all — `num_ctx` never set	no

None of them calls _reject_if_truncated. None passes context_tokens. _qa_ollama builds its body with no options dict whatsoever, so Ollama applies its 2048–4096 default and truncates the prompt silently — precisely the defect #337 was filed to fix, still live on this path.

_qa_anthropic was named explicitly in #338's body as in scope and was not touched.

Why it matters

/ask is how a GM queries their own campaign. #340 bounded what goes into the prompt (and that part is done — closed), but nothing checks what comes back. A truncated answer is indistinguishable from a complete one: it is prose, so it simply stops, and there is no finish_reason check to notice. On Ollama the prompt is silently cut instead, so the model answers a question it was only shown part of.

Lower severity than #425 or #397 — this degrades answers rather than destroying data — but it is the one remaining place where the pipeline trusts a provider response without checking it.

#336 — preflight covers 2 of ~17 generate_structured_text call sites. /ask is one of the 15 without it.
#337 — num_ctx is set only where a caller passes context_tokens; _qa_ollama never does.
#338 — the prose truncation work landed on _summarise_* but not _qa_*.

Those three stay open on their own merits; this issue exists so the /ask cluster is not lost inside them.

A design question worth answering first

_reject_if_truncated currently only raises under json_mode (llm_service.py:243-248), and there is a test asserting truncated prose is returned rather than raised (test_llamacpp_truncated_prose_is_returned). That directly contradicts #338's last acceptance criterion, which asks for a raise.

Both positions are defensible — half an answer may beat no answer for a GM asking a question — but the code and the issue currently disagree, and one of them should change. Decide that before implementing, because it determines whether this is "raise" or "return, flagged".

Acceptance criteria

Every _qa_* function sets an explicit, non-trivial max_tokens (1024 is small for a prose answer over ten sessions of context)
_qa_ollama sets options.num_ctx from the resolved context window
Every _qa_* function checks the provider's truncation signal, and the raise-versus-flag question above is settled and consistent
A truncated /ask answer is not silently presented as complete
Tests cover a truncated response on each provider path

Found by the v4.0.0 acceptance-criteria pass, while verifying #336, #337 and #338. Each of those three has a gap, and they turn out to be the *same* gap seen from three angles: **the `/ask` question-answering path received none of the context hygiene the summarisation path did.** ## The four `/ask` provider functions | function | `max_tokens` | context declared | truncation checked | |---|---|---|---| | `_qa_anthropic` (`audio_service.py:3994`) | hardcoded **1024** | — | **no** | | `_qa_openai` (`audio_service.py:4020`) | hardcoded **1024** | — | **no** | | `_qa_llamacpp` (`audio_service.py:3970`) | **absent** | — | **no** | | `_qa_ollama` (`audio_service.py:3955`) | — | **no `options` key at all** — `num_ctx` never set | **no** | None of them calls `_reject_if_truncated`. None passes `context_tokens`. `_qa_ollama` builds its body with no `options` dict whatsoever, so Ollama applies its 2048–4096 default and truncates the prompt silently — precisely the defect #337 was filed to fix, still live on this path. `_qa_anthropic` was **named explicitly in #338's body** as in scope and was not touched. ## Why it matters `/ask` is how a GM queries their own campaign. #340 bounded what goes *into* the prompt (and that part is done — closed), but nothing checks what comes *back*. A truncated answer is indistinguishable from a complete one: it is prose, so it simply stops, and there is no `finish_reason` check to notice. On Ollama the *prompt* is silently cut instead, so the model answers a question it was only shown part of. Lower severity than #425 or #397 — this degrades answers rather than destroying data — but it is the one remaining place where the pipeline trusts a provider response without checking it. ## Related, and deliberately left in the parent issues - **#336** — preflight covers 2 of ~17 `generate_structured_text` call sites. `/ask` is one of the 15 without it. - **#337** — `num_ctx` is set only where a caller passes `context_tokens`; `_qa_ollama` never does. - **#338** — the prose truncation work landed on `_summarise_*` but not `_qa_*`. Those three stay open on their own merits; this issue exists so the `/ask` cluster is not lost inside them. ## A design question worth answering first `_reject_if_truncated` currently **only raises under `json_mode`** (`llm_service.py:243-248`), and there is a test asserting truncated prose is *returned* rather than raised (`test_llamacpp_truncated_prose_is_returned`). That directly contradicts #338's last acceptance criterion, which asks for a raise. Both positions are defensible — half an answer may beat no answer for a GM asking a question — but the code and the issue currently disagree, and one of them should change. Decide that before implementing, because it determines whether this is "raise" or "return, flagged". ## Acceptance criteria - [ ] Every `_qa_*` function sets an explicit, non-trivial `max_tokens` (1024 is small for a prose answer over ten sessions of context) - [ ] `_qa_ollama` sets `options.num_ctx` from the resolved context window - [ ] Every `_qa_*` function checks the provider's truncation signal, and the raise-versus-flag question above is settled and consistent - [ ] A truncated `/ask` answer is not silently presented as complete - [ ] Tests cover a truncated response on each provider path

claude-bot added this to the v4.2.0 - AI Provider Abstraction & BYO-AI milestone

2026-08-27 23:28:58 +00:00

claude-bot added the

backend

bug

labels

2026-08-27 23:28:58 +00:00

claude-bot referenced this issue

2026-08-28 14:33:33 +00:00

[Backend] Extend truncation rejection and token budgeting to the prose paths #338

rbrooks referenced this issue from a commit

2026-08-28 16:37:13 +00:00

feat(backend): give every LLM call site the real context window (#336)

rbrooks referenced this issue from a commit

2026-08-28 16:37:13 +00:00

fix(backend): make a truncated summary visible to the GM (#338)

claude-bot referenced this issue

2026-08-28 16:37:35 +00:00

[Backend] Preflight every prompt against the provider's declared context window #336

claude-bot referenced this issue

2026-08-28 16:37:56 +00:00

[Backend] Extend truncation rejection and token budgeting to the prose paths #338

claude-bot commented

2026-08-28 16:38:53 +00:00

Author

Collaborator

#336, #337 and #338 are now closed. /ask was left untouched, as this issue intended — recording what changed underneath it and what did not, so the next person does not re-derive it.

The design question is settled

This issue asks for the raise-versus-flag question to be decided before implementing. Ryan decided it on #338: prose returns, flagged; structured output raises. Half a summary — or half an answer — a GM can read and correct beats none at all, whereas a partial JSON object cannot be trusted or repaired. The rationale and an explicit warning not to make the two halves consistent now live in _reject_if_truncated (llm_service.py:240-280).

So this issue's third criterion resolves to flag, not raise, and its fourth ("a truncated answer is not silently presented as complete") is the real work.

What changed under `/ask` for free

generate_structured_text now resolves an undeclared window to a conservative default rather than switching budgeting off, so any path routing through it is preflighted and gets num_ctx on Ollama. That does not cover _qa_*: those four functions build their own request bodies and bypass the transport entirely, exactly as this issue's table says. answer_question → _qa_ollama still sends no options dict at all.

The table is still accurate, with one addition

_summarise_ollama had the same defect as _qa_ollama — no _reject_if_truncated, no output cap — and was fixed in 65eebb4. It is worth copying that shape rather than inventing a new one:

options is merged, not assigned, so num_ctx and num_predict coexist. Assigning drops one silently; test_ollama_greedy_decoding_does_not_clobber_num_ctx exists because that already happened once.
_reject_if_truncated returns whether it fired, so a caller can record it rather than only logging.
_PROSE_MAX_TOKENS (4096) is the shared prose cap. The 1024 in _qa_anthropic and _qa_openai should become that.

Where a flagged `/ask` answer should go

Unlike the summary path there is no SummarisationRun to hang a flag on — /ask returns a response to a live request rather than persisting a run. So the flag likely belongs in the response body, and the front end has to render it. That is a genuine design decision this issue still owns, and it is the reason /ask was not simply swept up in #338: the mechanism that made "return, flagged" defensible for summaries does not exist here.

**#336, #337 and #338 are now closed. `/ask` was left untouched, as this issue intended** — recording what changed underneath it and what did not, so the next person does not re-derive it. ## The design question is settled This issue asks for the raise-versus-flag question to be decided before implementing. Ryan decided it on #338: **prose returns, flagged; structured output raises.** Half a summary — or half an answer — a GM can read and correct beats none at all, whereas a partial JSON object cannot be trusted or repaired. The rationale and an explicit warning not to make the two halves consistent now live in `_reject_if_truncated` ([llm_service.py:240-280](webapp/backend/app/services/llm_service.py#L240-L280)). So this issue's third criterion resolves to **flag**, not raise, and its fourth ("a truncated answer is not silently presented as complete") is the real work. ## What changed under `/ask` for free `generate_structured_text` now resolves an undeclared window to a conservative default rather than switching budgeting off, so **any** path routing through it is preflighted and gets `num_ctx` on Ollama. That does **not** cover `_qa_*`: those four functions build their own request bodies and bypass the transport entirely, exactly as this issue's table says. `answer_question` → `_qa_ollama` still sends no `options` dict at all. ## The table is still accurate, with one addition `_summarise_ollama` had the same defect as `_qa_ollama` — no `_reject_if_truncated`, no output cap — and was fixed in `65eebb4`. It is worth copying that shape rather than inventing a new one: - `options` is **merged**, not assigned, so `num_ctx` and `num_predict` coexist. Assigning drops one silently; `test_ollama_greedy_decoding_does_not_clobber_num_ctx` exists because that already happened once. - `_reject_if_truncated` returns whether it fired, so a caller can record it rather than only logging. - `_PROSE_MAX_TOKENS` (4096) is the shared prose cap. The 1024 in `_qa_anthropic` and `_qa_openai` should become that. ## Where a flagged `/ask` answer should go Unlike the summary path there is no `SummarisationRun` to hang a flag on — `/ask` returns a response to a live request rather than persisting a run. So the flag likely belongs in the response body, and the front end has to render it. That is a genuine design decision this issue still owns, and it is the reason `/ask` was not simply swept up in #338: the mechanism that made "return, flagged" defensible for summaries does not exist here.

claude-bot referenced this issue

2026-09-01 18:49:00 +00:00

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

claude-bot referenced this issue

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

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

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)

rbrooks referenced this issue from a commit

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

docs(changelog): record the /ask truncation fixes (#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 from a commit

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

Merge PR #486: stop presenting a truncated /ask answer as a complete one (#426)

claude-bot commented

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

Author

Collaborator

Done in PR #486 (merged, CI green on all 7 jobs). All five criteria met — closing.

Every _qa_* sets an explicit, non-trivial max_tokens — _PROSE_MAX_TOKENS (4096) on all four, replacing 1024 on Anthropic/OpenAI and nothing at all on llama.cpp.
_qa_ollama sets options.num_ctx from the resolved window — together with num_predict, in one literal rather than two assignments, for the reason test_ollama_greedy_decoding_does_not_clobber_num_ctx exists.
Every _qa_* checks the truncation signal, consistently — through _check_usage, the same helper the structured paths use. Settled as flag, not raise, per the decision recorded on #338.
A truncated answer is not silently presented as complete — QaAnswer → AskResponse.truncated → the Discord embed.
Tests cover a truncated response on each provider path — and the clean-stop case on each too, because a flag that is always on tells a GM nothing.

The Ollama half was worse than "degraded answers"

Worth recording plainly: with no options dict, Ollama applied its own 2–4k default to the prompt, so /ask on a self-hosted Ollama was answering from a fraction of the context it had assembled. That is #337's defect, still live on this path a release later, on the provider a self-hoster is most likely to run. The other three degrade the answer; this one degraded the question.

The design question this issue owned

Resolved as: the flag travels in the response body, and the bot renders it in the embed description, not the footer. A GM may act on a partial answer and footer text is small grey print. The answer budget shrinks to 3900 characters so the notice cannot push the embed past Discord's 4096-character cap — that failure would turn a degraded answer into no answer, which is strictly worse than the bug being fixed. There is a test for it.

Additive field, so no BOT_CONTRACT_VERSION bump; a test covers the older-backend direction where truncated is absent.

What this did not do

The four _qa_* transports are still near-duplicates — folding them into the shared one is #484, and doing it here would have been smuggling a refactor into a bugfix. What did change is that the checks are now shared, so the specific failure mode that produced this issue (one of four getting a fix the others did not) cannot recur even while the transports stay separate.

Two test doubles were fixed rather than worked around, both of which were passing while diverging from reality (#441's concern): test_recap_email's type("Cfg", ...) shim lacked context_tokens and so failed on an AttributeError raised before the code under test, and test_context_bounds mocked _qa_llamacpp with a bare "ok" that would have unpacked into two characters against the new tuple return and passed silently.

Done in PR #486 (merged, CI green on all 7 jobs). All five criteria met — closing. - [x] **Every `_qa_*` sets an explicit, non-trivial `max_tokens`** — `_PROSE_MAX_TOKENS` (4096) on all four, replacing 1024 on Anthropic/OpenAI and nothing at all on llama.cpp. - [x] **`_qa_ollama` sets `options.num_ctx` from the resolved window** — together with `num_predict`, in one literal rather than two assignments, for the reason `test_ollama_greedy_decoding_does_not_clobber_num_ctx` exists. - [x] **Every `_qa_*` checks the truncation signal, consistently** — through `_check_usage`, the same helper the structured paths use. Settled as **flag, not raise**, per the decision recorded on #338. - [x] **A truncated answer is not silently presented as complete** — `QaAnswer` → `AskResponse.truncated` → the Discord embed. - [x] **Tests cover a truncated response on each provider path** — and the clean-stop case on each too, because a flag that is always on tells a GM nothing. ## The Ollama half was worse than "degraded answers" Worth recording plainly: with no `options` dict, Ollama applied its own 2–4k default to the **prompt**, so `/ask` on a self-hosted Ollama was answering from a fraction of the context it had assembled. That is #337's defect, still live on this path a release later, on the provider a self-hoster is most likely to run. The other three degrade the *answer*; this one degraded the *question*. ## The design question this issue owned Resolved as: the flag travels in the response body, and the bot renders it in the embed **description, not the footer**. A GM may act on a partial answer and footer text is small grey print. The answer budget shrinks to 3900 characters so the notice cannot push the embed past Discord's 4096-character cap — that failure would turn a degraded answer into no answer, which is strictly worse than the bug being fixed. There is a test for it. Additive field, so no `BOT_CONTRACT_VERSION` bump; a test covers the older-backend direction where `truncated` is absent. ## What this did not do The four `_qa_*` transports are still near-duplicates — folding them into the shared one is **#484**, and doing it here would have been smuggling a refactor into a bugfix. What *did* change is that the checks are now shared, so the specific failure mode that produced this issue (one of four getting a fix the others did not) cannot recur even while the transports stay separate. Two test doubles were fixed rather than worked around, both of which were passing while diverging from reality (#441's concern): `test_recap_email`'s `type("Cfg", ...)` shim lacked `context_tokens` and so failed on an `AttributeError` raised *before* the code under test, and `test_context_bounds` mocked `_qa_llamacpp` with a bare `"ok"` that would have unpacked into two characters against the new tuple return and passed silently.

claude-bot closed this issue

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

rbrooks referenced this issue from a commit

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

chore(release): v4.1.1

claude-bot referenced this issue

2026-09-05 00:11:57 +00:00

chore(release): v4.1.1 #487