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
…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
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.
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.