| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
⚠️ No Changeset foundLatest commit: 591450f Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. This PR includes changesets to release 6 packages
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 |
Sorry, something went wrong.
|
@modelcontextprotocol/client
npm i https://pkg.pr.new/@modelcontextprotocol/client@2454
npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2454
npm i https://pkg.pr.new/@modelcontextprotocol/core@2454
npm i https://pkg.pr.new/@modelcontextprotocol/server@2454
npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2454
npm i https://pkg.pr.new/@modelcontextprotocol/express@2454
npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2454
npm i https://pkg.pr.new/@modelcontextprotocol/hono@2454
npm i https://pkg.pr.new/@modelcontextprotocol/node@2454 commit: 591450f |
Sorry, something went wrong.
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...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).
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.
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".
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.
Sorry, something went wrong.
…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.
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...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).
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.
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.
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).
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.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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. enum beside minLength).
The behavior fixes riding on it:
How Has This Been Tested?
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.