From befb4f430837dd6157ecc9146cda2d982049056b Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:56:03 +0200 Subject: [PATCH] refactor(cli): assert the clap env hook is absent without mutating the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two tests that pinned "clap must not read `MERGIFY_*` itself" each exported one variable empty and parsed one argv. They were the last two `temp_env` calls in `mergify-cli`, and the only ones whose reader was a dependency rather than our own code. They are two tests now, at the two altitudes the rule lives at. A walk over the built `clap::Command` tree asserts that no argument anywhere declares `env = "…"`, which catches the hook at declaration and covers every argument rather than `--config` and `--test-exit-code`. A parse of each of those two argvs asserts the observable property the deleted tests asserted, because the walk only sees that one spelling: `default_value_t = std::env::var(…).unwrap_or_default()` reproduces monorepo#33423 exactly and declares no hook. Neither test needs an environment. `MERGIFY_BASE_URL` joins the rest in treating exported-but-empty as unset. `install.sh` already guards the same lever with `[ -n … ]`, so the two halves of one feature disagreed: an empty value built the URL `/latest-release.json`, which reqwest rejects as relative, instead of falling back to the default host. The rest is plumbing: `MERGIFY_CLI_TESTING_UTF8_MODE`, `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE` and `self_update`'s `MERGIFY_BASE_URL` read through `mergify_core::env`. That is the `env` in scope in both files now; `args()` and `current_exe()` are spelled `std::env::`, since neither is the environment. Co-Authored-By: Claude Opus 5 Change-Id: I512c50f33a89380f3d9f4a8f7a37725a875d6804 --- Cargo.lock | 1 - crates/mergify-cli/Cargo.toml | 1 - crates/mergify-cli/src/main.rs | 128 +++++++++++++------------- crates/mergify-cli/src/self_update.rs | 37 +++++++- 4 files changed, 102 insertions(+), 65 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fda3f33a..1516c449 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1811,7 +1811,6 @@ dependencies = [ "serde_json", "serde_yaml_ng", "sha2 0.11.0", - "temp-env", "tempfile", "tokio", "tracing", diff --git a/crates/mergify-cli/Cargo.toml b/crates/mergify-cli/Cargo.toml index f10d5762..3bbdf97d 100644 --- a/crates/mergify-cli/Cargo.toml +++ b/crates/mergify-cli/Cargo.toml @@ -57,7 +57,6 @@ url = { workspace = true } insta = { workspace = true } regex = { workspace = true } serde_yaml_ng = { workspace = true } -temp-env = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread"] } wiremock = { workspace = true } diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 42c6daa4..3fbe63f0 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -8,7 +8,7 @@ //! ``?" suggestion off clap's built-in Levenshtein //! distance. -use std::env; +use mergify_core::env; use std::io::IsTerminal; use std::path::PathBuf; use std::process::ExitCode; @@ -53,7 +53,7 @@ mod self_update; const VERSION: &str = env!("MERGIFY_CLI_VERSION"); fn main() -> ExitCode { - let argv: Vec = env::args().skip(1).collect(); + let argv: Vec = std::env::args().skip(1).collect(); // Test hook used by `test_binary_build.py` to verify the // wheel-installed binary produces UTF-8 output (especially on @@ -2812,12 +2812,12 @@ enum ColorArg { /// `true` when `name` is exported to something other than the empty /// string, which is what means by "present". /// -/// The rule itself lives in `mergify_core::env::var_non_empty`, which -/// is where it is documented and tested; spelling it out a second time -/// here is how the color variables would drift away from every other -/// variable this CLI reads. +/// The rule itself lives in `mergify_core::env`, which is where it is +/// documented and tested; spelling it out a second time here is how +/// the color variables would drift away from every other variable +/// this CLI reads. fn non_empty(name: &str) -> bool { - mergify_core::env::var_non_empty(name).is_some() + env::var_os_non_empty(name).is_some() } /// Whether the log subscriber may emit ANSI on stderr. @@ -3984,8 +3984,9 @@ struct ScopesCliArgs { // `mergify_ci::scopes_detect::resolve_config_path` instead, // where empty correctly falls through to auto-detect. The // matching regression tests are - // `ci_scopes_parses_when_mergify_config_path_env_var_is_empty` - // (clap parse) and + // `no_argument_takes_its_value_from_the_environment` (asks the + // built `Command` whether *any* argument carries an `env` + // attribute, which covers this one) and // `resolve_config_path_treats_empty_env_var_as_unset` // (lower-level resolver). #[arg(long)] @@ -4897,72 +4898,75 @@ mod tests { // or test the terminal the suite happens to run under. } + /// No argument anywhere in the tree may take its value from the + /// environment through clap's `env = "…"` attribute. + /// + /// Twice now that attribute broke a caller who exports the + /// variable empty. `gha-mergify-ci` sets `MERGIFY_CONFIG_PATH=""` + /// when the user pinned no path, and clap read the empty string + /// as a present-but-empty `--config`, aborting with "a value is + /// required for '--config'" (monorepo#33423). Same shape for + /// `MERGIFY_TEST_EXIT_CODE=""` and `--test-exit-code`: "cannot + /// parse integer from empty string". Env lookup belongs in the + /// resolver, where `mergify_core::env::var_non_empty` treats + /// empty as unset. + /// + /// This replaces two tests that each exported one variable empty + /// and parsed one argv. Asking the built `Command` covers every + /// argument rather than those two, and needs no process + /// environment to mutate. #[test] - fn ci_scopes_parses_when_mergify_config_path_env_var_is_empty() { - // Regression for monorepo#33423 / gha-mergify-ci: - // the action sets `MERGIFY_CONFIG_PATH=""` (empty) when - // the caller didn't pin a config path, expecting - // auto-detect. The previous `ScopesCliArgs::config` - // declaration used `env = "MERGIFY_CONFIG_PATH"` on - // clap's side, which interpreted the empty env value as - // a present-but-empty `--config` flag and exited parsing - // with `a value is required for '--config'`. The clap - // env hook has been dropped — env lookup lives inside - // `scopes_detect::resolve_config_path` where empty is - // correctly treated as unset. Pin that here so the hook - // can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_CONFIG_PATH", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "scopes".to_string(), - "--write".to_string(), - "scopes.json".to_string(), - ]) - .expect("argv parses with empty MERGIFY_CONFIG_PATH") - }); + fn no_argument_takes_its_value_from_the_environment() { + fn walk(cmd: &clap::Command, path: &str, found: &mut Vec) { + for arg in cmd.get_arguments() { + if let Some(var) = arg.get_env() { + found.push(format!( + "{path} {} <- {}", + arg.get_id(), + var.to_string_lossy() + )); + } + } + for sub in cmd.get_subcommands() { + walk(sub, &format!("{path} {}", sub.get_name()), found); + } + } + + let mut found = Vec::new(); + walk(&CliRoot::command(), "mergify", &mut found); + assert!(found.is_empty(), "clap env hooks found: {found:#?}"); + } + + /// The observable half of the rule above, for the two arguments + /// it was reported on. + /// + /// The walk asks clap whether an `env = "…"` hook is declared, + /// which is the spelling that caused both regressions but not the + /// only one: `default_value_t = std::env::var(…).unwrap_or_default()` + /// or a `value_parser` that reads the environment reproduce it + /// exactly and declare no hook. This asserts what the user sees + /// instead. It needs no environment of its own — with the + /// variable unset, any of those spellings still surfaces a + /// present-but-empty value where `None` is required. + #[test] + fn an_omitted_flag_stays_omitted() { + let parsed = CliRoot::try_parse_from(["mergify", "ci", "scopes", "--write", "scopes.json"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiScopes(opts)) = dispatch_from_parsed(parsed) else { panic!("ci scopes must dispatch natively"); }; - // `--config` was never supplied; the empty env var must - // not surface as a value (which would change the - // downstream resolver's branch). assert!(opts.config.is_none(), "got: {:?}", opts.config); - } - #[test] - fn ci_junit_process_parses_when_mergify_test_exit_code_env_var_is_empty() { - // Second instance of the same class of regression as - // `ci_scopes_parses_when_…`: `gha-mergify-ci` exports - // `MERGIFY_TEST_EXIT_CODE=""` when the previous step - // didn't produce a runner exit code. Previously the clap - // `env = "MERGIFY_TEST_EXIT_CODE"` attribute on - // `--test-exit-code` tried to parse `""` as `i32` and - // exited parsing with `invalid value '' for - // '--test-exit-code': cannot parse integer from empty - // string`. The clap env hook has been dropped — env - // lookup lives in `junit_process::command::resolve_test_exit_code` - // where empty is correctly treated as `None`. Pin that - // here so the hook can't sneak back in. - let parsed = temp_env::with_var("MERGIFY_TEST_EXIT_CODE", Some(""), || { - CliRoot::try_parse_from([ - "mergify".to_string(), - "ci".to_string(), - "junit-process".to_string(), - "report.xml".to_string(), - ]) - .expect("argv parses with empty MERGIFY_TEST_EXIT_CODE") - }); + let parsed = CliRoot::try_parse_from(["mergify", "ci", "junit-process", "report.xml"]) + .expect("argv parses"); let Dispatch::Native(NativeCommand::CiJunitProcess(opts)) = dispatch_from_parsed(parsed) else { panic!("ci junit-process must dispatch natively"); }; - // `--test-exit-code` was never supplied; the empty env - // var must not surface as a value. assert!( opts.test_exit_code.is_none(), "got: {:?}", - opts.test_exit_code, + opts.test_exit_code ); } diff --git a/crates/mergify-cli/src/self_update.rs b/crates/mergify-cli/src/self_update.rs index 7c0b0000..f3e96d03 100644 --- a/crates/mergify-cli/src/self_update.rs +++ b/crates/mergify-cli/src/self_update.rs @@ -35,6 +35,7 @@ use std::path::Path; use std::time::Duration; use mergify_core::CliError; +use mergify_core::env; use serde::Deserialize; use sha2::Digest; use sha2::Sha256; @@ -58,7 +59,7 @@ const BASE_URL_ENV: &str = "MERGIFY_BASE_URL"; /// read from the response, never reconstructed, so we only need to /// know where the metadata lives. fn latest_release_url() -> String { - if let Ok(base) = std::env::var(BASE_URL_ENV) { + if let Some(base) = env::var_non_empty(BASE_URL_ENV) { format!("{base}/latest-release.json") } else { format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") @@ -443,6 +444,40 @@ mod tests { } } + #[test] + fn latest_release_url_falls_back_when_the_base_url_is_empty() { + // `gha-mergify-ci` exports unset variables as `""`, so an + // empty `MERGIFY_BASE_URL` must mean "no fixture" and not + // "fetch from `/latest-release.json`". This is the + // `var_non_empty` half of the empty-string rule; the + // overlay is what lets the case be written at all, since + // nothing may mutate the process environment. + let url = env::testing::with_var(BASE_URL_ENV, Some(""), latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + + #[test] + fn latest_release_url_uses_a_non_empty_base_url() { + let url = env::testing::with_var( + BASE_URL_ENV, + Some("https://example.test"), + latest_release_url, + ); + assert_eq!(url, "https://example.test/latest-release.json"); + } + + #[test] + fn latest_release_url_defaults_when_the_base_url_is_unset() { + let url = env::testing::with_no_vars(latest_release_url); + assert_eq!( + url, + format!("{DEFAULT_API_BASE}/repos/{REPO}/releases/latest") + ); + } + #[test] fn select_asset_matches_versioned_name() { let assets = [