diff --git a/integration-tests/run_shell_command.test.ts b/integration-tests/run_shell_command.test.ts index 4846ffafc0..cba8cb72ad 100644 --- a/integration-tests/run_shell_command.test.ts +++ b/integration-tests/run_shell_command.test.ts @@ -68,166 +68,6 @@ describe('run_shell_command', () => { validateModelOutput(result, 'test-stdin', 'Shell command stdin test'); }); - it('should run allowed sub-command in non-interactive mode', async () => { - const rig = new TestRig(); - await rig.setup('should run allowed sub-command in non-interactive mode'); - - const prompt = `use wc to tell me how many lines there are in /proc/meminfo`; - - // Provide the prompt via stdin to simulate non-interactive mode - const result = await rig.run({ - stdin: prompt, - args: ['--allowed-tools=run_shell_command(wc)'], - }); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - }); - - it('should succeed with no parens in non-interactive mode', async () => { - const rig = new TestRig(); - await rig.setup('should succeed with no parens in non-interactive mode'); - - const prompt = `use wc to tell me how many lines there are in /proc/meminfo`; - - const result = await rig.run({ - stdin: prompt, - args: ['--allowed-tools=run_shell_command'], - }); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - }); - - it('should succeed with --yolo mode', async () => { - const rig = new TestRig(); - await rig.setup('should succeed with --yolo mode'); - - const prompt = `use wc to tell me how many lines there are in /proc/meminfo`; - - const result = await rig.run( - { - prompt: prompt, - }, - '--yolo', - ); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - expect(result).toContain('lines in /proc/meminfo'); - }); - - it('should work with ShellTool alias', async () => { - const rig = new TestRig(); - await rig.setup('should work with ShellTool alias'); - - const prompt = `use wc to tell me how many lines there are in /proc/meminfo`; - - const result = await rig.run({ - stdin: prompt, - args: ['--allowed-tools=ShellTool(wc)'], - }); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - }); - - it('should combine multiple --allowed-tools flags', async () => { - const rig = new TestRig(); - await rig.setup('should combine multiple --allowed-tools flags'); - - const prompt = `use wc and ls`; - - const result = await rig.run({ - stdin: prompt, - args: [ - '--allowed-tools=run_shell_command(wc)', - '--allowed-tools=run_shell_command(ls)', - ], - }); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - }); - - it('should allow all with "ShellTool" and other specifics', async () => { - const rig = new TestRig(); - await rig.setup('should allow all with "ShellTool" and other specifics'); - - const prompt = `use date`; - - const result = await rig.run({ - stdin: prompt, - args: [ - '--allowed-tools=run_shell_command(wc)', - '--allowed-tools=run_shell_command', - ], - }); - - const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000); - - if (!foundToolCall) { - printDebugInfo(rig, result, { - 'Found tool call': foundToolCall, - }); - } - - expect( - foundToolCall, - 'Expected to find a run_shell_command tool call', - ).toBeTruthy(); - }); - it('should propagate environment variables to the child process', async () => { const rig = new TestRig(); await rig.setup('should propagate environment variables'); diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 8b77b13e84..3f8aff414b 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -1936,51 +1936,6 @@ describe('loadCliConfig tool exclusions', () => { expect(config.getExcludeTools()).not.toContain('replace'); expect(config.getExcludeTools()).not.toContain('write_file'); }); - - it('should not exclude shell tool in non-interactive mode when --allowed-tools="ShellTool" is set', async () => { - process.stdin.isTTY = false; - process.argv = [ - 'node', - 'script.js', - '-p', - 'test', - '--allowed-tools', - 'ShellTool', - ]; - const argv = await parseArguments({} as Settings); - const config = await loadCliConfig({}, [], 'test-session', argv); - expect(config.getExcludeTools()).not.toContain(ShellTool.Name); - }); - - it('should not exclude shell tool in non-interactive mode when --allowed-tools="run_shell_command" is set', async () => { - process.stdin.isTTY = false; - process.argv = [ - 'node', - 'script.js', - '-p', - 'test', - '--allowed-tools', - 'run_shell_command', - ]; - const argv = await parseArguments({} as Settings); - const config = await loadCliConfig({}, [], 'test-session', argv); - expect(config.getExcludeTools()).not.toContain(ShellTool.Name); - }); - - it('should not exclude shell tool in non-interactive mode when --allowed-tools="ShellTool(wc)" is set', async () => { - process.stdin.isTTY = false; - process.argv = [ - 'node', - 'script.js', - '-p', - 'test', - '--allowed-tools', - 'ShellTool(wc)', - ]; - const argv = await parseArguments({} as Settings); - const config = await loadCliConfig({}, [], 'test-session', argv); - expect(config.getExcludeTools()).not.toContain(ShellTool.Name); - }); }); describe('loadCliConfig interactive', () => { diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 1528f589c8..c74a503c34 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -31,7 +31,6 @@ import { ShellTool, EditTool, WriteFileTool, - SHELL_TOOL_NAMES, resolveTelemetrySettings, FatalConfigError, } from '@google/gemini-cli-core'; @@ -397,36 +396,6 @@ export async function loadHierarchicalGeminiMemory( ); } -/** - * Creates a filter function to determine if a tool should be excluded. - * - * In non-interactive mode, we want to disable tools that require user - * interaction to prevent the CLI from hanging. This function creates a predicate - * that returns `true` if a tool should be excluded. - * - * A tool is excluded if it's not in the `allowedToolsSet`. The shell tool - * has a special case: it's not excluded if any of its subcommands - * are in the `allowedTools` list. - * - * @param allowedTools A list of explicitly allowed tool names. - * @param allowedToolsSet A set of explicitly allowed tool names for quick lookups. - * @returns A function that takes a tool name and returns `true` if it should be excluded. - */ -function createToolExclusionFilter( - allowedTools: string[], - allowedToolsSet: Set, -) { - return (tool: string): boolean => { - if (tool === ShellTool.Name) { - // If any of the allowed tools is ShellTool (even with subcommands), don't exclude it. - return !allowedTools.some((allowed) => - SHELL_TOOL_NAMES.some((shellName) => allowed.startsWith(shellName)), - ); - } - return !allowedToolsSet.has(tool); - }; -} - export function isDebugMode(argv: CliArgs): boolean { return ( argv.debug || @@ -558,9 +527,6 @@ export async function loadCliConfig( const policyEngineConfig = createPolicyEngineConfig(settings, approvalMode); - const allowedTools = argv.allowedTools || settings.tools?.allowed || []; - const allowedToolsSet = new Set(allowedTools); - // Fix: If promptWords are provided, always use non-interactive mode const hasPromptWords = argv.promptWords && argv.promptWords.length > 0; const interactive = @@ -569,22 +535,14 @@ export async function loadCliConfig( // In non-interactive mode, exclude tools that require a prompt. const extraExcludes: string[] = []; if (!interactive && !argv.experimentalAcp) { - const defaultExcludes = [ShellTool.Name, EditTool.Name, WriteFileTool.Name]; - const autoEditExcludes = [ShellTool.Name]; - - const toolExclusionFilter = createToolExclusionFilter( - allowedTools, - allowedToolsSet, - ); - switch (approvalMode) { case ApprovalMode.DEFAULT: // In default non-interactive mode, all tools that require approval are excluded. - extraExcludes.push(...defaultExcludes.filter(toolExclusionFilter)); + extraExcludes.push(ShellTool.Name, EditTool.Name, WriteFileTool.Name); break; case ApprovalMode.AUTO_EDIT: // In auto-edit non-interactive mode, only tools that still require a prompt are excluded. - extraExcludes.push(...autoEditExcludes.filter(toolExclusionFilter)); + extraExcludes.push(ShellTool.Name); break; case ApprovalMode.YOLO: // No extra excludes for YOLO mode. @@ -656,7 +614,7 @@ export async function loadCliConfig( question, fullContext: argv.allFiles || false, coreTools: settings.tools?.core || undefined, - allowedTools: allowedTools.length > 0 ? allowedTools : undefined, + allowedTools: argv.allowedTools || settings.tools?.allowed || undefined, policyEngineConfig, excludeTools, toolDiscoveryCommand: settings.tools?.discoveryCommand, diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 3141401721..71e19b7074 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -873,57 +873,4 @@ describe('runNonInteractive', () => { expect(processStdoutSpy).toHaveBeenCalledWith('Acknowledged'); }); - - it('should allow a normally-excluded tool when --allowed-tools is set', async () => { - // By default, ShellTool is excluded in non-interactive mode. - // This test ensures that --allowed-tools overrides this exclusion. - vi.mocked(mockConfig.getToolRegistry).mockReturnValue({ - getTool: vi.fn().mockReturnValue({ - name: 'ShellTool', - description: 'A shell tool', - run: vi.fn(), - }), - getFunctionDeclarations: vi.fn().mockReturnValue([{ name: 'ShellTool' }]), - } as unknown as ToolRegistry); - - const toolCallEvent: ServerGeminiStreamEvent = { - type: GeminiEventType.ToolCallRequest, - value: { - callId: 'tool-shell-1', - name: 'ShellTool', - args: { command: 'ls' }, - isClientInitiated: false, - prompt_id: 'prompt-id-allowed', - }, - }; - const toolResponse: Part[] = [{ text: 'file.txt' }]; - mockCoreExecuteToolCall.mockResolvedValue({ responseParts: toolResponse }); - - const firstCallEvents: ServerGeminiStreamEvent[] = [toolCallEvent]; - const secondCallEvents: ServerGeminiStreamEvent[] = [ - { type: GeminiEventType.Content, value: 'file.txt' }, - { - type: GeminiEventType.Finished, - value: { reason: undefined, usageMetadata: { totalTokenCount: 10 } }, - }, - ]; - - mockGeminiClient.sendMessageStream - .mockReturnValueOnce(createStreamFromEvents(firstCallEvents)) - .mockReturnValueOnce(createStreamFromEvents(secondCallEvents)); - - await runNonInteractive( - mockConfig, - mockSettings, - 'List the files', - 'prompt-id-allowed', - ); - - expect(mockCoreExecuteToolCall).toHaveBeenCalledWith( - mockConfig, - expect.objectContaining({ name: 'ShellTool' }), - expect.any(AbortSignal), - ); - expect(processStdoutSpy).toHaveBeenCalledWith('file.txt'); - }); }); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index ac861bffb2..e73f69e64a 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -61,7 +61,6 @@ describe('ShellTool', () => { .mockReturnValue(createMockWorkspaceContext('/test/dir')), getGeminiClient: vi.fn(), getShouldUseNodePtyShell: vi.fn().mockReturnValue(false), - isInteractive: vi.fn().mockReturnValue(true), } as unknown as Config; shellTool = new ShellTool(mockConfig); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index de00170604..e480ea2299 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -22,7 +22,6 @@ import { ToolConfirmationOutcome, Kind, } from './tools.js'; -import { ApprovalMode } from '../config/config.js'; import { getErrorMessage } from '../utils/errors.js'; import { summarizeToolOutput } from '../utils/summarizer.js'; import type { @@ -35,58 +34,11 @@ import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { getCommandRoots, isCommandAllowed, - SHELL_TOOL_NAMES, stripShellWrapper, } from '../utils/shell-utils.js'; export const OUTPUT_UPDATE_INTERVAL_MS = 1000; -/** - * Parses the `--allowed-tools` flag to determine which sub-commands of the - * ShellTool are allowed. The flag can be provided multiple times. - * - * @param allowedTools The list of allowed tools from the config. - * @returns A Set of allowed sub-commands, or null if all commands are allowed. - * - `null`: All sub-commands are allowed (e.g., --allowed-tools="ShellTool"). - * - `Set`: A set of specifically allowed sub-commands (e.g., --allowed-tools="ShellTool(wc)" --allowed-tools="ShellTool(ls)"). - * - `Set<>` (empty): No sub-commands are allowed (e.g., --allowed-tools="ShellTool()"). - */ -function parseAllowedSubcommands( - allowedTools: readonly string[], -): Set | null { - const shellToolEntries = allowedTools.filter((tool) => - SHELL_TOOL_NAMES.some((name) => tool.startsWith(name)), - ); - - if (shellToolEntries.length === 0) { - return new Set(); // ShellTool not mentioned, so no subcommands are allowed. - } - - // If any entry is just "run_shell_command" or "ShellTool", all subcommands are allowed. - if (shellToolEntries.some((entry) => SHELL_TOOL_NAMES.includes(entry))) { - return null; - } - - const allSubcommands = new Set(); - const toolNamePattern = SHELL_TOOL_NAMES.join('|'); - const regex = new RegExp(`^(${toolNamePattern})\\((.*)\\)$`); - - for (const entry of shellToolEntries) { - const match = entry.match(regex); - if (match) { - const subcommands = match[2]; - if (subcommands) { - subcommands - .split(',') - .map((s) => s.trim()) - .forEach((s) => allSubcommands.add(s)); - } - } - } - - return allSubcommands; -} - export interface ShellToolParams { command: string; description?: string; @@ -124,30 +76,6 @@ export class ShellToolInvocation extends BaseToolInvocation< ): Promise { const command = stripShellWrapper(this.params.command); const rootCommands = [...new Set(getCommandRoots(command))]; - - // In non-interactive mode, we need to prevent the tool from hanging while - // waiting for user input. If a tool is not fully allowed (e.g. via - // --allowed-tools="ShellTool(wc)"), we should throw an error instead of - // prompting for confirmation. This check is skipped in YOLO mode. - if ( - !this.config.isInteractive() && - this.config.getApprovalMode() !== ApprovalMode.YOLO - ) { - const allowed = this.config.getAllowedTools() || []; - const allowedSubcommands = parseAllowedSubcommands(allowed); - if (allowedSubcommands !== null) { - // Not all commands are allowed, so we need to check. - const allCommandsAllowed = rootCommands.every((cmd) => - allowedSubcommands.has(cmd), - ); - if (!allCommandsAllowed) { - throw new Error( - `Command "${command}" is not in the list of allowed tools for non-interactive mode.`, - ); - } - } - } - const commandsToConfirm = rootCommands.filter( (command) => !this.allowlist.has(command), ); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index e038e7cf2d..4a1d033729 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -11,7 +11,7 @@ import { quote } from 'shell-quote'; import { doesToolInvocationMatch } from './tool-utils.js'; import { spawn, type SpawnOptionsWithoutStdio } from 'node:child_process'; -export const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; +const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; /** * An identifier for the shell type. diff --git a/packages/core/src/utils/tool-utils.test.ts b/packages/core/src/utils/tool-utils.test.ts index 672bc72309..5527e18643 100644 --- a/packages/core/src/utils/tool-utils.test.ts +++ b/packages/core/src/utils/tool-utils.test.ts @@ -36,15 +36,6 @@ describe('doesToolInvocationMatch', () => { expect(result).toBe(true); }); - it('should match a command with an alias', () => { - const invocation = { - params: { command: 'wc -l' }, - } as AnyToolInvocation; - const patterns = ['ShellTool(wc)']; - const result = doesToolInvocationMatch('ShellTool', invocation, patterns); - expect(result).toBe(true); - }); - it('should match a command that is a prefix', () => { const invocation = { params: { command: 'git status -v' }, diff --git a/packages/core/src/utils/tool-utils.ts b/packages/core/src/utils/tool-utils.ts index fe3da9856e..cd3053ff87 100644 --- a/packages/core/src/utils/tool-utils.ts +++ b/packages/core/src/utils/tool-utils.ts @@ -6,7 +6,8 @@ import type { AnyDeclarativeTool, AnyToolInvocation } from '../index.js'; import { isTool } from '../index.js'; -import { SHELL_TOOL_NAMES } from './shell-utils.js'; + +const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; /** * Checks if a tool invocation matches any of a list of patterns. @@ -60,7 +61,7 @@ export function doesToolInvocationMatch( if ( 'command' in invocation.params && - toolNames.some((name) => SHELL_TOOL_NAMES.includes(name)) + toolNames.includes('run_shell_command') ) { const argValue = String( (invocation.params as { command: string }).command,