diff --git a/.changeset/23509-mcp-coded-refusal.md b/.changeset/23509-mcp-coded-refusal.md new file mode 100644 index 00000000000..1a2106c64eb --- /dev/null +++ b/.changeset/23509-mcp-coded-refusal.md @@ -0,0 +1,31 @@ +--- +'@objectstack/mcp': minor +--- + +fix(mcp): the data tools, `list_actions` and the record resource answer a coded refusal with its `code` (and `status`) beside the message, on stdio and Streamable HTTP alike (#23509) + +Clause-②: yes (widening) + +**What was wrong.** When a producer refused with an ADR-0112 envelope (`code`, `status`, the message), these surfaces handed an MCP client the message alone. The code is the half a client branches on. The refusals affected: + +- a key whose owner is held: `MFA_REQUIRED` / 403, refused "with the same code" at every seam; +- `RECORD_NOT_FOUND` / 404; +- `PERMISSION_DENIED` / 403; +- `INVALID_FIELD` / 400; +- the exposure refusals `OBJECT_API_DISABLED` / 404 and `OBJECT_API_METHOD_NOT_ALLOWED` / 405. + +`run_action` and `resume_run` already rendered the code. The other tools did not, and neither did the record resource. + +**What a client reads now.** The tools render the shape `run_action` already answers with. The record resource adds the code beside its unchanged `error` sentence, in the `{ error, code, status }` vocabulary the sibling resources `objectstack://objects` and `objectstack://objects/{objectName}` already use. A refusal's message is unchanged on both. + +A refusal's `details` is not rendered on these surfaces. What `details` may disclose is decided per error class at the HTTP boundary. For example, a `PERMISSION_DENIED` from the security gate carries the caller's positions and permission sets, and REST withholds them. This package holds no copy of that policy. + +## FROM → TO + +| surface | refusal | FROM | TO | +|:--|:--|:--|:--| +| tools `list_objects`, `describe_object`, `validate_expression`, `query_records`, `aggregate_records`, `get_record`, `create_record`, `update_record`, `delete_record`, `list_actions` | coded | `isError: true`; the text is the message | `isError: true`; the text is the JSON `{ "error": { "code", "message", "status" } }`. `status` appears only when the refusal carries one. | +| resource `objectstack://objects/{objectName}/records/{recordId}` (stdio) | coded | `{ "error": MESSAGE }` | `{ "error": MESSAGE, "code", "status" }`. `status` appears only when the refusal carries one. `error` stays the same string. | +| either | no code (a plain `Error`, a thrown string, an empty or non-string `code`) | the message | unchanged, byte-identical | + +**Reading it.** For a tool error, parse the text. If it is an object with `error.code`, branch on the code and show `error.message`. Otherwise the text is the message. For the record resource, `typeof body.error === 'string'` still tells a refusal from a row, and `body.code` is present when the refusal carried one. diff --git a/packages/mcp/src/mcp-coded-refusal.test.ts b/packages/mcp/src/mcp-coded-refusal.test.ts new file mode 100644 index 00000000000..bb3a0927f20 --- /dev/null +++ b/packages/mcp/src/mcp-coded-refusal.test.ts @@ -0,0 +1,284 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23509] A thrown CODED refusal reaches an MCP client with its code, from + * every catch that renders one. A throw with no code renders exactly as before. + * + * ## What was open + * + * The data tools' catches and `list_actions`' returned + * `errorResult(messageOf(err))`, and the record resource wrote + * `{ error: message }`. So a refusal whose producer threw the ADR-0112 envelope + * reached the client as its sentence alone. That covers a held owner's key + * (`MFA_REQUIRED`, ruling C′ item 1: refused "with the same code" at every + * seam), `RECORD_NOT_FOUND`, and the exposure and permission refusals. + * `errorResultFromThrown` already rendered the code for `run_action` and + * `resume_run` (#15942); the other catches never used it. + * + * ## What is pinned here + * + * - The HTTP `/mcp` face, through `handleHttpRequest` and the real Streamable + * HTTP transport. Every tool's catch, each driven by its own bridge member, + * renders the real `recordNotFoundError` (the value `protocol.getData` + * throws when `get_record` misses on this face) as + * `{ error: { code, message, status } }` with `isError`. The table is held + * equal to the registered tool list, so a tool added later needs a row. + * - The same catches render a code-less throw as its bare message, + * byte-identical to `errorResult(messageOf(err))`. That includes the value + * the HTTP bridge's exposure gate throws today, `{ statusCode, message }` + * with no `code`. Nothing is invented for it. + * - The data tools and `list_actions` WITHHOLD `details`. Measured on `/mcp`, + * a `PERMISSION_DENIED` carries the caller's `positions` and + * `permissionSets` there, which the runtime dispatcher drops from the wire. + * `errorResultWithCode` says why. + * - The record resource adds `code` / `status` beside the unchanged `error` + * sentence and withholds `details` the same way. A code-less throw answers + * `{ error }` byte-identically. + * + * The stdio face's pins live beside the fixtures that already drive it over a + * real `StdioServerTransport`. `MFA_REQUIRED` is pinned in + * `stdio-api-key-account-posture.pin.test.ts`. The exposure refusals, on both + * the tool and the resource, are pinned in + * `plugin.record-resource-exposure.test.ts`. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; +import { recordNotFoundError } from '@objectstack/core'; +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 & Required>; + +/** Every member answers benignly, so a refusal comes only from the member a row names. */ +function benignBridge(): Bridge { + return { + async listObjects() { return [{ name: 'task', label: 'Task', fieldCount: 1 }]; }, + async describeObject() { return { name: 'task', fields: [{ name: 'title', 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 aggregate() { return []; }, + 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; + /** + * `run_action` / `resume_run` render through `errorResultFromThrown`, which + * carries `details`. That is #15942's contract, and it is not this card's to + * change. The `details` pin below covers every other row. + */ + actionDoor?: true; +} + +/** Every tool `wireBridgeTools` registers for {@link benignBridge}, each with the member it calls first. */ +const SITES: readonly Site[] = [ + { tool: 'list_objects', args: {}, member: 'listObjects' }, + { tool: 'describe_object', args: { objectName: 'task' }, member: 'describeObject' }, + { tool: 'validate_expression', args: { objectName: 'task', expression: 'record.title' }, member: 'describeObject' }, + { tool: 'query_records', args: { objectName: 'task' }, member: 'query' }, + { + tool: 'aggregate_records', + args: { objectName: 'task', aggregations: [{ function: 'count', alias: 'n' }] }, + member: 'aggregate', + }, + { 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' }, + { 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 }, +]; + +/** + * The security gate's denial as it reached `/mcp` when measured on the + * showcase stack: the caller's authorization topology rides in `details`. The + * runtime dispatcher drops these `details` from the wire. + */ +function securityGateDenial(): Error { + return Object.assign( + new Error('You do not have permission to perform this action. Contact your administrator if you need access.'), + { + code: 'PERMISSION_DENIED', + status: 403, + details: { + operation: 'delete', + object: 'showcase_private_note', + positions: ['org_member', 'everyone'], + permissionSets: ['showcase_member_default', 'member_default'], + }, + }, + ); +} + +function bridgeThrowing(member: keyof Bridge, thrown: unknown): Bridge { + const bridge = benignBridge(); + (bridge as unknown as Record)[member] = async () => { + throw thrown; + }; + return bridge; +} + +let nextId = 1; +async function rpcOverHttp(bridge: Bridge, 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, parsedBody: body }, + ); + expect(res.status, `${method}: the HTTP transport itself answered ${res.status}`).toBe(200); + const json = (await res.json()) as { result?: any; error?: unknown }; + expect(json.error, `${method}: JSON-RPC error ${JSON.stringify(json.error)}`).toBeUndefined(); + return json.result; +} + +async function callTool(site: Site, thrown: unknown) { + return rpcOverHttp(bridgeThrowing(site.member, thrown), 'tools/call', { name: site.tool, arguments: site.args }); +} + +describe('MCP tools over HTTP /mcp: a coded refusal carries its code (#23509)', () => { + it('the table names every registered tool, so a tool added later needs a row here', async () => { + const listed = await rpcOverHttp(benignBridge(), 'tools/list'); + const names = (listed.tools as Array<{ name: string }>).map((t) => t.name).sort(); + expect(names).toEqual(SITES.map((s) => s.tool).sort()); + }); + + for (const site of SITES) { + it(`${site.tool}: RECORD_NOT_FOUND from ${String(site.member)} answers code, message and status`, async () => { + const result = await callTool(site, recordNotFoundError('task', 't9')); + expect(result.isError).toBe(true); + expect(result.content).toHaveLength(1); + expect(JSON.parse(result.content[0].text)).toEqual({ + error: { code: 'RECORD_NOT_FOUND', message: 'Record t9 not found in task', status: 404 }, + }); + }); + + it(`${site.tool}: a code-less throw from ${String(site.member)} answers its bare message, as before`, async () => { + const result = await callTool(site, new Error('engine exploded')); + expect(result).toEqual({ content: [{ type: 'text', text: 'engine exploded' }], isError: true }); + }); + + if (!site.actionDoor) { + it(`${site.tool}: a PERMISSION_DENIED answers code, message and status, and WITHHOLDS its details`, async () => { + const result = await callTool(site, securityGateDenial()); + expect(result.isError).toBe(true); + expect(JSON.parse(result.content[0].text)).toEqual({ + error: { + code: 'PERMISSION_DENIED', + message: 'You do not have permission to perform this action. Contact your administrator if you need access.', + status: 403, + }, + }); + expect(result.content[0].text).not.toContain('permissionSets'); + }); + } + } + + // One site is enough below: every catch renders through the one reader. + const query = SITES.find((s) => s.tool === 'query_records')!; + + it('the HTTP bridge exposure gate\'s `{ statusCode, message }` (no `code`) answers its bare message: nothing is invented', async () => { + const thrown = { statusCode: 404, message: "Object 'task' is not exposed via the API" }; + expect(await callTool(query, thrown)).toEqual({ + content: [{ type: 'text', text: "Object 'task' is not exposed via the API" }], + isError: true, + }); + }); + + it('an empty or non-string `code`, or a thrown string, is not a code', async () => { + for (const thrown of [ + Object.assign(new Error('empty code'), { code: '', status: 403 }), + Object.assign(new Error('numeric code'), { code: 403, status: 403 }), + 'a thrown string', + ]) { + const text = thrown instanceof Error ? thrown.message : String(thrown); + expect(await callTool(query, thrown), text).toEqual({ content: [{ type: 'text', text }], isError: true }); + } + }); + + it('omits a `status` that is not a number', async () => { + const statusless = Object.assign(new Error('no status here'), { code: 'PERMISSION_DENIED', status: '403' }); + expect(JSON.parse((await callTool(query, statusless)).content[0].text)).toEqual({ + error: { code: 'PERMISSION_DENIED', message: 'no status here' }, + }); + }); +}); + +describe('the record resource: a coded refusal adds its code beside the unchanged sentence (#23509)', () => { + const URI = 'objectstack://objects/task/records/t1'; + + /** Read the record resource of a server whose reader throws `thrown`; answers the raw body text. */ + 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); + expect(read.contents[0]!.uri).toBe(URI); + return String((read.contents[0] as { text?: unknown }).text); + } finally { + await client.close(); + } + } + + it('MFA_REQUIRED / 403: `code` and `status` beside `error`, which stays the sentence', async () => { + const message = 'Multi-factor authentication is required. Please set up an authenticator app to continue.'; + const body = JSON.parse(await readRefused(Object.assign(new Error(message), { code: 'MFA_REQUIRED', status: 403 }))); + expect(body).toEqual({ error: message, code: 'MFA_REQUIRED', status: 403 }); + // The discriminator a resource client already uses to tell a refusal from a row. + expect(typeof body.error).toBe('string'); + }); + + it('a PERMISSION_DENIED answers code and status and WITHHOLDS its details', async () => { + const denial = securityGateDenial(); + expect(JSON.parse(await readRefused(denial))).toEqual({ + error: denial.message, + code: 'PERMISSION_DENIED', + status: 403, + }); + }); + + it('carries no `status` when the refusal has none', async () => { + const thrown = Object.assign(new Error('Field read denied: salary'), { code: 'PERMISSION_DENIED' }); + expect(JSON.parse(await readRefused(thrown))).toEqual({ + error: 'Field read denied: salary', + code: 'PERMISSION_DENIED', + }); + }); + + it('a code-less throw answers `{ error }` byte-identically', async () => { + expect(await readRefused(new Error('engine exploded'))).toBe(JSON.stringify({ error: 'engine exploded' })); + expect(await readRefused('a thrown string')).toBe(JSON.stringify({ error: 'a thrown string' })); + expect(await readRefused(Object.assign(new Error('empty code'), { code: '' }))).toBe( + JSON.stringify({ error: 'empty code' }), + ); + }); +}); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index 48c07ff637d..b7a491d6641 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -304,30 +304,76 @@ function errorResult(message: string) { * * Uncoded throws fall through to `errorResult` unchanged, so this widens what a * caller can read and narrows nothing. + * + * `run_action` and `resume_run` only. The other catches render through + * {@link errorResultWithCode}, which serves the same envelope WITHOUT `details`; + * that function says why. */ function errorResultFromThrown(err: unknown) { - const code = (err as { code?: unknown } | null | undefined)?.code; - if (typeof code !== 'string' || code.length === 0) return errorResult(messageOf(err)); + const envelope = thrownRefusalEnvelope(err); + if (!envelope) return errorResult(messageOf(err)); const details = (err as { details?: unknown }).details; - const status = (err as { status?: unknown }).status; + return codedErrorResult({ ...envelope, ...(details !== undefined ? { details } : {}) }); +} + +/** + * [#23509] The data tools' and `list_actions`' tool error: a thrown coded + * refusal renders its `code` and `status` beside the message, in + * {@link errorResultFromThrown}'s shape. A throw with no code renders as + * `errorResult(messageOf(err))`, byte-identical to what every one of these + * catches answered before. + * + * These catches used to answer the message alone, so a client never read the + * code it is meant to branch on. The affected refusals: a key whose owner is + * held (`MFA_REQUIRED`, which ruling C′ item 1 says is refused "with the same + * code" at every seam), `RECORD_NOT_FOUND`, the exposure and permission + * refusals, and `INVALID_FIELD`. + * + * ⛔ `details` is WITHHELD here, deliberately. What a refusal's `details` may + * disclose is decided per error class at the HTTP boundary, and this package + * holds no copy of that policy. Measured on `/mcp`: a `PERMISSION_DENIED` + * thrown by the security gate carries the caller's `positions` and + * `permissionSets` (and, on a cascade, an object the caller never addressed). + * The runtime dispatcher drops those `details` from the wire, so serving them + * here would disclose on MCP what REST withholds. `code`, `message` and + * `status` are what every boundary serves. + */ +function errorResultWithCode(err: unknown) { + const envelope = thrownRefusalEnvelope(err); + return envelope ? codedErrorResult({ ...envelope }) : errorResult(messageOf(err)); +} + +function codedErrorResult(error: ThrownRefusalEnvelope & { details?: unknown }) { return { - content: [ - { - type: 'text' as const, - text: jsonText({ - error: { - code, - message: messageOf(err), - ...(typeof status === 'number' ? { status } : {}), - ...(details !== undefined ? { details } : {}), - }, - }), - }, - ], + content: [{ type: 'text' as const, text: jsonText({ error }) }], isError: true as const, }; } +/** What {@link thrownRefusalEnvelope} reads off a thrown coded refusal. */ +export interface ThrownRefusalEnvelope { + code: string; + message: string; + status?: number; +} + +/** + * Read the ADR-0112 `code`, message and `status` off a thrown value, or + * `undefined` when it carries no non-empty string `code`. + * + * The one reader both MCP surfaces use, so the tools and the record resource + * (`MCPServerRuntime.bridgeResources`) agree on what counts as a coded + * 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. + */ +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 status = (err as { status?: unknown }).status; + return { code, message: messageOf(err), ...(typeof status === 'number' ? { status } : {}) }; +} + /** * [ADR-0090 D10 — ruling 2026-09-08, consequence 2] Attach the D10 narrowing * notice to a query result WITHOUT reshaping it. @@ -602,7 +648,7 @@ export function registerObjectTools( status: 503, }); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -626,7 +672,7 @@ export function registerObjectTools( if (!def) return errorResult(`Object "${objectName}" not found`); return textResult(def); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -701,7 +747,7 @@ export function registerObjectTools( }, }); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -781,7 +827,7 @@ export function registerObjectTools( if (!diagnosed?.narrowed || !diagnosed.statement) return textResult(result); return textResult(withDelegationNotice(result, diagnosed.statement)); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -873,7 +919,7 @@ export function registerObjectTools( ...(truncated ? { truncated: true } : {}), }); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -900,7 +946,7 @@ export function registerObjectTools( if (record == null) return errorResult(`Record "${recordId}" not found in "${objectName}"`); return textResult(record); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -929,7 +975,7 @@ export function registerObjectTools( try { return textResult(await bridge.create(objectName, data)); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -963,7 +1009,7 @@ export function registerObjectTools( try { return textResult(await bridge.update(objectName, recordId, data)); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -987,7 +1033,7 @@ export function registerObjectTools( try { return textResult(await bridge.remove(objectName, recordId)); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); @@ -1054,7 +1100,7 @@ export function registerActionTools( : actions.filter((a) => !a.objectName || !isSystemObject(a.objectName)); return textResult({ actions: visible, totalCount: visible.length }); } catch (err) { - return errorResult(messageOf(err)); + return errorResultWithCode(err); } }, ); diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 6a9c1c49323..3ef6ce641eb 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -7,7 +7,7 @@ import type { Logger, IMetadataService, AIToolDefinition } from '@objectstack/sp import type { Agent } from '@objectstack/spec/ai'; import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system'; import type { ToolRegistry, ToolExecutionResult } from './types.js'; -import { wireBridgeTools } from './mcp-http-tools.js'; +import { wireBridgeTools, thrownRefusalEnvelope } from './mcp-http-tools.js'; import type { McpDataBridge, McpActionBridge, @@ -1258,11 +1258,31 @@ export class MCPServerRuntime { }; } catch (err) { const message = err instanceof Error ? err.message : String(err); + // [#23509] A coded refusal carries its ADR-0112 `code` (and its + // `status`, when it has one) beside the sentence, so a client reads + // the code it branches on. A held owner's key is refused here "with + // the same code" (ruling C′ item 1) as on REST. The members are + // ADDED to the body, in the `{ error, code, status }` vocabulary + // the two sibling resources above already answer with (#6055, + // #6504). `error` stays the same string, so a client that tells a + // refusal from a row by `typeof body.error === 'string'` still + // does. A throw with no code answers `{ error }` as before. + // `details` is withheld, as on the data tools; `errorResultWithCode` + // in mcp-http-tools.ts says why. + const envelope = thrownRefusalEnvelope(err); return { contents: [{ uri: `objectstack://objects/${objectName}/records/${recordId}`, mimeType: 'application/json', - text: JSON.stringify({ error: message }), + text: JSON.stringify( + envelope + ? { + error: message, + code: envelope.code, + ...(envelope.status !== undefined ? { status: envelope.status } : {}), + } + : { error: message }, + ), }], }; } diff --git a/packages/mcp/src/plugin.record-resource-exposure.test.ts b/packages/mcp/src/plugin.record-resource-exposure.test.ts index 0220fe5f9db..38f9cb43e64 100644 --- a/packages/mcp/src/plugin.record-resource-exposure.test.ts +++ b/packages/mcp/src/plugin.record-resource-exposure.test.ts @@ -210,19 +210,38 @@ function mockContext(services: Record) { /** * One read path's answer, normalized to the same shape for both surfaces. * - * The two surfaces serialize a refusal differently and neither is this card's - * to change: the resource wraps it as a JSON `{ error }` body (its `catch` in - * `bridgeResources`), the tool answers `isError: true` with the bare message as - * text (`errorResult(messageOf(err))`). Normalizing here is what lets the cases - * below compare VERDICTS rather than two serialization accidents. + * The two surfaces serialize a refusal differently. The resource answers a JSON + * body with the sentence as `error` and the code and status beside it + * (`{ error, code, status }`, its `catch` in `bridgeResources`). The tool + * answers `isError: true` with the envelope as JSON text + * (`{ error: { code, message, status } }`, `errorResultFromThrown`). Both have + * carried the code since #23509; before it, each carried the sentence alone. + * Normalizing here is what lets the cases below compare VERDICTS rather than + * two serialization accidents. */ interface PathAnswer { /** The refusal message, or `undefined` when the path served a row. */ error?: string; + /** The refusal's ADR-0112 code, as the wire carried it. */ + code?: unknown; + /** The refusal's status, as the wire carried it. */ + status?: unknown; /** The row, or `undefined` when the path refused. */ record?: any; } +/** A tool error's text: the coded envelope when it is one, else the bare message (a code-less throw). */ +function toolRefusal(text: string): PathAnswer { + let parsed: any; + try { + parsed = JSON.parse(text); + } catch { + return { error: text }; + } + if (typeof parsed?.error?.message !== 'string') return { error: text }; + return { error: parsed.error.message, code: parsed.error.code, status: parsed.error.status }; +} + interface BothPaths { /** What `resources/read` on the record URI answered. */ resource: PathAnswer; @@ -273,7 +292,7 @@ async function readBothPaths(def: unknown): Promise { const resourceBody = JSON.parse(resourceFrame.result.contents[0].text); const resource: PathAnswer = typeof resourceBody?.error === 'string' - ? { error: resourceBody.error } + ? { error: resourceBody.error, code: resourceBody.code, status: resourceBody.status } : { record: resourceBody }; const toolFrame = await session.rpc('tools/call', { @@ -283,7 +302,7 @@ async function readBothPaths(def: unknown): Promise { expect(toolFrame.error, 'tools/call failed at the protocol level').toBeUndefined(); const toolText = toolFrame.result.content[0].text as string; const tool: PathAnswer = toolFrame.result.isError - ? { error: toolText } + ? toolRefusal(toolText) : { record: JSON.parse(toolText) }; return { @@ -338,6 +357,12 @@ describe('#8266 one declaration, both read paths, same verdict', () => { // sentence because they take the same decision from the same helper. expect(both.resource.error).toBe(both.tool.error); + // [#23509] …and the same CODE, which both wires now carry beside it. + expect(both.resource.code).toBe('OBJECT_API_DISABLED'); + expect(both.resource.status).toBe(404); + expect(both.tool.code).toBe(both.resource.code); + expect(both.tool.status).toBe(both.resource.status); + // Refused BEFORE dispatch — the engine was never asked for the row, by // either path. A gate that refused after reading would still have read. expect(both.rowReads).toHaveLength(0); @@ -387,6 +412,10 @@ describe('#8266 one declaration, both read paths, same verdict', () => { expect(both.tool.record).toBeUndefined(); expect(both.resource.error).toBe(both.tool.error); + expect(both.resource.code).toBe('OBJECT_API_METHOD_NOT_ALLOWED'); + expect(both.resource.status).toBe(405); + expect(both.tool.code).toBe(both.resource.code); + expect(both.tool.status).toBe(both.resource.status); expect(both.rowReads).toHaveLength(0); }); @@ -419,12 +448,11 @@ describe('#8266 one declaration, both read paths, same verdict', () => { describe('#8266 the refusal carries the ADR-0112 envelope', () => { /** - * The wire assertions above can only read `.message`: BOTH the resource - * serializer (`bridgeResources`' `catch`) and the `get_record` tool - * (`errorResult(messageOf(err))`) forward the message alone. That is - * pre-existing on both surfaces and is not this card's to change — so the - * envelope is asserted here, at the seam that actually carries it, rather - * than left unasserted or faked into a wire test that cannot see it. + * The wire assertions above read the code and the status since #23509; both + * surfaces forwarded the message alone before it. The member they still do + * not render is the 405's effective operation set (`allowedOperations`), so + * the envelope is asserted here, at the seam that actually carries it, + * rather than left unasserted or faked into a wire test that cannot see it. */ async function refusalFor(def: unknown, action: string): Promise { const metadataService = fakeMetadata(def) as unknown as IMetadataService; diff --git a/packages/mcp/src/stdio-api-key-account-posture.pin.test.ts b/packages/mcp/src/stdio-api-key-account-posture.pin.test.ts index 3b796459c08..0334b95f829 100644 --- a/packages/mcp/src/stdio-api-key-account-posture.pin.test.ts +++ b/packages/mcp/src/stdio-api-key-account-posture.pin.test.ts @@ -28,7 +28,9 @@ * principal-bound call (the data tools' bridge and the record resource) * rejects with `code` `MFA_REQUIRED`, `status` 403 and the session's * message, before any read or write; over the real stdio wire a - * `tools/call` answers a tool error carrying that remediation message; + * `tools/call` answers a tool error carrying that code, status and + * remediation message, and `resources/read` on the record answers the code + * and status beside the message (#23509); * - a key whose owner holds only `PASSWORD_RESET_REQUIRED` (scope * `credential`) → served; * - the same session: a posture that APPEARS refuses the next call, and a @@ -190,9 +192,11 @@ async function refusalOf(call: Promise): Promise<{ code?: unknown; stat } const HELD_ENVELOPE = { code: 'MFA_REQUIRED', status: 403, message: MFA_REQUIRED.message }; +/** The same envelope as a tool error renders it: `errorResultFromThrown`'s `{ code, message, status }`. */ +const HELD_ENVELOPE_WIRE = { code: 'MFA_REQUIRED', message: MFA_REQUIRED.message, status: 403 }; -/** One `tools/call` over a real `StdioServerTransport` attached to the long-lived server. */ -async function callToolOverStdio(runtime: MCPServerRuntime, name: string, args: Record) { +/** One request (`tools/call`, `resources/read`) over a real `StdioServerTransport` attached to the long-lived server. */ +async function rpcOverStdio(runtime: MCPServerRuntime, method: string, params: Record) { const serverStdin = new PassThrough(); const serverStdout = new PassThrough(); const transport = new StdioServerTransport(serverStdin, serverStdout); @@ -225,7 +229,7 @@ async function callToolOverStdio(runtime: MCPServerRuntime, name: string, args: send({ id: 1, method: 'initialize', params: { protocolVersion: '2025-06-18', capabilities: {}, clientInfo: { name: 'pin', version: '0.0.0' } } }); await answer(1); send({ method: 'notifications/initialized' }); - send({ id: 2, method: 'tools/call', params: { name, arguments: args } }); + send({ id: 2, method, params }); return await answer(2); } finally { await transport.close().catch(() => {}); @@ -251,11 +255,23 @@ describe('MCP stdio — a key answers as its owner\'s session does, on every cal expect(await refusalOf(getRecord(OBJECT, 'n1'))).toEqual(HELD_ENVELOPE); expect(host.fixture.notes.filter((n) => n.title === 'held write'), 'a held owner\'s write landed').toHaveLength(0); - // What a stdio client sees: a tool error carrying the remediation message. - const answered = await callToolOverStdio(runtime, 'query_records', { objectName: OBJECT }); + // What a stdio client sees: a tool error carrying the code, the status and + // the remediation message — "the same code" REST answers (#23509; the + // message alone reached it before). + const answered = await rpcOverStdio(runtime, 'tools/call', { name: 'query_records', arguments: { objectName: OBJECT } }); expect(answered.error).toBeUndefined(); expect(answered.result?.isError).toBe(true); - expect(answered.result?.content?.[0]?.text).toBe(MFA_REQUIRED.message); + expect(answered.result?.content).toHaveLength(1); + expect(JSON.parse(answered.result?.content?.[0]?.text)).toEqual({ error: HELD_ENVELOPE_WIRE }); + + // …and the record resource on the same wire: the code beside the sentence. + const read = await rpcOverStdio(runtime, 'resources/read', { uri: `objectstack://objects/${OBJECT}/records/n1` }); + expect(read.error).toBeUndefined(); + expect(JSON.parse(read.result?.contents?.[0]?.text)).toEqual({ + error: MFA_REQUIRED.message, + code: 'MFA_REQUIRED', + status: 403, + }); }); it('SERVES a key whose owner holds only a credential-scope posture', async () => { diff --git a/packages/mcp/src/stdio-data-bridge.relation-condition-target-exposure.test.ts b/packages/mcp/src/stdio-data-bridge.relation-condition-target-exposure.test.ts index 6e56811f2d9..4d93112f158 100644 --- a/packages/mcp/src/stdio-data-bridge.relation-condition-target-exposure.test.ts +++ b/packages/mcp/src/stdio-data-bridge.relation-condition-target-exposure.test.ts @@ -123,8 +123,15 @@ describe('[#22777] the stdio bridge refuses a condition on a target the decision }>; const refused = await call({ hidden: { name: 'x' } }); expect(refused.isError).toBe(true); - expect(refused.content[0]!.text).toContain("Object 'rcx_hidden' is not exposed via the API"); - expect(refused.content[0]!.text).toContain("'where.hidden'"); + // [#23509] The tool renders the refusal's envelope: the code and status + // the bridge threw, beside the unchanged sentence. + const { error } = JSON.parse(refused.content[0]!.text) as { + error: { code: unknown; status: unknown; message: string }; + }; + expect(error.code).toBe('OBJECT_API_DISABLED'); + expect(error.status).toBe(404); + expect(error.message).toContain("Object 'rcx_hidden' is not exposed via the API"); + expect(error.message).toContain("'where.hidden'"); expect(engine.find).not.toHaveBeenCalled(); const served = await call({ listonly: { name: 'x' } }); expect(served.isError).toBeFalsy();