feat: Add aggregate byte cap to subprocess output capture - #20
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 7448a84b1f47 · formal
Low-risk PR: the core change (switching tools.py from subprocess.run to subprocess.Popen with drain threads, and updating the test mocks accordingly) was reviewed by two finders and found sound. The single actionable item is a stale documentation reference — PROTO.md's "Test patterns" section still describes the old conftest.fake_run / CompletedProcess mechanism that no longer exists. Fix first: update that bullet to describe the new fake_popen / _FakeProc pattern. No panel disagreement; the verifier confirmed the finding by re-reading PROTO.md on main and cross-checking the diff. Coverage note: tools.py (the main production file) drew zero findings, which is consistent with the finders' assessment that the Popen refactor is correct.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | PROTO.md |
The PR moves CLI mocking from subprocess.run/conftest.fake_run to subprocess.Popen with a test-local fake_popen but leaves PROTO.md's 'Test patterns' section d… | confirmed |
findings JSON (machine-readable)
[
{
"file": "PROTO.md",
"line": 0,
"severity": "minor",
"category": "conventions",
"claim": "The PR moves CLI mocking from subprocess.run/conftest.fake_run to subprocess.Popen with a test-local fake_popen but leaves PROTO.md's 'Test patterns' section describing the old mechanism, so the repo's canonical grounding doc now misdescribes how the tool tests mock the CLI.",
"evidence": "PROTO.md (base ref): \"Mock the CLI \u2014 monkeypatch `subprocess.run` with `conftest.fake_run(...)`. It records the argv it was called with and returns a canned `CompletedProcess`\" \u2014 but the diff replaces every such call with `monkeypatch.setattr(tools.subprocess, \"Popen\", fake_popen(out=\"OPENED\", record=rec))` and adds a new local `def fake_popen(out=b\"\", err=b\"\", rc=0, timeout=False, record=None, procs=None):` in tests/test_agent_browser.py; PROTO.md is not touched by the PR.",
"verdict": "confirmed",
"note": "Re-read PROTO.md on main: the 'Test patterns' bullet verbatim says 'monkeypatch subprocess.run with conftest.fake_run(...) \u2026 returns a canned CompletedProcess'. The PR diff shows all test calls now use subprocess.Popen + local fake_popen (returning _FakeProc with BytesIO pipes), and PROTO.md is absent from the diff. The doc is stale."
}
]1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):
PROTO.md(minor) — The PR moves CLI mocking from subprocess.run/conftest.fake_run to subprocess.Popen with a test-local fake_popen but leaves PROTO.md's 'Test patterns' section de
Summary
The
_run()helper intools.pypreviously buffered a browser command's entire stdout+stderr viasubprocess.run(capture_output=True), so untrusted page content (viaget text,get html,eval) could consume process memory and flood the model's context with no plugin-owned bound._run()now usessubprocess.Popenwith two concurrent drain threads (one per pipe, so a full stderr can't deadlock stdout) that enforce an aggregate stdout+stderr byte cap. The cap is a new config key,max_response_bytes(default200000), read at tool-registration time and exposed as an operator-editable Settings field. On overflow the child is killed and the tool returns a boundedError: output exceeded {cap} bytes (truncated); on timeout the child is terminated — and in both cases reaped, so no zombies are left behind. Within-cap behavior is byte-for-byte unchanged (raw stdout stripped; stderr on failure still sliced to 500 chars), and the existingtimeout_scontinues to work alongside the new cap. Version bumped to0.6.5in bothprotoagent.plugin.yamlandpyproject.toml.Tests were retargeted from mocking
subprocess.runto aPopenstand-in withBytesIOpipes, plus new coverage for: over-cap truncation + child kill, exact-cap pass-through, aggregate stdout+stderr bounding, configured-cap override, the 200KB default when unconfigured, and timeout terminate-and-reap.Fixes #18