diff --git a/.changeset/23701-mcp-unique-violation.md b/.changeset/23701-mcp-unique-violation.md new file mode 100644 index 00000000000..70fe169e22d --- /dev/null +++ b/.changeset/23701-mcp-unique-violation.md @@ -0,0 +1,16 @@ +--- +'@objectstack/mcp': patch +--- + +fix(mcp): an MCP tool call that repeats a unique value answers the wire code `UNIQUE_VIOLATION`, not the engine's in-process `DUPLICATE_RECORD` (#23701) + +**Wire change in the MCP tool result: `DUPLICATE_RECORD` becomes `UNIQUE_VIOLATION`.** It applies to every tool error rendered from a thrown refusal, on both transports (Streamable HTTP and stdio), and to the record resource. Measured: `create_record` and `update_record` writing a value a unique field already holds, and `run_action` for an action whose handler write or declarative `operation: 'update'` repeats one. `resume_run` renders through the same writer. An MCP client, AI agents included, that matched `error.code === 'DUPLICATE_RECORD'` on that tool error must match `error.code === 'UNIQUE_VIOLATION'` instead. That is what the REST server, the batch and import rows, both Hono doors and the runtime dispatcher already answer for the same refusal. + +Why: the maintainer ruled one wire spelling for a unique-constraint refusal on every route, `UNIQUE_VIOLATION` (recorded in commit 65846bc46). The MCP tool writer still relayed the engine's spelling. + +- FROM `{"error":{"code":"DUPLICATE_RECORD","message":"Duplicate record refused on 'udoc': …","status":409}}`, with `isError: true` +- TO `{"error":{"code":"UNIQUE_VIOLATION","message":"Duplicate record refused on 'udoc': …","status":409}}`, with `isError: true` + +Only the code moves. The result is still an `isError` tool result, `status` stays `409`, and `message` is unchanged. The envelope's keys stay `code`, `message` and `status`: this envelope has no context slot, so the `developerMessage`, `object` and `field` that the HTTP doors carry in `error.details` are not added here. Only the engine's `DuplicateRecordError` is translated, recognised by its registered code and its class name together. A hook or action handler that throws the registered `DUPLICATE_RECORD` without being that class keeps its own code. Inside the engine, `DuplicateRecordError.code` is still `DUPLICATE_RECORD`, so an in-process caller (a hook, a flow node) is unaffected. Other coded refusals (`DELETE_RESTRICTED`, `CONCURRENT_UPDATE`) are unchanged. + +Clause-②: no diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index 232f1b56c01..378ec1bff87 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -38,6 +38,10 @@ import type { EngineAggregateOptions } 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'; +// [#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. +import { isEngineDuplicateRecordEnvelope, structuredCodeAnswer } from '@objectstack/types'; import { MCP_OAUTH_SCOPE_DATA_READ, MCP_OAUTH_SCOPE_DATA_WRITE, @@ -388,12 +392,57 @@ export interface ThrownRefusalEnvelope { * refusal. `undefined` tells the caller to keep its own message-only * rendering: nothing is invented for a throw that never had a code. `details` * is not read here; {@link errorResultWithCode} says why. + * + * ## One wire spelling for a unique-constraint refusal (#23701) + * + * Maintainer ruling (2026-09-03, recorded in commit 65846bc46): one wire + * spelling on every route, `UNIQUE_VIOLATION`; the row reports it too. + * + * The engine's `DuplicateRecordError` declares its in-process identity, + * `DUPLICATE_RECORD`, which an in-process caller (a hook, a flow node) + * branches on and which is ⛔ not renamed. This reader used to relay that + * code verbatim, so a tool call that repeated a unique value + * (`create_record`, `update_record`, a `run_action` body's write) answered + * `DUPLICATE_RECORD`, while `RestServer`'s arm, the batch and import rows, both + * Hono doors and the runtime dispatcher answer the same throw as + * `UNIQUE_VIOLATION`. An MCP client is out of process, so it gets the wire + * spelling. + * + * The code is decided here, before it is written, keyed as those doors key it + * ({@link isEngineDuplicateRecordEnvelope}, `@objectstack/types`). A producer + * that merely SPEAKS `DUPLICATE_RECORD` (a hook or an action handler throwing + * the registered code without being the engine's class) keeps its own code. + * `message` (the engine's sentence) and `status` (409) are unchanged. + * + * ⛔ Only the code moves. The other doors also carry the REST arm's context + * (`developerMessage`, `object`, `field`) in their envelope's context slot; + * this envelope has none. It is `code`, `message` and `status`, and the data + * tools withhold `details` by design ({@link errorResultWithCode}). */ export function thrownRefusalEnvelope(err: unknown): ThrownRefusalEnvelope | undefined { - const code = (err as { code?: unknown } | null | undefined)?.code; - if (typeof code !== 'string' || code.length === 0) return undefined; + const thrownCode = (err as { code?: unknown } | null | undefined)?.code; + if (typeof thrownCode !== 'string' || thrownCode.length === 0) return undefined; const status = (err as { status?: unknown }).status; - return { code, message: messageOf(err), ...(typeof status === 'number' ? { status } : {}) }; + return { + code: wireCodeOf(err, thrownCode), + message: messageOf(err), + ...(typeof status === 'number' ? { status } : {}), + }; +} + +/** + * [#23701] The code a thrown refusal crosses to an MCP client with: the + * thrown one, except for the engine's `DuplicateRecordError`, whose wire code + * is read off the shared classification's arm for that class + * (`structuredCodeAnswer`, `@objectstack/types`, the arm `RestServer` + * answers with). So this package spells no copy of the wire code of its own: + * it relays the classification's answer, as the REST door does, and the + * pins in `mcp-unique-violation-wire-code.test.ts` hold what that answer is. + */ +function wireCodeOf(err: unknown, thrownCode: string): string { + if (!isEngineDuplicateRecordEnvelope(err)) return thrownCode; + const armCode = structuredCodeAnswer(err)?.body.code; + return typeof armCode === 'string' ? armCode : thrownCode; } /** diff --git a/packages/mcp/src/mcp-unique-violation-wire-code.test.ts b/packages/mcp/src/mcp-unique-violation-wire-code.test.ts new file mode 100644 index 00000000000..9865d16f0e1 --- /dev/null +++ b/packages/mcp/src/mcp-unique-violation-wire-code.test.ts @@ -0,0 +1,237 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23701] An MCP tool call that repeats a unique value answers the ruled wire + * spelling, `UNIQUE_VIOLATION`, not the engine's in-process `DUPLICATE_RECORD`. + * + * Maintainer ruling (2026-09-03, recorded in commit 65846bc46): one wire + * spelling on every route, `UNIQUE_VIOLATION`; the row reports it too. + * + * ## What was wrong + * + * `thrownRefusalEnvelope` is the one reader both tool writers + * (`errorResultWithCode` for the data tools, `errorResultFromThrown` for + * `run_action` and `resume_run`) and the record resource share. It relayed the + * thrown `code` verbatim, so the engine's `DuplicateRecordError` reached an MCP + * client as `DUPLICATE_RECORD`. Measured before the fix, on a real + * `MCPServerRuntime` behind the runtime dispatcher's `/mcp` route over a real + * ObjectQL + SQLite engine: `create_record`, `update_record`, and `run_action` + * (a handler's write and a declarative update) each answered + * `{"error":{"code":"DUPLICATE_RECORD",…,"status":409}}` with `isError`. + * + * ## What is pinned here + * + * The HTTP `/mcp` face, through `handleHttpRequest` and the real Streamable + * HTTP transport, with a bridge double whose member throws a value shaped + * like the engine's class (`name`, `code`, `status`, `httpStatus`, `object`, + * `field`, its in-process `developerMessage`, a driver error on `cause`). This + * package composes no engine. + * + * - Each tool whose call can repeat a unique value answers `UNIQUE_VIOLATION` + * with `status` 409. The envelope's keys are unchanged: `code`, `message`, + * `status`. Neither the in-process spelling nor the driver's text appears. + * - The record resource, the reader's other consumer, answers the same code. + * - CONTROL: a producer that merely SPEAKS `DUPLICATE_RECORD` (a hook or a + * handler throwing the registered code without being the engine's class) + * keeps its own code. That is the ruling's own exception. + * - CONTROL: the class name with another code, and the engine's other 409 + * (`DELETE_RESTRICTED`), keep their codes. + * + * Codes and key presence are asserted, never wording. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import type { IMetadataService } from '@objectstack/spec/contracts'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { McpActionBridge, McpDataBridge } from './mcp-http-tools.js'; + +type Bridge = McpDataBridge & McpActionBridge; + +/** What the driver said. It must never reach the client. */ +const DRIVER_TEXT = "UNIQUE constraint failed: index 'uniq_udoc_organization_id_email'"; + +/** A value shaped like `@objectstack/objectql`'s `DuplicateRecordError`. */ +function engineDuplicate(): Error { + return Object.assign( + new Error("Duplicate record refused on 'udoc': a unique constraint on 'email' already holds this value. No record was written."), + { + name: 'DuplicateRecordError', + code: 'DUPLICATE_RECORD', + status: 409, + httpStatus: 409, + object: 'udoc', + field: 'email', + developerMessage: "Over the HTTP API this refusal is reported as UNIQUE_VIOLATION; inside the engine the thrown class carries code === 'DUPLICATE_RECORD'.", + cause: Object.assign(new Error(DRIVER_TEXT), { code: 'SQLITE_CONSTRAINT_UNIQUE' }), + }, + ); +} + +/** A hook or handler that speaks the registered code without being the engine's class. */ +function spokenDuplicate(): Error { + return Object.assign(new Error('That address is already on file'), { code: 'DUPLICATE_RECORD', status: 409 }); +} + +/** A value shaped like the engine's referential-integrity refusal: another engine 409. */ +function engineDeleteRestricted(): Error { + return Object.assign(new Error('This Parent is still referenced by 1 Child record(s).'), { + code: 'DELETE_RESTRICTED', + status: 409, + developerMessage: 'Cannot delete uparent (p1): 1 dependent uchild record(s) reference it via parent.', + }); +} + +function benignBridge(): Bridge { + return { + async listObjects() { return [{ name: 'udoc', label: 'Unique doc', fieldCount: 3 }]; }, + async describeObject() { return { name: 'udoc', fields: [{ name: 'email', type: 'text' }] }; }, + async query(object) { return { object, records: [], total: 0 }; }, + async get(_object, id) { return { id }; }, + async create(object, data) { return { object, id: 'n1', record: data }; }, + async update(object, id, data) { return { object, id, record: data }; }, + async remove(object, id) { return { object, id, success: true }; }, + async listActions() { return []; }, + async runAction(name) { return { ok: true, action: name }; }, + async resumeRun(runId) { return { ok: true, runId }; }, + }; +} + +interface Site { + tool: string; + args: Record; + /** The bridge member whose throw this tool's catch renders. */ + member: keyof Bridge; +} + +const CREATE: Site = { tool: 'create_record', args: { objectName: 'udoc', data: { email: 'taken@example.com' } }, member: 'create' }; +const UPDATE: Site = { + tool: 'update_record', + 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 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' }; + +let nextId = 1; +async function callTool(site: Site, thrown: unknown) { + const bridge = benignBridge(); + (bridge as unknown as Record)[site.member] = async () => { + throw thrown; + }; + const body = { + jsonrpc: '2.0', + id: nextId++, + method: 'tools/call', + params: { name: site.tool, arguments: site.args }, + }; + 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, parsedBody: body }, + ); + expect(res.status, `${site.tool}: the HTTP transport itself answered ${res.status}`).toBe(200); + const json = (await res.json()) as { result?: any; error?: unknown }; + expect(json.error, `${site.tool}: JSON-RPC error ${JSON.stringify(json.error)}`).toBeUndefined(); + const result = json.result; + expect(result.isError, `${site.tool}: not a tool error`).toBe(true); + expect(result.content).toHaveLength(1); + const text = String(result.content[0].text); + return { text, error: (JSON.parse(text) as { error: Record }).error }; +} + +describe('[#23701] an MCP tool call repeating a unique value answers UNIQUE_VIOLATION', () => { + for (const site of [CREATE, UPDATE, RUN_ACTION, RESUME_RUN]) { + it(`${site.tool}: the engine's class answers UNIQUE_VIOLATION / 409, with the envelope's own keys`, async () => { + const { text, error } = await callTool(site, engineDuplicate()); + + expect(error.code).toBe('UNIQUE_VIOLATION'); + expect(error.status).toBe(409); + expect(typeof error.message).toBe('string'); + // The envelope's shape does not move: no context slot is invented. + expect(Object.keys(error).sort()).toEqual(['code', 'message', 'status']); + // The in-process spelling does not cross, and neither does the driver's text. + expect(text).not.toContain('DUPLICATE_RECORD'); + expect(text).not.toContain(DRIVER_TEXT); + }); + } + + for (const site of [CREATE, RUN_ACTION]) { + it(`CONTROL ${site.tool}: a producer that merely speaks DUPLICATE_RECORD keeps its own code`, async () => { + const { error } = await callTool(site, spokenDuplicate()); + + expect(error.code).toBe('DUPLICATE_RECORD'); + expect(error.status).toBe(409); + expect(Object.keys(error).sort()).toEqual(['code', 'message', 'status']); + }); + } + + it('CONTROL: the class name with another registered code keeps that code', async () => { + const renamed = Object.assign(engineDuplicate(), { code: 'CONCURRENT_UPDATE' }); + const { error } = await callTool(UPDATE, renamed); + + expect(error.code).toBe('CONCURRENT_UPDATE'); + expect(error.status).toBe(409); + }); + + for (const site of [DELETE, RUN_ACTION]) { + it(`CONTROL ${site.tool}: the engine's other 409, DELETE_RESTRICTED, keeps its code and keys`, async () => { + const { error } = await callTool(site, engineDeleteRestricted()); + + expect(error.code).toBe('DELETE_RESTRICTED'); + expect(error.status).toBe(409); + expect(Object.keys(error).sort()).toEqual(['code', 'message', 'status']); + }); + } +}); + +describe('[#23701] the record resource, the reader\'s other consumer, answers the same code', () => { + const URI = 'objectstack://objects/udoc/records/r1'; + + async function readRefused(thrown: unknown): Promise { + const runtime = new MCPServerRuntime({ name: 'pin', version: '0.0.0' }); + const metadata = { + listObjects: vi.fn(async () => []), + getObject: vi.fn(async () => null), + } as unknown as IMetadataService; + runtime.bridgeResources(metadata, async () => { + throw thrown; + }); + const [clientSide, serverSide] = InMemoryTransport.createLinkedPair(); + await runtime.server.connect(serverSide); + const client = new Client({ name: 'pin', version: '0.0.0' }); + await client.connect(clientSide); + try { + const read = await client.readResource({ uri: URI }); + expect(read.contents).toHaveLength(1); + return String((read.contents[0] as { text?: unknown }).text); + } finally { + await client.close(); + } + } + + it('the engine\'s class: `code` UNIQUE_VIOLATION and `status` 409 beside the `error` sentence', async () => { + const text = await readRefused(engineDuplicate()); + const body = JSON.parse(text); + + expect(body.code).toBe('UNIQUE_VIOLATION'); + expect(body.status).toBe(409); + expect(typeof body.error).toBe('string'); + expect(Object.keys(body).sort()).toEqual(['code', 'error', 'status']); + expect(text).not.toContain('DUPLICATE_RECORD'); + }); + + it('CONTROL: a producer that merely speaks DUPLICATE_RECORD keeps its own code', async () => { + const body = JSON.parse(await readRefused(spokenDuplicate())); + + expect(body.code).toBe('DUPLICATE_RECORD'); + expect(body.status).toBe(409); + }); +});