Conversation
This was referenced Sep 18, 2026
Member
Author
|
This pull request is part of a Mergify stack:
|
Contributor
Merge Protections🔴 2 of 6 protections blocking · waiting on 👀 reviews
🔴 👀 Review RequirementsWaiting for
This rule is failing.
🔴 🔎 ReviewsWaiting for
This rule is failing.
Show 4 satisfied protections🟢 🤖 Continuous Integration
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 📕 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-env-funnel--1a360878
branch
from
September 18, 2026 07:38
1a8b4fd to
3dbf1c7
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.
🟡 Changes recommended
Scope the workspace-wide environment-isolation documentation claim.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors mergify-stack environment access through mergify_core::env and replaces test environment mutation with overlays.
Changes:
- Centralizes editor and GitHub server environment lookups.
- Migrates
notetests to environment overlays. - Removes the unused
temp-envdependency. - Clarifies per-command Git environment isolation.
File summaries
| File | Summary |
|---|---|
crates/mergify-stack/src/test_env.rs |
Updates environment-isolation documentation; the workspace-wide claim needs scoping. |
crates/mergify-stack/src/stack_context.rs |
Uses centralized environment lookup. |
crates/mergify-stack/src/commands/note.rs |
Uses funnel-based editor lookup and test overlays. |
crates/mergify-stack/Cargo.toml |
Removes the unused development dependency. |
Cargo.lock |
Updates the locked dependency list. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- 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
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 <noreply@anthropic.com> Change-Id: I1a360878f5ad6538ebfcae5e1092e2155a36b263
sileht
force-pushed
the
devs/sileht/test-env-access-ub/read-env-funnel--1a360878
branch
from
September 18, 2026 08:13
3dbf1c7 to
8e184c2
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-ci-env-funnel--668395af
to
main
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.
The editor chain (
GIT_EDITOR,VISUAL,EDITOR) andMERGIFY_GITHUB_SERVERnow read throughmergify_core::env, andnote's threetemp_envcalls install an overlay instead ofmutating the process environment.
note::non_empty_envmoves into the funnel asvar_os_non_empty.The "empty means unset" rule is the one
mergify_core::env's moduledoc calls twice-bitten and regression-prone, so it should not have a
second local copy. The
OsStringreturn, which exists so an editorpath reaches
Commandwithout a UTF-8 round trip, was the onlyreason it could not already call
var_non_empty.mergify-stackis the crate that already knew about this. Itstest_env.rsexists because the workspace forbidsunsafe_codeandso it could not
set_varGIT_CONFIG_GLOBALat process start: itpasses the variable per-
Commandinstead. That is the same answerthis 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::envworks; it now says what the helperdoes and does not cover.
MERGIFY_GITHUB_SERVERstill crosses a real process boundary: theintegration tests set it with
Command::envon the binary theyspawn, and a child's environment is not a shared one. Nothing there
changes.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com