diff --git a/.changeset/mcp-stdio-record-not-found-envelope.md b/.changeset/mcp-stdio-record-not-found-envelope.md new file mode 100644 index 0000000000..6f5497affe --- /dev/null +++ b/.changeset/mcp-stdio-record-not-found-envelope.md @@ -0,0 +1,21 @@ +--- +'@objectstack/mcp': patch +--- + +fix(mcp): stdio bridge's `update`/`remove` throw the shared `RECORD_NOT_FOUND` envelope (#8422) + +The stdio MCP bridge's `update()` and `remove()` — the two by-id write seams +that probe for the row before mutating it — minted their own local +`recordNotFound(object, id)`, returning a bare `Error` with neither `code` +nor `status`. The HTTP bridge's `callData` path already throws +`recordNotFoundError` (`code: 'RECORD_NOT_FOUND'`, `status: 404`, +`@objectstack/core`, #4435/#5138/#7867) for the identical miss, so the same +operation answered a missing id with two different envelopes depending on +which MCP transport served it. + +`packages/mcp/src/stdio-data-bridge.ts` now imports `recordNotFoundError` +from `@objectstack/core` — a dependency the package already declares — and +throws it from both seams instead. `registerObjectTools` still turns the +throw into a tool error exactly as before; only the thrown object's shape +changed. No exported symbol moves and no authorable metadata is affected, so +this ships as a `patch`. diff --git a/packages/mcp/src/stdio-data-bridge.not-found.test.ts b/packages/mcp/src/stdio-data-bridge.not-found.test.ts new file mode 100644 index 0000000000..53b81aa8bc --- /dev/null +++ b/packages/mcp/src/stdio-data-bridge.not-found.test.ts @@ -0,0 +1,125 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8422 — the stdio bridge's by-id write seams must throw the repo's ONE + * not-found envelope (`recordNotFoundError`, `@objectstack/core`), not a + * locally minted `Error`. + * + * The HTTP bridge routes its data verbs through `callData`, which throws + * `recordNotFoundError` — `code: 'RECORD_NOT_FOUND'`, `status: 404` + * (`packages/core/src/utils/record-not-found.ts`, #4435/#5138/#7867). The + * stdio bridge minted its own bare `Error` for the identical miss, so a + * stdio caller got a message with no machine-readable code and nothing that + * maps to 404 — the same operation, two different envelopes depending on + * which transport served it. + * + * Both by-id write seams are covered — `update()` and `remove()` — asserting + * on `code` AND `status`, not merely that something threw: an assertion that + * only checks for a thrown error would have passed against the bare `Error` + * this card exists to remove. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +import { createStdioDataBridge } from './stdio-data-bridge.js'; + +/** An engine that resolves NO row for any id — every by-id write is a miss. */ +function makeEmptyEngine() { + return { + find: vi.fn(async () => []), + findOne: vi.fn(async () => null), + insert: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + count: vi.fn(async () => 0), + aggregate: vi.fn(async () => [{ n: 0 }]), + }; +} + +/** An object definition with no exposure restriction, so the miss is what refuses. */ +function makeMetadata() { + return { + listObjects: vi.fn(async () => [{ name: 'task', label: 'Task', fields: {} }]), + getObject: vi.fn(async () => ({ name: 'task', label: 'Task', enable: { apiEnabled: true } })), + get: vi.fn(async () => null), + list: vi.fn(async () => []), + exists: vi.fn(async () => true), + getRegisteredTypes: vi.fn(async () => ['object']), + register: vi.fn(), + unregister: vi.fn(), + }; +} + +const PRINCIPAL = { userId: 'u1', isSystem: false } as unknown as ExecutionContext; + +function makeBridge() { + const engine = makeEmptyEngine(); + const metadataService = makeMetadata(); + const bridge = createStdioDataBridge({ + engine: engine as unknown as IDataEngine, + metadataService: metadataService as unknown as IMetadataService, + resolvePrincipal: async () => PRINCIPAL, + }); + return { bridge, engine, metadataService }; +} + +/** A rejection's payload, narrowed once so callers never juggle `unknown`. */ +type NotFoundEnvelope = Error & { code?: string; status?: number }; + +/** + * The rejection `run` throws, typed — or `null` if it resolved. One explicit + * cast, shared, rather than `.catch((e) => e)` at each call site: that idiom + * infers the settled value as `unknown` here (TResult from an `any`-typed + * catch parameter widens to `unknown`), which is a second, unrelated way to + * fail `tsc` — this repo's TEST_DEBT ledger already carries 53 raw errors for + * `packages/mcp` from the *other* half of that idiom (`await res.json()`), + * and a fix for this card must not add a fourth without narrowing it. + */ +async function catchError(run: () => Promise): Promise { + return (await run().then( + () => null, + (e: unknown) => e, + )) as NotFoundEnvelope | null; +} + +/** + * Assert a not-found refusal by its ENVELOPE, not by the fact that something + * threw — a bare `Error` also satisfies `.toThrow()`, which is exactly the + * defect this card removes. + */ +async function expectRecordNotFound(run: () => Promise): Promise { + const err = await catchError(run); + expect(err, 'the call resolved — no not-found refusal was raised').toBeTruthy(); + expect(err!.code).toBe('RECORD_NOT_FOUND'); + expect(err!.status).toBe(404); +} + +describe('#8422 stdio bridge by-id writes throw the shared not-found envelope', () => { + it('update() on a missing id throws RECORD_NOT_FOUND / 404', async () => { + const { bridge, engine } = makeBridge(); + + await expectRecordNotFound(() => bridge.update('task', 'ghost', { title: 'x' })); + // Refused before the write dispatched — the same existence-before-mutation + // property the HTTP path (`callData`) holds. + expect(engine.update).not.toHaveBeenCalled(); + }); + + it('remove() on a missing id throws RECORD_NOT_FOUND / 404', async () => { + const { bridge, engine } = makeBridge(); + + await expectRecordNotFound(() => bridge.remove('task', 'ghost')); + expect(engine.delete).not.toHaveBeenCalled(); + }); + + it('both seams throw the SAME envelope shape — one declaration, not two', async () => { + const { bridge } = makeBridge(); + + const updateErr = await catchError(() => bridge.update('task', 'ghost', { title: 'x' })); + const removeErr = await catchError(() => bridge.remove('task', 'ghost')); + + expect(updateErr?.code).toBe(removeErr?.code); + expect(updateErr?.status).toBe(removeErr?.status); + expect(updateErr?.code).toBe('RECORD_NOT_FOUND'); + }); +}); diff --git a/packages/mcp/src/stdio-data-bridge.ts b/packages/mcp/src/stdio-data-bridge.ts index 07fe7e5ab9..9c4179fa8b 100644 --- a/packages/mcp/src/stdio-data-bridge.ts +++ b/packages/mcp/src/stdio-data-bridge.ts @@ -65,6 +65,13 @@ import { } from '@objectstack/spec/data'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +// [#8422] The repo's ONE single-record 404 (#4435/#5138/#7867). Imported from +// `@objectstack/core` rather than re-minted here or reached via +// `@objectstack/metadata-protocol`'s re-export: this package already declares +// a direct `@objectstack/core` dependency (`plugin.ts` imports from it too), +// and `@objectstack/core` is the lowest package that carries the factory, so +// there is no reason to add a second import path to the same function. +import { recordNotFoundError } from '@objectstack/core'; import type { McpDataBridge, McpObjectSummary } from './mcp-http-tools.js'; /** What {@link createStdioDataBridge} needs from the host plugin. */ @@ -119,18 +126,6 @@ async function findById( return unwrapRows(res)[0] ?? null; } -/** - * The "this id names no row" refusal, raised BEFORE a write is attempted. - * - * A write path that answers success for an id that matched nothing is the - * #5138 / #5581 defect the HTTP path already paid for: an integrator reading - * a success receipt records the change as landed. `registerObjectTools` turns - * a throw into a tool error, so the caller is told. - */ -function recordNotFound(object: string, id: string): Error { - return new Error(`Record "${id}" not found in "${object}"`); -} - /** * Bridge method → the `callData` action name the HTTP bridge gates it under. * @@ -342,12 +337,21 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge async update(object, id, data) { const context = await resolvePrincipal(); - // Before the existence probe, not after: `recordNotFound` vs. a hit is an - // observable difference, so gating second would answer "that id names no - // row" for an object the author declared unexposed. + // Before the existence probe, not after: refusing on exposure vs. on a + // miss is an observable difference, so gating second would answer "that + // id names no row" for an object the author declared unexposed. await enforceApiExposure(metadataService, object, GATED_ACTIONS.update, context); const existing = await findById(engine, object, id, context); - if (!existing) throw recordNotFound(object, id); + // The "this id names no row" refusal, raised BEFORE the write is + // attempted — a write path that answered success for an id that matched + // nothing is the #5138/#5581 defect the HTTP path already paid for: an + // integrator reading a success receipt records the change as landed. + // `registerObjectTools` turns a throw into a tool error, so the caller + // is told. [#8422] Throws the repo's ONE not-found envelope + // (`recordNotFoundError`, `@objectstack/core`) rather than a bare + // `Error`, so a stdio caller sees the same `RECORD_NOT_FOUND` / 404 the + // HTTP bridge's `callData` path throws for the identical miss. + if (!existing) throw recordNotFoundError(object, id); await engine.update(object, data, { where: { id }, context }); return { object, id, record: { ...existing, ...data } }; }, @@ -357,7 +361,8 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge // Gated before the probe, for the reason `update` states. await enforceApiExposure(metadataService, object, GATED_ACTIONS.remove, context); const existing = await findById(engine, object, id, context); - if (!existing) throw recordNotFound(object, id); + // Same shared envelope as `update`, above. + if (!existing) throw recordNotFoundError(object, id); await engine.delete(object, { where: { id }, context }); // `success`, not `deleted` — the spec's `DeleteDataResponse` key (#5581). return { object, id, success: true }; diff --git a/scripts/check-engine-double-contract.mjs b/scripts/check-engine-double-contract.mjs index 045dcb269e..c27b10b3ef 100644 --- a/scripts/check-engine-double-contract.mjs +++ b/scripts/check-engine-double-contract.mjs @@ -720,21 +720,28 @@ function scanSource(fileName, text, slice = SLICES[0]) { // #5138, #5581, #7867). A narrower gate that could not be written is not a // better gate than a wide one that can. // -// ## Deliberately NOT asserted yet: WHICH not-found envelope (#8194) +// ## WHICH not-found envelope (#8194, tightened to SHARED_ONLY by #8422) // -// Three of the four seams reach `recordNotFoundError` -- the repo's ONE -// not-found envelope (`@objectstack/core`, moved there by #7867 for exactly -// the "two layers cannot disagree about it" reason its header argues). The -// fourth, `packages/mcp/src/stdio-data-bridge.ts`, mints its own local -// `recordNotFound` returning a bare `Error` with neither `code` nor `status`. +// #8194 measured all four seams and found three reaching `recordNotFoundError` +// -- the repo's ONE not-found envelope (`@objectstack/core`, moved there by +// #7867 for exactly the "two layers cannot disagree about it" reason its +// header argues) -- while the fourth, `packages/mcp/src/stdio-data-bridge.ts`, +// minted its own local `recordNotFound` returning a bare `Error` with neither +// `code` nor `status`. // -// That is a real divergence and it is filed as its own card, not laundered -// through a ledger entry here: this gate would have opened RED on a defect -// outside the change that introduced the gate, which is the one way to teach -// readers that a red run means "someone else's problem". So the verdict below -// records WHICH envelope each seam reaches and prints it, and requiring the -// shared one is a one-line tightening the day that card lands -- `SHARED_ONLY` -// is the switch, and the seam list is already both-directions complete. +// That was a real divergence and #8194 filed it as its own card rather than +// laundering it through a ledger entry here: opening this gate RED on a +// defect outside the change that introduced it would have taught readers that +// a red run means "someone else's problem". So the verdict recorded WHICH +// envelope each seam reached and printed it, with `refusal: 'local'` as the +// visible-but-not-failing state -- deliberately not `!x.refusal` (that already +// failed) and not silence either. +// +// #8422 fixed the fourth seam, so all four now reach the shared envelope -- +// the SHARED_ONLY tightening below is that one-line change, made the day the +// seam list actually went both-directions complete. `refusal !== 'shared'` +// now fails on EITHER a local mint or no refusal at all: a future fifth seam +// that reinvents the envelope reddens here instead of shipping unnoticed. /** Where the repo's ONE not-found envelope may be imported from (#7867). */ const ENVELOPE_MODULES = [ @@ -1240,11 +1247,18 @@ function audit() { ); } + // SHARED_ONLY (#8422): a seam must reach the shared envelope specifically -- + // `refusal !== 'shared'` catches both a local mint (`refusal === 'local'`) + // and no refusal at all (`refusal === null`), so a seam that merely throws + // SOME error no longer reads as compliant. for (const { file, seams } of seamFiles) { - for (const s of seams.filter((x) => !x.refusal)) { + for (const s of seams.filter((x) => x.refusal !== 'shared')) { + const state = s.refusal === 'local' + ? 'refuses through a locally minted error rather than the shared envelope' + : 'does not refuse anywhere before it'; errors.push( `REFUSES: ${file}:${s.line} — ${s.fn}() performs a by-id ${s.verb} on a caller-supplied id ` - + 'and then answers a success receipt, without refusing anywhere before it. A write that ' + + `and then answers a success receipt, and ${state}. A write that ` + 'touched zero rows reporting success is the #4435/#5138/#7867 defect: a typo\'d id, an ' + 'already-deleted row and a real write become indistinguishable, and an integrator ' + 'reading the receipt records the change as landed. Refuse before you answer — probe '