Skip to content

Commit e3b3cdd

Browse files
huangyiireneclaude
andauthored
fix(metadata-protocol): /discovery derives transactionalBatch from engineCanRollBack, not the engine-only probe (#18997) (#19286)
Fixes #18997 Clause-②: no `getDiscovery()` derived `capabilities.transactionalBatch` from the ENGINE alone — `typeof this.engine?.transaction === 'function'` — while `runAtomicBatch` refuses `batchData({ atomic: true })` with `501 NOT_IMPLEMENTED` on `engineCanRollBack(this.engine)`, which asks the DEFAULT DRIVER as well. `engine.transaction` is a function on every real engine, so `/discovery` answered `true` for exactly the compositions that then 501 — and the 501's own remedy text sends the caller to that bit ("probe `capabilities.transactionalBatch` on /discovery first"). The prescribed remedy routed the caller to a signal that was wrong in the only case the remedy exists for. One line at the read site: the advertisement now asks the predicate the refusal already trusts. ## What a consumer that sees `true` today will see after this lands `/discovery` is a published surface, so this is stated rather than assumed. Two compositions stop advertising `true` and begin advertising `false`. Both already answered `501 NOT_IMPLEMENTED` to an atomic batch before this change, so **nothing that was accepted becomes refused** — only a lying value becomes honest: - **(a) a default driver with no `beginTransaction` at all** — pre-existing on `main`, not introduced by #18063; - **(b) a default driver that inherits `beginTransaction` and declares `supports.transactionsUnsupported`** — the population #18063 added. The shipped example is `TursoDriver` on its remote transport (`packages/drivers/driver-turso/src/turso-driver.ts`, `transactionsUnsupported: this.transportMode === 'remote'`). A client that read `true` and proceeded was taking the 501; it now reads `false` and takes its non-atomic fallback ahead of the failure, which is what probing the capability was for. A client that hard-asserts `transactionalBatch === true` at startup against such a composition now fails at startup instead of at its first atomic batch. The REST producer's own AND with `api.enableBatch` (`packages/rest/src/rest-server.ts`) is unchanged and inherits the corrected input. Everything else is byte-identical: the key, its type, its `description` and its place in the closed capability vocabulary do not move. ## Clause-② — measured, not argued The claim comment grades this `no` on the ground that it narrows. The falsifier was to be measured, not assumed: if any composition advertised `true` where `origin/main` advertises `false`, that would be a widening. Enumerated over all 16 compositions of the four inputs the two predicates can read (engine has `transaction`; driver registry inspectable; driver has `beginTransaction`; driver declares `transactionsUnsupported`), comparing `main`'s derivation with this one: ``` compositions: 16 | widened (false -> true): 0 | narrowed (true -> false): 3 VERDICT: NO WIDENING — accept set untouched, advertisement narrows only ``` The 3 narrowed rows are population (a) x2 (driver has no `beginTransaction`, with and without the declaration) and population (b) x1. `Clause-②: no` holds. ## Both directions, or the card is not delivered "A fix that makes the flag honest by making it always `false` is worse than the bug." So the TRUE arm is pinned beside the FALSE arm, and each composition is driven through `batchData({ atomic: true })` in the same test — the advertisement and the refusal have to agree composition by composition. `packages/metadata-protocol/src/protocol.discovery-transactional-batch-honesty.test.ts` (6 cases): - a default driver that CAN roll back: still `true`, and the atomic batch commits; - a host whose driver registry is not inspectable (test doubles, metadata-only hosts): still `true` — the engine-level probe is all there is, which is what `engineCanRollBack` documents; - population (a): `false`, and the 501 arrives with the remedy sentence this card is about, having attempted no write; - population (b): `false`, same refusal; - no `transaction()` at all: `false` (unchanged); - and the rule itself — the advertised bit equals `engineCanRollBack()` for every composition, so a future composition that satisfies one end and not the other reds here even if nobody adds a case for it. **Against the shipped driver**, measured on the built `dist/` after the fix, not inferred: ``` real MemoryDriver: typeof beginTransaction = function | supports.transactionsUnsupported = undefined | driverSupportsTransactions = true /discovery capabilities.transactionalBatch = {"enabled":true} ``` The default dev composition keeps its `true`. ## Evidence - **Pre-fix reproduction.** The pin file was written first and run unmodified against unmodified `protocol.ts`: `Tests 3 failed | 3 passed (6)` — the two population cases and the predicate-agreement case red, the TRUE-arm controls already green. Post-fix: `Test Files 1 passed (1) · Tests 6 passed (6)`. - **Ablation** through `scripts/ablation-replace.mjs`, from the committed state, with on-disk proof: anchor `1 -> 0`, blob `0b04d1ff5d3a -> 7c63e7631f05`, then `3 failed | 3 passed`; restore verified `blob == HEAD (0b04d1f)` and `git diff HEAD` empty. No `dist` preflight is owed: the pin imports `./protocol.js`, so the mutation is on the resolution path directly. - **Package suite** `pnpm --filter @objectstack/metadata-protocol test`: `Test Files 181 passed | 3 skipped (184) · Tests 2591 passed | 19 skipped (2610)`. `typecheck` exit 0, and `tsc --noEmit --listFiles` names the new pin file among the 750 files in the program, so the type check really covers it. - **Cross-package consumer** `packages/objectql/src/protocol-discovery.test.ts` against the rebuilt `metadata-protocol` dist: `32 passed (32)` — including the real-`ObjectQL`-engine pin that must keep advertising `true`. - **Gates**, derived from the actual diff with `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` and reconciled with `--ran`: 59 derived, **56 run, all exit 0**, 3 NOT MEASURED. The 3 exited `3` (PREREQUISITE NOT MET, nothing measured) because they read a whole-workspace `dist/` this checkout does not carry — `check:dual-build-cjs-loads`, `check:lean-entry-closure`, `check:type-check-debt`; CI's `Build Core` supplies the prerequisite they refuse without. `check:plugin-teardown-shape --self-test` also exited 3 at first (its fixture commit is unreachable in a shallow clone) and exits 0 after fetching that commit. - **`pnpm lint`** (repo-wide `eslint . --no-inline-config`): exit 0. All readings are from `objectstack-ai/objectstack` at this branch's head commit. ## Acceptance notes - `publishPackageDrafts` in the same file keeps an engine-only probe (`typeof this.engine?.transaction === 'function'`) for its own `inTxn` helper. Noted, not filed, and deliberately not changed here: it advertises nothing to a caller, and both of its branches deliver the same guarantee today — `engine.transaction()` takes its declared non-transactional path (ADR-0119 D1) for exactly the drivers the probe would have excluded, so swapping the predicate there is a no-op in behaviour. It would become a real question only for whoever makes draft-publish atomicity a promise rather than a best effort. - The runtime dispatcher's hardcoded `transactionalBatch: { enabled: false }` (`packages/runtime/src/http-dispatcher.ts`) is correct and untouched: that face mounts no `/batch` route. - Scope held to `packages/metadata-protocol/src/` plus the changeset. `packages/spec` is not touched — the spec half of #18063 is landed and correct. - `origin/main` moved two commits while this was in flight (`#19162` checklist-status workflow, `#19242` hono-server). Neither touches this package; the `dispatch-gates.mjs` change in that range is to its own self-test roster and derives the same 59 families for this diff. Not merged down; the merge queue rebuilds on current `main`. --- _Generated by [Claude Code](https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 801415a commit e3b3cdd

3 files changed

Lines changed: 237 additions & 8 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
`/discovery` advertises `capabilities.transactionalBatch` from the predicate the atomic-batch refusal already trusts, so the advertisement and the 501 stop disagreeing (#18997).
6+
7+
`getDiscovery()` derived the bit from the ENGINE alone — `typeof this.engine?.transaction === 'function'` — while `runAtomicBatch` refuses `batchData({ atomic: true })` with `501 NOT_IMPLEMENTED` on `engineCanRollBack(engine)`, which asks the DEFAULT DRIVER as well. `engine.transaction` is a function on every real engine, so the advertisement answered `true` for compositions that then 501 — and the 501's own remedy text sends the caller to that very bit ("probe `capabilities.transactionalBatch` on /discovery first"). The prescribed remedy routed the caller to a signal that was wrong in exactly the case the remedy exists for.
8+
9+
**What a consumer sees.** Two compositions, measured separately, stop advertising `true` and now advertise `false`:
10+
11+
- **(a) a default driver with no `beginTransaction` at all** — pre-existing, not introduced by #18063;
12+
- **(b) a default driver that inherits `beginTransaction` and declares `supports.transactionsUnsupported`** — the population #18063 added; the shipped example is `TursoDriver` on its remote transport.
13+
14+
Both already answered `501 NOT_IMPLEMENTED` to an atomic batch, so nothing that was accepted becomes refused. A client that read `true` and proceeded was taking the 501; it now reads `false` and takes its non-atomic fallback ahead of the failure — which is what probing the capability was for. A client that hard-asserts `transactionalBatch === true` at startup against such a composition fails at startup instead of at the first atomic batch.
15+
16+
Unchanged in the other direction, and pinned so that "honest" cannot decay into "always `false`": a composition whose default driver **can** roll back still advertises `true`, and so does a host whose driver registry is not inspectable (test doubles, metadata-only hosts), where the engine-level probe is all there is. Measured over all 16 compositions of the four inputs the two predicates read: 0 go `false` → `true`, 3 go `true` → `false`.
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#18997] `/discovery`'s `capabilities.transactionalBatch` answers the SAME
5+
* question the atomic-batch refusal asks — "can this runtime actually roll
6+
* back?" — and is derived from the SAME predicate (`engineCanRollBack`).
7+
*
8+
* The defect: `getDiscovery()` derived the bit from the ENGINE alone
9+
* (`typeof this.engine?.transaction === 'function'`), while `runAtomicBatch`
10+
* refuses with `501 NOT_IMPLEMENTED` on `engineCanRollBack(this.engine)`, which
11+
* also asks the DEFAULT DRIVER. `engine.transaction` is always a function on a
12+
* real engine, so a composition whose default driver cannot carry a transaction
13+
* advertised `transactionalBatch: true` and then answered 501 — and the 501's
14+
* own remedy text tells the caller to 「probe `capabilities.transactionalBatch`
15+
* on /discovery first」. The prescribed remedy routed the caller to a signal
16+
* that was wrong in exactly the case the remedy exists for.
17+
*
18+
* BOTH DIRECTIONS ARE PINNED ON PURPOSE. A fix that made the flag honest by
19+
* making it always `false` would be worse than the bug, and a pin that only
20+
* asserts the `false` arm cannot tell the two apart. So every composition below
21+
* states the expected bit AND is driven through `batchData({ atomic: true })`
22+
* in the same test: the advertisement and the refusal must agree, composition
23+
* by composition.
24+
*
25+
* The two `false` populations are measured SEPARATELY because they arrived
26+
* separately and could regress separately:
27+
* (a) a default driver with NO `beginTransaction` at all — pre-existing;
28+
* (b) a default driver that INHERITS `beginTransaction` but declares
29+
* `supports.transactionsUnsupported` (the live example is
30+
* `TursoDriver` on its remote transport) — the population #18063 added
31+
* when `engineCanRollBack` moved from a presence test to
32+
* `driverSupportsTransactions`.
33+
*/
34+
35+
import { describe, it, expect, vi } from 'vitest';
36+
import { engineCanRollBack } from '@objectstack/core';
37+
import { ObjectStackProtocolImplementation } from './protocol.js';
38+
39+
const SCHEMA = {
40+
name: 'invoice',
41+
fields: {
42+
title: { name: 'title', type: 'text' },
43+
},
44+
};
45+
46+
interface Composition {
47+
/** Does the engine expose `transaction()` at all? */
48+
hasTransaction?: boolean;
49+
/**
50+
* Is the driver registry inspectable? A metadata-only host or a test double
51+
* exposes no `getDefaultDriverName`, and then the engine-level probe is all
52+
* there is — `engineCanRollBack` says so in its own header.
53+
*/
54+
driverRegistryInspectable?: boolean;
55+
/** Population (a) when false: the default driver has no `beginTransaction`. */
56+
driverHasBeginTransaction?: boolean;
57+
/** Population (b) when true: `beginTransaction` inherited, transport denies it. */
58+
driverDeclaresUnsupported?: boolean;
59+
}
60+
61+
function makeEngine(c: Composition = {}) {
62+
const {
63+
hasTransaction = true,
64+
driverRegistryInspectable = true,
65+
driverHasBeginTransaction = true,
66+
driverDeclaresUnsupported = false,
67+
} = c;
68+
69+
const handle = { id: 'trx-1' };
70+
const commits: unknown[] = [];
71+
const insert = vi.fn(async (_object: string, data: any, _options?: any) => ({
72+
id: `rec-${insert.mock.calls.length}`,
73+
...data,
74+
}));
75+
76+
const driver: Record<string, unknown> = {
77+
supports: driverDeclaresUnsupported ? { transactionsUnsupported: true } : {},
78+
};
79+
if (driverHasBeginTransaction) driver.beginTransaction = async () => handle;
80+
81+
const engine: any = {
82+
registry: { getObject: () => SCHEMA, getRegisteredTypes: () => [] },
83+
insert,
84+
};
85+
if (driverRegistryInspectable) {
86+
engine.getDefaultDriverName = () => 'default';
87+
engine.getDriverByName = () => driver;
88+
}
89+
if (hasTransaction) {
90+
engine.transaction = vi.fn(async (callback: (ctx: any) => Promise<any>, baseContext?: any) => {
91+
const result = await callback({ ...(baseContext ?? {}), transaction: handle });
92+
commits.push(handle);
93+
return result;
94+
});
95+
}
96+
return { engine, insert, commits };
97+
}
98+
99+
async function advertisedBit(engine: any): Promise<boolean> {
100+
const discovery: any = await new ObjectStackProtocolImplementation(engine).getDiscovery();
101+
return discovery.capabilities.transactionalBatch.enabled;
102+
}
103+
104+
/** Drive the refusal path on the same composition. `null` = it did NOT refuse. */
105+
async function atomicBatchRefusal(engine: any, insert: { mock: { calls: unknown[] } }) {
106+
const p = new ObjectStackProtocolImplementation(engine);
107+
try {
108+
await p.batchData({
109+
object: 'invoice',
110+
request: {
111+
operation: 'create',
112+
records: [{ data: { title: 'A' } }],
113+
options: { atomic: true },
114+
},
115+
} as any);
116+
return null;
117+
} catch (err: any) {
118+
// A refusal writes nothing — the caller asked for all-or-nothing.
119+
expect(insert.mock.calls).toHaveLength(0);
120+
return { status: err?.status, code: err?.code, message: String(err?.message ?? '') };
121+
}
122+
}
123+
124+
describe('[#18997] /discovery advertises transactionalBatch iff the runtime can actually roll back', () => {
125+
// ── The TRUE arm — without it, "honest" would be indistinguishable from
126+
// "the capability is turned off", which the card calls worse than the bug.
127+
it('STILL advertises true when the default driver CAN roll back, and the atomic batch then runs', async () => {
128+
const t = makeEngine();
129+
130+
expect(await advertisedBit(t.engine)).toBe(true);
131+
expect(await atomicBatchRefusal(t.engine, t.insert)).toBeNull();
132+
expect(t.commits).toHaveLength(1);
133+
});
134+
135+
it('STILL advertises true for a host whose driver registry is not inspectable (test doubles, metadata-only hosts)', async () => {
136+
const t = makeEngine({ driverRegistryInspectable: false });
137+
138+
expect(await advertisedBit(t.engine)).toBe(true);
139+
expect(await atomicBatchRefusal(t.engine, t.insert)).toBeNull();
140+
});
141+
142+
// ── Population (a): pre-existing on `main`, not introduced by #18063.
143+
it('population (a): advertises FALSE when the default driver has no beginTransaction — the 501 population', async () => {
144+
const t = makeEngine({ driverHasBeginTransaction: false });
145+
146+
expect(await advertisedBit(t.engine)).toBe(false);
147+
148+
const refusal = await atomicBatchRefusal(t.engine, t.insert);
149+
expect(refusal).toMatchObject({ status: 501, code: 'NOT_IMPLEMENTED' });
150+
// The remedy text is the reason this card exists: it sends the caller to
151+
// the very bit this test pins, so the bit must be the one it meant.
152+
expect(refusal!.message).toContain('probe capabilities.transactionalBatch on /discovery first');
153+
});
154+
155+
// ── Population (b): the population #18063 added.
156+
it('population (b): advertises FALSE when the default driver declares supports.transactionsUnsupported', async () => {
157+
const t = makeEngine({ driverDeclaresUnsupported: true });
158+
159+
expect(await advertisedBit(t.engine)).toBe(false);
160+
expect(await atomicBatchRefusal(t.engine, t.insert)).toMatchObject({
161+
status: 501,
162+
code: 'NOT_IMPLEMENTED',
163+
});
164+
});
165+
166+
it('advertises FALSE when the engine exposes no transaction() at all (unchanged)', async () => {
167+
const t = makeEngine({ hasTransaction: false });
168+
169+
expect(await advertisedBit(t.engine)).toBe(false);
170+
expect(await atomicBatchRefusal(t.engine, t.insert)).toMatchObject({
171+
status: 501,
172+
code: 'NOT_IMPLEMENTED',
173+
});
174+
});
175+
176+
// ── The rule itself, stated once over every composition: ONE predicate,
177+
// two ends. A future composition that satisfies one end and not the
178+
// other reds here even if nobody thought to add a case above.
179+
it('the advertised bit equals engineCanRollBack() for every composition — one predicate, two ends', async () => {
180+
const compositions: Array<[string, Composition]> = [
181+
['driver can roll back', {}],
182+
['driver registry not inspectable', { driverRegistryInspectable: false }],
183+
['(a) driver has no beginTransaction', { driverHasBeginTransaction: false }],
184+
['(b) driver declares transactionsUnsupported', { driverDeclaresUnsupported: true }],
185+
['engine has no transaction()', { hasTransaction: false }],
186+
];
187+
188+
for (const [label, composition] of compositions) {
189+
const { engine } = makeEngine(composition);
190+
expect(
191+
{ [label]: await advertisedBit(engine) },
192+
).toEqual({ [label]: engineCanRollBack(engine) });
193+
}
194+
});
195+
});

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6323,14 +6323,32 @@ export class ObjectStackProtocolImplementation implements
63236323
// Atomic cross-object batch (#3298 / #1604 / ADR-0034 item 4): the
63246324
// REST /batch endpoint runs its ops inside `engine.transaction()`,
63256325
// which only opens a real (all-or-nothing) transaction when the
6326-
// engine exposes one — otherwise it degrades to a non-atomic
6327-
// passthrough. Advertise the capability iff the runtime engine can
6328-
// honour a transaction, so `declared === enforced` (Prime Directive
6329-
// #10). The rest-server producer ANDs this with `api.enableBatch` so
6330-
// a server that doesn't mount the route reports `false` at its layer.
6331-
// (ADR-0119 D1: `transaction` is contract-declared, so this probe
6332-
// no longer needs a structural cast to ask the question.)
6333-
transactionalBatch: typeof this.engine?.transaction === 'function',
6326+
// DEFAULT DRIVER can carry one — otherwise it takes its declared
6327+
// non-transactional path (ADR-0119 D1) and the batch degrades to a
6328+
// non-atomic passthrough. Advertise the capability iff the runtime
6329+
// can actually roll back, so `declared === enforced` (Prime
6330+
// Directive #10). The rest-server producer ANDs this with
6331+
// `api.enableBatch` so a server that doesn't mount the route reports
6332+
// `false` at its layer.
6333+
//
6334+
// [#18997] `engineCanRollBack`, NOT `typeof this.engine?.transaction
6335+
// === 'function'`. The refusal this advertisement exists to help a
6336+
// caller avoid — `runAtomicBatch`'s `501 NOT_IMPLEMENTED`, whose own
6337+
// remedy text says to probe `capabilities.transactionalBatch` on
6338+
// /discovery first — already asks `engineCanRollBack`, which asks the
6339+
// DRIVER as well as the engine. `engine.transaction` is a function on
6340+
// every real engine, so the engine-only probe answered `true` for the
6341+
// two compositions that 501: a default driver with no
6342+
// `beginTransaction` at all, and one that INHERITED it and declared
6343+
// `supports.transactionsUnsupported` (#18063). An advertised
6344+
// capability must answer the same question the refusal path asks,
6345+
// from the same predicate — two derivations of one capability is how
6346+
// these drifted. Narrowing only: this predicate is the engine probe
6347+
// AND a driver clause, so no composition newly advertises `true`
6348+
// (`protocol.discovery-transactional-batch-honesty.test.ts` pins both
6349+
// directions, and the driver clause is skipped where the registry is
6350+
// not inspectable, so a test double keeps its old answer).
6351+
transactionalBatch: engineCanRollBack(this.engine),
63346352

63356353
// ── Joined the vocabulary with ruling A (#5672) ───────────────────
63366354
// These six used to be the runtime dispatcher's half of the split.

0 commit comments

Comments
 (0)