fix(approvals): screen expanded team members to the request's organization (#10739)

* fix(approvals): screen expanded team members to the request's organization (#10547)

#10230 made a `team` approver prove the TEAM's tenancy and deferred its
members on purpose. `sys_team_member` carries `team_id` and `user_id` and no
organization column, so a team that passed that screen still routed every
user id it listed.

Measured on a fixture before the change, not read off the schema — an `org_a`
request against an `org_a` team whose member holds membership only in `org_b`:

  [PROBE M1] pending_approvers = ["u_outsider"]
  [PROBE M8] sys_member reads = 0

The expansion now screens the members with the provably-outside (fail-open)
posture `managerIsProvablyOutsideOrg` and `teamIsProvablyOutsideOrg` already
pin, in ONE `$in` read for the whole slate:

  - membership rows exist and none is the request's organization
      => present and NEGATIVE => dropped, loudly;
  - no rows, an unreadable table, a possibly-truncated read, or a request
    carrying no organization
      => ABSENT => routing unchanged, and the last case reads nothing.

A truncated read fails OPEN deliberately: this read is the only evidence that
a member IS a tenant here, so incomplete evidence must not be spent as proof
of absence. #3807 is the recorded cost of reading an absent fact as a
negative one.

Decides nothing #7497 asks: no reads are granted and no read screen is applied
to any approver type that lacks one today.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042

* chore(gates): record the #10547 test file's pinned engine doubles

`check:engine-double-contract` reported the new
`team-member-org-screen.test.ts` double as RETAINED — pinned coverage the
ledger did not yet record, so it protected nothing. Regenerated with
`--write`: 2 rows added (one delete, one update), none lost.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0f14f70b-575c-5f2b-a235-4000a55db042

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-warren

.changeset/team-member-org-screen.md

@@ -0,0 +1,37 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+Screen expanded `team` approver members to the request's organization (#10547).
+
+#10230 made a `team` approver prove the TEAM's tenancy, and deferred the
+members on purpose. `sys_team_member` carries `team_id` and `user_id` and no
+organization column, so a team that passed that screen still routed every user
+id it listed — including a user whose only `sys_member` row is in another
+organization. Measured on a fixture, not read off the schema: an `org_a`
+request against an `org_a` team returned `["u_outsider","u_insider"]` with zero
+`sys_member` reads.
+
+The expansion now screens the members with the same provably-outside posture
+the neighbouring screens pin, in ONE `$in` read for the whole slate:
+
+- membership rows exist for the user and none is the request's organization
+  (present and NEGATIVE) — dropped, with a warning naming the users, the team
+  and both organizations;
+- no membership rows, an unreadable `sys_member`, a possibly-truncated read, or
+  a request carrying no organization (ABSENT) — routing is left exactly as it
+  was, and the no-organization case performs no read at all.
+
+Holding membership elsewhere is not disqualifying; holding none here is.
+
+⚠️ Behaviour change, confined to one non-default policy: a node whose only
+approver is a team staffed entirely by users provably outside the organization
+now resolves to no one. Under the default `onEmptyApprovers: 'admin_rescue'` it
+still opens, routed to the dead `team:<id>` literal as any unresolved slate is;
+under `onEmptyApprovers: 'fail'` it now throws `NO_APPROVERS` where it
+previously opened.
+
+Residual condition on the security value: the screen can only act on tenancy
+facts that exist. A deployment that stamps an organization on its approval
+requests but does not materialize `sys_member` rows sees no change — by design,
+since #3807 recorded what treating an absent fact as a negative one costs.

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

