Skip to content

fix/issue 5928#5990

Closed
nghetienhiep wants to merge 1 commit into
JSONbored:mainfrom
nghetienhiep:fix/issue-5928
Closed

fix/issue 5928#5990
nghetienhiep wants to merge 1 commit into
JSONbored:mainfrom
nghetienhiep:fix/issue-5928

Conversation

@nghetienhiep

@nghetienhiep nghetienhiep commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

@nghetienhiep
fix(mcp): make loopover-mcp CLI failures respect --json
2927553 
loopover-mcp had no equivalent of @loopover/miner's cli-error.js
contract, so any thrown error escaped the top-level dispatch as a raw
Node stack trace on stderr, ignoring --json entirely. Wrap the runCli
call in a try/catch and mirror the miner's reportCliFailure shape:
--json failures print { ok: false, error } on stdout, otherwise the
existing plain-text stderr message is unchanged.

Closes JSONbored#5928
@nghetienhiep
nghetienhiep requested a review from JSONbored as a code owner July 15, 2026 06:00
@superagent-security

superagent-security Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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

@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 •
edited
Loading

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-15 06:07:13 UTC

3 files · 1 AI reviewer · 1 blocker · readiness 95/100 · CI green · clean

🛑 Suggested Action - Reject/Close

Review summary
This PR wraps the CLI's top-level `runCli` call in a try/catch so a thrown error is converted to a `{ ok: false, error }` JSON object on stdout when `--json` is set, or a plain message on stderr otherwise, instead of leaking a raw Node stack trace — directly addressing issue #5928. The test harness is extended with `runExpectingFailure` to assert on exit code and both streams without throwing, and the new test exercises three real failure paths (unknown command, missing login, unsupported agent profile) plus the unchanged plain-text path. The change is narrow, correctly wired, and well covered.

Nits — 3 non-blocking
  • `reportCliFailure`/`console.error(message)` now surfaces only `error.message` for non-JSON failures, discarding the stack trace that previously escaped uncaught — intentional per the PR's stated contract, but worth confirming no callers relied on stack traces for debugging (packages/loopover-mcp/bin/loopover-mcp.js:591).
  • `argsWantJson` uses `arg?.startsWith(...)` even though `args` is typed as `string[]`, so the optional chaining is dead code (packages/loopover-mcp/bin/loopover-mcp.js:600).
  • Consider asserting `unknownCommand.stderr === ""` is checked for all three JSON-mode assertions in the new test (only done for the first), for symmetry with the plain-failure assertion at the end (test/unit/mcp-cli-basics.test.ts:165-183).

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent 
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 56 registered-repo PR(s), 28 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor nghetienhiep; Gittensor profile; 56 PR(s), 0 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Review context
  • Author: nghetienhiep
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Vue, C#, JavaScript, Python, C, C++, Go, Objective-C
  • Official Gittensor activity: 56 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (No linked issue detected). 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 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JSONbored JSONbored Awaiting requested review from JSONbored JSONbored is a code owner

Assignees

@nghetienhiep nghetienhiep

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nghetienhiep