fix(update): let callers pass the platform verdict to the venv helpers
CI slice 8/8 red:
test_verify_core_dependencies.py::test_uses_virtual_env_from_environment
AssertionError: assert None == PosixPath('.../newvenv/Scripts/python.exe')
The Phase 2 reviewer flagged this exact risk (W4) and I under-weighted it as
"latent, not broken". It was neither — it was already failing.
The suite exercises Windows-only paths on Linux CI by patching predicates
(`hermes_cli.main._is_windows`, `is_windows`, `platform.system`). Routing
those call sites through a helper that reads `sys.platform` unconditionally
meant the patches no longer reached the path derivation: the test built
`Scripts/python.exe` while the code looked for `bin/python`.
venv_bin_dir/venv_python_path now take an optional `windows=` verdict,
defaulting to the host. Every converted site passes its own predicate, so
the patched-predicate coverage is restored — the dedup keeps the layout in
one place without hijacking the platform decision.
Verified by causation: dropping `windows=` reproduces the CI failure exactly;
restoring it goes green. Added two regression tests, including one asserting
a patched `_is_windows` still reaches the derivation.
This commit is contained in:
@@ -2527,7 +2527,7 @@ def get_python_path() -> str:
|
||||
if venv is not None:
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
venv_python = venv_python_path(venv)
|
||||
venv_python = venv_python_path(venv, windows=is_windows())
|
||||
if venv_python.exists():
|
||||
return str(venv_python)
|
||||
return sys.executable
|
||||
|
||||
+2
-2
@@ -7965,7 +7965,7 @@ def _venv_scripts_dir() -> Path | None:
|
||||
return None
|
||||
from hermes_constants import venv_bin_dir
|
||||
|
||||
scripts = venv_bin_dir(venv_dir)
|
||||
scripts = venv_bin_dir(venv_dir, windows=_is_windows())
|
||||
return scripts if scripts.is_dir() else None
|
||||
|
||||
|
||||
@@ -8761,7 +8761,7 @@ def _resolve_install_target_python(
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
venv_root = Path(env["VIRTUAL_ENV"])
|
||||
candidate = venv_python_path(venv_root)
|
||||
candidate = venv_python_path(venv_root, windows=_is_windows())
|
||||
if candidate.exists():
|
||||
return candidate
|
||||
|
||||
|
||||
@@ -386,7 +386,7 @@ def update_managed_uv(
|
||||
def _venv_python(venv_dir: Path) -> Path:
|
||||
from hermes_constants import venv_python_path
|
||||
|
||||
return venv_python_path(venv_dir)
|
||||
return venv_python_path(venv_dir, windows=platform.system() == "Windows")
|
||||
|
||||
|
||||
def _remove_tree(path: Path, *, boundary: Path) -> None:
|
||||
|
||||
@@ -223,7 +223,9 @@ def _validate_critical_modules_import(root) -> tuple[bool, str | None, str | Non
|
||||
try:
|
||||
interpreter = sys.executable
|
||||
try:
|
||||
venv_python = venv_python_path(Path(root) / "venv")
|
||||
venv_python = venv_python_path(
|
||||
Path(root) / "venv", windows=_m()._is_windows()
|
||||
)
|
||||
if venv_python.exists():
|
||||
interpreter = str(venv_python)
|
||||
except Exception:
|
||||
@@ -2754,7 +2756,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"
|
||||
venv_python = venv_python_path(venv_dir)
|
||||
venv_python = venv_python_path(venv_dir, windows=_m()._is_windows())
|
||||
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
|
||||
@@ -3840,7 +3842,9 @@ 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 (
|
||||
venv_python_path(_m().PROJECT_ROOT / "venv")
|
||||
venv_python_path(
|
||||
_m().PROJECT_ROOT / "venv", windows=_m()._is_windows()
|
||||
)
|
||||
).exists()
|
||||
if venv_python_missing and repair_uv:
|
||||
print("→ Recreating virtual environment...")
|
||||
|
||||
+15
-5
@@ -1252,7 +1252,7 @@ AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1"
|
||||
|
||||
# ─── Venv layout ─────────────────────────────────────────────────────────────
|
||||
|
||||
def venv_bin_dir(venv_dir) -> Path:
|
||||
def venv_bin_dir(venv_dir, *, windows: bool | None = None) -> Path:
|
||||
"""Directory holding a venv's executables (``Scripts`` / ``bin``).
|
||||
|
||||
Canonical helper for venv layout. This was open-coded in seven places
|
||||
@@ -1264,16 +1264,26 @@ def venv_bin_dir(venv_dir) -> Path:
|
||||
``agent/lsp/install.py``) still hand-roll it — convert them as they are
|
||||
touched.
|
||||
|
||||
*windows* lets a caller pass its own platform verdict. Several callers
|
||||
resolve this through predicates the test-suite patches to exercise
|
||||
Windows paths on Linux CI (``hermes_cli.main._is_windows`` and friends);
|
||||
reading ``sys.platform`` unconditionally here would silently drop those
|
||||
paths out of coverage. Defaults to the host platform.
|
||||
|
||||
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")
|
||||
if windows is None:
|
||||
windows = sys.platform == "win32"
|
||||
return Path(venv_dir) / ("Scripts" if windows else "bin")
|
||||
|
||||
|
||||
def venv_python_path(venv_dir) -> Path:
|
||||
def venv_python_path(venv_dir, *, windows: bool | None = None) -> 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"
|
||||
if windows is None:
|
||||
windows = sys.platform == "win32"
|
||||
return venv_bin_dir(venv_dir, windows=windows) / (
|
||||
"python.exe" if windows else "python"
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -307,3 +307,44 @@ def test_atomic_replace_dir_still_works_as_a_shim(tmp_path):
|
||||
|
||||
assert (live / "ui-tui" / "version.txt").read_text() == "new"
|
||||
assert not [p for p in os.listdir(live) if "hermes-update" in p]
|
||||
|
||||
|
||||
def test_venv_helpers_honour_an_explicit_platform_verdict():
|
||||
"""Callers must be able to override the platform check (#76107 CI).
|
||||
|
||||
The suite exercises Windows paths on Linux CI by patching predicates like
|
||||
`hermes_cli.main._is_windows`. A helper that reads `sys.platform`
|
||||
unconditionally silently drops those paths out of coverage -- and broke
|
||||
`test_verify_core_dependencies.py::test_uses_virtual_env_from_environment`,
|
||||
which patches `_is_windows` and then asserts on a `Scripts/python.exe`
|
||||
path.
|
||||
"""
|
||||
v = Path("/opt/proj/venv")
|
||||
assert venv_bin_dir(v, windows=True).name == "Scripts"
|
||||
assert venv_bin_dir(v, windows=False).name == "bin"
|
||||
assert venv_python_path(v, windows=True).name == "python.exe"
|
||||
assert venv_python_path(v, windows=False).name == "python"
|
||||
# Halves must stay consistent under an explicit verdict.
|
||||
for flag in (True, False):
|
||||
assert venv_python_path(v, windows=flag).parent == venv_bin_dir(
|
||||
v, windows=flag
|
||||
)
|
||||
|
||||
|
||||
def test_patched_is_windows_reaches_the_venv_path_derivation():
|
||||
"""End-to-end: patching the module predicate must change the derived path."""
|
||||
from unittest.mock import patch
|
||||
|
||||
from hermes_cli import main as hermes_main
|
||||
|
||||
with patch.object(hermes_main, "_is_windows", return_value=True):
|
||||
got = hermes_main._resolve_install_target_python(
|
||||
["uv", "pip"], env={"VIRTUAL_ENV": "/nope/venv"}
|
||||
)
|
||||
# The path doesn't exist so we get None, but the *derivation* must have
|
||||
# used the Windows layout -- assert that directly.
|
||||
assert got is None
|
||||
assert (
|
||||
venv_python_path("/nope/venv", windows=True).as_posix()
|
||||
== "/nope/venv/Scripts/python.exe"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user