From 482368511ace07982a7cd3a23dd60cf62d6f68c8 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Tue, 1 Sep 2026 20:21:01 -0400 Subject: [PATCH] Fix Codex skill version metadata (#703) Move Codex and .agents skill versions under metadata while keeping all version readers compatible with legacy top-level frontmatter.\n\nAI assistance: prepared with Codex under maintainer direction. --- cli/bin/commands/skills.mjs | 38 ++++++++++++++++++++++-- scripts/lib/transformers/factory.js | 12 +++++++- scripts/lib/transformers/providers.js | 4 +++ scripts/lib/utils.js | 3 ++ scripts/lib/validate-plugin-versions.js | 38 ++++++++++++++++++++---- skill/scripts/context.mjs | 36 ++++++++++++++++++++-- tests/context.test.mjs | 20 +++++++++++-- tests/lib/transformers/providers.test.js | 10 +++++-- tests/lib/utils.test.js | 9 ++++++ tests/skills-cli.test.js | 27 +++++++++++++++++ tests/validate-plugin-versions.test.js | 10 +++++++ 11 files changed, 192 insertions(+), 15 deletions(-) diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index faa98a572..e15904104 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -557,6 +557,39 @@ async function showHelp() { // ─── version helpers ───────────────────────────────────────────────────────── +function parseSkillFrontmatterVersion(content) { + const match = String(content).match(/^---[ \t]*\r?\n([\s\S]*?)\r?\n---(?:[ \t]*\r?\n|[ \t]*$)/); + if (!match) return null; + + let metadataVersion = null; + let topLevelVersion = null; + let inMetadata = false; + let metadataIndent = null; + + for (const line of match[1].split(/\r?\n/)) { + if (!line.trim() || line.trimStart().startsWith('#')) continue; + const indentText = line.match(/^[ \t]*/)[0]; + const indent = indentText.replace(/\t/g, ' ').length; + + if (indent === 0) { + inMetadata = /^metadata:\s*(?:#.*)?$/.test(line); + metadataIndent = null; + const version = line.match(/^version:\s*(.+?)\s*$/); + if (version) topLevelVersion = version[1]; + continue; + } + + if (!inMetadata) continue; + if (metadataIndent === null) metadataIndent = indent; + if (indent !== metadataIndent) continue; + const version = line.trim().match(/^version:\s*(.+?)\s*$/); + if (version) metadataVersion = version[1]; + } + + const value = metadataVersion || topLevelVersion; + return value ? value.trim().replace(/^(["'])(.*)\1$/, '$2') : null; +} + /** * Read the skills version from the impeccable SKILL.md frontmatter. */ @@ -566,8 +599,8 @@ function getSkillsVersion(root, scope) { const skillMd = join(skillsDir, 'impeccable', 'SKILL.md'); if (!existsSync(skillMd)) continue; const content = readFileSync(skillMd, 'utf-8'); - const match = content.match(/^version:\s*(.+)$/m); - if (match) return match[1].trim().replace(/^["']|["']$/g, ''); + const version = parseSkillFrontmatterVersion(content); + if (version) return version; } } return null; @@ -2483,6 +2516,7 @@ export { expectedHookDests, extractZip, formatInstallDetectionLines, + getSkillsVersion, isUpToDate, hermesGlobalHome, HOME_SKILLS_DIR_OVERRIDES, diff --git a/scripts/lib/transformers/factory.js b/scripts/lib/transformers/factory.js index 5dfc19c30..684597ff5 100644 --- a/scripts/lib/transformers/factory.js +++ b/scripts/lib/transformers/factory.js @@ -241,6 +241,7 @@ export function createTransformer(config) { providerTags = [provider], writeOpenAIMetadata = false, includeVersion = true, + versionInMetadata = false, } = config; const placeholderKey = placeholderProvider || provider; @@ -274,7 +275,9 @@ export function createTransformer(config) { name: skillName, description: skill.description, }; - if (skillsVersion && includeVersion) frontmatterObj.version = skillsVersion; + if (skillsVersion && includeVersion && !versionInMetadata) { + frontmatterObj.version = skillsVersion; + } for (const spec of activeFields) { if (spec.condition && !spec.condition(skill)) continue; @@ -282,6 +285,13 @@ export function createTransformer(config) { if (val) frontmatterObj[spec.yamlKey] = val; } + if (skillsVersion && includeVersion && versionInMetadata) { + frontmatterObj.metadata = { + ...(frontmatterObj.metadata || {}), + version: skillsVersion, + }; + } + // Replace {{command_hint}} in argument-hint with command names from metadata, // grouped by category with middle dots between groups for natural line-breaking. if (frontmatterObj['argument-hint']?.includes('{{command_hint}}')) { diff --git a/scripts/lib/transformers/providers.js b/scripts/lib/transformers/providers.js index 4fdadc515..9a7591c17 100644 --- a/scripts/lib/transformers/providers.js +++ b/scripts/lib/transformers/providers.js @@ -48,6 +48,9 @@ export const PROVIDERS = { configDir: '.codex', displayName: 'Codex', frontmatterFields: [], + // Codex's validator rejects unknown top-level keys. Version remains + // available to Impeccable's updater under the spec-defined metadata map. + versionInMetadata: true, writeOpenAIMetadata: true, // No agentFormat: the Codex subagent ships nested inside the skill's own // agents/ folder (see CODEX_SKILL_PROVIDERS in factory.js), which Codex @@ -63,6 +66,7 @@ export const PROVIDERS = { displayName: 'Codex Repo Skills', placeholderProvider: 'codex', frontmatterFields: [], + versionInMetadata: true, writeOpenAIMetadata: true, }, github: { diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index d96eff378..faa28cdbd 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -787,6 +787,9 @@ export function generateYamlFrontmatter(data) { lines.push(` - ${formatYamlScalar(item)}`); } } + } else if (value && typeof value === 'object') { + lines.push(`${key}:`); + appendYamlObject(lines, value, 2); } else if (typeof value === 'boolean') { lines.push(`${key}: ${value}`); } else { diff --git a/scripts/lib/validate-plugin-versions.js b/scripts/lib/validate-plugin-versions.js index bfb1645fa..be40131b1 100644 --- a/scripts/lib/validate-plugin-versions.js +++ b/scripts/lib/validate-plugin-versions.js @@ -25,17 +25,43 @@ import fs from 'fs'; import path from 'path'; /** - * Pull the `version:` value out of a SKILL.md leading frontmatter block. + * 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. + * as a null version and trip a false mismatch. Codex builds carry the version + * under `metadata.version`; legacy provider builds keep the top-level key. */ export function readSkillFrontmatterVersion(content) { - const fm = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + const fm = String(content).match(/^---[ \t]*\r?\n([\s\S]*?)\r?\n---(?:[ \t]*\r?\n|[ \t]*$)/); if (!fm) return null; - const line = fm[1].match(/^version:\s*(.+)/m); - return line ? line[1].trim().replace(/^['"]|['"]$/g, '') : null; + + let metadataVersion = null; + let topLevelVersion = null; + let inMetadata = false; + let metadataIndent = null; + + for (const line of fm[1].split(/\r?\n/)) { + if (!line.trim() || line.trimStart().startsWith('#')) continue; + const indentText = line.match(/^[ \t]*/)[0]; + const indent = indentText.replace(/\t/g, ' ').length; + + if (indent === 0) { + inMetadata = /^metadata:\s*(?:#.*)?$/.test(line); + metadataIndent = null; + const version = line.match(/^version:\s*(.+?)\s*$/); + if (version) topLevelVersion = version[1]; + continue; + } + + if (!inMetadata) continue; + if (metadataIndent === null) metadataIndent = indent; + if (indent !== metadataIndent) continue; + const version = line.trim().match(/^version:\s*(.+?)\s*$/); + if (version) metadataVersion = version[1]; + } + + const value = metadataVersion || topLevelVersion; + return value ? value.trim().replace(/^(["'])(.*)\1$/, '$2') : null; } /** diff --git a/skill/scripts/context.mjs b/skill/scripts/context.mjs index 41112429a..cb16553c2 100644 --- a/skill/scripts/context.mjs +++ b/skill/scripts/context.mjs @@ -962,13 +962,45 @@ export function extractPlatform(product) { * (this file lives at `/scripts/context.mjs`). Returns null when the * frontmatter is missing or unreadable. */ +function parseSkillFrontmatterVersion(content) { + const match = String(content).match(/^---[ \t]*\r?\n([\s\S]*?)\r?\n---(?:[ \t]*\r?\n|[ \t]*$)/); + if (!match) return null; + + let metadataVersion = null; + let topLevelVersion = null; + let inMetadata = false; + let metadataIndent = null; + + for (const line of match[1].split(/\r?\n/)) { + if (!line.trim() || line.trimStart().startsWith('#')) continue; + const indentText = line.match(/^[ \t]*/)[0]; + const indent = indentText.replace(/\t/g, ' ').length; + + if (indent === 0) { + inMetadata = /^metadata:\s*(?:#.*)?$/.test(line); + metadataIndent = null; + const version = line.match(/^version:\s*(.+?)\s*$/); + if (version) topLevelVersion = version[1]; + continue; + } + + if (!inMetadata) continue; + if (metadataIndent === null) metadataIndent = indent; + if (indent !== metadataIndent) continue; + const version = line.trim().match(/^version:\s*(.+?)\s*$/); + if (version) metadataVersion = version[1]; + } + + const value = metadataVersion || topLevelVersion; + return value ? value.trim().replace(/^(["'])(.*)\1$/, '$2') : null; +} + function readLocalSkillVersion() { try { const here = path.dirname(fileURLToPath(import.meta.url)); const skillMd = path.join(here, '..', 'SKILL.md'); const content = fs.readFileSync(skillMd, 'utf-8'); - const match = content.match(/^version:\s*(.+)$/m); - return match ? match[1].trim().replace(/^["']|["']$/g, '') : null; + return parseSkillFrontmatterVersion(content); } catch { return null; } diff --git a/tests/context.test.mjs b/tests/context.test.mjs index 76db0648d..c747164df 100644 --- a/tests/context.test.mjs +++ b/tests/context.test.mjs @@ -1465,11 +1465,11 @@ describe('context.mjs update check', () => { const cachePath = () => path.join(scratch, 'update-check.json'); - function setup(cacheObj, { disable = false, host } = {}) { + function setup(cacheObj, { disable = false, host, skillFrontmatter } = {}) { const skillScript = stageContextBundle(path.join(scratch, 'skill', 'scripts')); fs.writeFileSync( path.join(scratch, 'skill', 'SKILL.md'), - `---\nname: impeccable\nversion: ${LOCAL_VERSION}\n---\n\nbody\n`, + `---\n${skillFrontmatter || `name: impeccable\nversion: ${LOCAL_VERSION}`}\n---\n\nbody\n`, ); fs.writeFileSync(cachePath(), JSON.stringify(cacheObj)); const project = path.join(scratch, 'project'); @@ -1522,6 +1522,22 @@ describe('context.mjs update check', () => { assert.match(res.stdout, /^# PRODUCT\.md/); }); + it('reads metadata.version and prefers it over the legacy top-level key', () => { + const metadataOnly = run( + { lastCheck: Date.now(), latestVersion: '2.0.0' }, + { skillFrontmatter: `name: impeccable\nmetadata:\n version: ${LOCAL_VERSION}` }, + ); + assert.equal(metadataOnly.status, 0); + assert.match(metadataOnly.stdout, /installed v1\.0\.0, latest v2\.0\.0/); + + const both = run( + { lastCheck: Date.now(), latestVersion: '2.0.0' }, + { skillFrontmatter: `name: impeccable\nversion: 9.0.0\nmetadata:\n version: ${LOCAL_VERSION}` }, + ); + assert.equal(both.status, 0); + assert.match(both.stdout, /installed v1\.0\.0, latest v2\.0\.0/); + }); + // The directive used to say "ask once" and "if they agree, run it" while also // saying to continue without waiting. Nothing gated the run on an answer that // could not arrive, so the command read as the next step. It now forbids diff --git a/tests/lib/transformers/providers.test.js b/tests/lib/transformers/providers.test.js index 3906de04d..a9b2bd991 100644 --- a/tests/lib/transformers/providers.test.js +++ b/tests/lib/transformers/providers.test.js @@ -68,8 +68,14 @@ for (const [key, config] of Object.entries(PROVIDERS)) { test('should emit skillsVersion in generated skill frontmatter', () => { const skills = [{ name: 'test', description: 'Test', body: 'Body' }]; transform(skills, TEST_DIR, { skillsVersion: '1.2.3-test' }); - const parsed = parseFrontmatter(fs.readFileSync(skillPath(config, 'test'), 'utf-8')); - expect(parsed.frontmatter.version).toBe('1.2.3-test'); + const content = fs.readFileSync(skillPath(config, 'test'), 'utf-8'); + const parsed = parseFrontmatter(content); + if (key === 'codex' || key === 'agents') { + expect(parsed.frontmatter.version).toBeUndefined(); + expect(content).toContain('metadata:\n version: 1.2.3-test'); + } else { + expect(parsed.frontmatter.version).toBe('1.2.3-test'); + } }); // Field-specific tests based on provider config diff --git a/tests/lib/utils.test.js b/tests/lib/utils.test.js index d7e5d6ed8..93ac0a67c 100644 --- a/tests/lib/utils.test.js +++ b/tests/lib/utils.test.js @@ -162,6 +162,15 @@ describe('generateYamlFrontmatter', () => { expect(result).toContain('user-invocable: true'); }); + test('should generate nested metadata', () => { + const result = generateYamlFrontmatter({ + name: 'test', + metadata: { version: '1.2.3' }, + }); + + expect(result).toContain('metadata:\n version: 1.2.3'); + }); + test('should roundtrip: generate and parse back', () => { const original = { name: 'roundtrip-test', diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index 781407461..6524cea5a 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -24,6 +24,7 @@ import { downloadFile, expectedHookDests, formatInstallDetectionLines, + getSkillsVersion, mergeHookManifests, migrateUnprefixImpeccable, resolveInstallTargets, @@ -169,6 +170,32 @@ function createPrefixedInstall(root, { prefix = 'i-', providers = ['.claude'], f } } +describe('skills version discovery', () => { + test('prefers metadata.version while accepting legacy top-level version', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-skill-version-')); + const skillDir = join(tmp, '.agents', 'skills', 'impeccable'); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), [ + '---', + 'name: impeccable', + 'version: 0.9.0', + 'metadata:', + ' version: 1.2.3', + '---', + '', + 'Body.', + ].join('\n')); + + try { + expect(getSkillsVersion(tmp, 'project')).toBe('1.2.3'); + writeFileSync(join(skillDir, 'SKILL.md'), '---\nname: impeccable\nversion: 0.9.0\n---\nBody.\n'); + expect(getSkillsVersion(tmp, 'project')).toBe('0.9.0'); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); +}); + // ─── Already-installed detection ───────────────────────────────────────────── // Remote e2e blocks (real bundle downloads from impeccable.style) run only diff --git a/tests/validate-plugin-versions.test.js b/tests/validate-plugin-versions.test.js index 128e88903..efea502dc 100644 --- a/tests/validate-plugin-versions.test.js +++ b/tests/validate-plugin-versions.test.js @@ -157,6 +157,16 @@ describe('readSkillFrontmatterVersion', () => { expect(readSkillFrontmatterVersion('---\nversion: "3.7.1"\n---\n')).toBe('3.7.1'); }); + test('prefers metadata.version while accepting the legacy top-level key', () => { + const content = '---\nname: impeccable\nversion: 3.0.0\nmetadata:\n version: 3.7.1\n---\n'; + expect(readSkillFrontmatterVersion(content)).toBe('3.7.1'); + }); + + test('reads metadata.version after nested metadata fields', () => { + const content = '---\nname: impeccable\nmetadata:\n interface:\n display_name: Impeccable\n version: "3.7.1"\n---\n'; + expect(readSkillFrontmatterVersion(content)).toBe('3.7.1'); + }); + test('returns null when there is no frontmatter block', () => { expect(readSkillFrontmatterVersion('no frontmatter here')).toBeNull(); });