Skip to content

fix(mcp): exit cleanly on stdout/stderr EPIPE instead of crashing - #8730

Closed
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/mcp-epipe-listeners-8691
Closed

fix(mcp): exit cleanly on stdout/stderr EPIPE instead of crashing#8730
joaovictor91123 wants to merge 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/mcp-epipe-listeners-8691

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

  • Install stdout/stderr error listeners so a broken pipe (EPIPE / destroyed stream) exits with a normal exit code instead of an uncaught exception crash.
  • Export installStdioErrorHandlers for branch-complete unit coverage (codecov/patch), plus a real subprocess early-close pipe test and a normal version --json regression.

Closes #8691

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Skipped monorepo-wide UI/workers/actionlint/audit checks: MCP CLI stdio error-handler only. Unit-tested handler paths for codecov/patch; subprocess EPIPE + normal-output regression included. Full MCP build/test runs in CI.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — CLI-only change.

State / title JPG/PNG evidence
N/A

Notes

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.27%. Comparing base (abf88ab) to head (6e6f3c4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/loopover-mcp/bin/loopover-mcp.ts 60.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #8730       +/-   ##
===========================================
- Coverage   93.79%   83.27%   -10.52%     
===========================================
  Files         797       97      -700     
  Lines       79478    24758    -54720     
  Branches    24079     4744    -19335     
===========================================
- Hits        74543    20618    -53925     
- Misses       3563     3950      +387     
+ Partials     1372      190     -1182     
Flag Coverage Δ
backend 11.06% <60.00%> (-84.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-mcp/bin/loopover-mcp.ts 11.06% <60.00%> (-43.01%) ⬇️

... and 700 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-26 01:19:14 UTC

2 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

Review summary
Adds a small, targeted stdio error handler (installStdioErrorHandlers) that listens for 'error' on stdout/stderr and exits with code 1 instead of letting an EPIPE/ERR_STREAM_DESTROYED crash as an uncaught exception; it is wired in only at real CLI entrypoint via the existing runAsCliEntrypoint guard, matching the pattern the file already uses. The fix is at the right layer (installed once at entrypoint, injectable streams/exit for testability) and the accompanying tests cover the unit-level branches plus two real-subprocess EPIPE scenarios. codecov/patch failed at 60% vs 99% target, which the diff itself doesn't explain — the new unit test does call installStdioErrorHandlers with all three error variants, so the shortfall likely comes from the placement of `if (runAsCliEntrypoint) { installStdioErrorHandlers(); }` or other partially-exercised lines/branches in the diff.

Nits — 5 non-blocking
  • The codecov/patch check FAILED at 60% vs the 99% target required by this repo's contributing guidelines; the PR's own checklist claims local coverage was verified, so this discrepancy should be resolved before merge, though I can't tell from the diff which specific line/branch is uncovered.
  • packages/loopover-mcp/bin/loopover-mcp.ts:1678 — the `if (runAsCliEntrypoint) { installStdioErrorHandlers(); }` call site adds a magic reference to issue fix(mcp): no error listener on stdout/stderr lets a broken pipe crash the CLI with an uncaught EPIPE exception #8691 in a comment but no named constant; this is fine as-is since it's just a comment reference, not a real magic number.
  • The exit code is hardcoded to `1` for every write-failure case (EPIPE, ERR_STREAM_DESTROYED, etc.) with no differentiation — consider whether callers relying on distinct exit codes for different failure classes need this, or confirm 1 is intentionally the catch-all per repo convention.
  • Confirm what specific line(s) codecov/patch is flagging as uncovered (likely branch inside installStdioErrorHandlers or the runAsCliEntrypoint gate itself) and add a targeted unit test rather than relying only on the two subprocess tests.
  • Consider whether `stdout.on('error', ...)` should also detach/remove the listener after exit is called, though since exitProcess normally terminates the process this is likely moot in production.

CI checks failing

  • codecov/patch — 60.00% of diff hit (target 99.00%)

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8691
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 161 registered-repo PR(s), 73 merged, 5 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 161 PR(s), 5 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff installs error listeners on process.stdout/process.stderr (wired at CLI entrypoint) that exit cleanly on EPIPE/stream-destroyed errors, and adds tests covering the handler's exit behavior, a real subprocess broken-pipe scenario, and a normal-output regression test.

Review context
  • Author: joaovictor91123
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, TypeScript, C++, CSS, Rust
  • Official Gittensor activity: 161 PR(s), 5 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch)). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(mcp): no error listener on stdout/stderr lets a broken pipe crash the CLI with an uncaught EPIPE exception

1 participant