Fix: Remove std::env::set_var/remove_var from tests (#101)

Introduce an Env trait in fabro-util so tests can inject a HashMap-backed
TestEnv instead of mutating process-global environment variables, which
is unsafe since Rust 1.66+ and causes flakiness in concurrent tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

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",

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"

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

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<String> {
             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<String> {
 #[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]

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

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;
 

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<String, std::env::VarError>;
+}
+
+/// Reads real process environment variables.
+pub struct SystemEnv;
+
+impl Env for SystemEnv {
+    fn var(&self, key: &str) -> Result<String, std::env::VarError> {
+        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<String, String>);
+
+impl Env for TestEnv {
+    fn var(&self, key: &str) -> Result<String, std::env::VarError> {
+        self.0
+            .get(key)
+            .cloned()
+            .ok_or(std::env::VarError::NotPresent)
+    }
+}

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;

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]

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]