Skip to content

fix(metadata-protocol): attribute a revert commit to the scope of the commit it reverts - #8427

Merged
os-zhuang merged 3 commits into
mainfrom
claude/issue-7860-revert-commit-org-attribution
Aug 13, 2026
Merged

fix(metadata-protocol): attribute a revert commit to the scope of the commit it reverts#8427
os-zhuang merged 3 commits into
mainfrom
claude/issue-7860-revert-commit-org-attribution

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#7860

revertCommit recorded its compensating commit under the requesting session's organization. packageId on that same recordPackageCommit call is already read off the reverted row; the org was the one field still taken from whoever asked. It now reads row.organization_id ?? null — the rule #7559 gave this function's items (resolveMetaItemOrgScope) and #7819 tier 2 gave duplicatePackage's copies, applied to the commit record that documents them.

One functional line. The rest of the protocol.ts diff is the comment stating the invariant and the measured numbers.

The card was NOT filed as a defect — the measurement settled it

#7860 was explicit that the behaviour is self-consistent for the caller who performed the revert, and asked for a measurement before choosing between "attribute to the request" and "attribute to the reverted row". Run on a real ObjectQL + SqlDriver (better-sqlite3) through the real publish path, after an org-scoped revert of an env-wide commit:

readerbeforeafter
the actor (org_active)[revert, apply][revert, apply]
a different organization[apply][revert, apply]
no-org (direct-mount REST)[revert, apply][revert, apply]

The middle row is the fork the card named: a different org could still see the original env-wide publish with no visible compensation, which makes this a concrete reporting defect rather than a design question — so the intent question was never mine to pick.

What makes it more than cosmetic is the artifact state measured alongside it: sys_metadata held no row for the reverted view afterwards. Items revert in the row's scope (#7559), so the artifact really was withdrawn env-wide — the effect was global while the record was private, and another organization saw an apply that was never compensated for an artifact already gone. Since #7814, rollbackToPackageCommitplans from listCommits, so this list is not merely an observability surface.

The mirror direction, same line

A no-org caller reverting an org-scoped commit stamped the revert env-wide, so every other organization read a dangling Revert: ... whose parentCommitId names a commit that door cannot see. Measured before: [revert] for an unrelated org; after: [].

Both collapse to one invariant: a revert commit is visible to exactly the readers who can see the commit it reverts.

Verification

