refactor: Replace exec with spawn (#8510)

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

@@ -21,11 +21,12 @@ import {
   isEditorAvailable,
   type EditorType,
 } from './editor.js';
-import { execSync, spawn } from 'node:child_process';
+import { execSync, spawn, spawnSync } from 'node:child_process';
 
 vi.mock('child_process', () => ({
   execSync: vi.fn(),
   spawn: vi.fn(),
+  spawnSync: vi.fn(() => ({ error: null, status: 0 })),
 }));
 
 const originalPlatform = process.platform;
@@ -314,23 +315,23 @@ describe('editor utils', () => {
   });
 
   describe('openDiff', () => {
-    const spawnEditors: EditorType[] = [
+    const guiEditors: EditorType[] = [
       'vscode',
       'vscodium',
       'windsurf',
       'cursor',
       'zed',
     ];
-    for (const editor of spawnEditors) {
+
+    for (const editor of guiEditors) {
       it(`should call spawn for ${editor}`, async () => {
-        const mockSpawn = {
-          on: vi.fn((event, cb) => {
-            if (event === 'close') {
-              cb(0);
-            }
-          }),
-        };
-        (spawn as Mock).mockReturnValue(mockSpawn);
+        const mockSpawnOn = vi.fn((event, cb) => {
+          if (event === 'close') {
+            cb(0);
+          }
+        });
+        (spawn as Mock).mockReturnValue({ on: mockSpawnOn });
+
         await openDiff('old.txt', 'new.txt', editor, () => {});
         const diffCommand = getDiffCommand('old.txt', 'new.txt', editor)!;
         expect(spawn).toHaveBeenCalledWith(
@@ -338,77 +339,53 @@ describe('editor utils', () => {
           diffCommand.args,
           {
             stdio: 'inherit',
-            shell: true,
           },
         );
-        expect(mockSpawn.on).toHaveBeenCalledWith(
-          'close',
-          expect.any(Function),
-        );
-        expect(mockSpawn.on).toHaveBeenCalledWith(
-          'error',
-          expect.any(Function),
-        );
+        expect(mockSpawnOn).toHaveBeenCalledWith('close', expect.any(Function));
+        expect(mockSpawnOn).toHaveBeenCalledWith('error', expect.any(Function));
       });
 
       it(`should reject if spawn for ${editor} fails`, async () => {
         const mockError = new Error('spawn error');
-        const mockSpawn = {
-          on: vi.fn((event, cb) => {
-            if (event === 'error') {
-              cb(mockError);
-            }
-          }),
-        };
-        (spawn as Mock).mockReturnValue(mockSpawn);
+        const mockSpawnOn = vi.fn((event, cb) => {
+          if (event === 'error') {
+            cb(mockError);
+          }
+        });
+        (spawn as Mock).mockReturnValue({ on: mockSpawnOn });
+
         await expect(
           openDiff('old.txt', 'new.txt', editor, () => {}),
         ).rejects.toThrow('spawn error');
       });
 
       it(`should reject if ${editor} exits with non-zero code`, async () => {
-        const mockSpawn = {
-          on: vi.fn((event, cb) => {
-            if (event === 'close') {
-              cb(1);
-            }
-          }),
-        };
-        (spawn as Mock).mockReturnValue(mockSpawn);
+        const mockSpawnOn = vi.fn((event, cb) => {
+          if (event === 'close') {
+            cb(1);
+          }
+        });
+        (spawn as Mock).mockReturnValue({ on: mockSpawnOn });
+
         await expect(
           openDiff('old.txt', 'new.txt', editor, () => {}),
         ).rejects.toThrow(`${editor} exited with code 1`);
       });
     }
 
-    const execSyncEditors: EditorType[] = ['vim', 'neovim', 'emacs'];
-    for (const editor of execSyncEditors) {
-      it(`should call execSync for ${editor} on non-windows`, async () => {
-        Object.defineProperty(process, 'platform', { value: 'linux' });
-        await openDiff('old.txt', 'new.txt', editor, () => {});
-        expect(execSync).toHaveBeenCalledTimes(1);
-        const diffCommand = getDiffCommand('old.txt', 'new.txt', editor)!;
-        const expectedCommand = `${
-          diffCommand.command
-        } ${diffCommand.args.map((arg) => `"${arg}"`).join(' ')}`;
-        expect(execSync).toHaveBeenCalledWith(expectedCommand, {
-          stdio: 'inherit',
-          encoding: 'utf8',
-        });
-      });
+    const terminalEditors: EditorType[] = ['vim', 'neovim', 'emacs'];
 
-      it(`should call execSync for ${editor} on windows`, async () => {
-        Object.defineProperty(process, 'platform', { value: 'win32' });
+    for (const editor of terminalEditors) {
+      it(`should call spawnSync for ${editor}`, async () => {
         await openDiff('old.txt', 'new.txt', editor, () => {});
-        expect(execSync).toHaveBeenCalledTimes(1);
         const diffCommand = getDiffCommand('old.txt', 'new.txt', editor)!;
-        const expectedCommand = `${diffCommand.command} ${diffCommand.args.join(
-          ' ',
-        )}`;
-        expect(execSync).toHaveBeenCalledWith(expectedCommand, {
-          stdio: 'inherit',
-          encoding: 'utf8',
-        });
+        expect(spawnSync).toHaveBeenCalledWith(
+          diffCommand.command,
+          diffCommand.args,
+          {
+            stdio: 'inherit',
+          },
+        );
       });
     }
 
@@ -424,38 +401,48 @@ describe('editor utils', () => {
     });
 
     describe('onEditorClose callback', () => {
-      it('should call onEditorClose for execSync editors', async () => {
-        (execSync as Mock).mockReturnValue(Buffer.from(`/usr/bin/`));
-        const onEditorClose = vi.fn();
-        await openDiff('old.txt', 'new.txt', 'vim', onEditorClose);
-        expect(execSync).toHaveBeenCalledTimes(1);
-        expect(onEditorClose).toHaveBeenCalledTimes(1);
-      });
-
-      it('should call onEditorClose for execSync editors when an error is thrown', async () => {
-        (execSync as Mock).mockImplementation(() => {
-          throw new Error('test error');
+      const terminalEditors: EditorType[] = ['vim', 'neovim', 'emacs'];
+      for (const editor of terminalEditors) {
+        it(`should call onEditorClose for ${editor} on close`, async () => {
+          const onEditorClose = vi.fn();
+          await openDiff('old.txt', 'new.txt', editor, onEditorClose);
+          expect(onEditorClose).toHaveBeenCalledTimes(1);
         });
-        const onEditorClose = vi.fn();
-        openDiff('old.txt', 'new.txt', 'vim', onEditorClose);
-        expect(execSync).toHaveBeenCalledTimes(1);
-        expect(onEditorClose).toHaveBeenCalledTimes(1);
-      });
 
-      it('should not call onEditorClose for spawn editors', async () => {
-        const onEditorClose = vi.fn();
-        const mockSpawn = {
-          on: vi.fn((event, cb) => {
+        it(`should call onEditorClose for ${editor} on error`, async () => {
+          const onEditorClose = vi.fn();
+          const mockError = new Error('spawn error');
+          (spawnSync as Mock).mockImplementation(() => {
+            throw mockError;
+          });
+
+          await expect(
+            openDiff('old.txt', 'new.txt', editor, onEditorClose),
+          ).rejects.toThrow('spawn error');
+          expect(onEditorClose).toHaveBeenCalledTimes(1);
+        });
+      }
+
+      const guiEditors: EditorType[] = [
+        'vscode',
+        'vscodium',
+        'windsurf',
+        'cursor',
+        'zed',
+      ];
+      for (const editor of guiEditors) {
+        it(`should not call onEditorClose for ${editor}`, async () => {
+          const onEditorClose = vi.fn();
+          const mockSpawnOn = vi.fn((event, cb) => {
             if (event === 'close') {
               cb(0);
             }
-          }),
-        };
-        (spawn as Mock).mockReturnValue(mockSpawn);
-        await openDiff('old.txt', 'new.txt', 'vscode', onEditorClose);
-        expect(spawn).toHaveBeenCalledTimes(1);
-        expect(onEditorClose).not.toHaveBeenCalled();
-      });
+          });
+          (spawn as Mock).mockReturnValue({ on: mockSpawnOn });
+          await openDiff('old.txt', 'new.txt', editor, onEditorClose);
+          expect(onEditorClose).not.toHaveBeenCalled();
+        });
+      }
     });
   });
 

packages/core/src/utils/editor.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { execSync, spawn } from 'node:child_process';
+import { execSync, spawn, spawnSync } from 'node:child_process';
 
 export type EditorType =
   | 'vscode'
@@ -173,57 +173,44 @@ export async function openDiff(
   }
 
   try {
-    switch (editor) {
-      case 'vscode':
-      case 'vscodium':
-      case 'windsurf':
-      case 'cursor':
-      case 'zed':
-        // Use spawn for GUI-based editors to avoid blocking the entire process
-        return new Promise((resolve, reject) => {
-          const childProcess = spawn(diffCommand.command, diffCommand.args, {
-            stdio: 'inherit',
-            shell: true,
-          });
+    const isTerminalEditor = ['vim', 'emacs', 'neovim'].includes(editor);
 
-          childProcess.on('close', (code) => {
-            if (code === 0) {
-              resolve();
-            } else {
-              reject(new Error(`${editor} exited with code ${code}`));
-            }
-          });
-
-          childProcess.on('error', (error) => {
-            reject(error);
-          });
+    if (isTerminalEditor) {
+      try {
+        const result = spawnSync(diffCommand.command, diffCommand.args, {
+          stdio: 'inherit',
         });
-
-      case 'vim':
-      case 'emacs':
-      case 'neovim': {
-        // Use execSync for terminal-based editors
-        const command =
-          process.platform === 'win32'
-            ? `${diffCommand.command} ${diffCommand.args.join(' ')}`
-            : `${diffCommand.command} ${diffCommand.args.map((arg) => `"${arg}"`).join(' ')}`;
-        try {
-          execSync(command, {
-            stdio: 'inherit',
-            encoding: 'utf8',
-          });
-        } catch (e) {
-          console.error('Error in onEditorClose callback:', e);
-        } finally {
-          onEditorClose();
+        if (result.error) {
+          throw result.error;
         }
-        break;
+        if (result.status !== 0) {
+          throw new Error(`${editor} exited with code ${result.status}`);
+        }
+      } finally {
+        onEditorClose();
       }
-
-      default:
-        throw new Error(`Unsupported editor: ${editor}`);
+      return;
     }
+
+    return new Promise<void>((resolve, reject) => {
+      const childProcess = spawn(diffCommand.command, diffCommand.args, {
+        stdio: 'inherit',
+      });
+
+      childProcess.on('close', (code) => {
+        if (code === 0) {
+          resolve();
+        } else {
+          reject(new Error(`${editor} exited with code ${code}`));
+        }
+      });
+
+      childProcess.on('error', (error) => {
+        reject(error);
+      });
+    });
   } catch (error) {
     console.error(error);
+    throw error;
   }
 }

packages/core/src/utils/shell-utils.ts

@@ -9,6 +9,7 @@ import type { Config } from '../config/config.js';
 import os from 'node:os';
 import { quote } from 'shell-quote';
 import { doesToolInvocationMatch } from './tool-utils.js';
+import { spawn, type SpawnOptionsWithoutStdio } from 'node:child_process';
 
 const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool'];
 
@@ -458,6 +459,37 @@ export function checkCommandPermissions(
  * @param config The application configuration.
  * @returns An object with 'allowed' boolean and optional 'reason' string if not allowed.
  */
+export const spawnAsync = (
+  command: string,
+  args: string[],
+  options?: SpawnOptionsWithoutStdio,
+): Promise<{ stdout: string; stderr: string }> =>
+  new Promise((resolve, reject) => {
+    const child = spawn(command, args, options);
+    let stdout = '';
+    let stderr = '';
+
+    child.stdout.on('data', (data) => {
+      stdout += data.toString();
+    });
+
+    child.stderr.on('data', (data) => {
+      stderr += data.toString();
+    });
+
+    child.on('close', (code) => {
+      if (code === 0) {
+        resolve({ stdout, stderr });
+      } else {
+        reject(new Error(`Command failed with exit code ${code}:\n${stderr}`));
+      }
+    });
+
+    child.on('error', (err) => {
+      reject(err);
+    });
+  });
+
 export function isCommandAllowed(
   command: string,
   config: Config,