diff --git a/lib/crates/fabro-sandbox/src/from_environment.rs b/lib/crates/fabro-sandbox/src/from_environment.rs index 93dde3e56..ac972f22c 100644 --- a/lib/crates/fabro-sandbox/src/from_environment.rs +++ b/lib/crates/fabro-sandbox/src/from_environment.rs @@ -71,9 +71,14 @@ 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. let env = settings - .resolve_env(process_env_var, |_| None) - .unwrap_or_else(|_| source_env(settings)); + .env + .iter() + .map(|(key, value)| (key.clone(), value.resolve_or_source(process_env_var))) + .collect(); docker_config_from_environment_env(settings, skip_clone, env) } @@ -128,23 +133,6 @@ fn docker_config_from_environment_env( } } -#[cfg(feature = "docker")] -fn source_env(settings: &RunEnvironmentSettings) -> std::collections::HashMap { - settings - .env - .iter() - .map(|(key, value)| { - #[expect( - clippy::disallowed_methods, - reason = "Docker manifest/preflight fallback preserves unresolved run environment \ - source when no secret lookup is available" - )] - let source = value.as_source(); - (key.clone(), source) - }) - .collect() -} - pub fn local_working_directory_from_environment( settings: &RunEnvironmentSettings, source_directory: Option<&Path>, diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index d67032be8..e46103f81 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -1069,11 +1069,11 @@ impl RunEnvironmentSettings { 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 resolved = HashMap::with_capacity(self.env.len()); for (name, value) in &self.env { - let mut ctx = ResolveCtx::new() - .with_env(&mut env_lookup) - .with_secrets(&mut secrets_lookup); let resolved_value = match value.resolve_with(&mut ctx) { Ok(resolved) => resolved.value, Err(err) if err.namespace == Namespace::Env => { @@ -1099,9 +1099,23 @@ impl Default for RunEnvironmentSettings { } } +/// Build a lookup closure over a fixed list of name/value pairs for the +/// run-boundary resolver tests. Shared by the env, secret, prepare-step, and +/// MCP transport test modules. +#[cfg(test)] +fn pair_lookup( + pairs: &'static [(&'static str, &'static str)], +) -> impl Fn(&str) -> Option + Copy { + move |name| { + pairs + .iter() + .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) + } +} + #[cfg(test)] mod run_environment_settings_tests { - use super::{HashMap, InterpString, RunEnvironmentSettings}; + use super::{HashMap, InterpString, RunEnvironmentSettings, pair_lookup as lookup}; fn settings(env: &[(&str, &str)]) -> RunEnvironmentSettings { RunEnvironmentSettings { @@ -1113,16 +1127,6 @@ mod run_environment_settings_tests { } } - fn lookup( - pairs: &'static [(&'static str, &'static str)], - ) -> impl Fn(&str) -> Option + Copy { - move |name| { - pairs - .iter() - .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) - } - } - #[test] fn resolve_env_substitutes_env_tokens_via_lookup() { let s = settings(&[("NODE_ENV", "{{ env.NODE_ENV }}"), ("STATIC", "value")]); @@ -1609,27 +1613,10 @@ mod resolve_transport_env_tests { use std::collections::HashMap; use super::super::interp::ResolveErrorKind; - use super::{McpHttpProtocol, McpServerSettings, McpTransport, Namespace}; - - fn env_lookup( - pairs: &'static [(&'static str, &'static str)], - ) -> impl Fn(&str) -> Option + Copy { - move |name| { - pairs - .iter() - .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) - } - } - - fn secret_lookup( - pairs: &'static [(&'static str, &'static str)], - ) -> impl Fn(&str) -> Option + Copy { - move |name| { - pairs - .iter() - .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) - } - } + use super::{ + McpHttpProtocol, McpServerSettings, McpTransport, Namespace, pair_lookup as env_lookup, + pair_lookup as secret_lookup, + }; #[test] fn literal_transport_passes_through() { @@ -1845,27 +1832,10 @@ mod resolve_step_env_tests { use std::collections::HashMap; use super::super::interp::ResolveErrorKind; - use super::{Namespace, PreparedStep, PreparedStepRun, RunPrepareSettings}; - - fn env_lookup( - pairs: &'static [(&'static str, &'static str)], - ) -> impl Fn(&str) -> Option + Copy { - move |name| { - pairs - .iter() - .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) - } - } - - fn secret_lookup( - pairs: &'static [(&'static str, &'static str)], - ) -> impl Fn(&str) -> Option + Copy { - move |name| { - pairs - .iter() - .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) - } - } + use super::{ + Namespace, PreparedStep, PreparedStepRun, RunPrepareSettings, pair_lookup as env_lookup, + pair_lookup as secret_lookup, + }; fn script_step(script: &str, env: HashMap) -> PreparedStep { PreparedStep { diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 2d9da8f8d..7fa47e938 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -377,15 +377,17 @@ impl RunSession { Some(vault) => Some(vault.read().await), None => None, }; + // 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 mcp_servers = resolved .agent .mcps .iter() .map(|(key, entry)| match entry { ResolvedMcpEntry::Resolved(server) => { - runtime_mcp_server(server, process_env_var, |name| { - vault_token_lookup(vault_guard.as_deref(), name) - }) + runtime_mcp_server(server, process_env_var, secret_lookup) } // References must be resolved to concrete servers before the run // spec is persisted (server-side run-preparation pass). Reaching @@ -417,9 +419,7 @@ impl RunSession { SandboxSpec::Local { working_directory } } SandboxProviderKind::Docker => SandboxSpec::Docker { - config: resolve_docker_config(resolved, |name| { - vault_token_lookup(vault_guard.as_deref(), name) - })?, + config: resolve_docker_config(resolved, secret_lookup)?, github_app: services.github_app.clone(), run_id: Some(record.run_id), clone_origin_url: record.repo_origin_url().map(str::to_string), @@ -443,9 +443,7 @@ impl RunSession { let toml_env = resolved .environment - .resolve_env(process_env_var, |name| { - vault_token_lookup(vault_guard.as_deref(), name) - }) + .resolve_env(process_env_var, 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()); @@ -463,9 +461,8 @@ impl RunSession { }; let pr_config = resolved.pull_request.clone(); - let setup_commands = runtime_setup_commands(&resolved.prepare, process_env_var, |name| { - vault_token_lookup(vault_guard.as_deref(), name) - })?; + let setup_commands = + runtime_setup_commands(&resolved.prepare, process_env_var, secret_lookup)?; drop(vault_guard); Ok(Self {