fix(compressor): age out stale tool-result images during compaction - #90001
Open
jackulau wants to merge 1 commit into
Open
fix(compressor): age out stale tool-result images during compaction#90001jackulau wants to merge 1 commit into
jackulau wants to merge 1 commit into
Conversation
11 tasks
Contributor
Author
|
The one red check on this PR is not this PR's. That is Cause is on main: |
jackulau
force-pushed
the
fix/89938-strip-stale-tool-result-images
branch
from
August 19, 2026 18:44
2499a77 to
a92a783
Compare
_strip_historical_media anchors on the newest image-bearing USER message and returns the list untouched when that anchor is index 0 or does not exist. A session whose images arrive from tools rather than attachments therefore has nothing to be "before": twenty vision_analyze results keep multi-MB of base64 in every request body, the provider answers 413, and the 413 handler's recovery compaction lands right back in this function and frees nothing. The reporter saw seven compactions in thirteen minutes, all below 200K tokens. Age tool-result images on their own timeline: keep the newest one, since that is the image the model is reasoning about, and strip every older one wherever it sits, including inside the protected tail. The tail exists to preserve conversational continuity, not to pin bytes the model has already moved past. User-message images keep today's treatment exactly. The user anchor is checked first, so a tool result that is the newest of its kind but still sits before that anchor is stripped as it always has been, and the anchor message itself is still kept byte-for-byte - test_compressor_zero_user_guard depends on that. Refs NousResearch#89938
jackulau
force-pushed
the
fix/89938-strip-stale-tool-result-images
branch
from
August 19, 2026 23:15
a92a783 to
749f378
Compare
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.
Relationship to #89965 - please read this first
#89965 and this PR fix different halves of #89938 and do not overlap by a single line. #89965 bounds the image bytes on every outgoing request so the 413 stops happening. This PR fixes the reason the session could not recover once it did.
In one sentence: #89965 prevents the 413; this makes the 413 recovery actually recover.
_strip_historical_mediais what the 413 handler's compaction ends up calling, and in the reported session it is a no-op - so every recovery pass returned a body that was still multi-MB and the provider answered 413 again. That is the "7 compactions in 13 minutes, all below 200K tokens" in the report. #89965 does not touch that function, so with only #89965 merged the wedge is unreachable in the common case but still there: any single window that exceeds the provider's body limit (a run of images larger thankeep_recent, or a provider whose limit is smaller than three screenshots) drops into the same loop with no way out.I am not proposing this as an alternative to #89965. If you would rather have one change, merge theirs - it is the larger fix and it is the one that stops the 413. This is
Refs #89938, notFixes, for exactly that reason.What does this PR do?
agent/context_compressor.py:_strip_historical_mediaanchors on the newest image-bearing user message and strips images from everything before it:A session whose images arrive from tools rather than attachments has nothing to be "before". The reported reproduction is the worst case of this and trips both exits at once:
anchor == 0and the early return fires; andvision_analyzeresults after it, which the user anchor would not have protected anyway.So the function returns the list untouched, the ~4MB of base64 rides along on every request, and the compaction the 413 handler invokes as recovery frees nothing.
This ages tool-result images on their own timeline: keep the newest one, strip every older one wherever it sits.
Related Issue
Refs #89938
Type of Change
Changes Made
agent/context_compressor.py-_strip_historical_mediagains a second anchor,tool_anchor, the newest image-bearingrole: "tool"message. A message is now stripped if it is before the user anchor (rule 1, unchanged) or it is an image-bearing tool message that is not the newest one (rule 2, new). The early return now fires only when neither rule can do anything.tests/agent/test_compressor_historical_media.py- 9 tests.Three decisions worth a maintainer's eye
1. Rule 1 keeps precedence. Where the two rules disagree - a tool result that is the newest of its kind but sits before the user anchor - rule 1 wins and it is stripped, which is exactly today's behaviour.
test_tool_image_before_the_user_anchor_is_still_strippedpins that, and it is the test to look at if you think the ordering should go the other way.2. The newest tool image survives inside the protected tail; older ones do not.
protect_last_nexists to preserve conversational continuity, not to pin bytes the model has already moved past, and a stale screenshot inside the tail is the single largest thing in the payload. This is the one behavioural change to the tail, and it is deliberate. If you would rather the tail stay byte-exact, rule 2 needs anindex >= tail_startexclusion - and the fix then stops working for the reported session, because all twenty results were in the tail.3. User images are untouched. I deliberately did not unify the two anchors into "newest image-bearing message of any role", which would have been the smaller diff. It would let a tool screenshot evict the user's most recent attachment, and
agent/context_compressor.py:7503-7510documents a live dependency on the user anchor being kept byte-for-byte (the_force_user_leadingguard reasons about an image-only user message having no text placeholder). Two anchors is more code and fewer surprises.How to Test
To see the old behaviour, restore the
if anchor <= 0: return messagesearly return and re-run:test_compress_frees_stale_vision_tool_resultsfails with bothvision_analyzeresults still carrying their base64.Verification
Mutation proof - 6 mutations, 6 caught:
i >= anchorloop (the bug)compress()onetool_anchorscans forward, so the OLDEST tool image is keptrole == "tool"test and applies to all rolesdrop_stale_api_contentremoved from the rewriteMutations 4 and 5 being caught by
test_compress_strips_historical_images- a test that predates this PR - is the useful signal: the existing user-path contract is still pinned, not merely believed.Pre-existing failures, named rather than hidden.
tests/agent/test_compression_review_76354.py::TestF6ExecutorSaturation::test_cancelled_fence_skips_summary_work_before_startfails on a pristine checkout of13ce0c5c67with this branch stashed, and fails identically with it applied. Same for four in the-k "image or vision"sweep (test_image_routing.pyx2,test_save_url_image.py,test_vision_routing_31179.py), all of which fail on the clean tree here. This PR touches none of them.Platform: Windows 11, Python 3.12,
uv. The change is pure list/dict manipulation - no paths, no processes, no platform-specific behaviour.Checklist
Code
fix(scope):,feat(scope):, etc.)_strip_historical_media's anchorpytest tests/ -qand all tests pass: the compression and multimodal suites are above, and the pre-existing failures are named rather than hiddenDocumentation & Housekeeping
docs/, docstrings) or N/A: the docstring now states both rules and why the tool rule existscli-config.yaml.exampleif I added/changed config keys or N/A: no config keysCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows or N/AScreenshots / Logs
The loop this removes, from the report:
The token count falls on every pass and the body does not, because the bytes that matter are base64 in tool results and nothing was stripping them.