merge queue: checking #1839 on main (6d95271) by mergify[bot] · Pull Request #1848 · Mergifyio/mergify-cli

mergify · 2026-09-18T14:03:46Z

🎉 This pull request has been checked successfully and will be merged soon. 🎉

#1839 is queued for merge on branch main (6d95271).

This pull request has been created by Mergify to check the mergeability of #1839.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.

Required conditions of queue rule default for merge:

Required conditions to stay in the queue:

---
checking_base_sha: 6d95271c6ccd5d4985cc7b2cdef233e1edec7f7f
previous_check_retries: []
previous_failed_batches: []
pull_requests:
  - number: 1839
    scopes: []
scopes: []
...

`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`. <https://no-color.org> 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 <noreply@anthropic.com> Change-Id: Ib099b7044861960ca2c9e85f443072914b333c20

…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 <noreply@anthropic.com> Change-Id: Ib777d5196b912ba69690a03554372442cdbc04be

`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 <noreply@anthropic.com> Change-Id: I668395af2157ffd73b41a70834a9d77dd504a2db

sileht and others added 4 commits September 18, 2026 09:21

Merge of #1839

be55680

mergify Bot deployed to Mergify Merge Protections September 18, 2026 14:03 Active

mergify Bot deployed to func-tests-live September 18, 2026 14:03 Active

mergify Bot mentioned this pull request Sep 18, 2026

refactor(ci): read the CI environment through the funnel #1839

Merged

mergify Bot closed this Sep 18, 2026

mergify Bot deleted the mergify/merge-queue/486b7d14e2 branch September 18, 2026 14:10

Navigation Menu

merge queue: checking #1839 on main (6d95271) - #1848

merge queue: checking #1839 on main (6d95271)#1848
mergify[bot] wants to merge 4 commits into
mainfrom
mergify/merge-queue/486b7d14e2

mergify Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Conversation

mergify Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

mergify Bot commented Sep 18, 2026 •

edited

Loading