feat(compat): registerTool/registerPrompt accept raw Zod shape, auto-wrap with z.object()#1901
feat(compat): registerTool/registerPrompt accept raw Zod shape, auto-wrap with z.object()#1901felixweinberger wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: a1bf55a 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: |
felixweinberger
force-pushed
the
fweinberger/v2-bc-register-rawshape
branch
from
182ec53 to
9b606ab
Compare
felixweinberger
force-pushed
the
fweinberger/v2-bc-register-rawshape
branch
from
9b606ab to
2972af1
Compare
…wrap with z.object)
felixweinberger
force-pushed
the
fweinberger/v2-bc-register-rawshape
branch
from
2972af1 to
a1bf55a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export function isZodRawShape(obj: unknown): obj is Record<string, StandardSchemaV1> { | ||
| if (typeof obj !== 'object' || obj === null) return false; | ||
| if (isStandardSchema(obj)) return false; | ||
| const values = Object.values(obj); | ||
| return values.length > 0 && values.every(v => isStandardSchema(v) || (typeof v === 'object' && v !== null && '_def' in v)); |
There was a problem hiding this comment.
🔴 An empty raw shape {} is not auto-wrapped: isZodRawShape requires values.length > 0, so normalizeRawShapeSchema({}) returns {} as-is, and tools/list then throws TypeError: Cannot read properties of undefined (reading 'jsonSchema'). Since {} satisfies the new ZodRawShape overload type and v1 accepted inputSchema: {} for no-arg tools, drop the values.length > 0 guard ([].every(...) is vacuously true) so empty shapes wrap to z.object({}).
Extended reasoning...
What the bug is
isZodRawShape() (packages/core/src/util/standardSchema.ts:148-152) returns false for an empty object {} because of the values.length > 0 clause. Consequently normalizeRawShapeSchema({}) skips the auto-wrap branch and returns {} unchanged, cast to StandardSchemaWithJSON. But {} is not a valid Standard Schema — it has no '~standard' property — so any downstream consumer that dereferences schema['~standard'] will throw.
The code path that triggers it
- User calls
server.registerTool('ping', { inputSchema: {} }, async () => ...). This type-checks via the new raw-shape overload because{}satisfiesZodRawShape = Record<string, StandardSchemaWithJSON>(an empty record is assignable to anyRecord<string, T>). registerToolcallsnormalizeRawShapeSchema({})(mcp.ts:910).- Inside
normalizeRawShapeSchema:{}is notundefined, andisZodRawShape({})evaluatesObject.values({}) → [], then[].length > 0 → false, so it returnsfalse. The function falls through and returns{}typed asStandardSchemaWithJSON. - The tool is stored with
inputSchema: {}(truthy). - A client sends
tools/list. The handler (mcp.ts) checkstool.inputSchema ? standardSchemaToJsonSchema(tool.inputSchema, 'input') : EMPTY_OBJECT_JSON_SCHEMA. Since{}is truthy, it callsstandardSchemaToJsonSchema({}). standardSchemaToJsonSchemaexecutesschema['~standard'].jsonSchema[io](...).{}['~standard']isundefined, so reading.jsonSchemathrows TypeError: Cannot read properties of undefined (reading 'jsonSchema').
The same crash occurs on tools/call via validateToolInput → validateStandardSchema → schema['~standard'].validate, and on prompts/list/prompts/get for registerPrompt('p', { argsSchema: {} }, ...).
Why existing code doesn't prevent it
The only guards before the crash site are schema === undefined (in normalizeRawShapeSchema) and the truthiness check tool.inputSchema ? ... : EMPTY_OBJECT_JSON_SCHEMA (in the tools/list handler). {} passes both. There is no isStandardSchemaWithJSON validation between normalizeRawShapeSchema and standardSchemaToJsonSchema.
Why this matters for the PR's stated goal
This PR explicitly targets v1 compat for raw shapes. v1's ZodRawShape was z.ZodRawShape = { [k: string]: ZodTypeAny }, which admits {}, and registerTool('x', { inputSchema: {} }, cb) was a valid v1 pattern for declaring a no-args tool. The new TypeScript overload also accepts {}. So the compat shim accepts this input at the type level but crashes at runtime — exactly the kind of migration footgun the PR is meant to eliminate.
Step-by-step proof
const server = new McpServer({ name: 't', version: '1.0.0' });
server.registerTool('ping', { inputSchema: {} }, async () => ({
content: [{ type: 'text', text: 'pong' }]
}));
// type-checks ✓
// Simulate tools/list:
// isZodRawShape({}) → values=[], 0>0 false → returns false
// normalizeRawShapeSchema({}) → returns {} (not undefined, not raw shape)
// tool.inputSchema = {} → truthy
// standardSchemaToJsonSchema({}) → ({})['~standard'].jsonSchema.input(...)
// → TypeError: Cannot read properties of undefinedFix
Drop the values.length > 0 guard — Array.prototype.every on an empty array is vacuously true, so isZodRawShape({}) will return true and {} will be wrapped as z.object({}), which serializes to { type: 'object', properties: {} } (matching EMPTY_OBJECT_JSON_SCHEMA):
const values = Object.values(obj);
return values.every(v => isStandardSchema(v) || (typeof v === 'object' && v \!== null && '_def' in v));Alternatively, special-case Object.keys(schema).length === 0 in normalizeRawShapeSchema to return undefined (so the existing EMPTY_OBJECT_JSON_SCHEMA fallback applies). Either is a one-line change.
| /** | ||
| * A plain record of field schemas, e.g. `{ name: z.string() }`. Accepted by | ||
| * `registerTool`/`registerPrompt` as a shorthand; auto-wrapped with `z.object()`. | ||
| */ | ||
| export type ZodRawShape = Record<string, StandardSchemaWithJSON>; | ||
|
|
||
| /** Infers the parsed-output type of a {@linkcode ZodRawShape}. */ | ||
| export type InferRawShape<S extends ZodRawShape> = { [K in keyof S]: StandardSchemaWithJSON.InferOutput<S[K]> }; | ||
|
|
||
| /** {@linkcode ToolCallback} variant used when `inputSchema` is a {@linkcode ZodRawShape}. */ | ||
| export type LegacyToolCallback<Args extends ZodRawShape | undefined> = Args extends ZodRawShape | ||
| ? (args: InferRawShape<Args>, ctx: ServerContext) => CallToolResult | Promise<CallToolResult> | ||
| : (ctx: ServerContext) => CallToolResult | Promise<CallToolResult>; | ||
|
|
||
| /** {@linkcode PromptCallback} variant used when `argsSchema` is a {@linkcode ZodRawShape}. */ | ||
| export type LegacyPromptCallback<Args extends ZodRawShape | undefined> = Args extends ZodRawShape | ||
| ? (args: InferRawShape<Args>, ctx: ServerContext) => GetPromptResult | Promise<GetPromptResult> | ||
| : (ctx: ServerContext) => GetPromptResult | Promise<GetPromptResult>; |
There was a problem hiding this comment.
🟡 The PR's intent is internally inconsistent: the changeset says "Both forms are first-class" and the tests assert console.warn is not called, yet the PR description says "auto-wraps and warns" / "@deprecated shim. Removed in v3", and the new public types are named LegacyToolCallback/LegacyPromptCallback. Please pick one: either (a) restore the deprecation warning + @deprecated JSDoc and reword the changeset, or (b) drop the Legacy prefix from the type names. Relatedly, these types are not re-exported from packages/server/src/index.ts — fine if they're a deprecated shim, but if first-class they should sit alongside ToolCallback/PromptCallback.
Extended reasoning...
What's inconsistent
There are four artifacts in this PR that disagree about whether raw-shape support is a first-class feature or a deprecated v1-compat shim:
| Artifact | Says |
|---|---|
| PR description (twice) | "auto-wraps raw shapes and warns" / "additive @deprecated shim. Removed in v3" |
Changeset (.changeset/register-rawshape-compat.md) |
"Both forms are first-class." |
Tests (mcp.compat.test.ts) |
All 3 cases explicitly assert expect(warn).not.toHaveBeenCalled() |
Exported type names (mcp.ts:1122,1127) |
LegacyToolCallback / LegacyPromptCallback |
The code matches the changeset (no warning, both accepted equally), but the type names match the PR description (legacy/deprecated). The PR description and changeset directly contradict each other.
Step-by-step proof
normalizeRawShapeSchemainpackages/core/src/util/standardSchema.ts:163-171contains noconsole.warncall — it just wraps and returns.mcp.compat.test.ts:23,35,51each spy onconsole.warnand assert it was not called — so the absence of a warning is intentional and locked in by tests.- The new overload JSDoc at
mcp.ts:878andmcp.ts:958says only "Raw-shape form: ... auto-wrapped" — no@deprecatedtag. - Yet the callback types at
mcp.ts:1122,1127are exported asLegacyToolCallback/LegacyPromptCallback— "Legacy" signals deprecation to anyone reading the .d.ts. - The changeset (which becomes the published CHANGELOG entry) promises "Both forms are first-class."
So a consumer reading the changelog will see "first-class", but a consumer reading the generated types will see "Legacy". And a maintainer reading the PR description will expect a warning that doesn't exist.
Why this matters
The changeset text ships to npm as the changelog. The type names ship in the published .d.ts. Having these two disagree ("first-class" vs "Legacy*") sends mixed signals to consumers about whether they should migrate off raw shapes. And if the warning was intended (per the PR description) but accidentally dropped, that's a behavioral gap — users won't be nudged toward the v2-native form before v3 removes the shim.
Secondary: index.ts re-exports
packages/server/src/index.ts uses explicit named exports and currently re-exports ToolCallback/PromptCallback but not ZodRawShape/InferRawShape/LegacyToolCallback/LegacyPromptCallback. Whether that's correct depends on the answer above:
- If first-class: parity suggests they should be re-exported alongside
ToolCallback/PromptCallbackso users can type a callback variable before passing it. - If a deprecated shim: omitting them from the package index is the right minimalist call (overload resolution + inference covers the common case, and there's no reason to grow the public surface for a shim).
I'm not asserting the omission is wrong — just that it should be a deliberate consequence of resolving the intent question, not an accident.
Suggested fix
Pick a lane:
Option A — deprecated shim (matches PR description):
- Add
console.warninnormalizeRawShapeSchema(or at theregisterTool/registerPromptcall sites). - Add
@deprecatedto the raw-shape overload JSDoc. - Reword the changeset to drop "first-class" and say "accepted for v1 compatibility; prefer
z.object()". - Update the tests to expect the warning.
- Keep the
Legacy*names; leave them out ofindex.ts.
Option B — first-class (matches changeset + current code):
- Rename
LegacyToolCallback→RawShapeToolCallback(and likewise for prompt). - Update the PR description to drop "warns" and "@deprecated".
- Consider re-exporting the new types from
packages/server/src/index.ts.
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
v2 requires StandardSchema objects (e.g.
z.object({...})) forinputSchema. v1 accepted raw shapes{x: z.string()}. This auto-wraps raw shapes and warns.Motivation and Context
v2 requires StandardSchema objects (e.g.
z.object({...})) forinputSchema. v1 accepted raw shapes{x: z.string()}. This auto-wraps raw shapes and warns.v1 vs v2 pattern & evidence
v1 pattern:
`registerTool('x', {inputSchema: {a: z.string()}}, cb)`v2-native:
`registerTool('x', {inputSchema: z.object({a: z.string()})}, cb)`Evidence: ~70% of typical server migration LOC was wrapping shapes. Took multiple OSS repos to zero.
How Has This Been Tested?
v2-bc-integrationvalidation branchpnpm typecheck:all && pnpm lint:all && pnpm test:allgreenBreaking Changes
None — additive
@deprecatedshim. Removed in v3.Types of changes
Checklist
Additional context
Stacks on: C1