From fa7eeca7ce08314d5781058c0fbfdf7a9ab38b40 Mon Sep 17 00:00:00 2001 From: rshirali Date: Wed, 28 Jan 2026 14:53:39 +0100 Subject: [PATCH 1/5] feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. --- src/gateway/exec-approval-manager.ts | 63 ++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 5e51f2abc..ed5dfec54 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -28,6 +28,7 @@ type PendingEntry = { resolve: (decision: ExecApprovalDecision | null) => void; reject: (err: Error) => void; timer: ReturnType; + promise: Promise; }; export class ExecApprovalManager { @@ -49,17 +50,47 @@ export class ExecApprovalManager { return record; } + /** + * Register an approval record and return a promise that resolves when the decision is made. + * This separates registration (synchronous) from waiting (async), allowing callers to + * confirm registration before the decision is made. + */ + register(record: ExecApprovalRecord, timeoutMs: number): Promise { + let resolvePromise: (decision: ExecApprovalDecision | null) => void; + let rejectPromise: (err: Error) => void; + const promise = new Promise((resolve, reject) => { + resolvePromise = resolve; + rejectPromise = reject; + }); + const timer = setTimeout(() => { + const entry = this.pending.get(record.id); + resolvePromise(null); + // Keep entry briefly for in-flight awaitDecision calls + setTimeout(() => { + if (this.pending.get(record.id) === entry) { + this.pending.delete(record.id); + } + }, 5000); + }, timeoutMs); + // Registration happens synchronously here + this.pending.set(record.id, { + record, + resolve: resolvePromise!, + reject: rejectPromise!, + timer, + promise, + }); + return promise; + } + + /** + * @deprecated Use register() instead for explicit separation of registration and waiting. + */ async waitForDecision( record: ExecApprovalRecord, timeoutMs: number, ): Promise { - return await new Promise((resolve, reject) => { - const timer = setTimeout(() => { - this.pending.delete(record.id); - resolve(null); - }, timeoutMs); - this.pending.set(record.id, { record, resolve, reject, timer }); - }); + return this.register(record, timeoutMs); } resolve(recordId: string, decision: ExecApprovalDecision, resolvedBy?: string | null): boolean { @@ -69,8 +100,15 @@ export class ExecApprovalManager { pending.record.resolvedAtMs = Date.now(); pending.record.decision = decision; pending.record.resolvedBy = resolvedBy ?? null; - this.pending.delete(recordId); + // Resolve the promise first, then delete after a grace period. + // This allows in-flight awaitDecision calls to find the resolved entry. pending.resolve(decision); + setTimeout(() => { + // Only delete if the entry hasn't been replaced + if (this.pending.get(recordId) === pending) { + this.pending.delete(recordId); + } + }, 5000); return true; } @@ -78,4 +116,13 @@ export class ExecApprovalManager { const entry = this.pending.get(recordId); return entry?.record ?? null; } + + /** + * Wait for decision on an already-registered approval. + * Returns the decision promise if the ID is pending, null otherwise. + */ + awaitDecision(recordId: string): Promise | null { + const entry = this.pending.get(recordId); + return entry?.promise ?? null; + } } From 20c2a9947698dce7397b5a00d532164244ba147b Mon Sep 17 00:00:00 2001 From: rshirali Date: Wed, 28 Jan 2026 14:53:52 +0100 Subject: [PATCH 2/5] feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. --- src/gateway/server-methods/exec-approval.ts | 46 ++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index 572afc58f..3b2afe55d 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -62,7 +62,9 @@ export function createExecApprovalHandlers( sessionKey: p.sessionKey ?? null, }; const record = manager.create(request, timeoutMs, explicitId); - const decisionPromise = manager.waitForDecision(record, timeoutMs); + // Use register() to synchronously add to pending map before sending any response. + // This ensures the approval ID is valid immediately after the "accepted" response. + const decisionPromise = manager.register(record, timeoutMs); context.broadcast( "exec.approval.requested", { @@ -83,7 +85,24 @@ export function createExecApprovalHandlers( .catch((err) => { context.logGateway?.error?.(`exec approvals: forward request failed: ${String(err)}`); }); + + // Send immediate "accepted" response so callers know the approval ID is registered. + // Callers using expectFinal:false will receive this and can return immediately. + // Callers using expectFinal:true will continue waiting for the decision. + // Note: "accepted" status is recognized by the gateway client for dual-response pattern. + respond( + true, + { + status: "accepted", + id: record.id, + createdAtMs: record.createdAtMs, + expiresAtMs: record.expiresAtMs, + }, + undefined, + ); + const decision = await decisionPromise; + // Send final response with decision for callers using expectFinal:true. respond( true, { @@ -95,6 +114,31 @@ export function createExecApprovalHandlers( undefined, ); }, + "exec.approval.waitDecision": async ({ params, respond }) => { + const p = params as { id?: string; timeoutMs?: number }; + const id = typeof p.id === "string" ? p.id.trim() : ""; + if (!id) { + respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "id is required")); + return; + } + const decisionPromise = manager.awaitDecision(id); + if (!decisionPromise) { + respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "unknown approval id")); + return; + } + const decision = await decisionPromise; + const snapshot = manager.getSnapshot(id); + respond( + true, + { + id, + decision, + createdAtMs: snapshot?.createdAtMs, + expiresAtMs: snapshot?.expiresAtMs, + }, + undefined, + ); + }, "exec.approval.resolve": async ({ params, respond, client, context }) => { if (!validateExecApprovalResolveParams(params)) { respond( From b1d409738b30f8b6301bedefcf19a687db9347be Mon Sep 17 00:00:00 2001 From: rshirali Date: Wed, 28 Jan 2026 14:54:03 +0100 Subject: [PATCH 3/5] fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes #2402 --- src/agents/bash-tools.exec.ts | 92 ++++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 28 deletions(-) diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index b9de81872..b67da4a48 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -1008,29 +1008,47 @@ export function createExecTool( if (requiresAsk) { const approvalId = crypto.randomUUID(); const approvalSlug = createApprovalSlug(approvalId); - const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; const contextKey = `exec:${approvalId}`; const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000)); const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; + // Register the approval with expectFinal:false to get immediate confirmation. + // This ensures the approval ID is valid before we return. + let expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + try { + const registrationResult = (await callGatewayTool( + "exec.approval.request", + { timeoutMs: 10_000 }, + { + id: approvalId, + command: commandText, + cwd: workdir, + host: "node", + security: hostSecurity, + ask: hostAsk, + agentId, + resolvedPath: undefined, + sessionKey: defaults?.sessionKey, + timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }, + { expectFinal: false }, + )) as { status?: string; expiresAtMs?: number } | null; + if (registrationResult?.expiresAtMs) { + expiresAtMs = registrationResult.expiresAtMs; + } + } catch (err) { + // Registration failed - throw to caller + throw new Error(`Exec approval registration failed: ${String(err)}`); + } + + // Fire-and-forget: wait for decision via waitDecision endpoint, then execute. void (async () => { let decision: string | null = null; try { const decisionResult = (await callGatewayTool( - "exec.approval.request", + "exec.approval.waitDecision", { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, - { - id: approvalId, - command: commandText, - cwd: workdir, - host: "node", - security: hostSecurity, - ask: hostAsk, - agentId, - resolvedPath: undefined, - sessionKey: defaults?.sessionKey, - timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, - }, + { id: approvalId }, )) as { decision?: string } | null; decision = decisionResult && typeof decisionResult === "object" @@ -1185,7 +1203,6 @@ export function createExecTool( if (requiresAsk) { const approvalId = crypto.randomUUID(); const approvalSlug = createApprovalSlug(approvalId); - const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; const contextKey = `exec:${approvalId}`; const resolvedPath = allowlistEval.segments[0]?.resolution?.resolvedPath; const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000)); @@ -1194,24 +1211,43 @@ export function createExecTool( typeof params.timeout === "number" ? params.timeout : defaultTimeoutSec; const warningText = warnings.length ? `${warnings.join("\n")}\n\n` : ""; + // Register the approval with expectFinal:false to get immediate confirmation. + // This ensures the approval ID is valid before we return. + let expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + try { + const registrationResult = (await callGatewayTool( + "exec.approval.request", + { timeoutMs: 10_000 }, + { + id: approvalId, + command: commandText, + cwd: workdir, + host: "gateway", + security: hostSecurity, + ask: hostAsk, + agentId, + resolvedPath, + sessionKey: defaults?.sessionKey, + timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }, + { expectFinal: false }, + )) as { status?: string; expiresAtMs?: number } | null; + if (registrationResult?.expiresAtMs) { + expiresAtMs = registrationResult.expiresAtMs; + } + } catch (err) { + // Registration failed - throw to caller + throw new Error(`Exec approval registration failed: ${String(err)}`); + } + + // Fire-and-forget: wait for decision via waitDecision endpoint, then execute. void (async () => { let decision: string | null = null; try { const decisionResult = (await callGatewayTool( - "exec.approval.request", + "exec.approval.waitDecision", { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, - { - id: approvalId, - command: commandText, - cwd: workdir, - host: "gateway", - security: hostSecurity, - ask: hostAsk, - agentId, - resolvedPath, - sessionKey: defaults?.sessionKey, - timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, - }, + { id: approvalId }, )) as { decision?: string } | null; decision = decisionResult && typeof decisionResult === "object" From 0ccb5584ee1338ec87cd20f0344b8ccc2125ef5d Mon Sep 17 00:00:00 2001 From: rshirali Date: Wed, 28 Jan 2026 14:54:13 +0100 Subject: [PATCH 4/5] test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. --- src/gateway/server-methods/exec-approval.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/gateway/server-methods/exec-approval.test.ts b/src/gateway/server-methods/exec-approval.test.ts index 71a63e5a3..24104ed0d 100644 --- a/src/gateway/server-methods/exec-approval.test.ts +++ b/src/gateway/server-methods/exec-approval.test.ts @@ -82,6 +82,13 @@ describe("exec approval handlers", () => { const id = (requested?.payload as { id?: string })?.id ?? ""; expect(id).not.toBe(""); + // First response should be "accepted" (registration confirmation) + expect(respond).toHaveBeenCalledWith( + true, + expect.objectContaining({ status: "accepted", id }), + undefined, + ); + const resolveRespond = vi.fn(); await handlers["exec.approval.resolve"]({ params: { id, decision: "allow-once" }, @@ -97,6 +104,7 @@ describe("exec approval handlers", () => { await requestPromise; expect(resolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined); + // Second response should contain the decision expect(respond).toHaveBeenCalledWith( true, expect.objectContaining({ id, decision: "allow-once" }), From a6a5e3cd51cc374559b88064659a458bcc8aaa9c Mon Sep 17 00:00:00 2001 From: rshirali Date: Wed, 28 Jan 2026 14:54:21 +0100 Subject: [PATCH 5/5] test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. --- src/agents/bash-tools.exec.approval-id.test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index a5587355b..43262476e 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -51,6 +51,11 @@ describe("exec approvals", () => { vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { if (method === "exec.approval.request") { + // Return registration confirmation (status: "accepted") + return { status: "accepted", id: (params as { id?: string })?.id }; + } + if (method === "exec.approval.waitDecision") { + // Return the decision when waitDecision is called return { decision: "allow-once" }; } if (method === "node.invoke") { @@ -159,10 +164,14 @@ describe("exec approvals", () => { resolveApproval = resolve; }); - vi.mocked(callGatewayTool).mockImplementation(async (method) => { + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { calls.push(method); if (method === "exec.approval.request") { resolveApproval?.(); + // Return registration confirmation + return { status: "accepted", id: (params as { id?: string })?.id }; + } + if (method === "exec.approval.waitDecision") { return { decision: "deny" }; } return { ok: true };