fix(approvals): resolve department approvers against env-wide business units (#3807) (#3811)

`expandBusinessUnitUsers` scoped its `sys_business_unit` reads with a strict
`organization_id = <request org>` equality, so a unit whose `organization_id`
is `null` was invisible: the seed check found no row, the expansion returned
`[]`, and the approver fell back to the dead `department:<id>` literal that
routes to nobody.

An app's org tree is seeded, and a seed cannot know the organization id the
runtime mints at boot, so every seeded unit carries `organization_id = null` —
while an approval request always carries one. Every business unit a flow author
could pick therefore resolved to nobody, silently.

Both the seed check and the subtree descent now scope to this org ∪ env-wide,
the same predicate `sys_metadata`'s pending-draft listing settled on for the
identical reason. The wall between two organizations is unchanged.

Author: os-zhuang

.changeset/department-approver-env-wide-business-unit.md

@@ -0,0 +1,36 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): a `department` approver resolves against env-wide business units (#3807)
+
+`expandBusinessUnitUsers` scoped its `sys_business_unit` reads with a strict
+`organization_id = <request org>` equality, so a unit whose `organization_id`
+is `null` was invisible: the seed check found no row, the expansion returned
+`[]`, and the approver fell back to the dead `department:<id>` literal that
+routes to nobody.
+
+That is the normal case, not an edge case. An app's org tree is seeded, and a
+seed cannot know the organization id the runtime mints at boot, so every seeded
+unit carries `organization_id = null` — while an approval request always
+carries an org. Every business unit a flow author could pick therefore resolved
+to nobody, silently: the request opens, the slate is empty, and (with
+`lockRecord`) the record stays locked with no one able to act (#3424 is the
+downstream shape of the same dead end). Verified against a live showcase stack:
+a `{ type: 'department', value: 'bu_hq_finance' }` approver produced
+`pending_approvers: "department:bu_hq_finance"` while the unit's member sat
+right there in `sys_business_unit_member`.
+
+Both the seed check and the subtree descent now scope to **this org ∪
+env-wide** — `$or: [{ organization_id: <org> }, { organization_id: null }]` —
+the same predicate `sys_metadata`'s pending-draft listing settled on for the
+identical reason (a strict equality silently dropping env-wide rows). The wall
+between two organizations is unchanged: another org's unit still fails the
+match, and a null-org parent does not drag another org's child unit into the
+subtree.
+
+Note the same strict-equality scope exists in `plugin-sharing`'s
+`BusinessUnitGraphService.orgScope`. It is not reachable today — every
+materialized `sys_sharing_rule` row carries `organization_id = null`, so the
+filter is skipped — and is left alone here rather than widen an
+access-granting path on a defect that cannot currently fire.

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -630,6 +630,69 @@ describe('ApprovalService (node era)', () => {
     expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']);
   });
 
+  // #3807 — an app's org tree is normally SEEDED, and a seed cannot know the
+  // organization id the runtime mints at boot, so those rows carry
+  // `organization_id = null` while every approval request carries an org. The
+  // old strict equality made each of them invisible and every `department`
+  // approver resolved to the dead `department:<id>` literal.
+  const departmentInput = (value: string) => positionInput({
+    config: {
+      approvers: [{ type: 'department' as const, value }],
+      behavior: 'first_response' as const,
+      lockRecord: true,
+    },
+  });
+
+  it('department approver: an env-wide (null-org) business unit still resolves (#3807)', async () => {
+    engine._tables['sys_business_unit'] = [
+      { id: 'bu_seeded', organization_id: null, active: true },
+      { id: 'bu_seeded_child', parent_business_unit_id: 'bu_seeded', organization_id: null, active: true },
+    ];
+    engine._tables['sys_business_unit_member'] = [
+      { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5' },
+      { id: 'bm2', business_unit_id: 'bu_seeded_child', user_id: 'u6' },
+    ];
+    const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX);
+    // Both the seed check AND the subtree descent must see the null-org rows.
+    expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']);
+  });
+
+  it('department approver: another organization’s unit stays invisible (#3807 keeps the wall)', async () => {
+    engine._tables['sys_business_unit'] = [
+      { id: 'bu_other', organization_id: 't2', active: true },
+      { id: 'bu_other_child', parent_business_unit_id: 'bu_other', organization_id: 't2', active: true },
+    ];
+    engine._tables['sys_business_unit_member'] = [
+      { id: 'bm1', business_unit_id: 'bu_other', user_id: 'intruder' },
+      { id: 'bm2', business_unit_id: 'bu_other_child', user_id: 'intruder2' },
+    ];
+    const req = await svc.openNodeRequest(departmentInput('bu_other'), CTX);
+    expect(req.pending_approvers).toEqual(['department:bu_other']);
+  });
+
+  it('department approver: a null-org subtree does not drag in another org’s child unit (#3807)', async () => {
+    engine._tables['sys_business_unit'] = [
+      { id: 'bu_seeded', organization_id: null, active: true },
+      { id: 'bu_mine', parent_business_unit_id: 'bu_seeded', organization_id: 't1', active: true },
+      { id: 'bu_theirs', parent_business_unit_id: 'bu_seeded', organization_id: 't2', active: true },
+    ];
+    engine._tables['sys_business_unit_member'] = [
+      { id: 'bm1', business_unit_id: 'bu_mine', user_id: 'u5' },
+      { id: 'bm2', business_unit_id: 'bu_theirs', user_id: 'intruder' },
+    ];
+    const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX);
+    expect(req.pending_approvers).toEqual(['u5']);
+  });
+
+  it('department approver: an inactive env-wide unit still contributes nobody (#3807)', async () => {
+    engine._tables['sys_business_unit'] = [{ id: 'bu_seeded', organization_id: null, active: false }];
+    engine._tables['sys_business_unit_member'] = [
+      { id: 'bm1', business_unit_id: 'bu_seeded', user_id: 'u5' },
+    ];
+    const req = await svc.openNodeRequest(departmentInput('bu_seeded'), CTX);
+    expect(req.pending_approvers).toEqual(['department:bu_seeded']);
+  });
+
   // ── decideNode ──────────────────────────────────────────────────
 
   it('decideNode: first_response approve finalizes immediately', async () => {

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -904,15 +904,41 @@ export class ApprovalService implements IApprovalService {
     return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)));
   }
 
