Breaking change: this was previously present both inside _meta and outside it. My assumption was that the intent was to show that other interfaces might add properties to params. In practice, such types overwrote this definition, so it only applied to PingRequest and ListRootsRequest which utilized the base definition.

If desired, this could be added back, but if I've understood the intent properly, I don't think that's actually desired behavior.

I think (read: speculate) the intent was to allow MCP to be extended with unofficial RPC methods that merely have to conform to the Request interface, which supports any properties in params. So I think we should leave this in.

Thanks for the initial review! I'm not 100% sure I follow this though. I'll put this back, but would like to make sure I understand how this delivers on that intent (or not?).

In the "test" object from the description, badKey fails type checking, but introducing arbitrary params means the test object should instead look something like:

const listToolsRequest: ListToolsRequest = {
  id: 'foo',
  jsonrpc: "2.0",
  method: "tools/list",
  params: {
    cursor: 'next',
    arbitraryKey: 'NO typescript error here - the spec allows this. Depending on the server implementation, this request might:\n' +
      '      - Fail - but hopefully not since the spec allows this\n' +
      '      - Succeed, with this param ignored\n' +
      '      - Succeed, with this param being handled in some unofficial manner',
    _meta: {
      any: 'no typescript error here',
      thing: {
        complex: 'object'
      }
    }
  }
}

Is the intent that the Succeed, with this param being handled in some unofficial manner case be allowed for the tools/list method? If so, I misunderstood what you meant by allow MCP to be extended with unofficial RPC methods, but the way I interpreted that was more like:

If I wanted to extend MCP with an unofficial RPC method, I think I would need to introduce some new method definition which would have a params definition of its own. As part of that definition, I would define the params it takes similar to how the spec does, and my definition would extend RequestParams with definitions for the specific params needed. For example, I might want to introduce a tools/call/cancel method, which extends existing tool calling functionality to enable requesting cancellation a prior tool call request.

To support this, I want a params definition that lets me specify the id of the Request that wish to cancel:

export interface CancelCallToolRequestParams extends RequestParams {
  /**
   * The id of the previously sent tool call request which cancellation is being requested for.
   */
  id: RequestId;
}

And then define the cancellation request which uses it:

export interface CancelCallToolRequest extends JSONRPCRequest {
  method: "tools/call/cancel";
  params: CancelCallToolRequestParams;
}

This enables me to send cancellation requests which specify the id of the prior request, along with the standard RequestParams, but not arbitrary params.

Which of these understandings is more in line with the intent behind:

allow MCP to be extended with unofficial RPC methods

If I wanted to extend MCP with an unofficial RPC method, I think I would need to introduce some new method definition which would have a params definition of its own. As part of that definition, I would define the params it takes similar to how the spec does, and my definition would extend RequestParams with definitions for the specific params needed.

My thinking (again, speculation) is that while a custom server / client might validate against a new request type that has its own dedicated params type, the SDK or other intermediary code would still validate against the Request type. So Request.params would need to allow arbitrary key-values in order to support that.

Is the intent that the Succeed, with this param being handled in some unofficial manner case be allowed for the tools/list method?

No, I don't think so. So perhaps I rescind my suggestion about renaming BaseRequestParams, and instead we could have both BaseRequestParams and RequestParams types.

All *RequestParams types would extend BaseRequestParams, which would define _meta. Additionally, the RequestParams type, which would be the type for Request.params, would allow arbitrary key-values. That way, we preserve backward compatibility.

Does that make sense?

IIUC, as described, that will introduce TypeScript errors like:

2430: Interface InitializeRequest incorrectly extends interface JSONRPCRequest
  Types of property params are incompatible.
    Type InitializeRequestParams is not assignable to type RequestParams
      Index signature for type string is missing in type InitializeRequestParams

How about?

/** @internal */
export interface Request {
  method: string;
  params?: RequestParams | { [key: string]: unknown };
}

This allows arbitrary params on Request without imposing that requirement on RequestParams and avoids re-introducing the Base nomenclature.

I've pushed a commit with this proposed change & addressing this same issue with notifications, but if there's an alternate route I should pursue that doesn't cause type errors, I can certainly revisit this.

IIUC, as described, that will introduce TypeScript errors like:

Ah, looks like that's due to microsoft/TypeScript#15300. TL;DR: if we used type aliases instead of interfaces, it would work.

How about?

/** @internal */
export interface Request {
  method: string;
  params?: RequestParams | { [key: string]: unknown };
}

That works, though the JSON schema ends up being:

{
    "Request": {
        "properties": {
            "method": {
                "type": "string"
            },
            "params": {
                "anyOf": [
                    {
                        "$ref": "#/definitions/RequestParams"
                    },
                    {
                        "additionalProperties": {},
                        "type": "object"
                    }
                ]
            }
        },
        "required": [
            "method"
        ],
        "type": "object"
    }
}

Which might be fine?

Another option is simply defining Request as:

/** @internal */
export interface Request {
  method: string;
  params?: { [key: string]: any };
}

Then JSON schema ends up as:

{
    "Request": {
        "properties": {
            "method": {
                "type": "string"
            },
            "params": {
                "additionalProperties": {},
                "type": "object"
            }
        },
        "required": [
            "method"
        ],
        "type": "object"
    }
}

Which feels slightly more accurate (in a nitpicky way).

I generally prefer unknown to any, but since we are contending with TypeScript's design decision more so than type safety, I'm inclined to just go with any. Thoughts?

Done!

E: I will leave this convo unresolved for visibility in future reviews, since it concerns the behavior called out as breaking in the description.

I think (read: speculate) the intent was to allow MCP to be extended with unofficial RPC methods that merely have to conform to the Request interface, which supports any properties in params. So I think we should leave this in.

Per @jonathanhefner 's comment here, and my response, after updating Request and Notification to allow extension using params?: { [key: string]: any };, the 7 models listed as having "breaking" changes in the description weren't actually experiencing any breaking changes.

If getting this merged is reliant on backing out the breaking change, I can do that, but in the meantime I've updated the PR to actually make that breaking change again, because I don't believe the behavior it breaks is intended.

Without this breaking change, the following is a valid PingRequest:

const pingRequest: PingRequest = {
  id: "foo",
  jsonrpc: "2.0",
  method: "ping",
  params: {
    _meta: {
      "anything": ["in", "here"]
    },
    "but also": "anything",
    "in": {
      "here": "too",
    }
  },
}

Depending on how the core maintainers lean, it might be better to make a separate PR based on top of this PR, and move the last commit over to the new PR. (That way, the fix for those 7 types can be evaluated separately.)

😞

In the interest of not blocking an accept+merge of this PR in the event that the breaking change is unacceptable to the core maintainers, I have opened #1692 with the breaking change and removed the commit from this PR. The description has been updated to reflect that there are no longer any breaking changes in this PR.

While the GHA checks are currently failing, I believe this error:

The job was not started because your account is locked due to a billing issue.

is concerning modelcontextprotocol, and not my account. I've opened a PR that targets the main branch in my account to demonstrate that the workflows are passing: ccreighton-apptio#2

E: rebased since GHA workflows seem to be happier now