fix(cli): explain missing API key when .env is skipped in untrusted folders#20048
fix(cli): explain missing API key when .env is skipped in untrusted folders#20048nsalerni wants to merge 12 commits intogoogle-gemini:mainfrom
Conversation
…olders - append a trust-specific hint to the Gemini API key auth error when a .env file is found but not loaded due to folder trust - return env-load metadata from loadEnvironment() so auth validation can distinguish trust-skips from normal missing env vars - add auth tests covering untrusted workspace .env present/absent cases Fixes google-gemini#20005
Summary of ChangesHello @nsalerni, 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 enhances the user experience by providing clearer feedback when the Gemini API key is not found, specifically addressing cases where a 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
|
There was a problem hiding this comment.
Code Review
The pull request successfully improves the user experience by providing a clear explanation when a .env file is skipped due to workspace trust policies. The implementation is robust, correctly handling cases where the workspace is sandboxed (allowing whitelisted variables) versus when it is completely untrusted. The logic for appending the hint is precise and avoids false positives. The changes to loadEnvironment to return metadata are well-integrated and the unit tests provide good coverage for the new behavior. No high or critical issues were identified.
Updated the change to avoid redundant settings reloads in `validateAuthMethod()`: - `validateAuthMethod()` now accepts injected `envLoadResult` / `settings` - `loadSettings()` now preserves the initial `loadEnvironment()` metadata on `LoadedSettings` (`initialEnvLoadResult`) - the main callers (`gemini.tsx`, `validateNonInterActiveAuth`, `AppContainer`, `useAuth`) now pass the already-computed metadata instead of forcing a fresh `loadSettings()` call This keeps the UX fix for `google-gemini#20005` while avoiding the extra sync config reads / directory walk in the common paths. Additionally, added a unit test asserting `validateAuthMethod()` does not call `loadSettings()` when `envLoadResult` is injected.
|
Hey @nsalerni, fantastic execution on this! When I filed the original issue detailing the trust boundary gap, avoiding that redundant Thanks for jumping in and getting this patched so quickly! |
|
Following up on this after doing a deeper technical scan of the diff just to ensure everything is watertight before it merges! I noticed a couple of minor structural gaps in the implementation that might be worth addressing (either here or in a fast-follow PR): Snapshot Sync gap: The LoadedSettings constructor was updated to accept initialEnvLoadResult, but the LoadedSettingsSnapshot interface (around line 305) was not updated to include it. Any system relying on snapshot serialization is going to drop that new state. Test Coverage Gap: The options bag resolution path (options.settings injected without options.envLoadResult) does not have a direct unit test asserting that loadSettings() is bypassed. Global Mock Shift: Mocking loadEnvironment globally in the beforeEach block of auth.test.ts is slightly risky; earlier tests in the file might have been implicitly relying on the real un-mocked function behavior. Just a heads-up for @galz10 and @nsalerni! Besides those minor lifecycle and test coverage points, the core parameter injection logic looks rock solid. |
- Added `initialEnvLoadResult` to `LoadedSettingsSnapshot` and included it in `computeSnapshot()` so snapshot consumers don’t lose that state.
- Added a unit test for the `validateAuthMethod(..., { settings })` path to confirm `loadSettings()` is not called when `settings` is injected.
|
@KumarADITHYA123 thanks for the follow-up. I've addressed the first two points:
On your last point, there is no shift from real to fake in beforeEach; the function is fake in all tests. auth.test.ts already does a module-level settings.js', ...), which means |
|
@galz10 I've requested an updated review (and merge if it looks good) based on the few minor updates. |
|
Verified - snapshot fix and test coverage additions look correct. The Point 3 explanation makes sense (vi.mock hoists at the module level), so earlier tests were isolated. Regarding the one CI failure: The Mac 22.x failure looks flaky since it passes identically on Mac 20.x and 24.x. nsalerni, you might just need to re-trigger that specific job. LGTM from my side! @galz10 I've verified the architectural updates from my previous review. This is unblocked and looks ready for your re-approval whenever you have a moment. |
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
|
@google-gemini/gemini-cli-maintainers friendly ping for a review. |
Summary
Improve the Gemini API key auth error in untrusted workspaces by explaining when a
.envfile was found but intentionally not loaded due to folder trust.This fixes a misleading UX where users are told
GEMINI_API_KEYis missing without any indication that the workspace trust policy blocked.envloading.Details
validateAuthMethod()now appends a trust-specific hint when all of the following are true:USE_GEMINIGEMINI_API_KEYis missing.envfile was found.envloading was skipped because the workspace is untrustedloadEnvironment()now returns lightweight metadata (envFilePath, trust result, sandbox state,skippedDueToTrust) while preserving existing behavior..envpresent (hint shown).env(no hint shown)Design choice:
/permissions trust) instead of introducing a new command reference.Related Issues
Fixes #20005
How to Validate
Run targeted tests:
npm --prefix packages/cli test -- src/config/auth.test.ts.envmessaging coverage.Run CLI package typecheck:
npm --prefix packages/cli run typecheck -- --pretty falseManual behavior check (optional, local):
GEMINI_API_KEY=...only in that workspace.env..envwas not loaded because the folder is untrusted and suggesting/permissions trust.Edge case check:
.envand repeat.Pre-Merge Checklist
If you want, I can also tailor the checklist to exactly what you personally validated (e.g. check
MacOS > npm runonly).