diff --git a/.changeset/30813-mcp-delete-record-confirmation.md b/.changeset/30813-mcp-delete-record-confirmation.md new file mode 100644 index 00000000000..fde82a15ab4 --- /dev/null +++ b/.changeset/30813-mcp-delete-record-confirmation.md @@ -0,0 +1,28 @@ +--- +'@objectstack/mcp': patch +'@objectstack/spec': patch +--- + +feat(mcp)!: the MCP `delete_record` tool refuses a call without `confirm: true`, with `run_action`'s confirmation refusal + +Clause-②: yes (narrowing) — the MCP `delete_record` tool refuses every call that does not carry `confirm: true`, where it deleted the record. The confirmation contract in `@objectstack/spec/contracts` (`AIActionConfirmation`, `ActionConfirmationRequiredDetails`) gains its second door in its docblock, and the error-code ledger lists `ACTION_CONFIRMATION_REQUIRED` under `@objectstack/mcp` as a second emitter; no shape changes. +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the MCP `delete_record` tool (`@objectstack/mcp`), over both transports (Streamable HTTP `/api/v1/mcp` and stdio): every MCP client that deletes is refused until it sends the confirmation member. + +- **Before.** `delete_record({ objectName, recordId })` deleted the record. A delete is hard and permanent, and the tool's only safety signal was `annotations.destructiveHint`, a hint a client may ignore. A call carrying `confirm` was refused as an unknown key. +- **Now.** A call without `confirm`, or with `confirm: false`, is refused before the data bridge is reached, and nothing is read or deleted. A non-boolean `confirm` (the string `"true"`, `1`) is refused before that by the tool's closed input schema, with no code and no `details`, exactly as on `run_action`. The answer to an absent or `false` member is the refusal `run_action` gives an action whose author declared `ai.requiresConfirmation: true`: an `isError` result whose text is `{ "error": { "code": "ACTION_CONFIRMATION_REQUIRED", "message", "status": 428, "details": { "actionName": "delete_record", "objectName", "confirmationMember": "confirm" } } }`. `delete_record({ objectName, recordId, confirm: true })` deletes as before, under the same exposure, credential and permission checks. The tool schema advertises `confirm` as an optional boolean, and the tool description tells the agent to ask the human first, then retry. +- **Who moves.** An MCP client or agent that calls `delete_record`. A client that already handles `run_action`'s `ACTION_CONFIRMATION_REQUIRED` (read `details.confirmationMember`, ask the human, resend the same call with that member set to `true`) needs no change. + +**The remedy.** Ask the human in the loop, then send the same `delete_record` call with `confirm: true` added beside `objectName` and `recordId`. The flag asserts an approval; it does not obtain one, and the gate makes forgetting loud without proving a human. + +**The bounds, as decided.** + +- REST `DELETE /api/v1/data/:object/:id` (the human path) is outside the gate and unchanged. +- `update_record` is not gated. +- No object is exempt: every object a caller may delete through `delete_record` takes the gate. +- Any future bulk-delete tool takes the same gate. + +The member, the code, the status and the `details` shape are the contract `run_action` already serves (`AI_ACTION_CONFIRMATION_MEMBER` and `ActionConfirmationRequiredDetails` in `@objectstack/spec/contracts`), so an agent learns one retry for both tools. The MCP server's own skill (`GET /api/v1/mcp/skill`) teaches the member on `delete_record`. diff --git a/content/docs/ai/connect-mcp.mdx b/content/docs/ai/connect-mcp.mdx index fd329a20729..01a7a361e12 100644 --- a/content/docs/ai/connect-mcp.mdx +++ b/content/docs/ai/connect-mcp.mdx @@ -375,6 +375,13 @@ drive this MCP server. Same word, different layer. an action as a code-review-worthy act — that's the real security boundary. - An action can also declare `ai.requiresConfirmation`; destructive-looking actions (`confirmText`, danger variants) default to requiring it. +- **`delete_record` needs `confirm: true` on every call.** A record delete over + MCP is permanent, so the tool refuses a call without it with `run_action`'s + refusal (`428` `ACTION_CONFIRMATION_REQUIRED`, its `details.actionName` + naming `delete_record`) and deletes nothing. The agent asks its human, then retries + with `confirm: true`; the flag asserts an approval, it does not obtain one. + REST `DELETE /api/v1/data/:object/:id` and `update_record` take no + confirmation. ## Verify the connection diff --git a/packages/mcp/README.md b/packages/mcp/README.md index 9f5448148d0..b709783bc4b 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -117,7 +117,7 @@ apply): 'get_record' // Fetch one by id 'create_record' // Create 'update_record' // Update by id -'delete_record' // Delete by id (destructive) +'delete_record' // Delete by id (destructive; refused without confirm: true) // Business actions — operate the app, not just its rows 'list_actions' // Invokable business actions the caller may run diff --git a/packages/mcp/src/mcp-coded-refusal.test.ts b/packages/mcp/src/mcp-coded-refusal.test.ts index b44827928bb..8890ffc8742 100644 --- a/packages/mcp/src/mcp-coded-refusal.test.ts +++ b/packages/mcp/src/mcp-coded-refusal.test.ts @@ -100,7 +100,8 @@ const SITES: readonly Site[] = [ { tool: 'get_record', args: { objectName: 'task', recordId: 't9' }, member: 'get' }, { tool: 'create_record', args: { objectName: 'task', data: { title: 'x' } }, member: 'create' }, { tool: 'update_record', args: { objectName: 'task', recordId: 't9', data: { title: 'x' } }, member: 'update' }, - { tool: 'delete_record', args: { objectName: 'task', recordId: 't9' }, member: 'remove' }, + // `confirm: true` passes the confirmation gate, so the call reaches `remove`. + { tool: 'delete_record', args: { objectName: 'task', recordId: 't9', confirm: true }, member: 'remove' }, { tool: 'list_actions', args: {}, member: 'listActions' }, { tool: 'run_action', args: { actionName: 'complete' }, member: 'runAction', actionDoor: true }, { tool: 'resume_run', args: { runId: 'r1' }, member: 'resumeRun', actionDoor: true }, diff --git a/packages/mcp/src/mcp-delete-record-confirmation.test.ts b/packages/mcp/src/mcp-delete-record-confirmation.test.ts new file mode 100644 index 00000000000..8001819ba8f --- /dev/null +++ b/packages/mcp/src/mcp-delete-record-confirmation.test.ts @@ -0,0 +1,289 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The MCP `delete_record` tool takes `run_action`'s confirmation gate: a call + * without `confirm: true` is REFUSED with `ACTION_CONFIRMATION_REQUIRED` and + * nothing is deleted. + * + * ## Why + * + * A delete over MCP is hard and permanent, and before this gate one call did + * it: `annotations.destructiveHint` is a hint a client may ignore, and the + * bridge's own checks (API exposure, credentials, the caller's permissions) + * say whether the caller MAY delete, never whether a human asked for it. An + * instruction planted in a record's text could lead an agent to delete rows + * its user may delete with no human asked. The maintainer chose option B: the + * server-side backstop `run_action` already has, widened to this one tool, + * with the same member, code, status and `details`. + * + * ## What is pinned, and where + * + * - The door, over the HTTP `/mcp` transport, against the REAL stdio data + * bridge on an engine double that holds the row: the member is advertised, + * an unconfirmed call (absent, `false`, the string `'true'`) is refused and + * the row is still there, a confirmed call deletes it. + * - The refusal's code, status and `details` keys are `run_action`'s, served + * through the same door from a bridge throwing what the runtime gate + * throws. The real runtime gate beside the real `delete_record` refusal, and + * REST `DELETE /data/:object/:id` (outside the gate), are driven together in + * `packages/qa/dogfood/test/mcp-delete-record-confirmation.dogfood.test.ts`. + * - The long-lived server the stdio transport serves gates too: both + * transports register their tools through `wireBridgeTools`. + * - Controls: `update_record` is not gated, and the bridge is never reached + * by a refused call. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { AI_ACTION_CONFIRMATION_MEMBER } from '@objectstack/spec/contracts'; +import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { + assertEngineDeleteDispatch, + assertEngineUpdateDispatch, + assertEngineFindOnePredicate, + type EngineFindOneQueryInput, +} from '@objectstack/metadata-core'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import { createStdioDataBridge } from './stdio-data-bridge.js'; +import type { McpActionBridge, McpDataBridge } from './mcp-http-tools.js'; + +const OBJECT = 'dlc_note'; + +const NOTE = { + name: OBJECT, + label: 'Note', + fields: { + id: { name: 'id', type: 'text' }, + title: { name: 'title', type: 'text' }, + }, + enable: {}, +}; + +/** A store of `dlc_note` rows behind an engine that applies every write it is handed. */ +function engineDouble(rows: Map>) { + return { + find: vi.fn(async (_o: string, query?: any) => { + const id = query?.where?.id; + const all = [...rows.values()].filter((r) => id === undefined || r.id === id); + return typeof query?.limit === 'number' ? all.slice(0, query.limit) : all; + }), + findOne: vi.fn(async (object: string, query?: EngineFindOneQueryInput) => { + assertEngineFindOnePredicate(object, query); + return rows.get(String((query as any)?.where?.id)) ?? null; + }), + insert: vi.fn(async () => { + throw new Error('not used'); + }), + update: vi.fn(async (_o: string, data: Record, options?: any) => { + assertEngineUpdateDispatch(data, options); + const id = String(options?.where?.id); + rows.set(id, { ...rows.get(id), ...data }); + return rows.get(id); + }), + delete: vi.fn(async (_o: string, options?: any) => { + assertEngineDeleteDispatch(options); + rows.delete(String(options?.where?.id)); + return { deleted: 1 }; + }), + count: vi.fn(async () => rows.size), + }; +} + +function metadataDouble(): IMetadataService { + return { + listObjects: vi.fn(async () => [NOTE]), + getObject: vi.fn(async () => NOTE), + } as unknown as IMetadataService; +} + +function stdioBridge(engine: ReturnType): McpDataBridge { + return createStdioDataBridge({ + engine: engine as unknown as IDataEngine, + metadataService: metadataDouble(), + resolvePrincipal: async () => ({ userId: 'u1' }) as unknown as ExecutionContext, + }); +} + +/** What the runtime's `run_action` gate throws for an author-gated action. */ +function runtimeGateRefusal(): Error { + return Object.assign( + new Error( + "Action 'archive_note' on 'dlc_note' declares ai.requiresConfirmation: true — nothing was run. " + + "Confirm with the human in the loop, then retry this call with 'confirm': true on the request.", + ), + { + code: 'ACTION_CONFIRMATION_REQUIRED', + status: 428, + details: { actionName: 'archive_note', objectName: OBJECT, confirmationMember: AI_ACTION_CONFIRMATION_MEMBER }, + }, + ); +} + +let nextId = 1; +async function rpc(bridge: McpDataBridge, method: string, params?: Record) { + const body = { jsonrpc: '2.0', id: nextId++, method, ...(params ? { params } : {}) }; + const runtime = new MCPServerRuntime({ name: 'pin', version: '0.0.0' }); + const res = await runtime.handleHttpRequest( + new Request('http://localhost/api/v1/mcp', { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: JSON.stringify(body), + }), + { bridge: bridge as any, parsedBody: body }, + ); + expect(res.status).toBe(200); + const json = (await res.json()) as { result?: any; error?: unknown }; + expect(json.error, JSON.stringify(json.error)).toBeUndefined(); + return json.result; +} + +async function callTool(bridge: McpDataBridge, name: string, args: Record) { + return (await rpc(bridge, 'tools/call', { name, arguments: args })) as { + content: Array<{ text: string }>; + isError?: boolean; + }; +} + +const envelopeOf = (result: { content: Array<{ text: string }> }) => JSON.parse(result.content[0]!.text) as { error: any }; + +describe('MCP delete_record refuses a call without confirm: true (ACTION_CONFIRMATION_REQUIRED)', () => { + let rows: Map>; + let engine: ReturnType; + let bridge: McpDataBridge; + + beforeEach(() => { + rows = new Map([['n1', { id: 'n1', title: 'keep me' }]]); + engine = engineDouble(rows); + bridge = stdioBridge(engine); + }); + + it('advertises the confirmation member on the tool schema, so a model can discover the retry', async () => { + const listed = await rpc(bridge, 'tools/list'); + const tool = (listed.tools as any[]).find((t) => t.name === 'delete_record'); + expect(tool, 'delete_record is registered').toBeDefined(); + const props = tool.inputSchema.properties; + // Control: the members that were already there are still advertised. + expect(Object.keys(props)).toEqual(expect.arrayContaining(['objectName', 'recordId'])); + expect(props[AI_ACTION_CONFIRMATION_MEMBER]?.type).toBe('boolean'); + // Optional in the schema: a call without it must reach the handler, which + // answers the coded refusal. Required there, it would be refused as a bare + // input-validation error naming no code and no retry. + expect(tool.inputSchema.required ?? []).not.toContain(AI_ACTION_CONFIRMATION_MEMBER); + // The description tells the agent the order: ask the human, then retry. + expect(tool.description).toContain('confirm: true'); + expect(tool.description).toMatch(/ask the human/i); + expect(tool.annotations?.destructiveHint).toBe(true); + }); + + it('refuses a call without the member: code, status and the retry details; the record is intact', async () => { + const result = await callTool(bridge, 'delete_record', { objectName: OBJECT, recordId: 'n1' }); + + expect(result.isError).toBe(true); + const { error } = envelopeOf(result); + expect(error.code).toBe('ACTION_CONFIRMATION_REQUIRED'); + expect(error.status).toBe(428); + expect(error.details).toEqual({ + actionName: 'delete_record', + objectName: OBJECT, + confirmationMember: AI_ACTION_CONFIRMATION_MEMBER, + }); + expect(String(error.message)).toContain('nothing was deleted'); + + // Nothing was deleted: the engine was never asked, and the row is there. + expect(engine.delete).not.toHaveBeenCalled(); + expect(rows.get('n1')).toEqual({ id: 'n1', title: 'keep me' }); + }); + + it.each([ + ['false', false], + ['the string "true"', 'true'], + ['1', 1], + ])('refuses %s as the member: only the boolean true attests, and the record is intact', async (_label, value) => { + const result = await callTool(bridge, 'delete_record', { + objectName: OBJECT, + recordId: 'n1', + [AI_ACTION_CONFIRMATION_MEMBER]: value, + }); + + expect(result.isError).toBe(true); + expect(engine.delete).not.toHaveBeenCalled(); + expect(rows.has('n1')).toBe(true); + }); + + it('deletes on the retry with confirm: true, once', async () => { + const result = await callTool(bridge, 'delete_record', { + objectName: OBJECT, + recordId: 'n1', + [AI_ACTION_CONFIRMATION_MEMBER]: true, + }); + + expect(result.isError, result.content[0]?.text).toBeFalsy(); + expect(JSON.parse(result.content[0]!.text)).toEqual({ object: OBJECT, id: 'n1', success: true }); + expect(engine.delete).toHaveBeenCalledTimes(1); + expect(rows.has('n1')).toBe(false); + }); + + it('answers with run_action\'s refusal shape: the same code, status, envelope keys and details keys', async () => { + const actionBridge: McpDataBridge & McpActionBridge = { + ...bridge, + async listActions() { + return [{ name: 'archive_note', objectName: OBJECT, requiresRecord: true, requiresConfirmation: true }]; + }, + async runAction() { + throw runtimeGateRefusal(); + }, + }; + + const viaAction = envelopeOf(await callTool(actionBridge, 'run_action', { actionName: 'archive_note', objectName: OBJECT, recordId: 'n1' })).error; + const viaDelete = envelopeOf(await callTool(actionBridge, 'delete_record', { objectName: OBJECT, recordId: 'n1' })).error; + + // Lit control: the run_action half really is the gate's refusal. + expect(viaAction.code).toBe('ACTION_CONFIRMATION_REQUIRED'); + + expect(viaDelete.code).toBe(viaAction.code); + expect(viaDelete.status).toBe(viaAction.status); + expect(Object.keys(viaDelete)).toEqual(Object.keys(viaAction)); + expect(Object.keys(viaDelete.details).sort()).toEqual(Object.keys(viaAction.details).sort()); + expect(viaDelete.details.confirmationMember).toBe(viaAction.details.confirmationMember); + expect(viaDelete.details.objectName).toBe(viaAction.details.objectName); + expect(rows.has('n1')).toBe(true); + }); + + it('control: update_record is not gated, and writes without the member', async () => { + const result = await callTool(bridge, 'update_record', { objectName: OBJECT, recordId: 'n1', data: { title: 'changed' } }); + + expect(result.isError, result.content[0]?.text).toBeFalsy(); + expect(engine.update).toHaveBeenCalledTimes(1); + expect(rows.get('n1')).toMatchObject({ title: 'changed' }); + }); + + it('the long-lived server the stdio transport serves gates the same way', async () => { + const runtime = new MCPServerRuntime({ name: 'pin-stdio', version: '0.0.0' }); + runtime.bridgeDataTools(bridge); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await runtime.server.connect(serverTransport); + const client = new Client({ name: 'pin-client', version: '0.0.0' }); + await client.connect(clientTransport); + try { + const refused: any = await client.callTool({ name: 'delete_record', arguments: { objectName: OBJECT, recordId: 'n1' } }); + expect(refused.isError).toBe(true); + const { error } = JSON.parse(refused.content[0].text); + expect(error.code).toBe('ACTION_CONFIRMATION_REQUIRED'); + expect(error.status).toBe(428); + expect(error.details).toEqual({ actionName: 'delete_record', objectName: OBJECT, confirmationMember: AI_ACTION_CONFIRMATION_MEMBER }); + expect(rows.has('n1')).toBe(true); + + const confirmed: any = await client.callTool({ + name: 'delete_record', + arguments: { objectName: OBJECT, recordId: 'n1', [AI_ACTION_CONFIRMATION_MEMBER]: true }, + }); + expect(confirmed.isError, confirmed.content?.[0]?.text).toBeFalsy(); + expect(rows.has('n1')).toBe(false); + } finally { + await client.close(); + } + }); +}); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index 76899f768dc..755a5bc9018 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -46,7 +46,7 @@ import type { EngineAggregateOptions } from '@objectstack/spec/data'; import { AggregationNodeSchema } from '@objectstack/spec/data'; // [#23529] The catalogue's spelling of `PERMISSION_DENIED`, the code whose // `details` `errorResultFromThrown` withholds. Imported, never hand-spelled. -import { StandardErrorCode } from '@objectstack/spec/api'; +import { StandardErrorCode, type ErrorCode } from '@objectstack/spec/api'; // [#23701] The one key every door that translates the engine's unique-violation // envelope asks (the registered code AND the class name, never message text), // and the one classification whose arm states the wire code for it. @@ -64,6 +64,7 @@ import { import { AI_ACTION_CONFIRMATION_MEMBER, type AIActionConfirmation, + type ActionConfirmationRequiredDetails, type ImportObjectResult, type RemoteTable, } from '@objectstack/spec/contracts'; @@ -515,6 +516,67 @@ function codedErrorResult(error: ThrownRefusalEnvelope & { details?: unknown }) }; } +/** + * The code and status the confirmation gate answers with, on `run_action`'s + * door (the runtime's `actionConfirmationRefusal`) and on `delete_record`'s + * (below). Typed as the registered vocabulary, so a misspelling fails to + * compile instead of reaching a client as a code nobody branches on. + */ +const ACTION_CONFIRMATION_REQUIRED: ErrorCode = 'ACTION_CONFIRMATION_REQUIRED'; +const ACTION_CONFIRMATION_REQUIRED_HTTP_STATUS = 428; + +/** The `details.actionName` a `delete_record` refusal names: the tool the refused call addressed. */ +const DELETE_RECORD_TOOL = 'delete_record'; + +/** + * The confirmation gate on the MCP `delete_record` tool: `run_action`'s gate, + * widened by the maintainer's decision to this one tool, for every object. + * + * A delete over MCP is permanent, and the bridge's checks say whether the + * caller MAY delete, never whether a human asked for it. So a call that does + * not carry the confirmation member as the boolean `true` is refused with the + * refusal `run_action` answers an author-gated action with: the same code, + * status, envelope keys and `details` shape (`ActionConfirmationRequiredDetails`), + * rendered by the same {@link codedErrorResult}. A client that already handles + * that refusal confirms with its human and retries this call unchanged but for + * the member. + * + * The runtime's builder is out of reach (`@objectstack/runtime` and this + * package do not depend on each other), so the contract types and the shared + * renderer are what hold the two refusals to one shape; the dogfood suite + * serves both side by side to one caller. + * + * What it is NOT, as on `run_action`: `confirm: true` is an unverifiable + * caller claim. The gate makes forgetting loud; it does not prove a human. + * + * Scope, as decided: this tool only. REST `DELETE /api/v1/data/...` (the human + * path) and `update_record` stay outside it, no object is exempt, and a future + * bulk-delete tool takes the same gate. + * + * Returns `undefined` when the call may proceed. + */ +function recordDeleteConfirmationRefusal( + objectName: string, + recordId: string, + request: AIActionConfirmation, +): (ThrownRefusalEnvelope & { details: ActionConfirmationRequiredDetails }) | undefined { + // Only the boolean `true` attests; absent, `false` and a truthy string do not. + if (request[AI_ACTION_CONFIRMATION_MEMBER] === true) return undefined; + return { + code: ACTION_CONFIRMATION_REQUIRED, + message: + `delete_record on '${objectName}' record '${recordId}' carries no confirmation — nothing was deleted. ` + + 'A delete over MCP is permanent: confirm with the human in the loop, then retry this call with ' + + `'${AI_ACTION_CONFIRMATION_MEMBER}': true on the request.`, + status: ACTION_CONFIRMATION_REQUIRED_HTTP_STATUS, + details: { + actionName: DELETE_RECORD_TOOL, + objectName, + confirmationMember: AI_ACTION_CONFIRMATION_MEMBER, + }, + }; +} + /** What {@link thrownRefusalEnvelope} reads off a thrown coded refusal. */ export interface ThrownRefusalEnvelope { code: string; @@ -1515,19 +1577,38 @@ export function registerObjectTools( server.registerTool( note('delete_record'), { - description: 'Delete a record by id. This is destructive.', + description: + 'Delete a record by id. This is destructive and permanent. A call is REFUSED unless it also sends ' + + 'confirm: true — ask the human first, then retry; nothing is deleted on a refused call.', inputSchema: strictToolInput( { surface: 'this delete_record call', aliases: { ...OBJECT_NAME_ALIASES, ...RECORD_ID_ALIASES } }, { objectName: z.string().describe('The object/table name'), recordId: z.string().describe('The record id'), + // The confirmation member, keyed off the contract's constant as + // `run_action` declares it. OPTIONAL in the schema on purpose: a + // required member would refuse an unconfirmed call as a bare input + // validation error, with no code and no retry details, before the + // handler could answer the gate's refusal. + [AI_ACTION_CONFIRMATION_MEMBER]: z + .boolean() + .optional() + .describe( + 'Set to true only when the human in the loop has approved deleting THIS record. Without it ' + + 'the call is refused and nothing is deleted.', + ), }, ), annotations: { readOnlyHint: false, destructiveHint: true, openWorldHint: false }, }, - async ({ objectName, recordId }) => { + async (args) => { + const { objectName, recordId } = args; const bad = guard(objectName); if (bad) return errorResult(bad); + // The gate, before the bridge: a refused call reaches no bridge, so + // nothing is read or deleted whichever bridge serves this server. + const unconfirmed = recordDeleteConfirmationRefusal(objectName, recordId, args); + if (unconfirmed) return codedErrorResult(unconfirmed); try { return textResult(await bridge.remove(objectName, recordId)); } catch (err) { diff --git a/packages/mcp/src/mcp-http-tools.unknown-argument-keys.test.ts b/packages/mcp/src/mcp-http-tools.unknown-argument-keys.test.ts index 8431c7bca98..4f95ca4b219 100644 --- a/packages/mcp/src/mcp-http-tools.unknown-argument-keys.test.ts +++ b/packages/mcp/src/mcp-http-tools.unknown-argument-keys.test.ts @@ -106,7 +106,8 @@ const VALID_ARGS: Record> = { get_record: { objectName: 'crm_opportunity', recordId: 'r1' }, create_record: { objectName: 'crm_opportunity', data: { name: 'x' } }, update_record: { objectName: 'crm_opportunity', recordId: 'r1', data: { name: 'x' } }, - delete_record: { objectName: 'crm_opportunity', recordId: 'r1' }, + // `confirm: true`: without it the confirmation gate refuses before the bridge. + delete_record: { objectName: 'crm_opportunity', recordId: 'r1', confirm: true }, list_actions: {}, run_action: { actionName: 'complete_task' }, resume_run: { runId: 'run_1' }, diff --git a/packages/mcp/src/mcp-server-runtime.http.test.ts b/packages/mcp/src/mcp-server-runtime.http.test.ts index 32c159d8535..a136024ab97 100644 --- a/packages/mcp/src/mcp-server-runtime.http.test.ts +++ b/packages/mcp/src/mcp-server-runtime.http.test.ts @@ -190,7 +190,7 @@ describe('MCPServerRuntime.handleHttpRequest (Streamable HTTP)', () => { jsonrpc: '2.0', id: 6, method: 'tools/call', - params: { name: 'delete_record', arguments: { objectName: 'task', recordId: '1' } }, + params: { name: 'delete_record', arguments: { objectName: 'task', recordId: '1', confirm: true } }, }, bridge, ); diff --git a/packages/mcp/src/mcp-unique-violation-wire-code.test.ts b/packages/mcp/src/mcp-unique-violation-wire-code.test.ts index 9865d16f0e1..4c0bd1a83b5 100644 --- a/packages/mcp/src/mcp-unique-violation-wire-code.test.ts +++ b/packages/mcp/src/mcp-unique-violation-wire-code.test.ts @@ -112,7 +112,7 @@ const UPDATE: Site = { args: { objectName: 'udoc', recordId: 'r2', data: { email: 'taken@example.com' } }, member: 'update', }; -const DELETE: Site = { tool: 'delete_record', args: { objectName: 'udoc', recordId: 'r2' }, member: 'remove' }; +const DELETE: Site = { tool: 'delete_record', args: { objectName: 'udoc', recordId: 'r2', confirm: true }, member: 'remove' }; const RUN_ACTION: Site = { tool: 'run_action', args: { actionName: 'add_duplicate', objectName: 'udoc' }, member: 'runAction' }; const RESUME_RUN: Site = { tool: 'resume_run', args: { runId: 'run_1', values: { email: 'taken@example.com' } }, member: 'resumeRun' }; diff --git a/packages/mcp/src/mcp-update-delete-record-stale-summary.test.ts b/packages/mcp/src/mcp-update-delete-record-stale-summary.test.ts index 55275d1b811..19c96b4ea24 100644 --- a/packages/mcp/src/mcp-update-delete-record-stale-summary.test.ts +++ b/packages/mcp/src/mcp-update-delete-record-stale-summary.test.ts @@ -167,7 +167,7 @@ describe('[#28844] MCP update_record / delete_record answer a write applied unde it('delete_record, stdio bridge: the success result, the delete applied once', async () => { const rows = new Map([['c2', { id: 'c2', name: 'c2', amount: 135 }]]); const writes: string[] = []; - const result = await callTool(stdioBridge(rows, writes), 'delete_record', { objectName: 'qsf_child', recordId: 'c2' }); + const result = await callTool(stdioBridge(rows, writes), 'delete_record', { objectName: 'qsf_child', recordId: 'c2', confirm: true }); expect(result.isError, result.content[0]?.text).toBeFalsy(); expect(JSON.parse(result.content[0]!.text)).toEqual({ object: 'qsf_child', id: 'c2', success: true }); expect(writes).toEqual(['delete:c2']); @@ -183,7 +183,7 @@ describe('[#28844] MCP update_record / delete_record answer a write applied unde const updated = await callTool(bridge, 'update_record', { objectName: 'qsf_child', recordId: 'r1', data: { amount: 6 } }); expect(updated.isError, updated.content[0]?.text).toBeFalsy(); expect(JSON.parse(updated.content[0]!.text)).toEqual(answers.update); - const removed = await callTool(bridge, 'delete_record', { objectName: 'qsf_child', recordId: 'r2' }); + const removed = await callTool(bridge, 'delete_record', { objectName: 'qsf_child', recordId: 'r2', confirm: true }); expect(removed.isError, removed.content[0]?.text).toBeFalsy(); expect(JSON.parse(removed.content[0]!.text)).toEqual(answers.remove); }); diff --git a/packages/mcp/src/skill-md.ts b/packages/mcp/src/skill-md.ts index bcefe9b5ec8..de39c4ee4c3 100644 --- a/packages/mcp/src/skill-md.ts +++ b/packages/mcp/src/skill-md.ts @@ -285,8 +285,10 @@ create/update payload. - **get_record({ objectName, recordId })** — fetch one record by id. - **create_record({ objectName, data })** — create a record. - **update_record({ objectName, recordId, data })** — change fields on a record. -- **delete_record({ objectName, recordId })** — delete a record (destructive — - confirm with the user first). +- **delete_record({ objectName, recordId, confirm })** — delete a record, + permanently. The server REFUSES a call without \`confirm: true\` + (\`ACTION_CONFIRMATION_REQUIRED\`) and nothing is deleted. Ask your human + first; the flag asserts an approval, it does not obtain one. - **list_actions()** — list the business actions available to you (only those the app author exposed to AI and you are permitted to run), with each action's declared parameters, whether it operates on a record, and whether diff --git a/packages/qa/dogfood/test/mcp-delete-record-confirmation.dogfood.test.ts b/packages/qa/dogfood/test/mcp-delete-record-confirmation.dogfood.test.ts new file mode 100644 index 00000000000..350e098226a --- /dev/null +++ b/packages/qa/dogfood/test/mcp-delete-record-confirmation.dogfood.test.ts @@ -0,0 +1,137 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The MCP `delete_record` tool refuses a call without `confirm: true` with +// `run_action`'s confirmation refusal, and REST `DELETE /data/:object/:id` +// stays outside that gate — proven together on a booted showcase, through the +// live `/api/v1/mcp` transport and the live REST data door, for one caller. +// +// ## Why this file and not only the package's own pins +// +// `@objectstack/mcp` and `@objectstack/runtime` do not depend on each other, +// so the door's pins (`packages/mcp/src/mcp-delete-record-confirmation.test.ts`) +// compare the delete refusal with a DOUBLE of the runtime gate. Here the +// runtime gate is the real one: `run_action` on an action the configuration +// declares `ai.requiresConfirmation: true` reaches `actionConfirmationRefusal` +// in `@objectstack/runtime`, and its served refusal is held against the +// `delete_record` refusal served by the same transport to the same caller. +// +// The derived configuration flags the showcase's own AI-exposed, object-less +// action (`showcase_portfolio_snapshot`) as gated; nothing else changes. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { type VerifyStack } from '@objectstack/verify'; +import { MCPServerPlugin } from '@objectstack/mcp'; +import { AI_ACTION_CONFIRMATION_MEMBER } from '@objectstack/spec/contracts'; +import { bootShowcase } from './showcase-boot.js'; + +const OBJECT = 'showcase_private_note'; +const GATED_ACTION = 'showcase_portfolio_snapshot'; +const idOf = (b: any) => b?.id ?? b?.record?.id ?? b?.data?.id ?? b?.recordId; + +describe('MCP delete_record takes run_action\'s confirmation gate; REST DELETE stays outside it', () => { + let stack: VerifyStack; + let admin: string; + + /** One `tools/call` over the live MCP transport; the tool's text content, parsed. */ + const tool = async (name: string, args: Record) => { + const res = await stack.api('/mcp', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + Authorization: `Bearer ${admin}`, + }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name, arguments: args } }), + }); + expect(res.status).toBe(200); + const rpc: any = await res.json(); + expect(rpc.error, JSON.stringify(rpc.error)).toBeUndefined(); + const text = String(rpc.result?.content?.[0]?.text ?? ''); + let payload: any; + try { payload = JSON.parse(text); } catch { payload = { message: text }; } + return { isError: rpc.result?.isError === true, payload, text }; + }; + + const createNote = async (title: string): Promise => { + const res = await stack.apiAs(admin, 'POST', `/data/${OBJECT}`, { title }); + expect(res.status, await res.clone().text()).toBeLessThan(300); + const id = idOf(await res.json()); + expect(id, 'created note id').toBeTruthy(); + return String(id); + }; + const readStatus = async (id: string) => (await stack.apiAs(admin, 'GET', `/data/${OBJECT}/${id}`)).status; + + beforeAll(async () => { + const declared = showcaseStack as unknown as { actions?: any[] }; + const actions = (declared.actions ?? []).map((a) => + a?.name === GATED_ACTION ? { ...a, ai: { ...(a.ai ?? {}), requiresConfirmation: true } } : a, + ); + expect(actions.some((a) => a?.name === GATED_ACTION && a.ai?.exposed === true), 'the fixture action moved').toBe(true); + stack = await bootShowcase( + { extraPlugins: [new MCPServerPlugin()] }, + { ...(showcaseStack as unknown as Record), actions }, + ); + admin = await stack.signIn(); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('refuses an unconfirmed delete_record with ACTION_CONFIRMATION_REQUIRED, and the record is intact', async () => { + const id = await createNote('mcp delete unconfirmed'); + + const refused = await tool('delete_record', { objectName: OBJECT, recordId: id }); + + expect(refused.isError, refused.text).toBe(true); + expect(refused.payload.error).toMatchObject({ + code: 'ACTION_CONFIRMATION_REQUIRED', + status: 428, + details: { actionName: 'delete_record', objectName: OBJECT, confirmationMember: AI_ACTION_CONFIRMATION_MEMBER }, + }); + expect(await readStatus(id), 'the refused delete removed the record').toBe(200); + }, 60_000); + + it('serves the same code, status, envelope keys and confirmation member as run_action\'s real gate', async () => { + const id = await createNote('mcp delete shape'); + + const viaAction = await tool('run_action', { actionName: GATED_ACTION }); + const viaDelete = await tool('delete_record', { objectName: OBJECT, recordId: id }); + + // Lit control: the runtime gate really refused the action. + expect(viaAction.isError, viaAction.text).toBe(true); + expect(viaAction.payload.error?.code).toBe('ACTION_CONFIRMATION_REQUIRED'); + expect(viaAction.payload.error?.details?.actionName).toBe(GATED_ACTION); + + const a = viaAction.payload.error; + const d = viaDelete.payload.error; + expect(d.code).toBe(a.code); + expect(d.status).toBe(a.status); + expect(Object.keys(d)).toEqual(Object.keys(a)); + expect(d.details.confirmationMember).toBe(a.details.confirmationMember); + // The action is object-less, so its details carry no `objectName`; every + // key it does carry, the delete refusal carries too. + expect(Object.keys(d.details)).toEqual(expect.arrayContaining(Object.keys(a.details))); + expect(await readStatus(id)).toBe(200); + }, 60_000); + + it('deletes on the retry with confirm: true', async () => { + const id = await createNote('mcp delete confirmed'); + + const deleted = await tool('delete_record', { objectName: OBJECT, recordId: id, [AI_ACTION_CONFIRMATION_MEMBER]: true }); + + expect(deleted.isError, deleted.text).toBe(false); + expect(deleted.payload).toMatchObject({ object: OBJECT, success: true }); + expect(await readStatus(id), 'the confirmed delete left the record').toBe(404); + }, 60_000); + + it('control: REST DELETE /data/:object/:id takes no confirmation and deletes', async () => { + const id = await createNote('rest delete'); + + const res = await stack.apiAs(admin, 'DELETE', `/data/${OBJECT}/${id}`); + + expect(res.status, await res.clone().text()).toBeLessThan(300); + expect(await readStatus(id)).toBe(404); + }, 60_000); +}); diff --git a/packages/qa/dogfood/test/mcp-stdio-package-object-discovery.test.ts b/packages/qa/dogfood/test/mcp-stdio-package-object-discovery.test.ts index f5a7c6ca1ab..32edda5b740 100644 --- a/packages/qa/dogfood/test/mcp-stdio-package-object-discovery.test.ts +++ b/packages/qa/dogfood/test/mcp-stdio-package-object-discovery.test.ts @@ -383,7 +383,8 @@ const DATA_TOOLS: ReadonlyArray Record ({ objectName: o, aggregations: [{ function: 'count', alias: 'n' }] })], ['create_record', (o) => ({ objectName: o, data: { name: 'written' } })], ['update_record', (o) => ({ objectName: o, recordId: 'r_any', data: { name: 'written' } })], - ['delete_record', (o) => ({ objectName: o, recordId: 'r_any' })], + // `confirm: true` passes the delete confirmation gate, so the call meets the gates behind it. + ['delete_record', (o) => ({ objectName: o, recordId: 'r_any', confirm: true })], ]; /** Every engine call a data tool could reach `object`'s store through. */ diff --git a/packages/qa/dogfood/test/mcp-stdio-runtime-object-exposure.test.ts b/packages/qa/dogfood/test/mcp-stdio-runtime-object-exposure.test.ts index bc580224cc6..b560cc8288d 100644 --- a/packages/qa/dogfood/test/mcp-stdio-runtime-object-exposure.test.ts +++ b/packages/qa/dogfood/test/mcp-stdio-runtime-object-exposure.test.ts @@ -366,7 +366,8 @@ describe.each(SHAPES)('the MCP stdio door follows a runtime-authored object\'s c expect(refusal(await k.call('get_record', { objectName: GATED, recordId: 'any' }))).toEqual(refused); expect(refusal(await k.call('create_record', { objectName: GATED, data: { name: 'leak-probe' } }))).toEqual(refused); expect(refusal(await k.call('update_record', { objectName: GATED, recordId: 'any', data: { name: 'x' } }))).toEqual(refused); - expect(refusal(await k.call('delete_record', { objectName: GATED, recordId: 'any' }))).toEqual(refused); + // `confirm: true` passes the delete confirmation gate, so the exposure gate behind it answers. + expect(refusal(await k.call('delete_record', { objectName: GATED, recordId: 'any', confirm: true }))).toEqual(refused); // Refused before dispatch: the table holds exactly what it held. expect(stores.get(GATED)?.size).toBe(2); diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index 21e349e0da3..b09882d2e40 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -369,6 +369,10 @@ export const ERROR_CODE_LEDGER = { // not a residue. Registered HERE and not under a producing service for the // same reason as its `FLOW_*` neighbours: the action door, not the // predicate, is where the wire vocabulary is named. + // Second emitter: the MCP `delete_record` tool's gate + // (`recordDeleteConfirmationRefusal`, `packages/mcp/src/mcp-http-tools.ts`) + // answers this code, 428, with the same `details` shape for every record + // delete without the member; listed under `@objectstack/mcp` too. 'ACTION_CONFIRMATION_REQUIRED', // [ADR-0126 §8 item 2] a packaged ACTION this installation switched off, // refused at dispatch — the activation-ledger consult in @@ -1440,6 +1444,14 @@ export const ERROR_CODE_LEDGER = { // for the data door. 'OBJECT_API_DISABLED', 'OBJECT_API_METHOD_NOT_ALLOWED', + // The `delete_record` tool's confirmation gate (`mcp-http-tools.ts`, + // `recordDeleteConfirmationRefusal`): a record delete without the + // confirmation member as the boolean `true` is refused before any bridge + // is reached, 428, `details` shaped as `ActionConfirmationRequiredDetails` + // with `actionName: 'delete_record'`. Rendered by the tools' own coded + // result on both transports. Second EMITTER of the code `@objectstack/runtime` + // registers for `run_action`'s gate: provenance, not identity. + 'ACTION_CONFIRMATION_REQUIRED', ], '@objectstack/service-datasource': [ 'DATASOURCE_ADMIN_ERROR', // lifecycle/validation refusal from the datasource-admin service diff --git a/packages/spec/src/contracts/action-confirmation-contract.pin.test.ts b/packages/spec/src/contracts/action-confirmation-contract.pin.test.ts index 8cad375f1e9..8af09e5d5fb 100644 --- a/packages/spec/src/contracts/action-confirmation-contract.pin.test.ts +++ b/packages/spec/src/contracts/action-confirmation-contract.pin.test.ts @@ -14,7 +14,8 @@ * ## What is pinned, and what is deliberately NOT * * Pinned: that the refusal code is admitted by the closed `ErrorCode` union, - * that the ledger registers it under the package that owns the action doors, + * that the ledger registers it under the package that owns the action doors + * and under exactly one second emitter, the MCP `delete_record` tool's gate, * that it is not a synonym of a standard-catalog member (which is the whole * argument for registering it rather than answering `PRECONDITION_REQUIRED`), * and that `ActionConfirmationRequiredDetails.confirmationMember` is typed to @@ -55,6 +56,11 @@ import { const CODE = 'ACTION_CONFIRMATION_REQUIRED'; /** The package that owns both AI-facing action doors, and so owns the code. */ const OWNER = '@objectstack/runtime'; +/** + * The one second EMITTER (provenance, not identity): the MCP `delete_record` + * tool's gate stamps the same code on a record delete without the member. + */ +const SECOND_EMITTER = '@objectstack/mcp'; /** Identity helpers, so a rejected shape errors on the argument's own line. */ const asDetails = (d: ActionConfirmationRequiredDetails) => d; @@ -69,15 +75,17 @@ describe('action-confirmation contract — an unconfirmed gated action is refuse expect(ErrorCode.safeParse('ACTION_CONFIRMATION_REQUIRE').success).toBe(false); }); - it('the ledger registers it under the package that owns the action doors', () => { + it('the ledger registers it under the package that owns the action doors, and under its one second emitter', () => { expect(ERROR_CODE_LEDGER[OWNER]).toContain(CODE); // Control: the row is not merely somewhere in the ledger. If the code // moved owners, the assertion above would still pass under a `flat()`. + // The one other package listing it is the MCP `delete_record` gate's; a + // further emitter is a new door, and moving this pin is how it says so. const otherOwners = Object.entries(ERROR_CODE_LEDGER) .filter(([pkg]) => pkg !== OWNER) .filter(([, codes]) => (codes as readonly string[]).includes(CODE)) .map(([pkg]) => pkg); - expect(otherOwners).toEqual([]); + expect(otherOwners).toEqual([SECOND_EMITTER]); }); it('it is NOT a synonym of a standard-catalog member', () => { diff --git a/packages/spec/src/contracts/ai-service.ts b/packages/spec/src/contracts/ai-service.ts index a6ac55fec7a..2c6291ce59c 100644 --- a/packages/spec/src/contracts/ai-service.ts +++ b/packages/spec/src/contracts/ai-service.ts @@ -213,8 +213,10 @@ export interface AIToolDefinition { * `invokeBusinessAction` (`objectName` / `recordId` / `params`, * `packages/runtime/src/action-execution.ts`), which the MCP bridge builds * that object from. Each grows the member in the change that enforces it, so - * neither ever accepts-and-ignores it. Two placements were rejected, and the - * reasons are the contract: + * neither ever accepts-and-ignores it. A third door carries the same member + * at the top level of its input: the MCP `delete_record` tool (see + * {@link AIActionConfirmation}, "The second gate"). Two placements were + * rejected, and the reasons are the contract: * * - NOT inside `params`. That bag is not a free surface: it is CLOSED against * the action author's own declared input vocabulary. `enforceActionParams` @@ -298,6 +300,25 @@ export const AI_ACTION_CONFIRMATION_MEMBER = 'confirm'; * names the action and the exact member to set. Nothing dispatches: the * action body does not run, no record is read or written, and the refusal * carries no `status`. + * + * ## The second gate: the MCP `delete_record` tool + * + * A record delete over MCP is permanent, so the same member gates it, by the + * maintainer's decision, for every object and with no declared flag to + * consult: `delete_record` (`packages/mcp/src/mcp-http-tools.ts`) refuses + * EVERY call that does not carry the member as the boolean `true`, before + * its data bridge is reached, so nothing is read or deleted. A non-boolean + * member (the string `"true"`, `1`) is refused first by the tool's closed + * input schema, with no code and no details, exactly as on `run_action`; an + * absent or `false` member reaches the gate, and its refusal is the one above: + * the same code and HTTP status, and `error.details` shaped as + * {@link ActionConfirmationRequiredDetails}, whose `actionName` is the tool's + * name, `delete_record`. A client that handles the action refusal confirms + * with its human and retries the same call with the member set. + * + * The gate's bounds are part of the decision: REST `DELETE /api/v1/data/...` + * (the human path) and the `update_record` tool stay outside it, no object is + * exempt, and any future bulk-delete tool takes the same gate. */ export interface AIActionConfirmation { /** @@ -317,7 +338,10 @@ export interface AIActionConfirmation { * client never has to hard-code the member's spelling from documentation. */ export interface ActionConfirmationRequiredDetails { - /** The declarative action name the refused call addressed. */ + /** + * The declarative action name the refused call addressed; on the MCP + * `delete_record` door, the tool's own name, `delete_record`. + */ actionName: string; /** The object the action operates on; omitted for object-less actions. */ objectName?: string; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 7a7fbd3e301..39bc7b5733b 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -81,6 +81,21 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/mcp/src/mcp-delete-record-confirmation.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/mcp/src/mcp-delete-record-confirmation.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/mcp/src/mcp-delete-record-confirmation.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/mcp/src/mcp-stdio-tools.test.ts", "verb": "delete",