fix(adapter): enforce tool_use/tool_result adjacency in _strip_orphaned_tool_blocks - #52145
fix(adapter): enforce tool_use/tool_result adjacency in _strip_orphaned_tool_blocks#52145fsaad1984 wants to merge 1 commit into
Conversation
Anthropic requires each tool_use block to have a matching tool_result in the IMMEDIATELY FOLLOWING user message. The previous implementation collected all tool_result IDs globally and dropped any tool_use whose ID wasn't in that set — but this missed the case where context compression removes messages *between* a tool_use and its result, leaving the IDs matching while the positional adjacency is broken. Anthropic rejects this with HTTP 400: messages.2: `tool_use` ids were found without `tool_result` blocks immediately after: toolu_01X.... Each `tool_use` block must have a corresponding `tool_result` block in the next message. Fix: two-pass approach. - Pass 1: for each assistant turn, collect tool_result IDs only from result[i+1] (the immediately following user message). Strip tool_use blocks that are not covered by that adjacent set. - Pass 2: rebuild the surviving tool_use ID set after pass 1, then strip any tool_result blocks whose ID no longer has a matching tool_use anywhere in the conversation. The thinking-signature invalidation flag (_thinking_signature_invalidated) is preserved: if stripping a tool_use leaves a signed thinking block in the same turn, we flag it so _manage_thinking_signatures can demote the stale signature instead of replaying it verbatim. All 40 existing guardrail tests pass unchanged. Co-authored-by: Faris Saad <fsaad1984@gmail.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused adapter fix. The premise is real on current main: _strip_orphaned_tool_blocks still uses a global tool_result_ids set at agent/anthropic_adapter.py:2093-2105, so it can keep a tool_use whose result exists later but not adjacent.
Problems
- The new adjacency check should match the final Anthropic payload, not the pre-merge list. Current main calls
_strip_orphaned_tool_blocks(result)and then_merge_consecutive_roles(result)atagent/anthropic_adapter.py:2390-2391. Since the PR checksresult[i + 1]inside the strip helper, it can strip a pair that would become valid after consecutive user messages are merged. - The PR diff changes only
agent/anthropic_adapter.py; there is no regression test for the non-adjacent/global-ID-match failure or the merge-order control case.
Suggested changes
- Run the adjacency sanitizer after
_merge_consecutive_roles, or make it validate against an equivalent merged/normalized view. - Add tests in
tests/agent/test_anthropic_adapter.pyfor both the broken non-adjacent case and the consecutive-user-message case.
Automated hermes-sweeper review.
|
|
||
| # Collect result IDs from the immediately following user message only. | ||
| adjacent_result_ids: set = set() | ||
| if i + 1 < len(result): |
There was a problem hiding this comment.
This checks adjacency before convert_messages_to_anthropic runs _merge_consecutive_roles on the result. If the tool_result is in the next consecutive user message, the final wire payload would be valid after merge, but this pass strips it first; consider validating after role-merge or against a merged copy.
The strip_orphaned_tool_blocks function checks result[i+1] for adjacency, but was running before _merge_consecutive_roles. This meant that two consecutive user messages (one plain, one with tool_result) would falsely look non-adjacent, causing valid tool_use blocks to be stripped. Fix: swap the call order — merge first, then strip orphans. Add two regression tests: - test_strips_non_adjacent_tool_use: verifies that a tool_use whose tool_result is separated by an intervening assistant turn is stripped - test_consecutive_user_messages_merged_before_adjacency_check: verifies that a valid pair is preserved when merge makes the result adjacent Addresses reviewer feedback on PR NousResearch#52145.
|
Thanks for the detailed review @teknium1. Both issues are now addressed: 1. Merge order fixed — 2. Tests added (
All 9 related tests pass. |
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from #52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. #52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…A (salvage NousResearch#52145 NousResearch#51948) (#268) Co-authored-by: qbit-mirror-bot <qbit-mirror-bot@users.noreply.github.com>
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…on to fork converter Two gaps in agent/fork/anthropic_messages.py surfaced by upstream's new test_anthropic_adapter tests (added in v2026.7.7.2): 1. Non-adjacent tool_use stripping (NousResearch#52145): the fork's orphan-strip pass matched tool_result IDs globally across the whole transcript, so a tool_use whose result appears LATER (not in the immediately following user message) was wrongly kept — Anthropic 400s on non-adjacent pairs. Ported upstream's adjacency-based logic: each tool_use must have a matching tool_result in the IMMEDIATELY FOLLOWING user message. 2. Top-level cache_control propagation: apply_anthropic_cache_control sets cache_control on the message dict (top-level) for assistant turns with empty content (pure tool_calls). The fork's converter dropped it — the tool_use block never received the marker. Added propagation in both the verbatim-replay path (anthropic_content_blocks) and the recomposition path (tool_calls): top-level cache_control is moved onto the last content block, where Anthropic expects it. Tests: test_strips_tool_use_when_result_not_immediately_adjacent, test_assistant_tool_use_cache_control_is_preserved, test_ordered_replay_tool_use_cache_control_is_preserved, test_keeps_tool_use_when_result_immediately_adjacent — all pass.
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
…ed_tool_blocks _strip_orphaned_tool_blocks collected tool_result ids across ALL user messages and kept any assistant tool_use whose id appeared anywhere, rather than requiring the result to be in the immediately-following user message. A stale match elsewhere in the transcript could keep a genuinely-orphaned tool_use, which Anthropic rejects. Rewrite to adjacency-checked two-pass logic so a tool_use is kept only when its result immediately follows. Salvaged from NousResearch#52145. Co-authored-by: fsaad1984 <38867992+fsaad1984@users.noreply.github.com>
… fix vacuous refresh-UA test (review) Review follow-up on the anthropic_adapter batch salvage: 1. NousResearch#52145 shipped no behavior test for the adjacency rewrite. Add test_strips_tool_use_when_result_not_immediately_adjacent (a tool_use whose result appears later but NOT in the immediately-following user message must be stripped — the exact case the old global id-match got wrong) plus an adjacent-pair control. Mutation-checked: reverting to a global match fails the non-adjacent test. 2. test_token_refresh_ua_prefix was vacuous — it bound to _refresh_oauth_token (a wrapper with no urllib.request.Request), so its assert never ran and it did NOT guard the real refresh UA site. Retarget it at refresh_anthropic_oauth_pure (:1048) with the header-scoped check. Mutation- checked: reverting :1048 to claude-cli/ now fails it.
Problem
After context compression, messages can be inserted between a
tool_useblock and its correspondingtool_result. The previous implementation of_strip_orphaned_tool_blocksonly checked whether a matchingtool_resultID existed anywhere in the conversation — but Anthropic requires it to be in the immediately following user message.This causes a non-retryable HTTP 400 error:
The error surfaces to the user as:
Root Cause
After compression,
tool_useat positionimay have its result at positioni+3instead ofi+1. The IDs still match globally, so the old code kept thetool_use— but Anthropic rejects it.Fix
Two-pass approach:
Pass 1: For each assistant turn with
tool_useblocks, collecttool_resultIDs only fromresult[i+1](the immediately following user message). Striptool_useblocks not covered by that adjacent set.Pass 2: Rebuild the surviving
tool_useID set after pass 1, then strip anytool_resultblocks whose ID no longer has a matchingtool_useanywhere.The existing
_thinking_signature_invalidatedflag is preserved for the case where stripping atool_useleaves a signed thinking block in the same turn.Testing
All 40 existing guardrail tests pass unchanged:
Reproduction
Trigger: long conversation with tool calls → context compression fires → next message hits HTTP 400.