diff --git a/.changeset/mcp-tool-annotations-declared-source.md b/.changeset/mcp-tool-annotations-declared-source.md new file mode 100644 index 0000000000..b7b881fba5 --- /dev/null +++ b/.changeset/mcp-tool-annotations-declared-source.md @@ -0,0 +1,16 @@ +--- +"@objectstack/mcp": patch +--- + +Bridged MCP tools are now annotated from what their definition DECLARES, not from a seven-name allowlist. `registerToolFromDefinition` built both safety hints as membership tests against two literal sets (`READ_ONLY_TOOLS`, 6 names; `DESTRUCTIVE_TOOLS`, 1), so every tool outside them — every tool an app registers under its own name, and every action-backed tool (`delete_opportunity`, `void_invoice`, `archive_account`, …) — reached each MCP client as `readOnlyHint: false, destructiveHint: false`. That pair is not a missing annotation: it is a positive claim of "not read-only, and not destructive", on the one field an MCP host reads to decide whether to interrupt the user before a call, so a destructive action-backed tool arrived flagged as safe. It also inverted the protocol's own conservative default (`@modelcontextprotocol/sdk` 1.30.0 documents `destructiveHint` as `Default: true`). + +The declared source is `AIToolDefinition.requiresConfirmation` — the runtime contract member that already carries the framework's one maintainer-ruled definition of destructive (`actionLooksDestructive`, #7828 Option A, whose output `summarizeAction` writes into that very field). ⛔ Nothing here restores the retired metadata key `ToolSchema.requiresConfirmation`, which ADR-0033 §2 removed and which still hard-rejects; it is a different member, on a different object, at a different layer, and no metadata author can reach the one read here. + +What a `tools/list` now serves, per tool: + +- **declares `requiresConfirmation: true`** → `destructiveHint: true, readOnlyHint: false` (was `destructiveHint: false`). ⚠️ Hosts will start prompting before these calls, which is the point of the change and the intended direction. +- **declares `requiresConfirmation: false`** → `destructiveHint: false`, no `readOnlyHint` (was an asserted `readOnlyHint: false`; the MCP default is `false`, so nothing changes for a conforming host). +- **declares nothing** → NEITHER hint (was `false, false`). MCP has no spelling for "unknown" other than absence, so the protocol's own defaults apply — `readOnlyHint` false, `destructiveHint` **true** — instead of a value this bridge cannot source. +- **a platform tool name** (`list_objects`, `describe_object`, `query_records`, `get_record`, `aggregate_data`, `delete_field`) → unchanged, as an explicit last-resort fallback for the names the platform itself registers, now outranked by anything the definition declares and pinned to be a subset of `PLATFORM_PROVIDED_TOOL_NAMES`. + +One name left the read-only fallback: `aggregate_records` is not a platform tool name (`aggregate_data` is) — it belongs to the object-CRUD bridge, which registers it, annotated `readOnlyHint: true`, at its own site in `mcp-http-tools.ts`, so nothing loses that annotation where it is actually served. `openWorldHint: false` is unchanged for every bridged tool. diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 0c6b4d2ca7..fe7722fd5e 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -54,25 +54,136 @@ interface ObjectDef { } /** - * Names of tools that are read-only (no side effects). - * Kept as a module-level constant for easy extension. + * PLATFORM tool names whose safety class is known from the platform's own + * registration rather than from anything the definition carries — the + * last-resort fallback inside {@link safetyAnnotations}, and deliberately NOT + * a general classifier. + * + * Every name here is a tool the cloud AI runtime registers statically + * (`PLATFORM_TOOLS_BY_PACKAGE` in `@objectstack/spec/system`) and hands to + * this bridge through the AI service's `ToolRegistry` carrying no + * `requiresConfirmation`. A sibling pin holds both sets to that registry, so + * the lists cannot drift back into folklore: a name the platform does not + * register is a name this bridge knows nothing about. + * + * ⛔ What the fallback must never do again is answer for tools it does NOT + * contain. These two sets used to be the ONLY source of both hints, so the + * `else` branch of the membership tests asserted + * `readOnlyHint: false, destructiveHint: false` — "not read-only and not + * destructive", the most permissive pair the annotation can express — for + * every app-registered and every action-backed tool, and inverted the + * protocol's own conservative default while doing it. + * + * `aggregate_records` left the read-only half because it was never a platform + * tool name (`aggregate_data` is): it belongs to the object-CRUD bridge, which + * registers it — annotated `readOnlyHint: true` — at its own registration site + * in `mcp-http-tools.ts`, and never reaches this path. */ -const READ_ONLY_TOOLS = new Set([ +const PLATFORM_READ_ONLY_TOOL_NAMES = new Set([ 'list_objects', 'describe_object', 'query_records', 'get_record', - 'aggregate_records', 'aggregate_data', ]); /** - * Names of tools that perform destructive mutations. + * The destructive half of the same platform-name fallback — see + * {@link PLATFORM_READ_ONLY_TOOL_NAMES} for what it is and is not for. */ -const DESTRUCTIVE_TOOLS = new Set([ +const PLATFORM_DESTRUCTIVE_TOOL_NAMES = new Set([ 'delete_field', ]); +/** The safety hints this bridge can source, as MCP spells them. */ +interface ToolSafetyHints { + readOnlyHint?: boolean; + destructiveHint?: boolean; +} + +/** + * The `readOnlyHint` / `destructiveHint` an {@link AIToolDefinition} can + * actually SOURCE — and nothing else. + * + * THE DEFECT. Both hints used to be membership tests against the two name sets + * above, so a bridged tool outside those seven literals was served to every + * MCP client as `readOnlyHint: false, destructiveHint: false`. That is not a + * missing annotation, it is a positive claim of "not read-only, and not + * destructive" — asserted over every tool an app registers under its own name + * and every action-backed tool (`delete_opportunity`, `void_invoice`, …). + * `destructiveHint` is what a host reads to decide whether to interrupt the + * user before a call, so a destructive action-backed tool arrived flagged as + * safe. + * + * THE DECLARED SOURCE is `AIToolDefinition.requiresConfirmation` + * (`@objectstack/spec/contracts`), documented on the member itself as carried + * by action-backed tools "from the action's confirmation policy + * (`action.ai.requiresConfirmation`, or the destructive-action default)". + * + * ⛔ NOT the retired metadata key. `ToolSchema.requiresConfirmation` was + * removed by ADR-0033 §2 and still hard-REJECTS with a prescription; nothing + * here asks for it back and no metadata author can reach the member read + * below. The two live on different objects at different layers: the retired + * one was authorable `tool` metadata, this one is the runtime contract the AI + * service registers, which the retirement never touched. + * + * ⛔ NOT a second definition of "destructive" either. The framework already + * has one, maintainer-ruled (`actionLooksDestructive` in + * `@objectstack/runtime`, #7828 Option A: `mode: 'delete'` / `variant: + * 'danger'` are the closed declared signals and `confirmText` deliberately is + * not), and `requiresConfirmation` is literally that function's output — + * `summarizeAction` fills the field by calling it. So the reuse this bridge + * owes the ruling is to READ the verdict it is handed, not to re-derive one: + * an MCP bridge never sees an action, and `@objectstack/mcp` does not depend + * on `@objectstack/runtime`. The same verdict already travels to MCP on the + * other path, as `requiresConfirmation` on each `list_actions` entry. + * + * WHY A TOOL THAT DECLARES NOTHING GETS NO HINT AT ALL. Measured in the + * pinned SDK (`@modelcontextprotocol/sdk` 1.30.0, `ToolAnnotationsSchema`): + * `readOnlyHint` documents `Default: false` and `destructiveHint` documents + * `Default: true`. Omitting a hint therefore hands the question to the + * protocol's own conservative default — "may perform destructive updates" — + * while claiming nothing this framework cannot source, and it is the same + * treatment the annotation vocabulary has no other word for: MCP has no + * spelling for "unknown" other than absence. Asserting `false` was the + * inversion; asserting `true` here would be a property presented as the + * tool's when it is really this bridge's ignorance. + * + * WHY `readOnlyHint` KEEPS A NAME FALLBACK AND NOTHING ELSE. There is no + * declared source for it at all: `AIToolDefinition` has no member expressing + * "this tool only reads". The asymmetry with `destructiveHint` is the MCP + * defaults' own asymmetry — a missing `readOnlyHint` reads as "not read-only", + * which is the conservative answer, so omission loses only information and + * never safety. The platform's own readers are the one place that information + * exists, so they keep it; every other tool is served no `readOnlyHint` + * rather than a fabricated `false`. + * + * PRECEDENCE: what the definition declares outranks what the name suggests. + */ +function safetyAnnotations(tool: AIToolDefinition): ToolSafetyHints { + if (tool.requiresConfirmation !== undefined) { + // A tool whose invocation is gated on human confirmation is by + // construction not a read (`readOnlyHint` is stated so the destructive + // hint is unambiguously meaningful — MCP reads it only when read-only is + // false). `false` is the action's declared "no confirmation needed", the + // one thing that legitimately sources a non-destructive claim. + return tool.requiresConfirmation + ? { readOnlyHint: false, destructiveHint: true } + : { destructiveHint: false }; + } + + if (PLATFORM_READ_ONLY_TOOL_NAMES.has(tool.name)) { + // Read-only entails non-destructive; both come from the one fact. + return { readOnlyHint: true, destructiveHint: false }; + } + + if (PLATFORM_DESTRUCTIVE_TOOL_NAMES.has(tool.name)) { + return { readOnlyHint: false, destructiveHint: true }; + } + + return {}; +} + // ── AIToolDefinition.parameters → MCP inputSchema ──────────────────────────── /** @@ -813,8 +924,9 @@ export class MCPServerRuntime { * Each registered tool becomes an MCP tool with the same name, description * and declared arguments: `AIToolDefinition.parameters` is JSON Schema, and * {@link toolInputSchema} converts it into the Zod schema the SDK requires - * for `inputSchema`. The handler delegates to the ToolRegistry's execute - * path. + * for `inputSchema`. Its safety annotations come from what the definition + * declares — see {@link safetyAnnotations}. The handler delegates to the + * ToolRegistry's execute path. */ bridgeTools(toolRegistry: ToolRegistry): void { const tools = toolRegistry.getAll(); @@ -876,6 +988,12 @@ export class MCPServerRuntime { * with no `arguments` anywhere on that `extra` (`RequestHandlerExtra` has no * such member), which is why a bridged tool used to execute with `{}` no * matter what the client sent. + * + * The safety annotations come from {@link safetyAnnotations}, which reads + * what the definition DECLARES and omits the hints it cannot source; the + * name-derived `readOnlyHint: false, destructiveHint: false` this call used + * to assert over every unlisted tool is gone. `openWorldHint` is untouched + * by that change and still asserted for every bridged tool. */ private registerToolFromDefinition(tool: AIToolDefinition, toolRegistry: ToolRegistry): void { const logger = this.config.logger; @@ -886,9 +1004,9 @@ export class MCPServerRuntime { description: tool.description, inputSchema: toolInputSchema(tool, logger), annotations: { - // Mark tools with write side-effects for destructive operations - destructiveHint: this.isDestructiveTool(tool.name), - readOnlyHint: this.isReadOnlyTool(tool.name), + // Only the hints {@link safetyAnnotations} can source — a tool that + // declares nothing is served neither, so the MCP defaults apply. + ...safetyAnnotations(tool), openWorldHint: false, }, }, @@ -925,20 +1043,6 @@ export class MCPServerRuntime { ); } - /** - * Check if a tool is read-only (data query tools). - */ - private isReadOnlyTool(name: string): boolean { - return READ_ONLY_TOOLS.has(name); - } - - /** - * Check if a tool performs destructive operations. - */ - private isDestructiveTool(name: string): boolean { - return DESTRUCTIVE_TOOLS.has(name); - } - // ── Resource Bridge ──────────────────────────────────────────── /** diff --git a/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts b/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts new file mode 100644 index 0000000000..f1bceca9f9 --- /dev/null +++ b/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts @@ -0,0 +1,277 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * What `bridgeTools` puts in a bridged tool's `annotations` — and what it must + * stop putting there. + * + * THE DEFECT. `registerToolFromDefinition` built both safety hints from the + * tool's NAME, as membership tests against two literal sets of seven names in + * `mcp-server-runtime.ts`. Every other bridged tool — every tool an app + * registers under its own name, every action-backed tool — reached each MCP + * client as `readOnlyHint: false, destructiveHint: false`: not a missing + * annotation but a positive claim of "not read-only, and not destructive", the + * most permissive pair the annotation can express, over a surface where + * `destructiveHint` is what a host reads to decide whether to interrupt the + * user before a call. The definition already carried the answer + * (`AIToolDefinition.requiresConfirmation`) and the bridge never read it. + * + * WHY THESE CASES DRIVE A REAL `StdioServerTransport`. What a client receives + * is only visible on the wire, and the pins that were green through the whole + * defect asserted the bridge's log line, which stays true of a bridge that + * annotates wrongly. Each case below speaks newline-delimited JSON-RPC down a + * real transport attached to the real long-lived server and reads + * `tools/list`, exactly as a desktop MCP host does — the shape + * `mcp-tool-bridge-input-schema.test.ts` established for the same call site. + * The client harness is duplicated from that file on purpose: a pin that + * exists to observe the wire should not be able to go green because a sibling + * pin's helper changed. + * + * WHAT THE CONTROLS ARE FOR. `a platform read-only name` and `delete_field` + * assert byte-identical annotations before and after the fix, so a red from + * the cases around them is a statement about the change rather than about the + * harness or the transport. + * + * ABSENCE IS THE ASSERTION in three cases. `toBeUndefined()` on a hint is not + * a weaker `toBe(false)`: MCP has no spelling for "unknown" other than + * omission, and the SDK's own `ToolAnnotationsSchema` documents the defaults + * that then apply (`readOnlyHint` false, `destructiveHint` **true**), which is + * the conservative reading the old `false` inverted. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { PassThrough } from 'node:stream'; +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; +import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import type { AIToolDefinition, ToolCallPart } from '@objectstack/spec/contracts'; +import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { ToolRegistry, ToolExecutionResult } from './types.js'; + +// --------------------------------------------------------------------------- +// A real stdio client: newline-delimited JSON-RPC over the transport's pipes +// --------------------------------------------------------------------------- + +interface JsonRpcFrame { + jsonrpc: string; + id?: number; + result?: any; + error?: { code: number; message: string }; +} + +interface StdioSession { + rpc(method: string, params?: unknown): Promise; + notify(method: string, params?: unknown): void; + close(): Promise; +} + +async function openStdio(server: McpServer): Promise { + const serverStdin = new PassThrough(); + const serverStdout = new PassThrough(); + const transport = new StdioServerTransport(serverStdin, serverStdout); + await server.connect(transport); + + let nextId = 1; + let buffered = ''; + const waiting = new Map void>(); + + serverStdout.on('data', (chunk: Buffer | string) => { + buffered += String(chunk); + let newline = buffered.indexOf('\n'); + while (newline >= 0) { + const line = buffered.slice(0, newline).trim(); + buffered = buffered.slice(newline + 1); + newline = buffered.indexOf('\n'); + if (!line) continue; + let frame: JsonRpcFrame; + try { + frame = JSON.parse(line) as JsonRpcFrame; + } catch { + continue; + } + const resolve = typeof frame.id === 'number' ? waiting.get(frame.id) : undefined; + if (resolve && typeof frame.id === 'number') { + waiting.delete(frame.id); + resolve(frame); + } + } + }); + + return { + rpc(method, params) { + const id = nextId++; + return new Promise((resolve, reject) => { + const giveUp = setTimeout( + () => reject(new Error(`stdio: no answer to "${method}" (id ${id}) within 5s`)), + 5_000, + ); + waiting.set(id, (frame) => { + clearTimeout(giveUp); + resolve(frame); + }); + serverStdin.write( + `${JSON.stringify({ jsonrpc: '2.0', id, method, ...(params ? { params } : {}) })}\n`, + ); + }); + }, + notify(method, params) { + serverStdin.write(`${JSON.stringify({ jsonrpc: '2.0', method, ...(params ? { params } : {}) })}\n`); + }, + async close() { + await transport.close().catch(() => {}); + }, + }; +} + +function makeRegistry(tools: AIToolDefinition[]): ToolRegistry { + return { + getAll: () => tools, + async execute(toolCall: ToolCallPart): Promise { + return { + type: 'tool-result', + toolCallId: toolCall.toolCallId, + toolName: toolCall.toolName, + output: { type: 'text', value: `executed ${toolCall.toolName}` }, + } as ToolExecutionResult; + }, + }; +} + +/** Bridge `tools`, then read what a client sees in `tools/list`. */ +async function annotationsOf( + tools: AIToolDefinition[], +): Promise<{ session: StdioSession; byName: Record }> { + const runtime = new MCPServerRuntime({ name: 'annotation-pin', version: '0.0.0-test' }); + runtime.bridgeTools(makeRegistry(tools)); + const session = await openStdio(runtime.server); + await session.rpc('initialize', { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'annotation-pin', version: '0.0.0' }, + }); + session.notify('notifications/initialized'); + + const listed = (await session.rpc('tools/list')).result?.tools ?? []; + return { session, byName: Object.fromEntries(listed.map((t: any) => [t.name, t])) }; +} + +const tool = (name: string, extra: Partial = {}): AIToolDefinition => ({ + name, + description: `the ${name} tool`, + parameters: { type: 'object', properties: {} }, + ...extra, +}); + +// --------------------------------------------------------------------------- + +describe('bridgeTools — the safety annotations a client receives', () => { + let openSession: StdioSession | undefined; + + afterEach(async () => { + await openSession?.close(); + openSession = undefined; + }); + + it('CONTROL: a platform read-only name keeps `readOnlyHint: true` (unchanged by this fix)', async () => { + const s = await annotationsOf([tool('query_records'), tool('list_objects')]); + openSession = s.session; + + expect(s.byName.query_records.annotations).toMatchObject({ + readOnlyHint: true, + destructiveHint: false, + openWorldHint: false, + }); + expect(s.byName.list_objects.annotations.readOnlyHint).toBe(true); + }); + + it('CONTROL: `delete_field` keeps `destructiveHint: true` (unchanged by this fix)', async () => { + const s = await annotationsOf([tool('delete_field')]); + openSession = s.session; + + expect(s.byName.delete_field.annotations).toMatchObject({ + readOnlyHint: false, + destructiveHint: true, + openWorldHint: false, + }); + }); + + it('a tool that declares `requiresConfirmation: true` is served `destructiveHint: true`', async () => { + const s = await annotationsOf([tool('delete_opportunity', { requiresConfirmation: true })]); + openSession = s.session; + + expect(s.byName.delete_opportunity.annotations.destructiveHint).toBe(true); + expect(s.byName.delete_opportunity.annotations.readOnlyHint).toBe(false); + }); + + it('a tool that declares `requiresConfirmation: false` is served `destructiveHint: false` and no read-only claim', async () => { + const s = await annotationsOf([tool('add_task_comment', { requiresConfirmation: false })]); + openSession = s.session; + + expect(s.byName.add_task_comment.annotations.destructiveHint).toBe(false); + expect(s.byName.add_task_comment.annotations.readOnlyHint).toBeUndefined(); + }); + + it('a tool that declares nothing is served NEITHER hint — not a fabricated `false`', async () => { + const s = await annotationsOf([tool('send_invoice_email')]); + openSession = s.session; + + const annotations = s.byName.send_invoice_email.annotations; + expect(annotations.destructiveHint).toBeUndefined(); + expect(annotations.readOnlyHint).toBeUndefined(); + // The hint the bridge does still assert for every tool, unchanged here. + expect(annotations.openWorldHint).toBe(false); + }); + + it('what the definition declares outranks what its name suggests', async () => { + const s = await annotationsOf([tool('query_records', { requiresConfirmation: true })]); + openSession = s.session; + + expect(s.byName.query_records.annotations.destructiveHint).toBe(true); + expect(s.byName.query_records.annotations.readOnlyHint).toBe(false); + }); + + it('`aggregate_records` gets no name-derived hint here — it is the object bridge\'s tool, not a platform tool name', async () => { + const s = await annotationsOf([tool('aggregate_records')]); + openSession = s.session; + + expect(PLATFORM_PROVIDED_TOOL_NAMES.has('aggregate_records')).toBe(false); + expect(s.byName.aggregate_records.annotations.readOnlyHint).toBeUndefined(); + expect(s.byName.aggregate_records.annotations.destructiveHint).toBeUndefined(); + }); + + /** + * The invariant that keeps the name fallback from drifting back into + * folklore, asserted from OUTSIDE the module (the two sets are private): + * only a name the platform itself registers may receive a hint it did not + * declare. Driving every platform name at once also proves the fallback is a + * SUBSET of that registry rather than merely overlapping it. + */ + it('no tool outside `PLATFORM_PROVIDED_TOOL_NAMES` receives a hint it did not declare', async () => { + const platform = [...PLATFORM_PROVIDED_TOOL_NAMES].map((name) => tool(name)); + const strangers = [ + 'aggregate_records', + 'delete_record', + 'void_invoice', + 'archive_account', + 'delete_opportunity', + 'send_invoice_email', + ].map((name) => tool(name)); + + const s = await annotationsOf([...platform, ...strangers]); + openSession = s.session; + + const annotated = Object.values(s.byName) + .filter((t: any) => t.annotations?.readOnlyHint !== undefined || t.annotations?.destructiveHint !== undefined) + .map((t: any) => t.name) + .sort(); + + expect(annotated.length).toBeGreaterThan(0); + for (const name of annotated) { + expect(PLATFORM_PROVIDED_TOOL_NAMES.has(name)).toBe(true); + } + for (const stranger of strangers) { + expect(s.byName[stranger.name].annotations.readOnlyHint).toBeUndefined(); + expect(s.byName[stranger.name].annotations.destructiveHint).toBeUndefined(); + } + }); +});