refactor(auth): read the environment through the funnel - #1842
Merged
mergify[bot] merged 2 commits intoSep 21, 2026
Merged
Conversation
This was referenced Sep 18, 2026
Member
Author
|
This pull request is part of a Mergify stack:
|
Contributor
Merge Protections🟢 All 6 merge protections satisfied — ready to merge. Show 6 satisfied protections🟢 🤖 Continuous Integration
🟢 👀 Review Requirements
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 Reviews
🟢 📕 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/read-env-funnel--7d4c755c
branch
from
September 18, 2026 07:38
c53311d to
2de27ff
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.
🟢 Approval recommended
No blocking issues were identified; the remaining comment is a minor cleanup nit.
Pull request overview
Refactors mergify-auth tests to use thread-local environment overlays instead of mutating process environment state.
Changes:
- Migrates machine, browser, and token tests to environment overlays.
- Preserves current-thread async token test behavior.
- Removes the crate-level
temp-envdependency and lockfile entry.
File summaries
| File | Description |
|---|---|
crates/mergify-auth/src/machine.rs |
Uses overlays for hostname tests. |
crates/mergify-auth/src/lib.rs |
Updates token-dependent async tests. |
crates/mergify-auth/src/browser.rs |
Uses overlays for browser environment tests. |
crates/mergify-auth/Cargo.toml |
Removes the direct temp-env dependency. |
Cargo.lock |
Removes the unused package entry. |
Review details
Suppressed comments (1)
crates/mergify-auth/Cargo.toml:27
- The crate-level dependency is removed, but
temp-env = "0.3"is still declared in the workspace dependency table atCargo.toml:75, and this is now the only repository reference to it. Please remove the unused workspace entry as part of dropping this dependency so the workspace manifest does not retain dead dependency configuration.
serde_json = { workspace = true }
- Files reviewed: 4/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sileht
force-pushed
the
devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c
branch
from
September 18, 2026 08:13
2de27ff to
6c614fe
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
…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
sileht
force-pushed
the
devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c
branch
from
September 21, 2026 05:43
6c614fe to
8ed6d62
Compare
sileht
had a problem deploying
to
func-tests-live
September 21, 2026 05:43 — with
GitHub Actions
Error
GitHub Actions
Error
Base automatically changed from
devs/sileht/test-env-access-ub/assert-clap-env-hook-absent-without-mutating-env--512c50f3
to
main
September 21, 2026 08:21
kozlek
approved these changes
Sep 21, 2026
JulianMaurin
approved these changes
Sep 21, 2026
Contributor
Merge Queue Status
This pull request spent 10 minutes 10 seconds in the queue, including 9 minutes 2 seconds running CI. Required conditions to merge
|
40 tasks
mergify
Bot
deleted the
devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c
branch
September 21, 2026 12:47
This branch was successfully deployed
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.
mergify-authlanded while this stack was in flight and itsproduction side already reads through
mergify_core::env, sincevar_non_emptypredates the funnel. Only its tests were left:machine'sCOMPUTERNAME/HOSTNAMEchain,browser'sSSH_CONNECTION/DISPLAY/WAYLAND_DISPLAYprobes, andwith_mergify_token.They install an overlay now, like everywhere else, so the crate
stops mutating the process environment and drops its
temp-envdependency.
with_mergify_tokenkeeps its shape: the overlay is onthis thread and the
current_threadruntime it builds drives thefuture on that same thread, so the closure form still works and no
call site changes.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com