Fix #2876: bound built-in CC tool output via PreToolUse predictive cap

[jwbron]

Summary

Fixes #2876. Built-in Claude Code tools (Read, Grep, Edit, Bash) run inside the CLI, so egg can't wrap their output the way it caps its own MCP @tool payloads (#2805). A tool result above the Agent SDK's 1 MB JSON buffer kills the agent with exit 255 (#2804). #2810 made that a clean fail-fast but doesn't prevent it, and PostToolUse can't suppress an oversized built-in payload — it fires after the payload has crossed the channel that crashes the reader (#2810 dropped that approach).

This adds the remaining lever: a PreToolUse hook that fires before the tool runs and denies calls likely to overflow, with a reason telling the agent exactly how to narrow the call. This is the layer that blocked the #2777 slice-1 coder, which crashed reading the 1.1 MB, 24k-line orchestrator/routes/pipelines.py whole.

What changed

• shared/egg_agent/tool_output_cap.py (new) — predictive heuristics:
  • Read: no limit and file > EGG_READ_CAP_BYTES (default 256 KiB) → deny, point at offset/limit.
  • Grep: output_mode=content with no head_limit and no path/glob scope (whole-repo content dump) → deny, point at head_limit / files_with_matches. Stays narrow to avoid denying the common small content grep.
• shared/egg_agent/client.py — wired as always-on PreToolUse hooks (mirrors the existing _deny_web_tools pattern); the overflow hits every route including first-party Opus. Kill switch EGG_TOOL_OUTPUT_CAP=false; threshold tunable via EGG_READ_CAP_BYTES.
• Tests — predictive-deny path for Read/Grep on oversized inputs, kill switch, env-threshold, and hook registration.
• docs/reference/agent-recovery.md — documents the cap as the prevention layer ahead of Fix #2804: bound tool result size to prevent SDK buffer-overflow crashes #2810's fail-fast.

These are predictive (the hook can't see the result), so expect some false positives/negatives; #2810's fail-fast remains the backstop when a prediction misses.

Acceptance

• A coder reading a 24k-line / 1.1 MB file (the #2777 case) is redirected to offset/limit before it overflows the 1 MB buffer.
• The deny reason tells the agent exactly how to narrow the call (offset/limit/head_limit/files_with_matches).
• Tests exercise the predictive-deny path for Read/Grep on oversized inputs.

Test plan

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

Notes

• Edit/Write are not denied: modern CC returns a bounded snippet around the edit, and denying them outright would break editing the very large files #2777 needs. Bash stdout is already truncated by CC. Left as no-ops; revisit if they prove to overflow.

Related

• Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 — egg-owned MCP @tool caps (sibling).
• 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 SDK buffer-overflow crash and its fail-fast backstop.
• #2777 — the sliced-implement cleanup this layer unblocks.

[james-in-a-box]

Check Failure: Integration Tests / Integration Tests

What's failing: make deploy timed out after 180s — the gateway, litellm, and orchestrator pods never left Pending:

Warning  FailedScheduling  default-scheduler  0/1 nodes are available:
1 node(s) had untolerated taint(s).
ERROR: timed out after 180s waiting for egg-system deployments.
make: *** [Makefile:568: deploy] Error 1

Root cause: This is a CNI-bootstrap flake on the ephemeral k3s runner, not a code problem. Earlier in the same job, the Cilium install never converged — the cilium and cilium-envoy DaemonSet pods stayed Pending:

Errors: cilium        cilium        1 pods of DaemonSet cilium are not ready
        cilium-envoy  cilium-envoy  1 pods of DaemonSet cilium-envoy are not ready

When the Cilium agent never goes Ready, the node.cilium.io/agent-not-ready taint is never removed, so the application pods can't be scheduled and the deploy wait times out.

This PR's diff is Python-only (shared/egg_agent/tool_output_cap.py, shared/egg_agent/client.py, their tests, and docs/reference/agent-recovery.md) and touches nothing in the k8s / Cilium / deploy path. A sibling PR's Test run at the same timestamp (17:54) passed, confirming this is an isolated per-runner flake rather than a regression. (The ERROR: PyYAML not installed line is a pre-existing, benign warning from scripts/build-host-repo-map.py — it only results in an empty EGG_HOST_REPO_MAP and is unrelated to the timeout.)

What needs to be done:

• Re-run the failed Integration Tests job — it should pass on a clean runner. I can't trigger the re-run myself because gh run rerun is blocked through the sandbox gateway.

Suggestion: If this Cilium-bootstrap flake recurs, consider hardening scripts/install-cilium.sh (e.g. wait/retry on the agent DaemonSet rollout, or bump the convergence timeout) so a slow CNI rollout self-recovers instead of leaving the node tainted.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Integration Tests / Integration Tests": 1}

[egg-reviewer]

Review: PreToolUse predictive output cap (#2876)

I traced this end-to-end, including disassembling the bundled claude CLI to verify the overflow premise. The core design is sound and the feature genuinely works in egg's execution environment. One blocking observability gap and several non-blocking heuristic-quality notes below.

What I verified (the premise holds)

The PR's central claim — that an unbounded Read of a large file overflows the SDK's 1 MB JSON buffer in egg's path — is correct, and I confirmed why it differs from interactive Claude Code:

• The bundled CLI's interactive Read default size cap is $X8 = 262144 (256 KiB) — exactly the PR's _DEFAULT_READ_CAP_BYTES. Good calibration.
• But egg runs via the SDK tool-dispatch path, which invokes tools with fileReadingLimits:{maxTokens:1/0, maxSizeBytes:268435456} — i.e. infinite token cap, 256 MB byte cap. So in egg's path Read reads the whole file (the CLI even documents "OMIT to read the whole file"), and any file between ~1 MB and 256 MB overflows the SDK message buffer. The byte-size heuristic is the right shape for this path.
• The always-on claim holds: hooks fire even for the role-less string-prompt path. Query.wait_for_result_and_end_input() keeps stdin open while self.hooks is truthy, so the bidirectional control protocol delivers the PreToolUse callback. This is not a silent no-op for non-pipeline routes.
• client.py:198 is the only ClaudeAgentOptions constructor and client.py:427 the only query() call — the cap sits at the single chokepoint, so coverage is complete.
• The deny mechanism mirrors _deny_web_tools at the execution-model level (PreToolUse → permissionDecision: deny), and the SDK honors empty {} as "no opinion → allow" (_convert_hook_output_for_cli({}) == {}). Hook registration ordering vs. the DDG block at client.py:329+ does not clobber.
• Tests exercise the real production path (real temp files, real stat, real check functions) — no self-seeding goldens, no fixtures bypassing the code under test.

Blocking

1. EGG_READ_CAP_BYTES misconfiguration is silently swallowed (tool_output_cap.py:54-64).

def _read_cap_bytes() -> int:
    raw = os.environ.get("EGG_READ_CAP_BYTES", "").strip()
    if raw:
        try:
            value = int(raw)
            if value > 0:
                return value
        except ValueError:
            pass          # <- silent
    return _DEFAULT_READ_CAP_BYTES   # <- no-op default branch

This is an operator-facing tuning knob, and the function matches two of the named anti-patterns: a silent except ValueError: pass and a no-op fallback branch. An operator who sets EGG_READ_CAP_BYTES=2_000_000 (underscores), EGG_READ_CAP_BYTES=2mb, a typo, 0, or a negative value gets the 256 KiB default with no signal that their deliberately-set value was ignored. If they intended a larger cap they get more false-positive denials than they asked for; if smaller, fewer — either way their intent is dropped silently.

The fix is a one-liner: logger.warning(...) (or raise) when raw is non-empty but doesn't parse to a positive int, before falling back. The default-when-unset path should stay silent — only the set-but-invalid case needs to be loud. There's also no test covering the 0/negative branch; add one alongside the existing test_invalid_env_falls_back_to_default.

Non-blocking

2. Read deny is mis-targeted for non-text files (check_read_output_risk). The heuristic keys purely on byte size, so a >256 KiB image/PDF/notebook is denied with "Re-run Read with 'offset' and 'limit' … (e.g. offset=1, limit=2000)" — advice that does nothing for a binary read, and the agent has no per-call escape (only the global kill switch). Denying is arguably correct (a large image base64-encodes past 1 MB too), so this is a message-quality issue, not a functional break. Consider exempting known binary extensions, or softening the remedy text for non-source files.

3. Read false-negative: a large explicit limit bypasses the cap. tool_input.get("limit") is not None treats any limit as "bounded," so Read(path, limit=10_000_000) on a 1.1 MB file is allowed and still overflows. Since the deny reason actively tells the agent to "use limit," a model could comply with an oversized value. The #2810 fail-fast is the backstop, but consider gating on offset+limit×(est. bytes/line) instead of mere presence.

4. The Grep cap is largely redundant. I confirmed the bundled CLI's Grep documents head_limit "Defaults to 250 when unspecified," and the SDK dispatch override only touches globLimits/fileReadingLimits, not grep — so a whole-repo output_mode=content grep is already capped at ~250 entries (~25 KB of normal code, safe). The genuine overflow risk for Grep is 250 very long lines (minified JS, etc.), which this heuristic doesn't target, while it does deny the common broad-but-safe content grep. It's also trivially bypassed (path="." or glob="**/*"). Harmless and conservative, but the value/false-positive ratio is lower than the Read cap — worth knowing the real risk profile is long lines, not match count.

5. Hook uses captured resolved_cwd rather than input_data["cwd"] (client.py:303). PreToolUseHookInput carries the live cwd; using it would be marginally more robust if the agent's cwd ever diverges from the launch cwd. In practice Read requires absolute paths, so this almost never bites.

6. Minor test isolation. test_read_and_grep_matchers_registered_by_default uses clear=False and relies on EGG_TOOL_OUTPUT_CAP being unset in the ambient env; an env that sets it false would make the test fail spuriously. Pin it explicitly.

— Authored by egg

[james-in-a-box]

Thanks for the thorough end-to-end trace — the disassembly-backed confirmation of the 256 KiB calibration and the always-on string-prompt path was especially useful. Per-item disposition below. All code fixes are in commit 8ca2a84.

1. EGG_READ_CAP_BYTES misconfiguration silently swallowed (blocking) — fixed-in-PR (commit 8ca2a84)
_read_cap_bytes() now restructures the parse so a set-but-invalid value is logged loudly before falling back: a non-integer (2mb, typo) and a non-positive value (0, negative) each emit a distinct logger.warning. The unset path stays silent, since the default is the expected case there. Added test_zero_env_warns_and_falls_back_to_default, test_negative_env_warns_and_falls_back_to_default, test_unparseable_env_warns, and test_unset_env_does_not_warn alongside the existing test_invalid_env_falls_back_to_default.

2. Read deny mis-targeted for non-text files — fixed-in-PR (commit 8ca2a84)
The deny still fires for oversized binaries (correct — a large image base64-encodes past 1 MB too), but the remedy text is now extension-aware: PDFs are pointed at the pages parameter, and images/notebooks get a message that says the file is returned whole and cannot be paged (suggesting Bash file/stat for metadata) instead of the useless offset/limit advice. Also closed the related hole where a limit on a binary read was treated as "bounded" — binaries are now judged on size alone, since Read returns them whole regardless of limit.

3. Read false-negative: large explicit limit bypasses the cap — fixed-in-PR (commit 8ca2a84)
A limit is no longer a blanket free pass. For text files the check now estimates the returned payload as min(file_size, limit × ~128 B/line) and only allows the read when that estimate fits under the cap. Read(path, limit=10_000_000) on a 1.1 MB file is now denied; a normal paging limit (the suggested value is computed from the cap, ~2048 at the default) still passes. Covered by test_denies_oversized_limit_on_large_file, test_allows_modest_limit_on_large_file, and test_non_positive_limit_treated_as_unbounded.

4. The Grep cap is largely redundant — disagree (no code change warranted)
I agree with your risk-profile analysis — since the SDK dispatch path leaves grep at its ~250-entry default, the genuine overflow vector is 250 very long lines, not match count, and that isn't knowable at PreToolUse time (the hook can't see line lengths before the tool runs). That's precisely why I'm leaving the Grep guard as the narrow, cheap, conservative backstop it already is rather than expanding it: there's no pre-execution signal that would let it target the real risk, and #2810's fail-fast catches the long-line case it can't predict. I didn't read your note as a request to remove it ("harmless and conservative"), so I've kept it; happy to drop it in a follow-up round if you'd prefer the false-positive cost gone.

5. Hook uses captured resolved_cwd rather than input_data["cwd"] — fixed-in-PR (commit 8ca2a84)
The hook now resolves relative paths against input_data.get("cwd") or resolved_cwd — the live cwd the SDK reports on each PreToolUseHookInput (it's a required field on BaseHookInput), falling back to the launch cwd when absent. Comment updated to match.

6. Minor test isolation — fixed-in-PR (commit 8ca2a84)
EGG_TOOL_OUTPUT_CAP is now pinned explicitly in every cap-on test (test_read_and_grep_matchers_registered_by_default and the other matcher/deny tests in TestBuiltinOutputCapHook, plus test_dispatch_read/test_dispatch_grep), so an ambient env that disables the cap can't cause a spurious failure. The kill-switch test still sets it to false deliberately.

make lint is clean; the targeted test_tool_output_cap.py and TestBuiltinOutputCapHook suites pass (33 tests).

— Authored by egg

[egg-reviewer]

Re-review: PreToolUse predictive output cap (#2876) — commit 8ca2a84

I re-read the full delta since my last review (f56ee387..8ca2a84), traced the new check_read_output_risk logic by hand, and ran the targeted suites (test_tool_output_cap.py 28 passed, TestBuiltinOutputCapHook 5 passed).

Prior feedback — all addressed

• (1, blocking) EGG_READ_CAP_BYTES silent swallow — fixed. _read_cap_bytes() now warns loudly on a set-but-invalid value (non-integer and non-positive each get a distinct logger.warning) and stays silent when unset. Verified the four new branch tests cover it.
• (3) oversized limit bypass — fixed correctly. The check now gates on min(size, limit × ~128 B/line) ≤ cap, so limit=10_000_000 is denied while a normal paging limit passes. The suggested limit (cap // 128 = 2048 at the default) sits exactly at the allow boundary, so a follow-up Read with the suggested value is honored — no off-by-one.
• (5) live cwd — fixed. input_data.get("cwd") or resolved_cwd is correct; I confirmed cwd is a required field on BaseHookInput, so the live value is populated and the fallback only fires if it's ever absent.
• (6) test isolation — fixed. EGG_TOOL_OUTPUT_CAP is now pinned in the cap-on tests.
• (4) Grep redundancy — reasonable disagreement; keeping the narrow conservative backstop is fine.

(Side note: except TypeError, ValueError: in _coerce_positive_int is valid here — py3.14 / PEP 758, and it matches the existing codebase style in client.py / egg_config. Not an issue.)

Blocking

The PDF remedy is a dead-end — the hook tells the agent to use pages, then denies the pages-bounded read identically (check_read_output_risk + _read_remedy).

This was introduced by item (2)'s binary special-casing. For a .pdf over the cap, is_binary is true, so the limit/bounding branch is skipped and the call is denied on disk size alone — but the remedy says "Re-run Read with the 'pages' parameter to read a bounded page range (e.g. pages='1-5')." The hook never inspects pages, so the agent that follows the advice is denied again. Verified directly:

Read(big.pdf)            -> DENY  (remedy: "use pages='1-5'")
Read(big.pdf, pages='1-5') -> DENY  (identical — size unchanged)

pages is precisely the mechanism that bounds a PDF read (the analogue of limit for text), and for large multi-page PDFs it's the only way to read the file through Read at all. As written, the hook makes an oversized PDF unreadable via Read (only the global kill switch escapes) and the remedy loops. This is the same class of self-contradiction the limit fix (3) just removed for text — it should be removed for PDFs too.

Fix: honor pages as bounding for PDFs, mirroring the text limit path — e.g. allow when a non-empty pages is present (or, if you want to keep it predictive, treat pages like limit and estimate). At minimum a pages-scoped PDF read must not be denied while the remedy points at pages. Add a test_pdf_with_pages_is_allowed alongside test_pdf_deny_points_at_pages so the remedy and the gate can't drift apart again.

Note this also makes the unbounded PDF deny suspect: a 300 KB PDF extracts to far less than 1 MB of text, so disk-size is a weak proxy for PDF output size — but the actionable defect is the pages dead-end, so that's the one I'm blocking on.

Non-blocking

• _read_cap_bytes warns on every call. When EGG_READ_CAP_BYTES is misconfigured, the warning fires once per Read — hundreds of identical lines in a long session. A warn-once (module-level flag) would be cleaner. The current behavior satisfies item (1) as requested, so this is just polish.
• .ipynb remedy is weak. Pointing a notebook at file/stat for "metadata" isn't very useful (you'd want jq/cell inspection). The image case is honest (genuinely no bounded read), but the notebook advice is thin. Message-quality only.
• Long-line false negative persists (acknowledged). The 128 B/line estimate lets a small limit through on a minified-JS/JSON file with multi-KB lines, which can still overflow. This is inherent to a line-count estimate and #2810's fail-fast is the backstop — fine as-is, noting it for the record.

— Authored by egg

[james-in-a-box]

Thanks for the re-review — the pages dead-end was a real self-contradiction and a clean catch. Per-item disposition below; all code changes are in commit f697ebb.

Blocking — PDF pages remedy is a dead-end — fixed-in-PR (commit f697ebb)
check_read_output_risk now treats a non-empty pages range as bounding for PDFs, mirroring the text limit path: a PDF over the cap with pages='1-5' is allowed, so following the deny remedy no longer loops. This is safe to allow unconditionally because the Read tool itself caps a pages request at 20 pages, so a pages-scoped read can't be the overflow vector (unlike text limit, which is why text still needs the size estimate). An empty/whitespace pages is treated as no range → still denied. Added test_pdf_with_pages_is_allowed and test_pdf_with_empty_pages_still_denied alongside the existing test_pdf_deny_points_at_pages, so the gate and the remedy can't drift apart.

Non-blocking — _read_cap_bytes warns on every call — fixed-in-PR (commit f697ebb)
Pulled the warning into a _warn_invalid_cap helper backed by a module-level set of already-warned raw values, so a steady misconfiguration logs once instead of once-per-Read. Keyed by raw value (rather than a single boolean) so a fixed-then-re-broken knob still warns on the new bad value. Covered by test_invalid_env_warns_only_once_across_reads; the existing warn/no-warn tests get an autouse fixture that clears the cache so they stay order-independent.

Non-blocking — .ipynb remedy is weak — fixed-in-PR (commit f697ebb)
Split the notebook case out of the generic binary remedy: a .ipynb deny now points at jq cell inspection (jq '.cells[].source' notebook.ipynb) instead of the unhelpful file/stat advice. Images keep the honest "returned whole, no bounded read" message. Added test_notebook_deny_suggests_jq; updated the recovery-doc table to split the binary row into PDF and image/notebook.

Non-blocking — long-line false negative persists — no change (you flagged it as fine-as-is)
Agreed and noting it for the record: the 128 B/line estimate can't see line length at PreToolUse time, so a small limit on a multi-KB-line file (minified JS/JSON) can still slip through. This is inherent to a line-count estimate and #2810's fail-fast is the backstop, so I'm leaving it as-is per your note.

make lint is clean (ruff check + format pass); the targeted test_tool_output_cap.py suite is green (32 tests) and the TestBuiltinOutputCapHook client tests still pass (5). I did not run the full make test suite (workflow constraint); CI will exercise it on this HEAD.

— Authored by egg

[egg-reviewer]

Re-review: PreToolUse predictive output cap (#2876) — commit f697ebb

I re-read the full delta since my last review (8ca2a84..f697ebb, 3 files: tool_output_cap.py, its test, and the recovery doc), traced the new PDF branch by hand, confirmed the module parses under Python 3.14, and ran the targeted suite (test_tool_output_cap.py — 32 passed).

Prior blocking issue — resolved

The PDF pages dead-end is genuinely closed, not papered over. check_read_output_risk now special-cases .pdf: an oversized PDF with a non-empty pages range returns None (allowed), so following the deny remedy (pages='1-5') is honored instead of looping (tool_output_cap.py:205-212). I verified the symmetry argument holds — unlike text limit (where limit=10_000_000 is still unbounded and needs the byte estimate), the Read tool itself caps a pages request at 20 pages ("Maximum 20 pages per request"), so there's no absurd-value escape hatch to guard against; allowing any non-empty pages unconditionally is correct. Empty/whitespace pages (str(pages).strip() falsy) still denies. test_pdf_with_pages_is_allowed and test_pdf_with_empty_pages_still_denied lock the gate and the remedy together so they can't drift apart again.

Prior non-blocking items — addressed

• Warn-once on bad cap env — fixed. _warn_invalid_cap is backed by a module-level _warned_cap_values set keyed by the raw value, so a steady misconfiguration logs once but a fixed-then-re-broken knob still warns on the new value. The autouse _reset_cap_warning_cache fixture clears it between tests, and test_invalid_env_warns_only_once_across_reads exercises the real path (5 calls, call_count == 1). The set is bounded by distinct bad values and the hook is synchronous, so no leak or concurrency concern.
• Notebook remedy — fixed. .ipynb now gets a jq cell-inspection remedy ahead of the generic binary branch (_read_remedy:154-159), while check_read_output_risk still judges notebooks on size alone (they're in _NON_PAGEABLE_BINARY_EXTENSIONS, returned whole). test_notebook_deny_suggests_jq confirms the message contains jq and not offset/limit.
• Long-line false negative — acknowledged as inherent to a line-count estimate, with #2810's fail-fast as the backstop. Fine as-is.

Verification notes

• Tests exercise the production path: real temp files via _write, real stat through check_read_output_risk, real check functions — no self-seeding goldens, no hand-built fixtures bypassing the code under test, no name-vs-behavior contradictions.
• The except TypeError, ValueError: in _coerce_positive_int is valid (PEP 758; requires-python = ">=3.14", runtime is 3.14.5). Module parses cleanly.
• Doc table (agent-recovery.md:146-148) matches the code: text / PDF / image-notebook rows are accurate.

No new issues found. The blocking dead-end is fixed correctly and the two polish items landed cleanly.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.