build(bootstrap): use mise to manage Node.js and pnpm - #1109
Merged
Conversation
Contributor
|
marcusds
marked this pull request as ready for review
August 6, 2026 04:30
Contributor
📝 WalkthroughWalkthroughThe PR pins Node.js and pnpm in ChangesMise-managed Studio toolchain
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant mise
participant VerifyScript as verify-node-version.sh
participant pnpm
Developer->>Makefile: make bootstrap-studio
Makefile->>mise: verify and install pinned tools
Makefile->>VerifyScript: validate node and pnpm
VerifyScript-->>Makefile: return tool status
Makefile->>pnpm: install dependencies
Makefile->>pnpm: build Studio assets
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
mise.toml (1)
3-3: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse an exact pnpm version.
Line 3 uses
pnpm = "10", a fuzzy major selector.web/package.jsondeclarespnpm@10.34.5, somise installcan select another 10.x release and make the toolchain drift. Usepnpm = "10.34.5"if this PR promises a reproducible package manager. If the major-wide range is intentional, commit a mise lockfile and document that contract. Mise documents major-only selectors as fuzzy. (mise.jdx.dev)Proposed fix
node = "22.23.2" -pnpm = "10" +pnpm = "10.34.5"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mise.toml` at line 3, Pin the pnpm version in the mise configuration from the fuzzy major selector to the exact version declared by web/package.json, 10.34.5, so the package-manager toolchain remains reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 212-214: Update the mise installation block in the Makefile to
download a pinned installer or artifact, verify its signature or checksum, and
permit only HTTPS redirects before execution. Keep the existing MISE_VERSION
selection while replacing direct execution of the unverified curl pipeline.
In `@script/verify-node-version.sh`:
- Around line 10-14: Update the missing-tool message in the node/pnpm validation
loop to account for NMP_SKIP_MISE=1: when the bypass is enabled, report that the
script is using the PATH-based toolchain and provide generic PATH recovery
guidance instead of mise-specific instructions; retain the existing mise-managed
message for the default mode.
In `@web/README.md`:
- Line 20: Update the README statement about Make targets to say they use
mise-managed versions by default, and document the supported NMP_SKIP_MISE=1
bypass command without changing the surrounding setup guidance.
- Around line 7-20: Update web/README.md lines 7-20 to present the mise-managed
and manual Node.js/pnpm installation workflows as separate tabs. Update SETUP.md
lines 102-115 to present one-off execution, shell activation, and
NMP_SKIP_MISE=1 workflows as separate tabs, preserving each workflow’s existing
instructions.
- Around line 7-18: Update the setup instructions in web/README.md (lines 7-18),
SETUP.md (lines 100-113), and web/AGENTS.md (line 35) to add $HOME/.local/bin to
PATH before invoking mise, or invoke mise via its absolute path; ensure all
documented mise activate and mise exec commands work from a fresh shell.
---
Nitpick comments:
In `@mise.toml`:
- Line 3: Pin the pnpm version in the mise configuration from the fuzzy major
selector to the exact version declared by web/package.json, 10.34.5, so the
package-manager toolchain remains reproducible.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80019fb8-0431-42b3-bff6-13b95ccfed19
📒 Files selected for processing (7)
AGENTS.mdMakefileSETUP.mdmise.tomlscript/verify-node-version.shweb/AGENTS.mdweb/README.md
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Bootstrap previously assumed Node.js and pnpm were already on PATH. Replace the `verify-pnpm` / `verify-node-version` shell-check path with a `verify-mise` target that installs mise (if absent) and resolves `node@22` / `pnpm@10` from a new `mise.toml`. Subsequent bootstrap steps invoke node and pnpm via `mise exec --` so they pick up the mise-managed versions regardless of what the caller already has on PATH. `mise trust --all --silent` runs before `mise install --yes` so the target is non-interactive in CI and on fresh developer machines. Signed-off-by: mschwab <mschwab@nvidia.com>
…tation `make update-web-sdk` previously called `cd web && pnpm gen` and assumed pnpm was on PATH — same fresh-checkout gap this branch fixes for the Studio bootstrap path. Make it depend on `verify-mise` and run pnpm via `$(MISE_EXEC)`. Also drop the `setup-mise-shell` target that appended `mise activate` to the developer's shell rc. It's invasive for what's only needed when a dev wants direnv-style auto-activation; the bootstrap and pre-commit paths already route through `mise exec --`, and `mise` itself prints the activation hint after its own installer runs. Signed-off-by: mschwab <mschwab@nvidia.com>
…de-version The script now runs under `mise exec --`, so node and pnpm are always present. The corepack-enable fallback and the separate missing-binary branches can never fire. Replace them with one guard that points at `make verify-node-version` for the case where someone runs the script directly, outside the mise-managed PATH. Signed-off-by: mschwab <mschwab@nvidia.com>
`mise trust --all` walked ancestor directories as well as the repo, so it silently trusted any mise config above the checkout. It was also never needed: a mise.toml holding only [tools] is trust-exempt, and `mise install` runs clean without it. Pin the installer with MISE_VERSION so `make bootstrap` can't pull an arbitrary release off mise.run, and add NMP_SKIP_MISE=1 for bootstrapping against a toolchain that is already on PATH (offline installs, nix shells, distro packages). web/package.json engines are enforced either way. Also drop the stale `pnpm env use --global` advice from the bootstrap failure message and document the mise workflow in SETUP.md, including that direct `pnpm` use in web/ needs `mise exec --` or `mise activate`. Signed-off-by: mschwab <mschwab@nvidia.com>
…issing With NMP_SKIP_MISE=1 on a machine without pnpm, verify-pnpm failed with make's bare `Error 127`. The guidance in verify-node-version.sh never printed because verify-pnpm runs first. Catch the failure and say which of the two toolchain paths is in play. Signed-off-by: mschwab <mschwab@nvidia.com>
web/README.md still told developers to install Node.js 22 from nodejs.org and `npm install -g pnpm`. Replace that with the two supported options: let mise manage the versions pinned in mise.toml, or install node and pnpm yourself. Note that the make targets use the mise-managed versions either way, and fix the `pnpm dev` working directory to match web/AGENTS.md. Also drop the stale "upgrade Node on the VM" advice from the Cursor Cloud notes and point agents at mise from web/AGENTS.md. Signed-off-by: mschwab <mschwab@nvidia.com>
`node = "22"` resolved to whatever the latest 22.x was at install time, so two developers bootstrapping months apart could land on different patches. web/package.json constrains node on both ends (`>=22.23.2 <23`), so pin it to the same 22.23.2 that .nvmrc already names. pnpm stays fuzzy — engines only sets a floor for it. Signed-off-by: mschwab <mschwab@nvidia.com>
Review follow-ups: - Pin pnpm to 10.34 so the package manager can't drift across a minor while still picking up patch releases. node stays exact because web/package.json constrains it on both ends. - mise installs to ~/.local/bin, which isn't on PATH by default on macOS, so `mise activate` and `mise exec` fail on a fresh shell. Say so in web/README.md, SETUP.md, and web/AGENTS.md. - verify-node-version.sh told users to run `make verify-node-version` when a tool was missing, which is the target that just failed, and it claimed a mise-managed toolchain even under NMP_SKIP_MISE=1. Branch on the bypass and point at `make verify-mise` otherwise. - web/README.md said the make targets "always" use mise-managed versions, contradicting NMP_SKIP_MISE=1. Signed-off-by: mschwab <mschwab@nvidia.com>
marcusds
force-pushed
the
astd-148-mise-nodejs-pnpm-bootstrap/mschwab
branch
from
August 6, 2026 07:43
3e52511 to
c33e867
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 203-204: Quote the MISE executable path in the Makefile so paths
containing spaces remain a single shell word. Update both the MISE_EXEC command
construction and the direct “mise install --yes” invocation, preserving the
existing NMP_SKIP_MISE behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62dd138f-950e-424a-9221-b903e30972bb
📒 Files selected for processing (7)
AGENTS.mdMakefileSETUP.mdmise.tomlscript/verify-node-version.shweb/AGENTS.mdweb/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- mise.toml
- script/verify-node-version.sh
- web/AGENTS.md
- AGENTS.md
- web/README.md
- SETUP.md
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
MISE resolves to `command -v mise` or $HOME/.local/bin/mise, either of which can contain spaces. The existence guards already quoted it, but the two invocation sites did not, so a home directory with a space split the path into separate shell words and bootstrap failed with a misleading "No such file or directory". Signed-off-by: mschwab <mschwab@nvidia.com>
maxdubrinsky
approved these changes
Aug 6, 2026
8 tasks
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bootstrap previously assumed Node.js and pnpm were already on PATH. This routes both through mise so a fresh checkout works without nvm, corepack, or a system Node.
mise.tomlpinningnodeto 22.23.2 — the version.nvmrcalready names — andpnpmto10(resolves to 10.34.5).web/package.jsonconstrains node on both ends, while pnpm only has a floor.verify-misetarget: installs mise (if absent) at a pinnedMISE_VERSION, thenmise install --yes. Non-interactive, so it works in CI and on fresh developer machines without prompts.verify-pnpm,verify-node-version,bootstrap-studio, andupdate-web-sdkinvoke node/pnpm throughmise exec --, so they use the mise-managed versions regardless of what the caller has on PATH.script/verify-node-version.shstill owns the engine-strict check and its diagnostics; it just runs undermise exec --now. Its corepack-enable fallback and missing-binary branches are unreachable under mise and have been dropped.NMP_SKIP_MISE=1bootstraps against the toolchain already on PATH instead (airgapped installs, nix shells, distro packages). Engines are enforced either way.mise exec --/mise activatefor runningpnpmdirectly inweb/.Ported from NVIDIA-NeMo/Platform#530, adapted to this repo's
script/verify-node-version.shbootstrap path.Linear: ASTD-148
Test plan
Verified on a brand-new Ubuntu 26.04 LTS (aarch64) VM with no
node, nopnpm, and nomiseinstalled —gitandcurlwere preinstalled;makewas installed viaapt-get install makeas a prerequisite:git clonethis branch, thenmake bootstrap-studio— mise installs to~/.local/bin/mise, resolves node 22.23.2 and pnpm 10.34.5 frommise.toml, installs web deps and builds Studio assets. Exit 0, 20 MB of assets inweb/packages/studio/distincludingindex.html.make verify-node-versionalone — exit 0, engine-strict check passes againstweb/package.json.mise exec -- node --version→v22.23.2;mise exec -- pnpm --version→10.34.5.make verify-node-version NMP_SKIP_MISE=1on that VM (no pnpm anywhere) — fails with an actionable message naming both toolchain paths rather than a bareError 127.MISE_VERSION, fullbootstrap-studioexits 0.mise installsucceeds withoutmise trust, confirming a[tools]-onlymise.tomlis trust-exempt.Summary by CodeRabbit
New Features
Documentation