Skip to content

Commit 0ee1ec2

Browse files
committed
fix(metadata-protocol): listCommits no longer hides env-wide commit history (#7779)
`protocol.listCommits` selected the ADR-0067 timeline with the strict `organization_id` equality that #7705 (PR #7771) had just replaced one function above it. `organization_id = '<org>'` matches no row whose column is NULL, so a session with an active organization was shown none of the commits recorded env-wide. Live, not latent, and measured before the fix was written — which the card set as step one. `recordPackageCommit` stores `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` both for a session with no active organization and for ANY throw on the auth seam, since its whole body is `catch`-wrapped. So a publish made before an org is selected, or during a transient auth blip, records its commit env-wide permanently — 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 the audit/observability one the card projected, and that is a finding rather than a detail: `rollbackToPackageCommit` derives the set of commits it must undo FROM THIS LIST. A commit the list could not see was silently never reverted — measured pre-fix, an org-scoped rollback past an env-wide commit answered `{success: true, revertedCommits: []}` with that commit's changes still 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` shape this package already uses for #3115, the shape #7705 applied to the sibling `deletePackage` read, and the shape the SQL driver's own tenant wall uses (#2734). Both directions that must not widen are pinned: another organization's commits stay invisible, another package's are never returned, and newest-first ordering is unchanged. The no-org branch is deliberately left package-wide rather than narrowed to `organization_id IS NULL` — narrowing would hide every org-scoped commit from that door instead, re-creating the bug pointed the other way, which is why #7705 left its own no-org branch alone. The whole shape, or none of it. The pin uses a real engine and a real driver and asserts the CONSEQUENCE. Both existing `deletePackage` suites stubbed `engine.find`, which is why neither could see the sibling defect; the question here is whether `organization_id = 'org'` matches a NULL column, which is a property of the driver's SQL and not of a stub's `filter()`. It seeds through the real publish path and reads back what landed in SQLite. Reverse-verified with the direction predicted first: restoring the strict equality was predicted to turn exactly the two positive cases red and leave all four others green, because strict equality is NARROWER than the `$or` — it cannot reach another org's rows or another package's, and does not touch the no-org branch. Measured on revert: exactly that, 2 failed / 4 passed, both failures `[1]` vs `[2]`. 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 rather than silent: the rollback above reports `success: false` naming the commit it could not resolve, instead of claiming success over a no-op. Strictly better and non-destructive, but not the whole repair, so the new suite asserts it and the remainder cannot drift unnoticed. Ratchets unchanged: runtime's TEST_DEBT measured exactly 227, its recorded ceiling, with zero errors attributable to the new file; the query-options-erasure ratchet holds at 67 non-test sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp1rUCEFGp3eYRztsRx1B1
1 parent 098b629 commit 0ee1ec2

3 files changed

Lines changed: 422 additions & 1 deletion

File tree

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
fix(metadata-protocol): `listCommits` no longer hides a package's env-wide commit history (#7779)
6+
7+
`protocol.listCommits` selected the ADR-0067 timeline with the same strict
8+
`organization_id` equality that #7705 (PR #7771) had just replaced one function
9+
above it:
10+
11+
```ts
12+
const where = { package_id: request.packageId };
13+
if (request.organizationId) where.organization_id = request.organizationId;
14+
```
15+
16+
`organization_id = '<org>'` matches no row whose column is NULL, so a session
17+
with an active organization was shown **none** of the commits recorded
18+
env-wide. The commits were in `sys_metadata_commit` the whole time; the read
19+
could not see them.
20+
21+
**Env-wide commit rows are actually written — this was live, not latent, and
22+
that was measured before the fix was written.** `recordPackageCommit` stores
23+
`organization_id: request.organizationId ?? null`, and the only door into a
24+
publish (the dispatcher's `POST /packages/:id/publish-drafts`) forwards an
25+
organization only when `resolveActiveOrganizationId` yields one. That resolver
26+
answers `undefined` both for a session with no active organization and for *any*
27+
throw on the auth seam, since its whole body is `catch`-wrapped. A publish made
28+
before an organization is selected — or during a transient auth failure — is
29+
therefore recorded env-wide permanently, because the timeline is append-only.
30+
Driven on a real engine over SQLite, a no-org publish wrote
31+
`organization_id: null` and the org-scoped read of that same package then
32+
returned `[]`.
33+
34+
**The blast radius is wider than audit.** `rollbackToPackageCommit` derives the
35+
set of commits it must undo *from this list*, so a commit the list could not see
36+
was a commit the rollback silently skipped: measured before the fix, an
37+
org-scoped rollback past an env-wide commit answered
38+
`{success: true, revertedCommits: []}` while that commit's changes stayed live.
39+
A rollback that reports success and rolls back nothing is a correctness defect,
40+
not a reporting one.
41+
42+
An org-scoped read now matches its own organization **or** env-wide — the
43+
`$or [{organization_id: oid}, {organization_id: null}]` shape this package
44+
already uses for the #3115 "orphaned draft" fix, the shape #7705 applied to the
45+
sibling `deletePackage` read, and the shape the SQL driver's own implicit tenant
46+
wall uses (`field = :tenant OR field IS NULL`, #2734).
47+
48+
Both directions that must not widen are pinned: another organization's commits
49+
stay invisible to an org-scoped read, and another package's commits are never
50+
returned. Newest-first ordering is unchanged. The **no-org** branch is
51+
deliberately left package-wide rather than narrowed to `organization_id IS
52+
NULL` — narrowing it would hide every org-scoped commit from that door instead,
53+
re-creating this bug pointed the other way, which is exactly why #7705 left its
54+
own no-org branch alone.
55+
56+
**Known remaining gap, reported on #7779 rather than fixed here** (this card
57+
holds `protocol.ts`, a serialized file, for `listCommits` alone): `revertCommit`
58+
and `rollbackToPackageCommit`'s own target lookups still carry the identical
59+
strict equality. The consequence is now *loud* instead of silent — the rollback
60+
above reports `success: false` naming the commit it could not resolve, rather
61+
than claiming success over a no-op. That is strictly better and non-destructive,
62+
but it is not the whole repair, and the new suite asserts it so the remainder
63+
cannot drift unnoticed before its own card lands.

packages/metadata-protocol/src/protocol.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12114,7 +12114,42 @@ export class ObjectStackProtocolImplementation implements
1211412114
}>> {
1211512115
try {
1211612116
const where: Record<string, unknown> = { package_id: request.packageId };
12117-
if (request.organizationId) where.organization_id = request.organizationId;
12117+
// [#7779] Surface BOTH org-scoped and env-wide (`organization_id IS
12118+
// NULL`) commit rows to an org-scoped caller — the same defect and
12119+
// the same remedy as the sibling {@link deletePackage} read one
12120+
// function above (#7705), and as {@link
12121+
// SysMetadataRepository.listDrafts} (#3115) in this package.
12122+
//
12123+
// Env-wide commit rows are not hypothetical: {@link
12124+
// recordPackageCommit} stores `organization_id: request.
12125+
// organizationId ?? null`, and the ONLY door into a publish — the
12126+
// dispatcher's `POST /packages/:id/publish-drafts` — forwards an
12127+
// org only when `resolveActiveOrganizationId` yields one. That
12128+
// resolver answers `undefined` for a session with no active
12129+
// organization AND for every failure on the auth seam (it is
12130+
// `catch`-wrapped). So a publish made before an org was selected —
12131+
// or during a transient auth blip — lands its commit env-wide,
12132+
// permanently, and the strict equality then hid it from every
12133+
// org-scoped read of that package's timeline.
12134+
//
12135+
// This is NOT merely an observability miss. {@link
12136+
// rollbackToPackageCommit} derives the set of commits to undo from
12137+
// this list, so an invisible commit was silently never reverted:
12138+
// measured pre-fix, a rollback past an env-wide commit answered
12139+
// `{success: true, revertedCommits: []}` while that commit's
12140+
// changes stayed live.
12141+
//
12142+
// The no-org branch is deliberately NOT narrowed to
12143+
// `organization_id IS NULL`, exactly as #7705 left its own: a
12144+
// caller with no active org reads the package's whole timeline,
12145+
// and restricting it to env-wide rows would hide every org-scoped
12146+
// commit instead — the same bug pointed the other way.
12147+
if (request.organizationId) {
12148+
where.$or = [
12149+
{ organization_id: request.organizationId },
12150+
{ organization_id: null },
12151+
];
12152+
}
1211812153
const rows = (await this.engine.find('sys_metadata_commit', {
1211912154
where,
1212012155
...(request.limit ? { limit: request.limit } : {}),

0 commit comments

Comments
 (0)