Skip to content

Commit 596ee7a

Browse files
committed
fix(plugin-approvals)!: an approval node refuses to open when a team, position or queue directory read fails; queue reads page to completion
Part 1 of #29604 (seat ruling 6648946388, route A). - plugin-approvals: a failed directory read behind a team, position or queue approver (and an expression's resolveAs team/position value) throws ApproverDirectoryUnreadableError, so openNodeRequest opens on no slate under any onEmptyApprovers policy, the fallback slate included. decide's pre-snapshot re-resolution keeps the literal slot. - plugin-sharing: the queue service's authored sys_queue / sys_queue_member reads go through the shared keyset walk instead of a 10,000-row cut; a failed read keeps today's answer until part 2. Claude-Session: https://claude.ai/code/session_01Utp6CXqXpuVf14Mqo1pznh Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6f095b9 commit 596ee7a

2 files changed

Lines changed: 178 additions & 29 deletions

File tree

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

Lines changed: 134 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,6 +1163,73 @@ function holderReadRefusal(scope: string, err: unknown): Error {
11631163
), { cause: err });
11641164
}
11651165

1166+
/**
1167+
* [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The
1168+
* approver types whose failed directory read refuses the approval node. A
1169+
* `queue`, `team` or `position` approver names a SET of people read from the
1170+
* directory at node entry; when that read fails, the slate it would build is
1171+
* missing people nobody can name, and every `onEmptyApprovers` policy would act
1172+
* on that wrong slate — open on the rest of it, open on the administrators or on
1173+
* a fallback, or approve. So the node opens on none of them.
1174+
*
1175+
* An `expression` approver's `resolveAs: 'team' | 'position'` values are read
1176+
* through the same expanders, and refuse the same way ({@link
1177+
* ApprovalService.resolveExpressionApprovers}). The other graph kinds
1178+
* (`department` and its dialects, `org_membership_level`) are not named by the
1179+
* ruling and keep their literal slot.
1180+
*/
1181+
const DIRECTORY_REFUSING_APPROVER_TYPES: ReadonlySet<string> = new Set(['queue', 'team', 'position']);
1182+
1183+
/**
1184+
* [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The
1185+
* approval node refuses to open because a directory read behind one of its
1186+
* approvers failed ({@link DIRECTORY_REFUSING_APPROVER_TYPES}).
1187+
*
1188+
* Its own class, and never `NO_APPROVERS` or "expanded to nobody": the slate
1189+
* was not found empty, it could not be read. The message names the approver
1190+
* and the read that failed, and starts with `APPROVER_DIRECTORY_UNREADABLE:` in
1191+
* the message-prefix family this service's refusals use. Retryable: nothing is
1192+
* written before it is thrown, so the run can be started again once the
1193+
* directory reads. No parked or auto-retry state is part of it.
1194+
*/
1195+
export class ApproverDirectoryUnreadableError extends Error {
1196+
override readonly name = 'ApproverDirectoryUnreadableError';
1197+
/** The run can be re-triggered once the directory reads again. */
1198+
readonly retryable = true;
1199+
/** The approver whose directory read failed: its type and value as authored. */
1200+
readonly approverType: string;
1201+
readonly approverValue: string;
1202+
/** What could not be read, as the read reported it. */
1203+
readonly failedRead: string;
1204+
1205+
constructor(input: {
1206+
type: string;
1207+
value: unknown;
1208+
organizationId: string | null | undefined;
1209+
failedRead: string;
1210+
/** The CEL source, when the approver is an `expression` resolved as `type`. */
1211+
expression?: string;
1212+
cause?: unknown;
1213+
}) {
1214+
const approver = input.expression === undefined
1215+
? `approver '${input.type}:${input.value}'`
1216+
: `approver 'expression' (resolveAs '${input.type}', value '${input.value}')`;
1217+
const inOrg = input.organizationId ? ` in organization '${input.organizationId}'` : '';
1218+
const read = input.failedRead.trim();
1219+
super(
1220+
`APPROVER_DIRECTORY_UNREADABLE: ${approver} could not be expanded${inOrg}, because a read behind it failed: `
1221+
+ `${read}${/[.!?]$/.test(read) ? '' : '.'} The approval node does not open: the people who could not be `
1222+
+ `read would be routed past, so the request is not opened on the rest of the slate, on the declared `
1223+
+ `fallbackApprovers or on the organization's administrators, and is not auto-approved, whatever the node's `
1224+
+ `onEmptyApprovers says. Run the flow again once the directory can be read.`,
1225+
);
1226+
if (input.cause !== undefined) Object.assign(this, { cause: input.cause });
1227+
this.approverType = input.type;
1228+
this.approverValue = String(input.value);
1229+
this.failedRead = input.failedRead;
1230+
}
1231+
}
1232+
11661233
/**
11671234
* Approver types resolved by QUERYING a graph rather than by taking `value`
11681235
* literally (#3807). Each can legitimately come back empty — an unstaffed
@@ -2432,9 +2499,19 @@ export class ApprovalService implements IApprovalService {
24322499
* as `__resolvedFrom` so "why these people" stays answerable later.
24332500
*/
24342501
resolvedFrom?: Record<string, unknown>;
2502+
/**
2503+
* [ADR-0142 D6 as amended] What a failed directory read behind a
2504+
* `queue` / `team` / `position` approver does: `'refuse'` (the default)
2505+
* throws {@link ApproverDirectoryUnreadableError}, so a node never opens on
2506+
* a slate it could not read; `'literal'` keeps the slot's `type:value`
2507+
* literal, for the one reader that re-resolves a slate AFTER a decision
2508+
* was recorded and must not fail it (`decide`'s pre-snapshot tally).
2509+
*/
2510+
onDirectoryFailure?: 'refuse' | 'literal';
24352511
},
24362512
): Promise<string[]> {
24372513
if (!step || !Array.isArray(step.approvers)) return [];
2514+
const onDirectoryFailure = opts?.onDirectoryFailure ?? 'refuse';
24382515
const now = opts?.now ?? this.clock.now().getTime();
24392516
const out: string[] = [];
24402517
// A person is a member of a per_group group once (#28684): the tally sizes
@@ -2468,7 +2545,7 @@ export class ApprovalService implements IApprovalService {
24682545
// contract of resolveApproverSpec cannot carry.
24692546
if (canonicalApproverType(String(a.type)) === 'expression') {
24702547
const resolved = await this.resolveExpressionApprovers(
2471-
a, record, organizationId, now, opts?.substitutions, opts?.exprCtx,
2548+
a, record, organizationId, now, opts?.substitutions, opts?.exprCtx, onDirectoryFailure,
24722549
);
24732550
if (opts?.resolvedFrom) opts.resolvedFrom[`expression#${idx}`] = resolved.raw;
24742551
for (const entry of resolved.slots) {
@@ -2484,7 +2561,7 @@ export class ApprovalService implements IApprovalService {
24842561
if (opts?.resolvedFrom && canonicalApproverType(String(a.type)) === 'field' && a.value != null) {
24852562
opts.resolvedFrom[`field:${a.value}`] = (record as any)?.[a.value] ?? null;
24862563
}
2487-
const ids = await this.resolveApproverSpec(a, record, organizationId, now, opts?.substitutions);
2564+
const ids = await this.resolveApproverSpec(a, record, organizationId, now, opts?.substitutions, onDirectoryFailure);
24882565
// per_group (#3266): tag each resolved id with this spec's group.
24892566
for (const u of ids) {
24902567
if (!u) continue;
@@ -2513,6 +2590,7 @@ export class ApprovalService implements IApprovalService {
25132590
organizationId: string | null | undefined,
25142591
now: number,
25152592
substitutions?: OooSubstitution[],
2593+
onDirectoryFailure: 'refuse' | 'literal' = 'refuse',
25162594
): Promise<string[]> {
25172595
// ADR-0090 D3: `role` is the deprecated spelling of `org_membership_level`.
25182596
// Resolve on the canonical type, and write the canonical type in the
@@ -2602,14 +2680,25 @@ export class ApprovalService implements IApprovalService {
26022680
//
26032681
// A cross-org slate is filtered to the people who can actually READ the
26042682
// request (D2 union); same-org routing is untouched and does no extra read.
2605-
const bounded = async (users: string[]): Promise<string[]> => (
2606-
crossOrg
2607-
? filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, {
2683+
//
2684+
// [#29604] A failure of the read filter itself is named as that read, not
2685+
// as the directory's, so the warning and the node's refusal below say which
2686+
// read could not be done.
2687+
const bounded = async (users: string[]): Promise<string[]> => {
2688+
if (!crossOrg) return users;
2689+
try {
2690+
return await filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, {
26082691
approverType: type, value: a.value != null ? String(a.value) : undefined,
26092692
directoryOrgId: directoryOrg,
2610-
})
2611-
: users
2612-
);
2693+
});
2694+
} catch (err) {
2695+
throw Object.assign(new Error(
2696+
`the cross-organization read filter (who among the people of organization '${directoryOrg}' can read `
2697+
+ `this request of organization '${organizationId}') could not be read: `
2698+
+ `${err instanceof Error ? err.message : String(err)}.`,
2699+
), { cause: err });
2700+
}
2701+
};
26132702

26142703
// The user a `manager` approver resolved FROM, kept for the warning below
26152704
// when no manager comes back.
@@ -2659,6 +2748,17 @@ export class ApprovalService implements IApprovalService {
26592748
// A directory lookup failed → handled below as resolving to nobody, and
26602749
// the graph-type warning names the failure (#29362).
26612750
directoryFailure = err instanceof Error ? err.message : String(err);
2751+
// [ADR-0142 D6 as amended; ruling record 6599731698, letter B] …unless
2752+
// the approver names a directory SET: then the node refuses to open,
2753+
// before any `onEmptyApprovers` policy can act on a slate missing the
2754+
// people who could not be read. Ahead of the `queue` branch's "expanded to
2755+
// nobody" return below, which would otherwise answer an unreadable queue
2756+
// as an empty one.
2757+
if (onDirectoryFailure === 'refuse' && DIRECTORY_REFUSING_APPROVER_TYPES.has(type)) {
2758+
throw new ApproverDirectoryUnreadableError({
2759+
type, value: a.value, organizationId: directoryOrg, failedRead: directoryFailure, cause: err,
2760+
});
2761+
}
26622762
}
26632763
// #22558: a PERSON type that resolved to nobody contributes NO slot — see
26642764
// PERSON_APPROVER_TYPES for why its `type:value` literal could never be
@@ -2695,7 +2795,19 @@ export class ApprovalService implements IApprovalService {
26952795
// are read at node entry, so a member who joins later is not owed this
26962796
// request. A `queue:` slot a stored request already holds stays as stored,
26972797
// decidable by the override (the ADR-0090 D3 `role:` precedent).
2798+
//
2799+
// A queue whose read FAILED reaches here only on the `'literal'` reader (a
2800+
// refusing one threw above); it still writes no slot, and its warning says
2801+
// the read failed rather than that the queue is empty.
26982802
if (type === 'queue') {
2803+
if (directoryFailure !== undefined) {
2804+
this.logger?.warn?.(
2805+
`[approvals] approver 'queue:${a.value}' could not be expanded — a directory or read-filter lookup failed. `
2806+
+ `It adds no slot to this slate, as an empty queue adds none; fix the read (see error).`,
2807+
{ type, value: a.value ?? null, organizationId: directoryOrg ?? null, error: directoryFailure },
2808+
);
2809+
return [];
2810+
}
26992811
this.logger?.warn?.(
27002812
`[approvals] approver 'queue:${a.value}' expanded to nobody — `
27012813
+ (this.queueSource()
@@ -2797,6 +2909,7 @@ export class ApprovalService implements IApprovalService {
27972909
now: number,
27982910
substitutions?: OooSubstitution[],
27992911
exprCtx?: ApproverExpressionContext,
2912+
onDirectoryFailure: 'refuse' | 'literal' = 'refuse',
28002913
): Promise<{ slots: Array<{ id: string; subGroup?: string }>; raw: string[] }> {
28012914
const source = String(a.value ?? '').trim();
28022915
if (!source) {
@@ -2916,6 +3029,15 @@ export class ApprovalService implements IApprovalService {
29163029
}
29173030
} catch (err: any) {
29183031
if (String(err?.message ?? '').startsWith('VALIDATION_FAILED')) throw err;
3032+
// [ADR-0142 D6 as amended; ruling record 6599731698, letter B] A
3033+
// `team` or `position` value is read through the same expanders as the
3034+
// static types, and refuses the node the same way.
3035+
if (onDirectoryFailure === 'refuse' && DIRECTORY_REFUSING_APPROVER_TYPES.has(resolveAs)) {
3036+
throw new ApproverDirectoryUnreadableError({
3037+
type: resolveAs, value: key, organizationId: directoryOrg,
3038+
failedRead: err instanceof Error ? err.message : String(err), expression: source, cause: err,
3039+
});
3040+
}
29193041
// [#29606] The literal slot below is the same fallback the static graph
29203042
// types take, and it says why exactly as they do: a failed read is not
29213043
// an unstaffed value, so an operator is pointed at the read rather than
@@ -4985,8 +5107,12 @@ export class ApprovalService implements IApprovalService {
49855107
groupMap = snapshotGroups;
49865108
original = Object.keys(snapshotGroups);
49875109
} else {
5110+
// The decision is already recorded above, so a directory outage here
5111+
// must not fail it: the legacy re-resolution keeps the literal slot it
5112+
// always kept (ADR-0142 D6's refusal is the node's, at open time).
49885113
original = await this.expandApprovers(
49895114
{ approvers: config.approvers }, parseJson(raw.payload_json, undefined), org,
5115+
{ onDirectoryFailure: 'literal' },
49905116
);
49915117
groupMap = {};
49925118
}

0 commit comments

Comments
 (0)