packages/runtime/src/package-revert-commit-attribution-org-scope.integration.test.ts — real engine, real driver (a stubbed engine.find cannot see NULL semantics; that is this family's repeated lesson).

Ablation, with a rebuild between measurements because these suites resolve metadata-protocol through its dist while stack traces map back to src. Direction predicted before running: restoring the request-derived orgId should turn the two defect-direction cases plus the raw-row case red and leave the actor/no-org case green. Measured: exactly that — 3 failed, 1 passed, the survivor being the predicted one.

AssertionError: expected [ 'apply' ] to deeply equal [ 'revert', 'apply' ]

Restored byte-identical (empty git diff HEAD), rebuilt, re-run green. Sibling #7819 and #7814 suites stay green: 18/18 across the three files. @objectstack/metadata-protocol full suite: 1170 passed (80 files).

Gates derived from the actual changed paths via scripts/pm/dispatch-gates.mjs and run locally — 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 — a pre-existing release-time pin chore, not this diff.

Coordination

#8333 is still open and in flight against this same function. Its revertCommit hunk ends at the catch block's closing brace; this change is the orgId argument roughly three lines below, so the two do not touch the same lines — but they are adjacent enough that whichever lands second may want a look. origin/main was merged in at 4827e915d1: clean, and no merged commit touched protocol.ts.


Generated by Claude Code

…driver
Measured on real ObjectQL + SqlDriver: before the fix a different
organization's listCommits showed the env-wide apply commit with no
compensation, while the artifact was already withdrawn env-wide.
Refs #7860
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 12:42pm

Request Review

@github-actionsgithub-actionsBot added size/m documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol.

3 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM note before review — a contradiction in my dispatch, and a second false alarm

Two things to put on the record while CI runs. Neither changes the verdict; both are mine, not the PR's.

1. My dispatch ruling contradicted triage, and the dev was right not to pick a side

My brief opened with "⛔ This is NOT filed as a defect, and you must not treat it as one." I took that from #7860's own body, which says so explicitly. But the triage comment of 2026-08-13 06:02 had already promoted the card to pm:queue with "implement". I read the card and wrote a ruling from its original framing without weighting the promotion that had superseded it — so the dev received one instruction saying "do not treat this as a defect" and another saying "implement it".

It handled that correctly: it did not pick a side, it ran the measurement both instructions agreed on first, and let the result select. That is the right resolution of a contradictory brief, and it is the reason the framing conflict cost nothing.

Ruled: option A. The fork in ruling 2 was binding and self-executing, and the measurement met its first branch on evidence — a different organization saw the env-wide publish with no visible compensation. The card's own words are that this is the condition under which it "stops being a design question". Nothing about the #7559 asymmetry was chosen here; the row-derived attribution is forced by the measurement, not preferred on taste. ⛔ The PR is not premature.

The seat-level lesson is the one this seat has now paid for twice today: a card's body is not its state. I re-read #8323's comments before dispatching precisely because I had been burned on that at 12:05Z, then wrote this brief from #7860's body anyway.

2. The premise-gate alarm is FALSE — and that is the second one today

My notes flagged that a protocol.ts comment "claims the opposite of the card". The dev located it and it does not: the comment sits at :13087inside duplicatePackage (the #7819 tier 2 copy limb, immediately above copyOrgId), and its cross-reference is to revertCommit's per-item scope rule — which genuinely exists — saying nothing about the commit record, which is what this card is about.

That is structurally identical to the alarm on #7748 earlier today, where a comment had relocated into getEffectiveLock's fail-open catch and its "writes its ordinary outcome:allowed row" referred to the save path. Two for two. The pattern is worth stating: a comment that cross-references a function is not a claim about every property of that function, and in a file this size a note recorded as "a comment contradicts the card" is more often an adjacent truth than a contradiction. Verify by content and by enclosing scope, never by proximity — which is exactly what both devs did.

On the #8333 adjacency

Agreed with the dev's option A: land whichever is ready. It verified by reading #8333's actual diff — that hunk ends at the catch block's closing brace, this change is the orgId argument roughly three lines below. Different lines, but inside git's three-line context, so a textual conflict is possible even though neither edit touches the other's statement. I have warned #8333 directly; whoever lands second merges main and takes a look rather than resolving blind.

⚠️ I will verify the check:objectui-pin-fresh claim against CI's own Console Pin Freshness check rather than accept it — that gate lives in objectui-pin-freshness.yml, not lint.yml, and I stated its location wrongly to a dev earlier today.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

ACCEPT — PM review, domain:metadata seat

Green at 34ea935d: all 25 checks, each job's own conclusion verified individually. Path-fork check clean — 3 files, none under docs/adr/**, .claude/skills/** or skills/**.

The check:objectui-pin-fresh claim is verified, not accepted. The report said it is red locally and red identically on an unmodified checkout. CI's Console Pin Freshness is success on this head, and that check — not the local script, and not lint.yml — is the authority for the pin. So the local red is the pre-existing release chore the dev said it was. I insisted on checking because I stated that gate's location wrongly to a dev earlier today; the claim holds.

The measurement settled the fork, and nobody chose an intent

The card asked for a measurement before choosing between request-derived and row-derived attribution, and ruled that a different organization seeing the env-wide publish with no visible compensation would make it a concrete reporting defect rather than a design question. That is exactly what was measured: [apply] alone for an unrelated org, against [revert, apply] for the actor and the no-org door. So the #7559 asymmetry was never adjudicated by a dev on taste — the fork resolved itself on evidence, which is what a measurement-first card is for.

The load-bearing detail is expect(meta).toEqual([]).sys_metadata holds no row for the reverted view afterwards, because items revert in the row's scope (#7559). So the artifact really was withdrawn env-wide while the record documenting it stayed private — another organization was being shown an apply that had already been undone underneath it. And since #7814rollbackToPackageCommitplans from listCommits, so this is planning input, not just an observability surface. Without that assertion the case would read as cosmetic; with it, it is a correctness defect.

What makes the test right rather than merely present

  • Real ObjectQL + real SqlDriver over on-disk better-sqlite3, seeded through the real publish path, with the reason stated in the header: "every assertion here turns on whether organization_id = 'org' matches a NULL column — a property of the driver's SQL, not of a stub's filter()." This family's stubbed suites are structurally unable to see any of it.
  • The third case is the anti-regression arm, and its comment names the failure it exists to prevent: "the change may only ADD the missing reader, never trade one blind spot for another." The actor's and the no-org door's timelines were coherent before and must stay so. A fix that moved the blind spot rather than closing it would pass cases 1, 2 and 4 and fail only this one.
  • The mirror direction is pinned, not just mentioned: a no-org caller reverting an org-scoped commit used to leave every other org reading a dangling Revert: … whose parentCommitId names a commit that door cannot see. Measured [revert] before, [] after — same line, opposite direction.
  • Ablation with a rebuild between measurements, with the dist hazard named in the header. Predicted 3 red / 1 green, measured exactly that, survivor as predicted.

One functional line, thirty-one lines of comment stating the invariant and both measured directions. That ratio is correct for a change whose difficulty is entirely in knowing which scope is right.

The invariant is worth keeping in the lane's vocabulary:a revert commit is visible to exactly the readers who can see the commit it reverts.

⚠️#8333 is still in flight against this same function. Its revertCommit hunk ends at the catch block's closing brace; this change is the orgId argument roughly three lines below — different statements, but inside git's three-line context. I have warned it directly, with the instruction not to resolve any conflict by taking either side: this line changes which organization a revert is attributed to, its change governs what error text reaches a client-facing payload, and neither suite would catch the other being dropped.

Marking ready and enqueueing. Fixes #7860 closes the card on merge.


Generated by Claude Code

@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 13:02
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit a2c82a8Aug 13, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-7860-revert-commit-org-attribution branch August 13, 2026 13:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-zhuang@claude