From 363d1aefa449e74c6fe1a92a14f76c59002fe69d Mon Sep 17 00:00:00 2001 From: "Gerardo Camorlinga Jr." Date: Tue, 21 Jul 2026 16:47:42 -0500 Subject: [PATCH 1/3] fix(update): recover web UI build when tsc/vite missing after npm install hermes update could report a successful workspace npm ci while devDependencies were omitted (production/omit-dev env leakage), leaving tsc/vite unlinked. The subsequent web UI build then failed with `tsc: not found` and fell back to a stale dashboard dist. Force npm_config_include=dev on install, verify toolchain shims after workspace install (and refuse to hash-cache a half tree), repair/reinstall when node_modules exists without tsc/vite, prepend workspace .bin dirs to PATH, and retry the build after a tsc/vite-not-found failure. --- hermes_cli/main.py | 158 ++++++++++++++++++++++++-- tests/hermes_cli/test_web_ui_build.py | 96 ++++++++++++++++ 2 files changed, 243 insertions(+), 11 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 3f3d5680647d..38bf0a96628d 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5195,6 +5195,15 @@ def _run_npm_install_deterministic( # --silent/capture_output). It no-ops when CI is set — same as the TUI # install path and nix/lib.nix npm ci hooks. run_env = {**os.environ, **(env or {}), "CI": "1"} + # Belt-and-suspenders on top of the CLI ``--include=dev`` flag: an + # inherited ``npm_config_omit=dev`` / ``npm_config_production=true`` from a + # shell profile, container image, or gateway service environment can still + # win depending on npm version + flag ordering, which leaves the install + # green (exit 0) while ``tsc``/``vite`` never land — then ``npm run build`` + # dies with ``tsc: not found`` (exit 127). Force the config-level knobs. + run_env["npm_config_include"] = "dev" + run_env["npm_config_production"] = "false" + run_env.pop("npm_config_omit", None) lockfile = cwd / "package-lock.json" if lockfile.exists(): @@ -5226,6 +5235,49 @@ def _run_npm_install_deterministic( ) +def _node_modules_bin_dir(root: Path) -> Path: + """Return ``root/node_modules/.bin`` (may not exist yet).""" + return root / "node_modules" / ".bin" + + +def _npm_bin_exists(bin_dir: Path, name: str) -> bool: + """True when an npm bin shim for *name* exists (POSIX or Windows).""" + return any( + (bin_dir / candidate).exists() + for candidate in (name, f"{name}.cmd", f"{name}.ps1", f"{name}.exe") + ) + + +def _web_build_toolchain_ready(project_root: Path) -> bool: + """Return True when the web UI build toolchain shims are present. + + ``hermes update`` can report a successful workspace ``npm ci`` while + ``tsc``/``vite`` are still missing — most often because an inherited + production/omit-dev npm config stripped ``devDependencies`` (exit 0, no + error), or because a prior install was interrupted mid-link. The web + build then fails with ``tsc: not found``. Callers use this as a readiness + gate after install and as a reason to invalidate the lockfile-hash skip. + """ + if not (project_root / "web" / "package.json").is_file(): + return True + bin_dir = _node_modules_bin_dir(project_root) + if not bin_dir.is_dir(): + return False + return _npm_bin_exists(bin_dir, "tsc") and _npm_bin_exists(bin_dir, "vite") + + +def _prepend_node_bin_dirs(env: dict[str, str], *roots: Path) -> dict[str, str]: + """Prepend each root's ``node_modules/.bin`` onto PATH (existing dirs only).""" + merged = dict(env) + parts = [p for p in merged.get("PATH", "").split(os.pathsep) if p] + for root in reversed(roots): + bin_dir = str(_node_modules_bin_dir(root)) + if Path(bin_dir).is_dir() and bin_dir not in parts: + parts.insert(0, bin_dir) + merged["PATH"] = os.pathsep.join(parts) + return merged + + def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: """Build the web UI frontend if npm is available, serializing across processes. @@ -5333,12 +5385,26 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_workspace_args: tuple[str, ...] = () if npm_cwd == web_dir else ("--workspace", "web") if _is_termux_startup_environment(): npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) - r1 = _run_npm_install_deterministic( - npm, - npm_cwd, - extra_args=(*npm_workspace_args, "--silent"), - env=build_env, - ) + + def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": + extra = (*npm_workspace_args,) + if silent: + extra = (*extra, "--silent") + return _run_npm_install_deterministic( + npm, + npm_cwd, + extra_args=extra, + env=build_env, + ) + + def _refresh_build_path() -> dict[str, str]: + # npm lifecycle scripts normally inject node_modules/.bin, but after a + # partial/hoisted workspace install the web package may have a + # node_modules tree without its own .bin while tsc/vite live at the + # workspace root. Prepending both keeps `tsc -b && vite build` resolvable. + return _prepend_node_bin_dirs(build_env, web_dir, npm_cwd) + + r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( f" {'✗' if fatal else '⚠'} Web UI npm install failed" @@ -5348,6 +5414,49 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: if fatal: _say(" Run manually: npm install --workspace web && npm run build -w web") return False + + # Install can exit 0 while still omitting devDependencies (production/omit + # config races). Detect missing tsc/vite and force a visible repair install + # before attempting the build — this is the failure mode behind + # ``tsc: not found`` during `hermes update`. + project_root_for_tools = ( + web_dir.parent.parent if web_dir.parent.name == "apps" else web_dir.parent + ) + # Only repair when an install produced a node_modules tree but left the + # toolchain incomplete. Unit tests mock npm with no filesystem side + # effects (no node_modules) — skip the extra install there. + if ( + (project_root_for_tools / "node_modules").is_dir() + and not _web_build_toolchain_ready(project_root_for_tools) + ): + _say(" ⚠ web toolchain (tsc/vite) missing after npm install — repairing...") + r_repair = _install_web_deps(silent=False) + if r_repair.returncode != 0: + _say( + f" {'✗' if fatal else '⚠'} Web UI toolchain repair failed" + + ("" if fatal else " (hermes web will not be available)") + ) + _relay(r_repair) + if fatal: + _say( + " Run manually: npm install --workspace web --include=dev " + "&& npm run build -w web" + ) + return False + if not _web_build_toolchain_ready(project_root_for_tools): + _say( + f" {'✗' if fatal else '⚠'} tsc/vite still missing after repair" + + ("" if fatal else " (hermes web will not be available)") + ) + if fatal: + _say( + " Run manually: npm install --workspace web --include=dev " + "&& npm run build -w web" + ) + return not fatal + + build_env = _refresh_build_path() + # First attempt — stream output via idle-timeout helper (issue #33788). # capture_output=True on a long Vite build looks identical to a hang; # users react by rebooting, which leaves the editable install in a @@ -5355,11 +5464,20 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - # Retry once after a short delay — covers boot-time races on Windows - # (antivirus scanning Node.js binaries, npm cache not ready, transient - # I/O when launched via Scheduled Task at logon). See issue #23817. - _time.sleep(3) - r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + build_out = (r2.stdout or "") + (r2.stderr or "") + # Targeted recovery for the classic missing-devDep failure before the + # generic delayed retry. Re-install (non-silent) + refresh PATH. + if "tsc: not found" in build_out or "vite: not found" in build_out: + _say(" ⚠ Build missing tsc/vite — reinstalling web devDependencies and retrying...") + _install_web_deps(silent=False) + build_env = _refresh_build_path() + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + if r2.returncode != 0: + # Retry once after a short delay — covers boot-time races on Windows + # (antivirus scanning Node.js binaries, npm cache not ready, transient + # I/O when launched via Scheduled Task at logon). See issue #23817. + _time.sleep(3) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # _run_with_idle_timeout merges stderr into stdout; older callers @@ -10140,6 +10258,12 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True + # A matching lockfile hash with a half-installed tree (e.g. production + # omit stripped tsc/vite) must NOT skip reinstall — otherwise every + # subsequent `hermes update` keeps serving a stale web dist after a + # silent toolchain gap. + if not _web_build_toolchain_ready(PROJECT_ROOT): + return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] @@ -10316,6 +10440,18 @@ def _update_node_dependencies() -> list[str]: env=nixos_env, ) if ws_result.returncode == 0: + if _web_build_toolchain_ready(PROJECT_ROOT): + _record_npm_lockfile_hash(shared_hermes_root) + print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") + return [] + # Mocked unit tests call this with a successful npm exit code but no + # real node_modules tree — trust the exit code in that case. A real + # install that left node_modules without tsc/vite is the failure mode + # we must not paper over (and must not hash-cache). + if (PROJECT_ROOT / "node_modules").is_dir(): + print(" ⚠ npm workspace install finished without tsc/vite shims") + print(" (devDependencies likely omitted — check NODE_ENV / npm omit)") + return _partial_update_failure("ui-tui, web workspaces") _record_npm_lockfile_hash(shared_hermes_root) print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") return [] diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index 95c0960de41a..b483db76da25 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -563,3 +563,99 @@ class TestBuildWebUIFlock: def test_lock_file_is_gitignored(self): gitignore = Path(__file__).resolve().parents[2] / ".gitignore" assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") + + +class TestNpmInstallDevDepEnvForcing: + """Config-level guards so production/omit-dev env can't strip tsc/vite.""" + + def test_forces_npm_config_include_dev_and_clears_omit(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (web_dir / "package-lock.json").write_text("{}", encoding="utf-8") + mock_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + with patch("hermes_cli.main.subprocess.run", return_value=mock_cp) as mock_run: + _run_npm_install_deterministic( + "/usr/bin/npm", + web_dir, + env={ + "NODE_ENV": "production", + "npm_config_omit": "dev", + "npm_config_production": "true", + }, + ) + _, kwargs = mock_run.call_args + env = kwargs["env"] + assert env["npm_config_include"] == "dev" + assert env["npm_config_production"] == "false" + assert "npm_config_omit" not in env + assert env["CI"] == "1" + + +class TestWebToolchainReadyAndRepair: + def test_toolchain_ready_requires_tsc_and_vite_shims(self, tmp_path): + from hermes_cli.main import _web_build_toolchain_ready + + web_dir, _ = _make_web_dir(tmp_path) + root = web_dir.parent + assert _web_build_toolchain_ready(root) is False + bin_dir = root / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + (bin_dir / "tsc").touch() + assert _web_build_toolchain_ready(root) is False + (bin_dir / "vite").touch() + assert _web_build_toolchain_ready(root) is True + + def test_build_repairs_when_node_modules_exists_without_tsc(self, tmp_path): + """Partial install (node_modules present, no tsc) triggers a repair install.""" + from hermes_cli.main import _resolve_node_runtime_npm + + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + # Simulate a production-omit install: tree exists, toolchain missing. + (tmp_path / "node_modules").mkdir() + (tmp_path / "node_modules" / ".bin").mkdir() + + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + install_calls = {"n": 0} + + def fake_install(*_a, **_k): + install_calls["n"] += 1 + # After the repair install, pretend tsc/vite appeared. + if install_calls["n"] >= 2: + bin_dir = tmp_path / "node_modules" / ".bin" + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + return install_ok + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", side_effect=fake_install) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + # First silent install + one repair install. + assert mock_install.call_count == 2 + + def test_build_retries_install_on_tsc_not_found(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_fail = __import__("subprocess").CompletedProcess( + [], 127, stdout="sh: 1: tsc: not found\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[build_fail, build_ok]) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + # Initial install + repair install after tsc: not found. + assert mock_install.call_count == 2 + assert mock_build.call_count == 2 From 690ecfa1c7d317338086a98e8849105be6501187 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 12:52:42 -0500 Subject: [PATCH 2/3] fix(update): anchor the web toolchain check on npm's actual search path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reshapes the salvaged fix so it recovers the `tsc: not found` build without mis-diagnosing healthy trees. The npm config forcing is dropped. It rested on the premise that an inherited `npm_config_omit=dev` can beat the `--include=dev` CLI flag; npm resolves command-line flags above environment config and filters `omit` by `include`, so the flag already wins. Verified on npm 10 and 11.5.1: with `npm_config_omit=dev` set and `--include=dev` passed, devDependencies install. `npm_config_production` is worse than redundant — npm 9 removed it, so setting it prints `npm warn config production Use --omit=dev instead.` on every install. The readiness probe now reads every root `npm run build` searches, not just the workspace root. npm links a package's bin shims under the package itself when it owns its lockfile (#42973), so a root-only check called a working tree broken: it forced a redundant install, skipped the build entirely, and made `hermes web` exit 1 on a layout that builds fine today. The pre-build probe is gone with it. A build that works is never second-guessed and no filesystem introspection gates it; recovery is driven by the failure instead. When the build cannot resolve tsc or vite, we reinstall (visibly) and retry before the generic delayed retry, which otherwise just reruns the same command and leaves the stale dist in place forever. The lockfile-hash skip still invalidates on an incomplete toolchain so the next update repairs itself. Also drops the branches that changed behavior based on whether a test mock was installed, and replaces the mock-call-count tests with real temp trees covering both hoisting layouts, the Windows shim extensions, and each shell's wording of an unresolvable binary. Co-authored-by: Gerardo Camorlinga Jr. --- hermes_cli/main.py | 174 +++++++++----------------- tests/hermes_cli/test_cmd_update.py | 35 ++++++ tests/hermes_cli/test_web_ui_build.py | 168 +++++++++++++++---------- 3 files changed, 198 insertions(+), 179 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 38bf0a96628d..f8ae45fdf2b0 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5195,15 +5195,6 @@ def _run_npm_install_deterministic( # --silent/capture_output). It no-ops when CI is set — same as the TUI # install path and nix/lib.nix npm ci hooks. run_env = {**os.environ, **(env or {}), "CI": "1"} - # Belt-and-suspenders on top of the CLI ``--include=dev`` flag: an - # inherited ``npm_config_omit=dev`` / ``npm_config_production=true`` from a - # shell profile, container image, or gateway service environment can still - # win depending on npm version + flag ordering, which leaves the install - # green (exit 0) while ``tsc``/``vite`` never land — then ``npm run build`` - # dies with ``tsc: not found`` (exit 127). Force the config-level knobs. - run_env["npm_config_include"] = "dev" - run_env["npm_config_production"] = "false" - run_env.pop("npm_config_omit", None) lockfile = cwd / "package-lock.json" if lockfile.exists(): @@ -5235,11 +5226,6 @@ def _run_npm_install_deterministic( ) -def _node_modules_bin_dir(root: Path) -> Path: - """Return ``root/node_modules/.bin`` (may not exist yet).""" - return root / "node_modules" / ".bin" - - def _npm_bin_exists(bin_dir: Path, name: str) -> bool: """True when an npm bin shim for *name* exists (POSIX or Windows).""" return any( @@ -5248,34 +5234,52 @@ def _npm_bin_exists(bin_dir: Path, name: str) -> bool: ) -def _web_build_toolchain_ready(project_root: Path) -> bool: - """Return True when the web UI build toolchain shims are present. +def _web_build_toolchain_ready(*roots: Path) -> bool: + """True when ``tsc`` and ``vite`` shims are reachable from any of *roots*. - ``hermes update`` can report a successful workspace ``npm ci`` while - ``tsc``/``vite`` are still missing — most often because an inherited - production/omit-dev npm config stripped ``devDependencies`` (exit 0, no - error), or because a prior install was interrupted mid-link. The web - build then fails with ``tsc: not found``. Callers use this as a readiness - gate after install and as a reason to invalidate the lockfile-hash skip. + Callers must pass every root the build would search; checking only one + reports a healthy tree as broken. """ - if not (project_root / "web" / "package.json").is_file(): - return True - bin_dir = _node_modules_bin_dir(project_root) - if not bin_dir.is_dir(): - return False - return _npm_bin_exists(bin_dir, "tsc") and _npm_bin_exists(bin_dir, "vite") + bin_dirs = [ + bin_dir + for bin_dir in (root / "node_modules" / ".bin" for root in roots) + if bin_dir.is_dir() + ] + return bool(bin_dirs) and all( + any(_npm_bin_exists(bin_dir, tool) for bin_dir in bin_dirs) + for tool in ("tsc", "vite") + ) -def _prepend_node_bin_dirs(env: dict[str, str], *roots: Path) -> dict[str, str]: - """Prepend each root's ``node_modules/.bin`` onto PATH (existing dirs only).""" - merged = dict(env) - parts = [p for p in merged.get("PATH", "").split(os.pathsep) if p] - for root in reversed(roots): - bin_dir = str(_node_modules_bin_dir(root)) - if Path(bin_dir).is_dir() and bin_dir not in parts: - parts.insert(0, bin_dir) - merged["PATH"] = os.pathsep.join(parts) - return merged +def _web_toolchain_roots(web_dir: Path) -> tuple[Path, ...]: + """Roots whose ``node_modules/.bin`` can satisfy the web build. + + ``npm run build`` prepends ``node_modules/.bin`` for the package and each + of its ancestors, so shims hoisted to the workspace root and shims nested + under a package that owns its lockfile (#42973) are equally valid. + """ + return (web_dir, web_dir.parent) + + +def _missing_web_build_tool(output: str) -> str | None: + """Return the build tool a failed ``npm run build`` could not resolve. + + Each shell words this differently: ``sh: 1: tsc: not found`` (dash), + ``vite: command not found`` (bash/zsh), and ``'tsc' is not recognized as + an internal or external command`` (cmd.exe). + """ + lowered = output.lower() + for tool in ("tsc", "vite"): + if any( + phrase in lowered + for phrase in ( + f"{tool}: not found", + f"{tool}: command not found", + f"'{tool}' is not recognized", + ) + ): + return tool + return None def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: @@ -5387,23 +5391,13 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": - extra = (*npm_workspace_args,) - if silent: - extra = (*extra, "--silent") return _run_npm_install_deterministic( npm, npm_cwd, - extra_args=extra, + extra_args=(*npm_workspace_args, "--silent") if silent else npm_workspace_args, env=build_env, ) - def _refresh_build_path() -> dict[str, str]: - # npm lifecycle scripts normally inject node_modules/.bin, but after a - # partial/hoisted workspace install the web package may have a - # node_modules tree without its own .bin while tsc/vite live at the - # workspace root. Prepending both keeps `tsc -b && vite build` resolvable. - return _prepend_node_bin_dirs(build_env, web_dir, npm_cwd) - r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( @@ -5414,49 +5408,6 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: if fatal: _say(" Run manually: npm install --workspace web && npm run build -w web") return False - - # Install can exit 0 while still omitting devDependencies (production/omit - # config races). Detect missing tsc/vite and force a visible repair install - # before attempting the build — this is the failure mode behind - # ``tsc: not found`` during `hermes update`. - project_root_for_tools = ( - web_dir.parent.parent if web_dir.parent.name == "apps" else web_dir.parent - ) - # Only repair when an install produced a node_modules tree but left the - # toolchain incomplete. Unit tests mock npm with no filesystem side - # effects (no node_modules) — skip the extra install there. - if ( - (project_root_for_tools / "node_modules").is_dir() - and not _web_build_toolchain_ready(project_root_for_tools) - ): - _say(" ⚠ web toolchain (tsc/vite) missing after npm install — repairing...") - r_repair = _install_web_deps(silent=False) - if r_repair.returncode != 0: - _say( - f" {'✗' if fatal else '⚠'} Web UI toolchain repair failed" - + ("" if fatal else " (hermes web will not be available)") - ) - _relay(r_repair) - if fatal: - _say( - " Run manually: npm install --workspace web --include=dev " - "&& npm run build -w web" - ) - return False - if not _web_build_toolchain_ready(project_root_for_tools): - _say( - f" {'✗' if fatal else '⚠'} tsc/vite still missing after repair" - + ("" if fatal else " (hermes web will not be available)") - ) - if fatal: - _say( - " Run manually: npm install --workspace web --include=dev " - "&& npm run build -w web" - ) - return not fatal - - build_env = _refresh_build_path() - # First attempt — stream output via idle-timeout helper (issue #33788). # capture_output=True on a long Vite build looks identical to a hang; # users react by rebooting, which leaves the editable install in a @@ -5464,13 +5415,15 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - build_out = (r2.stdout or "") + (r2.stderr or "") - # Targeted recovery for the classic missing-devDep failure before the - # generic delayed retry. Re-install (non-silent) + refresh PATH. - if "tsc: not found" in build_out or "vite: not found" in build_out: - _say(" ⚠ Build missing tsc/vite — reinstalling web devDependencies and retrying...") + # The install above can exit 0 while leaving the tree without a build + # toolchain — a lockfile-hash skip over a half-installed tree, or an + # interrupted link step. The generic retry below just reruns the same + # command, so `tsc: not found` survives it and the stale dist is + # served forever. Reinstall (non-silent, so the user sees it) first. + missing_tool = _missing_web_build_tool((r2.stdout or "") + (r2.stderr or "")) + if missing_tool: + _say(f" ⚠ Build could not resolve {missing_tool} — reinstalling web dependencies...") _install_web_deps(silent=False) - build_env = _refresh_build_path() r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # Retry once after a short delay — covers boot-time races on Windows @@ -10258,11 +10211,14 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True - # A matching lockfile hash with a half-installed tree (e.g. production - # omit stripped tsc/vite) must NOT skip reinstall — otherwise every - # subsequent `hermes update` keeps serving a stale web dist after a - # silent toolchain gap. - if not _web_build_toolchain_ready(PROJECT_ROOT): + # A matching lockfile hash over a tree whose web build toolchain never + # landed must NOT skip the reinstall — otherwise every later `hermes + # update` keeps rebuilding against a half-installed tree and serving a + # stale dist. + web_dir = PROJECT_ROOT / "web" + if (web_dir / "package.json").is_file() and not _web_build_toolchain_ready( + *_web_toolchain_roots(web_dir) + ): return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. @@ -10440,18 +10396,6 @@ def _update_node_dependencies() -> list[str]: env=nixos_env, ) if ws_result.returncode == 0: - if _web_build_toolchain_ready(PROJECT_ROOT): - _record_npm_lockfile_hash(shared_hermes_root) - print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") - return [] - # Mocked unit tests call this with a successful npm exit code but no - # real node_modules tree — trust the exit code in that case. A real - # install that left node_modules without tsc/vite is the failure mode - # we must not paper over (and must not hash-cache). - if (PROJECT_ROOT / "node_modules").is_dir(): - print(" ⚠ npm workspace install finished without tsc/vite shims") - print(" (devDependencies likely omitted — check NODE_ENV / npm omit)") - return _partial_update_failure("ui-tui, web workspaces") _record_npm_lockfile_hash(shared_hermes_root) print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") return [] diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 5a1914bc76bb..d550a2297230 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -149,6 +149,41 @@ class TestCmdUpdateNpmLockfileCache: ) assert hm._npm_lockfile_changed(tmp_path) is True + def test_missing_web_build_toolchain_defeats_skip(self, tmp_path, monkeypatch): + """A hash recorded over a tree that never got tsc/vite must not skip. + + Otherwise the half-installed tree is permanent: every later update + trusts the hash, the build keeps failing, and the stale dist is served + forever. + """ + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"workspaces": ["web"]}') + (tmp_path / "web").mkdir() + (tmp_path / "web" / "package.json").write_text("{}") + bin_dir = tmp_path / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is True + + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + assert hm._npm_lockfile_changed(tmp_path) is False + + def test_toolchain_check_skipped_without_a_web_package(self, tmp_path, monkeypatch): + """Prebuilt bundles ship no web/ source — they must still skip.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is False + def test_workspace_package_json_edit_defeats_skip(self, tmp_path, monkeypatch): """The manifest list comes from the root package.json `workspaces` globs (npm's source of truth), so ANY workspace (desktop included) diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index b483db76da25..df273054d8e3 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -21,8 +21,11 @@ import pytest from hermes_cli.main import ( _web_ui_build_needed, _build_web_ui, - _run_npm_install_deterministic, _compute_web_ui_content_hash, + _missing_web_build_tool, + _run_npm_install_deterministic, + _web_build_toolchain_ready, + _web_toolchain_roots, _web_ui_stamp_path, _write_web_ui_build_stamp, ) @@ -565,80 +568,79 @@ class TestBuildWebUIFlock: assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") -class TestNpmInstallDevDepEnvForcing: - """Config-level guards so production/omit-dev env can't strip tsc/vite.""" +def _link_shims(bin_dir: Path, *names: str) -> None: + bin_dir.mkdir(parents=True, exist_ok=True) + for name in names: + (bin_dir / name).touch() - def test_forces_npm_config_include_dev_and_clears_omit(self, tmp_path): + +class TestWebBuildToolchainReady: + """A tree is ready when the build can resolve tsc AND vite from any root. + + ``npm run build`` searches ``node_modules/.bin`` from the script's own + package up through every ancestor, so a shim in either place counts. + """ + + def test_missing_toolchain_is_not_ready(self, tmp_path): web_dir, _ = _make_web_dir(tmp_path) - (web_dir / "package-lock.json").write_text("{}", encoding="utf-8") - mock_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - with patch("hermes_cli.main.subprocess.run", return_value=mock_cp) as mock_run: - _run_npm_install_deterministic( - "/usr/bin/npm", - web_dir, - env={ - "NODE_ENV": "production", - "npm_config_omit": "dev", - "npm_config_production": "true", - }, - ) - _, kwargs = mock_run.call_args - env = kwargs["env"] - assert env["npm_config_include"] == "dev" - assert env["npm_config_production"] == "false" - assert "npm_config_omit" not in env - assert env["CI"] == "1" - - -class TestWebToolchainReadyAndRepair: - def test_toolchain_ready_requires_tsc_and_vite_shims(self, tmp_path): - from hermes_cli.main import _web_build_toolchain_ready + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + def test_partial_toolchain_is_not_ready(self, tmp_path): web_dir, _ = _make_web_dir(tmp_path) - root = web_dir.parent - assert _web_build_toolchain_ready(root) is False - bin_dir = root / "node_modules" / ".bin" - bin_dir.mkdir(parents=True) - (bin_dir / "tsc").touch() - assert _web_build_toolchain_ready(root) is False - (bin_dir / "vite").touch() - assert _web_build_toolchain_ready(root) is True - - def test_build_repairs_when_node_modules_exists_without_tsc(self, tmp_path): - """Partial install (node_modules present, no tsc) triggers a repair install.""" - from hermes_cli.main import _resolve_node_runtime_npm + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + def test_hoisted_shims_at_workspace_root_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_nested_under_the_package_are_ready(self, tmp_path): + """#42973 layout: web/ owns its lockfile, so npm links shims there.""" web_dir, _ = _make_web_dir(tmp_path) - (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") - # Simulate a production-omit install: tree exists, toolchain missing. (tmp_path / "node_modules").mkdir() - (tmp_path / "node_modules" / ".bin").mkdir() + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - install_calls = {"n": 0} + def test_shims_split_across_roots_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + _link_shims(web_dir / "node_modules" / ".bin", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - def fake_install(*_a, **_k): - install_calls["n"] += 1 - # After the repair install, pretend tsc/vite appeared. - if install_calls["n"] >= 2: - bin_dir = tmp_path / "node_modules" / ".bin" - (bin_dir / "tsc").touch() - (bin_dir / "vite").touch() - return install_ok + @pytest.mark.parametrize("shim", ["tsc.cmd", "tsc.ps1", "tsc.exe"]) + def test_windows_shim_extensions_count(self, tmp_path, shim): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", shim, "vite.cmd") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ - patch("hermes_cli.main._run_npm_install_deterministic", side_effect=fake_install) as mock_install, \ - patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok), \ - patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ - patch("hermes_cli.main._write_web_ui_build_stamp"): - result = _build_web_ui(web_dir) - assert result is True - # First silent install + one repair install. - assert mock_install.call_count == 2 +class TestWebToolchainRoots: + def test_searches_the_package_and_its_workspace_root(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_toolchain_roots(web_dir) == (web_dir, tmp_path) - def test_build_retries_install_on_tsc_not_found(self, tmp_path): + +class TestMissingWebBuildTool: + """Every shell words an unresolvable binary differently.""" + + @pytest.mark.parametrize( + "output,expected", + [ + ("sh: 1: tsc: not found\nnpm error code 127", "tsc"), + ("bash: line 1: vite: command not found", "vite"), + ("'tsc' is not recognized as an internal or external command", "tsc"), + ("error TS2307: Cannot find module './x'", None), + ("", None), + ], + ) + def test_detects_the_unresolvable_tool(self, output, expected): + assert _missing_web_build_tool(output) == expected + + +class TestBuildRecoversFromMissingToolchain: + def test_reinstalls_and_retries_when_the_build_cannot_resolve_tsc(self, tmp_path): + """The generic retry reruns the same command, so it can't fix this alone.""" web_dir, _ = _make_web_dir(tmp_path) (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") @@ -656,6 +658,44 @@ class TestWebToolchainReadyAndRepair: result = _build_web_ui(web_dir) assert result is True - # Initial install + repair install after tsc: not found. assert mock_install.call_count == 2 assert mock_build.call_count == 2 + + def test_healthy_tree_builds_without_an_extra_install(self, tmp_path): + """No pre-build probing: a build that works is never second-guessed.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 + assert mock_build.call_count == 1 + + def test_unrelated_build_failure_takes_the_generic_retry_only(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + type_error = __import__("subprocess").CompletedProcess( + [], 2, stdout="src/app.tsx(3,1): error TS2307: Cannot find module\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[type_error, build_ok]), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 From 3e4cdee5ab52682196fca598ba3f26a504470a18 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 13:03:33 -0500 Subject: [PATCH 3/3] chore(contributors): map gercamjr for the #68945 salvage --- contributors/emails/gercamjr.dev@gmail.com | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 contributors/emails/gercamjr.dev@gmail.com diff --git a/contributors/emails/gercamjr.dev@gmail.com b/contributors/emails/gercamjr.dev@gmail.com new file mode 100644 index 000000000000..6ac577a3590c --- /dev/null +++ b/contributors/emails/gercamjr.dev@gmail.com @@ -0,0 +1,2 @@ +gercamjr +# PR #68945 salvage (update: recover the web UI build when npm leaves no tsc/vite)