From c904f4cecec4f0416c7625017d5c0b6cef2b11d2 Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Thu, 22 Jan 2026 21:15:02 +0100 Subject: [PATCH] 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 585d7b9bf6..4374018ab5 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 f8d5f6b6b4..d71b9411d4 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 b4d33c5377..a3ca385e7b 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 e48a055d40..29dde6de9b 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. */