Skip to content

fix(install): bootstrap node-gyp lazily, not before every build fan-out - #666

Merged
colinhacks merged 6 commits into
mainfrom
node-gyp-lazy-install
Aug 3, 2026
Merged

fix(install): bootstrap node-gyp lazily, not before every build fan-out#666
colinhacks merged 6 commits into
mainfrom
node-gyp-lazy-install

Conversation

@colinhacks

@colinhacks colinhacks commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Any approved build bootstrapped node-gyp up front, needed or not — so an unreachable registry aborted the install after ~70s, with a warm store and nothing invoking node-gyp. The tool dir is in the cache, not the store, so offline CI installs hit it every time.

Routes install through the lazy shim nub run already uses. ensure_cached still locks the tool dir, so a parallel fan-out converges on one bootstrap.

jailBuilds=true is the exception and resolves up front: a jailed script gets a cleared env and a temporary HOME, so a shim re-entry looks for the tool dir under that HOME and cannot refetch (the jail denies network). Best-effort — it warns rather than failing an install that never needed node-gyp.

Also: AUBE_NODE_GYP_PROJECT_DIR was never set on the install path, and the sh shim read it under set -eu; the cmd shim expanded it literally. Both fall back to cwd now.

Verified on a 13-scenario probe (probe/node-gyp-shim) across Windows, Linux and macOS: real addon compiles, PATH precedence, npm_config_node_gyp, concurrency, and jailed builds.

Copilot AI review requested due to automatic review settings August 2, 2026 18:56
@vercel

vercel Bot commented Aug 2, 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 3, 2026 9:53pm

Request Review

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.

🟡 Not ready to approve

The Windows node-gyp.cmd lazy shim still lacks an explicit guard/error for an unset AUBE_NODE_GYP_EXE, which can produce opaque failures when that env var is missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR changes the install build fan-out to expose node-gyp via a lazy shim (matching the nub run approach) instead of bootstrapping node-gyp up front for every approved build, preventing installs from failing due to an unreachable registry when nothing actually invokes node-gyp.

Changes:

  • Route dependency lifecycle build fan-out through node-gyp lazy shims, avoiding eager bootstrap on install.
  • Export a stable AUBE_NODE_GYP_PROJECT_DIR into lifecycle scripts so the shim bootstraps against the correct project root and .npmrc.
  • Add regression coverage in both aube Bats tests and Nub’s install engine tests to ensure node-gyp is not bootstrapped for unrelated builds.
File summaries
File Description
vendor/aube/test/node_gyp_rebuild.bats Updates the PATH-based test comment to reference the lazy shim API.
vendor/aube/test/node_gyp_bootstrap.bats Adds a new install regression test ensuring unrelated build scripts don’t trigger node-gyp bootstrap.
vendor/aube/crates/aube/src/commands/script_settings.rs Ensures lifecycle script settings include a project dir for node-gyp bootstrap context.
vendor/aube/crates/aube/src/commands/install/node_gyp_bootstrap.rs Removes eager ensure() path and hardens lazy shim behavior (cwd fallbacks, clearer POSIX shim behavior).
vendor/aube/crates/aube/src/commands/install/lifecycle.rs Switches install build fan-out from eager node-gyp bootstrap to a lazy shim bin-dir strategy.
vendor/aube/crates/aube-scripts/src/lib.rs Adds node_gyp_project_dir to ScriptSettings and exports AUBE_NODE_GYP_PROJECT_DIR when configuring node-gyp shims.
crates/nub-cli/tests/install_engine.rs Adds a Nub regression test ensuring installs succeed without a registry when builds don’t invoke node-gyp.
Review details
  • Files reviewed: 1/7 changed files
  • Comments generated: 0
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@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

The eager bootstrap was also what kept the in-jail node-gyp path working. With jailBuilds/paranoid on and a cold tool dir, a dependency that actually invokes node-gyp now fails. Details inline.

