fix: address self-review on desktop MCP permission lifecycle (#7013)

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>

packages/desktop/packages/shared/src/agent/__tests__/pending-permissions-lifecycle.test.ts

@@ -0,0 +1,163 @@
+import { afterEach, describe, expect, it, jest } from 'bun:test';
+
+import type { RequestPermissionResponse } from '@agentclientprotocol/sdk';
+import type { PermissionCallback } from '../backend/types.ts';
+import { QwenAgent } from '../qwen-agent.ts';
+import {
+  createMockBackendConfig,
+  createMockSession,
+  createMockWorkspace,
+} from './test-utils.ts';
+
+type QwenPermissionInternals = {
+  handlePermissionRequest: (
+    params: unknown,
+  ) => Promise<RequestPermissionResponse>;
+  pendingPermissions: Map<string, unknown>;
+};
+
+function createAgent(permissionMode: 'ask' | 'allow-all' = 'ask'): QwenAgent {
+  const agent = new QwenAgent(
+    createMockBackendConfig({
+      workspace: createMockWorkspace({
+        id: 'workspace-qwen',
+        name: 'Qwen Workspace',
+        slug: 'qwen-workspace',
+        rootPath: '/tmp/qwen-permission-tests',
+      }),
+      session: createMockSession({
+        id: 'session-qwen',
+        name: 'Qwen Session',
+        workspaceRootPath: '/tmp/qwen-permission-tests',
+        permissionMode,
+      }),
+    }),
+  );
+  agent.setPermissionMode(permissionMode);
+  return agent;
+}
+
+function permissionRequest(): unknown {
+  return {
+    toolCall: {
+      title: 'Run a command',
+      kind: 'execute',
+      rawInput: { command: 'npm test' },
+      _meta: { toolName: 'shell' },
+    },
+    options: [
+      {
+        optionId: 'proceed_once',
+        name: 'Allow once',
+        kind: 'allow_once',
+      },
+      { optionId: 'reject_once', name: 'Reject', kind: 'reject_once' },
+    ],
+  };
+}
+
+function internals(agent: QwenAgent): QwenPermissionInternals {
+  return agent as unknown as QwenPermissionInternals;
+}
+
+afterEach(() => {
+  jest.useRealTimers();
+});
+
+describe('QwenAgent pending permission lifecycle', () => {
+  it('resolves an answered request once and clears its timeout', async () => {
+    jest.useFakeTimers();
+    const agent = createAgent();
+    agent.onPermissionRequest = ((request) => {
+      agent.respondToPermission(request.requestId, true);
+    }) satisfies PermissionCallback;
+
+    const responsePromise = internals(agent).handlePermissionRequest(
+      permissionRequest(),
+    );
+
+    expect(await responsePromise).toEqual({
+      outcome: { outcome: 'selected', optionId: 'proceed_once' },
+    });
+    expect(jest.getTimerCount()).toBe(0);
+
+    expect(internals(agent).pendingPermissions.size).toBe(0);
+    agent.destroy();
+  });
+
+  it('cancels every pending request when destroyed', async () => {
+    jest.useFakeTimers();
+    const agent = createAgent();
+    agent.onPermissionRequest = (() => {}) satisfies PermissionCallback;
+    const first = internals(agent).handlePermissionRequest(permissionRequest());
+    const second = internals(agent).handlePermissionRequest(
+      permissionRequest(),
+    );
+
+    expect(internals(agent).pendingPermissions.size).toBe(2);
+    agent.destroy();
+
+    await expect(first).resolves.toEqual({ outcome: { outcome: 'cancelled' } });
+    await expect(second).resolves.toEqual({
+      outcome: { outcome: 'cancelled' },
+    });
+    expect(internals(agent).pendingPermissions.size).toBe(0);
+    expect(jest.getTimerCount()).toBe(0);
+  });
+
+  it('cancels and removes a request when its response times out', async () => {
+    jest.useFakeTimers();
+    const agent = createAgent();
+    agent.onPermissionRequest = (() => {}) satisfies PermissionCallback;
+    const responsePromise = internals(agent).handlePermissionRequest(
+      permissionRequest(),
+    );
+
+    jest.runOnlyPendingTimers();
+
+    await expect(responsePromise).resolves.toEqual({
+      outcome: { outcome: 'cancelled' },
+    });
+    expect(internals(agent).pendingPermissions.size).toBe(0);
+    expect(jest.getTimerCount()).toBe(0);
+    agent.destroy();
+  });
+
+  it('ignores a response that arrives after the request times out', async () => {
+    jest.useFakeTimers();
+    const agent = createAgent();
+    let requestId = '';
+    agent.onPermissionRequest = ((request) => {
+      requestId = request.requestId;
+    }) satisfies PermissionCallback;
+    const responsePromise = internals(agent).handlePermissionRequest(
+      permissionRequest(),
+    );
+
+    jest.runOnlyPendingTimers();
+    expect(await responsePromise).toEqual({
+      outcome: { outcome: 'cancelled' },
+    });
+
+    agent.respondToPermission(requestId, true);
+    expect(await responsePromise).toEqual({
+      outcome: { outcome: 'cancelled' },
+    });
+    expect(internals(agent).pendingPermissions.size).toBe(0);
+    agent.destroy();
+  });
+
+  it('does not register a timeout when permission prompts are unset', async () => {
+    jest.useFakeTimers();
+    const agent = createAgent('allow-all');
+
+    await expect(
+      internals(agent).handlePermissionRequest(permissionRequest()),
+    ).resolves.toEqual({
+      outcome: { outcome: 'selected', optionId: 'proceed_once' },
+    });
+    expect(jest.getTimerCount()).toBe(0);
+    expect(internals(agent).pendingPermissions.size).toBe(0);
+    agent.destroy();
+  });
+});

packages/desktop/packages/shared/src/agent/qwen-agent.ts

@@ -156,6 +156,7 @@ type PendingPermission = {
     response: RequestPermissionResponse & { answers?: Record<string, string> },
   ) => void;
   options: AcpPermissionOption[];
+  timeout: ReturnType<typeof setTimeout>;
 };
 
 type MiniCollector = {
@@ -184,6 +185,7 @@ type SlashCommandInvocation = {
 const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue';
 const DEFAULT_REQUEST_TIMEOUT_MS = 30_000;
 const DEFAULT_INITIALIZE_TIMEOUT_MS = 120_000;
+const PERMISSION_REQUEST_TIMEOUT_MS = 5 * 60_000;
 const INCLUDE_CRAFT_CONTEXT_IN_QWEN_PROMPTS = false;
 const SHARED_ACP_IDLE_TTL_MS = 5 * 60_000;
 
@@ -2030,8 +2032,8 @@ export class QwenAgent extends BaseAgent {
     const pending = this.pendingPermissions.get(requestId);
     if (!pending) return;
 
-    this.pendingPermissions.delete(requestId);
-    pending.resolve(
+    this.resolvePendingPermission(
+      requestId,
       this.createPermissionResponse(
         pending.options,
         allowed,
@@ -2689,7 +2691,7 @@ export class QwenAgent extends BaseAgent {
   override destroy(): void {
     super.destroy();
     this.killSubprocess();
-    this.pendingPermissions.clear();
+    this.cancelPendingPermissions();
     this.miniCollectors.clear();
     this.historyCollectors.clear();
     this.ensureProcessPromise = null;
@@ -5038,7 +5040,10 @@ export class QwenAgent extends BaseAgent {
 
     return new Promise<RequestPermissionResponse>((resolve) => {
       const requestId = `qwen-permission-${++this.permissionRequestCounter}`;
-      this.pendingPermissions.set(requestId, { resolve, options });
+      const timeout = setTimeout(() => {
+        this.respondToPermission(requestId, false);
+      }, PERMISSION_REQUEST_TIMEOUT_MS);
+      this.pendingPermissions.set(requestId, { resolve, options, timeout });
 
       try {
         this.onPermissionRequest?.({
@@ -5058,8 +5063,7 @@ export class QwenAgent extends BaseAgent {
         this.debug(
           `Qwen permission callback failed: ${error instanceof Error ? error.message : String(error)}`,
         );
-        this.pendingPermissions.delete(requestId);
-        resolve(this.createPermissionResponse(options, false, false));
+        this.respondToPermission(requestId, false);
       }
     });
   }
@@ -5124,12 +5128,21 @@ export class QwenAgent extends BaseAgent {
   }
 
   private cancelPendingPermissions(): void {
-    for (const [, pending] of this.pendingPermissions) {
-      pending.resolve(
-        this.createPermissionResponse(pending.options, false, false),
-      );
+    for (const requestId of this.pendingPermissions.keys()) {
+      this.respondToPermission(requestId, false);
     }
-    this.pendingPermissions.clear();
+  }
+
+  private resolvePendingPermission(
+    requestId: string,
+    response: RequestPermissionResponse & { answers?: Record<string, string> },
+  ): void {
+    const pending = this.pendingPermissions.get(requestId);
+    if (!pending) return;
+
+    clearTimeout(pending.timeout);
+    this.pendingPermissions.delete(requestId);
+    pending.resolve(response);
   }
 
   protected override debug(message: string): void {