From a526c7619184b1841199997822d93dc6d9da056b Mon Sep 17 00:00:00 2001 From: Philippe Granger Date: Sat, 24 Jan 2026 19:58:50 +0100 Subject: [PATCH] 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 4374018ab5..60bbeac90e 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 d71b9411d4..e146ac640d 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 a3ca385e7b..be91148169 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 29dde6de9b..2d16ddd374 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. */