+  /**
+   * Tenant scope for a `sys_business_unit` read that may legitimately be
+   * env-wide (#3807).
+   *
+   * `organization_id = null` on a platform object means "owned by no
+   * organization" — a row written by a seed, the file layer, or bootstrap,
+   * i.e. before (or outside) any org exists. A strict
+   * `organization_id = <request org>` equality made every such row invisible:
+   * the seed check below found nothing, the whole expansion returned `[]`, and
+   * the approver fell back to the dead `department:<id>` literal that routes to
+   * nobody. That is not an edge case — an app's org tree is normally seeded
+   * (a seed cannot know the org id the runtime mints at boot) while the
+   * approval request always carries one, so EVERY department approver a
+   * designer could pick resolved to nobody.
+   *
+   * Widen to "this org ∪ env-wide", the same predicate `sys_metadata`'s
+   * pending-draft listing settled on for the identical reason. Another org's
+   * unit still fails the match, so the wall between two organizations is
+   * unchanged — only rows belonging to no org at all become visible.
+   */
+  private businessUnitOrgScope(
+    filter: Record<string, unknown>,
+    organizationId?: string | null,
+  ): Record<string, unknown> {
+    if (!organizationId) return filter;
+    return { ...filter, $or: [{ organization_id: organizationId }, { organization_id: null }] };
+  }
+
   /** Recursive department — walks `sys_business_unit.parent_business_unit_id`. */
   private async expandBusinessUnitUsers(businessUnitId: string, organizationId?: string | null): Promise<string[]> {
     if (!businessUnitId) return [];
     // Seed sanity check: skip if dept doesn't exist or is inactive within tenant.
     try {
       const seed = await this.engine.find('sys_business_unit', {
-        filter: organizationId
-          ? { id: businessUnitId, organization_id: organizationId }
-          : { id: businessUnitId },
+        filter: this.businessUnitOrgScope({ id: businessUnitId }, organizationId),
         fields: ['id', 'active'],
         limit: 1,
         context: SYSTEM_CTX,
@@ -927,8 +953,10 @@ export class ApprovalService implements IApprovalService {
       const parent = queue.shift()!;
       let kids: any[] = [];
       try {
-        const filter: any = { parent_business_unit_id: parent, active: { $ne: false } };
-        if (organizationId) filter.organization_id = organizationId;
+        const filter = this.businessUnitOrgScope(
+          { parent_business_unit_id: parent, active: { $ne: false } },
+          organizationId,
+        );
         kids = await this.engine.find('sys_business_unit', { filter, fields: ['id'], limit: 1000, context: SYSTEM_CTX } as any);
       } catch { kids = []; }
       for (const k of kids ?? []) {