Ensure _meta is available on all requests (#1284) #1520
Conversation
7e7f47a to
a0bfacc
Compare
There was a problem hiding this comment.
Per the comments in #1284 I believe it was intended that progressToken and _meta be available on all Request types. I'm certainly open to other approaches / names / naming conventions, but believe this matches the original intent.
| [key: string]: unknown; | ||
| }; | ||
| progressToken?: ProgressToken; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If I wanted to extend MCP with an unofficial RPC method, I think I would need to introduce some new
methoddefinition which would have aparamsdefinition of its own. As part of that definition, I would define theparamsit takes similar to how the spec does, and my definition wouldextend RequestParamswith 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 mannercase be allowed for thetools/listmethod?
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
6cb6289 to
da25740
Compare
| [key: string]: unknown; | ||
| }; | ||
| progressToken?: ProgressToken; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
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.
da25740 to
72f9f1d
Compare
|
@jonathanhefner thanks again for reviewing! For easy diff visibility, all changes are in a 2nd commit. Please see my comment about my concerns/understanding regarding arbitrary params & unofficial RPC methods.
|
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
Adds definitions for
params?on allRequests that avoid overwriting extended interfaces.Motivation and Context
In #1284, it's suggested to use
_metaontools/list& other requests, but this isn't currently possible.How Has This Been Tested?
Defined a sample ListToolsRequest to ensure that both
_metaandcursorwere supported params:Breaking Changes
Possibly yes:
[key: string]: unknown;was part of theparamsdefinition for any type which didn't override the definition fromRequestorNotification. If support for arbitrary params was used, this will no longer be allowed.This is potentially breaking for the following types:
PingRequestListRootsRequestInitializedNotificationResourceListChangedNotificationPromptListChangedNotificationToolListChangedNotificationRootsListChangedNotificationPlease see further discussion here.
Types of changes
Checklist
Additional context