From e8a065cb9f23c85b4adaa391b5ee0947228224ea Mon Sep 17 00:00:00 2001 From: mistergarrison Date: Fri, 26 Sep 2025 10:03:35 -0700 Subject: [PATCH] Make --allowed-tools work in non-interactive mode (#9114) Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> --- integration-tests/run_shell_command.test.ts | 160 ++++++++++++++++++++ packages/cli/src/config/config.test.ts | 45 ++++++ 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, 388 insertions(+), 7 deletions(-) diff --git a/integration-tests/run_shell_command.test.ts b/integration-tests/run_shell_command.test.ts index cba8cb72ad..4846ffafc0 100644 --- a/integration-tests/run_shell_command.test.ts +++ b/integration-tests/run_shell_command.test.ts @@ -68,6 +68,166 @@ 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 3f8aff414b..8b77b13e84 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -1936,6 +1936,51 @@ 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 c74a503c34..1528f589c8 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'; @@ -396,6 +397,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 || @@ -527,6 +558,9 @@ 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 = @@ -535,14 +569,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. @@ -614,7 +656,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 5562580d87..11802c7b57 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) => 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 a3afbc32e6..10238264b9 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,