Skip to content

fix(preload): one NODE_OPTIONS token per flag name, and resolve bare preload entries from the CWD - #673

Merged
colinhacks merged 7 commits into
mainfrom
preload-token-fixes
Aug 5, 2026
Merged

fix(preload): one NODE_OPTIONS token per flag name, and resolve bare preload entries from the CWD#673
colinhacks merged 7 commits into
mainfrom
preload-token-fixes

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Three fixes to how nub delivers preloads through NODE_OPTIONS, plus one npm-parity gap found alongside them.

The bug

Any consumer that re-parses NODE_OPTIONS destroys repeated same-name flags. Next.js parses it into a Record keyed by option name and reformats it for every forked worker, so --require=a --require=b collapses to --require=b. nub emitted one token per nub.jsonc preload entry on top of its own, so a single .cjs preload entry silently dropped nub's own preload — and with it the entire augmentation layer.

Measured on a real next@16.3.0 build, counting the processes nub's augmentation actually reached:

--require tokens nub-augmented processes
before 2 3 of 8
after 1 8 of 8

Exit 0 in both cases. Filed upstream as vercel/next.js#96582; the analysis is also on the existing #77550.

What changed

One token per flag name. nub writes a single chainer module that loads the user's entries in declared order, instead of one token each. A .cjs-only list rides nub's own --require preload, so no second token exists and --require's synchronous entry semantics survive. Anything else gets a .mjs chainer on its own --import, which also preserves top-level await — require() cannot load a TLA module at all.

Bare entries resolve from the CWD. Loading entries through a generated file changed where a bare specifier resolves from: the file's directory rather than the CWD. In a workspace where a member shadows a root dependency, cd packages/web && nub app.js loaded the ROOT copy where plain Node loads the member's — same specifier, different module, no error. nub now resolves bare entries with oxc_resolver using the condition set the channel implies, so the anchor is a decision rather than a side effect of where the file was written. This resolution is cold and pre-V8, so none of the JS-runtime inputs to Node's algorithm exist yet; nub's runtime hooks still delegate every in-process resolution to Node.

nub run honors node-options. npm and pnpm both apply the node-options npmrc field to script execution. nub honored it only for lifecycle scripts during PM operations, so nub run dropped it — a project raising its heap ceiling for a build would OOM under nub and succeed under pnpm. It is appended to nub's augmentation rather than assigned over it, so nub's preload survives where npm's own handling destroys the ambient value.

Verification

  • 204-cell matrix, all passing: 3 Node versions (22.14.0 compat tier, 22.15.0 fast-tier floor, 26.5.0) × {CJS, type: module} × 17 preload combinations × {.js, .mjs} entry. Each cell asserts every entry ran exactly once, in declared order, before the entry, with at most one --require and one --import, and nub's own token present.
  • Merge-base differential: an earlier run of the same matrix against the merge-base showed identical execution behavior in every cell — only the token shape changed.
  • Positive controls: the Next.js failure reproduces on the merge-base and not after; the anchor regression test was confirmed to fail (ROOT instead of MEMBER) with the fix removed.
  • Edges: missing preload, throwing preload, --node compat mode, cwd outside the project root, no node_modules, nub run, worker threads, six concurrent runs, and a real Yarn PnP project.
  • clippy --all-targets --all-features and fmt --check clean.

verify_deps.rs has three failures on this host (npmrc_error_aborts_before_running, fresh_clone_warns_but_still_runs_the_script, version_drift_warns_and_names_the_dependency). They reproduce identically on the merge-base and CI is green on main, so they are environmental rather than introduced here.

Rough edges, deliberately left

  • The chainer is written to node_modules/.nub/. Now that entries resolve to absolute paths it no longer needs to live inside the project, so it could move to nub's own cache dir — including for Yarn PnP projects, where nub currently creates a node_modules directory that would otherwise not exist.
  • An unresolvable bare entry produces Node's own ERR_MODULE_NOT_FOUND naming the chainer rather than nub.jsonc. The generated file's header comment names its source, so diagnosis is one step.
  • Under Yarn PnP, oxc_resolver is built without the yarn_pnp feature and cannot resolve; the entry passes through and PnP's own resolver handles it. Verified end-to-end, but those entries do not get the CWD anchor.
  • user_preload_injections in spawn.rs and its tests are now unused. They are marked superseded in place rather than deleted, since they still document the two measured Node facts that govern which channel the chainer rides.

