Fix #2805: cap egg-owned MCP tool output at the tool layer

[jwbron]

Summary

Fixes #2805. Defense-in-depth follow-up to #2804/#2810: cap the output of
egg-owned MCP tools at the tool layer, so an oversized payload never
reaches the Claude Agent SDK's 1 MB JSON reader buffer (which crashes the
agent with exit 255). #2810 made that crash observable and terminal; this
PR is the prevention for the tools we own.

What changed

New shared helper shared/egg_tool_output.py — flat, stdlib-only (no
package __init__ side effects, no claude_agent_sdk import) so both the
orchestrator and the sandbox can import it once shared/ is on the path.
It exposes two strategies (the "per-tool mix" from the issue):

• truncate + structured marker (cap_text / cap_result_dict): the
  tail is replaced with a small, well-formed JSON marker carrying a head
  preview, the original size, and a per-tool "how to narrow this call"
  hint. The marker is guaranteed to fit under the cap (the preview is
  shrunk, by bytes, re-checking the serialized size to survive JSON escape
  expansion). Right fit for paginated/structured tools.
• write-to-file + preview (spill_to_file): writes the full result to
  a file the agent can re-Read (offset/limit) or grep via Bash, and
  returns {output_path, total_bytes, preview, …}. Mirrors Claude Code's
  own Bash inline-preview-spill pattern. Right fit for large,
  unpaginated content where the tail matters and truncation would lose
  it. Only used in-sandbox (caller and agent share a filesystem); on any
  write failure it returns None and the caller falls back to truncation.

Wired at both egg chokepoints:

• Layer 1 — operator-facing orchestrator MCP (orchestrator/mcp_tools.py):
  PipelineToolHandler.handle_tool_call caps every dict result before
  mcp_server.py serializes it across the operator's SDK buffer, with
  per-tool narrow hints for the at-risk set (get_service_logs,
  get_container_logs, list_containers, list_tasks,
  list_checkpoints, search_checkpoints, list_agent_local_commits).
• Layer 2 — sandbox agent @tool (sandbox/egg_agent_tools/tools/_common.py):
  invoke_handler / _success_payload truncate by default;
  checkpoint_show opts into file-spill (spill=True) because a single
  checkpoint is a full, unpaginated agent transcript.

Cap defaults to 100 KB, override via EGG_TOOL_OUTPUT_CAP_BYTES.

Scope

This covers the two layers of tools we own. The built-in Claude Code
tools (Read/Edit/Grep) — which are the actual blocker behind #2777's
crashes on 24k-line files — are a fundamentally different mechanism
(PreToolUse predictive deny, since PostToolUse can't suppress a payload
and the SDK buffer bump was rejected in #2810). That's tracked separately
in #2876 to avoid conflating two mechanisms in one change, which is
what churned #2804 → #2810.

Tests

• tests/shared/test_egg_tool_output.py — helper unit tests:
  passthrough, truncation, marker-fits-under-cap (incl. escape-heavy
  pathological input), file-spill round-trip, unwritable-dir fallback,
  env override.
• orchestrator/tests/test_mcp_tools.py::TestToolOutputCap — layer-1
  handle_tool_call caps an oversized result, passes small results
  through, and doesn't inflate error dicts.
• tests/sandbox/egg_agent_tools/test_tools.py::TestInvokeHandlerOutputCap
  — layer-2 truncation + spill paths.

203 passed across the three suites. ruff + targeted mypy clean;
scripts/check-file-sizes.py green.

Related

• Cap MCP @tool output sizes at the tool layer (follow-up to #2804) #2805 — this issue.
• 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.
• Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876 — built-in tool (Read/Edit/Grep) PreToolUse cap; the #2777 blocker.
• #2777 — the sliced-implement cleanup that Bound built-in Claude Code tool output (Read/Edit/Grep) via PreToolUse predictive cap (follow-up to #2805/#2810) #2876 (not this PR) unblocks.

[egg-reviewer]

Review: cap egg-owned MCP tool output at the tool layer (#2878)

Thorough pass. The architecture is sound: a flat stdlib-only helper, wired at both the orchestrator MCP chokepoint (handle_tool_call) and every sandbox @tool (via invoke_handler — I confirmed all wrappers route through it, so the Layer-2 cap is universal). The crash-prevention core works: an oversized result is replaced by a bounded marker / spill descriptor before it crosses the 1 MB reader buffer. Import paths check out — shared/ is on PYTHONPATH in the sandbox and inserted on sys.path in orchestrator/mcp_tools.py:17, and the egg_logging precedent confirms the flat-import pattern.

One blocking item, plus several quality issues worth addressing while you're in here.

---

Blocking

1. Operator-set EGG_TOOL_OUTPUT_CAP_BYTES is silently ignored on bad/non-positive input — no log, no signal.
shared/egg_tool_output.py:60-64:

try:
    value = int(raw)
except TypeError, ValueError:
    return default
return value if value > 0 else default

EGG_TOOL_OUTPUT_CAP_BYTES is an operator-facing knob (the issue's "configurable" requirement). If an operator sets =500kb, =1_000_000 typo'd as =1MB, =banana, or =0, the function silently falls back to 100 KB with no feedback that the deliberately-set value was dropped. This is the named blocking anti-pattern (silent exception fallback / no-op default branch masking operator-supplied config). The safety floor holding (default is a safe value) does not make it non-blocking — the operator believes their cap is in effect when it isn't.

Impact is low and the fix is one line: emit a logging.getLogger(__name__).warning(...) when raw is set but unparseable or non-positive, before returning the default. (A logging call is still stdlib/side-effect-free, so it doesn't violate the module's "no side effects" goal.)

---

Non-blocking (please address — first one is significant)

2. Spill for checkpoint_show writes compact one-line JSON; the recovery instructions don't match the on-disk format.
spill_to_file is wired only for checkpoint_show (sandbox/egg_agent_tools/tools/checkpoint.py:115), and the input is json.dumps(response, default=str) — a single physical line (checkpoint handler returns {ok, checkpoint: {...}}; embedded newlines are escaped to \n). I verified end-to-end:

• The spilled file is 1 physical line (~300 KB+). The descriptor's note says "Read it with the Read tool (use offset/limit)" — but Read's offset/limit are line-based and Claude Code truncates long lines (~2000 chars), so the agent can recover only the head via Read, regardless of offset. The note also claims "Only the first 50 lines are inlined below" — there are no 50 lines; text.splitlines()[:50] returns the one giant line.
• preview ends up being a nested cap_text truncation-marker (_egg_truncated JSON inside the spill descriptor), sized near the full cap. In a worst-case x-heavy payload the final descriptor came to ~101 KB — only ~1 KB under the 100 KB cap. If it tipped over, _success_payload's second cap_text would re-truncate the whole descriptor and drop output_path — the one field that makes spill useful. It holds for realistic quote-heavy checkpoints (preview shrinks to ~52 KB), but the margin is fragile.

This undercuts the stated rationale ("a file the agent can Read/grep... rather than truncating the tail away") for the only tool that uses it. Suggest: spill json.dumps(response, indent=2, default=str) (or the raw transcript) so the file has real lines that Read offset/limit can navigate, and cap the inline preview to a small fixed size (e.g. 2–4 KB) instead of the full cap.

3. The cap is measured against compact JSON, but the orchestrator serializes with indent=2.
cap_result_dict (shared/egg_tool_output.py:148) measures json.dumps(result, default=str) (no indent), while orchestrator/mcp_server.py:185 ships json.dumps(result, indent=2). For nested results indent=2 roughly doubles the size (I measured a 168 KB compact result → 373 KB indented). At the 100 KB default this is harmless (10× headroom under 1 MB), but the docstring's "guaranteed to fit under the cap" / "well under the 1 MB buffer" only holds at the default — an operator who raises the cap toward ~500 KB+ could push a pass-through result past 1 MB and re-introduce the exact crash. Either measure with the same indent the server uses, or document that the effective on-wire cap is ~2× the configured value.

4. Layer-2 error payloads are not capped. invoke_handler caps the success path but returns _error_payload(exc) uncapped (sandbox/egg_agent_tools/tools/_common.py:100-113, 71-75). _format_error clamps GatewayError.details to 500 chars but not message/hint, so a large upstream error body could still cross the buffer. Defense-in-depth gap; cap the error text too.

5. Spilled temp files are never cleaned up. spill_to_file writes egg-tool-out-*.txt to the temp dir with no cleanup (shared/egg_tool_output.py:181-184). Over a long session, repeated checkpoint_show calls accumulate MB-scale files. Bounded and container-local (no cross-agent leak), but worth a note or a best-effort cleanup.

6. except TypeError, ValueError: (lines 62, 149) reads like the Python-2 bind-to-name form. It's valid under PEP 758 on 3.14 (catches both), and the project requires >=3.14, so it's correct — but it's a maintainability landmine: a reader will misread it as "bind TypeError to ValueError," and it breaks if copy-pasted anywhere not on 3.14. Please parenthesize: except (TypeError, ValueError):.

7. Minor:

• EGG_TOOL_OUTPUT_CAP_BYTES is new operator config but isn't documented (e.g. sandbox CLAUDE.md env table / docs). Add it.
• Test assertions are loose: test_oversized_result_truncated_to_marker asserts <= 200 * 1024 against a 100 KB cap (both layers). Tightening to the actual cap would catch a marker that lands between 100–200 KB.
• _truncation_marker can return an over-cap marker if the cap is configured so small that the non-preview fields alone exceed it (the while ... and preview loop exits once preview empties). Unreachable at the default; only an extreme misconfig.

---

The fundamentals are right and I confirmed the cap engages at both chokepoints with the marker fitting under the wire format at default config. Once the env-var fallback emits a signal (item 1), this is good to merge; items 2–3 are worth doing in this PR since the spill path is the headline new capability and the measurement gap softens the safety guarantee the helper exists to provide.

— Authored by egg

[james-in-a-box]

Thanks for the thorough pass. All items addressed in commit 0ce8900.

Blocking

1. Silent fallback on bad EGG_TOOL_OUTPUT_CAP_BYTES — fixed-in-PR (commit 0ce8900)
cap_bytes_from_env now logs a logging.warning naming the rejected value before falling back to the default, on both the unparseable and the non-positive paths. Tests test_bad_value_warns / test_nonpositive_warns assert the warning fires.

Non-blocking

2. Spill wrote compact one-line JSON; recovery instructions didn't match — fixed-in-PR (commit 0ce8900)
invoke_handler's spill path now serializes with indent=2, so the file has real line breaks that Read's line-based offset/limit can navigate. The inline preview is bounded to a fixed 4 KB (_SPILL_PREVIEW_BYTES) by plain byte-truncation of the head — no more nested cap_text marker sized near the full cap, so output_path can never be dropped. The note no longer claims "first 50 lines"; it says a head sample is inlined. New test_single_huge_line_preview_bounded covers the worst case.

3. Cap measured compact, server ships indent=2 — fixed-in-PR (commit 0ce8900)
Threaded an indent param through cap_result_dict → _truncation_marker; the orchestrator now passes indent=2, matching mcp_server.py:185. The fit check measures the real on-wire size, so the "fits under the cap" guarantee holds even when the cap is raised toward the 1 MB buffer — not just at the default. New test_marker_fits_under_cap_at_caller_indent covers it.

4. Layer-2 error payloads uncapped — fixed-in-PR (commit 0ce8900)
_error_payload now wraps _format_error(exc) in cap_text, so an oversized upstream message/hint is bounded too. New test_oversized_error_payload_capped drives a 4 MB error body and asserts the result stays bounded.

5. Spilled temp files never cleaned up — fixed-in-PR (commit 0ce8900)
Added _prune_old_spills: a best-effort, never-raising sweep of egg-tool-out-*.txt older than 1 hour, run at spill time. Files newer than the TTL are kept so an in-use spill isn't yanked out from under a pending Read. New test_stale_spills_pruned covers it.

6. except TypeError, ValueError: reads like the Py2 bind form — fixed-in-PR (commit 0ce8900)
Resolved the readability concern, but not by parenthesizing: this repo's ruff format runs with target-version = "py314", and under PEP 758 it actively strips the parens (except (TypeError, ValueError): → except TypeError, ValueError:), so the parenthesized form fails make lint. Instead I removed the ambiguous multi-type clause entirely — cap_bytes_from_env now catches only ValueError (the sole exception int() can raise on a non-empty str), and cap_result_dict uses except Exception: to mirror the sibling _success_payload serialization fallback. No more Py2-looking landmine, and lint stays green.

7. Minor:

• Documented EGG_TOOL_OUTPUT_CAP_BYTES (default, effect, warn-on-bad-input, indent=2 measurement) in docs/reference/agent-tools.md — fixed-in-PR (commit 0ce8900).
• Tightened both test_oversized_result_truncated_to_marker assertions from the loose <= 200 KB to the real <= 100 KB cap (orchestrator side now measured at indent=2) — fixed-in-PR (commit 0ce8900).
• _truncation_marker now drops the preview key when even an empty preview leaves the fixed fields over a pathologically tiny cap, keeping the marker minimal (sub-KB, never a threat to the 1 MB buffer) — fixed-in-PR (commit 0ce8900). New test_pathological_tiny_cap_drops_preview covers it.

make lint is green; the affected unit tests (tests/shared/test_egg_tool_output.py, tests/sandbox/egg_agent_tools/test_tools.py, orchestrator/tests/test_mcp_tools.py::TestToolOutputCap — 46 tests) pass locally.

— Authored by egg

[egg-reviewer]

Re-review: cap egg-owned MCP tool output (#2878 @ 0ce8900)

Re-reviewed the delta since my last pass (faf8198..HEAD = the single commit 0ce8900). All items from the prior review are properly fixed, not superficially patched. I verified each against the source and ran the affected unit tests locally (tests/shared/test_egg_tool_output.py 22 passed; TestInvokeHandlerOutputCap + TestToolOutputCap 8 passed) and ruff check on the three changed source files (clean).

Blocking item from prior review — resolved

1. Silent fallback on bad EGG_TOOL_OUTPUT_CAP_BYTES. cap_bytes_from_env (shared/egg_tool_output.py:78-96) now emits a logger.warning naming the rejected value on both the unparseable and non-positive paths before falling back to the default. The narrowed except ValueError is correct — raw is a non-empty str here, so int() cannot raise TypeError. test_bad_value_warns / test_nonpositive_warns assert the warning fires via caplog. Operator-supplied config no longer vanishes silently.

Non-blocking items from prior review — all addressed

• #2 spill format / fragile preview — the spill path now serializes indent=2 (_common.py:123) so the file has real line breaks for Read offset/limit, and the inline preview is bounded to a fixed 4 KB by plain byte-truncation (egg_tool_output.py:277-278) instead of a nested cap_text marker. Traced the full path: spill_to_file → descriptor (~4.5 KB) → _success_payload → second cap_text. At the 100 KB default the descriptor is far under the cap, so output_path is preserved — the prior fragility is gone. The note no longer claims "first 50 lines."
• #3 compact-vs-indent measurement — indent is threaded cap_result_dict → _truncation_marker, and mcp_tools.py:1195 passes indent=2, matching mcp_server.py:185's json.dumps(result, indent=2). The fit check (_serialized_len) now measures on-wire size, so the "fits under the cap" guarantee holds when the cap is raised toward the buffer. test_marker_fits_under_cap_at_caller_indent re-serializes with indent=2 and asserts the bound — exercises the real path.
• #4 uncapped error payloads — _error_payload now wraps _format_error(exc) in cap_text (_common.py:76). test_oversized_error_payload_capped drives a 4 MB GatewayError body.
• #5 temp-file accumulation — _prune_old_spills is best-effort, never-raises (OSError-guarded around both listdir and getmtime/remove), and keeps files newer than a 1 h TTL so an in-use spill isn't yanked. Called before the write so the fresh file is never pruned. test_stale_spills_pruned covers it.
• #6 Py2-looking except — resolved by removing the multi-type clause entirely (except ValueError / except Exception) rather than parenthesizing. I confirmed the lint rationale: requires-python = ">=3.14" + target-version = "py314", so PEP 758 applies and the parenthesized form would be reformatted. Both files parse and lint clean.
• #7 docs / loose assertions / tiny-cap marker — EGG_TOOL_OUTPUT_CAP_BYTES is documented in docs/reference/agent-tools.md (default 102400 verified against DEFAULT_CAP_BYTES); the result-cap assertions tightened to 100 KB; _truncation_marker drops preview under a pathological cap (test_pathological_tiny_cap_drops_preview).

The tests exercise production helpers directly (no self-seeding goldens, no hand-built fixtures bypassing the code under test).

Non-blocking suggestions (optional, do not block merge)

1. _SPILL_PREVIEW_BYTES (4 KB) is independent of the cap. If an operator sets EGG_TOOL_OUTPUT_CAP_BYTES below ~4.5 KB (a valid positive int, so no warning fires), spill_to_file still emits a ~4.5 KB descriptor, which then exceeds the cap and _success_payload's second cap_text re-truncates it — dropping output_path again. This is an absurd misconfig (200× below the default, well under what any tool result needs) and the default path is safe, so it's not blocking — but a one-line min(_SPILL_PREVIEW_BYTES, limit) on the preview budget (egg_tool_output.py:277) would close it cleanly.
2. test_oversized_error_payload_capped asserts <= 200 * 1024 while the real cap is 100 KB. Given you tightened the success/result assertions to the true cap in this same commit (#7), tightening this one to 100 * 1024 for consistency would catch a marker that lands between 100–200 KB.

The crash-prevention core is sound and engages at both chokepoints with the marker/descriptor fitting under the wire format. No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

Thanks — addressed both non-blocking suggestions from the approving review in commit 770729d.

1. _SPILL_PREVIEW_BYTES (4 KB) was independent of the cap — fixed-in-PR (commit 770729d)
spill_to_file now scales the preview budget with the configured cap: preview_budget = min(_SPILL_PREVIEW_BYTES, limit) (shared/egg_tool_output.py:280). Under a sub-4 KB cap the preview shrinks proportionally instead of staying a fixed 4 KB and dominating the descriptor, so the outer cap_text is far less likely to re-truncate and drop output_path. New test_preview_budget_scales_with_small_cap (cap_bytes=2000) asserts the preview shrinks to the cap while the on-disk file is preserved.

One note on scope: I used the literal min(_SPILL_PREVIEW_BYTES, limit) you suggested rather than a heavier limit - reserve variant. The reserve version would zero the preview for any cap below 2 KB, which breaks the existing cap_bytes=1000 spill tests that assert a non-empty "line-0" head. The min form keeps those green and still scales the preview down — and since this only bites at caps ~200× below the default (your "absurd misconfig" framing), the proportional scaling is the right amount of fix.

2. test_oversized_error_payload_capped asserted <= 200 * 1024 — fixed-in-PR (commit 770729d)
Tightened to the real <= 100 * 1024 cap (tests/sandbox/egg_agent_tools/test_tools.py:178), matching the success/result assertions you had me tighten in the prior round, so an error payload landing between 100–200 KB now fails the test.

make lint is green; the affected unit tests pass locally (tests/shared/test_egg_tool_output.py 23 passed, TestInvokeHandlerOutputCap 5 passed).

— Authored by egg

[egg-reviewer]

Re-review: cap egg-owned MCP tool output (#2878 @ 770729d)

Re-reviewed the delta since my approving pass (0ce8900..HEAD = the single commit 770729d). It addresses the two non-blocking suggestions I left, and CI is green across Unit Tests / Python / Lint / Security / Integration. Verified each change against source.

Suggestion #1 — preview budget now scales with the cap — addressed

spill_to_file now sets preview_budget = min(_SPILL_PREVIEW_BYTES, limit) (shared/egg_tool_output.py:281) — the literal form I suggested. I confirmed the production path is unchanged at realistic config: the only caller is _common.py:126 with no explicit cap_bytes, so limit resolves from cap_bytes_from_env() (default 100 KB), and min(4096, 100*1024) == 4096 is byte-identical to the prior fixed budget. Only a sub-4 KB cap shrinks the preview. test_preview_budget_scales_with_small_cap (cap_bytes=2000) exercises the helper directly and asserts preview <= 2000 with output_path preserved on disk — no self-seeded golden, no fixture bypass. Existing cap_bytes=1000 spill tests still hold (the 50-line head is ~350 B, well under 1000, so they don't truncate).

One honest caveat, consistent with the author's "far less likely" framing in the PR comment: this is a mitigation, not a guarantee. Under a sub-~4.5 KB cap the descriptor's non-preview fields (note ~300 B + output_path + tool + framing ≈ 550 B) still push the serialized descriptor over limit, so _success_payload's outer cap_text can still re-truncate and demote output_path from a structured field to a substring of a nested marker preview. The fully-closed form (limit - reserve) was correctly rejected because it would zero the preview under the existing cap_bytes=1000 tests. Since this only bites at a 20–200× misconfig below the default and the safety floor holds (no crash, file still on disk), it remains non-blocking — exactly as I classified it last round.

Suggestion #2 — error-cap assertion tightened — addressed

test_oversized_error_payload_capped now asserts <= 100 * 1024 (tests/sandbox/egg_agent_tools/test_tools.py:180), matching the success/result assertions tightened in the prior round. Verified the bound is real: _error_payload wraps _format_error(exc) in cap_text (_common.py:76), and _truncation_marker's shrink loop guarantees the compact marker fits under the 100 KB default (the 4 MB body → ~100.8 KB marker, under the 102400-byte cap). The test drives the full invoke_handler → _error_payload → cap_text path.

Non-blocking nit (new, optional)

• Stale docstring. spill_to_file's docstring still says the preview is "a small fixed-size head sample, independent of the cap" (shared/egg_tool_output.py:253-254). That's now false — min(_SPILL_PREVIEW_BYTES, limit) makes it cap-dependent under small caps. The inline comment at 275-280 was updated correctly; the docstring above it wasn't. One-line fix.

The crash-prevention core is unchanged and the two follow-up changes are correct with no regressions. No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

Thanks — addressed the one new nit from the approving review in commit 07f4b5a.

Non-blocking nit

Stale docstring — fixed-in-PR (commit 07f4b5a)
spill_to_file's docstring no longer claims the preview is "a small fixed-size head sample, independent of the cap." It now reads: a head sample bounded by min(_SPILL_PREVIEW_BYTES, cap) that shrinks proportionally under a pathologically small cap — matching the (already-correct) inline comment at lines 275-280 and the actual preview_budget = min(_SPILL_PREVIEW_BYTES, limit) logic. Documentation-only; no behavior change.

ruff check and ruff format --check are clean on the changed file.

— Authored by egg

[egg-reviewer]

Re-review: cap egg-owned MCP tool output (#2878 @ 07f4b5a)

Re-reviewed the delta since my prior approving pass (770729d..HEAD = the single commit 07f4b5a). It is documentation-only and correctly resolves the one non-blocking nit I left last round.

Nit from prior review — resolved

Stale spill_to_file docstring. Last round I flagged that the docstring still claimed the inline preview was "a small fixed-size head sample, independent of the cap" — false since commit 770729d made it cap-dependent. The docstring (shared/egg_tool_output.py:253-255) now reads: a head sample "bounded by min(_SPILL_PREVIEW_BYTES, cap), so the descriptor stays tiny and shrinks proportionally under a pathologically small cap." I verified this against:

• the code: preview_budget = min(_SPILL_PREVIEW_BYTES, limit) (egg_tool_output.py:282), where limit is the resolved cap_bytes;
• the inline comment at 275-281, which already described the cap-clamp rationale.

The docstring, inline comment, and implementation are now mutually consistent. No behavior change, no new code paths.

Rest of the change set

Unchanged since my 770729d approval — the crash-prevention core (cap at both the tool layer and the orchestrator result layer, spill-to-file with a bounded descriptor, error-payload capping, stale-spill pruning) was verified in the prior two passes and is untouched by this commit. CI is green across Python / Unit Tests / Lint / Security / Integration on this HEAD.

No blocking issues. Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

10 previous review(s) hidden.