Skip to content

Conversation

cliffhall
Copy link
Member

@cliffhall cliffhall commented Aug 29, 2025

Description

  • In spec.types.test.ts
    • Added two helper types WithJSONRPC<T> and WithJSONRPCRequest<T>
    • in test function arguments for notifications, make sdk argument type wrapped by WithJSONRPC
    • in test function arguments for requests, make sdk argument type wrapped by WithJSONRPCRequest
    • Add Error to MISSING_SDK_TYPES array (inner error object of a JSONRPCError)
    • Adjusted specTypes length check to match current actual length (92)

Motivation and Context

Tl;dr

A recent set of changes to the schema.ts file in the specification repo has broken a test in this repo which checks that our types all match the spec. This causes CI to fail and no further PRs can be merged until this is fixed.

The core issue stemmed from a deliberate design choice in the SDK: internal types for requests and notifications omit the jsonrpc and id properties for cleaner internal handling, while the specification-generated types define the full on-the-wire format, which includes them.

This PR fixes that by addressing the mismatch with two helper types that wrap SDK types before comparing them to spec types.

Here's what happened:

  • I noticed that when the author of Typescript SDK PR 819 merged main into his PR, the tests failed when they were not failing previously.
  • I saw a ton of errors in the test file: src/spec.types.test.ts similar to:
image
  • I looked at that test file and it hasn't changed in a month. However, I see that it is fetching the actual spec.types.ts file from the specification repo prior to the tests.
image
  • Now certain that the problem is with the src/spec.types.tst.ts file, I replaced my version of it from local history removed the npm run fetch:spec-types from the test npm script and ran it. All passed.
image

ok so this is a little nuanced - the type change from the mcp types is breaking the tests but is not affecting the behavior of the library at all because internally the messages are treated as missing the jsonrpc field, and then it gets added when the message gets sent over the wire. so basically the internal types are not JSONRPC messages. but that wasn't being enforced or caught before because the MCP schema had the same defect.

i think another way to put it might be that the Protocol class handles the obligatory JSONRPC fields like jsonrpc (kind of wrapper or "framing"), but then the sepc.types.test is comparing the non-wrapped internal types, which are no actually typed as JSONRPC messages, but this was not caught until the fix above.

How Has This Been Tested?

All tests now run locally:
Screenshot 2025-08-29 at 11 47 31 AM

This is the first PR since the problem occured that passes CI

  • First successful run was the initial version of the PR which simply omitted the test until a fix could be made.
  • Second successful run is the current version of the PR which actually fixes the tests
Screenshot 2025-08-29 at 11 57 30 AM

Breaking Changes

Nope.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

  - Omit spec.types.test.ts for now unti we can figure out how to make it work.
  - Changes in the spec file are causing it to fail vis-à-vis JSONRPCNotification vs Notification
  - see modelcontextprotocol/modelcontextprotocol#1026
@cliffhall cliffhall requested a review from a team as a code owner August 29, 2025 14:16
@cliffhall cliffhall requested a review from ihrpr August 29, 2025 14:16
@cliffhall cliffhall marked this pull request as draft August 29, 2025 15:36
  - Remove the previous ommision of spec.types.test.ts
* In spec.types.test.ts
  - Added two types WithJSONRPC<T> and WithJSONRPCRequest<T>
  - in test function arguments for notifications, make sdk argument type wrapped by WithJSONRPC
  - in test function arguments for requests, make sdk argument type wrapped by WithJSONRPCRequest
  - Add 'Error' to MISSING_SDK_TYPES array (inner error object of a JSONRPCError)
  - Adjusted specTypes length check to match current actual length (92)
@cliffhall cliffhall changed the title Temporarily omit test that is breaking CI Fix the SDK vs Spec types test that is breaking CI Aug 29, 2025
@cliffhall cliffhall marked this pull request as ready for review August 29, 2025 15:59
Comment on lines +24 to +29
// Adds the `jsonrpc` property to a type, to match the on-wire format of notifications.
type WithJSONRPC<T> = T & { jsonrpc: "2.0" };

// Adds the `jsonrpc` and `id` properties to a type, to match the on-wire format of requests.
type WithJSONRPCRequest<T> = T & { jsonrpc: "2.0"; id: SDKTypes.RequestId };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable approach matching the other generics here to bridge the gap between SDK types and the JSONRPC types.

@felixweinberger
Copy link
Contributor

Thanks for fixing this!

@felixweinberger felixweinberger merged commit 3dd074f into modelcontextprotocol:main Aug 29, 2025
2 checks passed
@cliffhall cliffhall deleted the omit-spec-type-tests branch August 29, 2025 18:03
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