mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
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 authored inline in the tool shapes are strict for the same reason. A whole Zod schema, the way a plugin's manifest-built tool arrives, passes through as its author wrote it.
This commit is contained in:
@@ -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)`.
|
||||
|
||||
@@ -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<string, unknown>, cb: (...cbArgs: unknown[]) => unknown): unknown;
|
||||
@@ -294,7 +306,7 @@ export class McpRegistry {
|
||||
const config: Record<string, unknown> = {
|
||||
title: options.title,
|
||||
description: options.description,
|
||||
inputSchema: options.inputSchema,
|
||||
inputSchema: strictInputSchema(options.inputSchema),
|
||||
outputSchema: options.outputSchema,
|
||||
annotations: options.annotations,
|
||||
_meta: options._meta,
|
||||
|
||||
@@ -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.
|
||||
*/
|
||||
|
||||
@@ -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.');
|
||||
|
||||
@@ -63,7 +63,7 @@ type EntryVisibility = z.infer<typeof ENTRY_VISIBILITY>;
|
||||
* 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([]),
|
||||
});
|
||||
|
||||
@@ -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'),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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"'),
|
||||
|
||||
@@ -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'),
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, unknown>, 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<string, never>, 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: {} });
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user