mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
fix(windows): keep a review tree from taking over the home's hermes command
expose_cli runs at each checkout's first boot. On Windows it published that checkout's launchers into <home>\bin, the folder on the User PATH, whichever checkout called it. A review tree or worktree started against the real home repointed every console's `hermes` at itself, and once that tree had no dependency environment, `hermes` failed with "no dependency environment is committed for this install". A checkout other than the home's own now leaves <home>\bin alone while the hermes command there starts another checkout that still exists. Missing launchers, and launchers whose checkout or interpreter is gone, are still replaced. The home's own checkout still republishes, which heals a machine an earlier run already rebound. Follow-up to #137951, which listed this case as not fixed.
This commit is contained in:
1 parent
d49e2ebbae
commit
3226ddd8a5
2 files changed
+183
-9
No files matched your search
@@ -716,6 +716,71 @@ def _launcher_python(target: Path) -> Path | None:
|
||||
return None
|
||||
|
||||
|
||||
def _launcher_checkout(target: Path) -> Path | None:
|
||||
"""Checkout an existing Windows launcher starts, else None.
|
||||
|
||||
Reads the script ``_launcher_script`` wrote: the ``__main__.py`` of the
|
||||
distlib executable, or the base64 payload of the ``.cmd`` fallback. Its
|
||||
first ``sys.path.insert`` names the checkout.
|
||||
"""
|
||||
import ast
|
||||
import base64
|
||||
import re
|
||||
from zipfile import BadZipFile, ZipFile
|
||||
|
||||
try:
|
||||
if Path(target).suffix.lower() == ".exe":
|
||||
with ZipFile(target) as archive:
|
||||
script = archive.read("__main__.py").decode("utf-8")
|
||||
else:
|
||||
encoded = re.search(r"exec\(base64\.b64decode\('([A-Za-z0-9+/=]+)'\)\)",
|
||||
Path(target).read_text(encoding="utf-8-sig"))
|
||||
if encoded is None:
|
||||
return None
|
||||
script = base64.b64decode(encoded[1], validate=True).decode("utf-8")
|
||||
inserted = re.search(r"""^sys\.path\.insert\(0, ('(?:[^'\\]|\\.)*'|"(?:[^"\\]|\\.)*")\)\r?$""",
|
||||
script, re.MULTILINE)
|
||||
return Path(ast.literal_eval(inserted[1])) if inserted else None
|
||||
except (OSError, BadZipFile, KeyError, ValueError, SyntaxError):
|
||||
# binascii.Error and UnicodeDecodeError are ValueErrors.
|
||||
return None
|
||||
|
||||
|
||||
def _launcher_of_another_checkout(directory: Path, root: Path) -> Path | None:
|
||||
"""A working command in *directory* that starts a checkout other than *root*, else None.
|
||||
|
||||
Only the file Windows runs counts: PATHEXT resolves ``.exe`` before
|
||||
``.cmd``. A launcher whose checkout or interpreter is gone does not count,
|
||||
and neither does one this module cannot read.
|
||||
"""
|
||||
for name in WINDOWS_BIN_LAUNCHERS:
|
||||
try:
|
||||
runs = next((target for target in (directory / f"{name}.exe", directory / f"{name}.cmd")
|
||||
if target.is_file()), None)
|
||||
if runs is None:
|
||||
continue
|
||||
checkout, python = _launcher_checkout(runs), _launcher_python(runs)
|
||||
if (checkout is not None and python is not None and checkout != root
|
||||
and checkout.is_dir() and python.is_file()):
|
||||
return runs
|
||||
except OSError:
|
||||
continue
|
||||
return None
|
||||
|
||||
|
||||
def _is_home_checkout(root: Path, store: Path) -> bool:
|
||||
"""Is *root* the checkout the default data root was installed with?
|
||||
|
||||
That checkout sits at ``<home>/<checkout>`` and runs ``<home>/tools``. The
|
||||
layout alone is not proof: a root that only borrows the checkout can match
|
||||
it through HERMES_RUNTIME_DIR=<home>/tools, so the home must also own it.
|
||||
"""
|
||||
from hermes_constants import get_default_hermes_root
|
||||
home = get_default_hermes_root().resolve()
|
||||
return (root.parent == home and store.resolve() == (home / "tools").resolve()
|
||||
and owning_home_root(root) is None)
|
||||
|
||||
|
||||
def _kept_shared_launcher(name: str, local: Path, store: Path, own: Path | None) -> Path | None:
|
||||
"""Existing checkout launcher already bound outside this root's store.
|
||||
|
||||
@@ -773,15 +838,9 @@ def ensure_install_launchers(repo_root: Path, out_dir: Path) -> list[str]:
|
||||
local.mkdir(parents=True, exist_ok=True)
|
||||
store = store_root(root)
|
||||
own = resolve_store_python(root)
|
||||
from hermes_constants import get_default_hermes_root
|
||||
home = get_default_hermes_root().resolve()
|
||||
# The data root this checkout was installed into (<home>/hermes-agent with
|
||||
# <home>/tools) always republishes, which also heals an install that an
|
||||
# earlier foreign launch already rebound. The layout alone is not proof: a
|
||||
# root that only borrows the checkout can match it through
|
||||
# HERMES_RUNTIME_DIR=<home>/tools, so it must also own the checkout.
|
||||
owner = (root.parent == home and store.resolve() == (home / "tools").resolve()
|
||||
and owning_home_root(root) is None)
|
||||
# The checkout this data root was installed with always republishes, which
|
||||
# also heals an install that an earlier foreign launch already rebound.
|
||||
owner = _is_home_checkout(root, store)
|
||||
written = []
|
||||
for name in WINDOWS_BIN_LAUNCHERS:
|
||||
# ponytail: one guard here covers every caller (launch, sync, repair,
|
||||
@@ -1011,6 +1070,10 @@ def _expose_windows_user_bin(root: Path, *, create: bool) -> dict:
|
||||
so an update converges a machine installed under the older venv\\Scripts
|
||||
convention instead of leaving it there forever. The registration happens
|
||||
only when new consoles resolve this home (``owns_user_path``).
|
||||
|
||||
Commands there that start another checkout which still exists stay in
|
||||
place unless *root* is the home's own checkout: a review tree or worktree
|
||||
started against the home leaves the installed ``hermes`` alone.
|
||||
"""
|
||||
from hermes_constants import get_default_hermes_root
|
||||
|
||||
@@ -1020,6 +1083,9 @@ def _expose_windows_user_bin(root: Path, *, create: bool) -> dict:
|
||||
# Bootstrap: repair only commands we already own, enable nothing new.
|
||||
published = _publish_conveniences(root, directory, WINDOWS_BIN_LAUNCHERS, create=False)
|
||||
return {"ok": True, "written": [path.name for path, changed in published.items() if changed]}
|
||||
if not _is_home_checkout(root, store_root(root)) and (
|
||||
other := _launcher_of_another_checkout(directory, root)) is not None:
|
||||
return {"ok": True, "skipped": f"{other} starts another checkout"}
|
||||
directory.mkdir(parents=True, exist_ok=True)
|
||||
written = ensure_install_launchers(root, directory)
|
||||
if len(written) != len(WINDOWS_BIN_LAUNCHERS):
|
||||
|
||||
@@ -13,9 +13,12 @@ gate gets its own test.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import io
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import sys
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
@@ -181,6 +184,111 @@ def test_environment_block_reader_counts_utf16_units():
|
||||
"A": "1", "MOOD": "\U0001F600", "HERMES_HOME": "C:\\h"}
|
||||
|
||||
|
||||
def _windows_home(tmp_path, monkeypatch) -> Path:
|
||||
"""The default data root under a temp LOCALAPPDATA, with a store interpreter; new consoles use it.
|
||||
|
||||
distlib is switched off, so publication writes the ``.cmd`` form on every test host."""
|
||||
local = tmp_path / "local"
|
||||
monkeypatch.setenv("LOCALAPPDATA", str(local))
|
||||
monkeypatch.delenv("HERMES_HOME", raising=False)
|
||||
monkeypatch.delenv("HERMES_RUNTIME_DIR", raising=False)
|
||||
monkeypatch.setattr(_launchers, "_new_console_environment", lambda: {"LOCALAPPDATA": str(local)})
|
||||
monkeypatch.setattr(_launchers, "_load_script_maker", lambda: None)
|
||||
monkeypatch.setattr(_launchers, "_store_script_maker", lambda python_exe: None)
|
||||
home = local / "hermes"
|
||||
_store_python(home).parent.mkdir(parents=True)
|
||||
_store_python(home).touch()
|
||||
(home / "tools" / "facts.json").write_text(
|
||||
json.dumps({"schema": 1, "packages": {"python": {"entry": _store_python(home).parent.name}}}),
|
||||
encoding="utf-8")
|
||||
return home
|
||||
|
||||
|
||||
def _store_python(home: Path) -> Path:
|
||||
return home / "tools" / "python-fixture" / "python.exe"
|
||||
|
||||
|
||||
def _checkout(path: Path) -> Path:
|
||||
(path / ".git").mkdir(parents=True)
|
||||
(path / "install-stamp.json").write_text(json.dumps({"updateMechanism": "self"}), encoding="utf-8")
|
||||
return path.resolve()
|
||||
|
||||
|
||||
def _write_exe_launcher(target: Path, python: Path, root: Path) -> None:
|
||||
"""distlib's shape: loader stub, '#!<python> -I' shebang, then a zip holding the launcher script."""
|
||||
payload = io.BytesIO()
|
||||
with zipfile.ZipFile(payload, "w") as archive:
|
||||
archive.writestr("__main__.py", _launchers._launcher_script(target.stem, root, None))
|
||||
target.write_bytes(b"MZ" + b"\x00" * 64 + b"#!" + str(python).encode() + b" -I\r\n" + payload.getvalue())
|
||||
|
||||
|
||||
def _command(home: Path) -> Path:
|
||||
"""The ``hermes`` a console runs: PATHEXT resolves .exe before .cmd."""
|
||||
exe = home / "bin" / "hermes.exe"
|
||||
return exe if exe.exists() else home / "bin" / "hermes.cmd"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def path_registrations(monkeypatch):
|
||||
registered = []
|
||||
monkeypatch.setattr(_launchers, "_register_windows_user_path",
|
||||
lambda entry: registered.append(Path(entry)) or "added")
|
||||
return registered
|
||||
|
||||
|
||||
@pytest.mark.platforms("windows")
|
||||
@pytest.mark.parametrize("form", ["exe", "cmd"])
|
||||
def test_review_tree_leaves_the_installed_commands_alone(form, path_registrations, tmp_path, monkeypatch):
|
||||
"""A review tree started against the real home keeps the installed checkout's commands in its bin.
|
||||
|
||||
Its boot bootstrap runs expose_cli too; republishing there would point every
|
||||
console's ``hermes`` at the review tree."""
|
||||
home = _windows_home(tmp_path, monkeypatch)
|
||||
installed = _checkout(home / "hermes-agent")
|
||||
review = _checkout(home / "reviews" / "pr-1" / "rebuild-wt")
|
||||
if form == "exe":
|
||||
(home / "bin").mkdir()
|
||||
for name in _launchers.WINDOWS_BIN_LAUNCHERS:
|
||||
_write_exe_launcher(home / "bin" / f"{name}.exe", _store_python(home), installed)
|
||||
else:
|
||||
assert _launchers._expose_windows_user_bin(installed, create=True)["path"] == "added"
|
||||
before = {path.name: path.read_bytes() for path in (home / "bin").iterdir()}
|
||||
|
||||
result = _launchers._expose_windows_user_bin(review, create=True)
|
||||
|
||||
assert {path.name: path.read_bytes() for path in (home / "bin").iterdir()} == before
|
||||
assert result == {"ok": True, "skipped": f"{home / 'bin' / f'hermes.{form}'} starts another checkout"}
|
||||
assert _launchers._launcher_checkout(_command(home)) == installed
|
||||
assert path_registrations == ([] if form == "exe" else [home / "bin"])
|
||||
|
||||
|
||||
@pytest.mark.platforms("windows")
|
||||
def test_review_tree_publishes_where_no_working_command_exists(path_registrations, tmp_path, monkeypatch):
|
||||
home = _windows_home(tmp_path, monkeypatch)
|
||||
first = _checkout(home / "reviews" / "pr-1" / "rebuild-wt")
|
||||
second = _checkout(home / "reviews" / "pr-2" / "rebuild-wt")
|
||||
|
||||
assert "skipped" not in _launchers._expose_windows_user_bin(first, create=True)
|
||||
assert _launchers._launcher_checkout(_command(home)) == first
|
||||
|
||||
# A command whose checkout is gone starts nothing, so it is replaced.
|
||||
shutil.rmtree(first)
|
||||
assert "skipped" not in _launchers._expose_windows_user_bin(second, create=True)
|
||||
assert _launchers._launcher_checkout(_command(home)) == second
|
||||
|
||||
|
||||
@pytest.mark.platforms("windows")
|
||||
def test_installed_checkout_takes_its_commands_back(path_registrations, tmp_path, monkeypatch):
|
||||
home = _windows_home(tmp_path, monkeypatch)
|
||||
installed = _checkout(home / "hermes-agent")
|
||||
review = _checkout(home / "reviews" / "pr-1" / "rebuild-wt")
|
||||
_launchers._expose_windows_user_bin(review, create=True)
|
||||
assert _launchers._launcher_checkout(_command(home)) == review
|
||||
|
||||
assert "skipped" not in _launchers._expose_windows_user_bin(installed, create=True)
|
||||
assert _launchers._launcher_checkout(_command(home)) == installed
|
||||
|
||||
|
||||
def test_registered_as_a_home_step():
|
||||
assert ("expose_cli", _launchers.expose_cli) in post_update.HOME_STEPS
|
||||
|
||||
|
||||
Reference in new issue
Block a user