Repository navigation
feat(cli): persist install hook opt-out - #1862
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
added
enhancement
|
Thank you for implementing the previously discussed persistent hook opt-out and for preserving foreign hook state, dry-run behavior, and the remaining installer surfaces. The contribution queue is currently quite full, so the full review may take a little time. We have the PR routed and will come back with grounded feedback as soon as possible. |
|
Reviewed. The direction is settled — this implements the approved outcome from #753 — so this is about one build break, and it is a small one. The Windows failure is real, and it is a missing platform guardBoth The compiler's suggestion is a red herring. The existing call site already knows this. At #ifndef _WIN32
if (cbm_remove_gemini_coverage_hook(cp, installed_binary) != CLI_OK) {
...
}
#endifYour new call is not wrapped. Adding the same Why only Windows caught it: clang treats an implicit function declaration as an error, and the CLANG64 legs are the ones building with clang. On POSIX the function is present, so every other leg compiles cleanly and says nothing. Worth knowing, because a green Linux and macOS run tells you nothing about this class of mistake. One knock-on to check while you are there: The restThe shape is right: hooks stay enabled by default, Clearance is Also please rebase once the guard is in: |
Signed-off-by: Andy11-cpu <canada11@duck.com>
Signed-off-by: Andy11-cpu <canada11@duck.com>
Signed-off-by: Andy11-cpu <canada11@duck.com>
fc203c6 to
0e8fc16
Compare
Maintainer carry of DeusData#1862 (persistent install-hook opt-out, direction approved in DeusData#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 (DeusData#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>
|
Thanks again for the persistent hook opt-out. The recorded review asks remain the Windows guard around the Gemini helper call and a branch refresh, preserving the non-mutating plan/dry-run behavior. The smoke clearance mentioned above is a maintainer step, not something you need to change in the patch. Sorry for the delay on our smoke clearance; I’ll follow up on that soon. |
Maintainer refresh of DeusData#1862 (persistent install-hook opt-out, direction decided in DeusData#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 (DeusData#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 DeusData#2719 is gated by the opt-out in the next commit. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Since DeusData#1180 a bare ~/.claude directory no longer counts as a Claude Code install. The fixture of cli_no_hooks_plan_covers_every_hook_installer_family now also writes .claude/settings.json. Without it claude-code drops out of agents_detected and the test fails on the tree merged with current main. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…-out DeusData#2719 added a PreToolUse Bash hook for Codex after this opt-out was written. In config.toml it lives inside the managed block, which the opt-out already removes. In hooks.json it is a separate entry beside the lifecycle pair, and the opt-out removed only the pair, so after install --no-hooks, and on every later bare install or update that respects the stored preference, our PreToolUse hook stayed registered. The --no-hooks branch now also calls cbm_remove_codex_pretool_hooks_json, the exact-command remover that uninstall uses since DeusData#2719. A PreToolUse hook the user owns is left alone. Test: cli_no_hooks_removes_codex_pretool_hook drives the real install dispatch through both hook representations. A default install writes ours beside a foreign hook; --no-hooks and a bare install with the stored preference remove ours and keep the foreign hook and the MCP entry; and --hooks restores ours. Without the fix it fails: ours is still in hooks.json after --no-hooks. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @Andy11-cpu, for carrying this all the way from #753. You did the hard part well. The opt-out is exhaustive: every hook-capable installer family is gated, not just Claude Code. The choice persists in the existing config store, so bare installs and
Because the branch now has new commits from us, please pull before you push again. Thanks again for building an opt-out that keeps the graph-first default for everyone else. It is a real improvement for the people who need it. |
…-out install --no-hooks and the persisted install_hooks=false preference removed the Augment SessionStart and PostToolUse view registrations from ~/.augment/settings.json but left the two scripts they pointed at, ~/.augment/hooks/codebase-memory-session.sh and codebase-memory-coverage.sh (.ps1 on Windows), on disk. The opt-out now deletes them the way uninstall does. Both paths share remove_augment_owned_hook_scripts(): each script is rebuilt from the binary path the install embedded and removed with cbm_text_remove_owned_document() only on a byte-for-byte match, so a script the user edited, or a foreign file with the same name, is kept. Uninstall keeps its error operation names; the opt-out records session_script_disable and coverage_script_disable. install --hooks writes both scripts again. Test: cli_no_hooks_removes_augment_owned_scripts drives the install command with fake activation ops in a temporary HOME: default install, --no-hooks (both scripts gone, MCP entry kept), a bare install with the stored preference (still gone), --hooks (restored), and a user-edited session script that --no-hooks keeps while it deletes the untouched coverage script. The test ends on --hooks because the opt-out is process state that later tests rely on. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks again, @Andy11-cpu! One more small commit is going on top of your branch before the merge, so here is what it does and why. With --no-hooks (and with the stored install_hooks=false preference) the installer already removed the Augment SessionStart and PostToolUse view entries from ~/.augment/settings.json, but the two scripts those entries pointed at, ~/.augment/hooks/codebase-memory-session.sh and codebase-memory-coverage.sh (.ps1 on Windows), stayed on disk. The new commit makes the opt-out remove them the same way uninstall already does: both paths now share one helper that rebuilds each script from the installed binary path and deletes it only on an exact byte match, so a script a user edited is left alone. install --hooks writes both scripts again. A new test, cli_no_hooks_removes_augment_owned_scripts, covers the whole cycle: install, --no-hooks, a bare install with the stored preference, --hooks, and an edited script that is kept. Your opt-out design made this a few-line change. If you push again, please pull the branch first so this commit is not lost. Thank you for the careful work and the patience with the extra round! |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks for your patience, @Andy11-cpu: CI went green, but #1430 (the ignore_worktrees option) merged into main overnight and touched src/cli/cli.c, cli.h and the docs, so we merged current main into your branch once more (7e67581, a plain signed-off merge commit, no conflicts; your change and our carry commits are unchanged) to let CI cover the exact tree that will merge. It merges as soon as this run is green. Please pull before pushing again. |
Removing the only Hermes pre_llm_call hook empties the hooks mapping, and
the mapping editor writes an emptied section as hooks: {} - the same
convention it uses for mcp_servers: {}. The hook sequence editor then
refused that file for both upsert and removal, because it required every
parent on the sequence path to be a block mapping header. On a config
where our hook was the only entry under hooks, install -> uninstall ->
install exited 1, and a repeated uninstall did too.
An exact empty flow mapping on a parent of the sequence path is now read
as that empty mapping, mirroring how the mapping-entry editor already
treats section: {}. Removal is a byte-identical no-op, and upsert expands
the header in place, keeping its comment, the line endings and every
other byte, so a reinstall lands back on the first install byte for
byte. The sequence key itself holding a mapping, any other flow value,
and content nested under the inline mapping stay refused. Hooks written
by the user and every foreign key are left untouched.
Found while finishing DeusData#1862: its persisted hook opt-out runs this remover
on every later install, so every Hermes user whose only hook was ours
would have hit the refusal on each install after the first.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Bring the branch up to main e0c7a1d (DeusData#2395), which includes DeusData#1724 (index resource worker and publication fence, 008b6c3). Conflict resolved (1 file): - docs/CONFIGURATION.md: both sides appended rows to the "Current keys" table at the same spot. Resolved as main rows index_max_rss_mb and index_max_duration_seconds first, then the branch row install_hooks and its paragraph that closes the table. No conflict side was concatenated as-is; the branch paragraph must follow the last table row. Auto-merged without conflict: README.md, src/cli/cli.c (CONFIG_KEYS now holds the new index_max_rss_mb / index_max_duration_seconds keys and install_hooks), src/main.c (worker publish-fence argv next to the --hooks/--no-hooks help line). Check: for all seven branch files, the zero-context patch-id of the merged tree against main equals the branch own change. Semantic check: the branch re-signs no function; main re-signed the daemon worker start callback and the supervisor result struct, which the branch does not use. No call site adapted. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again @Andy11-cpu for this change, and for your patience while it waits its turn in the queue. Main moved again today: #1724 (resource limits for the index worker) added two new config keys at exactly the spot where this PR adds install_hooks, so the branch no longer merged cleanly. We pushed one more merge commit that brings in current main. The only conflict was the "Current keys" table in docs/CONFIGURATION.md: it now lists the two new index_max_* keys first, then your install_hooks row and your note below the table. Everything else in your change is untouched. If you push to this branch again, please pull it first so this merge commit is not lost. Thank you! |
|
Merged - thank you very much, @Andy11-cpu, and thank you for your patience while this waited its turn and main moved under it several times. Persisting the hook opt-out means that a user who declines the install hooks once keeps that choice across later installs and updates, instead of having the hooks come back. The install_hooks key is documented in docs/CONFIGURATION.md next to the other config keys, and your tests pin the behaviour in the CLI suite. While reviewing this we also found a small, separate issue in the Hermes hook remover; that fix landed on its own in #2896. Thanks again for the careful work! |
Fixes #1857
Supersedes #753.
What
install --no-hooksandinstall --hooks.Why
#753 established the performance cost of per-call hook process spawning. This implements the maintainer-approved direction from that discussion: retain graph-first defaults while giving users a persistent opt-out.
Verification
scripts/test.shscripts/lint.sh --ci CLANG_FORMAT=clang-format-20scripts/ci/lint-mem.sh clang-tidy-22make -f Makefile.cbm securitygit diff --checkAll pass locally.