node: route Node 23.0-23.4 to the compat tier - #802
Conversation
`module.registerHooks` is a semver-minor that shipped on the 23.x line at 23.5.0 and on the 22.x LTS line only later, at 22.15.0 (nodejs/node#55698). The tier gate compared against 22.15.0 alone, so 23.0.0-23.4.x sorted above the fast floor without having the API. Those releases were injected with `--require runtime/preload.cjs`, whose unconditional `registerHooks` call threw `TypeError: module_.registerHooks is not a function` before any user code ran. `supports_augmentation` now excludes the band, which is the single point every downstream gate keys on (preload channel, user-preload routing, chainer channel). The compat `--import` path already works on 23.x. Two JS gates carried the same defect and are fixed with capability probes rather than version arithmetic: `__isFastTier` in preload.mjs, which is the file the band now lands on, and the fast/async branch in preload.cjs, which a grandchild `node` can still reach through inherited NODE_OPTIONS. Verified on real 18.19.0, 22.14.0, 22.15.0, 23.0.0, 23.3.0, 23.4.0, 23.5.0, 23.6.0, 24.0.0 and 24.19.0.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The core fix is right and well evidenced, but the same off-by-a-release-line defect is still live in nub compile's external-shim gate, and the new preload.cjs fallback branch does not deliver everything the compat tier delivers on this band.
Reviewed changes — the full diff at 45f3ed27 across all 8 files, plus the surrounding tier-routing code in crates/nub-core/src/node/, crates/nub-cli/src/, and runtime/.
supports_augmentationre-banded —MIN_AUGMENTED_23 = 23.5.0added, and the predicate branches onmajor() == 23. I confirmed this against nodejs/node#55698 and the v23.5.0 / v22.15.0 release notes:registerHooksreached 23.x at 23.5.0 and 22.x at 22.15.0 four months later, so 23.0.0–23.4.x is exactly the affected band and no other line is.tier()de-duplicated — the test-only classifier now callssupports_augmentation()/is_supported()instead of restating the comparisons, so the tests can no longer drift from production.- Runtime capability probes —
preload.mjs's__isFastTierswaps its version band fortypeof module.registerHooks === "function", andpreload.cjsgains ahasRegisterHooksguard for the inherited-NODE_OPTIONSgrandchild case. - Wiki + doc comments — an "Availability by release line" section in the coverage matrix with a probed
typeoftable, and the two-tier table inarchitecture.md.
Things I checked and cleared, so they don't come back in a later pass: module is a real binding in preload.mjs (import module from "node:module", line 43), so the typeof probe is valid there; the band/probe swap agrees with the old band on every Node version except 23.0–23.4; nodeHookComposeBroken()'s 22.15–24.11 band legitimately contains 23.0–23.4 because it guards a different defect (nodejs/node#59666), and its only consumer is inert on this band once hasRegisterHooks is false; and both new test groups genuinely go red without the fix — pre-fix, tier(23.0.0) returns FastPath and preload_injection_for(23.0.0) returns --require.
⚠️ nub compile's external-shim gate has the identical defect and is untouched
The PR body says supports_augmentation is "the one predicate every downstream gate keys on." That holds for the spawn path, but crates/nub-cli/src/compile/external.rs:82 keeps its own hardcoded NodeVersion::new(22, 15, 0), and the artifact it gates emits registerHooks directly. A nub compile --external … targeting 23.0–23.4 passes the check and produces a binary that dies at startup with the same registerHooks is not a function — the exact bug this PR exists to kill, one layer over.
Technical details
# `nub compile` external shim accepts a Node 23.0–23.4 target it cannot run on
## Affected sites
- `crates/nub-cli/src/compile/external.rs:82` — `fn min_node() -> NodeVersion { NodeVersion::new(22, 15, 0) }`, with a doc comment that repeats the same reasoning error: "`module.registerHooks` … landed in Node 22.15, the same floor nub's own fast tier uses."
- `crates/nub-cli/src/compile/external.rs:93` — `check_node_support` passes on `*version >= min_node()`, so `23.0.0 >= 22.15.0` is accepted.
- `crates/nub-cli/src/compile/external.rs:323,338` — the generated `__nub_external.mjs` destructures `registerHooks` from `node:module` and calls it unconditionally.
- `crates/nub-cli/src/compile/external.rs:106` — the refusal message says "which older Node does not have", which is the version-ordering fallacy stated out loud to the user.
Reachable only when `plan.needed()` is true, i.e. a `--external` or `--allow-dynamic-import` build. Note that a bare major pin floors at `X.0.0` (per the `check_node_support` doc comment), so plain `--target 23` lands squarely in the broken band.
## Required outcome
- A `--external` / `--allow-dynamic-import` build targeting any Node in 23.0.0–23.4.x is refused at build time with the existing message, rather than producing an artifact that throws at startup.
- The 22.15 floor and the 23.x floor stay expressed in one place, so the next reader cannot reintroduce the skew.
## Suggested approach (optional)
- Reuse the predicate this PR just made authoritative rather than adding a third copy of the band — e.g. gate on `version.supports_augmentation()` and derive the message's floor from the target's own major, so a 23.x target is told `--target 23.5` and a 22.x target is told `--target 22.15`. As written, a 23.4 user would be told to "pass `--target 22.15` (or newer)", which is both a downgrade and still wrong.
- `check_node_support`'s existing unit tests (`external.rs:456-481`) are the natural home for a `23.0.0` / `23.4.99` rejection case and a `23.5.0` acceptance case.
## Open questions for the human
- In scope for this PR, or a follow-up? It is the same defect class and the PR body claims the predicate is now centralized, so shipping without it leaves that claim inaccurate.ℹ️ No CI leg runs a 23.x Node, so the band this PR fixes has no end-to-end coverage
The bug was an end-to-end startup crash on a real binary, but everything added here is a unit test over NodeVersion and preload_injection_for. Nothing in CI ever executes nub on a 23.0–23.4 Node, so a future regression in the runtime JS — preload.mjs's probe, say — would go unnoticed exactly the way this one did. The repo already has the machinery for this; it just needs one more entry.
Technical details
# Add a 23.0–23.4 leg so the fixed band is actually exercised
## Affected sites
- `.github/workflows/ci.yml:167` — `test_tiers` is `22.15`, `22.13`, `20.11`, `18.19` (plus `24` via `test_os`). No 23.x anywhere.
- `.github/workflows/ci.yml:906-918` — the `install_node` block side-installs 22.13.0 / 18.18.0 / 20.11.0 / 26.5.0 and exports them as `TEST_NODE_BIN_<MAJ>_<MIN>_<PAT>`.
- `crates/nub-cli/tests/version_tiers.rs` — already consumes those vars via `find_node_bin_dir`, skipping when absent, which is the established route for pinning a Node from a Rust test.
## Required outcome
- A test that runs the real `nub` binary against a real 23.0–23.4 Node and asserts the process starts and lands on the compat tier — i.e. it would have caught the original `TypeError` rather than only the predicate that caused it.
## Suggested approach (optional)
- One line in the `install_node` block (`dir_23_4=$(install_node 23.4.0)` → `TEST_NODE_BIN_23_4_0`) plus a `version_tiers.rs` case. That reuses the existing skip-when-absent convention, so it costs nothing locally.
- 23.4.0 is the strongest single pick: it is the last release before the API landed, so it pins the exact boundary.ℹ️ Nitpicks
runtime/preload-common.cjs:308still justifies treating any--importit sees as FOREIGN with "its compat-tier--import preload.mjslives below 22.15 (outside the broken band this gates on)". This PR makes that clause false — the compat tier now overlapsnodeHookComposeBroken()'s 22.15–24.11 band on 23.0–23.4. I traced it and found no live self-detection bug (both injection sites callsplit_inherited_preloadsfirst, so the two token flavors never coexist), but it is a load-bearing invariant stated incorrectly in a file this PR already edits.- Four more places restate the old boundary in prose and were not swept:
runtime/preload.cjs:21("The compat tier (< 22.15)"),runtime/pnp-bin-run.cjs:13("on the compat tier (Node <22.15"), andcrates/nub-core/src/node/spawn.rs:3234and:5729. All are comments; the code at each is correct because it goes throughsupports_augmentation().
Claude Opus ｜ 𝕏
`nub compile`'s external-shim gate kept its own `22.15.0` floor, so a
`--external` / `--allow-dynamic-import` build targeting Node 23.0-23.4 was
accepted and the generated `__nub_external.mjs` called `registerHooks`
unconditionally — the artifact died at startup with the same TypeError the
tier fix addresses. A bare `--target 23` floors at 23.0.0, landing in the band.
The gate now calls `supports_augmentation`, so the band lives in one place.
`fast_tier_floor_for_line` is new on NodeVersion and derives the floor for a
version's own release line; the refusal uses it to suggest 23.5 to a 23.x
target instead of 22.15, and drops the "which older Node does not have"
ordering claim. A 22.15 suggestion now carves the band back out, since a bare
"or newer" from there would re-admit it.
The preload.cjs async fallback added for that band registered the loader
worker but never installed the main-thread CJS require hooks, so a 23.0-23.4
grandchild reached through inherited NODE_OPTIONS had no transpile path at all
for `require('./x.ts')`: no registerHooks until 23.5, no native type stripping
until 23.6, no classic shim. It now installs them exactly as preload.mjs does
on the same band, gated on the transform core being loaded — the
require(esm)-disabled entry has none. The branch comment described only that
entry's rationale and now covers all three.
Also stop the native-deps harness from discarding verify-load.cjs output: a
native addon that aborts rather than throws took the script down under `set -e`
through a failed command substitution, leaving a bare "exit code 134" and no
diagnostics.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Both open findings from the last pass are closed by e9dbd339, and the compile-gate fix is the right shape: it deletes the duplicate floor rather than re-stating the band. What is left is two comments that describe their own code slightly more broadly than the code delivers.
Reviewed changes — commit e9dbd339 on top of the previously-reviewed 45f3ed27, plus the surrounding gate-input code in crates/nub-cli/src/compile/mod.rs and the acceptance predicate in crates/nub-launcher/src/main.rs.
- Rewired
check_node_supportoff its private 22.15.0 floor — the gate is now!plan.needed() || version.supports_augmentation(), so the compile path and the spawn path key on one predicate. This was the IMPORTANT finding from the last review. - Added
NodeVersion::fast_tier_floor_for_line()so the refusal names a floor on the release line the user already targets, andsupports_augmentation()now delegates to it instead of carrying its own branch. - Rewrote the refusal message to lead with
module.registerHooks, state both line floors, and carve the hole out (other than 23.0 through 23.4) whenever it suggests the 22.x floor. I checked every version that can reach the message: a major-23 version necessarily lies in 23.0–23.4 (23.5+ early-returns), so the(or newer)form is only ever emitted from the 23.5.0 floor. The old wording would have told a 23.4 user to downgrade to 22.15 — that is gone. - Added two
external.rsunit tests — 23.0.0 / 23.4.0 / 23.4.99 refused with--target 23.5.0and explicitly not--target 22.15.0; 22.15.0 / 23.5.0 / 23.11.1 / 24.0.0 accepted; and a 20.19.0 case pinning the carve-out clause. Both go red against the pre-fix gate. - Installed
installCjsRequireHooks(core, !process.features?.typescript)inpreload.cjs's async branch and re-documented the branch's three entry conditions. ThewithClassicTranspileargument is byte-identical topreload.mjs:114's!__hasNativeTs, so it inherits an already-accepted tradeoff rather than introducing a new one. That closes the inline thread from the last pass, which I have replied to and resolved. - Hardened
tests/native-deps/run.shto captureverify-load.cjs's exit status separately, so a SIGABRT in a native addon prints diagnostics instead of taking the script down with a bare 134 underset -e. Correct, and unrelated to the tier fix.
One thing worth stating plainly since it looks like a regression and is not: --smol --target 23 now fails the gate. That is a true positive. A bare major pin floors at 23.0.0 and SmolTarget::matches accepts anything >= floor with no upper bound, so such an artifact really can be handed a 23.2 at run time.
ℹ️ Nitpicks
runtime/preload.cjs:190and:205scope the new shim call to "an inherited-NODE_OPTIONS23.0–23.4 grandchild" and "Entry 3 only", but the guard!hasRegisterHooks && corealso fires on 20.19–20.x and 22.12–22.14 —require(esm)was unflagged at 20.19.0 / 22.12.0, four minors beforeregisterHooksreached 22.15.0, socoreloads and the branch is entered on plain compat-tier Nodes too. The behavior there is right (it matches whatpreload.mjs:114does on the same versions); the comment just names a narrower band than the code serves.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Both open threads from the last pass are closed by 80476e09, and it is a comment-only commit: no behavioral surface at all. I re-derived both claims it now makes against the source rather than taking them on the text, and both hold — one of them just stops one shape short.
Reviewed changes — commit 80476e09 on top of the previously-reviewed e9dbd339, plus the pin-shape plumbing in crates/nub-cli/src/compile/mod.rs, crates/nub-core/src/version_management/mod.rs, and SmolTarget::matches in crates/nub-launcher/src/main.rs.
- Widened the
KNOWN GAPnote to cover theMajorMinorpin — it now says a major.minor pin "carries no range into the manifest, soSmolTarget::matchesfalls back tocandidate >= floor". Verified:smol_version_rangecarries a range only underVersionPin::Range(_) if range_minimum_is(pin, gate), andrange_minimum_isis itselfmatches!(pin, VersionPin::Range(..)), so aMajorMinorpin reaches the launcher withrange: None, exact: false. This was the run-2 inline finding, replied to and resolved. - Rewrote the async-branch entry enumeration in
preload.cjsfrom "available on two of them" to "installed on exactly ONE of them", and gave entry 2 (forceAsyncTier) an explicit statement of what it forgoes. Verified the count: entry 1 hascore === null, entry 2 sits entirely insidenodeHookComposeBroken()'s 22.15.0..=24.11.0 band sohasRegisterHooksis always true there, and entry 3 is the only one where!hasRegisterHooks && coreholds. The named forfeit — loader-workerimportcoverage, no main-thread_resolveFilenamepatch — matchesinstallCjsRequireHooksand is genuinely unchanged from before this PR. This closes the other run-2 thread by documenting rather than widening, which was one of the two options offered. - Softened the
ShimPlan::flagdoc comment off its "both share the 22.15 floor" phrasing, now that there is no single floor to share.
One thing I checked and cleared so it does not come back: the "caught" half of the same sentence lists an exact three-part target, which does not floor inside the band when the target is a 22.x one. That is still correct, because smol_requires_exact_target is matches!(pin, VersionPin::Exact(_)) and SmolTarget::matches then compares candidate == floor — an exact pin cannot drift upward into the hole at all.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
aeb4a76f is comment-only and applies the last pass's suggestion verbatim, so the open thread is closed and resolved. I re-derived the Alias claim in source rather than taking it on the text and it holds: resolve_pin_floor_in_index routes Alias to the newest-matching arm (version_management/mod.rs:878), smol_requires_exact_target and smol_version_range both decline it, so it reaches SmolTarget::matches with range: None, exact: false.
Reviewed changes — commit aeb4a76f on top of the previously-reviewed 80476e09, one hunk of three lines, plus an exhaustive re-trace of every VersionPin variant through the gate, the manifest writers, and the launcher's acceptance predicate.
- Added the
lts/<codename>alias to theKNOWN GAPenumeration — the note now names three shapes and groups the alias with the major.minor pin as the two that carry no range into the manifest. Accurate as written.
The one thing I would change is structural rather than factual. This is the third consecutive pass in which the closed count came up one short, and it is short again — an upper-only range (--target "<23") takes the same newest-matching floor path the alias does. Rather than a fourth item, the note is better off stating the invariant, which is what the inline comment proposes.
Claude Opus ｜ 𝕏
|
Shipped in v0.8.1: https://github.com/nubjs/nub/releases/tag/v0.8.1 |
Nub crashes at startup on Node 23.0 through 23.4 with
TypeError: module_.registerHooks is not a function.module.registerHooksis a semver-minor that shipped on the 23.x line at 23.5.0 and on the 22.x LTS line only later, at 22.15.0. The tier gate compared against 22.15.0 alone, so 23.0-23.4 sorted above the fast floor without having the API and was handed--require preload.cjs.The fix excludes the band in
supports_augmentation, the one predicate every downstream gate keys on. Two JS gates carried the same defect and now probe the capability instead of the version.Verified on real 18.19.0, 22.14.0, 22.15.0, 23.0.0, 23.3.0, 23.4.0, 23.5.0, 23.6.0, 24.0.0, 24.19.0. Both new tests confirmed red without the fix.