Colin McDonnell and others added 4 commits August 3, 2026 17:34
npm and pnpm both apply `node-options` from .npmrc to script execution;
nub honored it only for lifecycle scripts during PM operations, so
`nub run` silently dropped it. Its dominant use is raising the heap
ceiling for a build, which made a project that needs it fail under nub
and succeed under pnpm, with no diagnostic.

The value is appended to nub's own augmentation rather than assigned
over it, so nub's preload survives where npm's own handling destroys
the ambient NODE_OPTIONS. It lands before nub.jsonc `nodeOptions` so
the tool-owned surface wins a conflicting flag under Node's last-wins
rule, and is skipped in compat mode, matching nub.jsonc `nodeOptions`.

Adds split_node_options, the inverse of node_options_token: a raw
NODE_OPTIONS string from outside nub.jsonc has to arrive as one element
per flag, because compute_augmentation_env re-quotes each element and
would otherwise emit the single broken token "--a --b".
… chainer

nub emitted one NODE_OPTIONS token per nub.jsonc `preload` entry. Any
consumer that re-parses NODE_OPTIONS destroys repeated same-name flags:
Next.js keys it by option name and reformats it for every forked worker,
so `--require=a --require=b` collapses to `--require=b`. Since nub's own
preload is the first --require, a single .cjs preload entry silently
dropped nub's entire augmentation layer under next dev/build. Measured
end-to-end on a real next@16.3.0 build; filed as vercel/next.js#96582.

nub now writes one module that loads the entries in declared order, and
emits at most one --require and at most one --import.

The chainer is written INSIDE the project (<preload_root>/node_modules/
.nub/), which is what makes bare entries work: `dotenv/config` resolves
through that project's node_modules walk-up from the chainer's own
directory, exactly as Node resolves the same specifier on a --require
token. The identical file outside the project fails with
ERR_MODULE_NOT_FOUND. nub cannot resolve these itself -- its resolver is
additive-only and returns null for every bare specifier, because
node_modules and exports are deliberately Node's.

It loads after nub's hooks install, so entries keep full augmentation: a
.ts entry still transpiles and its tsconfig paths alias still resolves.

Channel follows the tier rules the per-entry router encoded: a .cjs-only
list rides nub's own --require preload so synchronous entry semantics
survive; anything else gets a .mjs chainer on its own --import, which
also preserves top-level await (require() raises
ERR_REQUIRE_ASYNC_MODULE).
The chainer introduced in the previous commit resolves a bare `nub.jsonc`
`preload` entry from the generated file's own directory. Node resolves a
bare --require/--import specifier from the CURRENT WORKING DIRECTORY, and
nub did too before the chainer, because it emitted the bare specifier as
its own token and let Node resolve it.

The divergence is silent and reachable in the layout nub targets. In a
workspace where a member shadows a root dependency, `cd packages/web &&
nub app.js` loaded the ROOT copy where plain node loads the member's.
Same specifier, different module, exit 0. Measured against a plain-node
reference; regression test asserts the member copy wins and was confirmed
to fail ("ROOT") with the fix removed.

nub now resolves bare entries itself with oxc_resolver, using the
condition set the chainer's channel implies, so the anchor is a decision
rather than a side effect of where nub wrote the file. Path-like entries
are unaffected -- they are already absolute, anchored at the nub.jsonc
that declared them.

This resolution is cold and pre-V8: nothing has executed, so none of the
JS-runtime inputs to Node's algorithm exist yet, which is what makes a
static resolver sound here. nub's runtime hooks still delegate every
in-process resolution to Node.

An unresolvable bare entry passes through untouched so Node raises its
own ERR_MODULE_NOT_FOUND naming the specifier. That fallback also covers
Yarn PnP, where oxc (built without the yarn_pnp feature) cannot resolve
and PnP's own resolver -- already installed via nub's .pnp.cjs token --
takes over; verified end-to-end in a real PnP project.
Copilot AI lite review requested due to automatic review settings August 5, 2026 00:28
@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 5, 2026 1:49am

Request Review

Self-review: the constructor returned Option but never returned None --
the no-readable-cwd case it documented lives in resolve_bare_preload.

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.

Pull request overview

This PR hardens Nub’s preload delivery via NODE_OPTIONS by collapsing multiple user preload entries into a single synthesized “chainer” module (avoiding repeated-flag loss in NODE_OPTIONS re-parsers), restoring correct CWD-based resolution for bare preload specifiers, and extending npm/pnpm parity by honoring the node-options npmrc field for nub run.