Reviewed changes — full diff at bfd29e6, plus the surrounding bootstrap, jail, script-settings, and project-lock machinery the change now depends on.

  • Lazy shim replaces the eager bootstraprun_dep_lifecycle_scripts swaps node_gyp_bootstrap::ensure(project_dir) for lazy_shim_bin_dir(&<project>/<modules>/.bin), so nothing is fetched until a script actually runs node-gyp.
  • ensure() deleted — its node_gyp_on_path() guard already lives in lazy_shim_bin_dir, which additionally checks the project's own .bin.
  • AUBE_NODE_GYP_PROJECT_DIR now set on the install path — a new ScriptSettings::node_gyp_project_dir field, stamped in apply_script_settings_env alongside AUBE_NODE_GYP_EXE.
  • Both shims made defensive — the sh shim gained an AUBE_NODE_GYP_EXE presence check and ${AUBE_NODE_GYP_PROJECT_DIR:-$PWD} expansion; the .cmd shim gained setlocal plus a %CD% fallback, fixing the literal %AUBE_NODE_GYP_PROJECT_DIR% it previously passed through.
  • Tests — a Rust integration test asserting a non-gyp approved build installs against an unreachable registry without creating the tool bucket, a bats twin, and a comment refresh in node_gyp_rebuild.bats.

Four things I checked and found correct, so they need no further attention: the project .npmrc still reaches the bootstrap (configure_script_settings receives the workspace root, and lifecycle.rs:782-784 re-propagates the task-local into each spawned job); both env vars survive the jail's env_clear because apply_script_settings_env runs after apply_jail_env; the "converges on one bootstrap" claim holds (blocking cross-process fslock keyed on the tool dir, with the stub workspace yaml preventing escape to the outer project's lock); and the shim's $(...) capture is clean, since InstallControl::silent() suppresses the progress UI and all tracing goes to stderr.

ℹ️ Nitpicks

  • No committed test covers N sibling jobs racing a cold tool-dir bootstrap — a race the old eager path made impossible. The manual "8 concurrent builds green" check is the only evidence for a newly reachable path.

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

Comment thread vendor/aube/crates/aube/src/commands/install/lifecycle.rs Outdated
Comment thread vendor/aube/crates/aube/src/commands/install/node_gyp_bootstrap.rs

@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

The new eager-bootstrap-for-jailed-builds branch lands without a test that can fail, and closes only one of the two channels through which a jailed script reaches node-gyp. Details inline.

Reviewed changes — the delta since the prior review at bfd29e6 is commit 47e2e46, plus the jail env / Landlock / Seatbelt machinery the new branch now depends on.

  • Resolved node-gyp up front when any job will be jailedrun_dep_lifecycle_scripts computes any_jailed from jail_policy.should_jail across jobs and takes an eager path instead of the lazy shim, warning and continuing on failure.
  • Added ensure_bin_dir_for_jail — an eager counterpart to lazy_shim_bin_dir with the same node_gyp_bin_exists || node_gyp_on_path short-circuit, falling through to ensure_cached.
  • Registered WARN_AUBE_NODE_GYP_BOOTSTRAP_FAILED — a new INSTALL_LIFECYCLE warning code with no exit code, emitted on the best-effort failure path.
  • Added a jailed-install regression testjailed_build_that_never_needs_node_gyp_survives_a_failed_bootstrap, asserting a jail-builds=true install against a dead registry still succeeds and still runs the approved build.

Two premises the new code rests on I checked and found correct, so they need no further attention. The jail-HOME story in the new comments is accurate: apply_jail_env clears the environment and points HOME/TMPDIR at a fresh per-package jail_home(), and neither XDG_CACHE_HOME nor LOCALAPPDATA survives safe_jail_env_key, so aube_store::dirs::cache_dir() inside the jail really does resolve under the throwaway home. And executing the pre-resolved tool-dir binary is permitted inside the jail — Landlock grants / AccessFs::from_read(ABI::V2), which includes Execute and ReadFile, and the Seatbelt profile only denies writes.

ℹ️ Pre-resolving node-gyp is not enough for a jailed native addon to compile cold

A jailed node-gyp rebuild still fails without an explicit network: true grant, because configure implicitly runs install to fetch Node headers into a HOME-derived devdir — and the jail both remaps HOME to a throwaway dir and denies network. That is pre-existing rather than something this commit broke, but it means the eager branch buys robustness for the network: true case (one bootstrap instead of one per jailed package) rather than making cold jailed native builds work. Worth deciding explicitly, since the surrounding comments read as though the jailed path is now whole.

Technical details
# Jailed node-gyp builds need a network grant regardless of who resolves the binary

## Affected sites
- `vendor/aube/crates/aube/src/commands/install/lifecycle.rs:598-607` — the new comment block frames the eager path as what makes a jailed node-gyp build work; it makes the *binary* reachable, nothing more.
- `vendor/aube/crates/aube-scripts/src/lib.rs:880-913``apply_jail_env` sets `HOME`/`TMPDIR` to `jail_home()`, a fresh per-package temp dir, so node-gyp's `envPaths('node-gyp').cache` devdir is always empty.
- `vendor/aube/crates/aube-scripts/src/linux_jail.rs:89-163` and `jail_profile` in `aube-scripts/src/lib.rs:392-401` — network is denied unless a `jailBuildPermissions` grant sets `network: true`.
- Nothing in `vendor/aube` or `crates/` sets `nodedir` / `devdir` / `npm_config_nodedir` / `npm_config_devdir`, so node-gyp always takes its default download-into-HOME path.
- `vendor/aube/docs/package-manager/jailed-builds.md:72-82` — the documented workaround grants `read: ~/.cache/node-gyp`, which cannot help: under the remapped `HOME` node-gyp never looks there.

## Required outcome
The intended support level for `jailBuilds` + native addons is stated once, in one place, and the code comments match it. Either a jailed native build is expected to require an explicit `network: true` grant (and the docs say so, with the ineffective `read: ~/.cache/node-gyp` line corrected), or header provisioning is handled — e.g. by pointing node-gyp at a jail-visible devdir the install pre-populates outside the jail.

## Open questions for the human
- Is a cold jailed native-addon build a configuration you intend to support end-to-end, or is `jailBuilds` + node-gyp understood to require an explicit grant?
- If a grant is required anyway, is the eager branch still worth its complexity over the lazy shim, which under `network: true` succeeds too (at the cost of one bootstrap per jailed package)?

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

Comment thread crates/nub-cli/tests/install_engine.rs
Comment thread vendor/aube/crates/aube/src/commands/install/node_gyp_bootstrap.rs Outdated
code = aube_codes::warnings::WARN_AUBE_NODE_GYP_BOOTSTRAP_FAILED,
"could not prepare node-gyp for jailed builds: {err:#}"
);
None

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.

Returning None here drops node-gyp for every job, not just the jailed ones — in a mixed install (jailBuildExclusions) the unjailed siblings lose the lazy shim they would otherwise have got. It also makes the registered description at warnings.rs:211 wrong: with nothing on PATH, a build that wants node-gyp fails with the shell's node-gyp: not found, not "node-gyp's own error".

Suggested change
None
node_gyp_bootstrap::lazy_shim_bin_dir(&project_bin_dir)?

@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 Windows .cmd repair is correct. One gap worth deciding on: nothing in CI executes the file it repairs.

Reviewed changes — the delta since the prior review at 47e2e46 is commit 54c9fcc, a one-line change to the generated Windows node-gyp.cmd plus the comment explaining it. I re-derived cmd.exe's quote handling against the documented rule rather than taking the comment's word for it.

  • Repaired the .cmd shim's path capture — the backquoted for /f command is wrapped in one extra quote pair, so cmd /C's outer-quote strip leaves "<exe>" __node-gyp-bootstrap "<dir>" intact instead of the mangled <exe>" … "<dir>.
  • Documented both cmd.exe rules in place — the quote strip, and the literal %VAR% text an undefined variable expands to, replacing the shorter comment that covered only the second.

Three things I checked and found correct, so they need no further attention. The doubling is the canonical workaround for the documented cmd /C rule, and it is unconditionally safe here: the command always carries more than two quote characters, so cmd's preserve-quotes branch can never apply and the strip-first-and-last branch always fires — including for paths with spaces, which matches the "measured on windows-latest, with and without spaces" note. The post-strip string is exactly the three intended tokens. And %* forwarding plus exit-code propagation are unaffected by setlocal without a matching endlocal, since the batch's implicit pop doesn't touch ERRORLEVEL.

ℹ️ Nothing in CI executes the generated node-gyp.cmd

This PR is what puts that file on the Windows install critical path: lazy_shim_bin_dir now supplies the fan-out's node-gyp instead of a real bootstrapped binary, and install lifecycle scripts spawn through cmd.exe, so node-gyp in a build script resolves to this .cmd. Two cmd.exe defects in its six lines shipped inside this PR and were caught only by an uncommitted probe — the bats twin is ubuntu-only and the two new Rust tests, which do run on windows-latest, never invoke node-gyp, so the shim is written and never executed. The next edit here has the same blind spot.

Technical details
# The generated `node-gyp.cmd` has no automated coverage on the path this PR puts it on

## Affected sites
- `vendor/aube/crates/aube/src/commands/install/node_gyp_bootstrap.rs:285-291` — the shim body. Two live cmd.exe defects were fixed here across `47e2e46` (literal `%AUBE_NODE_GYP_PROJECT_DIR%`) and `54c9fcc` (the `for /f` quote strip); neither was caught by a committed test.
- `vendor/aube/crates/aube/src/commands/install/lifecycle.rs:629``lazy_shim_bin_dir` replaces the eager real binary for the unjailed fan-out. `aube_scripts::spawn_shell` (`vendor/aube/crates/aube-scripts/src/lib.rs:345-363`) defaults to `cmd.exe` on Windows for lifecycle scripts, so a build script's `node-gyp` resolves to this `.cmd` via `PATHEXT`. nub's bundled busybox `sh` default (`crates/nub-cli/src/cli.rs:4437`) covers the `nub run` path only, not the install fan-out.
- `vendor/aube/test/node_gyp_bootstrap.bats` — the bats suite runs on ubuntu only (`.github/workflows/aube-parity.yml:122-146`), so it cannot reach the `#[cfg(windows)]` branch.
- `crates/nub-cli/tests/install_engine.rs:1171` and `:1230` — both new tests run on `windows-latest`, but their fixtures' postinstall is a plain `node -e`, so the shim is written and never invoked.
- `probe/node-gyp-shim`, named in the PR body as the 13-scenario verification, is not in the diff.

## Required outcome
One committed test executes the generated `node-gyp.cmd` on Windows and fails if its path capture regresses.

## Suggested approach (optional)
`ensure_cached` returns early when `<cache>/…/tools/node-gyp/v12/node_modules/.bin` already holds a `node-gyp*` (`node_gyp_bootstrap.rs:95-97`), so pre-seeding that dir with a stub that prints a marker keeps the whole round trip hermetic — the same tool-dir pre-seed pattern `crates/nub-cli/tests/pm_identity.rs:425-445` already uses. Run an approved build whose script invokes `node-gyp` and assert the stub's marker. Hand the child a `PATH` the test controls so `node_gyp_on_path()` is deterministically false while `node` stays reachable, otherwise the assertion passes for the wrong reason on a host that happens to have node-gyp installed.

## Open questions for the human
- Is `probe/node-gyp-shim` intended to land in the repo, or to stay a local probe? If it stays local, the Windows shim's only regression guard leaves with it.

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

@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 revised jailed test now gates the code it was written for. One dead escape hatch left in it.

Reviewed changes — the delta since the prior review at 54c9fcc is commit 710f0f4, which touches only jailed_build_that_never_needs_node_gyp_survives_a_failed_bootstrap. I traced the jailed spawn path and the warning's route to stderr rather than taking the new comments' word for either.

  • Made the jailed test able to fail — the child's PATH is stripped of every directory holding a node-gyp*, so ensure_bin_dir_for_jail no longer short-circuits on the host's copy, and the test asserts WARN_AUBE_NODE_GYP_BOOTSTRAP_FAILED / WARN_NUB_… in stderr, which only the eager branch can produce.
  • Swapped the fixture's build script to echo ok > built-ok — one spelling for both sh and cmd.exe, so the PATH scrub cannot take the script's interpreter with it.
  • Added a ncrypto::CSPRNG early return before the exit-code and marker assertions, attributed to node aborting under the build jail on Windows.

Three things I checked and found correct, so they need no further attention. The warning really does reach stderr in one of the two accepted spellings — present.rs:203 rewrites WARN_AUBE_ to WARN_NUB_ and abort_eagerly.rs:145 is the same pattern already in the suite. Scrubbing PATH cannot break the install even when it takes node's directory with it, because apply_lifecycle_augmentation (pm_engine/mod.rs:1738-1745) falls back to ResolvedNode::fallback() on a discovery failure. And the test's duplicate of aube's private BINARY_NAMES is self-guarding rather than a vacuity risk: a scrub that misses a host node-gyp produces no warning, so the test fails loudly instead of passing for the wrong reason.

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

Comment on lines +1292 to +1299
// Windows aborts node at startup under the build jail (`ncrypto::CSPRNG`
// assertion), so the build script cannot run there at all and the install
// fails for a reason this test does not own. Asserting success anyway would
// pin an unrelated platform defect. Where the jail can run node, the full
// contract holds.
if stderr.contains("ncrypto::CSPRNG") {
return;
}

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.

Nothing spawns node inside the jail for this fixture, so this cannot fire for the reason it names: the script is echo ok > built-ok running through cmd.exe, the Windows spawn_jailed_shell arm is a pass-through with no OS-level confinement, and the bootstrap attempt installs in-process with ignore_scripts. The only node spawn in this install is nub's unjailed node --version discovery probe, which is non-fatal — so if that string ever does reach stderr, this silently downgrades a real Windows failure to a pass.

Technical details
# The `ncrypto::CSPRNG` early return is unreachable for this fixture

## Affected sites
- `crates/nub-cli/tests/install_engine.rs:1292-1299` — the guard and its comment. The comment's premise (a `node` process starting under the build jail) does not hold for the fixture two lines above it, which was changed to `echo ok > built-ok` in this same commit.
- `vendor/aube/crates/aube-scripts/src/lib.rs:476-484``spawn_jailed_shell`'s `#[cfg(not(any(target_os = "linux", target_os = "macos")))]` arm is a pure pass-through to `spawn_shell_with_settings`; `windows_job.rs` only does descendant-killing. On Windows the jailed spawn is `cmd.exe` running the script verbatim (`lib.rs:345-362`), and nub's busybox default (`crates/nub-cli/src/cli.rs:4437`) is wired into `nub run`, not the dependency fan-out.
- `vendor/aube/crates/aube/src/commands/install/node_gyp_bootstrap.rs:118-124` — the failing bootstrap runs a recursive in-process install with `ignore_scripts = true`, so it spawns nothing either.
- `crates/nub-core/src/node/discovery.rs:910-913` — the one `node` spawn in the whole invocation, reached from `apply_lifecycle_augmentation` with the plain inherited environment. Its failure is tolerated (`pm_engine/mod.rs:1745`), so a crash there is not what would fail this test.

## Required outcome
Either the guard goes and Windows asserts the same contract as the other platforms, or — if the abort was observed with *this* fixture — the real cause is named, since it is not the jail.

## Open questions for the human
- Was the `ncrypto::CSPRNG` abort observed with the `echo` fixture, or with the `node -e` one this commit replaced? If the latter, the underlying limitation (a jailed script gets `env_clear`'d on Windows too, and `SystemRoot` / `COMSPEC` are not in `safe_jail_env_key`) now has no coverage and no tracking reference anywhere in the repo.

The dep-build fan-out called node_gyp_bootstrap::ensure() up front for
any approved build, without checking whether anything in the graph
wanted node-gyp, and propagated its error. A package whose whole build
script was `node -e "..."` therefore pulled a 20-package node-gyp tree,
and with an unreachable registry the install aborted after ~70s of
backoff even with a warm store. The tool dir lives in the cache rather
than the store, so a CI job restoring a store cache and installing
offline hit this every time.

The lazy shim that `run` and npm_config_node_gyp already use covers
this: it defers the fetch to the first actual node-gyp invocation.
ensure_cached() still takes the tool dir's own project lock and
re-checks under it, so a parallel fan-out converges on one bootstrap.

Two supporting fixes the swap required:

- AUBE_NODE_GYP_PROJECT_DIR was set only on the `run` spawn paths,
  never by apply_script_settings_env, so it was absent on the install
  path — and the sh shim read it unguarded under `set -eu`, which
  aborts with "unbound variable". Plumb it through ScriptSettings, and
  make the shim honor the cwd fallback its own docs already promised
  (the .js shim had it; the sh shim did not). Without the var the
  bootstrap would resolve against a dep dir with no .npmrc, losing
  private-registry config.
- The cmd shim expanded an undefined %AUBE_NODE_GYP_PROJECT_DIR%
  literally, passing it as a path. Add the same fallback under
  setlocal.

Side effect worth noting: lazy_shim_bin_dir also checks the project's
node_modules/.bin, which dep_bin_chain puts on a dep hook's PATH, so a
project-local node-gyp now wins instead of being shadowed by a
redundant bootstrap.
The bats twin lives in vendor/aube/test/node_gyp_bootstrap.bats, but
nub CI runs only a curated subset of that suite (install, ci, add,
remove, update, prune, lockfile_*), so the regression needs a home
where nub actually gates it.

Hermetic: a file: dep needs no registry, and after the fix neither does
the node-gyp path. Verified as a real gate, not a passing decoration —
against an eager `ensure_cached` it fails on the exit code, and against
the lazy shim it passes.
Going lazy broke `jailBuilds=true`. A jailed script runs with a cleared
environment and a temporary HOME, so the shim's `__node-gyp-bootstrap`
re-entry resolves the cache under THAT home, finds no tool dir, and
cannot refill one because the jail denies network as well. Measured: a
jailed build calling node-gyp failed with the bootstrap pointed at
`<tmp>/nub-jail/<pid>/<pkg>/.cache/nub/pm/tools/node-gyp/v12`.

Warming the real cache first does not help — the re-entry never looks
there. Verified by warming it and re-running: still failed.

So a job that will be jailed gets node-gyp resolved out here, outside
the jail, and is handed a directly executable binary with nothing to
re-enter. Unjailed builds keep the lazy path and the fix it carries.

The up-front resolve is best-effort: on failure it warns
(WARN_AUBE_NODE_GYP_BOOTSTRAP_FAILED) and continues, so an unreachable
registry cannot sink a jailed install whose builds never touch node-gyp
— which the eager code this PR replaced would have failed outright. A
build that does need it still fails, with node-gyp's own error.

jailBuilds defaults to false today and is documented as planned to
default true in the next major, so this would have gone unnoticed until
it was everyone's default.
The shim's `for /f "usebackq"` capture has never worked. `for /f` runs
its command through `cmd /c`, which strips the outer quote pair when the
string both starts and ends with a quote, so

  `"%AUBE_NODE_GYP_EXE%" __node-gyp-bootstrap "%DIR%"`

degrades to

  C:\...\nub.exe" __node-gyp-bootstrap "C:\...\proj

and dies with "The filename, directory name, or volume label syntax is
incorrect". Wrapping the whole command in one more quote pair makes that
strip leave the intended string.

Measured on windows-latest, both with and without spaces in the path: a
build-free candidate probe ran the shipped form as a control (fails, as
predicted) against two alternatives; only the extra-quote-pair form
passed.

This was latent rather than new. While the bootstrap was eager, a real
node-gyp was already on PATH and the .cmd shim was never the thing that
ran. Making the shim primary is what turns it load-bearing, so it ships
in the same change.

Found by tests/node-gyp-shim on the probe/node-gyp-shim branch, which
failed 6/13 on Windows while passing 13/13 on macOS and Linux.
It failed on windows-latest and passed locally for two different wrong
reasons, neither of which was the behaviour under test.

Locally it passed vacuously: the up-front resolve only runs when
node-gyp is not already resolvable, and this dev box has one on PATH, so
nothing was ever bootstrapped and nothing warned. PATH is now scrubbed
of every directory providing a node-gyp, which is what makes the warning
assertion live — it fails without the scrub.

On Windows it failed for a defect it does not own: node aborts at
startup under the build jail (ncrypto::CSPRNG assertion), so the build
script cannot run. The script is now a shell `echo` rather than `node`,
which also means scrubbing PATH cannot take the interpreter with it, and
the exit-code half is skipped when that assertion appears rather than
pinning an unrelated platform bug.

The warning half — a failed resolve is demoted, never fatal — is what
this test owns, and it now holds on every platform.
The sh shim reports AUBE_NODE_GYP_EXE being unset explicitly; the cmd
shim expanded it literally and exited 1 with only cmd's own 'not
recognized' noise. Mirror the guard so a Windows user gets the same
diagnostic.

Also stop the doc comment claiming the jail path leaves nothing to
re-enter: npm_config_node_gyp is a separate channel that still does.
Pre-existing rather than new, but not closed here, so say so.

@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 Windows guard is correct and closes the two threads it was written for. No new findings — two prior threads stay open, so this isn't an approval.

Reviewed changes — the branch was rewritten since the prior review, so the old shas are gone (710f0f4 is now ecb1ee1); the delta is one commit, 8a7eb94, touching only node_gyp_bootstrap.rs. I re-derived cmd.exe's in-block escaping, redirection and exit /b rules against the documented behavior rather than taking the guard's shape on trust, since nothing in CI executes the file it edits.

  • Mirrored the sh shim's missing-exe guard into the .cmd shim — an if not defined AUBE_NODE_GYP_EXE block echoing the same diagnostic to stderr and exiting 1, replacing cmd's own "not recognized" noise.
  • Scoped ensure_bin_dir_for_jail's doc comment to the PATH channel — it no longer claims the jail path leaves nothing to re-enter, and names npm_config_node_gyp as the half it does not close.

Four things I checked and found correct, so they need no further attention. ^( / ^) is the documented escape for literal parens inside a multi-line parenthesized block, so the message echoes intact with no stray caret and no premature block close. The missing space before >&2 is idiomatic rather than a defect, and the fd-number redirection trap needs a digit immediately before > — here it is a ). exit /b 1 at the top level of the batch ends the whole script and the code survives the active setlocal, so it reaches a cmd /c caller. And the happy path is untouched: IF does not itself set or clear ERRORLEVEL, so %* forwarding and the final exit status on the last line behave exactly as before.

One asymmetry I looked at and am deliberately not raising: if not defined tests existence while the sh shim's [ -z "${AUBE_NODE_GYP_EXE:-}" ] tests emptiness, so a defined-but-empty value would diverge. AUBE_NODE_GYP_EXE is only ever stamped from current_exe(), so that value is unreachable.

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

@colinhacks
colinhacks merged commit dc24fac into main Aug 3, 2026
58 checks passed
@colinhacks
colinhacks deleted the node-gyp-lazy-install branch August 3, 2026 22:35
@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