fix(metadata-protocol): let an org-scoped caller revert an env-wide commit (#7819 tier 1)

.changeset/revert-commit-org-scope.md

@@ -0,0 +1,126 @@
+---
+"@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.
+
+## 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 —
+`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.

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<string, unknown> = { 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 = <org>` 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<string, unknown> = { 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}'.`);

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: <org> }, { 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<string, unknown>, w: Record<string, unknown>): 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;
 };

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]);
  });
 });