fix(agent): drop the api_content sidecar when stripping images from history - #68811
Open
Frowtek wants to merge 1 commit into
Open
fix(agent): drop the api_content sidecar when stripping images from history#68811Frowtek wants to merge 1 commit into
Frowtek wants to merge 1 commit into
Conversation
…istory `api_content` is the byte-stability sidecar from NousResearch#67274: it holds the exact bytes previously sent for a message, and every turn substitutes it back into `content` when building `api_messages`. `drop_stale_api_content` exists so a content rewrite cannot be replayed from it — its own docstring states the contract, and names the historical image strip as one of the callers: Replaying the pre-rewrite sidecar would resend exactly what the rewrite removed, so it must be dropped — the cost is one cache boundary miss, never wrong content. `_strip_images_from_messages` never drops it. The image-rejection recovery in `conversation_loop` runs it over the persistent history, not just the per-call copy: agent._vision_supported = False _imgs_removed = _strip_images_from_messages(messages) # history if isinstance(api_messages, list): _strip_images_from_messages(api_messages) and `api_messages` are copies (`api_msg = msg.copy()`), so the history message keeps its sidecar. The strip is therefore undone on the very next turn. Reproduced with the real functions: history content after strip : [{'type': 'text', 'text': 'look'}] sidecar still present : True NEXT TURN sends : 'look<IMAGE BYTES SENT LAST TURN>' This is worse than a one-turn glitch, because the recovery cannot fire again: it is gated on `getattr(agent, "_vision_supported", True)` and just set that False. So on every subsequent turn the sidecar re-injects the images, the text-only endpoint rejects them again, and the branch that would strip them is disabled — the session stays wedged on a 4xx it already knew how to fix. Drop the sidecar on each message the strip rewrites, inside the function so every caller is covered. Messages with no images keep theirs, so only the rewritten message pays a cache boundary — the tradeoff the invariant prescribes. The two sibling recovery paths, `_sanitize_messages_surrogates` and `_sanitize_messages_non_ascii`, are already safe: both walk every string field on the message and so scrub the sidecar in passing. This one only touches `content`. tests/run_agent/test_image_rejection_fallback.py: new TestStripImagesDropsStaleApiContent — the rewritten message loses its sidecar, the next turn does not resend the stripped images, the tool-placeholder rewrite is covered too, and untouched messages keep their sidecar. All four fail on main. 53 passed across the image-rejection and api_content-sidecar suites; 307 passed across the sanitization/image/sidecar/replay agent tests (8 failures in test_image_routing.py / test_save_url_image.py are pre-existing and fail identically on clean main).
Contributor
|
Thanks for the focused sidecar-invalidation hardening. Problems
Suggested changes
Automated hermes-sweeper review. |
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
api_contentis the byte-stability sidecar from #67274: it holds the exactbytes previously sent for a message, and every turn substitutes it back into
contentwhen buildingapi_messages.drop_stale_api_contentexists so acontent rewrite cannot be replayed from it — its own docstring states the
contract, and names the historical image strip as one of the callers:
_strip_images_from_messagesnever drops it. The image-rejection recovery inconversation_loopruns it over the persistent history, not just theper-call copy:
and
api_messagesare copies (api_msg = msg.copy()), so the history messagekeeps its sidecar. The strip is undone on the very next turn.
Reproduction
With the real functions:
Why this is session-fatal, not a one-turn glitch
The recovery cannot fire again: it is gated on
getattr(agent, "_vision_supported", True)and just set thatFalse. So onevery subsequent turn the sidecar re-injects the images, the text-only endpoint
rejects them again, and the branch that would strip them is disabled — the
session stays wedged on a 4xx it already knew how to fix.
Fix
Drop the sidecar on each message the strip rewrites, inside the function so
every caller is covered. Messages with no images keep theirs, so only the
rewritten message pays a cache boundary — the tradeoff the invariant prescribes.
The two sibling recovery paths,
_sanitize_messages_surrogatesand_sanitize_messages_non_ascii, are already safe: both walk every string fieldon the message and so scrub the sidecar in passing. This one only touches
content. (The sibling rewrite inreplay_cleanupdoes it explicitly, with thesame rationale spelled out.)
Test plan
tests/run_agent/test_image_rejection_fallback.py— newTestStripImagesDropsStaleApiContent:All four fail on
main.436 passedintest_run_agent.py;53 passedacross the image-rejection + api_content-sidecar suites;
307 passedacross thesanitization/image/sidecar/replay agent tests. Remaining failures
(
test_image_routing.py,test_save_url_image.py,TestPathCanonicalization)are pre-existing — verified by re-running them against the pre-fix code.
Checklist
drop_stale_api_contentcontract the other rewrite paths follow