Skip to content

fix: resolve CLI mcp call protocol issues (BUG-001) - #10960

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.50from
YunyunZhai:fix/mcp-cli-call-protocol
Aug 23, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.50from
YunyunZhai:fix/mcp-cli-call-protocol

Conversation

@YunyunZhai

@YunyunZhai YunyunZhai commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Fix mcp status showing 'stopped' when server is online (check online/running fields)
  • Replace broken /api/mcp/tools/call route with proper JSON-RPC 2.0 protocol
  • Implement mcpJsonRpcCall() using initialize + tools/call via /api/mcp/stream
  • Support both stream and non-stream modes
  • Add Mcp-Session-Id header management

Changes

  • Modified bin/cli/commands/mcp.mjs to use new JSON-RPC client
  • Added bin/cli/mcpClient.mjs with shared MCP client utilities
  • Updated tests with mcpStreamMock helper
  • Fixed ESLint issues in test files

Test Plan

  • omniroute mcp status now correctly shows online status
  • omniroute mcp call <tool> works with JSON-RPC protocol
  • omniroute mcp call <tool> --stream works with SSE streaming
  • All existing tests pass

- Fix mcp status showing 'stopped' when server is online (check online/running fields)
- Replace broken /api/mcp/tools/call route with proper JSON-RPC 2.0 protocol
- Implement mcpJsonRpcCall() using initialize + tools/call via /api/mcp/stream
- Support both stream and non-stream modes
- Add Mcp-Session-Id header management
- Update tests with mcpStreamMock helper
The runSkillsDisable function was still using the old direct API call
to /api/mcp/tools/call instead of the shared mcpClient.mjs module.
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks for this — the JSON-RPC migration is the right fix and the root-cause write-up is accurate: /api/mcp/tools/call was never a real route (only GET /api/mcp/tools exists), so those CLI tool calls 404'd. mcp status now reading online instead of running is also correct (src/app/api/mcp/status/route.ts:34 returns online). Protocol-wise the initialize → Mcp-Session-Id → tools/call flow matches open-sse/mcp-server/httpTransport.ts.

