fix(desktop): provision Node before snapshotting build env in cmd_gui - #55288
fix(desktop): provision Node before snapshotting build env in cmd_gui#55288jecanore wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Desktop fix for Node.js provisioning before snapshotting build env.
Looks Good
- Calls _ensure_tui_node() before with_hermes_node_path() snapshot
- Fixes the case where no managed Node exists and system Node is invisible to non-shell launch
- Well-documented root cause: launchd PATH omits version-manager Node
- Placement is correct: PATH repair before env snapshot
A desktop rebuild launched from the GUI (Finder/launchd) — including the installer's `hermes desktop --build-only` update step — inherits a stripped PATH that omits version-manager and Homebrew Node (~/.nvm, ~/.fnm, /opt/homebrew/bin). When no Hermes-managed Node exists (the common case after a terminal install accepts an nvm/brew Node), cmd_gui's `find_node_executable` falls through to `shutil.which` against that stripped PATH and the rebuild fails with "npm was not found on PATH", even though a terminal builds fine. cmd_gui already imports the node-bootstrap cascade the TUI self-heals with (`_ensure_tui_node`) but never called it. Call it before snapshotting the build env via `with_hermes_node_path()`, gated on a build actually running. Placement matters: the build subprocess runs with the env snapshot, and npm's `#!/usr/bin/env node` shebang fails (exit 127) if node's dir is absent from that env — so PATH must be repaired before the snapshot, not just before npm resolution. Refs NousResearch#49242. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7c01948 to
a304239
Compare
|
Rebased onto current Current verification:
Requesting maintainer review from @OutThisLife because this follows the macOS desktop self-update path addressed in #38296. |
|
Thanks for the focused regression fix. Current main still takes the build-environment snapshot in The update path is real: Automated hermes-sweeper review. |
What does this PR do?
Fixes a macOS desktop-update failure where the GUI updater rebuild aborts with
Desktop GUI requires Node.js/npm, but npm was not found on PATH, even though aterminal can build the app fine.
Root cause. A desktop rebuild launched from the GUI (Finder/launchd) —
including the installer's
hermes desktop --build-onlyupdate step — inheritsthe launchd-default PATH (
/usr/bin:/bin:/usr/sbin:/sbin), which omitsversion-manager and Homebrew Node (
~/.nvm,~/.fnm,/opt/homebrew/bin).When no Hermes-managed Node tree exists — the common case after a terminal
install, where
ensure_nodeaccepts the user's existing nvm/brew Node and neverprovisions
$HERMES_HOME/node—cmd_gui'sfind_node_executable("npm")fallsthrough to
shutil.which("npm")against that stripped PATH and finds nothing.#49254 made Hermes prefer managed Node when it exists; this is the sibling
case where no managed Node exists at all and the system Node is invisible to a
non-shell launch.
Fix.
cmd_guialready imports the node-bootstrap cascade the TUI self-healswith (
_ensure_tui_node(), which sourcesscripts/lib/node-bootstrap.shandruns
ensure_node— fnm/proto/nvm/brew/bundled, discovering nvm by sourcingnvm.shdirectly rather than via PATH) but never called it. This calls it,gated on a build actually running.
Placement is the subtle part:
_ensure_tui_node()must run before the buildenv is snapshotted via
with_hermes_node_path(), not just before npmresolution. The build runs as
subprocess.run([npm, "run", ...], env=env), andnpm's
#!/usr/bin/env nodeshebang resolvesnodeagainst thatenv's PATH atexec time. Repairing PATH only after the snapshot would let npm resolve yet
fail the build with
env: node: No such file(exit 127). Repairingos.environ["PATH"]before the snapshot fixes both at once.Related Issue
Refs #49242 (the POSIX/macOS sibling of that issue — intentionally not
Fixes, since #49242 also tracks the Windows WhatsApp paths and the #49259resolver consolidation, which this PR does not touch).
Type of Change
Changes Made
hermes_cli/main.py(cmd_gui): hoist thesource_mode/skip_build/force_buildflags above the env snapshot and call_ensure_tui_node()therewhen a build may run, so node/npm (and the
nodenpm's shebang needs) are onPATH for both resolution and the build subprocess. No-op when node+npm are
already present, or when
node-bootstrap.sh/bash are absent (e.g. Windows,where managed Node under
%LOCALAPPDATA%\hermes\nodealready covers this).tests/hermes_cli/test_gui_command.py: addtest_gui_repairs_path_before_env_snapshot(asserts
_ensure_tui_node()runs beforewith_hermes_node_path()— theexit-127 ordering guard) and
test_gui_skips_path_repair_when_skipping_build.How to Test
Reproduce the GUI/launchd environment from a terminal (node visible normally, so
the bug needs a stripped PATH and no managed Node):
$HERMES_HOME/nodetree.npm was not found on PATH.node-bootstrap.shand the rebuild proceeds.Automated + mechanism proof run in this branch:
pytest tests/hermes_cli/test_gui_command.py tests/hermes_cli/test_cmd_update.py -q→ 90 passed.scripts/check-windows-footguns.py --diff upstream/main→ clean.nvm + empty
HERMES_HOME: with the buggy ordering (snapshot first)npmandnodeboth resolve toNone; with the fix,npm/noderesolve andnpm --version(the#!/usr/bin/env nodepath) returns rc 0.Checklist
Code
pytest tests/ -qand all tests pass — ran the targetedtest_gui_command.py+test_cmd_update.pysuites (90 passed) and the footgun check; leaving the full suite to CIDocumentation & Housekeeping
cli-config.yaml.example— N/ACONTRIBUTING.md/AGENTS.md— N/Anode-bootstrap.shabsent →_ensure_tui_node()returns early), where managed Node already covers the caseNotes for reviewers
_build_web_ui()(thehermes servedashboard build) has the sameresolve-then-snapshot shape and would benefit from the same treatment, but it's
a different command and not normally launched from a GUI/launchd context — left
out to keep this PR single-concern; happy to follow up if you'd like it folded
in.