Skip to content

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

Open
sileht wants to merge 1 commit into
devs/sileht/test-env-access-ub/assert-clap-env-hook-absent-without-mutating-env--512c50f3from
devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c
Open

sileht wants to merge 1 commit into
devs/sileht/test-env-access-ub/assert-clap-env-hook-absent-without-mutating-env--512c50f3from
devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c

Conversation

@sileht

@sileht sileht commented Sep 18, 2026

Copy link
Copy Markdown
Member

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

@sileht

sileht commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

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

mergify Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 3 of 7 protections blocking · waiting on 👀 reviews and ⛓️ dependency

Protection Waiting on
🔴 ⛓️ Depends-On Requirements ⛓️ dependency
🔴 👀 Review Requirements 👀 reviews
🔴 🔎 Reviews 👀 reviews
🟢 🤖 Continuous Integration
🟢 Enforce conventional commit
🟢 📕 PR description
🟢 🚦 Auto-queue

🔴 ⛓️ Depends-On Requirements

Waiting for

This rule is failing.

Requirement based on the presence of Depends-On in the body of the pull request

🔴 👀 Review Requirements

Waiting for

  • #approved-reviews-by>=2
This rule is failing.
  • any of:
    • #approved-reviews-by>=2
    • author = dependabot[bot]
    • author = mergify-ci-bot
    • author = renovate[bot]

🔴 🔎 Reviews

Waiting for

  • #review-requested = 0
This rule is failing.
  • #review-requested = 0
  • #changes-requested-reviews-by = 0
  • #review-threads-unresolved = 0

Show 4 satisfied protections

🟢 🤖 Continuous Integration

  • all of:
    • check-success=ci-gate

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|internal|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?!?:

🟢 📕 PR description

  • body ~= (?ms:.{48,})

🟢 🚦 Auto-queue

When all merge protections are satisfied, this pull request will be queued automatically.

@mergify
mergify Bot requested a review from a team September 18, 2026 07:14
Copilot AI lite review requested due to automatic review settings September 18, 2026 07:38
@sileht
sileht force-pushed the devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c branch from c53311d to 2de27ff Compare September 18, 2026 07:38
@sileht
sileht deployed to func-tests-live September 18, 2026 07:39 — with GitHub Actions Active
@sileht

sileht commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

Revision history

# Type Changes Reason Date
1 initial c53311d 2026-09-18 07:38 UTC
2 rebase c53311d → 2de27ff (rebase only) 2026-09-18 07:38 UTC
3 rebase 2de27ff → 6c614fe (rebase only) 2026-09-18 08:13 UTC

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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-env dependency 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 at Cargo.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.

`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
sileht force-pushed the devs/sileht/test-env-access-ub/read-env-funnel--7d4c755c branch from 2de27ff to 6c614fe Compare September 18, 2026 08:13
@sileht
sileht deployed to func-tests-live September 18, 2026 08:13 — with GitHub Actions Active
@mergify
mergify Bot had a problem deploying to Mergify Merge Protections September 18, 2026 08:14 Failure
@sileht
sileht marked this pull request as ready for review September 18, 2026 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants