diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 1144400025..64cdb9b377 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -1158,6 +1158,166 @@ def _print_update_completion(message: str) -> None: print(f"=== hermes-update completed {action_id} ===") +def _called_process_error_cmd_parts(exc: subprocess.CalledProcessError) -> list[str]: + """Normalize ``CalledProcessError.cmd`` into argv-style tokens.""" + cmd = exc.cmd + if cmd is None: + return [] + if isinstance(cmd, (str, bytes)): + text = cmd.decode("utf-8", "replace") if isinstance(cmd, bytes) else cmd + try: + return shlex.split(text, posix=os.name != "nt") + except ValueError: + return text.split() + return [str(part) for part in cmd] + + +def _called_process_error_is_git(exc: subprocess.CalledProcessError) -> bool: + """True when the failed subprocess was git itself.""" + parts = _called_process_error_cmd_parts(exc) + if not parts: + return False + # Windows argv may use backslashes; basename() on POSIX would otherwise + # keep the whole path. Normalize separators before taking the name. + name = os.path.basename(parts[0].replace("\\", "/")).lower() + return name in {"git", "git.exe"} + + +def _called_process_error_is_python_dep_install( + exc: subprocess.CalledProcessError, +) -> bool: + """True when the failed subprocess was a uv/pip (or ensurepip) install.""" + parts = [part.lower() for part in _called_process_error_cmd_parts(exc)] + if not parts: + return False + exe = os.path.basename(parts[0].replace("\\", "/")) + if "ensurepip" in parts: + return True + if "install" in parts and ( + "pip" in parts or exe in {"pip", "pip.exe", "pip3", "pip3.exe", "uv", "uv.exe"} + ): + return True + return False + + +def _format_update_failure_stage(exc: subprocess.CalledProcessError) -> str: + """Name the update stage that actually failed. + + The git pull and the Python-dependency install share one ``try`` in + ``_cmd_update_impl``. Calling every ``CalledProcessError`` a git failure + (the historical Windows message) sent users hunting in the wrong place + and, worse, keyed the ZIP overlay on exception *type* rather than on git + actually having failed (#87304, #85840). + """ + if _called_process_error_is_python_dep_install(exc): + return "Python dependency install failed" + if _called_process_error_is_git(exc): + return "Git update failed" + return "Update step failed" + + +def _should_zip_fallback_on_update_error(exc: BaseException) -> bool: + """ZIP fallback is for Windows git file-I/O breakage, not later stages. + + A dependency-install failure (locked ``hermes.exe`` / ``uv pip install`` + exit 2) is not a git failure. The pull has already succeeded by then, so + re-downloading the source ZIP cannot fix the install and would replace + every top-level entry except ``venv`` / ``node_modules`` / ``.git`` / + ``.env`` — permanently deleting uncommitted edits and untracked files. + """ + return ( + isinstance(exc, subprocess.CalledProcessError) + and _m()._is_windows() + and _called_process_error_is_git(exc) + ) + + +def _print_called_process_error_tail( + exc: subprocess.CalledProcessError, *, limit: int = 12 +) -> None: + """Print a captured stderr/stdout tail when the failing call recorded one.""" + blob = exc.stderr or exc.stdout or "" + if isinstance(blob, bytes): + blob = blob.decode("utf-8", "replace") + lines = [line for line in str(blob).splitlines() if line.strip()] + if not lines: + return + print(" Last output:") + for line in lines[-limit:]: + print(f" {line}") + + +def _zip_overlay_block_reason( + root: Path, *, ignore_staging_artifacts: bool = False +) -> Optional[str]: + """Why overlaying a ZIP onto ``root`` would destroy work, or None if safe. + + The ZIP path swaps every top-level entry (except a tiny preserve set) and + then deletes the backups, so uncommitted edits and untracked files under + a replaced directory are gone. Fail closed when git status cannot run: + unknown dirtiness is not a license to clobber the tree (#87304). + + ``ignore_staging_artifacts`` is for the pre-swap re-check: phase 1 of the + two-phase replace creates ``*.hermes-update-staging`` siblings inside the + checkout, which git reports as untracked. Those are our own artifacts, + not user work — without the filter the re-check would always refuse. + """ + if not (root / ".git").exists(): + return None + git_cmd = ["git"] + if sys.platform == "win32": + git_cmd = ["git", "-c", "windows.appendAtomically=false"] + result = subprocess.run( + # -uall: a user-level ``status.showUntrackedFiles = no`` git config + # would otherwise hide untracked files and silently blind this guard. + git_cmd + ["status", "--porcelain", "--untracked-files=all"], + cwd=root, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + ) + if result.returncode != 0: + detail = (result.stderr or result.stdout or "").strip().splitlines() + suffix = f" ({detail[0]})" if detail else "" + return f"could not check the working tree{suffix}" + lines = [line for line in (result.stdout or "").splitlines() if line.strip()] + if ignore_staging_artifacts: + lines = [ + line for line in lines if not _is_zip_staging_artifact_status_line(line) + ] + if lines: + return "the working tree has uncommitted changes or untracked files" + return None + + +_ZIP_STAGING_ARTIFACT_SUFFIXES = (".hermes-update-staging", ".hermes-update-old") + + +def _is_zip_staging_artifact_status_line(line: str) -> bool: + """True when a porcelain status line is our own two-phase-swap artifact.""" + payload = line[3:] if len(line) >= 3 else line + top_level = ( + payload.strip().strip('"').replace("\\", "/").rstrip("/").split("/", 1)[0] + ) + return top_level.endswith(_ZIP_STAGING_ARTIFACT_SUFFIXES) + + +def _abort_zip_update_if_dirty_tree() -> None: + """Refuse to overlay a ZIP onto a dirty git checkout (#87304).""" + reason = _zip_overlay_block_reason(_m().PROJECT_ROOT) + if reason is None: + return + print(f"✗ ZIP fallback refused: {reason}.") + print( + " Overlaying the ZIP would overwrite uncommitted edits and permanently " + "delete untracked files." + ) + print(" Stash or commit your changes, then rerun `hermes update`.") + print(" To inspect: git status --porcelain") + _m().sys.exit(1) + + def _read_project_version() -> str | None: """Read the ``version`` field from the checkout's pyproject.toml. @@ -1268,6 +1428,7 @@ def _update_via_zip(args, *, had_desktop_app_before_update: bool = False) -> boo f"--branch {branch}`, or update against main with `hermes update`." ) _m().sys.exit(1) + _abort_zip_update_if_dirty_tree() zip_url = ( f"https://github.com/NousResearch/hermes-agent/archive/refs/heads/{branch}.zip" ) @@ -1368,6 +1529,23 @@ def _update_via_zip(args, *, had_desktop_app_before_update: bool = False) -> boo raise try: + # Re-check the tree right before the swap (#87304 TOCTOU): the + # download + extract + staging window above can take minutes, and + # work created in it would be destroyed by the commit below. Our + # own phase-1 staging siblings are filtered out — they are the + # expected artifacts of getting here, not user work. + recheck_reason = _zip_overlay_block_reason( + _m().PROJECT_ROOT, ignore_staging_artifacts=True + ) + if recheck_reason is not None: + _discard_staged(staged) + print(f"✗ ZIP fallback aborted before the swap: {recheck_reason}.") + print( + " Files appeared in the checkout while the update was " + "downloading; committing the swap would delete them." + ) + print(" Stash or commit your changes, then rerun `hermes update`.") + _m().sys.exit(1) _commit_staged_replacements(staged) except Exception: # The rollback already restored every swapped entry, but staging @@ -7786,8 +7964,9 @@ def _cmd_update_impl(args, gateway_mode: bool): sys.exit(1) except subprocess.CalledProcessError as e: - if _m()._is_windows(): - print(f"⚠ Git update failed: {e}") + stage = _format_update_failure_stage(e) + if _should_zip_fallback_on_update_error(e): + print(f"⚠ {stage}: {e}") print("→ Falling back to ZIP download...") print() desktop_build_ok = _update_via_zip( @@ -7797,7 +7976,20 @@ def _cmd_update_impl(args, gateway_mode: bool): if gateway_mode: _write_gateway_update_exit_code(desktop_build_ok) else: - print(f"✗ Update failed: {e}") + print(f"✗ {stage}: {e}") + _print_called_process_error_tail(e) + if _called_process_error_is_python_dep_install(e): + print( + " The git update already finished. Re-downloading the source " + "ZIP cannot fix a dependency install error and would overwrite " + "local files." + ) + if _m()._is_windows(): + print(" Retry through the venv interpreter:") + print( + ' venv\\Scripts\\python.exe -c ' + '"from hermes_cli.main import main; main()" update --yes' + ) try: from hermes_cli.update_receipt import finalize_update_receipt diff --git a/tests/hermes_cli/test_update_zip_fallback_guards.py b/tests/hermes_cli/test_update_zip_fallback_guards.py new file mode 100644 index 0000000000..8b6bc426f7 --- /dev/null +++ b/tests/hermes_cli/test_update_zip_fallback_guards.py @@ -0,0 +1,249 @@ +"""ZIP fallback must not fire on dependency failures or clobber a dirty tree. + +Issue #87304: on Windows the update ``try`` spans git pull *and* ``uv pip +install``. A locked ``hermes.exe`` makes the install exit 2, the handler +prints ``Git update failed``, and ``_update_via_zip`` replaces every +top-level entry except ``venv`` / ``node_modules`` / ``.git`` / ``.env`` — +permanently deleting uncommitted edits and untracked files. The git pull +has already succeeded by then, so the ZIP cannot fix the actual failure. +""" + +from __future__ import annotations + +import subprocess +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from hermes_cli import main as hermes_main +from hermes_cli import update_cmd + + +def _cpe(cmd, returncode=2, stderr="", stdout="") -> subprocess.CalledProcessError: + exc = subprocess.CalledProcessError(returncode, cmd) + exc.stderr = stderr + exc.stdout = stdout + return exc + + +# --------------------------------------------------------------------------- +# Stage classification + ZIP gating +# --------------------------------------------------------------------------- + + +def test_uv_pip_install_is_a_dependency_failure_not_git(): + exc = _cpe([r"C:\venv\Scripts\uv.exe", "pip", "install", "-e", "."]) + assert update_cmd._called_process_error_is_git(exc) is False + assert update_cmd._called_process_error_is_python_dep_install(exc) is True + assert update_cmd._format_update_failure_stage(exc) == ( + "Python dependency install failed" + ) + + +def test_venv_pip_install_is_a_dependency_failure(): + exc = _cpe([r"C:\venv\Scripts\python.exe", "-m", "pip", "install", "-e", "."]) + assert update_cmd._called_process_error_is_python_dep_install(exc) is True + assert update_cmd._called_process_error_is_git(exc) is False + + +def test_ensurepip_is_a_dependency_failure(): + exc = _cpe([r"C:\venv\Scripts\python.exe", "-m", "ensurepip", "--upgrade"]) + assert update_cmd._called_process_error_is_python_dep_install(exc) is True + assert update_cmd._format_update_failure_stage(exc) == ( + "Python dependency install failed" + ) + + +def test_git_pull_is_classified_as_git(): + exc = _cpe(["git", "-c", "windows.appendAtomically=false", "pull"], returncode=1) + assert update_cmd._called_process_error_is_git(exc) is True + assert update_cmd._called_process_error_is_python_dep_install(exc) is False + assert update_cmd._format_update_failure_stage(exc) == "Git update failed" + + +def test_git_exe_path_is_still_git(): + exc = _cpe([r"C:\Program Files\Git\cmd\git.exe", "fetch", "origin", "main"]) + assert update_cmd._called_process_error_is_git(exc) is True + + +def test_unknown_command_gets_generic_stage(): + exc = _cpe(["npm", "install"], returncode=1) + assert update_cmd._format_update_failure_stage(exc) == "Update step failed" + + +def test_windows_dep_failure_does_not_zip_fallback(monkeypatch): + monkeypatch.setattr(hermes_main, "_is_windows", lambda: True) + exc = _cpe([r"C:\venv\Scripts\uv.exe", "pip", "install", "-e", "."]) + assert update_cmd._should_zip_fallback_on_update_error(exc) is False + + +def test_windows_git_failure_still_zips(monkeypatch): + monkeypatch.setattr(hermes_main, "_is_windows", lambda: True) + exc = _cpe(["git", "pull"], returncode=1) + assert update_cmd._should_zip_fallback_on_update_error(exc) is True + + +def test_posix_git_failure_does_not_zip(monkeypatch): + monkeypatch.setattr(hermes_main, "_is_windows", lambda: False) + exc = _cpe(["git", "pull"], returncode=1) + assert update_cmd._should_zip_fallback_on_update_error(exc) is False + + +def test_error_tail_prints_last_lines(capsys): + stderr = "\n".join(f"line-{i}" for i in range(20)) + exc = _cpe(["uv", "pip", "install"], stderr=stderr) + update_cmd._print_called_process_error_tail(exc) + out = capsys.readouterr().out + assert "Last output:" in out + assert "line-19" in out + assert "line-0" not in out + assert "line-7" not in out + assert "line-8" in out + + +# --------------------------------------------------------------------------- +# Dirty-tree overlay guard +# --------------------------------------------------------------------------- + + +def _porcelain_run(stdout: str, returncode: int = 0): + def fake_run(cmd, **kwargs): + joined = " ".join(str(c) for c in cmd) + if "status" in joined and "--porcelain" in joined: + return subprocess.CompletedProcess(cmd, returncode, stdout=stdout, stderr="") + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + return fake_run + + +def test_zip_overlay_allowed_without_git(tmp_path): + assert update_cmd._zip_overlay_block_reason(tmp_path) is None + + +def test_zip_overlay_blocked_on_modified_file(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + update_cmd.subprocess, "run", _porcelain_run(" M hermes_cli/update_cmd.py\n") + ) + reason = update_cmd._zip_overlay_block_reason(tmp_path) + assert reason is not None + assert "uncommitted" in reason + + +def test_zip_overlay_blocked_on_untracked_file(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr(update_cmd.subprocess, "run", _porcelain_run("?? notes.md\n")) + reason = update_cmd._zip_overlay_block_reason(tmp_path) + assert reason is not None + assert "untracked" in reason + + +def test_zip_overlay_blocked_when_git_status_fails(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + update_cmd.subprocess, + "run", + _porcelain_run("", returncode=128), + ) + reason = update_cmd._zip_overlay_block_reason(tmp_path) + assert reason is not None + assert "could not check" in reason + + +def test_zip_overlay_allowed_on_clean_git_checkout(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr(update_cmd.subprocess, "run", _porcelain_run("")) + assert update_cmd._zip_overlay_block_reason(tmp_path) is None + + +def test_update_via_zip_aborts_before_download_when_dirty( + tmp_path, monkeypatch, capsys +): + """The live tree must not be touched, and the ZIP must not be fetched.""" + fake_root = tmp_path / "install" + fake_root.mkdir() + (fake_root / ".git").mkdir() + local = fake_root / "keep-me.txt" + local.write_text("local work\n", encoding="utf-8") + untracked_dir = fake_root / "agent" / "scratch" + untracked_dir.mkdir(parents=True) + (untracked_dir / "wip.py").write_text("print('wip')\n", encoding="utf-8") + + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", fake_root) + monkeypatch.setattr( + update_cmd.subprocess, + "run", + _porcelain_run(" M keep-me.txt\n?? agent/scratch/wip.py\n"), + ) + + with patch("urllib.request.urlretrieve") as download: + with pytest.raises(SystemExit) as exc_info: + hermes_main._update_via_zip(SimpleNamespace(branch=None)) + + assert exc_info.value.code == 1 + download.assert_not_called() + assert local.read_text(encoding="utf-8") == "local work\n" + assert (untracked_dir / "wip.py").read_text(encoding="utf-8") == "print('wip')\n" + out = capsys.readouterr().out + assert "ZIP fallback refused" in out + assert "Downloading latest version" not in out + + +# --------------------------------------------------------------------------- +# Pre-swap TOCTOU re-check +# --------------------------------------------------------------------------- + + +def test_status_uses_untracked_files_all(tmp_path, monkeypatch): + """A user git config hiding untracked files must not blind the guard.""" + (tmp_path / ".git").mkdir() + seen = [] + + def fake_run(cmd, **kwargs): + seen.append(cmd) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + monkeypatch.setattr(update_cmd.subprocess, "run", fake_run) + update_cmd._zip_overlay_block_reason(tmp_path) + assert seen and "--untracked-files=all" in seen[0] + + +def test_staging_artifact_lines_are_recognized(): + is_artifact = update_cmd._is_zip_staging_artifact_status_line + assert is_artifact("?? agent.hermes-update-staging/") + assert is_artifact("?? cli.py.hermes-update-staging") + assert is_artifact("?? tools.hermes-update-old/") + # Nested user files under a staging-lookalike directory don't match the + # top-level test only when the TOP level itself is not an artifact. + assert not is_artifact("?? agent/scratch/wip.py") + assert not is_artifact(" M hermes_cli/update_cmd.py") + assert not is_artifact("?? notes.hermes-update-staging.txt") + + +def test_recheck_ignores_own_staging_artifacts(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + update_cmd.subprocess, + "run", + _porcelain_run("?? agent.hermes-update-staging/\n?? cli.py.hermes-update-old\n"), + ) + assert ( + update_cmd._zip_overlay_block_reason(tmp_path, ignore_staging_artifacts=True) + is None + ) + # Without the flag the same output still refuses (pre-download check). + assert update_cmd._zip_overlay_block_reason(tmp_path) is not None + + +def test_recheck_still_blocks_user_files_amid_staging_artifacts(tmp_path, monkeypatch): + (tmp_path / ".git").mkdir() + monkeypatch.setattr( + update_cmd.subprocess, + "run", + _porcelain_run("?? agent.hermes-update-staging/\n?? my-notes.md\n"), + ) + reason = update_cmd._zip_overlay_block_reason( + tmp_path, ignore_staging_artifacts=True + ) + assert reason is not None diff --git a/tests/hermes_cli/test_update_zip_symlink_reject.py b/tests/hermes_cli/test_update_zip_symlink_reject.py index 4ee7f84549..72359bcd54 100644 --- a/tests/hermes_cli/test_update_zip_symlink_reject.py +++ b/tests/hermes_cli/test_update_zip_symlink_reject.py @@ -41,8 +41,14 @@ def test_update_via_zip_rejects_symlink_member(tmp_path, monkeypatch): target="/etc/passwd", ) + fake_root = tmp_path / "install_dir" + fake_root.mkdir() + + from hermes_cli import main as hermes_main from hermes_cli.main import _update_via_zip + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", fake_root) + args = type("Args", (), {})() # Patch urlretrieve to "download" our pre-built malicious ZIP into the