feat(core): read the environment through one funnel, with a hermetic test overlay by sileht · Pull Request #1838 · Mergifyio/mergify-cli

sileht · 2026-09-18T07:07:15Z

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

sileht · 2026-09-18T07:07:23Z

This pull request is part of a Mergify stack:

#	Pull Request	Link
1	refactor(tui): resolve the color env overrides at the CLI entry point	#1837
2	feat(core): read the environment through one funnel, with a hermetic test overlay	#1838	👈
3	refactor(ci): read the CI environment through the funnel	#1839
4	refactor(stack): read the environment through the funnel	#1840
5	refactor(cli): assert the clap env hook is absent without mutating the environment	#1841
6	refactor(auth): read the environment through the funnel	#1842
7	chore(lint): make the next process-environment read fail the build	#1843

mergify · 2026-09-18T07:07:48Z

`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 · 2026-09-18T07:39:05Z

Revision history

#	Type	Changes	Date
1	initial	`46785e0`	2026-09-18 07:38 UTC
2	rebase	`46785e0 → 347397a` (rebase only)	2026-09-18 07:38 UTC
3	content	`347397a → 6f8c35c`	2026-09-18 08:13 UTC

Copilot

🟡 Changes recommended

Critical compilation and async overlay correctness issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Centralizes environment access behind a thread-local overlay and makes GitHub CLI token resolution injectable for hermetic tests.

Changes:

Adds synchronous and asynchronous environment overlays.
Migrates authentication tests away from temp_env.
Removes obsolete temp-env dependencies.

File summaries

File	Description
`crates/mergify-core/src/env.rs`	Environment funnel, overlays, and tests
`crates/mergify-core/src/auth.rs`	Injectable GitHub CLI token resolution
`crates/mergify-core/Cargo.toml`	Removes `temp-env`
`crates/mergify-config/Cargo.toml`	Removes `temp-env`
`Cargo.lock`	Updates dependency entries

Review details

Suppressed comments (3)

crates/mergify-core/src/env.rs:403

This restoration check has the same non-hermetic assumption as the earlier test: PATH may legitimately be unset, so the test reports a failure even though the overlay was restored correctly. Compare the value captured before the panic with the value after unwinding rather than requiring PATH to exist.

        assert!(
            var("PATH").is_some(),
            "the overlay outlived the panic that unwound through it"
        );

crates/mergify-core/src/env.rs:113

This claim is not enforced in the current workspace: there is no clippy.toml, [workspace.lints.clippy] does not enable clippy::disallowed_methods, and direct std::env::var* calls still exist in command crates. Without adding the lint/config (and migrating those callers), future code can bypass this funnel, so either add the enforcement in this change or make the comment describe a future step.

            // workspace; `clippy.toml` disallows `std::env::var*`
            // everywhere else so every other caller lands here.
            #[allow(clippy::disallowed_methods)]
            std::env::var_os(name)

crates/mergify-core/src/env.rs:104

On Windows, environment variable names are case-insensitive, but this replacement map uses exact OsString keys. For example, with_var("Path", Some(...), || var("PATH")) returns None even though the real std::env::var_os("PATH") would find the value, so the overlay does not preserve the platform's environment semantics. Normalize keys/lookups (or perform a Windows case-insensitive lookup) and add a regression test.

                map.get(OsStr::new(name)).cloned()

Files reviewed: 4/5 changed files
Comments generated: 3
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

sileht · 2026-09-18T08:15:35Z

The three suppressed comments in the Copilot review body, which have no thread to reply on:

env.rs:403, PATH may legitimately be unset — same finding as the thread on line 362, and correct. Both tests failed under env -i on the previous head. Fixed there; details in that thread.

env.rs:113, the comment names enforcement that does not exist yet — right, at this commit there is no clippy.toml. It arrives in #1843, five commits up. Dropped the forward reference so the comment is true wherever the stack is bisected to:

The single sanctioned process-environment read in the workspace: every other caller goes through the functions above.

env.rs:104, Windows environment names are case-insensitive — real, and not fixed. Documented in the module instead:

Keys match exactly. On Windows the real environment does not — std::env::var_os("PATH") finds a Path — so an overlay on Path leaves var("PATH") reading the host's. Nothing here reads a name in a case other than the one it writes, so no call site can tell the difference, and matching the platform would mean a Windows-only normalisation that this suite never runs: the unit tests are a Linux and macOS job, Windows only builds the binary.

The reasoning is that last clause. A #[cfg(windows)] normalisation path would compile in CI but never execute a single assertion, so the regression test you ask for could not run either. That trades a documented gap for an untested one. If the suite ever runs on Windows this becomes worth doing, and the note says what to do.

mergify · 2026-09-18T12:21:48Z

mergify Bot had a problem deploying to Mergify Merge Protections September 18, 2026 07:07 Failure

sileht deployed to func-tests-live September 18, 2026 07:07 — with GitHub Actions Active

sileht added this pull request to stack #1844 September 18, 2026 07:07

sileht mentioned this pull request Sep 18, 2026

chore(lint): make the next process-environment read fail the build #1843

Open

mergify Bot requested a review from a team September 18, 2026 07:17

Copilot AI lite review requested due to automatic review settings September 18, 2026 07:38

sileht force-pushed the devs/sileht/test-env-access-ub/read-env-one-funnel-hermetic-test-overlay--b777d519 branch from 46785e0 to 347397a Compare September 18, 2026 07:38

sileht had a problem deploying to func-tests-live September 18, 2026 07:39 — with GitHub Actions Error

sileht deployed to func-tests-live September 18, 2026 07:39 — with GitHub Actions Active

mergify Bot had a problem deploying to Mergify Merge Protections September 18, 2026 07:39 Failure

Copilot started reviewing on behalf of sileht September 18, 2026 07:39 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread crates/mergify-core/src/env.rs

Comment thread crates/mergify-core/src/env.rs

Comment thread crates/mergify-core/src/env.rs Outdated

mergify Bot added the review threads unresolved label Sep 18, 2026

sileht force-pushed the devs/sileht/test-env-access-ub/read-env-one-funnel-hermetic-test-overlay--b777d519 branch from 347397a to 6f8c35c Compare September 18, 2026 08:13

sileht deployed to func-tests-live September 18, 2026 08:13 — with GitHub Actions Active

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

mergify Bot removed the review threads unresolved label Sep 18, 2026

sileht marked this pull request as ready for review September 18, 2026 08:17

kozlek approved these changes Sep 18, 2026

View reviewed changes

mergify Bot requested a review from a team September 18, 2026 09:12

Base automatically changed from devs/sileht/test-env-access-ub/resolve-color-env-overrides-cli-entry-point--b099b704 to main September 18, 2026 09:45

jd approved these changes Sep 18, 2026

View reviewed changes

mergify Bot mentioned this pull request Sep 18, 2026

merge queue: checking #1838 on main (d963e8b) #1847

Closed

40 tasks

mergify Bot added the queued label Sep 18, 2026

mergify Bot merged commit 6d95271 into main Sep 18, 2026
22 checks passed

mergify Bot deleted the devs/sileht/test-env-access-ub/read-env-one-funnel-hermetic-test-overlay--b777d519 branch September 18, 2026 12:29

mergify Bot removed the queued label Sep 18, 2026

Navigation Menu

feat(core): read the environment through one funnel, with a hermetic test overlay - #1838

feat(core): read the environment through one funnel, with a hermetic test overlay#1838
mergify[bot] merged 2 commits into
mainfrom
devs/sileht/test-env-access-ub/read-env-one-funnel-hermetic-test-overlay--b777d519

sileht commented Sep 18, 2026

Uh oh!

sileht commented Sep 18, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Sep 18, 2026 •

edited

Loading

🟢 🤖 Continuous Integration

🟢 👀 Review Requirements

🟢 Enforce conventional commit

🟢 🔎 Reviews

🟢 📕 PR description

🟢 🚦 Auto-queue

Uh oh!

sileht commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sileht commented Sep 18, 2026

Uh oh!

mergify Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

sileht commented Sep 18, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Merge Protections

🟢 🤖 Continuous Integration

🟢 👀 Review Requirements

🟢 Enforce conventional commit

🟢 🔎 Reviews

🟢 📕 PR description

🟢 🚦 Auto-queue

Uh oh!

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

Uh oh!

Revision history

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sileht commented Sep 18, 2026

Uh oh!

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

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

sileht commented Sep 18, 2026 •

edited

Loading

mergify Bot commented Sep 18, 2026 •

edited

Loading

sileht commented Sep 18, 2026 •

edited

Loading

mergify Bot commented Sep 18, 2026 •

edited

Loading