chore(lint): make the next process-environment read fail the build by sileht · Pull Request #1843 · Mergifyio/mergify-cli

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

A convention nobody can enforce decays, and this one had already
decayed once: AGENTS.md told contributors to use temp_env "never
the unsound process-global std::env::set_var (unsafe_code = "forbid" bans it anyway)". Both halves were wrong. temp_env calls
set_var, from inside a dependency, where the workspace's
forbid(unsafe_code) does not reach.

unsafe_code = "forbid" was already the guard against our code
mutating the environment: set_var and remove_var are unsafe fn.
So the new coverage is on the read side, which is what drifts:

clippy.toml disallows std::env::var, var_os, vars and
vars_os, each with its replacement in the message, so a new
direct read fails the build with a pointer to mergify_core::env.
set_var / remove_var are listed as documentation of a rule
forbid(unsafe_code) already enforces, and to say what someone
relaxing that lint would be unlocking. set_current_dir joins
them: it is the safe process-global mutator, it races every
relative path in every other test thread, and two existing comments
(mergify-config/src/paths.rs, mergify-ci/src/scopes_detect/ changed_files.rs) show contributors were already tempted by it.
deny.toml bans the temp-env crate, since no lint over our
source can see a set_var that happens inside a dependency. It
blocks the one we used, not the class: cargo-deny cannot express
"nothing that calls setenv", and the file says so.

Two explicit allows, both for reads that are not the process
environment. build.rs reads cargo's environment for this one
invocation and cannot depend on mergify-core. live_smoke.rs
copies the parent's environment into the child it spawns, and does it
with vars_os now: vars panics on a single variable that is not
valid Unicode, which would have taken down every case in that file
before it spawned anything.

AGENTS.md states the rule with its reason, since the next person
needs the argument and not just the rule, and with the two things
that are easy to get wrong: an overlay hides the host environment,
and it does not reach a dependency or a child process.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

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

This pull request is part of a Mergify stack:

#	Pull Request	Link
1	refactor(cli): assert the clap env hook is absent without mutating the environment	#1841
2	refactor(auth): read the environment through the funnel	#1842
3	chore(lint): make the next process-environment read fail the build	#1843	👈

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

sileht · 2026-09-18T07:39:11Z

Revision history

#	Type	Changes	Date
1	initial	`2361d62`	2026-09-18 07:38 UTC
2	rebase	`2361d62 → e94d90b` (rebase only)	2026-09-18 07:38 UTC
3	rebase	`e94d90b → 3aefe93` (rebase only)	2026-09-18 08:13 UTC
4	rebase	`3aefe93 → 1cd2331` (rebase only)	2026-09-21 05:43 UTC

Copilot

🟡 Changes recommended

The cargo-deny ban and Clippy enforcement need correction before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request adds safeguards against unsafe process-environment access and removes the temp-env dependency.

Changes:

Adds Clippy and cargo-deny restrictions.
Updates build scripts and smoke tests with explicit exceptions.
Documents environment-testing practices.

File summaries

File	Reviewed change
`deny.toml`	Adds the `temp-env` ban; critical finding (3 votes): use `name` instead of `crate`.
`crates/mergify-cli/tests/live_smoke.rs`	Uses `vars_os` for child environment inheritance.
`crates/mergify-cli/build.rs`	Allows the build-time environment read.
`clippy.toml`	Defines forbidden APIs; moderate finding (2 votes): enable `clippy::disallowed_methods`; nit (1 vote): complete both messages.
`Cargo.toml`	Removes the `temp-env` dependency.
`AGENTS.md`	Documents safe environment-testing practices.

Review details

Suppressed comments (1)

clippy.toml:23

