From 876bae4d0f27b455d7718d7f39cdf293a214e375 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 18 Sep 2026 23:58:08 -0700 Subject: [PATCH] fix: spill oversized text parts of multimodal tool results like string results A browser_exec call that captured a screenshot returns a multimodal envelope whose text part carries the full stdout. _finalize_tool_result exempted every multimodal envelope from maybe_persist_tool_result, and on vision-capable routes the part list also slips past enforce_turn_budget (len(list) counts parts, not chars). A 760K-char browser result therefore stayed inline in hot context and was re-sent on every later request (#95429: 2.47 MB requests, repeated no-first-byte stalls on retry). Route each TEXT part (and text_summary) of a multimodal envelope through the same per-tool persistence threshold; image parts stay untouched (their size is already governed by the vision embed budget). Normal-sized envelopes are returned unchanged. Same defect class as PR #95458 (@fangliquanflq), ported minimally. --- agent/tool_executor.py | 32 ++++++++++- .../test_multimodal_tool_result_spill.py | 54 +++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tests/agent/test_multimodal_tool_result_spill.py diff --git a/agent/tool_executor.py b/agent/tool_executor.py index 4b55c1b88d..8afeb67f3b 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -1057,7 +1057,11 @@ def _commit_tool_result( agent._touch_activity(f"tool completed: {function_name} ({tool_duration:.1f}s){_status_suffix}") persisted_result = function_result - if not _is_multimodal_tool_result(persisted_result): + if _is_multimodal_tool_result(persisted_result): + persisted_result = _persist_multimodal_text_parts( + persisted_result, function_name, tool_call_id, get_active_env(effective_task_id), budget, + ) + else: persisted_result = maybe_persist_tool_result( content=persisted_result, tool_name=function_name, @@ -1093,6 +1097,32 @@ def _commit_tool_result( return persisted_result, function_result, tool_message.get("_tool_output_risk") +def _persist_multimodal_text_parts(result: dict, tool_name: str, tool_call_id: str, env, budget: BudgetConfig) -> dict: + """Spill oversized TEXT parts of a multimodal envelope through the same persistence policy as + string results (#95429). A ``browser_exec`` call that captured a screenshot bakes its full + stdout into the envelope's text part, which used to bypass ``maybe_persist_tool_result`` + entirely and ride every later request inline. Image parts are left untouched (their size is + governed by the vision embed budget); a fresh dict is returned so history is never mutated.""" + parts = result.get("content") or [] + bounded_parts, changed = [], False + for part in parts: + text = part.get("text") if isinstance(part, dict) and part.get("type") == "text" else None + if isinstance(text, str): + replaced = maybe_persist_tool_result(content=text, tool_name=tool_name, tool_use_id=tool_call_id, + env=env, config=budget) + if replaced != text: + part, changed = {**part, "text": replaced}, True + bounded_parts.append(part) + if not changed: + return result + bounded = {**result, "content": bounded_parts} + summary = bounded.get("text_summary") + if isinstance(summary, str): + bounded["text_summary"] = maybe_persist_tool_result(content=summary, tool_name=tool_name, + tool_use_id=tool_call_id, env=env, config=budget) + return bounded + + def _finalize_tool_batch(agent, messages: list, effective_task_id: str, num_tools: int, budget: BudgetConfig) -> None: """Per-turn aggregate budget enforcement, then /steer injection — in that order, so the steer marker is never truncated/discarded when enforcement replaces a result.""" diff --git a/tests/agent/test_multimodal_tool_result_spill.py b/tests/agent/test_multimodal_tool_result_spill.py new file mode 100644 index 0000000000..0c573bc839 --- /dev/null +++ b/tests/agent/test_multimodal_tool_result_spill.py @@ -0,0 +1,54 @@ +"""Oversized TEXT parts inside a multimodal tool envelope must go through the same persistence +policy as string results (#95429): a ``browser_exec`` call that captured a screenshot bakes its +whole stdout into the envelope's text part, and that used to bypass ``maybe_persist_tool_result`` +and ride every later provider request inline (760K chars -> multi-megabyte requests).""" + +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from tools.tool_result_storage import PERSISTED_OUTPUT_TAG +from tests.agent.test_tool_call_incremental_persistence import _make_agent, _mock_tool_call + + +def _run_sequential(agent, function_result): + tool_calls = [_mock_tool_call(name="browser_exec", call_id="call_browser")] + messages: list = [] + assistant_message = SimpleNamespace(content="", tool_calls=tool_calls) + agent._flush_messages_to_session_db = MagicMock() + # Vision-capable route (the reporter's case): the envelope stays a part LIST in history, which + # the per-turn aggregate budget cannot measure — so only per-part persistence bounds it. + with (patch("model_tools.handle_function_call", return_value=function_result), + patch.object(type(agent), "_model_supports_vision", lambda self: True), + patch.object(type(agent), "_provider_supports_vision_tool_messages", lambda self: True)): + agent._execute_tool_calls_sequential(assistant_message, messages, "task-1") + return [m for m in messages if m.get("role") == "tool"] + + +def _envelope(text: str) -> dict: + return {"_multimodal": True, "text_summary": text, "meta": {"screenshot_path": "/tmp/shot.png"}, + "content": [{"type": "text", "text": text}, + {"type": "image_url", "image_url": {"url": "data:image/jpeg;base64,QUJD"}}]} + + +def test_oversized_multimodal_text_part_is_spilled_and_recoverable(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: tmp_path, raising=False) + big = "x" * 760_396 + (tool_msg,) = _run_sequential(_make_agent(), _envelope(big)) + content = tool_msg["content"] + assert isinstance(content, list) + texts = [p["text"] for p in content if isinstance(p, dict) and p.get("type") == "text"] + assert len(texts) == 1 and PERSISTED_OUTPUT_TAG in texts[0] and len(texts[0]) < 10_000 + assert any(p.get("type") == "image_url" for p in content) # image part untouched + # The full text is recoverable from the spillover file. + spilled = list(Path(tmp_path, "cache", "spillover").glob("*")) + assert spilled and spilled[0].read_text() == big + + +def test_normal_multimodal_result_is_unchanged(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + small = "snapshot ok" + (tool_msg,) = _run_sequential(_make_agent(), _envelope(small)) + assert tool_msg["content"][0] == {"type": "text", "text": small} + assert not Path(tmp_path, "cache", "spillover").exists()