From 346743484e173425c96a73177450f24b49e00212 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 15:30:03 +0000 Subject: [PATCH 1/3] fix(metadata-protocol): revertCommit refuses a non-canonical stored type on its restore limb (#9174) The third at-rest door joins the two that already name this class on the wire. Measured per limb at HEAD: the restore limb answered success with reverted[].action 'restored' while registerItem was never called and the only trace was a server-side console.warn from the write-through's best-effort catch; the soft-remove limb performs its promise exactly and is deliberately left outside the gate. Matches saveMetaItem's refusal shape on this door's existing per-item failed[] channel: code STORED_TYPE_NOT_CANONICAL, which already exists and is already in the error-code ledger for this package. No fold on any ledger key, no audit row, no new receipt surface. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../revert-commit-stored-type-preflight.md | 67 ++ ...col.hydrate-overlay-canonical-type.test.ts | 7 +- ...tocol.revert-stored-type-canonical.test.ts | 571 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 132 +++- 4 files changed, 773 insertions(+), 4 deletions(-) create mode 100644 .changeset/revert-commit-stored-type-preflight.md create mode 100644 packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts diff --git a/.changeset/revert-commit-stored-type-preflight.md b/.changeset/revert-commit-stored-type-preflight.md new file mode 100644 index 0000000000..f049f3ebfa --- /dev/null +++ b/.changeset/revert-commit-stored-type-preflight.md @@ -0,0 +1,67 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): `revertCommit` refuses a non-canonical stored type on its restore limb, with the wire-visible code its sibling doors already give (#9174) + +`isNonCanonicalStoredType` (#8908) names a six-member class of AT-REST spellings +whose type the manifest-collection map omits — `fields`, `seeds`, +`external_catalogs`, `externalCatalogs`, `translations`, `email_templates`. Rows +of that class are pre-#7894 residue: `PUT /meta/fields/…` answered 200 and +persisted before the `/meta` boundary fold closed that door, and nothing +rewrites them on upgrade. + +Two doors that consume an at-rest `type` already answer for the class **by +name**: `publishPackageDrafts` refuses with a `failed[].code` of +`STORED_TYPE_NOT_CANONICAL` (#8908), and `migrateStoredMetadata` reports the row +`skipped` with the same reason stated in full (#8957). `revertCommit` is the +third consumer, and it is the producer #9111 traced and left explicitly +unguarded. + +**Measured at HEAD before choosing a shape**, end to end over the real +`SysMetadataRepository` on an unscoped kernel, per limb: + +- **restore limb** (`existedBefore: true`) — answered + `{ success: true, revertedCount: 1, failed: [] }` with + `reverted[0].action === 'restored'`, called `registerItem` **zero** times, and + left one line of server-side stderr as the only trace: + `[Protocol] registry write-through failed for fields/showcase_task.title: + [registry_type_not_canonical] …`. The receipt claims the pre-commit body is + what the platform now serves; for this class it cannot be — #9111's mint door + refuses the entry and boot refuses it too, so the restored body reaches no + reader at all. +- **soft-remove limb** (`existedBefore: false`) — answered + `{ success: true, action: 'removed' }`, the row **gone** from `sys_metadata`, + no warning emitted and no registry key touched. Nothing about that outcome is + wrong. + +**The shape is `saveMetaItem`'s refusal**, carried on this door's existing +per-item `failed[]` channel — the same one `VERSION_NOT_FOUND`, `ITEM_LOCKED` +and `NOT_OVERRIDABLE` already ride. No new receipt surface and no new error +code: `STORED_TYPE_NOT_CANONICAL` is already this package's and already in the +error-code ledger. The test that separates it from `migrateStoredMetadata`'s +decline is whether the door can do what it *promises* for this row: the migrate +pass declines because rewriting a stored type spelling is an identity move and +out of its reach entirely, so `skipped` must not poison `storedMigrationClean` +for a scan that runs forever; here the write is squarely in reach and still +delivers none of what `restored` promises, which is `saveMetaItem`'s case. So it +is refused, and `success` goes false — the commit the operator asked to undo was +not undone, and a one-shot operator action has no forever to poison. + +**The soft-remove limb is deliberately outside the gate.** It performs its +promise exactly and completely, and the removal is the one action that makes +this residue smaller; refusing it would answer `success: false` for a revert +that fully succeeded and would hand back an instruction ("drop the `fields` +row") naming the very operation it had just declined to perform. + +**Nothing is folded.** The refusal writes no audit row and no commit record, and +carries the stored spelling into `failed[]` verbatim, so #9161's ruling — the +caller's spelling reaches the ledger keys unfolded, and `AUDIT_TYPE_NOT_CANONICAL` +fires loudly when it is wrong — is untouched in both directions. A refused item +is simply absent from `reverted[]`, so the append-only revert commit built from +it never claims an undo that did not happen. + +The predicate stays the narrow at-rest one rather than the complete +`canonicalMetaType(t) !== t`: `objects`/`views` fold in the manifest map, so the +restore limb already hands the write-through a canonical key and those rows are +not this defect — widening would change a wire-visible `failed[].code` for them. diff --git a/packages/metadata-protocol/src/protocol.hydrate-overlay-canonical-type.test.ts b/packages/metadata-protocol/src/protocol.hydrate-overlay-canonical-type.test.ts index 08ecd7999d..237349cd4d 100644 --- a/packages/metadata-protocol/src/protocol.hydrate-overlay-canonical-type.test.ts +++ b/packages/metadata-protocol/src/protocol.hydrate-overlay-canonical-type.test.ts @@ -37,7 +37,12 @@ // `publishPackageDrafts` is pre-empted by #8908's `STORED_TYPE_NOT_CANONICAL` // pre-flight (`isNonCanonicalStoredType`). // 5. `revertCommit` → write-through `PLURAL_TO_SINGULAR[it.type]` -// over a STORED commit-item type. ← UNGUARDED +// over a STORED commit-item type. ← UNGUARDED at the time +// [#9174] Its restore limb now carries a `STORED_TYPE_NOT_CANONICAL` +// pre-flight of its own, so this producer no longer delivers the class to +// the assert. The assert stays the contract for a producer that stops +// folding; the pre-flight is what gives the caller a wire-visible verdict. +// Measured in `protocol.revert-stored-type-canonical.test.ts`. // 6. `loadMetaFromDb` boot hydration `PLURAL_TO_SINGULAR[record.type]` // over a STORED row type. ← UNGUARDED // diff --git a/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts new file mode 100644 index 0000000000..5c24de08d7 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts @@ -0,0 +1,571 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#9174] `revertCommit` refuses a non-canonical STORED type on its restore +// limb — the third at-rest door joining the two that already name this class +// on the wire. +// +// --------------------------------------------------------------------------- +// The question this file answers +// --------------------------------------------------------------------------- +// `isNonCanonicalStoredType` (#8908) names a six-member class of stored +// spellings whose type the manifest-collection map omits — `fields`, `seeds`, +// `external_catalogs`, `externalCatalogs`, `translations`, `email_templates`. +// Two doors that consume an at-rest `type` already answer for it by name: +// +// • `publishPackageDrafts` — pre-flight refusal, `failed[].code` of +// `STORED_TYPE_NOT_CANONICAL`, batch-atomic (#8908); +// • `migrateStoredMetadata` — the row reported `outcome: 'skipped'` with the +// same reason stated in full (#8957). +// +// `revertCommit` is the third consumer and had no such gate. It is the route +// #9111 traced as producer 5 and left explicitly UNGUARDED. +// +// --------------------------------------------------------------------------- +// What was MEASURED at HEAD, per limb, before any shape was chosen +// --------------------------------------------------------------------------- +// Driven end to end over the real `SysMetadataRepository` on an UNSCOPED +// kernel (the only one where the overlay-hydration limb runs at all), with a +// `fields/showcase_task.title` row at rest: +// +// RESTORE limb (`existedBefore: true`, `prevVersion: 1`) +// -> { success: true, revertedCount: 1, failedCount: 0, +// reverted: [ { type: 'fields', …, action: 'restored' } ], failed: [] } +// `registerItem` called ZERO times. The only trace anywhere: +// `[Protocol] registry write-through failed for fields/showcase_task.title: +// [registry_type_not_canonical] Refusing to register a SchemaRegistry +// overlay entry under the non-canonical metadata type 'fields' …` +// on the server's stderr. The receipt claims the pre-commit body is what +// the platform now serves; for this class it cannot be — #9111's mint +// door refuses the entry and boot refuses it too, so the restored body +// reaches no reader at all. +// +// SOFT-REMOVE limb (`existedBefore: false`) +// -> { success: true, reverted: [ { …, action: 'removed' } ], failed: [] }, +// the row GONE from `sys_metadata`, no warning emitted, no registry key +// touched. Nothing about that outcome is wrong. +// +// --------------------------------------------------------------------------- +// Which sibling shape this matches, and why REFUSE rather than DECLINE +// --------------------------------------------------------------------------- +// `saveMetaItem`'s: a wire-visible coded refusal, carried on this door's +// EXISTING per-item `failed[]` channel — the same one `VERSION_NOT_FOUND`, +// `ITEM_LOCKED` and `NOT_OVERRIDABLE` already ride. No new receipt surface, no +// new error code (`STORED_TYPE_NOT_CANONICAL` is already this package's, and +// already in the ledger). +// +// The test that separates the two siblings is whether the door can do what it +// PROMISES for this row. `migrateStoredMetadata` declines because rewriting a +// stored type spelling is an identity move and is out of that pass's reach +// entirely — nothing is refused, which is why `skipped` must not poison +// `storedMigrationClean` for a scan that runs forever. Here the write is +// squarely IN reach: the restore succeeds at the row and still delivers none of +// what `restored` promises. That is `saveMetaItem`'s case — a write this door is +// able to perform and must not — so it is refused, and `success` goes false +// because the commit the operator asked to undo was not undone. A one-shot +// operator action has no forever to poison. +// +// ⛔ Deliberately NOT on the soft-remove limb (section 2). That limb performs +// its promise exactly and completely, and the removal is the one action that +// reduces this residue; refusing it would answer `success: false` for a revert +// that fully succeeded and would hand back an instruction naming the very +// operation it had just declined to perform. #8908's own predicate is scoped +// the same way for the same reason. +// +// ⛔ No fold, no audit row, no commit-record entry for a refused item +// (sections 1 and 4) — #9161's ruling keeps the caller's spelling on the ledger +// keys unfolded so `AUDIT_TYPE_NOT_CANONICAL` fires loudly, and this gate +// touches none of them. +// +// --------------------------------------------------------------------------- +// Ablation directions, predicted BEFORE running (results in the PR body) +// --------------------------------------------------------------------------- +// 1. Ship state predicted GREEN +// 2. The pre-flight block deleted predicted RED in §1 only +// (this leg IS the pre-fix measurement quoted above) +// 3. `willRestore &&` dropped from the gate — i.e. +// the soft-remove limb refused too predicted RED in §2 +// (proves §2 is a real constraint, not decoration) +// 4. Predicate swapped to the complete +// `canonicalMetaType(t) !== t` predicted RED in §3 +// (proves the narrow at-rest predicate is load-bearing: `objects` folds +// in the manifest map and is NOT this defect) + +import { describe, expect, it, vi } from 'vitest'; +import { PLURAL_TO_SINGULAR, canonicalMetaUrlType } from '@objectstack/spec/shared'; +// [#5619] The producer's OWN write-verb dispatch decisions, imported from +// `@objectstack/metadata-core` and NOT from `@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; + checksum?: string; + version?: number; +} + +interface HistoryRow { + id: string; + type: string; + name: string; + version: number; + organization_id: string | null; + operation_type: string; + metadata?: string | null; + recorded_at?: string; +} + +/** ADR-0048 overlay key — (type, name, org, state, package_id). */ +const keyOf = (w: Record) => + `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`; + +/** Top-level eq + `$or` + explicit-NULL, the subset these paths emit. */ +function matchesWhere(r: Row, where: Record): boolean { + for (const [k, v] of Object.entries(where)) { + if (k === '$or') { + const clauses = v as Array>; + if (!clauses.some((c) => matchesWhere(r, c))) return false; + continue; + } + if (v === undefined) continue; + if ((r as unknown as Record)[k] !== v) return false; + } + return true; +} + +/** One `sys_metadata_commit` row in the driver's snake_case wire shape. */ +const commitRow = (items: unknown[]) => ({ + id: 'c1', + commit_id: 'c1', + package_id: 'app.demo', + organization_id: null, + operation: 'apply', + message: 'the commit under revert', + created_at: '2026-01-01T00:00:00Z', + items: JSON.stringify(items), +}); + +function makeStubEngine() { + const rows = new Map(); + const historyRows: HistoryRow[] = []; + let nextId = 0; + /** Every `registerItem` key, in call order — the registry observation channel. */ + const registeredItems: Array<{ type: string; name: unknown }> = []; + /** Rows the revert appends to the ADR-0067 commit ledger. */ + const commitLedger: Array> = []; + /** Rows the revert appends to the ADR-0010 audit ledger (expected: none). */ + const auditLedger: Array> = []; + let commit: unknown = null; + + 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); + if (r) return { key: k, row: r }; + } + for (const [k, r] of rows) if (matchesWhere(r, w)) return { key: k, row: r }; + return null; + }; + + const matchesHistory = (h: HistoryRow, w: Record): boolean => { + if (w.organization_id !== undefined && h.organization_id !== w.organization_id) return false; + if (w.type !== undefined && h.type !== w.type) return false; + if (w.name !== undefined && h.name !== w.name) return false; + if (w.version !== undefined && h.version !== w.version) return false; + if (w.operation_type !== undefined && h.operation_type !== w.operation_type) return false; + return true; + }; + + const engine: any = { + async findOne(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_commit') return commit; + if (table === 'sys_metadata_history') { + return historyRows.find((h) => matchesHistory(h, opts.where)) ?? null; + } + return findRow(opts.where)?.row ?? null; + }, + async find(table: string, opts?: { where?: Record }) { + if (table === 'sys_metadata_history') { + return historyRows.filter((h) => matchesHistory(h, opts?.where ?? {})); + } + if (table !== 'sys_metadata') return []; + return Array.from(rows.values()).filter((r) => matchesWhere(r, opts?.where ?? {})); + }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_commit') { + commitLedger.push(data); + return { id: String(data.id ?? 'commit_row') }; + } + if (table === 'sys_metadata_audit') { + auditLedger.push(data); + return { id: 'audit_row' }; + } + if (table === 'sys_metadata_history') { + nextId += 1; + const h = { ...(data as unknown as HistoryRow), id: `h_${nextId}` }; + historyRows.push(h); + return { id: h.id }; + } + if (table !== 'sys_metadata') return { id: 'side_effect_skip' }; + nextId += 1; + const row = { ...(data as unknown as Row), id: `r_${nextId}` }; + 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), 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: unknown, info: { owned: boolean }) => Promise): Promise { + return cb(undefined, { owned: true }); + }, + async syncObjectSchema() { return true; }, + async dropObjectSchema() { return true; }, + registry: { + registerItem: (type: string, item: any) => { + registeredItems.push({ type, name: item?.name }); + }, + registerObject: () => undefined, + listItems: () => [], + getItem: () => undefined, + getObject: () => undefined, + getPackage: () => undefined, + getArtifactItem: () => undefined, + removeRuntimeShadow: () => false, + removeOverlayEntry: () => undefined, + }, + }; + return { + engine, + rows, + registeredItems, + commitLedger, + auditLedger, + serveCommit: (c: unknown) => { commit = c; }, + }; +} + +function makeProtocol() { + const h = makeStubEngine(); + // `environmentId` UNDEFINED — the unscoped (control-plane) kernel. On a + // scoped one `applyRegistryWriteThrough` returns before the hydration limb, + // so the pre-fix defect would not even be reachable to compare against. + const protocol = new ObjectStackProtocolImplementation(h.engine, () => new Map(), undefined) as any; + return { protocol, ...h }; +} + +/** + * Write a row through the REPOSITORY, which stamps `type` exactly as given — + * the only way to produce a row whose STORED spelling is non-canonical, since + * every `/meta` entry point folds before it persists. This is what a pre-#7894 + * row looks like at rest, and nothing rewrites it on upgrade. + */ +async function seedRowVerbatim( + protocol: any, + args: { type: string; name: string; body: unknown }, +): Promise { + await protocol.ensureOverlayIndex(); + const repo = protocol.getOverlayRepo(null); + const ref = { type: args.type, name: args.name, org: 'env' }; + // Successive writes to one identity build the lineage a revert restores + // from, so each must present the CURRENT head as its parent. + const existing = await repo.get(ref, { state: 'active' }); + await repo.put(ref, args.body, { + parentVersion: existing?.hash ?? null, + actor: null, + source: 'test.at-rest-residue', + intent: 'runtime-only', + state: 'active', + }); +} + +const bodyAt = (name: string, label: string) => ({ name, label }); + +/** The stored body of `type/name`, or `undefined` when no row is left. */ +const storedBody = (rows: Map, type: string, name: string): any => { + for (const r of rows.values()) { + if (r.type === type && r.name === name) return JSON.parse(r.metadata); + } + return undefined; +}; + +// ═══════════════════════════════════════════════════════════════════════════ +// 1. The RESTORE limb is refused, by name, on the wire +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#9174] revertCommit refuses a non-canonical stored type on the restore limb', () => { + it('answers a wire-visible `failed[].code` and restores nothing', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, registeredItems, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { + type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V1'), + }); + await seedRowVerbatim(protocol, { + type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V2'), + }); + registeredItems.length = 0; + serveCommit(commitRow([ + { type: 'fields', name: 'showcase_task.title', existedBefore: true, prevVersion: 1 }, + ])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + // The receipt, not merely "it did not restore": the code is what a + // caller filters on and what makes this door agree with its siblings. + expect(res.failed).toHaveLength(1); + expect(res.failed[0].code).toBe('STORED_TYPE_NOT_CANONICAL'); + expect(res.failedCount).toBe(1); + expect(res.revertedCount).toBe(0); + expect(res.reverted).toEqual([]); + // An item the operator asked to undo was not undone. + expect(res.success).toBe(false); + warn.mockRestore(); + }); + + it('names the row, the canonical type, and what to do about it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { + type: 'translations', name: 'legacy_bundle', body: bodyAt('legacy_bundle', 'V1'), + }); + await seedRowVerbatim(protocol, { + type: 'translations', name: 'legacy_bundle', body: bodyAt('legacy_bundle', 'V2'), + }); + serveCommit(commitRow([ + { type: 'translations', name: 'legacy_bundle', existedBefore: true, prevVersion: 1 }, + ])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + const text = String(res.failed[0].error); + // The stored spelling AND the canonical one — the operator cannot act + // on either alone. + expect(text).toContain("'translations/legacy_bundle'"); + expect(text).toContain(`'${canonicalMetaUrlType('translations')}'`); + // The actionable instruction the two sibling doors already give. + expect(text).toContain(`PUT /meta/${canonicalMetaUrlType('translations')}/legacy_bundle`); + // …and the pointer that the migrate door cannot fix it either (#8957), + // so the operator does not bounce between two doors. + expect(text).toContain('_migrate-stored'); + warn.mockRestore(); + }); + + it('leaves the row at rest untouched and the registry unwritten — and emits no warning', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, registeredItems, rows, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { + type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V1'), + }); + await seedRowVerbatim(protocol, { + type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V2'), + }); + registeredItems.length = 0; + serveCommit(commitRow([ + { type: 'fields', name: 'showcase_task.title', existedBefore: true, prevVersion: 1 }, + ])); + + await protocol.revertCommit({ commitId: 'c1' }); + + // Refused UP FRONT: the head body is still V2. Pre-fix this was V1 — + // the write happened and only the registry half was refused. + expect(storedBody(rows, 'fields', 'showcase_task.title')?.label).toBe('V2'); + expect(registeredItems).toEqual([]); + // The `console.warn` IS the defect this card names. Nothing on this + // path may reach `applyRegistryWriteThrough`'s best-effort catch now — + // asserted as "not one warning from this call", so a RENAMED warning + // cannot slip past a substring match. + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('carries the caller’s spelling into `failed[]` UNFOLDED (#9161)', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); + serveCommit(commitRow([{ type: 'fields', name: 'a.b', existedBefore: true, prevVersion: 1 }])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + // The refusal must not "helpfully" fold: the item's identity in the + // receipt is the spelling the row really carries, which is the fact the + // operator has to act on. Folding here is what #9161 ruled against on + // the ledger keys, and the same reasoning holds for the receipt. + expect(res.failed[0].type).toBe('fields'); + expect(res.failed[0].name).toBe('a.b'); + warn.mockRestore(); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 2. The SOFT-REMOVE limb is deliberately outside the gate +// ═══════════════════════════════════════════════════════════════════════════ +// +// Ablation 3 is this section's reason to exist: dropping `willRestore &&` from +// the gate is the "obvious simplification", and it costs the operator the one +// operation that reduces this residue. + +describe('[#9174] the soft-remove limb keeps reverting, and says so', () => { + it('removes a commit-created non-canonical row and reports success', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, rows, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { + type: 'fields', name: 'legacy_only', body: bodyAt('legacy_only', 'V1'), + }); + serveCommit(commitRow([ + { type: 'fields', name: 'legacy_only', existedBefore: false, prevVersion: null }, + ])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + expect(res.failed).toEqual([]); + expect(res.reverted).toEqual([{ type: 'fields', name: 'legacy_only', action: 'removed' }]); + expect(res.success).toBe(true); + // The row is GONE — the undo the operator asked for, performed in full, + // and the one action that makes this residue smaller. + expect(storedBody(rows, 'fields', 'legacy_only')).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 3. The gate's edges — what it must NOT refuse +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { + it('a canonical type still reverts and still registers under its own key', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, registeredItems, rows, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V1') }); + await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V2') }); + registeredItems.length = 0; + serveCommit(commitRow([{ type: 'view', name: 'grid', existedBefore: true, prevVersion: 1 }])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + expect(res.failed).toEqual([]); + expect(res.success).toBe(true); + expect(storedBody(rows, 'view', 'grid')?.label).toBe('V1'); + // Non-vacuity for the whole file: the write-through really does run on + // this path, so section 1's empty `registeredItems` is a refusal rather + // than a harness that never registers anything. + expect(registeredItems).toEqual([{ type: 'view', name: 'grid' }]); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('a manifest-PRESENT plural is NOT refused — it is not this defect', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, registeredItems, serveCommit } = makeProtocol(); + // External dependency of the verdict, pinned rather than assumed: the + // manifest fold resolves `views`, so the restore limb hands the + // write-through the CANONICAL key and the entry lands correctly. + expect(PLURAL_TO_SINGULAR.views).toBe('view'); + await seedRowVerbatim(protocol, { type: 'views', name: 'legacy_grid', body: bodyAt('legacy_grid', 'V1') }); + await seedRowVerbatim(protocol, { type: 'views', name: 'legacy_grid', body: bodyAt('legacy_grid', 'V2') }); + registeredItems.length = 0; + serveCommit(commitRow([{ type: 'views', name: 'legacy_grid', existedBefore: true, prevVersion: 1 }])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + // Widening the predicate to the complete `canonicalMetaType(t) !== t` + // would change a wire-visible `failed[].code` for rows that are NOT + // this defect — #8908's own scoping argument, one door over. + expect(res.failed).toEqual([]); + expect(res.reverted).toEqual([{ type: 'views', name: 'legacy_grid', action: 'restored' }]); + expect(registeredItems).toEqual([{ type: 'view', name: 'legacy_grid' }]); + warn.mockRestore(); + }); + + it('the fold-map disagreement the gate rests on is real', () => { + // The gate's value dies the day these two maps stop disagreeing, so the + // disagreement is pinned here rather than assumed. Filtered against the + // live map — a hand-written list ships with members missing (#8908 + // measured exactly that) and no way to notice. + const blind = ['fields', 'seeds', 'external_catalogs', 'externalCatalogs', 'translations', 'email_templates'] + .filter((s) => (PLURAL_TO_SINGULAR[s] ?? s) === s); + expect(blind.length).toBeGreaterThan(0); + for (const spelling of blind) { + expect(canonicalMetaUrlType(spelling), `${spelling} must fold at the URL map`).not.toBe(spelling); + } + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// 4. The ledgers, and the neighbours +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#9174] a refused item touches no ledger key and stops no neighbour', () => { + it('is absent from the append-only revert commit, which records only what was reverted', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, commitLedger, auditLedger, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); + serveCommit(commitRow([{ type: 'fields', name: 'a.b', existedBefore: true, prevVersion: 1 }])); + + await protocol.revertCommit({ commitId: 'c1' }); + + // The revert commit is built from `reverted[]`, so a refused item is + // simply not in it — the ledger never claims an undo that did not + // happen, and no spelling was folded to put it there. + expect(commitLedger).toHaveLength(1); + expect(JSON.parse(String(commitLedger[0].items))).toEqual([]); + // ⛔ And no audit row: this function writes none for ANY of its per-item + // failures, and a row minted only for this class would need the type + // FOLDED to be readable — the one move #9161 rules out here. + expect(auditLedger).toEqual([]); + warn.mockRestore(); + }); + + it('reverts the neighbours of a refused item and reports the mix honestly', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const { protocol, rows, commitLedger, serveCommit } = makeProtocol(); + await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V1') }); + await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V2') }); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); + await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); + serveCommit(commitRow([ + { type: 'view', name: 'grid', existedBefore: true, prevVersion: 1 }, + { type: 'fields', name: 'a.b', existedBefore: true, prevVersion: 1 }, + ])); + + const res = await protocol.revertCommit({ commitId: 'c1' }); + + // Per ITEM, like every other verdict in this loop: one refusal does not + // abort the batch (that is the publish door's ADR-0067 D2 posture, and + // this door has no transaction to be atomic over). + expect(res.revertedCount).toBe(1); + expect(res.failedCount).toBe(1); + expect(res.success).toBe(false); + expect(storedBody(rows, 'view', 'grid')?.label).toBe('V1'); + expect(storedBody(rows, 'fields', 'a.b')?.label).toBe('V2'); + expect(JSON.parse(String(commitLedger[0].items))).toEqual([ + { type: 'view', name: 'grid', existedBefore: true, prevVersion: null }, + ]); + warn.mockRestore(); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f247ef9c56..f72725f9ab 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11447,7 +11447,9 @@ export class ObjectStackProtocolImplementation implements * boundary, and `publishPackageDrafts` is pre-empted by #8908's * `STORED_TYPE_NOT_CANONICAL` pre-flight ({@link isNonCanonicalStoredType}). * 5. `revertCommit` → write-through — `PLURAL_TO_SINGULAR[it.type]` - * over a STORED commit-item type. **Unguarded.** + * over a STORED commit-item type. Was **unguarded**; since #9174 the + * restore limb carries its own `STORED_TYPE_NOT_CANONICAL` pre-flight, + * so this producer no longer reaches the assert for that class. * 6. `loadMetaFromDb` (boot) — `PLURAL_TO_SINGULAR[record.type]` * over a STORED row type. **Unguarded.** * @@ -11484,7 +11486,10 @@ export class ObjectStackProtocolImplementation implements * {@link applyRegistryWriteThrough}'s best-effort `console.warn`, which is * correct there and not a softened assert — the row is already persisted, * and #4521's contract is that a registry hiccup must never fail a - * committed write. + * committed write. [#9174] Correct, and still less informative than the two + * at-rest doors that name this class on the wire — which is why + * {@link revertCommit}, the one producer that could reach this seam with the + * class, now refuses it up front instead of arriving here. * * ⚠️ This assert touches NO audit row, NO commit record and NO repository * key. It is scoped to the registry mint door, and no producer's spelling @@ -16152,6 +16157,16 @@ export class ObjectStackProtocolImplementation implements * dispatching the reverted-away body until restart — * {@link rollbackToPackageCommit} inherited it, so a whole-package * rollback could report success and change nothing the process could see. + * + * [#9174] An item whose STORED `type` is non-canonical + * ({@link isNonCanonicalStoredType}) is REFUSED on the restore limb, into + * `failed[]` with `code: 'STORED_TYPE_NOT_CANONICAL'` — the same wire-visible + * verdict the sibling at-rest doors give (`publishPackageDrafts`, #8908; + * `migrateStoredMetadata`, #8957), in place of the `console.warn` this door + * degraded to. The soft-remove limb is deliberately outside the gate: it + * performs its promise exactly, and the removal is the one action that + * reduces the residue. The measurement and the refuse-vs-decline reasoning + * are stated at the gate itself. */ async revertCommit(request: { commitId: string; @@ -16231,6 +16246,117 @@ export class ObjectStackProtocolImplementation implements // Reverse apply order so artifacts that depend on others (e.g. a view on // a new object) are removed before the thing they reference. for (const it of [...items].reverse()) { + // Which limb this item takes, bound ONCE (#9174): the pre-flight + // below and the `else if` further down must never drift apart — + // a gate that guards a limb it no longer describes is worse than + // no gate, because it reads as covered. + const willRestore = it.existedBefore + && it.prevVersion !== null && it.prevVersion !== undefined; + + // ═══ [#9174] The AT-REST spelling pre-flight, on the RESTORE limb ══ + // + // The third consumer of an at-rest `type` in this file, and the last + // one without this gate. `publishPackageDrafts` refuses the class by + // name (`STORED_TYPE_NOT_CANONICAL`, #8908) and `migrateStoredMetadata` + // reports it `skipped` with the same reason (#8957); this door gave it + // a server-side `console.warn` and a receipt that said `restored`. + // + // ── What was MEASURED at HEAD, per limb, before choosing a shape ─── + // + // Driven end to end over the real `SysMetadataRepository` with a + // `fields/showcase_task.title` row at rest (`protocol.revert-stored- + // type-canonical.test.ts` carries the harness): + // + // RESTORE limb — `{success: true, revertedCount: 1, failed: []}`, + // `reverted[0].action === 'restored'`, `registerItem` called ZERO + // times, and the only trace anywhere is + // `[Protocol] registry write-through failed for fields/… : + // [registry_type_not_canonical] …` on the server's stderr. The + // receipt claims the pre-commit body is what the platform now + // serves. It is not, and for this class it cannot be: #9111's mint + // door refuses the entry and boot refuses it too, so the restored + // body reaches no reader at all. + // + // SOFT-REMOVE limb — `{success: true, action: 'removed'}`, the row + // is GONE from `sys_metadata`, no warning is emitted and no registry + // key is touched (`restoreArtifactRegistryView` folds through the + // manifest map, a no-op for this class, and addresses only the + // phantom namespace). Nothing about that outcome is wrong. + // + // ── Which sibling this matches, and why it is REFUSE not DECLINE ─── + // + // `saveMetaItem`'s: a wire-visible coded refusal, carried on THIS + // door's existing per-item `failed[]` channel — the same channel every + // other refusal here already uses (`VERSION_NOT_FOUND`, `ITEM_LOCKED`, + // `NOT_OVERRIDABLE`). No new receipt surface, no new error code. + // + // The test that separates the two siblings is whether the door can do + // what it promises for this row. `migrateStoredMetadata` DECLINES + // because rewriting a stored type spelling is an identity move and is + // out of that pass's reach entirely — nothing is refused, so `skipped` + // must not poison `storedMigrationClean` for a scan that runs forever. + // Here the write is squarely IN reach: the restore would succeed at the + // row and still deliver none of what `restored` promises. That is + // `saveMetaItem`'s case — a write this door is able to perform and must + // not — so it is refused, and `success` goes false because the commit + // the operator asked to undo was not undone. A one-shot operator action + // has no forever to poison; answering `success: true` over a + // deliberately untouched item is the #5980 class of lie this file keeps + // paying for. + // + // ⛔ Deliberately NOT on the soft-remove limb, and this is a scope + // decision rather than a softened gate. That limb performs, exactly and + // completely, the one action that improves this residue: the row goes + // away. Refusing it would answer `success: false` for a revert that + // fully succeeded, and would hand the operator an instruction ("drop + // the '' row") naming the very operation it had just declined + // to perform. #8908's own predicate is scoped the same way for the same + // reason — it excludes the manifest-PRESENT plurals because those are + // already fail-closed, since "widening this gate would change a + // wire-visible `failed[].code` for rows that are NOT this defect". + // + // ⛔ NO fold, anywhere. The refusal writes no audit row and no commit + // record, and `it.type` is carried into `failed[]` verbatim, so #9161's + // ruling — the caller's spelling reaches the ledger keys unfolded, and + // #8908's `AUDIT_TYPE_NOT_CANONICAL` fires loudly when it is wrong — is + // untouched in both directions. The refused item is simply absent from + // `reverted[]`, so the append-only revert commit built from it below + // does not claim to have reverted what it did not. + // + // ⛔ And NOT an audit row either, unlike the publish pre-flight's. + // That door audits because every other refusal on that route audits; + // this one writes no audit row for ANY of its per-item failures, so a + // row minted only for this class would be a lone entry in a ledger the + // function otherwise never writes — and it would need the type FOLDED + // to be readable, which is the one thing the card rules out. + // + // The predicate is `isNonCanonicalStoredType`, NOT the complete + // `canonicalMetaType(t) !== t`: `objects` folds in the manifest map, so + // the restore limb hands the write-through the canonical key and the + // registry entry lands correctly (pinned, one file over, in + // `protocol.object-registry-write-through-spelling.test.ts`). Refusing + // it would be a wire-visible change for rows that are not this defect. + if (willRestore && isNonCanonicalStoredType(it.type)) { + const canonical = canonicalMetaType(it.type); + failed.push({ + type: it.type, + name: it.name, + error: `Commit item '${it.type}/${it.name}' is stored under the non-canonical ` + + `metadata type '${it.type}'; the canonical type for this item is ` + + `'${canonical}'. Restoring it would write the pre-commit body back into a ` + + `second namespace that no registry read and no compliance query on ` + + `'${canonical}' can see (#7894 closed this namespace at the '/meta' URL ` + + `door; this row predates that), and the registry refuses to serve it ` + + `(REGISTRY_TYPE_NOT_CANONICAL), so the restored body would reach no reader. ` + + `Re-author the item under '${canonical}' (PUT /meta/${canonical}/${it.name}) ` + + `and drop the '${it.type}' row. Note that POST /meta/_migrate-stored does NOT ` + + `rewrite a stored type spelling — it canonicalizes bodies, and reports rows ` + + `of this class as 'skipped' with that same reason (#8957).`, + code: 'STORED_TYPE_NOT_CANONICAL', + }); + continue; + } + // [#7559] PER ITEM, and from the ROW rather than from the request — // the same shape {@link publishPackageDrafts} already uses when it // promotes each draft in the draft's OWN scope and captures @@ -16350,7 +16476,7 @@ export class ObjectStackProtocolImplementation implements // caller skipped the heal entirely while answering success. await this.restoreArtifactRegistryView(it.type, it.name, itemOrgId); reverted.push({ type: it.type, name: it.name, action: 'removed' }); - } else if (it.prevVersion !== null && it.prevVersion !== undefined) { + } else if (willRestore) { // Edited an existing artifact → restore the pre-commit body. // // [#6563] The write INTENT is derived per item, exactly as the From 52d1cd13b6d7e129141f24f8b3a19e393278869b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 15:53:25 +0000 Subject: [PATCH 2/3] fix(metadata-protocol): bind the restore-limb guard as a narrowable const (#9174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hoisted boolean broke TypeScript's narrowing on `it.prevVersion` — a MUTABLE property, so a narrowing does not survive an aliased boolean — and `repo.restoreVersion(ref, it.prevVersion, …)` stopped compiling with `TS2345: 'number | null' is not assignable to 'number'`. Caught by the DTS build while all 1651 package tests stayed green, because vitest transpiles without type-checking. `restoreToVersion` is the same single source of truth for both the pre-flight and the branch, and narrows. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- ...tocol.revert-stored-type-canonical.test.ts | 79 ++++++++++++------- packages/metadata-protocol/src/protocol.ts | 29 ++++--- 2 files changed, 70 insertions(+), 38 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts index 5c24de08d7..debf8d379d 100644 --- a/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts +++ b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts @@ -77,20 +77,33 @@ // touches none of them. // // --------------------------------------------------------------------------- -// Ablation directions, predicted BEFORE running (results in the PR body) +// Ablation directions, predicted BEFORE running — and what actually happened // --------------------------------------------------------------------------- -// 1. Ship state predicted GREEN -// 2. The pre-flight block deleted predicted RED in §1 only -// (this leg IS the pre-fix measurement quoted above) -// 3. `willRestore &&` dropped from the gate — i.e. -// the soft-remove limb refused too predicted RED in §2 -// (proves §2 is a real constraint, not decoration) +// 1. Ship state predicted GREEN -> GREEN (10/10) +// 2. The pre-flight block deleted predicted RED §1 -> RED, 6 failed +// ⚠️ BROADER than predicted, and the prediction was the wrong shape: §4 +// goes red with §1, because a gate that refuses nothing lets the item back +// into `reverted[]` and therefore into the append-only revert commit. That +// is the leg's own point, so it is recorded as observed rather than +// trimmed to the prediction. §2 and §3 stayed GREEN, which is the half +// that matters: removing the gate changes nothing about the soft-remove +// limb or about the rows that are not this defect. +// This leg IS the pre-fix measurement quoted above. +// ⚠️ It also ran RED-but-unreadable the first time, on eight tests, for a +// reason that had nothing to do with the ablation — see `spyWarn` below. +// 3. `restoreToVersion !== null &&` dropped +// from the gate — the soft-remove limb +// refused too predicted RED §2 -> RED, exactly 1 +// failed: "removes a commit-created non-canonical row and reports +// success". The obvious simplification, and this is its price. // 4. Predicate swapped to the complete -// `canonicalMetaType(t) !== t` predicted RED in §3 -// (proves the narrow at-rest predicate is load-bearing: `objects` folds -// in the manifest map and is NOT this defect) +// `canonicalMetaType(t) !== t` predicted RED §3 -> RED, exactly 1 +// failed: "a manifest-PRESENT plural is NOT refused". The narrow at-rest +// predicate is load-bearing — `views`/`objects` fold in the manifest map, +// so those rows revert AND register correctly today and are not this +// defect. -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { PLURAL_TO_SINGULAR, canonicalMetaUrlType } from '@objectstack/spec/shared'; // [#5619] The producer's OWN write-verb dispatch decisions, imported from // `@objectstack/metadata-core` and NOT from `@objectstack/objectql`: objectql @@ -312,13 +325,30 @@ const storedBody = (rows: Map, type: string, name: string): any => return undefined; }; +/** + * Silence and observe `console.warn` — the channel this card is about. + * + * ⛔ Restored from `afterEach`, never from the test body. A body-tail + * `mockRestore()` is skipped the moment an assertion above it throws, so the + * spy survives into the NEXT test and accumulates its calls — measured here + * while running ablation 2: four tests that have nothing to do with the + * ablation went red on warnings emitted by earlier ones, and the ablation + * direction was unreadable until this was fixed. A leg that cannot be read is + * a leg that was not run. + */ +const spyWarn = () => vi.spyOn(console, 'warn').mockImplementation(() => undefined); + +afterEach(() => { + vi.restoreAllMocks(); +}); + // ═══════════════════════════════════════════════════════════════════════════ // 1. The RESTORE limb is refused, by name, on the wire // ═══════════════════════════════════════════════════════════════════════════ describe('[#9174] revertCommit refuses a non-canonical stored type on the restore limb', () => { it('answers a wire-visible `failed[].code` and restores nothing', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, registeredItems, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V1'), @@ -342,11 +372,10 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor expect(res.reverted).toEqual([]); // An item the operator asked to undo was not undone. expect(res.success).toBe(false); - warn.mockRestore(); }); it('names the row, the canonical type, and what to do about it', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'translations', name: 'legacy_bundle', body: bodyAt('legacy_bundle', 'V1'), @@ -370,11 +399,10 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor // …and the pointer that the migrate door cannot fix it either (#8957), // so the operator does not bounce between two doors. expect(text).toContain('_migrate-stored'); - warn.mockRestore(); }); it('leaves the row at rest untouched and the registry unwritten — and emits no warning', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, registeredItems, rows, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V1'), @@ -398,11 +426,10 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor // asserted as "not one warning from this call", so a RENAMED warning // cannot slip past a substring match. expect(warn).not.toHaveBeenCalled(); - warn.mockRestore(); }); it('carries the caller’s spelling into `failed[]` UNFOLDED (#9161)', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); @@ -416,7 +443,6 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor // the ledger keys, and the same reasoning holds for the receipt. expect(res.failed[0].type).toBe('fields'); expect(res.failed[0].name).toBe('a.b'); - warn.mockRestore(); }); }); @@ -430,7 +456,7 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor describe('[#9174] the soft-remove limb keeps reverting, and says so', () => { it('removes a commit-created non-canonical row and reports success', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, rows, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'legacy_only', body: bodyAt('legacy_only', 'V1'), @@ -448,7 +474,6 @@ describe('[#9174] the soft-remove limb keeps reverting, and says so', () => { // and the one action that makes this residue smaller. expect(storedBody(rows, 'fields', 'legacy_only')).toBeUndefined(); expect(warn).not.toHaveBeenCalled(); - warn.mockRestore(); }); }); @@ -458,7 +483,7 @@ describe('[#9174] the soft-remove limb keeps reverting, and says so', () => { describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { it('a canonical type still reverts and still registers under its own key', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, registeredItems, rows, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V1') }); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V2') }); @@ -475,11 +500,10 @@ describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { // than a harness that never registers anything. expect(registeredItems).toEqual([{ type: 'view', name: 'grid' }]); expect(warn).not.toHaveBeenCalled(); - warn.mockRestore(); }); it('a manifest-PRESENT plural is NOT refused — it is not this defect', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, registeredItems, serveCommit } = makeProtocol(); // External dependency of the verdict, pinned rather than assumed: the // manifest fold resolves `views`, so the restore limb hands the @@ -498,7 +522,6 @@ describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { expect(res.failed).toEqual([]); expect(res.reverted).toEqual([{ type: 'views', name: 'legacy_grid', action: 'restored' }]); expect(registeredItems).toEqual([{ type: 'view', name: 'legacy_grid' }]); - warn.mockRestore(); }); it('the fold-map disagreement the gate rests on is real', () => { @@ -521,7 +544,7 @@ describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { describe('[#9174] a refused item touches no ledger key and stops no neighbour', () => { it('is absent from the append-only revert commit, which records only what was reverted', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, commitLedger, auditLedger, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); @@ -538,11 +561,10 @@ describe('[#9174] a refused item touches no ledger key and stops no neighbour', // failures, and a row minted only for this class would need the type // FOLDED to be readable — the one move #9161 rules out here. expect(auditLedger).toEqual([]); - warn.mockRestore(); }); it('reverts the neighbours of a refused item and reports the mix honestly', async () => { - const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const warn = spyWarn(); const { protocol, rows, commitLedger, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V1') }); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V2') }); @@ -566,6 +588,5 @@ describe('[#9174] a refused item touches no ledger key and stops no neighbour', expect(JSON.parse(String(commitLedger[0].items))).toEqual([ { type: 'view', name: 'grid', existedBefore: true, prevVersion: null }, ]); - warn.mockRestore(); }); }); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index f72725f9ab..dc79798cf9 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -16246,12 +16246,23 @@ export class ObjectStackProtocolImplementation implements // Reverse apply order so artifacts that depend on others (e.g. a view on // a new object) are removed before the thing they reference. for (const it of [...items].reverse()) { - // Which limb this item takes, bound ONCE (#9174): the pre-flight - // below and the `else if` further down must never drift apart — - // a gate that guards a limb it no longer describes is worse than - // no gate, because it reads as covered. - const willRestore = it.existedBefore - && it.prevVersion !== null && it.prevVersion !== undefined; + // The version this item restores to, or `null` when it does not take + // the restore limb at all — bound ONCE (#9174), because the pre-flight + // below and the `else if` further down must never drift apart: a gate + // that guards a limb it no longer describes is worse than no gate, + // since it reads as covered. + // + // A `const` local rather than a hoisted boolean, deliberately, and this + // is a compiler fact rather than taste: `it.prevVersion` is a MUTABLE + // property, so TypeScript does not carry a narrowing on it through an + // aliased boolean, and `repo.restoreVersion(ref, it.prevVersion, …)` + // below stops compiling (`TS2345: 'number | null' is not assignable to + // 'number'`). Measured — the DTS build caught it while all 1651 package + // tests stayed green, because vitest transpiles without type-checking. + const restoreToVersion: number | null = + it.existedBefore && it.prevVersion !== null && it.prevVersion !== undefined + ? it.prevVersion + : null; // ═══ [#9174] The AT-REST spelling pre-flight, on the RESTORE limb ══ // @@ -16336,7 +16347,7 @@ export class ObjectStackProtocolImplementation implements // registry entry lands correctly (pinned, one file over, in // `protocol.object-registry-write-through-spelling.test.ts`). Refusing // it would be a wire-visible change for rows that are not this defect. - if (willRestore && isNonCanonicalStoredType(it.type)) { + if (restoreToVersion !== null && isNonCanonicalStoredType(it.type)) { const canonical = canonicalMetaType(it.type); failed.push({ type: it.type, @@ -16476,7 +16487,7 @@ export class ObjectStackProtocolImplementation implements // caller skipped the heal entirely while answering success. await this.restoreArtifactRegistryView(it.type, it.name, itemOrgId); reverted.push({ type: it.type, name: it.name, action: 'removed' }); - } else if (willRestore) { + } else if (restoreToVersion !== null) { // Edited an existing artifact → restore the pre-commit body. // // [#6563] The write INTENT is derived per item, exactly as the @@ -16521,7 +16532,7 @@ export class ObjectStackProtocolImplementation implements // the shape that ends in a `catch {}` swallowing a real outage // (#4867). Per ITEM, because a batch mixes bindings. const restorePackageId = await this.resolveOverlayPackageBinding(it.type, it.name, itemOrgId); - const restored = await repo.restoreVersion(ref, it.prevVersion, { + const restored = await repo.restoreVersion(ref, restoreToVersion, { actor, source: 'protocol.revertCommit', message: `revert commit ${request.commitId}`, From c40b70024c8b060906fcc8779352d9e6cd5e73a0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 16:17:50 +0000 Subject: [PATCH 3/3] test(metadata-protocol): drop the unused warn bindings the afterEach restore left behind (#9174) Moving the spy restore into `afterEach` (so a failing assertion cannot leak the spy into the next test) left six `const warn = spyWarn()` bindings unread. tsc counted them (TS6133) and the shrink-only DEBT ratchet for this package went 63 -> 69. Back to 63. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NTKPDRoynY8i3HmdSFUxFj --- .../protocol.revert-stored-type-canonical.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts index debf8d379d..3dc3bac68b 100644 --- a/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts +++ b/packages/metadata-protocol/src/protocol.revert-stored-type-canonical.test.ts @@ -348,7 +348,7 @@ afterEach(() => { describe('[#9174] revertCommit refuses a non-canonical stored type on the restore limb', () => { it('answers a wire-visible `failed[].code` and restores nothing', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, registeredItems, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'showcase_task.title', body: bodyAt('showcase_task.title', 'V1'), @@ -375,7 +375,7 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor }); it('names the row, the canonical type, and what to do about it', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'translations', name: 'legacy_bundle', body: bodyAt('legacy_bundle', 'V1'), @@ -429,7 +429,7 @@ describe('[#9174] revertCommit refuses a non-canonical stored type on the restor }); it('carries the caller’s spelling into `failed[]` UNFOLDED (#9161)', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); @@ -503,7 +503,7 @@ describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { }); it('a manifest-PRESENT plural is NOT refused — it is not this defect', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, registeredItems, serveCommit } = makeProtocol(); // External dependency of the verdict, pinned rather than assumed: the // manifest fold resolves `views`, so the restore limb hands the @@ -544,7 +544,7 @@ describe('[#9174] the pre-flight fires on exactly the at-rest class', () => { describe('[#9174] a refused item touches no ledger key and stops no neighbour', () => { it('is absent from the append-only revert commit, which records only what was reverted', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, commitLedger, auditLedger, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V1') }); await seedRowVerbatim(protocol, { type: 'fields', name: 'a.b', body: bodyAt('a.b', 'V2') }); @@ -564,7 +564,7 @@ describe('[#9174] a refused item touches no ledger key and stops no neighbour', }); it('reverts the neighbours of a refused item and reports the mix honestly', async () => { - const warn = spyWarn(); + spyWarn(); const { protocol, rows, commitLedger, serveCommit } = makeProtocol(); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V1') }); await seedRowVerbatim(protocol, { type: 'view', name: 'grid', body: bodyAt('grid', 'V2') });