fix(agent): enforce tool-result adjacency after compression - #36157
Closed
konsisumer wants to merge 1 commit into
Closed
fix(agent): enforce tool-result adjacency after compression#36157konsisumer wants to merge 1 commit into
konsisumer wants to merge 1 commit into
Conversation
Contributor
tonydwb
approved these changes
Jun 1, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
fix(agent): enforce tool-result adjacency after compression
Important correctness fix for context compression. Key observations:
- Problem: Context compression could leave orphaned
role: "tool"messages that were no longer adjacent to their originatingtool_callsassistant message. This invalid ordering triggers provider-side HTTP 400 errors. - Fix: Hardened
_sanitize_tool_pairs()with a strict ordering pass — only keeps tool results that are in the immediate assistant tool-results block and match pendingtool_call_ids. Misordered tool results are dropped with logging. - Scope: 2 files, well-focused.
- Tests: Regression test covering the boundary/order violation case.
Looks Good
- Addresses a crash-level bug (#20883)
- Conservative fix (drops orphaned tool results with logging)
- Well-tested
Reviewed by Hermes Agent
Contributor
Author
|
Thanks @daimon-nous for flagging this. I looked up #24374 and you're right — it's open and addresses #20883 as well, but at a different layer:
These two layers are complementary (defense-in-depth), but I recognize that maintainers may prefer a single canonical fix location. A few options:
This PR is currently labeled P1 while #24374 is P2. If maintainers want to close this in favor of #24374 or vice versa, please advise and I will act accordingly. |
Contributor
Author
|
Closing: referenced_issue_closed. all referenced issues closed: [20883] |
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.
What does this PR do?
Fixes a crash-level context compression correctness bug where a
role: "tool"message could survive compaction even when it was no longer directly adjacent to its originating assistanttool_callsmessage. That invalid ordering triggers provider-side HTTP 400 errors and can force session resets.Related Issue
Fixes #20883
Type of Change
Changes Made
agent/context_compressor.py: hardened_sanitize_tool_pairs()with a strict ordering pass that only keeps tool results when they are in the immediate assistant tool-results block and match pendingtool_call_ids; misordered tool results are dropped with logging.tests/agent/test_context_compressor.py: added regression testtest_sanitizer_drops_tool_results_not_adjacent_to_calling_assistantcovering the boundary/order violation case.How to Test
pytest tests/agent/test_context_compressor.py -q -k "sanitizer_drops_tool_results_not_adjacent_to_calling_assistant or sanitizer_matches_responses_call_id_when_id_differs" --timeout=60ruff check agent/context_compressor.py tests/agent/test_context_compressor.pypytest tests/agent/test_context_compressor.py -q -x --timeout=60Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AWhat platforms tested on
For New Skills
Not applicable.
Screenshots / Logs
pytest tests/agent/test_context_compressor.py -q -k "sanitizer_drops_tool_results_not_adjacent_to_calling_assistant or sanitizer_matches_responses_call_id_when_id_differs" --timeout=60→2 passed, 90 deselectedruff check agent/context_compressor.py tests/agent/test_context_compressor.py→All checks passed!python scripts/check-windows-footguns.pyran without finding blocking patterns in changed files.