Skip to content

Commit 425fa41

Browse files
committed
fix(metadata-protocol): revertCommit states its write intent per item (#6563)
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDU3qAuJyajAQm3GkUXdfA
1 parent e39dd66 commit 425fa41

3 files changed

Lines changed: 275 additions & 5 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): `revertCommit` states its write intent per item, so an `object` overlay can be reverted at all (#6563)
6+
7+
`ObjectStackProtocolImplementation.revertCommit` restored an edited artifact
8+
through `repo.restoreVersion(ref, prevVersion, { actor, source, message })` — with
9+
no `intent`. `SysMetadataRepository.restoreVersion` therefore fell back to its
10+
`?? 'override-artifact'` default, `put` opened with
11+
`assertAllowed(ref.type, opts.intent)`, and that gate refuses every type whose
12+
registry entry is not `allowOrgOverride`. `object` is exactly such a type, so
13+
every `object` item of a reverted commit came back in `failed[]`:
14+
15+
```
16+
[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry.
17+
Overlay-allowed: view, page, dashboard, app, action, report, dataset, ...
18+
```
19+
20+
The package-commit undo (ADR-0067) therefore could not revert the metadata type
21+
Studio and AI-built apps create most, while the same edit reverted fine one
22+
artifact at a time through the version-history revert — the two user-facing
23+
revert paths disagreed about what is revertable. The failure was per item, so
24+
the call still answered `success` overall with a populated `failed[]`, which
25+
reads as a flaky revert rather than a systematic refusal.
26+
`rollbackToPackageCommit` reverts through the same loop and inherited it, and
27+
there the symptom was quieter still: a per-item refusal never throws, so the
28+
rollback recorded the commit as reverted and answered `success: true` while the
29+
object was untouched.
30+
31+
`revertCommit` now derives the intent from the artifact the way its sibling
32+
`rollbackMetaItem` already does — `isArtifactBacked` gives `'override-artifact'`,
33+
otherwise `'runtime-only'` — and does it **per item**, because a commit is a
34+
batch that routinely mixes a runtime-created object with an overlay on a
35+
packaged view.
36+
37+
The repository's default is deliberately unchanged: it is right for callers that
38+
genuinely mean "override a packaged artifact", and the defect was this caller
39+
never saying which of the two cases it is. So the gate is not widened — an
40+
object a code package really ships still resolves to `'override-artifact'` and
41+
is still refused with `NOT_OVERRIDABLE`, which is pinned alongside the fix.

packages/metadata-protocol/src/protocol.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10163,10 +10163,38 @@ export class ObjectStackProtocolImplementation implements
1016310163
reverted.push({ type: it.type, name: it.name, action: 'removed' });
1016410164
} else if (it.prevVersion !== null && it.prevVersion !== undefined) {
1016510165
// Edited an existing artifact → restore the pre-commit body.
10166+
//
10167+
// [#6563] The write INTENT is derived per item, exactly as the
10168+
// sibling caller {@link rollbackMetaItem} derives it. Left
10169+
// unstated, `SysMetadataRepository.restoreVersion` defaults to
10170+
// `'override-artifact'` and `put`'s `assertAllowed` refuses every
10171+
// type that is not `allowOrgOverride` — `object` among them — so
10172+
// each `object` item of a reverted commit came back in `failed[]`
10173+
// as `NOT_OVERRIDABLE` while the same edit reverted fine one
10174+
// artifact at a time through the version-history revert. The
10175+
// repository's default is right for callers that genuinely mean
10176+
// "override a packaged artifact"; the defect was this caller never
10177+
// saying which of the two cases it is.
10178+
//
10179+
// Per ITEM, not per call: `revertCommit` reverts a batch, and a
10180+
// commit routinely mixes a runtime-created object with an overlay
10181+
// on a packaged view. A genuinely artifact-backed item still
10182+
// resolves to `'override-artifact'` and is still refused — the
10183+
// derivation states the case, it does not widen the gate.
10184+
//
10185+
// Two neighbours are deliberately NOT changed here, each filed
10186+
// with its own measurement: the soft-remove limb above states the
10187+
// same intent as a CONSTANT, so a commit that CREATED an object
10188+
// still cannot be reverted (#6620); and neither limb refreshes the
10189+
// SchemaRegistry the way `rollbackMetaItem` does, so a restored
10190+
// body is persisted but not yet dispatched on (#6621).
10191+
const intent: 'override-artifact' | 'runtime-only' =
10192+
this.isArtifactBacked(it.type, it.name) ? 'override-artifact' : 'runtime-only';
1016610193
await repo.restoreVersion(ref, it.prevVersion, {
1016710194
actor,
1016810195
source: 'protocol.revertCommit',
1016910196
message: `revert commit ${request.commitId}`,
10197+
intent,
1017010198
});
1017110199
reverted.push({ type: it.type, name: it.name, action: 'restored' });
1017210200
}

