From 092133b95e073af50f8e67e1817604d724a4f97e Mon Sep 17 00:00:00 2001 From: Mehdi ABAAKOUK Date: Wed, 9 Sep 2026 21:44:17 +0200 Subject: [PATCH] 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]