fix(tools): spillover is the canonical home for oversized results on every backend
Per review: even with an active sandbox env, spilled tool results belong in $HERMES_HOME/cache/spillover with the other Hermes-owned caches — not the sandbox temp dir as primary storage. - Host-side write happens first on every backend; local/no-env sessions reference the host path directly (unchanged). - cache/spillover joins the auto-mount/sync cache-dir list (credential_files._CACHE_DIRS), so docker bind-mounts it and modal/ssh/daytona file-sync it. Remote references use the translated in-sandbox path after a readability probe. - Probe failure (persistent containers created before spillover joined the mount list, translation failures) falls back to the previous in-sandbox temp-dir copy, so nothing regresses.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user