From 91c8b6f220153662f092823783efe58723c05591 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Sat, 18 Jul 2026 01:49:05 -0700 Subject: [PATCH] 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 --- .../pending-permissions-lifecycle.test.ts | 163 ++++++++++++++++++ .../packages/shared/src/agent/qwen-agent.ts | 35 ++-- 2 files changed, 187 insertions(+), 11 deletions(-) create mode 100644 packages/desktop/packages/shared/src/agent/__tests__/pending-permissions-lifecycle.test.ts diff --git a/packages/desktop/packages/shared/src/agent/__tests__/pending-permissions-lifecycle.test.ts b/packages/desktop/packages/shared/src/agent/__tests__/pending-permissions-lifecycle.test.ts new file mode 100644 index 0000000000..a74cc42df1 --- /dev/null +++ b/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; + pendingPermissions: Map; +}; + +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(); + }); +}); diff --git a/packages/desktop/packages/shared/src/agent/qwen-agent.ts b/packages/desktop/packages/shared/src/agent/qwen-agent.ts index 220c85e17a..0bd90ae176 100644 --- a/packages/desktop/packages/shared/src/agent/qwen-agent.ts +++ b/packages/desktop/packages/shared/src/agent/qwen-agent.ts @@ -156,6 +156,7 @@ type PendingPermission = { response: RequestPermissionResponse & { answers?: Record }, ) => void; options: AcpPermissionOption[]; + timeout: ReturnType; }; 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((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 }, + ): 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 {