refactor(cli): assert the clap env hook is absent without mutating the environment - #1841
Open
Conversation
This was referenced Sep 18, 2026
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🔴 3 of 7 protections blocking · waiting on 👀 reviews and ⛓️ dependency
🔴 ⛓️ Depends-On RequirementsWaiting for
This rule is failing.Requirement based on the presence of
🔴 👀 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. |
sileht
force-pushed
the
devs/sileht/test-env-access-ub/assert-clap-env-hook-absent-without-mutating-env--512c50f3
branch
from
September 18, 2026 07:38
7c98c5e to
7d63c95
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
Two review comments remain regarding documentation and regression-test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors CLI environment handling through mergify_core::env, removes environment-mutating tests, and treats empty MERGIFY_BASE_URL as unset.
Changes:
- Adds Clap command-tree and parsing assertions.
- Updates environment access in CLI and self-update code.
- Removes the unused
temp-envdependency.
File summaries
| File | Summary |
|---|---|
crates/mergify-cli/src/self_update.rs |
Handles empty base URLs; add focused regression tests. |
crates/mergify-cli/src/main.rs |
Updates environment access and Clap tests; refresh the stale deleted-test reference. |
crates/mergify-cli/Cargo.toml |
Removes temp-env. |
Cargo.lock |
Updates dependency metadata. |
Review details
- Files reviewed: 3/4 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
sileht
force-pushed
the
devs/sileht/test-env-access-ub/assert-clap-env-hook-absent-without-mutating-env--512c50f3
branch
from
September 18, 2026 08:13
7d63c95 to
083b61b
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
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 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_envcalls inmergify-cli, and the only ones whosereader 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::Commandtree asserts that no argumentanywhere declares
env = "…", which catches the hook at declarationand covers every argument rather than
--configand--test-exit-code. A parse of each of those two argvs asserts theobservable property the deleted tests asserted, because the walk
only sees that one spelling:
default_value_t = std::env::var(…).unwrap_or_default()reproduces monorepo#33423exactly and declares no hook. Neither test needs an environment.
MERGIFY_BASE_URLjoins the rest in treating exported-but-empty asunset.
install.shalready guards the same lever with[ -n … ], sothe two halves of one feature disagreed: an empty value built the URL
/latest-release.json, which reqwest rejects as relative, instead offalling back to the default host.
The rest is plumbing:
MERGIFY_CLI_TESTING_UTF8_MODE,NO_COLOR/FORCE_COLOR/CLICOLOR_FORCEandself_update'sMERGIFY_BASE_URLread throughmergify_core::env. That is theenvin scope in both files now;args()andcurrent_exe()arespelled
std::env::, since neither is the environment.Co-Authored-By: Claude Opus 5 noreply@anthropic.com