From 0723cb6c06a5a255f860c38eeb24bb9db67cb192 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 20 Aug 2026 12:41:33 -0500 Subject: [PATCH] fix(update): don't reinstall the editable package when the pull can't affect it (#90967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `uv pip install -e .` never audits an editable target. It reinstalls on every invocation and rewrites the console-script shims each time, which is the only reason `hermes update` has to quarantine the running `hermes.exe` on Windows — and a quarantine that loses its race is the whole `os error 32` family. Gate the reinstall on whether the pull actually touched a file that defines the install. It's safe to skip because the editable finder is pinned to a static module list (`py-modules` + `packages.find.include`), so the one source-only change that could stale it — a new top-level module or package — cannot land without a `pyproject.toml` diff. Dependencies and `[project.scripts]` live there too, and new submodules inside an already-mapped package resolve through the real directory. The predicate fails closed: no pre-pull SHA, an unresolvable one, or a failed `git diff` all reinstall as before. On the skip path the two verifiers that normally run inside the install run directly, so a wrong skip self-heals into a real install rather than leaving an unchecked venv. This is the pattern the file already uses everywhere else — `_tui_need_npm_install` diffs node_modules against package-lock.json, and the desktop build is gated on a content hash so `hermes update` "will skip if nothing actually changed". The Python editable install was the one path with no such gate. --- hermes_cli/update_cmd.py | 89 +++++++++++++-- ..._update_skip_unchanged_editable_install.py | 108 ++++++++++++++++++ 2 files changed, 186 insertions(+), 11 deletions(-) create mode 100644 tests/hermes_cli/test_update_skip_unchanged_editable_install.py diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 2f2c984569..1b8c967dd7 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -258,6 +258,55 @@ def _capture_head_sha(git_cmd, cwd) -> str | None: except (subprocess.CalledProcessError, OSError): return None +# Files that define the editable install. A pull that touches none of them +# cannot have invalidated it. +_INSTALL_DEFINING_FILES = ( + "pyproject.toml", + "setup.py", + "setup.cfg", + "MANIFEST.in", + "uv.lock", +) + +def _editable_install_is_current(git_cmd, cwd, pre_pull_sha: str | None) -> bool: + """True when the pulled commits cannot have invalidated the editable install. + + ``uv pip install -e .`` never audits an editable target — it reinstalls on + every invocation, and every reinstall rewrites the console-script shims. + On Windows that rewrite is the only reason the running ``hermes.exe`` has + to be quarantined, and a quarantine that loses its race is the whole + ``os error 32`` family. Not reinstalling when the reinstall provably + cannot change anything removes that risk outright for the common update, + rather than trying to make the rename win more often. + + Skipping is safe because Hermes pins its editable finder to a *static* + module list (``[tool.setuptools] py-modules`` plus + ``packages.find.include``). The one source-only change that would stale + that finder is a new top-level module or package, and it cannot land + without a ``pyproject.toml`` diff. Dependencies and ``[project.scripts]`` + live there too. New submodules inside an already-mapped package resolve + through the real package directory and need no reinstall. + + Fails closed: an unresolvable pre-pull SHA (shallow checkout, ZIP swap) + or a failed ``git diff`` returns False and the install runs as before. + """ + if not pre_pull_sha: + return False + try: + result = subprocess.run( + git_cmd + + ["diff", "--name-only", f"{pre_pull_sha}..HEAD", "--"] + + list(_INSTALL_DEFINING_FILES), + cwd=cwd, + capture_output=True, + text=True, encoding="utf-8", errors="replace", + ) + except OSError: + return False + if result.returncode != 0: + return False + return not result.stdout.strip() + def _validate_critical_files_syntax(root) -> tuple[bool, str | None, str | None]: """Compile each file in ``_UPDATE_CRITICAL_FILES`` to catch SyntaxErrors. @@ -5698,7 +5747,13 @@ def _cmd_update_impl(args, gateway_mode: bool): # via ``_recover_from_interrupted_install``. Cleared after the core # ``.[all]`` install completes — lazy refresh uses a separate marker. _write_update_incomplete_marker() - print("→ Updating Python dependencies...") + deps_current = _editable_install_is_current( + git_cmd, _m().PROJECT_ROOT, pre_pull_sha + ) + if deps_current: + print("→ Python dependencies unchanged — skipping reinstall") + else: + print("→ Updating Python dependencies...") from hermes_cli.managed_uv import ensure_uv, update_managed_uv # Keep managed uv current — runs `uv self update` if we already have one. @@ -5718,12 +5773,13 @@ def _cmd_update_impl(args, gateway_mode: bool): uv_env.pop("PYTHONHOME", None) install_group = "termux-all" print(" → Termux detected: using uv + curated termux-all optional profile...") - if _m()._is_termux_env(uv_env) and _is_android_python(): - print(" → Termux/Android detected: prebuilding psutil with Linux source path compatibility...") - _install_psutil_android_compat([uv_bin, "pip"], env=uv_env) - _m()._install_python_dependencies_with_optional_fallback( - [uv_bin, "pip"], env=uv_env, group=install_group - ) + if not deps_current: + if _m()._is_termux_env(uv_env) and _is_android_python(): + print(" → Termux/Android detected: prebuilding psutil with Linux source path compatibility...") + _install_psutil_android_compat([uv_bin, "pip"], env=uv_env) + _m()._install_python_dependencies_with_optional_fallback( + [uv_bin, "pip"], env=uv_env, group=install_group + ) else: # Use sys.executable to explicitly call the venv's pip module, # avoiding PEP 668 'externally-managed-environment' errors on Debian/Ubuntu. @@ -5746,14 +5802,25 @@ def _cmd_update_impl(args, gateway_mode: bool): if _m()._is_termux_env(): install_group = "termux-all" print(" → Termux detected: using curated termux-all optional profile...") - if _m()._is_termux_env() and _is_android_python(): - print(" → Termux/Android detected: prebuilding psutil with Linux source path compatibility...") - _install_psutil_android_compat(pip_cmd) - _m()._install_python_dependencies_with_optional_fallback(pip_cmd, group=install_group) + if not deps_current: + if _m()._is_termux_env() and _is_android_python(): + print(" → Termux/Android detected: prebuilding psutil with Linux source path compatibility...") + _install_psutil_android_compat(pip_cmd) + _m()._install_python_dependencies_with_optional_fallback(pip_cmd, group=install_group) install_prefix = [uv_bin, "pip"] if uv_bin else pip_cmd lazy_env = uv_env if uv_bin else None + if deps_current: + # The verification normally runs inside the install we just + # skipped. Run it here so a wrong skip self-heals into a real + # install (both verifiers reinstall what they find missing) + # instead of leaving a venv nobody checked. + _m()._verify_core_dependencies_installed( + install_prefix, env=lazy_env, group=install_group + ) + _m()._verify_console_scripts_installed(install_prefix, env=lazy_env) + # Core ``.[all]`` install finished. Clear the generic core breadcrumb # before the lazy-refresh phase — that phase uses its own marker so a # later lazy failure cannot be "healed" by clearing the core marker diff --git a/tests/hermes_cli/test_update_skip_unchanged_editable_install.py b/tests/hermes_cli/test_update_skip_unchanged_editable_install.py new file mode 100644 index 0000000000..afdb386ee3 --- /dev/null +++ b/tests/hermes_cli/test_update_skip_unchanged_editable_install.py @@ -0,0 +1,108 @@ +"""``hermes update`` skips the editable reinstall when the pull can't affect it. + +``uv pip install -e .`` never audits an editable target — it reinstalls on +every invocation and rewrites the console-script shims each time. On Windows +that rewrite is the only reason the running ``hermes.exe`` gets quarantined, +and a quarantine that loses its race is the ``os error 32`` family. The gate +under test removes the reinstall (and therefore the rename) for any update +that touches none of the files defining the install. + +These tests drive a REAL git repository. The predicate is a ``git diff`` +pathspec against the pre-pull SHA; mocking git would assert our idea of what +git prints rather than what it does. +""" + +import subprocess + +import pytest + +from hermes_cli.update_cmd import _editable_install_is_current + +GIT = ["git"] + + +@pytest.fixture +def repo(tmp_path): + """A repo with one commit, standing in for the pre-pull checkout.""" + subprocess.run(GIT + ["init", "-q", "-b", "main"], cwd=tmp_path, check=True) + subprocess.run( + GIT + ["config", "user.email", "t@example.com"], cwd=tmp_path, check=True + ) + subprocess.run(GIT + ["config", "user.name", "t"], cwd=tmp_path, check=True) + (tmp_path / "pyproject.toml").write_text("[project]\nname = 'hermes'\n") + (tmp_path / "agent").mkdir() + (tmp_path / "agent" / "__init__.py").write_text("") + (tmp_path / "cli.py").write_text("x = 1\n") + subprocess.run(GIT + ["add", "-A"], cwd=tmp_path, check=True) + subprocess.run(GIT + ["commit", "-qm", "base"], cwd=tmp_path, check=True) + return tmp_path + + +def _head(cwd): + return subprocess.run( + GIT + ["rev-parse", "HEAD"], cwd=cwd, capture_output=True, text=True, check=True + ).stdout.strip() + + +def _commit(cwd, message): + subprocess.run(GIT + ["add", "-A"], cwd=cwd, check=True) + subprocess.run(GIT + ["commit", "-qm", message], cwd=cwd, check=True) + + +def test_source_only_pull_skips_the_reinstall(repo): + """The common update: .py churn inside already-mapped packages.""" + before = _head(repo) + (repo / "cli.py").write_text("x = 2\n") + (repo / "agent" / "loop.py").write_text("y = 1\n") + _commit(repo, "source churn") + + assert _editable_install_is_current(GIT, repo, before) is True + + +def test_new_submodule_in_mapped_package_skips_the_reinstall(repo): + """A new file inside an existing package resolves through its __path__.""" + before = _head(repo) + (repo / "agent" / "brand_new.py").write_text("z = 1\n") + _commit(repo, "new submodule") + + assert _editable_install_is_current(GIT, repo, before) is True + + +@pytest.mark.parametrize( + "filename", + ["pyproject.toml", "setup.py", "setup.cfg", "MANIFEST.in", "uv.lock"], +) +def test_touching_a_file_that_defines_the_install_forces_the_reinstall(repo, filename): + """Dependencies, entry points and the static module list all live here.""" + before = _head(repo) + (repo / filename).write_text("# changed\n") + _commit(repo, f"touch {filename}") + + assert _editable_install_is_current(GIT, repo, before) is False + + +def test_source_churn_alongside_a_pyproject_edit_still_reinstalls(repo): + """The gate must not be fooled by burying the pyproject diff in noise.""" + before = _head(repo) + (repo / "cli.py").write_text("x = 3\n") + (repo / "pyproject.toml").write_text("[project]\nname = 'hermes'\ndeps = []\n") + _commit(repo, "mixed") + + assert _editable_install_is_current(GIT, repo, before) is False + + +def test_missing_pre_pull_sha_fails_closed(repo): + """No SHA (ZIP swap, capture failure) means we cannot prove anything.""" + assert _editable_install_is_current(GIT, repo, None) is False + assert _editable_install_is_current(GIT, repo, "") is False + + +def test_unresolvable_pre_pull_sha_fails_closed(repo): + """A shallow checkout whose base commit isn't present reinstalls as before.""" + assert _editable_install_is_current(GIT, repo, "0" * 40) is False + + +def test_unusable_git_fails_closed(repo): + """A git that cannot be executed must not be read as 'nothing changed'.""" + before = _head(repo) + assert _editable_install_is_current(["definitely-not-git"], repo, before) is False