Skip to content

fix(retrofit): union hook groups by command within a matcher (dogfood F1+F3) - #73

Merged
tigers1997 merged 1 commit into
mainfrom
fix/hook-merge-command-union
May 30, 2026
Merged

fix(retrofit): union hook groups by command within a matcher (dogfood F1+F3)#73
tigers1997 merged 1 commit into
mainfrom
fix/hook-merge-command-union

Conversation

@tigers1997

Copy link
Copy Markdown
Owner

What & why

The 2026-05-30 dogfood (upgrading an adjacent Next.js/pnpm project from cc-configure 2.6.0 to post-2.6.0 HEAD) surfaced a HIGH-severity defect that PR #61 had explicitly scoped out.

deep_merge_settings deduped hook groups per event by whole-dict == (_merge_unique_list). When the configurator moves a hook between releases from a standalone matcher group to a bundled one — exactly what the post-2.6.0 safety change does, splitting a lone block-dangerous-bash Bash group into [block-dangerous-bash, check-package-availability] — the two group dicts aren't equal, so the new bundled group is appended. The user's .claude/settings.json ends with two Bash groups both containing block-dangerous-bash, which then fires twice on every Bash call.

PR #61 called this "rare … out of scope." The dogfood revises that: adding a hook to an existing matcher group is the normal way a module grows, so ~100% of safety-module users upgrading 2.6.0→latest inherit a double-firing dangerous-bash guard. Silent and security-relevant.

The fix

  • F1 — _merge_hook_groups replaces whole-group dedup on the hooks path: key by matcher, union inner hooks[] by command, append a configurator command only when it's absent from every existing group sharing that matcher. The standalone+bundled pair collapses to one group, each command wired once.
  • F3 — belt-and-suspenders. check_settings_validates gains a third check (_find_duplicate_hook_commands) flagging any command wired more than once under the same (event, matcher). Keyed by matcher, so the legitimate "same command under different matchers" case is not flagged. Emits a [ SETTINGS WARNINGS ] line. The merge prevents the corruption on every shipped upgrade path; F3 is the net for a residual already-corrupted file (e.g. a user who tracked unreleased HEAD — that bundled patch was never tagged, so no released upgrade produces it).

Scope boundary (honest)

A config already corrupted with two same-matcher groups is not auto-collapsed by the merge (that would collide with PR #61's deliberate same-command-different-timeout preservation). It is instead caught by F3 (verified). Real-world population of that state ≈ the dogfood author's own project, already hand-fixed.

Tests / CI

  • New TDD fixtures (both fail against the old _merge_unique_list):
    • test/retrofit-hooks/test-standalone-bundled-merge.sh — the exact dogfood shape.
    • test/schema-hygiene/test-duplicate-hook-commands.sh — same-matcher dup flagged, cross-matcher allowed, within-group caught, clean + no-hooks-key pass.
  • CI gap fixed: test/retrofit-hooks/ and test/schema-hygiene/ (created by PRs fix(schema-hygiene): retire //-stub pattern + drop unvalidatable skillOverrides #60/fix(retrofit): dedupe hook groups so repeated --retrofit stops inflating settings.json #61) were never wired into check.yml — they only ran locally. Both dirs are now workflow steps, so all 7 prior fixtures + the 2 new ones run in CI for the first time.
  • Full end-to-end retrofit reproduction confirms block-dangerous-bash=1, check-package-availability=1, dupes=NONE after a standalone→bundled upgrade.
  • configure.py --check clean. Adversarially reviewed (verdict: SHIP); non-list hooks[] append-path crash hardened with a coercion guard for parity with the read-side helpers.

Not in this PR

Dogfood findings F2, F4–F8 are separate follow-ups. Upstream survey (CC 2.1.155–2.1.158, SchemaStore #5723 still open) found nothing forcing a tested_up_to bump — 2.1.150 stays pinned.

🤖 Generated with Claude Code

… F1+F3)

deep_merge_settings deduped hook groups by whole-dict ==, so when the
configurator moves a hook from a standalone matcher group to a bundled one
(post-2.6.0 safety: lone block-dangerous-bash -> [block-dangerous-bash,
check-package-availability]), both groups survived a retrofit and the shared
command fired twice on every matching call. PR #61 had scoped this out as
"rare"; the dogfood shows it's the dominant upgrade for safety-module users.

- New _merge_hook_groups: key by matcher, union inner hooks[] by command;
  append a command only when absent from every same-matcher group. Preserves
  user customizations (PR #61 test still passes) + old N+1 self-heal.
- F3: check_settings_validates gains _find_duplicate_hook_commands, flagging a
  command wired >1x under the same (event, matcher); cross-matcher allowed.
- [ MERGED ] summary gains an "N hook command(s)" counter.
- CI: wire test/retrofit-hooks/ and test/schema-hygiene/ into check.yml —
  created by PRs #60/#61 but never run in CI until now.
- New TDD fixtures: test-standalone-bundled-merge.sh (fails on old code),
  test-duplicate-hook-commands.sh. Adversarially reviewed; non-list hooks[]
  append path hardened.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented May 30, 2026

Copy link
Copy Markdown

VERDICT: PASS

Clean, well-scoped fix. The _merge_hook_groups replacement for _merge_unique_list on the hooks path correctly addresses the F1 double-firing regression, the F3 _find_duplicate_hook_commands validator provides the right belt-and-suspenders net, the two new TDD fixtures directly cover the dogfood shape and the cross-matcher non-flagging invariant, and wiring the previously-local retrofit-hooks/ and schema-hygiene/ suites into check.yml closes a CI gap that predates this PR. CHANGELOG ## Unreleased entry present; no new third-party code; no changes to templates/discipline-skills/; no new settings.json schema keys.

@tigers1997
tigers1997 merged commit a4dae9a into main May 30, 2026
4 checks passed
@tigers1997
tigers1997 deleted the fix/hook-merge-command-union branch May 30, 2026 21:07
tigers1997 added a commit that referenced this pull request Jun 26, 2026
…p on retrofit (PR #88 review)

Existing installs carry a matcherless SessionStart marker-clear group;
_merge_hook_groups keys by matcher, so cc-configure --retrofit would append
the new startup|clear group beside the old one and the stale matcherless group
kept wiping markers on resume/compact — re-negating the freeze-loss fix on
every upgrade (the same class as PR #73's standalone->bundled bug, for a
matcher change).

_merge_hook_groups now does a pre-pass: for each configurator command it
collects the matcher(s) the new template places it under, then strips that
command from any existing group whose matcher is not one of them (a stale
placement). This preserves the deliberate same-command-under-multiple-matchers
pattern (the mcp drift-check ships under both startup and resume — the
no-duplication / self-heal tests still expect 4 SessionStart groups) and never
touches a user's own command.

New test/retrofit-hooks/test-sessionstart-matcher-migration.sh. Also aligns
test-sessionstart-matcher.sh to the heredoc / sys.path '.' style used by the
merge tests (review advisory). --check clean, 104/104 fixtures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tigers1997 added a commit that referenced this pull request Jun 26, 2026
…tartup|clear (dogfood F1) (#88)

* fix(commands): scope microbit-enforcer SessionStart marker-clear to startup|clear (dogfood F1)

The SessionStart hook that clears the freeze/guard/careful markers shipped
with no matcher, firing on every source — including resume and compact — so
a /freeze was silently lost mid-session after compaction or --resume. Now
"matcher": "startup|clear": markers clear only on a fresh session or /clear
and survive --resume and compaction.

Sources verified against code.claude.com/docs/en/hooks-guide (startup,
resume, clear, compact; matcher filters on source). The freeze/guard/careful
SKILL.md notes, the microbit-enforcer.sh header, and the
templates/core/CLAUDE.md gitignore note are corrected to match, across both
the templates and the python-uv-fastapi example mirror. New
test/microbit-enforcer/test-sessionstart-matcher.sh.

Split from the originally-bundled PR #87 per CONTRIBUTING.md (one logical
change per PR); F2 (stack-reality preflight) ships as a separate PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(retrofit): migrate the matcherless SessionStart marker-clear group on retrofit (PR #88 review)

Existing installs carry a matcherless SessionStart marker-clear group;
_merge_hook_groups keys by matcher, so cc-configure --retrofit would append
the new startup|clear group beside the old one and the stale matcherless group
kept wiping markers on resume/compact — re-negating the freeze-loss fix on
every upgrade (the same class as PR #73's standalone->bundled bug, for a
matcher change).

_merge_hook_groups now does a pre-pass: for each configurator command it
collects the matcher(s) the new template places it under, then strips that
command from any existing group whose matcher is not one of them (a stale
placement). This preserves the deliberate same-command-under-multiple-matchers
pattern (the mcp drift-check ships under both startup and resume — the
no-duplication / self-heal tests still expect 4 SessionStart groups) and never
touches a user's own command.

New test/retrofit-hooks/test-sessionstart-matcher-migration.sh. Also aligns
test-sessionstart-matcher.sh to the heredoc / sys.path '.' style used by the
merge tests (review advisory). --check clean, 104/104 fixtures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant