From 425fa41ffb2d9447cec6601cb96f24968cebbe52 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 07:33:05 +0000 Subject: [PATCH] fix(metadata-protocol): revertCommit states its write intent per item (#6563) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `revertCommit` restored an edited artifact through `repo.restoreVersion(...)` with no `intent`, so `SysMetadataRepository.restoreVersion` fell back to its `?? 'override-artifact'` default and `put`'s `assertAllowed` refused every type that is not `allowOrgOverride` — `object` among them. Every `object` item of a reverted commit came back in `failed[]` as `NOT_OVERRIDABLE`, so the ADR-0067 package-commit undo could not revert the metadata type Studio creates most, while the same edit reverted fine through `rollbackMetaItem`, which derives the intent instead of defaulting it. The intent is now derived per item — `isArtifactBacked` gives `'override-artifact'`, otherwise `'runtime-only'` — because a commit is a batch that mixes runtime-created objects with overlays on packaged artifacts. The repository default is untouched: it is right for callers that mean it, and an artifact-backed object is still refused with `NOT_OVERRIDABLE`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDU3qAuJyajAQm3GkUXdfA --- .changeset/revert-commit-write-intent.md | 41 ++++ packages/metadata-protocol/src/protocol.ts | 28 +++ .../src/protocol-commit-history.test.ts | 211 +++++++++++++++++- 3 files changed, 275 insertions(+), 5 deletions(-) create mode 100644 .changeset/revert-commit-write-intent.md diff --git a/.changeset/revert-commit-write-intent.md b/.changeset/revert-commit-write-intent.md new file mode 100644 index 0000000000..351d3f2598 --- /dev/null +++ b/.changeset/revert-commit-write-intent.md @@ -0,0 +1,41 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): `revertCommit` states its write intent per item, so an `object` overlay can be reverted at all (#6563) + +`ObjectStackProtocolImplementation.revertCommit` restored an edited artifact +through `repo.restoreVersion(ref, prevVersion, { actor, source, message })` — with +no `intent`. `SysMetadataRepository.restoreVersion` therefore fell back to its +`?? 'override-artifact'` default, `put` opened with +`assertAllowed(ref.type, opts.intent)`, and that gate refuses every type whose +registry entry is not `allowOrgOverride`. `object` is exactly such a type, so +every `object` item 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, ... +``` + +The package-commit undo (ADR-0067) therefore could not revert the metadata type +Studio and AI-built apps create most, while the same edit reverted fine one +artifact at a time through the version-history revert — the two user-facing +revert paths disagreed about what is revertable. The failure was per item, so +the call still answered `success` overall with a populated `failed[]`, which +reads as a flaky revert rather than a systematic refusal. +`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 +object was untouched. + +`revertCommit` now derives the intent from the artifact the way its sibling +`rollbackMetaItem` already does — `isArtifactBacked` gives `'override-artifact'`, +otherwise `'runtime-only'` — and does it **per item**, because a commit is a +batch that routinely mixes a runtime-created object with an overlay on a +packaged view. + +The repository's default 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 it is. So the gate is not widened — 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 ad818cbe5f..3219039c0f 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -10163,10 +10163,38 @@ export class ObjectStackProtocolImplementation implements reverted.push({ type: it.type, name: it.name, action: 'removed' }); } else if (it.prevVersion !== null && it.prevVersion !== undefined) { // Edited an existing artifact → restore the pre-commit body. + // + // [#6563] The write INTENT is derived per item, exactly as the + // sibling caller {@link rollbackMetaItem} derives it. Left + // unstated, `SysMetadataRepository.restoreVersion` defaults to + // `'override-artifact'` and `put`'s `assertAllowed` refuses every + // type that is not `allowOrgOverride` — `object` among them — so + // each `object` item of a reverted commit came back in `failed[]` + // as `NOT_OVERRIDABLE` while the same edit reverted fine one + // artifact at a time through the version-history revert. The + // repository's default is right for callers that genuinely mean + // "override a packaged artifact"; the defect was this caller never + // saying which of the two cases it is. + // + // Per ITEM, not per call: `revertCommit` reverts a batch, and a + // commit routinely mixes a runtime-created object with an overlay + // on a packaged view. A genuinely artifact-backed item still + // 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). + const intent: 'override-artifact' | 'runtime-only' = + this.isArtifactBacked(it.type, it.name) ? 'override-artifact' : 'runtime-only'; await repo.restoreVersion(ref, it.prevVersion, { actor, source: 'protocol.revertCommit', message: `revert commit ${request.commitId}`, + intent, }); reverted.push({ type: it.type, name: it.name, action: 'restored' }); } diff --git a/packages/objectql/src/protocol-commit-history.test.ts b/packages/objectql/src/protocol-commit-history.test.ts index 334c6891dc..83b811971b 100644 --- a/packages/objectql/src/protocol-commit-history.test.ts +++ b/packages/objectql/src/protocol-commit-history.test.ts @@ -313,11 +313,11 @@ function makeRealRepoHarness(seedCommits: any[] = []) { * The reverted artifact is a `view`, not an `object`, and deliberately: the * repository's `assertAllowed` refuses `override-artifact` on `object` (not * `allowOrgOverride`), and `revertCommit` — unlike `rollbackMetaItem`, which - * derives `runtime-only` for a runtime-created artifact — passes no intent, so - * an object item fails that gate BEFORE reaching the scoping this file pins. - * That is a separate defect of the same family, filed as #6563; using an - * overlay-allowed type keeps this pin measuring one thing. The repository line - * under test is type-agnostic. + * derives `runtime-only` for a runtime-created artifact — passed no intent, so + * an object item failed that gate BEFORE reaching the scoping this file pins. + * That was a separate defect of the same family, fixed as #6563 (whose own pins + * are the last block in this file); using an overlay-allowed type keeps this pin + * measuring one thing. The repository line under test is type-agnostic. */ const gridBody = (label: string) => ({ name: 'myapp_case_grid', type: 'grid', label, columns: ['id', 'title'], @@ -431,3 +431,204 @@ describe('#6215 — revertCommit restores a PACKAGE-BOUND overlay row', () => { expect(JSON.parse(stored[0].metadata).label).toBe('Renamed'); }); }); + +/** + * #6563 — `revertCommit` states its write INTENT, per item. + * + * The block above could only be written about a `view`: `revertCommit` passed + * no `intent`, so `SysMetadataRepository.restoreVersion` fell back to its + * `?? 'override-artifact'` default, `put` opened with + * `assertAllowed(ref.type, opts.intent)`, and every type that is not + * `allowOrgOverride` was refused — `object` among them. So the metadata type + * Studio creates most could not be reverted through the package-commit undo AT + * ALL, while the same edit reverted fine one artifact at a time through + * `rollbackMetaItem`, which derives the intent instead of defaulting it. The + * two user-facing revert paths disagreed about what is revertable. + * + * The repository default is unchanged and correct — it is right for callers + * that genuinely mean "override a packaged artifact". What was missing is this + * caller saying which of the two cases each item is, which is why the fix is a + * per-item `isArtifactBacked` derivation in `revertCommit` and not a looser + * gate: the artifact-backed refusal below is the half that must NOT move. + */ + +const invoiceBody = (name: string, extra?: Record) => ({ + name, + label: 'Invoice', + fields: { + name: { name: 'name', type: 'text', label: 'Name' }, + amount: { name: 'amount', type: 'number', label: 'Amount' }, + }, + ...extra, +}); + +/** v2 of the same object: the commit's edit added a field. */ +const evolvedInvoiceBody = (name: string) => { + const body = invoiceBody(name); + (body.fields as Record).due_date = { name: 'due_date', type: 'date', label: 'Due' }; + return body; +}; + +/** The exact measured repro: saved twice through `saveMetaItem`, then reverted. */ +async function seedObjectEdit(protocol: any, name: string, packageId?: string) { + const pkg = packageId ? { packageId } : {}; + await protocol.saveMetaItem({ type: 'object', name, ...pkg, item: invoiceBody(name) }); + await protocol.saveMetaItem({ type: 'object', name, ...pkg, item: evolvedInvoiceBody(name) }); +} + +const storedFields = (rows: Map, name: string) => { + const stored = Array.from(rows.values()).filter((r) => r.name === name); + expect(stored).toHaveLength(1); + return { row: stored[0], fields: Object.keys(JSON.parse(stored[0].metadata).fields) }; +}; + +const objectCommit = (over: Record & { id: string; items: any[] }) => applyCommit({ + package_id: APP_PKG, + created_at: '2026-08-08T00:00:02.000Z', + ...over, +} as any); + +describe('#6563 — revertCommit restores a runtime-created `object`', () => { + it('a package-bound object reverts: revertedCount 1, failed [], pre-commit body back', async () => { + const { protocol, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_obj', + items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }], + })]); + await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG); + + const res = await protocol.revertCommit({ commitId: 'cmt_obj' }); + + // Pre-fix, verbatim: revertedCount 0 / failedCount 1 carrying + // "[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry." + expect(res.failed).toEqual([]); + expect(res.revertedCount).toBe(1); + expect(res.reverted[0]).toMatchObject({ type: 'object', name: 'myapp_invoice', action: 'restored' }); + // The restore also still lands IN PLACE on the bound row (#6215's half, now + // exercised on the type that could not reach it). + const { row, fields } = storedFields(rows, 'myapp_invoice'); + expect(row.package_id).toBe(APP_PKG); + expect(fields).not.toContain('due_date'); + }); + + it('a package-LESS object reverts identically — the binding was never the cause', async () => { + const { protocol, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_obj_global', + package_id: null, + items: [{ type: 'object', name: 'global_invoice', existedBefore: true, prevVersion: 1 }], + })]); + await seedObjectEdit(protocol, 'global_invoice'); + + const res = await protocol.revertCommit({ commitId: 'cmt_obj_global' }); + + expect(res.failed).toEqual([]); + expect(res.revertedCount).toBe(1); + const { row, fields } = storedFields(rows, 'global_invoice'); + expect(row.package_id).toBeNull(); + expect(fields).not.toContain('due_date'); + }); + + /** + * The refusal that must SURVIVE the fix. Deriving the intent is the caller + * stating its case, not a wider gate: an object a code package really ships + * resolves to `'override-artifact'` and is refused exactly as before. + * + * Staging it needs the ordering a real deployment has anyway — the overlay + * rows are 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 (the same + * lever #4636's B-minimal counter-example pulls). + * + * 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 is asserted at the + * throwing surface in `protocol-writepath-object-ownership.test.ts`. + */ + it('still REFUSES an artifact-backed object: NOT_OVERRIDABLE, nothing written', async () => { + const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_obj_artifact', + items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }], + })]); + await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG); + registry.registerObject(invoiceBody('myapp_invoice') as never, APP_PKG); + + const res = await protocol.revertCommit({ commitId: 'cmt_obj_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 edit the commit made is still the live body. + expect(storedFields(rows, 'myapp_invoice').fields).toContain('due_date'); + }); + + /** + * PER ITEM, not per call — the half a single-item fixture cannot see. One + * commit, two objects, opposite verdicts: a `for` loop that hoisted one + * intent for the batch would have to pick one and be wrong about the other. + */ + it('derives the intent PER ITEM: one object restored, its artifact-backed neighbour refused', async () => { + const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({ + id: 'cmt_obj_mixed', + items: [ + { type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }, + { type: 'object', name: 'myapp_quote', existedBefore: true, prevVersion: 1 }, + ], + })]); + await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG); + await seedObjectEdit(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_obj_mixed' }); + + expect(res.reverted).toEqual([ + { type: 'object', name: 'myapp_invoice', action: 'restored' }, + ]); + expect(res.failed).toHaveLength(1); + expect(res.failed[0]).toMatchObject({ name: 'myapp_quote', code: 'NOT_OVERRIDABLE' }); + expect(storedFields(rows, 'myapp_invoice').fields).not.toContain('due_date'); + expect(storedFields(rows, 'myapp_quote').fields).toContain('due_date'); + }); +}); + +/** + * #6563 — the inheritance. `rollbackToPackageCommit` reverts through the SAME + * loop, one `revertCommit` per apply commit newer than the target. + * + * Its own return shape cannot show this defect: `revertCommit` converts a + * per-item refusal into `failed[]` rather than throwing, so the rollback + * recorded the commit as reverted and answered `success: true` while the object + * was untouched. The assertion that goes red pre-fix is therefore the STORED + * BODY, not the status — asserting `success` alone would have been green on the + * defect. + */ +describe('#6563 — rollbackToPackageCommit inherits the per-item intent', () => { + it('rolls an object edit back through the loop — and the stored body really moved', async () => { + const { protocol, rows } = makeRealRepoHarness([ + objectCommit({ id: 'cmt_base', items: [], created_at: '2026-08-08T00:00:01.000Z' }), + objectCommit({ + id: 'cmt_edit', + items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }], + created_at: '2026-08-08T00:00:02.000Z', + }), + ]); + await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG); + + const res = await protocol.rollbackToPackageCommit({ commitId: 'cmt_base' }); + + expect(res.revertedCommits).toEqual(['cmt_edit']); + expect(res.failed).toEqual([]); + // `success: true` was ALREADY true pre-fix — this is the line that was not. + const { row, fields } = storedFields(rows, 'myapp_invoice'); + expect(row.package_id).toBe(APP_PKG); + expect(fields).not.toContain('due_date'); + }); +});