Skip to content

Commit e874ea2

Browse files
committed
fix(approvals): defense-in-depth for empty-approver routing (#3424)
Two authoring-time / demo-time complements to the admin-override recovery: - lint: new advisory rule `approval-approvers-may-resolve-empty` (info) fires when EVERY approver on an approval node routes to a group that can be empty (position/team/department) — the exact shape that dead-ends when nobody holds the group. It prescribes a guaranteed-staffed fallback (`{ type: 'org_membership_level', value: 'owner' }`). Advisory, since staffing is runtime data a linter can't see; individual/owner-tier fallbacks suppress it. Non-gating (info → suggestion). - showcase: the seed staffed `manager`/`finance`/`legal` but not `exec`, the second tier of `showcase_budget_approval` (manager → exec) — a user driving a budget approval to step 2 hit the exact #3424 dead-end. Staff `exec` too so every position the showcase flows route to is actionable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VmQPXXbgomoqrXtoxr3CS2
1 parent 26dee90 commit e874ea2

4 files changed

Lines changed: 108 additions & 2 deletions

File tree

examples/app-showcase/src/security/seed-approval-demo.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,16 @@ const SYS = { isSystem: true } as const;
4141

4242
const ADMIN_EMAIL = 'admin@objectos.ai';
4343

44-
/** Positions the admin is granted so they resolve as an approver on the demos. */
45-
const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal'] as const;
44+
/**
45+
* Positions the admin is granted so they resolve as an approver on the demos.
46+
* Covers every `{ type: 'position' }` the showcase approval flows route to and
47+
* expect the admin to hold — including `exec`, the SECOND tier of
48+
* `showcase_budget_approval` (manager → exec). Without `exec` a budget approval
49+
* that a user drives to step 2 routes to an unstaffed position and dead-ends
50+
* (framework#3424): undecidable request, record locked. Keep this in sync with
51+
* the position values authored in `automation/flows`.
52+
*/
53+
const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal', 'exec'] as const;
4654

4755
/** A phone-based demo persona (§6 "phone sign-in surfaces"). */
4856
const PHONE_DEMO_USER = {

packages/lint/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export {
106106
APPROVAL_APPROVER_TYPE_DEPRECATED,
107107
APPROVAL_APPROVER_TYPE_UNKNOWN,
108108
APPROVAL_ESCALATION_REASSIGN_NO_TARGET,
109+
APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
109110
} from './validate-approval-approvers.js';
110111
export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js';
111112

packages/lint/src/validate-approval-approvers.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
APPROVAL_APPROVER_TYPE_DEPRECATED,
88
APPROVAL_APPROVER_TYPE_UNKNOWN,
99
APPROVAL_ESCALATION_REASSIGN_NO_TARGET,
10+
APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
1011
} from './validate-approval-approvers.js';
1112

1213
function stackWithApprovers(approvers: unknown[]): Record<string, unknown> {
@@ -120,6 +121,56 @@ describe('validateApprovalApprovers', () => {
120121
expect(validateApprovalApprovers(stack)).toEqual([]);
121122
});
122123

124+
// ── empty-slate dead-end (#3424) ─────────────────────────────────────────
125+
126+
it('flags a node routed only to a single group (unstaffed-position dead-end)', () => {
127+
const findings = validateApprovalApprovers(stackWithApprovers([
128+
{ type: 'position', value: 'exec' },
129+
]));
130+
expect(findings).toHaveLength(1);
131+
expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY);
132+
expect(findings[0].severity).toBe('info');
133+
expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers');
134+
expect(findings[0].message).toContain('locked'); // lockRecord defaults true
135+
expect(findings[0].hint).toContain("org_membership_level', value: 'owner'");
136+
});
137+
138+
it('flags a node routed only to groups (all position/team/department)', () => {
139+
const findings = validateApprovalApprovers(stackWithApprovers([
140+
{ type: 'position', value: 'finance' },
141+
{ type: 'team', value: 't1' },
142+
{ type: 'department', value: 'bu_sales' },
143+
]));
144+
expect(findings.filter(f => f.rule === APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)).toHaveLength(1);
145+
});
146+
147+
it('does NOT flag when a guaranteed-staffed or individual fallback is present', () => {
148+
// position + owner tier — the prescribed fallback.
149+
expect(validateApprovalApprovers(stackWithApprovers([
150+
{ type: 'position', value: 'exec' },
151+
{ type: 'org_membership_level', value: 'owner' },
152+
]))).toEqual([]);
153+
// position + a specific user.
154+
expect(validateApprovalApprovers(stackWithApprovers([
155+
{ type: 'position', value: 'exec' },
156+
{ type: 'user', value: 'u1' },
157+
]))).toEqual([]);
158+
// position + the submitter's manager.
159+
expect(validateApprovalApprovers(stackWithApprovers([
160+
{ type: 'position', value: 'exec' },
161+
{ type: 'manager' },
162+
]))).toEqual([]);
163+
});
164+
165+
it('drops the record-lock clause when lockRecord is false', () => {
166+
const stack = stackWithApprovers([{ type: 'position', value: 'exec' }]);
167+
(stack.flows as any)[0].nodes[1].config.lockRecord = false;
168+
const findings = validateApprovalApprovers(stack);
169+
expect(findings).toHaveLength(1);
170+
expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY);
171+
expect(findings[0].message).not.toContain('locked');
172+
});
173+
123174
it('only scans approval nodes and tolerates malformed shapes', () => {
124175
const findings = validateApprovalApprovers({
125176
flows: [{

packages/lint/src/validate-approval-approvers.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
* | approval-approver-type-deprecated | warning | ADR-0090 D3 (#3133) |
2121
* | approval-approver-type-unknown | warning | contract-first (PD #12) |
2222
* | approval-escalation-reassign-no-target | warning | silent notify degradation |
23+
* | approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) |
2324
*
2425
* The first two are mutually exclusive by construction — a bad *value* wins,
2526
* because its fix (`position`) differs from the deprecation's fix
@@ -44,6 +45,19 @@ export const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER = 'approval-approver-not-memb
4445
export const APPROVAL_APPROVER_TYPE_DEPRECATED = 'approval-approver-type-deprecated';
4546
export const APPROVAL_APPROVER_TYPE_UNKNOWN = 'approval-approver-type-unknown';
4647
export const APPROVAL_ESCALATION_REASSIGN_NO_TARGET = 'approval-escalation-reassign-no-target';
48+
export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-resolve-empty';
49+
50+
/**
51+
* Approver types that route to a GROUP whose membership is runtime data and can
52+
* be empty (an unstaffed position, an empty team/department). When EVERY
53+
* approver on a node is one of these, the node can resolve to an empty slate at
54+
* runtime — the framework#3424 dead-end. Individually-routed types
55+
* (`user`/`field`/`manager`), the guaranteed-staffed `org_membership_level`
56+
* tiers, and the opaque `queue` are deliberately excluded: any of them present
57+
* signals the author has a non-group route, so the node isn't purely
58+
* group-gated.
59+
*/
60+
const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']);
4761

4862
export type ApprovalApproverSeverity = 'error' | 'warning' | 'info';
4963

@@ -171,6 +185,38 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin
171185
}
172186
}
173187

188+
// Empty-slate dead-end (#3424): when EVERY approver on the node routes to
189+
// a group whose membership can be empty (an unstaffed position, an empty
190+
// team/department), the request can resolve to an empty `pending_approvers`
191+
// at runtime — no concrete user can act, and with `lockRecord` the record
192+
// stays locked with no recovery except a platform/tenant admin override.
193+
// Advisory (`info`): staffing is runtime data a linter can't see, so this
194+
// flags the risky SHAPE and prescribes a guaranteed-staffed fallback.
195+
const routable = approvers.filter(
196+
(a) => a && typeof a === 'object' && typeof (a as AnyRec).type === 'string',
197+
);
198+
if (
199+
routable.length > 0 &&
200+
routable.every((a) => GROUP_ROUTED_TYPES.has(canonicalApproverType(String((a as AnyRec).type))))
201+
) {
202+
const locks = (cfg as AnyRec).lockRecord !== false; // default true
203+
findings.push({
204+
severity: 'info',
205+
rule: APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
206+
where,
207+
path: `flows[${fi}].nodes[${ni}].config.approvers`,
208+
message:
209+
`every approver on this node routes to a group (position/team/department) whose ` +
210+
`members are runtime data — if none is staffed, the request resolves to an empty ` +
211+
`slate and waits forever` +
212+
(locks ? `, and (lockRecord) the record stays locked with no in-product recovery.` : `.`),
213+
hint:
214+
`Make sure at least one target is always staffed, or add a guaranteed-staffed ` +
215+
`fallback approver, e.g. { type: 'org_membership_level', value: 'owner' }. A request ` +
216+
`that still lands empty is recoverable only by a platform/tenant admin override (#3424).`,
217+
});
218+
}
219+
174220
// escalation.action 'reassign' with no escalateTo silently degrades to a
175221
// plain SLA-breach notification at runtime — the hand-off the author
176222
// asked for never happens.

0 commit comments

Comments
 (0)