fix(agent): make repair_message_sequence self-contained for orphaned assistant(tool_calls) - #57041
Closed
JoaoMarcos44 wants to merge 2 commits into
Closed
Conversation
…quence Interruption (/stop, process kill, resume mid tool-loop) can leave an assistant(tool_calls) turn with some or all of its tool_call_ids unanswered. Strict OpenAI-compatible providers (DeepSeek v4, Moonshot/Kimi) reject that shape outright instead of completing, which bricks the session until /new. Add a repair pass that detects tool_calls not fully answered by the tool messages immediately following them and inserts a synthetic error result for each missing id, using the same stub shape already used by sanitize_api_messages. Stubbing (instead of stripping the tool_calls) keeps the model's stated intent visible in the transcript and heals the persisted message list at the source, rather than only the ephemeral per-call copy. No-op when every call already has a matching result, so the existing ongoing-dialog pattern (complete assistant(tool_calls)+tool pair followed by a user redirect) is left untouched. Fixes NousResearch#56980
…close pass The orphan-close pass added for NousResearch#56980 read tool_calls[].id directly, diverging from the call_id||id extractor sanitize_api_messages and the context compressor already use for the same lookup. Codex/Responses-style tool_calls key the id as call_id, so those orphans went undetected (silently inert, not corrupted, but the pass had no effect on that shape). Switch to AIAgent._get_tool_call_id_static / _get_tool_call_name_static, the same helpers the rest of the file already relies on for this. Fixes NousResearch#57036
Contributor
Author
|
Superseding this PR with a new one against a rebuilt issue — closing #57039 and reopening with a documented DeepSeek/OpenAI-compat proof chain per feedback. New issue + PR incoming. |
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 — straytoolmessages, consecutive-assistant merges, consecutive-user merges — but had no pass for the inverse case: anassistantmessage carryingtool_callswhosetool_call_ids are not answered by thetoolmessages 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
/local-github-update+/local-llamacpp-compile, buildc43aa6301).repair_message_sequence's own docstring said, verbatim, "Deliberately does NOT rewind orphanassistant(tool_calls)+toolpairs..." — an admitted, intentional omission.c43aa6301/v2026.7.1-46-gc43aa6301does not exist anywhere in the repo's full history.git describeonmainat the time wasv2026.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./local-github-updateand/local-llamacpp-compile, don't exist anywhere in the tracked codebase (git grep, zero matches).repair_message_sequence -> sanitize_api_messagespipeline 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 #56980 described — trailing unansweredtool_calls, partial parallel results, all-orphan after a merge, Codexcall_id-format, orphan-then-user-injection — none of them reproduced a 400.sanitize_api_messagesis called unconditionally right afterrepair_message_sequencein both production call sites (conversation_loop.py:894,chat_completion_helpers.py:1536), andAIAgent.run_conversation(used by CLI,gateway/run.py, andtui_gateway/server.pyalike — there is no other entry point) always routes throughagent/conversation_loop.py, so this is the single pipeline every client shares. It already injects a stub tool result for every orphanedtool_call_idbefore the request reaches the provider.main.The actual, verified gap
repair_message_sequenceruns on the canonical, persistedmessageslist (session state).sanitize_api_messagesonly patches the ephemeral per-callapi_messagescopy rebuilt fresh on every request — it never writes back to the persisted session.Consequence: any caller that invokes
repair_message_sequencewithout a followingsanitize_api_messagescall stays exposed to exactly the HTTP 400 #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 therun_agent.py:_repair_message_sequenceforwarder (currently exercised only by tests, but public API surface).This PR closes that gap at the source —
repair_message_sequenceitself — 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):Behavior, precisely:
toolmessages immediately following it — mirrors how the existing Pass 1 (known_tool_ids) already reasons about adjacency, so asystemmessage interleaved between anassistant(tool_calls)and its realtoolanswer 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).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).AIAgent._get_tool_call_id_static/_get_tool_call_name_static—call_id || id— the exact extractorsanitize_api_messages(agent_runtime_helpers.py:2393) andcontext_compressor._sanitize_tool_pairsalready use. An earlier version of this pass readtc.get("id")directly; that missed Codex/Responses-API-shaped tool_calls, which key the id ascall_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.tool_call_idis answered, so a second call reportsrepairs == 0for the same input.assistant(tool_calls)+toolpair 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
toolrun after assistanttool_call_idseen anywheredel msg["tool_calls"]with no content fallback ->{"content": None}, notool_calls— second schema violation risk, confirmed by reproducing that exact code path locallytool_callspreservedMerging 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_repair_closes_fully_unanswered_trailing_tool_callsassistant(tool_calls)with zero tool resultstest_repair_closes_partially_answered_tool_callstool_calls, some ids answered, some missingtest_repair_closes_orphaned_tool_calls_before_wakeup_messagetool_callsimmediately followed by an injecteduser(wakeup/background-notification) message — the #56980 trigger patterntest_repair_orphan_close_is_idempotenttest_repair_closes_orphaned_tool_calls_using_call_id_fieldcall_idinstead ofidtest_repair_still_preserves_complete_pair_before_user_redirectResults:
python -m pytest tests/run_agent/test_message_sequence_repair.py -v— 31/31 passingpython -m pytest tests/agent/test_context_compressor.py tests/agent/test_replay_cleanup.py tests/agent/test_close_interrupted_tool_sequence.py -q— 159/159 passing (adjacent sanitizers, no regression)python -m pytest tests/run_agent/test_run_agent.py -q— 414/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:
id/call_idkey choice) throughrepair_message_sequencethensanitize_api_messagestogether, asserting: everyassistant(tool_calls)is immediately followed by atoolfor every one of its ids (well-formedness invariant), and a second pass over the result reports 0 repairs (idempotency invariant).tool_call_id) to guard against the exact bug: duplicate tool response messages cause strict provider HTTP 400 schema error #55442 duplicate-response class this design explicitly avoids.Related
Fixes #57039. Refs #56980 (original report, not reproducible as filed), #57036 (extractor fix, folded into this PR), supersedes #57013 (closed, opened before this investigation concluded #56980 wasn't reproducible).
Type of Change
Checklist
call_id||idextractor as the rest of the file