[comparison] specTypeSchema/isSpecType as string-arg functions#1979
[comparison] specTypeSchema/isSpecType as string-arg functions#1979felixweinberger wants to merge 14 commits intomainfrom
Conversation
|
| Name | Type |
|---|---|
| @modelcontextprotocol/client | 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/server
@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.
No issues spotted in the implementation, but since this PR exists specifically for humans to compare two API shapes (record-keyed vs string-arg) and pick one, I'll leave the design call to a maintainer.
Extended reasoning...
Overview
This PR reshapes the not-yet-released isSpecType / specTypeSchemas API from #1887 into a string-arg function form: isSpecType('TypeName', value) and specTypeSchema('TypeName'). It touches the implementation in packages/core/src/types/specTypeSchema.ts, the public export barrel, the examples file, the test file, both migration docs, and the changeset. The diff targets the #1887 branch, so what's shown is purely the API-shape delta.
Security risks
None. This is a type-predicate / schema-lookup helper over an internal static table of Zod schemas; no I/O, auth, or untrusted-input handling is affected.
Level of scrutiny
The code change itself is small and mechanically correct — the overload signatures, the allSchemas population loop, the docs, tests, and changeset are all internally consistent, and the bug-hunting pass found nothing. However, the PR's stated purpose is "for side-by-side comparison" of two candidate public-API shapes. Per the repo's guiding principles (burden of proof on addition, one way to do things, kill at the highest level), choosing between record-property access and string-arg function is a design decision that a maintainer should make explicitly, not something to rubber-stamp.
Other factors
- The function form loses the standalone-callback ergonomics (
arr.filter(isSpecType.ContentBlock)→arr.filter(v => isSpecType('ContentBlock', v))), which the diff acknowledges by removing the eslint-disable and updating the docs/tests. Whether that trade-off is worth the simpler surface is the question this PR is asking. - The export rename (
specTypeSchemas→specTypeSchema) is a public-API change relative to #1887; whichever shape lands needs to be the one the team commits to before release. - Test coverage mirrors the previous shape and still asserts compile-time rejection of unknown/internal names and correct narrowing, so correctness parity is demonstrated.
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.
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.
…arg function form
Replaces the Record-of-predicates exports with two generic functions:
isSpecType('TypeName', value) and specTypeSchema('TypeName').
Drops the register() helper, the per-key closure creation (161 closures
at module init), the SchemaRecord/GuardRecord types, and both
Object.freeze(...) as unknown as casts. The single remaining cast is a
narrow Record<SpecTypeName, z.ZodType> at construction so indexing by
SpecTypeName is non-undefined under noUncheckedIndexedAccess.
Trade-off: arr.filter needs an arrow wrapper (mixed.filter(v =>
isSpecType('ContentBlock', v))). TypeScript's inferred type predicates
still narrow the result to ContentBlock[].
felixweinberger
force-pushed
the
fweinberger/spec-type-predicates-fn
branch
from
8f3d23b to
d7a5c96
Compare
| | Code | Description | | ||
| | ------------------------------------------------- | ---------------------------------------------- | | ||
| | `SdkErrorCode.NotConnected` | Transport is not connected | | ||
| | `SdkErrorCode.AlreadyConnected` | Transport is already connected | | ||
| | `SdkErrorCode.NotInitialized` | Protocol is not initialized | | ||
| | `SdkErrorCode.CapabilityNotSupported` | Required capability is not supported | | ||
| | `SdkErrorCode.RequestTimeout` | Request timed out waiting for response | | ||
| | `SdkErrorCode.ConnectionClosed` | Connection was closed | | ||
| | `SdkErrorCode.SendFailed` | Failed to send message | | ||
| | `SdkErrorCode.InvalidResult` | Response result failed local schema validation | | ||
| | `SdkErrorCode.ClientHttpNotImplemented` | HTTP POST request failed | | ||
| | `SdkErrorCode.ClientHttpAuthentication` | Server returned 401 after re-authentication | | ||
| | `SdkErrorCode.ClientHttpForbidden` | Server returned 403 after trying upscoping | | ||
| | `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response | | ||
| | `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream | | ||
| | `SdkErrorCode.ClientHttpFailedToTerminateSession` | Failed to terminate session | | ||
| | `SdkErrorCode.ClientHttpNotImplemented` | HTTP POST request failed | | ||
| | `SdkErrorCode.ClientHttpAuthentication` | Server returned 401 after re-authentication | | ||
| | `SdkErrorCode.ClientHttpForbidden` | Server returned 403 after trying upscoping | | ||
| | `SdkErrorCode.ClientHttpUnexpectedContent` | Unexpected content type in HTTP response | | ||
| | `SdkErrorCode.ClientHttpFailedToOpenStream` | Failed to open SSE stream | | ||
| | `SdkErrorCode.ClientHttpFailedToTerminateSession` | Failed to terminate session | |
There was a problem hiding this comment.
🟡 nit: Several hunks in docs/migration.md and docs/migration-SKILL.md are pure Prettier reformatting unrelated to the isSpecType/specTypeSchema change — this SdkErrorCode table re-padding, the items: [/* ... */] code-block reflow at 382-389/397-402, the prose re-wraps at 140-141/441-442, and the Types/shared-imports table re-padding in migration-SKILL.md. Since the PR's stated purpose is that the diff against #1887 "shows only the API-shape delta", it'd be worth running Prettier on the #1887 branch first so these whitespace hunks disappear from the comparison (you can't just drop them here — the format check would re-add them).
Extended reasoning...
What this is
This PR is explicitly a comparison artifact: the description says it "targets the #1887 branch so the diff shows only the API-shape delta." Its value to reviewers is that scrolling the diff should reveal exactly what changes when you swap isSpecType.X(v) / specTypeSchemas.X for isSpecType('X', v) / specTypeSchema('X') — and nothing else.
However, the diff contains several hunks that are pure Prettier reformatting with zero semantic relation to the API-shape change:
docs/migration.md:719-734— the entireSdkErrorCodetable is re-padded because one row (InvalidResult, "Response result failed local schema validation") is wider than the previous max, so every other row's trailing-space padding shifts. No content changed.docs/migration.md:382-389and397-402—return { items: [/* ... */] };is reflowed to a 5-line block. This code sample is in the §"Custom (non-spec) methods" section and has nothing to do withisSpecType.docs/migration.md:140-141and441-442— two prose paragraphs (the auth-helpers paragraph and therequest()schema-parameter paragraph) are re-wrapped at a different column. Content identical.docs/migration-SKILL.md— the "Types / shared imports" table is re-padded to a much wider second column, and the §8 auth paragraph is re-wrapped.
Step-by-step proof
Take the SdkErrorCode table hunk anchored on this line. Comparing old vs. new:
| old | new | |
|---|---|---|
NotConnected row |
Transport is not connected + 18 trailing spaces + ` |
` |
InvalidResult row |
`Response result failed local schema validation | ` (no extra padding) |
Every changed line in this hunk differs only in trailing-space count inside the table cell. Rendered Markdown is byte-for-byte identical. The same holds for the migration-SKILL.md table (the shared/stdio.js row's long parenthetical forces the column wider, and Prettier re-pads every other row to match). None of these hunks mention isSpecType, specTypeSchema, SpecTypeName, or anything else this PR is about.
Why existing tooling doesn't prevent it
It's the opposite — the repo's Prettier enforcement causes it. The base branch (#1887) evidently left these files in a non-Prettier-normalized state (likely a row was added to each table / a phrase was edited without re-running the formatter on the whole file). When this PR touched the same files to update the isSpecType examples, format-on-save / pnpm format normalized the surrounding content too, and those normalizations show up in the inter-branch diff.
Addressing the counter-argument
One could argue this isn't worth a comment because (a) the author can't simply drop these hunks — Prettier CI would fail or re-add them, (b) GitHub's ?w=1 whitespace toggle hides the table re-padding and prose re-wraps, and (c) the actionable fix lives on #1887, not here.
All three points are correct, and (a) in particular is why the suggested fix is not "drop these hunks from this PR" but rather "Prettier-clean the base #1887 branch and rebase." That is a one-command fix on #1887, after which this PR's diff collapses to exactly the API-shape delta it advertises. Point (b) mitigates but doesn't eliminate the noise: ?w=1 hides the table padding and prose wraps, but the items: [/* ... */] → multiline reflow at 382-389/397-402 still appears as a content change even with whitespace hidden, since it inserts newlines between tokens. And (c) is precisely why this is filed as a nit rather than a blocking issue — it's process feedback about the comparison's readability, surfaced here because this is where the noise is visible.
Impact and fix
No correctness impact whatsoever — rendered docs are identical. The only cost is reviewer attention: someone comparing the two API shapes has to mentally discard ~40 lines of table-padding diff and a couple of code-block reflows to find the ~15 lines that actually demonstrate the API difference. Given the PR exists specifically to make that comparison easy, tightening it up is worth a one-liner.
Fix: On the #1887 branch, run pnpm format (or equivalent) over docs/migration.md and docs/migration-SKILL.md and commit. Then rebase this branch; the formatting hunks vanish and only the isSpecType/specTypeSchema edits remain.
1 participant
Alternative API shape for #1887 using
isSpecType('TypeName', value)andspecTypeSchema('TypeName')instead of Record property access. For side-by-side comparison; targets the #1887 branch so the diff shows only the API-shape delta.