Skip to content

Commit 4810dd6

Browse files
os-zhuangclaude
andauthored
fix(mcp): stdio bridge throws the shared RECORD_NOT_FOUND envelope (#8507)
* fix(mcp): stdio bridge throws the shared RECORD_NOT_FOUND envelope The stdio MCP bridge's update()/remove() by-id write seams minted their own bare Error on a missing id. The HTTP bridge's callData path already throws recordNotFoundError (code RECORD_NOT_FOUND, status 404) for the identical miss, so the two transports answered the same operation with two different envelopes. Also tightens check-engine-double-contract.mjs's consumer-seam invariant from "refuses at all" to SHARED_ONLY, now that all four seams reach the shared envelope. * chore: add changeset for the stdio not-found envelope fix Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7vaLs7bhBPi9m3JyzkhDj * fix(mcp): narrow the not-found test's catch typing, no TEST_DEBT drift The third test's `.catch((e) => e)` idiom inferred the settled value as `unknown` (TResult from an `any`-typed catch parameter widens to `unknown` here), which is a second, unrelated way to fail tsc from the same file that already had this idiom's cousin (`await res.json()`) in the package's 53-error TEST_DEBT baseline. Replaced it with the same explicit-cast helper the other two tests already used, extracted once as `catchError`. pnpm check:type-check-debt (the real ratchet command, not check:type-check-coverage) on a full built closure: OK, 33 ledger entries re-measured, none above their recorded number — @objectstack/mcp back to its recorded 53. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7vaLs7bhBPi9m3JyzkhDj --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9f5cc79 commit 4810dd6

4 files changed

Lines changed: 197 additions & 32 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
'@objectstack/mcp': patch
3+
---
4+
5+
fix(mcp): stdio bridge's `update`/`remove` throw the shared `RECORD_NOT_FOUND` envelope (#8422)
6+
7+
The stdio MCP bridge's `update()` and `remove()` — the two by-id write seams
8+
that probe for the row before mutating it — minted their own local
9+
`recordNotFound(object, id)`, returning a bare `Error` with neither `code`
10+
nor `status`. The HTTP bridge's `callData` path already throws
11+
`recordNotFoundError` (`code: 'RECORD_NOT_FOUND'`, `status: 404`,
12+
`@objectstack/core`, #4435/#5138/#7867) for the identical miss, so the same
13+
operation answered a missing id with two different envelopes depending on
14+
which MCP transport served it.
15+
16+
`packages/mcp/src/stdio-data-bridge.ts` now imports `recordNotFoundError`
17+
from `@objectstack/core` — a dependency the package already declares — and
18+
throws it from both seams instead. `registerObjectTools` still turns the
19+
throw into a tool error exactly as before; only the thrown object's shape
20+
changed. No exported symbol moves and no authorable metadata is affected, so
21+
this ships as a `patch`.
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #8422 — the stdio bridge's by-id write seams must throw the repo's ONE
5+
* not-found envelope (`recordNotFoundError`, `@objectstack/core`), not a
6+
* locally minted `Error`.
7+
*
8+
* The HTTP bridge routes its data verbs through `callData`, which throws
9+
* `recordNotFoundError` — `code: 'RECORD_NOT_FOUND'`, `status: 404`
10+
* (`packages/core/src/utils/record-not-found.ts`, #4435/#5138/#7867). The
11+
* stdio bridge minted its own bare `Error` for the identical miss, so a
12+
* stdio caller got a message with no machine-readable code and nothing that
13+
* maps to 404 — the same operation, two different envelopes depending on
14+
* which transport served it.
15+
*
16+
* Both by-id write seams are covered — `update()` and `remove()` — asserting
17+
* on `code` AND `status`, not merely that something threw: an assertion that
18+
* only checks for a thrown error would have passed against the bare `Error`
19+
* this card exists to remove.
20+
*/
21+
22+
import { describe, it, expect, vi } from 'vitest';
23+
import type { ExecutionContext } from '@objectstack/spec/kernel';
24+
import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts';
25+
import { createStdioDataBridge } from './stdio-data-bridge.js';
26+
27+
/** An engine that resolves NO row for any id — every by-id write is a miss. */
28+
function makeEmptyEngine() {
29+
return {
30+
find: vi.fn(async () => []),
31+
findOne: vi.fn(async () => null),
32+
insert: vi.fn(),
33+
update: vi.fn(),
34+
delete: vi.fn(),
35+
count: vi.fn(async () => 0),
36+
aggregate: vi.fn(async () => [{ n: 0 }]),
37+
};
38+
}
39+
40+
/** An object definition with no exposure restriction, so the miss is what refuses. */
41+
function makeMetadata() {
42+
return {
43+
listObjects: vi.fn(async () => [{ name: 'task', label: 'Task', fields: {} }]),
44+
getObject: vi.fn(async () => ({ name: 'task', label: 'Task', enable: { apiEnabled: true } })),
45+
get: vi.fn(async () => null),
46+
list: vi.fn(async () => []),
47+
exists: vi.fn(async () => true),
48+
getRegisteredTypes: vi.fn(async () => ['object']),
49+
register: vi.fn(),
50+
unregister: vi.fn(),
51+
};
52+
}
53+
54+
const PRINCIPAL = { userId: 'u1', isSystem: false } as unknown as ExecutionContext;
55+
56+
function makeBridge() {
57+
const engine = makeEmptyEngine();
58+
const metadataService = makeMetadata();
59+
const bridge = createStdioDataBridge({
60+
engine: engine as unknown as IDataEngine,
61+
metadataService: metadataService as unknown as IMetadataService,
62+
resolvePrincipal: async () => PRINCIPAL,
63+
});
64+
return { bridge, engine, metadataService };
65+
}
66+
67+
/** A rejection's payload, narrowed once so callers never juggle `unknown`. */
68+
type NotFoundEnvelope = Error & { code?: string; status?: number };
69+
70+
/**
71+
* The rejection `run` throws, typed — or `null` if it resolved. One explicit
72+
* cast, shared, rather than `.catch((e) => e)` at each call site: that idiom
73+
* infers the settled value as `unknown` here (TResult from an `any`-typed
74+
* catch parameter widens to `unknown`), which is a second, unrelated way to
75+
* fail `tsc` — this repo's TEST_DEBT ledger already carries 53 raw errors for
76+
* `packages/mcp` from the *other* half of that idiom (`await res.json()`),
77+
* and a fix for this card must not add a fourth without narrowing it.
78+
*/
79+
async function catchError(run: () => Promise<unknown>): Promise<NotFoundEnvelope | null> {
80+
return (await run().then(
81+
() => null,
82+
(e: unknown) => e,
83+
)) as NotFoundEnvelope | null;
84+
}
85+
86+
/**
87+
* Assert a not-found refusal by its ENVELOPE, not by the fact that something
88+
* threw — a bare `Error` also satisfies `.toThrow()`, which is exactly the
89+
* defect this card removes.
90+
*/
91+
async function expectRecordNotFound(run: () => Promise<unknown>): Promise<void> {
92+
const err = await catchError(run);
93+
expect(err, 'the call resolved — no not-found refusal was raised').toBeTruthy();
94+
expect(err!.code).toBe('RECORD_NOT_FOUND');
95+
expect(err!.status).toBe(404);
96+
}
97+
98+
describe('#8422 stdio bridge by-id writes throw the shared not-found envelope', () => {
99+
it('update() on a missing id throws RECORD_NOT_FOUND / 404', async () => {
100+
const { bridge, engine } = makeBridge();
101+
102+
await expectRecordNotFound(() => bridge.update('task', 'ghost', { title: 'x' }));
103+
// Refused before the write dispatched — the same existence-before-mutation
104+
// property the HTTP path (`callData`) holds.
105+
expect(engine.update).not.toHaveBeenCalled();
106+
});
107+
108+
it('remove() on a missing id throws RECORD_NOT_FOUND / 404', async () => {
109+
const { bridge, engine } = makeBridge();
110+
111+
await expectRecordNotFound(() => bridge.remove('task', 'ghost'));
112+
expect(engine.delete).not.toHaveBeenCalled();
113+
});
114+
115+
it('both seams throw the SAME envelope shape — one declaration, not two', async () => {
116+
const { bridge } = makeBridge();
117+
118+
const updateErr = await catchError(() => bridge.update('task', 'ghost', { title: 'x' }));
119+
const removeErr = await catchError(() => bridge.remove('task', 'ghost'));
120+
121+
expect(updateErr?.code).toBe(removeErr?.code);
122+
expect(updateErr?.status).toBe(removeErr?.status);
123+
expect(updateErr?.code).toBe('RECORD_NOT_FOUND');
124+
});
125+
});

‎packages/mcp/src/stdio-data-bridge.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,13 @@ import {
7878
} from '@objectstack/spec/data';
7979
import type { ExecutionContext } from '@objectstack/spec/kernel';
8080
import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts';
81+
// [#8422] The repo's ONE single-record 404 (#4435/#5138/#7867). Imported from
82+
// `@objectstack/core` rather than re-minted here or reached via
83+
// `@objectstack/metadata-protocol`'s re-export: this package already declares
84+
// a direct `@objectstack/core` dependency (`plugin.ts` imports from it too),
85+
// and `@objectstack/core` is the lowest package that carries the factory, so
86+
// there is no reason to add a second import path to the same function.
87+
import { recordNotFoundError } from '@objectstack/core';
8188
import type { McpDataBridge, McpObjectSummary } from './mcp-http-tools.js';
8289

8390
/** What {@link createStdioDataBridge} needs from the host plugin. */
@@ -132,18 +139,6 @@ async function findById(
132139
return unwrapRows(res)[0] ?? null;
133140
}
134141

135-
/**
136-
* The "this id names no row" refusal, raised BEFORE a write is attempted.
137-
*
138-
* A write path that answers success for an id that matched nothing is the
139-
* #5138 / #5581 defect the HTTP path already paid for: an integrator reading
140-
* a success receipt records the change as landed. `registerObjectTools` turns
141-
* a throw into a tool error, so the caller is told.
142-
*/
143-
function recordNotFound(object: string, id: string): Error {
144-
return new Error(`Record "${id}" not found in "${object}"`);
145-
}
146-
147142
/**
148143
* Bridge method → the `callData` action name the HTTP bridge gates it under.
149144
*
@@ -362,12 +357,21 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge
362357

363358
async update(object, id, data) {
364359
const context = await resolvePrincipal();
365-
// Before the existence probe, not after: `recordNotFound` vs. a hit is an
366-
// observable difference, so gating second would answer "that id names no
367-
// row" for an object the author declared unexposed.
360+
// Before the existence probe, not after: refusing on exposure vs. on a
361+
// miss is an observable difference, so gating second would answer "that
362+
// id names no row" for an object the author declared unexposed.
368363
await enforceApiExposure(metadataService, object, GATED_ACTIONS.update, context);
369364
const existing = await findById(engine, object, id, context);
370-
if (!existing) throw recordNotFound(object, id);
365+
// The "this id names no row" refusal, raised BEFORE the write is
366+
// attempted — a write path that answered success for an id that matched
367+
// nothing is the #5138/#5581 defect the HTTP path already paid for: an
368+
// integrator reading a success receipt records the change as landed.
369+
// `registerObjectTools` turns a throw into a tool error, so the caller
370+
// is told. [#8422] Throws the repo's ONE not-found envelope
371+
// (`recordNotFoundError`, `@objectstack/core`) rather than a bare
372+
// `Error`, so a stdio caller sees the same `RECORD_NOT_FOUND` / 404 the
373+
// HTTP bridge's `callData` path throws for the identical miss.
374+
if (!existing) throw recordNotFoundError(object, id);
371375
await engine.update(object, data, { where: { id }, context });
372376
const record = { ...(existing as Record<string, unknown>), ...data };
373377
// [#8497] The engine's update RESULT is deliberately discarded here (this
@@ -388,7 +392,8 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge
388392
// Gated before the probe, for the reason `update` states.
389393
await enforceApiExposure(metadataService, object, GATED_ACTIONS.remove, context);
390394
const existing = await findById(engine, object, id, context);
391-
if (!existing) throw recordNotFound(object, id);
395+
// Same shared envelope as `update`, above.
396+
if (!existing) throw recordNotFoundError(object, id);
392397
await engine.delete(object, { where: { id }, context });
393398
// `success`, not `deleted` — the spec's `DeleteDataResponse` key (#5581).
394399
return { object, id, success: true };

‎scripts/check-engine-double-contract.mjs‎

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -725,21 +725,28 @@ function scanSource(fileName, text, slice = SLICES[0]) {
725725
// #5138, #5581, #7867). A narrower gate that could not be written is not a
726726
// better gate than a wide one that can.
727727
//
728-
// ## Deliberately NOT asserted yet: WHICH not-found envelope (#8194)
728+
// ## WHICH not-found envelope (#8194, tightened to SHARED_ONLY by #8422)
729729
//
730-
// Three of the four seams reach `recordNotFoundError` -- the repo's ONE
731-
// not-found envelope (`@objectstack/core`, moved there by #7867 for exactly
732-
// the "two layers cannot disagree about it" reason its header argues). The
733-
// fourth, `packages/mcp/src/stdio-data-bridge.ts`, mints its own local
734-
// `recordNotFound` returning a bare `Error` with neither `code` nor `status`.
730+
// #8194 measured all four seams and found three reaching `recordNotFoundError`
731+
// -- the repo's ONE not-found envelope (`@objectstack/core`, moved there by
732+
// #7867 for exactly the "two layers cannot disagree about it" reason its
733+
// header argues) -- while the fourth, `packages/mcp/src/stdio-data-bridge.ts`,
734+
// minted its own local `recordNotFound` returning a bare `Error` with neither
735+
// `code` nor `status`.
735736
//
736-
// That is a real divergence and it is filed as its own card, not laundered
737-
// through a ledger entry here: this gate would have opened RED on a defect
738-
// outside the change that introduced the gate, which is the one way to teach
739-
// readers that a red run means "someone else's problem". So the verdict below
740-
// records WHICH envelope each seam reaches and prints it, and requiring the
741-
// shared one is a one-line tightening the day that card lands -- `SHARED_ONLY`
742-
// is the switch, and the seam list is already both-directions complete.
737+
// That was a real divergence and #8194 filed it as its own card rather than
738+
// laundering it through a ledger entry here: opening this gate RED on a
739+
// defect outside the change that introduced it would have taught readers that
740+
// a red run means "someone else's problem". So the verdict recorded WHICH
741+
// envelope each seam reached and printed it, with `refusal: 'local'` as the
742+
// visible-but-not-failing state -- deliberately not `!x.refusal` (that already
743+
// failed) and not silence either.
744+
//
745+
// #8422 fixed the fourth seam, so all four now reach the shared envelope --
746+
// the SHARED_ONLY tightening below is that one-line change, made the day the
747+
// seam list actually went both-directions complete. `refusal !== 'shared'`
748+
// now fails on EITHER a local mint or no refusal at all: a future fifth seam
749+
// that reinvents the envelope reddens here instead of shipping unnoticed.
743750

744751
/** Where the repo's ONE not-found envelope may be imported from (#7867). */
745752
const ENVELOPE_MODULES = [
@@ -1319,11 +1326,18 @@ function audit() {
13191326
);
13201327
}
13211328

1329+
// SHARED_ONLY (#8422): a seam must reach the shared envelope specifically --
1330+
// `refusal !== 'shared'` catches both a local mint (`refusal === 'local'`)
1331+
// and no refusal at all (`refusal === null`), so a seam that merely throws
1332+
// SOME error no longer reads as compliant.
13221333
for (const { file, seams } of seamFiles) {
1323-
for (const s of seams.filter((x) => !x.refusal)) {
1334+
for (const s of seams.filter((x) => x.refusal !== 'shared')) {
1335+
const state = s.refusal === 'local'
1336+
? 'refuses through a locally minted error rather than the shared envelope'
1337+
: 'does not refuse anywhere before it';
13241338
errors.push(
13251339
`REFUSES: ${file}:${s.line} — ${s.fn}() performs a by-id ${s.verb} on a caller-supplied id `
1326-
+ 'and then answers a success receipt, without refusing anywhere before it. A write that '
1340+
+ `and then answers a success receipt, and ${state}. A write that `
13271341
+ 'touched zero rows reporting success is the #4435/#5138/#7867 defect: a typo\'d id, an '
13281342
+ 'already-deleted row and a real write become indistinguishable, and an integrator '
13291343
+ 'reading the receipt records the change as landed. Refuse before you answer — probe '

0 commit comments

Comments
 (0)