Both lint reasons end with building a child process's, which is an incomplete possessive and leaves the exception unclear. Say building a child process's environment in both entries.

    { path = "std::env::vars", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" },
    { path = "std::env::vars_os", reason = "read it through `mergify_core::env`; iterating the whole environment is only right when building a child process's, and needs an explicit allow" },

Files reviewed: 6/6 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…e environment The two tests that pinned "clap must not read `MERGIFY_*` itself" each exported one variable empty and parsed one argv. They were the last two `temp_env` calls in `mergify-cli`, and the only ones whose reader was a dependency rather than our own code. They are two tests now, at the two altitudes the rule lives at. A walk over the built `clap::Command` tree asserts that no argument anywhere declares `env = "…"`, which catches the hook at declaration and covers every argument rather than `--config` and `--test-exit-code`. A parse of each of those two argvs asserts the observable property the deleted tests asserted, because the walk only sees that one spelling: `default_value_t = std::env::var(…).unwrap_or_default()` reproduces monorepo#33423 exactly and declares no hook. Neither test needs an environment. `MERGIFY_BASE_URL` joins the rest in treating exported-but-empty as unset. `install.sh` already guards the same lever with `[ -n … ]`, so the two halves of one feature disagreed: an empty value built the URL `/latest-release.json`, which reqwest rejects as relative, instead of falling back to the default host. The rest is plumbing: `MERGIFY_CLI_TESTING_UTF8_MODE`, `NO_COLOR` / `FORCE_COLOR` / `CLICOLOR_FORCE` and `self_update`'s `MERGIFY_BASE_URL` read through `mergify_core::env`. That is the `env` in scope in both files now; `args()` and `current_exe()` are spelled `std::env::`, since neither is the environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Change-Id: I512c50f33a89380f3d9f4a8f7a37725a875d6804

`mergify-auth` landed while this stack was in flight and its production side already reads through `mergify_core::env`, since `var_non_empty` predates the funnel. Only its tests were left: `machine`'s `COMPUTERNAME` / `HOSTNAME` chain, `browser`'s `SSH_CONNECTION` / `DISPLAY` / `WAYLAND_DISPLAY` probes, and `with_mergify_token`. They install an overlay now, like everywhere else, so the crate stops mutating the process environment and drops its `temp-env` dependency. `with_mergify_token` keeps its shape: the overlay is on this thread and the `current_thread` runtime it builds drives the future on that same thread, so the closure form still works and no call site changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Change-Id: I7d4c755cee8cc90db511ea671e5c23215300c03c

A convention nobody can enforce decays, and this one had already decayed once: `AGENTS.md` told contributors to use `temp_env` "never the unsound process-global `std::env::set_var` (`unsafe_code = "forbid"` bans it anyway)". Both halves were wrong. `temp_env` calls `set_var`, from inside a dependency, where the workspace's `forbid(unsafe_code)` does not reach. `unsafe_code = "forbid"` was already the guard against *our* code mutating the environment: `set_var` and `remove_var` are `unsafe fn`. So the new coverage is on the read side, which is what drifts: - `clippy.toml` disallows `std::env::var`, `var_os`, `vars` and `vars_os`, each with its replacement in the message, so a new direct read fails the build with a pointer to `mergify_core::env`. `set_var` / `remove_var` are listed as documentation of a rule `forbid(unsafe_code)` already enforces, and to say what someone relaxing that lint would be unlocking. `set_current_dir` joins them: it is the *safe* process-global mutator, it races every relative path in every other test thread, and two existing comments (`mergify-config/src/paths.rs`, `mergify-ci/src/scopes_detect/ changed_files.rs`) show contributors were already tempted by it. - `deny.toml` bans the `temp-env` crate, since no lint over our source can see a `set_var` that happens inside a dependency. It blocks the one we used, not the class: cargo-deny cannot express "nothing that calls setenv", and the file says so. Two explicit allows, both for reads that are not the process environment. `build.rs` reads cargo's environment for this one invocation and cannot depend on `mergify-core`. `live_smoke.rs` copies the parent's environment into the child it spawns, and does it with `vars_os` now: `vars` panics on a single variable that is not valid Unicode, which would have taken down every case in that file before it spawned anything. `AGENTS.md` states the rule with its reason, since the next person needs the argument and not just the rule, and with the two things that are easy to get wrong: an overlay hides the host environment, and it does not reach a dependency or a child process. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Change-Id: I185b16b8d8b1c65f325301628c1e6b1d1de789c1

mergify · 2026-09-21T12:55:25Z

sileht mentioned this pull request Sep 18, 2026

refactor(tui): resolve the color env overrides at the CLI entry point #1837

Merged

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

sileht mentioned this pull request Sep 18, 2026

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

Merged

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

This was referenced Sep 18, 2026

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

Merged

refactor(stack): read the environment through the funnel #1840

Merged

refactor(cli): assert the clap env hook is absent without mutating the environment #1841

Merged

sileht mentioned this pull request Sep 18, 2026

refactor(auth): read the environment through the funnel #1842

Merged

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

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

sileht force-pushed the devs/sileht/test-env-access-ub/make-next-process-env-read-fail-build--185b16b8 branch from 2361d62 to e94d90b Compare September 18, 2026 07:38

Copilot AI lite review requested due to automatic review settings 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 deny.toml

Comment thread clippy.toml

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

sileht force-pushed the devs/sileht/test-env-access-ub/make-next-process-env-read-fail-build--185b16b8 branch from e94d90b to 3aefe93 Compare September 18, 2026 08:13

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

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

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

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

sileht and others added 2 commits September 21, 2026 07:13

sileht force-pushed the devs/sileht/test-env-access-ub/make-next-process-env-read-fail-build--185b16b8 branch from 3aefe93 to 1cd2331 Compare September 21, 2026 05:43

sileht had a problem deploying to func-tests-live September 21, 2026 05:43 — with GitHub Actions Error

sileht deployed to func-tests-live September 21, 2026 05:43 — with GitHub Actions Active

mergify Bot deployed to Mergify Merge Protections September 21, 2026 05:44 Active

kozlek approved these changes Sep 21, 2026

View reviewed changes

mergify Bot requested a review from a team September 21, 2026 08:39

Base automatically changed from devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c to main September 21, 2026 12:47

JulianMaurin approved these changes Sep 21, 2026

View reviewed changes

mergify Bot added the queued label Sep 21, 2026

mergify Bot mentioned this pull request Sep 21, 2026

merge queue: checking #1843 on main (7fe8e67) #1853

Closed

40 tasks

mergify Bot merged commit bef8713 into main Sep 21, 2026
22 of 42 checks passed

mergify Bot deleted the devs/sileht/test-env-access-ub/make-next-process-env-read-fail-build--185b16b8 branch September 21, 2026 13:05

mergify Bot removed the queued label Sep 21, 2026

Navigation Menu

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

chore(lint): make the next process-environment read fail the build#1843
mergify[bot] merged 3 commits into
mainfrom
devs/sileht/test-env-access-ub/make-next-process-env-read-fail-build--185b16b8

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!

mergify Bot commented Sep 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

This branch was successfully deployed

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 (1)

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

This branch was successfully deployed

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 21, 2026 •

edited

Loading