From 188c4d0fa760dbc884bdcad1be74fc7a9a092d81 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Oct 2026 03:48:10 +0000 Subject: [PATCH] fix(plugin-approvals): an expression approver id that names no account no longer opens a slot An `expression` approver resolved as users (`resolveAs: 'user'`, the default) now asks the same `namesNoAccount` predicate the `field` branch asks: an id no account carries warns "expanded to nobody" and adds no slot, so an emptied slate reaches the node's onEmptyApprovers. A failed directory read keeps the slot; a dead out-of-office delegate routes back to the id. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_016WV4czYgbnZDQp3qTKCWae --- .../27879-expression-id-names-no-account.md | 10 +++ .../src/approval-node.test.ts | 6 +- .../src/approval-service.test.ts | 90 +++++++++++++++++++ .../plugin-approvals/src/approval-service.ts | 16 +++- 4 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 .changeset/27879-expression-id-names-no-account.md diff --git a/.changeset/27879-expression-id-names-no-account.md b/.changeset/27879-expression-id-names-no-account.md new file mode 100644 index 00000000000..a4a733ee9e7 --- /dev/null +++ b/.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`. diff --git a/packages/plugins/plugin-approvals/src/approval-node.test.ts b/packages/plugins/plugin-approvals/src/approval-node.test.ts index e8f9b58f786..bac857efcfb 100644 --- a/packages/plugins/plugin-approvals/src/approval-node.test.ts +++ b/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` diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index d509fc06f3b..b6228855269 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -427,6 +427,7 @@ 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; @@ -434,6 +435,7 @@ describe('ApprovalService (node era)', () => { }); 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 = {}) => ({ + ...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 diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 20b0b9f0185..215cc94cbf1 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/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 }; }