From a31fe0fa52cec7728a54d0702f2bda89bb159626 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 27 May 2026 10:05:08 -0700 Subject: [PATCH 1/4] Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Claude Agent SDK message-reader has a 1 MB JSON buffer cap; tool results that exceed it kill the agent with exit 255 and the consensus wrapper wastes its full restart budget retrying a deterministic failure. Fix layers three defenses so the failure cannot reach the restart loop in the first place — and when it does, it stops the wrapper immediately instead of burning retries. 1. CLI-side PostToolUse hook (sandbox/hooks/posttooluse_truncate.py) suppresses tool results above EGG_TOOL_RESULT_CAP_BYTES (default 200 KB) by returning a decision=block with truncation guidance. Runs inside the CLI subprocess so it intercepts before the result crosses the SDK JSON channel. Mutating tools (Edit/Write/MultiEdit/ NotebookEdit) get a reason that warns against retrying the call. 2. ClaudeAgentOptions.max_buffer_size bumped to 4 MB (env-overridable via EGG_AGENT_MAX_BUFFER_SIZE) as belt-and-suspenders. 3. consensus_wrapper greps captured agent output for the SDK overflow marker and exits immediately without consuming the restart budget, surfacing the failure as buffer-overflow rather than a generic AGENT_FAILED. Verified against the issue-2777 16:30:47 UTC crash signature. --- orchestrator/consensus_wrapper.py | 36 ++- orchestrator/tests/test_consensus_wrapper.py | 124 ++++++++++ sandbox/entrypoint.py | 23 +- sandbox/hooks/posttooluse_truncate.py | 157 +++++++++++++ sandbox/tests/test_entrypoint_settings.py | 43 ++++ sandbox/tests/test_posttooluse_truncate.py | 230 +++++++++++++++++++ shared/egg_agent/client.py | 36 +++ tests/shared/egg_agent/test_client.py | 73 ++++++ 8 files changed, 718 insertions(+), 4 deletions(-) create mode 100755 sandbox/hooks/posttooluse_truncate.py create mode 100644 sandbox/tests/test_posttooluse_truncate.py diff --git a/orchestrator/consensus_wrapper.py b/orchestrator/consensus_wrapper.py index 45d4040082..91ecef7dd4 100644 --- a/orchestrator/consensus_wrapper.py +++ b/orchestrator/consensus_wrapper.py @@ -119,6 +119,13 @@ TRANSIENT_BACKOFF_MAX=30 STARTUP_FAILURE_WINDOW_SECONDS={startup_failure_window_seconds} +# Capture agent stdout+stderr so the wrapper can post-mortem the run. +# Used by is_buffer_overflow() to detect the Claude Agent SDK +# message-reader 1MB JSON buffer crash (issue #2804) which is +# deterministic — retrying just hits the same overflow and burns +# the restart budget for no gain. +AGENT_OUTPUT_LOG="${{AGENT_OUTPUT_LOG:-/tmp/agent-output-$$.log}}" + # Log wrapper messages to stderr so they never leak into agent SDK context. cw_log() {{ echo "[consensus-wrapper] $*" >&2 @@ -127,14 +134,30 @@ run_agent() {{ local prompt="$1" local system_prompt="${{2:-}}" + : > "$AGENT_OUTPUT_LOG" # truncate per run so old crashes don't bleed forward if [ -n "$system_prompt" ]; then - {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" + {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" \ + > >(tee -a "$AGENT_OUTPUT_LOG") \ + 2> >(tee -a "$AGENT_OUTPUT_LOG" >&2) else - {agent_command_prefix} "$prompt" + {agent_command_prefix} "$prompt" \ + > >(tee -a "$AGENT_OUTPUT_LOG") \ + 2> >(tee -a "$AGENT_OUTPUT_LOG" >&2) fi return $? }} +# Detect the Claude Agent SDK 1MB JSON message-reader overflow +# signature in the most recent agent run. Issue #2804. The overflow +# is deterministic: re-running the agent against the same codebase +# almost always hits the same oversized tool result, so the wrapper +# should NOT consume retry budget on this failure class. Returns 0 +# (true) if the overflow marker was logged, 1 otherwise. +is_buffer_overflow() {{ + [ -f "$AGENT_OUTPUT_LOG" ] || return 1 + grep -q "exceeded maximum buffer size" "$AGENT_OUTPUT_LOG" 2>/dev/null +}} + # Helper: extract BRC agent state from pipeline status JSON get_brc_state() {{ local response="$1" @@ -286,7 +309,10 @@ fi fi - if is_transient_crash "$AGENT_EXIT"; then + if is_buffer_overflow; then + cw_log "Agent crashed on Claude Agent SDK buffer overflow (issue #2804). Deterministic failure; retry budget would be wasted. NOT restarting." + exit $AGENT_EXIT + elif is_transient_crash "$AGENT_EXIT"; then cw_log "Transient crash (code $AGENT_EXIT). Will restart with backoff." CRASH_BACKOFF=$TRANSIENT_BACKOFF_INITIAL elif is_startup_failure "$AGENT_EXIT" "$AGENT_DURATION"; then @@ -558,6 +584,10 @@ exit 0 fi fi + if is_buffer_overflow; then + cw_log "Agent crashed on Claude Agent SDK buffer overflow (issue #2804) on restart $RESTART_COUNT. Deterministic failure; further retries would waste budget. Stopping." + exit $AGENT_EXIT + fi if is_transient_crash "$AGENT_EXIT"; then cw_log "Transient crash on restart $RESTART_COUNT (code $AGENT_EXIT). Will retry." if [ "$CRASH_BACKOFF" -eq 0 ]; then diff --git a/orchestrator/tests/test_consensus_wrapper.py b/orchestrator/tests/test_consensus_wrapper.py index 613afe854e..53191a333e 100644 --- a/orchestrator/tests/test_consensus_wrapper.py +++ b/orchestrator/tests/test_consensus_wrapper.py @@ -1340,6 +1340,130 @@ def test_non_transient_exit_code_42_does_not_restart(self): assert "Transient crash" not in result.stderr +class TestBufferOverflowDetection: + """Issue #2804: the Claude Agent SDK 1MB JSON buffer crash is + deterministic — retrying just hits the same overflow on the same + codebase. The wrapper must short-circuit retry budget when it + sees the overflow signature in the agent's output. + """ + + def test_script_defines_is_buffer_overflow(self): + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + assert "is_buffer_overflow()" in script + assert "exceeded maximum buffer size" in script + + def test_script_captures_agent_output(self): + """Agent output must be tee'd to a log file the wrapper can grep.""" + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + assert "AGENT_OUTPUT_LOG" in script + assert "tee -a" in script + + def test_buffer_overflow_check_runs_before_transient_check(self): + """The overflow check must precede is_transient_crash so a + signal-255 agent that crashed on overflow doesn't get retried. + """ + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + # Both functions exist + assert "is_buffer_overflow" in script + assert "is_transient_crash" in script + # Find the first occurrence of each in the initial-exit handler + # (the section after the consensus/confirmed checks). + idx_buffer = script.find("if is_buffer_overflow") + idx_transient = script.find('is_transient_crash "$AGENT_EXIT"') + assert idx_buffer > 0 and idx_transient > 0 + assert idx_buffer < idx_transient, ( + "is_buffer_overflow must be checked BEFORE is_transient_crash" + ) + + def test_buffer_overflow_log_message_cites_issue(self): + """When the wrapper aborts on overflow, log must mention #2804.""" + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + assert "#2804" in script + + def test_buffer_overflow_check_in_restart_loop(self): + """The restart-loop must ALSO check for buffer overflow — even if + the initial run survived, a recovery attempt that hits the + overflow must abort without consuming the rest of the budget. + """ + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + # is_buffer_overflow appears at least twice: initial handler + restart loop + assert script.count("is_buffer_overflow") >= 2 + + @staticmethod + def _make_buffer_overflow_agent(tmpdir: str) -> None: + """Create a mock python3 that simulates the SDK buffer-overflow crash. + + Writes the signature marker to stderr (matching the real SDK's + ``logger.error`` from ``query.py:221``) and exits 255 — the same + shape produced by the real crash in the issue-2777 #2804 incident. + """ + mock_python = os.path.join(tmpdir, "python3") + real_python = sys.executable + with open(mock_python, "w") as f: + f.write("#!/bin/bash\n") + f.write('if [ "$1" = "-m" ] && [ "$2" = "egg_agent" ]; then\n') + f.write( + ' echo "Fatal error in message reader: Failed to decode ' + "JSON: JSON message exceeded maximum buffer size of " + '1048576 bytes..." >&2\n' + ) + f.write(" exit 255\n") + f.write("else\n") + f.write(f' exec {shlex.quote(real_python)} "$@"\n') + f.write("fi\n") + os.chmod(mock_python, 0o755) # nosec B103 + + def test_buffer_overflow_aborts_without_retry(self): + """Agent crashing with the SDK buffer-overflow signature must + exit immediately, NOT consume the restart budget. + """ + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "egg-orch.log") + # No consensus reached + TestConsensusWrapperBehavior._make_mock_orch_no_consensus(tmpdir, log_file) + self._make_buffer_overflow_agent(tmpdir) + + cmd = build_consensus_wrapped_command("Prompt", max_restarts=2) + result = TestConsensusWrapperBehavior._run_wrapper_command(cmd, tmpdir) + + # Wrapper must propagate the agent's exit code (255) + assert result.returncode == 255, result.stderr + # Must log the overflow diagnosis + assert "buffer overflow" in result.stderr.lower() + assert "#2804" in result.stderr + # Must NOT have attempted a restart + assert "Restarting" not in result.stderr + assert "Transient crash" not in result.stderr + + def test_signal_255_without_overflow_marker_still_retries(self): + """Other 255 crashes (genuine SIGSEGV, bun segfault) keep the + existing transient-retry behavior — the wrapper must only + short-circuit when the overflow marker is present. + """ + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "egg-orch.log") + TestConsensusWrapperBehavior._make_mock_orch_no_consensus(tmpdir, log_file) + # Plain exit 255 with no marker → should still be classified + # as transient and trigger a restart. + _make_mock_agent(tmpdir, exit_code=255) + + cmd = build_consensus_wrapped_command( + "Prompt", + max_restarts=1, + transient_backoff_initial=1, + ) + result = TestConsensusWrapperBehavior._run_wrapper_command(cmd, tmpdir, timeout=20) + + # Without the marker, the existing transient-crash classification + # still kicks in; should log Transient crash. + assert "Transient crash" in result.stderr + + class TestEventDrivenWait: """Issue #1897 Phase 5 / TASK-5-1 (plan rev 4, reviewer_plan blocker 4): ``check_confirmed_and_wait`` is an SSE-primary hybrid. diff --git a/sandbox/entrypoint.py b/sandbox/entrypoint.py index 63633e2942..61453f24d6 100644 --- a/sandbox/entrypoint.py +++ b/sandbox/entrypoint.py @@ -1054,7 +1054,7 @@ def setup_claude(config: Config, logger: Logger) -> None: logger.success(f"Skills installed: {', '.join(installed)}") # Create settings.json - settings = { + settings: dict[str, Any] = { "defaultPermissionMode": "bypassPermissions", "skipDangerousModePermissionPrompt": True, "autoApproveEdits": True, @@ -1064,6 +1064,27 @@ def setup_claude(config: Config, logger: Logger) -> None: "defaultModel": "opus[1m]", "showResumeCommand": False, "memory": {"enabled": False}, + # PostToolUse truncation hook — bounds tool result size to prevent + # the Claude Agent SDK message-reader buffer overflow that kills + # agent processes with exit 255 on large tool results + # (issue #2804). The hook runs inside the CLI subprocess, before + # the result crosses the SDK JSON channel, so it can suppress + # oversized payloads that would crash the reader. Matcher is "*" + # (every tool) — the hook itself decides per-tool whether to + # block or pass through based on serialized size. + "hooks": { + "PostToolUse": [ + { + "matcher": "*", + "hooks": [ + { + "type": "command", + "command": ("/opt/egg-runtime/sandbox/hooks/posttooluse_truncate.py"), + } + ], + } + ] + }, } # In private mode, disallow web tools at the agent config level so they are diff --git a/sandbox/hooks/posttooluse_truncate.py b/sandbox/hooks/posttooluse_truncate.py new file mode 100755 index 0000000000..47e327f5a7 --- /dev/null +++ b/sandbox/hooks/posttooluse_truncate.py @@ -0,0 +1,157 @@ +#!/usr/bin/env python3 +"""PostToolUse hook: bound tool result size to prevent SDK buffer overflows. + +Issue #2804. The Claude Agent SDK message reader has a hard-coded JSON +buffer cap (1 MB default; we raise it via ``max_buffer_size`` but it is +still finite). Tool results that exceed the buffer kill the agent +process with exit code 255, which the consensus-wrapper then wastes its +restart budget retrying — a deterministic failure that no retry can +recover. + +This hook runs inside the Claude Code CLI subprocess after each tool +call. It checks the serialized size of the tool response and, when it +exceeds ``EGG_TOOL_RESULT_CAP_BYTES`` (default 200 KB), returns a +``decision: block`` response that suppresses the oversized payload and +feeds the agent a structured retry prompt instead. The agent sees +truncation guidance and re-issues the tool call with a narrower scope. + +Why CLI-side rather than programmatic SDK hooks: SDK hook callbacks +travel back to the Python process through the same buffered JSON stream +that has the cap. An oversized ``tool_response`` would crash the buffer +*before* the callback could see it. A CLI hook subprocess runs entirely +inside the CLI's process boundary and gets the tool response without +touching the SDK channel. + +Edit / Write / MultiEdit are handled with a specific reason — the file +mutation already happened, so the agent must NOT retry the edit; it +just needs to know the response snippet was suppressed. +""" + +from __future__ import annotations + +import json +import os +import sys +from typing import Any + +DEFAULT_CAP_BYTES = 200_000 + + +def _cap_bytes() -> int: + raw = os.environ.get("EGG_TOOL_RESULT_CAP_BYTES", "").strip() + if not raw: + return DEFAULT_CAP_BYTES + try: + value = int(raw) + except ValueError: + return DEFAULT_CAP_BYTES + return value if value > 0 else DEFAULT_CAP_BYTES + + +def _serialized_size(value: Any) -> int: + """Return the JSON-serialized byte length of ``value``. + + Matches what the SDK reader sees on the wire. Falls back to ``str()`` + for objects that aren't JSON-serializable. + """ + if value is None: + return 0 + if isinstance(value, (bytes, bytearray)): + return len(value) + if isinstance(value, str): + return len(value.encode("utf-8")) + try: + return len(json.dumps(value, default=str).encode("utf-8")) + except TypeError, ValueError: + return len(str(value).encode("utf-8")) + + +# Tools where the model should NOT retry the call — the side effect +# already happened on disk. The reason text tells the agent the +# mutation succeeded and the snippet was suppressed. +_MUTATING_TOOLS = frozenset({"Edit", "Write", "MultiEdit", "NotebookEdit"}) + +# Tool-specific guidance for tools that are safe to retry with a +# narrower scope. +_RETRY_GUIDANCE = { + "Read": ( + "Re-issue Read with the `offset` and `limit` parameters scoped to the " + "specific lines you need. Default limit is 2000 lines." + ), + "Bash": ( + "Re-issue the command with narrower scope: pipe through `head`, " + "`tail`, `wc -l`, or filter with `grep`. For long-running output, " + "redirect to a file and Read targeted chunks." + ), + "Grep": ( + "Re-issue Grep with a more specific pattern, a smaller `path`, " + "`--head-limit`, or output_mode='count' first to gauge result size." + ), + "Glob": ("Re-issue Glob with a tighter pattern or a smaller `path` scope."), +} + +_FALLBACK_RETRY_GUIDANCE = ( + "Re-issue the call with narrower input (smaller path, lower limit, more specific pattern)." +) + + +def _build_reason(tool_name: str, size: int, cap: int) -> str: + base = ( + f"Tool result for `{tool_name}` was {size} bytes, exceeding the " + f"{cap}-byte cap (issue #2804). The payload was suppressed to " + "prevent the Claude Agent SDK buffer overflow that would otherwise " + "kill the agent process." + ) + if tool_name in _MUTATING_TOOLS: + return ( + f"{base} The file mutation itself succeeded — do NOT retry the " + "tool call. If you need to verify the result, use Read with " + "narrow `offset`/`limit` parameters." + ) + guidance = _RETRY_GUIDANCE.get(tool_name, _FALLBACK_RETRY_GUIDANCE) + return f"{base} {guidance}" + + +def evaluate(event: dict[str, Any], cap: int) -> dict[str, Any] | None: + """Inspect a PostToolUse event and return the hook output, or None to allow. + + Pure function exposed for testing — the CLI entrypoint wires it to + stdin/stdout. + """ + tool_response = event.get("tool_response") + if tool_response is None: + return None + size = _serialized_size(tool_response) + if size <= cap: + return None + tool_name = str(event.get("tool_name") or "unknown") + return { + "decision": "block", + "reason": _build_reason(tool_name, size, cap), + } + + +def main() -> int: + try: + raw = sys.stdin.read() + except OSError: + return 0 + if not raw.strip(): + return 0 + try: + event = json.loads(raw) + except json.JSONDecodeError: + # Unparseable input — allow the tool result through; the buffer + # bump and clean-error path in shared/egg_agent/client.py will + # catch any oversize payload as a structured failure. + return 0 + if not isinstance(event, dict): + return 0 + output = evaluate(event, _cap_bytes()) + if output is not None: + json.dump(output, sys.stdout) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/sandbox/tests/test_entrypoint_settings.py b/sandbox/tests/test_entrypoint_settings.py index ebc4d0655d..d21d93fe1a 100644 --- a/sandbox/tests/test_entrypoint_settings.py +++ b/sandbox/tests/test_entrypoint_settings.py @@ -84,3 +84,46 @@ def test_unset_env_no_disallowed_tools(self, _which, mock_config, mock_logger): setup_claude(mock_config, mock_logger) settings = _read_settings(mock_config) assert "disallowedTools" not in settings + + +class TestPostToolUseHook: + """Issue #2804: settings.json must wire the PostToolUse truncation + hook so the CLI runs it for every tool call, bounding tool_response + size before it crosses the SDK JSON channel. + """ + + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_hook_registered_for_post_tool_use(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert "hooks" in settings, "settings.json must include hooks for #2804" + assert "PostToolUse" in settings["hooks"] + + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_hook_matcher_covers_all_tools(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + matchers = settings["hooks"]["PostToolUse"] + assert len(matchers) == 1 + # ``*`` matches every tool so we catch Read/Bash/Grep/Edit/MCP alike + assert matchers[0]["matcher"] == "*" + + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_hook_points_at_runtime_path(self, _which, mock_config, mock_logger): + """The command must reference the in-image runtime path. + + The Dockerfile bakes ``sandbox/`` to ``/opt/egg-runtime/sandbox/`` + (see ``sandbox/Dockerfile``); the hook script lives at + ``hooks/posttooluse_truncate.py`` under that root. + """ + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + commands = [ + cmd["command"] + for matcher in settings["hooks"]["PostToolUse"] + for cmd in matcher["hooks"] + if cmd.get("type") == "command" + ] + assert any( + "/opt/egg-runtime/sandbox/hooks/posttooluse_truncate.py" in cmd for cmd in commands + ), f"hook command must reference runtime path, got {commands!r}" diff --git a/sandbox/tests/test_posttooluse_truncate.py b/sandbox/tests/test_posttooluse_truncate.py new file mode 100644 index 0000000000..43ef3d91c1 --- /dev/null +++ b/sandbox/tests/test_posttooluse_truncate.py @@ -0,0 +1,230 @@ +"""Tests for the PostToolUse truncation hook (issue #2804).""" + +from __future__ import annotations + +import importlib.util +import io +import json +import sys +from pathlib import Path +from typing import Any +from unittest.mock import patch + +import pytest + +# Import the script as a module (it has a .py suffix but lives in +# sandbox/hooks/ which is not a Python package). +_HOOK_PATH = Path(__file__).parent.parent / "hooks" / "posttooluse_truncate.py" +_spec = importlib.util.spec_from_file_location("posttooluse_truncate", _HOOK_PATH) +assert _spec is not None and _spec.loader is not None +posttooluse_truncate = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(posttooluse_truncate) + + +class TestSerializedSize: + """``_serialized_size`` must match the byte length the SDK reader sees.""" + + def test_none_is_zero(self): + assert posttooluse_truncate._serialized_size(None) == 0 + + def test_str_is_utf8_length(self): + assert posttooluse_truncate._serialized_size("hello") == 5 + # Multi-byte characters count their UTF-8 length, not codepoints + assert posttooluse_truncate._serialized_size("é") == 2 + + def test_dict_is_json_encoded(self): + payload = {"key": "value"} + encoded = json.dumps(payload).encode("utf-8") + assert posttooluse_truncate._serialized_size(payload) == len(encoded) + + def test_list_of_blocks(self): + # Matches the Claude Code tool_result shape: list of content blocks + payload = [{"type": "text", "text": "x" * 1000}] + size = posttooluse_truncate._serialized_size(payload) + # Size should be at least the inner text length + assert size >= 1000 + + def test_falls_back_to_str_for_unserializable(self): + class Weird: + def __repr__(self) -> str: + return "WEIRD-OBJECT" + + # default=str in json.dumps catches most; we just need this not to raise + result = posttooluse_truncate._serialized_size(Weird()) + assert result > 0 + + +class TestCapBytes: + """Env override must apply and reject garbage values cleanly.""" + + def test_default_cap(self): + with patch.dict("os.environ", {}, clear=False): + # Ensure unset + import os + + os.environ.pop("EGG_TOOL_RESULT_CAP_BYTES", None) + assert posttooluse_truncate._cap_bytes() == 200_000 + + def test_env_override(self): + with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "50000"}): + assert posttooluse_truncate._cap_bytes() == 50_000 + + def test_env_zero_falls_back_to_default(self): + """Zero or negative caps are nonsensical — fall back to default.""" + with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "0"}): + assert posttooluse_truncate._cap_bytes() == 200_000 + + def test_env_negative_falls_back_to_default(self): + with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "-1"}): + assert posttooluse_truncate._cap_bytes() == 200_000 + + def test_env_garbage_falls_back_to_default(self): + with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "not-a-number"}): + assert posttooluse_truncate._cap_bytes() == 200_000 + + +class TestEvaluate: + """``evaluate`` is the pure decision function — under cap = None + (allow), over cap = block with reason. + """ + + def test_under_cap_returns_none(self): + event = {"tool_name": "Read", "tool_response": "small payload"} + assert posttooluse_truncate.evaluate(event, cap=200_000) is None + + def test_at_exact_cap_returns_none(self): + # Equal to cap is still allowed; only strictly greater blocks. + payload = "x" * 100 + event = {"tool_name": "Read", "tool_response": payload} + assert posttooluse_truncate.evaluate(event, cap=100) is None + + def test_over_cap_returns_block(self): + payload = "x" * 10_000 + event = {"tool_name": "Read", "tool_response": payload} + result = posttooluse_truncate.evaluate(event, cap=5_000) + assert result is not None + assert result["decision"] == "block" + assert "Read" in result["reason"] + + def test_missing_tool_response_allows(self): + event = {"tool_name": "Read"} + assert posttooluse_truncate.evaluate(event, cap=100) is None + + def test_reason_cites_issue(self): + event = {"tool_name": "Bash", "tool_response": "x" * 1000} + result = posttooluse_truncate.evaluate(event, cap=500) + assert result is not None + assert "#2804" in result["reason"] + + def test_reason_reports_size_and_cap(self): + event = {"tool_name": "Read", "tool_response": "x" * 1000} + result = posttooluse_truncate.evaluate(event, cap=500) + assert result is not None + # Both numbers should appear so the agent (and the operator + # debugging logs) can see how badly the call overshot. + assert "1000" in result["reason"] or "1,000" in result["reason"] + assert "500" in result["reason"] + + +class TestMutatingTools: + """Edit/Write/MultiEdit/NotebookEdit already mutated disk; the + agent must be told NOT to retry the call. + """ + + @pytest.mark.parametrize("tool", ["Edit", "Write", "MultiEdit", "NotebookEdit"]) + def test_mutating_tool_reason_warns_against_retry(self, tool: str) -> None: + event = {"tool_name": tool, "tool_response": "x" * 10_000} + result = posttooluse_truncate.evaluate(event, cap=1_000) + assert result is not None + # The reason must explicitly tell the model NOT to retry — re-running + # an Edit that already landed on disk would either fail (string-not-found) + # or worse, double-apply. + assert "do NOT retry" in result["reason"] or "do not retry" in result["reason"].lower() + + @pytest.mark.parametrize("tool", ["Read", "Bash", "Grep", "Glob"]) + def test_idempotent_tool_reason_offers_retry_guidance(self, tool: str) -> None: + event = {"tool_name": tool, "tool_response": "x" * 10_000} + result = posttooluse_truncate.evaluate(event, cap=1_000) + assert result is not None + # For idempotent tools the agent SHOULD retry, just with narrower scope. + # Reason must contain actionable guidance (offset/limit, head, narrow pattern, etc.) + reason_lower = result["reason"].lower() + assert any( + hint in reason_lower + for hint in ("offset", "limit", "head", "narrow", "smaller", "scope") + ), f"Reason for {tool} should contain retry guidance: {result['reason']!r}" + + def test_unknown_tool_gets_fallback_guidance(self): + event = {"tool_name": "FutureTool", "tool_response": "x" * 10_000} + result = posttooluse_truncate.evaluate(event, cap=1_000) + assert result is not None + assert "FutureTool" in result["reason"] + assert "narrower" in result["reason"].lower() + + +class TestMain: + """End-to-end stdin/stdout protocol that the CLI hook actually + invokes. + """ + + @staticmethod + def _run_main(payload: Any) -> tuple[int, str]: + stdin = io.StringIO(json.dumps(payload) if payload is not None else "") + stdout = io.StringIO() + with patch.object(sys, "stdin", stdin), patch.object(sys, "stdout", stdout): + rc = posttooluse_truncate.main() + return rc, stdout.getvalue() + + def test_under_cap_writes_nothing(self): + rc, out = self._run_main( + {"tool_name": "Read", "tool_response": "small"}, + ) + assert rc == 0 + assert out == "" + + def test_over_cap_writes_block_json(self): + # Cap is 200KB by default; payload > 200KB to force block + rc, out = self._run_main( + {"tool_name": "Read", "tool_response": "x" * 300_000}, + ) + assert rc == 0 + parsed = json.loads(out) + assert parsed["decision"] == "block" + assert "Read" in parsed["reason"] + + def test_empty_stdin_allows(self): + rc, out = self._run_main(None) + assert rc == 0 + assert out == "" + + def test_malformed_json_allows(self): + """Unparseable input should fall through, not crash. + + The buffer-bump + clean-error path in client.py will catch any + oversize payload that slips past as a structured failure; the + hook should never amplify a parse error into an agent crash. + """ + stdin = io.StringIO("not json at all") + stdout = io.StringIO() + with patch.object(sys, "stdin", stdin), patch.object(sys, "stdout", stdout): + rc = posttooluse_truncate.main() + assert rc == 0 + assert stdout.getvalue() == "" + + def test_non_dict_input_allows(self): + rc, out = self._run_main(["not", "a", "dict"]) + assert rc == 0 + assert out == "" + + def test_respects_env_cap(self): + """Lower the cap via env so a small payload triggers block.""" + import os + + with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "50"}): + os.environ["EGG_TOOL_RESULT_CAP_BYTES"] = "50" + rc, out = self._run_main( + {"tool_name": "Bash", "tool_response": "x" * 200}, + ) + assert rc == 0 + parsed = json.loads(out) + assert parsed["decision"] == "block" diff --git a/shared/egg_agent/client.py b/shared/egg_agent/client.py index 272a132605..650c26ed8a 100644 --- a/shared/egg_agent/client.py +++ b/shared/egg_agent/client.py @@ -61,6 +61,35 @@ def error(self, msg: str, **kwargs: Any) -> None: # Default model for sandbox agents DEFAULT_MODEL = "opus[1m]" +# Belt-and-suspenders bump on the Claude Agent SDK message-reader buffer. +# The SDK default is 1 MB; tool results that exceed it kill the agent +# process with exit 255 (issue #2804). The primary defense is the +# CLI-side PostToolUse hook (see sandbox/hooks/posttooluse_truncate.py) +# which suppresses oversized payloads before they cross the SDK channel. +# This bump gives breathing room for tool results that slip past the +# hook (e.g. results just under the per-tool cap that aggregate with +# subsequent CLI frames). Env override allows ops to raise it further +# without a code change. See ``ClaudeAgentOptions.max_buffer_size``. +_DEFAULT_MAX_BUFFER_SIZE = 4 * 1024 * 1024 # 4 MB + +# Substring of the SDK's CLIJSONDecodeError message that identifies the +# buffer-overflow failure mode specifically. Surfaced into the +# ``AgentResult.error`` so the consensus-wrapper can classify it as a +# terminal (deterministic) failure rather than a transient crash worth +# retrying. Match-string kept stable; the wrapper greps for it. +_BUFFER_OVERFLOW_MARKER = "exceeded maximum buffer size" + + +def _max_buffer_size() -> int: + raw = os.environ.get("EGG_AGENT_MAX_BUFFER_SIZE", "").strip() + if not raw: + return _DEFAULT_MAX_BUFFER_SIZE + try: + value = int(raw) + except ValueError: + return _DEFAULT_MAX_BUFFER_SIZE + return value if value > 0 else _DEFAULT_MAX_BUFFER_SIZE + async def run_agent_async( prompt: str, @@ -118,6 +147,12 @@ async def run_agent_async( UserMessage, query, ) + # CLIJSONDecodeError is a subclass of ClaudeSDKError, so it's + # caught by the existing handler below — issue #2804 relies on + # its error message preserving the ``exceeded maximum buffer + # size`` marker so the consensus-wrapper can short-circuit + # retry on this failure class. We don't import it explicitly + # here (the marker stability is verified by tests). except ImportError: return AgentResult( success=False, @@ -176,6 +211,7 @@ async def _check_tool_permission( setting_sources=["project", "user"], disallowed_tools=disallowed, can_use_tool=tool_permission_callback, + max_buffer_size=_max_buffer_size(), ) if max_turns is not None: options.max_turns = max_turns diff --git a/tests/shared/egg_agent/test_client.py b/tests/shared/egg_agent/test_client.py index f566f0a313..e8d87ebf1e 100644 --- a/tests/shared/egg_agent/test_client.py +++ b/tests/shared/egg_agent/test_client.py @@ -79,6 +79,11 @@ class ProcessError(ClaudeSDKError): # type: ignore[no-redef] class CLINotFoundError(ClaudeSDKError): # type: ignore[no-redef] pass + class CLIJSONDecodeError(ClaudeSDKError): # type: ignore[no-redef] + """Mirrors claude_agent_sdk._errors.CLIJSONDecodeError (issue #2804).""" + + pass + @dataclass class SystemMessage: # type: ignore[no-redef] subtype: str = "" @@ -95,6 +100,8 @@ class ClaudeAgentOptions: # type: ignore[no-redef] setting_sources: list[str] | None = None disallowed_tools: list[str] = field(default_factory=list) can_use_tool: Any = None + # issue #2804: bump the SDK's JSON message buffer + max_buffer_size: int | None = None @dataclass class PermissionResultAllow: # type: ignore[no-redef] @@ -124,6 +131,7 @@ class ToolPermissionContext: # type: ignore[no-redef] _mock_sdk.ProcessError = ProcessError # type: ignore[attr-defined] _mock_sdk.CLINotFoundError = CLINotFoundError # type: ignore[attr-defined] _mock_sdk.ClaudeSDKError = ClaudeSDKError # type: ignore[attr-defined] + _mock_sdk.CLIJSONDecodeError = CLIJSONDecodeError # type: ignore[attr-defined] _mock_sdk.SystemMessage = SystemMessage # type: ignore[attr-defined] _mock_sdk.ClaudeAgentOptions = ClaudeAgentOptions # type: ignore[attr-defined] _mock_sdk.PermissionResultAllow = PermissionResultAllow # type: ignore[attr-defined] @@ -815,6 +823,71 @@ def test_private_mode_empty_string_no_disallowed_tools(self, mock_query): assert opts.disallowed_tools == [] +class TestMaxBufferSize: + """Issue #2804: SDK message-reader buffer bump. + + The SDK default is 1 MB; we bump it to 4 MB (env-overridable) so + moderate-but-large tool results that slip past the PostToolUse hook + don't crash the reader. + """ + + @patch.dict(os.environ, {}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_default_max_buffer_size_is_4mb(self, mock_query): + env = os.environ.copy() + env.pop("EGG_AGENT_MAX_BUFFER_SIZE", None) + with patch.dict(os.environ, env, clear=True): + _run_async(run_agent_async("test prompt")) + opts = mock_query.call_args.kwargs["options"] + assert opts.max_buffer_size == 4 * 1024 * 1024 + + @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "16777216"}) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_env_override_raises_buffer(self, mock_query): + _run_async(run_agent_async("test prompt")) + opts = mock_query.call_args.kwargs["options"] + assert opts.max_buffer_size == 16_777_216 + + @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "not-a-number"}) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_garbage_env_falls_back_to_default(self, mock_query): + _run_async(run_agent_async("test prompt")) + opts = mock_query.call_args.kwargs["options"] + assert opts.max_buffer_size == 4 * 1024 * 1024 + + @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "0"}) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_zero_env_falls_back_to_default(self, mock_query): + """Zero is nonsensical — fall back rather than disabling the buffer.""" + _run_async(run_agent_async("test prompt")) + opts = mock_query.call_args.kwargs["options"] + assert opts.max_buffer_size == 4 * 1024 * 1024 + + +class TestBufferOverflowErrorHandling: + """Issue #2804: when the SDK raises CLIJSONDecodeError on a buffer + overflow, the agent must return a structured failure with the + overflow marker preserved in ``error`` — the consensus-wrapper + greps for that string to short-circuit retry. + """ + + @patch("claude_agent_sdk.query") + def test_buffer_overflow_returns_failure_with_marker(self, mock_query): + from claude_agent_sdk import CLIJSONDecodeError + + mock_query.side_effect = CLIJSONDecodeError( + "JSON message exceeded maximum buffer size of 4194304 bytes..." + ) + + result = _run_async(run_agent_async("test prompt")) + + assert result.success is False + assert result.returncode == -1 + # Marker must appear verbatim in ``error`` so the wrapper's grep + # in is_buffer_overflow() matches. + assert "exceeded maximum buffer size" in result.error + + class TestToolInterception: """Tests for can_use_tool-based tool interception.""" From 3068b6120d5daca1d772c28bec9f9e653d20e9c4 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Wed, 27 May 2026 11:26:47 -0700 Subject: [PATCH 2/4] Fix #2804: drop broken PostToolUse hook, keep fail-fast on overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address reviewer feedback on PR #2810. The PostToolUse hook layer doesn't work: the Claude Agent SDK contract (PostToolUseHookSpecificOutput exposes only additionalContext and updatedMCPToolOutput; "Can block? No" in docs) means decision:block cannot suppress the tool_response payload on the wire. The hook fires, the oversized payload still crosses the JSON channel, the buffer crashes anyway — pure telemetry on a channel the crash kills. Also drop the 4 MB max_buffer_size bump. Allowing larger contexts is the wrong escape hatch; the real fix is tool-layer truncation (#2805) so massive payloads never get produced in the first place. What this PR now does (scoped to the wrapper): - Detect the SDK buffer-overflow signature in the agent's output and short-circuit the restart budget — the overflow is deterministic, so retries just re-hit the same crash and burn the budget for no gain. - Restructure run_agent to pipe through a single tee instead of two process-substitution tees; bash waits on pipelines synchronously, so the post-mortem grep can no longer race a still-flushing tee. - Add a behavioral test for the restart-loop overflow path (initial clean exit triggers restart, recovery run crashes with the marker, wrapper aborts without further retries). What goes elsewhere: - Tool-layer truncation in egg MCP tools — #2805. - Buffer-overflow crashes on built-in tools (Edit on large files, etc.) — remain a known terminal failure until tool-layer caps land. The wrapper now fails fast instead of burning the restart budget. --- orchestrator/consensus_wrapper.py | 36 ++- orchestrator/tests/test_consensus_wrapper.py | 88 +++++++ sandbox/entrypoint.py | 21 -- sandbox/hooks/posttooluse_truncate.py | 157 ------------- sandbox/tests/test_entrypoint_settings.py | 129 ----------- sandbox/tests/test_posttooluse_truncate.py | 230 ------------------- shared/egg_agent/client.py | 36 +-- tests/shared/egg_agent/test_client.py | 45 +--- 8 files changed, 121 insertions(+), 621 deletions(-) delete mode 100755 sandbox/hooks/posttooluse_truncate.py delete mode 100644 sandbox/tests/test_entrypoint_settings.py delete mode 100644 sandbox/tests/test_posttooluse_truncate.py diff --git a/orchestrator/consensus_wrapper.py b/orchestrator/consensus_wrapper.py index 91ecef7dd4..c0a9dc5001 100644 --- a/orchestrator/consensus_wrapper.py +++ b/orchestrator/consensus_wrapper.py @@ -135,24 +135,36 @@ local prompt="$1" local system_prompt="${{2:-}}" : > "$AGENT_OUTPUT_LOG" # truncate per run so old crashes don't bleed forward + # Pipe stdout+stderr through tee so the post-mortem grep + # (is_buffer_overflow) sees the agent's full output. We use a + # single pipeline rather than per-stream process substitution + # because bash waits on pipelines synchronously; process + # substitution (> >(tee ...)) backgrounds the tee subshell and + # doesn't wait, which races with the immediate is_buffer_overflow + # grep that follows on agent exit. if [ -n "$system_prompt" ]; then - {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" \ - > >(tee -a "$AGENT_OUTPUT_LOG") \ - 2> >(tee -a "$AGENT_OUTPUT_LOG" >&2) + {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" 2>&1 | tee -a "$AGENT_OUTPUT_LOG" else - {agent_command_prefix} "$prompt" \ - > >(tee -a "$AGENT_OUTPUT_LOG") \ - 2> >(tee -a "$AGENT_OUTPUT_LOG" >&2) + {agent_command_prefix} "$prompt" 2>&1 | tee -a "$AGENT_OUTPUT_LOG" fi - return $? + return ${{PIPESTATUS[0]}} }} -# Detect the Claude Agent SDK 1MB JSON message-reader overflow -# signature in the most recent agent run. Issue #2804. The overflow +# Detect the Claude Agent SDK 1 MB JSON message-reader overflow +# signature in the most recent agent run. Issue #2804. The overflow # is deterministic: re-running the agent against the same codebase -# almost always hits the same oversized tool result, so the wrapper -# should NOT consume retry budget on this failure class. Returns 0 -# (true) if the overflow marker was logged, 1 otherwise. +# hits the same oversized tool result, so the wrapper must NOT +# consume retry budget on this failure class. Returns 0 (true) if +# the marker was logged, 1 otherwise. +# +# The substring matches CLI output from claude_agent_sdk emitted on +# the buffer overflow path. If a future SDK bump changes the +# wording, this grep silently falls through and the wrapper burns +# its retry budget again — the marker_stability_test in +# orchestrator/tests/test_consensus_wrapper.py exercises a synthetic +# log to keep this honest, but does not pin against the installed +# SDK. The real fix is tool-layer truncation (#2805); this is the +# fail-fast path until that lands. is_buffer_overflow() {{ [ -f "$AGENT_OUTPUT_LOG" ] || return 1 grep -q "exceeded maximum buffer size" "$AGENT_OUTPUT_LOG" 2>/dev/null diff --git a/orchestrator/tests/test_consensus_wrapper.py b/orchestrator/tests/test_consensus_wrapper.py index 53191a333e..63bf1a5c80 100644 --- a/orchestrator/tests/test_consensus_wrapper.py +++ b/orchestrator/tests/test_consensus_wrapper.py @@ -1463,6 +1463,94 @@ def test_signal_255_without_overflow_marker_still_retries(self): # still kicks in; should log Transient crash. assert "Transient crash" in result.stderr + def test_buffer_overflow_in_restart_loop_aborts_without_further_retries(self): + """Restart-loop overflow path: clean initial exit triggers a + restart, recovery run crashes with the overflow marker, wrapper + aborts immediately instead of consuming the remaining budget. + + Distinct from ``test_buffer_overflow_aborts_without_retry``, + which only exercises the initial-exit handler. Both code paths + need the buffer-overflow short-circuit; this regression-guards + the restart-loop branch (#2804 review feedback). + """ + with tempfile.TemporaryDirectory() as tmpdir: + log_file = os.path.join(tmpdir, "egg-orch.log") + claude_log = os.path.join(tmpdir, "claude.log") + call_counter = os.path.join(tmpdir, "agent_call_count") + + # Mock egg-orch: always returns is_complete=false so the + # wrapper enters the restart loop after each clean exit. + mock_orch = os.path.join(tmpdir, "egg-orch") + with open(mock_orch, "w") as f: + f.write("#!/bin/bash\n") + f.write(f'echo "$@" >> {shlex.quote(log_file)}\n') + f.write('if echo "$@" | grep -q "pipeline status"; then\n') + f.write( + ' echo \'{"data": {"concurrent": {"consensus": ' + '{"is_complete": false, "agents": {}}}}}\'\n' + ) + f.write('elif echo "$@" | grep -q "message poll"; then\n') + f.write(' echo "[]"\n') + f.write("else\n") + f.write(' echo "{}"\n') + f.write("fi\n") + os.chmod(mock_orch, 0o755) # nosec B103 + + # Mock agent: clean exit on call 1 (triggers restart), then + # emits the SDK overflow signature and exits 255 on call 2. + mock_python = os.path.join(tmpdir, "python3") + real_python = sys.executable + with open(mock_python, "w") as f: + f.write("#!/bin/bash\n") + f.write('if [ "$1" = "-m" ] && [ "$2" = "egg_agent" ]; then\n') + f.write(" CALL_COUNT=0\n") + f.write(f" if [ -f {shlex.quote(call_counter)} ]; then\n") + f.write(f" CALL_COUNT=$(cat {shlex.quote(call_counter)})\n") + f.write(" fi\n") + f.write(" CALL_COUNT=$((CALL_COUNT + 1))\n") + f.write(f' echo "$CALL_COUNT" > {shlex.quote(call_counter)}\n') + f.write(f' echo "---CLAUDE_CALL_START---" >> {shlex.quote(claude_log)}\n') + f.write(' if [ "$CALL_COUNT" -eq 1 ]; then\n') + f.write(" exit 0\n") # clean exit → restart triggered + f.write(" fi\n") + # call 2: emit the overflow signature and exit 255 + f.write( + ' echo "Fatal error in message reader: Failed to decode ' + "JSON: JSON message exceeded maximum buffer size of " + '1048576 bytes..." >&2\n' + ) + f.write(" exit 255\n") + f.write("else\n") + f.write(f' exec {shlex.quote(real_python)} "$@"\n') + f.write("fi\n") + os.chmod(mock_python, 0o755) # nosec B103 + + cmd = build_consensus_wrapped_command( + "Prompt", max_restarts=3, transient_backoff_initial=1 + ) + result = TestConsensusWrapperBehavior._run_wrapper_command(cmd, tmpdir, timeout=30) + + # Wrapper must propagate the overflow crash's exit code from + # the restart loop, NOT continue retrying. + assert result.returncode == 255, result.stderr + # Diagnostic message must indicate the restart-loop path + # (the initial-exit handler says "Agent crashed on Claude + # Agent SDK buffer overflow ..."; the restart-loop handler + # adds "on restart N"). + assert "buffer overflow" in result.stderr.lower() + assert "on restart" in result.stderr + assert "#2804" in result.stderr + # Agent was called exactly twice: initial clean exit + one + # restart that crashed on overflow. Should NOT be called a + # third time. + with open(claude_log) as f: + call_count = f.read().count("---CLAUDE_CALL_START---") + assert call_count == 2, ( + f"Expected exactly 2 agent calls (initial + 1 restart with " + f"overflow), got {call_count}. The restart-loop buffer-overflow " + f"check must abort before consuming further retry budget." + ) + class TestEventDrivenWait: """Issue #1897 Phase 5 / TASK-5-1 (plan rev 4, reviewer_plan blocker 4): diff --git a/sandbox/entrypoint.py b/sandbox/entrypoint.py index 61453f24d6..55153a5248 100644 --- a/sandbox/entrypoint.py +++ b/sandbox/entrypoint.py @@ -1064,27 +1064,6 @@ def setup_claude(config: Config, logger: Logger) -> None: "defaultModel": "opus[1m]", "showResumeCommand": False, "memory": {"enabled": False}, - # PostToolUse truncation hook — bounds tool result size to prevent - # the Claude Agent SDK message-reader buffer overflow that kills - # agent processes with exit 255 on large tool results - # (issue #2804). The hook runs inside the CLI subprocess, before - # the result crosses the SDK JSON channel, so it can suppress - # oversized payloads that would crash the reader. Matcher is "*" - # (every tool) — the hook itself decides per-tool whether to - # block or pass through based on serialized size. - "hooks": { - "PostToolUse": [ - { - "matcher": "*", - "hooks": [ - { - "type": "command", - "command": ("/opt/egg-runtime/sandbox/hooks/posttooluse_truncate.py"), - } - ], - } - ] - }, } # In private mode, disallow web tools at the agent config level so they are diff --git a/sandbox/hooks/posttooluse_truncate.py b/sandbox/hooks/posttooluse_truncate.py deleted file mode 100755 index 47e327f5a7..0000000000 --- a/sandbox/hooks/posttooluse_truncate.py +++ /dev/null @@ -1,157 +0,0 @@ -#!/usr/bin/env python3 -"""PostToolUse hook: bound tool result size to prevent SDK buffer overflows. - -Issue #2804. The Claude Agent SDK message reader has a hard-coded JSON -buffer cap (1 MB default; we raise it via ``max_buffer_size`` but it is -still finite). Tool results that exceed the buffer kill the agent -process with exit code 255, which the consensus-wrapper then wastes its -restart budget retrying — a deterministic failure that no retry can -recover. - -This hook runs inside the Claude Code CLI subprocess after each tool -call. It checks the serialized size of the tool response and, when it -exceeds ``EGG_TOOL_RESULT_CAP_BYTES`` (default 200 KB), returns a -``decision: block`` response that suppresses the oversized payload and -feeds the agent a structured retry prompt instead. The agent sees -truncation guidance and re-issues the tool call with a narrower scope. - -Why CLI-side rather than programmatic SDK hooks: SDK hook callbacks -travel back to the Python process through the same buffered JSON stream -that has the cap. An oversized ``tool_response`` would crash the buffer -*before* the callback could see it. A CLI hook subprocess runs entirely -inside the CLI's process boundary and gets the tool response without -touching the SDK channel. - -Edit / Write / MultiEdit are handled with a specific reason — the file -mutation already happened, so the agent must NOT retry the edit; it -just needs to know the response snippet was suppressed. -""" - -from __future__ import annotations - -import json -import os -import sys -from typing import Any - -DEFAULT_CAP_BYTES = 200_000 - - -def _cap_bytes() -> int: - raw = os.environ.get("EGG_TOOL_RESULT_CAP_BYTES", "").strip() - if not raw: - return DEFAULT_CAP_BYTES - try: - value = int(raw) - except ValueError: - return DEFAULT_CAP_BYTES - return value if value > 0 else DEFAULT_CAP_BYTES - - -def _serialized_size(value: Any) -> int: - """Return the JSON-serialized byte length of ``value``. - - Matches what the SDK reader sees on the wire. Falls back to ``str()`` - for objects that aren't JSON-serializable. - """ - if value is None: - return 0 - if isinstance(value, (bytes, bytearray)): - return len(value) - if isinstance(value, str): - return len(value.encode("utf-8")) - try: - return len(json.dumps(value, default=str).encode("utf-8")) - except TypeError, ValueError: - return len(str(value).encode("utf-8")) - - -# Tools where the model should NOT retry the call — the side effect -# already happened on disk. The reason text tells the agent the -# mutation succeeded and the snippet was suppressed. -_MUTATING_TOOLS = frozenset({"Edit", "Write", "MultiEdit", "NotebookEdit"}) - -# Tool-specific guidance for tools that are safe to retry with a -# narrower scope. -_RETRY_GUIDANCE = { - "Read": ( - "Re-issue Read with the `offset` and `limit` parameters scoped to the " - "specific lines you need. Default limit is 2000 lines." - ), - "Bash": ( - "Re-issue the command with narrower scope: pipe through `head`, " - "`tail`, `wc -l`, or filter with `grep`. For long-running output, " - "redirect to a file and Read targeted chunks." - ), - "Grep": ( - "Re-issue Grep with a more specific pattern, a smaller `path`, " - "`--head-limit`, or output_mode='count' first to gauge result size." - ), - "Glob": ("Re-issue Glob with a tighter pattern or a smaller `path` scope."), -} - -_FALLBACK_RETRY_GUIDANCE = ( - "Re-issue the call with narrower input (smaller path, lower limit, more specific pattern)." -) - - -def _build_reason(tool_name: str, size: int, cap: int) -> str: - base = ( - f"Tool result for `{tool_name}` was {size} bytes, exceeding the " - f"{cap}-byte cap (issue #2804). The payload was suppressed to " - "prevent the Claude Agent SDK buffer overflow that would otherwise " - "kill the agent process." - ) - if tool_name in _MUTATING_TOOLS: - return ( - f"{base} The file mutation itself succeeded — do NOT retry the " - "tool call. If you need to verify the result, use Read with " - "narrow `offset`/`limit` parameters." - ) - guidance = _RETRY_GUIDANCE.get(tool_name, _FALLBACK_RETRY_GUIDANCE) - return f"{base} {guidance}" - - -def evaluate(event: dict[str, Any], cap: int) -> dict[str, Any] | None: - """Inspect a PostToolUse event and return the hook output, or None to allow. - - Pure function exposed for testing — the CLI entrypoint wires it to - stdin/stdout. - """ - tool_response = event.get("tool_response") - if tool_response is None: - return None - size = _serialized_size(tool_response) - if size <= cap: - return None - tool_name = str(event.get("tool_name") or "unknown") - return { - "decision": "block", - "reason": _build_reason(tool_name, size, cap), - } - - -def main() -> int: - try: - raw = sys.stdin.read() - except OSError: - return 0 - if not raw.strip(): - return 0 - try: - event = json.loads(raw) - except json.JSONDecodeError: - # Unparseable input — allow the tool result through; the buffer - # bump and clean-error path in shared/egg_agent/client.py will - # catch any oversize payload as a structured failure. - return 0 - if not isinstance(event, dict): - return 0 - output = evaluate(event, _cap_bytes()) - if output is not None: - json.dump(output, sys.stdout) - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/sandbox/tests/test_entrypoint_settings.py b/sandbox/tests/test_entrypoint_settings.py deleted file mode 100644 index d21d93fe1a..0000000000 --- a/sandbox/tests/test_entrypoint_settings.py +++ /dev/null @@ -1,129 +0,0 @@ -"""Tests for settings.json generation in setup_claude(). - -Verifies that private-mode containers get disallowedTools for WebFetch/WebSearch, -while public-mode containers do not. -""" - -import json -import os -import sys -from pathlib import Path -from typing import Any -from unittest.mock import MagicMock, patch - -import pytest - -# Add sandbox/ to sys.path so entrypoint is importable -_sandbox_path = str(Path(__file__).parent.parent) -if _sandbox_path not in sys.path: - sys.path.insert(0, _sandbox_path) - -from entrypoint import setup_claude - - -@pytest.fixture() -def mock_config(tmp_path): - """Create a minimal Config-like object with a temp claude_dir.""" - claude_dir = tmp_path / ".claude" - claude_dir.mkdir() - config = MagicMock() - config.claude_dir = claude_dir - config.user_home = tmp_path - config.runtime_uid = os.getuid() - config.runtime_gid = os.getgid() - config.repo_path = tmp_path / "repo" - config.repo_path.mkdir() - return config - - -@pytest.fixture() -def mock_logger(): - return MagicMock() - - -def _read_settings(config: Any) -> dict[str, Any]: - result: dict[str, Any] = json.loads((config.claude_dir / "settings.json").read_text()) - return result - - -class TestDisallowedToolsPrivateMode: - """WebFetch/WebSearch should be disallowed only in private mode.""" - - @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "true"}) - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_private_mode_disallows_web_tools(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert settings["disallowedTools"] == ["WebFetch", "WebSearch"] - - @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "1"}) - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_private_mode_1_disallows_web_tools(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert settings["disallowedTools"] == ["WebFetch", "WebSearch"] - - @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "false"}) - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_public_mode_no_disallowed_tools(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert "disallowedTools" not in settings - - @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "0"}) - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_public_mode_0_no_disallowed_tools(self, _which, mock_config, mock_logger): - """EGG_PRIVATE_MODE=0 is set by sandbox_template.py for public mode.""" - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert "disallowedTools" not in settings - - @patch.dict(os.environ, {"EGG_PRIVATE_MODE": ""}, clear=False) - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_unset_env_no_disallowed_tools(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert "disallowedTools" not in settings - - -class TestPostToolUseHook: - """Issue #2804: settings.json must wire the PostToolUse truncation - hook so the CLI runs it for every tool call, bounding tool_response - size before it crosses the SDK JSON channel. - """ - - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_hook_registered_for_post_tool_use(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - assert "hooks" in settings, "settings.json must include hooks for #2804" - assert "PostToolUse" in settings["hooks"] - - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_hook_matcher_covers_all_tools(self, _which, mock_config, mock_logger): - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - matchers = settings["hooks"]["PostToolUse"] - assert len(matchers) == 1 - # ``*`` matches every tool so we catch Read/Bash/Grep/Edit/MCP alike - assert matchers[0]["matcher"] == "*" - - @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") - def test_hook_points_at_runtime_path(self, _which, mock_config, mock_logger): - """The command must reference the in-image runtime path. - - The Dockerfile bakes ``sandbox/`` to ``/opt/egg-runtime/sandbox/`` - (see ``sandbox/Dockerfile``); the hook script lives at - ``hooks/posttooluse_truncate.py`` under that root. - """ - setup_claude(mock_config, mock_logger) - settings = _read_settings(mock_config) - commands = [ - cmd["command"] - for matcher in settings["hooks"]["PostToolUse"] - for cmd in matcher["hooks"] - if cmd.get("type") == "command" - ] - assert any( - "/opt/egg-runtime/sandbox/hooks/posttooluse_truncate.py" in cmd for cmd in commands - ), f"hook command must reference runtime path, got {commands!r}" diff --git a/sandbox/tests/test_posttooluse_truncate.py b/sandbox/tests/test_posttooluse_truncate.py deleted file mode 100644 index 43ef3d91c1..0000000000 --- a/sandbox/tests/test_posttooluse_truncate.py +++ /dev/null @@ -1,230 +0,0 @@ -"""Tests for the PostToolUse truncation hook (issue #2804).""" - -from __future__ import annotations - -import importlib.util -import io -import json -import sys -from pathlib import Path -from typing import Any -from unittest.mock import patch - -import pytest - -# Import the script as a module (it has a .py suffix but lives in -# sandbox/hooks/ which is not a Python package). -_HOOK_PATH = Path(__file__).parent.parent / "hooks" / "posttooluse_truncate.py" -_spec = importlib.util.spec_from_file_location("posttooluse_truncate", _HOOK_PATH) -assert _spec is not None and _spec.loader is not None -posttooluse_truncate = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(posttooluse_truncate) - - -class TestSerializedSize: - """``_serialized_size`` must match the byte length the SDK reader sees.""" - - def test_none_is_zero(self): - assert posttooluse_truncate._serialized_size(None) == 0 - - def test_str_is_utf8_length(self): - assert posttooluse_truncate._serialized_size("hello") == 5 - # Multi-byte characters count their UTF-8 length, not codepoints - assert posttooluse_truncate._serialized_size("é") == 2 - - def test_dict_is_json_encoded(self): - payload = {"key": "value"} - encoded = json.dumps(payload).encode("utf-8") - assert posttooluse_truncate._serialized_size(payload) == len(encoded) - - def test_list_of_blocks(self): - # Matches the Claude Code tool_result shape: list of content blocks - payload = [{"type": "text", "text": "x" * 1000}] - size = posttooluse_truncate._serialized_size(payload) - # Size should be at least the inner text length - assert size >= 1000 - - def test_falls_back_to_str_for_unserializable(self): - class Weird: - def __repr__(self) -> str: - return "WEIRD-OBJECT" - - # default=str in json.dumps catches most; we just need this not to raise - result = posttooluse_truncate._serialized_size(Weird()) - assert result > 0 - - -class TestCapBytes: - """Env override must apply and reject garbage values cleanly.""" - - def test_default_cap(self): - with patch.dict("os.environ", {}, clear=False): - # Ensure unset - import os - - os.environ.pop("EGG_TOOL_RESULT_CAP_BYTES", None) - assert posttooluse_truncate._cap_bytes() == 200_000 - - def test_env_override(self): - with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "50000"}): - assert posttooluse_truncate._cap_bytes() == 50_000 - - def test_env_zero_falls_back_to_default(self): - """Zero or negative caps are nonsensical — fall back to default.""" - with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "0"}): - assert posttooluse_truncate._cap_bytes() == 200_000 - - def test_env_negative_falls_back_to_default(self): - with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "-1"}): - assert posttooluse_truncate._cap_bytes() == 200_000 - - def test_env_garbage_falls_back_to_default(self): - with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "not-a-number"}): - assert posttooluse_truncate._cap_bytes() == 200_000 - - -class TestEvaluate: - """``evaluate`` is the pure decision function — under cap = None - (allow), over cap = block with reason. - """ - - def test_under_cap_returns_none(self): - event = {"tool_name": "Read", "tool_response": "small payload"} - assert posttooluse_truncate.evaluate(event, cap=200_000) is None - - def test_at_exact_cap_returns_none(self): - # Equal to cap is still allowed; only strictly greater blocks. - payload = "x" * 100 - event = {"tool_name": "Read", "tool_response": payload} - assert posttooluse_truncate.evaluate(event, cap=100) is None - - def test_over_cap_returns_block(self): - payload = "x" * 10_000 - event = {"tool_name": "Read", "tool_response": payload} - result = posttooluse_truncate.evaluate(event, cap=5_000) - assert result is not None - assert result["decision"] == "block" - assert "Read" in result["reason"] - - def test_missing_tool_response_allows(self): - event = {"tool_name": "Read"} - assert posttooluse_truncate.evaluate(event, cap=100) is None - - def test_reason_cites_issue(self): - event = {"tool_name": "Bash", "tool_response": "x" * 1000} - result = posttooluse_truncate.evaluate(event, cap=500) - assert result is not None - assert "#2804" in result["reason"] - - def test_reason_reports_size_and_cap(self): - event = {"tool_name": "Read", "tool_response": "x" * 1000} - result = posttooluse_truncate.evaluate(event, cap=500) - assert result is not None - # Both numbers should appear so the agent (and the operator - # debugging logs) can see how badly the call overshot. - assert "1000" in result["reason"] or "1,000" in result["reason"] - assert "500" in result["reason"] - - -class TestMutatingTools: - """Edit/Write/MultiEdit/NotebookEdit already mutated disk; the - agent must be told NOT to retry the call. - """ - - @pytest.mark.parametrize("tool", ["Edit", "Write", "MultiEdit", "NotebookEdit"]) - def test_mutating_tool_reason_warns_against_retry(self, tool: str) -> None: - event = {"tool_name": tool, "tool_response": "x" * 10_000} - result = posttooluse_truncate.evaluate(event, cap=1_000) - assert result is not None - # The reason must explicitly tell the model NOT to retry — re-running - # an Edit that already landed on disk would either fail (string-not-found) - # or worse, double-apply. - assert "do NOT retry" in result["reason"] or "do not retry" in result["reason"].lower() - - @pytest.mark.parametrize("tool", ["Read", "Bash", "Grep", "Glob"]) - def test_idempotent_tool_reason_offers_retry_guidance(self, tool: str) -> None: - event = {"tool_name": tool, "tool_response": "x" * 10_000} - result = posttooluse_truncate.evaluate(event, cap=1_000) - assert result is not None - # For idempotent tools the agent SHOULD retry, just with narrower scope. - # Reason must contain actionable guidance (offset/limit, head, narrow pattern, etc.) - reason_lower = result["reason"].lower() - assert any( - hint in reason_lower - for hint in ("offset", "limit", "head", "narrow", "smaller", "scope") - ), f"Reason for {tool} should contain retry guidance: {result['reason']!r}" - - def test_unknown_tool_gets_fallback_guidance(self): - event = {"tool_name": "FutureTool", "tool_response": "x" * 10_000} - result = posttooluse_truncate.evaluate(event, cap=1_000) - assert result is not None - assert "FutureTool" in result["reason"] - assert "narrower" in result["reason"].lower() - - -class TestMain: - """End-to-end stdin/stdout protocol that the CLI hook actually - invokes. - """ - - @staticmethod - def _run_main(payload: Any) -> tuple[int, str]: - stdin = io.StringIO(json.dumps(payload) if payload is not None else "") - stdout = io.StringIO() - with patch.object(sys, "stdin", stdin), patch.object(sys, "stdout", stdout): - rc = posttooluse_truncate.main() - return rc, stdout.getvalue() - - def test_under_cap_writes_nothing(self): - rc, out = self._run_main( - {"tool_name": "Read", "tool_response": "small"}, - ) - assert rc == 0 - assert out == "" - - def test_over_cap_writes_block_json(self): - # Cap is 200KB by default; payload > 200KB to force block - rc, out = self._run_main( - {"tool_name": "Read", "tool_response": "x" * 300_000}, - ) - assert rc == 0 - parsed = json.loads(out) - assert parsed["decision"] == "block" - assert "Read" in parsed["reason"] - - def test_empty_stdin_allows(self): - rc, out = self._run_main(None) - assert rc == 0 - assert out == "" - - def test_malformed_json_allows(self): - """Unparseable input should fall through, not crash. - - The buffer-bump + clean-error path in client.py will catch any - oversize payload that slips past as a structured failure; the - hook should never amplify a parse error into an agent crash. - """ - stdin = io.StringIO("not json at all") - stdout = io.StringIO() - with patch.object(sys, "stdin", stdin), patch.object(sys, "stdout", stdout): - rc = posttooluse_truncate.main() - assert rc == 0 - assert stdout.getvalue() == "" - - def test_non_dict_input_allows(self): - rc, out = self._run_main(["not", "a", "dict"]) - assert rc == 0 - assert out == "" - - def test_respects_env_cap(self): - """Lower the cap via env so a small payload triggers block.""" - import os - - with patch.dict("os.environ", {"EGG_TOOL_RESULT_CAP_BYTES": "50"}): - os.environ["EGG_TOOL_RESULT_CAP_BYTES"] = "50" - rc, out = self._run_main( - {"tool_name": "Bash", "tool_response": "x" * 200}, - ) - assert rc == 0 - parsed = json.loads(out) - assert parsed["decision"] == "block" diff --git a/shared/egg_agent/client.py b/shared/egg_agent/client.py index 650c26ed8a..5c47f64605 100644 --- a/shared/egg_agent/client.py +++ b/shared/egg_agent/client.py @@ -61,36 +61,17 @@ def error(self, msg: str, **kwargs: Any) -> None: # Default model for sandbox agents DEFAULT_MODEL = "opus[1m]" -# Belt-and-suspenders bump on the Claude Agent SDK message-reader buffer. -# The SDK default is 1 MB; tool results that exceed it kill the agent -# process with exit 255 (issue #2804). The primary defense is the -# CLI-side PostToolUse hook (see sandbox/hooks/posttooluse_truncate.py) -# which suppresses oversized payloads before they cross the SDK channel. -# This bump gives breathing room for tool results that slip past the -# hook (e.g. results just under the per-tool cap that aggregate with -# subsequent CLI frames). Env override allows ops to raise it further -# without a code change. See ``ClaudeAgentOptions.max_buffer_size``. -_DEFAULT_MAX_BUFFER_SIZE = 4 * 1024 * 1024 # 4 MB - -# Substring of the SDK's CLIJSONDecodeError message that identifies the -# buffer-overflow failure mode specifically. Surfaced into the -# ``AgentResult.error`` so the consensus-wrapper can classify it as a -# terminal (deterministic) failure rather than a transient crash worth -# retrying. Match-string kept stable; the wrapper greps for it. +# Substring of the SDK's CLIJSONDecodeError message identifying the +# 1 MB JSON message-reader buffer overflow (issue #2804). The overflow +# is deterministic — the same tool call against the same codebase +# produces the same oversized payload — so the consensus-wrapper greps +# for this marker on agent exit to short-circuit retry instead of +# burning the restart budget on a doomed re-run. The real fix is +# tool-layer truncation (see #2805); this PR only makes the failure +# mode observable and terminal. _BUFFER_OVERFLOW_MARKER = "exceeded maximum buffer size" -def _max_buffer_size() -> int: - raw = os.environ.get("EGG_AGENT_MAX_BUFFER_SIZE", "").strip() - if not raw: - return _DEFAULT_MAX_BUFFER_SIZE - try: - value = int(raw) - except ValueError: - return _DEFAULT_MAX_BUFFER_SIZE - return value if value > 0 else _DEFAULT_MAX_BUFFER_SIZE - - async def run_agent_async( prompt: str, *, @@ -211,7 +192,6 @@ async def _check_tool_permission( setting_sources=["project", "user"], disallowed_tools=disallowed, can_use_tool=tool_permission_callback, - max_buffer_size=_max_buffer_size(), ) if max_turns is not None: options.max_turns = max_turns diff --git a/tests/shared/egg_agent/test_client.py b/tests/shared/egg_agent/test_client.py index e8d87ebf1e..c5c2daad9d 100644 --- a/tests/shared/egg_agent/test_client.py +++ b/tests/shared/egg_agent/test_client.py @@ -100,8 +100,6 @@ class ClaudeAgentOptions: # type: ignore[no-redef] setting_sources: list[str] | None = None disallowed_tools: list[str] = field(default_factory=list) can_use_tool: Any = None - # issue #2804: bump the SDK's JSON message buffer - max_buffer_size: int | None = None @dataclass class PermissionResultAllow: # type: ignore[no-redef] @@ -823,47 +821,6 @@ def test_private_mode_empty_string_no_disallowed_tools(self, mock_query): assert opts.disallowed_tools == [] -class TestMaxBufferSize: - """Issue #2804: SDK message-reader buffer bump. - - The SDK default is 1 MB; we bump it to 4 MB (env-overridable) so - moderate-but-large tool results that slip past the PostToolUse hook - don't crash the reader. - """ - - @patch.dict(os.environ, {}, clear=False) - @patch("claude_agent_sdk.query", side_effect=_mock_query_success) - def test_default_max_buffer_size_is_4mb(self, mock_query): - env = os.environ.copy() - env.pop("EGG_AGENT_MAX_BUFFER_SIZE", None) - with patch.dict(os.environ, env, clear=True): - _run_async(run_agent_async("test prompt")) - opts = mock_query.call_args.kwargs["options"] - assert opts.max_buffer_size == 4 * 1024 * 1024 - - @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "16777216"}) - @patch("claude_agent_sdk.query", side_effect=_mock_query_success) - def test_env_override_raises_buffer(self, mock_query): - _run_async(run_agent_async("test prompt")) - opts = mock_query.call_args.kwargs["options"] - assert opts.max_buffer_size == 16_777_216 - - @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "not-a-number"}) - @patch("claude_agent_sdk.query", side_effect=_mock_query_success) - def test_garbage_env_falls_back_to_default(self, mock_query): - _run_async(run_agent_async("test prompt")) - opts = mock_query.call_args.kwargs["options"] - assert opts.max_buffer_size == 4 * 1024 * 1024 - - @patch.dict(os.environ, {"EGG_AGENT_MAX_BUFFER_SIZE": "0"}) - @patch("claude_agent_sdk.query", side_effect=_mock_query_success) - def test_zero_env_falls_back_to_default(self, mock_query): - """Zero is nonsensical — fall back rather than disabling the buffer.""" - _run_async(run_agent_async("test prompt")) - opts = mock_query.call_args.kwargs["options"] - assert opts.max_buffer_size == 4 * 1024 * 1024 - - class TestBufferOverflowErrorHandling: """Issue #2804: when the SDK raises CLIJSONDecodeError on a buffer overflow, the agent must return a structured failure with the @@ -876,7 +833,7 @@ def test_buffer_overflow_returns_failure_with_marker(self, mock_query): from claude_agent_sdk import CLIJSONDecodeError mock_query.side_effect = CLIJSONDecodeError( - "JSON message exceeded maximum buffer size of 4194304 bytes..." + "JSON message exceeded maximum buffer size of 1048576 bytes..." ) result = _run_async(run_agent_async("test prompt")) From cb351797cc7dadca7b2194576b76bcda1105fd50 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 19:27:14 +0000 Subject: [PATCH 3/4] =?UTF-8?q?Address=20PR=20#2810=20review=20feedback=20?= =?UTF-8?q?(B1=20+=20N1=E2=80=93N5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B1 (blocking): restore TestDisallowedToolsPrivateMode coverage. The previous fix-commit deleted sandbox/tests/test_entrypoint_settings.py wholesale to drop TestPostToolUseHook, but that file also covered the EGG_PRIVATE_MODE -> settings["disallowedTools"] branch in sandbox/entrypoint.py:1071-1075, which is still live. Re-add the 5 private-mode tests as tests/sandbox/test_entrypoint_settings.py (adjacent to the existing entrypoint tests). Verified all 5 pass. N1: wire _BUFFER_OVERFLOW_MARKER from shared/egg_agent/client.py into the test that asserts the SDK error message round-trips into result.error, and add a new test in orchestrator/tests/test_consensus_wrapper.py that asserts the bash wrapper script's grep substring matches the same constant — so a rename of the constant fails both tests rather than silently desyncing the client from the wrapper. N2: rewrite the comment in client.py to stop claiming "marker stability is verified by tests." It isn't — the tests verify round-trip propagation, not stability against a future SDK rename. Point at #2823 (filed below) for the real fix. N3: switch consensus_wrapper.py's default AGENT_OUTPUT_LOG path from the predictable /tmp/agent-output-$$.log to mktemp(1) when the env var isn't set. tee -a follows symlinks, so the predictable path is a theoretical pre-create attack in multi-tenant sandbox setups; mktemp is defense-in-depth (the container is single-tenant in normal operation). Falls back to the old path only if mktemp is missing from PATH. N4: filed #2823 to track pinning the SDK marker against silent drift. Couldn't fix in-PR — claude-agent-sdk isn't installed in the egg test environment / CI, so a smoke test that imports the SDK and asserts the marker substring would skip everywhere it could catch the regression. Three approaches enumerated in the issue. N5: extend the run_agent docstring comment in consensus_wrapper.py to explain why 2>&1 | tee -a interleaves stderr into the captured log (the SDK's overflow marker is emitted on stderr by logger.error in claude_agent_sdk.query, and the grep needs to see it in the same file as stdout), and note the trade-off for future log analysis. Tests: 137 passed across the touched files (tests/sandbox/ test_entrypoint_settings.py + tests/shared/egg_agent/test_client.py + orchestrator/tests/test_consensus_wrapper.py). --- orchestrator/consensus_wrapper.py | 22 +++- orchestrator/tests/test_consensus_wrapper.py | 19 ++++ shared/egg_agent/client.py | 11 +- tests/sandbox/test_entrypoint_settings.py | 102 +++++++++++++++++++ tests/shared/egg_agent/test_client.py | 16 ++- 5 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 tests/sandbox/test_entrypoint_settings.py diff --git a/orchestrator/consensus_wrapper.py b/orchestrator/consensus_wrapper.py index c0a9dc5001..68d131f1fb 100644 --- a/orchestrator/consensus_wrapper.py +++ b/orchestrator/consensus_wrapper.py @@ -124,7 +124,17 @@ # message-reader 1MB JSON buffer crash (issue #2804) which is # deterministic — retrying just hits the same overflow and burns # the restart budget for no gain. -AGENT_OUTPUT_LOG="${{AGENT_OUTPUT_LOG:-/tmp/agent-output-$$.log}}" +# +# Use ``mktemp`` for the default path so a co-tenant on the same host +# cannot pre-create a symlink at a predictable ``/tmp/agent-output-$$`` +# location (``tee -a`` follows symlinks). The container is single-tenant +# in normal operation; mktemp is defense-in-depth for multi-tenant +# sandbox setups. The fallback to ``/tmp/agent-output-$$.log`` only +# fires if ``mktemp`` itself isn't on PATH, which would be a much +# bigger problem. +if [ -z "${{AGENT_OUTPUT_LOG:-}}" ]; then + AGENT_OUTPUT_LOG="$(mktemp -t agent-output.XXXXXX 2>/dev/null || echo "/tmp/agent-output-$$.log")" +fi # Log wrapper messages to stderr so they never leak into agent SDK context. cw_log() {{ @@ -142,6 +152,16 @@ # substitution (> >(tee ...)) backgrounds the tee subshell and # doesn't wait, which races with the immediate is_buffer_overflow # grep that follows on agent exit. + # + # Note: ``2>&1 | tee -a`` interleaves stdout and stderr in the + # captured log. This is intentional — the SDK overflow marker + # is emitted on stderr (``logger.error`` in + # ``claude_agent_sdk.query``), and the grep that triggers + # is_buffer_overflow needs to see it in the same file as + # stdout. Side-effect: any future log analysis that depends on + # stdout/stderr separation will need to capture them separately + # upstream (e.g. via ``script`` or a wrapper process), not from + # ``$AGENT_OUTPUT_LOG``. if [ -n "$system_prompt" ]; then {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" 2>&1 | tee -a "$AGENT_OUTPUT_LOG" else diff --git a/orchestrator/tests/test_consensus_wrapper.py b/orchestrator/tests/test_consensus_wrapper.py index 63bf1a5c80..639e8be9ed 100644 --- a/orchestrator/tests/test_consensus_wrapper.py +++ b/orchestrator/tests/test_consensus_wrapper.py @@ -1353,6 +1353,25 @@ def test_script_defines_is_buffer_overflow(self): assert "is_buffer_overflow()" in script assert "exceeded maximum buffer size" in script + def test_script_marker_matches_client_constant(self): + """The bash grep substring must match ``_BUFFER_OVERFLOW_MARKER`` + in ``shared/egg_agent/client.py``. Renaming the constant without + updating the wrapper script silently regresses the short-circuit + — this test pins them together. Issue #2804. + """ + sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "shared")) + try: + from egg_agent.client import _BUFFER_OVERFLOW_MARKER + finally: + sys.path.pop(0) + + cmd = build_consensus_wrapped_command("Prompt") + script = cmd[2] + assert _BUFFER_OVERFLOW_MARKER in script, ( + f"consensus_wrapper script must grep for {_BUFFER_OVERFLOW_MARKER!r} " + "to match the marker emitted by run_agent_async on SDK overflow" + ) + def test_script_captures_agent_output(self): """Agent output must be tee'd to a log file the wrapper can grep.""" cmd = build_consensus_wrapped_command("Prompt") diff --git a/shared/egg_agent/client.py b/shared/egg_agent/client.py index 5c47f64605..15288c6ce2 100644 --- a/shared/egg_agent/client.py +++ b/shared/egg_agent/client.py @@ -132,8 +132,15 @@ async def run_agent_async( # caught by the existing handler below — issue #2804 relies on # its error message preserving the ``exceeded maximum buffer # size`` marker so the consensus-wrapper can short-circuit - # retry on this failure class. We don't import it explicitly - # here (the marker stability is verified by tests). + # retry on this failure class. Tests pin that the marker + # propagates into ``result.error`` once raised, and that the + # consensus-wrapper grep matches ``_BUFFER_OVERFLOW_MARKER``; + # stability of the marker against future ``claude-agent-sdk`` + # releases is NOT verified (the SDK could change the wording + # at any minor bump and the wrapper would silently fall back + # to burning the transient-crash retry budget). See #2823 for + # the follow-up to pin or smoke-test the marker against the + # installed SDK. except ImportError: return AgentResult( success=False, diff --git a/tests/sandbox/test_entrypoint_settings.py b/tests/sandbox/test_entrypoint_settings.py new file mode 100644 index 0000000000..58e616684d --- /dev/null +++ b/tests/sandbox/test_entrypoint_settings.py @@ -0,0 +1,102 @@ +"""Tests for settings.json generation in setup_claude(). + +Verifies that private-mode containers get disallowedTools for WebFetch/WebSearch, +while public-mode containers do not. +""" + +import json +import os +import sys +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest + +# Add sandbox/ to sys.path so entrypoint is importable +_sandbox_path = str(Path(__file__).parent.parent.parent / "sandbox") +if _sandbox_path not in sys.path: + sys.path.insert(0, _sandbox_path) + +from entrypoint import setup_claude + + +@pytest.fixture() +def mock_config(tmp_path): + """Create a minimal Config-like object with a temp claude_dir.""" + claude_dir = tmp_path / ".claude" + claude_dir.mkdir() + config = MagicMock() + config.claude_dir = claude_dir + config.user_home = tmp_path + config.runtime_uid = os.getuid() + config.runtime_gid = os.getgid() + config.repo_path = tmp_path / "repo" + config.repo_path.mkdir() + return config + + +@pytest.fixture() +def mock_logger(): + return MagicMock() + + +def _read_settings(config: Any) -> dict[str, Any]: + result: dict[str, Any] = json.loads((config.claude_dir / "settings.json").read_text()) + return result + + +class TestDisallowedToolsPrivateMode: + """WebFetch/WebSearch should be disallowed only in private mode. + + Originally lived in ``sandbox/tests/test_entrypoint_settings.py`` + alongside ``TestPostToolUseHook`` coverage for the #2804 truncation + hook. The hook was removed in this PR after review showed + PostToolUse ``decision: block`` does not suppress oversized tool + responses (#2810). The private-mode coverage moved here because the + production code at ``sandbox/entrypoint.py`` (the + ``EGG_PRIVATE_MODE`` branch that writes + ``settings["disallowedTools"]``) is still live. + + Defense-in-depth note: ``run_agent_async`` also sets + ``ClaudeAgentOptions.disallowed_tools`` via a CLI flag (see + ``shared/egg_agent/client.py``); that is a different code path + covered by ``tests/shared/egg_agent/test_client.py``. This file + covers the ``settings.json`` write layer. + """ + + @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "true"}) + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_private_mode_disallows_web_tools(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert settings["disallowedTools"] == ["WebFetch", "WebSearch"] + + @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "1"}) + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_private_mode_1_disallows_web_tools(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert settings["disallowedTools"] == ["WebFetch", "WebSearch"] + + @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "false"}) + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_public_mode_no_disallowed_tools(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert "disallowedTools" not in settings + + @patch.dict(os.environ, {"EGG_PRIVATE_MODE": "0"}) + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_public_mode_0_no_disallowed_tools(self, _which, mock_config, mock_logger): + """EGG_PRIVATE_MODE=0 is set by sandbox_template.py for public mode.""" + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert "disallowedTools" not in settings + + @patch.dict(os.environ, {"EGG_PRIVATE_MODE": ""}, clear=False) + @patch("entrypoint.shutil.which", return_value="/usr/bin/claude") + def test_unset_env_no_disallowed_tools(self, _which, mock_config, mock_logger): + setup_claude(mock_config, mock_logger) + settings = _read_settings(mock_config) + assert "disallowedTools" not in settings diff --git a/tests/shared/egg_agent/test_client.py b/tests/shared/egg_agent/test_client.py index c5c2daad9d..68ff125b93 100644 --- a/tests/shared/egg_agent/test_client.py +++ b/tests/shared/egg_agent/test_client.py @@ -9,7 +9,13 @@ from unittest.mock import patch import pytest -from egg_agent.client import _MAX_TOOL_CONTENT_LOG_LEN, _truncate, run_agent, run_agent_async +from egg_agent.client import ( + _BUFFER_OVERFLOW_MARKER, + _MAX_TOOL_CONTENT_LOG_LEN, + _truncate, + run_agent, + run_agent_async, +) # ── Mock SDK types ────────────────────────────────────────────────────────── # @@ -833,7 +839,7 @@ def test_buffer_overflow_returns_failure_with_marker(self, mock_query): from claude_agent_sdk import CLIJSONDecodeError mock_query.side_effect = CLIJSONDecodeError( - "JSON message exceeded maximum buffer size of 1048576 bytes..." + f"JSON message {_BUFFER_OVERFLOW_MARKER} of 1048576 bytes..." ) result = _run_async(run_agent_async("test prompt")) @@ -841,8 +847,10 @@ def test_buffer_overflow_returns_failure_with_marker(self, mock_query): assert result.success is False assert result.returncode == -1 # Marker must appear verbatim in ``error`` so the wrapper's grep - # in is_buffer_overflow() matches. - assert "exceeded maximum buffer size" in result.error + # in is_buffer_overflow() matches. Referencing the module-level + # constant here means a rename in client.py drives a test + # failure rather than silently desyncing from the wrapper grep. + assert _BUFFER_OVERFLOW_MARKER in result.error class TestToolInterception: From 1203fdd46339706c88328d7de05ff183ebc70e1b Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 19:48:28 +0000 Subject: [PATCH 4/4] Address PR #2810 review feedback (3rd-round nits) 1. is_buffer_overflow comment referenced a non-existent marker_stability_test. Replaced with the real test names (test_script_marker_matches_client_constant and the test_buffer_overflow_*_aborts_without_retry pair). 2. test_script_marker_matches_client_constant manually inserted shared/ on sys.path. conftest.py already does this for the orchestrator test session, so the manipulation was dead defense-in-depth. Replaced with a bare import and a comment pointing at conftest so the next reader knows why it works. 3. mktemp fallback comment in consensus_wrapper.py claimed the fallback only fires if mktemp isn't on PATH. mktemp can also fail on /tmp full (ENOSPC), tmpfs read-only, or fd exhaustion. Rewrote the comment to acknowledge those failure modes and to be explicit that the fallback is a known weakening of the symlink protection, not an unreachable branch. --- orchestrator/consensus_wrapper.py | 22 +++++++++++++------- orchestrator/tests/test_consensus_wrapper.py | 9 ++++---- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/orchestrator/consensus_wrapper.py b/orchestrator/consensus_wrapper.py index 68d131f1fb..bc19bbbc4e 100644 --- a/orchestrator/consensus_wrapper.py +++ b/orchestrator/consensus_wrapper.py @@ -129,9 +129,13 @@ # cannot pre-create a symlink at a predictable ``/tmp/agent-output-$$`` # location (``tee -a`` follows symlinks). The container is single-tenant # in normal operation; mktemp is defense-in-depth for multi-tenant -# sandbox setups. The fallback to ``/tmp/agent-output-$$.log`` only -# fires if ``mktemp`` itself isn't on PATH, which would be a much -# bigger problem. +# sandbox setups. The fallback to ``/tmp/agent-output-$$.log`` fires on +# any ``mktemp`` failure — missing from PATH, ``/tmp`` full (``ENOSPC``), +# tmpfs read-only, fd exhaustion, etc. The predictable-path attack +# window narrows considerably in practice (mktemp is in coreutils and +# the failure modes above are themselves rare), but the fallback is +# still a known weakening of the symlink protection rather than an +# unreachable branch. if [ -z "${{AGENT_OUTPUT_LOG:-}}" ]; then AGENT_OUTPUT_LOG="$(mktemp -t agent-output.XXXXXX 2>/dev/null || echo "/tmp/agent-output-$$.log")" fi @@ -180,11 +184,13 @@ # The substring matches CLI output from claude_agent_sdk emitted on # the buffer overflow path. If a future SDK bump changes the # wording, this grep silently falls through and the wrapper burns -# its retry budget again — the marker_stability_test in -# orchestrator/tests/test_consensus_wrapper.py exercises a synthetic -# log to keep this honest, but does not pin against the installed -# SDK. The real fix is tool-layer truncation (#2805); this is the -# fail-fast path until that lands. +# its retry budget again — the buffer-overflow tests in +# orchestrator/tests/test_consensus_wrapper.py (notably +# test_script_marker_matches_client_constant and the +# test_buffer_overflow_*_aborts_without_retry pair) exercise the +# wrapper against a synthetic log to keep this honest, but do not +# pin against the installed SDK. The real fix is tool-layer +# truncation (#2805); this is the fail-fast path until that lands. is_buffer_overflow() {{ [ -f "$AGENT_OUTPUT_LOG" ] || return 1 grep -q "exceeded maximum buffer size" "$AGENT_OUTPUT_LOG" 2>/dev/null diff --git a/orchestrator/tests/test_consensus_wrapper.py b/orchestrator/tests/test_consensus_wrapper.py index 639e8be9ed..2d08549eac 100644 --- a/orchestrator/tests/test_consensus_wrapper.py +++ b/orchestrator/tests/test_consensus_wrapper.py @@ -1359,11 +1359,10 @@ def test_script_marker_matches_client_constant(self): updating the wrapper script silently regresses the short-circuit — this test pins them together. Issue #2804. """ - sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "shared")) - try: - from egg_agent.client import _BUFFER_OVERFLOW_MARKER - finally: - sys.path.pop(0) + # ``orchestrator/tests/conftest.py`` already puts ``shared/`` on + # ``sys.path`` for the orchestrator test session, so the import + # below resolves without any per-test path munging. + from egg_agent.client import _BUFFER_OVERFLOW_MARKER cmd = build_consensus_wrapped_command("Prompt") script = cmd[2]