From 83314ca381653ca66d0510499c81271171a7b022 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:18:55 +0530 Subject: [PATCH] fix(update): make the ZIP replace atomic across all entries + dedupe venv layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #76104, closes #76105. #76104 — `_atomic_replace_dir` (#49145) made each individual directory swap safe, but `_update_via_zip` replaced ~70 top-level entries in a loop with no atomicity across iterations. `agent/` lands at os.listdir index 13 and `tools/` at 66, so an interruption between them left the new `agent/context_compressor.py` (module-level `from tools.todo_tool import TODO_INJECTION_HEADER`) beside a stale `tools/todo_tool.py` — every file valid Python, the tree unbootable. That is the mechanism behind the ImportError fixed in #76091, and the "partial update" field report in #63717. Split into stage-all-then-swap-all: - `_stage_replacement` copies each dir to a sibling staging path, touching nothing live, so a failure during the long copy phase is a no-op. - `_commit_staged_replacements` performs the renames and, if any fails, restores every entry already swapped — the tree lands wholly new or wholly old, never mixed. This shrinks the failure window from a full tree copy to N renames and makes what remains recoverable. Added an up-front free-space check, since staging needs a second copy of the tree; a clear error beats running out mid-swap. #76105 — venv interpreter resolution was open-coded in 7 places across 4 files using 3 different Windows predicates. #76091 added the seventh because the correct behaviour lived 2400 lines away. Hoisted `venv_bin_dir()` / `venv_python_path()` into hermes_constants (import-safe, no new imports) and routed every site through them; `managed_uv._venv_python` now delegates so its 6 callers are untouched. `_atomic_replace_dir` is retained — it is re-exported from main.py and has its own #49145 regression test; removing it is out of scope here. Tests: 10 new (rollback-on-mid-swap-failure is mutation-verified — it fails when the rollback loop is removed), plus a guard that fails if a new call site hand-rolls Scripts/bin again. E2E-verified against the real staging + commit helpers with a live tree. --- hermes_cli/gateway.py | 7 +- hermes_cli/main.py | 9 +- hermes_cli/managed_uv.py | 6 +- hermes_cli/update_cmd.py | 117 ++++++++++-- hermes_constants.py | 24 +++ tests/hermes_cli/test_update_zip_two_phase.py | 178 ++++++++++++++++++ 6 files changed, 312 insertions(+), 29 deletions(-) create mode 100644 tests/hermes_cli/test_update_zip_two_phase.py diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 55b8a196f14c8..cb79e47ea0514 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -2525,10 +2525,9 @@ def _detect_venv_dir() -> Path | None: def get_python_path() -> str: venv = _detect_venv_dir() if venv is not None: - if is_windows(): - venv_python = venv / "Scripts" / "python.exe" - else: - venv_python = venv / "bin" / "python" + from hermes_constants import venv_python_path + + venv_python = venv_python_path(venv) if venv_python.exists(): return str(venv_python) return sys.executable diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 614ef37cec9c2..46ecba0276887 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7963,7 +7963,9 @@ def _venv_scripts_dir() -> Path | None: venv_dir = PROJECT_ROOT / "venv" if not venv_dir.is_dir(): return None - scripts = venv_dir / ("Scripts" if _is_windows() else "bin") + from hermes_constants import venv_bin_dir + + scripts = venv_bin_dir(venv_dir) return scripts if scripts.is_dir() else None @@ -8756,9 +8758,10 @@ def _resolve_install_target_python( ``importlib.metadata`` queries the right site-packages. """ if env and "VIRTUAL_ENV" in env: + from hermes_constants import venv_python_path + venv_root = Path(env["VIRTUAL_ENV"]) - scripts = venv_root / ("Scripts" if _is_windows() else "bin") - candidate = scripts / ("python.exe" if _is_windows() else "python") + candidate = venv_python_path(venv_root) if candidate.exists(): return candidate diff --git a/hermes_cli/managed_uv.py b/hermes_cli/managed_uv.py index 364c2f169fe03..e7821e79b5b1e 100644 --- a/hermes_cli/managed_uv.py +++ b/hermes_cli/managed_uv.py @@ -384,9 +384,9 @@ def update_managed_uv( def _venv_python(venv_dir: Path) -> Path: - if platform.system() == "Windows": - return venv_dir / "Scripts" / "python.exe" - return venv_dir / "bin" / "python" + from hermes_constants import venv_python_path + + return venv_python_path(venv_dir) def _remove_tree(path: Path, *, boundary: Path) -> None: diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index bf0415ce0b003..6a55aad8227c1 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -37,6 +37,7 @@ from pathlib import Path from typing import Optional from hermes_cli.config import get_hermes_home +from hermes_constants import venv_python_path logger = logging.getLogger(__name__) @@ -222,9 +223,7 @@ def _validate_critical_modules_import(root) -> tuple[bool, str | None, str | Non try: interpreter = sys.executable try: - bin_dir = "Scripts" if _m()._is_windows() else "bin" - python_name = "python.exe" if _m()._is_windows() else "python" - venv_python = Path(root) / "venv" / bin_dir / python_name + venv_python = venv_python_path(Path(root) / "venv") if venv_python.exists(): interpreter = str(venv_python) except Exception: @@ -622,6 +621,64 @@ def _atomic_replace_dir(src: str, dst: str) -> None: if os.path.exists(backup): shutil.rmtree(backup, ignore_errors=True) + +def _stage_replacement(src: str, dst: str) -> str: + """Copy *src* to a sibling staging dir for *dst*; return the staging path. + + Phase 1 of the two-phase replace. Touches nothing live, so a failure here + leaves the whole install untouched. + """ + staging = f"{dst}.hermes-update-staging" + backup = f"{dst}.hermes-update-old" + for leftover in (staging, backup): + if os.path.exists(leftover): + shutil.rmtree(leftover, ignore_errors=True) + shutil.copytree(src, staging) + return staging + + +def _commit_staged_replacements(staged: list[tuple[str, str]]) -> None: + """Phase 2: swap every staged dir into place, rolling back all on failure. + + ``_atomic_replace_dir`` makes each *individual* directory swap safe, but + the ZIP update replaces ~70 top-level entries in a loop, and nothing made + the loop atomic *as a whole*. A failure partway left some entries at the + new version and the rest at the old one — every file valid Python, the + combination unbootable (issue #76104; the ``ImportError`` in #76091 and + the field report in #63717 are both this). + + Splitting stage-all-then-swap-all shrinks the failure window from "the + duration of a full tree copy" to "the duration of N renames", and makes + the remaining window recoverable: if a rename fails we restore every + entry already swapped, so the tree lands wholly new or wholly old. + """ + swapped: list[tuple[str, str]] = [] # (dst, backup) in swap order + try: + for staging, dst in staged: + backup = f"{dst}.hermes-update-old" + if os.path.exists(dst): + os.rename(dst, backup) + swapped.append((dst, backup)) + else: + swapped.append((dst, "")) + os.rename(staging, dst) + except OSError: + # Undo every swap already made so the install stays self-consistent. + for dst, backup in reversed(swapped): + try: + if os.path.exists(dst): + shutil.rmtree(dst, ignore_errors=True) + if backup and os.path.exists(backup): + os.rename(backup, dst) + except OSError: + pass # best-effort; the raise below reports the real failure + raise + # All swaps succeeded — drop the backups (best-effort, never fatal). + for _dst, backup in swapped: + if backup and os.path.exists(backup): + shutil.rmtree(backup, ignore_errors=True) + + def _update_via_zip(args): """Update Hermes Agent by downloading a ZIP archive. @@ -700,19 +757,46 @@ def _update_via_zip(args): # Copy updated files over existing installation, preserving venv/node_modules/.git preserve = {"venv", "node_modules", ".git", ".env"} - update_count = 0 - for item in os.listdir(extracted): - if item in preserve: - continue + entries = [i for i in os.listdir(extracted) if i not in preserve] + + # Two-phase replace (#76104). Phase 1 copies every directory into a + # sibling staging dir without touching anything live; phase 2 swaps + # them all in with same-filesystem renames and rolls back every swap + # if any one fails. Replacing entries one-at-a-time (the previous + # shape) meant an interruption partway left `agent/` new and `tools/` + # stale — all files valid, the tree unbootable. + # + # Staging costs a second copy of the tree on disk. Check up front so + # we fail with a clear message instead of running out mid-swap. + need = sum( + os.path.getsize(os.path.join(dirpath, f)) + for entry in entries + for dirpath, _dirs, files in os.walk(os.path.join(extracted, entry)) + for f in files + if os.path.isfile(os.path.join(dirpath, f)) + ) + free = shutil.disk_usage(str(_m().PROJECT_ROOT)).free + if free < need * 2: + raise RuntimeError( + f"not enough free disk space to stage the update safely " + f"(need ~{need * 2 // (1024 * 1024)} MB, have " + f"{free // (1024 * 1024)} MB)" + ) + + staged: list[tuple[str, str]] = [] + plain_files: list[tuple[str, str]] = [] + for item in entries: src = os.path.join(extracted, item) dst = os.path.join(str(_m().PROJECT_ROOT), item) if os.path.isdir(src): - # Atomic-ish replace: never leave dst half-deleted if the copy - # fails partway (the failure mode behind #49145 on Windows). - _atomic_replace_dir(src, dst) + staged.append((_stage_replacement(src, dst), dst)) else: - shutil.copy2(src, dst) - update_count += 1 + plain_files.append((src, dst)) + + _commit_staged_replacements(staged) + for src, dst in plain_files: + shutil.copy2(src, dst) + update_count = len(staged) + len(plain_files) print(f"✓ Updated {update_count} items from ZIP") @@ -2636,9 +2720,7 @@ def _venv_core_imports_healthy() -> tuple[bool, str]: healthy so a probe failure can't force needless reinstalls. """ venv_dir = _m().PROJECT_ROOT / "venv" - python_name = "python.exe" if _m()._is_windows() else "python" - bin_dir = "Scripts" if _m()._is_windows() else "bin" - venv_python = venv_dir / bin_dir / python_name + venv_python = venv_python_path(venv_dir) if not venv_python.exists(): # No venv interpreter at all. In a dev checkout that's normal (the # dev may run hermes from any interpreter), so report healthy to @@ -3724,10 +3806,7 @@ def _cmd_update_impl(args, gateway_mode: bool): # repair after the old venv was moved aside) needs the venv # recreated before dependencies can be installed into it. venv_python_missing = not ( - _m().PROJECT_ROOT - / "venv" - / ("Scripts" if _m()._is_windows() else "bin") - / ("python.exe" if _m()._is_windows() else "python") + venv_python_path(_m().PROJECT_ROOT / "venv") ).exists() if venv_python_missing and repair_uv: print("→ Recreating virtual environment...") diff --git a/hermes_constants.py b/hermes_constants.py index 9ad478f9e2736..471941a88e493 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -1250,6 +1250,30 @@ OPENROUTER_MODELS_URL = f"{OPENROUTER_BASE_URL}/models" AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1" +# ─── Venv layout ───────────────────────────────────────────────────────────── + +def venv_bin_dir(venv_dir) -> Path: + """Directory holding a venv's executables (``Scripts`` / ``bin``). + + Single source of truth for venv layout. This was open-coded in seven + places across four files using three different Windows predicates + (``platform.system()``, ``is_windows()``, ``_is_windows()``); each new + call site had to re-derive it, and #76091 shipped an eighth copy because + the correct behaviour lived 2400 lines away in another function. + + The path is returned unconditionally — callers legitimately differ on + whether a missing venv is an error, so existence checking stays with them. + """ + return Path(venv_dir) / ("Scripts" if sys.platform == "win32" else "bin") + + +def venv_python_path(venv_dir) -> Path: + """Path to the Python interpreter inside *venv_dir* (may not exist).""" + return venv_bin_dir(venv_dir) / ( + "python.exe" if sys.platform == "win32" else "python" + ) + + # ─── Partial-update diagnostics ────────────────────────────────────────────── # Top-level packages/modules that ship as part of Hermes itself. An ImportError diff --git a/tests/hermes_cli/test_update_zip_two_phase.py b/tests/hermes_cli/test_update_zip_two_phase.py new file mode 100644 index 0000000000000..9f6caed0f2e4c --- /dev/null +++ b/tests/hermes_cli/test_update_zip_two_phase.py @@ -0,0 +1,178 @@ +"""Tests for the two-phase ZIP replace and the shared venv-layout helpers. + +``_atomic_replace_dir`` (#49145) made each *individual* directory swap safe, +but the ZIP update replaced ~70 top-level entries in a loop with no atomicity +across iterations. An interruption partway left some entries at the new +version and the rest at the old one -- every file valid Python, the +combination unbootable. That is the mechanism behind the ``ImportError`` in +#76091 and the field report in #63717. + +Reference: issues #76104 (ZIP atomicity) and #76105 (venv-helper duplication). +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from hermes_cli import update_cmd +from hermes_constants import venv_bin_dir, venv_python_path + + +# --------------------------------------------------------------------------- +# Two-phase replace +# --------------------------------------------------------------------------- + +def _live_tree(root: Path, names: dict[str, str]) -> None: + for name, marker in names.items(): + d = root / name + d.mkdir(parents=True, exist_ok=True) + (d / "version.txt").write_text(marker) + + +def _stage_all(root: Path, new: Path, names: list[str]) -> list[tuple[str, str]]: + return [ + ( + update_cmd._stage_replacement(str(new / n), str(root / n)), + str(root / n), + ) + for n in names + ] + + +def test_staging_touches_nothing_live(tmp_path): + """Phase 1 must not modify the install -- a failure there is a no-op.""" + live, new = tmp_path / "live", tmp_path / "new" + _live_tree(live, {"agent": "old", "tools": "old"}) + _live_tree(new, {"agent": "new", "tools": "new"}) + + _stage_all(live, new, ["agent", "tools"]) + + assert (live / "agent" / "version.txt").read_text() == "old" + assert (live / "tools" / "version.txt").read_text() == "old" + + +def test_commit_swaps_every_entry(tmp_path): + live, new = tmp_path / "live", tmp_path / "new" + _live_tree(live, {"agent": "old", "tools": "old"}) + _live_tree(new, {"agent": "new", "tools": "new"}) + + update_cmd._commit_staged_replacements(_stage_all(live, new, ["agent", "tools"])) + + assert (live / "agent" / "version.txt").read_text() == "new" + assert (live / "tools" / "version.txt").read_text() == "new" + # No staging/backup litter left behind. + assert not [p for p in os.listdir(live) if "hermes-update" in p] + + +def test_failed_swap_rolls_back_every_earlier_swap(tmp_path, monkeypatch): + """The regression: a mid-loop failure must not leave a mixed-version tree. + + Before the two-phase split this produced `agent/` new + `tools/` stale -- + the exact shape that yields + `ImportError: cannot import name 'TODO_INJECTION_HEADER'`. + """ + live, new = tmp_path / "live", tmp_path / "new" + _live_tree(live, {"agent": "old", "tools": "old"}) + _live_tree(new, {"agent": "new", "tools": "new"}) + staged = _stage_all(live, new, ["agent", "tools"]) + + real_rename = os.rename + calls = {"n": 0} + + def flaky_rename(src, dst): + calls["n"] += 1 + # Let the first entry swap fully (2 renames), then break the second. + if calls["n"] == 4: + raise OSError("simulated AV interference") + return real_rename(src, dst) + + monkeypatch.setattr(update_cmd.os, "rename", flaky_rename) + + with pytest.raises(OSError): + update_cmd._commit_staged_replacements(staged) + + monkeypatch.undo() + # Both entries must be back at the OLD version -- not one new, one old. + versions = { + n: (live / n / "version.txt").read_text() for n in ("agent", "tools") + } + assert versions == {"agent": "old", "tools": "old"}, ( + f"mixed-version tree after rollback: {versions}" + ) + + +def test_commit_handles_entries_absent_from_the_install(tmp_path): + """A brand-new top-level dir has no live counterpart to move aside.""" + live, new = tmp_path / "live", tmp_path / "new" + live.mkdir() + _live_tree(new, {"brand_new": "new"}) + + update_cmd._commit_staged_replacements(_stage_all(live, new, ["brand_new"])) + + assert (live / "brand_new" / "version.txt").read_text() == "new" + + +def test_staging_clears_leftovers_from_an_interrupted_run(tmp_path): + live, new = tmp_path / "live", tmp_path / "new" + _live_tree(live, {"agent": "old"}) + _live_tree(new, {"agent": "new"}) + stale = Path(f"{live / 'agent'}.hermes-update-staging") + stale.mkdir() + (stale / "junk.txt").write_text("from a previous crash") + + update_cmd._commit_staged_replacements(_stage_all(live, new, ["agent"])) + + assert (live / "agent" / "version.txt").read_text() == "new" + assert not (live / "agent" / "junk.txt").exists() + + +# --------------------------------------------------------------------------- +# Shared venv helpers (#76105) +# --------------------------------------------------------------------------- + +def test_venv_helpers_agree_with_each_other(): + v = Path("/opt/proj/venv") + assert venv_python_path(v).parent == venv_bin_dir(v) + + +def test_venv_helpers_accept_str_and_path(): + assert venv_python_path("/opt/x/venv") == venv_python_path(Path("/opt/x/venv")) + + +def test_venv_helpers_are_platform_consistent(): + """Whatever the platform, the two halves must not disagree.""" + v = Path("/opt/proj/venv") + bin_name = venv_bin_dir(v).name + exe_name = venv_python_path(v).name + assert (bin_name, exe_name) in {("Scripts", "python.exe"), ("bin", "python")} + + +def test_managed_uv_helper_delegates_to_the_shared_one(): + from hermes_cli.managed_uv import _venv_python + + v = Path("/opt/proj/venv") + assert _venv_python(v) == venv_python_path(v) + + +def test_no_open_coded_venv_layout_remains_in_hermes_cli(): + """Fails if a new call site hand-rolls Scripts/bin again (#76105).""" + import hermes_cli + + pkg = Path(hermes_cli.__file__).parent + offenders = [] + for py in pkg.rglob("*.py"): + for lineno, line in enumerate( + py.read_text(encoding="utf-8", errors="replace").splitlines(), 1 + ): + if '"Scripts"' not in line: + continue + # Comments and docstrings referencing the path are fine. + stripped = line.strip() + if stripped.startswith("#") or stripped.startswith(("'", '"', "-")): + continue + if "if" in line or "/" in line: + offenders.append(f"{py.relative_to(pkg)}:{lineno}: {stripped}") + assert not offenders, "open-coded venv layout found:\n" + "\n".join(offenders)