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.
This commit is contained in:
@@ -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."""
|
||||
|
||||
54
tests/agent/test_multimodal_tool_result_spill.py
Normal file
54
tests/agent/test_multimodal_tool_result_spill.py
Normal file
@@ -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()
|
||||
Reference in New Issue
Block a user