fix(plugin-approvals): a user approver naming no account expands to nobody and takes onEmptyApprovers

.changeset/27397-user-approver-names-no-account.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+fix(plugin-approvals): a `user` approver whose id or email no account carries expands to nobody, warns, and takes the node's `onEmptyApprovers` route instead of opening on a slot no one can take
+
+Clause-②: no
+Metadata-protocol: none
+
+A `{ type: 'user', value }` approver used to put `value` on the slate as it was written, without asking whether any account carries it. A mistyped id, or the id of a person removed since the flow was authored, opened a request pending on that slot: no one could ever decide it, it appeared in no one's queue, and nothing was logged.
+
+Now the value is checked against `sys_user` (by id, or by email for an email-shaped value), with the same predicate the reassign door already uses. When no account carries it:
+
+- the approver logs the same `approver 'user:<value>' expanded to nobody` warning the team, department, position and membership-tier approvers log;
+- it adds no slot to the request;
+- a slate this leaves empty is decided by the node's `onEmptyApprovers`, by default `admin_rescue`, which opens the request on the organization's administrators;
+- beside other approvers, only the unresolvable one is dropped, and the others carry the request.
+
+Unchanged: a real account keeps its slot, including a banned one and a person from another organization's directory (a `user` approver names a person outright, so no tenancy screen is applied). An out-of-office delegation that routes the approver to a delegate still routes. A failed `sys_user` read keeps the slot, because it does not prove that the account is missing.

content/docs/automation/approvals.mdx

@@ -320,8 +320,8 @@ node parses its config on entry — **not** at authoring time, and `os lint` has
 no pairing arm for this one. The refusal is loud, names both keys, and lands
 before the node opens any approval request.
 
-Prefer a fallback target that cannot itself resolve empty (a literal `user`, or
-the `owner` membership tier). If it does resolve empty the node degrades to
+Prefer a fallback target that cannot itself resolve empty (a `user` naming a
+real account, or the `owner` membership tier). If it does resolve empty the node degrades to
 `admin_rescue` — the request opens on the organization's administrators and the
 run is never killed — and the log says both that the fallback fired and that it
 found nobody.
@@ -422,8 +422,10 @@ organization's directory resolves them — see
 What an entry that resolves to nobody leaves on the slate depends on its kind:
 
 - **A person entry** (`manager`, `field`) adds no slot. Nor does a `queue`
-  with no members, or an `expression` that yields no ids. A `user` entry never
-  resolves to nobody: its value is the slot.
+  with no members, or an `expression` that yields no ids. A `user` entry's
+  value is the slot when an account carries that id or email; one that no
+  account carries adds no slot, and logs the same `expanded to nobody` warning
+  as a group entry.
 - **A group entry** (`position`, `team`, `department`, `org_membership_level`)
   keeps its `<type>:<value>` literal as a slot, and so does an
   `expression` value that `resolveAs` re-expands into a group with nobody in it.

packages/plugins/plugin-approvals/src/admin-exemption-retired.test.ts

@@ -71,9 +71,16 @@ import { bindApprovalLockHook, bindDelegationWriteGuard } from './lifecycle-hook
 
 interface FakeRow { [k: string]: any }
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['u9'];
+
 /** The same minimal engine shape `approval-service.test.ts` uses. */
 function makeFakeEngine() {
-  const tables: Record<string, FakeRow[]> = {};
+  const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
   const ensure = (n: string) => (tables[n] ??= []);
   const hooks: Record<string, Array<{ handler: (ctx: any) => any; object?: string | string[]; packageId?: string }>> = {};
 
@@ -253,7 +260,7 @@ describe('#4839 evidence — the sanctioned admin path covers an unavailable app
     bindApprovalLockHook(engine as any);
     engine._tables['opportunity'] = [{ id: 'opp1', amount: 100, stage: 'new' }];
     // The substitute has an account: a reassign target must name one (#25485).
-    engine._tables['sys_user'] = [{ id: 'u7' }];
+    engine._tables['sys_user'].push({ id: 'u7' });
   });
 
   /** An ordinary member's edit of the approval's target record. */

packages/plugins/plugin-approvals/src/admin-rescue-named-slate.integration.test.ts

@@ -291,6 +291,57 @@ describe('an empty admin_rescue slate opens on the organization\'s administrator
     );
   });
 
