From e99b9753abcd35ddd47bf65516f09b92c2d6e7a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 14:21:49 +0000 Subject: [PATCH] fix(metadata-protocol): revertCommit's soft-remove limb states its write intent per item (#6620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The limb that undoes an artifact a commit CREATED stated its intent as the constant 'override-artifact'. SysMetadataRepository.delete opens with assertAllowed(ref.type, opts.intent), which refuses every type that is not allowOrgOverride — 'object' among them — so a commit that created an object could not be reverted at all, and the first-build undo left the package half-reverted with success: false. The intent is now derived PER ITEM from isArtifactBacked, exactly as the sibling delete caller deleteMetaItem and the sibling revert caller rollbackMetaItem already derive it, so all three agree. The repository's gate is untouched: a genuinely artifact-backed item still resolves to 'override-artifact' and is still refused with NOT_OVERRIDABLE. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W6bLax4KMrSfnE1ydFU8Dw --- .../revert-commit-soft-remove-intent.md | 48 ++++ packages/metadata-protocol/src/protocol.ts | 48 +++- .../src/protocol-commit-history.test.ts | 219 ++++++++++++++++++ 3 files changed, 308 insertions(+), 7 deletions(-) create mode 100644 .changeset/revert-commit-soft-remove-intent.md 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 e2e42453c3..ac1e89db16 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -10205,12 +10205,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', }); } @@ -10236,12 +10270,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); + }); +});