Improve error messages for missing api keys #256
Conversation
- Add validation to detect missing API keys early - Show clear error messages with setup instructions - Include provider-specific links and environment variable examples - Suggest Ollama as alternative for local usage
Summary of ChangesHello @ZuebeyirEser, 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 user experience by introducing proactive and informative API key validation. It ensures that users are immediately notified of missing API keys with clear, actionable instructions before any resource-intensive operations begin. The changes include a new utility for generating detailed error messages, a centralized configuration for API key information, and integration of this validation logic across key CLI commands and model configuration settings. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience by adding early validation for API keys and providing clear, helpful error messages. The changes are well-structured, particularly the centralization of API key information and the creation of a formatted error message function.
I've included a few suggestions to further refine the implementation:
- A refactoring to remove redundant validation logic, which will make the code cleaner and more efficient.
- Some minor improvements in
config.pyto enhance maintainability by using constants and simplifying code.
Overall, this is a great enhancement to the project's usability and robustness.
Greptile OverviewGreptile SummaryThis PR attempts to add early API key validation with helpful error messages, but introduces a critical bug that breaks core CLI functionality. Critical IssueThe validation added to
Additional IssuesCode Quality Violations (Custom Instructions):
What Works✅ The RecommendationThe PR requires significant rework. The validation logic needs to be moved out of property getters to avoid breaking the model update flow. Consider validating only at the point where models are actually used, not when configs are accessed. Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant CLI as cli.py
participant Main as main.py
participant Config as config.py (AppConfig)
participant Model as ModelConfig
User->>CLI: graph-code start --orchestrator openai:gpt-4
CLI->>Main: update_model_settings("openai:gpt-4", None)
Main->>Main: _update_single_model_setting(ORCHESTRATOR, ...)
Note over Main,Config: BUG: Validates OLD config before updating
Main->>Config: settings.active_orchestrator_config
Config->>Model: validate_api_key(ORCHESTRATOR)
Model-->>Config: ValueError (if old config missing API key)
Config-->>Main: FAILS before new config is set
Note over Main,Config: If validation passed (e.g., Ollama default)
Main->>Config: set_orchestrator(provider, model, **kwargs)
Config->>Config: _active_orchestrator = ModelConfig(...)
CLI->>CLI: validate_models_early()
CLI->>Config: settings.active_orchestrator_config
Note over CLI,Config: Redundant validation #1
Config->>Model: validate_api_key(ORCHESTRATOR)
CLI->>Main: main_async(...)
Main->>Main: _initialize_services_and_agent()
Main->>Main: _validate_provider_config()
Note over Main: Redundant validation #2
Note over User,Model: Result: Validation happens 3 times,<br/>breaks CLI model switching
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@vitali87 Hi! I've resolved the merge conflict and addressed the code review feedback from @greptile-apps and @gemini-code-assist. The PR is now ready for review. Thanks! |
…etters, use StrEnum constants
|
@vitali87 I have resolved all the suggestion form greptile and ready for review! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience by adding early validation for API keys and providing clear, helpful error messages when a key is missing. The changes are well-structured, with new providers and constants added cleanly.
I have two suggestions for improvement:
- In
codebase_rag/cli.py, consider refactoring duplicated code for model setting updates and validation into a helper function, while also considering the trade-off with readability that intentional duplication can sometimes offer. - In
codebase_rag/config.py, a condition for checking API keys can be made more concise and Pythonic.
Overall, this is a valuable enhancement to the project.
| try: | ||
| update_model_settings(orchestrator, cypher) | ||
| except ValueError as e: | ||
| app_context.console.print(style(str(e), cs.Color.RED)) | ||
| raise typer.Exit(1) from None | ||
|
|
||
| validate_models_early() |
There was a problem hiding this comment.
This block of code for updating and validating model settings is duplicated in the optimize function (lines 317-323). To improve maintainability and reduce redundancy, consider extracting this logic into a helper function. For example:
def _update_and_validate_models(orchestrator: str | None, cypher: str | None) -> None:
try:
update_model_settings(orchestrator, cypher)
except ValueError as e:
app_context.console.print(style(str(e), cs.Color.RED))
raise typer.Exit(1) from None
validate_models_early()You could then call _update_and_validate_models(orchestrator, cypher) in both start and optimize commands. However, remember that intentional code duplication can sometimes be preferable to abstraction if it improves readability by making the control flow for slightly different cases more explicit. This is a trade-off to be consciously made.
References
- Intentional code duplication may be preferable to abstraction if it improves readability by making the control flow for slightly different cases more explicit. This is a trade-off to be consciously made.
| not self.api_key | ||
| or self.api_key.strip() == "" |
There was a problem hiding this comment.
The condition to check for a missing or whitespace-only API key can be simplified. The expression not (self.api_key and self.api_key.strip()) is a more concise and Pythonic way to achieve the same result as not self.api_key or self.api_key.strip() == "". It correctly handles None, empty strings, and whitespace-only strings.
| not self.api_key | |
| or self.api_key.strip() == "" | |
| not (self.api_key and self.api_key.strip()) |
2 participants
Closes #243