From 7448a84b1f4779119fa0b0bc620a4206a9d0e1ae Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Fri, 28 Aug 2026 01:53:02 -0700 Subject: [PATCH] feat(tools): cap subprocess output with an aggregate byte limit (v0.6.5) Replace subprocess.run(capture_output=True) in _run() with Popen + two concurrent drain threads that enforce a plugin-owned aggregate stdout+stderr byte cap (max_response_bytes, default 200000). On overflow the child is killed and a bounded "Error: output exceeded {cap} bytes (truncated)" is returned; on timeout the child is terminated and reaped. Draining both pipes concurrently avoids deadlock, and the child is always reaped (no zombies). Within-cap behavior is unchanged; stderr on failure is still sliced to 500 chars. Co-Authored-By: Claude Opus 4.8 (1M context) --- protoagent.plugin.yaml | 6 +- pyproject.toml | 2 +- tests/test_agent_browser.py | 132 +++++++++++++++++++++++++++++++++--- tools.py | 74 ++++++++++++++++++-- 4 files changed, 197 insertions(+), 17 deletions(-) diff --git a/protoagent.plugin.yaml b/protoagent.plugin.yaml index ce8a19c..d55c28a 100644 --- a/protoagent.plugin.yaml +++ b/protoagent.plugin.yaml @@ -1,6 +1,6 @@ id: agent_browser name: Agent Browser -version: 0.6.4 +version: 0.6.5 description: >- Browser automation for protoAgent, backed by **agent-browser** (vercel-labs) — a fast native-Rust CLI/daemon that drives Chrome over CDP with accessibility-tree @@ -23,6 +23,9 @@ config_section: agent_browser config: binary: agent-browser # the agent-browser CLI on PATH (override for a pinned path) timeout_s: 60 # per-command subprocess timeout + max_response_bytes: 200000 # aggregate stdout+stderr byte cap per command; untrusted page + # output over this is dropped and the tool returns a bounded error + # (memory + model-context safety). Read/enforced by the plugin wrapper. home_url: "" # homepage the panel opens to. When set, the panel auto-opens it if # no browser page is open; the empty state also shows a Start button. # Blank → the Start button opens about:blank (no auto-open). @@ -59,6 +62,7 @@ settings: - { key: stream_quality, label: "Panel stream quality", type: number, description: "Interactive panel JPEG quality (1–100). Higher = crisper, more bandwidth. The panel also renders at your dock's size × device-pixel-ratio." } - { key: max_output, label: "Max page-text output (chars)", type: number, description: "Cap characters of page text returned to the model (LLM-safety). 0 = the CLI default." } - { key: timeout_s, label: "Command timeout (s)", type: number, description: "Per-command subprocess timeout for the browser tools." } + - { key: max_response_bytes, label: "Max response bytes", type: number, description: "Aggregate stdout+stderr byte cap per browser command. Output beyond this is dropped and the tool returns a bounded error — protects process memory and the model's context from unbounded page content. Default 200000." } - { key: binary, label: "agent-browser binary", type: string, description: "The agent-browser CLI on PATH (override with a pinned absolute path if needed)." } # Console view (ADR 0026) — an interactive, drivable browser viewport (CDP screencast + input). diff --git a/pyproject.toml b/pyproject.toml index 1efa443..75feec3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agent-browser-plugin" -version = "0.6.4" +version = "0.6.5" description = "Browser-automation plugin for protoAgent, backed by vercel-labs/agent-browser (tools + skill + workflows + an interactive, drivable browser panel)." requires-python = ">=3.11" diff --git a/tests/test_agent_browser.py b/tests/test_agent_browser.py index 9fdf242..de726f1 100644 --- a/tests/test_agent_browser.py +++ b/tests/test_agent_browser.py @@ -5,6 +5,8 @@ from __future__ import annotations +import io +import subprocess from pathlib import Path import pytest @@ -20,12 +22,64 @@ def _toolmap(cfg=None): return {t.name: t for t in tools.get_browser_tools(cfg or {})} +# ── a subprocess.Popen stand-in for the tool wrappers ──────────────────────────── +# _run() now streams the child's pipes through drain threads under a byte cap, so the +# tool tests mock Popen (not run): BytesIO pipes yield the canned bytes, wait()/kill() +# drive the timeout + reap paths. + + +class _FakeProc: + """Minimal Popen: BytesIO pipes + wait/kill, enough for _run's drain loop.""" + + def __init__(self, argv, out=b"", err=b"", rc=0, timeout=False): + self._argv = list(argv) + self.stdout = io.BytesIO(out) + self.stderr = io.BytesIO(err) + self._rc = rc + self._timeout = timeout # make wait(timeout=…) raise until killed + self.returncode = None + self.killed = False + + def wait(self, timeout=None): + if self._timeout and timeout is not None and not self.killed: + raise subprocess.TimeoutExpired(cmd=self._argv[0], timeout=timeout) + if self.returncode is None: + self.returncode = -9 if self.killed else self._rc + return self.returncode + + def kill(self): + self.killed = True + self.returncode = -9 + + def poll(self): + return self.returncode + + +def fake_popen(out=b"", err=b"", rc=0, timeout=False, record=None, procs=None): + """A subprocess.Popen stand-in: records argv, returns a _FakeProc whose pipes yield + the canned bytes. Swallows the stdout=/stderr= PIPE kwargs the wrapper passes.""" + if isinstance(out, str): + out = out.encode() + if isinstance(err, str): + err = err.encode() + + def _popen(argv, **kw): + if record is not None: + record.append(list(argv)) + p = _FakeProc(argv, out=out, err=err, rc=rc, timeout=timeout) + if procs is not None: + procs.append(p) + return p + + return _popen + + # ── the tools: arg-building ────────────────────────────────────────────────────── async def test_open_passes_url_and_curated_launch_flags(monkeypatch): rec = [] - monkeypatch.setattr(tools.subprocess, "run", fake_run(stdout="OPENED", record=rec)) + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out="OPENED", record=rec)) # headless so argv is clean (headed injects anti-throttle --args; covered in test_runtime) t = _toolmap({"binary": "ab", "allowed_domains": "x.com", "max_output": 500}) out = await t["browser_open"].ainvoke({"url": "https://x.com"}) @@ -35,14 +89,14 @@ async def test_open_passes_url_and_curated_launch_flags(monkeypatch): async def test_open_blank_url_omits_it(monkeypatch): rec = [] - monkeypatch.setattr(tools.subprocess, "run", fake_run(record=rec)) + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(record=rec)) await _toolmap({"binary": "ab"})["browser_open"].ainvoke({}) assert rec[-1] == ["ab", "open"] async def test_action_tools_pass_refs(monkeypatch): rec = [] - monkeypatch.setattr(tools.subprocess, "run", fake_run(record=rec)) + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(record=rec)) t = _toolmap({"binary": "ab"}) await t["browser_click"].ainvoke({"selector": "@e2"}) assert rec[-1] == ["ab", "click", "@e2"] @@ -71,29 +125,76 @@ def test_all_16_tools_present(): async def test_missing_binary_returns_install_hint(monkeypatch): - def boom(args, **kw): + def boom(argv, **kw): raise FileNotFoundError() - monkeypatch.setattr(tools.subprocess, "run", boom) + monkeypatch.setattr(tools.subprocess, "Popen", boom) out = await _toolmap({"binary": "nope"})["browser_snapshot"].ainvoke({}) assert "not on PATH" in out and "npm i -g agent-browser" in out -async def test_timeout_returns_readable_error(monkeypatch): - def slow(args, **kw): - raise tools.subprocess.TimeoutExpired(cmd="ab", timeout=1) - - monkeypatch.setattr(tools.subprocess, "run", slow) +async def test_timeout_returns_readable_error_and_reaps_child(monkeypatch): + procs = [] + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(timeout=True, procs=procs)) out = await _toolmap({"binary": "ab", "timeout_s": 1})["browser_snapshot"].ainvoke({}) assert "timed out" in out + assert procs[0].killed # child terminated + reaped on timeout — never a zombie async def test_nonzero_exit_surfaces_stderr(monkeypatch): - monkeypatch.setattr(tools.subprocess, "run", fake_run(rc=2, stderr="boom")) + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(rc=2, err="boom")) out = await _toolmap({"binary": "ab"})["browser_click"].ainvoke({"selector": "@e9"}) assert out.startswith("Error:") and "boom" in out +# ── the tools: aggregate stdout+stderr byte cap (memory + context safety) ────────── + + +async def test_output_within_cap_is_unchanged(monkeypatch): + # under the cap → behavior identical to before: raw stdout, stripped. + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out="hello world\n")) + out = await _toolmap({"binary": "ab", "max_response_bytes": 100})["browser_get_text"].ainvoke({"selector": "body"}) + assert out == "hello world" + + +async def test_output_over_cap_is_truncated_with_diagnostic(monkeypatch): + procs = [] + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out=b"x" * 5000, procs=procs)) + t = _toolmap({"binary": "ab", "max_response_bytes": 100}) + out = await t["browser_get_text"].ainvoke({"selector": "body"}) + assert out == "Error: output exceeded 100 bytes (truncated)" + assert procs[0].killed # overflow kills the child cleanly + + +async def test_aggregate_stdout_plus_stderr_is_bounded(monkeypatch): + # neither stream alone exceeds the cap, but together they do → still bounded. + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out=b"a" * 60, err=b"b" * 60)) + out = await _toolmap({"binary": "ab", "max_response_bytes": 100})["browser_snapshot"].ainvoke({}) + assert out == "Error: output exceeded 100 bytes (truncated)" + + +async def test_configured_cap_overrides_default(monkeypatch): + # a small configured cap trips where the 200KB default would not. + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out=b"y" * 1000)) + out = await _toolmap({"binary": "ab", "max_response_bytes": 10})["browser_get_html"].ainvoke({}) + assert out == "Error: output exceeded 10 bytes (truncated)" + + +async def test_default_cap_is_200kb_when_unconfigured(monkeypatch): + # no max_response_bytes key → 200000 default applies; 250KB overflows, and the + # diagnostic names the default limit. + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out=b"z" * 250_000)) + out = await _toolmap({"binary": "ab"})["browser_get_text"].ainvoke({"selector": "body"}) + assert out == "Error: output exceeded 200000 bytes (truncated)" + + +async def test_output_at_the_cap_is_not_truncated(monkeypatch): + # exactly the cap is allowed through (only strictly-larger output overflows). + monkeypatch.setattr(tools.subprocess, "Popen", fake_popen(out=b"q" * 50)) + out = await _toolmap({"binary": "ab", "max_response_bytes": 50})["browser_get_text"].ainvoke({"selector": "body"}) + assert out == "q" * 50 + + # ── register() wiring ──────────────────────────────────────────────────────────── @@ -136,6 +237,15 @@ def test_settings_fields_are_valid_and_back_real_config(): assert set(by_key) <= set(m["config"]) +def test_max_response_bytes_default_is_declared(): + import yaml + + m = yaml.safe_load((ROOT / "protoagent.plugin.yaml").read_text()) + assert m["config"]["max_response_bytes"] == 200000 # the wrapper's default cap + by_key = {f["key"]: f for f in m["settings"]} + assert by_key["max_response_bytes"]["type"] == "number" # operator-editable knob + + # ── the panel routes (page / ticket / WS gating / nav) ─────────────────────────── diff --git a/tools.py b/tools.py index 8491285..7f6d871 100644 --- a/tools.py +++ b/tools.py @@ -15,6 +15,7 @@ import asyncio import logging import subprocess +import threading from langchain_core.tools import tool @@ -22,24 +23,89 @@ log = logging.getLogger("protoagent.plugins.agent_browser") +# Default aggregate stdout+stderr byte cap when `max_response_bytes` is unset. +_DEFAULT_MAX_RESPONSE_BYTES = 200_000 +_READ_CHUNK = 65_536 + def get_browser_tools(cfg: dict | None): cfg = cfg or {} binary = str(cfg.get("binary") or "agent-browser") timeout = float(cfg.get("timeout_s", 60)) + # Plugin-owned cap on the total bytes a single invocation may buffer. Untrusted page + # content (get text/html, eval) can emit unbounded output that would otherwise pile up + # in memory and flood the model's context window, so we read the pipes incrementally + # and stop the child the moment the aggregate crosses the cap. + max_bytes = int(cfg.get("max_response_bytes", _DEFAULT_MAX_RESPONSE_BYTES) or _DEFAULT_MAX_RESPONSE_BYTES) def _run(*args: str) -> str: - """Run `agent-browser ` and return stdout, or a readable error.""" + """Run `agent-browser ` and return stdout, or a readable error. + + Uses Popen with one drain thread per pipe (concurrent, so a full stderr can't + deadlock stdout) enforcing an aggregate byte cap owned here. On overflow the child + is killed and a bounded diagnostic is returned; on timeout the child is terminated. + Either way the child is reaped — no zombies. + """ try: - proc = subprocess.run([binary, *args], capture_output=True, text=True, timeout=timeout) + proc = subprocess.Popen([binary, *args], stdout=subprocess.PIPE, stderr=subprocess.PIPE) except FileNotFoundError: return (f"Error: {binary!r} not on PATH — install it: " "`npm i -g agent-browser && agent-browser install`") + + out_buf, err_buf = bytearray(), bytearray() + total = 0 + overflow = False + lock = threading.Lock() + + def _drain(pipe, buf: bytearray) -> None: + nonlocal total, overflow + hit = False + try: + for block in iter(lambda: pipe.read(_READ_CHUNK), b""): + with lock: + if overflow: + break + total += len(block) + if total > max_bytes: # aggregate crossed the cap → stop + kill + overflow = True + hit = True + break + buf.extend(block) + except (OSError, ValueError): + pass # pipe closed under us (e.g. after kill) — nothing more to read + finally: + try: + pipe.close() + except Exception: + pass + if hit: + proc.kill() # unblock the sibling reader and let wait() return + + drains = [threading.Thread(target=_drain, args=(proc.stdout, out_buf), daemon=True), + threading.Thread(target=_drain, args=(proc.stderr, err_buf), daemon=True)] + for t in drains: + t.start() + + timed_out = False + try: + proc.wait(timeout=timeout) except subprocess.TimeoutExpired: + timed_out = True + proc.kill() # terminate … + try: + proc.wait(timeout=5) # … and reap, so we never leave a zombie + except subprocess.TimeoutExpired: + pass + for t in drains: + t.join() + + if timed_out: return f"Error: `agent-browser {' '.join(args)}` timed out after {timeout:g}s" - out = (proc.stdout or "").strip() + if overflow: + return f"Error: output exceeded {max_bytes} bytes (truncated)" + out = out_buf.decode("utf-8", "replace").strip() if proc.returncode != 0: - err = (proc.stderr or out or "").strip() + err = (err_buf.decode("utf-8", "replace") or out or "").strip() return f"Error: `agent-browser {' '.join(args)}` failed: {err[:500]}" return out or "(ok)"