feat(extensions): add support for plan directory in extension manifest#20354
feat(extensions): add support for plan directory in extension manifest#20354
Conversation
Summary of ChangesHello @mahimashanware, 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 significant enhancements to the Gemini CLI's extension system by enabling extensions to define and contribute policy rules and safety checkers. This allows for greater customization and security enforcement within the CLI environment, while also ensuring that core security principles are maintained by explicitly ignoring potentially risky extension configurations. Additionally, extensions can now influence the default plan directory, providing more flexibility in project setup. Highlights
Changelog
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 introduces the ability for extensions to contribute to the Policy Engine and define plan directories. This is achieved by adding support for a policies/ directory and a plan field in gemini-extension.json. The implementation includes important security measures, such as preventing path traversal when loading extension files and filtering out potentially dangerous policy rules (allow and yolo mode) contributed by extensions. The changes are well-tested and appear to be implemented correctly. I have not found any high or critical issues in this pull request.
9be5347 to
84a6dc0
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces logic to allow extensions to define a default plan directory, which is a great feature for improving user experience. The implementation correctly prioritizes user-defined settings over extension settings. My review includes one high-severity comment pointing out a bug in the configuration merging logic that could lead to user settings being discarded. The suggested fix aligns with best practices for merging configuration objects to ensure all user-defined settings are respected.
a602421 to
fe7f725
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces logic to merge plan directory settings from extensions, prioritizing user-defined settings. The changes look good and the tests are comprehensive. I've found a couple of areas for improvement regarding maintainability, type consistency, and ensuring merge strategies for settings are robust and security-conscious.
fe7f725 to
903482c
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements the logic for merging plan directory settings from extensions, prioritizing user-defined configurations. The changes are well-tested and cover various scenarios, including multiple extensions and precedence rules. My main feedback concerns the complexity of the settings merge logic for planSettings in packages/cli/src/config/config.ts, which, while correct, introduces a maintainability risk by duplicating the settings merge order and deviating from consistent merge operations. I've suggested adding a comment to clarify the logic if a simpler implementation isn't feasible at this time.
903482c to
496d857
Compare
|
Size Change: +269 B (0%) Total Size: 25.8 MB ℹ️ View Unchanged
|
545f8ff to
816e458
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed generic contribution model for extension settings, which is a significant improvement for maintainability over the previous manual merging approach. The implementation correctly updates the settings precedence chain and includes important security guardrails to prevent extensions from modifying sensitive configuration namespaces. The refactoring of loadCliConfig simplifies the loading logic considerably.
My main feedback is a critical one: the new extension contributions are not validated against the settings schema. This could allow a buggy or malicious extension to inject malformed settings, potentially causing runtime errors or unexpected behavior throughout the CLI. I've added a comment with a suggested fix to validate these contributions before they are merged, aligning with our security best practices for handling untrusted input in configuration.
816e458 to
81e2ae6
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a generic mechanism for extensions to contribute default settings, which is a great improvement for maintainability and extensibility. The changes in settings.ts to handle the new "Extension Layer" in the precedence chain and the security guardrails to block sensitive keys are well-implemented. However, there is a critical issue in the re-merging logic within loadCliConfig that incorrectly inverts the settings precedence, causing schema defaults to override extension contributions. This violates the principle of consistent merge operations and order-dependent logic. This needs to be addressed to ensure the feature works as intended.
50c1bca to
41da38c
Compare
dea9ed3 to
e01f2f1
Compare
e01f2f1 to
1c63e60
Compare
1c63e60 to
33560af
Compare
#20354) Co-authored-by: Mahima Shanware <mshanware@google.com> Co-authored-by: Jerop Kipruto <jerop@google.com>
google-gemini#20354) Co-authored-by: Mahima Shanware <mshanware@google.com> Co-authored-by: Jerop Kipruto <jerop@google.com>
google-gemini#20354) Co-authored-by: Mahima Shanware <mshanware@google.com> Co-authored-by: Jerop Kipruto <jerop@google.com>
google-gemini#20354) Co-authored-by: Mahima Shanware <mshanware@google.com> Co-authored-by: Jerop Kipruto <jerop@google.com>
Summary
This PR adds support for a
plan.directoryfield in the extension manifest (gemini-extension.json). This allows extensions to specify a default directory for planning artifacts, which serves as a fallback if the user hasn't explicitly configured one in their settings.Details
plan.directorytoExtensionConfig(CLI) andGeminiCLIExtension(Core).loadCliConfigto extract the plan directory from the first active extension that provides it.Configto only add the plan directory to the workspace context if it actually exists on disk, preventing unnecessary folder creation.planmanifest property.packages/cliandpackages/coreto verify fallback behavior, user precedence, and existence checks.Related Issues
Related to #20354
How to Validate
planobject in itsgemini-extension.json:general.plan.directorysetting.Pre-Merge Checklist