Skip to content

Commit b13a1ed

Browse files
fix(plugin-approvals): a field approver id or an out-of-office delegate that names no account adds no phantom slot (#27852)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} --> _Requested by **Elon** · [project thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9DMNav4GoJtdKDeq5tRaBC9R)_ Fixes #27752 ## 维护者速读 - 改了什么:审批节点里,按记录字段取的审批人(`field`)如果对应的账号不存在,不再生成一个没人能处理的空槽位;外出委托指向一个已删除的账号时,审批退回给原审批人本人。 - 为什么改:#27749 已经把直接指定用户(`user`)的这种情况修好了,这两条兄弟路径还是老样子,审批单会卡在一个不存在的人身上,日志里也没有任何提示。 - 风险与回滚:只改 `plugin-approvals` 的审批人展开;目录读取失败时一律保留原槽位,不会因为一次读失败清空审批人。回滚即还原这一个提交。 - 先看:`approval-service.ts` 里 `resolveApproverSpec` 的 `field` 分支和 `applyOooDelegation` 末尾的退回逻辑。 - 两分钟验证:`pnpm --filter @objectstack/plugin-approvals exec vitest run src/approval-service.test.ts -t 27752`,9 条用例全绿。 - 需要你做的:无。 ## What changes - **`type: 'field'`** (`resolveApproverSpec`): each id the record field holds now takes #27749's `user` treatment. The id is a slot only when an account carries it, asked through the same predicate (`reassignTargetRefusal` with no organization). An id no account carries logs the shared "expanded to nobody" warning (naming the field and the id), adds no slot, and a slate it empties reaches the node's `onEmptyApprovers`. A failed directory read keeps the slot. - **Out-of-office delegation** (`applyOooDelegation`): a chain that ends on a delegate no account carries routes the slot back to the original approver, per the triage ruling (6510482313). It logs an "expanded to nobody" warning naming delegator and delegate, and the chain's hops are taken back out of the substitution collector, so no `ooo_substitute` action or notification is written for a reroute that did not happen. A failed read keeps the delegate. A removed delegator with a live delegate still routes to the delegate (#24065). The fallback lives in `applyOooDelegation`, so it covers every caller: `user`, `field`, `manager` and `expression` with `resolveAs: 'user'`. - A small private `namesNoAccount(value)` wraps the predicate plus its fail-open catch, and the `user` branch now uses it too (same behaviour). ## Why #27749 (#27397) closed the phantom-slot path for `user` approvers. The `field` branch and the delegation step still handed an id no account carries straight to the slate: the request opened on a slot nobody could take, with no warning and no `onEmptyApprovers`. ## Verification - Measured on `main` first: of the 9 new cases, 5 were red (the two `field` cases and the three dead-delegate cases) and the 4 pins (live ids quiet, failed-read keeps the slot for both paths, live delegate routes with its substitution) were green. - Ablation: disabling the `field` check reds the 2 `field` cases. Disabling the delegate check reds the 3 delegate cases. Keeping the check but dropping the collector rollback reds the 2 cases that assert no substitution. Restored: all green. - `plugin-approvals` full suite: 105 files, 1347 tests pass. Four existing cases routed to ids their test double's directory did not carry (`u4`, `u7`, `bob`). Their fixtures now staff those accounts, test-only, as #27749 did for `user`. - Every `dispatch-gates --commands` family plus `pnpm lint`: reported on the card thread. Clause-②: no Metadata-protocol: none ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (not touched) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016WV4cwhsfZfJRshUTzrYYT Co-authored-by: Claude <noreply@anthropic.com>
1 parent 64e5efd commit b13a1ed

5 files changed

Lines changed: 213 additions & 9 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): a `field` approver id, or an out-of-office delegate, that names no account no longer opens a slot no one can take
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
#27397 made a `{ type: 'user' }` approver that names no account expand to nobody. Its two siblings now follow it:
11+
12+
- **`type: 'field'`.** Each id the record field holds is a slot only when an account carries it (the same predicate the reassign door asks, with no organization). An id no account carries logs the shared "expanded to nobody" warning, naming the field and the id, and adds no slot, so a slate it leaves empty reaches the node's `onEmptyApprovers`. A failed directory read keeps the slot.
13+
- **Out-of-office delegation.** A delegation chain that ends on a delegate no account carries routes the slot back to the original approver, with an "expanded to nobody" warning naming both. It never empties the slate, and no `ooo_substitute` action or notification is recorded for it. A failed read keeps the delegate. A removed delegator whose delegate is live still routes to the delegate, as before.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ describe('#4839 evidence — a delegation cannot rescue an IN-FLIGHT approval',
238238
});
239239

