diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 8c075c6071..f7b221c7bf 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -24,6 +24,30 @@ import { export const SHADOW_REPO_AUTHOR_NAME = 'Gemini CLI'; export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com'; +const SHADOW_REPO_UNSAFE_OPTIONS = { + allowUnsafeAlias: true, + allowUnsafeAskPass: true, + allowUnsafeConfigEnvCount: true, + allowUnsafeConfigPaths: true, + allowUnsafeCredentialHelper: true, + allowUnsafeCustomBinary: true, + allowUnsafeDiffExternal: true, + allowUnsafeDiffTextConv: true, + allowUnsafeEditor: true, + allowUnsafeFilter: true, + allowUnsafeFsMonitor: true, + allowUnsafeGitProxy: true, + allowUnsafeGpgProgram: true, + allowUnsafeHooksPath: true, + allowUnsafeMergeDriver: true, + allowUnsafePack: true, + allowUnsafePager: true, + allowUnsafeProtocolOverride: true, + allowUnsafeSshCommand: true, + allowUnsafeTemplateDir: true, +} satisfies NonNullable & + Record<`allowUnsafe${string}`, boolean>; + /** * Common configuration for the shadow Git repository used for checkpointing. * @@ -32,28 +56,7 @@ export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com'; * regardless of the user's local environment (e.g., PAGER, EDITOR, or SSH settings). */ const SHADOW_REPO_GIT_OPTIONS: Partial = { - unsafe: { - allowUnsafeAlias: true, - allowUnsafeAskPass: true, - allowUnsafeConfigEnvCount: true, - allowUnsafeConfigPaths: true, - allowUnsafeCredentialHelper: true, - allowUnsafeCustomBinary: true, - allowUnsafeDiffExternal: true, - allowUnsafeDiffTextConv: true, - allowUnsafeEditor: true, - allowUnsafeFilter: true, - allowUnsafeFsMonitor: true, - allowUnsafeGitProxy: true, - allowUnsafeGpgProgram: true, - allowUnsafeHooksPath: true, - allowUnsafeMergeDriver: true, - allowUnsafePack: true, - allowUnsafePager: true, - allowUnsafeProtocolOverride: true, - allowUnsafeSshCommand: true, - allowUnsafeTemplateDir: true, - }, + unsafe: SHADOW_REPO_UNSAFE_OPTIONS, }; export class GitService { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index e1dd6bdf84..2c29a316f5 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -152,6 +152,7 @@ describe('ShellTool', () => { getGeminiClient: vi.fn().mockReturnValue({}), getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000), getEnableInteractiveShell: vi.fn().mockReturnValue(false), + isInteractiveShellEnabled: vi.fn().mockReturnValue(false), getShellBackgroundCompletionBehavior: vi.fn().mockReturnValue('silent'), getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), getSandboxEnabled: vi.fn().mockReturnValue(false), @@ -213,7 +214,7 @@ describe('ShellTool', () => { callback: (event: ShellOutputEvent) => void, ) => { mockShellOutputCallback = callback; - const match = cmd.match(/pgrep -g 0 >([^ ]+)/); + const match = cmd.match(/_bgpids_file=([^\r\n]+)/); if (match) { extractedTmpFile = match[1].replace(/['"]/g, ''); } @@ -308,19 +309,22 @@ describe('ShellTool', () => { resolveExecutionPromise(fullResult); }; - it('should wrap command on linux and parse pgrep output', async () => { + it('should wrap command on linux and parse background PID output', async () => { const invocation = shellTool.build({ command: 'my-command &' }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); - // Simulate pgrep output file creation by the shell command + // Simulate background PID output file creation by the shell command fs.writeFileSync(extractedTmpFile, `54321${os.EOL}54322${os.EOL}`); resolveShellExecution({ pid: 54321 }); const result = await promise; + const wrappedCommand = mockShellExecutionService.mock.calls[0][0]; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching( + /_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp['"]?\n\(\n {2}trap 'jobs -p > "\$_bgpids_file"' EXIT/, + ), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -331,11 +335,59 @@ describe('ShellTool', () => { sandboxManager: expect.any(Object), }), ); + expect(wrappedCommand).toMatch( + /^_bgpids_file=.*\n\(\n {2}trap 'jobs -p > "\$_bgpids_file"' EXIT\nmy-command &\n\)\n__code=\$\?\nexit \$__code$/, + ); expect(result.llmContent).toContain('Background PIDs: 54322'); // The file should be deleted by the tool expect(fs.existsSync(extractedTmpFile)).toBe(false); }); + it('should preserve exit code and capture background PIDs when command uses explicit exit', async () => { + const invocation = shellTool.build({ + command: "sh -c 'sleep 60 & exit 1'", + }); + const promise = invocation.execute({ abortSignal: mockAbortSignal }); + + fs.writeFileSync(extractedTmpFile, `67890${os.EOL}`); + expect(fs.readFileSync(extractedTmpFile, 'utf8').trim()).toBe('67890'); + + resolveShellExecution({ exitCode: 1, output: '' }); + + const result = await promise; + const wrappedCommand = mockShellExecutionService.mock.calls[0][0]; + + expect(wrappedCommand).toContain( + 'trap \'jobs -p > "$_bgpids_file"\' EXIT', + ); + expect(wrappedCommand).toContain('sleep 60 & exit 1'); + expect(result.llmContent).toContain('Exit Code: 1'); + expect(result.llmContent).toContain('Background PIDs: 67890'); + expect(fs.existsSync(extractedTmpFile)).toBe(false); + }); + + it('should disable PTY execution when interactive shell is unavailable', async () => { + (mockConfig.getEnableInteractiveShell as Mock).mockReturnValue(true); + (mockConfig.isInteractiveShellEnabled as Mock).mockReturnValue(false); + + const invocation = shellTool.build({ command: 'python --version' }); + const promise = invocation.execute({ abortSignal: mockAbortSignal }); + resolveShellExecution(); + + await promise; + + expect(mockShellExecutionService).toHaveBeenCalledWith( + expect.any(String), + tempRootDir, + expect.any(Function), + expect.any(AbortSignal), + false, + expect.objectContaining({ + pager: 'cat', + }), + ); + }); + it('should add a space when command ends with a backslash to prevent escaping newline', async () => { const invocation = shellTool.build({ command: 'ls\\' }); const promise = invocation.execute({ abortSignal: mockAbortSignal }); @@ -343,7 +395,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -359,7 +411,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -379,7 +431,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/), subdir, expect.any(Function), expect.any(AbortSignal), @@ -402,7 +454,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/), path.join(tempRootDir, 'subdir'), expect.any(Function), expect.any(AbortSignal), @@ -481,7 +533,7 @@ EOF`; await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringMatching(/pgrep -g 0 >.*gemini-shell-.*[/\\]pgrep\.tmp/), + expect.stringMatching(/_bgpids_file=.*gemini-shell-.*[/\\]bgpids\.tmp/), tempRootDir, expect.any(Function), expect.any(AbortSignal), @@ -508,7 +560,7 @@ EOF`; const result = await promise; expect(result.llmContent).toContain('Error: wrapped command failed'); - expect(result.llmContent).not.toContain('pgrep'); + expect(result.llmContent).not.toContain('background pid output'); expect(result.display).toEqual( expect.objectContaining({ name: 'Shell', @@ -599,7 +651,7 @@ EOF`; it('should clean up the temp file on synchronous execution error', async () => { const error = new Error('sync spawn error'); mockShellExecutionService.mockImplementation((cmd: string) => { - const match = cmd.match(/pgrep -g 0 >([^ ]+)/); + const match = cmd.match(/_bgpids_file=([^\r\n]+)/); if (match) { extractedTmpFile = match[1].replace(/['"]/g, ''); // remove any quotes if present // Create the temp file before throwing to simulate it being left behind @@ -616,7 +668,7 @@ EOF`; expect(fs.existsSync(extractedTmpFile)).toBe(false); }); - it('should not log "missing pgrep output" when process is backgrounded', async () => { + it('should not log "missing background pid output" when process is backgrounded', async () => { vi.useFakeTimers(); const debugErrorSpy = vi.spyOn(debugLogger, 'error'); @@ -631,7 +683,9 @@ EOF`; await promise; - expect(debugErrorSpy).not.toHaveBeenCalledWith('missing pgrep output'); + expect(debugErrorSpy).not.toHaveBeenCalledWith( + 'missing background pid output', + ); }); describe('Streaming to `updateOutput`', () => { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 13965d94f6..621d77846e 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -108,15 +108,16 @@ export class ShellToolInvocation extends BaseToolInvocation< } /** - * Wraps a command in a subshell `()` to capture background process IDs (PIDs) using pgrep. - * Uses newlines to prevent breaking heredocs or trailing comments. + * Wraps a command in a subshell to capture background process IDs (PIDs) + * using an EXIT trap. Uses newlines to prevent breaking heredocs or trailing + * comments. * * @param command The raw command string to execute. * @param tempFilePath Path to the temporary file where PIDs will be written. * @param isWindows Whether the current platform is Windows (if true, the command is returned as-is). * @returns The wrapped command string. */ - private wrapCommandForPgrep( + private wrapCommandForBackgroundPIDs( command: string, tempFilePath: string, isWindows: boolean, @@ -132,7 +133,7 @@ export class ShellToolInvocation extends BaseToolInvocation< trimmed += ' '; } const escapedTempFilePath = escapeShellArg(tempFilePath, 'bash'); - return `(\n${trimmed}\n)\n__code=$?; pgrep -g 0 >${escapedTempFilePath} 2>&1; exit $__code;`; + return `_bgpids_file=${escapedTempFilePath}\n(\n trap 'jobs -p > "$_bgpids_file"' EXIT\n${trimmed}\n)\n__code=$?\nexit $__code`; } private getContextualDetails(): string { @@ -497,10 +498,10 @@ export class ShellToolInvocation extends BaseToolInvocation< const onAbort = () => combinedController.abort(); try { tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-shell-')); - tempFilePath = path.join(tempDir, 'pgrep.tmp'); + tempFilePath = path.join(tempDir, 'bgpids.tmp'); - // pgrep is not available on Windows, so we can't get background PIDs - const commandToExecute = this.wrapCommandForPgrep( + // Windows shells do not support the POSIX jobs output used here. + const commandToExecute = this.wrapCommandForBackgroundPIDs( strippedCommand, tempFilePath, isWindows, @@ -651,7 +652,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } }, combinedController.signal, - this.context.config.getEnableInteractiveShell(), + this.context.config.isInteractiveShellEnabled(), { ...shellExecutionConfig, sessionId: this.context.config?.getSessionId?.() ?? 'default', @@ -736,12 +737,15 @@ export class ShellToolInvocation extends BaseToolInvocation< } if (tempFileExists) { - const pgrepContent = await fsPromises.readFile(tempFilePath, 'utf8'); - const pgrepLines = pgrepContent + const backgroundPIDContent = await fsPromises.readFile( + tempFilePath, + 'utf8', + ); + const backgroundPIDLines = backgroundPIDContent .split('\n') .map((line) => line.trim()) .filter(Boolean); - for (const line of pgrepLines) { + for (const line of backgroundPIDLines) { if (!/^\d+$/.test(line)) { if ( line.includes('sysmond service not found') || @@ -750,7 +754,7 @@ export class ShellToolInvocation extends BaseToolInvocation< ) { continue; } - debugLogger.error(`pgrep: ${line}`); + debugLogger.error(`background pid output: ${line}`); } const pid = Number(line); if (pid !== result.pid) { @@ -759,7 +763,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } } else { if (!signal.aborted && !result.backgrounded) { - debugLogger.error('missing pgrep output'); + debugLogger.error('missing background pid output'); } } } @@ -1089,7 +1093,7 @@ export class ShellTool extends BaseDeclarativeTool< // Errors are surfaced when parsing commands. }); const definition = getShellDefinition( - context.config.getEnableInteractiveShell(), + context.config.isInteractiveShellEnabled(), context.config.getEnableShellOutputEfficiency(), context.config.getSandboxEnabled(), ); @@ -1139,7 +1143,7 @@ export class ShellTool extends BaseDeclarativeTool< override getSchema(modelId?: string) { const definition = getShellDefinition( - this.context.config.getEnableInteractiveShell(), + this.context.config.isInteractiveShellEnabled(), this.context.config.getEnableShellOutputEfficiency(), this.context.config.getSandboxEnabled(), ); diff --git a/packages/core/src/tools/shell_proactive.test.ts b/packages/core/src/tools/shell_proactive.test.ts index c2327789de..cfc2d9611d 100644 --- a/packages/core/src/tools/shell_proactive.test.ts +++ b/packages/core/src/tools/shell_proactive.test.ts @@ -83,6 +83,7 @@ describe('ShellTool Proactive Expansion', () => { getModeConfig: vi.fn().mockReturnValue({ readonly: false }), }, getEnableInteractiveShell: vi.fn().mockReturnValue(false), + isInteractiveShellEnabled: vi.fn().mockReturnValue(false), getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000), } as unknown as Config;