From d9fdff339a5a9bc1d9349ec215d4dd49493f40f8 Mon Sep 17 00:00:00 2001 From: mistergarrison Date: Mon, 6 Oct 2025 12:15:21 -0700 Subject: [PATCH] Re-submission: Make --allowed-tools work in non-interactive mode (#10289) Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: matt korwel --- integration-tests/run_shell_command.test.ts | 187 ++++++++++++++++++++ packages/cli/src/config/config.test.ts | 72 ++++++++ packages/cli/src/config/config.ts | 48 ++++- packages/cli/src/nonInteractiveCli.test.ts | 53 ++++++ packages/core/src/tools/shell.test.ts | 1 + packages/core/src/tools/shell.ts | 72 ++++++++ packages/core/src/utils/shell-utils.ts | 2 +- packages/core/src/utils/tool-utils.test.ts | 9 + packages/core/src/utils/tool-utils.ts | 5 +- 9 files changed, 442 insertions(+), 7 deletions(-) diff --git a/integration-tests/run_shell_command.test.ts b/integration-tests/run_shell_command.test.ts index cba8cb72ad..f839a45104 100644 --- a/integration-tests/run_shell_command.test.ts +++ b/integration-tests/run_shell_command.test.ts @@ -6,6 +6,24 @@ import { describe, it, expect } from 'vitest'; import { TestRig, printDebugInfo, validateModelOutput } from './test-helper.js'; +import { getShellConfiguration } from '../packages/core/src/utils/shell-utils.js'; + +const { shell } = getShellConfiguration(); + +function getLineCountCommand(): { command: string; tool: string } { + switch (shell) { + case 'powershell': + return { + command: `(Get-Content test.txt).Length`, + tool: 'Get-Content', + }; + case 'cmd': + return { command: `find /c /v "" test.txt`, tool: 'find' }; + case 'bash': + default: + return { command: `wc -l test.txt`, tool: 'wc' }; + } +} describe('run_shell_command', () => { it('should be able to run a shell command', async () => { @@ -68,6 +86,175 @@ 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 testFile = rig.createFile('test.txt', 'Lorem\nIpsum\nDolor\n'); + const { tool } = getLineCountCommand(); + const prompt = `use ${tool} to tell me how many lines there are in ${testFile}`; + + // Provide the prompt via stdin to simulate non-interactive mode + const result = await rig.run({ + stdin: prompt, + args: [`--allowed-tools=run_shell_command(${tool})`], + }); + + 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 testFile = rig.createFile('test.txt', 'Lorem\nIpsum\nDolor\n'); + const { tool } = getLineCountCommand(); + const prompt = `use ${tool} to tell me how many lines there are in ${testFile}`; + + 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 testFile = rig.createFile('test.txt', 'Lorem\nIpsum\nDolor\n'); + const { tool } = getLineCountCommand(); + const prompt = `use ${tool} to tell me how many lines there are in ${testFile}`; + + 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(); + }); + + it('should work with ShellTool alias', async () => { + const rig = new TestRig(); + await rig.setup('should work with ShellTool alias'); + + const testFile = rig.createFile('test.txt', 'Lorem\nIpsum\nDolor\n'); + const { tool } = getLineCountCommand(); + const prompt = `use ${tool} to tell me how many lines there are in ${testFile}`; + + const result = await rig.run({ + stdin: prompt, + args: [`--allowed-tools=ShellTool(${tool})`], + }); + + 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 { tool } = getLineCountCommand(); + const prompt = `use ${tool} and ls`; + + const result = await rig.run({ + stdin: prompt, + args: [ + `--allowed-tools=run_shell_command(${tool})`, + '--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 { tool } = getLineCountCommand(); + const prompt = `use date`; + + const result = await rig.run({ + stdin: prompt, + args: [ + `--allowed-tools=run_shell_command(${tool})`, + '--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 e6bb244f59..6de5c27865 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -2773,6 +2773,78 @@ 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( + {}, + [], + new ExtensionEnablementManager( + ExtensionStorage.getUserExtensionsDir(), + argv.extensions, + ), + '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( + {}, + [], + new ExtensionEnablementManager( + ExtensionStorage.getUserExtensionsDir(), + argv.extensions, + ), + '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( + {}, + [], + new ExtensionEnablementManager( + ExtensionStorage.getUserExtensionsDir(), + argv.extensions, + ), + '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 805f2f4e1a..bda3b8a5b2 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -31,6 +31,7 @@ import { ShellTool, EditTool, WriteFileTool, + SHELL_TOOL_NAMES, resolveTelemetrySettings, FatalConfigError, } from '@google/gemini-cli-core'; @@ -430,6 +431,36 @@ 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 || @@ -562,6 +593,9 @@ export async function loadCliConfig( const policyEngineConfig = createPolicyEngineConfig(settings, approvalMode); + const allowedTools = argv.allowedTools || settings.tools?.allowed || []; + const allowedToolsSet = new Set(allowedTools); + // Interactive mode: explicit -i flag or (TTY + no args + no -p flag) const hasQuery = !!argv.query; const interactive = @@ -570,14 +604,22 @@ 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(ShellTool.Name, EditTool.Name, WriteFileTool.Name); + extraExcludes.push(...defaultExcludes.filter(toolExclusionFilter)); break; case ApprovalMode.AUTO_EDIT: // In auto-edit non-interactive mode, only tools that still require a prompt are excluded. - extraExcludes.push(ShellTool.Name); + extraExcludes.push(...autoEditExcludes.filter(toolExclusionFilter)); break; case ApprovalMode.YOLO: // No extra excludes for YOLO mode. @@ -649,7 +691,7 @@ export async function loadCliConfig( question, fullContext: argv.allFiles || false, coreTools: settings.tools?.core || undefined, - allowedTools: argv.allowedTools || settings.tools?.allowed || undefined, + allowedTools: allowedTools.length > 0 ? allowedTools : undefined, policyEngineConfig, excludeTools, toolDiscoveryCommand: settings.tools?.discoveryCommand, diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 71e19b7074..3141401721 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -873,4 +873,57 @@ 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 e73f69e64a..ac861bffb2 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -61,6 +61,7 @@ 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 e480ea2299..5990208fa2 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -22,6 +22,7 @@ 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 { @@ -34,11 +35,58 @@ 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) => s && allSubcommands.add(s)); + } + } + } + + return allSubcommands; +} + export interface ShellToolParams { command: string; description?: string; @@ -76,6 +124,30 @@ 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 4a1d033729..e038e7cf2d 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'; -const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; +export 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 5527e18643..672bc72309 100644 --- a/packages/core/src/utils/tool-utils.test.ts +++ b/packages/core/src/utils/tool-utils.test.ts @@ -36,6 +36,15 @@ 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 cd3053ff87..fe3da9856e 100644 --- a/packages/core/src/utils/tool-utils.ts +++ b/packages/core/src/utils/tool-utils.ts @@ -6,8 +6,7 @@ import type { AnyDeclarativeTool, AnyToolInvocation } from '../index.js'; import { isTool } from '../index.js'; - -const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; +import { SHELL_TOOL_NAMES } from './shell-utils.js'; /** * Checks if a tool invocation matches any of a list of patterns. @@ -61,7 +60,7 @@ export function doesToolInvocationMatch( if ( 'command' in invocation.params && - toolNames.includes('run_shell_command') + toolNames.some((name) => SHELL_TOOL_NAMES.includes(name)) ) { const argValue = String( (invocation.params as { command: string }).command,