mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-26 17:13:32 +00:00
fix(acp): sweep review worktree leases at the end of each prompt turn (#7694)
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-<n> and the qwen-review/pr-<n> 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.
This commit is contained in:
parent
1f9318f974
commit
effb59ff73
3 changed files with 273 additions and 0 deletions
|
|
@ -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<string | undefined>;
|
||||
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<typeof vi.fn>
|
||||
).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` }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -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-<n>`
|
||||
// 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'] }) =>
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue