From 1a2906a8ad6e9cf7a68441c956af91d189eff417 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Fri, 15 Aug 2025 10:27:33 -0700 Subject: [PATCH] Revert #6088 (#6328) --- esbuild.config.js | 9 +- package-lock.json | 116 +------ package.json | 8 - .../ui/hooks/shellCommandProcessor.test.ts | 8 +- .../cli/src/ui/hooks/shellCommandProcessor.ts | 11 +- .../cli/src/ui/hooks/useReactToolScheduler.ts | 5 - .../cli/src/ui/hooks/useToolScheduler.test.ts | 15 +- packages/core/package.json | 12 +- .../core/src/core/coreToolScheduler.test.ts | 6 - packages/core/src/core/coreToolScheduler.ts | 11 +- .../services/shellExecutionService.test.ts | 299 ++++++++++++------ .../src/services/shellExecutionService.ts | 279 ++++++++-------- packages/core/src/tools/shell.test.ts | 34 +- packages/core/src/tools/shell.ts | 25 +- packages/core/src/tools/tools.ts | 13 +- packages/vscode-ide-companion/tsconfig.json | 3 +- 16 files changed, 412 insertions(+), 442 deletions(-) diff --git a/esbuild.config.js b/esbuild.config.js index bf83132f..0cb8e0fa 100644 --- a/esbuild.config.js +++ b/esbuild.config.js @@ -21,14 +21,7 @@ esbuild outfile: 'bundle/gemini.js', platform: 'node', format: 'esm', - external: [ - '@lydell/node-pty', - '@lydell/node-pty-darwin-arm64', - '@lydell/node-pty-darwin-x64', - '@lydell/node-pty-linux-x64', - '@lydell/node-pty-win32-arm64', - '@lydell/node-pty-win32-x64', - ], + external: [], alias: { 'is-in-ci': path.resolve( __dirname, diff --git a/package-lock.json b/package-lock.json index 3f6a3706..a85be05e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,7 +11,6 @@ "packages/*" ], "dependencies": { - "@lydell/node-pty": "1.1.0", "node-fetch": "^3.3.2" }, "bin": { @@ -51,13 +50,6 @@ }, "engines": { "node": ">=20.0.0" - }, - "optionalDependencies": { - "@lydell/node-pty-darwin-arm64": "1.1.0", - "@lydell/node-pty-darwin-x64": "1.1.0", - "@lydell/node-pty-linux-x64": "1.1.0", - "@lydell/node-pty-win32-arm64": "1.1.0", - "@lydell/node-pty-win32-x64": "1.1.0" } }, "node_modules/@alcalzone/ansi-tokenize": { @@ -1463,98 +1455,6 @@ "integrity": "sha512-GaHYm+c0O9MjZRu0ongGBRbinu8gVAMd2UZjji6jVmqKtZluZnptXGWhz1E8j8D2HJ3f/yMxKAUC0b+57wncIw==", "license": "MIT" }, - "node_modules/@lydell/node-pty": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty/-/node-pty-1.1.0.tgz", - "integrity": "sha512-VDD8LtlMTOrPKWMXUAcB9+LTktzuunqrMwkYR1DMRBkS6LQrCt+0/Ws1o2rMml/n3guePpS7cxhHF7Nm5K4iMw==", - "license": "MIT", - "optionalDependencies": { - "@lydell/node-pty-darwin-arm64": "1.1.0", - "@lydell/node-pty-darwin-x64": "1.1.0", - "@lydell/node-pty-linux-arm64": "1.1.0", - "@lydell/node-pty-linux-x64": "1.1.0", - "@lydell/node-pty-win32-arm64": "1.1.0", - "@lydell/node-pty-win32-x64": "1.1.0" - } - }, - "node_modules/@lydell/node-pty-darwin-arm64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-darwin-arm64/-/node-pty-darwin-arm64-1.1.0.tgz", - "integrity": "sha512-7kFD+owAA61qmhJCtoMbqj3Uvff3YHDiU+4on5F2vQdcMI3MuwGi7dM6MkFG/yuzpw8LF2xULpL71tOPUfxs0w==", - "cpu": [ - "arm64" - ], - "license": "MIT", - "optional": true, - "os": [ - "darwin" - ] - }, - "node_modules/@lydell/node-pty-darwin-x64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-darwin-x64/-/node-pty-darwin-x64-1.1.0.tgz", - "integrity": "sha512-XZdvqj5FjAMjH8bdp0YfaZjur5DrCIDD1VYiE9EkkYVMDQqRUPHYV3U8BVEQVT9hYfjmpr7dNaELF2KyISWSNA==", - "cpu": [ - "x64" - ], - "license": "MIT", - "optional": true, - "os": [ - "darwin" - ] - }, - "node_modules/@lydell/node-pty-linux-arm64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-linux-arm64/-/node-pty-linux-arm64-1.1.0.tgz", - "integrity": "sha512-yyDBmalCfHpLiQMT2zyLcqL2Fay4Xy7rIs8GH4dqKLnEviMvPGOK7LADVkKAsbsyXBSISL3Lt1m1MtxhPH6ckg==", - "cpu": [ - "arm64" - ], - "license": "MIT", - "optional": true, - "os": [ - "linux" - ] - }, - "node_modules/@lydell/node-pty-linux-x64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-linux-x64/-/node-pty-linux-x64-1.1.0.tgz", - "integrity": "sha512-NcNqRTD14QT+vXcEuqSSvmWY+0+WUBn2uRE8EN0zKtDpIEr9d+YiFj16Uqds6QfcLCHfZmC+Ls7YzwTaqDnanA==", - "cpu": [ - "x64" - ], - "license": "MIT", - "optional": true, - "os": [ - "linux" - ] - }, - "node_modules/@lydell/node-pty-win32-arm64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-win32-arm64/-/node-pty-win32-arm64-1.1.0.tgz", - "integrity": "sha512-JOMbCou+0fA7d/m97faIIfIU0jOv8sn2OR7tI45u3AmldKoKoLP8zHY6SAvDDnI3fccO1R2HeR1doVjpS7HM0w==", - "cpu": [ - "arm64" - ], - "license": "MIT", - "optional": true, - "os": [ - "win32" - ] - }, - "node_modules/@lydell/node-pty-win32-x64": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/@lydell/node-pty-win32-x64/-/node-pty-win32-x64-1.1.0.tgz", - "integrity": "sha512-3N56BZ+WDFnUMYRtsrr7Ky2mhWGl9xXcyqR6cexfuCqcz9RNWL+KoXRv/nZylY5dYaXkft4JaR1uVu+roiZDAw==", - "cpu": [ - "x64" - ], - "license": "MIT", - "optional": true, - "os": [ - "win32" - ] - }, "node_modules/@modelcontextprotocol/sdk": { "version": "1.15.1", "resolved": "https://registry.npmjs.org/@modelcontextprotocol/sdk/-/sdk-1.15.1.tgz", @@ -3362,12 +3262,6 @@ "url": "https://opencollective.com/vitest" } }, - "node_modules/@xterm/headless": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/@xterm/headless/-/headless-5.5.0.tgz", - "integrity": "sha512-5xXB7kdQlFBP82ViMJTwwEc3gKCLGKR/eoxQm4zge7GPBl86tCdI0IdPJjoKd8mUSFXz5V7i/25sfsEkP4j46g==", - "license": "MIT" - }, "node_modules/accepts": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/accepts/-/accepts-2.0.0.tgz", @@ -12599,7 +12493,6 @@ "version": "0.1.21", "dependencies": { "@google/genai": "1.13.0", - "@lydell/node-pty": "1.1.0", "@modelcontextprotocol/sdk": "^1.11.0", "@opentelemetry/api": "^1.9.0", "@opentelemetry/exporter-logs-otlp-grpc": "^0.52.0", @@ -12609,7 +12502,6 @@ "@opentelemetry/sdk-node": "^0.52.0", "@types/glob": "^8.1.0", "@types/html-to-text": "^9.0.4", - "@xterm/headless": "5.5.0", "ajv": "^8.17.1", "chardet": "^2.1.0", "diff": "^7.0.0", @@ -12628,6 +12520,7 @@ "picomatch": "^4.0.1", "shell-quote": "^1.8.3", "simple-git": "^3.28.0", + "strip-ansi": "^7.1.0", "undici": "^7.10.0", "ws": "^8.18.0" }, @@ -12644,13 +12537,6 @@ }, "engines": { "node": ">=20" - }, - "optionalDependencies": { - "@lydell/node-pty-darwin-arm64": "1.1.0", - "@lydell/node-pty-darwin-x64": "1.1.0", - "@lydell/node-pty-linux-x64": "1.1.0", - "@lydell/node-pty-win32-arm64": "1.1.0", - "@lydell/node-pty-win32-x64": "1.1.0" } }, "packages/core/node_modules/ajv": { diff --git a/package.json b/package.json index 090b723d..8a40681e 100644 --- a/package.json +++ b/package.json @@ -91,14 +91,6 @@ "yargs": "^17.7.2" }, "dependencies": { - "@lydell/node-pty": "1.1.0", "node-fetch": "^3.3.2" - }, - "optionalDependencies": { - "@lydell/node-pty-darwin-arm64": "1.1.0", - "@lydell/node-pty-darwin-x64": "1.1.0", - "@lydell/node-pty-linux-x64": "1.1.0", - "@lydell/node-pty-win32-arm64": "1.1.0", - "@lydell/node-pty-win32-x64": "1.1.0" } } diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts index 49dd9c2a..d5270aba 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts @@ -104,6 +104,8 @@ describe('useShellCommandProcessor', () => { ): ShellExecutionResult => ({ rawOutput: Buffer.from(overrides.output || ''), output: 'Success', + stdout: 'Success', + stderr: '', exitCode: 0, signal: null, error: null, @@ -221,6 +223,7 @@ describe('useShellCommandProcessor', () => { act(() => { mockShellOutputCallback({ type: 'data', + stream: 'stdout', chunk: 'hello', }); }); @@ -231,9 +234,12 @@ describe('useShellCommandProcessor', () => { // Advance time and send another event to trigger the throttled update await act(async () => { await vi.advanceTimersByTimeAsync(OUTPUT_UPDATE_INTERVAL_MS + 1); + }); + act(() => { mockShellOutputCallback({ type: 'data', - chunk: 'hello world', + stream: 'stdout', + chunk: ' world', }); }); diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.ts index 537b21ac..08df0a74 100644 --- a/packages/cli/src/ui/hooks/shellCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/shellCommandProcessor.ts @@ -104,6 +104,7 @@ export const useShellCommandProcessor = ( const execPromise = new Promise((resolve) => { let lastUpdateTime = Date.now(); let cumulativeStdout = ''; + let cumulativeStderr = ''; let isBinaryStream = false; let binaryBytesReceived = 0; @@ -141,7 +142,11 @@ export const useShellCommandProcessor = ( case 'data': // Do not process text data if we've already switched to binary mode. if (isBinaryStream) break; - cumulativeStdout = event.chunk; + if (event.stream === 'stdout') { + cumulativeStdout += event.chunk; + } else { + cumulativeStderr += event.chunk; + } break; case 'binary_detected': isBinaryStream = true; @@ -167,7 +172,9 @@ export const useShellCommandProcessor = ( '[Binary output detected. Halting stream...]'; } } else { - currentDisplayOutput = cumulativeStdout; + currentDisplayOutput = + cumulativeStdout + + (cumulativeStderr ? `\n${cumulativeStderr}` : ''); } // Throttle pending UI updates to avoid excessive re-renders. diff --git a/packages/cli/src/ui/hooks/useReactToolScheduler.ts b/packages/cli/src/ui/hooks/useReactToolScheduler.ts index ce9727b7..93e05387 100644 --- a/packages/cli/src/ui/hooks/useReactToolScheduler.ts +++ b/packages/cli/src/ui/hooks/useReactToolScheduler.ts @@ -62,8 +62,6 @@ export type TrackedToolCall = | TrackedCompletedToolCall | TrackedCancelledToolCall; -import { useTerminalSize } from './useTerminalSize.js'; - export function useReactToolScheduler( onComplete: (tools: CompletedToolCall[]) => Promise, config: Config, @@ -73,7 +71,6 @@ export function useReactToolScheduler( getPreferredEditor: () => EditorType | undefined, onEditorClose: () => void, ): [TrackedToolCall[], ScheduleFn, MarkToolsAsSubmittedFn] { - const terminalSize = useTerminalSize(); const [toolCallsForDisplay, setToolCallsForDisplay] = useState< TrackedToolCall[] >([]); @@ -143,7 +140,6 @@ export function useReactToolScheduler( onToolCallsUpdate: toolCallsUpdateHandler, getPreferredEditor, config, - getTerminalSize: () => terminalSize, onEditorClose, }), [ @@ -152,7 +148,6 @@ export function useReactToolScheduler( allToolCallsCompleteHandler, toolCallsUpdateHandler, getPreferredEditor, - terminalSize, onEditorClose, ], ); diff --git a/packages/cli/src/ui/hooks/useToolScheduler.test.ts b/packages/cli/src/ui/hooks/useToolScheduler.test.ts index c5d968fe..b2071931 100644 --- a/packages/cli/src/ui/hooks/useToolScheduler.test.ts +++ b/packages/cli/src/ui/hooks/useToolScheduler.test.ts @@ -36,13 +36,6 @@ import { HistoryItemToolGroup, } from '../types.js'; -vi.mock('./useTerminalSize', () => ({ - useTerminalSize: () => ({ - columns: 80, - rows: 24, - }), -})); - // Mocks vi.mock('@google/gemini-cli-core', async () => { const actual = await vi.importActual('@google/gemini-cli-core'); @@ -231,8 +224,8 @@ describe('useReactToolScheduler in YOLO Mode', () => { request.args, expect.any(AbortSignal), undefined, - 80, - 24, + undefined, + undefined, ); // Check that onComplete was called with success @@ -383,8 +376,8 @@ describe('useReactToolScheduler', () => { request.args, expect.any(AbortSignal), undefined, - 80, - 24, + undefined, + undefined, ); expect(onComplete).toHaveBeenCalledWith([ expect.objectContaining({ diff --git a/packages/core/package.json b/packages/core/package.json index d3c652ac..6f670f2c 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -48,17 +48,9 @@ "picomatch": "^4.0.1", "shell-quote": "^1.8.3", "simple-git": "^3.28.0", + "strip-ansi": "^7.1.0", "undici": "^7.10.0", - "ws": "^8.18.0", - "@lydell/node-pty": "1.1.0", - "@xterm/headless": "5.5.0" - }, - "optionalDependencies": { - "@lydell/node-pty-darwin-arm64": "1.1.0", - "@lydell/node-pty-darwin-x64": "1.1.0", - "@lydell/node-pty-linux-x64": "1.1.0", - "@lydell/node-pty-win32-arm64": "1.1.0", - "@lydell/node-pty-win32-x64": "1.1.0" + "ws": "^8.18.0" }, "devDependencies": { "@google/gemini-cli-test-utils": "file:../test-utils", diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 6ba85b04..5cf49350 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -61,7 +61,6 @@ describe('CoreToolScheduler', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); @@ -118,7 +117,6 @@ describe('CoreToolScheduler with payload', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); @@ -415,7 +413,6 @@ describe('CoreToolScheduler edit cancellation', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); @@ -504,7 +501,6 @@ describe('CoreToolScheduler YOLO mode', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); @@ -590,7 +586,6 @@ describe('CoreToolScheduler request queueing', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); @@ -700,7 +695,6 @@ describe('CoreToolScheduler request queueing', () => { onAllToolCallsComplete, onToolCallsUpdate, getPreferredEditor: () => 'vscode', - getTerminalSize: () => ({ columns: 80, rows: 24 }), onEditorClose: vi.fn(), }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index bccb724a..0d442f9d 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -232,7 +232,6 @@ interface CoreToolSchedulerOptions { onToolCallsUpdate?: ToolCallsUpdateHandler; getPreferredEditor: () => EditorType | undefined; config: Config; - getTerminalSize: () => { columns: number; rows: number }; onEditorClose: () => void; } @@ -244,7 +243,6 @@ export class CoreToolScheduler { private onToolCallsUpdate?: ToolCallsUpdateHandler; private getPreferredEditor: () => EditorType | undefined; private config: Config; - private getTerminalSize: () => { columns: number; rows: number }; private onEditorClose: () => void; private isFinalizingToolCalls = false; private isScheduling = false; @@ -262,7 +260,6 @@ export class CoreToolScheduler { this.onAllToolCallsComplete = options.onAllToolCallsComplete; this.onToolCallsUpdate = options.onToolCallsUpdate; this.getPreferredEditor = options.getPreferredEditor; - this.getTerminalSize = options.getTerminalSize; this.onEditorClose = options.onEditorClose; } @@ -831,14 +828,8 @@ export class CoreToolScheduler { } : undefined; - const terminalSize = this.getTerminalSize(); invocation - .execute( - signal, - liveOutputCallback, - terminalSize.columns, - terminalSize.rows, - ) + .execute(signal, liveOutputCallback) .then(async (toolResult: ToolResult) => { if (signal.aborted) { this.setStatusInternal( diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 06092bd9..2fe51a5e 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -5,12 +5,14 @@ */ import { vi, describe, it, expect, beforeEach, type Mock } from 'vitest'; -const mockPtySpawn = vi.hoisted(() => vi.fn()); -vi.mock('@lydell/node-pty', () => ({ - spawn: mockPtySpawn, +const mockSpawn = vi.hoisted(() => vi.fn()); +vi.mock('child_process', () => ({ + spawn: mockSpawn, })); import EventEmitter from 'events'; +import { Readable } from 'stream'; +import { type ChildProcess } from 'child_process'; import { ShellExecutionService, ShellOutputEvent, @@ -29,13 +31,12 @@ vi.mock('os', () => ({ platform: mockPlatform, })); +const mockProcessKill = vi + .spyOn(process, 'kill') + .mockImplementation(() => true); + describe('ShellExecutionService', () => { - let mockPtyProcess: EventEmitter & { - pid: number; - kill: Mock; - onData: Mock; - onExit: Mock; - }; + let mockChildProcess: EventEmitter & Partial; let onOutputEventMock: Mock<(event: ShellOutputEvent) => void>; beforeEach(() => { @@ -46,27 +47,26 @@ describe('ShellExecutionService', () => { onOutputEventMock = vi.fn(); - mockPtyProcess = new EventEmitter() as EventEmitter & { - pid: number; - kill: Mock; - onData: Mock; - onExit: Mock; - }; - mockPtyProcess.pid = 12345; - mockPtyProcess.kill = vi.fn(); - mockPtyProcess.onData = vi.fn(); - mockPtyProcess.onExit = vi.fn(); + mockChildProcess = new EventEmitter() as EventEmitter & + Partial; + // FIX: Cast simple EventEmitters to the expected stream type. + mockChildProcess.stdout = new EventEmitter() as Readable; + mockChildProcess.stderr = new EventEmitter() as Readable; + mockChildProcess.kill = vi.fn(); - mockPtySpawn.mockReturnValue(mockPtyProcess); + // FIX: Use Object.defineProperty to set the readonly 'pid' property. + Object.defineProperty(mockChildProcess, 'pid', { + value: 12345, + configurable: true, + }); + + mockSpawn.mockReturnValue(mockChildProcess); }); // Helper function to run a standard execution simulation const simulateExecution = async ( command: string, - simulation: ( - ptyProcess: typeof mockPtyProcess, - ac: AbortController, - ) => void, + simulation: (cp: typeof mockChildProcess, ac: AbortController) => void, ) => { const abortController = new AbortController(); const handle = ShellExecutionService.execute( @@ -77,123 +77,215 @@ describe('ShellExecutionService', () => { ); await new Promise((resolve) => setImmediate(resolve)); - simulation(mockPtyProcess, abortController); + simulation(mockChildProcess, abortController); const result = await handle.result; return { result, handle, abortController }; }; describe('Successful Execution', () => { - it('should execute a command and capture output', async () => { - const { result, handle } = await simulateExecution('ls -l', (pty) => { - pty.onData.mock.calls[0][0]('file1.txt\n'); - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + it('should execute a command and capture stdout and stderr', async () => { + const { result, handle } = await simulateExecution('ls -l', (cp) => { + cp.stdout?.emit('data', Buffer.from('file1.txt\n')); + cp.stderr?.emit('data', Buffer.from('a warning')); + cp.emit('exit', 0, null); }); - expect(mockPtySpawn).toHaveBeenCalledWith( - 'bash', - ['-c', 'ls -l'], - expect.any(Object), + expect(mockSpawn).toHaveBeenCalledWith( + 'ls -l', + [], + expect.objectContaining({ shell: 'bash' }), ); expect(result.exitCode).toBe(0); expect(result.signal).toBeNull(); expect(result.error).toBeNull(); expect(result.aborted).toBe(false); - expect(result.output).toBe('file1.txt'); + expect(result.stdout).toBe('file1.txt\n'); + expect(result.stderr).toBe('a warning'); + expect(result.output).toBe('file1.txt\n\na warning'); expect(handle.pid).toBe(12345); expect(onOutputEventMock).toHaveBeenCalledWith({ type: 'data', - chunk: 'file1.txt', + stream: 'stdout', + chunk: 'file1.txt\n', + }); + expect(onOutputEventMock).toHaveBeenCalledWith({ + type: 'data', + stream: 'stderr', + chunk: 'a warning', }); }); it('should strip ANSI codes from output', async () => { - const { result } = await simulateExecution('ls --color=auto', (pty) => { - pty.onData.mock.calls[0][0]('a\u001b[31mred\u001b[0mword'); - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + const { result } = await simulateExecution('ls --color=auto', (cp) => { + cp.stdout?.emit('data', Buffer.from('a\u001b[31mred\u001b[0mword')); + cp.emit('exit', 0, null); }); - expect(result.output).toBe('aredword'); + expect(result.stdout).toBe('aredword'); expect(onOutputEventMock).toHaveBeenCalledWith({ type: 'data', + stream: 'stdout', chunk: 'aredword', }); }); it('should correctly decode multi-byte characters split across chunks', async () => { - const { result } = await simulateExecution('echo "你好"', (pty) => { - const multiByteChar = '你好'; - pty.onData.mock.calls[0][0](multiByteChar.slice(0, 1)); - pty.onData.mock.calls[0][0](multiByteChar.slice(1)); - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + const { result } = await simulateExecution('echo "你好"', (cp) => { + const multiByteChar = Buffer.from('你好', 'utf-8'); + cp.stdout?.emit('data', multiByteChar.slice(0, 2)); + cp.stdout?.emit('data', multiByteChar.slice(2)); + cp.emit('exit', 0, null); }); - expect(result.output).toBe('你好'); + expect(result.stdout).toBe('你好'); }); it('should handle commands with no output', async () => { - const { result } = await simulateExecution('touch file', (pty) => { - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + const { result } = await simulateExecution('touch file', (cp) => { + cp.emit('exit', 0, null); }); + expect(result.stdout).toBe(''); + expect(result.stderr).toBe(''); expect(result.output).toBe(''); expect(onOutputEventMock).not.toHaveBeenCalled(); }); }); describe('Failed Execution', () => { - it('should capture a non-zero exit code', async () => { - const { result } = await simulateExecution('a-bad-command', (pty) => { - pty.onData.mock.calls[0][0]('command not found'); - pty.onExit.mock.calls[0][0]({ exitCode: 127, signal: null }); + it('should capture a non-zero exit code and format output correctly', async () => { + const { result } = await simulateExecution('a-bad-command', (cp) => { + cp.stderr?.emit('data', Buffer.from('command not found')); + cp.emit('exit', 127, null); }); expect(result.exitCode).toBe(127); - expect(result.output).toBe('command not found'); + expect(result.stderr).toBe('command not found'); + expect(result.stdout).toBe(''); + expect(result.output).toBe('\ncommand not found'); expect(result.error).toBeNull(); }); it('should capture a termination signal', async () => { - const { result } = await simulateExecution('long-process', (pty) => { - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: 15 }); + const { result } = await simulateExecution('long-process', (cp) => { + cp.emit('exit', null, 'SIGTERM'); }); - expect(result.exitCode).toBe(0); - expect(result.signal).toBe(15); + expect(result.exitCode).toBeNull(); + expect(result.signal).toBe('SIGTERM'); }); - it('should handle a synchronous spawn error', async () => { - const spawnError = new Error('spawn ENOENT'); - mockPtySpawn.mockImplementation(() => { - throw spawnError; + it('should handle a spawn error', async () => { + const spawnError = new Error('spawn EACCES'); + const { result } = await simulateExecution('protected-cmd', (cp) => { + cp.emit('error', spawnError); + cp.emit('exit', 1, null); }); - const handle = ShellExecutionService.execute( - 'any-command', - '/test/dir', - onOutputEventMock, - new AbortController().signal, - ); - const result = await handle.result; - expect(result.error).toBe(spawnError); expect(result.exitCode).toBe(1); - expect(result.output).toBe(''); - expect(handle.pid).toBeUndefined(); + }); + + it('handles errors that do not fire the exit event', async () => { + const error = new Error('spawn abc ENOENT'); + const { result } = await simulateExecution('touch cat.jpg', (cp) => { + cp.emit('error', error); // No exit event is fired. + }); + + expect(result.error).toBe(error); + expect(result.exitCode).toBe(1); }); }); describe('Aborting Commands', () => { - it('should abort a running process and set the aborted flag', async () => { - const { result } = await simulateExecution( - 'sleep 10', - (pty, abortController) => { - abortController.abort(); - pty.onExit.mock.calls[0][0]({ exitCode: 1, signal: null }); - }, + describe.each([ + { + platform: 'linux', + expectedSignal: 'SIGTERM', + expectedExit: { signal: 'SIGKILL' as const }, + }, + { + platform: 'win32', + expectedCommand: 'taskkill', + expectedExit: { code: 1 }, + }, + ])( + 'on $platform', + ({ platform, expectedSignal, expectedCommand, expectedExit }) => { + it('should abort a running process and set the aborted flag', async () => { + mockPlatform.mockReturnValue(platform); + + const { result } = await simulateExecution( + 'sleep 10', + (cp, abortController) => { + abortController.abort(); + if (expectedExit.signal) + cp.emit('exit', null, expectedExit.signal); + if (typeof expectedExit.code === 'number') + cp.emit('exit', expectedExit.code, null); + }, + ); + + expect(result.aborted).toBe(true); + + if (platform === 'linux') { + expect(mockProcessKill).toHaveBeenCalledWith( + -mockChildProcess.pid!, + expectedSignal, + ); + } else { + expect(mockSpawn).toHaveBeenCalledWith(expectedCommand, [ + '/pid', + String(mockChildProcess.pid), + '/f', + '/t', + ]); + } + }); + }, + ); + + it('should gracefully attempt SIGKILL on linux if SIGTERM fails', async () => { + mockPlatform.mockReturnValue('linux'); + vi.useFakeTimers(); + + // Don't await the result inside the simulation block for this specific test. + // We need to control the timeline manually. + const abortController = new AbortController(); + const handle = ShellExecutionService.execute( + 'unresponsive_process', + '/test/dir', + onOutputEventMock, + abortController.signal, ); + abortController.abort(); + + // Check the first kill signal + expect(mockProcessKill).toHaveBeenCalledWith( + -mockChildProcess.pid!, + 'SIGTERM', + ); + + // Now, advance time past the timeout + await vi.advanceTimersByTimeAsync(250); + + // Check the second kill signal + expect(mockProcessKill).toHaveBeenCalledWith( + -mockChildProcess.pid!, + 'SIGKILL', + ); + + // Finally, simulate the process exiting and await the result + mockChildProcess.emit('exit', null, 'SIGKILL'); + const result = await handle.result; + + vi.useRealTimers(); + expect(result.aborted).toBe(true); - expect(mockPtyProcess.kill).toHaveBeenCalled(); + expect(result.signal).toBe('SIGKILL'); + // The individual kill calls were already asserted above. + expect(mockProcessKill).toHaveBeenCalledTimes(2); }); }); @@ -203,10 +295,10 @@ describe('ShellExecutionService', () => { const binaryChunk1 = Buffer.from([0x89, 0x50, 0x4e, 0x47]); const binaryChunk2 = Buffer.from([0x0d, 0x0a, 0x1a, 0x0a]); - const { result } = await simulateExecution('cat image.png', (pty) => { - pty.onData.mock.calls[0][0](binaryChunk1); - pty.onData.mock.calls[0][0](binaryChunk2); - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + const { result } = await simulateExecution('cat image.png', (cp) => { + cp.stdout?.emit('data', binaryChunk1); + cp.stdout?.emit('data', binaryChunk2); + cp.emit('exit', 0, null); }); expect(result.rawOutput).toEqual( @@ -229,13 +321,14 @@ describe('ShellExecutionService', () => { it('should not emit data events after binary is detected', async () => { mockIsBinary.mockImplementation((buffer) => buffer.includes(0x00)); - await simulateExecution('cat mixed_file', (pty) => { - pty.onData.mock.calls[0][0](Buffer.from('some text')); - pty.onData.mock.calls[0][0](Buffer.from([0x00, 0x01, 0x02])); - pty.onData.mock.calls[0][0](Buffer.from('more text')); - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + await simulateExecution('cat mixed_file', (cp) => { + cp.stdout?.emit('data', Buffer.from('some text')); + cp.stdout?.emit('data', Buffer.from([0x00, 0x01, 0x02])); + cp.stdout?.emit('data', Buffer.from('more text')); + cp.emit('exit', 0, null); }); + // FIX: Provide explicit type for the 'call' parameter in the map function. const eventTypes = onOutputEventMock.mock.calls.map( (call: [ShellOutputEvent]) => call[0].type, ); @@ -251,27 +344,31 @@ describe('ShellExecutionService', () => { describe('Platform-Specific Behavior', () => { it('should use cmd.exe on Windows', async () => { mockPlatform.mockReturnValue('win32'); - await simulateExecution('dir "foo bar"', (pty) => - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }), + await simulateExecution('dir "foo bar"', (cp) => + cp.emit('exit', 0, null), ); - expect(mockPtySpawn).toHaveBeenCalledWith( - 'cmd.exe', - ['/c', 'dir "foo bar"'], - expect.any(Object), + expect(mockSpawn).toHaveBeenCalledWith( + 'dir "foo bar"', + [], + expect.objectContaining({ + shell: true, + detached: false, + }), ); }); - it('should use bash on Linux', async () => { + it('should use bash and detached process group on Linux', async () => { mockPlatform.mockReturnValue('linux'); - await simulateExecution('ls "foo bar"', (pty) => - pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }), - ); + await simulateExecution('ls "foo bar"', (cp) => cp.emit('exit', 0, null)); - expect(mockPtySpawn).toHaveBeenCalledWith( - 'bash', - ['-c', 'ls "foo bar"'], - expect.any(Object), + expect(mockSpawn).toHaveBeenCalledWith( + 'ls "foo bar"', + [], + expect.objectContaining({ + shell: 'bash', + detached: true, + }), ); }); }); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 26d884b4..3749fcf6 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -4,35 +4,29 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as pty from '@lydell/node-pty'; +import { spawn } from 'child_process'; import { TextDecoder } from 'util'; import os from 'os'; +import stripAnsi from 'strip-ansi'; import { getCachedEncodingForBuffer } from '../utils/systemEncoding.js'; import { isBinary } from '../utils/textUtils.js'; -import pkg from '@xterm/headless'; -const { Terminal } = pkg; -// @ts-expect-error getFullText is not a public API. -const getFullText = (terminal: Terminal) => { - const buffer = terminal.buffer.active; - const lines: string[] = []; - for (let i = 0; i < buffer.length; i++) { - const line = buffer.getLine(i); - lines.push(line ? line.translateToString(true) : ''); - } - return lines.join('\n').trim(); -}; +const SIGKILL_TIMEOUT_MS = 200; /** A structured result from a shell command execution. */ export interface ShellExecutionResult { /** The raw, unprocessed output buffer. */ rawOutput: Buffer; - /** The combined, decoded output as a string. */ + /** The combined, decoded stdout and stderr as a string. */ output: string; + /** The decoded stdout as a string. */ + stdout: string; + /** The decoded stderr as a string. */ + stderr: string; /** The process exit code, or null if terminated by a signal. */ exitCode: number | null; /** The signal that terminated the process, if any. */ - signal: number | null; + signal: NodeJS.Signals | null; /** An error object if the process failed to spawn. */ error: Error | null; /** A boolean indicating if the command was aborted by the user. */ @@ -56,6 +50,8 @@ export type ShellOutputEvent = | { /** The event contains a chunk of output data. */ type: 'data'; + /** The stream from which the data originated. */ + stream: 'stdout' | 'stderr'; /** The decoded string chunk. */ chunk: string; } @@ -77,7 +73,7 @@ export type ShellOutputEvent = */ export class ShellExecutionService { /** - * Executes a shell command using `node-pty`, capturing all output and lifecycle events. + * Executes a shell command using `spawn`, capturing all output and lifecycle events. * * @param commandToExecute The exact command string to run. * @param cwd The working directory to execute the command in. @@ -91,150 +87,167 @@ export class ShellExecutionService { cwd: string, onOutputEvent: (event: ShellOutputEvent) => void, abortSignal: AbortSignal, - terminalColumns?: number, - terminalRows?: number, ): ShellExecutionHandle { const isWindows = os.platform() === 'win32'; - const shell = isWindows ? 'cmd.exe' : 'bash'; - const args = isWindows - ? ['/c', commandToExecute] - : ['-c', commandToExecute]; - let ptyProcess; - try { - ptyProcess = pty.spawn(shell, args, { - cwd, - name: 'xterm-color', - cols: terminalColumns ?? 200, - rows: terminalRows ?? 20, - env: { - ...process.env, - GEMINI_CLI: '1', - }, - handleFlowControl: true, - }); - } catch (e) { - const error = e as Error; - return { - pid: undefined, - result: Promise.resolve({ - rawOutput: Buffer.from(''), - output: '', - exitCode: 1, - signal: null, - error, - aborted: false, - pid: undefined, - }), - }; - } + const child = spawn(commandToExecute, [], { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + // Use bash unless in Windows (since it doesn't support bash). + // For windows, just use the default. + shell: isWindows ? true : 'bash', + // Use process groups on non-Windows for robust killing. + // Windows process termination is handled by `taskkill /t`. + detached: !isWindows, + env: { + ...process.env, + GEMINI_CLI: '1', + }, + }); const result = new Promise((resolve) => { - const headlessTerminal = new Terminal({ - allowProposedApi: true, - cols: terminalColumns ?? 200, - rows: terminalRows ?? 20, - }); - let processingChain = Promise.resolve(); - let decoder: TextDecoder | null = null; - let output = ''; + // Use decoders to handle multi-byte characters safely (for streaming output). + let stdoutDecoder: TextDecoder | null = null; + let stderrDecoder: TextDecoder | null = null; + + let stdout = ''; + let stderr = ''; const outputChunks: Buffer[] = []; - const error: Error | null = null; + let error: Error | null = null; let exited = false; let isStreamingRawContent = true; const MAX_SNIFF_SIZE = 4096; let sniffedBytes = 0; - const handleOutput = (data: Buffer) => { - // NOTE: The migration from `child_process` to `node-pty` means we - // no longer have separate `stdout` and `stderr` streams. The `data` - // buffer contains the merged output. If a drop in LLM quality is - // observed after this change, we may need to revisit this and - // explore ways to re-introduce that distinction. - processingChain = processingChain.then( - () => - new Promise((resolve) => { - if (!decoder) { - const encoding = getCachedEncodingForBuffer(data); - try { - decoder = new TextDecoder(encoding); - } catch { - decoder = new TextDecoder('utf-8'); - } - } + const handleOutput = (data: Buffer, stream: 'stdout' | 'stderr') => { + if (!stdoutDecoder || !stderrDecoder) { + const encoding = getCachedEncodingForBuffer(data); + try { + stdoutDecoder = new TextDecoder(encoding); + stderrDecoder = new TextDecoder(encoding); + } catch { + // If the encoding is not supported, fall back to utf-8. + // This can happen on some platforms for certain encodings like 'utf-32le'. + stdoutDecoder = new TextDecoder('utf-8'); + stderrDecoder = new TextDecoder('utf-8'); + } + } - outputChunks.push(data); + outputChunks.push(data); - // First, check if we need to switch to binary mode. - if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { - const sniffBuffer = Buffer.concat(outputChunks.slice(0, 20)); - sniffedBytes = sniffBuffer.length; + // Binary detection logic. This only runs until we've made a determination. + if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { + const sniffBuffer = Buffer.concat(outputChunks.slice(0, 20)); + sniffedBytes = sniffBuffer.length; - if (isBinary(sniffBuffer)) { - isStreamingRawContent = false; - onOutputEvent({ type: 'binary_detected' }); - } - } + if (isBinary(sniffBuffer)) { + // Change state to stop streaming raw content. + isStreamingRawContent = false; + onOutputEvent({ type: 'binary_detected' }); + } + } - // Now, based on the *current* state, either process as text or binary. - if (isStreamingRawContent) { - const decodedChunk = decoder.decode(data, { stream: true }); - headlessTerminal.write(decodedChunk, () => { - const newStrippedOutput = getFullText(headlessTerminal); - output = newStrippedOutput; - onOutputEvent({ type: 'data', chunk: newStrippedOutput }); - resolve(); - }); - } else { - // Once in binary mode, we only emit progress events. - const totalBytes = outputChunks.reduce( - (sum, chunk) => sum + chunk.length, - 0, - ); - onOutputEvent({ - type: 'binary_progress', - bytesReceived: totalBytes, - }); - resolve(); - } - }), - ); + const decodedChunk = + stream === 'stdout' + ? stdoutDecoder.decode(data, { stream: true }) + : stderrDecoder.decode(data, { stream: true }); + const strippedChunk = stripAnsi(decodedChunk); + + if (stream === 'stdout') { + stdout += strippedChunk; + } else { + stderr += strippedChunk; + } + + if (isStreamingRawContent) { + onOutputEvent({ type: 'data', stream, chunk: strippedChunk }); + } else { + const totalBytes = outputChunks.reduce( + (sum, chunk) => sum + chunk.length, + 0, + ); + onOutputEvent({ type: 'binary_progress', bytesReceived: totalBytes }); + } }; - ptyProcess.onData((data) => { - const bufferData = Buffer.from(data, 'utf-8'); - handleOutput(bufferData); - }); - - ptyProcess.onExit(({ exitCode, signal }) => { - exited = true; - abortSignal.removeEventListener('abort', abortHandler); - - processingChain.then(() => { - const finalBuffer = Buffer.concat(outputChunks); - - resolve({ - rawOutput: finalBuffer, - output, - exitCode, - signal: signal ?? null, - error, - aborted: abortSignal.aborted, - pid: ptyProcess.pid, - }); + child.stdout.on('data', (data) => handleOutput(data, 'stdout')); + child.stderr.on('data', (data) => handleOutput(data, 'stderr')); + child.on('error', (err) => { + const { stdout, stderr, finalBuffer } = cleanup(); + error = err; + resolve({ + error, + stdout, + stderr, + rawOutput: finalBuffer, + output: stdout + (stderr ? `\n${stderr}` : ''), + exitCode: 1, + signal: null, + aborted: false, + pid: child.pid, }); }); const abortHandler = async () => { - if (ptyProcess.pid && !exited) { - ptyProcess.kill('SIGHUP'); + if (child.pid && !exited) { + if (isWindows) { + spawn('taskkill', ['/pid', child.pid.toString(), '/f', '/t']); + } else { + try { + // Kill the entire process group (negative PID). + // SIGTERM first, then SIGKILL if it doesn't die. + process.kill(-child.pid, 'SIGTERM'); + await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)); + if (!exited) { + process.kill(-child.pid, 'SIGKILL'); + } + } catch (_e) { + // Fall back to killing just the main process if group kill fails. + if (!exited) child.kill('SIGKILL'); + } + } } }; abortSignal.addEventListener('abort', abortHandler, { once: true }); + + child.on('exit', (code: number, signal: NodeJS.Signals) => { + const { stdout, stderr, finalBuffer } = cleanup(); + + resolve({ + rawOutput: finalBuffer, + output: stdout + (stderr ? `\n${stderr}` : ''), + stdout, + stderr, + exitCode: code, + signal, + error, + aborted: abortSignal.aborted, + pid: child.pid, + }); + }); + + /** + * Cleans up a process (and it's accompanying state) that is exiting or + * erroring and returns output formatted output buffers and strings + */ + function cleanup() { + exited = true; + abortSignal.removeEventListener('abort', abortHandler); + if (stdoutDecoder) { + stdout += stripAnsi(stdoutDecoder.decode()); + } + if (stderrDecoder) { + stderr += stripAnsi(stderrDecoder.decode()); + } + + const finalBuffer = Buffer.concat(outputChunks); + + return { stdout, stderr, finalBuffer }; + } }); - return { pid: ptyProcess.pid, result }; + return { pid: child.pid, result }; } } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index c968af26..96ff49a1 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -66,6 +66,7 @@ describe('ShellTool', () => { Buffer.from('abcdef', 'hex'), ); + // Capture the output callback to simulate streaming events from the service mockShellExecutionService.mockImplementation((_cmd, _cwd, callback) => { mockShellOutputCallback = callback; return { @@ -122,6 +123,8 @@ describe('ShellTool', () => { const fullResult: ShellExecutionResult = { rawOutput: Buffer.from(result.output || ''), output: 'Success', + stdout: 'Success', + stderr: '', exitCode: 0, signal: null, error: null, @@ -138,7 +141,7 @@ describe('ShellTool', () => { resolveShellExecution({ pid: 54321 }); vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.readFileSync).mockReturnValue('54321\n54322\n'); + vi.mocked(fs.readFileSync).mockReturnValue('54321\n54322\n'); // Service PID and background PID const result = await promise; @@ -149,8 +152,6 @@ describe('ShellTool', () => { expect.any(String), expect.any(Function), mockAbortSignal, - undefined, - undefined, ); expect(result.llmContent).toContain('Background PIDs: 54322'); expect(vi.mocked(fs.unlinkSync)).toHaveBeenCalledWith(tmpFile); @@ -163,6 +164,8 @@ describe('ShellTool', () => { resolveShellExecution({ rawOutput: Buffer.from(''), output: '', + stdout: '', + stderr: '', exitCode: 0, signal: null, error: null, @@ -175,8 +178,6 @@ describe('ShellTool', () => { expect.any(String), expect.any(Function), mockAbortSignal, - undefined, - undefined, ); }); @@ -188,13 +189,16 @@ describe('ShellTool', () => { error, exitCode: 1, output: 'err', + stderr: 'err', rawOutput: Buffer.from('err'), + stdout: '', signal: null, aborted: false, pid: 12345, }); const result = await promise; + // The final llmContent should contain the user's command, not the wrapper expect(result.llmContent).toContain('Error: wrapped command failed'); expect(result.llmContent).not.toContain('pgrep'); }); @@ -227,6 +231,8 @@ describe('ShellTool', () => { resolveExecutionPromise({ output: 'long output', rawOutput: Buffer.from('long output'), + stdout: 'long output', + stderr: '', exitCode: 0, signal: null, error: null, @@ -251,7 +257,7 @@ describe('ShellTool', () => { mockShellExecutionService.mockImplementation(() => { throw error; }); - vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.existsSync).mockReturnValue(true); // Pretend the file exists const invocation = shellTool.build({ command: 'a-command' }); await expect(invocation.execute(mockAbortSignal)).rejects.toThrow(error); @@ -274,26 +280,33 @@ describe('ShellTool', () => { const invocation = shellTool.build({ command: 'stream' }); const promise = invocation.execute(mockAbortSignal, updateOutputMock); + // First chunk, should be throttled. mockShellOutputCallback({ type: 'data', + stream: 'stdout', chunk: 'hello ', }); expect(updateOutputMock).not.toHaveBeenCalled(); + // Advance time past the throttle interval. await vi.advanceTimersByTimeAsync(OUTPUT_UPDATE_INTERVAL_MS + 1); + // Send a second chunk. THIS event triggers the update with the CUMULATIVE content. mockShellOutputCallback({ type: 'data', - chunk: 'hello world', + stream: 'stderr', + chunk: 'world', }); // It should have been called once now with the combined output. expect(updateOutputMock).toHaveBeenCalledOnce(); - expect(updateOutputMock).toHaveBeenCalledWith('hello world'); + expect(updateOutputMock).toHaveBeenCalledWith('hello \nworld'); resolveExecutionPromise({ rawOutput: Buffer.from(''), output: '', + stdout: '', + stderr: '', exitCode: 0, signal: null, error: null, @@ -319,13 +332,16 @@ describe('ShellTool', () => { }); expect(updateOutputMock).toHaveBeenCalledOnce(); + // Advance time past the throttle interval. await vi.advanceTimersByTimeAsync(OUTPUT_UPDATE_INTERVAL_MS + 1); + // Send a SECOND progress event. This one will trigger the flush. mockShellOutputCallback({ type: 'binary_progress', bytesReceived: 2048, }); + // Now it should be called a second time with the latest progress. expect(updateOutputMock).toHaveBeenCalledTimes(2); expect(updateOutputMock).toHaveBeenLastCalledWith( '[Receiving binary output... 2.0 KB received]', @@ -334,6 +350,8 @@ describe('ShellTool', () => { resolveExecutionPromise({ rawOutput: Buffer.from(''), output: '', + stdout: '', + stderr: '', exitCode: 0, signal: null, error: null, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 621ff90f..5b01a82f 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -97,8 +97,6 @@ class ShellToolInvocation extends BaseToolInvocation< async execute( signal: AbortSignal, updateOutput?: (output: string) => void, - terminalColumns?: number, - terminalRows?: number, ): Promise { const strippedCommand = stripShellWrapper(this.params.command); @@ -131,7 +129,9 @@ class ShellToolInvocation extends BaseToolInvocation< this.params.directory || '', ); - let cumulativeOutput = ''; + let cumulativeStdout = ''; + let cumulativeStderr = ''; + let lastUpdateTime = Date.now(); let isBinaryStream = false; @@ -148,9 +148,15 @@ class ShellToolInvocation extends BaseToolInvocation< switch (event.type) { case 'data': - if (isBinaryStream) break; - cumulativeOutput = event.chunk; - currentDisplayOutput = cumulativeOutput; + if (isBinaryStream) break; // Don't process text if we are in binary mode + if (event.stream === 'stdout') { + cumulativeStdout += event.chunk; + } else { + cumulativeStderr += event.chunk; + } + currentDisplayOutput = + cumulativeStdout + + (cumulativeStderr ? `\n${cumulativeStderr}` : ''); if (Date.now() - lastUpdateTime > OUTPUT_UPDATE_INTERVAL_MS) { shouldUpdate = true; } @@ -181,8 +187,6 @@ class ShellToolInvocation extends BaseToolInvocation< } }, signal, - terminalColumns, - terminalRows, ); const result = await resultPromise; @@ -214,7 +218,7 @@ class ShellToolInvocation extends BaseToolInvocation< if (result.aborted) { llmContent = 'Command was cancelled by user before it could complete.'; if (result.output.trim()) { - llmContent += ` Below is the output before it was cancelled:\n${result.output}`; + llmContent += ` Below is the output (on stdout and stderr) before it was cancelled:\n${result.output}`; } else { llmContent += ' There was no output before it was cancelled.'; } @@ -228,7 +232,8 @@ class ShellToolInvocation extends BaseToolInvocation< llmContent = [ `Command: ${this.params.command}`, `Directory: ${this.params.directory || '(root)'}`, - `Output: ${result.output || '(empty)'}`, + `Stdout: ${result.stdout || '(empty)'}`, + `Stderr: ${result.stderr || '(empty)'}`, `Error: ${finalError}`, // Use the cleaned error string. `Exit Code: ${result.exitCode ?? '(none)'}`, `Signal: ${result.signal ?? '(none)'}`, diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 5684e4ac..68ea00af 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -50,8 +50,6 @@ export interface ToolInvocation< execute( signal: AbortSignal, updateOutput?: (output: string) => void, - terminalColumns?: number, - terminalRows?: number, ): Promise; } @@ -80,8 +78,6 @@ export abstract class BaseToolInvocation< abstract execute( signal: AbortSignal, updateOutput?: (output: string) => void, - terminalColumns?: number, - terminalRows?: number, ): Promise; } @@ -200,16 +196,9 @@ export abstract class DeclarativeTool< params: TParams, signal: AbortSignal, updateOutput?: (output: string) => void, - terminalColumns?: number, - terminalRows?: number, ): Promise { const invocation = this.build(params); - return invocation.execute( - signal, - updateOutput, - terminalColumns, - terminalRows, - ); + return invocation.execute(signal, updateOutput); } } diff --git a/packages/vscode-ide-companion/tsconfig.json b/packages/vscode-ide-companion/tsconfig.json index b3e700de..2fec2bd9 100644 --- a/packages/vscode-ide-companion/tsconfig.json +++ b/packages/vscode-ide-companion/tsconfig.json @@ -6,8 +6,7 @@ "lib": ["ES2022", "dom"], "sourceMap": true, "rootDir": "src", - "strict": true /* enable all strict type-checking options */, - "skipLibCheck": true /* Helps avoid potential type conflicts between different third-party libraries in node_modules. */ + "strict": true /* enable all strict type-checking options */ /* Additional Checks */ // "noImplicitReturns": true, /* Report error when not all code paths in function return a value. */ // "noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */