Fix #2884: raise SDK reader buffer (the real #2804 crash fix); reframe per-tool caps as cost discipline

[jwbron]

Summary

Fixes #2884. The #2804 buffer-overflow crash recurs on a routine Edit to a large file (orchestrator/routes/pipelines.py, 1.1 MB / 25k lines) despite #2876's Read/Grep cap — and it blocks #2777's implement phase.

Corrected root cause (the issue's first framing was a guess from missing logs; confirmed here against the checkpoint and the CLI source): the overflowing message is not model-bound. Claude Code attaches the entire original file to every Edit/Write result as transcript metadata (toolUseResult.originalFile) that the model never sees — only egg's Agent SDK stream-json reader decodes it. So a ~2 KB edit to a 1.1 MB file emits a >1 MB stream message and crashes the reader (exit 255), even though the model's tool_result is just the bounded fBB cat -n snippet.

Verified in cli.js 2.1.157:

• fBB (the Edit snippet builder) bounds the echoed snippet to new_string lines + 8 — file-size-independent, so it was never the culprit.
• The Edit tool yields data: { …, originalFile: <full file>, structuredPatch, … }; a separate mapToolResultToToolResultBlockParam builds the model-bound block (the small snippet). The full-file field rides the stream message, not the model conversation.

A per-call input/output cap can't see or police that metadata — so the correct lever is the reader buffer itself.

What changed

• client.py — the fix. Raise ClaudeAgentOptions.max_buffer_size above the 1 MiB SDK default to 32 MiB (tunable via EGG_SDK_MAX_BUFFER_BYTES; invalid values are logged and ignored). This costs no model context or tokens — the oversized field never reaches the model, and egg logs at most _MAX_TOOL_CONTENT_LOG_LEN of any result — only transient reader memory for one message. It also can't leak an oversized payload to the model: model-bound result sizes stay bounded independently (the predictive Read/Grep caps, the MCP @tool caps Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805, and CC's own Bash truncation). 32 MiB covers source files far larger than anything in the repo while bounding a runaway stream; Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810's fail-fast remains the clean backstop above it.

• De-misleading the prior layers (no reverts). The chain (Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 → Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810 → Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876) was framed as "prevent the 1 MB buffer crash, per tool." That frame is wrong for this crash. Reframed in tool_output_cap.py's docstring, the client.py predictive-cap and overflow-marker comments, and docs/reference/agent-recovery.md: the per-tool Read/Grep caps are model-context/cost discipline (a whole-file Read = ~275k tokens to the model), not crash prevention — the raised reader buffer is. Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805/Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876/Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810 keep their independent value; nothing is reverted.

• Tests. max_buffer_size wiring + EGG_SDK_MAX_BUFFER_BYTES resolver (default / override / invalid); the mock ClaudeAgentOptions gains max_buffer_size.

Why not the alternatives

• PreToolUse cap on Edit/Write input size — the crashing edit's new_string was 2.5 KB; an input cap never fires. Wrong axis.
• PreToolUse deny keyed on target-file size — would deny every edit to pipelines.py, i.e. block the #2777 decomposition it's meant to unblock. Self-defeating.
• Custom-transport read-layer truncation (the issue's "primary") — more general, but overrides SDK internals (its own fragility/durability cost) to survive a case that can't occur for source files; Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810 already makes the residual >32 MiB case a clean fail.

Test plan

• pytest tests/shared/egg_agent/test_client.py tests/shared/egg_agent/test_tool_output_cap.py — 97 passed.
• ruff check / ruff format / mypy clean on changed files (one pre-existing unrelated egg_agent_tools import-untyped note).

Related

• Agent SDK message reader has 1MB JSON buffer limit — large tool results crash agents with exit code 255 #2804 / Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810 — the crash and its fail-fast backstop (kept).
• Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 — egg-owned MCP @tool caps (kept; model-bound).
• Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876 / Fix #2876: bound built-in CC tool output via PreToolUse predictive cap #2877 — Read/Grep predictive caps (kept, reframed as cost discipline).
• #2777 — the sliced-implement decomposition this unblocks.
• Pin claude-agent-sdk buffer-overflow marker against silent SDK drift #2823 — pin the overflow marker against the SDK (still open).

[egg-reviewer]

Review

The core change — raising ClaudeAgentOptions.max_buffer_size above the SDK's 1 MiB default — is correct, the env-var resolver is well-defended (empty/non-int/non-positive all handled with logged fallback), and the investigation of toolUseResult.originalFile as the actual overflow source matches what I can verify in the installed SDK (subprocess_cli.py raises CLIJSONDecodeError when len(json_buffer) > self._max_buffer_size, and types.py:1467 confirms max_buffer_size: int | None = None).

But the PR is sold as a reframing on top of the fix — "the per-tool Read/Grep caps are model-context/cost discipline, not crash prevention" — and that reframing is applied inconsistently. The places the reader will actually consult are still telling the old story.

Blocking

1. shared/egg_tool_output.py / shared/egg_agent/tool_output_cap.py deny reasons still tell the agent the call would crash the session on a 1 MB buffer.

The PR rewrites the module docstring of tool_output_cap.py to say these caps are "model-context/cost discipline, not the buffer-crash fix" — but the strings the agent actually reads on a deny are untouched:

• tool_output_cap.py:222-226 (Read deny):

  "…reading it whole risks overflowing the agent's 1 MB message buffer and crashing the session (issue #2804)…"

• tool_output_cap.py:247-254 (Grep deny):

  "…can return an unbounded volume of matching lines and overflow the agent's 1 MB message buffer (issue #2804)…"

Both are now wrong in two ways: (a) the buffer is no longer 1 MB on the egg path — it's _DEFAULT_SDK_MAX_BUFFER_BYTES = 32 * 1024 * 1024 (32 MiB), and (b) the PR's own framing says these caps are not about crash prevention. Each Read/Grep deny the agent sees in production will contradict the module docstring you're shipping in the same diff. Rewrite the deny strings around context/cost (e.g. "Read denied: '…' is ~N KB, large enough that reading it whole would dump ~M tokens to the model — use offset/limit to page through it") and either drop the #2804 reference or pin it to #2884 for the residual-buffer story.

The same applies to shared/egg_tool_output.py (the egg MCP @tool cap module, not in this diff but in the same blast radius): shared/egg_tool_output.py:3, :45, :127, :294 all still describe the SDK reader as a "hard 1 MB JSON buffer" / "1 MB buffer crash". The reframing claim in the PR description ("Reframed in tool_output_cap.py's docstring…") leaves these neighbors inconsistent.

2. shared/egg_agent/tool_output_cap.py:40-46 justifies the 256 KiB default via the now-raised buffer.

# Default byte threshold above which a whole-file ``Read`` is denied.
# The SDK buffer is 1 MB; a Read result is the file bytes plus per-line
# number prefixes (~7-8 bytes/line) plus JSON-escaping inflation, and it
# shares the 1 MB message with the rest of the turn. 256 KiB leaves ample
# headroom while still letting moderate files through whole. Override with
# EGG_READ_CAP_BYTES.
_DEFAULT_READ_CAP_BYTES = 256 * 1024

Two of those sentences are now false on the egg path (The SDK buffer is 1 MB and it shares the 1 MB message). The default value (256 KiB) is still defensible under the new framing — 256 KiB is ~64k tokens, real cost discipline — but the reason for the value as documented here is no longer the operative one. Re-justify in cost/token terms so the next person tuning it isn't reading rationale that the rest of the PR contradicts.

3. orchestrator/consensus_wrapper.py still claims #2805 is the real fix.

consensus_wrapper.py:192-198:

# 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.

This is the comment block any human (or agent) walking the BRC failure path will land on first, and it directly contradicts the PR description: the PR argues the real fix is the raised reader buffer (#2884), not tool-layer truncation (#2805). consensus_wrapper.py:129 and :182 also still describe the buffer as "1 MB". The PR is in the area of the buffer story and explicitly takes a position on what the real fix is — finish the job here.

Non-blocking suggestions

4. docs/reference/agent-recovery.md:126 still describes the upstream cap as the operative one.

The new "SDK Reader Buffer" section is well-written, but the section immediately preceding it (lines 122-132) reads top-down as "egg's buffer is 1 MB, so the wrapper greps for the marker…" without indicating that egg overrides the default. The line-134 note you added points forward, but a reader who hasn't yet hit it sees an apparent contradiction with the next section. Reordering or re-prefacing line 126 to say "the upstream SDK default is 1 MiB; egg raises it (next section)" would close the loop.

5. Other stale 1 MB references in docs.

• docs/development/STRUCTURE.md:273: tool_output_cap.py description still says "denies calls likely to overflow the 1 MB SDK buffer".
• docs/reference/agent-tools.md:218,228: the egg MCP @tool cap section still says "hard 1 MB JSON reader buffer" and "The default leaves ~10× headroom under the 1 MB buffer". Both true upstream; both now misleading on the egg path.

6. _sdk_max_buffer_bytes() warning fires once per run_agent_async invocation; the sibling _read_cap_bytes() deduplicates.

tool_output_cap.py:108-122 maintains _warned_cap_values so a steady bad EGG_READ_CAP_BYTES warns once per distinct value. client.py:_sdk_max_buffer_bytes has no equivalent, so an operator who sets EGG_SDK_MAX_BUFFER_BYTES=2mb gets the warning every time an agent is spawned. Low priority because run_agent_async is called orders of magnitude less often than a Read tool call, but the inconsistency reads like a missed pattern transfer.

7. test_env_resolver_rejects_invalid_values doesn't verify the "logged" half of "logged and ignored".

def test_env_resolver_rejects_invalid_values(self):
    for bad in ("not-a-number", "0", "-5", "2mb"):
        with patch.dict(os.environ, {"EGG_SDK_MAX_BUFFER_BYTES": bad}):
            assert _sdk_max_buffer_bytes() == _DEFAULT_SDK_MAX_BUFFER_BYTES

The function's docstring says the bad value "is logged and ignored, so an operator typo can't silently re-expose the 1 MiB-overflow crash". A regression that dropped the logger.warning call (or routed it to debug level) would still pass this test. Patch the logger and assert at least one warning fires.

8. No upper bound on EGG_SDK_MAX_BUFFER_BYTES.

_sdk_max_buffer_bytes accepts any positive integer. An operator typo like EGG_SDK_MAX_BUFFER_BYTES=34359738368000 (a stray suffix-conversion) would be silently accepted as ~34 TiB, at which point the SDK reader is effectively unbounded and a runaway/malformed stream OOMs the container. The check would be cheap (e.g. cap at 1 GiB and warn) and consistent with the defensive posture you've already taken on the lower bound.

— Authored by egg

[james-in-a-box]

Thanks for the review. All eight items addressed in-PR as bbd7c9c.

Reframing follow-through (items 1–5):

1. Read/Grep predictive-cap deny strings reframed — shared/egg_agent/tool_output_cap.py: both deny messages no longer mention the 1 MB SDK buffer or "overflow"; they now describe the cost as "dump ~Nk tokens to the model in a single tool result — wasteful of context budget" / "dumping a large slice of the repo to the model in a single tool result". → fixed-in-PR (commit bbd7c9c)

2. _DEFAULT_READ_CAP_BYTES rationale rewritten — the 256 KiB justification now reasons in token terms (~4 B/token → ~64k tokens) rather than referencing the 1 MB buffer it no longer protects. → fixed-in-PR (commit bbd7c9c)

3. consensus_wrapper.py no longer calls Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 the "real fix" — is_buffer_overflow's comment now points at the raised reader buffer (Edit/Write result envelope overflows the 1MB SDK buffer on large files (not the snippet) — gap in #2876 #2884) as the structural fix and frames Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805/Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876 as independent cost discipline; the AGENT_OUTPUT_LOG comment drops the "1 MB JSON buffer" phrasing and notes egg raises it to 32 MiB. → fixed-in-PR (commit bbd7c9c)

4. docs/reference/agent-recovery.md line 126 — rewritten to acknowledge that egg raises the reader buffer to 32 MiB (configurable via EGG_SDK_MAX_BUFFER_BYTES), so the configured cap is what matters, not the SDK's 1 MiB default. → fixed-in-PR (commit bbd7c9c)

5. docs/development/STRUCTURE.md + docs/reference/agent-tools.md — both updated so the per-tool caps are described as cost/context discipline with cross-references to Edit/Write result envelope overflows the 1MB SDK buffer on large files (not the snippet) — gap in #2876 #2884 for the buffer change, rather than as the crash backstop. → fixed-in-PR (commit bbd7c9c)

Env-resolver hardening (items 6–8):

6. _sdk_max_buffer_bytes warning dedup — added _warned_sdk_buffer_values: set[str] and a _warn_invalid_sdk_buffer() helper mirroring the EGG_READ_CAP_BYTES pattern in tool_output_cap.py. A steady bad value now warns once per process; a re-broken knob warns again on the new bad value. → fixed-in-PR (commit bbd7c9c)

7. Test asserts the warning fires — test_env_resolver_rejects_invalid_values now patches client.logger and asserts mock_logger.warning.called for each rejected value, and clears the dedup set per iteration so each case exercises the warn path. New test_env_resolver_warning_dedups_per_value asserts warning.call_count == 1 after 5 calls with the same bad value. → fixed-in-PR (commit bbd7c9c)

8. Upper bound + clamp — added _MAX_SDK_MAX_BUFFER_BYTES = 1 GiB. Values above the cap now clamp to 1 GiB and emit a warning rather than being honored literally (catches the 34 TiB-typo class of mistakes). New test_env_resolver_clamps_absurdly_large_value covers the clamp path. → fixed-in-PR (commit bbd7c9c)

Smoke-tested the new behavior locally: default unset → 33554432 (32 MiB); EGG_SDK_MAX_BUFFER_BYTES=34359738368000 → 1073741824 (1 GiB) with one warning; EGG_SDK_MAX_BUFFER_BYTES=0 called 5× → 1 warning. make lint passes; the pre-existing mypy untyped-call warnings on check_builtin_tool_output_risk already existed on db6d685 and are unrelated to this PR.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Re-review

The eight items from the prior review were addressed in bbd7c9c:

• Items 1–3 / reframing of tool_output_cap.py deny strings, _DEFAULT_READ_CAP_BYTES rationale, and consensus_wrapper.py "real fix" comments — confirmed in the diff. The two main Read/Grep deny strings now talk about "context budget" rather than the 1 MB buffer crash, _DEFAULT_READ_CAP_BYTES is re-justified in token terms (~4 B/token → 64k tokens), and is_buffer_overflow()'s comment block now points at #2884 as the structural fix.
• Items 4–5 / agent-recovery.md and STRUCTURE.md + agent-tools.md — agent-recovery.md:126 now prefaces the "1 MiB SDK default vs. configured cap" distinction; the STRUCTURE.md tool_output_cap.py row and agent-tools.md cap section explicitly cross-ref #2884 and disclaim crash-prevention.
• Items 6–7 / env-resolver dedup + warning assertion in tests — _warned_sdk_buffer_values plus _warn_invalid_sdk_buffer() mirror the tool_output_cap pattern; test_env_resolver_rejects_invalid_values now patches logger and asserts warning.called per bad value, and a new test_env_resolver_warning_dedups_per_value pins the once-per-value behaviour.
• Item 8 / upper-bound clamp — _MAX_SDK_MAX_BUFFER_BYTES = 1 GiB with a clamping branch and test_env_resolver_clamps_absurdly_large_value exercising the 34 TiB-typo path.

7dd5814 is the necessary follow-up to test_denies_unbounded_read_of_large_file — the old assertion pinned #2804, which the bbd7c9c deny-string rewrite removed; the new assertion ("context budget" in reason) tracks the new framing.

That said, the bbd7c9c reframing pass missed two spots in modified files — both in scope for "finish the reframing":

Blocking

1. shared/egg_tool_output.py:160-163 contradicts its own new top-of-file docstring.

The new file docstring (you just wrote it in this PR) is:

"These caps are model-context/cost discipline, not the buffer-crash fix. The Agent SDK reader's buffer-overflow crash (#2804/#2884) is prevented by raising ClaudeAgentOptions.max_buffer_size in shared/egg_agent/client.py…"

But inside _truncation_marker, the trailing fallback comment still says:

    # Pathological tiny cap: even an empty preview leaves the fixed fields
    # over the cap. Drop the preview entirely so the marker is as small as it
    # can be — still sub-KB, so it can never threaten the 1 MB SDK buffer the
    # cap exists to protect.

"the 1 MB SDK buffer the cap exists to protect" is precisely the framing the rest of the PR — and the docstring at the top of this same file — is rejecting. Rewrite around "so the marker stays well within the cap budget even on a pathologically tiny cap_bytes" (or whatever phrasing matches the cost-discipline framing); drop the SDK-buffer-protection claim.

2. shared/egg_agent/tool_output_cap.py:164-169 agent-facing deny string for non-pageable binaries still says "without risking the overflow".

_read_remedy()'s binary-extension branch — the message an agent sees when it tries to whole-Read a large PNG / JPG / .ipynb (since notebooks are in _NON_PAGEABLE_BINARY_EXTENSIONS too) — was not rewritten:

    if suffix in _NON_PAGEABLE_BINARY_EXTENSIONS:
        return (
            "This binary file is returned whole and cannot be paged, so it "
            "cannot be read without risking the overflow. Avoid reading it "
            "whole; if you only need metadata, use Bash (e.g. 'file' or 'stat')."
        )

The other two deny strings (check_read_output_risk:225-230, check_grep_output_risk:254-262) were rewritten in this PR to talk about "context budget"; this third one was missed, so the three deny strings the agent can see from this module are not in the same voice. Worse, an agent landing on this message is told the failure mode is "the overflow" — the exact failure class the PR argues is now handled by the raised reader buffer, not by this hook — and might reasonably try to set EGG_SDK_MAX_BUFFER_BYTES higher (which won't help: this hook fires before the SDK reader ever sees the result). Rewrite the binary branch around the same cost-discipline framing as the text/PDF branches, e.g. "would dump the whole binary to the model in a single tool result — wasteful of context budget."

Non-blocking

3. shared/egg_agent/tool_output_cap.py:179-180 and :236-237 — docstrings of the two check_*_output_risk entry points still say "likely to overflow".

These are the public-API summary lines for both checkers:

• :180 — """Return a deny reason if a ``Read`` call is likely to overflow."""
• :237 — """Return a deny reason if a ``Grep`` call is likely to overflow."""

Internal docstring, not agent-facing, so lower priority — but for someone reading the module top-down, the file docstring says "not the buffer-crash fix", _DEFAULT_READ_CAP_BYTES's comment says "cost knob, not crash-prevention", and then the two functions' docstrings re-introduce "likely to overflow". Re-phrase as "likely to produce an excessive model-bound result" (or similar) so the file is internally consistent end-to-end.

4. docs/reference/agent-tools.md:245 — #2876 is still described as "predicts overflows".

"#2876 adds a PreToolUse hook that predicts overflows before the tool runs and denies the call with a narrowing hint."

This is the "Built-in tool cap (complementary)" paragraph you just rewrote; the surrounding cap paragraph (lines 217-227) and the cross-ref to agent-recovery.md#sdk-reader-buffer-the-crash-prevention-layer both spell out the new framing. The "predicts overflows" line still tells the reader the hook's job is overflow prediction. Re-phrase as "predicts when a result would be excessive before the tool runs and denies the call with a narrowing hint" to match the framing in the rest of the section.

5. _warn_invalid_sdk_buffer and _warned_sdk_buffer_values grow unbounded over a process lifetime.

Mirrors tool_output_cap._warned_cap_values (so the inconsistency from the prior review is fixed), but the set is never pruned. In normal use the cardinality is tiny — an operator sets one value and either fixes it or doesn't — so this is benign. Calling it out only because the same pattern in tool_output_cap has the same bound; if a future fix prunes one, prune both.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. All five items addressed in-PR as ee2c457.

Blocking:

1. shared/egg_tool_output.py:160-163 — _truncation_marker fallback comment reframed. The "still sub-KB, so it can never threaten the 1 MB SDK buffer the cap exists to protect" line now reads "well within the cap's context budget even at a pathologically tiny cap_bytes", matching the file docstring's cost-discipline framing. → fixed-in-PR (commit ee2c457)

2. shared/egg_agent/tool_output_cap.py:164-169 — binary-extension Read deny string rewritten. The agent-facing message no longer says "without risking the overflow"; it now reads "reading it would dump the entire binary to the model in a single tool result — wasteful of context budget", so the three Read/Grep deny strings the agent can see from this module are all in the same voice. → fixed-in-PR (commit ee2c457)

Non-blocking:

3. shared/egg_agent/tool_output_cap.py:179-180 and :236-237 — check_*_output_risk docstring summaries reframed. Both now read "would produce an excessive model-bound result" instead of "is likely to overflow", so the file is internally consistent end-to-end from docstring → cap-rationale → entry-point summaries → deny strings. → fixed-in-PR (commit ee2c457)

4. docs/reference/agent-tools.md:245 — #2876 description reframed. Now reads "predicts when a result would be excessive before the tool runs and denies the call with a narrowing hint", matching the surrounding cap paragraph and the cross-ref to agent-recovery.md#sdk-reader-buffer-the-crash-prevention-layer. → fixed-in-PR (commit ee2c457)

5. _warned_sdk_buffer_values growth bound. → disagree (benign, cardinality is bounded in practice by operator-set values). The reviewer's own note acknowledges this is benign — an operator sets one value and either fixes it or doesn't, so the set's cardinality is tiny across a process lifetime. Pruning would add code for a problem that does not occur, and the cross-module "future fix" coordination the reviewer suggests is exactly the kind of speculative change worth not making until there is a real reason. The tool_output_cap._warned_cap_values set has the same unbounded shape and the same benign profile; if a future fix prunes one, the other is a trivial follow-up.

make lint passes (the two pre-existing egg_agent_tools import-untyped mypy notes on client.py:387-388 were already present on 7dd5814 and remain unrelated to this PR).

— Authored by egg

[egg-reviewer]

Re-review

The four blocking items from the prior review are addressed in ee2c457:

• Items 1 / shared/egg_tool_output.py:160-163 _truncation_marker fallback comment — confirmed at shared/egg_tool_output.py:160-163. The "still sub-KB, so it can never threaten the 1 MB SDK buffer the cap exists to protect" line is now "well within the cap's context budget even at a pathologically tiny cap_bytes", which matches the file docstring's cost-discipline framing.
• Item 2 / shared/egg_agent/tool_output_cap.py:164-169 binary-extension Read deny string — confirmed at shared/egg_agent/tool_output_cap.py:164-170. The agent-facing message no longer says "without risking the overflow"; it now reads "reading it would dump the entire binary to the model in a single tool result — wasteful of context budget", so the three Read/Grep deny strings from this module are all in the same voice.
• Item 3 / shared/egg_agent/tool_output_cap.py:180,237 docstring summaries — both check_read_output_risk and check_grep_output_risk now read "would produce an excessive model-bound result" instead of "is likely to overflow", so the file is internally consistent top-to-bottom.
• Item 4 / docs/reference/agent-tools.md:244-246 #2876 description — confirmed: now reads "predicts when a result would be excessive before the tool runs", matching the surrounding cap section.
• Item 5 / _warned_sdk_buffer_values unbounded growth — the author's disagreement is reasonable. The set's cardinality is bounded in practice by distinct operator-set values, and the symmetric pattern in tool_output_cap._warned_cap_values has the same shape; pruning one without the other would be the inconsistency. Acceptable as-is.

The 7dd5814..ee2c457 diff is purely the reframing strings + the agent-tools.md tweak — no functional code, no test behavior changes. The Read/Grep deny-string assertion in test_denies_unbounded_read_of_large_file already moved to "context budget" in 7dd5814 and remains valid.

Non-blocking

1. tests/shared/egg_agent/test_client.py:981-986 — TestBuiltinOutputCapHook class docstring still says the hook denies calls "before they overflow the SDK's 1 MB buffer".

class TestBuiltinOutputCapHook:
    """Issue #2876: a PreToolUse hook predicts oversized built-in tool
    results (Read/Grep) and denies them before they overflow the SDK's
    1 MB buffer, telling the agent how to narrow the call. Always-on
    (not route-gated); disabled via EGG_TOOL_OUTPUT_CAP=false.
    """

This contradicts the new framing the PR ships in the same diff:

• shared/egg_agent/tool_output_cap.py:3 — "These caps are model-context/cost discipline, not the buffer-crash fix."
• docs/reference/agent-recovery.md:150 — "These caps are model-context/cost discipline, not crash prevention"
• orchestrator/consensus_wrapper.py:203-204 — "The per-tool MCP @tool caps (#2805) and Read/Grep predictive caps (#2876) are independent model-context/cost discipline — not the crash fix."

The PR is in the area of this file (it added the new TestSdkReaderBuffer class at :1076 immediately below this docstring), and the very next test class's docstring (TestBufferOverflowErrorHandling:1158-1164) was updated to acknowledge the raised reader buffer (#2884) makes the overflow path a rare backstop. This docstring is the last spot in the modified files where the old "1 MB buffer / overflow" framing still describes the predictive-cap hook's purpose. Reframe around "predicts oversized built-in tool results … and denies them as context-budget discipline" (or similar) so the test file matches the implementation it exercises.

Lower priority than the agent-facing strings — it's an internal test docstring — but it's the natural symmetric counterpart to items 2-3 (deny-string + entry-point docstring) you've already taken in this PR, and a reader skimming this test class to understand what's being tested would land on the contradicting framing first.

— Authored by egg

[james-in-a-box]

Thanks for the re-review and approval. The one non-blocking item addressed in-PR as a10e35e.

Non-blocking:

1. tests/shared/egg_agent/test_client.py:981-986 — TestBuiltinOutputCapHook class docstring reframed. The docstring no longer says the hook denies calls "before they overflow the SDK's 1 MB buffer". It now reads "denies them as model-context/cost discipline … Not the buffer-crash fix — that lives in the raised SDK reader buffer (Edit/Write result envelope overflows the 1MB SDK buffer on large files (not the snippet) — gap in #2876 #2884)", matching the framing in tool_output_cap.py:3, docs/reference/agent-recovery.md:150, and consensus_wrapper.py:203-204, and consistent with the adjacent TestBufferOverflowErrorHandling docstring (:1158-1164) that already acknowledges Edit/Write result envelope overflows the 1MB SDK buffer on large files (not the snippet) — gap in #2876 #2884 makes the overflow path a rare backstop. → fixed-in-PR (commit a10e35e)

make lint passes (the two pre-existing egg_agent_tools import-untyped mypy notes on client.py:387-388 were already present on ee2c457 and remain unrelated to this PR).

— Authored by egg

[egg-reviewer]

Re-review

The single non-blocking item from the prior review is addressed in a10e35e:

• tests/shared/egg_agent/test_client.py:981-986 — TestBuiltinOutputCapHook class docstring reframed. Confirmed in the diff at tests/shared/egg_agent/test_client.py:982-986: the old "denies them before they overflow the SDK's 1 MB buffer" is now "denies them as model-context/cost discipline … Not the buffer-crash fix — that lives in the raised SDK reader buffer (#2884)." This matches the framing in shared/egg_agent/tool_output_cap.py:3, docs/reference/agent-recovery.md:150, and orchestrator/consensus_wrapper.py:203-204, and is internally consistent with the two adjacent test classes in the same file — TestSdkReaderBuffer:1077-1082 (which describes the raised buffer as the #2884 fix) and TestBufferOverflowErrorHandling:1159-1165 (which acknowledges the raised buffer makes the overflow path a rare backstop).

The remaining 1 MiB references in this file (lines 1079, 1092) describe the upstream SDK default that egg overrides — they are load-bearing for the explanation of why egg raises the buffer and should stay.

The ee2c4577..a10e35e diff is purely the docstring rewrite — no functional code, no new assertions, no test behavior changes. All CI checks pass (Lint, Unit Tests, Integration Tests, Security Scan).

No blocking issues. No further non-blocking suggestions.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

11 previous review(s) hidden.