diff --git a/src/agents/bash-tools.exec-approval-request.test.ts b/src/agents/bash-tools.exec-approval-request.test.ts index a0722002c..1cd221e30 100644 --- a/src/agents/bash-tools.exec-approval-request.test.ts +++ b/src/agents/bash-tools.exec-approval-request.test.ts @@ -22,7 +22,13 @@ describe("requestExecApprovalDecision", () => { }); it("returns string decisions", async () => { - vi.mocked(callGatewayTool).mockResolvedValue({ decision: "allow-once" }); + vi.mocked(callGatewayTool) + .mockResolvedValueOnce({ + status: "accepted", + id: "approval-id", + expiresAtMs: DEFAULT_APPROVAL_TIMEOUT_MS, + }) + .mockResolvedValueOnce({ decision: "allow-once" }); const result = await requestExecApprovalDecision({ id: "approval-id", @@ -52,12 +58,22 @@ describe("requestExecApprovalDecision", () => { resolvedPath: "/usr/bin/echo", sessionKey: "session", timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + twoPhase: true, }, + { expectFinal: false }, + ); + expect(callGatewayTool).toHaveBeenNthCalledWith( + 2, + "exec.approval.waitDecision", + { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, + { id: "approval-id" }, ); }); it("returns null for missing or non-string decisions", async () => { - vi.mocked(callGatewayTool).mockResolvedValueOnce({}); + vi.mocked(callGatewayTool) + .mockResolvedValueOnce({ status: "accepted", id: "approval-id", expiresAtMs: 1234 }) + .mockResolvedValueOnce({}); await expect( requestExecApprovalDecision({ id: "approval-id", @@ -70,7 +86,9 @@ describe("requestExecApprovalDecision", () => { }), ).resolves.toBeNull(); - vi.mocked(callGatewayTool).mockResolvedValueOnce({ decision: 123 }); + vi.mocked(callGatewayTool) + .mockResolvedValueOnce({ status: "accepted", id: "approval-id-2", expiresAtMs: 1234 }) + .mockResolvedValueOnce({ decision: 123 }); await expect( requestExecApprovalDecision({ id: "approval-id-2", @@ -83,4 +101,20 @@ describe("requestExecApprovalDecision", () => { }), ).resolves.toBeNull(); }); + + it("returns final decision directly when gateway already replies with decision", async () => { + vi.mocked(callGatewayTool).mockResolvedValue({ decision: "deny", id: "approval-id" }); + + const result = await requestExecApprovalDecision({ + id: "approval-id", + command: "echo hi", + cwd: "/tmp", + host: "gateway", + security: "allowlist", + ask: "on-miss", + }); + + expect(result).toBe("deny"); + expect(vi.mocked(callGatewayTool).mock.calls).toHaveLength(1); + }); }); diff --git a/src/agents/bash-tools.exec-approval-request.ts b/src/agents/bash-tools.exec-approval-request.ts index 7f0b59736..83323845c 100644 --- a/src/agents/bash-tools.exec-approval-request.ts +++ b/src/agents/bash-tools.exec-approval-request.ts @@ -18,10 +18,45 @@ export type RequestExecApprovalDecisionParams = { sessionKey?: string; }; -export async function requestExecApprovalDecision( +type ParsedDecision = { present: boolean; value: string | null }; + +function parseDecision(value: unknown): ParsedDecision { + if (!value || typeof value !== "object") { + return { present: false, value: null }; + } + // Distinguish "field missing" from "field present but null/invalid". + // Registration responses intentionally omit `decision`; decision waits can include it. + if (!Object.hasOwn(value, "decision")) { + return { present: false, value: null }; + } + const decision = (value as { decision?: unknown }).decision; + return { present: true, value: typeof decision === "string" ? decision : null }; +} + +function parseString(value: unknown): string | undefined { + return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; +} + +function parseExpiresAtMs(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) ? value : undefined; +} + +export type ExecApprovalRegistration = { + id: string; + expiresAtMs: number; + finalDecision?: string | null; +}; + +export async function registerExecApprovalRequest( params: RequestExecApprovalDecisionParams, -): Promise { - const decisionResult = await callGatewayTool<{ decision: string }>( +): Promise { + // Two-phase registration is critical: the ID must be registered server-side + // before exec returns `approval-pending`, otherwise `/approve` can race and orphan. + const registrationResult = await callGatewayTool<{ + id?: string; + expiresAtMs?: number; + decision?: string; + }>( "exec.approval.request", { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, { @@ -36,13 +71,46 @@ export async function requestExecApprovalDecision( resolvedPath: params.resolvedPath, sessionKey: params.sessionKey, timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, + twoPhase: true, }, + { expectFinal: false }, ); - const decisionValue = - decisionResult && typeof decisionResult === "object" - ? (decisionResult as { decision?: unknown }).decision - : undefined; - return typeof decisionValue === "string" ? decisionValue : null; + const decision = parseDecision(registrationResult); + const id = parseString(registrationResult?.id) ?? params.id; + const expiresAtMs = + parseExpiresAtMs(registrationResult?.expiresAtMs) ?? Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + if (decision.present) { + return { id, expiresAtMs, finalDecision: decision.value }; + } + return { id, expiresAtMs }; +} + +export async function waitForExecApprovalDecision(id: string): Promise { + try { + const decisionResult = await callGatewayTool<{ decision: string }>( + "exec.approval.waitDecision", + { timeoutMs: DEFAULT_APPROVAL_REQUEST_TIMEOUT_MS }, + { id }, + ); + return parseDecision(decisionResult).value; + } catch (err) { + // Timeout/cleanup path: treat missing/expired as no decision so askFallback applies. + const message = String(err).toLowerCase(); + if (message.includes("approval expired or not found")) { + return null; + } + throw err; + } +} + +export async function requestExecApprovalDecision( + params: RequestExecApprovalDecisionParams, +): Promise { + const registration = await registerExecApprovalRequest(params); + if (Object.hasOwn(registration, "finalDecision")) { + return registration.finalDecision ?? null; + } + return await waitForExecApprovalDecision(registration.id); } export async function requestExecApprovalDecisionForHost(params: { @@ -70,3 +138,29 @@ export async function requestExecApprovalDecisionForHost(params: { sessionKey: params.sessionKey, }); } + +export async function registerExecApprovalRequestForHost(params: { + approvalId: string; + command: string; + workdir: string; + host: "gateway" | "node"; + nodeId?: string; + security: ExecSecurity; + ask: ExecAsk; + agentId?: string; + resolvedPath?: string; + sessionKey?: string; +}): Promise { + return await registerExecApprovalRequest({ + id: params.approvalId, + command: params.command, + cwd: params.workdir, + nodeId: params.nodeId, + host: params.host, + security: params.security, + ask: params.ask, + agentId: params.agentId, + resolvedPath: params.resolvedPath, + sessionKey: params.sessionKey, + }); +} diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index be81e703e..607119109 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -17,7 +17,10 @@ import { detectCommandObfuscation } from "../infra/exec-obfuscation-detect.js"; import type { SafeBinProfile } from "../infra/exec-safe-bin-policy.js"; import { logInfo } from "../logger.js"; import { markBackgrounded, tail } from "./bash-process-registry.js"; -import { requestExecApprovalDecisionForHost } from "./bash-tools.exec-approval-request.js"; +import { + registerExecApprovalRequestForHost, + waitForExecApprovalDecision, +} from "./bash-tools.exec-approval-request.js"; import { DEFAULT_APPROVAL_TIMEOUT_MS, DEFAULT_NOTIFY_TAIL_CHARS, @@ -135,28 +138,42 @@ export async function processGatewayAllowlist( 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(params.approvalRunningNoticeMs / 1000)); const effectiveTimeout = typeof params.timeoutSec === "number" ? params.timeoutSec : params.defaultTimeoutSec; const warningText = params.warnings.length ? `${params.warnings.join("\n")}\n\n` : ""; + let expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + let preResolvedDecision: string | null | undefined; + + try { + // Register first so the returned approval ID is actionable immediately. + const registration = await registerExecApprovalRequestForHost({ + approvalId, + command: params.command, + workdir: params.workdir, + host: "gateway", + security: hostSecurity, + ask: hostAsk, + agentId: params.agentId, + resolvedPath, + sessionKey: params.sessionKey, + }); + expiresAtMs = registration.expiresAtMs; + preResolvedDecision = registration.finalDecision; + } catch (err) { + throw new Error(`Exec approval registration failed: ${String(err)}`, { cause: err }); + } void (async () => { - let decision: string | null = null; + let decision: string | null = preResolvedDecision ?? null; try { - decision = await requestExecApprovalDecisionForHost({ - approvalId, - command: params.command, - workdir: params.workdir, - host: "gateway", - security: hostSecurity, - ask: hostAsk, - agentId: params.agentId, - resolvedPath, - sessionKey: params.sessionKey, - }); + // Some gateways may return a final decision inline during registration. + // Only call waitDecision when registration did not already carry one. + if (preResolvedDecision === undefined) { + decision = await waitForExecApprovalDecision(approvalId); + } } catch { emitExecSystemEvent( `Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`, diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index fc6893b93..5a45c8692 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -14,7 +14,10 @@ import { import { detectCommandObfuscation } from "../infra/exec-obfuscation-detect.js"; import { buildNodeShellCommand } from "../infra/node-shell.js"; import { logInfo } from "../logger.js"; -import { requestExecApprovalDecisionForHost } from "./bash-tools.exec-approval-request.js"; +import { + registerExecApprovalRequestForHost, + waitForExecApprovalDecision, +} from "./bash-tools.exec-approval-request.js"; import { DEFAULT_APPROVAL_TIMEOUT_MS, createApprovalSlug, @@ -180,25 +183,39 @@ export async function executeNodeHostCommand( 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(params.approvalRunningNoticeMs / 1000)); const warningText = params.warnings.length ? `${params.warnings.join("\n")}\n\n` : ""; + let expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; + let preResolvedDecision: string | null | undefined; + + try { + // Register first so the returned approval ID is actionable immediately. + const registration = await registerExecApprovalRequestForHost({ + approvalId, + command: params.command, + workdir: params.workdir, + host: "node", + nodeId, + security: hostSecurity, + ask: hostAsk, + agentId: params.agentId, + sessionKey: params.sessionKey, + }); + expiresAtMs = registration.expiresAtMs; + preResolvedDecision = registration.finalDecision; + } catch (err) { + throw new Error(`Exec approval registration failed: ${String(err)}`, { cause: err }); + } void (async () => { - let decision: string | null = null; + let decision: string | null = preResolvedDecision ?? null; try { - decision = await requestExecApprovalDecisionForHost({ - approvalId, - command: params.command, - workdir: params.workdir, - host: "node", - nodeId, - security: hostSecurity, - ask: hostAsk, - agentId: params.agentId, - sessionKey: params.sessionKey, - }); + // Some gateways may return a final decision inline during registration. + // Only call waitDecision when registration did not already carry one. + if (preResolvedDecision === undefined) { + decision = await waitForExecApprovalDecision(approvalId); + } } catch { emitExecSystemEvent( `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${params.command}`, diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 4fb5b4bf4..37a1215e5 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -65,7 +65,9 @@ describe("exec approvals", () => { vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { if (method === "exec.approval.request") { - // Approval request now carries the decision directly. + return { status: "accepted", id: (params as { id?: string })?.id }; + } + if (method === "exec.approval.waitDecision") { return { decision: "allow-once" }; } if (method === "node.invoke") { @@ -191,6 +193,7 @@ describe("exec approvals", () => { expect(result.details.status).toBe("approval-pending"); await approvalSeen; expect(calls).toContain("exec.approval.request"); + expect(calls).toContain("exec.approval.waitDecision"); }); it("denies node obfuscated command when approval request times out", async () => { @@ -204,6 +207,9 @@ describe("exec approvals", () => { vi.mocked(callGatewayTool).mockImplementation(async (method) => { calls.push(method); if (method === "exec.approval.request") { + return { status: "accepted", id: "approval-id" }; + } + if (method === "exec.approval.waitDecision") { return {}; } if (method === "node.invoke") { @@ -237,6 +243,9 @@ describe("exec approvals", () => { vi.mocked(callGatewayTool).mockImplementation(async (method) => { if (method === "exec.approval.request") { + return { status: "accepted", id: "approval-id" }; + } + if (method === "exec.approval.waitDecision") { return {}; } return { ok: true };