From 793feda5a0f3b315e9c789f9917635622455e486 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 20 Jun 2026 15:51:16 +0900 Subject: [PATCH] Guard plugin/skill version drift in the build (issue #274) (#278) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Guard plugin/skill version drift in the build (issue #274) The Claude Code marketplace installs from the committed ./plugin subtree, so a version disagreement between the hand-edited manifests and the generated subtree ships stale content under a wrong version. This is the class of bug reported in #274: a version bump that doesn't regenerate ./plugin (e.g. PR #252, where root plugin.json was 3.7.0 while plugin/.claude-plugin/plugin.json was still 3.6.0) merges a drift window onto main, and marketplace/Cowork installs pull the stale subtree. Add a build-time validator that treats root .claude-plugin/plugin.json as the source of truth and fails the build if any of these disagree: - .claude-plugin/marketplace.json plugins[0].version (hand-edited; the post-merge sync workflow never bumps versions, so it can't repair a mismatch here) - plugin/.claude-plugin/plugin.json version (generated subtree) - plugin/skills/impeccable/SKILL.md frontmatter version (bundled skill) It only fires on an inconsistent bump; PRs that don't touch versions keep every file in agreement and stay silent. The pure comparison lives in scripts/lib/validate-plugin-versions.js with direct unit coverage; build.js owns the logging and the non-zero exit. Documents the regenerate-on-bump step in CLAUDE.md's Versioning section. Co-Authored-By: Claude Opus 4.8 (1M context) * Harden version-drift collector against malformed/incomplete manifests Address Greptile review on #278: - Wrap every file read/parse in a sentinel helper (extractFromFile) so a half-edited manifest — the exact state during a version bump — yields a clean "could not parse (...)" diagnostic naming the file instead of a raw JSON.parse stack trace out of build(). - Report a present-but-malformed root plugin.json, or one missing its `version` field, as an explicit error. Previously `undefined` version short-circuited the build wrapper's `source == null` guard and passed silently. collectPluginVersions now returns an `errors` array; build.js fails on errors + mismatches combined, and only the genuinely-absent root manifest is a no-op skip. Adds 4 unit tests: malformed checked manifest, malformed root, missing version field, and the absent-root no-errors case. Co-Authored-By: Claude Opus 4.8 (1M context) * Make SKILL.md frontmatter version read CRLF-tolerant Address Cursor Bugbot review on #278: readSkillFrontmatterVersion only matched `\n` delimiters, while the shared parseFrontmatter in scripts/lib/utils.js accepts `\r?\n`. A bundled SKILL.md saved with CRLF line endings would parse to a null version and trip a false mismatch against root plugin.json even when the version line is correct. Match the shared parser's `\r?\n` tolerance and drop the `$` anchor on the version line (it would not match before a `\r`). Adds CRLF coverage for both readSkillFrontmatterVersion and collectPluginVersions. Co-Authored-By: Claude Opus 4.8 (1M context) * Re-trigger CI (no file change) CI did not fire for 5cda9f6b; force a fresh run on the current tree. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CLAUDE.md | 3 +- scripts/build.js | 38 ++++- scripts/lib/validate-plugin-versions.js | 122 ++++++++++++++++ scripts/test-suites.mjs | 1 + tests/validate-plugin-versions.test.js | 177 ++++++++++++++++++++++++ 5 files changed, 339 insertions(+), 2 deletions(-) create mode 100644 scripts/lib/validate-plugin-versions.js create mode 100644 tests/validate-plugin-versions.test.js diff --git a/CLAUDE.md b/CLAUDE.md index 59368668e..c52972136 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -222,9 +222,10 @@ There are three independently versioned components. Only bump the one(s) that ac - Bump when: CLI code changes (`cli/bin/`, `cli/engine/detect-antipatterns.mjs`, etc.) **Skills** (Claude Code plugin / skill definitions): -- `.claude-plugin/plugin.json` → `version` +- `.claude-plugin/plugin.json` → `version` (source of truth) - `.claude-plugin/marketplace.json` → `plugins[0].version` - Bump when: skill content changes (`skill/`, reference files, command metadata, etc.) +- After bumping, run `bun run build:release` so the committed `./plugin` subtree (`plugin/.claude-plugin/plugin.json` + `plugin/skills/impeccable/SKILL.md`) is regenerated to the new version. The build validator (`validatePluginVersions` in `scripts/build.js`) fails if `marketplace.json`, the `./plugin` manifest, or the bundled `SKILL.md` frontmatter disagree with `plugin.json` — this guards the marketplace install path against version drift (issue #274). **Chrome extension**: - `extension/manifest.json` → `version` diff --git a/scripts/build.js b/scripts/build.js index e27a00c53..a405cc939 100644 --- a/scripts/build.js +++ b/scripts/build.js @@ -23,6 +23,7 @@ import { generateApiData } from './lib/api-data.js'; import { createTransformer, PROVIDERS } from './lib/transformers/index.js'; import { hooksJsonFor, buildClaudePluginHooksManifest } from './lib/transformers/hooks.js'; import { createAllZips } from './lib/zip.js'; +import { collectPluginVersions } from './lib/validate-plugin-versions.js'; import { ANTIPATTERNS } from '../cli/engine/registry/antipatterns.mjs'; // Sub-page generation is now handled by Astro content collections. @@ -112,6 +113,37 @@ function generateCounts(rootDir, skills, buildDir) { return errors; } +/** + * Guard against plugin/skill version drift (issue #274). The pure comparison + * lives in ./lib/validate-plugin-versions.js (so it's unit-tested directly); + * this wrapper owns the console output and the error count the build gates on. + */ +function validatePluginVersions(rootDir) { + const { source, mismatches, errors } = collectPluginVersions(rootDir); + // No root manifest at all → nothing to check (source null with no errors). + if (source == null && errors.length === 0) return 0; + + for (const { relPath, reason } of errors) { + console.error(` ❌ ${relPath}: ${reason}`); + } + for (const { relPath, found, expected } of mismatches) { + console.error( + ` ❌ ${relPath}: version "${found}" disagrees with .claude-plugin/plugin.json "${expected}"`, + ); + } + + const total = errors.length + mismatches.length; + if (total > 0) { + console.error( + `\n❌ ${total} plugin/skill version problem(s). Bump every version together and run ` + + `\`bun run build:release\` to regenerate the ./plugin subtree (issue #274).`, + ); + } else { + console.log(`✓ Plugin/skill versions agree: ${source}`); + } + return total; +} + function validateSkillFrontmatter(skills) { let errors = 0; @@ -766,6 +798,10 @@ async function build() { // Generate authoritative counts and validate references const countErrors = generateCounts(ROOT_DIR, skills, buildDir); + // Guard plugin/skill version drift: marketplace + ./plugin subtree must + // match root plugin.json so marketplace installs never ship a stale version. + const versionErrors = validatePluginVersions(ROOT_DIR); + // Verify every hand-authored HTML page carries the shared site header const headerErrors = validateSiteHeader(ROOT_DIR); @@ -779,7 +815,7 @@ async function build() { // that has no technical reading. Hardening repetition is intentionally allowed. const skillProseErrors = validateSkillProse(ROOT_DIR); - if (countErrors > 0 || headerErrors > 0 || themeErrors > 0 || proseErrors > 0 || skillProseErrors > 0) { + if (countErrors > 0 || versionErrors > 0 || headerErrors > 0 || themeErrors > 0 || proseErrors > 0 || skillProseErrors > 0) { process.exit(1); } diff --git a/scripts/lib/validate-plugin-versions.js b/scripts/lib/validate-plugin-versions.js new file mode 100644 index 000000000..0681d1d45 --- /dev/null +++ b/scripts/lib/validate-plugin-versions.js @@ -0,0 +1,122 @@ +/** + * Plugin/skill version-drift detection (issue #274). + * + * The Claude Code marketplace installs from the committed `./plugin` subtree, + * so any version disagreement between the hand-edited manifests and the + * generated subtree ships stale content reporting a wrong version. Root + * `.claude-plugin/plugin.json` is the single source of truth (build() reads + * skillsVersion from it). Every other version-bearing file must match it: + * + * - `.claude-plugin/marketplace.json` plugins[0].version — hand-edited + * alongside plugin.json; the post-merge sync workflow can't repair a + * mismatch here because it never bumps versions. + * - `plugin/.claude-plugin/plugin.json` version — generated, derived from + * root at build:release; checked so a bump that forgets to regenerate the + * subtree fails loudly instead of merging a drift window onto main. + * - `plugin/skills/impeccable/SKILL.md` frontmatter version — generated; + * same rationale. + * + * The collector is pure (filesystem-in, data-out) so it can be unit-tested + * against fixtures; build.js owns the logging and the non-zero exit. + */ +import fs from 'fs'; +import path from 'path'; + +/** + * Pull the `version:` value out of a SKILL.md leading frontmatter block. + * CRLF-tolerant (`\r?\n`) to match the shared parseFrontmatter in + * scripts/lib/utils.js — a bundle saved with CRLF line endings must not read + * as a null version and trip a false mismatch. `(.+)` stops at the line + * terminator (so a trailing `\r` is excluded), and `.trim()` mops up the rest. + */ +export function readSkillFrontmatterVersion(content) { + const fm = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!fm) return null; + const line = fm[1].match(/^version:\s*(.+)/m); + return line ? line[1].trim().replace(/^['"]|['"]$/g, '') : null; +} + +/** + * Read a file and extract a value, turning read/parse failures into a clean + * sentinel instead of a raw throw. A version bump is exactly the moment a + * manifest is half-edited, so a malformed file must produce an actionable + * diagnostic naming the file, not a stack trace out of build(). + * + * @returns {{ value: any } | { error: string }} + */ +function extractFromFile(absPath, extract) { + let raw; + try { + raw = fs.readFileSync(absPath, 'utf-8'); + } catch (err) { + return { error: `could not read file (${err.code || err.message})` }; + } + try { + return { value: extract(raw) }; + } catch (err) { + return { error: `could not parse (${err.message})` }; + } +} + +/** + * Compare every version-bearing plugin/skill file against root plugin.json. + * + * @param {string} rootDir repository root + * @returns {{ + * source: string|null, + * checked: Array<{relPath:string, found:any}>, + * mismatches: Array<{relPath:string, found:any, expected:string}>, + * errors: Array<{relPath:string, reason:string}>, + * }} + * `source` is null only when root plugin.json is absent (nothing to check). + * A present-but-malformed root, or one missing its `version` field, instead + * reports an entry in `errors` so the build fails loudly rather than passing. + */ +export function collectPluginVersions(rootDir) { + const rootRel = '.claude-plugin/plugin.json'; + const rootManifestPath = path.join(rootDir, rootRel); + const empty = { source: null, checked: [], mismatches: [], errors: [] }; + if (!fs.existsSync(rootManifestPath)) return empty; + + const rootResult = extractFromFile(rootManifestPath, (raw) => JSON.parse(raw).version); + if (rootResult.error) { + return { ...empty, errors: [{ relPath: rootRel, reason: rootResult.error }] }; + } + const source = rootResult.value; + if (source == null) { + return { ...empty, errors: [{ relPath: rootRel, reason: 'missing "version" field' }] }; + } + + const checks = [ + { + relPath: '.claude-plugin/marketplace.json', + read: (raw) => JSON.parse(raw).plugins?.[0]?.version, + }, + { + relPath: 'plugin/.claude-plugin/plugin.json', + read: (raw) => JSON.parse(raw).version, + }, + { + relPath: 'plugin/skills/impeccable/SKILL.md', + read: (raw) => readSkillFrontmatterVersion(raw), + }, + ]; + + const checked = []; + const mismatches = []; + const errors = []; + for (const { relPath, read } of checks) { + const absPath = path.join(rootDir, relPath); + if (!fs.existsSync(absPath)) continue; + const result = extractFromFile(absPath, read); + if (result.error) { + errors.push({ relPath, reason: result.error }); + continue; + } + const found = result.value; + checked.push({ relPath, found }); + if (found !== source) mismatches.push({ relPath, found, expected: source }); + } + + return { source, checked, mismatches, errors }; +} diff --git a/scripts/test-suites.mjs b/scripts/test-suites.mjs index c911626f4..afe89cdb1 100644 --- a/scripts/test-suites.mjs +++ b/scripts/test-suites.mjs @@ -47,6 +47,7 @@ export const SUITES = { 'tests/lib/transformers/providers.test.js', 'tests/docs-integrity.test.js', 'tests/skills-cli.test.js', + 'tests/validate-plugin-versions.test.js', ], }, { diff --git a/tests/validate-plugin-versions.test.js b/tests/validate-plugin-versions.test.js new file mode 100644 index 000000000..c7d2dbf2c --- /dev/null +++ b/tests/validate-plugin-versions.test.js @@ -0,0 +1,177 @@ +/** + * Unit coverage for the plugin/skill version-drift guard (issue #274). + * + * The Claude Code marketplace installs from the committed `./plugin` subtree, + * so a version disagreement between the hand-edited manifests and the + * generated subtree ships stale content under a wrong version number. The + * guard treats root `.claude-plugin/plugin.json` as the source of truth and + * flags any other version-bearing file that disagrees. + */ +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { + collectPluginVersions, + readSkillFrontmatterVersion, +} from '../scripts/lib/validate-plugin-versions.js'; + +function skillMd(version) { + return `---\nname: impeccable\nversion: ${version}\nuser-invocable: true\n---\n\nBody.\n`; +} + +function writeFixture(root, { plugin, marketplace, subtreePlugin, skill } = {}) { + const write = (rel, contents) => { + const abs = path.join(root, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, contents); + }; + if (plugin !== undefined) { + write('.claude-plugin/plugin.json', JSON.stringify({ name: 'impeccable', version: plugin }, null, 2)); + } + if (marketplace !== undefined) { + write('.claude-plugin/marketplace.json', JSON.stringify({ plugins: [{ name: 'impeccable', version: marketplace }] }, null, 2)); + } + if (subtreePlugin !== undefined) { + write('plugin/.claude-plugin/plugin.json', JSON.stringify({ name: 'impeccable', version: subtreePlugin, skills: './skills/' }, null, 2)); + } + if (skill !== undefined) { + write('plugin/skills/impeccable/SKILL.md', skillMd(skill)); + } +} + +describe('collectPluginVersions', () => { + let root; + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-ver-')); + }); + afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + test('no mismatches when every version agrees', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.7.1', subtreePlugin: '3.7.1', skill: '3.7.1' }); + const { source, mismatches } = collectPluginVersions(root); + expect(source).toBe('3.7.1'); + expect(mismatches).toEqual([]); + }); + + test('flags a lagging marketplace.json (the half the sync workflow cannot repair)', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.1.1', subtreePlugin: '3.7.1', skill: '3.7.1' }); + const { mismatches } = collectPluginVersions(root); + expect(mismatches).toEqual([ + { relPath: '.claude-plugin/marketplace.json', found: '3.1.1', expected: '3.7.1' }, + ]); + }); + + test('flags a stale ./plugin subtree manifest', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.7.1', subtreePlugin: '3.6.0', skill: '3.7.1' }); + const { mismatches } = collectPluginVersions(root); + expect(mismatches).toEqual([ + { relPath: 'plugin/.claude-plugin/plugin.json', found: '3.6.0', expected: '3.7.1' }, + ]); + }); + + test('flags a stale bundled SKILL.md frontmatter version', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.7.1', subtreePlugin: '3.7.1', skill: '3.1.1' }); + const { mismatches } = collectPluginVersions(root); + expect(mismatches).toEqual([ + { relPath: 'plugin/skills/impeccable/SKILL.md', found: '3.1.1', expected: '3.7.1' }, + ]); + }); + + test('reports every drifted file at once', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.1.1', subtreePlugin: '3.5.0', skill: '3.1.1' }); + const { mismatches } = collectPluginVersions(root); + expect(mismatches.map((m) => m.relPath)).toEqual([ + '.claude-plugin/marketplace.json', + 'plugin/.claude-plugin/plugin.json', + 'plugin/skills/impeccable/SKILL.md', + ]); + }); + + test('skips files that do not exist instead of throwing', () => { + writeFixture(root, { plugin: '3.7.1' }); // only root manifest present + const { source, checked, mismatches, errors } = collectPluginVersions(root); + expect(source).toBe('3.7.1'); + expect(checked).toEqual([]); + expect(mismatches).toEqual([]); + expect(errors).toEqual([]); + }); + + test('returns a null source with no errors when root plugin.json is absent', () => { + const { source, mismatches, errors } = collectPluginVersions(root); + expect(source).toBeNull(); + expect(mismatches).toEqual([]); + expect(errors).toEqual([]); + }); + + test('reports a malformed checked manifest as an error instead of throwing', () => { + writeFixture(root, { plugin: '3.7.1', subtreePlugin: '3.7.1', skill: '3.7.1' }); + // marketplace.json half-edited mid-bump: invalid JSON. + fs.writeFileSync(path.join(root, '.claude-plugin/marketplace.json'), '{ "plugins": [ { "version": '); + const { mismatches, errors } = collectPluginVersions(root); + expect(mismatches).toEqual([]); + expect(errors).toHaveLength(1); + expect(errors[0].relPath).toBe('.claude-plugin/marketplace.json'); + expect(errors[0].reason).toMatch(/parse/i); + }); + + test('reports a malformed root plugin.json as an error, not a thrown stack', () => { + fs.mkdirSync(path.join(root, '.claude-plugin'), { recursive: true }); + fs.writeFileSync(path.join(root, '.claude-plugin/plugin.json'), '{ not json'); + const { source, errors } = collectPluginVersions(root); + expect(source).toBeNull(); + expect(errors).toHaveLength(1); + expect(errors[0].relPath).toBe('.claude-plugin/plugin.json'); + expect(errors[0].reason).toMatch(/parse/i); + }); + + test('flags a root plugin.json that exists but has no version field', () => { + fs.mkdirSync(path.join(root, '.claude-plugin'), { recursive: true }); + fs.writeFileSync(path.join(root, '.claude-plugin/plugin.json'), JSON.stringify({ name: 'impeccable' })); + const { source, errors } = collectPluginVersions(root); + // source stays null, but it is reported as an error rather than silently passing. + expect(source).toBeNull(); + expect(errors).toEqual([{ relPath: '.claude-plugin/plugin.json', reason: 'missing "version" field' }]); + }); +}); + +describe('readSkillFrontmatterVersion', () => { + test('reads an unquoted version', () => { + expect(readSkillFrontmatterVersion(skillMd('3.7.1'))).toBe('3.7.1'); + }); + + test('strips surrounding quotes', () => { + expect(readSkillFrontmatterVersion('---\nversion: "3.7.1"\n---\n')).toBe('3.7.1'); + }); + + test('returns null when there is no frontmatter block', () => { + expect(readSkillFrontmatterVersion('no frontmatter here')).toBeNull(); + }); + + test('reads a version from CRLF-encoded frontmatter', () => { + const crlf = '---\r\nname: impeccable\r\nversion: 3.7.1\r\nuser-invocable: true\r\n---\r\n\r\nBody.\r\n'; + expect(readSkillFrontmatterVersion(crlf)).toBe('3.7.1'); + }); +}); + +describe('collectPluginVersions with CRLF line endings', () => { + let root; + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-ver-crlf-')); + }); + afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); + }); + + test('a CRLF-saved SKILL.md at the right version is not a false mismatch', () => { + writeFixture(root, { plugin: '3.7.1', marketplace: '3.7.1', subtreePlugin: '3.7.1', skill: '3.7.1' }); + // Re-save the bundled SKILL.md with CRLF line endings. + const skillPath = path.join(root, 'plugin/skills/impeccable/SKILL.md'); + fs.writeFileSync(skillPath, fs.readFileSync(skillPath, 'utf-8').replace(/\n/g, '\r\n')); + const { mismatches, errors } = collectPluginVersions(root); + expect(mismatches).toEqual([]); + expect(errors).toEqual([]); + }); +});