Skip to content

fix(mcp): negotiate MCP 2025-11-25 so clients see invalid-argument errors - #18490

Open
mfazekas wants to merge 5 commits into
pingdotgg:mainfrom
mfazekas:fix/mcp-protocol-2025-11-25
Open

mfazekas wants to merge 5 commits into
pingdotgg:mainfrom
mfazekas:fix/mcp-protocol-2025-11-25

Conversation

@mfazekas

@mfazekas mfazekas commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

MCP clients requesting protocol 2025-11-25 are negotiated down to 2025-06-18, where invalid tool arguments return a JSON-RPC error that some clients show without the validation details.

Offer the supported 2025-11-25 adapter alongside 2025-06-18. Newer clients receive the validation message as an isError tool result; clients requesting 2025-06-18 keep its existing -32602 response. The first offered version also becomes the fallback for unsupported initialize versions. Tool definitions, handlers, authentication, and access controls are unchanged.

Based on the original fix by @mfazekas in #18490. This replaces the approach in #18098; 2026-07-28 server/discover support in #16067 remains outside scope.

Scope and approval

This is a focused fix to the pinned MCP protocol list, using the newer adapter already shipped by Effect while retaining the legacy adapter. Maintainer takeover and publication were explicitly authorized in this campaign; the takeover notice records ownership. That authorization does not approve landing. Macroscope's Needs Human Approval verdict, represented by the NEUTRAL Approvability check on the previous head, remains a human landing gate. It has not been waived or satisfied by the takeover or the independent code reviews.

Verification

The reviewed head b3e239b2ad preserves the contributor's commits and merges main at 62144bf0f8. Recorded checks on that head:

  • All 27 McpHttpServer.test.ts HTTP tests passed.
  • Server typecheck and lint of the two changed files passed.
  • One added HTTP regression is retained from the original one-test contribution. It checks negotiation and invalid-argument responses for both versions through the real transport. Before the main merge, temporarily removing either protocol failed its corresponding negotiation assertion; both probes were restored, and the full 27-test suite passed after integration.
  • Fresh independent full-diff reviews by Opus and Sol found no code defects. Integration was checked against the current toolkit registration, analytics wrapper, tool access layer, and session-registry interface.

No UI change or screenshots are required. No live Claude client session was run during this takeover. These are local verification results; GitHub CI and configured reviews must be checked for the published head before landing. Historical checks on the previous head do not certify this head.

Taken over with Claude Opus 5.5 (tests and review), Claude Fable 5.1 (implementation), and GPT-6.1 Sol (review) through T3 Code; publication coordinated with GPT-6.1 Sol in the Codex harness.

…rors

T3 offered only MCP 2025-06-18, so every client was held to it even when
it asked for 2025-11-25, as the Claude app and Claude Code both do. Under
2025-06-18 a tool call with invalid arguments fails as a JSON-RPC
InvalidParams error, which the Claude app shows only as a generic "tool
call failed", hiding the validation message from the user and the model.
Effect already reports these as isError tool results for 2025-11-25, so
offer that version and keep 2025-06-18 for older clients.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 12, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The MCP transport now enables 2025-11-25 by default, changing protocol negotiation and the user-visible handling of invalid tool arguments while preserving the legacy version. The focused regression test covers both paths, but the default production behavior change requires human review.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 12, 2026 — with ChatGPT Codex Connector
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 70d79cb0-ccda-4001-8379-98b726f87909
📥 Commits

Reviewing files that changed from the base of the PR and between e688034 and 723d587.

📒 Files selected for processing (2)
  • apps/server/src/mcp/McpHttpServer.test.ts
  • apps/server/src/mcp/McpHttpServer.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough
📝 Walkthrough

Walkthrough

The HTTP MCP transport now advertises protocol versions 2025-11-25 and 2025-06-18. A test checks the invalid-argument response for each version.

Changes

MCP protocol compatibility

Layer / File(s) Summary
Protocol support and invalid-argument responses
apps/server/src/mcp/McpHttpServer.ts, apps/server/src/mcp/McpHttpServer.test.ts
The transport advertises both protocol versions. The test checks for an error-marked tool result with an invalid-parameters message for 2025-11-25, and JSON-RPC error code -32602 for 2025-06-18.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 723d5

The transport retains the older protocol and supports the newer version’s validation-message response. No actionable merge-blocking risk is established; normal checks remain appropriate.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 723d5

The newer protocol preserves legacy support and uses the existing authentication and tool-access controls. No introduced security weakness was identified. Compatibility for clients requesting other protocol versions remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed behavior is reachable by callers of the existing authenticated MCP endpoint. The examined request path retains credential-derived environment, caller and capability scope; no new unauthenticated entrypoint or broader handler authority was identified.

Trust Boundaries and Controls

  • observed — The HTTP middleware resolves the bearer credential through the session registry or client authenticator and rejects unresolved credentials before running the transport. It supplies invocation scope per request. Toolkit registration continues to use declared tool-access layers without capturing a registration-time caller scope.
  • observed — The newer failure conversion uses the same message fields that the legacy JSON-RPC mapper already returned. Toolkit registration also retains its existing handling that logs and scrubs unexpected internal failures. This is a client-visible representation change, not evidence that new internal error detail is exposed.

Resilience and Maintainability Implications

  • inferred — The lifecycle inspection found shared session binding, request tracking, cancellation and termination machinery for both adapters. Selecting the newer adapter does not introduce a separate ownership or cleanup transition; session termination continues to interrupt requests bound to that session. This conclusion is source-based, not runtime concurrency verification.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: negotiating MCP protocol version 2025-11-25 so clients receive invalid-argument errors with validation details.
Description check Passed The description covers the problem, change, scope and approval context, verification results, limitations, and tooling used. It explains the behavior for both supported protocol versions and identifie…
Full details: Description check

Explanation

The description covers the problem, change, scope and approval context, verification results, limitations, and tooling used. It explains the behavior for both supported protocol versions and identifies the outstanding requirement to check GitHub CI and configured reviews before landing.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants