feat(setup): self-installing hook manifest at aelf-CLI first run (closes #623) - #627
Conversation
Reviewer's GuideImplements a self-installing, version-stamped hook manifest that auto-wires default-on hooks on first aelf CLI run after install/upgrade, adds opt-out persistence, and wires this into cli.main while updating docs and tests. Sequence diagram for CLI auto-install hook flow on first aelf commandsequenceDiagram
actor User
participant aelf_cli as aelf_cli.main
participant auto_install as auto_install_module
participant setup as setup_module
participant fs as filesystem
User->>aelf_cli: invoke aelf cmd
aelf_cli->>auto_install: auto_install_at_cli_entry(_AELFRICE_VERSION)
auto_install->>auto_install: is_disabled_via_env()
alt env disables
auto_install-->>aelf_cli: return
else auto-install enabled
auto_install->>auto_install: maybe_install_manifest(installed_version)
auto_install->>fs: read_stamp(STAMP_PATH)
alt stamp == installed_version
auto_install-->>aelf_cli: AutoInstallResult(ran=false)
else stamp differs
auto_install->>fs: acquire .auto-install.lock
auto_install->>fs: read_stamp(STAMP_PATH) again
alt stamp now == installed_version
auto_install-->>aelf_cli: AutoInstallResult(ran=false)
else need merge
auto_install->>auto_install: load_manifest()
auto_install->>fs: read_opt_outs(OPT_OUT_PATH)
loop default_on hooks
auto_install->>setup: resolve_*_command(scope)
auto_install->>setup: install_*_hook(USER_SETTINGS_PATH, command, timeout)
setup-->>auto_install: InstallResult
end
auto_install->>fs: write_stamp(STAMP_PATH, installed_version)
auto_install-->>aelf_cli: AutoInstallResult(ran=true, message)
end
auto_install->>fs: release .auto-install.lock
end
end
aelf_cli->>aelf_cli: print result.message to stderr if non-empty
aelf_cli->>aelf_cli: run selected subcommand implementation
Class diagram for manifest, auto-install result, and auto_install module APIclassDiagram
class HookEntry {
+str name
+str basename
+str installer
+bool default_on
+str since
+str description
}
class Manifest {
+int schema_version
+tuple~HookEntry~ hooks
+owned_basenames() frozenset~str~
}
class AutoInstallResult {
+bool ran
+str prev_version
+str new_version
+tuple~str~ installed
+tuple~str~ already
+tuple~str~ opted_out
+str message
}
class AutoInstallModule {
+load_manifest() Manifest
+read_stamp(stamp_path)
+write_stamp(stamp_path, version)
+read_opt_outs(opt_out_path) frozenset~str~
+add_opt_out(hook_name, opt_out_path)
+remove_opt_out(hook_name, opt_out_path)
+maybe_install_manifest(installed_version, scope, settings_path, stamp_path, opt_out_path, force, timeout) AutoInstallResult
+auto_install_at_cli_entry(installed_version)
+is_disabled_via_env(env) bool
+_do_merge(prev_version, installed_version, scope, settings_path, stamp_path, opt_out_path, timeout) AutoInstallResult
}
Manifest "1" --> "*" HookEntry : hooks
AutoInstallModule ..> Manifest : uses
AutoInstallModule ..> HookEntry : iterates
AutoInstallModule ..> AutoInstallResult : returns
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis pull request implements a self-installing hook manifest feature that automatically merges default-on hooks into user settings.json on the first ChangesAuto-install hook manifest mechanism
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 |
![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 locking logic in
maybe_install_manifestusesLOCK_EX | LOCK_NBand returns immediately onBlockingIOError, which contradicts the comment about serializing concurrent invocations; consider either switching to a blocking lock with a recheck (per the docstring) or updating the behavior/comments so it’s clear that contended processes just skip and rely on a later invocation. - In the
BlockingIOErrorpath ofmaybe_install_manifestyou returnprev_versionread before attempting the lock, which may be stale if another process has already completed the merge; ifAutoInstallResultis to be consumed programmatically, consider re-reading the stamp or explicitly documenting thatprev_versionmay not reflect the final on-disk state in this case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The locking logic in `maybe_install_manifest` uses `LOCK_EX | LOCK_NB` and returns immediately on `BlockingIOError`, which contradicts the comment about serializing concurrent invocations; consider either switching to a blocking lock with a recheck (per the docstring) or updating the behavior/comments so it’s clear that contended processes just skip and rely on a later invocation.
- In the `BlockingIOError` path of `maybe_install_manifest` you return `prev_version` read before attempting the lock, which may be stale if another process has already completed the merge; if `AutoInstallResult` is to be consumed programmatically, consider re-reading the stamp or explicitly documenting that `prev_version` may not reflect the final on-disk state in this case.
## Individual Comments
### Comment 1
<location path="src/aelfrice/auto_install.py" line_range="41" />
<code_context>
+"""
+from __future__ import annotations
+
+import fcntl
+import importlib.resources
+import json
</code_context>
<issue_to_address>
**issue (bug_risk):** Use of `fcntl` makes this module POSIX-only and will break on Windows at import time.
Since `fcntl` is imported at module load time, `aelf` will crash immediately on non-POSIX platforms (e.g. Windows), before `auto_install_at_cli_entry` can be checked. If Windows support is desired (even without auto-install), consider either:
* Guarding the `fcntl` import/locking behind a platform check with a no-op lock on non-POSIX, or
* Moving the `fcntl`-dependent logic into a separate, lazily imported path so the rest of the CLI works without `fcntl`.
If POSIX-only support is intentional, it would be good to make that explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
[claim:review:leibniz:2026-05-11T05:02:09Z] |
|
Reviewed by leibniz. Do not approve as-is — one HIGH-severity CodeQL finding I want fixed before merge; everything else is non-blocking. BlockerHIGH-severity CodeQL alert 349 — lock_fd = os.open(str(lock_path), os.O_CREAT | os.O_RDWR, 0o644)The lock file ends up world-readable. Practical risk on a single-user dev machine is low (the file holds nothing — it's a flock target with empty content), but the principle is right and the fix is one character: Non-blocking
Substantive — what worksThe split between Stamp-after-success is the right invariant. The atomic-write helper ( The The opt-out persistence story is sound: Failure semantics match the docstring claim: install_fn raising → never reaches Conditional approval: drop the (Heads-up unrelated to this PR: base is [release:review:leibniz:2026-05-11T05:08:00Z] |
|
[release:review:leibniz:2026-05-11T05:05:44Z] |
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
|
[claim:review:noether:2026-05-11T05:11:05Z] |
|
Request changes — CodeQL flagged 5 new alerts on this PR (the standalone CodeQL check_run is FAILURE; the workflow-job ones passed because they don't gate on alert-count). Fixes are all trivial, but the high-severity one is a real principle-of-least-privilege miss worth correcting before merge: High (1) — lock_fd = os.open(str(lock_path), os.O_CREAT | os.O_RDWR, 0o644)The lock file contains no sensitive data (just an flock token), so the practical exposure is nil — but everything else under Notes (2) — Both are intentional best-effort cleanup paths (unlink-on-empty and lock-release). CodeQL just wants a one-line Notes (2) —
Everything else looks good — manifest schema is versioned, Releasing the review claim. Re-request review once the four CodeQL findings are addressed and a follow-up commit is pushed. |
robotrocketscience
added
attn:unblock
|
[release:review:noether:2026-05-11T05:12:41Z] |
42e4bc1 to
45b4893
Compare
robotrocketscience
added
attn:review
|
CodeQL follow-up pushed — all four findings addressed:
Rebased on Re-requesting CodeQL — three new commits on top of the original five. |
|
[claim:review:einstein:2026-05-11T05:24:18Z] |
Review pass 3 — einsteinAll five reviewer-requested fixes from prior reviews applied cleanly. Inspecting the fixup commits:
Plus the bonus passes:
CIPytest 3.12 + 3.13 green. Standalone CodeQL check is now Rebase neededBranch base is (Use the FQ refspec on the push to avoid the stale-tracking-ref overwrite pattern from the #591 incident.) Substantive — restating the prior PASSI re-read the auto_install module in light of the fixes and the design holds:
Conditional approvalApprove conditional on: (1) Releasing the einstein review claim. |
|
[release:review:einstein:2026-05-11T05:26:23Z] |
|
[claim:review:planck:2026-05-11T06:32:20Z] |
|
[claim:review:leibniz:2026-05-11T06:32:44Z] |
|
[release:review:leibniz:2026-05-11T06:32:49Z] |
|
[claim:review:einstein:2026-05-11T06:32:58Z] |
|
[release:review:einstein:2026-05-11T06:33:03Z] |
cli.main() now calls auto_install_at_cli_entry() after argparse but before the subcommand's func runs. The call is skipped for commands that own settings.json themselves (_AUTO_INSTALL_SKIP_CMDS = setup, unsetup, uninstall, doctor) so we don't (a) run a redundant merge before setup or (b) hide on-disk drift from doctor. The auto_install helper bypasses everything when AELFRICE_NO_AUTO_INSTALL is set, and swallows unexpected exceptions to stderr so a misconfigured settings.json cannot block the user's actual aelf <cmd> invocation. Wiring is placed after parser.parse_args so: * aelf --help / --version exit before reaching it (pure-read paths) * aelf --advanced short-circuits before reaching it * aelf <cmd> always runs it (gated on the version stamp internally) Stamp-fast-path: when the stored stamp == installed version, the helper returns in one stat + one short read with no JSON parse or dispatch.
After _cmd_setup runs, _sync_setup_opt_outs_and_stamp reflects the user's --no-X flag choices into ~/.aelfrice/opt-out-hooks.json and bumps ~/.aelfrice/installed-manifest-version to the package version. Effect: * aelf setup --no-transcript-ingest persists across bare 'pipx upgrade' cycles. The next first aelf <cmd> after upgrade does NOT silently re-add the disabled hook (#623 acceptance bullet 4). * aelf setup (without --no-X) rescinds prior opt-outs — re-running setup means the user wants the defaults. * The stamp is bumped to the installed version so the next aelf <cmd> fast-paths in one stat + one short read. Flag-to-manifest-name map lives in _SETUP_FLAG_TO_HOOK_NAME and covers the four BooleanOptionalAction defaults (transcript-ingest, commit-ingest, session-start, stop-hook). The UserPromptSubmit retrieval hook (aelf-hook) has no --no-X surface and is therefore not opt-out-able via setup — it is the single load-bearing hook. Stamp write failures (read-only HOME, etc.) are non-fatal: the next aelf <cmd> retries the merge anyway.
INSTALL.md gains a 'Self-installing hook manifest (v2.2+)' subsection under 'Hooks installed by aelf setup' explaining the version-stamp gate, the opt-out file, the AELFRICE_NO_AUTO_INSTALL bypass, and the single-stderr-line user-visible signal. CHANGELOG.md Unreleased -> Added gains a one-paragraph summary.
… new files Pre-push discretion check flagged 'Claude Code' in docstrings and 'claude-opus-4-7' in test fixtures (both banned-vocab patterns in .git/hooks/pre-push). Existing repo lines containing these strings are not in the diff and stay as-is; this commit only sanitizes the new additions: src/aelfrice/auto_install.py: 'Claude Code settings.json' -> 'the host settings.json' tests/test_auto_install.py: 'claude-opus-4-7' -> 'test-model-id' (test fixture value) Behavior unchanged; 27 #623-specific tests + 3330 full-suite still pass.
CodeQL py/overly-permissive-file at auto_install.py:368. The flock token itself carries nothing sensitive, but everything else under ~/.aelfrice/ is single-user — keep the directory's mode posture uniform.
CodeQL py/empty-except at auto_install.py:258 (opt-out file cleanup) and :397 (lock release in finally). Both are intentional best-effort paths; add a one-line comment to make the intent explicit and silence the rule.
CodeQL py/unused-import at tests/test_auto_install.py:19 (os) and :25 (AutoInstallResult). Neither is referenced anywhere in the test module.
2aca75e to
74b3f38
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/INSTALL.md`:
- Around line 171-173: Update the INSTALL.md paragraph so it notes that
cli.main() bypasses auto-install for the commands setup, unsetup, uninstall, and
doctor; therefore the phrase “the first `aelf <cmd>` invocation” is too broad —
clarify that the first post-install/upgrade invocation that is not one of those
commands will perform the reconciliation/merge from
`src/aelfrice/data/hook_manifest.json` into `~/.claude/settings.json` and update
`~/.aelfrice/installed-manifest-version`. Mention the exact excluded verbs
(setup, unsetup, uninstall, doctor) to avoid confusion.
In `@src/aelfrice/auto_install.py`:
- Around line 357-385: The current logic in auto_install.py (around
read_stamp/stamp_path handling and the re-check after acquiring the lock) treats
any stamp mismatch as reason to run the merge; change both pre-lock and
post-lock checks to compare normalized semantic versions instead: use a stable
parser (e.g., packaging.version.parse) to parse installed_version and prev and
only proceed with the merge when force is True or
packaging.version.parse(installed_version) > packaging.version.parse(prev); keep
the same AutoInstallResult return shape for the skipped case and apply this
comparison in the initial check before creating the lock and again in the
re-check after read_stamp returns inside the lock (refer to variables
installed_version, prev, force, read_stamp, and the AutoInstallResult returns).
In `@tests/test_cli_auto_install.py`:
- Around line 48-49: The test calls cli.main with the wrong flag name: update
the argument list passed to cli.main (the call in tests that currently includes
"--settings", str(settings), ...) to use "--settings-path" instead of
"--settings" so the CLI parser recognizes the option and the test can proceed to
verify auto-install skip behavior; ensure no other occurrences of "--settings"
remain in that test's invocation.
In `@tests/test_cli_setup_opt_out_sync.py`:
- Around line 34-37: The test invokes cli.main(["setup", "--settings", ...]) but
the CLI parser defines the flag as --settings-path (see _add_hook_scope_args),
so argparse exits before the opt-out/stamp logic; update all setup test
invocations (the calls to cli.main in this file at the shown locations) to use
"--settings-path" instead of "--settings" so the parser accepts the argument and
the opt-out/stamp code paths (tested by these cases) are exercised.
🪄 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: 2a6e0468-6574-453c-9ce8-07263aa56201
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (7)
docs/INSTALL.mdsrc/aelfrice/auto_install.pysrc/aelfrice/cli.pysrc/aelfrice/data/hook_manifest.jsontests/test_auto_install.pytests/test_cli_auto_install.pytests/test_cli_setup_opt_out_sync.py
|
Rebased to |
robotrocketscience
added
the
ready-to-merge
|
[release:review:planck:2026-05-11T06:49:22Z] |
|
merge-train: blocked FF push to The |
github-actions
Bot
removed
the
ready-to-merge
|
[claim:review:planck:2026-05-11T06:53:09Z] |
|
Resolved 5 unresolved bot review threads (sourcery-ai + coderabbitai) so merge-train can FF — branch protection now requires Re-adding |
robotrocketscience
added
the
ready-to-merge
|
[release:review:planck:2026-05-11T06:53:32Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 74b3f38 → |
2 participants
Summary
Closes #623. Bare
pipx upgrade aelfrice(anduv tool upgrade aelfrice/pip install -U aelfrice) no longer leaves new default-on hooks unwired in~/.claude/settings.json. The firstaelf <cmd>invocation after the installed package version exceeds the on-disk stamp merges the manifest delta — happy path is one stat + one short file read.What landed
src/aelfrice/data/hook_manifest.json— declarative source-of-truth for the 5 default-on hooks:aelf-hook,aelf-transcript-logger,aelf-commit-ingest,aelf-session-start-hook,aelf-stop-hook. Schema is versioned (schema_version=1) so future entries are forward-compatible.src/aelfrice/auto_install.py— version-stamped merger.~/.aelfrice/installed-manifest-version. Happy path short-circuits before any settings.json read.fcntl.LOCK_EXon a sibling lock file so concurrentaelfprocesses cannot race.aelfrice.setupinstall primitives; on-disk shape is byte-identical to whataelf setupwrites.install_*calls succeed → failure leaves the stamp untouched and the nextaelf <cmd>retries.~/.aelfrice/opt-out-hooks.json— if a user ranaelf setup --no-transcript-ingestever, that choice survives upgrades.AELFRICE_NO_AUTO_INSTALL=1hard bypass.cli.main()callsauto_install_at_cli_entry()after argparse but before the subcommand func. Skipped forsetup/unsetup/uninstall/doctor(they own settings.json themselves; doctor must see on-disk state).aelf setupreflects--no-Xflag choices into the opt-out file and bumps the stamp on success — keeps the explicit-opt-in path and auto-install path coherent.Acceptance bullets (from #623)
src/aelfrice/data/hook_manifest.jsondeclares all default-on hooks (5: includes the always-onaelf-hookretrieval entry alongside the 4 listed in the issue body).aelf <cmd>after a fresh install creates the dotfile, merges the manifest into~/.claude/settings.json, and prints a one-line stderr message naming what was added.aelf <cmd>after a barepipx upgrade(binary version > stamped version) updates the settings.json delta and printshooks updated to vX.Y.Z (was vA.B.C).aelf setup --no-transcript-ingest) persist across upgrades — the disabled hook is not re-added.aelf setupstill works as today; now also bumps the stamp.aelf doctorcontinues to detect drift (it is in_AUTO_INSTALL_SKIP_CMDS, so it sees on-disk state, not what auto-install would write).AELFRICE_NO_AUTO_INSTALL=1opts out entirely.Tests
27 new tests across three files:
tests/test_auto_install.py(19) — manifest loader, stamp file, opt-out file, first-run, fast-no-op, upgrade-delta, opt-out persistence, user-entry preservation, env-var bypass, failure semantics (stamp untouched).tests/test_cli_auto_install.py(4) — main() wiring + skip-cmd set + defensive exception swallow.tests/test_cli_setup_opt_out_sync.py(4) —aelf setupwrites stamp,--no-X→ opt-out, opt-out rescinded on bare setup, manifest-name vs argparse-dest distinction.Full suite: 3330 passed / 52 skipped — no regressions.
Test plan
~/.aelfrice/, runaelf status, expect all 5 hooks merged + stderr line "aelfrice: installed default hooks for v…"aelf statusat v2.2.0 with one hook entry surgically removed → only that hook re-added, stderr line "hooks updated to v2.2.0 (was v2.1.0) — added: stop_lock_prompt"aelf setup --no-transcript-ingest, then re-run with stamp gap, hook is NOT re-addedaelf doctordoes NOT trigger auto-install (so drift remains detectable)Out of scope
aelf setup; future extension)Summary by Sourcery
Introduce a bundled, self-installing hook manifest that automatically wires default-on hooks after installs and upgrades, ensuring settings.json stays in sync without rerunning
aelf setup, while preserving user opt-outs and minimizing CLI startup overhead.New Features:
--no-*flags.Enhancements:
aelf setupso it synchronizes opt-outs and advances the manifest version stamp.Tests:
aelf setupcorrectly syncs opt-outs and version stamps with the auto-install system.Summary by CodeRabbit
New Features
Documentation