hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 28 Packages Projects Releases Wiki Activity

Port from lobehub/lobehub#18258: verify persisted tool-result archives before referencing them

Browse Source 
Oversized tool results are archived to disk and replaced in-context with a
'Full output saved to: <path>' reference. Until now the write was trusted
blind: a partially-flushed host file (ENOSPC/quota races) or a lossy sandbox
write (API-body truncation on payload backends) still produced the archive
reference, so the model was told the full result was recoverable when bytes
had silently vanished.

Both persistence paths now round-trip-verify size before building the
reference and fail closed to the bounded inline truncation otherwise:

- _write_to_spillover: byte-count check via os.stat after write; mismatched
  archives are deleted and the caller falls through to inline truncation.
- _write_to_sandbox: wc -c probe after the cat; heredoc-mode backends get a
  +1 byte tolerance (wrap_modal_stdin_heredoc appends one newline by
  construction), unprobeable backends stay best-effort success.

Regression tests fail without the fix (verified by stashing the source
change: 4 failed). E2E-verified against a temp HERMES_HOME with real file
I/O including multibyte content and a simulated short write.
This commit is contained in:
Teknium
2026-08-19 21:13:33 -07:00
parent 0468acdce6
commit fc93a1b55a
2 changed files with 179 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

137
tests/tools/test_tool_result_storage.py
View File

