From 10419c7264352d5b0f57cf940d8add05907b7ec2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 11:09:32 -0400 Subject: [PATCH] refactor(auth): simplify credential source reuse Reuse credential-source APIs across provider discovery and tests, target known-provider LLM client resolution, and remove duplicated hook/source plumbing from the review cleanup. --- Cargo.lock | 1 - docs/reference/sdk.mdx | 7 +- lib/crates/fabro-agent/src/cli.rs | 23 ++++-- .../fabro-auth/src/credential_source.rs | 14 ++++ lib/crates/fabro-auth/src/env_source.rs | 16 ++++ lib/crates/fabro-auth/src/resolve.rs | 35 ++++---- lib/crates/fabro-auth/src/vault_source.rs | 79 ++++++++----------- lib/crates/fabro-hooks/src/bridge.rs | 1 - lib/crates/fabro-hooks/src/executor.rs | 64 ++++++--------- lib/crates/fabro-hooks/src/runner.rs | 24 +----- lib/crates/fabro-llm/src/client.rs | 22 ++++++ lib/crates/fabro-llm/src/generate.rs | 42 +++++----- lib/crates/fabro-server/src/server.rs | 42 +++++----- lib/crates/fabro-workflow/Cargo.toml | 1 - .../fabro-workflow/src/handler/llm/api.rs | 65 ++++++++------- .../fabro-workflow/src/pipeline/initialize.rs | 29 +++---- .../src/pipeline/pull_request.rs | 62 ++++----------- .../fabro-workflow/tests/it/integration.rs | 62 ++++----------- 18 files changed, 266 insertions(+), 323 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d142e8df9..5d9b3ef58 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2309,7 +2309,6 @@ dependencies = [ "futures", "git2", "hex", - "httpmock", "md5", "mime_guess", "object_store", diff --git a/docs/reference/sdk.mdx b/docs/reference/sdk.mdx index c23e2232a..b3e60fdb8 100644 --- a/docs/reference/sdk.mdx +++ b/docs/reference/sdk.mdx @@ -36,12 +36,12 @@ use std::sync::Arc; #[tokio::main] async fn main() -> Result<(), Box> { let source = EnvCredentialSource::new(); - let client = Client::from_source(&source).await?.as_ref().clone(); + let client = Client::from_source(&source).await?; let sandbox = Arc::new(LocalSandbox::new(PathBuf::from("."))); let profile = Arc::new(AnthropicProfile::new("claude-sonnet-4-5")); let config = SessionOptions::default(); - let mut session = Session::new(client, profile, sandbox, config); + let mut session = Session::new(client, profile, sandbox, config, None); session.initialize().await; // Subscribe to events before sending input @@ -72,6 +72,7 @@ pub fn new( provider_profile: Arc, sandbox: Arc, config: SessionOptions, + subagent_manager: Option>>, ) -> Self ``` @@ -447,7 +448,7 @@ You cannot use both `.prompt()` and `.messages()` on the same request — this r | Method | Type | Description | |---|---|---| -| `new(model, client)` | `(impl Into, Arc)` | Required. Model ID or alias plus the client to use | +| `new(model, client)` | `(impl Into, impl Into>)` | Required. Model ID or alias plus the client to use | | `.prompt(text)` | `impl Into` | Convenience: sends a single user message | | `.messages(msgs)` | `Vec` | Full conversation history | | `.system(text)` | `impl Into` | System prompt | diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 1ce8f4870..331bb2f9b 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -448,11 +448,11 @@ pub async fn run_with_args_and_source( mcp_servers: Vec, ) -> anyhow::Result<()> { let provider = parse_provider(&args)?; - let client = Client::from_source(llm_source.as_ref()) + let client = Client::from_source_for(llm_source.as_ref(), &[provider]) .await .map_err(|e| anyhow::anyhow!("Failed to create LLM client: {e}"))?; ensure_provider_registered(&client, provider)?; - run_with_args_and_client(args, client, mcp_servers).await + run_with_resolved_provider(args, provider, client, mcp_servers).await } #[allow( @@ -462,6 +462,22 @@ pub async fn run_with_args_and_source( )] pub async fn run_with_args_and_client( args: AgentArgs, + client: Client, + mcp_servers: Vec, +) -> anyhow::Result<()> { + let provider = parse_provider(&args)?; + ensure_provider_registered(&client, provider)?; + run_with_resolved_provider(args, provider, client, mcp_servers).await +} + +#[allow( + clippy::print_stdout, + clippy::print_stderr, + reason = "Assistant output stays on stdout while prompts and diagnostics use stderr." +)] +async fn run_with_resolved_provider( + args: AgentArgs, + provider: Provider, mut client: Client, mcp_servers: Vec, ) -> anyhow::Result<()> { @@ -469,9 +485,6 @@ pub async fn run_with_args_and_client( // threads let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - let provider = parse_provider(&args)?; - ensure_provider_registered(&client, provider)?; - if args.verbose { client.add_middleware(Arc::new(VerboseMiddleware { styles })); } else if args.debug { diff --git a/lib/crates/fabro-auth/src/credential_source.rs b/lib/crates/fabro-auth/src/credential_source.rs index bba9f4600..ff554bf22 100644 --- a/lib/crates/fabro-auth/src/credential_source.rs +++ b/lib/crates/fabro-auth/src/credential_source.rs @@ -13,5 +13,19 @@ pub struct ResolvedCredentials { pub trait CredentialSource: Send + Sync { async fn resolve(&self) -> anyhow::Result; + async fn resolve_providers( + &self, + providers: &[Provider], + ) -> anyhow::Result { + let mut resolved = self.resolve().await?; + resolved + .credentials + .retain(|credential| providers.contains(&credential.provider)); + resolved + .auth_issues + .retain(|(provider, _)| providers.contains(provider)); + Ok(resolved) + } + async fn configured_providers(&self) -> Vec; } diff --git a/lib/crates/fabro-auth/src/env_source.rs b/lib/crates/fabro-auth/src/env_source.rs index c387a1e84..fe95fd1ac 100644 --- a/lib/crates/fabro-auth/src/env_source.rs +++ b/lib/crates/fabro-auth/src/env_source.rs @@ -89,6 +89,22 @@ impl CredentialSource for EnvCredentialSource { }) } + async fn resolve_providers( + &self, + providers: &[Provider], + ) -> anyhow::Result { + let credentials = providers + .iter() + .copied() + .filter_map(|provider| self.credential_for(provider)) + .collect(); + + Ok(ResolvedCredentials { + credentials, + auth_issues: Vec::new(), + }) + } + async fn configured_providers(&self) -> Vec { Provider::ALL .iter() diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index 03643d6c8..5b8512a30 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -8,6 +8,8 @@ use tokio::sync::RwLock as AsyncRwLock; use tokio::task::spawn_blocking; use crate::credential::{ApiKeyHeader, AuthCredential, AuthDetails, credential_id_for}; +use crate::credential_source::CredentialSource; +use crate::env_source::EnvCredentialSource; use crate::refresh::refresh_oauth_credential; use crate::vault_ext::{vault_get_credential, vault_set_credential}; @@ -180,8 +182,12 @@ impl CredentialResolver { } } - #[must_use] - pub fn configured_providers(&self, vault: &Vault) -> Vec { + pub async fn configured_providers(&self) -> Vec { + let vault = self.vault.read().await; + self.configured_providers_for_vault(&vault) + } + + fn configured_providers_for_vault(&self, vault: &Vault) -> Vec { Provider::ALL .iter() .copied() @@ -312,20 +318,11 @@ pub async fn configured_providers_from_process_env( ) -> Vec { match vault { Some(vault_arc) => { - let resolver = CredentialResolver::new(Arc::clone(vault_arc)); - let guard = vault_arc.read().await; - resolver.configured_providers(&guard) + CredentialResolver::new(Arc::clone(vault_arc)) + .configured_providers() + .await } - None => Provider::ALL - .iter() - .copied() - .filter(|provider| { - provider - .api_key_env_vars() - .iter() - .any(|env_var| std::env::var(env_var).is_ok()) - }) - .collect(), + None => EnvCredentialSource::new().configured_providers().await, } } @@ -726,9 +723,7 @@ mod tests { ) .unwrap(); let resolver = test_resolver(vault, Arc::new(|_| None)); - let vault = resolver.vault.read().await; - - assert_eq!(resolver.configured_providers(&vault), vec![ + assert_eq!(resolver.configured_providers().await, vec![ Provider::OpenAi ]); } @@ -741,9 +736,7 @@ mod tests { vault, Arc::new(|name| (name == "OPENAI_API_KEY").then(|| "env-key".to_string())), ); - let vault = resolver.vault.read().await; - - assert_eq!(resolver.configured_providers(&vault), vec![ + assert_eq!(resolver.configured_providers().await, vec![ Provider::OpenAi ]); } diff --git a/lib/crates/fabro-auth/src/vault_source.rs b/lib/crates/fabro-auth/src/vault_source.rs index 052171fc1..4fc797805 100644 --- a/lib/crates/fabro-auth/src/vault_source.rs +++ b/lib/crates/fabro-auth/src/vault_source.rs @@ -10,15 +10,14 @@ use crate::{CredentialResolver, CredentialUsage, EnvLookup, ResolveError, Resolv #[derive(Clone)] pub struct VaultCredentialSource { - vault: Arc>, resolver: CredentialResolver, } impl VaultCredentialSource { #[must_use] pub fn new(vault: Arc>) -> Self { - let resolver = CredentialResolver::new(Arc::clone(&vault)); - Self { vault, resolver } + let resolver = CredentialResolver::new(vault); + Self { resolver } } #[must_use] @@ -27,8 +26,8 @@ impl VaultCredentialSource { F: Fn(&str) -> Option + Send + Sync + 'static, { let env_lookup: EnvLookup = Arc::new(env_lookup); - let resolver = CredentialResolver::with_env_lookup(Arc::clone(&vault), env_lookup); - Self { vault, resolver } + let resolver = CredentialResolver::with_env_lookup(vault, env_lookup); + Self { resolver } } } @@ -42,10 +41,17 @@ impl std::fmt::Debug for VaultCredentialSource { #[async_trait] impl CredentialSource for VaultCredentialSource { async fn resolve(&self) -> anyhow::Result { + self.resolve_providers(Provider::ALL).await + } + + async fn resolve_providers( + &self, + providers: &[Provider], + ) -> anyhow::Result { let mut credentials = Vec::new(); let mut auth_issues = Vec::new(); - for provider in Provider::ALL { + for provider in providers { match self .resolver .resolve(*provider, CredentialUsage::ApiRequest) @@ -64,8 +70,7 @@ impl CredentialSource for VaultCredentialSource { } async fn configured_providers(&self) -> Vec { - let vault = self.vault.read().await; - self.resolver.configured_providers(&vault) + self.resolver.configured_providers().await } } @@ -75,12 +80,12 @@ mod tests { use chrono::{Duration, Utc}; use fabro_model::Provider; - use fabro_vault::{SecretType, Vault}; + use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; use super::VaultCredentialSource; use crate::credential::{AuthCredential, AuthDetails, OAuthConfig, OAuthTokens}; - use crate::{CredentialSource, ResolveError}; + use crate::{CredentialSource, ResolveError, vault_set_credential}; fn api_key_credential(provider: Provider, key: &str) -> AuthCredential { AuthCredential { @@ -117,23 +122,13 @@ mod tests { async fn resolve_returns_credentials_and_auth_issues() { let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "openai_codex", - &serde_json::to_string(&expired_openai_credential()).unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); - vault - .set( - "anthropic", - &serde_json::to_string(&api_key_credential(Provider::Anthropic, "anthropic-key")) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); + vault_set_credential(&mut vault, "openai_codex", &expired_openai_credential()).unwrap(); + vault_set_credential( + &mut vault, + "anthropic", + &api_key_credential(Provider::Anthropic, "anthropic-key"), + ) + .unwrap(); let source = VaultCredentialSource::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |_| None); @@ -156,24 +151,18 @@ mod tests { async fn configured_providers_reads_from_vault_without_refreshing() { let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "openai", - &serde_json::to_string(&api_key_credential(Provider::OpenAi, "openai-key")) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); - vault - .set( - "anthropic", - &serde_json::to_string(&api_key_credential(Provider::Anthropic, "anthropic-key")) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); + vault_set_credential( + &mut vault, + "openai", + &api_key_credential(Provider::OpenAi, "openai-key"), + ) + .unwrap(); + vault_set_credential( + &mut vault, + "anthropic", + &api_key_credential(Provider::Anthropic, "anthropic-key"), + ) + .unwrap(); let source = VaultCredentialSource::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |_| None); diff --git a/lib/crates/fabro-hooks/src/bridge.rs b/lib/crates/fabro-hooks/src/bridge.rs index a762a53d7..0c51172a5 100644 --- a/lib/crates/fabro-hooks/src/bridge.rs +++ b/lib/crates/fabro-hooks/src/bridge.rs @@ -95,7 +95,6 @@ mod tests { context: &HookContext, _sandbox: Arc, _work_dir: Option<&Path>, - _llm_source: &dyn fabro_auth::CredentialSource, ) -> HookResult { self.captured_contexts.lock().unwrap().push(context.clone()); HookResult { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index dbdc86021..09b8bce32 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -49,7 +49,6 @@ pub trait HookExecutor: Send + Sync { context: &HookContext, sandbox: Arc, work_dir: Option<&Path>, - llm_source: &dyn CredentialSource, ) -> HookResult; } @@ -76,9 +75,16 @@ where } /// Executes hooks via shell commands or HTTP POST. -pub struct HookExecutorImpl; +pub struct HookExecutorImpl { + llm_source: Arc, +} impl HookExecutorImpl { + #[must_use] + pub fn new(llm_source: Arc) -> Self { + Self { llm_source } + } + /// Parse a hook decision from JSON stdout and exit code. fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision { if exit_code == 0 { @@ -616,7 +622,6 @@ impl HookExecutor for HookExecutorImpl { context: &HookContext, sandbox: Arc, work_dir: Option<&Path>, - llm_source: &dyn CredentialSource, ) -> HookResult { use std::sync::OnceLock; static HTTP_CLIENTS: OnceLock = OnceLock::new(); @@ -674,7 +679,7 @@ impl HookExecutor for HookExecutorImpl { model.as_deref(), context, &env, - llm_source, + self.llm_source.as_ref(), ) .await } @@ -698,7 +703,7 @@ impl HookExecutor for HookExecutorImpl { context, sandbox, &env, - llm_source, + self.llm_source.as_ref(), ) .await } @@ -816,54 +821,42 @@ mod tests { #[tokio::test] async fn command_executor_host_success() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = make_definition("exit 0"); let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert_eq!(result.decision, HookDecision::Proceed); assert_eq!(result.hook_name.as_deref(), Some("test-hook")); } #[tokio::test] async fn command_executor_host_failure() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = make_definition("exit 1"); let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert!(matches!(result.decision, HookDecision::Block { .. })); } #[tokio::test] async fn command_executor_host_skip_via_exit_2() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = make_definition("exit 2"); let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert!(matches!(result.decision, HookDecision::Block { .. })); } #[tokio::test] async fn command_executor_host_json_decision() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = make_definition(r#"echo '{"decision": "skip", "reason": "test skip"}'"#); let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert_eq!(result.decision, HookDecision::Skip { reason: Some("test skip".into()), }); @@ -871,22 +864,19 @@ mod tests { #[tokio::test] async fn command_executor_env_vars_set() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); // Print env vars to stdout for verification let def = make_definition("echo $ARC_EVENT:$ARC_RUN_ID:$ARC_WORKFLOW"); let mut ctx = make_context(); ctx.node_id = Some("plan".into()); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert_eq!(result.decision, HookDecision::Proceed); } #[tokio::test] async fn no_hook_type_blocks() { - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = HookDefinition { name: None, event: HookEvent::StageStart, @@ -899,10 +889,7 @@ mod tests { }; let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; assert!(matches!(result.decision, HookDecision::Block { .. })); } @@ -1276,7 +1263,7 @@ mod tests { }) .await; - let executor = HookExecutorImpl; + let executor = HookExecutorImpl::new(test_llm_source()); let def = HookDefinition { name: Some("http-test".into()), event: HookEvent::StageStart, @@ -1294,10 +1281,7 @@ mod tests { }; let ctx = make_context(); let sandbox = make_sandbox(); - let source = test_llm_source(); - let result = executor - .execute(&def, &ctx, sandbox, None, source.as_ref()) - .await; + let result = executor.execute(&def, &ctx, sandbox, None).await; mock.assert_async().await; assert_eq!(result.decision, HookDecision::Proceed); diff --git a/lib/crates/fabro-hooks/src/runner.rs b/lib/crates/fabro-hooks/src/runner.rs index 110b3d58c..f90342d7f 100644 --- a/lib/crates/fabro-hooks/src/runner.rs +++ b/lib/crates/fabro-hooks/src/runner.rs @@ -4,8 +4,6 @@ use std::sync::Arc; use fabro_agent::Sandbox; use fabro_auth::CredentialSource; -#[cfg(test)] -use fabro_auth::EnvCredentialSource; use crate::config::{HookDefinition, HookSettings}; use crate::executor::{HookExecutor, HookExecutorImpl}; @@ -16,7 +14,6 @@ use crate::types::{HookContext, HookDecision}; pub struct HookRunner { config: HookSettings, executor: Arc, - llm_source: Arc, /// Pre-compiled regexes keyed by matcher pattern string. compiled_matchers: HashMap, } @@ -27,8 +24,7 @@ impl HookRunner { let compiled_matchers = Self::compile_matchers(&config); Self { config, - executor: Arc::new(HookExecutorImpl), - llm_source, + executor: Arc::new(HookExecutorImpl::new(llm_source)), compiled_matchers, } } @@ -40,7 +36,6 @@ impl HookRunner { Self { config, executor, - llm_source: Arc::new(EnvCredentialSource::new()), compiled_matchers, } } @@ -146,13 +141,7 @@ impl HookRunner { ); let result = self .executor - .execute( - hook, - context, - sandbox.clone(), - work_dir, - self.llm_source.as_ref(), - ) + .execute(hook, context, sandbox.clone(), work_dir) .await; tracing::debug!( hook = %hook.effective_name(), @@ -200,13 +189,7 @@ impl HookRunner { ); let result = self .executor - .execute( - hook, - context, - sandbox.clone(), - work_dir, - self.llm_source.as_ref(), - ) + .execute(hook, context, sandbox.clone(), work_dir) .await; tracing::debug!( hook = %hook.effective_name(), @@ -248,7 +231,6 @@ mod tests { _context: &HookContext, _sandbox: Arc, _work_dir: Option<&Path>, - _llm_source: &dyn CredentialSource, ) -> HookResult { HookResult { hook_name: definition.name.clone(), diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index 865cff207..cc33aa63f 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -52,6 +52,28 @@ impl Client { Self::from_credentials(resolved.credentials).await } + /// Create a Client from a credential source, resolving only the requested + /// providers. + /// + /// # Errors + /// + /// Returns `Error` if the source cannot resolve credentials or any provider + /// adapter fails to initialize. + pub async fn from_source_for( + source: &dyn CredentialSource, + providers: &[fabro_model::Provider], + ) -> Result { + let resolved = + source + .resolve_providers(providers) + .await + .map_err(|err| Error::Configuration { + message: format!("Failed to resolve LLM credentials: {err}"), + source: None, + })?; + Self::from_credentials(resolved.credentials).await + } + /// Create a Client from typed provider credentials. /// /// # Errors diff --git a/lib/crates/fabro-llm/src/generate.rs b/lib/crates/fabro-llm/src/generate.rs index 798e434c5..58943b303 100644 --- a/lib/crates/fabro-llm/src/generate.rs +++ b/lib/crates/fabro-llm/src/generate.rs @@ -296,30 +296,30 @@ pub struct GenerateParams { } impl GenerateParams { - pub fn new(model: impl Into, client: Arc) -> Self { + pub fn new(model: impl Into, client: impl Into>) -> Self { Self { - model: model.into(), - prompt: None, - messages: None, - system: None, - tools: None, - tool_choice: None, - max_tool_rounds: 1, - response_format: None, - temperature: None, - top_p: None, - max_tokens: None, - stop_sequences: None, + model: model.into(), + prompt: None, + messages: None, + system: None, + tools: None, + tool_choice: None, + max_tool_rounds: 1, + response_format: None, + temperature: None, + top_p: None, + max_tokens: None, + stop_sequences: None, reasoning_effort: None, - speed: None, - provider: None, + speed: None, + provider: None, provider_options: None, - metadata: None, - max_retries: 2, - timeout: None, - client, - abort_signal: None, - stop_when: None, + metadata: None, + max_retries: 2, + timeout: None, + client: client.into(), + abort_signal: None, + stop_when: None, repair_tool_call: None, } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 1a85f1ec3..f6b396177 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -8014,17 +8014,15 @@ provider = "invalid-provider" 5, |_| None, ); - state - .vault - .write() - .await - .set( + { + let mut vault = state.vault.write().await; + fabro_auth::vault_set_credential( + &mut vault, "openai_codex", - &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), - SecretType::Credential, - None, + &openai_api_key_credential("vault-openai-key"), ) .unwrap(); + } let llm_result = state.resolve_llm_client().await.unwrap(); @@ -8040,17 +8038,15 @@ provider = "invalid-provider" 5, |_| None, ); - state - .vault - .write() - .await - .set( + { + let mut vault = state.vault.write().await; + fabro_auth::vault_set_credential( + &mut vault, "openai_codex", - &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), - SecretType::Credential, - None, + &openai_api_key_credential("vault-openai-key"), ) .unwrap(); + } assert_eq!(state.llm_source.configured_providers().await, vec![ Provider::OpenAi @@ -8082,17 +8078,15 @@ provider = "invalid-provider" _ => None, }, ); - state - .vault - .write() - .await - .set( + { + let mut vault = state.vault.write().await; + fabro_auth::vault_set_credential( + &mut vault, "openai_codex", - &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), - SecretType::Credential, - None, + &openai_api_key_credential("vault-openai-key"), ) .unwrap(); + } let llm_result = state.resolve_llm_client().await.unwrap(); let response = llm_result diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index b04d92e8d..1611a7bdd 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -73,7 +73,6 @@ tokio = { workspace = true, features = ["test-util", "macros"] } object_store.workspace = true assert_cmd = "2" predicates = "3" -httpmock = "0.8" fabro-macros = { path = "../fabro-macros" } fabro-test = { workspace = true } fabro-types = { path = "../fabro-types", features = ["test-support"] } diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index 36c93f5c3..46c44bf45 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -35,6 +35,18 @@ fn build_profile(model: &str, provider: Provider) -> Box { } } +fn credential_providers(primary: Provider, fallback_chain: &[FallbackTarget]) -> Vec { + let mut providers = vec![primary]; + for target in fallback_chain { + if let Ok(provider) = target.provider.parse::() { + if !providers.contains(&provider) { + providers.push(provider); + } + } + } + providers +} + /// Shared state for tracking file modifications from agent tool calls. struct FileTracking { /// Maps tool_call_id → file_path for in-flight write/edit calls. @@ -203,7 +215,7 @@ impl AgentApiBackend { tool_hooks: Option>, mcp_servers: Vec, ) -> Result { - let client = Client::from_source(source) + let client = Client::from_source_for(source, &[provider]) .await .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; @@ -286,16 +298,28 @@ impl CodergenBackend for AgentApiBackend { prompt: &str, system_prompt: Option<&str>, ) -> Result { - let client = Client::from_source(self.source.as_ref()) - .await - .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; - let model = node.model().unwrap_or(&self.model); let provider = node .provider() .map(String::from) .or_else(|| Some(self.provider.as_str().to_string())); + // Build per-request fallback chain: if the node overrides the provider, + // no failover is available; otherwise use the backend's. + let fallback_chain: &[FallbackTarget] = if node.provider().is_some() { + &[] + } else { + &self.fallback_chain + }; + let primary_provider = provider + .as_deref() + .and_then(|provider| provider.parse::().ok()) + .unwrap_or(self.provider); + let providers = credential_providers(primary_provider, fallback_chain); + let client = Client::from_source_for(self.source.as_ref(), &providers) + .await + .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; + let max_tokens = node.max_tokens().or_else(|| { fabro_model::Catalog::builtin() .get(model) @@ -325,14 +349,6 @@ impl CodergenBackend for AgentApiBackend { provider_options: None, }; - // Build per-request fallback chain: if the node overrides the provider, - // no failover is available; otherwise use the backend's. - let fallback_chain: &[FallbackTarget] = if node.provider().is_some() { - &[] - } else { - &self.fallback_chain - }; - let result = client.complete(&request).await; let default_provider = self.provider.as_str().to_string(); @@ -640,7 +656,7 @@ impl CodergenBackend for AgentApiBackend { mod tests { use fabro_agent::subagent::SessionFactory; use fabro_auth::{AuthCredential, AuthDetails, VaultCredentialSource}; - use fabro_vault::{SecretType, Vault}; + use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; use super::*; @@ -792,20 +808,13 @@ mod tests { async fn api_backend_uses_source_credentials() { let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "anthropic", - &serde_json::to_string(&AuthCredential { - provider: Provider::Anthropic, - details: AuthDetails::ApiKey { - key: "anthropic-key".to_string(), - }, - }) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); + fabro_auth::vault_set_credential(&mut vault, "anthropic", &AuthCredential { + provider: Provider::Anthropic, + details: AuthDetails::ApiKey { + key: "anthropic-key".to_string(), + }, + }) + .unwrap(); let backend = AgentApiBackend::new( "claude-opus-4-6".to_string(), Provider::Anthropic, diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index d9117b824..ffef4f7f3 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -13,11 +13,10 @@ use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookRunner}; use fabro_sandbox::{ ReadBeforeWriteSandbox, SandboxEventCallback, SandboxSpec, WorkdirStrategy, WorktreeOptions, - WorktreeSandbox, + WorktreeSandbox, shell_quote, }; use fabro_vault::Vault; use futures::future::try_join_all; -use shlex::try_quote; use tokio::process::Command as TokioCommand; use tokio::runtime::Handle; use tokio::sync::RwLock as AsyncRwLock; @@ -384,6 +383,7 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { .arg("-c") .arg(&shell_command) .current_dir(&cwd) + .kill_on_drop(true) .output(), ) .await @@ -418,7 +418,7 @@ async fn resolve_devcontainer(options: &mut InitOptions) -> Result<(), Error> { fabro_devcontainer::Command::Args(args) => { let shell_command = args .iter() - .map(|arg| try_quote(arg).unwrap_or_else(|_| arg.into()).to_string()) + .map(|arg| shell_quote(arg)) .collect::>() .join(" "); run_shell(shell_command).await?; @@ -749,7 +749,7 @@ mod tests { use fabro_sandbox::SandboxSpec; use fabro_store::Database; use fabro_types::{RunId, WorkflowSettings, fixtures}; - use fabro_vault::{SecretType, Vault}; + use fabro_vault::Vault; use object_store::memory::InMemory; use tokio::sync::RwLock as AsyncRwLock; @@ -954,20 +954,13 @@ mod tests { async fn build_registry_accepts_vault_only_llm_provider() { let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "anthropic", - &serde_json::to_string(&AuthCredential { - provider: fabro_llm::Provider::Anthropic, - details: AuthDetails::ApiKey { - key: "anthropic-key".to_string(), - }, - }) - .unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); + fabro_auth::vault_set_credential(&mut vault, "anthropic", &AuthCredential { + provider: fabro_llm::Provider::Anthropic, + details: AuthDetails::ApiKey { + key: "anthropic-key".to_string(), + }, + }) + .unwrap(); let (graph, _) = llm_graph(); let vault = Arc::new(AsyncRwLock::new(vault)); diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 6894fa0f8..67b4df095 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -605,10 +605,8 @@ mod tests { }; use fabro_store::Database; use fabro_types::{BilledTokenCounts, RunSpec, SuccessReason, fixtures}; - use fabro_vault::{SecretType, Vault}; + use fabro_vault::Vault; use futures::stream; - use httpmock::Method::POST; - use httpmock::MockServer; use object_store::memory::InMemory; use tokio::sync::RwLock as AsyncRwLock; @@ -721,30 +719,6 @@ mod tests { } } - fn openai_responses_payload(text: &str) -> serde_json::Value { - serde_json::json!({ - "id": "resp_1", - "model": "gpt-5.4", - "output": [ - { - "type": "message", - "role": "assistant", - "content": [ - { - "type": "output_text", - "text": text - } - ] - } - ], - "status": "completed", - "usage": { - "input_tokens": 10, - "output_tokens": 20 - } - }) - } - fn make_test_conclusion() -> Conclusion { Conclusion { timestamp: Utc::now(), @@ -1289,29 +1263,24 @@ mod tests { #[tokio::test] async fn build_pr_body_uses_vault_only_openai_codex_source() { - let server = MockServer::start_async().await; - let response_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/v1/responses") - .header("authorization", "Bearer vault-openai-key"); - then.status(200) - .header("content-type", "application/json") - .json_body(openai_responses_payload("Narrative from vault source.")); - }) + let twin = fabro_test::twin_openai().await; + let namespace = format!("{}::{}", module_path!(), line!()); + fabro_test::TwinScenarios::new(namespace.clone()) + .scenario( + fabro_test::TwinScenario::responses("gpt-5.4").text("Narrative from vault source."), + ) + .load(twin) .await; let dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); - vault - .set( - "openai_codex", - &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); - let base_url = server.url("/v1"); + fabro_auth::vault_set_credential( + &mut vault, + "openai_codex", + &openai_api_key_credential(&namespace), + ) + .unwrap(); + let base_url = twin.base_url.clone(); let llm_source: Arc = Arc::new(VaultCredentialSource::with_env_lookup( Arc::new(AsyncRwLock::new(vault)), @@ -1337,7 +1306,6 @@ mod tests { .unwrap(); assert!(body.contains("Narrative from vault source.")); - response_mock.assert_async().await; } // ── parse_dot_summary tests ───────────────────────────────────────── diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 39acd777f..acd63b1fc 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6610,30 +6610,6 @@ fn openai_api_key_credential(key: &str) -> fabro_auth::AuthCredential { } } -fn openai_responses_payload(text: &str) -> serde_json::Value { - serde_json::json!({ - "id": "resp_1", - "model": "gpt-5.4", - "output": [ - { - "type": "message", - "role": "assistant", - "content": [ - { - "type": "output_text", - "text": text - } - ] - } - ], - "status": "completed", - "usage": { - "input_tokens": 10, - "output_tokens": 20 - } - }) -} - // --------------------------------------------------------------------------- // Wait.human freeform edge integration tests (Section 4.6) // --------------------------------------------------------------------------- @@ -6643,21 +6619,16 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { use chrono::Utc; use fabro_auth::{CredentialSource, VaultCredentialSource}; use fabro_types::Conclusion; - use fabro_vault::{SecretType, Vault}; - use httpmock::Method::POST; - use httpmock::MockServer; + use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; - let server = MockServer::start_async().await; - let response_mock = server - .mock_async(|when, then| { - when.method(POST) - .path("/v1/responses") - .header("authorization", "Bearer vault-openai-key"); - then.status(200) - .header("content-type", "application/json") - .json_body(openai_responses_payload("Narrative from vault source.")); - }) + let twin = fabro_test::twin_openai().await; + let namespace = format!("{}::{}", module_path!(), line!()); + fabro_test::TwinScenarios::new(namespace.clone()) + .scenario( + fabro_test::TwinScenario::responses("gpt-5.4").text("Narrative from vault source."), + ) + .load(twin) .await; let mut graph = Graph::new("VaultOpenAiCodexPrBody"); @@ -6684,15 +6655,13 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { let vault_dir = tempfile::tempdir().unwrap(); let mut vault = Vault::load(vault_dir.path().join("secrets.json")).unwrap(); - vault - .set( - "openai_codex", - &serde_json::to_string(&openai_api_key_credential("vault-openai-key")).unwrap(), - SecretType::Credential, - None, - ) - .unwrap(); - let base_url = server.url("/v1"); + fabro_auth::vault_set_credential( + &mut vault, + "openai_codex", + &openai_api_key_credential(&namespace), + ) + .unwrap(); + let base_url = twin.base_url.clone(); let llm_source: Arc = Arc::new(VaultCredentialSource::with_env_lookup( Arc::new(AsyncRwLock::new(vault)), move |name| match name { @@ -6757,7 +6726,6 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { .expect("PR body should build from vault-only credentials"); assert!(body.contains("Narrative from vault source.")); - response_mock.assert_async().await; } /// Freeform-only human gate: free-text input routes through the freeform edge