fix: convert elicitation Standard Schemas via an explicit wire-grammar walk#2454
Conversation
|
| Name | Type |
|---|---|
| @modelcontextprotocol/core-internal | Minor |
| @modelcontextprotocol/server | Minor |
| @modelcontextprotocol/express | Major |
| @modelcontextprotocol/fastify | Major |
| @modelcontextprotocol/hono | Major |
| @modelcontextprotocol/node | Major |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/elicitation-minimal-fixes
branch
from
998367c to
23e8f6a
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/elicitation-minimal-fixes
branch
from
23e8f6a to
257dc98
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs in the wire-grammar walk or the format-pattern handling, but this reworks the entire elicitation Standard Schema conversion mechanism (grammar derivation, per-vendor pattern trust, new rejection semantics), so the design decisions here deserve a human look.
Extended reasoning...
Overview
This PR replaces the parse-and-diff gate for Standard Schema elicitation conversion with an explicit walk of the converted JSON Schema against a wire grammar derived at module load from the Zod wire schemas (packages/core-internal/src/shared/elicitation.ts), adds a format-companion-pattern classifier in packages/core-internal/src/util/standardSchema.ts, a small shapeKeys util, docs/JSDoc/changeset updates, and a substantial new test matrix (unit + integration with real ArkType/Valibot schemas).
Security risks
Low. The changes affect server-side outbound request construction (what requestedSchema ships in elicitation/create), not inbound untrusted input handling. The main risk class is silently weakening an advertised constraint (dropping a user's custom pattern), which the PR explicitly guards against by rejecting customized zod patterns; non-zod vendors' companion patterns are trusted-and-dropped by design, which is a documented policy choice rather than a bug. The __proto__/dangling-required guard is a hardening improvement.
Level of scrutiny
This is core-internal SDK logic with several judgment calls a maintainer should weigh: deriving the wire grammar reflectively from the Zod schemas (.shape, .unwrap().options), trusting non-zod vendors' format-companion regexes as droppable, generating zod reference patterns at runtime, and the new reject-vs-drop semantics for root keywords. None of these looked incorrect to me — the walk's key sets match the wire schemas in schemas.ts, the residual dropped-constraint check remains as a safety net, and the behavior matrix is well pinned in tests — but the mechanism is intricate enough that it falls well outside "simple and obviously correct".
Other factors
The PR targets the in-progress Standard Schema elicitation branch (follow-up to #2369) rather than main, so nothing here is released behavior. Test coverage is thorough (format variants, rejection paths, vendor routing, extension-key parity, real ArkType/Valibot integration), and the docs/changeset accurately describe the implemented behavior. No bugs were found by the automated review; deferring solely because the design/mechanism decisions warrant maintainer judgment.
felixweinberger
force-pushed
the
fweinberger/elicitation-minimal-fixes
branch
from
257dc98 to
ddd3c82
Compare
…r walk
Replaces the parse-then-diff gate with a walk of the converted schema
against the wire grammar, derived at module load from the wire zod
schemas (root keys, per-type property key sets, the format enum) so a
spec revision updates it automatically. Each key is kept, dropped as
annotation-only, or rejected with its exact path; value validity stays
owned by the wire schemas, with a residual check rejecting key
combinations no single wire shape can carry. Behavior fixes riding on
the mechanism:
- The natural zod format forms work: z.email(), z.iso.date(), and
z.iso.datetime() emit a format-check regex beside the format that the
wire cannot carry; the library's own pattern is dropped as redundant.
For zod the exact patterns are derived from the resolved zod at
runtime (never vendored, so in-range zod upgrades cannot break them)
and a customized z.email({ pattern }) still rejects rather than
silently weakening the advertised constraint. Other vendors'
realizations are unknowable, so a pattern beside a matching retained
format is trusted as the library's own — ArkType's string.email now
converts (pinned with real ArkType and Valibot in the integration
suite).
- Unknown root keywords reject instead of leaking onto the wire: the
wire schema's root catchall let z.strictObject()'s
additionalProperties: false (and $defs, etc.) through; the root is
now held to the spec shape, with annotation-only root keywords
dropped.
- Schema detection routes on isStandardSchema rather than
isStandardSchemaWithJSON, matching normalizeRawShapeSchema, so zod
4.0/4.1 schemas reach the converter's fallback instead of shipping
zod internals raw.
- Rejection errors name the offending paths concisely (previously a
multi-kilobyte zod union dump buried the property name); parsing is
scoped to the converted schema so params-level fields behave exactly
as on the raw branch, which also keeps unknown params extension keys.
- Converted schemas whose required entries name properties that
properties does not define (zod's toJSONSchema does this for a
__proto__ property) reject instead of shipping a corrupt schema.
- The elicit JSDoc and the input-required guide document the conversion
contract: accepted forms, what rejects, and that responses are
validated by re-passing the schema to acceptedContent().
Tests pin the full behavior matrix through inputRequired.elicit —
formats and their variants, customized-pattern and literal-union
rejections, root keyword and annotation handling, function-typed and
validate-only schema routing, multi-select/optional/default shapes,
params extension-key parity, and real ArkType/Valibot conversion in the
integration suite.
felixweinberger
force-pushed
the
fweinberger/elicitation-minimal-fixes
branch
from
ddd3c82 to
591450f
Compare
There was a problem hiding this comment.
Both points from my earlier review are addressed in the latest revision — the Object.hasOwn guard on the type-key lookup (with a pinning test) and the doc/changeset caveats for inclusive-only number bounds and the non-zod format-pattern trust rule — and I found no further issues. Deferring to a maintainer since this reworks the core-internal conversion mechanism and its accept/reject contract, which warrants human sign-off.
Extended reasoning...
Overview
This PR replaces the parse-and-diff gate for Standard Schema elicitation conversion with an explicit walk of the converted schema against a wire grammar derived at module load from the wire zod schemas (elicitation.ts), plus supporting utilities (shapeKeys, isLibraryFormatPattern with runtime-derived zod reference patterns), routing changes (isStandardSchema instead of isStandardSchemaWithJSON), new guards (root-keyword rejection, dangling-required corrupt-conversion check), and extensive test coverage in both the core-internal unit suite and the integration suite (real ArkType and Valibot schemas).
Status of prior review feedback
My earlier inline comments flagged two nits: (1) prose over-promising the conversion contract (unconditional customized-pattern rejection; "numbers and their bounds" without the exclusive-bound caveat) and (2) the PROPERTY_KEYS_BY_TYPE lookup resolving through Object.prototype for a type like 'constructor'. Both are resolved in the current revision: the docs, JSDoc, and changeset now state the inclusive-bounds limitation and the non-zod trusted-and-dropped pattern behavior, and the lookup is guarded with Object.hasOwn with a dedicated test. The bug hunting system found no issues on this revision.
Security risks
Low. The code runs server-side at request-build time on developer-supplied schemas, not on untrusted client input; the walk fails closed (reject rather than ship) on anything outside the wire grammar, and the __proto__/dangling-required and prototype-chain hazards are explicitly handled and tested. Response content remains validated separately by acceptedContent() with the original schema, so a weakened advertised schema cannot bypass server-side validation.
Level of scrutiny
This is core protocol conversion logic with a deliberately chosen accept/reject contract (e.g. trusting non-zod format-companion patterns, deriving zod reference patterns from the resolved zod at runtime). Those are design tradeoffs a maintainer should endorse, not something to shadow-approve — the change is neither mechanical nor small, even though it is well-tested (behavior matrix written before the rework, real-library integration pins, full suites reported green).
Other factors
The changeset and documentation now accurately describe the shipped behavior, the derived-grammar approach makes the gate track spec revisions automatically, and error messages now name exact offending paths. No outstanding reviewer comments remain unaddressed.
1 participant
Follow-up for the reworked Standard Schema elicitation support, as a PR onto this branch so CI and review run before it lands in #2369. One commit: it fixes the outstanding conversion bugs and restructures the gate so this class of bug can't quietly return.
Motivation and Context
The mechanism. The previous gate worked by parsing the converted schema through the wire zod schema (which strips unknown keys) and diffing for what disappeared. That indirection is what made the recent bugs possible: the root catchall meant root keywords never showed up in the diff, zod's format-companion regexes showed up as false positives, and rejection errors were whatever zod's union parse produced (multi-KB dumps naming the offending property once, at the end).
This PR replaces it with an explicit walk of the converted schema against the wire grammar. The grammar — root keys, per-type property key sets, the supported format enum — is derived at module load from the wire zod schemas themselves, so a spec revision updates it automatically. The walk decides each key's fate (keep / drop annotation / reject with its exact path); value validity stays owned by the wire schemas; a residual check rejects key combinations no single wire shape can carry (e.g.
enumbesideminLength).The behavior fixes riding on it:
z.email(),z.iso.date(), andz.iso.datetime()emit a format-check regex beside theformatthat the wire can't carry — previously they all threw (masked in tests byz.string().meta({ format }), which emits no pattern and validates nothing). The library's own pattern now drops as redundant. For zod the exact patterns are derived from the resolved zod at runtime — never vendored, so an in-range zod upgrade can't silently break this — and a customizedz.email({ pattern })still rejects rather than silently weakening the advertised constraint. Other vendors' format realizations are unknowable from here, so a pattern beside a matching retained format is trusted as the library's own: ArkType'sstring.emailnow converts (pinned with real ArkType and Valibot schemas in the integration suite).z.strictObject()'sadditionalProperties: false(and$defs, registryids, …) ship inrequestedSchema; the spec declares a closed root shape, and the converted root is now held to it. Annotation-only root keywords (title/descriptionfrom.meta()) drop rather than reject.isStandardSchemarather thanisStandardSchemaWithJSON, matchingnormalizeRawShapeSchema: zod 4.0/4.1 schemas (validate but no~standard.jsonSchema) now reach the converter's fallback instead of shipping zod internals raw on the wire.z.union([z.literal('a'), z.literal('b')])now produces a 154-character error namingproperties.role, instead of a ~5KB zod union dump. Parsing is scoped to the converted schema, so params-level fields behave exactly as on the raw branch (no moremessage: 42blamed onrequestedSchema, and unknown params extension keys survive on both branches).requirednames a property thatpropertiesdoesn't define (zod'stoJSONSchemadoes this for a__proto__property) rejects instead of shipping.elicitJSDoc and the input-required guide state the accepted forms, what rejects, the supported formats, and that responses are validated by re-passing the schema toacceptedContent().How Has This Been Tested?
inputRequired.elicitand was written before the mechanism rework — every test passed unchanged across the restructure, which is the behavior-preservation argument: format variants (incl. offset/precision datetimes and the.meta({ format })style), customized-pattern and literal-union rejections, root keyword rejections and annotation drops, function-typed (ArkType-style) and validate-only schema routing, multi-select/optional/default/z.literal(['a','b'])shapes, params extension-key parity, and the__proto__guard.type({ email: 'string.email' })) and Valibot conversions run in the integration suite.pnpm check:all; manually drove the builder and the 2025 shim leg end-to-end overInMemoryTransportthrough the public package exports.Breaking Changes
None released — this adjusts behavior introduced on this branch. In-branch: the natural zod format forms now work instead of throwing; unknown root keywords and corrupt conversions reject instead of shipping; validate-only Standard Schemas route to the converter. The changeset names the accepted and rejected forms.
Types of changes
Checklist
Additional context
Known residual, documented in a code comment: the zod reference patterns come from the zod copy this package resolves, so an install with two zod copies whose format regexes diverged fails closed (a false rejection, never a bad wire message). Deferred follow-ups worth separate PRs: a CI leg testing the minimum supported zod peer version, converted-schema coverage in the conformance everything-server, and surfacing schema-validation failures distinctly from missing/declined in
acceptedContent.