fix: friendly onboarding when API key missing#67
Conversation
Before: cryptic 'Error: set ANTHROPIC_API_KEY [MC-E001]' After: welcome box with setup instructions for all providers Shows: - Anthropic, OpenAI, Gemini, Groq, OpenRouter examples - Local model option (ollama, no key needed) - Persist hint (~/.bashrc) - Docs link
📝 WalkthroughWalkthroughThe CLI's provider initialization now captures provider creation failures and checks for missing API key errors. When detected, it displays a welcome message with environment variable setup instructions and exits cleanly with code 1, instead of propagating raw errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 introduces a user-friendly onboarding message that displays when a required LLM API key is missing, providing setup instructions for various providers. The review feedback suggests refining the error detection logic to prevent the message from appearing for invalid or expired keys, refactoring the extensive print statements into a dedicated helper function to improve code readability in the main function, and removing a redundant type conversion.
| if err_str.contains("MC-E001") | ||
| || err_str.contains("missing API key") | ||
| || err_str.contains("MissingApiKey") | ||
| || err_str.contains("API_KEY") |
There was a problem hiding this comment.
The check err_str.contains("API_KEY") is too broad. It will likely trigger the onboarding message for other errors, such as an invalid or expired key (e.g., [MC-E003] api error 401: Invalid API_KEY), which should be shown as a standard error to the user rather than a welcome message. Relying on the specific error code MC-E001 or the variant name MissingApiKey is more robust.
| if err_str.contains("MC-E001") | |
| || err_str.contains("missing API key") | |
| || err_str.contains("MissingApiKey") | |
| || err_str.contains("API_KEY") | |
| if err_str.contains("MC-E001") | |
| || err_str.contains("missing API key") | |
| || err_str.contains("MissingApiKey") |
| eprintln!(); | ||
| eprintln!(" ╭─────────────────────────────────────────╮"); | ||
| eprintln!(" │ Welcome to magic-code! 🚀 │"); | ||
| eprintln!(" ╰─────────────────────────────────────────╯"); | ||
| eprintln!(); | ||
| eprintln!(" To get started, set an API key for your LLM provider:"); | ||
| eprintln!(); | ||
| eprintln!(" # Anthropic (default)"); | ||
| eprintln!(" export ANTHROPIC_API_KEY=\"sk-ant-...\""); | ||
| eprintln!(); | ||
| eprintln!(" # Or use another provider:"); | ||
| eprintln!(" export OPENAI_API_KEY=\"sk-...\" # then: magic-code --provider openai"); | ||
| eprintln!(" export GEMINI_API_KEY=\"...\" # then: magic-code --provider gemini"); | ||
| eprintln!( | ||
| " export GROQ_API_KEY=\"gsk_...\" # then: magic-code --provider groq" | ||
| ); | ||
| eprintln!(" export OPENROUTER_API_KEY=\"sk-or-...\" # then: magic-code --provider openrouter"); | ||
| eprintln!(); | ||
| eprintln!(" # Or use a local model (no API key needed):"); | ||
| eprintln!(" magic-code --provider ollama --model llama3"); | ||
| eprintln!(); | ||
| eprintln!(" Add to ~/.bashrc or ~/.zshrc to persist."); | ||
| eprintln!(" Docs: https://github.com/kienbui1995/mc-code#install"); | ||
| eprintln!(); |
There was a problem hiding this comment.
This large block of eprintln! calls clutters the main function. Consider refactoring this onboarding message into a dedicated helper function to keep the high-level logic in main clean and focused.
| eprintln!(); | ||
| std::process::exit(1); | ||
| } | ||
| return Err(e.into()); |
There was a problem hiding this comment.
The call to .into() is redundant here because e is already an anyhow::Error and the function returns anyhow::Result<()>.
| return Err(e.into()); | |
| return Err(e); |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mc/crates/mc-cli/src/main.rs (1)
209-214:⚠️ Potential issue | 🟡 MinorTreat empty
--api-keyvalues as missing before provider creation.Right now
cli.api_key.as_deref()passesSome("")through. For the OpenAI/Groq/OpenRouter constructors inmc/crates/mc-cli/src/provider.rs, that is considered “set”, somagic-code --api-key ""skips the env fallback and never reaches this new onboarding flow.🛠️ Small normalization
let primary = provider::create_provider( &provider_name, &config.provider_config, cli.base_url.as_deref(), - cli.api_key.as_deref(), + cli.api_key.as_deref().filter(|key| !key.is_empty()), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-cli/src/main.rs` around lines 209 - 214, The call to provider::create_provider is passing cli.api_key.as_deref() which forwards Some("") as a set API key; normalize empty strings to None before provider creation by converting cli.api_key.as_deref() into an Option that returns None for empty strings (e.g., use .and_then / .filter on the Option<&str> to drop "") and pass that normalized option into provider::create_provider so empty --api-key values trigger env fallback and onboarding logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 209-214: The call to provider::create_provider is passing
cli.api_key.as_deref() which forwards Some("") as a set API key; normalize empty
strings to None before provider creation by converting cli.api_key.as_deref()
into an Option that returns None for empty strings (e.g., use .and_then /
.filter on the Option<&str> to drop "") and pass that normalized option into
provider::create_provider so empty --api-key values trigger env fallback and
onboarding logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 293dd334-2124-4c9c-aae1-05254f3fe8ba
📒 Files selected for processing (1)
mc/crates/mc-cli/src/main.rs
| let primary = match primary { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| let err_str = format!("{e:?}"); // Debug format includes full chain | ||
| if err_str.contains("MC-E001") | ||
| || err_str.contains("missing API key") | ||
| || err_str.contains("MissingApiKey") | ||
| || err_str.contains("API_KEY") | ||
| { | ||
| eprintln!(); | ||
| eprintln!(" ╭─────────────────────────────────────────╮"); | ||
| eprintln!(" │ Welcome to magic-code! 🚀 │"); | ||
| eprintln!(" ╰─────────────────────────────────────────╯"); | ||
| eprintln!(); | ||
| eprintln!(" To get started, set an API key for your LLM provider:"); | ||
| eprintln!(); | ||
| eprintln!(" # Anthropic (default)"); | ||
| eprintln!(" export ANTHROPIC_API_KEY=\"sk-ant-...\""); | ||
| eprintln!(); | ||
| eprintln!(" # Or use another provider:"); | ||
| eprintln!(" export OPENAI_API_KEY=\"sk-...\" # then: magic-code --provider openai"); | ||
| eprintln!(" export GEMINI_API_KEY=\"...\" # then: magic-code --provider gemini"); | ||
| eprintln!( | ||
| " export GROQ_API_KEY=\"gsk_...\" # then: magic-code --provider groq" | ||
| ); | ||
| eprintln!(" export OPENROUTER_API_KEY=\"sk-or-...\" # then: magic-code --provider openrouter"); | ||
| eprintln!(); | ||
| eprintln!(" # Or use a local model (no API key needed):"); | ||
| eprintln!(" magic-code --provider ollama --model llama3"); | ||
| eprintln!(); | ||
| eprintln!(" Add to ~/.bashrc or ~/.zshrc to persist."); | ||
| eprintln!(" Docs: https://github.com/kienbui1995/mc-code#install"); | ||
| eprintln!(); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
Preserve machine-readable failures when --json is set.
--json is documented for automation/scripting, but this branch always prints a human-only banner and exits. That breaks callers expecting structured output on auth failures.
⚙️ Minimal guard
let primary = match primary {
Ok(p) => p,
Err(e) => {
+ if cli.json {
+ return Err(e.into());
+ }
let err_str = format!("{e:?}"); // Debug format includes full chain
if err_str.contains("MC-E001")
|| err_str.contains("missing API key")
|| err_str.contains("MissingApiKey")
|| err_str.contains("API_KEY")
1 participant
Before:
After:
Summary by CodeRabbit
Bug Fixes