From e54fef760adf423c34f474cf854c3a3aabc0fc6a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 20:36:54 -0400 Subject: [PATCH] refactor(auth): remove EnvCredentialSource and make the run vault required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EnvCredentialSource` resolved provider credentials from the process environment. It had no production entry point of its own — it was only ever reached as the `None` arm of an `Option` in three places: `build_llm_source`, `configured_providers_for_start`, and `configured_providers_from_process_env`. That optional vault is not a state the product can be in. Every run has a server behind it, the server always spawns workers with `--storage-dir` (`worker_runtime.rs`), and `SqlVaultCredentialSource` backs both the server and the CLI. So the fallback only served to silently degrade credential resolution to whatever the worker process happened to have in its environment. Make the vault required across the run path — `RunOptions`, `StartServices`, `build_llm_source`, `tool_secrets_from_configured_sources`, `vault_token_lookup`, and the CLI GitHub helpers — so the invariant is enforced by types rather than assumed. A worker spawned without `--storage-dir` now fails with a clear message instead of quietly continuing without a vault. `configured_providers_from_process_env` had no callers at all and is deleted. `AgentApiBackend::new_from_env` was public but only ever called from its own tests; it is deleted too. Test-only credential sources move to a feature-gated `fabro_auth::test_support`, wired through dev-dependencies so they never link into production builds. The CLI worker tests now pass `--storage-dir`, matching what the server actually does. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-cli/src/commands/run/runner.rs | 38 +- lib/apps/fabro-cli/src/shared/github.rs | 8 +- lib/apps/fabro-cli/tests/it/cmd/runner.rs | 35 ++ lib/apps/fabro-server/Cargo.toml | 1 + lib/apps/fabro-server/src/server.rs | 2 +- .../tests/it/scenario/run_completion.rs | 9 +- lib/components/fabro-agent/Cargo.toml | 1 + .../fabro-agent/tests/it/parity_matrix.rs | 4 +- lib/components/fabro-hooks/Cargo.toml | 1 + lib/components/fabro-hooks/src/executor.rs | 4 +- lib/components/fabro-hooks/src/runner.rs | 7 +- .../fabro-hooks/tests/host_command_hooks.rs | 4 +- lib/components/fabro-workflow/Cargo.toml | 3 +- .../fabro-workflow/src/handler/llm/api.rs | 57 ++- .../fabro-workflow/src/lifecycle/git.rs | 3 +- .../fabro-workflow/src/operations/start.rs | 47 +-- .../src/pipeline/execute/tests.rs | 9 +- .../fabro-workflow/src/pipeline/finalize.rs | 5 +- .../fabro-workflow/src/pipeline/initialize.rs | 101 ++--- .../src/pipeline/pull_request.rs | 4 +- .../fabro-workflow/src/pipeline/types.rs | 2 +- .../fabro-workflow/src/test_support.rs | 4 +- .../fabro-workflow/tests/it/integration.rs | 16 +- lib/foundation/fabro-auth/Cargo.toml | 3 + lib/foundation/fabro-auth/src/env_source.rs | 381 ------------------ lib/foundation/fabro-auth/src/lib.rs | 5 +- lib/foundation/fabro-auth/src/resolve.rs | 19 - lib/foundation/fabro-auth/src/test_support.rs | 62 +++ 28 files changed, 260 insertions(+), 575 deletions(-) delete mode 100644 lib/foundation/fabro-auth/src/env_source.rs create mode 100644 lib/foundation/fabro-auth/src/test_support.rs diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 4e6b72b8b..45c2dcecf 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -137,13 +137,10 @@ pub(crate) async fn execute( if let Some(control_manager) = &mut control_manager { control_manager.wait_for_first_connection().await?; } - let vault = load_worker_vault(storage_dir.as_deref()).await?; + let vault = load_worker_vault(storage_dir.as_deref(), &run_dir).await?; let github_app = { - let vault_guard = match &vault { - Some(arc) => Some(arc.read().await), - None => None, - }; - maybe_build_github_credentials(&run_spec.settings, vault_guard.as_deref())? + let vault_guard = vault.read().await; + maybe_build_github_credentials(&run_spec.settings, &vault_guard)? }; let services = StartServices { run_id, @@ -272,10 +269,23 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { } } -async fn load_worker_vault(storage_dir: Option<&Path>) -> Result>>> { - let Some(storage_dir) = storage_dir else { - return Ok(None); - }; +/// Load the worker's secret vault from the run's storage root. +/// +/// A worker always runs against a server-created run, which lives under +/// `/scratch/`, so the storage root is always resolvable. Failing +/// here is better than continuing without a vault: credentials would silently +/// fall back to whatever the worker process happens to have in its environment. +async fn load_worker_vault( + storage_dir: Option<&Path>, + run_dir: &Path, +) -> Result>> { + let storage_dir = storage_dir.with_context(|| { + format!( + "run worker for {} was spawned without --storage-dir; it needs the storage root to \ + load its secret vault", + run_dir.display() + ) + })?; let storage = Storage::new(storage_dir); let vault = SecretStore::open_snapshot(storage.sqlite_path(), storage.secrets_path()) @@ -287,7 +297,7 @@ async fn load_worker_vault(storage_dir: Option<&Path>) -> Result RunEvent { fn maybe_build_github_credentials( settings: &WorkflowSettings, - vault: Option<&fabro_vault::Vault>, + vault: &fabro_vault::Vault, ) -> Result> { let resolved_run = &settings.run; let resolved_server = ServerSettingsBuilder::load_default().ok(); @@ -1747,7 +1757,9 @@ mod tests { .set("ANTHROPIC_API_KEY", "vault-key", SecretType::Token, None) .unwrap(); - let loaded = load_worker_vault(Some(temp.path())).await.unwrap().unwrap(); + let loaded = load_worker_vault(Some(temp.path()), temp.path()) + .await + .unwrap(); let guard = loaded.read().await; let credential = guard.get("ANTHROPIC_API_KEY").unwrap(); diff --git a/lib/apps/fabro-cli/src/shared/github.rs b/lib/apps/fabro-cli/src/shared/github.rs index 731467d1d..f6e6ec9f0 100644 --- a/lib/apps/fabro-cli/src/shared/github.rs +++ b/lib/apps/fabro-cli/src/shared/github.rs @@ -8,7 +8,7 @@ pub(crate) fn build_github_credentials( strategy: GithubIntegrationStrategy, app_id: Option<&str>, app_slug: Option<&str>, - vault: Option<&Vault>, + vault: &Vault, ) -> anyhow::Result> { match strategy { GithubIntegrationStrategy::App => { @@ -31,7 +31,7 @@ pub(crate) fn build_github_credentials( /// Look up GitHub token: GITHUB_TOKEN env -> vault GITHUB_TOKEN -> GH_TOKEN env /// -> vault GH_TOKEN -fn lookup_github_token(vault: Option<&Vault>) -> Option { +fn lookup_github_token(vault: &Vault) -> Option { lookup_env_or_vault(EnvVars::GITHUB_TOKEN, vault) .or_else(|| lookup_env_or_vault(EnvVars::GH_TOKEN, vault)) } @@ -40,10 +40,10 @@ fn lookup_github_token(vault: Option<&Vault>) -> Option { clippy::disallowed_methods, reason = "GitHub credential resolution intentionally falls back from vault to documented process-env names." )] -fn lookup_env_or_vault(name: &str, vault: Option<&Vault>) -> Option { +fn lookup_env_or_vault(name: &str, vault: &Vault) -> Option { std::env::var(name) .ok() - .or_else(|| vault.and_then(|v| v.get(name).map(str::to_string))) + .or_else(|| vault.get(name).map(str::to_string)) .map(|t| t.trim().to_string()) .filter(|t| !t.is_empty()) } diff --git a/lib/apps/fabro-cli/tests/it/cmd/runner.rs b/lib/apps/fabro-cli/tests/it/cmd/runner.rs index 6828b6251..5bce373f8 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/runner.rs @@ -71,6 +71,11 @@ fn spawn_worker_process( ); cmd.args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", server, "--run-dir", @@ -215,6 +220,11 @@ fn worker_requires_fabro_worker_token_env() { .command() .args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", "http://127.0.0.1:32276", "--run-dir", @@ -273,6 +283,11 @@ digraph CachedGraph { let output = worker_command(&context, run_id.as_str()) .args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", server.as_str(), "--run-dir", @@ -342,6 +357,11 @@ digraph GitHubApp { cmd.env("GITHUB_APP_PRIVATE_KEY", "%%%not-base64%%%"); cmd.args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", server.as_str(), "--run-dir", @@ -391,6 +411,11 @@ digraph DetachedStoreOnly { let output = worker_command(&context, run_id.as_str()) .args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", server.as_str(), "--run-dir", @@ -609,6 +634,11 @@ digraph Test { let mut cmd = worker_command(&context, &run_id); cmd.args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", &server, "--run-dir", @@ -674,6 +704,11 @@ fn runner_reports_malformed_run_state_without_prefetching_events() { let output = worker_command(&context, &run_id) .args([ "__run-worker", + "--storage-dir", + context + .storage_dir + .to_str() + .expect("storage directory path should be valid UTF-8"), "--server", &format!("{}/api/v1", server.base_url()), "--run-dir", diff --git a/lib/apps/fabro-server/Cargo.toml b/lib/apps/fabro-server/Cargo.toml index 1f2c5bb61..e47243f32 100644 --- a/lib/apps/fabro-server/Cargo.toml +++ b/lib/apps/fabro-server/Cargo.toml @@ -108,6 +108,7 @@ fabro-build-support = { path = "../../foundation/build-support" } chrono = { workspace = true } [dev-dependencies] +fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } tokio = { workspace = true, features = ["test-util", "macros"] } tower = "0.5" http-body-util = "0.1" diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 11a9a0948..daeadaea2 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -4115,7 +4115,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { run_control: None, github_app, github_permissions, - vault: Some(Arc::new(AsyncRwLock::new(vault.into_vault()))), + vault: Arc::new(AsyncRwLock::new(vault.into_vault())), catalog: state.catalog(), on_node: None, registry_override, diff --git a/lib/apps/fabro-server/tests/it/scenario/run_completion.rs b/lib/apps/fabro-server/tests/it/scenario/run_completion.rs index a0e40e06c..8c7e5aeb1 100644 --- a/lib/apps/fabro-server/tests/it/scenario/run_completion.rs +++ b/lib/apps/fabro-server/tests/it/scenario/run_completion.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use axum::body::Body; use axum::http::{Request, StatusCode}; -use fabro_auth::EnvCredentialSource; +use fabro_auth::test_support; use fabro_model::{Catalog, ProviderId}; use fabro_static::EnvVars; use fabro_test::{TwinScenario, TwinScenarios, twin_openai}; @@ -43,12 +43,11 @@ fn test_app_with_openai_agent_backend(openai_base_url: String, api_key: String) ); let source_api_key = api_key.clone(); let env_api_key = api_key.clone(); - let llm_source: Arc = Arc::new( - EnvCredentialSource::with_env_lookup(Arc::new(move |name| match name { + let llm_source: Arc = + test_support::env_credential_source(move |name| match name { "OPENAI_API_KEY" => Some(source_api_key.clone()), _ => None, - })), - ); + }); let state = fabro_server::test_support::TestAppStateBuilder::new() .runtime_settings(settings.server_settings, settings.manifest_run_defaults) .max_concurrent_runs(5) diff --git a/lib/components/fabro-agent/Cargo.toml b/lib/components/fabro-agent/Cargo.toml index 07f8497fc..2e9a299b6 100644 --- a/lib/components/fabro-agent/Cargo.toml +++ b/lib/components/fabro-agent/Cargo.toml @@ -59,6 +59,7 @@ htmd = "0.5" libc = "0.2" [dev-dependencies] +fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } insta.workspace = true tokio = { workspace = true, features = ["test-util", "macros"] } tempfile = "3" diff --git a/lib/components/fabro-agent/tests/it/parity_matrix.rs b/lib/components/fabro-agent/tests/it/parity_matrix.rs index e37a29003..385bd5275 100644 --- a/lib/components/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/components/fabro-agent/tests/it/parity_matrix.rs @@ -13,7 +13,7 @@ use fabro_agent::{ AgentEvent, AgentProfile, AgentProfileBuilder, LocalSandbox, OpenAiProfile, Session, SessionOptions, SubAgentSupervisor, ToolSecrets, WebFetchSummarizer, }; -use fabro_auth::EnvCredentialSource; +use fabro_auth::test_support; use fabro_llm::client::Client; use fabro_llm::provider::ProviderAdapter; use fabro_llm::providers::{OpenAiAdapter, OpenAiCompatibleAdapter}; @@ -132,7 +132,7 @@ async fn make_client(provider: &Provider, twin: Option<&OpenAiTwinOptions>) -> C return make_twin_client(twin.expect("openai twin config should be provided")); } - let source = EnvCredentialSource::new(); + let source = test_support::StubCredentialSource; let catalog = Arc::new(Catalog::from_builtin().expect("default catalog should build")); Client::from_source(&source, catalog) .await diff --git a/lib/components/fabro-hooks/Cargo.toml b/lib/components/fabro-hooks/Cargo.toml index 02f25fdad..2c1a68422 100644 --- a/lib/components/fabro-hooks/Cargo.toml +++ b/lib/components/fabro-hooks/Cargo.toml @@ -30,6 +30,7 @@ tracing.workspace = true tokio-util.workspace = true [dev-dependencies] +fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } httpmock = "0.8" tokio = { workspace = true, features = ["test-util", "macros"] } toml.workspace = true diff --git a/lib/components/fabro-hooks/src/executor.rs b/lib/components/fabro-hooks/src/executor.rs index 08b39b723..d92f73c8e 100644 --- a/lib/components/fabro-hooks/src/executor.rs +++ b/lib/components/fabro-hooks/src/executor.rs @@ -836,7 +836,7 @@ impl HookExecutor for HookExecutorImpl { #[cfg(test)] mod tests { - use fabro_auth::{CredentialSource, EnvCredentialSource}; + use fabro_auth::{CredentialSource, test_support}; use fabro_types::fixtures; use fabro_util::env::TestEnv; @@ -855,7 +855,7 @@ mod tests { } fn test_llm_source() -> Arc { - Arc::new(EnvCredentialSource::new()) + test_support::vault_only_credential_source() } fn test_catalog() -> Arc { diff --git a/lib/components/fabro-hooks/src/runner.rs b/lib/components/fabro-hooks/src/runner.rs index 9435dba9a..c3ec53987 100644 --- a/lib/components/fabro-hooks/src/runner.rs +++ b/lib/components/fabro-hooks/src/runner.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use fabro_agent::Sandbox; use fabro_auth::CredentialSource; #[cfg(test)] -use fabro_auth::EnvCredentialSource; +use fabro_auth::test_support; use fabro_model::Catalog; use crate::config::{HookDefinition, HookSettings}; @@ -46,7 +46,7 @@ impl HookRunner { Self { config, executor, - llm_source: Arc::new(EnvCredentialSource::new()), + llm_source: test_support::vault_only_credential_source(), catalog: Arc::new(Catalog::from_builtin().expect("default catalog should build")), compiled_matchers, } @@ -238,7 +238,6 @@ impl HookRunner { #[cfg(test)] mod tests { - use fabro_auth::EnvCredentialSource; use fabro_types::fixtures; use super::*; @@ -279,7 +278,7 @@ mod tests { } fn test_llm_source() -> Arc { - Arc::new(EnvCredentialSource::new()) + test_support::vault_only_credential_source() } fn test_catalog() -> Arc { diff --git a/lib/components/fabro-hooks/tests/host_command_hooks.rs b/lib/components/fabro-hooks/tests/host_command_hooks.rs index 4bbe0c5a3..fe73371dd 100644 --- a/lib/components/fabro-hooks/tests/host_command_hooks.rs +++ b/lib/components/fabro-hooks/tests/host_command_hooks.rs @@ -2,7 +2,7 @@ use std::path::Path; use std::sync::Arc; use fabro_agent::{LocalSandbox, Sandbox}; -use fabro_auth::{CredentialSource, EnvCredentialSource}; +use fabro_auth::{CredentialSource, test_support}; use fabro_hooks::{ HookContext, HookDecision, HookDefinition, HookEvent, HookExecutionContext, HookRunner, HookSettings, InterpString, @@ -12,7 +12,7 @@ use fabro_types::RunId; use tokio::fs; fn test_llm_source() -> Arc { - Arc::new(EnvCredentialSource::new()) + test_support::vault_only_credential_source() } fn test_catalog() -> Arc { diff --git a/lib/components/fabro-workflow/Cargo.toml b/lib/components/fabro-workflow/Cargo.toml index 932bb0071..5ce0c0cac 100644 --- a/lib/components/fabro-workflow/Cargo.toml +++ b/lib/components/fabro-workflow/Cargo.toml @@ -14,7 +14,7 @@ readme = "README.md" doctest = false [features] -test-support = [] +test-support = ["fabro-auth/test-support"] [lints] workspace = true @@ -74,6 +74,7 @@ tempfile = "3" toml.workspace = true fabro-vault = { path = "../../foundation/fabro-vault" } [dev-dependencies] +fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } base64.workspace = true fabro-acp = { path = "../fabro-acp", features = ["test-support"] } fabro-workflow = { path = ".", features = ["test-support"] } diff --git a/lib/components/fabro-workflow/src/handler/llm/api.rs b/lib/components/fabro-workflow/src/handler/llm/api.rs index c4ce3737e..def8369ce 100644 --- a/lib/components/fabro-workflow/src/handler/llm/api.rs +++ b/lib/components/fabro-workflow/src/handler/llm/api.rs @@ -10,7 +10,7 @@ use fabro_agent::{ Sandbox, Session, SessionOptions, SessionShutdownReason, StaticEnvProvider, ToolEnvProvider, ToolSecrets, canonical_tool_name, register_question_tools, }; -use fabro_auth::{CredentialSource, EnvCredentialSource}; +use fabro_auth::CredentialSource; use fabro_graphviz::graph::{AttrValue, Node}; use fabro_llm::client::Client; use fabro_llm::types::{ @@ -695,22 +695,6 @@ impl AgentApiBackend { } } - #[must_use] - pub fn new_from_env( - model: String, - provider_id: impl Into, - fallback_chain: Vec, - steering_hub: Arc, - ) -> Self { - Self::new( - model, - provider_id, - fallback_chain, - Arc::new(EnvCredentialSource::new()), - steering_hub, - ) - } - #[must_use] pub fn with_env(mut self, env: HashMap) -> Self { self.tool_env = Some(Arc::new(StaticEnvProvider(env))); @@ -1718,7 +1702,7 @@ mod tests { use fabro_agent::subagent::SessionFactory; use fabro_agent::{AgentProfile, LocalSandbox, ToolRegistry}; use fabro_api::types; - use fabro_auth::{EnvCredentialSource, VaultCredentialSource}; + use fabro_auth::{VaultCredentialSource, test_support as auth_test_support}; use fabro_llm::provider::{ProviderAdapter, StreamEventStream}; use fabro_llm::{Error as LlmError, ProviderErrorDetail, ProviderErrorKind}; use fabro_tool::FabroToolBackend; @@ -1895,18 +1879,18 @@ reasoning = false } fn mock_api_backend(server: &MockServer) -> AgentApiBackend { - let source = EnvCredentialSource::with_env_lookup(Arc::new(|name| { + let source = auth_test_support::env_credential_source(|name| { if name == "MOCK_API_KEY" { Some("sk-test".to_string()) } else { None } - })); + }); AgentApiBackend::new_with_catalog( "mock-model".to_string(), ProviderId::from("mock"), Vec::new(), - Arc::new(source), + source, SteeringHub::for_tests(), mock_llm_catalog(server), ) @@ -2004,10 +1988,11 @@ reasoning = false #[test] fn agent_backend_stores_config() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "claude-opus-4-6".to_string(), ProviderId::openai(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ); assert_eq!(backend.model, "claude-opus-4-6"); @@ -2016,10 +2001,11 @@ reasoning = false #[test] fn agent_backend_initializes_empty_sessions() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "claude-opus-4-6".to_string(), ProviderId::anthropic(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ); assert!(backend.sessions.lock().unwrap().is_empty()); @@ -2722,7 +2708,7 @@ enabled = true "gpt-5.4".to_string(), ProviderId::from("openrouter"), Vec::new(), - Arc::new(EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), Arc::new(Catalog::from_builtin_with_overrides(&settings).unwrap()), ); @@ -2738,7 +2724,7 @@ enabled = true "gpt-5.4".to_string(), ProviderId::from("openrouter"), Vec::new(), - Arc::new(EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), Arc::new(Catalog::from_builtin().unwrap()), ); @@ -2785,7 +2771,7 @@ reasoning = false "acme-llama".to_string(), ProviderId::from("acme"), Vec::new(), - Arc::new(EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), catalog, ); @@ -2832,7 +2818,7 @@ reasoning = false "acme-claude".to_string(), ProviderId::from("acme"), Vec::new(), - Arc::new(EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), catalog, ); @@ -2857,7 +2843,7 @@ enabled = true "openai/gpt-5.4".to_string(), ProviderId::from("openrouter"), Vec::new(), - Arc::new(EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), catalog, ); @@ -2872,10 +2858,11 @@ enabled = true #[test] fn run_model_controls_apply_when_node_omits_controls() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "gpt-5.4".to_string(), ProviderId::openai(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ) .with_run_model_controls(fabro_types::settings::run::RunModelControls { @@ -2892,10 +2879,11 @@ enabled = true #[test] fn node_controls_override_run_model_controls() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "gpt-5.4".to_string(), ProviderId::openai(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ) .with_run_model_controls(fabro_types::settings::run::RunModelControls { @@ -2920,10 +2908,11 @@ enabled = true #[test] fn omitted_reasoning_effort_stays_unset() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "gpt-5.4".to_string(), ProviderId::openai(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ); let node = Node::new("work"); @@ -2969,10 +2958,11 @@ enabled = true provider: "openai".to_string(), model: "gpt-5.5".to_string(), }]; - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "claude-fable-5".to_string(), ProviderId::anthropic(), fallback_chain.clone(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ); let mut providers = HashMap::new(); @@ -3242,10 +3232,11 @@ enabled = true #[tokio::test] async fn api_backend_shutdown_closes_cached_sessions_once() { - let backend = AgentApiBackend::new_from_env( + let backend = AgentApiBackend::new( "gpt-5.4".to_string(), ProviderId::openai(), Vec::new(), + auth_test_support::vault_only_credential_source(), SteeringHub::for_tests(), ); let emitter = Arc::new(Emitter::new(fabro_types::RunId::new())); diff --git a/lib/components/fabro-workflow/src/lifecycle/git.rs b/lib/components/fabro-workflow/src/lifecycle/git.rs index 4db74c904..21402ec27 100644 --- a/lib/components/fabro-workflow/src/lifecycle/git.rs +++ b/lib/components/fabro-workflow/src/lifecycle/git.rs @@ -593,6 +593,7 @@ mod tests { use anyhow::Result; use async_trait::async_trait; use bytes::Bytes; + use fabro_auth::test_support as auth_test_support; use fabro_core::graph::Graph as CoreGraph; use fabro_core::lifecycle::RunLifecycle; use fabro_core::state::ExecutionState; @@ -1261,7 +1262,7 @@ mod tests { tokio_util::sync::CancellationToken::new(), fabro_model::ProviderId::anthropic(), "claude-sonnet-4-6".to_string(), - Arc::new(fabro_auth::EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), Arc::new(Catalog::from_builtin().expect("default catalog should build")), Arc::new(SandboxGitRuntime::new()), Arc::clone(&lifecycle.metadata_runtime), diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 3b560c7cb..eb3d49218 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -3,7 +3,7 @@ use std::path::Path; use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; -use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_auth::{CredentialSource, VaultCredentialSource}; use fabro_interview::{AutoApproveInterviewer, Interviewer}; use fabro_llm::client::Client as LlmClient; use fabro_mcp::config::McpServerSettings; @@ -81,7 +81,7 @@ struct RunSession { workflow_path: Option, workflow_bundle: Option>, run_control: Option>, - vault: Option>>, + vault: Arc>, catalog: Arc, fabro_run_tools: Option, } @@ -106,7 +106,7 @@ pub struct StartServices { /// Server-resolved GitHub integration permissions to inject into the /// sandbox env. Empty when github integration has no permissions. pub github_permissions: HashMap, - pub vault: Option>>, + pub vault: Arc>, pub catalog: Arc, pub on_node: crate::OnNodeCallback, pub registry_override: Option>, @@ -374,7 +374,7 @@ impl RunSession { resolve_sandbox_provider(resolved).effective_for(resolved.execution.mode); let catalog = Arc::clone(&services.catalog); let configured = - configured_providers_for_start(services.vault.as_ref(), Arc::clone(&catalog)).await; + configured_providers_for_start(&services.vault, Arc::clone(&catalog)).await; #[cfg(feature = "test-support")] let configured = workflow_test_support::test_configured_provider_ids( catalog.as_ref(), @@ -383,14 +383,11 @@ impl RunSession { .is_some_and(|value| !matches!(value.as_str(), "" | "0" | "false" | "no")), ); let llm = resolve_start_llm(catalog.as_ref(), &configured, resolved)?; - let vault_guard = match services.vault.as_ref() { - Some(vault) => Some(vault.read().await), - None => None, - }; + let vault_guard = services.vault.read().await; // Token-only secrets lookup over the vault read guard, shared across // every run-boundary resolver. A missing or non-Token secret becomes // `None`, so resolution fails closed with a secret error. - let secret_lookup = |name: &str| vault_token_lookup(vault_guard.as_deref(), name); + let secret_lookup = |name: &str| vault_token_lookup(&vault_guard, name); let mcp_servers = resolved .agent .mcps @@ -436,10 +433,9 @@ impl RunSession { clone_branch: record.base_branch().map(str::to_string), }, SandboxProviderKind::Daytona => { - let api_key = match vault_guard.as_deref() { - Some(vault) => vault.get(EnvVars::DAYTONA_API_KEY).map(str::to_string), - None => None, - }; + let api_key = vault_guard + .get(EnvVars::DAYTONA_API_KEY) + .map(str::to_string); SandboxSpec::Daytona { config: Box::new(resolve_daytona_config(resolved)), github_app: services.github_app.clone(), @@ -522,16 +518,13 @@ impl RunSession { } async fn configured_providers_for_start( - vault: Option<&Arc>>, + vault: &Arc>, catalog: Arc, ) -> Vec { - let source: Arc = match vault { - Some(vault) => Arc::new(VaultCredentialSource::with_env_lookup( - Arc::clone(vault), - process_env_var, - )), - None => Arc::new(EnvCredentialSource::new()), - }; + let source: Arc = Arc::new(VaultCredentialSource::with_env_lookup( + Arc::clone(vault), + process_env_var, + )); match LlmClient::from_source_report(source.as_ref(), catalog).await { Ok(report) => report .client @@ -572,8 +565,8 @@ fn process_env_var(name: &str) -> Option { std::env::var(name).ok() } -fn vault_token_lookup(vault: Option<&Vault>, name: &str) -> Option { - vault.and_then(|vault| fabro_auth::vault_get_token(vault, name).ok().flatten()) +fn vault_token_lookup(vault: &Vault, name: &str) -> Option { + fabro_auth::vault_get_token(vault, name).ok().flatten() } async fn load_accepted_run_definition( @@ -1780,7 +1773,7 @@ reasoning = false )]))); let session = RunSession::new(&persisted, StartServices { - vault: Some(vault), + vault, ..test_start_services(&store, &storage_root, emitter, registry).await }) .await @@ -1838,7 +1831,7 @@ reasoning = false let vault = Arc::new(AsyncRwLock::new(start_vault(&[]))); let Err(err) = RunSession::new(&persisted, StartServices { - vault: Some(vault), + vault, ..test_start_services(&store, &storage_root, emitter, registry).await }) .await @@ -2004,7 +1997,7 @@ reasoning = false run_control: None, github_app: None, github_permissions: HashMap::new(), - vault: Some(Arc::new(AsyncRwLock::new(start_vault(&[])))), + vault: Arc::new(AsyncRwLock::new(start_vault(&[]))), catalog: test_catalog(), on_node: None, registry_override: Some(registry), @@ -2032,7 +2025,7 @@ reasoning = false } fn vault_secret_lookup(vault: &Vault) -> impl FnMut(&str) -> Option + '_ { - move |name| vault_token_lookup(Some(vault), name) + move |name| vault_token_lookup(vault, name) } fn prepare_with_step(step: PreparedStep) -> RunPrepareSettings { diff --git a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs index 2322e6b71..98a1feabb 100644 --- a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs @@ -12,6 +12,7 @@ use std::time::Duration; use async_trait::async_trait; use fabro_agent::Sandbox; +use fabro_auth::test_support as auth_test_support; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_hooks::HookSettings; use fabro_interview::AutoApproveInterviewer; @@ -286,7 +287,7 @@ async fn execute_test_run_with_options( github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: git_options, run_control: None, registry_override, @@ -348,7 +349,7 @@ async fn execute_runs_start_to_exit_and_returns_final_context() { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: None, @@ -487,7 +488,7 @@ async fn resumed_in_flight_node_starts_a_new_stage_execution() { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: Some(Arc::new(make_registry())), @@ -598,7 +599,7 @@ async fn run_with_lifecycle( github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: Some(Arc::new(registry)), diff --git a/lib/components/fabro-workflow/src/pipeline/finalize.rs b/lib/components/fabro-workflow/src/pipeline/finalize.rs index 949578880..a6d6581ec 100644 --- a/lib/components/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/components/fabro-workflow/src/pipeline/finalize.rs @@ -645,6 +645,7 @@ mod tests { use anyhow::Result; use async_trait::async_trait; use bytes::Bytes; + use fabro_auth::test_support as auth_test_support; use fabro_graphviz::graph::Graph; use fabro_model::Catalog; use fabro_sandbox::test_support::MockSandbox; @@ -1020,7 +1021,7 @@ mod tests { tokio_util::sync::CancellationToken::new(), fabro_model::ProviderId::anthropic(), "claude-sonnet-4-6".to_string(), - Arc::new(fabro_auth::EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), Arc::new(Catalog::from_builtin().expect("default catalog should build")), Arc::new(SandboxGitRuntime::new()), metadata_runtime, @@ -1053,7 +1054,7 @@ mod tests { tokio_util::sync::CancellationToken::new(), fabro_model::ProviderId::anthropic(), "claude-sonnet-4-6".to_string(), - Arc::new(fabro_auth::EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), Arc::new(Catalog::from_builtin().expect("default catalog should build")), Arc::new(SandboxGitRuntime::new()), Arc::new(RunMetadataRuntime::new()), diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 149a9ee59..0b776944b 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -5,8 +5,7 @@ use std::time::Instant; use fabro_agent::{Sandbox, ToolSecrets}; use fabro_auth::{ - CredentialSource, EnvCredentialSource, ExtraHeadersCredentialSource, VaultCredentialSource, - auth_issue_message, + CredentialSource, ExtraHeadersCredentialSource, VaultCredentialSource, auth_issue_message, }; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, HookRunner}; @@ -237,21 +236,12 @@ async fn build_registry( } } -#[expect( - clippy::disallowed_methods, - reason = "CLI/library workflow runs without a vault explicitly pass the Brave Search process-env credential into tool configuration; server runs pass a vault." -)] -async fn tool_secrets_from_configured_sources( - vault: Option<&Arc>>, -) -> ToolSecrets { - let brave_search_api_key = match vault { - Some(vault) => vault - .read() - .await - .get(EnvVars::BRAVE_SEARCH_API_KEY) - .map(str::to_string), - None => std::env::var(EnvVars::BRAVE_SEARCH_API_KEY).ok(), - }; +async fn tool_secrets_from_configured_sources(vault: &Arc>) -> ToolSecrets { + let brave_search_api_key = vault + .read() + .await + .get(EnvVars::BRAVE_SEARCH_API_KEY) + .map(str::to_string); ToolSecrets { brave_search_api_key, } @@ -267,15 +257,11 @@ fn graph_needs_api_backend(graph: &graph::Graph) -> bool { const SESSION_ID_HEADER: &str = "x-session-id"; fn build_llm_source( - vault: Option>>, + vault: Arc>, run_id: fabro_types::RunId, ) -> Arc { - let inner: Arc = match vault { - Some(vault) => Arc::new(VaultCredentialSource::new(vault)), - None => Arc::new(EnvCredentialSource::new()), - }; Arc::new(ExtraHeadersCredentialSource::new( - inner, + Arc::new(VaultCredentialSource::new(vault)), HashMap::from([(SESSION_ID_HEADER.to_string(), run_id.to_string())]), )) } @@ -298,7 +284,7 @@ pub async fn initialize( options.run_options.git = options.git.clone(); let llm_source = build_llm_source(options.vault.clone(), options.run_options.run_id); - let tool_secrets = tool_secrets_from_configured_sources(options.vault.as_ref()).await; + let tool_secrets = tool_secrets_from_configured_sources(&options.vault).await; let catalog = Arc::clone(&options.catalog); let sandbox_git = Arc::new(SandboxGitRuntime::new()); let metadata_runtime = Arc::new(RunMetadataRuntime::new()); @@ -356,14 +342,12 @@ pub async fn initialize( let instance = record.instance().ok_or_else(|| { Error::Precondition("cannot resume run: run sandbox was not initialized".to_string()) })?; - let daytona_api_key = match &options.vault { - Some(vault) => vault - .read() - .await - .get(EnvVars::DAYTONA_API_KEY) - .map(str::to_string), - None => None, - }; + let daytona_api_key = options + .vault + .read() + .await + .get(EnvVars::DAYTONA_API_KEY) + .map(str::to_string); let sandbox = reconnect_for_run_with_callback( instance, daytona_api_key, @@ -663,6 +647,7 @@ mod tests { use std::time::Duration; use fabro_acp::test_support::fake_acp_agent_script; + use fabro_auth::test_support as auth_test_support; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_interview::AutoApproveInterviewer; use fabro_sandbox::SandboxSpec; @@ -864,7 +849,7 @@ mod tests { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: None, @@ -945,7 +930,7 @@ mod tests { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: None, @@ -1053,7 +1038,7 @@ mod tests { let run_id = test_run_id(); let expected_session_id = run_id.to_string(); - let source = build_llm_source(Some(vault), run_id); + let source = build_llm_source(vault, run_id); let resolved = source.resolve(test_catalog().as_ref()).await.unwrap(); assert!(!resolved.credentials.is_empty()); @@ -1136,13 +1121,13 @@ mod tests { let store = memory_store(); let run_store = store.create_run(&test_run_id()).await.unwrap(); let initialized = initialize(test_persisted(graph, source, &run_dir), InitOptions { - run_store: run_store.into(), - dry_run: false, - emitter: emitter.clone(), - sandbox: SandboxSpec::Local { + run_store: run_store.into(), + dry_run: false, + emitter: emitter.clone(), + sandbox: SandboxSpec::Local { working_directory: temp.path().to_path_buf(), }, - llm: LlmSpec { + llm: LlmSpec { model: "fake-acp".to_string(), provider_id: fabro_model::ProviderId::openai(), fallback_chain: Vec::new(), @@ -1150,30 +1135,30 @@ mod tests { model_controls: RunModelControls::default(), dry_run: false, }, - interviewer: Arc::new(AutoApproveInterviewer::engine()), - steering_hub: Arc::new(crate::steering_hub::SteeringHub::new(emitter)), - catalog: test_catalog(), - lifecycle: crate::run_options::LifecycleOptions { + interviewer: Arc::new(AutoApproveInterviewer::engine()), + steering_hub: Arc::new(crate::steering_hub::SteeringHub::new(emitter)), + catalog: test_catalog(), + lifecycle: crate::run_options::LifecycleOptions { setup_commands: Vec::new(), setup_command_timeout_ms: 1_000, }, - run_options: test_settings(&run_dir), - workflow_path: None, - workflow_bundle: None, - hooks: fabro_hooks::HookSettings { hooks: vec![] }, - sandbox_env: SandboxEnvSpec { + run_options: test_settings(&run_dir), + workflow_path: None, + workflow_bundle: None, + hooks: fabro_hooks::HookSettings { hooks: vec![] }, + sandbox_env: SandboxEnvSpec { toml_env: HashMap::new(), github_permissions: None, origin_url: None, }, - vault: Some(vault), - git: None, - run_control: None, + vault, + git: None, + run_control: None, registry_override: None, - artifact_sink: None, - resume: None, - seed_context: None, - fabro_run_tools: None, + artifact_sink: None, + resume: None, + seed_context: None, + fabro_run_tools: None, }) .await .unwrap(); @@ -1261,7 +1246,7 @@ mod tests { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: None, @@ -1403,7 +1388,7 @@ mod tests { github_permissions: None, origin_url: None, }, - vault: None, + vault: auth_test_support::empty_vault(), git: None, run_control: None, registry_override: None, diff --git a/lib/components/fabro-workflow/src/pipeline/pull_request.rs b/lib/components/fabro-workflow/src/pipeline/pull_request.rs index 0dcc7745c..1266e0f41 100644 --- a/lib/components/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/components/fabro-workflow/src/pipeline/pull_request.rs @@ -677,7 +677,7 @@ mod tests { use std::time::Duration; use chrono::Utc; - use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; + use fabro_auth::{CredentialSource, VaultCredentialSource, test_support as auth_test_support}; use fabro_graphviz::graph::Graph; use fabro_llm::Error as LlmError; use fabro_llm::client::Client; @@ -819,7 +819,7 @@ mod tests { } fn test_llm_source() -> Arc { - Arc::new(EnvCredentialSource::new()) + auth_test_support::vault_only_credential_source() } fn test_projection() -> RunProjection { diff --git a/lib/components/fabro-workflow/src/pipeline/types.rs b/lib/components/fabro-workflow/src/pipeline/types.rs index 425cba9aa..288826686 100644 --- a/lib/components/fabro-workflow/src/pipeline/types.rs +++ b/lib/components/fabro-workflow/src/pipeline/types.rs @@ -299,7 +299,7 @@ pub struct InitOptions { pub workflow_bundle: Option>, pub hooks: fabro_hooks::HookSettings, pub sandbox_env: SandboxEnvSpec, - pub vault: Option>>, + pub vault: Arc>, pub git: Option, pub registry_override: Option>, pub artifact_sink: Option, diff --git a/lib/components/fabro-workflow/src/test_support.rs b/lib/components/fabro-workflow/src/test_support.rs index 5ece3320c..afc17a213 100644 --- a/lib/components/fabro-workflow/src/test_support.rs +++ b/lib/components/fabro-workflow/src/test_support.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use std::time::Duration; use fabro_agent::Sandbox; -use fabro_auth::{CredentialSource, EnvCredentialSource}; +use fabro_auth::{CredentialSource, test_support as auth_test_support}; use fabro_graphviz::graph::Graph as GvGraph; use fabro_interview::AutoApproveInterviewer; use fabro_model::Catalog; @@ -249,7 +249,7 @@ async fn initialized( "claude-sonnet-4-6".to_string(), options .llm_source - .unwrap_or_else(|| Arc::new(EnvCredentialSource::new())), + .unwrap_or_else(auth_test_support::vault_only_credential_source), Arc::new(Catalog::from_builtin().expect("default catalog should build")), Arc::new(SandboxGitRuntime::new()), Arc::new(RunMetadataRuntime::new()), diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index dc13bcbf8..46d189825 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -2165,7 +2165,6 @@ async fn smoke_test_with_mock_codergen_backend() { #[tokio::test] async fn shared_thread_compaction_before_routing_audit_succeeds() { - use fabro_auth::EnvCredentialSource; use fabro_workflow::steering_hub::SteeringHub; use httpmock::Method::POST; use httpmock::MockServer; @@ -2292,9 +2291,9 @@ reasoning = false )) .expect("test catalog should parse"); let catalog = Arc::new(Catalog::from_builtin_with_overrides(&settings).unwrap()); - let source = Arc::new(EnvCredentialSource::with_env_lookup(Arc::new(|name| { + let source = auth_test_support::env_credential_source(|name| { (name == "COMPACT_API_KEY").then(|| "sk-test".to_string()) - }))); + }); let backend = AgentApiBackend::new_with_catalog( "compact-model".to_string(), ProviderId::from("compact"), @@ -2397,7 +2396,6 @@ reasoning = false #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn workflow_persists_authoritative_openrouter_cost_for_agent_stage() { - use fabro_auth::EnvCredentialSource; use fabro_workflow::steering_hub::SteeringHub; use httpmock::Method::POST; use httpmock::MockServer; @@ -2448,9 +2446,9 @@ base_url = "{}" )) .expect("test catalog should parse"); let catalog = Arc::new(Catalog::from_builtin_with_overrides(&settings).unwrap()); - let source = Arc::new(EnvCredentialSource::with_env_lookup(Arc::new(|name| { + let source = auth_test_support::env_credential_source(|name| { (name == "OPENROUTER_API_KEY").then(|| "sk-test".to_string()) - }))); + }); let backend = AgentApiBackend::new_with_catalog( "openai/gpt-5.4".to_string(), ProviderId::from("openrouter"), @@ -6915,6 +6913,7 @@ mod real_llm { use std::sync::Arc; use async_trait::async_trait; + use fabro_auth::test_support as auth_test_support; use fabro_graphviz::graph::Node; use fabro_llm::client::Client; use fabro_llm::providers::OpenAiAdapter; @@ -7008,7 +7007,7 @@ mod real_llm { } fabro_test::require_env("ANTHROPIC_API_KEY")?; - let source = fabro_auth::EnvCredentialSource::new(); + let source = auth_test_support::StubCredentialSource; Some(Arc::new( Client::from_source(&source, super::default_catalog()) .await @@ -8418,7 +8417,7 @@ fn subgraph_without_label_no_class_derived() { fn hook_runner_from_defs(hooks: Vec) -> Arc { Arc::new(fabro_hooks::HookRunner::new( fabro_hooks::HookSettings { hooks }, - Arc::new(fabro_auth::EnvCredentialSource::new()), + auth_test_support::vault_only_credential_source(), default_catalog(), )) } @@ -10329,6 +10328,7 @@ async fn node_dir_uses_visit_count_on_revisit() { // Git checkpoint e2e (Local) // --------------------------------------------------------------------------- +use fabro_auth::test_support as auth_test_support; use fabro_workflow::handler::fan_in::FanInHandler; use fabro_workflow::handler::parallel::ParallelHandler; diff --git a/lib/foundation/fabro-auth/Cargo.toml b/lib/foundation/fabro-auth/Cargo.toml index 90ad9891e..8fcfa13b0 100644 --- a/lib/foundation/fabro-auth/Cargo.toml +++ b/lib/foundation/fabro-auth/Cargo.toml @@ -9,6 +9,9 @@ description = "Typed provider credential storage and resolution for Fabro" [lints] workspace = true +[features] +test-support = [] + [dependencies] anyhow.workspace = true async-trait.workspace = true diff --git a/lib/foundation/fabro-auth/src/env_source.rs b/lib/foundation/fabro-auth/src/env_source.rs deleted file mode 100644 index 8a2d7cc53..000000000 --- a/lib/foundation/fabro-auth/src/env_source.rs +++ /dev/null @@ -1,381 +0,0 @@ -use std::collections::HashMap; -use std::sync::Arc; - -use async_trait::async_trait; -use fabro_model::catalog::CatalogProvider; -use fabro_model::{Catalog, CredentialRef, ProviderId}; -use fabro_static::EnvVars; -use fabro_types::settings::ResolveCtx; - -use crate::credential_source::{CredentialSource, ResolvedCredentials}; -use crate::resolve::{apply_openai_api_env_context, apply_openai_codex_api_context}; -use crate::{ApiCredential, EnvLookup, ResolveError, build_api_key_header, resolve}; - -#[derive(Clone)] -pub struct EnvCredentialSource { - env_lookup: EnvLookup, -} - -impl EnvCredentialSource { - #[must_use] - #[expect( - clippy::disallowed_methods, - reason = "EnvCredentialSource is the provider API-key process-env facade." - )] - pub fn new() -> Self { - Self::with_env_lookup(Arc::new(|name| std::env::var(name).ok())) - } - - #[must_use] - pub fn with_env_lookup(env_lookup: EnvLookup) -> Self { - Self { env_lookup } - } - - fn lookup(&self, name: &str) -> Option { - (self.env_lookup)(name) - } - - fn credential_for( - &self, - provider: &CatalogProvider, - ) -> Result, ResolveError> { - let (auth_header, extra_headers) = match &provider.auth { - Some(auth) => { - let Some(key) = auth.credentials.iter().find_map(|credential_ref| { - let CredentialRef::Env(name) = credential_ref else { - return None; - }; - self.lookup(name) - }) else { - return Ok(None); - }; - ( - Some(build_api_key_header(auth.header.clone(), key)), - self.resolved_extra_headers(provider)?, - ) - } - None => (None, self.resolved_extra_headers(provider)?), - }; - - let mut cred = ApiCredential { - provider: provider.id.clone(), - auth_header, - extra_headers, - base_url: provider.base_url.clone(), - codex_mode: false, - org_id: None, - project_id: None, - }; - if provider.id == ProviderId::openai() && cred.auth_header.is_some() { - if let Some(account_id) = self.lookup(EnvVars::CHATGPT_ACCOUNT_ID) { - apply_openai_codex_api_context(&mut cred, Some(&account_id), &*self.env_lookup); - } else { - apply_openai_api_env_context(&mut cred, &*self.env_lookup); - } - } - Ok(Some(cred)) - } - - fn resolved_extra_headers( - &self, - provider: &CatalogProvider, - ) -> Result, ResolveError> { - let mut ctx = ResolveCtx::new().with_env(|env_name| self.lookup(env_name)); - resolve::resolve_extra_headers(&provider.id, &provider.extra_headers, &mut ctx) - } -} - -impl std::fmt::Debug for EnvCredentialSource { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("EnvCredentialSource") - .finish_non_exhaustive() - } -} - -impl Default for EnvCredentialSource { - fn default() -> Self { - Self::new() - } -} - -#[async_trait] -impl CredentialSource for EnvCredentialSource { - async fn resolve(&self, catalog: &Catalog) -> anyhow::Result { - let mut credentials = Vec::new(); - let mut auth_issues = Vec::new(); - - for provider in catalog.providers() { - match self.credential_for(provider) { - Ok(Some(credential)) => credentials.push(credential), - Ok(None) => {} - Err(ResolveError::NotConfigured(_) | ResolveError::Interpolation { .. }) - if provider.auth.is_some() => {} - Err(err) => auth_issues.push((provider.id.clone(), err)), - } - } - - Ok(ResolvedCredentials { - credentials, - auth_issues, - }) - } - - async fn configured_providers(&self, catalog: &Catalog) -> Vec { - catalog - .providers() - .iter() - .filter(|provider| match &provider.auth { - Some(auth) => auth.credentials.iter().any(|credential_ref| { - matches!(credential_ref, CredentialRef::Env(name) if self.lookup(name).is_some()) - }), - None => self.resolved_extra_headers(provider).is_ok(), - }) - .map(|provider| provider.id.clone()) - .collect() - } -} - -#[cfg(test)] -mod tests { - use std::collections::HashMap; - use std::sync::Arc; - - use fabro_model::catalog::LlmCatalogSettings; - use fabro_model::{Catalog, ProviderId}; - - use super::EnvCredentialSource; - use crate::CredentialSource; - - fn test_source(entries: &[(&str, &str)]) -> EnvCredentialSource { - let entries: HashMap = entries - .iter() - .map(|(key, value)| ((*key).to_string(), (*value).to_string())) - .collect(); - EnvCredentialSource::with_env_lookup(Arc::new(move |name| entries.get(name).cloned())) - } - - fn catalog_with(overrides: &str) -> Catalog { - let settings: LlmCatalogSettings = toml::from_str(overrides).unwrap(); - Catalog::from_builtin_with_overrides(&settings).unwrap() - } - - fn default_catalog() -> Catalog { - catalog_with("") - } - - /// A no-auth portkey provider whose only variation is its `extra_headers` - /// TOML lines. - fn portkey_catalog(extra_headers: &str) -> Catalog { - catalog_with(&format!( - r#" -[providers.portkey] -display_name = "Portkey Bedrock" -adapter = "anthropic" -agent_profile = "anthropic" -base_url = "https://api.portkey.ai/v1" - -[providers.portkey.extra_headers] -{extra_headers} - -[models."portkey-claude"] -provider = "portkey" -display_name = "Portkey Claude" -family = "claude" -default = true - -[models."portkey-claude".limits] -context_window = 200000 - -[models."portkey-claude".features] -tools = true -vision = true -reasoning = true -reasoning_effort = "levels" -"# - )) - } - - #[tokio::test] - async fn configured_providers_reads_injected_env() { - let source = test_source(&[("ANTHROPIC_API_KEY", "anthropic-key")]); - let catalog = default_catalog(); - - assert_eq!(source.configured_providers(&catalog).await, vec![ - ProviderId::anthropic() - ]); - } - - #[tokio::test] - async fn resolve_returns_empty_when_no_keys_are_configured() { - let source = test_source(&[]); - let catalog = default_catalog(); - - let resolved = source.resolve(&catalog).await.unwrap(); - - assert!(resolved.credentials.is_empty()); - assert!(resolved.auth_issues.is_empty()); - } - - #[tokio::test] - async fn resolve_builds_openai_codex_env_credential() { - let source = test_source(&[ - ("OPENAI_API_KEY", "openai-key"), - ("CHATGPT_ACCOUNT_ID", "acct_123"), - ("OPENAI_PROJECT_ID", "project_123"), - ]); - let catalog = default_catalog(); - - let resolved = source.resolve(&catalog).await.unwrap(); - let credential = resolved.credentials.first().unwrap(); - - assert_eq!(credential.provider, ProviderId::openai()); - assert!(credential.codex_mode); - assert_eq!( - credential.base_url.as_deref(), - Some("https://chatgpt.com/backend-api/codex") - ); - assert_eq!( - credential.extra_headers.get("ChatGPT-Account-Id"), - Some(&"acct_123".to_string()) - ); - assert_eq!(credential.project_id.as_deref(), Some("project_123")); - } - - #[tokio::test] - async fn resolve_uses_catalog_credentials_and_base_url_for_openai_compatible_providers() { - let source = test_source(&[("KIMI_API_KEY", "kimi-key")]); - let catalog = default_catalog(); - - let resolved = source.resolve(&catalog).await.unwrap(); - let credential = resolved.credentials.first().unwrap(); - - assert_eq!(credential.provider, ProviderId::new("kimi")); - assert_eq!( - credential.base_url.as_deref(), - Some("https://api.moonshot.ai/v1") - ); - } - - #[tokio::test] - async fn resolve_registers_custom_env_backed_provider() { - let catalog = catalog_with( - r#" -[providers.acme] -display_name = "Acme" -adapter = "openai_compatible" -agent_profile = "openai" -base_url = "https://api.acme.test/v1" - -[providers.acme.auth] -credentials = ["env:ACME_API_KEY"] - -[models."acme-large"] -provider = "acme" -display_name = "Acme Large" -family = "acme" -default = true - -[models."acme-large".limits] -context_window = 128000 - -[models."acme-large".features] -tools = true -vision = false -reasoning = false -"#, - ); - let source = test_source(&[("ACME_API_KEY", "acme-key")]); - - let resolved = source.resolve(&catalog).await.unwrap(); - let credential = resolved - .credentials - .iter() - .find(|credential| credential.provider == ProviderId::new("acme")) - .expect("custom provider should resolve from the supplied catalog"); - - assert_eq!( - credential.auth_header.as_ref().unwrap(), - &crate::ApiKeyHeader::Bearer("acme-key".to_string(),) - ); - assert_eq!( - credential.base_url.as_deref(), - Some("https://api.acme.test/v1") - ); - } - - #[tokio::test] - async fn env_source_resolves_literal_and_env_header_tokens() { - let catalog = portkey_catalog( - r#" -x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}" -x-portkey-provider = "@bedrock-prod" -"#, - ); - let source = test_source(&[("PORTKEY_API_KEY", "pk-live")]); - - let resolved = source.resolve(&catalog).await.unwrap(); - let credential = resolved - .credentials - .iter() - .find(|credential| credential.provider == ProviderId::new("portkey")) - .expect("no-auth provider should register when extra headers resolve"); - - assert!(credential.auth_header.is_none()); - assert_eq!( - credential.extra_headers.get("x-portkey-api-key"), - Some(&"pk-live".to_string()) - ); - assert_eq!( - credential.extra_headers.get("x-portkey-provider"), - Some(&"@bedrock-prod".to_string()) - ); - } - - #[tokio::test] - async fn env_source_secrets_header_token_is_unavailable() { - let catalog = portkey_catalog(r#"x-team-secret = "{{ secrets.gateway_team_secret }}""#); - let source = test_source(&[]); - - let resolved = source.resolve(&catalog).await.unwrap(); - - assert!( - !resolved - .credentials - .iter() - .any(|credential| credential.provider == ProviderId::new("portkey")) - ); - let (_, issue) = resolved - .auth_issues - .iter() - .find(|(provider, _)| provider == &ProviderId::new("portkey")) - .expect("secrets token should surface as an auth issue"); - assert!(matches!( - issue, - crate::ResolveError::Interpolation { provider, .. } - if provider == &ProviderId::new("portkey") - )); - assert!(issue.to_string().contains("gateway_team_secret")); - } - - #[tokio::test] - async fn env_source_reports_missing_env_header_for_no_auth_provider() { - let catalog = portkey_catalog(r#"x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}""#); - let source = test_source(&[]); - - let resolved = source.resolve(&catalog).await.unwrap(); - - assert!( - !resolved - .credentials - .iter() - .any(|credential| credential.provider == ProviderId::new("portkey")) - ); - let (_, issue) = resolved - .auth_issues - .iter() - .find(|(provider, _)| provider == &ProviderId::new("portkey")) - .expect("missing env header should surface as an auth issue"); - assert!(matches!(issue, crate::ResolveError::Interpolation { .. })); - assert!(issue.to_string().contains("PORTKEY_API_KEY")); - } -} diff --git a/lib/foundation/fabro-auth/src/lib.rs b/lib/foundation/fabro-auth/src/lib.rs index 77c217317..b57075718 100644 --- a/lib/foundation/fabro-auth/src/lib.rs +++ b/lib/foundation/fabro-auth/src/lib.rs @@ -1,12 +1,13 @@ mod context; mod credential; mod credential_source; -mod env_source; mod extra_headers_source; mod refresh; mod resolve; mod sql_vault_source; mod strategy; +#[cfg(any(test, feature = "test-support"))] +pub mod test_support; mod vault_ext; mod vault_source; @@ -15,13 +16,11 @@ pub mod strategies; pub use context::{AuthContextRequest, AuthContextResponse}; pub use credential::{ApiKeyHeader, OAuthConfig, OAuthCredential, OAuthTokens}; pub use credential_source::{CredentialSource, ResolvedCredentials}; -pub use env_source::EnvCredentialSource; pub use extra_headers_source::ExtraHeadersCredentialSource; pub use refresh::refresh_oauth_credential; pub use resolve::{ ApiCredential, CredentialResolver, CredentialUsage, EnvLookup, ResolveError, ResolvedCredential, auth_issue_message, build_api_key_header, - configured_providers_from_process_env, }; pub use sql_vault_source::SqlVaultCredentialSource; pub use strategy::{ diff --git a/lib/foundation/fabro-auth/src/resolve.rs b/lib/foundation/fabro-auth/src/resolve.rs index 60a4576b7..55afb8a30 100644 --- a/lib/foundation/fabro-auth/src/resolve.rs +++ b/lib/foundation/fabro-auth/src/resolve.rs @@ -10,8 +10,6 @@ use tokio::sync::RwLock as AsyncRwLock; use tokio::task::spawn_blocking; use crate::credential::{ApiKeyHeader, OAuthCredential}; -use crate::credential_source::CredentialSource; -use crate::env_source::EnvCredentialSource; use crate::refresh::refresh_oauth_credential; use crate::vault_ext::{ VaultLookupError, vault_get_oauth, vault_get_token, vault_set_oauth, vault_token_lookup, @@ -515,23 +513,6 @@ fn vault_lookup_error(provider: &ProviderId, name: &str, err: VaultLookupError) } } -pub async fn configured_providers_from_process_env( - vault: Option<&Arc>>, - catalog: &Catalog, -) -> Vec { - match vault { - Some(vault_arc) => { - let resolver = CredentialResolver::new(Arc::clone(vault_arc)); - let guard = vault_arc.read().await; - resolver.configured_providers(&guard, catalog) - } - None => { - EnvCredentialSource::new() - .configured_providers(catalog) - .await - } - } -} #[cfg(test)] mod tests { use std::error::Error as _; diff --git a/lib/foundation/fabro-auth/src/test_support.rs b/lib/foundation/fabro-auth/src/test_support.rs new file mode 100644 index 000000000..944f83342 --- /dev/null +++ b/lib/foundation/fabro-auth/src/test_support.rs @@ -0,0 +1,62 @@ +//! Test-only credential sources. +//! +//! Feature-gated so they never link into production builds. Production code +//! resolves credentials through [`VaultCredentialSource`] over a real vault; +//! these helpers exist so tests can supply a source without one. + +use std::collections::HashMap; +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_model::{Catalog, ProviderId}; +use fabro_vault::Vault; +use tokio::sync::RwLock as AsyncRwLock; + +use crate::credential_source::{CredentialSource, ResolvedCredentials}; +use crate::vault_source::VaultCredentialSource; + +/// A credential source that resolves nothing, for tests that need a source but +/// never make a provider request. +#[derive(Debug, Default, Clone, Copy)] +pub struct StubCredentialSource; + +#[async_trait] +impl CredentialSource for StubCredentialSource { + async fn resolve(&self, _catalog: &Catalog) -> anyhow::Result { + Ok(ResolvedCredentials { + credentials: Vec::new(), + auth_issues: Vec::new(), + }) + } + + async fn configured_providers(&self, _catalog: &Catalog) -> Vec { + Vec::new() + } +} + +/// A detached in-memory vault holding no secrets. +#[must_use] +pub fn empty_vault() -> Arc> { + Arc::new(AsyncRwLock::new(Vault::from_entries(HashMap::new()))) +} + +/// A vault-backed source whose credentials come only from `env_lookup`. +/// +/// Tests that inject fake provider keys use this instead of reading the real +/// process environment, which would make them order-dependent. +#[must_use] +pub fn env_credential_source(env_lookup: F) -> Arc +where + F: Fn(&str) -> Option + Send + Sync + 'static, +{ + Arc::new(VaultCredentialSource::with_env_lookup( + empty_vault(), + env_lookup, + )) +} + +/// A vault-backed source over an empty vault with no process-env fallback. +#[must_use] +pub fn vault_only_credential_source() -> Arc { + Arc::new(VaultCredentialSource::vault_only(empty_vault())) +}