refactor(ci): read the CI environment through the funnel - #1839
Merged
mergify[bot] merged 3 commits intoSep 18, 2026
Merged
mergify[bot] merged 3 commits into
mergify[bot] merged 3 commits into
Conversation
Member
Author
|
This pull request is part of a Mergify stack:
|
This was referenced Sep 18, 2026
sileht
added this pull request to stack #1844
September 18, 2026 07:07
Contributor
Merge Protections🟢 All 6 merge protections satisfied — ready to merge. Show 6 satisfied protections🟢 🤖 Continuous Integration
🟢 👀 Review Requirements
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 Reviews
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
`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
sileht
force-pushed
the
devs/sileht/test-env-access-ub/read-ci-env-funnel--668395af
branch
from
September 18, 2026 07:38
0801c3a to
fa102c1
Compare
sileht
had a problem deploying
to
func-tests-live
September 18, 2026 07:39 — with 
GitHub Actions
Error
GitHub Actions
Error
Member
Author
Revision history
|
There was a problem hiding this comment.
🟢 Approval recommended
The environment-read migration is consistent across the changed CI production paths and tests, with no unresolved correctness issues found.
Pull request overview
Refactors mergify-ci to use the shared environment funnel for production reads and exhaustive test overlays.
Changes:
- Replaced direct environment access with
mergify_core::env. - Removed
with_ci_env,CI_ENV_VARS, andtemp-envusage from CI tests. - Updated dependency lockfiles accordingly.
File summaries
| File | Description |
|---|---|
crates/mergify-ci/src/detector.rs |
Routes CI detection through the shared environment module. |
crates/mergify-ci/src/git_refs.rs |
Uses shared environment reads and overlays in tests. |
crates/mergify-ci/src/github_event.rs |
Uses non-empty shared environment lookups. |
crates/mergify-ci/src/github_output.rs |
Uses the shared environment funnel. |
crates/mergify-ci/src/junit_process/command.rs |
Migrates runtime reads and test overlays. |
crates/mergify-ci/src/junit_process/spans.rs |
Updates span tests to use overlays. |
crates/mergify-ci/src/junit_process/split.rs |
Updates split tests to use overlays. |
crates/mergify-ci/src/queue_info.rs |
Updates output-related tests. |
crates/mergify-ci/src/scopes_detect/mod.rs |
Migrates config/debug reads and tests. |
crates/mergify-ci/src/scopes_detect/outputs.rs |
Migrates output environment reads. |
crates/mergify-ci/src/scopes_send.rs |
Replaces CI test helpers with shared overlays. |
crates/mergify-ci/src/testing.rs |
Removes obsolete CI environment helpers. |
crates/mergify-ci/src/tests_quarantine.rs |
Updates async environment test setup. |
crates/mergify-ci/src/tests_show.rs |
Updates async environment test setup. |
crates/mergify-ci/Cargo.toml |
Removes the unused temp-env dependency. |
Cargo.lock |
Refreshes dependency entries. |
Review details
- Files reviewed: 15/16 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…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
force-pushed
the
devs/sileht/test-env-access-ub/read-ci-env-funnel--668395af
branch
from
September 18, 2026 08:13
fa102c1 to
89eaa4a
Compare
sileht
had a problem deploying
to
func-tests-live
September 18, 2026 08:13 — with
GitHub Actions
Error
GitHub Actions
Error
sileht
marked this pull request as ready for review
September 18, 2026 08:17
Base automatically changed from
devs/sileht/test-env-access-ub/read-env-one-funnel-hermetic-test-overlay--b777d519
to
main
September 18, 2026 12:29
jd
approved these changes
Sep 18, 2026
remyduthu
approved these changes
Sep 18, 2026
Contributor
Merge Queue Status
This pull request spent 7 minutes 5 seconds in the queue, including 6 minutes 31 seconds running CI. Required conditions to merge
|
40 tasks
mergify
Bot
deleted the
devs/sileht/test-env-access-ub/read-ci-env-funnel--668395af
branch
September 18, 2026 14:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mergify-ciis where most of the process-environment mutationlived: 27 direct
temp_envcalls plus the 69 that went throughtesting::with_ci_env. All of them now install amergify_core::envoverlay instead, and the production side reads through the same
module.
CI_ENV_VARSgoes with them, andwith_ci_envwith it. That helperexisted 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 acrossdetector::*; new detectorhelpers 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_varsdirectly,which is what they now mean.
detector::non_empty_envgoes too: once the reads route throughmergify_core::envit was a second name forenv::var_non_empty,which the same file calls.
No behaviour changes.
env::varreturnsOptionwherestd::env::varreturnedResult, so== Ok("true")becomes== Some("true")and the.ok().filter(|s| !s.is_empty())chainscollapse 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