Repository navigation
[Fix] Pass NODE_OPTIONS to Electron as arguments in the macOS shims - #528
Merged
Merged
Conversation
On macOS, Electron ignores NODE_OPTIONS in any process the app did not start itself, and every child launched through the node shim is started by bash. A build that sets NODE_OPTIONS for a child lost it: older Gutenberg trunk runs `cross-env NODE_OPTIONS=--import=... tz build` in @wordpress/theme, so "Apply and rebuild" failed on pull requests based on it (reproduced with gutenberg#76292). The macOS shims now split NODE_OPTIONS the way Node does and pass the options as arguments ahead of the CLI and the caller's own. Linux and Windows shims are unchanged. Fixes #525 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ch signed ones An unsigned Electron (npm start, CI) still reads NODE_OPTIONS from a bash parent, so it applied every forwarded option twice, and a --loader registered twice. The shims now empty the variable for that call. The tests run the shim under /bin/bash, the 3.2 users get, and the comments say what a signed Electron actually does: it removes the variable, so only the shim's own child gets the options. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOn macOS, the POSIX node, npm, and npx shims tokenize Assessment against linked issues
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The macOS shims forward options to Electron as intended; no identified issue blocks merging after normal checks. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A signed Electron prints "Node.js environment variables are disabled because this process is invoked by other apps" whenever NODE_OPTIONS is present, even empty, so emptying it for the Electron call printed that line for every process a Gutenberg build starts. Unset, Electron never sees it: no warning, and the options still arrive once, as arguments. The test expected an empty variable, which is what let this through; it now expects the variable unset. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Manual pass on macOS, Buildkite artifact for 9766088, on a new Gutenberg site with PR 76292 checked out:
|
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
juanmaguitar
added a commit
that referenced
this pull request
Sep 25, 2026
## Why `v1.2.0-beta.2` shipped on 23 September. Five commits reach trunk since then: #524, #528, #530 and #532 fix what the beta.2 testing turned up (files another branch ignored counted as changes after a switch, `NODE_OPTIONS` lost in the macOS shims, and a trunk-only Gutenberg route left behind by a switch to an older pull request, which broke `wp-build`), and #531 makes the manual **Real WordPress setup** workflow pass again, green on both platforms and both targets on trunk's app code. Milestone `v1.2.0` holds only the umbrella #251. `electron-builder` derives artifact names from `package.json`, so the version has to move before the tag and the artifacts exist. This is a stable release: it takes `releases/latest` from `v1.1.0`. ## What changes Only the version. All three package-version fields in `package.json` and `package-lock.json` move together from `1.2.0-beta.2` to `1.2.0`. No dependency versions change, and there is no application behaviour change in this PR. ## How to test this Platforms: any for the suite; macOS and Windows for the artifact checks that follow the merge. 1. `npm run lint` is clean and `npm test` passes, from the repository root. 2. `node -e "console.log(require('./package.json').version)"` prints `1.2.0`. 3. `python3 -c "import json;d=json.load(open('package-lock.json'));print(d['version'], d['packages']['']['version'])"` prints `1.2.0 1.2.0`. **What must not have happened:** no dependency version may change. `git diff` must show exactly three changed lines, all of them the root package version. After merge, the release build must produce artifacts named `wordpress-contributor-toolkit-1.2.0-*`; the asar root must be exactly `node_modules`, `package.json`, `src`, with npm 11 under `node_modules/npm` and no `.codesigning` directory; on macOS `CFBundleShortVersionString` reads `1.2.0`. ## Risks and limitations No UI change. The risk is a mis-scoped edit in `package-lock.json`, where dependencies also carry version fields; the replacement was confined to the first 2000 bytes of the file, which hold the two root entries and nothing else, and both files were re-parsed as JSON afterwards. Manual passes on what reached trunk after beta.2: #528 and #530 on macOS in their PRs; #530 plus #532 on Windows on #532's artifact (c7ec033), including the case that failed on e0a3faa. ## Related Part of #251. Release notes follow on the release; docs publish for the tag per #487. --- <details> <summary>Review outcome (required, see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up]. Deterministic layer only: lint clean, 1667 tests pass, both files valid JSON, diff confined to the three root version fields. No judgement pass was run: this change has no logic to review. - **Review:** completed, deterministic layer, in session; head is the single bump commit / base `4c3b05a` (trunk); evidence above; outcome: nothing to fix. - **Since review:** none. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
Why
On macOS, "Apply and rebuild" fails on Gutenberg pull requests based on trunk from before 17 April 2026, such as WordPress/gutenberg#76292. The screen says only
Build failed: Command failed: npm run --if-present --workspaces --silent build, exit 1. Without--silent,@wordpress/theme'sbuild:tokensfails:Reported in #525.
What changes
Root cause: the Gutenberg script runs
cross-env NODE_OPTIONS=--import=esbuild-esm-loader/register tz build.tzstarts through our bashnodeshim, and on macOS a signed Electron removesNODE_OPTIONSfrom any process whose parent is not part of the app, so the--importnever arrives. It is not a regression from #519: the main binary fails the same way as the Helper, and the same build passes on the systemnode.The macOS
node/npm/npxshims now splitNODE_OPTIONSthe way Node does (quotes, backslash escapes, no globbing) and pass the options as arguments, after our--requireand before the CLI and the caller's own arguments. Then they unset the variable, so an unsigned Electron (npm start, CI) does not apply every option twice and dev behaves like the release. Unset, not emptied: a signed Electron prints anode_main.cc:153warning whenever the variable exists, even empty. Linux and Windows shims are byte-identical to before.Deliberately not in it: only the shim's own child gets the options. What that child starts inherits no
NODE_OPTIONS, as before this PR (follow-up below).How to test this
Platforms: macOS only; the change is macOS only. Build: the Buildkite artifact for the current head.
Starting state: a Gutenberg site on trunk, no work item linked, no pull request checked out. A copy you can throw away is best: the build rewrites
packages/theme/tokens/color.json.76292, check it out and let the install finish.What must not have happened: no
node_main.cc:153 Node.js environment variables are disabledlines in the terminal; no Dock tiles while the build runs (#518).If the root build then fails on
routes/dashboard/package.json, that is a different bug (a trunk-only route left behind with onlynode_modules, same with the systemnode); deleteroutes/dashboard/on the test site and rebuild.I reproduced the failure and checked the fix on a copy of a real site on the PR 76292 commit, running the app's exact command,
npm run --if-present --workspaces --silent build:node: exit 0.The new tests in
tests/unit/node-shims.test.cjsrun the real shim under/bin/bash(3.2, as macOS ships it) and fail on the old code.Risks and limitations
NODE_OPTIONSbut not as a flag: none found. Command-line-only flags are now accepted, which is more lenient, not broken.NODE_OPTIONS(unterminated quote) is passed through as text where Node would refuse it./bin/bashin the tests) and the wording corrected. 1 follow-up, the grandchild gap above.npm run buildexit 0 with no warnings.Related
Fixes #525. Found while testing #524.
🤖 Generated with Claude Code