fix(improvement): handle MCP verifier stdin pipe errors#508
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 3a66effd
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-11T06:01:26Z
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 89.3s (2 bridge agents) |
| Total | 89.3s |
💰 Value — sound
Adds a child.stdin 'error' listener that routes async EPIPE through the existing single-settlement failCandidate path instead of crashing the process — coherent, in-grain, no better approach found.
- What it does: Adds child.stdin.on('error', ...) at src/improvement/mcp-serve-verifier.ts:103-106. Node delivers EPIPE asynchronously via the stream 'error' event, not via a synchronous throw from Writable.write(); without a listener that error is uncaught and crashes the test/verifier process. The handler calls failCandidate(
writing to MCP server stdin failed: ${err.message}), so a server that closes fd 0 bec - Goals it achieves: Closes a real hole in the outcome taxonomy the file header documents (mcp-serve-verifier.ts:9-12): every server-side failure must resolve as a failed candidate, never an uncaught exception. The existing send() try/catch (lines 77-86) already states this intent ('EPIPE: the server died mid-handshake — a failed candidate') but only catches synchronous throws, which is not how Node surfaces EPIPE; th
- Assessment: Good on its merits and built in the codebase's grain. It reuses the existing failCandidate -> settle -> resolve({ok:false}) machinery (lines 62-74), the settled guard already prevents double-resolution if exit+stdin-error both fire, and it deliberately reuses the identical message string as send()'s catch (line 83 vs 105), so feedback is uniform regardless of which path catches the pipe failure. N
- Better / existing approach: none — this is the right approach. Searched the repo for any shared stdin/pipe-error helper to reuse: grep 'stdin.on'/'stdin.write' across src/bench/examples finds only write-then-end sites (bench/src/browser/process-adapter.ts:81, src/runtime/supervise/runtime.ts:791, bench/src/benchmarks/appworld.ts:276, examples/mcp-delegation/mcp-delegation.ts:146); none implement an async EPIPE-to-settlemen
- Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
Correct async EPIPE handling for the MCP verifier's stdin pipe, routed through the existing single-settlement path — a real crash bug fixed in the grain of the code.
- Integration:
mcpServeVerifieris the intrinsic verifier wired intobuildVerifierat src/lifecycle/tool-build.ts:136 foropts.kind === 'mcp', and re-exported from src/improvement/index.ts:35. The newchild.stdin.on('error', ...)handler is on the same spawned child the verifier already manages, so it is reachable on every MCP candidate run whose server closes fd 0 mid-handshake. No new surface; the hand - Fit with existing patterns: Follows the established pattern exactly.
child.on('exit', ...)at src/improvement/mcp-serve-verifier.ts:98-102 already routes an asynchronous child event throughfailCandidate->settle. The new handler at :103-106 does the identical thing for the stdin stream's async 'error' event.settle(line 62-69) is idempotent via thesettledflag, so a synchronoussend()failure followed by the - Real-world viability: Addresses a genuine Node behavior:
child.stdin.write()buffers and returns true synchronously, so the real EPIPE arrives later as an asynchronous 'error' event on the stream — which, unhandled, crashes the process as an uncaughtException. The reproduction (server closes fd 0 then emits an initialize response) is realistic for a misbehaving generated MCP server. The fix correctly classifies this - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 send() try/catch now overlaps the async stdin 'error' handler [maintenance] ``
send()'s try/catch (mcp-serve-verifier.ts:77-86) duplicates the new handler's intent and message. EPIPE is delivered asynchronously, so the catch rarely fires for the real failure; it still covers genuine synchronous write errors. Optionally drop or narrow the catch and let the listener own pipe failures, but keeping both is harmless and not gating.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| deepseek | kimi-code | aggregate | |
|---|---|---|---|
| Readiness | 92 | 89 | 89 |
| Confidence | 70 | 70 | 70 |
| Correctness | 92 | 89 | 89 |
| Security | 92 | 89 | 89 |
| Testing | 92 | 89 | 89 |
| Architecture | 92 | 89 | 89 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision.
🟡 LOW Duplicated stdin-failure message string — src/improvement/mcp-serve-verifier.ts
The literal
writing to MCP server stdin failed: ${err.message}now appears in two places: the synchronoussend()catch (line 83) and the new asyncchild.stdin.on('error')handler (line 104). If one is edited and the other is not, the closed-stdin test (tests/mcp-serve-verifier.test.ts) which matches this exact substring could drift from reality. Non-blocking; optionally extract to a sharedconst stdinFail = (e: Error) => ...used by both. No functional impact.
🟡 LOW Theoretical race: test may exercise wrong error path if EPIPE is delayed — tests/mcp-serve-verifier.test.ts
The closed-stdin server closes stdin then writes a response with id=1 (matching the verifier's initId). If the parent's child.stdin.write() buffers successfully before the kernel returns EPIPE, and if the child's stdout response arrives on the parent's readline before the 'error' event fires, the verifier processes the init response as if it were legitimate and proceeds to send notifications/initialized + tools/list. Those subsequent writes would also hit the broken pipe, but if EPIPE is delayed across multiple writes, the 800ms timeout could fire first, producing a timeout message instead of the expected 'writing to MCP server stdin failed'. The settled guard prevents double-resolution but doesn't prevent the wrong path being taken. Extremely unlikely in practice — EPIPE fires in the same
🟡 LOW closed-stdin fixture implicitly couples to verifier's initId === 1 — tests/mcp-serve-verifier.test.ts
The fake initialize response hardcodes id: 1, which only matches the verifier because the source allocates initId as the first id (nextId starts at 1). The EPIPE that this test asserts actually fires on the SECOND write (notifications/initialized), which is only sent after the readline matches msg.id === initId. If the verifier ever changes its id allocation (e.g. starts nextId at 0, or emits a prior message), the fixture's id:1 line would no longer match initId, the verifier would never send the second write, EPIPE would never fire, and the test would then fall through to the 800ms timeout path and assert feedback 'did not complete the handshake' — failing toContain('writing to MCP server stdin failed'). Not a current bug (verified passing 100/100), but the test would silently stop coveri
tangletools · 2026-07-11T06:14:02Z · trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 3 non-blocking findings — 3a66effd
Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 2 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-07-11T06:14:02Z · immutable trace
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 662e52c1
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-11T06:17:17Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 58.4s (2 bridge agents) |
| Total | 58.4s |
💰 Value — sound-with-nits
Routes async MCP-server stdin pipe errors through the existing single-settlement failed-candidate path instead of crashing the process — coherent, in-grain, with a matching regression; ship.
- What it does: Adds
child.stdin.on('error', failStdin)at src/improvement/mcp-serve-verifier.ts:106 and extracts the inline EPIPE message into a sharedfailStdinhelper (line 76) used by both the new async event and the existing synchronous try/catch insend()(line 85). Both now resolve{ok:false, feedback}via the samesettleguard as exit/timeout/handshake failures. Adds aclosed-stdintest varian - Goals it achieves: Pipe failures normally arrive asynchronously, outside
send()'s try/catch — before this they surfaced as an uncaughtEPIPEthat crashed the test process. The goal is to classify a server that closes stdin as a failed candidate, matching the documented contract at the top of the file (lines 9–12): handshake/transport failures are{ok:false}candidates fed into the next generation shot, never p - Assessment: Good on its merits. It reuses the existing settlement machinery (
settle,failCandidate,withStderr) rather than introducing a parallel path, and preserves the deliberate failed-candidate vs setup-fault distinction. The 2-file scope (source + one regression) is proportional to the one-line behavioral gap. Verification in the PR body (targeted + full suite + lint/typecheck/build + clean merge- - Better / existing approach: None materially better. Searched src/ for stdin error/write patterns: the only other
stdin.writeis src/runtime/supervise/runtime.ts:791, an unrelated supervisor path with no shared stdin-error utility to reuse. No existing helper centralizes child-process pipe failure, so there is nothing to fold into. ThefailStdinextraction is slightly heavy for two call sites but keeps the sync and async - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound
A minimal, in-grain robustness fix that routes asynchronously-arriving stdin pipe errors through the existing single-settlement path, preventing process crashes on the already-integrated MCP boot-and-probe verifier.
- Integration: Fully reachable and in active use.
mcpServeVerifieris exported from src/improvement/index.ts:35, called from the production build path at src/lifecycle/tool-build.ts:136 (buildVerifier()→mcpServeVerifier(opts.mcp)), exercised end-to-end in tests/build-composition.test.ts:102, and catalogued in docs/api/primitive-catalog.md:60. The PR touches only an internal error path of an already-wired - Fit with existing patterns: Matches the established pattern exactly. The new
child.stdin.on('error', failStdin)(src/improvement/mcp-serve-verifier.ts:106) feeds the samefailCandidate→settlesingle-settlement path (lines 62-74) already used by the synchronous EPIPE handler insend()(lines 84-85) and the exit handler (line 103). The siblingcommandVerifier(agentic-generator.ts:247) usesspawnSyncand so is st - Real-world viability: Correct under realistic conditions. Node stream writes can emit
'error'asynchronously after a synchronouswrite()succeeds; without the listener at line 106 that event would be unhandled and crash the test/runner process. Becausesettle()guards on thesettledflag (line 57), racing sync and async error emissions remain idempotent — exactly one outcome wins. The deterministic regression t - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 failStdin helper is a small abstraction for two call sites [maintenance] ``
Extracting the message into
failStdin(mcp-serve-verifier.ts:76) is defensible since it guarantees identical feedback and routing across the sync catch (line 85) and async event (line 106), but it is marginal. Not gating — leave as-is.
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
Summary
Reproduction
Before the fix, the new regression produced one failed test plus one uncaught
EPIPE.Verification
origin/main