feat: Add aggregate byte cap to subprocess output capture

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

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"
 

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) ───────────────────────────
 
 

tools.py

@@ -15,31 +15,97 @@
 import asyncio
 import logging
 import subprocess
+import threading
 
 from langchain_core.tools import tool
 
 from .runtime import launch_flags
 
 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 <args>` and return stdout, or a readable error."""
+        """Run `agent-browser <args>` 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)"