-
Notifications
You must be signed in to change notification settings - Fork 450
Fix API key loading logic to read directly from config file #386
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: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @DavidLee528, 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 addresses a critical configuration flaw that caused agent initialization failures due to misinterpreting the API key's source. By directly loading the API key from 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. 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.
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 correctly fixes the API key loading logic to read the key directly from config.json instead of treating it as an environment variable name. The change is straightforward and addresses the bug described. I've added a couple of suggestions to further improve the robustness of the configuration handling: one to add validation for the placeholder API key value, and another to consider using environment variables for secrets as a security best practice. Overall, good fix!
| if not config['api_key']: | ||
| print('api_key configuration not set.') | ||
| sys.exit(1) | ||
| api_key = os.getenv(config['api_key']) | ||
| api_key = config['api_key'] |
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.
The validation for api_key only checks for an empty value. The default config.json uses "API_KEY" as a placeholder. It would be more robust to also check for this placeholder value to prevent runtime errors and provide a clearer message to the user if they haven't configured their key.
| if not config['api_key']: | |
| print('api_key configuration not set.') | |
| sys.exit(1) | |
| api_key = os.getenv(config['api_key']) | |
| api_key = config['api_key'] | |
| api_key = config['api_key'] | |
| if not api_key or api_key == 'API_KEY': | |
| print('Error: The "api_key" in config.json is missing or is a placeholder. Please provide a valid API key.') | |
| sys.exit(1) |
| print('api_key configuration not set.') | ||
| sys.exit(1) | ||
| api_key = os.getenv(config['api_key']) | ||
| api_key = config['api_key'] |
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.
For enhanced security, it's a best practice to avoid storing secrets like API keys in plaintext configuration files, as they can be accidentally committed to version control. Consider loading the key directly from an environment variable instead (e.g., api_key = os.getenv('OPENAI_API_KEY')). This is a suggestion for future improvement and doesn't block this PR.
Summary:
This PR fixes a configuration issue where the agent attempted to read the API key as an environment variable name from
config.json, causing initialization failures. The code now correctly loads the API key value directly from the configuration file and improves the error message for missing keys.Changes:
os.getenv(config['api_key']).config['api_key'].Impact:
Ensures consistent API key loading from
config.jsonand prevents false “environment variable not set” errors during agent initialization.