diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 5abbeae95..0a67d240b 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 }; diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index ad77d10e6..5269ce4be 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" 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; + } } 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" }), 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(