[Backend] /ask is unprotected on every provider — no context budget, no num_ctx, no truncation check #426
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
/askquestion-answering path received none of the context hygiene the summarisation path did.The four
/askprovider functionsmax_tokens_qa_anthropic(audio_service.py:3994)_qa_openai(audio_service.py:4020)_qa_llamacpp(audio_service.py:3970)_qa_ollama(audio_service.py:3955)optionskey at all —num_ctxnever setNone of them calls
_reject_if_truncated. None passescontext_tokens._qa_ollamabuilds its body with nooptionsdict 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_anthropicwas named explicitly in #338's body as in scope and was not touched.Why it matters
/askis 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 nofinish_reasoncheck 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
generate_structured_textcall sites./askis one of the 15 without it.num_ctxis set only where a caller passescontext_tokens;_qa_ollamanever does._summarise_*but not_qa_*.Those three stay open on their own merits; this issue exists so the
/askcluster is not lost inside them.A design question worth answering first
_reject_if_truncatedcurrently only raises underjson_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
_qa_*function sets an explicit, non-trivialmax_tokens(1024 is small for a prose answer over ten sessions of context)_qa_ollamasetsoptions.num_ctxfrom the resolved context window_qa_*function checks the provider's truncation signal, and the raise-versus-flag question above is settled and consistent/askanswer is not silently presented as complete#336, #337 and #338 are now closed.
/askwas 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
/askfor freegenerate_structured_textnow resolves an undeclared window to a conservative default rather than switching budgeting off, so any path routing through it is preflighted and getsnum_ctxon 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_ollamastill sends nooptionsdict at all.The table is still accurate, with one addition
_summarise_ollamahad the same defect as_qa_ollama— no_reject_if_truncated, no output cap — and was fixed in65eebb4. It is worth copying that shape rather than inventing a new one:optionsis merged, not assigned, sonum_ctxandnum_predictcoexist. Assigning drops one silently;test_ollama_greedy_decoding_does_not_clobber_num_ctxexists because that already happened once._reject_if_truncatedreturns 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_anthropicand_qa_openaishould become that.Where a flagged
/askanswer should goUnlike the summary path there is no
SummarisationRunto hang a flag on —/askreturns 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/askwas not simply swept up in #338: the mechanism that made "return, flagged" defensible for summaries does not exist here.Done in PR #486 (merged, CI green on all 7 jobs). All five criteria met — closing.
_qa_*sets an explicit, non-trivialmax_tokens—_PROSE_MAX_TOKENS(4096) on all four, replacing 1024 on Anthropic/OpenAI and nothing at all on llama.cpp._qa_ollamasetsoptions.num_ctxfrom the resolved window — together withnum_predict, in one literal rather than two assignments, for the reasontest_ollama_greedy_decoding_does_not_clobber_num_ctxexists._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.QaAnswer→AskResponse.truncated→ the Discord embed.The Ollama half was worse than "degraded answers"
Worth recording plainly: with no
optionsdict, Ollama applied its own 2–4k default to the prompt, so/askon 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_VERSIONbump; a test covers the older-backend direction wheretruncatedis 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'stype("Cfg", ...)shim lackedcontext_tokensand so failed on anAttributeErrorraised before the code under test, andtest_context_boundsmocked_qa_llamacppwith a bare"ok"that would have unpacked into two characters against the new tuple return and passed silently.