fix(plugin-approvals): an expression approver id that names no account adds no phantom slot

.changeset/27879-expression-id-names-no-account.md

@@ -0,0 +1,10 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+fix(plugin-approvals): an `expression` approver id that names no account no longer opens a slot no one can take
+
+Clause-②: no
+Metadata-protocol: none
+
+#27397 (`user`) and #27752 (`field`, out-of-office delegate) made an approver id that no account carries expand to nobody. The third sibling now follows them: an `expression` approver resolved as users (`resolveAs: 'user'`, the default) keeps a slot for each id its expression yields only when an account carries it. An id no account carries logs the shared "expanded to nobody" warning, naming the id and the expression, and adds no slot, so a slate it leaves empty reaches the node's `onEmptyApprovers`. A failed directory read keeps the slot. A dead out-of-office delegate on an expression id routes back to that id, as it does for `user` and `field`.

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

@@ -13,10 +13,10 @@ const noopLogger = {
 
 /**
  * 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.
+ * holds a slot only when an account carries it (#27397), as does an
+ * `expression` id (#27879), so the double's directory starts with them.
  */
-const ROUTED_USERS = ['u1', 'u2', 'u9', 'lead'];
+const ROUTED_USERS = ['u1', 'u2', 'u3', 'u7', 'u8', 'u9', 'lead'];
 
 /**
  * Tiny in-memory ObjectQL stand-in — supports the `where`-equality + `$in`

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

@@ -427,13 +427,15 @@ describe('ApprovalService (node era)', () => {
 
   it('expression: trigger.* reads the submit-time snapshot, not the live row (#3447 P2)', async () => {
     engine._tables['opportunity'] = [{ id: 'opp1', reviewer: 'new_reviewer' }];
+    staffDirectory(engine, 'old_reviewer'); // an `expression` id holds a slot only when an account carries it (#27879)
     const req = await svc.openNodeRequest(
       exprInput('trigger.reviewer', { record: { id: 'opp1', reviewer: 'old_reviewer' } }), CTX,
     ) as any;
     expect(req.pending_approvers).toEqual(['old_reviewer']);
   });
 
   it('expression: vars.* reads flow variables; a CSV string fans out (#3447 P2)', async () => {
+    staffDirectory(engine, 'u6'); // an `expression` id holds a slot only when an account carries it (#27879)
     const req = await svc.openNodeRequest(
       exprInput('vars.approval_lead.next_reviewers', {
         variables: { approval_lead: { next_reviewers: 'u5, u6' } },
@@ -443,6 +445,7 @@ describe('ApprovalService (node era)', () => {
   });
 
   it('expression: an array result fans out into one slot per id (#3447 P2)', async () => {
+    staffDirectory(engine, 'u4'); // an `expression` id holds a slot only when an account carries it (#27879)
     const req = await svc.openNodeRequest(
       exprInput('vars.picked', { variables: { picked: ['u2', 'u3', 'u4'] } }), CTX,
     ) as any;
@@ -4145,6 +4148,93 @@ describe('ApprovalService — a `field` id or an out-of-office delegate that nam
   });
 });
 
+// #27879: the third sibling of #27397 / #27752. An `expression` approver
+// resolved as users (`resolveAs: 'user'`, the default) treats each value its
+// CEL yields as a person, so an id no account carries was a phantom slot.
+describe('ApprovalService — an `expression` id that names no account (#27879)', () => {
+  const svcWithWarnings = (engine: any) => {
+    const warnings: any[] = [];
+    let n = 0;
+    const svc = new ApprovalService({
+      engine,
+      clock: { now: () => new Date(1757000000000 + (n++) * 1000) },
+      logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) },
+    });
+    return { svc, warnings };
+  };
+
+  const exprInput = (picked: unknown, configExtra: Record<string, any> = {}) => ({
+    ...openInput([]),
+    variables: { picked },
+    config: {
+      approvers: [{ type: 'expression' as const, value: 'vars.picked' }],
+      behavior: 'first_response' as const,
+      lockRecord: false,
+      ...configExtra,
+    },
+  });
+  const nobody = (warnings: any[]) => warnings.filter(([msg]) => String(msg).includes('expanded to nobody'));
+
+  it('an id no account carries warns as `user` does, and adds no slot', async () => {
+    const engine = makeFakeEngine();
+    const { svc, warnings } = svcWithWarnings(engine);
+    const req = await svc.openNodeRequest(exprInput(['u2', 'u_gone']) as any, CTX) as any;
+
+    expect(req.pending_approvers).toEqual(['u2']);
+    const hits = nobody(warnings);
+    expect(hits).toHaveLength(1);
+    expect(String(hits[0][0])).toContain("approver 'expression' id 'u_gone' expanded to nobody");
+    expect(hits[0][1]).toMatchObject({ type: 'expression', value: 'vars.picked', id: 'u_gone', organizationId: 't1' });
+  });
+
+  it('a slate it empties reaches the node\'s onEmptyApprovers', async () => {
+    const engine = makeFakeEngine();
+    const { svc } = svcWithWarnings(engine);
+    const req = await svc.openNodeRequest(exprInput('u_gone', {
+      onEmptyApprovers: 'fallback',
+      fallbackApprovers: [{ type: 'user', value: 'u_backstop' }],
+    }) as any, CTX) as any;
+
+    expect(req.pending_approvers).toEqual(['u_backstop']);
+  });
+
+  it('ids an account carries stay quiet', async () => {
+    const engine = makeFakeEngine();
+    const { svc, warnings } = svcWithWarnings(engine);
+    const req = await svc.openNodeRequest(exprInput('u2,u3') as any, CTX) as any;
+
+    expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']);
+    expect(nobody(warnings)).toEqual([]);
+  });
+
+  it('a failed directory read keeps the slot', async () => {
+    const engine = makeFakeEngine();
+    const find = engine.find.bind(engine);
+    engine.find = async (object: string, options?: any) => {
+      if (object === 'sys_user') throw new Error('directory down');
+      return find(object, options);
+    };
+    const { svc, warnings } = svcWithWarnings(engine);
+    const req = await svc.openNodeRequest(exprInput(['u_gone']) as any, CTX) as any;
+
+    expect(req.pending_approvers).toEqual(['u_gone']);
+    expect(nobody(warnings)).toEqual([]);
+  });
+
+  it('a dead out-of-office delegate on an expression id falls back to that id', async () => {
+    const engine = makeFakeEngine();
+    engine._tables['sys_approval_delegation'] = [{
+      id: 'del_u3', delegator_id: 'u3', delegate_id: 'u_gone',
+      valid_from: '2025-01-01T00:00:00Z', valid_until: '2027-01-01T00:00:00Z',
+      reason: 'leave', organization_id: 't1',
+    }];
+    const { svc } = svcWithWarnings(engine);
+    const req = await svc.openNodeRequest(exprInput(['u2', 'u3']) as any, CTX) as any;
+
+    expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']);
+  });
+});
+
 // The empty-slate policy that NAMES people. Before it, an empty
 // `{ type: 'manager' }` rung had three endings and all three were bad: open on
 // a slate nobody can act on and wait for an admin (`admin_rescue`), kill the

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

@@ -2708,11 +2708,23 @@ export class ApprovalService implements IApprovalService {
     // unstaffed-target behaviour (and #3424 admin rescue) as the static types.
     const resolveAs = String(a.resolveAs ?? 'user');
     if (resolveAs === 'user') {
+      // #27879: each value takes the `field` treatment (#27752) — the slot is
+      // the id only when an account carries it. One that resolves to no one
+      // warns and adds no slot, so a slate it leaves empty reaches the node's
+      // onEmptyApprovers; a failed read keeps the slot.
       const slots: Array<{ id: string }> = [];
       for (const id of raw) {
-        for (const routed of await this.applyOooDelegation(id, now, organizationId, substitutions)) {
-          slots.push({ id: routed });
+        const routed = await this.applyOooDelegation(id, now, organizationId, substitutions);
+        if (routed.length === 1 && routed[0] === id && !id.includes(':') && await this.namesNoAccount(id)) {
+          this.logger?.warn?.(
+            `[approvals] approver 'expression' id '${id}' expanded to nobody — the slot names no one, so no `
+            + `one is asked through it. Staff it or re-point the expression; a slate left naming no one is decided `
+            + `by the node's onEmptyApprovers.`,
+            { type: 'expression', value: source, id, organizationId: organizationId ?? null },
+          );
+          continue;
         }
+        for (const r of routed) slots.push({ id: r });
       }
       return { slots, raw };
     }