Skip to content

fix(scripts): gate Node version in desktop rebuild path - #85997

Open
woshicby wants to merge 1 commit into
NousResearch:mainfrom
woshicby:fix/desktop-rebuild-node-check
Open

fix(scripts): gate Node version in desktop rebuild path#85997
woshicby wants to merge 1 commit into
NousResearch:mainfrom
woshicby:fix/desktop-rebuild-node-check

Conversation

@woshicby

@woshicby woshicby commented Aug 14, 2026

Copy link
Copy Markdown

fix(scripts): gate Node version in desktop rebuild path

Problem

hermes update on a machine whose default node is incompatible with the
desktop build (e.g. Node 25 + nanoid@6, which requires ^22 || ^24 || >=26)
silently fails during the Electron rebuild step:

Change

scripts/desktop-update/posix.sh:

  1. Node build gate — new node_satisfies_build() evaluates a candidate
    Node against the desktop's declared floor. The predicate is NOT hardcoded:
    it reads apps/desktop/package.json's engines.node fresh on every call
    via the dependency-free scripts/lib/node-version-check.js semver
    evaluator, so when the official dependency declaration changes the gate
    follows automatically — no manual copy to keep in sync.
  2. Engines declaration tightenedapps/desktop/package.json now carries
    the real floor ^22.22.0 || ^24.0.0 || >=26.0.0 (matching nanoid@6),
    replacing the looser >=22.22.0 that let odd-numbered releases 23/25 pass
    the root floor and then die in npm ci with EBADENGINE.
  3. Auto-select a compatible Node — new prepare_node_for_build() runs
    before the update/rebuild: if PATH already has a compatible node, leave it
    alone; otherwise probe the standard Homebrew versioned installs
    (/opt/homebrew/opt/node@22|24|26/bin, /usr/local/opt/...) and prepend
    the first compatible one to PATH. HERMES_NODE_CANDIDATE_DIRS overrides
    the candidate list for custom layouts. A candidate only counts if both
    node and npm are present.
  4. Self-test mode--self-test-node-gate --node-version <v> prints
    compatible/incompatible for CI tests, mirroring the official
    --self-test-gate precedent.
  5. Diagnosable failure — when the rebuild fails after auto-selection, the
    exit-6 message now includes the Node version actually used.

Tests (tests-js/desktop-rebuild-node-gate.test.ts, 8 tests) drive the
self-test gate over the version matrix, assert the shared evaluator matches
the gate, and cover candidate selection (node-without-npm is skipped, node+npm
candidate is picked). Full tests-js suite: 32/32 passing.

Design note — install vs rebuild

The install path (#84397, since tightened further to require Node 26 with the
Hermes-managed toolchain) can afford to be strict on a fresh install; the
REBUILD path must be tolerant of existing machines, so this PR auto-selects an
already-installed compatible Node instead of failing. They are complementary:
the gate stays strict about what Node is used, while users with a valid Node
installed somewhere on the machine (very common on macOS via Homebrew versioned
formulae) aren't forced to edit their PATH just to run an update.

Related

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 14, 2026
@woshicby
woshicby force-pushed the fix/desktop-rebuild-node-check branch 2 times, most recently from afc058a to d46b779 Compare August 15, 2026 06:05
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(scripts): gate Node version in desktop rebuild path — good extraction of a shared predicate and a pragmatic no-download rebuild gate. Observations:

  1. scripts/lib/node-version-check.sh — the header markets this as the single source of truth "shared with install.sh", but scripts/install.sh still carries its own node_satisfies_build copy (the comment itself says "Keep in sync with install.sh's node_satisfies_build"). The drift problem the file was created to solve persists until install.sh actually sources it — consider migrating install.sh in the same PR.

  2. scripts/desktop-update/posix.sh prepare_node_for_buildfor cand in ${HERMES_NODE_CANDIDATE_DIRS:-...} relies on word splitting, so a candidate path containing spaces silently fails the [ -x "$cand/node" ] check. The comment documents space-separated, so acceptable — but note prepare_node_for_build mutates the global PATH via export; if ever invoked twice (retry paths), the candidate is prepended twice (harmless but worth knowing).

  3. tests-js/desktop-rebuild-node-gate.test.ts shells out to bash and writes to a hardcoded /tmp/hermes-node-gate-test. On a Windows test host without bash (or a different temp layout) the suite fails outright. If these tests are expected on the Windows lane, gate with process.platform or use os.tmpdir().

@woshicby
woshicby force-pushed the fix/desktop-rebuild-node-check branch from d46b779 to 7747582 Compare August 17, 2026 06:42
@woshicby
woshicby requested a review from a team August 17, 2026 06:42
@woshicby

Copy link
Copy Markdown
Author

Agreed — the "single source of truth" claim was not accurate while install.sh carried its own copy. We went further than syncing the comment: the desktop gate no longer hardcodes any version set at all. apps/desktop/package.json's engines.node now declares the real floor (^22.22.0 || ^24.0.0 || >=26.0.0, matching nanoid@6's ^22||^24||>=26), and node-version-check.sh reads that declaration fresh on every run via a small dependency-free semver evaluator (scripts/lib/node-version-check.js). When the upstream dependency declaration changes, the gate follows automatically — no manual copy to keep in sync, and no drift window.

Note the two gates are intentionally distinct: install.sh's node_satisfies_build guards browser tools (floor >=22.22, where 23/25 are fine), while this one guards the desktop dependency tree (where nanoid@6 excludes 23/25). Keeping install.sh's copy untouched preserves its behavior; the desktop floor now lives where it belongs, in the desktop package declaration.

Copy link
Copy Markdown
Contributor

Windows complement published in #91079.

This PR remains the POSIX desktop-update Node gate. #91079 closes the different direct Windows hermes desktop packaging path established by #90134: npm may launch the workspace script under one runtime while the script's bare node resolves through fnm/system PATH to another.

The Windows wrapper now selects npm's exact npm_node_execpath, permits one bounded self-reexec, and gates Node >=22.22.0 plus require(esm) before loading electron-builder.

Exact Windows head: 8e641c78f1073321a79195b62c105982f6a01240, with CI, Docker, and Nix green on that exact object.

@woshicby

Copy link
Copy Markdown
Author

Thanks for the complement — this splits cleanly. #91079 owns the Windows direct packaging path (npm's exact npm_node_execpath, the bounded self-reexec, and the >=22.22.0 + require(esm) gate before electron-builder), while #85997 stays scoped to the POSIX desktop-update rebuild path.

Good to see the same Node floor on both sides: our predicate in #85997 is the engines-derived ^22.22.0 || ^24.0.0 || >=26.0.0 (nanoid engines ∩ react-router floor), and the Windows wrapper gates on the same >=22.22.0 baseline — the two PRs agree on the boundary without coupling. Files don't overlap either: the only shared file is apps/desktop/package.json, and we touch different fields (engines vs. build/beforePack script chain), so both should merge cleanly.

The desktop dependency tree (nanoid@6 and friends) only accepts Node
22.22+/24/26+; odd-numbered releases (23/25) pass the root engines.node
floor but then die in npm ci with EBADENGINE. hermes update makes the GUI
build failure non-fatal, so a bad Node silently leaves the user on the
previous build with a generic rebuild-failed message.

Gate the REBUILD path (scripts/desktop-update/posix.sh): prefer an
already-installed compatible Node on PATH; otherwise look for a
Homebrew/usr-local Node to prepend. The version predicate is not
hardcoded — node-version-check.js reads apps/desktop/package.json's
engines.node fresh on every run (now ^22.22.0 || ^24.0.0 || >=26.0.0,
matching nanoid@6), so the gate follows the official declaration with no
manual copy to keep in sync. Adds --self-test-node-gate for tests.
@woshicby
woshicby force-pushed the fix/desktop-rebuild-node-check branch from 7747582 to c74e205 Compare August 25, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants