diff --git a/packages/cli/src/ui/components/SessionSummaryDisplay.test.tsx b/packages/cli/src/ui/components/SessionSummaryDisplay.test.tsx index 9de9e1385b..e63a5907bc 100644 --- a/packages/cli/src/ui/components/SessionSummaryDisplay.test.tsx +++ b/packages/cli/src/ui/components/SessionSummaryDisplay.test.tsx @@ -12,7 +12,6 @@ import { useConfig } from '../contexts/ConfigContext.js'; import { type SessionMetrics } from '../contexts/SessionContext.js'; import { ToolCallDecision, - getShellConfiguration, isWindows, type WorktreeSettings, } from '@google/gemini-cli-core'; @@ -22,7 +21,6 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { await importOriginal(); return { ...actual, - getShellConfiguration: vi.fn(), isWindows: vi.fn(), }; }); @@ -45,7 +43,6 @@ vi.mock('../contexts/ConfigContext.js', async (importOriginal) => { }; }); -const getShellConfigurationMock = vi.mocked(getShellConfiguration); const isWindowsMock = vi.mocked(isWindows); const useSessionStatsMock = vi.mocked(SessionContext.useSessionStats); @@ -104,11 +101,6 @@ describe('', () => { }; beforeEach(() => { - getShellConfigurationMock.mockReturnValue({ - executable: 'bash', - argsPrefix: ['-c'], - shell: 'bash', - }); isWindowsMock.mockReturnValue(false); }); @@ -173,11 +165,6 @@ describe('', () => { it('renders a standard UUID-formatted session ID in the footer (powershell) on Windows', async () => { isWindowsMock.mockReturnValue(true); - getShellConfigurationMock.mockReturnValue({ - executable: 'powershell.exe', - argsPrefix: ['-NoProfile', '-Command'], - shell: 'powershell', - }); const uuidSessionId = '1234-abcd-5678-efgh'; const { lastFrame, unmount } = await renderWithMockedStats( @@ -192,11 +179,7 @@ describe('', () => { }); it('sanitizes a malicious session ID in the footer (powershell)', async () => { - getShellConfigurationMock.mockReturnValue({ - executable: 'powershell.exe', - argsPrefix: ['-NoProfile', '-Command'], - shell: 'powershell', - }); + isWindowsMock.mockReturnValue(true); const maliciousSessionId = "'; rm -rf / #"; const { lastFrame, unmount } = await renderWithMockedStats( diff --git a/packages/cli/src/ui/components/SessionSummaryDisplay.tsx b/packages/cli/src/ui/components/SessionSummaryDisplay.tsx index 55ef50c746..973a9e5296 100644 --- a/packages/cli/src/ui/components/SessionSummaryDisplay.tsx +++ b/packages/cli/src/ui/components/SessionSummaryDisplay.tsx @@ -10,8 +10,8 @@ import { useSessionStats } from '../contexts/SessionContext.js'; import { useConfig } from '../contexts/ConfigContext.js'; import { escapeShellArg, - getShellConfiguration, isWindows, + type ShellType, } from '@google/gemini-cli-core'; interface SessionSummaryDisplayProps { @@ -23,7 +23,7 @@ export const SessionSummaryDisplay: React.FC = ({ }) => { const { stats } = useSessionStats(); const config = useConfig(); - const { shell } = getShellConfiguration(); + const shell: ShellType = isWindows() ? 'powershell' : 'bash'; const worktreeSettings = config.getWorktreeSettings(); diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index 5b155a8516..d2645e7b4a 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -365,7 +365,7 @@ describe('HookRunner', () => { ); expect(spawn).toHaveBeenCalledWith( - expect.stringMatching(/bash|powershell/), + expect.stringMatching(/bash|pwsh|powershell/), expect.arrayContaining([ expect.stringMatching(/['"]?\/test\/project['"]?\/hooks\/test\.sh/), ]), @@ -408,7 +408,7 @@ describe('HookRunner', () => { ); expect(spawn).toHaveBeenCalledWith( - expect.stringMatching(/bash|powershell/), + expect.stringMatching(/bash|pwsh|powershell/), expect.arrayContaining([ expect.stringMatching( /ls ['"]\/test\/project\/plans with spaces['"]/, @@ -447,7 +447,7 @@ describe('HookRunner', () => { // If secure, spawn will be called with the shell executable and escaped command expect(spawn).toHaveBeenCalledWith( - expect.stringMatching(/bash|powershell/), + expect.stringMatching(/bash|pwsh|powershell/), expect.arrayContaining([ expect.stringMatching(/ls (['"]).*echo.*pwned.*\1/), ]), diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index f1aa08f41f..aba20dff0e 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -213,7 +213,7 @@ describe('ShellExecutionService', () => { mockSerializeTerminalToObject.mockReturnValue([]); mockIsBinary.mockReturnValue(false); mockPlatform.mockReturnValue('linux'); - mockResolveExecutable.mockImplementation(async (exe: string) => exe); + mockResolveExecutable.mockImplementation((exe: string) => exe); process.env['PATH'] = '/test/path'; mockGetPty.mockResolvedValue({ module: { spawn: mockPtySpawn }, @@ -2064,7 +2064,7 @@ describe('ShellExecutionService environment variables', () => { sandboxManager: mockSandboxManager, }; - mockResolveExecutable.mockResolvedValue('/bin/bash/resolved'); + mockResolveExecutable.mockReturnValue('/bin/bash/resolved'); const mockChild = new EventEmitter() as unknown as ChildProcess; mockChild.stdout = new EventEmitter() as unknown as Readable; mockChild.stderr = new EventEmitter() as unknown as Readable; diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 5817ffd338..043a40acc3 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -414,8 +414,7 @@ export class ShellExecutionService { executable = 'cmd.exe'; } - const resolvedExecutable = - (await resolveExecutable(executable)) ?? executable; + const resolvedExecutable = resolveExecutable(executable) ?? executable; const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); const spawnArgs = [...argsPrefix, guardedCommand]; diff --git a/packages/core/src/services/shellExecutionService.windows.integration.test.ts b/packages/core/src/services/shellExecutionService.windows.integration.test.ts new file mode 100644 index 0000000000..069f4e46da --- /dev/null +++ b/packages/core/src/services/shellExecutionService.windows.integration.test.ts @@ -0,0 +1,89 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import os from 'node:os'; +import { ShellExecutionService } from './shellExecutionService.js'; +import { NoopSandboxManager } from './sandboxManager.js'; + +const isWindows = os.platform() === 'win32'; + +/** + * Real-shell integration tests that reproduce the regression class from + * issue #25859: commands with inline double quotes executed on Windows + * lose their quotes when they reach the native executable, because + * Windows PowerShell 5.1 mangles embedded " during native-command + * argument passing. PowerShell 7 (pwsh.exe) passes arguments correctly. + * + * These tests exercise the full pipeline end-to-end. They pass when + * gemini-cli selects pwsh.exe from PATH; they fail when the pipeline + * routes through Windows PowerShell 5.1. + */ +describe.skipIf(!isWindows)( + 'ShellExecutionService Windows quoting (real shell)', + () => { + const baseConfig = { + sanitizationConfig: { + allowedEnvironmentVariables: [], + blockedEnvironmentVariables: [], + enableEnvironmentVariableRedaction: false, + }, + sandboxManager: new NoopSandboxManager(), + }; + + async function runReal(command: string) { + const controller = new AbortController(); + const handle = await ShellExecutionService.execute( + command, + process.cwd(), + () => {}, + controller.signal, + false, + baseConfig, + ); + const result = await handle.result; + return { result, output: result.output }; + } + + it('should preserve inline double quotes through node -e', async () => { + const { result, output } = await runReal( + `node -e 'console.log("preserved")'`, + ); + expect(result.exitCode).toBe(0); + expect(output).toBe('preserved'); + }); + + it('should preserve double quotes inside JSON output', async () => { + const { result, output } = await runReal( + `node -e 'console.log(JSON.stringify({ok:"yes"}))'`, + ); + expect(result.exitCode).toBe(0); + expect(output).toBe('{"ok":"yes"}'); + }); + + it('should handle quoted argument containing a space', async () => { + const { result, output } = await runReal( + `node -e "console.log('hello world')"`, + ); + expect(result.exitCode).toBe(0); + expect(output).toBe('hello world'); + }); + + it('should handle a mixed-quote regex literal', async () => { + const { result, output } = await runReal( + `node -e 'console.log(String("a").match(/"/))'`, + ); + expect(result.exitCode).toBe(0); + expect(output).toBe('null'); + }); + + it('should pass a literal double-quote byte through to stdout', async () => { + const { result, output } = await runReal(`node -e 'console.log("\\"")'`); + expect(result.exitCode).toBe(0); + expect(output).toBe('"'); + }); + }, +); diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 5abadd50a0..ba34ab4d75 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -1825,7 +1825,7 @@ describe('resolveRipgrepPath', () => { it('should fall back to system PATH if both bundled paths are missing and system is trusted', async () => { vi.mocked(fileExists).mockResolvedValue(false); - vi.mocked(resolveExecutable).mockResolvedValue('/usr/bin/rg'); + vi.mocked(resolveExecutable).mockReturnValue('/usr/bin/rg'); vi.mocked(resolveToRealPath).mockReturnValue('/usr/bin/rg'); const resolvedPath = await resolveRipgrepPath(); @@ -1836,7 +1836,7 @@ describe('resolveRipgrepPath', () => { it('should reject system PATH if it is in the current working directory', async () => { vi.mocked(fileExists).mockResolvedValue(false); const unsafePath = path.join(process.cwd(), 'rg'); - vi.mocked(resolveExecutable).mockResolvedValue(unsafePath); + vi.mocked(resolveExecutable).mockReturnValue(unsafePath); vi.mocked(resolveToRealPath).mockReturnValue(unsafePath); const resolvedPath = await resolveRipgrepPath(); @@ -1848,7 +1848,7 @@ describe('resolveRipgrepPath', () => { const trustedLink = '/usr/local/bin/rg'; const trustedRealPath = '/opt/homebrew/Cellar/ripgrep/13.0.0/bin/rg'; - vi.mocked(resolveExecutable).mockResolvedValue(trustedLink); + vi.mocked(resolveExecutable).mockReturnValue(trustedLink); vi.mocked(resolveToRealPath).mockReturnValue(trustedRealPath); const resolvedPath = await resolveRipgrepPath(); @@ -1857,7 +1857,7 @@ describe('resolveRipgrepPath', () => { it('should return null if binary is missing from both bundled paths and system PATH', async () => { vi.mocked(fileExists).mockResolvedValue(false); - vi.mocked(resolveExecutable).mockResolvedValue(undefined); + vi.mocked(resolveExecutable).mockReturnValue(undefined); const resolvedPath = await resolveRipgrepPath(); expect(resolvedPath).toBeNull(); @@ -1883,7 +1883,7 @@ describe('resolveRipgrepPath', () => { it('should fall back to system PATH if system is trusted on Windows', async () => { vi.mocked(fileExists).mockResolvedValue(false); - vi.mocked(resolveExecutable).mockResolvedValue( + vi.mocked(resolveExecutable).mockReturnValue( 'C:\\Windows\\System32\\rg.exe', ); vi.mocked(resolveToRealPath).mockReturnValue( @@ -1898,7 +1898,7 @@ describe('resolveRipgrepPath', () => { it('should reject system PATH if it is untrusted on Windows', async () => { vi.mocked(fileExists).mockResolvedValue(false); const unsafePath = 'D:\\Downloads\\rg.exe'; - vi.mocked(resolveExecutable).mockResolvedValue(unsafePath); + vi.mocked(resolveExecutable).mockReturnValue(unsafePath); vi.mocked(resolveToRealPath).mockReturnValue(unsafePath); const resolvedPath = await resolveRipgrepPath(); diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 1a5ae54214..a6721b9b5a 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -72,7 +72,7 @@ export async function resolveRipgrepPath(): Promise { } // 3. Fallback: check system PATH - const systemRg = await resolveExecutable('rg'); + const systemRg = resolveExecutable('rg'); if (systemRg) { // Security: Validate the system executable to prevent Search Path Interruption. const realPath = resolveToRealPath(systemRg); diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index ed1d709dae..bc96d0716c 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -38,17 +38,13 @@ vi.mock('os', () => ({ homedir: mockHomedir, })); -const mockAccess = vi.hoisted(() => vi.fn()); +const mockAccessSync = vi.hoisted(() => vi.fn()); vi.mock('node:fs', () => ({ default: { - promises: { - access: mockAccess, - }, + accessSync: mockAccessSync, constants: { X_OK: 1 }, }, - promises: { - access: mockAccess, - }, + accessSync: mockAccessSync, constants: { X_OK: 1 }, })); @@ -458,10 +454,8 @@ describe('escapeShellArg', () => { }); describe('getShellConfiguration', () => { - const originalEnv = { ...process.env }; - afterEach(() => { - process.env = originalEnv; + vi.unstubAllEnvs(); }); it('should return bash configuration on Linux', () => { @@ -483,19 +477,38 @@ describe('getShellConfiguration', () => { describe('on Windows', () => { beforeEach(() => { mockPlatform.mockReturnValue('win32'); + vi.stubEnv('ComSpec', ''); + mockAccessSync.mockImplementation(() => { + throw new Error('ENOENT'); + }); }); it('should return PowerShell configuration by default', () => { - delete process.env['ComSpec']; const config = getShellConfiguration(); expect(config.executable).toBe('powershell.exe'); expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); expect(config.shell).toBe('powershell'); }); + it.skipIf(!isWindowsRuntime)( + 'should prefer pwsh.exe over powershell.exe when pwsh is available in PATH', + () => { + const pwshDir = path.resolve('C:\\Program Files\\PowerShell\\7'); + const pwshPath = path.join(pwshDir, 'pwsh.exe'); + vi.stubEnv('PATH', pwshDir); + mockAccessSync.mockImplementation((p: string) => { + if (p === pwshPath) return; + throw new Error('ENOENT'); + }); + const config = getShellConfiguration(); + expect(config.executable).toBe(pwshPath); + expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); + expect(config.shell).toBe('powershell'); + }, + ); + it('should ignore ComSpec when pointing to cmd.exe', () => { - const cmdPath = 'C:\\WINDOWS\\system32\\cmd.exe'; - process.env['ComSpec'] = cmdPath; + vi.stubEnv('ComSpec', 'C:\\WINDOWS\\system32\\cmd.exe'); const config = getShellConfiguration(); expect(config.executable).toBe('powershell.exe'); expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); @@ -505,7 +518,7 @@ describe('getShellConfiguration', () => { it('should return PowerShell configuration if ComSpec points to powershell.exe', () => { const psPath = 'C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; - process.env['ComSpec'] = psPath; + vi.stubEnv('ComSpec', psPath); const config = getShellConfiguration(); expect(config.executable).toBe(psPath); expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); @@ -514,7 +527,7 @@ describe('getShellConfiguration', () => { it('should return PowerShell configuration if ComSpec points to pwsh.exe', () => { const pwshPath = 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; - process.env['ComSpec'] = pwshPath; + vi.stubEnv('ComSpec', pwshPath); const config = getShellConfiguration(); expect(config.executable).toBe(pwshPath); expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); @@ -522,7 +535,7 @@ describe('getShellConfiguration', () => { }); it('should be case-insensitive when checking ComSpec', () => { - process.env['ComSpec'] = 'C:\\Path\\To\\POWERSHELL.EXE'; + vi.stubEnv('ComSpec', 'C:\\Path\\To\\POWERSHELL.EXE'); const config = getShellConfiguration(); expect(config.executable).toBe('C:\\Path\\To\\POWERSHELL.EXE'); expect(config.argsPrefix).toEqual(['-NoProfile', '-Command']); @@ -566,63 +579,70 @@ describe('hasRedirection (PowerShell via mock)', () => { }); describe('resolveExecutable', () => { - const originalEnv = process.env; - beforeEach(() => { - process.env = { ...originalEnv }; - mockAccess.mockReset(); + mockAccessSync.mockReset(); }); afterEach(() => { - process.env = originalEnv; + vi.unstubAllEnvs(); }); - it('should return the absolute path if it exists and is executable', async () => { + it('should return the absolute path if it exists and is executable', () => { const absPath = path.resolve('/usr/bin/git'); - mockAccess.mockResolvedValue(undefined); // success - expect(await resolveExecutable(absPath)).toBe(absPath); - expect(mockAccess).toHaveBeenCalledWith(absPath, 1); + mockAccessSync.mockImplementation(() => undefined); + expect(resolveExecutable(absPath)).toBe(absPath); + expect(mockAccessSync).toHaveBeenCalledWith(absPath, 1); }); - it('should return undefined for absolute path if it does not exist', async () => { + it('should return undefined for absolute path if it does not exist', () => { const absPath = path.resolve('/usr/bin/nonexistent'); - mockAccess.mockRejectedValue(new Error('ENOENT')); - expect(await resolveExecutable(absPath)).toBeUndefined(); + mockAccessSync.mockImplementation(() => { + throw new Error('ENOENT'); + }); + expect(resolveExecutable(absPath)).toBeUndefined(); }); - it('should resolve executable in PATH', async () => { + it('should resolve executable in PATH', () => { const binDir = path.resolve('/bin'); const usrBinDir = path.resolve('/usr/bin'); - process.env['PATH'] = `${binDir}${path.delimiter}${usrBinDir}`; + vi.stubEnv('PATH', `${binDir}${path.delimiter}${usrBinDir}`); mockPlatform.mockReturnValue('linux'); const targetPath = path.join(usrBinDir, 'ls'); - mockAccess.mockImplementation(async (p: string) => { + mockAccessSync.mockImplementation((p: string) => { if (p === targetPath) return undefined; throw new Error('ENOENT'); }); - expect(await resolveExecutable('ls')).toBe(targetPath); + expect(resolveExecutable('ls')).toBe(targetPath); }); - it('should try extensions on Windows', async () => { + it('should try extensions on Windows', () => { const sys32 = path.resolve('C:\\Windows\\System32'); - process.env['PATH'] = sys32; + vi.stubEnv('PATH', sys32); mockPlatform.mockReturnValue('win32'); - mockAccess.mockImplementation(async (p: string) => { - // Use includes because on Windows path separators might differ + mockAccessSync.mockImplementation((p: string) => { if (p.includes('cmd.exe')) return undefined; throw new Error('ENOENT'); }); - expect(await resolveExecutable('cmd')).toContain('cmd.exe'); + expect(resolveExecutable('cmd')).toContain('cmd.exe'); }); - it('should return undefined if not found in PATH', async () => { - process.env['PATH'] = path.resolve('/bin'); + it('should return undefined if not found in PATH', () => { + vi.stubEnv('PATH', path.resolve('/bin')); mockPlatform.mockReturnValue('linux'); - mockAccess.mockRejectedValue(new Error('ENOENT')); + mockAccessSync.mockImplementation(() => { + throw new Error('ENOENT'); + }); - expect(await resolveExecutable('unknown')).toBeUndefined(); + expect(resolveExecutable('unknown')).toBeUndefined(); + }); + + it('should return undefined if PATH is unset', () => { + vi.stubEnv('PATH', ''); + mockPlatform.mockReturnValue('linux'); + + expect(resolveExecutable('anything')).toBeUndefined(); }); }); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index e1e5127e57..2c2d2aca7e 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -76,29 +76,30 @@ export interface ShellConfiguration { shell: ShellType; } -export async function resolveExecutable( - exe: string, -): Promise { - if (path.isAbsolute(exe)) { - try { - await fs.promises.access(exe, fs.constants.X_OK); - return exe; - } catch { - return undefined; - } +function isExecutable(filePath: string): boolean { + try { + fs.accessSync(filePath, fs.constants.X_OK); + return true; + } catch { + return false; + } +} + +export function resolveExecutable(exe: string): string | undefined { + if (path.isAbsolute(exe)) { + return isExecutable(exe) ? exe : undefined; + } + const pathEnv = process.env['PATH']; + if (!pathEnv) { + return undefined; } - const paths = (process.env['PATH'] || '').split(path.delimiter); const extensions = os.platform() === 'win32' ? ['.exe', '.cmd', '.bat', ''] : ['']; - - for (const p of paths) { + for (const dir of pathEnv.split(path.delimiter)) { for (const ext of extensions) { - const fullPath = path.join(p, exe + ext); - try { - await fs.promises.access(fullPath, fs.constants.X_OK); + const fullPath = path.join(dir, exe + ext); + if (isExecutable(fullPath)) { return fullPath; - } catch { - continue; } } } @@ -644,6 +645,14 @@ export function parseCommandDetails( * This ensures we can execute command strings predictably and securely across platforms * using the `spawn(executable, [...argsPrefix, commandString], { shell: false })` pattern. * + * On Windows, PowerShell 7 (pwsh.exe) is preferred over Windows PowerShell 5.1 + * (powershell.exe) when available on PATH. Windows PowerShell 5.1 silently + * strips embedded double quotes from arguments to native executables — see + * issue #25859. PowerShell 7 uses standards-compliant argument passing and + * does not exhibit this regression. When pwsh.exe is not installed, we fall + * back to powershell.exe to preserve the existing behavior and the full + * cmdlet surface users depend on. + * * @returns The ShellConfiguration for the current environment. */ export function getShellConfiguration(): ShellConfiguration { @@ -663,7 +672,16 @@ export function getShellConfiguration(): ShellConfiguration { } } - // Default to PowerShell for all other Windows configurations. + const pwshPath = resolveExecutable('pwsh.exe'); + if (pwshPath) { + return { + executable: pwshPath, + argsPrefix: ['-NoProfile', '-Command'], + shell: 'powershell', + }; + } + + // Fall back to Windows PowerShell 5.1 when pwsh.exe is not installed. return { executable: 'powershell.exe', argsPrefix: ['-NoProfile', '-Command'],