Skip to content

fix(auto-install): gate cli-entry on uv-tool install context (#834) - #836

Merged
github-actions[bot] merged 3 commits into
mainfrom
fix/issue-834-auto-install-skip-non-uv-tool
May 15, 2026
Merged

fix(auto-install): gate cli-entry on uv-tool install context (#834)#836
github-actions[bot] merged 3 commits into
mainfrom
fix/issue-834-auto-install-skip-non-uv-tool

Conversation

@robotrocketscience

@robotrocketscience robotrocketscience commented May 15, 2026

Copy link
Copy Markdown
Owner

Closes #834.

Bug

auto_install_at_cli_entry ran on every CLI invocation, including uv run aelf <cmd> from a project worktree's local .venv. With ~/.aelfrice/installed-manifest-version stamped at one version and a worktree pinned to a different one (the routine multi-worktree workflow), the merge ran against the worktree's bundled manifest and rewrote the user's global ~/.claude/settings.json — re-pinning hook commands to the worktree's .venv paths and re-stamping the version backwards. The "opted out: …" line in the bug report is a separate symptom: the user already had those six hooks in ~/.aelfrice/opt-out-hooks.json (from a prior aelf setup --no-X); the merge correctly skipped them, but the surrounding stamp downgrade and settings rewrite happened without consent.

Fix

Option A from the issue body. New is_running_from_uv_tool_install() delegates to the existing lifecycle._is_uv_tool_install (single-source detection: ~/.local/share/uv/tools/aelfrice/ exists, or sys.prefix resolves under the uv tools root). auto_install_at_cli_entry gates on it after the existing AELFRICE_NO_AUTO_INSTALL env check.

After this PR:

context auto-install fires
aelf <cmd> from uv tool install aelfrice yes
uv run aelf <cmd> from a worktree .venv no (was: yes)
contributor pytest against checked-out source no (was: yes)
python -m aelfrice.cli from a system Python no (was: yes)
aelf setup (explicit) yes — setup calls maybe_install_manifest directly, gate doesn't apply

Symmetric with the existing AELFRICE_NO_AUTO_INSTALL=1 env override; the new gate is the implicit version of the same escape hatch for non-uv-tool contexts.

Tests

Three new tests in tests/test_auto_install.py:

  • test_auto_install_at_cli_entry_skips_when_not_uv_tool — gate False → no stamp written, no settings.json created. Direct regression reproducer.
  • test_is_running_from_uv_tool_install_delegates_to_lifecycle — confirms both branches of the gate flow through lifecycle._is_uv_tool_install so detection stays in one place.
  • test_auto_install_at_cli_entry_runs_when_uv_tool — gate True path still invokes maybe_install_manifest. Uses a stub rather than the real merge: maybe_install_manifest binds STAMP_PATH / OPT_OUT_PATH at function-definition time, so an end-to-end test would leak to the user's actual ~/.aelfrice/.

test_cli_auto_install.py::test_auto_install_at_cli_entry_does_not_propagate_exceptions updated to monkeypatch the new gate to True so the post-gate exception-handling path is still exercised.

Full suite: 4271 passed, 62 skipped, 75 xfailed (pytest -x -q --timeout=120, run during pre-PR gate).

Out of scope

  • The demotion_pressure IndexError ([bug] aelf v3.1.0 crashes on any DB touched by post-#814 code — demotion_pressure schema/reader skew #833 — being worked separately).
  • Refactoring maybe_install_manifest so monkeypatching auto_install.STAMP_PATH / OPT_OUT_PATH propagates into the merge path. The default-arg binding is a real test-pollution hazard but not the user-facing bug; flagging here as a follow-up.
  • Fix B from the issue body (refuse to downgrade without consent). Fix A is the strictly stronger guarantee — Fix A skips entirely; Fix B would still permit a uv-tool downgrade. The Out-of-Worktree contract is the load-bearing property.

Summary by CodeRabbit

  • Bug Fixes
    • Auto-install functionality now executes exclusively for uv tool installations, preventing unintended modifications to global configuration and settings files when running from project worktrees with incompatible or mismatched manifest versions. The existing AELFRICE_NO_AUTO_INSTALL environment variable override remains fully functional, and test coverage has been expanded to validate the new behavior.

Review Change Stack

@robotrocketscience robotrocketscience added the author-oppenheimer Author label for oppenheimer session label May 15, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown

PR-size soft cap

This PR is over the advisory size threshold:

  • 117 changed lines (limit: 200)
  • 4 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@yoshi280 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 23 minutes and 49 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 81ab57b6-ce4b-4a2d-9df9-b7dbce3444c6

📥 Commits

Reviewing files that changed from the base of the PR and between c40550a and d91c5fc.

📒 Files selected for processing (4)
  • CHANGELOG/v3.md
  • src/aelfrice/auto_install.py
  • tests/test_auto_install.py
  • tests/test_cli_auto_install.py
📝 Walkthrough

Walkthrough

This PR gates the auto-install manifest merge to only execute when aelfrice runs from the user's installed uv tool copy, preventing worktree-resolved versions from silently rewriting global settings.

Changes

UVTool Install Context Gating

Layer / File(s) Summary
UVTool context detection and entrypoint gating
src/aelfrice/auto_install.py, tests/test_auto_install.py, tests/test_cli_auto_install.py, CHANGELOG/v3.md
A new is_running_from_uv_tool_install() helper delegates to lifecycle._is_uv_tool_install() to detect the execution context. The auto_install_at_cli_entry() function now gates its manifest merge behind this check, returning early without file access when running from a worktree venv. New test coverage validates the short-circuit path, helper delegation, and normal flow execution when the gate is open; an existing exception-handling test is updated to force the gate open for its scope.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

author-Faraday

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(auto-install): gate cli-entry on uv-tool install context (#834)' clearly and concisely summarizes the primary change: gating auto-install on uv-tool context to prevent unintended mutations of global settings.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering bug rationale, the chosen fix approach, behavior matrix, test coverage, and out-of-scope items. It aligns with the template's purpose of explaining the 'why' and implementation details.
Linked Issues check ✅ Passed All three coding objectives from #834 are met: detection of uv-tool vs worktree context via is_running_from_uv_tool_install() [added], gating of auto-setup on that detection [implemented], and regression test coverage [three new tests in test_auto_install.py plus updated existing test].
Out of Scope Changes check ✅ Passed All changes are directly aligned with the gating logic and testing requirements for #834. The PR author explicitly notes out-of-scope items (demotion_pressure IndexError, maybe_install_manifest refactoring, alternative fix approaches) and does not attempt to address them.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-834-auto-install-skip-non-uv-tool

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@robotrocketscience robotrocketscience added the attn:review Needs review (PR open, awaiting reviewer) label May 15, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:prince:2026-05-15T01:49:10Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Reviewed 51d1d2a80f78c23dc40550a0. Three atomic signed commits; CI green; mergeState=CLEAN.

Fix is correct. is_running_from_uv_tool_install() delegates to lifecycle._is_uv_tool_install, which is the existing single-source detection (~/.local/share/uv/tools/aelfrice/ exists or sys.prefix resolves under the uv tools root). The local import inside the function avoids the early-CLI-bootstrap circular import. Gate fires after AELFRICE_NO_AUTO_INSTALL so the env override still short-circuits first; it's the implicit version of the same escape hatch for non-uv-tool contexts.

Tests cover the right axes:

  • test_auto_install_at_cli_entry_skips_when_not_uv_tool — gate False → no stamp, no settings.json. Direct regression reproducer for [bug] uv run aelf from a worktree silently mutates ~/.claude/settings.json and opts user out of hooks #834. Monkeypatches STAMP_PATH/OPT_OUT_PATH/USER_SETTINGS_PATH to tmp_path so leakage to ~/.aelfrice/ is impossible.
  • test_is_running_from_uv_tool_install_delegates_to_lifecycle — pinpoints the lifecycle seam, exercises both branches. Catches the failure mode where someone adds a second uv-tool-detection path that drifts from the single-source.
  • test_auto_install_at_cli_entry_runs_when_uv_tool — gate True still invokes maybe_install_manifest. Stub-based, with a comment explaining why end-to-end isn't run (the STAMP_PATH/OPT_OUT_PATH are bound at function-definition time → real-merge leaks to user's ~/.aelfrice/). That's a known sharp edge in the surrounding test architecture; the test takes the cautious path.
  • Existing test_auto_install_at_cli_entry_does_not_propagate_exceptions was updated to monkeypatch the new gate to True — keeps the post-gate exception path exercised.

Changelog entry is accurate: notes that the "opted out: …" line in the bug report was a pre-existing ~/.aelfrice/opt-out-hooks.json condition, not caused by the bug — the bug was the surrounding stamp downgrade and settings rewrite. Lands under [Unreleased] → Fixed (correct — main release queue will migrate when the next release dates this section).

Behavior matrix from the PR body matches my read of the diff:

  • aelf <cmd> from uv tool install → fires (gate True)
  • uv run aelf from worktree .venv → skipped (gate False, was: ran)
  • contributor pytest → skipped (gate False, was: ran)
  • python -m aelfrice.cli from system Python → skipped (gate False, was: ran)
  • aelf setup → still works (calls maybe_install_manifest directly, bypasses the entry helper)

Minor — out of scope, worth noting:

  • The function-definition-time binding of STAMP_PATH / OPT_OUT_PATH that forced the stub-based test pattern is a real testability foot-gun. Hoisting them to function kwargs (or reading at call time) would let future tests exercise the real merge path with tmp_path overrides. Not gating this PR; worth a small follow-up.
  • Option B from the issue ("refuse to downgrade without consent") still has the standalone value the issue noted — but it's the right call to ship A first (smaller blast radius, plus B is additive to A). If we want to revisit, a separate issue for "warn-on-downgrade in aelf setup itself" would catch the explicit-invocation case too.

Adding ready-to-merge.

@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label May 15, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:prince:2026-05-15T01:50:56Z]

@github-actions

Copy link
Copy Markdown

merge-train: blocked

branch is not fast-forward on main (branch base 9cbbc7a05d0250ca8619a1f45b02e233c98fb956, current main d664fd72ce8e5a8ce88ee17332406e4b3c5307b1). Rebase locally (git rebase github/main), force-push, and re-add the label.

The ready-to-merge label has been removed. Address the issue above and re-add the label when you're ready for another attempt.

@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label May 15, 2026
A worktree's `uv run aelf <cmd>` resolves to the worktree's local
`.venv` aelfrice — not the user's installed `uv tool` copy — yet
`auto_install_at_cli_entry` ran unconditionally and rewrote the user's
global `~/.claude/settings.json` against the worktree's bundled
manifest. With a stamp at one version and a worktree pinned to a
different version, the merge re-pinned hook commands to the worktree's
`.venv` paths and re-stamped the version backwards.

New `is_running_from_uv_tool_install()` delegates to
`lifecycle._is_uv_tool_install` so detection stays in one place.
`auto_install_at_cli_entry` gates on it after the existing
`AELFRICE_NO_AUTO_INSTALL` env check; explicit `aelf setup` still
calls into `maybe_install_manifest` directly so contributors can opt
in to a worktree merge.
Three new tests in test_auto_install.py:
- test_auto_install_at_cli_entry_skips_when_not_uv_tool — gate False,
  the helper short-circuits before touching stamp / settings.json
  (the regression reproducer for the original bug).
- test_is_running_from_uv_tool_install_delegates_to_lifecycle — both
  branches via the lifecycle._is_uv_tool_install seam.
- test_auto_install_at_cli_entry_runs_when_uv_tool — sanity that the
  gate True path still invokes maybe_install_manifest. Uses a stub
  rather than running the merge end-to-end because
  maybe_install_manifest binds STAMP_PATH / OPT_OUT_PATH at function
  definition time and the real merge path would leak to the user's
  actual ~/.aelfrice/.

Existing test_auto_install_at_cli_entry_does_not_propagate_exceptions
in test_cli_auto_install.py monkeypatches the new gate to True so it
continues to exercise the post-gate exception-handling path.
@yoshi280
yoshi280 force-pushed the fix/issue-834-auto-install-skip-non-uv-tool branch from c40550a to d91c5fc Compare May 15, 2026 01:55
@robotrocketscience robotrocketscience added ready-to-merge Trigger merge-train: FF main to this PR's head and removed attn:review Needs review (PR open, awaiting reviewer) labels May 15, 2026
@github-actions

Copy link
Copy Markdown

merge-train: merged d91c5fcmain via FF push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-oppenheimer Author label for oppenheimer session

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] uv run aelf from a worktree silently mutates ~/.claude/settings.json and opts user out of hooks

1 participant