Make --allowed-tools work in non-interactive mode #9114
Conversation
The `--allowed-tools` flag was not being respected in non-interactive mode. This was because the logic that excludes tools in non-interactive mode was overriding the `allowedTools` setting. This change modifies the logic to check if a tool is in the `allowedTools` list before excluding it. This allows the user to explicitly enable tools in non-interactive mode. A new test case has been added to verify that a normally-excluded tool can be used in non-interactive mode when it is specified with the `--allowed-tools` flag.
This change fixes an issue where the `--allowed-tools` flag did not correctly handle sub-commands for `ShellTool` in non-interactive mode. Previously, `loadCliConfig` was performing an exact match on the tool name, which would cause `ShellTool` to be excluded even if a sub-command was allowed (e.g., `ShellTool(wc)`). The new behavior is as follows: 1. `loadCliConfig` no longer excludes `ShellTool` if it is present in the `--allowed-tools` list, even with sub-commands. 2. A runtime check is added to `ShellTool.shouldConfirmExecute` to verify that the command is allowed in non-interactive mode. If the command is not in the allowlist, an error is thrown, preventing the tool from hanging. 3. The `parseAllowedSubcommands` helper function in `shell.ts` now correctly handles multiple `--allowed-tools` flags for `ShellTool`, taking the union of specified sub-commands and treating a bare `ShellTool` as a wildcard. 4. Added comments to `shell.ts` to explain the motivation for the changes. Integration tests have been added to verify the new behavior, including special cases like `ShellTool()`, `ShellTool`, and multiple `--allowed-tools` flags.
This change fixes an issue where the `--allowed-tools` flag did not correctly handle sub-commands for `ShellTool` in non-interactive mode. Previously, `loadCliConfig` was performing an exact match on the tool name, which would cause `ShellTool` to be excluded even if a sub-command was allowed (e.g., `ShellTool(wc)`). The new behavior is as follows: 1. `loadCliConfig` no longer excludes `ShellTool` if it is present in the `--allowed-tools` list, even with sub-commands. 2. A runtime check is added to `ShellTool.shouldConfirmExecute` to verify that the command is allowed in non-interactive mode. If the command is not in the allowlist, an error is thrown, preventing the tool from hanging. This check is skipped in YOLO mode. 3. The `parseAllowedSubcommands` helper function in `shell.ts` now correctly handles multiple `--allowed-tools` flags for `ShellTool`, taking the union of specified sub-commands and treating a bare `ShellTool` or `run_shell_command` as a wildcard. It also handles the `ShellTool` alias. 4. Added comments to `shell.ts` to explain the motivation for the changes. Integration tests have been added to verify the new behavior, including special cases like `ShellTool()`, `ShellTool`, multiple `--allowed-tools` flags, the `ShellTool` alias, and `--yolo` mode.
This change fixes an issue where the `--allowed-tools` flag did not correctly handle sub-commands for `ShellTool` in non-interactive mode. Previously, `loadCliConfig` was performing an exact match on the tool name, which would cause `ShellTool` to be excluded even if a sub-command was allowed (e.g., `ShellTool(wc)`). The new behavior is as follows: 1. `loadCliConfig` no longer excludes `ShellTool` if it is present in the `--allowed-tools` list, even with sub-commands. 2. A runtime check is added to `ShellTool.shouldConfirmExecute` to verify that the command is allowed in non-interactive mode. If the command is not in the allowlist, an error is thrown, preventing the tool from hanging. This check is skipped in YOLO mode. 3. The `parseAllowedSubcommands` helper function in `shell.ts` now correctly handles multiple `--allowed-tools` flags for `ShellTool`, taking the union of specified sub-commands and treating a bare `ShellTool` or `run_shell_command` as a wildcard. It also handles the `ShellTool` alias. 4. Added comments to `shell.ts` to explain the motivation for the changes. Integration tests have been added to verify the new behavior, including special cases like `ShellTool()`, `ShellTool`, multiple `--allowed-tools` flags, the `ShellTool` alias, and `--yolo` mode.
The `--allowed-tools` flag was not correctly handling the `ShellTool` alias. This was because the `doesToolInvocationMatch` function was hardcoding a check for `run_shell_command`. This change modifies the check to use the `SHELL_TOOL_NAMES` constant, which includes both `run_shell_command` and `ShellTool`. A test case has been added to verify that the alias works as expected. Additionally, a failing test in `shell.test.ts` has been fixed by mocking the `isInteractive` function in the test configuration.
Fix shell tool alias
The `--allowed-tools` flag was not correctly identifying `ShellTool` and its aliases in non-interactive mode. This caused the tool to be excluded even when explicitly allowed. This change fixes the issue by using `SHELL_TOOL_NAMES` to check for all valid names of the shell tool. Adds unit tests to verify the fix.
…active fix: Allow ShellTool in non-interactive mode via --allowed-tools
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @mistergarrison, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical usability issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully enables the use of --allowed-tools in non-interactive mode, particularly for ShellTool. The changes correctly adjust the tool exclusion logic in loadCliConfig and add a preemptive check within ShellTool to prevent hangs on unapproved commands. The updates are well-supported by a comprehensive set of new integration and unit tests. My review includes one high-severity suggestion to refactor duplicated code in packages/cli/src/config/config.ts to improve long-term maintainability.
Extracted the duplicated tool exclusion filter logic into a new helper function `createToolExclusionFilter`. This improves maintainability and readability of the code. Added a JSDoc comment to the new function to explain what it does and why.
Refactor duplicated tool exclusion logic in config.ts
|
The failing test (because it runs long) looks unrelated: it('should not be interactive if positional prompt words are provided with other flags', async () => {
process.stdin.isTTY = true;
process.argv = ['node', 'script.js', '--model', 'gemini-1.5-pro', 'Hello'];
const argv = await parseArguments({} as Settings);
const config = await loadCliConfig({}, [], 'test-session', argv);
expect(config.isInteractive()).toBe(false);
}); |
e8a065c
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
TLDR
Non-interactive mode excludes tools that can block (including ShellTool), which means that passing --allowed-tools with shell tools doesn't do anything.
This change blocks ShellTool from being excluded if it is allowed, and preemptively rejects non-allowed shell tools (so that they don't hang waiting for user approval that will never come).
This should not affect interactive behavior at all, nor should it affect the behavior of YOLO mode.
Reviewer Test Plan
Testing Matrix
Linked issues / bugs
Fixes #9011