fix(session): drop empty tool_calls in repair_message_sequence (#77921) - #77944
fix(session): drop empty tool_calls in repair_message_sequence (#77921)#77944webtecnica wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
|
Closing in favor of #78063 — you're right that it has grown into a superset (Fix 2 api_content sidecar + Fix 3 orphan tool_call_id detection are genuinely beyond what #77944 covers, and the extra regression tests are valuable). Consolidating avoids parallel implementations of the same fix. Thanks for the fair flag on Fix 1 and for crediting #77944 as the original for the |
I will definitely mention you in the commit when I code again :) Just give me your github email and I will put you as co-author |
But if I'm unable to mention you before the merge, the maintainers will do it. |
Follow-up to NousResearch#58755 / NousResearch#59110 — still reproducing in v0.19.1 (NousResearch#77921). PR NousResearch#59110 fixed the symptom at sanitize_api_messages, the final pre-API chokepoint: it strips tool_calls:[]/None on the per-call wire copy. But the consecutive-assistant merge in repair_message_sequence has its own gap: when the surviving turn already carried a stale tool_calls:[]/None and the turn being merged in has no real tool_calls either, neither the `if new_calls` nor `elif prev_calls` branch fires, so prev["tool_calls"] is left untouched and the stale falsy value survives the merge verbatim into the repaired (and persisted) messages list — the actual generative site the issue's diagnosis pointed at, not just the known exit chokepoint. Add an else branch that drops the key entirely in that case. This closes the leak at its origin instead of relying solely on one known downstream consumer to clean it up, so any other reader of the repaired list (now or in the future) never sees the poisoned value in the first place. Non-destructive to persisted history: a falsy tool_calls is already normalized to NULL on every DB write (_insert_message_rows), so dropping the key here changes nothing about what state.db ends up storing. Same one-line fix (`else: prev.pop("tool_calls", None)`) was independently implemented first in NousResearch#77944 by webtecnica — credited as co-author below. Co-Authored-By: webtecnica <webtecnica@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…esearch#69280) + clear_session_cache on reset - repair_message_sequence: else branch pops stale tool_calls: [] on merged assistant turns (PR NousResearch#77944 upstream, not merged yet) - run_agent.py: call engine.clear_session_cache() on /new reset Local production patches, reaplicados no sync v2026.8.3.
Rebased onto current main to drop the empty-tool_calls fix (already on main via NousResearch#86654, cherry-picked from NousResearch#77944 with @webtecnica's authorship). This PR now carries only the two fixes unique to it: 1. A pre-existing api_content sidecar left stale on the consecutive- assistant merge. The sidecar takes priority over content at API-build time, so a merge could silently discard its own freshly concatenated content on the next call. Only dropped when the merge actually changes the resulting value (wz-heng, NousResearch#78063 review) -- content_rewritten compares before/after value, not just whether an assignment branch fired, so a falsy new_content (e.g. "") that strips to nothing no longer trips a spurious sidecar drop. 2. sanitize_api_messages never flagged a tool result with a missing/ empty tool_call_id -- its orphan-detection set only ever collected truthy ids, so an unpaired result with no id passed the final chokepoint untouched. Addresses teknium1's rebase request and wz-heng's review findings on NousResearch#78063.
3 participants
Summary
Fixes #77921 — the empty
tool_calls: []failure (HTTP 400 "Invalid 'messages[N].tool_calls': empty array" on strict providers like DeepSeek v4) still reproduces in v0.19.1 despite the #59110 chokepoint fix, on long sessions (~370K–395K tokens, always around messages[465]).Root Cause
The #59110 fix (
sanitize_api_messages) drops emptytool_callson the per-call wire copy — verified working. Butrepair_message_sequence's Pass-0 consecutive-assistant merge preserves a pre-existingtool_calls: []on the surviving/live message: its union branches only ever set non-empty lists or leave the key untouched. Because the chokepoint sanitizer only patches the wire copy, the[]lives on in the repaired live/persisted trajectory — the same list returned to callers, persisted to state.db, and replayed on the next turn. Every downstream consumer (gateway/WebUI transcript, session resume, subagents, cron) sees the poisoned shape, which is how #58755 keeps reproducing on long sessions. DB round-trips cannot be the source (append_messagestores empty lists as NULL) — the empty array is created in memory during repair.Change
agent/agent_runtime_helpers.py— inrepair_message_sequence's consecutive-assistant merge, theelsebranch (neither turn carries tool calls) now pops a staletool_calls: []from the surviving message. Dropping the key at the source is non-destructive (an empty array carries no information) and fixes the live/persisted trajectory that the chokepoint sanitizer never sees.tests/run_agent/test_message_sequence_repair.py— new regressiontest_repair_drops_stale_empty_tool_calls_on_merged_assistant: merged consecutive-assistant turn with a staletool_calls: []ends without the key. Verified failing before the fix, passing after.Verification
pytest tests/run_agent/test_message_sequence_repair.py→ 15 passed.Closes #77921