Skip to content

fix(mcp): make loopover-mcp CLI failures respect --json - #6113

Merged
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-mcp-json-errors-5928-v2
Jul 15, 2026
Merged

fix(mcp): make loopover-mcp CLI failures respect --json#6113
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-mcp-json-errors-5928-v2

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Add packages/loopover-mcp/lib/cli-error.js mirroring @loopover/miner's { ok: false, error } contract
  • Wrap top-level runCli dispatch in try/catch so thrown errors never escape as raw Node stack traces
  • Extend mcp-cli harness with runExpectingFailure and tests for unknown command, missing --login, and unsupported --agent-profile failures with --json
  • Allow lib/cli-error.js in MCP package dry-run check

Closes #5928

Supersedes #6104 (fork PR head stuck on prior SHA).

Test plan

  • npx vitest run test/unit/mcp-cli-basics.test.ts (21/21 pass)
  • npx vitest run test/unit/mcp-cli- (all pass on Linux CI)

RealDiligent and others added 2 commits July 15, 2026 17:34
Wrap top-level runCli dispatch in try/catch and mirror @loopover/miner's
cli-error contract so --json failures emit { ok: false, error } on stdout
instead of a raw Node stack trace.

Closes JSONbored#5928

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 15, 2026 10:00
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.24%. Comparing base (3312431) to head (5027cbc).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6113   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files         595      595           
  Lines       47046    47046           
  Branches    15015    15015           
=======================================
  Hits        44808    44808           
  Misses       1493     1493           
  Partials      745      745           
Flag Coverage Δ
shard-1 44.01% <ø> (ø)
shard-2 36.62% <ø> (ø)
shard-3 32.06% <ø> (ø)
shard-4 33.73% <ø> (+<0.01%) ⬆️
shard-5 31.65% <ø> (ø)
shard-6 44.50% <ø> (ø)

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

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

loopover-orb Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-15 10:12:49 UTC

6 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds a shared `reportCliFailure`/`argsWantJson`/`describeCliError` helper and wraps the top-level `runCli` dispatch in try/catch so uncaught throws in the MCP CLI produce the same `{ ok: false, error }` JSON contract (or plain stderr text) that @​loopover/miner uses, instead of a raw Node stack trace. The `argsWantJson` check operates on raw argv before any parsing, which is correct since a thrown error can occur before parsed options exist. The harness and package-allowlist/build-script updates are consistent with the new file, and the new test exercises the unknown-command, missing-`--login`, and unsupported-`--agent-profile` failure paths for both `--json` and plain-text output, matching the stated intent and issue #5928.

Nits — 5 non-blocking
  • cli-error.js:9 reportCliFailure defaults exitCode to 2 but the only call site in bin/loopover-mcp.js:596 passes 1 explicitly, so the default is dead code — either drop the default or use it consistently with other error-path exit codes in the CLI.
  • mcp-cli-basics.test.ts:165-183 doesn't assert the actual numeric exit code (only `not.toBe(0)`), so a regression that changes the exit code from 1 to some other non-zero value would slip through silently.
  • mcp-cli-harness.ts:29-30 the comment explains why stdout is folded into the error message, but the same logic is now duplicated across `run` and `runAsync`/`runExpectingFailure` — consider a small shared helper to normalize failure output once.
  • Add a coverage assertion for the real exit code (`unknownCommand.status`) to lock in the `1` used by reportCliFailure's call site.
  • Consider exporting a shared 'foldFailureStreams' helper used by run/runAsync/runExpectingFailure to avoid the near-duplicated stdout+stderr concatenation logic in mcp-cli-harness.ts.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #5928
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High 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: 352 registered-repo PR(s), 170 merged, 29 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 352 PR(s), 29 issue(s).
Gate result ✅ Passing No configured blocker found.
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds a cli-error.js module mirroring the miner's { ok: false, error } contract, wraps the top-level runCli dispatch in try/catch calling reportCliFailure with argsWantJson/describeCliError, and adds new tests via runExpectingFailure covering unknown command, missing --login, and unsupported --agent-profile with --json plus a non-json regression check.

Review context
Contributor next steps
  • Start here: Review top overlaps.
  • Then work through the remaining 2 steps in the Signals table above.
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://gittensory.aethereal.dev/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 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.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit cf1e22c into JSONbored:main Jul 15, 2026
16 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 15, 2026
12 tasks
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): loopover-mcp CLI errors never respect --json, unlike @loopover/miner's cli-error contract

1 participant