240240
it('the delegation DOES apply to a request opened afterwards — it is an open-time rule, nothing more', async () => {
241+
engine._tables['sys_user'].push({ id: 'u7' }); // a delegate routes only when an account carries it (#27752)
241242
seedDelegation('u9', 'u7');
242243
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
243244
// Guard the guard: the delegation machinery is wired and working in this

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

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,7 @@ describe('ApprovalService (node era)', () => {
372372

373373
it('field approver: fans a multi-select user field into one slot per user (#3447)', async () => {
374374
engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: ['u2', 'u3', 'u4'] }];
375+
staffDirectory(engine, 'u4'); // a `field` id holds a slot only when an account carries it (#27752)
375376
const req = await svc.openNodeRequest(fieldInput({ record: { id: 'opp1' } }), CTX);
376377
expect(req.pending_approvers.sort()).toEqual(['u2', 'u3', 'u4']);
377378
});
@@ -386,6 +387,7 @@ describe('ApprovalService (node era)', () => {
386387
// No `opportunity` row — hard-deleted between submit and node entry. The
387388
// snapshot still carries a value, so the request opens against it (warn but
388389
// proceed) rather than wedging the flow.
390+
staffDirectory(engine, 'u7');
389391
const req = await svc.openNodeRequest(
390392
fieldInput({ record: { id: 'opp1', approvers_dynamic: ['u7'] } }), CTX,
391393
);
@@ -3995,6 +3997,154 @@ describe('ApprovalService — a graph approver that expands to nobody warns (#38
39953997
});
39963998
});
39973999

4000+
// #27752: the two siblings of #27397's `user` treatment. A `field` approver
4001+
// reads its ids off the record, and an out-of-office delegation reroutes an
4002+
// approver to its delegate: either can name an id no account carries, which
4003+
// was a phantom slot no one could take.
4004+
describe('ApprovalService — a `field` id or an out-of-office delegate that names no account (#27752)', () => {
4005+
const svcWithWarnings = (engine: any) => {
4006+
const warnings: any[] = [];
4007+
let n = 0;
4008+
const svc = new ApprovalService({
4009+
engine,
4010+
clock: { now: () => new Date(1757000000000 + (n++) * 1000) },
4011+
logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) },
4012+
});
4013+
return { svc, warnings };
4014+
};
4015+
4016+
const fieldInput = (held: unknown, configExtra: Record<string, any> = {}) => ({
4017+
...openInput([]),
4018+
record: { id: 'opp1', amount: 100, approver_ids: held },
4019+
config: {
4020+
approvers: [{ type: 'field' as const, value: 'approver_ids' }],
4021+
behavior: 'first_response' as const,
4022+
lockRecord: false,
4023+
...configExtra,
4024+
},
4025+
});
4026+
const userInput = (value: string) => ({
4027+
...openInput([]),
4028+
config: {
4029+
approvers: [{ type: 'user' as const, value }],
4030+
behavior: 'first_response' as const,
4031+
lockRecord: false,
4032+
},
4033+
});
4034+
const delegation = (delegator: string, delegate: string, id = `del_${delegator}`) => ({
4035+
id, delegator_id: delegator, delegate_id: delegate,
4036+
valid_from: '2025-01-01T00:00:00Z', valid_until: '2027-01-01T00:00:00Z',
4037+
reason: 'leave', organization_id: 't1',
4038+
});
4039+
const nobody = (warnings: any[]) => warnings.filter(([msg]) => String(msg).includes('expanded to nobody'));
4040+
const substitutes = (engine: any) => (engine._tables['sys_approval_action'] ?? [])
4041+
.filter((r: any) => r.action === 'ooo_substitute');
4042+
const sysUserDown = (engine: any) => {
4043+
const find = engine.find.bind(engine);
4044+
engine.find = async (object: string, options?: any) => {
4045+
if (object === 'sys_user') throw new Error('directory down');
4046+
return find(object, options);
4047+
};
4048+
};
4049+
4050+
it('`field`: an id no account carries warns as `user` does, and adds no slot', async () => {
4051+
const engine = makeFakeEngine();
4052+
const { svc, warnings } = svcWithWarnings(engine);
4053+
const req = await svc.openNodeRequest(fieldInput(['u2', 'u_gone']) as any, CTX) as any;
4054+
4055+
expect(req.pending_approvers).toEqual(['u2']);
4056+
const hits = nobody(warnings);
4057+
expect(hits).toHaveLength(1);
4058+
expect(String(hits[0][0])).toContain("approver 'field:approver_ids' id 'u_gone' expanded to nobody");
4059+
expect(hits[0][1]).toMatchObject({ type: 'field', value: 'approver_ids', id: 'u_gone', organizationId: 't1' });
4060+
});
4061+
4062+
it('`field`: a slate it empties reaches the node\'s onEmptyApprovers', async () => {
4063+
const engine = makeFakeEngine();
4064+
const { svc } = svcWithWarnings(engine);
4065+
const req = await svc.openNodeRequest(fieldInput('u_gone', {
4066+
onEmptyApprovers: 'fallback',
4067+
fallbackApprovers: [{ type: 'user', value: 'u_backstop' }],
4068+
}) as any, CTX) as any;
4069+
4070+
expect(req.pending_approvers).toEqual(['u_backstop']);
4071+
});
4072+
4073+
it('`field`: ids an account carries stay quiet', async () => {
4074+
const engine = makeFakeEngine();
4075+
const { svc, warnings } = svcWithWarnings(engine);
4076+
const req = await svc.openNodeRequest(fieldInput('u2,u3') as any, CTX) as any;
4077+
4078+
expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']);
4079+
expect(nobody(warnings)).toEqual([]);
4080+
});
4081+
4082+
it('`field`: a failed directory read keeps the slot', async () => {
4083+
const engine = makeFakeEngine();
4084+
sysUserDown(engine);
4085+
const { svc, warnings } = svcWithWarnings(engine);
4086+
const req = await svc.openNodeRequest(fieldInput(['u_gone']) as any, CTX) as any;
4087+
4088+
expect(req.pending_approvers).toEqual(['u_gone']);
4089+
expect(nobody(warnings)).toEqual([]);
4090+
});
4091+
4092+
it('a delegate no account carries falls back to the original approver, with the warning and no substitution', async () => {
4093+
const engine = makeFakeEngine();
4094+
engine._tables['sys_approval_delegation'] = [delegation('u2', 'u_gone')];
4095+
const { svc, warnings } = svcWithWarnings(engine);
4096+
const req = await svc.openNodeRequest(userInput('u2') as any, CTX) as any;
4097+
4098+
expect(req.pending_approvers).toEqual(['u2']);
4099+
const hits = nobody(warnings);
4100+
expect(hits).toHaveLength(1);
4101+
expect(String(hits[0][0])).toContain("out-of-office delegate 'u_gone' of 'u2' expanded to nobody");
4102+
expect(hits[0][1]).toMatchObject({ delegatorId: 'u2', delegateId: 'u_gone', organizationId: 't1' });
4103+
expect(substitutes(engine)).toEqual([]);
4104+
});
4105+
4106+
it('a dead delegate at the end of a chain falls back to the original approver, not a middle hop', async () => {
4107+
const engine = makeFakeEngine();
4108+
engine._tables['sys_approval_delegation'] = [delegation('u2', 'u3'), delegation('u3', 'u_gone')];
4109+
const { svc } = svcWithWarnings(engine);
4110+
const req = await svc.openNodeRequest(userInput('u2') as any, CTX) as any;
4111+
4112+
expect(req.pending_approvers).toEqual(['u2']);
4113+
expect(substitutes(engine)).toEqual([]);
4114+
});
4115+
4116+
it('a dead delegate on a `field` id falls back to that id', async () => {
4117+
const engine = makeFakeEngine();
4118+
engine._tables['sys_approval_delegation'] = [delegation('u3', 'u_gone')];
4119+
const { svc } = svcWithWarnings(engine);
4120+
const req = await svc.openNodeRequest(fieldInput(['u2', 'u3']) as any, CTX) as any;
4121+
4122+
expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']);
4123+
});
4124+
4125+
it('a live delegate still routes, with its substitution', async () => {
4126+
const engine = makeFakeEngine();
4127+
engine._tables['sys_approval_delegation'] = [delegation('u2', 'u9')];
4128+
const { svc, warnings } = svcWithWarnings(engine);
4129+
const req = await svc.openNodeRequest(userInput('u2') as any, CTX) as any;
4130+
4131+
expect(req.pending_approvers).toEqual(['u9']);
4132+
expect(nobody(warnings)).toEqual([]);
4133+
expect(substitutes(engine)).toHaveLength(1);
4134+
});
4135+
4136+
it('a failed directory read keeps the delegate', async () => {
4137+
const engine = makeFakeEngine();
4138+
engine._tables['sys_approval_delegation'] = [delegation('u2', 'u_gone')];
4139+
sysUserDown(engine);
4140+
const { svc, warnings } = svcWithWarnings(engine);
4141+
const req = await svc.openNodeRequest(userInput('u2') as any, CTX) as any;
4142+
4143+
expect(req.pending_approvers).toEqual(['u_gone']);
4144+
expect(nobody(warnings)).toEqual([]);
4145+
});
4146+
});
4147+
39984148
// The empty-slate policy that NAMES people. Before it, an empty
39994149
// `{ type: 'manager' }` rung had three endings and all three were bad: open on
40004150
// a slate nobody can act on and wait for an admin (`admin_rescue`), kill the

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

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2436,7 +2436,8 @@ export class ApprovalService implements IApprovalService {
24362436
// account still resolves, as a banned team or unit member does (neither
24372437
// expansion reads the flag). What is asked is the slot the route ENDS
24382438
// on: an out-of-office hop that carries the request to a delegate has
2439-
// named someone (a removed account's delegation still routes, #24065).
2439+
// named someone (a removed account's delegation still routes, #24065;
2440+
// a delegate no account carries routes back here, #27752).
24402441
// One that resolves to no one falls through to the graph types'
24412442
// "expanded to nobody" warning below and adds no slot, so a slate it
24422443
// leaves empty reaches the node's onEmptyApprovers. A failed read proves
@@ -2447,18 +2448,31 @@ export class ApprovalService implements IApprovalService {
24472448
const value = String(a.value);
24482449
const routed = await this.applyOooDelegation(value, now, organizationId, substitutions);
24492450
if (routed.length !== 1 || routed[0] !== value || value.includes(':')) return routed;
2450-
let refusal: string | undefined;
2451-
try { refusal = await this.reassignTargetRefusal(value, null); } catch { refusal = undefined; }
2452-
if (refusal === undefined) return routed;
2451+
if (!await this.namesNoAccount(value)) return routed;
24532452
}
24542453
if (type === 'field' && record) {
24552454
// #3447: a record field can name MANY approvers — a multi-select user
24562455
// field arrives as an array (or a legacy CSV string). Fan each out into
24572456
// its own slot and OOO-substitute per person; collapsing to `String(...)`
24582457
// (→ `'u1,u2'`) would mint one bogus approver id and skip every delegate.
2458+
//
2459+
// #27752: each id takes the `user` treatment above — the slot is the id
2460+
// only when an account carries it. One that resolves to no one warns as
2461+
// `user` does and adds no slot, so a slate it leaves empty reaches the
2462+
// node's onEmptyApprovers; a failed read keeps the slot.
24592463
const out: string[] = [];
24602464
for (const id of csvSplit((record as any)[a.value])) {
2461-
out.push(...await this.applyOooDelegation(id, now, organizationId, substitutions));
2465+
const routed = await this.applyOooDelegation(id, now, organizationId, substitutions);
2466+
if (routed.length === 1 && routed[0] === id && !id.includes(':') && await this.namesNoAccount(id)) {
2467+
this.logger?.warn?.(
2468+
`[approvals] approver 'field:${a.value}' id '${id}' expanded to nobody — the slot names no one, so no `
2469+
+ `one is asked through it. Staff it or re-point the approver; a slate left naming no one is decided `
2470+
+ `by the node's onEmptyApprovers.`,
2471+
{ type, value: a.value, id, organizationId: organizationId ?? null },
2472+
);
2473+
continue;
2474+
}
2475+
out.push(...routed);
24622476
}
24632477
return out;
24642478
}
@@ -3411,6 +3425,7 @@ export class ApprovalService implements IApprovalService {
34113425
): Promise<string[]> {
34123426
const start = String(userId ?? '').trim();
34133427
if (!start) return [];
3428+
const mark = collector?.length ?? 0;
34143429
let current = start;
34153430
const visited = new Set<string>([current]);
34163431
for (let hop = 0; hop < OOO_MAX_CHAIN; hop++) {
@@ -3422,9 +3437,34 @@ export class ApprovalService implements IApprovalService {
34223437
visited.add(to);
34233438
current = to;
34243439
}
3440+
// #27752: a delegate no account carries is a slot no one can take. The
3441+
// original approver is a person the route already named, and delegation is
3442+
// only their convenience, so a broken one routes back to them — never to
3443+
// no slot — with the "expanded to nobody" warning, and its hops leave the
3444+
// collector so no substitution is audited or notified. A failed read
3445+
// proves nothing, so the delegate stays.
3446+
if (current !== start && !current.includes(':') && await this.namesNoAccount(current)) {
3447+
this.logger?.warn?.(
3448+
`[approvals] out-of-office delegate '${current}' of '${start}' expanded to nobody — the delegation `
3449+
+ `names no account, so the slot stays with '${start}'. Point the delegation at a current user, or `
3450+
+ `delete it.`,
3451+
{ delegatorId: start, delegateId: current, organizationId: organizationId ?? null },
3452+
);
3453+
if (collector) collector.length = mark;
3454+
return [start];
3455+
}
34253456
return [current];
34263457
}
34273458

3459+
/**
3460+
* Whether no account carries `value` — the {@link reassignTargetRefusal}
3461+
* predicate asked with NO organization, as a `user` approver asks it
3462+
* (#27397). A failed read proves nothing, so it answers `false`.
3463+
*/
3464+
private async namesNoAccount(value: string): Promise<boolean> {
3465+
try { return (await this.reassignTargetRefusal(value, null)) !== undefined; } catch { return false; }
3466+
}
3467+
34283468
/**
34293469
* The active OOO delegation for a delegator at `now`, or null. Validity is the
34303470
* shared `isGrantActive` half-open window (ADR-0091 D2), enforced here at

‎packages/plugins/plugin-approvals/src/ooo-delegation-organization.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,11 @@ import { ApprovalService } from './approval-service.js';
2929
interface Row { [k: string]: any }
3030

3131
/**
32-
* The accounts this suite routes `user` approver slots to: a `user` approver
33-
* holds a slot only when an account carries it (#27397), so the double's
34-
* directory starts with them.
32+
* The accounts this suite routes approver slots to: a `user` approver holds a
33+
* slot only when an account carries it (#27397), and a delegate only when an
34+
* account carries it (#27752), so the double's directory starts with them.
3535
*/
36-
const ROUTED_USERS = ['alice', 'carol'];
36+
const ROUTED_USERS = ['alice', 'bob', 'carol'];
3737

3838
function makeEngine(seed: Record<string, Row[]> = {}) {
3939
const tables: Record<string, Row[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })), ...seed };

0 commit comments

Comments
 (0)