fix(agents): handle ripgrep stdout/stderr stream errors in grep tool#101014
Conversation
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 5:00 PM ET / 21:00 UTC. Summary PR surface: Source +13, Tests +78. Total +91 across 2 files. Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main and v2026.6.11 attach no raw stdout/stderr error listeners in the grep spawn path, and Node throws unhandled error events. I did not execute a failing current-main run because this review was read-only. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused grep stream-error fix after the normal base refresh and exact-head CI, keeping the implementation local to the grep child-process lifecycle and the regression test focused on stream failures. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: current main and v2026.6.11 attach no raw stdout/stderr error listeners in the grep spawn path, and Node throws unhandled error events. I did not execute a failing current-main run because this review was read-only. Is this the best way to solve the issue? Yes. The fix belongs at the grep-owned child-process/readline boundary; a shared abstraction is not required for this small patch, and the shape matches the merged find-tool invariant. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c7295e417d5d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +13, Tests +78. Total +91 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles)
|
423533a to
50f7e4f
Compare
|
@clawsweeper re-review\n\nUpdated the stdout/stderr stream-error handlers to call stopChild() before rejecting, and extended the regression tests to assert the child is killed. |
5e0a530 to
33d7013
Compare
|
@clawsweeper re-review\n\nFixed the mock child so the kill spy mutates |
a151dd8 to
a1bcbaa
Compare
98c7238 to
66d6b72
Compare
66d6b72 to
612a686
Compare
|
Land-ready maintainer proof
Known proof gaps: none. |
|
Merged via squash.
|
…penclaw#101014) * fix(agents): handle ripgrep stdout/stderr stream errors in grep tool * fix(agents): terminate ripgrep child on stdout/stderr stream errors * test(agents): make mock child reflect kill state in grep stream-error test * refactor(agents): harden grep stream cleanup * refactor(agents): centralize grep stream failure cleanup * test(agents): model grep child kill state safely --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
…penclaw#101014) * fix(agents): handle ripgrep stdout/stderr stream errors in grep tool * fix(agents): terminate ripgrep child on stdout/stderr stream errors * test(agents): make mock child reflect kill state in grep stream-error test * refactor(agents): harden grep stream cleanup * refactor(agents): centralize grep stream failure cleanup * test(agents): model grep child kill state safely --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
What Problem This Solves
The built-in grep tool owned a piped ripgrep child but did not handle stdout or stderr stream failures. Node treats an unhandled
errorevent as process-fatal, and readline removes its input-error listener when the interface closes, so a pipe failure could crash the agent runtime or escape after cleanup.Why This Change Was Made
One lifecycle-owned stream-error helper now terminates ripgrep, closes readline, removes the abort listener, and rejects the tool promise exactly once. Both readline's re-emitted input error and the direct stdout guard remain attached; the direct guard is required after readline closes. The implementation mirrors the landed find-tool invariant without adding a new shared abstraction.
Duplicate proof scaffolding and the root changelog edit were removed. Test kill state is modeled without mutating Node's readonly child-process property.
User Impact
Grep tool calls fail with a contextual stdout/stderr error instead of crashing the Gateway, hanging, or returning incomplete matches.
Evidence
612a686a5c18d31abfd5811e6c21fe9255cfd807; focused head66d6b728a001f23075bf82ff6e8244d09c7e7c3don sanitized public-network AWS Crabboxcbx_81807643f51awith no Tailscale or instance profile:run_f0ff41fe6048:pnpm test src/agents/sessions/tools/grep.stream-errors.test.ts— 3/3 passed.run_678bc00cab3e: real managed ripgrep searched a fixture successfully, then real stdout and stderr pipe failures terminated their children while the parent stayed alive.The stream contract was checked against Node's official EventEmitter documentation and readline implementation.