feat(compat): restore McpServer.tool()/.prompt()/.resource() variadic overloads#1900
feat(compat): restore McpServer.tool()/.prompt()/.resource() variadic overloads#1900felixweinberger wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: b44029f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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-mcpserver-variadic
branch
from
4e0c457 to
4ab915b
Compare
felixweinberger
force-pushed
the
fweinberger/v2-bc-mcpserver-variadic
branch
2 times, most recently
from
4a6b4de to
910a861
Compare
felixweinberger
force-pushed
the
fweinberger/v2-bc-mcpserver-variadic
branch
from
910a861 to
b44029f
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // --------------------------------------------------------------------- | ||
| // v1-compat variadic registration methods. Frozen at 2025-03-26 surface. | ||
| // --------------------------------------------------------------------- |
There was a problem hiding this comment.
🔴 The migration docs still say these methods were removed, which now contradicts the implementation: docs/migration.md:185-187 ("McpServer.tool(), .prompt(), .resource() removed" / "have been removed") and docs/migration-SKILL.md:211 ("are removed"). Since this PR restores them as @deprecated forwarders, both files should be updated to say they're deprecated (still available, will be removed in v3) — the PR checklist claims docs were updated but no docs/ files are in the diff.
Extended reasoning...
What the bug is
This PR restores McpServer.tool(), .prompt(), and .resource() as @deprecated v1-compat shims that forward to registerTool/registerPrompt/registerResource. However, the v2 migration guides — which ship to users — still describe these methods as removed, not deprecated. After this PR merges, the published documentation will directly contradict the implementation.
Specific locations
docs/migration.md:185— section heading: "McpServer.tool(),.prompt(),.resource()removed"docs/migration.md:187— body: "The deprecated variadic-overload methods have been removed."docs/migration-SKILL.md:211— "The variadic.tool(),.prompt(),.resource()methods are removed."
Neither file appears in this PR's diff (only .changeset/, packages/server/src/index.ts, packages/server/src/server/mcp.ts, and the new test file are touched), even though the PR checklist ticks "I have added or updated documentation as needed".
Why this matters / why nothing prevents it
The repo's review checklist (REVIEW.md > Tests & docs) explicitly calls out: "Bugfix or behavior change: check whether docs/**/*.md describes the old behavior and needs updating; flag prose that now contradicts the implementation." CLAUDE.md likewise requires keeping migration.md / migration-SKILL.md in sync with breaking-change semantics. There is no automated check that catches prose drift, so this slips through CI.
Step-by-step proof
- After this PR,
packages/server/src/server/mcp.tsdefinestool(),prompt(), andresource()as public (deprecated) methods onMcpServer. - A user upgrading to v2 reads
docs/migration.md§"McpServer.tool(), .prompt(), .resource() removed" and is told the methods have been removed and that they must rewrite toregister*. - The user spends effort migrating ~8,300 call sites (per the PR description's own evidence) under the belief it's a hard break — when in fact the entire point of this PR is that a bump-only upgrade works and migration can be deferred to v3.
- Conversely, a user who tries
server.tool(...)and sees it work will conclude the migration guide is wrong/stale, eroding trust in the docs.
Impact
Users following the official migration guide get incorrect information about what is required vs. optional for the v2 upgrade. The migration guidance (use register*) is still the right recommendation, but the factual claim "removed" is now false.
Fix
Update both files to reflect deprecation rather than removal, e.g.:
docs/migration.md:185heading → "McpServer.tool(),.prompt(),.resource()deprecated"docs/migration.md:187→ "The variadic-overload methods are deprecated and will be removed in v3. They remain available as compatibility shims that forward toregisterTool/registerPrompt/registerResourceand emit a one-time deprecation warning. Prefer theregister*methods, which use an explicit config object rather than positional arguments."docs/migration-SKILL.md:211→ "The variadic.tool(),.prompt(),.resource()methods are deprecated (removed in v3). Use theregister*methods with a config object."
| function isZodRawShape(obj: unknown): obj is ZodRawShape { | ||
| 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)); | ||
| } |
There was a problem hiding this comment.
🔴 isZodRawShape() returns false for an empty object (values.length > 0 fails), so server.tool('x', {}, cb) falls into the else if (typeof first === 'object') branch and {} is treated as annotations instead of inputSchema. v1 explicitly accepted {} as a raw shape (isEmptyObject || ...some(isZodTypeLike)), so this is a behavioral regression: the callback is now invoked as cb(ctx) instead of cb({}, ctx), breaking v1 code that destructures the second ctx parameter. Fix: match v1 by treating an empty plain object as a raw shape.
Extended reasoning...
What the bug is
isZodRawShape() (mcp.ts:1219–1224) requires at least one value:
const values = Object.values(obj);
return values.length > 0 && values.every(v => isStandardSchema(v));For {}, values.length is 0, so this returns false. The v1 SDK's isZodRawShape (verified against v1.18.2 / v1.21.0, src/server/mcp.ts:1054–1062) was deliberately written to accept the empty object:
const isEmptyObject = Object.keys(obj).length === 0;
return isEmptyObject || Object.values(obj).some(isZodTypeLike);Code path that triggers it
For server.tool('ping', {}, (args, ctx) => ...):
rest = [{}, cb],rest.length > 1→ enter the disambiguation block.isZodRawShape({})→ false;isStandardSchema({})→ false.- Falls into
else if (typeof first === 'object' && first \!== null)→annotations = {}. inputSchemaremainsundefined.createToolExecutor(undefined, cb)returns the no-args executor:(_args, ctx) => callback(ctx).
Why existing code doesn't prevent it
The disambiguation in tool() relies entirely on isZodRawShape/isStandardSchema to tell schemas apart from annotations. Since neither matches {}, the generic object fallback wins. There is no separate empty-object check anywhere in the new shim, and wrapRawShape is never reached for this argument in the tool() path.
Impact
For migrating v1 code written as server.tool(name, {}, (args, ctx) => { ctx.sendNotification(...) }):
argsreceives theServerContextobject (not{}).ctxisundefined, soctx.sendNotification(...)throwsTypeError: Cannot read properties of undefined.- Additionally,
annotationsis set to{}(harmless but wrong) and the advertisedinputSchemadiffers from v1 (usesEMPTY_OBJECT_JSON_SCHEMAfallback instead of a realz.object({}), which is observably the same JSON Schema but semantically different internally).
This defeats the PR's stated goal ("Frozen at 2025-03-26 surface") for a case v1 went out of its way to handle.
(prompt() is also affected via wrapRawShape({}), which returns {} cast as StandardSchemaWithJSON rather than z.object({}), but the tool() case is the clearer runtime break.)
Step-by-step proof
// v1 code being migrated:
server.tool('ping', {}, (_args, ctx) => {
return { content: [{ type: 'text', text: ctx.requestId }] };
});| Step | v1 behavior | This PR |
|---|---|---|
isZodRawShape({}) |
true (isEmptyObject) |
false (values.length > 0 fails) |
{} assigned to |
paramsSchema → z.object({}) |
annotations |
inputSchema |
defined | undefined |
| executor calls | cb({}, ctx) |
cb(ctx) |
ctx in callback |
ServerContext |
undefined → TypeError |
Fix
Match v1 semantics:
function isZodRawShape(obj: unknown): obj is ZodRawShape {
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));
}This also fixes wrapRawShape({}) to return z.object({}) for the prompt() path.
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| Restore `McpServer.tool()`, `.prompt()`, `.resource()` variadic overloads as `@deprecated` v1-compat shims forwarding to `registerTool`/`registerPrompt`/`registerResource`. Emits a one-time deprecation warning; removed in v3. |
There was a problem hiding this comment.
🔴 The changeset claims these shims "Emits a one-time deprecation warning", but the .tool()/.prompt()/.resource() implementations contain no runtime warning — only @deprecated JSDoc tags (which are compile-time/IDE only). The PR description also lists "warn-once" as one of 5 test cases, yet mcp.compat.test.ts has no such test. Either implement the one-time runtime warning (e.g. a module-level guard + process.emitWarning) or drop the claim from the changeset.
Extended reasoning...
What the bug is
The changeset at .changeset/mcpserver-variadic-compat.md:5 states:
Restore
McpServer.tool(),.prompt(),.resource()variadic overloads as@deprecatedv1-compat shims … Emits a one-time deprecation warning; removed in v3.
The PR description's "How Has This Been Tested?" section also claims mcp.compat.test.ts covers "5 cases (all 3 methods, raw shape, warn-once)".
However, the actual .tool() / .prompt() / .resource() implementations in packages/server/src/server/mcp.ts contain no runtime warning emission whatsoever — they only carry /** @deprecated … */ JSDoc tags, which produce TypeScript strikethrough/lint warnings at compile time but emit nothing at runtime.
The specific code path
Grepping mcp.ts for warn|deprecat|emitWarning yields only:
- The
@deprecatedJSDoc annotations on each overload signature validateAndWarnToolName(unrelated — validates tool name format, called for all tool registrations includingregisterTool)
There is no console.warn(...), no process.emitWarning(...), and no module-level let warned = false guard anywhere in the three new method bodies. Similarly, grepping mcp.compat.test.ts for warn returns zero matches — the file contains 5 it() blocks but none of them spy on console.warn or assert warn-once behavior.
Why existing code doesn't prevent it
The @deprecated JSDoc tag is a TypeScript/TSDoc annotation consumed by editors and the @typescript-eslint/no-deprecated rule (which the test file explicitly disables on line 1). It has no runtime representation. A user calling server.tool('x', cb) from plain JavaScript, or from TypeScript with that lint rule off, will see nothing — directly contradicting the published changelog entry.
Step-by-step proof
- Install
@modelcontextprotocol/serverafter this PR ships. - Read the CHANGELOG (generated from this changeset): it says calling
.tool()"emits a one-time deprecation warning". - Run:
const { McpServer } = require('@modelcontextprotocol/server'); const s = new McpServer({ name: 't', version: '1' }); s.tool('x', () => ({ content: [] }));
- Observe: nothing is printed to stderr/stdout. No
DeprecationWarningis emitted. - The changelog promised behavior the package does not deliver.
Impact
Changeset text is rendered into the published CHANGELOG.md on npm. Consumers reading it will expect a runtime nudge toward registerTool/registerPrompt/registerResource and may rely on the warning to flag remaining call sites before the v3 removal — but they'll get no signal. This is the exact pattern called out in the repo's review guide under Documentation & Changesets: "prose that promises behavior the code no longer ships misleads consumers".
How to fix
Either:
- Implement it: add a module-level
let _legacyWarned = falseand at the top of each shim body callprocess.emitWarning('McpServer.tool()/.prompt()/.resource() are deprecated; use registerTool/registerPrompt/registerResource', 'DeprecationWarning')guarded by the flag, plus a test that spies onprocess.emitWarningand asserts it fires exactly once across two calls; or - Drop it: change the changeset sentence to end at "…forwarding to
registerTool/registerPrompt/registerResource. Removed in v3." and remove the "warn-once" mention from the PR description.
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
v2 removed the variadic
.tool()/.prompt()/.resource()methods in favor ofregister*with config objects. This restores them as@deprecatedforwarders.Motivation and Context
v2 removed the variadic
.tool()/.prompt()/.resource()methods in favor ofregister*with config objects. This restores them as@deprecatedforwarders.v1 vs v2 pattern & evidence
v1 pattern:
`server.tool('echo', {msg: z.string()}, async ({msg}) => ({content: [{type:'text', text: msg}]}))`v2-native:
`server.registerTool('echo', {inputSchema: z.object({msg: z.string()})}, async ({msg}) => ...)`Evidence: GitHub code search: ~8,300 files use
server.tool(. The single highest-volume break.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