Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .changeset/mcp-stdio-record-not-found-envelope.md
Original file line numberDiff line numberDiff line change
@@ -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`.
125 changes: 125 additions & 0 deletions packages/mcp/src/stdio-data-bridge.not-found.test.ts
Original file line numberDiff line numberDiff line change
@@ -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<unknown>): Promise<NotFoundEnvelope | null> {
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<unknown>): Promise<void> {
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');
});
});
39 changes: 22 additions & 17 deletions packages/mcp/src/stdio-data-bridge.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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. */
Expand DownExpand Up@@ -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.
*
Expand DownExpand Up@@ -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 } };
},
Expand All@@ -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 };
Expand Down
44 changes: 29 additions & 15 deletions scripts/check-engine-double-contract.mjs
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = [
Expand DownExpand Up@@ -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 '
Expand Down
Loading