From 279e6453fc5150619b364fd8c9d60082bdf12f17 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 28 Apr 2026 11:37:16 +0100 Subject: [PATCH] fix(gateway): make repeated approval resolves idempotent --- src/gateway/exec-approval-manager.ts | 15 ++++- src/gateway/server-methods/approval-shared.ts | 61 +++++++++++++++++++ .../server-methods/server-methods.test.ts | 58 +++++++++++++++++- 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 2579efbc511..4bc5e2bc059 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -33,6 +33,7 @@ export type ExecApprovalRecord = { requestedByClientId?: string | null; resolvedAtMs?: number; decision?: ExecApprovalDecision; + consumedDecision?: ExecApprovalDecision; resolvedBy?: string | null; }; @@ -181,6 +182,7 @@ export class ExecApprovalManager { } // One-time approvals must be consumed atomically so the same runId // cannot be replayed during the resolved-entry grace window. + record.consumedDecision = record.decision; record.decision = undefined; return true; } @@ -194,7 +196,10 @@ export class ExecApprovalManager { return entry?.promise ?? null; } - lookupPendingId(input: string): ExecApprovalIdLookupResult { + lookupApprovalId( + input: string, + opts: { includeResolved?: boolean } = {}, + ): ExecApprovalIdLookupResult { const normalized = input.trim(); if (!normalized) { return { kind: "none" }; @@ -202,7 +207,7 @@ export class ExecApprovalManager { const exact = this.pending.get(normalized); if (exact) { - return exact.record.resolvedAtMs === undefined + return opts.includeResolved || exact.record.resolvedAtMs === undefined ? { kind: "exact", id: normalized } : { kind: "none" }; } @@ -210,7 +215,7 @@ export class ExecApprovalManager { const lowerPrefix = normalizeLowercaseStringOrEmpty(normalized); const matches: string[] = []; for (const [id, entry] of this.pending.entries()) { - if (entry.record.resolvedAtMs !== undefined) { + if (!opts.includeResolved && entry.record.resolvedAtMs !== undefined) { continue; } if (normalizeLowercaseStringOrEmpty(id).startsWith(lowerPrefix)) { @@ -226,4 +231,8 @@ export class ExecApprovalManager { } return { kind: "none" }; } + + lookupPendingId(input: string): ExecApprovalIdLookupResult { + return this.lookupApprovalId(input); + } } diff --git a/src/gateway/server-methods/approval-shared.ts b/src/gateway/server-methods/approval-shared.ts index fabd220e0ec..677ce1277df 100644 --- a/src/gateway/server-methods/approval-shared.ts +++ b/src/gateway/server-methods/approval-shared.ts @@ -11,8 +11,19 @@ import type { GatewayClient, GatewayRequestContext, RespondFn } from "./types.js export const APPROVAL_NOT_FOUND_DETAILS = { reason: ErrorCodes.APPROVAL_NOT_FOUND, + remediation: "Re-request the action; pending approvals are cleared after expiry or restart.", } as const; +const APPROVAL_ALREADY_RESOLVED_DETAILS = { + reason: "APPROVAL_ALREADY_RESOLVED", +} as const; + +function resolveRecordedApprovalDecision( + record: ExecApprovalRecord, +): ExecApprovalDecision | undefined { + return record.decision ?? record.consumedDecision; +} + type PendingApprovalLookupError = | "missing" | { @@ -97,6 +108,37 @@ export function resolvePendingApprovalRecord(params: { return { ok: true, approvalId: resolvedId.id, snapshot }; } +function resolveResolvedApprovalRecord(params: { + manager: ExecApprovalManager; + inputId: string; + exposeAmbiguousPrefixError?: boolean; +}): + | { + ok: true; + approvalId: string; + snapshot: ExecApprovalRecord; + } + | { + ok: false; + response: PendingApprovalLookupError; + } { + const resolvedId = params.manager.lookupApprovalId(params.inputId, { includeResolved: true }); + if (resolvedId.kind !== "exact" && resolvedId.kind !== "prefix") { + return { + ok: false, + response: resolvePendingApprovalLookupError({ + resolvedId, + exposeAmbiguousPrefixError: params.exposeAmbiguousPrefixError, + }), + }; + } + const snapshot = params.manager.getSnapshot(resolvedId.id); + if (!snapshot || snapshot.resolvedAtMs === undefined) { + return { ok: false, response: "missing" }; + } + return { ok: true, approvalId: resolvedId.id, snapshot }; +} + export function respondPendingApprovalLookupError(params: { respond: RespondFn; response: PendingApprovalLookupError; @@ -259,6 +301,25 @@ export async function handleApprovalResolve { }, hasExecApprovalClients: () => true, }; - return { handlers, broadcasts, respond, context }; + return { manager, handlers, broadcasts, respond, context }; } function createForwardingExecApprovalFixture(opts?: { @@ -1011,6 +1011,62 @@ describe("exec approval handlers", () => { expect(broadcasts.some((entry) => entry.event === "exec.approval.resolved")).toBe(true); }); + it("treats duplicate same-decision exec resolves as idempotent during grace", async () => { + const { manager, handlers, broadcasts, respond, context } = createExecApprovalFixture(); + + const requestPromise = requestExecApproval({ + handlers, + respond, + context, + params: { id: "approval-repeat-1", twoPhase: true }, + }); + + const firstResolveRespond = vi.fn(); + await resolveExecApproval({ + handlers, + id: "approval-repeat-1", + respond: firstResolveRespond, + context, + }); + await requestPromise; + expect(manager.consumeAllowOnce("approval-repeat-1")).toBe(true); + + const resolvedBroadcastCount = broadcasts.filter( + (entry) => entry.event === "exec.approval.resolved", + ).length; + + const repeatResolveRespond = vi.fn(); + await resolveExecApproval({ + handlers, + id: "approval-repeat-1", + respond: repeatResolveRespond, + context, + }); + + const conflictingResolveRespond = vi.fn(); + await resolveExecApproval({ + handlers, + id: "approval-repeat-1", + decision: "deny", + respond: conflictingResolveRespond, + context, + }); + + expect(firstResolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined); + expect(repeatResolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined); + expect(broadcasts.filter((entry) => entry.event === "exec.approval.resolved")).toHaveLength( + resolvedBroadcastCount, + ); + expect(conflictingResolveRespond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: "approval already resolved", + details: expect.objectContaining({ reason: "APPROVAL_ALREADY_RESOLVED" }), + }), + ); + }); + it("rejects allow-always when the request ask mode is always", async () => { const { handlers, broadcasts, respond, context } = createExecApprovalFixture();