+  // A `user` approver whose value names no user expands to nobody, exactly as
+  // an unstaffed graph target does: the same per-slot warning, no slot of its
+  // own, and the node's onEmptyApprovers decides a slate this leaves empty —
+  // here the default admin_rescue. Before, the bare value went onto the slate
+  // as a slot no one could ever take, and nothing was logged.
+  describe('a `user` approver naming no user expands to nobody (#27397)', () => {
+    const PHANTOM = { type: 'user', value: 'usr_does_not_exist' };
+    const phantomLine = "approver 'user:usr_does_not_exist' expanded to nobody";
+
+    it('alone, it takes the default admin_rescue route with the graph types\' warning, and holds no slot', async () => {
+      const before = warnings.length;
+      const row = await open('phantom_user', [PHANTOM]);
+      expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']);
+      expect(row.pending_approvers).not.toContain('usr_does_not_exist');
+      expect(told(row.id)).toEqual(['u_admin', 'u_owner']);
+      const lines = warnings.slice(before);
+      expect(lines.filter((w) => w.includes(phantomLine))).toHaveLength(1);
+      expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toHaveLength(1);
+      expect((await snapshotOf(row.id)).__resolvedFrom['admin_rescue:abandoned']).toEqual([]);
+    });
+
+    it('beside a real user, only the phantom is dropped — and it is still named in the log', async () => {
+      const before = warnings.length;
+      const row = await open('phantom_mixed', [{ type: 'user', value: 'u_alice' }, PHANTOM]);
+      expect(row.pending_approvers).toEqual(['u_alice']);
+      expect(told(row.id)).toEqual(['u_alice']);
+      const lines = warnings.slice(before);
+      expect(lines.filter((w) => w.includes(phantomLine))).toHaveLength(1);
+      expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toEqual([]);
+    });
+
+    it('an email no account carries is dropped; one an account carries keeps its slot', async () => {
+      const row = await open('phantom_email', [
+        { type: 'user', value: 'u_alice@example.com' }, { type: 'user', value: 'nobody@example.com' },
+      ]);
+      expect(row.pending_approvers).toEqual(['u_alice@example.com']);
+    });
+
+    it('CONTROL: a person of another organization\'s directory, named outright, keeps the slot (ADR-0105 D9)', async () => {
+      const row = await open('other_org_user', [{ type: 'user', value: 'u_other_owner' }]);
+      expect(row.pending_approvers).toEqual(['u_other_owner']);
+    });
+
+    it('CONTROL: a real user keeps the slot and no expanded-to-nobody line is logged', async () => {
+      const before = warnings.length;
+      const row = await open('real_user', [{ type: 'user', value: 'u_alice' }]);
+      expect(row.pending_approvers).toEqual(['u_alice']);
+      expect(warnings.slice(before).filter((w) => w.includes('expanded to nobody'))).toEqual([]);
+    });
+  });
+
   // Last of the cases whose slate is `org_a`'s administrators: the delegation
   // it writes stays, and no case after it reads that slate.
   it('an administrator out of office is covered by their delegate, as any named approver is', async () => {

packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts

@@ -32,9 +32,16 @@ import { ApprovalService, SLA_ACTOR_ID } from './approval-service.js';
 
 interface FakeRow { [k: string]: any }
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['bob', 'carol'];
+
 /** Equality/`$in`/`$ne`/`$contains` WHERE matcher — mirrors approval-service.test.ts. */
 function makeFakeEngine() {
-  const tables: Record<string, FakeRow[]> = {};
+  const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
   const ensure = (n: string) => (tables[n] ??= []);
 
   function matches(row: FakeRow, filter: any): boolean {

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

@@ -11,6 +11,13 @@ const noopLogger = {
   info() {}, warn() {}, error() {}, debug() {},
 };
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['u1', 'u2', 'u9', 'lead'];
+
 /**
  * Tiny in-memory ObjectQL stand-in — supports the `where`-equality + `$in`
  * queries the approval service issues, enough to drive the node bridge.
@@ -23,7 +30,7 @@ function makeFakeEngine(
   // the documented degradation, exercised below rather than assumed.
   schemas?: Record<string, any>,
 ) {
-  const tables = new Map<string, any[]>();
+  const tables = new Map<string, any[]>([['sys_user', ROUTED_USERS.map((id) => ({ id }))]]);
   const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
   const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
     if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);

packages/plugins/plugin-approvals/src/approval-override-audit.test.ts

@@ -24,9 +24,16 @@ import { ApprovalService } from './approval-service.js';
 
 interface FakeRow { [k: string]: any }
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['u9'];
+
 /** The same minimal engine shape `approval-service.test.ts` uses. */
 function makeFakeEngine() {
-  const tables: Record<string, FakeRow[]> = {};
+  const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
   const ensure = (n: string) => (tables[n] ??= []);
   function matches(row: FakeRow, filter: any): boolean {
     if (!filter || typeof filter !== 'object') return true;
@@ -103,7 +110,7 @@ describe('approval override audit marker (#4466)', () => {
       clock: { now: () => new Date(baseTime + (n++) * 1000) },
     });
     // The reassign target has an account: a reassign target must name one (#25485).
-    engine._tables['sys_user'] = [{ id: 'u7' }];
+    engine._tables['sys_user'].push({ id: 'u7' });
   });
 
   /** A request whose slate is properly STAFFED — one real, designated user. */

packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts

@@ -34,9 +34,16 @@ import { registerApprovalNode } from './approval-node.js';
 const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any;
 const noopLogger = { info() {}, warn() {}, error() {}, debug() {} };
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['u1'];
+
 /** In-memory ObjectQL stand-in — the approvals tables outlive the "restart". */
 function makeFakeEngine() {
-  const tables = new Map<string, any[]>();
+  const tables = new Map<string, any[]>([['sys_user', ROUTED_USERS.map((id) => ({ id }))]]);
   const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
   const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
     if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);

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

@@ -33,9 +33,16 @@ const asUser = (userId: string) =>
 
 const noopLogger = { info() {}, warn() {}, error() {}, debug() {} };
 
+/**
+ * The accounts this suite routes `user` approver slots to: a `user` approver
+ * holds a slot only when an account carries it (#27397), so the double's
+ * directory starts with them.
+ */
+const ROUTED_USERS = ['u1', 'u2'];
+
 /** In-memory ObjectQL stand-in: equality/$in where, orderBy, limit. */
 function makeFakeEngine() {
-  const tables = new Map<string, any[]>();
+  const tables = new Map<string, any[]>([['sys_user', ROUTED_USERS.map((id) => ({ id }))]]);
   const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
   const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
     if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);