mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(terminal): make bounded capture opt-in for the foreground terminal path only
Review finding on the salvaged collector: _wait_for_process is the shared drain for EVERY env.execute() consumer, not just the terminal tool. Applying tool_output.max_bytes there silently truncated file-operation cat reads (read_file_raw feeds the patch engine — read-modify-write on any file >50KB would corrupt it), paginated read_file, code-execution RPC reads, and log reads. bounded_capture is now an explicit opt-in on execute()/_wait_for_process, set only by the foreground terminal tool. Default preserves the historical full-fidelity capture via an effectively-unbounded collector (single code path). Modal transports accept the kwarg for signature parity. New regression test: default execute() returns a 200KB payload complete and untruncated. E2E: 20MB internal read intact; ShellFileOperations read_file_raw round-trips byte-exact; terminal path still bounded at 50KB.
This commit is contained in:
parent
0a07609173
commit
cab457d722
4 changed files with 82 additions and 11 deletions
|
|
@ -101,7 +101,7 @@ class TestBackgroundChildDoesNotHang:
|
|||
"'\\nTAIL-SENTINEL')\""
|
||||
)
|
||||
|
||||
result = local_env.execute(command, timeout=10)
|
||||
result = local_env.execute(command, timeout=10, bounded_capture=True)
|
||||
|
||||
assert result["returncode"] == 0
|
||||
assert len(result["output"]) <= 10_000
|
||||
|
|
@ -109,6 +109,31 @@ class TestBackgroundChildDoesNotHang:
|
|||
assert result["output"].endswith("TAIL-SENTINEL")
|
||||
assert "[OUTPUT TRUNCATED" in result["output"]
|
||||
|
||||
def test_default_capture_is_full_fidelity_for_internal_consumers(
|
||||
self, local_env
|
||||
):
|
||||
"""Default execute() (no bounded_capture) must return complete output.
|
||||
|
||||
Internal consumers — file-operation ``cat`` reads that feed the patch
|
||||
engine, code-execution RPC reads, log reads — rely on full-fidelity
|
||||
capture. Bounding them at tool_output.max_bytes would CORRUPT files
|
||||
on read-modify-write (#64435 review finding), so only the foreground
|
||||
terminal tool opts in via bounded_capture=True.
|
||||
"""
|
||||
# ~200 KB — four times the default 50 KB cap.
|
||||
command = (
|
||||
"python3 -c \"import sys; "
|
||||
"sys.stdout.write('START-MARK\\n' + ('y' * 200000) + '\\nEND-MARK')\""
|
||||
)
|
||||
|
||||
result = local_env.execute(command, timeout=10)
|
||||
|
||||
assert result["returncode"] == 0
|
||||
assert "[OUTPUT TRUNCATED" not in result["output"]
|
||||
assert result["output"].startswith("START-MARK")
|
||||
assert result["output"].endswith("END-MARK")
|
||||
assert len(result["output"]) > 200000
|
||||
|
||||
def test_continuous_output_still_honors_foreground_timeout(
|
||||
self, local_env, monkeypatch
|
||||
):
|
||||
|
|
@ -120,7 +145,7 @@ class TestBackgroundChildDoesNotHang:
|
|||
)
|
||||
|
||||
started = time.monotonic()
|
||||
result = local_env.execute(command, timeout=1)
|
||||
result = local_env.execute(command, timeout=1, bounded_capture=True)
|
||||
elapsed = time.monotonic() - started
|
||||
|
||||
assert elapsed < 4.0
|
||||
|
|
|
|||
|
|
@ -45,9 +45,14 @@ if _DEBUG_INTERRUPT:
|
|||
_activity_callback_local = threading.local()
|
||||
|
||||
|
||||
# Sentinel capacity for full-fidelity capture (internal consumers). Large
|
||||
# enough that the collector never evicts in practice, keeping a single code
|
||||
# path for both bounded and unbounded modes.
|
||||
_UNBOUNDED_CAPTURE_CHARS = 2**63 - 1
|
||||
|
||||
|
||||
class _BoundedOutputCollector:
|
||||
"""Retain a bounded 40/60 head-tail window of streamed text."""
|
||||
|
||||
def __init__(self, max_chars: int):
|
||||
self.max_chars = max(1, int(max_chars))
|
||||
self._head_limit = int(self.max_chars * 0.4)
|
||||
|
|
@ -673,11 +678,21 @@ class BaseEnvironment(ABC):
|
|||
# Process lifecycle
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
def _wait_for_process(self, proc: ProcessHandle, timeout: int = 120) -> dict:
|
||||
def _wait_for_process(
|
||||
self, proc: ProcessHandle, timeout: int = 120, *, bounded_capture: bool = False
|
||||
) -> dict:
|
||||
"""Poll-based wait with interrupt checking and stdout draining.
|
||||
|
||||
Shared across all backends — not overridden.
|
||||
|
||||
``bounded_capture=True`` (foreground terminal-tool path only) retains
|
||||
at most ``tool_output.max_bytes`` of output in a head/tail window
|
||||
while draining, so a verbose subprocess cannot OOM the process
|
||||
(#64435). The default (False) preserves full-fidelity capture for
|
||||
internal consumers — file-operation ``cat`` reads feeding the patch
|
||||
engine, code-execution RPC reads, log reads — where truncation would
|
||||
corrupt data.
|
||||
|
||||
Fires the ``activity_callback`` (if set on this instance) every 10s
|
||||
while the process is running so the gateway's inactivity timeout
|
||||
doesn't kill long-running commands.
|
||||
|
|
@ -689,12 +704,18 @@ class BaseEnvironment(ABC):
|
|||
an orphan with ``PPID=1`` when python is shut down mid-tool — the
|
||||
``sleep 300``-survives-30-min bug Physikal and I both hit.
|
||||
"""
|
||||
try:
|
||||
from tools.tool_output_limits import get_max_bytes
|
||||
if bounded_capture:
|
||||
try:
|
||||
from tools.tool_output_limits import get_max_bytes
|
||||
|
||||
capture_limit = get_max_bytes()
|
||||
except Exception:
|
||||
capture_limit = 50_000
|
||||
capture_limit = get_max_bytes()
|
||||
except Exception:
|
||||
capture_limit = 50_000
|
||||
else:
|
||||
# Full fidelity: effectively unbounded collector (single head
|
||||
# segment, no eviction) so behavior matches the historical
|
||||
# accumulate-everything semantics.
|
||||
capture_limit = _UNBOUNDED_CAPTURE_CHARS
|
||||
output = _BoundedOutputCollector(capture_limit)
|
||||
|
||||
# Non-blocking drain via select().
|
||||
|
|
@ -1032,8 +1053,19 @@ class BaseEnvironment(ABC):
|
|||
timeout: int | None = None,
|
||||
stdin_data: str | None = None,
|
||||
rewrite_compound_background: bool = True,
|
||||
bounded_capture: bool = False,
|
||||
) -> dict:
|
||||
"""Execute a command, return {"output": str, "returncode": int}."""
|
||||
"""Execute a command, return {"output": str, "returncode": int}.
|
||||
|
||||
``bounded_capture=True`` caps stdout/stderr retention at
|
||||
``tool_output.max_bytes`` WHILE the stream is drained (head/tail
|
||||
window) instead of holding the full output in memory (#64435).
|
||||
It must only be set by callers whose output is destined for the
|
||||
model/tool payload (the foreground terminal tool). Internal
|
||||
full-fidelity consumers — file operations ``cat`` reads that feed
|
||||
the patch engine, code-execution RPC reads, log reads — MUST leave
|
||||
it False: truncating those corrupts data, not just display.
|
||||
"""
|
||||
self._before_execute()
|
||||
|
||||
exec_command, sudo_stdin = self._prepare_command(command)
|
||||
|
|
@ -1068,7 +1100,9 @@ class BaseEnvironment(ABC):
|
|||
proc = self._run_bash(
|
||||
wrapped, login=login, timeout=effective_timeout, stdin_data=effective_stdin
|
||||
)
|
||||
result = self._wait_for_process(proc, timeout=effective_timeout)
|
||||
result = self._wait_for_process(
|
||||
proc, timeout=effective_timeout, bounded_capture=bounded_capture
|
||||
)
|
||||
self._update_cwd(result)
|
||||
|
||||
return result
|
||||
|
|
|
|||
|
|
@ -80,11 +80,17 @@ class BaseModalExecutionEnvironment(BaseEnvironment):
|
|||
timeout: int | None = None,
|
||||
stdin_data: str | None = None,
|
||||
rewrite_compound_background: bool = True,
|
||||
bounded_capture: bool = False,
|
||||
) -> dict:
|
||||
# Managed/remote modal transports execute commands via explicit transport
|
||||
# and do not rely on shell background rewriters. Keep parameter for
|
||||
# compatibility with BaseEnvironment callers.
|
||||
_ = rewrite_compound_background
|
||||
# bounded_capture: accepted for BaseEnvironment.execute() signature
|
||||
# parity (the terminal tool passes it). Modal transports return the
|
||||
# remote function's result in one payload, so streaming-time bounding
|
||||
# does not apply; the terminal tool's final truncation still caps it.
|
||||
_ = bounded_capture
|
||||
self._before_execute()
|
||||
prepared = self._prepare_modal_exec(
|
||||
command,
|
||||
|
|
|
|||
|
|
@ -2657,6 +2657,12 @@ def terminal_tool(
|
|||
execute_kwargs = {
|
||||
"timeout": effective_timeout,
|
||||
"cwd": command_cwd,
|
||||
# Foreground model-facing output: cap retention while
|
||||
# streaming (head/tail window) so a verbose command
|
||||
# can't OOM the gateway before truncation (#64435).
|
||||
# Internal env.execute() consumers (file ops cat
|
||||
# reads, RPC reads) intentionally stay unbounded.
|
||||
"bounded_capture": True,
|
||||
}
|
||||
result = env.execute(command, **execute_kwargs)
|
||||
except Exception as e:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue