From e510ff25ab896546582e7bfe1ae355ea5b41f7b3 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 22 Sep 2026 09:55:45 +0200 Subject: [PATCH] fix(mcp): refuse an argument a tool never declared instead of dropping it The SDK wraps a raw shape in a plain z.object, which strips an undeclared key and runs the handler as if nothing was wrong: an assistant that wrote stopType for stop_type was told the place was updated while nothing had changed. The registry registers every shape as z.strictObject, so the call answers "Unrecognized key" and tools/list advertises additionalProperties false. The nested objects a tool takes, authored inline or spread from a shared schema (legs, stops, links, transit places), are strict at this boundary for the same reason; the shared schemas themselves stay open for REST. A whole Zod schema, the way a plugin's manifest-built tool arrives, passes through as its author wrote it. --- server/src/nest-mcp/README.md | 4 +- server/src/nest-mcp/registry.ts | 20 ++++++++-- server/src/nest-mcp/types.ts | 5 ++- server/src/nest/budget/budget.mcp.ts | 4 +- .../src/nest/collections/collections.mcp.ts | 12 ++++-- server/src/nest/journey/journey.mcp.ts | 2 +- server/src/nest/maps/maps.mcp.ts | 2 +- server/src/nest/packing/packing.mcp.ts | 2 +- server/src/nest/places/places.mcp.ts | 8 ++-- .../contributions/plugin-search.mcp.ts | 2 +- .../src/nest/reservations/reservations.mcp.ts | 9 +++-- server/src/nest/roadtrip/google-route.mcp.ts | 4 +- server/src/nest/roadtrip/roadtrip.mcp.ts | 6 +-- server/src/nest/settings/settings.module.ts | 2 +- server/src/nest/transit/transit.mcp.ts | 13 ++++--- server/tests/unit/mcp/tools-places.test.ts | 17 ++++++++ server/tests/unit/nest-mcp/attach.test.ts | 39 +++++++++++++++++-- wiki/MCP-Tools-and-Resources.md | 2 + 18 files changed, 113 insertions(+), 40 deletions(-) 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/collections/collections.mcp.ts b/server/src/nest/collections/collections.mcp.ts index c90bcdcc8..7761c9ffc 100644 --- a/server/src/nest/collections/collections.mcp.ts +++ b/server/src/nest/collections/collections.mcp.ts @@ -15,6 +15,7 @@ import { collectionCopyToTripRequestSchema, collectionLabelCreateRequestSchema, collectionLabelUpdateRequestSchema, collectionLabelAssignRequestSchema, collectionInviteRequestSchema, collectionSetStatusFromTripRequestSchema, + collectionLinkSchema, COLLECTION_STATUSES, COLLECTION_ROLES, } from '@trek/shared'; import type { @@ -53,6 +54,9 @@ const collectionsAddonOn = addonGate(ADDON_IDS.COLLECTIONS); * plus the `when:` collections-addon gate the legacy registrar lacked (fixed * in the trailing quirk commit — REST and the plugin host always gated). */ +// The shared link schema stays open for REST; a tool refuses a key it did not declare. +const linksInput = z.array(z.strictObject(collectionLinkSchema.shape)).max(30).optional(); + @McpController() export class CollectionsMcp { constructor( @@ -138,7 +142,7 @@ export class CollectionsMcp { @Tool({ name: 'create_collection', description: 'Create a new saved-place collection (list) owned by the user.', - inputSchema: collectionCreateRequestSchema.shape, + inputSchema: { ...collectionCreateRequestSchema.shape, links: linksInput }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, @@ -151,7 +155,7 @@ export class CollectionsMcp { @Tool({ name: 'update_collection', description: 'Update a collection\'s name, description, colour, icon, cover, links, or sort order. Owner/admin only.', - inputSchema: { collectionId: z.number().int().positive(), ...collectionUpdateRequestSchema.shape }, + inputSchema: { collectionId: z.number().int().positive(), ...collectionUpdateRequestSchema.shape, links: linksInput }, annotations: TOOL_ANNOTATIONS_WRITE, when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, @@ -192,7 +196,7 @@ export class CollectionsMcp { @Tool({ name: 'save_place_to_collection', description: 'Save a place into a collection from a raw payload (name required; set google_place_id/osm_id from search_place for rich details). Returns a duplicate marker instead of saving when a similar place already exists, unless force is true.', - inputSchema: collectionSavePlaceRequestSchema.shape, + inputSchema: { ...collectionSavePlaceRequestSchema.shape, links: linksInput }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, @@ -226,7 +230,7 @@ export class CollectionsMcp { @Tool({ name: 'update_collection_place', description: 'Update a saved place\'s name, address, coordinates (lat/lng), description, notes, status, category, links, tags, labels, image, or move it to another collection (set collection_id).', - inputSchema: { placeId: z.number().int().positive(), ...collectionPlaceUpdateRequestSchema.shape }, + inputSchema: { placeId: z.number().int().positive(), ...collectionPlaceUpdateRequestSchema.shape, links: linksInput }, annotations: TOOL_ANNOTATIONS_WRITE, when: collectionsAddonOn, access: { group: 'collections', mode: 'write' }, 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/places/places.mcp.ts b/server/src/nest/places/places.mcp.ts index b8cd1bb95..2f9649331 100644 --- a/server/src/nest/places/places.mcp.ts +++ b/server/src/nest/places/places.mcp.ts @@ -80,7 +80,7 @@ export class PlacesMcp { image_url: placeImageUrlSchema.optional().describe('Thumbnail for the place: an /uploads/ path, an /api/maps/place-photo/ path, an inline data: image, or an https URL'), price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'), currency: z.string().length(3).optional().describe('ISO 4217 currency code (e.g. "EUR", "USD")'), - stop_type: roadtripStopTypeSchema.optional().describe('Marks the place as a stop on a drive rather than a destination: fuel, charging, rest_area, campsite, restaurant, sights or hotel. A service stop is left out of the stop count for the day, so a day with a charger between four places still reads as four stops. Leave unset for an ordinary place.'), + stop_type: roadtripStopTypeSchema.optional().describe('Marks the place as a stop on a drive rather than a destination: fuel, charging, rest_area, campsite, restaurant, sights or hotel. A service stop is left out of the stop count for the day, so a day with a charger between four places still reads as four stops. Leave unset for an ordinary place. calculate_roadtrip reports the same value on a stop as stopType; this field is stop_type.'), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, access: { group: 'places', mode: 'write' }, @@ -125,7 +125,7 @@ export class PlacesMcp { assignment_notes: z.string().max(500).optional().describe('Notes for this day assignment'), price: z.number().nonnegative().optional().describe('Cost of this place/activity (e.g. ticket price, entry fee)'), currency: z.string().length(3).optional().describe('ISO 4217 currency code (e.g. "EUR", "USD")'), - stop_type: roadtripStopTypeSchema.optional().describe('Marks the place as a stop on a drive rather than a destination: fuel, charging, rest_area, campsite, restaurant, sights or hotel. A service stop is left out of the stop count for the day, so a day with a charger between four places still reads as four stops. Leave unset for an ordinary place.'), + stop_type: roadtripStopTypeSchema.optional().describe('Marks the place as a stop on a drive rather than a destination: fuel, charging, rest_area, campsite, restaurant, sights or hotel. A service stop is left out of the stop count for the day, so a day with a charger between four places still reads as four stops. Leave unset for an ordinary place. calculate_roadtrip reports the same value on a stop as stopType; this field is stop_type.'), }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, access: { group: 'places', mode: 'write' }, @@ -184,7 +184,7 @@ export class PlacesMcp { google_place_id: z.string().optional().describe('Google Place ID (e.g. "ChIJd8BlQ2BZwokRAFUEcm_qrcA")'), google_ftid: z.string().optional().describe('Google Maps feature ID (e.g. "0x89c259b7abdd4769:0x103aaf1c8bf8a050")'), amap_poi_id: z.string().optional().describe('Amap (高德地图) POI ID, amap:-prefixed (e.g. "amap:B000A83M61")'), - stop_type: roadtripStopTypeSchema.nullable().optional().describe('What kind of stop on a drive this is: fuel, charging, rest_area, campsite, restaurant, sights or hotel. Pass null to turn a service stop back into an ordinary place.'), + stop_type: roadtripStopTypeSchema.nullable().optional().describe('What kind of stop on a drive this is: fuel, charging, rest_area, campsite, restaurant, sights or hotel. Pass null to turn a service stop back into an ordinary place. calculate_roadtrip reports the same value on a stop as stopType; this field is stop_type.'), fill_percent: z.number().int().min(1).max(100).nullable().optional().describe('How full THIS stop fills the tank, 1-100. A motorway rapid charger is worth about 80 %, the one at the hotel 100 %. Pass null to follow whatever the traveller set as their default fill.'), }, annotations: TOOL_ANNOTATIONS_WRITE, @@ -464,7 +464,7 @@ export class PlacesMcp { phone: z.string().max(50).optional(), image_url: placeImageUrlSchema.nullable().optional().describe('Thumbnail for every listed place: an /uploads/ path, an /api/maps/place-photo/ path, an inline data: image, or an https URL. Pass null to strip the pictures off a batch at once'), description: z.string().max(2000).optional(), - stop_type: roadtripStopTypeSchema.nullable().optional().describe('What kind of stop on a drive this is: fuel, charging, rest_area, campsite, restaurant, sights or hotel. Pass null to turn a service stop back into an ordinary place.'), + stop_type: roadtripStopTypeSchema.nullable().optional().describe('What kind of stop on a drive this is: fuel, charging, rest_area, campsite, restaurant, sights or hotel. Pass null to turn a service stop back into an ordinary place. calculate_roadtrip reports the same value on a stop as stopType; this field is stop_type.'), fill_percent: z.number().int().min(1).max(100).nullable().optional().describe('How full THIS stop fills the tank, 1-100. A motorway rapid charger is worth about 80 %, the one at the hotel 100 %. Pass null to follow whatever the traveller set as their default fill.'), }, annotations: TOOL_ANNOTATIONS_WRITE, 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..4e3772437 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -14,7 +14,7 @@ import { DaysService } from '../days/days.service'; import { findByIata } from '../airports/airports.data'; import type { EndpointInput } from './reservations.service'; import { AssignmentsService } from '../assignments/assignments.service'; -import { transportLegsInputSchema, reservationUrlSchema, type TransportLegInput } from '@trek/shared'; +import { transportLegInputSchema, reservationUrlSchema, type TransportLegInput } from '@trek/shared'; // What counts as a transport booking, for the update_transport gate. Every value // ReservationsPanel renders with a transport icon, so a stored `transit` row is @@ -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")'), @@ -115,7 +115,8 @@ function parseId(value: string | string[]): number | null { // and the importers write, and keep the two stores in step. // --------------------------------------------------------------------------- -const legsSchema = transportLegsInputSchema.optional().describe( +// Strict at this boundary: a leg written as flightNumber must not save half-empty. +const legsSchema = z.array(z.strictObject(transportLegInputSchema.shape)).optional().describe( 'Per-segment detail of a stopover flight or train: one entry per segment, in route order, exactly ONE FEWER than endpoints[]. ' + 'Each leg carries its own departure and arrival day + local time (dep_day_id/dep_time, arr_day_id/arr_time) plus airline/flight_number (flights) or train_number/platform (trains). ' + 'A leg may also carry its own confirmation_number when that segment was issued a separate booking reference; leave it out and the booking-level confirmation_number covers the segment. ' @@ -520,7 +521,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/google-route.mcp.ts b/server/src/nest/roadtrip/google-route.mcp.ts index 3a5e6a566..8050f931d 100644 --- a/server/src/nest/roadtrip/google-route.mcp.ts +++ b/server/src/nest/roadtrip/google-route.mcp.ts @@ -1,5 +1,5 @@ import { z } from 'zod'; -import { googleRouteImportSchema, googleRoutePreviewRequestSchema, type GoogleRouteImport } from '@trek/shared'; +import { googleRouteImportSchema, googleRoutePreviewRequestSchema, googleRouteStopSchema, type GoogleRouteImport } from '@trek/shared'; import { McpController, Tool, TOOL_ANNOTATIONS_READONLY, TOOL_ANNOTATIONS_NON_IDEMPOTENT, ok, type McpContext } from '../../nest-mcp'; import { ADDON_IDS } from '../../addons'; import { AddonsService } from '../addons/addons.service'; @@ -18,7 +18,7 @@ export class GoogleRouteMcp { async preview({ url }: { url: string }) { return answeringRefusals(async () => ok(await this.routes.preview(url))); } @Tool({ name: 'import_google_maps_route', description: 'Append reviewed Google Maps stops to an existing trip day in supplied order. Creates places and visits atomically. Requires place and day editing permissions. Existing visits remain. TREK calculates the road geometry; no Google route geometry is preserved.', - inputSchema: { tripId: z.number().int().positive(), ...googleRouteImportSchema.shape }, + inputSchema: { tripId: z.number().int().positive(), ...googleRouteImportSchema.shape, stops: z.array(z.strictObject(googleRouteStopSchema.shape)).min(2).max(30) }, annotations: TOOL_ANNOTATIONS_NON_IDEMPOTENT, // What the tool writes, not where the link came from: this inserts up to // thirty places and their day assignments, which is exactly what create_place 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/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index 25152f243..6ca1120b6 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -6,7 +6,7 @@ import { SettingsService } from './settings.service'; import { SettingsMcp } from './settings.mcp'; import { AppConfigModule } from '../app-config/app-config.module'; -/** Exports SettingsService for in-container consumers (admin, share, llm-parse). */ +/** Exports SettingsService for in-container consumers (admin, share, llm-parse, journey, roadtrip, trips). */ @Module({ // AuthModule for the admin gate on the defaults routes, and for the // AuthService that SettingsMcp's demo gate injects. diff --git a/server/src/nest/transit/transit.mcp.ts b/server/src/nest/transit/transit.mcp.ts index a76408d4a..425a81ea2 100644 --- a/server/src/nest/transit/transit.mcp.ts +++ b/server/src/nest/transit/transit.mcp.ts @@ -57,6 +57,9 @@ function rateLimit(userId: number, bucket: string, max: number) { * days/reservations bridge imports became injected services; the itinerary * schemas/helpers live in the colocated transit-itinerary.helpers.ts. */ +// The shared place schema stays open for REST; a tool refuses a key it did not declare. +const transitPlaceInput = z.strictObject(transitPlaceSchema.shape); + @McpController() export class TransitMcp { constructor( @@ -76,7 +79,7 @@ export class TransitMcp { query: z.string().min(2).max(200), language: z.string().min(2).max(5).optional(), near: z - .object(transitCoordinatesSchema.shape) + .strictObject(transitCoordinatesSchema.shape) .optional() .describe('Optional coordinates used to bias nearby results'), }, @@ -101,8 +104,8 @@ export class TransitMcp { description: 'Search scheduled public-transit routes between two coordinates, via whichever backend the instance is configured for. Returns itineraries that can be passed unchanged to create_transit_journey. `dropped` counts provider itineraries that failed validation and are therefore absent from the results — a non-zero value means the provider offered routes this tool could not represent.', inputSchema: { - from: transitPlaceSchema, - to: transitPlaceSchema, + from: transitPlaceInput, + to: transitPlaceInput, time: z .string() .datetime({ offset: true }) @@ -162,8 +165,8 @@ export class TransitMcp { inputSchema: { tripId: z.number().int().positive(), dayId: z.number().int().positive().describe('Trip day on which the journey departs'), - from: transitPlaceSchema, - to: transitPlaceSchema, + from: transitPlaceInput, + to: transitPlaceInput, itinerary: transitItinerarySchema, notes: z.string().max(1000).optional(), }, 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