diff --git a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts index 68166c84dd..55ef51e68d 100644 --- a/packages/cli/src/ui/hooks/useExecutionLifecycle.ts +++ b/packages/cli/src/ui/hooks/useExecutionLifecycle.ts @@ -21,6 +21,7 @@ import { ExecutionLifecycleService, CoreToolCallStatus, moveToolOutputToFile, + debugLogger, } from '@google/gemini-cli-core'; import { type PartListUnion } from '@google/genai'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; @@ -378,11 +379,6 @@ export const useExecutionLifecycle = ( let cumulativeStdout: string | AnsiOutput = ''; let isBinaryStream = false; let binaryBytesReceived = 0; - let totalBytesWritten = 0; - - const outputFileName = `gemini_shell_output_${crypto.randomBytes(6).toString('hex')}.log`; - const outputFilePath = path.join(os.tmpdir(), outputFileName); - const outputStream = fs.createWriteStream(outputFilePath); const initialToolDisplay: IndividualToolCallDisplay = { callId, @@ -400,7 +396,6 @@ export const useExecutionLifecycle = ( }); let executionPid: number | undefined; - let fullOutputReturned = false; const abortHandler = () => { onDebugMessage( @@ -431,25 +426,13 @@ export const useExecutionLifecycle = ( switch (event.type) { case 'raw_data': - // We rely on 'file_data' for the clean output stream. - break; case 'file_data': - if (!isBinaryStream) { - outputStream.write(event.chunk); - totalBytesWritten += Buffer.byteLength(event.chunk); - } break; case 'data': if (isBinaryStream) break; if (typeof event.chunk === 'string') { if (typeof cumulativeStdout === 'string') { cumulativeStdout += event.chunk; - // Keep a small buffer for the UI to prevent memory spikes and Ink lagging - const MAX_UI_LENGTH = 100000; // 100KB - if (cumulativeStdout.length > MAX_UI_LENGTH) { - cumulativeStdout = - cumulativeStdout.slice(-MAX_UI_LENGTH); - } } else { cumulativeStdout = event.chunk; } @@ -535,9 +518,6 @@ export const useExecutionLifecycle = ( } const result = await resultPromise; - await new Promise((resolve) => { - outputStream.end(resolve); - }); setPendingHistoryItem(null); if (result.backgrounded && result.pid) { @@ -557,10 +537,9 @@ export const useExecutionLifecycle = ( } else { mainContent = result.output.trim() || '(Command produced no output)'; - const threshold = config.getTruncateToolOutputThreshold(); - if (threshold > 0 && totalBytesWritten >= threshold) { + if (result.fullOutputFilePath) { const { outputFile: savedPath } = await moveToolOutputToFile( - outputFilePath, + result.fullOutputFilePath, SHELL_COMMAND_NAME, callId, config.storage.getProjectTempDir(), @@ -575,7 +554,6 @@ export const useExecutionLifecycle = ( warning, ) : `${mainContent}\n\n${warning}`; - fullOutputReturned = true; } } @@ -679,19 +657,17 @@ export const useExecutionLifecycle = ( ); } finally { abortSignal.removeEventListener('abort', abortHandler); - if (!outputStream.closed) { - outputStream.destroy(); - } if (pwdFilePath) { - fs.promises.unlink(pwdFilePath).catch(() => {}); + fs.promises.unlink(pwdFilePath).catch((err) => { + debugLogger.warn( + `Failed to cleanup pwd file: ${pwdFilePath}`, + err, + ); + }); } dispatch({ type: 'SET_ACTIVE_PTY', pid: null }); setShellInputFocused(false); - - if (!fullOutputReturned) { - fs.promises.unlink(outputFilePath).catch(() => {}); - } } }; diff --git a/packages/core/.geminiignore b/packages/core/.geminiignore deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/packages/core/.gitignore b/packages/core/.gitignore deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/packages/core/src/services/executionLifecycleService.ts b/packages/core/src/services/executionLifecycleService.ts index 534b0f4cff..f8aa348b32 100644 --- a/packages/core/src/services/executionLifecycleService.ts +++ b/packages/core/src/services/executionLifecycleService.ts @@ -27,6 +27,7 @@ export interface ExecutionResult { pid: number | undefined; executionMethod: ExecutionMethod; backgrounded?: boolean; + fullOutputFilePath?: string; } export interface ExecutionHandle { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index ef5d7f127b..f0437d52c5 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -33,7 +33,16 @@ const mockIsBinary = vi.hoisted(() => vi.fn()); const mockPlatform = vi.hoisted(() => vi.fn()); const mockHomedir = vi.hoisted(() => vi.fn()); const mockMkdirSync = vi.hoisted(() => vi.fn()); -const mockCreateWriteStream = vi.hoisted(() => vi.fn()); +const mockCreateWriteStream = vi.hoisted(() => + vi.fn().mockReturnValue({ + write: vi.fn(), + end: vi.fn().mockImplementation((cb?: () => void) => { + if (cb) cb(); + }), + destroy: vi.fn(), + closed: false, + }), +); const mockGetPty = vi.hoisted(() => vi.fn()); const mockSerializeTerminalToObject = vi.hoisted(() => vi.fn()); const mockResolveExecutable = vi.hoisted(() => vi.fn()); @@ -92,6 +101,7 @@ vi.mock('node:os', () => ({ default: { platform: mockPlatform, homedir: mockHomedir, + tmpdir: () => '/tmp', constants: { signals: { SIGTERM: 15, @@ -208,6 +218,15 @@ describe('ShellExecutionService', () => { beforeEach(() => { vi.clearAllMocks(); + mockCreateWriteStream.mockReturnValue({ + write: vi.fn(), + end: vi.fn().mockImplementation((cb?: () => void) => { + if (cb) cb(); + }), + destroy: vi.fn(), + closed: false, + on: vi.fn(), + }); ExecutionLifecycleService.resetForTest(); ShellExecutionService.resetForTest(); mockSerializeTerminalToObject.mockReturnValue([]); @@ -621,7 +640,7 @@ describe('ShellExecutionService', () => { }); it('should handle a synchronous spawn error', async () => { - mockGetPty.mockImplementation(() => null); + mockGetPty.mockResolvedValue(null); mockCpSpawn.mockImplementation(() => { throw new Error('Simulated PTY spawn error'); @@ -725,7 +744,13 @@ describe('ShellExecutionService', () => { }); describe('Backgrounding', () => { - let mockWriteStream: { write: Mock; end: Mock; on: Mock }; + let mockWriteStream: { + write: Mock; + end: Mock; + on: Mock; + destroy: Mock; + closed: boolean; + }; let mockBgChildProcess: EventEmitter & Partial; beforeEach(async () => { @@ -733,6 +758,8 @@ describe('ShellExecutionService', () => { write: vi.fn(), end: vi.fn().mockImplementation((cb) => cb?.()), on: vi.fn(), + destroy: vi.fn(), + closed: false, }; mockMkdirSync.mockReturnValue(undefined); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index ba7c33a896..ab9b28afaf 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -12,6 +12,7 @@ import type { Writable } from 'node:stream'; import os from 'node:os'; import fs, { mkdirSync } from 'node:fs'; import path from 'node:path'; +import crypto from 'node:crypto'; import type { IPty } from '@lydell/node-pty'; import { getCachedEncodingForBuffer } from '../utils/systemEncoding.js'; import { @@ -370,32 +371,114 @@ export class ShellExecutionService { shouldUseNodePty: boolean, shellExecutionConfig: ShellExecutionConfig, ): Promise { + const outputFileName = `gemini_shell_output_${crypto.randomBytes(6).toString('hex')}.log`; + const outputFilePath = path.join(os.tmpdir(), outputFileName); + const outputStream = fs.createWriteStream(outputFilePath); + + let isBinaryStream = false; + let totalBytesWritten = 0; + + const interceptedOnOutputEvent = (event: ShellOutputEvent) => { + switch (event.type) { + case 'raw_data': + break; + case 'file_data': + if (!isBinaryStream) { + outputStream.write(event.chunk); + totalBytesWritten += Buffer.byteLength(event.chunk); + } + break; + case 'binary_detected': + case 'binary_progress': + isBinaryStream = true; + break; + default: + break; + } + onOutputEvent(event); + }; + + let handlePromise: Promise; + if (shouldUseNodePty) { - const ptyInfo = await getPty(); - if (ptyInfo) { - try { - return await this.executeWithPty( + handlePromise = getPty().then((ptyInfo) => { + if (ptyInfo) { + return this.executeWithPty( commandToExecute, cwd, - onOutputEvent, + interceptedOnOutputEvent, abortSignal, shellExecutionConfig, ptyInfo, + ).catch(() => + this.childProcessFallback( + commandToExecute, + cwd, + interceptedOnOutputEvent, + abortSignal, + shellExecutionConfig, + shouldUseNodePty, + ), ); - } catch { - // Fallback to child_process } - } + return this.childProcessFallback( + commandToExecute, + cwd, + interceptedOnOutputEvent, + abortSignal, + shellExecutionConfig, + shouldUseNodePty, + ); + }); + } else { + handlePromise = this.childProcessFallback( + commandToExecute, + cwd, + interceptedOnOutputEvent, + abortSignal, + shellExecutionConfig, + shouldUseNodePty, + ); } - return this.childProcessFallback( - commandToExecute, - cwd, - onOutputEvent, - abortSignal, - shellExecutionConfig, - shouldUseNodePty, - ); + const handle = await handlePromise; + + const wrappedResultPromise = handle.result + .then(async (result) => { + await new Promise((resolve) => { + outputStream.end(resolve); + }); + // The threshold logic is handled later by ToolExecutor/caller, so we just return the full file path if anything was written + if ( + totalBytesWritten > 0 && + !result.backgrounded && + !abortSignal.aborted && + !result.error + ) { + return { + ...result, + fullOutputFilePath: outputFilePath, + }; + } else { + if (!outputStream.closed) { + outputStream.destroy(); + } + await fs.promises.unlink(outputFilePath).catch(() => undefined); + return result; + } + }) + .catch(async (err) => { + if (!outputStream.closed) { + outputStream.destroy(); + } + await fs.promises.unlink(outputFilePath).catch(() => undefined); + throw err; + }); + + return { + pid: handle.pid, + result: wrappedResultPromise, + }; } private static appendAndTruncate(