Skip to content

Commit 2dc69d7

Browse files
fix(plugin-approvals): a user approver naming no account expands to nobody and takes onEmptyApprovers (#27749)
domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F <!-- claude-projects-attribution: {"github_login":"os-sales"} --> _Requested by **Sales** · [project thread](https://claude.ai/code/project/chan_01D5bWFY3rFzgJcytD5gD4Uu?thread=cmsg_01D5bWFY3rFzgJcytD5gD4UuPMZ8BHWtPf6nBTp51qSM64)_ Fixes #27397 Clause-②: no Metadata-protocol: none Skills-docs: none — no `skills/**` text describes a `user` approver's resolution; the one docs page that did (`content/docs/automation/approvals.mdx`, two sentences that this change makes false) is corrected here ## 维护者速读 - **改了什么**:`user` 类型审批人现在会先查目录(`sys_user`,按 id 或邮箱)。查不到这个账号时,它和部门、团队、岗位这些图类型一样:打同一条 `expanded to nobody` 警告,不占槽位。名单因此变空时,走节点的 `onEmptyApprovers`,默认是 `admin_rescue`。 - **为什么改**:以前写错的用户 id,或者已经删掉的账号,会作为一个谁都处理不了的幽灵槽位开出审批单。没有日志,也没有转交管理员,这张单子会永远挂着。 - **风险与回滚**:低。只有 `sys_user` 读成功、而且确实没有这一行时才丢槽位:读失败时保留原槽位;被封禁的账号、别的组织目录里的人、以及 OOO 委托到真实代理人的情况都不变。回滚就是 revert 这个 PR。 - **先看**:`packages/plugins/plugin-approvals/src/approval-service.ts` 的 `resolveApproverSpec` 里 `type === 'user'` 分支和共用的警告分支;`admin-rescue-named-slate.integration.test.ts` 新增的 describe;`approval-service.test.ts` 的 `ROUTED_USERS` 与 `putAccounts`(夹具改动)。 - **两分钟验证**:`pnpm --filter @objectstack/plugin-approvals exec vitest run src/admin-rescue-named-slate.integration.test.ts`,预期 22/22 通过,其中包含 "a `user` approver naming no user expands to nobody" 这一组。 - **需要你做的**:无,席位落地。 ## What changes - **The `user` approver branch** in `resolveApproverSpec`. A `user` value is now asked of the directory through the predicate the reassign door already uses (`reassignTargetRefusal`). That predicate looks for a `sys_user` row carrying the id, or the email for an email-shaped value. It is asked with no organization, so its tenancy screen does not run: `user` is directory-less, and ADR-0105 D9 leaves it untouched (the existing `approver-cross-org` pin holds that). When no account carries the value, the branch falls through to the graph types' existing warning (same text, now reached for `user` too) and returns no slot. A slate it leaves empty reaches the node's `onEmptyApprovers` by the existing path; there is no new branch and no new warning text. - **What counts as "no such user"**: a successful `sys_user` read that returns no row. These cases still keep the slot: - a **banned** account. The team and department expansions do not read the flag either; I measured this on `expandTeamUsers` and `expandBusinessUnitUsers`. - a **failed read**. A failed read proves nothing, which is this file's fail-open posture on addressing paths. A new unit pin covers it. - a value spelled as a `TYPE:VALUE` literal. `slateNamesAPerson` already reads that as naming no one. - **Checked after OOO delegation**: the check asks about the slot the route ends on. A removed account whose out-of-office delegation names a live delegate still routes to the delegate (the existing #24065 notification-body pin). - **A mixed slate** (one real user plus one phantom) keeps the real one, drops only the phantom, and logs the warning once for the phantom. That is the same per-entry warning a graph entry beside a person logs. - **Docs**: `content/docs/automation/approvals.mdx` said that a `user` entry never resolves to nobody. Two sentences are corrected. - **Fixtures**: the unit doubles started with an empty `sys_user`, so every `user` approver in them was a phantom by the new rule. They now start with the accounts each suite routes to (`ROUTED_USERS`, plus `putAccounts` where a test writes a named row). Three dogfood fixtures used an address deliberately carried by no account: `CO_APPROVER` (two of them) and `DEPARTED_APPROVER`. Each pin now signs that address up as an account that never acts, so the scene each pin measures is unchanged. The test-typecheck ledger shrinks by two, because a replaced test took two pre-existing errors with it. ## Tests The PIN is through the real `ApprovalService.openNodeRequest` on a real ObjectQL plus SqlDriver, with real administrators. It is a new describe in `admin-rescue-named-slate.integration.test.ts`: - a `user` approver naming `usr_does_not_exist` takes the default `admin_rescue` route. The request opens on `u_admin` and `u_owner`, and they are told. `approver 'user:usr_does_not_exist' expanded to nobody` is logged once, and the routing line is logged once. `pending_approvers` holds no `usr_does_not_exist`, and `admin_rescue:abandoned` is `[]`. - a mixed list `[u_alice, usr_does_not_exist]` opens on `['u_alice']` alone, with the phantom's warning logged once and no administrator routing. - an email that no account carries is dropped. An email that an account carries keeps its slot. - CONTROL: a real user keeps the slot, with no expanded-to-nobody line. A person of another organization's directory keeps the slot too (ADR-0105 D9). The unit pins are in `approval-service.test.ts`. The old "stays quiet for `user`" test, which pinned the phantom slot, is replaced by three tests: - the warning and its meta, with no slot; - a control in which a real account stays quiet; - a failed `sys_user` read keeps the slot. <details><summary>Red first, then green</summary> Before the fix, the new describe at base `a75ddba1` gave 4 failed and 17 passed (21): `expected [ 'usr_does_not_exist' ] to deeply equal [ 'u_admin', 'u_owner' ]` · `expected [ 'u_alice', 'usr_does_not_exist' ] to deeply equal [ 'u_alice' ]` · the email leg · (and the other-org leg, which was later turned into the D9 control). At final head ``10a61cc4``, with the closure rebuilt after the `origin/main` merge, the run under the verify lock gave: `pnpm --filter @objectstack/plugin-approvals exec vitest run`: Test Files 105 passed (105), Tests 1338 passed (1338). `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0, `check:test-typecheck: OK` (8 files / 321 errors / 27 pinned signatures). `pnpm --filter @objectstack/http-conformance exec vitest run src/hono-approvals-act.conformance.test.ts`: 4/4. It covers an unregistered `sys_user`: the read throws and the slot is kept. </details> <details><summary>Ablations (scripts/ablation-replace.mjs; the suite reads src, so no dist leg)</summary> 1. `if (refusal === undefined) return routed;` was replaced with `return routed;`, which removes the existence check. Result: 4 failed and 405 passed (409). The failures were the three integration legs and the unit warning pin, for example `expected [ 'usr_does_not_exist' ] to deeply equal [ 'u_admin', 'u_owner' ]`. The controls stayed green. The restore was checked: blob `ae44df7ffede` == HEAD, and `git diff HEAD` was empty. 2. `if (GRAPH_APPROVER_TYPES.has(type) || type === 'user') {` was replaced with `if (GRAPH_APPROVER_TYPES.has(type)) {`, which removes the shared warning. Result: 3 failed and 406 passed (409): `expected [] to have a length of 1 but got +0` (twice) and `no warning for user: expected undefined to be truthy`. The restore was checked: blob == HEAD, and `git diff HEAD` was empty. </details> ## Acceptance notes - Out of scope, and not fixed here: `type: 'field'` has the same unchecked shape. A record field holding a user id that no account carries opens a phantom slot in the same way (`resolveApproverSpec`, the `field` branch, `csvSplit(record[value])` straight to `applyOooDelegation`). Not filed by this PR; it is named in the report for the seat. - Out of scope, and not fixed here: a delegation's `delegate_id` is not existence-checked either. An OOO hop to a removed account would open a phantom slot the same way. Noted, not filed. - `examples/app-showcase` `approver-bindings.flow.ts` carries `{ type: 'user', value: '' }` on an `obsolete` flow. If that flow ever runs, it now logs `approver 'user:' expanded to nobody`, where before it was silent. The empty slot was already dropped before this change. - The dogfood battery does not run on pull requests (its `if:` excludes them), so the dogfood fixture edits are verified by the local run below, not by CI on this PR. ## Verification <details><summary>Gates on `10a61cc4`</summary> `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 94 command(s) on `10a61cc4`. I ran every one at that head and captured each exit code before any pipe: **94/94 exit 0**. `dispatch-gates --ran`: `Run reconciliation — 94 derived, 94 run, 0 NOT-MEASURED, 0 UNRUN` · `a DERIVED zero — all 94 recorded an exit code and none of them is 3`. I also ran these locally, outside the derived set: - eslint `--no-inline-config` over `packages/plugins/plugin-approvals` and the six dogfood files: exit 0. - the package test suite and typecheck (above). - the http-conformance approvals pin. - the nine approval dogfood files that open `user` approvers (override-composite, my-pending-position, position-address-readers, admin-rescue-slate, notification-body, pending-approver-record-read, record-lock-read, sla-reassign-target-resolves, seed-ownership-claim-dispatch). Run under the verify lock after building the dogfood closure: Test Files 9 passed (9), Tests 9 passed (9). `merge-tree` against the three open PRs that touch `packages/qa/dogfood/test/` (#27713, #27667, #27649) shows no conflict on any file this PR changes. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b2b4972 commit 2dc69d7

24 files changed

Lines changed: 307 additions & 53 deletions
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
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
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
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.
11+
12+
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:
13+
14+
- the approver logs the same `approver 'user:<value>' expanded to nobody` warning the team, department, position and membership-tier approvers log;
15+
- it adds no slot to the request;
16+
- 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;
17+
- beside other approvers, only the unresolvable one is dropped, and the others carry the request.
18+
19+
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‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,8 +320,8 @@ node parses its config on entry — **not** at authoring time, and `os lint` has
320320
no pairing arm for this one. The refusal is loud, names both keys, and lands
321321
before the node opens any approval request.
322322

323-
Prefer a fallback target that cannot itself resolve empty (a literal `user`, or
324-
the `owner` membership tier). If it does resolve empty the node degrades to
323+
Prefer a fallback target that cannot itself resolve empty (a `user` naming a
324+
real account, or the `owner` membership tier). If it does resolve empty the node degrades to
325325
`admin_rescue` — the request opens on the organization's administrators and the
326326
run is never killed — and the log says both that the fallback fired and that it
327327
found nobody.
@@ -422,8 +422,10 @@ organization's directory resolves them — see
422422
What an entry that resolves to nobody leaves on the slate depends on its kind:
423423

424424
- **A person entry** (`manager`, `field`) adds no slot. Nor does a `queue`
425-
with no members, or an `expression` that yields no ids. A `user` entry never
426-
resolves to nobody: its value is the slot.
425+
with no members, or an `expression` that yields no ids. A `user` entry's
426+
value is the slot when an account carries that id or email; one that no
427+
account carries adds no slot, and logs the same `expanded to nobody` warning
428+
as a group entry.
427429
- **A group entry** (`position`, `team`, `department`, `org_membership_level`)
428430
keeps its `<type>:<value>` literal as a slot, and so does an
429431
`expression` value that `resolveAs` re-expands into a group with nobody in it.

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,16 @@ import { bindApprovalLockHook, bindDelegationWriteGuard } from './lifecycle-hook
7171

7272
interface FakeRow { [k: string]: any }
7373

74+
/**
75+
* The accounts this suite routes `user` approver slots to: a `user` approver
76+
* holds a slot only when an account carries it (#27397), so the double's
77+
* directory starts with them.
78+
*/
79+
const ROUTED_USERS = ['u9'];
80+
7481
/** The same minimal engine shape `approval-service.test.ts` uses. */
7582
function makeFakeEngine() {
76-
const tables: Record<string, FakeRow[]> = {};
83+
const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
7784
const ensure = (n: string) => (tables[n] ??= []);
7885
const hooks: Record<string, Array<{ handler: (ctx: any) => any; object?: string | string[]; packageId?: string }>> = {};
7986

@@ -253,7 +260,7 @@ describe('#4839 evidence — the sanctioned admin path covers an unavailable app
253260
bindApprovalLockHook(engine as any);
254261
engine._tables['opportunity'] = [{ id: 'opp1', amount: 100, stage: 'new' }];
255262
// The substitute has an account: a reassign target must name one (#25485).
256-
engine._tables['sys_user'] = [{ id: 'u7' }];
263+
engine._tables['sys_user'].push({ id: 'u7' });
257264
});
258265

259266
/** An ordinary member's edit of the approval's target record. */

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,57 @@ describe('an empty admin_rescue slate opens on the organization\'s administrator
291291
);
292292
});
293293

