diff --git a/.changeset/revert-commit-soft-remove-intent.md b/.changeset/revert-commit-soft-remove-intent.md new file mode 100644 index 0000000000..618a4a6061 --- /dev/null +++ b/.changeset/revert-commit-soft-remove-intent.md @@ -0,0 +1,48 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): `revertCommit`'s soft-remove limb states its write intent per item, so a commit that CREATED an object can be reverted (#6620) + +`ObjectStackProtocolImplementation.revertCommit` has two limbs. #6563 (PR #6642) +fixed the one that RESTORES an edited artifact, where the intent was unstated and +fell through to `restoreVersion`'s `?? 'override-artifact'` default. The other +limb — an artifact the commit CREATED, which the revert soft-removes — stated the +same intent as a literal constant: + +``` +intent: 'override-artifact', +``` + +`SysMetadataRepository.delete` opens with `this.assertAllowed(ref.type, opts.intent)`, +the same gate `put` uses, and it refuses every type whose registry entry is not +`allowOrgOverride`. `object` is exactly such a type, so every created object of a +reverted commit came back in `failed[]`: + +``` +[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry. +Overlay-allowed: view, page, dashboard, app, action, report, dataset, ... +``` + +This is the FIRST-BUILD undo — the Studio / AI flow that publishes a brand-new app +and then undoes it. Every object the commit created stayed behind, the call +answered `success: false` with a populated `failed[]`, and the package was left +half-reverted: its overlay-allowed items removed, its objects not. +`rollbackToPackageCommit` reverts through the same loop and inherited it, and +there the symptom was quieter still — a per-item refusal never throws, so the +rollback recorded the commit as reverted and answered `success: true` while the +created object was untouched. + +The limb now derives the intent from the artifact the way the sibling DELETE +caller `deleteMetaItem` already does — `isArtifactBacked` gives +`'override-artifact'`, otherwise `'runtime-only'` — and does it **per item**, +because one first-build commit routinely creates a runtime object beside a +packaged-artifact name. All three delete/revert callers (`deleteMetaItem`, +`rollbackMetaItem`, both `revertCommit` limbs) now derive the same fact the same +way. + +The repository's gate is deliberately unchanged: it is right for callers that +genuinely mean "override a packaged artifact", and the defect was this caller +never saying which of the two cases each item is. An object a code package really +ships still resolves to `'override-artifact'` and is still refused with +`NOT_OVERRIDABLE`, which is pinned alongside the fix. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 64e4bff9e1..a6f8436001 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -10370,12 +10370,46 @@ export class ObjectStackProtocolImplementation implements const current = await repo.get(ref, { state: 'active' }); if (!it.existedBefore) { // Created by this commit → soft-remove (metadata only; table stays). + // + // [#6620] The write INTENT is derived per item, exactly as + // the sibling DELETE caller {@link deleteMetaItem} derives + // it (and as the sibling revert caller + // {@link rollbackMetaItem} derives its own) — all three now + // agree. Stated as the CONSTANT `'override-artifact'` this + // limb used to carry, `SysMetadataRepository.delete` opened + // with `assertAllowed(ref.type, opts.intent)` — the same + // gate `put` uses — which refuses every type that is not + // `allowOrgOverride`, `object` among them. So a commit that + // CREATED an object could not be reverted at all: the + // first-build undo (publish a brand-new app, then undo it) + // left every created object behind, answered `success: + // false` with a populated `failed[]`, and left the package + // half-reverted — its overlay-allowed items removed, its + // objects not. + // + // Per ITEM, not per call: one first-build commit routinely + // creates a runtime object beside a packaged-artifact name, + // so a hoisted intent has to pick one and be wrong about + // the other. A genuinely artifact-backed item still + // resolves to `'override-artifact'` and is still refused + // with `NOT_OVERRIDABLE` — the derivation states the + // caller's case, it does not widen the repository's gate, + // which is unchanged and right. + // + // Sibling limb: #6563 (PR #6642) did the same for the + // restore branch below, where the intent was UNSTATED and + // fell through to `restoreVersion`'s `?? 'override-artifact'` + // default. Still not addressed here, filed with its own + // measurement: neither limb refreshes the SchemaRegistry the + // way `rollbackMetaItem` does (#6621). + const intent: 'override-artifact' | 'runtime-only' = + this.isArtifactBacked(it.type, it.name) ? 'override-artifact' : 'runtime-only'; if (current) { await repo.delete(ref, { parentVersion: current.hash, actor, source: 'protocol.revertCommit', - intent: 'override-artifact', + intent, state: 'active', }); } @@ -10401,12 +10435,12 @@ export class ObjectStackProtocolImplementation implements // resolves to `'override-artifact'` and is still refused — the // derivation states the case, it does not widen the gate. // - // Two neighbours are deliberately NOT changed here, each filed - // with its own measurement: the soft-remove limb above states the - // same intent as a CONSTANT, so a commit that CREATED an object - // still cannot be reverted (#6620); and neither limb refreshes the - // SchemaRegistry the way `rollbackMetaItem` does, so a restored - // body is persisted but not yet dispatched on (#6621). + // 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. One neighbour is still open, filed with its own + // measurement: neither limb refreshes the SchemaRegistry the way + // `rollbackMetaItem` does, so a restored body is persisted but not + // yet dispatched on (#6621). const intent: 'override-artifact' | 'runtime-only' = this.isArtifactBacked(it.type, it.name) ? 'override-artifact' : 'runtime-only'; await repo.restoreVersion(ref, it.prevVersion, { diff --git a/packages/objectql/src/protocol-commit-history.test.ts b/packages/objectql/src/protocol-commit-history.test.ts index 83b811971b..3b84faa20c 100644 --- a/packages/objectql/src/protocol-commit-history.test.ts +++ b/packages/objectql/src/protocol-commit-history.test.ts @@ -632,3 +632,222 @@ describe('#6563 — rollbackToPackageCommit inherits the per-item intent', () => expect(fields).not.toContain('due_date'); }); }); + +/** + * #6620 — the OTHER limb of the same loop: SOFT-REMOVE states its intent too. + * + * `revertCommit` has two limbs, and #6563 (above) only fixed the restore one. + * The limb that undoes an artifact the commit CREATED stated its intent as a + * CONSTANT — `intent: 'override-artifact'`, written into the `repo.delete(...)` + * call — and `SysMetadataRepository.delete` opens with the same + * `assertAllowed(ref.type, opts.intent)` gate `put` uses. So `object`, which is + * not `allowOrgOverride`, was refused on the delete path exactly as it had been + * on the restore path, and a commit that CREATED an object could not be + * reverted either. + * + * That is the FIRST-BUILD undo — publish a brand-new app, then undo it — which + * is the flow Studio and AI authoring produce most. Every object the commit + * created stayed behind, `success` came back `false` with a populated + * `failed[]`, and the package was left half-reverted: its overlay-allowed items + * removed, its objects not. + * + * The two causes are different even though the symptom rhymes: #6563 was an + * UNSTATED intent falling through to the repository's `?? 'override-artifact'` + * default, this one is a literal the caller wrote down. The fix is the same + * family shape — derive it per item from `isArtifactBacked`, the way the + * sibling delete caller `deleteMetaItem` and the sibling revert caller + * `rollbackMetaItem` both already do — so all three delete/revert callers now + * agree, and the repository's gate is untouched. + */ + +/** The commit item shape for an artifact this commit CREATED (ADR-0067). */ +const createdItem = (name: string) => ({ + type: 'object', name, existedBefore: false, prevVersion: null, +}); + +/** The first-build shape: authored ONCE, never edited — nothing to restore to. */ +async function seedCreatedObject(protocol: any, name: string, packageId?: string) { + await protocol.saveMetaItem({ + type: 'object', name, ...(packageId ? { packageId } : {}), item: invoiceBody(name), + }); +} + +const storedRows = (rows: Map, name: string) => + Array.from(rows.values()).filter((r) => r.name === name); + +describe('#6620 — revertCommit soft-removes a runtime-CREATED `object`', () => { + it('a package-bound created object reverts: revertedCount 1, failed [], row gone', async () => { + const { protocol, rows, historyRows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_new', + items: [createdItem('myapp_invoice')], + })]); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(1); + + const res = await protocol.revertCommit({ commitId: 'cmt_new' }); + + // Pre-fix, verbatim (the issue's measurement): success false, revertedCount + // 0, failedCount 1 carrying "[NOT_OVERRIDABLE] 'object' is not + // allowOrgOverride in the registry.", and the row still standing. + expect(res.failed).toEqual([]); + expect(res.success).toBe(true); + expect(res.revertedCount).toBe(1); + expect(res.reverted[0]).toMatchObject({ type: 'object', name: 'myapp_invoice', action: 'removed' }); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + // Soft, not hard: ADR-0067 §5 keeps the removal recoverable, so the delete + // is an append-only tombstone in history rather than a vanished lineage. + const tombstone = historyRows.filter( + (h) => h.name === 'myapp_invoice' && h.operation_type === 'delete', + ); + expect(tombstone).toHaveLength(1); + expect(tombstone[0].metadata).toBeNull(); + }); + + it('a package-LESS created object reverts identically — the binding was never the cause', async () => { + const { protocol, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_new_global', + package_id: null, + items: [createdItem('global_invoice')], + })]); + await seedCreatedObject(protocol, 'global_invoice'); + + const res = await protocol.revertCommit({ commitId: 'cmt_new_global' }); + + expect(res.failed).toEqual([]); + expect(res.revertedCount).toBe(1); + expect(storedRows(rows, 'global_invoice')).toHaveLength(0); + }); + + /** + * The refusal that must SURVIVE the fix — and the one case the constant got + * right by accident, which is why its direction is INVERTED: it was green + * before the change and is green after. It cannot go red by removing the fix, + * because removing the fix refuses EVERYTHING. What it does go red on is the + * wrong fix — hard-coding `'runtime-only'` in place of the old + * `'override-artifact'` — which is the mistake a one-line "just make objects + * work" edit would make, and which would let a revert tombstone an artifact a + * code package genuinely ships. + * + * Staged the way a real deployment stages it (as in #6563's block): the + * overlay row is authored while the name is runtime-only, and the artifact + * arrives with the package that later claims it. `registerObject(body, pkg)` + * with no `_provenance` is the shape `applyProtection` stamps as `'package'`, + * which is what `getArtifactItem` reads and `isArtifactBacked` answers on. + * + * Envelope note (ADR-0112): `revertCommit` converts a per-item throw into a + * `failed[]` record whose DECLARED shape is `{ type, name, error, code? }` — + * no `status`. So `code` is asserted here together with the condition's own + * first sentence, and the full `{ code, status }` pair belongs to the + * throwing surface (`protocol-writepath-object-ownership.test.ts`), exactly + * as #6563 split it. + */ + it('still REFUSES soft-removing an artifact-backed object: NOT_OVERRIDABLE, row kept', async () => { + const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_new_artifact', + items: [createdItem('myapp_invoice')], + })]); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + registry.registerObject(invoiceBody('myapp_invoice') as never, APP_PKG); + + const res = await protocol.revertCommit({ commitId: 'cmt_new_artifact' }); + + expect(res.revertedCount).toBe(0); + expect(res.failedCount).toBe(1); + expect(res.failed[0]).toMatchObject({ + type: 'object', + name: 'myapp_invoice', + code: 'NOT_OVERRIDABLE', + }); + expect(res.failed[0].error).toContain( + `[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry.`, + ); + // Refused means refused: the artifact-backed row is still there. + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(1); + }); + + /** + * PER ITEM, not per call — the half a single-item fixture cannot see, on the + * soft-remove limb this time. One commit, two created objects, opposite + * verdicts: a loop that hoisted one intent for the batch (which is precisely + * what the constant did) has to pick one and be wrong about the other. + */ + it('derives the intent PER ITEM: one created object removed, its artifact-backed neighbour refused', async () => { + const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_new_mixed', + items: [createdItem('myapp_invoice'), createdItem('myapp_quote')], + })]); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + await seedCreatedObject(protocol, 'myapp_quote', APP_PKG); + // Only the quote is claimed by a code artifact. + registry.registerObject(invoiceBody('myapp_quote') as never, APP_PKG); + + const res = await protocol.revertCommit({ commitId: 'cmt_new_mixed' }); + + expect(res.reverted).toEqual([ + { type: 'object', name: 'myapp_invoice', action: 'removed' }, + ]); + expect(res.failed).toHaveLength(1); + expect(res.failed[0]).toMatchObject({ name: 'myapp_quote', code: 'NOT_OVERRIDABLE' }); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + expect(storedRows(rows, 'myapp_quote')).toHaveLength(1); + }); + + /** + * A commit that created BOTH an overlay-allowed item and an object is the + * half-reverted package the issue describes: pre-fix the view came out and + * the object stayed, so `success` was `false` and the package sat in a state + * neither before nor after the commit. + */ + it('reverts a mixed-TYPE first build whole: the view and the object both come out', async () => { + const { protocol, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_new_build', + items: [ + createdItem('myapp_invoice'), + { type: 'view', name: 'myapp_case_grid', existedBefore: false, prevVersion: null }, + ], + })]); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + await protocol.saveMetaItem({ + type: 'view', name: 'myapp_case_grid', packageId: APP_PKG, item: gridBody('Cases'), + }); + + const res = await protocol.revertCommit({ commitId: 'cmt_new_build' }); + + expect(res.failed).toEqual([]); + expect(res.success).toBe(true); + expect(res.revertedCount).toBe(2); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + expect(storedRows(rows, 'myapp_case_grid')).toHaveLength(0); + }); +}); + +/** + * #6620 — the inheritance, on the soft-remove limb. `rollbackToPackageCommit` + * reverts through the SAME loop, so it carried the same constant. + * + * As in #6563's inheritance pin, the status cannot show the defect: + * `revertCommit` turns a per-item refusal into `failed[]` instead of throwing, + * so the rollback recorded the commit as reverted and answered `success: true` + * while the created object was never removed. The line that goes red pre-fix is + * the STORED ROW. + */ +describe('#6620 — rollbackToPackageCommit inherits the per-item soft-remove intent', () => { + it('rolls a first build back through the loop — and the created row really went away', async () => { + const { protocol, rows } = makeRealRepoHarness([ + objectCommit({ id: 'cmt_base', items: [], created_at: '2026-08-08T00:00:01.000Z' }), + objectCommit({ + id: 'cmt_build', + items: [createdItem('myapp_invoice')], + created_at: '2026-08-08T00:00:02.000Z', + }), + ]); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + + const res = await protocol.rollbackToPackageCommit({ commitId: 'cmt_base' }); + + expect(res.revertedCommits).toEqual(['cmt_build']); + expect(res.failed).toEqual([]); + // `success: true` was ALREADY true pre-fix — this is the line that was not. + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + }); +});