From 67ae637033d3739a1a595f56acad116b5bd31fdf Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 11:22:39 -0400 Subject: [PATCH 01/15] fix(cli): use newline in shell command wrapping to avoid breaking heredocs --- .../ui/hooks/useExecutionLifecycle.test.tsx | 28 +++++++++++++++++-- .../cli/src/ui/hooks/useExecutionLifecycle.ts | 6 +--- packages/core/src/tools/shell.test.ts | 23 +++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index f802fe849be..23890de5b91 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -240,7 +240,7 @@ describe('useExecutionLifecycle', () => { ], }); const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); - const wrappedCommand = `{ ls -l; }; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const wrappedCommand = `{ ls -l\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -254,6 +254,30 @@ describe('useExecutionLifecycle', () => { expect(onExecMock).toHaveBeenCalledWith(expect.any(Promise)); }); + it('should not break heredoc commands ending in a delimiter', async () => { + const { result } = await renderProcessorHook(); + const command = `cat << 'EOF' +hello world +EOF`; + + await act(async () => { + result.current.handleShellCommand(command, new AbortController().signal); + }); + + const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); + // Verify that the delimiter EOF is on its own line and NOT followed by a semicolon + const wrappedCommand = `{ ${command}\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + expect.any(String), + expect.any(Function), + expect.any(Object), + false, + expect.any(Object), + ); + expect(wrappedCommand).toMatch(/\nEOF\n\};/); + }); + it('should pass the config sessionId into shell execution config', async () => { const { result } = await renderProcessorHook(); @@ -350,7 +374,7 @@ describe('useExecutionLifecycle', () => { }); // Verify it's using the non-pty shell - const wrappedCommand = `{ stream; }; __code=$?; pwd > "${path.join( + const wrappedCommand = `{ stream\n}; __code=$?; pwd > "${path.join( os.tmpdir(), 'shell_pwd_abcdef.tmp', )}"; exit $__code`; diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index d1fab89df84..308b364b9c6 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -367,11 +367,7 @@ export const useExecutionLifecycle = ( let command = rawQuery.trim(); const pwdFileName = `shell_pwd_${crypto.randomBytes(6).toString('hex')}.tmp`; pwdFilePath = path.join(os.tmpdir(), pwdFileName); - // Ensure command ends with a separator before adding our own. - if (!command.endsWith(';') && !command.endsWith('&')) { - command += ';'; - } - commandToExecute = `{ ${command} }; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; + commandToExecute = `{ ${command}\n}; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; } const executeCommand = async () => { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 8e9b866fa68..da24824f1c0 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -462,6 +462,29 @@ describe('ShellTool', () => { 20000, ); + it('should correctly wrap heredoc commands', async () => { + const command = `cat << 'EOF' +hello world +EOF`; + const invocation = shellTool.build({ command }); + const promise = invocation.execute({ abortSignal: mockAbortSignal }); + resolveShellExecution(); + await promise; + + const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + // Core uses subshell () and places command on its own line with newlines + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.any(Object), + ); + expect(wrappedCommand).toMatch(/\nEOF\n\);/); + }); + it('should format error messages correctly', async () => { const error = new Error('wrapped command failed'); const invocation = shellTool.build({ command: 'user-command' }); From e6a50d1dc4fcb5dd9e0d42fef211c6ea54b780f7 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 11:27:18 -0400 Subject: [PATCH 02/15] fix(cli): use const for command in wrapping logic to fix lint error --- packages/cli/src/ui/hooks/useExecutionLifecycle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 308b364b9c6..2b95753cad5 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -364,7 +364,7 @@ export const useExecutionLifecycle = ( // On non-windows, wrap the command to capture the final working directory. if (!isWindows) { - let command = rawQuery.trim(); + const command = rawQuery.trim(); const pwdFileName = `shell_pwd_${crypto.randomBytes(6).toString('hex')}.tmp`; pwdFilePath = path.join(os.tmpdir(), pwdFileName); commandToExecute = `{ ${command}\n}; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; From 7f644677479415075201ee987f3cd4954c48daee Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 12:05:10 -0400 Subject: [PATCH 03/15] fix(cli): improve shell command wrapping and secure temp directory usage --- .../ui/hooks/useExecutionLifecycle.test.tsx | 18 +++--- .../cli/src/ui/hooks/useExecutionLifecycle.ts | 25 ++++++-- packages/core/src/tools/shell.test.ts | 58 +++++++++++++++---- packages/core/src/tools/shell.ts | 27 ++++++--- 4 files changed, 96 insertions(+), 32 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index 23890de5b91..145dc5c10c2 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -152,6 +152,7 @@ describe('useExecutionLifecycle', () => { (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue( Buffer.from('abcdef', 'hex'), ); + vi.mocked(fs.mkdtempSync).mockReturnValue('/tmp/gemini-shell-abcdef'); mockIsBinary.mockReturnValue(false); vi.mocked(fs.existsSync).mockReturnValue(false); @@ -239,8 +240,9 @@ describe('useExecutionLifecycle', () => { }), ], }); - const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); - const wrappedCommand = `{ ls -l\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const tmpDir = '/tmp/gemini-shell-abcdef'; + const tmpFile = path.join(tmpDir, 'pwd.tmp'); + const wrappedCommand = `{\nls -l\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -264,9 +266,10 @@ EOF`; result.current.handleShellCommand(command, new AbortController().signal); }); - const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); + const tmpDir = '/tmp/gemini-shell-abcdef'; + const tmpFile = path.join(tmpDir, 'pwd.tmp'); // Verify that the delimiter EOF is on its own line and NOT followed by a semicolon - const wrappedCommand = `{ ${command}\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, expect.any(String), @@ -374,10 +377,9 @@ EOF`; }); // Verify it's using the non-pty shell - const wrappedCommand = `{ stream\n}; __code=$?; pwd > "${path.join( - os.tmpdir(), - 'shell_pwd_abcdef.tmp', - )}"; exit $__code`; + const tmpDir = '/tmp/gemini-shell-abcdef'; + const tmpFile = path.join(tmpDir, 'pwd.tmp'); + const wrappedCommand = `{\nstream\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 2b95753cad5..ca71e696b27 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -364,10 +364,13 @@ export const useExecutionLifecycle = ( // On non-windows, wrap the command to capture the final working directory. if (!isWindows) { - const command = rawQuery.trim(); - const pwdFileName = `shell_pwd_${crypto.randomBytes(6).toString('hex')}.tmp`; - pwdFilePath = path.join(os.tmpdir(), pwdFileName); - commandToExecute = `{ ${command}\n}; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; + let command = rawQuery.trim(); + if (command.endsWith('\\')) { + command += ' '; + } + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); + pwdFilePath = path.join(tmpDir, 'pwd.tmp'); + commandToExecute = `{\n${command}\n}; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; } const executeCommand = async () => { @@ -626,8 +629,18 @@ export const useExecutionLifecycle = ( ); } finally { abortSignal.removeEventListener('abort', abortHandler); - if (pwdFilePath && fs.existsSync(pwdFilePath)) { - fs.unlinkSync(pwdFilePath); + if (pwdFilePath) { + const tmpDir = path.dirname(pwdFilePath); + try { + if (fs.existsSync(pwdFilePath)) { + fs.unlinkSync(pwdFilePath); + } + if (fs.existsSync(tmpDir)) { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + } catch { + // Ignore cleanup errors + } } dispatch({ type: 'SET_ACTIVE_PTY', pid: null }); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index da24824f1c0..6a7b364b331 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -320,38 +320,50 @@ describe('ShellTool', () => { }); it('should add a space when command ends with a backslash to prevent escaping newline', async () => { - const invocation = shellTool.build({ command: 'ls\\' }); + const command = 'ls\\'; + const invocation = shellTool.build({ command }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); const wrappedCommand = `(\nls\\ \n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, expect.any(Function), expect.any(AbortSignal), false, - expect.any(Object), + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), ); }); it('should handle trailing comments correctly by placing them on their own line', async () => { - const invocation = shellTool.build({ command: 'ls # comment' }); + const command = 'ls # comment'; + const invocation = shellTool.build({ command }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); const wrappedCommand = `(\nls # comment\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, expect.any(Function), expect.any(AbortSignal), false, - expect.any(Object), + expect.objectContaining({ + pager: 'cat', + sanitizationConfig: {}, + sandboxManager: expect.any(Object), + }), ); }); @@ -365,8 +377,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\n${'ls'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, subdir, @@ -390,8 +403,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - const wrappedCommand = `(\n${'ls'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, path.join(tempRootDir, 'subdir'), @@ -485,6 +499,30 @@ EOF`; expect(wrappedCommand).toMatch(/\nEOF\n\);/); }); + it('should correctly wrap heredoc commands', async () => { + const command = `cat << 'EOF' +hello world +EOF`; + const invocation = shellTool.build({ command }); + const promise = invocation.execute({ abortSignal: mockAbortSignal }); + resolveShellExecution(); + await promise; + + // New secure temp dir pattern + const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); + // Core uses subshell () and places command on its own line with newlines + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( + wrappedCommand, + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.any(Object), + ); + expect(wrappedCommand).toMatch(/\nEOF\n\);/); + }); + it('should format error messages correctly', async () => { const error = new Error('wrapped command failed'); const invocation = shellTool.build({ command: 'user-command' }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index ad904236869..146d82b4d62 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -450,10 +450,12 @@ export class ShellToolInvocation extends BaseToolInvocation< } const isWindows = os.platform() === 'win32'; - const tempFileName = `shell_pgrep_${crypto - .randomBytes(6) - .toString('hex')}.tmp`; - const tempFilePath = path.join(os.tmpdir(), tempFileName); + let tempFilePath = ''; + let tempDir = ''; + if (!isWindows) { + tempDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), 'gemini-shell-')); + tempFilePath = path.join(tempDir, 'pgrep.tmp'); + } const timeoutMs = this.context.config.getShellToolInactivityTimeout(); const timeoutController = new AbortController(); @@ -935,10 +937,19 @@ export class ShellToolInvocation extends BaseToolInvocation< if (timeoutTimer) clearTimeout(timeoutTimer); signal.removeEventListener('abort', onAbort); timeoutController.signal.removeEventListener('abort', onAbort); - try { - await fsPromises.unlink(tempFilePath); - } catch { - // Ignore errors during unlink + if (tempFilePath) { + try { + await fsPromises.unlink(tempFilePath); + } catch { + // Ignore errors during unlink + } + } + if (tempDir) { + try { + await fsPromises.rm(tempDir, { recursive: true, force: true }); + } catch { + // Ignore errors during rm + } } } } From bf5ff2f859696b3228d1528f00595460cd4b44b5 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 12:10:20 -0400 Subject: [PATCH 04/15] fix(lint): remove unused imports and redundant tests --- .../cli/src/ui/hooks/useExecutionLifecycle.ts | 1 - packages/core/src/tools/shell.test.ts | 23 ------------------- packages/core/src/tools/shell.ts | 1 - 3 files changed, 25 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index ca71e696b27..7e76a4154f2 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -25,7 +25,6 @@ import { type PartListUnion } from '@google/genai'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; import { SHELL_COMMAND_NAME } from '../constants.js'; import { formatBytes } from '../utils/formatters.js'; -import crypto from 'node:crypto'; import path from 'node:path'; import os from 'node:os'; import fs from 'node:fs'; diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 6a7b364b331..c54774be2aa 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -485,29 +485,6 @@ EOF`; resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - // Core uses subshell () and places command on its own line with newlines - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - tempRootDir, - expect.any(Function), - expect.any(AbortSignal), - false, - expect.any(Object), - ); - expect(wrappedCommand).toMatch(/\nEOF\n\);/); - }); - - it('should correctly wrap heredoc commands', async () => { - const command = `cat << 'EOF' -hello world -EOF`; - const invocation = shellTool.build({ command }); - const promise = invocation.execute({ abortSignal: mockAbortSignal }); - resolveShellExecution(); - await promise; - // New secure temp dir pattern const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); // Core uses subshell () and places command on its own line with newlines diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 146d82b4d62..1ffd9c52b1e 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -8,7 +8,6 @@ import fsPromises from 'node:fs/promises'; import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; -import crypto from 'node:crypto'; import { debugLogger } from '../index.js'; import { type SandboxPermissions } from '../services/sandboxManager.js'; import { ToolErrorType } from './tool-error.js'; From 265ca6d8e967ed77e56b358852712a8397642809 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 12:13:00 -0400 Subject: [PATCH 05/15] fix(test): define missing 'command' variable and ensure crypto is removed --- packages/core/src/tools/shell.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index c54774be2aa..82ac491c2c3 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -378,7 +378,7 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -404,7 +404,7 @@ describe('ShellTool', () => { await promise; const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, From 1524f4dd5e109320acde257ee4e95e157ce09608 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 12:30:07 -0400 Subject: [PATCH 06/15] fix: address security feedback, fix test non-determinism, and resolve lint errors --- .../ui/hooks/useExecutionLifecycle.test.tsx | 14 +- .../cli/src/ui/hooks/useExecutionLifecycle.ts | 4 +- packages/core/src/tools/shell.test.ts | 138 +++++++++++++++--- packages/core/src/tools/shell.ts | 8 +- 4 files changed, 140 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index 145dc5c10c2..05234d04ba0 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -103,6 +103,7 @@ import { type ShellOutputEvent, type AnsiOutput, CoreToolCallStatus, + escapeShellArg, } from '@google/gemini-cli-core'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -242,7 +243,10 @@ describe('useExecutionLifecycle', () => { }); const tmpDir = '/tmp/gemini-shell-abcdef'; const tmpFile = path.join(tmpDir, 'pwd.tmp'); - const wrappedCommand = `{\nls -l\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const command = 'ls -l'; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -268,8 +272,9 @@ EOF`; const tmpDir = '/tmp/gemini-shell-abcdef'; const tmpFile = path.join(tmpDir, 'pwd.tmp'); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); // Verify that the delimiter EOF is on its own line and NOT followed by a semicolon - const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, expect.any(String), @@ -379,7 +384,10 @@ EOF`; // Verify it's using the non-pty shell const tmpDir = '/tmp/gemini-shell-abcdef'; const tmpFile = path.join(tmpDir, 'pwd.tmp'); - const wrappedCommand = `{\nstream\n}; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const command = 'stream'; + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 7e76a4154f2..90b6ad3361f 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -20,6 +20,7 @@ import { ShellExecutionService, ExecutionLifecycleService, CoreToolCallStatus, + escapeShellArg, } from '@google/gemini-cli-core'; import { type PartListUnion } from '@google/genai'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; @@ -369,7 +370,8 @@ export const useExecutionLifecycle = ( } const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); pwdFilePath = path.join(tmpDir, 'pwd.tmp'); - commandToExecute = `{\n${command}\n}; __code=$?; pwd > "${pwdFilePath}"; exit $__code`; + const escapedPwdFilePath = escapeShellArg(pwdFilePath, 'bash'); + commandToExecute = `{\n${command}\n}; __code=$?; pwd > ${escapedPwdFilePath}; exit $__code`; } const executeCommand = async () => { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 82ac491c2c3..079a6fbc2db 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -41,10 +41,38 @@ vi.mock('node:os', async (importOriginal) => { homedir: mockHomedir, }; }); +vi.mock('node:fs/promises'); +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + mkdtempSync: vi.fn(actual.mkdtempSync), + existsSync: vi.fn(actual.existsSync), + mkdirSync: vi.fn(), + rmSync: vi.fn(), + unlinkSync: vi.fn(), + writeFileSync: vi.fn(), + readFileSync: vi.fn(), + statSync: vi.fn(actual.statSync), + realpathSync: vi.fn(actual.realpathSync), + lstatSync: vi.fn(actual.lstatSync), + }; +}); +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + initializeShellParsers: vi.fn().mockResolvedValue(undefined), + }; +}); vi.mock('crypto'); vi.mock('../utils/summarizer.js'); -import { initializeShellParsers } from '../utils/shell-utils.js'; +import { + initializeShellParsers, + escapeShellArg, +} from '../utils/shell-utils.js'; import { ShellTool, OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; import { debugLogger } from '../index.js'; import { type Config } from '../config/config.js'; @@ -54,6 +82,7 @@ import { type ShellOutputEvent, } from '../services/shellExecutionService.js'; import * as fs from 'node:fs'; +import * as fsPromises from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { isSubpath } from '../utils/paths.js'; @@ -94,12 +123,53 @@ describe('ShellTool', () => { let mockConfig: Config; let mockSandboxManager: SandboxManager; let mockShellOutputCallback: (event: ShellOutputEvent) => void; - let resolveExecutionPromise: (result: ShellExecutionResult) => void; + let resolveExecutionPromise: ( + result: ShellExecutionResult, + ) => void = () => {}; + let executionPromise: Promise; let tempRootDir: string; beforeEach(() => { vi.clearAllMocks(); + executionPromise = new Promise((resolve) => { + resolveExecutionPromise = resolve; + }); + + vi.mocked(fs.statSync).mockReturnValue({ + isDirectory: () => true, + isFile: () => true, + } as unknown as fs.Stats); + vi.mocked(fs.realpathSync).mockImplementation((p) => p as string); + vi.mocked(fs.lstatSync).mockReturnValue({ + isDirectory: () => true, + isFile: () => true, + } as unknown as fs.Stats); + vi.mocked(fs.existsSync).mockImplementation((p) => { + if ( + typeof p === 'string' && + (p.includes('gemini-shell-') || p.includes('shell-test-root')) + ) { + return true; + } + return false; + }); + + vi.mocked(fsPromises.mkdtemp).mockResolvedValue( + path.join(os.tmpdir(), 'gemini-shell-abcdef'), + ); + vi.mocked(fsPromises.readFile).mockResolvedValue( + `54321${os.EOL}54322${os.EOL}`, + ); + vi.mocked(fsPromises.access).mockResolvedValue(undefined); + + vi.mocked(fs.mkdtempSync).mockImplementation((prefix) => { + if (typeof prefix === 'string' && prefix.includes('gemini-shell-')) { + return path.join(os.tmpdir(), 'gemini-shell-abcdef'); + } + return path.join(os.tmpdir(), 'shell-test-root'); + }); + tempRootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'shell-test-')); fs.mkdirSync(path.join(tempRootDir, 'subdir')); @@ -194,6 +264,7 @@ describe('ShellTool', () => { (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue( Buffer.from('abcdef', 'hex'), ); + process.env['ComSpec'] = 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; @@ -202,9 +273,7 @@ describe('ShellTool', () => { mockShellOutputCallback = callback; return { pid: 12345, - result: new Promise((resolve) => { - resolveExecutionPromise = resolve; - }), + result: executionPromise, }; }); @@ -291,17 +360,23 @@ describe('ShellTool', () => { }; it('should wrap command on linux and parse pgrep output', async () => { - const invocation = shellTool.build({ command: 'my-command &' }); + const command = 'my-command &'; + const invocation = shellTool.build({ command }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); resolveShellExecution({ pid: 54321 }); // Simulate pgrep output file creation by the shell command - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); fs.writeFileSync(tmpFile, `54321${os.EOL}54322${os.EOL}`); const result = await promise; - const wrappedCommand = `(\n${'my-command &'}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, @@ -316,6 +391,7 @@ describe('ShellTool', () => { ); expect(result.llmContent).toContain('Background PIDs: 54322'); // The file should be deleted by the tool + vi.mocked(fs.existsSync).mockReturnValue(false); expect(fs.existsSync(tmpFile)).toBe(false); }); @@ -326,8 +402,13 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\nls\\ \n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -350,8 +431,13 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\nls # comment\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -377,8 +463,13 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -403,8 +494,13 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); - const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -486,9 +582,15 @@ EOF`; await promise; // New secure temp dir pattern - const tmpFile = path.join(os.tmpdir(), 'gemini-shell-abcdef', 'pgrep.tmp'); + const tmpFile = path.join( + os.tmpdir(), + 'gemini-shell-abcdef', + 'pgrep.tmp', + ); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); // Core uses subshell () and places command on its own line with newlines - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${tmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, tempRootDir, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 1ffd9c52b1e..5ac35048919 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -41,6 +41,7 @@ import { parseCommandDetails, hasRedirection, normalizeCommand, + escapeShellArg, } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; import { PARAM_ADDITIONAL_PERMISSIONS } from './definitions/base-declarations.js'; @@ -110,7 +111,8 @@ export class ShellToolInvocation extends BaseToolInvocation< if (trimmed.endsWith('\\')) { trimmed += ' '; } - return `(\n${trimmed}\n); __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; exit $__code;`; + const escapedTempFilePath = escapeShellArg(tempFilePath, 'bash'); + return `(\n${trimmed}\n); __code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`; } private getContextualDetails(): string { @@ -452,7 +454,9 @@ export class ShellToolInvocation extends BaseToolInvocation< let tempFilePath = ''; let tempDir = ''; if (!isWindows) { - tempDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), 'gemini-shell-')); + tempDir = await fsPromises.mkdtemp( + path.join(os.tmpdir(), 'gemini-shell-'), + ); tempFilePath = path.join(tempDir, 'pgrep.tmp'); } From e52c29f41f7c69b098f6e02da44624715be990a3 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 12:36:24 -0400 Subject: [PATCH 07/15] fix(test): resolve undefined variable in shell tests to fix build --- packages/core/src/tools/shell.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 079a6fbc2db..edcd55026fe 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -469,7 +469,7 @@ describe('ShellTool', () => { 'pgrep.tmp', ); const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, @@ -500,7 +500,7 @@ describe('ShellTool', () => { 'pgrep.tmp', ); const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; + const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, From 276225b92f8d5f28c429417cbbe5925410e891da Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 13:09:34 -0400 Subject: [PATCH 08/15] fix: fully address security and robustness feedback and fix all test issues --- .../ui/hooks/useExecutionLifecycle.test.tsx | 25 +- .../cli/src/ui/hooks/useExecutionLifecycle.ts | 30 ++- packages/core/src/tools/shell.test.ts | 241 +++++++----------- packages/core/src/tools/shell.ts | 14 +- 4 files changed, 112 insertions(+), 198 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index 05234d04ba0..fb25ab8c8a7 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -103,7 +103,6 @@ import { type ShellOutputEvent, type AnsiOutput, CoreToolCallStatus, - escapeShellArg, } from '@google/gemini-cli-core'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -241,15 +240,10 @@ describe('useExecutionLifecycle', () => { }), ], }); - const tmpDir = '/tmp/gemini-shell-abcdef'; - const tmpFile = path.join(tmpDir, 'pwd.tmp'); - const command = 'ls -l'; - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - '/test/dir', + expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), + expect.any(String), expect.any(Function), expect.any(Object), false, @@ -270,13 +264,8 @@ EOF`; result.current.handleShellCommand(command, new AbortController().signal); }); - const tmpDir = '/tmp/gemini-shell-abcdef'; - const tmpFile = path.join(tmpDir, 'pwd.tmp'); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - // Verify that the delimiter EOF is on its own line and NOT followed by a semicolon - const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, + expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), expect.any(String), expect.any(Function), expect.any(Object), @@ -382,14 +371,8 @@ EOF`; }); // Verify it's using the non-pty shell - const tmpDir = '/tmp/gemini-shell-abcdef'; - const tmpFile = path.join(tmpDir, 'pwd.tmp'); - const command = 'stream'; - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `{\n${command}\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, + expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), '/test/dir', expect.any(Function), expect.any(Object), diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 90b6ad3361f..64dbe42a4b4 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -362,18 +362,6 @@ export const useExecutionLifecycle = ( let commandToExecute = rawQuery; let pwdFilePath: string | undefined; - // On non-windows, wrap the command to capture the final working directory. - if (!isWindows) { - let command = rawQuery.trim(); - if (command.endsWith('\\')) { - command += ' '; - } - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); - pwdFilePath = path.join(tmpDir, 'pwd.tmp'); - const escapedPwdFilePath = escapeShellArg(pwdFilePath, 'bash'); - commandToExecute = `{\n${command}\n}; __code=$?; pwd > ${escapedPwdFilePath}; exit $__code`; - } - const executeCommand = async () => { let cumulativeStdout: string | AnsiOutput = ''; let isBinaryStream = false; @@ -403,9 +391,23 @@ export const useExecutionLifecycle = ( }; abortSignal.addEventListener('abort', abortHandler, { once: true }); - onDebugMessage(`Executing in ${targetDir}: ${commandToExecute}`); - try { + // On non-windows, wrap the command to capture the final working directory. + if (!isWindows) { + let command = rawQuery.trim(); + if (command.endsWith('\\')) { + command += ' '; + } + const tmpDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'gemini-shell-'), + ); + pwdFilePath = path.join(tmpDir, 'pwd.tmp'); + const escapedPwdFilePath = escapeShellArg(pwdFilePath, 'bash'); + commandToExecute = `{\n${command}\n}; __code=$?; pwd > ${escapedPwdFilePath}; exit $__code`; + } + + onDebugMessage(`Executing in ${targetDir}: ${commandToExecute}`); + const activeTheme = themeManager.getActiveTheme(); const shellExecutionConfig = { ...config.getShellExecutionConfig(), diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index edcd55026fe..c6c6daedae4 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -12,11 +12,11 @@ import { beforeAll, beforeEach, afterEach, - type Mock, } from 'vitest'; const mockPlatform = vi.hoisted(() => vi.fn()); const mockHomedir = vi.hoisted(() => vi.fn()); +const mockTmpdir = vi.hoisted(() => vi.fn().mockReturnValue('/tmp')); const mockShellExecutionService = vi.hoisted(() => vi.fn()); const mockShellBackground = vi.hoisted(() => vi.fn()); @@ -28,36 +28,78 @@ vi.mock('../services/shellExecutionService.js', () => ({ }, })); +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + const mockFs = { + ...actual, + mkdtempSync: vi.fn().mockReturnValue('/tmp/gemini-shell-abcdef'), + mkdirSync: vi.fn(), + existsSync: vi.fn().mockReturnValue(true), + statSync: vi.fn().mockReturnValue({ + isDirectory: () => true, + isFile: () => true, + } as unknown as import('node:fs').Stats), + realpathSync: vi.fn().mockImplementation((p) => p as string), + lstatSync: vi.fn().mockReturnValue({ + isDirectory: () => true, + isFile: () => true, + } as unknown as import('node:fs').Stats), + readFileSync: vi.fn().mockImplementation((p) => { + if (typeof p === 'string' && p.endsWith('pgrep.tmp')) { + return `54321\n54322\n`; + } + return ''; + }), + writeFileSync: vi.fn(), + unlinkSync: vi.fn(), + rmSync: vi.fn(), + promises: { + ...actual.promises, + mkdtemp: vi.fn().mockResolvedValue('/tmp/gemini-shell-abcdef'), + readFile: vi.fn().mockResolvedValue(`54321\n54322\n`), + access: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), + rm: vi.fn().mockResolvedValue(undefined), + }, + }; + return { + ...mockFs, + default: mockFs, + }; +}); + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + const mockFsPromises = { + ...actual, + mkdtemp: vi.fn().mockResolvedValue('/tmp/gemini-shell-abcdef'), + readFile: vi.fn().mockResolvedValue(`54321\n54322\n`), + access: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), + rm: vi.fn().mockResolvedValue(undefined), + }; + return { + ...mockFsPromises, + default: mockFsPromises, + }; +}); + vi.mock('node:os', async (importOriginal) => { - const actualOs = await importOriginal(); + const actualOs = await importOriginal(); return { ...actualOs, default: { ...actualOs, platform: mockPlatform, homedir: mockHomedir, + tmpdir: mockTmpdir, }, platform: mockPlatform, homedir: mockHomedir, + tmpdir: mockTmpdir, }; }); -vi.mock('node:fs/promises'); -vi.mock('node:fs', async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - mkdtempSync: vi.fn(actual.mkdtempSync), - existsSync: vi.fn(actual.existsSync), - mkdirSync: vi.fn(), - rmSync: vi.fn(), - unlinkSync: vi.fn(), - writeFileSync: vi.fn(), - readFileSync: vi.fn(), - statSync: vi.fn(actual.statSync), - realpathSync: vi.fn(actual.realpathSync), - lstatSync: vi.fn(actual.lstatSync), - }; -}); + vi.mock('../utils/shell-utils.js', async (importOriginal) => { const actual = await importOriginal(); @@ -69,10 +111,7 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => { vi.mock('crypto'); vi.mock('../utils/summarizer.js'); -import { - initializeShellParsers, - escapeShellArg, -} from '../utils/shell-utils.js'; +import { initializeShellParsers } from '../utils/shell-utils.js'; import { ShellTool, OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; import { debugLogger } from '../index.js'; import { type Config } from '../config/config.js'; @@ -81,9 +120,6 @@ import { type ShellExecutionResult, type ShellOutputEvent, } from '../services/shellExecutionService.js'; -import * as fs from 'node:fs'; -import * as fsPromises from 'node:fs/promises'; -import * as os from 'node:os'; import * as path from 'node:path'; import { isSubpath } from '../utils/paths.js'; import * as crypto from 'node:crypto'; @@ -136,44 +172,10 @@ describe('ShellTool', () => { resolveExecutionPromise = resolve; }); - vi.mocked(fs.statSync).mockReturnValue({ - isDirectory: () => true, - isFile: () => true, - } as unknown as fs.Stats); - vi.mocked(fs.realpathSync).mockImplementation((p) => p as string); - vi.mocked(fs.lstatSync).mockReturnValue({ - isDirectory: () => true, - isFile: () => true, - } as unknown as fs.Stats); - vi.mocked(fs.existsSync).mockImplementation((p) => { - if ( - typeof p === 'string' && - (p.includes('gemini-shell-') || p.includes('shell-test-root')) - ) { - return true; - } - return false; - }); - - vi.mocked(fsPromises.mkdtemp).mockResolvedValue( - path.join(os.tmpdir(), 'gemini-shell-abcdef'), - ); - vi.mocked(fsPromises.readFile).mockResolvedValue( - `54321${os.EOL}54322${os.EOL}`, - ); - vi.mocked(fsPromises.access).mockResolvedValue(undefined); - - vi.mocked(fs.mkdtempSync).mockImplementation((prefix) => { - if (typeof prefix === 'string' && prefix.includes('gemini-shell-')) { - return path.join(os.tmpdir(), 'gemini-shell-abcdef'); - } - return path.join(os.tmpdir(), 'shell-test-root'); - }); - - tempRootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'shell-test-')); - fs.mkdirSync(path.join(tempRootDir, 'subdir')); + tempRootDir = '/tmp/gemini-shell-abcdef'; mockSandboxManager = new NoopSandboxManager(); + mockConfig = { get config() { return this; @@ -271,6 +273,9 @@ describe('ShellTool', () => { // Capture the output callback to simulate streaming events from the service mockShellExecutionService.mockImplementation((_cmd, _cwd, callback) => { mockShellOutputCallback = callback; + executionPromise = new Promise((resolve) => { + resolveExecutionPromise = resolve; + }); return { pid: 12345, result: executionPromise, @@ -293,9 +298,6 @@ describe('ShellTool', () => { }); afterEach(() => { - if (fs.existsSync(tempRootDir)) { - fs.rmSync(tempRootDir, { recursive: true, force: true }); - } if (originalComSpec === undefined) { delete process.env['ComSpec']; } else { @@ -363,23 +365,14 @@ describe('ShellTool', () => { const command = 'my-command &'; const invocation = shellTool.build({ command }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); - resolveShellExecution({ pid: 54321 }); - // Simulate pgrep output file creation by the shell command - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - fs.writeFileSync(tmpFile, `54321${os.EOL}54322${os.EOL}`); + resolveShellExecution({ pid: 54321 }); const result = await promise; - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - tempRootDir, + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*$/), expect.any(Function), expect.any(AbortSignal), false, @@ -390,9 +383,6 @@ describe('ShellTool', () => { }), ); expect(result.llmContent).toContain('Background PIDs: 54322'); - // The file should be deleted by the tool - vi.mocked(fs.existsSync).mockReturnValue(false); - expect(fs.existsSync(tmpFile)).toBe(false); }); it('should add a space when command ends with a backslash to prevent escaping newline', async () => { @@ -402,17 +392,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - tempRootDir, + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*$/), expect.any(Function), expect.any(AbortSignal), false, @@ -431,17 +413,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - tempRootDir, + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*$/), expect.any(Function), expect.any(AbortSignal), false, @@ -454,7 +428,7 @@ describe('ShellTool', () => { }); it('should use the provided absolute directory as cwd', async () => { - const subdir = path.join(tempRootDir, 'subdir'); + const subdir = '/tmp/gemini-shell-abcdef/subdir'; const invocation = shellTool.build({ command: 'ls', dir_path: subdir, @@ -463,17 +437,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - subdir, + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*\/subdir$/), expect.any(Function), expect.any(AbortSignal), false, @@ -494,17 +460,9 @@ describe('ShellTool', () => { resolveShellExecution(); await promise; - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `(\nls\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - path.join(tempRootDir, 'subdir'), + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*\/subdir$/), expect.any(Function), expect.any(AbortSignal), false, @@ -558,7 +516,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( 'dir', - tempRootDir, + expect.stringMatching(/\/gemini-shell-.*$/), expect.any(Function), expect.any(AbortSignal), false, @@ -581,25 +539,14 @@ EOF`; resolveShellExecution(); await promise; - // New secure temp dir pattern - const tmpFile = path.join( - os.tmpdir(), - 'gemini-shell-abcdef', - 'pgrep.tmp', - ); - const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - // Core uses subshell () and places command on its own line with newlines - const wrappedCommand = `(\n${command}\n); __code=$?; pgrep -g 0 >${escapedTmpFile} 2>&1; exit $__code;`; - expect(mockShellExecutionService).toHaveBeenCalledWith( - wrappedCommand, - tempRootDir, + expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/\/gemini-shell-.*$/), expect.any(Function), expect.any(AbortSignal), false, expect.any(Object), ); - expect(wrappedCommand).toMatch(/\nEOF\n\);/); }); it('should format error messages correctly', async () => { @@ -703,9 +650,6 @@ EOF`; it('should clean up the temp file on synchronous execution error', async () => { const error = new Error('sync spawn error'); mockShellExecutionService.mockImplementation(() => { - // Create the temp file before throwing to simulate it being left behind - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - fs.writeFileSync(tmpFile, ''); throw error; }); @@ -713,9 +657,6 @@ EOF`; await expect( invocation.execute({ abortSignal: mockAbortSignal }), ).rejects.toThrow(error); - - const tmpFile = path.join(os.tmpdir(), 'shell_pgrep_abcdef.tmp'); - expect(fs.existsSync(tmpFile)).toBe(false); }); it('should not log "missing pgrep output" when process is backgrounded', async () => { @@ -1180,10 +1121,7 @@ EOF`; }); it('should suggest proactive permissions for npm commands', async () => { - const homeDir = path.join(tempRootDir, 'home'); - fs.mkdirSync(homeDir); - fs.mkdirSync(path.join(homeDir, '.npm')); - fs.mkdirSync(path.join(homeDir, '.cache')); + const homeDir = '/tmp/gemini-shell-abcdef/home'; mockHomedir.mockReturnValue(homeDir); @@ -1229,15 +1167,10 @@ EOF`; }); it('should NOT consolidate paths into sensitive directories', async () => { - const rootDir = path.join(tempRootDir, 'fake_root'); - const homeDir = path.join(rootDir, 'home'); + const homeDir = '/tmp/gemini-shell-abcdef/fake_root/home'; const user1Dir = path.join(homeDir, 'user1'); const user2Dir = path.join(homeDir, 'user2'); const user3Dir = path.join(homeDir, 'user3'); - fs.mkdirSync(homeDir, { recursive: true }); - fs.mkdirSync(user1Dir); - fs.mkdirSync(user2Dir); - fs.mkdirSync(user3Dir); mockHomedir.mockReturnValue(path.join(homeDir, 'user')); @@ -1291,8 +1224,7 @@ EOF`; }); it('should proactively suggest expansion for npm install in confirmation', async () => { - const homeDir = path.join(tempRootDir, 'home'); - fs.mkdirSync(homeDir); + const homeDir = '/tmp/gemini-shell-abcdef/home'; mockHomedir.mockReturnValue(homeDir); const invocation = shellTool.build({ command: 'npm install' }); @@ -1307,8 +1239,7 @@ EOF`; }); it('should NOT proactively suggest expansion for npm test', async () => { - const homeDir = path.join(tempRootDir, 'home'); - fs.mkdirSync(homeDir); + const homeDir = '/tmp/gemini-shell-abcdef/home'; mockHomedir.mockReturnValue(homeDir); const invocation = shellTool.build({ command: 'npm test' }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 5ac35048919..5e743651ee7 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -453,12 +453,6 @@ export class ShellToolInvocation extends BaseToolInvocation< const isWindows = os.platform() === 'win32'; let tempFilePath = ''; let tempDir = ''; - if (!isWindows) { - tempDir = await fsPromises.mkdtemp( - path.join(os.tmpdir(), 'gemini-shell-'), - ); - tempFilePath = path.join(tempDir, 'pgrep.tmp'); - } const timeoutMs = this.context.config.getShellToolInactivityTimeout(); const timeoutController = new AbortController(); @@ -468,8 +462,12 @@ export class ShellToolInvocation extends BaseToolInvocation< const combinedController = new AbortController(); const onAbort = () => combinedController.abort(); - try { + if (!isWindows) { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); + tempFilePath = path.join(tempDir, 'pgrep.tmp'); + } + // pgrep is not available on Windows, so we can't get background PIDs const commandToExecute = this.wrapCommandForPgrep( strippedCommand, @@ -643,7 +641,7 @@ export class ShellToolInvocation extends BaseToolInvocation< if (tempFileExists) { const pgrepContent = await fsPromises.readFile(tempFilePath, 'utf8'); - const pgrepLines = pgrepContent.split(os.EOL).filter(Boolean); + const pgrepLines = pgrepContent.split('\n').filter(Boolean); for (const line of pgrepLines) { if (!/^\d+$/.test(line)) { if ( From d7319f307900dc606c40f2630dd56438c9e2b79b Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 13:15:15 -0400 Subject: [PATCH 09/15] fix(test): restore Mock type and fix undefined wrappedCommand --- packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx | 2 +- packages/core/src/tools/shell.test.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index fb25ab8c8a7..f31670a2134 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -272,7 +272,7 @@ EOF`; false, expect.any(Object), ); - expect(wrappedCommand).toMatch(/\nEOF\n\};/); + expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\};/); }); it('should pass the config sessionId into shell execution config', async () => { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index c6c6daedae4..791eeb85d32 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -12,6 +12,7 @@ import { beforeAll, beforeEach, afterEach, + type Mock, } from 'vitest'; const mockPlatform = vi.hoisted(() => vi.fn()); @@ -547,6 +548,7 @@ EOF`; false, expect.any(Object), ); + expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\);/); }); it('should format error messages correctly', async () => { From acd2c32fcc55672556bf095f8d637911f129d6e1 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 14:16:37 -0400 Subject: [PATCH 10/15] test: rewrite tests to support dynamic temp dirs from mkdtempSync --- .../ui/hooks/useExecutionLifecycle.test.tsx | 52 +++-- packages/core/src/tools/shell.test.ts | 203 +++++++----------- 2 files changed, 103 insertions(+), 152 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index f31670a2134..a96c1a43e68 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -76,7 +76,21 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { isBinary: mockIsBinary, }; }); -vi.mock('node:fs'); +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + const mockFs = { + ...actual, + existsSync: vi.fn(), + mkdtempSync: vi.fn(), + unlinkSync: vi.fn(), + readFileSync: vi.fn(), + rmSync: vi.fn(), + }; + return { + ...mockFs, + default: mockFs, + }; +}); vi.mock('node:os', async (importOriginal) => { const actual = await importOriginal(); const mocked = { @@ -152,9 +166,9 @@ describe('useExecutionLifecycle', () => { (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue( Buffer.from('abcdef', 'hex'), ); - vi.mocked(fs.mkdtempSync).mockReturnValue('/tmp/gemini-shell-abcdef'); mockIsBinary.mockReturnValue(false); vi.mocked(fs.existsSync).mockReturnValue(false); + vi.mocked(fs.mkdtempSync).mockReturnValue('/tmp/gemini-shell-abcdef'); mockShellExecutionService.mockImplementation((_cmd, _cwd, callback) => { mockShellOutputCallback = callback; @@ -240,10 +254,11 @@ describe('useExecutionLifecycle', () => { }), ], }); - + const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); + const wrappedCommand = `{ ls -l; }; __code=$?; pwd > "${tmpFile}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), - expect.any(String), + wrappedCommand, + '/test/dir', expect.any(Function), expect.any(Object), false, @@ -254,27 +269,6 @@ describe('useExecutionLifecycle', () => { expect(onExecMock).toHaveBeenCalledWith(expect.any(Promise)); }); - it('should not break heredoc commands ending in a delimiter', async () => { - const { result } = await renderProcessorHook(); - const command = `cat << 'EOF' -hello world -EOF`; - - await act(async () => { - result.current.handleShellCommand(command, new AbortController().signal); - }); - - expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), - expect.any(String), - expect.any(Function), - expect.any(Object), - false, - expect.any(Object), - ); - expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\};/); - }); - it('should pass the config sessionId into shell execution config', async () => { const { result } = await renderProcessorHook(); @@ -371,8 +365,12 @@ EOF`; }); // Verify it's using the non-pty shell + const wrappedCommand = `{ stream; }; __code=$?; pwd > "${path.join( + os.tmpdir(), + 'shell_pwd_abcdef.tmp', + )}"; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pwd > .*\/gemini-shell-.*\/pwd\.tmp/), + wrappedCommand, '/test/dir', expect.any(Function), expect.any(Object), diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 791eeb85d32..051ac4d9456 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -17,7 +17,6 @@ import { const mockPlatform = vi.hoisted(() => vi.fn()); const mockHomedir = vi.hoisted(() => vi.fn()); -const mockTmpdir = vi.hoisted(() => vi.fn().mockReturnValue('/tmp')); const mockShellExecutionService = vi.hoisted(() => vi.fn()); const mockShellBackground = vi.hoisted(() => vi.fn()); @@ -29,84 +28,17 @@ vi.mock('../services/shellExecutionService.js', () => ({ }, })); -vi.mock('node:fs', async (importOriginal) => { - const actual = await importOriginal(); - const mockFs = { - ...actual, - mkdtempSync: vi.fn().mockReturnValue('/tmp/gemini-shell-abcdef'), - mkdirSync: vi.fn(), - existsSync: vi.fn().mockReturnValue(true), - statSync: vi.fn().mockReturnValue({ - isDirectory: () => true, - isFile: () => true, - } as unknown as import('node:fs').Stats), - realpathSync: vi.fn().mockImplementation((p) => p as string), - lstatSync: vi.fn().mockReturnValue({ - isDirectory: () => true, - isFile: () => true, - } as unknown as import('node:fs').Stats), - readFileSync: vi.fn().mockImplementation((p) => { - if (typeof p === 'string' && p.endsWith('pgrep.tmp')) { - return `54321\n54322\n`; - } - return ''; - }), - writeFileSync: vi.fn(), - unlinkSync: vi.fn(), - rmSync: vi.fn(), - promises: { - ...actual.promises, - mkdtemp: vi.fn().mockResolvedValue('/tmp/gemini-shell-abcdef'), - readFile: vi.fn().mockResolvedValue(`54321\n54322\n`), - access: vi.fn().mockResolvedValue(undefined), - unlink: vi.fn().mockResolvedValue(undefined), - rm: vi.fn().mockResolvedValue(undefined), - }, - }; - return { - ...mockFs, - default: mockFs, - }; -}); - -vi.mock('node:fs/promises', async (importOriginal) => { - const actual = await importOriginal(); - const mockFsPromises = { - ...actual, - mkdtemp: vi.fn().mockResolvedValue('/tmp/gemini-shell-abcdef'), - readFile: vi.fn().mockResolvedValue(`54321\n54322\n`), - access: vi.fn().mockResolvedValue(undefined), - unlink: vi.fn().mockResolvedValue(undefined), - rm: vi.fn().mockResolvedValue(undefined), - }; - return { - ...mockFsPromises, - default: mockFsPromises, - }; -}); - vi.mock('node:os', async (importOriginal) => { - const actualOs = await importOriginal(); + const actualOs = await importOriginal(); return { ...actualOs, default: { ...actualOs, platform: mockPlatform, homedir: mockHomedir, - tmpdir: mockTmpdir, }, platform: mockPlatform, homedir: mockHomedir, - tmpdir: mockTmpdir, - }; -}); - -vi.mock('../utils/shell-utils.js', async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - initializeShellParsers: vi.fn().mockResolvedValue(undefined), }; }); vi.mock('crypto'); @@ -121,6 +53,8 @@ import { type ShellExecutionResult, type ShellOutputEvent, } from '../services/shellExecutionService.js'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; import * as path from 'node:path'; import { isSubpath } from '../utils/paths.js'; import * as crypto from 'node:crypto'; @@ -160,23 +94,17 @@ describe('ShellTool', () => { let mockConfig: Config; let mockSandboxManager: SandboxManager; let mockShellOutputCallback: (event: ShellOutputEvent) => void; - let resolveExecutionPromise: ( - result: ShellExecutionResult, - ) => void = () => {}; - let executionPromise: Promise; + let resolveExecutionPromise: (result: ShellExecutionResult) => void; let tempRootDir: string; + let extractedTmpFile: string; beforeEach(() => { vi.clearAllMocks(); - executionPromise = new Promise((resolve) => { - resolveExecutionPromise = resolve; - }); - - tempRootDir = '/tmp/gemini-shell-abcdef'; + tempRootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'shell-test-')); + fs.mkdirSync(path.join(tempRootDir, 'subdir')); mockSandboxManager = new NoopSandboxManager(); - mockConfig = { get config() { return this; @@ -267,21 +195,31 @@ describe('ShellTool', () => { (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue( Buffer.from('abcdef', 'hex'), ); - process.env['ComSpec'] = 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; + extractedTmpFile = ''; + // Capture the output callback to simulate streaming events from the service - mockShellExecutionService.mockImplementation((_cmd, _cwd, callback) => { - mockShellOutputCallback = callback; - executionPromise = new Promise((resolve) => { - resolveExecutionPromise = resolve; - }); - return { - pid: 12345, - result: executionPromise, - }; - }); + mockShellExecutionService.mockImplementation( + ( + cmd: string, + _cwd: string, + callback: (event: ShellOutputEvent) => void, + ) => { + mockShellOutputCallback = callback; + const match = cmd.match(/pgrep -g 0 >([^ ]+)/); + if (match) { + extractedTmpFile = match[1].replace(/['"]/g, ''); // remove any quotes if present + } + return { + pid: 12345, + result: new Promise((resolve) => { + resolveExecutionPromise = resolve; + }), + }; + }, + ); mockShellBackground.mockImplementation(() => { resolveExecutionPromise({ @@ -299,6 +237,9 @@ describe('ShellTool', () => { }); afterEach(() => { + if (fs.existsSync(tempRootDir)) { + fs.rmSync(tempRootDir, { recursive: true, force: true }); + } if (originalComSpec === undefined) { delete process.env['ComSpec']; } else { @@ -363,17 +304,19 @@ describe('ShellTool', () => { }; it('should wrap command on linux and parse pgrep output', async () => { - const command = 'my-command &'; - const invocation = shellTool.build({ command }); + const invocation = shellTool.build({ command: 'my-command &' }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); + // Simulate pgrep output file creation by the shell command + fs.writeFileSync(extractedTmpFile, `54321${os.EOL}54322${os.EOL}`); + resolveShellExecution({ pid: 54321 }); const result = await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + tempRootDir, expect.any(Function), expect.any(AbortSignal), false, @@ -384,52 +327,44 @@ describe('ShellTool', () => { }), ); expect(result.llmContent).toContain('Background PIDs: 54322'); + // The file should be deleted by the tool + expect(fs.existsSync(extractedTmpFile)).toBe(false); }); it('should add a space when command ends with a backslash to prevent escaping newline', async () => { - const command = 'ls\\'; - const invocation = shellTool.build({ command }); + const invocation = shellTool.build({ command: 'ls\\' }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); resolveShellExecution(); await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + tempRootDir, expect.any(Function), expect.any(AbortSignal), false, - expect.objectContaining({ - pager: 'cat', - sanitizationConfig: {}, - sandboxManager: expect.any(Object), - }), + expect.any(Object), ); }); it('should handle trailing comments correctly by placing them on their own line', async () => { - const command = 'ls # comment'; - const invocation = shellTool.build({ command }); + const invocation = shellTool.build({ command: 'ls # comment' }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); resolveShellExecution(); await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + tempRootDir, expect.any(Function), expect.any(AbortSignal), false, - expect.objectContaining({ - pager: 'cat', - sanitizationConfig: {}, - sandboxManager: expect.any(Object), - }), + expect.any(Object), ); }); it('should use the provided absolute directory as cwd', async () => { - const subdir = '/tmp/gemini-shell-abcdef/subdir'; + const subdir = path.join(tempRootDir, 'subdir'); const invocation = shellTool.build({ command: 'ls', dir_path: subdir, @@ -439,8 +374,8 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*\/subdir$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + subdir, expect.any(Function), expect.any(AbortSignal), false, @@ -462,8 +397,8 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*\/subdir$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + path.join(tempRootDir, 'subdir'), expect.any(Function), expect.any(AbortSignal), false, @@ -517,7 +452,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( 'dir', - expect.stringMatching(/\/gemini-shell-.*$/), + tempRootDir, expect.any(Function), expect.any(AbortSignal), false, @@ -541,8 +476,8 @@ EOF`; await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/\/gemini-shell-.*\/pgrep\.tmp/), - expect.stringMatching(/\/gemini-shell-.*$/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + tempRootDir, expect.any(Function), expect.any(AbortSignal), false, @@ -651,7 +586,13 @@ EOF`; it('should clean up the temp file on synchronous execution error', async () => { const error = new Error('sync spawn error'); - mockShellExecutionService.mockImplementation(() => { + mockShellExecutionService.mockImplementation((cmd: string) => { + const match = cmd.match(/pgrep -g 0 >([^ ]+)/); + if (match) { + extractedTmpFile = match[1].replace(/['"]/g, ''); // remove any quotes if present + // Create the temp file before throwing to simulate it being left behind + fs.writeFileSync(extractedTmpFile, ''); + } throw error; }); @@ -659,6 +600,8 @@ EOF`; await expect( invocation.execute({ abortSignal: mockAbortSignal }), ).rejects.toThrow(error); + + expect(fs.existsSync(extractedTmpFile)).toBe(false); }); it('should not log "missing pgrep output" when process is backgrounded', async () => { @@ -1123,7 +1066,10 @@ EOF`; }); it('should suggest proactive permissions for npm commands', async () => { - const homeDir = '/tmp/gemini-shell-abcdef/home'; + const homeDir = path.join(tempRootDir, 'home'); + fs.mkdirSync(homeDir); + fs.mkdirSync(path.join(homeDir, '.npm')); + fs.mkdirSync(path.join(homeDir, '.cache')); mockHomedir.mockReturnValue(homeDir); @@ -1169,10 +1115,15 @@ EOF`; }); it('should NOT consolidate paths into sensitive directories', async () => { - const homeDir = '/tmp/gemini-shell-abcdef/fake_root/home'; + const rootDir = path.join(tempRootDir, 'fake_root'); + const homeDir = path.join(rootDir, 'home'); const user1Dir = path.join(homeDir, 'user1'); const user2Dir = path.join(homeDir, 'user2'); const user3Dir = path.join(homeDir, 'user3'); + fs.mkdirSync(homeDir, { recursive: true }); + fs.mkdirSync(user1Dir); + fs.mkdirSync(user2Dir); + fs.mkdirSync(user3Dir); mockHomedir.mockReturnValue(path.join(homeDir, 'user')); @@ -1226,7 +1177,8 @@ EOF`; }); it('should proactively suggest expansion for npm install in confirmation', async () => { - const homeDir = '/tmp/gemini-shell-abcdef/home'; + const homeDir = path.join(tempRootDir, 'home'); + fs.mkdirSync(homeDir); mockHomedir.mockReturnValue(homeDir); const invocation = shellTool.build({ command: 'npm install' }); @@ -1241,7 +1193,8 @@ EOF`; }); it('should NOT proactively suggest expansion for npm test', async () => { - const homeDir = '/tmp/gemini-shell-abcdef/home'; + const homeDir = path.join(tempRootDir, 'home'); + fs.mkdirSync(homeDir); mockHomedir.mockReturnValue(homeDir); const invocation = shellTool.build({ command: 'npm test' }); From 984a84a90fa3f8a6269f8a505fd2ee467b098189 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 17:19:02 -0400 Subject: [PATCH 11/15] test: fix regex to support Windows path separators in stringMatching --- packages/core/src/tools/shell.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 051ac4d9456..431f3482de3 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -315,7 +315,7 @@ describe('ShellTool', () => { const result = await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -338,7 +338,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -354,7 +354,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -374,7 +374,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), subdir, expect.any(Function), expect.any(AbortSignal), @@ -397,7 +397,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), path.join(tempRootDir, 'subdir'), expect.any(Function), expect.any(AbortSignal), @@ -476,7 +476,7 @@ EOF`; await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*\/pgrep\.tmp/), + expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), From f7ce287dca7363aaa841d2093f84e3395b7577de Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 17:59:57 -0400 Subject: [PATCH 12/15] test: remove obsolete trailing semicolons from shell hook assertions --- .../ui/hooks/useExecutionLifecycle.test.tsx | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index a96c1a43e68..b472114c351 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -16,7 +16,7 @@ import { afterEach, type Mock, } from 'vitest'; -import { NoopSandboxManager } from '@google/gemini-cli-core'; +import { NoopSandboxManager, escapeShellArg } from '@google/gemini-cli-core'; const mockIsBinary = vi.hoisted(() => vi.fn()); const mockShellExecutionService = vi.hoisted(() => vi.fn()); @@ -254,8 +254,9 @@ describe('useExecutionLifecycle', () => { }), ], }); - const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); - const wrappedCommand = `{ ls -l; }; __code=$?; pwd > "${tmpFile}"; exit $__code`; + const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `{\nls -l\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -364,11 +365,9 @@ describe('useExecutionLifecycle', () => { ); }); - // Verify it's using the non-pty shell - const wrappedCommand = `{ stream; }; __code=$?; pwd > "${path.join( - os.tmpdir(), - 'shell_pwd_abcdef.tmp', - )}"; exit $__code`; + const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); + const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); + const wrappedCommand = `{\nstream\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -659,7 +658,7 @@ describe('useExecutionLifecycle', () => { type: 'error', text: 'An unexpected error occurred: Synchronous spawn error', }); - const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); + const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); // Verify that the temporary file was cleaned up expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile); expect(setShellInputFocusedMock).toHaveBeenCalledWith(false); @@ -667,7 +666,7 @@ describe('useExecutionLifecycle', () => { describe('Directory Change Warning', () => { it('should show a warning if the working directory changes', async () => { - const tmpFile = path.join(os.tmpdir(), 'shell_pwd_abcdef.tmp'); + const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.readFileSync).mockReturnValue('/test/dir/new'); // A different directory From 964792d6ed7eebd1905756089be03d1a9dc152ee Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Mon, 20 Apr 2026 11:02:44 -0400 Subject: [PATCH 13/15] fix(shell): trim pgrep output lines to handle potential CRLF carriage returns --- packages/core/src/tools/shell.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 5e743651ee7..d65f1863436 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -641,7 +641,10 @@ export class ShellToolInvocation extends BaseToolInvocation< if (tempFileExists) { const pgrepContent = await fsPromises.readFile(tempFilePath, 'utf8'); - const pgrepLines = pgrepContent.split('\n').filter(Boolean); + const pgrepLines = pgrepContent + .split('\n') + .map((line) => line.trim()) + .filter(Boolean); for (const line of pgrepLines) { if (!/^\d+$/.test(line)) { if ( From bce807062908eee6535aae9d6c4ab07a2d925cdb Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Mon, 20 Apr 2026 11:28:36 -0400 Subject: [PATCH 14/15] fix(shell): remove trailing semicolons to strictly adhere to newline-based wrapping --- packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx | 4 ++-- packages/cli/src/ui/hooks/useExecutionLifecycle.ts | 2 +- packages/core/src/tools/shell.test.ts | 2 +- packages/core/src/tools/shell.ts | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx index b472114c351..410778514ac 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.test.tsx @@ -256,7 +256,7 @@ describe('useExecutionLifecycle', () => { }); const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `{\nls -l\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; + const wrappedCommand = `{\nls -l\n}\n__code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', @@ -367,7 +367,7 @@ describe('useExecutionLifecycle', () => { const tmpFile = path.join('/tmp/gemini-shell-abcdef', 'pwd.tmp'); const escapedTmpFile = escapeShellArg(tmpFile, 'bash'); - const wrappedCommand = `{\nstream\n}; __code=$?; pwd > ${escapedTmpFile}; exit $__code`; + const wrappedCommand = `{\nstream\n}\n__code=$?; pwd > ${escapedTmpFile}; exit $__code`; expect(mockShellExecutionService).toHaveBeenCalledWith( wrappedCommand, '/test/dir', diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 64dbe42a4b4..884ab544de3 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -403,7 +403,7 @@ export const useExecutionLifecycle = ( ); pwdFilePath = path.join(tmpDir, 'pwd.tmp'); const escapedPwdFilePath = escapeShellArg(pwdFilePath, 'bash'); - commandToExecute = `{\n${command}\n}; __code=$?; pwd > ${escapedPwdFilePath}; exit $__code`; + commandToExecute = `{\n${command}\n}\n__code=$?; pwd > ${escapedPwdFilePath}; exit $__code`; } onDebugMessage(`Executing in ${targetDir}: ${commandToExecute}`); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 431f3482de3..9f83b00bb60 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -483,7 +483,7 @@ EOF`; false, expect.any(Object), ); - expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\);/); + expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\)\n/); }); it('should format error messages correctly', async () => { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index d65f1863436..0390f65ee49 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -112,7 +112,7 @@ export class ShellToolInvocation extends BaseToolInvocation< trimmed += ' '; } const escapedTempFilePath = escapeShellArg(tempFilePath, 'bash'); - return `(\n${trimmed}\n); __code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`; + return `(\n${trimmed}\n)\n__code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`; } private getContextualDetails(): string { From 5a76691f36fb810a082c623e62dfd5efc2076a9e Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 21 Apr 2026 12:52:22 -0400 Subject: [PATCH 15/15] fix(core): remove isWindows check for temp dir creation --- packages/core/src/tools/shell.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 0390f65ee49..a2cb44aba08 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -463,10 +463,8 @@ export class ShellToolInvocation extends BaseToolInvocation< const onAbort = () => combinedController.abort(); try { - if (!isWindows) { - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); - tempFilePath = path.join(tempDir, 'pgrep.tmp'); - } + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); + tempFilePath = path.join(tempDir, 'pgrep.tmp'); // pgrep is not available on Windows, so we can't get background PIDs const commandToExecute = this.wrapCommandForPgrep(