diff --git a/packages/cli/src/ui/hooks/useGitBranchName.test.ts b/packages/cli/src/ui/hooks/useGitBranchName.test.ts index 9b1319ce6b..6f0a03adfe 100644 --- a/packages/cli/src/ui/hooks/useGitBranchName.test.ts +++ b/packages/cli/src/ui/hooks/useGitBranchName.test.ts @@ -7,20 +7,31 @@ import type { MockedFunction } from 'vitest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { act } from 'react'; -import { renderHook } from '@testing-library/react'; +import { renderHook, waitFor } from '@testing-library/react'; import { useGitBranchName } from './useGitBranchName.js'; import { fs, vol } from 'memfs'; // For mocking fs -import { EventEmitter } from 'node:events'; -import { exec as mockExec, type ChildProcess } from 'node:child_process'; -import type { FSWatcher } from 'memfs/lib/volume.js'; -// Mock child_process -vi.mock('child_process'); +import type { FSWatcher } from 'memfs/lib/volume.js'; +import { spawnAsync as mockSpawnAsync } from '@google/gemini-cli-core'; + +// Mock @google/gemini-cli-core +vi.mock('@google/gemini-cli-core', async () => { + const original = await vi.importActual< + typeof import('@google/gemini-cli-core') + >('@google/gemini-cli-core'); + return { + ...original, + spawnAsync: vi.fn(), + }; +}); // Mock fs and fs/promises vi.mock('node:fs', async () => { const memfs = await vi.importActual('memfs'); - return memfs.fs; + return { + ...memfs.fs, + default: memfs.fs, + }; }); vi.mock('node:fs/promises', async () => { @@ -29,13 +40,13 @@ vi.mock('node:fs/promises', async () => { }); const CWD = '/test/project'; -const GIT_HEAD_PATH = `${CWD}/.git/HEAD`; +const GIT_LOGS_HEAD_PATH = `${CWD}/.git/logs/HEAD`; describe('useGitBranchName', () => { beforeEach(() => { vol.reset(); // Reset in-memory filesystem vol.fromJSON({ - [GIT_HEAD_PATH]: 'ref: refs/heads/main', + [GIT_LOGS_HEAD_PATH]: 'ref: refs/heads/main', }); vi.useFakeTimers(); // Use fake timers for async operations }); @@ -46,13 +57,11 @@ describe('useGitBranchName', () => { }); it('should return branch name', async () => { - (mockExec as MockedFunction).mockImplementation( - (_command, _options, callback) => { - callback?.(null, 'main\n', ''); - return new EventEmitter() as ChildProcess; - }, + (mockSpawnAsync as MockedFunction).mockResolvedValue( + { + stdout: 'main\n', + } as { stdout: string; stderr: string }, ); - const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -64,11 +73,8 @@ describe('useGitBranchName', () => { }); it('should return undefined if git command fails', async () => { - (mockExec as MockedFunction).mockImplementation( - (_command, _options, callback) => { - callback?.(new Error('Git error'), '', 'error output'); - return new EventEmitter() as ChildProcess; - }, + (mockSpawnAsync as MockedFunction).mockRejectedValue( + new Error('Git error'), ); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -82,16 +88,16 @@ describe('useGitBranchName', () => { }); it('should return short commit hash if branch is HEAD (detached state)', async () => { - (mockExec as MockedFunction).mockImplementation( - (command, _options, callback) => { - if (command === 'git rev-parse --abbrev-ref HEAD') { - callback?.(null, 'HEAD\n', ''); - } else if (command === 'git rev-parse --short HEAD') { - callback?.(null, 'a1b2c3d\n', ''); - } - return new EventEmitter() as ChildProcess; - }, - ); + ( + mockSpawnAsync as MockedFunction + ).mockImplementation(async (command: string, args: string[]) => { + if (args.includes('--abbrev-ref')) { + return { stdout: 'HEAD\n' } as { stdout: string; stderr: string }; + } else if (args.includes('--short')) { + return { stdout: 'a1b2c3d\n' } as { stdout: string; stderr: string }; + } + return { stdout: '' } as { stdout: string; stderr: string }; + }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -102,16 +108,16 @@ describe('useGitBranchName', () => { }); it('should return undefined if branch is HEAD and getting commit hash fails', async () => { - (mockExec as MockedFunction).mockImplementation( - (command, _options, callback) => { - if (command === 'git rev-parse --abbrev-ref HEAD') { - callback?.(null, 'HEAD\n', ''); - } else if (command === 'git rev-parse --short HEAD') { - callback?.(new Error('Git error'), '', 'error output'); - } - return new EventEmitter() as ChildProcess; - }, - ); + ( + mockSpawnAsync as MockedFunction + ).mockImplementation(async (command: string, args: string[]) => { + if (args.includes('--abbrev-ref')) { + return { stdout: 'HEAD\n' } as { stdout: string; stderr: string }; + } else if (args.includes('--short')) { + throw new Error('Git error'); + } + return { stdout: '' } as { stdout: string; stderr: string }; + }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); await act(async () => { @@ -123,12 +129,15 @@ describe('useGitBranchName', () => { it('should update branch name when .git/HEAD changes', async ({ skip }) => { skip(); // TODO: fix - (mockExec as MockedFunction).mockImplementationOnce( - (_command, _options, callback) => { - callback?.(null, 'main\n', ''); - return new EventEmitter() as ChildProcess; - }, - ); + (mockSpawnAsync as MockedFunction) + .mockResolvedValueOnce({ stdout: 'main\n' } as { + stdout: string; + stderr: string; + }) + .mockResolvedValueOnce({ stdout: 'develop\n' } as { + stdout: string; + stderr: string; + }); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -138,34 +147,27 @@ describe('useGitBranchName', () => { }); expect(result.current).toBe('main'); - // Simulate a branch change - (mockExec as MockedFunction).mockImplementationOnce( - (_command, _options, callback) => { - callback?.(null, 'develop\n', ''); - return new EventEmitter() as ChildProcess; - }, - ); - // Simulate file change event // Ensure the watcher is set up before triggering the change await act(async () => { - fs.writeFileSync(GIT_HEAD_PATH, 'ref: refs/heads/develop'); // Trigger watcher + fs.writeFileSync(GIT_LOGS_HEAD_PATH, 'ref: refs/heads/develop'); // Trigger watcher vi.runAllTimers(); // Process timers for watcher and exec rerender(); }); - expect(result.current).toBe('develop'); + await waitFor(() => { + expect(result.current).toBe('develop'); + }); }); it('should handle watcher setup error silently', async () => { - // Remove .git/HEAD to cause an error in fs.watch setup - vol.unlinkSync(GIT_HEAD_PATH); + // Remove .git/logs/HEAD to cause an error in fs.watch setup + vol.unlinkSync(GIT_LOGS_HEAD_PATH); - (mockExec as MockedFunction).mockImplementation( - (_command, _options, callback) => { - callback?.(null, 'main\n', ''); - return new EventEmitter() as ChildProcess; - }, + (mockSpawnAsync as MockedFunction).mockResolvedValue( + { + stdout: 'main\n', + } as { stdout: string; stderr: string }, ); const { result, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -177,23 +179,21 @@ describe('useGitBranchName', () => { expect(result.current).toBe('main'); // Branch name should still be fetched initially - // Try to trigger a change that would normally be caught by the watcher - (mockExec as MockedFunction).mockImplementationOnce( - (_command, _options, callback) => { - callback?.(null, 'develop\n', ''); - return new EventEmitter() as ChildProcess; - }, - ); + ( + mockSpawnAsync as MockedFunction + ).mockResolvedValueOnce({ + stdout: 'develop\n', + } as { stdout: string; stderr: string }); // This write would trigger the watcher if it was set up // but since it failed, the branch name should not update // We need to create the file again for writeFileSync to not throw vol.fromJSON({ - [GIT_HEAD_PATH]: 'ref: refs/heads/develop', + [GIT_LOGS_HEAD_PATH]: 'ref: refs/heads/develop', }); await act(async () => { - fs.writeFileSync(GIT_HEAD_PATH, 'ref: refs/heads/develop'); + fs.writeFileSync(GIT_LOGS_HEAD_PATH, 'ref: refs/heads/develop'); vi.runAllTimers(); rerender(); }); @@ -209,11 +209,10 @@ describe('useGitBranchName', () => { close: closeMock, } as unknown as FSWatcher); - (mockExec as MockedFunction).mockImplementation( - (_command, _options, callback) => { - callback?.(null, 'main\n', ''); - return new EventEmitter() as ChildProcess; - }, + (mockSpawnAsync as MockedFunction).mockResolvedValue( + { + stdout: 'main\n', + } as { stdout: string; stderr: string }, ); const { unmount, rerender } = renderHook(() => useGitBranchName(CWD)); @@ -224,7 +223,10 @@ describe('useGitBranchName', () => { }); unmount(); - expect(watchMock).toHaveBeenCalledWith(GIT_HEAD_PATH, expect.any(Function)); + expect(watchMock).toHaveBeenCalledWith( + GIT_LOGS_HEAD_PATH, + expect.any(Function), + ); expect(closeMock).toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/hooks/useGitBranchName.ts b/packages/cli/src/ui/hooks/useGitBranchName.ts index d1ce55fdbf..d56af9a32f 100644 --- a/packages/cli/src/ui/hooks/useGitBranchName.ts +++ b/packages/cli/src/ui/hooks/useGitBranchName.ts @@ -5,7 +5,7 @@ */ import { useState, useEffect, useCallback } from 'react'; -import { exec } from 'node:child_process'; +import { spawnAsync } from '@google/gemini-cli-core'; import fs from 'node:fs'; import fsPromises from 'node:fs/promises'; import path from 'node:path'; @@ -13,36 +13,28 @@ import path from 'node:path'; export function useGitBranchName(cwd: string): string | undefined { const [branchName, setBranchName] = useState(undefined); - const fetchBranchName = useCallback( - () => - exec( - 'git rev-parse --abbrev-ref HEAD', + const fetchBranchName = useCallback(async () => { + try { + const { stdout } = await spawnAsync( + 'git', + ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd }, - (error, stdout, _stderr) => { - if (error) { - setBranchName(undefined); - return; - } - const branch = stdout.toString().trim(); - if (branch && branch !== 'HEAD') { - setBranchName(branch); - } else { - exec( - 'git rev-parse --short HEAD', - { cwd }, - (error, stdout, _stderr) => { - if (error) { - setBranchName(undefined); - return; - } - setBranchName(stdout.toString().trim()); - }, - ); - } - }, - ), - [cwd, setBranchName], - ); + ); + const branch = stdout.toString().trim(); + if (branch && branch !== 'HEAD') { + setBranchName(branch); + } else { + const { stdout: hashStdout } = await spawnAsync( + 'git', + ['rev-parse', '--short', 'HEAD'], + { cwd }, + ); + setBranchName(hashStdout.toString().trim()); + } + } catch (_error) { + setBranchName(undefined); + } + }, [cwd, setBranchName]); useEffect(() => { fetchBranchName(); // Initial fetch diff --git a/packages/cli/src/ui/utils/clipboardUtils.ts b/packages/cli/src/ui/utils/clipboardUtils.ts index 43e2be153f..85582ad621 100644 --- a/packages/cli/src/ui/utils/clipboardUtils.ts +++ b/packages/cli/src/ui/utils/clipboardUtils.ts @@ -4,12 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { exec } from 'node:child_process'; -import { promisify } from 'node:util'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; - -const execAsync = promisify(exec); +import { spawnAsync } from '@google/gemini-cli-core'; /** * Checks if the system clipboard contains an image (macOS only for now) @@ -22,11 +19,10 @@ export async function clipboardHasImage(): Promise { try { // Use osascript to check clipboard type - const { stdout } = await execAsync( - `osascript -e 'clipboard info' 2>/dev/null | grep -qE "«class PNGf»|TIFF picture|JPEG picture|GIF picture|«class JPEG»|«class TIFF»" && echo "true" || echo "false"`, - { shell: '/bin/bash' }, - ); - return stdout.trim() === 'true'; + const { stdout } = await spawnAsync('osascript', ['-e', 'clipboard info']); + const imageRegex = + /«class PNGf»|TIFF picture|JPEG picture|GIF picture|«class JPEG»|«class TIFF»/; + return imageRegex.test(stdout); } catch { return false; } @@ -84,7 +80,7 @@ export async function saveClipboardImage( end try `; - const { stdout } = await execAsync(`osascript -e '${script}'`); + const { stdout } = await spawnAsync('osascript', ['-e', script]); if (stdout.trim() === 'success') { // Verify the file was created and has content diff --git a/packages/core/src/ide/ide-installer.test.ts b/packages/core/src/ide/ide-installer.test.ts index 4b1f2bc186..4225bd391a 100644 --- a/packages/core/src/ide/ide-installer.test.ts +++ b/packages/core/src/ide/ide-installer.test.ts @@ -12,7 +12,14 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { DetectedIde } from './detect-ide.js'; -vi.mock('child_process'); +vi.mock('node:child_process', async (importOriginal) => { + const actual = (await importOriginal()) as typeof child_process; + return { + ...actual, + execSync: vi.fn(), + spawnSync: vi.fn(() => ({ status: 0 })), + }; +}); vi.mock('fs'); vi.mock('os'); @@ -97,8 +104,13 @@ describe('ide-installer', () => { platform: 'linux', }); await installer.install(); - expect(child_process.execSync).toHaveBeenCalledWith( - '"code" --install-extension google.gemini-cli-vscode-ide-companion --force', + expect(child_process.spawnSync).toHaveBeenCalledWith( + 'code', + [ + '--install-extension', + 'google.gemini-cli-vscode-ide-companion', + '--force', + ], { stdio: 'pipe' }, ); }); diff --git a/packages/core/src/ide/ide-installer.ts b/packages/core/src/ide/ide-installer.ts index aabb413146..720b0f3c06 100644 --- a/packages/core/src/ide/ide-installer.ts +++ b/packages/core/src/ide/ide-installer.ts @@ -119,9 +119,23 @@ class VsCodeInstaller implements IdeInstaller { }; } - const command = `"${commandPath}" --install-extension google.gemini-cli-vscode-ide-companion --force`; try { - child_process.execSync(command, { stdio: 'pipe' }); + const result = child_process.spawnSync( + commandPath, + [ + '--install-extension', + 'google.gemini-cli-vscode-ide-companion', + '--force', + ], + { stdio: 'pipe' }, + ); + + if (result.status !== 0) { + throw new Error( + `Failed to install extension: ${result.stderr?.toString()}`, + ); + } + return { success: true, message: `${this.ideInfo.displayName} companion extension was installed successfully.`, diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index 5ff450c93b..17860d22be 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -4,18 +4,25 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + describe, + it, + expect, + vi, + beforeEach, + afterEach, + type Mock, +} from 'vitest'; import { GitService } from './gitService.js'; import { Storage } from '../config/storage.js'; import * as path from 'node:path'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; -import type { ChildProcess } from 'node:child_process'; import { getProjectHash, GEMINI_DIR } from '../utils/paths.js'; +import { spawnAsync } from '../utils/shell-utils.js'; -const hoistedMockExec = vi.hoisted(() => vi.fn()); -vi.mock('node:child_process', () => ({ - exec: hoistedMockExec, +vi.mock('../utils/shell-utils.js', () => ({ + spawnAsync: vi.fn(), })); const hoistedMockEnv = vi.hoisted(() => vi.fn()); @@ -69,13 +76,9 @@ describe('GitService', () => { vi.clearAllMocks(); hoistedIsGitRepositoryMock.mockReturnValue(true); - hoistedMockExec.mockImplementation((command, callback) => { - if (command === 'git --version') { - callback(null, 'git version 2.0.0'); - } else { - callback(new Error('Command not mocked')); - } - return {}; + (spawnAsync as Mock).mockResolvedValue({ + stdout: 'git version 2.0.0', + stderr: '', }); hoistedMockHomedir.mockReturnValue(homedir); @@ -120,13 +123,11 @@ describe('GitService', () => { it('should resolve true if git --version command succeeds', async () => { const service = new GitService(projectRoot, storage); await expect(service.verifyGitAvailability()).resolves.toBe(true); + expect(spawnAsync).toHaveBeenCalledWith('git', ['--version']); }); it('should resolve false if git --version command fails', async () => { - hoistedMockExec.mockImplementation((command, callback) => { - callback(new Error('git not found')); - return {} as ChildProcess; - }); + (spawnAsync as Mock).mockRejectedValue(new Error('git not found')); const service = new GitService(projectRoot, storage); await expect(service.verifyGitAvailability()).resolves.toBe(false); }); @@ -134,10 +135,7 @@ describe('GitService', () => { describe('initialize', () => { it('should throw an error if Git is not available', async () => { - hoistedMockExec.mockImplementation((command, callback) => { - callback(new Error('git not found')); - return {} as ChildProcess; - }); + (spawnAsync as Mock).mockRejectedValue(new Error('git not found')); const service = new GitService(projectRoot, storage); await expect(service.initialize()).rejects.toThrow( 'Checkpointing is enabled, but Git is not installed. Please install Git or disable checkpointing to continue.', diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 374be81eeb..350d9b1781 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -7,7 +7,7 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import { isNodeError } from '../utils/errors.js'; -import { exec } from 'node:child_process'; +import { spawnAsync } from '../utils/shell-utils.js'; import type { SimpleGit } from 'simple-git'; import { simpleGit, CheckRepoActions } from 'simple-git'; import type { Storage } from '../config/storage.js'; @@ -41,16 +41,13 @@ export class GitService { } } - verifyGitAvailability(): Promise { - return new Promise((resolve) => { - exec('git --version', (error) => { - if (error) { - resolve(false); - } else { - resolve(true); - } - }); - }); + async verifyGitAvailability(): Promise { + try { + await spawnAsync('git', ['--version']); + return true; + } catch (_error) { + return false; + } } /** diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index c60640c38b..db72124047 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/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(); + }); + } }); }); diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index c68780d95b..8b507926a0 100644 --- a/packages/core/src/utils/editor.ts +++ b/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((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; } } diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index cedebe6cdd..a3afbc32e6 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/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, diff --git a/scripts/telemetry_utils.js b/scripts/telemetry_utils.js index 340a057644..ebf45a4e1c 100644 --- a/scripts/telemetry_utils.js +++ b/scripts/telemetry_utils.js @@ -10,7 +10,7 @@ import path from 'node:path'; import fs from 'node:fs'; import net from 'node:net'; import os from 'node:os'; -import { execSync } from 'node:child_process'; +import { spawnSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import crypto from 'node:crypto'; @@ -44,10 +44,14 @@ export function getJson(url) { `gemini-cli-releases-${Date.now()}.json`, ); try { - execSync( - `curl -sL -H "User-Agent: gemini-cli-dev-script" -o "${tmpFile}" "${url}"`, - { stdio: 'pipe' }, + const result = spawnSync( + 'curl', + ['-sL', '-H', 'User-Agent: gemini-cli-dev-script', '-o', tmpFile, url], + { stdio: 'pipe', encoding: 'utf-8' }, ); + if (result.status !== 0) { + throw new Error(result.stderr); + } const content = fs.readFileSync(tmpFile, 'utf-8'); return JSON.parse(content); } catch (e) { @@ -62,9 +66,13 @@ export function getJson(url) { export function downloadFile(url, dest) { try { - execSync(`curl -fL -sS -o "${dest}" "${url}"`, { + const result = spawnSync('curl', ['-fL', '-sS', '-o', dest, url], { stdio: 'pipe', + encoding: 'utf-8', }); + if (result.status !== 0) { + throw new Error(result.stderr); + } return dest; } catch (e) { console.error(`Failed to download file from ${url}`); @@ -254,10 +262,20 @@ export async function ensureBinary( const actualExt = asset.name.endsWith('.zip') ? 'zip' : 'tar.gz'; + let result; if (actualExt === 'zip') { - execSync(`unzip -o "${archivePath}" -d "${tmpDir}"`, { stdio: 'pipe' }); + result = spawnSync('unzip', ['-o', archivePath, '-d', tmpDir], { + stdio: 'pipe', + encoding: 'utf-8', + }); } else { - execSync(`tar -xzf "${archivePath}" -C "${tmpDir}"`, { stdio: 'pipe' }); + result = spawnSync('tar', ['-xzf', archivePath, '-C', tmpDir], { + stdio: 'pipe', + encoding: 'utf-8', + }); + } + if (result.status !== 0) { + throw new Error(result.stderr); } const nameToFind = binaryNameInArchive || executableName;