-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Test Fixes for draft SEP-1319 changes - modelcontextprotocol/modelcontextprotocol#1770 needed first #1084
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
Test Fixes for draft SEP-1319 changes - modelcontextprotocol/modelcontextprotocol#1770 needed first #1084
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -451,6 +451,26 @@ const MISSING_SDK_TYPES = [ | |
| 'Role', | ||
| 'Error', // The inner error object of a JSONRPCError | ||
|
|
||
| // 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', | ||
|
|
||
|
Comment on lines
+454
to
+473
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, if the path @ksinder was taking is acceptable, then the only pending change here is getting modelcontextprotocol/modelcontextprotocol#1770 merged so that the |
||
| // These aren't supported by the SDK yet: | ||
| // TODO: Add definitions to the SDK | ||
| 'Annotations', | ||
|
|
@@ -470,7 +490,7 @@ describe('Spec Types', () => { | |
| it('should define some expected types', () => { | ||
| expect(specTypes).toContain('JSONRPCNotification'); | ||
| expect(specTypes).toContain('ElicitResult'); | ||
| expect(specTypes).toHaveLength(94); | ||
| expect(specTypes).toHaveLength(112); | ||
|
Comment on lines
-473
to
+493
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New tests and types are still needed... tests currently fail without them. |
||
| }); | ||
|
|
||
| it('should have up to date list of missing sdk types', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,30 +27,28 @@ const RequestMetaSchema = z | |
| }) | ||
| .passthrough(); | ||
|
|
||
| const BaseRequestParamsSchema = z | ||
| .object({ | ||
| _meta: z.optional(RequestMetaSchema) | ||
| }) | ||
| .passthrough(); | ||
| const BaseRequestParamsSchema = z.object({ | ||
| _meta: z.optional(RequestMetaSchema) | ||
| }); | ||
|
Comment on lines
-30
to
+32
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| export const RequestSchema = z.object({ | ||
| method: z.string(), | ||
| params: z.optional(BaseRequestParamsSchema) | ||
| params: z.optional(z.record(z.any())) | ||
| }); | ||
|
Comment on lines
34
to
37
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. base classes in the spec enable new extensions of |
||
|
|
||
| const BaseNotificationParamsSchema = z | ||
| .object({ | ||
| /** | ||
| * See [MCP specification](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/47339c03c143bb4ec01a26e721a1b8fe66634ebe/docs/specification/draft/basic/index.mdx#general-fields) | ||
| * for notes on _meta usage. | ||
| */ | ||
| _meta: z.optional(z.object({}).passthrough()) | ||
| }) | ||
| .passthrough(); | ||
| const NotificationsMetaSchema = z.object({}).passthrough(); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if splitting this out will be necessary in the end, but was following the style set by having |
||
|
|
||
| const BaseNotificationParamsSchema = z.object({ | ||
| /** | ||
| * See [MCP specification](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/47339c03c143bb4ec01a26e721a1b8fe66634ebe/docs/specification/draft/basic/index.mdx#general-fields) | ||
| * for notes on _meta usage. | ||
| */ | ||
| _meta: z.optional(NotificationsMetaSchema) | ||
| }); | ||
|
|
||
| export const NotificationSchema = z.object({ | ||
| method: z.string(), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| params: z.optional(z.record(z.any())) | ||
| }); | ||
|
Comment on lines
49
to
52
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. base classes in the spec enable new extensions of |
||
|
|
||
| export const ResultSchema = z | ||
|
|
@@ -395,7 +393,8 @@ export const InitializeResultSchema = ResultSchema.extend({ | |
| * This notification is sent from the client to the server after initialization has finished. | ||
| */ | ||
| export const InitializedNotificationSchema = NotificationSchema.extend({ | ||
| method: z.literal('notifications/initialized') | ||
| method: z.literal('notifications/initialized'), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| }); | ||
|
Comment on lines
395
to
398
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Individual types are now where |
||
|
|
||
| export const isInitializedNotification = (value: unknown): value is InitializedNotification => | ||
|
|
@@ -406,26 +405,25 @@ export const isInitializedNotification = (value: unknown): value is InitializedN | |
| * A ping, issued by either the server or the client, to check that the other party is still alive. The receiver must promptly respond, or else may be disconnected. | ||
| */ | ||
| export const PingRequestSchema = RequestSchema.extend({ | ||
| method: z.literal('ping') | ||
| method: z.literal('ping'), | ||
| params: z.optional(BaseRequestParamsSchema) | ||
| }); | ||
|
|
||
| /* Progress notifications */ | ||
| 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()) | ||
| }); | ||
|
Comment on lines
-413
to
+426
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is from E: Actually, looks like I removed a |
||
|
|
||
| /** | ||
| * An out-of-band notification used to inform the receiver of a progress update for a long-running request. | ||
|
|
@@ -622,7 +620,8 @@ export const ReadResourceResultSchema = ResultSchema.extend({ | |
| * An optional notification from the server to the client, informing it that the list of resources it can read from has changed. This may be issued by servers without any previous subscription from the client. | ||
| */ | ||
| export const ResourceListChangedNotificationSchema = NotificationSchema.extend({ | ||
| method: z.literal('notifications/resources/list_changed') | ||
| method: z.literal('notifications/resources/list_changed'), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| }); | ||
|
|
||
| /** | ||
|
|
@@ -860,7 +859,8 @@ export const GetPromptResultSchema = ResultSchema.extend({ | |
| * An optional notification from the server to the client, informing it that the list of prompts it offers has changed. This may be issued by servers without any previous subscription from the client. | ||
| */ | ||
| export const PromptListChangedNotificationSchema = NotificationSchema.extend({ | ||
| method: z.literal('notifications/prompts/list_changed') | ||
| method: z.literal('notifications/prompts/list_changed'), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| }); | ||
|
|
||
| /* Tools */ | ||
|
|
@@ -1037,7 +1037,8 @@ export const CallToolRequestSchema = RequestSchema.extend({ | |
| * An optional notification from the server to the client, informing it that the list of tools it offers has changed. This may be issued by servers without any previous subscription from the client. | ||
| */ | ||
| export const ToolListChangedNotificationSchema = NotificationSchema.extend({ | ||
| method: z.literal('notifications/tools/list_changed') | ||
| method: z.literal('notifications/tools/list_changed'), | ||
| params: z.optional(BaseNotificationParamsSchema) | ||
| }); | ||
|
|
||
| /* Logging */ | ||
|
|
||
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.
This is a temporary change until modelcontextprotocol/modelcontextprotocol#1770 is merged to fix a regression.