From effb59ff73b3fe38cf691e8cb4003952ef5e1f35 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sat, 25 Jul 2026 08:58:42 +0800 Subject: [PATCH] fix(acp): sweep review worktree leases at the end of each prompt turn (#7694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP prompt path never entered promptIdContext — only the TUI (useGeminiStream) and headless (nonInteractiveCli) entry points do — so shell subprocesses in daemon sessions saw an empty QWEN_CODE_PROMPT_ID and `qwen review fetch-pr` silently skipped recording its worktree lease. A cancelled or errored /review in a Web Shell session therefore left .qwen/tmp/review-pr- and the qwen-review/pr- branch behind until the next review of the same PR happened to clean them up. - Bind promptIdContext in #executePromptInner (enterWith, mirroring the sessionIdContext wrapper in #executePrompt) so lease creation works and shell subprocesses can identify the prompt that spawned them. - Sweep the prompt's leases in the turn-wide finally, unconditionally like the headless path: the ACP loop runs whole turns, so unlike the TUI's per-continuation submitQuery this can never fire mid-review. No-op when the review's own cleanup step already released the lease. --- .../session/Session.review-lease.test.ts | 245 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 25 ++ .../session/Session.worktree.test.ts | 3 + 3 files changed, 273 insertions(+) create mode 100644 packages/cli/src/acp-integration/session/Session.review-lease.test.ts diff --git a/packages/cli/src/acp-integration/session/Session.review-lease.test.ts b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts new file mode 100644 index 0000000000..e580cf54b4 --- /dev/null +++ b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts @@ -0,0 +1,245 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + * + * ACP prompt turns and the /review worktree lease. + * + * Coverage: + * RL1: the turn body runs inside promptIdContext, so shell subprocesses + * (via getShellContextEnvVars) see QWEN_CODE_PROMPT_ID and + * `qwen review fetch-pr` can record its worktree lease. + * RL2: a completed prompt sweeps this prompt's review-worktree leases + * (no-op when the review's own cleanup step already cleared them). + * RL3: the sweep still runs when the model stream throws — the + * interrupted-/review case the lease mechanism exists for. + * RL4: consecutive prompts sweep under their own prompt IDs. + * + * Mirrors the harness in Session.worktree.test.ts: real Session, no + * module-level mock of @qwen-code/qwen-code-core. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { Session } from './Session.js'; +import type { Config, GeminiChat } from '@qwen-code/qwen-code-core'; +import { + ApprovalMode, + AuthType, + Storage, + promptIdContext, +} from '@qwen-code/qwen-code-core'; +import * as core from '@qwen-code/qwen-code-core'; +import type { + AgentSideConnection, + PromptRequest, +} from '@agentclientprotocol/sdk'; +import type { LoadedSettings } from '../../config/settings.js'; + +vi.mock('../../nonInteractiveCliCommands.js', () => ({ + ALLOWED_BUILTIN_COMMANDS_NON_INTERACTIVE: [], + getAvailableCommands: vi.fn().mockResolvedValue([]), + handleSlashCommand: vi.fn(), +})); + +const cleanupReviewWorktreeLeases = vi.hoisted(() => vi.fn()); +vi.mock('../../services/review-worktree-lease.js', () => ({ + cleanupReviewWorktreeLeases, +})); + +function createEmptyStream() { + return (async function* () {})(); +} + +function makePromptRequest(text = 'hello'): PromptRequest { + return { + sessionId: 'lease-test-session', + prompt: [{ type: 'text', text }], + }; +} + +describe('Session review-worktree lease sweep', () => { + const SESSION_ID = 'lease-test-session'; + const PROJECT_ROOT = '/repo'; + + /** promptIdContext store observed inside each model send. */ + let observedPromptIds: Array; + let mockChat: GeminiChat; + let mockConfig: Config; + let mockClient: AgentSideConnection; + let mockSettings: LoadedSettings; + + beforeEach(() => { + cleanupReviewWorktreeLeases.mockClear(); + observedPromptIds = []; + + mockChat = { + sendMessageStream: vi.fn().mockImplementation(async () => { + observedPromptIds.push(promptIdContext.getStore()); + return createEmptyStream(); + }), + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue([]), + setHistory: vi.fn(), + truncateHistory: vi.fn(), + stripThoughtsFromHistory: vi.fn(), + } as unknown as GeminiChat; + + const mockGeminiClient = { + getChat: vi.fn().mockReturnValue(mockChat), + tryCompressChat: vi.fn().mockResolvedValue({ + originalTokenCount: 0, + newTokenCount: 0, + compressionStatus: core.CompressionStatus.NOOP, + }), + }; + + mockConfig = { + storage: { + getRuntimeBaseDir: vi.fn(() => Storage.getRuntimeBaseDir()), + }, + setApprovalMode: vi.fn(), + getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), + switchModel: vi.fn(), + getModel: vi.fn().mockReturnValue('qwen3'), + getSessionId: vi.fn().mockReturnValue(SESSION_ID), + assertCanStartTurn: vi.fn().mockResolvedValue(undefined), + getWorkingDir: vi.fn().mockReturnValue('/tmp'), + getTelemetryLogPromptsEnabled: vi.fn().mockReturnValue(false), + getUsageStatisticsEnabled: vi.fn().mockReturnValue(false), + getContentGeneratorConfig: vi.fn().mockReturnValue(undefined), + getChatRecordingService: vi.fn().mockReturnValue({ + recordUserMessage: vi.fn(), + recordUiTelemetryEvent: vi.fn(), + recordToolResult: vi.fn(), + recordSlashCommand: vi.fn(), + rewindRecording: vi.fn(), + setTitleRecordedCallback: vi.fn(), + }), + getToolRegistry: vi.fn().mockReturnValue({ + getTool: vi.fn(), + ensureTool: vi.fn().mockResolvedValue(true), + }), + getFileService: vi.fn().mockReturnValue({ + shouldGitIgnoreFile: vi.fn().mockReturnValue(false), + }), + getFileFilteringRespectGitIgnore: vi.fn().mockReturnValue(true), + getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false), + getTargetDir: vi.fn().mockReturnValue('/tmp'), + getProjectRoot: vi.fn().mockReturnValue(PROJECT_ROOT), + getDebugMode: vi.fn().mockReturnValue(false), + getAuthType: vi.fn().mockReturnValue(AuthType.USE_OPENAI), + isCronEnabled: vi.fn().mockReturnValue(false), + getSessionTokenLimit: vi.fn().mockReturnValue(0), + getGeminiClient: vi.fn().mockReturnValue(mockGeminiClient), + getDisableAllHooks: vi.fn().mockReturnValue(true), + hasHooksForEvent: vi.fn().mockReturnValue(false), + getMessageBus: vi.fn().mockReturnValue(undefined), + getStopHookBlockingCap: vi.fn().mockReturnValue(0), + getBackgroundTaskRegistry: vi.fn().mockReturnValue({ + setNotificationCallback: vi.fn(), + }), + getMonitorRegistry: vi.fn().mockReturnValue({ + setNotificationCallback: vi.fn(), + }), + getBackgroundShellRegistry: vi.fn().mockReturnValue({ + setNotificationCallback: vi.fn(), + }), + setSubSessionSpawner: vi.fn(), + getSubSessionSpawner: vi.fn(), + } as unknown as Config; + + mockClient = { + sessionUpdate: vi.fn().mockResolvedValue(undefined), + requestPermission: vi.fn().mockResolvedValue({ + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }), + extNotification: vi.fn().mockResolvedValue(undefined), + } as unknown as AgentSideConnection; + + mockSettings = { + merged: {}, + isTrusted: false, + user: { settings: {} }, + workspace: { settings: {} }, + setValue: vi.fn(), + } as unknown as LoadedSettings; + }); + + afterEach(() => { + Storage.setRuntimeBaseDir(null); + vi.restoreAllMocks(); + vi.clearAllTimers(); + }); + + it('RL1: the turn body observes this prompt in promptIdContext', async () => { + const session = new Session( + SESSION_ID, + mockConfig, + mockClient, + mockSettings, + ); + + await session.prompt(makePromptRequest()); + + expect(observedPromptIds).toEqual([`${SESSION_ID}########1`]); + }); + + it('RL2: a completed prompt sweeps its review-worktree leases', async () => { + const session = new Session( + SESSION_ID, + mockConfig, + mockClient, + mockSettings, + ); + + await session.prompt(makePromptRequest()); + + expect(cleanupReviewWorktreeLeases).toHaveBeenCalledTimes(1); + expect(cleanupReviewWorktreeLeases).toHaveBeenCalledWith({ + sessionId: SESSION_ID, + promptId: `${SESSION_ID}########1`, + repositoryRoot: PROJECT_ROOT, + }); + }); + + it('RL3: the sweep still runs when the model stream throws', async () => { + ( + mockChat.sendMessageStream as ReturnType + ).mockRejectedValueOnce(new Error('stream exploded')); + const session = new Session( + SESSION_ID, + mockConfig, + mockClient, + mockSettings, + ); + + await session.prompt(makePromptRequest()).catch(() => {}); + + expect(cleanupReviewWorktreeLeases).toHaveBeenCalledTimes(1); + expect(cleanupReviewWorktreeLeases).toHaveBeenCalledWith( + expect.objectContaining({ promptId: `${SESSION_ID}########1` }), + ); + }); + + it('RL4: consecutive prompts sweep under their own prompt IDs', async () => { + const session = new Session( + SESSION_ID, + mockConfig, + mockClient, + mockSettings, + ); + + await session.prompt(makePromptRequest('first')); + await session.prompt(makePromptRequest('second')); + + expect(cleanupReviewWorktreeLeases).toHaveBeenCalledTimes(2); + expect(cleanupReviewWorktreeLeases).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ promptId: `${SESSION_ID}########1` }), + ); + expect(cleanupReviewWorktreeLeases).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ promptId: `${SESSION_ID}########2` }), + ); + }); +}); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 4fe7e1178b..51677df7e8 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -143,6 +143,7 @@ import { clearGoalTerminalObserver, setGoalTerminalObserver, sessionIdContext, + promptIdContext, dedupeToolCallsById, getProviderToolCallId, parsePositiveIntegerEnv, @@ -168,6 +169,7 @@ import { } from '@qwen-code/acp-bridge/bridgeTypes'; import { SERVE_CONTROL_EXT_METHODS } from '@qwen-code/acp-bridge/status'; import { getCommandSubcommandNames } from '../../services/commandMetadata.js'; +import { cleanupReviewWorktreeLeases } from '../../services/review-worktree-lease.js'; import { getEffectiveSupportedModes } from '../../services/commandUtils.js'; import { normalizeChannelDeliveryText } from '../../serve/channel-delivery.js'; import { readVoiceModel } from '../../services/voice-settings.js'; @@ -2425,6 +2427,16 @@ export class Session implements SessionContext { this.turn += 1; const promptId = this.config.getSessionId() + '########' + this.turn; + // Bind the prompt ID for the remainder of this turn, mirroring the + // sessionIdContext.run wrapper in #executePrompt. Shell subprocesses + // read it via getShellContextEnvVars (QWEN_CODE_PROMPT_ID) — without + // it, `qwen review fetch-pr` cannot record its worktree lease and an + // interrupted /review leaves the review worktree behind. TUI and + // headless enter this context at their prompt entry points + // (useGeminiStream.ts / nonInteractiveCli.ts); ACP had no equivalent. + // enterWith (not run) so the 500-line turn body below stays unnested; + // the binding dies with this async scope. + promptIdContext.enterWith(promptId); const parentContext = extractDaemonTraceContext(params); return await withInteractionSpan( @@ -3009,6 +3021,19 @@ export class Session implements SessionContext { turnCount, ), ); + // Remove review worktrees leased during this prompt and not + // released by the skill's own cleanup step — a cancelled or + // errored /review otherwise leaves `.qwen/tmp/review-pr-` + // and its branch behind. Unconditional like the headless + // finally (nonInteractiveCli.ts): the ACP turn loop runs + // whole turns, so unlike the TUI's per-continuation + // submitQuery this can never fire mid-review. No-op when the + // lease was already cleared by `qwen review cleanup`. + cleanupReviewWorktreeLeases({ + sessionId: this.config.getSessionId(), + promptId, + repositoryRoot: this.config.getProjectRoot(), + }); } }, (result: { stopReason: PromptResponse['stopReason'] }) => diff --git a/packages/cli/src/acp-integration/session/Session.worktree.test.ts b/packages/cli/src/acp-integration/session/Session.worktree.test.ts index 8da4005902..c23bae2833 100644 --- a/packages/cli/src/acp-integration/session/Session.worktree.test.ts +++ b/packages/cli/src/acp-integration/session/Session.worktree.test.ts @@ -131,6 +131,9 @@ describe('Session.pendingWorktreeNotice', () => { getFileFilteringRespectGitIgnore: vi.fn().mockReturnValue(true), getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false), getTargetDir: vi.fn().mockReturnValue('/tmp'), + // The prompt turn's finally sweeps review-worktree leases against the + // project root (see Session.review-lease.test.ts). + getProjectRoot: vi.fn().mockReturnValue('/tmp'), getDebugMode: vi.fn().mockReturnValue(false), getAuthType: vi.fn().mockReturnValue(AuthType.USE_OPENAI), isCronEnabled: vi.fn().mockReturnValue(false),