From e1762bd30bf1762870a6da36c1985a6e444f11e7 Mon Sep 17 00:00:00 2001 From: Frowtek Date: Tue, 21 Jul 2026 20:09:48 +0300 Subject: [PATCH] fix(agent): drop the api_content sidecar when stripping images from history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `api_content` is the byte-stability sidecar from #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' 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). --- agent/message_sanitization.py | 10 +++ .../test_image_rejection_fallback.py | 77 +++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/agent/message_sanitization.py b/agent/message_sanitization.py index 88c2304459..3ba2520fcb 100644 --- a/agent/message_sanitization.py +++ b/agent/message_sanitization.py @@ -416,8 +416,16 @@ def _strip_images_from_messages(messages: list) -> bool: practice this only hits synthetic image-only user messages appended for attachment delivery; real user turns always include text. + This runs on the persistent history as well as the per-call copy, so any + message it rewrites must also lose its ``api_content`` sidecar: the sidecar + carries the exact bytes previously sent — here, the images this strip + exists to remove — and the next turn substitutes it back into ``content``, + undoing the strip on the wire. + Returns True if any image parts were removed. """ + from agent.turn_context import drop_stale_api_content + found = False to_delete = [] for i, msg in enumerate(messages): @@ -443,6 +451,8 @@ def _strip_images_from_messages(messages: list) -> bool: # Synthetic image-only user/assistant message with no text; # safe to drop. to_delete.append(i) + # Content was rewritten — the pre-strip sidecar is now stale. + drop_stale_api_content(msg) for i in reversed(to_delete): del messages[i] return found diff --git a/tests/run_agent/test_image_rejection_fallback.py b/tests/run_agent/test_image_rejection_fallback.py index 6ea6449480..f9e851605a 100644 --- a/tests/run_agent/test_image_rejection_fallback.py +++ b/tests/run_agent/test_image_rejection_fallback.py @@ -173,3 +173,80 @@ class TestImageRejectionPhraseIsolation: assert self._matches(body) is True, f"false negative on: {body}" +class TestStripImagesDropsStaleApiContent: + """The strip runs on the persistent history, not just the per-call copy. + + ``api_content`` is the byte-stability sidecar: it holds the exact bytes + previously sent for a message, and the next turn substitutes it back into + ``content``. Leaving it in place on a message this function rewrote would + replay the images the strip just removed — and the recovery cannot re-fire, + because it sets ``_vision_supported = False`` and gates itself on that. The + session would then send rejected images on every subsequent turn. + + Same contract the other content-rewrite paths follow (stale-confirmation + redaction in ``replay_cleanup``, compression rewrites, merge-into-tail): + "the cost is one cache boundary miss, never wrong content". + """ + + @staticmethod + def _wire(msg): + """What the next turn actually sends for this history message.""" + from agent.turn_context import substitute_api_content + + api_msg = msg.copy() + substitute_api_content(api_msg) + return api_msg["content"] + + def _image_msg(self, sidecar="look"): + return { + "role": "user", + "content": [ + {"type": "text", "text": "look"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}}, + ], + "api_content": sidecar, + } + + def test_stripped_message_loses_its_sidecar(self): + msgs = [self._image_msg()] + assert _strip_images_from_messages(msgs) is True + assert "api_content" not in msgs[0] + + def test_next_turn_does_not_resend_the_stripped_images(self): + msgs = [self._image_msg()] + _strip_images_from_messages(msgs) + + wire = self._wire(msgs[0]) + assert "IMAGE BYTES" not in str(wire), ( + "the stale sidecar replayed the images the strip removed" + ) + assert wire == [{"type": "text", "text": "look"}] + + def test_tool_placeholder_message_also_loses_its_sidecar(self): + """An image-only tool result becomes a placeholder — same rewrite.""" + msgs = [ + { + "role": "tool", + "tool_call_id": "call_1", + "content": [{"type": "image_url", "image_url": {"url": "x"}}], + "api_content": "", + } + ] + assert _strip_images_from_messages(msgs) is True + assert "api_content" not in msgs[0] + assert "image content removed" in msgs[0]["content"] + + def test_untouched_messages_keep_their_sidecar(self): + """Only rewritten messages pay the cache boundary — not the whole prefix.""" + msgs = [ + { + "role": "user", + "content": [{"type": "text", "text": "no images here"}], + "api_content": "no images here", + }, + self._image_msg(), + ] + _strip_images_from_messages(msgs) + + assert msgs[0]["api_content"] == "no images here" + assert "api_content" not in msgs[1]