fix(*): stop silent output truncation from becoming a retry loop - #308
fix(*): stop silent output truncation from becoming a retry loop#308gloryfromca wants to merge 33 commits into
Conversation
|
Claude finished @gloryfromca's task in 4m 1s —— View job Review: no blockers; this can mergeAll 7 findings from the earlier review passes on this PR have been addressed by the subsequent commits (
I re-verified the fixes against their call sites rather than trusting the commit messages alone: checked that Azure/Codex providers don't override Did not find any new issues introduced by the latest commits ( Note: I could not execute |
2ba9790 to
76111c2
Compare
chat_stream declared literal defaults for max_tokens, temperature and reasoning_effort. The agent loop calls it with messages/tools/model only, so those literals shadowed the configured values on every streaming turn: requests went out at 4096/0.7/unset regardless of agents.defaults. Only timeout was read from self.generation, which is what marks this as an omission rather than a design choice. Resolve the three from self.generation through the same _SENTINEL the sibling chat_with_retry already uses, so both parameter-assembly paths share one semantics instead of the streaming path carrying its own. The base-class fallback needs the same treatment. It is the path taken by providers without real streaming (azure / codex / custom); leaving its literals in place would keep those on 4096 after the LiteLLM path is fixed. generation is read defensively there: a subclass that never runs the base __init__ reaches chat_stream with the attribute missing, and crashing on it would be worse than the settings this restores. Tests assert the outgoing request body rather than that a call happened -- the defect is invisible to a call-count assertion, and equally invisible in review, since the signature reads as perfectly reasonable. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
StreamDelta has carried a finish_reason field since the provider redesign, but nothing ever wrote the upstream value into it: _normalize_stream_chunk only filled "error" on the failure path. The loop consequently had no way to tell a completed response from one cut off at the output ceiling, and synthesised its own value from whether tool calls were present. Read choices[0].finish_reason and pass it through. The emptiness check that decides whether to skip a chunk has to account for it too -- upstream states why it stopped only on the terminal chunk, which carries no content, no tool calls and no usage, so the old check dropped exactly the chunk that holds the signal. The field now carries two unrelated meanings: the loop reads finish_reason == "error" as a replayed provider failure, and upstream values ride the same field. They do not collide today (the loop compares against "error" exactly) and a test pins that, since a truncated response silently taking the error path would be worse than not detecting truncation at all. test_normalize_stream_chunk_returns_none_for_empty_payload asserted that a stop-marker chunk is skipped. That chunk is no longer empty -- finish_reason is its payload -- so the case is split: a chunk carrying nothing at all is still skipped, and a new case asserts the terminal chunk survives. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The streaming path synthesised its own finish_reason from whether tool calls
were present, so a response cut off at the output ceiling was indistinguishable
from one the model chose to end. Nothing downstream -- the model, the logs, the
trace -- could tell the two apart.
Decide it from three independent signals, OR'd:
upstream finish_reason == "length" costs nothing to trust, but some
backends report a clean stop on a
truncated reply
output_tokens >= the ceiling survives a lying finish_reason, needs
the ceiling to be known
tool arguments failed to parse computed locally from what arrived,
but only exists on a tool-call turn
None subsumes the others, hence OR rather than a single check. The bias is
deliberately toward over-reporting: a false positive costs the model one extra
sentence it can weigh against what it just wrote, while a false negative costs
a retry loop where it re-sends the same oversized payload and is told again
that a field is missing.
_finalize_tool_calls now also reports whether any arguments blob failed to
parse, which is the third signal. The ceiling is read from the provider's
generation settings; a provider that has none simply sits that signal out
rather than misfiring on it.
The verdict rides on LLMResponse so the span extractor can stamp
llm.truncated / llm.max_tokens, and only when true -- a truncated call is the
rare one worth finding in a trace, and an always-present false would bury it.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
When a tool call is cut off at the output ceiling, schema validation reports whatever the model had not reached yet as a missing required field. That message is true and useless: it tells a model that wrote a path that it forgot the path, and the generic hint appended to it -- "try a different approach" -- points away from the only approach that works. The observed result was the model resending the same oversized payload until the turn ran out. The loop now marks the calls from a truncated turn, and the registry names the real cause instead of the schema's account of it. The hint is dropped along with the message it qualifies. Marks go on every call from the turn rather than only those whose JSON failed to parse. When the transport closes the braces for us the blob parses cleanly and simply lacks the fields the model never reached, which is exactly the shape that reads as a model error and is not one. The markers are stripped before anything else inspects params -- they are the loop's note to this method, not arguments any tool declared. This does not cover a call whose fields all validate but whose last value was cut mid-word: it executes and writes a half-file. Catching that means refusing the call before dispatch, which is a control-flow change; the boundary is pinned by a test so it does not read as an oversight. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Reasoning that a backend inlines into content is stripped by matching paired <think>...</think>. When the turn is cut off inside such a block, what arrives is the second half: a closing tag with no opener to pair against. The substitution finds nothing, an eleven-character string reads as a real answer, empty-response recovery is skipped, and the tag is what the user sees. After stripping paired blocks, check whether anything survives removing the tags. If not, the response is empty and recovery runs. The check is on residue rather than on vendor spellings: which prefix a backend picked is not knowable in advance, and getting one wrong costs a missed recovery rather than a mangled answer. The returned text is never rewritten by this check, so an answer that talks about tags keeps its own words -- only one made of nothing else is discarded. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
A cut-off turn was flagged by writing `_truncated` and `_truncated_at` into the call's own arguments dict, which the registry popped back out before dispatch. That dict is the wrong carrier: `to_openai_tool_call` serializes it into the assistant message, and the loop does that before the registry ever sees the call. The markers were already in the conversation history by the time they were stripped, so the model read back a field it never sent, and the UI tool event carried them too. Move the note onto the request itself as a `TruncationInfo` the registry takes as a keyword argument. Only `arguments` is serialized, so history and tool events stay clean, and a tool that happens to declare a parameter named `_truncated` no longer has it eaten. The wording moves onto `TruncationInfo` with it: the registry asks the note for a sentence rather than knowing which fields it carries. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Every call from a truncated turn was marked, which is wider than the fact. The stream emits calls in order, so anything before the last one finished arriving before the ceiling was reached. A complete call that also happens to be malformed then gets told it was truncated -- the wrong diagnosis for a mistake the model really did make, which is the same misdirection this branch set out to remove. Mark the last call only. The reason for not narrowing further, down to just the calls whose JSON failed to parse, is unchanged: when the transport closes the braces the blob parses cleanly and simply lacks whatever the model had not reached yet. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
`agents.defaults.maxTokens` defaulted to 8192 for every model, and onboarding wrote that default into the user's config file, so the number arrived at the provider indistinguishable from one the user had chosen. It could not be right for everyone: claude-sonnet-4 can emit 64000 and gpt-4o 16384, while some models top out below 8192, where sending the larger number is a 400. Retire the setting and resolve the ceiling per model from the same catalogue the context window already comes from. An old config carrying the key is ignored rather than rejected -- `AgentDefaults` does not forbid extras -- so no migration is needed and nobody has to edit a file. `resolve_max_output_tokens` answers with a number rather than None, since the caller is about to build a request: catalogue first, then a fixed `DEFAULT_MAX_OUTPUT_TOKENS` of 8192. That constant is the value the retired setting defaulted to, so a model the catalogue does not know behaves exactly as it did before. This is the shape LiteLLM's own Anthropic path uses, and the shape the two implementations without known config-shadowing bugs use; the four with such bugs all merge a global constant into user configuration. `GenerationSettings.max_tokens` stays as a pin for call sites that ask for a deliberately short answer (the personalizer wants 100 tokens, not the model's maximum). None there now means "no opinion", not 4096. `send_max_tokens` is one function rather than two call sites computing the same thing, because the loop's ceiling check compares against it. Resolved separately, the two would drift the moment either grew a bound, and the check would stop firing without ever failing. Also stop the context-window lookup from falling back to `max_tokens`: in the catalogue that field is the *output* ceiling, so a model missing `max_input_tokens` was being sized with 16384 where it has 128000. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Two patterns still matched only the bare spellings. The orphan-close split looked for `</think>` and `</thinking>`, and the inline-thinking scan looked for opening tags alone. Both shapes they missed are ones a real turn produces. Vendors namespace the tag (MiniMax emits `<mm:think>`), and a turn cut off inside an inlined block arrives as a closing tag with no opener. The scan then reports that the turn produced no reasoning, so a reasoning-only response is treated as a real answer and empty-response recovery never runs. Matched by shape rather than by a list of spellings: which prefix a backend picked is not knowable in advance, and getting one wrong costs a missed recovery rather than a mangled answer. Prose that merely says "thinking" is still not a match -- the pattern is anchored to a tag. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
A file the model cannot finish in one call had no second move: write_file only overwrote, so continuing meant re-sending everything written so far -- the same oversized call that got truncated the first time. `mode=append` adds to the end, `overwrite` stays the default so existing behaviour is unchanged. The description now tells the model to split a long file, because the schema is the only thing it reads before choosing a call shape. An append with empty content is refused rather than treated as a no-op: that is exactly what a call cut off before its content field looks like, and the one thing it must never silently become is an overwrite of the part already written. An unknown mode is refused for the same reason -- guessing which one was meant picks between "add" and "replace" on the model's behalf. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…he tool The loop-break nudge fired after N consecutive hard failures of the same tool, regardless of whether they were the same failure. A model getting a truncation, then a schema error, then a missing path from write_file is adapting, not stuck -- and it was told to stop repeating itself. The streak now keys on (tool, failure class), so it only fires on a genuinely repeated dead call. The classes are coarse on purpose; the question being asked is "same error again?", not "which error exactly?". The nudge text no longer branches on a guessed cause. It used to offer three guesses, one of which told the model to re-examine the path -- advice that, given to a model whose arguments had been truncated, sends it hunting for a mistake it did not make. It now points at the error text, which is the one account of the failure that is actually true. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The fallback was 8192, chosen so a model the catalogue does not know would behave exactly as it had under the retired `agents.defaults.maxTokens`. That made the change safe to land but left unmapped backends -- self-hosted deployments, gateways, models newer than the table -- asking for half of what a mapped model of the same class gets. 16384 is picked for breadth rather than for any one model: Claude Code defaults to 16000, the Anthropic SDK suggests ~16000 for non-streaming, and gpt-4o's real ceiling is also 16384. Unmapped backends are OpenAI-compatible servers in practice, and those clamp an over-large value rather than rejecting it, so the direction of a wrong guess here is a smaller answer rather than a failed request. Note this is not a default in the sense the surveyed agents use one: theirs applies to every model, mapped or not, which is what makes a single constant wrong for someone. This one only answers where the catalogue cannot. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
`registry.execute` took a bare `truncation` argument, which named one fact rather than the category it belongs to. Truncation is not the only thing that can be true about the call a tool result came from, and the next such fact would have arrived as a second bare parameter. `RunMeta` holds what happened around a call, as opposed to what the call asks for. Today it carries one field, so it is a container ahead of its second occupant -- the payoff is that adding one does not touch a signature or a call site. It stays keyword-only for the same reason: five call sites read better naming what they pass, and position must not become part of the contract. The reason it is separate from `arguments` is unchanged and is the point of the split: anything in that dict is serialized into the assistant message before the registry ever sees the call. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Two groups had no reason to live in a 3278-line module. The think-tag pattern existed twice, character for character: `recovery.py` already owned one and `main.py` grew an identical copy when the residue check was added. Two copies of a pattern that has to stay in step is a defect waiting for the next vendor spelling -- one gets updated, the other does not, and the mismatch shows up as a missed recovery rather than as a failure. `recovery.py` keeps the pattern and answers the two questions asked of it; `_strip_think` delegates. Failure-streak accounting moves to `failure_streak.py`: which failures are deterministic enough to count, what counts as the same failure, and what the nudge says. The loop keeps only the counting. The three names are public now that they cross a module boundary. No behaviour change; `main.py` loses 75 lines. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
A call site pinning `max_tokens` won outright over the catalogue. Every pin in the tree is far below any real ceiling -- the personalizer asks for 100, the curator for 2000 -- so this never fires today, which is exactly why it needs writing down: the first pin above a model's limit would be a rejected request, and nothing at the call site would say why. The pin still wins as a request for a shorter answer. It just cannot ask for more than the model accepts. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
A previous commit here removed `max_tokens` from the context-window lookup, on the grounds that the field is the output ceiling and not a window. The field reading was right; the conclusion was not, and the scenario given for it does not exist: a model with a 128k window always carries `max_input_tokens`, so the fallback was never reached for one of those. What it was actually reached for is six rows that carry no `max_input_tokens` at all. Removing it dropped them to this module's documented default of 65536 -- an eight-fold over-estimate for gemma-2, whose window really is 8192, and a wrong direction: over-estimating a window lets a prompt exceed it, where under-estimating only trims more history than needed. Restored, with the reason stated as the bound it is: a model's window is never smaller than what the model is allowed to emit, so the output ceiling is a safe lower bound when the window itself is unknown. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…ink> Debris detection and the orphan-close split both match think tags by shape -- any of the three names, with or without a vendor namespace prefix. The paired substitution still matched one literal spelling, so it was the last place a vendor could get a tag past us, and the worst place for that to happen: a complete `<mm:think>...</mm:think>` block was left in the content and rendered to the user as if the reasoning were the answer. `<thinking>...</thinking>` went the same way. The pattern now matches the same shape as the other two. A backreference keeps the two ends the same tag name, so `<think>x</thinking>` stays untouched -- deleting everything between two unrelated tags would take real content with it. The prefixes are deliberately not tied to each other: a backend that stamps one end and not the other still wrote one block. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Truncation detection lived inside `_llm_call_stream`, so it only ran when a
caller had wired token callbacks. Everything that does not -- `raven agent` on
the CLI, sub-agents, the sentinel -- went through `chat_with_retry` with no
detection at all, and a cut-off tool call was reported to the model as a
missing required field. That is the misdiagnosis this branch set out to
remove, still fully in place on the path most non-TUI callers take.
Reproduced live before fixing: a CLI turn with the ceiling forced low spent
twelve iterations re-sending the same `write_file({"path": ...})`, told each
time that it had forgotten `content`, with the model eventually announcing it
would supply the field it had never omitted.
The decision moves to `loop/truncation.py` and both paths call it. What each
can observe differs -- only the streaming path sees whether the arguments JSON
needed repairing, since the non-streaming parser repairs it before the loop is
handed the result -- but the two remaining signals stand on their own, and the
one that fired here (upstream `finish_reason`) is available to both.
The end-to-end case drives a real AgentLoop with no token callback, which is
what makes it able to fail: the six unit cases around the decision function
all stay green when the non-streaming call site is deleted.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
A `git add -A` swept an unrelated working-tree edit into the previous commit: a debugging value of 60 on `GenerationSettings.max_tokens` and on `LLMResponse.max_tokens`. The first one caps every request in the tree at 60 output tokens. Nothing in the suite would have caught it, which is the more useful half of this fix: every provider stub sets its own `generation`, so the shipped default is never read by a test. A case now asserts it directly. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…ceiling The sentinel rewrite reached `base.py` and `litellm_provider.py` and stopped there. Three providers kept `max_tokens: int = 4096` in their own signatures, which was harmless while the resolved value was always a number and became a crash once it could be `None`: `chat_with_retry` passes the resolved value explicitly, so the literal default never applies, and Azure took that `None` straight into `max(1, max_tokens)`. Azure now resolves it the same way the other providers do. Codex and MiniMax only forward the value, so their signatures were the whole lie. A case walks every shipped `LLMProvider` subclass and fails on any integer literal defaulted into `max_tokens`, because this is the second time the same defect has been fixed in some places and missed in others -- it is the exact shape of the bug this branch started from. Test doubles are exempt: they never build a request, and twenty-three of them carry the old literal. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The docstring still explained `DEFAULT_MAX_OUTPUT_TOKENS` as "the value the retired setting defaulted to", which stopped being true when it moved from 8192 to 16384. Points at the constant's own comment instead of restating a reason that can drift again. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The truncation error ended with "Write it in several smaller pieces instead", which is advice, and advice cannot be generic. Two live runs showed both ways it fails. For `write_file` it was too vague: the model read "send less", sent the same call with `content` omitted, and did that for thirty-two iterations. It had already worked out on its own that the file needed splitting -- its closing summary said so -- and still never used `mode=append`, because that parameter is declared in the tool schema and a model in a retry loop only ever re-reads the error text. For `exec` it was wrong outright: half a command is not a command, and the sentence told the model to send one anyway. `Tool.truncation_hint` lets the tool that knows say what to do; the shared message now stops at "do not resend this", which is true everywhere. write_file names the append mode, exec says it has no smaller form, and a tool that declares nothing gets the neutral message alone -- which beats advice that does not fit it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The message ended with "Do not resend the same content", which is the
opposite of what has to happen. Nothing past the cut survives: the upstream
drops the key it was still writing, so the call that reaches the tool carries
only the fields that finished, and the assistant message stored in history is
that same stub. The content exists nowhere and must be sent again.
A live run showed the model following the sentence exactly. It spent forty
iterations sending `write_file({"path": "snake.py"})` -- twenty characters,
content withheld -- while the session record confirms not one of those turns
ever carried a content field, and the first eight were billed 1200 output
tokens each. It was not confused; it was obeying.
The message now says what is true: everything past the cut is gone and has to
come again, just not as one call that size. What to send instead still comes
from the tool.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The shared message had grown an instruction -- "it has to be sent again, but not as one call this size" -- which is both advice and a near-duplicate of what write_file's own hint already says. Advice cannot be shared: it was wrong for exec twice over, first telling it to split an argument that has no halves, then repeating a size warning it had no way to act on. Two facts remain, true of every tool: the call was cut off, and nothing past the cut was kept. Everything after that comes from the tool. The write_file message drops from 380 to 250 characters and says more. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
f3be4b0 to
9dc00ab
Compare
| tools: list[dict[str, Any]] | None = None, | ||
| model: str | None = None, | ||
| max_tokens: int = 4096, | ||
| max_tokens: int | None = None, |
There was a problem hiding this comment.
[blocker] This override reintroduces exactly the shadowing bug this PR fixes elsewhere. AgentLoop calls self.provider.chat_stream(messages, tools, model) with no temperature/reasoning_effort, so this override's own literal defaults (temperature: float = 0.7, reasoning_effort: str | None = None at lines 57-58) win and get forwarded as concrete positional args to super().chat_stream(...) (LiteLLMProvider.chat_stream, litellm_provider.py) at lines 62-64.
LiteLLMProvider.chat_stream only falls back to self.generation.temperature / self.generation.reasoning_effort when it receives LLMProvider._SENTINEL (litellm_provider.py:513-519). Since this override always passes a concrete value (never the sentinel), that fallback path is dead for every MiniMax OAuth streaming call — a user-configured temperature/reasoning_effort is silently ignored, the same failure mode as §1 of the PR description.
max_tokens here happens to still work only because LiteLLMProvider.chat_stream has an additional if max_tokens is None: max_tokens = send_max_tokens(...) check that isn't sentinel-gated — temperature/reasoning_effort have no equivalent fallback.
Fix: give this override the same object = LLMProvider._SENTINEL defaults used in litellm_provider.py/base.py, and forward them unresolved so the parent resolves against self.generation. The same shadowing exists in chat() above, though it's masked there because chat_with_retry always resolves settings before calling chat() directly — chat_stream has no such wrapper.
No test in tests/test_minimax_oauth.py or tests/test_litellm_provider_stream.py exercises this override's defaults, so the new regression suite doesn't catch it.
| # Opening or closing reasoning tag, with or without a vendor namespace prefix | ||
| # (<think>, </thinking>, <mm:think>, ...). Used only to ask whether a stripped | ||
| # response is nothing but tag debris -- matching a spelling wrong costs a | ||
| # missed recovery, never a mangled answer. |
There was a problem hiding this comment.
[nit] This comment block (opening/closing reasoning tag, vendor namespace prefixes) describes the think-tag regex logic that actually lives in recovery.py (_THINK_TAG_RE, strip_think_blocks) — it's dangling here above _strip_inline_images, an unrelated image-stripping helper. Looks like a stray leftover from moving that logic into recovery.py; safe to delete.
`flag_truncation` returns the ceiling it compared against, and the streaming path puts it on the response. The non-streaming call site discarded it, so a truncated turn there reached tracing with `llm.max_tokens` unset -- and that attribute is only stamped when `llm.truncated` is true, which makes it exactly the record where the number matters. Behaviour is unchanged: the number the model sees comes from `TruncationInfo.at_tokens`, which `flag_truncation` sets internally on the cut call. Only the span was missing it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
| # no token callback and so lands here -- reports a cut-off tool | ||
| # call as a missing required field, which is the misdiagnosis | ||
| # this whole path exists to remove. | ||
| response.max_tokens, response.truncated = flag_truncation( |
There was a problem hiding this comment.
[blocker] This flag_truncation call (and the whole point of the last commit, "record the sent ceiling on non-streaming responses too") runs after chat_with_retry has already returned — but chat_with_retry is itself wrapped in @trace.instrument("llm.call", extract=semconv.llm_call) (raven/providers/base.py:724). trace.instrument's awrapper runs _close/extract in a finally block and closes the span via the with span(...) context manager before returning control to the caller (raven/tracing/trace.py:367-385). semconv.llm_call builds attrs = llm_attrs(result, ...) synchronously off the result object at that point (raven/tracing/semconv.py:623), reading getattr(resp, "truncated", False) / getattr(resp, "max_tokens", None) — both still the defaults (False / None) since this mutation hasn't happened yet.
So on this whole branch (raven agent CLI, sub-agents that would use it, anything with no token callback), response.max_tokens/response.truncated are set here for downstream use in the loop (correctly — the tool-facing [truncated] error message still works because it reads tool_calls[-1].run_meta, mutated on the same objects), but the llm.call trace span for that request will always record llm.truncated as absent and never gets llm.max_tokens — contradicting this commit's stated goal and the PR's own risk section ("llm.truncated / llm.max_tokens are stamped on the span only when truncation is detected"). No test asserts on the span attributes for this path, which is why it wasn't caught (compare to the streaming path, where flag_truncation runs inside _llm_call_stream before that function — also @trace.instrument-wrapped — returns, so its span is correct).
Fix: either call flag_truncation before chat_with_retry returns (not possible without changing that method), or move the instrumentation point / re-emit the attribute after the fact, or accept and document that non-streaming truncation tracing is a known gap.
A cut-off call whose required fields happen to have arrived still validated, and so it ran. That is not a smaller version of the call the model meant to make -- it can be a different call. `mode` is optional on write_file, so a cut landing before it falls back to "overwrite" and silently replaces everything an earlier append had written, then reports success. The model has no way to notice: it asked to append, was told it succeeded, and the file is now the last chunk alone. Validation cannot see this. The call is well-formed; only the fact that it was cut says the intent behind it is incomplete. So the refusal moves ahead of validation, keyed on truncation alone. Being wrong here costs one retry, because a turn can finish a tool call and then hit the ceiling in the prose that follows. The streaming path can tell those apart -- deltas arrive in generation order, so it reports whether the cut landed inside tool arguments, and a call that finished first is left dispatchable. The non-streaming path cannot: litellm flattens the response into separate `content` and `tool_calls` fields with no record of which came last, so it says nothing and the call is refused. A wasted turn is cheaper than a silent overwrite. The message says all of this in one line now: where it stopped, that nothing ran, and why that matters. It also drops the word "saved" -- the tool most likely to be truncated is the one that saves files, where "not saved" reads as "the write failed" rather than "your arguments never arrived". Closes the boundary a test used to pin open; that test is inverted here. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
| ``run_meta`` carries what happened around the call rather than what it | ||
| asks for -- today only whether it finished arriving, which changes what | ||
| a validation failure means. Keyword-only so the five call sites stay | ||
| self-describing and so a second field does not reorder anything. |
There was a problem hiding this comment.
[nit] Stale docstring after this commit. It still says run_meta "changes what a validation failure means," but the refusal now happens at line 79, before cast_params/validate_params ever run (lines 85-90) — a truncated call is refused unconditionally, regardless of whether it would have validated. Worth updating to say it preempts dispatch rather than reinterprets a validation error.
Truncation is decided from three signals, but only two of them reached the non-streaming path. The third -- a tool call whose arguments needed repairing to parse -- was dropped in the provider: `json_repair.loads` closed the blob and nothing recorded that it had to. That was left alone on the grounds that the field could not be malformed there, since LiteLLM's Anthropic transform parses `input` into an object and dumps it back. Measured against the gateway Raven actually uses, that is not what arrives. Eight probes through openrouter at ceilings of 40 and 120 tokens, streaming and not, Anthropic- and OpenAI-backed: claude-sonnet-4.5 finish_reason=length arguments unclosed gpt-4o finish_reason=tool_calls arguments unclosed Two findings, both against what was assumed. Arguments arrive as the raw unclosed fragment on every combination, not as a re-serialised object. And gpt-4o answers a ceiling hit with `tool_calls` -- not a clean stop but a positive "I finished calling the tool" -- in 4 of 4 probes, while usage sits exactly at the ceiling. So on gpt-4o the first signal does not fire at all, and the non-streaming path was down to one: usage reaching a ceiling it also has to have resolved correctly. Parsing strictly first, then repairing, puts it back to two. The repair still happens; only the discarded fact is kept. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
| # Marks the synthetic user message that carries images a transport cannot put in | ||
| # a tool result. Not persisted: the tool result above it already names the file | ||
| # path, so the only thing this message would add to the transcript is a user turn | ||
| # saying "[image]" that the user never sent -- misleading on resume and in | ||
| # session export. Deliberately a different key from ``_recovery_synthetic``: | ||
| # that one marks empty-response recovery scaffolding, and collapsing the two | ||
| # would make either meaning impossible to reason about separately. |
There was a problem hiding this comment.
[nit] This comment block (synthetic image-message rationale, _recovery_synthetic vs. attached-image key) describes _ATTACHED_IMAGE_KEY, which lives in raven/agent/loop/main.py:185 — it has nothing to do with is_hard_tool_failure below it. Looks like leftover residue from the 24da3e4 refactor(agent): move loop helpers out of main.py extraction (same class of issue already flagged on main.py:191 for the think-tag comment). Safe to delete.
The three signals do not share a granularity, and flattening them lost that. `finish_reason` and a usage ceiling hit are facts about the turn, so the call they apply to is the last one -- generation is sequential. An unparseable arguments blob is a fact about one call, wherever it sits. Reducing it to a per-response boolean and then marking `tool_calls[-1]` gets that wrong in a way the previous commit made harmful: with a malformed first call out of three, the third was refused and the first was dispatched. Before truncated calls were refused this only produced an odd message; now it drops a good call and runs the bad one. The observation moves onto `RunMeta` beside the conclusion it feeds, which is what that container was opened for. The provider reports only what it can see -- this call's JSON needed repairing -- and the loop still owns the decision, because the streaming path assembles its tool calls in the loop where a provider has nothing to attach a conclusion to. Splitting the two is what keeps one decision point serving both paths. `RunMeta.__bool__` goes with it. It answered on `truncation` alone, so the second field would have been invisible to every `if run_meta`. A repair with no response-level signal is no longer reported as truncation. That shape is a model writing bad JSON, and calling it a ceiling hit both mis-records the turn in tracing and tells the model to send its content in smaller pieces -- advice for a problem it does not have. The call is refused either way; the stated cause now matches the evidence. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
| # this whole path exists to remove. | ||
| response.max_tokens, response.truncated = flag_truncation( | ||
| getattr(self.provider, "generation", None), | ||
| model=call_model, |
There was a problem hiding this comment.
[blocker] flag_truncation is called here with model=call_model — the model this turn was requested under — but the response it is judging may have come from a different model. chat_with_retry (raven/providers/base.py:741-818) walks [model, *fallback_models] and returns as soon as some model in that chain answers without finish_reason == "error"; LLMResponse has no field recording which model in the chain actually produced it (confirmed — no response.model/served_model anywhere in base.py). fallback_models is a live feature here (wired from self.router.select_model_chain(content) at main.py:2676), so when the primary model errors and a fallback actually serves the turn, send_max_tokens(generation, call_model) inside flag_truncation resolves the primary's ceiling, not the ceiling that request actually carried.
Concretely: if the fallback's real output ceiling is much larger than the primary's (typical — that's often why it's a fallback, e.g. a different vendor), a normal-length completed tool call can have output_tokens >= sent_max_tokens against the wrong (too-small) ceiling → false positive → a complete call gets refused as [truncated]. If the fallback's ceiling is smaller than the primary's, a genuine cut-off is compared against a too-large ceiling → false negative → silently missed truncation, which is the exact incident this PR exists to prevent.
The streaming path (_llm_call_stream) doesn't have this gap since it never goes through the fallback chain. Fix: have chat_with_retry/_chat_attempt_with_retry record which model in the chain actually produced the response (e.g. a field on LLMResponse) and pass that into flag_truncation here instead of call_model.
A repair was re-read as truncation whenever the turn carried a ceiling signal, whichever call it sat on. Position rules that out for every call but the last: a cut leaves no later calls to arrive, so arguments that failed to parse anywhere earlier were written badly by the model, no matter what the turn did at its end. The effect was to tell a model to resend in smaller pieces something that was never too long -- the same misdirected advice this branch keeps removing, one layer further in. Both calls are still refused. Only the stated cause changes, and position is what decides it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…judgements
Detection had grown a layer per question asked of it: whether the cut landed
in tool arguments or in prose, which position in the turn a repair sat at,
which of two causes to name. Each layer refined an inference, and each one
rested on an assumption that nothing had measured -- twice they were wrong and
the fix was another layer.
None of them changed what the model has to do next. A call that may be
incomplete gets resent either way, and whether it was cut or simply malformed
does not alter that. The refinements bought a fraction of a retry and cost a
decision no one can follow.
What is left makes two judgements, and only one of them guesses:
arguments could not be parsed -- local, certain, names its own call
the turn stopped at the ceiling -- upstream said so; the call it puts in
doubt is the last one, which is where a
sequential generator stops
The second is the guess, and it stays coarse on purpose. Refusing a whole call
costs one retry. Dispatching a cut one writes half a file, or turns an append
into an overwrite because the optional `mode` never arrived, and reports
success either way.
The message now separates what is known from what is inferred: the turn
stopped at the limit (fact), so this call may have been cut short (inference).
Stating the inference as fact sent a model to split up a call that was whole.
Tool hints become conditional for the same reason.
The tool schema says what append is for rather than what goes wrong without
it, and drops an invented "40 lines" threshold -- how much fits in one call
depends on a ceiling this file cannot see.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
| low = model_text[:200].lower() | ||
| if "[truncated]" in low: | ||
| return "truncated" | ||
| if "invalid parameters" in low: |
There was a problem hiding this comment.
[nit] failure_class has no case for the [invalid arguments] error text that ToolRegistry.execute now returns for a call whose JSON failed to parse (raven/agent/tools/registry.py, the run_meta.arguments_repaired branch added in commit d27016a, after this file was written in 24da3e4). It falls through to "other", the same bucket as every unrelated uncategorized error.
That defeats the point of this file: the (tool, failure_class) streak key exists so "a truncation, then a schema error, then a missing path" reads as adapting, not stuck (per the PR description). A repeated [invalid arguments] failure interleaved with a genuinely different "other" failure will now be counted as the same failure class, which is the exact conflation the per-class keying was built to avoid.
| if truncated and tool_calls: | ||
| last = tool_calls[-1] | ||
| last.run_meta = replace( | ||
| last.run_meta or RunMeta(), | ||
| truncation=TruncationInfo(at_tokens=sent_max_tokens), | ||
| ) | ||
| logger.warning( |
There was a problem hiding this comment.
[nit] The logger.warning call moved inside if truncated and tool_calls: in the latest commit (4da66a6); before that (d27016a) it fired whenever truncated was True, independent of tool_calls. A turn that hits finish_reason == "length" or the ceiling with no tool calls at all (a plain truncated text reply) still returns truncated=True — which still reaches response.truncated for the llm.truncated trace attribute in semconv.py — but this warning is now silently skipped for that shape, making truncated non-tool-call turns invisible in the logs.
The description had been rewritten around truncation -- how a long file gets split, why a call that runs over is lost. But every caller of write_file reads it, and almost none of them are recovering from a cut-off turn. One scenario had taken over a shared tool's contract, and in the process the original sentence was replaced rather than added to. The description keeps its original wording and gains one line about what the new parameter does. The parameter says when append is useful in terms that hold for appending to a log or resuming a file, with no invented threshold for "long" -- how much fits in one call depends on a ceiling this file cannot see. The fact that only a truncated caller needs -- that an over-long call is discarded whole rather than partly saved, which is why splitting is worth doing at all -- moves into `truncation_hint`, where only that caller sees it. A test now asserts the description carries no truncation vocabulary, so the next scenario that wants something said to every caller has to argue for it. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
… be seen Three findings from review, two of which share a cause: the non-streaming verdict was reached at the wrong moment and from the wrong place. The caller ran `flag_truncation` after `chat_with_retry` returned. But that method is wrapped in `@trace.instrument`, which extracts span attributes in a `finally` and closes the span before the result reaches the caller -- so `llm.truncated` and `llm.max_tokens` read the defaults and were written that way, every time. The commit that added them recorded nothing. The same call site passed `model=call_model`, the model this turn was requested under. `chat_with_retry` walks `[model, *fallback_models]` and `LLMResponse` records nothing about which one answered, so once a fallback serves the turn the ceiling comes from a model that did not produce the response. Too small and a complete call reads as truncated; too large and a real cut is missed. Both go away by deciding inside the method, before it returns: the span is still open and `current_model` is the model that answered. `truncation.py` moves to `providers/` with it -- everything it reads lives at that layer, and the streaming path still calls the same function from the loop, where its tool calls are assembled. Separately, `MiniMaxOAuthProvider.chat_stream` still declared literal defaults for `temperature` and `reasoning_effort`. The loop calls chat_stream with messages/tools/model only, so those literals were the values sent and the parent's fallback to `self.generation` never ran -- a configured temperature of 0.1 went out as 0.7. This is the defect of section 1 of this PR, surviving in the one signature where only `max_tokens` had been fixed, because that was the field this branch was about. The guard test grew the same blind spot; it now checks all three on every shipped `chat_stream`. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
A session that asked the agent to write a terminal game produced 15 tool failures, 11 of them from one root cause, and burned 51% of its output budget re-sending the same payload. The chain: output was capped far below what the model could produce, the resulting truncation was reported as a missing required field, and the model's most reasonable answer to that message was to send the same oversized call again.
This PR cuts the chain at every link, and the last third of it comes from running the thing rather than reading it -- a live reproduction turned up four defects that review had not.
1. The ceiling: from one number to the model's own limit
chat_streamdeclared literal defaults formax_tokens,temperatureandreasoning_effort. The agent loop calls it withmessages/tools/modelonly, so those literals shadowed the configured values on every streaming turn -- requests went out at 4096 / 0.7 / unset. Across the last 10 days of traces, exactly one call out of 1549 produced more than 4096 output tokens, and that one took the non-streaming path.Lifting the shadow exposed the next problem: the value it had been hiding,
agents.defaults.maxTokens, defaulted to 8192 for every model, and onboarding wrote that default into the user's config file -- so by the time it reached the provider it was indistinguishable from a number the user had chosen. No single number works here: claude-sonnet-4 emits 64000 and gpt-4o 16384, while models below 8192 reject the larger value outright.The setting is retired and the ceiling now resolves per model from the same catalogue the context window comes from. An old config carrying the key is ignored rather than rejected (
AgentDefaultsdoes not forbid extras), so no migration is needed and nobody edits a file.resolve_max_output_tokensalways answers with a number -- catalogue first, then a fixed 16384 -- because the caller is about to build a request.This is the shape LiteLLM's own Anthropic path uses, and the shape of the two surveyed implementations without known config-shadowing bugs; the four with such bugs all merge a global constant into user configuration somewhere.
2. Detecting truncation, on both paths
Two judgements, and only one of them guesses.
About the turn --
finish_reason == "length", oroutput_tokensreaching the ceiling we sent. Neither can be rested on alone: measured through openrouter, gpt-4o answers a ceiling hit with"tool_calls"-- a positive claim of success -- in 4 of 4 probes, with usage sitting exactly at the ceiling. The call this puts in doubt is the last one, because generation is sequential.About one call -- its arguments had to be repaired to parse. Local, certain, and it names its own call wherever that sits in the turn.
The second needs no ceiling signal and makes no guess. The first is the guess, and it stays coarse on purpose: refusing a whole call costs one retry, while dispatching a cut one writes half a file, or turns an append into an overwrite because the optional
modenever arrived, and reports success either way.An earlier draft refined that guess further -- whether the cut landed in tool arguments or in prose, which position a repair sat at, which of two causes to name. Each layer rested on an assumption nothing had measured, two were wrong, and none of them changed what the model does next. They are gone.
The ceiling used by the first judgement is the one the request actually carried, resolved through the same function the provider uses. Computed separately the two would drift the moment either grew a bound, and the check would stop firing without ever failing.
Where the judgement happens matters as much as what it decides. The non-streaming one runs inside
chat_with_retry, before it returns:trace.instrumentextracts span attributes in afinallyand closes the span before the caller sees the result, so a verdict reached afterwards is recorded asFalseevery time. That method also walks[model, *fallback_models], andLLMResponserecords nothing about which one answered -- deciding inside is the only place the model that actually served the turn is still known. The streaming path calls the same function from the loop, where its tool calls are assembled.3. What the model is told, and who tells it
The shared message states what happened and why nothing ran: the call was cut at the ceiling, it was therefore not executed, and the missing part could have changed what it does. All three are true of every tool. It avoids the word "saved" on purpose -- the tool most likely to be truncated is the one that saves files, where "not saved" reads as "the write failed" rather than "your arguments never arrived".
What to do about it is the tool's to say, through
Tool.truncation_hint.write_filenames the append mode;execsays to shorten the command, because half a command is not a command and the previous generic advice told it to send one anyway.A call refused for malformed JSON gets a different message and no hint at all: it is asked to resend with well-formed arguments, not to split anything up.
Two earlier wordings were wrong and both were caught by running it. The first said "do not resend the same content" -- the exact opposite of what has to happen, since nothing past the cut survives anywhere, and the model followed it for forty turns. The second put the instruction back into the shared sentence, where it duplicated the tool's own hint and still did not fit
exec.4. Downstream effects of a cut-off turn
<mm:think>) and the orphan closing tag. The paired-block substitution was the last one still matching a literal, which meant a complete<mm:think>...</mm:think>block reached the user as if the reasoning were the answer.5. Somewhere to go
write_fileonly overwrote, so a file it could not finish in one call had no second move: continuing meant re-sending everything already written, which is the same oversized call that just got truncated.mode=appendadds to the end,overwritestays the default. An append with empty content is refused rather than treated as a no-op -- that is precisely what a call cut off before its content field looks like, and the one thing it must never silently become is an overwrite of the part already written.The tool's description keeps its original wording and gains one line about the new parameter. An earlier draft had rewritten it around truncation -- how to split a long file, why an over-long call is lost -- but every caller of
write_filereads that description and almost none are recovering from a cut-off turn. What only a truncated caller needs lives intruncation_hint, where only that caller sees it. A test asserts the description carries no truncation vocabulary, so the next scenario wanting something said to every caller has to argue for it.6. A truncated call is not dispatched at all
This was originally left out of scope as "a call whose fields all validate still executes, writing a half-file". Running the branch showed the boundary is worse than that description: it is not only about incomplete content.
modeis optional onwrite_file. A cut landing before it arrives falls back tooverwrite, which silently replaces everything an earlier append had written -- and reports success. The model asked to append, was told it worked, and the file is now the last chunk alone. Validation cannot see this: the call is well-formed. Only the fact that it was cut says the intent behind it is incomplete.So the refusal moves ahead of validation and is keyed on truncation alone. Being wrong costs one retry; being right avoids a silent overwrite.
Also fixed on the way
max_tokens: int = 4096in their own signatures. Harmless while the resolved value was always a number; a crash once it could beNone, sincechat_with_retrypasses the resolved value explicitly and Azure took thatNoneintomax(1, max_tokens).max_tokensfallback in this branch. The field reading behind that was right, the conclusion was not: six catalogue rows carry nomax_input_tokens, and dropping them to the documented 65536 default over-estimates gemma-2's real 8192 window eightfold. Restored, with the reason stated as the bound it is -- a model's window is never smaller than what it is allowed to emit.Type
Verification
The one failure is
test_cli_theme::test_bold_accent_renders_styled_not_bare, pre-existing and unrelated: it fails identically on a clean base, and passes alone but fails when its file runs as a whole -- intra-file state leakage that predates this branch.Live reproduction
The branch was run against a real backend (openrouter/anthropic/claude-sonnet-4-5) with the ceiling pinned low enough to force truncation, on the same task the original incident used. Same task, same ceiling, before and after:
The run before the fixes is the incident in miniature: forty turns of
write_file({"path": "snake.py"})with the content withheld, the model announcing at turn 5 that it would supply a field it had never omitted. The session record confirms every one of those forty calls carried exactly twenty characters of arguments, while the first eight were billed 1200 output tokens each -- the content was written, charged for, and dropped before it reached anything that stores it.After: turn 1 is cut off, turn 2 says "I'll write this in two parts" and lands 2934 bytes with
mode=overwrite, turn 3 appends 2935 more, turn 4 reports done.Four defects came out of that run and would not have come out of review: the non-streaming path had no detection at all; three providers still hardcoded the old ceiling, one of them into a crash; the message told the model to withhold exactly what it needed to resend; and the generic advice did not fit tools with no smaller form.
It also exercised a design decision that had only been argued on paper. Turn 2 emitted two calls -- the first landed 2934 bytes, the second was cut off. Only the last call of a turn is marked, so the first executed normally. Marking every call would have rejected a write that succeeded.
Regression detection
This class of defect does not turn a suite red; it makes the suite unable to see the behaviour. For each change the implementation was reverted while keeping its tests, and the new cases were confirmed to fail before restoring it:
Two of those checks failed the first time and are worth naming, because both are ways a test can look thorough and verify nothing:
AgentLoopwith no token callback is what actually fails.Assertions land on outgoing request bodies, returned messages, the serialized assistant message, and file contents on disk -- not on whether a call happened.
What an outside review found that self-checking did not
An automated review on the branch raised seven findings and all seven held up. Five share a shape: a change made here, its counterpart left alone.
MiniMaxOAuthProvider.chat_streamstill declared literal defaults fortemperatureandreasoning_effort-- the same shadowing this PR opens with, in the one signature where onlymax_tokenshad been fixed, because that was the field this branch was about. A configured 0.1 went out as 0.7. The guard test had the same blind spot and now checks all three.failure_classdid not know about[invalid arguments], a message this branch introduced after that classifier was written, so it shared theotherbucket with every uncategorised error -- undoing the per-class streak keying for the newest failure shape.The other two are the span timing and the fallback ceiling described in section 2 -- both cases of adding code to a call chain without knowing what the rest of it does.
None of these were reachable by checking the diff, because the diff is where the change is and the damage was next to it.
Problems surfaced while building this
_SENTINELdoes not resolve across modules. It is a class attribute onbase.py, so naming it directly in a subclass signature raisesNameErrorat import. Qualified asLLMProvider._SENTINEL.Three existing tests broke, and the breakage was real. A subclass that never runs the base
__init__has noself.generation, and the basechat_streamfallback exists precisely for such thin implementations. Crashing there would be worse than the settings it restores, sogenerationis read defensively.The terminal chunk was being dropped.
finish_reasonrides only on the chunk carrying no content, no tool calls and no usage, which the emptiness check skipped. One existing assertion was split rather than loosened.The truncation flag was reaching the model.
to_openai_tool_callserializesargumentsinto the assistant message, and the loop does that before the registry strips anything -- so a marker stored there was already fixed into history. Now covered by a test asserting on the serialized payload.One test's premise stopped existing.
test_unknown_ceiling_does_not_flag_truncationpinned that signal 2 sits out when no ceiling is known. Every model now resolves to one, so the case was rewritten to pin what replaced it.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Longer completions, higher per-turn cost. Streaming turns now honour temperature and reasoning effort, and every turn asks for the model's real output ceiling instead of 4096. On claude-sonnet-4 that is 64000 rather than 8192. This reaches installs that never configured anything.
agents.defaults.maxTokensis gone. A config still carrying it is ignored, not rejected, so nothing breaks and no file needs editing -- but a user who set a low value to cap cost will find it no longer has an effect. There is no replacement setting: the ceiling is a per-model fact now. Models the catalogue does not know keep 16384.write_filegained a parameter. A tool schema is a contract the model reads; this one is additive,modeis optional and defaults to the existing behaviour.A truncated tool call is no longer executed. Previously it ran whenever its required fields happened to have arrived. Callers that relied on partial execution -- there are none in this tree -- would see the call refused instead, with the model asked to resend it in pieces. The cost when the refusal is wrong is one retry.
Error text the model reads changed. Anything parsing tool errors by string match will see different wording on truncated calls.
registry.executegained one keyword-only parameter with a default, so its four other call sites are unaffected. Truncation metadata rides beside the arguments rather than inside them, so it reaches neither the tool nor the assistant message.llm.truncated/llm.max_tokensare stamped on the span only when truncation is detected.Rollback is a straight revert of the branch; nothing persists state and no data migration is involved.
Related Issues
N/A