Skip to content

fix(improvement): handle MCP verifier stdin pipe errors#508

Merged
drewstone merged 2 commits into
mainfrom
fix/mcp-verifier-epipe
Jul 11, 2026
Merged

fix(improvement): handle MCP verifier stdin pipe errors#508
drewstone merged 2 commits into
mainfrom
fix/mcp-verifier-epipe

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • handle asynchronous child stdin failures through the existing single-settlement path
  • classify a server that closes stdin as a failed candidate instead of crashing the test process
  • add a deterministic closed-pipe regression

Reproduction

Before the fix, the new regression produced one failed test plus one uncaught EPIPE.

Verification

  • targeted verifier tests: 6/6 passed
  • full suite: 1,317 passed, 2 skipped across 134 files
  • lint: 387 files passed
  • typecheck passed
  • build passed
  • clean merge-tree against current origin/main

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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: mcpServeVerifier is the intrinsic verifier wired into buildVerifier at src/lifecycle/tool-build.ts:136 for opts.kind === 'mcp', and re-exported from src/improvement/index.ts:35. The new child.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 through failCandidate -> 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 the settled flag, so a synchronous send() 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.

value-audit · 20260711T060438Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 3a66effd

Review health 100/100 · Reviewer score 89/100 · Confidence 70/100 · 3 findings (3 low)

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 synchronous send() catch (line 83) and the new async child.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 shared const 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
tangletools previously approved these changes Jul 11, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 shared failStdin helper (line 76) used by both the new async event and the existing synchronous try/catch in send() (line 85). Both now resolve {ok:false, feedback} via the same settle guard as exit/timeout/handshake failures. Adds a closed-stdin test varian
  • Goals it achieves: Pipe failures normally arrive asynchronously, outside send()'s try/catch — before this they surfaced as an uncaught EPIPE that 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.write is 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. The failStdin extraction 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. mcpServeVerifier is 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 same failCandidatesettle single-settlement path (lines 62-74) already used by the synchronous EPIPE handler in send() (lines 84-85) and the exit handler (line 103). The sibling commandVerifier (agentic-generator.ts:247) uses spawnSync and so is st
  • Real-world viability: Correct under realistic conditions. Node stream writes can emit 'error' asynchronously after a synchronous write() succeeds; without the listener at line 106 that event would be unhandled and crash the test/runner process. Because settle() guards on the settled flag (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.

value-audit · 20260711T062002Z

@drewstone drewstone merged commit 906e24c into main Jul 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants