fix(agent): drop unpairable tool calls before API send

[JoaoMarcos44]

Summary

• Removes assistant tool calls with no usable pairing identifier from the final per-call API copy.
• Preserves valid tool calls and results.
• Leaves persisted conversation history unchanged.
• Adds a mixed valid/unpairable batch regression test.

Problem and impact

An assistant message could contain two tool calls while only one had a correlatable role: tool result. The malformed payload reached the final API boundary as N calls and N-1 results, allowing strict provider validation to reject the request and leaving the model with an unpairable call.

Root cause

sanitize_api_messages() indexed only calls returned by _tool_call_id_variants(). A call with no usable ID was excluded from the missing-result stub pass, but the later reconstruction loop copied the original call into the assistant message anyway.

Implementation decision

The fix drops only unpairable assistant calls from the per-call copy. It deliberately does not invent an ID because there is no safe way to correlate a future result with a fabricated identifier. The stored transcript remains unchanged for prompt-cache and history stability.

Scope and compatibility

• Valid id, call_id, and alias variants remain unchanged.
• Empty/null tool_calls arrays remain covered by the existing empty-array fixes, including PR fix(agent): close the empty tool_calls 400 class at all three chokepoints (#83312, #77921) #86654.
• No credentials, provider data, or network fixtures are used.

Files

• agent/agent_runtime_helpers.py
• tests/run_agent/test_message_sequence_repair.py

Verification

• Local: scripts/run_tests.sh tests/run_agent/test_message_sequence_repair.py -q → 36 passed.
• Local ruff check: passed.
• Local git diff --check: passed.
• Remote required checks: passed at last verification.
• The real pre-fix reproduction observed 2 assistant calls, 1 result, and 1 unpaired call remaining.

Fixes #93769

[jmeadlock]

Nice work — I'd independently built the same fix this morning (same insertion point, same tool_call_id_variants filter) before finding this PR, so I'm not opening a duplicate. Two edge cases surfaced in review of my version that apply to this shape too:

1. All-dropped + empty content: if every call in the batch is unpairable and content is "", dropping tool_calls emits an empty non-final assistant message — the shape repair_empty_non_final_messages() exists to prevent (and it has already run by this point). Healing with the same _INTERRUPTED_PLACEHOLDER fixes it.
2. Ordering vs. the empty-name repair: that pass mutates nested tool-call dicts in place, so a call that's both unpairable and blank-named gets renamed in the stored history before your drop removes it from the wire — breaking the byte-stability the PR body promises. Running the drop before the name repair fixes it; blank-name calls with real ids are unaffected.

Full write-up with repro code and four liftable regression tests (all-dropped healing, blank-name immutability, whitespace-only id, alias-only call_id): https://al-engr.com/hermes-93769-unpairable-tool-calls-notes.html — take anything useful, no attribution needed.

[kokhlo]

Reviewed this against current main — the core fix works and the approach (drop on the per-call copy, never touch stored history) is right. One gap though:

Dropping all tool_calls from a message can leave an empty non-final assistant turn.

repair_empty_non_final_messages() runs at line ~3708, before the new unpairable pass (~3800). If a non-final assistant message's only payload was an unpairable tool_call, the drop leaves:

{"role": "assistant", "content": None}   # ← survives to the wire

Repro (against this branch, sanitize_api_messages input):

[
    {"role": "user", "content": "run tools"},
    {"role": "assistant", "content": None, "tool_calls": [
        {"type": "function", "function": {"name": "terminal", "arguments": "{}"}},  # no id
    ]},
    {"role": "user", "content": "next turn"},
]

Output:

Pre-call sanitizer: dropped 1 assistant tool_call(s) without a usable pairing id
  {'role': 'user', 'content': 'run tools'}
  {'role': 'assistant', 'content': None}     ← empty non-final survives
  {'role': 'user', 'content': 'next turn'}

Anthropic (and the litellm/Bedrock proxies in front of it) 400 on exactly this — "all messages must have non-empty content except for the optional final assistant message" — so the session stays poisoned, just with a different shape of poison.

Suggested fix — re-run the empty-message healing after the drop pass (3 lines):

    if unpairable_tool_calls:
        messages = normalized_unpairable
        # The drop can strip the only payload from a non-final assistant turn;
        # heal the empties it may have created before anything downstream runs.
        messages = repair_empty_non_final_messages(messages)
        _ra().logger.warning(...)

And a regression test for the all-calls-dropped case:

def test_sanitize_drops_all_unpairable_pads_empty():
    messages = [
        {"role": "user", "content": "run tools"},
        {"role": "assistant", "content": None, "tool_calls": [
            {"type": "function", "function": {"name": "terminal", "arguments": "{}"}},
        ]},
        {"role": "user", "content": "next turn"},
    ]
    out = sanitize_api_messages(list(messages))
    assistant = next(m for m in out if m.get("role") == "assistant")
    assert "tool_calls" not in assistant
    assert assistant.get("content")  # placeholder from the healing pass, not None

Everything else checks out from my run: your existing test passes, the orphaned tool-result direction is already handled by the later pairing pass, and the stored trajectory is untouched (shallow copies only).