From 1af0edf7ff222f6784a1ffa45cdacc89fa383feb Mon Sep 17 00:00:00 2001 From: Ramin Shirali Hossein Zade Date: Fri, 13 Feb 2026 19:57:02 +0100 Subject: [PATCH] fix: ensure exec approval is registered before returning (#2402) (#3357) * 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. * 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. * 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 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * 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. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali Co-authored-by: rshirali Co-authored-by: Peter Steinberger --- CHANGELOG.md | 12 ++ .../OpenClawProtocol/GatewayModels.swift | 6 +- .../OpenClawProtocol/GatewayModels.swift | 6 +- .../bash-tools.exec.approval-id.e2e.test.ts | 15 ++- src/agents/bash-tools.exec.ts | 104 +++++++++++++----- .../pi-tools.workspace-paths.e2e.test.ts | 10 +- src/discord/monitor/threading.test.ts | 3 - src/gateway/exec-approval-manager.ts | 84 ++++++++++++-- src/gateway/protocol/schema/exec-approvals.ts | 1 + src/gateway/server-methods-list.ts | 1 + src/gateway/server-methods.ts | 6 +- .../server-methods/exec-approval.test.ts | 9 ++ src/gateway/server-methods/exec-approval.ts | 66 ++++++++++- 13 files changed, 272 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddf85d1cd7..156b137b36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -295,6 +295,18 @@ Docs: https://docs.openclaw.ai ### Fixes +- Control UI: add hardened fallback for asset resolution in global npm installs. (#4855) Thanks @anapivirtua. +- Update: remove dead restore control-ui step that failed on gitignored dist/ output. +- Update: avoid wiping prebuilt Control UI assets during dev auto-builds (`tsdown --no-clean`), run update doctor via `openclaw.mjs`, and auto-restore missing UI assets after doctor. (#10146) Thanks @gumadeiras. +- Models: add forward-compat fallback for `openai-codex/gpt-5.3-codex` when model registry hasn't discovered it yet. (#9989) Thanks @w1kke. +- Auto-reply/Docs: normalize `extra-high` (and spaced variants) to `xhigh` for Codex thinking levels, and align Codex 5.3 FAQ examples. (#9976) Thanks @slonce70. +- Compaction: remove orphaned `tool_result` messages during history pruning to prevent session corruption from aborted tool calls. (#9868, fixes #9769, #9724, #9672) +- Telegram: pass `parentPeer` for forum topic binding inheritance so group-level bindings apply to all topics within the group. (#9789, fixes #9545, #9351) +- CLI: pass `--disable-warning=ExperimentalWarning` as a Node CLI option when respawning (avoid disallowed `NODE_OPTIONS` usage; fixes npm pack). (#9691) Thanks @18-RAJAT. +- CLI: resolve bundled Chrome extension assets by walking up to the nearest assets directory; add resolver and clipboard tests. (#8914) Thanks @kelvinCB. +- Tests: stabilize Windows ACL coverage with deterministic os.userInfo mocking. (#9335) Thanks @M00N7682. +- Exec approvals: coerce bare string allowlist entries to objects to prevent allowlist corruption. (#9903, fixes #9790) Thanks @mcaxtr. +- Exec approvals: ensure two-phase approval registration/decision flow works reliably by validating `twoPhase` requests and exposing `waitDecision` as an approvals-scoped gateway method. (#3357, fixes #2402) Thanks @ramin-shirali. - Heartbeat: allow explicit accountId routing for multi-account channels. (#8702) Thanks @lsh411. - TUI/Gateway: handle non-streaming finals, refresh history for non-local chat runs, and avoid event gap warnings for targeted tool streams. (#8432) Thanks @gumadeiras. - Shell completion: auto-detect and migrate slow dynamic patterns to cached files for faster terminal startup; add completion health checks to doctor/update/onboard. diff --git a/apps/macos/Sources/OpenClawProtocol/GatewayModels.swift b/apps/macos/Sources/OpenClawProtocol/GatewayModels.swift index fca8eac3a9..241dc58fa0 100644 --- a/apps/macos/Sources/OpenClawProtocol/GatewayModels.swift +++ b/apps/macos/Sources/OpenClawProtocol/GatewayModels.swift @@ -2380,6 +2380,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { public let resolvedpath: AnyCodable? public let sessionkey: AnyCodable? public let timeoutms: Int? + public let twophase: Bool? public init( id: String?, @@ -2391,7 +2392,8 @@ public struct ExecApprovalRequestParams: Codable, Sendable { agentid: AnyCodable?, resolvedpath: AnyCodable?, sessionkey: AnyCodable?, - timeoutms: Int? + timeoutms: Int?, + twophase: Bool? ) { self.id = id self.command = command @@ -2403,6 +2405,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { self.resolvedpath = resolvedpath self.sessionkey = sessionkey self.timeoutms = timeoutms + self.twophase = twophase } private enum CodingKeys: String, CodingKey { case id @@ -2415,6 +2418,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { case resolvedpath = "resolvedPath" case sessionkey = "sessionKey" case timeoutms = "timeoutMs" + case twophase = "twoPhase" } } diff --git a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift index fca8eac3a9..241dc58fa0 100644 --- a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift +++ b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift @@ -2380,6 +2380,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { public let resolvedpath: AnyCodable? public let sessionkey: AnyCodable? public let timeoutms: Int? + public let twophase: Bool? public init( id: String?, @@ -2391,7 +2392,8 @@ public struct ExecApprovalRequestParams: Codable, Sendable { agentid: AnyCodable?, resolvedpath: AnyCodable?, sessionkey: AnyCodable?, - timeoutms: Int? + timeoutms: Int?, + twophase: Bool? ) { self.id = id self.command = command @@ -2403,6 +2405,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { self.resolvedpath = resolvedpath self.sessionkey = sessionkey self.timeoutms = timeoutms + self.twophase = twophase } private enum CodingKeys: String, CodingKey { case id @@ -2415,6 +2418,7 @@ public struct ExecApprovalRequestParams: Codable, Sendable { case resolvedpath = "resolvedPath" case sessionkey = "sessionKey" case timeoutms = "timeoutMs" + case twophase = "twoPhase" } } diff --git a/src/agents/bash-tools.exec.approval-id.e2e.test.ts b/src/agents/bash-tools.exec.approval-id.e2e.test.ts index 5abbeae956..4da098c6a9 100644 --- a/src/agents/bash-tools.exec.approval-id.e2e.test.ts +++ b/src/agents/bash-tools.exec.approval-id.e2e.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") { @@ -108,9 +113,7 @@ describe("exec approvals", () => { if (method === "node.invoke") { return { payload: { success: true, stdout: "ok" } }; } - if (method === "exec.approval.request") { - return { decision: "allow-once" }; - } + // exec.approval.request should NOT be called when allowlist is satisfied return { ok: true }; }); @@ -159,10 +162,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 f8755a5c96..8464f1411e 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -1135,29 +1135,51 @@ 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<{ + status?: string; + expiresAtMs?: number; + }>( + "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, + twoPhase: true, + }, + { expectFinal: false }, + ); + if (registrationResult?.expiresAtMs) { + expiresAtMs = registrationResult.expiresAtMs; + } + } catch (err) { + // Registration failed - throw to caller + throw new Error(`Exec approval registration failed: ${String(err)}`, { cause: err }); + } + + // Fire-and-forget: wait for decision via waitDecision endpoint, then execute. void (async () => { let decision: string | null = null; try { - const decisionResult = await callGatewayTool<{ decision: string }>( - "exec.approval.request", + const decisionResult = await callGatewayTool<{ decision?: string }>( + "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 }, ); const decisionValue = decisionResult && typeof decisionResult === "object" @@ -1315,7 +1337,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)); @@ -1324,24 +1345,47 @@ 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<{ + status?: string; + expiresAtMs?: number; + }>( + "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, + twoPhase: true, + }, + { expectFinal: false }, + ); + if (registrationResult?.expiresAtMs) { + expiresAtMs = registrationResult.expiresAtMs; + } + } catch (err) { + // Registration failed - throw to caller + throw new Error(`Exec approval registration failed: ${String(err)}`, { cause: err }); + } + + // Fire-and-forget: wait for decision via waitDecision endpoint, then execute. void (async () => { let decision: string | null = null; try { - const decisionResult = await callGatewayTool<{ decision: string }>( - "exec.approval.request", + const decisionResult = await callGatewayTool<{ decision?: string }>( + "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 }, ); const decisionValue = decisionResult && typeof decisionResult === "object" diff --git a/src/agents/pi-tools.workspace-paths.e2e.test.ts b/src/agents/pi-tools.workspace-paths.e2e.test.ts index ea53e691ac..eb58b58a11 100644 --- a/src/agents/pi-tools.workspace-paths.e2e.test.ts +++ b/src/agents/pi-tools.workspace-paths.e2e.test.ts @@ -101,7 +101,10 @@ describe("workspace path resolution", () => { it("defaults exec cwd to workspaceDir when workdir is omitted", async () => { await withTempDir("openclaw-ws-", async (workspaceDir) => { - const tools = createOpenClawCodingTools({ workspaceDir, exec: { host: "gateway" } }); + const tools = createOpenClawCodingTools({ + workspaceDir, + exec: { host: "gateway", ask: "off", security: "full" }, + }); const execTool = tools.find((tool) => tool.name === "exec"); expect(execTool).toBeDefined(); @@ -124,7 +127,10 @@ describe("workspace path resolution", () => { it("lets exec workdir override the workspace default", async () => { await withTempDir("openclaw-ws-", async (workspaceDir) => { await withTempDir("openclaw-override-", async (overrideDir) => { - const tools = createOpenClawCodingTools({ workspaceDir, exec: { host: "gateway" } }); + const tools = createOpenClawCodingTools({ + workspaceDir, + exec: { host: "gateway", ask: "off", security: "full" }, + }); const execTool = tools.find((tool) => tool.name === "exec"); expect(execTool).toBeDefined(); diff --git a/src/discord/monitor/threading.test.ts b/src/discord/monitor/threading.test.ts index d00c7f416c..0d8a4bb0da 100644 --- a/src/discord/monitor/threading.test.ts +++ b/src/discord/monitor/threading.test.ts @@ -115,12 +115,9 @@ describe("resolveDiscordReplyDeliveryPlan", () => { describe("maybeCreateDiscordAutoThread", () => { it("returns existing thread ID when creation fails due to race condition", async () => { - // First call succeeds (simulating another agent creating the thread) - let callCount = 0; const client = { rest: { post: async () => { - callCount++; throw new Error("A thread has already been created on this message"); }, get: async () => { diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 3c33aac4d5..f4e7dd9994 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -1,6 +1,9 @@ import { randomUUID } from "node:crypto"; import type { ExecApprovalDecision } from "../infra/exec-approvals.js"; +// Grace period to keep resolved entries for late awaitDecision calls +const RESOLVED_ENTRY_GRACE_MS = 15_000; + export type ExecApprovalRequestPayload = { command: string; cwd?: string | null; @@ -27,6 +30,7 @@ type PendingEntry = { resolve: (decision: ExecApprovalDecision | null) => void; reject: (err: Error) => void; timer: ReturnType; + promise: Promise; }; export class ExecApprovalManager { @@ -48,17 +52,61 @@ 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 { + const existing = this.pending.get(record.id); + if (existing) { + // Idempotent: return existing promise if still pending + if (existing.record.resolvedAtMs === undefined) { + return existing.promise; + } + // Already resolved - don't allow re-registration + throw new Error(`approval id '${record.id}' already resolved`); + } + let resolvePromise: (decision: ExecApprovalDecision | null) => void; + let rejectPromise: (err: Error) => void; + const promise = new Promise((resolve, reject) => { + resolvePromise = resolve; + rejectPromise = reject; + }); + // Create entry first so we can capture it in the closure (not re-fetch from map) + const entry: PendingEntry = { + record, + resolve: resolvePromise!, + reject: rejectPromise!, + timer: null as unknown as ReturnType, + promise, + }; + entry.timer = setTimeout(() => { + // Update snapshot fields before resolving (mirror resolve()'s bookkeeping) + record.resolvedAtMs = Date.now(); + record.decision = undefined; + record.resolvedBy = null; + resolvePromise(null); + // Keep entry briefly for in-flight awaitDecision calls + setTimeout(() => { + // Compare against captured entry instance, not re-fetched from map + if (this.pending.get(record.id) === entry) { + this.pending.delete(record.id); + } + }, RESOLVED_ENTRY_GRACE_MS); + }, timeoutMs); + this.pending.set(record.id, entry); + 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 { @@ -66,12 +114,23 @@ export class ExecApprovalManager { if (!pending) { return false; } + // Prevent double-resolve (e.g., if called after timeout already resolved) + if (pending.record.resolvedAtMs !== undefined) { + return false; + } clearTimeout(pending.timer); 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); + } + }, RESOLVED_ENTRY_GRACE_MS); return true; } @@ -79,4 +138,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/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index a88cdffcdc..05c2e03760 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -99,6 +99,7 @@ export const ExecApprovalRequestParamsSchema = Type.Object( resolvedPath: Type.Optional(Type.Union([Type.String(), Type.Null()])), sessionKey: Type.Optional(Type.Union([Type.String(), Type.Null()])), timeoutMs: Type.Optional(Type.Integer({ minimum: 1 })), + twoPhase: Type.Optional(Type.Boolean()), }, { additionalProperties: false }, ); diff --git a/src/gateway/server-methods-list.ts b/src/gateway/server-methods-list.ts index b4989aad6a..bb691f08ea 100644 --- a/src/gateway/server-methods-list.ts +++ b/src/gateway/server-methods-list.ts @@ -24,6 +24,7 @@ const BASE_METHODS = [ "exec.approvals.node.get", "exec.approvals.node.set", "exec.approval.request", + "exec.approval.waitDecision", "exec.approval.resolve", "wizard.start", "wizard.next", diff --git a/src/gateway/server-methods.ts b/src/gateway/server-methods.ts index fe79f5d0a8..e6086301c7 100644 --- a/src/gateway/server-methods.ts +++ b/src/gateway/server-methods.ts @@ -32,7 +32,11 @@ const WRITE_SCOPE = "operator.write"; const APPROVALS_SCOPE = "operator.approvals"; const PAIRING_SCOPE = "operator.pairing"; -const APPROVAL_METHODS = new Set(["exec.approval.request", "exec.approval.resolve"]); +const APPROVAL_METHODS = new Set([ + "exec.approval.request", + "exec.approval.waitDecision", + "exec.approval.resolve", +]); const NODE_ROLE_METHODS = new Set(["node.invoke.result", "node.event", "skills.bins"]); const PAIRING_METHODS = new Set([ "node.pair.request", diff --git a/src/gateway/server-methods/exec-approval.test.ts b/src/gateway/server-methods/exec-approval.test.ts index 0a80b9e9d2..ac0373343b 100644 --- a/src/gateway/server-methods/exec-approval.test.ts +++ b/src/gateway/server-methods/exec-approval.test.ts @@ -67,6 +67,7 @@ describe("exec approval handlers", () => { cwd: "/tmp", host: "node", timeoutMs: 2000, + twoPhase: true, }, respond, context: context as unknown as Parameters< @@ -82,6 +83,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 +105,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 beb3f03725..f88e0d6a0b 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -40,7 +40,9 @@ export function createExecApprovalHandlers( resolvedPath?: string; sessionKey?: string; timeoutMs?: number; + twoPhase?: boolean; }; + const twoPhase = p.twoPhase === true; const timeoutMs = typeof p.timeoutMs === "number" ? p.timeoutMs : 120_000; const explicitId = typeof p.id === "string" && p.id.trim().length > 0 ? p.id.trim() : null; if (explicitId && manager.getSnapshot(explicitId)) { @@ -62,7 +64,21 @@ 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. + let decisionPromise: Promise< + import("../../infra/exec-approvals.js").ExecApprovalDecision | null + >; + try { + decisionPromise = manager.register(record, timeoutMs); + } catch (err) { + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, `registration failed: ${String(err)}`), + ); + return; + } context.broadcast( "exec.approval.requested", { @@ -83,7 +99,24 @@ export function createExecApprovalHandlers( .catch((err) => { context.logGateway?.error?.(`exec approvals: forward request failed: ${String(err)}`); }); + + // Only send immediate "accepted" response when twoPhase is requested. + // This preserves single-response semantics for existing callers. + if (twoPhase) { + 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 +128,37 @@ export function createExecApprovalHandlers( undefined, ); }, + "exec.approval.waitDecision": async ({ params, respond }) => { + const p = params as { id?: string }; + 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, "approval expired or not found"), + ); + return; + } + // Capture snapshot before await (entry may be deleted after grace period) + const snapshot = manager.getSnapshot(id); + const decision = await decisionPromise; + // Return decision (can be null on timeout) - let clients handle via askFallback + respond( + true, + { + id, + decision, + createdAtMs: snapshot?.createdAtMs, + expiresAtMs: snapshot?.expiresAtMs, + }, + undefined, + ); + }, "exec.approval.resolve": async ({ params, respond, client, context }) => { if (!validateExecApprovalResolveParams(params)) { respond(