-
Notifications
You must be signed in to change notification settings - Fork 11.1k
fix(config): treat system settings as read-only during migration and warn user #18277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @spencer426, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
Summary of ChangesHello @spencer426, 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 significantly improves the robustness and user experience of the CLI's auto-update feature. By introducing both pre-flight permission checks and post-update command verification, it ensures that updates are performed reliably and that the CLI remains functional afterward. This proactive approach minimizes potential issues during the update process and provides clearer, actionable feedback to users in case of failure. Highlights
Changelog
Activity
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
|
|
Size Change: +1.7 kB (+0.01%) Total Size: 23.8 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces valuable pre-flight and post-update verifications for the auto-update feature, significantly enhancing its reliability. The implementation is solid, including corresponding unit tests. I've identified one area for improvement: a race condition in the non-interactive update check that could prevent it from running. My review includes specific suggestions to address this point.
95cb208 to
413e716
Compare
packages/cli/src/config/settings.ts
Outdated
| coreEvents.emitSettingsChanged(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is too much duplicated logic with the regular setValue. We should just instead avoid saving the file if it is a system settings file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
packages/cli/src/config/settings.ts
Outdated
| if (modified) { | ||
| loadedSettings.setValue(scope, 'general', newGeneral); | ||
| anyModified = true; | ||
| if (!isSystemScope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having system scope specific checks here is a little scary. why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to use the SettingsFile interface to include a readOnly flag instead.
packages/cli/src/config/settings.ts
Outdated
|
|
||
| // Migrate codebaseInvestigatorSettings -> agents.overrides.codebase_investigator | ||
| if (experimentalSettings['codebaseInvestigatorSettings']) { | ||
| if (foundDeprecated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor so we don't have to repeat strings like codeBaseInvestigatorSettings in these migrate blocks. Add a shared helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Combined setValue and setValueInMemory using a new helper that checks if a scope needs to be saved or just kept in memory. I also cleaned up the migration code to remove duplicate strings.
a9697c6 to
cd6c338
Compare
cd6c338 to
fb63f3b
Compare
jacob314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Summary
This PR prevents the CLI from crashing when attempting to automatically migrate deprecated configuration settings in the system and system_default scopes. These scopes are now treated as
read-only, and the CLI will instead warn the user to perform manual updates for these files.
Details
prevents EACCES errors when the CLI doesn't have write permissions to system-wide configuration directories.
changes are no longer persisted to disk for these specific scopes.
the specific flags that require manual intervention.
Related Issues
How to Validate
echo '{"general": {"disableUpdateNag": true}}' > /tmp/mock-system.json
GEMINI_CLI_SYSTEM_SETTINGS_PATH=/tmp/mock-system.json npm run start
Pre-Merge Checklist