Skip to content

F-029: feat(doctor): verify cloud provider connectivity #29

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.

Sign up for GitHub

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

Open
Sephyi wants to merge 1 commit into
base: development
Choose a base branch
from
Open

F-029: feat(doctor): verify cloud provider connectivity #29

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 67 additions & 5 deletions src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::path::PathBuf;
use console::style;
use dialoguer::{Confirm, Editor, Input, Select};
use globset::{Glob, GlobSetBuilder};
use secrecy::ExposeSecret;
use tokio::signal;
use tokio::sync::mpsc;
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -579,11 +580,34 @@ impl App {
}
}
other => {
eprint!(" {} API key: ", other);
if self.config.api_key.is_some() {
eprintln!("{}", style("configured").green());
} else {
eprintln!("{}", style("MISSING").red().bold());
eprint!(" {}: ", other);
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
);
}
Comment on lines +599 to +604
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Err(e) => {
eprintln!("{}: {}", style("ERROR").red().bold(), e);
}
},
}
}
Comment on lines +584 to +610
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
}
},

Copilot uses AI. Check for mistakes.
}
}
}
Expand Down Expand Up @@ -1499,6 +1523,23 @@ fi
}
}

/// Redact an API key for display, preserving only the last 4 characters.
///
/// Used by `doctor` to confirm which key is in effect without leaking it into
/// logs or terminal scrollback. Keys shorter than 8 characters are fully
/// masked since partial disclosure would be a large fraction of the secret.
fn redact_api_key(key: &str) -> String {
const SUFFIX_LEN: usize = 4;
const MIN_LEN_FOR_SUFFIX: usize = 8;

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();
Comment on lines +1535 to +1539
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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();

Copilot uses AI. Check for mistakes.
format!("****{tail}")
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -1524,4 +1565,25 @@ mod tests {
assert!(result.contains("Make it shorter"));
assert!(result.contains("Respond with ONLY the refined JSON object."));
}

#[test]
fn redact_api_key_masks_full_key_when_short() {
assert_eq!(redact_api_key(""), "****");
assert_eq!(redact_api_key("abc"), "****");
assert_eq!(redact_api_key("abcdefg"), "****");
}

#[test]
fn redact_api_key_exposes_only_last_four() {
assert_eq!(redact_api_key("sk-proj-abcdef1234"), "****1234");
assert_eq!(redact_api_key("abcdefgh"), "****efgh");
}

#[test]
fn redact_api_key_handles_multibyte_chars() {
// Ensure we don't slice through a UTF-8 codepoint boundary.
let key = "keyé🔑ABCD";
let redacted = redact_api_key(key);
assert_eq!(redacted, "****ABCD");
}
}
Loading