From ffb6546e512b444e1c5c1f548bee459b6c6c517a Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 3 Sep 2026 09:15:14 +0200 Subject: [PATCH] fix(mcp): prompts take the trip id as the string every client sends (#2207) Prompt arguments cross the wire as strings, but the three trip prompts declared tripId as z.number(), so every prompts/get failed with -32602 "expected number, received string", MCP Inspector included. The id is now parsed from the string through a shared tripIdPromptArg (still an integer in the safe range), PromptOptions only admits string-input schemas so the mistake cannot come back unnoticed, and a prompt declared with an empty argsSchema no longer demands an empty arguments object from the client. The prompt tests run through a real in-memory MCP client instead of calling the callbacks directly. --- server/src/nest-mcp/README.md | 1 + server/src/nest-mcp/index.ts | 1 + server/src/nest-mcp/registry.ts | 10 +- server/src/nest-mcp/types.ts | 15 +- server/src/nest/mcp-shared/prompt-args.ts | 13 ++ server/src/nest/trips/trip-prompts.mcp.ts | 6 +- server/src/nest/trips/trips.mcp.ts | 3 +- server/tests/unit/mcp/tools-prompts.test.ts | 220 ++++++++++++-------- server/tests/unit/nest-mcp/attach.test.ts | 44 +++- 9 files changed, 211 insertions(+), 102 deletions(-) create mode 100644 server/src/nest/mcp-shared/prompt-args.ts diff --git a/server/src/nest-mcp/README.md b/server/src/nest-mcp/README.md index f6319dec7..f10535219 100644 --- a/server/src/nest-mcp/README.md +++ b/server/src/nest-mcp/README.md @@ -38,6 +38,7 @@ export class ThingsMcp { - `@Resource(options)` — fixed URI (`uri`, `mimeType`, …). Handler: `(uri: URL, ctx)`. - `@ResourceTemplate(options)` — `uriTemplate` (RFC 6570). Handler: `(uri: URL, variables, ctx)`. (No `list`/`complete` template callbacks yet — extend when a domain needs them.) - `@Prompt(options)` — mirrors `registerPrompt` (`argsSchema` entries must be string-valued Zod schemas). Handler: `(args, ctx)`. + Prompt arguments arrive as strings, so every `argsSchema` entry has to accept a string (`PromptArgsShape`): parse numbers with `z.string().regex(...).transform(Number)`, never `z.number()`. The handler receives the transformed values. `ctx` always takes the SDK `extra` slot — the last handler argument. diff --git a/server/src/nest-mcp/index.ts b/server/src/nest-mcp/index.ts index f772f2782..0b3279d7e 100644 --- a/server/src/nest-mcp/index.ts +++ b/server/src/nest-mcp/index.ts @@ -35,6 +35,7 @@ export { type McpModuleOptions, type McpRegistryListing, type McpZodSchema, + type PromptArgsShape, type PromptOptions, type ResourceOptions, type ResourceTemplateOptions, diff --git a/server/src/nest-mcp/registry.ts b/server/src/nest-mcp/registry.ts index d48b7a802..ab020875e 100644 --- a/server/src/nest-mcp/registry.ts +++ b/server/src/nest-mcp/registry.ts @@ -368,13 +368,19 @@ export class McpRegistry { ctx: McpContext, opts?: McpAttachOptions, ): void { + // An empty shape means "no arguments", and is registered as such: with a + // shape the SDK parses request.params.arguments, which a client that has + // nothing to send may omit entirely, and z.object({}) then rejects the + // undefined with -32602. + const argsSchema = + options.argsSchema !== undefined && Object.keys(options.argsSchema).length > 0 ? options.argsSchema : undefined; const config: Record = { title: options.title, description: options.description, - argsSchema: options.argsSchema, + argsSchema, }; const cb = - options.argsSchema !== undefined + argsSchema !== undefined ? (args: unknown, _extra: unknown) => { opts?.onInvoke?.({ kind: 'prompt', name: options.name }); return handler.call(instance, args, ctx); diff --git a/server/src/nest-mcp/types.ts b/server/src/nest-mcp/types.ts index 6cad03e3b..82b895fa1 100644 --- a/server/src/nest-mcp/types.ts +++ b/server/src/nest-mcp/types.ts @@ -1,6 +1,6 @@ import type { Scope, ScopeGroup } from '../mcp/scopes'; -import type { ZodRawShape } from 'zod'; +import type { ZodRawShape, ZodType } from 'zod'; /** * The mode half of every scope: 'read' | 'write' | 'delete' | 'share'. @@ -152,9 +152,18 @@ export interface ResourceTemplateOptions extends McpEntryOptionsBase { _meta?: Record; } -/** Mirrors the SDK's `registerPrompt` config (argsSchema entries must be string-valued Zod schemas). */ +/** + * Prompt arguments cross the wire as strings (GetPromptRequest.arguments is + * a Record), so every entry has to accept a string and may + * transform from there. A bare z.number() type-checks against ZodRawShape but + * rejects every real client with "expected number, received string" (#2207); + * the input constraint here turns that into a compile error. + */ +export type PromptArgsShape = Record>; + +/** Mirrors the SDK's `registerPrompt` config. */ export interface PromptOptions extends McpEntryOptionsBase { - argsSchema?: ZodRawShape; + argsSchema?: PromptArgsShape; } export type McpEntryKind = 'tool' | 'resource' | 'resourceTemplate' | 'prompt'; diff --git a/server/src/nest/mcp-shared/prompt-args.ts b/server/src/nest/mcp-shared/prompt-args.ts new file mode 100644 index 000000000..b27781da4 --- /dev/null +++ b/server/src/nest/mcp-shared/prompt-args.ts @@ -0,0 +1,13 @@ +import { z } from 'zod'; + +/** + * Prompt arguments arrive as strings (GetPromptRequest.arguments is a + * Record), so a numeric id has to be parsed out of one. + * The old z.number().int().positive() failed every client, MCP Inspector + * included, with "expected number, received string" (#2207). + */ +export const tripIdPromptArg = z + .string() + .regex(/^[1-9]\d*$/, 'Trip ID must be a positive integer') + .transform(Number) + .pipe(z.number().int().positive()); diff --git a/server/src/nest/trips/trip-prompts.mcp.ts b/server/src/nest/trips/trip-prompts.mcp.ts index 4d88b4608..2f1506a10 100644 --- a/server/src/nest/trips/trip-prompts.mcp.ts +++ b/server/src/nest/trips/trip-prompts.mcp.ts @@ -1,8 +1,8 @@ import { McpController, Prompt, type McpContext } from '../../nest-mcp'; -import { z } from 'zod'; import { ADDON_IDS } from '../../addons'; import { addonGate } from '../addons/addon-gate'; import { AddonsService } from '../addons/addons.service'; +import { tripIdPromptArg } from '../mcp-shared/prompt-args'; import { PackingService } from '../packing/packing.service'; import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; import { TripsService } from './trips.service'; @@ -35,7 +35,7 @@ export class TripPromptsMcp { title: 'Budget Overview', description: 'Get a formatted budget summary for a trip', argsSchema: { - tripId: z.number().int().positive().describe('Trip ID'), + tripId: tripIdPromptArg.describe('Trip ID'), }, when: budgetAddonOn, }) @@ -72,7 +72,7 @@ export class TripPromptsMcp { title: 'Packing List', description: 'Get a formatted packing checklist for a trip', argsSchema: { - tripId: z.number().int().positive().describe('Trip ID'), + tripId: tripIdPromptArg.describe('Trip ID'), }, when: packingAddonOn, }) diff --git a/server/src/nest/trips/trips.mcp.ts b/server/src/nest/trips/trips.mcp.ts index 3340f5ae0..483abc018 100644 --- a/server/src/nest/trips/trips.mcp.ts +++ b/server/src/nest/trips/trips.mcp.ts @@ -5,6 +5,7 @@ import { demoDenied, errorResult, ok, } from '../../nest-mcp'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; +import { tripIdPromptArg } from '../mcp-shared/prompt-args'; import { z } from 'zod'; import { AuthService } from '../auth/auth.service'; import { CalendarService } from '../calendar/calendar.service'; @@ -579,7 +580,7 @@ export class TripsMcp { title: 'Trip Summary', description: 'Load a full summary of a trip for context before planning or modifications', argsSchema: { - tripId: z.number().int().positive().describe('Trip ID to summarize'), + tripId: tripIdPromptArg.describe('Trip ID to summarize'), }, }) async tripSummaryPrompt({ tripId }: { tripId: number }, ctx: McpContext) { diff --git a/server/tests/unit/mcp/tools-prompts.test.ts b/server/tests/unit/mcp/tools-prompts.test.ts index 61f11811d..cb3dc90dc 100644 --- a/server/tests/unit/mcp/tools-prompts.test.ts +++ b/server/tests/unit/mcp/tools-prompts.test.ts @@ -1,13 +1,17 @@ /** * Unit tests for MCP prompts: token_auth_notice, trip-summary, packing-list, budget-overview. * - * Note: MCP prompt arguments must be Record per protocol spec. - * The prompts.ts argsSchema uses z.number() for tripId, which is incompatible - * with the MCP client's type-safe getPrompt. We therefore test prompt callbacks - * directly via the registered prompt handlers on the server instance. + * Every prompt is fetched the way a real client fetches it: over an in-memory + * transport, with the arguments as the strings the protocol carries + * (GetPromptRequest.arguments is Record). The tripId schemas + * used to be z.number(), which no client could ever satisfy (#2207); these + * cases used to sidestep that by calling the callbacks directly. */ -import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from 'vitest'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; +import { Client } from '@modelcontextprotocol/sdk/client/index'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory'; +import { ErrorCode } from '@modelcontextprotocol/sdk/types'; const { testDb, dbMock } = vi.hoisted(() => { const Database = require('better-sqlite3'); @@ -175,31 +179,42 @@ afterAll(() => { testDb.close(); }); -/** Build a fresh McpServer with prompts registered for the given userId. */ -function buildServer(userId: number, opts: { isStaticToken?: boolean } = {}): McpServer { +const openSessions: Array<{ client: Client; server: McpServer }> = []; + +afterEach(async () => { + for (const { client, server } of openSessions.splice(0)) { + await client.close(); + await server.close(); + } +}); + +/** A connected client over a fresh McpServer with the prompts registered for the given userId. */ +async function buildServer(userId: number, opts: { isStaticToken?: boolean } = {}): Promise { const server = new McpServer({ name: 'trek-test', version: '1.0.0' }); // Every prompt is DI-discovered now; attach them the way registerTools does in // production, including the isStaticToken flag the notice's `when` gate reads. createTestRegistry([tripsMcp, tripPromptsMcp, packingMcp, budgetMcp, authMcp], { accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }) .attach(server, { userId, scopes: null, isStaticToken: opts.isStaticToken ?? false }); - return server; + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + await server.connect(serverTransport); + await client.connect(clientTransport); + openSessions.push({ client, server }); + return client; } -/** Invoke a registered prompt callback directly, bypassing the MCP transport. */ -async function invokePrompt(server: McpServer, name: string, args: Record): Promise { - const prompts = (server as any)._registeredPrompts; - const prompt = prompts[name]; - if (!prompt) throw new Error(`Prompt "${name}" not registered`); - const result = await prompt.callback(args, {}); +/** Fetch a prompt over the transport; numeric ids go out as the strings a client sends. */ +async function invokePrompt(client: Client, name: string, args: Record = {}): Promise { + const wireArgs = Object.fromEntries(Object.entries(args).map(([key, value]) => [key, String(value)])); + const result = await client.getPrompt({ name, arguments: wireArgs }); const msg = result.messages[0]; if (msg?.content?.type === 'text') return msg.content.text; return ''; } -/** List registered prompt names. */ -function listRegisteredPrompts(server: McpServer): string[] { - const prompts = (server as any)._registeredPrompts; - return Object.keys(prompts); +/** Prompt names the session advertises. */ +async function listRegisteredPrompts(client: Client): Promise { + return (await client.listPrompts()).prompts.map((p) => p.name); } // ───────────────────────────────────────────────────────────────────────────── @@ -207,8 +222,8 @@ function listRegisteredPrompts(server: McpServer): string[] { // ───────────────────────────────────────────────────────────────────────────── /** Return only the text of a prompt result, ignoring error shapes. */ -async function invokePromptText(server: McpServer, name: string, args: Record): Promise { - return invokePrompt(server, name, args); +async function invokePromptText(client: Client, name: string, args: Record): Promise { + return invokePrompt(client, name, args); } // ───────────────────────────────────────────────────────────────────────────── @@ -218,22 +233,68 @@ async function invokePromptText(server: McpServer, name: string, args: Record { it('is registered and returns deprecation notice when isStaticToken=true', async () => { const { user } = createUser(testDb); - const server = buildServer(user.id, { isStaticToken: true }); - const names = listRegisteredPrompts(server); + const client = await buildServer(user.id, { isStaticToken: true }); + const names = await listRegisteredPrompts(client); expect(names).toContain('token_auth_notice'); - const text = await invokePrompt(server, 'token_auth_notice', {}); + const text = await invokePrompt(client, 'token_auth_notice', {}); expect(text).toContain('static API token'); expect(text).toContain('deprecated'); }); + it('answers a request that carries no arguments at all', async () => { + const { user } = createUser(testDb); + const client = await buildServer(user.id, { isStaticToken: true }); + // The SDK client sends no `arguments` key when none are given; a prompt + // without arguments must not demand an empty object. + const result = await client.getPrompt({ name: 'token_auth_notice' }); + expect(result.messages[0]?.content).toMatchObject({ type: 'text' }); + }); + it('is NOT registered when isStaticToken=false', async () => { const { user } = createUser(testDb); - const server = buildServer(user.id, { isStaticToken: false }); - const names = listRegisteredPrompts(server); + const client = await buildServer(user.id, { isStaticToken: false }); + const names = await listRegisteredPrompts(client); expect(names).not.toContain('token_auth_notice'); }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Prompt arguments on the wire (#2207) +// ───────────────────────────────────────────────────────────────────────────── + +describe('Prompt arguments on the wire', () => { + it.each(['trip-summary', 'packing-list', 'budget-overview'])('%s takes the trip id as the string every client sends (#2207)', async (name) => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Wire Trip' }); + createPackingItem(testDb, trip.id, { name: 'Charger', category: 'Tech' }); + + const client = await buildServer(user.id); + const result = await client.getPrompt({ name, arguments: { tripId: String(trip.id) } }); + expect(result.description).toContain('Wire Trip'); + }); + + it.each(['abc', '0', '-3', '1.5', '', '99999999999999999999'])('rejects the trip id %j as invalid params instead of reaching the handler', async (tripId) => { + const { user } = createUser(testDb); + createTrip(testDb, user.id, { title: 'Untouched' }); + mockGetTripSummary.mockClear(); + + const client = await buildServer(user.id); + await expect(client.getPrompt({ name: 'trip-summary', arguments: { tripId } })) + .rejects.toMatchObject({ code: ErrorCode.InvalidParams }); + expect(mockGetTripSummary).not.toHaveBeenCalled(); + }); + + it('advertises tripId as a required argument of every trip prompt', async () => { + const { user } = createUser(testDb); + const client = await buildServer(user.id); + const { prompts } = await client.listPrompts(); + const byName = Object.fromEntries(prompts.map((p) => [p.name, p.arguments])); + expect(byName['trip-summary']).toEqual([{ name: 'tripId', description: 'Trip ID to summarize', required: true }]); + expect(byName['packing-list']).toEqual([{ name: 'tripId', description: 'Trip ID', required: true }]); + expect(byName['budget-overview']).toEqual([{ name: 'tripId', description: 'Trip ID', required: true }]); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // trip-summary // ───────────────────────────────────────────────────────────────────────────── @@ -241,8 +302,8 @@ describe('Prompt: token_auth_notice', () => { describe('Prompt: trip-summary', () => { it('is always registered regardless of addons', async () => { const { user } = createUser(testDb); - const server = buildServer(user.id); - expect(listRegisteredPrompts(server)).toContain('trip-summary'); + const client = await buildServer(user.id); + expect(await listRegisteredPrompts(client)).toContain('trip-summary'); }); it('returns access denied message for non-member trip', async () => { @@ -250,8 +311,8 @@ describe('Prompt: trip-summary', () => { const { user: other } = createUser(testDb); const trip = createTrip(testDb, other.id, { title: 'Private Trip' }); - const server = buildServer(user.id); - const text = await invokePrompt(server, 'trip-summary', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'trip-summary', { tripId: trip.id }); expect(text.toLowerCase()).toContain('access denied'); }); @@ -261,18 +322,10 @@ describe('Prompt: trip-summary', () => { const trip = createTrip(testDb, user.id, { title: 'Paris Trip', start_date: '2026-07-01', end_date: '2026-07-03' }); addTripMember(testDb, trip.id, member.id); - const server = buildServer(user.id); - // The prompt callback accesses packing/budget from getTripSummary which returns - // object shapes; this verifies the trip is accessible and a response is produced. - try { - const text = await invokePrompt(server, 'trip-summary', { tripId: trip.id }); - expect(text).toContain('Paris Trip'); - } catch (err: any) { - // getTripSummary returns { packing: { items, total, checked }, budget: { items, total, ... } } - // but prompts.ts calls packing.filter() expecting an array — known source discrepancy. - // Verify the trip IS accessible (access denied would not throw, it returns a message). - expect(err.message).not.toContain('access denied'); - } + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'trip-summary', { tripId: trip.id }); + expect(text).toContain('Paris Trip'); + expect(text).toContain('Dates: 2026-07-01 to 2026-07-03'); }); it('returns "Trip not found." when getTripSummary returns null for accessible trip', async () => { @@ -282,8 +335,8 @@ describe('Prompt: trip-summary', () => { // Override mock to return null (covers lines 46-48 in prompts.ts) mockGetTripSummary.mockReturnValueOnce(null); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'trip-summary', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'trip-summary', { tripId: trip.id }); expect(text).toContain('Trip not found.'); }); @@ -302,8 +355,8 @@ describe('Prompt: trip-summary', () => { collabNotes: [], }); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'trip-summary', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'trip-summary', { tripId: trip.id }); expect(text).toContain('Untitled'); expect(text).toContain('?'); // start/end date fallback expect(text).toContain('EUR'); // currency fallback @@ -318,15 +371,15 @@ describe('Prompt: packing-list', () => { it('prompt is NOT registered when packing addon is disabled', async () => { isAddonEnabledMock.mockReturnValue(false); const { user } = createUser(testDb); - const server = buildServer(user.id); - expect(listRegisteredPrompts(server)).not.toContain('packing-list'); + const client = await buildServer(user.id); + expect(await listRegisteredPrompts(client)).not.toContain('packing-list'); }); it('prompt is registered when packing addon is enabled', async () => { // isAddonEnabledMock returns true by default const { user } = createUser(testDb); - const server = buildServer(user.id); - expect(listRegisteredPrompts(server)).toContain('packing-list'); + const client = await buildServer(user.id); + expect(await listRegisteredPrompts(client)).toContain('packing-list'); }); it('returns access denied for non-member trip', async () => { @@ -334,8 +387,8 @@ describe('Prompt: packing-list', () => { const { user: other } = createUser(testDb); const trip = createTrip(testDb, other.id); - const server = buildServer(user.id); - const text = await invokePrompt(server, 'packing-list', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'packing-list', { tripId: trip.id }); expect(text.toLowerCase()).toContain('access denied'); }); @@ -343,8 +396,8 @@ describe('Prompt: packing-list', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { title: 'Empty Trip' }); - const server = buildServer(user.id); - const text = await invokePrompt(server, 'packing-list', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'packing-list', { tripId: trip.id }); expect(text).toContain('No packing items found'); }); @@ -354,8 +407,8 @@ describe('Prompt: packing-list', () => { createPackingItem(testDb, trip.id, { name: 'Sunscreen', category: 'Essentials' }); createPackingItem(testDb, trip.id, { name: 'Passport', category: 'Documents' }); - const server = buildServer(user.id); - const text = await invokePrompt(server, 'packing-list', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'packing-list', { tripId: trip.id }); expect(text).toContain('Packing List'); expect(text).toContain('Sunscreen'); expect(text).toContain('Passport'); @@ -373,8 +426,8 @@ describe('Prompt: packing-list', () => { // Null out the getTripSummary call inside packing-list (line 94: || {}) mockGetTripSummary.mockReturnValueOnce(null); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'packing-list', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'packing-list', { tripId: trip.id }); expect(text).toContain('Toothbrush'); // Falls back to 'Trip' literal since trip?.title is undefined (getTripSummary null → || {}) expect(text).toContain('Packing List: Trip'); @@ -389,14 +442,14 @@ describe('Prompt: budget-overview', () => { it('prompt is NOT registered when budget addon is disabled', async () => { isAddonEnabledMock.mockReturnValue(false); const { user } = createUser(testDb); - const server = buildServer(user.id); - expect(listRegisteredPrompts(server)).not.toContain('budget-overview'); + const client = await buildServer(user.id); + expect(await listRegisteredPrompts(client)).not.toContain('budget-overview'); }); it('prompt is registered when budget addon is enabled', async () => { const { user } = createUser(testDb); - const server = buildServer(user.id); - expect(listRegisteredPrompts(server)).toContain('budget-overview'); + const client = await buildServer(user.id); + expect(await listRegisteredPrompts(client)).toContain('budget-overview'); }); it('returns access denied for non-member trip', async () => { @@ -404,8 +457,8 @@ describe('Prompt: budget-overview', () => { const { user: other } = createUser(testDb); const trip = createTrip(testDb, other.id); - const server = buildServer(user.id); - const text = await invokePrompt(server, 'budget-overview', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'budget-overview', { tripId: trip.id }); expect(text.toLowerCase()).toContain('access denied'); }); @@ -413,20 +466,9 @@ describe('Prompt: budget-overview', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { title: 'Budget Trip' }); - const server = buildServer(user.id); - // The prompt destructures budget from getTripSummary, which now returns - // { items, item_count, total, currency } instead of an array. - // prompts.ts calls budget?.reduce() expecting an array — known source discrepancy. - // This test verifies the prompt is reachable and the trip access check passes. - try { - const text = await invokePrompt(server, 'budget-overview', { tripId: trip.id }); - // If source shape matches, text should contain the trip title - expect(text).toContain('Budget Trip'); - } catch (err: any) { - // The TypeError from budget.reduce confirms the trip was accessible - // (access denied produces a message, not an exception). - expect(err.message).toContain('is not a function'); - } + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'budget-overview', { tripId: trip.id }); + expect(text).toContain('Budget Trip'); }); it('produces output for an accessible trip with budget items', async () => { @@ -435,14 +477,10 @@ describe('Prompt: budget-overview', () => { createBudgetItem(testDb, trip.id, { name: 'Flight', category: 'Transport', total_price: 300 }); createBudgetItem(testDb, trip.id, { name: 'Hotel', category: 'Accommodation', total_price: 500 }); - const server = buildServer(user.id); - try { - const text = await invokePrompt(server, 'budget-overview', { tripId: trip.id }); - expect(text).toContain('Italy Trip'); - } catch (err: any) { - // Confirms trip was accessible; TypeError from budget.reduce is a source discrepancy - expect(err.message).toContain('is not a function'); - } + const client = await buildServer(user.id); + const text = await invokePrompt(client, 'budget-overview', { tripId: trip.id }); + expect(text).toContain('Italy Trip'); + expect(text).toContain('Total: 800'); }); it('returns "Trip not found." when getTripSummary returns null for accessible trip', async () => { @@ -452,8 +490,8 @@ describe('Prompt: budget-overview', () => { // Override mock to return null (covers lines 116-118 in prompts.ts) mockGetTripSummary.mockReturnValueOnce(null); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'budget-overview', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'budget-overview', { tripId: trip.id }); expect(text).toContain('Trip not found.'); }); @@ -466,8 +504,8 @@ describe('Prompt: budget-overview', () => { createBudgetItem(testDb, trip.id, { name: 'Bus', category: 'Transport', total_price: 50 }); createBudgetItem(testDb, trip.id, { name: 'Hotel', category: 'Accommodation', total_price: 300 }); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'budget-overview', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'budget-overview', { tripId: trip.id }); expect(text).toContain('Budget Trip'); expect(text).toContain('Transport'); expect(text).toContain('Accommodation'); @@ -479,8 +517,8 @@ describe('Prompt: budget-overview', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { title: 'Empty Budget' }); - const server = buildServer(user.id); - const text = await invokePromptText(server, 'budget-overview', { tripId: trip.id }); + const client = await buildServer(user.id); + const text = await invokePromptText(client, 'budget-overview', { tripId: trip.id }); expect(text).toContain('No expenses recorded.'); }); }); diff --git a/server/tests/unit/nest-mcp/attach.test.ts b/server/tests/unit/nest-mcp/attach.test.ts index d8324c538..ba5e826e0 100644 --- a/server/tests/unit/nest-mcp/attach.test.ts +++ b/server/tests/unit/nest-mcp/attach.test.ts @@ -8,6 +8,7 @@ import { Tool, type McpAccessPolicy, type McpContext, + type PromptArgsShape, } from '../../../src/nest-mcp'; import { createAttachHarness, type AttachHarness, type TestCtx } from './harness'; @@ -71,6 +72,17 @@ class FixtureMcp { async fixturePrompt({ topic }: { topic: string }, ctx: TestCtx) { return { messages: [{ role: 'user', content: { type: 'text', text: `${topic} for ${ctx.userId}` } }] }; } + + // Arguments reach the server as strings; a numeric one is parsed on the way in. + @Prompt({ name: 'fixture_count', argsSchema: { count: z.string().regex(/^\d+$/).transform(Number) } }) + async fixtureCount({ count }: { count: number }, ctx: TestCtx) { + return { messages: [{ role: 'user', content: { type: 'text', text: `${typeof count} ${count + 1} for ${ctx.userId}` } }] }; + } + + @Prompt({ name: 'fixture_bare', argsSchema: {} }) + async fixtureBare(args: Record, ctx: TestCtx) { + return { messages: [{ role: 'user', content: { type: 'text', text: `${Object.keys(args).length} args for ${ctx.userId}` } }] }; + } } @McpController() @@ -143,8 +155,8 @@ describe('McpRegistry.attach', () => { expect(resources).toEqual(['fixture_doc']); const templates = (await harness.client.listResourceTemplates()).resourceTemplates.map((t) => t.name); expect(templates).toEqual(['fixture_item']); - const prompts = (await harness.client.listPrompts()).prompts.map((p) => p.name); - expect(prompts).toEqual(['fixture_prompt']); + const prompts = (await harness.client.listPrompts()).prompts.map((p) => p.name).sort(); + expect(prompts).toEqual(['fixture_bare', 'fixture_count', 'fixture_prompt']); }); it('filters declarative access through the policy per mode', async () => { @@ -200,6 +212,34 @@ describe('McpRegistry.attach', () => { expect(result.messages[0]?.content).toMatchObject({ type: 'text', text: 'packing for 3' }); }); + it('hands prompt handlers the transformed argument, not the wire string (#2207)', async () => { + harness = await createAttachHarness(buildRegistry(), { userId: 3 }); + const result = await harness.client.getPrompt({ name: 'fixture_count', arguments: { count: '41' } }); + expect(result.messages[0]?.content).toMatchObject({ type: 'text', text: 'number 42 for 3' }); + await expect(harness.client.getPrompt({ name: 'fixture_count', arguments: { count: 'many' } })) + .rejects.toThrow(/Invalid arguments for prompt fixture_count/); + }); + + it('serves a prompt with an empty argsSchema to a request that omits arguments', async () => { + harness = await createAttachHarness(buildRegistry(), { userId: 3 }); + const result = await harness.client.getPrompt({ name: 'fixture_bare' }); + expect(result.messages[0]?.content).toMatchObject({ type: 'text', text: '0 args for 3' }); + const listed = (await harness.client.listPrompts()).prompts.find((p) => p.name === 'fixture_bare'); + expect(listed?.arguments).toBeUndefined(); + }); + + it('only admits string-input schemas as prompt arguments', () => { + // Compile-time contract: a z.number() argument can never be satisfied over + // the wire, so PromptArgsShape refuses it while string, optional string and + // string-to-number transforms all pass. + type Admits = S extends PromptArgsShape ? true : false; + const numberRefused: Admits<{ tripId: z.ZodNumber }> = false; + const stringAdmitted: Admits<{ topic: z.ZodString }> = true; + const optionalAdmitted: Admits<{ topic: z.ZodOptional }> = true; + const transformAdmitted: Admits<{ count: z.ZodPipe> }> = true; + expect([numberRefused, stringAdmitted, optionalAdmitted, transformAdmitted]).toEqual([false, true, true, true]); + }); + it('ANDs `when` in front of declarative access (both must pass)', async () => { const registry = createTestRegistry([new WhenMcp()], { accessPolicy: policy }); harness = await createAttachHarness(registry, { userId: 7, canRead: true, allow: true });