fix(update): don't reinstall the editable package when the pull can't affect it (#90967)
`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.
This commit is contained in:
+78
-11
@@ -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
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user