fix(cli): coerce env-resolved boolean strings in settings#25608
fix(cli): coerce env-resolved boolean strings in settings#25608joycebeatriz wants to merge 3 commits intogoogle-gemini:mainfrom
Conversation
|
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, 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 improves the handling of environment-based boolean configurations in settings.json. By introducing schema-aware coercion, the CLI now correctly interprets interpolated environment variables as booleans when expected, resolving validation issues while maintaining existing behavior for string-only maps. 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
|
There was a problem hiding this comment.
Code Review
This pull request implements automatic boolean coercion for configuration settings resolved from environment variables, ensuring strings like "true" or "false" are cast to booleans based on the schema. It includes documentation updates and new test cases. However, the review identifies critical issues: the coercion logic does not handle schema references ($ref), which will cause it to fail for many settings, and the internal type definitions for schema nodes are incompatible with nested collections, potentially leading to incomplete normalization.
joycebeatriz
left a comment
There was a problem hiding this comment.
Addressed in 340c2bc: boolean coercion is now ref-aware (via schema definitions), recursive traversal was fixed for nested structures, and regression tests were added.
joycebeatriz
left a comment
There was a problem hiding this comment.
Could a maintainer please approve the pending workflows and provide code owner review? Feedback was addressed in 340c2bc.
|
I am not the maintainer, but I am the original issue reporter. My PR took the opposite approach, keeping the changes minimal. |
|
Hi @midnight-wonderer! No problem at all! It’s actually nice to see more than one solution to the same problem. Thanks for sharing yours. |
Summary
This PR fixes env-based boolean configuration for
settings.json.Previously, interpolated env values were always strings, which could trigger validation warnings for boolean settings (for example,
ui.autoThemeSwitching). This change adds schema-aware coercion so"true"/"false"values are interpreted as booleans where appropriate.Details
envvalues stay strings).Related Issues
Fixes #25573
How to Validate
settings.json, for example:"ui": { "autoThemeSwitching": "${GEMINI_AUTO_THEME:-true}" }GEMINI_AUTO_THEMEtoTRUEorFALSE.npm test -w @google/gemini-cli -- src/config/settings.test.tsshould coerce env-resolved boolean setting values from stringsshould not coerce env-resolved values in string-only map settingsPre-Merge Checklist