diff --git a/server/src/nest-mcp/README.md b/server/src/nest-mcp/README.md index f10535219..0bbf9bd1e 100644 --- a/server/src/nest-mcp/README.md +++ b/server/src/nest-mcp/README.md @@ -24,7 +24,7 @@ export class ThingsMcp { @Tool({ name: 'list_things', description: 'List all things.', - inputSchema: {}, // ZodRawShape, passed straight to the SDK + inputSchema: {}, // ZodRawShape, registered as a strict object annotations: { readOnlyHint: true }, access: { group: 'things', mode: 'read' }, // resolved by the accessPolicy }) @@ -34,7 +34,7 @@ export class ThingsMcp { } ``` -- `@Tool(options)` — mirrors `server.registerTool` (`name`, `title`, `description`, `inputSchema`, `outputSchema`, `annotations`, `_meta`). Handler: `(args, ctx)`; `args` is `{}` when no `inputSchema` was declared. +- `@Tool(options)` — mirrors `server.registerTool` (`name`, `title`, `description`, `inputSchema`, `outputSchema`, `annotations`, `_meta`). Handler: `(args, ctx)`; `args` is `{}` when no `inputSchema` was declared. A shape is registered as `z.strictObject`, so an argument the tool never declared is refused (`Unrecognized key`) and `tools/list` advertises `additionalProperties: false`; the SDK's own wrapping would strip it and run the handler regardless. A whole Zod schema is passed through as written. - `@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)`. diff --git a/server/src/nest-mcp/registry.ts b/server/src/nest-mcp/registry.ts index ab020875e..acfa565cd 100644 --- a/server/src/nest-mcp/registry.ts +++ b/server/src/nest-mcp/registry.ts @@ -15,12 +15,24 @@ import type { } from './types'; import { ResourceTemplate as SdkResourceTemplate } from '@modelcontextprotocol/sdk/server/mcp'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; +import { z } from 'zod'; interface BoundEntry { entry: McpEntry; instance: object; } +/** + * A shape is registered strict: the SDK's own z.object wrapping would strip an + * undeclared key and run the handler as if nothing was wrong, and strict also + * puts `additionalProperties: false` into tools/list. A whole schema is the + * author's call and passes through untouched. + */ +function strictInputSchema(schema: ToolOptions['inputSchema']): unknown { + if (schema === undefined || '_zod' in schema) return schema; + return z.strictObject(schema); +} + export interface McpRegistryOptions { accessPolicy?: McpAccessPolicy; validateAccess?: McpAccessValidator; @@ -30,9 +42,9 @@ type AnyHandler = (this: unknown, ...handlerArgs: unknown[]) => unknown; /** * Structural view of the SDK registration surface. The SDK's real signatures - * are generic over the Zod shapes; the registry passes schemas straight - * through and binds `ctx` where the SDK would pass `extra`, so it needs (and - * exposes) none of that type-level machinery. + * are generic over the Zod shapes; the registry hands schemas over as objects + * and binds `ctx` where the SDK would pass `extra`, so it needs (and exposes) + * none of that type-level machinery. */ interface LooseRegistrar { registerTool(name: string, config: Record, cb: (...cbArgs: unknown[]) => unknown): unknown; @@ -294,7 +306,7 @@ export class McpRegistry { const config: Record = { title: options.title, description: options.description, - inputSchema: options.inputSchema, + inputSchema: strictInputSchema(options.inputSchema), outputSchema: options.outputSchema, annotations: options.annotations, _meta: options._meta, diff --git a/server/src/nest-mcp/types.ts b/server/src/nest-mcp/types.ts index 82b895fa1..e8e19686e 100644 --- a/server/src/nest-mcp/types.ts +++ b/server/src/nest-mcp/types.ts @@ -126,8 +126,9 @@ export interface McpZodSchema { } /** - * Mirrors the SDK's `registerTool` config. `inputSchema` is passed straight - * through — the SDK does the parsing/validation. + * Mirrors the SDK's `registerTool` config. A shape `inputSchema` is registered + * as a strict object (see registry.ts); a whole schema is passed as written. The + * SDK does the parsing/validation either way. * `annotations`/`_meta` are kept structural so the package's public d.ts * never imports deep SDK subpaths. */ diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 5664e0c92..77df7e97b 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -25,13 +25,13 @@ const budgetAddonOn = addonGate(ADDON_IDS.BUDGET); * signed like the REST contract's (#2176): a negative payer is the recipient * of a refund recorded as a negative expense. */ -const payersSchema = z.array(z.object({ +const payersSchema = z.array(z.strictObject({ user_id: z.number().int().positive(), amount: z.number(), })).describe('Who actually paid, and how much each paid, in the expense currency. Ask the user; do not guess.'); /** Reusable Zod shape for an unequal split: what each participant owes. Signed, like the REST contract (#2176). */ -const splitMembersSchema = z.array(z.object({ +const splitMembersSchema = z.array(z.strictObject({ user_id: z.number().int().positive(), amount: z.number(), })).describe('Unequal split: what each participant owes, in the expense currency. The amounts must add up to the expense total. Ask the user; do not guess.'); diff --git a/server/src/nest/journey/journey.mcp.ts b/server/src/nest/journey/journey.mcp.ts index 57a1aa719..46ed01502 100644 --- a/server/src/nest/journey/journey.mcp.ts +++ b/server/src/nest/journey/journey.mcp.ts @@ -63,7 +63,7 @@ type EntryVisibility = z.infer; * only the half it has, which is what the entry editor does when a place was * all good or all bad. The service stores nothing when both come back empty. */ -const PROS_CONS = z.object({ +const PROS_CONS = z.strictObject({ pros: z.array(z.string()).default([]), cons: z.array(z.string()).default([]), }); diff --git a/server/src/nest/maps/maps.mcp.ts b/server/src/nest/maps/maps.mcp.ts index 156e1d441..9f6b0e7ec 100644 --- a/server/src/nest/maps/maps.mcp.ts +++ b/server/src/nest/maps/maps.mcp.ts @@ -95,7 +95,7 @@ export class MapsMcp { z.enum(POI_CATEGORY_KEYS), z.array(z.enum(POI_CATEGORY_KEYS)).min(1).max(MAX_POI_CATEGORIES), ]).describe('Which kind of place to look for. Pass several to cover them in one query rather than one request per kind'), - bbox: z.object({ + bbox: z.strictObject({ south: z.number().min(-90).max(90).describe('Southern edge, latitude'), west: z.number().min(-180).max(180).describe('Western edge, longitude'), north: z.number().min(-90).max(90).describe('Northern edge, latitude'), diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index 047bc2cf5..f4e04031d 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -511,7 +511,7 @@ export class PackingMcp { description: 'Import multiple packing items at once from a list. Optionally assign each to a bag (by name — created if missing), set its weight, or pre-check it.', inputSchema: { tripId: z.number().int().positive(), - items: z.array(z.object({ + items: z.array(z.strictObject({ name: z.string().min(1).max(200), category: z.string().optional(), quantity: z.number().int().positive().optional(), diff --git a/server/src/nest/plugins/contributions/plugin-search.mcp.ts b/server/src/nest/plugins/contributions/plugin-search.mcp.ts index 056e5f27f..1f7d77fbd 100644 --- a/server/src/nest/plugins/contributions/plugin-search.mcp.ts +++ b/server/src/nest/plugins/contributions/plugin-search.mcp.ts @@ -35,7 +35,7 @@ export class PluginSearchMcp { description: 'Search for a place in the search indexes installed plugins provide, which are the ones TREK does not ship itself. Use it alongside search_place, never instead of it: search_place is TREK\'s own index and OpenStreetMap, this is whatever else the instance owner installed, and only these results can carry a rating — open data has none, so a question like "the best rated hotel near here" can only be answered from this list. Results have the same shape search_place returns, plus a `rating` and the `pluginId` that found them. Returns an empty list when no plugin provides a search index, which is the normal case.', inputSchema: { query: z.string().min(1).max(MAX_QUERY).describe('Place name or address to search for'), - near: z.object({ lat: z.number(), lng: z.number() }) + near: z.strictObject({ lat: z.number(), lng: z.number() }) .optional() .describe('Centre the search on a coordinate. Pass it whenever the trip has a destination: a bare name like "Central Station" otherwise resolves wherever the provider guesses'), lang: z.string().max(20).optional().describe('BCP 47 language for the result names, e.g. "de" or "ja"'), diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index fba5bb671..8fb29e9f1 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -47,7 +47,7 @@ const urlField = reservationUrlSchema.max(2000).optional() type TransportType = typeof CREATABLE_TRANSPORT_TYPES[number]; type BookingType = typeof BOOKING_TYPES[number]; -const endpointObjectSchema = z.object({ +const endpointObjectSchema = z.strictObject({ role: z.enum(['from', 'to', 'stop']).describe('Endpoint role: "from" (origin), "to" (destination), or "stop" (intermediate)'), sequence: z.number().int().min(0).describe('Order within the route (0-based)'), name: z.string().min(1).describe('Location name (e.g. "Paris Gare de Lyon", "ZRH Terminal 2")'), @@ -520,7 +520,7 @@ export class ReservationsMcp { description: 'Update the display order of reservations within a day.', inputSchema: { tripId: z.number().int().positive(), - positions: z.array(z.object({ + positions: z.array(z.strictObject({ id: z.number().int().positive(), day_plan_position: z.number().int().min(0), })).describe('Array of { id, day_plan_position } pairs'), diff --git a/server/src/nest/roadtrip/roadtrip.mcp.ts b/server/src/nest/roadtrip/roadtrip.mcp.ts index c584bbab6..33de03f00 100644 --- a/server/src/nest/roadtrip/roadtrip.mcp.ts +++ b/server/src/nest/roadtrip/roadtrip.mcp.ts @@ -89,14 +89,14 @@ export class RoadtripMcp { inputSchema: { tripId: z.number().int().positive(), dayId: z.number().int().positive(), - vias: z.array(z.object({ + vias: z.array(z.strictObject({ after_order_index: z.number().int().min(0).describe('Which stop of the day this via follows, counting from 0'), lat: z.number().min(-90).max(90), lng: z.number().min(-180).max(180), })).max(100).describe('In the order the drive passes through them'), replace_legs: z.array(z.number().int().min(0)).max(100).optional() .describe('Legs to clear before inserting, by the index of the stop they follow'), - track: z.object({ + track: z.strictObject({ place_id: z.number().int().positive().describe('The imported track this chain was fitted to, as its place id'), stray_km: z.number().min(0).max(40_000).nullable().optional() .describe('How far the fitted route still runs from the track at its worst point'), @@ -139,7 +139,7 @@ export class RoadtripMcp { inputSchema: { tripId: z.number().int().positive(), dayId: z.number().int().positive(), - vias: z.array(z.object({ + vias: z.array(z.strictObject({ id: z.number().int().positive(), after_order_index: z.number().int().min(0).describe('Which stop of the day the via now follows, counting from 0'), })).max(500), diff --git a/server/tests/unit/mcp/tools-places.test.ts b/server/tests/unit/mcp/tools-places.test.ts index 85b88acb2..d48ac5d7a 100644 --- a/server/tests/unit/mcp/tools-places.test.ts +++ b/server/tests/unit/mcp/tools-places.test.ts @@ -247,6 +247,23 @@ describe('Tool: update_place', () => { }); }); + it('refuses a misspelt field instead of silently updating nothing', async () => { + // An assistant wrote stopType for stop_type; the call used to come back as a + // success with the place untouched, and the road trip never got its fuel stop. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Aral Bad Segeberg' }); + await withHarness(user.id, async (h) => { + const refused = await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: place.id, stopType: 'fuel' } }); + expect(refused.isError).toBe(true); + expect((refused.content as Array<{ text: string }>)[0].text).toMatch(/Unrecognized key.*stopType/); + expect((testDb.prepare('SELECT stop_type FROM places WHERE id = ?').get(place.id) as { stop_type: string | null }).stop_type).toBeNull(); + expect(broadcastMock).not.toHaveBeenCalled(); + const fixed = parseToolResult(await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: place.id, stop_type: 'fuel' } })) as any; + expect(fixed.place.stop_type).toBe('fuel'); + }); + }); + it('sets image_url on an existing place (#37)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest-mcp/attach.test.ts b/server/tests/unit/nest-mcp/attach.test.ts index ba5e826e0..42fc8c4de 100644 --- a/server/tests/unit/nest-mcp/attach.test.ts +++ b/server/tests/unit/nest-mcp/attach.test.ts @@ -43,6 +43,13 @@ class FixtureMcp { return { content: [{ type: 'text', text: `wrote as ${ctx.userId}` }] }; } + // A whole schema, the way a plugin's manifest-built tool arrives: its author + // decided how open it is, and the registry must not tighten it. + @Tool({ name: 'loose_tool', inputSchema: z.looseObject({ name: z.string() }) }) + async looseTool(args: Record, ctx: TestCtx) { + return { content: [{ type: 'text', text: JSON.stringify({ args, userId: ctx.userId }) }] }; + } + @Tool({ name: 'predicate_tool', access: (ctx: McpContext) => (ctx as TestCtx).allow === true }) async predicateTool(args: Record, ctx: TestCtx) { return { content: [{ type: 'text', text: JSON.stringify({ args, userId: ctx.userId }) }] }; @@ -150,7 +157,7 @@ describe('McpRegistry.attach', () => { it('registers everything for a ctx passing all access checks', async () => { harness = await createAttachHarness(buildRegistry(), { userId: 7, canRead: true, canWrite: true, allow: true }); const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort(); - expect(tools).toEqual(['open_tool', 'predicate_tool', 'read_tool', 'write_tool']); + expect(tools).toEqual(['loose_tool', 'open_tool', 'predicate_tool', 'read_tool', 'write_tool']); const resources = (await harness.client.listResources()).resources.map((r) => r.name); expect(resources).toEqual(['fixture_doc']); const templates = (await harness.client.listResourceTemplates()).resourceTemplates.map((t) => t.name); @@ -162,13 +169,13 @@ describe('McpRegistry.attach', () => { it('filters declarative access through the policy per mode', async () => { harness = await createAttachHarness(buildRegistry(), { userId: 7, canRead: true, canWrite: false, allow: false }); const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort(); - expect(tools).toEqual(['open_tool', 'read_tool']); + expect(tools).toEqual(['loose_tool', 'open_tool', 'read_tool']); }); it('filters everything gated when the ctx grants nothing, keeping ungated entries', async () => { harness = await createAttachHarness(buildRegistry(), { userId: 7 }); const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort(); - expect(tools).toEqual(['open_tool']); + expect(tools).toEqual(['loose_tool', 'open_tool']); expect((await harness.client.listResources()).resources).toEqual([]); const templates = (await harness.client.listResourceTemplates()).resourceTemplates.map((t) => t.name); expect(templates).toEqual(['fixture_item']); @@ -188,6 +195,32 @@ describe('McpRegistry.attach', () => { expect(JSON.parse(textOf(result))).toEqual({ greeting: 'hello trek', userId: 42 }); }); + it('refuses an argument the tool never declared instead of stripping it', async () => { + harness = await createAttachHarness(buildRegistry(), { userId: 42, canRead: true, canWrite: true, allow: true }); + // A shape is registered strict: the SDK would otherwise drop `nmae` and run + // the handler as if the caller had sent nothing wrong. + const refused = await harness.client.callTool({ name: 'read_tool', arguments: { name: 'trek', nmae: 'typo' } }); + expect(refused.isError).toBe(true); + expect(textOf(refused)).toMatch(/Unrecognized key.*nmae/); + // An empty shape is strict too: nothing declared means nothing accepted. + const stray = await harness.client.callTool({ name: 'write_tool', arguments: { anything: 1 } }); + expect(stray.isError).toBe(true); + expect(textOf(stray)).toMatch(/Unrecognized key.*anything/); + expect(textOf(await harness.client.callTool({ name: 'write_tool', arguments: {} }))).toBe('wrote as 42'); + // tools/list says so up front, so a client that reads the schema never sends it. + const tools = (await harness.client.listTools()).tools; + expect(tools.find((t) => t.name === 'read_tool')?.inputSchema).toMatchObject({ additionalProperties: false }); + expect(tools.find((t) => t.name === 'write_tool')?.inputSchema).toMatchObject({ additionalProperties: false }); + }); + + it('hands a whole schema to the SDK as written, however open its author left it', async () => { + harness = await createAttachHarness(buildRegistry(), { userId: 42 }); + const result = await harness.client.callTool({ name: 'loose_tool', arguments: { name: 'trek', extra: 'kept' } }); + expect(JSON.parse(textOf(result))).toEqual({ args: { name: 'trek', extra: 'kept' }, userId: 42 }); + const loose = (await harness.client.listTools()).tools.find((t) => t.name === 'loose_tool'); + expect(loose?.inputSchema).not.toHaveProperty('additionalProperties', false); + }); + it('normalizes args to {} for tools declared without an inputSchema', async () => { harness = await createAttachHarness(buildRegistry(), { userId: 42 }); const result = await harness.client.callTool({ name: 'open_tool', arguments: {} }); diff --git a/wiki/MCP-Tools-and-Resources.md b/wiki/MCP-Tools-and-Resources.md index 7edcf4acf..63a27d60a 100644 --- a/wiki/MCP-Tools-and-Resources.md +++ b/wiki/MCP-Tools-and-Resources.md @@ -2,6 +2,8 @@ TREK exposes **tools** (read and write actions) and **resources** (read-only `trek://` URIs). Tools are registered per-session based on OAuth scopes and enabled addons. +Every built-in tool checks its arguments against its published schema before anything runs. An argument the tool does not declare (a misspelt field such as `stopType` for `stop_type`) is refused with `Unrecognized key`, so the AI sees the mistake and can retry rather than reporting a change that never happened. Plugin-contributed tools follow the schema their manifest declares. + For addon-gated tools (Packing, To-Dos, Atlas, Collab, Collections, Vacay, Journey, Dawarich, Document sync, Road trip) and their resources, see [MCP-Addon-Tools](MCP-Addon-Tools). ## Tools