Core(fix)!: Requiring mode when setting policy to prevent all modes by default (breaking)#24758
Core(fix)!: Requiring mode when setting policy to prevent all modes by default (breaking)#24758
Conversation
Summary of ChangesHello, 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 breaking change to the policy engine by mandating the 'modes' parameter for all policy configurations. This ensures safer defaults and prevents accidental policy application across unintended modes, particularly for sensitive operations. The change includes updates to all built-in policies, adjustments to the policy loader's validation logic, and comprehensive updates to tests and documentation to reflect the new requirement. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the modes field mandatory for all policy rules and safety checkers. The changes include updating core type definitions, enforcing the field in the TOML loader's schema, and updating all default policy files, internal rule generation logic, and test suites to include explicit mode associations. Documentation has also been updated to reflect this requirement. I have no feedback to provide.
Note: Security Review did not run due to the size of the PR.
| toolName = "enter_plan_mode" | ||
| decision = "allow" | ||
| priority = 50 | ||
| modes = ["plan"] |
There was a problem hiding this comment.
this doesn't look right, we want this tool to be available in all modes except plan, but when non-interactive
gemini-cli
bot
added
the
status/need-issue
|
Not a part of this PR, but maybe we should change |
| decision = "allow" | ||
| priority = 15 | ||
| modes = ["autoEdit"] | ||
| modes = ["plan", "default", "autoEdit", "yolo"] |
There was a problem hiding this comment.
I can understand that we expand it to [ "autoEdit", "yolo"], but why plan and default? Same for write_file and web_search below?
|
Size Change: +533 B (0%) Total Size: 34 MB
ℹ️ View Unchanged
|
kschaab
removed
the
status/need-issue
gemini-cli
bot
added
area/enterprise
| readonly = true | ||
| approvedTools = [] | ||
| allowOverrides = false | ||
| allowOverrides = true |
There was a problem hiding this comment.
is this an intentional change?
| ] | ||
| decision = "deny" | ||
| priority = 10 | ||
| modes = ["plan", "default"] |
There was a problem hiding this comment.
why isn't this for all modes, otherwise we'd have some ask_user decisions in headless mode, no?
There was a problem hiding this comment.
You're right, but those modes are overriding these polcies anyway. It is safer to keep this for all since the priority will control that aspect instead of removing the modes here.
| readonly: true, | ||
| approvedTools: [], | ||
| allowOverrides: false, | ||
| allowOverrides: true, |
There was a problem hiding this comment.
is this an intentional change?
3 participants
Summary
This change will fail loading of any policies where
modesis not present. New auto rules will include this, but old auto rules will be broken by this change along with any user policy wheremodeswas not provided. This provides a fix to help with safe defaults for modes when specifying policies. This prevents unintentional configuration of policies by missing this parameter which currently defaults to all. This specifically prevents an unintentional assignment of non-readonly policies for planning mode.Details
This change updates policy loading to require
modesconfiguration. It updates all built in policies to includemodes. It also updates the policy loader's suggestion message to be more generic where it listed built in fields before. This is because the policy loader already indicates the missing field upon loading the policy file and the loader has different assignable zod schemas so they cannot be dynamically shown without a union of required types for all supported schemas which is not helpful to the user.Resolves #24797
Pre-Merge Checklist