From 266b2b361147dc3d9f770a3c63b5c4a71c5f67d5 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 13 Aug 2026 13:42:42 -0500 Subject: [PATCH] fix(update): repair failed Node deps on an already-current checkout (#85539) A failed npm install during `hermes update` prints "Fix npm and re-run `hermes update`" -- but re-running on a current checkout hit the "Already up to date!" early return before the Node refresh, so the repair advice could never work and node_modules stayed stale forever (#77211). The commit_count == 0 path now runs the Node refresh through _repair_node_deps_on_current_checkout. _update_node_dependencies self-gates on the lockfile hash, which is only recorded after a SUCCESSFUL npm install (and re-trips when node_modules is missing or the web toolchain never landed), so healthy installs pay one hash check and nothing else; a previously failed install actually repairs. A clean refresh pairs with the web build like every other call site; a failed one surfaces the fix-npm hint instead of "Already up to date!". Fixes #77211. Co-authored-by: RelaxJonh Co-authored-by: JonthanaHanh --- hermes_cli/update_cmd.py | 30 ++++++++++++- .../test_update_current_node_repair.py | 44 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 tests/hermes_cli/test_update_current_node_repair.py diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 20cd33e5c61bd..a139e52b66cd6 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2102,6 +2102,34 @@ def _record_npm_lockfile_hash(hermes_root: Path) -> None: except OSError: logger.debug("Could not write npm lockfile hash cache") +def _repair_node_deps_on_current_checkout(print_completion) -> None: + """Repair Node deps on the ``commit_count == 0`` path (#77211). + + A current checkout does not imply healthy Node deps: a previous npm + install may have failed (EBADENGINE from a node/npm mismatch, network + timeout, interrupted install) and its error message says to "re-run + hermes update" — but the early return never reached the Node refresh, + so that repair advice could never work. ``_update_node_dependencies`` + self-gates on the lockfile hash, which is only recorded after a + SUCCESSFUL npm install (and re-trips when node_modules is missing or + the web toolchain never landed), so this is a cheap no-op on healthy + installs and a real repair after a failed one. + """ + node_failures = _update_node_dependencies() + if node_failures: + print(f" ⚠ Node.js refresh failed for: {', '.join(node_failures)}") + print(" Fix npm and re-run `hermes update`.") + print_completion( + "⚠ Checkout is current, but Node.js dependencies could not be repaired." + ) + return + # Pair the refresh with the web build like every other + # _update_node_dependencies call site; it staleness-checks internally, + # so this is a no-op when nothing changed. + _m()._build_web_ui(_m().PROJECT_ROOT / "web") + print_completion("✓ Already up to date!") + + def _update_node_dependencies() -> list[str]: """Refresh Node deps for the ui-tui and web workspaces. @@ -4228,7 +4256,7 @@ def _cmd_update_impl(args, gateway_mode: bool): print(f"⚠ Venv still unhealthy after repair: {detail_after}") print(" Close all Hermes windows/gateways and re-run: hermes update") else: - _print_update_completion("✓ Already up to date!") + _repair_node_deps_on_current_checkout(_print_update_completion) if runtime_repaired is not None and not _m()._is_windows(): print() print( diff --git a/tests/hermes_cli/test_update_current_node_repair.py b/tests/hermes_cli/test_update_current_node_repair.py new file mode 100644 index 0000000000000..3af7ce62790bc --- /dev/null +++ b/tests/hermes_cli/test_update_current_node_repair.py @@ -0,0 +1,44 @@ +"""The commit_count == 0 path must repair Node deps, not just Python (#77211). + +A previous ``hermes update`` whose npm install failed printed "Fix npm and +re-run `hermes update`" — but re-running hit the "Already up to date!" early +return before the Node refresh, so the advice could never work. The repair +now runs through ``_repair_node_deps_on_current_checkout``, which delegates +to ``_update_node_dependencies`` (self-gating on the lockfile hash, recorded +only after a successful install, so healthy installs stay a cheap no-op). +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from hermes_cli import update_cmd + + +def test_current_checkout_repairs_failed_node_deps(capsys): + """A recorded failure surfaces the fix-npm hint, not 'Already up to date!'.""" + completion = MagicMock() + with patch.object( + update_cmd, "_update_node_dependencies", return_value=["ui-tui, web workspaces"] + ), patch.object(update_cmd, "_m") as m: + update_cmd._repair_node_deps_on_current_checkout(completion) + + m.return_value._build_web_ui.assert_not_called() + completion.assert_called_once() + assert "could not be repaired" in completion.call_args[0][0] + out = capsys.readouterr().out + assert "Node.js refresh failed for: ui-tui, web workspaces" in out + assert "Fix npm and re-run `hermes update`." in out + + +def test_current_checkout_healthy_node_deps_reports_up_to_date(): + """A clean refresh (or lockfile-hash no-op) still says 'Already up to date!'.""" + completion = MagicMock() + with patch.object( + update_cmd, "_update_node_dependencies", return_value=[] + ), patch.object(update_cmd, "_m") as m: + update_cmd._repair_node_deps_on_current_checkout(completion) + + # The refresh pairs with the web build like every other call site. + m.return_value._build_web_ui.assert_called_once() + completion.assert_called_once_with("✓ Already up to date!")