From c904f4cecec4f0416c7625017d5c0b6cef2b11d2 Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Thu, 22 Jan 2026 21:15:02 +0100 Subject: [PATCH 01/10] fix: resolve infinite loop when using 'Modify with external editor' (#7669) This fix addresses the infinite loop issue reported in #7669 where selecting "Modify with external editor" would loop indefinitely when no editor was configured or available. Root cause: When getPreferredEditor() returned undefined, the code silently returned without changing the outcome, causing the while loop to repeat. Changes: - Add detectFirstAvailableEditor() to auto-detect available editors - Add resolveEditor() to handle editor resolution with proper error messages - Update confirmation.ts to break the loop and show error when editor unavailable - Update coreToolScheduler.ts to cancel operation with feedback when editor unavailable - Add 11 new tests for the new editor resolution functions The fix: 1. Properly validates editor availability before attempting to use it 2. Auto-detects an available editor if none is configured 3. Provides clear error messages explaining why the editor cannot be used 4. Breaks the loop gracefully instead of looping infinitely --- packages/core/src/core/coreToolScheduler.ts | 18 ++- packages/core/src/scheduler/confirmation.ts | 54 ++++++++- packages/core/src/utils/editor.test.ts | 128 ++++++++++++++++++++ packages/core/src/utils/editor.ts | 73 +++++++++++ 4 files changed, 263 insertions(+), 10 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 585d7b9bf68..4374018ab5a 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -12,7 +12,8 @@ import { type ToolConfirmationPayload, ToolConfirmationOutcome, } from '../tools/tools.js'; -import type { EditorType } from '../utils/editor.js'; +import { resolveEditor, type EditorType } from '../utils/editor.js'; +import { coreEvents } from '../utils/events.js'; import type { Config } from '../config/config.js'; import { PolicyDecision, ApprovalMode } from '../policy/types.js'; import { logToolCall } from '../telemetry/loggers.js'; @@ -758,8 +759,17 @@ export class CoreToolScheduler { } else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { const waitingToolCall = toolCall as WaitingToolCall; - const editorType = this.getPreferredEditor(); - if (!editorType) { + // Use resolveEditor to check availability and auto-detect if needed + const preferredEditor = this.getPreferredEditor(); + const resolution = resolveEditor(preferredEditor); + + if (!resolution.editor) { + // No editor available - emit error feedback and cancel the operation + // This fixes the infinite loop issue reported in #7669 + if (resolution.error) { + coreEvents.emitFeedback('error', resolution.error); + } + this.cancelAll(signal); return; } @@ -770,7 +780,7 @@ export class CoreToolScheduler { const result = await this.toolModifier.handleModifyWithEditor( waitingToolCall, - editorType, + resolution.editor, signal, ); diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index f8d5f6b6b4b..d71b9411d49 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -21,10 +21,11 @@ import type { ValidatingToolCall, WaitingToolCall } from './types.js'; import type { Config } from '../config/config.js'; import type { SchedulerStateManager } from './state-manager.js'; import type { ToolModificationHandler } from './tool-modifier.js'; -import type { EditorType } from '../utils/editor.js'; +import { resolveEditor, type EditorType } from '../utils/editor.js'; import type { DiffUpdateResult } from '../ide/ide-client.js'; import { fireToolNotificationHook } from '../core/coreToolHookTriggers.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { coreEvents } from '../utils/events.js'; export interface ConfirmationResult { outcome: ToolConfirmationOutcome; @@ -151,7 +152,21 @@ export async function resolveConfirmation( outcome = response.outcome; if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { - await handleExternalModification(deps, toolCall, signal); + const modResult = await handleExternalModification( + deps, + toolCall, + signal, + ); + if (!modResult.success) { + // Editor is not available - emit error feedback and break the loop + // by cancelling the operation to prevent infinite loop + if (modResult.error) { + coreEvents.emitFeedback('error', modResult.error); + } + // Break the loop by changing outcome to Cancel + // This prevents the infinite loop issue reported in #7669 + outcome = ToolConfirmationOutcome.Cancel; + } } else if (response.payload?.newContent) { await handleInlineModification(deps, toolCall, response.payload, signal); outcome = ToolConfirmationOutcome.ProceedOnce; @@ -178,8 +193,19 @@ async function notifyHooks( } } +/** + * Result of attempting external modification. + */ +interface ExternalModificationResult { + /** Whether the modification was successful (editor was opened) */ + success: boolean; + /** Error message if the modification failed */ + error?: string; +} + /** * Handles modification via an external editor (e.g. Vim). + * Returns a result indicating success or failure with an error message. */ async function handleExternalModification( deps: { @@ -189,14 +215,29 @@ async function handleExternalModification( }, toolCall: ValidatingToolCall, signal: AbortSignal, -): Promise { +): Promise { const { state, modifier, getPreferredEditor } = deps; - const editor = getPreferredEditor(); - if (!editor) return; + + // Use the new resolveEditor function which handles: + // 1. Checking if preferred editor is available + // 2. Auto-detecting an available editor if none is configured + // 3. Providing helpful error messages + const preferredEditor = getPreferredEditor(); + const resolution = resolveEditor(preferredEditor); + + if (!resolution.editor) { + // No editor available - return failure with error message + return { + success: false, + error: + resolution.error || + 'No external editor is available. Please run /editor to configure one.', + }; + } const result = await modifier.handleModifyWithEditor( state.firstActiveCall as WaitingToolCall, - editor, + resolution.editor, signal, ); if (result) { @@ -207,6 +248,7 @@ async function handleExternalModification( newInvocation, ); } + return { success: true }; } /** diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index b4d33c53778..a3ca385e7bf 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -19,6 +19,8 @@ import { openDiff, allowEditorTypeInSandbox, isEditorAvailable, + detectFirstAvailableEditor, + resolveEditor, type EditorType, } from './editor.js'; import { execSync, spawn, spawnSync } from 'node:child_process'; @@ -542,4 +544,130 @@ describe('editor utils', () => { expect(isEditorAvailable('neovim')).toBe(true); }); }); + + describe('detectFirstAvailableEditor', () => { + it('should return undefined when no editors are installed', () => { + (execSync as Mock).mockImplementation(() => { + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + expect(detectFirstAvailableEditor()).toBeUndefined(); + }); + + it('should prioritize terminal editors over GUI editors', () => { + // Mock vim as available + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('vim') && !cmd.includes('nvim')) { + return Buffer.from('/usr/bin/vim'); + } + if (cmd.includes('code')) { + return Buffer.from('/usr/bin/code'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + expect(detectFirstAvailableEditor()).toBe('vim'); + }); + + it('should return vim when vim is the only editor available in sandbox mode', () => { + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('vim') && !cmd.includes('nvim')) { + return Buffer.from('/usr/bin/vim'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', 'sandbox'); + expect(detectFirstAvailableEditor()).toBe('vim'); + }); + + it('should skip GUI editors in sandbox mode', () => { + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('code')) { + return Buffer.from('/usr/bin/code'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', 'sandbox'); + // vscode is installed but not allowed in sandbox + expect(detectFirstAvailableEditor()).toBeUndefined(); + }); + + it('should return first available terminal editor (neovim)', () => { + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('nvim')) { + return Buffer.from('/usr/bin/nvim'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + expect(detectFirstAvailableEditor()).toBe('neovim'); + }); + }); + + describe('resolveEditor', () => { + it('should return the preferred editor when available', () => { + (execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/vim')); + vi.stubEnv('SANDBOX', ''); + const result = resolveEditor('vim'); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + + it('should return error when preferred editor is not installed', () => { + (execSync as Mock).mockImplementation(() => { + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + const result = resolveEditor('vim'); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('Vim'); + expect(result.error).toContain('not installed'); + }); + + it('should return error when preferred GUI editor cannot be used in sandbox mode', () => { + (execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code')); + vi.stubEnv('SANDBOX', 'sandbox'); + const result = resolveEditor('vscode'); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('VS Code'); + expect(result.error).toContain('sandbox mode'); + }); + + it('should auto-detect editor when no preference is set', () => { + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('vim') && !cmd.includes('nvim')) { + return Buffer.from('/usr/bin/vim'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + const result = resolveEditor(undefined); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + + it('should return error when no preference is set and no editors are available', () => { + (execSync as Mock).mockImplementation(() => { + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', ''); + const result = resolveEditor(undefined); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('No external editor'); + expect(result.error).toContain('/editor'); + }); + + it('should work with terminal editors in sandbox mode when no preference is set', () => { + (execSync as Mock).mockImplementation((cmd: string) => { + if (cmd.includes('vim') && !cmd.includes('nvim')) { + return Buffer.from('/usr/bin/vim'); + } + throw new Error('Command not found'); + }); + vi.stubEnv('SANDBOX', 'sandbox'); + const result = resolveEditor(undefined); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + }); }); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index e48a055d401..29dde6de9bb 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -142,6 +142,79 @@ export function isEditorAvailable(editor: string | undefined): boolean { return false; } +/** + * Detects the first available editor from the supported list. + * Prioritizes terminal editors (vim, neovim, emacs, hx) as they work in all environments + * including sandboxed mode, then falls back to GUI editors. + * Returns undefined if no supported editor is found. + */ +export function detectFirstAvailableEditor(): EditorType | undefined { + // Prioritize terminal editors as they work in sandbox mode + for (const editor of TERMINAL_EDITORS) { + if (isEditorAvailable(editor)) { + return editor; + } + } + // Fall back to GUI editors (won't work in sandbox mode but checked above) + for (const editor of GUI_EDITORS) { + if (isEditorAvailable(editor)) { + return editor; + } + } + return undefined; +} + +/** + * Result of attempting to resolve an editor for use. + */ +export interface EditorResolutionResult { + /** The editor to use, if available */ + editor?: EditorType; + /** Error message if no editor is available */ + error?: string; +} + +/** + * Resolves an editor to use for external editing. + * 1. If a preferred editor is set and available, uses it. + * 2. If a preferred editor is set but not available, returns an error. + * 3. If no preferred editor is set, attempts to auto-detect an available editor. + * 4. If no editor can be found, returns an error with instructions. + */ +export function resolveEditor( + preferredEditor: EditorType | undefined, +): EditorResolutionResult { + // Case 1: Preferred editor is set + if (preferredEditor) { + if (isEditorAvailable(preferredEditor)) { + return { editor: preferredEditor }; + } + // Preferred editor is set but not available + const displayName = getEditorDisplayName(preferredEditor); + if (!checkHasEditorType(preferredEditor)) { + return { + error: `${displayName} is configured as your preferred editor but is not installed. Please install it or run /editor to choose a different editor.`, + }; + } + // If the editor is installed but not available, it must be due to sandbox restrictions. + return { + error: `${displayName} cannot be used in sandbox mode. Please run /editor to choose a terminal-based editor (vim, neovim, emacs, or helix).`, + }; + } + + // Case 2: No preferred editor set, try to auto-detect + const detectedEditor = detectFirstAvailableEditor(); + if (detectedEditor) { + return { editor: detectedEditor }; + } + + // Case 3: No editor available at all + return { + error: + 'No external editor is configured or available. Please run /editor to set your preferred editor, or install one of the supported editors: vim, neovim, emacs, helix, VS Code, Cursor, Zed, or Windsurf.', + }; +} + /** * Get the diff command for a specific editor. */ From a526c7619184b1841199997822d93dc6d9da056b Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Sat, 24 Jan 2026 19:58:50 +0100 Subject: [PATCH 02/10] refactor(editor): use async functions to avoid blocking event loop Replace synchronous execSync calls with async alternatives in editor detection functions to prevent blocking the Node.js event loop. Changes: - Add commandExistsAsync using promisified exec - Add checkHasEditorTypeAsync, isEditorAvailableAsync, detectFirstAvailableEditorAsync, and resolveEditorAsync - Update confirmation.ts and coreToolScheduler.ts to use resolveEditorAsync - Mark synchronous resolveEditor as deprecated - Add comprehensive tests for all async functions The synchronous versions are kept for UI components that require synchronous execution (useEditorSettings, editorSettingsManager). --- packages/core/src/core/coreToolScheduler.ts | 7 +- packages/core/src/scheduler/confirmation.ts | 7 +- packages/core/src/utils/editor.test.ts | 163 +++++++++++++++++++- packages/core/src/utils/editor.ts | 116 +++++++++++++- 4 files changed, 285 insertions(+), 8 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 4374018ab5a..60bbeac90ed 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -12,7 +12,7 @@ import { type ToolConfirmationPayload, ToolConfirmationOutcome, } from '../tools/tools.js'; -import { resolveEditor, type EditorType } from '../utils/editor.js'; +import { resolveEditorAsync, type EditorType } from '../utils/editor.js'; import { coreEvents } from '../utils/events.js'; import type { Config } from '../config/config.js'; import { PolicyDecision, ApprovalMode } from '../policy/types.js'; @@ -759,9 +759,10 @@ export class CoreToolScheduler { } else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { const waitingToolCall = toolCall as WaitingToolCall; - // Use resolveEditor to check availability and auto-detect if needed + // Use resolveEditorAsync to check availability and auto-detect if needed + // Using async version to avoid blocking the event loop const preferredEditor = this.getPreferredEditor(); - const resolution = resolveEditor(preferredEditor); + const resolution = await resolveEditorAsync(preferredEditor); if (!resolution.editor) { // No editor available - emit error feedback and cancel the operation diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index d71b9411d49..e146ac640d2 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -21,7 +21,7 @@ import type { ValidatingToolCall, WaitingToolCall } from './types.js'; import type { Config } from '../config/config.js'; import type { SchedulerStateManager } from './state-manager.js'; import type { ToolModificationHandler } from './tool-modifier.js'; -import { resolveEditor, type EditorType } from '../utils/editor.js'; +import { resolveEditorAsync, type EditorType } from '../utils/editor.js'; import type { DiffUpdateResult } from '../ide/ide-client.js'; import { fireToolNotificationHook } from '../core/coreToolHookTriggers.js'; import { debugLogger } from '../utils/debugLogger.js'; @@ -218,12 +218,13 @@ async function handleExternalModification( ): Promise { const { state, modifier, getPreferredEditor } = deps; - // Use the new resolveEditor function which handles: + // Use the new resolveEditorAsync function which handles: // 1. Checking if preferred editor is available // 2. Auto-detecting an available editor if none is configured // 3. Providing helpful error messages + // Using async version to avoid blocking the event loop const preferredEditor = getPreferredEditor(); - const resolution = resolveEditor(preferredEditor); + const resolution = await resolveEditorAsync(preferredEditor); if (!resolution.editor) { // No editor available - return failure with error message diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index a3ca385e7bf..be91148169c 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -15,18 +15,23 @@ import { } from 'vitest'; import { checkHasEditorType, + checkHasEditorTypeAsync, getDiffCommand, openDiff, allowEditorTypeInSandbox, isEditorAvailable, + isEditorAvailableAsync, detectFirstAvailableEditor, + detectFirstAvailableEditorAsync, resolveEditor, + resolveEditorAsync, type EditorType, } from './editor.js'; -import { execSync, spawn, spawnSync } from 'node:child_process'; +import { exec, execSync, spawn, spawnSync } from 'node:child_process'; import { debugLogger } from './debugLogger.js'; vi.mock('child_process', () => ({ + exec: vi.fn(), execSync: vi.fn(), spawn: vi.fn(), spawnSync: vi.fn(() => ({ error: null, status: 0 })), @@ -670,4 +675,160 @@ describe('editor utils', () => { expect(result.error).toBeUndefined(); }); }); + + // Helper to create a mock exec that simulates async behavior + const mockExecAsync = (implementation: (cmd: string) => boolean): void => { + (exec as unknown as Mock).mockImplementation( + ( + cmd: string, + callback: (error: Error | null, stdout: string, stderr: string) => void, + ) => { + if (implementation(cmd)) { + callback(null, '/usr/bin/cmd', ''); + } else { + callback(new Error('Command not found'), '', ''); + } + }, + ); + }; + + describe('checkHasEditorTypeAsync', () => { + it('should return true if vim command exists', async () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + mockExecAsync((cmd) => cmd.includes('vim')); + expect(await checkHasEditorTypeAsync('vim')).toBe(true); + }); + + it('should return false if vim command does not exist', async () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + mockExecAsync(() => false); + expect(await checkHasEditorTypeAsync('vim')).toBe(false); + }); + + it('should check zed and zeditor commands in order', async () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + mockExecAsync((cmd) => cmd.includes('zeditor')); + expect(await checkHasEditorTypeAsync('zed')).toBe(true); + }); + }); + + describe('isEditorAvailableAsync', () => { + it('should return false for undefined editor', async () => { + expect(await isEditorAvailableAsync(undefined)).toBe(false); + }); + + it('should return false for empty string editor', async () => { + expect(await isEditorAvailableAsync('')).toBe(false); + }); + + it('should return false for invalid editor type', async () => { + expect(await isEditorAvailableAsync('invalid-editor')).toBe(false); + }); + + it('should return true for vscode when installed and not in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('code')); + vi.stubEnv('SANDBOX', ''); + expect(await isEditorAvailableAsync('vscode')).toBe(true); + }); + + it('should return false for vscode when not installed', async () => { + mockExecAsync(() => false); + expect(await isEditorAvailableAsync('vscode')).toBe(false); + }); + + it('should return false for vscode in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('code')); + vi.stubEnv('SANDBOX', 'sandbox'); + expect(await isEditorAvailableAsync('vscode')).toBe(false); + }); + + it('should return true for vim in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('vim')); + vi.stubEnv('SANDBOX', 'sandbox'); + expect(await isEditorAvailableAsync('vim')).toBe(true); + }); + }); + + describe('detectFirstAvailableEditorAsync', () => { + it('should return undefined when no editors are installed', async () => { + mockExecAsync(() => false); + vi.stubEnv('SANDBOX', ''); + expect(await detectFirstAvailableEditorAsync()).toBeUndefined(); + }); + + it('should prioritize terminal editors over GUI editors', async () => { + mockExecAsync( + (cmd) => + (cmd.includes('vim') && !cmd.includes('nvim')) || + cmd.includes('code'), + ); + vi.stubEnv('SANDBOX', ''); + expect(await detectFirstAvailableEditorAsync()).toBe('vim'); + }); + + it('should return vim in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); + vi.stubEnv('SANDBOX', 'sandbox'); + expect(await detectFirstAvailableEditorAsync()).toBe('vim'); + }); + + it('should skip GUI editors in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('code')); + vi.stubEnv('SANDBOX', 'sandbox'); + expect(await detectFirstAvailableEditorAsync()).toBeUndefined(); + }); + }); + + describe('resolveEditorAsync', () => { + it('should return the preferred editor when available', async () => { + mockExecAsync((cmd) => cmd.includes('vim')); + vi.stubEnv('SANDBOX', ''); + const result = await resolveEditorAsync('vim'); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + + it('should return error when preferred editor is not installed', async () => { + mockExecAsync(() => false); + vi.stubEnv('SANDBOX', ''); + const result = await resolveEditorAsync('vim'); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('Vim'); + expect(result.error).toContain('not installed'); + }); + + it('should return error when preferred GUI editor cannot be used in sandbox mode', async () => { + mockExecAsync((cmd) => cmd.includes('code')); + vi.stubEnv('SANDBOX', 'sandbox'); + const result = await resolveEditorAsync('vscode'); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('VS Code'); + expect(result.error).toContain('sandbox mode'); + }); + + it('should auto-detect editor when no preference is set', async () => { + mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); + vi.stubEnv('SANDBOX', ''); + const result = await resolveEditorAsync(undefined); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + + it('should return error when no preference is set and no editors are available', async () => { + mockExecAsync(() => false); + vi.stubEnv('SANDBOX', ''); + const result = await resolveEditorAsync(undefined); + expect(result.editor).toBeUndefined(); + expect(result.error).toContain('No external editor'); + expect(result.error).toContain('/editor'); + }); + + it('should work with terminal editors in sandbox mode when no preference is set', async () => { + mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); + vi.stubEnv('SANDBOX', 'sandbox'); + const result = await resolveEditorAsync(undefined); + expect(result.editor).toBe('vim'); + expect(result.error).toBeUndefined(); + }); + }); }); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 29dde6de9bb..2d16ddd3745 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -4,7 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { execSync, spawn, spawnSync } from 'node:child_process'; +import { exec, execSync, spawn, spawnSync } from 'node:child_process'; +import { promisify } from 'node:util'; import { debugLogger } from './debugLogger.js'; import { coreEvents, CoreEvent } from './events.js'; @@ -73,6 +74,8 @@ interface DiffCommand { args: string[]; } +const execAsync = promisify(exec); + function commandExists(cmd: string): boolean { try { execSync( @@ -85,6 +88,17 @@ function commandExists(cmd: string): boolean { } } +async function commandExistsAsync(cmd: string): Promise { + try { + await execAsync( + process.platform === 'win32' ? `where.exe ${cmd}` : `command -v ${cmd}`, + ); + return true; + } catch { + return false; + } +} + /** * Editor command configurations for different platforms. * Each editor can have multiple possible command names, listed in order of preference. @@ -112,6 +126,20 @@ export function checkHasEditorType(editor: EditorType): boolean { return commands.some((cmd) => commandExists(cmd)); } +export async function checkHasEditorTypeAsync( + editor: EditorType, +): Promise { + const commandConfig = editorCommands[editor]; + const commands = + process.platform === 'win32' ? commandConfig.win32 : commandConfig.default; + for (const cmd of commands) { + if (await commandExistsAsync(cmd)) { + return true; + } + } + return false; +} + export function getEditorCommand(editor: EditorType): string { const commandConfig = editorCommands[editor]; const commands = @@ -142,6 +170,23 @@ export function isEditorAvailable(editor: string | undefined): boolean { return false; } +/** + * Async version of isEditorAvailable. + * Check if the editor is valid and can be used without blocking the event loop. + * Returns false if preferred editor is not set / invalid / not available / not allowed in sandbox. + */ +export async function isEditorAvailableAsync( + editor: string | undefined, +): Promise { + if (editor && isValidEditorType(editor)) { + return ( + (await checkHasEditorTypeAsync(editor)) && + allowEditorTypeInSandbox(editor) + ); + } + return false; +} + /** * Detects the first available editor from the supported list. * Prioritizes terminal editors (vim, neovim, emacs, hx) as they work in all environments @@ -164,6 +209,31 @@ export function detectFirstAvailableEditor(): EditorType | undefined { return undefined; } +/** + * Async version of detectFirstAvailableEditor. + * Detects the first available editor from the supported list without blocking the event loop. + * Prioritizes terminal editors (vim, neovim, emacs, hx) as they work in all environments + * including sandboxed mode, then falls back to GUI editors. + * Returns undefined if no supported editor is found. + */ +export async function detectFirstAvailableEditorAsync(): Promise< + EditorType | undefined +> { + // Prioritize terminal editors as they work in sandbox mode + for (const editor of TERMINAL_EDITORS) { + if (await isEditorAvailableAsync(editor)) { + return editor; + } + } + // Fall back to GUI editors (won't work in sandbox mode but checked above) + for (const editor of GUI_EDITORS) { + if (await isEditorAvailableAsync(editor)) { + return editor; + } + } + return undefined; +} + /** * Result of attempting to resolve an editor for use. */ @@ -180,6 +250,8 @@ export interface EditorResolutionResult { * 2. If a preferred editor is set but not available, returns an error. * 3. If no preferred editor is set, attempts to auto-detect an available editor. * 4. If no editor can be found, returns an error with instructions. + * + * @deprecated Use resolveEditorAsync instead to avoid blocking the event loop. */ export function resolveEditor( preferredEditor: EditorType | undefined, @@ -215,6 +287,48 @@ export function resolveEditor( }; } +/** + * Async version of resolveEditor. + * Resolves an editor to use for external editing without blocking the event loop. + * 1. If a preferred editor is set and available, uses it. + * 2. If a preferred editor is set but not available, returns an error. + * 3. If no preferred editor is set, attempts to auto-detect an available editor. + * 4. If no editor can be found, returns an error with instructions. + */ +export async function resolveEditorAsync( + preferredEditor: EditorType | undefined, +): Promise { + // Case 1: Preferred editor is set + if (preferredEditor) { + if (await isEditorAvailableAsync(preferredEditor)) { + return { editor: preferredEditor }; + } + // Preferred editor is set but not available + const displayName = getEditorDisplayName(preferredEditor); + if (!(await checkHasEditorTypeAsync(preferredEditor))) { + return { + error: `${displayName} is configured as your preferred editor but is not installed. Please install it or run /editor to choose a different editor.`, + }; + } + // If the editor is installed but not available, it must be due to sandbox restrictions. + return { + error: `${displayName} cannot be used in sandbox mode. Please run /editor to choose a terminal-based editor (vim, neovim, emacs, or helix).`, + }; + } + + // Case 2: No preferred editor set, try to auto-detect + const detectedEditor = await detectFirstAvailableEditorAsync(); + if (detectedEditor) { + return { editor: detectedEditor }; + } + + // Case 3: No editor available at all + return { + error: + 'No external editor is configured or available. Please run /editor to set your preferred editor, or install one of the supported editors: vim, neovim, emacs, helix, VS Code, Cursor, Zed, or Windsurf.', + }; +} + /** * Get the diff command for a specific editor. */ From 4bd33f3742e3c21a6ea206648a9c71e818500893 Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Sat, 24 Jan 2026 20:05:33 +0100 Subject: [PATCH 03/10] refactor(editor): extract command construction to shared function Extract the platform-specific command construction into getCommandExistsCmd() to avoid duplication between commandExists and commandExistsAsync. --- packages/core/src/utils/editor.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 2d16ddd3745..cbc1209b676 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -76,12 +76,15 @@ interface DiffCommand { const execAsync = promisify(exec); +function getCommandExistsCmd(cmd: string): string { + return process.platform === 'win32' + ? `where.exe ${cmd}` + : `command -v ${cmd}`; +} + function commandExists(cmd: string): boolean { try { - execSync( - process.platform === 'win32' ? `where.exe ${cmd}` : `command -v ${cmd}`, - { stdio: 'ignore' }, - ); + execSync(getCommandExistsCmd(cmd), { stdio: 'ignore' }); return true; } catch { return false; @@ -90,9 +93,7 @@ function commandExists(cmd: string): boolean { async function commandExistsAsync(cmd: string): Promise { try { - await execAsync( - process.platform === 'win32' ? `where.exe ${cmd}` : `command -v ${cmd}`, - ); + await execAsync(getCommandExistsCmd(cmd)); return true; } catch { return false; From 37dabd873af7a04951c0d831b42012308a160655 Mon Sep 17 00:00:00 2001 From: Philippe GRANGER Date: Wed, 4 Feb 2026 16:16:36 +0100 Subject: [PATCH 04/10] fix(confirmation): use type guard for ToolConfirmationPayload union type Replace direct property access with 'in' operator to properly narrow the union type, fixing TypeScript compilation error. --- packages/core/src/scheduler/confirmation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index 5f2e1d1bc12..3d1ef3b46c5 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -171,7 +171,7 @@ export async function resolveConfirmation( // This prevents the infinite loop issue reported in #7669 outcome = ToolConfirmationOutcome.Cancel; } - } else if (response.payload?.newContent) { + } else if (response.payload && 'newContent' in response.payload) { await handleInlineModification(deps, toolCall, response.payload, signal); outcome = ToolConfirmationOutcome.ProceedOnce; } From 02fc2aaef22889dea9f1a42d8427d4933ce67d9b Mon Sep 17 00:00:00 2001 From: ehedlund Date: Wed, 4 Feb 2026 14:00:08 -0500 Subject: [PATCH 05/10] fix(core): resolve infinite loop and improve editor selection flow - Fixes an infinite loop when using 'Modify with Editor' without a configured editor. - Implements interactive editor selection via a UI dialog. - Returns to the previous confirmation prompt if selection is cancelled or fails. - Simplifies editor availability logic and removes deprecated sync functions. Fixes #7669 --- packages/cli/src/ui/AppContainer.tsx | 26 +- .../cli/src/ui/hooks/useEditorSettings.ts | 4 + packages/core/src/core/coreToolScheduler.ts | 23 +- packages/core/src/scheduler/confirmation.ts | 19 +- packages/core/src/utils/editor.test.ts | 255 +++++------------- packages/core/src/utils/editor.ts | 187 +++---------- packages/core/src/utils/events.ts | 12 + 7 files changed, 147 insertions(+), 379 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 7c105699023..9799382d453 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -524,12 +524,22 @@ export const AppContainer = (props: AppContainerProps) => { refreshStatic(); }, [refreshStatic, isAlternateBuffer, app, config]); + const [editorError, setEditorError] = useState(null); + const { + isEditorDialogOpen, + openEditorDialog, + handleEditorSelect, + exitEditorDialog, + } = useEditorSettings(settings, setEditorError, historyManager.addItem); + useEffect(() => { coreEvents.on(CoreEvent.ExternalEditorClosed, handleEditorClose); + coreEvents.on(CoreEvent.RequestEditorSelection, openEditorDialog); return () => { coreEvents.off(CoreEvent.ExternalEditorClosed, handleEditorClose); + coreEvents.off(CoreEvent.RequestEditorSelection, openEditorDialog); }; - }, [handleEditorClose]); + }, [handleEditorClose, openEditorDialog]); useEffect(() => { if ( @@ -543,6 +553,9 @@ export const AppContainer = (props: AppContainerProps) => { } }, [bannerVisible, bannerText, settings, config, refreshStatic]); + const { isSettingsDialogOpen, openSettingsDialog, closeSettingsDialog } = + useSettingsCommand(); + const { isThemeDialogOpen, openThemeDialog, @@ -738,17 +751,6 @@ Logging in with Google... Restarting Gemini CLI to continue. onAuthError, ]); - const [editorError, setEditorError] = useState(null); - const { - isEditorDialogOpen, - openEditorDialog, - handleEditorSelect, - exitEditorDialog, - } = useEditorSettings(settings, setEditorError, historyManager.addItem); - - const { isSettingsDialogOpen, openSettingsDialog, closeSettingsDialog } = - useSettingsCommand(); - const { isModelDialogOpen, openModelDialog, closeModelDialog } = useModelCommand(); diff --git a/packages/cli/src/ui/hooks/useEditorSettings.ts b/packages/cli/src/ui/hooks/useEditorSettings.ts index fa152026613..a855b0b4052 100644 --- a/packages/cli/src/ui/hooks/useEditorSettings.ts +++ b/packages/cli/src/ui/hooks/useEditorSettings.ts @@ -15,6 +15,8 @@ import { allowEditorTypeInSandbox, checkHasEditorType, getEditorDisplayName, + coreEvents, + CoreEvent, } from '@google/gemini-cli-core'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; @@ -66,6 +68,7 @@ export const useEditorSettings = ( ); setEditorError(null); setIsEditorDialogOpen(false); + coreEvents.emit(CoreEvent.EditorSelected, { editor: editorType }); } catch (error) { setEditorError(`Failed to set editor preference: ${error}`); } @@ -75,6 +78,7 @@ export const useEditorSettings = ( const exitEditorDialog = useCallback(() => { setIsEditorDialogOpen(false); + coreEvents.emit(CoreEvent.EditorSelected, { editor: undefined }); }, []); return { diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index f1f37b6c5c8..79d5f132a6d 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -752,18 +752,19 @@ export class CoreToolScheduler { } else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { const waitingToolCall = toolCall as WaitingToolCall; - // Use resolveEditorAsync to check availability and auto-detect if needed - // Using async version to avoid blocking the event loop const preferredEditor = this.getPreferredEditor(); - const resolution = await resolveEditorAsync(preferredEditor); + const editor = await resolveEditorAsync(preferredEditor, signal); - if (!resolution.editor) { - // No editor available - emit error feedback and cancel the operation - // This fixes the infinite loop issue reported in #7669 - if (resolution.error) { - coreEvents.emitFeedback('error', resolution.error); - } - this.cancelAll(signal); + if (!editor) { + // No editor available - emit error feedback and return to previous confirmation screen + coreEvents.emitFeedback( + 'error', + 'No external editor is available. Please run /editor to configure one.', + ); + this.setStatusInternal(callId, 'awaiting_approval', signal, { + ...waitingToolCall.confirmationDetails, + isModifying: false, + } as ToolCallConfirmationDetails); return; } @@ -774,7 +775,7 @@ export class CoreToolScheduler { const result = await this.toolModifier.handleModifyWithEditor( waitingToolCall, - resolution.editor, + editor, signal, ); diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index 3d1ef3b46c5..dbd17c6312e 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -162,14 +162,11 @@ export async function resolveConfirmation( signal, ); if (!modResult.success) { - // Editor is not available - emit error feedback and break the loop - // by cancelling the operation to prevent infinite loop + // Editor is not available - emit error feedback and stay in the loop + // to return to previous confirmation screen. if (modResult.error) { coreEvents.emitFeedback('error', modResult.error); } - // Break the loop by changing outcome to Cancel - // This prevents the infinite loop issue reported in #7669 - outcome = ToolConfirmationOutcome.Cancel; } } else if (response.payload && 'newContent' in response.payload) { await handleInlineModification(deps, toolCall, response.payload, signal); @@ -222,27 +219,21 @@ async function handleExternalModification( ): Promise { const { state, modifier, getPreferredEditor } = deps; - // Use the new resolveEditorAsync function which handles: - // 1. Checking if preferred editor is available - // 2. Auto-detecting an available editor if none is configured - // 3. Providing helpful error messages - // Using async version to avoid blocking the event loop const preferredEditor = getPreferredEditor(); - const resolution = await resolveEditorAsync(preferredEditor); + const editor = await resolveEditorAsync(preferredEditor, signal); - if (!resolution.editor) { + if (!editor) { // No editor available - return failure with error message return { success: false, error: - resolution.error || 'No external editor is available. Please run /editor to configure one.', }; } const result = await modifier.handleModifyWithEditor( state.firstActiveCall as WaitingToolCall, - resolution.editor, + editor, signal, ); if (result) { diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index b4aa98b253f..7f31ca1c12b 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -21,12 +21,10 @@ import { allowEditorTypeInSandbox, isEditorAvailable, isEditorAvailableAsync, - detectFirstAvailableEditor, - detectFirstAvailableEditorAsync, - resolveEditor, resolveEditorAsync, type EditorType, } from './editor.js'; +import { coreEvents, CoreEvent } from './events.js'; import { exec, execSync, spawn, spawnSync } from 'node:child_process'; import { debugLogger } from './debugLogger.js'; @@ -550,132 +548,6 @@ describe('editor utils', () => { }); }); - describe('detectFirstAvailableEditor', () => { - it('should return undefined when no editors are installed', () => { - (execSync as Mock).mockImplementation(() => { - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - expect(detectFirstAvailableEditor()).toBeUndefined(); - }); - - it('should prioritize terminal editors over GUI editors', () => { - // Mock vim as available - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('vim') && !cmd.includes('nvim')) { - return Buffer.from('/usr/bin/vim'); - } - if (cmd.includes('code')) { - return Buffer.from('/usr/bin/code'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - expect(detectFirstAvailableEditor()).toBe('vim'); - }); - - it('should return vim when vim is the only editor available in sandbox mode', () => { - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('vim') && !cmd.includes('nvim')) { - return Buffer.from('/usr/bin/vim'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', 'sandbox'); - expect(detectFirstAvailableEditor()).toBe('vim'); - }); - - it('should skip GUI editors in sandbox mode', () => { - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('code')) { - return Buffer.from('/usr/bin/code'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', 'sandbox'); - // vscode is installed but not allowed in sandbox - expect(detectFirstAvailableEditor()).toBeUndefined(); - }); - - it('should return first available terminal editor (neovim)', () => { - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('nvim')) { - return Buffer.from('/usr/bin/nvim'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - expect(detectFirstAvailableEditor()).toBe('neovim'); - }); - }); - - describe('resolveEditor', () => { - it('should return the preferred editor when available', () => { - (execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/vim')); - vi.stubEnv('SANDBOX', ''); - const result = resolveEditor('vim'); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); - }); - - it('should return error when preferred editor is not installed', () => { - (execSync as Mock).mockImplementation(() => { - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - const result = resolveEditor('vim'); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('Vim'); - expect(result.error).toContain('not installed'); - }); - - it('should return error when preferred GUI editor cannot be used in sandbox mode', () => { - (execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code')); - vi.stubEnv('SANDBOX', 'sandbox'); - const result = resolveEditor('vscode'); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('VS Code'); - expect(result.error).toContain('sandbox mode'); - }); - - it('should auto-detect editor when no preference is set', () => { - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('vim') && !cmd.includes('nvim')) { - return Buffer.from('/usr/bin/vim'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - const result = resolveEditor(undefined); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); - }); - - it('should return error when no preference is set and no editors are available', () => { - (execSync as Mock).mockImplementation(() => { - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', ''); - const result = resolveEditor(undefined); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('No external editor'); - expect(result.error).toContain('/editor'); - }); - - it('should work with terminal editors in sandbox mode when no preference is set', () => { - (execSync as Mock).mockImplementation((cmd: string) => { - if (cmd.includes('vim') && !cmd.includes('nvim')) { - return Buffer.from('/usr/bin/vim'); - } - throw new Error('Command not found'); - }); - vi.stubEnv('SANDBOX', 'sandbox'); - const result = resolveEditor(undefined); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); - }); - }); - // Helper to create a mock exec that simulates async behavior const mockExecAsync = (implementation: (cmd: string) => boolean): void => { (exec as unknown as Mock).mockImplementation( @@ -749,86 +621,93 @@ describe('editor utils', () => { }); }); - describe('detectFirstAvailableEditorAsync', () => { - it('should return undefined when no editors are installed', async () => { - mockExecAsync(() => false); - vi.stubEnv('SANDBOX', ''); - expect(await detectFirstAvailableEditorAsync()).toBeUndefined(); - }); - - it('should prioritize terminal editors over GUI editors', async () => { - mockExecAsync( - (cmd) => - (cmd.includes('vim') && !cmd.includes('nvim')) || - cmd.includes('code'), - ); - vi.stubEnv('SANDBOX', ''); - expect(await detectFirstAvailableEditorAsync()).toBe('vim'); - }); - - it('should return vim in sandbox mode', async () => { - mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); - vi.stubEnv('SANDBOX', 'sandbox'); - expect(await detectFirstAvailableEditorAsync()).toBe('vim'); - }); - - it('should skip GUI editors in sandbox mode', async () => { - mockExecAsync((cmd) => cmd.includes('code')); - vi.stubEnv('SANDBOX', 'sandbox'); - expect(await detectFirstAvailableEditorAsync()).toBeUndefined(); - }); - }); - describe('resolveEditorAsync', () => { it('should return the preferred editor when available', async () => { mockExecAsync((cmd) => cmd.includes('vim')); vi.stubEnv('SANDBOX', ''); const result = await resolveEditorAsync('vim'); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); + expect(result).toBe('vim'); }); - it('should return error when preferred editor is not installed', async () => { + it('should request editor selection when preferred editor is not installed', async () => { mockExecAsync(() => false); vi.stubEnv('SANDBOX', ''); - const result = await resolveEditorAsync('vim'); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('Vim'); - expect(result.error).toContain('not installed'); + const resolvePromise = resolveEditorAsync('vim'); + setTimeout( + () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'neovim' }), + 0, + ); + const result = await resolvePromise; + expect(result).toBe('neovim'); }); - it('should return error when preferred GUI editor cannot be used in sandbox mode', async () => { + it('should request editor selection when preferred GUI editor cannot be used in sandbox mode', async () => { mockExecAsync((cmd) => cmd.includes('code')); vi.stubEnv('SANDBOX', 'sandbox'); - const result = await resolveEditorAsync('vscode'); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('VS Code'); - expect(result.error).toContain('sandbox mode'); + const resolvePromise = resolveEditorAsync('vscode'); + setTimeout( + () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'vim' }), + 0, + ); + const result = await resolvePromise; + expect(result).toBe('vim'); }); - it('should auto-detect editor when no preference is set', async () => { - mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); + it('should request editor selection when no preference is set', async () => { + const emitSpy = vi.spyOn(coreEvents, 'emit'); vi.stubEnv('SANDBOX', ''); - const result = await resolveEditorAsync(undefined); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); + + const resolvePromise = resolveEditorAsync(undefined); + + // Simulate UI selection + setTimeout( + () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'vim' }), + 0, + ); + + const result = await resolvePromise; + expect(result).toBe('vim'); + expect(emitSpy).toHaveBeenCalledWith(CoreEvent.RequestEditorSelection); }); - it('should return error when no preference is set and no editors are available', async () => { - mockExecAsync(() => false); - vi.stubEnv('SANDBOX', ''); - const result = await resolveEditorAsync(undefined); - expect(result.editor).toBeUndefined(); - expect(result.error).toContain('No external editor'); - expect(result.error).toContain('/editor'); + it('should return undefined when editor selection is cancelled', async () => { + const resolvePromise = resolveEditorAsync(undefined); + + // Simulate UI cancellation (exit dialog) + setTimeout( + () => coreEvents.emit(CoreEvent.EditorSelected, { editor: undefined }), + 0, + ); + + const result = await resolvePromise; + expect(result).toBeUndefined(); }); - it('should work with terminal editors in sandbox mode when no preference is set', async () => { - mockExecAsync((cmd) => cmd.includes('vim') && !cmd.includes('nvim')); + it('should return undefined when abort signal is triggered', async () => { + const controller = new AbortController(); + const resolvePromise = resolveEditorAsync(undefined, controller.signal); + + setTimeout(() => controller.abort(), 0); + + const result = await resolvePromise; + expect(result).toBeUndefined(); + }); + + it('should request editor selection in sandbox mode when no preference is set', async () => { + const emitSpy = vi.spyOn(coreEvents, 'emit'); vi.stubEnv('SANDBOX', 'sandbox'); - const result = await resolveEditorAsync(undefined); - expect(result.editor).toBe('vim'); - expect(result.error).toBeUndefined(); + + const resolvePromise = resolveEditorAsync(undefined); + + // Simulate UI selection + setTimeout( + () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'vim' }), + 0, + ); + + const result = await resolvePromise; + expect(result).toBe('vim'); + expect(emitSpy).toHaveBeenCalledWith(CoreEvent.RequestEditorSelection); }); }); }); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index deafca2c3c9..db6611ed6f3 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -6,8 +6,9 @@ import { exec, execSync, spawn, spawnSync } from 'node:child_process'; import { promisify } from 'node:util'; +import { once } from 'node:events'; import { debugLogger } from './debugLogger.js'; -import { coreEvents, CoreEvent } from './events.js'; +import { coreEvents, CoreEvent, type EditorSelectedPayload } from './events.js'; const GUI_EDITORS = [ 'vscode', @@ -123,20 +124,21 @@ const editorCommands: Record< hx: { win32: ['hx'], default: ['hx'] }, }; -export function checkHasEditorType(editor: EditorType): boolean { +function getEditorCommands(editor: EditorType): string[] { const commandConfig = editorCommands[editor]; - const commands = - process.platform === 'win32' ? commandConfig.win32 : commandConfig.default; - return commands.some((cmd) => commandExists(cmd)); + return process.platform === 'win32' + ? commandConfig.win32 + : commandConfig.default; +} + +export function checkHasEditorType(editor: EditorType): boolean { + return getEditorCommands(editor).some((cmd) => commandExists(cmd)); } export async function checkHasEditorTypeAsync( editor: EditorType, ): Promise { - const commandConfig = editorCommands[editor]; - const commands = - process.platform === 'win32' ? commandConfig.win32 : commandConfig.default; - for (const cmd of commands) { + for (const cmd of getEditorCommands(editor)) { if (await commandExistsAsync(cmd)) { return true; } @@ -145,9 +147,7 @@ export async function checkHasEditorTypeAsync( } export function getEditorCommand(editor: EditorType): string { - const commandConfig = editorCommands[editor]; - const commands = - process.platform === 'win32' ? commandConfig.win32 : commandConfig.default; + const commands = getEditorCommands(editor); return ( commands.slice(0, -1).find((cmd) => commandExists(cmd)) || commands[commands.length - 1] @@ -163,15 +163,20 @@ export function allowEditorTypeInSandbox(editor: EditorType): boolean { return true; } +function isEditorTypeAvailable( + editor: string | undefined, +): editor is EditorType { + return ( + !!editor && isValidEditorType(editor) && allowEditorTypeInSandbox(editor) + ); +} + /** * Check if the editor is valid and can be used. * Returns false if preferred editor is not set / invalid / not available / not allowed in sandbox. */ export function isEditorAvailable(editor: string | undefined): boolean { - if (editor && isValidEditorType(editor)) { - return checkHasEditorType(editor) && allowEditorTypeInSandbox(editor); - } - return false; + return isEditorTypeAvailable(editor) && checkHasEditorType(editor); } /** @@ -182,155 +187,29 @@ export function isEditorAvailable(editor: string | undefined): boolean { export async function isEditorAvailableAsync( editor: string | undefined, ): Promise { - if (editor && isValidEditorType(editor)) { - return ( - (await checkHasEditorTypeAsync(editor)) && - allowEditorTypeInSandbox(editor) - ); - } - return false; -} - -/** - * Detects the first available editor from the supported list. - * Prioritizes terminal editors (vim, neovim, emacs, hx) as they work in all environments - * including sandboxed mode, then falls back to GUI editors. - * Returns undefined if no supported editor is found. - */ -export function detectFirstAvailableEditor(): EditorType | undefined { - // Prioritize terminal editors as they work in sandbox mode - for (const editor of TERMINAL_EDITORS) { - if (isEditorAvailable(editor)) { - return editor; - } - } - // Fall back to GUI editors (won't work in sandbox mode but checked above) - for (const editor of GUI_EDITORS) { - if (isEditorAvailable(editor)) { - return editor; - } - } - return undefined; -} - -/** - * Async version of detectFirstAvailableEditor. - * Detects the first available editor from the supported list without blocking the event loop. - * Prioritizes terminal editors (vim, neovim, emacs, hx) as they work in all environments - * including sandboxed mode, then falls back to GUI editors. - * Returns undefined if no supported editor is found. - */ -export async function detectFirstAvailableEditorAsync(): Promise< - EditorType | undefined -> { - // Prioritize terminal editors as they work in sandbox mode - for (const editor of TERMINAL_EDITORS) { - if (await isEditorAvailableAsync(editor)) { - return editor; - } - } - // Fall back to GUI editors (won't work in sandbox mode but checked above) - for (const editor of GUI_EDITORS) { - if (await isEditorAvailableAsync(editor)) { - return editor; - } - } - return undefined; -} - -/** - * Result of attempting to resolve an editor for use. - */ -export interface EditorResolutionResult { - /** The editor to use, if available */ - editor?: EditorType; - /** Error message if no editor is available */ - error?: string; -} - -/** - * Resolves an editor to use for external editing. - * 1. If a preferred editor is set and available, uses it. - * 2. If a preferred editor is set but not available, returns an error. - * 3. If no preferred editor is set, attempts to auto-detect an available editor. - * 4. If no editor can be found, returns an error with instructions. - * - * @deprecated Use resolveEditorAsync instead to avoid blocking the event loop. - */ -export function resolveEditor( - preferredEditor: EditorType | undefined, -): EditorResolutionResult { - // Case 1: Preferred editor is set - if (preferredEditor) { - if (isEditorAvailable(preferredEditor)) { - return { editor: preferredEditor }; - } - // Preferred editor is set but not available - const displayName = getEditorDisplayName(preferredEditor); - if (!checkHasEditorType(preferredEditor)) { - return { - error: `${displayName} is configured as your preferred editor but is not installed. Please install it or run /editor to choose a different editor.`, - }; - } - // If the editor is installed but not available, it must be due to sandbox restrictions. - return { - error: `${displayName} cannot be used in sandbox mode. Please run /editor to choose a terminal-based editor (vim, neovim, emacs, or helix).`, - }; - } - - // Case 2: No preferred editor set, try to auto-detect - const detectedEditor = detectFirstAvailableEditor(); - if (detectedEditor) { - return { editor: detectedEditor }; - } - - // Case 3: No editor available at all - return { - error: - 'No external editor is configured or available. Please run /editor to set your preferred editor, or install one of the supported editors: vim, neovim, emacs, helix, VS Code, Cursor, Zed, or Windsurf.', - }; + return ( + isEditorTypeAvailable(editor) && (await checkHasEditorTypeAsync(editor)) + ); } /** - * Async version of resolveEditor. * Resolves an editor to use for external editing without blocking the event loop. * 1. If a preferred editor is set and available, uses it. - * 2. If a preferred editor is set but not available, returns an error. - * 3. If no preferred editor is set, attempts to auto-detect an available editor. - * 4. If no editor can be found, returns an error with instructions. + * 2. If no preferred editor is set (or preferred is unavailable), requests selection from user and waits for it. */ export async function resolveEditorAsync( preferredEditor: EditorType | undefined, -): Promise { - // Case 1: Preferred editor is set - if (preferredEditor) { - if (await isEditorAvailableAsync(preferredEditor)) { - return { editor: preferredEditor }; - } - // Preferred editor is set but not available - const displayName = getEditorDisplayName(preferredEditor); - if (!(await checkHasEditorTypeAsync(preferredEditor))) { - return { - error: `${displayName} is configured as your preferred editor but is not installed. Please install it or run /editor to choose a different editor.`, - }; - } - // If the editor is installed but not available, it must be due to sandbox restrictions. - return { - error: `${displayName} cannot be used in sandbox mode. Please run /editor to choose a terminal-based editor (vim, neovim, emacs, or helix).`, - }; + signal?: AbortSignal, +): Promise { + if (preferredEditor && (await isEditorAvailableAsync(preferredEditor))) { + return preferredEditor; } - // Case 2: No preferred editor set, try to auto-detect - const detectedEditor = await detectFirstAvailableEditorAsync(); - if (detectedEditor) { - return { editor: detectedEditor }; - } + coreEvents.emit(CoreEvent.RequestEditorSelection); - // Case 3: No editor available at all - return { - error: - 'No external editor is configured or available. Please run /editor to set your preferred editor, or install one of the supported editors: vim, neovim, emacs, helix, VS Code, Cursor, Zed, or Windsurf.', - }; + return once(coreEvents, CoreEvent.EditorSelected, { signal }) + .then(([payload]) => (payload as EditorSelectedPayload).editor) + .catch(() => undefined); } /** diff --git a/packages/core/src/utils/events.ts b/packages/core/src/utils/events.ts index cea80952f98..33d137980a2 100644 --- a/packages/core/src/utils/events.ts +++ b/packages/core/src/utils/events.ts @@ -8,6 +8,7 @@ import { EventEmitter } from 'node:events'; import type { AgentDefinition } from '../agents/types.js'; import type { McpClient } from '../tools/mcp-client.js'; import type { ExtensionEvents } from './extensionLoader.js'; +import type { EditorType } from './editor.js'; /** * Defines the severity level for user-facing feedback. @@ -143,6 +144,15 @@ export enum CoreEvent { RetryAttempt = 'retry-attempt', ConsentRequest = 'consent-request', AgentsDiscovered = 'agents-discovered', + RequestEditorSelection = 'request-editor-selection', + EditorSelected = 'editor-selected', +} + +/** + * Payload for the 'editor-selected' event. + */ +export interface EditorSelectedPayload { + editor?: EditorType; } export interface CoreEvents extends ExtensionEvents { @@ -162,6 +172,8 @@ export interface CoreEvents extends ExtensionEvents { [CoreEvent.RetryAttempt]: [RetryAttemptPayload]; [CoreEvent.ConsentRequest]: [ConsentRequestPayload]; [CoreEvent.AgentsDiscovered]: [AgentsDiscoveredPayload]; + [CoreEvent.RequestEditorSelection]: never[]; + [CoreEvent.EditorSelected]: [EditorSelectedPayload]; } type EventBacklogItem = { From 8fd65b58c0fccf08f10098e3dceffff290d22074 Mon Sep 17 00:00:00 2001 From: ehedlund Date: Wed, 4 Feb 2026 14:12:01 -0500 Subject: [PATCH 06/10] Move error message to constant. --- packages/core/src/core/coreToolScheduler.ts | 11 ++++++----- packages/core/src/scheduler/confirmation.ts | 9 ++++++--- packages/core/src/utils/editor.ts | 3 +++ 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 79d5f132a6d..00bd2924ee6 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -12,7 +12,11 @@ import { type ToolConfirmationPayload, ToolConfirmationOutcome, } from '../tools/tools.js'; -import { resolveEditorAsync, type EditorType } from '../utils/editor.js'; +import { + resolveEditorAsync, + type EditorType, + NO_EDITOR_AVAILABLE_ERROR, +} from '../utils/editor.js'; import { coreEvents } from '../utils/events.js'; import type { Config } from '../config/config.js'; import { PolicyDecision } from '../policy/types.js'; @@ -757,10 +761,7 @@ export class CoreToolScheduler { if (!editor) { // No editor available - emit error feedback and return to previous confirmation screen - coreEvents.emitFeedback( - 'error', - 'No external editor is available. Please run /editor to configure one.', - ); + coreEvents.emitFeedback('error', NO_EDITOR_AVAILABLE_ERROR); this.setStatusInternal(callId, 'awaiting_approval', signal, { ...waitingToolCall.confirmationDetails, isModifying: false, diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index dbd17c6312e..2d0dedea0f3 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -21,7 +21,11 @@ import type { ValidatingToolCall, WaitingToolCall } from './types.js'; import type { Config } from '../config/config.js'; import type { SchedulerStateManager } from './state-manager.js'; import type { ToolModificationHandler } from './tool-modifier.js'; -import { resolveEditorAsync, type EditorType } from '../utils/editor.js'; +import { + resolveEditorAsync, + type EditorType, + NO_EDITOR_AVAILABLE_ERROR, +} from '../utils/editor.js'; import type { DiffUpdateResult } from '../ide/ide-client.js'; import { debugLogger } from '../utils/debugLogger.js'; import { coreEvents } from '../utils/events.js'; @@ -226,8 +230,7 @@ async function handleExternalModification( // No editor available - return failure with error message return { success: false, - error: - 'No external editor is available. Please run /editor to configure one.', + error: NO_EDITOR_AVAILABLE_ERROR, }; } diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index db6611ed6f3..89e08e01939 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -25,6 +25,9 @@ const GUI_EDITORS_SET = new Set(GUI_EDITORS); const TERMINAL_EDITORS_SET = new Set(TERMINAL_EDITORS); const EDITORS_SET = new Set(EDITORS); +export const NO_EDITOR_AVAILABLE_ERROR = + 'No external editor is available. Please run /editor to configure one.'; + export const DEFAULT_GUI_EDITOR: GuiEditorType = 'vscode'; export type GuiEditorType = (typeof GUI_EDITORS)[number]; From 819a08d8a5eafe16a6bdd0d0b365f7f992df0e23 Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Wed, 4 Feb 2026 20:48:31 +0100 Subject: [PATCH 07/10] test(core): mock resolveEditorAsync in modifyWithEditor test The test was timing out because resolveEditorAsync waits for an EditorSelected event when no editor is available. Mock the function to return 'vscode' directly. --- packages/core/src/core/coreToolScheduler.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 6a5e3524a00..02fc6af2351 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -41,6 +41,7 @@ import { MOCK_TOOL_SHOULD_CONFIRM_EXECUTE, } from '../test-utils/mock-tool.js'; import * as modifiableToolModule from '../tools/modifiable-tool.js'; +import * as editorModule from '../utils/editor.js'; import { DEFAULT_GEMINI_MODEL } from '../config/models.js'; import type { PolicyEngine } from '../policy/policy-engine.js'; import { DiscoveredMCPTool } from '../tools/mcp-tool.js'; @@ -1762,6 +1763,10 @@ describe('CoreToolScheduler Sequential Execution', () => { }); it('should pass confirmation diff data into modifyWithEditor overrides', async () => { + const resolveEditorAsyncSpy = vi + .spyOn(editorModule, 'resolveEditorAsync') + .mockResolvedValue('vscode'); + const modifyWithEditorSpy = vi .spyOn(modifiableToolModule, 'modifyWithEditor') .mockResolvedValue({ @@ -1842,6 +1847,7 @@ describe('CoreToolScheduler Sequential Execution', () => { }); modifyWithEditorSpy.mockRestore(); + resolveEditorAsyncSpy.mockRestore(); }); it('should handle inline modify with empty new content', async () => { From 2dcc4539f7bd4f1ddcf23649d1025c4b8e1d1981 Mon Sep 17 00:00:00 2001 From: ehedlund Date: Wed, 4 Feb 2026 16:15:34 -0500 Subject: [PATCH 08/10] Revert changes to legacy file coreToolScheduler. --- .../core/src/core/coreToolScheduler.test.ts | 6 ------ packages/core/src/core/coreToolScheduler.ts | 21 ++++--------------- 2 files changed, 4 insertions(+), 23 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 02fc6af2351..6a5e3524a00 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -41,7 +41,6 @@ import { MOCK_TOOL_SHOULD_CONFIRM_EXECUTE, } from '../test-utils/mock-tool.js'; import * as modifiableToolModule from '../tools/modifiable-tool.js'; -import * as editorModule from '../utils/editor.js'; import { DEFAULT_GEMINI_MODEL } from '../config/models.js'; import type { PolicyEngine } from '../policy/policy-engine.js'; import { DiscoveredMCPTool } from '../tools/mcp-tool.js'; @@ -1763,10 +1762,6 @@ describe('CoreToolScheduler Sequential Execution', () => { }); it('should pass confirmation diff data into modifyWithEditor overrides', async () => { - const resolveEditorAsyncSpy = vi - .spyOn(editorModule, 'resolveEditorAsync') - .mockResolvedValue('vscode'); - const modifyWithEditorSpy = vi .spyOn(modifiableToolModule, 'modifyWithEditor') .mockResolvedValue({ @@ -1847,7 +1842,6 @@ describe('CoreToolScheduler Sequential Execution', () => { }); modifyWithEditorSpy.mockRestore(); - resolveEditorAsyncSpy.mockRestore(); }); it('should handle inline modify with empty new content', async () => { diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 00bd2924ee6..96cb05d9707 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -12,12 +12,7 @@ import { type ToolConfirmationPayload, ToolConfirmationOutcome, } from '../tools/tools.js'; -import { - resolveEditorAsync, - type EditorType, - NO_EDITOR_AVAILABLE_ERROR, -} from '../utils/editor.js'; -import { coreEvents } from '../utils/events.js'; +import type { EditorType } from '../utils/editor.js'; import type { Config } from '../config/config.js'; import { PolicyDecision } from '../policy/types.js'; import { logToolCall } from '../telemetry/loggers.js'; @@ -756,16 +751,8 @@ export class CoreToolScheduler { } else if (outcome === ToolConfirmationOutcome.ModifyWithEditor) { const waitingToolCall = toolCall as WaitingToolCall; - const preferredEditor = this.getPreferredEditor(); - const editor = await resolveEditorAsync(preferredEditor, signal); - - if (!editor) { - // No editor available - emit error feedback and return to previous confirmation screen - coreEvents.emitFeedback('error', NO_EDITOR_AVAILABLE_ERROR); - this.setStatusInternal(callId, 'awaiting_approval', signal, { - ...waitingToolCall.confirmationDetails, - isModifying: false, - } as ToolCallConfirmationDetails); + const editorType = this.getPreferredEditor(); + if (!editorType) { return; } @@ -776,7 +763,7 @@ export class CoreToolScheduler { const result = await this.toolModifier.handleModifyWithEditor( waitingToolCall, - editor, + editorType, signal, ); From 4db4cf9922f0c781db5c0f5b16366f3384d2403f Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Thu, 5 Feb 2026 20:29:45 +0100 Subject: [PATCH 09/10] refactor(core): address PR review feedback - Remove redundant `success` field from ExternalModificationResult, check for error presence instead - Rename checkHasEditorType to hasValidEditorCommand for clarity - Run command existence checks in parallel in hasValidEditorCommandAsync - Simplify JSDoc comment for isEditorAvailableAsync --- .../src/ui/editors/editorSettingsManager.ts | 4 +-- .../src/ui/hooks/useEditorSettings.test.tsx | 6 ++--- .../cli/src/ui/hooks/useEditorSettings.ts | 4 +-- packages/core/src/scheduler/confirmation.ts | 12 +++------ packages/core/src/utils/editor.test.ts | 26 +++++++++---------- packages/core/src/utils/editor.ts | 21 +++++++-------- 6 files changed, 33 insertions(+), 40 deletions(-) diff --git a/packages/cli/src/ui/editors/editorSettingsManager.ts b/packages/cli/src/ui/editors/editorSettingsManager.ts index 5a9b2e3147d..6869cd7f8e8 100644 --- a/packages/cli/src/ui/editors/editorSettingsManager.ts +++ b/packages/cli/src/ui/editors/editorSettingsManager.ts @@ -6,7 +6,7 @@ import { allowEditorTypeInSandbox, - checkHasEditorType, + hasValidEditorCommand, type EditorType, EDITOR_DISPLAY_NAMES, } from '@google/gemini-cli-core'; @@ -31,7 +31,7 @@ class EditorSettingsManager { disabled: false, }, ...editorTypes.map((type) => { - const hasEditor = checkHasEditorType(type); + const hasEditor = hasValidEditorCommand(type); const isAllowedInSandbox = allowEditorTypeInSandbox(type); let labelSuffix = !isAllowedInSandbox diff --git a/packages/cli/src/ui/hooks/useEditorSettings.test.tsx b/packages/cli/src/ui/hooks/useEditorSettings.test.tsx index 2b39fae02cf..db78a9a72c1 100644 --- a/packages/cli/src/ui/hooks/useEditorSettings.test.tsx +++ b/packages/cli/src/ui/hooks/useEditorSettings.test.tsx @@ -24,7 +24,7 @@ import { SettingScope } from '../../config/settings.js'; import { MessageType } from '../types.js'; import { type EditorType, - checkHasEditorType, + hasValidEditorCommand, allowEditorTypeInSandbox, } from '@google/gemini-cli-core'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; @@ -35,12 +35,12 @@ vi.mock('@google/gemini-cli-core', async () => { const actual = await vi.importActual('@google/gemini-cli-core'); return { ...actual, - checkHasEditorType: vi.fn(() => true), + hasValidEditorCommand: vi.fn(() => true), allowEditorTypeInSandbox: vi.fn(() => true), }; }); -const mockCheckHasEditorType = vi.mocked(checkHasEditorType); +const mockCheckHasEditorType = vi.mocked(hasValidEditorCommand); const mockAllowEditorTypeInSandbox = vi.mocked(allowEditorTypeInSandbox); describe('useEditorSettings', () => { diff --git a/packages/cli/src/ui/hooks/useEditorSettings.ts b/packages/cli/src/ui/hooks/useEditorSettings.ts index a855b0b4052..0a432e303b4 100644 --- a/packages/cli/src/ui/hooks/useEditorSettings.ts +++ b/packages/cli/src/ui/hooks/useEditorSettings.ts @@ -13,7 +13,7 @@ import { MessageType } from '../types.js'; import type { EditorType } from '@google/gemini-cli-core'; import { allowEditorTypeInSandbox, - checkHasEditorType, + hasValidEditorCommand, getEditorDisplayName, coreEvents, CoreEvent, @@ -47,7 +47,7 @@ export const useEditorSettings = ( (editorType: EditorType | undefined, scope: LoadableSettingScope) => { if ( editorType && - (!checkHasEditorType(editorType) || + (!hasValidEditorCommand(editorType) || !allowEditorTypeInSandbox(editorType)) ) { return; diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index 2d0dedea0f3..ba77aaaffec 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -165,7 +165,7 @@ export async function resolveConfirmation( toolCall, signal, ); - if (!modResult.success) { + if (modResult.error) { // Editor is not available - emit error feedback and stay in the loop // to return to previous confirmation screen. if (modResult.error) { @@ -200,10 +200,9 @@ async function notifyHooks( /** * Result of attempting external modification. + * If error is defined, the modification failed. */ interface ExternalModificationResult { - /** Whether the modification was successful (editor was opened) */ - success: boolean; /** Error message if the modification failed */ error?: string; } @@ -228,10 +227,7 @@ async function handleExternalModification( if (!editor) { // No editor available - return failure with error message - return { - success: false, - error: NO_EDITOR_AVAILABLE_ERROR, - }; + return { error: NO_EDITOR_AVAILABLE_ERROR }; } const result = await modifier.handleModifyWithEditor( @@ -247,7 +243,7 @@ async function handleExternalModification( newInvocation, ); } - return { success: true }; + return {}; } /** diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index 7f31ca1c12b..d46c58d6771 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -14,8 +14,8 @@ import { type Mock, } from 'vitest'; import { - checkHasEditorType, - checkHasEditorTypeAsync, + hasValidEditorCommand, + hasValidEditorCommandAsync, getDiffCommand, openDiff, allowEditorTypeInSandbox, @@ -56,7 +56,7 @@ describe('editor utils', () => { }); }); - describe('checkHasEditorType', () => { + describe('hasValidEditorCommand', () => { const testCases: Array<{ editor: EditorType; commands: string[]; @@ -94,7 +94,7 @@ describe('editor utils', () => { (execSync as Mock).mockReturnValue( Buffer.from(`/usr/bin/${commands[0]}`), ); - expect(checkHasEditorType(editor)).toBe(true); + expect(hasValidEditorCommand(editor)).toBe(true); expect(execSync).toHaveBeenCalledWith(`command -v ${commands[0]}`, { stdio: 'ignore', }); @@ -108,7 +108,7 @@ describe('editor utils', () => { throw new Error(); // first command not found }) .mockReturnValueOnce(Buffer.from(`/usr/bin/${commands[1]}`)); // second command found - expect(checkHasEditorType(editor)).toBe(true); + expect(hasValidEditorCommand(editor)).toBe(true); expect(execSync).toHaveBeenCalledTimes(2); }); } @@ -118,7 +118,7 @@ describe('editor utils', () => { (execSync as Mock).mockImplementation(() => { throw new Error(); // all commands not found }); - expect(checkHasEditorType(editor)).toBe(false); + expect(hasValidEditorCommand(editor)).toBe(false); expect(execSync).toHaveBeenCalledTimes(commands.length); }); @@ -128,7 +128,7 @@ describe('editor utils', () => { (execSync as Mock).mockReturnValue( Buffer.from(`C:\\Program Files\\...\\${win32Commands[0]}`), ); - expect(checkHasEditorType(editor)).toBe(true); + expect(hasValidEditorCommand(editor)).toBe(true); expect(execSync).toHaveBeenCalledWith( `where.exe ${win32Commands[0]}`, { @@ -147,7 +147,7 @@ describe('editor utils', () => { .mockReturnValueOnce( Buffer.from(`C:\\Program Files\\...\\${win32Commands[1]}`), ); // second command found - expect(checkHasEditorType(editor)).toBe(true); + expect(hasValidEditorCommand(editor)).toBe(true); expect(execSync).toHaveBeenCalledTimes(2); }); } @@ -157,7 +157,7 @@ describe('editor utils', () => { (execSync as Mock).mockImplementation(() => { throw new Error(); // all commands not found }); - expect(checkHasEditorType(editor)).toBe(false); + expect(hasValidEditorCommand(editor)).toBe(false); expect(execSync).toHaveBeenCalledTimes(win32Commands.length); }); }); @@ -564,23 +564,23 @@ describe('editor utils', () => { ); }; - describe('checkHasEditorTypeAsync', () => { + describe('hasValidEditorCommandAsync', () => { it('should return true if vim command exists', async () => { Object.defineProperty(process, 'platform', { value: 'linux' }); mockExecAsync((cmd) => cmd.includes('vim')); - expect(await checkHasEditorTypeAsync('vim')).toBe(true); + expect(await hasValidEditorCommandAsync('vim')).toBe(true); }); it('should return false if vim command does not exist', async () => { Object.defineProperty(process, 'platform', { value: 'linux' }); mockExecAsync(() => false); - expect(await checkHasEditorTypeAsync('vim')).toBe(false); + expect(await hasValidEditorCommandAsync('vim')).toBe(false); }); it('should check zed and zeditor commands in order', async () => { Object.defineProperty(process, 'platform', { value: 'linux' }); mockExecAsync((cmd) => cmd.includes('zeditor')); - expect(await checkHasEditorTypeAsync('zed')).toBe(true); + expect(await hasValidEditorCommandAsync('zed')).toBe(true); }); }); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 89e08e01939..8bc808c31fd 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -134,19 +134,17 @@ function getEditorCommands(editor: EditorType): string[] { : commandConfig.default; } -export function checkHasEditorType(editor: EditorType): boolean { +export function hasValidEditorCommand(editor: EditorType): boolean { return getEditorCommands(editor).some((cmd) => commandExists(cmd)); } -export async function checkHasEditorTypeAsync( +export async function hasValidEditorCommandAsync( editor: EditorType, ): Promise { - for (const cmd of getEditorCommands(editor)) { - if (await commandExistsAsync(cmd)) { - return true; - } - } - return false; + const results = await Promise.all( + getEditorCommands(editor).map((cmd) => commandExistsAsync(cmd)), + ); + return results.some(Boolean); } export function getEditorCommand(editor: EditorType): string { @@ -179,19 +177,18 @@ function isEditorTypeAvailable( * Returns false if preferred editor is not set / invalid / not available / not allowed in sandbox. */ export function isEditorAvailable(editor: string | undefined): boolean { - return isEditorTypeAvailable(editor) && checkHasEditorType(editor); + return isEditorTypeAvailable(editor) && hasValidEditorCommand(editor); } /** - * Async version of isEditorAvailable. - * Check if the editor is valid and can be used without blocking the event loop. + * Check if the editor is valid and can be used. * Returns false if preferred editor is not set / invalid / not available / not allowed in sandbox. */ export async function isEditorAvailableAsync( editor: string | undefined, ): Promise { return ( - isEditorTypeAvailable(editor) && (await checkHasEditorTypeAsync(editor)) + isEditorTypeAvailable(editor) && (await hasValidEditorCommandAsync(editor)) ); } From 4e648eb9aa963098659f999508bad57e73743e9c Mon Sep 17 00:00:00 2001 From: ehedlund Date: Thu, 5 Feb 2026 14:48:53 -0500 Subject: [PATCH 10/10] - Rename `mockCheckHasEditorType` -> `mockHasValidEditorCommand` - Remove redundant `if (modResult.error)` check - Update parallel implementation of `hasValidEditorCommandAsync` to short-circuit return true as soon as the first command is found to exist --- packages/cli/src/ui/hooks/useEditorSettings.test.tsx | 6 +++--- packages/core/src/scheduler/confirmation.ts | 8 +++----- packages/core/src/utils/editor.ts | 9 +++++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/ui/hooks/useEditorSettings.test.tsx b/packages/cli/src/ui/hooks/useEditorSettings.test.tsx index db78a9a72c1..68c2b93f22c 100644 --- a/packages/cli/src/ui/hooks/useEditorSettings.test.tsx +++ b/packages/cli/src/ui/hooks/useEditorSettings.test.tsx @@ -40,7 +40,7 @@ vi.mock('@google/gemini-cli-core', async () => { }; }); -const mockCheckHasEditorType = vi.mocked(hasValidEditorCommand); +const mockHasValidEditorCommand = vi.mocked(hasValidEditorCommand); const mockAllowEditorTypeInSandbox = vi.mocked(allowEditorTypeInSandbox); describe('useEditorSettings', () => { @@ -69,7 +69,7 @@ describe('useEditorSettings', () => { mockAddItem = vi.fn(); // Reset mock implementations to default - mockCheckHasEditorType.mockReturnValue(true); + mockHasValidEditorCommand.mockReturnValue(true); mockAllowEditorTypeInSandbox.mockReturnValue(true); }); @@ -224,7 +224,7 @@ describe('useEditorSettings', () => { it('should not set preference for unavailable editors', () => { render(); - mockCheckHasEditorType.mockReturnValue(false); + mockHasValidEditorCommand.mockReturnValue(false); const editorType: EditorType = 'vscode'; const scope = SettingScope.User; diff --git a/packages/core/src/scheduler/confirmation.ts b/packages/core/src/scheduler/confirmation.ts index ba77aaaffec..4fba731cfb2 100644 --- a/packages/core/src/scheduler/confirmation.ts +++ b/packages/core/src/scheduler/confirmation.ts @@ -165,12 +165,10 @@ export async function resolveConfirmation( toolCall, signal, ); + // Editor is not available - emit error feedback and stay in the loop + // to return to previous confirmation screen. if (modResult.error) { - // Editor is not available - emit error feedback and stay in the loop - // to return to previous confirmation screen. - if (modResult.error) { - coreEvents.emitFeedback('error', modResult.error); - } + coreEvents.emitFeedback('error', modResult.error); } } else if (response.payload && 'newContent' in response.payload) { await handleInlineModification(deps, toolCall, response.payload, signal); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 8bc808c31fd..08cb359a498 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -141,10 +141,11 @@ export function hasValidEditorCommand(editor: EditorType): boolean { export async function hasValidEditorCommandAsync( editor: EditorType, ): Promise { - const results = await Promise.all( - getEditorCommands(editor).map((cmd) => commandExistsAsync(cmd)), - ); - return results.some(Boolean); + return Promise.any( + getEditorCommands(editor).map((cmd) => + commandExistsAsync(cmd).then((exists) => exists || Promise.reject()), + ), + ).catch(() => false); } export function getEditorCommand(editor: EditorType): string {