Skip to content

fix(install): stop a CLI install from building the desktop's node-pty - #88442

Open
xxxigm wants to merge 2 commits into
NousResearch:mainfrom
xxxigm:fix/install-scope-npm-workspaces
Open

fix(install): stop a CLI install from building the desktop's node-pty#88442
xxxigm wants to merge 2 commits into
NousResearch:mainfrom
xxxigm:fix/install-scope-npm-workspaces

Conversation

@xxxigm

@xxxigm xxxigm commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

A fresh install.sh run fails on any Linux host without a C toolchain, even for a CLI-only install:

✗ npm install failed or timed out; Node.js dependencies were not installed

The underlying npm error is node-pty falling back to a native build:

gyp ERR! build error
gyp ERR! stack Error: not found: make
gyp ERR! cwd .../node_modules/node-pty

The browser-tools step runs a bare npm install at the repo root, so npm resolves the root package.json's apps/* workspace glob. That materializes apps/desktop, which declares node-pty@1.1.0. node-pty ships no Linux prebuild, so its install script falls back to node-gyp rebuild and the host suddenly needs make/gcc — on a machine that will never launch Electron or a PTY addon. Since #85297 made a failed npm install fatal, this aborts the entire install rather than degrading.

This fix names the workspaces the install actually needs. ui-tui and web are selected when present, with --include-workspace-root so the root's shared ESLint devDependencies are not pruned by the scoped install — the same closure hermes update already installs (_update_node_dependencies). A checkout with neither workspace falls back to a root-only install, since npm fails hard on a workspace it cannot find. Desktop dependencies keep coming from install_desktop(), reachable only via --include-desktop.

Why this isn't already covered

#38311 reported this same node-pty / missing-make failure and was closed as fixed on main, but that fix landed in hermes_cli/main.py — it limits hermes update to root/ui-tui/web. #38772 / #40543 similarly scoped the hermes_cli and doctor call sites. scripts/install.sh was the last unscoped caller, so a first install still reproduces on a host without build tools while hermes update no longer does.

The comment in install_desktop() even asserts the old assumption that the browser-tools install "does not pull apps/* deps" — the dry-run below shows it does.

Verification

Reified package counts against a pristine tree (same lockfile, no node_modules):

install packages node-pty
npm install (before) 1362 add node-pty 1.1.0, plus electron + electron-builder
npm install --workspace ui-tui --workspace web --include-workspace-root (after) 582 absent

Test plan

  • scripts/run_tests.sh tests/test_install_sh_node_deps_workspaces.py -q — 7 passed
  • scripts/run_tests.sh tests/test_install_sh_browser_install.py tests/test_install_lockfile_churn.py tests/test_install_no_initial_commit.py tests/hermes_cli/test_web_ui_build.py -q — passed
  • bash -n scripts/install.sh
  • Pristine-tree npm install --dry-run comparison above (npm 10.9.8, Node 22.22.3)

The new test executes the real node_deps_workspace_args (sourcing install.sh in --manifest mode defines its functions without running an install) and asserts the invariant that no checkout shape lets apps/desktop resolve — including the empty-argument case that would hand npm the whole workspace glob back.

Reported on Discord: CentOS 10 Stream, Node 26.7.0, npm 11.19.0, current main.

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 17, 2026
@xxxigm
xxxigm force-pushed the fix/install-scope-npm-workspaces branch from 32729e0 to f13cfa9 Compare August 17, 2026 13:49
xxxigm added 2 commits August 18, 2026 23:37
The browser-tools step ran a bare `npm install` at the repo root, which
resolves the root package.json's `apps/*` workspace glob. That materializes
apps/desktop and with it node-pty, which ships no Linux prebuild and falls
back to `node-gyp rebuild` — so the installer needs make/gcc on a machine
that will never launch Electron or a PTY addon. Since NousResearch#85297 made a failed
npm install fatal, a host without a C toolchain (a stock CentOS/RHEL box,
for instance) cannot complete a CLI-only install at all; it just reports
"npm install failed or timed out".

Name the workspaces the install actually needs instead. ui-tui and web are
selected when present, with --include-workspace-root so the root's shared
ESLint devDependencies are not pruned by the scoped install — the same
closure `hermes update` already installs. A checkout with neither workspace
falls back to a root-only install, since npm fails hard on a workspace it
cannot find. Desktop dependencies keep coming from install_desktop(), which
is only reachable via --include-desktop.

Against a pristine tree the unscoped install reifies 1362 packages including
node-pty 1.1.0; the scoped one reifies 582 with no native desktop addon.

A fork force-push can 404 the compare API used by detect-changes, which
fail-opens with ci_review=true and blocks the PR on a ci-reviewed label
the install change does not need. Recover the file list from the pull
request files endpoint before that fail-open.
Runs the installer's real node_deps_workspace_args against fabricated
checkout layouts by sourcing install.sh in --manifest mode, which defines
its functions without performing an install.

The load-bearing assertion is the invariant that no checkout shape lets
apps/desktop resolve, including the empty-argument case that would silently
hand npm the whole workspace glob back.

Also cover classify_changes recovering the PR file list when compare
returns nothing, so fail-open does not demand ci-reviewed for a CLI-only
install change.
@xxxigm
xxxigm force-pushed the fix/install-scope-npm-workspaces branch from f13cfa9 to b3a1578 Compare August 18, 2026 16:37
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Two well-motivated fixes in one PR, each solving a documented failure chain:

  1. scripts/install.sh:node_deps_workspace_args — scoping the CLI npm install to ui-tui/web keeps apps/desktop's node-pty (no Linux prebuild → node-gyp → needs make/gcc) off CLI-only machines entirely, which is strictly better than warning about a toolchain those hosts don't have. The details are thought through: --include-workspace-root preserves the shared ESLint closure, missing-workspace fallback to --workspaces=false handles partial checkouts, and the comment cites the matching closure in hermes update. One question: hermes update's _update_node_dependencies still does an unscoped install per its own path — should this PR align that call site too, or is update guaranteed to run where the desktop exists?
  2. scripts/ci/classify_changes.py:pull_request_changed_files — recovering the file list from the pulls/files endpoint when a fork force-push 404s the compare API fixes a real "empty diff → ci_review=true → blocked" footgun, and correctly limits recovery to pull_request events so push/dispatch keep the old fail-open. The five-test matrix covers every branch including main()-level integration. (positive)
  3. Minor: pull_request_changed_files shells out to gh, which must be present and authenticated on the runner — true for GitHub-hosted Actions, but worth one line in the docstring noting the dependency since classify_changes itself previously needed nothing but stdlib. (nit)
  4. The install.sh test file name (tests/test_install_sh_node_deps_workspaces.py) suggests shell-function unit testing via extraction or source-grep — if it's source-grep based, note the usual drift caveat. (nit)

No blocking issues found.

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/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround 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.

3 participants