From b4ea578b82efb0aca1dcb114650be83a998c5034 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 21:01:34 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): let an org-scoped caller revert an env-wide commit (#7819 tier 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `revertCommit` and `rollbackToPackageCommit`'s target lookup each resolved their target commit with a strict `organization_id` equality, which matches no row whose column is NULL. An org-scoped caller therefore got COMMIT_NOT_FOUND (404) for any commit recorded env-wide — a row that demonstrably exists and that the same caller's `listCommits` hands back. Both lookups now accept org-scoped or env-wide rows, the same `$or` `deletePackage` (#7705) and `listCommits` (#7779) already carry. The `$or` was chosen over the two alternatives rather than copied. `where` is keyed on `id`, so the predicate reads like an authorization filter on a unique key; measured against the only door it is not one. Authorization is `requireManageMetadata`, checked before the call, and the `organizationId` that arrives is the session's active org selection from `resolveActiveOrganizationId` — a resolver whose body is entirely catch-wrapped and whose `undefined` omits the predicate, i.e. the widest reading. A boundary that fails open is not a boundary, which rules out "keep the check but distinguish 'not yours' from 'no such commit'". Dropping the predicate outright would newly let an org caller revert another organization's commit by id, a widening this card never asked for. The body already agreed with the `$or`: #7559 made each item resolve its scope from the row, and since #7814 `rollbackToPackageCommit` plans from `listCommits` (org + env-wide) and fed each id back into a lookup that refused half of them. The no-org branch is deliberately left un-narrowed, exactly as #7705 and #7779 left theirs. Pinned by a new real-engine/real-driver suite in packages/runtime (eight cases: the premise out of SQLite, the positive per site, both negative directions, and the no-org door per site; refusals asserted on code AND status per ADR-0112). The #7814 handoff assertion that pinned this as known-incomplete now asserts the rollback succeeds. Reverse verification, direction predicted first: 3 failed | 11 passed, exactly the three positive cases. Tier 1 only — `duplicatePackage` and `reassignOrphanedMetadata` are untouched and #7819 stays open to carry them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019hxiiv8qFCUmDThHU1k7HV --- .changeset/revert-commit-org-scope.md | 98 +++++ packages/metadata-protocol/src/protocol.ts | 62 ++- ...list-commits-org-scope.integration.test.ts | 37 +- ...evert-commit-org-scope.integration.test.ts | 353 ++++++++++++++++++ 4 files changed, 534 insertions(+), 16 deletions(-) create mode 100644 .changeset/revert-commit-org-scope.md create mode 100644 packages/runtime/src/package-revert-commit-org-scope.integration.test.ts diff --git a/.changeset/revert-commit-org-scope.md b/.changeset/revert-commit-org-scope.md new file mode 100644 index 0000000000..0a2fb932a5 --- /dev/null +++ b/.changeset/revert-commit-org-scope.md @@ -0,0 +1,98 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): let an org-scoped caller revert an env-wide commit (#7819, tier 1) + +`revertCommit` and `rollbackToPackageCommit`'s target lookup each resolved +their target commit with a strict `organization_id` equality: + +```ts +const where = { id: request.commitId }; +if (request.organizationId) where.organization_id = request.organizationId; +``` + +`organization_id = 'org'` matches no row whose column is NULL, so an org-scoped +caller got `COMMIT_NOT_FOUND` (404) for any commit recorded env-wide — a row +that demonstrably exists and that the **same caller's** `listCommits` hands +back. Both lookups now accept org-scoped **or** env-wide rows, the same `$or` +`deletePackage` (#7705) and `listCommits` (#7779) already carry. + +Env-wide commit rows are not hypothetical: `recordPackageCommit` stores +`request.organizationId ?? null`, and the publish door forwards an org only when +`resolveActiveOrganizationId` yields one — a resolver that answers `undefined` +for a session with no active organization *and* for any throw on the auth seam. +A publish made before an org was selected lands its commit env-wide, +permanently, since the timeline is append-only. + +**User-visible change.** An org-scoped rollback past an env-wide publish now +performs the rollback instead of refusing it. #7814 had already converted this +from silent to loud (pre-#7814: `{success: true, revertedCommits: []}` with the +changes still live; after it: `success: false` naming the commit), so this +closes a blocked-but-attributable operation rather than a silent data defect. + +## Why the `$or` here, and not the other two remedies + +Unlike the earlier members of this family, `where` is keyed on `id` — a +primary-key lookup — so the org predicate reads like an **authorization filter +on a unique key** rather than scan scoping, and widening it would be widening an +authorization boundary. Measured against the only door, it is not one: + +1. Authorization on `POST /packages/:id/commits/:commitId/revert` and + `POST /packages/:id/rollback` is `requireManageMetadata`, checked **before** + the protocol call. The org never gates the call. +2. The `organizationId` that arrives is the session's *active org selection* + from `resolveActiveOrganizationId`, whose body is entirely `catch`-wrapped. +3. On any auth-seam throw it answers `undefined`, which **omits** the predicate + — the widest reading, every organization's commits. A boundary that fails + **open** is not a boundary. + +That rules out remedy 3 (keep the check, distinguish "not yours" from "no such +commit"): there is no authorization here to make precise, and asserting one +would be inventing a boundary, not repairing one. Remedy 2 (drop the predicate +outright, defensible on an id lookup) was rejected because it would newly let an +org caller revert **another organization's** commit by id — a widening this card +never asked for. The `$or` admits the env-wide rows and refuses that one. + +The decisive in-code evidence is that the **body already accepted what the +lookup refused**: #7559 made `revertCommit` resolve each item's scope from the +row rather than the request, precisely because "a batch legitimately mixes an +env-wide artifact with an org overlay". `rollbackToPackageCommit` made the +contradiction self-evident — since #7814 it plans from `listCommits` (org + +env-wide) and fed each id straight back into a lookup that refused half of them. + +The **no-org branch is deliberately not narrowed** to `organization_id IS NULL`, +exactly as #7705 and #7779 left theirs: the direct-mount REST registrar passes +no `organizationId` at all, and restricting that door to env-wide rows would +make every org-scoped commit unrevertable — the same bug pointed the other way. + +## Pin + +`packages/runtime/src/package-revert-commit-org-scope.integration.test.ts` — a +real `ObjectQL` over a real `SqlDriver` on better-sqlite3, seeded through the +real publish path, because the question is whether `organization_id = 'org'` +matches a NULL column: a property of the driver's SQL, not of a stub's +`filter()`. (It lives in `packages/runtime` because `metadata-protocol` cannot +import `objectql` — dependency cycle.) Eight cases: the premise measured out of +SQLite, the positive for each site, **both** negative directions (another +organization's commit refused on each site; another package's commits not +reached by the planner), and the no-org door on each site. Refusals are asserted +on `code` **and** `status` per ADR-0112, never on "it threw". + +`package-list-commits-org-scope.integration.test.ts` (#7814) carried the handoff +assertion that pinned this defect as known-incomplete +(`rollback.success === false`, `failed == [c2]`); it now asserts the rollback +succeeds and reverts `c2`, and survives as the family's end-to-end case. + +**Reverse verification**, direction predicted before running: restoring the +strict equality turns exactly the two positive cases red plus the updated +handoff assertion, and leaves both negative directions and both no-org doors +green, since strict equality is *narrower* than the `$or`. Measured: 3 failed | +11 passed, exactly those three. + +## Scope + +Tier 1 of #7819 only. The two remaining strict equalities in this file — +`duplicatePackage` and `reassignOrphanedMetadata`, a different table +(`sys_metadata`) whose step one is the unanswered "are these states even +reachable" — are deliberately untouched, and #7819 stays open to carry them. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7003ceb994..febdd09749 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -12212,7 +12212,52 @@ export class ObjectStackProtocolImplementation implements await this.ensureOverlayIndex(); const orgId = request.organizationId ?? null; const where: Record = { id: request.commitId }; - if (request.organizationId) where.organization_id = request.organizationId; + // [#7819] Resolve BOTH org-scoped and env-wide (`organization_id IS + // NULL`) commit rows for an org-scoped caller — the same defect and + // remedy as the sibling {@link listCommits} (#7779) and {@link + // deletePackage} (#7705). `organization_id = ` matches no NULL + // column, so this answered `COMMIT_NOT_FOUND` (404) for a row that + // demonstrably exists and that the SAME caller's `listCommits` + // returns. + // + // ⚠️ This site is NOT the family's plain scan-scoping, and the `$or` + // was chosen over the two alternatives rather than copied. `where` is + // keyed on `id`, so the predicate reads like an AUTHORIZATION filter + // layered on a unique key. Measured against the only door, it is not + // one: authorization on `POST /packages/:id/commits/:commitId/revert` + // is `requireManageMetadata`, checked before this call; the + // `organizationId` that arrives is the session's *active org + // selection* from `resolveActiveOrganizationId`, whose body is + // entirely `catch`-wrapped and answers `undefined` on any auth-seam + // throw — and `undefined` omits this predicate, which is the WIDEST + // reading (every organization's commits). A boundary that fails OPEN + // is not a boundary, so there is no authz here to make precise; that + // rules out "keep it but distinguish 'not yours' from 'no such + // commit'". Dropping the predicate outright is defensible on an id + // lookup, but it would newly let an org caller revert ANOTHER + // organization's commit by id — a widening this card never asked for. + // The `$or` admits the env-wide rows and refuses that one. + // + // The body already agreed with this reading before the lookup did: + // #7559 made each item resolve its scope FROM THE ROW ({@link + // resolveMetaItemOrgScope}) precisely because "a batch legitimately + // mixes an env-wide artifact with an org overlay", so the loop below + // processes env-wide items for an org caller while the lookup above + // refused to hand them over. {@link rollbackToPackageCommit} made the + // contradiction self-evident: since #7814 it plans from `listCommits` + // (org + env-wide) and fed each id straight back into this lookup. + // + // The no-org branch is deliberately NOT narrowed to `organization_id + // IS NULL`, exactly as #7705 and #7779 left theirs: the direct-mount + // REST registrar passes no `organizationId` at all, and restricting + // that door to env-wide rows would make every org-scoped commit + // unrevertable — the same bug pointed the other way. + if (request.organizationId) { + where.$or = [ + { organization_id: request.organizationId }, + { organization_id: null }, + ]; + } const row = (await this.engine.findOne('sys_metadata_commit', { where })) as any; if (!row) { const err: any = new Error(`[commit_not_found] No commit '${request.commitId}'.`); @@ -12487,7 +12532,20 @@ export class ObjectStackProtocolImplementation implements failed: Array<{ commitId: string; error: string }>; }> { const where: Record = { id: request.commitId }; - if (request.organizationId) where.organization_id = request.organizationId; + // [#7819] Same widening as the {@link revertCommit} lookup above, and + // for the sharper reason: this function PLANS from {@link listCommits}, + // which since #7814 returns org-scoped and env-wide rows alike to an + // org caller. With the strict equality here, an org-scoped rollback + // whose TARGET happened to be recorded env-wide answered 404 before it + // planned anything at all — for a commit the caller's own timeline had + // just listed. The rationale for the `$or` over the alternatives, and + // for leaving the no-org branch un-narrowed, is stated in full there. + if (request.organizationId) { + where.$or = [ + { organization_id: request.organizationId }, + { organization_id: null }, + ]; + } const target = (await this.engine.findOne('sys_metadata_commit', { where })) as any; if (!target) { const err: any = new Error(`[commit_not_found] No commit '${request.commitId}'.`); 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 index e30ae7789d..cd40861e3b 100644 --- a/packages/runtime/src/package-list-commits-org-scope.integration.test.ts +++ b/packages/runtime/src/package-list-commits-org-scope.integration.test.ts @@ -303,21 +303,30 @@ describe('#7779 — org-scoped listCommits must not hide env-wide commit rows', 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. + // [#7819 tier 1] ⭐ THE GAP THIS SUITE HANDED ON IS NOW CLOSED — these + // lines are the handoff, and they changed exactly as it predicted. // - // `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. + // What they asserted until #7819: `revertCommit` (its own `findOne`) and + // `rollbackToPackageCommit` (its target lookup) still carried the + // byte-identical strict equality, because `protocol.ts` is serialized and + // #7779 held it for `listCommits` alone. So the planner SAW C2 and asked + // `revertCommit` to undo it, and that lookup could not resolve an env-wide + // row — the rollback answered `success: false` naming C2. Already strictly + // better than the silent `success: true` of before #7814 (loud, + // attributable, non-destructive), but still a legitimate operation + // blocked; the assertion existed so the remaining half could not drift + // unnoticed before its own card landed. + // + // #7819 tier 1 widened both lookups to the same `$or` this suite pinned + // for `listCommits`, so C2 now resolves and is actually undone. The case + // survives as the family's END-TO-END pin: the planner sees the env-wide + // commit (asserted above) AND can now act on it — the only combination + // under which an org-scoped rollback past an env-wide publish does what it + // reports. Its own negative directions live in the sibling + // `package-revert-commit-org-scope.integration.test.ts`. 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]); + expect(rollback.failed).toEqual([]); + expect(rollback.success).toBe(true); + expect(rollback.revertedCommits).toEqual([c2]); }); }); diff --git a/packages/runtime/src/package-revert-commit-org-scope.integration.test.ts b/packages/runtime/src/package-revert-commit-org-scope.integration.test.ts new file mode 100644 index 0000000000..bc33752883 --- /dev/null +++ b/packages/runtime/src/package-revert-commit-org-scope.integration.test.ts @@ -0,0 +1,353 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Real-engine regression for #7819 tier 1 — `protocol.revertCommit` and +// `protocol.rollbackToPackageCommit`'s target lookup each resolved their +// target commit with a strict `organization_id` equality, so an org-scoped +// caller got `COMMIT_NOT_FOUND` (404) for a commit row that demonstrably +// exists and that the very same caller's `listCommits` hands back. + +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 the remedy here is NOT self-evidently the family's. + * + * Both sites carried the predicate the rest of this family carried: + * + * const where = { id: request.commitId }; + * if (request.organizationId) where.organization_id = request.organizationId; + * + * `organization_id = 'org'` matches no row whose column is NULL, so an + * org-scoped caller could not resolve any commit recorded env-wide. + * + * --------------------------------------------------------------------------- + * Why this site needed a decision the earlier cards did not face + * --------------------------------------------------------------------------- + * `where` is keyed on `id` — a primary-key lookup. The org predicate is not + * scoping a scan the way {@link listCommits} (#7779) or {@link deletePackage} + * (#7705) were; it reads like an authorization filter layered on a unique key. + * If it WERE one, widening it would be widening an authorization boundary, and + * "consistent with the family" would be the wrong reason to do it. + * + * Measured against the only door, it is not one: + * + * 1. Authorization on both routes is `requireManageMetadata(deps, _context)` + * in `packages/runtime/src/domains/packages.ts`, checked BEFORE the + * protocol is called. The org never gates the call. + * 2. The `organizationId` those routes pass comes from + * `resolveActiveOrganizationId` (`packages/runtime/src/http-dispatcher.ts`), + * which reads the session's `activeOrganizationId` — a "which org am I + * looking at" selection — and whose whole body is `catch`-wrapped so ANY + * throw on the auth seam answers `undefined`. + * 3. `undefined` omits the predicate entirely, which is the WIDEST reading: + * every organization's commits. A boundary that fails OPEN is not a + * boundary. That single fact rules out reading these lines as authz, and + * with it rules out remedy 3 (keep the check, distinguish "not yours" + * from "no such commit") — there is no authz here to make precise. + * + * So the choice was between mirroring the family's `$or` and dropping the + * predicate outright. The `$or` is what landed, because dropping it would let + * an org-scoped caller revert ANOTHER organization's commit by id — a genuine + * widening this card never asked for — while the `$or` refuses exactly that + * (pinned below) and admits only the env-wide rows. + * + * --------------------------------------------------------------------------- + * The decisive in-code evidence: the body already accepts what the lookup refused + * --------------------------------------------------------------------------- + * #7559 changed `revertCommit` to resolve each item's scope FROM THE ROW rather + * than from the request ({@link resolveMetaItemOrgScope}), with the in-code + * rationale that "a batch legitimately mixes an env-wide artifact with an org + * overlay". Verified here rather than taken on faith: that helper answers + * `null` — env scope — for an item whose history is env-wide, even when the + * request carries an org. The design therefore already accepts an org caller + * operating on env-wide artifacts; a target lookup that refuses the same row + * contradicted the body that would have processed it. + * + * `rollbackToPackageCommit` closes the argument. It derives its work list from + * `listCommits`, which since #7814 returns org-scoped AND env-wide rows to an + * org caller — then fed each id straight back into a lookup that refused half + * of them. One function contradicted itself inside a single call. + * + * --------------------------------------------------------------------------- + * Why a REAL engine and a REAL driver + * --------------------------------------------------------------------------- + * The question is whether `organization_id = 'org'` matches a NULL column, a + * property of the driver's SQL rather than of a stub's `filter()`. Both + * `deletePackage` suites and the ADR-0067 commit-history suites stub + * `engine.find`, which is precisely why none of them could see any member of + * this family. This file seeds through the REAL publish path, exactly as its + * sibling `package-list-commits-org-scope.integration.test.ts` (#7814) does, + * and for the same reason it lives in `packages/runtime`: `metadata-protocol` + * cannot import `objectql` (dependency cycle). + * + * --------------------------------------------------------------------------- + * Measured BEFORE the fix, on this branch's base + * --------------------------------------------------------------------------- + * Every positive case below was run against the unfixed protocol first: + * `revertCommit` on the env-wide commit threw `COMMIT_NOT_FOUND` / 404, and + * `rollbackToPackageCommit` answered `{success: false, failed: [c2]}` — the + * state the sibling suite pinned as KNOWN-INCOMPLETE and handed to this card. + * Reverse verification (restoring the strict equality after the fix) was + * predicted to turn exactly the positive cases red and leave both negative + * directions and the no-org door green, since strict equality is NARROWER than + * the `$or`; measured on revert: exactly that. Numbers in the changeset. + */ + +const PKG = 'com.repro.revert'; +const OTHER_PKG = 'com.other.revert'; +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-7819-')); + 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` holds the rows under test; the other three are what + // the real publish path writes through on its way to recording a commit, + // and what the revert then reads back and rewrites. + 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. + // Registered under the PLATFORM package, never under `PKG`, so the tables + // cannot be torn out from under the assertions that read them 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) => ({ + name, + label: name, + 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` 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; +} + +/** Distinct `created_at` values — the timeline is sorted by that ISO string. */ +const tick = () => new Promise((r) => setTimeout(r, 5)); + +/** + * ADR-0112 refusal shape. A bare `toThrow()` goes green against an + * implementation that throws anything at all, including a bare `Error`, so + * every refusal below is asserted on `code` AND `status` — the pair the + * dispatcher's `errorFromThrown` turns into the HTTP answer. + */ +async function expectRefusal(run: () => Promise, code: string, status: number) { + const err = await run().then( + () => { throw new Error(`expected ${code} (${status}), but the call resolved`); }, + (e: any) => e, + ); + expect({ code: err?.code, status: err?.status }).toEqual({ code, status }); +} + +describe('#7819 tier 1 — an org-scoped caller must be able to revert an env-wide commit', () => { + 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: 'revert_env', packageId: PKG, message: 'env-wide publish', + }); + + // Straight out of SQLite: the column really is NULL, so the strict + // equality this card removes really had 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('revertCommit resolves an env-wide commit for an org caller (was: COMMIT_NOT_FOUND 404)', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const envWide = await publishOne(p, { + view: 'revert_env', packageId: PKG, message: 'env-wide publish', + }); + + const result = await p.revertCommit({ commitId: envWide, organizationId: ACTIVE_ORG }); + + // The CONSEQUENCE, not the call: the artifact the env-wide commit created + // is gone. Before the fix this line was never reached — the lookup threw + // 404 for a row the caller's own `listCommits` returns. + expect(result.success).toBe(true); + expect(result.reverted).toEqual([ + { type: 'view', name: 'revert_env', action: 'removed' }, + ]); + expect(result.failed).toEqual([]); + }); + + it('revertCommit still REFUSES another organization’s commit', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const foreign = await publishOne(p, { + view: 'revert_foreign', packageId: PKG, organizationId: OTHER_ORG, message: 'other-org publish', + }); + + // The direction that must not widen. Dropping the predicate outright — + // remedy 2, defensible on an id lookup — would have made this resolve, so + // this case is what chose the `$or` over it. + await expectRefusal( + () => p.revertCommit({ commitId: foreign, organizationId: ACTIVE_ORG }), + 'COMMIT_NOT_FOUND', + 404, + ); + }); + + it('a caller with NO org can still revert an org-scoped commit (the other door)', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const own = await publishOne(p, { + view: 'revert_own', packageId: PKG, organizationId: ACTIVE_ORG, message: 'own-org publish', + }); + + // The no-org branch is deliberately left un-narrowed, exactly as #7705 and + // #7779 left theirs. Narrowing it to `organization_id IS NULL` would hide + // every org-scoped commit from this door — the same bug pointed the other + // way. The direct-mount REST registrar passes no `organizationId` at all. + const result = await p.revertCommit({ commitId: own }); + expect(result.success).toBe(true); + expect(result.reverted.map((r: any) => r.name)).toEqual(['revert_own']); + }); + + it('rollbackToPackageCommit resolves an env-wide TARGET for an org caller', async () => { + const { protocol } = await boot(); + const p = protocol as any; + // The target itself env-wide, with a strictly newer org-scoped commit to + // undo — the mirror image of the handoff case, exercising the second of + // the two sites in isolation. Pre-fix the TARGET lookup threw 404 before + // any planning happened. + const envTarget = await publishOne(p, { + view: 'roll_env_target', packageId: PKG, message: 'env-wide target', + }); + await tick(); + await publishOne(p, { + view: 'roll_newer', packageId: PKG, organizationId: ACTIVE_ORG, message: 'newer', + }); + + const rollback = await p.rollbackToPackageCommit({ + commitId: envTarget, organizationId: ACTIVE_ORG, + }); + + expect(rollback.failed).toEqual([]); + expect(rollback.success).toBe(true); + expect(rollback.revertedCommits).toHaveLength(1); + }); + + it('rollbackToPackageCommit still REFUSES another organization’s target commit', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const foreign = await publishOne(p, { + view: 'roll_foreign', packageId: PKG, organizationId: OTHER_ORG, message: 'other-org target', + }); + + await expectRefusal( + () => p.rollbackToPackageCommit({ commitId: foreign, organizationId: ACTIVE_ORG }), + 'COMMIT_NOT_FOUND', + 404, + ); + }); + + it('rollbackToPackageCommit does NOT reach into another package’s commits', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const target = await publishOne(p, { + view: 'roll_target', packageId: PKG, organizationId: ACTIVE_ORG, message: 'target', + }); + await tick(); + // Strictly NEWER than the target and env-wide, so it clears both filters + // the planner applies except the package one. If widening the target + // lookup had leaked package scope, this commit would be reverted too. + const otherPkg = await publishOne(p, { + view: 'roll_other_pkg', packageId: OTHER_PKG, message: 'other package, newer', + }); + + const rollback = await p.rollbackToPackageCommit({ + commitId: target, organizationId: ACTIVE_ORG, + }); + + expect(rollback.success).toBe(true); + expect(rollback.revertedCommits).toEqual([]); + expect(rollback.revertedCommits).not.toContain(otherPkg); + }); + + it('a caller with NO org can still roll back to an org-scoped target (the other door)', async () => { + const { protocol } = await boot(); + const p = protocol as any; + const target = await publishOne(p, { + view: 'roll_own_target', packageId: PKG, organizationId: ACTIVE_ORG, message: 'own-org target', + }); + await tick(); + await publishOne(p, { view: 'roll_after', packageId: PKG, message: 'after' }); + + const rollback = await p.rollbackToPackageCommit({ commitId: target }); + + expect(rollback.failed).toEqual([]); + expect(rollback.success).toBe(true); + expect(rollback.revertedCommits).toHaveLength(1); + }); +}); From c21f09e1558a51effc559c7d1a70af175b0fb19e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 21:52:03 +0000 Subject: [PATCH 2/2] test(objectql): teach the commit-history double `$or`, conjoined (#7819 tier 1, part of #7620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI red on PR #7857: `packages/objectql/src/protocol-commit-history.test.ts` had two org-scoped revert cases fail with COMMIT_NOT_FOUND. Measured rather than assumed: its `matchesWhere` was pure flat equality, so the widened lookup `{ id, $or: [{organization_id: }, {organization_id: null}] }` compared `row['$or']` against the array and matched nothing. The double is the blind party, not the fix. Both failing rows carry the CALLER'S OWN org (`organization_id: 'org_a'`, request org `'org_a'`), so they match the FIRST `$or` branch outright — the same row the strict equality already accepted. No real behaviour changed, and neither case's subject (#6602's registry org-asymmetry) involves the commit lookup at all; it is merely the door they enter through. Conjoined with the sibling keys in the entries loop, matching the corrected form #7846 landed across six doubles in this package an hour earlier. Not the early-returning `if ($or) return …some(…)` shape those six carried before it: that discards sibling keys, so `{ id, $or: [...] }` would stop constraining `id` and could return some other commit whose org matched. This file was not among #7846's six because it had no operator handling to correct, so it is a new member of the #7620 lane rather than a regression of it. `undefined` normalises to `null` on comparison, same as the six, because a column a row never set reads as NULL out of a real driver. @objectstack/objectql: 185 files / 3274 tests passing (was 184/3272 with the two failures) — exactly the two cases restored, nothing else moved. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019hxiiv8qFCUmDThHU1k7HV --- .changeset/revert-commit-org-scope.md | 28 ++++++++++ .../src/protocol-commit-history.test.ts | 52 ++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/.changeset/revert-commit-org-scope.md b/.changeset/revert-commit-org-scope.md index 0a2fb932a5..2b8cd8771c 100644 --- a/.changeset/revert-commit-org-scope.md +++ b/.changeset/revert-commit-org-scope.md @@ -90,6 +90,34 @@ handoff assertion, and leaves both negative directions and both no-org doors green, since strict equality is *narrower* than the `$or`. Measured: 3 failed | 11 passed, exactly those three. +## A blind test double, taught rather than accommodated + +`packages/objectql/src/protocol-commit-history.test.ts` went red on two +org-scoped revert cases. Measured, not assumed: its `matchesWhere` was pure flat +equality, so it compared `row['$or']` against the array and matched nothing. + +The double was the blind party, not the fix — both failing rows carry the +**caller's own** org (`organization_id: 'org_a'`, request org `'org_a'`), so +they match the first `$or` branch outright: the same row the strict equality +already accepted. Neither case's subject (#6602's registry org-asymmetry) +involves the commit lookup at all; it is merely the door they enter through. + +It now understands `$or`/`$and`, **conjoined with the sibling keys in the +entries loop** — the corrected form #7846 landed across six doubles in this +package (part of #7620), not the early-returning `if ($or) return …some(…)` +shape those six carried before it. That shape discards sibling keys, so +`{ id, $or: [...] }` would stop constraining `id` and the lookup could return +some *other* commit whose org matched. This file was not among #7846's six +because it had no operator handling to correct, so it reads as a new member of +the #7620 lane rather than a regression of it. + +⚠️ Recorded deliberately: this makes the double a *reimplementation* of `$or`, +so any assertion whose **subject** is the org predicate would be measuring the +double rather than the protocol. No case in that file has that subject — which +is exactly why it could never see this family — and a comment there says so and +asks that org-scoping cases not be added. The operator's real behaviour against +a real driver stays pinned on the real engine in `packages/runtime`. + ## Scope Tier 1 of #7819 only. The two remaining strict equalities in this file — diff --git a/packages/objectql/src/protocol-commit-history.test.ts b/packages/objectql/src/protocol-commit-history.test.ts index 5dae11dc78..ba16dd8a7b 100644 --- a/packages/objectql/src/protocol-commit-history.test.ts +++ b/packages/objectql/src/protocol-commit-history.test.ts @@ -247,10 +247,60 @@ describe('ADR-0067 — publishPackageDrafts records a commit', () => { /** A Studio authoring workspace id — writable under ADR-0070. */ const APP_PKG = 'app.myapp'; +/** + * [#7819] `$or` / `$and` are understood, because a double that silently drops + * an operator does not answer "no match" — it answers a WRONG match. + * + * `revertCommit` and `rollbackToPackageCommit` resolve their target commit with + * `{ id, $or: [{ organization_id: }, { organization_id: null }] }` so an + * org-scoped caller can reach a commit recorded env-wide. This helper was flat + * equality, so it compared `row['$or']` against the array and every lookup + * missed — including the two org-scoped cases below, whose seeded rows carry + * the caller's OWN org and therefore match the FIRST branch outright. Nothing + * about their subject (#6602's registry org-asymmetry) changed; the double just + * could not evaluate the predicate that now guards the door they enter through. + * + * ⚠️ CONJOINED with the sibling keys, in the entries loop — the corrected form + * #7846 just landed across six doubles in this package (part of #7620), and + * deliberately NOT the early-returning `if ($or) return …some(…)` shape those + * six carried before it. That shape discards every sibling equality key, so + * `{ id, $or: [...] }` would stop constraining `id` at all and this lookup + * would return SOME OTHER commit whose org happened to match. With one seeded + * commit per harness that is invisible today — which is exactly what makes it + * worth ruling out here rather than discovering later. + * + * This file was not among #7846's six because it had no operator handling at + * all to correct (pure flat equality), so it reads as a new member of the same + * #7620 lane rather than a regression of it. + * + * ⚠️ What this helper does NOT do is pin `$or` semantics — it is a + * reimplementation of them, so any assertion whose SUBJECT is the org predicate + * would be measuring this function rather than the protocol. No case in this + * file has that subject (which is precisely why this file could never see the + * #7705/#7779/#7819 family), and the operator's real behaviour against a real + * driver — whether `organization_id = 'org'` matches a NULL column — is pinned + * on a real engine in `packages/runtime/src/package-revert-commit-org-scope. + * integration.test.ts`. Keep it that way: do not add org-scoping cases here. + */ const matchesWhere = (r: Record, w: Record): boolean => { + if (!w || typeof w !== 'object') return true; for (const [k, v] of Object.entries(w)) { + if (k === '$and' && Array.isArray(v)) { + if (!v.every((s: any) => matchesWhere(r, s))) return false; + continue; + } + if (k === '$or' && Array.isArray(v)) { + if (!v.some((s: any) => matchesWhere(r, s))) return false; + continue; + } + if (k.startsWith('$')) continue; if (v === undefined) continue; - if (r[k] !== v) return false; + // A column a row never set reads as NULL out of a real driver, so an + // absent field must satisfy `{ organization_id: null }` — the env-wide + // branch of the `$or`. Comparing `undefined !== null` would make this + // double refuse rows SQLite returns. + const actual = r[k] === undefined ? null : r[k]; + if (actual !== v) return false; } return true; };