fix(cli): Prevent truncation of first character in shell commands

- The shell command processor was incorrectly truncating the first
  character of the command (e.g., 'ls' became 's') due to an
  erroneous `slice(1)` operation, likely introduced during a
  previous merge. This change removes the slice, ensuring the full
  command is processed.
- Introduces unit tests for the shellCommandProcessor hook.
- Fixes a minor grammatical issue in the display of GEMINI.md file count.
This commit is contained in:
Taylor Mullen 2025-05-20 00:21:01 -07:00 committed by N. Taylor Mullen
parent 9c72a3ae12
commit 6ca446bded
3 changed files with 324 additions and 28 deletions

View File

@ -348,7 +348,8 @@ export const App = ({
<Text color={Colors.AccentRed}>|_| </Text> <Text color={Colors.AccentRed}>|_| </Text>
)} )}
<Text color={Colors.SubtleComment}> <Text color={Colors.SubtleComment}>
Using {geminiMdFileCount} GEMINI.md files Using {geminiMdFileCount} GEMINI.md file
{geminiMdFileCount > 1 ? 's' : ''}
</Text> </Text>
</Box> </Box>
)} )}

View File

@ -0,0 +1,285 @@
/**
* @license
* Copyright 2025 Google LLC
* 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-code/server';
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
// Mocks
const mockAddItemToHistory = 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', () => ({
default: {
randomBytes: vi.fn(() => ({ toString: vi.fn(() => 'randomBytes') })),
},
randomBytes: vi.fn(() => ({ toString: vi.fn(() => 'randomBytes') })),
}));
vi.mock('path', () => ({
default: {
join: vi.fn((...args) => args.join('/')),
sep: '/',
},
join: vi.fn((...args) => args.join('/')),
sep: '/',
}));
vi.mock('os', () => ({
default: {
tmpdir: vi.fn(() => '/tmp'),
},
tmpdir: vi.fn(() => '/tmp'),
}));
// 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,
};
});
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>>;
beforeEach(() => {
vi.clearAllMocks();
mockExecuteCommand = vi.fn(); // Injected exec command mock
// 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>
>;
// It's important to reset these spies if they are to be configured per test
existsSyncSpy.mockReset();
readFileSyncSpy.mockReset();
unlinkSyncSpy.mockReset();
mockGetTargetDir.mockReturnValue('/current/dir');
});
const setupHook = () =>
renderHook(() =>
useShellCommandProcessor(
mockAddItemToHistory,
mockOnExec,
mockOnDebugMessage,
mockConfig,
mockExecuteCommand as unknown as typeof ExecType, // Cast to satisfy TypeScript
),
);
it('should return false for non-string input', () => {
const { result } = setupHook();
const handled = result.current.handleShellCommand([
{ text: 'not a string' },
] as PartListUnion);
expect(handled).toBe(false);
expect(mockAddItemToHistory).not.toHaveBeenCalled();
});
it('should handle empty shell command', () => {
const { result } = setupHook();
act(() => {
const handled = result.current.handleShellCommand('');
expect(handled).toBe(true);
});
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 = '';
mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
if (callback) callback(null, stdout, stderr);
return {} as any;
});
existsSyncSpy.mockReturnValue(false);
await act(async () => {
result.current.handleShellCommand(command);
await new Promise(process.nextTick);
});
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; }; pwd >/tmp/shell_pwd_randomBytes.tmp',
{ cwd: '/current/dir' },
expect.any(Function),
);
expect(mockOnExec).toHaveBeenCalled();
expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
2,
{ type: 'info', text: stdout },
expect.any(Number),
);
});
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';
mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
if (callback) callback(null, stdout, stderr);
return {} as any;
});
existsSyncSpy.mockReturnValue(false);
await act(async () => {
result.current.handleShellCommand(command);
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),
);
});
it('should handle command with only stderr output', async () => {
const { result } = setupHook();
const command = '!command_only_stderr';
const stdout = '';
const stderr = 'just stderr';
mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
if (callback) callback(null, stdout, stderr);
return {} as any;
});
existsSyncSpy.mockReturnValue(false);
await act(async () => {
result.current.handleShellCommand(command);
await new Promise(process.nextTick);
});
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');
mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
if (callback) callback(error, '', '');
return {} as any;
});
existsSyncSpy.mockReturnValue(false);
await act(async () => {
result.current.handleShellCommand(command);
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),
);
});
it('should correctly handle commands ending with &', async () => {
const { result } = setupHook();
const command = '!sleep 5 &';
mockGetTargetDir.mockReturnValue('/current/dir');
mockExecuteCommand.mockImplementation((_cmd, _options, callback) => {
if (callback) callback(null, '', '');
return {} as any;
});
existsSyncSpy.mockReturnValue(false);
await act(async () => {
result.current.handleShellCommand(command);
await new Promise(process.nextTick);
});
expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
1,
{ type: 'user_shell', text: command },
expect.any(Number),
);
expect(mockExecuteCommand).toHaveBeenCalledWith(
'{ !sleep 5 & }; pwd >/tmp/shell_pwd_randomBytes.tmp',
{ cwd: '/current/dir' },
expect.any(Function),
);
expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
2,
{ type: 'info', text: '(Command produced no output)' },
expect.any(Number),
);
});
});

View File

@ -4,7 +4,8 @@
* SPDX-License-Identifier: Apache-2.0 * SPDX-License-Identifier: Apache-2.0
*/ */
import { exec as _exec } from 'child_process'; import { exec as defaultExec } from 'child_process';
import type { exec as ExecType } from 'child_process';
import { useCallback } from 'react'; import { useCallback } from 'react';
import { Config } from '@gemini-code/server'; import { Config } from '@gemini-code/server';
import { type PartListUnion } from '@google/genai'; import { type PartListUnion } from '@google/genai';
@ -13,6 +14,7 @@ import crypto from 'crypto';
import path from 'path'; import path from 'path';
import os from 'os'; import os from 'os';
import fs from 'fs'; import fs from 'fs';
/** /**
* Hook to process shell commands (e.g., !ls, $pwd). * Hook to process shell commands (e.g., !ls, $pwd).
* Executes the command in the target directory and adds output/errors to history. * Executes the command in the target directory and adds output/errors to history.
@ -22,6 +24,7 @@ export const useShellCommandProcessor = (
onExec: (command: Promise<void>) => void, onExec: (command: Promise<void>) => void,
onDebugMessage: (message: string) => void, onDebugMessage: (message: string) => void,
config: Config, config: Config,
executeCommand: typeof ExecType = defaultExec, // Injectable for testing
) => { ) => {
/** /**
* Checks if the query is a shell command, executes it, and adds results to history. * Checks if the query is a shell command, executes it, and adds results to history.
@ -33,7 +36,7 @@ export const useShellCommandProcessor = (
return false; return false;
} }
let commandToExecute = rawQuery.trim().slice(1).trimStart(); let commandToExecute = rawQuery.trim().trimStart();
// wrap command to write pwd to temporary file // wrap command to write pwd to temporary file
const pwdFileName = `shell_pwd_${crypto.randomBytes(6).toString('hex')}.tmp`; const pwdFileName = `shell_pwd_${crypto.randomBytes(6).toString('hex')}.tmp`;
@ -48,7 +51,7 @@ export const useShellCommandProcessor = (
userMessageTimestamp, userMessageTimestamp,
); );
if (!commandToExecute) { if (rawQuery.trim() === '') {
addItemToHistory( addItemToHistory(
{ type: 'error', text: 'Empty shell command.' }, { type: 'error', text: 'Empty shell command.' },
userMessageTimestamp, userMessageTimestamp,
@ -65,37 +68,44 @@ export const useShellCommandProcessor = (
}; };
const execPromise = new Promise<void>((resolve) => { const execPromise = new Promise<void>((resolve) => {
_exec(commandToExecute, execOptions, (error, stdout, stderr) => { executeCommand(
if (error) { commandToExecute,
addItemToHistory( execOptions,
{ type: 'error', text: error.message }, (error, stdout, stderr) => {
userMessageTimestamp, if (error) {
); addItemToHistory(
} else { { type: 'error', text: error.message },
let output = ''; userMessageTimestamp,
if (stdout) output += stdout; );
if (stderr) output += (output ? '\n' : '') + stderr; // Include stderr as info } else {
let output = '';
if (stdout) output += stdout;
if (stderr) output += (output ? '\n' : '') + stderr; // Include stderr as info
addItemToHistory(
{ type: 'info', text: output || '(Command produced no output)' },
userMessageTimestamp,
);
}
if (fs.existsSync(pwdFilePath)) {
const pwd = fs.readFileSync(pwdFilePath, 'utf8').trim();
if (pwd !== targetDir) {
addItemToHistory( addItemToHistory(
{ {
type: 'info', type: 'info',
text: `WARNING: shell mode is stateless; \`cd ${pwd}\` will not apply to next command`, text: output || '(Command produced no output)',
}, },
userMessageTimestamp, userMessageTimestamp,
); );
} }
fs.unlinkSync(pwdFilePath); if (fs.existsSync(pwdFilePath)) {
} const pwd = fs.readFileSync(pwdFilePath, 'utf8').trim();
resolve(); if (pwd !== targetDir) {
}); addItemToHistory(
{
type: 'info',
text: `WARNING: shell mode is stateless; \`cd ${pwd}\` will not apply to next command`,
},
userMessageTimestamp,
);
}
fs.unlinkSync(pwdFilePath);
}
resolve();
},
);
}); });
try { try {
@ -106,7 +116,7 @@ export const useShellCommandProcessor = (
return true; // Command was initiated return true; // Command was initiated
}, },
[config, onDebugMessage, addItemToHistory, onExec], [config, onDebugMessage, addItemToHistory, onExec, executeCommand],
); );
return { handleShellCommand }; return { handleShellCommand };