diff --git a/Cargo.lock b/Cargo.lock index 04d417a..a804b46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -674,7 +674,7 @@ checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" [[package]] name = "git-ai" -version = "1.1.0" +version = "1.1.1" dependencies = [ "anyhow", "async-openai", diff --git a/src/commit.rs b/src/commit.rs index efcf603..95aafd6 100644 --- a/src/commit.rs +++ b/src/commit.rs @@ -12,7 +12,6 @@ use crate::multi_step_integration::{generate_commit_message_local, generate_comm const INSTRUCTION_TEMPLATE: &str = include_str!("../resources/prompt.md"); /// Returns the instruction template for the AI model. -/// This template guides the model in generating appropriate commit messages. /// /// # Returns /// * `Result` - The rendered template or an error @@ -85,58 +84,32 @@ pub async fn generate(patch: String, remaining_tokens: usize, model: Model, sett .and_then(|s| s.max_commit_length) .or(config::APP_CONFIG.max_commit_length); - // Check if we have a valid API key configuration - let has_valid_api_key = if let Some(custom_settings) = settings { - custom_settings - .openai_api_key - .as_ref() - .map(|key| !key.is_empty() && key != "") - .unwrap_or(false) - } else { - // Check environment variable or config - config::APP_CONFIG - .openai_api_key - .as_ref() - .map(|key| !key.is_empty() && key != "") - .unwrap_or(false) - || std::env::var("OPENAI_API_KEY") - .map(|key| !key.is_empty()) - .unwrap_or(false) - }; - - if !has_valid_api_key { - bail!("OpenAI API key not configured. Please set your API key using:\n git-ai config set openai-api-key \nor set the OPENAI_API_KEY environment variable."); - } - // Use custom settings if provided if let Some(custom_settings) = settings { - if let Some(api_key) = &custom_settings.openai_api_key { - if !api_key.is_empty() && api_key != "" { - match openai::create_openai_config(custom_settings) { - Ok(config) => { - let client = Client::with_config(config); - let model_str = model.to_string(); + // Always try to create config - this will handle API key validation and fallback + match openai::create_openai_config(custom_settings) { + Ok(config) => { + let client = Client::with_config(config); + let model_str = model.to_string(); - match generate_commit_message_multi_step(&client, &model_str, &patch, max_length).await { - Ok(message) => return Ok(openai::Response { response: message }), - Err(e) => { - // Check if it's an API key error - if e.to_string().contains("invalid_api_key") || e.to_string().contains("Incorrect API key") { - bail!("Invalid OpenAI API key. Please check your API key configuration."); - } - log::warn!("Multi-step generation with custom settings failed: {e}"); - if let Some(session) = debug_output::debug_session() { - session.set_multi_step_error(e.to_string()); - } - } - } - } + match generate_commit_message_multi_step(&client, &model_str, &patch, max_length).await { + Ok(message) => return Ok(openai::Response { response: message }), Err(e) => { - // If config creation fails due to API key, propagate the error - return Err(e); + // Check if it's an API key error + if e.to_string().contains("invalid_api_key") || e.to_string().contains("Incorrect API key") { + bail!("Invalid OpenAI API key. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable"); + } + log::warn!("Multi-step generation with custom settings failed: {e}"); + if let Some(session) = debug_output::debug_session() { + session.set_multi_step_error(e.to_string()); + } } } } + Err(e) => { + // If config creation fails due to API key, propagate the error + return Err(e); + } } } else { // Try with default settings @@ -150,7 +123,7 @@ pub async fn generate(patch: String, remaining_tokens: usize, model: Model, sett Err(e) => { // Check if it's an API key error if e.to_string().contains("invalid_api_key") || e.to_string().contains("Incorrect API key") { - bail!("Invalid OpenAI API key. Please check your API key configuration."); + bail!("Invalid OpenAI API key. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable"); } log::warn!("Multi-step generation failed: {e}"); if let Some(session) = debug_output::debug_session() { @@ -247,7 +220,7 @@ mod tests { assert!(result.is_err()); let error_message = result.unwrap_err().to_string(); assert!( - error_message.contains("OpenAI API key not configured"), + error_message.contains("OpenAI API key not found") || error_message.contains("git-ai config set openai-api-key"), "Expected error message about missing API key, got: {}", error_message ); @@ -276,9 +249,44 @@ mod tests { assert!(result.is_err()); let error_message = result.unwrap_err().to_string(); assert!( - error_message.contains("OpenAI API key not configured"), + error_message.contains("OpenAI API key not found") || error_message.contains("git-ai config set openai-api-key"), "Expected error message about invalid API key, got: {}", error_message ); } + + #[test] + fn test_api_key_fallback_to_env() { + // Test the API key resolution logic without creating OpenAI config objects + + // Set a valid-looking API key in environment + std::env::set_var("OPENAI_API_KEY", "sk-test123456789012345678901234567890123456789012345678"); + + // Test case 1: Placeholder in config should fall back to env var + let placeholder_key = Some("".to_string()); + if let Some(key) = &placeholder_key { + if !key.is_empty() && key != "" { + panic!("Should not use placeholder key"); + } else { + // Should fall back to env var + let env_result = std::env::var("OPENAI_API_KEY"); + assert!(env_result.is_ok(), "Environment variable should be available"); + assert_eq!(env_result.unwrap(), "sk-test123456789012345678901234567890123456789012345678"); + } + } + + // Test case 2: Empty config should fall back to env var + let empty_key: Option = None; + if empty_key.is_none() { + let env_result = std::env::var("OPENAI_API_KEY"); + assert!(env_result.is_ok(), "Environment variable should be available for empty config"); + } + + // Clean up + std::env::remove_var("OPENAI_API_KEY"); + + // Verify environment variable is removed + let env_result_after_cleanup = std::env::var("OPENAI_API_KEY"); + assert!(env_result_after_cleanup.is_err(), "Environment variable should be removed"); + } } diff --git a/src/openai.rs b/src/openai.rs index 8125e83..7f6d4aa 100644 --- a/src/openai.rs +++ b/src/openai.rs @@ -111,13 +111,29 @@ pub async fn generate_commit_message(diff: &str) -> Result { /// Creates an OpenAI configuration from application settings pub fn create_openai_config(settings: &AppConfig) -> Result { - let api_key = settings - .openai_api_key - .as_ref() - .ok_or_else(|| anyhow!("OpenAI API key not configured"))?; + // Try config first, then environment variable + let api_key = if let Some(key) = &settings.openai_api_key { + if !key.is_empty() && key != "" { + key.clone() + } else { + // Try environment variable as fallback + std::env::var("OPENAI_API_KEY") + .map_err(|_| anyhow!( + "OpenAI API key not found. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable" + ))? + } + } else { + // No config key, try environment variable + std::env::var("OPENAI_API_KEY") + .map_err(|_| anyhow!( + "OpenAI API key not found. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable" + ))? + }; - if api_key.is_empty() || api_key == "" { - return Err(anyhow!("Invalid OpenAI API key")); + if api_key.is_empty() { + return Err(anyhow!( + "OpenAI API key cannot be empty. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable" + )); } let config = OpenAIConfig::new().with_api_key(api_key); @@ -342,7 +358,7 @@ pub async fn call_with_config(request: Request, config: OpenAIConfig) -> Result< // Check if it's an API key error - fail immediately without retrying if let OpenAIError::ApiError(ref api_err) = &last_error.as_ref().unwrap() { if api_err.code.as_deref() == Some("invalid_api_key") { - let error_msg = format!("Invalid OpenAI API key: {}", api_err.message); + let error_msg = format!("Invalid OpenAI API key: {}. Set via:\n1. git-ai config set openai-api-key \n2. OPENAI_API_KEY environment variable", api_err.message); log::error!("{error_msg}"); return Err(anyhow!(error_msg)); } diff --git a/tests/model_validation_test.rs b/tests/model_validation_test.rs index 6497b4e..510fa74 100644 --- a/tests/model_validation_test.rs +++ b/tests/model_validation_test.rs @@ -81,8 +81,8 @@ fn test_model_as_ref() { s.as_ref().to_string() } - assert_eq!(takes_str_ref(&Model::GPT41), "gpt-4.1"); - assert_eq!(takes_str_ref(&Model::GPT41Mini), "gpt-4.1-mini"); + assert_eq!(takes_str_ref(Model::GPT41), "gpt-4.1"); + assert_eq!(takes_str_ref(Model::GPT41Mini), "gpt-4.1-mini"); } #[test]