Skip to content

Make install hooks opt-in via --hooks - #753

Closed
Andy11-cpu wants to merge 1 commit into
DeusData:mainfrom
Andy11-cpu:fix/install-hooks-opt-in
Closed

Make install hooks opt-in via --hooks#753
Andy11-cpu wants to merge 1 commit into
DeusData:mainfrom
Andy11-cpu:fix/install-hooks-opt-in

Conversation

@Andy11-cpu

Copy link
Copy Markdown
Contributor

Summary

  • PreToolUse / SessionStart augmenter hooks are not installed by default
  • Pass --hooks to install to opt in
  • Install plan receipts omit hook paths unless --hooks is set

Motivation

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_default in test_cli.c
  • make -f Makefile.cbm test (CI)

Signed-off-by: Andy11-cpu canada11@duck.com

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>
@Andy11-cpu
Andy11-cpu requested a review from DeusData as a code owner July 2, 2026 01:56
@DeusData DeusData added enhancement New feature or request editor/integration Editor compatibility and CLI integration ux/behavior Display bugs, docs, adoption UX priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Jul 3, 2026
@DeusData

DeusData commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Thanks for splitting out the hook opt-in behavior. Triage: installer/editor-integration UX change, currently conflicting.

Please rebase on current main. After that, review needs to confirm the product decision around default hook installation, that dry-run/install receipts are clear, and that uninstall remains idempotent for both old default-hook installs and new opt-in installs.

@DeusData DeusData added this to the 0.9.2-rc milestone Jul 8, 2026
@DeusData

Copy link
Copy Markdown
Owner

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. cli_install_plan_hooks_opt_in_default pins the default through the same record-only dispatch the real install uses, so the plan receipt cannot drift from actual behaviour. It is binding — it does not even compile against the old signature, and the behavioural assert fails against hooks-on. That is the right way to test a default.

One thing I would want changed regardless of which way the decision goes, because it is a genuine design gap rather than a preference:

cbm_cmd_update hardcodes install_hooks = false, and there is no --hooks flag on update. So a user who already has hooks — including everyone who installed before this change — would never have their hook scripts refreshed again. That matters more than it sounds: main has shipped several legacy-hook migration fixes that run during config refresh, and under this PR those migrations would never reach the people who need them. Stale hook scripts persisting across versions is precisely the failure class that work was fighting. If hooks become opt-in, update still needs to refresh hooks for users who already have them — presence-detection or a persisted choice would do it.

Three smaller things, all consequences of it being a user-facing default change:

  • The skip notice is printed only for Claude Code. Codex, Gemini and Antigravity silently stop receiving hooks — inconsistent for a change users need to notice.
  • No README or docs update, so the documented install flow would quietly stop delivering hooks.
  • Smoke phases 8d and 8e assert hooks exist after a plain install. The PR does not touch the smoke suite, so even a perfectly clean rebase goes red on all three pr-smoke legs. Making smoke --hooks-aware is part of the change rather than a mechanical fix-up.

On staleness, so you can plan: every signature you touch has moved. cbm_install_agent_configs is now public and returns int with a _with_previous wrapper, and cbm_build_install_plan_json now delegates to cbm_build_install_plan_json_options(home, path, skip_config) — whose third boolean means something entirely different from yours. A naive rebase would silently cross those wires, which is the kind of bug that survives review. Main has also gained a third hook (subagent-reminder) and more agent surfaces, so the opt-in would currently be partial.

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.

@Andy11-cpu

Copy link
Copy Markdown
Contributor Author

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%

@DeusData

Copy link
Copy Markdown
Owner

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. install --no-hooks skips them; install --hooks turns them back on. Bare install keeps today's behaviour.

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 — cbm_config_t, SQLite at ~/.cache/codebase-memory-mcp/_config.db, with a CONFIG_KEYS[] registry in src/cli/cli.c alongside auto_index / auto_watch / ui_enabled. Add something like install_hooks defaulting to "true". Then:

  • install --no-hooks writes false; install --hooks writes true;
  • bare install reads the stored value rather than assuming the default.

That last point is what makes it real: update in release builds just re-runs install.sh, which re-runs install. Without the stored preference, every update silently puts back the hooks you removed — which also resolves the cbm_cmd_update gap I flagged earlier, without needing a --hooks flag on update at all.

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 install_claude_code_config, install_gemini_config, install_cli_agent_configs, install_editor_agent_configs and install_additional_agent_configs, each interleaved with MCP/skills writes that must keep running.

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. g_client_selection in the same file is the established pattern for a cross-cutting install option: a file-scope flag set once at entry, read at each site, rather than threading a bool through five dispatchers.

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 --no-hooks sibling asserting the absence of ~/.claude/hooks/cbm-code-discovery-gate and of the PreToolUse entry would be the thing that stops this regressing later.

6. A README/docs line for the flag, and a config entry so config get install_hooks shows the stored state.

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 main now, and there's real signature drift in the install path since July.

@Andy11-cpu

Copy link
Copy Markdown
Contributor Author

On it

@Andy11-cpu

Copy link
Copy Markdown
Contributor Author

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.

@Andy11-cpu Andy11-cpu closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

editor/integration Editor compatibility and CLI integration enhancement New feature or request priority/normal Standard review queue; useful PR with ordinary maintainer urgency. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants