fix(update): don't report success when the Desktop rebuild failed (supersedes #88359, #87984)

hermes_cli/update_cmd.py

@@ -1034,11 +1034,50 @@ def _update_complete_message(pre_version: str | None) -> str:
     return "✓ Update complete!"
 
 
-def _update_via_zip(args, *, had_desktop_app_before_update: bool = False):
+def _print_update_summary(
+    *,
+    node_failures: list,
+    desktop_build_ok: bool,
+    pre_update_version: str | None,
+) -> None:
+    """Final update banner. A failed Desktop rebuild is non-fatal for the
+    Python side, but must not print ``✓ Update complete!`` (#88251)."""
+    print()
+    if node_failures or not desktop_build_ok:
+        parts = []
+        if node_failures:
+            parts.append(
+                f"Node.js dependencies for {', '.join(node_failures)} did not refresh"
+            )
+        if not desktop_build_ok:
+            parts.append(
+                "the desktop app was not rebuilt and is still on the previous build"
+            )
+        print("⚠ Update partially complete — " + "; ".join(parts) + ".")
+        if node_failures:
+            print("  Code and Python deps are updated, but the dashboard/TUI may")
+            print("  be in a mixed state until the Node deps are rebuilt.")
+        if not desktop_build_ok:
+            print("  Run `hermes desktop` to retry the desktop rebuild.")
+    else:
+        _print_update_completion(_update_complete_message(pre_update_version))
+
+
+def _write_gateway_update_exit_code(ok: bool) -> None:
+    path = get_hermes_home() / ".update_exit_code"
+    try:
+        path.write_text("0" if ok else "1", encoding="utf-8")
+    except OSError:
+        pass
+
+
+def _update_via_zip(args, *, had_desktop_app_before_update: bool = False) -> bool:
     """Update Hermes Agent by downloading a ZIP archive.
 
     Used on Windows when git file I/O is broken (antivirus, NTFS filter
     drivers causing 'Invalid argument' errors on file creation).
+
+    Returns ``False`` when a Desktop rebuild ran and failed; ``True`` otherwise.
     """
     active_tool_dependencies = _m()._capture_active_tool_dependencies()
 
@@ -1291,7 +1330,7 @@ def _update_via_zip(args, *, had_desktop_app_before_update: bool = False):
 
     node_failures = _update_node_dependencies()
     _m()._build_web_ui(_m().PROJECT_ROOT / "web")
-    _rebuild_desktop_after_update(
+    desktop_build_ok = _rebuild_desktop_after_update(
         _m().PROJECT_ROOT / "apps" / "desktop",
         had_desktop_app_before_update=had_desktop_app_before_update,
     )
@@ -1396,16 +1435,11 @@ def _update_via_zip(args, *, had_desktop_app_before_update: bool = False):
             "Post-update state.db integrity check (zip path) failed: %s", exc
         )
 
-    print()
-    if node_failures:
-        print(
-            "⚠ Update partially complete — Node.js dependencies for "
-            f"{', '.join(node_failures)} did not refresh."
-        )
-        print("  Code and Python deps are updated, but the dashboard/TUI may")
-        print("  be in a mixed state until the Node deps are rebuilt.")
-    else:
-        _print_update_completion(_update_complete_message(pre_update_version))
+    _print_update_summary(
+        node_failures=node_failures,
+        desktop_build_ok=desktop_build_ok,
+        pre_update_version=pre_update_version,
+    )
     try:
         _print_curator_first_run_notice()
     except Exception as e:
@@ -1417,6 +1451,7 @@ def _update_via_zip(args, *, had_desktop_app_before_update: bool = False):
     # Don't stop a working dashboard when the Node refresh failed — see the
     # git-update path for rationale (#30271).
     _finish_dashboard_update_cleanup(node_failures)
+    return desktop_build_ok
 
 def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[str]:
     status = subprocess.run(
@@ -4535,8 +4570,15 @@ def _desktop_app_present(desktop_dir: Path) -> bool:
 
 def _rebuild_desktop_after_update(
     desktop_dir: Path, *, had_desktop_app_before_update: bool
-) -> None:
-    """Rebuild an installed Desktop app when its source or artifact changed."""
+) -> bool:
+    """Rebuild an installed Desktop app when its source or artifact changed.
+
+    Returns ``False`` only when a rebuild was attempted and failed, so the
+    caller can withhold ``✓ Update complete!`` and (in gateway mode) write
+    a failing ``.update_exit_code`` (#88251). Every other outcome — nothing
+    to rebuild, up to date, build succeeded, Desktop never installed —
+    returns ``True``.
+    """
     # The release tree is ignored by git and can disappear during an update.
     # Its pre-update presence is enough to restore it; do not make people who
     # have never used Desktop pay for an Electron build.
@@ -4546,7 +4588,7 @@ def _rebuild_desktop_after_update(
         and _m()._resolve_node_runtime_npm()
         and has_desktop_app
     ):
-        return
+        return True
 
     print("→ Checking if desktop app needs rebuilding...")
     # Consult the content-hash stamp IN-PROCESS first. The spawned
@@ -4565,7 +4607,7 @@ def _rebuild_desktop_after_update(
         skip_desktop_build = False
     if skip_desktop_build:
         print("  ✓ Desktop app up to date")
-        return
+        return True
 
     desktop_build_cmd = [sys.executable, "-m", "hermes_cli.main", "desktop", "--build-only"]
     # Capture the (very loud) Electron/vite build output into update.log
@@ -4590,15 +4632,16 @@ def _rebuild_desktop_after_update(
             desktop_build_cmd, cwd=_m().PROJECT_ROOT, env=build_env
         )
     if build_result.returncode != 0:
-        print("  ⚠ Desktop build failed (non-fatal; run `hermes desktop` to retry)")
+        print("  ⚠ Desktop build failed (run `hermes desktop` to retry)")
         tail = "\n".join((build_result.stdout or "").strip().splitlines()[-15:])
         if tail:
             print(tail)
         from hermes_constants import display_hermes_home as _dhh
 
         print(f"  Full build log: {_dhh()}/logs/update.log")
-    else:
-        print("  ✓ Desktop app up to date")
+        return False
+    print("  ✓ Desktop app up to date")
+    return True
 
 
 def _cmd_update_impl(args, gateway_mode: bool):
@@ -4815,12 +4858,14 @@ def _cmd_update_impl(args, gateway_mode: bool):
     if use_zip_update:
         # ZIP-based update for Windows when git is broken
         try:
-            _update_via_zip(
+            desktop_build_ok = _update_via_zip(
                 args,
                 had_desktop_app_before_update=had_desktop_app_before_update,
             )
         finally:
             _m()._resume_windows_gateways_after_update(_windows_gateway_resume)
+        if gateway_mode:
+            _write_gateway_update_exit_code(desktop_build_ok)
         return
 
     # Fetch and pull
@@ -5468,7 +5513,7 @@ def _cmd_update_impl(args, gateway_mode: bool):
         node_failures = _update_node_dependencies()
         _m()._build_web_ui(_m().PROJECT_ROOT / "web")
 
-        _rebuild_desktop_after_update(
+        desktop_build_ok = _rebuild_desktop_after_update(
             desktop_dir,
             had_desktop_app_before_update=had_desktop_app_before_update,
         )
@@ -5845,16 +5890,11 @@ def _print_items(items, label, key, fallback_key=None):
             # Never let the cron safety net break an otherwise-good update.
             logger.debug("Cron jobs auto-restore check failed: %s", exc)
 
-        print()
-        if node_failures:
-            print(
-                "⚠ Update partially complete — Node.js dependencies for "
-                f"{', '.join(node_failures)} did not refresh."
-            )
-            print("  Code and Python deps are updated, but the dashboard/TUI may")
-            print("  be in a mixed state until the Node deps are rebuilt.")
-        else:
-            _print_update_completion(_update_complete_message(pre_update_version))
+        _print_update_summary(
+            node_failures=node_failures,
+            desktop_build_ok=desktop_build_ok,
+            pre_update_version=pre_update_version,
+        )
 
         # Search-index optimization notice (v23). Existing installs keep their
         # working search index untouched on update; the compact v23 layout —
@@ -5959,13 +5999,11 @@ def _print_items(items, label, key, fallback_key=None):
         #
         # Writing the marker here — after git pull + pip install succeed but
         # before we attempt the restart — ensures the new gateway sees it
-        # regardless of how we die.
+        # regardless of how we die. Gated on desktop_build_ok (#88251): a
+        # Desktop rebuild failure must not be reported as "0" — the gateway's
+        # /update watcher (gateway/run.py) polls this file.
         if gateway_mode:
-            _exit_code_path = get_hermes_home() / ".update_exit_code"
-            try:
-                _exit_code_path.write_text("0", encoding="utf-8")
-            except OSError:
-                pass
+            _write_gateway_update_exit_code(desktop_build_ok)
 
         gateway_fleet_restart_incomplete = False
         # Snapshot of gateways running before we touch anything. Stays empty
@@ -6790,10 +6828,12 @@ def _on_unit_timeout(svc_name: str, exc: subprocess.TimeoutExpired) -> None:
             print(f"⚠ Git update failed: {e}")
             print("→ Falling back to ZIP download...")
             print()
-            _update_via_zip(
+            desktop_build_ok = _update_via_zip(
                 args,
                 had_desktop_app_before_update=had_desktop_app_before_update,
             )
+            if gateway_mode:
+                _write_gateway_update_exit_code(desktop_build_ok)
         else:
             print(f"✗ Update failed: {e}")
             sys.exit(1)

tests/hermes_cli/test_update_desktop_stale_warning.py

@@ -0,0 +1,168 @@
+"""A failed Desktop pack must not look like a successful update.
+
+#88251: ``hermes update`` treated a failed desktop pack as non-fatal, printed
+an early warning, then still ended with ``✓ Update complete!``. The Python
+side moved on; the Electron app stayed on the previous build.
+
+``_rebuild_desktop_after_update`` returns False only when a rebuild was
+attempted and failed. The final banner then prints ``⚠ Update partially
+complete`` instead of the success line, and gateway mode writes ``1`` to
+``.update_exit_code``.
+"""
+
+import pytest
+
+from hermes_cli import update_cmd
+from hermes_cli.update_cmd import (
+    _print_update_summary,
+    _rebuild_desktop_after_update,
+    _write_gateway_update_exit_code,
+)
+
+
+class _Result:
+    def __init__(self, returncode: int, stdout: str = ""):
+        self.returncode = returncode
+        self.stdout = stdout
+
+
+@pytest.fixture()
+def desktop_env(tmp_path, monkeypatch):
+    """A desktop dir that looks installed and a faked CLI main module."""
+    desktop_dir = tmp_path / "apps" / "desktop"
+    desktop_dir.mkdir(parents=True)
+    (desktop_dir / "package.json").write_text("{}", encoding="utf-8")
+
+    calls = {"builds": 0, "build_needed": True}
+
+    class _FakeMain:
+        PROJECT_ROOT = tmp_path
+
+        @staticmethod
+        def _resolve_node_runtime_npm():
+            return "/fake/npm"
+
+        @staticmethod
+        def _desktop_build_needed(*_a, **_kw):
+            return calls["build_needed"]
+
+        @staticmethod
+        def _run_logged_subprocess(cmd, cwd=None, env=None):
+            calls["builds"] += 1
+            return _Result(1, stdout="Error: [stage-native-deps] boom")
+
+    monkeypatch.setattr(update_cmd, "_m", lambda: _FakeMain)
+    monkeypatch.setattr(
+        "hermes_constants.with_hermes_node_path", lambda: {}, raising=False
+    )
+    monkeypatch.setattr(
+        "hermes_constants.display_hermes_home", lambda: str(tmp_path), raising=False
+    )
+    return desktop_dir, calls
+
+
+def _run(desktop_dir):
+    return _rebuild_desktop_after_update(
+        desktop_dir, had_desktop_app_before_update=True
+    )
+
+
+def test_failed_rebuild_returns_false_and_keeps_the_retry_hint(desktop_env, capsys):
+    desktop_dir, calls = desktop_env
+    assert _run(desktop_dir) is False
+    assert calls["builds"] == 2
+    out = capsys.readouterr().out
+    assert "Desktop build failed" in out
+    assert "stage-native-deps" in out
+    assert "Update complete" not in out
+
+
+def test_successful_rebuild_returns_true(desktop_env, monkeypatch, capsys):
+    desktop_dir, _calls = desktop_env
+    builds = []
+    monkeypatch.setattr(
+        update_cmd._m(),
+        "_run_logged_subprocess",
+        staticmethod(lambda cmd, cwd=None, env=None: builds.append(cmd) or _Result(0)),
+    )
+    assert _run(desktop_dir) is True
+    assert len(builds) == 1
+    assert "Desktop app up to date" in capsys.readouterr().out
+
+
+def test_up_to_date_desktop_returns_true_without_spawning(desktop_env):
+    desktop_dir, calls = desktop_env
+    calls["build_needed"] = False
+    assert _run(desktop_dir) is True
+    assert calls["builds"] == 0
+
+
+def test_desktop_never_installed_returns_true(tmp_path, monkeypatch):
+    spawned = []
+    monkeypatch.setattr(
+        update_cmd,
+        "_m",
+        lambda: type(
+            "_M",
+            (),
+            {
+                "PROJECT_ROOT": tmp_path,
+                "_resolve_node_runtime_npm": staticmethod(lambda: "/fake/npm"),
+                "_run_logged_subprocess": staticmethod(
+                    lambda *a, **k: spawned.append(1) or _Result(0)
+                ),
+            },
+        ),
+    )
+    missing = tmp_path / "apps" / "desktop"
+    missing.mkdir(parents=True)
+    assert _run(missing) is True
+    assert spawned == []
+
+
+def test_summary_omits_success_banner_when_desktop_rebuild_failed(capsys):
+    _print_update_summary(
+        node_failures=[],
+        desktop_build_ok=False,
+        pre_update_version="0.20.1",
+    )
+    out = capsys.readouterr().out
+    assert "Update complete" not in out
+    assert "partially complete" in out
+    assert "desktop app was not rebuilt" in out
+    assert "hermes desktop" in out
+
+
+def test_summary_keeps_success_banner_when_desktop_ok(capsys, monkeypatch):
+    monkeypatch.setattr(
+        update_cmd, "_update_complete_message", lambda _v: "✓ Update complete! (v0.20.2)"
+    )
+    monkeypatch.setattr(update_cmd, "_branch_head_suffix", lambda *a, **k: "")
+    _print_update_summary(
+        node_failures=[],
+        desktop_build_ok=True,
+        pre_update_version="0.20.1",
+    )
+    out = capsys.readouterr().out
+    assert "✓ Update complete!" in out
+    assert "partially complete" not in out
+
+
+def test_summary_combines_node_and_desktop_failures(capsys):
+    _print_update_summary(
+        node_failures=["dashboard"],
+        desktop_build_ok=False,
+        pre_update_version="0.20.1",
+    )
+    out = capsys.readouterr().out
+    assert "Update complete" not in out
+    assert "dashboard" in out
+    assert "desktop app was not rebuilt" in out
+
+
+def test_gateway_exit_code_file_tracks_desktop_rebuild(tmp_path, monkeypatch):
+    monkeypatch.setattr(update_cmd, "get_hermes_home", lambda: tmp_path)
+    _write_gateway_update_exit_code(True)
+    assert (tmp_path / ".update_exit_code").read_text(encoding="utf-8") == "0"
+    _write_gateway_update_exit_code(False)
+    assert (tmp_path / ".update_exit_code").read_text(encoding="utf-8") == "1"