Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Fix: Fix JSONRPC Types#1026

Merged
dsp-ant merged 5 commits intomodelcontextprotocol:mainmodelcontextprotocol/modelcontextprotocol:mainfrom
hesreallyhim-forks:fix/fix-JSONRPC-typeshesreallyhim-forks/modelcontextprotocol:fix/fix-JSONRPC-typesCopy head branch name to clipboard
Aug 28, 2025
Merged

Fix: Fix JSONRPC Types#1026
dsp-ant merged 5 commits intomodelcontextprotocol:mainmodelcontextprotocol/modelcontextprotocol:mainfrom
hesreallyhim-forks:fix/fix-JSONRPC-typeshesreallyhim-forks/modelcontextprotocol:fix/fix-JSONRPC-typesCopy head branch name to clipboard

Conversation

@hesreallyhim
Copy link
Contributor

In the schema, JSONRPC{Request,Response,Notification,Error} generally extends {Request,Response,Notification,Error}. But then nothing actually consumes the JSONRPCX interface - Request sub-types extend Request; Response sub-types extend Response; etc. In this PR, I fix this confusion by having the sub-types extend the JSONRPC types.

Motivation and Context

JSONRPCX extends X.
SpecificSubtypeX extends X.
Therefore:
JSONRPCX and SpecificSubtypeX are siblings in the type hierarchy. But intuitively, the JSONRPC types are supposed to be the base/foundation. One expects: JSONRPC-SUPER-TYPE -> TYPE -> SUB-TYPE. Maybe there is a reallllly good reason for this.

Concrete example:

export interface Request {
  method: string;
  params?: {
    ... 
  };
}
export interface JSONRPCRequest extends Request {
  jsonrpc: typeof JSONRPC_VERSION;
  id: RequestId;
}

(So far, so good....)

export interface InitializeRequest extends Request {
  method: "initialize";
  params: {
    ...
  };
}

OK, let's gooooo:

const req: InitializeRequest = {
  method: "initialize"; ✅
  params: {
    ...
  }; ✅
  jsonrpc: "2.0"; ❌ 
  id: 1 ❌  
}

How Has This Been Tested?

There are no tests of this kind in this repo - I am happy to write tests in whatever style is preferred. The assumption was that tsc would catch type errors. But this isn't a type/compilation error - the schema is valid, it just implies that an InitializationRequest is not a JSON-RPC 2.0 Request.

If you want to see for yourself, you can paste the existing schema into a IDE/playground (with type-checking), and then observe what happens with these statements:

type _SchemaTest = InitializeRequest extends JSONRPCRequest ? true : never;

Also, Claude was nice enough to provide a little snippet:
interface-compatibility-test.txt

Breaking Changes

As far as I can tell, basically no. It seems to me that each SDK declares the types themselves. I searched on GitHub and couldn't find one instance of anyone importing from this repo directly. So in that sense - no.

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 - I think some docs will need updating/correcting

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

I will complete the items on the checklist once I get confirmation that this analysis is correct.

Additional context

We probably don't need Request if my analysis is correct. Result is OK because it's a property of a JSONRPCResult. Also I pulled out "Error" into its own interface because it was stylistically aberrant.

@hesreallyhim hesreallyhim requested a review from a team July 22, 2025 07:20
@cliffhall
Copy link
Member

LGTM! 👍

schema/draft/schema.ts Show resolved Hide resolved
dsp-ant
dsp-ant previously approved these changes Aug 26, 2025
Copy link
Member

@dsp-ant dsp-ant left a comment

Choose a reason for hiding this comment

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

I think this is fine.

@hesreallyhim
Copy link
Contributor Author

thanks @dsp-ant I merged in main and there was a conflict in the mdx but not in the schema.ts so i just ran generate again, but it means it dismissed your review. Cheers!

@hesreallyhim hesreallyhim requested a review from dsp-ant August 26, 2025 15:39
@dsp-ant dsp-ant merged commit a470342 into modelcontextprotocol:main Aug 28, 2025
2 checks passed
cliffhall added a commit to cliffhall/mcp-typescript-sdk that referenced this pull request Aug 29, 2025
  - 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
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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.