feat: Adds shell command context to gemini history (#1076)

1 files changed, 118 insertions, 232 deletions

diff --git a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts
index 129a5401..847ce054 100644
--- a/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts
+++ b/packages/cli/src/ui/hooks/shellCommandProcessor.test.ts
@@ -4,290 +4,176 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-/* eslint-disable @typescript-eslint/no-explicit-any */
 import { act, renderHook } from '@testing-library/react';
-import { useShellCommandProcessor } from './shellCommandProcessor.js';
-import { type Config } from '@gemini-cli/core';
-import { type PartListUnion } from '@google/genai';
-import { existsSync, readFileSync, unlinkSync } from 'fs';
-import type * as FsMod from 'fs';
-import type { exec as ExecType } from 'child_process'; // For typing the injected mock
+import { vi } from 'vitest';
+import { useShellCommandProcessor } from './shellCommandProcessor';
+import { Config, GeminiClient } from '@gemini-cli/core';
+import * as fs from 'fs';
+import EventEmitter from 'events';
 
-// Mocks
-const mockAddItemToHistory = vi.fn();
-const mockSetPendingHistoryItem = vi.fn();
-const mockOnExec = vi.fn(async (promise) => await promise);
-const mockOnDebugMessage = vi.fn();
-const mockGetTargetDir = vi.fn();
-let mockExecuteCommand: ReturnType<typeof vi.fn>; // This will be our injected mock for child_process.exec
-
-const mockConfig = {
-  getTargetDir: mockGetTargetDir,
-} as unknown as Config;
-
-vi.mock('crypto', () => ({
+// Mock dependencies
+vi.mock('child_process');
+vi.mock('fs');
+vi.mock('os', () => ({
   default: {
-    randomBytes: vi.fn(() => ({ toString: vi.fn(() => 'randomBytes') })),
+    platform: () => 'linux',
+    tmpdir: () => '/tmp',
   },
-  randomBytes: vi.fn(() => ({ toString: vi.fn(() => 'randomBytes') })),
+  platform: () => 'linux',
+  tmpdir: () => '/tmp',
 }));
-
-vi.mock('path', () => ({
-  default: {
-    join: vi.fn((...args) => args.join('/')),
-    sep: '/',
-  },
-  join: vi.fn((...args) => args.join('/')),
-  sep: '/',
+vi.mock('@gemini-cli/core');
+vi.mock('../utils/textUtils.js', () => ({
+  isBinary: vi.fn(),
 }));
 
-vi.mock('os', () => ({
-  default: {
-    tmpdir: vi.fn(() => '/tmp'),
-    platform: vi.fn(() => 'linux'),
-  },
-  tmpdir: vi.fn(() => '/tmp'),
-  platform: vi.fn(() => 'linux'),
-}));
+describe('useShellCommandProcessor', () => {
+  let spawnEmitter: EventEmitter;
+  let addItemToHistoryMock: vi.Mock;
+  let setPendingHistoryItemMock: vi.Mock;
+  let onExecMock: vi.Mock;
+  let onDebugMessageMock: vi.Mock;
+  let configMock: Config;
+  let geminiClientMock: GeminiClient;
 
-// Configure the fs mock to use new vi.fn() instances created within the factory
-vi.mock('fs', async (importOriginal) => {
-  const original = (await importOriginal()) as typeof FsMod;
-  const internalMockFsExistsSync = vi.fn();
-  const internalMockFsReadFileSync = vi.fn();
-  const internalMockFsUnlinkSync = vi.fn();
-  return {
-    ...original,
-    existsSync: internalMockFsExistsSync,
-    readFileSync: internalMockFsReadFileSync,
-    unlinkSync: internalMockFsUnlinkSync,
-  };
-});
+  beforeEach(async () => {
+    const { spawn } = await import('child_process');
+    spawnEmitter = new EventEmitter();
+    spawnEmitter.stdout = new EventEmitter();
+    spawnEmitter.stderr = new EventEmitter();
+    (spawn as vi.Mock).mockReturnValue(spawnEmitter);
 
-describe('useShellCommandProcessor', () => {
-  // These spies will point to the vi.fn() instances created by the vi.mock('fs') factory.
-  let existsSyncSpy: ReturnType<typeof vi.mocked<typeof existsSync>>;
-  let readFileSyncSpy: ReturnType<typeof vi.mocked<typeof readFileSync>>;
-  let unlinkSyncSpy: ReturnType<typeof vi.mocked<typeof unlinkSync>>;
+    vi.spyOn(fs, 'existsSync').mockReturnValue(false);
+    vi.spyOn(fs, 'readFileSync').mockReturnValue('');
+    vi.spyOn(fs, 'unlinkSync').mockReturnValue(undefined);
 
-  beforeEach(() => {
-    vi.clearAllMocks();
-    mockExecuteCommand = vi.fn(); // Injected exec command mock
+    addItemToHistoryMock = vi.fn();
+    setPendingHistoryItemMock = vi.fn();
+    onExecMock = vi.fn();
+    onDebugMessageMock = vi.fn();
 
-    // Assign the imported (and mocked by vi.mock factory) fs functions to spies
-    existsSyncSpy = existsSync as unknown as ReturnType<
-      typeof vi.mocked<typeof existsSync>
-    >;
-    readFileSyncSpy = readFileSync as unknown as ReturnType<
-      typeof vi.mocked<typeof readFileSync>
-    >;
-    unlinkSyncSpy = unlinkSync as unknown as ReturnType<
-      typeof vi.mocked<typeof unlinkSync>
-    >;
+    configMock = {
+      getTargetDir: () => '/test/dir',
+    } as unknown as Config;
 
-    // It's important to reset these spies if they are to be configured per test
-    existsSyncSpy.mockReset();
-    readFileSyncSpy.mockReset();
-    unlinkSyncSpy.mockReset();
+    geminiClientMock = {
+      addHistory: vi.fn(),
+    } as unknown as GeminiClient;
+  });
 
-    mockGetTargetDir.mockReturnValue('/current/dir');
+  afterEach(() => {
+    vi.restoreAllMocks();
   });
 
-  const setupHook = () =>
+  const renderProcessorHook = () =>
     renderHook(() =>
       useShellCommandProcessor(
-        mockAddItemToHistory,
-        mockSetPendingHistoryItem,
-        mockOnExec,
-        mockOnDebugMessage,
-        mockConfig,
-        mockExecuteCommand as unknown as typeof ExecType, // Cast to satisfy TypeScript
+        addItemToHistoryMock,
+        setPendingHistoryItemMock,
+        onExecMock,
+        onDebugMessageMock,
+        configMock,
+        geminiClientMock,
       ),
     );
 
-  it('should return false for non-string input', () => {
-    const { result } = setupHook();
-    const handled = result.current.handleShellCommand(
-      [{ text: 'not a string' }] as PartListUnion,
-      new AbortController().signal,
-    );
-    expect(handled).toBe(false);
-    expect(mockAddItemToHistory).not.toHaveBeenCalled();
-  });
+  it('should execute a command and update history on success', async () => {
+    const { result } = renderProcessorHook();
+    const abortController = new AbortController();
 
-  it('should handle empty shell command', () => {
-    const { result } = setupHook();
     act(() => {
-      const handled = result.current.handleShellCommand(
-        '',
-        new AbortController().signal,
-      );
-      expect(handled).toBe(true);
+      result.current.handleShellCommand('ls -l', abortController.signal);
     });
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: '' },
-      expect.any(Number),
-    );
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'error', text: 'Empty shell command.' },
-      expect.any(Number),
-    );
-    expect(mockExecuteCommand).not.toHaveBeenCalled();
-  });
 
-  it('should execute a successful command and add output to history', async () => {
-    const { result } = setupHook();
-    const command = '!ls -l';
-    const stdout = 'total 0';
-    const stderr = '';
+    expect(onExecMock).toHaveBeenCalledTimes(1);
+    const execPromise = onExecMock.mock.calls[0][0];
+
+    // Simulate stdout
+    act(() => {
+      spawnEmitter.stdout.emit('data', Buffer.from('file1.txt\nfile2.txt'));
+    });
 
-    mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
-      if (callback) callback(null, stdout, stderr);
-      return {} as any;
+    // Simulate process exit
+    act(() => {
+      spawnEmitter.emit('exit', 0, null);
     });
-    existsSyncSpy.mockReturnValue(false);
 
     await act(async () => {
-      result.current.handleShellCommand(command, new AbortController().signal);
-      await new Promise(process.nextTick);
+      await execPromise;
     });
 
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: command },
-      expect.any(Number),
-    );
-    expect(mockOnDebugMessage).toHaveBeenCalledWith(
-      expect.stringContaining('Executing shell command in /current/dir:'),
-    );
-    expect(mockExecuteCommand).toHaveBeenCalledWith(
-      '{ !ls -l; }; __code=$?; pwd >/tmp/shell_pwd_randomBytes.tmp; exit $__code',
-      { cwd: '/current/dir' },
-      expect.any(Function),
-    );
-    expect(mockOnExec).toHaveBeenCalled();
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'info', text: stdout },
-      expect.any(Number),
-    );
+    expect(addItemToHistoryMock).toHaveBeenCalledTimes(2);
+    expect(addItemToHistoryMock.mock.calls[1][0]).toEqual({
+      type: 'info',
+      text: 'file1.txt\nfile2.txt',
+    });
+    expect(geminiClientMock.addHistory).toHaveBeenCalledTimes(1);
   });
 
-  it('should handle command with stderr output', async () => {
-    const { result } = setupHook();
-    const command = '!some_command_with_stderr';
-    const stdout = 'some output';
-    const stderr = 'some error output';
+  it('should handle binary output', async () => {
+    const { result } = renderProcessorHook();
+    const abortController = new AbortController();
+    const { isBinary } = await import('../utils/textUtils.js');
+    (isBinary as vi.Mock).mockReturnValue(true);
 
-    mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
-      if (callback) callback(null, stdout, stderr);
-      return {} as any;
+    act(() => {
+      result.current.handleShellCommand(
+        'cat myimage.png',
+        abortController.signal,
+      );
     });
-    existsSyncSpy.mockReturnValue(false);
 
-    await act(async () => {
-      result.current.handleShellCommand(command, new AbortController().signal);
-      await new Promise(process.nextTick);
-    });
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: command },
-      expect.any(Number),
-    );
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'info', text: `${stdout}\n${stderr}` },
-      expect.any(Number),
-    );
-  });
+    expect(onExecMock).toHaveBeenCalledTimes(1);
+    const execPromise = onExecMock.mock.calls[0][0];
 
-  it('should handle command with only stderr output', async () => {
-    const { result } = setupHook();
-    const command = '!command_only_stderr';
-    const stdout = '';
-    const stderr = 'just stderr';
+    act(() => {
+      spawnEmitter.stdout.emit('data', Buffer.from([0x89, 0x50, 0x4e, 0x47]));
+    });
 
-    mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
-      if (callback) callback(null, stdout, stderr);
-      return {} as any;
+    act(() => {
+      spawnEmitter.emit('exit', 0, null);
     });
-    existsSyncSpy.mockReturnValue(false);
 
     await act(async () => {
-      result.current.handleShellCommand(command, new AbortController().signal);
-      await new Promise(process.nextTick);
+      await execPromise;
+    });
+
+    expect(addItemToHistoryMock).toHaveBeenCalledTimes(2);
+    expect(addItemToHistoryMock.mock.calls[1][0]).toEqual({
+      type: 'info',
+      text: '[Command produced binary output, which is not shown.]',
     });
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: command },
-      expect.any(Number),
-    );
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'info', text: stderr },
-      expect.any(Number),
-    );
   });
 
-  it('should handle command error', async () => {
-    const { result } = setupHook();
-    const command = '!failing_command';
-    const error = new Error('Command failed');
+  it('should handle command failure', async () => {
+    const { result } = renderProcessorHook();
+    const abortController = new AbortController();
 
-    mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
-      if (callback) callback(error, '', '');
-      return {} as any;
+    act(() => {
+      result.current.handleShellCommand(
+        'a-bad-command',
+        abortController.signal,
+      );
     });
-    existsSyncSpy.mockReturnValue(false);
 
-    await act(async () => {
-      result.current.handleShellCommand(command, new AbortController().signal);
-      await new Promise(process.nextTick);
-    });
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: command },
-      expect.any(Number),
-    );
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'error', text: error.message },
-      expect.any(Number),
-    );
-  });
+    const execPromise = onExecMock.mock.calls[0][0];
 
