Skip to content

fix(agent): make repair_message_sequence self-contained for orphaned assistant(tool_calls) - #843

Closed
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57041
Closed

fix(agent): make repair_message_sequence self-contained for orphaned assistant(tool_calls)#843
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-57041

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Summary

repair_message_sequence (agent/agent_runtime_helpers.py) sanitizes the persisted message history before every LLM call. It already handled 3 classes of malformed history — stray tool messages, consecutive-assistant merges, consecutive-user merges — but had no pass for the inverse case: an assistant message carrying tool_calls whose tool_call_ids are not answered by the tool messages that immediately follow it.

This PR adds that pass, makes the function self-contained, and fixes an id-extraction gap discovered during review.

Background — how we got here

  1. [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980 reported this as a live HTTP 400 from DeepSeek v4 in production, with a specific repro (30+ message session, /local-github-update + /local-llamacpp-compile, build c43aa6301).
  2. Investigating the code independently (before even finding the issue) confirmed the underlying gap is real: repair_message_sequence's own docstring said, verbatim, "Deliberately does NOT rewind orphan assistant(tool_calls)+tool pairs..." — an admitted, intentional omission.
  3. Cross-checking [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980's own "Related Issues" list against the tracker confirmed 6/6 are real (bug: duplicate tool response messages cause strict provider HTTP 400 schema error NousResearch/hermes-agent#55442, Fix: Partial tool-call results brick sessions on resume NousResearch/hermes-agent#53716, DeepSeek 400: assistant message with content+tool_calls split into 2 messages in API payload NousResearch/hermes-agent#49147, fix(agent): merge consecutive assistant messages before API replay (#29148, #49147) NousResearch/hermes-agent#55603, fix(agent): close tool-call sequence on interrupt to prevent role alternation violation (#48879) NousResearch/hermes-agent#51727, [Bug]: Role alternation violation (Tool -> User) when interrupting via /stop leads to model hallucinations and user message continuation NousResearch/hermes-agent#48879), two of them merged fixes from a maintainer for adjacent alternation bugs — so the bug class has a real, well-documented history.
  4. But the specific incident in [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980 doesn't hold up:
    • The cited commit c43aa6301 / v2026.7.1-46-gc43aa6301 does not exist anywhere in the repo's full history. git describe on main at the time was v2026.7.1-17-g88d1d6206 — 17 commits past the tag, not 46. The claimed commit is 29 commits further than the repo's actual history reached — not a typo, a geometrically impossible reference.
    • The two cited slash commands, /local-github-update and /local-llamacpp-compile, don't exist anywhere in the tracked codebase (git grep, zero matches).
  5. Running the real repair_message_sequence -> sanitize_api_messages pipeline against every orphan shape [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980 described — trailing unanswered tool_calls, partial parallel results, all-orphan after a merge, Codex call_id-format, orphan-then-user-injection — none of them reproduced a 400. sanitize_api_messages is called unconditionally right after repair_message_sequence in both production call sites (conversation_loop.py:894, chat_completion_helpers.py:1536), and AIAgent.run_conversation (used by CLI, gateway/run.py, and tui_gateway/server.py alike — there is no other entry point) always routes through agent/conversation_loop.py, so this is the single pipeline every client shares. It already injects a stub tool result for every orphaned tool_call_id before the request reaches the provider.
  6. A maintainer-side close of [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980 reached the same empirical conclusion independently (matching finding, same broken commit citation carried over from the report without being caught) and asked for a raw rejected-payload artifact to proceed — which nobody has produced, because the described incident doesn't reproduce against main.
  7. Conclusion: [Bug]: repair_message_sequence fails to prevent HTTP 400 "insufficient tool messages following tool_calls" on DeepSeek v4 after long tool-call sessions NousResearch/hermes-agent#56980 as filed is not reproducible. The design gap it pointed at is real anyway — see next section — so this work continues as repair_message_sequence: orphaned assistant(tool_calls) not self-contained (only patched downstream by sanitize_api_messages) NousResearch/hermes-agent#57039, a clean issue that documents the verified problem on its own terms instead of resting on a fabricated incident report.

The actual, verified gap

repair_message_sequence runs on the canonical, persisted messages list (session state). sanitize_api_messages only patches the ephemeral per-call api_messages copy rebuilt fresh on every request — it never writes back to the persisted session.

Consequence: any caller that invokes repair_message_sequence without a following sanitize_api_messages call stays exposed to exactly the HTTP 400 NousResearch#56980 described, and even in the covered path, the persisted session itself never actually heals — the same repair gets silently redone on every single call instead of converging once. Known/plausible callers in this position: sub-agent spawns, MoA reference-model calls, plugins, and the run_agent.py:_repair_message_sequence forwarder (currently exercised only by tests, but public API surface).

This PR closes that gap at the source — repair_message_sequence itself — instead of continuing to depend on a second, separately-invoked function to catch it downstream on every call.

What changed

agent/agent_runtime_helpers.py — new Pass 2 inserted between the existing stray-tool-drop pass and the consecutive-user-merge pass (renumbered to Pass 3):

# Pass 2: close assistant(tool_calls) turns whose ids are not fully
# answered by the tool messages immediately following them.
idx = 0
while idx < len(filtered):
    msg = filtered[idx]
    if not (isinstance(msg, dict) and msg.get("role") == "assistant"):
        idx += 1
        continue
    call_ids = [
        _ra().AIAgent._get_tool_call_id_static(tc)
        for tc in (msg.get("tool_calls") or [])
        if isinstance(tc, dict) and _ra().AIAgent._get_tool_call_id_static(tc)
    ]
    if not call_ids:
        idx += 1
        continue
    run_end = idx + 1
    answered: set = set()
    while (run_end < len(filtered) and isinstance(filtered[run_end], dict)
           and filtered[run_end].get("role") == "tool"):
        answered.add((filtered[run_end].get("tool_call_id") or "").strip())
        run_end += 1
    missing = [cid for cid in call_ids if cid not in answered]
    if missing:
        names = {
            _ra().AIAgent._get_tool_call_id_static(tc): _ra().AIAgent._get_tool_call_name_static(tc)
            for tc in (msg.get("tool_calls") or []) if isinstance(tc, dict)
        }
        stubs = [{
            "role": "tool", "tool_call_id": cid,
            "name": names.get(cid) or "unknown",
            "content": "Tool execution was interrupted before a result was returned.",
        } for cid in missing]
        filtered[run_end:run_end] = stubs
        repairs += len(stubs)
        run_end += len(stubs)
    idx = run_end

Behavior, precisely:

  • Scoped per assistant turn, looking only at the run of tool messages immediately following it — mirrors how the existing Pass 1 (known_tool_ids) already reasons about adjacency, so a system message interleaved between an assistant(tool_calls) and its real tool answer still lets that answer count (matches pre-existing Pass 1 semantics; a naive merge of the two passes into one loop was tried and rejected in review because it broke exactly this case — see "Alternatives considered" below).
  • Stubs, does not strip. If a turn's calls are fully orphaned, a strip approach (del msg["tool_calls"]) would leave {"role": "assistant", "content": None} — a contentless turn with no fallback text. Stubbing avoids that failure mode entirely and keeps the model's stated intent (what it tried to call) visible in the transcript, which also gives the model useful signal on its next turn ("execution was interrupted" vs. silence).
  • Uses AIAgent._get_tool_call_id_static / _get_tool_call_name_staticcall_id || id — the exact extractor sanitize_api_messages (agent_runtime_helpers.py:2393) and context_compressor._sanitize_tool_pairs already use. An earlier version of this pass read tc.get("id") directly; that missed Codex/Responses-API-shaped tool_calls, which key the id as call_id. Not a corruption risk (missing id just made the entry a silent no-op), but inconsistent with the rest of the file and inert exactly where the rest of the codebase already guards against this shape. Fixed and covered by a dedicated test.
  • Idempotent: once a stub is inserted, that tool_call_id is answered, so a second call reports repairs == 0 for the same input.
  • No-op on the existing "ongoing dialog" pattern: a complete assistant(tool_calls)+tool pair followed by a user redirect (valid when the previous turn finished normally and the user jumped in before the model's continuation turn) is untouched, because every id in that turn already has a matching result.

Docstring updated to describe this as pass "2." and drop the now-inaccurate "Deliberately does NOT rewind..." paragraph; renumbered the user-merge pass to "3."

Alternatives considered

This PR (stub, separate pass) Merge into Pass 1 (rejected) Strip tool_calls (rejected)
Detects orphan Per-turn, contiguous tool run after assistant Would require single "run closes" definition shared with Pass 1 Global set of every tool_call_id seen anywhere
System message between assistant and its real tool answer Still counts (scoped correctly) Breaks — the merged pass would prematurely close the run and later duplicate the real answer against an already-inserted stub, reintroducing the NousResearch#55442 duplicate-tool-response class N/A (doesn't insert anything)
All calls in a turn orphaned Inserts stub per id, assistant content untouched del msg["tool_calls"] with no content fallback -> {"content": None}, no tool_calls — second schema violation risk, confirmed by reproducing that exact code path locally
Consistent with existing extractor convention Yes (`call_id id`, post-fix)
Data loss None — tool_calls preserved Yes — erases that the model attempted a call

Merging into Pass 1 would have saved ~25 lines but requires collapsing two genuinely different "when does a run close" definitions into one, which is exactly the kind of premature abstraction that trades a small line-count win for cross-cutting coupling and a real regression risk. Kept as two small, single-responsibility passes instead.

Testing

tests/run_agent/test_message_sequence_repair.py — 6 new tests:

Test Covers
test_repair_closes_fully_unanswered_trailing_tool_calls Trailing assistant(tool_calls) with zero tool results
test_repair_closes_partially_answered_tool_calls Parallel tool_calls, some ids answered, some missing
test_repair_closes_orphaned_tool_calls_before_wakeup_message Orphaned tool_calls immediately followed by an injected user (wakeup/background-notification) message — the NousResearch#56980 trigger pattern
test_repair_orphan_close_is_idempotent Running the repair twice does not insert duplicate stubs
test_repair_closes_orphaned_tool_calls_using_call_id_field Codex/Responses-style tool_calls keyed by call_id instead of id
test_repair_still_preserves_complete_pair_before_user_redirect Non-regression: complete pair + user redirect stays untouched

Results:

  • python -m pytest tests/run_agent/test_message_sequence_repair.py -v31/31 passing
  • python -m pytest tests/agent/test_context_compressor.py tests/agent/test_replay_cleanup.py tests/agent/test_close_interrupted_tool_sequence.py -q159/159 passing (adjacent sanitizers, no regression)
  • python -m pytest tests/run_agent/test_run_agent.py -q414/414 passing (full suite touching this module)

Suggested differential testing (not included in this PR, follow-up)

To validate no regression across long/real sessions and catch the N+1 issue class:

  1. Fuzz synthetic message histories (random mix of assistant/tool/user, random orphan gaps, random id/call_id key choice) through repair_message_sequence then sanitize_api_messages together, asserting: every assistant(tool_calls) is immediately followed by a tool for every one of its ids (well-formedness invariant), and a second pass over the result reports 0 repairs (idempotency invariant).
  2. Replay real 50+ message session dumps (with interruption points) through the full pipeline and assert the same two invariants, to catch any non-contiguous or multi-turn interleaving the unit tests don't construct.
  3. Assert message count monotonicity bounds (repairs only ever insert stubs or merge, never duplicate an existing tool_call_id) to guard against the exact bug: duplicate tool response messages cause strict provider HTTP 400 schema error NousResearch/hermes-agent#55442 duplicate-response class this design explicitly avoids.

Related

Fixes NousResearch#57039. Refs NousResearch#56980 (original report, not reproducible as filed), NousResearch#57036 (extractor fix, folded into this PR), supersedes NousResearch#57013 (closed, opened before this investigation concluded NousResearch#56980 wasn't reproducible).

Type of Change

  • Bug fix / hardening (non-breaking)

Checklist

  • No new dependencies
  • No changes to message serialization format
  • No breaking changes to OpenAI/Anthropic provider compatibility
  • Existing "ongoing dialog" pattern (complete pair + user redirect) covered by a regression test
  • Uses the same call_id||id extractor as the rest of the file

Mirror-of: NousResearch#57041
NousResearch#57041

@hashbender hashbender closed this Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repair_message_sequence: orphaned assistant(tool_calls) not self-contained (only patched downstream by sanitize_api_messages)

1 participant