diff --git a/.changeset/list-commits-env-wide-org-scope.md b/.changeset/list-commits-env-wide-org-scope.md new file mode 100644 index 0000000000..5b80171fe9 --- /dev/null +++ b/.changeset/list-commits-env-wide-org-scope.md @@ -0,0 +1,63 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): `listCommits` no longer hides a package's env-wide commit history (#7779) + +`protocol.listCommits` selected the ADR-0067 timeline with the same strict +`organization_id` equality that #7705 (PR #7771) had just replaced one function +above it: + +```ts +const where = { package_id: request.packageId }; +if (request.organizationId) where.organization_id = request.organizationId; +``` + +`organization_id = ''` matches no row whose column is NULL, so a session +with an active organization was shown **none** of the commits recorded +env-wide. The commits were in `sys_metadata_commit` the whole time; the read +could not see them. + +**Env-wide commit rows are actually written — this was live, not latent, and +that was measured before the fix was written.** `recordPackageCommit` stores +`organization_id: request.organizationId ?? null`, and the only door into a +publish (the dispatcher's `POST /packages/:id/publish-drafts`) forwards an +organization only when `resolveActiveOrganizationId` yields one. That resolver +answers `undefined` both for a session with no active organization and for *any* +throw on the auth seam, since its whole body is `catch`-wrapped. A publish made +before an organization is selected — or during a transient auth failure — is +therefore recorded env-wide permanently, because the timeline is append-only. +Driven on a real engine over SQLite, a no-org publish wrote +`organization_id: null` and the org-scoped read of that same package then +returned `[]`. + +**The blast radius is wider than audit.** `rollbackToPackageCommit` derives the +set of commits it must undo *from this list*, so a commit the list could not see +was a commit the rollback silently skipped: measured before the fix, an +org-scoped rollback past an env-wide commit answered +`{success: true, revertedCommits: []}` while that commit's changes stayed live. +A rollback that reports success and rolls back nothing is a correctness defect, +not a reporting one. + +An org-scoped read now matches its own organization **or** env-wide — the +`$or [{organization_id: oid}, {organization_id: null}]` shape this package +already uses for the #3115 "orphaned draft" fix, the shape #7705 applied to the +sibling `deletePackage` read, and the shape the SQL driver's own implicit tenant +wall uses (`field = :tenant OR field IS NULL`, #2734). + +Both directions that must not widen are pinned: another organization's commits +stay invisible to an org-scoped read, and another package's commits are never +returned. Newest-first ordering is unchanged. The **no-org** branch is +deliberately left package-wide rather than narrowed to `organization_id IS +NULL` — narrowing it would hide every org-scoped commit from that door instead, +re-creating this bug pointed the other way, which is exactly why #7705 left its +own no-org branch alone. + +**Known remaining gap, reported on #7779 rather than fixed here** (this card +holds `protocol.ts`, a serialized file, for `listCommits` alone): `revertCommit` +and `rollbackToPackageCommit`'s own target lookups still carry the identical +strict equality. The consequence is now *loud* instead of silent — the rollback +above reports `success: false` naming the commit it could not resolve, rather +than claiming success over a no-op. That is strictly better and non-destructive, +but it is not the whole repair, and the new suite asserts it so the remainder +cannot drift unnoticed before its own card lands. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index c4bc93572f..7003ceb994 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -12114,7 +12114,42 @@ export class ObjectStackProtocolImplementation implements }>> { try { const where: Record = { package_id: request.packageId }; - if (request.organizationId) where.organization_id = request.organizationId; + // [#7779] Surface BOTH org-scoped and env-wide (`organization_id IS + // NULL`) commit rows to an org-scoped caller — the same defect and + // the same remedy as the sibling {@link deletePackage} read one + // function above (#7705), and as {@link + // SysMetadataRepository.listDrafts} (#3115) in this package. + // + // Env-wide commit rows are not hypothetical: {@link + // recordPackageCommit} stores `organization_id: request. + // organizationId ?? null`, and the ONLY door into a publish — the + // dispatcher's `POST /packages/:id/publish-drafts` — forwards an + // org only when `resolveActiveOrganizationId` yields one. That + // resolver answers `undefined` for a session with no active + // organization AND for every failure on the auth seam (it is + // `catch`-wrapped). So a publish made before an org was selected — + // or during a transient auth blip — lands its commit env-wide, + // permanently, and the strict equality then hid it from every + // org-scoped read of that package's timeline. + // + // This is NOT merely an observability miss. {@link + // rollbackToPackageCommit} derives the set of commits to undo from + // this list, so an invisible commit was silently never reverted: + // measured pre-fix, a rollback past an env-wide commit answered + // `{success: true, revertedCommits: []}` while that commit's + // changes stayed live. + // + // The no-org branch is deliberately NOT narrowed to + // `organization_id IS NULL`, exactly as #7705 left its own: a + // caller with no active org reads the package's whole timeline, + // and restricting it to env-wide rows would hide every org-scoped + // commit instead — the same bug pointed the other way. + if (request.organizationId) { + where.$or = [ + { organization_id: request.organizationId }, + { organization_id: null }, + ]; + } const rows = (await this.engine.find('sys_metadata_commit', { where, ...(request.limit ? { limit: request.limit } : {}), diff --git a/packages/runtime/src/package-list-commits-org-scope.integration.test.ts b/packages/runtime/src/package-list-commits-org-scope.integration.test.ts new file mode 100644 index 0000000000..e30ae7789d --- /dev/null +++ b/packages/runtime/src/package-list-commits-org-scope.integration.test.ts @@ -0,0 +1,323 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Real-engine regression for #7779 — `protocol.listCommits` asked for commit +// rows with a strict `organization_id` equality and therefore returned NONE of +// the rows stored env-wide, hiding whole stretches of a package's ADR-0067 +// timeline from any session that had an active organization. + +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 mechanism, and why it is LIVE rather than latent. + * + * `listCommits` selected its rows with the predicate its sibling one function + * above carried until #7705 (PR #7771) replaced it: + * + * const where = { package_id: request.packageId }; + * if (request.organizationId) where.organization_id = request.organizationId; + * + * `organization_id = 'org'` matches no row whose column is NULL, so an + * org-scoped read returned nothing for every commit recorded env-wide. + * + * --------------------------------------------------------------------------- + * Step one, MEASURED before the fix was written: are env-wide commit rows + * actually WRITTEN? + * --------------------------------------------------------------------------- + * Yes — on the only door there is. {@link recordPackageCommit} persists + * `organization_id: args.orgId`, and `orgId` is `request.organizationId ?? + * null`. The single door into a publish is the dispatcher's `POST + * /packages/:id/publish-drafts` + * (`packages/runtime/src/domains/packages.ts`), which forwards an org only + * when `resolveActiveOrganizationId` yields one — and that resolver + * (`packages/runtime/src/http-dispatcher.ts`) answers `undefined` both for a + * session with no active organization and for ANY throw on the auth seam, + * because its whole body is `catch`-wrapped. So a publish issued before an org + * is selected, or during a transient auth failure, records its commit env-wide + * — permanently, since the timeline is append-only. + * + * Driving that real path (no `organizationId`, real engine, real driver) + * produced `organization_id: null` in SQLite, and the org-scoped read of the + * same package then answered `[]`. This suite is that measurement. + * + * --------------------------------------------------------------------------- + * Why this is not "just" an observability defect + * --------------------------------------------------------------------------- + * The card framed the blast radius as audit/observability, in contrast with + * #7705's data-lifecycle one. Measurement widened it: + * {@link rollbackToPackageCommit} derives the set of commits it must undo FROM + * THIS LIST. A commit the list cannot see is a commit the rollback silently + * skips — measured pre-fix, an org-scoped `rollbackToPackageCommit` past an + * env-wide commit answered `{success: true, revertedCommits: []}` while that + * commit's changes stayed live. A rollback that reports success and rolls back + * nothing is a correctness defect, not a reporting one. + * + * --------------------------------------------------------------------------- + * Why a REAL engine and a REAL driver + * --------------------------------------------------------------------------- + * The question is whether `organization_id = 'org'` matches a NULL column. + * That is a property of the driver's SQL, not of a stub's `filter()` — a + * hand-built double answers whatever its author assumed. Both existing + * `deletePackage` suites stubbed `engine.find`, which is precisely why neither + * could see the sibling defect, and the ADR-0067 commit-history suites stub + * the store too. So this file seeds through the REAL publish path + * (`publishPackageDrafts`, the same call the dispatcher makes) and reads back + * what actually landed in SQLite. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE the revert was run + * --------------------------------------------------------------------------- + * Restoring the strict equality was predicted to turn the two positive cases + * red — the env-wide commit disappearing from the org-scoped list — and to + * leave both negative cases green, because strict equality is NARROWER than + * the `$or`: it cannot reach another organization's rows or another package's, + * and it does not touch the no-org branch at all. Measured on revert: exactly + * that. See the changeset for the recorded numbers. + */ + +const PKG = 'com.repro.commits'; +const OTHER_PKG = 'com.other.commits'; +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-7779-')); + cleanup.push(() => rmSync(dir, { recursive: true, force: true })); + + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'data.sqlite') }, + useNullAsDefault: true, + }); + // `sys_metadata_commit` is the table under test; the other three are what + // the real publish path writes through on its way to recording a commit. + 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(); + // `registerObject(schema, packageId)` — the second argument is REQUIRED. + // Omitting it compiles under this package's own `typecheck` script (which + // excludes `*.test.ts`) but is a raw TS2554 to the type-check-coverage + // ratchet, which measures `tsc --noEmit` with the tests put back. Registered + // under the PLATFORM package, never under `PKG`, so the table cannot be torn + // out from under the assertions that read it back. + for (const o of objects) engine.registry.registerObject(o, PLATFORM_PKG); + cleanup.push(() => { void engine.destroy(); }); + + // `'package-author'` is the genuine control-plane assembly's channel — the + // #4463 runtime authoring gate is for environment-channel writes and 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, label: string) => ({ + name, + label, + type: 'grid', + data: { provider: 'object', object: 'anything' }, + columns: ['id'], +}); + +/** + * Author one draft and publish it, which is what records ONE commit row. The + * commit's `organization_id` is the PUBLISH REQUEST's org (`?? null`), so + * omitting `organizationId` here 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, 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; +} + +/** Distinct `created_at` values — the timeline is sorted by that ISO string. */ +const tick = () => new Promise((r) => setTimeout(r, 5)); + +/** + * Seed one commit in each scope that matters. Returns the ids by role so the + * assertions name what they mean rather than an opaque position. + */ +async function seed(protocol: any) { + // The miss: recorded env-wide, because the publishing session had no + // active organization. + const envWide = await publishOne(protocol, { + view: 'commits_env', packageId: PKG, message: 'env-wide publish', + }); + await tick(); + // The caller's OWN org — the only rows the strict equality ever found. + const own = await publishOne(protocol, { + view: 'commits_own', packageId: PKG, organizationId: ACTIVE_ORG, message: 'own-org publish', + }); + await tick(); + // Negative 1 — same package, ANOTHER organization. Must stay invisible. + const foreign = await publishOne(protocol, { + view: 'commits_foreign', packageId: PKG, organizationId: OTHER_ORG, message: 'other-org publish', + }); + await tick(); + // Negative 2 — ANOTHER package, env-wide. Must never appear. + const otherPkg = await publishOne(protocol, { + view: 'commits_other_pkg', packageId: OTHER_PKG, message: 'other package publish', + }); + return { envWide, own, foreign, otherPkg }; +} + +const idsOf = (commits: any[]): string[] => commits.map((c) => c.id).sort(); + +describe('#7779 — org-scoped listCommits must not hide env-wide commit rows', () => { + it('a no-org publish really does record an env-wide commit row (the premise, measured)', async () => { + const { engine, protocol } = await boot(); + const id = await publishOne(protocol as any, { + view: 'commits_env', packageId: PKG, message: 'env-wide publish', + }); + + // Straight out of SQLite: the column really is NULL, so the strict + // equality below really has nothing to match. + const rows = (await engine.find('sys_metadata_commit', { where: {} })) as any[]; + expect(rows.map((r) => ({ id: r.id, org: r.organization_id ?? null }))).toEqual([ + { id, org: null }, + ]); + }); + + it('returns the env-wide commits to an org-scoped caller (was: returned none of them)', async () => { + const { protocol } = await boot(); + const { envWide, own } = await seed(protocol as any); + + const commits = await (protocol as any).listCommits({ + packageId: PKG, + organizationId: ACTIVE_ORG, + }); + + // The CONSEQUENCE: the caller's own commit AND the env-wide one. Before + // the fix this was `[own]` alone — `envWide` was missing outright. + expect(idsOf(commits)).toEqual([envWide, own].sort()); + + // Newest-first is the contract the timeline UI and + // `rollbackToPackageCommit` both rely on, and widening the predicate must + // not disturb it: `own` was published after `envWide`. + expect(commits[0].id).toBe(own); + expect(commits.map((c: any) => c.message)).toEqual(['own-org publish', 'env-wide publish']); + }); + + it('does NOT surface another organization’s commits', async () => { + const { protocol } = await boot(); + const { foreign } = await seed(protocol as any); + + const commits = await (protocol as any).listCommits({ + packageId: PKG, + organizationId: ACTIVE_ORG, + }); + + // The direction that must not widen. A commit timeline that leaked + // another tenant's authoring history would be a worse defect than the + // under-reporting this closes. + expect(idsOf(commits)).not.toContain(foreign); + }); + + it('does NOT surface another package’s commits', async () => { + const { protocol } = await boot(); + const { otherPkg } = await seed(protocol as any); + + const commits = await (protocol as any).listCommits({ + packageId: PKG, + organizationId: ACTIVE_ORG, + }); + + expect(idsOf(commits)).not.toContain(otherPkg); + }); + + it('a caller with NO org still sees the package’s whole timeline (the other door)', async () => { + const { protocol } = await boot(); + const { envWide, own, foreign } = await seed(protocol as any); + + // The no-org branch is deliberately left package-wide. Narrowing it to + // `organization_id IS NULL` — mirroring only half the #3115 shape — would + // hide every org-scoped commit from this door instead, re-creating the + // bug pointed the other way. #7705 left its own no-org branch alone for + // the same reason and this pins the equivalent here. + const commits = await (protocol as any).listCommits({ packageId: PKG }); + + expect(idsOf(commits)).toEqual([envWide, own, foreign].sort()); + }); + + it('the timeline the rollback planner reads now contains the env-wide commit', async () => { + const { protocol } = await boot(); + const p = protocol as any; + + // C1 under an active org, then C2 env-wide and strictly newer — the shape + // a session hits when its org resolution lapses between two publishes. + const c1 = await publishOne(p, { + view: 'roll_a', packageId: PKG, organizationId: ACTIVE_ORG, message: 'c1', + }); + await tick(); + const c2 = await publishOne(p, { view: 'roll_b', packageId: PKG, message: 'c2' }); + + // `rollbackToPackageCommit` filters THIS list to decide what to undo, so + // the fix is what makes C2 reachable by the planner at all. Before it, the + // list was `[c1]` and an org-scoped rollback to C1 answered + // `{success: true, revertedCommits: []}` — reporting a rollback it had not + // performed, with C2's changes still live. + const commits = await p.listCommits({ packageId: PKG, organizationId: ACTIVE_ORG }); + expect(idsOf(commits)).toEqual([c1, c2].sort()); + + // ⚠️ KNOWN REMAINING GAP, measured and reported on #7779 rather than fixed + // here — `packages/metadata-protocol/src/protocol.ts` is serialized and + // this card holds it for `listCommits` alone. + // + // `revertCommit` (its own `findOne`) and `rollbackToPackageCommit` (its + // target lookup) still carry the byte-identical strict equality. So the + // planner now SEES C2 and asks `revertCommit` to undo it, and that lookup + // still cannot find an env-wide row: the rollback reports + // `success: false` naming C2, instead of the silent `success: true` it + // reported before. That is strictly better — the failure is now loud, + // attributable and non-destructive rather than invisible — but it is not + // the whole repair, and this assertion is here so the remaining half + // cannot drift unnoticed before its own card lands. + const rollback = await p.rollbackToPackageCommit({ commitId: c1, organizationId: ACTIVE_ORG }); + expect(rollback.success).toBe(false); + expect(rollback.failed.map((f: any) => f.commitId)).toEqual([c2]); + }); +});