diff --git a/orchestrator/consensus_wrapper.py b/orchestrator/consensus_wrapper.py index 45d4040082..bc19bbbc4e 100644 --- a/orchestrator/consensus_wrapper.py +++ b/orchestrator/consensus_wrapper.py @@ -119,6 +119,27 @@ 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. +# +# 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`` 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 + # Log wrapper messages to stderr so they never leak into agent SDK context. cw_log() {{ echo "[consensus-wrapper] $*" >&2 @@ -127,12 +148,52 @@ run_agent() {{ 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. + # + # 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" + {agent_command_prefix} --system-prompt "$system_prompt" "$prompt" 2>&1 | tee -a "$AGENT_OUTPUT_LOG" else - {agent_command_prefix} "$prompt" + {agent_command_prefix} "$prompt" 2>&1 | tee -a "$AGENT_OUTPUT_LOG" fi - return $? + return ${{PIPESTATUS[0]}} +}} + +# 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 +# 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 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 }} # Helper: extract BRC agent state from pipeline status JSON @@ -286,7 +347,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 +622,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..2d08549eac 100644 --- a/orchestrator/tests/test_consensus_wrapper.py +++ b/orchestrator/tests/test_consensus_wrapper.py @@ -1340,6 +1340,236 @@ 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_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. + """ + # ``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] + 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") + 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 + + 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): ``check_confirmed_and_wait`` is an SSE-primary hybrid. diff --git a/sandbox/entrypoint.py b/sandbox/entrypoint.py index 63633e2942..55153a5248 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, diff --git a/shared/egg_agent/client.py b/shared/egg_agent/client.py index 272a132605..15288c6ce2 100644 --- a/shared/egg_agent/client.py +++ b/shared/egg_agent/client.py @@ -61,6 +61,16 @@ def error(self, msg: str, **kwargs: Any) -> None: # Default model for sandbox agents DEFAULT_MODEL = "opus[1m]" +# 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" + async def run_agent_async( prompt: str, @@ -118,6 +128,19 @@ 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. 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/sandbox/tests/test_entrypoint_settings.py b/tests/sandbox/test_entrypoint_settings.py similarity index 78% rename from sandbox/tests/test_entrypoint_settings.py rename to tests/sandbox/test_entrypoint_settings.py index ebc4d0655d..58e616684d 100644 --- a/sandbox/tests/test_entrypoint_settings.py +++ b/tests/sandbox/test_entrypoint_settings.py @@ -14,7 +14,7 @@ import pytest # Add sandbox/ to sys.path so entrypoint is importable -_sandbox_path = str(Path(__file__).parent.parent) +_sandbox_path = str(Path(__file__).parent.parent.parent / "sandbox") if _sandbox_path not in sys.path: sys.path.insert(0, _sandbox_path) @@ -47,7 +47,23 @@ def _read_settings(config: Any) -> dict[str, Any]: class TestDisallowedToolsPrivateMode: - """WebFetch/WebSearch should be disallowed only in private mode.""" + """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") diff --git a/tests/shared/egg_agent/test_client.py b/tests/shared/egg_agent/test_client.py index f566f0a313..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 ────────────────────────────────────────────────────────── # @@ -79,6 +85,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 = "" @@ -124,6 +135,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 +827,32 @@ def test_private_mode_empty_string_no_disallowed_tools(self, mock_query): assert opts.disallowed_tools == [] +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( + f"JSON message {_BUFFER_OVERFLOW_MARKER} of 1048576 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. 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: """Tests for can_use_tool-based tool interception."""