packages/objectql/src/protocol-commit-history.test.ts

Lines changed: 206 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -313,11 +313,11 @@ function makeRealRepoHarness(seedCommits: any[] = []) {
313313
* The reverted artifact is a `view`, not an `object`, and deliberately: the
314314
* repository's `assertAllowed` refuses `override-artifact` on `object` (not
315315
* `allowOrgOverride`), and `revertCommit` — unlike `rollbackMetaItem`, which
316-
* derives `runtime-only` for a runtime-created artifact — passes no intent, so
317-
* an object item fails that gate BEFORE reaching the scoping this file pins.
318-
* That is a separate defect of the same family, filed as #6563; using an
319-
* overlay-allowed type keeps this pin measuring one thing. The repository line
320-
* under test is type-agnostic.
316+
* derives `runtime-only` for a runtime-created artifact — passed no intent, so
317+
* an object item failed that gate BEFORE reaching the scoping this file pins.
318+
* That was a separate defect of the same family, fixed as #6563 (whose own pins
319+
* are the last block in this file); using an overlay-allowed type keeps this pin
320+
* measuring one thing. The repository line under test is type-agnostic.
321321
*/
322322
const gridBody = (label: string) => ({
323323
name: 'myapp_case_grid', type: 'grid', label, columns: ['id', 'title'],
@@ -431,3 +431,204 @@ describe('#6215 — revertCommit restores a PACKAGE-BOUND overlay row', () => {
431431
expect(JSON.parse(stored[0].metadata).label).toBe('Renamed');
432432
});
433433
});
434+
435+
/**
436+
* #6563 — `revertCommit` states its write INTENT, per item.
437+
*
438+
* The block above could only be written about a `view`: `revertCommit` passed
439+
* no `intent`, so `SysMetadataRepository.restoreVersion` fell back to its
440+
* `?? 'override-artifact'` default, `put` opened with
441+
* `assertAllowed(ref.type, opts.intent)`, and every type that is not
442+
* `allowOrgOverride` was refused — `object` among them. So the metadata type
443+
* Studio creates most could not be reverted through the package-commit undo AT
444+
* ALL, while the same edit reverted fine one artifact at a time through
445+
* `rollbackMetaItem`, which derives the intent instead of defaulting it. The
446+
* two user-facing revert paths disagreed about what is revertable.
447+
*
448+
* The repository default is unchanged and correct — it is right for callers
449+
* that genuinely mean "override a packaged artifact". What was missing is this
450+
* caller saying which of the two cases each item is, which is why the fix is a
451+
* per-item `isArtifactBacked` derivation in `revertCommit` and not a looser
452+
* gate: the artifact-backed refusal below is the half that must NOT move.
453+
*/
454+
455+
const invoiceBody = (name: string, extra?: Record<string, unknown>) => ({
456+
name,
457+
label: 'Invoice',
458+
fields: {
459+
name: { name: 'name', type: 'text', label: 'Name' },
460+
amount: { name: 'amount', type: 'number', label: 'Amount' },
461+
},
462+
...extra,
463+
});
464+
465+
/** v2 of the same object: the commit's edit added a field. */
466+
const evolvedInvoiceBody = (name: string) => {
467+
const body = invoiceBody(name);
468+
(body.fields as Record<string, unknown>).due_date = { name: 'due_date', type: 'date', label: 'Due' };
469+
return body;
470+
};
471+
472+
/** The exact measured repro: saved twice through `saveMetaItem`, then reverted. */
473+
async function seedObjectEdit(protocol: any, name: string, packageId?: string) {
474+
const pkg = packageId ? { packageId } : {};
475+
await protocol.saveMetaItem({ type: 'object', name, ...pkg, item: invoiceBody(name) });
476+
await protocol.saveMetaItem({ type: 'object', name, ...pkg, item: evolvedInvoiceBody(name) });
477+
}
478+
479+
const storedFields = (rows: Map<string, any>, name: string) => {
480+
const stored = Array.from(rows.values()).filter((r) => r.name === name);
481+
expect(stored).toHaveLength(1);
482+
return { row: stored[0], fields: Object.keys(JSON.parse(stored[0].metadata).fields) };
483+
};
484+
485+
const objectCommit = (over: Record<string, unknown> & { id: string; items: any[] }) => applyCommit({
486+
package_id: APP_PKG,
487+
created_at: '2026-08-08T00:00:02.000Z',
488+
...over,
489+
} as any);
490+
491+
describe('#6563 — revertCommit restores a runtime-created `object`', () => {
492+
it('a package-bound object reverts: revertedCount 1, failed [], pre-commit body back', async () => {
493+
const { protocol, rows } = makeRealRepoHarness([objectCommit({
494+
id: 'cmt_obj',
495+
items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }],
496+
})]);
497+
await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG);
498+
499+
const res = await protocol.revertCommit({ commitId: 'cmt_obj' });
500+
501+
// Pre-fix, verbatim: revertedCount 0 / failedCount 1 carrying
502+
// "[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry."
503+
expect(res.failed).toEqual([]);
504+
expect(res.revertedCount).toBe(1);
505+
expect(res.reverted[0]).toMatchObject({ type: 'object', name: 'myapp_invoice', action: 'restored' });
506+
// The restore also still lands IN PLACE on the bound row (#6215's half, now
507+
// exercised on the type that could not reach it).
508+
const { row, fields } = storedFields(rows, 'myapp_invoice');
509+
expect(row.package_id).toBe(APP_PKG);
510+
expect(fields).not.toContain('due_date');
511+
});
512+
513+
it('a package-LESS object reverts identically — the binding was never the cause', async () => {
514+
const { protocol, rows } = makeRealRepoHarness([objectCommit({
515+
id: 'cmt_obj_global',
516+
package_id: null,
517+
items: [{ type: 'object', name: 'global_invoice', existedBefore: true, prevVersion: 1 }],
518+
})]);
519+
await seedObjectEdit(protocol, 'global_invoice');
520+
521+
const res = await protocol.revertCommit({ commitId: 'cmt_obj_global' });
522+
523+
expect(res.failed).toEqual([]);
524+
expect(res.revertedCount).toBe(1);
525+
const { row, fields } = storedFields(rows, 'global_invoice');
526+
expect(row.package_id).toBeNull();
527+
expect(fields).not.toContain('due_date');
528+
});
529+
530+
/**
531+
* The refusal that must SURVIVE the fix. Deriving the intent is the caller
532+
* stating its case, not a wider gate: an object a code package really ships
533+
* resolves to `'override-artifact'` and is refused exactly as before.
534+
*
535+
* Staging it needs the ordering a real deployment has anyway — the overlay
536+
* rows are authored while the name is runtime-only, and the artifact arrives
537+
* with the package that later claims it. `registerObject(body, pkg)` with no
538+
* `_provenance` is the shape `applyProtection` stamps as `'package'`, which
539+
* is what `getArtifactItem` reads and `isArtifactBacked` answers on (the same
540+
* lever #4636's B-minimal counter-example pulls).
541+
*
542+
* Envelope note (ADR-0112): `revertCommit` converts a per-item throw into a
543+
* `failed[]` record whose DECLARED shape is `{ type, name, error, code? }` —
544+
* no `status`. So `code` is asserted here together with the condition's own
545+
* first sentence, and the full `{ code, status }` pair is asserted at the
546+
* throwing surface in `protocol-writepath-object-ownership.test.ts`.
547+
*/
548+
it('still REFUSES an artifact-backed object: NOT_OVERRIDABLE, nothing written', async () => {
549+
const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({
550+
id: 'cmt_obj_artifact',
551+
items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }],
552+
})]);
553+
await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG);
554+
registry.registerObject(invoiceBody('myapp_invoice') as never, APP_PKG);
555+
556+
const res = await protocol.revertCommit({ commitId: 'cmt_obj_artifact' });
557+
558+
expect(res.revertedCount).toBe(0);
559+
expect(res.failedCount).toBe(1);
560+
expect(res.failed[0]).toMatchObject({
561+
type: 'object',
562+
name: 'myapp_invoice',
563+
code: 'NOT_OVERRIDABLE',
564+
});
565+
expect(res.failed[0].error).toContain(
566+
`[NOT_OVERRIDABLE] 'object' is not allowOrgOverride in the registry.`,
567+
);
568+
// Refused means refused: the edit the commit made is still the live body.
569+
expect(storedFields(rows, 'myapp_invoice').fields).toContain('due_date');
570+
});
571+
572+
/**
573+
* PER ITEM, not per call — the half a single-item fixture cannot see. One
574+
* commit, two objects, opposite verdicts: a `for` loop that hoisted one
575+
* intent for the batch would have to pick one and be wrong about the other.
576+
*/
577+
it('derives the intent PER ITEM: one object restored, its artifact-backed neighbour refused', async () => {
578+
const { protocol, registry, rows } = makeRealRepoHarness([objectCommit({
579+
id: 'cmt_obj_mixed',
580+
items: [
581+
{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 },
582+
{ type: 'object', name: 'myapp_quote', existedBefore: true, prevVersion: 1 },
583+
],
584+
})]);
585+
await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG);
586+
await seedObjectEdit(protocol, 'myapp_quote', APP_PKG);
587+
// Only the quote is claimed by a code artifact.
588+
registry.registerObject(invoiceBody('myapp_quote') as never, APP_PKG);
589+
590+
const res = await protocol.revertCommit({ commitId: 'cmt_obj_mixed' });
591+
592+
expect(res.reverted).toEqual([
593+
{ type: 'object', name: 'myapp_invoice', action: 'restored' },
594+
]);
595+
expect(res.failed).toHaveLength(1);
596+
expect(res.failed[0]).toMatchObject({ name: 'myapp_quote', code: 'NOT_OVERRIDABLE' });
597+
expect(storedFields(rows, 'myapp_invoice').fields).not.toContain('due_date');
598+
expect(storedFields(rows, 'myapp_quote').fields).toContain('due_date');
599+
});
600+
});
601+
602+
/**
603+
* #6563 — the inheritance. `rollbackToPackageCommit` reverts through the SAME
604+
* loop, one `revertCommit` per apply commit newer than the target.
605+
*
606+
* Its own return shape cannot show this defect: `revertCommit` converts a
607+
* per-item refusal into `failed[]` rather than throwing, so the rollback
608+
* recorded the commit as reverted and answered `success: true` while the object
609+
* was untouched. The assertion that goes red pre-fix is therefore the STORED
610+
* BODY, not the status — asserting `success` alone would have been green on the
611+
* defect.
612+
*/
613+
describe('#6563 — rollbackToPackageCommit inherits the per-item intent', () => {
614+
it('rolls an object edit back through the loop — and the stored body really moved', async () => {
615+
const { protocol, rows } = makeRealRepoHarness([
616+
objectCommit({ id: 'cmt_base', items: [], created_at: '2026-08-08T00:00:01.000Z' }),
617+
objectCommit({
618+
id: 'cmt_edit',
619+
items: [{ type: 'object', name: 'myapp_invoice', existedBefore: true, prevVersion: 1 }],
620+
created_at: '2026-08-08T00:00:02.000Z',
621+
}),
622+
]);
623+
await seedObjectEdit(protocol, 'myapp_invoice', APP_PKG);
624+
625+
const res = await protocol.rollbackToPackageCommit({ commitId: 'cmt_base' });
626+
627+
expect(res.revertedCommits).toEqual(['cmt_edit']);
628+
expect(res.failed).toEqual([]);
629+
// `success: true` was ALREADY true pre-fix — this is the line that was not.
630+
const { row, fields } = storedFields(rows, 'myapp_invoice');
631+
expect(row.package_id).toBe(APP_PKG);
632+
expect(fields).not.toContain('due_date');
633+
});
634+
});

0 commit comments

Comments
 (0)