Skip to content

Fix: Fix JSONRPC Types #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

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 as a code owner July 22, 2025 07:20
@cliffhall
Copy link
Member

LGTM! 👍

/**
* The error type that occurred.
*/
code: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a type in this file that defines the standard error codes? They're only defined in the reference documentation I linked for some reason.

ErrorCode | number just reduces to number anyways, but this information should still be in the actual schema somewhere, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LucaButBoring that is a good idea. however, i'd prefer to keep this PR separate, so it has a single purpose, since it's kind of non-trivial anyway. Would be a good follow-up though!

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.

3 participants