@@ -268,6 +268,18 @@ function actingUserId(context: ExecutionContext | undefined): string | null {
  */
 const OOO_MAX_CHAIN = 8;
 
+/**
+ * Row cap on the single `sys_member` read that screens an expanded team slate
+ * (#10547, {@link ApprovalService.dropMembersProvablyOutsideOrg}).
+ *
+ * Sized above the 10000-member cap the `sys_team_member` expansion itself
+ * carries, because the screen reads MEMBERSHIP rows and a person may hold
+ * several. A result that comes back at the cap is treated as no evidence at
+ * all — see the truncation note on that method; the cap is therefore a
+ * fail-open threshold, never a silent trim of the slate.
+ */
+const MEMBER_SCREEN_READ_LIMIT = 50000;
+
 /**
  * Approver types resolved by QUERYING a graph rather than by taking `value`
  * literally (#3807). Each can legitimately come back empty — an unstaffed
@@ -1187,15 +1199,35 @@ export class ApprovalService implements IApprovalService {
    * team routed that organization's people an approval over a record they are
    * not a tenant of.
    *
-   * ⚠️ The screen is on the TEAM, not on its members, and that is the whole
-   * difference from the screen next door ({@link managerIsProvablyOutsideOrg},
-   * #10153). `sys_user` carries no tenancy fact at all, so a manager can only
-   * be placed by his `sys_member` rows; `sys_team` carries `organization_id`
-   * outright (`packages/platform-objects/src/identity/sys-team.object.ts`), so
-   * a team id transitively names exactly one organization and ONE row answers
-   * the question. Screening the MEMBERS instead would be both a wider read and
-   * a different assertion — it would rule on #7497 (does approver routing imply
-   * record read visibility?), which this card does not.
+   * TWO screens run here, and they assert different things (#10230, #10547):
+   *
+   *   1. the TEAM must not provably belong to another organization
+   *      ({@link teamIsProvablyOutsideOrg}) — `sys_team` carries
+   *      `organization_id` outright
+   *      (`packages/platform-objects/src/identity/sys-team.object.ts`), so a
+   *      team id transitively names exactly one organization and ONE row
+   *      answers the question;
+   *   2. each expanded MEMBER must not provably hold membership only in other
+   *      organizations ({@link dropMembersProvablyOutsideOrg}) —
+   *      `sys_team_member` carries `team_id` and `user_id` and NO tenancy
+   *      column at all, so passing (1) says nothing whatever about the people
+   *      it lists.
+   *
+   * #10230 landed (1) alone and deferred (2) on purpose. What closed the
+   * deferral is that (1) does not imply (2) even a little: a member removed
+   * from the organization but left on the team, a team re-parented across
+   * organizations (`/organization/update-team` accepts `organizationId` in its
+   * partial body), or a `sys_team_member` row written by a seed rather than
+   * through better-auth all produce a team that passes (1) carrying a user who
+   * is provably a tenant of somewhere else. Measured on this tree, not read off
+   * the schema — the probe is quoted in `team-member-org-screen.test.ts`.
+   *
+   * (2) is the SAME assertion as {@link managerIsProvablyOutsideOrg}, one hop
+   * further out, and it is asserted the same way: `sys_user` carries no tenancy
+   * fact, so `sys_member` rows are the only evidence that a person is placed
+   * anywhere. Like that screen, this one grants no reads and applies no read
+   * screen to any approver type that lacks one today, so it decides nothing
+   * #7497 (does approver routing imply record read visibility?) asks.
    */
   private async expandTeamUsers(teamId: string, organizationId?: string | null): Promise<string[]> {
     if (!teamId) return [];
@@ -1209,7 +1241,10 @@ export class ApprovalService implements IApprovalService {
         context: SYSTEM_CTX,
       } as any);
     } catch { rows = []; }
