From 41a9081638d9f6ecca43a2d3b4e595056a82d8f0 Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Tue, 8 Sep 2026 17:17:58 +0800 Subject: [PATCH 1/5] feat(loop): let a tool hand the model an image, not a description of one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool that had an image to return could only send prose about it: the products called a separate vision model and put its transcript into the history. The main model never saw the pixels, could not look again when a later turn raised a new question about the same picture, and the round trip was not even cheap — a transcript runs 2-6K tokens where the image it describes is a few hundred. A tool can now return `tool_content(text, images=[image_attachment(...)])`. The envelope is a plain JSON dict rather than a dataclass because sandbox-native tools execute in a child process and their return value crosses `json.dumps(default=str)` on the way back; a dataclass arrives as its repr. `ToolResult.images` carries the attachments and `result` stays the full text, so observers, spill/recovery, repeat detection and the trajectory see exactly what they saw before. Visibility is the loop's decision, not the tool's. `ModelProfile.supports_images` (default False) and the wire protocol gate it; only `chat_completions` is implemented, as an OpenAI `image_url` part carrying a data URI. The Anthropic block spelling is not interchangeable — a served OpenAI-compatible endpoint answers it with HTTP 400 and a pydantic union error, failing the whole turn including the other tool results in it — so an unimplemented protocol withholds instead of guessing. `HistoryPolicy.max_images_in_history` (default 5) now bounds the history. Both flags existed as unread placeholders. An image that leaves the context always leaves a sentence behind, whether it was withheld for capability or evicted for room, and `text_of` renders an image block as a placeholder rather than as nothing. This is the finding the whole design is built around: in the calibration run, deleting the image block from an otherwise working request did not make the model report a missing image — it reported a four-digit code and three shapes, every one invented. A tool result that reads as though an image were delivered gets answered as though one were. `estimate_message_tokens` now charges for inline images, priced off the `image_meta` message key written at attach time (~1 token per 1024 px, fitted against measured prompt_tokens; 1080p ≈ 2.4K, 4K ≈ 8.5K). It previously returned the caption length alone, so a history of screenshots measured as nearly empty to the context guard and every compaction trigger. The estimate is driven by the image blocks actually present, so a compactor that rewrites content to a string stops the charge with them. Tests: 33 cases across tests/test_tool_content_images.py (envelope validation, header sniffing for PNG/JPEG/WEBP/GIF, attachment, withholding, eviction order, the stale-bookkeeping case) and tests/test_agent_loop_image_results.py (the same behaviours driven through run_agent_loop). Two bugs were found by writing them: a multi-image result evicted its own newest page first, and stale `image_meta` kept charging for images a compactor had already dropped. Generated with [Claude Code](https://claude.ai/code) via [Very Happy](https://github.com/Mereithhh/very-happy) Co-Authored-By: Claude --- CHANGELOG.md | 56 ++++ agent_core/loop_types.py | 7 + agent_core/messages.py | 25 ++ agent_core/runtime/loop/agent_loop.py | 25 ++ agent_core/runtime/loop/image_attach.py | 234 ++++++++++++++++ agent_core/runtime/loop/tool_exec.py | 13 +- agent_core/tokens.py | 7 + agent_core/tool_content.py | 356 ++++++++++++++++++++++++ docs/agent-loop-boundary.md | 47 ++++ pyproject.toml | 2 +- tests/test_agent_loop_image_results.py | 215 ++++++++++++++ tests/test_tool_content_images.py | 348 +++++++++++++++++++++++ uv.lock | 2 +- 13 files changed, 1334 insertions(+), 3 deletions(-) create mode 100644 agent_core/runtime/loop/image_attach.py create mode 100644 agent_core/tool_content.py create mode 100644 tests/test_agent_loop_image_results.py create mode 100644 tests/test_tool_content_images.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b98c7d8..573b61f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,62 @@ the GitHub Release body, so a release with no entry here fails. Versioning follows [docs/versioning.md](docs/versioning.md). +## [0.10.0] - 2026-09-08 + +### Added + +- A tool can return images that the model itself looks at, instead of prose from + a second model describing them. Return + `tool_content(text, images=[image_attachment(png_bytes, "image/png", label=path)])` + from `agent_core.tool_content` — a plain JSON dict, not a dataclass, so it + survives the `json.dumps` boundary a sandbox-native tool's return value + crosses. `ToolResult` grows an `images` field; `result` remains the full text, + so observers, spill/recovery, repeat detection and the trajectory see exactly + what they saw before. + + Whether the pixels reach the provider is decided by the loop, from + `ModelProfile.supports_images` (default `False`, so nothing changes for a + product that does not opt in) and the wire protocol. Only + `chat_completions` is implemented: the attachment becomes an OpenAI + `image_url` content part carrying a data URI. The Anthropic block spelling is + **not** interchangeable — a served OpenAI-compatible endpoint rejects it with + HTTP 400 and a pydantic union error, taking the whole turn down, so an + unimplemented protocol withholds rather than guessing. + + `HistoryPolicy.max_images_in_history` (default 5) now does something: after + each tool batch the loop keeps that many of the newest images and replaces + the rest with text. Both flags existed as unread placeholders before this + release. + + **An image that leaves the context always leaves a sentence behind** — when + it is withheld for capability reasons, and when it is evicted for room. This + is not cosmetic. In the calibration run, deleting the image block from an + otherwise working request did not make the model report a missing image: it + reported a four-digit code and three shapes, all invented. A tool result that + reads as though an image were delivered will be answered as though one were. + Products adding their own image paths should preserve this property. + +### Changed + +- `messages.text_of` renders an `image_url` content block as + `[image — not visible in this text-only rendering]` instead of the empty + string. Every caller is either sizing a message or building a text-only + rendering — the Anthropic message translation, a compaction summary, a + trajectory line — and in the rendering case the image is being dropped at + that call. **If a product already puts `image_url` blocks in `content`, its + flattened text changes**; nothing else in this package produced such blocks + before 0.10.0. +- `tokens.estimate_message_tokens` charges for inline images, read off the + `image_meta` message key written at attach time (~1 token per 1024 pixels, + fitted against measured `prompt_tokens`; a 1080p screenshot is ~2.4K tokens, + a 4K one ~8.5K). It previously returned the length of the caption alone, so a + history of screenshots measured as nearly empty to the context guard and to + every compaction trigger. Estimates for image-free histories are unchanged. +- `Message` gains the in-process key `image_meta`, positionally aligned with the + `image_url` blocks in `content`. It is outside `WIRE_MESSAGE_KEYS`, so + `for_wire` strips it. It is message-level rather than per-block because a + content part carrying an unknown key is rejected by the served endpoint. + ## [0.9.1] - 2026-09-08 ### Fixed diff --git a/agent_core/loop_types.py b/agent_core/loop_types.py index 0bae9a6..4920482 100644 --- a/agent_core/loop_types.py +++ b/agent_core/loop_types.py @@ -312,6 +312,13 @@ class ToolResult: error_kind: str = "" # Opaque host-owned handle for a result body shed from model context. result_id: str = "" + # Image attachments the tool returned alongside ``result``, validated by + # ``agent_core.tool_content.parse_tool_content``. ``result`` stays the + # complete TEXT of the result, so every consumer that reads a string -- + # observers, spill/recovery, repeat detection, the trajectory -- is + # unaffected by a tool that attaches images. Only the message built for the + # provider looks at this, and only when the model can actually see it. + images: list[dict[str, Any]] = field(default_factory=list[dict[str, Any]]) # Host-provided repeated-invocation metadata. Execution is never skipped. repeat_count: int = 1 repeat_recovery_id: str = "" diff --git a/agent_core/messages.py b/agent_core/messages.py index 52225db..97f5f16 100644 --- a/agent_core/messages.py +++ b/agent_core/messages.py @@ -75,6 +75,15 @@ class Message(TypedDict, total=False): # the text back, which is what makes an index distinguishable from a summary # that happens to quote one. Filtered out by ``for_wire``. spill_refs: list[str] + # Per-image bookkeeping for the ``image_url`` blocks in ``content``, in the + # same order: ``{"label": str, "tokens": int}`` each. Written by + # ``runtime.loop.image_attach.attach_images`` and read back by eviction (to + # name an image it is removing) and by the token estimate (so a history of + # images is not costed at zero). It cannot live inside the content blocks: + # a content part carrying an unknown key is rejected outright by the served + # endpoint's pydantic union, whereas a message-level key outside + # ``WIRE_MESSAGE_KEYS`` is dropped by ``for_wire``. + image_meta: list[dict[str, Any]] # ── Wire boundary ──────────────────────────────────────────────────────── @@ -222,6 +231,10 @@ def assistant_msg_with_reasoning( # ── Helpers ────────────────────────────────────────────────────────────── +# What an inline image renders as once its blocks are flattened away. +_IMAGE_PLACEHOLDER = "[image — not visible in this text-only rendering]" + + def text_of(content: Any) -> str: """Flatten an OpenAI/Anthropic message content to plain text.""" if content is None: @@ -235,6 +248,18 @@ def text_of(content: Any) -> str: parts.append(block) elif isinstance(block, dict): content_block = cast(dict[str, object], block) + # An image flattened to text must leave a mark. Every caller is + # either estimating size or building a text-only rendering -- + # the Anthropic translation, a compaction summary, a trajectory + # line -- and in the rendering case the image is being dropped + # right here. Returning nothing for it yields a tool result that + # reads as though it were pure text and never mentioned an + # image, which is the exact input that made the calibration + # model invent a reading of a picture it could not see (see + # ``runtime.loop.image_attach``). + if content_block.get("type") == "image_url": + parts.append(_IMAGE_PLACEHOLDER) + continue val = content_block.get("text") or content_block.get("content") or "" if isinstance(val, str): parts.append(val) diff --git a/agent_core/runtime/loop/agent_loop.py b/agent_core/runtime/loop/agent_loop.py index 5992128..f9b5889 100644 --- a/agent_core/runtime/loop/agent_loop.py +++ b/agent_core/runtime/loop/agent_loop.py @@ -49,6 +49,7 @@ DefaultMessageCompactor, estimate_tokens, ) +from agent_core.runtime.loop.image_attach import attach_images, evict_old_images from agent_core.runtime.loop.llm_client import ( RUNAWAY_STATE_KEY, TRUNCATION_CONTINUATION_GUIDANCE, @@ -527,6 +528,8 @@ async def _run_loop_inner( runtime.body_has_spill_reference, runtime.render_tool_result, result_max_chars=tool_result_cap, + profile=profile, + max_images_in_history=policy.max_images_in_history, ) total_tool_calls += tool_calls_executed if stop_reason: @@ -1259,6 +1262,10 @@ async def _execute_tool_calls( ], *, result_max_chars: int | None = None, + profile: ModelProfile | None = None, + # -1 disables eviction. Not 0: a defaulted caller must not silently mean + # "throw every image away", which is what a 0 default would spell. + max_images_in_history: int = -1, ) -> tuple[str, int]: executable: list[tuple[int, dict]] = [] synthetic: list[tuple[int, ToolResult]] = [] @@ -1356,8 +1363,26 @@ async def _execute_tool_calls( tr_result.tool_call_id, ) cast("dict[str, Any]", history_message).update(message_metadata) + # After the recovery handle and the host metadata, so the text the model + # reads is final before it becomes the text block of a multimodal + # message. Called unconditionally: when the model cannot see images + # ``attach_images`` writes the note saying so, and a caller that skipped + # it on capability grounds would produce the one shape that is actually + # dangerous -- a result reading as though an image had been delivered, + # with no image in it. + attach_images( + history_message, + tr_result.images, + profile=profile or ModelProfile(model_id="default", provider="openai"), + ) messages.append(history_message) + # Bound the history's image count once the batch is in, not before: the + # newest results are the ones worth keeping, and evicting first would let a + # turn that returned several images push out its own. + if max_images_in_history >= 0: + evict_old_images(messages, max_images_in_history) + if any(result.interrupted for result in results): wait_interventions = await notify_observers(obs, "on_tool_wait_interrupted", ctx) merged_wait = merge_interventions(wait_interventions) diff --git a/agent_core/runtime/loop/image_attach.py b/agent_core/runtime/loop/image_attach.py new file mode 100644 index 0000000..5f3a9a3 --- /dev/null +++ b/agent_core/runtime/loop/image_attach.py @@ -0,0 +1,234 @@ +"""Put a tool's image attachments into the message the provider sees. + +Two decisions live here and nowhere else: whether an attachment is allowed onto +the wire at all, and how many stay in history as the run goes on. Both resolve +the same way when the answer is no -- the image is replaced by a sentence saying +an image was there and is not any more. + +That is the point of the module rather than an incidental nicety. The +calibration run behind this feature (MiroHarness +``internal-docs/designs/2026-09-08-native-image-in-tool-result-calibration.md``, +case E) took a working request and deleted only the image block. The model was instructed to +answer ``NO_IMAGE`` if it could not see an image. It instead reported a +four-digit code and three shapes, all confidently wrong. A model handed a tool +result that reads like an image was delivered will describe the image it +expects; only text that contradicts that expectation stops it. + +Per-image bookkeeping (label, estimated tokens) rides on the message under +``image_meta``, positionally aligned with the ``image_url`` blocks in +``content``. It is deliberately message-level and not inside the blocks: a +content part with an unrecognised key is rejected by the served endpoint's +pydantic union, while a message-level key outside ``WIRE_MESSAGE_KEYS`` is +dropped by :func:`agent_core.messages.for_wire` before the request is built. +Keeping the token estimate here is also what stops the context guard from +having to base64-decode the whole history on every turn to find out how big it +is. +""" + +from __future__ import annotations + +import logging +from typing import Any, cast + +from agent_core.messages import Message +from agent_core.runtime.loop.model_profile import ModelProfile +from agent_core.tool_content import ( + image_tokens, + message_image_meta, + message_image_tokens, +) + +logger = logging.getLogger(__name__) + +__all__ = [ + "attach_images", + "evict_old_images", + "image_blocks_in", + "message_image_tokens", +] + +# Wire protocols whose inline-image block shape is the one built below. +# Anthropic Messages and the OpenAI Responses API both accept images and both +# spell the block differently (``{"type": "image", "source": {...}}`` / +# ``{"type": "input_image", ...}``). This is not a shape a server tolerates and +# ignores when wrong: the calibration run sent the Anthropic spelling to the +# OpenAI-compatible endpoint and got HTTP 400 with a pydantic union error, which +# fails the entire turn including the other tool results in it. So the protocol +# is checked rather than assumed, and an unhandled one withholds with a note. +_INLINE_IMAGE_PROTOCOLS = frozenset({"chat_completions"}) + + +def _as_block(block: Any) -> dict[str, Any] | None: + """A content entry as a keyed block, or ``None`` if it is not one.""" + return cast("dict[str, Any]", block) if isinstance(block, dict) else None + + +def _is_image(block: Any) -> bool: + entry = _as_block(block) + return entry is not None and entry.get("type") == "image_url" + + +def attach_images( + message: Message, + images: list[dict[str, Any]], + *, + profile: ModelProfile, +) -> None: + """Add *images* to a tool *message*, in place. + + When the model cannot take them, the message keeps plain-string content and + gains an explicit note. Callers never branch on capability: they always call + this, and the message is correct either way. + """ + if not images: + return + + why = "" + if not profile.supports_images: + why = f"the model in use ({profile.model_id}) cannot accept images" + elif profile.protocol not in _INLINE_IMAGE_PROTOCOLS: + why = ( + f"inline images are not implemented for the {profile.protocol} " + "wire protocol" + ) + if why: + logger.info("withholding %d image(s) from tool message: %s", len(images), why) + text = _text_content(message) + note = _withheld_note(images, why) + message["content"] = f"{text}\n\n{note}" if text else note + return + + text = _text_content(message) + blocks: list[dict[str, Any]] = [] + if text: + blocks.append({"type": "text", "text": text}) + meta: list[dict[str, Any]] = [] + for image in images: + blocks.append({ + "type": "image_url", + "image_url": { + "url": f"data:{image['mime_type']};base64,{image['data']}", + }, + }) + meta.append({ + "label": str(image.get("label") or ""), + "tokens": image_tokens(image), + }) + message["content"] = blocks + cast("dict[str, Any]", message)["image_meta"] = meta + + +def evict_old_images(messages: list[Message], max_images: int) -> int: + """Keep only the newest *max_images* inline images; return how many went. + + Walks newest-first, so what survives is what the model is most likely to + still be working from. An evicted image leaves a sentence behind, and a + message left with no images goes back to plain-string content -- there is no + reason to keep a block list, and a string is what every checkpoint, replay + and text-flattening path handles most cheaply. + """ + if max_images < 0: + return 0 + + kept = 0 + evicted = 0 + for message in messages[::-1]: + content = message.get("content") + if not isinstance(content, list): + continue + meta = message_image_meta(message) + blocks = cast("list[Any]", content) + image_positions = [ + index for index, block in enumerate(blocks) if _is_image(block) + ] + if not image_positions: + continue + # Newest-first WITHIN the message too, not just across messages. A + # single tool result can return several images (a PDF rendered page by + # page), and walking its blocks forward while walking the history + # backward keeps the wrong end of that result. + drop_at: set[int] = set() + for index in reversed(image_positions): + if kept < max_images: + kept += 1 + continue + drop_at.add(index) + evicted += 1 + if not drop_at: + continue + at_image = set(image_positions) + rebuilt: list[Any] = [] + surviving: list[dict[str, Any]] = [] + seen = 0 + for index, block in enumerate(blocks): + if index not in at_image: + rebuilt.append(block) + continue + entry = meta[seen] if seen < len(meta) else {} + seen += 1 + if index in drop_at: + rebuilt.append({"type": "text", "text": _evicted_note(entry)}) + else: + rebuilt.append(block) + surviving.append(entry) + if surviving: + message["content"] = rebuilt + cast("dict[str, Any]", message)["image_meta"] = surviving + else: + message["content"] = "\n".join( + str(entry.get("text") or "") + for entry in (_as_block(block) for block in rebuilt) + if entry is not None and entry.get("text") + ) + cast("dict[str, Any]", message).pop("image_meta", None) + if evicted: + logger.info( + "evicted %d image(s) from history, keeping the newest %d", + evicted, max_images, + ) + return evicted + + +def image_blocks_in(message: Message) -> int: + """How many inline images this message currently carries.""" + content = message.get("content") + if not isinstance(content, list): + return 0 + return sum(1 for block in cast("list[Any]", content) if _is_image(block)) + + +def _withheld_note(images: list[dict[str, Any]], why: str) -> str: + named = ", ".join( + label for label in (str(i.get("label") or "") for i in images) if label + ) + subject = "1 image" if len(images) == 1 else f"{len(images)} images" + where = f" ({named})" if named else "" + return ( + f"[{subject}{where} could not be shown to you: {why}. You have NOT " + "seen this image. Do not describe, transcribe, or draw any conclusion " + "from its contents.]" + ) + + +def _evicted_note(entry: dict[str, Any]) -> str: + label = str(entry.get("label") or "") + where = f" {label}" if label else "" + return ( + f"[An image{where} was here and has been dropped from context to make " + "room. You can no longer see it. Read it again if you still need it, " + "and do not rely on remembering what it showed.]" + ) + + +def _text_content(message: Message) -> str: + content = message.get("content") + if isinstance(content, str): + return content + if isinstance(content, list): + parts = [ + str(entry.get("text") or "") + for entry in (_as_block(block) for block in cast("list[Any]", content)) + if entry is not None and entry.get("type") == "text" + ] + return "\n".join(part for part in parts if part) + return "" if content is None else str(content) diff --git a/agent_core/runtime/loop/tool_exec.py b/agent_core/runtime/loop/tool_exec.py index 10c97cb..ba027a1 100644 --- a/agent_core/runtime/loop/tool_exec.py +++ b/agent_core/runtime/loop/tool_exec.py @@ -13,6 +13,7 @@ from typing import Any, Protocol, runtime_checkable from agent_core.loop_types import ToolResult +from agent_core.tool_content import parse_tool_content logger = logging.getLogger(__name__) @@ -335,7 +336,16 @@ async def _run_one(call: dict[str, Any], index: int) -> ToolResult: else: raw = await invocation - result = runtime.transform_result(name, str(raw) if raw is not None else "") + # A tool may hand back text plus image attachments instead of a + # bare string. Split them BEFORE the stringify: ``str()`` on the + # envelope would put a base64 blob's repr into the model's context + # and lose the attachment at the same time. + structured = parse_tool_content(raw) + if structured is not None: + raw_text, images = structured + else: + raw_text, images = (str(raw) if raw is not None else ""), [] + result = runtime.transform_result(name, raw_text) metadata_raw = _safe_hook( "result_metadata", lambda: runtime.result_metadata(name, args, raw, result), @@ -354,6 +364,7 @@ async def _run_one(call: dict[str, Any], index: int) -> ToolResult: duration_ms=int((time.monotonic() - start) * 1000), tool_call_id=tool_call_id, is_error=bool(error_kind), + images=images, interrupted=woke_for_interrupt, error_kind=error_kind, result_id=str(metadata.get("result_id") or ""), diff --git a/agent_core/tokens.py b/agent_core/tokens.py index 05cd7e5..106b9c3 100644 --- a/agent_core/tokens.py +++ b/agent_core/tokens.py @@ -32,6 +32,7 @@ from typing import Any, cast from agent_core.messages import text_of +from agent_core.tool_content import message_image_tokens # Per-message wire overhead (role, delimiters, the trailing separator). Same # constant ``compact.estimate_tokens`` adds when it sums a whole history. @@ -84,4 +85,10 @@ def estimate_message_tokens(message: Any) -> int: tokens = estimate_text_tokens(text_of(content)) + _PER_MESSAGE_OVERHEAD if tool_calls: tokens += estimate_text_tokens(_tool_calls_text(tool_calls)) + # Inline images contribute no text, so without this a tool result carrying a + # 1080p screenshot -- about 2.4K prompt tokens on the model this was + # calibrated against -- measures as the length of its caption. The context + # guard and every compaction trigger read this number, so undercounting it + # is how a history walks into an overflow the guard reported as comfortable. + tokens += message_image_tokens(message) return tokens diff --git a/agent_core/tool_content.py b/agent_core/tool_content.py new file mode 100644 index 0000000..bfcb92c --- /dev/null +++ b/agent_core/tool_content.py @@ -0,0 +1,356 @@ +"""Structured tool output: text plus image attachments the model itself reads. + +A tool normally returns a string and that string becomes the whole tool message. +Some tools have something to hand back that is not text — an image. The +alternative to this module is what the products did before it: call a separate +vision model, and put ITS prose transcript into the history. That is a lossy +round trip (the main model never sees the pixels, only another model's summary +of them, and cannot go back and look again when a later turn raises a new +question about the same picture) and it is not cheaper — a transcript runs +2-6K tokens where the image it describes is a few hundred. + +The wire shape a tool returns is a plain JSON dict, never a dataclass, because +sandbox-native tools are executed in a child process and their return value +crosses a ``json.dumps(default=str)`` boundary on the way back +(``plugins/tool_runtime/server.py`` in MiroHarness). A dataclass survives that +trip as its repr. Build it with :func:`tool_content`:: + + return tool_content( + "Screenshot of the failing dialog:", + images=[image_attachment(png_bytes, "image/png", label="/tmp/shot.png")], + ) + +Nothing downstream is obliged to honour it. Whether the attachment reaches the +provider is decided in the loop, from ``ModelProfile.supports_images`` and the +wire protocol — a model with no vision gets the text and an explicit note that +an image was withheld. That note is not politeness. In the calibration run for +this feature the control case removed the image block and left everything else +identical: the model did not report a missing image, it invented a confident, +wrong reading of one. An image that silently vanishes from a tool result is +therefore a correctness bug, not a degraded capability, and every path here that +declines to attach says so in the text instead. + +The measurements behind the constants in this module — the pixels-per-token +figure, the caps, the block shape the server actually accepts — are in +MiroHarness ``internal-docs/designs/2026-09-08-native-image-in-tool-result-calibration.md``. +Read it before changing any of them. +""" + +from __future__ import annotations + +import base64 +import binascii +import logging +from typing import Any, cast + +logger = logging.getLogger(__name__) + +__all__ = [ + "IMAGE_MIME_TYPES", + "MAX_IMAGES_PER_RESULT", + "MAX_IMAGE_BYTES", + "TOOL_CONTENT_MARKER", + "image_attachment", + "image_tokens", + "message_image_meta", + "message_image_tokens", + "parse_tool_content", + "sniff_image_size", + "tool_content", +] + +# Present and truthy on a dict that means "structured tool output". A marker key +# rather than duck-typing on ``{"text", "images"}``: tools are free to return +# ordinary dicts, and one that happens to carry those two keys must keep +# stringifying the way it always did. +TOOL_CONTENT_MARKER = "__tool_content__" + +# What the OpenAI content-part schema accepts as an inline image. Enforced here +# because an unsupported type is a 400 from the server, and a 400 fails the whole +# turn rather than just the attachment. +IMAGE_MIME_TYPES = frozenset( + {"image/png", "image/jpeg", "image/webp", "image/gif"} +) + +# Per-image and per-result ceilings. Both are about the context window, not the +# HTTP body: a 4K screenshot measured 8.5K prompt tokens against apodex-1.1-mini, +# so a handful of them at full resolution is the whole budget. Producers are +# expected to downscale; these are the backstop for producers that did not. +MAX_IMAGE_BYTES = 6 * 1024 * 1024 +MAX_IMAGES_PER_RESULT = 8 + +# Pixels per token, fitted against apodex-1.1-mini prompt_tokens over the same +# image at 640x360 / 1280x720 / 1920x1080 / 3840x2160 (227 / 908 / 2043 / 8170 +# image tokens, r² > 0.999 on a straight px term). Providers differ, and a +# provider that tiles differently will be off by a constant factor — that is +# acceptable for a budget estimate, and catastrophically better than the zero +# this used to contribute, which let a history full of images look empty to the +# context guard. +_PIXELS_PER_TOKEN = 1024 +# Charged when dimensions cannot be read. Deliberately not "0" and not the 4K +# figure: an unknown image is assumed to be roughly a 1080p screenshot. +_UNKNOWN_IMAGE_TOKENS = 2400 + + +def tool_content(text: str, *, images: list[dict[str, Any]] | None = None) -> dict[str, Any]: + """Build the structured return value for a tool with attachments. + + ``text`` is what the tool would have returned on its own, and stays the + tool's result string everywhere in the loop — observers, spill/recovery, + repeat detection and the trajectory all keep seeing a plain string. The + images ride alongside it. + """ + return { + TOOL_CONTENT_MARKER: 1, + "text": text, + "images": list(images or []), + } + + +def image_attachment( + data: bytes | str, + mime_type: str, + *, + label: str = "", + width: int = 0, + height: int = 0, +) -> dict[str, Any]: + """One image attachment. ``data`` may be raw bytes or an existing base64 str. + + ``label`` is what the model is told the image IS (usually the path it asked + for). It is carried separately from the text so a placeholder can name the + image after the bytes are gone. + """ + b64 = base64.b64encode(data).decode("ascii") if isinstance(data, bytes) else data + attachment: dict[str, Any] = {"mime_type": mime_type, "data": b64} + if label: + attachment["label"] = label + if width > 0 and height > 0: + attachment["width"] = int(width) + attachment["height"] = int(height) + return attachment + + +def parse_tool_content(raw: Any) -> tuple[str, list[dict[str, Any]]] | None: + """Split a tool return value into ``(text, images)``, or ``None``. + + ``None`` means "this is not structured output" and the caller should keep + its existing ``str(raw)`` behaviour. A malformed attachment inside a + well-formed envelope is dropped and reported in the text, never silently: + see the module docstring on why a vanishing image is worse than no image. + """ + if not isinstance(raw, dict): + return None + envelope = cast("dict[str, Any]", raw) + if not envelope.get(TOOL_CONTENT_MARKER): + return None + text = envelope.get("text") + text = text if isinstance(text, str) else ("" if text is None else str(text)) + + raw_images = envelope.get("images") + candidates: list[Any] = ( + list(cast("list[Any]", raw_images)) if isinstance(raw_images, list) else [] + ) + images: list[dict[str, Any]] = [] + rejected: list[str] = [] + for index, candidate in enumerate(candidates): + if len(images) >= MAX_IMAGES_PER_RESULT: + rejected.append( + f"image {index + 1} and the ones after it (more than " + f"{MAX_IMAGES_PER_RESULT} images in one result)" + ) + break + accepted, why = _validated_image(candidate) + if accepted is None: + rejected.append(f"image {index + 1} ({why})") + continue + images.append(accepted) + + if rejected: + note = "not attached: " + "; ".join(rejected) + logger.warning("tool content dropped %s", note) + text = f"{text}\n\n[{note}]" if text else f"[{note}]" + return text, images + + +def _validated_image(candidate: Any) -> tuple[dict[str, Any] | None, str]: + if not isinstance(candidate, dict): + return None, "not an object" + image = cast("dict[str, Any]", candidate) + mime = str(image.get("mime_type") or "") + if mime not in IMAGE_MIME_TYPES: + return None, f"unsupported type {mime or 'unset'!r}" + data = image.get("data") + if not isinstance(data, str) or not data: + return None, "no base64 payload" + # Validated here rather than at the provider: a bad payload is an HTTP 400 + # that takes the whole turn down, and the turn's other tool results with it. + try: + decoded_size = len(base64.b64decode(data, validate=True)) + except (binascii.Error, ValueError): + return None, "payload is not valid base64" + if decoded_size == 0: + return None, "payload is empty" + if decoded_size > MAX_IMAGE_BYTES: + return None, ( + f"{decoded_size // 1024} KB exceeds the " + f"{MAX_IMAGE_BYTES // (1024 * 1024)} MB per-image cap" + ) + + accepted: dict[str, Any] = {"mime_type": mime, "data": data} + label = image.get("label") + if isinstance(label, str) and label: + accepted["label"] = label + width, height = _declared_size(image) + if width <= 0 or height <= 0: + width, height = sniff_image_size(base64.b64decode(data, validate=True)) + if width > 0 and height > 0: + accepted["width"] = width + accepted["height"] = height + return accepted, "" + + +def _declared_size(image: dict[str, Any]) -> tuple[int, int]: + try: + return int(image.get("width") or 0), int(image.get("height") or 0) + except (TypeError, ValueError): + return 0, 0 + + +def sniff_image_size(data: bytes) -> tuple[int, int]: + """Pixel dimensions from an image header, or ``(0, 0)``. + + Header parsing rather than a decode: the only consumer is the token + estimate, this runs on every attachment, and AgentCore has no imaging + dependency to decode with. + """ + try: + if data[:8] == b"\x89PNG\r\n\x1a\n" and data[12:16] == b"IHDR": + return ( + int.from_bytes(data[16:20], "big"), + int.from_bytes(data[20:24], "big"), + ) + if data[:3] == b"GIF": + return ( + int.from_bytes(data[6:8], "little"), + int.from_bytes(data[8:10], "little"), + ) + if data[:4] == b"RIFF" and data[8:12] == b"WEBP": + return _webp_size(data) + if data[:2] == b"\xff\xd8": + return _jpeg_size(data) + except (IndexError, ValueError): + return 0, 0 + return 0, 0 + + +def _webp_size(data: bytes) -> tuple[int, int]: + chunk = data[12:16] + if chunk == b"VP8X": + return ( + int.from_bytes(data[24:27], "little") + 1, + int.from_bytes(data[27:30], "little") + 1, + ) + if chunk == b"VP8 ": + return ( + int.from_bytes(data[26:28], "little") & 0x3FFF, + int.from_bytes(data[28:30], "little") & 0x3FFF, + ) + if chunk == b"VP8L": + bits = int.from_bytes(data[21:25], "little") + return (bits & 0x3FFF) + 1, ((bits >> 14) & 0x3FFF) + 1 + return 0, 0 + + +def _jpeg_size(data: bytes) -> tuple[int, int]: + # Walk the marker chain to the frame header. SOF0/1/2/3/5/6/7/9-11/13-15 all + # carry the dimensions at the same offset; DHT/DAC/RST/SOS do not and are + # skipped by length like any other segment. + offset = 2 + end = len(data) + while offset + 9 < end: + if data[offset] != 0xFF: + offset += 1 + continue + marker = data[offset + 1] + if marker in (0xD8, 0x01) or 0xD0 <= marker <= 0xD7: + offset += 2 + continue + if marker == 0xDA: # start of scan — no frame header past here + return 0, 0 + segment_length = int.from_bytes(data[offset + 2:offset + 4], "big") + if segment_length < 2: + return 0, 0 + if 0xC0 <= marker <= 0xCF and marker not in (0xC4, 0xC8, 0xCC): + return ( + int.from_bytes(data[offset + 7:offset + 9], "big"), + int.from_bytes(data[offset + 5:offset + 7], "big"), + ) + offset += 2 + segment_length + return 0, 0 + + +def image_tokens(image: dict[str, Any]) -> int: + """Estimated prompt tokens an attachment costs. Never zero.""" + width, height = _declared_size(image) + if width <= 0 or height <= 0: + return _UNKNOWN_IMAGE_TOKENS + return max(1, (width * height) // _PIXELS_PER_TOKEN) + + +def message_image_meta(message: Any) -> list[dict[str, Any]]: + """The ``image_meta`` list on a message, positionally aligned with images.""" + if not isinstance(message, dict): + return [] + raw = cast("dict[str, Any]", message).get("image_meta") + if not isinstance(raw, list): + return [] + return [ + cast("dict[str, Any]", entry) + for entry in cast("list[Any]", raw) + if isinstance(entry, dict) + ] + + +def message_image_tokens(message: Any) -> int: + """Estimated prompt tokens the inline images on *message* cost. + + Counted from the ``image_url`` blocks actually present, priced from + ``image_meta``. Driven by the content rather than by the metadata because a + compactor is free to rewrite ``content`` to a plain string -- which drops + the images -- without knowing that a bookkeeping key describes them. Trusting + the metadata there would keep charging for pixels no longer in the request. + + The prices are read off ``image_meta`` rather than measured: the bytes are a + data URI by this point, and base64-decoding the whole history on every + estimate would make the context guard cost more than the turn it guards. An + image with no usable price still charges something -- an unknown image is + not a free one. + + Lives here rather than beside the rest of the message-image handling in + ``runtime.loop.image_attach`` for one reason: ``tokens`` needs it, and this + module imports nothing from ``agent_core``, so there is no import cycle to + get wrong later. + """ + if not isinstance(message, dict): + return 0 + content = cast("dict[str, Any]", message).get("content") + if not isinstance(content, list): + return 0 + blocks = sum( + 1 + for block in cast("list[Any]", content) + if isinstance(block, dict) and cast("dict[str, Any]", block).get("type") == "image_url" + ) + if not blocks: + return 0 + meta = message_image_meta(message) + total = 0 + for index in range(blocks): + entry = meta[index] if index < len(meta) else {} + try: + priced = max(0, int(entry.get("tokens") or 0)) + except (TypeError, ValueError): + priced = 0 + total += priced or _UNKNOWN_IMAGE_TOKENS + return total diff --git a/docs/agent-loop-boundary.md b/docs/agent-loop-boundary.md index dbec126..dfaa3e1 100644 --- a/docs/agent-loop-boundary.md +++ b/docs/agent-loop-boundary.md @@ -98,3 +98,50 @@ product words its note. Two consequences worth stating: - AgentCore still words nothing about repeats. A product moving off its own loop copy must port its note into `render_tool_result`; nothing here will fail if it forgets, which is why this paragraph exists. + +## Image attachments: the core decides visibility, the product supplies pixels + +A tool returns images by returning the `agent_core.tool_content.tool_content` +envelope — a plain JSON dict — instead of a string. Sandbox-native tools run in +a child process and their return value crosses `json.dumps(default=str)` on the +way back, so the envelope is a dict by requirement, not by taste; a dataclass +arrives as its repr. + +The split of responsibility: + +- **The product** decides what is worth attaching, and downscales. Pixel count + is the cost driver (roughly 1 token per 1024 px against the model this was + calibrated on — a 1080p screenshot ~2.4K tokens, a 4K one ~8.5K), and + AgentCore has no imaging dependency with which to resize. The caps in + `tool_content` (6 MB and 8 images per result) are a backstop against a + runaway producer, not a resize policy. +- **AgentCore** decides whether an attachment is shown, from + `ModelProfile.supports_images` and `ModelProfile.protocol`, and how many stay + in history, from `HistoryPolicy.max_images_in_history`. Products do not + pre-filter on capability: `attach_images` is called unconditionally and writes + the withheld note itself. + +`ToolResult.images` is populated by core (`tool_exec` parses the envelope) and +consumed by core (`agent_loop` builds the message). It is not a host-supplied +field, so unlike `result_id` and the repeated-invocation metadata above it needs +no boundary exemption. + +### Why every disappearance is narrated + +Both the capability path and the eviction path replace the image with a sentence +saying an image was there and is not visible. The calibration run behind this +feature (MiroHarness +`internal-docs/designs/2026-09-08-native-image-in-tool-result-calibration.md`) +removed the image block from an otherwise working request and changed nothing +else. The model had been told to answer `NO_IMAGE` if it could not see +an image; it instead produced a four-digit code and three shapes, every one of +them invented. A tool result whose text reads as though an image were delivered +will be answered as though one were, so the text has to say otherwise. A product +that adds its own image-bearing path — or its own compactor that touches these +messages — inherits that obligation. + +For the same reason `messages.text_of` renders an `image_url` block as a +placeholder rather than as nothing: every flattening caller (Anthropic message +translation, compaction summaries, trajectory lines) is dropping the image at +that call, and a transcript that never mentions the picture is the same trap in +a different place. diff --git a/pyproject.toml b/pyproject.toml index e61789e..d67ba85 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "apodex-agent-core" -version = "0.9.1" +version = "0.10.0" description = "Shared, product-neutral runtime primitives for Apodex agents" readme = "README.md" license = "Apache-2.0" diff --git a/tests/test_agent_loop_image_results.py b/tests/test_agent_loop_image_results.py new file mode 100644 index 0000000..9ac01b7 --- /dev/null +++ b/tests/test_agent_loop_image_results.py @@ -0,0 +1,215 @@ +"""End-to-end: a tool that attaches an image, driven through ``run_agent_loop``. + +The unit tests in ``test_tool_content_images`` cover the pieces. These cover the +wiring -- that a tool's envelope actually survives execution, rendering, the +recovery-handle step and the history append, and that the model profile is what +decides whether the pixels go on the wire. +""" + +from __future__ import annotations + +import struct +import zlib +from typing import Any + +import pytest + +from agent_core.llm import LLMResponse +from agent_core.loop_types import LoopConfig, LoopPolicy +from agent_core.messages import for_wire +from agent_core.runtime.loop.agent_loop import run_agent_loop +from agent_core.runtime.loop.model_profile import HistoryPolicy, ModelProfile +from agent_core.tool_content import image_attachment, tool_content + + +def _png(width: int, height: int) -> bytes: + def chunk(tag: bytes, body: bytes) -> bytes: + return ( + struct.pack(">I", len(body)) + + tag + + body + + struct.pack(">I", zlib.crc32(tag + body) & 0xFFFFFFFF) + ) + + ihdr = struct.pack(">IIBBBBB", width, height, 8, 2, 0, 0, 0) + raw = b"".join(b"\x00" + b"\x00\x00\x00" * width for _ in range(height)) + return ( + b"\x89PNG\r\n\x1a\n" + + chunk(b"IHDR", ihdr) + + chunk(b"IDAT", zlib.compress(raw)) + + chunk(b"IEND", b"") + ) + + +class SequenceLLM: + def __init__(self, responses: list[LLMResponse]) -> None: + self.responses = responses + self.calls: list[list[dict[str, Any]]] = [] + + async def chat(self, messages, **_kwargs) -> LLMResponse: + self.calls.append([dict(message) for message in messages]) + return self.responses.pop(0) + + def stream(self, messages, **_kwargs): + raise AssertionError("streaming was not requested") + + +class ShotTool: + """A view_image-shaped tool: a caption plus the bytes themselves.""" + + name = "view_image" + + def __init__(self, count: int = 1) -> None: + self.count = count + + async def ainvoke(self, args: dict[str, Any]) -> Any: + path = str(args.get("path") or "/tmp/shot.png") + return tool_content( + f"Image {path} (64x32) is attached below.", + images=[ + image_attachment(_png(64, 32), "image/png", label=path) + for _ in range(self.count) + ], + ) + + def to_openai_schema(self) -> dict[str, Any]: + return { + "type": "function", + "function": { + "name": self.name, + "description": "read an image", + "parameters": {"type": "object"}, + }, + } + + +def _call(index: int, path: str) -> dict[str, Any]: + return { + "id": f"tc{index}", + "type": "function", + "function": {"name": "view_image", "arguments": f'{{"path":"{path}"}}'}, + } + + +def _config() -> LoopConfig: + return LoopConfig( + max_turns=6, loop_policy=LoopPolicy(no_tool_behavior="stop"), max_llm_retries=1, + ) + + +async def _run( + profile: ModelProfile, + *, + paths: list[str], + policy: HistoryPolicy | None = None, + images_per_call: int = 1, +): + responses = [ + LLMResponse(content="", tool_calls=[_call(index, path)]) + for index, path in enumerate(paths) + ] + responses.append(LLMResponse(content="done")) + llm = SequenceLLM(responses) + result = await run_agent_loop( + system_prompt="system", + user_message="look", + llm=llm, + tools=[ShotTool(images_per_call)], + config=_config(), + model_profile=profile, + history_policy=policy or HistoryPolicy(), + ) + return llm, result + + +def _tool_messages(messages: list[dict[str, Any]]) -> list[dict[str, Any]]: + return [m for m in messages if m.get("role") == "tool"] + + +@pytest.mark.asyncio +async def test_image_reaches_history_as_a_content_part() -> None: + _, result = await _run( + ModelProfile(model_id="apodex-1.1-mini", provider="apodex", supports_images=True), + paths=["/tmp/a.png"], + ) + (message,) = _tool_messages(result.messages) + content = message["content"] + assert isinstance(content, list) + assert content[0]["type"] == "text" + assert "/tmp/a.png" in content[0]["text"] + assert content[1]["image_url"]["url"].startswith("data:image/png;base64,") + + +@pytest.mark.asyncio +async def test_the_image_is_still_there_on_the_next_request() -> None: + """A tool result is only useful if it survives into the following turn.""" + llm, _ = await _run( + ModelProfile(model_id="apodex-1.1-mini", provider="apodex", supports_images=True), + paths=["/tmp/a.png"], + ) + sent = llm.calls[-1] + images = [ + block + for message in sent + if isinstance(message.get("content"), list) + for block in message["content"] + if isinstance(block, dict) and block.get("type") == "image_url" + ] + assert len(images) == 1 + # And nothing in-process rode along with it. + assert all("image_meta" not in message for message in for_wire(sent)) + + +@pytest.mark.asyncio +async def test_a_text_only_model_gets_the_warning_instead_of_the_bytes() -> None: + _, result = await _run( + ModelProfile(model_id="text-only", provider="openai"), paths=["/tmp/a.png"], + ) + (message,) = _tool_messages(result.messages) + content = message["content"] + assert isinstance(content, str) + assert "base64" not in content + assert "have NOT" in content + + +@pytest.mark.asyncio +async def test_history_is_bounded_by_max_images_in_history() -> None: + profile = ModelProfile( + model_id="apodex-1.1-mini", provider="apodex", supports_images=True, + ) + _, result = await _run( + profile, + paths=[f"/tmp/{index}.png" for index in range(4)], + policy=HistoryPolicy(max_images_in_history=2), + ) + messages = _tool_messages(result.messages) + assert len(messages) == 4 + live = [ + message for message in messages if isinstance(message.get("content"), list) + ] + assert len(live) == 2 + # The two that went are named, not silently missing. + gone = [m for m in messages if isinstance(m.get("content"), str)] + assert all("no longer see it" in m["content"] for m in gone) + assert "/tmp/0.png" in gone[0]["content"] + + +@pytest.mark.asyncio +async def test_one_result_with_several_images_is_not_self_evicting() -> None: + """A multi-page read must not push out its own earlier pages first.""" + profile = ModelProfile( + model_id="apodex-1.1-mini", provider="apodex", supports_images=True, + ) + _, result = await _run( + profile, + paths=["/tmp/doc.png"], + policy=HistoryPolicy(max_images_in_history=5), + images_per_call=3, + ) + (message,) = _tool_messages(result.messages) + urls = [ + block + for block in message["content"] + if isinstance(block, dict) and block.get("type") == "image_url" + ] + assert len(urls) == 3 diff --git a/tests/test_tool_content_images.py b/tests/test_tool_content_images.py new file mode 100644 index 0000000..821569a --- /dev/null +++ b/tests/test_tool_content_images.py @@ -0,0 +1,348 @@ +"""Images returned by a tool: envelope parsing, attachment, eviction, cost. + +The behavioural anchor for the whole feature is +``test_withheld_image_says_so_in_the_text`` and its eviction twin. Everything +else here is plumbing; those two encode why the plumbing is shaped this way. +See ``agent_core/runtime/loop/image_attach.py`` for the calibration run. +""" + +from __future__ import annotations + +import base64 +import struct +import zlib +from typing import Any + +import pytest + +from agent_core.messages import for_wire, text_of, tool_msg +from agent_core.runtime.loop.image_attach import ( + attach_images, + evict_old_images, + image_blocks_in, +) +from agent_core.runtime.loop.model_profile import ModelProfile +from agent_core.tokens import estimate_message_tokens +from agent_core.tool_content import ( + MAX_IMAGE_BYTES, + MAX_IMAGES_PER_RESULT, + image_attachment, + image_tokens, + parse_tool_content, + sniff_image_size, + tool_content, +) + + +def _png(width: int, height: int) -> bytes: + """A real, minimal PNG of the requested size (no imaging dependency).""" + def chunk(tag: bytes, body: bytes) -> bytes: + return ( + struct.pack(">I", len(body)) + + tag + + body + + struct.pack(">I", zlib.crc32(tag + body) & 0xFFFFFFFF) + ) + + ihdr = struct.pack(">IIBBBBB", width, height, 8, 2, 0, 0, 0) + raw = b"".join(b"\x00" + b"\x00\x00\x00" * width for _ in range(height)) + return ( + b"\x89PNG\r\n\x1a\n" + + chunk(b"IHDR", ihdr) + + chunk(b"IDAT", zlib.compress(raw)) + + chunk(b"IEND", b"") + ) + + +def _vision_profile() -> ModelProfile: + return ModelProfile( + model_id="apodex-1.1-mini", provider="apodex", supports_images=True, + ) + + +def _blind_profile() -> ModelProfile: + return ModelProfile(model_id="text-only", provider="openai") + + +def _attached(text: str, images: list[dict[str, Any]], profile: ModelProfile): + message = tool_msg(text, "call_1") + attach_images(message, images, profile=profile) + return message + + +# ── envelope ────────────────────────────────────────────────────────────── + + +def test_plain_returns_are_not_envelopes() -> None: + assert parse_tool_content("just text") is None + assert parse_tool_content(None) is None + # A tool returning an ordinary dict that happens to have these keys keeps + # its existing stringify behaviour -- the marker is what opts in. + assert parse_tool_content({"text": "hi", "images": []}) is None + + +def test_envelope_splits_text_from_images() -> None: + envelope = tool_content( + "screenshot:", + images=[image_attachment(_png(64, 32), "image/png", label="/tmp/a.png")], + ) + parsed = parse_tool_content(envelope) + assert parsed is not None + text, images = parsed + assert text == "screenshot:" + assert len(images) == 1 + assert images[0]["label"] == "/tmp/a.png" + assert (images[0]["width"], images[0]["height"]) == (64, 32) + + +@pytest.mark.parametrize( + ("image", "reason"), + [ + ({"mime_type": "image/tiff", "data": "aGk="}, "unsupported type"), + ({"mime_type": "image/png", "data": "not base64!!"}, "not valid base64"), + ({"mime_type": "image/png", "data": ""}, "no base64 payload"), + ({"mime_type": "image/png"}, "no base64 payload"), + ("not-a-dict", "not an object"), + ], +) +def test_a_rejected_image_is_reported_not_dropped(image: Any, reason: str) -> None: + parsed = parse_tool_content(tool_content("body", images=[image])) + assert parsed is not None + text, images = parsed + assert images == [] + assert "not attached" in text + assert reason in text + + +def test_oversized_image_is_rejected_with_its_size() -> None: + payload = base64.b64encode(b"\x89PNG\r\n\x1a\n" + b"\x00" * MAX_IMAGE_BYTES) + parsed = parse_tool_content( + tool_content("body", images=[{"mime_type": "image/png", "data": payload.decode()}]) + ) + assert parsed is not None + text, images = parsed + assert images == [] + assert "per-image cap" in text + + +def test_image_count_is_capped_and_the_overflow_is_named() -> None: + one = image_attachment(_png(8, 8), "image/png") + parsed = parse_tool_content( + tool_content("body", images=[dict(one) for _ in range(MAX_IMAGES_PER_RESULT + 3)]) + ) + assert parsed is not None + text, images = parsed + assert len(images) == MAX_IMAGES_PER_RESULT + assert "more than" in text + + +# ── header sniffing and cost ────────────────────────────────────────────── + + +def test_png_dimensions_come_off_the_header() -> None: + assert sniff_image_size(_png(1920, 4)) == (1920, 4) + + +def test_unreadable_header_still_costs_something() -> None: + assert sniff_image_size(b"not an image") == (0, 0) + # An image whose size cannot be read must not be free; a zero here is how a + # history of images measures as empty to the context guard. + assert image_tokens({}) > 1000 + + +def test_token_estimate_tracks_pixels() -> None: + small = image_tokens({"width": 640, "height": 360}) + large = image_tokens({"width": 1920, "height": 1080}) + assert small == pytest.approx(225, abs=30) + assert large == pytest.approx(2043, abs=200) + + +def test_estimate_counts_attached_images() -> None: + caption = tool_msg("screenshot:", "call_1") + text_only = estimate_message_tokens(caption) + withimage = _attached( + "screenshot:", + [image_attachment(_png(1920, 1080), "image/png")], + _vision_profile(), + ) + assert estimate_message_tokens(withimage) - text_only > 1500 + + +# ── attachment ──────────────────────────────────────────────────────────── + + +def test_attached_image_becomes_an_openai_content_part() -> None: + message = _attached( + "screenshot:", + [image_attachment(_png(64, 32), "image/png", label="/tmp/a.png")], + _vision_profile(), + ) + content = message["content"] + assert isinstance(content, list) + assert content[0] == {"type": "text", "text": "screenshot:"} + assert content[1]["type"] == "image_url" + assert content[1]["image_url"]["url"].startswith("data:image/png;base64,") + # The block itself carries nothing extra: the served endpoint's pydantic + # union rejects an unknown key inside a content part outright. + assert set(content[1]) == {"type", "image_url"} + assert set(content[1]["image_url"]) == {"url"} + + +def test_bookkeeping_never_reaches_the_wire() -> None: + message = _attached( + "screenshot:", [image_attachment(_png(64, 32), "image/png", label="/tmp/a.png")], + _vision_profile(), + ) + assert message["image_meta"][0]["label"] == "/tmp/a.png" + assert "image_meta" not in for_wire([message])[0] + + +def test_withheld_image_says_so_in_the_text() -> None: + """The anchor case: a model that cannot see the image must be told. + + Removing the image block and changing nothing else is what made the + calibration model report a confident, entirely invented reading of the + picture. Silence here is a correctness bug, not a missing feature. + """ + message = _attached( + "screenshot:", + [image_attachment(_png(64, 32), "image/png", label="/tmp/a.png")], + _blind_profile(), + ) + content = message["content"] + assert isinstance(content, str) + assert "screenshot:" in content + assert "have NOT" in content + assert "/tmp/a.png" in content + assert "text-only" in content + + +def test_unsupported_protocol_withholds_rather_than_guessing_a_block_shape() -> None: + profile = ModelProfile( + model_id="claude", provider="anthropic", + supports_images=True, protocol="anthropic", + ) + message = _attached("shot:", [image_attachment(_png(8, 8), "image/png")], profile) + assert isinstance(message["content"], str) + assert "anthropic" in message["content"] + + +def test_no_images_leaves_the_message_exactly_as_it_was() -> None: + message = tool_msg("plain result", "call_1") + attach_images(message, [], profile=_vision_profile()) + assert message == {"content": "plain result", "role": "tool", "tool_call_id": "call_1"} + + +def test_flattening_to_text_leaves_a_mark() -> None: + message = _attached( + "shot:", [image_attachment(_png(8, 8), "image/png")], _vision_profile(), + ) + # Anthropic translation, compaction summaries and the trajectory all read + # content through text_of; an image that flattens to nothing produces a + # transcript claiming a picture was never there. + assert "[image" in text_of(message["content"]) + + +# ── eviction ────────────────────────────────────────────────────────────── + + +def _history(count: int) -> list[Any]: + history: list[Any] = [] + for index in range(count): + history.append( + _attached( + f"shot {index}:", + [image_attachment(_png(16, 16), "image/png", label=f"/tmp/{index}.png")], + _vision_profile(), + ) + ) + return history + + +def test_eviction_keeps_the_newest_images() -> None: + history = _history(5) + assert evict_old_images(history, 2) == 3 + assert [image_blocks_in(m) for m in history] == [0, 0, 0, 1, 1] + + +def test_evicted_image_leaves_a_sentence_naming_it() -> None: + history = _history(2) + evict_old_images(history, 1) + oldest = history[0]["content"] + assert isinstance(oldest, str) + assert "shot 0:" in oldest + assert "/tmp/0.png" in oldest + assert "no longer see it" in oldest + + +def test_eviction_drops_the_bookkeeping_it_no_longer_describes() -> None: + history = _history(3) + evict_old_images(history, 1) + assert "image_meta" not in history[0] + assert len(history[-1]["image_meta"]) == 1 + + +def test_partial_eviction_within_one_message_keeps_the_rest_aligned() -> None: + message = _attached( + "pages:", + [ + image_attachment(_png(16, 16), "image/png", label="/tmp/p1.png"), + image_attachment(_png(16, 16), "image/png", label="/tmp/p2.png"), + ], + _vision_profile(), + ) + assert evict_old_images([message], 1) == 1 + assert image_blocks_in(message) == 1 + # The surviving entry must be the one still present -- p2, the newer. + assert [entry["label"] for entry in message["image_meta"]] == ["/tmp/p2.png"] + assert "/tmp/p1.png" in text_of(message["content"]) + + +def test_eviction_is_idempotent_and_ignores_text_messages() -> None: + history = _history(2) + history.insert(0, tool_msg("no images here", "call_x")) + assert evict_old_images(history, 1) == 1 + assert evict_old_images(history, 1) == 0 + assert history[0]["content"] == "no images here" + + +def test_negative_budget_disables_eviction() -> None: + history = _history(3) + assert evict_old_images(history, -1) == 0 + assert sum(image_blocks_in(m) for m in history) == 3 + + +def test_zero_budget_evicts_everything_but_says_so_each_time() -> None: + history = _history(2) + assert evict_old_images(history, 0) == 2 + assert all(isinstance(m["content"], str) for m in history) + assert all("no longer see it" in m["content"] for m in history) + + +# ── interaction with compaction ─────────────────────────────────────────── + + +def test_stale_bookkeeping_stops_charging_once_the_images_are_gone() -> None: + """A compactor may rewrite content to a string without knowing about images. + + ``compress_tool_results`` flattens tool content through ``text_of`` and + assigns a plain string. The images are gone from the request at that point, + so the estimate must stop charging for them even though ``image_meta`` is + still on the message. + """ + message = _attached( + "shot:", [image_attachment(_png(1920, 1080), "image/png")], _vision_profile(), + ) + assert estimate_message_tokens(message) > 1500 + message["content"] = "…condensed by a compactor…" + assert "image_meta" in message + assert estimate_message_tokens(message) < 100 + + +def test_images_without_bookkeeping_are_still_charged() -> None: + """History restored from a checkpoint written before ``image_meta`` existed.""" + message = _attached( + "shot:", [image_attachment(_png(1920, 1080), "image/png")], _vision_profile(), + ) + del message["image_meta"] + assert estimate_message_tokens(message) > 1000 diff --git a/uv.lock b/uv.lock index 3d5cbca..0d01738 100644 --- a/uv.lock +++ b/uv.lock @@ -50,7 +50,7 @@ wheels = [ [[package]] name = "apodex-agent-core" -version = "0.9.1" +version = "0.10.0" source = { editable = "." } dependencies = [ { name = "anthropic", extra = ["bedrock"] }, From 3205c3ff26c9b983b2c739db37fd2cf85bf03463 Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Tue, 8 Sep 2026 17:18:19 +0800 Subject: [PATCH 2/5] fix(skills): detect a config edit by its contents, not by its clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ExtensionsConfig.has_changed` compared `st_mtime > loaded_mtime`, which is asking a coarse clock to order edits far finer than it can see. Measured on this filesystem: the timestamp advances in 1 ms steps, and two consecutive writes land on an identical mtime about 92% of the time. A skill toggled within a millisecond of the load was therefore invisible to `get_enabled_skills`, and the operator's change simply did not take. The same strict `>` could not see a timestamp moving BACKWARD either, which is the ordinary result of restoring a backup, a `git checkout`, or an `rsync --times` of an older revision: contents changed, config reported no change. And it fired for an identical rewrite, forcing a reload with nothing to reload. Compare a blake2b-128 digest of the bytes instead, taken from the exact bytes that were parsed so the fingerprint and the loaded state can never describe different contents. The file is a small JSON document and `get_enabled_skills` is cached by its callers, so reading it is cheaper than being wrong about it. This was already reaching CI as an intermittent failure in test_skills_loader_reload.py, at a rate that tracked machine speed — the test passed only because the work between its two writes usually spilled into the next millisecond, and a warm suite run did not. Its own `os.utime(+10)` defence never helped: it shifts both timestamps by the same constant. The three regression tests pin both mtimes to one value instead, so they fail deterministically against the old check rather than a quarter of the time, and they cover the backward-moving timestamp and the identical rewrite as well. Verified failing on the previous implementation and passing on this one; 60+ full-suite runs since, with no recurrence. Generated with [Claude Code](https://claude.ai/code) via [Very Happy](https://github.com/Mereithhh/very-happy) Co-Authored-By: Claude --- CHANGELOG.md | 15 ++++++ .../components/skills/extensions_config.py | 45 +++++++++++++---- tests/test_skills_loader_reload.py | 48 +++++++++++++++++++ 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 573b61f..2e97345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,21 @@ Versioning follows [docs/versioning.md](docs/versioning.md). reads as though an image were delivered will be answered as though one were. Products adding their own image paths should preserve this property. +### Fixed + +- `ExtensionsConfig.has_changed` compares a digest of the file's bytes instead + of `st_mtime > loaded_mtime`, so a skill toggled on disk is actually picked up + by `get_enabled_skills`. Timestamps are much coarser than the edits they were + being asked to order: the filesystem clock advances in 1 ms steps and two + consecutive writes collide on a single mtime about 92% of the time, so a + change landing in the same millisecond as the load was invisible. The strict + `>` also could not see a timestamp moving BACKWARD -- restoring a backup, a + `git checkout`, an `rsync --times` of an older revision -- and it reported a + change for an identical rewrite, forcing a reload with nothing to reload. + This surfaced as an intermittent failure in `test_skills_loader_reload.py` + whose rate tracked machine speed; the regression tests now pin both + timestamps to one value and fail deterministically without the fix. + ### Changed - `messages.text_of` renders an `image_url` content block as diff --git a/agent_core/components/skills/extensions_config.py b/agent_core/components/skills/extensions_config.py index 49abe6c..046114c 100644 --- a/agent_core/components/skills/extensions_config.py +++ b/agent_core/components/skills/extensions_config.py @@ -7,10 +7,10 @@ from __future__ import annotations +import hashlib import json import logging import os -from contextlib import suppress from pathlib import Path from typing import Any @@ -56,7 +56,8 @@ class ExtensionsConfig(BaseModel): skills: dict[str, SkillStateConfig] = Field(default_factory=dict) _file_path: Path | None = PrivateAttr(default=None) - _file_mtime: float = PrivateAttr(default=0.0) + # Digest of the bytes this config was parsed from -- see ``has_changed``. + _file_digest: str = PrivateAttr(default="") model_config = {"populate_by_name": True} @@ -79,14 +80,15 @@ def from_file(cls, config_path: str | Path | None = None) -> ExtensionsConfig: return cls() try: - with open(resolved, encoding="utf-8") as f: - data = json.load(f) + raw = resolved.read_bytes() + data = json.loads(raw.decode("utf-8")) _resolve_env_variables(data) logger.info("Loaded extensions config from %s", resolved) instance = cls.model_validate(data) instance._file_path = resolved - with suppress(OSError): - instance._file_mtime = resolved.stat().st_mtime + # Digest the exact bytes that were parsed, so the stored fingerprint + # and the loaded state can never describe different file contents. + instance._file_digest = _digest(raw) return instance except Exception as e: logger.warning("Failed to load extensions config %s: %s", resolved, e) @@ -103,11 +105,33 @@ def source_path(self) -> Path | None: return self._file_path def has_changed(self) -> bool: - """Return True if the backing file has been modified since load.""" + """Return True if the backing file's contents differ from what we hold. + + Compares a digest of the bytes, not the modification time. Two reasons, + both of which bit this code: + + Timestamps are far coarser than the edits they are meant to order. The + filesystem clock here advances in 1 ms steps, and two consecutive writes + land on an identical mtime about 92% of the time -- so a change made + within a millisecond of the load was simply invisible, and an operator + toggling a skill got the old state until something else touched the + file. That was reaching the test suite as an intermittent failure whose + rate tracked how fast the machine happened to be running. + + A strict ``>`` also cannot see a file whose timestamp moves BACKWARD, + which is the normal outcome of restoring a backup, a ``git checkout``, + or an ``rsync --times`` of an older revision. The content changed; the + config went on reporting that it had not. + + The file is a small JSON document and this is called from + ``get_enabled_skills``, which its callers cache -- reading it is cheaper + than being wrong about it. An identical rewrite correctly reports no + change, since nothing needs reloading. + """ if self._file_path is None or not self._file_path.is_file(): return False try: - return self._file_path.stat().st_mtime > self._file_mtime + return _digest(self._file_path.read_bytes()) != self._file_digest except OSError: return False @@ -117,6 +141,11 @@ def is_skill_enabled(self, skill_name: str) -> bool: return state.enabled if state else True +def _digest(raw: bytes) -> str: + """Content fingerprint. Not a security boundary -- just change detection.""" + return hashlib.blake2b(raw, digest_size=16).hexdigest() + + def _resolve_env_variables(obj: Any) -> Any: """Recursively replace $VAR_NAME with environment variable values.""" if isinstance(obj, str) and obj.startswith("$"): diff --git a/tests/test_skills_loader_reload.py b/tests/test_skills_loader_reload.py index 38b799d..14f0db6 100644 --- a/tests/test_skills_loader_reload.py +++ b/tests/test_skills_loader_reload.py @@ -152,3 +152,51 @@ def test_toggle_on_a_fresh_loader_is_not_a_silent_no_op(skill_dir, config_file): def test_toggle_still_reports_a_genuinely_missing_skill(skill_dir, config_file): loader = _loader(skill_dir, config_file) assert loader.toggle_skill("no-such-skill", False) is False + + +def _pin_mtime(path, when: float = 1_700_000_000.0) -> None: + """Force an exact mtime, so a change is invisible to a timestamp check.""" + os.utime(path, (when, when)) + + +def test_change_within_one_clock_tick_is_still_seen(skill_dir, config_file): + """Two edits sharing an mtime must not look like no edit at all. + + This is the real shape of the flake that used to surface here: the + filesystem clock advances in 1 ms steps, consecutive writes collide on a + single value about 92% of the time, and the old ``st_mtime > loaded_mtime`` + check reported "unchanged" for every one of those. It only passed as often + as it did because the work between the two writes usually spilled into the + next millisecond. Pinning both timestamps to the same value reproduces it + every run instead of a quarter of them. + """ + _write(config_file, {"debug": False}) + _pin_mtime(config_file) + loader = _loader(skill_dir, config_file) + assert {s.skill_id for s in loader.get_enabled_skills()} == {"code-review"} + + _write(config_file, {"code-review": False}) + _pin_mtime(config_file) + assert {s.skill_id for s in loader.get_enabled_skills()} == {"debug"} + + +def test_a_timestamp_moving_backward_is_still_a_change(skill_dir, config_file): + """Restoring a backup, a git checkout, an rsync --times of an older tree.""" + _write(config_file, {"debug": False}) + _pin_mtime(config_file, 1_700_000_000.0) + loader = _loader(skill_dir, config_file) + assert {s.skill_id for s in loader.get_enabled_skills()} == {"code-review"} + + _write(config_file, {"code-review": False}) + _pin_mtime(config_file, 1_600_000_000.0) + assert {s.skill_id for s in loader.get_enabled_skills()} == {"debug"} + + +def test_an_identical_rewrite_is_not_a_change(skill_dir, config_file): + """Nothing to reload, so nothing should be reloaded.""" + _write(config_file, {"debug": False}) + loader = _loader(skill_dir, config_file) + assert {s.skill_id for s in loader.get_enabled_skills()} == {"code-review"} + + _write(config_file, {"debug": False}) + assert loader._extensions_config.has_changed() is False From 2627051d917e07a7f16810668ea93b2003d45d9b Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Tue, 8 Sep 2026 17:49:54 +0800 Subject: [PATCH 3/5] fix(trajectory): stop writing image base64 into the trace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_message_to_dict` copied a history message verbatim, which was correct while content was always text. With inline images it means the base64 of every attachment is written into the trajectory again for each turn the image survives in history — a single 1080p screenshot is ~137 KB, so a handful of them turn a readable trace into tens of megabytes of unreadable one. `tool_content.redacted_for_trace` replaces the payload with its size and is used by the observer. It is exposed rather than inlined because the trajectory is not the only thing that writes a message somewhere other than the provider; a log line or an event record wants the same treatment. The block keeps its `image_url` type and states how large the image was instead of vanishing. Same reason the loop narrates an eviction: a trace that shows no image where the model was looking at one misrepresents what the model was answering, and a trace is read precisely when someone is trying to work out why an answer was what it was. Non-image messages are returned unchanged, by identity, so the common path allocates nothing. Generated with [Claude Code](https://claude.ai/code) via [Very Happy](https://github.com/Mereithhh/very-happy) Co-Authored-By: Claude --- CHANGELOG.md | 6 ++ agent_core/components/observers/trajectory.py | 6 +- agent_core/tool_content.py | 56 +++++++++++++++++++ tests/test_tool_content_images.py | 49 ++++++++++++++++ 4 files changed, 116 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e97345..5ca670a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,12 @@ Versioning follows [docs/versioning.md](docs/versioning.md). a 4K one ~8.5K). It previously returned the length of the caption alone, so a history of screenshots measured as nearly empty to the context guard and to every compaction trigger. Estimates for image-free histories are unchanged. +- `TrajectoryFileObserver` writes `[N KB of image data elided from trace]` in + place of an inline image's base64, via the new + `tool_content.redacted_for_trace`. It copied messages verbatim, so one 1080p + screenshot wrote ~137 KB into the trajectory for every turn it survived in + history. The block keeps its `image_url` type and states its size, because a + trace showing no image where the model saw one misreports what was answered. - `Message` gains the in-process key `image_meta`, positionally aligned with the `image_url` blocks in `content`. It is outside `WIRE_MESSAGE_KEYS`, so `for_wire` strips it. It is message-level rather than per-block because a diff --git a/agent_core/components/observers/trajectory.py b/agent_core/components/observers/trajectory.py index b11649b..d1293d4 100644 --- a/agent_core/components/observers/trajectory.py +++ b/agent_core/components/observers/trajectory.py @@ -24,6 +24,7 @@ ToolResult, TurnContext, ) +from agent_core.tool_content import redacted_for_trace _FORMATS: tuple[str, ...] = ("json", "jsonl") _DEFAULT_FORMATS: tuple[str, ...] = _FORMATS @@ -399,7 +400,10 @@ def _message_to_dict(self, m: Any) -> dict | None: is a copy. Anything that isn't a role-bearing dict is dropped. """ if isinstance(m, dict) and m.get("role"): - return dict(m) + # ``redacted_for_trace`` swaps an inline image's base64 for its + # size. Verbatim, one 1080p screenshot writes ~137 KB here for + # every turn it stays in history. + return dict(redacted_for_trace(m)) return None # ── Lifecycle hooks ───────────────────────────────────────────────── diff --git a/agent_core/tool_content.py b/agent_core/tool_content.py index bfcb92c..40a98a9 100644 --- a/agent_core/tool_content.py +++ b/agent_core/tool_content.py @@ -55,6 +55,7 @@ "message_image_meta", "message_image_tokens", "parse_tool_content", + "redacted_for_trace", "sniff_image_size", "tool_content", ] @@ -354,3 +355,58 @@ def message_image_tokens(message: Any) -> int: priced = 0 total += priced or _UNKNOWN_IMAGE_TOKENS return total + + +def redacted_for_trace(message: Any) -> Any: + """A copy of *message* with inline image payloads replaced by a marker. + + For anything that writes a message somewhere other than the provider: a + trajectory file, a log line, an event record. The base64 of a single 1080p + screenshot is ~137 KB, and a trace that copies messages verbatim writes that + again for every turn the image survives in history -- a few screenshots turn + a readable trajectory into tens of megabytes of unreadable one. + + The block KEEPS its ``image_url`` type and gains a stated size, so a reader + can still see that an image was in the request and how big it was. That + matters for the same reason the loop narrates evictions: a trace that shows + no image where the model saw one misrepresents what the model was answering. + + Returns the message unchanged (not a copy) when it carries no inline image, + which is nearly every message. + """ + if not isinstance(message, dict): + return message + typed = cast("dict[str, Any]", message) + content = typed.get("content") + if not isinstance(content, list): + return typed + blocks = cast("list[Any]", content) + if not any( + isinstance(block, dict) + and cast("dict[str, Any]", block).get("type") == "image_url" + for block in blocks + ): + return typed + + redacted: list[Any] = [] + for block in blocks: + entry = cast("dict[str, Any]", block) if isinstance(block, dict) else None + if entry is None or entry.get("type") != "image_url": + redacted.append(block) + continue + url = entry.get("image_url") + raw = str(cast("dict[str, Any]", url).get("url") or "") if isinstance(url, dict) else "" + redacted.append({ + "type": "image_url", + "image_url": {"url": _elided_data_uri(raw)}, + }) + return {**typed, "content": redacted} + + +def _elided_data_uri(url: str) -> str: + """``data:image/png;base64,<...>`` → a same-shaped string stating the size.""" + if not url.startswith("data:") or ";base64," not in url: + return url + prefix, payload = url.split(";base64,", 1) + approx_kb = max(1, (len(payload) * 3 // 4) // 1024) + return f"{prefix};base64,[{approx_kb} KB of image data elided from trace]" diff --git a/tests/test_tool_content_images.py b/tests/test_tool_content_images.py index 821569a..14aff43 100644 --- a/tests/test_tool_content_images.py +++ b/tests/test_tool_content_images.py @@ -346,3 +346,52 @@ def test_images_without_bookkeeping_are_still_charged() -> None: ) del message["image_meta"] assert estimate_message_tokens(message) > 1000 + + +# ── trace redaction ─────────────────────────────────────────────────────── + + +def test_trace_redaction_keeps_the_shape_and_states_the_size() -> None: + from agent_core.tool_content import redacted_for_trace + + message = _attached( + "shot:", [image_attachment(_png(1920, 1080), "image/png")], _vision_profile(), + ) + traced = redacted_for_trace(message) + url = traced["content"][1]["image_url"]["url"] + assert "base64" in url + assert "elided from trace" in url + assert "KB" in url + # Still visibly an image, so a trace does not misrepresent what the model saw. + assert traced["content"][1]["type"] == "image_url" + assert traced["content"][0] == {"type": "text", "text": "shot:"} + # And the real message is untouched. + assert message["content"][1]["image_url"]["url"].startswith("data:image/png;base64,i") + + +def test_trace_redaction_is_a_no_op_for_ordinary_messages() -> None: + from agent_core.tool_content import redacted_for_trace + + plain = tool_msg("no images", "call_1") + assert redacted_for_trace(plain) is plain + assert redacted_for_trace("not a message") == "not a message" + + +def test_the_trajectory_observer_does_not_write_base64() -> None: + import json + import tempfile + from pathlib import Path + + from agent_core.components.observers.trajectory import TrajectoryFileObserver + + message = _attached( + "shot:", [image_attachment(_png(1920, 1080), "image/png")], _vision_profile(), + ) + with tempfile.TemporaryDirectory() as tmp: + observer = TrajectoryFileObserver(Path(tmp)) + rendered = observer._message_to_dict(message) + assert rendered is not None + body = json.dumps(rendered) + assert "elided from trace" in body + # A 1080p PNG is ~137 KB of base64; the trace entry must not carry it. + assert len(body) < 2000 From 1c0e8da8dcd9e42e41d0728ce279fe9bb7927eb6 Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Tue, 8 Sep 2026 20:05:29 +0800 Subject: [PATCH 4/5] fix(images): validate tool attachments before delivery --- CHANGELOG.md | 14 +++- agent_core/components/observers/trajectory.py | 17 +++- agent_core/tool_content.py | 79 +++++++++++++++--- docs/agent-loop-boundary.md | 6 +- tests/test_tool_content_images.py | 82 +++++++++++++++++++ 5 files changed, 176 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ca670a..55de31e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,12 @@ Versioning follows [docs/versioning.md](docs/versioning.md). ### Fixed +- Structured image results now reject Base64-valid data that is not a + recognizable PNG, JPEG, WEBP or GIF, and reject a declared MIME type that + disagrees with the payload. The Apodex endpoint otherwise fails the entire + completion while decoding the bad image. Token-accounting dimensions are + always read from the payload, so stale producer metadata cannot price a 4K + image as one token and bypass the context guard. - `ExtensionsConfig.has_changed` compares a digest of the file's bytes instead of `st_mtime > loaded_mtime`, so a skill toggled on disk is actually picked up by `get_enabled_skills`. Timestamps are much coarser than the edits they were @@ -75,10 +81,10 @@ Versioning follows [docs/versioning.md](docs/versioning.md). every compaction trigger. Estimates for image-free histories are unchanged. - `TrajectoryFileObserver` writes `[N KB of image data elided from trace]` in place of an inline image's base64, via the new - `tool_content.redacted_for_trace`. It copied messages verbatim, so one 1080p - screenshot wrote ~137 KB into the trajectory for every turn it survived in - history. The block keeps its `image_url` type and states its size, because a - trace showing no image where the model saw one misreports what was answered. + `tool_content.redacted_for_trace`. Live tool-result JSON and JSONL entries + record the same redacted shape from `ToolResult.images`; previously that path + kept only the text and omitted that the tool returned an image. The block + keeps its `image_url` type and states its size, without copying the Base64. - `Message` gains the in-process key `image_meta`, positionally aligned with the `image_url` blocks in `content`. It is outside `WIRE_MESSAGE_KEYS`, so `for_wire` strips it. It is message-level rather than per-block because a diff --git a/agent_core/components/observers/trajectory.py b/agent_core/components/observers/trajectory.py index d1293d4..8df03d2 100644 --- a/agent_core/components/observers/trajectory.py +++ b/agent_core/components/observers/trajectory.py @@ -24,7 +24,7 @@ ToolResult, TurnContext, ) -from agent_core.tool_content import redacted_for_trace +from agent_core.tool_content import redacted_for_trace, redacted_tool_result_content _FORMATS: tuple[str, ...] = ("json", "jsonl") _DEFAULT_FORMATS: tuple[str, ...] = _FORMATS @@ -567,7 +567,7 @@ async def on_tool_result( # — that fallback advances ``_tool_results_seen``, so sharing it would # double-count, and a synthesised id matches nothing outside the # snapshot anyway. Empty here means the runtime itself had no id. - self._write_jsonl({ + jsonl_record: dict[str, Any] = { "t": "result", "turn": ctx.turn, "name": result.name, @@ -575,7 +575,12 @@ async def on_tool_result( "result": result.result, "error": result.is_error, "ms": result.duration_ms, - }) + } + if result.images: + # The result event predates message attachment/capability gating, so + # this states what the TOOL returned, not that every profile saw it. + jsonl_record["images"] = redacted_tool_result_content("", result.images) + self._write_jsonl(jsonl_record) if "json" in self._formats: cid = getattr(result, "tool_call_id", "") or "" @@ -588,10 +593,14 @@ async def on_tool_result( ) self._tool_results_seen[ctx.turn] = seen + 1 body = _clip(self._stringify(result.result), _BODY_MAX_CHARS) + rendered_body = f"[error] {body}" if result.is_error else body self._append_message({ "role": "tool", "tool_call_id": cid, - "content": f"[error] {body}" if result.is_error else body, + "content": redacted_tool_result_content( + rendered_body, + result.images, + ), }) self._flush_json() return None diff --git a/agent_core/tool_content.py b/agent_core/tool_content.py index 40a98a9..822107e 100644 --- a/agent_core/tool_content.py +++ b/agent_core/tool_content.py @@ -56,6 +56,7 @@ "message_image_tokens", "parse_tool_content", "redacted_for_trace", + "redacted_tool_result_content", "sniff_image_size", "tool_content", ] @@ -184,12 +185,14 @@ def _validated_image(candidate: Any) -> tuple[dict[str, Any] | None, str]: data = image.get("data") if not isinstance(data, str) or not data: return None, "no base64 payload" - # Validated here rather than at the provider: a bad payload is an HTTP 400 - # that takes the whole turn down, and the turn's other tool results with it. + # Validated here rather than at the provider: a bad payload fails the whole + # completion (HTTP 500 on the calibrated Apodex endpoint), including the + # turn's other tool results. try: - decoded_size = len(base64.b64decode(data, validate=True)) + decoded = base64.b64decode(data, validate=True) except (binascii.Error, ValueError): return None, "payload is not valid base64" + decoded_size = len(decoded) if decoded_size == 0: return None, "payload is empty" if decoded_size > MAX_IMAGE_BYTES: @@ -198,16 +201,27 @@ def _validated_image(candidate: Any) -> tuple[dict[str, Any] | None, str]: f"{MAX_IMAGE_BYTES // (1024 * 1024)} MB per-image cap" ) - accepted: dict[str, Any] = {"mime_type": mime, "data": data} + actual_mime = _sniff_image_mime(decoded) + if actual_mime is None: + return None, "payload is not a recognizable supported image" + if actual_mime != mime: + return None, f"payload is {actual_mime}, not declared {mime}" + width, height = sniff_image_size(decoded) + if width <= 0 or height <= 0: + return None, "image dimensions could not be read" + + accepted: dict[str, Any] = { + "mime_type": mime, + "data": data, + # Always derive accounting dimensions from the payload. A producer's + # stale or incorrect declaration must not turn a 4K image into a + # one-token image and bypass the context-overflow guard. + "width": width, + "height": height, + } label = image.get("label") if isinstance(label, str) and label: accepted["label"] = label - width, height = _declared_size(image) - if width <= 0 or height <= 0: - width, height = sniff_image_size(base64.b64decode(data, validate=True)) - if width > 0 and height > 0: - accepted["width"] = width - accepted["height"] = height return accepted, "" @@ -245,19 +259,32 @@ def sniff_image_size(data: bytes) -> tuple[int, int]: return 0, 0 +def _sniff_image_mime(data: bytes) -> str | None: + """Recognize one of the supported image encodings from its header.""" + if len(data) >= 24 and data[:8] == b"\x89PNG\r\n\x1a\n" and data[12:16] == b"IHDR": + return "image/png" + if len(data) >= 10 and data[:6] in (b"GIF87a", b"GIF89a"): + return "image/gif" + if len(data) >= 25 and data[:4] == b"RIFF" and data[8:12] == b"WEBP": + return "image/webp" + if len(data) >= 11 and data[:2] == b"\xff\xd8": + return "image/jpeg" + return None + + def _webp_size(data: bytes) -> tuple[int, int]: chunk = data[12:16] - if chunk == b"VP8X": + if chunk == b"VP8X" and len(data) >= 30: return ( int.from_bytes(data[24:27], "little") + 1, int.from_bytes(data[27:30], "little") + 1, ) - if chunk == b"VP8 ": + if chunk == b"VP8 " and len(data) >= 30: return ( int.from_bytes(data[26:28], "little") & 0x3FFF, int.from_bytes(data[28:30], "little") & 0x3FFF, ) - if chunk == b"VP8L": + if chunk == b"VP8L" and len(data) >= 25 and data[20] == 0x2F: bits = int.from_bytes(data[21:25], "little") return (bits & 0x3FFF) + 1, ((bits >> 14) & 0x3FFF) + 1 return 0, 0 @@ -403,6 +430,32 @@ def redacted_for_trace(message: Any) -> Any: return {**typed, "content": redacted} +def redacted_tool_result_content( + text: str, + images: list[dict[str, Any]], +) -> str | list[dict[str, Any]]: + """Render returned images for a trace without retaining their Base64. + + This describes what the tool returned. Delivery to the provider remains a + separate loop decision, so trace consumers must not interpret these blocks + as proof that a text-only profile received the pixels. + """ + if not images: + return text + blocks: list[dict[str, Any]] = [] + if text: + blocks.append({"type": "text", "text": text}) + for image in images: + mime = str(image.get("mime_type") or "image/unknown") + payload = str(image.get("data") or "") + prefix = f"data:{mime}" + blocks.append({ + "type": "image_url", + "image_url": {"url": _elided_data_uri(f"{prefix};base64,{payload}")}, + }) + return blocks + + def _elided_data_uri(url: str) -> str: """``data:image/png;base64,<...>`` → a same-shaped string stating the size.""" if not url.startswith("data:") or ";base64," not in url: diff --git a/docs/agent-loop-boundary.md b/docs/agent-loop-boundary.md index dfaa3e1..6cbd0f0 100644 --- a/docs/agent-loop-boundary.md +++ b/docs/agent-loop-boundary.md @@ -119,7 +119,11 @@ The split of responsibility: `ModelProfile.supports_images` and `ModelProfile.protocol`, and how many stay in history, from `HistoryPolicy.max_images_in_history`. Products do not pre-filter on capability: `attach_images` is called unconditionally and writes - the withheld note itself. + the withheld note itself. Before that decision, core verifies that the decoded + bytes have a supported image header matching the declared MIME type. It also + derives dimensions from those bytes rather than trusting producer metadata; + the dimensions drive context-budget accounting and cannot be allowed to + understate the actual image. `ToolResult.images` is populated by core (`tool_exec` parses the envelope) and consumed by core (`agent_loop` builds the message). It is not a host-supplied diff --git a/tests/test_tool_content_images.py b/tests/test_tool_content_images.py index 14aff43..b3c4a0e 100644 --- a/tests/test_tool_content_images.py +++ b/tests/test_tool_content_images.py @@ -114,6 +114,24 @@ def test_a_rejected_image_is_reported_not_dropped(image: Any, reason: str) -> No assert reason in text +def test_base64_valid_non_image_is_rejected_before_the_provider() -> None: + image = image_attachment(b"not an image", "image/png") + parsed = parse_tool_content(tool_content("image follows", images=[image])) + assert parsed is not None + text, images = parsed + assert images == [] + assert "not a recognizable supported image" in text + + +def test_declared_mime_must_match_the_payload() -> None: + image = image_attachment(_png(8, 8), "image/jpeg") + parsed = parse_tool_content(tool_content("image follows", images=[image])) + assert parsed is not None + text, images = parsed + assert images == [] + assert "payload is image/png, not declared image/jpeg" in text + + def test_oversized_image_is_rejected_with_its_size() -> None: payload = base64.b64encode(b"\x89PNG\r\n\x1a\n" + b"\x00" * MAX_IMAGE_BYTES) parsed = parse_tool_content( @@ -157,6 +175,25 @@ def test_token_estimate_tracks_pixels() -> None: assert large == pytest.approx(2043, abs=200) +def test_payload_dimensions_override_an_incorrect_declaration() -> None: + envelope = tool_content( + "screenshot:", + images=[ + image_attachment( + _png(1920, 1080), + "image/png", + width=1, + height=1, + ) + ], + ) + parsed = parse_tool_content(envelope) + assert parsed is not None + _, images = parsed + assert (images[0]["width"], images[0]["height"]) == (1920, 1080) + assert image_tokens(images[0]) > 1500 + + def test_estimate_counts_attached_images() -> None: caption = tool_msg("screenshot:", "call_1") text_only = estimate_message_tokens(caption) @@ -395,3 +432,48 @@ def test_the_trajectory_observer_does_not_write_base64() -> None: assert "elided from trace" in body # A 1080p PNG is ~137 KB of base64; the trace entry must not carry it. assert len(body) < 2000 + + +@pytest.mark.asyncio +async def test_live_trajectory_records_returned_images_without_base64(tmp_path) -> None: + import json + from types import SimpleNamespace + + from agent_core.components.observers.trajectory import TrajectoryFileObserver + from agent_core.loop_types import LoopConfig, ToolResult + + attachment = image_attachment( + _png(64, 32), + "image/png", + label="/tmp/live.png", + ) + observer = TrajectoryFileObserver( + tmp_path, + filename="live", + formats=["json", "jsonl"], + ) + await observer.on_loop_start(LoopConfig(task_id="probe")) + await observer.on_tool_result( + SimpleNamespace(turn=1), + ToolResult( + name="view_image", + args={"path": "/tmp/live.png"}, + result="image follows", + duration_ms=1, + tool_call_id="call_1", + is_error=False, + images=[attachment], + ), + ) + observer._flush_json(force=True) + + envelope = json.loads((tmp_path / "live.json").read_text(encoding="utf-8")) + content = envelope["messages"][-1]["content"] + assert content[1]["type"] == "image_url" + assert "elided from trace" in content[1]["image_url"]["url"] + + jsonl = (tmp_path / "live.jsonl").read_text(encoding="utf-8") + assert '"images"' in jsonl + assert "elided from trace" in jsonl + assert attachment["data"] not in json.dumps(envelope) + assert attachment["data"] not in jsonl From 246a3ba07bb5c37d678d4796bc9e1d75d59a783d Mon Sep 17 00:00:00 2001 From: zhanghanduo Date: Tue, 8 Sep 2026 20:16:48 +0800 Subject: [PATCH 5/5] test(images): cover valid and malformed tool payloads --- CHANGELOG.md | 10 ++- agent_core/tool_content.py | 51 ++++++++++++- docs/agent-loop-boundary.md | 3 +- tests/test_agent_loop_image_results.py | 38 ++++++++++ tests/test_tool_content_images.py | 100 +++++++++++++++++++++++++ 5 files changed, 194 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55de31e..6e0af00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,10 +46,12 @@ Versioning follows [docs/versioning.md](docs/versioning.md). - Structured image results now reject Base64-valid data that is not a recognizable PNG, JPEG, WEBP or GIF, and reject a declared MIME type that - disagrees with the payload. The Apodex endpoint otherwise fails the entire - completion while decoding the bad image. Token-accounting dimensions are - always read from the payload, so stale producer metadata cannot price a 4K - image as one token and bypass the context guard. + disagrees with the payload. Lightweight container checks also reject common + truncation and corruption before delivery (including PNG chunk/CRC damage). + The Apodex endpoint otherwise fails the entire completion while decoding the + bad image. Token-accounting dimensions are always read from the payload, so + stale producer metadata cannot price a 4K image as one token and bypass the + context guard. - `ExtensionsConfig.has_changed` compares a digest of the file's bytes instead of `st_mtime > loaded_mtime`, so a skill toggled on disk is actually picked up by `get_enabled_skills`. Timestamps are much coarser than the edits they were diff --git a/agent_core/tool_content.py b/agent_core/tool_content.py index 822107e..4fcf1a2 100644 --- a/agent_core/tool_content.py +++ b/agent_core/tool_content.py @@ -41,6 +41,7 @@ import base64 import binascii import logging +import zlib from typing import Any, cast logger = logging.getLogger(__name__) @@ -150,11 +151,13 @@ def parse_tool_content(raw: Any) -> tuple[str, list[dict[str, Any]]] | None: text = text if isinstance(text, str) else ("" if text is None else str(text)) raw_images = envelope.get("images") - candidates: list[Any] = ( - list(cast("list[Any]", raw_images)) if isinstance(raw_images, list) else [] - ) + candidates: list[Any] = [] images: list[dict[str, Any]] = [] rejected: list[str] = [] + if isinstance(raw_images, list): + candidates = list(cast("list[Any]", raw_images)) + elif raw_images is not None: + rejected.append("images field (must be a list)") for index, candidate in enumerate(candidates): if len(images) >= MAX_IMAGES_PER_RESULT: rejected.append( @@ -206,6 +209,8 @@ def _validated_image(candidate: Any) -> tuple[dict[str, Any] | None, str]: return None, "payload is not a recognizable supported image" if actual_mime != mime: return None, f"payload is {actual_mime}, not declared {mime}" + if not _has_complete_image_container(decoded, actual_mime): + return None, "payload is truncated or structurally invalid" width, height = sniff_image_size(decoded) if width <= 0 or height <= 0: return None, "image dimensions could not be read" @@ -272,6 +277,46 @@ def _sniff_image_mime(data: bytes) -> str | None: return None +def _has_complete_image_container(data: bytes, mime: str) -> bool: + """Reject common truncation/corruption without adding an image decoder.""" + if mime == "image/png": + return _has_complete_png_container(data) + if mime == "image/jpeg": + return data.endswith(b"\xff\xd9") + if mime == "image/gif": + return data.endswith(b"\x3b") + if mime == "image/webp": + return len(data) >= 12 and int.from_bytes(data[4:8], "little") + 8 == len(data) + return False + + +def _has_complete_png_container(data: bytes) -> bool: + """Check PNG chunk bounds, CRCs, ordering anchors, and the final IEND.""" + offset = 8 + saw_header = False + saw_data = False + while offset + 12 <= len(data): + body_size = int.from_bytes(data[offset:offset + 4], "big") + chunk_end = offset + 12 + body_size + if chunk_end > len(data): + return False + chunk_type = data[offset + 4:offset + 8] + body = data[offset + 8:offset + 8 + body_size] + expected_crc = int.from_bytes(data[offset + 8 + body_size:chunk_end], "big") + if zlib.crc32(chunk_type + body) & 0xFFFFFFFF != expected_crc: + return False + if not saw_header: + if chunk_type != b"IHDR" or body_size != 13: + return False + saw_header = True + elif chunk_type == b"IDAT": + saw_data = True + elif chunk_type == b"IEND": + return body_size == 0 and saw_data and chunk_end == len(data) + offset = chunk_end + return False + + def _webp_size(data: bytes) -> tuple[int, int]: chunk = data[12:16] if chunk == b"VP8X" and len(data) >= 30: diff --git a/docs/agent-loop-boundary.md b/docs/agent-loop-boundary.md index 6cbd0f0..b49c6ef 100644 --- a/docs/agent-loop-boundary.md +++ b/docs/agent-loop-boundary.md @@ -120,7 +120,8 @@ The split of responsibility: in history, from `HistoryPolicy.max_images_in_history`. Products do not pre-filter on capability: `attach_images` is called unconditionally and writes the withheld note itself. Before that decision, core verifies that the decoded - bytes have a supported image header matching the declared MIME type. It also + bytes have a supported image header matching the declared MIME type and a + complete lightweight container shape (including PNG chunk CRCs). It also derives dimensions from those bytes rather than trusting producer metadata; the dimensions drive context-budget accounting and cannot be allowed to understate the actual image. diff --git a/tests/test_agent_loop_image_results.py b/tests/test_agent_loop_image_results.py index 9ac01b7..e5cd195 100644 --- a/tests/test_agent_loop_image_results.py +++ b/tests/test_agent_loop_image_results.py @@ -83,6 +83,15 @@ def to_openai_schema(self) -> dict[str, Any]: } +class BrokenShotTool(ShotTool): + async def ainvoke(self, args: dict[str, Any]) -> Any: + path = str(args.get("path") or "/tmp/broken.png") + return tool_content( + f"Image {path} is attached below.", + images=[image_attachment(b"not an image", "image/png", label=path)], + ) + + def _call(index: int, path: str) -> dict[str, Any]: return { "id": f"tc{index}", @@ -172,6 +181,35 @@ async def test_a_text_only_model_gets_the_warning_instead_of_the_bytes() -> None assert "have NOT" in content +@pytest.mark.asyncio +async def test_invalid_image_is_reported_and_never_reaches_the_llm() -> None: + llm = SequenceLLM([ + LLMResponse(content="", tool_calls=[_call(0, "/tmp/broken.png")]), + LLMResponse(content="done"), + ]) + result = await run_agent_loop( + system_prompt="system", + user_message="look", + llm=llm, + tools=[BrokenShotTool()], + config=_config(), + model_profile=ModelProfile( + model_id="apodex-1.1-mini", + provider="apodex", + supports_images=True, + ), + ) + + (history_message,) = _tool_messages(result.messages) + assert isinstance(history_message["content"], str) + assert "not a recognizable supported image" in history_message["content"] + assert "base64" not in history_message["content"] + + (sent_message,) = _tool_messages(llm.calls[-1]) + assert isinstance(sent_message["content"], str) + assert "not a recognizable supported image" in sent_message["content"] + + @pytest.mark.asyncio async def test_history_is_bounded_by_max_images_in_history() -> None: profile = ModelProfile( diff --git a/tests/test_tool_content_images.py b/tests/test_tool_content_images.py index b3c4a0e..1e7cc35 100644 --- a/tests/test_tool_content_images.py +++ b/tests/test_tool_content_images.py @@ -54,6 +54,32 @@ def chunk(tag: bytes, body: bytes) -> bytes: ) +_GIF_2X3 = base64.b64decode( + "R0lGODlhAgADAPAAAP8AAAAAACH5BAAAAAAALAAAAAACAAMAAAIChF8AOw==" +) +_JPEG_2X3 = base64.b64decode( + "/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgG" + "BgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/2wBDAQMD" + "AwQDBAgEBAgQCwkLEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQ" + "EBAQEBAQEBAQEBAQEBD/wAARCAADAAIDAREAAhEBAxEB/8QAFAABAAAAAAAAAAAAAA" + "AAAAAACP/EABQQAQAAAAAAAAAAAAAAAAAAAAD/xAAVAQEBAAAAAAAAAAAAAAAAAAAHC" + "f/EABQRAQAAAAAAAAAAAAAAAAAAAAD/2gAMAwEAAhEDEQA/ADoDFU3/2Q==" +) +_WEBP_2X3 = base64.b64decode( + "UklGRjwAAABXRUJQVlA4IDAAAADQAQCdASoCAAMAAgA0JaACdLoB+AADsAD+8MQL" + "/yC5YXXI1/8gP+QH/ID/+PIAAAA=" +) + + +def _real_images() -> list[tuple[str, bytes]]: + return [ + ("image/png", _png(2, 3)), + ("image/gif", _GIF_2X3), + ("image/jpeg", _JPEG_2X3), + ("image/webp", _WEBP_2X3), + ] + + def _vision_profile() -> ModelProfile: return ModelProfile( model_id="apodex-1.1-mini", provider="apodex", supports_images=True, @@ -132,6 +158,80 @@ def test_declared_mime_must_match_the_payload() -> None: assert "payload is image/png, not declared image/jpeg" in text +@pytest.mark.parametrize(("mime", "payload"), _real_images()) +def test_each_supported_real_image_format_is_accepted( + mime: str, + payload: bytes, +) -> None: + parsed = parse_tool_content( + tool_content("image follows", images=[image_attachment(payload, mime)]) + ) + assert parsed is not None + text, images = parsed + assert text == "image follows" + assert len(images) == 1 + assert images[0]["mime_type"] == mime + assert (images[0]["width"], images[0]["height"]) == (2, 3) + + +@pytest.mark.parametrize(("mime", "payload"), _real_images()) +def test_truncated_real_images_are_rejected( + mime: str, + payload: bytes, +) -> None: + parsed = parse_tool_content( + tool_content("image follows", images=[image_attachment(payload[:-1], mime)]) + ) + assert parsed is not None + text, images = parsed + assert images == [] + assert "truncated or structurally invalid" in text + + +def test_corrupt_png_crc_is_rejected() -> None: + payload = bytearray(_png(2, 3)) + payload[-1] ^= 0x01 + parsed = parse_tool_content( + tool_content( + "image follows", + images=[image_attachment(bytes(payload), "image/png")], + ) + ) + assert parsed is not None + text, images = parsed + assert images == [] + assert "structurally invalid" in text + + +def test_malformed_images_field_is_reported() -> None: + parsed = parse_tool_content({ + "__tool_content__": 1, + "text": "image follows", + "images": {"mime_type": "image/png", "data": "aGk="}, + }) + assert parsed is not None + text, images = parsed + assert images == [] + assert "images field (must be a list)" in text + + +def test_mixed_valid_and_invalid_images_keep_the_valid_one_and_report_the_other() -> None: + parsed = parse_tool_content( + tool_content( + "two images", + images=[ + image_attachment(_png(2, 3), "image/png", label="valid"), + image_attachment(b"not an image", "image/png", label="invalid"), + ], + ) + ) + assert parsed is not None + text, images = parsed + assert [image["label"] for image in images] == ["valid"] + assert "image 2" in text + assert "not a recognizable supported image" in text + + def test_oversized_image_is_rejected_with_its_size() -> None: payload = base64.b64encode(b"\x89PNG\r\n\x1a\n" + b"\x00" * MAX_IMAGE_BYTES) parsed = parse_tool_content(