feat: flip search-tool / search-tool-bash defaults to on (#738) - #742
Conversation
`aelf setup` (no flags) now installs both PreToolUse hooks. The flags follow the `--X / --no-X` BooleanOptionalAction convention used by every other default-on hook (transcript-ingest, commit-ingest, session-start, stop-hook). The old `--search-tool` / `--search-tool-bash` opt-in flags and the asymmetric `--no-search-tool-bash` setup-time uninstall are gone. `aelf unsetup` (no flags) now removes both for symmetry. Use `--no-search-tool[-bash]` on setup to skip + record an opt-out (persisted across upgrades via `~/.aelfrice/opt-out-hooks.json`), or on unsetup to leave the entries in place. Per #738 audit: README §"How it works" advertises four parallel retrieval lanes, but the agent's own Grep/Glob/Bash-search calls skipped them unless the user explicitly opted in. The product as documented and the product as installed are now the same product. Worst-case latency cost: ~22ms / turn for 5 Grep fires (4.5ms p99 each, README:47) + ~12ms / turn for 3 Bash-search fires (capped at 3 per turn via BASH_FIRE_CAP_PER_TURN in hook_search_tool.py:75). No new retrieval code paths; the `retrieve()` call site reused is already validated for UserPromptSubmit fires. Manifest reconcile + tests + docs land in follow-up commits.
) Add `search_tool` and `search_tool_bash` rows to `hook_manifest.json` with `default_on: true` and `since: "3.0.1"`, and register the corresponding install/resolve functions in `auto_install._DISPATCH`. Without this, the cli.py default-on flip in the prior commit only reaches users who explicitly re-run `aelf setup` after upgrading. With it, the first `aelf <cmd>` invocation after `uv tool upgrade aelfrice` reconciles the bumped manifest version against `~/.aelfrice/installed-manifest-version` and merges both new entries into `~/.claude/settings.json` automatically — closing the upgrade-path loop for the new defaults. Opt-outs continue to live in `~/.aelfrice/opt-out-hooks.json`. Users who run `aelf setup --no-search-tool[-bash]` get the opt-out persisted via `_SETUP_FLAG_TO_HOOK_NAME` (added in the prior commit), and the reconciler skips those names on every subsequent run. Both hooks share the `aelf-search-tool-hook` script basename — fine for `owned_basenames()` (frozenset dedup) and reflects the on-disk reality (setup.py:659,665 both point at the same script; the matcher differs).
Three existing tests in `test_aelf_setup_search_tool_bash.py` asserted the old `aelf setup --no-search-tool-bash → removes existing entry` semantic. Under the new BooleanOptionalAction convention (matching transcript-ingest / commit-ingest / session-start / stop-hook), setup-time --no-X skips install + records the opt-out but does NOT actively uninstall. Use `aelf unsetup` for explicit removal. Rewrites: - `*_no_search_tool_bash_removes` → `*_does_not_remove_existing` + new assertion that the bash entry remains. - `*_idempotent_when_absent` → `*_skips_install` (no "no search-tool-bash" removal message on the setup path anymore). - `*_leaves_grep_glob_intact` → `*_installs_grep_glob_only` (single setup call, asserts the asymmetric default). New coverage: - Bare `aelf setup` wires both PreToolUse hooks. - `aelf setup --no-search-tool --no-search-tool-bash` reproduces the pre-3.0.1 no-PreToolUse default. - `aelf setup --no-search-tool` wires search-tool-bash only. - Bare `aelf unsetup` removes both. - `aelf unsetup --no-search-tool` leaves the Grep|Glob hook in place. Opt-out persistence (test_cli_setup_opt_out_sync.py): - `aelf setup --no-search-tool` records `search_tool` in opt-out-hooks.json so manifest reconcile skips it on upgrade. - Same for `search_tool_bash`. - Bare `aelf setup` after a prior --no-X rescinds both opt-outs. Full suite: 3820 passed, 59 skipped, 75 xfailed (76s).
CHANGELOG.md [Unreleased] §Changed: new entry covering the flag flip, the convention shift, the opt-out surface, the auto-install reconcile hook, and the latency budget. docs/INSTALL.md: hooks-installed table now lists search-tool and search-tool-bash as default-on (with the "(v3.0.1+)" annotation), adds the missing stop-lock-prompt row, and the opt-out / opt-in code blocks reflect the new shape. The rebuilder is the only off-by-default hook left. docs/search_tool_hook.md: top-of-doc Status flips from "spec / default-on candidate at v1.3.0" to "shipped; default-on as of v3.0.1." Hook registration section drops the `--search-tool` opt-in language. Historical design-discussion sections (lines 200+) are not retroactively edited — the top-of-doc note tells readers that "default-OFF" framing below pre-dates the flip. README: no changes. The product copy describes the four-lane retrieval stack at the user-perception level and does not mention the `--search-tool` flag.
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent 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 PR implements issue ChangesSearch-tool hooks default-on flip (v3.0.1+)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 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 |
Closes #738.
What
aelf setup(no flags) now installs bothPreToolUse:Grep|GlobandPreToolUse:Bashretrieval hooks. The flags follow the--X / --no-XBooleanOptionalAction convention used by every other default-on hook (transcript-ingest, commit-ingest, session-start, stop-hook). The old--search-tool/--search-tool-bashopt-in flags and the asymmetric--no-search-tool-bashsetup-time uninstall are gone — useaelf setup --no-search-tool[-bash]to skip + persist an opt-out, oraelf unsetup(default-on for both flags) for explicit removal.Why
Per the #738 audit: README §"How it works" advertises four parallel retrieval lanes (L0/L1/L2/L2.5), but the agent's own Grep/Glob/Bash-search calls skipped the retrieval pipeline unless the operator explicitly opted in via
aelf setup --search-tool. The product as documented and the product as installed were not the same product. This PR closes that gap.The
bfs_enabled(L2 graph walk) flip is tracked separately under #739 and is bench-gated. Cross-fire dedup is tracked under #740.Latency budget (per issue body)
search-tool(Grep / Glob)search-tool-bash(grep/rg/find/fd/ack)BASH_FIRE_CAP_PER_TURN)No new retrieval code paths enabled — the
retrieve()call site reused is already validated for UserPromptSubmit fires.Changes (4 atomic commits)
feat(cli)— flag rename + setup/unsetup body defaults flip + opt-out mapping.feat(auto-install)—hook_manifest.json+_DISPATCHso firstaelf <cmd>after upgrade auto-installs the two new entries.test— rewrite 3 broken tests (old--no-search-tool-bash → uninstallsemantic), add coverage for the new defaults + opt-out persistence.docs—CHANGELOG.md [Unreleased] §Changed,docs/INSTALL.mdhooks table + opt-out block,docs/search_tool_hook.mdtop-of-doc status flip.Verification
aelf setup --helpshows both--X / --no-Xforms for both flags.aelf setup(no flags) installs both hooks (new test).aelf setup --no-search-tool --no-search-tool-bashreproduces the pre-3.0.1 no-PreToolUse default (new test).aelf setup --no-search-tool[-bash]writes the opt-out to~/.aelfrice/opt-out-hooks.json(new tests intest_cli_setup_opt_out_sync.py).aelf unsetupremoves both (new test).test_first_run_creates_stamp_and_writes_all_hooksset-inclusion assertion covers it; newsearch_tool/search_tool_bashnames appear inresult.installed).Full test suite: 3820 passed, 59 skipped, 75 xfailed in 76s — no regressions.
Out of scope
bfs_enableddefault flip (Flip default: bfs_enabled (L2 graph-walk lane) on by default — gated on v3.0 latency bench re-run #739) — bench-gated.Summary by CodeRabbit
New Features
--no-search-tooland--no-search-tool-bashopt-out flags.Documentation
Tests