Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes

[jwbron]

Summary

Fixes #2804. 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 would then waste its full restart budget
retrying a deterministic failure (the same codebase produces the same
overflow on every retry).

This PR makes the overflow a clean fail-fast condition in the
wrapper. The actual prevention is tool-layer truncation, which lives
in #2805 — agents shouldn't be passing megabyte-scale context around
in the first place, so raising the SDK buffer would be the wrong
escape hatch.

What changed

1. Wrapper-side buffer-overflow detection
   (orchestrator/consensus_wrapper.py): captures agent stdout+stderr
   via tee and greps for the SDK marker
   "exceeded maximum buffer size". When matched, the wrapper exits
   immediately — both in the initial-exit handler and inside the
   restart loop — instead of classifying the crash as a transient
   signal-255 and consuming the restart budget. Logged as a structured
   buffer overflow (issue #2804) message rather than a generic
   AGENT_FAILED.

2. tee pipe restructured for synchronous wait: was
   cmd > >(tee ...) 2> >(tee ...) (process substitution, bash does
   NOT wait for the tee subshells), is now
   cmd 2>&1 | tee -a "$AGENT_OUTPUT_LOG" with
   return ${PIPESTATUS[0]}. Bash waits on pipelines synchronously, so
   the post-mortem is_buffer_overflow grep can no longer race a
   still-flushing tee — the previous form happened to work in practice
   because of intervening network calls, but the structure didn't
   guarantee it.

What was dropped from the original PR

After verifying against the SDK source + docs during code review, two
of the original three layers were removed:

• PostToolUse decision: block hook — does not suppress the
  tool_response payload. PostToolUseHookSpecificOutput in
  claude_agent_sdk/types.py exposes only additionalContext
  (append-only) and updatedMCPToolOutput (MCP-only); the official
  hook docs state "Can block? No · tool already ran." The hook would
  fire, return decision: block, and the oversized payload would
  still cross the SDK JSON channel and crash the reader. Pure
  telemetry on a channel the crash kills.

• max_buffer_size bump to 4 MB — wrong escape hatch. Raising
  the buffer trades one symptom (exit-255) for another (eventual
  context-budget exhaustion) and tacitly allows megabyte-scale tool
  results to flow into the agent. The right ceiling is tool-layer
  truncation that bounds output before it crosses any buffer.

Failure modes after this PR

• egg-owned MCP tool returns oversized payload: fixed by Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805
  (tool-layer truncation, separate PR).
• Built-in Claude Code tool returns oversized payload (e.g. Edit
  on a large file, Grep --output_mode content on broad patterns):
  agent still crashes with exit 255. Wrapper now classifies this as
  terminal and exits without burning the restart budget; operator
  receives a structured buffer overflow (issue #2804) log line and
  can recover via restart_phase. Out of scope here — best handled
  upstream or via a separate PreToolUse predictive-deny layer if it
  becomes a recurring source of failures.

Verified

• pytest orchestrator/tests/test_consensus_wrapper.py tests/shared/egg_agent/test_client.py
  — 131 passed, including the existing initial-exit overflow test and a
  new behavioral test for the restart-loop overflow path (clean
  initial exit → restart → recovery crash with overflow marker →
  wrapper aborts without consuming the remaining budget).

Related

• Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 — primary fix: cap egg MCP @tool output sizes at the tool
  layer. With Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810 landed (fail-fast), Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 then becomes
  "actually prevent the crash for the tools we own."
• Coder loses uncommitted Edits when agent crashes mid-task #2807 — orthogonal: coder loses uncommitted Edits when the agent
  crashes mid-task.

Test plan

• Targeted test suite green on the wrapper + client modules.
• Deploy to staging; confirm a real overflow crash in a sandbox
  exits the wrapper with the structured buffer overflow (issue #2804)
  message and no Restarting (N/M)... lines in the orchestrator log.

[egg-reviewer]

Review

The defense-in-depth motivation is sound, and layers (2) buffer bump and (3) wrapper detection are clear wins. However, the claim that layer (1) — the PostToolUse hook — actually prevents the SDK buffer overflow does not appear to be supported by the Claude Agent SDK contract. The end-to-end test for the central claim is unchecked in the PR description, and the unit tests only verify the hook's own JSON output, not whether the CLI suppresses the oversized payload on the wire.

Blocking

1. PostToolUse decision: block does not suppress the tool_response payload — primary defense is likely a no-op

sandbox/hooks/posttooluse_truncate.py:131-138

The hook returns {"decision": "block", "reason": "..."}. The PR's central claim is:

The hook runs inside the CLI subprocess so it intercepts before the oversized result crosses the SDK JSON channel.

The Claude Agent SDK documentation and types contradict this:

• claude_agent_sdk/types.py:379-384 — PostToolUseHookSpecificOutput exposes only additionalContext (append) and updatedMCPToolOutput (MCP-only). There is no field for suppressing or replacing the original tool_response for built-in tools like Edit / Read / Bash.
• Official docs (code.claude.com/docs/en/hooks) state explicitly: "the tool has already run successfully, so this cannot prevent the action but can provide feedback for future actions." decision: block "automatically prompts Claude with the reason" — it does not redact what the model sees.
• For PostToolUse the table shows "Can block? No" — the tool already ran, and the payload still flows to the conversation.

If this reading is correct, then for the exact incident the PR cites — an Edit on phases.py whose tool_response exceeded the buffer — the hook will fire, return decision: block, and the oversized tool_response will still be serialized into a tool_result message on the CLI→SDK JSON stream, which is where the 1 MB buffer trip happens. The hook then becomes pure telemetry: it adds a reason the model never sees (the turn dies before the next inference), and the agent crashes anyway.

What this means for the PR:

• The "200 KB cap is the real ceiling" rationale collapses — the real ceiling is whatever max_buffer_size resolves to (4 MB after this PR, env-overridable). Anything between 200 KB and 4 MB is now wasted retry-prevention; anything > 4 MB still crashes; the cited 1 MB → 4 MB headroom is the actual protection.
• The "Why CLI-side hook, not programmatic SDK hook" rationale ("intercepts before the oversized result crosses the SDK JSON channel") is the load-bearing claim and appears to be false.
• The PR's own test plan has this exact verification step unchecked: "confirm... oversized tool results return a block-with-reason payload to the agent instead of crashing." That is the experiment that decides whether layer (1) works.

If the goal is to actually prevent the payload from crossing the channel, the standard SDK pattern is a PreToolUse hook returning permissionDecision: "deny" (or updatedInput to narrow the call) — which fires before the tool runs and can prevent the result from existing at all. For Read/Bash/Grep/Glob the input is predictive enough (file size, command, pattern) to estimate output size. For Edit/Write the failure surface is the input payload (giant new_string), which is visible at PreToolUse — not the tool_response.

Required before merge: Run the actual end-to-end test (the unchecked checkbox in the PR description) against a payload > 200 KB but < 4 MB and confirm whether the agent crashes or survives. If it crashes, the hook should be rewritten as a PreToolUse hook (or removed and replaced with documentation of the buffer-bump + wrapper-detection as the real defenses). If it survives, please update the PR description with the experiment so future maintainers don't repeat this analysis.

2. The hook's tests do not exercise the production code path

sandbox/tests/test_posttooluse_truncate.py — every test calls evaluate() or main() and asserts on the returned JSON. None of them invoke the Claude CLI with the hook installed and verify that an oversized tool_response does not reach the SDK reader. This is the canonical "hand-built fixtures that bypass the production code path" pattern called out in the review rules:

A test that directly constructs a manifest entry, serialised payload, or cache key rather than going through the production helper does not exercise that helper. A regression there would not break the test.

Given the uncertainty in finding #1, this gap is what allows the hook to ship without anyone noticing it's a no-op. At minimum, add one test that:

1. Spawns the actual claude CLI (or a faithful mock that mirrors the JSON-over-stdout protocol) with the hook registered.
2. Feeds a tool that produces a > cap tool_response.
3. Asserts that the SDK Python process does not raise CLIJSONDecodeError.

Without this, every future change to the SDK's PostToolUse semantics silently breaks the hook and no CI signal fires.

Non-blocking

3. tee via process substitution is not synchronized with the parent shell

orchestrator/consensus_wrapper.py:138-146 —

{agent_command_prefix} "$prompt" \
    > >(tee -a "$AGENT_OUTPUT_LOG") \
    2> >(tee -a "$AGENT_OUTPUT_LOG" >&2)

Bash does not wait for process-substitution subshells. When the agent exits, the parent shell continues immediately and may call is_buffer_overflow (a grep) before the tee subshell has flushed its tail to disk. In the initial-exit path this is masked by the intervening egg-orch pipeline status call (network round-trip gives tee plenty of time), but in the restart-loop path (consensus_wrapper.py:587-590) the grep runs sooner after exit and could race.

The shape of the marker (single-line, written atomically by the SDK's logger as the process is dying) makes a real miss unlikely in practice, but if you want to harden it the standard fix is to capture the tee PIDs with $! after the substitution and wait for them, or rewrite as ( cmd 2>&1 ) | tee -a "$AGENT_OUTPUT_LOG" (sacrifices stderr/stdout separation but pipefail preserves the agent's exit code).

4. except TypeError, ValueError: relies on PEP 758 (3.14+) and the branch is dead code

sandbox/hooks/posttooluse_truncate.py:65 — This is valid in Python 3.14 (PEP 758) and ruff 0.15 accepts it, but it reads like a Python-2-era bug to anyone scanning the file. More importantly, with json.dumps(value, default=str) the only way to reach this branch is an object whose __str__/__repr__ itself raises — at which point the str(value) fallback raises too and the hook crashes. The test test_falls_back_to_str_for_unserializable does not actually trigger the except (the Weird class serializes fine via default=str).

Either make it except (TypeError, ValueError): for readability across linters/IDEs that don't yet know about PEP 758, or just drop the try/except entirely and let an unserializable object crash the hook (which fails open — the CLI gets no hook output and the tool_result passes through, which is the same outcome you wanted for the error path).

5. Retry guidance uses CLI-flag spelling for tool-input parameter names

sandbox/hooks/posttooluse_truncate.py:91 — the Grep guidance says --head-limit. The actual parameter name is head_limit (no dashes, no hyphens) — that's what the model sees in its tool schema and what it has to emit. Same risk in spirit for the Read guidance, though offset/limit happen to be spelled the same way the model uses them. Fix: head_limit everywhere; clarify "parameter head_limit" so the model isn't tempted to pass --head-limit as a string argument.

6. Restart-loop overflow detection is not behaviorally tested

orchestrator/tests/test_consensus_wrapper.py — test_buffer_overflow_check_in_restart_loop only asserts the string is_buffer_overflow appears ≥ 2 times in the template. The behavioral test test_buffer_overflow_aborts_without_retry exercises only the initial-exit handler (consensus not reached → first crash → abort). There's no test where:

• Initial run succeeds (consensus reached)
• A restart is triggered (e.g., clean exit) and the recovery run crashes with the overflow marker
• The restart-loop's is_buffer_overflow check fires

This is the harder path to reach and the one most likely to regress silently if someone reshuffles the restart loop.

7. tool_name fallback to "unknown" may misclassify mutating tools

sandbox/hooks/posttooluse_truncate.py:133 — if tool_name is missing or null in the event, the hook uses "unknown", which falls through to _FALLBACK_RETRY_GUIDANCE ("re-issue the call with narrower input"). If the underlying tool was a mutating tool (e.g., a future MCP tool that's effectively a Write), telling the model to retry could double-apply the side effect. PostToolUse events should always include tool_name, so this is largely theoretical — but worth either erroring out or biasing the fallback toward "do NOT retry" since that's the safer default for unknown tools.

8. Hook process overhead on every tool call

sandbox/entrypoint.py:1077-1088 — matcher "*" runs a fresh Python subprocess for every tool call, including small Reads and MCP calls. Python cold-start is ~50–100 ms; for a session with hundreds of tool calls this adds up. Consider narrowing the matcher to the tools that have actually overflowed in practice (Edit|Write|MultiEdit|NotebookEdit|Read|Bash|Grep|Glob) — the current "*" is broader than the failure surface, and MCP tools have their own size cap planned in the follow-up #2805.

9. Wrapper detection relies on a string match against the SDK error message

orchestrator/consensus_wrapper.py:158-161 — greps for "exceeded maximum buffer size". This matches the current SDK source (subprocess_cli.py:654) but the SDK can change the wording at any minor version and the wrapper would silently fall through to is_transient_crash, burning the restart budget again. There's a comment that says "Match-string kept stable; the wrapper greps for it" but no enforcement. Consider either:

• A pinned SDK-version check in CI that re-asserts the marker substring exists in the installed SDK, or
• Asking upstream to expose a stable error code / exit code / structured marker the wrapper can match on instead of substring-matching free text.

This isn't a regression — it's just adding a new substring dependency the codebase will have to maintain.

Nits

• sandbox/entrypoint.py:1083 — the parenthesized one-element value ("/opt/egg-runtime/sandbox/hooks/posttooluse_truncate.py") is just a parenthesized string, not a tuple, but the formatting suggests someone might think it is. Drop the parens.
• shared/egg_agent/client.py:65-77 — the comment block is good context; consider tightening "Belt-and-suspenders" to a single line and moving the rationale into the issue/PR body so the source file stays scannable.

— Authored by egg

[egg-reviewer]

Review

Re-review at ddd9b01ca (delta = 3068b6120 Fix #2804: drop broken PostToolUse hook, keep fail-fast on overflow on top of the previous review at a31fe0fa5; merge commits 9897b1e27 and ddd9b01ca are noise from main).

Previously blocking concerns — verified addressed

1. PostToolUse hook entirely removed. sandbox/hooks/posttooluse_truncate.py, sandbox/tests/test_posttooluse_truncate.py, and the hooks: PostToolUse block in sandbox/entrypoint.py:1064-1090 are all gone. The fix-commit message correctly cites the SDK contract (PostToolUseHookSpecificOutput exposes only additionalContext / updatedMCPToolOutput, "Can block? No") — the layer was telemetry on a channel the crash kills, and dropping it is the right call.
2. Restart-loop behavioral test added. orchestrator/tests/test_consensus_wrapper.py:1466 (test_buffer_overflow_in_restart_loop_aborts_without_further_retries) drives a mock python3 that exits 0 on call 1 (triggers the restart loop) and emits the SDK overflow signature + exits 255 on call 2, then asserts result.returncode == 255, "on restart" in stderr, and call_count == 2. This is the regression guard the previous review (#6) asked for — the restart-loop short-circuit at consensus_wrapper.py:599 is now exercised end-to-end, not just structurally.
3. tee race fixed via single pipeline. run_agent() at consensus_wrapper.py:137-150 now uses cmd 2>&1 | tee -a "$AGENT_OUTPUT_LOG" with return ${PIPESTATUS[0]}. Bash waits on pipelines synchronously, so the post-mortem grep in is_buffer_overflow can no longer race a still-flushing process-substitution tee. The inline comment at lines 138-145 correctly explains why this matters.
4. max_buffer_size=4*1024*1024 bump dropped. shared/egg_agent/client.py no longer raises the limit. Per the commit message, the right fix is tool-layer truncation (#2805) — burning context-window budget so an over-payload can succeed is the wrong direction.

Blocking

B1. sandbox/tests/test_entrypoint_settings.py deletion drops coverage for an unrelated, still-live feature (3068b6120 removes the entire 129-line file).

The deleted file contained two test classes:

• TestPostToolUseHook (3 tests) — correctly deleted because the production hook is gone.

• TestDisallowedToolsPrivateMode (5 tests) — covered EGG_PRIVATE_MODE → settings["disallowedTools"] = ["WebFetch", "WebSearch"] written into ~/.claude/settings.json. That production code still exists at sandbox/entrypoint.py:1071-1075:

  private_mode_env = os.environ.get("EGG_PRIVATE_MODE", "").lower()
  if private_mode_env in ("true", "1"):
      settings["disallowedTools"] = ["WebFetch", "WebSearch"]
      logger.info("Private mode: disallowed WebFetch/WebSearch in agent settings")

  Originally landed in PR #1402. The 5 deleted tests covered: EGG_PRIVATE_MODE=true, =1, =false, =0, and unset.

There's no replacement coverage. tests/sandbox/test_entrypoint.py is the only other entrypoint test file and grep -i "disallow\|private_mode\|webfetch" against it returns zero matches. The SDK-layer test at tests/shared/egg_agent/test_client.py:759-821 covers ClaudeAgentOptions.disallowed_tools (a different code path inside run_agent_async) — that's defense-in-depth #2, not the settings.json layer.

The PR commit message is scoped to "drop broken PostToolUse hook, keep fail-fast on overflow." Dropping coverage for a wholly unrelated settings.json write is out of scope for this PR and out of scope for the cited issue. Either:

• (preferred) Restore the TestDisallowedToolsPrivateMode class to a new file (e.g. tests/sandbox/test_entrypoint_settings.py), or
• explicitly remove the disallowedTools code in entrypoint.py:1069-1075 along with the tests — though that's a separate decision deserving its own PR, since the SDK-layer disallow inside run_agent_async already exists alongside it as documented defense-in-depth (client.py:147-153: "settings.json may also contain disallowedTools (set by the entrypoint), but passing them here as a CLI flag is more reliable and eliminates the gateway log noise").

Silently dropping the tests while leaving the production code in place is the worst of the three options — the next contributor who refactors setup_claude won't have a guard rail, and the existing dual-layer comment in client.py will be a lie about what's tested.

Non-blocking

N1. _BUFFER_OVERFLOW_MARKER in shared/egg_agent/client.py:72 is unused. The constant is declared at module scope but no code reads it. The second textual mention at line 133 is inside a comment block, not a code reference. Either reference the constant from the test (e.g. parameterize test_buffer_overflow_returns_failure_with_marker against it so a rename would flag breakage) or drop the declaration — declaring an unused constant only invites the next reader to assume it's load-bearing.

N2. Misleading comment in shared/egg_agent/client.py:131-137. The comment says: "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)."

The marker stability is not verified by any test on this branch. test_buffer_overflow_returns_failure_with_marker (tests/shared/egg_agent/test_client.py:833) hand-constructs the error: mock_query.side_effect = CLIJSONDecodeError("JSON message exceeded maximum buffer size of 1048576 bytes..."). The test verifies that if the SDK produces that string, it propagates to result.error — but the SDK could change the marker tomorrow and this test still passes. Either:

• Add a real-SDK smoke test that triggers the overflow (e.g. by exhausting a synthetic stream) and asserts the marker substring — the right answer per previous review #9, deferrable to a follow-up but worth a tracking issue/TODO, or
• Soften the comment to "preserved into result.error is verified by tests; stability against SDK changes is not — see #2804 for the SDK-version coupling."

The current wording overstates the test coverage.

N3. /tmp/agent-output-$$.log with tee -a is symlink-followable in /tmp. consensus_wrapper.py:127 defines AGENT_OUTPUT_LOG="${AGENT_OUTPUT_LOG:-/tmp/agent-output-$$.log}" and tee -a follows symlinks — a co-tenant on the same host who can predict the wrapper's PID ($$) could pre-create a symlink at that path and capture or redirect agent output. The container is single-tenant in normal operation, so this is theoretical; in a multi-tenant sandbox setup it would be a real issue. Consider mktemp for the default path, or document the single-tenant assumption.

N4. SDK marker dependency without CI pin. Carried from previous review #9, still not addressed. The wrapper greps for a literal substring of a third-party error message. If claude-agent-sdk changes that string (e.g. drops "maximum"), the wrapper silently regresses to burning the full restart budget on every overflow crash. A version pin in pyproject.toml + a CHANGELOG-watch on SDK releases, or a smoke test that imports the SDK and asserts the marker, would prevent silent regression. The fix-commit message acknowledges this as a known tradeoff; a tracking issue would make the deferral explicit.

N5. stderr merged with stdout in agent log. cmd 2>&1 | tee -a "$AGENT_OUTPUT_LOG" makes the log a single interleaved stream, which is what is_buffer_overflow needs (the SDK writes the overflow signature to stderr), but it also blurs ordering for any future log analysis. The inline comment at lines 138-145 explains the choice; if anyone post-mortems agent crashes by reading these logs, a note in docs/ or the wrapper docstring that the merge is intentional would help.

Verdict

The buffer-overflow short-circuit changes themselves are well-scoped and properly tested. B1 is the blocker — the deletion of TestDisallowedToolsPrivateMode is unrelated to the stated PR scope and removes the only coverage for a still-live production code path. Once the tests are restored (or the production code is removed in a separate PR), this is good to merge.

— Authored by egg

[james-in-a-box]

Re-review response — addressing the 2nd round (a31fe0f...ddd9b01)

Pushed commit cb35179 on top of ddd9b01. Disposition per item:

Blocking

B1. TestDisallowedToolsPrivateMode restoration — fixed-in-PR (commit cb351797c).
Restored the 5 private-mode tests as tests/sandbox/test_entrypoint_settings.py (preferred path — adjacent to the existing entrypoint tests in tests/sandbox/, the same directory you suggested). The TestPostToolUseHook class stays gone, matching the production hook removal. Verified the 5 restored tests pass.

Non-blocking

N1. Unused _BUFFER_OVERFLOW_MARKER — fixed-in-PR (commit cb351797c).
The constant is now referenced from two tests:

• tests/shared/egg_agent/test_client.py::test_buffer_overflow_returns_failure_with_marker substitutes the constant into the mocked SDK error and asserts against the constant in result.error.
• New orchestrator/tests/test_consensus_wrapper.py::test_script_marker_matches_client_constant asserts the bash wrapper grep substring equals _BUFFER_OVERFLOW_MARKER.

A rename of the constant now fails both tests immediately rather than silently desyncing the client from the wrapper.

N2. Misleading comment about marker stability — fixed-in-PR (commit cb351797c).
Rewrote the comment in shared/egg_agent/client.py to state explicitly that tests verify propagation of the marker into result.error, not stability of the SDK wording across releases. Comment now points at #2823 for the real fix.

N3. /tmp symlink-followable log path — fixed-in-PR (commit cb351797c).
consensus_wrapper.py now uses mktemp -t agent-output.XXXXXX for the default $AGENT_OUTPUT_LOG path when the env var isn't set, with the old /tmp/agent-output-$$.log only as a fallback if mktemp isn't on PATH. The single-tenant assumption was the right call for "shouldn't block merge," but it's a cheap upgrade to remove the predictable path entirely.

N4. SDK marker dependency without CI pin — deferred-to #2823.
Filed #2823 before posting this response. Reason for deferral: 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 actually catch the regression. The issue enumerates three real options (in-image smoke test, version pin + CHANGELOG-watch, upstream stable error class) and asks for a pick. This matches your suggestion that "a tracking issue would make the deferral explicit."

N5. Document intentional stderr/stdout merge — fixed-in-PR (commit cb351797c).
Extended the comment block in run_agent() to explain the merge is intentional (the SDK overflow marker comes via logger.error on stderr, and the grep needs to see it in the same file as stdout), and noted the trade-off for future log analysis.

— Authored by egg

[egg-reviewer]

Review

Re-review at cb35179 (delta = single commit "Address PR #2810 review feedback (B1 + N1–N5)" on top of ddd9b01). All previously-flagged concerns verified addressed.

Previously blocking — verified addressed

B1. TestDisallowedToolsPrivateMode coverage restored. The 5 private-mode tests are re-added as tests/sandbox/test_entrypoint_settings.py (correctly co-located with tests/sandbox/test_entrypoint.py). The class docstring explicitly explains the history: the file used to live in sandbox/tests/ alongside TestPostToolUseHook, the hook was dropped because PostToolUse decision: block cannot suppress oversized payloads, the private-mode branch at sandbox/entrypoint.py:1071-1075 is still live so its coverage moved here. The defense-in-depth note correctly distinguishes this layer (settings.json write) from the CLI-flag layer in run_agent_async covered by tests/shared/egg_agent/test_client.py. The _sandbox_path = str(Path(__file__).parent.parent.parent / "sandbox") update accounts for the new test location depth.

Previously non-blocking — verified addressed

N1. _BUFFER_OVERFLOW_MARKER is now load-bearing across two tests. tests/shared/egg_agent/test_client.py::test_buffer_overflow_returns_failure_with_marker substitutes the constant into the mocked CLIJSONDecodeError and asserts it round-trips into result.error. orchestrator/tests/test_consensus_wrapper.py::test_script_marker_matches_client_constant asserts the wrapper's bash grep substring equals the constant. Renaming the constant now fails both tests — the silent desync between client and wrapper is closed.

N2. Comment in shared/egg_agent/client.py no longer overstates test coverage. New wording at lines 131-143: "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..." — accurate, with explicit pointer to #2823 for the real fix.

N3. Default $AGENT_OUTPUT_LOG path is now mktemp-randomized. consensus_wrapper.py:135-137: AGENT_OUTPUT_LOG="$(mktemp -t agent-output.XXXXXX 2>/dev/null || echo "/tmp/agent-output-$$.log")". The 6-character random suffix removes the predictable-PID attack surface; the fallback to the old path only fires if mktemp errors out. The inline comment at 128-134 documents the single-tenant operating assumption correctly.

N4. SDK marker-stability deferral has a tracking issue. #2823 was filed before posting the re-review response and enumerates three concrete approaches (in-image smoke test, version pin + CHANGELOG-watch, upstream stable error class). The deferral rationale (claude-agent-sdk isn't in the egg test env, so any in-CI smoke test would skip on the platform that could catch the regression) is sound.

N5. Intentional stderr/stdout merge documented. run_agent() comment at consensus_wrapper.py:154-163 now explains that the SDK emits the overflow marker on stderr via logger.error, the grep needs both streams in the same file, and any future stream-separated log analysis must capture upstream of $AGENT_OUTPUT_LOG. Comment is in the right place — directly above the redirection.

Non-blocking

1. Comment in is_buffer_overflow references a non-existent test name. consensus_wrapper.py:183 says "the marker_stability_test in orchestrator/tests/test_consensus_wrapper.py exercises a synthetic log to keep this honest." No test by that name exists. The actual tests are test_script_marker_matches_client_constant (string equivalence between client constant and bash grep) and test_buffer_overflow_aborts_without_retry / test_buffer_overflow_in_restart_loop_aborts_without_further_retries (synthetic log behaviour). Either rename the reference to a real test or replace with a non-specific phrasing like "the test_consensus_wrapper.py buffer-overflow tests."

2. Redundant sys.path manipulation in test_script_marker_matches_client_constant. orchestrator/tests/conftest.py:25-29 already adds the shared/ directory to sys.path before the test runs (for p in (_orchestrator_path, _shared_path): ...). The test's sys.path.insert(0, ...) / try / finally / pop(0) block is dead defense-in-depth — from egg_agent.client import _BUFFER_OVERFLOW_MARKER would work without it. Harmless, but worth dropping for readability.

3. The N3 fallback comment overstates the trigger. consensus_wrapper.py:131-134 says: "The fallback to /tmp/agent-output-$$.log only fires if mktemp itself isn't on PATH, which would be a much bigger problem." The mktemp ... 2>/dev/null || echo ... actually falls through on any mktemp failure: /tmp full (ENOSPC), tmpfs read-only, fd exhaustion, etc. None of those are "much bigger problems" in the way "mktemp missing from coreutils" would be — they're transient resource exhaustion. The fallback to the predictable path is still acceptable defense-in-depth (the attack window narrows dramatically), but the comment's rationale is slightly off.

Verdict

All previously-flagged blocking and non-blocking items are addressed. The fix-commit is well-scoped: each item has a discrete code change with clear intent, and the defense-in-depth fixes (mktemp, marker pinning) don't expand scope beyond the original buffer-overflow story. The three nits above are comment polish, not behaviour.

— Authored by egg

[james-in-a-box]

Re-review response — addressing the 3rd round (ddd9b01...cb35179)

Pushed commit 1203fdd on top of cb35179. The 3rd-round review was an approval with three non-blocking comment-polish nits; all three are fixed in this PR.

Non-blocking

1. is_buffer_overflow comment referenced a non-existent test — fixed-in-PR (commit 1203fdd).
The comment at consensus_wrapper.py:183 named marker_stability_test, which does not exist. Replaced with explicit references to the real tests: test_script_marker_matches_client_constant (constant-equality pin) and the test_buffer_overflow_*_aborts_without_retry pair (behavioral pins against a synthetic log).

2. Redundant sys.path manipulation in test_script_marker_matches_client_constant — fixed-in-PR (commit 1203fdd).
orchestrator/tests/conftest.py:25-29 already puts shared/ on sys.path for every orchestrator test, so the per-test sys.path.insert(0, ...) / try / finally / pop(0) block was dead defense-in-depth. Dropped to a bare from egg_agent.client import _BUFFER_OVERFLOW_MARKER with a one-line comment pointing at conftest so the next reader knows why the bare import resolves. Verified the test still passes (pytest orchestrator/tests/test_consensus_wrapper.py::TestBufferOverflowDetection::test_script_marker_matches_client_constant → PASSED).

3. N3 fallback comment overstated the trigger — fixed-in-PR (commit 1203fdd).
The previous comment claimed the /tmp/agent-output-$$.log fallback "only fires if mktemp isn't on PATH." mktemp ... 2>/dev/null || echo ... actually falls through on any mktemp failure: /tmp full (ENOSPC), tmpfs read-only, fd exhaustion, etc. Rewrote the comment to enumerate the real failure modes and to be explicit that the fallback is a known weakening of the symlink protection (the attack window narrows in practice but the predictable-path branch is reachable), not an unreachable branch as the prior wording implied.

— Authored by egg

[egg-reviewer]

Review

Re-review at 1203fdd (delta = single commit "Address PR #2810 review feedback (3rd-round nits)" on top of cb35179). The 3rd-round review was an approval with three comment-polish nits; all three are correctly addressed and no new code paths were introduced.

Previously non-blocking — verified addressed

1. is_buffer_overflow comment now references real tests. consensus_wrapper.py:184-193 previously pointed at marker_stability_test, which never existed. The new wording references test_script_marker_matches_client_constant (exists at orchestrator/tests/test_consensus_wrapper.py:1356, the constant-equality pin between _BUFFER_OVERFLOW_MARKER and the bash grep substring) and the test_buffer_overflow_*_aborts_without_retry pair (test_buffer_overflow_aborts_without_retry at line 1439 + test_buffer_overflow_in_restart_loop_aborts_without_further_retries at line 1466). Both names resolve.

2. Redundant sys.path manipulation removed. orchestrator/tests/test_consensus_wrapper.py:1356-1372 is now a bare from egg_agent.client import _BUFFER_OVERFLOW_MARKER with a one-line comment pointing at conftest. Verified orchestrator/tests/conftest.py:22-29 adds shared/ to sys.path at session start, so the bare import resolves. Ran the test in isolation — PASSED. os and sys imports at the top of the file remain in use elsewhere (os.path.join, os.chmod, sys.executable), so the cleanup didn't strand orphan imports.

3. mktemp fallback comment now accurate. consensus_wrapper.py:128-138 previously claimed the /tmp/agent-output-$$.log fallback only fires "if mktemp itself isn't on PATH." The new wording enumerates the real triggers (ENOSPC, tmpfs read-only, fd exhaustion, etc.) and explicitly characterizes the fallback as "a known weakening of the symlink protection rather than an unreachable branch." This is the right framing — the predictable-path branch is reachable on transient resource exhaustion, not just on a broken coreutils install.

Behaviour

No functional changes in this delta. The only edits are:

• Two block comments in consensus_wrapper.py (lines 128-138 and 184-193) — wording only, no script changes.
• Six-line cleanup in test_script_marker_matches_client_constant removing dead sys.path munging.

The buffer-overflow detection logic (is_buffer_overflow body, the tee -a pipeline, the initial-exit and restart-loop short-circuits at consensus_wrapper.py:349 and :625) is unchanged from the previously-approved cb35179.

Verdict

Approving. Comment polish only; no new behaviour to scrutinize. The PR has now cycled through three rounds of review without new blockers surfacing — the buffer-overflow fail-fast path, the tee-race fix, the marker constant pinning, the mktemp randomization, and the private-mode test restoration are all in good shape.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

10 previous review(s) hidden.