mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-08 10:00:06 +00:00
fix(core): fall back to CLI confirmation when IDE diff open fails (#3031)
* refactor: centralize IDE diff interaction in CoreToolScheduler - Move openDiff/confirmation handling from edit.ts and write-file.ts into CoreToolScheduler.openIdeDiffIfEnabled(), called after permission hooks - Use structuredClone in buildInvocation to prevent params mutation leaking to LLM history (fixes #2709 token waste) - Use confirmationDetails as single data source for IDE diff content, only rely on ModifyContext.createUpdatedParams() for parameter transform - Skip inline modify when IDE content unchanged, preserving original tool params for multi-edit-on-same-file scenarios (mitigates #2702) - Remove ideConfirmation field from ToolEditConfirmationDetails - Remove dead resolveIdeDiffForOutcome from ACP Session.ts - Fix memory tool scope fallback in createUpdatedParams Closes #2709 Closes #2673 * fix(core): fall back to CLI confirmation when IDE diff open fails * fix(core): narrow IDE diff error handling scope --------- Co-authored-by: 胡玮文 <huweiwen.hww@alibaba-inc.com> Co-authored-by: tanzhenxin <tanzhenxing1987@gmail.com>
This commit is contained in:
parent
56392c7397
commit
19f2d292f9
2 changed files with 129 additions and 8 deletions
|
|
@ -3485,6 +3485,116 @@ describe('CoreToolScheduler IDE interaction', () => {
|
|||
expect(completedCalls[0].status).toBe('cancelled');
|
||||
});
|
||||
|
||||
it('should fall back to CLI confirmation when opening the IDE diff fails', async () => {
|
||||
const { mockConfig } = createIdeMockConfig({
|
||||
ideMode: true,
|
||||
});
|
||||
|
||||
mockIdeClient.openDiff.mockRejectedValue(new Error('IDE disconnected'));
|
||||
|
||||
const onAllToolCallsComplete = vi.fn();
|
||||
const onToolCallsUpdate = vi.fn();
|
||||
|
||||
const scheduler = new CoreToolScheduler({
|
||||
config: mockConfig,
|
||||
onAllToolCallsComplete,
|
||||
onToolCallsUpdate,
|
||||
getPreferredEditor: () => 'vscode',
|
||||
onEditorClose: vi.fn(),
|
||||
});
|
||||
|
||||
const request = {
|
||||
callId: 'ide-open-fail-1',
|
||||
name: 'mockModifiableTool',
|
||||
args: { param: 'value' },
|
||||
isClientInitiated: false,
|
||||
prompt_id: 'prompt-ide-open-fail-1',
|
||||
};
|
||||
|
||||
const abortController = new AbortController();
|
||||
await scheduler.schedule([request], abortController.signal);
|
||||
|
||||
const awaitingCall = (await waitForStatus(
|
||||
onToolCallsUpdate,
|
||||
'awaiting_approval',
|
||||
)) as WaitingToolCall;
|
||||
|
||||
expect(awaitingCall.status).toBe('awaiting_approval');
|
||||
expect(mockIdeClient.openDiff).toHaveBeenCalled();
|
||||
expect(onAllToolCallsComplete).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should not swallow confirmation handling errors after IDE diff opens', async () => {
|
||||
const { mockConfig } = createIdeMockConfig({
|
||||
ideMode: true,
|
||||
});
|
||||
|
||||
mockIdeClient.openDiff.mockResolvedValue({
|
||||
status: 'rejected',
|
||||
});
|
||||
|
||||
const scheduler = new CoreToolScheduler({
|
||||
config: mockConfig,
|
||||
onAllToolCallsComplete: vi.fn(),
|
||||
onToolCallsUpdate: vi.fn(),
|
||||
getPreferredEditor: () => 'vscode',
|
||||
onEditorClose: vi.fn(),
|
||||
});
|
||||
|
||||
const request = {
|
||||
callId: 'ide-confirmation-error-1',
|
||||
name: 'mockModifiableTool',
|
||||
args: { param: 'value' },
|
||||
isClientInitiated: false,
|
||||
prompt_id: 'prompt-ide-confirmation-error-1',
|
||||
};
|
||||
const confirmationDetails = {
|
||||
type: 'edit',
|
||||
title: 'Confirm Mock Tool',
|
||||
fileName: 'test.txt',
|
||||
filePath: 'test.txt',
|
||||
fileDiff: 'diff',
|
||||
originalContent: 'originalContent',
|
||||
newContent: 'newContent',
|
||||
onConfirm: vi.fn(),
|
||||
} satisfies ToolCallConfirmationDetails;
|
||||
const confirmationError = new Error('confirmation handling failed');
|
||||
|
||||
(
|
||||
scheduler as unknown as {
|
||||
toolCalls: WaitingToolCall[];
|
||||
}
|
||||
).toolCalls = [
|
||||
{
|
||||
status: 'awaiting_approval',
|
||||
request,
|
||||
tool: {} as never,
|
||||
invocation: {} as never,
|
||||
confirmationDetails,
|
||||
},
|
||||
];
|
||||
|
||||
vi.spyOn(scheduler, 'handleConfirmationResponse').mockRejectedValue(
|
||||
confirmationError,
|
||||
);
|
||||
|
||||
await expect(
|
||||
(
|
||||
scheduler as unknown as {
|
||||
openIdeDiffIfEnabled: (
|
||||
confirmationDetails: ToolCallConfirmationDetails,
|
||||
callId: string,
|
||||
signal: AbortSignal,
|
||||
) => Promise<void>;
|
||||
}
|
||||
).openIdeDiffIfEnabled(
|
||||
confirmationDetails,
|
||||
request.callId,
|
||||
new AbortController().signal,
|
||||
),
|
||||
).rejects.toThrow('confirmation handling failed');
|
||||
});
|
||||
|
||||
it('should not call openDiff when IDE mode is disabled', async () => {
|
||||
const { mockConfig } = createIdeMockConfig({
|
||||
ideMode: false,
|
||||
|
|
|
|||
|
|
@ -1219,13 +1219,24 @@ export class CoreToolScheduler {
|
|||
if (confirmationDetails.type !== 'edit' || !this.config.getIdeMode()) {
|
||||
return;
|
||||
}
|
||||
const ideClient = await IdeClient.getInstance();
|
||||
if (!ideClient.isDiffingEnabled()) return;
|
||||
|
||||
const resolution = await ideClient.openDiff(
|
||||
confirmationDetails.filePath,
|
||||
confirmationDetails.newContent,
|
||||
);
|
||||
let resolution: Awaited<ReturnType<IdeClient['openDiff']>>;
|
||||
try {
|
||||
const ideClient = await IdeClient.getInstance();
|
||||
if (!ideClient.isDiffingEnabled()) return;
|
||||
|
||||
resolution = await ideClient.openDiff(
|
||||
confirmationDetails.filePath,
|
||||
confirmationDetails.newContent,
|
||||
);
|
||||
} catch (error) {
|
||||
if (!signal.aborted) {
|
||||
debugLogger.warn(
|
||||
`IDE diff open failed for ${callId}: ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// Guard: skip if the tool was already handled (e.g. by CLI
|
||||
// confirmation). Without this check, resolveDiffFromCli
|
||||
|
|
@ -1244,7 +1255,7 @@ export class CoreToolScheduler {
|
|||
const userEdited =
|
||||
resolution.content != null &&
|
||||
resolution.content !== confirmationDetails.newContent;
|
||||
this.handleConfirmationResponse(
|
||||
await this.handleConfirmationResponse(
|
||||
callId,
|
||||
confirmationDetails.onConfirm,
|
||||
ToolConfirmationOutcome.ProceedOnce,
|
||||
|
|
@ -1252,7 +1263,7 @@ export class CoreToolScheduler {
|
|||
userEdited ? { newContent: resolution.content } : undefined,
|
||||
);
|
||||
} else {
|
||||
this.handleConfirmationResponse(
|
||||
await this.handleConfirmationResponse(
|
||||
callId,
|
||||
confirmationDetails.onConfirm,
|
||||
ToolConfirmationOutcome.Cancel,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue