feat(cli): configure policy engine from existing settings#8348
Merged
allenhutchison merged 15 commits intomainfrom Sep 18, 2025
Merged
feat(cli): configure policy engine from existing settings#8348allenhutchison merged 15 commits intomainfrom
allenhutchison merged 15 commits intomainfrom
Conversation
This commit introduces the logic to generate a default configuration for the Policy Engine based on existing user settings. This ensures backwards compatibility and a seamless transition to the new permissions model. The new `createPolicyEngineConfig` function translates settings from `config.yaml` into the structured rules required by the Policy Engine. This includes: - Mapping `tools.allowed` and `tools.exclude` to allow/deny rules. - Translating `mcp.allowed` and `mcp.excluded` for MCP tool sources. - Preserving the behavior of `tools.autoAccept` for read-only tools. - Handling approval modes such as `YOLO` and `AUTO_EDIT`. This change ensures that users' existing configurations continue to work as expected with the introduction of the underlying Policy Engine. Comprehensive unit tests have been added to verify that the translation from legacy settings to policy rules is correct.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary of Changes
Hello @allenhutchison, 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 introduces a robust mechanism to bridge existing user configurations with a new Policy Engine, ensuring backward compatibility and a smooth transition to an updated permissions model. By translating current config.yaml settings into structured policy rules, the system can maintain expected behavior for tool allowances, exclusions, and approval workflows without requiring users to reconfigure their environments. This change is foundational for enhancing the flexibility and security of tool execution within the CLI.
Highlights
- Policy Engine Configuration Generation: A new
createPolicyEngineConfigfunction has been introduced to dynamically generate policy engine configurations based on existing user settings. - Backward Compatibility: This function translates legacy
config.yamlsettings, such astools.allowed,tools.exclude,mcp.allowed,mcp.excluded,tools.autoAccept, anduseSmartEdit, into structured policy rules to ensure seamless transition to the new permissions model. - Approval Mode Handling: The new configuration logic properly accounts for different approval modes like
YOLOandAUTO_EDIT, preserving their intended behavior within the new policy framework. - Integration with CLI Configuration: The generated policy engine configuration is now integrated into the
loadCliConfigfunction, ensuring the new permissions model is applied during the CLI's configuration loading process. - Comprehensive Unit Testing: Extensive unit tests have been added to thoroughly validate the correct translation of various legacy settings into the new policy rules, covering different scenarios and edge cases.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a policy engine configuration layer to maintain backward compatibility with existing settings. The implementation correctly translates most legacy settings into policy rules. However, I've found a critical issue in the implementation of YOLO approval mode, where it fails to auto-approve tools that perform write actions due to incorrect rule priorities. I've provided a suggestion to fix this logic and an update to the corresponding unit test. The rest of the changes, including the new tests and wiring in config.ts, look good.
|
Size Change: +3.26 kB (+0.02%) Total Size: 17.3 MB
ℹ️ View Unchanged
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
gemini-cli
bot
added
kind/enhancement
priority/p2
abhipatel12
reviewed
Sep 12, 2025
Contributor
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
Looks great! Have a couple of questions!
packages/cli/src/config/policy.ts
Outdated
| if (approvalMode === ApprovalMode.YOLO) { | ||
| rules.push({ | ||
| decision: PolicyDecision.ALLOW, | ||
| priority: 0, // Lowest priority |
Contributor
There was a problem hiding this comment.
Is YOLO supposed to have the lowest priority?
In general, how are we assigning the priority number here? Will we want some central map or set that tells us the various numbers in use at any given point?
Contributor
Author
There was a problem hiding this comment.
Added updated comments.
packages/cli/src/config/policy.ts
Outdated
| } | ||
| } | ||
|
|
||
| for (const tool of WRITE_TOOLS) { |
Contributor
There was a problem hiding this comment.
Are we adding this loop for readability? We have a default decision below as well for ASK_USER
Contributor
Author
There was a problem hiding this comment.
Yes I wanted it to be explicit while we transition over to the new system.
packages/cli/src/config/policy.ts
Outdated
| if (settings.mcp?.excluded) { | ||
| for (const server of settings.mcp.excluded) { | ||
| rules.push({ | ||
| toolName: `^mcp://${server}/.*`, |
Contributor
There was a problem hiding this comment.
Does the PolicyEngine in core support pattern matching like we have here? I thought the matching was exact string matching?
Add comprehensive support for MCP server trust configurations in the policy engine, enabling centralized tool permission management. This change: - Adds policy rules for MCP servers with trust=true flag (priority 90) - Adds policy rules for allowed MCP servers from settings.mcp.allowed (priority 85) - Adds policy rules for excluded MCP servers from settings.mcp.excluded (priority 195) - Uses the serverName__* pattern to match MCP tool naming convention - Documents the complete priority hierarchy for policy rules - Updates tests to cover all MCP server configuration scenarios including trust flags, allowed/excluded lists, and priority conflicts - Ensures proper priority ordering: excludes > allows > trust > general rules This centralizes all tool confirmation logic into the policy engine, making it easier to understand and maintain the security model for both built-in tools and MCP server tools.
…vers Enhance the policy engine to support wildcard patterns in tool names using the "serverName__*" syntax. This enables MCP server-wide policy rules that match all tools from a specific server. Changes: - Modified ruleMatches() to detect and handle "__*" wildcard patterns - Pattern "serverName__*" now matches any tool like "serverName__toolName" - Added comprehensive tests for wildcard pattern matching - Ensures specific tool rules can override server-wide wildcards via priority This change is essential for the MCP server trust integration, allowing policies to efficiently manage permissions for all tools from a server without needing to enumerate each tool individually.
Fix policy engine integration issues and add comprehensive integration tests to ensure the policy configuration works correctly with the PolicyEngine. Changes: - Fixed TypeScript error in wildcard pattern matching for undefined tool names - Fixed YOLO mode to not add write tool rules that would override the wildcard ALLOW rule (priority issue) - Added comprehensive integration test suite that verifies: - Basic tool allow/deny configurations - MCP server wildcard patterns work correctly - Priority ordering is applied properly - YOLO and AUTO_EDIT modes function as expected - Edge cases like conflicting configurations - Non-interactive mode transformations The integration tests ensure that the policy configuration generated by createPolicyEngineConfig() produces valid PolicyEngine configurations that behave correctly in all scenarios.
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a policy engine configuration layer to maintain backward compatibility with existing settings. The implementation correctly translates various settings like tools.allowed, mcp.allowed, etc., into policy rules. The changes are well-tested with new unit and integration tests.
My review focuses on a critical correctness issue in the policy priority logic and a high-severity maintainability issue related to deep package imports. The current priority assignment leads to counterintuitive permission behavior where general rules override specific ones. Additionally, deep imports into the dist folder of a dependency create a fragile coupling between packages. I've provided suggestions to address these points.
Comment on lines
+46
to
+59
| // Priority system for policy rules: | ||
| // - Higher priority numbers win over lower priority numbers | ||
| // - When multiple rules match, the highest priority rule is applied | ||
| // - Rules are evaluated in order of priority (highest first) | ||
| // | ||
| // Priority levels used in this configuration: | ||
| // 0: Default allow-all (YOLO mode only) | ||
| // 10: Write tools default to ASK_USER | ||
| // 50: Auto-accept read-only tools | ||
| // 85: MCP servers allowed list | ||
| // 90: MCP servers with trust=true | ||
| // 100: Explicitly allowed individual tools | ||
| // 195: Explicitly excluded MCP servers | ||
| // 200: Explicitly excluded individual tools (highest priority) |
Contributor
There was a problem hiding this comment.
The current priority system for policy rules can lead to counterintuitive behavior. Specifically, a general server exclusion (mcp.excluded with priority 195) overrides a specific tool permission (tools.allowed with priority 100). A user would reasonably expect a more specific rule to take precedence over a general one.
For example, if a user excludes mcp-server but wants to allow mcp-server__safe-tool, the current logic will deny the tool. This is confirmed by tests in policy-engine.integration.test.ts.
To fix this, the priorities should be adjusted to give specific rules higher precedence. Here is a suggested new priority scheme:
210:tools.exclude(Specific Deny)200:tools.allowed(Specific Allow)195:mcp.excluded(Wildcard Deny)90:mcp.trust(Wildcard Allow)85:mcp.allowed(Wildcard Allow)
This would make the behavior more predictable and align with user expectations. The existing tests will need to be updated to reflect this corrected logic.
- Export PolicyEngine from @google/gemini-cli-core to avoid deep imports - Update integration test to use proper import path - Fix misleading comment about priority precedence - Add comprehensive test coverage for all settings combinations This addresses maintainability concerns about brittle deep imports into the dist folder and clarifies the intentional priority system where denies always override allows for security.
abhipatel12
reviewed
Sep 16, 2025
Contributor
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
Looks good! Had a couple of questions!
|
|
||
| // READ_ONLY_TOOLS is a list of built-in tools that do not modify the user's | ||
| // files or system state. | ||
| const READ_ONLY_TOOLS = new Set([ |
Contributor
There was a problem hiding this comment.
nit: should we consider using the actual tool names (by importing the tool) in the off chance these change during some hill climbing exercise where we change tool names.
Contributor
There was a problem hiding this comment.
Same comment for the write tools below!
Contributor
Author
There was a problem hiding this comment.
Good point. Done
| // 85: MCP servers allowed list | ||
| // 90: MCP servers with trust=true | ||
| // 100: Explicitly allowed individual tools | ||
| // 195: Explicitly excluded MCP servers |
Contributor
There was a problem hiding this comment.
This may be the intended behavior, but is there a way for a user to exclude a MCP server in general but then specifically allow a single tool?
Is there any reason we'd want to give higher priority to explicitly allowed or disallowed tools than server wide inclusions/exclusions?
Contributor
Author
There was a problem hiding this comment.
Includes and excludes for MCP work on a per-tool basis. There is a trust setting that covers the entire MCP server config.
packages/cli/src/config/policy.ts
Outdated
Comment on lines
+128
to
+134
| if (settings.useSmartEdit) { | ||
| rules.push({ | ||
| toolName: 'replace', | ||
| decision: PolicyDecision.DENY, | ||
| priority: 200, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
the smart edit tool uses the same name as the regular edit (replace).
Contributor
Author
There was a problem hiding this comment.
Good catch. I thought it was actually called SmartEdit from the file name. Thanks.
|
|
||
| // If auto-accept is enabled, allow all read-only tools. | ||
| // Priority: 50 | ||
| if (settings.tools?.autoAccept) { |
Contributor
There was a problem hiding this comment.
Did not know we even had this haha!
Contributor
Author
There was a problem hiding this comment.
Right, we have so many settings for permissions. At least this will bring them all together in one place.
3 tasks
abhipatel12
approved these changes
Sep 18, 2025
Contributor
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
Awesome, really excited to build on this! LGTM
packages/cli/src/config/policy.ts
Outdated
Comment on lines
+51
to
+52
| WebFetchTool.Name, | ||
| WebSearchTool.Name, |
Contributor
There was a problem hiding this comment.
Did you mean to move web search from read only to write?
Contributor
Author
There was a problem hiding this comment.
fixed. Good catch. That's what I get for manual coding :-/
…hTool` to `READ_TOOLS` instead of `WRITE_TOOLS`
mattKorwel
removed this pull request from the merge queue due to the queue being cleared
abhipatel12
pushed a commit
that referenced
this pull request
Sep 19, 2025
nagendrareddy10
pushed a commit
to nagendrareddy10/gemini-cli
that referenced
this pull request
Sep 22, 2025
yashv6655
added a commit
to yashv6655/gemini-cli
that referenced
this pull request
Sep 22, 2025
giraffe-tree
pushed a commit
to giraffe-tree/gemini-cli
that referenced
this pull request
Oct 10, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit introduces the logic to generate a default configuration for the Policy Engine based on existing user settings. This ensures backwards compatibility and a seamless transition to the new permissions model.
The new
createPolicyEngineConfigfunction translates settings fromconfig.yamlinto the structured rules required by the Policy Engine. This includes:tools.allowedandtools.excludeto allow/deny rules.mcp.allowedandmcp.excludedfor MCP tool sources.tools.autoAcceptfor read-only tools.YOLOandAUTO_EDIT.This change ensures that users' existing configurations continue to work as expected with the introduction of the underlying Policy Engine.
Comprehensive unit tests have been added to verify that the translation from legacy settings to policy rules is correct.
Part of the work for #7231