fix(plugin-approvals): screen expanded business-unit members to the directory organization (#14946)

[zhuangjianguo]

Closes #14946

What was wrong

ApprovalService.expandBusinessUnitUsers (packages/plugins/plugin-approvals/src/approval-service.ts) screens the sys_business_unit rows through businessUnitOrgScope — null-inclusive since #3807, because a seeded unit carries organization_id = null by construction and is admitted on purpose — and then read sys_business_unit_member with no organization predicate at all, under SYSTEM_CTX, which carries no tenant either. A seeded unit id exists identically in every tenant, so a department:UNIT_ID approver on tenant A's request resolved the shared unit and then collected every tenant's membership rows hanging off it: approval authority over A's record, routed to B's users.

Measured red before the fix, on this branch with approval-service.ts byte-identical to origin/main (commit 6fc3d1009):

[PROBE B1] org_a request, seeded unit, org_a+org_b members -> pending_approvers = ["u_a","u_b","u_a_child","u_b_child"]
[PROBE B5] sys_business_unit_member where = {"business_unit_id":{"$in":["bu_seeded","bu_seeded_child"]}}
Tests  5 failed | 1 passed (6)     EXIT=1

The fix

The member read now goes through a new private businessUnitMemberScope, a strict organization_id equality against the same directory organization the unit screen uses (ADR-0105 D9 directoryOrg, at both call sites — the static department type and expression / resolveAs: 'department'). One read for the whole subtree, as before.

After the fix, same probes at 16dbdf55b:

[PROBE B1] org_a request, seeded unit, org_a+org_b members -> pending_approvers = ["u_a","u_a_child"]
[PROBE B5] sys_business_unit_member where = {"business_unit_id":{"$in":["bu_seeded","bu_seeded_child"]},"organization_id":"org_a"}
Tests  6 passed (6)     EXIT=0

Why strict, and not a copy of the unit screen's $or null arm (the A4 reading)

The PM's dispatch asked whether a seeded member row is the same case as a seeded unit row, and whether the object even carries the column. Measured on this tree:

• sys_business_unit_member declares no organization_id (packages/platform-objects/src/identity/sys-business-unit-member.object.ts). The column is injected by applySystemFields (packages/objectql/src/registry.ts; packages/spec/src/data/injected-system-columns.ts, TENANT_SCOPE_COLUMN = 'organization_id'), and the committed tenancy census lists the object with "reach": "in", "tenantField": "organization_id" (scripts/platform-object-tenancy-census.json).
• Who fills it: REST / session writes do (SqlDriver.injectTenantOnInsert). Seed replay does not (packages/metadata-protocol/src/seed-loader.ts withholds its fallbackOrgId from every sys_ / cloud_ / ai_ object). Elevated system-context writes do not (sys_business_unit_member is unclassified in PLATFORM_OBJECT_TENANCY, tracked in sys_business_unit_member is unadjudicated in PLATFORM_OBJECT_TENANCY, so seed-replayed and system-written membership rows land organization-less #14570, open).

So a NULL on a membership row means unknown tenancy, not "platform-global" — the opposite of what a NULL on a seeded unit means — and routing must fail closed on it. This is the same ruling plugin-sharing's memberScope applies to the same rows (landed under #14547 / #14949 and deliberately kept by the #15078 revert, which dropped only the unit half), and the same posture this file already takes for sys_team_member (#10547) and sys_user_position. The sibling's answer and the right answer here agree; nothing to report as a divergence.

⚠️ Not copied from the sibling: the sibling's unit screen was reverted to a strict equality before 17.3 (ADR-0131 D8). The approvals unit screen stays null-inclusive here — it is released 17.x behaviour, ADR-0131 D14 says no 17.x card narrows or removes an arm, and it is out of this card's scope. This PR changes only the member read.

Declared cost (confirmed, as the card asked, not assumed)

On a deployment whose membership rows (not merely its units) were seeded or written under a system context, a department approver on a request that carries an organization now expands to nobody. That is loud, not silent: the graph-type fallback already warns approver 'department:UNIT_ID' expanded to nobody (#3807), the slot falls to the literal, and onEmptyApprovers governs the request as for any unstaffed target (admin_rescue by default). Pinned as B3. The repair on such a deployment is to stamp the membership rows (#14570), never to widen this screen.

Measured against the stock example: examples/app-showcase/src/data/seed/index.ts seeds business units only — its own comment says memberships "stay runtime admin actions", i.e. the session write path, which stamps the column. The showcase's by_department approver is therefore unaffected. The packages/qa/dogfood suites that insert org-less membership rows exercise sharing, not department approvers (no department approver in packages/qa/dogfood/test).

Fixture triage in the existing suite

Four approval-service.test.ts fixtures carried org-less membership rows on org-stamped or seeded units under a t1 context; each was re-judged rather than batch-edited. All four represent session-written rows, so they now carry organization_id: 't1' (and 't2' for the other tenant's row in the subtree case) and their assertions are unchanged — including the #3807 pins, which stay pins on the unit screen. The org-less-membership case is pinned on its own (B3) as the declared cost.

Tests and checks (exit codes captured before any pipe; verdict lines quoted from the gates)

At 16dbdf55b unless stated:

• pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/business-unit-member-org-screen.test.ts → Tests 6 passed (6), GREEN_EXIT=0 (red at 6fc3d1009: 5 failed | 1 passed, EXIT=1, above)
• pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 → Test Files 38 passed (38), Tests 671 passed (671), SUITE_EXIT=0
• pnpm --filter @objectstack/plugin-approvals typecheck → check:test-typecheck: OK, TYPECHECK_EXIT=0; tsc --listFiles shows the new test file in the tsconfig.test.json program and approval-service.ts in the main program
• pnpm --filter '@objectstack/plugin-approvals^...' build (dependency closure) and pnpm --filter @objectstack/plugin-approvals build → command-exit 0
• node scripts/pm/dispatch-gates.mjs (derived from the working tree, not a hand list): 93 families at 16dbdf55b (the mechanical content/docs/** and scripts/** touches grew the list from the 56 the source-only diff owed); 90 exit 0, 3 exit 3 (PREREQUISITE NOT MET, listed below). The first-pass reds at fe370ce48 — check-system-context-census (line rot, repaired with --fix), check:engine-double-contract (ledger, --write), check:objectql-double-limit (the fake engine's find read this and paged by default) — were repaired in 16dbdf55b and are green in this run. check:skill-examples was re-run after building @objectstack/client-react's closure: 257 prose examples type-check across 3 surface(s).
• Targeted eslint (--no-inline-config --format json) on the three touched .ts files → 3 files judged, 0 errors, 0 warnings, exit 0. Declared narrowing of pnpm lint: the repo's single eslint.config.mjs never enables type-aware linting (its own comment at line 328: no parserOptions.project, no typed rules), so this diff cannot move any untouched file's verdict.
• pnpm check:nul-bytes → OK (scanned 7665 text file(s) ... no raw ASCII control bytes); unpiped control-character scan of the touched files: no hits.

Not measured locally, declared: check:dual-build-cjs-loads, check:i18n, check:type-check-debt each exit 3 — PREREQUISITE NOT MET (they read the built output of the whole workspace closure, which this worktree does not have). CI builds that closure before them. None of the three reads anything this diff changes in kind (no new package, no label/i18n source, and the new test file typechecks under the package's own test program above).

Clause-② re-declaration: no

Measured against the built dist/: the only new declaration is private businessUnitMemberScope; inside declare class ApprovalService (dist/index.d.ts), which TypeScript emits for a private member and no consumer can call; the export list of dist/index.d.ts is unchanged, and no accepted key or value changes. No needs:contract-review label hung.

Residue not fixed here

• sys_business_unit_member is unadjudicated in PLATFORM_OBJECT_TENANCY, so seed-replayed and system-written membership rows land organization-less #14570 (open): sys_business_unit_member unadjudicated in PLATFORM_OBJECT_TENANCY, so system-context writes still land org-less rows — the population B3 drops. Runtime classification, not a published surface.
• The approvals unit screen's null arm (businessUnitOrgScope) is the released 17.x behaviour and is governed by ADR-0131 (D8/D14, fixed structurally on the v18 line by C1); not a published API surface (dist/index.d.ts exports no such symbol; no content/docs/** page documents the predicate).
• The approvals: a department approver never resolves when the business unit has organization_id = null (every seeded BU) #3807 expanded to nobody warning names the approver and the request organization but not why the expansion was empty; it is the existing diagnostic and was left as is.

Two files outside plugin-approvals moved mechanically: content/docs/permissions/system-context.mdx (six approval-service.ts line anchors re-aimed by +57 — the net size of the docblock inserted above them — via check-system-context-census --fix) and scripts/engine-double-contract.pinned.json (the ledger learns the new pin file via --write).

Draft on purpose: not flipped ready, no auto-merge armed.

🤖 Generated with Claude Code

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 3 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

• content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class))

What this run could not see

• the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
• a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json da1cffb755ab5c528badc6401eaaf602b5210d74 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 22c9366b70e43fb841ceee8b73b6d81c0af9c29b — the merge of head 16dbdf55bc418b2a17bfe6266e290acc16d7a252 into base da1cffb755ab5c528badc6401eaaf602b5210d74, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 22c9366b70e43fb841ceee8b73b6d81c0af9c29b && git checkout 22c9366b70e43fb841ceee8b73b6d81c0af9c29b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin da1cffb755ab5c528badc6401eaaf602b5210d74 16dbdf55bc418b2a17bfe6266e290acc16d7a252 && git checkout -B drift-repro da1cffb755ab5c528badc6401eaaf602b5210d74 && git merge --no-ff 16dbdf55bc418b2a17bfe6266e290acc16d7a252

node scripts/docs-audit/affected-docs.mjs --json da1cffb755ab5c528badc6401eaaf602b5210d74

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs da1cffb755ab5c528badc6401eaaf602b5210d74 → pass the list as
args.docs, on the commit named under Which tree this was computed on.