294+
// A `user` approver whose value names no user expands to nobody, exactly as
295+
// an unstaffed graph target does: the same per-slot warning, no slot of its
296+
// own, and the node's onEmptyApprovers decides a slate this leaves empty —
297+
// here the default admin_rescue. Before, the bare value went onto the slate
298+
// as a slot no one could ever take, and nothing was logged.
299+
describe('a `user` approver naming no user expands to nobody (#27397)', () => {
300+
const PHANTOM = { type: 'user', value: 'usr_does_not_exist' };
301+
const phantomLine = "approver 'user:usr_does_not_exist' expanded to nobody";
302+
303+
it('alone, it takes the default admin_rescue route with the graph types\' warning, and holds no slot', async () => {
304+
const before = warnings.length;
305+
const row = await open('phantom_user', [PHANTOM]);
306+
expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']);
307+
expect(row.pending_approvers).not.toContain('usr_does_not_exist');
308+
expect(told(row.id)).toEqual(['u_admin', 'u_owner']);
309+
const lines = warnings.slice(before);
310+
expect(lines.filter((w) => w.includes(phantomLine))).toHaveLength(1);
311+
expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toHaveLength(1);
312+
expect((await snapshotOf(row.id)).__resolvedFrom['admin_rescue:abandoned']).toEqual([]);
313+
});
314+
315+
it('beside a real user, only the phantom is dropped — and it is still named in the log', async () => {
316+
const before = warnings.length;
317+
const row = await open('phantom_mixed', [{ type: 'user', value: 'u_alice' }, PHANTOM]);
318+
expect(row.pending_approvers).toEqual(['u_alice']);
319+
expect(told(row.id)).toEqual(['u_alice']);
320+
const lines = warnings.slice(before);
321+
expect(lines.filter((w) => w.includes(phantomLine))).toHaveLength(1);
322+
expect(lines.filter((w) => w.includes("opening on this organization's administrators"))).toEqual([]);
323+
});
324+
325+
it('an email no account carries is dropped; one an account carries keeps its slot', async () => {
326+
const row = await open('phantom_email', [
327+
{ type: 'user', value: 'u_alice@example.com' }, { type: 'user', value: 'nobody@example.com' },
328+
]);
329+
expect(row.pending_approvers).toEqual(['u_alice@example.com']);
330+
});
331+
332+
it('CONTROL: a person of another organization\'s directory, named outright, keeps the slot (ADR-0105 D9)', async () => {
333+
const row = await open('other_org_user', [{ type: 'user', value: 'u_other_owner' }]);
334+
expect(row.pending_approvers).toEqual(['u_other_owner']);
335+
});
336+
337+
it('CONTROL: a real user keeps the slot and no expanded-to-nobody line is logged', async () => {
338+
const before = warnings.length;
339+
const row = await open('real_user', [{ type: 'user', value: 'u_alice' }]);
340+
expect(row.pending_approvers).toEqual(['u_alice']);
341+
expect(warnings.slice(before).filter((w) => w.includes('expanded to nobody'))).toEqual([]);
342+
});
343+
});
344+
294345
// Last of the cases whose slate is `org_a`'s administrators: the delegation
295346
// it writes stays, and no case after it reads that slate.
296347
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‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,16 @@ import { ApprovalService, SLA_ACTOR_ID } from './approval-service.js';
3232

