Skip to content

Commit 14c27a9

Browse files
committed
✨ Add Google/Gemini provider support to parallel analyze subagents
Extend SubagentRunner to support Google/Gemini as a provider option alongside OpenAI and Anthropic. Add resolve_gemini_client helper with the same API key resolution pattern (config → env → client default) and error sanitization. Also expand OpenAI API key validation to accept both 'sk-' and 'sk-proj-' prefixes via new api_key_prefixes() method, allowing project-scoped keys. Add api_key_if_set() helper to ProviderConfig for cleaner empty-string handling and refactor IrisAgent and IrisAgentService to use it. Includes comprehensive test coverage for multi-prefix validation, api_key_if_set behavior, and all-provider resolution paths.
1 parent 4019c3b commit 14c27a9

File tree

5 files changed

+179
-36
lines changed

5 files changed

+179
-36
lines changed

src/agents/iris.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -416,8 +416,7 @@ impl IrisAgent {
416416
self.config
417417
.as_ref()
418418
.and_then(|c| c.get_provider_config(&self.provider))
419-
.map(|pc| pc.api_key.as_str())
420-
.filter(|k| !k.is_empty())
419+
.and_then(|pc| pc.api_key_if_set())
421420
}
422421

423422
/// Build the actual agent for execution

src/agents/provider.rs

Lines changed: 63 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//! This module provides runtime provider selection using enum dispatch,
44
//! allowing git-iris to work with any supported provider based on config.
55
6-
use anyhow::{Context, Result};
6+
use anyhow::Result;
77
use rig::{
88
agent::{Agent, AgentBuilder, PromptResponse},
99
client::{CompletionClient, ProviderClient},
@@ -95,16 +95,16 @@ fn validate_and_warn(key: &str, provider: Provider, source: &str) {
9595
/// in config while still supporting env-only setups.
9696
pub fn resolve_api_key(api_key: Option<&str>, provider: Provider) -> (Option<String>, ApiKeySource) {
9797
// If explicit key provided and non-empty, use it
98-
if let Some(key) = api_key {
99-
if !key.is_empty() {
100-
tracing::trace!(
101-
provider = %provider,
102-
source = "config",
103-
"Using API key from configuration"
104-
);
105-
validate_and_warn(key, provider, "config");
106-
return (Some(key.to_string()), ApiKeySource::Config);
107-
}
98+
if let Some(key) = api_key
99+
&& !key.is_empty()
100+
{
101+
tracing::trace!(
102+
provider = %provider,
103+
source = "config",
104+
"Using API key from configuration"
105+
);
106+
validate_and_warn(key, provider, "config");
107+
return (Some(key.to_string()), ApiKeySource::Config);
108108
}
109109

110110
// Fall back to environment variable
@@ -138,11 +138,17 @@ pub fn resolve_api_key(api_key: Option<&str>, provider: Provider) -> (Option<Str
138138
///
139139
/// # Errors
140140
/// Returns an error if client creation fails (invalid credentials or missing env var).
141+
///
142+
/// # Security
143+
/// Error messages are sanitized to prevent potential API key exposure.
141144
pub fn openai_builder(model: &str, api_key: Option<&str>) -> Result<OpenAIBuilder> {
142145
let (resolved_key, _source) = resolve_api_key(api_key, Provider::OpenAI);
143146
let client = match resolved_key {
144147
Some(key) => openai::Client::new(&key)
145-
.context("Failed to create OpenAI client with provided credentials")?,
148+
// Sanitize error to prevent potential key exposure in error messages
149+
.map_err(|_| anyhow::anyhow!(
150+
"Failed to create OpenAI client: authentication or configuration error"
151+
))?,
146152
None => openai::Client::from_env(),
147153
};
148154
Ok(client.completions_api().agent(model))
@@ -159,11 +165,17 @@ pub fn openai_builder(model: &str, api_key: Option<&str>) -> Result<OpenAIBuilde
159165
///
160166
/// # Errors
161167
/// Returns an error if client creation fails (invalid credentials or missing env var).
168+
///
169+
/// # Security
170+
/// Error messages are sanitized to prevent potential API key exposure.
162171
pub fn anthropic_builder(model: &str, api_key: Option<&str>) -> Result<AnthropicBuilder> {
163172
let (resolved_key, _source) = resolve_api_key(api_key, Provider::Anthropic);
164173
let client = match resolved_key {
165174
Some(key) => anthropic::Client::new(&key)
166-
.context("Failed to create Anthropic client with provided credentials")?,
175+
// Sanitize error to prevent potential key exposure in error messages
176+
.map_err(|_| anyhow::anyhow!(
177+
"Failed to create Anthropic client: authentication or configuration error"
178+
))?,
167179
None => anthropic::Client::from_env(),
168180
};
169181
Ok(client.agent(model))
@@ -180,11 +192,17 @@ pub fn anthropic_builder(model: &str, api_key: Option<&str>) -> Result<Anthropic
180192
///
181193
/// # Errors
182194
/// Returns an error if client creation fails (invalid credentials or missing env var).
195+
///
196+
/// # Security
197+
/// Error messages are sanitized to prevent potential API key exposure.
183198
pub fn gemini_builder(model: &str, api_key: Option<&str>) -> Result<GeminiBuilder> {
184199
let (resolved_key, _source) = resolve_api_key(api_key, Provider::Google);
185200
let client = match resolved_key {
186201
Some(key) => gemini::Client::new(&key)
187-
.context("Failed to create Gemini client with provided credentials")?,
202+
// Sanitize error to prevent potential key exposure in error messages
203+
.map_err(|_| anyhow::anyhow!(
204+
"Failed to create Gemini client: authentication or configuration error"
205+
))?,
188206
None => gemini::Client::from_env(),
189207
};
190208
Ok(client.agent(model))
@@ -242,4 +260,35 @@ mod tests {
242260
assert_eq!(ApiKeySource::ClientDefault, ApiKeySource::ClientDefault);
243261
assert_ne!(ApiKeySource::Config, ApiKeySource::Environment);
244262
}
263+
264+
#[test]
265+
fn test_resolve_api_key_all_providers() {
266+
// Test that resolve_api_key works for all supported providers
267+
for provider in Provider::ALL {
268+
let (key, source) =
269+
resolve_api_key(Some("test-key-123456789012345"), *provider);
270+
assert_eq!(key, Some("test-key-123456789012345".to_string()));
271+
assert_eq!(source, ApiKeySource::Config);
272+
}
273+
}
274+
275+
#[test]
276+
fn test_resolve_api_key_config_precedence() {
277+
// Even if env var is set, config should take precedence
278+
// We can't easily mock env vars in unit tests, but we can verify
279+
// that a provided config key is always used regardless of env state
280+
let config_key = "sk-from-config-abcdef1234567890";
281+
let (key, source) = resolve_api_key(Some(config_key), Provider::OpenAI);
282+
283+
assert_eq!(key.as_deref(), Some(config_key));
284+
assert_eq!(source, ApiKeySource::Config);
285+
}
286+
287+
#[test]
288+
fn test_api_key_source_debug_impl() {
289+
// Verify Debug is implemented for logging purposes
290+
let source = ApiKeySource::Config;
291+
let debug_str = format!("{:?}", source);
292+
assert!(debug_str.contains("Config"));
293+
}
245294
}

src/agents/setup.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ impl IrisAgentService {
488488
pub fn api_key(&self) -> Option<String> {
489489
self.config
490490
.get_provider_config(&self.provider)
491-
.filter(|pc| !pc.api_key.is_empty())
492-
.map(|pc| pc.api_key.clone())
491+
.and_then(|pc| pc.api_key_if_set())
492+
.map(String::from)
493493
}
494494
}

src/agents/tools/parallel_analyze.rs

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use anyhow::Result;
88
use rig::{
99
client::{CompletionClient, ProviderClient},
1010
completion::{Prompt, ToolDefinition},
11-
providers::{anthropic, openai},
11+
providers::{anthropic, gemini, openai},
1212
tool::Tool,
1313
};
1414
use schemars::JsonSchema;
@@ -71,6 +71,10 @@ enum SubagentRunner {
7171
client: anthropic::Client,
7272
model: String,
7373
},
74+
Gemini {
75+
client: gemini::Client,
76+
model: String,
77+
},
7478
}
7579

7680
impl SubagentRunner {
@@ -90,14 +94,21 @@ impl SubagentRunner {
9094
model: model.to_string(),
9195
})
9296
}
97+
"google" | "gemini" => {
98+
let client = Self::resolve_gemini_client(api_key)?;
99+
Ok(Self::Gemini {
100+
client,
101+
model: model.to_string(),
102+
})
103+
}
93104
_ => Err(anyhow::anyhow!(
94-
"Unsupported provider for parallel analysis: {}",
105+
"Unsupported provider for parallel analysis: {}. Supported: openai, anthropic, google",
95106
provider
96107
)),
97108
}
98109
}
99110

100-
/// Create OpenAI client using shared resolution logic
111+
/// Create `OpenAI` client using shared resolution logic
101112
///
102113
/// Uses `resolve_api_key` from provider module to maintain consistent
103114
/// resolution order: config → env var → client default
@@ -111,7 +122,7 @@ impl SubagentRunner {
111122
}
112123
}
113124

114-
/// Create Anthropic client using shared resolution logic
125+
/// Create `Anthropic` client using shared resolution logic
115126
///
116127
/// Uses `resolve_api_key` from provider module to maintain consistent
117128
/// resolution order: config → env var → client default
@@ -125,6 +136,20 @@ impl SubagentRunner {
125136
}
126137
}
127138

139+
/// Create `Gemini` client using shared resolution logic
140+
///
141+
/// Uses `resolve_api_key` from provider module to maintain consistent
142+
/// resolution order: config → env var → client default
143+
fn resolve_gemini_client(api_key: Option<&str>) -> Result<gemini::Client> {
144+
let (resolved_key, _source) = resolve_api_key(api_key, Provider::Google);
145+
match resolved_key {
146+
Some(key) => gemini::Client::new(&key)
147+
// Sanitize error to avoid exposing key material
148+
.map_err(|_| anyhow::anyhow!("Failed to create Gemini client: authentication or configuration error")),
149+
None => Ok(gemini::Client::from_env()),
150+
}
151+
}
152+
128153
async fn run_task(&self, task: &str) -> SubagentResult {
129154
let preamble = "You are a specialized analysis sub-agent. Complete the assigned \
130155
task thoroughly and return a focused summary.\n\n\
@@ -146,6 +171,11 @@ impl SubagentRunner {
146171
let agent = crate::attach_core_tools!(builder).build();
147172
agent.prompt(task).await
148173
}
174+
Self::Gemini { client, model } => {
175+
let builder = client.agent(model).preamble(preamble).max_tokens(4096);
176+
let agent = crate::attach_core_tools!(builder).build();
177+
agent.prompt(task).await
178+
}
149179
};
150180

151181
match result {

src/providers.rs

Lines changed: 79 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,22 +66,33 @@ impl Provider {
6666
}
6767
}
6868

69-
/// Expected API key prefix for basic format validation
69+
/// Valid API key prefixes for format validation
7070
///
71-
/// Returns the expected prefix for the provider's API keys, if known.
72-
/// This helps catch typos and misconfigurations early.
71+
/// Returns the expected prefixes for the provider's API keys.
72+
/// `OpenAI` has multiple valid prefixes (sk-, sk-proj-).
73+
pub fn api_key_prefixes(&self) -> &'static [&'static str] {
74+
match self {
75+
Self::OpenAI => &["sk-", "sk-proj-"],
76+
Self::Anthropic => &["sk-ant-"],
77+
Self::Google => &[], // Google API keys don't have a consistent prefix
78+
}
79+
}
80+
81+
/// Expected API key prefix for basic format validation (primary prefix)
82+
///
83+
/// Returns the primary expected prefix for display in error messages.
7384
pub const fn api_key_prefix(&self) -> Option<&'static str> {
7485
match self {
7586
Self::OpenAI => Some("sk-"),
7687
Self::Anthropic => Some("sk-ant-"),
77-
Self::Google => None, // Google API keys don't have a consistent prefix
88+
Self::Google => None,
7889
}
7990
}
8091

8192
/// Validate API key format
8293
///
8394
/// Performs basic validation to catch obvious misconfigurations:
84-
/// - Checks for expected prefix (OpenAI: `sk-`, Anthropic: `sk-ant-`)
95+
/// - Checks for expected prefix (`OpenAI`: `sk-` or `sk-proj-`, `Anthropic`: `sk-ant-`)
8596
/// - Ensures key is not suspiciously short
8697
///
8798
/// Returns `Ok(())` if valid, or a warning message if potentially invalid.
@@ -97,15 +108,23 @@ impl Provider {
97108
));
98109
}
99110

100-
// Check expected prefix
101-
if let Some(prefix) = self.api_key_prefix() {
102-
if !key.starts_with(prefix) {
103-
return Err(format!(
104-
"{} API key should start with '{}' (key has unexpected prefix)",
105-
self.name(),
106-
prefix
107-
));
108-
}
111+
// Check expected prefixes
112+
let prefixes = self.api_key_prefixes();
113+
if !prefixes.is_empty() && !prefixes.iter().any(|p| key.starts_with(p)) {
114+
let expected = if prefixes.len() == 1 {
115+
format!("'{}'", prefixes[0])
116+
} else {
117+
prefixes
118+
.iter()
119+
.map(|p| format!("'{p}'"))
120+
.collect::<Vec<_>>()
121+
.join(" or ")
122+
};
123+
return Err(format!(
124+
"{} API key should start with {} (key has unexpected prefix)",
125+
self.name(),
126+
expected
127+
));
109128
}
110129

111130
Ok(())
@@ -210,6 +229,19 @@ impl ProviderConfig {
210229
pub fn has_api_key(&self) -> bool {
211230
!self.api_key.is_empty()
212231
}
232+
233+
/// Get API key if set (non-empty), otherwise None
234+
///
235+
/// This is the canonical way to extract an API key for passing to
236+
/// provider builders. Returns `None` for empty strings, allowing
237+
/// fallback to environment variables.
238+
pub fn api_key_if_set(&self) -> Option<&str> {
239+
if self.api_key.is_empty() {
240+
None
241+
} else {
242+
Some(&self.api_key)
243+
}
244+
}
213245
}
214246

215247
#[cfg(test)]
@@ -251,13 +283,44 @@ mod tests {
251283
assert_eq!(Provider::Google.api_key_prefix(), None);
252284
}
253285

286+
#[test]
287+
fn test_api_key_if_set() {
288+
// Non-empty key returns Some
289+
let mut config = ProviderConfig::with_defaults(Provider::OpenAI);
290+
config.api_key = "sk-test-key-12345678901234567890".to_string();
291+
assert_eq!(
292+
config.api_key_if_set(),
293+
Some("sk-test-key-12345678901234567890")
294+
);
295+
296+
// Empty key returns None
297+
config.api_key = String::new();
298+
assert_eq!(config.api_key_if_set(), None);
299+
}
300+
301+
#[test]
302+
fn test_api_key_prefixes() {
303+
// OpenAI accepts multiple prefixes
304+
assert_eq!(Provider::OpenAI.api_key_prefixes(), &["sk-", "sk-proj-"]);
305+
assert_eq!(Provider::Anthropic.api_key_prefixes(), &["sk-ant-"]);
306+
assert!(Provider::Google.api_key_prefixes().is_empty());
307+
}
308+
254309
#[test]
255310
fn test_api_key_validation_valid_openai() {
256311
// Valid OpenAI key format (starts with sk-, long enough)
257312
let result = Provider::OpenAI.validate_api_key_format("sk-1234567890abcdefghijklmnop");
258313
assert!(result.is_ok());
259314
}
260315

316+
#[test]
317+
fn test_api_key_validation_valid_openai_project_key() {
318+
// Valid OpenAI project key format (starts with sk-proj-, long enough)
319+
let result =
320+
Provider::OpenAI.validate_api_key_format("sk-proj-1234567890abcdefghijklmnop");
321+
assert!(result.is_ok());
322+
}
323+
261324
#[test]
262325
fn test_api_key_validation_valid_anthropic() {
263326
// Valid Anthropic key format (starts with sk-ant-, long enough)
@@ -287,6 +350,8 @@ mod tests {
287350
assert!(result.is_err());
288351
let err = result.unwrap_err();
289352
assert!(err.contains("should start with"));
353+
// Error should mention valid prefixes
354+
assert!(err.contains("'sk-'") || err.contains("'sk-proj-'"));
290355
// Verify we don't expose the actual key prefix in error messages
291356
assert!(!err.contains("wrong-"));
292357
}

0 commit comments

Comments
 (0)