fix(agent): drop stale empty tool_calls on repair_message_sequence merge (#77921)

[JoaoMarcos44]

Summary

Follow-up to #58755 / #59110, tracked in #77921 (empty tool_calls array still causing HTTP 400 from DeepSeek v4 in v0.19.1 — 3 reproductions between 2026-08-01 and 2026-08-03, session permanently stuck after the first hit).

This PR fixes three stale-field bugs found in agent/agent_runtime_helpers.py, all sharing the same shape: a merge or chokepoint rewrites a message but leaves a derived field pointing at pre-rewrite state, and that stale field later wins over the fresh one.

#	Function	Stale field	Symptom
1	repair_message_sequence (consecutive-assistant merge)	tool_calls: []/None	HTTP 400 "empty array" from DeepSeek v4
2	repair_message_sequence (same merge)	api_content sidecar	Merge's freshly-concatenated content silently discarded on next call
3	sanitize_api_messages (final pre-API chokepoint)	orphan-detection set	Tool result with missing/empty tool_call_id reaches the provider unfiltered

---

Fix 1 — stale tool_calls survives the merge (#77921)

Root cause: #59110 fixed the symptom at sanitize_api_messages by stripping tool_calls: []/None on the per-call wire copy. But repair_message_sequence's consecutive-assistant merge has its own gap at the origin: when the surviving turn already carries a stale tool_calls: []/None and the turn being merged in has no real tool_calls either, both prev_calls and new_calls end up empty, so neither if new_calls nor elif prev_calls fires — prev["tool_calls"] is left untouched and the stale falsy value survives the merge into the repaired (and persisted) messages list.

Fix: added an else branch — when neither side carries real tool_calls, drop the key entirely. Non-destructive to persisted history: a falsy tool_calls is already normalized to NULL on every DB write (_insert_message_rows).

%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%%
graph TD
    A[🔒 Consecutive Assistant Turns] --> B{⚡ Merge in repair_message_sequence}
    B -->|prev has real tool_calls| C[✅ Union: prev + new]
    B -->|new has real tool_calls, prev empty| D[✅ Union: new only]
    B -->|BOTH empty/None — the gap| E[🐛 Before: stale tool_calls left as-is]
    E --> F[🚀 Wire: tool_calls sent empty]
    F --> G[❌ DeepSeek v4: HTTP 400 empty array rejected]
    B -->|BOTH empty/None — fixed| H[🔧 After: prev.pop tool_calls]
    H --> I[🚀 Wire: key absent — accepted]
    style E fill:#ff007f,stroke:#ff0038
    style G fill:#ff007f,stroke:#ff0038
    style H fill:#00f0ff,stroke:#00f0ff,color:#0a0a16
    style I fill:#00f0ff,stroke:#00f0ff,color:#0a0a16

Loading

---

Fix 2 — stale api_content sidecar survives the same merge

Root cause: the same consecutive-assistant merge concatenates content onto the surviving turn, but a pre-existing api_content sidecar on that turn was left untouched. That sidecar is the exact bytes previously sent to the API when they diverge from the clean stored content (stamped by _flush_messages_to_session_db whenever raw content diverges from what sanitize_context would produce — e.g. echoed <memory-context> blocks). It takes priority over content at API-build time for role assistant (conversation_loop's api_messages build), so a merge could silently discard its own freshly-concatenated content and replay pre-merge bytes on the next call.

Fix: drop_stale_api_content(prev) on every consecutive-assistant merge, mirroring what the consecutive-user merge (Pass 2, same file) already does.

Fix 3 — sanitize_api_messages never flags an unpaired tool result with no id

Root cause: the orphan-detection set (result_call_ids) only ever collects truthy tool_call_id values, so a tool result with a missing/empty id is never added to it — and therefore can never land in the orphaned-ids set-difference either. The message passes through the "final chokepoint" completely unfiltered and can reach the provider with no tool_call_id at all, a schema violation on strict OpenAI-compatible providers.

Fix: drop such messages unconditionally, mirroring the guard repair_message_sequence's Pass 1 already applies (if tc_id and tc_id in known_tool_ids).

%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%%
graph TD
    A[🩸 Consecutive Assistant Merge] -->|content rewritten| B{🔥 prev carries stale api_content sidecar?}
    B -->|before fix| C[⚔️ Sidecar wins at API-build time]
    C --> D[❌ Stale pre-merge bytes sent — new content silently lost]
    B -->|fixed| E[🔥 drop_stale_api_content clears sidecar on merge]
    E --> F[⚔️ Freshly merged content reaches the wire]

    G[🩸 sanitize_api_messages orphan sweep] -->|tool_call_id empty/missing| H{🔥 truthy-id-only check}
    H -->|before fix| I[⚔️ Never added to result_call_ids, never flagged orphaned]
    I --> J[❌ Unpaired tool result reaches provider — schema violation]
    H -->|fixed| K[🔥 Dropped unconditionally, regardless of id truthiness]
    K --> L[⚔️ Only properly-paired results survive]

    style C fill:#8b0000,stroke:#ff0038
    style D fill:#8b0000,stroke:#ff0038
    style I fill:#8b0000,stroke:#ff0038
    style J fill:#8b0000,stroke:#ff0038
    style E fill:#ff0038,stroke:#ff0038,color:#0a0204
    style F fill:#ff0038,stroke:#ff0038,color:#0a0204
    style K fill:#ff0038,stroke:#ff0038,color:#0a0204
    style L fill:#ff0038,stroke:#ff0038,color:#0a0204

Loading

---

Changes

• agent/agent_runtime_helpers.py
  • repair_message_sequence() — drop the tool_calls key on the surviving turn when the merge has nothing real to union in (Fix 1); drop the stale api_content sidecar on every consecutive-assistant merge (Fix 2).
  • sanitize_api_messages() — drop tool results with a missing/empty tool_call_id unconditionally (Fix 3).
• tests/run_agent/test_message_sequence_repair.py — 6 new regression tests: stale [] dropped, stale None dropped, real tool_calls preserved (negative control), union from later turn still works (negative control), stale api_content sidecar dropped on merge, unpaired tool result dropped by the sanitizer.

Validation

Scenario	Before	After
Merge: prev tool_calls: [], new turn has none	[] preserved	key dropped
Merge: prev tool_calls: None, new turn has none	None preserved	key dropped
Merge: prev has real tool_calls, new turn has none	kept (unaffected)	kept (unaffected)
Merge: prev has none, new turn has real tool_calls	unioned (unaffected)	unioned (unaffected)
Merge: prev has stale api_content sidecar	sidecar survives, overrides merged content	sidecar dropped, merged content wins
Sanitizer: tool result with missing/empty tool_call_id	passes through unfiltered	dropped

Suite	Result
tests/run_agent/test_message_sequence_repair.py	20/20 passed
tests/hermes_state/test_restore_alternation_repair.py	3/3 passed
tests/run_agent/test_agent_guardrails.py + test_session_meta_filtering.py	32/32 passed
ruff check	clean

Infographic :

Test plan

• pytest tests/run_agent/test_message_sequence_repair.py -q — 20 passed
• pytest tests/hermes_state/test_restore_alternation_repair.py -q — 3 passed
• pytest tests/run_agent/test_agent_guardrails.py tests/run_agent/test_session_meta_filtering.py -q — 32 passed
• ruff check agent/agent_runtime_helpers.py tests/run_agent/test_message_sequence_repair.py — clean
• Maintainer: confirm against a real DeepSeek v4 session that previously hit Still reproducing in v0.19.1: empty tool_calls after repair_message_sequence (follow-up to #58755) #77921's repro shape

Related

• Closes Still reproducing in v0.19.1: empty tool_calls after repair_message_sequence (follow-up to #58755) #77921
• Closes sanitize_api_messages does not catch tool messages with missing/empty tool_call_id #78071 (folded into Fix 3)

A note on the AI-triage "duplicate" flag

An automated triage comment flagged this PR as a duplicate of #77944 — accurate for Fix 1: both PRs land on the identical one-line change (else: prev.pop("tool_calls", None)) for #77921. @webtecnica opened #77944 first; credit for Fix 1 goes there.

Fix 2 (api_content sidecar) and Fix 3 (sanitize_api_messages missing-id gap) are not in #77944 — found and implemented independently here while auditing the same function for other instances of the same failure shape, with their own regression tests. See the full discussion for the detailed comparison.

[alt-glitch]

This was generated by AI during triage.

Duplicate of #77944, the earlier open implementation. Diff verification found identical production code: both add else: prev.pop("tool_calls", None) at the same consecutive-assistant merge in repair_message_sequence for #77921. This PR has additional tests that could be carried into #77944.

[JoaoMarcos44]

@alt-glitch That triage was accurate at the time — at that point this PR and #77944 did carry identical production code (the same else: prev.pop("tool_calls", None) for #77921).

Since then this PR grew beyond that single fix. While auditing repair_message_sequence / sanitize_api_messages for other unaudited corruption paths of the same shape, two more real gaps turned up and are now folded in (commit ebb02ed9d):

1. api_content sidecar left stale on the consecutive-assistant merge. The merge rewrites content on the surviving turn but didn't drop a pre-existing api_content sidecar, which takes priority over content at API-build time — a merge could silently discard its own freshly-concatenated content on the next call. fix(session): drop empty tool_calls in repair_message_sequence (#77921) #77944 doesn't have this.
2. sanitize_api_messages never flagged a tool result with a missing/empty tool_call_id — its orphan-detection set only ever collects truthy ids, so an unpaired result with no id passed the "final chokepoint" untouched. fix(session): drop empty tool_calls in repair_message_sequence (#77921) #77944 doesn't have this either.

Current state: 3 distinct fixes, 6 new regression tests (20/20 total passing vs #77944's 15), ruff clean. This PR is no longer a duplicate of #77944 — it's a superset. Full breakdown in the updated PR description above.

Happy to have #77944 closed in favor of this one, or to have the maintainer merge whichever they prefer — just flagging that the "identical diff" basis for the duplicate call no longer holds.

[JoaoMarcos44]

@alt-glitch Fair flag for Fix 1 — that one-line change (else: prev.pop("tool_calls", None)) is identical to #77944's, and @webtecnica opened that PR first. Credit for Fix 1 goes to #77944.

Fix 2 (api_content sidecar dropped stale on the same merge) and Fix 3 (sanitize_api_messages dropping tool results with a missing/empty tool_call_id) aren't in #77944 — added here after auditing the same function for other instances of the same failure shape, each with its own regression test. PR description updated to reflect this.

[wz-heng]

Review finding: drop_stale_api_content(prev) is currently unconditional, but the merge does not always rewrite prev["content"]. For example, if the later assistant turn has content=None (or either side has multimodal/list content), the content branches intentionally leave the surviving content unchanged. In that shape an existing assistant api_content sidecar is still the exact previously-sent payload; dropping it changes replay bytes and breaks the prompt-cache invariant without fixing stale content.

Please only drop the sidecar when the surviving content actually changes (e.g. compare before/after), and add a regression test with prev={"content": "clean", "api_content": "wire bytes"} plus a consecutive assistant whose content is None / list, asserting that api_content remains. The existing changed-string test should continue to assert the opposite.

[alt-glitch]

This was generated by AI during triage.

Re-triage correction: #77944 is closed, and this PR now contains its empty tool_calls repair plus distinct api_content and missing-tool_call_id fixes. It is related rather than a duplicate. A maintainer should also resolve the live concern that unconditional api_content removal can alter replay/cache bytes when the merge did not rewrite content.

[JoaoMarcos44]

Review finding: drop_stale_api_content(prev) is currently unconditional, but the merge does not always rewrite prev["content"]. For example, if the later assistant turn has content=None (or either side has multimodal/list content), the content branches intentionally leave the surviving content unchanged. In that shape an existing assistant api_content sidecar is still the exact previously-sent payload; dropping it changes replay bytes and breaks the prompt-cache invariant without fixing stale content.

Please only drop the sidecar when the surviving content actually changes (e.g. compare before/after), and add a regression test with prev={"content": "clean", "api_content": "wire bytes"} plus a consecutive assistant whose content is None / list, asserting that api_content remains. The existing changed-string test should continue to assert the opposite.

TYSM , i´m solve this

[JoaoMarcos44]

@wz-heng— fixed in de61802. Tracked whether a branch above actually reassigned prev["content"] (content_rewritten flag) and only call drop_stale_api_content(prev) when it did. Added the two negative controls you asked for:

• test_repair_merge_preserves_api_content_sidecar_when_content_unchanged — prev={"content": "clean", "api_content": "wire bytes"}, later turn content=None, asserts api_content survives.
• test_repair_merge_preserves_api_content_sidecar_with_multimodal_content — same, later turn content is a list (multimodal).

The existing changed-string test (test_repair_merge_drops_stale_api_content_sidecar_on_surviving_turn) still asserts the sidecar gets dropped when content actually changes. 22/22 passing, ruff clean.

[wz-heng]

Thanks — the None and multimodal controls cover the two cases I called out. One remaining variant: content_rewritten currently means “entered an assignment branch”, not “the value changed”. With prev={"content": "clean", "api_content": "wire bytes"} and a later assistant { "content": "" }, joined is still "clean", but the flag is true and the sidecar is dropped despite identical content.

Could you make the decision from the before/after value (or leave the flag false when the joined value equals prev_content) and add this empty-string negative control? Then the sidecar is invalidated exactly when its corresponding content changes.

[teknium1]

Status update: Fix 1 (else: prev.pop("tool_calls", None) on the repair merge) is now on main via #86654, cherry-picked from #77944 with @webtecnica's authorship per the credit you already established in this thread. Fixes 2 (api_content sidecar) and 3 (orphan tool_call_id results) remain unique to this PR — please rebase onto current main dropping Fix 1, and address @wz-heng's open finding that the api_content drop must be conditional on the merge actually rewriting prev["content"] (unconditional removal can alter replay/cache bytes when the content branches left it unchanged). Then it's ready for re-review.

[alt-glitch]

This was generated by AI during triage.

Duplicate of #86654, whose merged three-chokepoint repair covers this PR's empty tool_calls, stale sidecar, and missing tool_call_id protections.

[JoaoMarcos44]

@teknium1 done — pushed a209233697.

content_rewritten now compares the value before/after the merge instead of just checking whether the assignment branch fired. Both merge branches (isinstance str/str and the not prev_content fallback) set the flag from new_value != prev_content, so a falsy new_content (e.g. "") that strips away and leaves joined == prev_content no longer trips a sidecar drop.

Added @wz-heng's requested negative control: test_repair_merge_preserves_api_content_sidecar_when_content_unchanged_by_empty_string — later turn {"content": ""}, asserts api_content survives even though the assignment branch still runs.

23/23 tests passing, ruff clean. Ready for re-review.

[JoaoMarcos44]

@teknium1 done — rebased and force-pushed (1208c2a).

• Dropped Fix 1 entirely (the empty-tool_calls else-pop). Branch is now rebased directly onto current main, which already has it via fix(agent): close the empty tool_calls 400 class at all three chokepoints (#83312, #77921) #86654.
• PR now carries only the two fixes unique to it: the api_content sidecar drop and the orphan tool_call_id sweep in sanitize_api_messages.
• @wz-heng's open finding is addressed: content_rewritten now compares the value before/after the merge instead of just checking whether the assignment branch fired, so a falsy new_content (e.g. "") that strips away and leaves prev["content"] unchanged no longer trips a sidecar drop. Added the empty-string negative control requested (test_repair_merge_preserves_api_content_sidecar_when_content_unchanged_by_empty_string).

Diff is now clean against main: 2 files, 184 insertions, 0 deletions — no leftover Fix 1 duplication. 43/43 tests passing, ruff clean. Ready for re-review.