revert(plugin-sharing): drop the NULL-inclusive business-unit screen 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

Author: claude

.changeset/revert-null-inclusive-business-unit-screen.md

@@ -0,0 +1,30 @@
+---
+"@objectstack/plugin-sharing": patch
+---
+
+revert(plugin-sharing): drop the NULL-inclusive business-unit screen added after 17.2.0
+
+17.3 does not ship the NULL-inclusive business-unit screen added after 17.2.0;
+#14547 remains as in 17.2.0 and is fixed structurally in v18 (ADR-0131 C1: the
+Default Organization exists before application seed datasets load, and the seed
+loader stamps `sys_business_unit` seeds).
+
+`BusinessUnitGraphService.orgScope` briefly read
+`$or: [{ organization_id: <rule org> }, { organization_id: null }]` so that an
+org-stamped sharing rule could name a seeded (org-less) `sys_business_unit`
+row. It is restored to the strict `organization_id = <rule org>` equality
+17.2.0 ships. That shape re-implemented, a second time and in a second place,
+the predicate `SqlDriver.applyTenantScope` already owns — the duplication
+ADR-0131 exists to retire (#10103 cause 1) — and it had not been released, so
+reverting costs nothing while shipping it would have owed v18 a breaking change
+and a migration.
+
+The other half of the same change is KEPT and is not touched:
+`BusinessUnitGraphService.memberScope` still screens both
+`sys_business_unit_member` reads with a strict equality. Those reads previously
+carried no organization predicate at all, so an org-stamped rule reaching any
+visible unit collected every tenant's membership rows hanging off it; 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.
+`SharingRuleService.warnOnEmptyUnitExpansion` is also kept: it is what keeps the
+remaining #14547 symptom loud instead of silent.

packages/plugins/plugin-sharing/src/business-unit-graph.test.ts

@@ -3,43 +3,47 @@
 /**
  * BusinessUnitGraphService — the TWO tenant screens, pinned as a pair.
  *
- * ## What this file used to say, and why it changed
+ * ## Both screens are STRICT equalities, and that is the shipped posture
  *
- * Until #14547 `orgScope()` AND-composed a strict `organization_id = <rule
- * org>` equality onto the UNIT read. A unit written with no organization at
- * all (a seeded / file-layer / bootstrap row — a seed cannot know the org id
- * the runtime mints at boot) therefore matched nothing, the seed check failed,
- * and BOTH widths expanded to zero members. #3807 had already fixed exactly
- * that on the approvals side; this file recorded the sharing side's divergence
- * as deliberate on the grounds that it was unreachable, because every
- * materialized `sys_sharing_rule` row carried `organization_id = null`.
+ *   - the UNIT screen (`orgScope`) AND-composes `organization_id = <rule org>`
+ *     onto the `sys_business_unit` read. A unit written with no organization
+ *     at all (a seeded / file-layer / bootstrap row — a seed cannot know the
+ *     org id the runtime mints at boot) therefore matches nothing for an
+ *     org-stamped rule, the seed check fails, and BOTH widths expand to zero
+ *     members;
+ *   - the MEMBER screen (`memberScope`) AND-composes the same equality onto
+ *     the `sys_business_unit_member` reads. Both member reads used to carry no
+ *     organization predicate at all — completely unscoped by organization —
+ *     which is the cross-tenant hole #14949 closed and this file still pins.
  *
- * It was reachable. #14547 is the external report: an org admin creating a
- * rule at runtime gets an org-stamped rule, the seeded unit carries none, and
- * the rule is accepted, stays active, materialises zero `sys_record_share`
- * rows and logs nothing. The `[divergence]` test that pinned the old posture
- * is gone — replaced, not merely flipped, because an assertion that keeps
- * passing while the mechanism under it changes is worse than no assertion.
+ * ## #14547 is OPEN, and these tests reproduce it rather than fix it
  *
- * ## The pair this file now pins
+ * #14547 is the external report of the unit half: an org admin creating a rule
+ * at runtime gets an org-stamped rule, the seeded unit carries no
+ * organization, and the rule is accepted, stays active and materialises zero
+ * `sys_record_share` rows. #14949 briefly closed it by giving the UNIT screen
+ * the platform's NULL-inclusive `$or` arm. That was reverted before 17.3 was
+ * cut (ADR-0131 D8): it re-implements the predicate
+ * `SqlDriver.applyTenantScope` already owns, a second time in a second place —
+ * the duplication ADR-0131 exists to retire — and it had not shipped.
  *
- * The fix is ASYMMETRIC and both halves have to be pinned, because each one
- * alone is a defect:
+ * So 17.3 behaves exactly as 17.2.0 does here, and the cases below name
+ * **#14547** as the open defect they reproduce. It is fixed structurally on
+ * the v18 line by ADR-0131 C1 (the Default Organization exists before
+ * application seed datasets load, and the seed loader stamps
+ * `sys_business_unit` seeds), so the row this screen reads carries an
+ * organization and no screen has to special-case it. What #14949 left behind —
+ * and what keeps the 17.x symptom LOUD rather than silent — is
+ * `SharingRuleService.warnOnEmptyUnitExpansion`.
  *
- *   - the UNIT screen (`orgScope`) is NULL-INCLUSIVE — the platform's own
- *     `(organization_id = ? OR organization_id IS NULL)`, the predicate
- *     `SqlDriver.applyTenantScope` writes and `plugin-approvals` already
- *     applies to these very rows;
- *   - the MEMBER screen (`memberScope`) is STRICT. Both member reads used to
- *     carry no organization predicate at all, and the strict unit screen was
- *     the only thing holding an org-stamped rule away from that unscoped
- *     query. Widening the unit screen ALONE turns a silent under-grant into a
- *     silent CROSS-TENANT OVER-GRANT, since a seeded unit id exists
- *     identically in every tenant.
+ * ## Why the member screen is pinned on ORG-STAMPED units
  *
- * So the security half is pinned separately from the functional half below: a
- * change that expands the right members while also expanding another
- * organization's members satisfies the functional pin completely.
+ * ⚠️ The member pins below deliberately anchor on a unit stamped with the
+ * rule's own organization. With the unit screen strict, a seeded (org-less)
+ * unit is invisible to an org-stamped rule, so a member pin written on one
+ * would pass no matter what `memberScope` did — the unit screen would answer
+ * first and the assertion would never reach the member read. Anchoring on a
+ * visible unit is what keeps these assertions about the MEMBER screen.
  */
 
 import { describe, it, expect } from 'vitest';
@@ -179,33 +183,36 @@ describe('BusinessUnitGraphService — the two widths are actually two widths (#
   });
 
   it('the narrow width is org-predicated exactly like the wide one', async () => {
-    // [#14547] Same fixture, new mechanism — and the mechanism is spelled out
-    // because the ASSERTION did not move. `DIV_MEMBERS` carry no organization,
-    // so before #14547 this returned `[]` because the strict UNIT screen hid
-    // the seeded unit, and after it returns `[]` because the strict MEMBER
-    // screen refuses membership rows of unknown tenancy. An unchanged
-    // expectation over a changed cause is exactly the kind of pin that stops
-    // guarding anything, so the two causes are separated below: the unit is
-    // now visible (`descendants` sees the whole seeded tree), and it is the
-    // members that are refused.
+    // [#14547] `DIV_UNITS` are seeded (org-less) and `DIV_MEMBERS` carry no
+    // organization either, so BOTH strict screens refuse this fixture. The
+    // unit screen answers FIRST and is the one being pinned here: the subtree
+    // walk cannot see the seeded tree at all, which is why the expansion is
+    // empty. The member screen's own refusal is pinned separately, on a unit
+    // the rule can actually see, in the MEMBER screen block below.
     const g = new BusinessUnitGraphService({
       engine: makeEngine(DIV_UNITS, DIV_MEMBERS),
       organizationId: 'org_a',
     });
     expect(await g.expandUnitMembers('bu_div')).toEqual([]);
-    expect((await g.descendants('bu_div')).sort()).toEqual(['bu_dept', 'bu_div', 'bu_office']);
+    expect(await g.descendants('bu_div')).toEqual([]);
   });
 
-  it('[#14547] both widths reach org-stamped members of a SEEDED unit tree', async () => {
-    // The one change that flips the outcome: the membership rows are stamped,
-    // exactly as a REST/session write stamps them. The units stay seeded.
+  it('[#14547] NEITHER width reaches a SEEDED unit tree, even with stamped members', async () => {
+    // ⚠️ This reproduces the OPEN defect #14547 — it does not assert a fix.
+    // The membership rows are stamped exactly as a REST/session write stamps
+    // them, so the MEMBER screen would admit every one of them; the units stay
+    // seeded, so the strict UNIT screen hides the tree before the member read
+    // is ever issued. Both widths therefore answer nobody, which is precisely
+    // the 17.2.0 symptom: rule accepted, active, zero shares.
+    // Fixed structurally in v18 by ADR-0131 C1 (seed loader stamps
+    // `sys_business_unit`), NOT by widening this screen.
     const members: MemberRow[] = DIV_MEMBERS.map((m) => ({ ...m, organization_id: 'org_a' }));
     const g = new BusinessUnitGraphService({
       engine: makeEngine(DIV_UNITS, members),
       organizationId: 'org_a',
     });
-    expect(await g.expandUnitMembers('bu_div')).toEqual(['u_div']);
-    expect((await g.expandUsers('bu_div')).sort()).toEqual(['u_dept', 'u_div', 'u_office']);
+    expect(await g.expandUnitMembers('bu_div')).toEqual([]);
+    expect(await g.expandUsers('bu_div')).toEqual([]);
   });
 
   it('the two widths do NOT share a cache entry for the same unit id', async () => {
@@ -280,54 +287,83 @@ describe('BusinessUnitGraphService — org scoping (#3807)', () => {
 });
 
 /**
- * [#14547] The UNIT screen is null-inclusive — the divergence from
- * `plugin-approvals` (#3807) is CLOSED.
+ * [#14547] The UNIT screen is STRICT — the divergence from `plugin-approvals`
+ * (#3807) is OPEN, and 17.3 ships it exactly as 17.2.0 did.
+ *
+ * ⚠️ Every case in this block reproduces the OPEN defect **#14547**. None of
+ * them asserts a fix. #14949 closed the defect by giving this screen the
+ * platform's NULL-inclusive arm; that was reverted before the 17.3 tag
+ * (ADR-0131 D8) because it wrote `SqlDriver.applyTenantScope`'s own predicate
+ * a second time, in a second place, and had not shipped. The structural fix is
+ * ADR-0131 C1 on the v18 line: the Default Organization exists before
+ * application seed datasets load and the seed loader stamps
+ * `sys_business_unit` seeds, so these rows arrive already carrying an
+ * organization and this screen needs no NULL arm to find them.
  *
- * The `[divergence]` test that used to live in the block above pinned the
- * opposite posture on the grounds that it could not fire. It fired: the
- * external report is an org admin creating a rule at runtime against a unit
- * the app seeded.
+ * ⛔ If a future change makes any assertion here fail, that is a NULL arm
+ * coming back — re-read ADR-0131 D8 before "fixing" the test.
  */
 describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => {
   const STAMPED_MEMBERS: MemberRow[] = SEEDED_MEMBERS.map((m) => ({
     ...m,
     organization_id: 'org_a',
   }));
 
-  it('an org-scoped rule DOES see an env-wide (null-org) seeded unit', async () => {
+  it('an org-scoped rule does NOT see an env-wide (null-org) seeded unit', async () => {
+    // The reported reproduction, pinned as the behaviour 17.x ships: the
+    // members are stamped and would pass the member screen, so the empty
+    // answer is the UNIT screen's alone.
     const g = new BusinessUnitGraphService({
       engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS),
       organizationId: 'org_a',
     });
-    expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']);
-    expect(await g.expandUnitMembers('bu_root')).toEqual(['u_root']);
+    expect(await g.expandUsers('bu_root')).toEqual([]);
+    expect(await g.expandUnitMembers('bu_root')).toEqual([]);
   });
 
-  it('the seed check and the subtree walk BOTH admit the seeded rows', async () => {
+  it('the seed check and the subtree walk BOTH refuse the seeded rows', async () => {
     // `seedIsUsable` and the `descendants` BFS are two separate reads through
-    // the same screen; a widening applied to one and not the other would still
-    // answer `[]` for the subtree width.
+    // the same screen. Pinning the BFS separately keeps a widening applied to
+    // only one of them from passing as "unchanged".
     const g = new BusinessUnitGraphService({
       engine: makeEngine(SEEDED_UNITS, STAMPED_MEMBERS),
       organizationId: 'org_a',
     });
-    expect((await g.descendants('bu_root')).sort()).toEqual(['bu_child', 'bu_root']);
+    expect(await g.descendants('bu_root')).toEqual([]);
   });
 
-  it('`headOf` resolves the manager of a seeded unit too', async () => {
+  it('`headOf` does not resolve the manager of a seeded unit either', async () => {
+    // The third read through `orgScope`, pinned so the screen cannot be
+    // widened at one call site while the other two stay strict.
     const units: UnitRow[] = [
       { id: 'bu_root', organization_id: null, active: true, manager_user_id: 'u_head' },
     ];
     const g = new BusinessUnitGraphService({
       engine: makeEngine(units, []),
       organizationId: 'org_a',
     });
+    expect(await g.headOf('bu_root')).toBeNull();
+  });
+
+  it('an org-stamped unit IS visible — the screen is strict, not broken', async () => {
+    // The control that separates "strict" from "refuses everything". Same
+    // fixture shape as the seeded case above; only the unit's organization
+    // moves, and that single change flips every read to visible.
+    const units: UnitRow[] = [
+      { id: 'bu_root', organization_id: 'org_a', active: true, manager_user_id: 'u_head' },
+      { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true },
+    ];
+    const g = new BusinessUnitGraphService({
+      engine: makeEngine(units, STAMPED_MEMBERS),
+      organizationId: 'org_a',
+    });
+    expect((await g.expandUsers('bu_root')).sort()).toEqual(['u_child', 'u_root']);
+    expect(await g.expandUnitMembers('bu_root')).toEqual(['u_root']);
+    expect((await g.descendants('bu_root')).sort()).toEqual(['bu_child', 'bu_root']);
     expect(await g.headOf('bu_root')).toBe('u_head');
   });
 
-  it('ONLY the NULL arm widened — another org’s unit is still invisible', async () => {
-    // The control that separates "null-inclusive" from "unscoped". Without it
-    // a screen that had simply been deleted would pass every assertion above.
+  it('another org’s unit is invisible too', async () => {
     const units: UnitRow[] = [
       { id: 'bu_root', organization_id: 'org_b', active: true },
       { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_b', active: true },
@@ -342,10 +378,14 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => {
     expect(await g.headOf('bu_root')).toBeNull();
   });
 
-  it('an INACTIVE seeded unit still contributes nobody', async () => {
-    const units: UnitRow[] = SEEDED_UNITS.map((u) =>
-      u.id === 'bu_root' ? { ...u, active: false } : u,
-    );
+  it('an INACTIVE org-stamped unit still contributes nobody', async () => {
+    // `active: false` is a hard filter independent of either screen — pinned
+    // on a VISIBLE unit so the empty answer is the active flag's doing and
+    // not the tenant screen answering first.
+    const units: UnitRow[] = [
+      { id: 'bu_root', organization_id: 'org_a', active: false },
+      { id: 'bu_child', parent_business_unit_id: 'bu_root', organization_id: 'org_a', active: true },
+    ];
     const g = new BusinessUnitGraphService({
       engine: makeEngine(units, STAMPED_MEMBERS),
       organizationId: 'org_a',
@@ -356,23 +396,32 @@ describe('BusinessUnitGraphService — the UNIT screen (#14547)', () => {
 });
 
 /**
- * [#14547] The MEMBER screen is STRICT — the leak the unit widening would
- * otherwise have opened.
+ * [#14949, KEPT] The MEMBER screen is STRICT.
+ *
+ * ⚠️ This is the SECURITY half of #14949 and it is NOT part of the ADR-0131 D8
+ * revert. Both `sys_business_unit_member` reads used to carry no organization
+ * predicate whatever — completely unscoped by organization — so an org-stamped
+ * rule reaching any visible unit collected every tenant's membership rows off
+ * it. `memberScope` closed that, and it stays closed.
  *
- * ⚠️ These are the SECURITY half and they are pinned apart from the functional
- * half on purpose: a change that expands the right members while also
- * expanding another organization's members passes every assertion in the block
- * above.
+ * ⚠️ The anchor unit here is stamped with the rule's OWN organization, not
+ * seeded. That is load-bearing: with the unit screen strict again, a seeded
+ * (org-less) unit is invisible to an org-stamped rule, so a member assertion
+ * written on one would be answered by the UNIT screen before the member read
+ * ever ran — it would pass with `memberScope` deleted. Anchoring on a visible
+ * unit is what keeps every assertion below a pin on the MEMBER screen.
  */
 describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => {
   /**
-   * One SEEDED unit id with two tenants' memberships hanging off it — the
-   * shape that exists on every deployment whose org chart came from a seed,
-   * and the one the widened unit screen makes reachable.
+   * One unit tree the rule's organization owns, with two tenants' membership
+   * rows hanging off it — the shape an unscoped member read turns into a
+   * cross-tenant over-grant. `sys_business_unit_member` is not
+   * organization-stamped on every write path, so mixed tenancy on one visible
+   * unit is a real deployment shape, not a contrived one.
    */
   const SHARED_SEED_UNITS: UnitRow[] = [
-    { id: 'bu_market', organization_id: null, active: true },
-    { id: 'bu_market_west', parent_business_unit_id: 'bu_market', organization_id: null, active: true },
+    { id: 'bu_market', organization_id: 'org_a', active: true },
+    { id: 'bu_market_west', parent_business_unit_id: 'bu_market', organization_id: 'org_a', active: true },
   ];
   const TWO_TENANT_MEMBERS: MemberRow[] = [
     { business_unit_id: 'bu_market', user_id: 'u_a', organization_id: 'org_a' },
@@ -403,9 +452,9 @@ describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => {
   it('an org-LESS membership row is NOT a member of an org-scoped rule', async () => {
     // Unknown tenancy, not platform-global: `sys_business_unit_member` is not
     // organization-stamped by seed replay or by an elevated system write, so a
-    // NULL here cannot be read the way a NULL on the UNIT row is read. The
-    // grant fails closed, and `SharingRuleService` warns rather than staying
-    // silent about it.
+    // NULL here fails CLOSED. The anchor unit is visible to the rule, so this
+    // empty answer is the member screen's own — `SharingRuleService` warns
+    // rather than staying silent about it.
     const members: MemberRow[] = [
       { business_unit_id: 'bu_market', user_id: 'u_seeded', organization_id: null },
     ];
@@ -418,8 +467,9 @@ describe('BusinessUnitGraphService — the MEMBER screen (#14547)', () => {
   });
 
   it('an org-LESS rule is unmoved — both screens stay no-ops', async () => {
-    // The dominant shape today (declared rules bootstrap org-less). #14547
-    // must not change what they expand to, in either direction.
+    // The dominant shape today (declared rules bootstrap org-less). Neither
+    // #14949 nor the ADR-0131 D8 revert of its unit half may change what a
+    // platform-global rule expands to, in either direction.
     const g = new BusinessUnitGraphService({
       engine: makeEngine(SHARED_SEED_UNITS, TWO_TENANT_MEMBERS),
       organizationId: null,