-
Notifications
You must be signed in to change notification settings - Fork 1.4k
SEP-1319: Decouple Request Payloads, Remove passthrough iteration, Typecheck fixes #1086
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
SEP-1319: Decouple Request Payloads, Remove passthrough iteration, Typecheck fixes #1086
Conversation
| protected assertRequestHandlerCapability(method: string): void { | ||
| switch (method) { | ||
| case 'sampling/createMessage': | ||
| if (!this._capabilities.sampling) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Servers do not have sampling capabilities, thus removed. Swapped with completions capabilities.
| street: { type: 'string' }, | ||
| city: { type: 'string' }, | ||
| // @ts-expect-error - pattern is not a valid property by MCP spec, however it is making use of the Ajv validator | ||
| zipCode: { type: 'string', pattern: '^[0-9]{5}$' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patterns not supported by the spec. Passing them will work, but the TS error needs ignoring.
|
|
||
| // Create first client | ||
| const client1 = new Client({ | ||
| id: clientId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No such property.
| type: 'string', | ||
| title: 'Start Time', | ||
| description: 'Event start time (HH:MM)', | ||
| pattern: '^([0-1]?[0-9]|2[0-3]):[0-5][0-9]$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern could be left here, but needs ts-ignore, as not supported by the spec. We could allow it on an optional basis and still match the spec if we wanted.
| switch (request.params.ref.type) { | ||
| case 'ref/prompt': | ||
| assertCompleteRequestPrompt(request); | ||
| return this.handlePromptCompletion(request, request.params.ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When passthrough removed, assertions are needed to proceed with a narrowed type to alleviate type errors downstream.
| { | ||
| name: 'new client', | ||
| version: '1.0', | ||
| protocolVersion: LATEST_PROTOCOL_VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property doesn't exist.
…nd not in control of the SDK
|
The |
pcarleton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Implemented modelcontextprotocol/modelcontextprotocol#1692, which lead to opportunities to remove .passthrough() on many places.
NOTE: Added all spec types, however spec tests will still fail until modelcontextprotocol/modelcontextprotocol#1770 gets merged, which fixes an accidental change of
CallToolRequestParams,argumentsto be{ [key: string]: string };instead of{ [key: string]: unknown };in modelcontextprotocol/modelcontextprotocol#1692EDIT: #1770 in the spec repo has been merged, so no issues in spec type checks.
Motivation and Context
Implement SEP-1319, fix spec tests failures, remove passthrough where possibly (although the PR could possibly be achieved without removing .passthrough() as well).
How Has This Been Tested?
Unit tests, examples
Breaking Changes
Any client(s) and server(s) passing through any invalid properties will get typecheck errors. Our examples and tests contained such, which previously slipped through due to type checks not working. Examples of such can be seen in this PR's changes. Any client(s) / server(s) copying from such examples in our codebase would get a typecheck error.
Types of changes
Checklist
Additional context
argumentstightening inCallToolRequestParamsmodelcontextprotocol#1770 gets merged in the spec repo.npm run checkstep is failing becausestep.types.ts(the main protocol spec) is failing our linting. Have ignored the file from linting, as it's not within the SDK's control.