-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
gemini-cli: write to system settings file #8333
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
base: master
Are you sure you want to change the base?
gemini-cli: write to system settings file #8333
Conversation
`gemini` writes state back to the user config file (`~/.gemini/settings.json`), which doesn't work when it's a symlink into the nix store. To fix this, we can write our settings to the "system" settings file instead, and set the appropriate environment variable so that this file can still be placed in `~/.config/gemini`. This leaves `setttings.json` free for `gemini` to write to as it wishes. Signed-off-by: Iain Lane <[email protected]>
6f1aa82 to
37354da
Compare
rrvsh
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.
lgtm good idea
| GEMINI_MODEL = cfg.defaultModel; | ||
| } | ||
| // lib.optionalAttrs (cfg.settings != { }) { | ||
| GEMINI_CLI_SYSTEM_SETTINGS_PATH = "${config.home.homeDirectory}/.gemini/system-settings.json"; |
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.
This will work, only concern would be for anyone who might want to actually do a system management through nixpkgs and then user level management through home manager is no longer going to have that separation.
EDIT: Maybe make the system settings a option so someone who wants to keep it user settings level can, but those who want to use the system level has that choice?
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.
@khaneliman good point. What I see is that then we'll have to go back to writing settings.json, and gemini doesn't work properly if this points to the store. You can't log in interactively as it wants to write the auth block. Settings can't be saved either, but this perhaps less of a blocker for nix users?
On my system right now this file has:
{
"security": {
"auth": {
"selectedType": "oauth-personal"
}
},
"general": {
"previewFeatures": true
}
}...which was written by the tool itself.
Counter proposal: we could document that this sets GEMINI_CLI_SYSTEM_SETTINGS_PATH, and this makes it incompatible with managing that file another way.
WDYT? I don't mind adding an option really, but it's worth a brief discussion.
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.
I guess I'd prefer an option to keep it flexible for more scenarios. We can even default it to enabled so that it works the way a normal user would expect.
What I see is that then we'll have to go back to writing settings.json, and gemini doesn't work properly if this points to the store. You can't log in interactively as it wants to write the auItith block. Settings can't be saved either, but this perhaps less of a blocker for nix users?
You CAN use it still. It just doesn't persist settings because it can't write to the file. But, you can add the settings yourself and it works just fine persisting the things you tell it to.
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.
You CAN use it still. It just doesn't persist settings because it can't write to the file. But, you can add the settings yourself and it works just fine persisting the things you tell it to.
I'd just like to check what you mean here because I can't quite tell what you are disagreeing with / correcting me on. Feels like that's what I said. But I don't want to misunderstand and not fix the PR properly.
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.
Just meant that the app works fine, it's just unusable to save changes to the file through the app.
Description
geminiwrites state back to the user config file (~/.gemini/settings.json), which doesn't work when it's a symlink into the nix store. To fix this, we can write our settings to the "system" settings file instead, and set the appropriate environment variable so that this file can still be placed in~/.config/gemini. This leavessetttings.jsonfree forgeminito write to as it wishes.Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.Code tested through
nix run .#tests -- test-allornix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.