Modify registerPrompt to accept undefined Args type#1516
Modify registerPrompt to accept undefined Args type#1516andershagbard wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
🦋 Changeset detectedLatest commit: b32c18d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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: |
The Args generic now extends StandardSchemaWithJSON | undefined with a default of undefined, matching registerTool. This lets PromptCallback resolve to the (ctx) => result overload when argsSchema is omitted. Co-authored-by: Anders Søgaard <andershagbard@gmail.com>
felixweinberger
force-pushed
the
patch-1
branch
from
8695e01 to
797078a
Compare
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| function registerPrompt_withArgs() { | ||
| server.registerPrompt( | ||
| 'with-args', | ||
| { argsSchema: z.object({ code: z.string() }) }, | ||
| (args: { code: string }, ctx: ServerContext) => result | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 The registerPrompt_withArgs test only covers the positive case (callback correctly accepts (args, ctx)) but is missing a @ts-expect-error negative test verifying TypeScript rejects a no-args callback when argsSchema is provided. The registerPrompt_noArgs function correctly has both a positive and a negative test, but registerPrompt_withArgs only has the positive -- a regression in the Args extends StandardSchemaWithJSON branch of PromptCallback could go undetected.
Extended reasoning...
The new test file packages/server/test/server/mcp.types.test.ts was specifically added to validate the conditional type resolution of PromptCallback under both overloads of registerPrompt. The registerPrompt_noArgs function (lines 20-24) demonstrates the correct pattern: one positive test followed by one @ts-expect-error negative test. However, registerPrompt_withArgs (lines 27-34) only contains the positive case.
The missing negative test would verify that TypeScript rejects a no-args callback when argsSchema is present:
// @ts-expect-error -- callback must accept args when argsSchema is present
server.registerPrompt('with-args', { argsSchema: z.object({ code: z.string() }) }, (ctx: ServerContext) => result);
Without this test, a specific class of regression in PromptCallback could go undetected. The production code is correct today, and TypeScript structural typing means ServerContext (which has a required mcpReq property) is not directly assignable to { code: string }, providing incidental coverage. However, this structural incompatibility is a side-effect, not an intentional contract check.
The verifier refutation argues this is a test enhancement request rather than a blocking defect, which is fair. The primary fix (allowing undefined as the default Args type) is already validated by the existing tests, and the structural incompatibility between ServerContext and { code: string } means TypeScript would reject the mis-typed callback today even without an explicit negative test.
Nevertheless, the test file was designed to document and enforce the full type contract of both overloads symmetrically. The asymmetry between noArgs (2 tests: positive + negative) and withArgs (1 test: positive only) is a genuine gap. A deliberate regression that widens PromptCallback to accept either arity when Args extends StandardSchemaWithJSON would pass the existing positive test but would be caught by a negative test.
Step-by-step proof of the gap: (1) Suppose PromptCallback is changed to accept either arity as a union when Args extends StandardSchemaWithJSON. (2) The existing positive withArgs test still passes -- a callback with (args, ctx) is accepted. (3) There is no negative test to verify a no-arg-only callback (ctx: ServerContext) => result is rejected, so the regression goes undetected. (4) A @ts-expect-error negative test directly asserts this rejection, closing the gap.
|
@claude review |
| enabled: boolean; | ||
| enable(): void; | ||
| disable(): void; | ||
| update<Args extends StandardSchemaWithJSON>(updates: { | ||
| update<Args extends StandardSchemaWithJSON | undefined = undefined>(updates: { | ||
| name?: string | null; | ||
| title?: string; | ||
| description?: string; |
There was a problem hiding this comment.
🔴 The update() type change introduces a runtime mismatch: TypeScript now accepts a no-args callback on a prompt originally registered with an argsSchema, but at runtime the no-args callback silently receives the parsed schema data as its ctx parameter instead of the real ServerContext. To fix this properly, RegisteredPrompt must be made generic (carrying the original Args type) so that update() can enforce callback arity matches the stored schema.
Extended reasoning...
What the bug is
The PR changes update<Args extends StandardSchemaWithJSON | undefined = undefined>, giving Args a default of undefined. This means calling prompt.update({ callback: (ctx) => newResult }) on a prompt that was originally registered with an argsSchema is now accepted by TypeScript — it infers Args = undefined, making callback?: PromptCallback<undefined> which matches the no-args callback signature. Before the PR, update<Args extends StandardSchemaWithJSON> had no default and no | undefined, so TypeScript would reject that call, inadvertently blocking this misuse.
The specific code path that triggers it
Inside _createRegisteredPrompt's update closure (around line 727 of mcp.ts), there is a guard: if (updates.argsSchema !== undefined) { currentArgsSchema = updates.argsSchema; }. If the caller does not supply argsSchema in the update object, this guard does not fire — currentArgsSchema remains the original zod schema. Meanwhile, updates.callback IS supplied, so currentCallback is updated to the new no-args callback. Then createPromptHandler(name, currentArgsSchema, noArgsCallback) is called.
Why existing code does not prevent it
Inside createPromptHandler, the if (argsSchema) branch is entered because currentArgsSchema is still truthy. It casts callback to (args: unknown, ctx: ServerContext) => ... and calls typedCallback(parseResult.data, ctx). But typedCallback is actually the no-args (ctx) => newResult — it receives parseResult.data (the parsed args object, e.g. {code: "something"}) as its ctx parameter, and the real ServerContext is silently dropped as the second argument. No exception is thrown; the callback silently uses wrong data.
Impact
Any server that registers a with-args prompt and later calls .update({ callback: (ctx) => ... }) (intending to replace the callback while keeping the schema) will silently produce incorrect results. The callback's ctx will contain schema-parsed data instead of the MCP ServerContext, so operations like ctx.authInfo, ctx.sessionId, etc. will be undefined or behave unexpectedly — a silent correctness regression with no compile-time or runtime warning.
How to fix it
The root cause is that RegisteredPrompt is non-generic: it does not carry the original Args type, so update() cannot verify that the new callback arity matches the existing schema. The proper fix is to make RegisteredPrompt<Args> generic so the update method ties its callback type to the original Args. A simpler short-term mitigation: require that if callback is provided without a new argsSchema, the caller must explicitly pass argsSchema: undefined to signal intent to clear the schema, preventing the implicit mismatch.
Step-by-step proof
- Register:
const prompt = server.registerPrompt('p', { argsSchema: z.object({ code: z.string() }) }, (args, ctx) => result)—currentArgsSchemais set to the zod schema. - Call:
prompt.update({ callback: (ctx) => newResult })— TypeScript infersArgs = undefined(satisfiesStandardSchemaWithJSON | undefined), accepts the call without error. - In the update closure:
updates.argsSchemaisundefined— the guardif (updates.argsSchema !== undefined)does NOT fire —currentArgsSchemastays as the original zod schema. updates.callbackis provided —currentCallbackis updated to the no-args(ctx) => newResult.createPromptHandler(name, currentArgsSchema, noArgsCallback)is called with the original schema still set.- In
createPromptHandler:if (argsSchema)istrue— enters the with-args branch — callstypedCallback(parseResult.data, ctx). - The no-args callback
(ctx) => newResultreceivesparseResult.data(e.g.{code: "something"}) as itsctx— the realServerContextis silently discarded as the ignored second argument.
| describe('registerPrompt types', () => { | ||
| it('compiles', () => { | ||
| // The functions above are compile-time type assertions; this suite | ||
| // exists so vitest detects the file as containing tests. | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 The new mcp.types.test.ts file covers only the registerPrompt half of the type fix, leaving RegisteredPrompt.update() without compile-time assertions. If the update fix were reverted, the test suite would not catch the regression.
Extended reasoning...
The PR makes two parallel type fixes: adding undefined = undefined to Args in both registerPrompt (line 918) and RegisteredPrompt.update (line 1242). Both fixes are present in the diff and production code is correct. However, mcp.types.test.ts only exercises registerPrompt via registerPrompt_noArgs and registerPrompt_withArgs, with zero compile-time assertions for the update() method.
The specific uncovered path is RegisteredPrompt.update typed as update<Args extends StandardSchemaWithJSON | undefined = undefined>. The test file verifies registerPrompt overloads but never calls .update() on a returned RegisteredPrompt handle.
If the update fix were reverted to update, no existing test would fail. The registerPrompt_noArgs and registerPrompt_withArgs functions call server.registerPrompt() directly and never exercise .update() on the returned handle. TypeScript would only surface the regression when a user called .update({ callback: (ctx) => result }) on a no-args prompt in real code.
This is a test coverage gap, not a production bug — the actual fix in mcp.ts is correct. The risk is a future refactor accidentally removing the undefined from update() without any test catching it, since the test file was specifically introduced to guard against this class of type regression.
To fix, add to mcp.types.test.ts: const prompt = server.registerPrompt('no-args', {}, (ctx: ServerContext) => result); prompt.update({ callback: (ctx: ServerContext) => result }); // should compile. And a @ts-expect-error line rejecting an args callback when no argsSchema.
Step-by-step proof: (1) Revert line 1242 of mcp.ts from update<Args extends StandardSchemaWithJSON | undefined = undefined> back to update. (2) Run the type check — all tests in mcp.types.test.ts still pass because none call .update(). (3) A user calling prompt.update({ callback: (ctx) => result }) in their own project would get a TS error, but the test suite stays green. (4) Adding prompt.update() assertions to the test file would immediately catch the regression in step 2.
3 participants
Motivation and Context
When using
registerPromptwithout anyargsSchemathe callback will still be typed as the first argument is the input of the argsSchema, which is not the case.This PR corrects that.
Code:
Current type:
Correct type:
How Has This Been Tested?
Tested in a project using
mcp-handlerBreaking Changes
Maybe someone has used the types incorrectly. Unsure if this counts as breaking, as it's a bugfix.
Types of changes
Checklist