F-029: feat(doctor): verify cloud provider connectivity#29
F-029: feat(doctor): verify cloud provider connectivity#29Sephyi wants to merge 1 commit intodevelopmentfrom
Conversation
…esence) `commitbee doctor` previously reported OpenAI/Anthropic as "configured" whenever `config.api_key.is_some()`, without checking that the key was actually valid or that the provider was reachable. Only Ollama got a real `provider.verify().await` health check. Extend the cloud-provider branch of `run_doctor` to: - call `llm::create_provider(&self.config)` and then `provider.verify()` for the configured cloud provider, - on success print `reachable (key '****<last-4>')` so operators can confirm which key is in effect without leaking the full secret, - on error pretty-print the failure on stderr with the same `ERROR: <e>` style used by the Ollama fallback arm, - preserve the existing "no key" case and report it clearly without attempting a network call. Network failures remain non-fatal — `doctor` is diagnostic, so one failed provider probe must not abort the rest of the report (git-repo check still runs). A small `redact_api_key` helper keeps the secret-redaction logic UTF-8-safe (tests cover empty, short, normal, and multi-byte keys). Closes audit entry F-029 from #3.
There was a problem hiding this comment.
Pull request overview
Adds cloud-provider connectivity verification to the doctor command so users can confirm their configured LLM provider is reachable before running generation flows (addresses audit finding F-029).
Changes:
- Extend
doctor’s Provider Check to verify non-Ollama providers viaprovider.verify().await. - Add API key redaction helper used by
doctor, plus unit tests for masking behavior (including UTF-8 safety).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| eprintln!( | ||
| "{} (key '{}')", | ||
| style("reachable").green().bold(), | ||
| redacted | ||
| ); | ||
| } |
There was a problem hiding this comment.
doctor prints a redacted API key suffix ((key '****1234')). Even partial disclosure of an API key can be sensitive (it can help correlate keys across logs/screenshots and increases exposure surface), and it also contradicts the doc comment’s goal of avoiding leaking secrets into terminal scrollback.
Recommendation: don’t print any part of the key by default (just "configured"), or gate suffix/fingerprint display behind an explicit opt-in flag (e.g., --show-secrets) or an interactive confirmation.
| let char_count = key.chars().count(); | ||
| if char_count < MIN_LEN_FOR_SUFFIX { | ||
| return "****".to_string(); | ||
| } | ||
| let tail: String = key.chars().skip(char_count - SUFFIX_LEN).collect(); |
There was a problem hiding this comment.
redact_api_key walks the string twice (chars().count() and then chars().skip(...)), which is unnecessary and makes the function a bit harder to reason about. You can compute the last 4 characters in a single pass (e.g., via a reverse iterator) while still staying UTF-8 safe.
| let char_count = key.chars().count(); | |
| if char_count < MIN_LEN_FOR_SUFFIX { | |
| return "****".to_string(); | |
| } | |
| let tail: String = key.chars().skip(char_count - SUFFIX_LEN).collect(); | |
| let tail_chars: Vec<char> = key.chars().rev().take(SUFFIX_LEN).collect(); | |
| if tail_chars.len() < SUFFIX_LEN | |
| || key.chars().nth(MIN_LEN_FOR_SUFFIX - SUFFIX_LEN).is_none() | |
| { | |
| return "****".to_string(); | |
| } | |
| let tail: String = tail_chars.into_iter().rev().collect(); |
| match self.config.api_key.as_ref() { | ||
| None => { | ||
| eprintln!("{} (no API key configured)", style("MISSING").red().bold()); | ||
| } | ||
| Some(key) => { | ||
| let redacted = redact_api_key(key.expose_secret()); | ||
| // Build the provider to reach verify(). Construction itself | ||
| // can fail (e.g. HTTP client build); keep that non-fatal so | ||
| // the rest of `doctor` still runs. | ||
| match llm::create_provider(&self.config) { | ||
| Err(e) => { | ||
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | ||
| } | ||
| Ok(provider) => match provider.verify().await { | ||
| Ok(()) => { | ||
| eprintln!( | ||
| "{} (key '{}')", | ||
| style("reachable").green().bold(), | ||
| redacted | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
run_doctor can print "MISSING (no API key configured)", but Config::load() currently calls config.validate() which errors for Commands::Doctor when provider != Ollama and api_key is None (see src/config.rs:380-401). As a result, this branch is effectively unreachable in normal execution, and doctor can’t be used to diagnose a missing API key.
Consider relaxing validation for doctor (don’t require an API key for this subcommand) and letting run_doctor report the missing-key condition, or bypassing validation when the command is Doctor and moving the check into run_split_flow/generation paths instead.
| match self.config.api_key.as_ref() { | |
| None => { | |
| eprintln!("{} (no API key configured)", style("MISSING").red().bold()); | |
| } | |
| Some(key) => { | |
| let redacted = redact_api_key(key.expose_secret()); | |
| // Build the provider to reach verify(). Construction itself | |
| // can fail (e.g. HTTP client build); keep that non-fatal so | |
| // the rest of `doctor` still runs. | |
| match llm::create_provider(&self.config) { | |
| Err(e) => { | |
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | |
| } | |
| Ok(provider) => match provider.verify().await { | |
| Ok(()) => { | |
| eprintln!( | |
| "{} (key '{}')", | |
| style("reachable").green().bold(), | |
| redacted | |
| ); | |
| } | |
| Err(e) => { | |
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | |
| } | |
| }, | |
| } | |
| } | |
| let key = self | |
| .config | |
| .api_key | |
| .as_ref() | |
| .expect("non-Ollama providers are validated to have an API key before doctor runs"); | |
| let redacted = redact_api_key(key.expose_secret()); | |
| // Build the provider to reach verify(). Construction itself | |
| // can fail (e.g. HTTP client build); keep that non-fatal so | |
| // the rest of `doctor` still runs. | |
| match llm::create_provider(&self.config) { | |
| Err(e) => { | |
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | |
| } | |
| Ok(provider) => match provider.verify().await { | |
| Ok(()) => { | |
| eprintln!( | |
| "{} (key '{}')", | |
| style("reachable").green().bold(), | |
| redacted | |
| ); | |
| } | |
| Err(e) => { | |
| eprintln!("{}: {}", style("ERROR").red().bold(), e); | |
| } | |
| }, |
Summary
feat(doctor): verify cloud provider connectivity.
Audit context
Closes audit entry F-029 from #3.
Verification
cargo fmt --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets