Repository navigation
Make install hooks opt-in via --hooks - #753
Andy11-cpu wants to merge 1 commit into
Conversation
PreToolUse and SessionStart augmenter hooks are no longer installed by default. Pass --hooks to install to opt in. Install plan receipts omit hook paths unless --hooks is set. Signed-off-by: Andy11-cpu <canada11@duck.com>
|
Thanks for splitting out the hook opt-in behavior. Triage: installer/editor-integration UX change, currently conflicting. Please rebase on current |
|
Thank you for this, and for splitting it out of #625 as asked — that made it reviewable on its own terms. What this actually is: a product-direction question, not a code review. Making hooks opt-in changes what every user of the documented one-line install gets. The performance motivation is real — a process spawn per Grep and Glob is not nothing — but the hooks are also the mechanism that steers agents toward the graph rather than grepping, which is this project's whole findability-first premise. Making them opt-in means the majority who use the one-liner silently lose that steering layer. Both positions are defensible, and it is not mine to decide. I have put it to the maintainer directly: hooks opt-in (performance-polite) or opt-out (findability-first default)? Everything else here is fixable mechanics that follow from that answer. Your test is well-placed. One thing I would want changed regardless of which way the decision goes, because it is a genuine design gap rather than a preference:
Three smaller things, all consequences of it being a user-facing default change:
On staleness, so you can plan: every signature you touch has moved. Please hold off reworking until the direction answer arrives. Between the framework rebuild and the smoke changes this needs, it is a substantial piece of work, and it would be unfair to have you do it against a decision that might go the other way. I will come back to you either way. |
|
No worries. Havent had a chance to catch up with this , i have been using a heavily modified version of this as a daily driver but the set up is something pretty manual and not very agentic in my opinion ( the way i like it ) . if you need anything though let me know , would like to pitch in 100% |
|
The direction question has an answer — thank you for waiting on it, and sorry it took this long. Verdict: keep hooks on by default, add an opt-OUT flag, and persist the choice. The reasoning, so it's not just a verdict: those hooks are the mechanism that steers an agent toward the graph instead of grep. If they become opt-in, the overwhelming majority of installs silently never get graph-first behaviour, and the product quietly becomes worse for everyone who didn't read the flag list. But your complaint is real and you measured it — per-call process-spawn overhead is a genuine cost, not a preference — so the escape hatch should exist. It just needs to point the other way. Concretely, what we'd merge: 1. 2. The choice persists. This is the part that matters most and wasn't in the original PR. There's an existing per-user key-value store —
That last point is what makes it real: 3. Every hook site must be gated, not just Claude Code's. This is the part I'd most want you to know before starting, because I went looking and it's larger than it appears. Hook writes live in at least: Claude Code (gate + session + subagent), Gemini CLI (before-tool + after-tool + session), Codex, Copilot/VS Code, Qoder, GitLab Duo, Devin, Kimi, Hermes, Qwen, Factory, Augment — plus the agent-client registry path. They're spread across A partially-applied opt-out — hooks off for Claude, still written for Codex — is worse than no flag at all, so this needs to be exhaustive. 4. Each skipped client should say so. The original only printed a skip notice on the Claude Code path. Silence about a skipped group is precisely what made #1387 hard to see; one line per client ("hooks: skipped (install --hooks to enable)") is enough. 5. Smoke phases 8d/8e assert hooks exist after a plain install. Since the default is unchanged they should stay green as-is — but a 6. A I started sketching this myself and stopped: it's a wider change than it looks, and it's the install path, where a half-applied version is genuinely harmful. You offered to help and you're the one who cared enough to profile the overhead — so it's yours if you want it, and I'll review promptly this time rather than leaving you on a queue. If you'd rather not carry it, say so and I'll pick it up; no hard feelings either way. Either way, please rebase before starting — the branch conflicts with |
|
On it |
|
Superseded by #1862. I rebuilt the implementation from current \main\ around the maintainer-approved default-on, persistent opt-out design and expanded coverage across the current hook surfaces. |
Maintainer carry of #1862 (persistent install-hook opt-out, direction approved in #753). Main's current behaviour is kept everywhere; only the PR's intent -- persisting `install --no-hooks` / `--hooks` -- is re-applied on top of it. Textual conflicts: - docs/CONFIGURATION.md: both sides appended rows to the "Current keys" table. Kept main's `index_max_files` and `index_max_source_mb` rows unchanged, then added the PR's `install_hooks` row after them. The PR's explanatory paragraph follows the table, so the table stays one contiguous block. - src/main.c (print_help): main added a second `install` usage line for `[--clients=<tokens>]`; the PR appended `[--hooks|--no-hooks]` to the first line. Kept both: main's first line plus the PR's hook flags, followed by main's `--clients` line unchanged. Semantic conflicts (merged cleanly as text, wrong against main): - src/cli/cli.c, install_claude_code_config() --no-hooks branch: main now registers Claude hooks shell-free (#1733), as `<binary> hook-augment`, and changed cbm_remove_session_hooks() to take (settings_path, binary_path). The PR's disable path still called cbm_remove_session_hooks(settings_path), which no longer compiles, and used the NULL-binary cbm_remove_claude_hooks() / cbm_remove_claude_subagent_hooks() wrappers, which do not match the exec-form entries main installs. The disable path now calls the same binary-aware removers as main's uninstall_claude_code(): cbm_remove_claude_hooks_with_binary(), cbm_remove_session_hooks(..., binary_path) and cbm_remove_claude_subagent_hooks_with_binary(), all with the binary_path the install side registers. Legacy shell-form entries are still covered through those functions' old_commands lists. - tests/test_cli.c, cli_install_hooks_preference_persists_and_migrates_owned_state: the settings.json checks looked for "cbm-code-discovery-gate", but main no longer writes that name into settings.json (main's own cli_claude_hook_commands_use_exec_form_with_custom_config_dir asserts it is absent). The presence checks would be RED and the absence checks would pass without testing anything. They now use "hook-augment", the owned marker main's tests use. The script-file checks are unchanged, because main still writes the shim scripts. - scripts/smoke-test.sh 8e-i: same stale marker. The --no-hooks check now also rejects "hook-augment" in settings.json, and the --hooks check requires it. Both checks use "$GATE_SCRIPT" (set per platform in 8e) in place of the extensionless shim path, which on Windows (.cmd) could never exist and so checked nothing there. Everything else (cli.c hook gating, cli.h key, README, CONFIG_KEYS table, test registrations) auto-merged against main without change. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Maintainer refresh of #1862 (persistent install-hook opt-out, direction decided in #753) onto main a5b248f. The first refresh is 2577a86 and carries the semantic fixes for shell-free Claude hooks. Textual conflicts, all resolved by keeping both sides: - src/cli/cli.h: main added CBM_CONFIG_DOC_CANDIDATES on the line where the PR added CBM_CONFIG_INSTALL_HOOKS. Both defines are kept, in alphabetical order. - src/cli/cli.c CONFIG_KEYS[]: main added the watch_non_git row where the PR added install_hooks. Main row stays first, followed by the install_hooks row. - src/cli/cli.c Codex hook install: main (#2719) declared pretool_written in front of the hooks.json branch the PR prefixed with its --no-hooks removal branch. The declaration stays first, then the PR removal branch, then main hooks.json and config.toml upsert branches unchanged. Check: for every file the PR touches, the added and removed lines of the merged tree against main equal those of the PR against its old base. No compile-level semantic conflict: no function the PR calls changed its signature, and the PR re-signs no function main calls. The new Codex PreToolUse hook from #2719 is gated by the opt-out in the next commit. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Summary
--hookstoinstallto opt in--hooksis setMotivation
Split from #625 — avoids spawning hook augmenters on every Grep/Glob unless the user explicitly opts in.
Test plan
cli_install_plan_hooks_opt_in_defaultintest_cli.cmake -f Makefile.cbm test(CI)Signed-off-by: Andy11-cpu canada11@duck.com