From e54fef760adf423c34f474cf854c3a3aabc0fc6a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 20:36:54 -0400 Subject: [PATCH 1/4] 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())) +} From f0a7423b510f8ae2b9ae6da6afbffba269644426 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 21:09:35 -0400 Subject: [PATCH 2/4] refactor(config): stop resolving {{ env.* }} in interpolated config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The process environment is no longer a configuration source. `{{ vars.NAME }}` (non-sensitive, server-stored) and `{{ secrets.NAME }}` (vault-backed) cover both cases, and reading the worker's ambient environment made a run's inputs depend on how its process happened to be launched. `Namespace::Env` is kept but wired to nothing, so `{{ env.NAME }}` still parses and fails with a message naming its replacement rather than reaching a consumer as literal text. `ResolveCtx::with_env` is gone, so no call site can opt back in. Two long-standing warts were env-only and go with it: - `InterpString::resolve_or_source`, the "fall back to the raw template source on failure" path, which let an unresolved token reach a sandbox or the GitHub API as literal `{{ ... }}` text. Its own comment noted it was slated for hard-error semantics. - `RunEnvironmentSettings::resolve_env`'s matching source fallback for env-only values. Both carried `#[expect(clippy::disallowed_methods)]` escape hatches. Every run-boundary resolver — sandbox env, prepare steps, MCP transports, GitHub permissions, Slack channels, run goal files, provider extra_headers — now fails closed instead. Hooks lose their `allowed_env_vars` allowlist, `resolve_header`, and `HeaderResolveError` along with the `E: Env` generic threaded through the executor. They keep `{{ vars.* }}`, which `RunSettings::substitute_variables` already substitutes server-side at run creation. `allowed_env_vars` is removed from the OpenAPI spec and the generated TypeScript client. The docs example showing `{{ env.* }}` in `[server.slatedb.s3].bucket` was already wrong — that field is a plain String and never interpolated — and is now a literal. Co-Authored-By: Claude Opus 5 (1M context) --- .../administration/server-configuration.mdx | 4 +- docs/public/agents/hooks.mdx | 10 +- docs/public/agents/mcp.mdx | 3 +- docs/public/api-reference/fabro-api.yaml | 16 +- docs/public/core-concepts/models.mdx | 4 +- docs/public/execution/environments.mdx | 6 +- docs/public/execution/run-configuration.mdx | 17 +- docs/public/integrations/slack.mdx | 4 +- docs/public/reference/user-configuration.mdx | 6 +- docs/public/workflows/variables.mdx | 2 +- lib/apps/fabro-cli/src/commands/exec.rs | 10 +- lib/apps/fabro-cli/src/commands/run/runner.rs | 11 +- lib/apps/fabro-server/src/run_manifest.rs | 4 +- lib/apps/fabro-server/src/server.rs | 22 +- lib/apps/fabro-server/src/server/tests.rs | 16 +- lib/components/fabro-hooks/src/executor.rs | 335 ++++------------- .../fabro-sandbox/src/from_environment.rs | 23 +- .../fabro-workflow/src/operations/start.rs | 35 +- lib/foundation/fabro-auth/src/resolve.rs | 27 +- lib/foundation/fabro-config/src/layers/run.rs | 34 +- .../fabro-config/src/resolve/mod.rs | 7 +- .../fabro-config/src/resolve/run.rs | 1 - lib/foundation/fabro-config/src/run.rs | 29 +- .../fabro-types/src/settings/interp.rs | 186 +++++----- .../fabro-types/src/settings/run.rs | 339 ++++++------------ .../src/models/hook-definition.ts | 4 - .../src/models/prepared-step.ts | 2 +- 27 files changed, 369 insertions(+), 788 deletions(-) diff --git a/docs/public/administration/server-configuration.mdx b/docs/public/administration/server-configuration.mdx index 49a448138..e0d01155f 100644 --- a/docs/public/administration/server-configuration.mdx +++ b/docs/public/administration/server-configuration.mdx @@ -218,7 +218,7 @@ provider = "s3" disk_cache = true [server.slatedb.s3] -bucket = "{{ env.SLATEDB_BUCKET }}" +bucket = "fabro-production" region = "us-east-1" ``` @@ -463,7 +463,7 @@ GitHub App mode stores these secrets in the vault. `fabro install` writes them a ### Slack integration (optional) -Slack credentials are server-level secrets. Add `[server.integrations.slack]` to enable one Slack connection that is shared by human interview prompts and run lifecycle notifications. `server.integrations.slack.default_channel` is an optional literal channel name used only as the default destination for interview prompts; it does not interpolate `{{ env.* }}`. Lifecycle notifications use `[run.notifications..slack].channel` in run or workflow configuration. +Slack credentials are server-level secrets. Add `[server.integrations.slack]` to enable one Slack connection that is shared by human interview prompts and run lifecycle notifications. `server.integrations.slack.default_channel` is an optional literal channel name used only as the default destination for interview prompts; it does not interpolate. Lifecycle notifications use `[run.notifications..slack].channel` in run or workflow configuration. Fabro resolves these from the vault only. When `[server.integrations.slack]` is present and both credentials are present, startup logs `Slack integration enabled` and then the Slack Socket Mode connection status. If the Slack config table is absent or `enabled = false`, startup logs `Slack integration disabled by server configuration`. If the table is present but either credential is missing or empty, startup logs `Slack integration disabled; missing credentials` with the missing variable names. diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index aafdd356f..6ac3de5cb 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -28,19 +28,19 @@ POST the event context as JSON to an HTTP endpoint. Useful for webhooks, externa event = "run_complete" type = "http" url = "https://hooks.example.com/done" -allowed_env_vars = ["API_KEY"] [hooks.headers] -Authorization = "Bearer {{ env.API_KEY }}" +Authorization = "Bearer {{ vars.WEBHOOK_TOKEN }}" ``` | Field | Description | |---|---| -| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. Supports `{{ env.NAME }}` interpolation. | -| `headers` | Optional HTTP headers. Values support `{{ env.NAME }}` interpolation, scoped to the names in `allowed_env_vars`. A token for any other env var fails to resolve and the hook blocks (fail-closed). | -| `allowed_env_vars` | Allowlist of environment variable names a header may read via `{{ env.NAME }}`. Empty (the default) means no env vars may be interpolated into headers. | +| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. Supports `{{ vars.NAME }}` interpolation. | +| `headers` | Optional HTTP headers. Values support `{{ vars.NAME }}` interpolation. A token that is still unresolved when the hook fires blocks it (fail-closed), so a header is never sent half-rendered. | | `tls` | TLS mode: `"verify"` (default), `"no_verify"`, or `"off"`. | +`{{ vars.NAME }}` is substituted when the run is created. `{{ env.NAME }}` and `{{ secrets.NAME }}` are not available in hooks. + ### Prompt A single-turn LLM call that evaluates the event context and returns an `ok`/`block` decision. The model responds with structured JSON. diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index 0fdd921e0..06c7e645a 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -149,12 +149,11 @@ Inline transport fields can interpolate values at the run boundary: | Syntax | Resolution time | |---|---| | `{{ vars.NAME }}` | When the server creates the run, using that run's variable snapshot | -| `{{ env.NAME }}` | When the worker launches the MCP transport | | `{{ secrets.NAME }}` | When the worker launches the MCP transport, using a token secret from the server vault | Interpolation applies to stdio and sandbox commands and env values, plus HTTP URLs and headers. Variable tokens are replaced in the created run configuration. Worker-time environment and secret expressions remain in persisted configuration, while resolved secret values do not. A missing environment variable, missing secret, or non-token secret fails MCP startup instead of passing an unresolved token to the transport. -Standalone `fabro exec` can resolve `{{ env.* }}` from its process environment, but it has no server vault. A `{{ secrets.* }}` reference therefore fails with an explicit error in standalone execution. +Standalone `fabro exec` has no server vault, so a `{{ secrets.* }}` reference fails with an explicit error in standalone execution. ## Transports diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index b028d89c0..6d4362735 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14169,7 +14169,7 @@ components: script-vs-argv distinction via the `type` discriminator: a `script` is a raw shell snippet kept verbatim, while a `command` is an argv whose elements are shell-quoted and joined at the run boundary (after - `{{ env.* }}` resolution) so an interpolated value cannot inject shell + `{{ secrets.* }}` resolution) so an interpolated value cannot inject shell syntax. Optional per-step `env` is shared by both shapes. type: object required: [type] @@ -14574,17 +14574,9 @@ components: - type: "null" description: >- Optional HTTP headers for an http hook. Values support - `{{ env.NAME }}` interpolation, scoped to the names listed in - `allowed_env_vars`; a token for any other env var fails to resolve - and the hook blocks (fail-closed). - allowed_env_vars: - type: array - items: - type: string - description: >- - Allowlist of environment variable names that an http hook header may - read via `{{ env.NAME }}`. An empty list (the default) permits no env - vars in headers. + `{{ vars.NAME }}` interpolation, substituted when the run is + created; a token left unresolved at fire time blocks the hook + (fail-closed). tls: $ref: "#/components/schemas/TlsMode" prompt: diff --git a/docs/public/core-concepts/models.mdx b/docs/public/core-concepts/models.mdx index 1a6f094b3..52560d428 100644 --- a/docs/public/core-concepts/models.mdx +++ b/docs/public/core-concepts/models.mdx @@ -84,7 +84,7 @@ aliases = ["gateway"] credentials = ["env:ACME_GATEWAY_API_KEY", "vault:ACME_GATEWAY_API_KEY"] [llm.providers.proxy.extra_headers] -x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}" +x-portkey-api-key = "{{ secrets.PORTKEY_API_KEY }}" x-portkey-config = "@bedrock-prod" [llm.providers.proxy.models."team-code-large"] @@ -153,7 +153,7 @@ Historical built-in catalog keys that exposed provider API IDs remain accepted a Model roles are separate: `default = true` controls normal model selection for workflow execution, while `small_default = true` marks the provider's small/cheap utility model for metadata tasks such as generated run titles. If a provider has no small default, Fabro falls back to that provider's normal default. -Provider auth is declared in `[llm.providers..auth]` with ordered `env:` or `vault:` refs. The primary auth header defaults to `bearer`; override with `header = { custom = "Header-Name" }` for providers like Anthropic that use `x-api-key`. Omit the `[llm.providers..auth]` block entirely for providers that need no API key (e.g. Ollama). Custom headers for any provider — including providers that need only interpolation headers and no API-key auth — go in `extra_headers` as literal text, `{{ env.NAME }}` tokens, or `{{ secrets.NAME }}` tokens. Put credentials in secrets and reference them with `{{ secrets.NAME }}` instead of a bare literal. +Provider auth is declared in `[llm.providers..auth]` with ordered `env:` or `vault:` refs. The primary auth header defaults to `bearer`; override with `header = { custom = "Header-Name" }` for providers like Anthropic that use `x-api-key`. Omit the `[llm.providers..auth]` block entirely for providers that need no API key (e.g. Ollama). Custom headers for any provider — including providers that need only interpolation headers and no API-key auth — go in `extra_headers` as literal text or `{{ secrets.NAME }}` tokens. Put credentials in secrets and reference them with `{{ secrets.NAME }}` instead of a bare literal. Workflow runs also add `x-session-id: ` to every LLM request so compatible gateways can group requests from the same run. An explicitly configured `x-session-id` in provider `extra_headers` takes precedence. diff --git a/docs/public/execution/environments.mdx b/docs/public/execution/environments.mdx index 22c4da95e..82a72d5ed 100644 --- a/docs/public/execution/environments.mdx +++ b/docs/public/execution/environments.mdx @@ -152,16 +152,16 @@ preserve = true ## Environment value interpolation -Environment `env` values can mix literal text with `{{ vars.NAME }}`, `{{ env.NAME }}`, and `{{ secrets.NAME }}` tokens: +Environment `env` values can mix literal text with `{{ vars.NAME }}` and `{{ secrets.NAME }}` tokens: ```toml title="workflow.toml" [environments.fabro-dev.env] DEPLOY_ENV = "{{ vars.DEPLOY_ENV }}" -SERVICE_URL = "https://api.{{ env.REGION }}.example.com" +SERVICE_URL = "https://api.{{ vars.REGION }}.example.com" SERVICE_TOKEN = "{{ secrets.SERVICE_TOKEN }}" ``` -Server-managed variables resolve when the run is created. Worker environment variables and token secrets resolve immediately before the sandbox starts, so resolved secret values are not persisted in the run definition. A missing or non-token secret fails closed. For backward compatibility, a value containing only missing `{{ env.* }}` references is passed through in source form. +Server-managed variables resolve when the run is created. Token secrets resolve immediately before the sandbox starts, so resolved secret values are not persisted in the run definition. A missing or non-token secret fails closed, as does any `{{ env.* }}` reference: the process environment is not a configuration source. ## Selecting an environment from the CLI diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 300f0a802..207d2df1d 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -79,7 +79,7 @@ memory = "8GB" disk = "20GB" [environments.cloud.env] -API_KEY = "{{ env.MY_API_KEY }}" +API_KEY = "{{ secrets.MY_API_KEY }}" NODE_ENV = "production" [run.integrations.github.permissions] @@ -192,13 +192,13 @@ env = { NPM_TOKEN = "{{ secrets.NPM_TOKEN }}" } | Field | Description | |---|---| -| `script` | Bash source, evaluated by the sandbox's non-login Bash (`bash -c`). Supports `{{ vars.* }}`, `{{ env.* }}`, and `{{ secrets.* }}` interpolation. | +| `script` | Bash source, evaluated by the sandbox's non-login Bash (`bash -c`). Supports `{{ vars.* }}` and `{{ secrets.* }}` interpolation. | | `command` | Argv-style command, mutually exclusive with `script`. Each resolved element is shell-quoted as one argument. | | `env` | Additional environment variables for this step. Values support the same interpolation as `script` and `command`. | Each step must exit with status 0. If any step fails, the run aborts before the workflow starts. Prepare steps replace across layers — the higher-precedence layer wins wholesale. -Fabro substitutes `{{ vars.* }}` when the server creates the run, then resolves `{{ env.* }}` from the worker process and `{{ secrets.* }}` from token entries in the server vault immediately before the worker executes the steps. Worker-time environment and secret expressions remain in the persisted run definition; resolved secret values are not persisted. A missing environment variable, missing secret, or non-token secret aborts startup with the affected step and token named in the error. +Fabro substitutes `{{ vars.* }}` when the server creates the run, then resolves `{{ secrets.* }}` from token entries in the server vault immediately before the worker executes the steps. Secret expressions remain in the persisted run definition; resolved secret values are not persisted. A missing or non-token secret aborts startup with the affected step and token named in the error. ### `[run.clone]` @@ -301,7 +301,7 @@ Environment variable values can combine literal text with server variables, work [environments.ci.env] API_KEY = "{{ secrets.SERVICE_API_KEY }}" NODE_ENV = "production" -SERVICE_URL = "https://api.{{ env.REGION }}.example.com" +SERVICE_URL = "https://api.{{ vars.REGION }}.example.com" RELEASE_CHANNEL = "{{ vars.RELEASE_CHANNEL }}" ``` @@ -309,11 +309,10 @@ RELEASE_CHANNEL = "{{ vars.RELEASE_CHANNEL }}" |---|---| | `"literal"` | Static value passed as-is | | `"{{ vars.NAME }}"` | Server-managed variable substituted when the run is created | -| `"{{ env.VARNAME }}"` | Worker process environment value resolved when the run starts | | `"{{ secrets.NAME }}"` | Token secret resolved from the server vault when the run starts | -| `"prefix-{{ env.X }}-suffix"` | Substring interpolation; multiple supported tokens per string are allowed | +| `"prefix-{{ vars.X }}-suffix"` | Substring interpolation; multiple supported tokens per string are allowed | -Missing or non-token secret references fail closed before sandbox startup. For backward compatibility, an environment value that references only a missing `{{ env.* }}` value is passed through in source form; use preflight or prepare-step interpolation when an absent worker variable must be a hard error. +Missing or non-token secret references fail closed before sandbox startup. `{{ env.* }}` is not supported: the process environment is not a configuration source. Use `{{ vars.NAME }}` for a non-sensitive value or `{{ secrets.NAME }}` for a credential. ### `[run.integrations.github.permissions]` @@ -349,7 +348,7 @@ channel = "#deploys" | `enabled` | Enables this route. Defaults to `false`. | | `provider` | Notification provider. Use `"slack"` for Slack lifecycle notifications. Other provider names may be parsed but are not delivered by the server yet. | | `events` | Raw Fabro event names that trigger this route, such as `run.started`, `run.completed`, and `run.failed`. | -| `[run.notifications..slack].channel` | Required for Slack lifecycle notifications. Literal channel names and `{{ env.VAR }}` interpolation are supported. | +| `[run.notifications..slack].channel` | Required for Slack lifecycle notifications. Literal channel names and `{{ vars.NAME }}` interpolation are supported. | Each enabled Slack route posts once for each matching lifecycle event. Messages include the run ID, an Open in Fabro link when available, workflow label, terminal result, duration, and pull request details when those are already present in the run event stream. @@ -489,7 +488,7 @@ id = "sentry" | `startup_timeout` | Max duration for server startup + MCP handshake (e.g. `"10s"`, `"1m"`). | `"10s"` | | `tool_timeout` | Max duration for a single tool call. | `"60s"` | -Inline transport commands, URLs, env values, and headers support `{{ vars.* }}`, `{{ env.* }}`, and `{{ secrets.* }}` interpolation. As with prepare steps, server variables resolve at run creation and worker env/token secrets resolve at launch; missing values fail closed. See [MCP runtime interpolation](/agents/mcp#runtime-interpolation) for the standalone `fabro exec` difference. +Inline transport commands, URLs, env values, and headers support `{{ vars.* }}` and `{{ secrets.* }}` interpolation. As with prepare steps, server variables resolve at run creation and token secrets resolve at launch; missing values fail closed. See [MCP runtime interpolation](/agents/mcp#runtime-interpolation) for the standalone `fabro exec` difference. The `sandbox` transport runs the MCP server inside the workflow's sandbox. This is useful for tools that need access to the sandbox environment, such as browser automation with Playwright. See [MCP](/agents/mcp#sandbox) for details. diff --git a/docs/public/integrations/slack.mdx b/docs/public/integrations/slack.mdx index 9420be8fd..11c8de802 100644 --- a/docs/public/integrations/slack.mdx +++ b/docs/public/integrations/slack.mdx @@ -117,7 +117,7 @@ enabled = true default_channel = "#fabro-reviews" ``` -`default_channel` is a literal channel name used only for human-in-the-loop interview prompts. Fabro does not interpolate `{{ env.* }}` in this server setting. Run lifecycle notifications use per-run or per-workflow `[run.notifications]` routes instead, whose channel values can use environment interpolation. +`default_channel` is a literal channel name used only for human-in-the-loop interview prompts; Fabro does not interpolate it. Run lifecycle notifications use per-run or per-workflow `[run.notifications]` routes instead, whose channel values support `{{ vars.NAME }}` interpolation. ### 8. Invite the bot @@ -182,7 +182,7 @@ Each enabled route posts one message when a matching event is emitted. Lifecycle `run.failed` is a terminal run event. A stage can fail and still be followed by another graph edge that lets the run complete; in that case a route listening for `run.completed` fires, not `run.failed`. -The route-level Slack channel is required for lifecycle notifications. The channel may be a literal (`"#deploys"`) or an environment interpolation (`"{{ env.DEPLOYS_SLACK_CHANNEL }}"`). If the channel is missing, empty, or cannot be resolved, Fabro logs a warning and skips that route without affecting the run or other notification routes. +The route-level Slack channel is required for lifecycle notifications. The channel may be a literal (`"#deploys"`) or a server variable (`"{{ vars.DEPLOYS_SLACK_CHANNEL }}"`). If the channel is missing, empty, or cannot be resolved, Fabro logs a warning and skips that route without affecting the run or other notification routes. Lifecycle notifications are one-way and fire-and-forget. They never accept answers, register reply threads, update prior messages, or interact with interview state. diff --git a/docs/public/reference/user-configuration.mdx b/docs/public/reference/user-configuration.mdx index 8ae378442..1e6021555 100644 --- a/docs/public/reference/user-configuration.mdx +++ b/docs/public/reference/user-configuration.mdx @@ -94,7 +94,7 @@ aliases = ["gateway"] credentials = ["env:ACME_GATEWAY_API_KEY", "vault:ACME_GATEWAY_API_KEY"] [llm.providers.proxy.extra_headers] -x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}" +x-portkey-api-key = "{{ secrets.PORTKEY_API_KEY }}" x-portkey-config = "@bedrock-prod" [llm.providers.proxy.models."team-code-large"] @@ -184,7 +184,7 @@ aliases = ["gateway"] credentials = ["env:ACME_GATEWAY_API_KEY", "vault:ACME_GATEWAY_API_KEY"] [llm.providers.proxy.extra_headers] -x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}" +x-portkey-api-key = "{{ secrets.PORTKEY_API_KEY }}" x-portkey-config = "@bedrock-prod" x-team-secret = "{{ secrets.gateway_team_secret }}" ``` @@ -199,7 +199,7 @@ x-team-secret = "{{ secrets.gateway_team_secret }}" | `auth` | table | omitted | API-key auth config. Omit the table entirely for providers that need no API key; any `extra_headers` are still attached. | | `auth.credentials` | array | required when `auth` present | Ordered credential refs. Accepted forms are `vault:`, `env:`, and `aws_sigv4` (sign requests from the AWS default credential chain — Bedrock). Literal secret strings are rejected. | | `auth.header` | `"bearer"` or `{ custom = "Header-Name" }` | `"bearer"` | Primary API-key header policy. Omit when the provider uses a standard bearer token. | -| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are interpolation strings: literal text, an `{{ env.NAME }}` token, or a `{{ secrets.NAME }}` token. Put credentials in a secret and reference them with a `{{ secrets.NAME }}` token, not a bare literal. | +| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are interpolation strings: literal text or a `{{ secrets.NAME }}` token. Put credentials in a secret and reference them with a `{{ secrets.NAME }}` token, not a bare literal. | | `priority` | integer | `0` | Higher-priority ready providers win unqualified model and default selection; ties use canonical provider ID. | | `enabled` | boolean | `true` | Set `false` to disable a provider after lower-precedence layers define it. | | `aliases` | array | `[]` | Additional provider names accepted by model routing and fallback config. | diff --git a/docs/public/workflows/variables.mdx b/docs/public/workflows/variables.mdx index fb42d126b..c541d126f 100644 --- a/docs/public/workflows/variables.mdx +++ b/docs/public/workflows/variables.mdx @@ -15,7 +15,7 @@ Goal templates can reference inputs and server-managed variables. Prompt templat | `{{ inputs.name }}` | A value from `[run.inputs]`, optionally overridden by CLI input flags | | `{{ vars.NAME }}` | A server-managed variable snapshotted when the run is created | -Environment variables and secrets are **not** available in goal or prompt templates. Use `{{ env.NAME }}` and `{{ secrets.NAME }}` only in the configuration fields that support run-boundary interpolation. +Secrets are **not** available in goal or prompt templates. Use `{{ secrets.NAME }}` only in the configuration fields that support run-boundary interpolation. ## Run config inputs diff --git a/lib/apps/fabro-cli/src/commands/exec.rs b/lib/apps/fabro-cli/src/commands/exec.rs index 49316eacf..48d729c63 100644 --- a/lib/apps/fabro-cli/src/commands/exec.rs +++ b/lib/apps/fabro-cli/src/commands/exec.rs @@ -282,14 +282,6 @@ impl ProviderAdapter for AuthenticatedFabroServerAdapter { } } -#[expect( - clippy::disallowed_methods, - reason = "exec-boundary MCP transport InterpString resolution facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - fn run_mcp_servers_for_exec( mcps: &HashMap, ) -> AnyResult> { @@ -351,7 +343,7 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu .into_iter() .map(|settings| { settings - .resolve_transport_env(process_env_var, |_| None) + .resolve_transport_env(|_| None) .with_context(|| format!("failed to resolve MCP server {:?}", settings.name)) }) .collect::>>()?; diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 45c2dcecf..b031d7620 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -167,7 +167,8 @@ pub(crate) async fn execute( .run .integrations .github - .resolve_permissions(process_env_var), + .resolve_permissions() + .context("failed to resolve github permissions")?, vault, catalog, on_node: None, @@ -1153,14 +1154,6 @@ fn maybe_build_github_credentials( Ok(None) } -#[expect( - clippy::disallowed_methods, - reason = "CLI worker InterpString resolution facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - /// Hard-gate for the CLI worker path: a run-level token is requested, or /// a clone-based sandbox in non-dry-run mode will need credentials to /// pull the repository. Pull-request-driven credential acquisition is diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 65cc6d36e..84a55693a 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -44,7 +44,6 @@ use futures_util::stream::{self, StreamExt}; use tokio::process::Command; use tokio::time; -use crate::interp::process_env_var; use crate::server::AppState; use crate::server_secrets::LlmClientResult; @@ -1214,7 +1213,8 @@ async fn run_github_token_check( let github_permissions = resolved_run .integrations .github - .resolve_permissions(process_env_var); + .resolve_permissions() + .unwrap_or_default(); let perm_details = github_permissions .iter() diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index daeadaea2..284800b85 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -152,7 +152,6 @@ use crate::git_checkout::GitRepoCache; use crate::github_webhooks::{ WEBHOOK_ROUTE, WEBHOOK_SECRET_ENV, parse_event_metadata, verify_signature, }; -use crate::interp::process_env_var; use crate::jwt_auth::{self, AuthMode}; use crate::principal_middleware::{ AuthContextSlot, RequestAuth, RequestAuthContext, RequireRunBlob, RequireRunManagementTarget, @@ -873,13 +872,8 @@ impl SlackService { let blocks = &blocks; let posts = routes.into_iter().filter_map(|(route_name, route)| { - let channel = resolve_slack_lifecycle_route_channel( - state, - event.run_id, - route_name, - route, - event_name, - )?; + let channel = + resolve_slack_lifecycle_route_channel(event.run_id, route_name, route, event_name)?; Some(async move { if let Err(err) = self.client.post_message(&channel, blocks, None).await { warn!( @@ -1059,7 +1053,6 @@ fn slack_lifecycle_pull_request_from_link(link: &PullRequestLink) -> SlackLifecy } fn resolve_slack_lifecycle_route_channel( - state: &AppState, run_id: RunId, route_name: &str, route: &NotificationRouteSettings, @@ -1079,7 +1072,10 @@ fn resolve_slack_lifecycle_route_channel( return None; }; - let resolved = match channel.resolve(|name| (state.env_lookup)(name)) { + // `{{ vars.* }}` is substituted at run creation, so the channel is literal + // here; anything still unresolved skips the route rather than sending to a + // half-rendered channel name. + let resolved = match channel.resolve_with(&mut fabro_types::settings::ResolveCtx::new()) { Ok(resolved) => resolved, Err(err) => { warn!( @@ -4088,7 +4084,11 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { .run .integrations .github - .resolve_permissions(process_env_var); + .resolve_permissions() + .unwrap_or_else(|err| { + tracing::warn!(error = %err, "github permission interpolation failed"); + std::collections::HashMap::new() + }); let vault = match state.stores.vault.snapshot().await { Ok(vault) => vault, Err(err) => { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 71b4dfebf..5ed5bb140 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -4587,19 +4587,11 @@ async fn slack_lifecycle_missing_channel_is_skipped_without_blocking_other_route "100.5", ) .await; - let state = test_app_state_with_env_lookup( - default_test_server_settings(), - fabro_config::RunLayer::default(), - 5, - |name| match name { - "SLACK_ROUTE_CHANNEL" => Some("#ops".to_string()), - _ => None, - }, - ); + let state = test_app_state(); let service = slack_lifecycle_service(server.base_url(), None); let run_id = fixtures::RUN_1; let settings = workflow_settings_with_run_notifications( - r#" + r##" [run.notifications.missing] enabled = true provider = "slack" @@ -4619,8 +4611,8 @@ provider = "slack" events = ["run.started"] [run.notifications.valid.slack] -channel = "{{ env.SLACK_ROUTE_CHANNEL }}" -"#, +channel = "#ops" +"##, Some("Deploy workflow"), ); let run_store = create_slack_notification_run(&state, run_id, settings, "deploy", None).await; diff --git a/lib/components/fabro-hooks/src/executor.rs b/lib/components/fabro-hooks/src/executor.rs index d92f73c8e..9acc2fdaa 100644 --- a/lib/components/fabro-hooks/src/executor.rs +++ b/lib/components/fabro-hooks/src/executor.rs @@ -1,6 +1,5 @@ use std::borrow::Cow; use std::collections::HashMap; -use std::fmt; use std::sync::{Arc, LazyLock}; use std::time::Instant; @@ -13,9 +12,7 @@ use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; use fabro_model::Catalog; use fabro_redact::redacted_url_for_log; -use fabro_types::settings::interp::Namespace; -use fabro_types::settings::{InterpString, ResolveError}; -use fabro_util::env::{Env, SystemEnv}; +use fabro_types::settings::{InterpString, ResolveCtx, ResolveError}; use tokio::process::Command as TokioCommand; use tokio::time::timeout as tokio_timeout; use tokio_util::sync::CancellationToken; @@ -57,27 +54,22 @@ pub trait HookExecutor: Send + Sync { ) -> HookResult; } -/// Resolve a typed [`InterpString`] hook segment at fire time, looking up -/// `{{ env.* }}` tokens against `env`. +/// Resolve a typed [`InterpString`] hook segment at fire time. /// -/// Only the `env` namespace is wired here; `{{ secrets.* }}`, `{{ vars.* }}`, -/// and `{{ inputs.* }}` tokens have no lookup in this context and resolve as -/// `Unavailable`, which is a hard error — so a hook that references one fails -/// closed rather than firing with a half-resolved value. +/// No namespace is wired here. `{{ vars.* }}` is already substituted +/// server-side when the run is created, so a literal value resolves unchanged +/// and any remaining token — `secrets`, `inputs`, `env` — surfaces as +/// `Unavailable`. That is a hard error, so a hook referencing one fails closed +/// rather than firing with a half-resolved value. /// /// The value stays typed end-to-end: it is carried as an `InterpString` /// through the config resolve layer and resolved here from its segments — -/// there is no `InterpString -> String -> InterpString` re-parse. A missing or -/// out-of-scope token is a hard error (fail-closed); there is no fallback to -/// the unresolved source. +/// there is no `InterpString -> String -> InterpString` re-parse. /// /// Returns the typed [`ResolveError`] so callers keep the source until the /// decision boundary renders it; do not flatten it to a `String` here. -fn resolve_interp(value: &InterpString, env: &E) -> Result -where - E: Env + ?Sized, -{ - value.resolve(|name| env.var(name).ok()) +fn resolve_interp(value: &InterpString) -> Result { + value.resolve_with(&mut ResolveCtx::new()) } #[expect( @@ -90,66 +82,6 @@ fn safe_url_source_for_log(url: &InterpString) -> String { redacted_url_for_log(&url.as_source()) } -#[derive(Debug, Clone, PartialEq, Eq)] -enum HeaderResolveError { - NotAllowed { name: String }, - Resolve(ResolveError), -} - -impl fmt::Display for HeaderResolveError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::NotAllowed { name } => write!( - f, - "environment variable {name:?} referenced by an HTTP hook header is not listed in \ - allowed_env_vars" - ), - Self::Resolve(error) => error.fmt(f), - } - } -} - -impl std::error::Error for HeaderResolveError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::NotAllowed { .. } => None, - Self::Resolve(error) => Some(error), - } - } -} - -/// Resolve an HTTP-hook **header** value at fire time, scoping its -/// `{{ env.* }}` lookups to `allowed_env_vars`. -/// -/// Headers carry credentials, so unlike every other hook field they read env -/// through an allowlist: a `{{ env.NAME }}` token resolves only when `NAME` is -/// listed in the hook's `allowed_env_vars`. A name outside the allowlist fails -/// with a distinct error before any lookup, while an allowlisted-but-unset name -/// still surfaces as the normal `Missing` error. An empty `allowed_env_vars` -/// therefore permits no env vars in headers at all. This mirrors the previous -/// template-based `with_env_lookup_allowed` behavior without reviving any -/// template engine. -fn resolve_header( - value: &InterpString, - allowed_env_vars: &[String], - env: &E, -) -> Result -where - E: Env + ?Sized, -{ - if let Some(name) = value.names(Namespace::Env).into_iter().find(|name| { - !allowed_env_vars - .iter() - .any(|allowed| allowed.as_str() == *name) - }) { - return Err(HeaderResolveError::NotAllowed { - name: name.to_string(), - }); - } - - resolve_interp(value, env).map_err(HeaderResolveError::Resolve) -} - /// Executes hooks via shell commands or HTTP POST. pub struct HookExecutorImpl; @@ -179,36 +111,27 @@ impl HookExecutorImpl { /// Resolve the prompt and optional model segments at fire time. /// - /// Fail-closed: only `{{ env.* }}` is wired here; a missing env token (or a - /// token in any other, unavailable namespace) is a hard error so the hook - /// never fires with a half-resolved value. The caller turns the error into - /// a `Block` decision, matching the command-hook behavior. - fn resolve_prompt_and_model( + /// Fail-closed: an unresolved token is a hard error so the hook never + /// fires with a half-resolved value. The caller turns the error into a + /// `Block` decision, matching the command-hook behavior. + fn resolve_prompt_and_model( prompt: &InterpString, model: Option<&InterpString>, - env: &E, - ) -> Result<(String, Option), ResolveError> - where - E: Env + ?Sized, - { - let prompt = resolve_interp(prompt, env)?; - let model = model.map(|model| resolve_interp(model, env)).transpose()?; + ) -> Result<(String, Option), ResolveError> { + let prompt = resolve_interp(prompt)?; + let model = model.map(resolve_interp).transpose()?; Ok((prompt, model)) } /// Execute a command hook (sandbox or host). - async fn execute_command( + async fn execute_command( definition: &HookDefinition, command: &InterpString, context: &HookContext, sandbox: &Arc, execution_context: &HookExecutionContext, - env: &E, - ) -> HookDecision - where - E: Env + ?Sized, - { - let command = match resolve_interp(command, env) { + ) -> HookDecision { + let command = match resolve_interp(command) { Ok(command) => command, Err(error) => { return HookDecision::Block { @@ -354,19 +277,15 @@ impl HookExecutorImpl { } /// Execute a prompt hook: single-turn LLM call returning ok/block. - async fn execute_prompt( + async fn execute_prompt( definition: &HookDefinition, prompt: &InterpString, model: Option<&InterpString>, context: &HookContext, - env: &E, llm_source: &dyn CredentialSource, catalog: Arc, - ) -> HookDecision - where - E: Env + ?Sized, - { - let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) { + ) -> HookDecision { + let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model) { Ok(resolved) => resolved, Err(error) => { tracing::error!(error = %error, "prompt hook env resolution failed, not firing"); @@ -421,21 +340,17 @@ impl HookExecutorImpl { /// Reuses the core `ToolRegistry` from `fabro_agent` so the agent hook has /// the same tools (read_file, write_file, shell, grep, glob, etc.) as /// a normal agent session. - async fn execute_agent( + async fn execute_agent( definition: &HookDefinition, prompt: &InterpString, model: Option<&InterpString>, max_tool_rounds: Option, context: &HookContext, sandbox: Arc, - env: &E, llm_source: &dyn CredentialSource, catalog: Arc, - ) -> HookDecision - where - E: Env + ?Sized, - { - let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) { + ) -> HookDecision { + let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model) { Ok(resolved) => resolved, Err(error) => { tracing::error!(error = %error, "agent hook env resolution failed, not firing"); @@ -566,20 +481,15 @@ impl HookExecutorImpl { /// half-resolved URL or an empty credential header. Transport outcomes /// (non-2xx, connection errors, unparseable body) stay fail-open and /// return `Proceed`. - async fn execute_http( + async fn execute_http( client: &fabro_http::HttpClient, url: &InterpString, headers: Option<&HashMap>, - allowed_env_vars: &[String], tls: &TlsMode, context: &HookContext, timeout: std::time::Duration, - env: &E, - ) -> HookDecision - where - E: Env + ?Sized, - { - let resolved_url = match resolve_interp(url, env) { + ) -> HookDecision { + let resolved_url = match resolve_interp(url) { Ok(url) => url, Err(error) => { tracing::error!( @@ -611,11 +521,7 @@ impl HookExecutorImpl { if let Some(hdrs) = headers { for (key, value) in hdrs { - // Headers resolve through the per-hook env allowlist: a - // `{{ env.NAME }}` not in `allowed_env_vars` blocks before any - // lookup, while an allowlisted-but-unset name still fails as - // missing. - let interpolated = match resolve_header(value, allowed_env_vars, env) { + let interpolated = match resolve_interp(value) { Ok(rendered) => rendered, Err(error) => { tracing::error!( @@ -730,34 +636,24 @@ impl HookExecutor for HookExecutorImpl { static HTTP_CLIENTS: OnceLock = OnceLock::new(); let start = Instant::now(); - let env = SystemEnv; let decision = match definition.resolved_hook_type() { Some( Cow::Borrowed(HookType::Command { ref command }) | Cow::Owned(HookType::Command { ref command }), ) => { - Self::execute_command( - definition, - command, - context, - &sandbox, - execution_context, - &env, - ) - .await + Self::execute_command(definition, command, context, &sandbox, execution_context) + .await } Some( Cow::Borrowed(HookType::Http { ref url, ref headers, - ref allowed_env_vars, ref tls, }) | Cow::Owned(HookType::Http { ref url, ref headers, - ref allowed_env_vars, ref tls, }), ) => { @@ -766,11 +662,9 @@ impl HookExecutor for HookExecutorImpl { clients.get(*tls), url, headers.as_ref(), - allowed_env_vars, tls, context, definition.timeout(), - &env, ) .await } @@ -789,7 +683,6 @@ impl HookExecutor for HookExecutorImpl { prompt, model.as_ref(), context, - &env, llm_source, Arc::clone(&catalog), ) @@ -814,7 +707,6 @@ impl HookExecutor for HookExecutorImpl { *max_tool_rounds, context, sandbox, - &env, llm_source, Arc::clone(&catalog), ) @@ -838,7 +730,7 @@ impl HookExecutor for HookExecutorImpl { mod tests { use fabro_auth::{CredentialSource, test_support}; use fabro_types::fixtures; - use fabro_util::env::TestEnv; + use fabro_types::settings::ResolveErrorKind; use super::*; use crate::config::HookType; @@ -1144,14 +1036,6 @@ mod tests { // --- hook segment resolution helpers --- - fn test_env(vars: &[(&str, &str)]) -> TestEnv { - TestEnv( - vars.iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(), - ) - } - fn interp(value: &str) -> InterpString { InterpString::parse(value) } @@ -1175,81 +1059,31 @@ mod tests { assert_eq!(safe, ""); } - // Headers resolve `{{ env.NAME }}` tokens through the per-hook - // `allowed_env_vars` allowlist: an allowlisted name resolves, anything else - // fails closed before lookup. + /// Hook values are resolved from their typed segments at fire time, never + /// via a String -> InterpString re-parse. `{{ vars.* }}` is already + /// substituted server-side, so a literal value passes straight through. #[test] - fn header_resolves_allowlisted_var() { - let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); - let result = resolve_header( - &interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), - &["FABRO_TEST_KEY_1".to_string()], - &env, - ) - .unwrap(); - assert_eq!(result, "Bearer secret123"); - } - - // Fail-closed: a header may not read an env var that is set in the process - // but missing from `allowed_env_vars`. This is distinct from an unset - // allowlisted variable, so the block reason points at the allowlist. - #[test] - fn header_rejects_unlisted_var() { - let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); - let err = resolve_header( - &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), - &[], - &env, - ) - .unwrap_err(); - assert_eq!(err, HeaderResolveError::NotAllowed { - name: "FABRO_TEST_KEY_3".to_string(), - }); - } - - #[test] - fn header_missing_token_is_hard_error() { - let env = test_env(&[]); - let err = resolve_header( - &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), - &["FABRO_TEST_KEY_3".to_string()], - &env, - ) - .unwrap_err(); - match err { - HeaderResolveError::Resolve(error) => assert_eq!(error.name, "FABRO_TEST_KEY_3"), - HeaderResolveError::NotAllowed { .. } => { - panic!("expected missing token resolve error, got {err:?}") - } - } - } - - // The value stays a typed `InterpString`: it resolves at fire time from its - // segments, never via a String -> InterpString re-parse. - #[test] - fn resolve_interp_resolves_embedded_token_from_typed_value() { - let env = test_env(&[("FABRO_TEST_KEY_2", "val")]); - let value = interp("x{{ env.FABRO_TEST_KEY_2 }}y"); - let result = resolve_interp(&value, &env).unwrap(); - assert_eq!(result, "xvaly"); - } - - #[test] - fn resolve_interp_errors_on_missing_var() { - let env = test_env(&[]); - let err = resolve_interp(&interp("a{{ env.FABRO_TEST_NOEXIST }}-b"), &env).unwrap_err(); - assert_eq!(err.name, "FABRO_TEST_NOEXIST"); - } - - #[test] - fn resolve_interp_without_tokens_passes_through() { - let env = test_env(&[]); + fn resolve_interp_passes_through_literal_values() { + assert_eq!(resolve_interp(&interp("plain text")).unwrap(), "plain text"); assert_eq!( - resolve_interp(&interp("plain text"), &env).unwrap(), - "plain text" + resolve_interp(&interp("Bearer already-substituted")).unwrap(), + "Bearer already-substituted" ); } + /// Fail-closed: a token that survived to fire time can never resolve, so a + /// hook referencing one blocks rather than sending a half-rendered header. + #[test] + fn resolve_interp_errors_on_an_unresolved_token() { + let err = resolve_interp(&interp("a{{ env.FABRO_TEST_NOEXIST }}-b")).unwrap_err(); + assert_eq!(err.name, "FABRO_TEST_NOEXIST"); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + + let err = resolve_interp(&interp("Bearer {{ secrets.API_KEY }}")).unwrap_err(); + assert_eq!(err.name, "API_KEY"); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + } + // --- HTTP hook execution tests --- #[tokio::test] @@ -1270,11 +1104,9 @@ mod tests { &client, &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1299,11 +1131,9 @@ mod tests { &client, &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1326,11 +1156,9 @@ mod tests { &client, &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1345,21 +1173,19 @@ mod tests { &client, &interp("http://127.0.0.1:1"), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(1), - &test_env(&[]), ) .await; assert_eq!(decision, HookDecision::Proceed); } + /// `{{ vars.* }}` is substituted server-side, so a header arrives literal + /// and is sent as-is. #[tokio::test] - async fn http_hook_sends_interpolated_headers() { - let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); - + async fn http_hook_sends_substituted_headers() { let server = httpmock::MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -1370,21 +1196,16 @@ mod tests { }) .await; - let headers = HashMap::from([( - "Authorization".to_string(), - interp("Bearer {{ env.FABRO_TEST_TOKEN }}"), - )]); + let headers = HashMap::from([("Authorization".to_string(), interp("Bearer my-secret"))]); let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, &interp(&server.url("/hook")), Some(&headers), - &["FABRO_TEST_TOKEN".to_string()], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &env, ) .await; @@ -1392,12 +1213,10 @@ mod tests { assert_eq!(decision, HookDecision::Proceed); } - // Fail-closed: a header that references an env var set in the process but - // absent from `allowed_env_vars` must block and never fire the request. + /// Fail-closed: `{{ env.* }}` no longer resolves anywhere, so a header + /// referencing one must block rather than send a half-rendered credential. #[tokio::test] - async fn http_hook_unlisted_header_var_blocks_without_firing() { - let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); - + async fn http_hook_env_header_token_blocks_without_firing() { let server = httpmock::MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -1416,12 +1235,9 @@ mod tests { &client, &interp(&server.url("/hook")), Some(&headers), - // Empty allowlist: the env var is set, but headers may read nothing. - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &env, ) .await; @@ -1432,15 +1248,15 @@ mod tests { reason .as_deref() .is_some_and(|reason| reason.contains("FABRO_TEST_TOKEN")), - "block reason should name the unlisted token, got: {reason:?}" + "block reason should name the token, got: {reason:?}" ); } - other => panic!("expected Block on unlisted header var, got {other:?}"), + other => panic!("expected Block on env header token, got {other:?}"), } } #[tokio::test] - async fn http_hook_resolves_url_before_dispatch() { + async fn http_hook_dispatches_a_substituted_url() { let server = httpmock::MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -1450,16 +1266,13 @@ mod tests { .await; let client = test_http_client(); - let env = test_env(&[("FABRO_TEST_URL", &server.url("/hook"))]); let decision = HookExecutorImpl::execute_http( &client, - &interp("{{ env.FABRO_TEST_URL }}"), + &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &env, ) .await; @@ -1482,11 +1295,9 @@ mod tests { &client, &interp("{{ env.FABRO_TEST_MISSING_URL }}/hook"), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1526,11 +1337,9 @@ mod tests { &interp(&server.url("/hook")), Some(&headers), // Allowlisted but unset: still blocks on the Missing lookup. - &["FABRO_TEST_MISSING_HEADER".to_string()], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1549,11 +1358,9 @@ mod tests { &client, &interp("http://example.com/hook"), None, - &[], &TlsMode::Verify, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1567,11 +1374,9 @@ mod tests { &client, &interp("http://example.com/hook"), None, - &[], &TlsMode::NoVerify, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1593,11 +1398,9 @@ mod tests { &client, &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1621,10 +1424,9 @@ mod tests { event: HookEvent::StageStart, command: None, hook_type: Some(HookType::Http { - url: interp(&server.url("/hook")), - headers: None, - allowed_env_vars: vec![], - tls: TlsMode::Off, + url: interp(&server.url("/hook")), + headers: None, + tls: TlsMode::Off, }), matcher: None, blocking: None, @@ -1659,7 +1461,6 @@ mod tests { &make_context(), &sandbox, &HookExecutionContext::default(), - &test_env(&[]), ) .await; @@ -1675,7 +1476,6 @@ mod tests { &interp("{{ env.MISSING_HOOK_VALUE }}"), None, &make_context(), - &test_env(&[]), test_llm_source().as_ref(), test_catalog(), ) @@ -1704,7 +1504,6 @@ mod tests { Some(1), &make_context(), make_sandbox(), - &test_env(&[]), test_llm_source().as_ref(), test_catalog(), ) diff --git a/lib/components/fabro-sandbox/src/from_environment.rs b/lib/components/fabro-sandbox/src/from_environment.rs index ac972f22c..5399927ea 100644 --- a/lib/components/fabro-sandbox/src/from_environment.rs +++ b/lib/components/fabro-sandbox/src/from_environment.rs @@ -71,13 +71,18 @@ pub fn docker_config_from_environment( settings: &RunEnvironmentSettings, skip_clone: bool, ) -> DockerSandboxOptions { - // No vault is available on this path (server preflight / manifest), so - // resolve `{{ env.* }}` against the process environment and let every other - // token (including `{{ secrets.* }}`) fall back to its source form. + // No vault is available on this path (server preflight / manifest), so a + // `{{ secrets.* }}` value keeps its source form. Nothing else is left to + // resolve: `{{ vars.* }}` is substituted at run creation. + #[expect( + clippy::disallowed_methods, + reason = "preflight has no vault, so an unresolved secret token is carried in source \ + form; the real value is resolved by docker_config_from_environment_with_secrets" + )] let env = settings .env .iter() - .map(|(key, value)| (key.clone(), value.resolve_or_source(process_env_var))) + .map(|(key, value)| (key.clone(), value.as_source())) .collect(); docker_config_from_environment_env(settings, skip_clone, env) } @@ -88,7 +93,7 @@ pub fn docker_config_from_environment_with_secrets( skip_clone: bool, secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { - let env = settings.resolve_env(process_env_var, secrets_lookup)?; + let env = settings.resolve_env(secrets_lookup)?; Ok(docker_config_from_environment_env( settings, skip_clone, env, )) @@ -158,14 +163,6 @@ pub fn local_working_directory_from_environment( } #[cfg(feature = "docker")] -#[expect( - clippy::disallowed_methods, - reason = "Environment interpolation owns a process-env lookup facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - #[cfg(feature = "daytona")] fn duration_to_minutes_i32(duration: std::time::Duration) -> i32 { let minutes = duration.as_secs() / 60; diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index eb3d49218..d9fbd89e6 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -393,9 +393,7 @@ impl RunSession { .mcps .iter() .map(|(key, entry)| match entry { - ResolvedMcpEntry::Resolved(server) => { - runtime_mcp_server(server, process_env_var, secret_lookup) - } + ResolvedMcpEntry::Resolved(server) => runtime_mcp_server(server, secret_lookup), // References must be resolved to concrete servers before the run // spec is persisted (server-side run-preparation pass). Reaching // worker startup with an unresolved reference is an invariant @@ -449,7 +447,7 @@ impl RunSession { let toml_env = resolved .environment - .resolve_env(process_env_var, secret_lookup) + .resolve_env(secret_lookup) .map_err(|err| Error::engine_with_source("failed to resolve run environment", err))?; let github_permissions: Option> = (!services.github_permissions.is_empty()).then(|| services.github_permissions.clone()); @@ -467,8 +465,7 @@ impl RunSession { }; let pr_config = resolved.pull_request.clone(); - let setup_commands = - runtime_setup_commands(&resolved.prepare, process_env_var, secret_lookup)?; + let setup_commands = runtime_setup_commands(&resolved.prepare, secret_lookup)?; drop(vault_guard); Ok(Self { @@ -739,11 +736,10 @@ impl ModelRegistry for CatalogModelRegistry<'_> { /// error — no fallback to the unresolved source. fn runtime_mcp_server( settings: &ResolvedMcpServerSettings, - env_lookup: impl FnMut(&str) -> Option, secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { settings - .resolve_transport_env(env_lookup, secrets_lookup) + .resolve_transport_env(secrets_lookup) .map_err(|err| { Error::engine_with_source( format!("failed to resolve MCP server {:?}", settings.name), @@ -766,11 +762,10 @@ fn runtime_mcp_server( /// a hard error — no fallback to the unresolved source. fn runtime_setup_commands( prepare: &ResolvedRunPrepareSettings, - env_lookup: impl FnMut(&str) -> Option, secrets_lookup: impl FnMut(&str) -> Option, ) -> Result, Error> { let resolved = prepare - .resolve_step_env(env_lookup, secrets_lookup) + .resolve_step_env(secrets_lookup) .map_err(|err| Error::engine_with_source("failed to resolve prepare step", err))?; Ok(resolved .steps @@ -1576,7 +1571,7 @@ reasoning = false ..ResolvedMcpServerSettings::default() }; - let err = runtime_mcp_server(&settings, |_| None, |_| None).unwrap_err(); + let err = runtime_mcp_server(&settings, |_| None).unwrap_err(); assert_eq!( err.to_string(), @@ -1598,8 +1593,7 @@ reasoning = false )]), )); - let commands = - runtime_setup_commands(&prepare, |_| None, vault_secret_lookup(&vault)).unwrap(); + let commands = runtime_setup_commands(&prepare, vault_secret_lookup(&vault)).unwrap(); assert_eq!(commands.len(), 1); assert_eq!( @@ -1617,8 +1611,7 @@ reasoning = false HashMap::new(), )); - let commands = - runtime_setup_commands(&prepare, |_| None, vault_secret_lookup(&vault)).unwrap(); + let commands = runtime_setup_commands(&prepare, vault_secret_lookup(&vault)).unwrap(); let tokens = shlex::split(&commands[0].command).expect("resolved command should remain valid shell"); @@ -1646,8 +1639,7 @@ reasoning = false ..ResolvedMcpServerSettings::default() }; - let resolved = - runtime_mcp_server(&settings, |_| None, vault_secret_lookup(&vault)).unwrap(); + let resolved = runtime_mcp_server(&settings, vault_secret_lookup(&vault)).unwrap(); let ResolvedMcpTransport::Stdio { env, .. } = resolved.transport else { panic!("expected stdio transport"); @@ -1666,8 +1658,7 @@ reasoning = false HashMap::new(), )); - let Err(err) = runtime_setup_commands(&prepare, |_| None, vault_secret_lookup(&vault)) - else { + let Err(err) = runtime_setup_commands(&prepare, vault_secret_lookup(&vault)) else { panic!("missing secret should fail setup command resolution"); }; @@ -1691,8 +1682,7 @@ reasoning = false )]), )); - let Err(err) = runtime_setup_commands(&prepare, |_| None, vault_secret_lookup(&vault)) - else { + let Err(err) = runtime_setup_commands(&prepare, vault_secret_lookup(&vault)) else { panic!("OAuth secret should fail setup command resolution"); }; @@ -1714,8 +1704,7 @@ reasoning = false )]), )); - let Err(err) = runtime_setup_commands(&prepare, |_| None, vault_secret_lookup(&vault)) - else { + let Err(err) = runtime_setup_commands(&prepare, vault_secret_lookup(&vault)) else { panic!("file secret should fail setup command resolution"); }; diff --git a/lib/foundation/fabro-auth/src/resolve.rs b/lib/foundation/fabro-auth/src/resolve.rs index 55afb8a30..ee74a3f85 100644 --- a/lib/foundation/fabro-auth/src/resolve.rs +++ b/lib/foundation/fabro-auth/src/resolve.rs @@ -218,8 +218,7 @@ impl CredentialResolver { }; if catalog_provider.auth.is_none() { let vault = self.vault.read().await; - return self - .api_credential_from_provider_auth(&vault, catalog_provider, catalog) + return Self::api_credential_from_provider_auth(&vault, catalog_provider, catalog) .map(ResolvedCredential::Api); } let initial_secret = { @@ -312,9 +311,7 @@ impl CredentialResolver { catalog: &Catalog, ) -> bool { let Some(auth) = &provider.auth else { - return self - .resolved_extra_headers_for_catalog(vault, &provider.id, catalog) - .is_ok(); + return Self::resolved_extra_headers_for_catalog(vault, &provider.id, catalog).is_ok(); }; auth.credentials.iter().any(|credential_ref| { self.credential_from_ref(vault, &provider.id, credential_ref) @@ -352,10 +349,6 @@ impl CredentialResolver { } } - fn lookup_env(&self, name: &str) -> Option { - (self.env_lookup)(name) - } - fn provider_base_url_for_catalog(provider: &ProviderId, catalog: &Catalog) -> Option { catalog .provider(provider) @@ -363,7 +356,6 @@ impl CredentialResolver { } fn resolved_extra_headers_for_catalog( - &self, vault: &Vault, provider: &ProviderId, catalog: &Catalog, @@ -371,9 +363,8 @@ impl CredentialResolver { let Some(catalog_provider) = catalog.provider(provider) else { return Ok(HashMap::new()); }; - let mut ctx = ResolveCtx::new() - .with_env(|env_name| self.lookup_env(env_name)) - .with_secrets(|secret_name| vault_token_lookup(vault, secret_name)); + let mut ctx = + ResolveCtx::new().with_secrets(|secret_name| vault_token_lookup(vault, secret_name)); resolve_extra_headers(provider, &catalog_provider.extra_headers, &mut ctx) } @@ -391,7 +382,7 @@ impl CredentialResolver { ResolvedSecret::AwsSigv4 => Ok(ApiCredential { provider: provider_id.clone(), auth_header: Some(ApiKeyHeader::AwsSigv4), - extra_headers: self.resolved_extra_headers_for_catalog( + extra_headers: Self::resolved_extra_headers_for_catalog( vault, provider_id, catalog, @@ -409,7 +400,7 @@ impl CredentialResolver { let mut cred = ApiCredential { provider: provider_id.clone(), auth_header: Some(auth_header), - extra_headers: self.resolved_extra_headers_for_catalog( + extra_headers: Self::resolved_extra_headers_for_catalog( vault, provider_id, catalog, @@ -428,7 +419,7 @@ impl CredentialResolver { let mut api_credential = ApiCredential { provider: provider_id.clone(), auth_header: Some(ApiKeyHeader::Bearer(credential.tokens.access_token.clone())), - extra_headers: self.resolved_extra_headers_for_catalog( + extra_headers: Self::resolved_extra_headers_for_catalog( vault, provider_id, catalog, @@ -451,7 +442,6 @@ impl CredentialResolver { } fn api_credential_from_provider_auth( - &self, vault: &Vault, provider: &CatalogProvider, catalog: &Catalog, @@ -459,8 +449,7 @@ impl CredentialResolver { if provider.auth.is_some() { return Err(ResolveError::NotConfigured(provider.id.clone())); } - let extra_headers = - self.resolved_extra_headers_for_catalog(vault, &provider.id, catalog)?; + let extra_headers = Self::resolved_extra_headers_for_catalog(vault, &provider.id, catalog)?; Ok(ApiCredential { provider: provider.id.clone(), auth_header: None, diff --git a/lib/foundation/fabro-config/src/layers/run.rs b/lib/foundation/fabro-config/src/layers/run.rs index 655b79640..552cbab57 100644 --- a/lib/foundation/fabro-config/src/layers/run.rs +++ b/lib/foundation/fabro-config/src/layers/run.rs @@ -730,40 +730,38 @@ impl<'de> Deserialize<'de> for McpEntryLayer { pub struct HookEntry { /// Optional merge identity. Hooks with the same `id` replace in place. #[serde(default, skip_serializing_if = "Option::is_none")] - pub id: Option, + pub id: Option, /// Display-only human name. #[serde(default, skip_serializing_if = "Option::is_none")] - pub name: Option, - pub event: HookEvent, + pub name: Option, + pub event: HookEvent, #[serde(default, skip_serializing_if = "Option::is_none")] - pub matcher: Option, + pub matcher: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub blocking: Option, + pub blocking: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub timeout: Option, + pub timeout: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub sandbox: Option, + pub sandbox: Option, // Exactly one of the following groups is expected: #[serde(default, skip_serializing_if = "Option::is_none")] - pub script: Option, + pub script: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub command: Option>, + pub command: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] - pub url: Option, + pub url: Option, #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub headers: HashMap, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub allowed_env_vars: Vec, + pub headers: HashMap, #[serde(default, skip_serializing_if = "Option::is_none")] - pub tls: Option, + pub tls: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub prompt: Option, + pub prompt: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub model: Option, + pub model: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub max_tool_rounds: Option, + pub max_tool_rounds: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub agent: Option, + pub agent: Option, } #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] diff --git a/lib/foundation/fabro-config/src/resolve/mod.rs b/lib/foundation/fabro-config/src/resolve/mod.rs index 083042604..487f55aa9 100644 --- a/lib/foundation/fabro-config/src/resolve/mod.rs +++ b/lib/foundation/fabro-config/src/resolve/mod.rs @@ -202,13 +202,12 @@ Authorization = "Bearer {{ env.HOOK_TOKEN }}" assert_eq!( hook.resolved_hook_type().as_deref(), Some(&HookType::Http { - url: InterpString::parse("https://hooks.example.com"), - headers: Some(HashMap::from([( + url: InterpString::parse("https://hooks.example.com"), + headers: Some(HashMap::from([( "Authorization".to_string(), InterpString::parse("Bearer {{ env.HOOK_TOKEN }}"), )])), - allowed_env_vars: Vec::new(), - tls: TlsMode::Verify, + tls: TlsMode::Verify, }) ); } diff --git a/lib/foundation/fabro-config/src/resolve/run.rs b/lib/foundation/fabro-config/src/resolve/run.rs index d2cbda4d5..2fe78b32e 100644 --- a/lib/foundation/fabro-config/src/resolve/run.rs +++ b/lib/foundation/fabro-config/src/resolve/run.rs @@ -595,7 +595,6 @@ fn resolve_hook_type(hook: &HookEntry) -> Option { return Some(HookType::Http { url: url.clone(), headers, - allowed_env_vars: hook.allowed_env_vars.clone(), tls, }); } diff --git a/lib/foundation/fabro-config/src/run.rs b/lib/foundation/fabro-config/src/run.rs index d6114b835..3adee2f0a 100644 --- a/lib/foundation/fabro-config/src/run.rs +++ b/lib/foundation/fabro-config/src/run.rs @@ -11,8 +11,8 @@ use std::path::{Path, PathBuf}; -use fabro_types::settings::InterpString; use fabro_types::settings::run::{ResolvedGoalSource, ResolvedRunGoal, RunGoal, RunNamespace}; +use fabro_types::settings::{InterpString, ResolveCtx, ResolveError}; use crate::load::{load_settings_path, resolve_goal_file_path}; use crate::parse::{SettingsSource, validate_settings_source}; @@ -60,8 +60,8 @@ pub fn resolve_graph_path(workflow_toml: &Path, graph_relative: &str) -> PathBuf #[derive(Debug)] pub enum ResolveRunGoalError { - EnvLookup { - var: String, + Interpolation { + source: ResolveError, }, Io { path: PathBuf, @@ -72,10 +72,9 @@ pub enum ResolveRunGoalError { impl std::fmt::Display for ResolveRunGoalError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::EnvLookup { var } => write!( - f, - "run.goal.file references env var `{var}` which is not set" - ), + Self::Interpolation { source } => { + write!(f, "run.goal.file interpolation failed: {source}") + } Self::Io { path, source } => { write!(f, "failed to read goal file {}: {source}", path.display()) } @@ -86,7 +85,7 @@ impl std::fmt::Display for ResolveRunGoalError { impl std::error::Error for ResolveRunGoalError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { - Self::EnvLookup { .. } => None, + Self::Interpolation { source } => Some(source), Self::Io { source, .. } => Some(source), } } @@ -118,9 +117,11 @@ fn resolve_goal_file( file: &InterpString, base_dir: &Path, ) -> std::result::Result { + // `{{ vars.* }}` is substituted server-side at run creation, so the path is + // literal by this point; anything left unresolved fails closed. let resolved = file - .resolve(process_env_var) - .map_err(|err| ResolveRunGoalError::EnvLookup { var: err.name })?; + .resolve_with(&mut ResolveCtx::new()) + .map_err(|source| ResolveRunGoalError::Interpolation { source })?; let path = resolve_goal_file_path(&resolved, base_dir); let text = std::fs::read_to_string(&path).map_err(|source| ResolveRunGoalError::Io { path: path.clone(), @@ -132,14 +133,6 @@ fn resolve_goal_file( }) } -#[expect( - clippy::disallowed_methods, - reason = "Run config interpolation owns a process-env lookup facade for {{ env.* }} values." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - #[expect( clippy::disallowed_methods, reason = "goal text intentionally passes through in source form; goals become importable \ diff --git a/lib/foundation/fabro-types/src/settings/interp.rs b/lib/foundation/fabro-types/src/settings/interp.rs index e31c3e8db..8bb1c4a06 100644 --- a/lib/foundation/fabro-types/src/settings/interp.rs +++ b/lib/foundation/fabro-types/src/settings/interp.rs @@ -1,21 +1,25 @@ //! Interpolation for config strings. //! //! An [`InterpString`] field may contain narrow `{{ .NAME }}` -//! tokens — no template logic. Three [`Namespace`]s resolve here: `env`, -//! `vars`, and `secrets`. `inputs` is **template-only**: it is a -//! recognized namespace so an `{{ inputs.* }}` token fails loudly with a clear -//! message instead of passing through as literal text, but it never resolves -//! in an `InterpString` field — it belongs in prompts and goals. Which of the -//! resolvable namespaces actually apply is scope-determined by the caller -//! through [`ResolveCtx`]: server-scope settings provide `env` (and eventually -//! `secrets`), run-scope settings additionally provide `vars`. A token whose -//! namespace is not available in the resolution context fails loudly. +//! tokens — no template logic. Which [`Namespace`]s resolve is +//! scope-determined by the caller through [`ResolveCtx`]: run-scope settings +//! provide `vars` and `secrets`, a command node `script` provides `inputs`, +//! `vars`, and `goal`. A token whose namespace has no lookup in the resolution +//! context fails loudly rather than passing through as literal text. //! -//! Resolution timing is split: `vars` substitutes early (server-side, at run -//! creation) via [`InterpString::substitute_with`], while `env`/`secrets` -//! resolve late, at consumption time in the process that owns -//! the value, via [`InterpString::resolve_with`]. Resolved secret values are -//! plain strings; sensitivity is not tracked. Redaction of run output is +//! Two namespaces parse but never resolve. `inputs` and `goal` are bound only +//! where a run's values are in scope, which is the workflow graph rather than +//! general config. `env` resolves nowhere at all: the process environment is +//! not a configuration source, and `{{ vars.NAME }}` (non-sensitive, stored on +//! the server) or `{{ secrets.NAME }}` (vault-backed) replaces it. Keeping +//! them parseable is what lets an out-of-scope token fail with a message that +//! names the alternative instead of reaching a consumer as literal text. +//! +//! Resolution timing is split: `vars` and `inputs` substitute early +//! (server-side, at run creation) via [`InterpString::substitute_with`], while +//! `secrets` resolves late, at consumption time in the process that owns the +//! value, via [`InterpString::resolve_with`]. Resolved secret values are plain +//! strings; sensitivity is not tracked. Redaction of run output is //! content-based (entropy analysis plus credential patterns), applied where //! output is serialized. @@ -49,7 +53,8 @@ enum Segment { )] #[strum(serialize_all = "lowercase")] pub enum Namespace { - /// `{{ env.NAME }}` — process environment, resolved at consumption time. + /// `{{ env.NAME }}` — the process environment. Parses so the token fails + /// loudly, but resolves nowhere: use `vars` or `secrets` instead. Env, /// `{{ vars.NAME }}` — non-sensitive run variables, substituted early. Vars, @@ -115,7 +120,6 @@ impl Namespace { /// [`InterpString::substitute_with`]. #[derive(Default)] pub struct ResolveCtx<'a> { - env: Option>, vars: Option>, secrets: Option>, } @@ -128,12 +132,6 @@ impl<'a> ResolveCtx<'a> { Self::default() } - #[must_use] - pub fn with_env(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { - self.env = Some(Box::new(lookup)); - self - } - #[must_use] pub fn with_vars(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { self.vars = Some(Box::new(lookup)); @@ -148,14 +146,15 @@ impl<'a> ResolveCtx<'a> { fn lookup_for(&mut self, namespace: Namespace) -> Option<&mut LookupFn<'a>> { match namespace { - Namespace::Env => self.env.as_mut(), Namespace::Vars => self.vars.as_mut(), Namespace::Secrets => self.secrets.as_mut(), - // `inputs` is template-only: an `InterpString` resolve context - // never provides it, so an `{{ inputs.* }}` token is always - // unavailable here. `substitute_with` still preserves the token so a - // goal (an `InterpString` that feeds a template) can forward it. - Namespace::Inputs => None, + // Neither is ever wired. `env` has no lookup because the process + // environment is not a configuration source; `inputs` is + // template-only. Both variants exist so the token fails with a + // message naming where the value belongs, and `substitute_with` + // still preserves them so a goal (an `InterpString` that feeds a + // template) can forward `{{ inputs.* }}` to the template layer. + Namespace::Env | Namespace::Inputs => None, } } } @@ -334,35 +333,6 @@ impl InterpString { Ok(Self { segments }) } - /// Resolve in an env-only context, e.g. server-scope settings. - /// - /// `lookup` should return the current value for a given env var name (or - /// `None` if unset). Tokens in any other namespace fail with - /// [`ResolveErrorKind::Unavailable`]. - pub fn resolve(&self, lookup: F) -> Result - where - F: FnMut(&str) -> Option, - { - self.resolve_with(&mut ResolveCtx::new().with_env(lookup)) - } - - /// Resolve in an env-only context, falling back to the raw template - /// source when resolution fails so a missing env var surfaces as a - /// recognizable diagnostic instead of a silently dropped value. - #[expect( - clippy::disallowed_methods, - reason = "intentional raw-source fallback so a missing env var surfaces as a \ - recognizable diagnostic; slated for hard-error semantics in the \ - interpolation cleanup" - )] - #[must_use] - pub fn resolve_or_source(&self, lookup: F) -> String - where - F: FnMut(&str) -> Option, - { - self.resolve(lookup).unwrap_or_else(|_| self.as_source()) - } - /// Substitute only `{{ vars.* }}` tokens while preserving all other /// namespaces for their consumption-time resolution. pub fn substitute_variables(&self, lookup: F) -> Result @@ -471,6 +441,16 @@ impl fmt::Display for ResolveError { config fields", self.name ), + // `env` resolves nowhere. Name the replacement rather than + // reporting a generic out-of-scope error. + Namespace::Env => write!( + f, + "{{{{ env.{} }}}} is not supported: the process environment is not a \ + configuration source. Use {{{{ vars.{} }}}} for a non-sensitive value \ + (`fabro variable set`) or {{{{ secrets.{} }}}} for a credential \ + (`fabro secret set`)", + self.name, self.name, self.name + ), _ => write!( f, "{noun} {:?} referenced by {{{{ {namespace}.{} }}}} is not supported in \ @@ -567,56 +547,67 @@ mod tests { assert_eq!(s.names(Namespace::Env), vec!["USER", "HOST", "PORT"]); } + fn resolve_vars(s: &InterpString, pairs: &[(&str, &str)]) -> Result { + s.resolve_with(&mut ResolveCtx::new().with_vars(lookup_from(pairs))) + } + #[test] fn resolve_literal_string() { let s = InterpString::parse("static"); - let resolved = s.resolve(lookup_from(&[])).unwrap(); - assert_eq!(resolved, "static"); + assert_eq!(resolve_vars(&s, &[]).unwrap(), "static"); } #[test] fn resolve_whole_value() { - let s = InterpString::parse("{{ env.API_KEY }}"); - let resolved = s - .resolve(lookup_from(&[("API_KEY", "secret-123")])) - .unwrap(); - assert_eq!(resolved, "secret-123"); + let s = InterpString::parse("{{ vars.API_KEY }}"); + assert_eq!( + resolve_vars(&s, &[("API_KEY", "secret-123")]).unwrap(), + "secret-123" + ); } #[test] fn resolve_substring() { - let s = InterpString::parse("Bearer {{ env.TOKEN }}"); - let resolved = s.resolve(lookup_from(&[("TOKEN", "abc")])).unwrap(); - assert_eq!(resolved, "Bearer abc"); + let s = InterpString::parse("Bearer {{ vars.TOKEN }}"); + assert_eq!(resolve_vars(&s, &[("TOKEN", "abc")]).unwrap(), "Bearer abc"); } #[test] fn resolve_multiple_tokens() { - let s = InterpString::parse("{{ env.USER }}@{{ env.HOST }}"); - let resolved = s - .resolve(lookup_from(&[("USER", "root"), ("HOST", "example.com")])) - .unwrap(); - assert_eq!(resolved, "root@example.com"); + let s = InterpString::parse("{{ vars.USER }}@{{ vars.HOST }}"); + assert_eq!( + resolve_vars(&s, &[("USER", "root"), ("HOST", "example.com")]).unwrap(), + "root@example.com" + ); } #[test] - fn resolve_missing_env_fails_with_name() { - let s = InterpString::parse("{{ env.MISSING }}"); - let err = s.resolve(lookup_from(&[])).unwrap_err(); + fn resolve_missing_var_fails_with_name() { + let s = InterpString::parse("{{ vars.MISSING }}"); + let err = resolve_vars(&s, &[]).unwrap_err(); assert_eq!(err.name, "MISSING"); - assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.namespace, Namespace::Vars); assert_eq!(err.kind, ResolveErrorKind::Missing); - assert_eq!( - err.to_string(), - "environment variable \"MISSING\" referenced by {{ env.MISSING }} is not set" - ); + } + + /// `env` still parses so the token fails loudly, but it resolves nowhere + /// and the message names its replacements. + #[test] + fn env_token_parses_but_never_resolves() { + let s = InterpString::parse("{{ env.API_KEY }}"); + let err = resolve_vars(&s, &[("API_KEY", "ignored")]).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + let message = err.to_string(); + assert!(message.contains("vars.API_KEY"), "{message}"); + assert!(message.contains("secrets.API_KEY"), "{message}"); } #[test] fn unterminated_token_treated_as_literal() { let s = InterpString::parse("{{ env.OPEN"); - let resolved = s.resolve(lookup_from(&[])).unwrap(); - assert_eq!(resolved, "{{ env.OPEN"); + assert_eq!(resolve_vars(&s, &[]).unwrap(), "{{ env.OPEN"); } #[test] @@ -631,8 +622,7 @@ mod tests { ] { let s = InterpString::parse(raw); assert!(s.is_literal(), "{raw} should stay literal"); - let resolved = s.resolve(lookup_from(&[])).unwrap(); - assert_eq!(resolved, raw); + assert_eq!(resolve_vars(&s, &[]).unwrap(), raw); } } @@ -672,14 +662,14 @@ mod tests { } #[test] - fn resolve_with_substitutes_env_and_var_tokens() { - let s = InterpString::parse("https://{{ env.REGION }}.{{ vars.DOMAIN }}"); + fn resolve_with_substitutes_secret_and_var_tokens() { + let s = InterpString::parse("https://{{ vars.REGION }}.{{ secrets.DOMAIN }}"); let resolved = s .resolve_with( &mut ResolveCtx::new() - .with_env(lookup_from(&[("REGION", "us-east-1")])) - .with_vars(lookup_from(&[("DOMAIN", "example.com")])), + .with_vars(lookup_from(&[("REGION", "us-east-1")])) + .with_secrets(lookup_from(&[("DOMAIN", "example.com")])), ) .unwrap(); @@ -691,11 +681,7 @@ mod tests { let s = InterpString::parse("{{ vars.MISSING }}"); let err = s - .resolve_with( - &mut ResolveCtx::new() - .with_env(lookup_from(&[])) - .with_vars(lookup_from(&[])), - ) + .resolve_with(&mut ResolveCtx::new().with_vars(lookup_from(&[]))) .unwrap_err(); assert_eq!(err.name, "MISSING"); @@ -708,10 +694,10 @@ mod tests { } #[test] - fn env_only_resolution_rejects_vars_reference() { + fn empty_context_rejects_vars_reference() { let s = InterpString::parse("{{ vars.RUNTIME_TOKEN }}"); - let err = s.resolve(lookup_from(&[])).unwrap_err(); + let err = s.resolve_with(&mut ResolveCtx::new()).unwrap_err(); assert_eq!(err.name, "RUNTIME_TOKEN"); assert_eq!(err.namespace, Namespace::Vars); @@ -724,10 +710,10 @@ mod tests { } #[test] - fn env_only_resolution_rejects_secrets_reference() { + fn empty_context_rejects_secrets_reference() { let s = InterpString::parse("{{ secrets.API_KEY }}"); - let err = s.resolve(lookup_from(&[])).unwrap_err(); + let err = s.resolve_with(&mut ResolveCtx::new()).unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.kind, ResolveErrorKind::Unavailable); @@ -739,13 +725,13 @@ mod tests { } #[test] - fn resolve_with_substitutes_secrets_and_env() { - let s = InterpString::parse("Bearer {{ secrets.API_KEY }} via {{ env.PROXY }}"); + fn resolve_with_substitutes_secrets_and_vars() { + let s = InterpString::parse("Bearer {{ secrets.API_KEY }} via {{ vars.PROXY }}"); let resolved = s .resolve_with( &mut ResolveCtx::new() - .with_env(lookup_from(&[("PROXY", "proxy.internal")])) + .with_vars(lookup_from(&[("PROXY", "proxy.internal")])) .with_secrets(lookup_from(&[("API_KEY", "vault-value")])), ) .unwrap(); diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index a582ddee9..e7c5a95df 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -422,13 +422,12 @@ mod run_namespace_variable_substitution_tests { event: HookEvent::RunComplete, command: None, hook_type: Some(HookType::Http { - url: InterpString::parse("https://hooks.example/{{ vars.ENV }}"), - headers: Some(HashMap::from([( + url: InterpString::parse("https://hooks.example/{{ vars.ENV }}"), + headers: Some(HashMap::from([( "X-Env".to_string(), InterpString::parse("{{ vars.ENV }}"), )])), - allowed_env_vars: Vec::new(), - tls: super::TlsMode::Verify, + tls: super::TlsMode::Verify, }), matcher: None, blocking: None, @@ -617,26 +616,22 @@ impl RunIntegrationsGithubSettings { !self.permissions.is_empty() } - /// Resolve every `permissions` value's `{{ env.* }}` tokens via - /// `lookup`, falling back to the raw template source when resolution - /// fails so callers see a recognizable diagnostic instead of a - /// silently dropped key. The `lookup` seam keeps tests free of - /// process-env coupling; production callers pass a thin wrapper over - /// `std::env::var`. - pub fn resolve_permissions(&self, mut lookup: F) -> HashMap - where - F: FnMut(&str) -> Option, - { + /// Resolve every `permissions` value. `{{ vars.* }}` is substituted + /// server-side at run creation, so values are literal by this point; a + /// still-unresolved token fails closed rather than reaching the GitHub API + /// as literal text. + pub fn resolve_permissions(&self) -> Result, ResolveError> { + let mut ctx = ResolveCtx::new(); self.permissions .iter() - .map(|(name, value)| (name.clone(), value.resolve_or_source(&mut lookup))) + .map(|(name, value)| Ok((name.clone(), value.resolve_with(&mut ctx)?))) .collect() } } #[cfg(test)] mod run_integrations_github_tests { - use super::{HashMap, InterpString, RunIntegrationsGithubSettings}; + use super::{InterpString, Namespace, RunIntegrationsGithubSettings}; fn settings(permissions: &[(&str, &str)]) -> RunIntegrationsGithubSettings { RunIntegrationsGithubSettings { @@ -654,30 +649,26 @@ mod run_integrations_github_tests { } #[test] - fn resolve_permissions_substitutes_env_tokens_via_lookup() { - let s = settings(&[("issues", "{{ env.GH_PERM_LEVEL }}"), ("contents", "read")]); - let resolved = s.resolve_permissions(|name| match name { - "GH_PERM_LEVEL" => Some("write".to_string()), - _ => None, - }); + fn resolve_permissions_passes_through_literal_values() { + let s = settings(&[("issues", "write"), ("contents", "read")]); + let resolved = s.resolve_permissions().unwrap(); assert_eq!(resolved.get("issues"), Some(&"write".to_string())); assert_eq!(resolved.get("contents"), Some(&"read".to_string())); } + /// `{{ vars.* }}` is substituted at run creation, so a token still present + /// here can never resolve and must fail rather than reach the GitHub API + /// as literal text. #[test] - fn resolve_permissions_falls_back_to_source_when_lookup_fails() { - let s = settings(&[("issues", "{{ env.GH_PERM_MISSING }}")]); - let resolved = s.resolve_permissions(|_| None); - assert_eq!( - resolved.get("issues"), - Some(&"{{ env.GH_PERM_MISSING }}".to_string()) - ); + fn resolve_permissions_fails_on_an_unresolved_token() { + let s = settings(&[("issues", "{{ env.GH_PERM_LEVEL }}")]); + let err = s.resolve_permissions().unwrap_err(); + assert_eq!(err.namespace, Namespace::Env); } #[test] fn resolve_permissions_is_empty_for_empty_settings() { - let s: HashMap = settings(&[]).resolve_permissions(|_| None); - assert!(s.is_empty()); + assert!(settings(&[]).resolve_permissions().unwrap().is_empty()); } } @@ -764,17 +755,16 @@ impl RunPrepareSettings { /// A referenced env var or secret that is unset is a hard error — no /// fallback to the unresolved source. Reserved `inputs` tokens have no /// lookup here and surface as a loud - /// [`super::interp::ResolveErrorKind::Unavailable`] error rather than + /// [`ResolveErrorKind::Unavailable`] error rather than /// passing through as literal text. pub fn resolve_step_env( &self, - mut env_lookup: impl FnMut(&str) -> Option, mut secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { let mut resolved = self.clone(); for step in &mut resolved.steps { visit_prepared_step_strings(step, &mut |value| { - resolve_env_string(value, &mut env_lookup, &mut secrets_lookup) + resolve_env_string(value, &mut secrets_lookup) })?; } Ok(resolved) @@ -1094,35 +1084,17 @@ impl RunEnvironmentSettings { } } - /// Resolve every environment value's `{{ env.* }}` and `{{ secrets.* }}` - /// tokens via the supplied lookups. Missing env vars retain the historical - /// fallback to the original source string for env-only values; values that - /// reference secrets fail closed instead of preserving a secret token. + /// Resolve every environment value's `{{ secrets.* }}` tokens via + /// `secrets_lookup`. `{{ vars.* }}` is already substituted server-side at + /// run creation, so anything still unresolved here fails closed. pub fn resolve_env( &self, - mut env_lookup: impl FnMut(&str) -> Option, mut secrets_lookup: impl FnMut(&str) -> Option, ) -> Result, ResolveError> { - let mut ctx = ResolveCtx::new() - .with_env(&mut env_lookup) - .with_secrets(&mut secrets_lookup); + let mut ctx = ResolveCtx::new().with_secrets(&mut secrets_lookup); let mut resolved = HashMap::with_capacity(self.env.len()); for (name, value) in &self.env { - let references_secrets = value.references(Namespace::Secrets); - let resolved_value = match value.resolve_with(&mut ctx) { - Ok(resolved) => resolved, - Err(err) if err.namespace == Namespace::Env && !references_secrets => { - #[expect( - clippy::disallowed_methods, - reason = "intentional raw-source fallback preserves existing \ - environment variable behavior for env-only run environment values" - )] - let source = value.as_source(); - source - } - Err(err) => return Err(err), - }; - resolved.insert(name.clone(), resolved_value); + resolved.insert(name.clone(), value.resolve_with(&mut ctx)?); } Ok(resolved) } @@ -1151,6 +1123,7 @@ fn pair_lookup( #[cfg(test)] mod run_environment_settings_tests { use super::{HashMap, InterpString, RunEnvironmentSettings, pair_lookup as lookup}; + use crate::settings::ResolveErrorKind; fn settings(env: &[(&str, &str)]) -> RunEnvironmentSettings { RunEnvironmentSettings { @@ -1163,33 +1136,20 @@ mod run_environment_settings_tests { } #[test] - fn resolve_env_substitutes_env_tokens_via_lookup() { - let s = settings(&[("NODE_ENV", "{{ env.NODE_ENV }}"), ("STATIC", "value")]); - let resolved = s - .resolve_env(lookup(&[("NODE_ENV", "test")]), lookup(&[])) - .unwrap(); + fn resolve_env_passes_through_literal_values() { + let s = settings(&[("NODE_ENV", "production"), ("STATIC", "value")]); + let resolved = s.resolve_env(lookup(&[])).unwrap(); - assert_eq!(resolved.get("NODE_ENV"), Some(&"test".to_string())); + assert_eq!(resolved.get("NODE_ENV"), Some(&"production".to_string())); assert_eq!(resolved.get("STATIC"), Some(&"value".to_string())); } - #[test] - fn resolve_env_falls_back_to_source_when_lookup_fails() { - let s = settings(&[("NODE_ENV", "{{ env.MISSING_NODE_ENV }}")]); - let resolved = s.resolve_env(lookup(&[]), lookup(&[])).unwrap(); - - assert_eq!( - resolved.get("NODE_ENV"), - Some(&"{{ env.MISSING_NODE_ENV }}".to_string()) - ); - } - #[test] fn resolve_env_substitutes_secret_tokens_via_lookup() { let s = settings(&[("API_TOKEN", "Bearer {{ secrets.API_TOKEN }}")]); let resolved = s - .resolve_env(lookup(&[]), lookup(&[("API_TOKEN", "vault-token")])) + .resolve_env(lookup(&[("API_TOKEN", "vault-token")])) .unwrap(); assert_eq!( @@ -1202,31 +1162,28 @@ mod run_environment_settings_tests { fn resolve_env_returns_secret_error_without_source_fallback() { let s = settings(&[("API_TOKEN", "{{ secrets.MISSING_TOKEN }}")]); - let err = s.resolve_env(lookup(&[]), lookup(&[])).unwrap_err(); + let err = s.resolve_env(lookup(&[])).unwrap_err(); assert_eq!(err.namespace, super::Namespace::Secrets); assert_eq!(err.name, "MISSING_TOKEN"); } + /// `{{ env.* }}` no longer resolves anywhere. It fails closed rather than + /// falling back to source form, which previously let an unresolved token + /// reach the sandbox as literal text. #[test] - fn resolve_env_does_not_source_fallback_mixed_values_that_reference_secrets() { - let s = settings(&[( - "API_TOKEN", - "{{ env.MISSING_PREFIX }} {{ secrets.API_TOKEN }}", - )]); + fn resolve_env_fails_closed_on_an_env_token() { + let s = settings(&[("NODE_ENV", "{{ env.NODE_ENV }}")]); - let err = s - .resolve_env(lookup(&[]), lookup(&[("API_TOKEN", "vault-token")])) - .unwrap_err(); + let err = s.resolve_env(lookup(&[])).unwrap_err(); assert_eq!(err.namespace, super::Namespace::Env); - assert_eq!(err.name, "MISSING_PREFIX"); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); } #[test] fn resolve_env_is_empty_for_empty_settings() { - let s: HashMap = - settings(&[]).resolve_env(lookup(&[]), lookup(&[])).unwrap(); + let s: HashMap = settings(&[]).resolve_env(lookup(&[])).unwrap(); assert!(s.is_empty()); } } @@ -1630,30 +1587,26 @@ impl McpServerSettings { /// error rather than passing through as literal text. pub fn resolve_transport_env( &self, - mut env_lookup: impl FnMut(&str) -> Option, mut secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { let mut resolved = self.clone(); visit_mcp_transport_strings(&mut resolved.transport, &mut |value| { - resolve_env_string(value, &mut env_lookup, &mut secrets_lookup) + resolve_env_string(value, &mut secrets_lookup) })?; Ok(resolved) } } -/// Resolve `{{ env.* }}` and `{{ secrets.* }}` tokens in one run-boundary +/// Resolve `{{ secrets.* }}` tokens in one run-boundary /// string. A literal value (no tokens) round-trips unchanged. fn resolve_env_string( value: &mut String, - env_lookup: &mut impl FnMut(&str) -> Option, secrets_lookup: &mut impl FnMut(&str) -> Option, ) -> Result<(), ResolveError> { if !value.contains("{{") { return Ok(()); } - let mut ctx = ResolveCtx::new() - .with_env(&mut *env_lookup) - .with_secrets(&mut *secrets_lookup); + let mut ctx = ResolveCtx::new().with_secrets(&mut *secrets_lookup); *value = InterpString::parse(value).resolve_with(&mut ctx)?; Ok(()) } @@ -1664,8 +1617,7 @@ mod resolve_transport_env_tests { use super::super::interp::ResolveErrorKind; use super::{ - McpHttpProtocol, McpServerSettings, McpTransport, Namespace, pair_lookup as env_lookup, - pair_lookup as secret_lookup, + McpHttpProtocol, McpServerSettings, McpTransport, Namespace, pair_lookup as secret_lookup, }; #[test] @@ -1679,9 +1631,7 @@ mod resolve_transport_env_tests { ..McpServerSettings::default() }; - let resolved = settings - .resolve_transport_env(env_lookup(&[]), secret_lookup(&[])) - .unwrap(); + let resolved = settings.resolve_transport_env(secret_lookup(&[])).unwrap(); let McpTransport::Stdio { command, env } = resolved.transport else { panic!("expected stdio transport"); @@ -1690,75 +1640,6 @@ mod resolve_transport_env_tests { assert_eq!(env.get("TOKEN").map(String::as_str), Some("literal-value")); } - #[test] - fn stdio_command_and_env_resolve() { - let settings = McpServerSettings { - name: "gemini".to_string(), - transport: McpTransport::Stdio { - command: vec!["python".to_string(), "{{ env.SERVER_PATH }}".to_string()], - env: HashMap::from([( - "GEMINI_API_KEY".to_string(), - "{{ env.GEMINI_API_KEY }}".to_string(), - )]), - }, - ..McpServerSettings::default() - }; - - let resolved = settings - .resolve_transport_env( - env_lookup(&[ - ("SERVER_PATH", "/srv/mcp.py"), - ("GEMINI_API_KEY", "real-key"), - ]), - secret_lookup(&[]), - ) - .unwrap(); - - let McpTransport::Stdio { command, env } = resolved.transport else { - panic!("expected stdio transport"); - }; - assert_eq!(command, vec![ - "python".to_string(), - "/srv/mcp.py".to_string() - ]); - assert_eq!( - env.get("GEMINI_API_KEY").map(String::as_str), - Some("real-key") - ); - } - - #[test] - fn http_url_and_headers_resolve() { - let settings = McpServerSettings { - name: "remote".to_string(), - transport: McpTransport::Http { - protocol: McpHttpProtocol::default(), - url: "https://{{ env.MCP_HOST }}/mcp".to_string(), - headers: HashMap::from([( - "Authorization".to_string(), - "Bearer {{ env.MCP_TOKEN }}".to_string(), - )]), - }, - ..McpServerSettings::default() - }; - - let resolved = settings - .resolve_transport_env( - env_lookup(&[("MCP_HOST", "mcp.example"), ("MCP_TOKEN", "abc123")]), - secret_lookup(&[]), - ) - .unwrap(); - - let McpTransport::Http { url, headers, .. } = resolved.transport else { - panic!("expected http transport"); - }; - assert_eq!(url, "https://mcp.example/mcp"); - assert_eq!( - headers.get("Authorization").map(String::as_str), - Some("Bearer abc123") - ); - } - #[test] fn missing_env_is_hard_error() { let settings = McpServerSettings { @@ -1767,17 +1648,17 @@ mod resolve_transport_env_tests { command: vec!["python".to_string()], env: HashMap::from([( "GEMINI_API_KEY".to_string(), - "{{ env.GEMINI_API_KEY }}".to_string(), + "{{ secrets.GEMINI_API_KEY }}".to_string(), )]), }, ..McpServerSettings::default() }; let err = settings - .resolve_transport_env(env_lookup(&[]), secret_lookup(&[])) + .resolve_transport_env(secret_lookup(&[])) .unwrap_err(); - assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "GEMINI_API_KEY"); assert_eq!(err.kind, ResolveErrorKind::Missing); } @@ -1801,10 +1682,10 @@ mod resolve_transport_env_tests { }; let resolved = settings - .resolve_transport_env( - env_lookup(&[]), - secret_lookup(&[("SERVER_BIN", "/srv/mcp"), ("API_TOKEN", "vault-token")]), - ) + .resolve_transport_env(secret_lookup(&[ + ("SERVER_BIN", "/srv/mcp"), + ("API_TOKEN", "vault-token"), + ])) .unwrap(); let McpTransport::Stdio { command, env } = resolved.transport else { @@ -1837,10 +1718,10 @@ mod resolve_transport_env_tests { }; let resolved = settings - .resolve_transport_env( - env_lookup(&[]), - secret_lookup(&[("MCP_HOST", "mcp.example"), ("MCP_TOKEN", "vault-token")]), - ) + .resolve_transport_env(secret_lookup(&[ + ("MCP_HOST", "mcp.example"), + ("MCP_TOKEN", "vault-token"), + ])) .unwrap(); let McpTransport::Http { url, headers, .. } = resolved.transport else { @@ -1868,7 +1749,7 @@ mod resolve_transport_env_tests { }; let err = settings - .resolve_transport_env(env_lookup(&[]), secret_lookup(&[])) + .resolve_transport_env(secret_lookup(&[])) .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); @@ -1883,8 +1764,7 @@ mod resolve_step_env_tests { use super::super::interp::ResolveErrorKind; use super::{ - Namespace, PreparedStep, PreparedStepRun, RunPrepareSettings, pair_lookup as env_lookup, - pair_lookup as secret_lookup, + Namespace, PreparedStep, PreparedStepRun, RunPrepareSettings, pair_lookup as secret_lookup, }; fn script_step(script: &str, env: HashMap) -> PreparedStep { @@ -1943,9 +1823,7 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let resolved = settings - .resolve_step_env(env_lookup(&[]), secret_lookup(&[])) - .unwrap(); + let resolved = settings.resolve_step_env(secret_lookup(&[])).unwrap(); assert_eq!(resolved.steps[0].to_shell_command(), "echo hello"); assert_eq!( @@ -1956,19 +1834,18 @@ mod resolve_step_env_tests { #[test] fn script_resolves_verbatim() { - // A script is a raw shell snippet: its `{{ env.* }}` token resolves but - // the result is NOT shell-quoted — the shell interprets the snippet as - // written. + // A script is a raw shell snippet: its token resolves but the result is + // NOT shell-quoted — the shell interprets the snippet as written. let settings = RunPrepareSettings { steps: vec![script_step( - "deploy {{ env.REGION }} && echo done", + "deploy {{ secrets.REGION }} && echo done", HashMap::new(), )], timeout_ms: 1_000, }; let resolved = settings - .resolve_step_env(env_lookup(&[("REGION", "us-east-1")]), secret_lookup(&[])) + .resolve_step_env(secret_lookup(&[("REGION", "us-east-1")])) .unwrap(); assert_eq!( @@ -1978,20 +1855,23 @@ mod resolve_step_env_tests { } #[test] - fn command_and_env_resolve() { + fn command_and_env_resolve_secret_tokens() { let settings = RunPrepareSettings { steps: vec![command_step( - &["deploy", "{{ env.REGION }}"], - HashMap::from([("TOKEN".to_string(), "{{ env.DEPLOY_TOKEN }}".to_string())]), + &["deploy", "{{ secrets.REGION }}"], + HashMap::from([( + "TOKEN".to_string(), + "{{ secrets.DEPLOY_TOKEN }}".to_string(), + )]), )], timeout_ms: 1_000, }; let resolved = settings - .resolve_step_env( - env_lookup(&[("REGION", "us-east-1"), ("DEPLOY_TOKEN", "secret-token")]), - secret_lookup(&[]), - ) + .resolve_step_env(secret_lookup(&[ + ("REGION", "us-east-1"), + ("DEPLOY_TOKEN", "secret-token"), + ])) .unwrap(); assert_eq!(resolved.steps[0].to_shell_command(), "deploy us-east-1"); @@ -2006,15 +1886,15 @@ mod resolve_step_env_tests { // A resolved argv element that contains a space must survive as a // single shell word, not re-split into two. let settings = RunPrepareSettings { - steps: vec![command_step(&["echo", "{{ env.MESSAGE }}"], HashMap::new())], + steps: vec![command_step( + &["echo", "{{ secrets.MESSAGE }}"], + HashMap::new(), + )], timeout_ms: 1_000, }; let resolved = settings - .resolve_step_env( - env_lookup(&[("MESSAGE", "hello world")]), - secret_lookup(&[]), - ) + .resolve_step_env(secret_lookup(&[("MESSAGE", "hello world")])) .unwrap(); let shell = resolved.steps[0].to_shell_command(); @@ -2032,17 +1912,14 @@ mod resolve_step_env_tests { let malicious = "x'; touch PWNED; echo '"; let settings = RunPrepareSettings { steps: vec![command_step( - &["echo", "{{ env.USER_INPUT }}"], + &["echo", "{{ secrets.USER_INPUT }}"], HashMap::new(), )], timeout_ms: 1_000, }; let resolved = settings - .resolve_step_env( - |name| (name == "USER_INPUT").then(|| malicious.to_string()), - secret_lookup(&[]), - ) + .resolve_step_env(|name| (name == "USER_INPUT").then(|| malicious.to_string())) .unwrap(); let shell = resolved.steps[0].to_shell_command(); @@ -2063,39 +1940,38 @@ mod resolve_step_env_tests { } #[test] - fn missing_env_in_command_is_hard_error() { + fn missing_secret_in_command_is_hard_error() { let settings = RunPrepareSettings { steps: vec![command_step( - &["deploy", "{{ env.REGION }}"], + &["deploy", "{{ secrets.REGION }}"], HashMap::new(), )], timeout_ms: 1_000, }; - let err = settings - .resolve_step_env(env_lookup(&[]), secret_lookup(&[])) - .unwrap_err(); + let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); - assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "REGION"); assert_eq!(err.kind, ResolveErrorKind::Missing); } #[test] - fn missing_env_in_step_env_value_is_hard_error() { + fn missing_secret_in_step_env_value_is_hard_error() { let settings = RunPrepareSettings { steps: vec![script_step( "echo hi", - HashMap::from([("TOKEN".to_string(), "{{ env.DEPLOY_TOKEN }}".to_string())]), + HashMap::from([( + "TOKEN".to_string(), + "{{ secrets.DEPLOY_TOKEN }}".to_string(), + )]), )], timeout_ms: 1_000, }; - let err = settings - .resolve_step_env(env_lookup(&[]), secret_lookup(&[])) - .unwrap_err(); + let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); - assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "DEPLOY_TOKEN"); assert_eq!(err.kind, ResolveErrorKind::Missing); } @@ -2117,14 +1993,11 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env( - env_lookup(&[]), - secret_lookup(&[ - ("REGION", "us-east-1"), - ("DEPLOY_TOKEN", "vault-token"), - ("MESSAGE", "hello world"), - ]), - ) + .resolve_step_env(secret_lookup(&[ + ("REGION", "us-east-1"), + ("DEPLOY_TOKEN", "vault-token"), + ("MESSAGE", "hello world"), + ])) .unwrap(); assert_eq!( @@ -2148,9 +2021,7 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let err = settings - .resolve_step_env(env_lookup(&[]), secret_lookup(&[])) - .unwrap_err(); + let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "API_KEY"); @@ -2232,12 +2103,10 @@ pub enum HookType { command: InterpString, }, Http { - url: InterpString, - headers: Option>, + url: InterpString, + headers: Option>, #[serde(default)] - allowed_env_vars: Vec, - #[serde(default)] - tls: TlsMode, + tls: TlsMode, }, Prompt { prompt: InterpString, diff --git a/lib/packages/fabro-api-client/src/models/hook-definition.ts b/lib/packages/fabro-api-client/src/models/hook-definition.ts index 85092d14f..a8b2100e8 100644 --- a/lib/packages/fabro-api-client/src/models/hook-definition.ts +++ b/lib/packages/fabro-api-client/src/models/hook-definition.ts @@ -27,10 +27,6 @@ export interface HookDefinition { 'type'?: HookDefinitionTypeEnum | null; 'url'?: string | null; 'headers'?: { [key: string]: string; } | null; - /** - * Allowlist of environment variable names that an http hook header may read via `{{ env.NAME }}`. An empty list (the default) permits no env vars in headers. - */ - 'allowed_env_vars'?: Array; 'tls'?: TlsMode; 'prompt'?: string | null; 'model'?: string | null; diff --git a/lib/packages/fabro-api-client/src/models/prepared-step.ts b/lib/packages/fabro-api-client/src/models/prepared-step.ts index 24daf4f94..21e5c57ec 100644 --- a/lib/packages/fabro-api-client/src/models/prepared-step.ts +++ b/lib/packages/fabro-api-client/src/models/prepared-step.ts @@ -22,6 +22,6 @@ import type { PreparedScriptStep } from './prepared-script-step'; /** * @type PreparedStep - * A single resolved prepare step. The runnable part preserves the script-vs-argv distinction via the `type` discriminator: a `script` is a raw shell snippet kept verbatim, while a `command` is an argv whose elements are shell-quoted and joined at the run boundary (after `{{ env.* }}` resolution) so an interpolated value cannot inject shell syntax. Optional per-step `env` is shared by both shapes. + * A single resolved prepare step. The runnable part preserves the script-vs-argv distinction via the `type` discriminator: a `script` is a raw shell snippet kept verbatim, while a `command` is an argv whose elements are shell-quoted and joined at the run boundary (after `{{ secrets.* }}` resolution) so an interpolated value cannot inject shell syntax. Optional per-step `env` is shared by both shapes. */ export type PreparedStep = { type: 'command' } & PreparedCommandStep | { type: 'script' } & PreparedScriptStep; From 6226c8c517e27958af81f3d57489f6ca91e76244 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 17:30:50 -0400 Subject: [PATCH 3/4] fix: address env interpolation review findings Restore the documented SDK env credential facade without reintroducing run fallback behavior. Fail closed on GitHub permission resolution, require worker storage at the CLI boundary, and align interpolation names and generated docs. --- docs/public/agents/mcp.mdx | 2 +- docs/public/api-reference/fabro-api.yaml | 2 +- docs/public/execution/run-configuration.mdx | 4 +- docs/public/reference/user-configuration.mdx | 4 +- lib/apps/fabro-cli/src/args.rs | 2 +- lib/apps/fabro-cli/src/commands/exec.rs | 11 +- lib/apps/fabro-cli/src/commands/run/runner.rs | 27 +-- lib/apps/fabro-cli/src/main.rs | 12 +- lib/apps/fabro-cli/tests/it/cmd/runner.rs | 80 +++----- .../fabro-cli/tests/it/cmd/worker_auth.rs | 22 +-- lib/apps/fabro-server/src/run_manifest.rs | 100 ++++++++-- lib/apps/fabro-server/src/server.rs | 24 ++- .../fabro-agent/tests/it/parity_matrix.rs | 4 +- lib/components/fabro-hooks/src/executor.rs | 15 +- .../fabro-workflow/src/operations/start.rs | 34 ++-- .../fabro-workflow/tests/it/integration.rs | 4 +- lib/foundation/fabro-auth/src/env_source.rs | 174 ++++++++++++++++++ lib/foundation/fabro-auth/src/lib.rs | 2 + lib/foundation/fabro-auth/src/test_support.rs | 23 +-- lib/foundation/fabro-config/src/layers/llm.rs | 7 +- .../fabro-config/src/resolve/run.rs | 39 ++-- .../src/commands/docs_options_reference.rs | 4 +- .../fabro-types/src/settings/interp.rs | 43 ++--- .../fabro-types/src/settings/run.rs | 129 ++++++------- .../src/models/run-goal-file.ts | 2 +- .../src/models/run-goal-inline.ts | 2 +- .../src/models/run-namespace.ts | 2 +- 27 files changed, 478 insertions(+), 296 deletions(-) create mode 100644 lib/foundation/fabro-auth/src/env_source.rs diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index 06c7e645a..1a7008d21 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -151,7 +151,7 @@ Inline transport fields can interpolate values at the run boundary: | `{{ vars.NAME }}` | When the server creates the run, using that run's variable snapshot | | `{{ secrets.NAME }}` | When the worker launches the MCP transport, using a token secret from the server vault | -Interpolation applies to stdio and sandbox commands and env values, plus HTTP URLs and headers. Variable tokens are replaced in the created run configuration. Worker-time environment and secret expressions remain in persisted configuration, while resolved secret values do not. A missing environment variable, missing secret, or non-token secret fails MCP startup instead of passing an unresolved token to the transport. +Interpolation applies to stdio and sandbox commands and env values, plus HTTP URLs and headers. Variable tokens are replaced in the created run configuration. Secret expressions remain in persisted configuration, while resolved secret values do not. A missing or non-token secret fails MCP startup instead of passing an unresolved token to the transport. `{{ env.* }}` is unsupported and also fails before launch. Standalone `fabro exec` has no server vault, so a `{{ secrets.* }}` reference fails with an explicit error in standalone execution. diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 6d4362735..69f91a2c3 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -13961,7 +13961,7 @@ components: $ref: "#/components/schemas/RunNamespace" InterpString: - description: Resolved config string that may contain env interpolation tokens. + description: Config string that can contain typed interpolation tokens. type: string StringMap: diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 207d2df1d..557056d74 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -295,7 +295,7 @@ When `provider = "local"`, Fabro runs directly in the resolved working directory. If you want local isolation, create or enter a separate clone or Git worktree yourself. -Environment variable values can combine literal text with server variables, worker environment variables, and token secrets: +Environment variable values can combine literal text with server variables and token secrets: ```toml title="run.toml" [environments.ci.env] @@ -354,7 +354,7 @@ Each enabled Slack route posts once for each matching lifecycle event. Messages `run.failed` is emitted only when the run terminally fails. A failed stage that routes onward to a normal completion path produces `run.completed`, not `run.failed`. -If a Slack route's channel is missing, empty, or references an unresolved environment variable, Fabro logs a warning and skips that route. Delivery failures are logged and never fail or alter the run. +If a Slack route's channel is missing, empty, or contains an unsupported interpolation token, Fabro logs a warning and skips that route. Delivery failures are logged and never fail or alter the run. ### `[run.checkpoint]` diff --git a/docs/public/reference/user-configuration.mdx b/docs/public/reference/user-configuration.mdx index 1e6021555..4299be1dd 100644 --- a/docs/public/reference/user-configuration.mdx +++ b/docs/public/reference/user-configuration.mdx @@ -184,7 +184,7 @@ aliases = ["gateway"] credentials = ["env:ACME_GATEWAY_API_KEY", "vault:ACME_GATEWAY_API_KEY"] [llm.providers.proxy.extra_headers] -x-portkey-api-key = "{{ secrets.PORTKEY_API_KEY }}" +x-portkey-api-key = "{{ secrets.portkey_api_key }}" x-portkey-config = "@bedrock-prod" x-team-secret = "{{ secrets.gateway_team_secret }}" ``` @@ -199,7 +199,7 @@ x-team-secret = "{{ secrets.gateway_team_secret }}" | `auth` | table | omitted | API-key auth config. Omit the table entirely for providers that need no API key; any `extra_headers` are still attached. | | `auth.credentials` | array | required when `auth` present | Ordered credential refs. Accepted forms are `vault:`, `env:`, and `aws_sigv4` (sign requests from the AWS default credential chain — Bedrock). Literal secret strings are rejected. | | `auth.header` | `"bearer"` or `{ custom = "Header-Name" }` | `"bearer"` | Primary API-key header policy. Omit when the provider uses a standard bearer token. | -| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are interpolation strings: literal text or a `{{ secrets.NAME }}` token. Put credentials in a secret and reference them with a `{{ secrets.NAME }}` token, not a bare literal. | +| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are literal text or `{{ secrets.NAME }}` interpolation strings. Put credentials in a secret and reference them with a token, not a bare literal. | | `priority` | integer | `0` | Higher-priority ready providers win unqualified model and default selection; ties use canonical provider ID. | | `enabled` | boolean | `true` | Set `false` to disable a provider after lower-precedence layers define it. | | `aliases` | array | `[]` | Additional provider names accepted by model routing and fallback config. | diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index f560b62e6..fb20b1158 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -1036,7 +1036,7 @@ pub(crate) struct RunWorkerArgs { /// Fabro storage directory for loading worker-visible secrets #[arg(long, hide = true)] - pub(crate) storage_dir: Option, + pub(crate) storage_dir: PathBuf, /// Run scratch directory #[arg(long)] diff --git a/lib/apps/fabro-cli/src/commands/exec.rs b/lib/apps/fabro-cli/src/commands/exec.rs index 48d729c63..f72df2c22 100644 --- a/lib/apps/fabro-cli/src/commands/exec.rs +++ b/lib/apps/fabro-cli/src/commands/exec.rs @@ -333,17 +333,14 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu .transpose()? .unwrap_or_default(), }; - // Resolve `{{ env.* }}` in MCP transport config at the exec boundary, - // against the CLI process env — the mirror of the `fabro run` worker - // boundary in `fabro_workflow::operations::start::runtime_mcp_server`. - // Both consumers read the same source-form settings; missing env is a hard - // error. `fabro exec` has no server vault, so secrets/inputs tokens surface - // loudly rather than leaking. + // Fully validate MCP transport config at the exec boundary. `fabro exec` + // has no server vault, so secret and unsupported tokens fail instead of + // reaching the transport. let mcp_servers = mcp_servers .into_iter() .map(|settings| { settings - .resolve_transport_env(|_| None) + .resolve_transport_secrets(|_| None) .with_context(|| format!("failed to resolve MCP server {:?}", settings.name)) }) .collect::>>()?; diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index b031d7620..8148d0adc 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -76,7 +76,7 @@ enum WorkerTitlePhase { pub(crate) async fn execute( run_id: RunId, server: String, - storage_dir: Option, + storage_dir: PathBuf, run_dir: PathBuf, mode: RunWorkerMode, worker_token: &str, @@ -137,7 +137,7 @@ 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(), &run_dir).await?; + let vault = load_worker_vault(&storage_dir).await?; let github_app = { let vault_guard = vault.read().await; maybe_build_github_credentials(&run_spec.settings, &vault_guard)? @@ -272,22 +272,9 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { /// 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() - ) - })?; - +/// A worker always receives the server storage root so it can load the same +/// secret vault as the server. +async fn load_worker_vault(storage_dir: &Path) -> Result>> { let storage = Storage::new(storage_dir); let vault = SecretStore::open_snapshot(storage.sqlite_path(), storage.secrets_path()) .await @@ -1750,9 +1737,7 @@ mod tests { .set("ANTHROPIC_API_KEY", "vault-key", SecretType::Token, None) .unwrap(); - let loaded = load_worker_vault(Some(temp.path()), temp.path()) - .await - .unwrap(); + let loaded = load_worker_vault(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/main.rs b/lib/apps/fabro-cli/src/main.rs index 9579f8e79..0d0aa1c79 100644 --- a/lib/apps/fabro-cli/src/main.rs +++ b/lib/apps/fabro-cli/src/main.rs @@ -496,7 +496,7 @@ async fn pre_tracing_bootstrap(command: &Commands) -> Result { - prepare_run_worker_bootstrap(args.storage_dir.as_deref(), &args.run_dir) + prepare_run_worker_bootstrap(&args.storage_dir, &args.run_dir) } _ => Ok(PreTracingBootstrap::cli()), } @@ -541,10 +541,10 @@ async fn prepare_server_bootstrap( } fn prepare_run_worker_bootstrap( - storage_dir: Option<&std::path::Path>, + storage_dir: &std::path::Path, run_dir: &std::path::Path, ) -> Result { - let local_config = local_server::LocalServerConfig::load_with_storage_dir(storage_dir)?; + let local_config = local_server::LocalServerConfig::load_with_storage_dir(Some(storage_dir))?; let runtime_directory = fabro_config::RuntimeDirectory::new(local_config.storage_dir()); let log_destination = fabro_config::resolve_log_destination( local_config.config_log_destination().unwrap_or_default(), @@ -1515,6 +1515,8 @@ destination = "{destination}" "__run-worker", "--server", "/tmp/fabro.sock", + "--storage-dir", + "/tmp/storage", "--run-dir", "/tmp/run", "--run-id", @@ -1526,6 +1528,7 @@ destination = "{destination}" match *cli.command.unwrap() { Commands::RunCmd(RunCommands::RunWorker(args)) => { assert_eq!(args.server, "/tmp/fabro.sock"); + assert_eq!(args.storage_dir, std::path::PathBuf::from("/tmp/storage")); assert_eq!(args.run_dir, std::path::PathBuf::from("/tmp/run")); assert_eq!(args.run_id, "01ARZ3NDEKTSV4RRFFQ69G5FAV".parse().unwrap()); assert!(matches!(args.mode, args::RunWorkerMode::Start)); @@ -1541,6 +1544,8 @@ destination = "{destination}" "__run-worker", "--server", "http://127.0.0.1:3000", + "--storage-dir", + "/tmp/storage", "--run-dir", "/tmp/run", "--run-id", @@ -1552,6 +1557,7 @@ destination = "{destination}" match *cli.command.unwrap() { Commands::RunCmd(RunCommands::RunWorker(args)) => { assert_eq!(args.server, "http://127.0.0.1:3000"); + assert_eq!(args.storage_dir, std::path::PathBuf::from("/tmp/storage")); assert_eq!(args.run_dir, std::path::PathBuf::from("/tmp/run")); assert_eq!(args.run_id, "01ARZ3NDEKTSV4RRFFQ69G5FAV".parse().unwrap()); assert!(matches!(args.mode, args::RunWorkerMode::Resume)); diff --git a/lib/apps/fabro-cli/tests/it/cmd/runner.rs b/lib/apps/fabro-cli/tests/it/cmd/runner.rs index 5bce373f8..1d356f20f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/runner.rs @@ -69,24 +69,17 @@ fn spawn_worker_process( "FABRO_WORKER_TOKEN", issue_test_worker_jwt(&context.storage_dir, 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", - run_dir - .to_str() - .expect("run directory path should be valid UTF-8"), - "--run-id", - run_id, - "--mode", - mode, - ]); + cmd.arg("__run-worker") + .arg("--storage-dir") + .arg(&context.storage_dir) + .arg("--server") + .arg(server) + .arg("--run-dir") + .arg(run_dir) + .arg("--run-id") + .arg(run_id) + .arg("--mode") + .arg(mode); cmd.stdin(Stdio::piped()); cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); @@ -129,8 +122,16 @@ fn child_output(mut child: Child, status: ExitStatus) -> Output { } } -fn worker_command(context: &fabro_test::TestContext, run_id: &str) -> assert_cmd::Command { +fn worker_base_command(context: &fabro_test::TestContext) -> assert_cmd::Command { let mut cmd = context.command(); + cmd.arg("__run-worker") + .arg("--storage-dir") + .arg(&context.storage_dir); + cmd +} + +fn worker_command(context: &fabro_test::TestContext, run_id: &str) -> assert_cmd::Command { + let mut cmd = worker_base_command(context); cmd.env( "FABRO_WORKER_TOKEN", issue_test_worker_jwt(&context.storage_dir, run_id), @@ -194,7 +195,7 @@ fn help() { ----- stdout ----- Internal: execute a single workflow run locally - Usage: fabro __run-worker [OPTIONS] --server --run-dir --run-id --mode + Usage: fabro __run-worker [OPTIONS] --server --storage-dir --run-dir --run-id --mode Options: --json Output as JSON [env: FABRO_JSON=] @@ -216,15 +217,8 @@ fn worker_requires_fabro_worker_token_env() { let context = auth_context(); let run_dir = tempfile::tempdir().unwrap(); let run_id = unique_run_id(); - let output = context - .command() + let output = worker_base_command(&context) .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", @@ -282,12 +276,6 @@ 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", @@ -356,12 +344,6 @@ digraph GitHubApp { let mut cmd = worker_command(&context, run_id.as_str()); 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", @@ -410,12 +392,6 @@ digraph DetachedStoreOnly { let server = server_target(&context.storage_dir); 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", @@ -633,12 +609,6 @@ 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", @@ -703,12 +673,6 @@ 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-cli/tests/it/cmd/worker_auth.rs b/lib/apps/fabro-cli/tests/it/cmd/worker_auth.rs index eb9ff0290..5e66a9e82 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/worker_auth.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/worker_auth.rs @@ -393,17 +393,17 @@ fn runner_rejects_bogus_worker_token_against_github_only_server() { cmd.env("FABRO_HOME", &worker_home); cmd.env("FABRO_AUTH_FILE", &auth_file); cmd.env("FABRO_WORKER_TOKEN", bogus_token); - cmd.args([ - "__run-worker", - "--server", - &target, - "--run-dir", - run_dir.to_str().unwrap(), - "--run-id", - &run_id, - "--mode", - "start", - ]); + cmd.arg("__run-worker") + .arg("--server") + .arg(&target) + .arg("--storage-dir") + .arg(&server.storage_dir) + .arg("--run-dir") + .arg(&run_dir) + .arg("--run-id") + .arg(&run_id) + .arg("--mode") + .arg("start"); cmd.stdin(Stdio::null()); cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 84a55693a..1f0b9ed67 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -583,9 +583,10 @@ async fn build_preflight_report( llm_result, ) .await; - run_github_token_check(&mut checks, prepared, &resolved_run, github_app).await; + let github_token_ok = + run_github_token_check(&mut checks, prepared, &resolved_run, github_app).await; - let checks_ok = sandbox_ok && repository_access_ok && llm_ok; + let checks_ok = sandbox_ok && repository_access_ok && llm_ok && github_token_ok; Ok(( CheckReport { @@ -1203,49 +1204,63 @@ async fn run_github_token_check( prepared: &PreparedManifest, resolved_run: &RunNamespace, github_app: Option, -) { +) -> bool { if !resolved_run.integrations.github.is_token_requested() { - return; + return true; } // Resolve InterpString permission values eagerly for token minting and // for display in the preflight report. - let github_permissions = resolved_run - .integrations - .github - .resolve_permissions() - .unwrap_or_default(); + let github_permissions = match resolved_run.integrations.github.resolve_permissions() { + Ok(permissions) => permissions, + Err(err) => { + checks.push(CheckResult { + name: "GitHub Token".into(), + status: CheckStatus::Error, + summary: "invalid permissions".into(), + details: vec![], + remediation: Some(format!("Failed to resolve GitHub permissions: {err}")), + }); + return false; + } + }; let perm_details = github_permissions .iter() .map(|(key, value)| CheckDetail::new(format!("{key}: {value}"))) .collect::>(); - match (&github_app, prepared.git.as_ref()) { - (Some(creds), Some(git)) => { - match mint_github_token(creds, &git.origin_url, &github_permissions).await { - Ok(_) => checks.push(CheckResult { + if let (Some(creds), Some(git)) = (&github_app, prepared.git.as_ref()) { + match mint_github_token(creds, &git.origin_url, &github_permissions).await { + Ok(_) => { + checks.push(CheckResult { name: "GitHub Token".into(), status: CheckStatus::Pass, summary: "minted".into(), details: perm_details, remediation: None, - }), - Err(err) => checks.push(CheckResult { + }); + true + } + Err(err) => { + checks.push(CheckResult { name: "GitHub Token".into(), status: CheckStatus::Error, summary: "failed".into(), details: perm_details, remediation: Some(format!("Failed to mint GitHub token: {err}")), - }), + }); + false } } - _ => checks.push(CheckResult { + } else { + checks.push(CheckResult { name: "GitHub Token".into(), status: CheckStatus::Warning, summary: "skipped".into(), details: perm_details, remediation: Some("No GitHub credentials or origin URL available".to_string()), - }), + }); + true } } @@ -2260,6 +2275,55 @@ issues = "read" ); } + #[tokio::test] + async fn preflight_rejects_unresolved_github_permissions() { + let state = crate::test_support::test_app_state(); + let mut manifest = minimal_manifest(); + manifest.workflows.get_mut("workflow.fabro").unwrap().config = + Some(types::ManifestWorkflowConfig { + path: "workflow.toml".to_string(), + source: r#"_version = 1 + +[run.environment] +id = "local" + +[run.integrations.github.permissions] +issues = "{{ env.GITHUB_ISSUES_PERMISSION }}" +"# + .to_string(), + }); + + let prepared = prepare_manifest( + &manifest_run_defaults(Some(&default_settings_fixture())), + &manifest, + ) + .unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + assert!(!validated.has_errors()); + + let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); + let github_token_check = response.checks.sections[0] + .checks + .iter() + .find(|check| check.name == "GitHub Token") + .expect("GitHub Token check should report invalid permissions"); + + assert!(!ok); + assert_eq!( + github_token_check.status, + types::PreflightCheckResultStatus::Error + ); + assert_eq!(github_token_check.summary, "invalid permissions"); + assert!( + github_token_check + .remediation + .as_deref() + .is_some_and(|message| message.contains("GITHUB_ISSUES_PERMISSION")) + ); + } + #[tokio::test] async fn preflight_allows_pull_request_enabled_without_github_credentials() { let state = crate::test_support::test_app_state(); diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 284800b85..b8fc73da5 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -4078,17 +4078,31 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { return; } }; - let github_permissions = persisted + let github_permissions = match persisted .run_spec() .settings .run .integrations .github .resolve_permissions() - .unwrap_or_else(|err| { - tracing::warn!(error = %err, "github permission interpolation failed"); - std::collections::HashMap::new() - }); + { + Ok(permissions) => permissions, + Err(err) => { + tracing::error!( + run_id = %run_id, + error = %err, + "GitHub permission interpolation failed" + ); + fail_run_before_execution( + &state, + run_id, + FailureReason::WorkflowError, + format!("Failed to resolve GitHub permissions: {err}"), + ) + .await; + return; + } + }; let vault = match state.stores.vault.snapshot().await { Ok(vault) => vault, Err(err) => { diff --git a/lib/components/fabro-agent/tests/it/parity_matrix.rs b/lib/components/fabro-agent/tests/it/parity_matrix.rs index 385bd5275..e37a29003 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::test_support; +use fabro_auth::EnvCredentialSource; 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 = test_support::StubCredentialSource; + let source = EnvCredentialSource::new(); 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/src/executor.rs b/lib/components/fabro-hooks/src/executor.rs index 9acc2fdaa..4aa87d168 100644 --- a/lib/components/fabro-hooks/src/executor.rs +++ b/lib/components/fabro-hooks/src/executor.rs @@ -74,9 +74,9 @@ fn resolve_interp(value: &InterpString) -> Result { #[expect( clippy::disallowed_methods, - reason = "hook HTTP logs use the unresolved token source, not the resolved URL, so env-sourced \ - URL material is not logged; redacted_url_for_log masks literal credentials in \ - parseable source URLs and replaces unparseable sources with a placeholder" + reason = "hook HTTP logs use the unresolved token source, not the resolved URL; \ + redacted_url_for_log masks literal credentials in parseable source URLs and \ + replaces unparseable sources with a placeholder" )] fn safe_url_source_for_log(url: &InterpString) -> String { redacted_url_for_log(&url.as_source()) @@ -288,7 +288,7 @@ impl HookExecutorImpl { let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model) { Ok(resolved) => resolved, Err(error) => { - tracing::error!(error = %error, "prompt hook env resolution failed, not firing"); + tracing::error!(error = %error, "prompt hook interpolation failed, not firing"); return HookDecision::Block { reason: Some(error.to_string()), }; @@ -353,7 +353,7 @@ impl HookExecutorImpl { let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model) { Ok(resolved) => resolved, Err(error) => { - tracing::error!(error = %error, "agent hook env resolution failed, not firing"); + tracing::error!(error = %error, "agent hook interpolation failed, not firing"); return HookDecision::Block { reason: Some(error.to_string()), }; @@ -495,7 +495,7 @@ impl HookExecutorImpl { tracing::error!( url_source = %safe_url_source_for_log(url), error = %error, - "HTTP hook URL env resolution failed, not firing" + "HTTP hook URL interpolation failed, not firing" ); return HookDecision::Block { reason: Some(error.to_string()), @@ -528,7 +528,7 @@ impl HookExecutorImpl { url_source = %safe_url_source_for_log(url), header = %key, error = %error, - "HTTP hook header env resolution failed, not firing" + "HTTP hook header interpolation failed, not firing" ); return HookDecision::Block { reason: Some(error.to_string()), @@ -1336,7 +1336,6 @@ mod tests { &client, &interp(&server.url("/hook")), Some(&headers), - // Allowlisted but unset: still blocks on the Missing lookup. &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index d9fbd89e6..1b08ab611 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -556,7 +556,7 @@ fn git_checkpoint_options_from_start( #[expect( clippy::disallowed_methods, - reason = "Run startup interpolation owns a process-env lookup facade for {{ env.* }} values." + reason = "Run startup reads process env only for explicit provider credential refs and test mode." )] fn process_env_var(name: &str) -> Option { std::env::var(name).ok() @@ -722,24 +722,22 @@ impl ModelRegistry for CatalogModelRegistry<'_> { } } -/// Build the launch-time MCP config from resolved settings, resolving any -/// `{{ env.* }}` and `{{ secrets.* }}` tokens in the transport -/// (`command`/`url`/`env`/`headers`) against the worker process environment and -/// vault — the run boundary where the MCP is actually launched. +/// Build the launch-time MCP config from resolved settings. Secret tokens in +/// the transport (`command`/`url`/`env`/`headers`) resolve from the vault at +/// the run boundary. Unsupported tokens fail. /// /// The resolution itself lives on the type -/// ([`McpServerSettings::resolve_transport_env`]) so `fabro run` (here) and +/// ([`McpServerSettings::resolve_transport_secrets`]) so `fabro run` (here) and /// `fabro exec` share one resolver; this wrapper just adds the server name to /// the error. MCP transport strings are carried in source form out of the -/// config resolve layer so `fabro validate` stays portable (it never requires -/// env to be set), and a referenced env var or secret that is unset is a hard -/// error — no fallback to the unresolved source. +/// config resolve layer so `fabro validate` stays portable. A missing or +/// non-token secret is a hard error. fn runtime_mcp_server( settings: &ResolvedMcpServerSettings, secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { settings - .resolve_transport_env(secrets_lookup) + .resolve_transport_secrets(secrets_lookup) .map_err(|err| { Error::engine_with_source( format!("failed to resolve MCP server {:?}", settings.name), @@ -748,24 +746,22 @@ fn runtime_mcp_server( }) } -/// Build the launch-time setup (prepare) commands from resolved settings, -/// resolving any `{{ env.* }}` and `{{ secrets.* }}` tokens in each step's -/// command and per-step env against the worker process environment and vault — -/// the run boundary where the steps actually run. +/// Build the launch-time setup (prepare) commands from resolved settings. +/// Secret tokens in each step's command and per-step env resolve from the vault +/// at the run boundary. Unsupported tokens fail. /// /// The resolution itself lives on the type -/// ([`ResolvedRunPrepareSettings::resolve_step_env`]) so prepare-step env +/// ([`ResolvedRunPrepareSettings::resolve_step_secrets`]) so prepare-step /// resolution shares one resolver with the rest of the run-boundary /// interpolation. Prepare-step commands and env are carried in source form out -/// of the config resolve layer so `fabro validate` stays portable (it never -/// requires env to be set), and a referenced env var or secret that is unset is -/// a hard error — no fallback to the unresolved source. +/// of the config resolve layer so `fabro validate` stays portable. A missing or +/// non-token secret is a hard error. fn runtime_setup_commands( prepare: &ResolvedRunPrepareSettings, secrets_lookup: impl FnMut(&str) -> Option, ) -> Result, Error> { let resolved = prepare - .resolve_step_env(secrets_lookup) + .resolve_step_secrets(secrets_lookup) .map_err(|err| Error::engine_with_source("failed to resolve prepare step", err))?; Ok(resolved .steps diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index 46d189825..395417253 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -6913,7 +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_auth::EnvCredentialSource; use fabro_graphviz::graph::Node; use fabro_llm::client::Client; use fabro_llm::providers::OpenAiAdapter; @@ -7007,7 +7007,7 @@ mod real_llm { } fabro_test::require_env("ANTHROPIC_API_KEY")?; - let source = auth_test_support::StubCredentialSource; + let source = EnvCredentialSource::new(); Some(Arc::new( Client::from_source(&source, super::default_catalog()) .await diff --git a/lib/foundation/fabro-auth/src/env_source.rs b/lib/foundation/fabro-auth/src/env_source.rs new file mode 100644 index 000000000..36e02635a --- /dev/null +++ b/lib/foundation/fabro-auth/src/env_source.rs @@ -0,0 +1,174 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_model::{Catalog, ProviderId}; +use fabro_static::EnvVars; +use fabro_vault::Vault; +use tokio::sync::RwLock as AsyncRwLock; + +use crate::resolve::apply_openai_codex_api_context; +use crate::{CredentialSource, EnvLookup, ResolvedCredentials, VaultCredentialSource}; + +/// A credential source for provider credentials declared as `env:`. +/// +/// This public SDK facade does not resolve `{{ env.NAME }}` settings +/// interpolation. Provider extra headers can use literals, but secret +/// interpolation requires a vault-backed source. +#[derive(Clone)] +pub struct EnvCredentialSource { + inner: VaultCredentialSource, + env_lookup: EnvLookup, +} + +impl EnvCredentialSource { + #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "EnvCredentialSource is the provider credential 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 { + let vault = Arc::new(AsyncRwLock::new(Vault::from_entries(HashMap::new()))); + let inner_lookup = Arc::clone(&env_lookup); + let inner = VaultCredentialSource::with_env_lookup(vault, move |name| inner_lookup(name)); + Self { inner, env_lookup } + } + + fn lookup(&self, name: &str) -> Option { + (self.env_lookup)(name) + } +} + +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 resolved = self.inner.resolve(catalog).await?; + if let (Some(account_id), Some(credential)) = ( + self.lookup(EnvVars::CHATGPT_ACCOUNT_ID), + resolved + .credentials + .iter_mut() + .find(|credential| credential.provider == ProviderId::openai()), + ) { + apply_openai_codex_api_context(credential, Some(&account_id), self.env_lookup.as_ref()); + } + Ok(resolved) + } + + async fn configured_providers(&self, catalog: &Catalog) -> Vec { + self.inner.configured_providers(catalog).await + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::sync::Arc; + + use fabro_model::catalog::LlmCatalogSettings; + use fabro_model::{Catalog, ProviderId}; + use fabro_types::settings::interp::Namespace; + + 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())) + } + + #[tokio::test] + async fn configured_providers_reads_injected_provider_env() { + let source = test_source(&[("ANTHROPIC_API_KEY", "anthropic-key")]); + let catalog = Catalog::from_builtin().unwrap(); + + assert_eq!(source.configured_providers(&catalog).await, vec![ + ProviderId::anthropic() + ]); + } + + #[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 = Catalog::from_builtin().unwrap(); + + 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 env_settings_interpolation_remains_unsupported() { + let settings: LlmCatalogSettings = toml::from_str( + 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"] + +[providers.acme.extra_headers] +x-account = "{{ env.ACME_ACCOUNT }}" +"#, + ) + .unwrap(); + let catalog = Catalog::from_builtin_with_overrides(&settings).unwrap(); + let source = test_source(&[("ACME_API_KEY", "acme-key"), ("ACME_ACCOUNT", "account-id")]); + + let resolved = source.resolve(&catalog).await.unwrap(); + + assert!( + resolved + .credentials + .iter() + .all(|credential| credential.provider != ProviderId::new("acme")) + ); + assert!(resolved.auth_issues.iter().any(|(provider, issue)| { + provider == &ProviderId::new("acme") + && matches!( + issue, + crate::ResolveError::Interpolation { source, .. } + if source.namespace == Namespace::Env + ) + })); + } +} diff --git a/lib/foundation/fabro-auth/src/lib.rs b/lib/foundation/fabro-auth/src/lib.rs index b57075718..e54266f86 100644 --- a/lib/foundation/fabro-auth/src/lib.rs +++ b/lib/foundation/fabro-auth/src/lib.rs @@ -1,6 +1,7 @@ mod context; mod credential; mod credential_source; +mod env_source; mod extra_headers_source; mod refresh; mod resolve; @@ -16,6 +17,7 @@ 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::{ diff --git a/lib/foundation/fabro-auth/src/test_support.rs b/lib/foundation/fabro-auth/src/test_support.rs index 944f83342..d3bb14cde 100644 --- a/lib/foundation/fabro-auth/src/test_support.rs +++ b/lib/foundation/fabro-auth/src/test_support.rs @@ -7,33 +7,12 @@ 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::credential_source::CredentialSource; 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> { diff --git a/lib/foundation/fabro-config/src/layers/llm.rs b/lib/foundation/fabro-config/src/layers/llm.rs index d1faa7e8d..1e49fe1c9 100644 --- a/lib/foundation/fabro-config/src/layers/llm.rs +++ b/lib/foundation/fabro-config/src/layers/llm.rs @@ -76,10 +76,9 @@ pub struct ProviderSettings { #[serde(default, skip_serializing_if = "Option::is_none")] pub base_url: Option, /// Extra HTTP headers attached to every outgoing provider request after - /// credential resolution. Values are interpolation strings: literal text, - /// `{{ env.NAME }}`, or `{{ secrets.NAME }}`. Put credentials in a secret - /// and reference them with a `{{ secrets.NAME }}` token, not a bare - /// literal. + /// credential resolution. Values are literal text or + /// `{{ secrets.NAME }}` interpolation strings. Put credentials in a secret + /// and reference them with a token, not a bare literal. #[serde(default, skip_serializing_if = "Option::is_none")] pub extra_headers: Option>, /// Higher wins; missing → `0`; ties broken by canonical provider ID. diff --git a/lib/foundation/fabro-config/src/resolve/run.rs b/lib/foundation/fabro-config/src/resolve/run.rs index 2fe78b32e..1d20db7a6 100644 --- a/lib/foundation/fabro-config/src/resolve/run.rs +++ b/lib/foundation/fabro-config/src/resolve/run.rs @@ -156,8 +156,9 @@ fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings { #[expect( clippy::disallowed_methods, reason = "intentional source preservation: prepare step commands and per-step env are carried \ - in source form so `fabro validate` stays portable; their {{ env.* }} tokens resolve \ - at the run boundary in fabro_types::settings::run::RunPrepareSettings::resolve_step_env" + in source form so `fabro validate` stays portable; secret tokens resolve and \ + unsupported tokens fail at the run boundary in \ + fabro_types::settings::run::RunPrepareSettings::resolve_step_secrets" )] fn resolve_prepare( prepare: Option<&RunPrepareLayer>, @@ -168,18 +169,16 @@ fn resolve_prepare( let mut steps = Vec::new(); for (index, step) in prepare.steps.iter().enumerate() { let run = match (&step.script, &step.command) { - // A `script` is a raw shell snippet: carry it verbatim so the shell - // interprets it. Its `{{ env.* }}` tokens resolve at the run - // boundary. + // A `script` is a raw shell snippet. Carry it verbatim so the shell + // interprets it after late secret resolution. (Some(script), None) => PreparedStepRun::Script { script: script.as_source(), }, // A `command` is an argv: carry it as a vector of element source - // strings — neither pre-joined nor shell-quoted here. Each element's - // `{{ env.* }}` token resolves at the run boundary, and only the - // resolved value is shell-quoted (resolve-then-quote), so an - // interpolated value can never break out of its argument and inject - // shell syntax. + // strings — neither pre-joined nor shell-quoted here. Secret tokens + // resolve at the run boundary, and only the resolved value is + // shell-quoted (resolve-then-quote), so an interpolated value can + // never break out of its argument and inject shell syntax. (None, Some(argv)) => PreparedStepRun::Command { command: argv.iter().map(InterpString::as_source).collect(), }, @@ -386,8 +385,9 @@ pub(crate) fn resolve_enabled_mcps( #[expect( clippy::disallowed_methods, reason = "intentional source preservation: MCP transport strings are carried in source form \ - so `fabro validate` stays portable; their {{ env.* }} tokens resolve at the run \ - boundary in fabro_workflow::operations::start::runtime_mcp_server" + so `fabro validate` stays portable; secret tokens resolve and unsupported tokens \ + fail at the run boundary in \ + fabro_workflow::operations::start::runtime_mcp_server" )] pub(crate) fn resolve_mcp_entry(name: &str, entry: &McpEntryLayer) -> McpServerSettings { let transport = match entry { @@ -482,8 +482,9 @@ pub(crate) fn resolve_mcp_entry(name: &str, entry: &McpEntryLayer) -> McpServerS #[expect( clippy::disallowed_methods, reason = "intentional source preservation: MCP transport strings are carried in source form \ - so `fabro validate` stays portable; their {{ env.* }} tokens resolve at the run \ - boundary in fabro_workflow::operations::start::runtime_mcp_server" + so `fabro validate` stays portable; secret tokens resolve and unsupported tokens \ + fail at the run boundary in \ + fabro_workflow::operations::start::runtime_mcp_server" )] fn resolve_mcp_command( script: Option<&InterpString>, @@ -705,8 +706,8 @@ mod resolve_prepare_tests { assert_eq!(steps[0].run, PreparedStepRun::Script { script: "setup".to_string(), }); - // The per-step env survives resolution in source form (its {{ env.* }} - // token resolves later, at the run boundary). + // The per-step env survives config resolution in source form. The + // unsupported token fails later at the run boundary. assert_eq!( steps[0].env.get("TOKEN").map(String::as_str), Some("{{ env.DEPLOY_TOKEN }}") @@ -731,9 +732,9 @@ mod resolve_prepare_tests { let steps = resolve(&layer); // Argv elements are carried as separate source strings, NOT joined and - // NOT shell-quoted here. Quoting happens at the run boundary, after - // `{{ env.* }}` resolution, so the resolved value (not the source - // token) is what gets quoted. + // NOT shell-quoted here. Quoting happens after late interpolation at + // the run boundary, so a resolved value (not the source token) is what + // gets quoted. assert_eq!(steps[0].run, PreparedStepRun::Command { command: vec![ "echo".to_string(), diff --git a/lib/foundation/fabro-dev/src/commands/docs_options_reference.rs b/lib/foundation/fabro-dev/src/commands/docs_options_reference.rs index faf2ec963..6d80271d1 100644 --- a/lib/foundation/fabro-dev/src/commands/docs_options_reference.rs +++ b/lib/foundation/fabro-dev/src/commands/docs_options_reference.rs @@ -231,7 +231,7 @@ aliases = ["gateway"] credentials = ["env:ACME_GATEWAY_API_KEY", "vault:ACME_GATEWAY_API_KEY"] [llm.providers.proxy.extra_headers] -x-portkey-api-key = "{{ env.PORTKEY_API_KEY }}" +x-portkey-api-key = "{{ secrets.portkey_api_key }}" x-portkey-config = "@bedrock-prod" x-team-secret = "{{ secrets.gateway_team_secret }}" ``` @@ -246,7 +246,7 @@ x-team-secret = "{{ secrets.gateway_team_secret }}" | `auth` | table | omitted | API-key auth config. Omit the table entirely for providers that need no API key; any `extra_headers` are still attached. | | `auth.credentials` | array | required when `auth` present | Ordered credential refs. Accepted forms are `vault:`, `env:`, and `aws_sigv4` (sign requests from the AWS default credential chain — Bedrock). Literal secret strings are rejected. | | `auth.header` | `"bearer"` or `{ custom = "Header-Name" }` | `"bearer"` | Primary API-key header policy. Omit when the provider uses a standard bearer token. | -| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are interpolation strings: literal text, an `{{ env.NAME }}` token, or a `{{ secrets.NAME }}` token. Put credentials in a secret and reference them with a `{{ secrets.NAME }}` token, not a bare literal. | +| `extra_headers` | table | `{}` | Additional headers attached to provider requests. Values are literal text or `{{ secrets.NAME }}` interpolation strings. Put credentials in a secret and reference them with a token, not a bare literal. | | `priority` | integer | `0` | Higher-priority ready providers win unqualified model and default selection; ties use canonical provider ID. | | `enabled` | boolean | `true` | Set `false` to disable a provider after lower-precedence layers define it. | | `aliases` | array | `[]` | Additional provider names accepted by model routing and fallback config. | diff --git a/lib/foundation/fabro-types/src/settings/interp.rs b/lib/foundation/fabro-types/src/settings/interp.rs index 8bb1c4a06..fa1637bba 100644 --- a/lib/foundation/fabro-types/src/settings/interp.rs +++ b/lib/foundation/fabro-types/src/settings/interp.rs @@ -2,26 +2,26 @@ //! //! An [`InterpString`] field may contain narrow `{{ .NAME }}` //! tokens — no template logic. Which [`Namespace`]s resolve is -//! scope-determined by the caller through [`ResolveCtx`]: run-scope settings -//! provide `vars` and `secrets`, a command node `script` provides `inputs`, -//! `vars`, and `goal`. A token whose namespace has no lookup in the resolution -//! context fails loudly rather than passing through as literal text. +//! scope-determined by the caller through [`ResolveCtx`]. Run-scope settings +//! provide `vars` during run creation and `secrets` at consumption time. A +//! token whose namespace has no lookup in the resolution context fails loudly +//! rather than passing through as literal text. //! -//! Two namespaces parse but never resolve. `inputs` and `goal` are bound only -//! where a run's values are in scope, which is the workflow graph rather than -//! general config. `env` resolves nowhere at all: the process environment is -//! not a configuration source, and `{{ vars.NAME }}` (non-sensitive, stored on -//! the server) or `{{ secrets.NAME }}` (vault-backed) replaces it. Keeping -//! them parseable is what lets an out-of-scope token fail with a message that -//! names the alternative instead of reaching a consumer as literal text. +//! Two namespaces parse but have no [`ResolveCtx`] lookup. `inputs` is handled +//! by the workflow template layer, not general config interpolation. `env` +//! resolves nowhere: the process environment is not a configuration source. +//! Use `{{ vars.NAME }}` for non-sensitive server-owned values or +//! `{{ secrets.NAME }}` for vault-backed values. Keeping both namespaces +//! parseable lets an out-of-scope token fail with a useful message instead of +//! reaching a consumer as literal text. //! -//! Resolution timing is split: `vars` and `inputs` substitute early -//! (server-side, at run creation) via [`InterpString::substitute_with`], while -//! `secrets` resolves late, at consumption time in the process that owns the -//! value, via [`InterpString::resolve_with`]. Resolved secret values are plain -//! strings; sensitivity is not tracked. Redaction of run output is -//! content-based (entropy analysis plus credential patterns), applied where -//! output is serialized. +//! Resolution timing is split: `vars` substitute early (server-side, at run +//! creation) via [`InterpString::substitute_with`], while `secrets` resolve +//! late, at consumption time in the process that owns the value, via +//! [`InterpString::resolve_with`]. Resolved secret values are plain strings; +//! sensitivity is not tracked. Redaction of run output is content-based +//! (entropy analysis plus credential patterns), applied where output is +//! serialized. use std::borrow::Cow; use std::fmt; @@ -308,9 +308,10 @@ impl InterpString { /// for the namespaces it does not — their resolution happens later, /// possibly in a different process. /// - /// This is the early, server-side pass (`vars`/`inputs`); late-bound - /// namespaces (`env`/`secrets`) survive in token form for their - /// consumption-time [`InterpString::resolve_with`]. + /// This is the early, server-side `vars` pass. `secrets` survive in token + /// form for consumption-time [`InterpString::resolve_with`]. Unsupported + /// `env` tokens and template-only `inputs` tokens also remain so the next + /// full-resolution boundary can reject or route them explicitly. pub fn substitute_with(&self, ctx: &mut ResolveCtx<'_>) -> Result { let mut segments = Vec::new(); for seg in &self.segments { diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index e7c5a95df..c5f62b480 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -269,9 +269,9 @@ where /// value. Both interpolation passes route through this one traversal so they /// cannot drift as `PreparedStepRun` or `PreparedStep` grow fields: the /// `{{ vars.* }}` pass ([`RunNamespace::substitute_variables`]) passes a -/// `substitute_string` visitor, the `{{ env.* }}` pass -/// ([`RunPrepareSettings::resolve_step_env`]) passes a `resolve_env_string` -/// visitor. Mirrors [`visit_mcp_transport_strings`]. +/// `substitute_string` visitor, and the late secret pass +/// ([`RunPrepareSettings::resolve_step_secrets`]) passes a +/// `resolve_secret_string` visitor. Mirrors [`visit_mcp_transport_strings`]. fn visit_prepared_step_strings( step: &mut PreparedStep, visitor: &mut F, @@ -735,36 +735,32 @@ impl Default for RunPrepareSettings { } impl RunPrepareSettings { - /// Resolve `{{ env.* }}` and `{{ secrets.* }}` tokens in every prepare - /// step's runnable part and per-step `env` values against the supplied - /// lookups, returning a copy with the tokens replaced and every other field - /// preserved. A `script` step's snippet resolves in place; a `command` - /// step's argv resolves per element (each element is shell-quoted later, in - /// [`PreparedStep::to_shell_command`], so quoting applies to the resolved - /// value rather than the source token). + /// Resolve `{{ secrets.* }}` tokens in every prepare step's runnable part + /// and per-step `env` values against the supplied lookup. Unsupported + /// tokens fail instead of reaching the process. A `script` step's snippet + /// resolves in place; a `command` step's argv resolves per element (each + /// element is shell-quoted later, in [`PreparedStep::to_shell_command`], so + /// quoting applies to the resolved value rather than the source token). /// /// This is the late, use-time half of prepare-step interpolation, the /// counterpart to the server-side `{{ vars.* }}` substitution in /// [`RunNamespace::substitute_variables`]: `{{ vars.* }}` are substituted - /// earlier, server-side, while `{{ env.* }}` and `{{ secrets.* }}` resolve - /// here — in whichever process actually runs the steps (the run worker for - /// `fabro run`). + /// earlier, server-side, while `{{ secrets.* }}` resolves in whichever + /// process actually runs the steps (the run worker for `fabro run`). /// Carrying the source form out of the config resolve layer keeps - /// `fabro validate` portable (it never requires env to be set). + /// `fabro validate` portable. /// - /// A referenced env var or secret that is unset is a hard error — no - /// fallback to the unresolved source. Reserved `inputs` tokens have no - /// lookup here and surface as a loud - /// [`ResolveErrorKind::Unavailable`] error rather than - /// passing through as literal text. - pub fn resolve_step_env( + /// A missing or non-token secret is a hard error. Unsupported `env` and + /// template-only `inputs` tokens surface as + /// [`ResolveErrorKind::Unavailable`] errors. + pub fn resolve_step_secrets( &self, mut secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { let mut resolved = self.clone(); for step in &mut resolved.steps { visit_prepared_step_strings(step, &mut |value| { - resolve_env_string(value, &mut secrets_lookup) + resolve_secret_string(value, &mut secrets_lookup) })?; } Ok(resolved) @@ -774,9 +770,9 @@ impl RunPrepareSettings { /// A single resolved prepare step: the thing to run plus the per-step /// environment variables it should see. The runnable part keeps the /// script-vs-argv distinction (see [`PreparedStepRun`]), and every string is -/// carried in source form out of the config resolve layer; their `{{ env.* }}` -/// tokens resolve at the run boundary via -/// [`RunPrepareSettings::resolve_step_env`]. +/// carried in source form out of the config resolve layer. Secret tokens +/// resolve at the run boundary via +/// [`RunPrepareSettings::resolve_step_secrets`]. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct PreparedStep { #[serde(flatten)] @@ -793,10 +789,10 @@ pub struct PreparedStep { /// for the shell to interpret. /// - [`Command`](PreparedStepRun::Command) is an argv: a vector of element /// source strings, neither pre-joined nor shell-quoted at config time. Its -/// `{{ env.* }}` tokens resolve per element at the run boundary, and only -/// then is each *resolved* element shell-quoted and joined. Resolving before -/// quoting is what stops an interpolated env value from breaking out of its -/// argument and injecting shell syntax. +/// `{{ secrets.* }}` tokens resolve per element at the run boundary, and only +/// then is each resolved element shell-quoted and joined. Resolving before +/// quoting stops an interpolated secret from breaking out of its argument and +/// injecting shell syntax. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "snake_case")] pub enum PreparedStepRun { @@ -811,8 +807,8 @@ impl PreparedStep { /// For a script, the snippet is returned verbatim. For an argv `command`, /// each element is shell-quoted and joined with spaces so an argument that /// contains spaces or shell metacharacters survives as a single token. This - /// must run *after* [`RunPrepareSettings::resolve_step_env`] so the quoting - /// applies to the resolved values, not the `{{ env.* }}` source. + /// must run *after* [`RunPrepareSettings::resolve_step_secrets`] so the + /// quoting applies to the resolved values, not the secret token source. pub fn to_shell_command(&self) -> String { match &self.run { PreparedStepRun::Script { script } => script.clone(), @@ -1568,30 +1564,27 @@ impl McpServerSettings { StdDuration::from_secs(self.tool_timeout_secs) } - /// Resolve `{{ env.* }}` and `{{ secrets.* }}` tokens in this server's - /// transport strings (`command`/`args`/`url`/`env`/`headers`) against the - /// supplied lookups, returning a copy with the tokens replaced and every - /// other field preserved. + /// Resolve `{{ secrets.* }}` tokens in this server's transport strings + /// (`command`/`args`/`url`/`env`/`headers`) against the supplied lookup. + /// Unsupported tokens fail instead of reaching the transport. /// /// This is the late, use-time half of MCP interpolation, the counterpart /// to [`substitute_mcp_transport`]: `{{ vars.* }}` are substituted - /// earlier, server-side, while `{{ env.* }}` and `{{ secrets.* }}` resolve - /// here — in whichever process actually launches the server (the run worker - /// for `fabro run`, the CLI process for `fabro exec`). Carrying the source - /// form out of the config resolve layer keeps `fabro validate` portable (it - /// never requires env to be set). + /// earlier, server-side, while `{{ secrets.* }}` resolves in whichever + /// process actually launches the server (the run worker for `fabro run`, + /// the CLI process for `fabro exec`). Carrying the source form out of the + /// config resolve layer keeps `fabro validate` portable. /// - /// A referenced env var or secret that is unset is a hard error — no - /// fallback to the unresolved source. Reserved `inputs` tokens have no - /// lookup here and surface as a loud [`ResolveErrorKind::Unavailable`] - /// error rather than passing through as literal text. - pub fn resolve_transport_env( + /// A missing or non-token secret is a hard error. Unsupported `env` and + /// template-only `inputs` tokens surface as + /// [`ResolveErrorKind::Unavailable`] errors. + pub fn resolve_transport_secrets( &self, mut secrets_lookup: impl FnMut(&str) -> Option, ) -> Result { let mut resolved = self.clone(); visit_mcp_transport_strings(&mut resolved.transport, &mut |value| { - resolve_env_string(value, &mut secrets_lookup) + resolve_secret_string(value, &mut secrets_lookup) })?; Ok(resolved) } @@ -1599,7 +1592,7 @@ impl McpServerSettings { /// Resolve `{{ secrets.* }}` tokens in one run-boundary /// string. A literal value (no tokens) round-trips unchanged. -fn resolve_env_string( +fn resolve_secret_string( value: &mut String, secrets_lookup: &mut impl FnMut(&str) -> Option, ) -> Result<(), ResolveError> { @@ -1612,7 +1605,7 @@ fn resolve_env_string( } #[cfg(test)] -mod resolve_transport_env_tests { +mod resolve_transport_secrets_tests { use std::collections::HashMap; use super::super::interp::ResolveErrorKind; @@ -1631,7 +1624,9 @@ mod resolve_transport_env_tests { ..McpServerSettings::default() }; - let resolved = settings.resolve_transport_env(secret_lookup(&[])).unwrap(); + let resolved = settings + .resolve_transport_secrets(secret_lookup(&[])) + .unwrap(); let McpTransport::Stdio { command, env } = resolved.transport else { panic!("expected stdio transport"); @@ -1641,7 +1636,7 @@ mod resolve_transport_env_tests { } #[test] - fn missing_env_is_hard_error() { + fn missing_secret_is_hard_error() { let settings = McpServerSettings { name: "gemini".to_string(), transport: McpTransport::Stdio { @@ -1655,7 +1650,7 @@ mod resolve_transport_env_tests { }; let err = settings - .resolve_transport_env(secret_lookup(&[])) + .resolve_transport_secrets(secret_lookup(&[])) .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); @@ -1682,7 +1677,7 @@ mod resolve_transport_env_tests { }; let resolved = settings - .resolve_transport_env(secret_lookup(&[ + .resolve_transport_secrets(secret_lookup(&[ ("SERVER_BIN", "/srv/mcp"), ("API_TOKEN", "vault-token"), ])) @@ -1718,7 +1713,7 @@ mod resolve_transport_env_tests { }; let resolved = settings - .resolve_transport_env(secret_lookup(&[ + .resolve_transport_secrets(secret_lookup(&[ ("MCP_HOST", "mcp.example"), ("MCP_TOKEN", "vault-token"), ])) @@ -1749,7 +1744,7 @@ mod resolve_transport_env_tests { }; let err = settings - .resolve_transport_env(secret_lookup(&[])) + .resolve_transport_secrets(secret_lookup(&[])) .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); @@ -1759,7 +1754,7 @@ mod resolve_transport_env_tests { } #[cfg(test)] -mod resolve_step_env_tests { +mod resolve_step_secrets_tests { use std::collections::HashMap; use super::super::interp::ResolveErrorKind; @@ -1823,7 +1818,7 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let resolved = settings.resolve_step_env(secret_lookup(&[])).unwrap(); + let resolved = settings.resolve_step_secrets(secret_lookup(&[])).unwrap(); assert_eq!(resolved.steps[0].to_shell_command(), "echo hello"); assert_eq!( @@ -1845,7 +1840,7 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env(secret_lookup(&[("REGION", "us-east-1")])) + .resolve_step_secrets(secret_lookup(&[("REGION", "us-east-1")])) .unwrap(); assert_eq!( @@ -1868,7 +1863,7 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env(secret_lookup(&[ + .resolve_step_secrets(secret_lookup(&[ ("REGION", "us-east-1"), ("DEPLOY_TOKEN", "secret-token"), ])) @@ -1894,7 +1889,7 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env(secret_lookup(&[("MESSAGE", "hello world")])) + .resolve_step_secrets(secret_lookup(&[("MESSAGE", "hello world")])) .unwrap(); let shell = resolved.steps[0].to_shell_command(); @@ -1904,7 +1899,7 @@ mod resolve_step_env_tests { #[test] fn command_arg_resolving_to_shell_metacharacters_is_not_injected() { - // Regression test for the command-injection defect: an `{{ env.* }}` + // Regression test for the command-injection defect: an interpolated // value containing a single quote and `;` must be resolved THEN quoted // so it stays a single argument and cannot break out to inject extra // shell commands. Quoting the source token *before* resolving (the old @@ -1919,7 +1914,7 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env(|name| (name == "USER_INPUT").then(|| malicious.to_string())) + .resolve_step_secrets(|name| (name == "USER_INPUT").then(|| malicious.to_string())) .unwrap(); let shell = resolved.steps[0].to_shell_command(); @@ -1949,7 +1944,9 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); + let err = settings + .resolve_step_secrets(secret_lookup(&[])) + .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "REGION"); @@ -1969,7 +1966,9 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); + let err = settings + .resolve_step_secrets(secret_lookup(&[])) + .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "DEPLOY_TOKEN"); @@ -1993,7 +1992,7 @@ mod resolve_step_env_tests { }; let resolved = settings - .resolve_step_env(secret_lookup(&[ + .resolve_step_secrets(secret_lookup(&[ ("REGION", "us-east-1"), ("DEPLOY_TOKEN", "vault-token"), ("MESSAGE", "hello world"), @@ -2021,7 +2020,9 @@ mod resolve_step_env_tests { timeout_ms: 1_000, }; - let err = settings.resolve_step_env(secret_lookup(&[])).unwrap_err(); + let err = settings + .resolve_step_secrets(secret_lookup(&[])) + .unwrap_err(); assert_eq!(err.namespace, Namespace::Secrets); assert_eq!(err.name, "API_KEY"); diff --git a/lib/packages/fabro-api-client/src/models/run-goal-file.ts b/lib/packages/fabro-api-client/src/models/run-goal-file.ts index 096ca898b..a2b0052ef 100644 --- a/lib/packages/fabro-api-client/src/models/run-goal-file.ts +++ b/lib/packages/fabro-api-client/src/models/run-goal-file.ts @@ -17,7 +17,7 @@ export interface RunGoalFile { 'type': RunGoalFileTypeEnum; /** - * Resolved config string that may contain env interpolation tokens. + * Config string that can contain typed interpolation tokens. */ 'value': string; } diff --git a/lib/packages/fabro-api-client/src/models/run-goal-inline.ts b/lib/packages/fabro-api-client/src/models/run-goal-inline.ts index 257d415f9..d0afe3a11 100644 --- a/lib/packages/fabro-api-client/src/models/run-goal-inline.ts +++ b/lib/packages/fabro-api-client/src/models/run-goal-inline.ts @@ -17,7 +17,7 @@ export interface RunGoalInline { 'type': RunGoalInlineTypeEnum; /** - * Resolved config string that may contain env interpolation tokens. + * Config string that can contain typed interpolation tokens. */ 'value': string; } diff --git a/lib/packages/fabro-api-client/src/models/run-namespace.ts b/lib/packages/fabro-api-client/src/models/run-namespace.ts index 9d3a003fd..cea567a72 100644 --- a/lib/packages/fabro-api-client/src/models/run-namespace.ts +++ b/lib/packages/fabro-api-client/src/models/run-namespace.ts @@ -71,7 +71,7 @@ import type { RunScmSettings } from './run-scm-settings'; export interface RunNamespace { 'goal': RunGoal | null; /** - * Resolved config string that may contain env interpolation tokens. + * Config string that can contain typed interpolation tokens. */ 'working_dir': string | null; 'metadata': { [key: string]: string; }; From e1805f4f33e59328551fd26952ab33695e1a83e8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 18:04:54 -0400 Subject: [PATCH 4/4] docs: clarify hook variable sensitivity --- docs/public/agents/hooks.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index 6ac3de5cb..e8b83f483 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -30,7 +30,7 @@ type = "http" url = "https://hooks.example.com/done" [hooks.headers] -Authorization = "Bearer {{ vars.WEBHOOK_TOKEN }}" +X-Deployment-Environment = "{{ vars.DEPLOY_ENV }}" ``` | Field | Description | @@ -39,7 +39,7 @@ Authorization = "Bearer {{ vars.WEBHOOK_TOKEN }}" | `headers` | Optional HTTP headers. Values support `{{ vars.NAME }}` interpolation. A token that is still unresolved when the hook fires blocks it (fail-closed), so a header is never sent half-rendered. | | `tls` | TLS mode: `"verify"` (default), `"no_verify"`, or `"off"`. | -`{{ vars.NAME }}` is substituted when the run is created. `{{ env.NAME }}` and `{{ secrets.NAME }}` are not available in hooks. +`{{ vars.NAME }}` is substituted when the run is created. Use variables only for non-sensitive metadata. Do not store tokens, API keys, or other credentials in variables or literal hook configuration. `{{ env.NAME }}` and `{{ secrets.NAME }}` are not available in hooks. ### Prompt