Merge pull request #72794 from NousResearch/bb/web-build-toolchain

fix(update): recover the web UI build when npm leaves no tsc/vite

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)

hermes_cli/main.py

@@ -5226,6 +5226,62 @@ def _run_npm_install_deterministic(
     )
 
 
+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(*roots: Path) -> bool:
+    """True when ``tsc`` and ``vite`` shims are reachable from any of *roots*.
+
+    Callers must pass every root the build would search; checking only one
+    reports a healthy tree as broken.
+    """
+    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 _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:
     """Build the web UI frontend if npm is available, serializing across processes.
 
@@ -5333,12 +5389,16 @@ 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":
+        return _run_npm_install_deterministic(
+            npm,
+            npm_cwd,
+            extra_args=(*npm_workspace_args, "--silent") if silent else npm_workspace_args,
+            env=build_env,
+        )
+
+    r1 = _install_web_deps(silent=True)
     if r1.returncode != 0:
         _say(
             f"  {'✗' if fatal else '⚠'} Web UI npm install failed"
@@ -5355,11 +5415,22 @@ 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)
+        # 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)
+            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 +10211,15 @@ 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 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.
         cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12]

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)

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,
 )
@@ -563,3 +566,136 @@ 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")
+
+
+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()
+
+
+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)
+        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)
+        _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 / "node_modules").mkdir()
+        _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite")
+        assert _web_build_toolchain_ready(web_dir, tmp_path) is True
+
+    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
+
+    @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
+
+
+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)
+
+
+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="")
+        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
+        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