Changes:

  • Generate a single preload chainer module and ensure at most one --require and one --import token are emitted.
  • Resolve bare preload entries from the current working directory (rather than the generated file’s directory).
  • Apply npmrc node-options to nub run without displacing Nub’s augmentation preload; add regression tests.

Reviewed changes

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

Show a summary per file
File Description
runtime/preload.mjs Loads user preload chain last (ESM path) so user entries observe a fully augmented realm.
runtime/preload.cjs Loads user preload chain last (CJS path) on fast tier.
runtime/preload-common.cjs Adds runtime-config-driven user preload chain loader helpers (require/import).
crates/nub-core/src/node/version.rs Exposes supports_augmentation for cross-crate tier decisions.
crates/nub-core/src/node/spawn.rs Adds file_url_for + split_node_options; keeps superseded per-entry preload routing for documentation/tests.
crates/nub-cli/tests/project_runtime_config.rs Adds regression tests for npmrc node-options, token collapsing, and CWD anchoring for bare preloads.
crates/nub-cli/src/project_config.rs Extends runtime config wire format with preloadRoot and preloadChain.
crates/nub-cli/src/pm_engine/mod.rs Adjusts lifecycle augmentation to pass mutable runtime config into option assembly.
crates/nub-cli/src/cli.rs Implements chainer synthesis, bare-preload resolution, and npmrc node-options ingestion.
crates/nub-cli/Cargo.toml Adds oxc_resolver dependency for cold, pre-V8 bare-preload resolution.
Cargo.lock Locks new transitive dependencies from oxc_resolver.

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

Comment thread crates/nub-cli/src/cli.rs
Comment on lines +3197 to +3208
let resolver = bare_preload_resolver(esm);
for spec in &runtime.preload {
let spec = resolve_bare_preload(resolver.as_ref(), spec);
// JSON string escaping is exactly JS string escaping for our purposes, and it
// is what makes a Windows path or a quote in a specifier safe to embed.
let literal = serde_json::to_string(&spec)?;
if esm {
body.push_str(&format!("import {literal};\n"));
} else {
body.push_str(&format!("require({literal});\n"));
}
}
Comment thread crates/nub-cli/src/cli.rs Outdated
Comment on lines +3236 to +3237
/// `None` when the process has no readable cwd, in which case bare entries are left
/// alone and Node raises its own error.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Two of the three changes here introduce silent behavior regressions against main. The chainer is only loaded on one of the two branches runtime/preload.cjs can take, so a .cjs-only preload list is dropped with no error on a reachable configuration; and the new CWD anchor uses nub's own process cwd, which is the wrong directory in exactly the workspace fan-out case the new anchor test is meant to protect.

Reviewed changes — the full 11-file diff at 2ea77f61, plus the surrounding preload/spawn plumbing the diff does not show (runtime/preload.cjs, runtime/preload-async-hooks.mjs, compute_augmentation_env, the workspace fan-out call sites, and apply_lifecycle_augmentation).

  • One NODE_OPTIONS token per flag nameprepare_preload_chain writes a generated chainer to <preload_root>/node_modules/.nub/preload-chain.{cjs,mjs} that loads every nub.jsonc preload entry in declared order, replacing one token per entry. Either it gets its own --import (fast tier, any non-.cjs entry) or it rides nub's own preload with no token at all.
  • Bare entries pre-resolved in Rust — a new oxc_resolver dependency turns a bare preload entry into an absolute path before Node starts, with the condition set the chainer's channel implies.
  • nub run honors the .npmrc node-options field — appended before nub.jsonc nodeOptions so the tool-owned value wins under Node's last-wins rule, with a new split_node_options tokenizer.
  • Plumbingruntime_node_options takes &mut RuntimeConfig at all five call sites, RuntimeConfig gains preload_root + preload_chain (serde default, so the cross-version __NUB_RUNTIME_CONFIG wire format still round-trips), and supports_augmentation / to_file_url become public.
  • Tests — three new integration tests (token count per flag, npmrc precedence, bare-entry anchor) and a split_node_options round-trip unit test. user_preload_injections and its tests are retained and marked superseded.

⚠️ The npmrc node-options field still misses the path npm and pnpm document it for

