-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(core): preserve field descriptions in the zod <4.2 JSON Schema fallback #2244
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
Open
felixweinberger
wants to merge
4
commits into
main
Choose a base branch
from
fweinberger/zod-fallback-quality
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+491
−9
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
6bf76c0
Recover .describe() descriptions in the zod <4.2 JSON Schema fallback
felixweinberger 304f331
Document the zod <4.2 JSON Schema fallback and its limits
felixweinberger 42e7659
Add wire-level test for foreign-zod description recovery and strength…
felixweinberger 28abf10
Recover descriptions for schema instances reused at multiple positions
felixweinberger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@modelcontextprotocol/core': patch | ||
| '@modelcontextprotocol/client': patch | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| Recover `.describe()` descriptions when converting schemas from zod versions without `~standard.jsonSchema` (zod 4.0/4.1 and the zod@3.25.x `zod/v4` subpath). The bundled-converter fallback previously dropped all registry-held metadata, silently advertising tool schemas without | ||
| any field documentation. The fallback warning can now be silenced with `MCP_SUPPRESS_ZOD_FALLBACK_WARNING=1` and mentions what is and isn't preserved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 recovery walk in
recoverNodeonly descends throughproperties(paired with the foreign.shape) anditems(paired with.element), never intoanyOf/oneOfbranches — but the bundledz.toJSONSchema()emits.nullable()fields,z.union(), and discriminated unions as{ anyOf: [...] }nodes with no top-levelproperties/items, so descriptions nested inside a nullable or union object field (e.g. thestreetdescription inz.object({ street: z.string().describe('street and number') }).describe('postal address').nullable()) are silently dropped from the advertisedtools/listschema. Either descend intoanyOf/oneOfentries (pairing each non-null branch with the unwrapped inner schema) or narrow the migration-doc claim, which currently lists.nullable()among the covered cases.Extended reasoning...
What the bug is.
recoverNode(packages/core/src/util/standardSchema.ts:264-290) walks the foreign zod schema alongside the converted JSON Schema and fills in missingdescriptionfields. It descends through exactly two JSON Schema keys:jsonSchema.properties(paired with the foreign.shape) andjsonSchema.items(paired with.element). There is no handling ofanyOforoneOfanywhere in the walk. The SDK-bundledz.toJSONSchema()(zod 4, targetdraft-2020-12) represents.nullable()fields as{ anyOf: [ <inner schema>, { type: 'null' } ] }, andz.union([...])/ discriminated unions asanyOf/oneOfarrays — none of these nodes carry top-levelpropertiesoritems, so the walk stops there and everything nested below is left without its description.Step-by-step proof. Take a foreign-zod (4.0/4.1 or zod@3.25.x
zod/v4) schema:properties.homeis{ anyOf: [ { type: 'object', properties: { street: { type: 'string' } } }, { type: 'null' } ] }.recoverForeignDescriptionsreachesproperties.homepaired with the foreignZodNullable.readForeignDescriptionDeepunwrapsinnerTypeand recovers'postal address'onto theanyOfnode — so the wrapper-level description is recovered (and the docs'.nullable()wrapper claim is backed for that level).recoverNodethen looks forjsonSchema.propertiesandjsonSchema.itemson theanyOfnode. Neither exists, so the walk returns. It never visitsanyOf[0].properties.street.'street and number'is missing from the advertisedtools/listschema, while the identical field without.nullable()keeps it (the existing test foraddress.streetcovers that path).The same applies to every object member of a
z.union([...])orz.discriminatedUnion(...)field, which are common in tool input schemas.Why existing code/tests don't prevent it.
foreignShape/foreignElementdo unwrap.nullable()on the zod side, but the pairing logic on the JSON Schema side requires aproperties/itemskey on the same node — which the converter places one level down insideanyOf[0]. The new test files (standardSchema.zodForeignDescriptions.test.ts,mcp.foreignZodDescriptions.test.ts) exercise nested objects, arrays, optional wrappers, and reused instances, but no.nullable()object field or union, so nothing pins this path.Impact. This is the same silent-metadata-loss failure mode the PR sets out to fix, just one level deeper: nested field documentation disappears from the schema tool-calling models see, with no error or warning. It also creates a doc/implementation mismatch within this diff: docs/migration.md and the migration-SKILL matrix added here say descriptions are recovered for 'top level, object properties, array elements, and
.optional()/.nullable()/.default()wrappers' — a reader with a nullable object field would naturally expect its property descriptions to survive, and they don't. To be fair, the recovery is explicitly billed as best-effort, conversion itself is unaffected, and pre-PR behavior lost all descriptions, so this only degrades an improvement rather than regressing anything — hence non-blocking.How to fix. In
recoverNode, whenjsonSchema.anyOf(oroneOf) is an array, pair each non-{ type: 'null' }entry with the unwrapped foreign schema (unwrapForeignSchemaalready walks_zod.def.innerType; for true unions, the foreign_zod.def.optionsarray can be zipped with the branches) and recurse withdepth + 1. The existing path-scoped cycle guard and depth cap already bound the extra recursion. Alternatively, narrow the doc parenthetical to make clear that content nested inside nullable/union fields is not recovered.