-    return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)));
+    const users = Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean)));
+    // #10547: the TEAM proved its tenancy above; its members have not proved
+    // theirs, and `sys_team_member` holds no fact that could. One read.
+    return await this.dropMembersProvablyOutsideOrg(teamId, users, organizationId);
   }
 
   /**
@@ -1272,6 +1307,109 @@ export class ApprovalService implements IApprovalService {
     return true;
   }
 
+  /**
+   * Drop the expanded team members who are PROVABLY tenants of other
+   * organizations and not of `organizationId`. (#10547)
+   *
+   * Returns the survivors, in the order they were expanded.
+   *
+   * Posture — identical to {@link managerIsProvablyOutsideOrg} and
+   * {@link teamIsProvablyOutsideOrg}, deliberately, because it is the same
+   * assertion about the same table:
+   *
+   *   - membership rows exist for this user, none in `organizationId`
+   *       ⇒ the tenancy fact is present and NEGATIVE ⇒ drop him;
+   *   - no membership rows at all for him, the read failed, or the request
+   *     carries no organization
+   *       ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was.
+   *
+   * The absent limb is load-bearing rather than timid, and #3807 is the recorded
+   * cost of getting it wrong: a stack that stamps an organization on requests
+   * but never materializes `sys_member` rows would otherwise lose EVERY team
+   * approver at once. This package's own `team_ok` expansion fixture and
+   * #10230's T2/T3 fixtures are exactly such stacks — they carry team rows and
+   * a request organization and no `sys_member` table at all — so the absent
+   * limb is exercised by neighbours on every run of this suite.
+   *
+   * ONE read for the whole slate, never one per person: the expansion is capped
+   * at 10000 members and a per-user query would turn a single team approver
+   * into 10000 round trips.
+   *
+   * ⚠️ A TRUNCATED read fails open, and that is the subtle half. This read is
+   * the only evidence that a member IS a tenant here, so a result cut off at
+   * the limit could be missing the very row that keeps a legitimate approver on
+   * the slate — screening him out on missing evidence, which inverts the
+   * posture into fail-CLOSED precisely where it must not. When the read comes
+   * back at the cap it is treated as no evidence at all.
+   */
+  private async dropMembersProvablyOutsideOrg(
+    teamId: string,
+    userIds: string[],
+    organizationId?: string | null,
+  ): Promise<string[]> {
+    const requestOrg = organizationId ? String(organizationId) : '';
+    // No organization on the request ⇒ nothing to screen against, and no read.
+    // The ordinary single-organization / embedded stack costs nothing here.
+    if (!requestOrg || !userIds.length) return userIds;
+    let rows: any[] = [];
+    try {
+      // No `as any` on this options bag — #4918's ratchet grandfathers this
+      // file for its EXISTING erasures only, and a NEW one must carry the
+      // declared type. `ApprovalEngine.find` already accepts it as written.
+      rows = await this.engine.find('sys_member', {
+        where: { user_id: { $in: userIds } },
+        fields: ['user_id', 'organization_id'],
+        limit: MEMBER_SCREEN_READ_LIMIT,
+        context: SYSTEM_CTX,
+      });
+    } catch { return userIds; } // membership unreadable — see the fail-open note
+    if ((rows?.length ?? 0) >= MEMBER_SCREEN_READ_LIMIT) {
+      // Possibly truncated ⇒ the evidence is incomplete ⇒ no evidence.
+      this.logger?.warn?.(
+        `[approvals] #10547: the membership screen for team '${teamId}' read `
+        + `${rows.length} 'sys_member' rows, at or above its ${MEMBER_SCREEN_READ_LIMIT}-row `
+        + `cap, so the result may be truncated. Routing is left unchanged rather than risk `
+        + `dropping a member whose proof of membership fell outside the read.`,
+        { teamId, requestOrganizationId: requestOrg, rowsRead: rows.length },
+      );
+      return userIds;
+    }
+    const orgsByUser = new Map<string, string[]>();
+    for (const r of rows ?? []) {
+      const uid = String((r as any)?.user_id ?? '');
+      const org = String((r as any)?.organization_id ?? '');
+      if (!uid || !org) continue;
+      const seen = orgsByUser.get(uid);
+      if (seen) seen.push(org); else orgsByUser.set(uid, [org]);
+    }
+    const kept: string[] = [];
+    const dropped: Array<{ userId: string; organizationIds: string[] }> = [];
+    for (const uid of userIds) {
+      const orgs = orgsByUser.get(uid);
+      if (!orgs?.length) { kept.push(uid); continue; }        // no tenancy fact recorded
+      if (orgs.includes(requestOrg)) { kept.push(uid); continue; } // a member here
+      dropped.push({ userId: uid, organizationIds: orgs });
+    }
+    if (dropped.length) {
+      this.logger?.warn?.(
+        `[approvals] #10547: ${dropped.length} member(s) of team '${teamId}' were dropped from `
+        + `the approver slate — ${dropped.map(d => `'${d.userId}'`).join(', ')} hold membership `
+        + `in other organization(s), none of them the request's organization '${requestOrg}', so `
+        + `routing this approval to them would put approval authority over the record outside its `
+        + `tenant. The TEAM itself belongs to this organization; its 'sys_team_member' rows carry `
+        + `no organization of their own. Remove them from the team, grant them a membership in `
+        + `this organization, or route this step with an approver type that names someone in it.`,
+        {
+          teamId,
+          requestOrganizationId: requestOrg,
+          droppedUserIds: dropped.map(d => d.userId),
+          droppedMemberOrganizationIds: dropped.map(d => d.organizationIds),
+        },
+      );
+    }
+    return kept;
+  }
+
   /**
    * Tenant scope for a `sys_business_unit` read that may legitimately be
    * env-wide (#3807).