fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali

Mock both exec.approval.request (registration) and exec.approval.waitDecision
(decision) calls to match the new internal implementation.

CHANGELOG.md

@@ -46,6 +46,7 @@ Docs: https://docs.openclaw.ai
 - 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.
 - Security: stop exposing Gateway auth tokens via URL query parameters in Control UI entrypoints, and reject hook tokens in query parameters. (#9436) Thanks @coygeek.

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") {
@@ -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 };

src/agents/bash-tools.exec.ts

@@ -1120,29 +1120,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"
@@ -1300,7 +1322,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));
@@ -1309,24 +1330,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"

src/agents/pi-tools.workspace-paths.test.ts

@@ -105,7 +105,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();
 
@@ -128,7 +131,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();
 

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<typeof setTimeout>;
+  promise: Promise<ExecApprovalDecision | null>;
 };
 
 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<ExecApprovalDecision | null> {
+    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<ExecApprovalDecision | null>((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<typeof setTimeout>,
+      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<ExecApprovalDecision | null> {
-    return await new Promise<ExecApprovalDecision | null>((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<ExecApprovalDecision | null> | null {
+    const entry = this.pending.get(recordId);
+    return entry?.promise ?? null;
+  }
 }

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 },
 );

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",

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",

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" }),

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(