Uh oh!
There was an error while loading. Please reload this page.
Test Fixes for draft SEP-1319 changes - modelcontextprotocol/modelcontextprotocol#1770 needed first - #1084
Conversation
| ], | ||
| "scripts": { | ||
| "fetch:spec-types": "curl -o spec.types.ts https://raw.githubusercontent.com/modelcontextprotocol/modelcontextprotocol/refs/heads/main/schema/draft/schema.ts", | ||
| "fetch:spec-types": "curl -o spec.types.ts https://raw.githubusercontent.com/ccreighton-apptio/modelcontextprotocol/refs/heads/fix/tool-call-arguments-regression/schema/draft/schema.ts", |
There was a problem hiding this comment.
This is a temporary change until modelcontextprotocol/modelcontextprotocol#1770 is merged to fix a regression.
| expect(specTypes).toContain('JSONRPCNotification'); | ||
| expect(specTypes).toContain('ElicitResult'); | ||
| expect(specTypes).toHaveLength(94); | ||
| expect(specTypes).toHaveLength(112); |
There was a problem hiding this comment.
New tests and types are still needed... tests currently fail without them.
| const BaseRequestParamsSchema = z | ||
| .object({ | ||
| _meta: z.optional(RequestMetaSchema) | ||
| }) | ||
| .passthrough(); | ||
| const BaseRequestParamsSchema = z.object({ | ||
| _meta: z.optional(RequestMetaSchema) | ||
| }); |
There was a problem hiding this comment.
passthrough() behavior on base params has been removed from the spec.
| export const RequestSchema = z.object({ | ||
| method: z.string(), | ||
| params: z.optional(BaseRequestParamsSchema) | ||
| params: z.optional(z.record(z.any())) | ||
| }); |
There was a problem hiding this comment.
base classes in the spec enable new extensions of RequestSchema by NOT using the broken-out types in their param definitions
| _meta: z.optional(z.object({}).passthrough()) | ||
| }) | ||
| .passthrough(); | ||
| const NotificationsMetaSchema = z.object({}).passthrough(); |
There was a problem hiding this comment.
Not sure if splitting this out will be necessary in the end, but was following the style set by having RequestMetaSchema defined separately from BaseRequestParamsSchema
| export const NotificationSchema = z.object({ | ||
| method: z.string(), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| params: z.optional(z.record(z.any())) | ||
| }); |
There was a problem hiding this comment.
base classes in the spec enable new extensions of NotificationSchema by NOT using the broken-out types in their param definitions
| export const InitializedNotificationSchema = NotificationSchema.extend({ | ||
| method: z.literal('notifications/initialized') | ||
| method: z.literal('notifications/initialized'), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| }); |
There was a problem hiding this comment.
Individual types are now where Base params are applied, where previously they were included from extending
| export const ProgressSchema = z | ||
| .object({ | ||
| /** | ||
| * The progress thus far. This should increase every time progress is made, even if the total is unknown. | ||
| */ | ||
| progress: z.number(), | ||
| /** | ||
| * Total number of items to process (or total progress required), if known. | ||
| */ | ||
| total: z.optional(z.number()), | ||
| /** | ||
| * An optional message describing the current progress. | ||
| */ | ||
| message: z.optional(z.string()) | ||
| }) | ||
| .passthrough(); | ||
| export const ProgressSchema = z.object({ | ||
| /** | ||
| * The progress thus far. This should increase every time progress is made, even if the total is unknown. | ||
| */ | ||
| progress: z.number(), | ||
| /** | ||
| * Total number of items to process (or total progress required), if known. | ||
| */ | ||
| total: z.optional(z.number()), | ||
| /** | ||
| * An optional message describing the current progress. | ||
| */ | ||
| message: z.optional(z.string()) | ||
| }); |
There was a problem hiding this comment.
this is from lint:fix
E: Actually, looks like I removed a passthrough() here (resulting in the lint change) because it causes ServerNotification not to be spec-compliant.
| // Params types are not exported as standalone types in the SDK (they're inferred from Zod schemas): | ||
| 'RequestParams', | ||
| 'NotificationParams', | ||
| 'CancelledNotificationParams', | ||
| 'InitializeRequestParams', | ||
| 'ProgressNotificationParams', | ||
| 'PaginatedRequestParams', | ||
| 'ResourceRequestParams', | ||
| 'ReadResourceRequestParams', | ||
| 'SubscribeRequestParams', | ||
| 'UnsubscribeRequestParams', | ||
| 'SetLevelRequestParams', | ||
| 'GetPromptRequestParams', | ||
| 'CompleteRequestParams', | ||
| 'CallToolRequestParams', | ||
| 'CreateMessageRequestParams', | ||
| 'LoggingMessageNotificationParams', | ||
| 'ResourceUpdatedNotificationParams', | ||
| 'ElicitRequestParams', | ||
There was a problem hiding this comment.
Alternatively, if the path @ksinder was taking is acceptable, then the only pending change here is getting modelcontextprotocol/modelcontextprotocol#1770 merged so that the fetch:spec-types change can be reverted.
KKonstantinov
commented
Nov 7, 2025
Hi, FYI have opened #1086, which resolves the spec issues (apart from modelcontextprotocol/modelcontextprotocol#1770). However, it contains removal of .passthrough() as well and needs review/acceptance. Depending on decisions, could open it without the passthrough removals. |
ccreighton-apptio
commented
Nov 7, 2025
Cool! At a glance, that looks like a much more complete implementation of the SEP. I can't speak to the desires of the typescript-sdk maintainers, but note that in this PR I've only removed passthrough behavior from the specific places it needed to be to comply with the removal of passthrough behavior in the draft spec. |
Uh oh!
There was an error while loading. Please reload this page.
SEP-1319 breaks out params into individual types. Arbitrary parameters were fixed for several types in the process.
Motivation and Context
Tests are failing
How Has This Been Tested?
Tests are passing
Breaking Changes
From draft schema changes, yes
Types of changes
Checklist
Additional context
This is incomplete - modelcontextprotocol/modelcontextprotocol#1770 must be merged first and then the
fetch:spec-typeschange must be reverted.This does not implement SEP-1319 here, only:
If there is a desire to implement SEP-1319 here, the params types would need separate definitions and test coverage. I do not intend to complete that refactor. These changes will unblock the current build failures pending a complete implementation.