From 34b38852176a9bce07121f88398675ece460842a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:09:13 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): attribute a revert commit to the scope of what it reverted --- packages/metadata-protocol/src/protocol.ts | 33 +++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index ebc9e712c5..95ca5ae8fe 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -13711,8 +13711,39 @@ export class ObjectStackProtocolImplementation implements } // Record the revert as its own commit (append-only history). + // + // [#7860] The scope of the commit being REVERTED, not the request's — + // the same rule #7559 gave this function's items ({@link + // resolveMetaItemOrgScope}) and #7819 tier 2 gave {@link + // duplicatePackage}'s copies, now applied to the commit RECORD that + // documents them. `packageId` on this very call is already read off + // `row`; the org was the one field still taken from whoever asked. + // + // Reachable only since #7819 tier 1: before it, the lookup above + // answered COMMIT_NOT_FOUND for an env-wide row, so an org caller + // could not reach this line with a mismatched scope at all. + // + // The invariant it restores: a revert commit is visible to exactly + // the readers who can see the commit it reverts. Measured on a real + // driver, both directions were incoherent without it — + // + // env-wide commit reverted by an org caller: the revert row was + // stamped with that org, so a DIFFERENT org's `listCommits` showed + // the env-wide `apply` with no compensation anywhere after it — + // while the artifact really was removed env-wide (the items revert + // in the ROW's scope), i.e. the effect was global and the record + // private. That is the reporting defect this card was opened to + // measure, and it is not cosmetic: {@link rollbackToPackageCommit} + // plans from `listCommits`. + // + // org-scoped commit reverted by the no-org REST door: the revert + // row was stamped env-wide, so every OTHER org read a dangling + // `Revert: …` entry whose `parentCommitId` names a commit that + // door cannot see. + // + // Both collapse to one line because both are the same mismatch. const revertCommit = await this.recordPackageCommit({ - orgId, + orgId: (row.organization_id ?? null) as string | null, packageId: row.package_id, operation: 'revert', message: `Revert: ${row.message ?? request.commitId}`, From 7f593579f8c346d96c3b8190a03276cef774cc36 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:32:23 +0000 Subject: [PATCH 2/2] test(runtime): pin the revert-commit attribution invariant on a real driver Measured on real ObjectQL + SqlDriver: before the fix a different organization's listCommits showed the env-wide apply commit with no compensation, while the artifact was already withdrawn env-wide. Refs #7860 --- .../revert-commit-attribution-org-scope.md | 71 +++++ ...-attribution-org-scope.integration.test.ts | 242 ++++++++++++++++++ 2 files changed, 313 insertions(+) create mode 100644 .changeset/revert-commit-attribution-org-scope.md create mode 100644 packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts diff --git a/.changeset/revert-commit-attribution-org-scope.md b/.changeset/revert-commit-attribution-org-scope.md new file mode 100644 index 0000000000..dd2aef772b --- /dev/null +++ b/.changeset/revert-commit-attribution-org-scope.md @@ -0,0 +1,71 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): attribute a revert commit to the scope of the commit it reverts (#7860) + +`revertCommit` recorded its compensating commit under the **requesting +session's** organization: + +```ts +const orgId = request.organizationId ?? null; +// … +await this.recordPackageCommit({ orgId, packageId: row.package_id, … }); +``` + +`packageId` on that same call is already read off the reverted `row`; the org +was the one field still taken from whoever asked. It now reads +`row.organization_id ?? null` — the rule #7559 gave this function's **items** +(`resolveMetaItemOrgScope`) and #7819 tier 2 gave `duplicatePackage`'s copies, +applied to the commit **record** that documents them. + +## Filed as a question, settled by measurement + +The card was explicit that this was **not** filed as a defect: the behaviour is +self-consistent for the caller who performed the revert, and it asked for a +measurement before any choice between "attribute to the request" and "attribute +to the reverted row". Measured on a real ObjectQL + `SqlDriver` +(`better-sqlite3`), after an org-scoped revert of an env-wide commit: + +| reader | before | after | +|:--|:--|:--| +| the actor (`org_active`) | `[revert, apply]` | `[revert, apply]` | +| **a different organization** | **`[apply]`** | `[revert, apply]` | +| no-org (direct-mount REST) | `[revert, apply]` | `[revert, apply]` | + +The middle row is a concrete reporting defect, which is what settled the +question rather than a preference. What makes it more than cosmetic is the +artifact state measured alongside it: `sys_metadata` held **no** row for the +reverted view afterwards. Items revert in the **row's** scope (#7559), so the +artifact really was withdrawn env-wide — the effect was global while the record +was private, and a reader in another organization saw an `apply` commit that +was never compensated for an artifact already gone. Since #7814 +`rollbackToPackageCommit` **plans** from `listCommits`, so this list is not +merely an observability surface. + +The mirror direction is the same mismatch pointed the other way, and the same +line fixes it: a no-org caller reverting an **org-scoped** commit stamped the +revert env-wide, so every other organization read a dangling `Revert: …` whose +`parentCommitId` names a commit that door cannot see. Measured before: +`[revert]` for an unrelated org; after: `[]`. + +The invariant both collapse to: **a revert commit is visible to exactly the +readers who can see the commit it reverts.** + +## Reachability + +Only since #7819 tier 1. Before it the target lookup answered +`COMMIT_NOT_FOUND` (404) for an env-wide row, so an org-scoped caller could not +reach the attribution line with a mismatched scope at all — a dormant quirk +whose reachability was created by a fix in the same function. + +## Verification + +`packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts` +— real engine, real driver, seeded through the real publish path (a stubbed +`engine.find` cannot see NULL semantics). Ablation, with a rebuild between +measurements because these suites resolve `metadata-protocol` through its +`dist`: restoring the request-derived `orgId` turned exactly 3 of the 4 cases +red — `expected [ 'apply' ] to deeply equal [ 'revert', 'apply' ]` — and left +green precisely the case predicted to be unaffected, the actor's and the no-org +door's timelines. The sibling #7819 and #7814 suites stay green (18/18). diff --git a/packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts b/packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts new file mode 100644 index 0000000000..5dc3b45b03 --- /dev/null +++ b/packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts @@ -0,0 +1,242 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Real-engine regression for #7860 — `protocol.revertCommit` recorded its +// compensating commit under the REQUEST's organization +// (`recordPackageCommit({ orgId: request.organizationId ?? null })`) rather +// than under the scope of the commit it was reverting. + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { + SysMetadataObject, + SysMetadataHistoryObject, + SysMetadataAuditObject, + SysMetadataCommitObject, +} from '@objectstack/metadata-core'; + +/** + * The invariant, stated once: a revert commit is visible to exactly the + * readers who can see the commit it reverts. + * + * --------------------------------------------------------------------------- + * Why this was a REPORTING defect and not a design question + * --------------------------------------------------------------------------- + * The card (#7860) was filed explicitly NOT as a defect — the behaviour is + * self-consistent for the caller who performed the revert, and it asked for a + * measurement first: after an org-scoped revert of an env-wide commit, what + * does a DIFFERENT organization's `listCommits` show, and what does the no-org + * (direct-mount REST) door see? Measured here, on a real driver, before any + * edit: + * + * actor (`org_active`) → [revert, apply] coherent + * different org → [apply] the env-wide publish, with NO + * compensation anywhere after it + * no-org REST → [revert, apply] coherent + * + * The middle row is the defect, and what makes it more than cosmetic is the + * artifact state measured alongside it: `sys_metadata` held NO row for the + * reverted view afterwards. `revertCommit` resolves each item's scope from the + * ROW (#7559), so the artifact really was removed ENV-WIDE — the effect is + * global while the record documenting it was private. A reader in another + * organization saw an `apply` commit that was never compensated, for an + * artifact that had in fact already been withdrawn underneath it. And since + * #7814, `rollbackToPackageCommit` PLANS from `listCommits`, so this list is + * not merely an observability surface. + * + * The mirror direction is the same mismatch pointed the other way and is + * pinned below: a no-org caller reverting an ORG-SCOPED commit stamped the + * revert env-wide, so every other organization read a dangling `Revert: …` + * whose `parentCommitId` names a commit that door cannot see. + * + * --------------------------------------------------------------------------- + * Why a REAL engine and a REAL driver + * --------------------------------------------------------------------------- + * Every assertion here turns on whether `organization_id = 'org'` matches a + * NULL column — a property of the driver's SQL, not of a stub's `filter()`. + * The suites in this family that stub `engine.find` are structurally unable to + * see any of it. This file seeds through the REAL publish path, exactly as its + * siblings `package-revert-commit-org-scope.integration.test.ts` (#7819) and + * `package-list-commits-org-scope.integration.test.ts` (#7814), and lives in + * `packages/runtime` for the same reason they do: `metadata-protocol` cannot + * import `objectql` (dependency cycle). + * + * ⚠️ These suites resolve `metadata-protocol` through its `dist` while stack + * traces are source-mapped back to `src`, so an edited-but-unbuilt `src` looks + * like it is running while the old bytes execute. Every number above was taken + * with a rebuild between measurements. + * + * --------------------------------------------------------------------------- + * Reachability + * --------------------------------------------------------------------------- + * Only since #7819 tier 1. Before it the target lookup answered + * `COMMIT_NOT_FOUND` (404) for an env-wide row, so an org-scoped caller could + * not reach the attribution line with a mismatched scope at all. + */ + +const PKG = 'com.repro.attrib'; +const PLATFORM_PKG = '@objectstack/platform-objects'; +const ACTIVE_ORG = 'org_active'; +const OTHER_ORG = 'org_other'; + +let cleanup: Array<() => void> = []; +afterEach(() => { + for (const c of cleanup) c(); + cleanup = []; +}); + +/** REAL ObjectQL wired to a REAL SqlDriver over on-disk better-sqlite3. */ +async function boot() { + const dir = mkdtempSync(join(tmpdir(), 'os-7860-')); + cleanup.push(() => rmSync(dir, { recursive: true, force: true })); + + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + const objects = [ + SysMetadataObject, + SysMetadataHistoryObject, + SysMetadataAuditObject, + SysMetadataCommitObject, + ] as any[]; + await driver.initObjects(objects); + + const engine = new ObjectQL(); + engine.registerDriver(driver as any, true); + await engine.init(); + for (const o of objects) engine.registry.registerObject(o, PLATFORM_PKG); + cleanup.push(() => { void engine.destroy(); }); + + // `'package-author'` is the control-plane assembly's channel — the #4463 + // runtime authoring gate would otherwise refuse the seeding saves below. + const protocol = new ObjectStackProtocolImplementation( + engine as any, undefined, undefined, 'package-author', + ); + return { engine, protocol }; +} + +const viewBody = (name: string) => ({ + name, + label: name, + type: 'grid', + object: 'anything', // [#7741] the inline arm requires the object binding pair + viewKind: 'list', + data: { provider: 'object', object: 'anything' }, + columns: ['id'], +}); + +/** + * Author one draft and publish it — what records ONE commit row. The commit's + * `organization_id` is the PUBLISH REQUEST's org (`?? null`), so omitting + * `organizationId` reproduces exactly what the dispatcher sends when the + * session has no active organization. + */ +async function publishOne( + protocol: any, + args: { view: string; packageId: string; organizationId?: string; message: string }, +): Promise { + await protocol.saveMetaItem({ + type: 'view', + name: args.view, + item: viewBody(args.view), + packageId: args.packageId, + mode: 'draft', + }); + const res = await protocol.publishPackageDrafts({ + packageId: args.packageId, + ...(args.organizationId ? { organizationId: args.organizationId } : {}), + message: args.message, + }); + expect(res.success).toBe(true); + expect(res.commitId).toBeTruthy(); + return res.commitId as string; +} + +const ops = (commits: any[]) => commits.map((c) => c.operation); + +describe('#7860 — a revert commit is attributed to what it reverted, not to who asked', () => { + it('an org caller reverting an ENV-WIDE commit records the revert env-wide', async () => { + const { engine, protocol } = await boot(); + const p = protocol as any; + const envWide = await publishOne(p, { + view: 'attr_env', packageId: PKG, message: 'env-wide publish', + }); + + const result = await p.revertCommit({ commitId: envWide, organizationId: ACTIVE_ORG }); + expect(result.success).toBe(true); + + // Straight out of SQLite. Pre-fix this row carried `org_active`. + const rows = (await engine.find('sys_metadata_commit', { where: {} })) as any[]; + expect(rows.map((r) => ({ op: r.operation, org: r.organization_id ?? null }))).toEqual([ + { op: 'apply', org: null }, + { op: 'revert', org: null }, + ]); + }); + + it('a DIFFERENT organization sees the compensation, not a bare uncompensated publish', async () => { + const { engine, protocol } = await boot(); + const p = protocol as any; + const envWide = await publishOne(p, { + view: 'attr_env', packageId: PKG, message: 'env-wide publish', + }); + await p.revertCommit({ commitId: envWide, organizationId: ACTIVE_ORG }); + + // THE defect this card was opened to measure. Pre-fix: `['apply']` — the + // env-wide publish alone, with nothing recording that it was undone. + const asOther = await p.listCommits({ packageId: PKG, organizationId: OTHER_ORG }); + expect(ops(asOther)).toEqual(['revert', 'apply']); + expect(asOther[0].parentCommitId).toBe(envWide); + + // Why the omission mattered: the artifact really is gone ENV-WIDE (items + // revert in the ROW's scope, #7559), so the reader above was being shown + // an `apply` that had already been withdrawn underneath it. + const meta = (await engine.find('sys_metadata', { where: { name: 'attr_env' } })) as any[]; + expect(meta).toEqual([]); + }); + + it('the actor and the no-org REST door keep the timeline they already had', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const envWide = await publishOne(p, { + view: 'attr_env', packageId: PKG, message: 'env-wide publish', + }); + await p.revertCommit({ commitId: envWide, organizationId: ACTIVE_ORG }); + + // Both were coherent BEFORE the fix and must stay so after it — the change + // may only ADD the missing reader, never trade one blind spot for another. + expect(ops(await p.listCommits({ packageId: PKG, organizationId: ACTIVE_ORG }))) + .toEqual(['revert', 'apply']); + expect(ops(await p.listCommits({ packageId: PKG }))).toEqual(['revert', 'apply']); + }); + + it('mirror — the no-org door reverting an ORG-SCOPED commit records the revert in THAT org', async () => { + const { engine, protocol } = await boot(); + const p = protocol as any; + const owned = await publishOne(p, { + view: 'attr_owned', packageId: PKG, organizationId: ACTIVE_ORG, message: 'org_active publish', + }); + + const result = await p.revertCommit({ commitId: owned }); + expect(result.success).toBe(true); + + const rows = (await engine.find('sys_metadata_commit', { where: {} })) as any[]; + expect(rows.map((r) => ({ op: r.operation, org: r.organization_id ?? null }))).toEqual([ + { op: 'apply', org: ACTIVE_ORG }, + { op: 'revert', org: ACTIVE_ORG }, + ]); + + // Pre-fix the revert was stamped env-wide, so an unrelated organization + // read a DANGLING `Revert: …` whose parent it cannot see. The owning org + // and the no-org door still see the pair. + expect(await p.listCommits({ packageId: PKG, organizationId: OTHER_ORG })).toEqual([]); + expect(ops(await p.listCommits({ packageId: PKG, organizationId: ACTIVE_ORG }))) + .toEqual(['revert', 'apply']); + expect(ops(await p.listCommits({ packageId: PKG }))).toEqual(['revert', 'apply']); + }); +});