feat(compat): restore McpServer.tool()/.prompt()/.resource() variadic overloads#1900
feat(compat): restore McpServer.tool()/.prompt()/.resource() variadic overloads#1900felixweinberger wants to merge 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 47dc6b5 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}
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
All prior feedback is addressed (empty-object raw shape, changeset wording, InferRawShape export, zod/v4 imports) and I found nothing new — deferring only because this adds ~180 LOC of variadic-disambiguation logic plus four new public type exports, which is enough new API surface to warrant a human sign-off.
Extended reasoning...
Overview
Restores McpServer.tool()/.prompt()/.resource() as @deprecated v1-compat shims that forward to the register* methods. Touches packages/server/src/server/mcp.ts (~180 added lines: 6+4+4 overload signatures, variadic argument disambiguation, isZodRawShape/wrapRawShape helpers, 4 new exported types), packages/server/src/index.ts (re-exports), a new mcp.compat.test.ts, and a changeset.
Feedback status
All four of my earlier comments are resolved in 0dafed3 and 1e2946b: empty {} is now treated as a raw shape (with a regression test), the changeset no longer claims a runtime warning, InferRawShape is re-exported, and both files use import * as z from 'zod/v4'. The migration-docs contradiction is intentionally deferred to #1910 per the author; that's reasonable for a coordinated v2-bc series, though it does mean the docs will briefly disagree with the implementation if this lands first.
Security risks
None. No I/O, auth, or untrusted-input handling — purely internal argument-shape disambiguation that delegates to existing registration paths.
Level of scrutiny
Medium. The implementation is straightforward forwarding, but it (a) adds public API surface (4 exported types + 3 deprecated methods) which per REVIEW.md should be intentional, (b) re-introduces the notoriously fiddly v1 variadic overload resolution where ordering/shape sniffing matters, and (c) introduces a direct zod import into mcp.ts. None of this is wrong, but it's the kind of API-shape decision a maintainer should rubber-stamp rather than a bot.
Other factors
Test coverage exists for the main overloads (raw shape, full 5-arg, zero-arg, empty-object, prompt, resource string+template). The disambiguation logic mirrors v1 semantics including the empty-object edge case. I did not find any new correctness issues on this pass.
There was a problem hiding this comment.
All prior feedback is addressed (empty-object raw shape, changeset wording, InferRawShape export, zod/v4 imports) and I have no further findings — deferring final sign-off to a human since this adds ~180 lines of public (deprecated) API surface with non-trivial variadic disambiguation as part of the coordinated v2-bc series.
Extended reasoning...
Overview
Adds @deprecated v1-compat shims McpServer.tool()/.prompt()/.resource() that forward to the v2 register* methods, plus four supporting type exports (ZodRawShape, InferRawShape, LegacyToolCallback, LegacyPromptCallback) and two @internal helpers (isZodRawShape, wrapRawShape). Touches packages/server/src/server/mcp.ts (+~180 lines), the package barrel index.ts, a new mcp.compat.test.ts, and a changeset. All five of my prior inline comments are resolved and verified in the current diff: empty {} is now treated as a raw shape (with a regression test), the changeset no longer claims a runtime warning, InferRawShape is re-exported, and both new zod imports use import * as z from 'zod/v4'.
Security risks
None identified. The shims are pure argument-shape dispatch over user-provided registration data and delegate to the existing _createRegisteredTool/_createRegisteredPrompt/registerResource paths — no new I/O, auth, or untrusted-input parsing.
Level of scrutiny
Medium-high. This is additive and deprecated-from-birth, but it expands the curated public API surface of @modelcontextprotocol/server and encodes variadic disambiguation logic that ~8.3k downstream call sites will exercise. Per REVIEW.md ("every new export is intentional", "burden of proof is on addition"), public-surface additions warrant a maintainer's eye even when the implementation looks correct.
Other factors
- The migration-docs drift I flagged is intentionally deferred to #1910 per the author; reasonable for a stacked series.
- Tests cover the main overload permutations including the empty-object edge case; the author also reports OSS integration validation.
- Two prior review rounds were promptly addressed; no outstanding threads remain.
- I'm deferring rather than approving because this is a deliberate API-design decision in a coordinated series (see linked reviewer guide), not a mechanical change — a human should ratify the surface shape, not just the implementation correctness.
There was a problem hiding this comment.
All prior feedback is addressed and I found nothing new — deferring only because this adds three public methods (14 overloads) and four exported types to McpServer, which is enough new public API surface that a maintainer should give the final sign-off.
Extended reasoning...
Overview
Restores the v1 variadic McpServer.tool()/.prompt()/.resource() methods as @deprecated forwarders to registerTool/registerPrompt/registerResource. Touches packages/server/src/server/mcp.ts (+~180 lines: 3 methods with 14 overload signatures, 2 internal helpers, 4 exported types), packages/server/src/index.ts (re-exports), a new mcp.compat.test.ts, and a changeset. Part of the labeled v2-bc series.
Prior feedback — all resolved
- Empty
{}now treated as a raw shape (isZodRawShapeuses.every()which is vacuously true) with a regression test. - Changeset no longer claims a runtime deprecation warning.
InferRawShapere-exported from the package barrel.- Both new files use
import * as z from 'zod/v4'. - Migration-doc updates deferred to #1910 (reasonable scoping).
Security risks
None identified. Pure additive registration-API forwarders; no transport, auth, or untrusted-input handling beyond what register* already does.
Level of scrutiny
Medium-high. The implementation is straightforward and well-tested, but per REVIEW.md ("burden of proof is on addition", "every new export is intentional") this is a deliberate expansion of the package's public surface on its primary class. The design decision (restore-as-deprecated vs. hard break) appears settled at the series level, but signing off on 4 new exports + 3 new methods is a maintainer call, not a bot call.
Other factors
Bug-hunting pass found nothing. Integration-tested against 5 OSS repos per the PR description. CI green.
|
Closing — |
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.Types of changes
Checklist
Additional context
Stacks on: C1