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 <rshirali@rshirali-haga.local>
Co-authored-by: rshirali <rshirali@rshirali-haga-1.home>
Co-authored-by: Peter Steinberger <steipete@gmail.com>

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;
+  }
 }