merge queue: checking #1843 on main (7fe8e67) - #1853
Closed
mergify[bot] wants to merge 4 commits into
Closed
mergify[bot] wants to merge 4 commits into
mergify[bot] wants to merge 4 commits into
Conversation
…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
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
1 participant
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.
🎉 This pull request has been checked successfully and will be merged soon. 🎉
#1843 is queued for merge on branch main (7fe8e67).
This pull request has been created by Mergify to check the mergeability of #1843.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.
Required conditions of queue rule
defaultfor merge:github-review-approved[🛡 GitHub branch protection]github-review-approved[🛡 GitHub repository ruleset ruleRequire pull request for default branch]Enforce conventional commit]:title ~= ^(fix|feat|internal|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?!?:👀 Review Requirements]:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]📕 PR description]:body ~= (?ms:.{48,})🔎 Reviews]:#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0🤖 Continuous Integration]:check-success=ci-gateRequired conditions to stay in the queue:
base=maingithub-review-approved[🛡 GitHub branch protection]github-review-approved[🛡 GitHub repository ruleset ruleRequire pull request for default branch]label!=manual mergeEnforce conventional commit]:title ~= ^(fix|feat|internal|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?!?:👀 Review Requirements]:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]📕 PR description]:body ~= (?ms:.{48,})🔎 Reviews]:#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0🤖 Continuous Integration]:check-success=ci-gate