npm's own config docs scope this field to lifecycle scripts — "does not impact how npm itself is executed but it does impact how lifecycle scripts are called" — and pnpm's nodeOptions docs repeat the sentence verbatim. The new reader is wired only into build_script_command, so nub install's lifecycle scripts still go through the older, file-blind mechanism in apply_lifecycle_augmentation, and the same field now has two divergent implementations.

Technical details
# `node-options` reaches `nub run` but not lifecycle scripts

## Affected sites
- `crates/nub-cli/src/cli.rs:4728` — the only call site of `npmrc_script_node_options`, inside
  `build_script_command`, which is reached only from `spawn_script` / `spawn_script_prefixed`
  (`nub run`, including the workspace fan-out).
- `crates/nub-cli/src/pm_engine/mod.rs:1763` — lifecycle scripts read only the
  `NPM_CONFIG_NODE_OPTIONS` / `npm_config_node_options` ENV form, only as a seed when the
  ambient `NODE_OPTIONS` is unset, and never consult `.npmrc` at all.
- Not reached at all: `apply_exec_augmentation` (`nub exec` / `nubx`), `run_watch`, the plain
  file run.

## Required outcome
- One implementation of the field, applied on the surfaces npm and pnpm document it for —
  lifecycle scripts included — with the same append-not-assign discipline the new function uses.
- The docstring at `cli.rs:3284-3302` should say which surfaces are covered, since it currently
  reads as a general parity claim.

## Open questions for the human
- pnpm 11 moved scalar settings out of `.npmrc` into `pnpm-workspace.yaml` / global
  `config.yaml`, so `nodeOptions` is not an npmrc field on the current pnpm major. Per the
  per-major compat rule that is a separate target; per the brand boundary a pnpm-NAMED file is
  only readable when pnpm is the incumbent. Is honoring it for a pnpm-11 incumbent in scope, or
  is npm + pnpm ≤10 the intended target?
- Should the `nub exec` / `nubx` / `nub watch` paths get the field too, or is `nub run` +
  lifecycle deliberately the whole surface?

ℹ️ user_preload_injections is dead but retained with no deletion plan

The PR notes this deliberately, and the reason given is sound — the doc comment carries two measured Node facts that still govern the chainer's channel. Worth resolving in this PR rather than leaving a dead router beside a live one, because the retained facts are load-bearing for the branch gap flagged inline: "every module.register() loader worker RE-RUNS the --require preloads in its own realm" and "--import preloads are skipped in loader workers" are precisely what the new design has to reason about. Either restate them at prepare_preload_chain and delete the function plus its tests, or file a follow-up so the next reader does not update the superseded one.

ℹ️ Nitpicks

  • bare_preload_resolver (crates/nub-cli/src/cli.rs:3238) always returns Some, but its doc describes a None-when-there-is-no-readable-cwd case that is actually implemented in resolve_bare_preload. Either drop the Option or move the cwd probe into the constructor so the doc matches.
  • split_node_options (crates/nub-core/src/node/spawn.rs:2921) separates on char::is_whitespace(), where Node's ParseNodeOptionsEnvVar separates only on ASCII space — a tab inside a node-options value becomes two tokens under nub and stays one under Node. The divergence errs toward being more forgiving, but the doc comment claims the parser is mirrored.
  • preload_entries_collapse_to_one_token_per_flag_name uses only relative-path entries, so resolve_bare_preload never executes in it, and the anchor test covers only the single-invocation path. A bare entry in the token-count matrix and a -r case in the anchor test would both have caught findings above.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread runtime/preload.cjs
// alias resolves, matching what the old per-entry `--require` token gave by
// sitting after nub's own. A no-op unless the spawn path put the chainer on nub's
// preload rather than its own `--import`. See requireUserPreloadChain.
common.requireUserPreloadChain();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the only call site of requireUserPreloadChain, and it sits inside the !requireEsmDisabled && !forceAsyncTier branch. The async loader-worker else branch has no equivalent call, and preload-async-hooks.mjs never loads the chain either — so a .cjs-only preload list on the fast tier, which is exactly the case prepare_preload_chain deliberately delivers with no NODE_OPTIONS token, is silently skipped whenever that branch fires. Pre-PR those entries were their own --require tokens that Node processed independently of nub's internal tier fallback.

Technical details
# The async loader-worker branch never loads the preload chainer

## Affected sites
- `runtime/preload.cjs:175``common.requireUserPreloadChain()`, the only call site, inside the
  sync fast-tier branch.
- `runtime/preload.cjs:176-207` — the `else` branch (entered when `requireEsmDisabled` or
  `forceAsyncTier`) registers the loader worker and installs polyfills, but never touches the
  chain. `runtime/preload-async-hooks.mjs` has no chain logic either.
- `crates/nub-cli/src/cli.rs:3178-3183, 3225-3228` — the Rust channel decision is static and
  cannot see which branch the JS will take: with `esm == false` it sets
  `runtime.preload_chain = Some(path)` and returns `Ok(None)`, emitting no token, so this call is
  the *only* thing that runs the user's entries.

## How a user reaches it
All three require an all-`.cjs` `preload` list on Node >= 22.15:
- Ambient `NODE_OPTIONS="--import tsx/esm"` (tsx's documented CI/shell delivery) on Node
  22.15.0-24.11.0 — `shouldAutoAsyncTierAtPreload()` fires via `foreignAsyncLoaderFlagPresent()`.
- `__NUB_FORCE_ASYNC_TIER`, set by the launcher's predictive argv scan when nub spawns
  tsx/ts-node on that band.
- `--no-experimental-require-module` in `nodeOptions` or `NODE_OPTIONS`, which sets
  `requireEsmDisabled`.
Outcome in each: every entry silently does not run. No warning, no stderr, exit 0.

## Required outcome
- The chainer must load on every branch `preload.cjs` can take whenever
  `runtime.preload_chain` is set, since no `NODE_OPTIONS` token exists in that configuration.
- Ordering must stay "after nub's augmentation", as the fast-tier branch has it.
- A test that drives the async-tier branch (e.g. ambient `NODE_OPTIONS=--import <a loader>` on
  the broken band, or `--no-experimental-require-module`) with a `.cjs`-only list, asserting the
  entry ran. The current 204-cell matrix cannot see this because it never forces that branch.

## Suggested approach
The chainer is `.cjs` in every case that reaches this path, so `module.require(chain)` works in
the `else` branch too — hoisting the call to just after the `if`/`else` block covers both while
keeping it last. The `require()`-side transpile shim is unavailable in async-tier mode, which is
the documented degradation for that tier and applies to the user's entries the same way.

Comment thread crates/nub-cli/src/cli.rs
let Some(resolver) = resolver else {
return spec.to_string();
};
let Ok(cwd) = std::env::current_dir() else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

current_dir() is nub's own process cwd, which is not the directory the child will run in. build_script_command sets command.current_dir(&project.root) — the member dir during a -r / --filter fan-out — and run_one_workspace_bin threads member.dir into run_file_in_dir, while nub's own cwd never changes across the fan-out. So nub -r run build resolves every member's bare entry from the invocation directory, which is the wrong-copy behavior a_bare_preload_entry_resolves_from_the_cwd_like_node_does pins for the single-invocation case.

Technical details
# The bare-entry anchor is nub's process cwd, not the child's cwd

## Affected sites
- `crates/nub-cli/src/cli.rs:3275-3281``resolve_bare_preload` reads `std::env::current_dir()`
  and resolves against it.
- `crates/nub-cli/src/cli.rs:4812``build_script_command` sets the child's cwd to
  `project.root`, which for `run_one_workspace_script` is a synthesized `Project` whose `root` is
  `member.dir` (`cli.rs:4466`). nub's own cwd is unchanged; neither fan-out loop calls
  `set_current_dir`.
- `crates/nub-cli/src/cli.rs:6278` — the workspace-bin path calls
  `run_file_in_dir(.., cwd = member.dir, ..)`, whose whole documented purpose is an explicit cwd
  that overrides the process cwd and is threaded onto `SpawnConfig`. `prepare_preload_chain`
  ignores it.
- Unaffected (the two directories coincide): `run_file_with_compat`, `run_watch`, and
  `apply_lifecycle_augmentation`.

## What the user observes
Workspace root `nub.jsonc` with `"preload": ["shadowed"]`, and
`packages/web/node_modules/shadowed` shadowing the root copy:
- `cd packages/web && nub app.js` — correct, loads the member's copy (this is what the new test
  covers).
- `nub -r run start` or `nub exec -r <bin>` from the root — every member loads the ROOT copy.
  Plain `node --require shadowed` with the member as cwd loads the member's copy, and so did nub
  before this PR, because the bare specifier was emitted verbatim and Node resolved it in the
  child.

## Required outcome
- Resolution must be anchored at the cwd the spawned child will actually get, per call site,
  rather than at nub's process cwd.
- The fan-out cases need coverage: a `-r run` and an `exec -r` case in the anchor test would
  fail today.
- Note the interaction with the chainer's fixed filename (separate comment): once resolution is
  per-member, one nub process writes several different chainers, so the path has to vary with the
  content.

Comment thread crates/nub-cli/src/cli.rs
let dir = root.join("node_modules").join(".nub");
std::fs::create_dir_all(&dir)
.with_context(|| format!("could not create the preload chainer dir {}", dir.display()))?;
let path = dir.join(if esm {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The chainer path is fixed per project and channel, but its contents now vary with the resolving cwd. Two concurrent nub runs in one project from different directories therefore overwrite each other's chainer, and since the --import token is inherited, a process that forks workers later (the Next.js case this PR targets) can pick up the other run's resolution. This also blocks the per-child-cwd fix for the anchor bug, since a single fan-out would need several different contents at this one path.

Technical details
# A fixed chainer filename with cwd-dependent contents

## Affected sites
- `crates/nub-cli/src/cli.rs:3188-3192` — the path is `preload-chain.{mjs,cjs}` under
  `<preload_root>/node_modules/.nub`, keyed only on the channel.
- `crates/nub-cli/src/cli.rs:3209-3215` — the temp-file + rename makes each write atomic, so a
  torn read is impossible, but the last writer still wins for every reader.
- `crates/nub-cli/src/cli.rs:3198-3199` — contents are a function of `resolve_bare_preload`,
  i.e. of the cwd, which is what makes the shared path unsafe now.

## Required outcome
- Two nub invocations in one project whose preload lists resolve differently must not be able to
  observe each other's chainer.
- The naming scheme has to support several live chainers in one project at once, so the
  per-child-cwd fix for the anchor bug can write one per resolution.

## Suggested approach
Content-address the file name — hash the resolved entry list (plus the channel) into it. That
also makes the write idempotent for the common case, so the concurrent-same-content path stops
rewriting a shared file at all.

Comment thread crates/nub-cli/src/cli.rs
});

let dir = root.join("node_modules").join(".nub");
std::fs::create_dir_all(&dir)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Propagating this error turns an unwritable project directory into a hard abort for every augmented command in a project that declares a preload — including apply_exec_augmentation, whose contract is "No-op if augmentation can't be set up". A read-only checkout (a read-only container mount, a Nix store path) ran fine before. Relatedly, a preload declared in the global ~/.config/nub/nub.jsonc anchors preload_root at the config dir, so nub creates ~/.config/nub/node_modules/.nub/ there.

Technical details
# The chainer write is a hard failure, in a directory nub may not own

## Affected sites
- `crates/nub-cli/src/cli.rs:3186-3187` and `3212-3215``create_dir_all`, `write` and `rename`
  all propagate with `?` through `runtime_node_options`.
- `crates/nub-cli/src/cli.rs:6373-6383``apply_exec_augmentation` documents itself as a no-op
  when augmentation cannot be set up, but now returns the error.
- `crates/nub-cli/src/project_config.rs:728-729``preload_root` is
  `source_root(ConfigKey::Preload)`, and `ConfigSource::file` sets `root` to the config file's
  parent, so a global-only `preload` (allowed: `preload` is in `ROOT_KEYS` and the global parser
  retains it) puts the chainer in `~/.config/nub/node_modules/.nub/`.
- Yarn PnP projects get a `node_modules` directory created that would not otherwise exist — the
  PR notes this under rough edges.

## Required outcome
- A project directory nub cannot write must not abort an otherwise-working run, or if it must,
  it should fail with a message naming `nub.jsonc` `preload` rather than a raw I/O context.
- The chainer should not be written into the user's global config directory.

## Suggested approach
The PR's own rough-edge note applies: now that entries resolve to absolute paths, the chainer no
longer needs to live inside the project, so writing it under nub's cache dir addresses the
read-only case, the global-config case, and the PnP `node_modules` creation at once. The one
remaining reason to stay in-project is the PnP fallback where `oxc_resolver` cannot resolve and
the bare specifier is passed through — worth deciding explicitly rather than inheriting.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ The one new commit is a clean mechanical refactor. The findings from the previous review are still open.

Reviewed changes — only 91ea397b, the single commit pushed since the previous review at 2ea77f61.

  • bare_preload_resolver no longer returns an Option — it returns oxc_resolver::Resolver directly, resolve_bare_preload takes &oxc_resolver::Resolver, and the "no readable cwd" comment moved down to the current_dir() probe that actually implements it. Behavior is unchanged: the removed Some(..)/let Some(..) else pair was unreachable, and the early-return for an unreadable cwd is untouched. This resolves the nitpick from the previous review.

ℹ️ Nothing in this commit touches the four open findings

Recorded so the state is unambiguous, since the refactor lands in the same function the anchor finding is about: 91ea397b changes only a signature. The async-tier chainer gap in runtime/preload.cjs, the process-cwd anchor in resolve_bare_preload, the fixed chainer filename, and the hard-failing chainer write all still stand as written in the previous review.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Windows CI caught this: an absolute PATH is not a valid ESM specifier
there. POSIX tolerates a bare `/abs/path`, so the macOS sweep could not
see it, but Windows rejects `C:\...` with

  ERR_UNSUPPORTED_ESM_URL_SCHEME: On Windows, absolute paths must be
  valid file:// URLs. Received protocol 'c:'

which took out the whole project_runtime_config suite, since its shared
fixture declares a relative preload that resolves to an absolute path.
A malformed specifier also reached nub's own load hook, surfacing as a
second error from transform-core's fileURLToPath.

The ESM chainer now converts absolute paths to file:// URLs on both
platforms rather than branching. `require` still takes the raw path, and
a BARE specifier stays bare on either channel so Node resolves it.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ The one new commit is a correct, narrowly-scoped platform fix. The four findings from the earlier review are all still open.

Reviewed changes — only a78b1079, the single commit pushed since the previous review at 91ea397b.

  • Absolute ESM chainer entries are emitted as file:// URLs — the emit loop in prepare_preload_chain (crates/nub-cli/src/cli.rs:3200-3209) routes a specifier through file_url_for when the chainer is ESM and the specifier is an absolute path. Bare specifiers stay bare on either channel, and the require channel keeps raw paths. This repairs a Windows-total break that 242bddcb introduced: the ESM chainer was emitting import "C:\…", which Node rejects outright with ERR_UNSUPPORTED_ESM_URL_SCHEME.

I traced the two things that could have made this a trade rather than a fix, and neither holds. It is POSIX-neutral: import "/abs/x.mjs" and import "file:///abs/x.mjs" resolve to the same URL against the chainer's file: base, and every path shape that misbehaves under one (#, ?, %, a literal \ on POSIX) misbehaves identically under the other, so no previously-working path stops working. And both host-bound halves agree on platform — Path::is_absolute and file_url_for's cfg!(windows) — so this does not reproduce the predicate/formatter disagreement is_absolute_path (crates/nub-core/src/node/spawn.rs:2795) exists to warn about.

The fix is also guarded, which is worth recording since the bug it repairs is invisible on a POSIX host: preload_entries_collapse_to_one_token_per_flag_name asserts nub -e exits 0 for ./a.mjs entries, and ci.yml's matrix-plan puts a windows-latest leg in the test job on pull_request, so a recurrence surfaces as a red leg rather than silence.

ℹ️ Nothing in this commit touches the four open findings

Recorded so the state is unambiguous, since the commit lands inside prepare_preload_chain, which two of them are about. The async-tier chainer gap in runtime/preload.cjs, the process-cwd anchor in resolve_bare_preload, the fixed chainer filename with cwd-dependent contents, and the hard-failing chainer write all stand exactly as written.

Technical details
# Prior findings unaffected by `a78b1079`

## Affected sites
- `runtime/preload.cjs:175``common.requireUserPreloadChain()` is still called only inside the
  `!requireEsmDisabled && !forceAsyncTier` branch; the async loader-worker `else` branch and
  `preload-async-hooks.mjs` still have no equivalent.
- `crates/nub-cli/src/cli.rs:3281``resolve_bare_preload` still anchors at nub's own process
  cwd, which is not the child's cwd under `nub -r run` / `nub exec -r`.
- `crates/nub-cli/src/cli.rs:3188-3192` — one chainer path per project+channel, contents now
  cwd-dependent.
- `crates/nub-cli/src/cli.rs:3186``create_dir_all(...)?` still aborts every augmented command
  in an unwritable project, and a global-only `preload` still writes under
  `~/.config/nub/node_modules/.nub/`.

## Required outcome
- No new outcome; this section exists only to confirm the four threads remain open rather than
  having been silently addressed by the new commit.

ℹ️ Nitpicks

  • The new pub fn file_url_for (crates/nub-core/src/node/spawn.rs:2822) was inserted between to_file_url's doc block and to_file_url itself, so the long "Convert a filesystem path to a file:// URL…" explanation now documents the wrapper — trailing into "Public wrapper over [to_file_url]" — and to_file_url (2826) is left undocumented. Landed in 242bddcb, but this commit adds the second caller, so the wrapper is now the doc a reader lands on. Moving the mechanics back onto to_file_url and leaving the wrapper with its own two lines restores both.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

Both failed on Windows CI for reasons in the assertions, not the product
-- the same run had the other 11 tests in the file green.

The token check compared against `runtime/preload.` while Windows emits
`runtime\preload.cjs`; it now normalizes separators before comparing.

The node-options ordering check used find(), the FIRST occurrence, but a
nested nub run can carry more than one augmentation block in
NODE_OPTIONS. The token Node actually applies is the last of each, so it
now uses rfind().

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

This commit normalizes one of the two runtime/preload assertions the new tests added. The sibling one, 90 lines up in the npmrc test, still compares the raw string — so the Windows test leg stays red for exactly the reason this commit set out to fix.

Reviewed changes — only 5b9224fa, the single commit pushed since the previous review at a78b1079. It is test-only, both hunks in crates/nub-cli/tests/project_runtime_config.rs, so no product behavior moved and the four findings from the earlier review are untouched.

  • Located the two --max-old-space-size offsets with rfind in the npmrc-vs-nub.jsonc precedence assertion, on the grounds that a nested run can carry more than one augmentation block and Node applies the last token of each.
  • Compared nub's own preload token on a separator-normalized copy of NODE_OPTIONS in preload_entries_collapse_to_one_token_per_flag_name, since the fast-tier token is a raw path (--require=…\runtime\preload.cjs on Windows).

I checked that the rfind swap does not turn the precedence assertion into theatre. In the shape this test actually produces, each of 8192 and 4096 occurs once, so find and rfind return the same index and a reversed emit order still fails the from_npmrc < from_nub_jsonc check. The normalization is likewise sound rather than a workaround: find_preload returns a backslashed path on Windows (crates/nub-core/src/node/spawn.rs:3162 and :3178) and node_options_token does not translate separators, so the raw string genuinely cannot contain runtime/preload there.

ℹ️ Nitpicks

  • options.replace('\\', "/") is the right normalization only while the preload path has no whitespace. node_options_token (crates/nub-core/src/node/spawn.rs:2891) quotes a whitespace-bearing value and doubles every backslash inside it — the case its own doc calls the load-bearing Windows one (C:\Users\John Doe\…) — so the normalized copy would read runtime//preload. and the assertion would fail on a spacey checkout. Collapsing runs (.replace("\\\\", "/") first, or a /// pass) covers both sites.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

"a multi-flag value must split into separate tokens, not one quoted blob: {options}"
);
assert!(
options.contains("runtime/preload"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assertion has the same platform dependence the commit just fixed at line 1163, and this file is deliberately not #![cfg(unix)] — on the fast tier nub's own token is --require=<path>\runtime\preload.cjs, so the raw string never contains runtime/preload on Windows. Normalize here too.

Suggested change
options.contains("runtime/preload"),
options.replace('\\', "/").contains("runtime/preload"),

@colinhacks
colinhacks merged commit cc927db into main Aug 5, 2026
66 checks passed
@colinhacks
colinhacks deleted the preload-token-fixes branch August 5, 2026 02:18
colinhacks added a commit that referenced this pull request Aug 5, 2026
Merging main brought in #673, which reaches the user's preload chain through a
specifier preload-common.cjs builds at run time. Compile bundles that preload
into every artifact, so the unresolvable-import gate started firing on nub's own
machinery: 33 tests failed and every build failed, including a one-line app.js.

The gate speaks to the author about the author's code — make the specifier
static, or pass --allow-dynamic-import — and nobody compiling an app can act on
nub's internals. The site is also unreachable in an artifact: the chain comes
from __NUB_RUNTIME_CONFIG, which a compiled launcher never sets, so
importUserPreloadChain returns at its guard.

So the sites are dropped before the gate, and before the hook count, which
exists for the author's imports. Verified by compiling and running a fixture,
not only by the suite.
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants