From 7878c4c90275c6511aa91e1fa71f10b18ecf5229 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 05:58:01 +0000 Subject: [PATCH 1/5] fix(metadata-protocol): discriminate four read seams that answered a failed read from an empty accumulator (#8896) --- packages/metadata-protocol/src/protocol.ts | 155 ++++++++++++++++-- packages/metadata-protocol/src/seed-loader.ts | 42 ++++- 2 files changed, 177 insertions(+), 20 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 096bebc41b..e95f8b137e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -8941,6 +8941,16 @@ export class ObjectStackProtocolImplementation implements * * RBAC/RLS is enforced by forwarding the caller's `context` to * `engine.find` so users only see records they are entitled to read. + * + * ## [#8896] A swept object that could not be READ fails the search + * + * `totalObjects` / `totalHits` / `truncated` describe a COMPLETE sweep of + * the objects that were in scope, so an object whose read failed may not + * simply be dropped out of them — that hands the caller a partial scan + * wearing a whole one's numbers. The single benign failure is an object + * whose table was never provisioned (`isMissingTableError`): it can hold no + * rows, so "no hits from here" is the truth and it is skipped exactly as + * before. Every other read failure propagates. See the `catch` below. */ async searchAll(request: { q: string; @@ -9151,9 +9161,42 @@ export class ObjectStackProtocolImplementation implements record: row, }); } - } catch { - // RBAC denial or driver hiccup — skip silently per object - continue; + } catch (error) { + // [#8896] Discriminate by error TYPE. The bare `catch { continue }` + // this replaces skipped the object on ANY failure while the + // response below kept reporting `totalObjects`, `totalHits` and + // `truncated` as though the sweep had been complete — so a + // caller was handed a partial scan labelled as a whole one, and + // "not found" was invented for objects that were never read + // (ADR-0110 D3). + // + // Benign: the object is REGISTERED but its table was never + // provisioned (schema sync not run yet). It can hold no rows, so + // contributing no hits IS the truth and skipping it is correct — + // and this case is routine, since the registry lists every + // declared object whether or not a given deployment provisioned + // it. Asked through the shared `isMissingTableError` predicate + // (`@objectstack/metadata/errors`, #4825), never a hand-rolled + // code test. + // + // Everything else propagates: a connection drop, a timeout, a + // query error or a refused datasource all mean the object's rows + // may well match and simply were not seen. + // + // The comment this replaces named "RBAC denial" as a benign + // reason. Measured on this tree, that is not a failure mode of + // this seam: object-level authorization is enforced at the REST + // door (`enforceAuth`) BEFORE `searchAll` is reached, and + // row-level security narrows `find`'s result set rather than + // throwing. Nothing in-repo registers a `beforeFind` hook that + // denies by throwing. Were one added, the ruling for this family + // still applies: a read that could not run must not be answered + // "there are no matches here". + // + // No new response field and no new error code — the caller + // receives the read's own failure, envelope intact. + if (isMissingTableError(error)) continue; + throw error; } } @@ -14138,8 +14181,32 @@ export class ObjectStackProtocolImplementation implements // ADR-0067 — capture each artifact's PRE-publish state so this turn can // be recorded as ONE revertible commit. existedBefore=false → the commit // creates it (revert = soft-remove); true → it edits an existing artifact - // (revert = restoreVersion(prevVersion)). Best-effort: a capture failure - // just omits that item from the revert plan, never blocks the publish. + // (revert = restoreVersion(prevVersion)). + // + // [#8896] This paragraph used to end "Best-effort: a capture failure + // just omits that item from the revert plan, never blocks the publish." + // Both halves were wrong, and they were wrong in different directions — + // the comment described the code inaccurately, AND the behaviour it + // described would not have been correct either: + // + // * The code never omitted. It pushed a FABRICATED entry — + // `existedBefore: false, prevVersion: null` — i.e. the literal + // opposite of the healthy branch's `existedBefore: !!activeRow` for + // any artifact that did exist. `existedBefore: false` means "revert + // = soft-remove", so reverting this commit DELETES an artifact + // whose previous version was supposed to be restored. A read that + // failed was answered with a value, and the value chosen was the + // destructive one. + // * Omitting would not have been the fix. An item missing from the + // plan is simply not reverted, so the revert silently leaves the + // newly published version live while reporting the turn undone. + // + // Both are the same defect underneath: a revert plan derived from a + // read that did not happen. So the capture is discriminated by error + // TYPE instead, per the maintainer's ruling for this family — + // unprovisioned is truthful emptiness, everything else surfaces. See + // the `catch` below for why the benign branch keeps exactly the push + // that was wrong unconditionally. const commitItems: Array<{ type: string; name: string; existedBefore: boolean; prevVersion: number | null }> = []; for (const d of ordered) { try { @@ -14156,7 +14223,28 @@ export class ObjectStackProtocolImplementation implements existedBefore: !!activeRow, prevVersion: activeRow && typeof activeRow.version === 'number' ? activeRow.version : null, }); - } catch { + } catch (error) { + // [#8896] Benign: `sys_metadata` has not been provisioned yet. + // There is then genuinely no active row for anything, so + // `existedBefore: false, prevVersion: null` IS this artifact's + // pre-publish state and the revert plan it produces (revert = + // soft-remove) is correct. That is the ONE case in which the + // unconditional push above was ever right, and it is kept + // byte-for-byte. Asked through the shared `isMissingTableError` + // predicate (`@objectstack/metadata/errors`, #4825). + // + // Everything else — a connection drop, a timeout, a query + // error — means an active row may well exist and simply was not + // seen, so no honest `existedBefore` can be computed. It + // propagates, and the position of this loop is what makes that + // safe: the capture pass runs BEFORE Phase 1's transaction, so + // nothing has been written yet and the publish fails having + // changed nothing. Refusing to publish beats publishing with a + // revert plan that would delete an artifact on the way back. + // + // No new error code and no new response field: the caller + // receives the read's own failure, envelope intact. + if (!isMissingTableError(error)) throw error; commitItems.push({ type: d.type, name: d.name, existedBefore: false, prevVersion: null }); } } @@ -17409,8 +17497,16 @@ export class ObjectStackProtocolImplementation implements * type-narrowing). * * Coverage is driven by the hand-curated {@link REFERENCE_PATHS} - * registry. Types not present in the registry simply return no hits - * — the engine never throws. + * registry. A target type not present in the registry, and a SOURCE type + * this deployment does not declare, both simply produce no hits — neither + * is an error. + * + * [#8896] A source type that could not be READ is a different fact and is + * no longer answered the same way. This list is what an admin consults + * before a rename / delete / type-narrowing, so a silently short answer + * reads as "nothing depends on this" and licenses the destructive action. + * A `sys_metadata` outage therefore propagates as the 503 `getMetaItems` + * already raises for it, rather than being swallowed per matcher. */ async findReferencesToMeta(request: { type: string; @@ -17438,16 +17534,39 @@ export class ObjectStackProtocolImplementation implements // Walk distinct source types in parallel. await Promise.all( matchers.map(async (matcher) => { - let items: unknown[] = []; - try { - const result = await this.getMetaItems({ - type: matcher.fromType, - ...(request.organizationId ? { organizationId: request.organizationId } : {}), - }); - items = (result?.items ?? []) as unknown[]; - } catch { - return; - } + // [#8896] NO `catch` here, deliberately — the discrimination + // this seam owes already happened one layer down, and the + // `catch { return; }` that used to sit here threw it away. + // + // `getMetaItems` classifies its own read failures through + // {@link rethrowUnlessMetadataStoreUnprovisioned}: the one + // benign reason (`sys_metadata` not provisioned yet) returns + // normally with whatever the registry holds, and every other + // failure becomes a 503 `SERVICE_UNAVAILABLE` carrying the + // driver error as `cause` (#5532). A source type absent from + // this deployment is not an error at all — `listItems` answers + // `[]`. So the only thing the old `catch` could swallow was the + // 503 raised on purpose immediately below it, and swallowing it + // silently shrank `out[]`. + // + // That is the worst place in this file to shrink an accumulator + // quietly: this answer drives "what would break if I delete + // this", rendered as the admin UI's "Used by" panel before a + // rename / delete / type-narrowing. An empty or short list is + // read as "nothing depends on it — safe to remove", so a source + // type whose read failed turned an unanswerable question into a + // green light for a destructive action (ADR-0110 D3). + // + // `Promise.all` is what makes propagation right-shaped here: the + // first rejection rejects the whole scan, so no half-scanned + // `references` array can reach a caller. No new response field + // and no new error code — the 503 the store read already raised + // is what surfaces. + const result = await this.getMetaItems({ + type: matcher.fromType, + ...(request.organizationId ? { organizationId: request.organizationId } : {}), + }); + const items = (result?.items ?? []) as unknown[]; for (const raw of items) { if (!raw || typeof raw !== 'object') continue; const sourceName = (raw as any).name as string | undefined; diff --git a/packages/metadata-protocol/src/seed-loader.ts b/packages/metadata-protocol/src/seed-loader.ts index ab907d251d..a3de815853 100644 --- a/packages/metadata-protocol/src/seed-loader.ts +++ b/packages/metadata-protocol/src/seed-loader.ts @@ -21,6 +21,11 @@ import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteR // both dispatcher error exits use. Imported rather than re-spelled so the seed // channel and the HTTP boundaries cannot drift about what counts as one. import { validationFailureDetails } from '@objectstack/types'; +// [#8896] The platform's ONE answer to "did this READ fail because the table +// has not been provisioned yet?" — the same predicate `DatabaseLoader` (#5108), +// `SysMetadataRepository` (#4867) and `cascadeDeleteRelations` (#8895) ask, so +// a driver quirk is taught to the platform once instead of per seam. +import { isMissingTableError } from '@objectstack/metadata/errors'; interface Logger { info(message: string, meta?: Record): void; @@ -2055,8 +2060,41 @@ export class SeedLoaderService implements ISeedLoaderService { map.set(key, record); } } - } catch { - // Object may not have records yet + } catch (error) { + // [#8896] Discriminate by error TYPE. This map is not a convenience — + // it IS the decision, in all three of its callers, and an empty map is + // the answer that means "write these rows": + // + // 1. the upsert/update/ignore pre-load above: an unmatched key is + // written as a new row, so a failed read turns every update into an + // INSERT — the duplicate-row outcome, against a table whose rows + // were simply not seen; + // 2. `writeBatchPartial`'s `attempt > 1` recheck: `bulkWrite` is + // at-least-once, so a batch may have COMMITTED before its response + // was lost (framework#3149). The recheck is the only thing standing + // between that retry and a duplicate of every row it already wrote, + // and an empty map disarms it silently; + // 3. `writeOne`'s per-row form of the same recheck, on the degradation + // path. + // + // The bare `catch {}` this replaces reached all three with "there are no + // existing rows" no matter WHY the read failed — a connection drop, a + // timeout, a permission denial, a query error — which is ADR-0110 D3's + // exact shape: "the read found nothing" and "the read could not run" are + // different facts, and here they have opposite consequences. + // + // Benign, unchanged: the object's TABLE has not been provisioned yet + // (schema sync has not run — the seed's own write provisions it). It can + // hold no rows, so an empty map IS the truth and every caller's "write + // it" verdict is correct. Note the swallowed comment named a case that + // cannot reach here at all: an object that merely "may not have records + // yet" answers `find` with `[]`, it does not throw. + // + // Everything else propagates: the loader stops rather than computing a + // write plan from data it never read. No new error code and no new + // result field — the caller receives the read's own failure, envelope + // intact, and the seed's existing error accounting reports it. + if (!isMissingTableError(error)) throw error; } return map; } From 67e39e01b34742cb03082a6bc723d72beb2e3c1a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 07:13:32 +0000 Subject: [PATCH 2/5] test(metadata-protocol): pin the four read seams against a failed read answered from an empty accumulator (#8896) --- ...d-seam-empty-accumulator-discrimination.md | 62 +++ ...ublish-commit-capture-read-failure.test.ts | 385 ++++++++++++++++++ ...otocol.read-seam-empty-accumulator.test.ts | 314 ++++++++++++++ ...ader-existing-records-read-failure.test.ts | 347 ++++++++++++++++ 4 files changed, 1108 insertions(+) create mode 100644 .changeset/read-seam-empty-accumulator-discrimination.md create mode 100644 packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts create mode 100644 packages/metadata-protocol/src/protocol.read-seam-empty-accumulator.test.ts create mode 100644 packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts diff --git a/.changeset/read-seam-empty-accumulator-discrimination.md b/.changeset/read-seam-empty-accumulator-discrimination.md new file mode 100644 index 0000000000..d421a924d6 --- /dev/null +++ b/.changeset/read-seam-empty-accumulator-discrimination.md @@ -0,0 +1,62 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): four read seams that FAILED no longer answer out of an empty accumulator — only an unprovisioned table is read as truthful emptiness (#8896) + +Four reads in `@objectstack/metadata-protocol` sat behind a bare `catch` that +fell through — or, in one case, jumped — above a value the read was supposed to +fill. Each handed its caller an answer indistinguishable from a legitimate one, +with nothing logged and no field saying the answer was incomplete. Per ADR-0110 +D3 those are different facts, and at every one of these seams they have opposite +consequences: + +- **`SeedLoaderService.loadExistingRecords()`** returned an empty `Map`. That map + is not a cache — it IS the write decision, in all three of its callers, and + "empty" means *write these rows*: the upsert pre-load turns every update into + an INSERT, and `bulkWrite`'s `attempt > 1` recheck — the only thing standing + between an at-least-once retry and a duplicate of every row the first attempt + already committed (framework#3149) — is silently disarmed. +- **`searchAll()`** skipped the object on a per-object `catch { continue; }` + while the response still reported `totalObjects` / `totalHits` / `truncated` + as though the sweep had been complete: a partial scan wearing a whole one's + numbers. +- **`findReferencesToMeta()`** dropped a whole source type on a per-matcher + `catch { return; }`. That list answers "what would break if I delete this" and + is rendered as the admin UI's "Used by" panel, so a silently short list reads + as "nothing depends on it — safe to remove". +- **`publishPackageDrafts()`** did not fall through: it pushed a **fabricated** + ADR-0067 revert-plan entry, `{ existedBefore: false, prevVersion: null }` — + the literal opposite of the healthy branch's `existedBefore: !!activeRow`. + `existedBefore: false` means "revert = soft-remove", so reverting that commit + DELETES an artifact whose previous version was supposed to be restored. + +None of the four `catch`es is removed; each is **discriminated by error type**, +through the same shared `isMissingTableError` predicate +(`@objectstack/metadata/errors`) that `DatabaseLoader`, `SysMetadataRepository` +and `cascadeDeleteRelations` already use: + +- **benign, unchanged** — the table was never provisioned (schema sync not run + yet). It can hold no rows, so the empty answer is the truth and each seam + behaves exactly as before: the seed writes its rows, the search skips the + object, the publish records `existedBefore: false`. +- **everything else now surfaces** — a connection drop, a timeout, a permission + denial, a query error, a missing column on a provisioned table. The caller + receives the read's own failure, envelope intact. + +`findReferencesToMeta` is the one seam that gets no predicate of its own: it +reads through `getMetaItems`, which already performs exactly this discrimination +(`rethrowUnlessMetadataStoreUnprovisioned`, #5532) and raises a 503 +`SERVICE_UNAVAILABLE` for a real outage. The only thing its `catch` could +swallow was that deliberate 503, so it is simply gone. + +No new error code and no new response field. The behavioural change is that a +seed load, a global search, a reference scan or a package publish which used to +report success over an unreadable store now reports the failure that made it +unreadable. `publishPackageDrafts` refuses before Phase 1's transaction, so a +refused publish leaves the draft pending and writes nothing. + +The comment above the publish capture claimed a capture failure "just omits that +item from the revert plan". That was wrong twice — the code fabricated rather +than omitted, and omitting would have left the item unreverted while reporting +the turn undone — and it now describes what the code does. diff --git a/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts b/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts new file mode 100644 index 0000000000..8e51e068a7 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts @@ -0,0 +1,385 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8896] `publishPackageDrafts` must not FABRICATE a revert-plan entry when + * the pre-publish capture read fails. + * + * Before promoting anything, the publish captures each artifact's pre-publish + * state so the turn can be recorded as one revertible ADR-0067 commit: + * `existedBefore: false` → the commit CREATED it, so revert = soft-remove; + * `true` → it edited an existing artifact, so revert = restoreVersion. + * + * That capture sat behind a bare `catch` which pushed + * `{ existedBefore: false, prevVersion: null }` — the literal opposite of the + * healthy branch's `existedBefore: !!activeRow` for any artifact that DID + * exist. So a read that failed was answered with a value, and the value chosen + * was the destructive one: reverting that commit DELETES an artifact whose + * previous version was supposed to be restored. + * + * ## The comment/code contradiction the card asks about, decided + * + * The comment five lines above read "Best-effort: a capture failure just omits + * that item from the revert plan, never blocks the publish." Both halves were + * wrong, in different directions, and the fix answers both: + * + * - the CODE never omitted — it fabricated (see above); + * - and OMITTING would not have been correct either: an item missing from the + * plan is simply not reverted, so the revert silently leaves the newly + * published version live while reporting the turn undone. + * + * Both are the same defect underneath — a revert plan derived from a read that + * did not happen — so neither the comment nor the code was the survivor. The + * capture is discriminated by error TYPE instead, and the comment now describes + * that. + * + * ## Why refusing the publish is the safe direction here + * + * The capture pass runs BEFORE Phase 1's transaction, so a throw leaves nothing + * written: the draft stays a draft, no active row appears, no commit is + * recorded. Refusing to publish beats publishing with a revert plan that would + * delete an artifact on the way back. + * + * Every expectation below is written against LITERALS — the exact injected + * error object, its literal message and code, the literal `existedBefore` / + * `prevVersion` values read out of the stored commit row, literal row states. + * The failure cases are paired with positive controls in the same file: a + * capture that RUNS over a first publish (`existedBefore: false` is then the + * TRUTH) and over a second publish (`existedBefore: true, prevVersion: 2`) — + * the value the fabricated entry destroyed — plus proof, in the benign case, + * that the injected throw actually fired. + * + * The reproduction is built on the post-#8986 tree: seam 4 now sits downstream + * of that card's pre-flight gates, so the fixture publishes through them (no + * declared package namespace → the ADR-0028 prefix gate is grandfathered) and + * the capture loop is reached the way a real publish reaches it. + */ + +import { describe, it, expect } from 'vitest'; +// [#5619] The producer's OWN write-verb dispatch decisions, so the fake engine +// cannot accept a call ObjectQL refuses. From `@objectstack/metadata-core`, not +// `@objectstack/objectql` — objectql depends on THIS package, so that import +// would close a dependency cycle turbo rejects outright. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; + state: string; + metadata: string; + version?: number; +} + +interface CommitItem { + type: string; + name: string; + existedBefore: boolean; + prevVersion: number | null; +} + +/** ADR-0048 overlay key — `(type, name, org, state, package)`. */ +const keyOf = (w: Record) => + `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`; + +function matchesWhere(row: Record, where: Record): boolean { + for (const [k, v] of Object.entries(where)) { + if (k === '$or') { + if (!(v as Array>).some((c) => matchesWhere(row, c))) return false; + continue; + } + // `undefined` = "dimension not constrained"; `null` = "must be NULL". + if (v === undefined) continue; + if (row[k] !== v) return false; + } + return true; +} + +/** + * Stub engine + a ONE-SHOT injector on the pre-publish capture read. + * + * The capture is `findOne('sys_metadata', { where: { …, state: 'active' } })` + * and it is the FIRST such read a publish makes. The injector is one-shot so + * only the capture fails: the promote path's own reads run normally, which is + * what keeps the assertions about "nothing was written" attributable to this + * seam and not to a generally broken engine. + */ +function makeStubEngine() { + const rows = new Map(); + const sideTables: Record>> = {}; + const activeReads: Array> = []; + let nextId = 0; + let captureFailure: unknown = null; + let captureFailureFired = false; + + const findRow = (w: Record): { key: string; row: Row } | null => { + if (w.id !== undefined) { + for (const [k, r] of rows) if (r.id === w.id) return { key: k, row: r }; + return null; + } + if (w.package_id !== undefined) { + const k = keyOf(w); + const r = rows.get(k); + return r ? { key: k, row: r } : null; + } + for (const [k, r] of rows) if (matchesWhere(r as unknown as Record, w)) return { key: k, row: r }; + return null; + }; + + const engine = { + async findOne(table: string, opts: { where: Record }) { + if (table === 'sys_metadata' && opts?.where?.state === 'active') { + activeReads.push(opts.where); + if (captureFailure !== null) { + const err = captureFailure; + captureFailure = null; + captureFailureFired = true; + throw err; + } + } + if (table !== 'sys_metadata') { + return (sideTables[table] ?? []).find((r) => matchesWhere(r, opts.where)) ?? null; + } + return findRow(opts.where)?.row ?? null; + }, + async find(table: string, opts?: { where?: Record }) { + const where = opts?.where ?? {}; + if (table !== 'sys_metadata') { + return (sideTables[table] ?? []).filter((r) => matchesWhere(r, where)); + } + return Array.from(rows.values()) + .filter((r) => matchesWhere(r as unknown as Record, where)); + }, + async insert(table: string, data: Record) { + nextId += 1; + if (table !== 'sys_metadata') { + (sideTables[table] ??= []).push({ id: `x_${nextId}`, ...data }); + return { id: `x_${nextId}` }; + } + const row = { id: `r_${nextId}`, ...data } as unknown as Row; + rows.set(keyOf(data), row); + return { id: row.id }; + }, + async update(_t: string, data: Record, opts: { where: Record }) { + assertEngineUpdateDispatch(data, opts); + const found = findRow(opts.where); + if (!found) return { id: null }; + const merged = { ...found.row, ...data } as unknown as Row; + rows.delete(found.key); + rows.set(keyOf(merged as unknown as Record), merged); + return { id: found.row.id }; + }, + async delete(_t: string, opts: { where: Record }) { + assertEngineDeleteDispatch(opts); + const found = findRow(opts.where); + if (!found) return { deleted: 0 }; + rows.delete(found.key); + return { deleted: 1 }; + }, + async transaction(cb: (ctx: undefined, info: { owned: boolean }) => Promise): Promise { + return cb(undefined, { owned: true }); + }, + registry: { + registerItem: () => {}, + registerObject: () => {}, + // No declared package namespace → the ADR-0028 prefix pre-flight is + // grandfathered, exactly as for a legacy package. + getPackage: () => undefined, + }, + }; + + return { + engine, + rows, + sideTables, + activeReads, + armCaptureFailure: (error: unknown) => { captureFailure = error; captureFailureFired = false; }, + captureFailureFired: () => captureFailureFired, + }; +} + +/** [#8308] Authored OWD — the publish gate refuses an OWD-less custom object. */ +const objectBody = (name: string, label: string) => ({ + name, + label, + sharingModel: 'private', + fields: { title: { type: 'text', label: 'Title' } }, +}); + +/** The revert plan as it was actually STORED, parsed from the commit row. */ +const storedCommitItems = (sideTables: Record>>): CommitItem[][] => + (sideTables['sys_metadata_commit'] ?? []).map((c) => JSON.parse(String(c.items)) as CommitItem[]); + +const rowsInState = (rows: Map, state: string) => + Array.from(rows.values()).filter((r) => r.state === state); + +/** The real driver phrasings, verbatim. */ +const connectionDropped = () => + Object.assign(new Error('connection terminated unexpectedly'), { code: 'ECONNRESET' }); + +async function rejection(run: () => Promise): Promise & { message?: string }> { + let caught: unknown; + let resolved: unknown; + let didResolve = false; + try { + resolved = await run(); + didResolve = true; + } catch (e) { + caught = e; + } + expect( + didResolve, + `expected a rejection, but the publish resolved with ${JSON.stringify(resolved)}`, + ).toBe(false); + return caught as Record & { message?: string }; +} + +describe('[#8896] publishPackageDrafts — a failed pre-publish capture must not invent a revert plan', () => { + // ── POSITIVE CONTROLS — the capture RUNS, so both of its real answers are + // observable here. Without these, the refusal below could pass on a + // harness that no longer records a commit at all. + + it('control: a capture that RUNS over a NEW artifact records existedBefore=false truthfully', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + const res = await protocol.publishPackageDrafts({ packageId: 'app.demo' }); + + expect(res.publishedCount).toBe(1); + expect(storedCommitItems(stub.sideTables)).toEqual([ + [{ type: 'object', name: 'solo_ticket', existedBefore: false, prevVersion: null }], + ]); + }); + + it('control: a capture that RUNS over an EXISTING artifact records existedBefore=true and its prevVersion', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + await protocol.publishPackageDrafts({ packageId: 'app.demo' }); + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v2'), + packageId: 'app.demo', mode: 'draft', + } as never); + await protocol.publishPackageDrafts({ packageId: 'app.demo' }); + + // THIS is the value the fabricated entry destroyed: the second commit + // must revert by RESTORING version 2, not by soft-removing the object. + expect(storedCommitItems(stub.sideTables)[1]).toEqual([ + { type: 'object', name: 'solo_ticket', existedBefore: true, prevVersion: 2 }, + ]); + }); + + // ── THE FIX — a capture that could not run must surface, not invent. + + it('a capture read that FAILS refuses the publish and writes nothing', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + + const injected = connectionDropped(); + stub.armCaptureFailure(injected); + + const caught = await rejection(() => protocol.publishPackageDrafts({ packageId: 'app.demo' })); + + // The caller receives the READ's own failure, envelope intact — no new + // error code and no new response field. + expect(caught).toBe(injected); + expect(caught.message).toBe('connection terminated unexpectedly'); + expect(caught.code).toBe('ECONNRESET'); + // Proof the capture really ran and really threw. + expect(stub.captureFailureFired()).toBe(true); + + // The capture pass runs BEFORE Phase 1's transaction, so a refusal + // leaves the world untouched: the draft is still a draft, no active row + // appeared, and no commit was recorded. + expect(rowsInState(stub.rows, 'draft').map((r) => r.name)).toEqual(['solo_ticket']); + expect(rowsInState(stub.rows, 'active')).toEqual([]); + expect(storedCommitItems(stub.sideTables)).toEqual([]); + // Pre-fix this published successfully and stored + // `existedBefore: false` — a revert plan that soft-REMOVES an artifact + // whose previous version was never read. + }); + + it('a missing COLUMN on a provisioned sys_metadata stays loud (the superstring case)', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + + const injected = Object.assign( + new Error('column "version" of relation "sys_metadata" does not exist'), + { code: '42703' }, + ); + stub.armCaptureFailure(injected); + + const caught = await rejection(() => protocol.publishPackageDrafts({ packageId: 'app.demo' })); + + expect(caught).toBe(injected); + expect(caught.message).toBe('column "version" of relation "sys_metadata" does not exist'); + expect(storedCommitItems(stub.sideTables)).toEqual([]); + }); + + // ── THE ONE BENIGN CASE — with `sys_metadata` unprovisioned there is + // genuinely no active row for anything, so `existedBefore: false` IS the + // artifact's pre-publish state and the push is kept byte-for-byte. + + it('an UNPROVISIONED sys_metadata is truthful emptiness: the publish proceeds with existedBefore=false', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + stub.armCaptureFailure( + Object.assign(new Error('SQLITE_ERROR: no such table: sys_metadata'), { code: 'SQLITE_ERROR' }), + ); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.demo' }); + + expect(res.success).toBe(true); + expect(res.publishedCount).toBe(1); + expect(storedCommitItems(stub.sideTables)).toEqual([ + [{ type: 'object', name: 'solo_ticket', existedBefore: false, prevVersion: null }], + ]); + // Proof the benign branch was actually EXERCISED — the capture ran and + // threw. Without this, the passing publish above would be consistent + // with a harness in which the injector never fired at all. + expect(stub.captureFailureFired()).toBe(true); + }); + + it('an UNPROVISIONED sys_metadata in the postgres phrasing (42P01) is benign too', async () => { + const stub = makeStubEngine(); + const protocol = new ObjectStackProtocolImplementation(stub.engine as never); + + await protocol.saveMetaItem({ + type: 'object', name: 'solo_ticket', item: objectBody('solo_ticket', 'v1'), + packageId: 'app.demo', mode: 'draft', + } as never); + stub.armCaptureFailure( + Object.assign(new Error('relation "sys_metadata" does not exist'), { code: '42P01' }), + ); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.demo' }); + + expect(res.publishedCount).toBe(1); + expect(stub.captureFailureFired()).toBe(true); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.read-seam-empty-accumulator.test.ts b/packages/metadata-protocol/src/protocol.read-seam-empty-accumulator.test.ts new file mode 100644 index 0000000000..bff6108f25 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.read-seam-empty-accumulator.test.ts @@ -0,0 +1,314 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8896] Two protocol read seams that answered a FAILED read out of an empty + * accumulator, with no log and no field saying the answer was incomplete. + * + * Both have the shape ADR-0110 D3 forbids: the caller receives an answer + * indistinguishable from a legitimate one, and acts on it. + * + * - `searchAll` — a per-object `catch { continue; }`, while the response kept + * reporting `totalObjects` / `totalHits` / `truncated` as though the sweep + * had been complete. A partial scan wearing a whole one's numbers. + * - `findReferencesToMeta` — a per-matcher `catch { return; }` inside a + * `Promise.all`, silently shortening the list that answers "what would + * break if I delete this". A short list reads as "nothing depends on it". + * + * The repairs are not symmetric, and that is the point of measuring per seam + * rather than stamping one template on both: + * + * - `searchAll` reads the DATA store directly, so it asks the shared + * `isMissingTableError` predicate itself: a registered object whose table + * was never provisioned can hold no rows, so contributing no hits is the + * truth; everything else propagates. + * - `findReferencesToMeta` reads through `getMetaItems`, which ALREADY does + * that discrimination (`rethrowUnlessMetadataStoreUnprovisioned`, #5532): + * the benign case returns normally, and a real outage is raised as a 503. + * So this seam gets NO predicate of its own — the only thing its `catch` + * could swallow was the 503 raised deliberately one line below it. A second + * discrimination here would be a second vocabulary of "benign", which is + * exactly the debt `@objectstack/metadata/errors` exists to retire. + * + * Every expectation is written against LITERALS — the exact injected error + * object, its literal message and code, the literal 503 / `SERVICE_UNAVAILABLE` + * envelope, literal hit and reference counts. Each failure assertion is paired + * with a positive control in the same describe (the read SUCCEEDING and + * producing hits/references) and, for the benign branch, with proof that the + * injected throw actually fired — so a harness that had stopped exercising the + * seam could not pass vacuously. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ErrorCode } from '@objectstack/spec/api'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +interface FixtureObject { + name: string; + label: string; + fields: Record; +} + +const objectFixture = (name: string): FixtureObject => ({ + name, + label: name, + fields: { name: { name: 'name', label: 'Name', type: 'text' } }, +}); + +/** + * A registry carrying `objects` (what `searchAll` sweeps) and `items` (what + * `getMetaItems` folds the `sys_metadata` overlay onto). + */ +function fixtureRegistry(objects: FixtureObject[], items: Record = {}) { + return { + getObject: (n: string) => objects.find((o) => o.name === n), + getAllObjects: () => objects, + getItem: () => undefined, + listItems: (type: string) => items[type] ?? [], + applyNavContributions: (x: unknown) => x, + isPackageDisabled: () => false, + getObjectOwner: () => undefined, + getPackage: () => undefined, + }; +} + +/** The real driver phrasings, verbatim. */ +const connectionDropped = () => + Object.assign(new Error('connection terminated unexpectedly'), { code: 'ECONNRESET' }); +const tableNotProvisioned = (table: string) => + Object.assign(new Error(`SQLITE_ERROR: no such table: ${table}`), { code: 'SQLITE_ERROR' }); + +/** Capture a rejection without letting a resolve pass silently. */ +async function rejection(run: () => Promise): Promise & { message?: string }> { + let caught: unknown; + let resolved: unknown; + let didResolve = false; + try { + resolved = await run(); + didResolve = true; + } catch (e) { + caught = e; + } + expect( + didResolve, + `expected a rejection, but the call resolved with ${JSON.stringify(resolved)}`, + ).toBe(false); + return caught as Record & { message?: string }; +} + +// ═══════════════════════════════════════════════════════════════════════════ +// searchAll +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8896] searchAll — an object that could not be READ is not an object with no matches', () => { + const acct = objectFixture('acct'); + const lead = objectFixture('lead'); + + /** `acct` always answers with one matching row; `lead`'s read is the variable. */ + function engineWhereLeadFails(error: unknown) { + const readCalls: string[] = []; + const engine = { + registry: fixtureRegistry([acct, lead]), + find: vi.fn(async (object: string) => { + readCalls.push(object); + if (object === 'lead') throw error; + return [{ id: 'a1', name: 'Acme' }]; + }), + findOne: vi.fn(async () => null), + }; + return { engine, readCalls }; + } + + it('control: a sweep whose reads all RUN returns the hits and counts them', async () => { + const engine = { + registry: fixtureRegistry([acct, lead]), + find: vi.fn(async (object: string) => ( + object === 'acct' ? [{ id: 'a1', name: 'Acme' }] : [{ id: 'l1', name: 'Acme Lead' }] + )), + findOne: vi.fn(async () => null), + }; + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.searchAll({ q: 'Acme' }); + + expect(result.totalObjects).toBe(2); + expect(result.totalHits).toBe(2); + expect(result.truncated).toBe(false); + expect(result.hits.map((h) => h.object)).toEqual(['acct', 'lead']); + }); + + it('a swept object whose read FAILS surfaces that error instead of shrinking the answer', async () => { + const injected = connectionDropped(); + const { engine, readCalls } = engineWhereLeadFails(injected); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const caught = await rejection(() => protocol.searchAll({ q: 'Acme' })); + + // The caller receives the READ's own failure, envelope intact — this + // fix mints no new code and no new response field. + expect(caught).toBe(injected); + expect(caught.message).toBe('connection terminated unexpectedly'); + expect(caught.code).toBe('ECONNRESET'); + // Proof the failing object really was swept. + expect(readCalls).toContain('lead'); + // Pre-fix this resolved with `{ totalObjects: 2, totalHits: 1, + // truncated: false }` — a scan of one object, reported as a complete + // scan of two. + }); + + it('a missing COLUMN on a provisioned table stays loud (the superstring case)', async () => { + // Postgres phrases this as `column "x" of relation "y" does not exist`, + // which CONTAINS a complete, legal missing-table phrase. The table is + // there; the read still did not happen. `isMissingTableError`'s + // front-exclusion is what keeps this loud, and this pin is what stops a + // future hand-rolled code test from reading it as benign. + const injected = Object.assign( + new Error('column "name" of relation "lead" does not exist'), + { code: '42703' }, + ); + const { engine } = engineWhereLeadFails(injected); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const caught = await rejection(() => protocol.searchAll({ q: 'Acme' })); + + expect(caught).toBe(injected); + expect(caught.message).toBe('column "name" of relation "lead" does not exist'); + }); + + it('an UNPROVISIONED table is truthful emptiness: the sweep continues', async () => { + // Routine, not exotic: the registry lists every DECLARED object whether + // or not this deployment provisioned its table, and such an object can + // hold no rows — so contributing no hits IS the truth. + const { engine, readCalls } = engineWhereLeadFails(tableNotProvisioned('lead')); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.searchAll({ q: 'Acme' }); + + expect(result.totalHits).toBe(1); + expect(result.hits.map((h) => h.object)).toEqual(['acct']); + // Proof the benign branch was actually EXERCISED — the read ran and + // threw. Without this, the passing search above would be consistent + // with a harness that never sweeps `lead` at all. + expect(readCalls).toContain('lead'); + }); + + it('an UNPROVISIONED table in the postgres phrasing (42P01) is benign too', async () => { + const { engine, readCalls } = engineWhereLeadFails( + Object.assign(new Error('relation "lead" does not exist'), { code: '42P01' }), + ); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.searchAll({ q: 'Acme' }); + + expect(result.totalHits).toBe(1); + expect(readCalls).toContain('lead'); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// findReferencesToMeta +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#8896] findReferencesToMeta — a source type that could not be READ is not a source type with no references', () => { + /** + * `view` has three matchers (`dashboard`, `app`, `page`), so a single + * failing source type leaves the other two answering — which is exactly the + * pre-fix trap: a SHORT list that looks complete. `page` carries a real + * reference to `my_view`, so the healthy half is observable. + */ + const pageReferencingTheView = { name: 'home_page', label: 'Home', viewName: 'my_view' }; + + function engineWhereTypeFails(failingType: string | null, error?: unknown) { + const typeReads: string[] = []; + const engine = { + registry: fixtureRegistry([], { page: [pageReferencingTheView] }), + find: vi.fn(async (_table: string, query?: { where?: { type?: string } }) => { + const type = query?.where?.type; + if (typeof type === 'string') typeReads.push(type); + if (failingType !== null && type === failingType) throw error; + return []; + }), + findOne: vi.fn(async () => null), + }; + return { engine, typeReads }; + } + + it('control: a scan whose reads all RUN returns the reference it found', async () => { + const { engine, typeReads } = engineWhereTypeFails(null); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.findReferencesToMeta({ type: 'view', name: 'my_view' }); + + expect(result.references).toEqual([ + { type: 'page', name: 'home_page', label: 'Home', path: 'viewName', kind: 'page' }, + ]); + // All three source types were really consulted — this is what makes + // "one of them failed" a meaningful condition below. + expect(typeReads).toContain('dashboard'); + expect(typeReads).toContain('app'); + expect(typeReads).toContain('page'); + }); + + it('a source type whose read FAILS fails the whole scan, envelope intact', async () => { + const injected = connectionDropped(); + const { engine, typeReads } = engineWhereTypeFails('dashboard', injected); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const caught = await rejection( + () => protocol.findReferencesToMeta({ type: 'view', name: 'my_view' }), + ); + + // The 503 `getMetaItems` already raises for an unreadable store — NOT a + // new code minted here, and not the driver error raw (unwrapped it has + // no status, and `mapDataError` guesses `no such table` into a 404). + expect(caught.status).toBe(503); + expect(caught.code).toBe('SERVICE_UNAVAILABLE'); + // ADR-0112: the wire code must be in the declared vocabulary, or the + // envelope fails `ApiErrorSchema.parse` at the boundary that ships it. + expect(ErrorCode.safeParse(caught.code).success).toBe(true); + // The driver's own error is not lost — it rides as `cause`. + expect(caught.cause).toBe(injected); + expect(typeReads).toContain('dashboard'); + // Pre-fix this resolved `{ references: [ …the page hit… ] }` — one real + // reference presented as the complete dependency list, which an admin + // reads as "safe to delete". + }); + + it('a source type this deployment does not declare is NOT an error — it simply has no hits', async () => { + // The seam's benign case is structural, not an error class: `listItems` + // answers `[]` for an unknown type and the overlay read finds nothing, + // so nothing is thrown in the first place. This pin is what keeps the + // repair from over-reaching into "any absent source type is an outage". + const { engine } = engineWhereTypeFails(null); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.findReferencesToMeta({ type: 'tool', name: 'no_such_tool' }); + + expect(result.references).toEqual([]); + }); + + it('a target type absent from REFERENCE_PATHS still returns an empty list without reading anything', async () => { + const { engine, typeReads } = engineWhereTypeFails(null); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.findReferencesToMeta({ type: 'not_a_tracked_type', name: 'x' }); + + expect(result.references).toEqual([]); + expect(typeReads).toEqual([]); + }); + + it('an UNPROVISIONED sys_metadata is truthful emptiness: the scan answers from the registry', async () => { + // The benign discrimination lives in `getMetaItems`, one layer down — + // this seam inherits it rather than repeating it, and this pin is what + // proves the inheritance still holds through the removed `catch`. + const { engine, typeReads } = engineWhereTypeFails('dashboard', tableNotProvisioned('sys_metadata')); + const protocol = new ObjectStackProtocolImplementation(engine as never); + + const result = await protocol.findReferencesToMeta({ type: 'view', name: 'my_view' }); + + expect(result.references).toEqual([ + { type: 'page', name: 'home_page', label: 'Home', path: 'viewName', kind: 'page' }, + ]); + // Proof the benign branch was actually EXERCISED. + expect(typeReads).toContain('dashboard'); + }); +}); diff --git a/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts new file mode 100644 index 0000000000..95b3c27dea --- /dev/null +++ b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts @@ -0,0 +1,347 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8896] `loadExistingRecords` must not answer a FAILED read with an empty + * `Map`. + * + * The map is not a cache — it IS the write decision, in all three of its + * callers, and "empty" is the answer that means *write these rows*: + * + * 1. the upsert/update/ignore pre-load — an unmatched natural key is written + * as a NEW row, so a failed read turns every update into an insert; + * 2. `writeBatchPartial`'s `attempt > 1` recheck — `bulkWrite` is + * at-least-once, so a batch may have COMMITTED before its response was + * lost (framework#3149). This recheck is the only thing standing between + * the retry and a duplicate of every row the first attempt already wrote; + * 3. `writeOne`'s per-row form of the same recheck, on the degradation path. + * + * It sat behind a bare `catch { /* Object may not have records yet *\/ }`, so a + * connection drop, a timeout, a permission denial or a query error all arrived + * at those three as "there are no existing rows" — ADR-0110 D3's exact shape, + * where "the read found nothing" and "the read could not run" have opposite + * consequences. (The swallowed comment also named a case that cannot reach it: + * an object that merely has no rows yet answers `find` with `[]`, it does not + * throw.) + * + * The repair is discrimination, not deletion of the `catch`: only an + * unprovisioned TABLE is truthful emptiness, and everything else propagates. + * + * Every expectation below is written against LITERALS — the exact injected + * error object, its literal message and code, literal row counts, literal + * summary counters — never a value re-derived from the code under test. And + * each failure assertion is paired with a positive control in this same file + * (the read SUCCEEDING and matching, the read SUCCEEDING and not matching, and + * — for the benign branch — proof that the injected throw actually fired), so a + * harness that had stopped exercising the seam could not pass vacuously. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +import { SeedLoaderService } from './seed-loader'; + +interface StoreRow extends Record { + id: string; +} + +function createLogger() { + return { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; +} + +/** + * A faithful in-memory engine plus a read-failure injector. + * + * `failFind` makes every `find` throw exactly that value — the object identity + * is what the assertions check, so nothing has to guess how the loader might + * re-wrap a driver error. `findCalls` records that the read really ran, which + * is what turns "the seed proceeded" into "the seed proceeded AND the injected + * throw fired". + */ +function createEngine() { + const store: Record = {}; + const findCalls: string[] = []; + let failFind: unknown = null; + let idCounter = 0; + + const engine = { + find: vi.fn(async (objectName: string, query?: { where?: Record; limit?: number }) => { + findCalls.push(objectName); + if (failFind !== null) throw failFind; + let records = store[objectName] ?? []; + if (query?.where) { + const where = query.where; + records = records.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)); + } + if (typeof query?.limit === 'number') records = records.slice(0, query.limit); + return records; + }), + findOne: vi.fn(async (objectName: string, query?: Record) => { + const rows = await (engine.find as unknown as (o: string, q: unknown) => Promise)( + objectName, { ...query, limit: 1 }, + ); + return rows[0] ?? null; + }), + insert: vi.fn(async (objectName: string, data: Record | Record[]) => { + store[objectName] ??= []; + if (Array.isArray(data)) { + const records = data.map((d) => ({ ...d, id: `gen-${++idCounter}` }) as StoreRow); + store[objectName].push(...records); + return records; + } + const record = { ...data, id: `gen-${++idCounter}` } as StoreRow; + store[objectName].push(record); + return record; + }), + update: vi.fn(async (objectName: string, data: Record) => { + const records = store[objectName] ?? []; + const idx = records.findIndex((r) => r.id === data.id); + if (idx >= 0) { + records[idx] = { ...records[idx], ...data } as StoreRow; + return records[idx]; + } + return data; + }), + delete: vi.fn(async () => ({ deleted: 1 })), + count: vi.fn(async (objectName: string) => (store[objectName] ?? []).length), + aggregate: vi.fn(async () => []), + } as unknown as IDataEngine; + + return { + engine, + store, + findCalls, + failReadsWith: (error: unknown) => { failFind = error; }, + stopFailingReads: () => { failFind = null; }, + }; +} + +const WIDGET = { + name: 'my_app_widget', + fields: { + name: { type: 'text' }, + sku: { type: 'text' }, + }, +}; + +function createMetadata(): IMetadataService { + return { + getObject: vi.fn(async () => WIDGET), + listObjects: vi.fn(async () => [WIDGET]), + register: vi.fn(async () => {}), + get: vi.fn(async () => WIDGET), + list: vi.fn(async () => []), + unregister: vi.fn(async () => {}), + exists: vi.fn(async () => false), + listNames: vi.fn(async () => []), + } as unknown as IMetadataService; +} + +const CONFIG = { + dryRun: false, + haltOnError: false, + multiPass: true, + defaultMode: 'insert', + batchSize: 1000, + transaction: false, +} as never; + +const seedOf = (mode: string, records: Array>) => [{ + object: 'my_app_widget', + externalId: 'sku', + mode, + env: ['prod', 'dev', 'test'], + records, +}] as never; + +/** The real driver phrasings, verbatim. */ +const connectionDropped = () => + Object.assign(new Error('connection terminated unexpectedly'), { code: 'ECONNRESET' }); +const tableNotProvisioned = () => + Object.assign(new Error('SQLITE_ERROR: no such table: my_app_widget'), { code: 'SQLITE_ERROR' }); + +/** Capture a rejection without letting a resolve pass silently. */ +async function rejection(run: () => Promise): Promise<{ code?: string; message?: string } & Record> { + let caught: unknown; + let resolved: unknown; + let didResolve = false; + try { + resolved = await run(); + didResolve = true; + } catch (e) { + caught = e; + } + expect( + didResolve, + `expected a rejection, but the load resolved with ${JSON.stringify(resolved)}`, + ).toBe(false); + return caught as { code?: string; message?: string } & Record; +} + +const rowsWithSku = (store: Record, sku: string) => + (store.my_app_widget ?? []).filter((r) => r.sku === sku); + +describe('[#8896] seed loader — an existing-records read that FAILED is not "no existing rows"', () => { + // ── POSITIVE CONTROLS. Without these, every assertion below could pass on + // a harness that no longer consults `loadExistingRecords` at all. + + it('control: a read that RUNS and matches updates the row in place — no insert', async () => { + const { engine, store } = createEngine(); + store.my_app_widget = [{ id: 'gen-0', name: 'Existing', sku: 'W-A' }]; + + const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('upsert', [{ name: 'Updated', sku: 'W-A' }]), + config: CONFIG, + }); + + expect(result.summary.totalUpdated).toBe(1); + expect(result.summary.totalInserted).toBe(0); + expect(result.summary.totalErrored).toBe(0); + expect(rowsWithSku(store, 'W-A')).toHaveLength(1); + expect(store.my_app_widget[0].name).toBe('Updated'); + }); + + it('control: a read that RUNS and matches nothing inserts the row', async () => { + const { engine, store } = createEngine(); + + const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('upsert', [{ name: 'Fresh', sku: 'W-A' }]), + config: CONFIG, + }); + + expect(result.summary.totalInserted).toBe(1); + expect(result.summary.totalUpdated).toBe(0); + expect(rowsWithSku(store, 'W-A')).toHaveLength(1); + }); + + // ── THE FIX — a read that could not run must surface, not invent "none". + + it('an upsert pre-load that fails surfaces that error and writes nothing', async () => { + const { engine, store } = createEngine(); + // A row that the upsert MUST match. Pre-fix the failed read hid it and + // the seed inserted a second row carrying the same natural key. + store.my_app_widget = [{ id: 'gen-0', name: 'Existing', sku: 'W-A' }]; + + const injected = connectionDropped(); + const loader = new SeedLoaderService(engine, createMetadata(), createLogger()); + (engine.find as unknown as { mockImplementation: (f: () => Promise) => void }) + .mockImplementation(async () => { throw injected; }); + + const caught = await rejection(() => loader.load({ + seeds: seedOf('upsert', [{ name: 'Updated', sku: 'W-A' }]), + config: CONFIG, + })); + + // The caller receives the READ's own failure, envelope intact — this + // fix mints no new code and no new result field. + expect(caught).toBe(injected); + expect(caught.message).toBe('connection terminated unexpectedly'); + expect(caught.code).toBe('ECONNRESET'); + // …and emphatically NOT the pre-fix outcome: a second row under the + // same natural key, reported as a clean insert. + expect(rowsWithSku(store, 'W-A')).toHaveLength(1); + expect(store.my_app_widget[0].name).toBe('Existing'); + }); + + it('the at-least-once RETRY recheck no longer duplicates every committed row (framework#3149)', async () => { + const { engine, store, findCalls, failReadsWith } = createEngine(); + + // turso's commit-then-lost-response shape: the array insert lands both + // rows and THEN throws, so `bulkWrite` retries. The retry's recheck — + // `loadExistingRecords` — is the only thing that keeps it from writing + // both rows a second time, and here that recheck cannot be performed. + const realInsert = (engine.insert as unknown as { getMockImplementation: () => (...a: unknown[]) => Promise }) + .getMockImplementation(); + let arrayInsertCalls = 0; + (engine.insert as unknown as { mockImplementation: (f: (o: string, d: unknown, x: unknown) => Promise) => void }) + .mockImplementation(async (objectName: string, data: unknown, opts: unknown) => { + if (objectName === 'my_app_widget' && Array.isArray(data)) { + arrayInsertCalls += 1; + if (arrayInsertCalls === 1) { + await realInsert(objectName, data, opts); // the commit lands + failReadsWith(connectionDropped()); // …the recheck cannot run + throw new Error('fetch failed'); // …and the response is lost + } + } + return realInsert(objectName, data, opts); + }); + + const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('insert', [{ name: 'A', sku: 'W-A' }, { name: 'B', sku: 'W-B' }]), + config: CONFIG, + }); + + // Proof the recheck really was attempted and really threw — without + // this the row counts below would also be satisfied by a harness that + // never rechecks. + expect(findCalls.length).toBeGreaterThan(0); + // The rows the FIRST attempt committed, exactly once each. Pre-fix the + // empty map made the retry re-insert both, and the load reported four + // rows as two clean inserts. + expect(rowsWithSku(store, 'W-A')).toHaveLength(1); + expect(rowsWithSku(store, 'W-B')).toHaveLength(1); + expect(store.my_app_widget).toHaveLength(2); + // The seed's EXISTING error accounting carries the failure — the fix + // adds no new result field. + expect(result.summary.totalErrored).toBe(2); + expect(result.summary.totalInserted).toBe(0); + }); + + // ── THE ONE BENIGN CASE — an unprovisioned table can hold no rows, so an + // empty map is the truth and every caller's "write it" verdict is right. + + it('an UNPROVISIONED table is truthful emptiness: the seed writes its rows', async () => { + const { engine, store, findCalls, failReadsWith } = createEngine(); + failReadsWith(tableNotProvisioned()); + + const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('upsert', [{ name: 'Fresh', sku: 'W-A' }]), + config: CONFIG, + }); + + expect(result.summary.totalInserted).toBe(1); + expect(result.summary.totalErrored).toBe(0); + // Proof the benign branch was actually EXERCISED — the read ran and + // threw. Without this, the passing insert above would be consistent + // with a harness that never pre-loads at all. + expect(findCalls).toContain('my_app_widget'); + expect(rowsWithSku(store, 'W-A')).toHaveLength(1); + }); + + it('an UNPROVISIONED table in the postgres phrasing (42P01) is benign too', async () => { + const { engine, findCalls, failReadsWith } = createEngine(); + failReadsWith(Object.assign( + new Error('relation "my_app_widget" does not exist'), + { code: '42P01' }, + )); + + const result = await new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('upsert', [{ name: 'Fresh', sku: 'W-A' }]), + config: CONFIG, + }); + + expect(result.summary.totalInserted).toBe(1); + expect(findCalls).toContain('my_app_widget'); + }); + + it('a missing COLUMN on an existing table stays loud (the superstring case)', async () => { + const { engine } = createEngine(); + // Postgres phrases this failure as `column "x" of relation "y" does not + // exist` — which CONTAINS a complete, legal missing-table phrase. + // `isMissingTableError`'s front-exclusion is what keeps it loud, and + // this pin is what stops a future hand-rolled code test reading it as + // benign and silently re-arming the duplicate-row outcome. + const injected = Object.assign( + new Error('column "sku" of relation "my_app_widget" does not exist'), + { code: '42703' }, + ); + (engine.find as unknown as { mockImplementation: (f: () => Promise) => void }) + .mockImplementation(async () => { throw injected; }); + + const caught = await rejection(() => new SeedLoaderService(engine, createMetadata(), createLogger()).load({ + seeds: seedOf('upsert', [{ name: 'Fresh', sku: 'W-A' }]), + config: CONFIG, + })); + + expect(caught).toBe(injected); + expect(caught.message).toBe('column "sku" of relation "my_app_widget" does not exist'); + }); +}); From 6ee50a633ea122d50c799d222f13fdf0e1842135 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 07:49:14 +0000 Subject: [PATCH 3/5] test(metadata-protocol): refuse unimplemented combinators in the publish fixture double, route the seed double through the dispatch predicates (#8896) --- ...ublish-commit-capture-read-failure.test.ts | 11 +++++++++ ...ader-existing-records-read-failure.test.ts | 24 +++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts b/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts index 8e51e068a7..bb2cad88ad 100644 --- a/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts +++ b/packages/metadata-protocol/src/protocol.publish-commit-capture-read-failure.test.ts @@ -84,12 +84,23 @@ interface CommitItem { const keyOf = (w: Record) => `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`; +/** + * The overlay reads this fixture serves use flat equality plus `$or`, so those + * are the two shapes implemented — and every OTHER combinator is refused + * loudly rather than read as a field name (#8494). A double that silently + * answers a combinator it does not implement is wrong in the direction no + * assertion can see: `row['$and']` is `undefined`, so the clause "does not + * match" for a reason that has nothing to do with the data. + */ function matchesWhere(row: Record, where: Record): boolean { for (const [k, v] of Object.entries(where)) { if (k === '$or') { if (!(v as Array>).some((c) => matchesWhere(row, c))) return false; continue; } + if (k.startsWith('$')) { + throw new Error(`fake engine: unsupported combinator ${k}`); + } // `undefined` = "dimension not constrained"; `null` = "must be NULL". if (v === undefined) continue; if (row[k] !== v) return false; diff --git a/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts index 95b3c27dea..a9a377afa1 100644 --- a/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts +++ b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts @@ -37,6 +37,12 @@ import { describe, it, expect, vi } from 'vitest'; import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +// [#5619] The producer's OWN write-verb dispatch decisions (#4550 delete / +// #5480 update), so the fake engine below cannot accept a call ObjectQL +// refuses. From `@objectstack/metadata-core`, not `@objectstack/objectql` — +// objectql depends on THIS package, so that import would close a dependency +// cycle turbo rejects outright. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { SeedLoaderService } from './seed-loader'; interface StoreRow extends Record { @@ -69,7 +75,14 @@ function createEngine() { let records = store[objectName] ?? []; if (query?.where) { const where = query.where; - records = records.filter((r) => Object.entries(where).every(([k, v]) => r[k] === v)); + records = records.filter((r) => Object.entries(where).every(([k, v]) => { + // REFUSE rather than guess: a combinator read as a field + // name is a silently-wrong matcher, and this fixture only + // ever receives flat equality (`organization_id`). Same + // convention as `seed-loader-retry.test.ts`. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return r[k] === v; + })); } if (typeof query?.limit === 'number') records = records.slice(0, query.limit); return records; @@ -92,6 +105,10 @@ function createEngine() { return record; }), update: vi.fn(async (objectName: string, data: Record) => { + // The seed loader dispatches an update by the id carried IN `data` + // (no `where`), so the producer's own decision is asked in exactly + // that form — same call as `seed-loader-retry.test.ts`. + assertEngineUpdateDispatch(data, undefined); const records = store[objectName] ?? []; const idx = records.findIndex((r) => r.id === data.id); if (idx >= 0) { @@ -100,7 +117,10 @@ function createEngine() { } return data; }), - delete: vi.fn(async () => ({ deleted: 1 })), + delete: vi.fn(async (_objectName: string, options?: { where?: Record }) => { + assertEngineDeleteDispatch(options); + return { deleted: 1 }; + }), count: vi.fn(async (objectName: string) => (store[objectName] ?? []).length), aggregate: vi.fn(async () => []), } as unknown as IDataEngine; From 43f0902fad554372e07ef786567880173bb163e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 07:58:10 +0000 Subject: [PATCH 4/5] test(metadata-protocol): use the nodenext-explicit specifier so the new pin adds nothing to TEST_DEBT (#8896) --- .../src/seed-loader-existing-records-read-failure.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts index a9a377afa1..f051d08589 100644 --- a/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts +++ b/packages/metadata-protocol/src/seed-loader-existing-records-read-failure.test.ts @@ -43,7 +43,11 @@ import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts' // objectql depends on THIS package, so that import would close a dependency // cycle turbo rejects outright. import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; -import { SeedLoaderService } from './seed-loader'; +// `.js` extension deliberately, unlike the older sibling seed-loader tests: +// `moduleResolution: nodenext` requires it, and an extensionless specifier is +// exactly the TS2835 that makes up part of this package's frozen TEST_DEBT +// (#5278). That ledger is shrink-only, so a new file may not add to it. +import { SeedLoaderService } from './seed-loader.js'; interface StoreRow extends Record { id: string; From 6366de7530bf330cd3f91a3914e772505a335767 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 08:31:19 +0000 Subject: [PATCH 5/5] test(objectql): give the publishPackageDrafts fixtures a real capture double (#8896) --- packages/objectql/src/build-probes.test.ts | 10 +- .../protocol-publish-package-drafts.test.ts | 177 ++++++++++++++++-- 2 files changed, 175 insertions(+), 12 deletions(-) diff --git a/packages/objectql/src/build-probes.test.ts b/packages/objectql/src/build-probes.test.ts index d7d90bf5b2..3591425b56 100644 --- a/packages/objectql/src/build-probes.test.ts +++ b/packages/objectql/src/build-probes.test.ts @@ -192,8 +192,16 @@ describe('publishPackageDrafts — probes ride the response (ADR-0038 L3)', () = vi.spyOn(protocol as any, 'runPublishSideEffects').mockResolvedValue({}); vi.spyOn(protocol as any, 'applySeedBodies').mockResolvedValue({ success: false, inserted: 0, updated: 0, error: 'boom' }); // Probe reads: active items + an engine whose table stayed empty. + // + // [#8896] `findOne` answers the ADR-0067 pre-publish capture, and it + // returns `null` EXPLICITLY: this package has never been published, so no + // artifact has an active row and `existedBefore: false` is the truth here. + // Before the capture was discriminated by error type, this fixture had no + // `findOne` at all — the resulting TypeError was swallowed and the same + // `false` was FABRICATED, so this test passed without the read ever + // running. The two are now distinguishable, and this is the truthful one. (protocol as any).getMetaItem = async ({ type, name }: any) => ({ item: ITEMS[`${type} ${name}`] }); - (protocol as any).engine = { find: async () => [] }; + (protocol as any).engine = { find: async () => [], findOne: async () => null }; const res = await protocol.publishPackageDrafts({ packageId: 'app.exp' }); diff --git a/packages/objectql/src/protocol-publish-package-drafts.test.ts b/packages/objectql/src/protocol-publish-package-drafts.test.ts index 669c437ef8..181c904a33 100644 --- a/packages/objectql/src/protocol-publish-package-drafts.test.ts +++ b/packages/objectql/src/protocol-publish-package-drafts.test.ts @@ -13,11 +13,84 @@ import { assertEngineUpdateDispatch } from './engine-update-dispatch.js'; * metadata committed. These tests cover the orchestration contract; the * per-item guards live in `publishMetaItem`'s own suites. */ -function makeProtocol(drafts: Array<{ type: string; name: string }>) { +/** An artifact that is already ACTIVE before the publish under test runs. */ +interface ActiveRow { + type: string; + name: string; + organizationId?: string | null; + version: number; +} + +/** + * [#8896] A real double for the two engine calls `publishPackageDrafts` makes + * on its own — the ADR-0067 pre-publish CAPTURE read, and the commit write. + * + * ## Why this fixture had to grow a `findOne` + * + * It never had one, and until #8896 that was invisible: the capture sat behind + * a bare `catch` which swallowed the resulting `TypeError` and pushed a + * fabricated `{ existedBefore: false, prevVersion: null }` entry. So every test + * in this file was green on a path that never ran — including the ones whose + * names claim to cover the batch end to end. `existedBefore` was `false` for + * every item of every case here, not because the fixture said so but because + * the read crashed and the crash was hidden. Nothing in the repo exercised the + * real capture. + * + * Once the capture discriminates by error type (only an unprovisioned table is + * benign; a `TypeError` from an engine missing the method is not), the missing + * method surfaces as it should. The repair belongs HERE: the production + * behaviour is right and the fixture was lying. + * + * ## Two things it is careful about + * + * `null` is returned EXPLICITLY for "no active row exists" rather than falling + * out of an absent method, because those two are now distinguishable and only + * one of them is a truthful answer. And `insert` records the commit row, so the + * revert plan this fixture produces is observable instead of being swallowed a + * second time by `recordCommit`'s own catch. + */ +function makeCaptureEngine(activeRows: ActiveRow[] = []) { + const captureReads: Array> = []; + const commitRows: Array> = []; + const engine = { + findOne: async (table: string, opts?: { where?: Record }) => { + if (table !== 'sys_metadata') return null; + const where = opts?.where ?? {}; + if (where.state !== 'active') return null; + captureReads.push(where); + const hit = activeRows.find( + (r) => r.type === where.type + && r.name === where.name + && (r.organizationId ?? null) === (where.organization_id ?? null), + ); + // Explicitly `null`, never `undefined`-by-omission — see the docblock. + return hit ? { version: hit.version } : null; + }, + insert: async (table: string, data: Record) => { + if (table === 'sys_metadata_commit') commitRows.push(data); + return { id: `${table}_${commitRows.length}` }; + }, + }; + /** The revert plan as it was actually STORED, parsed from the commit row. */ + const storedCommitItems = (): Array> => commitRows.map((c) => JSON.parse(String(c.items))); + return { engine, captureReads, commitRows, storedCommitItems }; +} + +function makeProtocol( + drafts: Array<{ type: string; name: string }>, + activeRows: ActiveRow[] = [], +) { const protocol = new ObjectStackProtocolImplementation({} as never); // Stub the bits that need a real engine/overlay so we can exercise the loop. (protocol as any).ensureOverlayIndex = async () => {}; (protocol as any).getOverlayRepo = () => ({ listDrafts: async () => drafts }); + // [#8896] The capture/commit double. Tests that need MORE engine surface + // spread this rather than replacing it — replacing it takes `findOne` away + // again and re-arms exactly the vacuity described above. + const capture = makeCaptureEngine(activeRows); + (protocol as any).engine = capture.engine; // Phase-1 / Phase-2 seams (ADR-0067 D2). const promote = vi.spyOn(protocol as any, 'promoteDraftForPublish'); const sideEffects = vi @@ -28,7 +101,15 @@ function makeProtocol(drafts: Array<{ type: string; name: string }>) { orgId: null, result: { version: 'h', seq: 1, item: { body: { name: req.name } }, packageId: null }, }); - return { protocol, promote, sideEffects, promoteOk }; + return { + protocol, + promote, + sideEffects, + promoteOk, + baseEngine: capture.engine, + captureReads: capture.captureReads, + storedCommitItems: capture.storedCommitItems, + }; } /** A fake engine whose transaction() tracks commit/rollback (ADR-0067 D2). */ @@ -57,11 +138,23 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { { type: 'object', name: 'student' }, { type: 'view', name: 'course_list' }, ]; - const { protocol, promote, sideEffects, promoteOk } = makeProtocol(drafts); + const { protocol, promote, sideEffects, promoteOk, captureReads } = makeProtocol(drafts); promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + // [#8896] The ADR-0067 capture really RAN, once per draft. Until the + // capture was discriminated by error type this fixture had no `findOne` at + // all and the resulting TypeError was swallowed, so every assertion below + // held over a batch whose revert plan was fabricated rather than read. + // The capture pass runs BEFORE Phase 1, so its reads are the FIRST three — + // one per draft, in draft order, each in the draft's own scope. (Reads + // after these belong to the ADR-0038 L3 probes, which run post-commit.) + expect(captureReads.slice(0, 3)).toEqual([ + { organization_id: null, type: 'object', name: 'course', state: 'active' }, + { organization_id: null, type: 'object', name: 'student', state: 'active' }, + { organization_id: null, type: 'view', name: 'course_list', state: 'active' }, + ]); expect(promote).toHaveBeenCalledTimes(3); expect((promote.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'course' }); // Side effects ran once per promoted item, AFTER promotion, in order. @@ -78,13 +171,63 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { expect(res.published.map((p) => p.name)).toEqual(['course', 'student', 'course_list']); }); + /** + * [#8896] The ADR-0067 revert plan, actually exercised. + * + * `existedBefore: false` means "revert = soft-remove"; `true` means "revert = + * restoreVersion(prevVersion)". Those are opposite operations, and until this + * card every test in this file recorded `false` for every item — not because + * the fixture had no active rows but because the capture read crashed on a + * missing `findOne` and a bare `catch` fabricated `false` over the crash. So + * the `true` branch had never once been reached from here, and a regression + * that made every revert a deletion would have kept this suite green. + * + * Both answers are asserted in ONE batch so neither can pass by the fixture + * simply having no active rows at all. + */ + it('records the real pre-publish state per item: existedBefore true with prevVersion, false for a new artifact', async () => { + const { protocol, promote, promoteOk, captureReads, storedCommitItems } = makeProtocol( + [ + { type: 'object', name: 'course' }, // already active at version 4 + { type: 'object', name: 'student' }, // brand new + ], + [{ type: 'object', name: 'course', version: 4 }], + ); + promote.mockImplementation(async (req: any) => promoteOk(req)); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + + expect(res).toMatchObject({ success: true, publishedCount: 2 }); + // The capture ran for both items, in their own scope — this is what makes + // the values below evidence rather than defaults. + expect(captureReads.slice(0, 2)).toEqual([ + { organization_id: null, type: 'object', name: 'course', state: 'active' }, + { organization_id: null, type: 'object', name: 'student', state: 'active' }, + ]); + // One commit, carrying one entry per promoted item, each with the state + // that was actually READ. + expect(storedCommitItems()).toEqual([ + [ + { type: 'object', name: 'course', existedBefore: true, prevVersion: 4 }, + { type: 'object', name: 'student', existedBefore: false, prevVersion: null }, + ], + ]); + // And the commit was really recorded — `recordCommit` swallows its own + // write failure, so an unasserted `commitId` proves nothing (see #9066). + expect(res.commitId).toBeDefined(); + }); + it('rejects an object draft missing the package namespace prefix — atomic, before promoting', async () => { - const { protocol, promote } = makeProtocol([ + const { protocol, promote, baseEngine } = makeProtocol([ { type: 'object', name: 'edu_course' }, { type: 'object', name: 'ticket' }, // missing the 'edu_' prefix ]); // Package declares namespace 'edu' (derived+persisted at install time). - (protocol as any).engine = { registry: { getPackage: () => ({ manifest: { namespace: 'edu' } }) } }; + // [#8896] Spread, never replace — see `makeCaptureEngine`. + (protocol as any).engine = { + ...baseEngine, + registry: { getPackage: () => ({ manifest: { namespace: 'edu' } }) }, + }; const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); @@ -97,11 +240,15 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { }); it('publishes compliant prefixed object drafts under a declared namespace', async () => { - const { protocol, promote, promoteOk } = makeProtocol([ + const { protocol, promote, promoteOk, baseEngine } = makeProtocol([ { type: 'object', name: 'edu_course' }, { type: 'object', name: 'edu_student' }, ]); - (protocol as any).engine = { registry: { getPackage: () => ({ manifest: { namespace: 'edu' } }) } }; + // [#8896] Spread, never replace — see `makeCaptureEngine`. + (protocol as any).engine = { + ...baseEngine, + registry: { getPackage: () => ({ manifest: { namespace: 'edu' } }) }, + }; promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); @@ -161,12 +308,13 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { }); it('wraps the batch in ONE engine transaction and rolls it back on failure', async () => { - const { protocol, promote, promoteOk } = makeProtocol([ + const { protocol, promote, promoteOk, baseEngine } = makeProtocol([ { type: 'object', name: 'course' }, { type: 'object', name: 'student' }, ]); const { engine, txn } = makeTxnEngine(); - (protocol as any).engine = engine; + // [#8896] Spread, never replace — see `makeCaptureEngine`. + (protocol as any).engine = { ...baseEngine, ...engine }; promote.mockImplementation(async (req: any) => { if (req.name === 'student') throw new Error('boom'); return promoteOk(req); @@ -181,11 +329,12 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { }); it('commits the transaction once on a clean batch', async () => { - const { protocol, promote, promoteOk } = makeProtocol([ + const { protocol, promote, promoteOk, baseEngine } = makeProtocol([ { type: 'object', name: 'course' }, ]); const { engine, txn } = makeTxnEngine(); - (protocol as any).engine = engine; + // [#8896] Spread, never replace — see `makeCaptureEngine`. + (protocol as any).engine = { ...baseEngine, ...engine }; promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); @@ -236,6 +385,12 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { ]; const protocol = new ObjectStackProtocolImplementation({} as never); (protocol as any).ensureOverlayIndex = async () => {}; + // [#8896] This case builds its own protocol rather than going through + // `makeProtocol`, so it needs the capture double explicitly — nothing here + // is published yet, so every artifact is new and `findOne` truthfully + // answers `null`. + const capture = makeCaptureEngine(); + (protocol as any).engine = capture.engine; const seedBodyByName: Record = { project_sample: { object: 'project', records: [{ name: 'Apollo' }] }, task_sample: { object: 'task', records: [{ name: 'Design' }] },