merge the stop hook into an existing settings.json

[jesseworld22]

description

Merge handled permissions and PreToolUse but never called mergeStopHooks. Just MergeCodex did.

So chunk init on a repo that already had a .claude/settings.json merged its commit hooks and dropped the Stop hook on the floor. The repo ended up looking fully configured while nothing ran chunk validate at session end. Every Stop assertion in the suite was on the MergeCodex path, which is how this survived.

changes since last review

The fix started as one line in Merge calling the existing mergeStopHooks. Self-review found that landing it that way traded one silent-data-loss bug for another, so mergeHooks now owns hook entries rather than hook groups. That is a bigger diff than "one line," and it changes two behaviors beyond the original bug:

• PreToolUse no longer deletes a user's own Bash hooks. CommitMatcher is the bare tool name Bash, not a command glob, so any team with a non-commit Bash PreToolUse hook shares that group with chunk. Replacing the group wholesale — which is what the old code did, and what the one-line fix would have kept doing for PreToolUse — dropped their entry. Chunk's entries are identified by CommitIfFilter instead, and a group still on the legacy matcher is treated as chunk's in full since its entries predate the if tag.
• Duplicate chunk validate entries now collapse. The old mergeStopHooks replaced the whole group, which incidentally deduped it. Replacing a single entry in place does not, so stale duplicates would have survived every subsequent merge and run validation twice per session. Every owned entry is stripped before the generated one is reinserted, so re-running init converges.

Ownership is now symmetric across both hook types and both the Claude and Codex paths share it, which is what let MergeCodex diverge from Merge in the first place.

tests

internal/settings/merge_test.go covers the merge itself: adding Stop to an existing file, replacing chunk's entry in place, leaving a user's own Stop group alone, preserving a user entry that shares chunk's group, the same for a user entry in the Bash PreToolUse group, duplicate collapse across groups and matchers, and idempotency on each shared-group path.

On the acceptance test asked for in review — one is added (TestInitWritesFiringStopHook, plus a real Stop-hook assertion on the settings.example.json fallback), but it is worth being explicit that it does not catch this bug, and I verified that by reintroducing the defect: the acceptance test still passes and only internal/cmd TestWriteSettingsExistingMergeApplied fails.

The reason is structural. writeSettings only calls Merge when a .claude/settings.json already exists, and that path is gated behind tui.Confirm, which returns ErrNoTTY under a test harness and falls back to writing settings.example.json from the generated bytes. So a fresh-repo init never reaches Merge, and the one acceptance-reachable existing-settings path never reaches it either. Covering the merge from an acceptance test needs either a PTY dependency (none in the tree today — kr/pty is only an indirect go.sum hash) or a non-interactive apply flag. Happy to add either if that is the bar; flagging it rather than implying the acceptance test guards the fix.

The actual regression guard is at the cmd layer, where the write happens. That test previously asserted only merged["hooks"] != nil, which is exactly why the bug shipped: hooks was non-nil from the merged commit hooks while Stop was absent. It now asserts per hook type.

scope change since first review

This PR originally also added chunk hook repair, Stop-hook reporting in chunk validate and chunk hook status, and a validation.requireStopHook opt-out. Per review, that general-purpose repair tooling is out of scope - the ticket was to find the affected repos and fix them directly. All of it has been dropped; what's left is the root-cause bug fix.

affected repos (fixed directly)

What I did: Swept all 594 non-archived, non-fork repos in circleci and CircleCI-Public via the GitHub GraphQL API for .chunk/config.json, then checked each for a firing Stop hook. 9 repos have a chunk config; all 9 are sidecar-configured; 5 had no firing Stop hook. PRs are open on each:

Repo	Gap	PR
CircleCI-Public/circleci-cli	had settings.json, no Stop block — this bug	CircleCI-Public/circleci-cli#1739
circleci/opentelemetry-collector-deploy	no .claude/settings.json	circleci/opentelemetry-collector-deploy#773
circleci/public-api-service	no .claude/settings.json	circleci/public-api-service#1236
circleci/factory-bot	no .claude/settings.json	circleci/factory-bot#170
circleci/sandbox-provisioner	no .claude/settings.json	circleci/sandbox-provisioner#368

Already healthy: soc-integrations, notifications, llmops-service, chunk-cli.

Note that only circleci-cli is explained by this merge bug — the other four never had a .claude/settings.json written at all, which is a separate gap.

[michael-webster]

Can you add an acceptance test for this that confirms the behavior.

[michael-webster]

i don't think you need a new function to merge the stop hooks, merge hooks just wasn't handling the case probably. i'd extend merge hooks to also include the stop hook merging as well.

[jesseworld22]

from claude:

done in adbf765 - mergeStopHooks is gone and mergeHooks handles both hook types, so Merge and MergeCodex make one call instead of two.

kept the difference in how they merge, since it is not incidental: chunk owns the whole PreToolUse group (matched by matcher) but only a single Stop entry (matched by command), because a user may have their own entries in that same group and replacing the group would delete them. That is now documented on the one function instead of split across two, and the duplicated "create hooks if absent" block collapses into a hooksMap helper.

no behavior change — the existing tests pin both halves independently: six exercise generated settings carrying only PreToolUse, six only Stop, three both.

[jesseworld22]

claude summary on acceptance test:

added TestInitWritesFiringStopHook in acceptance/init_test.go, + a real Stop-hook assertion on the settings.example.json fallback (that test previously only checked the file existed).

being straight about this though: the acceptance test does not catch this bug. checked by reintroducing the defect - it still passes, only internal/cmd TestWriteSettingsExistingMergeApplied fails.

it's structural. writeSettings only calls Merge when a .claude/settings.json already exists, and that path is gated behind tui.Confirm → ErrNoTTY → falls back to writing settings.example.json. so fresh-repo init never reaches Merge, and the one acceptance-reachable existing-settings path doesn't either. getting there needs a PTY dep (none in the tree) or a --yes flag. both easy, both new surface area on a PR you already asked me to shrink — tell me which and I'll add it.

the real guard is the cmd-layer test. it previously asserted only merged["hooks"] != nil, which is why this shipped: hooks was non-nil from the merged commit hooks while Stop was missing. now asserts per hook type.

[jesseworld22]

added TestInitWritesFiringStopHook in acceptance/init_test.go, + a real Stop-hook assertion on the settings.example.json fallback (that test previously only checked the file existed).

the acceptance test does not catch this bug.

checked by reintroducing the defect - acceptance test still passes, and only internal/cmd TestWriteSettingsExistingMergeApplied fails.