diff --git a/tests/tools/test_tool_result_storage.py b/tests/tools/test_tool_result_storage.py index 4cca1a315b..bdb50a7361 100644 --- a/tests/tools/test_tool_result_storage.py +++ b/tests/tools/test_tool_result_storage.py @@ -196,13 +196,17 @@ class TestMaybePersistToolResult: assert PERSISTED_OUTPUT_TAG in result assert "tc_456.txt" in result assert len(result) < len(content) - env.execute.assert_called_once() def test_persists_full_content_as_is(self): """Content is persisted verbatim — no JSON extraction.""" import json env = MagicMock() - env.execute.return_value = {"output": "", "returncode": 0} + # Readability probe fails -> falls back to the in-sandbox write. + env.execute.side_effect = [ + {"output": "", "returncode": 1}, + {"output": "", "returncode": 0}, + ] + env.get_temp_dir.return_value = "" raw = "line1\nline2\n" * 5_000 content = json.dumps({"output": raw, "exit_code": 0, "error": None}) result = maybe_persist_tool_result( @@ -220,7 +224,11 @@ class TestMaybePersistToolResult: def test_tool_use_id_cannot_escape_storage_dir(self): env = MagicMock() - env.execute.return_value = {"output": "", "returncode": 0} + # Readability probe fails -> in-sandbox write is the reference path. + env.execute.side_effect = [ + {"output": "", "returncode": 1}, + {"output": "", "returncode": 0}, + ] env.get_temp_dir.return_value = "" content = "x" * 60_000 result = maybe_persist_tool_result( @@ -370,11 +378,12 @@ class TestSpillover: assert (get_spillover_dir() / "tc_local_1.txt").exists() env.execute.assert_not_called() - def test_remote_env_still_uses_sandbox_write(self): - """Non-local envs keep the in-sandbox write path.""" + def test_remote_env_probe_success_references_mounted_path(self): + """Remote env: host-side write is canonical; when the sandbox can read + the mounted/synced spillover path, the reference uses it and no + in-sandbox copy is written.""" env = MagicMock() # not a LocalEnvironment - env.execute.return_value = {"output": "", "returncode": 0} - env.get_temp_dir.return_value = "/tmp" + env.execute.return_value = {"output": "", "returncode": 0} # probe OK content = "z" * 60_000 result = maybe_persist_tool_result( content=content, @@ -384,8 +393,34 @@ class TestSpillover: threshold=30_000, ) assert PERSISTED_OUTPUT_TAG in result - env.execute.assert_called_once() - assert not (get_spillover_dir() / "tc_remote_1.txt").exists() + # Canonical host copy always exists now. + assert (get_spillover_dir() / "tc_remote_1.txt").exists() + # Only the readability probe ran — no cat-into-sandbox call. + assert env.execute.call_count == 1 + assert "test -r" in env.execute.call_args[0][0] + + def test_remote_env_probe_failure_falls_back_to_sandbox_write(self): + """Persistent containers without the spillover mount still get a + readable in-sandbox copy.""" + env = MagicMock() + env.execute.side_effect = [ + {"output": "", "returncode": 1}, # probe: not readable + {"output": "", "returncode": 0}, # cat > sandbox path + ] + env.get_temp_dir.return_value = "/tmp" + content = "z" * 60_000 + result = maybe_persist_tool_result( + content=content, + tool_name="terminal", + tool_use_id="tc_remote_2", + env=env, + threshold=30_000, + ) + assert PERSISTED_OUTPUT_TAG in result + assert "/tmp/hermes-results/tc_remote_2.txt" in result + assert env.execute.call_count == 2 + # Host canonical copy exists regardless. + assert (get_spillover_dir() / "tc_remote_2.txt").exists() def test_spillover_write_failure_falls_back_to_inline(self, monkeypatch): import tools.tool_result_storage as trs diff --git a/tools/credential_files.py b/tools/credential_files.py index 92a1e9e346..b429d4efa7 100644 --- a/tools/credential_files.py +++ b/tools/credential_files.py @@ -416,6 +416,11 @@ _CACHE_DIRS: list[tuple[str, str]] = [ ("cache/screenshots", "browser_screenshots"), ("cache/web", "web_cache"), ("cache/delegation", "delegation_cache"), + # Oversized tool results (tools/tool_result_storage.py). Host-side is the + # single canonical location; mounting/syncing it lets remote backends + # read spilled results at the translated path instead of needing a + # separate in-sandbox copy. + ("cache/spillover", "cache/spillover"), # Desktop/clipboard/PDF uploads land in the flat top-level ``images/`` dir # (tui_gateway attach RPCs), not under ``cache/``. Mount it so vision can # reach uploads inside sandbox containers (#69575). No legacy alias exists, diff --git a/tools/tool_result_storage.py b/tools/tool_result_storage.py index acb7b4c29d..47bf3799f8 100644 --- a/tools/tool_result_storage.py +++ b/tools/tool_result_storage.py @@ -11,20 +11,24 @@ Defense against context-window overflow operates at three levels: (registry.get_max_result_size), the full output is persisted and the in-context content is replaced with a preview + file path reference. - Where it lands depends on the backend: + The canonical home is ALWAYS host-side: + ``$HERMES_HOME/cache/spillover/{tool_use_id}.txt`` — alongside the other + Hermes-owned caches (images, audio, documents, ...) instead of littering + the OS temp dir. This needs no sandbox environment, so it also works for + sessions that never ran a terminal command (MCP-only, cron, gateway) — + previously those hit the inline-truncate fallback because + ``get_active_env()`` returned None until the first terminal call created + an environment. - - **Local backend (or no active env):** written host-side to - ``$HERMES_HOME/cache/spillover/{tool_use_id}.txt`` — alongside the - other Hermes-owned caches (images, audio, documents, ...) instead of - littering the OS temp dir. This path needs no sandbox environment, - so it also works for sessions that never ran a terminal command - (MCP-only, cron, gateway) — previously those hit the inline-truncate - fallback because ``get_active_env()`` returned None until the first - terminal call created an environment. - - **Remote backends (docker/ssh/modal/daytona):** written INTO THE - SANDBOX temp dir (for example /tmp/hermes-results/{id}.txt) via - env.execute(), because HERMES_HOME does not exist inside the - container and read_file resolves in-sandbox there. + What the model sees depends on the backend: + + - **Local backend (or no active env):** the host path itself. + - **Remote backends (docker/ssh/modal/daytona):** ``cache/spillover`` is + in the auto-mounted/synced cache-dir list (tools/credential_files.py), + so the reference is the translated in-sandbox path (probed for + readability first). When the sandbox can't see it (e.g. a persistent + container created before spillover joined the mount list), fall back + to writing a copy into the sandbox temp dir via env.execute(). The spillover dir is pruned two ways: the gateway housekeeping loop sweeps it hourly with the other media caches, and a once-per-process @@ -155,6 +159,41 @@ def _write_to_spillover(content: str, filename: str): return str(path) +def _sandbox_visible_spillover_path(host_path: str, env) -> str | None: + """Return the path where a remote backend can read *host_path*, or None. + + ``cache/spillover`` is one of the auto-mounted/synced cache dirs + (tools/credential_files.py), so on docker it is bind-mounted and on + modal/ssh/daytona it is file-synced into the sandbox. Translate the + host path with the same helper the image tools use, force a sync for + synced backends, then PROBE readability — a persistent docker + container created before spillover joined the mount list won't have + the bind mount, and must fall back to the in-sandbox write. + """ + try: + from tools.credential_files import to_agent_visible_cache_path + + visible = to_agent_visible_cache_path(host_path) + except Exception as exc: + logger.debug("Spillover path translation failed: %s", exc) + return None + + sync_manager = getattr(env, "_sync_manager", None) + if sync_manager is not None: + try: + sync_manager.sync(force=True) + except Exception as exc: + logger.debug("Spillover sync failed: %s", exc) + + try: + result = env.execute(f"test -r {shlex.quote(visible)}", timeout=15) + if result.get("returncode", 1) == 0: + return visible + except Exception as exc: + logger.debug("Spillover readability probe failed: %s", exc) + return None + + def _resolve_storage_dir(env) -> str: """Return the best temp-backed storage dir for this environment.""" if env is not None: @@ -287,12 +326,12 @@ def maybe_persist_tool_result( filename = _safe_result_filename(tool_use_id) preview, has_more = generate_preview(content, max_chars=config.preview_size) - # Host-side path: no active sandbox env (MCP-only / cron / gateway - # sessions that never ran a terminal command) or the local backend. - # Write into $HERMES_HOME/cache/spillover with the other Hermes-owned - # caches instead of shelling out to /tmp. + # Always persist host-side first: $HERMES_HOME/cache/spillover is the + # single canonical home for spilled results (with the other Hermes-owned + # caches, pruned by gateway housekeeping) regardless of backend. + host_path = _write_to_spillover(content, filename) + if _is_host_side_env(env): - host_path = _write_to_spillover(content, filename) if host_path is not None: logger.info( "Persisted large tool result: %s (%s, %d chars -> %s)", @@ -300,6 +339,19 @@ def maybe_persist_tool_result( ) return _build_persisted_message(preview, has_more, len(content), host_path) elif env is not None: + # Remote backend: the spillover dir is auto-mounted (docker) or + # file-synced (modal/ssh/daytona) into the sandbox, so reference the + # translated path when the sandbox can actually read it. + if host_path is not None: + visible = _sandbox_visible_spillover_path(host_path, env) + if visible is not None: + logger.info( + "Persisted large tool result: %s (%s, %d chars -> %s [host: %s])", + tool_name, tool_use_id, len(content), visible, host_path, + ) + return _build_persisted_message(preview, has_more, len(content), visible) + # Fallback: write into the sandbox temp dir (pre-existing containers + # without the spillover mount, translation/probe failures). storage_dir = _resolve_storage_dir(env) remote_path = f"{storage_dir}/{filename}" try: