fix(core): preserve field descriptions in the zod <4.2 JSON Schema fallback#2244
fix(core): preserve field descriptions in the zod <4.2 JSON Schema fallback#2244felixweinberger wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 28abf10 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The reused-schema-instance issue I flagged earlier is now addressed (the cycle guard is path-scoped and covered by new tests), and I found no further bugs — but this still deserves a maintainer's eyes since it adds best-effort foreign-zod introspection to core and a new MCP_SUPPRESS_ZOD_FALLBACK_WARNING env var, which are design/API-surface calls rather than correctness questions.
Extended reasoning...
Overview
This PR adds ~130 lines to packages/core/src/util/standardSchema.ts implementing a best-effort recovery walk that re-attaches .describe() descriptions when the SDK falls back to its bundled z.toJSONSchema() for foreign zod instances (zod 4.0/4.1 and the zod@3.25.x zod/v4 subpath). It also makes the fallback warning suppressible via a new MCP_SUPPRESS_ZOD_FALLBACK_WARNING env var, updates the warning text, adds a zod version matrix and a withJsonSchema recipe to the migration docs, and adds two real-second-zod-instance test files (core unit-level and a wire-level tools/list test in the server package) backed by a zod-v40 npm-alias devDependency.
Prior review status
In an earlier run I flagged a real defect: the recursion guard was a global instance-keyed WeakSet, so a schema instance reused at multiple positions only had its first occurrence's description recovered. The current revision fixes this — the guard is now scoped to the recursion path (path.add/path.delete in a try/finally) with the depth cap as a backstop, and new tests cover sibling reuse, reused nested objects, and a synthetic cyclic graph. The current bug-hunting pass found no further bugs, and I verified the fix and the no-overwrite/hostile-getter behaviors against the diff.
Security risks
Low. The recovery walk reads getters on user-supplied schema objects, but every foreign read is wrapped in try/catch with depth caps and a path-scoped cycle guard, so a hostile schema can degrade recovery but not break conversion or throw. The env-var read goes through globalThis.process?.env defensively. No auth, transport, or wire-protocol surfaces are touched.
Level of scrutiny
The correctness of the implementation looks solid and well-tested, but the PR is not a mechanical change: it adds duck-typed introspection of foreign zod internals (_zod.def.innerType, .shape, .element) to core, introduces a new environment variable to the de-facto public surface, and changes user-visible warning text. Under this repo's minimalism/burden-of-proof principles, whether the SDK should carry this compatibility shim (versus pointing users at zod >=4.2 or the documented withJsonSchema wrapper) is a design decision a maintainer should make, so I am not shadow-approving.
Other factors
Test coverage is strong (cross-instance behavior exercised with a real zod@4.0.17 alias, wire-level assertion of what a peer sees, error paths and warning-suppression covered), the changeset and docs match the implemented behavior, and the docs prose accurately describes the best-effort limits (.meta() loss). The package.json/lockfile changes are devDependency-only.
Zod stores .describe() text in a per-instance metadata registry, so when a schema comes from a zod version without ~standard.jsonSchema (zod 4.0/4.1, or the zod@3.25.x zod/v4 subpath), converting it with the SDK-bundled z.toJSONSchema() silently dropped every field description from the advertised tools/list schema. The schema's own .description getters still work across instances, so walk the schema alongside the converted JSON Schema (object properties, array elements, optional/nullable/default wrappers) and fill in anything the converter missed. Existing descriptions are never overwritten, foreign getters are treated as untrusted, and recovery can never break conversion. Also allow silencing the one-time fallback warning via MCP_SUPPRESS_ZOD_FALLBACK_WARNING for applications that cannot emit stray console output, and update the warning text to say what is and is not preserved. Tests use a real second zod instance (npm alias zod-v40 -> zod@4.0.17) rather than mocks, so the cross-registry behavior is exercised for real.
Add a migration-guide subsection and a zod version matrix covering: native conversion on zod >=4.2, best-effort description recovery + one-time warning (and MCP_SUPPRESS_ZOD_FALLBACK_WARNING) on 4.0/4.1-era lineages, and the clear error for zod 3 classic schemas, with a wrapper recipe that supplies the converter from the consumer's own zod instance.
…en precedence test The no-overwrite test previously asserted a value that the recovery walk would have produced anyway, so it could not distinguish preserved from overwritten descriptions; the field getter now reports a different string than the converter emits. The new server test pins what a connected peer sees in tools/list when a tool is registered with a schema from a zod release without ~standard.jsonSchema: descriptions survive in the advertised schema and validated calls still round-trip.
Cycle detection in the foreign-description walk was keyed on the schema instance across the whole traversal, so a described schema reused at several positions (sibling properties, nested objects) was only recovered at the first occurrence and silently skipped afterwards, even though the converter emits a distinct JSON Schema node per occurrence. Scope the guard to the current recursion path instead; the existing depth cap remains the backstop for runaway graphs.
felixweinberger
force-pushed
the
fweinberger/zod-fallback-quality
branch
from
5bfb209 to
28abf10
Compare
| function recoverNode(schema: object, jsonSchema: Record<string, unknown>, path: WeakSet<object>, depth: number): void { | ||
| if (jsonSchema.description === undefined) { | ||
| const description = readForeignDescriptionDeep(schema); | ||
| if (description !== undefined) jsonSchema.description = description; | ||
| } | ||
|
|
||
| const properties = jsonSchema.properties; | ||
| if (properties != null && typeof properties === 'object') { | ||
| const shape = foreignShape(schema); | ||
| if (shape) { | ||
| for (const [key, fieldSchema] of Object.entries(shape)) { | ||
| const fieldJson = (properties as Record<string, unknown>)[key]; | ||
| if (fieldJson != null && typeof fieldJson === 'object') { | ||
| recoverForeignDescriptions(fieldSchema, fieldJson as Record<string, unknown>, path, depth + 1); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const items = jsonSchema.items; | ||
| if (items != null && typeof items === 'object' && !Array.isArray(items)) { | ||
| const element = foreignElement(schema); | ||
| if (element != null) { | ||
| recoverForeignDescriptions(element, items as Record<string, unknown>, path, depth + 1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The recovery walk in recoverNode only descends through properties (paired with the foreign .shape) and items (paired with .element), never into anyOf/oneOf branches — but the bundled z.toJSONSchema() emits .nullable() fields, z.union(), and discriminated unions as { anyOf: [...] } nodes with no top-level properties/items, so descriptions nested inside a nullable or union object field (e.g. the street description in z.object({ street: z.string().describe('street and number') }).describe('postal address').nullable()) are silently dropped from the advertised tools/list schema. Either descend into anyOf/oneOf entries (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 missing description fields. It descends through exactly two JSON Schema keys: jsonSchema.properties (paired with the foreign .shape) and jsonSchema.items (paired with .element). There is no handling of anyOf or oneOf anywhere in the walk. The SDK-bundled z.toJSONSchema() (zod 4, target draft-2020-12) represents .nullable() fields as { anyOf: [ <inner schema>, { type: 'null' } ] }, and z.union([...]) / discriminated unions as anyOf/oneOf arrays — none of these nodes carry top-level properties or items, 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:
const schema = zOld.object({
home: zOld.object({ street: zOld.string().describe('street and number') })
.describe('postal address')
.nullable()
});- The bundled converter cannot see the foreign registry, so the raw output has no descriptions anywhere.
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.- Result:
'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([...]) or z.discriminatedUnion(...) field, which are common in tool input schemas.
Why existing code/tests don't prevent it. foreignShape/foreignElement do unwrap .nullable() on the zod side, but the pairing logic on the JSON Schema side requires a properties/items key on the same node — which the converter places one level down inside anyOf[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, when jsonSchema.anyOf (or oneOf) is an array, pair each non-{ type: 'null' } entry with the unwrapped foreign schema (unwrapForeignSchema already walks _zod.def.innerType; for true unions, the foreign _zod.def.options array can be zipped with the branches) and recurse with depth + 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.
1 participant
Recover
.describe()descriptions in the zod <4.2 JSON Schema fallback, and make the fallback warning suppressible.Motivation and Context
Backwards-compatibility error this fixes: in v1,
server.tool()/registerTool()accepted a schema from whatever zod version the application had installed and advertised its full JSON Schema — including every field's.describe()documentation. In v2, an application on zod 4.0, 4.1, or the widely-deployed zod@3.25.xzod/v4subpath hits the bundled-converter fallback, which silently strips every description from the advertisedtools/listschema — and logs an unsuppressable warning while doing it.Observed while migrating a large production MCP host application from SDK v1 to v2 (the application pins zod 3.25.x and uses the
zod/v4subpath, which implementsStandardSchemaV1but not~standard.jsonSchema):v1 advertised (tools/list):
{ "properties": { "command": { "type": "string", "enum": [...], "description": "The debugger command to run" }, ... } }v2 today advertises (same code, zod <4.2):
{ "properties": { "command": { "type": "string", "enum": [...] }, ... } }…plus
[mcp-sdk] Your zod version does not implement ~standard.jsonSchema…on stderr. Every field description is gone, so tool-calling models lose all argument documentation — silently, since nothing fails. For a CLI application the stray warning is itself a regression, and there was no way to turn it off.Root cause: zod stores
.describe()text in a per-instance metadata registry. The SDK-bundledz.toJSONSchema()reads its own registry, which never contains entries for schemas built by the application's zod instance.How Has This Been Tested?
packages/core/test/util/standardSchema.zodForeignDescriptions.test.tsuses a real second zod instance (npm aliaszod-v40→zod@4.0.17, devDependency of core) rather than mocks, so the cross-registry behavior is exercised for real: top-level descriptions, object properties, nested objects, array elements,.describe()before and after.optional(), no-overwrite of converter-produced descriptions, hostile getters (recovery may never break conversion), the zod-3 error path, once-per-process warning, and env-var suppression. Also covered: a schema instance reused at several positions (sibling properties, nested objects, and over the wire in the server test) is recovered at every occurrence, and cyclic schema graphs terminate (cycle detection is scoped to the current recursion path, with the depth cap as backstop).packages/server/test/server/mcp.foreignZodDescriptions.test.ts: a tool registered onMcpServerwith a foreign-zod schema, driven over anInMemoryTransportpair throughinitialize→tools/list→tools/call— asserts the descriptions a connected peer actually sees in the advertisedinputSchema(top-level, property, and nested-object paths) and that cross-instance validation still round-trips arguments.zod/v4object schemas using.describe().Breaking Changes
None. Recovery only fills
descriptionfields the converter left empty; schemas from zod ≥4.2 and other Standard Schema libraries are untouched. The warning text changed (still contains "zod 4.2.0") and now mentionsMCP_SUPPRESS_ZOD_FALLBACK_WARNING.Types of changes
Checklist
Additional context
.meta()registry metadata other than descriptions is still lost in the fallback (called out in the warning and the migration guide). Full fidelity requires zod ≥4.2 or supplying a converter from the consumer's own zod instance — the migration guide now includes a ~15-linewithJsonSchemawrapper recipe for that.try/catch, depth caps, cycle guard): a misbehaving schema can never breaktools/list.globalThis.process?.envso core stays runtime-neutral for browser/Workers bundles.migration-SKILL.md, since "which zod am I on" turned out to be the first question every migrating codebase has to answer.