Populate process ENV from EnvManager by default by mokagio · Pull Request #723 · wordpress-mobile/release-toolkit

mokagio · 2026-05-20T02:52:57Z

What does it do?

Flips EnvManager's default back to populating the process ENV from the loaded .env file (no-override semantics — pre-existing ENV wins).
Adds a mutate_env: false opt-out for callers who want to preserve the parse-only / instance-isolation semantics introduced in #578.

Why

#578 switched EnvManager from Dotenv.load to Dotenv.parse to make instances independent of process ENV and to give reset! something to undo.

Alas, fastlane actions look up their default_value: via ENV.fetch(...), so with ENV pristine, calls like app_store_connect_api_key can't see anything loaded from the .env file.

This PR keeps the structure of #578 (parse + per-instance dict) but layers values into ENV by default.
reset! tracks the keys it added and removes only those, so pre-existing ENV entries are never disturbed.

Migration

This is technically a breaking behavior change for anyone who upgraded to a release that included #578 and depended on ENV staying pristine.
Mitigation: pass mutate_env: false to set_up (or new) to restore the previous behavior.

It's unfortunate to have to do a major version bump for this little used feature, but versions are cheap so we're better off keeping semantic integrity, IMHO.

Checklist before requesting a review

Run bundle exec rubocop to test for code style violations and recommendations.
Add Unit Tests (aka specs/*_spec.rb) if applicable.
Run bundle exec rspec to run the whole test suite and ensure all your tests pass.
Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
If applicable, add an entry in the MIGRATION.md file — pending a decision on whether this ships in a major or minor bump.

PR #578 switched `EnvManager` from `Dotenv.load` to `Dotenv.parse` to gain instance isolation and clean `reset!` semantics. The trade was net negative: fastlane actions read their `default_value:` from `ENV.fetch(...)`, so without ENV mutation the loaded values are invisible to actions like `app_store_connect_api_key`, `match`, or `upload_to_testflight` — defeating the affordance `EnvManager` exists to provide. Layer parsed values into process `ENV` by default, with no-override semantics so pre-existing `ENV` entries (e.g. set by CI) still win. Track which keys we added so `reset!` can roll back precisely, without disturbing pre-existing entries. Pass `mutate_env: false` to opt out and keep the parse-only / instance-isolation semantics for callers (mostly tests) that want them. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR changes EnvManager’s default behavior to once again populate the process ENV from the loaded .env file (while preserving “no-override” semantics where pre-existing ENV values win). It adds an opt-out (mutate_env: false) for callers that want the parse-only / per-instance isolation behavior introduced previously, and updates reset behavior to roll back any default-instance ENV mutations.

Changes:

Default EnvManager initialization now layers parsed .env values into process ENV (no-override), with mutate_env: false to opt out.
Track and roll back ENV mutations via restore_env!, called from EnvManager.reset!.
Update specs to cover default mutation behavior, opt-out isolation, and rollback semantics; add a breaking-change changelog entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`lib/fastlane/plugin/wpmreleasetoolkit/env_manager/env_manager.rb`	Adds `mutate_env` defaulting to true, implements `restore_env!`, and makes `reset!` roll back default-instance mutations.
`spec/env_manager_spec.rb`	Updates and extends tests to validate default ENV mutation, opt-out behavior, and rollback/idempotency.
`CHANGELOG.md`	Documents the breaking behavior change and the `mutate_env: false` opt-out.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mokagio · 2026-05-26T04:08:52Z

+      def restore_env!
+        @mutated_keys.each { |key| ENV.delete(key) }
+        @mutated_keys = []


Good catch — fixed in 97a2002.

@mutated_keys is now @mutations (a key => value hash recording what this instance wrote), and restore_env! only deletes when ENV[key] still equals the value we set. Any later overwrite by another caller is left in place. Added a regression test in spec/env_manager_spec.rb.

Posted by Claude (Opus 4.7) on behalf of @mokagio with approval.

`restore_env!` previously deleted every key this instance had set, even if a later caller had overwritten the value via `ENV[key] = ...`. That contradicts the intent of "remove only what this instance added" and could silently drop a caller's update. Track the value written for each mutated key and delete only when `ENV[key]` still matches. Addresses #723 (comment). --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

mokagio · 2026-06-17T11:28:54Z

 ### Breaking Changes

-_None_
+- `EnvManager`: populate the process `ENV` from the loaded `.env` file by default (no-override semantics — pre-existing `ENV` values win), so fastlane actions that read their `default_value:` from `ENV` find loaded secrets without callers having to thread them through explicitly. Pass `mutate_env: false` to opt out and keep the parse-only / instance-isolation semantics introduced in [#578]. `EnvManager.reset!` now also rolls back the keys it added. [#XXX]


Already addressed in the current PR head: the changelog entry now references #723 instead of the placeholder.

Posted by Codex (GPT-5) on behalf of @mokagio with approval.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

The `mutate_env: true` default in #723 is a breaking behavior change for anyone on a `14.4.x`+ release that depended on `ENV` staying pristine. Give them the `mutate_env: false` escape hatch under a new 14.x-to-15.0.0 section. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

mokagio · 2026-06-18T05:52:02Z

Thanks for picking this up @iangmaia !

Looks like Copilot has nothing to add to its previous reviews, which I had already addressed.

What do you think of this? For live validation, see 4-ghe-Automattic/readablog/files#diff-61bacd7fc8b74e42763f36b953e5dfd00182b336a71446ea1c8ddcfbfcb8f147R10-R15 and 147-ghe-Automattic/everyfirst-app/files#diff-d09ea66f8227784ff4393d88a19836f321c915ae10031d16c93d67e6283ab55fR10-R15

iangmaia · 2026-06-19T09:44:59Z

@mokagio I've disabled auto-merge just in case we want to merge @oguzkocer 's #733 first and release a new version before adding this breaking change on trunk.

iangmaia · 2026-06-19T18:10:33Z

+
+### `EnvManager` populates the process `ENV` by default
+
+`EnvManager` now layers the values from the loaded `.env` file into the process `ENV` when you call `set_up`/`new`, so fastlane actions that resolve their `default_value:` via `ENV.fetch(...)` can see them. Pre-existing `ENV` entries always win (no-override), and `reset!` removes only the keys this instance added.


This mentions set_up/new, then says reset! removes keys this instance added. Correct me if I'm wrong, but reset! only restores the default instance created by set_up and callers using new need restore_env! instead?

You are right — fixed in ea2c709.

EnvManager.reset! only rolls back the default instance created by set_up; an instance you built with new rolls back via restore_env!. MIGRATION.md now says exactly that.

Posted by Claude (Opus 5) on behalf of @mokagio with approval.

iangmaia · 2026-06-19T18:16:19Z

+        # Always parse rather than load: it lets us track exactly which keys we
+        # add to `ENV` so `reset!` can undo only those, without disturbing keys
+        # that pre-existed in the process environment.
        @loaded_env = File.exist?(@env_path) ? Dotenv.parse(@env_path) : {}


💭 One concern, maybe a bit of an edge case: if I understand correctly, it seems Dotenv.parse + manual ENV mutation does not fully match Dotenv.load semantics for interpolation.
If ENV['A'] already exists and the .env has A=from_file plus B=${A}, Dotenv.load resolves B from the pre-existing ENV['A'], but this implementation resolves it from the file value.

I wonder if a possible solution could be to use Dotenv.load when mutate_env: true, record the ENV delta for rollback, and keep Dotenv.parse only for mutate_env: false? 🤔

Good catch, and confirmed against dotenv 2.8.1 — Dotenv::Substitutions::Variable flips precedence on the is_load flag: env.merge(ENV) for load, ENV.to_h.merge(env) for parse. So with ENV['A'] set, parse really did resolve B=${A} from the file value.

Fixed in fbe4253 along the lines you suggested: mutate_env: true now uses Dotenv.load and records the ENV delta around the call for rollback, while mutate_env: false keeps Dotenv.parse (that mode is about staying out of the process environment entirely). Two specs cover both directions; the mutate_env: true one fails against the previous implementation with expected "from_env", got "from_file".

Posted by Claude (Opus 5) on behalf of @mokagio with approval.

iangmaia

Approving to unblock.
I left a couple of comments, I'll leave them up to you. I'd just be careful when finally merging this as it is a breaking version, so perhaps good to keep an eye and coordinate with other PRs we have in the queue.

…utate-env-default-true

@iangmaia

`reset!` only rolls back the default instance created by `set_up`; callers holding an instance from `new` roll back with `restore_env!`. Flagged in review by @iangmaia. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>

@iangmaia

`Dotenv.parse` resolves `B=${A}` against the file's own `A`, while `Dotenv.load` resolves it against `ENV['A']` when one is set. Parsing and then writing to `ENV` ourselves therefore diverged from Dotenv's no-override semantics for interpolated values. Loading and diffing `ENV` around the call keeps those semantics and still tells us which keys to undo in `restore_env!`. `mutate_env: false` keeps parsing, since that mode is about staying out of the process environment entirely. Raised in review by @iangmaia. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>

…utate-env-default-true

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

mokagio · 2026-08-27T06:43:13Z

Thanks again @iangmaia for the review. I finally got onto this because I wanted to ship David's #772 to close the loop on wordpress-mobile/GutenbergKit#591 , but given #773 landed since, and it's a breaking change, I thought I might as well land this and ship two breaking changes in one major version bump.

Copilot AI review requested due to automatic review settings May 20, 2026 02:52

mokagio self-assigned this May 20, 2026

Copilot started reviewing on behalf of mokagio May 20, 2026 02:53 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

iangmaia requested a review from Copilot June 2, 2026 11:20

Copilot started reviewing on behalf of iangmaia June 2, 2026 11:21 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Add PR number in changelog for EnvManager ENV processing

b33ee53

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

mokagio commented Jun 9, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Streamline changelog

e875207

mokagio commented Jun 9, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

mokagio and others added 2 commits June 9, 2026 14:47

Fix PR number in changelo

3c28110

mokagio requested a review from a team June 9, 2026 05:29

iangmaia requested a review from Copilot June 17, 2026 12:14

Copilot started reviewing on behalf of iangmaia June 17, 2026 12:15 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

mokagio enabled auto-merge June 18, 2026 05:50

iangmaia disabled auto-merge June 19, 2026 09:43

iangmaia reviewed Jun 19, 2026

View reviewed changes

iangmaia approved these changes Jun 19, 2026

View reviewed changes

mokagio and others added 4 commits July 1, 2026 09:06

Merge remote-tracking branch 'origin/trunk' into mokagio/envmanager-m…

d0cd27e

…utate-env-default-true

Merge remote-tracking branch 'origin/trunk' into mokagio/envmanager-m…

2e5b005

…utate-env-default-true

mokagio requested a lite review from Copilot August 27, 2026 06:32

Copilot started reviewing on behalf of mokagio August 27, 2026 06:33 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

mokagio commented Aug 27, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Tidy up AI-generated changelog entry

9eda1b1

mokagio enabled auto-merge August 27, 2026 06:44

mokagio merged commit 6fe0926 into trunk Aug 27, 2026
6 checks passed

mokagio deleted the mokagio/envmanager-mutate-env-default-true branch August 27, 2026 06:46

mokagio mentioned this pull request Aug 27, 2026

Release 15.0.0 into trunk #774

Merged


		### `EnvManager` populates the process `ENV` by default

		`EnvManager` now layers the values from the loaded `.env` file into the process `ENV` when you call `set_up`/`new`, so fastlane actions that resolve their `default_value:` via `ENV.fetch(...)` can see them. Pre-existing `ENV` entries always win (no-override), and `reset!` removes only the keys this instance added.

Uh oh!

Conversation

mokagio commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does it do?

Why

Migration

Checklist before requesting a review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mokagio May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mokagio Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mokagio commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iangmaia commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iangmaia Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

iangmaia Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 27, 2026

Choose a reason for hiding this comment

Uh oh!

iangmaia left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mokagio commented Aug 27, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mokagio commented May 20, 2026 •

edited

Loading

mokagio commented Jun 18, 2026 •

edited

Loading

iangmaia commented Jun 19, 2026 •

edited

Loading