[Backend] Extend truncation rejection and token budgeting to the prose paths #338
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?
Severity: HIGH. Found in the August 2026 session lifecycle review (#319).
The defect
Three related gaps, all on the prose (non-JSON) paths that #293 never covered:
chat_template_kwargs: {"enable_thinking": False}is set only insideif json_mode:(llm_service.py:232-241)._summarise_llamacppand_qa_llamacppbuild their own bodies with neither that flag nor anymax_tokens. A reasoning model therefore spends unbounded tokens thinking inside a slot the prompt has already nearly filled — generation hits the ceiling mid-summary,finish_reason=lengthis logged, and the clipped prose is saved as the session summary. A summary cut off mid-session reads exactly like "the timeline is wrong"._summarise_anthropicand_qa_anthropiccap output atmax_tokens: 1024(audio_service.py:2148,:2289) with nostop_reasoncheck. A 3-6 paragraph summary is 400-900 tokens, so this clips occasionally and silently._summarise_openai(:2174) is the same with nofinish_reasoncheck.generate_structured_text(..., json_mode=False)callers —merge_lore_entry_body,rephrase,promote,expand_backstory— all run with thinking enabled for the same reason.Proposed fix
Extend
_reject_if_truncatedto every prose path. Set explicitmax_tokenson llama.cpp prose calls. Disable thinking on prose paths, or budget it explicitly and account for it in the preflight estimate. Raise the Anthropic/OpenAI prose caps well above 1024 and checkstop_reason/finish_reasonon every response.Acceptance criteria
max_tokensset explicitly on llama.cpp prose callsstop_reasonchecked_reject_if_truncatedcovers prose responsesDecision recorded, 2026-08-28: truncated prose is returned-but-flagged, not raised.
This issue's last acceptance criterion asks for a test that "a truncated prose response raises rather than being persisted". The shipped code does the opposite —
_reject_if_truncatedraises only underjson_mode(llm_service.py:243-248), andtest_llamacpp_truncated_prose_is_returnedasserts a truncated prose response comes back. The code and the issue have been contradicting each other since this landed.Ryan's call: return-but-flag is right. Half a summary a GM can read and correct beats no summary at all, which is what raising would produce. Structured output is different and keeps raising — a partial JSON object cannot be trusted or repaired, whereas partial prose is merely short.
So the criterion is amended rather than the code:
Tests assert a truncated prose response raises rather than being persisted_reject_if_truncatedso the next reader does not "fix" one to match the otherThe second point is the actual remaining work. Today a truncated prose response is returned with nothing marking it —
_warn_if_prompt_truncatedlogs, but the prose truncation path does not, so a summary that stops mid-sentence is indistinguishable from one that ended there. Flagging is what makes "return" defensible rather than merely lenient.Related: #426 covers the
/askhalf of this issue's scope, which was never touched.Done in
65eebb4, against the amended criteria from the decision comment above.Correction to my own premise in that comment
I wrote that "
_warn_if_prompt_truncatedlogs, but the prose truncation path does not". That is wrong —_reject_if_truncatedhas logged a warning for prose since #293 (f3264d9), before theif json_mode: raise. The gap is narrower than stated but still real: it was only a log line, so nothing reached the GM or the persisted run.Acceptance criteria
chat_template_kwargson_summarise_llamacpp.max_tokensset explicitly on llama.cpp prose calls —_PROSE_MAX_TOKENS, and now on Ollama too (see below).stop_reasonchecked — both at_PROSE_MAX_TOKENSwith the check._reject_if_truncatedcovers prose responses — it now genuinely does; one provider was missing (below).summarisation_runs.truncated, migratione6f7a8b0c1d2, surfaced through #333's API in session_beat_service.py:107._reject_if_truncated— llm_service.py:240-280, with your reasoning and an explicit warning not to make the two halves consistent._summarise_ollamachecked nothing at allWorth flagging because it undercuts the issue's framing that this was about raising the caps. The other three prose providers called
_reject_if_truncated; Ollama did not call it at all, and sent no output cap either — so it took whatevernum_predictdefault the operator's version happens to have and returned a clipped summary with no signal whatsoever. On a self-hosted-first product that is the backend most likely to be doing it.Both fixed.
optionsis now merged rather than assigned sonum_ctxandnum_predictcoexist — the same bugtest_ollama_greedy_decoding_does_not_clobber_num_ctxalready pins down on the structured path, which would have silently eaten one of the two.How the flag travels
_reject_if_truncatedreturns whether it fired;_dispatch_prosethreads aSummarisationRecordto every provider function. Any prose call in a run sets it — single-shot, a window note, the reduce step, the compose step. Not per-call by design: a GM asking "is what I am reading cut short" does not care which call it happened in, and the map step is the easiest to lose because its output is consumed by the reduce step rather than shown._persist_summarisation_runpreviously returned early whenever a run had no beats, so a prose-fallback run persisted nothing — meaning the exact runs most likely to be truncated were the ones that could not record it. It now writes a row when there are beats or a truncation. The general rule is unchanged: an empty row otherwise reads as "this run found no events".On the tests
The persistence tests build a record by hand, so they would keep passing if nothing ever set the flag — the failure mode you have been bitten by three times. Three more tests in
test_summarise_chunkingpatch the HTTP layer and drive a realsummarise(), including one that truncates only the first window's note so the reduce step completes normally and nothing downstream looks wrong.Verified by mutation rather than assumption: neutering the record threading inside
_dispatch_prosefails both truncation tests and leaves the negative control passing.Migration applied and rolled back against a real database; the full chain applies cleanly from empty to
e6f7a8b0c1d2./askuntouched — #426.1,286 → 1,296 passing, lint clean.