feat(core): add isSpecType / specTypeSchemas Records for runtime validation of spec types#1887
Conversation
🦋 Changeset detectedLatest commit: 55d795b 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/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, additive, follows established patterns.
Extended reasoning...
Overview
This PR adds five new type-guard functions (, , , , ) to packages/core/src/types/guards.ts, exports them via the public API barrel, and adds 29 unit tests. It also corrects a pre-existing issue where guards.test.ts lived under src/types/ and was never executed by vitest (which only scans test/**).
Security risks
None. These are pure read-only type predicates that delegate to Zod's safeParse — no side effects, no I/O, no auth-sensitive paths.
Level of scrutiny
Low. Each new guard is a single-line lambda following the exact same pattern as the existing isCallToolResult and other guards already present in the file. The public-index export list is alphabetized correctly. The test coverage is thorough (valid inputs, missing required fields, non-object primitives).
Other factors
The only reported issue is a nit: CLAUDE.md's co-location guideline contradicts the vitest include pattern. This is a pre-existing doc inconsistency and is already flagged as an inline comment on the PR — it does not affect the correctness of the code changes. No outstanding human-reviewer comments exist. The changeset correctly marks downstream packages as patch releases.
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates
branch
2 times, most recently
from
1219736 to
06882c8
Compare
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates
branch
from
a08445c to
98091e7
Compare
|
@claude review |
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates
branch
from
193cb96 to
226f06e
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates
branch
2 times, most recently
from
58ecb98 to
452a903
Compare
Adds specTypeSchema(name) returning a StandardSchemaV1 validator for any named MCP spec type, and isSpecType(name, value) as a boolean predicate. The SpecTypeName union and SpecTypes map are derived from the internal Zod schemas, so they cover every spec type with no curation. Replaces the earlier curated-5-predicates approach with a single keyed entrypoint that does not require a new export each time an extension embeds a different spec type. Also: moves guards.test.ts to test/ (vitest include is test/**/*.test.ts; the file was previously dead) and corrects CLAUDE.md test-location guidance.
specTypeSchema()/isSpecType() now also cover the OAuth/OpenID types from shared/auth.ts (OAuthTokens, OAuthMetadata, OAuthClientMetadata, etc.), addressing the standalone-validation use noted in #1680 for auth implementers.
…cTypeSchema()/isSpecType()
- Explicit auth-schema allowlist (excludes SafeUrl/OptionalSafeUrl/IdJagTokenExchangeResponse helpers) - Guard predicate types value as schema input (z.input), not output, since safeParse only proves input shape - SchemaRecord typed as StandardSchemaV1<In, Out> so validate() output is the spec type - JSDoc/migration examples await validate() (Result | Promise<Result>); drop stale setCustom* refs
…-checked @example snippets - Replace import-* derivation with explicit SPEC_SCHEMA_KEYS tuple (150 entries with a matching public type in types.ts). Excludes internal helper schemas (ListChangedOptionsBase, BaseRequestParams, NotificationsParams, Client/ServerTasksCapability, ResourceTemplate name mismatch). - Add test asserting internals are not in SpecTypeName. - Bump changeset patch -> minor (5 new public exports). - Move @example code into specTypeSchema.examples.ts and source via sync:snippets, per repo convention.
There was a problem hiding this comment.
All earlier feedback is addressed and I have nothing new to flag — deferring to a human for sign-off on the new public API surface (isSpecType / specTypeSchemas / SpecTypeName / SpecTypes / StandardSchemaV1) and the hand-maintained SPEC_SCHEMA_KEYS allowlist.
Extended reasoning...
Overview
Adds isSpecType and specTypeSchemas records (plus SpecTypeName, SpecTypes, StandardSchemaV1 types) to core/public for runtime validation of any MCP spec type by name — the v2 replacement for the removed *Schema Zod constant exports. Implementation is a new ~300-line specTypeSchema.ts driven by a 150-entry hand-maintained SPEC_SCHEMA_KEYS allowlist plus an explicit OAuth-schema pick-list. Also: type-checked .examples.ts, comprehensive vitest coverage (runtime + expectTypeOf), z.ZodType<T, T> annotations on the recursive JSON schemas, migration-doc updates, a CLAUDE.md test-location correction, and a guards.test.ts move from src/ → test/.
Prior review rounds
This PR has been through six bot review cycles; every flagged item is now resolved in the current diff: explicit auth pick-list (no SafeUrl/OptionalSafeUrl leakage), explicit protocol allowlist (no ListChangedOptionsBase leakage), ResourceTemplate included, GuardRecord narrows to z.input<> (sound for .default()/.coerce()), JSON*Schema annotated with both type params so isSpecType.JSONObject narrows, await on validate() examples, .parse() parenthetical dropped, non-existent setCustomRequestHandler reference removed, JSDoc reworded to "satisfies the schema's input type", changeset bumped to minor. The current bug-hunting pass found nothing new.
Security risks
None identified. The change is additive type/validator plumbing with no I/O, auth, or transport logic. The OAuth schemas referenced are existing internal Zod objects re-exposed behind StandardSchemaV1; no new parsing of untrusted input is introduced beyond what callers explicitly opt into.
Level of scrutiny
Medium-high — not because the code is risky, but because it expands the curated public API by five symbols and commits to a specific shape (record-keyed predicates/validators, StandardSchemaV1 as the public validator type, a hand-maintained 150-entry allowlist that future spec additions must remember to extend). Per REVIEW.md ("burden of proof is on addition", "every new export is intentional", "removing public API is far harder than not adding it"), that's a design call for a human maintainer, not a bot.
Other factors
Test coverage is solid (positive/negative type-level assertions, runtime guards, filter-callback narrowing, internal-schema exclusion, ResourceTemplate ↔ ResourceTemplateType equivalence). Docs and changeset are consistent with the implementation. Large chunks of the migration*.md diff are prettier table-realignment noise. Given the iterative correctness fixes this PR needed and the public-surface implications, I'm deferring rather than approving.
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates
branch
from
42062b3 to
7e199ae
Compare
…emas with defaults The previous 'narrows the value type' test asserted against SpecTypes['Implementation'] (the output type), which only passed because Implementation has no defaults. Clarified that case and added a CallToolResult case that proves the documented input-type narrowing: content is optional in the narrowed type, and the narrowed type is not CallToolResult.
specTypeSchemas.X is typed as StandardSchemaV1, which only exposes ['~standard'].validate. The .parse() method exists at runtime (Zod) but is not on the public type, so the parenthetical produced a TS error if followed.
ResourceTemplate is a first-class spec type with a public type export (ResourceTemplateType). It was excluded under the assumption that the name collides with the server package's ResourceTemplate class, but isSpecType.ResourceTemplate is a record property, not a top-level export, so no collision occurs. Including it makes the migration docs' 'every named type in the MCP spec' claim accurate. Corrected the allowlist comment (ResourceTemplate is not an internal helper) and added type-level and runtime tests.
…ype describe block
JSONValueSchema/JSONObjectSchema/JSONArraySchema were typed as z.ZodType<T> (output only). Zod v4's ZodType<O, I> defaults I to unknown, so z.input<typeof JSONValueSchema> was unknown and isSpecType.JSONValue was (v: unknown) => v is unknown, a no-op predicate. Annotating as z.ZodType<T, T> fixes the input type at the source (these schemas have no defaults/transforms, so input equals output). Test asserts the narrowing.
There was a problem hiding this comment.
All three prior 🔴 threads are now addressed by e7aff86–dbd3555 (ResourceTemplate in the allowlist + tests, two-param z.ZodType<T,T> on the JSON* schemas with a narrowing type-test, and the .parse() parenthetical dropped). What remains is a one-word changeset nit (inline) — leaving final sign-off on the public SpecTypeName allowlist to a maintainer since it locks in ~160 names as published API.
Extended reasoning...
Overview
Adds isSpecType / specTypeSchemas records plus the SpecTypeName / SpecTypes types to core/public, backed by an explicit 151-entry SPEC_SCHEMA_KEYS allowlist plus 11 OAuth schemas. Touches schemas.ts (two-param z.ZodType annotations on the recursive JSON schemas), migration docs, CLAUDE.md, a relocated guards.test.ts, a new type-checked .examples.ts, and a new test file with both runtime and expectTypeOf assertions.
Security risks
None identified. The new surface is read-only validators over existing Zod schemas; no auth, transport, or I/O paths change. The schemas.ts edit only adds an Input type-param to three annotations (no runtime behaviour change).
Level of scrutiny
High — this is new public API. SpecTypeName is a published literal union; once shipped, removing a member is a breaking change. The repo's review conventions put the burden of proof on additions and ask that every export be intentional, so the final allowlist composition is a maintainer call rather than something I should approve unilaterally.
Other factors
- All three unresolved 🔴 inline comments from my 2026-04-29 review are now fixed on HEAD (dbd3555) and locked in by tests (
isSpecType.JSONObjectnarrowing,SpecTypes['ResourceTemplate'] ↔ ResourceTemplateType, ResourceTemplate runtime guard). - The one new finding is a nit: the changeset lists
StandardSchemaV1as a new export, but that line is unchanged context inexports/public/index.ts— CHANGELOG-attribution only, zero runtime impact. - Test coverage is good: positive/negative membership, input-vs-output narrowing for
.default()schemas, filter-callback usage, and exclusion of internal helpers are all asserted.
It was already exported from core/public on main; this PR only adds isSpecType, specTypeSchemas, SpecTypeName, and SpecTypes.
There was a problem hiding this comment.
All prior review feedback has now landed (55d795b addressed the last changeset nit) and I have nothing further to flag. Deferring final sign-off to a maintainer since this adds four new public exports plus a ~160-entry SpecTypeName literal union to the curated API surface — worth a human eye on the allowlist scope and the isSpecType input-type-narrowing semantics before it ships.
Extended reasoning...
Overview
Adds isSpecType / specTypeSchemas records plus the SpecTypeName / SpecTypes types to @modelcontextprotocol/core/public (re-exported by client and server). Backed by an explicit 151-entry SPEC_SCHEMA_KEYS allowlist in specTypeSchema.ts plus an 11-entry OAuth pick-list, with type-checked .examples.ts, runtime + type-level tests, migration-guide updates, a CLAUDE.md test-location correction, and a z.ZodType<T, T> annotation fix on the recursive JSON schemas. Also relocates the previously-dead guards.test.ts into test/.
Security risks
None identified. The new surface is read-only (frozen records of Zod validators / predicates derived from existing internal schemas). No auth, transport, or wire-protocol behaviour changes; the schemas.ts edit only adds an Input type parameter to three explicit annotations.
Level of scrutiny
High — this is net-new public API on the curated core/public surface, and per REVIEW.md the burden of proof is on addition. The SpecTypeName literal union (~160 members) becomes a compatibility commitment, and the isSpecType guards intentionally narrow to z.input<> rather than the named output type (documented in JSDoc, but a design choice a maintainer should ratify). The hand-maintained allowlist also warrants a human spot-check against types.ts.
Other factors
This PR has been through seven bot review rounds; every finding (auth-helper leakage, ListChangedOptionsBase leakage, output-vs-input narrowing soundness, JSON*Schema no-op predicates, ResourceTemplate omission, non-typechecking examples, doc references to nonexistent APIs, changeset bump level/prose) has been addressed and is covered by tests in specTypeSchema.test.ts. No human reviewer has weighed in yet, and the repo guidance ("every new export is intentional") makes maintainer sign-off the right gate here rather than bot approval.
KKonstantinov
left a comment
KKonstantinov
left a comment
There was a problem hiding this comment.
LGTM.
A few nits, for a follow up PR:
as unknown asusages could be avoided- good to add a test which checks if all schemas are added to the
SPEC_SCHEMA_KEYS, so there's a test failing when someone adds a new schema in the future
2 participants
Part of the v2 backwards-compatibility series — see reviewer guide.
Adds
specTypeSchema(name)/isSpecType(name, value)for runtime validation of any spec type, including OAuth metadata schemas. The v2-native replacement for importing Zod*Schemaconstants directly.Motivation and Context
v2 stopped exporting Zod schema constants. Consumers validating HTTP boundary data (OAuth token responses, OIDC discovery, custom transport frames) lost runtime validators.
specTypeSchema('OAuthTokens')returns the schema;isSpecType('OAuthTokens', x)is a type predicate.How Has This Been Tested?
pnpm typecheck:all && pnpm test:allgreenBreaking Changes
None — additive.
Types of changes
Checklist
Additional context
Stacks on: none. The companion
/server/zod-schemassubpath PR re-exports the raw constants for consumers who need the Zod object directly.