diff --git a/.changeset/29414-commit-revert-own-package-row.md b/.changeset/29414-commit-revert-own-package-row.md new file mode 100644 index 00000000000..e3707fa8387 --- /dev/null +++ b/.changeset/29414-commit-revert-own-package-row.md @@ -0,0 +1,43 @@ +--- +'@objectstack/metadata-protocol': patch +'@objectstack/spec': patch +--- + +fix(metadata-protocol)!: a package commit's revert plan and its revert address the committing package's own row, and a restore into a `.custom` pair's row is judged by the override write gate + +Clause-②: no (narrowing) — the commit revert (`POST /api/v1/packages/:id/commits/:commitId/revert`) and the package rollback (`POST /api/v1/packages/:id/rollback`) no longer restore a stored version into a `.custom` pair's row unless the version declares the override the pair's save requires, and no longer restore into another package's row of the name, or into a package-less row, when the committing package's own row was deleted since: they restore into the committing package's row, so a commit of an installed read-only package itself whose row is gone is refused by the close naming the pair. They restored into whichever row of the name the store returned first, or inserted a package-less row, before. + +Metadata-protocol: none + +Skills-docs: none needed — `skills/objectstack-platform/SKILL.md` places every customization of an installed base in its paired `.custom`, declaring `overrides`, and says a pair row is honoured only when declared; nothing it says becomes false. + + + +**BREAKING** write narrowing at the commit doors (`@objectstack/metadata-protocol`), shipped as `patch` (`Metadata-protocol: none`). `@objectstack/spec` carries the text widening of the `packageless-overlay-write-closed` upgrade entry. + +The plan a package publish records (`POST /api/v1/packages/:id/publish-drafts`, the commit's `existedBefore` and `prevVersion` per item) now reads the publishing package's own active row, and the revert removes or restores that row only. Before, both read any package's active row of the name: the first publish of a `.custom` pair's declared override beside a row bound to the base (stored before the write naming the base was closed) was recorded as an edit of the base row's version, and its revert addressed the base row and was refused there instead of removing the pair's row. It now removes the pair's row, and the row bound to the base is served again. + +A pair commit recorded before this change beside such a row still names the base row's version as the pair's pre-commit state. Its revert now restores into the pair's own row, so the restored body meets the override write gate exactly as the pair's per-item rollback does: the base row's body declares no override, so the item is refused per item (`403 NOT_OVERRIDABLE`, the save door's sentence) and the pair's declared row stays as it is. + +### Migration: FROM → TO + +FROM, undoing a pair commit recorded before this change beside a row bound to the installed package `com.acme.crm`: + +```http +POST /api/v1/packages/com.acme.crm.custom/commits/cmt_123/revert +``` + +It answers `200` with `success: false`, and the item in `failed[]` with `code: NOT_OVERRIDABLE`; nothing is written. TO, write the version you want into the pair, declaring what it overrides, or remove the pair's row to serve what is under it: + +```http +PUT /api/v1/meta/view/account_list?package=com.acme.crm.custom +{ "name": "account_list", "label": "My accounts", "overrides": { "package": "com.acme.crm", "basedOn": "1.4.0" } } + +DELETE /api/v1/meta/view/account_list?package=com.acme.crm.custom +``` + +FROM, undoing a commit of `com.acme.crm` itself (recorded before the write naming it was closed) that edited a customization whose row was deleted since: the restore wrote a package-less row. It now restores into the row bound to `com.acme.crm`, which the close refuses (`NOT_OVERRIDABLE`, the rollback sentence naming the pair); TO, the same `PUT …?package=com.acme.crm.custom` with the `overrides` declaration. + +- **Now refused, per item.** A restore into a `.custom` pair's row of a version that does not declare the override the pair's save requires (the override write gate). A restore of a commit of a read-only package itself into its row bound to the package, now also when that row was deleted since (the close naming the pair). The commit's other items still revert, and `POST /api/v1/packages//rollback` reports each refused item in its `failed[]` and continues. +- **Now restored into the committing package's row.** A commit whose row was deleted since, of a writable package or of the pair (a declared version): the row is created again under that package, never as a package-less row. +- **Still open.** Every restore into a row the committing package holds that the gate admits (a writable package's in place, the pair's declared version in place), and every removal. diff --git a/packages/metadata-protocol/src/protocol.extension-field-suffix.test.ts b/packages/metadata-protocol/src/protocol.extension-field-suffix.test.ts index 09842c97500..5a9c6f000fc 100644 --- a/packages/metadata-protocol/src/protocol.extension-field-suffix.test.ts +++ b/packages/metadata-protocol/src/protocol.extension-field-suffix.test.ts @@ -306,9 +306,15 @@ async function attempt(run: () => Promise): Promise<{ result: T | null; er afterEach(() => vi.restoreAllMocks()); -const commitRow = (items: unknown[]) => ({ +/** + * [#29414] `packageId` — the package the commit is recorded for, which a + * revert addresses the rows of; omitted, no `package_id` (the package-less + * rows those cases seed). + */ +const commitRow = (items: unknown[], packageId?: string) => ({ id: 'c1', commit_id: 'c1', + ...(packageId !== undefined ? { package_id: packageId } : {}), organization_id: null, operation: 'apply', message: 'the commit under revert', @@ -457,7 +463,7 @@ describe('[ADR-0146 D10] the restore doors refuse a stored version carrying a `_ it('revertCommit\'s restore limb reports it in failed[] with the envelope\'s code; nothing is written', async () => { const h = makeProtocol(); const before = await threeVersionsWithFieldResidueAtV2(h, BOUND); - h.serveCommit(commitRow([{ type: 'object', name: 'probe_ticket', existedBefore: true, prevVersion: 2 }])); + h.serveCommit(commitRow([{ type: 'object', name: 'probe_ticket', existedBefore: true, prevVersion: 2 }], BOUND)); vi.spyOn(console, 'warn').mockImplementation(() => {}); const res = await h.protocol.revertCommit({ commitId: 'c1' }); diff --git a/packages/metadata-protocol/src/protocol.item-name-every-door.test.ts b/packages/metadata-protocol/src/protocol.item-name-every-door.test.ts index d859bbfa757..c60b0c2b7ce 100644 --- a/packages/metadata-protocol/src/protocol.item-name-every-door.test.ts +++ b/packages/metadata-protocol/src/protocol.item-name-every-door.test.ts @@ -391,9 +391,15 @@ async function threeVersionsWithResidueAtV2(h: Harness) { return { historyBefore: h.historyRows.length, activeBefore: JSON.stringify(stored(h, 'dashboard')) }; } -const commitRow = (items: unknown[]) => ({ +/** + * [#29414] `packageId` — the package the commit is recorded for, which a + * revert addresses the rows of; omitted, no `package_id` (the package-less + * rows those cases seed). + */ +const commitRow = (items: unknown[], packageId?: string) => ({ id: 'c1', commit_id: 'c1', + ...(packageId !== undefined ? { package_id: packageId } : {}), organization_id: null, operation: 'apply', message: 'the commit under revert', @@ -794,7 +800,7 @@ describe('[ADR-0086 D3] the restore doors refuse a stored version whose body `pa it('revertCommit\'s restore limb reports a disagreeing version in failed[] with the envelope\'s code; nothing is written', async () => { const h = makeProtocol(); const before = await threeSetVersionsWithOwnerResidueAtV2(h, BOUND, OTHER); - h.serveCommit(commitRow([{ type: 'permission', name: 'probe_member', existedBefore: true, prevVersion: 2 }])); + h.serveCommit(commitRow([{ type: 'permission', name: 'probe_member', existedBefore: true, prevVersion: 2 }], BOUND)); vi.spyOn(console, 'warn').mockImplementation(() => {}); const res = await h.protocol.revertCommit({ commitId: 'c1' }); @@ -812,7 +818,7 @@ describe('[ADR-0086 D3] the restore doors refuse a stored version whose body `pa it('CONTROL: revertCommit\'s restore limb restores a version whose body agrees with the binding', async () => { const h = makeProtocol(); await threeSetVersionsWithOwnerResidueAtV2(h, BOUND, BOUND); - h.serveCommit(commitRow([{ type: 'permission', name: 'probe_member', existedBefore: true, prevVersion: 2 }])); + h.serveCommit(commitRow([{ type: 'permission', name: 'probe_member', existedBefore: true, prevVersion: 2 }], BOUND)); const res = await h.protocol.revertCommit({ commitId: 'c1' }); diff --git a/packages/metadata-protocol/src/protocol.named-base-overlay-write.test.ts b/packages/metadata-protocol/src/protocol.named-base-overlay-write.test.ts index aa17477e781..4a79bb8f1c1 100644 --- a/packages/metadata-protocol/src/protocol.named-base-overlay-write.test.ts +++ b/packages/metadata-protocol/src/protocol.named-base-overlay-write.test.ts @@ -476,4 +476,119 @@ describe('the commit doors: restoring a row bound to the base is closed, removin expect(rows(VIEW.type, VIEW.name)).toEqual([{ package_id: PAIR, state: 'active', label: 'PairFirst' }]); expect((await protocol.getMetaItem({ type: VIEW.type, name: VIEW.name })).item.label).toBe('PairFirst'); }); + + // [#29414] The plan and both revert limbs address the committing package's + // own row. Before, each read any package's active row, so the pair's + // commit beside a row bound to the base addressed the base row. + const planOf = async (protocol: any, packageId: string, commitId: string) => + ((await protocol.listCommits({ packageId })) as Array<{ id: string; items: unknown[] }>).find((c) => c.id === commitId)?.items; + + it('[#29414] the pair\'s first commit beside a row bound to the base records a create, and its revert removes the pair row; the base row is served again', async () => { + const { protocol, rows } = makeProtocol(); + await intoBaseBeforeClose(protocol, 'BaseRow'); + const created = await publishCommit(protocol, PAIR, declared('PairFirst')); + expect(await planOf(protocol, PAIR, created)).toEqual([ + expect.objectContaining({ type: VIEW.type, name: VIEW.name, existedBefore: false, prevVersion: null }), + ]); + + tick(); + const result = await protocol.revertCommit({ commitId: created, packageId: PAIR, actor: 'admin' }); + + expect(result).toMatchObject({ success: true, revertedCount: 1, failed: [], reverted: [{ type: VIEW.type, name: VIEW.name, action: 'removed' }] }); + expect(rows(VIEW.type, VIEW.name)).toEqual([{ package_id: BASE, state: 'active', label: 'BaseRow' }]); + expect((await protocol.getMetaItem({ type: VIEW.type, name: VIEW.name })).item.label).toBe('BaseRow'); + }); + + it('[#29414] the pair\'s edit commit beside a row bound to the base restores the pair\'s own row; the base row is untouched', async () => { + const { protocol, rows } = makeProtocol(); + await intoBaseBeforeClose(protocol, 'BaseRow'); + await publishCommit(protocol, PAIR, declared('PairFirst')); + const edit = await publishCommit(protocol, PAIR, declared('PairSecond')); + expect(await planOf(protocol, PAIR, edit)).toEqual([ + expect.objectContaining({ type: VIEW.type, name: VIEW.name, existedBefore: true }), + ]); + + tick(); + const result = await protocol.revertCommit({ commitId: edit, packageId: PAIR, actor: 'admin' }); + + expect(result).toMatchObject({ success: true, revertedCount: 1, failed: [], reverted: [{ type: VIEW.type, name: VIEW.name, action: 'restored' }] }); + expect(rows(VIEW.type, VIEW.name)).toEqual([ + { package_id: BASE, state: 'active', label: 'BaseRow' }, + { package_id: PAIR, state: 'active', label: 'PairFirst' }, + ]); + }); + + it('[#29414] a revert of the base\'s edit commit whose row was since deleted restores into the base\'s own row, so the named-base close refuses it; nothing is written', async () => { + const { protocol, rows } = makeProtocol(); + await intoBaseBeforeClose(protocol, 'First'); + const edit = await intoBaseBeforeClose(protocol, 'Second'); + expect(await protocol.deleteMetaItem({ type: VIEW.type, name: VIEW.name, packageId: BASE })).toMatchObject({ success: true }); + expect(rows(VIEW.type, VIEW.name)).toEqual([]); + + tick(); + const result = await protocol.revertCommit({ commitId: edit, packageId: BASE, actor: 'admin' }); + + expect(result).toMatchObject({ success: false, revertedCount: 0, failedCount: 1, reverted: [] }); + expect(result.failed).toEqual([expect.objectContaining({ type: VIEW.type, name: VIEW.name, code: 'NOT_OVERRIDABLE' })]); + expectClosedSentence(result.failed[0].error, VIEW.type, VIEW.name, 'rollback'); + expect(rows(VIEW.type, VIEW.name)).toEqual([]); + expect((await protocol.getMetaItem({ type: VIEW.type, name: VIEW.name })).item.label).toBe(PACKAGED); + }); + + it('[#29414] a pair commit recorded before #29414 beside a row bound to the base names the base row\'s version: its revert is refused by the override write gate, and the pair\'s declared row is unchanged', async () => { + const { protocol, rows } = makeProtocol(); + await intoBaseBeforeClose(protocol, 'BaseRow'); + const created = await publishCommit(protocol, PAIR, declared('PairFirst')); + // The plan such a commit recorded before #29414: the base row's version, read off any package's row. + const engine = protocol.engine; + const baseVersion = (await engine.findOne('sys_metadata', { where: { type: VIEW.type, name: VIEW.name, state: 'active', package_id: BASE } })).version; + expect(typeof baseVersion).toBe('number'); + await engine.update('sys_metadata_commit', { + items: JSON.stringify([{ type: VIEW.type, name: VIEW.name, existedBefore: true, prevVersion: baseVersion }]), + }, { where: { id: created } }); + expect(await planOf(protocol, PAIR, created)).toEqual([ + expect.objectContaining({ existedBefore: true, prevVersion: baseVersion }), + ]); + + tick(); + const result = await protocol.revertCommit({ commitId: created, packageId: PAIR, actor: 'admin' }); + + expect(result).toMatchObject({ success: false, revertedCount: 0, failedCount: 1, reverted: [] }); + expect(result.failed).toEqual([expect.objectContaining({ type: VIEW.type, name: VIEW.name, code: 'NOT_OVERRIDABLE' })]); + expect(rows(VIEW.type, VIEW.name)).toEqual([ + { package_id: BASE, state: 'active', label: 'BaseRow' }, + { package_id: PAIR, state: 'active', label: 'PairFirst' }, + ]); + const pairRow = await engine.findOne('sys_metadata', { where: { type: VIEW.type, name: VIEW.name, state: 'active', package_id: PAIR } }); + expect(JSON.parse(pairRow.metadata).overrides).toEqual({ package: BASE, basedOn: BASE_VERSION }); + expect((await protocol.getMetaItem({ type: VIEW.type, name: VIEW.name })).item.label).toBe('PairFirst'); + }); + + it('[#29414] CONTROL: the pair\'s edit commit whose row was since deleted restores the pair\'s declared row again', async () => { + const { protocol, rows } = makeProtocol(); + await publishCommit(protocol, PAIR, declared('PairFirst')); + const edit = await publishCommit(protocol, PAIR, declared('PairSecond')); + expect(await protocol.deleteMetaItem({ type: VIEW.type, name: VIEW.name, packageId: PAIR })).toMatchObject({ success: true }); + expect(rows(VIEW.type, VIEW.name)).toEqual([]); + + tick(); + const result = await protocol.revertCommit({ commitId: edit, packageId: PAIR, actor: 'admin' }); + + expect(result).toMatchObject({ success: true, revertedCount: 1, failed: [], reverted: [{ type: VIEW.type, name: VIEW.name, action: 'restored' }] }); + expect(rows(VIEW.type, VIEW.name)).toEqual([{ package_id: PAIR, state: 'active', label: 'PairFirst' }]); + expect((await protocol.getMetaItem({ type: VIEW.type, name: VIEW.name })).item.label).toBe('PairFirst'); + }); + + it('[#29414] CONTROL: a writable package\'s edit commit whose row was since deleted restores into that package\'s row, never a package-less one', async () => { + const { protocol, rows } = makeProtocol(); + await publishCommit(protocol, WRITABLE, VIEW.body('First')); + const edit = await publishCommit(protocol, WRITABLE, VIEW.body('Second')); + expect(await protocol.deleteMetaItem({ type: VIEW.type, name: VIEW.name, packageId: WRITABLE })).toMatchObject({ success: true }); + + tick(); + const result = await protocol.revertCommit({ commitId: edit, packageId: WRITABLE, actor: 'admin' }); + + expect(result).toMatchObject({ success: true, revertedCount: 1, failed: [], reverted: [{ type: VIEW.type, name: VIEW.name, action: 'restored' }] }); + expect(rows(VIEW.type, VIEW.name)).toEqual([{ package_id: WRITABLE, state: 'active', label: 'First' }]); + }); }); diff --git a/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts b/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts index 26163de1fed..7333f9469f2 100644 --- a/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts +++ b/packages/metadata-protocol/src/protocol.package-publish-audit-rows.test.ts @@ -175,8 +175,10 @@ type Harness = { * between the two reads `promoteDraft` makes (`get` for the parent version, * then `put`'s own read inside its transaction), which is exactly the * 409 `METADATA_CONFLICT` the repository raises. Scoped to package-scoped - * active reads so the pre-flight `commitItems` scan — which reads active - * rows WITHOUT a `package_id` — does not consume the one-shot. + * active reads, and armed on the SECOND one of the name: [#29414] the + * pre-flight `commitItems` scan reads the publishing package's active row + * too (it read any package's before), and it runs before the promotion, + * so it must not consume the one-shot. */ faults: { failMetadataWriteFor?: string; advanceActiveOnRead?: string }; /** Audit rows that LANDED and were not rolled back. */ @@ -191,6 +193,8 @@ type Harness = { function makeStubEngine(opts: { failAudit?: boolean } = {}): Harness { const faults: { failMetadataWriteFor?: string; advanceActiveOnRead?: string } = {}; + /** [#29414] The names whose pre-flight plan read has passed (see `advanceActiveOnRead`). */ + const planReadPassed = new Set(); const rows = new Map(); const historyRows: HistoryRow[] = []; const commitRows: any[] = []; @@ -244,6 +248,10 @@ function makeStubEngine(opts: { failAudit?: boolean } = {}): Harness { if (faults.advanceActiveOnRead === hit.row.name && opts2.where?.state === 'active' && 'package_id' in (opts2.where ?? {})) { + if (!planReadPassed.has(hit.row.name)) { + planReadPassed.add(hit.row.name); + return hit.row; + } delete faults.advanceActiveOnRead; const asItStood = { ...hit.row }; hit.row.checksum = 'sha256:advanced_by_a_rival'; diff --git a/packages/metadata-protocol/src/protocol.recovery-doors-emit-mutation.test.ts b/packages/metadata-protocol/src/protocol.recovery-doors-emit-mutation.test.ts index 7a295a23bf8..13b829df149 100644 --- a/packages/metadata-protocol/src/protocol.recovery-doors-emit-mutation.test.ts +++ b/packages/metadata-protocol/src/protocol.recovery-doors-emit-mutation.test.ts @@ -161,9 +161,11 @@ function matchesWhere(r: Row, where: Record): boolean { } /** One `sys_metadata_commit` row in the driver's snake_case wire shape. */ +/** [#29414] Recorded for `PKG`, the package the rows below are bound to: a commit's revert addresses its own package's rows. */ const commitRow = (items: unknown[]) => ({ id: 'c1', commit_id: 'c1', + package_id: PKG, organization_id: null, operation: 'apply', message: 'the commit under revert', diff --git a/packages/metadata-protocol/src/protocol.recovery-doors-mutation-projector.test.ts b/packages/metadata-protocol/src/protocol.recovery-doors-mutation-projector.test.ts index ced797802ef..7b70ace95c2 100644 --- a/packages/metadata-protocol/src/protocol.recovery-doors-mutation-projector.test.ts +++ b/packages/metadata-protocol/src/protocol.recovery-doors-mutation-projector.test.ts @@ -126,9 +126,11 @@ function matchesWhere(r: Row, where: Record): boolean { } /** One `sys_metadata_commit` row in the driver's snake_case wire shape. */ +/** [#29414] Recorded for `PKG`, the package the rows below are bound to: a commit's revert addresses its own package's rows. */ const commitRow = (items: unknown[]) => ({ id: 'c1', commit_id: 'c1', + package_id: PKG, organization_id: null, operation: 'apply', message: 'the commit under revert', 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 0457d14984f..71955a7772b 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 @@ -305,14 +305,17 @@ async function seedRowVerbatim( 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' }); + // from, so each must present the CURRENT head as its parent. [#29414] + // Bound to the package the commit under revert is recorded for + // (`commitRow`): a commit's revert addresses that package's own rows. + const existing = await repo.get(ref, { state: 'active', packageId: 'app.demo' }); await repo.put(ref, args.body, { parentVersion: existing?.hash ?? null, actor: null, source: 'test.at-rest-residue', intent: 'runtime-only', state: 'active', + packageId: 'app.demo', }); } diff --git a/packages/metadata-protocol/src/protocol.rollback-carries-item-failures.test.ts b/packages/metadata-protocol/src/protocol.rollback-carries-item-failures.test.ts index 9f73c0cf758..2b8c9781b56 100644 --- a/packages/metadata-protocol/src/protocol.rollback-carries-item-failures.test.ts +++ b/packages/metadata-protocol/src/protocol.rollback-carries-item-failures.test.ts @@ -173,18 +173,23 @@ function makeProtocol(opts: { unreadableCommits?: readonly string[] } = {}) { return { protocol, rows, commits }; } -/** Write a row through the repository, which stores `type` exactly as given. */ +/** + * Write a row through the repository, which stores `type` exactly as given, + * bound to the package the commits are recorded for: [#29414] a commit's + * revert addresses the committing package's own rows, as its publish wrote them. + */ async function seed(protocol: any, type: string, name: string, label: string): Promise { await protocol.ensureOverlayIndex(); const repo = protocol.getOverlayRepo(null); const ref = { type, name, org: 'env' }; - const existing = await repo.get(ref, { state: 'active' }); + const existing = await repo.get(ref, { state: 'active', packageId: PKG }); await repo.put(ref, { name, label }, { parentVersion: existing?.hash ?? null, actor: null, source: 'test.rollback-item-failures', intent: 'runtime-only', state: 'active', + packageId: PKG, }); } diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 9c7faba87bb..bea8671a34d 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -1519,6 +1519,45 @@ export class ObjectStackProtocolImplementation implements return restoredBodyWriter.call(host, type, name, binding, source); } + /** + * [ADR-0146 D2] The write intent and the body writer of a restore into + * the row `binding` names — the per-item rollback's ({@link rollbackMetaItem}) + * and [#29414] a commit revert's restore limb ({@link revertCommit}), one + * derivation for both. + * + * A restore into the base's paired `.custom` row is the pair's declared + * override, as its save is: it edits no managed item, so the store is told + * a runtime-only write, and the restored body is judged by the override + * write gate into the pair ({@link overrideWriteVerdict}). A version that + * declares no override (a body the base row stored, which a commit plan + * recorded before #29414 can name) is refused as the save is + * (`overrideWriteRefusalError`, `403 NOT_OVERRIDABLE`), thrown from inside + * `deriveRestoredBody`, so nothing is written. Any other binding: the + * caller's `artifactBacked` decides the intent, and the body writer is + * {@link restoredBodyWriter}'s alone. + */ + private restoreWriteFor( + singularType: string, + name: string, + binding: string | null, + artifactBacked: boolean, + source: string, + ): { intent: 'override-artifact' | 'runtime-only'; deriveRestoredBody: (historyBody: unknown) => Promise } { + const owner = artifactBacked ? this.packagedArtifactOwner({ type: singularType, name }) : undefined; + const pairRow = owner !== undefined && isCustomizationPairOf(binding, owner); + const intent: 'override-artifact' | 'runtime-only' = artifactBacked && !pairRow ? 'override-artifact' : 'runtime-only'; + const writeRestored = this.restoredBodyWriter(singularType, name, binding, source); + const deriveRestoredBody = pairRow + ? async (historyBody: unknown): Promise => { + const body = await writeRestored(historyBody); + const verdict = this.overrideWriteVerdict({ type: singularType, name, packageId: binding, item: body }); + if (verdict.verdict === 'refuse') throw overrideWriteRefusalError(verdict); + return body; + } + : writeRestored; + return { intent, deriveRestoredBody }; + } + /** * [#25278] The save door's refusal family, asked of a history body a * restore is about to make the active row. @@ -17307,9 +17346,17 @@ export class ObjectStackProtocolImplementation implements // [#22877] …and the draft's own bundle member: another // member's active row is not this member's pre-publish state, // and the revert plan records the member it describes. + // [#29414] …and the publishing package's own row: every draft + // listed here is bound to `request.packageId` and is promoted + // into that package's row, so another package's active row of + // the same name (a row bound to the read-only base beside its + // `.custom` pair) is not this item's pre-publish state. Read + // package-agnostically, the pair's first publish was recorded + // as an edit of the base row's version, and its revert + // addressed the base row instead of removing the pair's. const activeRow = (await this.findEnvironmentRow('sys_metadata', { where: { - type: d.type, name: d.name, state: 'active', + type: d.type, name: d.name, state: 'active', package_id: request.packageId, ...overlayMemberWhere(d.type, d.member), }, context: { isSystem: true }, @@ -19706,6 +19753,18 @@ export class ObjectStackProtocolImplementation implements * rollback sentence naming the base's `.custom` pair) with nothing written, * and {@link rollbackToPackageCommit} reports it per item and continues. * The soft-remove limb stays open: removing such a row writes nothing. + * + * [#29414, ADR-0048] Both limbs address the committing package's own row + * (`package_id` = the commit's), as the plan {@link publishPackageDrafts} + * records describes it: a pair commit beside a row bound to its read-only + * base removes or restores the pair's row and leaves the base's served. + * The restore writes the committing package's row, in place or created + * again when it was deleted since, so this door never writes the + * package-less row; a commit of the read-only base itself is refused by + * the #29390 close. A restore into the base's paired `.custom` row is judged + * by the override write gate as {@link rollbackMetaItem} judges it + * ({@link restoreWriteFor}): a version declaring no override is refused + * into `failed[]`. */ async revertCommit(request: { commitId: string; @@ -19730,6 +19789,9 @@ export class ObjectStackProtocolImplementation implements ); await this.ensureOverlayIndex(); const row = await this.readRevertableCommit(request.commitId, request.packageId); + // [#29414] The package whose rows this commit's plan describes: every + // item's rows are read, removed and restored under it below. + const commitPackageId: string = row.package_id; const items = this.parseCommitItems(row.items); // #4556 — threaded into repo.put/delete → `recorded_by`; NULL when the // revert carries no human actor. @@ -19876,7 +19938,14 @@ export class ObjectStackProtocolImplementation implements // Absent (the canonical member) on every other item, as before. const member = it.member; try { - const current = await repo.get(ref, { state: 'active', ...memberOption(member) }); + // [#29414] The committing package's own row, and no other + // package's: the plan describes the rows this package's publish + // promoted into, so every read, removal and restore below names + // `commitPackageId`. Read package-agnostically, a pair commit + // beside a row bound to its read-only base addressed whichever + // row the driver returned first — the base row, refused below + // or restored in the pair's name. + const current = await repo.get(ref, { state: 'active', packageId: commitPackageId, ...memberOption(member) }); if (!it.existedBefore) { // Created by this commit → soft-remove (metadata only; table stays). // @@ -19919,6 +19988,7 @@ export class ObjectStackProtocolImplementation implements source: 'protocol.revertCommit', intent, state: 'active', + packageId: commitPackageId, ...memberOption(member), }); } @@ -20038,8 +20108,18 @@ export class ObjectStackProtocolImplementation implements // The soft-remove limb above stated the same intent as a // CONSTANT and was fixed the same way (#6620), so both limbs now // derive it. The registry half of both is #6621, below. - const intent: 'override-artifact' | 'runtime-only' = - this.isArtifactBacked(it.type, it.name) ? 'override-artifact' : 'runtime-only'; + // + // [#29414, ADR-0146 D2] …and a restore into the base's paired + // `.custom` row is the pair's declared override, judged by the + // override write gate exactly as {@link rollbackMetaItem} + // judges it ({@link restoreWriteFor}, one derivation): a + // version that declares no override — the base row's body a + // plan recorded before #29414 names as the pair's pre-commit + // version — is refused into `failed[]`, nothing written. + const { intent, deriveRestoredBody } = this.restoreWriteFor( + PLURAL_TO_SINGULAR[it.type] ?? it.type, it.name, commitPackageId, + this.isArtifactBacked(it.type, it.name), 'protocol.revertCommit', + ); // [#6621 / #4636] The ownership key the write-through needs, // read from the ROW rather than from the request — the sibling // revert caller {@link rollbackMetaItem} reads it exactly this @@ -20058,7 +20138,17 @@ export class ObjectStackProtocolImplementation implements // fallible query downstream of a write that already succeeded — // 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, member); + // + // [#29414, ADR-0048, ADR-0067 D3] The binding is the + // committing package's, never another package's row's: the + // restore writes the committing package's own row, in place + // when it is stored and created again when it was deleted + // since (`restoreVersion` is handed `commitPackageId` and + // writes it either way). So this door never writes the + // package-less row, and the package-less close has nothing + // here to judge (every commit names its package); the write + // naming the read-only base itself is closed below. + const restorePackageId: string = commitPackageId; // [#29390, ADR-0146 D1, D11] A restore writes the row this // binding names, so a restore into a row bound to the // read-only base itself (a commit recorded before #28905 @@ -20076,6 +20166,8 @@ export class ObjectStackProtocolImplementation implements const restored = await repo.restoreVersion(ref, restoreToVersion, { actor, source: 'protocol.revertCommit', + // [#29414] The row restored is the committing package's. + packageId: commitPackageId, message: `revert commit ${request.commitId}`, intent, // [#21470] The divergent `name` refusal, [ADR-0086 D3] @@ -20086,9 +20178,8 @@ export class ObjectStackProtocolImplementation implements // in `failed[]` below, with nothing written. Judged under // the SINGULAR type, the spelling the write-through // registers under. See {@link restoredBodyWriter}. - deriveRestoredBody: this.restoredBodyWriter( - PLURAL_TO_SINGULAR[it.type] ?? it.type, it.name, restorePackageId, 'protocol.revertCommit', - ), + // [ADR-0146 D2] …and, for the pair's row, the override write gate. + deriveRestoredBody, ...memberOption(member), }); // [#6621] #4521 — a revert is a live write like any other: the @@ -20478,21 +20569,10 @@ export class ObjectStackProtocolImplementation implements // the override write gate as that save is ({@link overrideWriteVerdict} // into the pair): a version that declares no override is refused as // the save is, with nothing written. - const owner = artifactBacked ? this.packagedArtifactOwner({ type: singularType, name: request.name }) : undefined; - const pairRowRollback = owner !== undefined && isCustomizationPairOf(rollbackPackageId, owner); - const intent: 'override-artifact' | 'runtime-only' = artifactBacked && !pairRowRollback - ? 'override-artifact' : 'runtime-only'; - const writeRestored = this.restoredBodyWriter(singularType, request.name, rollbackPackageId, 'protocol.rollbackMetaItem'); - const deriveRestoredBody = pairRowRollback - ? async (historyBody: unknown): Promise => { - const body = await writeRestored(historyBody); - const verdict = this.overrideWriteVerdict({ - type: singularType, name: request.name, packageId: rollbackPackageId, item: body, - }); - if (verdict.verdict === 'refuse') throw overrideWriteRefusalError(verdict); - return body; - } - : writeRestored; + // [#29414] The derivation a commit revert's restore limb shares ({@link restoreWriteFor}). + const { intent, deriveRestoredBody } = this.restoreWriteFor( + singularType, request.name, rollbackPackageId, artifactBacked, 'protocol.rollbackMetaItem', + ); try { const result = await repo.restoreVersion(ref, request.toVersion, { // #4556 — NULL, not 'system', for an actor-less rollback. diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index 83fb10a21d8..dc9ba82f943 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -1683,6 +1683,16 @@ export class SysMetadataRepository implements MetadataRepository { * of ANOTHER member of the bundle is not this member's version. */ member?: string | null; + /** + * [#29414] The package whose active row is restored. Stated → the + * active row (its parent hash and its binding) is resolved under the + * ADR-0048 key `(org, type, name, package_id)`, so another package's + * row of the same name is never the one restored, and the restore is + * written under it, creating that package's row again when it is gone. + * Omitted → the "match any package" resolution and, with no active row, + * the unbound row, as before. A LOCAL extension, as on {@link delete}. + */ + packageId?: string | null; }, ): Promise<{ version: string; seq: number; item: MetadataItem }> { this.assertOpen(); @@ -1739,8 +1749,9 @@ export class SysMetadataRepository implements MetadataRepository { // A missing active row (deleted, or never published) yields `null` — the // unbound row, which is the only defined answer: `sys_metadata_history` // carries no `package_id` column, so a vanished binding is not recoverable. + // [#29414] …under the package the caller names, when it names one. const activeRow = await this.findEnvironmentRow({ - where: this.whereFor(ref, 'active', undefined, member), + where: this.whereFor(ref, 'active', 'packageId' in opts ? (opts.packageId ?? null) : undefined, member), context: { isSystem: true }, }); const activePackageId = @@ -1754,7 +1765,8 @@ export class SysMetadataRepository implements MetadataRepository { intent: opts.intent ?? 'override-artifact', state: 'active', opType: 'revert', - packageId: activePackageId, + // [#29414] The stated package's row, found or not. + packageId: 'packageId' in opts ? (opts.packageId ?? null) : activePackageId, }); } @@ -2649,9 +2661,9 @@ export class SysMetadataRepository implements MetadataRepository { // package A promoted package B's pending draft, drained B's row, recorded // it under A's commit, and left A's own edit pending while answering // `success: true`. A caller that KNOWS the binding must state it — see - // {@link promoteDraft}'s `packageId`. The same question is open for the - // `delete` and `restoreVersion` resolving reads (#6215); they are - // deliberately unchanged here. + // {@link promoteDraft}'s `packageId`. The `delete` and `restoreVersion` + // resolving reads take the same option (#6215); [#29414] the commit revert + // states it on both. if (packageId !== undefined) where.package_id = packageId; // string → eq; null → IS NULL if (member !== undefined) where[OVERLAY_MEMBER_COLUMN] = member; // string → eq; null → IS NULL return where; diff --git a/packages/objectql/src/protocol-publish-package-drafts.test.ts b/packages/objectql/src/protocol-publish-package-drafts.test.ts index fe2a8b7608f..404293e2cf6 100644 --- a/packages/objectql/src/protocol-publish-package-drafts.test.ts +++ b/packages/objectql/src/protocol-publish-package-drafts.test.ts @@ -157,9 +157,9 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { // 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([ - { type: 'object', name: 'course', state: 'active' }, - { type: 'object', name: 'student', state: 'active' }, - { type: 'view', name: 'course_list', state: 'active' }, + { type: 'object', name: 'course', state: 'active', package_id: 'app.edu' }, + { type: 'object', name: 'student', state: 'active', package_id: 'app.edu' }, + { type: 'view', name: 'course_list', state: 'active', package_id: 'app.edu' }, ]); expect(promote).toHaveBeenCalledTimes(3); expect((promote.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'course' }); @@ -205,10 +205,12 @@ describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { 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. + // the values below evidence rather than defaults. [#29414] The scope names + // the publishing package: another package's row of the name is not the + // item's pre-publish state. expect(captureReads.slice(0, 2)).toEqual([ - { type: 'object', name: 'course', state: 'active' }, - { type: 'object', name: 'student', state: 'active' }, + { type: 'object', name: 'course', state: 'active', package_id: 'app.edu' }, + { type: 'object', name: 'student', state: 'active', package_id: 'app.edu' }, ]); // One commit, carrying one entry per promoted item, each with the state // that was actually READ. diff --git a/packages/qa/dogfood/test/package-commit-revert-path-package.dogfood.test.ts b/packages/qa/dogfood/test/package-commit-revert-path-package.dogfood.test.ts index 96ea5f93c8f..ac2e220c592 100644 --- a/packages/qa/dogfood/test/package-commit-revert-path-package.dogfood.test.ts +++ b/packages/qa/dogfood/test/package-commit-revert-path-package.dogfood.test.ts @@ -51,6 +51,14 @@ // restoring it would write the row bound to the read-only base again. The // last case pins both on such a commit, recorded with the close shadowed on // the booted instance only (what a build before the close did). +// +// [#29414] A commit names its own package's rows. The pair's first publish +// beside such a base-bound row is recorded as a create (its plan used to read +// the base row's version), so its revert removes the pair's row and the base +// row is served again. A revert whose row was deleted since restores into the +// committing package's own row, never a package-less one, so the base's own +// commit is refused by the close naming the pair. Control: a writable +// package's edit commit restores its own row in place. import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { type VerifyStack } from '@objectstack/verify'; @@ -286,4 +294,89 @@ describe('dogfood: package commit revert and rollback act only on a commit of th }); expect(await served()).toBe(shipped); }); + + // [#29414] The commit a package records names its OWN rows: the plan reads the + // pre-publish state of the publishing package's row, and the revert removes or + // restores that row and no other package's. Before, both read any package's + // active row, so the pair's first publish beside a base-bound row was recorded + // as an edit of the base row's version, and its revert addressed the base row. + const view = (label: string) => ({ + name: OVERLAY, object: 'showcase_task', viewKind: 'list', label, + config: { type: 'grid', columns: [{ field: 'title' }] }, + }); + const servedLabel = async () => (await call('GET', `/meta/view/${OVERLAY}`)).json?.item?.label; + const publishBaseBound = async (label: string): Promise => { + const protocol = (stack.kernel as any).getService('protocol'); + protocol.namedBaseOverlayWriteRefusal = () => null; + try { + const saved = await call('PUT', `/meta/view/${OVERLAY}?mode=draft&package=${SHOWCASE}`, view(label)); + expect(saved.status, JSON.stringify(saved.json)).toBe(200); + const published = await call('POST', `/packages/${SHOWCASE}/publish-drafts`, {}); + expect(published.status, JSON.stringify(published.json)).toBe(200); + return published.json?.data?.commitId as string; + } finally { + delete protocol.namedBaseOverlayWriteRefusal; + } + }; + + it('[#29414] a pair commit beside a base-bound row records the pair\'s own row, and its revert removes the pair row and serves the base row again', async () => { + const shipped = await servedLabel(); + await publishBaseBound('Base-bound row'); + expect(await servedLabel()).toBe('Base-bound row'); + + const pair = await ensurePair(stack, admin, SHOWCASE); + const saved = await call('PUT', inPair(`/meta/view/${OVERLAY}?mode=draft`, SHOWCASE), declaredOverride(view('Pair override'), SHOWCASE, '0.1.0')); + expect(saved.status, JSON.stringify(saved.json)).toBe(200); + const published = await call('POST', `/packages/${pair}/publish-drafts`, {}); + expect(published.status, JSON.stringify(published.json)).toBe(200); + const commitId = published.json?.data?.commitId as string; + expect(await servedLabel()).toBe('Pair override'); + + const commit = (await commitsOf(pair)).find((c) => c.id === commitId) as any; + expect(commit?.items, JSON.stringify(commit)).toEqual([ + expect.objectContaining({ type: 'view', name: OVERLAY, existedBefore: false, prevVersion: null }), + ]); + + const res = await call('POST', `/packages/${pair}/commits/${commitId}/revert`, {}); + expect(res.status, JSON.stringify(res.json)).toBe(200); + expect(res.json?.data).toMatchObject({ + success: true, revertedCount: 1, failedCount: 0, reverted: [{ type: 'view', name: OVERLAY, action: 'removed' }], + }); + expect(await servedLabel()).toBe('Base-bound row'); + + // The base-bound row is cleared the one way the close leaves open: a delete naming the base. + const cleared = await call('DELETE', `/meta/view/${OVERLAY}?package=${SHOWCASE}`); + expect(cleared.status, JSON.stringify(cleared.json)).toBe(200); + expect(await servedLabel()).toBe(shipped); + }); + + it('[#29414] a commit that edited a base-bound row since deleted restores into the base\'s own row, so it is refused by the close naming the pair, and writes nothing', async () => { + const shipped = await servedLabel(); + await publishBaseBound('Deleted later 1'); + const edited = await publishBaseBound('Deleted later 2'); + const cleared = await call('DELETE', `/meta/view/${OVERLAY}?package=${SHOWCASE}`); + expect(cleared.status, JSON.stringify(cleared.json)).toBe(200); + expect(await servedLabel()).toBe(shipped); + + const res = await call('POST', `/packages/${SHOWCASE}/commits/${edited}/revert`, {}); + expect(res.status, JSON.stringify(res.json)).toBe(200); + expect(res.json?.data).toMatchObject({ success: false, revertedCount: 0, failedCount: 1 }); + expect(res.json?.data?.failed?.[0]).toMatchObject({ type: 'view', name: OVERLAY, code: 'NOT_OVERRIDABLE' }); + expect(res.json.data.failed[0].error).toContain('a rollback of its row bound to the read-only base is closed'); + expect(res.json.data.failed[0].error).toContain(`?package=${SHOWCASE}.custom`); + expect(await servedLabel()).toBe(shipped); + }); + + it('[#29414] control: a writable package\'s edit commit restores its own row in place', async () => { + await publishObject(A, 'cmtpa_edited', 'A edited 1'); + await publishObject(A, 'cmtpa_edited', 'A edited 2'); + const edit = (await commitsOf(A))[0] as any; + expect(edit.items).toEqual([expect.objectContaining({ type: 'object', name: 'cmtpa_edited', existedBefore: true })]); + const res = await call('POST', `/packages/${A}/commits/${edit.id}/revert`, {}); + expect(res.status, JSON.stringify(res.json)).toBe(200); + expect(res.json?.data).toMatchObject({ + success: true, revertedCount: 1, reverted: [{ type: 'object', name: 'cmtpa_edited', action: 'restored' }], + }); + expect((await call('GET', '/meta/object/cmtpa_edited')).json?.item?.label).toBe('A edited 1'); + }); }); diff --git a/packages/spec/src/migrations/entries/semantic/18.packageless-overlay-write-closed.ts b/packages/spec/src/migrations/entries/semantic/18.packageless-overlay-write-closed.ts index f33a735b4c6..749a05d9631 100644 --- a/packages/spec/src/migrations/entries/semantic/18.packageless-overlay-write-closed.ts +++ b/packages/spec/src/migrations/entries/semantic/18.packageless-overlay-write-closed.ts @@ -9,7 +9,9 @@ import type { SemanticMigration } from '../../types.js'; // its row as a declared override (a row it moves as the pair's own member // stays open), and a save, publish or rollback naming the read-only package // itself onto an item it ships is refused where the pair can take the -// declared override (a delete naming the package stays open). +// declared override (a delete naming the package stays open; a commit revert +// or package rollback restoring a commit item into such a row is that +// rollback, refused per item). // Registered because an upgrading deployment can carry the rows those writes // made, package-less or bound to the read-only package itself, and the // operator step that moves both kinds into the base's `.custom` pair ships in @@ -28,7 +30,10 @@ export const entry: SemanticMigration = { + 'member, an email template locale the base does not ship, keeps the write); (2) naming the ' + 'read-only package itself (a save in draft or publish mode or a per-item publish under ' + '/api/v1/meta//?package=, POST /api/v1/packages//publish-drafts ' - + 'holding such a draft, and a rollback of a row bound to the package), where the pair can be ' + + 'holding such a draft, and a rollback of a row bound to the package: the per-item rollback, ' + + 'and the restore of a commit item into that row by POST /api/v1/packages//commits/' + + '/revert and POST /api/v1/packages//rollback, reported per item in their ' + + 'failed[]), where the pair can be ' + 'ensured, the base records a version and the override write gate admits the declared body ' + 'into the pair (a delete naming the package keeps the write); and the sys_metadata rows ' + 'those writes stored, package-less or bound to the read-only package itself', @@ -80,7 +85,9 @@ export const entry: SemanticMigration = { + 'member, keeps its package-less write. The close reaches only what the command moves as a ' + 'declared override (its own classification and its own move steps), so no customization is ' + 'left without a write door. The save, publish or rollback naming the read-only package ' - + 'itself is refused 403 NOT_OVERRIDABLE naming the pair and the `overrides` declaration ' + + 'itself (a commit revert or package rollback restoring a commit item into the row bound to ' + + 'it among them, which reports the item in its failed[] and reverts the rest) is refused 403 ' + + 'NOT_OVERRIDABLE naming the pair and the `overrides` declaration ' + 'wherever the pair can take the declared override (it can be ensured, the base records a ' + 'version and the override write gate admits the declared body); a delete naming the package ' + 'stays open, as the way to remove such a row, and so does every write the pair could not '