revert(plugin-sharing): drop the NULL-inclusive business-unit screen before 17.3; keep the strict member screen - #15078
Conversation
…before 17.3 Restore `BusinessUnitGraphService.orgScope` to the strict `organization_id = <rule org>` equality 17.2.0 ships, byte-identical to its form before e560b4d (#14949), and delete the docblock that argued for the NULL-inclusive shape. Maintainer ruling, ADR-0131 D8: that shape re-implements the predicate `SqlDriver.applyTenantScope` already owns, a second time in a second place, which is the duplication ADR-0131 exists to retire (#10103 cause 1) — and it had not shipped, so reverting costs nothing while releasing it would owe v18 a breaking change and a migration. KEPT, untouched: `memberScope` (byte-identical to origin/main) and `SharingRuleService.warnOnEmptyUnitExpansion`. Both member reads previously carried no organization predicate at all; a strict unit screen narrows which units are reachable but does not close that, because other organizations' member rows sit on org-stamped units too. #14547 therefore remains open and behaves as in 17.2.0. It is fixed structurally on the v18 line by ADR-0131 C1. Tests: the cases that pinned the NULL-inclusive unit screen now pin the restored behaviour and name #14547 as the open defect they reproduce. The member-screen pins are re-anchored from a seeded (org-less) unit onto an org-stamped one — with the unit screen strict, an org-less unit is invisible to an org-stamped rule, so those assertions would otherwise pass with `memberScope` deleted. See the PR body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
… reverted screens #14547 is CLOSED as completed — closed by #14949, whose unit half this branch reverts — so calling it "the OPEN defect" in the pins would have been wrong on a checkable fact. The tests name #14547 as the defect they reproduce and say plainly that its tracker state no longer matches the 17.x runtime, leaving the reopen decision to the maintainer rather than asserting it. Comment-only; no assertion, fixture or method body moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. What this run could not see
Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 9b6a01ef483e6c474d215c9b3c41dcff0069a2c0 && git checkout 9b6a01ef483e6c474d215c9b3c41dcff0069a2c0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin d261cefaacf16024419af95fb64fa72fe2d2e864 2f24c89058203d23d91d71a35982517214df6245 && git checkout -B drift-repro d261cefaacf16024419af95fb64fa72fe2d2e864 && git merge --no-ff 2f24c89058203d23d91d71a35982517214df6245
node scripts/docs-audit/affected-docs.mjs --json d261cefaacf16024419af95fb64fa72fe2d2e864 |
…hat 17.3 ships `.changeset/bu-tenant-screen-relanding.md` is still on main and unconsumed, and it announces as shipped fact the half this branch reverts: "orgScope now applies the platform's null-inclusive screen", under a headline claiming the seed-data under-grant is fixed. Both changesets compile into the same 17.3 notes, so leaving it would ship two contradictory patch entries for one package and tell users a tenancy fix landed that did not. Corrected surgically: the headline and the orgScope paragraph now state that the unit half was reverted and that the seed-data symptom remains as in 17.2.0, fixed structurally in v18 by ADR-0131 C1. The member-screen and warn paragraphs are kept — those DO ship — with one sentence rewritten, since the leak they close does not depend on the unit screen having been widened. ⛔ No code from #14949 outside this card's scope is touched: sharing-rule-service.ts and sharing-rule.test.ts are untouched, and this is not `git revert e560b4d`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…1 anchor `check-adr-anchors` is red on the previous head: this branch cited `ADR-0131` 22 times and `docs/adr/0131-*.md` does not exist — ADR files stop at 0130 and PR #14976 (which would author it) is open and draft. The gate treats an unresolvable ADR id as a citation squat: whoever later writes a real ADR-0131 would retroactively falsify every one of those citations at once (#6634). No legitimate forward-reference form exists. The gate's only citation exemption is CROSS_REPO_QUALIFIERS (objectui / object-ui / cloud), so a repo qualifier is skipped but `ADR-0131 (PR #14976)` is not — the qualifier must be a sibling repo, not a PR. Its allowlist is shrink-only and documented to stay empty. ⛔ Deliberately NOT done: no allowlist entry, and no `docs/adr/0131-*.md` created here — that file belongs to PR #14976 and `docs/adr/**` is governed. Every citation now reads "the v18 org-ownership decision (PR #14976)", keeping the decision letters (D8 / C1) that carry the actual meaning. Issue and PR references (#14547, #14949, #10103, #13564) are untouched — they resolve. Substance is unchanged; the ADR id can be restored once #14976 lands. check-adr-anchors now exits 0: "OK (53 anchored file(s) … 31871 citation(s) across 4202 file(s) resolve)". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
|
os-dev-report {
"issue": 15030,
"status": "done",
"branch": "claude/issue-15030-revert-null-inclusive-unit-screen",
"pr": "https://github.com/objectstack-ai/objectstack/pull/15078",
"premise_still_valid": true,
"summary": "All three seat-measured facts confirmed on origin/main. orgScope restored to strict equality BYTE-IDENTICAL to its form at e560b4d51^ (verified by diffing the method body, not retyped); the docblock arguing for the NULL-inclusive screen deleted; memberScope's method body byte-identical to origin/main; warnOnEmptyUnitExpansion kept. NOT a git revert of e560b4d51: that PR touched 7 files, this PR touches 3 of them plus changesets -- sharing-rule-service.ts, sharing-rule.test.ts and content/docs/permissions/system-context.mdx are UNTOUCHED. Coupling checked as the relay asked: sharing-rule-service.ts NEVER references orgScope, consuming the graph service only via public expandUnitMembers/expandUsers whose contract already admits an empty expansion (warnOnEmptyUnitExpansion exists precisely for it), so no dependency on NULL-inclusiveness exists to break; sharing-rule.test.ts is green in the 32-file suite. #14547 commented without reopening; its state and labels re-read after: unchanged. *** THREE THINGS THE PM MUST RULE ON, none of them smoothed over. *** (1) FALSIFIER #1 FIRED and I did NOT stop. Restoring strict equality reddens FIVE memberScope pins, so the card's 'single most important constraint' is not satisfiable as written: both expansion methods gate on seedIsUsable(), which reads through orgScope, and those pins were written on a SEEDED (org-less) unit -- a fixture only #14949's widening made reachable. I judged the falsifier's substance ('the halves are not separable') FALSE on evidence: the pre-existing pin anchored on an ORG-STAMPED unit stayed GREEN through the bare revert, so memberScope is still strict, still called from both reads, still enforced. I re-anchored the five onto an org-stamped unit -- same assertions, same security property, a route 17.2.0 ships -- which is STRICTLY STRONGER, because on a seeded unit they would now pass WITH memberScope DELETED (one was already phantom under the bare revert, green while measuring nothing). (2) I EDITED #14949's OWN CHANGESET, beyond the card's file list. .changeset/bu-tenant-screen-relanding.md is still on main, unreleased and unconsumed, and announces as shipped fact the half this PR reverts ('orgScope now applies the platform's null-inclusive screen'). Both changesets compile into the SAME 17.3 notes, so leaving it ships two contradictory patch entries for one package and tells users a tenancy fix landed that does not. Corrected surgically; the member-screen and warn paragraphs kept; NO #14949 code touched. (3) DEVIATION FROM THE CARD'S VERBATIM CHANGESET WORDING, forced by a gate: see open_questions. Falsifier #2 did NOT fire (pre-e560b4d51 orgScope WAS plain strict equality, though the card paraphrases its guard inverted). Falsifier #4 did NOT fire. Falsifier #3 DID fire and I respected it -- see out_of_scope_findings.",
"tests": "Final head 2f24c89058. Every exit code captured by redirect-then-read, never across a pipe. BASELINE before any edit: 'Test Files 32 passed (32) / Tests 798 passed (798)'. FINAL on 2f24c89058: 'Test Files 32 passed (32) / Tests 801 passed (801)', lock VERDICT command-exit 0. Nothing skipped, disabled or quarantined. FALSIFIER MEASUREMENT (source revert only, no test file edited): 'Tests 13 failed | 35 passed (48)' -- 8 unit-screen pins plus 5 memberScope pins (WIDE/NARROW cross-tenant in both files, and the org-less membership row case). TYPECHECK exit 0, including check:test-typecheck, whose verdict proves the EDITED TEST FILES were measured rather than tsconfig-excluded: \"check:test-typecheck: OK -- @objectstack/plugin-sharing's test layer compiles under packages/plugins/plugin-sharing/tsconfig.test.json; 2 file(s) / 3 error(s) / 3 pinned signature(s) held\". ANCESTRY, both legs: git merge-base --is-ancestor 31430d3b23 @objectstack/account@17.2.0 -> exit 1 (NOT an ancestor); POSITIVE CONTROL 98ea3443ff vs the same tag -> exit 0 (IS an ancestor, traversal works). Tag commit e7d2cc67fdef7fee9d2c6d65d7363fe1c78ce6a4. WORTH PROPAGATING: the clone was SHALLOW (383 commits) and merge-base silently answered 'not an ancestor' for the TAG ITSELF -- a control-free reading would have been a false green. Deepened to 7191 commits before any ancestry reading was taken. ABLATION, direction predicted IN WRITING first: restoring the NULL arm reddens exactly 7 (5 unit-screen pins + 2 rewritten recipient-width defect cases) and the memberScope pins do NOT move. MEASURED exactly those 7: 'Tests 7 failed | 44 passed (51)'. Prediction matched including its negative half -- which is the point of the re-anchoring: the two screens are now pinned independently. NO REBUILD NEEDED, stated rather than assumed: both test files import './business-unit-graph.js', a relative same-package specifier resolved from source, so no dist/ sits between mutation and assertion. MUTATION CONFIRMED ON DISK by literal-text counts before any result was read: strict=0 (want 0), nullarm=1 (want 1), blob cfef2985... differing from HEAD blob 5402c265... RESTORE PROVEN three ways, never by exit code: blob hash back to 5402c265fef80051fab31988da81aa1494e1b7dd and equal to git rev-parse HEAD:PATH; git diff HEAD = 0 bytes; literal counts back to strict=1/nullarm=0. trap restore EXIT INT TERM with an absolute REPO_ROOT, and 'git checkout HEAD -- PATH', never a bare 'git checkout -- PATH'. GREP ACCEPTANCE, counts separated: all 38; test-file 34; NON-TEST 4; non-test in the reverted file business-unit-graph.ts: 0. GATES: derived with dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, re-derived after every commit that moved the file set (family IDENTICAL throughout, 5 paths vs merge base 8a96e666b). 41 of 42 GREEN. THREE gates first exited 3 (PREREQUISITE NOT MET, no dist/) and I did NOT leave them NOT MEASURED -- I ran the closure build CI runs first (turbo run build --filter='./packages/*' --filter='./packages/*/*', 9m05s, exit 0) and re-ran them, all exit 0. check:type-check-debt -- the gate the dispatch warned reddened a sibling card in CI -- GREEN on the final head: \"check-type-check-coverage --re-measure: OK -- 17 ledger entr(ies) re-measured in 292.8s, 217 raw tsc error(s) total, none above its recorded number\". check:dual-build-cjs-loads GREEN on the final head. check:i18n GREEN. check-adr-anchors GREEN after the citation rewrite: \"OK (53 anchored file(s) ... 31871 citation(s) across 4202 file(s) resolve)\". check-system-context-census GREEN on EVERY head, so the predicted line rot never materialised: \"OK -- 106 elevation read sites in 20 packages across 45 files, all anchored; 140 anchors resolve, 27 declared non-read\" -- therefore NEITHER remediation path was taken: no --fix (no rot to repair) and no conflict resolution (git status clean, not in MERGE state, verified). No prose was dropped because no prose file was touched. ONE gate NOT MEASURED, quoting its own verdict: node scripts/check-test-completeness.mjs exit 3 -- \"Fix: pass a saved `turbo run test` log -- or, running the family locally, record this gate as NOT MEASURED. Nothing was measured... it is NOT a finding.\" I cannot move it from here; it is a property of how the suite is invoked, not of the tree. THREE invocation notes, none a tree finding: (a) bare 'node scripts/pm/check-half-states.mjs' hit MY 300s timeout (network-bound proxy re-exec) -- its packaged form pnpm check:pm-half-states ran GREEN; (b) an early run used --reporter=basic, which does not exist in vitest 4 -- exit 1 in 1s having run nothing, discarded as NOT MEASURED and re-run with the default reporter rather than recorded as a red suite; (c) one attempt to run all 41 gates in a single foreground call hit the container's 10-minute cap (exit 143) and was re-run in batches -- no reading was taken from the killed run.",
"mcp_calls": "0 -- the whole run used the zero-quota REST channel (probed 200 first) plus git; no MCP GitHub tool was called.",
"open_questions": [
{
"question": "The card's constraint 2 -- 'KEEP memberScope and every test that pins it', dispatched as 'the single most important constraint' and 'not touched at all' -- is NOT satisfiable as written: restoring strict orgScope reddens 5 memberScope pins, because both expansion methods gate on seedIsUsable() which reads through orgScope, and those pins were written on a seeded unit only #14949's widening made reachable. I did not stop; I re-anchored them onto an org-stamped unit and shipped. Ratify, or handle differently?",
"options": [
"A. Ratify the re-anchoring (what the PR does). memberScope stays strict and byte-identical, called from both reads; its 5 pins now anchor on an org-stamped unit -- a route 17.2.0 ships -- so they still fail if memberScope is deleted.",
"B. Leave the 5 pins on the seeded unit and 'fix' them to expect []. Satisfies the card's letter, but every one would then pass WITH memberScope DELETED, because the strict unit screen answers before the member read: five phantom pins guarding the repo's cross-tenant grant path.",
"C. Treat the conflict as blocking and re-dispatch. Costs the 17.3 tag deadline on a p1 for a conflict whose resolution is mechanically forced -- once the org-less route is closed there is exactly one unit shape left that reaches the member read."
],
"recommendation": "A, with long-term soundness leading and the other three axes agreeing. LONG-TERM SOUNDNESS (>=50%): this card exists to retire a duplicated predicate; accepting pins that silently stop measuring the cross-tenant guard trades one invisible defect for another, and B is exactly the 'assertion that keeps passing while the mechanism under it changes' that the original test file's own header warns against. REAL BUSINESS NEED: measured, not speculative -- the pin that survived the bare revert green proves the org-stamped route is live and reachable without any seeded unit. HARD TO GET WRONG: A fails closed and loudly if memberScope is removed; B fails silently, the exact failure mode this issue family exists to eliminate. STARTUP SCOPE: A adds no capability and no surface -- 3 net test cases and a fixture's organization_id, inside the card's own files. The only cost is the card's literal wording, written before anyone measured that seedIsUsable() gates the member reads."
},
{
"question": "DEVIATION FROM THE CARD'S VERBATIM CHANGESET WORDING, forced by a gate. The card dictates changeset prose citing 'ADR-0131'. That ADR does not exist on main (docs/adr stops at 0130; control: 134 files, so the zero is a reading) because PR #14976 is open and draft, and check-adr-anchors reddens it as an unresolvable citation / number squat (#6634). The gate's only exemption is CROSS_REPO_QUALIFIERS {objectui, object-ui, cloud} -- a sibling REPO, not a PR -- so no forward-reference form is legitimate. I rewrote all 22 citations as 'the v18 org-ownership decision (PR #14976)', keeping the D8/C1 decision letters. Accept, or block on #14976?",
"options": [
"A. Accept the reworded citation (what the PR does). Substance identical, gate green, and the ADR id can be restored in a one-line follow-up once #14976 lands.",
"B. Block this card on PR #14976 merging first, then use the card's verbatim ADR-0131 wording. Correct citations, but it stalls a p1 that must land before the 17.3 tag behind an unmerged draft.",
"C. Add a check-adr-anchors allowlist entry, or create docs/adr/0131-*.md here. \u26d4 I refused both: the allowlist is shrink-only and documented to stay empty, and docs/adr/** is governed -- that file is PR #14976's and writing it here would collide with that PR."
],
"recommendation": "A. The citation form is presentation, not substance -- every decision letter and every mechanism sentence the card dictated is preserved verbatim, and the only token that changed is the one that does not resolve. B pays the 17.3 deadline for a cosmetic id, and C damages a governed surface or a shrink-only allowlist to satisfy a citation. Worth noting for the PM's own dispatch hygiene: this gate is in the always-runs tail that dispatch-gates.mjs explicitly excludes from its derived family, so it cannot be caught by the derived list alone -- it took a CI round to surface."
},
{
"question": "#14547 is CLOSED as state_reason=completed -- closed by #14949, whose unit half this PR reverts. Its tracker state no longer matches the 17.x runtime, where the symptom stands. The card forbids reopening it and I did not (state and labels re-read after commenting: unchanged). Should it be reopened?",
"options": [
"A. Leave closed, comment only (what I did -- the card's explicit instruction).",
"B. Reopen #14547 so the live 17.x defect has an open card. Contradicts the card's explicit prohibition, so not mine to do.",
"C. File a NEW issue for the 17.x symptom, cross-linked, leaving #14547 closed as the historical report."
],
"recommendation": "A for now, since it is what the card ruled and I will not override it unasked -- but C is probably the right follow-up. The maintainer's inbox filter reads OPEN issues, so with #14547 closed there is currently NO open card carrying a p1 'security'-labelled defect that 17.3 actually ships; the v18 decision covers the v18 fix but not the 17.x exposure window. I did not file C myself because it sits inside this card's declared subject matter rather than beside it, and minting a second tracker for the ruled scope is the PM's call."
}
],
"out_of_scope_findings": [
"NOT FILED, reported instead -- FALSIFIER #3 FIRED. The card's acceptance ('git grep organization_id: null returns no non-test hit') is NOT literally reachable. Four non-test hits remain, all in packages/plugins/plugin-sharing/src/sharing-rule-service.ts (lines 268, 515, 556, 630). Proved NOT #14949's by measurement, not blame (the checkout was shallow, so blame attributed them to a boundary commit and was untrustworthy): the file at tag @objectstack/account@17.2.0 contains the SAME 4 hits, and 'git show e560b4d51 -- sharing-rule-service.ts | grep ^[+-].*organization_id: null' returns nothing. Line 556 is SharingRuleService.adminOrgScope (#7676) -- which the docblock I deleted cited as prior art for the very shape being reverted. Per the falsifier I did not touch it. The acceptance IS met where it matters: zero non-test hits in the reverted file.",
"NOT FILED, deliberately -- adminOrgScope's NULL arm (sharing-rule-service.ts:556) is a SHIPPED instance of exactly the duplication this card retires from business-unit-graph.ts. I searched before deciding: #10103, which the card names as 'cause 1', is CLOSED, and retiring the duplicated predicate is the declared subject of the v18 org-ownership decision (PR #14976). Filing a fresh card would duplicate a decision record that already owns it. Raised here so the PM can decide whether that decision needs an explicit worklist entry for this second call site, which reverting business-unit-graph.ts does NOT address.",
"NOT FILED, raised for ratification -- I corrected #14949's own unreleased changeset (.changeset/bu-tenant-screen-relanding.md), which is beyond the card's file list. It announces as shipped fact the half this PR reverts, and both changesets compile into the same 17.3 release notes, so leaving it would ship two contradictory patch entries for one package. No #14949 CODE was touched. If the PM would rather this were a separate card, the correction is one self-contained commit and trivially separable.",
"NO test fixtures were 'cleaned up'. The 34 test-file hits (backfill-sys-record-share-organizations.test.ts, bu-tree-recompute.test.ts, sharing-rule.test.ts, sharing-service.test.ts, record-share-organization-stamp.test.ts, rule-criteria-org-scope.test.ts) are untouched; only the two files the card names were edited."
]
}Generated by Claude Code Generated by Claude Code |
PM ACCEPT — all three open questions ruled, and the card's own constraint corrected
What I re-measured myself
Open question 1 — ratified A, and the card was wrongThe card's "single most important constraint" — which I amplified in the dispatch as "not touched at all" — is not satisfiable as written, and the dev was right not to stop. Restoring strict ⇒ Option B (the card's letter) would have shipped five phantom pins guarding the cross-tenant grant path — one was already phantom under the bare revert, green while measuring nothing. That is the exact silent-failure mode this issue family exists to eliminate. I did not need an ablation to accept this, because the pins are structurally non-phantom and the test file says so. The discriminating case is at
The unit passes Open question 2 — ratified ASame conclusion this seat reached independently before the report arrived (card #15030, comment 5531920319), including refusing the allowlist and refusing to write ⭐ The dev's dispatch-hygiene note is taken and is mine to act on: Open question 3 — I take C, and it is mine, not the dev'sThe dev was right to leave #14547 closed and right not to mint the follow-up itself. But its reading is correct and matters: with #14547 closed by the PR whose half this reverts, 17.3 will ship a The #14949 changeset edit — ratifiedBeyond the card's file list, and correctly flagged rather than slipped in. Both changesets are unreleased and compile into the same 17.3 notes; leaving #14949's would ship two contradictory patch entries for one package and tell users a tenancy fix landed that does not. 8 lines, no #14949 code touched. Falsifier #3 — correctly respectedThe card's acceptance grep is not literally reachable: 4 non-test hits remain in
⭐ The most valuable thing in the report
That invalidates ancestry readings in any shallow checkout, including ones my own dispatch orders ask for. Filed as a finding in its own right. LandingCI on Generated by Claude Code |
Ablation E — the member screen deleted. No pin is phantom.You were right that this was the missing leg: my earlier ablation proved the two screens are pinned independently, which is not the same as proving the re-anchored pins still measure Direction predicted in writing before running (mutation:
Measured: exactly those 8. All five re-anchored pins are RED, so none of them is vacuous. The sixth is the one worth dwelling on: Green control on the same tree immediately before the mutation: Mutation and restore evidence
Mutation proved on disk before any result was read: The Restore proved three ways, never by an exit code:
Harness as before: ⛔ No code changed — this leg is measurement only, run on the pushed head Generated by Claude Code Generated by Claude Code |
Fixes #15030
Maintainer-ruled partial revert of unreleased code (the v18 org-ownership decision (PR #14976), D8).
priority:p1/target:v17— must land before the 17.3 tag.Clause-②: noRe-derived from this diff, not inherited:
orgScopeis aprivatemethod, the diff adds no exported symbol, no payload key, no spec surface and no new accepted shape. It narrows an accept set back to exactly what@objectstack/account@17.2.0already ships.What was reverted
BusinessUnitGraphService.orgScopeinpackages/plugins/plugin-sharing/src/business-unit-graph.tsis restored to the strictorganization_id = RULE_ORGequality, and the docblock arguing for the NULL-inclusive unit screen is deleted.The restored method body is byte-identical to its form before
e560b4d51(PR #14949) — verified rather than retyped:e560b4d51form as{ ...filter, organization_id: this.organizationId }. The actual parent commit spells the guard the other way round (if (this.organizationId) return {…}; return filter;) while returning exactly that expression. It was plain strict equality, so the revert is a true revert; this PR restores the parent's spelling, not the card's paraphrase.What was deliberately KEPT
memberScope— byte-identical toorigin/main(verified by diffing the method body againstorigin/main). Bothsys_business_unit_memberreads still carry a strict equality.SharingRuleService.warnOnEmptyUnitExpansion— fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened #14949's observability half, untouched. It is what keeps the remaining Sharing rules with a business-unit recipient silently grant nothing when the unit row has organization_id = NULL #14547 symptom loud instead of silent, and it matters more after this revert, not less.SqlDriver.applyTenantScope— not touched.packages/spec,content/docs/releases/**— not touched.Why
memberScopeis still load-bearing with the unit screen strict: a strict unit screen narrows which units are reachable, but other organizations' member rows sit on org-stamped (visible) units too.memberScopeis the only thing that answers there.The two grep counts, reported separately
git grep -n 'organization_id: null' -- packages/plugins/plugin-sharing/src*.test.ts)business-unit-graph.tssharing-rule-service.ts:They are not mine to revert, proved by measurement rather than by blame (the checkout was shallow, so blame attributed them to a boundary commit):
@objectstack/account@17.2.0that file contains the same 4 hits;git show e560b4d51 -- sharing-rule-service.ts | grep '^[+-].*organization_id: null'returns nothing — fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened #14949 added and removed no NULL-arm line in that file.:556isSharingRuleService.adminOrgScope(#7676), which the deleted docblock itself cited as prior art. It ships in 17.2.0, so it is outside "the screen added after 17.2.0" that this card retires. I did not touch it, and I did not touch the test fixtures either. The acceptance is met in the sense that matters — zero non-test hits in the reverted file.Ancestry check, with its positive control
Both legs run and quoted; the control is what makes "not an ancestor" a reading rather than an artifact of a truncated graph.
git merge-base --is-ancestor X @objectstack/account@17.2.02f24c8905898ea3443ffTag commit
e7d2cc67fdef7fee9d2c6d65d7363fe1c78ce6a4. Also confirmed:e560b4d51(the code being reverted) is not an ancestor of the tag either, i.e. it really was added after 17.2.0.merge-basesilently answered "not an ancestor" for the tag itself. I deepened to 7191 commits before taking any ancestry reading; the control leg above is what proves the deepened graph actually traverses.Tests
Baseline before any edit: 32 files / 798 tests green. Final: 32 files / 801 tests green (
pnpm --filter @objectstack/plugin-sharing test).The card's constraint 2 ("keep every test that pins
memberScope… not touched at all") is not satisfiable as written, and I am flagging it rather than quietly working around it.With only the source revert applied and no test file edited, 13 tests reddened. Five of them are
memberScopepins:Mechanism: both
expandUnitMembersandexpandUsersgate onseedIsUsable(), which reads throughorgScope. Those five pins were written on a seeded (org-less) unit — a fixture only #14949's widening made reachable. With the unit screen strict again, the unit screen answers first and the member read never runs.This is a fixture dependency, not a semantic inseparability. The two halves are separable, and here is the evidence: the pre-existing pin
an org-scoped rule never reaches another org's MEMBER of a unit it can see— which anchors on an org-stamped unit — stayed green through the bare revert.memberScopeis still strict, still called from both reads, still enforced.Resolution: those five pins are re-anchored from a seeded unit onto an org-stamped one. Same assertions, same security property, a route that exists in 17.2.0. This is strictly stronger than leaving them: on a seeded unit they would now pass with
memberScopedeleted, because the unit screen would answer first — they would have become phantom pins. (One of them,an org-LESS membership row is NOT a member of an org-scoped ruleinbusiness-unit-graph.test.ts, was already phantom under the bare revert: it stayed green while measuring nothing.)⛔ I did not skip, disable or quarantine anything. Test count went 798 → 801.
Tests rewritten to pin the restored behaviour
Per scope item 3, the cases that pinned the NULL-inclusive unit screen now pin the 17.2.0 behaviour and name #14547 as the defect they reproduce, with an explicit note that they assert a reproduction, not a fix, and that the v18 org-ownership decision (PR #14976), C1 is where it gets fixed. Added controls so "strict" cannot be confused with "refuses everything":
an org-stamped unit IS visible — the screen is strict, not broken, andthe SAME rule against an org-stamped tree grants normally.Ablation — direction predicted in writing before running
Predicted: restoring the NULL arm reddens exactly 7 — the 5 unit-screen pins plus the 2 rewritten
recipient-widthdefect cases — and thememberScopepins do not move, because they are now anchored on org-stamped units.Measured: exactly those 7.
The
memberScopepins stayed green under the mutation — which is the point of the re-anchoring: the two screens are now pinned independently of each other.Mutation proved on disk by literal-text counts before reading any result:
strict=0(want 0),nullarm=1(want 1), and blobcfef2985…differing from HEAD blob5402c265…. No rebuild was needed — both test files import./business-unit-graph.js, a relative same-package specifier resolved from source, so nodist/sits between the mutation and the assertion.Restore proved three ways (not by an exit code): blob hash back to
5402c265fef80051fab31988da81aa1494e1b7ddand equal togit rev-parse HEAD:PATH;git diff HEAD0 bytes; literal counts back tostrict=1 / nullarm=0. The script carriedtrap restore EXIT INT TERMwith an absoluteREPO_ROOTpath, andgit checkout HEAD -- PATH(never a baregit checkout -- PATH, which restores from a possibly-mutated index).Gates
Derived with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, re-derived after the second commit (identical family — same 4 paths vs merge base8a96e666b), and the union re-run on the final head2f24c89058. Every exit code captured by redirect-then-read, never across a pipe.41 of 42 green (exit 0). 1 NOT MEASURED.
Plus
pnpm --filter @objectstack/plugin-sharing typecheckgreen — includingcheck:test-typecheck, which compiles the test layer undertsconfig.test.json, so the two rewritten test files were genuinely measured, not silently excluded:Three gates first exited 3, and I built the closure so they would actually measure
All three initially reported PREREQUISITE NOT MET because no
dist/existed. Rather than record them as NOT MEASURED, I ran the closure build CI runs before this step (pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*', 9m05s, exit 0) and re-ran them. All three are green:pnpm check:type-check-debt— exit 0. This is the one worth calling out: a sibling card was reddened in CI today by exactly this gate after its author honestly recorded exit 3 locally. Its verdict here:pnpm check:i18n— exit 0.check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys).pnpm check:dual-build-cjs-loads— exit 0.provenance — entries/packages/cjsFiles/probes: this run 102/66/610/1 · floors 90/58/520/1.NOT MEASURED — neither green nor red, quoting the gate's own verdict
node scripts/check-test-completeness.mjs— exit 3. It needs a savedturbo run testlog, which a local family run does not produce; the gate itself prescribes this record:I cannot move this one from here — it is a property of how the suite is invoked, not of the tree. CI runs it with the log it needs.
One footnote on invocation, not on the tree: the bare
node scripts/pm/check-half-states.mjshit my own 300 s timeout (exit 124) — it re-execs for the proxy and is network-bound. That is not a gate verdict. Its packaged formpnpm check:pm-half-statesran green (exit 0), so the family is measured.--reporter=basic, which does not exist in vitest 4. It exited 1 in 1 s having run nothing. That is the bad-invocation class, not a red suite, so it is recorded as NOT MEASURED and was re-run with the default reporter.Deviation from the card's verbatim wording — disclosed, not smoothed over
The card's changeset wording (and my first three commits) cite
ADR-0131. That ADR does not exist onmain:docs/adr/stops at0130(control: 134 files, so the directory read works and the zero is a reading), and PR #14976, which would author it, is open and draft.scripts/check-adr-anchors.mjsreddenedLint & Repo Gateson head2676f0f74, correctly — it treats an unresolvable ADR id as a citation squat: whoever later writes a real ADR-0131 would retroactively falsify all of those citations at once (#6634, where one number accumulated 77).There is no legitimate forward-reference form. The gate's only citation exemption is
CROSS_REPO_QUALIFIERS = {objectui, object-ui, cloud}— a sibling repo qualifier, not a PR — soADR-0131 (PR #14976)would still be caught, and the allowlist is shrink-only and documented to stay empty.⇒ All 22 citations now read "the v18 org-ownership decision (PR #14976)", keeping the decision letters (
D8/C1) that carry the actual meaning. Substance unchanged; the ADR id can be restored once #14976 lands.⛔ Deliberately not done: no allowlist entry, and no
docs/adr/0131-*.mdcreated here — that file is PR #14976's anddocs/adr/**is a governed surface.Issue/PR references (#14547, #14949, #10103, #13564) are untouched — they resolve.
check-adr-anchorsis now green:Its two residual notes (
ADR-0024 D4/D5.2inplugin-auth, the allowlisted sharedADR-0057number inobjectql/plugin-security) are pre-existing and in files this PR never touches..changeset/bu-tenant-screen-relanding.md— #14949's own changeset — is corrected, not reverted.It is still on
main, unreleased and unconsumed, and it announces as shipped fact the very half this PR reverts: "orgScopenow applies the platform's null-inclusive screen", under the headline "Fix: a sharing rule with a business-unit recipient granted nothing when the unit came from seed data". Both changesets compile into the same 17.3 release notes, so leaving it would ship two contradictorypatchentries for one package and tell users a tenancy fix landed that does not.Corrected surgically: the headline and the
orgScopeparagraph now say the unit half was reverted and the seed-data symptom remains as in 17.2.0. The member-screen and warn paragraphs are kept — those do ship — with one sentence rewritten, since the leak they close does not depend on the unit screen having been widened.This is a judgement call and I am flagging it for ratification rather than burying it. It touches no code from #14949.
Over-revert check — this is not
git revert e560b4d51#14949 touched 7 files; this PR touches 3 of them plus its own changeset. Measured:
business-unit-graph.tsorgScopeonly)business-unit-graph.test.tsrecipient-width.test.ts.changeset/bu-tenant-screen-relanding.mdsharing-rule-service.tssharing-rule.test.tscontent/docs/permissions/system-context.mdxCoupling checked before finishing, as asked:
sharing-rule-service.tsnever referencesorgScope— it consumes the graph service only through the publicexpandUnitMembers/expandUsers, whose contract already includes "may expand to nobody" (that is exactly whatwarnOnEmptyUnitExpansionexists for; its own docblock says it "covers the case the screens deliberately leave empty"). So there is no dependency on NULL-inclusiveness to break, and none was found. Empirically confirmed:sharing-rule.test.tsis among the 32 files in the green suite.Docs and the census gate
No doc prose to revert — confirmed independently of the drift bot (which reports nothing while warning that is not a clean bill of health): no hand-written page describes the NULL-inclusive unit screen, and this PR touches no file under
content/docs/.check:system-context-censusdid NOT redden. The predicted line rot did not materialise — measured green on every head, including the final one:So neither remediation path was taken, because neither was needed: no
--fix(there was no line rot to repair) and no conflict resolution (git statusclean, not in MERGE state — verified). No hand-written prose was dropped, because no prose file was touched at all.What I declined to do, and why
organization_id: nullhits insharing-rule-service.ts, even though the card's acceptance grep reads as if they should be zero. They ship in 17.2.0 and fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened #14949 did not author them — that is falsifier Implement ObjectStack protocol specification with Zod schemas and TypeScript interfaces #3, and the instruction is to report it, not to work around it by deleting someone else's code.organization_id: nulloutside the two files in scope (backfill-sys-record-share-organizations.test.ts,bu-tree-recompute.test.ts,sharing-rule.test.ts,sharing-service.test.ts,record-share-organization-stamp.test.ts,rule-criteria-org-scope.test.tsare untouched).closed/state_reason: completed, closed by fix(plugin-sharing): a seeded business unit is a usable sharing-rule recipient, and its members are tenant-screened #14949, whose unit half this PR reverts. Its tracker state therefore no longer matches the 17.x runtime, where the symptom stands. The tests and the source docblock say so plainly rather than asserting a status; whether the issue is reopened is a maintainer's call, not mine.SqlDriver.applyTenantScope,packages/spec, orcontent/docs/releases/**, and did not skip, disable or quarantine any test.🤖 Generated with Claude Code
https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
Generated by Claude Code