diff --git a/docs/reference/agent-recovery.md b/docs/reference/agent-recovery.md index 109b60cb08..6cfec60d0d 100644 --- a/docs/reference/agent-recovery.md +++ b/docs/reference/agent-recovery.md @@ -133,6 +133,23 @@ The agent output is captured by piping stdout and stderr through `tee` into a te > **Note:** The buffer-overflow marker string is synchronized between the wrapper's `grep` and the `_BUFFER_OVERFLOW_MARKER` constant in `shared/egg_agent/client.py`. If a future `claude-agent-sdk` release changes the wording, the wrapper silently falls back to burning the transient-crash retry budget. See [#2823](https://github.com/jwbron/egg/issues/2823) for the follow-up to pin this against the installed SDK. The real fix is tool-layer truncation of oversized payloads ([#2805](https://github.com/jwbron/egg/issues/2805)); this is the fail-fast path until that lands. +### Predictive Output Cap (PreToolUse) + +Source: `shared/egg_agent/tool_output_cap.py`, wired in `shared/egg_agent/client.py`. + +The fail-fast above is a backstop, not prevention. egg caps its own MCP `@tool` payloads at the tool boundary ([#2805](https://github.com/jwbron/egg/issues/2805)), but **built-in** Claude Code tools (`Read`, `Grep`, `Edit`, `Bash`) run inside the CLI and can't be wrapped. [#2876](https://github.com/jwbron/egg/issues/2876) bounds those via a **PreToolUse** hook that fires *before* the tool runs and denies calls whose result is likely to overflow — for example a whole-file `Read` of the 1.1 MB, 24k-line `orchestrator/routes/pipelines.py` that crashed the [#2777](https://github.com/jwbron/egg/issues/2777) slice-1 coder. PostToolUse can't help here: the payload has already crossed the channel that crashes the reader ([#2810](https://github.com/jwbron/egg/issues/2810) dropped that approach). + +Current heuristics — predictive, so expect some false positives/negatives, with the fail-fast as the backstop when a prediction misses: + +| Tool | Denied when | Deny reason points at | +|------|-------------|------------------------| +| `Read` (text) | Target file > `EGG_READ_CAP_BYTES` (default 256 KiB) **and** the read is unbounded — no `limit`, or a `limit` whose estimated payload (`limit` × ~128 B/line) still exceeds the cap | `offset` / `limit` to page through the file (with a suggested `limit` that fits the cap) | +| `Read` (PDF) | Target PDF > `EGG_READ_CAP_BYTES` **and** no non-empty `pages` range — a `pages`-scoped read is bounded (the Read tool caps it at 20 pages), mirroring `limit` for text | `pages` to read a bounded page range (e.g. `pages='1-5'`) | +| `Read` (image/notebook) | Target image/notebook > `EGG_READ_CAP_BYTES` (returned whole; `offset`/`limit`/`pages` don't bound it) | images: avoid reading whole, use Bash (`file`/`stat`) for metadata; notebooks: inspect cells with `jq` (e.g. `jq '.cells[].source'`) | +| `Grep` | `output_mode=content`, no `head_limit`, **and** no `path`/`glob` scope (whole-repo content dump) | `head_limit`, a `path`/`glob` scope, or `output_mode=files_with_matches` | + +The hook is **always-on** (the overflow hits every route, including first-party Opus). Set `EGG_TOOL_OUTPUT_CAP=false` (or `0`/`no`/`off`) to disable; set `EGG_READ_CAP_BYTES` to tune the `Read` threshold (a set-but-invalid value — non-integer or non-positive — is logged and ignored in favour of the default). + ### Transient Exit Codes The `is_transient_crash()` function classifies these exit codes as transient: diff --git a/shared/egg_agent/client.py b/shared/egg_agent/client.py index ebf290b648..a2cf53f84e 100644 --- a/shared/egg_agent/client.py +++ b/shared/egg_agent/client.py @@ -275,6 +275,58 @@ async def _check_tool_permission( error=str(e), ) + # --- Predictive output cap for built-in CC tools (#2876) --- + # Built-in tools (Read/Grep/...) run inside the CLI; egg can't wrap their + # output the way it caps its own MCP @tool payloads (#2805). A result above + # the Agent SDK's 1 MB JSON buffer kills the agent with exit 255 (#2804) — + # the case that crashed the #2777 slice-1 coder on the 1.1 MB, 24k-line + # orchestrator/routes/pipelines.py. A PreToolUse hook can't see the result, + # but it can predict the overflow from the inputs and deny *before* the tool + # runs, telling the agent how to narrow the call. #2810's fail-fast is the + # backstop when a prediction misses. Always-on (the overflow hits every + # route, including first-party Opus); disable via EGG_TOOL_OUTPUT_CAP=false. + from egg_agent.tool_output_cap import ( + check_builtin_tool_output_risk, + is_output_cap_disabled, + ) + + if not is_output_cap_disabled(): + # Resolves Read's relative file_paths the same way the tool will: prefer + # the live cwd the SDK reports on each PreToolUse event, falling back to + # the launch cwd if absent. Returns {} (no decision) when the call is + # within bounds, so allowed calls fall through to the normal flow. + async def _cap_builtin_tool_output( + input_data: HookInput, tool_use_id: str | None, context: HookContext + ) -> HookJSONOutput: + reason = check_builtin_tool_output_risk( + input_data.get("tool_name", ""), + input_data.get("tool_input", {}) or {}, + input_data.get("cwd") or resolved_cwd, + ) + if reason is None: + return {} + logger.info( + "Predictive output cap denied built-in tool call", + event_type="tool_intercepted", + event_subtype="output_cap_deny", + tool_name=input_data.get("tool_name"), + tool_use_id=tool_use_id, + reason=reason, + ) + return { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + } + } + + existing_hooks = getattr(options, "hooks", None) or {} + pre_tool_use = list(existing_hooks.get("PreToolUse", [])) + pre_tool_use.append(HookMatcher(matcher="Read", hooks=[_cap_builtin_tool_output])) + pre_tool_use.append(HookMatcher(matcher="Grep", hooks=[_cap_builtin_tool_output])) + options.hooks = {**existing_hooks, "PreToolUse": pre_tool_use} + # --- DuckDuckGo MCP fallback for the LiteLLM→non-Anthropic path (#2856) --- # On that path (signalled by ANTHROPIC_CUSTOM_MODEL_OPTION) the built-in # WebSearch/WebFetch tools silently no-op: LiteLLM's drop_params strips the diff --git a/shared/egg_agent/tool_output_cap.py b/shared/egg_agent/tool_output_cap.py new file mode 100644 index 0000000000..aa2b3d5514 --- /dev/null +++ b/shared/egg_agent/tool_output_cap.py @@ -0,0 +1,270 @@ +"""Predictive PreToolUse caps for built-in Claude Code tools (issue #2876). + +Built-in tools (``Read``, ``Grep``, ``Edit``, ``Write``, ``Bash``) run +inside the Claude Code CLI; egg cannot wrap their output the way it caps +its own MCP ``@tool`` payloads (#2805). A tool result that exceeds the +Agent SDK's 1 MB JSON message buffer kills the agent with exit 255 +(#2804); #2810 made that a clean fail-fast but does **not** prevent it. + +This module supplies *predictive* heuristics for a PreToolUse hook: the +hook fires **before** the tool runs and denies calls whose result is +likely to overflow, returning a reason that tells the agent exactly how +to narrow the call (``offset``/``limit``/``head_limit``/ +``files_with_matches``). Because the hook fires before execution it +cannot see the result, so the heuristics are necessarily approximate +(false positives/negatives are expected); #2810's fail-fast remains the +backstop when a prediction misses. + +The load-bearing case is ``Read`` of a very large source file — e.g. the +24k-line ``orchestrator/routes/pipelines.py`` (~1.1 MB) that crashed the +#2777 slice-1 coder. Reading it whole produces a tool result larger than +the 1 MB buffer; redirecting the agent to ``offset``/``limit`` keeps each +page bounded. +""" + +from __future__ import annotations + +import logging +import os +from pathlib import Path +from typing import Any + +try: + from egg_logging import get_logger + + logger: Any = get_logger("egg-agent") +except ImportError: # pragma: no cover - egg_logging always present in-sandbox + logger = logging.getLogger(__name__) + +# 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 + +# Rough average bytes per source line, used to estimate how many bytes a +# *bounded* Read (``limit`` lines) will return. Deliberately conservative +# (real source averages well under this) so a normal paging limit like +# ``limit=2000`` stays under the 256 KiB default while an absurd +# ``limit=10_000_000`` is still recognised as unbounded. +_EST_BYTES_PER_LINE = 128 + +# Binary file types ``Read`` returns whole, where line-based ``offset`` / +# ``limit`` paging does not apply. PDFs are the exception — they page via +# ``pages`` (the Read tool caps a request at 20 pages), so a ``pages``-scoped +# PDF read is bounded and allowed. Notebooks are JSON but Read returns every +# cell whole, so they get a ``jq``-oriented remedy rather than the generic one. +_PDF_EXTENSION = ".pdf" +_NOTEBOOK_EXTENSION = ".ipynb" +_NON_PAGEABLE_BINARY_EXTENSIONS = frozenset( + {".png", ".jpg", ".jpeg", ".gif", ".bmp", ".webp", ".tiff", ".tif", ".ico", _NOTEBOOK_EXTENSION} +) + +# Raw EGG_READ_CAP_BYTES values we've already warned about, so a misconfigured +# knob logs once per distinct value rather than on every Read in the session. +_warned_cap_values: set[str] = set() + + +def is_output_cap_disabled() -> bool: + """True when the predictive cap is switched off via env. + + Mirrors the EGG_MCP_TOOLS kill-switch convention so operators can + disable the heuristic without a code change if it proves too noisy. + """ + return os.environ.get("EGG_TOOL_OUTPUT_CAP", "").strip().lower() in ( + "false", + "0", + "no", + "off", + ) + + +def _read_cap_bytes() -> int: + """Resolve the Read byte cap from env, falling back to the default. + + ``EGG_READ_CAP_BYTES`` is an operator tuning knob, so a *set-but-invalid* + value is logged loudly before falling back — an operator who typoed the + value or used an unsupported suffix (``2mb``, ``0``, a negative) would + otherwise silently get the default and a different false-positive rate + than they intended. The unset case stays silent (the default is expected). + """ + raw = os.environ.get("EGG_READ_CAP_BYTES", "").strip() + if not raw: + return _DEFAULT_READ_CAP_BYTES + try: + value = int(raw) + except ValueError: + _warn_invalid_cap(raw, "is not an integer") + return _DEFAULT_READ_CAP_BYTES + if value <= 0: + _warn_invalid_cap(raw, "must be a positive integer") + return _DEFAULT_READ_CAP_BYTES + return value + + +def _warn_invalid_cap(raw: str, problem: str) -> None: + """Warn that an invalid EGG_READ_CAP_BYTES is being ignored, once per value. + + The cap is resolved on every ``Read``, so warning unconditionally would emit + hundreds of identical lines for one misconfiguration. Track the raw values + already warned about so a fixed-then-re-broken knob still warns on the new + bad value, but a steady bad value warns only once. + """ + if raw in _warned_cap_values: + return + _warned_cap_values.add(raw) + logger.warning( + f"EGG_READ_CAP_BYTES={raw!r} {problem}; ignoring it and using the " + f"default {_DEFAULT_READ_CAP_BYTES} bytes" + ) + + +def _resolve_path(file_path: str, cwd: str | None) -> Path: + """Resolve a possibly-relative tool ``file_path`` against the agent cwd.""" + path = Path(file_path) + if not path.is_absolute() and cwd: + path = Path(cwd) / path + return path + + +def _coerce_positive_int(value: Any) -> int | None: + """Return ``value`` as a positive int, or None if it isn't one.""" + try: + n = int(value) + except TypeError, ValueError: + return None + return n if n > 0 else None + + +def _read_remedy(suffix: str, cap: int) -> str: + """Build the deny-message remedy clause tailored to the file type. + + Line-based ``offset``/``limit`` paging only makes sense for text files; + for PDFs the agent should page with ``pages``, for notebooks it should + pull individual cells out with ``jq``, and for other binaries (images) + ``Read`` returns the whole file so no paging applies. + """ + if suffix == _PDF_EXTENSION: + return ( + "Re-run Read with the 'pages' parameter to read a bounded page " + "range (e.g. pages='1-5')." + ) + if suffix == _NOTEBOOK_EXTENSION: + return ( + "Read returns the whole notebook (every cell and its outputs), so " + "line paging does not apply. Inspect individual cells with Bash and " + "jq instead (e.g. jq '.cells[].source' notebook.ipynb)." + ) + 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')." + ) + suggested_limit = max(1, cap // _EST_BYTES_PER_LINE) + return ( + f"Re-run Read with 'offset' and 'limit' to page through it " + f"(e.g. offset=1, limit={suggested_limit}), or use Grep with " + f"output_mode='files_with_matches' / a 'head_limit' to locate the " + f"lines you need first, then Read that range." + ) + + +def check_read_output_risk(tool_input: dict[str, Any], cwd: str | None) -> str | None: + """Return a deny reason if a ``Read`` call is likely to overflow. + + Denies when the target file exceeds the configured byte cap and the read + is not bounded to a small enough range. A text read is "bounded" when its + ``limit`` × ~bytes-per-line estimate stays under the cap — a mere ``limit`` + is *not* a free pass, since ``limit=10_000_000`` would still read the whole + file (the #2810 fail-fast caught that gap). A PDF is "bounded" when a + non-empty ``pages`` range is given (the analogue of ``limit`` for PDFs, and + the mechanism the deny remedy points at — capped at 20 pages by the Read + tool). Other binaries (images, notebooks) ignore ``offset``/``limit``/ + ``pages``, so they are judged on size alone. Missing/unstattable files are + allowed (let the real tool report the error). + """ + file_path = tool_input.get("file_path") + if not file_path: + return None + + path = _resolve_path(str(file_path), cwd) + try: + size = path.stat().st_size + except OSError: + # Missing/unreadable — let the real Read tool surface the error. + return None + + cap = _read_cap_bytes() + if size <= cap: + return None + + suffix = path.suffix.lower() + is_pdf = suffix == _PDF_EXTENSION + + if is_pdf: + # A `pages`-scoped PDF read is bounded (the Read tool caps it at 20 + # pages), so it must not be denied while the remedy points at `pages`. + pages = tool_input.get("pages") + if pages is not None and str(pages).strip(): + return None + elif suffix not in _NON_PAGEABLE_BINARY_EXTENSIONS: + # A bounded text read whose estimated payload fits under the cap is safe. + # (Binary reads ignore offset/limit, so a limit never makes them safe.) + limit = _coerce_positive_int(tool_input.get("limit")) + if limit is not None and min(size, limit * _EST_BYTES_PER_LINE) <= cap: + return None + + approx_kb = size // 1024 + return ( + f"Read denied: '{file_path}' is ~{approx_kb} KB, large enough that " + f"reading it whole risks overflowing the agent's 1 MB message buffer " + f"and crashing the session (issue #2804). {_read_remedy(suffix, cap)}" + ) + + +def check_grep_output_risk(tool_input: dict[str, Any]) -> str | None: + """Return a deny reason if a ``Grep`` call is likely to overflow. + + Targets the genuinely unbounded case: ``output_mode='content'`` with + no ``head_limit`` **and** no path/glob narrowing, i.e. dumping every + matching line across the whole repo. Content greps that are scoped + (by ``path`` or ``glob``) or capped (by ``head_limit``) are allowed — + the heuristic deliberately stays narrow to avoid denying the common, + small content grep. + """ + if tool_input.get("output_mode") != "content": + return None + if tool_input.get("head_limit") is not None: + return None + # Scoped to a subtree or file glob → bounded enough; allow. + if tool_input.get("path") or tool_input.get("glob"): + return None + + return ( + "Grep denied: output_mode='content' across the whole repo with no " + "'head_limit' can return an unbounded volume of matching lines and " + "overflow the agent's 1 MB message buffer (issue #2804). Add a " + "'head_limit' (e.g. head_limit=100), scope the search with 'path' or " + "'glob', or use output_mode='files_with_matches' to list files first " + "and then Read the relevant ranges." + ) + + +def check_builtin_tool_output_risk( + tool_name: str, tool_input: dict[str, Any], cwd: str | None +) -> str | None: + """Dispatch a built-in tool call to its predictive-cap checker. + + Returns a deny reason string, or None when the call is allowed (or the + cap is disabled via EGG_TOOL_OUTPUT_CAP). + """ + if is_output_cap_disabled(): + return None + if tool_name == "Read": + return check_read_output_risk(tool_input, cwd) + if tool_name == "Grep": + return check_grep_output_risk(tool_input) + return None diff --git a/tests/shared/egg_agent/test_client.py b/tests/shared/egg_agent/test_client.py index 3bb99d398e..a7f0675dc2 100644 --- a/tests/shared/egg_agent/test_client.py +++ b/tests/shared/egg_agent/test_client.py @@ -975,6 +975,101 @@ def test_web_tool_deny_hook_not_registered_in_private_mode(self, mock_query): assert "WebFetch" not in matchers +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. + """ + + @staticmethod + def _matchers(opts): + hooks = getattr(opts, "hooks", None) or {} + return [hm.matcher for hm in hooks.get("PreToolUse", [])] + + @staticmethod + def _hook_for(opts, matcher): + hooks = opts.hooks["PreToolUse"] + return next(hm for hm in hooks if hm.matcher == matcher).hooks[0] + + @patch.dict(os.environ, {"EGG_MCP_TOOLS": "false", "EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_read_and_grep_matchers_registered_by_default(self, mock_query): + result = _run_async(run_agent_async("test prompt")) + assert result.success is True + opts = mock_query.call_args.kwargs["options"] + matchers = self._matchers(opts) + assert "Read" in matchers + assert "Grep" in matchers + + @patch.dict(os.environ, {"EGG_TOOL_OUTPUT_CAP": "false"}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_kill_switch_removes_matchers(self, mock_query): + result = _run_async(run_agent_async("test prompt")) + assert result.success is True + opts = mock_query.call_args.kwargs["options"] + matchers = self._matchers(opts) + assert "Read" not in matchers + assert "Grep" not in matchers + + @patch.dict(os.environ, {"EGG_MCP_TOOLS": "false", "EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_read_hook_denies_large_file(self, mock_query, tmp_path): + big = tmp_path / "big.py" + big.write_bytes(b"x" * (300 * 1024)) + _run_async(run_agent_async("test prompt", cwd=str(tmp_path))) + opts = mock_query.call_args.kwargs["options"] + hook = self._hook_for(opts, "Read") + out = _run_async( + hook( + {"tool_name": "Read", "tool_input": {"file_path": "big.py"}}, + "tool-1", + None, + ) + ) + decision = out["hookSpecificOutput"] + assert decision["hookEventName"] == "PreToolUse" + assert decision["permissionDecision"] == "deny" + assert "limit" in decision["permissionDecisionReason"] + + @patch.dict(os.environ, {"EGG_MCP_TOOLS": "false", "EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_read_hook_allows_small_file(self, mock_query, tmp_path): + small = tmp_path / "small.py" + small.write_bytes(b"x" * 1024) + _run_async(run_agent_async("test prompt", cwd=str(tmp_path))) + opts = mock_query.call_args.kwargs["options"] + hook = self._hook_for(opts, "Read") + out = _run_async( + hook( + {"tool_name": "Read", "tool_input": {"file_path": "small.py"}}, + "tool-1", + None, + ) + ) + assert out == {} + + @patch.dict(os.environ, {"EGG_MCP_TOOLS": "false", "EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + @patch("claude_agent_sdk.query", side_effect=_mock_query_success) + def test_grep_hook_denies_unbounded_content_grep(self, mock_query): + _run_async(run_agent_async("test prompt")) + opts = mock_query.call_args.kwargs["options"] + hook = self._hook_for(opts, "Grep") + out = _run_async( + hook( + { + "tool_name": "Grep", + "tool_input": {"pattern": "x", "output_mode": "content"}, + }, + "tool-1", + None, + ) + ) + decision = out["hookSpecificOutput"] + assert decision["permissionDecision"] == "deny" + assert "head_limit" in decision["permissionDecisionReason"] + + class TestBufferOverflowErrorHandling: """Issue #2804: when the SDK raises CLIJSONDecodeError on a buffer overflow, the agent must return a structured failure with the diff --git a/tests/shared/egg_agent/test_tool_output_cap.py b/tests/shared/egg_agent/test_tool_output_cap.py new file mode 100644 index 0000000000..87be59f20f --- /dev/null +++ b/tests/shared/egg_agent/test_tool_output_cap.py @@ -0,0 +1,246 @@ +"""Tests for egg_agent.tool_output_cap predictive PreToolUse caps (#2876).""" + +import os +from unittest.mock import patch + +import pytest +from egg_agent import tool_output_cap +from egg_agent.tool_output_cap import ( + check_builtin_tool_output_risk, + check_grep_output_risk, + check_read_output_risk, + is_output_cap_disabled, +) + + +@pytest.fixture(autouse=True) +def _reset_cap_warning_cache(): + # The invalid-cap warning is now once-per-value (module-level cache), so + # clear it between tests to keep the warn/no-warn assertions order-independent. + tool_output_cap._warned_cap_values.clear() + yield + tool_output_cap._warned_cap_values.clear() + + +def _write(tmp_path, name, size): + p = tmp_path / name + p.write_bytes(b"x" * size) + return p + + +class TestReadCap: + def test_denies_unbounded_read_of_large_file(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + reason = check_read_output_risk({"file_path": str(big)}, str(tmp_path)) + assert reason is not None + # Reason must tell the agent how to narrow the call. + assert "offset" in reason and "limit" in reason + assert "#2804" in reason + + def test_allows_large_file_when_limit_given(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + reason = check_read_output_risk({"file_path": str(big), "limit": 2000}, str(tmp_path)) + assert reason is None + + def test_allows_small_file(self, tmp_path): + small = _write(tmp_path, "small.py", 1024) + assert check_read_output_risk({"file_path": str(small)}, str(tmp_path)) is None + + def test_allows_missing_file(self, tmp_path): + # Let the real Read tool surface the "file not found" error. + assert check_read_output_risk({"file_path": "does-not-exist.py"}, str(tmp_path)) is None + + def test_resolves_relative_path_against_cwd(self, tmp_path): + _write(tmp_path, "rel.py", 300 * 1024) + reason = check_read_output_risk({"file_path": "rel.py"}, str(tmp_path)) + assert reason is not None + + def test_no_file_path_allowed(self, tmp_path): + assert check_read_output_risk({}, str(tmp_path)) is None + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "1024"}) + def test_threshold_configurable_via_env(self, tmp_path): + mid = _write(tmp_path, "mid.py", 2048) + assert check_read_output_risk({"file_path": str(mid)}, str(tmp_path)) is not None + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "not-a-number"}) + def test_invalid_env_falls_back_to_default(self, tmp_path): + # 2 KB is under the 256 KiB default, so it is allowed despite bad env. + mid = _write(tmp_path, "mid.py", 2048) + assert check_read_output_risk({"file_path": str(mid)}, str(tmp_path)) is None + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "0"}) + @patch("egg_agent.tool_output_cap.logger") + def test_zero_env_warns_and_falls_back_to_default(self, mock_logger, tmp_path): + # 0 is non-positive → invalid; fall back to the 256 KiB default and warn. + big = _write(tmp_path, "big.py", 300 * 1024) + assert check_read_output_risk({"file_path": str(big)}, str(tmp_path)) is not None + small = _write(tmp_path, "small.py", 2048) + assert check_read_output_risk({"file_path": str(small)}, str(tmp_path)) is None + mock_logger.warning.assert_called() + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "-5"}) + @patch("egg_agent.tool_output_cap.logger") + def test_negative_env_warns_and_falls_back_to_default(self, mock_logger, tmp_path): + small = _write(tmp_path, "small.py", 2048) + assert check_read_output_risk({"file_path": str(small)}, str(tmp_path)) is None + mock_logger.warning.assert_called() + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "2mb"}) + @patch("egg_agent.tool_output_cap.logger") + def test_unparseable_env_warns(self, mock_logger, tmp_path): + # set-but-unparseable must be loud, not silently swallowed (#2876 review). + small = _write(tmp_path, "small.py", 2048) + check_read_output_risk({"file_path": str(small)}, str(tmp_path)) + mock_logger.warning.assert_called() + + @patch.dict(os.environ, {"EGG_READ_CAP_BYTES": "0"}) + @patch("egg_agent.tool_output_cap.logger") + def test_invalid_env_warns_only_once_across_reads(self, mock_logger, tmp_path): + # A steady misconfiguration must not spam one warning per Read (#2876 + # re-review): resolve the cap several times, expect a single warning. + small = _write(tmp_path, "small.py", 1024) + for _ in range(5): + check_read_output_risk({"file_path": str(small)}, str(tmp_path)) + assert mock_logger.warning.call_count == 1 + + @patch("egg_agent.tool_output_cap.logger") + def test_unset_env_does_not_warn(self, mock_logger, tmp_path): + # The unset case uses the expected default — it must stay silent. + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("EGG_READ_CAP_BYTES", None) + small = _write(tmp_path, "small.py", 1024) + check_read_output_risk({"file_path": str(small)}, str(tmp_path)) + mock_logger.warning.assert_not_called() + + def test_denies_oversized_limit_on_large_file(self, tmp_path): + # A huge limit still reads (nearly) the whole file → must be denied, not + # waved through on the mere presence of a limit (#2876 review item 3). + big = _write(tmp_path, "big.py", 300 * 1024) + reason = check_read_output_risk({"file_path": str(big), "limit": 10_000_000}, str(tmp_path)) + assert reason is not None + + def test_allows_modest_limit_on_large_file(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + assert check_read_output_risk({"file_path": str(big), "limit": 100}, str(tmp_path)) is None + + def test_non_positive_limit_treated_as_unbounded(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + assert ( + check_read_output_risk({"file_path": str(big), "limit": 0}, str(tmp_path)) is not None + ) + + def test_pdf_deny_points_at_pages(self, tmp_path): + # offset/limit are line-based; for a PDF the remedy is the 'pages' param. + pdf = _write(tmp_path, "big.pdf", 300 * 1024) + reason = check_read_output_risk({"file_path": str(pdf)}, str(tmp_path)) + assert reason is not None + assert "pages" in reason + + def test_pdf_with_pages_is_allowed(self, tmp_path): + # The deny remedy tells the agent to use 'pages'; a pages-scoped read + # must then be honored, not denied again (#2876 re-review). The Read + # tool caps a pages request at 20 pages, so it's inherently bounded. + pdf = _write(tmp_path, "big.pdf", 300 * 1024) + assert ( + check_read_output_risk({"file_path": str(pdf), "pages": "1-5"}, str(tmp_path)) is None + ) + + def test_pdf_with_empty_pages_still_denied(self, tmp_path): + # An empty/whitespace 'pages' is not a real page range → still unbounded. + pdf = _write(tmp_path, "big.pdf", 300 * 1024) + assert ( + check_read_output_risk({"file_path": str(pdf), "pages": " "}, str(tmp_path)) + is not None + ) + + def test_notebook_deny_suggests_jq(self, tmp_path): + # Read returns a notebook whole; offset/limit/pages don't help, so the + # remedy should point at jq cell inspection rather than file/stat. + nb = _write(tmp_path, "big.ipynb", 300 * 1024) + reason = check_read_output_risk({"file_path": str(nb)}, str(tmp_path)) + assert reason is not None + assert "jq" in reason + assert "offset" not in reason and "limit" not in reason + + def test_image_deny_does_not_suggest_line_paging(self, tmp_path): + png = _write(tmp_path, "big.png", 300 * 1024) + reason = check_read_output_risk({"file_path": str(png)}, str(tmp_path)) + assert reason is not None + # offset/limit are meaningless for a binary read — must not be suggested. + assert "offset" not in reason and "limit" not in reason + assert "binary" in reason + + def test_binary_limit_does_not_bypass_cap(self, tmp_path): + # Read returns a binary file whole, so a limit never bounds it. + png = _write(tmp_path, "big.png", 300 * 1024) + assert ( + check_read_output_risk({"file_path": str(png), "limit": 10}, str(tmp_path)) is not None + ) + + +class TestGrepCap: + def test_denies_unbounded_content_grep(self): + reason = check_grep_output_risk({"pattern": "foo", "output_mode": "content"}) + assert reason is not None + assert "head_limit" in reason + assert "files_with_matches" in reason + + def test_allows_content_grep_with_head_limit(self): + assert ( + check_grep_output_risk({"pattern": "foo", "output_mode": "content", "head_limit": 50}) + is None + ) + + def test_allows_content_grep_scoped_by_path(self): + assert ( + check_grep_output_risk( + {"pattern": "foo", "output_mode": "content", "path": "orchestrator/"} + ) + is None + ) + + def test_allows_content_grep_scoped_by_glob(self): + assert ( + check_grep_output_risk({"pattern": "foo", "output_mode": "content", "glob": "*.py"}) + is None + ) + + def test_allows_files_with_matches_mode(self): + assert ( + check_grep_output_risk({"pattern": "foo", "output_mode": "files_with_matches"}) is None + ) + + def test_allows_default_mode(self): + # No output_mode → files_with_matches default → bounded. + assert check_grep_output_risk({"pattern": "foo"}) is None + + +class TestDispatchAndKillSwitch: + # Pin EGG_TOOL_OUTPUT_CAP for the cap-on dispatch cases so an ambient + # env that disables the cap can't make them fail spuriously (#2876 review). + @patch.dict(os.environ, {"EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + def test_dispatch_read(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + assert ( + check_builtin_tool_output_risk("Read", {"file_path": str(big)}, str(tmp_path)) + is not None + ) + + @patch.dict(os.environ, {"EGG_TOOL_OUTPUT_CAP": ""}, clear=False) + def test_dispatch_grep(self): + assert ( + check_builtin_tool_output_risk("Grep", {"pattern": "x", "output_mode": "content"}, None) + is not None + ) + + def test_dispatch_other_tool_allowed(self, tmp_path): + assert check_builtin_tool_output_risk("Edit", {"file_path": "x"}, str(tmp_path)) is None + + @patch.dict(os.environ, {"EGG_TOOL_OUTPUT_CAP": "false"}) + def test_kill_switch_disables_dispatch(self, tmp_path): + big = _write(tmp_path, "big.py", 300 * 1024) + assert is_output_cap_disabled() is True + assert ( + check_builtin_tool_output_risk("Read", {"file_path": str(big)}, str(tmp_path)) is None + )