From 258a1dd3b7d867264655e5a69e6364921e3d0c79 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:49:58 +0800 Subject: [PATCH] fix(skills): parse metadata only from YAML frontmatter --- scripts/generate-package-metadata.mjs | 12 +- scripts/generate-skill-metadata.mjs | 25 ++- scripts/verify-agent-experience.mjs | 11 +- scripts/verify-package-pack.mjs | 15 +- src/application/SetupSkill.ts | 17 +- src/domain/skillManifest.test.ts | 120 +++++++++++++ src/domain/skillManifest.ts | 165 ++++++++++++++++++ .../filesystem/dependencyInstall.test.ts | 3 + .../filesystem/generatedSkill.test.ts | 46 ++++- tests/boundary/filesystem/setupSkill.test.ts | 23 +++ turbo.json | 4 + 11 files changed, 410 insertions(+), 31 deletions(-) create mode 100644 src/domain/skillManifest.test.ts create mode 100644 src/domain/skillManifest.ts diff --git a/scripts/generate-package-metadata.mjs b/scripts/generate-package-metadata.mjs index a516d7716f..fc21776dae 100644 --- a/scripts/generate-package-metadata.mjs +++ b/scripts/generate-package-metadata.mjs @@ -3,6 +3,12 @@ import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { ensureGeneratedFile } from "./lib/generated-file.mjs"; +import { importVerifierSource } from "./lib/verifier-source-loader.mjs"; + +const { parseSkillManifestMetadata } = await importVerifierSource( + new URL("../src/domain/skillManifest.ts", import.meta.url).href, + import.meta.url, +); const arguments_ = new Set(process.argv.slice(2)); for (const argument of arguments_) @@ -25,8 +31,10 @@ const skill = await readFile( join(root, ".agents/skills/reverse-engineer-anything/SKILL.md"), "utf8", ); -const skillVersion = /^\s{2}version:\s*"([^"]+)"\s*$/mu.exec(skill)?.[1]; -if (skillVersion === undefined) +const skillMetadata = parseSkillManifestMetadata(skill); +if (!skillMetadata.ok) throw skillMetadata.error; +const skillVersion = skillMetadata.value.version; +if (skillVersion === null) throw new Error("Missing reverse-engineer-anything skill metadata version"); const versionOf = (name) => { const value = lock.packages?.[`node_modules/${name}`]?.version; diff --git a/scripts/generate-skill-metadata.mjs b/scripts/generate-skill-metadata.mjs index f705501ad5..6c5a348900 100644 --- a/scripts/generate-skill-metadata.mjs +++ b/scripts/generate-skill-metadata.mjs @@ -3,6 +3,12 @@ import { dirname, join } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { ensureGeneratedFile } from "./lib/generated-file.mjs"; +import { importVerifierSource } from "./lib/verifier-source-loader.mjs"; + +const { addSkillManifestToolCount } = await importVerifierSource( + new URL("../src/domain/skillManifest.ts", import.meta.url).href, + import.meta.url, +); const arguments_ = new Set(process.argv.slice(2)); for (const argument of arguments_) @@ -23,22 +29,15 @@ const { FULL_CATALOG_IDENTITY } = await import( const { PRODUCT_IDENTITY } = await import( `${pathToFileURL(join(root, "dist/identity.js")).href}?${cacheBust}` ); -if (/^\s{2}(?:tool_count|catalog_digest):/mu.test(current)) - throw new Error( - "Catalog metadata belongs in the generated skill, not the authored source", - ); -const versionLine = /^ {2}version: "[^"\r\n]+"$/mu; -if (!versionLine.test(current)) - throw new Error("Missing authored skill version"); // A shipped skill must run the version it ships with, not whatever `@latest` resolves to. const latestSpecifier = PRODUCT_IDENTITY.packageSpecifier; const pinnedSpecifier = PRODUCT_IDENTITY.registrationPackageSpecifier; -const source = current - .replace( - versionLine, - `$&\n tool_count: ${String(FULL_CATALOG_IDENTITY.counts.mcp_tools)}`, - ) - .replaceAll(latestSpecifier, pinnedSpecifier); +const generated = addSkillManifestToolCount( + current, + FULL_CATALOG_IDENTITY.counts.mcp_tools, +); +if (!generated.ok) throw generated.error; +const source = generated.value.replaceAll(latestSpecifier, pinnedSpecifier); const paths = await filePaths(sourceRoot); // Rebuild this owned output directory so removed references cannot survive a build. if (!check) await rm(outputRoot, { recursive: true, force: true }); diff --git a/scripts/verify-agent-experience.mjs b/scripts/verify-agent-experience.mjs index a9923e4e61..7626a4b6a5 100644 --- a/scripts/verify-agent-experience.mjs +++ b/scripts/verify-agent-experience.mjs @@ -1,3 +1,5 @@ +import { importVerifierSource } from "./lib/verifier-source-loader.mjs"; + import { createHash } from "node:crypto"; import { chmod, @@ -22,6 +24,11 @@ import { } from "./lib/verifier-cleanup.mjs"; import { completeVerifierRun, createVerifierRun } from "./lib/verifier-run.mjs"; +const { parseSkillManifestMetadata } = await importVerifierSource( + new URL("../src/domain/skillManifest.ts", import.meta.url).href, + import.meta.url, +); + const { evaluateCodexEvents } = await tsImport( new URL("../tests/evaluation/model-evals/CodexAgentEval.ts", import.meta.url) .href, @@ -148,8 +155,10 @@ await withVerifierCleanup( join(skillDestination, "SKILL.md"), "utf8", ); + const skillMetadata = parseSkillManifestMetadata(installedSkill); if ( - !installedSkill.includes(` version: "${PRODUCT_IDENTITY.skillVersion}"`) + !skillMetadata.ok || + skillMetadata.value.version !== PRODUCT_IDENTITY.skillVersion ) throw new Error("Setup did not install the current packaged REA skill"); const requestedScenarioIds = new Set( diff --git a/scripts/verify-package-pack.mjs b/scripts/verify-package-pack.mjs index 1c75d4ac1b..c8debd0155 100644 --- a/scripts/verify-package-pack.mjs +++ b/scripts/verify-package-pack.mjs @@ -1,3 +1,5 @@ +import { importVerifierSource } from "./lib/verifier-source-loader.mjs"; + import { mkdir } from "node:fs/promises"; import { join } from "node:path"; import { FULL_CATALOG_IDENTITY } from "../dist/catalogIdentity.js"; @@ -5,6 +7,11 @@ import { FULL_CATALOG_IDENTITY } from "../dist/catalogIdentity.js"; import { parseTarListing } from "./lib/verify-package-listing.mjs"; import { verifyPackedBridge } from "./verify-packed-bridge.mjs"; +const { parseSkillManifestMetadata } = await importVerifierSource( + new URL("../src/domain/skillManifest.ts", import.meta.url).href, + import.meta.url, +); + /** Create a tarball and assert production packaging constraints. */ export async function verifyPackagePack({ root, workspace, commands }) { const packDirectory = join(workspace, "pack"); @@ -55,11 +62,11 @@ export async function verifyPackagePack({ root, workspace, commands }) { "package/skills/reverse-engineer-anything/SKILL.md", ]) ).stdout; + const skillMetadata = parseSkillManifestMetadata(skill); if ( - !skill.includes( - ` tool_count: ${String(FULL_CATALOG_IDENTITY.counts.mcp_tools)}\n`, - ) || - skill.includes(" catalog_digest:") + !skillMetadata.ok || + skillMetadata.value.toolCount !== FULL_CATALOG_IDENTITY.counts.mcp_tools || + skillMetadata.value.hasCatalogDigest ) throw new Error( "packaged skill metadata did not match the catalog shipped in the package", diff --git a/src/application/SetupSkill.ts b/src/application/SetupSkill.ts index 2d94e6c83b..9ffc2bfaec 100644 --- a/src/application/SetupSkill.ts +++ b/src/application/SetupSkill.ts @@ -2,6 +2,7 @@ import { copyFile, lstat, mkdir, readFile } from "node:fs/promises"; import { constants as fsConstants } from "node:fs"; import { dirname, join } from "node:path"; import { err, ok, type Result } from "../domain/result.js"; +import { parseSkillManifestMetadata } from "../domain/skillManifest.js"; import { readBoundedFileBytes } from "../process/BoundedFileBytes.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; import { ArtifactReaderFailure } from "../artifacts/ArtifactReader.js"; @@ -698,17 +699,15 @@ export const readInstalledSkillIdentity = async ( ? inspection.value.manifest : inspection.error.manifest; const issues = inspection.ok ? [] : inspection.error.issues; - const toolCount = - content === undefined - ? undefined - : /^\s{2}tool_count:\s*(\d+)\s*$/mu.exec(content)?.[1]; + const parsedMetadata = + content === undefined ? undefined : parseSkillManifestMetadata(content); + const metadata = parsedMetadata?.ok + ? parsedMetadata.value + : parsedMetadata?.error.metadata; return { canonical: inspection.ok && inspection.value.canonical, - version: - content === undefined - ? null - : (/^\s{2}version:\s*"([^"]+)"\s*$/mu.exec(content)?.[1] ?? null), - toolCount: toolCount === undefined ? null : Number.parseInt(toolCount, 10), + version: metadata?.version ?? null, + toolCount: metadata?.toolCount ?? null, ...(issues.length === 0 ? {} : { issues }), }; }; diff --git a/src/domain/skillManifest.test.ts b/src/domain/skillManifest.test.ts new file mode 100644 index 0000000000..5b475ae80d --- /dev/null +++ b/src/domain/skillManifest.test.ts @@ -0,0 +1,120 @@ +import { expect, it } from "vitest"; +import { + addSkillManifestToolCount, + parseSkillManifestMetadata, +} from "./skillManifest.js"; + +const manifest = (metadata: string, body = "\n# Instructions\n") => + `---\nname: reverse-engineer-anything\nmetadata:\n${metadata}---\n${body}`; + +it.each(["'49'", '"49"'])( + "reads semantic metadata with version %s", + (version) => { + expect( + parseSkillManifestMetadata( + manifest(` version: ${version}\n tool_count: 0\n`), + ), + ).toEqual({ + ok: true, + value: { + version: "49", + toolCount: 0, + hasToolCount: true, + hasCatalogDigest: false, + }, + }); + }, +); + +it("does not read counterfeit metadata from the Markdown body", () => { + const content = + '---\nname: reverse-engineer-anything\n---\n\n```yaml\nmetadata:\n version: "counterfeit"\n tool_count: 42\n```\n'; + expect(parseSkillManifestMetadata(content)).toEqual({ + ok: true, + value: { + version: null, + toolCount: null, + hasToolCount: false, + hasCatalogDigest: false, + }, + }); + expect(addSkillManifestToolCount(content, 5)).toMatchObject({ + ok: false, + error: { message: "Missing authored skill version" }, + }); +}); + +it.each(["-1", "1.5", "9007199254740992", '"7"', "null"])( + "retains a valid version when tool_count is invalid: %s", + (count) => { + expect( + parseSkillManifestMetadata( + manifest(` version: '49'\n tool_count: ${count}\n`), + ), + ).toMatchObject({ + ok: false, + error: { + _tag: "SkillManifestMetadataError", + metadata: { version: "49", toolCount: null }, + }, + }); + }, +); + +it.each([ + " version: [\n", + " version: '49'\n version: '50'\n", + " version: '49'\n tool_count: 1\n tool_count: 2\n", +])("rejects invalid or duplicate YAML metadata", (metadata) => { + expect(parseSkillManifestMetadata(manifest(metadata))).toMatchObject({ + ok: false, + }); +}); + +it("retains a valid count when the version is invalid", () => { + expect( + parseSkillManifestMetadata(manifest(" version: 49\n tool_count: 7\n")), + ).toMatchObject({ + ok: false, + error: { metadata: { version: null, toolCount: 7 } }, + }); +}); + +it("updates bounded frontmatter without changing Markdown examples or comments", () => { + const body = + '\n# Instructions\n\n```yaml\nmetadata:\n version: "body-only"\n tool_count: 99\n```\n'; + const content = manifest(" version: '49' # keep version comment\n", body); + const generated = addSkillManifestToolCount(content, 7); + expect(generated.ok).toBe(true); + if (!generated.ok) throw generated.error; + expect(generated.value.endsWith(body)).toBe(true); + expect(generated.value).toContain("version: '49' # keep version comment"); + expect(parseSkillManifestMetadata(generated.value)).toMatchObject({ + ok: true, + value: { version: "49", toolCount: 7 }, + }); +}); + +it.each(["tool_count: 0", "catalog_digest: abc"])( + "rejects authored generated metadata: %s", + (field) => { + expect( + addSkillManifestToolCount(manifest(` version: '49'\n ${field}\n`), 7), + ).toMatchObject({ + ok: false, + error: { message: expect.stringContaining("Catalog metadata belongs") }, + }); + }, +); + +it("rejects unsupported YAML tags without treating their scalars as metadata", () => { + expect( + parseSkillManifestMetadata(manifest(" version: !custom '49'\n")), + ).toMatchObject({ + ok: false, + error: { + message: expect.stringContaining("Unsupported skill YAML frontmatter"), + metadata: { version: null }, + }, + }); +}); diff --git a/src/domain/skillManifest.ts b/src/domain/skillManifest.ts new file mode 100644 index 0000000000..5682811051 --- /dev/null +++ b/src/domain/skillManifest.ts @@ -0,0 +1,165 @@ +import { isMap, parseDocument, type Document } from "yaml"; +import { err, ok, type Result } from "./result.js"; + +export interface SkillManifestMetadata { + readonly version: string | null; + readonly toolCount: number | null; + readonly hasToolCount: boolean; + readonly hasCatalogDigest: boolean; +} + +export class SkillManifestMetadataError extends Error { + readonly _tag = "SkillManifestMetadataError" as const; + constructor( + message: string, + readonly metadata: SkillManifestMetadata, + ) { + super(message); + } +} + +const emptyMetadata = (): SkillManifestMetadata => ({ + version: null, + toolCount: null, + hasToolCount: false, + hasCatalogDigest: false, +}); + +/** The frontmatter range owns YAML; body examples never supply metadata. */ +const readFrontmatter = ( + content: string, +): Result< + { + document: Document; + body: string; + }, + SkillManifestMetadataError +> => { + const opening = /^---\r?\n/u.exec(content); + if (opening === null) + return err( + new SkillManifestMetadataError( + "Skill manifest has no YAML frontmatter", + emptyMetadata(), + ), + ); + const remainder = content.slice(opening[0].length); + const closing = /^---(?:\r?\n|$)/mu.exec(remainder); + if (closing === null) + return err( + new SkillManifestMetadataError( + "Skill manifest YAML frontmatter is unterminated", + emptyMetadata(), + ), + ); + const document = parseDocument(remainder.slice(0, closing.index), { + logLevel: "silent", + }); + if (document.errors.length > 0) + return err( + new SkillManifestMetadataError( + `Invalid skill YAML frontmatter: ${document.errors.map((error) => error.message).join("; ")}`, + emptyMetadata(), + ), + ); + if (document.warnings.length > 0) + return err( + new SkillManifestMetadataError( + `Unsupported skill YAML frontmatter: ${document.warnings.map((warning) => warning.message).join("; ")}`, + emptyMetadata(), + ), + ); + if (!isMap(document.contents)) + return err( + new SkillManifestMetadataError( + "Skill YAML frontmatter must be a mapping", + emptyMetadata(), + ), + ); + return ok({ + document, + body: remainder.slice(closing.index + closing[0].length), + }); +}; + +const metadataOf = ( + document: Document, +): Result => { + const metadata = document.get("metadata", true); + if (metadata === undefined) return ok(emptyMetadata()); + if (!isMap(metadata)) + return err( + new SkillManifestMetadataError( + "Skill metadata must be a mapping", + emptyMetadata(), + ), + ); + const version = document.getIn(["metadata", "version"]); + const toolCount = document.getIn(["metadata", "tool_count"]); + const facts: SkillManifestMetadata = { + version: typeof version === "string" && version.length > 0 ? version : null, + toolCount: + typeof toolCount === "number" && + Number.isSafeInteger(toolCount) && + toolCount >= 0 + ? toolCount + : null, + hasToolCount: metadata.has("tool_count"), + hasCatalogDigest: metadata.has("catalog_digest"), + }; + const issues = []; + if (metadata.has("version") && facts.version === null) + issues.push("version must be a nonempty string"); + if (facts.hasToolCount && facts.toolCount === null) + issues.push("tool_count must be a safe nonnegative integer"); + return issues.length === 0 + ? ok(facts) + : err( + new SkillManifestMetadataError( + `Invalid skill metadata: ${issues.join("; ")}`, + facts, + ), + ); +}; + +export const parseSkillManifestMetadata = ( + content: string, +): Result => { + const frontmatter = readFrontmatter(content); + return frontmatter.ok ? metadataOf(frontmatter.value.document) : frontmatter; +}; + +/** Update only authored YAML frontmatter, retaining the instruction body. */ +export const addSkillManifestToolCount = ( + content: string, + toolCount: number, +): Result => { + const frontmatter = readFrontmatter(content); + if (!frontmatter.ok) return frontmatter; + const metadata = metadataOf(frontmatter.value.document); + if (!metadata.ok) return metadata; + if (metadata.value.version === null) + return err( + new SkillManifestMetadataError( + "Missing authored skill version", + metadata.value, + ), + ); + if (metadata.value.hasToolCount || metadata.value.hasCatalogDigest) + return err( + new SkillManifestMetadataError( + "Catalog metadata belongs in the generated skill, not the authored source", + metadata.value, + ), + ); + if (!Number.isSafeInteger(toolCount) || toolCount < 0) + return err( + new SkillManifestMetadataError( + "Generated tool_count must be a safe nonnegative integer", + metadata.value, + ), + ); + const { document, body } = frontmatter.value; + document.setIn(["metadata", "tool_count"], toolCount); + return ok(`---\n${document.toString({ lineWidth: 0 })}---\n${body}`); +}; diff --git a/tests/boundary/filesystem/dependencyInstall.test.ts b/tests/boundary/filesystem/dependencyInstall.test.ts index 06db663fc0..2dd9a542df 100644 --- a/tests/boundary/filesystem/dependencyInstall.test.ts +++ b/tests/boundary/filesystem/dependencyInstall.test.ts @@ -205,6 +205,9 @@ const createBuildCacheFixture = async (): Promise => { "scripts/clean-build-output.mjs", "scripts/generate-package-metadata.mjs", "scripts/lib/generated-file.mjs", + "scripts/lib/verifier-source-loader.mjs", + "src/domain/skillManifest.ts", + "src/domain/result.ts", ".agents/skills/reverse-engineer-anything/SKILL.md", "src/generatedPackageMetadata.ts", ]; diff --git a/tests/boundary/filesystem/generatedSkill.test.ts b/tests/boundary/filesystem/generatedSkill.test.ts index b08d98981a..1bfbc298b3 100644 --- a/tests/boundary/filesystem/generatedSkill.test.ts +++ b/tests/boundary/filesystem/generatedSkill.test.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { copyFile, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { copyFile, cp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; @@ -17,10 +17,19 @@ async function fixture(eol = "\n") { for (const path of [ "scripts/generate-skill-metadata.mjs", "scripts/lib/generated-file.mjs", + "scripts/lib/verifier-source-loader.mjs", + "src/domain/skillManifest.ts", + "src/domain/result.ts", ]) { await mkdir(dirname(join(root, path)), { recursive: true }); await copyFile(join(repositoryRoot, path), join(root, path)); } + await mkdir(join(root, "node_modules")); + await cp( + join(repositoryRoot, "node_modules/yaml"), + join(root, "node_modules/yaml"), + { recursive: true }, + ); const sourceRoot = join(root, ".agents/skills/reverse-engineer-anything"); const outputRoot = join(root, "skills/reverse-engineer-anything"); await mkdir(join(sourceRoot, "references"), { recursive: true }); @@ -81,7 +90,13 @@ it.each(["\n", "\r\n"])( expect( await readFile(join(f.sourceRoot, "references/guide.md"), "utf8"), ).toBe(`# Guide${eol}Preserve evidence.${eol}`); - await expect(f.generate("--check")).resolves.toMatchObject({ stderr: "" }); + const checked = await f.generate("--check"); + expect( + checked.stderr.replace( + /^\(node:\d+\) ExperimentalWarning: stripTypeScriptTypes is an experimental feature and might change at any time\r?\n\(Use `node --trace-warnings \.\.\.` to show where the warning was created\)\r?\n$/u, + "", + ), + ).toBe(""); }, ); @@ -143,3 +158,30 @@ it("rejects catalog-dependent metadata in the authored source", async () => { ), }); }); + +it("generates metadata from quoted frontmatter while preserving body metadata examples", async () => { + const f = await fixture(); + const body = + '\n# Instructions\n\n```yaml\nmetadata:\n version: "body-only"\n tool_count: 99\n catalog_digest: example\n```\n'; + await writeFile( + join(f.sourceRoot, "SKILL.md"), + "---\nname: reverse-engineer-anything\nmetadata:\n version: '33'\n---\n" + + body, + ); + await f.generate(); + const generated = await readFile(join(f.outputRoot, "SKILL.md"), "utf8"); + expect(generated.endsWith(body)).toBe(true); + expect(generated).toContain("version: '33'"); + expect(generated).toContain("tool_count: 139"); +}); + +it("rejects body-only versions instead of generating metadata inside examples", async () => { + const f = await fixture(); + await writeFile( + join(f.sourceRoot, "SKILL.md"), + '---\nname: reverse-engineer-anything\n---\n\n```yaml\nmetadata:\n version: "body-only"\n```\n', + ); + await expect(f.generate()).rejects.toMatchObject({ + stderr: expect.stringContaining("Missing authored skill version"), + }); +}); diff --git a/tests/boundary/filesystem/setupSkill.test.ts b/tests/boundary/filesystem/setupSkill.test.ts index 47e7953599..37832c546f 100644 --- a/tests/boundary/filesystem/setupSkill.test.ts +++ b/tests/boundary/filesystem/setupSkill.test.ts @@ -720,3 +720,26 @@ it("doctor verifies installed instructions and references even when metadata is ); } }); + +it("reads installed frontmatter metadata without treating body examples as identity", async () => { + const home = await createTestTempDirectory("rea-installed-metadata-"); + const installed = await installCanonicalSkill(home, ["claude_code"]); + expect(installed.ok).toBe(true); + const destination = skillDestinations(home, ["claude_code"])[0]; + if (destination === undefined) throw new Error("Missing skill destination"); + const manifest = join(destination.path, "SKILL.md"); + await writeFile( + manifest, + "---\nname: reverse-engineer-anything\nmetadata:\n version: '49'\n tool_count: -1\n---\n\n```yaml\nmetadata:\n version: \"counterfeit\"\n tool_count: 42\n```\n", + ); + expect(await readInstalledSkillIdentity(home, ["claude_code"])).toMatchObject( + { version: "49", toolCount: null, canonical: false }, + ); + await writeFile( + manifest, + '---\nname: reverse-engineer-anything\n---\n\n```yaml\nmetadata:\n version: "counterfeit"\n tool_count: 42\n```\n', + ); + expect(await readInstalledSkillIdentity(home, ["claude_code"])).toMatchObject( + { version: null, toolCount: null, canonical: false }, + ); +}); diff --git a/turbo.json b/turbo.json index c8530d501d..8b5ce3ae8e 100644 --- a/turbo.json +++ b/turbo.json @@ -64,6 +64,9 @@ "package-lock.json", "scripts/generate-package-metadata.mjs", "scripts/lib/generated-file.mjs", + "scripts/lib/verifier-source-loader.mjs", + "src/domain/skillManifest.ts", + "src/domain/result.ts", ".agents/skills/reverse-engineer-anything/SKILL.md", "src/generatedPackageMetadata.ts" ] @@ -213,6 +216,7 @@ "package-lock.json", "scripts/generate-skill-metadata.mjs", "scripts/lib/generated-file.mjs", + "scripts/lib/verifier-source-loader.mjs", ".agents/skills/reverse-engineer-anything/**" ], "outputs": ["skills/**"]