From b4410b4baddbc83732241c655ff39ac72ffba865 Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Sun, 27 Sep 2026 01:12:38 -0400 Subject: [PATCH] fix(tools): spill MCP result envelopes as pageable text An MCP tool result reaches the model as the handler envelope `{"result": , ...}` (tools/mcp_tool_handlers.py::_render_call_tool_result) so structured metadata survives inline delivery. When that string crossed the persistence threshold it was written to $HERMES_HOME/cache/spillover verbatim, so a ~200 KB document landed on ONE line with every newline escaped (`\n`), making the read_file offset/limit pagination the block recommends unusable. maybe_persist_tool_result now unwraps that envelope before persisting: the spill file and the preview carry the model-facing text with real newlines. The envelope is recognized by SHAPE (a JSON object whose keys are a subset of {"result", "structuredContent", "_meta"} with a non-empty string "result") rather than by tool name, so opaque JSON from any other tool is still persisted verbatim -- and the aggregate path is covered too, since enforce_turn_budget persists under __budget_enforcement__ where a `mcp__` prefix test would miss exactly the results it has to fix. Sibling members (structuredContent/_meta) are appended after the text in a delimited metadata block instead of being dropped: they are the payloads _render_call_tool_result keeps for the model on purpose (#115430), and the spill file is the only copy left once the envelope is replaced by the preview. Fixes #90426 --- tests/tools/test_tool_result_storage.py | 114 ++++++++++++++++++++++++ tools/tool_result_storage.py | 60 +++++++++++-- 2 files changed, 169 insertions(+), 5 deletions(-) diff --git a/tests/tools/test_tool_result_storage.py b/tests/tools/test_tool_result_storage.py index 6d9b228ce3..f7b8c812da 100644 --- a/tests/tools/test_tool_result_storage.py +++ b/tests/tools/test_tool_result_storage.py @@ -1,5 +1,6 @@ """Tests for tools/tool_result_storage.py -- 3-layer tool result persistence.""" +import json import os import subprocess import sys @@ -16,6 +17,7 @@ from tools.tool_result_storage import ( PERSISTED_OUTPUT_CLOSING_TAG, STORAGE_DIR, _build_persisted_message, + _pageable_text, _resolve_storage_dir, _safe_result_filename, _write_to_sandbox, @@ -515,3 +517,115 @@ class TestSpillover: assert (spill_dir / "tc_prune_1.txt").exists() # ── recovery hint in the persisted preview ──────────────────────────── + +# ── MCP envelope unwrapping (#90426) ────────────────────────────────── + +class TestPageableText: + """Only the MCP handler's own envelope shape is unwrapped; every other JSON stays opaque.""" + + def test_single_result_envelope_is_unwrapped(self): + assert _pageable_text(json.dumps({"result": "line one\nline two"})) == "line one\nline two" + + def test_envelope_metadata_is_kept_after_the_text(self): + out = _pageable_text(json.dumps({"result": "text\n", "structuredContent": {"answer": 42}})) + text, marker, tail = out.partition("\n\n\n") + assert text == "text\n" + assert marker + assert json.loads(tail.split("\n")[0]) == {"structuredContent": {"answer": 42}} + + @pytest.mark.parametrize( + "content", + [ + json.dumps({"output": "line\n", "exit_code": 0}), # ordinary tool JSON + json.dumps({"result": "line\n", "exit_code": 0}), # unknown sibling key + json.dumps([{"result": "line\n"}]), # not an object + json.dumps({"result": {"blob": "line\n"}}), # structuredContent-only style + json.dumps({"structuredContent": {"a": 1}}), # no model-facing text at all + json.dumps({"result": ""}), # empty text + "not json at all", + "plain text result", + ], + ) + def test_unrecognized_content_is_verbatim(self, content): + assert _pageable_text(content) == content + +class TestMcpEnvelopeSpillover: + """An oversized MCP result spills its pageable text, not one escaped JSON line (#90426).""" + + @pytest.fixture(autouse=True) + def _isolated_home(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + import tools.tool_result_storage as trs + monkeypatch.setattr(trs, "_spillover_pruned_homes", set()) + yield + + def test_envelope_spills_real_newlines(self): + markdown = "# Research guide\n\n" + "pageable content\n" * 4_000 + envelope = json.dumps({"result": markdown}, ensure_ascii=False) + assert len(envelope) > 30_000 + + result = maybe_persist_tool_result( + content=envelope, tool_name="mcp__archive__read_guide", tool_use_id="tc_mcp_text", + env=None, threshold=30_000) + + assert PERSISTED_OUTPUT_TAG in result + spill_file = get_spillover_dir() / "tc_mcp_text.txt" + assert spill_file.read_text(encoding="utf-8") == markdown + # read_file offset/limit is only usable if the preview is the text too. + preview = result.split("Preview (first", 1)[1] + assert "# Research guide" in preview + assert "\\n" not in preview + + def test_metadata_survives_after_the_text(self): + markdown = "body line\n" * 5_000 + envelope = json.dumps({"result": markdown, "structuredContent": {"count": 5_000}}) + maybe_persist_tool_result( + content=envelope, tool_name="mcp__archive__structured", tool_use_id="tc_mcp_meta", + env=None, threshold=30_000) + + spill = (get_spillover_dir() / "tc_mcp_meta.txt").read_text(encoding="utf-8") + text, _, tail = spill.partition("\n\n\n") + assert text == markdown + assert json.loads(tail.split("\n")[0]) == {"structuredContent": {"count": 5_000}} + + def test_non_envelope_json_still_verbatim(self): + content = json.dumps({"output": "line\n" * 8_000, "exit_code": 0}) + maybe_persist_tool_result( + content=content, tool_name="terminal", tool_use_id="tc_json_verbatim", + env=None, threshold=30_000) + assert (get_spillover_dir() / "tc_json_verbatim.txt").read_text(encoding="utf-8") == content + + def test_structured_content_only_result_still_verbatim(self): + content = json.dumps({"result": {"blob": "z" * 40_000}}) + maybe_persist_tool_result( + content=content, tool_name="mcp__archive__opaque", tool_use_id="tc_mcp_opaque", + env=None, threshold=30_000) + assert (get_spillover_dir() / "tc_mcp_opaque.txt").read_text(encoding="utf-8") == content + + def test_sandbox_write_carries_pageable_text(self): + env = MagicMock() # not a LocalEnvironment -> remote path + env.execute.side_effect = [ + {"output": "", "returncode": 1}, # mounted-path probe: not readable + {"output": "", "returncode": 0}, # cat > sandbox path + {"output": "", "returncode": 1}, # wc -c probe: no answer -> best effort + ] + env.get_temp_dir.return_value = "" + markdown = "remote line\n" * 5_000 + maybe_persist_tool_result( + content=json.dumps({"result": markdown}), tool_name="mcp__archive__remote", + tool_use_id="tc_mcp_remote", env=env, threshold=30_000) + assert env.execute.call_args_list[1][1]["stdin_data"] == markdown + + def test_turn_budget_spill_unwraps_envelope(self): + """The aggregate layer persists under __budget_enforcement__, so a tool-name gate would + miss the very results it must fix; the shape gate still applies.""" + markdown = "budgeted line\n" * 3_000 + msgs = [{ + "role": "tool", "name": "mcp__archive__read", "tool_call_id": "tc_budget_mcp", + "content": json.dumps({"result": markdown}), + }] + + enforce_turn_budget(msgs, env=None, config=BudgetConfig(turn_budget=10_000)) + + assert PERSISTED_OUTPUT_TAG in msgs[0]["content"] + assert (get_spillover_dir() / "tc_budget_mcp.txt").read_text(encoding="utf-8") == markdown diff --git a/tools/tool_result_storage.py b/tools/tool_result_storage.py index 958d22a7bd..b35166a39b 100644 --- a/tools/tool_result_storage.py +++ b/tools/tool_result_storage.py @@ -6,6 +6,7 @@ ran a terminal), remote backends get the translated in-sandbox path (probed for else a copy in the sandbox temp dir; (3) ``enforce_turn_budget``.""" import hashlib +import json import logging import os import re @@ -23,6 +24,11 @@ STORAGE_DIR = os.path.join(tempfile.gettempdir(), "hermes-results") SPILLOVER_SUBDIR = "cache/spillover" SPILLOVER_MAX_AGE_HOURS = 24 _BUDGET_TOOL_NAME = "__budget_enforcement__" +# The exact key set tools/mcp_tool_handlers.py::_render_call_tool_result emits. A JSON object whose +# keys stay inside this set is that handler's own envelope, never an arbitrary tool's JSON payload. +_MCP_ENVELOPE_KEYS = frozenset({"result", "structuredContent", "_meta"}) +_ENVELOPE_METADATA_TAG = "" +_ENVELOPE_METADATA_CLOSING_TAG = "" _UNSAFE_RESULT_FILENAME_CHARS = re.compile(r"[^A-Za-z0-9_.-]+") _MAX_RESULT_FILENAME_STEM = 120 @@ -173,6 +179,47 @@ def generate_preview(content: str, max_chars: int = DEFAULT_PREVIEW_SIZE_CHARS) return content[:last_nl + 1 if last_nl > max_chars // 2 else max_chars], True +def _pageable_text(content: str) -> str: + """Rewrite an MCP handler result envelope into the text it wraps, else return *content*. + + ``tools/mcp_tool_handlers.py::_render_call_tool_result`` hands the model a JSON string + (``{"result": , ...}``) so structured metadata survives inline delivery. Persisting + that string verbatim put a multi-hundred-KB document on ONE line with escaped newlines, so + the ``read_file`` offset/limit pagination the ```` block recommends could + not be used at all (#90426). + + Recognized by SHAPE, not by tool name: the envelope is the only JSON object in the codebase + whose keys are a subset of ``{"result", "structuredContent", "_meta"}`` with a non-empty + string ``result``. That keeps opaque JSON from any other tool verbatim, and it also covers + the aggregate path (``enforce_turn_budget`` persists under ``_BUDGET_TOOL_NAME``, so a + ``mcp__``-prefix test would miss exactly the results it has to fix). + + Sibling members are appended after the text in a delimited metadata block instead of being + dropped: they are the structured payloads ``_render_call_tool_result`` deliberately keeps + for the model (#115430), and the spill file is the only copy left once the envelope is + replaced by the preview. Anything unrecognized — unparseable JSON, a non-object, an unknown + key, a missing/empty/non-string ``result`` (e.g. a structuredContent-only result, which has + no pageable text) — is persisted verbatim, exactly as before. + """ + try: + payload = json.loads(content) + except (TypeError, ValueError): + return content + if not isinstance(payload, dict) or not payload or not set(payload) <= _MCP_ENVELOPE_KEYS: + return content + text = payload.get("result") + if not isinstance(text, str) or not text: + return content + extras = {key: value for key, value in payload.items() if key != "result"} + if not extras: + return text + try: + metadata = json.dumps(extras, ensure_ascii=False, indent=1, sort_keys=True, default=str) + except (TypeError, ValueError): + return text + return f"{text}\n\n{_ENVELOPE_METADATA_TAG}\n{metadata}\n{_ENVELOPE_METADATA_CLOSING_TAG}\n" + + def _write_to_sandbox(content: str, remote_path: str, env) -> bool: """Write content into the sandbox via env.execute(); True on success. Content goes through stdin, not the command string: Linux ``MAX_ARG_STRLEN`` caps one argv element at 128 KB, @@ -253,16 +300,19 @@ def maybe_persist_tool_result(content: str, tool_name: str, tool_use_id: str, en threshold = config.resolve_threshold(tool_name) if threshold == float("inf") or len(content) <= threshold: return content + # The size decision above stays on the raw inline result (that is what cost context); the file + # and the preview carry the pageable text inside an MCP envelope (#90426). + persisted_content = _pageable_text(content) filename = _safe_result_filename(tool_use_id) - preview, has_more = generate_preview(content, max_chars=config.preview_size) + preview, has_more = generate_preview(persisted_content, max_chars=config.preview_size) def _persisted(path: str, host_suffix: str = "") -> str: logger.info("Persisted large tool result: %s (%s, %d chars -> %s%s)", - tool_name, tool_use_id, len(content), path, host_suffix) - return _build_persisted_message(preview, has_more, len(content), path) + tool_name, tool_use_id, len(persisted_content), path, host_suffix) + return _build_persisted_message(preview, has_more, len(persisted_content), path) # Always persist host-side first: cache/spillover is the single canonical home. - host_path = _write_to_spillover(content, filename) + host_path = _write_to_spillover(persisted_content, filename) host_side = _is_host_side_env(env) if host_side and host_path is not None: return _persisted(host_path) @@ -274,7 +324,7 @@ def maybe_persist_tool_result(content: str, tool_name: str, tool_use_id: str, en return _persisted(visible, f" [host: {host_path}]") remote_path = f"{_resolve_storage_dir(env)}/{filename}" try: - if _write_to_sandbox(content, remote_path, env): + if _write_to_sandbox(persisted_content, remote_path, env): return _persisted(remote_path) except Exception as exc: logger.warning("Sandbox write failed for %s: %s", tool_use_id, exc)