3333
interface FakeRow { [k: string]: any }
3434

35+
/**
36+
* The accounts this suite routes `user` approver slots to: a `user` approver
37+
* holds a slot only when an account carries it (#27397), so the double's
38+
* directory starts with them.
39+
*/
40+
const ROUTED_USERS = ['bob', 'carol'];
41+
3542
/** Equality/`$in`/`$ne`/`$contains` WHERE matcher — mirrors approval-service.test.ts. */
3643
function makeFakeEngine() {
37-
const tables: Record<string, FakeRow[]> = {};
44+
const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
3845
const ensure = (n: string) => (tables[n] ??= []);
3946

4047
function matches(row: FakeRow, filter: any): boolean {

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ const noopLogger = {
1111
info() {}, warn() {}, error() {}, debug() {},
1212
};
1313

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

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,16 @@ import { ApprovalService } from './approval-service.js';
2424

2525
interface FakeRow { [k: string]: any }
2626

27+
/**
28+
* The accounts this suite routes `user` approver slots to: a `user` approver
29+
* holds a slot only when an account carries it (#27397), so the double's
30+
* directory starts with them.
31+
*/
32+
const ROUTED_USERS = ['u9'];
33+
2734
/** The same minimal engine shape `approval-service.test.ts` uses. */
2835
function makeFakeEngine() {
29-
const tables: Record<string, FakeRow[]> = {};
36+
const tables: Record<string, FakeRow[]> = { sys_user: ROUTED_USERS.map((id) => ({ id })) };
3037
const ensure = (n: string) => (tables[n] ??= []);
3138
function matches(row: FakeRow, filter: any): boolean {
3239
if (!filter || typeof filter !== 'object') return true;
@@ -103,7 +110,7 @@ describe('approval override audit marker (#4466)', () => {
103110
clock: { now: () => new Date(baseTime + (n++) * 1000) },
104111
});
105112
// The reassign target has an account: a reassign target must name one (#25485).
106-
engine._tables['sys_user'] = [{ id: 'u7' }];
113+
engine._tables['sys_user'].push({ id: 'u7' });
107114
});
108115

109116
/** A request whose slate is properly STAFFED — one real, designated user. */

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,16 @@ import { registerApprovalNode } from './approval-node.js';
3434
const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any;
3535
const noopLogger = { info() {}, warn() {}, error() {}, debug() {} };
3636

37+
/**
38+
* The accounts this suite routes `user` approver slots to: a `user` approver
39+
* holds a slot only when an account carries it (#27397), so the double's
40+
* directory starts with them.
41+
*/
42+
const ROUTED_USERS = ['u1'];
43+
3744
/** In-memory ObjectQL stand-in — the approvals tables outlive the "restart". */
3845
function makeFakeEngine() {
39-
const tables = new Map<string, any[]>();
46+
const tables = new Map<string, any[]>([['sys_user', ROUTED_USERS.map((id) => ({ id }))]]);
4047
const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
4148
const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
4249
if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,16 @@ const asUser = (userId: string) =>
3333

3434
const noopLogger = { info() {}, warn() {}, error() {}, debug() {} };
3535

36+
/**
37+
* The accounts this suite routes `user` approver slots to: a `user` approver
38+
* holds a slot only when an account carries it (#27397), so the double's
39+
* directory starts with them.
40+
*/
41+
const ROUTED_USERS = ['u1', 'u2'];
42+
3643
/** In-memory ObjectQL stand-in: equality/$in where, orderBy, limit. */
3744
function makeFakeEngine() {
38-
const tables = new Map<string, any[]>();
45+
const tables = new Map<string, any[]>([['sys_user', ROUTED_USERS.map((id) => ({ id }))]]);
3946
const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
4047
const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
4148
if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`);

0 commit comments

Comments
 (0)