Skip to content

Commit e8fb0d3

Browse files
committed
refactor(plugin-approvals): hold only answers that were read in the pending approver's memo
A rejected read leaves nothing behind, with no cleanup step; the system context needs no branch of its own (the sharing filter bypasses it). Claude-Session: https://claude.ai/code/session_016NZtJpJqVWVJA1XDU9dbnz Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1def5ba commit e8fb0d3

1 file changed

Lines changed: 14 additions & 24 deletions

File tree

‎packages/plugins/plugin-approvals/src/pending-approver-read-grant.ts‎

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ import { actingAddresses } from './approver-address.js';
5252
* `sys_approval_request` read per object. The override holder pays one
5353
* `sys_approval_request` read per object. Never a read per row.
5454
*
55-
* A failed read is not memoized and is rethrown: the caller (plugin-sharing)
55+
* A failed read is never held and is rethrown: the caller (plugin-sharing)
5656
* fails closed and grants nothing.
5757
*/
5858

@@ -81,11 +81,12 @@ export const PENDING_APPROVER_READ_CAP = 10_000;
8181

8282
const NONE: PendingApproverReadGrants = new Map();
8383

84+
/** One request context's answers at one write epoch. Only an answer that was read is held. */
8485
interface MemoEntry {
8586
epoch: number;
8687
/** The pending request ids naming the caller as a person; `null` for the override holder. */
87-
slate?: Promise<readonly string[] | null>;
88-
byObject: Map<string, Promise<PendingApproverReadGrants>>;
88+
slate?: readonly string[] | null;
89+
byObject: Map<string, PendingApproverReadGrants>;
8990
}
9091

9192
/** The caller's active organization, read as the service reads it everywhere else. */
@@ -112,20 +113,17 @@ export class PendingApproverReadGrantSource {
112113

113114
/** The record ids of `object` the caller may read through a pending approval step. */
114115
async forObject(object: string, context: ExecutionContext): Promise<PendingApproverReadGrants> {
115-
if (!object || !context || context.isSystem) return NONE;
116+
// A system context never reaches here: the sharing filter bypasses it.
117+
if (!object || !context) return NONE;
116118
const userId = context.userId != null ? String(context.userId) : '';
117119
if (!userId) return NONE;
118120
const entry = this.entryFor(context);
119-
if (!entry) return this.compute(object, context, undefined);
120-
const cached = entry.byObject.get(object);
121+
const cached = entry?.byObject.get(object);
121122
if (cached) return cached;
122-
const pending = this.compute(object, context, entry);
123-
entry.byObject.set(object, pending);
124-
// A failed read is never reused: the next read asks again.
125-
pending.catch(() => {
126-
if (entry.byObject.get(object) === pending) entry.byObject.delete(object);
127-
});
128-
return pending;
123+
// A failed read rejects before anything is held: the next read asks again.
124+
const grants = await this.compute(object, context, entry);
125+
entry?.byObject.set(object, grants);
126+
return grants;
129127
}
130128

131129
/** The memo entry for this context at the current epoch, or `undefined` when nothing may be reused. */
@@ -145,18 +143,10 @@ export class PendingApproverReadGrantSource {
145143
entry: MemoEntry | undefined,
146144
): Promise<PendingApproverReadGrants> {
147145
const org = activeOrganization(context);
148-
let slate: readonly string[] | null;
149-
if (entry) {
150-
if (!entry.slate) {
151-
entry.slate = this.slateRequestIds(context, org);
152-
const held = entry.slate;
153-
held.catch(() => {
154-
if (entry.slate === held) entry.slate = undefined;
155-
});
156-
}
157-
slate = await entry.slate;
158-
} else {
146+
let slate = entry?.slate;
147+
if (slate === undefined) {
159148
slate = await this.slateRequestIds(context, org);
149+
if (entry) entry.slate = slate;
160150
}
161151
if (slate !== null && slate.length === 0) return NONE;
162152

0 commit comments

Comments
 (0)