chore(lint): make the next process-environment read fail the build

AGENTS.md

@@ -191,8 +191,27 @@ A change is not done without a test. Use the highest-fidelity tool per layer.
   which masks the empty-body bug the rule exists to catch.
 - Pin exact exit codes for the `CliError` contract; add a regression test for
   every fixed failure mode (exit code, message, missing-field tolerance).
-- Use `temp_env::with_var` for env-dependent tests — never the unsound
-  process-global `std::env::set_var` (`unsafe_code = "forbid"` bans it anyway).
+- **Nothing mutates the process environment, tests included.** Read it through
+  `mergify_core::env` (`var`, `var_os`, `var_non_empty`); give the code under
+  test one with `mergify_core::env::testing::with_vars`, which installs a
+  thread-local overlay and touches no global state. `clippy.toml` disallows
+  `std::env::var*`, `set_var` and `remove_var`, and `deny.toml` bans
+  `temp-env`.
+
+  Why, in one line: `setenv` racing `getenv` on another thread is a
+  use-after-free, libtest runs tests on many threads, and the concurrent
+  reader is usually not even ours (`std::env::temp_dir` behind every
+  `tempfile::tempdir()`, `Command::spawn` building a child's environment).
+  `crates/mergify-core/src/env.rs` carries the full argument and the
+  overlay's limits — read it before reaching for an exception.
+
+  Two consequences worth knowing before you write the test. An overlay **is**
+  the environment while installed, so a variable you do not list reads as
+  unset whatever the host exports: name what the case is about and nothing
+  else, and it behaves the same on a laptop and on a CI runner. And it covers
+  our reads only — a dependency, or a process you spawn, still sees the real
+  environment. Giving a **child** a variable is a different job with a
+  different tool, `Command::env`, and is untouched by any of this.
 
 ## Dependencies
 

Cargo.toml

@@ -72,7 +72,6 @@ tracing-subscriber = { version = "0.3", default-features = false, features = ["f
 unicode-width = "0.2"
 url = "2"
 insta = { version = "1", features = ["json", "redactions"] }
-temp-env = "0.3"
 wiremock = "0.6"
 
 [workspace.lints.rust]

clippy.toml

@@ -0,0 +1,27 @@
+# The process environment is read through `mergify_core::env` and
+# written nowhere. `crates/mergify-core/src/env.rs` carries the
+# argument; these entries carry the replacement.
+#
+# `set_var` / `remove_var` are `unsafe fn`, so the workspace's
+# `unsafe_code = "forbid"` is what actually stops a mutation — these
+# two lines are documentation, and a contributor who relaxes that lint
+# is unlocking more than they think. `set_current_dir` is the safe
+# process-global mutator, banned for the same reason: it races every
+# relative path in every other test thread.
+#
+# `std::env::args` and `current_exe` are not the environment and are
+# not listed. `temp_dir` is a `getenv` for `TMPDIR` and is unlisted
+# for a different reason: it takes no variable name, so there is
+# nothing to route through the funnel, and it is a read, which was
+# never the unsound half. A build script reads cargo's environment,
+# not the process's, and cannot depend on `mergify-core`;
+# `crates/mergify-cli/build.rs` allows the lint in place.
+disallowed-methods = [
+    { path = "std::env::var", reason = "read it through `mergify_core::env::var` / `var_non_empty`" },
+    { path = "std::env::var_os", reason = "read it through `mergify_core::env::var_os` / `var_os_non_empty`" },
+    { path = "std::env::vars", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" },
+    { path = "std::env::vars_os", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" },
+    { path = "std::env::set_var", reason = "nothing mutates the process environment; give the code under test one with `mergify_core::env::testing::with_vars`" },
+    { path = "std::env::remove_var", reason = "nothing mutates the process environment; give the code under test one with `mergify_core::env::testing::with_vars`" },
+    { path = "std::env::set_current_dir", reason = "process-global, so it races every relative path in every other test thread; take the directory as a parameter" },
+]

crates/mergify-auth/Cargo.toml

@@ -25,7 +25,6 @@ url = { workspace = true }
 mergify-core = { path = "../mergify-core", features = ["test-support"] }
 mergify-test-support = { path = "../mergify-test-support" }
 serde_json = { workspace = true }
-temp-env = { workspace = true }
 tempfile = { workspace = true }
 tokio = { workspace = true, features = ["rt-multi-thread"] }
 wiremock = { workspace = true }

crates/mergify-auth/src/browser.rs

@@ -231,7 +231,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn macos_opens_the_url_with_open() {
-        let command = temp_env::with_vars(
+        let command = mergify_core::env::testing::with_vars(
             [("SSH_CONNECTION", None::<&str>), ("SSH_TTY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device"),
         )
@@ -246,7 +246,7 @@ mod tests {
     #[cfg(all(unix, not(target_os = "macos")))]
     #[test]
     fn a_graphical_session_gets_xdg_open() {
-        let command = temp_env::with_vars(
+        let command = mergify_core::env::testing::with_vars(
             [("DISPLAY", Some(":0")), ("WAYLAND_DISPLAY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device"),
         )
@@ -263,7 +263,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn an_ssh_session_to_a_mac_opens_nothing() {
-        let opened = temp_env::with_vars(
+        let opened = mergify_core::env::testing::with_vars(
             [
                 ("SSH_CONNECTION", Some("10.0.0.1 52000 10.0.0.2 22")),
                 ("SSH_TTY", None),
@@ -281,7 +281,7 @@ mod tests {
     #[cfg(all(unix, not(target_os = "macos")))]
     #[test]
     fn a_headless_session_opens_nothing() {
-        let opened = temp_env::with_vars(
+        let opened = mergify_core::env::testing::with_vars(
             [("DISPLAY", None::<&str>), ("WAYLAND_DISPLAY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device").is_ok(),
         );

crates/mergify-auth/src/lib.rs

@@ -41,17 +41,18 @@ mod testing {
     /// Run `body` to completion with `MERGIFY_TOKEN` forced to
     /// `value`.
     ///
-    /// `temp_env` cannot wrap an `.await`, so the future is driven
-    /// inside the closure instead. Without this the wiring that
-    /// reads the variable is untestable, and untestable wiring is
-    /// wiring a future edit can delete with the suite still green:
-    /// asserting on the renderer alone proves only that the renderer
-    /// can print a note, never that anything asks it to.
+    /// The overlay is installed on this thread and the future is
+    /// driven on it, by a `current_thread` runtime built here.
+    /// Without this the wiring that reads the variable is
+    /// untestable, and untestable wiring is wiring a future edit can
+    /// delete with the suite still green: asserting on the renderer
+    /// alone proves only that the renderer can print a note, never
+    /// that anything asks it to.
     pub fn with_mergify_token<F: std::future::Future>(value: Option<&str>, body: F) -> F::Output {
         let runtime = tokio::runtime::Builder::new_current_thread()
             .enable_all()
             .build()
             .unwrap();
-        temp_env::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body))
+        mergify_core::env::testing::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body))
     }
 }

crates/mergify-auth/src/machine.rs

@@ -111,7 +111,7 @@ mod tests {
     // the variable instead.
     #[test]
     fn the_variables_are_the_fallback() {
-        let from_windows = temp_env::with_vars(
+        let from_windows = mergify_core::env::testing::with_vars(
             [
                 ("COMPUTERNAME", Some("WIN-BOX")),
                 ("HOSTNAME", Some("ignored")),
@@ -120,13 +120,13 @@ mod tests {
         );
         assert_eq!(from_windows.as_deref(), Some("WIN-BOX"));
 
-        let from_shell = temp_env::with_vars(
+        let from_shell = mergify_core::env::testing::with_vars(
             [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))],
             from_env,
         );
         assert_eq!(from_shell.as_deref(), Some("build-42"));
 
-        let from_nothing = temp_env::with_vars(
+        let from_nothing = mergify_core::env::testing::with_vars(
             [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)],
             from_env,
         );

crates/mergify-cli/Cargo.toml

@@ -57,7 +57,6 @@ url = { workspace = true }
 insta = { workspace = true }
 regex = { workspace = true }
 serde_yaml_ng = { workspace = true }
-temp-env = { workspace = true }
 tempfile = { workspace = true }
 tokio = { workspace = true, features = ["rt-multi-thread"] }
 wiremock = { workspace = true }

crates/mergify-cli/build.rs

@@ -14,6 +14,11 @@ fn main() {
     // Rebuild when the env var changes so a release rebuild after a
     // dev build actually picks up the new value.
     println!("cargo:rerun-if-env-changed=MERGIFY_RELEASE_VERSION");
+    // A build script's environment is cargo's, handed to it for this
+    // one invocation — not the process environment `clippy.toml`
+    // guards, and not reachable through `mergify_core::env`, which a
+    // build script cannot depend on.
+    #[allow(clippy::disallowed_methods)]
     let resolved = std::env::var("MERGIFY_RELEASE_VERSION")
         .ok()
         .filter(|v| !v.is_empty())

crates/mergify-cli/src/main.rs

@@ -8,7 +8,7 @@
 //! `<closest>`?" suggestion off clap's built-in Levenshtein
 //! distance.
 
-use std::env;
+use mergify_core::env;
 use std::io::IsTerminal;
 use std::path::PathBuf;
 use std::process::ExitCode;
@@ -53,7 +53,7 @@ mod self_update;
 const VERSION: &str = env!("MERGIFY_CLI_VERSION");
 
 fn main() -> ExitCode {
-    let argv: Vec<String> = env::args().skip(1).collect();
+    let argv: Vec<String> = std::env::args().skip(1).collect();
 
     // Test hook used by `test_binary_build.py` to verify the
     // wheel-installed binary produces UTF-8 output (especially on
@@ -2812,12 +2812,12 @@ enum ColorArg {
 /// `true` when `name` is exported to something other than the empty
 /// string, which is what <https://no-color.org> means by "present".
 ///
-/// The rule itself lives in `mergify_core::env::var_non_empty`, which
-/// is where it is documented and tested; spelling it out a second time
-/// here is how the color variables would drift away from every other
-/// variable this CLI reads.
+/// The rule itself lives in `mergify_core::env`, which is where it is
+/// documented and tested; spelling it out a second time here is how
+/// the color variables would drift away from every other variable
+/// this CLI reads.
 fn non_empty(name: &str) -> bool {
-    mergify_core::env::var_non_empty(name).is_some()
+    env::var_os_non_empty(name).is_some()
 }
 
 /// Whether the log subscriber may emit ANSI on stderr.
@@ -3984,8 +3984,9 @@ struct ScopesCliArgs {
     // `mergify_ci::scopes_detect::resolve_config_path` instead,
     // where empty correctly falls through to auto-detect. The
     // matching regression tests are
-    // `ci_scopes_parses_when_mergify_config_path_env_var_is_empty`
-    // (clap parse) and
+    // `no_argument_takes_its_value_from_the_environment` (asks the
+    // built `Command` whether *any* argument carries an `env`
+    // attribute, which covers this one) and
     // `resolve_config_path_treats_empty_env_var_as_unset`
     // (lower-level resolver).
     #[arg(long)]
@@ -4897,72 +4898,75 @@ mod tests {
         // or test the terminal the suite happens to run under.
     }
 
+    /// No argument anywhere in the tree may take its value from the
+    /// environment through clap's `env = "…"` attribute.
+    ///
+    /// Twice now that attribute broke a caller who exports the
+    /// variable empty. `gha-mergify-ci` sets `MERGIFY_CONFIG_PATH=""`
+    /// when the user pinned no path, and clap read the empty string
+    /// as a present-but-empty `--config`, aborting with "a value is
+    /// required for '--config'" (monorepo#33423). Same shape for
+    /// `MERGIFY_TEST_EXIT_CODE=""` and `--test-exit-code`: "cannot
+    /// parse integer from empty string". Env lookup belongs in the
+    /// resolver, where `mergify_core::env::var_non_empty` treats
+    /// empty as unset.
+    ///
+    /// This replaces two tests that each exported one variable empty
+    /// and parsed one argv. Asking the built `Command` covers every
+    /// argument rather than those two, and needs no process
+    /// environment to mutate.
     #[test]
-    fn ci_scopes_parses_when_mergify_config_path_env_var_is_empty() {
-        // Regression for monorepo#33423 / gha-mergify-ci:
-        // the action sets `MERGIFY_CONFIG_PATH=""` (empty) when
-        // the caller didn't pin a config path, expecting
-        // auto-detect. The previous `ScopesCliArgs::config`
-        // declaration used `env = "MERGIFY_CONFIG_PATH"` on
-        // clap's side, which interpreted the empty env value as
-        // a present-but-empty `--config` flag and exited parsing
-        // with `a value is required for '--config'`. The clap
-        // env hook has been dropped — env lookup lives inside
-        // `scopes_detect::resolve_config_path` where empty is
-        // correctly treated as unset. Pin that here so the hook
-        // can't sneak back in.
-        let parsed = temp_env::with_var("MERGIFY_CONFIG_PATH", Some(""), || {
-            CliRoot::try_parse_from([
-                "mergify".to_string(),
-                "ci".to_string(),
-                "scopes".to_string(),
-                "--write".to_string(),
-                "scopes.json".to_string(),
-            ])
-            .expect("argv parses with empty MERGIFY_CONFIG_PATH")
-        });
+    fn no_argument_takes_its_value_from_the_environment() {
+        fn walk(cmd: &clap::Command, path: &str, found: &mut Vec<String>) {
+            for arg in cmd.get_arguments() {
+                if let Some(var) = arg.get_env() {
+                    found.push(format!(
+                        "{path} {} <- {}",
+                        arg.get_id(),
+                        var.to_string_lossy()
+                    ));
+                }
+            }
+            for sub in cmd.get_subcommands() {
+                walk(sub, &format!("{path} {}", sub.get_name()), found);
+            }
+        }
+
+        let mut found = Vec::new();
+        walk(&CliRoot::command(), "mergify", &mut found);
+        assert!(found.is_empty(), "clap env hooks found: {found:#?}");
+    }
+
+    /// The observable half of the rule above, for the two arguments
+    /// it was reported on.
+    ///
+    /// The walk asks clap whether an `env = "…"` hook is declared,
+    /// which is the spelling that caused both regressions but not the
+    /// only one: `default_value_t = std::env::var(…).unwrap_or_default()`
+    /// or a `value_parser` that reads the environment reproduce it
+    /// exactly and declare no hook. This asserts what the user sees
+    /// instead. It needs no environment of its own — with the
+    /// variable unset, any of those spellings still surfaces a
+    /// present-but-empty value where `None` is required.
+    #[test]
+    fn an_omitted_flag_stays_omitted() {
+        let parsed = CliRoot::try_parse_from(["mergify", "ci", "scopes", "--write", "scopes.json"])
+            .expect("argv parses");
         let Dispatch::Native(NativeCommand::CiScopes(opts)) = dispatch_from_parsed(parsed) else {
             panic!("ci scopes must dispatch natively");
         };
-        // `--config` was never supplied; the empty env var must
-        // not surface as a value (which would change the
-        // downstream resolver's branch).
         assert!(opts.config.is_none(), "got: {:?}", opts.config);
-    }
 
-    #[test]
-    fn ci_junit_process_parses_when_mergify_test_exit_code_env_var_is_empty() {
-        // Second instance of the same class of regression as
-        // `ci_scopes_parses_when_…`: `gha-mergify-ci` exports
-        // `MERGIFY_TEST_EXIT_CODE=""` when the previous step
-        // didn't produce a runner exit code. Previously the clap
-        // `env = "MERGIFY_TEST_EXIT_CODE"` attribute on
-        // `--test-exit-code` tried to parse `""` as `i32` and
-        // exited parsing with `invalid value '' for
-        // '--test-exit-code': cannot parse integer from empty
-        // string`. The clap env hook has been dropped — env
-        // lookup lives in `junit_process::command::resolve_test_exit_code`
-        // where empty is correctly treated as `None`. Pin that
-        // here so the hook can't sneak back in.
-        let parsed = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || {
-            CliRoot::try_parse_from([
-                "mergify".to_string(),
-                "ci".to_string(),
-                "junit-process".to_string(),
-                "report.xml".to_string(),
-            ])
-            .expect("argv parses with empty MERGIFY_TEST_EXIT_CODE")
-        });
+        let parsed = CliRoot::try_parse_from(["mergify", "ci", "junit-process", "report.xml"])
+            .expect("argv parses");
         let Dispatch::Native(NativeCommand::CiJunitProcess(opts)) = dispatch_from_parsed(parsed)
         else {
             panic!("ci junit-process must dispatch natively");
         };
-        // `--test-exit-code` was never supplied; the empty env
-        // var must not surface as a value.
         assert!(
             opts.test_exit_code.is_none(),
             "got: {:?}",
-            opts.test_exit_code,
+            opts.test_exit_code
         );
     }