node: keep --experimental-webstorage off the inherited NODE_OPTIONS channel - #812
Conversation
NODE_OPTIONS is inherited by the whole process subtree, and this flag does not exist before Node 22.4 — above nub's 18.19 support floor. Any descendant on an older Node therefore aborted at startup with exit 9, which is reachable rather than theoretical: a host on the 22.4-24 band running Electron 34 or older (which embeds Node 20.18.1) hit exactly this. strip_unsupported_node_options never covered it, because that applies only to the INHERITED string, never to nub's own freshly pushed tokens. Inject it on argv instead, at the site outside the augment block that the version-gated feature flags already use, so it reaches the spawned process and nothing below it. A child `node` still gets it: that invocation re-enters nub through the PATH shim and is given the flag on its own argv. Move the paired __NUB_NEUTRALIZE_LOCALSTORAGE signal to the same site. The preload deliberately does not delete that var, and that inheritance was what covered the subtree; once the flag is per-process, a level can no longer be assumed to inherit a signal for a flag it received directly. Setting both together is sufficient and idempotent. The nearby comment claiming the preload deletes the var was already stale and is corrected. Refs #7
… test a control The version-band and injection-policy unit tests all pass with the injection wired to the wrong channel, because none of them observe which channel a real spawn used. webstorage_flag_rides_argv_and_never_node_options asserts both halves against a real spawn: present on argv, absent from NODE_OPTIONS. Verified to fail for the right reason by running it against a pre-change binary, where it panics on the NODE_OPTIONS assertion rather than anywhere else. localstorage_neutralization_reaches_grandchildren could not fail. It asserted only that localStorage is "undefined" at each level, which is equally true of a process that never received --experimental-webstorage at all — so it passed whether or not the flag still reached the subtree, which is exactly how moving the flag off the inherited channel could have broken it silently. Assert sessionStorage is "object" alongside it: that is "object" only when the flag really arrived.
…xclude decision wiki/design/architecture.md still described the pre-#777 mechanism, where the version-gated flags travelled in NODE_OPTIONS and only the argv-only shape did not. Since #777 every injected flag rides argv; the two unflag shapes now differ only in WHY NODE_OPTIONS is closed to them — Node refuses the argv-only ones by name, while the rest are kept off it because that string is inherited by the whole subtree and aborts a descendant on an older Node. Also correct the probe sentence: the ordinary set is still filtered against Node's accepted-in-NODE_OPTIONS list, which stays a valid existence check even though delivery is argv, while the argv-only set cannot use that list at all and is probed by spawning the binary. Record in spawn.rs that keeping --test-coverage-exclude on the inherited channel is a decided trade rather than an oversight, naming the alternatives that were declined so it is not silently "fixed" later.
# Conflicts: # wiki/design/architecture.md
|
Your Claude subscription has hit its usage limit. It resets at 12:40am (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
#815 moved the restatement of Node's default test-file pattern to argv, so NODE_OPTIONS now carries a single --test-coverage-exclude token holding nub's own runtime glob. The note added here before that landed still described two, which contradicted the comment directly below it.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions inline — the channel move itself is correct.
Reviewed changes. No prior Pullfrog review existed on this PR, so this pass covers the whole change rather than only 7918832c (a comment-only correction of the coverage-exclude token count).
Verified the three things most likely to be wrong in a move like this, all of them fine:
node_optionsat the new argv site resolves to the outerenv::var("NODE_OPTIONS")binding, not the shadow insideif !node_opts_parts.is_empty(), so the predicate still sees the user's inherited value.- The injection precedes
cmd.args(config.user_args), so a user-supplied token still wins on argv. - The new block is gated on
!compat_mode && preload.is_some()and deliberately not on!is_reentrant, so re-entrant PATH-shim children still receive the flag.
Other spawn paths do not regress: nub run and nubx/dlx/exec route through spawn_node, the compiled launcher already injected on argv via compile_webstorage_policy, and worker-polyfill.mjs merges process.execArgv into the Worker's execArgv so the flag now flows through as a literal token. The new test is a real wiring test — process.execArgv excludes NODE_OPTIONS-derived flags, so both halves of the assertion carry weight — and the localstorage_neutralization_reaches_grandchildren update turns a bare absence check into a genuine positive control.
Stale comments elsewhere still name NODE_OPTIONS as the webstorage channel
Four sites outside the diff describe the channel this PR just closed. The first is load-bearing rather than cosmetic:
runtime/polyfills.cjs:76-79— this comment is the stated justification for keeping__NUB_NEUTRALIZE_LOCALSTORAGEset rather than deleting it: "anode- ornub-spawned grandchild re-inherits the webstorage flag viaNODE_OPTIONS". After this PR the grandchild re-inherits it via the PATH shim and its own argv instead. The behavior is still right, but the reason is now wrong, and a future reader could reasonably delete the var on the strength of it.crates/nub-cli/src/cli.rs:5616-5618— "NODE_OPTIONS + PATH shim so childnodeprocesses … inherit transpilation, polyfills, flag injection, and webstorage".crates/nub-cli/src/cli.rs:5500-5502— "NODE_OPTIONS(source maps + preload + webstorage; …)".crates/nub-core/src/node/discovery.rs:466-473— "inheritedNODE_OPTIONSflags computed for the project Node (e.g.--experimental-webstorage) then abort the older one". This one is describing the exact failure mode the PR fixes, using the example it no longer applies to.
(discovery.rs:1862-1869 reads as historical narration of a past abort and is defensible as-is.)
The script path has no channel-observing test
compute_augmentation_env stopped pushing the flag, and nothing asserts it is absent from the NODE_OPTIONS string that function produces. script_spawn_policy_respects_quoted_node_options exercises the predicate rather than the emitted string, so it would stay green if the push came back. The new integration test covers the spawn_node path well; the script path is the half without a guard. A unit assertion that the produced NODE_OPTIONS does not contain --experimental-webstorage would close it cheaply.
Nitpicks
webstorage_flag_rides_argv_and_never_node_optionsremoves its temp dir at the start but not at the end; the sibling test atintegration.rs:3592cleans up on both sides.
Claude Opus ｜ 𝕏
| Below a feature's floor no band matches and Nub does nothing — the feature is unavailable rather than half-present. | ||
|
|
||
| Banding is exact because it has to be. Injecting an experimental flag on a version that does not have it is a hard startup abort, not a warning. Several rows carry two disjoint bands where a backport reached one release line and not another; `node:sqlite` is the clearest case, having been unflagged, re-flagged, and unflagged again. ShadowRealm is never injected at all, because the flag crashes embedded Node through a snapshot hash mismatch. That hazard is what separates the two unflag shapes: a flag in `NODE_OPTIONS` is inherited by every process below, including an embedded Node booting from a V8 snapshot, while an argv flag reaches only the process Nub spawns. | ||
| Banding is exact because it has to be. Injecting an experimental flag on a version that does not have it is a hard startup abort, not a warning. Several rows carry two disjoint bands where a backport reached one release line and not another; `node:sqlite` is the clearest case, having been unflagged, re-flagged, and unflagged again. ShadowRealm is never injected at all, because the flag crashes embedded Node through a snapshot hash mismatch. That hazard generalizes, and it is why no version-gated flag rides `NODE_OPTIONS`. That string is inherited by every process below, including an embedded Node booting from a V8 snapshot, and the set Nub builds is matched to the version of the Node it resolved — so a descendant running an older Node meets a flag it cannot parse and aborts at startup. Every injected flag travels on argv instead, reaching the process Nub spawns and nothing beneath it; a child `node` gets its own copy by re-entering Nub through the PATH shim. What stays in `NODE_OPTIONS` is the preload, which every Node accepts, and flags whose floor sits at or below Nub's own support floor. |
There was a problem hiding this comment.
This paragraph states the rule as an absolute — "no version-gated flag rides NODE_OPTIONS" — and then closes by enumerating what remains on that channel as "the preload … and flags whose floor sits at or below Nub's own support floor". Both omit --test-coverage-exclude, which has a floor of 22.5 (well above the 18.19 support floor) and which this same PR still pushes onto NODE_OPTIONS.
Worth naming the exception here, since the PR's own spawn.rs comment goes out of its way to say it must not be silently "fixed" — and this doc is the more likely place for a future reader to form their model of the rule.
Technical details
spawn.rs:1174-1191 (added by this PR) documents it explicitly:
ONE DELIBERATE EXCEPTION REMAINS:
--test-coverage-exclude(floor 22.5) … It breaks the rule knowingly — a descendant below 22.5 aborts on it — because it is the only token here that MUST share a channel with the preload … do NOT "fix" it silently.
The push itself is at spawn.rs:1259-1289, and it is not gated on coverage_active, so it is present on ordinary runs rather than only under coverage.
| let stdout = String::from_utf8_lossy(&out.stdout); | ||
| let opts = stdout | ||
| .lines() | ||
| .find_map(|l| l.strip_prefix("OPTS=")) | ||
| .unwrap_or_else(|| panic!("probe printed no OPTS line; stdout={stdout:?}")); | ||
| let argv = stdout | ||
| .lines() | ||
| .find_map(|l| l.strip_prefix("ARGV=")) | ||
| .unwrap_or_else(|| panic!("probe printed no ARGV line; stdout={stdout:?}")); |
There was a problem hiding this comment.
nit: both panics print only stdout, so a probe that fails to start reports as a missing-token assertion with no clue why. The sibling test at integration.rs:3572-3578 includes stderr.
| let stdout = String::from_utf8_lossy(&out.stdout); | |
| let opts = stdout | |
| .lines() | |
| .find_map(|l| l.strip_prefix("OPTS=")) | |
| .unwrap_or_else(|| panic!("probe printed no OPTS line; stdout={stdout:?}")); | |
| let argv = stdout | |
| .lines() | |
| .find_map(|l| l.strip_prefix("ARGV=")) | |
| .unwrap_or_else(|| panic!("probe printed no ARGV line; stdout={stdout:?}")); | |
| let out = String::from_utf8_lossy(&run.stdout); | |
| let err = String::from_utf8_lossy(&run.stderr); | |
| let argv = out | |
| .lines() | |
| .find_map(|l| l.strip_prefix("ARGV=")) | |
| .unwrap_or_else(|| panic!("probe printed no ARGV line; stdout={out:?} stderr={err:?}")); | |
| let opts = out | |
| .lines() | |
| .find_map(|l| l.strip_prefix("OPTS=")) | |
| .unwrap_or_else(|| panic!("probe printed no OPTS line; stdout={out:?} stderr={err:?}")); |
--experimental-webstoragedoes not exist before Node 22.4, which is above nub's 18.19 support floor, andNODE_OPTIONSis inherited by the entire process subtree. So any descendant on an older Node aborted at startup with exit 9 on a flag it could not parse.That was reachable rather than theoretical: a host on the 22.4–24 band running Electron 34 or older (which embeds Node 20.18.1) hit exactly this.
strip_unsupported_node_optionsnever covered it, because that applies only to the inherited string, never to nub's own freshly pushed tokens. Issue #7 was the same flag reaching an older child through a nested.nvmrc, and it was closed by the node_modules pin guard rather than by taking the flag off this channel.The flag now rides argv, from the site outside the augment block that the version-gated feature flags already use since #777. A child
nodestill gets it: that invocation re-enters nub through the PATH shim and is given the flag on its own argv.The neutralize signal moves with it, and has to.
runtime/polyfills.cjsdeliberately does not delete__NUB_NEUTRALIZE_LOCALSTORAGE, and that inheritance was what covered the subtree. Once the flag is per-process, a level can no longer be assumed to inherit a signal for a flag it received directly, so both are set at one site. A nearby comment claiming the preload deletes the var was already stale and is corrected.Verification
Measured against two independent controls — current
mainand released v0.7.4, both of which carry the flag inNODE_OPTIONS:NODE_OPTIONS, present onexecArgv,sessionStoragelivelocalStorageneutralized andsessionStoragelive at all three levelsnub run, including a nestedsh -c 'node ...'main--localstorage-fileopt-inmain--no-experimental-webstoragemainexits 9; this branch survivesTests
webstorage_flag_rides_argv_and_never_node_optionsis the wiring test: the version-band and injection-policy unit tests all pass with the injection wired to the wrong channel, because none of them observe which channel a real spawn used. Confirmed to fail for the right reason by running it against a pre-change binary, where it panics on theNODE_OPTIONSassertion specifically.localstorage_neutralization_reaches_grandchildrencould not fail before. It asserted only thatlocalStorageisundefinedat each level, which is equally true of a process that never received the flag at all — so it passed whether or not the flag still reached the subtree, which is exactly how this change could have broken it silently. It now assertssessionStorageisobjectalongside, which is true only when the flag really arrived.Not changed
--test-coverage-exclude(floor 22.5) stays onNODE_OPTIONS, so a host on 22.5+ still cannot run Electron 34 or older. It is the one token that must share a channel with the preload, since a coverage grandchild nub never spawns inherits the preload through that string alone. Keeping it was decided explicitly and is recorded inspawn.rswith the declined alternatives, so it is not silently "fixed" later.The docs commit also corrects
wiki/design/architecture.md, which still described the pre-#777 mechanism where version-gated flags travelled inNODE_OPTIONS.Refs #7, #246