From cffc8bb2a4b5c1b6de818291a8f521adf167e694 Mon Sep 17 00:00:00 2001 From: xxxigm Date: Tue, 18 Aug 2026 23:37:18 +0700 Subject: [PATCH] fix(install): stop a CLI install from building the desktop's node-pty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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. --- scripts/ci/classify_changes.py | 64 ++++++++++++++++++++++++++++++++++ scripts/install.sh | 35 ++++++++++++++++++- 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/scripts/ci/classify_changes.py b/scripts/ci/classify_changes.py index 7f608d0af9..935703c870 100644 --- a/scripts/ci/classify_changes.py +++ b/scripts/ci/classify_changes.py @@ -57,6 +57,7 @@ from __future__ import annotations import json import os +import subprocess import sys _FRONTEND = ("ui-tui/", "web/", "apps/") # TS typecheck-matrix packages @@ -230,9 +231,72 @@ def classify(files: list[str]) -> dict[str, bool]: return ret +def _pull_request_number() -> str | None: + """Read the PR number from the Actions event payload, if present.""" + event_path = os.environ.get("GITHUB_EVENT_PATH") + if not event_path: + return None + try: + with open(event_path, encoding="utf-8") as fh: + payload = json.load(fh) + except (OSError, json.JSONDecodeError): + return None + number = (payload.get("pull_request") or {}).get("number") + return str(number) if number else None + + +def pull_request_changed_files() -> list[str]: + """Recover the PR file list when the compare API returned nothing. + + ``detect-changes`` calls ``repos/.../compare/base...head`` with raw SHAs. + A fork force-push can 404 for ~30s until GitHub attaches the new head SHA + to the base repo, so the action fails open with an empty file list. That + forces ``ci_review=true`` and blocks the PR on a ``ci-reviewed`` label + even when no CI-sensitive file changed. + + The pull-request files endpoint already knows the PR's files (it is how + this action used to classify), so use it as a fallback on pull_request + events only. Push/dispatch keep the empty-diff fail-open. + """ + if os.environ.get("EVENT_NAME") != "pull_request": + return [] + repo = os.environ.get("REPO") or os.environ.get("GITHUB_REPOSITORY") or "" + pr = _pull_request_number() + if not repo or not pr: + return [] + try: + completed = subprocess.run( + [ + "gh", + "api", + "--paginate", + f"repos/{repo}/pulls/{pr}/files", + "--jq", + ".[].filename", + ], + check=False, + capture_output=True, + text=True, + timeout=30, + ) + except (OSError, subprocess.TimeoutExpired): + return [] + if completed.returncode != 0: + return [] + return [line.strip() for line in completed.stdout.splitlines() if line.strip()] + def main() -> int: files = sys.stdin.read().splitlines() + if not any(f.strip() for f in files): + recovered = pull_request_changed_files() + if recovered: + print( + f"compare API returned no files; recovered {len(recovered)} " + "path(s) from the pull request files endpoint", + file=sys.stderr, + ) + files = recovered lanes = classify(files) out = "\n".join([ *(f"{key}={str(value).lower()}" for key, value in lanes.items()), diff --git a/scripts/install.sh b/scripts/install.sh index ea69ab6329..916a06ebf2 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -2531,6 +2531,36 @@ configure_browser_env_from_system_browser() { log_success "Configured browser tools to use $browser_path" } +# Select the npm workspaces a CLI install actually needs, into the +# NODE_DEPS_WORKSPACE_ARGS array. +# +# A bare `npm install` at the repo root resolves package.json's `apps/*` +# glob, which materializes apps/desktop — and with it node-pty, which ships +# no Linux prebuild and falls back to `node-gyp rebuild`. On a host without +# make/gcc that rebuild fails, and since #85297 made a failed npm install +# fatal it aborts the whole install of a machine that will never launch +# Electron or a PTY addon (#38311, #38772). Desktop dependencies are +# installed by install_desktop(), reachable only via --include-desktop. +# +# Naming ui-tui/web excludes the unnamed apps/* workspaces, and +# --include-workspace-root keeps the root's own devDependencies (the shared +# ESLint flat config each workspace imports) from being pruned by the scoped +# install — the same closure `hermes update` installs +# (hermes_cli/main.py::_update_node_dependencies). Prebuilt/partial checkouts +# can lack a workspace, and naming a missing one makes npm fail hard, so fall +# back to a root-only install that still skips apps/*. +node_deps_workspace_args() { + local install_dir="$1" + NODE_DEPS_WORKSPACE_ARGS=() + [ -f "$install_dir/ui-tui/package.json" ] && NODE_DEPS_WORKSPACE_ARGS+=(--workspace ui-tui) + [ -f "$install_dir/web/package.json" ] && NODE_DEPS_WORKSPACE_ARGS+=(--workspace web) + if [ "${#NODE_DEPS_WORKSPACE_ARGS[@]}" -eq 0 ]; then + NODE_DEPS_WORKSPACE_ARGS=(--workspaces=false) + return 0 + fi + NODE_DEPS_WORKSPACE_ARGS+=(--include-workspace-root) +} + install_node_deps() { if [ "$HAS_NODE" = false ]; then log_info "Skipping Node.js dependencies (Node not installed)" @@ -2553,9 +2583,12 @@ install_node_deps() { # installed", hiding the degradation from the user (#77003). Now it # fails the install outright instead of burying the warning (#85297). # Capture npm output so failures are diagnosable (#87340). + # Scoped to the workspaces a CLI install needs so apps/desktop's + # node-pty is never built here — see node_deps_workspace_args(). + node_deps_workspace_args "$INSTALL_DIR" local npm_log npm_log="$(mktemp)" - if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install --silent \ + if ! run_with_timeout "$NODE_DEPS_TIMEOUT" npm install "${NODE_DEPS_WORKSPACE_ARGS[@]}" --silent \ >"$npm_log" 2>&1; then log_error "npm install failed or timed out; Node.js dependencies were not installed" if [ -s "$npm_log" ]; then