Skip to content

node-version binds later nub invocations; export NODE_EXECUTABLE - #13

Open
yamcodes wants to merge 5 commits into
nubjs:mainfrom
yamcodes:bind-explicit-node-version
Open

yamcodes wants to merge 5 commits into
nubjs:mainfrom
yamcodes:bind-explicit-node-version

Conversation

@yamcodes

@yamcodes yamcodes commented Sep 21, 2026 •

Copy link
Copy Markdown

node-version / node-version-file already provisioned that Node and fronted it on PATH, but later nub / nub run still followed the project's pin. A matrix job pinned to 24.21.0 that passed node-version: 26 got PATH node 26 and nub run on 24.21.0; the only workaround was rewriting the tracked pin for that job.

This exports NODE_EXECUTABLE to the provisioned binary — nub's per-job override, which outranks pin files without rewriting them — so PATH and later nub agree. provision-node: false and the no-input path leave NODE_EXECUTABLE unset, so those jobs still follow the project pin. The pin-vs-PATH mismatch warning is gone because they no longer diverge.

Smoke: explicit-binds-nub pins 20, passes node-version: 22, and asserts later nub (and PATH node) are 22.x on ubuntu, macOS, and Windows. eager-provision and additive assert NODE_EXECUTABLE stays unset.

Closes #12

yamcodes and others added 2 commits September 21, 2026 22:31
…TABLE.

PATH already fronted the requested Node, but nub still followed the project pin. Exporting NODE_EXECUTABLE makes nub / nub run use the same binary without rewriting pin files.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@yamcodes yamcodes changed the title Bind later nub invocations to an explicit node-version node-version binds later nub invocations; export NODE_EXECUTABLE Sep 21, 2026

@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.

ℹ️ No blockers — the mechanism checks out against nub's published contract. Two coverage/compat questions and one small guard note.

Reviewed changes — the full diff of all three files at 950bf21, plus independent verification of nub's NODE_EXECUTABLE contract against nub's published docs.

  • action.yml exports NODE_EXECUTABLE — when an explicit node-version / node-version-file resolves to a nub-managed Node, the OS-native binary path is appended to $GITHUB_ENV using the same NUBEOF heredoc form the file already uses for NODE_AUTH_TOKEN.
  • New from_explicit flag — set only inside the pin_probe branch, so a failed mktemp -d (where resolved is the project pin, not the requested version) correctly does not export.
  • Mismatch ::warning:: downgraded to a plain log line — the PATH-vs-pin divergence it described no longer exists once later nub is bound to the same binary.
  • Docs — README.md and both input descriptions now say an explicit version is authoritative for the rest of the job; the "remaining nuance vs setup-node" paragraph is removed. No stale references to the old warning remain.
  • Smoke — new explicit-binds-nub job (pin 20 + node-version: 22) plus NODE_EXECUTABLE-unset assertions in eager-provision and additive.

Two things I checked rather than assumed, both of which came back clean:

  • The contract is real and exactly as assumed. nub's docs list pin sources highest-first as NODE_EXECUTABLE ("an absolute path to a Node binary (a hard override)") → nodeExecutable in nub.jsonc → devEngines.runtime → .node-version → .nvmrc → .tool-versions → engines.node → PATH. Variable name, value shape, and precedence all match what lines 611-617 assume, and the .exe suffix is explicitly tolerated. The capture at line 575 sitting between the CR strip and the slash conversion is the right spot — the export keeps the OS-native path while resolved stays slash-normalized for dirname and the sed version extraction.
  • The new smoke assertion can actually fail. nub discovery "walks up from your working directory", not from the script's directory, so putting the fixture script in $RUNNER_TEMP does not weaken it: cwd for that step is the checkout root holding the 20 pin, and setup-nub's own repo ships no root package.json / .node-version / .nvmrc / .tool-versions / nub.jsonc, so the fixture is the only pin. Delete the export and line 188 goes red on every OS.

node-version-file is not separately exercised, but it provably shares the path — lines 496-504 only derive want, and every downstream branch keys on want alone. No extra job needed there.

ℹ️ The reported bug is about nub run, but only nub <script> is covered

Issue #12's repro is literally - run: nub run test, and both README.md:63 and the node-version input description promise later nub / nub run are bound. The smoke job only exercises nub <script> (smoke.yml:186). nub's docs describe pin-source resolution under the runtime/script path and never state that nub run consults the same chain, so the claim in the docs is currently one entry point wider than the evidence.

Technical details
# `nub run` is documented as bound but never exercised

## Affected sites
- `.github/workflows/smoke.yml:185-188` — asserts only `nub <script>` resolves 22.x
- `README.md:63` and `action.yml:19` — claim later `nub` / `nub run` are both bound
- Issue #12's repro uses `nub run test`, not a bare script invocation

## Required outcome
- The `explicit-binds-nub` job proves the bound version reaches `nub run` as well, or the
  docs narrow their claim to the entry points actually verified.

## Suggested approach (optional)
- Extend the existing fixture step to also write a `package.json` whose `scripts` entry prints
  `process.versions.node`, then assert `nub run <script>` reports 22.x alongside the existing
  `nub <file>` assertion. Keep it in the same job — it needs no new matrix.

ℹ️ Existing consumers who pass node-version get new behavior with no opt-out

setup-nub is a published @v0 action, and the PATH-vs-nub split this removes was previously documented behavior ("the remaining nuance vs setup-node"). A consumer who sets node-version to front a specific Node for bare node/npm steps while deliberately letting nub run follow the project pin will change behavior on upgrade with no signal — and provision-node: false is not an escape hatch, since it disables the eager provision and PATH front too. The change looks right as a default; the question is whether it needs a release note, and whether the no-escape-hatch shape is intended.

Technical details
# Behavior change for existing `node-version` consumers

## Affected sites
- `action.yml:611-617` — unconditional export whenever an explicit version resolves to a managed Node
- `README.md:63,67` — the paragraph documenting the old divergence is deleted, not deprecated
- `action.yml:633-654` — `$GITHUB_ENV` reaches later steps of the same composite action, so
  `nub pm shim` and every shimmed `npm`/`pnpm`/`yarn` in later steps also run under the explicit
  Node. Consistent with the new contract, but neither the diff comments nor the README say so.

## Open questions for the human
- Does this warrant a minor bump plus a changelog/release note, given `@v0` consumers track a
  floating major?
- Is "explicit `node-version` always binds nub" intended to be unconditional, or should there be a
  way to keep the old PATH-only semantics without also giving up the eager provision?
- Should `README.md` mention the `shim: true` interaction explicitly?

ℹ️ Nitpicks

  • nub's config docs say an overriding executable whose version "contradicts a pin the project also declares" causes nub itself to warn, and describe NODE_EXECUTABLE as "the per-shell form of the same setting". If that applies to the env form, the pin-20 + input-22 case this PR targets may now print a nub warning on every later nub call, replacing the single action-level ::warning:: at action.yml:607 that this PR removes. Worth a glance at the explicit-binds-nub job logs to confirm it isn't noisy — I could not settle it from the docs alone.

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

Comment thread action.yml Outdated

@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.

ℹ️ Delta is docs and comments only — no new findings. Flagging that the earlier review's points are still open.

Reviewed changes — only what landed in 9d24ac0 since my review of 950bf21. I diffed the executable regions and confirmed the shell logic is byte-identical to what I already reviewed; this commit carries no behavioral change.

  • README.md:59 — appends a sentence to the "Node on the global PATH" paragraph stating that an explicit node-version / node-version-file also exports NODE_EXECUTABLE so later nub / nub run use that binary.
  • action.yml:557-559 — the comment above the pin_probe branch reworded from "authoritative for PATH" to "authoritative for PATH and later nub". The from_explicit=true assignment below it is unchanged.
  • .github/workflows/smoke.yml:173-175 — explanatory comment on the explicit-binds-nub fixture step, mirroring the one the pre-provision job already carries, explaining why the disagreeing .node-version has to sit at the checkout root. Accurate, and consistent with nub resolving pins by walking up from the working directory.
  • PR title and body rewritten to lead with the behavior change rather than the mechanism.

All three doc edits are accurate and consistent with the code.

ℹ️ Prior review's findings are all still open

Nothing in 9d24ac0 addresses them, and one point got slightly larger: the nub run claim now appears in three places (README.md:59, README.md:63, action.yml:19) while the smoke job still exercises only nub <script> at smoke.yml:189. Widening the documented promise without widening coverage is the part worth a second look before merge.

Technical details
# Carry-over from review of 950bf21

## Still open
- `smoke.yml:189` — only `nub <script>` is asserted; issue #12's repro is `nub run test`, and
  `README.md:59`, `README.md:63`, `action.yml:19` all claim `nub run` is bound.
- `action.yml:612` (was 611 before this commit shifted it by one) — the `managed = true` term in
  the guard removes the only annotated signal for "explicit `node-version` requested but nub fell
  back to the host Node"; the redundant `[ -n "$node_executable" ]` term is still there.
- Consumer-visible behavior change for existing `node-version` users with no opt-out, on a
  published `@v0` action.
- Possible per-invocation nub warning when the bound version contradicts a declared pin.

## Required outcome
- Each point is either addressed or consciously declined — none of them blocks merge on its own.

## Note
- The inline thread from the previous review is anchored to the old line number and will read as
  one line off against this head.

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

… run.

The managed-only export guard left that failure silent, and smoke only exercised nub <file> while the docs promised nub run.

Co-authored-by: Cursor <cursoragent@cursor.com>
@yamcodes

Copy link
Copy Markdown
Author

Addressed the review notes in 259ab75:

  • Host-fallback silence — an explicit node-version that resolves to the host Node now emits ::warning::. The no-input host-fallback stays a plain log line. Dropped the redundant [ -n "$node_executable" ] term.
  • nub run coverage — explicit-binds-nub now writes a package.json scripts.ver and asserts nub run ver is 22.x alongside nub <file>.
  • shim: true — README notes that later shimmed npm/pnpm/yarn inherit NODE_EXECUTABLE.

On the remaining questions: the PATH-vs-nub split was the bug, so the bind is unconditional — no PATH-only opt-out besides provision-node: false. @v0 is a floating tag; a release note can ride the next cut. Whether nub itself warns on a pin contradiction is something to watch in this job's logs.

@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.

ℹ️ Both prior blockers are genuinely fixed. One newly-added doc sentence outruns its evidence — minor, inline.

Reviewed changes — only what landed in 259ab75 since the review of 9d24ac0, plus independent verification of nub's nub run and nub pm shim contracts against its published docs.

  • Restored the host-fallback diagnostic — action.yml:603-606 adds an elif [ -n "$want" ] branch emitting ::warning::node-version input (…) did not resolve to a nub-managed Node, while the no-input case keeps the plain echo at 608. Exactly the split the previous review asked for, and the step's if: inputs.provision-node != 'false' gate at 471 keeps it from firing on the opt-out path.
  • Dropped the redundant guard term — action.yml:617 is now from_explicit && managed. Correct: managed=true requires [ -n "$resolved" ] at 582, and node_executable is $resolved captured pre-slash-conversion at 576, so the two are non-empty together by construction.
  • nub run is now exercised — the fixture at smoke.yml:178-182 writes a root package.json with a ver script, and smoke.yml:196-198 asserts nub run ver reports 22.x alongside the existing nub <file> assertion. This closes the widest open item from the first review.
  • README documents the shim: true interaction — README.md:92 states that later shimmed pnpm/npm/yarn inherit NODE_EXECUTABLE.

Two things I checked rather than assumed on this delta:

  • The new nub run assertion can actually fail. This was the one that worried me: the script body is a bare node -p process.versions.node, and the action has already fronted Node 22 on the job PATH — so if nub run simply spawned a shell, the check would pass identically with the export deleted. It doesn't. https://nubjs.com/docs/run states "the nub executable is aliased as node for the duration of a nub run call, so any subprocess that spawns node … gets Nub's augmentation too", which routes the bare node back through nub's own pin chain rather than the ambient PATH. Delete the NODE_EXECUTABLE export and nub falls through to the committed 20 pin, so line 198 goes red. Real coverage, not theatre.
  • The fixture package.json doesn't quietly change the job. It declares no engines/devEngines, so .node-version 20 is still the only pin and all three assertions stay falsifiable; and it carries no packageManager/devEngines field and no lockfile, so the cache auto-detect at action.yml:289-297 stays off exactly as before.

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

Comment thread README.md
The README claimed shimmed npm/pnpm/yarn run on the requested Node; smoke only covered nub and nub run. Turning shim on in that job and asserting npm version --json.node is 22.x backs the sentence.

Co-authored-by: Cursor <cursoragent@cursor.com>

@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 shim: true coverage gap is properly closed. One side effect of the fixture change, and one note on what the new assertion does and doesn't settle.

Reviewed changes — only what landed in 54d3c03 since the review of 259ab75. This is a smoke-only delta: action.yml and README.md are untouched, so the mechanism I reviewed at 259ab75 is unchanged.

  • Closed the shim: true coverage gap — explicit-binds-nub now passes shim: true alongside node-version: "22" and adds "packageManager": "npm@10.9.2" to the fixture, so the job exercises the shim path the README claims is bound.
  • Three new assertions at smoke.yml:203-210 — command -v npm resolves under the shim dir, and the shimmed npm version --json reports Node 22.x rather than the committed 20 pin.

Two things I checked rather than assumed on this delta:

  • The assertion mechanics are sound at the pinned npm. I read lib/commands/version.js at tag v10.9.2, the version the fixture pins. list() guards the package's own entry with if (pkg.name && pkg.version) — skipped here, since the fixture has no version — then unconditionally copies every process.versions key into the result, so .node is present regardless of the missing version, private: true, or the absent lockfile and node_modules. lib/utils/display.js routes all log/notice/warn output, update-notifier included, to stderr, and --json writes only the serialized object to stdout, so nothing can break the JSON.parse. The pipeline at 208 will not produce a confusing red for an unrelated reason.
  • The shim assertion is falsifiable, and the PATH ordering that makes it so is deliberate. action.yml:633-638 appends the shim dir to $GITHUB_PATH after the provision step fronts the Node bin dir, specifically so the shim wins over Node's bundled npm — which is what makes 205 meaningful. The shim dir carries no node entry, so node -p at 208 is still the provisioned 22. And packageManager is a package-manager pin, not a Node pin: the fixture still declares no engines/devEngines, so .node-version 20 remains the only Node pin and every assertion in the job stays live.

ℹ️ README.md:92 now rests entirely on an assertion that hasn't run yet

The sentence added in 259ab75 says a shimmed pnpm/npm/yarn runs on the requested Node. nub's published docs still don't confirm that: /docs/pm/pm-shim says a shimmed PM "dispatches to it in-process under the project's Node", and no page ties that phrase back to the eight-step chain that puts NODE_EXECUTABLE on top. The same page's architectural description does point the right way — "The Nub shim is a hardlink to the native binary: the process that resolves the pin and dispatches is the one already running" implies nub's own override-aware resolver rather than a naive PATH-node exec — but that is inference, not a quote. So this one job is the only evidence, and the check-runs API currently returns just the pullfrog check for this head; smoke hasn't reported yet.

Technical details
# `README.md:92` is backed by exactly one unrun assertion

## Affected sites
- `README.md:92` — "a shimmed `pnpm`/`npm`/`yarn` runs on the requested Node"
- `.github/workflows/smoke.yml:208-210` — the only assertion that tests it
- Upstream docs (`https://nubjs.com/docs/pm/pm-shim`, `/docs/node`, `/docs/config`,
  `/docs/pm`) never state that `nub pm shim` consults `NODE_EXECUTABLE`

## Required outcome
- The `explicit node-version binds later nub` job is seen green on all three OSes before merge,
  or `README.md:92` is narrowed to what is actually known.

## Note for whoever reacts to a red run
- If line `210` fails, the fix is to soften `README.md:92` — not to touch `action.yml`. The
  binding itself is independently proven by `smoke.yml:196` (`nub <file>`) and `199`
  (`nub run ver`), neither of which depends on how the pm shim resolves its Node.

## Open questions for the human
- Does `nub pm shim`'s in-process dispatch resolve Node through the full chain, or specifically
  through the pin files? You may simply know; the docs do not say.

ℹ️ Nitpicks

  • explicit-binds-nub is now the first job to exercise pinned-package-manager dispatch through the shim on macOS and Windows — range-and-shim gates its pnpm@10.15.1 check to runner.os == 'Linux' (smoke.yml:431). 21b317c's commit message states that scoping as a fact without naming a defect behind it, so this is most likely just broader coverage. Worth knowing, though, that a shim-provisioning problem on those OSes would now turn a NODE_EXECUTABLE test red for an unrelated reason.

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

Comment thread .github/workflows/smoke.yml
packageManager on the fixture flipped cache auto-detect on; this job only needs to prove the NODE_EXECUTABLE bind.

Co-authored-by: Cursor <cursoragent@cursor.com>

@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.

✅ No new issues found.

Reviewed changes — only what landed in 2b5a075 since the review of 54d3c03. A one-input smoke-only delta; action.yml and README.md are untouched, so the mechanism reviewed at 259ab75 is unchanged.

  • Took explicit-binds-nub off the cache path — smoke.yml:189 passes package-manager-cache: false, with a two-line comment explaining that the fixture's packageManager would otherwise auto-enable caching. This closes the only open inline thread.

Two things I checked rather than assumed:

  • The knob actually disables caching here. The job sets no cache input, so action.yml:271 falls through and the elif at 277 matches pmc = false → enabled=false. Every cache step (cachepaths 308, cachekey 326, actions/cache 372) is gated on steps.cacheflag.outputs.enabled == 'true', so restore and save are both skipped.
  • No assertion in the job weakens. Nothing in the provision step or the nub pm shim step (action.yml steps 6-7) reads steps.cachepaths.* or steps.cacheflag.*, so all six assertions — NODE_EXECUTABLE set, nub <file>, nub run ver, PATH node, command -v npm, shimmed npm version --json — run exactly as before against a cold store. A bare YAML false reaches the composite as the string "false", the same way cache: true at smoke.yml:456 already relies on.

One status note carried from the previous run, not a new finding: GET /commits/2b5a075…/check-runs still returns only the pullfrog check, so the three-OS explicit node-version binds later nub job has not reported on any head of this branch yet. That job remains the sole evidence for README.md:92's shim: true claim, since nub's published docs never tie nub pm shim to NODE_EXECUTABLE. Worth seeing green before merge.

Pullfrog  | View workflow run | Using Claude Opus | 𝕏

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.

node-version binds PATH only; later nub follows the project pin

1 participant