@@ -49,14 +49,15 @@ class TestWriteToSandbox:
env.execute.return_value = {"output": "", "returncode": 0}
result = _write_to_sandbox("hello world", "/tmp/hermes-results/abc.txt", env)
assert result is True
env.execute.assert_called_once()
cmd = env.execute.call_args[0][0]
# First call is the write; a second call round-trip-verifies the
# persisted size (unparseable probe output = best-effort success).
cmd = env.execute.call_args_list[0][0][0]
assert "mkdir -p" in cmd
# Content travels through stdin, NOT inside the command string —
# otherwise large content would hit Linux's 128 KB MAX_ARG_STRLEN
# ceiling on `bash -c <cmd>` (#22906).
assert "hello world" not in cmd
assert env.execute.call_args[1]["stdin_data"] == "hello world"
assert env.execute.call_args_list[0][1]["stdin_data"] == "hello world"
def test_large_content_via_stdin(self):
@@ -66,9 +67,9 @@ class TestWriteToSandbox:
env.execute.return_value = {"output": "", "returncode": 0}
big = "x" * 200_000
_write_to_sandbox(big, "/tmp/hermes-results/big.txt", env)
cmd = env.execute.call_args[0][0]
cmd = env.execute.call_args_list[0][0][0]
assert len(cmd) < 1_000 # cmd is just `mkdir -p X && cat > Y`
assert env.execute.call_args[1]["stdin_data"] == big
assert env.execute.call_args_list[0][1]["stdin_data"] == big
def test_path_with_spaces_is_quoted(self):
@@ -76,7 +77,7 @@ class TestWriteToSandbox:
env.execute.return_value = {"output": "", "returncode": 0}
remote_path = "/tmp/hermes results/abc file.txt"
_write_to_sandbox("content", remote_path, env)
cmd = env.execute.call_args[0][0]
cmd = env.execute.call_args_list[0][0][0]
assert "'/tmp/hermes results'" in cmd
assert "'/tmp/hermes results/abc file.txt'" in cmd
@@ -86,7 +87,7 @@ class TestWriteToSandbox:
env.execute.return_value = {"output": "", "returncode": 0}
malicious_path = "/tmp/hermes-results/$(whoami).txt"
_write_to_sandbox("content", malicious_path, env)
cmd = env.execute.call_args[0][0]
cmd = env.execute.call_args_list[0][0][0]
# The $() must not appear unquoted — shlex.quote wraps it
assert "'/tmp/hermes-results/$(whoami).txt'" in cmd
@@ -95,10 +96,73 @@ class TestWriteToSandbox:
env.execute.return_value = {"output": "", "returncode": 0}
malicious_path = "/tmp/x; rm -rf /; echo .txt"
_write_to_sandbox("content", malicious_path, env)
cmd = env.execute.call_args[0][0]
cmd = env.execute.call_args_list[0][0][0]
# The semicolons must be inside quotes, not acting as command separators
assert "'/tmp/x; rm -rf /; echo .txt'" in cmd
def test_size_mismatch_fails_closed_and_removes_archive(self):
"""A sandbox archive that lost bytes must be discarded, not referenced.
Port of lobehub/lobehub#18258: verify the persisted archive before
telling the model the full result is available; fail closed to the
bounded inline truncation when persistence is not lossless.
"""
env = MagicMock()
env._stdin_mode = "pipe"
content = "x" * 1000
env.execute.side_effect = [
{"output": "", "returncode": 0}, # write
{"output": "512\n", "returncode": 0}, # wc -c: short!
{"output": "", "returncode": 0}, # rm -f cleanup
]
assert _write_to_sandbox(content, "/tmp/hermes-results/short.txt", env) is False
rm_cmd = env.execute.call_args_list[2][0][0]
assert rm_cmd.startswith("rm -f ")
assert "/tmp/hermes-results/short.txt" in rm_cmd
def test_exact_size_match_succeeds(self):
env = MagicMock()
env._stdin_mode = "pipe"
content = "abcé" # multi-byte: expected size is the UTF-8 byte count
expected = len(content.encode("utf-8"))
env.execute.side_effect = [
{"output": "", "returncode": 0},
{"output": f"{expected}\n", "returncode": 0},
]
assert _write_to_sandbox(content, "/tmp/hermes-results/ok.txt", env) is True
def test_heredoc_backend_tolerates_single_trailing_newline(self):
"""Heredoc-mode backends append one newline by construction."""
env = MagicMock()
env._stdin_mode = "heredoc"
content = "y" * 100
env.execute.side_effect = [
{"output": "", "returncode": 0},
{"output": "101\n", "returncode": 0},
]
assert _write_to_sandbox(content, "/tmp/hermes-results/hd.txt", env) is True
def test_pipe_backend_rejects_extra_byte(self):
"""The +1 tolerance is heredoc-only; pipe backends must be exact."""
env = MagicMock()
env._stdin_mode = "pipe"
content = "y" * 100
env.execute.side_effect = [
{"output": "", "returncode": 0},
{"output": "101\n", "returncode": 0},
{"output": "", "returncode": 0}, # rm -f
]
assert _write_to_sandbox(content, "/tmp/hermes-results/hd2.txt", env) is False
def test_unprobeable_backend_is_best_effort_success(self):
"""No wc / probe crash must not discard a likely-good archive."""
env = MagicMock()
env.execute.side_effect = [
{"output": "", "returncode": 0},
RuntimeError("exec transport gone"),
]
assert _write_to_sandbox("data", "/tmp/hermes-results/np.txt", env) is True
class TestResolveStorageDir:
def test_defaults_to_storage_dir_without_env(self):
@@ -185,10 +249,12 @@ class TestMaybePersistToolResult:
"""Content is persisted verbatim — no JSON extraction."""
import json
env = MagicMock()
# Readability probe fails -> falls back to the in-sandbox write.
# Readability probe fails -> falls back to the in-sandbox write,
# whose size probe returns unparseable output (best-effort success).
env.execute.side_effect = [
{"output": "", "returncode": 1},
{"output": "", "returncode": 0},
{"output": "", "returncode": 1}, # wc -c size probe: no answer
]
env.get_temp_dir.return_value = ""
raw = "line1\nline2\n" * 5_000
@@ -203,7 +269,7 @@ class TestMaybePersistToolResult:
assert PERSISTED_OUTPUT_TAG in result
# Content is delivered through stdin (no longer embedded in the
# command string — see test_large_content_via_stdin for why).
assert env.execute.call_args[1]["stdin_data"] == content
assert env.execute.call_args_list[1][1]["stdin_data"] == content
def test_tool_use_id_cannot_escape_storage_dir(self):
@@ -212,6 +278,7 @@ class TestMaybePersistToolResult:
env.execute.side_effect = [
{"output": "", "returncode": 1},
{"output": "", "returncode": 0},
{"output": "", "returncode": 1}, # wc -c size probe: no answer
]
env.get_temp_dir.return_value = ""
content = "x" * 60_000
@@ -222,7 +289,7 @@ class TestMaybePersistToolResult:
env=env,
threshold=30_000,
)
cmd = env.execute.call_args[0][0]
cmd = env.execute.call_args_list[1][0][0]
target = cmd.split("cat > ", 1)[1].split(" <<", 1)[0]
assert "Full output saved to: /tmp/hermes-results/outside_whoami_x_" in result
@@ -390,6 +457,7 @@ class TestSpillover:
env.execute.side_effect = [
{"output": "", "returncode": 1}, # probe: not readable
{"output": "", "returncode": 0}, # cat > sandbox path
{"output": "60000\n", "returncode": 0}, # wc -c verification
]
env.get_temp_dir.return_value = "/tmp"
content = "z" * 60_000
@@ -402,7 +470,7 @@ class TestSpillover:
)
assert PERSISTED_OUTPUT_TAG in result
assert "/tmp/hermes-results/tc_remote_2.txt" in result
assert env.execute.call_count == 2
assert env.execute.call_count == 3
# Host canonical copy exists regardless.
assert (get_spillover_dir() / "tc_remote_2.txt").exists()
@@ -483,3 +551,48 @@ class TestRecoveryHint:
assert msg.startswith(PERSISTED_OUTPUT_TAG)
assert msg.endswith(PERSISTED_OUTPUT_CLOSING_TAG)
assert "read_file" in msg
# ── host-side spillover round-trip verification ───────────────────────
# Port of lobehub/lobehub#18258: verify the persisted archive before
# telling the model the full result is available; fail closed otherwise.
class TestSpilloverWriteVerification:
def test_lossless_write_returns_path(self):
from tools.tool_result_storage import _write_to_spillover
path = _write_to_spillover("hello spill", "tc_verify_ok.txt")
assert path is not None
with open(path, encoding="utf-8") as fh:
assert fh.read() == "hello spill"
def test_short_write_is_discarded(self):
"""A partially-flushed archive must not be referenced to the model."""
from unittest.mock import patch as _patch
from tools.tool_result_storage import _write_to_spillover
real_stat = __import__("os").stat
class _ShortStat:
st_size = 3 # pretend only 3 bytes landed
def fake_stat(p, *a, **kw):
if str(p).endswith("tc_verify_short.txt"):
return _ShortStat()
return real_stat(p, *a, **kw)
with _patch("tools.tool_result_storage.os.stat", side_effect=fake_stat):
path = _write_to_spillover("this is much longer than 3 bytes", "tc_verify_short.txt")
assert path is None
assert not (get_spillover_dir() / "tc_verify_short.txt").exists()
def test_multibyte_content_verified_by_byte_count(self):
"""Verification compares UTF-8 bytes, not characters."""
from tools.tool_result_storage import _write_to_spillover
content = "héllo wörld ✓" * 10
path = _write_to_spillover(content, "tc_verify_mb.txt")
assert path is not None
with open(path, encoding="utf-8") as fh:
assert fh.read() == content

58
tools/tool_result_storage.py
View File

@@ -84,15 +84,33 @@ def _is_host_side_env(env) -> bool:
def _write_to_spillover(content: str, filename: str):
"""Write host-side to $HERMES_HOME/cache/spillover; returns path str or None."""
"""Write host-side to $HERMES_HOME/cache/spillover; returns path str or None.
The write is size-verified before the caller tells the model "Full output saved":
a partially-flushed file (quota, ENOSPC race) fails closed to the bounded inline
truncation instead of referencing an archive that silently lost bytes.
"""
data = content.encode("utf-8", errors="replace")
try:
spill_dir = get_spillover_dir()
spill_dir.mkdir(parents=True, exist_ok=True)
path = spill_dir / filename
path.write_text(content, encoding="utf-8", errors="replace")
path.write_bytes(data)
persisted_size = os.stat(path).st_size
except OSError as exc:
logger.warning("Spillover write failed for %s: %s", filename, exc)
return None
if persisted_size != len(data):
logger.warning(
"Spillover write for %s is not lossless (%d bytes on disk, "
"expected %d) — discarding archive",
filename, persisted_size, len(data),
)
try:
path.unlink()
except OSError:
pass
return None
_prune_spillover_once()
return str(path)
@@ -157,10 +175,42 @@ def generate_preview(content: str, max_chars: int = DEFAULT_PREVIEW_SIZE_CHARS)
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,
so a heredoc-in-command silently failed for exactly the oversized results this handles."""
so a heredoc-in-command silently failed for exactly the oversized results this handles.
The write is round-trip verified with ``wc -c`` (one extra exec RTT per oversized result):
a zero exit from ``cat`` does not prove the bytes landed (quota/ENOSPC races, API-body
truncation on payload backends). A measured mismatch removes the archive and fails closed;
an unprobeable backend (no ``wc``, exec error, unparseable output) stays best-effort success.
Heredoc-mode backends append exactly one trailing newline by construction
(``wrap_modal_stdin_heredoc``), so one extra byte is accepted there."""
storage_dir = os.path.dirname(remote_path)
cmd = f"mkdir -p {shlex.quote(storage_dir)} && cat > {shlex.quote(remote_path)}"
return env.execute(cmd, timeout=30, stdin_data=content).get("returncode", 1) == 0
if env.execute(cmd, timeout=30, stdin_data=content).get("returncode", 1) != 0:
return False
expected = len(content.encode("utf-8", errors="replace"))
try:
probe = env.execute(f"wc -c < {shlex.quote(remote_path)}", timeout=15)
except Exception as exc:
logger.debug("Sandbox size probe failed for %s: %s", remote_path, exc)
return True
if probe.get("returncode", 1) != 0:
return True
raw = str(probe.get("output", "") or "").strip().split()
if not raw or not raw[-1].isdigit():
return True
persisted_size = int(raw[-1])
if persisted_size == expected:
return True
if persisted_size == expected + 1 and getattr(env, "_stdin_mode", None) == "heredoc":
return True
logger.warning("Sandbox spill for %s is not lossless (%d bytes in sandbox, expected %d) — discarding archive",
remote_path, persisted_size, expected)
try:
env.execute(f"rm -f {shlex.quote(remote_path)}", timeout=15)
except Exception:
pass
return False
def _build_persisted_message(preview: str, has_more: bool, original_size: int,