Repository navigation
Conversation
…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>
There was a problem hiding this comment.
ℹ️ 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.ymlexportsNODE_EXECUTABLE— when an explicitnode-version/node-version-fileresolves to a nub-managed Node, the OS-native binary path is appended to$GITHUB_ENVusing the sameNUBEOFheredoc form the file already uses forNODE_AUTH_TOKEN.- New
from_explicitflag — set only inside thepin_probebranch, so a failedmktemp -d(whereresolvedis 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 laternubis bound to the same binary. - Docs —
README.mdand 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-nubjob (pin20+node-version: 22) plusNODE_EXECUTABLE-unset assertions ineager-provisionandadditive.
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)") →nodeExecutableinnub.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.exesuffix 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 whileresolvedstays slash-normalized fordirnameand thesedversion 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_TEMPdoes not weaken it: cwd for that step is the checkout root holding the20pin, and setup-nub's own repo ships no rootpackage.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_EXECUTABLEas "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 laternubcall, replacing the single action-level::warning::ataction.yml:607that this PR removes. Worth a glance at theexplicit-binds-nubjob logs to confirm it isn't noisy — I could not settle it from the docs alone.
Claude Opus | 𝕏
There was a problem hiding this comment.
ℹ️ 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 explicitnode-version/node-version-filealso exportsNODE_EXECUTABLEso laternub/nub runuse that binary.action.yml:557-559— the comment above thepin_probebranch reworded from "authoritative for PATH" to "authoritative for PATH and later nub". Thefrom_explicit=trueassignment below it is unchanged..github/workflows/smoke.yml:173-175— explanatory comment on theexplicit-binds-nubfixture step, mirroring the one thepre-provisionjob already carries, explaining why the disagreeing.node-versionhas 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.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>
|
Addressed the review notes in 259ab75:
On the remaining questions: the PATH-vs- |
There was a problem hiding this comment.
ℹ️ 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-606adds anelif [ -n "$want" ]branch emitting::warning::node-version input (…) did not resolve to a nub-managed Node, while the no-input case keeps the plainechoat608. Exactly the split the previous review asked for, and the step'sif: inputs.provision-node != 'false'gate at471keeps it from firing on the opt-out path. - Dropped the redundant guard term —
action.yml:617is nowfrom_explicit && managed. Correct:managed=truerequires[ -n "$resolved" ]at582, andnode_executableis$resolvedcaptured pre-slash-conversion at576, so the two are non-empty together by construction. nub runis now exercised — the fixture atsmoke.yml:178-182writes a rootpackage.jsonwith averscript, andsmoke.yml:196-198assertsnub run verreports 22.x alongside the existingnub <file>assertion. This closes the widest open item from the first review.- README documents the
shim: trueinteraction —README.md:92states that later shimmedpnpm/npm/yarninheritNODE_EXECUTABLE.
Two things I checked rather than assumed on this delta:
- The new
nub runassertion can actually fail. This was the one that worried me: the script body is a barenode -p process.versions.node, and the action has already fronted Node 22 on the job PATH — so ifnub runsimply spawned a shell, the check would pass identically with the export deleted. It doesn't.https://nubjs.com/docs/runstates "thenubexecutable is aliased asnodefor the duration of anub runcall, so any subprocess that spawnsnode… gets Nub's augmentation too", which routes the barenodeback through nub's own pin chain rather than the ambient PATH. Delete theNODE_EXECUTABLEexport and nub falls through to the committed20pin, so line198goes red. Real coverage, not theatre. - The fixture
package.jsondoesn't quietly change the job. It declares noengines/devEngines, so.node-version20is still the only pin and all three assertions stay falsifiable; and it carries nopackageManager/devEnginesfield and no lockfile, so the cache auto-detect ataction.yml:289-297stays off exactly as before.
Claude Opus | 𝕏
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>
There was a problem hiding this comment.
ℹ️ The
shim: truecoverage 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: truecoverage gap —explicit-binds-nubnow passesshim: truealongsidenode-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 npmresolves under the shim dir, and the shimmednpm version --jsonreports Node 22.x rather than the committed20pin.
Two things I checked rather than assumed on this delta:
- The assertion mechanics are sound at the pinned npm. I read
lib/commands/version.jsat tagv10.9.2, the version the fixture pins.list()guards the package's own entry withif (pkg.name && pkg.version)— skipped here, since the fixture has noversion— then unconditionally copies everyprocess.versionskey into the result, so.nodeis present regardless of the missingversion,private: true, or the absent lockfile andnode_modules.lib/utils/display.jsroutes alllog/notice/warnoutput, update-notifier included, to stderr, and--jsonwrites only the serialized object to stdout, so nothing can break theJSON.parse. The pipeline at208will 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-638appends the shim dir to$GITHUB_PATHafter the provision step fronts the Node bin dir, specifically so the shim wins over Node's bundled npm — which is what makes205meaningful. The shim dir carries nonodeentry, sonode -pat208is still the provisioned 22. AndpackageManageris a package-manager pin, not a Node pin: the fixture still declares noengines/devEngines, so.node-version20remains 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-nubis now the first job to exercise pinned-package-manager dispatch through the shim on macOS and Windows —range-and-shimgates itspnpm@10.15.1check torunner.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 aNODE_EXECUTABLEtest red for an unrelated reason.
Claude Opus | 𝕏
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>
There was a problem hiding this comment.
✅ 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-nuboff the cache path —smoke.yml:189passespackage-manager-cache: false, with a two-line comment explaining that the fixture'spackageManagerwould 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
cacheinput, soaction.yml:271falls through and theelifat277matchespmc = false→enabled=false. Every cache step (cachepaths308,cachekey326,actions/cache372) is gated onsteps.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 shimstep (action.ymlsteps 6-7) readssteps.cachepaths.*orsteps.cacheflag.*, so all six assertions —NODE_EXECUTABLEset,nub <file>,nub run ver, PATHnode,command -v npm, shimmednpm version --json— run exactly as before against a cold store. A bare YAMLfalsereaches the composite as the string"false", the same waycache: trueatsmoke.yml:456already 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.
Claude Opus | 𝕏

node-version/node-version-filealready provisioned that Node and fronted it on PATH, but laternub/nub runstill followed the project's pin. A matrix job pinned to 24.21.0 that passednode-version: 26got PATH node 26 andnub runon 24.21.0; the only workaround was rewriting the tracked pin for that job.This exports
NODE_EXECUTABLEto the provisioned binary — nub's per-job override, which outranks pin files without rewriting them — so PATH and laternubagree.provision-node: falseand the no-input path leaveNODE_EXECUTABLEunset, so those jobs still follow the project pin. The pin-vs-PATH mismatch warning is gone because they no longer diverge.Smoke:
explicit-binds-nubpins 20, passesnode-version: 22, and asserts laternub(and PATH node) are 22.x on ubuntu, macOS, and Windows.eager-provisionandadditiveassertNODE_EXECUTABLEstays unset.Closes #12