…tion (#850) * fix(llm): drop truncated tool calls instead of poisoning the conversation When a model runs out of output tokens partway through a tool call, the streamed `arguments` arrive as a fragment such as `{"filename": "1787784579_..._topic` that is not valid JSON. The streaming accumulator concatenated the fragments with no validation, handed the result to the tool executor, and appended it verbatim to the assistant message. Providers re-parse every tool call in the message history on each request, so one such fragment made every subsequent turn in that conversation fail with `OpenAIException - Unterminated string starting at: line 1 column 73` -- a 400 that no retry could clear, because the poison was in the history rather than in the request. The user only saw "the model provider rejected this request as invalid; retrying will not help", which was accurate but described a self-inflicted wound. Tool calls whose arguments do not parse as JSON are now dropped at the point they are accumulated, in both the streaming and non-streaming paths. When other well-formed calls survive the turn continues with those, so one bad call does not cost the user the calls that arrived intact. When nothing usable is left the turn fails with a new `LLMMalformedToolCallError`, which -- unlike the provider rejection it replaces -- tells the user the failure is transient and worth retrying, and distinguishes an output-limit truncation (`finish_reason=length`) from a model that simply emitted bad JSON. `_raise_llm_domain_error` now passes an already-classified `LLMError` through untouched; the guard raises from inside the block that wraps the provider call, and without the pass-through its precise message was demoted to the generic "the LLM service encountered an error". Empty arguments still count as valid: models legitimately emit `""` for no-argument tools. * docs: changelog entry for PR #850 * Address review: propagate the failure past partial narration Review findings on #850. Streamed narration must not hide a dropped tool call. Both streaming consumers suppress a mid-stream error once any text has been streamed -- right for a transport fault, wrong here: the model announced work, the call was dropped, and the narration was then saved as a successful final answer that silently skipped it. `AgenticLoop._call_llm_streaming` now re-raises `LLMMalformedToolCallError` even after partial text, closing the open token stream first so the UI does not leave a live cursor behind the error, and `ToolsModeRunner.run_streaming` sends the `malformed_tool_call` error frame regardless of accumulated content. The two `call_with_rag*` domain-error passthrough tuples enumerated the LLM error subclasses by hand, so the new error was not in them and fell through to the fallback retry -- which re-sends the original messages and discards the already-injected RAG context. They now catch `LLMError`, which is exactly the set they listed plus any future member; `LLMAuthenticationError` and `DataSourcePermissionError` sit outside that hierarchy and stay listed. Tool names reaching the log are sanitized: they come from model output, so a name carrying a newline could otherwise forge a log line (CodeQL). `tool_call_function_field` is no longer annotated `-> str`: `arguments` can arrive already decoded as a dict or list, so the annotation was wrong and misleading to callers. Adds `test/pr-validation/test_pr850_truncated_tool_call_guard.sh` per the repository's per-PR validation convention -- 9 end-to-end checks including one that serializes the surviving tool calls exactly as the agentic loop writes them to history and re-parses each one, which is the assertion that would have caught the original bug. * Address review: catch pre-argument truncation, stop repairing cut-off values Second round of review findings on #850. A tool call cut off *before* its first argument delta arrives has `arguments == ""`, which parses fine as "no arguments" and would execute with `{}`. For a tool whose parameters are all optional that is not a failure the user sees -- it is the wrong action performed silently. Parseability alone cannot see this, so `partition_tool_calls_by_json_validity` now takes `truncated` (set from `finish_reason == "length"`) and drops the last call when its arguments are empty. Only the last: earlier calls in the same response completed before the limit was hit, so a genuine no-argument call among them is still honoured. `_try_repair_json` in the tool executor closed an open string value, so `{"e": "355/113` became `{"e": "355/113"}`. That is safe only when the cut-off value happens to still be meaningful, and for the failure that motivated this PR it was not: the production fragment `{"filename": "1787784579_..._topic` repaired into a different, valid-looking filename, and the tool would have executed confidently against the wrong file. The two layers also disagreed about what a truncated call means once the guard landed upstream. Both are settled the same way -- a repair may complete the shape of an object, never the content of a value -- so the string-closing branch is gone and brace balancing stays. The RAG passthrough finding from this round was already addressed in c5814da, which widened both tuples to `LLMError`. Also closes the gaps called out in the notes: a unit test now asserts that every surviving tool call still parses after the round-trip through `_to_tool_call_dict` that builds the assistant message (the assertion the original bug would have failed); the non-streaming path gains sibling-survives and finish_reason cases; and `docs/developer/error-flow-diagram.md` documents the new error type, its retryable semantics, and the boundary between the guard and the executor's repair. * Address review: repair brace-only damage, announce partial drops, never save an empty turn Third round of review findings on #850. Missing enclosing braces is a shape some models emit routinely -- `"q": "hi"` rather than `{"q": "hi"}` -- not a truncation, and it was repaired and executed long before this guard existed. Failing the turn over it was a regression the guard introduced. `partition_tool_calls_by_json_validity` now runs one structural repair before declaring a call malformed, and writes the repaired string back onto the call: that write-back is what keeps the history copy parseable on every later request, which repairing downstream in the executor never did. `repair_structural_json` in the LLM layer is now the single definition of what a repair may do, and the executor's `_try_repair_json` delegates to it, so the two layers cannot drift apart. The dependency also runs the right way -- the LLM layer no longer needs anything from the application layer. A partial drop was invisible. The turn continues with the calls that parsed, so without an explicit signal neither the user nor the model learns one was discarded, and the user sees an answer built on less work than the model intended. `LLMResponse.dropped_tool_calls` carries the names, both mode runners publish a `warning` frame from it (the frontend renders this frame -- I checked `websocketHandlers.js`, having previously claimed otherwise), and a `malformed_tool_call` metric counts the drop. Neither mode saved a failed turn correctly. Tools mode returned without persisting the narration the user had watched stream in, so it vanished on reload and the turn was saved with no assistant reply; it is now persisted and flagged `incomplete`. Agent mode re-raised without closing the turn, leaving the same hole the interrupted-turn path exists to prevent; it now closes with a terminal message following that contract. `finish_reason` is allow-listed against the known OpenAI vocabulary before it reaches a log line or a span attribute -- it is provider text, and the tool names beside it were already sanitized. The truncation copy led with "starting a new conversation", which shortens the input and cannot relieve an output-token limit. It now leads with "asking for one thing at a time" and says why the other advice does not apply. Findings 3, 6 and 7 from this round were already addressed in 23894bb. * Address scanner findings: sanitize the model id, drop a redundant import CodeQL flagged the model id as the one value still interpolated into the malformed-tool-call log line unsanitized -- it is request-supplied, and the tool names beside it were already sanitized. Also removes a function-level import of json that the module already imports. * Sanitize the dropped names at the interpolation point CodeQL kept flagging the log line after the model id was sanitized. The repo already models sanitize_for_logging's return value as a log-injection sanitizer (.github/codeql/extensions.yml), but that model tracks a direct call rather than values laundered through a list first, so the joined names are now sanitized where they are interpolated. * Address review: no repair on a truncated call, no {} fallback, shared warning copy Fourth round of review findings on #850. Brace-balancing a *mid-object* truncation is not a safe repair. `{"path": "/data", "recursive": true` closes into valid JSON whose remaining keys were silently dropped: it executes, and it is written to history, looking entirely well-formed. That is the same silent-wrong-answer the string-closing branch produced, reached a different way. The structural repair is now skipped for the last call of a truncated response -- completing the shape is only honest when nothing is known to be missing. The same shape is still repaired when nothing was truncated, since then it is only a sloppy envelope. The executor's `parsed_args = {}` fallback contradicted the policy the rest of this change is built on: for a tool whose parameters are all optional it succeeds and returns a plausible result for a request the user never made. A failed repair now raises `ToolError`, which `execute_single_tool` turns into a failed `ToolResult`, so the model is told and can reissue. Agent mode discarded the narration the user watched stream in while tools mode kept it. It is now persisted as the same display-only `agent_intermediate` row a completed step writes, flagged `incomplete`. The dropped-call warning was duplicated verbatim in two modules and its copy was wrong three ways: it always blamed truncation though the path also fires for unparseable arguments, it did not pluralize, and it claimed "the other tool calls ran normally" before `execute_multiple_tools` had run. One implementation now lives beside the guard, branches on the real cause (carried on the response as `dropped_tool_calls_truncated`), pluralizes, and says the step is continuing rather than that it finished. The guard also moved out of `models.py` -- documented as holding data models -- into `tool_call_guard.py`. It is policy, not a data model. Tests: the truncated-vs-not repair pair, the executor raising instead of running with `{}`, agent-mode warning and narration persistence, the warning copy in all four cause/number combinations, and the log-safety values. The `tool_calls is None` normalization is now asserted on a path that returns (a provider list holding nothing usable) rather than one that raises. Also removes a duplicated changelog bullet that had landed in the #849 section. * Close the review nits: warn on every path, name the real cause, widen cut-off The partial-drop warning was published from the streaming paths only, so with streaming off a dropped sibling call was still silent -- the exact failure the warning exists to prevent, just on the other half of the paths. `publish_dropped_call_warning` is now one helper in ``utilities/dropped_calls.py``, called from every path that produces an ``LLMResponse``: streaming and non-streaming, tools mode and agent mode. Agent mode's turn-closing text always said the call "was cut off", but the error also fires on plain invalid JSON, and that text is written into history and read back later. ``LLMMalformedToolCallError`` now carries ``truncated`` and the persisted copy branches on it. Truncation detection for the *repair policy* widened from ``finish_reason == "length"`` to any unclean finish: a ``content_filter`` stop or an unrecognized provider string can also end a response mid-object, and the last call is then just as suspect. A missing ``finish_reason`` still counts as clean, because providers routinely omit it on the chunks Atlas accumulates and treating that as truncation would drop legitimate no-argument calls across the board. The user-facing "ran out of room" wording stays keyed to the output limit specifically -- a content filter cutting a response off is not the model running out of room, and the copy should not claim it was. * Close the second round of nits: forward truncated, key repair on shape, dedupe ``safe_call_llm_with_tools`` rebuilt ``LLMMalformedToolCallError`` without forwarding ``truncated``, so every failure reaching the caller through that path reported ``truncated=False``. That flag decides the turn-closing text agent mode *persists into history*, so the wrong cause was recorded permanently. Forwarded, and the two rebuild branches are now separate rather than sharing one call that could only pass what both accept. The repair now splits on shape rather than on ``finish_reason`` alone. Text that opens with a brace and never closes it is the cut-off signature, so supplying the closing brace requires positive evidence the response finished; text missing the *opening* brace is a sloppy envelope around a complete intent and is always repaired, because a cut lands at the end and not the start. This is what lets an absent ``finish_reason`` -- routinely omitted on the chunks Atlas accumulates -- keep the envelope repair without allowing a truncated object to be completed with keys the model never sent. Earlier calls in a response are repaired unconditionally, since a cut can only have reached the last one. ``partition_tool_calls_by_json_validity`` therefore takes ``finish_reason`` directly rather than a precomputed boolean: the three states it distinguishes (clean, cut off, absent) do not collapse into two. The ~20-line guard block and the ``finish_reason == "length"`` rule behind the user-facing copy were duplicated across the streaming and non-streaming callers. Both now go through ``_guard_tool_calls()``.

Uh oh!

remove the special message for agent mode, make agent the default. #849

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions