From 092133b95e073af50f8e67e1817604d724a4f97e Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:44:17 +0200 Subject: [PATCH 1/4] refactor(tui): resolve the color env overrides at the CLI entry point `Theme::detect()` read `NO_COLOR`, `FORCE_COLOR` and `CLICOLOR_FORCE` on every call, and it is called from production paths in `mergify-freeze`, `mergify-queue`, `mergify-events` and `mergify-stack`. So any test in any of those crates read the process environment, on whatever thread it happened to run on, just by rendering a themed line. That is the read half of the data race this stack is about: the tests in `mergify-ci`, `mergify-core`, `mergify-cli` and `mergify-stack` mutate the process environment while these read it. Those three reads could never change the answer inside a test binary anyway. `resolve_enabled` returns `false` for a `None` choice whatever they say, and only `mergify-cli`'s entry point ever records a choice, which the doc comment on `set_color_choice` already said. A test harness read three environment variables to compute a value it had already decided. `mergify-tui` now takes the resolved choice and never reads the environment. `--color always|never` still wins, `NO_COLOR` still beats `FORCE_COLOR` / `CLICOLOR_FORCE`, and `auto` with neither still defers to the TTY. The precedence moves to `resolve_color_choice` in the binary, next to the clap enum it maps from, which is also where the workspace's environment funnel lives. `mergify-tui` cannot use that funnel: it is `mergify-core`, and this crate deliberately stays dependency-light. Two behaviour fixes come with it, both in the same three variables: Exported-but-empty no longer counts as set. A workflow writing `NO_COLOR: ${{ inputs.no_color }}` with no input exports the empty string, and every `mergify` in that job lost color with no way back but `--color always`. says "present and not an empty string"; so does every other variable this CLI reads. The log stream obeys the same decision. `init_tracing` was choosing ANSI from `stderr().is_terminal()` alone, so `NO_COLOR=1` and `--color never` gave plain stdout and a `-vv` stream still full of escape sequences, which is the one output people redirect to a file. The remaining visible difference is *when* the variables are read: once at startup instead of at every `Theme::detect()`. Nothing mutates the environment mid-run, so the answer is the same either way. Co-Authored-By: Claude Opus 5 Change-Id: Ib099b7044861960ca2c9e85f443072914b333c20 --- AGENTS.md | 11 ++- README.md | 3 +- crates/mergify-cli/src/main.rs | 119 ++++++++++++++++++++++++--- crates/mergify-stack/src/progress.rs | 10 ++- crates/mergify-tui/src/lib.rs | 6 +- crates/mergify-tui/src/select.rs | 6 +- crates/mergify-tui/src/theme.rs | 110 ++++++++++--------------- 7 files changed, 175 insertions(+), 90 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9683d1cb..ab20e377 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,10 +122,13 @@ allowed — use an exhaustive `enum` match. - Commands emit results through `&mut dyn mergify_core::output::Output`, not `println!`. Call `emit` / `emit_json_value` once. - **Color goes through `mergify_tui::theme`**, never hardcoded SGR escapes. The - theme honors `--color ` (resolved once via - `set_color_choice`), then `NO_COLOR`, then `FORCE_COLOR`/`CLICOLOR_FORCE`, then - the TTY. Cursor-movement / erase escapes in the progress renderer are not color - and are fine. + precedence is `--color `, then `NO_COLOR`, then + `FORCE_COLOR`/`CLICOLOR_FORCE`, then the TTY, and it is resolved once at the + CLI entry point: `resolve_color_choice` in `main.rs` folds the three variables + into the `ColorChoice` it hands `set_color_choice`. `mergify-tui` itself reads + no environment variable, which is why a consumer crate's tests no longer touch + the environment just by rendering a themed line. Cursor-movement / erase + escapes in the progress renderer are not color and are fine. ```rust // GOOD diff --git a/README.md b/README.md index bab1769e..79090710 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,8 @@ These are accepted on every command: | `GITHUB_REPOSITORY` | Default `owner/repo` when `--repository` is omitted. | | `MERGIFY_API_URL` | API base URL (default `https://api.mergify.com`). | | `RUST_LOG` | Fine-grained log filtering; overrides `--verbose`. | -| `NO_COLOR` | Disable colored output. | +| `NO_COLOR` | Disable colored output. Must be set to a non-empty value; exported-but-empty counts as unset. | +| `FORCE_COLOR`, `CLICOLOR_FORCE` | Force colored output when it would otherwise be off. Same non-empty rule, and `NO_COLOR` wins over both. All three apply to `--color auto` only. | | `MERGIFY_INSTALL_DIR`, `MERGIFY_VERSION` | Install-script target directory / pinned version. | ## Exit codes diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index c918ff53..42c6daa4 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -688,8 +688,17 @@ fn detect_dispatch(argv: &[String]) -> Dispatch { }; // Resolve the color preference once, before any command builds a // theme via `Theme::detect`. - mergify_tui::set_color_choice(parsed.color.into()); - init_tracing(parsed.verbose, parsed.debug); + // Exported-but-empty is not set: a workflow writing + // `NO_COLOR: ${{ inputs.no_color }}` with no input exports the + // empty string, and says that does not + // count. Same rule as every other variable this CLI reads. + let color = resolve_color_choice( + parsed.color, + non_empty("NO_COLOR"), + non_empty("FORCE_COLOR") || non_empty("CLICOLOR_FORCE"), + ); + mergify_tui::set_color_choice(color); + init_tracing(parsed.verbose, parsed.debug, color); dispatch_from_parsed(parsed) } @@ -698,7 +707,7 @@ fn detect_dispatch(argv: &[String]) -> Dispatch { /// trace), with `--debug` flooring at debug; an explicit `RUST_LOG` /// overrides both. Only our own crates are raised — third-party deps /// stay at `warn` so `-vv` doesn't drown in hyper/reqwest noise. -fn init_tracing(verbose: u8, debug: bool) { +fn init_tracing(verbose: u8, debug: bool, color: mergify_tui::ColorChoice) { use tracing_subscriber::EnvFilter; let level = match verbose { @@ -717,7 +726,7 @@ fn init_tracing(verbose: u8, debug: bool) { let _ = tracing_subscriber::fmt() .with_env_filter(filter) .with_writer(std::io::stderr) - .with_ansi(std::io::stderr().is_terminal()) + .with_ansi(ansi_on_stderr(color)) .try_init(); } @@ -2800,13 +2809,56 @@ enum ColorArg { Never, } -impl From for mergify_tui::ColorChoice { - fn from(c: ColorArg) -> Self { - match c { - ColorArg::Auto => Self::Auto, - ColorArg::Always => Self::Always, - ColorArg::Never => Self::Never, - } +/// `true` when `name` is exported to something other than the empty +/// string, which is what 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. +fn non_empty(name: &str) -> bool { + mergify_core::env::var_non_empty(name).is_some() +} + +/// Whether the log subscriber may emit ANSI on stderr. +/// +/// The same decision `Theme::detect` makes for stdout, against +/// stderr, because that is where the logs go. Without this +/// `--color never` and `NO_COLOR` styled stdout and left the `-vv` +/// stream on stderr full of escapes, which is the one place a user +/// redirects to a file. +fn ansi_on_stderr(color: mergify_tui::ColorChoice) -> bool { + match color { + mergify_tui::ColorChoice::Always => true, + mergify_tui::ColorChoice::Never => false, + mergify_tui::ColorChoice::Auto => std::io::stderr().is_terminal(), + } +} + +/// Map `--color` to the choice `mergify-tui` records, folding in +/// `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE`. +/// +/// An explicit `--color always|never` wins over all three, which is +/// why only the `Auto` arms consult them. `NO_COLOR` beats the two +/// force flags, per . +/// +/// This lives here rather than in `mergify-tui` so that crate never +/// reads the environment: it is the whole reason a consumer crate's +/// test binary used to touch the environment just by rendering a +/// themed line. +fn resolve_color_choice( + arg: ColorArg, + no_color: bool, + force_color: bool, +) -> mergify_tui::ColorChoice { + use mergify_tui::ColorChoice; + + match arg { + ColorArg::Always => ColorChoice::Always, + ColorArg::Never => ColorChoice::Never, + ColorArg::Auto if no_color => ColorChoice::Never, + ColorArg::Auto if force_color => ColorChoice::Always, + ColorArg::Auto => ColorChoice::Auto, } } @@ -4800,6 +4852,51 @@ mod tests { ); } + #[test] + fn color_choice_folds_env_overrides_into_auto_only() { + use mergify_tui::ColorChoice; + + // `--color always|never` is the user being explicit; no env + // var may override it. + assert_eq!( + resolve_color_choice(ColorArg::Always, true, false), + ColorChoice::Always + ); + assert_eq!( + resolve_color_choice(ColorArg::Never, false, true), + ColorChoice::Never + ); + // Auto: NO_COLOR wins over FORCE_COLOR / CLICOLOR_FORCE. + assert_eq!( + resolve_color_choice(ColorArg::Auto, true, true), + ColorChoice::Never + ); + assert_eq!( + resolve_color_choice(ColorArg::Auto, false, true), + ColorChoice::Always + ); + // Auto with neither set stays Auto, i.e. defers to the TTY. + assert_eq!( + resolve_color_choice(ColorArg::Auto, false, false), + ColorChoice::Auto + ); + } + + #[test] + fn stderr_ansi_follows_the_resolved_color_choice() { + use mergify_tui::ColorChoice; + + // The log stream obeys the same decision as stdout: before + // this, `--color never` and `NO_COLOR` gave a plain stdout and + // a `-vv` stream still full of escapes. + assert!(ansi_on_stderr(ColorChoice::Always)); + assert!(!ansi_on_stderr(ColorChoice::Never)); + // `Auto` is deliberately not asserted: it is + // `stderr().is_terminal()`, which is whatever the harness was + // given, so an assertion here would either restate the branch + // or test the terminal the suite happens to run under. + } + #[test] fn ci_scopes_parses_when_mergify_config_path_env_var_is_empty() { // Regression for monorepo#33423 / gha-mergify-ci: diff --git a/crates/mergify-stack/src/progress.rs b/crates/mergify-stack/src/progress.rs index c3e28e0f..b4811a8f 100644 --- a/crates/mergify-stack/src/progress.rs +++ b/crates/mergify-stack/src/progress.rs @@ -109,8 +109,14 @@ pub struct Progress { impl Progress { /// Detect the output mode from stdout. Interactive (cursor /// redraw + spinner) only on a real terminal whose size we can - /// read. Color is delegated to [`Theme::detect`] (TTY-and- - /// `NO_COLOR`-aware, suppressed under tests). + /// read. + /// + /// Color follows that same interactivity, so it is narrower than + /// the recorded `--color` choice: a non-interactive `Progress` is + /// always plain, including under `--color always`. Only the + /// interactive branch consults [`Theme::detect`] (the recorded + /// choice, else the TTY; off outside the CLI entry point, so + /// tests are never colored). See the comment in the body. #[must_use] pub fn new() -> Self { let stdout = std::io::stdout(); diff --git a/crates/mergify-tui/src/lib.rs b/crates/mergify-tui/src/lib.rs index c419e99e..816cf067 100644 --- a/crates/mergify-tui/src/lib.rs +++ b/crates/mergify-tui/src/lib.rs @@ -11,8 +11,10 @@ //! Modules: //! //! - [`theme`]: [`Theme`] struct that wraps `anstyle::Style` with -//! TTY-and-`NO_COLOR`-aware enable/disable, plus a named-color -//! palette. The same closure-based emit code paths produce +//! choice-and-TTY-aware enable/disable, plus a named-color +//! palette. This crate reads no environment variables; the +//! `NO_COLOR` family reaches it folded into the recorded +//! [`ColorChoice`]. The same closure-based emit code paths produce //! styled output on a TTY and plain text everywhere else with no //! conditional branching at every write. //! - [`glyph`]: [`StyledGlyph`] — pairs a Unicode icon with the diff --git a/crates/mergify-tui/src/select.rs b/crates/mergify-tui/src/select.rs index 18bf47f2..20581614 100644 --- a/crates/mergify-tui/src/select.rs +++ b/crates/mergify-tui/src/select.rs @@ -67,9 +67,9 @@ pub fn fuzzy_select(prompt: &str, items: &[String], default: usize) -> io::Resul install_sigint_handler(); let colorful = ColorfulTheme::default(); let simple = SimpleTheme; - // Same color policy as every other renderer (`--color` override - // > `NO_COLOR` > `FORCE_COLOR`/`CLICOLOR_FORCE` > TTY), reused - // from `theme.rs` rather than re-derived. + // Same color policy as every other renderer — the choice the + // entry point recorded, else the TTY — reused from `theme.rs` + // rather than re-derived. let theme: &dyn Theme = if colors_enabled() { &colorful } else { &simple }; PICKER_ACTIVE.store(true, Ordering::SeqCst); let result = FuzzySelect::with_theme(theme) diff --git a/crates/mergify-tui/src/theme.rs b/crates/mergify-tui/src/theme.rs index 1f35bbd5..ed057732 100644 --- a/crates/mergify-tui/src/theme.rs +++ b/crates/mergify-tui/src/theme.rs @@ -1,4 +1,8 @@ -//! ANSI styling wrapped with TTY/`NO_COLOR` detection. +//! ANSI styling, enabled by the recorded [`ColorChoice`] and the TTY. +//! +//! This crate reads no environment variable of its own: the +//! `NO_COLOR` family arrives folded into that choice. See +//! [`set_color_choice`]. //! //! The intent is to write normal `format!` / `write!` code paths //! that emit styled output on an interactive terminal and produce @@ -15,8 +19,9 @@ use std::sync::OnceLock; use anstyle::AnsiColor; use anstyle::Style; -/// The user's `--color` preference. `Auto` defers to env vars and TTY -/// detection; `Always`/`Never` override both. +/// The user's resolved color preference. `Auto` defers to TTY +/// detection; `Always`/`Never` override it. The `NO_COLOR` family is +/// folded in by the caller of [`set_color_choice`], not here. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub enum ColorChoice { #[default] @@ -27,14 +32,23 @@ pub enum ColorChoice { static COLOR_CHOICE: OnceLock = OnceLock::new(); -/// Record the process-wide color preference (from `--color`), once, -/// at startup before any [`Theme::detect`]. Subsequent calls are -/// ignored, so a stray second call can't flip colors mid-run. +/// Record the process-wide color preference, once, at startup before +/// any [`Theme::detect`]. Subsequent calls are ignored, so a stray +/// second call can't flip colors mid-run. /// /// Calling this is also what makes color possible at all: until it /// does, [`Theme::detect`] reports disabled. Only the CLI entry point /// calls it, so a process that never went through `main` — a test /// harness, a doctest, an embedder — is never colored. +/// +/// The caller passes the *resolved* choice: `--color` with the +/// `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE` overrides already +/// folded in (`mergify-cli`'s `resolve_color_choice`). This crate +/// deliberately never reads the environment itself. Two reasons: +/// it keeps `mergify-tui` dependency-light and free of the +/// workspace's env funnel, and it means no consumer crate's test +/// binary reads the environment on a worker thread just by +/// rendering a themed line. pub fn set_color_choice(choice: ColorChoice) { let _ = COLOR_CHOICE.set(choice); } @@ -44,8 +58,8 @@ pub fn set_color_choice(choice: ColorChoice) { /// colors are enabled) or `Style::new()` (when disabled — emits /// nothing); `reset` mirrors that with `"\x1b[0m"` vs `""`. /// -/// Construct via [`Theme::detect`] for the production policy -/// (TTY-only, `NO_COLOR`-aware, off outside the CLI entry point). +/// Construct via [`Theme::detect`] for the production policy (the +/// recorded choice, else the TTY; off outside the CLI entry point). /// Tests that need to assert on styled output explicitly can pass /// `enabled = true` to [`Theme::new`]. pub struct Theme { @@ -77,11 +91,12 @@ impl Theme { /// 1. No [`set_color_choice`] yet ⇒ disabled. Only the CLI entry /// point records one, so this is a test harness or an embedder: /// it asserts on in-memory buffers and must not take a - /// dependency on the developer's terminal or environment. + /// dependency on the developer's terminal. /// 2. `--color always`/`never` (via [`set_color_choice`]) wins. - /// 3. Otherwise (`auto`): `NO_COLOR` forces off; `FORCE_COLOR` / - /// `CLICOLOR_FORCE` force on (e.g. through a pager or CI - /// viewer); else `stdout` must be a terminal. + /// 3. Otherwise (`auto`): `stdout` must be a terminal. + /// + /// `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE` are **not** read + /// here — see [`set_color_choice`]. #[must_use] pub fn detect() -> Self { Self::new(colors_enabled()) @@ -121,28 +136,15 @@ impl Theme { } /// Pure color decision, factored out of [`colors_enabled`] so the -/// precedence is unit-testable without touching global state, env, or -/// the real TTY. -fn resolve_enabled( - choice: Option, - no_color: bool, - force_color: bool, - is_tty: bool, -) -> bool { +/// precedence is unit-testable without touching global state or the +/// real TTY. +fn resolve_enabled(choice: Option, is_tty: bool) -> bool { match choice { Some(ColorChoice::Always) => true, // `None` is nobody having recorded a preference, which means // nobody is watching a terminal — see [`set_color_choice`]. None | Some(ColorChoice::Never) => false, - Some(ColorChoice::Auto) => { - if no_color { - false - } else if force_color { - true - } else { - is_tty - } - } + Some(ColorChoice::Auto) => is_tty, } } @@ -153,16 +155,7 @@ pub(crate) fn colors_enabled() -> bool { // *consumer* crate's tests were reading the developer's // environment after all. `FORCE_COLOR=1 cargo test` failed on the // escape sequences that leaked into asserted output. - let choice = COLOR_CHOICE.get().copied(); - let no_color = std::env::var_os("NO_COLOR").is_some(); - let force_color = - std::env::var_os("FORCE_COLOR").is_some() || std::env::var_os("CLICOLOR_FORCE").is_some(); - resolve_enabled( - choice, - no_color, - force_color, - std::io::stdout().is_terminal(), - ) + resolve_enabled(COLOR_CHOICE.get().copied(), std::io::stdout().is_terminal()) } #[cfg(test)] @@ -172,34 +165,17 @@ mod tests { #[test] fn color_precedence() { // No recorded choice: not the CLI, so nothing is colored — - // not even with a TTY and FORCE_COLOR both saying yes. This - // is what keeps a consumer crate's tests reproducible. - assert!(!resolve_enabled(None, false, true, true)); - // Explicit choice overrides everything. - assert!(resolve_enabled( - Some(ColorChoice::Always), - true, - false, - false - )); - assert!(!resolve_enabled( - Some(ColorChoice::Never), - false, - true, - true - )); - // Auto: NO_COLOR wins over FORCE_COLOR and TTY. - assert!(!resolve_enabled(Some(ColorChoice::Auto), true, true, true)); - // Auto: FORCE_COLOR turns it on without a TTY. - assert!(resolve_enabled(Some(ColorChoice::Auto), false, true, false)); - // Auto: otherwise follow the TTY. - assert!(resolve_enabled(Some(ColorChoice::Auto), false, false, true)); - assert!(!resolve_enabled( - Some(ColorChoice::Auto), - false, - false, - false - )); + // not even on a TTY. This is what keeps a consumer crate's + // tests reproducible. + assert!(!resolve_enabled(None, true)); + // Explicit choice overrides the TTY in both directions. + assert!(resolve_enabled(Some(ColorChoice::Always), false)); + assert!(!resolve_enabled(Some(ColorChoice::Never), true)); + // Auto follows the TTY. The env overrides that used to be + // decided here now reach us already folded into the choice — + // see `set_color_choice`. + assert!(resolve_enabled(Some(ColorChoice::Auto), true)); + assert!(!resolve_enabled(Some(ColorChoice::Auto), false)); } #[test] From 6f8c35c2baa06dd6998e4c7aa580711f315e3fcc Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:48:49 +0200 Subject: [PATCH 2/4] feat(core): read the environment through one funnel, with a hermetic test overlay `mergify_core::env` becomes the one place this workspace reads the process environment, and `env::testing::with_vars` the one way a test changes what it says. Nothing mutates the process environment any more. `temp_env` did, and that is undefined behaviour in a test binary. `setenv` became `unsafe` in Rust 1.80 because on Unix it can reallocate `environ` while another thread sits inside `getenv`: a use-after-free, not a race whose worst case is a wrong assertion. libtest runs its tests on many threads at once. The concurrent reader was never only ours, which is what makes serialising our own reads a non-answer. `std::env::temp_dir` is a `getenv`, so every `tempfile::tempdir()` is one, and there are 94 of those across the four crates that called `temp_env`. So is `Command::spawn` snapshotting `environ` for the child, and the CLI's tests spawn `git` constantly. `temp_env`'s lock serialises against other `temp_env` calls and against nothing else. So the environment is now read, never written. `var`, `var_os`, `var_non_empty` and `var_os_non_empty` consult a thread-local overlay first and the process environment otherwise; `env::testing::with_vars` installs one for the duration of a closure or a future. No global state changes, so tests that use it can run concurrently with anything. The overlay is a *replacement*, not a patch: while it is installed, a name the test did not list reads as unset. That is deliberate and it is worth more than the UB fix. It is what makes a test that reads `GITHUB_ACTIONS` behave the same on a laptop and on a GitHub Actions runner, which is the problem `mergify-ci`'s hand-maintained `CI_ENV_VARS` scrub list existed to paper over, with a comment admitting new detector inputs had to be added to it or their tests went flaky on CI. It is compiled unconditionally rather than behind `cfg(test)`. `cfg(test)` provably cannot work here: it is false whenever the crate is compiled as a dependency, so every consumer crate's tests would read the real environment anyway. `mergify_tui::theme` carries a comment about having been bitten by exactly that. Three things the module says about itself, because each one is a way to misuse it. It is the one place *this workspace* reads the environment, not the one place the process does: `tracing-subscriber` reads `RUST_LOG`, `dirs` reads `HOME`, `reqwest` reads the proxy variables, and no overlay reaches inside a dependency. The guard is `!Send`, so spawning a guarded future is a compile error, but work the body itself hands to another thread is not covered. And two overlay scopes must nest rather than overlap. `with_no_vars` installs a genuinely empty map rather than layering an empty list onto the enclosing overlay, which would have made it a no-op exactly where its name promises the most. Both accessors use `try_with`, since the overlay owns a `HashMap` and therefore registers a thread-local destructor: a `Drop` impl reading a variable while its thread tears down would otherwise panic where `std::env::var_os` could not, and a panic in a `Drop` during unwinding aborts. The `gh auth token` leg of both token resolvers becomes a parameter. Their tests used to point `PATH` at a nonexistent directory, and two of them wrote a shell script named `gh` into a temp dir and put *that* on `PATH`, which needs the environment to really change, for a grandchild process. A closure says the same thing, including the case worth pinning (`gh` echoing back the `mut_` value it was handed), without a shell script, a temp directory or a `cfg(unix)` gate. Nothing else moves yet: `mergify-ci`, `mergify-stack`, `mergify-cli` and `mergify-auth` still call `temp_env`, and the lint that bans it lands at the top of the stack. Co-Authored-By: Claude Opus 5 Change-Id: Ib777d5196b912ba69690a03554372442cdbc04be --- Cargo.lock | 2 - crates/mergify-config/Cargo.toml | 1 - crates/mergify-core/Cargo.toml | 1 - crates/mergify-core/src/auth.rs | 224 +++++++-------- crates/mergify-core/src/env.rs | 464 ++++++++++++++++++++++++++++++- 5 files changed, 568 insertions(+), 124 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9a347f08..6ad8804b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1823,7 +1823,6 @@ dependencies = [ "serde", "serde_json", "serde_yaml_ng", - "temp-env", "tempfile", "tokio", "url", @@ -1840,7 +1839,6 @@ dependencies = [ "reqwest", "serde", "serde_json", - "temp-env", "tempfile", "thiserror", "tokio", diff --git a/crates/mergify-config/Cargo.toml b/crates/mergify-config/Cargo.toml index b83e950b..b8941d5d 100644 --- a/crates/mergify-config/Cargo.toml +++ b/crates/mergify-config/Cargo.toml @@ -20,7 +20,6 @@ url = { workspace = true } [dev-dependencies] mergify-test-support = { path = "../mergify-test-support" } tempfile = { workspace = true } -temp-env = { workspace = true } tokio = { workspace = true } wiremock = { workspace = true } diff --git a/crates/mergify-core/Cargo.toml b/crates/mergify-core/Cargo.toml index ceb96d0d..e9898ae4 100644 --- a/crates/mergify-core/Cargo.toml +++ b/crates/mergify-core/Cargo.toml @@ -29,7 +29,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] -temp-env = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-core/src/auth.rs b/crates/mergify-core/src/auth.rs index 10329635..1c494cb5 100644 --- a/crates/mergify-core/src/auth.rs +++ b/crates/mergify-core/src/auth.rs @@ -127,7 +127,8 @@ pub fn resolve_mergify_token( audience: Audience, ) -> Result { let store = CredentialStore::discover(); - let resolved = resolve_mergify_token_with(explicit, api_url, audience, &store)?; + let resolved = + resolve_mergify_token_with(explicit, api_url, audience, &store, || gh_auth_token().ok())?; if let Some(notice) = deprecation_notice(resolved.source, audience) { warn_once(¬ice); } @@ -149,6 +150,7 @@ fn resolve_mergify_token_with( api_url: &Url, audience: Audience, store: &CredentialStore, + gh_token: impl FnOnce() -> Option, ) -> Result { if let Some(value) = explicit.filter(|s| !s.is_empty()) { return Ok(ResolvedToken { @@ -185,9 +187,7 @@ fn resolve_mergify_token_with( source: TokenSource::GitHubTokenEnv, }); } - if let Ok(token) = gh_auth_token() - && !token.is_empty() - { + if let Some(token) = gh_token().filter(|t| !t.is_empty()) { return Ok(ResolvedToken { token, source: TokenSource::GhCli, @@ -258,6 +258,22 @@ fn warn_once(message: &str) { /// next source tried. If nothing is left, the failure names the /// sources it skipped rather than the ones the user should have set. pub fn resolve_github_token(explicit: Option<&str>) -> Result { + resolve_github_token_with(explicit, || gh_auth_token().ok()) +} + +/// [`resolve_github_token`] with its `gh auth token` leg supplied by +/// the caller. +/// +/// The tests drive it with a closure rather than putting a fake `gh` +/// on `PATH`: `PATH` only reaches a child process by mutating this +/// one's environment, which nothing in this workspace does. It also +/// makes the cases that matter here — `gh` absent, `gh` echoing the +/// `mut_` value it was handed — expressible without a shell script, +/// a temp directory, or a `cfg(unix)` gate. +fn resolve_github_token_with( + explicit: Option<&str>, + gh_token: impl FnOnce() -> Option, +) -> Result { if let Some(value) = explicit.filter(|s| !s.is_empty()) { return Ok(value.to_string()); } @@ -294,9 +310,7 @@ pub fn resolve_github_token(explicit: Option<&str>) -> Result // the value out of the request: a GitHub Actions job exporting a // `mut_` token gets it handed straight back here. Check what // came out, not where it came from. - if let Ok(token) = gh_auth_token() - && !token.is_empty() - { + if let Some(token) = gh_token().filter(|t| !t.is_empty()) { if token.starts_with(MERGIFY_USER_TOKEN_PREFIX) { tracing::debug!( "`gh auth token` returned a Mergify user token, which GitHub cannot accept; \ @@ -457,6 +471,7 @@ fn parse_slug(url: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use crate::env; /// A store that cannot reach the developer's real keychain, /// optionally holding a credential for [`api_url`]. @@ -481,20 +496,36 @@ mod tests { Url::parse("https://api.mergify.com").unwrap() } - /// The chain, with no keychain anywhere near it. `None` for the - /// store means "this machine has no credential store at all". + /// The chain, with no keychain anywhere near it and no `gh`. + /// + /// A machine with an authenticated `gh` used to answer the last + /// step and make these tests assert on the wrong thing, which is + /// why so many of them pointed `PATH` at a nonexistent + /// directory. Injecting the step says the same thing without + /// needing the process environment to change. fn resolve( explicit: Option<&str>, audience: Audience, store: &CredentialStore, ) -> Result { - resolve_mergify_token_with(explicit, &api_url(), audience, store) + resolve_with_gh(explicit, audience, store, || None) + } + + /// [`resolve`] for a case that is about what `gh auth token` + /// answered. + fn resolve_with_gh( + explicit: Option<&str>, + audience: Audience, + store: &CredentialStore, + gh_token: impl FnOnce() -> Option, + ) -> Result { + resolve_mergify_token_with(explicit, &api_url(), audience, store, gh_token) } #[test] fn resolve_mergify_token_prefers_explicit_over_everything() { let (_dir, store) = store_with(Some("stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -512,7 +543,7 @@ mod tests { #[test] fn resolve_mergify_token_prefers_the_mergify_env_var_over_the_store() { let (_dir, store) = store_with(Some("stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -531,7 +562,7 @@ mod tests { #[test] fn the_stored_credential_beats_github_token() { let (_dir, store) = store_with(Some("mut_stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -550,7 +581,7 @@ mod tests { #[test] fn the_ci_audience_never_uses_the_stored_credential() { let (_dir, store) = store_with(Some("mut_stored")); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -566,7 +597,7 @@ mod tests { #[test] fn resolve_mergify_token_falls_back_to_github_env_when_nothing_is_stored() { let (_dir, store) = store_with(None); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), @@ -584,45 +615,26 @@ mod tests { #[test] fn a_credential_stored_for_another_deployment_is_not_used() { let (_dir, store) = store_with(Some("mut_stored")); - // `PATH` too: without it a developer machine with an - // authenticated `gh` answers the last step of the chain and - // the test asserts on the wrong thing. - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let other = Url::parse("https://mergify.internal.example/api").unwrap(); - let err = - resolve_mergify_token_with(None, &other, Audience::User, &store).unwrap_err(); - assert!( - err.to_string().contains("no Mergify credential"), - "got {err}" - ); - }, - ); + env::testing::with_no_vars(|| { + let other = Url::parse("https://mergify.internal.example/api").unwrap(); + let err = resolve_mergify_token_with(None, &other, Audience::User, &store, || None) + .unwrap_err(); + assert!( + err.to_string().contains("no Mergify credential"), + "got {err}" + ); + }); } #[test] fn resolve_mergify_token_error_names_the_command_that_fixes_it() { - // Forcing PATH to a directory with no `gh` keeps the test - // hermetic on machines that do have the GitHub CLI installed. let (_dir, store) = store_with(None); - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let err = resolve(None, Audience::User, &store).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("mergify auth login"), "got {msg:?}"); - assert!(msg.contains("MERGIFY_TOKEN"), "got {msg:?}"); - }, - ); + env::testing::with_no_vars(|| { + let err = resolve(None, Audience::User, &store).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("mergify auth login"), "got {msg:?}"); + assert!(msg.contains("MERGIFY_TOKEN"), "got {msg:?}"); + }); } // A `ci` command cannot be fixed by `auth login`; the message it @@ -630,19 +642,12 @@ mod tests { #[test] fn the_ci_audience_is_not_told_to_run_auth_login() { let (_dir, store) = store_with(None); - temp_env::with_vars( - [ - ("MERGIFY_TOKEN", None), - ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), - ], - || { - let err = resolve(None, Audience::ApplicationKey, &store).unwrap_err(); - let msg = err.to_string(); - assert!(!msg.contains("auth login"), "got {msg:?}"); - assert!(msg.contains("application key"), "got {msg:?}"); - }, - ); + env::testing::with_no_vars(|| { + let err = resolve(None, Audience::ApplicationKey, &store).unwrap_err(); + let msg = err.to_string(); + assert!(!msg.contains("auth login"), "got {msg:?}"); + assert!(msg.contains("application key"), "got {msg:?}"); + }); } #[test] @@ -689,7 +694,7 @@ mod tests { // wrapper itself must at least be wired to the chain. #[test] fn the_public_resolver_answers_the_chain() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", None), @@ -705,10 +710,10 @@ mod tests { #[test] fn overriding_env_var_reports_what_outranks_the_stored_credential() { - temp_env::with_var("MERGIFY_TOKEN", Some("env-mergify"), || { + env::testing::with_var("MERGIFY_TOKEN", Some("env-mergify"), || { assert_eq!(overriding_env_var(), Some("MERGIFY_TOKEN")); }); - temp_env::with_var("MERGIFY_TOKEN", None::<&str>, || { + env::testing::with_var("MERGIFY_TOKEN", None::<&str>, || { assert_eq!(overriding_env_var(), None); }); } @@ -719,13 +724,16 @@ mod tests { // about why. #[test] fn resolve_github_token_skips_a_mergify_user_token() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-github"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-github" + ); }, ); } @@ -735,14 +743,13 @@ mod tests { // set, for a reason they cannot see. #[test] fn resolve_github_token_says_why_it_skipped_the_mergify_token() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", None), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let err = resolve_github_token(None).unwrap_err(); + let err = resolve_github_token_with(None, || None).unwrap_err(); let message = err.to_string(); assert!( message.contains("GitHub does not accept"), @@ -764,14 +771,15 @@ mod tests { // the broken one. #[test] fn resolve_github_token_names_the_variable_it_actually_skipped() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("mut_from_auth_login")), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + let message = resolve_github_token_with(None, || None) + .unwrap_err() + .to_string(); assert!(message.starts_with("GITHUB_TOKEN holds"), "got {message:?}"); assert!( !message.contains("MERGIFY_TOKEN"), @@ -787,14 +795,15 @@ mod tests { #[test] fn resolve_github_token_names_both_variables_when_both_were_skipped() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_one")), ("GITHUB_TOKEN", Some("mut_two")), - ("PATH", Some("/nonexistent-directory-for-test")), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + let message = resolve_github_token_with(None, || None) + .unwrap_err() + .to_string(); assert!( message.starts_with("MERGIFY_TOKEN and GITHUB_TOKEN hold a"), "got {message:?}", @@ -803,35 +812,24 @@ mod tests { ); } - // A `gh` on `PATH` answering `auth token` with whatever the body - // prints. The real one echoes `$GITHUB_TOKEN` when that is set, - // which is the case worth reproducing. - #[cfg(unix)] - fn fake_gh(body: &str) -> tempfile::TempDir { - use std::os::unix::fs::PermissionsExt; - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("gh"); - std::fs::write(&path, format!("#!/bin/sh\n{body}\n")).unwrap(); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); - dir - } - // Skipping the variable is not enough on its own: `gh auth token` // prints `$GITHUB_TOKEN`, so a GitHub Actions job exporting a // `mut_` value gets the same token back through the fallback and // sends it to GitHub anyway. - #[cfg(unix)] #[test] fn resolve_github_token_does_not_let_gh_hand_back_the_skipped_variable() { - let gh = fake_gh("printf '%s' \"$GITHUB_TOKEN\""); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("mut_from_auth_login")), - ("PATH", Some(gh.path().to_str().unwrap())), ], || { - let message = resolve_github_token(None).unwrap_err().to_string(); + // What the real `gh auth token` prints when + // `GITHUB_TOKEN` is set: the variable, straight back. + let message = + resolve_github_token_with(None, || Some("mut_from_auth_login".to_string())) + .unwrap_err() + .to_string(); assert!( message.starts_with("GITHUB_TOKEN and `gh auth token` hold a"), "got {message:?}", @@ -848,19 +846,17 @@ mod tests { // The filter is on the value, not on the source: a GitHub token // from `gh` is still the answer when the variables hold nothing // usable. - #[cfg(unix)] #[test] fn resolve_github_token_still_takes_a_real_gh_token() { - let gh = fake_gh("printf 'ghp_from_gh_auth_login'"); - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("mut_from_auth_login")), ("GITHUB_TOKEN", None), - ("PATH", Some(gh.path().to_str().unwrap())), ], || { assert_eq!( - resolve_github_token(None).unwrap(), + resolve_github_token_with(None, || Some("ghp_from_gh_auth_login".to_string())) + .unwrap(), "ghp_from_gh_auth_login" ); }, @@ -872,7 +868,7 @@ mod tests { // the more surprising failure. #[test] fn resolve_github_token_still_honours_an_explicit_mergify_token() { - temp_env::with_var("GITHUB_TOKEN", Some("env-github"), || { + env::testing::with_var("GITHUB_TOKEN", Some("env-github"), || { assert_eq!( resolve_github_token(Some("mut_explicit")).unwrap(), "mut_explicit", @@ -887,7 +883,7 @@ mod tests { // if that divergence ever reaches the GitHub side. #[test] fn resolve_github_token_prefers_explicit_over_env() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), @@ -903,33 +899,39 @@ mod tests { #[test] fn resolve_github_token_falls_back_to_mergify_env() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", Some("env-mergify")), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-mergify"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-mergify" + ); }, ); } #[test] fn resolve_github_token_falls_back_to_github_env_when_mergify_unset() { - temp_env::with_vars( + env::testing::with_vars( [ ("MERGIFY_TOKEN", None), ("GITHUB_TOKEN", Some("env-github")), ], || { - assert_eq!(resolve_github_token(None).unwrap(), "env-github"); + assert_eq!( + resolve_github_token_with(None, || None).unwrap(), + "env-github" + ); }, ); } #[test] fn resolve_api_url_default() { - temp_env::with_var("MERGIFY_API_URL", None::<&str>, || { + env::testing::with_var("MERGIFY_API_URL", None::<&str>, || { let url = resolve_api_url(None).unwrap(); assert_eq!(url.as_str(), "https://api.mergify.com/"); }); @@ -937,7 +939,7 @@ mod tests { #[test] fn resolve_api_url_prefers_explicit() { - temp_env::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { + env::testing::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { let url = resolve_api_url(Some("https://explicit.example/")).unwrap(); assert_eq!(url.as_str(), "https://explicit.example/"); }); @@ -945,7 +947,7 @@ mod tests { #[test] fn resolve_api_url_uses_env_var_when_explicit_empty() { - temp_env::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { + env::testing::with_var("MERGIFY_API_URL", Some("https://from-env.example/"), || { let url = resolve_api_url(None).unwrap(); assert_eq!(url.as_str(), "https://from-env.example/"); }); @@ -953,7 +955,7 @@ mod tests { #[test] fn resolve_api_url_rejects_garbage() { - temp_env::with_var("MERGIFY_API_URL", None::<&str>, || { + env::testing::with_var("MERGIFY_API_URL", None::<&str>, || { let err = resolve_api_url(Some("not a url")).unwrap_err(); assert!(err.to_string().contains("invalid --api-url")); }); @@ -961,7 +963,7 @@ mod tests { #[test] fn resolve_repository_prefers_explicit() { - temp_env::with_var("GITHUB_REPOSITORY", Some("owner-from-env/repo"), || { + env::testing::with_var("GITHUB_REPOSITORY", Some("owner-from-env/repo"), || { assert_eq!( resolve_repository(Some("explicit/repo")).unwrap(), "explicit/repo", @@ -971,7 +973,7 @@ mod tests { #[test] fn resolve_repository_falls_back_to_env() { - temp_env::with_var("GITHUB_REPOSITORY", Some("owner/repo"), || { + env::testing::with_var("GITHUB_REPOSITORY", Some("owner/repo"), || { assert_eq!(resolve_repository(None).unwrap(), "owner/repo"); }); } diff --git a/crates/mergify-core/src/env.rs b/crates/mergify-core/src/env.rs index d651e383..1c837d44 100644 --- a/crates/mergify-core/src/env.rs +++ b/crates/mergify-core/src/env.rs @@ -1,12 +1,76 @@ -//! Environment-variable helpers shared across commands. +//! The one place *this workspace* reads the process environment, and +//! the only way a test changes what it says. +//! +//! Not the only place the process reads it: `tracing-subscriber` +//! reads `RUST_LOG`, `dirs` reads `HOME` / `XDG_CONFIG_HOME` / +//! `APPDATA` under `credentials_file()`, and `reqwest` reads +//! `HTTP_PROXY` / `NO_PROXY` when it builds a client. Those are +//! inside dependencies, so neither the lint nor a test overlay +//! reaches them: overlaying `HOME` compiles, runs, and changes +//! nothing. +//! +//! # Why this is not `std::env` +//! +//! Tests used to set up environment-dependent behaviour with +//! `temp_env`, which mutates the process environment for the duration +//! of a closure. `setenv` is `unsafe` since Rust 1.80 for a concrete +//! reason: on Unix it can reallocate `environ` while another thread is +//! inside `getenv`, and that is a use-after-free, not a race whose +//! worst case is a wrong value. A libtest binary runs its tests on +//! many threads at once, and the concurrent reader does not have to be +//! ours — `std::env::temp_dir` (every `tempfile::tempdir()`), +//! `Command::spawn` building a child's environment, and any dependency +//! that consults the environment are all `getenv` on another thread. +//! `temp_env`'s own lock cannot serialise against any of them; it only +//! serialises against other `temp_env` calls. +//! +//! So nothing in this workspace mutates the process environment, in +//! tests or anywhere else. Production reads it through [`var`], +//! [`var_os`] and [`var_non_empty`]; a test replaces what those return +//! with [`testing::with_vars`], which touches no global state at all. +//! +//! # What a test overlay does +//! +//! [`testing::with_vars`] installs a **replacement** environment on the +//! current thread. While it is installed, the process environment is +//! invisible: a name the test did not list reads as unset. That is the +//! point — it is what makes a test that reads `GITHUB_ACTIONS` behave +//! the same on a laptop and on a GitHub Actions runner, without a +//! hand-maintained list of variables to scrub first. +//! +//! The overlay covers *our* reads, and only ours. A dependency that +//! consults the environment (`dirs`, `keyring`, `reqwest`'s proxy +//! variables) and any process we spawn both see the real one, and +//! they see it silently — an overlay that sets `HOME` or `PATH` +//! changes nothing for them. Setting a variable for a child is a +//! different job with a different tool: `Command::env`, which touches +//! no shared state and is not affected by any of this. +//! +//! Keys match exactly. On Windows the real environment does not — +//! `std::env::var_os("PATH")` finds a `Path` — so an overlay on +//! `Path` leaves `var("PATH")` reading the host's. Nothing here reads +//! a name in a case other than the one it writes, so no call site can +//! tell the difference, and matching the platform would mean a +//! Windows-only normalisation that this suite never runs: the unit +//! tests are a Linux and macOS job, Windows only builds the binary. +//! +//! The overlay is thread-local, and it is compiled into release builds +//! rather than gated behind `cfg(test)`. `cfg(test)` cannot do the job: +//! it is false whenever this crate is compiled as a dependency, so +//! every consumer crate's tests would read the real environment +//! anyway. `mergify_tui::theme` carries the same note for the same +//! reason. The cost is one thread-local read per environment lookup on +//! a path that runs a handful of times per process. +//! +//! # The empty-string rule //! //! The CLI's resolver pattern (`flag → env → default`) treats the //! empty string as "not set", because callers in the wild — most //! notably the `gha-mergify-ci` GitHub Action — `export VAR=""` //! when no value is available. Inlining -//! `std::env::var(NAME).ok().filter(|s| !s.is_empty())` on every call -//! site looks innocuous but invites the same bug we've now hit -//! twice (monorepo#33423, `MERGIFY_CONFIG_PATH` and +//! `var(NAME).filter(|s| !s.is_empty())` on every call site looks +//! innocuous but invites the same bug we've now hit twice +//! (monorepo#33423, `MERGIFY_CONFIG_PATH` and //! `MERGIFY_TEST_EXIT_CODE`): a contributor adds clap's //! `env = "MERGIFY_FOO"` attribute on a flag instead, and clap's //! parser treats an empty env value as a present-but-empty flag @@ -16,7 +80,72 @@ //! → default` chain. Do **not** wire env vars through clap's //! `env = ...` attribute for any of the `MERGIFY_*` namespace. -use std::env; +use std::cell::Cell; +use std::cell::RefCell; +use std::collections::HashMap; +use std::ffi::OsStr; +use std::ffi::OsString; + +thread_local! { + /// The replacement environment installed by + /// [`testing::with_vars`], or `None` when the real process + /// environment is in effect. `Some(map)` is exhaustive: names + /// absent from `map` read as unset. + static OVERLAY: RefCell>> = const { RefCell::new(None) }; + + /// How many overlay scopes are open on this thread. Each scope + /// records the value it pushed and checks it again on the way + /// out, so two overlapping scopes are a panic rather than a + /// wrong answer — see `testing::Restore`'s `Drop`. + static DEPTH: Cell = const { Cell::new(0) }; +} + +/// Read an environment variable as an `OsString`. +/// +/// Returns the test overlay's value when one is installed on this +/// thread (see the module docs), otherwise the process environment's. +#[must_use] +pub fn var_os(name: &str) -> Option { + // `try_with`, not `with`: `OVERLAY` owns a `HashMap` and so + // registers a thread-local destructor, and `with` panics once + // that has run. A `Drop` impl reading a variable while its thread + // tears down would abort the process, which `std::env::var_os` + // never did. No overlay can exist at that point anyway. + OVERLAY + .try_with(|slot| { + slot.borrow().as_ref().map(|map| { + // An installed overlay *is* the environment: a name + // it does not carry reads as unset. + map.get(OsStr::new(name)).cloned() + }) + }) + .unwrap_or(None) + .unwrap_or_else(|| { + // The single sanctioned process-environment read in + // the workspace: every other caller goes through the + // functions above. + #[allow(clippy::disallowed_methods)] + std::env::var_os(name) + }) +} + +/// Read an environment variable as a `String`. +/// +/// A value that is not valid UTF-8 reads as unset, matching +/// `std::env::var(name).ok()`. +#[must_use] +pub fn var(name: &str) -> Option { + var_os(name).and_then(|value| value.into_string().ok()) +} + +/// [`var_non_empty`] for a value that need not be UTF-8 — the editor +/// chain (`GIT_EDITOR` / `VISUAL` / `EDITOR`) hands its value +/// straight to a `Command`, so decoding it would only be to re-encode +/// it. +#[must_use] +pub fn var_os_non_empty(name: &str) -> Option { + var_os(name).filter(|value| !value.is_empty()) +} /// Read an environment variable and return its value if it's set /// to a non-empty string. Unset or empty both collapse to `None`. @@ -26,7 +155,228 @@ use std::env; /// command. See the module doc for the empty-as-unset rationale. #[must_use] pub fn var_non_empty(name: &str) -> Option { - env::var(name).ok().filter(|s| !s.is_empty()) + var(name).filter(|value| !value.is_empty()) +} + +/// Give a test environment-dependent behaviour without mutating the +/// process environment. +/// +/// Compiled unconditionally, not behind `cfg(test)` or a feature — +/// see the module docs for why that is the only thing that works. +pub mod testing { + use std::collections::HashMap; + use std::ffi::OsStr; + use std::ffi::OsString; + use std::future::Future; + use std::marker::PhantomData; + use std::rc::Rc; + + use super::DEPTH; + use super::OVERLAY; + + /// Restores the enclosing overlay (usually `None`) when the scope + /// ends, including while a panic unwinds. + /// + /// Deliberately `!Send` (the `Rc` marker). The overlay lives on + /// the thread that installed it, so a guard that reached another + /// thread would restore the wrong thread's slot and strand the + /// overlay on the original one — permanently, since nothing else + /// clears it. Making the guard unsendable makes the future + /// returned by [`with_vars_async`] unsendable too, so + /// `tokio::spawn`-ing it is a compile error rather than a silent + /// leak. + struct Restore { + previous: Option>, + /// The value this scope pushed onto `DEPTH`, checked again on + /// the way out. + depth: u64, + _unsend: PhantomData>, + } + + impl Restore { + /// Make `map` this thread's environment until the returned + /// guard drops. + fn install_map(map: HashMap) -> Self { + let depth = DEPTH.with(|open| { + let pushed = open.get() + 1; + open.set(pushed); + pushed + }); + Self { + previous: OVERLAY.with_borrow_mut(|slot| slot.replace(map)), + depth, + _unsend: PhantomData, + } + } + } + + impl Drop for Restore { + fn drop(&mut self) { + // Ignore a thread already past its destructors: there is + // nothing left to restore, and panicking in a `Drop` + // during unwinding aborts. + let _ = OVERLAY.try_with(|slot| *slot.borrow_mut() = self.previous.take()); + let Ok(innermost) = DEPTH.try_with(|open| { + let innermost = open.get(); + open.set(self.depth - 1); + innermost + }) else { + return; + }; + // Never report while already unwinding: a panic inside a + // `Drop` during a panic aborts the process. + if std::thread::panicking() { + return; + } + // Scopes have to close in the order they opened, and the + // sync helpers cannot do otherwise: the guard is a local. + // `join!` of two `with_vars_async` on one thread can — + // each future holds its guard across an await, so the + // first to finish restores the state saved before the + // second, and the second resumes without its variables. + // Restoring `previous` above has already done that damage + // by the time we get here; what this turns into a failure + // is the silence. + assert_eq!( + innermost, self.depth, + "environment overlay scopes closed out of order: two \ + overlays overlap on this thread. Wrap the `join!`, \ + do not wrap each arm." + ); + } + } + + fn install(vars: I) -> Restore + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + { + // Nesting layers onto the enclosing overlay rather than + // replacing it, so an inner `with_var` reads like the + // process-environment version it replaces. + let mut map = OVERLAY.with_borrow(|slot| slot.clone().unwrap_or_default()); + for (name, value) in vars { + match value { + Some(value) => { + map.insert(name.as_ref().to_os_string(), value.as_ref().to_os_string()); + } + None => { + map.remove(name.as_ref()); + } + } + } + Restore::install_map(map) + } + + /// Run `f` with `vars` as the entire environment, as far as + /// [`super::var`] and friends are concerned. + /// + /// A `None` value means "unset", which only matters when nesting: + /// at the outermost level every name not listed is already unset. + pub fn with_vars(vars: I, f: F) -> R + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + F: FnOnce() -> R, + { + let _restore = install(vars); + f() + } + + /// The empty environment: every name reads as unset. + /// + /// What a case whose whole point is "none of this is configured" + /// wants. Unlike [`with_vars`], this does *not* inherit an + /// enclosing overlay — an empty `with_vars` would be a no-op + /// inside one, which is the opposite of what the name promises. + pub fn with_no_vars(f: F) -> R + where + F: FnOnce() -> R, + { + let _restore = Restore::install_map(HashMap::new()); + f() + } + + /// [`with_no_vars`] for an `async` body. + pub async fn with_no_vars_async(future: Fut) -> Fut::Output + where + Fut: Future, + { + let _restore = Restore::install_map(HashMap::new()); + future.await + } + + /// [`with_vars`] for a single variable. + pub fn with_var(name: K, value: Option, f: F) -> R + where + K: AsRef, + V: AsRef, + F: FnOnce() -> R, + { + with_vars([(name, value)], f) + } + + /// [`with_vars`] for an `async` body. + /// + /// The overlay lives on the thread that polls `future`. The + /// returned future is `!Send`, so `tokio::spawn`ing *it* is a + /// compile error rather than a silent read of the real + /// environment. + /// + /// Work the body hands to another thread is a different matter + /// and is not guarded: `spawn_blocking`, `std::thread::spawn`, + /// and `tokio::spawn` / `JoinSet::spawn` on a `multi_thread` + /// runtime all run where there is no overlay. Every + /// `#[tokio::test]` here is the default `current_thread` flavor, + /// where a spawned task stays on this thread and does see it; + /// adding `flavor = "multi_thread"` to a test that spawns under + /// an overlay would silently start reading the host's + /// environment. Such a test needs the value passed in. + /// + /// # Panics + /// + /// Guards must also nest. Two overlay scopes overlapping on one + /// thread (`join!` of two `with_vars_async`) would see each + /// other's variables and restore in the wrong order, so the + /// second scope to close panics instead. Wrap the `join!`, don't + /// wrap each arm. + pub async fn with_vars_async(vars: I, future: Fut) -> Fut::Output + where + I: IntoIterator)>, + K: AsRef, + V: AsRef, + Fut: Future, + { + let _restore = install(vars); + future.await + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + #[should_panic(expected = "closed out of order")] + fn overlapping_scopes_panic_instead_of_answering_wrongly() { + // What `join!`-ing two `with_vars_async` arms does, with + // the awaits taken out: the outer guard drops first. + let outer = install([("MERGIFY_TEST_A", Some("outer"))]); + let inner = install([("MERGIFY_TEST_B", Some("inner"))]); + drop(outer); + drop(inner); + } + + #[test] + fn nesting_in_order_does_not_panic() { + let outer = install([("MERGIFY_TEST_A", Some("outer"))]); + let inner = install([("MERGIFY_TEST_B", Some("inner"))]); + drop(inner); + drop(outer); + assert_eq!(super::super::var("MERGIFY_TEST_A"), None); + } + } } #[cfg(test)] @@ -35,7 +385,7 @@ mod tests { #[test] fn returns_some_for_non_empty_value() { - let got = temp_env::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || { + let got = testing::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || { var_non_empty("MERGIFY_TEST_HELPER_X") }); assert_eq!(got.as_deref(), Some("hello")); @@ -46,7 +396,7 @@ mod tests { // The whole point of this helper: an empty env var is // treated as if it were not set. Regression-prone enough // that we pin it explicitly. - let got = temp_env::with_var("MERGIFY_TEST_HELPER_Y", Some(""), || { + let got = testing::with_var("MERGIFY_TEST_HELPER_Y", Some(""), || { var_non_empty("MERGIFY_TEST_HELPER_Y") }); assert_eq!(got, None); @@ -54,9 +404,105 @@ mod tests { #[test] fn returns_none_when_unset() { - let got = temp_env::with_var_unset("MERGIFY_TEST_HELPER_Z", || { + let got = testing::with_var("MERGIFY_TEST_HELPER_Z", None::<&str>, || { var_non_empty("MERGIFY_TEST_HELPER_Z") }); assert_eq!(got, None); } + + #[test] + fn var_os_non_empty_treats_empty_as_unset() { + // Same empty-as-unset rule as `var_non_empty`, on the path + // that keeps the value an `OsString`. + testing::with_vars( + [("MERGIFY_TEST_A", Some("x")), ("MERGIFY_TEST_B", Some(""))], + || { + assert_eq!(var_os_non_empty("MERGIFY_TEST_A"), Some("x".into())); + assert_eq!(var_os_non_empty("MERGIFY_TEST_B"), None); + assert_eq!(var_os_non_empty("MERGIFY_TEST_C"), None); + }, + ); + } + + #[test] + fn with_no_vars_is_the_empty_environment() { + assert_eq!(testing::with_no_vars(|| var("PATH")), None); + } + + #[test] + fn with_no_vars_does_not_inherit_an_enclosing_overlay() { + // The trap this exists to avoid: `with_vars` layers onto the + // enclosing overlay, so an *empty* `with_vars` nested inside + // one would keep everything and scrub nothing. + testing::with_var("MERGIFY_TEST_A", Some("outer"), || { + testing::with_no_vars(|| assert_eq!(var("MERGIFY_TEST_A"), None)); + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("outer")); + }); + } + + #[test] + fn overlay_hides_the_process_environment() { + // The property the old `temp_env` scrub lists existed to + // fake: a variable the test did not list is unset, whatever + // the host exports. `PATH` is the likeliest name to be + // exported, but nothing guarantees it — a process started + // with a cleared environment has none — so read the host's + // value rather than requiring one. + let host_path = var("PATH"); + let got = testing::with_var("MERGIFY_TEST_HELPER_X", Some("hello"), || var("PATH")); + assert_eq!(got, None, "an installed overlay is the whole environment"); + assert_eq!( + var("PATH"), + host_path, + "outside an overlay the real environment is visible again" + ); + } + + #[test] + fn nested_overlays_layer_and_unwind() { + testing::with_vars( + [ + ("MERGIFY_TEST_A", Some("outer")), + ("MERGIFY_TEST_B", Some("b")), + ], + || { + testing::with_vars( + [("MERGIFY_TEST_A", Some("inner")), ("MERGIFY_TEST_B", None)], + || { + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("inner")); + assert_eq!(var("MERGIFY_TEST_B"), None); + }, + ); + assert_eq!(var("MERGIFY_TEST_A").as_deref(), Some("outer")); + assert_eq!(var("MERGIFY_TEST_B").as_deref(), Some("b")); + }, + ); + assert_eq!(var("MERGIFY_TEST_A"), None); + } + + #[test] + fn overlay_is_restored_after_a_panic() { + let panicked = std::panic::catch_unwind(|| { + testing::with_var("MERGIFY_TEST_A", Some("x"), || panic!("boom")); + }); + assert!(panicked.is_err()); + // The overlay's own variable, not a host one: `PATH` reads as + // unset both under a leaked overlay and on a host that does + // not export it, so it cannot tell those two apart. + assert_eq!( + var("MERGIFY_TEST_A"), + None, + "the overlay outlived the panic that unwound through it" + ); + } + + #[tokio::test] + async fn overlay_spans_await_points() { + let got = testing::with_vars_async([("MERGIFY_TEST_A", Some("x"))], async { + tokio::task::yield_now().await; + var("MERGIFY_TEST_A") + }) + .await; + assert_eq!(got.as_deref(), Some("x")); + } } From 89eaa4a7e88b36576dd5e7e5e39c5852f6e32451 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:52:39 +0200 Subject: [PATCH 3/4] refactor(ci): read the CI environment through the funnel `mergify-ci` is where most of the process-environment mutation lived: 27 direct `temp_env` calls plus the 69 that went through `testing::with_ci_env`. All of them now install a `mergify_core::env` overlay instead, and the production side reads through the same module. `CI_ENV_VARS` goes with them, and `with_ci_env` with it. That helper existed to merge a list of 43 variable names to unset into each case's overrides, carrying the instruction to "keep this list aligned with every `env::var(...)` call across `detector::*`; new detector helpers must add their inputs here or their tests will be flaky on CI". An overlay is exhaustive, so a name a test does not list is unset whatever the host exports, and there is nothing left for the list to be aligned with. The wrapper had nothing left to wrap. The call sites say `env::testing::with_vars` / `with_no_vars` directly, which is what they now mean. `detector::non_empty_env` goes too: once the reads route through `mergify_core::env` it was a second name for `env::var_non_empty`, which the same file calls. No behaviour changes. `env::var` returns `Option` where `std::env::var` returned `Result`, so `== Ok("true")` becomes `== Some("true")` and the `.ok().filter(|s| !s.is_empty())` chains collapse into `var_non_empty`, which is what they spelled out. Not done here, and worth naming: seven of those `== Some("true")` sites re-derive what `detector::get_ci_provider()` already answers, and the crate would be better served by building one CI snapshot at each command entry point and passing it down, with no ambient read at all. That is a different change with different risk, since the two precedences do not agree today, and it is not a prerequisite for removing the undefined behaviour. Co-Authored-By: Claude Opus 5 Change-Id: I668395af2157ffd73b41a70834a9d77dd504a2db --- Cargo.lock | 2 - crates/mergify-ci/Cargo.toml | 1 - crates/mergify-ci/src/detector.rs | 196 +++++++++--------- crates/mergify-ci/src/git_refs.rs | 49 ++--- crates/mergify-ci/src/github_event.rs | 10 +- crates/mergify-ci/src/github_output.rs | 14 +- .../mergify-ci/src/junit_process/command.rs | 68 +++--- crates/mergify-ci/src/junit_process/spans.rs | 22 +- crates/mergify-ci/src/junit_process/split.rs | 5 +- crates/mergify-ci/src/queue_info.rs | 5 +- crates/mergify-ci/src/scopes_detect/mod.rs | 32 ++- .../mergify-ci/src/scopes_detect/outputs.rs | 11 +- crates/mergify-ci/src/scopes_send.rs | 19 +- crates/mergify-ci/src/testing.rs | 121 ----------- crates/mergify-ci/src/tests_quarantine.rs | 6 +- crates/mergify-ci/src/tests_show.rs | 6 +- 16 files changed, 212 insertions(+), 355 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ad8804b..257a0689 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1772,7 +1772,6 @@ dependencies = [ "serde", "serde_json", "serde_yaml_ng", - "temp-env", "tempfile", "tokio", "url", @@ -3100,7 +3099,6 @@ version = "0.3.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "96374855068f47402c3121c6eed88d29cb1de8f3ab27090e273e420bdabcf050" dependencies = [ - "futures", "parking_lot", ] diff --git a/crates/mergify-ci/Cargo.toml b/crates/mergify-ci/Cargo.toml index 4bcb2609..562bc9eb 100644 --- a/crates/mergify-ci/Cargo.toml +++ b/crates/mergify-ci/Cargo.toml @@ -30,7 +30,6 @@ url = { workspace = true } [dev-dependencies] mergify-test-support = { path = "../mergify-test-support" } tempfile = { workspace = true } -temp-env = { workspace = true, features = ["async_closure"] } tokio = { workspace = true } wiremock = { workspace = true } diff --git a/crates/mergify-ci/src/detector.rs b/crates/mergify-ci/src/detector.rs index b2e020c8..56001a1a 100644 --- a/crates/mergify-ci/src/detector.rs +++ b/crates/mergify-ci/src/detector.rs @@ -6,7 +6,7 @@ //! Rust commands are mirrored; the rest stays in Python until its //! command is ported. -use std::env; +use mergify_core::env; use mergify_core::CliError; use mergify_core::auth; @@ -38,16 +38,16 @@ impl CIProvider { #[must_use] pub fn get_ci_provider() -> Option { - if env::var("JENKINS_URL").is_ok_and(|v| !v.is_empty()) { + if env::var_non_empty("JENKINS_URL").is_some() { return Some(CIProvider::Jenkins); } - if env::var("GITHUB_ACTIONS").as_deref() == Ok("true") { + if env::var("GITHUB_ACTIONS").as_deref() == Some("true") { return Some(CIProvider::GithubActions); } - if env::var("CIRCLECI").as_deref() == Ok("true") { + if env::var("CIRCLECI").as_deref() == Some("true") { return Some(CIProvider::CircleCi); } - if env::var("BUILDKITE").as_deref() == Ok("true") { + if env::var("BUILDKITE").as_deref() == Some("true") { return Some(CIProvider::Buildkite); } None @@ -58,7 +58,7 @@ pub fn get_ci_provider() -> Option { /// repository URL into ``owner/repo``. Returns ``None`` when the var /// is unset or the value doesn't parse. fn get_github_repository_from_env(env_name: &str) -> Option { - let raw = env::var(env_name).ok()?; + let raw = env::var(env_name)?; parse_repository_url(&raw) } @@ -151,7 +151,7 @@ pub fn split_owner_repo(value: &str) -> Result<(&str, &str), CliError> { #[must_use] pub fn get_github_repository() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => env::var("GITHUB_REPOSITORY").ok().filter(|s| !s.is_empty()), + CIProvider::GithubActions => env::var_non_empty("GITHUB_REPOSITORY"), CIProvider::CircleCi => get_github_repository_from_env("CIRCLE_REPOSITORY_URL"), CIProvider::Jenkins => get_github_repository_from_env("GIT_URL"), CIProvider::Buildkite => get_github_repository_from_env("BUILDKITE_REPO"), @@ -199,9 +199,13 @@ pub fn get_github_pull_request_number() -> Result, CliError> { match get_ci_provider() { Some(CIProvider::GithubActions) => read_github_event_pull_request_number(), Some(CIProvider::Buildkite) => match env::var("BUILDKITE_PULL_REQUEST") { - Ok(pr) if !pr.is_empty() && pr != "false" => pr.parse::().map(Some).map_err(|e| { - CliError::Configuration(format!("BUILDKITE_PULL_REQUEST is not an integer: {e}")) - }), + Some(pr) if !pr.is_empty() && pr != "false" => { + pr.parse::().map(Some).map_err(|e| { + CliError::Configuration(format!( + "BUILDKITE_PULL_REQUEST is not an integer: {e}" + )) + }) + } _ => Ok(None), }, _ => Ok(None), @@ -216,7 +220,7 @@ fn read_github_event_pull_request_number() -> Result, CliError> { // [`read_github_event_pull_request_head_sha`]) stays lenient // because every one of its callers has somewhere to go without // an answer. - let Some(event_path) = env::var("GITHUB_EVENT_PATH").ok().filter(|s| !s.is_empty()) else { + let Some(event_path) = env::var_non_empty("GITHUB_EVENT_PATH") else { return Ok(None); }; let content = match std::fs::read_to_string(&event_path) { @@ -276,8 +280,8 @@ pub fn get_github_pull_request_head_sha() -> Option { // Buildkite and CircleCI both build a pull request from its head // commit, so their revision var *is* that head (`git_refs` reads // `BUILDKITE_COMMIT` for the same purpose). - CIProvider::Buildkite => non_empty_env("BUILDKITE_COMMIT"), - CIProvider::CircleCi => non_empty_env("CIRCLE_SHA1"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_COMMIT"), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_SHA1"), CIProvider::Jenkins => None, }?; is_sha1_object_name(&sha).then_some(sha) @@ -310,19 +314,18 @@ pub fn get_pipeline_name() -> Option { CIProvider::Buildkite => "BUILDKITE_PIPELINE_SLUG", CIProvider::CircleCi => return None, }; - non_empty_env(var) + env::var_non_empty(var) } /// `cicd.pipeline.task.name` — the job within a pipeline. #[must_use] pub fn get_job_name() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => non_empty_env("GITHUB_JOB"), - CIProvider::CircleCi => non_empty_env("CIRCLE_JOB"), - CIProvider::Jenkins => non_empty_env("JOB_NAME"), - CIProvider::Buildkite => { - non_empty_env("BUILDKITE_LABEL").or_else(|| non_empty_env("BUILDKITE_STEP_KEY")) - } + CIProvider::GithubActions => env::var_non_empty("GITHUB_JOB"), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_JOB"), + CIProvider::Jenkins => env::var_non_empty("JOB_NAME"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_LABEL") + .or_else(|| env::var_non_empty("BUILDKITE_STEP_KEY")), } } @@ -334,10 +337,10 @@ pub fn get_head_ref_name() -> Option { // GitHub Actions sets `GITHUB_HEAD_REF` only on PR // events. Fall back to `GITHUB_REF_NAME` everywhere // else (the bare branch name, not `/merge`). - non_empty_env("GITHUB_HEAD_REF").or_else(|| non_empty_env("GITHUB_REF_NAME")) + env::var_non_empty("GITHUB_HEAD_REF").or_else(|| env::var_non_empty("GITHUB_REF_NAME")) } - CIProvider::CircleCi => non_empty_env("CIRCLE_BRANCH"), - CIProvider::Jenkins => non_empty_env("GIT_BRANCH").map(|raw| { + CIProvider::CircleCi => env::var_non_empty("CIRCLE_BRANCH"), + CIProvider::Jenkins => env::var_non_empty("GIT_BRANCH").map(|raw| { // Jenkins' Git plugin sets `GIT_BRANCH` to // `/` (or `refs/heads/` when // the job's configured for a refspec). Strip the @@ -350,7 +353,7 @@ pub fn get_head_ref_name() -> Option { } raw }), - CIProvider::Buildkite => non_empty_env("BUILDKITE_BRANCH"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_BRANCH"), } } @@ -358,9 +361,9 @@ pub fn get_head_ref_name() -> Option { #[must_use] pub fn get_base_ref_name() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => non_empty_env("GITHUB_BASE_REF"), - CIProvider::Jenkins => non_empty_env("CHANGE_TARGET"), - CIProvider::Buildkite => non_empty_env("BUILDKITE_PULL_REQUEST_BASE_BRANCH"), + CIProvider::GithubActions => env::var_non_empty("GITHUB_BASE_REF"), + CIProvider::Jenkins => env::var_non_empty("CHANGE_TARGET"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_PULL_REQUEST_BASE_BRANCH"), CIProvider::CircleCi => None, } } @@ -369,9 +372,9 @@ pub fn get_base_ref_name() -> Option { #[must_use] pub fn get_cicd_pipeline_runner_name() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => non_empty_env("RUNNER_NAME"), - CIProvider::Jenkins => non_empty_env("NODE_NAME"), - CIProvider::Buildkite => non_empty_env("BUILDKITE_AGENT_NAME"), + CIProvider::GithubActions => env::var_non_empty("RUNNER_NAME"), + CIProvider::Jenkins => env::var_non_empty("NODE_NAME"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_AGENT_NAME"), CIProvider::CircleCi => None, } } @@ -382,10 +385,10 @@ pub fn get_cicd_pipeline_runner_name() -> Option { #[must_use] pub fn get_cicd_pipeline_run_id() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => non_empty_env("GITHUB_RUN_ID"), - CIProvider::CircleCi => non_empty_env("CIRCLE_WORKFLOW_ID"), - CIProvider::Jenkins => non_empty_env("BUILD_ID"), - CIProvider::Buildkite => non_empty_env("BUILDKITE_BUILD_ID"), + CIProvider::GithubActions => env::var_non_empty("GITHUB_RUN_ID"), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_WORKFLOW_ID"), + CIProvider::Jenkins => env::var_non_empty("BUILD_ID"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_BUILD_ID"), } } @@ -393,11 +396,11 @@ pub fn get_cicd_pipeline_run_id() -> Option { #[must_use] pub fn get_cicd_pipeline_run_attempt() -> Option { match get_ci_provider()? { - CIProvider::GithubActions => non_empty_env("GITHUB_RUN_ATTEMPT")?.parse().ok(), - CIProvider::CircleCi => non_empty_env("CIRCLE_BUILD_NUM")?.parse().ok(), + CIProvider::GithubActions => env::var_non_empty("GITHUB_RUN_ATTEMPT")?.parse().ok(), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_BUILD_NUM")?.parse().ok(), // Buildkite uses 0-indexed retries; add 1 so a fresh run // reads as attempt 1 (matching the GHA/CircleCI semantics). - CIProvider::Buildkite => non_empty_env("BUILDKITE_RETRY_COUNT")? + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_RETRY_COUNT")? .parse::() .ok() .map(|n| n + 1), @@ -409,7 +412,7 @@ pub fn get_cicd_pipeline_run_attempt() -> Option { #[must_use] pub fn get_cicd_pipeline_run_url() -> Option { match get_ci_provider()? { - CIProvider::Buildkite => non_empty_env("BUILDKITE_BUILD_URL"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_BUILD_URL"), _ => None, } } @@ -420,9 +423,9 @@ pub fn get_cicd_pipeline_run_url() -> Option { #[must_use] pub fn get_repository_url() -> Option { match get_ci_provider()? { - CIProvider::Buildkite => non_empty_env("BUILDKITE_REPO"), - CIProvider::CircleCi => non_empty_env("CIRCLE_REPOSITORY_URL"), - CIProvider::Jenkins => non_empty_env("GIT_URL"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_REPO"), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_REPOSITORY_URL"), + CIProvider::Jenkins => env::var_non_empty("GIT_URL"), CIProvider::GithubActions => None, } } @@ -444,19 +447,19 @@ pub fn get_repository_url() -> Option { pub fn get_head_sha() -> Option { match get_ci_provider()? { CIProvider::GithubActions => get_github_actions_head_sha(), - CIProvider::CircleCi => non_empty_env("CIRCLE_SHA1"), - CIProvider::Jenkins => non_empty_env("GIT_COMMIT"), - CIProvider::Buildkite => non_empty_env("BUILDKITE_COMMIT"), + CIProvider::CircleCi => env::var_non_empty("CIRCLE_SHA1"), + CIProvider::Jenkins => env::var_non_empty("GIT_COMMIT"), + CIProvider::Buildkite => env::var_non_empty("BUILDKITE_COMMIT"), } } fn get_github_actions_head_sha() -> Option { - if env::var("GITHUB_EVENT_NAME").as_deref() == Ok("pull_request") + if env::var("GITHUB_EVENT_NAME").as_deref() == Some("pull_request") && let Some(sha) = read_github_event_pull_request_head_sha() { return Some(sha); } - non_empty_env("GITHUB_SHA") + env::var_non_empty("GITHUB_SHA") } /// Read `GITHUB_EVENT_PATH` and pluck the @@ -475,7 +478,7 @@ fn read_github_event_pull_request_head_sha() -> Option { } fn read_github_event_json() -> Option { - let event_path = env::var("GITHUB_EVENT_PATH").ok()?; + let event_path = env::var("GITHUB_EVENT_PATH")?; if event_path.is_empty() { return None; } @@ -483,10 +486,6 @@ fn read_github_event_json() -> Option { serde_json::from_str(&content).ok() } -fn non_empty_env(name: &str) -> Option { - env::var(name).ok().filter(|s| !s.is_empty()) -} - /// Branch the quarantine API should look up tests for. Mirrors /// Python's `get_tests_target_branch`: the PR base branch when /// available, otherwise the head branch — i.e. "the branch the @@ -500,13 +499,12 @@ pub fn get_tests_target_branch() -> Option { #[cfg(test)] mod tests { use super::*; - use crate::testing::with_ci_env; use crate::testing::write_github_event; #[test] fn ci_provider_jenkins_takes_precedence() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("JENKINS_URL", Some("http://jenkins")), ("GITHUB_ACTIONS", Some("true")), ("CIRCLECI", Some("true")), @@ -520,15 +518,15 @@ mod tests { #[test] fn ci_provider_returns_none_when_unset() { - with_ci_env(&[], || { + env::testing::with_no_vars(|| { assert_eq!(get_ci_provider(), None); }); } #[test] fn github_repository_github_actions() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ], @@ -540,8 +538,8 @@ mod tests { #[test] fn github_repository_buildkite_ssh() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_REPO", Some("git@github.com:owner/repo.git")), ], @@ -553,8 +551,8 @@ mod tests { #[test] fn github_repository_buildkite_https() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_REPO", Some("https://github.com/owner/repo")), ], @@ -566,8 +564,8 @@ mod tests { #[test] fn github_repository_circleci() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("CIRCLECI", Some("true")), ( "CIRCLE_REPOSITORY_URL", @@ -582,8 +580,8 @@ mod tests { #[test] fn github_repository_jenkins() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("JENKINS_URL", Some("http://jenkins")), ("GIT_URL", Some("https://github.com/owner/repo.git")), ], @@ -595,15 +593,15 @@ mod tests { #[test] fn github_repository_returns_none_with_no_provider() { - with_ci_env(&[("GITHUB_REPOSITORY", Some("owner/repo"))], || { + env::testing::with_vars([("GITHUB_REPOSITORY", Some("owner/repo"))], || { assert_eq!(get_github_repository(), None); }); } #[test] fn resolve_repository_prefers_flag_over_env() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("env/env")), ], @@ -619,8 +617,8 @@ mod tests { // used — even though this test runs inside a git checkout whose // `origin` would otherwise resolve to a different slug. Asserts // both the CI fallback and its precedence over the git remote. - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ], @@ -636,7 +634,7 @@ mod tests { // git-remote fallback (`parse_slug`) accepts multi-segment // paths that would inject extra request-path segments; an // explicit value exercises the same guard deterministically. - with_ci_env(&[], || { + env::testing::with_no_vars(|| { assert!(matches!( resolve_repository(Some("owner/repo/extra")), Err(CliError::Configuration(_)) @@ -652,8 +650,8 @@ mod tests { #[test] fn pull_request_buildkite_reads_env() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_PULL_REQUEST", Some("42")), ], @@ -665,8 +663,8 @@ mod tests { #[test] fn pull_request_buildkite_returns_none_when_false() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_PULL_REQUEST", Some("false")), ], @@ -678,14 +676,14 @@ mod tests { #[test] fn pull_request_buildkite_returns_none_when_unset() { - with_ci_env(&[("BUILDKITE", Some("true"))], || { + env::testing::with_vars([("BUILDKITE", Some("true"))], || { assert_eq!(get_github_pull_request_number().unwrap(), None); }); } #[test] fn pull_request_returns_none_with_no_provider() { - with_ci_env(&[], || { + env::testing::with_no_vars(|| { assert_eq!(get_github_pull_request_number().unwrap(), None); }); } @@ -699,8 +697,8 @@ mod tests { serde_json::json!({ "pull_request": { "number": 123 } }).to_string(), ) .unwrap(); - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), ], @@ -714,8 +712,8 @@ mod tests { fn pull_request_github_actions_missing_event_file_returns_none() { let tmp = tempfile::tempdir().unwrap(); let missing = tmp.path().join("nope.json"); - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_PATH", Some(missing.to_str().unwrap())), ], @@ -764,8 +762,8 @@ mod tests { ); for event_name in ["pull_request", "pull_request_target"] { - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some(event_name)), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), @@ -793,8 +791,8 @@ mod tests { // not. let tmp = tempfile::tempdir().unwrap(); let event_path = write_github_event(tmp.path(), &serde_json::json!({})); - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("push")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), @@ -811,8 +809,8 @@ mod tests { #[test] fn pull_request_head_sha_uses_buildkite_commit() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ( "BUILDKITE_COMMIT", @@ -832,8 +830,8 @@ mod tests { fn pull_request_head_sha_drops_a_value_that_is_not_a_revision() { // An unset `BUILDKITE_COMMIT` is what `git_refs` reads as the // literal `HEAD`; callers must not have to re-check the shape. - with_ci_env( - &[ + env::testing::with_vars( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_COMMIT", Some("HEAD")), ], @@ -845,8 +843,8 @@ mod tests { #[test] fn pull_request_head_sha_uses_circle_sha1() { - with_ci_env( - &[ + env::testing::with_vars( + [ ("CIRCLECI", Some("true")), ( "CIRCLE_SHA1", @@ -868,8 +866,8 @@ mod tests { // plugin builds a pull request merged into its target, which is // its default, and nothing distinguishes that from the head-only // configuration. No answer beats the wrong one. - with_ci_env( - &[ + env::testing::with_vars( + [ ("JENKINS_URL", Some("http://ci")), ( "GIT_COMMIT", @@ -976,8 +974,8 @@ mod tests { ) .unwrap(); - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), @@ -1003,8 +1001,8 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let event_path = tmp.path().join("event.json"); std::fs::write(&event_path, serde_json::json!({}).to_string()).unwrap(); - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("push")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), @@ -1021,8 +1019,8 @@ mod tests { // Workflows without an event file (e.g. local // `act` runs) still set GITHUB_SHA — we must not regress // to `None` just because the JSON file isn't there. - with_ci_env( - &[ + env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some("/this/path/does/not/exist")), diff --git a/crates/mergify-ci/src/git_refs.rs b/crates/mergify-ci/src/git_refs.rs index 2ca9ab59..24c46ef9 100644 --- a/crates/mergify-ci/src/git_refs.rs +++ b/crates/mergify-ci/src/git_refs.rs @@ -27,7 +27,7 @@ //! `BUILDKITE=true` it invokes `buildkite-agent meta-data set` for //! base/head/source. -use std::env; +use mergify_core::env; use std::io::Write; use std::process::Command; @@ -152,7 +152,7 @@ pub fn detect( output: &mut dyn Output, notes_reader: NotesReader<'_>, ) -> Result { - if env::var("BUILDKITE").as_deref() == Ok("true") + if env::var("BUILDKITE").as_deref() == Some("true") && let Some(refs) = detect_from_buildkite(notes_reader) { return Ok(refs); @@ -188,16 +188,12 @@ pub fn detect( } fn detect_from_buildkite(notes_reader: NotesReader<'_>) -> Option { - let pr = env::var("BUILDKITE_PULL_REQUEST").ok()?; + let pr = env::var("BUILDKITE_PULL_REQUEST")?; if pr.is_empty() || pr == "false" { return None; } - let commit = env::var("BUILDKITE_COMMIT") - .ok() - .filter(|s| !s.is_empty()) - .unwrap_or_else(|| "HEAD".to_string()); - if let Ok(branch) = env::var("BUILDKITE_BRANCH") - && !branch.is_empty() + let commit = env::var_non_empty("BUILDKITE_COMMIT").unwrap_or_else(|| "HEAD".to_string()); + if let Some(branch) = env::var_non_empty("BUILDKITE_BRANCH") && let Some(base) = notes_reader(&branch, &commit) { return Some(References { @@ -206,9 +202,7 @@ fn detect_from_buildkite(notes_reader: NotesReader<'_>) -> Option { source: ReferencesSource::MergeQueue, }); } - let base_branch = env::var("BUILDKITE_PULL_REQUEST_BASE_BRANCH") - .ok() - .filter(|s| !s.is_empty())?; + let base_branch = env::var_non_empty("BUILDKITE_PULL_REQUEST_BASE_BRANCH")?; Some(References { base: Some(base_branch), head: commit, @@ -471,7 +465,7 @@ fn write_github_output(refs: &References) -> Result<(), CliError> { } fn write_buildkite_metadata(refs: &References) -> std::io::Result<()> { - if env::var("BUILDKITE").as_deref() != Ok("true") { + if env::var("BUILDKITE").as_deref() != Some("true") { return Ok(()); } if let Some(base) = refs.base.as_deref() { @@ -582,10 +576,9 @@ mod tests { #[test] fn falls_back_to_head_pair_when_no_event() { let mut cap = Captured::human(); - let refs = temp_env::with_vars_unset( - ["GITHUB_EVENT_NAME", "GITHUB_EVENT_PATH", "BUILDKITE"], - || detect(&mut cap.output, &no_notes).unwrap(), - ); + // An empty overlay is the empty environment: no provider + // variable is visible, whatever the host exports. + let refs = env::testing::with_no_vars(|| detect(&mut cap.output, &no_notes).unwrap()); assert_eq!(refs.base.as_deref(), Some("HEAD^")); assert_eq!(refs.head, "HEAD"); assert_eq!(refs.source, ReferencesSource::FallbackLastCommit); @@ -604,7 +597,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -625,7 +618,7 @@ mod tests { &serde_json::json!({"before": "old-sha", "after": "new-sha"}), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("push")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -652,7 +645,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -688,7 +681,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -719,7 +712,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -757,7 +750,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -793,7 +786,7 @@ mod tests { }), ); let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -834,7 +827,7 @@ mod tests { } }; let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -853,7 +846,7 @@ mod tests { &serde_json::json!({"pull_request": {"head": {"sha": "h"}}}), ); let mut cap = Captured::human(); - let err = temp_env::with_vars( + let err = env::testing::with_vars( [ ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())), @@ -867,7 +860,7 @@ mod tests { #[test] fn detects_buildkite_pull_request() { let mut cap = Captured::human(); - let refs = temp_env::with_vars( + let refs = env::testing::with_vars( [ ("BUILDKITE", Some("true")), ("BUILDKITE_PULL_REQUEST", Some("42")), @@ -966,7 +959,7 @@ mod tests { head: NOTE_BASE.into(), source: ReferencesSource::MergeQueue, }; - temp_env::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { + env::testing::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { write_github_output(&refs).unwrap(); }); let written = std::fs::read_to_string(&path).unwrap(); diff --git a/crates/mergify-ci/src/github_event.rs b/crates/mergify-ci/src/github_event.rs index f725225b..4495e9fc 100644 --- a/crates/mergify-ci/src/github_event.rs +++ b/crates/mergify-ci/src/github_event.rs @@ -5,7 +5,7 @@ //! `deny_unknown_fields` on purpose) so the payload's superset of //! fields doesn't break us. -use std::env; +use mergify_core::env; use std::path::PathBuf; use serde::Deserialize; @@ -65,12 +65,8 @@ pub const PULL_REQUEST_EVENTS: &[&str] = &[ /// `GitHubEventNotFoundError` being converted to a fallback. #[must_use] pub fn load() -> Option<(String, GitHubEvent)> { - let event_name = env::var("GITHUB_EVENT_NAME") - .ok() - .filter(|s| !s.is_empty())?; - let event_path = env::var("GITHUB_EVENT_PATH") - .ok() - .filter(|s| !s.is_empty())?; + let event_name = env::var_non_empty("GITHUB_EVENT_NAME")?; + let event_path = env::var_non_empty("GITHUB_EVENT_PATH")?; let path = PathBuf::from(event_path); if !path.is_file() { return None; diff --git a/crates/mergify-ci/src/github_output.rs b/crates/mergify-ci/src/github_output.rs index 001fdeec..9a9f725e 100644 --- a/crates/mergify-ci/src/github_output.rs +++ b/crates/mergify-ci/src/github_output.rs @@ -33,7 +33,7 @@ //! ahead of the `<<`) would let the runner read the block as //! something else. -use std::env; +use mergify_core::env; use std::fmt::Write as _; use std::fs::OpenOptions; use std::io::Write as _; @@ -45,7 +45,7 @@ use mergify_core::CliError; /// output. No-op when the variable is unset or empty — i.e. anywhere /// but a GitHub Actions runner. pub(crate) fn append(outputs: &[(&'static str, &str)]) -> Result<(), CliError> { - let Some(path) = env::var("GITHUB_OUTPUT").ok().filter(|s| !s.is_empty()) else { + let Some(path) = env::var_non_empty("GITHUB_OUTPUT") else { return Ok(()); }; // Assembled first, then written once. Three `writeln!` calls on an @@ -90,13 +90,13 @@ mod tests { #[test] fn append_is_a_noop_outside_github_actions() { - temp_env::with_var("GITHUB_OUTPUT", None::<&str>, || { + env::testing::with_var("GITHUB_OUTPUT", None::<&str>, || { append(&[("k", "v")]).unwrap(); }); // An empty value is treated the same as unset: the runner // exports `GITHUB_OUTPUT=` in some contexts, and an empty // path is not openable. - temp_env::with_var("GITHUB_OUTPUT", Some(""), || { + env::testing::with_var("GITHUB_OUTPUT", Some(""), || { append(&[("k", "v")]).unwrap(); }); } @@ -105,7 +105,7 @@ mod tests { fn append_wraps_every_output_in_its_own_heredoc() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("gha_output"); - temp_env::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { + env::testing::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { append(&[("base", "cafef00d"), ("head", "0badc0de")]).unwrap(); }); let written = std::fs::read_to_string(&path).unwrap(); @@ -134,7 +134,7 @@ mod tests { // step output (MRGFY-8845). let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("gha_output"); - temp_env::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { + env::testing::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { append(&[("base", "cafef00d\nevil=1")]).unwrap(); }); let written = std::fs::read_to_string(&path).unwrap(); @@ -152,7 +152,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("gha_output"); std::fs::write(&path, "earlier=1\n").unwrap(); - temp_env::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { + env::testing::with_var("GITHUB_OUTPUT", Some(path.to_str().unwrap()), || { append(&[("base", "cafef00d")]).unwrap(); }); let written = std::fs::read_to_string(&path).unwrap(); diff --git a/crates/mergify-ci/src/junit_process/command.rs b/crates/mergify-ci/src/junit_process/command.rs index 2f8b96aa..3780b675 100644 --- a/crates/mergify-ci/src/junit_process/command.rs +++ b/crates/mergify-ci/src/junit_process/command.rs @@ -25,7 +25,7 @@ use std::path::{Path, PathBuf}; use mergify_core::auth; -use mergify_core::env::var_non_empty; +use mergify_core::env; use mergify_core::{CliError, ExitCode, Output}; use crate::detector; @@ -164,7 +164,7 @@ async fn run_with_cap( let metadata = UploadMetadata { test_framework: opts.test_framework.map(str::to_string), test_language: opts.test_language.map(str::to_string), - mergify_test_job_name: var_non_empty("MERGIFY_TEST_JOB_NAME"), + mergify_test_job_name: env::var_non_empty("MERGIFY_TEST_JOB_NAME"), quarantined: quarantine_result .quarantined .iter() @@ -337,7 +337,7 @@ fn resolve_test_exit_code(explicit: Option) -> Result, CliError if explicit.is_some() { return Ok(explicit); } - let Some(raw) = var_non_empty("MERGIFY_TEST_EXIT_CODE") else { + let Some(raw) = env::var_non_empty("MERGIFY_TEST_EXIT_CODE") else { return Ok(None); }; raw.parse::().map(Some).map_err(|e| { @@ -606,10 +606,7 @@ fn upload_status_label( /// on stderr instead of erroring. fn maybe_write_github_output(status: &str) { use std::io::Write as _; - let Some(path) = std::env::var("GITHUB_OUTPUT") - .ok() - .filter(|s| !s.is_empty()) - else { + let Some(path) = env::var_non_empty("GITHUB_OUTPUT") else { return; }; let result = std::fs::OpenOptions::new() @@ -634,7 +631,7 @@ fn maybe_write_github_output(status: &str) { /// they're permanent misconfiguration; transient failures (5xx, /// 408, 429, network) as warnings. fn gha_upload_annotation(error: &upload::UploadError) -> Option { - if std::env::var("GITHUB_ACTIONS").as_deref() != Ok("true") { + if env::var("GITHUB_ACTIONS").as_deref() != Some("true") { return None; } Some(if error.is_rejection() { @@ -659,7 +656,7 @@ fn gha_upload_annotation(error: &upload::UploadError) -> Option { /// summary / checks UI — otherwise it only appears in the human /// report prose. Never an error: the CI outcome is unaffected. fn gha_oversized_annotation(names: &[String]) -> Option { - if std::env::var("GITHUB_ACTIONS").as_deref() != Ok("true") { + if env::var("GITHUB_ACTIONS").as_deref() != Some("true") { return None; } Some(format!( @@ -890,7 +887,7 @@ mod tests { // `gha-mergify-ci` action uses when no runner exit code is // available. Pin so a future refactor can't accidentally // invert the precedence. - let got = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some("42"), || { + let got = env::testing::with_var("MERGIFY_TEST_EXIT_CODE", Some("42"), || { resolve_test_exit_code(Some(0)).unwrap() }); assert_eq!(got, Some(0)); @@ -909,7 +906,7 @@ mod tests { // fix drops the clap `env` hook and routes the env var // through here — empty must collapse to `None`, the // same shape no env var would produce. - let got = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || { + let got = env::testing::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || { resolve_test_exit_code(None).unwrap() }); assert_eq!(got, None); @@ -917,7 +914,7 @@ mod tests { #[test] fn resolve_test_exit_code_parses_non_empty_env_var() { - let got = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some("7"), || { + let got = env::testing::with_var("MERGIFY_TEST_EXIT_CODE", Some("7"), || { resolve_test_exit_code(None).unwrap() }); assert_eq!(got, Some(7)); @@ -929,7 +926,7 @@ mod tests { // real misconfiguration, not a "no value" sentinel — // error loudly with the offending value in the message so // the user can spot the typo without having to dig. - let err = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some("not-an-int"), || { + let err = env::testing::with_var("MERGIFY_TEST_EXIT_CODE", Some("not-an-int"), || { resolve_test_exit_code(None).unwrap_err() }); let msg = err.to_string(); @@ -1098,13 +1095,13 @@ mod tests { #[test] fn gha_oversized_annotation_lists_names_only_on_gha() { // Outside GitHub Actions: no annotation. - let none = temp_env::with_var("GITHUB_ACTIONS", None::<&str>, || { + let none = env::testing::with_var("GITHUB_ACTIONS", None::<&str>, || { gha_oversized_annotation(&["a.big".to_string()]) }); assert!(none.is_none()); // On GitHub Actions: a warning naming the dropped tests, never // an error (CI outcome is unaffected). - let ann = temp_env::with_var("GITHUB_ACTIONS", Some("true"), || { + let ann = env::testing::with_var("GITHUB_ACTIONS", Some("true"), || { gha_oversized_annotation(&["a.big".to_string(), "b.huge".to_string()]) }) .unwrap(); @@ -1134,7 +1131,7 @@ mod tests { write_oversized_cases(&mut report, std::slice::from_ref(&long)); assert!(!report.contains(&long), "the report printed the whole name"); - let ann = temp_env::with_var("GITHUB_ACTIONS", Some("true"), || { + let ann = env::testing::with_var("GITHUB_ACTIONS", Some("true"), || { gha_oversized_annotation(std::slice::from_ref(&long)) }) .unwrap(); @@ -1161,7 +1158,6 @@ mod tests { // banner text drifting). mod orchestrator { use super::*; - use crate::testing::with_ci_env_async; use mergify_core::{OutputMode, StdioOutput}; use std::sync::{Arc, Mutex}; use wiremock::matchers::{method, path}; @@ -1240,7 +1236,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1281,7 +1277,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); let cap_bytes = 4 * 1024; - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1422,8 +1418,8 @@ mod tests { // checks survive stamping. With the environment scrubbed // it would only ever emit `test.run.id`, and the check // would pass on an empty resource. - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ( @@ -1525,7 +1521,7 @@ mod tests { let file = write_xml(&tmp, "report.xml", &incompressible_failures_xml(30, 2048)); let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1566,7 +1562,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1604,7 +1600,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1640,7 +1636,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1732,7 +1728,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1761,7 +1757,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - with_ci_env_async(&[], async { + env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1829,8 +1825,8 @@ mod tests { let output_path = github_output.to_string_lossy().into_owned(); let mut cap = captured(); // 64-byte cap: smaller than any single case's gzipped span. - let code = with_ci_env_async( - &[ + let code = env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_OUTPUT", Some(&output_path)), ], @@ -1872,7 +1868,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1908,7 +1904,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -1952,7 +1948,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -2011,8 +2007,8 @@ mod tests { ) -> (ExitCode, String) { let mut cap = captured(); let output_path = github_output.to_string_lossy().into_owned(); - let code = with_ci_env_async( - &[ + let code = env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_OUTPUT", Some(&output_path)), ], @@ -2131,7 +2127,7 @@ mod tests { let api_url = server.uri(); let mut cap = captured(); - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some(&api_url), token: Some("secret"), @@ -2179,7 +2175,7 @@ mod tests { // No mock server: if the orchestrator skips the early // exit and tries to reach the API, the bogus URL will // fail the test loudly. - let code = with_ci_env_async(&[], async { + let code = env::testing::with_no_vars_async(async { let opts = JunitProcessOptions { api_url: Some("http://127.0.0.1:1"), token: Some("secret"), diff --git a/crates/mergify-ci/src/junit_process/spans.rs b/crates/mergify-ci/src/junit_process/spans.rs index ae988ec2..4d863bea 100644 --- a/crates/mergify-ci/src/junit_process/spans.rs +++ b/crates/mergify-ci/src/junit_process/spans.rs @@ -446,9 +446,10 @@ impl RandomBytes for OsRandom { #[cfg(test)] mod tests { + use mergify_core::env; + use super::*; use crate::junit_process::junit::Failure; - use crate::testing::with_ci_env; /// Deterministic byte source for tests. Bytes are consumed /// in order. Tests provide enough buffer for the spans they @@ -538,7 +539,8 @@ mod tests { let now: u64 = 1_700_000_000_000_000_000; let metadata = UploadMetadata::default(); - let built = with_ci_env(&[], || build_traces_with(&parsed, &metadata, now, &mut rng)); + let built = + env::testing::with_no_vars(|| build_traces_with(&parsed, &metadata, now, &mut rng)); assert_eq!(built.oversized_case_names, vec![oversized.clone()]); @@ -589,7 +591,7 @@ mod tests { bytes.extend(std::iter::repeat_n(0x55, 8)); let mut rng = FixedRng::new(bytes); - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with( &parsed, &UploadMetadata::default(), @@ -618,7 +620,7 @@ mod tests { let now: u64 = 1_700_000_000_000_000_000; let metadata = UploadMetadata::default(); - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with(&sample_parsed(), &metadata, now, &mut rng) }); @@ -661,7 +663,7 @@ mod tests { let mut rng = FixedRng::new(vec![0xFF; 256]); let now: u64 = 1_700_000_000_000_000_000; let metadata = UploadMetadata::default(); - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with(&sample_parsed(), &metadata, now, &mut rng) }); let spans = &built.request.resource_spans[0].scope_spans[0].spans; @@ -696,7 +698,7 @@ mod tests { fn case_attributes_include_file_line_and_code_function() { let mut rng = FixedRng::new(vec![0xFF; 256]); let metadata = UploadMetadata::default(); - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with(&sample_parsed(), &metadata, 0, &mut rng) }); let spans = &built.request.resource_spans[0].scope_spans[0].spans; @@ -737,8 +739,8 @@ mod tests { fn resource_attributes_carry_ci_env_when_set() { let mut rng = FixedRng::new(vec![0xFF; 256]); let metadata = UploadMetadata::default(); - let built = with_ci_env( - &[ + let built = env::testing::with_vars( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ("GITHUB_WORKFLOW", Some("CI")), @@ -795,7 +797,7 @@ mod tests { mergify_test_job_name: None, quarantined: BTreeSet::new(), }; - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with(&sample_parsed(), &metadata, 0, &mut rng) }); let spans = &built.request.resource_spans[0].scope_spans[0].spans; @@ -822,7 +824,7 @@ mod tests { let mut rng = FixedRng::new(vec![0xFF; 256]); let now: u64 = 1_000_000_000_000_000_000; let metadata = UploadMetadata::default(); - let built = with_ci_env(&[], || { + let built = env::testing::with_no_vars(|| { build_traces_with(&sample_parsed(), &metadata, now, &mut rng) }); let spans = &built.request.resource_spans[0].scope_spans[0].spans; diff --git a/crates/mergify-ci/src/junit_process/split.rs b/crates/mergify-ci/src/junit_process/split.rs index 54d8442d..bfc8170a 100644 --- a/crates/mergify-ci/src/junit_process/split.rs +++ b/crates/mergify-ci/src/junit_process/split.rs @@ -555,7 +555,8 @@ mod tests { use super::*; use crate::junit_process::junit::{Failure, ParseResult, TestCase, TestStatus}; use crate::junit_process::spans::{UploadMetadata, build_traces}; - use crate::testing::{incompressible, with_ci_env}; + use crate::testing::incompressible; + use mergify_core::env; use std::collections::BTreeSet; use std::time::Duration; @@ -595,7 +596,7 @@ mod tests { mergify_test_job_name: None, quarantined: BTreeSet::new(), }; - with_ci_env(&[], || build_traces(&parsed, &metadata)).request + env::testing::with_no_vars(|| build_traces(&parsed, &metadata)).request } /// Collect the `test.case.name` of every case span in a chunk. diff --git a/crates/mergify-ci/src/queue_info.rs b/crates/mergify-ci/src/queue_info.rs index d2db01e2..7c2b845a 100644 --- a/crates/mergify-ci/src/queue_info.rs +++ b/crates/mergify-ci/src/queue_info.rs @@ -109,6 +109,7 @@ fn write_github_output(metadata: &Value) -> Result<(), CliError> { #[cfg(test)] mod tests { use mergify_core::ExitCode; + use mergify_core::env; use mergify_test_support::Captured; use serde_json::json; @@ -141,7 +142,7 @@ mod tests { fn prints_whole_note_payload() { let note = || Some(sample()); let mut cap = Captured::human(); - temp_env::with_var("GITHUB_OUTPUT", None::<&str>, || { + env::testing::with_var("GITHUB_OUTPUT", None::<&str>, || { run_with_reader(&mut cap.output, ¬e).unwrap(); }); let stdout = cap.stdout(); @@ -161,7 +162,7 @@ mod tests { let gha_output = dir.path().join("gha_output"); let note = || Some(sample()); let mut cap = Captured::human(); - temp_env::with_var("GITHUB_OUTPUT", Some(gha_output.to_str().unwrap()), || { + env::testing::with_var("GITHUB_OUTPUT", Some(gha_output.to_str().unwrap()), || { run_with_reader(&mut cap.output, ¬e).unwrap(); }); let written = std::fs::read_to_string(&gha_output).unwrap(); diff --git a/crates/mergify-ci/src/scopes_detect/mod.rs b/crates/mergify-ci/src/scopes_detect/mod.rs index b313e346..b85e34f3 100644 --- a/crates/mergify-ci/src/scopes_detect/mod.rs +++ b/crates/mergify-ci/src/scopes_detect/mod.rs @@ -25,14 +25,13 @@ pub mod config; pub mod matching; pub mod outputs; -use std::env; use std::io::Write; use std::path::Path; use std::path::PathBuf; use mergify_core::CliError; use mergify_core::Output; -use mergify_core::env::var_non_empty; +use mergify_core::env; use serde::Serialize; use crate::git_refs; @@ -128,7 +127,7 @@ fn resolve_config_path(explicit: Option<&Path>) -> Result { path.display(), ))); } - if let Some(env_path) = var_non_empty("MERGIFY_CONFIG_PATH") { + if let Some(env_path) = env::var_non_empty("MERGIFY_CONFIG_PATH") { let p = PathBuf::from(&env_path); if !p.is_file() { return Err(CliError::Configuration(format!( @@ -240,7 +239,7 @@ fn emit_scopes_listing( by_scope: &std::collections::BTreeMap>, output: &mut dyn Output, ) -> Result<(), CliError> { - let actions_debug = env::var("ACTIONS_STEP_DEBUG").as_deref() == Ok("true"); + let actions_debug = env::var("ACTIONS_STEP_DEBUG").as_deref() == Some("true"); if hit.is_empty() { output.status("No scopes matched.")?; return Ok(()); @@ -289,7 +288,6 @@ fn write_detected_scopes( #[cfg(test)] mod tests { use super::*; - use crate::testing::with_ci_env; use mergify_test_support::Captured; #[test] @@ -330,7 +328,7 @@ mod tests { // so this function owns the lookup — and the empty branch // here must fall through to autodetect rather than report // a malformed env var. - let result = temp_env::with_var("MERGIFY_CONFIG_PATH", Some(""), || { + let result = env::testing::with_var("MERGIFY_CONFIG_PATH", Some(""), || { resolve_config_path(None) }); // Either autodetect found a real config (cargo test runs @@ -354,9 +352,10 @@ mod tests { // value that doesn't exist, the error must name the env // var + the bogus path so the user can spot the typo // without having to dig. - let err = temp_env::with_var("MERGIFY_CONFIG_PATH", Some("/no/such/.mergify.yml"), || { - resolve_config_path(None).unwrap_err() - }); + let err = + env::testing::with_var("MERGIFY_CONFIG_PATH", Some("/no/such/.mergify.yml"), || { + resolve_config_path(None).unwrap_err() + }); let msg = err.to_string(); assert!(msg.contains("MERGIFY_CONFIG_PATH="), "got: {msg}"); assert!(msg.contains("/no/such/.mergify.yml"), "got: {msg}"); @@ -402,11 +401,10 @@ mod tests { // "select all scopes" branch and reports every // configured scope as touched. No git operations. // - // The `with_ci_env` wrapper scrubs `GITHUB_OUTPUT` so a - // GHA runner executing the suite doesn't see `run()` - // append a heredoc to its real step-output file (which - // would break the runner step with "Matching delimiter - // not found"). + // The empty overlay hides `GITHUB_OUTPUT` so a GHA runner + // executing the suite doesn't see `run()` append a heredoc + // to its real step-output file (which would break the + // runner step with "Matching delimiter not found"). let tmp = tempfile::tempdir().unwrap(); let cfg = tmp.path().join("mergify.yml"); std::fs::write( @@ -415,7 +413,7 @@ mod tests { ) .unwrap(); let mut cap = Captured::human(); - with_ci_env(&[], || { + env::testing::with_no_vars(|| { run( ScopesOptions { config: Some(&cfg), @@ -444,7 +442,7 @@ mod tests { let cfg = tmp.path().join("mergify.yml"); std::fs::write(&cfg, "scopes:\n source:\n manual: null\n").unwrap(); let mut cap = Captured::human(); - let err = with_ci_env(&[], || { + let err = env::testing::with_no_vars(|| { run( ScopesOptions { config: Some(&cfg), @@ -475,7 +473,7 @@ mod tests { .unwrap(); let out = tmp.path().join("detected.json"); let mut cap = Captured::human(); - with_ci_env(&[], || { + env::testing::with_no_vars(|| { run( ScopesOptions { config: Some(&cfg), diff --git a/crates/mergify-ci/src/scopes_detect/outputs.rs b/crates/mergify-ci/src/scopes_detect/outputs.rs index d3582beb..da3bacd4 100644 --- a/crates/mergify-ci/src/scopes_detect/outputs.rs +++ b/crates/mergify-ci/src/scopes_detect/outputs.rs @@ -5,8 +5,8 @@ //! `mergify_cli/ci/scopes/cli.py` and stay quiet when their //! respective environment knob is absent. +use mergify_core::env; use std::collections::BTreeSet; -use std::env; use std::fmt::Write as _; use std::fs::OpenOptions; use std::io::Write; @@ -64,7 +64,7 @@ pub fn maybe_write_buildkite_metadata( all: &BTreeSet, hit: &BTreeSet, ) -> Result<(), CliError> { - if env::var("BUILDKITE").as_deref() != Ok("true") { + if env::var("BUILDKITE").as_deref() != Some("true") { return Ok(()); } let payload = scopes_dict_json(all, hit); @@ -114,10 +114,7 @@ pub fn maybe_write_github_step_summary( all: &BTreeSet, hit: &BTreeSet, ) -> Result<(), CliError> { - let Some(path) = env::var("GITHUB_STEP_SUMMARY") - .ok() - .filter(|s| !s.is_empty()) - else { + let Some(path) = env::var_non_empty("GITHUB_STEP_SUMMARY") else { return Ok(()); }; let md = build_summary_markdown(refs, all, hit); @@ -138,7 +135,7 @@ pub fn maybe_write_buildkite_annotation( all: &BTreeSet, hit: &BTreeSet, ) { - if env::var("BUILDKITE").as_deref() != Ok("true") { + if env::var("BUILDKITE").as_deref() != Some("true") { return; } let md = build_summary_markdown(refs, all, hit); diff --git a/crates/mergify-ci/src/scopes_send.rs b/crates/mergify-ci/src/scopes_send.rs index 06a1a265..5fba0e37 100644 --- a/crates/mergify-ci/src/scopes_send.rs +++ b/crates/mergify-ci/src/scopes_send.rs @@ -228,6 +228,7 @@ struct SendScopesRequest<'a> { mod tests { use std::fs; + use mergify_core::env; use mergify_test_support::Captured; use wiremock::Mock; use wiremock::MockServer; @@ -238,13 +239,11 @@ mod tests { use wiremock::matchers::path; use super::*; - use crate::testing::with_ci_env; - use crate::testing::with_ci_env_async; use crate::testing::write_github_event; #[test] fn resolve_pull_request_prefers_explicit() { - with_ci_env(&[], || { + env::testing::with_no_vars(|| { assert_eq!(resolve_pull_request(Some(7)).unwrap(), Some(7)); }); } @@ -287,7 +286,7 @@ mod tests { #[tokio::test] async fn run_skips_when_no_pull_request_detected() { let mut cap = Captured::human(); - with_ci_env_async(&[("GITHUB_REPOSITORY", Some("owner/repo"))], async { + env::testing::with_vars_async([("GITHUB_REPOSITORY", Some("owner/repo"))], async { run( ScopesSendOptions { repository: None, @@ -331,8 +330,8 @@ mod tests { let api_url = server.uri(); let direct = vec!["a".to_string()]; - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("BUILDKITE", Some("true")), ("BUILDKITE_REPO", Some("git@github.com:owner/repo.git")), ("BUILDKITE_PULL_REQUEST", Some("99")), @@ -554,8 +553,8 @@ mod tests { let api_url = server.uri(); let direct = vec!["a".to_string()]; - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), @@ -620,8 +619,8 @@ mod tests { let api_url = server.uri(); let direct = vec!["backend".to_string()]; - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_EVENT_NAME", Some("pull_request")), ("GITHUB_EVENT_PATH", Some(event_path.to_str().unwrap())), diff --git a/crates/mergify-ci/src/testing.rs b/crates/mergify-ci/src/testing.rs index db729e76..1a496126 100644 --- a/crates/mergify-ci/src/testing.rs +++ b/crates/mergify-ci/src/testing.rs @@ -1,130 +1,9 @@ //! Test-only helpers shared across the CI-aware command modules //! (`detector`, `scopes_send`, `tests_show`, `tests_quarantine`). -//! -//! These modules test CI-provider-aware code paths and need to scrub -//! the host's CI env vars before running each case — otherwise a -//! test running on a real Buildkite/Actions/Circle/Jenkins host -//! inherits provider state and the detector picks the wrong branch. -//! Two flavors: a sync `with_ci_env` and an async `with_ci_env_async` -//! (used by the `#[tokio::test]` cases). -use std::future::Future; use std::path::Path; use std::path::PathBuf; -/// Env vars the CI-provider detection chain inspects. Clear every -/// one of them before applying the test-specific overrides, so the -/// host environment can't leak into the test — running the test -/// suite *on* a real GitHub Actions / `CircleCI` / Jenkins / Buildkite -/// host would otherwise produce `vcs.ref.head.name` etc. values -/// taken from the runner instead of the test's explicit override -/// and silently fail. -/// -/// `GITHUB_OUTPUT` belongs on this list too — when the suite runs -/// on a GHA runner that var points at the runner's real -/// step-output file, and any test that exercises a code path -/// appending a heredoc (e.g. `ci scopes` → -/// `MERGIFY_SCOPES<)]) -> Vec<(String, Option)> { - let mut vars: Vec<(String, Option)> = CI_ENV_VARS - .iter() - .map(|k| ((*k).to_string(), None)) - .collect(); - for (k, v) in extra { - vars.push(((*k).to_string(), v.map(ToString::to_string))); - } - vars -} - -/// Run `f` with the CI-provider env vars cleared, plus the -/// `extra` overrides applied on top. -pub(crate) fn with_ci_env(extra: &[(&str, Option<&str>)], f: F) -> R -where - F: FnOnce() -> R, -{ - temp_env::with_vars(merged_overrides(extra), f) -} - -/// Async counterpart to [`with_ci_env`]. Used by `#[tokio::test]` -/// cases in `scopes_send` — the sync variant can't bridge `.await` -/// points. -pub(crate) async fn with_ci_env_async(extra: &[(&str, Option<&str>)], f: F) -> R -where - F: Future, -{ - temp_env::async_with_vars(merged_overrides(extra), f).await -} - /// Write `payload` as a GitHub Actions event file under `dir` and /// return its path, ready to point `GITHUB_EVENT_PATH` at. The /// payload-reading helpers across `detector` and `scopes_send` all diff --git a/crates/mergify-ci/src/tests_quarantine.rs b/crates/mergify-ci/src/tests_quarantine.rs index df934ce5..fd85ee1b 100644 --- a/crates/mergify-ci/src/tests_quarantine.rs +++ b/crates/mergify-ci/src/tests_quarantine.rs @@ -401,6 +401,7 @@ mod tests { use mergify_core::OutputMode; use mergify_core::StdioOutput; + use mergify_core::env; use serde_json::json; use wiremock::Mock; use wiremock::MockServer; @@ -410,7 +411,6 @@ mod tests { use wiremock::matchers::path as path_matcher; use super::*; - use crate::testing::with_ci_env_async; type SharedBytes = Arc>>; @@ -805,8 +805,8 @@ mod tests { // With no `--repository`, the command resolves the repository // from the CI environment — here GitHub Actions' // `GITHUB_REPOSITORY` — and queries that repository's endpoint. - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ], diff --git a/crates/mergify-ci/src/tests_show.rs b/crates/mergify-ci/src/tests_show.rs index 227dfbe0..ad049a49 100644 --- a/crates/mergify-ci/src/tests_show.rs +++ b/crates/mergify-ci/src/tests_show.rs @@ -370,6 +370,7 @@ mod tests { use mergify_core::OutputMode; use mergify_core::StdioOutput; + use mergify_core::env; use serde_json::json; use wiremock::Mock; use wiremock::MockServer; @@ -378,7 +379,6 @@ mod tests { use wiremock::matchers::path as path_matcher; use super::*; - use crate::testing::with_ci_env_async; type SharedBytes = Arc>>; @@ -522,8 +522,8 @@ mod tests { // With no `--repository`, the command resolves the repository // from the CI environment — here GitHub Actions' // `GITHUB_REPOSITORY` — and queries that repository's endpoint. - with_ci_env_async( - &[ + env::testing::with_vars_async( + [ ("GITHUB_ACTIONS", Some("true")), ("GITHUB_REPOSITORY", Some("owner/repo")), ], From 8e184c2d6ff592687ac4fc743874178705adecc1 Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:54:01 +0200 Subject: [PATCH 4/4] refactor(stack): read the environment through the funnel The editor chain (`GIT_EDITOR`, `VISUAL`, `EDITOR`) and `MERGIFY_GITHUB_SERVER` now read through `mergify_core::env`, and `note`'s three `temp_env` calls install an overlay instead of mutating the process environment. `note::non_empty_env` moves into the funnel as `var_os_non_empty`. The "empty means unset" rule is the one `mergify_core::env`'s module doc calls twice-bitten and regression-prone, so it should not have a second local copy. The `OsString` return, which exists so an editor path reaches `Command` without a UTF-8 round trip, was the only reason it could not already call `var_non_empty`. `mergify-stack` is the crate that already knew about this. Its `test_env.rs` exists because the workspace forbids `unsafe_code` and so it could not `set_var` `GIT_CONFIG_GLOBAL` at process start: it passes the variable per-`Command` instead. That is the same answer this stack generalises, build the environment for the consumer explicitly rather than mutating the process's own, reached for git config alone back in June. Its module doc claimed a git child spawned by production code inherits variables set on a *sibling* `Command`, which is not how `Command::env` works; it now says what the helper does and does not cover. `MERGIFY_GITHUB_SERVER` still crosses a real process boundary: the integration tests set it with `Command::env` on the binary they spawn, and a child's environment is not a shared one. Nothing there changes. Co-Authored-By: Claude Opus 5 Change-Id: I1a360878f5ad6538ebfcae5e1092e2155a36b263 --- Cargo.lock | 1 - crates/mergify-stack/Cargo.toml | 1 - crates/mergify-stack/src/commands/note.rs | 21 +++++++-------------- crates/mergify-stack/src/stack_context.rs | 4 +--- crates/mergify-stack/src/test_env.rs | 23 +++++++++++++---------- 5 files changed, 21 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 257a0689..ca992c7b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1906,7 +1906,6 @@ dependencies = [ "regex", "serde", "serde_json", - "temp-env", "tempfile", "terminal_size", "tokio", diff --git a/crates/mergify-stack/Cargo.toml b/crates/mergify-stack/Cargo.toml index 87d56830..487436b1 100644 --- a/crates/mergify-stack/Cargo.toml +++ b/crates/mergify-stack/Cargo.toml @@ -38,7 +38,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] -temp-env = { workspace = true } tokio = { workspace = true } url = { workspace = true } wiremock = { workspace = true } diff --git a/crates/mergify-stack/src/commands/note.rs b/crates/mergify-stack/src/commands/note.rs index 8b496bf4..0a609020 100644 --- a/crates/mergify-stack/src/commands/note.rs +++ b/crates/mergify-stack/src/commands/note.rs @@ -16,6 +16,7 @@ use std::process::Command; use crate::git::{resolve_repo_toplevel, run_git_capture as run_git}; use mergify_core::CliError; +use mergify_core::env; use crate::change_id; use crate::local_commits::{self, STACK_NOTES_REF}; @@ -212,9 +213,9 @@ fn read_note_from_editor() -> Result { // Treat empty env-var values as unset so `GIT_EDITOR=` falls // through to `$VISUAL` / `$EDITOR` / `vi` instead of spawning // an empty command. Matches Python's `or`-chain semantics. - let editor = non_empty_env("GIT_EDITOR") - .or_else(|| non_empty_env("VISUAL")) - .or_else(|| non_empty_env("EDITOR")) + let editor = env::var_os_non_empty("GIT_EDITOR") + .or_else(|| env::var_os_non_empty("VISUAL")) + .or_else(|| env::var_os_non_empty("EDITOR")) .unwrap_or_else(|| OsString::from("vi")); let mut tmp = tempfile::Builder::new() @@ -261,14 +262,6 @@ fn read_note_from_editor() -> Result { Ok(cleaned) } -/// Read an env var, returning `None` for both unset *and* empty. -/// `OsString::is_empty` covers both `KEY` being absent and -/// `KEY=` exporting an empty string (which Python's `or` chain -/// in `_read_note_from_editor` also treats as unset). -fn non_empty_env(name: &str) -> Option { - std::env::var_os(name).filter(|v| !v.is_empty()) -} - #[cfg(unix)] fn invoke_editor(editor: &OsString, path: &str) -> Result { let cmd_line = format!("{} \"$@\"", editor.to_string_lossy()); @@ -454,7 +447,7 @@ mod tests { .unwrap(); set_executable(&editor); - temp_env::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { + env::testing::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { run(Some(dir.path()), None, Action::FromEditor).unwrap(); }); assert_eq!( @@ -479,7 +472,7 @@ mod tests { .unwrap(); set_executable(&editor); - let err = temp_env::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { + let err = env::testing::with_var("GIT_EDITOR", Some(editor.to_str().unwrap()), || { run(Some(dir.path()), None, Action::FromEditor).unwrap_err() }); match err { @@ -500,7 +493,7 @@ mod tests { std::fs::write(&editor, "#!/bin/sh\nprintf 'from VISUAL\\n' > \"$1\"\n").unwrap(); set_executable(&editor); - temp_env::with_vars( + env::testing::with_vars( [ ("GIT_EDITOR", Some(String::new())), ("VISUAL", Some(editor.to_str().unwrap().to_string())), diff --git a/crates/mergify-stack/src/stack_context.rs b/crates/mergify-stack/src/stack_context.rs index 9ec14426..08415d29 100644 --- a/crates/mergify-stack/src/stack_context.rs +++ b/crates/mergify-stack/src/stack_context.rs @@ -173,9 +173,7 @@ pub fn resolve_repo( /// for local wiremock servers) without the coercion getting in /// the way. pub fn resolve_github_server(repo_dir: Option<&Path>) -> Result { - if let Ok(raw) = std::env::var("MERGIFY_GITHUB_SERVER") - && !raw.is_empty() - { + if let Some(raw) = mergify_core::env::var_non_empty("MERGIFY_GITHUB_SERVER") { return Url::parse(&raw).map_err(|e| { CliError::InvalidState(format!("invalid MERGIFY_GITHUB_SERVER '{raw}': {e}")) }); diff --git a/crates/mergify-stack/src/test_env.rs b/crates/mergify-stack/src/test_env.rs index 6d46393a..9df3bf16 100644 --- a/crates/mergify-stack/src/test_env.rs +++ b/crates/mergify-stack/src/test_env.rs @@ -7,16 +7,19 @@ //! is sporadic `git failed` panics in otherwise-pure tests //! that just happen to spawn git as a side effect. //! -//! The workspace forbids `unsafe_code`, so we can't `set_var` at -//! process start. Instead, [`isolated_git`] returns a fresh -//! `Command` with `GIT_CONFIG_GLOBAL=/dev/null` and -//! `GIT_CONFIG_NOSYSTEM=1` pre-applied; child git invocations -//! made *by the production code under test* will inherit these -//! when the parent test set them via the same helper before any -//! production call — i.e. wire `isolated_git` through the test -//! fixtures that build the repository, and the production code's -//! own `git` children pick up the same env via inheritance from -//! the spawned-fixture parent process (us). +//! Nothing here mutates the process environment — `mergify_core::env` +//! says why, and the rest of the workspace is being moved onto the +//! same footing — so this cannot be a `set_var` at process start. Instead [`isolated_git`] returns a fresh `Command` +//! with `GIT_CONFIG_GLOBAL=/dev/null` and `GIT_CONFIG_NOSYSTEM=1` +//! already on it. `Command::env` sets the *child's* environment, so +//! each git invocation carries the isolation itself; nothing is +//! shared and nothing has to be restored. +//! +//! It only covers the git commands that go through it. A `git` child +//! spawned by production code under test builds its own environment +//! from ours and sees neither these variables nor a test overlay, so +//! a fixture that needs isolation must create its repository state +//! through this helper. //! //! Practically: call [`isolated_git`] wherever the tests used to //! call `std::process::Command::new("git")`.