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

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
DeusData added a commit that referenced this pull request Nov 4, 2026
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>
DeusData added a commit that referenced this pull request Nov 4, 2026
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>
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