-
Notifications
You must be signed in to change notification settings - Fork 1.5k
SPEC COMPLIANCE: Remove loose/passthrough types not allowed/defined by MCP spec + Task types #1242
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
SPEC COMPLIANCE: Remove loose/passthrough types not allowed/defined by MCP spec + Task types #1242
Conversation
commit: |
felixweinberger
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.
Looking at this change to Request and Notification, I'm not convinced 0 passthrough is actually the right objective for all types. I don't think the spec's types are all that strict.
Some types are intended to be flexible per the spec, these ones being specific examples. See the spec's schema.ts which literally has a comment "Allow unofficial extensions of Request.params without impacting `RequestParams")
In practice we're introducing new Generic types just to be able to keep the flexibility which seems odd? Runtime behavior hasn't really changed, we just introduced a new type to hide the flexibility elsewhere?
… types from 2025-11-25 spec
…ub.com:KKonstantinov/typescript-sdk into feature/passthrough-looseobject-removal-part-2
…ub.com:KKonstantinov/typescript-sdk into feature/passthrough-looseobject-removal-part-2
felixweinberger
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.
Thanks @KKonstantinov for reworking this AND syncing our schema.ts to the latest version! Some small follow-ups but I think we can do that in a fast-follow PR if easier.
- Let's use the
npm run fetch:spec-typesto update the spec as that will be what the nightly build does anyway. To align with the 2025-11-25 spec this makes sense, let's make sure we stay synced with the protocol. - The timeout increase - was that necessary for this PR or just bypassing some flakiness?
- Let's remove the
console.logfrom the test.
| * Last updated from commit: 7dcdd69262bd488ddec071bf4eefedabf1742023 | ||
| * | ||
| * DO NOT EDIT THIS FILE MANUALLY. Changes will be overwritten by automated updates. | ||
| * To update this file, run: npm run fetch:spec-types |
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.
did you run this? not sure we should be removing this docstring
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.
In the interim it's fine to do this I think, but we'll want to restore the nightly logic as a fast follow. Ran it locally and there are some small diffs in comments and the 2026-DRAFT version which I think should be the one this points to longer term
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.
Yeah I didn't run it, but got the stable spec from 2025-25-11 version to test against. Now it's restored via #1274
| /* eslint-disable @typescript-eslint/no-unsafe-function-type */ | ||
|
|
||
| // Removes index signatures added by ZodObject.passthrough(). | ||
| type RemovePassthrough<T> = T extends object |
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.
🥳
| } | ||
| } | ||
|
|
||
| console.log(missingTests); |
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.
nit: I'm guessing this was accidentally included during debugging?
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.
Yes
| // Verify we received the notification that was sent while disconnected | ||
| expect(allText).toContain('Missed while disconnected'); | ||
| }, 10000); | ||
| }, 15000); |
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.
nit: This increase in timeouts seems unrelated to this change right? Just to reduce flakiness?
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.
Yes for some reason 10s was still not enough to remove flakiness, I've not looked into why in this PR.
| export const RequestSchema = z.object({ | ||
| method: z.string(), | ||
| params: BaseRequestParamsSchema.optional() | ||
| params: BaseRequestParamsSchema.loose().optional() |
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.
👍
| }); | ||
|
|
||
| const NotificationsParamsSchema = z.looseObject({ | ||
| const NotificationsParamsSchema = z.object({ |
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.
👍
- Restore spec.types.ts docstring header via npm run fetch:spec-types - Remove debugging console.log from spec.types.test.ts - Revert unrelated timeout increase in streamableHttp.test.ts
- Restore spec.types.ts docstring header via npm run fetch:spec-types - Remove debugging console.log from spec.types.test.ts - Revert unrelated timeout increase in streamableHttp.test.ts
Typescript SDK has for a long time contained types which are allowing more than the actual specification. This has been masked by the spec test (spec.types.ts) having a
RemovePassthroughhelper to make types pass typechecks. This has unfortunately lead to actual implementation in the SDK to have wrong types, as well as SDK users to have the ability to use out-of-spec attributes in various JSONRPC methods - knowingly or unknowingly. Further, in many occasions it has broken types for SDK end users.TL; DR:
Motivation and Context
Passthrough and loose objects not allowed by the spec were largely removed in another PR, but a few still remain.
How Has This Been Tested?
Unit tests, type checking.
Breaking Changes
None. Bringing SDK back to full spec compliance.
Types of changes
Checklist