fix(lifecycle): gate hook auto-install on the running process, not disk presence (#1044) - #1046
Conversation
…sk presence (#1044) _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 (v3.8.0 -> v3.6.0), reintroducing #834 for any user who also had a uv-tool install. Split the predicate: the auto-install gate now delegates to a new lifecycle._running_from_uv_tool() (running-process check only), while _is_uv_tool_install() keeps disk-presence semantics for upgrade_advice(). Defense in depth: maybe_install_manifest refuses to stamp the hook surface backwards in the non-force path (skips with a notice; `aelf setup` still re-stamps). Regression tests cover the presence-vs-process split and the never-downgrade guard.
Reviewer's GuideFixes incorrect classification of worktree invocations as uv-tool installs and prevents automatic hook manifest downgrades by splitting uv-tool detection into process-vs-presence and adding a version-based guard in the auto-install path, with corresponding tests and changelog updates. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSplits uv-tool detection into process-based ( ChangesDowngrade guard and detection fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as aelf CLI entry
participant Gate as auto_install.is_running_from_uv_tool_install
participant Lifecycle as lifecycle._running_from_uv_tool
participant Installer as maybe_install_manifest
participant Stamp as version stamp file
participant Settings as ~/.claude/settings.json
CLI->>Gate: check if auto-install should run
Gate->>Lifecycle: query running process location
Lifecycle-->>Gate: true/false (process under uv tools root)
Gate-->>CLI: gate result
CLI->>Installer: maybe_install_manifest(installed_version)
Installer->>Stamp: read prev_version
Installer->>Installer: is_downgrade(installed_version, prev_version)
alt is downgrade and not forced
Installer-->>CLI: skip result, "not downgrading"
else safe to proceed
Installer->>Settings: merge hook entries
Installer->>Stamp: write new version
Installer-->>CLI: success result
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The downgrade guard in
maybe_install_manifestis evaluated twice (once before acquiring the lock and once after re-reading the stamp); consider consolidating this into a single post-lock check to avoid duplication and ensure the decision is always based on the up-to-date stamp. - The logic for deriving the uv tools root (
Path.home() / '.local' / 'share' / 'uv' / 'tools'withreplace('\', '/')) is duplicated in_running_from_uv_tooland_is_uv_tool_install; factoring this into a shared helper would reduce repetition and keep future changes to the path semantics in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The downgrade guard in `maybe_install_manifest` is evaluated twice (once before acquiring the lock and once after re-reading the stamp); consider consolidating this into a single post-lock check to avoid duplication and ensure the decision is always based on the up-to-date stamp.
- The logic for deriving the uv tools root (`Path.home() / '.local' / 'share' / 'uv' / 'tools'` with `replace('\', '/')`) is duplicated in `_running_from_uv_tool` and `_is_uv_tool_install`; factoring this into a shared helper would reduce repetition and keep future changes to the path semantics in one place.
## Individual Comments
### Comment 1
<location path="src/aelfrice/lifecycle.py" line_range="335-344" />
<code_context>
+def _running_from_uv_tool() -> bool:
</code_context>
<issue_to_address>
**issue (bug_risk):** Using string `startswith` on normalized paths can mis-detect when one path is a sibling rather than a true descendant of the uv tools root.
`candidate.replace("\\", "/").startswith(uv_tools_root)` can incorrectly return True for siblings like `/home/me/.local/share/uv/toolshed/...` when `uv_tools_root` is `/home/me/.local/share/uv/tools`, causing non‑uv environments to be treated as uv-tool installs. Instead, use a path-based containment check such as `Path(candidate).resolve().is_relative_to(Path(uv_tools_root))` (3.9+) or `os.path.commonpath([candidate, uv_tools_root]) == uv_tools_root` for a correct descendant test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_auto_install.py (1)
425-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a companion test for the
force=Trueoverride path.This test only covers the non-force skip. The PR objectives explicitly state explicit
aelf setup/upgrade flows must still be able to restamp even when it would be a downgrade — that behavior isn't exercised here. Adding aforce=Truecase would close the loop on this guard's full contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auto_install.py` around lines 425 - 445, The current test only covers the non-force downgrade guard in maybe_install_manifest, so add a companion assertion for the force override path. Extend test_maybe_install_manifest_never_downgrades (or add a nearby test) to call auto_install.maybe_install_manifest with force=True and verify that it does restamp and update settings even when installed_version is older. Keep the existing symbols auto_install.maybe_install_manifest, auto_install.read_stamp, and write_stamp as the anchor points for the new coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aelfrice/auto_install.py`:
- Around line 359-382: The version comparison in _version_key() and
_is_downgrade() is using a custom tuple parser that does not follow PEP 440, so
prereleases and normalized versions can be misordered. Replace the current logic
in these helpers with packaging.version.Version comparisons so installed_version
and prev are evaluated with proper PEP 440 semantics, while still preserving the
_UNSTAMPED sentinel check in _is_downgrade().
In `@src/aelfrice/lifecycle.py`:
- Around line 352-357: The `_running_from_uv_tool()` check is using string
prefix matching, which can falsely match paths like uv tools worktrees; update
this logic to resolve `sys.prefix` and `sys.executable` and use the existing
path containment helper instead of `startswith()`. Keep the `uv_tools_root`
guard, but route the `candidate` checks through the containment function so only
the actual uv tools install is treated as true.
---
Nitpick comments:
In `@tests/test_auto_install.py`:
- Around line 425-445: The current test only covers the non-force downgrade
guard in maybe_install_manifest, so add a companion assertion for the force
override path. Extend test_maybe_install_manifest_never_downgrades (or add a
nearby test) to call auto_install.maybe_install_manifest with force=True and
verify that it does restamp and update settings even when installed_version is
older. Keep the existing symbols auto_install.maybe_install_manifest,
auto_install.read_stamp, and write_stamp as the anchor points for the new
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bd78429-9a23-4454-8400-70cf354ca931
📒 Files selected for processing (4)
CHANGELOG/v3.mdsrc/aelfrice/auto_install.pysrc/aelfrice/lifecycle.pytests/test_auto_install.py
|
[claim:review:Setr:2026-07-01T23:32:54Z] |
|
Reviewed — LGTM, merging. Both parts of the issue's suggested fix land cleanly:
Regression coverage is on point (process-vs-presence split, under-root true case, never-downgrade). FF-clean on main, single signed commit, all CI green. |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Setr:2026-07-01T23:34:35Z] |
|
merge-train: blocked 2 review thread(s) are unresolved on these files: src/aelfrice/auto_install.py, src/aelfrice/lifecycle.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
…1044 review) _running_from_uv_tool matched sys.prefix/sys.executable against the uv tools root with a bare startswith, so a sibling like .../uv/toolshed would satisfy the prefix test against .../uv/tools and be misread as the uv-tool install. Anchor the comparison with a trailing separator so only true descendants match. Regression test covers the sibling case.
|
Review-thread dispositions (reviewer: unblocking for merge): Path-containment (Sourcery + CodeRabbit, `lifecycle.py`) — valid, fixed in `eed247b8`. `_running_from_uv_tool()` now anchors the uv-tools-root comparison with a trailing separator, so siblings like `.../uv/toolshed` and `.../uv/tools-worktree` no longer satisfy the prefix test. Added a regression test for the sibling case. PEP 440 version comparison (CodeRabbit, `auto_install.py`) — declined, with reason:
Resolving all three threads. |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged eed247b → |
1 participant
Fixes #1044.
Problem
The #834 guard (a worktree's
uv run aelfmust not rewrite the user's global~/.claude/settings.jsonhooks) was inert for any user who also had auv toolinstall.lifecycle._is_uv_tool_install()short-circuitsTrueon a filesystem-presence check —~/.local/share/uv/tools/aelfrice/existing anywhere on the box — before the correctsys.prefix-under-tools-root check runs. So a source worktree (whosesys.prefixis its own venv, not the uv-tool env) was misclassified as the uv-tool install, and everyaelfinvocation from the tree silently rewrote the global hooks backwards (observed v3.8.0 → v3.6.0, 8 default-on hooks reset).Fix
lifecycle._running_from_uv_tool()returns True iffsys.prefix/sys.executableresolves under the uv tools root.auto_install.is_running_from_uv_tool_install()now delegates to it._is_uv_tool_install()keeps its exists-or-running semantics forupgrade_advice(), which legitimately asks "is a uv-tool install present on this box?" (the two callers want different questions, so they no longer share one predicate).maybe_install_manifestnow skips the merge (non-force path) when the running version is older than the on-disk stamp, emitting a one-line notice instead of stamping backwards. An explicitaelf setup(force=True) still re-stamps for a genuine downgrade.Tests
test_running_from_uv_tool_distinguishes_process_from_presence— the exact repro: uv-tool dir exists on disk butsys.prefixis a worktree venv →_is_uv_tool_install()True (presence),_running_from_uv_tool()False (process), gate stays False.test_running_from_uv_tool_true_under_tools_root— process under tools root → True.test_maybe_install_manifest_never_downgrades— older running version leaves stamp + settings untouched.🤖 Generated with Claude Code
Summary by Sourcery
Guard hook auto-installation based on the currently running uv-tool process instead of mere disk presence, and prevent non-forced runs from downgrading previously installed hook versions.
Bug Fixes:
uv run aelfinvocations no longer rewrite or downgrade the user's globally installed hooks when a separate uv-tool install exists on disk.maybe_install_manifestfrom downgrading the on-disk hook version when a lower version of the tool is running, unless explicitly forced via setup.Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests