diff --git a/Cargo.lock b/Cargo.lock index a804dc467..ce95cc186 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1457,6 +1457,7 @@ dependencies = [ name = "fabro-devcontainer" version = "0.174.0" dependencies = [ + "fabro-util", "insta", "reqwest 0.12.28", "serde", @@ -1534,6 +1535,7 @@ dependencies = [ "fabro-agent", "fabro-config", "fabro-llm", + "fabro-util", "mockito", "regex", "reqwest 0.12.28", diff --git a/lib/crates/fabro-devcontainer/Cargo.toml b/lib/crates/fabro-devcontainer/Cargo.toml index c12194663..aa70a07ed 100644 --- a/lib/crates/fabro-devcontainer/Cargo.toml +++ b/lib/crates/fabro-devcontainer/Cargo.toml @@ -9,6 +9,7 @@ description = "Parse and resolve devcontainer.json into Dockerfiles and lifecycl doctest = false [dependencies] +fabro-util = { path = "../fabro-util" } serde = { workspace = true } serde_json = { workspace = true } serde_yaml = "0.9" diff --git a/lib/crates/fabro-devcontainer/src/lib.rs b/lib/crates/fabro-devcontainer/src/lib.rs index 1e2ae3381..84c94e863 100644 --- a/lib/crates/fabro-devcontainer/src/lib.rs +++ b/lib/crates/fabro-devcontainer/src/lib.rs @@ -174,10 +174,12 @@ impl DevcontainerResolver { .clone() .unwrap_or_else(|| format!("/workspaces/{repo_name}")); + let system_env = fabro_util::env::SystemEnv; let preliminary_vars = variables::VariableContext { local_workspace_folder: repo_root.to_string_lossy().to_string(), local_workspace_folder_basename: repo_name.clone(), container_workspace_folder: raw_workspace_folder.clone(), + env: &system_env, }; let workspace_folder = variables::substitute(&raw_workspace_folder, &preliminary_vars); @@ -185,6 +187,7 @@ impl DevcontainerResolver { local_workspace_folder: repo_root.to_string_lossy().to_string(), local_workspace_folder_basename: repo_name.clone(), container_workspace_folder: workspace_folder.clone(), + env: &system_env, }; // Handle compose mode diff --git a/lib/crates/fabro-devcontainer/src/variables.rs b/lib/crates/fabro-devcontainer/src/variables.rs index 98d09699a..fd9aee496 100644 --- a/lib/crates/fabro-devcontainer/src/variables.rs +++ b/lib/crates/fabro-devcontainer/src/variables.rs @@ -1,8 +1,9 @@ /// Context for variable substitution. -pub struct VariableContext { +pub struct VariableContext<'a> { pub local_workspace_folder: String, pub local_workspace_folder_basename: String, pub container_workspace_folder: String, + pub env: &'a dyn fabro_util::env::Env, } /// Replace devcontainer variables in a string value. @@ -55,9 +56,13 @@ fn resolve_variable(expr: &str, ctx: &VariableContext) -> Option { if let Some(colon_pos) = var_part.find(':') { let var_name = &var_part[..colon_pos]; let default = &var_part[colon_pos + 1..]; - Some(std::env::var(var_name).unwrap_or_else(|_| default.to_string())) + Some( + ctx.env + .var(var_name) + .unwrap_or_else(|_| default.to_string()), + ) } else { - Some(std::env::var(var_part).unwrap_or_default()) + Some(ctx.env.var(var_part).unwrap_or_default()) } } _ => None, @@ -67,12 +72,17 @@ fn resolve_variable(expr: &str, ctx: &VariableContext) -> Option { #[cfg(test)] mod tests { use super::*; + use fabro_util::env::{SystemEnv, TestEnv}; + use std::collections::HashMap; - fn test_ctx() -> VariableContext { + fn test_ctx() -> VariableContext<'static> { + // Tests that don't exercise localEnv don't care about the env impl. + // Use SystemEnv which has no lifetime/allocation concerns. VariableContext { local_workspace_folder: "/home/user/project".to_string(), local_workspace_folder_basename: "project".to_string(), container_workspace_folder: "/workspaces/project".to_string(), + env: &SystemEnv, } } @@ -124,6 +134,7 @@ mod tests { local_workspace_folder: "/home/user/repos/my-app".to_string(), local_workspace_folder_basename: "my-app".to_string(), container_workspace_folder: "/workspaces/repos/my-app".to_string(), + env: &SystemEnv, }; assert_eq!( substitute("${containerWorkspaceFolderBasename}", &ctx), @@ -151,23 +162,34 @@ mod tests { #[test] fn local_env_with_set_variable() { - let ctx = test_ctx(); - std::env::set_var("FABRO_TEST_VAR_SET", "hello"); + let env = TestEnv(HashMap::from([( + "FABRO_TEST_VAR_SET".into(), + "hello".into(), + )])); + let ctx = VariableContext { + env: &env, + ..test_ctx() + }; assert_eq!(substitute("${localEnv:FABRO_TEST_VAR_SET}", &ctx), "hello"); - std::env::remove_var("FABRO_TEST_VAR_SET"); } #[test] fn local_env_unset_returns_empty() { - let ctx = test_ctx(); - std::env::remove_var("FABRO_TEST_VAR_UNSET_123"); + let env = TestEnv(HashMap::new()); + let ctx = VariableContext { + env: &env, + ..test_ctx() + }; assert_eq!(substitute("${localEnv:FABRO_TEST_VAR_UNSET_123}", &ctx), ""); } #[test] fn local_env_with_default_when_unset() { - let ctx = test_ctx(); - std::env::remove_var("FABRO_TEST_VAR_DEFAULT_456"); + let env = TestEnv(HashMap::new()); + let ctx = VariableContext { + env: &env, + ..test_ctx() + }; assert_eq!( substitute("${localEnv:FABRO_TEST_VAR_DEFAULT_456:fallback}", &ctx), "fallback" @@ -176,13 +198,18 @@ mod tests { #[test] fn local_env_with_default_when_set() { - let ctx = test_ctx(); - std::env::set_var("FABRO_TEST_VAR_DEFAULT_SET", "actual"); + let env = TestEnv(HashMap::from([( + "FABRO_TEST_VAR_DEFAULT_SET".into(), + "actual".into(), + )])); + let ctx = VariableContext { + env: &env, + ..test_ctx() + }; assert_eq!( substitute("${localEnv:FABRO_TEST_VAR_DEFAULT_SET:fallback}", &ctx), "actual" ); - std::env::remove_var("FABRO_TEST_VAR_DEFAULT_SET"); } #[test] diff --git a/lib/crates/fabro-hooks/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index 62f5bac30..377f4238f 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -12,6 +12,7 @@ doctest = false fabro-agent = { path = "../fabro-agent" } fabro-config = { path = "../fabro-config" } fabro-llm = { path = "../fabro-llm" } +fabro-util = { path = "../fabro-util" } serde.workspace = true serde_json.workspace = true tokio.workspace = true diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 64beac36d..12b0b9a32 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -40,7 +40,11 @@ pub trait HookExecutor: Send + Sync { /// Interpolate `$VAR` and `${VAR}` references in `value` using environment /// variables, but only when the variable name appears in `allowed_vars`. /// Unlisted or missing vars are replaced with the empty string. -pub fn interpolate_env_vars(value: &str, allowed_vars: &[String]) -> String { +pub fn interpolate_env_vars( + value: &str, + allowed_vars: &[String], + env: &dyn fabro_util::env::Env, +) -> String { let mut result = String::with_capacity(value.len()); let mut chars = value.chars().peekable(); @@ -66,7 +70,7 @@ pub fn interpolate_env_vars(value: &str, allowed_vars: &[String]) -> String { } if !var_name.is_empty() && allowed_vars.iter().any(|v| v == &var_name) { - if let Ok(val) = std::env::var(&var_name) { + if let Ok(val) = env.var(&var_name) { result.push_str(&val); } } @@ -403,6 +407,7 @@ impl HookExecutorImpl { /// Execute an HTTP hook: POST context JSON and parse the response. /// Fail-open: non-2xx and connection errors return `Proceed`. + #[allow(clippy::too_many_arguments)] async fn execute_http( client: &reqwest::Client, url: &str, @@ -411,6 +416,7 @@ impl HookExecutorImpl { tls: &TlsMode, context: &HookContext, timeout: std::time::Duration, + env: &dyn fabro_util::env::Env, ) -> HookDecision { // Enforce URL scheme based on TLS mode match tls { @@ -430,7 +436,7 @@ impl HookExecutorImpl { if let Some(hdrs) = headers { for (key, value) in hdrs { - let interpolated = interpolate_env_vars(value, allowed_env_vars); + let interpolated = interpolate_env_vars(value, allowed_env_vars, env); request = request.header(key, interpolated); } } @@ -547,6 +553,7 @@ impl HookExecutor for HookExecutorImpl { tls, context, definition.timeout(), + &fabro_util::env::SystemEnv, ) .await } @@ -837,60 +844,69 @@ mod tests { // --- interpolate_env_vars tests --- + fn test_env(vars: &[(&str, &str)]) -> fabro_util::env::TestEnv { + fabro_util::env::TestEnv( + vars.iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + ) + } + #[test] fn interpolate_resolves_allowed_var() { - std::env::set_var("FABRO_TEST_KEY_1", "secret123"); + let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); let result = interpolate_env_vars( "Bearer $FABRO_TEST_KEY_1", &["FABRO_TEST_KEY_1".to_string()], + &env, ); assert_eq!(result, "Bearer secret123"); - std::env::remove_var("FABRO_TEST_KEY_1"); } #[test] fn interpolate_resolves_braced_var() { - std::env::set_var("FABRO_TEST_KEY_2", "val"); - let result = - interpolate_env_vars("x${FABRO_TEST_KEY_2}y", &["FABRO_TEST_KEY_2".to_string()]); + let env = test_env(&[("FABRO_TEST_KEY_2", "val")]); + let result = interpolate_env_vars( + "x${FABRO_TEST_KEY_2}y", + &["FABRO_TEST_KEY_2".to_string()], + &env, + ); assert_eq!(result, "xvaly"); - std::env::remove_var("FABRO_TEST_KEY_2"); } #[test] fn interpolate_unlisted_var_becomes_empty() { - std::env::set_var("FABRO_TEST_KEY_3", "should_not_appear"); - let result = interpolate_env_vars("prefix-$FABRO_TEST_KEY_3-suffix", &[]); + let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); + let result = interpolate_env_vars("prefix-$FABRO_TEST_KEY_3-suffix", &[], &env); assert_eq!(result, "prefix--suffix"); - std::env::remove_var("FABRO_TEST_KEY_3"); } #[test] fn interpolate_missing_var_becomes_empty() { - std::env::remove_var("FABRO_TEST_NOEXIST"); + let env = test_env(&[]); let result = interpolate_env_vars( "a$FABRO_TEST_NOEXIST-b", &["FABRO_TEST_NOEXIST".to_string()], + &env, ); assert_eq!(result, "a-b"); } #[test] fn interpolate_no_vars_passes_through() { - assert_eq!(interpolate_env_vars("plain text", &[]), "plain text"); + let env = test_env(&[]); + assert_eq!(interpolate_env_vars("plain text", &[], &env), "plain text"); } #[test] fn interpolate_mixed_text() { - std::env::set_var("FABRO_TEST_A", "hello"); - std::env::set_var("FABRO_TEST_B", "world"); + let env = test_env(&[("FABRO_TEST_A", "hello"), ("FABRO_TEST_B", "world")]); let result = interpolate_env_vars( "$FABRO_TEST_A ${FABRO_TEST_B}!", &["FABRO_TEST_A".to_string(), "FABRO_TEST_B".to_string()], + &env, ); assert_eq!(result, "hello world!"); - std::env::remove_var("FABRO_TEST_A"); - std::env::remove_var("FABRO_TEST_B"); } // --- HTTP hook execution tests --- @@ -915,6 +931,7 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; @@ -946,6 +963,7 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; @@ -972,6 +990,7 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; @@ -990,6 +1009,7 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(1), + &test_env(&[]), ) .await; @@ -998,7 +1018,7 @@ mod tests { #[tokio::test] async fn http_hook_sends_interpolated_headers() { - std::env::set_var("FABRO_TEST_TOKEN", "my-secret"); + let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); let mut server = mockito::Server::new_async().await; let mock = server @@ -1023,12 +1043,12 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), + &env, ) .await; mock.assert_async().await; assert_eq!(decision, HookDecision::Proceed); - std::env::remove_var("FABRO_TEST_TOKEN"); } // --- TLS mode enforcement tests --- @@ -1044,6 +1064,7 @@ mod tests { &TlsMode::Verify, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; @@ -1061,6 +1082,7 @@ mod tests { &TlsMode::NoVerify, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; @@ -1086,6 +1108,7 @@ mod tests { &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), + &test_env(&[]), ) .await; diff --git a/lib/crates/fabro-util/src/env.rs b/lib/crates/fabro-util/src/env.rs new file mode 100644 index 000000000..3c2dcfebc --- /dev/null +++ b/lib/crates/fabro-util/src/env.rs @@ -0,0 +1,32 @@ +/// Abstraction over environment variable lookup. +/// +/// Production code uses [`SystemEnv`] which delegates to [`std::env::var`]. +/// Tests inject a [`TestEnv`] backed by a `HashMap` so they never mutate +/// process-global state. +pub trait Env: Send + Sync { + fn var(&self, key: &str) -> Result; +} + +/// Reads real process environment variables. +pub struct SystemEnv; + +impl Env for SystemEnv { + fn var(&self, key: &str) -> Result { + std::env::var(key) + } +} + +/// In-memory environment double — no process-global mutation. +/// +/// Intended for use in tests across the workspace. Unconditionally compiled +/// because it is trivial and has no external dependencies. +pub struct TestEnv(pub std::collections::HashMap); + +impl Env for TestEnv { + fn var(&self, key: &str) -> Result { + self.0 + .get(key) + .cloned() + .ok_or(std::env::VarError::NotPresent) + } +} diff --git a/lib/crates/fabro-util/src/lib.rs b/lib/crates/fabro-util/src/lib.rs index d8f8e87dd..5a6880219 100644 --- a/lib/crates/fabro-util/src/lib.rs +++ b/lib/crates/fabro-util/src/lib.rs @@ -1,4 +1,5 @@ pub mod check_report; +pub mod env; pub mod path; pub mod redact; pub mod run_log; diff --git a/lib/crates/fabro-util/src/telemetry/mod.rs b/lib/crates/fabro-util/src/telemetry/mod.rs index 3b2db9d5e..3d4e7f88a 100644 --- a/lib/crates/fabro-util/src/telemetry/mod.rs +++ b/lib/crates/fabro-util/src/telemetry/mod.rs @@ -71,7 +71,11 @@ impl Telemetry { } pub fn telemetry_level() -> TelemetryLevel { - match std::env::var("FABRO_TELEMETRY").as_deref() { + telemetry_level_from(&crate::env::SystemEnv) +} + +pub fn telemetry_level_from(env: &dyn crate::env::Env) -> TelemetryLevel { + match env.var("FABRO_TELEMETRY").as_deref() { Ok("off") => TelemetryLevel::Off, Ok("errors") => TelemetryLevel::Errors, Ok("all") => TelemetryLevel::All, @@ -88,28 +92,27 @@ pub fn telemetry_level() -> TelemetryLevel { #[cfg(test)] mod tests { use super::*; + use crate::env::TestEnv; use serde_json::json; + use std::collections::HashMap; #[test] fn telemetry_level_defaults_to_off_in_debug() { // In test builds (debug_assertions=true), default is Off - std::env::remove_var("FABRO_TELEMETRY"); - assert_eq!(telemetry_level(), TelemetryLevel::Off); + let env = TestEnv(HashMap::new()); + assert_eq!(telemetry_level_from(&env), TelemetryLevel::Off); } #[test] fn telemetry_level_parses_env_var() { - std::env::set_var("FABRO_TELEMETRY", "all"); - assert_eq!(telemetry_level(), TelemetryLevel::All); + let env = TestEnv(HashMap::from([("FABRO_TELEMETRY".into(), "all".into())])); + assert_eq!(telemetry_level_from(&env), TelemetryLevel::All); - std::env::set_var("FABRO_TELEMETRY", "errors"); - assert_eq!(telemetry_level(), TelemetryLevel::Errors); + let env = TestEnv(HashMap::from([("FABRO_TELEMETRY".into(), "errors".into())])); + assert_eq!(telemetry_level_from(&env), TelemetryLevel::Errors); - std::env::set_var("FABRO_TELEMETRY", "off"); - assert_eq!(telemetry_level(), TelemetryLevel::Off); - - // Clean up - std::env::remove_var("FABRO_TELEMETRY"); + let env = TestEnv(HashMap::from([("FABRO_TELEMETRY".into(), "off".into())])); + assert_eq!(telemetry_level_from(&env), TelemetryLevel::Off); } #[test] diff --git a/lib/crates/fabro-util/src/telemetry/panic.rs b/lib/crates/fabro-util/src/telemetry/panic.rs index 2503d33c8..ff52a593e 100644 --- a/lib/crates/fabro-util/src/telemetry/panic.rs +++ b/lib/crates/fabro-util/src/telemetry/panic.rs @@ -163,11 +163,14 @@ mod tests { #[test] fn report_panic_noop_when_telemetry_off() { - // Set telemetry off and verify report_panic doesn't panic itself. - std::env::set_var("FABRO_TELEMETRY", "off"); - // We can't easily create a PanicHookInfo, so test the individual pieces: - assert_eq!(super::super::telemetry_level(), TelemetryLevel::Off); - std::env::remove_var("FABRO_TELEMETRY"); + use crate::env::TestEnv; + use std::collections::HashMap; + // Verify telemetry_level_from returns Off without mutating process env. + let env = TestEnv(HashMap::from([("FABRO_TELEMETRY".into(), "off".into())])); + assert_eq!( + super::super::telemetry_level_from(&env), + TelemetryLevel::Off + ); } #[test]