fix(core): fall back to CLI confirmation when IDE diff open fails

[yiliang114]

TLDR

Follow-up fix for the scheduler-based IDE diff refactor in #2728.

When CoreToolScheduler.openIdeDiffIfEnabled() cannot open the IDE diff because the IDE companion disconnected, the MCP request failed, or openDiff returned an error, Qwen Code should fall back to the existing CLI confirmation flow instead of surfacing an unhandled rejection.

This is a stacked PR and currently depends on #2728.

Screenshots / Video Demo

N/A — no user-facing change in the normal success path. This only hardens the IDE diff error path and preserves the existing CLI confirmation fallback.

Dive Deeper

Dependency note

• Depends on refactor: centralize IDE diff interaction in CoreToolScheduler #2728
• This stacked PR is currently based on deps/pr-2728-coretoolscheduler
• After refactor: centralize IDE diff interaction in CoreToolScheduler #2728 is merged, this PR can be retargeted to main

Problem

The scheduler-level IDE diff flow introduced in #2728 intentionally opens the IDE diff asynchronously so CLI and IDE confirmation can coexist. However, the IDE-open path did not locally handle failures from IdeClient.getInstance() / ideClient.openDiff().

That meant IDE-side failures such as:

• IDE companion disconnection
• MCP request errors
• openDiff tool errors

could escape as unhandled rejections instead of degrading gracefully to the already-available CLI confirmation path.

Solution

• Wrap the IDE diff open path in CoreToolScheduler.openIdeDiffIfEnabled() with local error handling
• Log a warning when opening the IDE diff fails
• Keep the tool in the normal CLI awaiting_approval state so the user can still confirm or cancel from the terminal
• Add a regression test covering the openDiff() rejection path

Reviewer Test Plan

1. Check out this PR on top of refactor: centralize IDE diff interaction in CoreToolScheduler #2728.
2. Run npm run test --workspace=packages/core -- src/core/coreToolScheduler.test.ts.
3. Verify the new regression test passes:
   should fall back to CLI confirmation when opening the IDE diff fails
4. Optionally mock or instrument the IDE client so openDiff() rejects, then confirm the tool remains in CLI confirmation flow instead of producing an unhandled rejection.

Testing Matrix

	🍏	🪟	🐧
npm run	✅	❓	❓
npx	❓	❓	❓
Docker	❓	❓	❓
Podman	❓	-	-
Seatbelt	❓	-	-

Linked issues / bugs

• Depends on refactor: centralize IDE diff interaction in CoreToolScheduler #2728
• Related to the IDE diff error-path handling introduced by refactor: centralize IDE diff interaction in CoreToolScheduler #2728

[github-actions]

📋 Review Summary

This PR introduces error handling for the IDE diff open path in CoreToolScheduler.openIdeDiffIfEnabled(), ensuring that IDE-side failures (disconnection, MCP errors, tool errors) are caught and logged rather than escaping as unhandled rejections. The implementation correctly preserves the existing CLI confirmation fallback when IDE diff fails. The changes are well-scoped and include appropriate test coverage.

🔍 General Feedback

• Positive aspects:

  • Clean error handling pattern that gracefully degrades to CLI confirmation
  • Good test coverage for the error path scenario
  • Proper use of debugLogger.warn() for logging failures without disrupting user experience
  • Correct handling of aborted signals to avoid spurious warnings
  • Maintains backward compatibility - no user-facing changes in the success path

• Code quality:

  • The try-catch wrapper is appropriately scoped around only the IDE-specific operations
  • Guard clause for signal.aborted in the catch block prevents noise during cancellation
  • Consistent with existing error handling patterns in the codebase

🎯 Specific Feedback

🔵 Low

• File: packages/core/src/core/coreToolScheduler.ts:1250-1252 - Consider whether the error should be logged at warn level or debug level. Since this is an expected fallback scenario (not truly a warning condition) and the CLI confirmation flow continues normally, debugLogger.debug() might be more appropriate than debugLogger.warn(). However, this depends on your logging philosophy for graceful degradation scenarios.

  Suggestion:

  } catch (error) {
    if (!signal.aborted) {
      debugLogger.debug(  // Consider 'debug' instead of 'warn'
        `IDE diff open failed for ${callId}: ${error instanceof Error ? error.message : String(error)}`,
      );
    }
  }

• File: packages/core/src/core/coreToolScheduler.test.ts:3488-3525 - The test could benefit from an additional assertion to verify that the warning was logged via debugLogger. This would ensure the error path is fully exercised and documented.

  Suggestion:

  // Add mock for debugLogger if not already mocked
  import { createDebugLogger } from '../utils/debugLogger.js';
  // Then assert the warning was logged

• File: packages/core/src/core/coreToolScheduler.ts:1250 - Minor consistency note: the error message format uses template literal interpolation for the error message. Consider using a more structured format that includes the error type for better debugging:

  Suggestion:

  debugLogger.warn(
    `IDE diff open failed for ${callId}: ${error instanceof Error ? error.message : String(error)} (${error?.constructor?.name || 'Unknown'})`,
  );

✅ Highlights

• Excellent defensive programming - wrapping async IDE operations with proper error handling
• The test case "should fall back to CLI confirmation when opening the IDE diff fails" is well-named and clearly documents the expected behavior
• Maintains the existing CLI confirmation flow as a fallback, ensuring users can still confirm/cancel operations even when IDE companion is unavailable
• Proper handling of the abort signal prevents false warnings during user-initiated cancellations
• No breaking changes - purely additive error handling

[wenshao]

[Suggestion] try-catch 范围过宽 — handleConfirmationResponse 错误被误报为 "IDE diff open failed"

try 块不仅包裹了 IDE diff 操作（IdeClient.getInstance()、openDiff()），还包裹了下游的 handleConfirmationResponse() 调用。由于 openIdeDiffIfEnabled 在调用处（~第 1043 行）是 fire-and-forget 调用，之前 handleConfirmationResponse 的拒绝会作为 unhandled promise rejection 暴露。现在通过在现有 try-catch 内添加 await，这些错误被静默吞掉并记录为 "IDE diff open failed"——这是事实性错误。

建议将 try-catch 缩小到仅覆盖 IDE diff 操作，将 guard 检查和 handleConfirmationResponse 移到 try-catch 之外，失败时 return 回退到 CLI 确认。

Suggested change

	let resolution;
	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
	// triggers this handler AND the CLI's onConfirm, causing a
	// race where ProceedOnce overwrites ProceedAlways.
	const still = this.toolCalls.find(
	(c) => c.request.callId === callId && c.status === 'awaiting_approval',
	);
	if (!still) return;
	if (resolution.status === 'accepted') {
	// When content is unchanged, skip the inline modify path so that
	// the original tool params (e.g. partial old_string for edit tool)
	// are preserved. Mitigate the multi-edit-on-same-file issue (#2702)
	// for the common accept-without-edit case.
	const userEdited =
	resolution.content != null &&
	resolution.content !== confirmationDetails.newContent;
	await this.handleConfirmationResponse(
	callId,
	confirmationDetails.onConfirm,
	ToolConfirmationOutcome.ProceedOnce,
	signal,
	userEdited ? { newContent: resolution.content } : undefined,
	);
	} else {
	await this.handleConfirmationResponse(
	callId,
	confirmationDetails.onConfirm,
	ToolConfirmationOutcome.Cancel,
	signal,
	);
	}

— glm-5.1 via Qwen Code /review