Merge ff66581882 into da71eaebd2

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

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 },
-                  id: approvalId,
-                  command: commandText,
-                  cwd: workdir,
-                  host: "node",
-                  security: hostSecurity,
-                  ask: hostAsk,
-                  agentId,
-                  resolvedPath: undefined,
-                  sessionKey: defaults?.sessionKey,
-                  timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS,
-                },
               )) 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 },
-                  id: approvalId,
-                  command: commandText,
-                  cwd: workdir,
-                  host: "gateway",
-                  security: hostSecurity,
-                  ask: hostAsk,
-                  agentId,
-                  resolvedPath,
-                  sessionKey: defaults?.sessionKey,
-                  timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS,
-                },
               )) as { decision?: string } | null;
               decision =
                 decisionResult && typeof decisionResult === "object"

src/gateway/exec-approval-manager.ts

@@ -28,6 +28,7 @@ type PendingEntry = {
   resolve: (decision: ExecApprovalDecision | null) => void;
   reject: (err: Error) => void;
   timer: ReturnType<typeof setTimeout>;
+  promise: Promise<ExecApprovalDecision | null>;
 };
 
 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<ExecApprovalDecision | null> {
+    let resolvePromise: (decision: ExecApprovalDecision | null) => void;
+    let rejectPromise: (err: Error) => void;
+    const promise = new Promise<ExecApprovalDecision | null>((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<ExecApprovalDecision | null> {
-    return await new Promise<ExecApprovalDecision | null>((resolve, reject) => {
+    return this.register(record, timeoutMs);
-      const timer = setTimeout(() => {
-        this.pending.delete(record.id);
-        resolve(null);
-      }, timeoutMs);
-      this.pending.set(record.id, { record, resolve, reject, timer });
-    });
   }
 
   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<ExecApprovalDecision | null> | null {
+    const entry = this.pending.get(recordId);
+    return entry?.promise ?? null;
+  }
 }

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

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(