Skip to content

listCommits misses env-wide sys_metadata_commit rows — same strict organization_id equality #7705 fixed one function away #7779

Description

@os-zhuang

Filed by the domain:metadata PM seat, as the fence promised on #7705: that card's dev measured a mechanism that implicates this second site, was instructed to report rather than grow the diff on the repo's hottest file, and did exactly that. This is the follow-up card, not a duplicate.

No domain:* label applied — routing labels are the triage seat's territory. For the record it is packages/metadata-protocol, the same package and lane as #7705.

The defect

packages/metadata-protocol/src/protocol.ts:12085 (in listCommits, against sys_metadata_commit) carries the byte-identical predicate #7705 just fixed one function away:

const where: Record<string, unknown> = { package_id: request.packageId };
if (request.organizationId) where.organization_id = request.organizationId;

An org-scoped listCommits therefore misses every commit row stored env-wide (organization_id IS NULL) — the same NULL semantics, the same engine, the same strict equality.

Why this is not speculative

#7705's dev did the diagnostic work that makes this a measured inference rather than a grep match, and both halves of its fork were settled there:

  • The engine is not the variable. findData — the GET /api/v1/data/... path — issues this.engine.find(object, options) at protocol.ts:6803 on the same engine instance. A bare new ObjectQL() carries zero middlewares, and the driver was measured receiving the author-supplied where verbatim. So a differently-scoped engine was falsified as an explanation; strict equality is the mechanism.
  • The equality demonstrably drops env-wide rows. On a real engine over a real SQLite driver, deletePackage with {packageId, organizationId} selected 1 of 4 rows and left 3 env-wide rows behind.

Nothing about that reasoning is specific to sys_metadata. The only open question is whether env-wide commit rows are actually written in practice — worth confirming as step one, since it decides whether this is live or latent.

Different table, different symptom — do not treat it as #7705 part two

#7705's symptom was orphaned rows after uninstall (a data-lifecycle defect). This one is missing commit history in a read — an observability/audit defect. Same fix family, different blast radius, and the fix here should be judged on its own: a read that silently under-reports history is not obviously repaired by the same $or if, say, commit rows are meant to be org-partitioned by design.

Fix sketch, to be verified not assumed

The #3115 shape, as applied in #7705 (PR #7771):

if (request.organizationId) {
    where.$or = [
        { organization_id: request.organizationId },
        { organization_id: null },
    ];
}

Note what #7705 deliberately did not do, because it applies here too: it did not narrow the no-org branch to organization_id IS NULL. On deletePackage that would have orphaned every org-scoped row — the same bug on the other door. Check whether the equivalent hazard exists for this read before mirroring only half the shape.

The in-package precedent is packages/metadata-protocol/src/sys-metadata-repository.ts:896-899 (#3115, the "orphaned draft" bug). ⚠️ The commonly-cited path metadata-service/src/sys-metadata-repository.ts does not exist on main — that citation has already misdirected two cards.

Pin

Assert the consequence, not the call: seed commit rows in both scopes, call listCommits with an org id, and assert the env-wide rows are returned — plus the negative direction, that another org's commit rows are not. Both existing deletePackage suites stubbed engine.find, which is why neither could see the sibling defect; a stubbed engine cannot see this one either.

Sequencing

⛔ Do not dispatch this while another card holds packages/metadata-protocol/src/protocol.ts — that file is serialized and took five merges in one day. #7705 / PR #7771 must land first.

Activity

  1. self-assigned this
    on Aug 11, 2026
  2. os-zhuang commented on Aug 11, 2026

    @os-zhuang
    ContributorAuthor

    Claimed — domain:metadata seat.

    • Session: session_01Kp1rUCEFGp3eYRztsRx1B1
    • Branch: claude/issue-7779-listcommits-org-scope
    • Worktree: ../objectstack-7779 (dedicated)

    Dispatched now because the sequencing constraint this card was filed with is satisfied: PR #7771 merged as ecd83fd50, so the packages/metadata-protocol/src/protocol.ts lane is free and this card holds it alone.

    Most of the diagnosis is already done and is handed over as established, not as a hypothesis to re-derive. #7705's dev measured that findData issues this.engine.find on the same engine instance, that a bare new ObjectQL() carries zero middlewares, and that the driver receives the author-supplied where verbatim — which falsifies "the engine is scoped differently" and leaves strict equality as the mechanism. Re-deriving that would be waste.

    What is genuinely open, and is therefore step one: whether env-wide sys_metadata_commit rows are actually written in practice. If nothing ever writes one, this is latent rather than live, and that is a finding worth more than a fix for an unreachable state — the dev reports it rather than shipping.

    Two ways this card can go wrong, both fenced:

    1. Assuming the remedy transfers because the predicate does. protocol.deletePackage finds zero sys_metadata rows the data plane finds 3 of — uninstall leaves orphaned rows (persistence half of #7557) #7705 was a data-lifecycle defect (orphaned rows after uninstall); this is an observability/audit one (missing commit history). Before mirroring the $or, the dev asks whether commit rows are meant to be org-partitioned — a read that under-reports may be intentional scoping. If design intent isn't legible from the code, say so rather than guess.
    2. Copying half the shape. protocol.deletePackage finds zero sys_metadata rows the data plane finds 3 of — uninstall leaves orphaned rows (persistence half of #7557) #7705 deliberately did not narrow its no-org branch to organization_id IS NULL, because the direct-mount REST door passes no org and narrowing would have orphaned every org-scoped row — the same bug on the other door. The dev checks whether the equivalent hazard exists here before applying half the pattern.

    Anchors: the card says protocol.ts:12085; #7771 just added 32 lines above it, so that is already stale — re-measure on the branch point. This file has taken six merges today. The precedent remains packages/metadata-protocol/src/sys-metadata-repository.ts:896-899 (#3115); ⚠️ the frequently-cited metadata-service/... path does not exist on main and has already misdirected two cards.

    Pin must use a real engine and a real driver. Both existing deletePackage suites stub engine.find — which is exactly why neither could see the sibling defect, and a stub cannot see this one either. The question is whether organization_id = 'org' matches a NULL column, and that is a property of the driver's SQL, not of a stub's filter(). #7705's packages/runtime/src/package-uninstall-org-scope.integration.test.ts is the worked example, including why it lives in packages/runtime.

    ⛔ Out of scope: #7221 (in-memory layer) and #7780 (the cross-tenant no-org uninstall product question, which is a maintainer call and not this card's to make).


    Generated by Claude Code

  3. claude commented on Aug 11, 2026

    @claude
    Contributor

    #7779 — shipped as draft PR #7814

    Branched from main after #7705/PR #7771 (ecd83fd50) had landed, and read that fix first as the worked example. Merged main again before opening (098b62914).

    Anchor drift, re-measured as instructed: the card says protocol.ts:12085; on the main I branched from it was :12100 (predicate at :12116-12117).


    Step one: the premise is confirmed LIVE, not latent — measured before any fix was written

    The card asked me to settle this first and report rather than ship if nothing writes env-wide commit rows. Something does.

    recordPackageCommit stores organization_id: request.organizationId ?? null. The only door into a publish is the dispatcher's POST /packages/:id/publish-drafts, which forwards an org only when resolveActiveOrganizationId yields one — and that resolver (http-dispatcher.ts:1668) 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 org is selected, or during a transient auth blip, records its commit env-wide permanently (the timeline is append-only).

    On a real engine over real SQLite: a no-org publish wrote organization_id: null; the org-scoped read of that same package then returned [].

    The one thing that changes the card's framing rather than its details

    The card scoped the blast radius as observability/audit, explicitly contrasted with #7705's data-lifecycle defect. Measurement widened it. rollbackToPackageCommit derives the set of commits it must undo from listCommits (protocol.ts:12463), so an invisible commit was silently never reverted:

    pre-fix, measured: an org-scoped rollbackToPackageCommit 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. I did not treat this as re-shaping the card enough to stop, since it widens the justification for the fix the card already specified rather than redirecting it — but it belongs on the record, and the changeset and PR both carry it.

    On design intent — is the under-reporting deliberate scoping?

    The card rightly asked. It is not. revertCommit was already changed by #7559 to resolve each item's scope from the row rather than the request, with the in-code rationale that "a batch legitimately mixes an env-wide artifact with an org overlay." The design already accepts that an org caller legitimately operates on env-wide artifacts, so a timeline that hides them is inconsistent with the revert path that consumes it. No ADR was needed.

    The whole shape, not half of it

    The no-org branch is deliberately not narrowed to organization_id IS NULL. I checked for the equivalent hazard the card warned about and it exists: a no-org caller reads the package's whole timeline, so narrowing that branch would hide every org-scoped commit from that door — the same bug pointed the other way. A dedicated case pins it.

    Reverse verification — direction predicted first

    Predicted: restoring the strict equality turns exactly two cases red (the env-wide commit vanishing from the org-scoped list; the rollback-planner case failing at its list assertion before reaching the rollback lines) and leaves the other four green, because strict equality is narrower than the $or.

    Measured on revert: 2 failed | 4 passed, both failures [1] vs [2]. Prediction held — no finding to record here.

    ⚠️ Follow-up needed — reported, not fixed (a card's worth of work)

    revertCommit (protocol.ts:12180) and rollbackToPackageCommit's own target lookup (:12455) carry the byte-identical strict equality. Since protocol.ts is serialized and this card holds it for listCommits alone, I followed #7705's precedent and report rather than grow the diff.

    Fixing listCommits alone converts rollback's failure from silent to loud: it now 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. The new suite asserts this explicitly so the remainder cannot drift unnoticed before its own card lands.

    Gates — honest status

    Gate Status
    metadata-protocol tests ✅ 1066 passed (72 files)
    objectql tests ✅ 3237 passed (183 files)
    runtime tests ✅ 2039 passed (131 files), incl. the 6 new
    check:type-check-debt ✅ holds — runtime measured exactly 227, its recorded ceiling, 0 errors from the new file
    check:query-options-erasure ✅ holds — 67 non-test sites, none new
    eslint (changed files) ✅ clean
    CI ⏳ in_progress — not awaited, reported at push per #6644 L2

    The type-check trap the card flagged was real and is cleared: runtime's own typecheck excludes *.test.ts via its tsconfig, so I measured the new file the way the ratchet does (raw tsc --noEmit with tests put back, full closure built first) — 227 total, 0 attributable to it.

    Scope respected: no content/docs/releases/ edits, no docs/adr/**, a .changeset/*.md is included, and #7221 / #7780 were left alone.


    Generated by Claude Code

  4. os-zhuang commented on Aug 11, 2026

    @os-zhuang
    ContributorAuthor

    ACCEPT — domain:metadata PM seat. Shipped as PR #7814 (claude/issue-7779-listcommits-org-scope, head 0ee1ec2b2, base 098b62914). All 25 checks green; path face carries no docs/adr/**, so this takes the normal landing route.

    What shipped

    listCommits' strict organization_id equality replaced with the family's $or [{organization_id: oid}, {organization_id: null}], plus packages/runtime/src/package-list-commits-org-scope.integration.test.ts — 6 cases on a real ObjectQL over a real driver-sql, seeded through the real publish path. 422 additions across 3 files, changeset included.

    The three things the card fenced, and how each came back

    Step one — is it live? Answered live, and measured before any fix was written, which is the order the card asked for. recordPackageCommit stores organization_id: request.organizationId ?? null, and the sole door into a publish forwards an org only when resolveActiveOrganizationId yields one — a resolver whose whole body is catch-wrapped, so it answers undefined both for "no active organization" and for any throw on the auth seam. A publish during a transient auth blip therefore records env-wide permanently, the timeline being append-only. That is a stronger result than the card projected: it identifies a mechanism reachable without any user error at all.

    Is the under-reporting deliberate scoping? The dev found the answer in the code rather than assuming either way: #7559 already changed revertCommit to resolve each item's scope from the row rather than the request, on the in-code rationale that "a batch legitimately mixes an env-wide artifact with an org overlay." The design already accepts org callers operating on env-wide artifacts, so a timeline hiding them contradicts the revert path that consumes it. No ADR needed, and no guess made.

    Half the shape? Not taken. The no-org branch is deliberately left package-wide, with a dedicated case pinning it, on the same reasoning that kept #7705's own no-org branch intact.

    The finding that changes the card's framing

    The card scoped this as observability/audit, explicitly contrasted with #7705's data-lifecycle defect. Measurement widened it, and the card was wrong on that point. rollbackToPackageCommit derives the set of commits it must undo from this list, so an invisible commit was a commit the rollback silently skipped: 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 reporting success over a no-op is a correctness defect, not a reporting one. The dev's call to keep going rather than stop and re-scope was right — this widens the justification for the fix the card already specified rather than redirecting it — and it put the finding on the record in the issue, the PR and the changeset rather than letting it ride silently inside a diff.

    Reverse verification was run with the direction predicted first (exactly two cases red, four green, because strict equality is narrower than the $or), and measured 2 failed | 4 passed matching the prediction. That is the discipline working as intended, not a formality.

    Remaining work is now #7819, not this card

    Fixes #7779 is the correct binding: this card was listCommits and listCommits is closed. The dev reported two further sites carrying the byte-identical predicate rather than growing the diff on a serialized file, per #7705's precedent — the right call, and the PM seat's own grep of the file face at 098b62914 found two more it did not report (duplicatePackage and reassignOrphanedMetadata, both on sys_metadata — a different table, outside this card's lane, so not a miss on the dev's part).

    All four are filed as #7819, with the evidence tiers kept apart: revertCommit / rollbackToPackageCommit are measured, the two sys_metadata sites are a grep match whose step one is to establish whether they are live at all. #7819 carries Blocked-by: #7814 and will return to the queue when this lands.

    One handoff detail worth naming here, because it is load-bearing: the new suite's closing assertion (rollback.success === false) pins a known-incomplete state, not a desired one. #7819 says so explicitly, so no later reader mistakes it for the contract.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions