fix: resolve infinite loop when using 'Modify with external editor' (#17453)

Co-authored-by: Jack Wotherspoon <jackwoth@google.com>
Co-authored-by: ehedlund <ehedlund@google.com>

packages/core/src/utils/editor.test.ts

@@ -14,17 +14,22 @@ import {
   type Mock,
 } from 'vitest';
 import {
-  checkHasEditorType,
+  hasValidEditorCommand,
+  hasValidEditorCommandAsync,
   getDiffCommand,
   openDiff,
   allowEditorTypeInSandbox,
   isEditorAvailable,
+  isEditorAvailableAsync,
+  resolveEditorAsync,
   type EditorType,
 } from './editor.js';
-import { execSync, spawn, spawnSync } from 'node:child_process';
+import { coreEvents, CoreEvent } from './events.js';
+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 })),
@@ -51,7 +56,7 @@ describe('editor utils', () => {
     });
   });
 
-  describe('checkHasEditorType', () => {
+  describe('hasValidEditorCommand', () => {
     const testCases: Array<{
       editor: EditorType;
       commands: string[];
@@ -89,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',
           });
@@ -103,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);
           });
         }
@@ -113,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);
         });
 
@@ -123,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]}`,
             {
@@ -142,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);
           });
         }
@@ -152,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);
         });
       });
@@ -542,4 +547,167 @@ describe('editor utils', () => {
       expect(isEditorAvailable('neovim')).toBe(true);
     });
   });
+
+  // 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('hasValidEditorCommandAsync', () => {
+    it('should return true if vim command exists', async () => {
+      Object.defineProperty(process, 'platform', { value: 'linux' });
+      mockExecAsync((cmd) => cmd.includes('vim'));
+      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 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 hasValidEditorCommandAsync('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('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).toBe('vim');
+    });
+
+    it('should request editor selection when preferred editor is not installed', async () => {
+      mockExecAsync(() => false);
+      vi.stubEnv('SANDBOX', '');
+      const resolvePromise = resolveEditorAsync('vim');
+      setTimeout(
+        () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'neovim' }),
+        0,
+      );
+      const result = await resolvePromise;
+      expect(result).toBe('neovim');
+    });
+
+    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 resolvePromise = resolveEditorAsync('vscode');
+      setTimeout(
+        () => coreEvents.emit(CoreEvent.EditorSelected, { editor: 'vim' }),
+        0,
+      );
+      const result = await resolvePromise;
+      expect(result).toBe('vim');
+    });
+
+    it('should request editor selection when no preference is set', async () => {
+      const emitSpy = vi.spyOn(coreEvents, 'emit');
+      vi.stubEnv('SANDBOX', '');
+
+      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 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 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 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);
+    });
+  });
 });