-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(core): add parse/safeParse methods to specTypeSchemas entries #2241
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
base: main
Are you sure you want to change the base?
Changes from all commits
5f4253e
cc29d4b
c0b5827
b330712
5b68b05
76561bd
81335d9
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| '@modelcontextprotocol/server': minor | ||
| --- | ||
|
|
||
| Add v1-style `parse`/`safeParse` methods to every `specTypeSchemas` entry, so v1 call sites migrate with a one-line rename: `CallToolResultSchema.parse(value)` becomes `specTypeSchemas.CallToolResult.parse(value)`. `parse` returns the parsed value or throws `SpecTypeValidationError` (an `SdkError` subclass with code `SdkErrorCode.InvalidSpecType`, carrying `.specType` and `.issues`); `safeParse` returns a `{ success, data | issues }` discriminated union so migrated call sites keep their control flow. Both are synchronous, and each entry remains a Standard Schema (`['~standard'].validate`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,7 @@ New `SdkErrorCode` enum values: | |
| - `SdkErrorCode.ConnectionClosed` = `'CONNECTION_CLOSED'` | ||
| - `SdkErrorCode.SendFailed` = `'SEND_FAILED'` | ||
| - `SdkErrorCode.InvalidResult` = `'INVALID_RESULT'` | ||
| - `SdkErrorCode.InvalidSpecType` = `'INVALID_SPEC_TYPE'` | ||
| - `SdkErrorCode.ClientHttpNotImplemented` = `'CLIENT_HTTP_NOT_IMPLEMENTED'` | ||
| - `SdkErrorCode.ClientHttpAuthentication` = `'CLIENT_HTTP_AUTHENTICATION'` | ||
| - `SdkErrorCode.ClientHttpForbidden` = `'CLIENT_HTTP_FORBIDDEN'` | ||
|
|
@@ -471,16 +472,19 @@ For **custom (non-spec)** methods, keep the result-schema argument — see §9. | |
|
|
||
| Remove unused schema imports: `CallToolResultSchema`, `CompatibilityCallToolResultSchema`, `ElicitResultSchema`, `CreateMessageResultSchema`, etc., when they were only used in `request()`/`send()`/`callTool()` calls. | ||
|
|
||
| If a `*Schema` constant was used for **runtime validation** (not just as a `request()` argument), replace with `isSpecType` / `specTypeSchemas`: | ||
| If a `*Schema` constant was used for **runtime validation** (not just as a `request()` argument), the `specTypeSchemas.<TypeName>` entries carry `parse`/`safeParse` methods — a mechanical rename: | ||
|
|
||
| | v1 pattern | v2 replacement | | ||
| | -------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | | ||
| | `CallToolResultSchema.safeParse(value).success` | `isSpecType.CallToolResult(value)` | | ||
| | `<TypeName>Schema.safeParse(value).success` | `isSpecType.<TypeName>(value)` | | ||
| | `<TypeName>Schema.parse(value)` | `specTypeSchemas.<TypeName>['~standard'].validate(value)` (returns a `Result` synchronously, not the value) | | ||
| | Passing `<TypeName>Schema` as a validator argument | `specTypeSchemas.<TypeName>` (a `StandardSchemaV1Sync<In, Out>`) | | ||
| | v1 pattern | v2 replacement | | ||
| | -------------------------------------------------- | -------------------------------------------------------------------------------------- | | ||
| | `<TypeName>Schema.parse(value)` | `specTypeSchemas.<TypeName>.parse(value)` (returns the parsed value; throws `SpecTypeValidationError`, an `SdkError` subclass with code `SdkErrorCode.InvalidSpecType`, with `.issues`) | | ||
| | `<TypeName>Schema.safeParse(value)` | `specTypeSchemas.<TypeName>.safeParse(value)` (`{ success: true, data }` \| `{ success: false, issues }`) | | ||
| | `CallToolResultSchema.safeParse(value).success` | `isSpecType.CallToolResult(value)` | | ||
| | `<TypeName>Schema.safeParse(value).success` | `isSpecType.<TypeName>(value)` | | ||
| | Passing `<TypeName>Schema` as a validator argument | `specTypeSchemas.<TypeName>` (a `StandardSchemaV1Sync<In, Out>` with `parse`/`safeParse`) | | ||
|
|
||
| `isCallToolResult(value)` still works, but `isSpecType` covers every spec type by name. | ||
| All validation is synchronous (`StandardSchemaV1Sync`) — never add `await`. `isCallToolResult(value)` still works, but `isSpecType` covers every spec type by name. | ||
|
|
||
| Note: the v1→v2 codemod currently rewrites these call sites to the lower-level `specTypeSchemas.<TypeName>['~standard'].validate(value)` form. That form is equivalent and stays supported, but `parse`/`safeParse` is the recommended form when writing or revising call sites by hand; a codemod update to emit the methods directly is planned. | ||
|
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. 🟡 The note at the end of section 11 says the v1→v2 codemod "currently rewrites these call sites to the lower-level Extended reasoning...What the prose claims vs. what the codemod does. The new note added at What the codemod actually rewrites. The transform handles four shapes: (1) Step-by-step proof. Take v1 code containing Why this matters. The SKILL doc is explicitly written for mechanical application by an LLM or a migrating user. The note exists to set expectations about what codemod output will look like versus what to write by hand; as written it tells readers the codemod has already converted their Relationship to the existing review comment. This is distinct from the earlier inline comment on this file about the codemod's own diagnostic strings becoming stale (" How to fix. Narrow the sentence to match reality, e.g.: "Note: the v1→v2 codemod currently rewrites captured |
||
|
|
||
| ## 12. Experimental tasks interception removed | ||
|
|
||
|
|
||
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.
🔴 The v1→v2 codemod's spec-schema transform (
packages/codemod/src/migrations/v1-to-v2/transforms/specSchemaAccess.ts, untouched by this PR) still emits user-facing diagnostics stating that.parse()/.safeParse()are "not available in v2" and that a "Manual rewrite required" — statements that become factually wrong once this PR lands, so the published@modelcontextprotocol/codemodwill directly contradict the SDK and this migration table. Even if retargeting the codemod's rewrite output to emit.parse/.safeParsestays a follow-up, the "not available" diagnostic prose should be corrected in the same change so the migration tooling doesn't steer users away from the API this PR adds.Extended reasoning...
What the bug is. This PR adds
.parse()/.safeParse()methods to everyspecTypeSchemasentry and rewrites both migration guides to present them as the primary replacement for v1's<TypeName>Schema.parse()/.safeParse()call sites. But the v1→v2 codemod's spec-schema transform —packages/codemod/src/migrations/v1-to-v2/transforms/specSchemaAccess.ts, not touched by this PR — still emits user-facing diagnostics that flatly contradict the new API:${localName}.safeParse() not available in v2. Use isSpecType.${typeName}(value) for boolean validation, or specTypeSchemas.${typeName}['~standard'].validate(value) for full result.${localName}.parse() not available in v2. Use isSpecType... or ['~standard'].validate(value) and check for issues.Replaced ${localName} with specTypeSchemas.${typeName}. Note: typed as StandardSchemaV1, not ZodType — Zod methods like .safeParse()/.parse() are not available.(line 137 adds "Manual rewrite required.")In addition,
rewriteCapturedSafeParse(lines ~249–331) still rewrites capturedsafeParsecall sites into the verbose['~standard'].validateremap (.success→.issues === undefined,.data→.value) — exactly the error-prone pattern this PR's description says it was written to eliminate.The code path that triggers it. A user runs the published
@modelcontextprotocol/codemod(the pkg-pr-new bot shows it is built and published from this same monorepo and this PR) on v1 code containingCallToolResultSchema.parse(value)orOAuthTokensSchema.safeParse(value). The transform replaces the identifier withspecTypeSchemas.CallToolResultand then prints a warning telling the user that.parse()/.safeParse()"are not available" and that a manual rewrite to['~standard'].validateis required.Step-by-step proof. (1) This PR's
register()inpackages/core/src/types/specTypeSchema.tsattachesparse/safeParseto everyspecTypeSchemasentry, sospecTypeSchemas.CallToolResult.parse(value)works. (2) The codemod's value-position rewrite already turnsCallToolResultSchemaintospecTypeSchemas.CallToolResult, so the remaining trailing.parse(value)in the user's code now compiles and runs correctly with no further edits. (3) Yet the diagnostic emitted at lines 137/162/188 for that exact rewrite says the Zod-style methods "are not available" and (at 137) that a "Manual rewrite required" — directing the user to undo a working call site and replace it with the['~standard'].validateremap thatdocs/migration.mdanddocs/migration-SKILL.md(updated in this PR) now demote to "the underlying mechanism". (4) The lines 107–108/120–121 diagnostics for the bare-constant case likewise recommend onlyisSpecTypeor the['~standard'].validateremap, never mentioning the one-line rename this PR documents as the primary path.Why nothing prevents it. Nothing in this PR touches the codemod package, and there is no test or lint coupling the codemod's diagnostic strings to the SDK's actual API surface, so the contradiction ships silently. The PR description's "Follow-up opportunity" only covers the enhancement of emitting
.parse/.safeParserewrites directly; it does not address that the existing diagnostic prose becomes affirmatively false the moment this merges.Impact. The official migration tool will contradict the official migration docs and the SDK's behavior in the same release: users who trust the codemod output will hand-rewrite working call sites into the verbose, inversion-prone
['~standard'].validatepattern this PR exists to remove, and users who read the docs will see the codemod calling the documented API nonexistent. No runtime breakage, but published, user-facing guidance becomes incorrect.How to fix. At minimum, in this same change update the diagnostic strings in
specSchemaAccess.ts(lines 107–108, 120–121, 137, 162, 188) to say the entries now carry.parse()/.safeParse()(e.g. "the rewrittenspecTypeSchemas.Xentry supports.parse()/.safeParse()directly;.parseAsync()is not needed — validation is synchronous"). The fuller follow-up — having the transform emit.parse/.safeParserewrites and retiringrewriteCapturedSafeParse's['~standard'].validateremap — can land separately as planned.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 for adding the note in
docs/migration-SKILL.md(commit b8deb0b) acknowledging that the codemod currently rewrites these call sites to the['~standard'].validateform — that resolves the docs-side inconsistency for readers of the migration guide.However, the core issue is still open: the codemod's own diagnostic strings in
packages/codemod/src/migrations/v1-to-v2/transforms/specSchemaAccess.tsremain unchanged and become factually wrong once this PR lands:.safeParse()/.parse()are "not available in v2" and point only toisSpecTypeor the['~standard'].validateremap.A user running the published
@modelcontextprotocol/codemod(built from this same monorepo) sees those messages without ever reading the migration guide, so the official tooling will still steer them away from the API this PR adds — and away from the docs' recommended form. The doc note even calls the validate form merely "the lower-level" equivalent, while the codemod calls the recommended form nonexistent.The retargeting of the codemod's rewrite output (emitting
.parse/.safeParsedirectly, retiringrewriteCapturedSafeParse's remap) can certainly stay a follow-up as planned — but the diagnostic prose itself is a small string-only change that would keep the published codemod from contradicting the SDK in the same release. E.g. update those messages to say the rewrittenspecTypeSchemas.Xentry now supports.parse()/.safeParse()directly (with.parseAsync()unnecessary since validation is synchronous).