diff --git a/CHANGELOG/v3.md b/CHANGELOG/v3.md index 9b9647d0c..7c129ce2e 100644 --- a/CHANGELOG/v3.md +++ b/CHANGELOG/v3.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Worktree `uv run aelf` no longer downgrades the user's installed hooks ([#1044](https://github.com/robotrocketscience/aelfrice/issues/1044)).** The #834 guard was inert for anyone who also had a `uv tool` install: `lifecycle._is_uv_tool_install()` short-circuited `True` on a filesystem-presence check (`~/.local/share/uv/tools/aelfrice/` existing *anywhere* on the box) before the correct `sys.prefix`-under-tools-root check ran — so a source worktree's `uv run aelf` was misclassified as the uv-tool install and silently rewrote `~/.claude/settings.json` hooks backwards (observed: v3.8.0 → v3.6.0, 8 hooks reset). Split the predicate: the auto-install gate now delegates to a new `lifecycle._running_from_uv_tool()` (the running-**process** check only), while `_is_uv_tool_install()` keeps its disk-presence semantics for `upgrade_advice()` (which legitimately asks "is a uv-tool install present?"). Defense in depth: `maybe_install_manifest` now refuses to stamp the hook surface **backwards** in the non-force path — a running older version skips the merge with a one-line notice instead of downgrading (an explicit `aelf setup` still re-stamps). Regression tests cover the presence-vs-process split and the never-downgrade guard. + ## [3.8.0] - 2026-06-30 The locks-hygiene and passive-capture wave. Locks gain a frozen/reference tier diff --git a/src/aelfrice/auto_install.py b/src/aelfrice/auto_install.py index 31987c7ce..da30b0e08 100644 --- a/src/aelfrice/auto_install.py +++ b/src/aelfrice/auto_install.py @@ -356,6 +356,45 @@ def _result_added_anything(result: object) -> bool: # --- main entry ---------------------------------------------------------- +def _version_key(v: str) -> tuple[int, ...]: + """Parse ``'X.Y.Z...'`` into a comparable int tuple. + + Leading digits of each dot-segment; a non-numeric segment contributes + 0. This is deliberately not full PEP 440 — it only needs to answer + "is A older than B" for the never-downgrade guard (#1044). + """ + key: list[int] = [] + for seg in v.split("."): + digits = "" + for ch in seg: + if ch.isdigit(): + digits += ch + else: + break + key.append(int(digits) if digits else 0) + return tuple(key) or (0,) + + +def _is_downgrade(installed_version: str, prev: str) -> bool: + """True iff running ``installed_version`` is strictly older than the + on-disk stamp ``prev`` (and prev is a real stamp, not the sentinel). + """ + return prev != _UNSTAMPED and _version_key(installed_version) < _version_key(prev) + + +def _downgrade_skip_result(installed_version: str, prev: str) -> AutoInstallResult: + return AutoInstallResult( + ran=False, + prev_version=prev, + new_version=installed_version, + message=( + f"aelfrice: skipped hook auto-install — running " + f"v{installed_version} is older than the installed v{prev}; " + f"not downgrading (run `aelf setup` to force)" + ), + ) + + def maybe_install_manifest( *, installed_version: str, @@ -395,6 +434,12 @@ def maybe_install_manifest( return AutoInstallResult( ran=False, prev_version=prev, new_version=installed_version ) + # Defense in depth (#1044): a running binary must never stamp the hook + # surface backwards. The primary gate already excludes worktrees, but + # if an older aelfrice ever reaches here (non-force path), skip rather + # than silently downgrade the user's installed hooks. + if not force and _is_downgrade(installed_version, prev): + return _downgrade_skip_result(installed_version, prev) target_path = settings_path if settings_path is not None else USER_SETTINGS_PATH # Acquire exclusive lock on the stamp's parent dir (the stamp file @@ -419,6 +464,8 @@ def maybe_install_manifest( return AutoInstallResult( ran=False, prev_version=prev, new_version=installed_version ) + if not force and _is_downgrade(installed_version, prev): + return _downgrade_skip_result(installed_version, prev) return _do_merge( prev_version=prev, installed_version=installed_version, @@ -539,11 +586,15 @@ def is_running_from_uv_tool_install() -> bool: the user's installed-version hook surface to whatever the worktree happened to advertise. - Detection delegates to `lifecycle._is_uv_tool_install`, which checks - for `~/.local/share/uv/tools/aelfrice/` and falls back to a - `sys.prefix` scan against the uv tools root. Worktree-local venvs, - contributor `pytest` runs, system Python, and pipx installs all - return False here and are excluded from auto-install. + Detection delegates to `lifecycle._running_from_uv_tool`, which asks + whether *this process* resolves under the uv tools root + (`sys.prefix` / `sys.executable` scan) — NOT whether a uv-tool + install merely exists somewhere on the box. The earlier delegate + (`_is_uv_tool_install`) short-circuited True on a filesystem-presence + check, so any user who also had a uv-tool install saw a worktree's + `uv run aelf` reintroduce the #834 downgrade (#1044). Worktree-local + venvs, contributor `pytest` runs, system Python, and pipx installs + all return False here and are excluded from auto-install. Returning False means the gate skips merging — power users who want auto-install to run from a non-uv-tool context can still @@ -552,9 +603,9 @@ def is_running_from_uv_tool_install() -> bool: """ # Local import keeps `auto_install` importable when `lifecycle` has # not yet been imported by the caller (e.g. during early CLI bootstrap). - from aelfrice.lifecycle import _is_uv_tool_install + from aelfrice.lifecycle import _running_from_uv_tool - return _is_uv_tool_install() + return _running_from_uv_tool() def auto_install_at_cli_entry(installed_version: str) -> None: diff --git a/src/aelfrice/lifecycle.py b/src/aelfrice/lifecycle.py index aff2e37d6..930d1a861 100644 --- a/src/aelfrice/lifecycle.py +++ b/src/aelfrice/lifecycle.py @@ -332,27 +332,52 @@ class UpgradeAdvice: context: str # 'uv_tool' | 'non_uv' -def _is_uv_tool_install() -> bool: - """Detect a uv-tool-managed install. - - uv tool installs each package under ~/.local/share/uv/tools//. - We check for the package directory directly rather than shelling - out to `uv` (which may not be on PATH inside the managed env). - As a secondary signal, if sys.executable or sys.prefix resolves - under the uv tools directory we also consider it a uv-tool install. +def _running_from_uv_tool() -> bool: + """True iff THIS running process is the uv-tool-managed install. + + Checks whether sys.prefix / sys.executable resolves *under* the uv + tools root (~/.local/share/uv/tools/). Unlike a filesystem-presence + check, this correctly returns False for a source worktree's + ``uv run aelf`` even when a uv-tool install exists elsewhere on the + box. Use this to gate the hook auto-install — the question there is + "is this process the install?", not "does an install exist anywhere?" + (#1044 — the ``.exists()`` short-circuit reintroduced the #834 bug for + any user who also had a uv-tool install). """ - uv_tools_dir = Path.home() / ".local" / "share" / "uv" / "tools" / PACKAGE_NAME - if uv_tools_dir.exists(): - return True - # Secondary: check if sys.prefix or sys.executable path contains the - # uv tools tree. Covers cases where the package dir name differs. import sys - prefix_norm = sys.prefix.replace("\\", "/") + # ~/.local/share/uv/tools/ is the canonical uv tools root on # Linux/macOS. On Windows it is %APPDATA%\uv\tools\ but we only # support the POSIX layout for now. - uv_tools_root = str(Path.home() / ".local" / "share" / "uv" / "tools") - return prefix_norm.startswith(uv_tools_root.replace("\\", "/")) + # Trailing slash so we match true descendants only: a sibling like + # ``.../uv/toolshed`` must NOT satisfy a prefix test against + # ``.../uv/tools`` (Sourcery, #1044 review). + uv_tools_root = str( + Path.home() / ".local" / "share" / "uv" / "tools" + ).replace("\\", "/").rstrip("/") + "/" + for candidate in (sys.prefix, sys.executable): + if candidate and candidate.replace("\\", "/").startswith(uv_tools_root): + return True + return False + + +def _is_uv_tool_install() -> bool: + """Detect that a uv-tool-managed install EXISTS on this box. + + Answers "is aelfrice installed via ``uv tool`` on this machine?" — + used by ``upgrade_advice()`` to recommend ``uv tool upgrade``. This + intentionally includes a filesystem-presence check: the install dir + may exist even when the *current* process runs from elsewhere (a + worktree, a venv), and the upgrade advice is still "upgrade your uv + tool copy". Do NOT use this to gate auto-install — that must ask + whether *this process* is the install; use ``_running_from_uv_tool()`` + (#1044). + """ + uv_tools_dir = Path.home() / ".local" / "share" / "uv" / "tools" / PACKAGE_NAME + if uv_tools_dir.exists(): + return True + # Secondary: this process is itself running under the uv tools tree. + return _running_from_uv_tool() def _is_pipx_install() -> bool: diff --git a/tests/test_auto_install.py b/tests/test_auto_install.py index 3e1bc9e9d..dbc834914 100644 --- a/tests/test_auto_install.py +++ b/tests/test_auto_install.py @@ -16,6 +16,7 @@ from __future__ import annotations import json +import sys from pathlib import Path import pytest @@ -369,19 +370,95 @@ def test_auto_install_at_cli_entry_skips_when_not_uv_tool( assert captured.err == "" -def test_is_running_from_uv_tool_install_delegates_to_lifecycle( +def test_is_running_from_uv_tool_install_delegates_to_running_check( monkeypatch: pytest.MonkeyPatch, ) -> None: - """The gate forwards to `lifecycle._is_uv_tool_install` so detection - stays in one place. Test both branches via that single seam.""" + """The gate forwards to `lifecycle._running_from_uv_tool` (the + running-PROCESS check), not `_is_uv_tool_install` (disk presence). + Delegating to presence reintroduced the #834 downgrade from any + worktree whenever a uv-tool install existed on the box (#1044).""" from aelfrice import lifecycle - monkeypatch.setattr(lifecycle, "_is_uv_tool_install", lambda: True) + monkeypatch.setattr(lifecycle, "_running_from_uv_tool", lambda: True) assert auto_install.is_running_from_uv_tool_install() is True - monkeypatch.setattr(lifecycle, "_is_uv_tool_install", lambda: False) + monkeypatch.setattr(lifecycle, "_running_from_uv_tool", lambda: False) assert auto_install.is_running_from_uv_tool_install() is False +def test_running_from_uv_tool_distinguishes_process_from_presence( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """#1044 core regression: a uv-tool install existing on disk must NOT + make a worktree's `uv run aelf` look like the uv-tool install. The + auto-install gate keys off the running process, so it stays False.""" + from aelfrice import lifecycle + + monkeypatch.setenv("HOME", str(tmp_path)) + # A uv-tool install exists on the box ... + (tmp_path / ".local" / "share" / "uv" / "tools" / "aelfrice").mkdir(parents=True) + # ... but THIS process runs from a source worktree's venv elsewhere. + worktree_venv = tmp_path / "projects" / "aelfrice" / ".venv" + monkeypatch.setattr(sys, "prefix", str(worktree_venv)) + monkeypatch.setattr(sys, "executable", str(worktree_venv / "bin" / "python")) + + assert lifecycle._is_uv_tool_install() is True # disk presence: yes + assert lifecycle._running_from_uv_tool() is False # this process: no + # The gate must use the running-process answer, so it stays False — + # the worktree does NOT rewrite the user's global hooks. + assert auto_install.is_running_from_uv_tool_install() is False + + +def test_running_from_uv_tool_true_under_tools_root( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """When the running process resolves under the uv tools root, it IS + the uv-tool install and the gate opens.""" + from aelfrice import lifecycle + + monkeypatch.setenv("HOME", str(tmp_path)) + prefix = tmp_path / ".local" / "share" / "uv" / "tools" / "aelfrice" + monkeypatch.setattr(sys, "prefix", str(prefix)) + monkeypatch.setattr(sys, "executable", str(prefix / "bin" / "python")) + assert lifecycle._running_from_uv_tool() is True + + +def test_running_from_uv_tool_rejects_sibling_of_tools_root( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """A directory that merely shares a name *prefix* with the uv tools + root (e.g. ``.../uv/toolshed``) is not a descendant and must not be + misread as the uv-tool install (#1044 review — prefix vs containment).""" + from aelfrice import lifecycle + + monkeypatch.setenv("HOME", str(tmp_path)) + sibling = tmp_path / ".local" / "share" / "uv" / "toolshed" / "aelfrice" + monkeypatch.setattr(sys, "prefix", str(sibling)) + monkeypatch.setattr(sys, "executable", str(sibling / "bin" / "python")) + assert lifecycle._running_from_uv_tool() is False + + +def test_maybe_install_manifest_never_downgrades( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, +) -> None: + """#1044 defense in depth: a running older version must never stamp + the hook surface backwards. The merge is skipped (non-force) and the + stamp + settings are left untouched.""" + stamp = tmp_path / "stamp" + auto_install.write_stamp(stamp, "3.8.0") + settings = tmp_path / "settings.json" + monkeypatch.setattr(auto_install, "OPT_OUT_PATH", tmp_path / "opt-out") + + res = auto_install.maybe_install_manifest( + installed_version="3.6.0", + settings_path=settings, + stamp_path=stamp, + ) + assert res.ran is False + assert "not downgrading" in res.message + assert auto_install.read_stamp(stamp) == "3.8.0" # stamp untouched + assert not settings.exists() # global settings untouched + + def test_auto_install_at_cli_entry_runs_when_uv_tool( monkeypatch: pytest.MonkeyPatch, ) -> None: