fix(core-internal): make zod toJSONSchema conversion wire-truthful for tool schemas - #2556
fix(core-internal): make zod toJSONSchema conversion wire-truthful for tool schemas#2556claude[bot] wants to merge 16 commits into
Conversation
…r tool schemas
The SDK validates tool payloads with the user's zod schema but ships the
raw object, so the advertised JSON Schema must describe that raw shape.
Pass zod-scoped conversion options (via libraryOptions on the Standard
JSON Schema path, and directly on the zod 4.0-4.1 fallback):
- unrepresentable: 'any' so one z.date()/z.bigint() field no longer
throws and fails the entire tools/list response
- rewrite z.date() to {type: 'string', format: 'date-time'}, the shape
JSON.stringify actually produces for a Date
- for output schemas, drop .default()-carrying fields from required and
drop additionalProperties: false on plain z.object() (kept for
z.strictObject()), so validating clients accept legitimate
structuredContent the server ships as returned
Fixes #2464
Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 98a3379 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/core
@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.
Beyond the inline findings, the input-side analogue of the date rewrite was also examined and ruled out: advertising z.date() inputs as string/date-time while server-side zod validation rejects strings is not a regression — pre-PR such a tool failed the entire tools/list, so no previously-working flow breaks, and the behavior matches the v1 converter (#2467); z.coerce.date() remains the way to accept string dates.
Extended reasoning...
Two real bugs plus a nit were found and posted inline, so approval is off the table and the inline comments already convey the need for human review. This note only records the one additional candidate that was examined and refuted this run — the input-schema z.date() advertise/validate mismatch — so a later pass doesn't re-explore it from scratch. It is informational, not a correctness guarantee.
- Keep user annotations (.describe()/.meta()) on rewritten z.date()
fields: under unrepresentable: 'any' the node carries only annotation
keywords, so the previous wipe loop deleted exactly the user's
metadata and nothing else.
- Key the output required-filter on the zod shape instead of the
emitted default keyword: a registered .default() hides its default
behind a $ref (and stayed required), a .meta({default}) annotation on
a genuinely-required field was wrongly dropped, and undefined-
accepting fields (z.any(), z.unknown(), z.undefined()) stayed
required even though JSON.stringify drops undefined-valued keys from
the wire. A field is now dropped from required iff validating
undefined succeeds (missing key semantics).
- Opt the elicitation path out of the graceful-degradation options via
a new unrepresentable: 'throw' conversion option: a z.date()
rewritten to string/date-time would pass the wire checks, but the
accepted response could never satisfy z.date() on handler re-entry,
so acceptedContent() would silently discard the user's answer.
z.date() keeps throwing the documented TypeError before anything is
sent.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Beyond the inline findings, I also examined whether advertising z.date() input fields as string/date-time creates a new mismatch with server-side validation — tools/call does validate arguments with the raw zod schema (mcp.ts validateToolInput), so a string date fails that individual call with InvalidParams. Ruled out as a new issue: pre-PR the same schema failed the entire tools/list (every tool disappeared), so a per-call validation error is strictly less severe, and it matches the v1/zod-3 converter's long-standing behavior for z.date() inputs.
Extended reasoning...
Bugs were found this run (unhandled-rejection crash risk in the fieldAcceptsMissingKey probe, and the required-filter gap for enum-keyed records), so approval is not appropriate; the inline comments carry those findings. This note only records the one additional candidate examined and refuted: the input-side z.date() advertised-vs-validated mismatch is pre-existing v1 behavior and an improvement over the pre-PR total tools/list failure, not a new defect introduced by this PR.
… records
- fieldAcceptsMissingKey no longer lets a throwing .transform()/.refine()
escape the probe: depending on the zod build, ~standard.validate(undefined)
either throws synchronously or returns a rejecting Promise whose unhandled
rejection crashes the process during tools/list conversion. Both modes now
conservatively keep the field required, and the Promise branch attaches a
no-op catch so no rejection floats.
- Enum-keyed records (def.type === 'record') also emit a required list zod
does not enforce when the shared value schema is defaulted or
undefined-accepting; the override now drops it (all-or-nothing, since every
key shares the one value schema).
- Move StandardSchemaToJsonSchemaOptions above the conversion function's
JSDoc block so the doc comment re-attaches to the function it describes,
and give the interface its own one-liner.
- Stop overclaiming in the changeset and JSDoc: BigInt values embedded as
defaults or metadata (.default(0n), .meta({default: 1n})) still fail
conversion inside zod's own processors, outside the override's reach.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Beyond the inline nit, this round also examined and ruled out two input-side candidates: (1) input (io: 'input') schemas keeping required on enum-keyed records / undefined-accepting fields — not the #2464 failure class, since an over-listed input required only makes clients supply keys validation would tolerate omitting; no legitimate payload is rejected. (2) the z.date() → string/date-time rewrite on input schemas creating an unsatisfiable per-call contract — the resulting tools/call validation failure is a loud InvalidParams error (matching v1 converter behavior), strictly better than the pre-PR whole-tools/list failure, and the elicitation path now opts out via unrepresentable: 'throw'.
Extended reasoning...
Bugs were found this run (one nit on pipe/transform/coerce output schemas, posted inline), so this note only records what else was examined and refuted. The prior two rounds' findings (date annotation wipe, JSON-keyed required proxy, record required, unhandled probe rejection, elicitation date path, JSDoc attachment) are all addressed in commits 034ccc1 and d11d6fc — verified against the current source. The PR remains non-trivial core conversion logic and should still get a human look before merge; no approval.
…oerce outputs Output schemas containing .transform()/.pipe()/z.coerce still advertise the post-transform shape (io: 'output') while the server validates and ships the raw pre-transform value — a pre-existing mismatch this PR does not address. Note it as a known residual gap in the zodConversionOptions contract and scope the changeset claim accordingly, instead of rewriting pipe nodes in the override: a per-node input-side re-conversion would break $refs to registered schemas (a nested conversion's $defs land at the wrong document root), and advertising output schemas with input semantics wholesale is a design decision that interacts with SEP-2106 non-object output roots. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Beyond the inline finding, this run also examined the symmetric input-side concern — that rewriting z.date() (and degrading z.bigint()) in inputSchema advertises argument shapes the tool's zod validation can never accept (a JSON string never satisfies z.date() on tools/call) — and ruled it out as a defect of this PR: those schemas were entirely unlistable before (the whole tools/list threw), so no working case regresses, and the rewrite matches the v1/zod-3 converter's long-standing behavior. Recording it here so a later pass doesn't re-explore it.
Extended reasoning...
Bugs were found this run (one nit inline: the zod 4.0–4.2.x isParent guard can skip the override on clone-parent nodes, making the fix inert on the declared peer floor while the lockfile's zod 4.3.6 hides it from tests). Per the review process, the inline comment already signals the needed attention; this comment only records that the input-side z.date()/z.bigint() advertisement concern was independently examined and refuted (no regression vs. pre-PR total tools/list failure, and v1 converter parity), so future review passes need not re-derive it.
…chemas
On zod 4.0-4.2.x, toJSONSchema guards the override hook with
'if (!seen.isParent)' (v4/core/to-json-schema.js), and a schema instance
is marked isParent whenever a clone of it (.describe()/.meta()) appears
in the same conversion — so a schema reused both bare and via a clone
skips sanitization on the bare node: untruthful required/
additionalProperties survive and z.date() emits {}. zod 4.3.0 removed
the guard, so the lockfile resolution (4.3.6) and the test suite cannot
observe it. Verified against the published 4.2.1 and 4.3.0 tarballs.
Document it as a known residual gap and scope the changeset claim;
bumping the declared zod floor (^4.2.0 -> ^4.3.0) would close it for
the primary path but is a maintainer decision.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Beyond the inline findings, this pass also examined the input-side rewrite — z.date() in an inputSchema advertised as string/date-time while tools/call validates arguments with the raw zod schema — and ruled it out: it matches the deliberate v1 #2467 / zod-v3 converter behavior this PR cites, and it strictly improves on the pre-PR failure mode (the entire tools/list dying), so it is a design precedent rather than a bug introduced here.
Extended reasoning...
This run's three findings are all nit-level residual gaps (async-staged defaults / .catch() nodes missing from the documented gap list, non-object unrepresentable roots bypassing the input-root guard, and the z.date() divergence over the non-serializing InMemoryTransport) — none is a regression, and each follows the prose-scoping pattern the author already applied in prior rounds. Separately, a finder raised the input-side date/unrepresentable rewrite as advertising argument shapes the server's own tools/call validation rejects; verification confirmed validateToolInput does run the raw zod schema (mcp.ts), but the rewrite is the same intentional choice made in the v1 #2467 fix and the zod v3 converter, and pre-PR the identical registration failed the whole listing, so it was ruled out as established design rather than a defect of this PR. Recording it here so a later pass does not re-explore it.
…e-root gaps
- fieldAcceptsMissingKey short-circuits on def.type 'default'/'prefault'
before the validate(undefined) probe: a defaulted field accepts a
missing key by construction, but an async stage (.refine(async ...))
pushed the probe to a Promise and conservatively kept the field in the
advertised required list while async-capable server validation accepts
the omission.
- Output .catch() nodes degrade to an unconstrained schema (annotations
and the emitted default kept): catch-validation accepts any raw value
— the fallback replaces it only in the parsed result, which the
server never ships — so advertising the inner constraints made
validating clients reject legitimate results.
- The input-root guard keys on the emitted type, which
unrepresentable: 'any' erases for bare z.bigint()/z.map()/z.set()/
z.symbol() roots — they were stamped {type: 'object'} and advertised
as permanently-uncallable tools. Recover the signal from the zod def
so misregistered roots keep throwing the actionable 'must describe
objects' error.
- Document the InMemoryTransport transport-dependence of the z.date()
string/date-time advertisement (pass-by-reference, no JSON
round-trip) as a known residual gap in the JSDoc and changeset.
Co-Authored-By: Claude <noreply@anthropic.com>
…er root guard
- Skip the .catch() degrade at the conversion root: deleting
'type: object' there flipped the 2025-era codec's legacy-wrap
predicate (isNonObjectJsonSchemaRoot), silently shipping
structuredContent as {result: ...} to 2025-era peers for root-level
.catch() output schemas that were object-rooted pre-PR.
- Preserve x-* vendor-extension keys through the nested .catch()
degrade, mirroring the elicitation walker's annotation-only
convention — they carry no validation constraint.
- Extend NON_OBJECT_UNREPRESENTABLE_ROOTS with 'void', 'undefined',
'nan', and 'function': all degrade to a typeless {} under
unrepresentable: 'any' and can never accept a JSON object, so they
must keep throwing the actionable root error (z.custom() stays
excluded — it can legitimately accept objects).
- Document the input-side z.date() round-trip impossibility (advertised
string/date-time vs raw-zod input validation) as a known residual
gap in the JSDoc and changeset, pointing at z.iso.date()/
z.iso.datetime() as the supported input spellings.
Co-Authored-By: Claude <noreply@anthropic.com>
… the root guard
- Extend the .catch() degrade bail from the conversion root to every
position that feeds the output epilogue's root object proof (members
of root-level, possibly nested, anyOf/oneOf/allOf compositions):
degrading such a member broke isProvablyObjectShapedRoot's
every-member proof, left the root typeless, and flipped the 2025-era
legacy wrap — silently shipping structuredContent as {result: ...}
for previously-working union/intersection output schemas.
- The input-root guard now unwraps the zod def chain (optional/
nullable/readonly/default/prefault/catch via innerType, lazy via its
getter with a seen-set cycle guard) before consulting the non-object
set, so z.bigint().optional() and z.lazy(() => z.bigint()) no longer
become phantom {type: 'object'} tools; 'literal' joins the set
(typeless literal roots — unrepresentable values like
z.literal(undefined) or mixed-type value lists — cannot describe
objects; representable single-type literal roots already throw via
the explicit-type guard).
- Document dynamic catch values (.catch(ctx => ...)) as a known
residual gap: zod's catchProcessor throws before the override hook
runs, so the degrade covers static fallback values only; changeset
claim scoped to match.
Co-Authored-By: Claude <noreply@anthropic.com>
…romise roots - The catch-degrade verdict must be position-independent: zod deduplicates reused schema instances and runs the override once per instance, so a .catch() shared between a nested position and a root(-composition) position got one verdict applied to both sites — nested-first broke the root object proof (2025-era legacy-wrap flip), root-first kept unenforced inner constraints at the nested copy (the #2464 client-rejection class). The degrade now always keeps the emitted 'type' (dropping properties/required/additionalProperties and other constraints), which preserves the root proof at every position; the position-dependent feedsRootObjectProof branch is removed and the stale JSDoc bullet reworded to the new rule. - unwrappedZodDefType now unwraps pipe nodes via their INPUT side (def.in — the side io: 'input' conversion and input validation consume; never def.out) and promise nodes via innerType, so z.bigint().transform(...), z.bigint().pipe(...), and z.promise(z.bigint()) roots throw the actionable 'must describe objects' error instead of becoming phantom {type: 'object'} tools. z.object({...}).transform(...) stays accepted, and bare standalone z.transform(fn) stays excluded like z.custom(). Co-Authored-By: Claude <noreply@anthropic.com>
…uired, catch-of-union skeleton
- The input-root guard now recurses into compositions via a new
nonObjectTypelessRootType helper: a union root is rejected when EVERY
member unwinds to a non-object typeless type (one representable
object member keeps it accepted), an intersection when ANY side does
(the value must satisfy both) — so z.union([z.bigint(), z.symbol()])
and z.intersection(z.bigint(), z.bigint()) throw the actionable root
error again instead of becoming phantom {type: 'object'} tools.
'nonoptional' joins the transparent-wrapper set (safe:
z.object({...}).nonoptional() emits an explicit type: 'object').
- fieldAcceptsMissingKey also returns true for fields whose unwrapped
def type is 'symbol' or 'function': JSON.stringify drops Symbol- and
function-valued keys from the payload by the same mechanism that
drops undefined-valued keys, so no serialized result can carry them
(covers both the object required-filter and the record branch).
- The .catch() degrade reduces composition keywords (anyOf/oneOf/allOf,
emitted when the catch wraps a union or intersection) to member type
skeletons instead of deleting them: the catch node emits no 'type'
key for these shapes, so the keep-type rule alone left the root
typeless and flipped the 2025-era legacy wrap for previously-working
registrations; the JSDoc bullet is updated to the full rule.
Audited sibling shapes: z.file() roots already throw via the explicit
type guard (representable as string/binary); z.never() and
fully/mixed-representable non-object unions were silently stamped
pre-PR too (status quo, not regressed here); catch-of-$ref roots were
typeless pre-PR as well (no wrap change) and now correctly drop the
unenforced $ref constraints; BigInt-valued output fields make
JSON.stringify throw (a transport-level ship failure already noted for
defaults in the changeset).
Co-Authored-By: Claude <noreply@anthropic.com>
…-only catch type
- Fixes a regression introduced by the composition recursion: 'literal'
in the reject set classified fully-representable members like
z.literal('admin') as non-object, so
z.union([z.literal('admin'), z.literal('member')]) — zod's idiomatic
enum spelling, which converted and listed fine pre-PR — threw and
failed the ENTIRE tools/list (and prompts/list) with -32603, the exact
whole-list outage this change eliminates. A composition member now
counts as non-object ONLY when it unwinds to a genuinely
unrepresentable type (bigint/symbol/map/set/void/undefined/nan/
function) or to a literal whose values are unrepresentable or
mixed-type; representable members (single-type literals, z.string(),
z.null()) keep the composition accepted exactly as pre-PR.
- Fixes the fail-open direction of the same recursion: the shared
seen-set conflated cycle protection with the member verdict, so
const b = z.bigint(); z.union([b, b]) returned a false may-be-object
verdict. The guard now tracks only the current traversal path
(ancestors), giving shared instances a real verdict at every
occurrence while recursive lazies stay bounded.
- The .catch() degrade keeps 'type' only when it is 'object': a
non-object type is an unenforced constraint that rejects the
wrong-typed raw values catch exists to tolerate, and the 2025-era
legacy-wrap object proof only ever consumes type === 'object' (same
rule in compositionTypeSkeleton). z.number().catch(0) now advertises
{default: 0}.
- Generalize the input-side residual-gap prose beyond z.date(): any
REQUIRED unrepresentable input field (z.bigint()/z.map()/z.set()/
z.symbol()) makes the tool listed yet uncallable via JSON — use
JSON-representable types or make the field optional (JSDoc +
changeset).
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Beyond the inline findings, this pass also examined whether mixed-type literal roots/unions (e.g. z.literal(['a', 1]) at root position) wrongly regress into a whole-tools/list throw — ruled out: such values are provably non-object (never callable via JSON), the root guard's loud rejection is the intended behavior for misregistered roots, and the PR's own tests pin both this rejection and the acceptance of representable literal unions (z.union([z.literal('admin'), z.literal('member')])).
Extended reasoning...
Bugs were found this run and are posted as inline comments, so no approval. This note records the one additional candidate examined and refuted this run — mixed-type literal roots failing the whole tools/list — so later review passes don't re-explore it from scratch. The refutation rests on the guard's every-member rule (representable members keep compositions accepted, pinned in this PR's tests) and on the loud-failure convention for provably non-object roots that earlier review rounds converged on. This is informational only, not a correctness guarantee.
…wind piped defaults
- Composition member classification: 'date' joins
NON_OBJECT_UNREPRESENTABLE_TYPES — a Date value can never be a JSON
object, and at member position the rewritten string/date-time node
nests inside anyOf/allOf where the explicit-type root guard cannot
see it, so z.union([z.date(), z.date()]),
z.union([z.date(), z.bigint()]), and
z.intersection(z.object({...}), z.date()) were stamped as phantom
{type: 'object'} tools (pre-PR each threw loudly). Bare date ROOTS
are unaffected (they throw via the explicit-type guard after the
rewrite). isNonObjectTypelessLiteral now also rejects non-finite
number literal values (Infinity/-Infinity/NaN are typeof 'number'
but cannot ride JSON). Mixed unions with a representable member
(z.union([z.date(), z.string()])) stay accepted under the
every-member rule.
- fieldAcceptsMissingKey's structural default short-circuit read only
the outermost def type, missing a default hidden inside a pipe:
z.number().default(7).transform(async v => v) is outer-'pipe' with
the ZodDefault at def.in, so the probe went async and the field
wrongly stayed in the advertised output required list while
async-capable validation fills the default. A new
hasStructuralDefault helper unwinds pipe INPUT sides, lazies, and
transparent wrappers looking for a default/prefault node (verified:
every wrapper in the set fills the inner default on undefined).
Co-Authored-By: Claude <noreply@anthropic.com>
…table literals; broaden tolerance detection
- oneOf skeletons: catch-of-discriminated-union output schemas (zod
emits DUs as oneOf) kept the oneOf keyword while members reduced to
identical {type: 'object'} skeletons, so every legitimate payload
matched BOTH members and Ajv rejected 'must match exactly one schema
in oneOf' — a strict regression vs pre-PR where the constrained
members were disjoint. The degrade loop and compositionTypeSkeleton
now emit oneOf skeletons under anyOf, the honest loosening once the
discriminating constraints are stripped (wrap-neutral:
isProvablyObjectShapedRoot treats the composition keywords
identically).
- isNonObjectTypelessLiteral: drop the jsonTypes.size > 1 branch — a
mixed-but-REPRESENTABLE literal (z.literal(['a', 1]) emits a valid
{enum: ['a', 1]}) listed silently pre-PR, and classifying it
non-object made one such registration fail the entire tools/list, a
round-10-introduced fail-closed regression. Only genuinely
unrepresentable values (undefined/bigint/symbol/non-finite numbers)
reject now, matching the loudness-parity contract.
- hasStructuralDefault -> hasStructuralMissingKeyTolerance: recognize
static .catch() BEFORE the wrapper unwind would step past it (any
input, including undefined, is replaced by the fallback — tolerance
by construction), and recurse into union def.options with ANY-member
semantics (a union accepts a missing key whenever any member does;
intersections deliberately excluded). Fixes catch/union-nested
defaults with async stages staying in the advertised output required.
Co-Authored-By: Claude <noreply@anthropic.com>
…t verdicts - hasStructuralMissingKeyTolerance now also recognizes: symbol/function leaves (JSON.stringify drops such keys, now detected through union members and the record call site, not just single-child chains), any/unknown leaves (undefined-accepting by construction — previously only rescued by the sync probe, so an async stage kept them required), and defaults at a pipe's OUTPUT side (z.preprocess(fn, z.number().default(7)) puts the transform at def.in and the tolerant node at def.out). All checks err loosen-only; the now-redundant unwrappedZodDefType carve-out is removed. The throwing-transform crash pin keeps exercising the probe hardening via a z.custom pipe (no structural verdict), while the z.unknown-piped field now counts tolerant structurally. - The input-root guard's composition verdicts split into loud vs quiet (nonObjectTypelessRootVerdict + nonObjectLiteralLoudness): loud shapes made pre-#2464 conversion throw (bigint/symbol/date/..., literals with undefined/bigint/symbol values) and keep throwing; non-finite number literals (Infinity/-Infinity/NaN) are non-object but zod silently emits {type: 'number', const: null} for them, so compositions built SOLELY of non-finite literals — which listed silently pre-#2464 — keep listing instead of failing the whole tools/list. z.union([z.literal(Infinity), z.bigint()]) stays loud via the bigint co-member (pinned). Co-Authored-By: Claude <noreply@anthropic.com>
…ion tolerance
- nonObjectTypelessRootVerdict's pipe branch mirrored the opposite-pipe
unwind already present in hasStructuralMissingKeyTolerance: when
def.in unwinds to a 'transform' node with no verdict, the guard now
recurses def.out — so z.preprocess(v => v, z.bigint()) roots (and
such members inside compositions) throw the actionable 'must describe
objects' error again instead of being stamped as phantom
{type: 'object'} tools. Object-rooted preprocess stays accepted
(explicit type: 'object' never reaches the guard) and
z.preprocess(fn, z.date()) keeps throwing via the explicit-type
guard.
- hasStructuralMissingKeyTolerance now recognizes: 'optional' as
tolerance-by-construction BEFORE the wrapper unwind would step past
it (z.string().optional().transform(async v => v ?? 'x') stays in
zod's output-io required while validation tolerates absence — the
same bug class as the fixed catch case); 'void'/'undefined' leaves
and literals whose value list includes undefined; and intersections
with EVERY-side semantics (undefined must parse through BOTH sides —
each default fills and zod merges the results). All loosen-only;
covers the record call site via the shared predicate.
Co-Authored-By: Claude <noreply@anthropic.com>
…des; fix stale linkcode
- A discriminated union whose DISCRIMINATORS are .catch()-wrapped (legal
zod; catch-wrapped members are rejected at construction) regressed:
the nested catch degrade stripped each discriminator's type/const and
the required-filter dropped 't', making the members mutually
satisfiable while the untouched parent DU node kept oneOf — so Ajv
(including the SDK's own Client.callTool re-validation) rejected
every legitimate payload with 'must match exactly one schema in
oneOf'. zodConversionOptions now takes a per-conversion loosened flag
set by the catch degrade and by required-filter drops (object and
record branches); when an output conversion was loosened anywhere,
the epilogue rewrites every emitted oneOf to anyOf (loosen-only and
wrap-neutral — isProvablyObjectShapedRoot treats the composition
keywords identically). Untouched schemas are not loosened: a DU
without degraded nodes keeps its oneOf (pinned).
- Fix the stale {@linkcode isNonObjectTypelessLiteral} reference on
NON_OBJECT_UNREPRESENTABLE_TYPES — the loud/quiet refactor renamed
the helper to nonObjectLiteralLoudness.
Co-Authored-By: Claude <noreply@anthropic.com>
knoal
left a comment
knoal
left a comment
There was a problem hiding this comment.
Reviewing via MCE A/B pilot 7 (sophia@hermes.local).
Summary
Substantial change: ~336 lines added to standardSchema.ts plus changes across core-internal and server packages. Makes zod-to-JSON-Schema conversion wire-truthful for tool schemas — z.date() is now advertised as {type: 'string', format: 'date-time'} rather than throwing and failing the entire tools/list response.
What's good
- The fix is correct and the changeset is exemplary — documents 6 known residual gaps honestly (BigInt defaults still throw, zod 4.0-4.2.x sanitization hook skipped, InMemoryTransport passes Date by reference, output .transform() ships pre-transform value, etc.). The "honest disclosure of gaps" is exactly the right tone.
- Aligns with StdioServerTransport's existing pattern — symmetric handling.
⚠️ Pre-merge consideration
Per MCP TS SDK's CONTRIBUTING.md:
"Please open an issue before starting work on new features or significant changes. ... We'll close PRs for undiscussed features — not because we don't appreciate the effort, but because every merged feature becomes an ongoing maintenance burden for our small team of maintainers."
This PR is 1.2k LOC, changes wire-format behavior across multiple modules, and is marked patch (not minor) — a significant change in a small package's resource budget.
Recommend: confirm the PR has a linked design discussion (issue, SEP, or PR comment thread) about the wire-truthful direction. If yes, this is APPROVE. If no, consider requesting the author open a tracking issue before merge.
APPROVE_WITH_CAVEATS — the fix is sound, but the policy gate around it is real.
— sophia
2 participants
_Requested by Felix Weinberger
Before / After
Before: a single
z.date()(or any type zod can't represent in JSON Schema, e.g.z.bigint()) in any registered tool's schema made the entiretools/listrequest fail withMCP error -32603: Date cannot be represented in JSON Schema— every tool on the server disappeared. Separately, output schemas were advertised with constraints the server never enforces on the rawstructuredContentit ships:.default()-carrying fields were listed asrequiredand plainz.object()outputs carriedadditionalProperties: false, so validating clients (including the SDK's ownClient.callTool) rejected perfectly legitimate tool results with errors likedata must have required property 'counted', data must NOT have additional properties.After:
tools/listsucceeds —z.date()is advertised as{"type": "string", "format": "date-time"}(the shapeJSON.stringifyactually puts on the wire for aDate, matching the zod v3 converter), and other unrepresentable types degrade to an unconstrained schema instead of throwing. Advertised output schemas now describe the raw payload the server actually ships: defaulted fields are optional, andadditionalProperties: falseis only kept where zod really enforces it (z.strictObject()), so validstructuredContentpasses client-side validation.How
The conversion in
packages/core-internal/src/util/standardSchema.ts(standardSchemaToJsonSchema) called zod's converter with hardcoded options:~standard.jsonSchema[io]({ target })on the primary path andz.toJSONSchema(schema, { target, io })on the zod 4.0–4.1 fallback. This PR adds azodConversionOptions(io)helper —unrepresentable: 'any'plus anoverridehook that rewritesZodDatenodes tostring/date-timeand, forio: 'output'object nodes, drops non-strictadditionalProperties: falseand removes defaulted properties fromrequired. It is passed vialibraryOptionson the Standard JSON Schema path (gated tovendor === 'zod', so other Standard Schema vendors are untouched) and spread into the fallback call. This mirrors the option semantics of the v1.x fix in #2467, adapted to the v2 layout's standard-schema conversion path.Tests: schema-level regression tests in
packages/core-internal/test/util/standardSchema.test.ts(plus a fallback-path test), and end-to-end server tests inpackages/server/test/server/toolSchemaWireShape.test.tsthat verifytools/listsucceeds with az.date()tool registered and that shippedstructuredContent(omitting a defaulted field, carrying an extra key) validates against the advertisedoutputSchemawith the SDK's own Ajv validator — all of which fail without the fix. A patch changeset forcore-internalandserveris included.Fixes #2464
Generated by Claude Code