-  it('should correctly handle commands ending with &', async () => {
-    const { result } = setupHook();
-    const command = '!sleep 5 &';
-    mockGetTargetDir.mockReturnValue('/current/dir');
+    act(() => {
+      spawnEmitter.stderr.emit('data', Buffer.from('command not found'));
+    });
 
-    mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
-      if (callback) callback(null, '', '');
-      return {} as any;
+    act(() => {
+      spawnEmitter.emit('exit', 127, null);
     });
-    existsSyncSpy.mockReturnValue(false);
 
     await act(async () => {
-      result.current.handleShellCommand(command, new AbortController().signal);
-      await new Promise(process.nextTick);
+      await execPromise;
     });
 
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      1,
-      { type: 'user_shell', text: command },
-      expect.any(Number),
-    );
-    expect(mockExecuteCommand).toHaveBeenCalledWith(
-      '{ !sleep 5 & }; __code=$?; pwd >/tmp/shell_pwd_randomBytes.tmp; exit $__code',
-      { cwd: '/current/dir' },
-      expect.any(Function),
-    );
-    expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
-      2,
-      { type: 'info', text: '(Command produced no output)' },
-      expect.any(Number),
-    );
+    expect(addItemToHistoryMock).toHaveBeenCalledTimes(2);
+    expect(addItemToHistoryMock.mock.calls[1][0]).toEqual({
+      type: 'error',
+      text: 'Command exited with code 127.\ncommand not found',
+    });
   });
 });