fix(skill-quality): drop the duplicate version member from plugin.json - #1494
Conversation
The manifest carried two `version` members, 0.11.0 and 0.12.0, so the advertised release was parser-dependent: last-wins readers resolve 0.12.0, first-wins readers retain 0.11.0, and duplicate-rejecting consumers can reject the manifest outright. Any of those can misidentify or block a plugin update. 0.12.0 is correct. Main carried 0.11.0 before #1096 (released in #1450), and this plugin's CHANGELOG has `## [0.12.0]` above main's `## [0.11.0]`, so the 0.11.0 member is the stale one. Introduced resolving a main merge in #1096: both sides of that conflict carried 0.11.0, so git kept the version line as common context OUTSIDE the conflict region and placed only the differing description inside it, and a scripted replacement of that region emitted a fresh version member above the surviving one. Closes #1492
|
Claude finished @kyle-sexton's task in 51s —— View job Review
LGTM. The fix is correct, minimal, and well-reasoned. Diff: The deletion removes Correctness:
Scope: Single-file, single-deletion, no test or documentation changes needed — the manifest carries no executable logic. Verification note: The PR body's observation that No issues. Ready to merge. |
|
Claude finished @kyle-sexton's task in 35s —— View job Security Review
No security issues found. The sole change is a one-line deletion from a static JSON manifest: - "version": "0.11.0",
"version": "0.12.0",
There is nothing security-relevant to flag:
The change is a correctness fix (removing a parser-dependent duplicate key) with no security footprint. |
…1506) Closes #1498 ## Summary *This was generated by AI during work-loop execution.* No gate in CI could detect a duplicate key in a plugin/marketplace manifest. JSON Schema validates the already-parsed document (structurally blind to a repeated member), and every consumer downstream resolves it last-wins with no signal — the exact shape that shipped `plugins/skill-quality/.claude-plugin/plugin.json` with two `"version"` members through a fully green 27-context suite (#1492). ## Fix Added `scripts/check-manifest-duplicate-keys.py`: reads each `plugins/*/.claude-plugin/plugin.json` and `.claude-plugin/marketplace.json` via `json.loads(text, object_pairs_hook=...)`, intercepting the raw key/value pairs of every JSON object literal (at any nesting depth) *before* the standard de-duplicating collapse into a dict. A duplicate is recorded (not raised) so the parse still completes last-wins — identical to how `json.load`/`JSON.parse`/`jq` resolve it — and every distinct duplicate key in a file is reported in one pass. Wired into the `hygiene` job in `.github/workflows/ci.yml`, directly adjacent to the existing "Validate plugin manifests" step (per the issue's own routing rationale — same file scope, same tier as "does it parse"/"does it match the schema"), gated the same way the four `check-jsonschema` steps are (skipped only on a provably docs-only diff), and its outcome is added as one line to `scripts/aggregate-hygiene-results.sh`'s existing `CHECK_RESULTS` block so a duplicate key fails the job exactly like any other hygiene check. ## Verification - `bash scripts/check-manifest-duplicate-keys.test.sh` — 17 unit/CLI tests, all passing (nested-object duplicates, sibling-object non-duplicates, multiple distinct duplicate keys in one file, malformed JSON *not* double-reported, default-discovery glob covering both `plugin.json` and `marketplace.json`, missing-file handling, exit codes). - `test_catches_the_1492_shaped_duplicate_version_key` reproduces the actual #1492 defect shape (two `"version"` members) as an inline fixture and asserts the gate fails on it — the fixture proving the gate catches a real duplicate key, per the issue's own ask. - Live empirical cross-check against the two claims in the issue's verification table, run locally against a copy of the real (now-fixed) `plugins/skill-quality/.claude-plugin/plugin.json` with the duplicate `"version"` member re-injected: - `check-jsonschema --schemafile https://json.schemastore.org/claude-code-plugin-manifest.json <file>` → `ok -- validation done`, exit 0 (confirms schema validation is blind to it). - `python3 scripts/check-manifest-duplicate-keys.py <file>` → `DUPLICATE KEY: ... defines 'version' more than once ...`, exit 1 (confirms the new gate catches exactly what schema validation cannot). - `python3 scripts/check-manifest-duplicate-keys.py` (default discovery, no args) against the actual repo tree → `No duplicate JSON object keys found in 62 manifest file(s).`, exit 0 — no false positives on the real manifest set. - `actionlint .github/workflows/ci.yml` — clean. - `shellcheck --rcfile=.shellcheckrc scripts/check-manifest-duplicate-keys.test.sh` — clean. - YAML parse + step-order check of the modified `hygiene` job — new steps land in the intended position, ids wired correctly into `CHECK_RESULTS`. - `typos --config _typos.toml` over the new/changed files — clean. - No `plugins/*` manifest touched, so the per-plugin CHANGELOG-parity gate does not apply to this change. ## Related - #1492 — the shipped duplicate-key instance this gate would have caught. - #1494 — the manifest fix for #1492. - #1096, #1450 — the version-collision conflict shape that produces this defect class. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ing YAML comments (#2002) Three stranded post-merge review findings on the `skill-quality` plugin, plus the repository-level doc contradiction filed alongside them. Reviews posted after their PRs merged were invisible to the merge gate, so these sat unread. ## Zero-padded integer overrides were parsed as octal `require_positive_number`'s `^[0-9]+$` accepts a zero-padded value, but bash arithmetic and `printf %d` then read it in base 8. Reproduced three ways against `origin/main` in a detached worktree: - `CHECK_SKILL_LISTING_BUDGET_CHARS=08` — `printf: 08: invalid octal number`, the budget rendered as `0`, and the report still announced `OK` and exited 0. - `CHECK_SKILL_LISTING_BUDGET_CHARS=0123` — budget silently became 83. - `CHECK_SKILL_LISTING_MAX_DESC_CHARS=010` — entries capped at 8 instead of the requested 10. Accepted integer overrides are now forced to base 10 at the one place the digits become a number. All three `require_positive_number ... int` call sites are covered. The ratio and fraction overrides are deliberately left alone: `0.01` is the documented default fraction and must keep its leading zero, and both reach only `awk`, which has no octal input. `CHECK_SKILL_LISTING_CHARS_PER_TOKEN` is likewise untouched because it never reaches `$(( ))` or `%d`. No other script in this plugin takes a numeric environment override, so there is no sibling occurrence of the pattern left behind. ## A trailing YAML comment was measured as part of the listing scalars `skill_frontmatter::field` returned the comment along with the value, which also hid the surrounding quotes from `strip_quotes` so the quoting was counted too. A fixture with commented `description` and `when_to_use` scalars measured 52 characters against a true 15, producing false overflow warnings and wrong contributor sizes. The fix went into the shared helper rather than the reporter, because `check-skill.sh` reads the same fields through it — the per-skill entry cap (Check 2) and the trigger-preservation diff had the same defect. Stripping is quote-aware and confined to the plain/flow branch: inside a block scalar a `#` is content, such as a markdown heading in a `description: |` body. `normalize_bool`'s own `sed` comment strip is now redundant and was removed, along with the comment asserting that comment-stripping is "never applied to `description` / `when_to_use`". It has exactly one caller and that caller sources its value through `skill_frontmatter::field`, so nothing loses the strip. Blast radius was measured rather than predicted: every `SKILL.md` under `plugins/` was run through both the old and the new extractor and the measured `description` / `when_to_use` lengths are identical across the repo — no real skill's measurement moves. ## The fresh-eyes rule contradicted the dispatch ladder `docs/PLUGIN-PHILOSOPHY.md` stated the normative rule as requiring "a named subagent" while its own dispatch ladder makes a generic fresh-context subagent the default and its named-agent bar says a named agent is earned, not default. An author following the rule would create a named agent unnecessarily. Resolved toward the ladder rather than hedged, because that is what the repo actually enforces: the delegation-wording detector in `check-skill.sh` accepts a generic worker — it matches `fresh-context` plus any of agent/subagent/worker/advisor/reviewer/verifier/ dispatch/delegate — and never requires a named agent. `docs/topics/fresh-eyes-checkpoint-audit/PLAN.md` specified this same reconciliation ("generic or named"); it was applied to the preceding paragraph and missed on the rule sentence itself. The author-facing spec `skills/check/reference/fresh-eyes-declarations.md` is doctrine-agnostic and needed no change. Folding this nine-word documentation clause into the plugin PR instead of buying a second full CI cycle is a deliberate, operator-approved deviation from one-PR-per-plugin. ## Also verified, no change needed The fourth finding in this batch reported two `version` members in `plugins/skill-quality/.claude-plugin/plugin.json`. Already fixed on main by `dcfa7c8b70` (#1494); the manifest carries exactly one. Recorded here so the thread resolves against evidence rather than silence. ## Testing Seven regression cases added to `check-listing-budget.test.sh` covering each octal manifestation, the decimal fraction that must keep working, the quoted and plain comment forms, a `#` that is content rather than a comment, and a `#` inside a block scalar. Gates run green from the worktree root: the listing-budget and `check-skill` suites, `run-plugin-tests.sh`, `check-changelog-parity.sh --check` / `--check-bump` / `--check-order`, `check-changed-skills.sh`, `check-shell-portability.sh` (+ its suite), `check-contract-slice-prune.sh --check` / `--check-diff`, `check-contract-clause-coverage.py`, ShellCheck over the three changed scripts, markdownlint over the two changed docs, and the CI invocation `check-listing-budget.sh plugins/*/skills`. ## Related Refs #1938 — the stranded-findings triage sweep these threads came from. No linked issue --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
plugins/skill-quality/.claude-plugin/plugin.jsoncarried twoversionmembers onmain:The advertised release was therefore parser-dependent — last-wins readers (
jq, Pythonjson.load) resolve0.12.0, first-wins readers retain0.11.0, and duplicate-rejecting consumers can reject the manifest outright. Any of those can misidentify or block a plugin update.Why
0.12.0is correct.maincarried0.11.0before #1096, released in #1450, and this plugin's CHANGELOG has## [0.12.0]above main's## [0.11.0]. The0.11.0member is the stale one, and it is what this PR deletes.Scope. One file, one deletion. Nothing else in the manifest and nothing else in the repo.
How it happened. Introduced while resolving a
mainmerge in #1096. Both sides of that conflict carried"version": "0.11.0"— main had released 0.11.0 in #1450 and the branch also claimed 0.11.0 — so git treated the version line as common context outside the conflict region and placed only the differingdescriptioninside it. A scripted replacement of the conflict region then emitted a freshversion+descriptionpair above the surviving common-context one.Test plan
A duplicate-key-aware parse is the only check that observes this, and it is clean after the change:
Also confirmed:
git diff --statis exactly1 file changed, 1 deletion(-).## [0.12.0], so the manifest and the release notes agree.changelog-parity-gateandplugin-gatepass in CI on this branch.Worth flagging for reviewers:
json.load,jq, and JSON Schema validation all resolve duplicate keys last-wins, so every one of them reports0.12.0whether or not the duplicate is present. That is why this defect passed a fully green 27-context suite on #1096, and it is why the fix is verified with a duplicate-key hook rather than a plain parse.Related
0.11.0onmain, the collision that produced the conflict shape.(verified: schema validation cannot express key uniqueness, and every consumer resolves last-wins).
Closes #1492