fix(server): surface raw-shape schema failures at registration time, restore mixed-zod guard#2243
fix(server): surface raw-shape schema failures at registration time, restore mixed-zod guard#2243felixweinberger wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 35303b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Raw shapes whose field schemas come from a different zod build than the
SDK bundles previously registered fine and then crashed tools/list with
'[toJSONSchema]: Non-representable type encountered'. normalizeRawShapeSchema
now runs the JSON Schema conversion eagerly on the auto-wrap path so the
failure is an actionable TypeError at registerTool/registerPrompt.
Shapes mixing zod v3 and v4 fields now throw
Error('Mixed Zod versions detected in object shape.') — the same
registration-time error v1's objectFromShape threw — instead of being
misreported as an all-v3 shape. The all-v3 message also gains the
fromJsonSchema() alternative.
…ools/list round trip The structural mocks proved the guard mechanism but not the real-world claim. A zod-v40 devDependency alias (npm:zod@4.0.17) now reproduces the actual serve-time conversion failure as a registration-time error, a native z.custom field covers the version-independent case, and an InMemoryTransport tools/list round trip verifies auto-wrapped native shapes advertise correct schemas (properties, required, descriptions) so the eager check has no false positives.
…x the zod-v3 remedy in the migration guides
felixweinberger
force-pushed
the
fweinberger/raw-shape-compat
branch
from
d2c757e to
35303b7
Compare
| test('native z.custom raw-shape field fails at normalization time (version-independent construct)', () => { | ||
| const shape = { c: z.custom<string>(v => typeof v === 'string') }; | ||
| expect(() => normalizeRawShapeSchema(shape as never)).toThrow(/could not be converted to JSON Schema/); | ||
| // The thrown message itself carries the converter's diagnosis, so the | ||
| // bundled-zod non-representable case is distinguishable from a foreign | ||
| // zod build without unwrapping `cause`. | ||
| expect(() => normalizeRawShapeSchema(shape as never)).toThrow(/Non-representable type|custom/i); | ||
| }); |
There was a problem hiding this comment.
🟡 The second .toThrow(/Non-representable type|custom/i) assertion is vacuous: the custom alternative matches the static boilerplate "e.g. z.custom()" that is always present in the TypeError thrown by normalizeRawShapeSchema, so this test passes even if the : ${detail} interpolation (the converter's own diagnosis) is removed from the message — contrary to what the accompanying comment claims is being verified. Tighten it to match only the converter's own text, e.g. .toThrow(/Non-representable type encountered/), or assert the dynamic detail explicitly.
Extended reasoning...
What the assertion claims to verify vs. what it actually verifies. The test 'native z.custom raw-shape field fails at normalization time (version-independent construct)' (packages/core/test/util/zodCompat.test.ts:145-152) carries the comment: "The thrown message itself carries the converter's diagnosis, so the bundled-zod non-representable case is distinguishable from a foreign zod build without unwrapping cause." The assertion meant to back that claim is expect(() => normalizeRawShapeSchema(shape as never)).toThrow(/Non-representable type|custom/i). But the TypeError thrown by normalizeRawShapeSchema (packages/core/src/util/zodCompat.ts:78-82) always contains the static boilerplate text "or when a field is not representable in JSON Schema (e.g. z.custom())" — and the case-insensitive custom branch of the alternation matches the literal z.custom() in that boilerplate on every invocation of this catch path, regardless of whether the dynamic ${detail} (the converter's own message) was interpolated.\n\nStep-by-step proof. (1) Suppose a future refactor drops the : ${detail} interpolation from the message at zodCompat.ts:78, leaving only the static prose. (2) The thrown message is now "Raw-shape inputSchema/outputSchema/argsSchema could not be converted to JSON Schema. This happens when ... or when a field is not representable in JSON Schema (e.g. z.custom()). ...". (3) The first assertion /could not be converted to JSON Schema/ still matches the static prefix. (4) The second assertion /Non-representable type|custom/i evaluates its alternation: Non-representable type no longer matches (the converter detail is gone, and the static text only says "not representable" / "non-representable fields", neither of which matches "non-representable type"), but custom matches z.custom() case-insensitively — so the assertion passes anyway. (5) The mutation goes undetected by this test, even though its comment says the message-carries-the-diagnosis property is exactly what it checks.\n\nWhy no other test catches it either. The only other test that inspects the converter's own error is 'raw shape from a real foreign zod build fails at normalization time, not serve time' (lines 154-165), and it does so via the cause chain — expect(String((e as TypeError).cause)).toMatch(/Non-representable type/) — not via the thrown message. The remaining conversion-failure tests in both packages match only the static prefix /could not be converted to JSON Schema/. So if the ${detail} interpolation were removed, no test in core or server would fail, despite the PR description's claim that mutation testing covers the eager check and that the registration-time error "carr[ies] the converter's own message".\n\nWhy this is a nit, not a blocker. The implementation is correct as shipped — detail is interpolated into the message — so there is no runtime bug here. The issue is purely that the second assertion adds zero verification power beyond the first one while its comment overstates what is verified, leaving the advertised behavior unprotected against regression. All four verifiers independently confirmed this and agreed on nit severity.\n\nFix. Drop the |custom alternation so the assertion matches only the converter's own diagnosis: .toThrow(/Non-representable type encountered/) (the bundled zod's toJSONSchema error for z.custom() contains that phrase, as the foreign-build test's cause assertion demonstrates for the analogous .optional() case). Alternatively, capture the thrown error and assert that its message contains String((e as TypeError).cause && (e.cause as Error).message) — i.e. assert the dynamic detail explicitly. Either change makes the test actually enforce what its comment promises.\n\nNote this is distinct from the existing inline comment on zodCompat.ts (about the runtime error's wording for the non-representable case) — that comment concerns the production message; this one concerns the test assertion that is supposed to verify the message.
1 participant
Surface raw-shape schema failures at registration time, and restore v1's mixed-zod-versions error.
Motivation and Context
This fixes two backwards-compatibility hazards that bite during a v1 → v2 migration of code using the raw-shape shorthand (
server.tool('greet', { name: z.string() }, cb)and the equivalentregisterToolconfig form). Both were observed while migrating a large production MCP host application from SDK v1 to v2.1. Serve-time
tools/listcrash instead of a registration-time error.A migrating application typically has its own zod installation (commonly zod 3.25.x using the
zod/v4subpath, or zod 4.0/4.1). Raw shapes built from that zod passisZodRawShape(the fields carry_zod), get auto-wrapped with the SDK-bundledz.object(), and register without error — then everytools/listcrashes:because the bundled converter cannot walk another zod build's field internals. The failure surfaces far from its cause, in a request handler, on every list call. v1 never had this failure mode: its zod peer-dependency model meant shape fields and the wrapping
z.object()always came from the same zod instance.With this change,
normalizeRawShapeSchemaruns the exact JSON Schema conversion that listing performs eagerly at registration, so the failure becomes an actionableTypeErrorfromregisterTool/registerPrompt(with the original error attached ascause), carrying the converter's own message and telling the user the fix for each cause: wrap with their own zod ≥ 4.2z.object()(foreign zod build), or pass JSON Schema viafromJsonSchema()(non-representable fields such asz.custom(), which fail the same way even on the bundled zod).2. Mixed zod-version shapes lost their dedicated error.
v1's zod-compat layer (
objectFromShape) had three-way dispatch: all-v4 → wrap, all-v3 → wrap with v3, mixed →throw new Error('Mixed Zod versions detected in object shape.'). In v2, a shape mixing v3 and v4 fields falls into the all-v3 branch and reports "Got a Zod v3 field schema" — a misleading diagnosis for a user whose imports are mostlyzod/v4(the actual problem is one strayzodclassic import). This restores v1's dedicated mixed-versions error, byte-identical so code and tests written against v1 keep matching.The all-v3 error message now also mentions the
fromJsonSchema()alternative.How Has This Been Tested?
packages/core/test/util/zodCompat.test.ts: mixed-version shape throws the exact v1 message (and not the all-v3 message); foreign-build v4 fields fail at normalization time with the underlying error preserved ascause; well-formed shapes still wrap and convert for bothinputandoutputpositions.zod-v40devDependency alias (npm:zod@4.0.17) provides an actual second zod install — raw shapes built from it reproduce the exact serve-time failure ([toJSONSchema]: Non-representable type encountered) and are now rejected at registration with that error preserved ascause. A nativez.custom()field covers the version-independent case the same way.packages/server/test/server/mcp.compat.test.ts:registerToolthrows at registration (nottools/call/tools/list) for the mixed, structural-mock, and real-second-install failure modes; a fulltools/listround trip overInMemoryTransportproves auto-wrapped native shapes still advertise correct schemas (properties,required, field descriptions) — the eager check introduces no false positives.core(559) andserver(76) suites pass; typecheck and lint clean.tools/listthrows) and is converted to a registration-time error by this change.Breaking Changes
None for valid schemas. Shapes that previously registered successfully and then crashed every
tools/listnow fail fast at registration — strictly earlier surfacing of an existing failure. Mixed-version shapes change error class fromTypeError(misleading message) toErrorwith v1's message.Types of changes
Checklist
Additional context
normalizeRawShapeSchemagains anio: 'input' | 'output'parameter so the eager conversion check matches the conversion the listing path will actually run for that schema position (outputSchemaconverts withio: 'output').