fix(schema-hygiene): retire //-stub pattern + drop unvalidatable skillOverrides - #60
Merged
Merged
Conversation
…lOverrides Dogfood from upgrading an adjacent project to the latest configurator version surfaced two real Claude Code settings-validator complaints: 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. The string form was based on a comment in the patch file that claimed a global "apply to all skills" form existed; verification against the official doc (code.claude.com/docs/en/settings) shows it never did. Also: the setting explicitly doesn't apply to plugin skills, further narrowing its usefulness as a tier-wide default. 2. The `// foo` "commented opt-in stub" pattern propagated literal top-level keys into the user's `.claude/settings.json`. PR #56's `_is_doc_label` filter intentionally kept `// foo` stubs (stripping only bare `//` and `//<digits>`) so users could uncomment them. Empirically, those propagated keys trigger schema-validator complaints in the user's editor — the intent was wrong. A third bug was found during verification: `compute_merged_settings` applies the doc-label filter to file-based patches but bypasses inline `extraSettings` (ui module). So the ui module's bare `"//"` doc-labels were also leaking to user settings.json. Fixes: - `_is_doc_label` now strips ALL `//`-prefixed keys. - New `_strip_doc_labels()` recursively scrubs merged settings — catches nested cases (e.g., `statusLine.// hideVimModeIndicator`) the shallow per-merge filters miss. - `extraSettings` merge path now applies the filter. - `skillOverrides: "name-only"` deleted from tier-pro (replaced with a docstring explaining why no default is shipped). - All `// foo` stubs deleted from 4 source patch files + 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()` runtime preflight emits a `[ SETTINGS WARNINGS ]` block when violations are detected. - New static `--check` step asserts no patch file ships `skillOverrides` as a non-object (regression guard — fires on the current bug when first run). New tests under `test/schema-hygiene/`: - All 5 personas render settings.json with zero `//`-prefixed keys - No settings-patch ships `skillOverrides` as a non-object - `check_settings_validates` catches 4 violation classes + clean case - `_strip_doc_labels` recursively strips at all depths Compat: existing users get the cleanup on next `cc-configure --retrofit`. Deep-merge preserves their customizations; the new strip removes leaks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 24, 2026
tigers1997
added a commit
that referenced
this pull request
May 30, 2026
… F1+F3) (#73) 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dogfood from upgrading an adjacent project surfaced two real Claude Code settings-validator complaints. Both are configurator bugs introduced by PRs #17, #18, #56, and #57.
Bug 1 —
skillOverrides: "name-only"(string form, PR #57). The current Claude Code schema requires the per-skill object map ({"skill-name": "name-only"}); the string form was based on an incorrect assumption documented in the tier-pro patch's//comment. Verified againstcode.claude.com/docs/en/settings: only object form is documented, and the setting explicitly does not apply to plugin skills, further narrowing its usefulness as a tier-wide default.Bug 2 —
// foo"commented opt-in stub" pattern (PRs #17, #18, #56). These literal top-level keys propagate from source patches into the user's.claude/settings.json, where Claude Code's editor schema rejects them as unknown properties. PR #56's_is_doc_labelfilter intentionally kept// fookeys while stripping bare//and//<digits>; the dogfood proves that intent was wrong.Third bug found during verification:
compute_merged_settingsapplies the doc-label filter to file-based patches (lines 473, 488) but bypasses inlineextraSettings(line 477-478). The ui module's bare"//"keys were also leaking to user settings.json — not just the// foostubs.What changes
configure.py_is_doc_labelstrips ALL//-prefixed keys; new_strip_doc_labels()recursive helper applied at end ofcompute_merged_settings;extraSettingsmerge filter added; newcheck_settings_validates()runtime preflight; new static--checkstep assertsskillOverridesshapetemplates/token-efficiency/settings-patch.tier-pro.jsonskillOverrides: "name-only"; replaced docstring with corrected rationaletemplates/safety/settings-patch.json,templates/multi-agent/settings-patch.json,templates/git-workflow/settings-patch.json// fooopt-in stubs; updated//Ndocstrings to point at the new opt-in homeconfig_schema.py(ui module)// hideVimModeIndicator+// subagentStatusLinefromextraSettingstemplates/core/dot-claude/settings.local.json.example.examplesuffix means CC doesn't parse this file directly, so// foostubs are safe). Covers: sandbox, worktree, prUrlTemplate, subagentStatusLine + sibling notes on hideVimModeIndicator, skillOverridestest/schema-hygiene/CHANGELOG.md## UnreleasedVerification
The new static
--checkstep fires on the current bug when run for the first time (before the fix is applied). After the fix:Rendered settings.json under
solo-experiencednow has top-level keys:['$schema', 'autoMode', 'env', 'hooks', 'model', 'permissions', 'statusLine']— zero//-prefixed keys at any depth, noskillOverrides.All 5 persona snapshot fixtures still match (file paths unchanged; we changed content only).
Test plan
python3 configure.py --checkpasses (would have failed before the tier-pro fix — verified via the newskillOverrides-shape static check fired on the bug).claude/settings.json// statusLineVimstub — uncommenting produced wrong top-level key; broken// envCapstub — synthetic key name); third finding (placeholder keys in// skillOverrides) addressed by removing the placeholdersCompat for downstream users
Users on cc-configure 2.6.0 with a populated
.claude/settings.jsonget the cleanup on nextcc-configure --retrofit. The deep-merge preserves their customizations; the new strip removes the stub-leaks. Users who manually uncommented and activated an opt-in keep the activated value (it's a real key likesandbox, not a//-prefixed one).Users who want one of the formerly-shipped opt-ins (
sandbox,worktree,prUrlTemplate,subagentStatusLine,skillOverrides) can copy the stub fromsettings.local.json.exampleinto their personalsettings.local.jsonand uncomment.🤖 Generated with Claude Code