Skip to content

feat(cli): persist install hook opt-out - #1862

Open
Andy11-cpu wants to merge 3 commits into
DeusData:mainfrom
Andy11-cpu:persistent-install-hook-opt-out
Open

feat(cli): persist install hook opt-out#1862
Andy11-cpu wants to merge 3 commits into
DeusData:mainfrom
Andy11-cpu:persistent-install-hook-opt-out

Conversation

@Andy11-cpu

Copy link
Copy Markdown
Contributor

Fixes #1857

Supersedes #753.

What

  • Keeps install hooks enabled by default.
  • Adds persistent install --no-hooks and install --hooks.
  • Applies the policy across current hook-capable installer surfaces.
  • Preserves MCP, skills, instructions, profiles, and foreign hook state.
  • Keeps plan/dry-run non-mutating.

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.sh
  • scripts/lint.sh --ci CLANG_FORMAT=clang-format-20
  • scripts/ci/lint-mem.sh clang-tidy-22
  • make -f Makefile.cbm security
  • agent-config smoke
  • focused hook-policy tests
  • git diff --check

All pass locally.

@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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 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 Sep 1, 2026
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

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.

@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

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 guard

Both test-windows shards, pr-smoke (windows-latest) and test-windows-guards fail for the same reason: the tree does not compile on Windows.

src/cli/cli.c:8846: error: call to undeclared function 'cbm_remove_gemini_coverage_hook'
                    did you mean 'cbm_remove_augment_coverage_hook'?

The compiler's suggestion is a red herring. cbm_remove_gemini_coverage_hook does exist — at src/cli/cli.c:5769 — but its definition sits inside #ifndef _WIN32, which opens a few lines above it. On Windows it is compiled out entirely.

The existing call site already knows this. At cli.c:11104:

#ifndef _WIN32
        if (cbm_remove_gemini_coverage_hook(cp, installed_binary) != CLI_OK) {
            ...
        }
#endif

Your new call is not wrapped. Adding the same #ifndef _WIN32 / #endif pair around it should be the whole fix.

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: cbm_upsert_gemini_coverage_hook lives under the same guard at :5753. If the persistent-policy work added an upsert call anywhere, it needs the same treatment — the compiler only reports the first error.

The rest

The shape is right: hooks stay enabled by default, --no-hooks / --hooks persist the choice, plan and dry-run stay non-mutating, and foreign hook state along with MCP, skills, instructions and profiles is preserved. Keeping the graph-first default while giving a persistent opt-out is what #753 concluded, and doing it as a persisted policy rather than a per-invocation flag is what actually solves the reported problem.

Clearance is REVIEW(1) on scripts/smoke-test.sh — the established shape here. It needs a maintainer marker, not a change from you.

Also please rebase once the guard is in: main moved three times yesterday (broken by a duplicate-symbol merge, repaired by #1993, then #1703 landed).

Signed-off-by: Andy11-cpu <canada11@duck.com>
Signed-off-by: Andy11-cpu <canada11@duck.com>
Signed-off-by: Andy11-cpu <canada11@duck.com>
@Andy11-cpu
Andy11-cpu force-pushed the persistent-install-hook-opt-out branch from fc203c6 to 0e8fc16 Compare September 2, 2026 04:22
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.

feat(cli): persistent opt-out for install hooks

2 participants