From af3e115bfa9a45419620ea5074208fa550ca5e68 Mon Sep 17 00:00:00 2001 From: bob Date: Sun, 24 May 2026 11:03:04 -0400 Subject: [PATCH] fix(retrofit): dedupe hook groups so repeated --retrofit stops inflating settings.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `deep_merge_settings` historically concatenated hook groups without dedup (configure.py:1149-1151 docstring). The rationale ("preserve user customizations that happen to share a matcher") held when the existing hooks were genuinely user-authored, but broke on retrofit — the existing hooks are CONFIGURATOR-shipped from a prior scaffold. After N retrofits, every configurator hook fires N+1 times. Dogfood empirically confirmed: a fresh solo-experienced scaffold has 3 PreToolUse + 3 PostToolUse + 1 Stop + 4 SessionStart hook groups; after 3 retrofits those inflate to 12, 12, 4, 16 respectively. Fix: hook-list merge now goes through the existing _merge_unique_list helper (which already handles permissions.allow/ask/deny correctly) — structural equality via Python's `==` on dicts. Two-step merge: 1. _merge_unique_list(existing, new) — appends only what's new 2. _merge_unique_list([], merged) — collapses any prior-retrofit duplicates already in the existing list (self-heal) Self-healing: a user whose settings.json already accumulated N duplicates from prior versions sees them collapse to 1 on their next retrofit. User customizations survive: a hook group with a different matcher, different command, or different timeout is structurally distinct from configurator-shipped ones and is preserved. New `test/retrofit-hooks/` directory with 3 fixtures: - no-op retrofit doesn't inflate (3 retrofits, counts stable) - prior-buildup collapses on next retrofit (12→3) - three flavors of user customization preserved across retrofit Known limitation (out of scope): when the configurator changes a shipped hook between releases (e.g., bumps a timeout from 5→10), the user's old version + new version both survive structural dedup. Rare in practice; same-release retrofits — the dominant case — are fully fixed. Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 1 + configure.py | 21 ++++-- .../test-no-duplication-on-retrofit.sh | 51 +++++++++++++ .../test-self-heal-prior-buildup.sh | 51 +++++++++++++ .../test-user-customizations-preserved.sh | 75 +++++++++++++++++++ 5 files changed, 194 insertions(+), 5 deletions(-) create mode 100755 test/retrofit-hooks/test-no-duplication-on-retrofit.sh create mode 100755 test/retrofit-hooks/test-self-heal-prior-buildup.sh create mode 100755 test/retrofit-hooks/test-user-customizations-preserved.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 758308e..b8782a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to this project. Format: [Keep a Changelog](https://keepacha ## Unreleased +- **fix(retrofit): dedupe hook groups so repeated `cc-configure --retrofit` stops inflating settings.json.** `deep_merge_settings` historically concatenated hook groups onto the existing list without dedup (configure.py:1149-1151 docstring). The rationale ("preserve user customizations that happen to share a matcher") held when the existing hooks were genuinely user-authored, but broke on retrofit — where the existing hooks are CONFIGURATOR-shipped from a prior scaffold. After N retrofits, every configurator hook fires N+1 times. Dogfood empirically confirmed: a fresh `solo-experienced` scaffold has 3 PreToolUse + 3 PostToolUse + 1 Stop + 4 SessionStart hook groups; after 3 retrofits those inflate to 12, 12, 4, 16 respectively. Fix: hook-list merge now goes through the existing `_merge_unique_list` helper (which already handles permissions.allow/ask/deny correctly) — structural equality via Python's `==` on dicts. Self-healing: a user whose settings.json already accumulated N duplicates from prior versions sees them collapse back to 1 on their next retrofit (the merged list gets re-deduped against itself). User customizations survive: a hook group with a different matcher, different command, or different timeout is structurally distinct from configurator-shipped ones and is preserved. New `test/retrofit-hooks/` directory with 3 fixtures: no-op retrofit doesn't inflate; prior-buildup collapses on next retrofit; three flavors of user customization all preserved across retrofit. Known limitation (out of scope): when the configurator changes a shipped hook between releases (e.g., bumps a timeout from 5→10), the user's old version + new version both survive structural dedup. Rare in practice; same-release retrofits — the dominant case — are fully fixed. Future fix would track configurator provenance per hook group (e.g., a marker field), but that conflicts with the schema-hygiene retired `//` pattern from PR #60. - **fix(schema-hygiene): retire //-stub pattern + drop unvalidatable skillOverrides default + add settings preflight.** Dogfood from upgrading an adjacent project surfaced two real validator-rejection bugs introduced by PRs #17, #18, #56, and #57: (1) `templates/token-efficiency/settings-patch.tier-pro.json` shipped `"skillOverrides": "name-only"` (string form), but the current Claude Code schema requires the per-skill object map (`{"skill-name": "name-only"}`) and rejects the string — `name-only` was a legacy/imagined "apply to all skills" form that the official doc never sanctioned; further, per code.claude.com/docs/en/settings the setting doesn't apply to plugin skills at all, narrowing its usefulness as a tier-wide default; (2) the `// foo` "commented opt-in stub" pattern (`// sandbox`, `// env`, `// prUrlTemplate`, `// worktree`, `// subagentStatusLine`, `// hideVimModeIndicator`) propagated as literal top-level keys into the user's `.claude/settings.json`, where Claude Code's editor schema validator flags them as unknown properties. The design intent (PR #56's `_is_doc_label` filter) was to keep `// foo` stubs and strip only bare `//` / `//N` doc labels; the dogfood proves the intent was wrong. **Fixes:** `_is_doc_label` now strips ALL `//`-prefixed keys; new `_strip_doc_labels` recursively scrubs the merged settings before render (catches nested cases like `statusLine.// hideVimModeIndicator` that the shallow per-merge filter missed); `extraSettings` merge path now applies the filter (was previously bypassed — UI module's bare `"//"` was leaking too); `skillOverrides: "name-only"` deleted from tier-pro (replaced with a docstring explaining why no default ships); all `// foo` stubs deleted from source patch files (`safety/settings-patch.json`, `multi-agent/settings-patch.json`, `git-workflow/settings-patch.json`) and from the ui module's inline `extraSettings`; opt-in discovery moved to `templates/core/dot-claude/settings.local.json.example` (the `.example` suffix means Claude Code doesn't parse it directly, so `// foo` stubs there are safe). New `check_settings_validates()` preflight catches both bug classes at scaffold time + emits a `[ SETTINGS WARNINGS ]` block; new static `--check` step asserts no settings-patch ships `skillOverrides` as a non-object (regression guard, caught the current bug when run for the first time). New test directory `test/schema-hygiene/` with 4 fixtures: cross-persona no-leak assertion, source-patch shape assertion, preflight-detects-violations (4 violation classes + clean case), recursive `_strip_doc_labels` invariant. Compat note: users on cc-configure 2.6.0 + a populated `.claude/settings.json` get the cleanup on next `cc-configure --retrofit` — the deep-merge preserves their customizations and the new strip removes the stub-leaks. - **feat: discipline-skills module — curated 7-skill subset forked from obra/superpowers v5.1.0.** New `templates/discipline-skills/` module ships seven discipline skills as project-level `.claude/skills//SKILL.md`: `brainstorming`, `writing-plans`, `executing-plans`, `verification-before-completion`, `using-git-worktrees`, `subagent-driven-development`, `finishing-a-development-branch`. Forked verbatim from the MIT-licensed upstream plugin with three surgical edits: visual-companion section stripped from `brainstorming` (the upstream's browser-based companion server is omitted — text-only mode is the default fallback anyway); `superpowers:` prefix stripped from every inter-skill cross-reference so they resolve correctly as project-level skills; the `requesting-code-review` template embedded inline into `subagent-driven-development/code-quality-reviewer-prompt.md` and the cosmetic `test-driven-development` companion line dropped from `subagent-driven-development/SKILL.md`'s Integration section. Module includes a slim SessionStart bootstrap (`hooks/sessionstart-discipline.sh`) that primes the model with a ~400-token seven-skill overview (vs. upstream's ~1,200-token `using-superpowers` injection) and auto-suppresses when the upstream `superpowers` plugin is also installed (detects `~/.claude/plugins/cache/claude-plugins-official/superpowers/`). Wired into `config_schema.py` with `extraSettingsHook` for the SessionStart registration and into the `solo-newer`, `solo-experienced`, `small-team` personas by default. Upstream MIT LICENSE ships at `.claude/skills/_LICENSE-discipline-skills.md`; attribution to Jesse Vincent (© 2025) added under README `## Acknowledgments`. New `/verify-setup` Check #12 flags duplicate installation (both the configurator's module and the upstream plugin present) so users can pick one. New `docs/10-plugin-ecosystem.md` section "Discipline skills: bundled vs. upstream plugin" documents the trade-off (~930 tokens saved per session vs. losing access to the 7 unused upstream skills) and the maintainer-internal `templates/discipline-skills/SYNC.md` documents the upstream-sync workflow. Recommend-plugins copy reframes the upstream `superpowers` row as the "full-suite alternative" rather than the configurator's first-choice recommendation. diff --git a/configure.py b/configure.py index 8a19b82..01d8b5e 100755 --- a/configure.py +++ b/configure.py @@ -1146,9 +1146,16 @@ def deep_merge_settings(existing: dict, new: dict): existing entries first, new entries appended without duplicates. - permissions.disableBypassPermissionsMode: ours wins (security default the user opted into by selecting safety). - - hooks: concatenate per-event groups (existing first, then ours). No - dedupe — if the user has a hook group with the same matcher, both - run; user can manually remove duplicates if undesired. + - hooks: per-event groups merged via _merge_unique_list (existing + first, then ours, structural-equality dedup). Critical for + retrofits: without dedup, every cc-configure --retrofit run + re-appends the configurator's own hook set against the prior + scaffold's identical set, so after N retrofits every hook fires + N+1 times. User-customized hook groups (different matcher, + different command list, different timeout) are structurally + distinct from configurator-shipped ones and survive dedup. + Self-heals existing buildup: a user whose settings.json already + accumulated N duplicates collapses them to 1 on next retrofit. - env: dict merge with existing keys winning on collision (preserves user's deliberate overrides). - statusLine, model: preserve existing if set; otherwise use new. @@ -1177,8 +1184,12 @@ def deep_merge_settings(existing: dict, new: dict): out_hooks = dict(out.get("hooks", {})) for event, new_groups in new["hooks"].items(): existing_groups = out_hooks.get(event, []) - out_hooks[event] = list(existing_groups) + list(new_groups) - counts["hook_groups_added"] += len(new_groups) + merged = _merge_unique_list(existing_groups, new_groups) + # Also collapse any prior-retrofit duplicates already in + # existing_groups (self-heal). Reapply unique-list against itself. + merged = _merge_unique_list([], merged) + counts["hook_groups_added"] += len(merged) - len(existing_groups) + out_hooks[event] = merged out["hooks"] = out_hooks if "env" in new: diff --git a/test/retrofit-hooks/test-no-duplication-on-retrofit.sh b/test/retrofit-hooks/test-no-duplication-on-retrofit.sh new file mode 100755 index 0000000..1eebebe --- /dev/null +++ b/test/retrofit-hooks/test-no-duplication-on-retrofit.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# Repeated cc-configure --retrofit on the same project must NOT accumulate +# duplicate hook entries. Demonstrates + guards against the dogfood-reported +# bug (configure.py:1149-1151 historically concatenated without dedup, so +# every retrofit re-appended the configurator's own hook set; after N +# retrofits each hook fired N+1 times). +set -euo pipefail + +tmp=$(mktemp -d) +trap "rm -rf $tmp" EXIT + +# Initial scaffold (writes .claude/settings.json with the configurator's hooks) +python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null + +# Count hook entries in each event after the initial scaffold +count_hooks() { + local event="$1" + python3 -c " +import json +data = json.load(open('$tmp/.claude/settings.json')) +print(len(data.get('hooks', {}).get('$event', []))) +" +} + +initial_pretool=$(count_hooks PreToolUse) +initial_posttool=$(count_hooks PostToolUse) +initial_stop=$(count_hooks Stop) +initial_sessstart=$(count_hooks SessionStart) + +# Three more retrofits — same persona, same target — should be no-ops for hooks. +for i in 1 2 3; do + python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null +done + +after_pretool=$(count_hooks PreToolUse) +after_posttool=$(count_hooks PostToolUse) +after_stop=$(count_hooks Stop) +after_sessstart=$(count_hooks SessionStart) + +fail=0 +[ "$after_pretool" = "$initial_pretool" ] \ + || { echo "FAIL: PreToolUse: $initial_pretool → $after_pretool after 3 retrofits"; fail=1; } +[ "$after_posttool" = "$initial_posttool" ] \ + || { echo "FAIL: PostToolUse: $initial_posttool → $after_posttool after 3 retrofits"; fail=1; } +[ "$after_stop" = "$initial_stop" ] \ + || { echo "FAIL: Stop: $initial_stop → $after_stop after 3 retrofits"; fail=1; } +[ "$after_sessstart" = "$initial_sessstart" ] \ + || { echo "FAIL: SessionStart: $initial_sessstart → $after_sessstart after 3 retrofits"; fail=1; } + +[ "$fail" -eq 0 ] || exit 1 +echo "PASS: 3 retrofits don't duplicate hook entries (PreToolUse=$initial_pretool, PostToolUse=$initial_posttool, Stop=$initial_stop, SessionStart=$initial_sessstart, stable across retrofits)" diff --git a/test/retrofit-hooks/test-self-heal-prior-buildup.sh b/test/retrofit-hooks/test-self-heal-prior-buildup.sh new file mode 100755 index 0000000..b87eb47 --- /dev/null +++ b/test/retrofit-hooks/test-self-heal-prior-buildup.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# A user who upgraded across several pre-fix releases may have a settings.json +# with N duplicate hook entries already. On their NEXT retrofit, the new +# dedup logic must collapse the prior buildup back to 1 entry per group. +set -euo pipefail + +tmp=$(mktemp -d) +trap "rm -rf $tmp" EXIT + +# Initial scaffold +python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null + +# Simulate prior-version buildup: manually duplicate every hook group 3 times. +python3 -c " +import json +p = '$tmp/.claude/settings.json' +data = json.load(open(p)) +for event, groups in data.get('hooks', {}).items(): + data['hooks'][event] = groups * 4 # 1 + 3 dupes = 4 total +json.dump(data, open(p, 'w'), indent=2) +" + +# Sanity check: file now has inflated counts +inflated_pretool=$(python3 -c "import json; print(len(json.load(open('$tmp/.claude/settings.json'))['hooks']['PreToolUse']))") +[ "$inflated_pretool" -gt 3 ] || { echo "FAIL: test setup didn't actually inflate"; exit 1; } + +# One retrofit — should collapse the inflated counts back to the baseline. +python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null + +healed_pretool=$(python3 -c "import json; print(len(json.load(open('$tmp/.claude/settings.json'))['hooks']['PreToolUse']))") +healed_posttool=$(python3 -c "import json; print(len(json.load(open('$tmp/.claude/settings.json'))['hooks']['PostToolUse']))") +healed_stop=$(python3 -c "import json; print(len(json.load(open('$tmp/.claude/settings.json'))['hooks']['Stop']))") +healed_sessstart=$(python3 -c "import json; print(len(json.load(open('$tmp/.claude/settings.json'))['hooks']['SessionStart']))") + +fail=0 +# Expected baseline counts come from a fresh solo-experienced scaffold. +expected_pretool=3 +expected_posttool=3 +expected_stop=1 +expected_sessstart=4 +[ "$healed_pretool" = "$expected_pretool" ] \ + || { echo "FAIL: PreToolUse: inflated to $inflated_pretool, healed to $healed_pretool (expected $expected_pretool)"; fail=1; } +[ "$healed_posttool" = "$expected_posttool" ] \ + || { echo "FAIL: PostToolUse: healed to $healed_posttool (expected $expected_posttool)"; fail=1; } +[ "$healed_stop" = "$expected_stop" ] \ + || { echo "FAIL: Stop: healed to $healed_stop (expected $expected_stop)"; fail=1; } +[ "$healed_sessstart" = "$expected_sessstart" ] \ + || { echo "FAIL: SessionStart: healed to $healed_sessstart (expected $expected_sessstart)"; fail=1; } + +[ "$fail" -eq 0 ] || exit 1 +echo "PASS: inflated $inflated_pretool entries collapsed to baseline $expected_pretool on next retrofit" diff --git a/test/retrofit-hooks/test-user-customizations-preserved.sh b/test/retrofit-hooks/test-user-customizations-preserved.sh new file mode 100755 index 0000000..5b14173 --- /dev/null +++ b/test/retrofit-hooks/test-user-customizations-preserved.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# Dedup must not destroy genuine user customizations. A user-added hook +# group with a different matcher (or different command, or different +# timeout) is structurally distinct from configurator-shipped ones and +# must survive a retrofit unchanged. +set -euo pipefail + +tmp=$(mktemp -d) +trap "rm -rf $tmp" EXIT + +python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null + +# Inject three user customizations: +# 1. A wholly new hook group (different matcher than anything we ship) +# 2. A hook with same matcher as a shipped one but a different command +# 3. A timeout-tweaked duplicate of a shipped hook (different value) +python3 -c " +import json +p = '$tmp/.claude/settings.json' +data = json.load(open(p)) +data['hooks'].setdefault('PreToolUse', []).extend([ + { + 'matcher': 'WebSearch', + 'hooks': [{'type': 'command', 'command': '/usr/local/bin/log-websearch.sh', 'timeout': 5}] + }, + { + 'matcher': 'Bash', + 'hooks': [{'type': 'command', 'command': '/home/user/scripts/my-custom-bash-guard.sh', 'timeout': 10}] + }, +]) +# Tweak the timeout of an existing shipped hook to simulate a customization +for group in data['hooks']['PreToolUse']: + if group.get('matcher') == 'Bash' and 'block-dangerous-bash' in str(group): + new_group = json.loads(json.dumps(group)) + new_group['hooks'][0]['timeout'] = 999 # user bumped the timeout + data['hooks']['PreToolUse'].append(new_group) + break +json.dump(data, open(p, 'w'), indent=2) +" + +# Capture the user-added entries +before_user_websearch=$(python3 -c " +import json +data = json.load(open('$tmp/.claude/settings.json')) +print(any(g.get('matcher') == 'WebSearch' for g in data['hooks']['PreToolUse'])) +") +[ "$before_user_websearch" = "True" ] || { echo "FAIL: test setup didn't add WebSearch hook"; exit 1; } + +# Run retrofit +python3 configure.py --persona solo-experienced --yes --dir "$tmp" >/dev/null + +# All three customizations must survive +after_user_websearch=$(python3 -c " +import json +data = json.load(open('$tmp/.claude/settings.json')) +print(any(g.get('matcher') == 'WebSearch' for g in data['hooks']['PreToolUse'])) +") +after_user_custom_bash=$(python3 -c " +import json +data = json.load(open('$tmp/.claude/settings.json')) +print(any('my-custom-bash-guard' in str(g) for g in data['hooks']['PreToolUse'])) +") +after_user_timeout=$(python3 -c " +import json +data = json.load(open('$tmp/.claude/settings.json')) +print(any('block-dangerous-bash' in str(g) and any(h.get('timeout') == 999 for h in g.get('hooks', [])) for g in data['hooks']['PreToolUse'])) +") + +fail=0 +[ "$after_user_websearch" = "True" ] || { echo "FAIL: WebSearch hook lost after retrofit"; fail=1; } +[ "$after_user_custom_bash" = "True" ] || { echo "FAIL: custom-bash-guard hook lost after retrofit"; fail=1; } +[ "$after_user_timeout" = "True" ] || { echo "FAIL: timeout=999 customization lost after retrofit"; fail=1; } + +[ "$fail" -eq 0 ] || exit 1 +echo "PASS: 3 user customizations (new matcher, different command, tweaked timeout) all preserved across retrofit"