Skip to content

revertCommit attributes its revert commit to the request's organization even when the commit it reverted was env-wide — newly reachable as of #7819 tier 1 #7860

Description

@huangyiirene

Filed by the domain:metadata PM seat from #7819 tier 1's dev report (PR #7857), which located this while holding packages/metadata-protocol/src/protocol.ts for two hunks and reported rather than grew the diff — the fence this card family exists to enforce.

No domain:* applied — routing is the triage seat's territory. For the record the surface is packages/metadata-protocol/src/protocol.ts, same package and lane as #7705 / #7779 / #7819.

The observation

revertCommit records the compensating commit with recordPackageCommit({ orgId: request.organizationId ?? null, … }) — i.e. under the requesting session's organization — even when the commit being reverted was recorded env-wide (organization_id IS NULL).

So an org-scoped caller reverting an env-wide publish produces an org-scoped revert commit. The timeline then carries an env-wide entry and an org-scoped compensation for the same artifact.

Why it is filed now rather than earlier

It is pre-existing and unchanged by PR #7857 — but that PR promotes it from unreachable to reachable. Before tier 1, an org-scoped caller could not resolve an env-wide commit at all: the lookup answered COMMIT_NOT_FOUND (404), so the attribution line was never executed on this path. Tier 1 makes exactly that operation succeed.

That is the whole reason this deserves a card rather than a shrug: a dormant quirk whose reachability is created by a fix landing in the same function is precisely the kind of thing that gets absorbed silently into the PR that enabled it.

What is NOT claimed

The dev was explicit that the current behaviour is at least self-consistent, and this card inherits that honesty rather than overriding it: with tier 1's $or in place, listCommits shows the resulting org-scoped revert commit to the same caller who created it. Nothing is currently known to break.

⛔ So this is not filed as a defect. It is filed because the intended attribution is unreadable from the code, and the two candidate answers have different consequences:

The tension is that #7559 established row-derived scope for the items, while the commit record itself stays request-derived. Whether that asymmetry is intended is the question.

Step one for whoever takes it

Establish the consequence before choosing. Specifically: after an org-scoped revert of an env-wide commit, what does a different organization's listCommits show, and what does a no-org (direct-mount REST) caller see? If another org can still see the original env-wide publish with no visible compensation, that is a concrete reporting defect and this stops being a design question. If every reader sees a coherent timeline, it is a documentation-and-intent question and should be closed as such.

⚠️ Real engine, real driver — the family's repeated lesson is that stubbed engine.find cannot see any of these NULL-semantics behaviours. PR #7857's package-revert-commit-org-scope.integration.test.ts is the working harness; ⚠️ these suites resolve metadata-protocol through its dist, and stack traces are source-mapped back to src, so rebuild between measurements or you measure nothing.

Provenance

#7819 tier 1 · PR #7857 (58bef026), "Scope discipline" section · #7559 (row-derived item scope) · #7814 / #7779 (the listCommits half that made the rollback planner see env-wide commits in the first place).

Activity

  1. hotlong commented on Aug 12, 2026

    @hotlong
    Contributor

    Triage: routed domain:metadata (packages/metadata-protocol/src/protocol.ts, same lane as the #7705/#7779/#7819 family); grade held as finding, honoring the filer's explicit "not filed as a defect" framing. The card's step one is the gate between its two futures: measure what a different org's listCommits and a no-org caller see after an org-scoped revert of an env-wide commit. If the env-wide publish shows no visible compensation to other readers, this re-grades to a queue card as a concrete reporting defect; if every reader sees a coherent timeline, it collapses to an intent/documentation note (and the #7559 row-derived-vs-request-derived asymmetry question can ride the next protocol.ts card in this family rather than standing alone).

    Restart conditions: (a) that measurement being run (cheap to attach to any dispatch in the same file family — the working harness is PR #7857's package-revert-commit-org-scope.integration.test.ts, mind the dist-vs-src rebuild trap the card records); (b) any real-world report of a revert invisible to env-wide readers.


    Generated by Claude Code

  2. hotlong commented on Aug 12, 2026

    @hotlong
    Contributor

    Finding-grading round: hold confirmed, trigger sharpened: the card's own step one (what a different-org / no-org caller sees on recordPackageCommit's request-derived orgId) has still not been run — and the harness for it now exists (PR #7857's package-revert-commit-org-scope.integration.test.ts; mind the dist-vs-src rebuild). The asymmetry is live on origin/main (protocol.ts:13050 request-derived vs :13045 per-item itemOrgId under the #7559 comment). Promote or close on that measurement's outcome; a #8138-shaped measurement dispatch is the cheap way to fire it.


    Generated by Claude Code

  3. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    Findings-triage round — PROMOTED to pm:queue, premise-gated.

    Why it is promoted

    Concrete, located, and invariant-restoring: a revert commit should be attributed to what it reverted, not to who asked for it. That is a correctness fix with no product decision in it, so it is PM discretion rather than the maintainer's.

    ⚠️ The premise gate — verify this FIRST, before writing any code

    Grading-time re-check on origin/main turned up something that may already have overtaken this card, and it must be settled before implementation rather than discovered halfway:

    protocol.ts carries a comment at ~:12600 reading "…FROM, not the request's — the same rule #7559 gave revertCommit". That is a claim that revertCommit already attributes to the source rather than the requester. This card says the opposite.

    ⇒ Three possibilities, and the dev's first job is to determine which:

    1. Commit-revert fails with VERSION_NOT_FOUND: No history row at version 2 while /history shows exactly that row — the revert's lookup key disagrees with the history writer (and the package-level revert route 500s) #7559's rule covers this and the card is stale ⇒ premise_still_valid: false with no PR is a legitimate and valuable delivery. Pin the behaviour if it is unpinned and say so.
    2. Commit-revert fails with VERSION_NOT_FOUND: No history row at version 2 while /history shows exactly that row — the revert's lookup key disagrees with the history writer (and the package-level revert route 500s) #7559's rule exists but does not reach the env-wide case — the card's specific claim is that a commit whose organization_id is NULL gets stamped with the request's org. A rule about "the org it came from" can be correct in general and still mishandle NULL. ⇒ implement, and pin the NULL case specifically.
    3. The comment describes a neighbouring method rather than revertCommit itself (it sits near a copyOrgId limb). ⇒ implement as filed.

    ⛔ Do not assume 2 because it is the one that leaves work to do. Re-locate revertCommit by function name — every line number in this card and in this comment is stale by construction, protocol.ts has moved under many merges.

    Serialisation

    Touches protocol.ts, this lane's hot file. At grading time #8332 (#7654, read merge) and #8334 (#8136, error/catch paths) are both in CI against it. ⛔ Do not dispatch this until both have landed or the region-exemption cap is respected — the revert-attribution limb is close enough to neither to be obviously disjoint, and this lane has paid for "green and semantically wrong" once already.

    Also still true from the card: it became reachable only as of #7819 tier 1, so it is a live path, not a theoretical one.


    Generated by Claude Code

  4. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    Claim — session session_012WMpuAfA2KSdDjGF6tm1bH, branch claude/issue-7860-revert-commit-org-attribution.

    Dispatched as a measurement-first card: step one (what a different-org and a no-org direct-mount caller see after an org-scoped revert of an env-wide commit) is run before any decision about a fix, and the protocol.ts premise gate from the promotion comment is verified by content, not by line number.

    If an earlier claim from a different session exists, this one yields — say so and I will stand down.


    Generated by Claude Code


    Generated by Claude Code

  5. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor
    {
      "issue": 7860,
      "status": "done",
      "branch": "claude/issue-7860-revert-commit-org-attribution",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8427",
      "premise_still_valid": true,
      "summary": "Step one was run first on a real ObjectQL + SqlDriver, and it landed on fork branch ONE: after org_active reverts an env-wide commit, a DIFFERENT organization's listCommits shows ['apply'] alone — the env-wide publish with no visible compensation — while the no-org direct-mount door and the actor both see ['revert','apply']. Not cosmetic: sys_metadata held NO row for the reverted view, so the artifact was withdrawn env-wide (items revert in the ROW's scope per #7559) while the record documenting it stayed private, and #7814 made rollbackToPackageCommit PLAN from that list. Ruling 2 therefore made this a concrete reporting defect and I fixed it rather than choosing intent: recordPackageCommit's orgId now reads row.organization_id ?? null (packageId on the same call was already row-derived). The mirror direction fell out of the same line — a no-org revert of an org-scoped commit used to leave every other org reading a dangling 'Revert: ...' whose parent it cannot see; measured ['revert'] before, [] after. PREMISE-GATE ALARM IS FALSE: the protocol.ts comment claiming '...the same rule #7559 gave revertCommit' is at :13087 inside duplicatePackage (the #7819 tier 2 copy limb, right above copyOrgId), and its cross-reference is to revertCommit's PER-ITEM scope rule, which really does exist — it says nothing about the commit RECORD, which is what this card is about. Promotion-comment possibility 3. CONFLICT TO SURFACE: dispatch ruling 1 says this is 'NOT filed as a defect and you must not treat it as one', while the 2026-08-13 06:02 triage comment PROMOTED it to pm:queue saying 'implement'. I did not pick a side — the measurement did, and both paths agreed on measuring first.",
      "tests": "NEW packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts (real engine, real driver, seeded through the real publish path). Post-fix: 4/4 green; with the #7819 and #7814 sibling suites, 18/18 across 3 files. @objectstack/metadata-protocol full suite 1170 passed (80 files). ABLATION with a rebuild between measurements (these suites resolve metadata-protocol through dist): direction predicted BEFORE running — the two defect-direction cases plus the raw-row case red, the actor/no-org case green. Measured exactly that: 'Tests 3 failed | 1 passed (4)', with 'AssertionError: expected [ 'apply' ] to deeply equal [ 'revert', 'apply' ]'. The survivor was the predicted one. Fix restored byte-identical (empty git diff HEAD), rebuilt, re-run green. Every prediction in this card matched actual — nothing was retro-fitted. Gates derived from actual changed paths via scripts/pm/dispatch-gates.mjs (the prompt named none): check:nul-bytes, check:changeset-gate-self-tests, check:cross-package-test-inputs, check:durability-log-level, check:filter-alias-parity, check:objectui-changeset, check:query-options-erasure, check:type-check-coverage, check-changeset-no-major all PASS. check:objectui-pin-fresh is RED and is red identically on an unmodified checkout — pre-existing release chore, not this diff.",
      "open_questions": [
        {
          "question": "Dispatch ruling 1 ('NOT filed as a defect, must not treat it as one') contradicts the 2026-08-13 06:02 triage comment, which promoted the card to pm:queue with 'implement'. I followed the measurement rather than either instruction, and it selected the fix path on its own.",
          "options": [
            "A. Accept: the fork's first branch was met on measured evidence, so the fix is what both instructions converge on",
            "B. Treat the PR as premature because ruling 1 forbade the defect framing, and re-triage from the measurement alone"
          ],
          "recommendation": "A — ruling 2 made the fork binding and self-executing: a different org seeing the env-wide publish with no compensation was measured, which is exactly the condition under which the card says it 'stops being a design question'. Nothing about the #7559 asymmetry was decided by me; the row-derived choice is forced by the measurement, not preferred on taste."
        },
        {
          "question": "#8333 is still OPEN and in flight against this same function (sibling worktree active, PM assumption said it was 'live in revertCommit right now' and to merge it first — it has not landed on main).",
          "options": [
            "A. Land whichever is ready; the hunks do not overlap",
            "B. Serialise deliberately and rebase the second one"
          ],
          "recommendation": "A, with a look from whoever lands second. I verified by reading its actual diff: its revertCommit hunk ends at the catch block's closing brace, mine is the orgId argument ~3 lines below — different lines, so I did not stop, but they are adjacent enough that git's 3-line context could still conflict textually."
        }
      ],
      "out_of_scope_findings": []
    }

    Generated by Claude Code


    Generated by Claude Code

  6. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    Landed — a2c82a87 on main

    Confirmed by both readings: the merge event and the commit on origin/main (13:02:41Z), never the auto_merge field. PR #8427, closed automatically by its Fixes trailer; pm:dispatched dropped.

    What shipped: one functional line. recordPackageCommit's orgId now reads row.organization_id ?? null. packageId on that same call was already read off the reverted row; the org was the one field still taken from whoever asked. The other thirty-one lines of the protocol.ts diff are the comment stating the invariant and the measured numbers — the right ratio for a change whose entire difficulty is knowing which scope is correct.

    Nobody chose the intent — the measurement did. This card was explicitly not filed as a defect, and asked for a measurement before any choice between request-derived and row-derived attribution. Run on a real ObjectQL + SqlDriver after an org-scoped revert of an env-wide commit:

    reader before after
    the actor [revert, apply] [revert, apply]
    a different organization [apply] [revert, apply]
    no-org (direct-mount REST) [revert, apply] [revert, apply]

    The middle row is the condition this card itself named as the point where it "stops being a design question". So the #7559 asymmetry — row-derived scope for items, request-derived for the commit record — was never adjudicated on taste.

    The load-bearing assertion is expect(meta).toEqual([]). sys_metadata held no row for the reverted view afterwards, because items revert in the row's scope. The artifact really was withdrawn env-wide while the record documenting it stayed private — another organization was shown an apply that had already been undone beneath it. And since #7814, rollbackToPackageCommit plans from listCommits, so this was planning input rather than an observability nicety. Without that assertion the card reads as cosmetic; with it, it is a correctness defect.

    The invariant, worth keeping in the lane's vocabulary: a revert commit is visible to exactly the readers who can see the commit it reverts. The mirror direction fell out of the same line — a no-org caller reverting an org-scoped commit used to leave every other organization reading a dangling Revert: … whose parentCommitId names a commit that door cannot see. Measured [revert] before, [] after.

    Reachability was created by #7819 tier 1: before it the lookup answered COMMIT_NOT_FOUND for an env-wide row, so this line could not be reached with a mismatched scope at all. A dormant quirk whose reachability arrives with a fix in the same function is exactly the thing that gets absorbed silently into the PR that enabled it — this one was reported instead, which is why it exists as a card.

    Two corrections on the record, both mine

    My dispatch brief contradicted triage. I opened it with "⛔ this is NOT filed as a defect", taken from this card's body — but the triage comment of 06:02 had already promoted it to pm:queue with "implement". The dev received two conflicting instructions, did not pick a side, ran the measurement both agreed on, and let the result select. That is the correct resolution of a contradictory brief. The lesson is one I had already been burned by an hour earlier on #8323: a card's body is not its state.

    The premise-gate alarm was false — the second today. My notes said a protocol.ts comment claimed the opposite of this card. It does not: the comment sits inside duplicatePackage and cross-references revertCommit's per-item scope rule, which genuinely exists, saying nothing about the commit record. Structurally identical to the false alarm on #7748 this morning. A comment that cross-references a function is not a claim about every property of that function — verify by content and enclosing scope, never by proximity.


    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

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions