Skip to content

Commit 6bab93d

Browse files
Hidetsugu55Hidetsugu55
committed
fix(approvals): distinguish expired, already-resolved, and unknown approval ids
Resolving an exec/plugin approval that was not pending previously collapsed expired, already-resolved, and never-seen ids into a single "unknown or expired approval id" error, so operators could not tell whether to re-run the command or whether their decision had already landed. - ExecApprovalManager keeps a bounded FIFO archive of terminated records and exposes classifyApprovalId(), which respects caller visibility so it never leaks the existence of approvals the caller could not see. - handleApprovalResolve now reports an elapsed window as "approval expired" (reason APPROVAL_EXPIRED, with remediation) both inside the grace window and after the record is archived, and reports a post-grace decided approval as "approval already resolved". Genuinely unknown ids still return the existing message. Same-decision idempotent retries are unchanged. - Add isApprovalExpiredError / isApprovalAlreadyResolvedError detectors. Tests: exec-approval-manager (classify pending/resolved/expired/unknown, visibility, post-grace archive, FIFO eviction), approval-shared (in-grace and post-grace expired vs already-resolved), approval-errors detectors.
1 parent 8be3bee commit 6bab93d

6 files changed

Lines changed: 436 additions & 10 deletions

File tree

‎src/gateway/exec-approval-manager.test.ts‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,4 +87,106 @@ describe("ExecApprovalManager", () => {
8787
"approval expiry is unavailable",
8888
);
8989
});
90+
91+
describe("classifyApprovalId", () => {
92+
it("classifies pending, unknown, and unseen ids without leaking existence", () => {
93+
const manager = new ExecApprovalManager();
94+
const record = manager.create({ command: "echo ok" }, 60_000, "approval-classify-pending");
95+
void manager.register(record, 60_000);
96+
97+
expect(manager.classifyApprovalId("approval-classify-pending")).toBe("pending");
98+
expect(manager.classifyApprovalId("never-existed")).toBe("unknown");
99+
expect(manager.classifyApprovalId(" ")).toBe("unknown");
100+
// A record the caller cannot see must not be revealed as pending.
101+
expect(manager.classifyApprovalId("approval-classify-pending", { filter: () => false })).toBe(
102+
"unknown",
103+
);
104+
105+
manager.resolve("approval-classify-pending", "allow-once");
106+
});
107+
108+
it("distinguishes resolved from expired records still inside the grace window", () => {
109+
const manager = new ExecApprovalManager();
110+
const resolvedRecord = manager.create(
111+
{ command: "echo ok" },
112+
60_000,
113+
"approval-grace-resolved",
114+
);
115+
void manager.register(resolvedRecord, 60_000);
116+
expect(manager.resolve("approval-grace-resolved", "allow-once")).toBe(true);
117+
expect(manager.classifyApprovalId("approval-grace-resolved")).toBe("resolved");
118+
119+
const expiredRecord = manager.create(
120+
{ command: "echo ok" },
121+
60_000,
122+
"approval-grace-expired",
123+
);
124+
void manager.register(expiredRecord, 60_000);
125+
expect(manager.expire("approval-grace-expired")).toBe(true);
126+
expect(manager.classifyApprovalId("approval-grace-expired")).toBe("expired");
127+
});
128+
129+
it("keeps resolved/expired classification after the grace window archives the record", () => {
130+
vi.useFakeTimers();
131+
try {
132+
const manager = new ExecApprovalManager();
133+
const resolvedRecord = manager.create(
134+
{ command: "echo ok" },
135+
600_000,
136+
"approval-archive-resolved",
137+
);
138+
void manager.register(resolvedRecord, 600_000);
139+
manager.resolve("approval-archive-resolved", "allow-once");
140+
141+
const expiredRecord = manager.create(
142+
{ command: "echo ok" },
143+
600_000,
144+
"approval-archive-expired",
145+
);
146+
void manager.register(expiredRecord, 600_000);
147+
manager.expire("approval-archive-expired");
148+
149+
// Fire the 15s grace cleanup; the live records are dropped but archived.
150+
vi.advanceTimersByTime(15_000);
151+
152+
expect(manager.getSnapshot("approval-archive-resolved")).toBeNull();
153+
expect(manager.getSnapshot("approval-archive-expired")).toBeNull();
154+
expect(manager.classifyApprovalId("approval-archive-resolved")).toBe("resolved");
155+
expect(manager.classifyApprovalId("approval-archive-expired")).toBe("expired");
156+
} finally {
157+
vi.useRealTimers();
158+
}
159+
});
160+
161+
it("classifies a consumed allow-once approval as resolved", () => {
162+
const manager = new ExecApprovalManager();
163+
const record = manager.create({ command: "echo ok" }, 60_000, "approval-consumed");
164+
void manager.register(record, 60_000);
165+
manager.resolve("approval-consumed", "allow-once");
166+
expect(manager.consumeAllowOnce("approval-consumed")).toBe(true);
167+
// decision moved to consumedDecision; still a resolved terminal state.
168+
expect(manager.classifyApprovalId("approval-consumed")).toBe("resolved");
169+
});
170+
171+
it("evicts the oldest archived terminal record beyond the bounded cap", () => {
172+
vi.useFakeTimers();
173+
try {
174+
const manager = new ExecApprovalManager();
175+
// Cap is MAX_RECENTLY_TERMINATED (512). Archive one past it and assert FIFO eviction.
176+
const total = 513;
177+
for (let index = 0; index < total; index += 1) {
178+
const id = `approval-evict-${index}`;
179+
const record = manager.create({ command: "echo ok" }, 600_000, id);
180+
void manager.register(record, 600_000);
181+
manager.resolve(id, "allow-once");
182+
}
183+
vi.advanceTimersByTime(15_000);
184+
185+
expect(manager.classifyApprovalId("approval-evict-0")).toBe("unknown");
186+
expect(manager.classifyApprovalId(`approval-evict-${total - 1}`)).toBe("resolved");
187+
} finally {
188+
vi.useRealTimers();
189+
}
190+
});
191+
});
90192
});

‎src/gateway/exec-approval-manager.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,15 @@ import { resolveTimerTimeoutMs } from "../shared/number-coercion.js";
1212
// Grace period to keep resolved entries for late awaitDecision calls
1313
const RESOLVED_ENTRY_GRACE_MS = 15_000;
1414

15+
// Bounded archive of terminated (resolved/expired) approval ids so a resolve
16+
// attempt arriving after the grace window can still be told apart as
17+
// already-resolved vs expired vs genuinely unknown instead of collapsing to
18+
// "unknown or expired approval id". Capped to avoid unbounded growth.
19+
const MAX_RECENTLY_TERMINATED = 512;
20+
21+
/** Terminal classification of an approval id for resolve-time UX messaging. */
22+
export type ApprovalIdClassification = "pending" | "resolved" | "expired" | "unknown";
23+
1524
function unrefTimer(timer: ReturnType<typeof setTimeout>): void {
1625
const unref = (timer as { unref?: () => void }).unref;
1726
if (typeof unref === "function") {
@@ -30,6 +39,16 @@ function resolveApprovalTimeoutMs(timeoutMs: number): number {
3039
return resolveTimerTimeoutMs(timeoutMs, 1);
3140
}
3241

42+
/**
43+
* A terminated record carries a decision (including a consumed allow-once) when
44+
* it was resolved by an operator; an elapsed window leaves no decision.
45+
*/
46+
function terminalStateOf<TPayload>(record: ExecApprovalRecord<TPayload>): "resolved" | "expired" {
47+
return record.decision !== undefined || record.consumedDecision !== undefined
48+
? "resolved"
49+
: "expired";
50+
}
51+
3352
type ExecApprovalRequestPayload = InfraExecApprovalRequestPayload;
3453

3554
export type ExecApprovalRecord<TPayload = ExecApprovalRequestPayload> = {
@@ -63,6 +82,22 @@ export type ExecApprovalIdLookupResult =
6382

6483
export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
6584
private pending = new Map<string, PendingEntry<TPayload>>();
85+
// Insertion-ordered archive of recently terminated records (FIFO eviction).
86+
private recentlyTerminated = new Map<string, ExecApprovalRecord<TPayload>>();
87+
88+
private archiveTerminatedRecord(record: ExecApprovalRecord<TPayload>): void {
89+
// Refresh insertion order so the newest terminal state wins and survives
90+
// eviction longest.
91+
this.recentlyTerminated.delete(record.id);
92+
this.recentlyTerminated.set(record.id, record);
93+
while (this.recentlyTerminated.size > MAX_RECENTLY_TERMINATED) {
94+
const oldest = this.recentlyTerminated.keys().next().value;
95+
if (oldest === undefined) {
96+
break;
97+
}
98+
this.recentlyTerminated.delete(oldest);
99+
}
100+
}
66101

67102
create(request: TPayload, timeoutMs: number, id?: string | null): ExecApprovalRecord<TPayload> {
68103
const now = Date.now();
@@ -151,6 +186,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
151186
// Only delete if the entry hasn't been replaced
152187
if (this.pending.get(recordId) === pending) {
153188
this.pending.delete(recordId);
189+
this.archiveTerminatedRecord(pending.record);
154190
}
155191
});
156192
return true;
@@ -172,6 +208,7 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
172208
scheduleResolvedEntryCleanup(() => {
173209
if (this.pending.get(recordId) === pending) {
174210
this.pending.delete(recordId);
211+
this.archiveTerminatedRecord(pending.record);
175212
}
176213
});
177214
return true;
@@ -182,6 +219,32 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
182219
return entry?.record ?? null;
183220
}
184221

222+
/**
223+
* Classifies an approval id for resolve-time UX without leaking records the
224+
* caller cannot see. A terminated record with a recorded decision is
225+
* "resolved"; one whose window elapsed with no decision is "expired". Live
226+
* records still in the grace window are classified from their current state,
227+
* and ids the caller cannot see (or never existed) are "unknown".
228+
*/
229+
classifyApprovalId(
230+
input: string,
231+
opts: { filter?: (record: ExecApprovalRecord<TPayload>) => boolean } = {},
232+
): ApprovalIdClassification {
233+
const normalized = input.trim();
234+
if (!normalized) {
235+
return "unknown";
236+
}
237+
const live = this.pending.get(normalized)?.record;
238+
if (live && (opts.filter?.(live) ?? true)) {
239+
return live.resolvedAtMs === undefined ? "pending" : terminalStateOf(live);
240+
}
241+
const archived = this.recentlyTerminated.get(normalized);
242+
if (archived && (opts.filter?.(archived) ?? true)) {
243+
return terminalStateOf(archived);
244+
}
245+
return "unknown";
246+
}
247+
185248
listPendingRecords(): ExecApprovalRecord<TPayload>[] {
186249
return Array.from(this.pending.values())
187250
.map((entry) => entry.record)

‎src/gateway/server-methods/approval-shared.test.ts‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,128 @@ describe("handlePendingApprovalRequest", () => {
850850
expect(manager.getSnapshot(record.id)?.decision).toBeUndefined();
851851
});
852852

853+
it("reports an expired approval distinctly while it is still in the grace window", async () => {
854+
const manager = new ExecApprovalManager();
855+
const record = manager.create({ command: "echo ok" }, 60_000, "approval-expired-grace");
856+
void manager.register(record, 60_000);
857+
expect(manager.expire("approval-expired-grace")).toBe(true);
858+
const respond = vi.fn();
859+
860+
await handleApprovalResolve({
861+
manager,
862+
inputId: record.id,
863+
decision: "allow-once",
864+
respond,
865+
context: {
866+
broadcast: vi.fn(),
867+
broadcastToConnIds: vi.fn(),
868+
} as unknown as GatewayRequestContext,
869+
client: createApprovalClient({
870+
connId: "conn-operator",
871+
clientId: GATEWAY_CLIENT_IDS.IOS_APP,
872+
scopes: ["operator.admin"],
873+
}),
874+
resolvedEventName: "exec.approval.resolved",
875+
buildResolvedEvent: ({ approvalId, decision, snapshot }) => ({
876+
id: approvalId,
877+
decision,
878+
request: snapshot.request,
879+
}),
880+
});
881+
882+
expect(respond).toHaveBeenCalledWith(
883+
false,
884+
undefined,
885+
expect.objectContaining({
886+
code: "INVALID_REQUEST",
887+
message: "approval expired",
888+
details: expect.objectContaining({ reason: "APPROVAL_EXPIRED" }),
889+
}),
890+
);
891+
});
892+
893+
it("reports expired vs already-resolved distinctly after the grace window archives the record", async () => {
894+
vi.useFakeTimers();
895+
try {
896+
const manager = new ExecApprovalManager();
897+
const expiredRecord = manager.create(
898+
{ command: "echo ok" },
899+
600_000,
900+
"approval-expired-archived",
901+
);
902+
void manager.register(expiredRecord, 600_000);
903+
manager.expire("approval-expired-archived");
904+
const resolvedRecord = manager.create(
905+
{ command: "echo ok" },
906+
600_000,
907+
"approval-resolved-archived",
908+
);
909+
void manager.register(resolvedRecord, 600_000);
910+
manager.resolve("approval-resolved-archived", "allow-once");
911+
// Fire the grace cleanup so both live records move to the bounded archive.
912+
vi.advanceTimersByTime(15_000);
913+
914+
const client = createApprovalClient({
915+
connId: "conn-operator",
916+
clientId: GATEWAY_CLIENT_IDS.IOS_APP,
917+
scopes: ["operator.admin"],
918+
});
919+
const baseArgs = {
920+
manager,
921+
decision: "allow-once" as const,
922+
context: {
923+
broadcast: vi.fn(),
924+
broadcastToConnIds: vi.fn(),
925+
} as unknown as GatewayRequestContext,
926+
client,
927+
resolvedEventName: "exec.approval.resolved",
928+
buildResolvedEvent: ({
929+
approvalId,
930+
decision,
931+
snapshot,
932+
}: {
933+
approvalId: string;
934+
decision: string;
935+
snapshot: { request: unknown };
936+
}) => ({ id: approvalId, decision, request: snapshot.request }),
937+
};
938+
939+
const expiredRespond = vi.fn();
940+
await handleApprovalResolve({
941+
...baseArgs,
942+
inputId: "approval-expired-archived",
943+
respond: expiredRespond,
944+
});
945+
expect(expiredRespond).toHaveBeenCalledWith(
946+
false,
947+
undefined,
948+
expect.objectContaining({
949+
message: "approval expired",
950+
details: expect.objectContaining({ reason: "APPROVAL_EXPIRED" }),
951+
}),
952+
);
953+
954+
const resolvedRespond = vi.fn();
955+
await handleApprovalResolve({
956+
...baseArgs,
957+
inputId: "approval-resolved-archived",
958+
// Submit a different decision so this is not treated as an idempotent retry.
959+
decision: "deny",
960+
respond: resolvedRespond,
961+
});
962+
expect(resolvedRespond).toHaveBeenCalledWith(
963+
false,
964+
undefined,
965+
expect.objectContaining({
966+
message: "approval already resolved",
967+
details: expect.objectContaining({ reason: "APPROVAL_ALREADY_RESOLVED" }),
968+
}),
969+
);
970+
} finally {
971+
vi.useRealTimers();
972+
}
973+
});
974+
853975
it("does not wait on decisions for approvals hidden from the caller", async () => {
854976
const manager = new ExecApprovalManager();
855977
const record = manager.create(

0 commit comments

Comments
 (0)