Three things need a fix-in-place before this can merge (all small, happy to help):

  1. [MANDATORY] Compression REST fallback is now dead for the common config. /api/mcp/stream returns 400 when the MCP transport is stdio (the default) and 503 when MCP is disabled (src/app/api/mcp/stream/route.ts:14-31) — not 404/501. mcpCall in bin/cli/commands/compression.mjs:84-96 only falls back on 404/501, so on a default server compression status/configure/engine-set now throws. On base it worked because the old /tools/call POST was a guaranteed 404. I reproduced this with a mock returning 400 — the command threw instead of falling back. Please also fall back on 400/503 (or key off the 'transport is set to' message) and add a test that mocks 400, not 404.

  2. [MANDATORY] The X-MCP-Scopes header is never sent. mcpClient.mjs:50 builds scopeHeader but never passes it to callMcpEndpoint (the two calls at :52/:71 only forward {timeout, stream}), and mcpJsonRpcCall in mcp.mjs:73-86 also ignores opts.scope. So mcp call --scope … now silently sends no scope header (the old path did). That's also a leftover dead variable. Please forward it on the tools/call POST and assert it in a test.

  3. [MANDATORY] The changes to registerMcp delete five subcommands that were working — mcp tools list/info/schema and mcp audit tail/stats (bin/cli/commands/mcp.mjs:24-87 no longer registers them), even though the /api/mcp/tools and /api/mcp/audit/* REST routes still exist. Removing working CLI surface is out of scope for a 'call protocol' fix; please restore the block or explicitly justify the removal in the PR description.

Minor/optional: there are now two copies of the MCP client (mcpJsonRpcCall with raw fetch vs the shared mcpClient.mjs), so mcp call bypasses the CLI-token/remote-context handling every other migrated command gets — consider routing mcp call through the shared client. And the rewritten tests add a lot of new any casts that compound the already-red tests/ lint; the typed makeMcpStreamFetch helper is a good path to keep them clean.

Everything else looks solid — tests pass (23/23 on the touched files) and the server-side protocol check is correct. Thank you.

@diegosouzapw
diegosouzapw merged commit 14d70a7 into cheaper-inference:release/v3.8.50 Aug 23, 2026
6 of 16 checks passed
diegosouzapw added a commit that referenced this pull request Aug 25, 2026
… a masked assert

Second base-red batch from the release pre-flight, measured on the .113 with a
clean npm ci (the devbox tree resolves eslint-plugin-react-hooks 7.1.1 from a
stray pnpm store instead of the lockfile 7.0.1 and reports 925 phantom errors).

Provider count 350 -> 352, one root cause behind three reds. Two providers
landed this cycle (volcengine-agent-plan, volcengine-coding-plan) without
regenerating the artifacts that quote the count:

  - docs/reference/PROVIDER_REFERENCE.md regenerated (gen:provider-reference).
  - README / AGENTS / llm.txt (+42 mirrors) / package.json description / 4 SVG
    diagrams updated, including the section heading AND the anchor that links to
    it, so the link does not break.
  - tests/snapshots/provider/translate-path.json regenerated. The diff is purely
    additive: 46 insertions, 0 deletions, exactly the two new providers.

GLM effort tiers. #11415 added the explicit glm-5.3-max tier and left two
sibling vitest specs pinning the old 16-model inventory and an empty tier list
for it. Aligned to the shipped contract (inventory order matches glmProvider.ts;
glm-5.3-max declares ["max"]).

Test-masking. Four assert reductions surfaced once the deleted-file signal was
resolved. Three are legitimate and are allowlisted with their reasoning:
#11355 inverted the startup-cooldown contract (preserve future quota cooldowns),
#11280 replaced two unrolled hops with a 3-hop loop that asserts MORE, and the
Gemini 3.5 Flash retirement removed the models those capability asserts described.

The fourth was real masking: #10960 rewrote the oneproxy status test to install a
stream mock, immediately overwrite it with a passthrough to the real fetch, and
assert `calls.length >= 0` — always true. Restored to assert what the test name
claims (the JSON-RPC tools/call carries omniroute_oneproxy_stats and its result
reaches the caller), with a scope note that it pins the MCP client contract
rather than the commander wiring.

Also allowlists the Gemini 3.5 Flash test deletion as _deletedWithReplacement
(the model was retired by 2764812; gemini-models-parser.test.ts pins the new
"excluded from the parsed list" contract), and rebaselines bundleSize
8045 -> 8461 with per-entry measurements — every entrypoint stays far below its
absolute budget.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…e#10960)

Validated on the combined 8-PR board: all seven CLI command suites green (combo-suggest, compression, mcp-call, oneproxy, resilience, skills + the shared mcpStreamMock helper) within the board's 88/88, typecheck:core clean, gates within baseline. The CLI mcp call protocol fixes (BUG-001) land with full regression coverage across the affected commands. Thank you @YunyunZhai — and thank you for your patience while this one waited for review.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
… a masked assert

Second base-red batch from the release pre-flight, measured on the .113 with a
clean npm ci (the devbox tree resolves eslint-plugin-react-hooks 7.1.1 from a
stray pnpm store instead of the lockfile 7.0.1 and reports 925 phantom errors).

Provider count 350 -> 352, one root cause behind three reds. Two providers
landed this cycle (volcengine-agent-plan, volcengine-coding-plan) without
regenerating the artifacts that quote the count:

  - docs/reference/PROVIDER_REFERENCE.md regenerated (gen:provider-reference).
  - README / AGENTS / llm.txt (+42 mirrors) / package.json description / 4 SVG
    diagrams updated, including the section heading AND the anchor that links to
    it, so the link does not break.
  - tests/snapshots/provider/translate-path.json regenerated. The diff is purely
    additive: 46 insertions, 0 deletions, exactly the two new providers.

GLM effort tiers. cheaper-inference#11415 added the explicit glm-5.3-max tier and left two
sibling vitest specs pinning the old 16-model inventory and an empty tier list
for it. Aligned to the shipped contract (inventory order matches glmProvider.ts;
glm-5.3-max declares ["max"]).

Test-masking. Four assert reductions surfaced once the deleted-file signal was
resolved. Three are legitimate and are allowlisted with their reasoning:
cheaper-inference#11355 inverted the startup-cooldown contract (preserve future quota cooldowns),
cheaper-inference#11280 replaced two unrolled hops with a 3-hop loop that asserts MORE, and the
Gemini 3.5 Flash retirement removed the models those capability asserts described.

The fourth was real masking: cheaper-inference#10960 rewrote the oneproxy status test to install a
stream mock, immediately overwrite it with a passthrough to the real fetch, and
assert `calls.length >= 0` — always true. Restored to assert what the test name
claims (the JSON-RPC tools/call carries omniroute_oneproxy_stats and its result
reaches the caller), with a scope note that it pins the MCP client contract
rather than the commander wiring.

Also allowlists the Gemini 3.5 Flash test deletion as _deletedWithReplacement
(the model was retired by bc825a1; gemini-models-parser.test.ts pins the new
"excluded from the parsed list" contract), and rebaselines bundleSize
8045 -> 8461 with per-entry measurements — every entrypoint stays far below its
absolute budget.
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