Skip to content

fix(mcp): shape a remote JSON-RPC error into a tool result in the gateway proxy - #10216

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10036
Jul 31, 2026
Merged

fix(mcp): shape a remote JSON-RPC error into a tool result in the gateway proxy#10216
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10036

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(mcp): shape a remote JSON-RPC error into a tool result in the gateway proxy

registerProxiedTool's handler returned result ?? payload verbatim. The remote
runs with enableJsonResponse, so a request-level failure (an unknown tool, bad
arguments) still arrives as HTTP 200 with { jsonrpc, id, error } and no
result key -- apiPost only throws on a non-2xx status, so that raw envelope
reached the caller as if it were the tool's own answer: no content, no
isError, not a CallToolResult at all. wrapStdioToolHandler's
ok = result?.isError !== true then read every one of those as a success,
since the envelope carries no isError of its own.

The handler now inspects what came back: a result is still returned
unwrapped and unchanged, but an error (or a payload carrying neither) is
shaped into a conformant isError:true result with a closed-set
{ error: { code, message } } envelope, classifying the remote's message
through resolveErrorCode rather than surfacing its numeric JSON-RPC code as
the telemetry error_code.

Closes #10036

…eway proxy

registerProxiedTool's handler returned `result ?? payload` verbatim. The remote
runs with enableJsonResponse, so a request-level failure (an unknown tool, bad
arguments) still arrives as HTTP 200 with `{ jsonrpc, id, error }` and no
`result` key -- apiPost only throws on a non-2xx status, so that raw envelope
reached the caller as if it were the tool's own answer: no content, no
isError, not a CallToolResult at all. wrapStdioToolHandler's
`ok = result?.isError !== true` then read every one of those as a success,
since the envelope carries no isError of its own.

The handler now inspects what came back: a `result` is still returned
unwrapped and unchanged, but an `error` (or a payload carrying neither) is
shaped into a conformant isError:true result with a closed-set
{ error: { code, message } } envelope, classifying the remote's message
through resolveErrorCode rather than surfacing its numeric JSON-RPC code as
the telemetry error_code.
@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 31, 2026 12:50
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 13:02:34 UTC

5 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The diff fixes registerProxiedTool's handler so a remote JSON-RPC error envelope (arriving as HTTP 200 due to enableJsonResponse) is shaped into a proper isError:true CallToolResult with a closed-set {error:{code,message}} envelope via resolveErrorCode, rather than leaking the raw {jsonrpc,id,error} object as a fake success. The change is well-targeted, correctly traced (wrapStdioToolHandler's `ok = result?.isError !== true` genuinely misread these envelopes as success), and is backed by both a new dedicated regression test file exercising the real in-process handler and updates to two existing test files that previously asserted the old (buggy) 'hand back the whole payload' behavior as intended. The `loopover-mcp.ts` file is large, but that predates this diff and the change itself is a small, surgical addition.

Nits — 3 non-blocking
  • packages/loopover-mcp/bin/loopover-mcp.ts: the fallback message for a payload with neither result nor error is a hardcoded string; consider whether this case should also log/warn since it indicates a genuinely unexpected remote response shape.
  • test/unit/mcp-gateway-proxy.test.ts duplicates a decent amount of setup already covered by test/unit/mcp-gateway-mount-inprocess.test.ts's resultless-envelope test; not wrong, but worth a comment noting why both exist (already explained in a comment, so this is minor).
  • Consider asserting in the new proxy test that the `content[0].text` for the resultless-envelope case matches the exact fallback string, to lock in that wording as intentional rather than incidental.

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 #10036
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 ⚠️ 20/25 Preflight is ready, but the PR body does not name the validation run.
Contributor workload ✅ 10/10 Author activity: 130 registered-repo PR(s), 92 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 130 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The diff modifies registerProxiedTool's handler to detect a missing result (error or neither key) and shapes it into a conformant isError:true CallToolResult with structuredContent.error.code resolved via resolveErrorCode, leaving the success path and wrapStdioToolHandler chokepoint untouched as required, and adds tests covering the error, resultless, and success cases plus telemetry classificatio

Review context
  • Author: bitfathers94
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 130 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add validation command/output.
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://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

@superagent-security

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 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.09%. Comparing base (cc9d102) to head (46e8295).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10216      +/-   ##
==========================================
- Coverage   80.47%   80.09%   -0.38%     
==========================================
  Files         282      283       +1     
  Lines       58844    61056    +2212     
  Branches     6977     7804     +827     
==========================================
+ Hits        47354    48905    +1551     
- Misses      11199    11521     +322     
- Partials      291      630     +339     
Flag Coverage Δ
backend 70.11% <100.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 70.11% <100.00%> (ø)

@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 fa9789e into JSONbored:main Jul 31, 2026
8 checks passed
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.

mcp(gateway): translate a remote JSON-RPC error into a tool error instead of returning the envelope

1 participant