diff --git a/.github/scripts/lint-skill-entry.mjs b/.github/scripts/lint-skill-entry.mjs new file mode 100644 index 0000000..4878991 --- /dev/null +++ b/.github/scripts/lint-skill-entry.mjs @@ -0,0 +1,152 @@ +#!/usr/bin/env node +// +// Structural validator for skill contributions. +// +// Reuses the installer's own parser (bin/metamask-skills.mjs collectSkills / +// parseFrontmatter) so that it validates exactly what ships, rather than a +// parallel model. Errors block; warnings advise. Exits non-zero on any error. +// +// Run against the repo: node .github/scripts/lint-skill-entry.mjs +// Run against another tree: SKILLS_LINT_ROOT=/path node .github/scripts/lint-skill-entry.mjs + +import { readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { collectSkills, parseFrontmatter } from '../../bin/metamask-skills.mjs'; +import { + ALLOWED_SIBLING_DIRS, + DESCRIPTION_MAX, + INSTALLED_PREFIX, + KNOWN_FRONTMATTER, + KNOWN_REPOS, + MATURITY_VALUES, + NAME_PATTERN, + RECOMMENDED_SECTIONS, +} from '../../tools/skill-schema.mjs'; + +const ROOT = process.env.SKILLS_LINT_ROOT + ? path.resolve(process.env.SKILLS_LINT_ROOT) + : path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); + +const allowedSiblings = new Set(ALLOWED_SIBLING_DIRS); +const TRUTHY = new Set(['1', 'true', 'yes', 'on']); + +export function lintSkill(skill) { + const errors = []; + const warnings = []; + const dirName = skill.id.slice(skill.domain.length + 1); + + let raw; + try { + raw = parseFrontmatter(readFileSync(path.join(skill.path, 'skill.md'), 'utf8')); + } catch (error) { + return { errors: [`could not read skill.md: ${error.message}`], warnings }; + } + + if (!raw.name) { + errors.push('missing required `name` in frontmatter'); + } else { + if (raw.name !== dirName) { + errors.push(`\`name\` "${raw.name}" must match the directory "${dirName}"`); + } + if (!NAME_PATTERN.test(raw.name)) { + errors.push(`\`name\` "${raw.name}" must be kebab-case`); + } + if (raw.name.startsWith(INSTALLED_PREFIX)) { + errors.push(`source \`name\` must not carry the \`${INSTALLED_PREFIX}\` prefix; the installer adds it`); + } + } + + if (!raw.description) { + errors.push('missing required `description` in frontmatter'); + } else if (raw.description.length > DESCRIPTION_MAX) { + errors.push(`\`description\` is ${raw.description.length} chars, over the ${DESCRIPTION_MAX}-char operator ceiling`); + } + + if (raw.maturity && !MATURITY_VALUES.includes(raw.maturity)) { + errors.push(`\`maturity\` "${raw.maturity}" must be one of: ${MATURITY_VALUES.join(', ')}`); + } + + // On-demand-only contract: a source skill must not force persistent loading. + if (raw.alwaysApply !== undefined && TRUTHY.has(String(raw.alwaysApply).toLowerCase())) { + errors.push('skills are on-demand only; remove `alwaysApply: true` (always-on guidance belongs in AGENTS.md)'); + } + + // Sibling directories: bundle dirs and repos/ only. knowledge/ is rejected. + let entries = []; + try { + entries = readdirSync(skill.path, { withFileTypes: true }); + } catch { + // skill dir vanished mid-run; nothing to check + } + for (const entry of entries) { + if (entry.isDirectory() && !allowedSiblings.has(entry.name)) { + errors.push(`unexpected directory "${entry.name}/" beside skill.md (allowed: ${[...allowedSiblings].join(', ')}); domain knowledge belongs in references/`); + } + } + + for (const repo of skill.repos) { + if (!KNOWN_REPOS.includes(repo)) { + warnings.push(`repos/${repo}.md targets an unknown consumer (known: ${KNOWN_REPOS.join(', ')})`); + } + } + + for (const key of Object.keys(raw)) { + if (!KNOWN_FRONTMATTER.includes(key) && key !== 'alwaysApply') { + warnings.push(`unknown frontmatter key "${key}"; operators silently ignore unrecognised keys (typo?)`); + } + } + + for (const section of RECOMMENDED_SECTIONS) { + if (!new RegExp(`^#{1,4}\\s+${section}\\b`, 'imu').test(skill.body)) { + warnings.push(`missing recommended section "## ${section}"`); + } + } + + return { errors, warnings }; +} + +// Restrict to skills touched by the given file paths (the CI gate passes the +// PR's changed files, so pre-existing drift in untouched skills never blocks an +// unrelated change). With no paths, every skill is linted (a full audit). +function skillsForPaths(skills, paths) { + const resolved = paths.map((file) => path.resolve(ROOT, file)); + return skills.filter((skill) => + resolved.some((file) => file === skill.path || file.startsWith(`${skill.path}${path.sep}`)), + ); +} + +function main() { + const paths = process.argv.slice(2).filter((arg) => !arg.startsWith('-')); + const all = collectSkills([ROOT]); + const skills = paths.length > 0 ? skillsForPaths(all, paths) : all; + let errorCount = 0; + let warningCount = 0; + + for (const skill of skills) { + const { errors, warnings } = lintSkill(skill); + if (errors.length > 0 || warnings.length > 0) { + console.log(`\n${skill.id}`); + for (const message of errors) { + console.log(` error: ${message}`); + } + for (const message of warnings) { + console.log(` warning: ${message}`); + } + } + errorCount += errors.length; + warningCount += warnings.length; + } + + console.log(`\n${skills.length} skill(s) checked, ${errorCount} error(s), ${warningCount} warning(s).`); + // Set exitCode rather than process.exit() so buffered stdout flushes when it + // is a pipe (e.g. under CI or execFileSync), instead of being truncated. + process.exitCode = errorCount > 0 ? 1 : 0; +} + +const invokedDirectly = + process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url); +if (invokedDirectly) { + main(); +} diff --git a/.github/workflows/lint-skill-entry.yml b/.github/workflows/lint-skill-entry.yml new file mode 100644 index 0000000..0ca4ec3 --- /dev/null +++ b/.github/workflows/lint-skill-entry.yml @@ -0,0 +1,33 @@ +name: Lint skill entries + +on: + pull_request: + paths: + - 'domains/**' + - 'tools/**' + - '.github/scripts/lint-skill-entry.mjs' + - '.github/workflows/lint-skill-entry.yml' + +permissions: + contents: read + +jobs: + lint-skills: + name: Lint skill entries + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 24.x + + - name: Get changed skill files + id: changed + uses: tj-actions/changed-files@v45 + with: + files: domains/** + + - name: Lint changed skills + if: steps.changed.outputs.any_changed == 'true' + run: node .github/scripts/lint-skill-entry.mjs ${{ steps.changed.outputs.all_changed_files }} diff --git a/package.json b/package.json index 2d7a510..9aeafc3 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,8 @@ "test": "node --test test/*.test.mjs", "pack:dry-run": "yarn pack --dry-run", "lint": "yarn lint:changelog", - "lint:changelog": "auto-changelog validate --formatter oxfmt" + "lint:changelog": "auto-changelog validate --formatter oxfmt", + "lint:skills": "node .github/scripts/lint-skill-entry.mjs" }, "publishConfig": { "access": "public", diff --git a/test/lint-skill-entry.test.mjs b/test/lint-skill-entry.test.mjs new file mode 100644 index 0000000..4aa72fd --- /dev/null +++ b/test/lint-skill-entry.test.mjs @@ -0,0 +1,110 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterEach, describe, test } from 'node:test'; + +const LINTER = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..', + '.github', + 'scripts', + 'lint-skill-entry.mjs', +); + +const roots = []; + +function makeRoot() { + const root = mkdtempSync(path.join(os.tmpdir(), 'skill-lint-')); + roots.push(root); + return root; +} + +afterEach(() => { + while (roots.length > 0) { + rmSync(roots.pop(), { recursive: true, force: true }); + } +}); + +function writeSkill(root, domain, name, frontmatter, body) { + const dir = path.join(root, 'domains', domain, 'skills', name); + mkdirSync(dir, { recursive: true }); + const defaultBody = '## When To Use\n\n- always\n\n## Workflow\n\n1. do the thing\n'; + writeFileSync(path.join(dir, 'skill.md'), `---\n${frontmatter}\n---\n\n${body ?? defaultBody}`); + return dir; +} + +function lint(root) { + const result = spawnSync(process.execPath, [LINTER], { + env: { ...process.env, SKILLS_LINT_ROOT: root }, + encoding: 'utf8', + }); + return { code: result.status, output: `${result.stdout}${result.stderr}` }; +} + +describe('lint-skill-entry', () => { + test('a well-formed skill passes', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', 'name: unit-testing\ndescription: Write unit tests'); + assert.equal(lint(root).code, 0); + }); + + test('missing name fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', 'description: x'); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /missing required `name`/u); + }); + + test('name not matching the directory fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', 'name: wrong-name\ndescription: x'); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /must match the directory/u); + }); + + test('a knowledge/ sibling directory fails (the conversion guarantee)', () => { + const root = makeRoot(); + const dir = writeSkill(root, 'perps', 'fix-bug', 'name: fix-bug\ndescription: x'); + mkdirSync(path.join(dir, 'knowledge')); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /knowledge/u); + }); + + test('an mms- prefix in the source name fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'mms-unit', 'name: mms-unit\ndescription: x'); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /prefix/u); + }); + + test('a description over the operator ceiling fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', `name: unit-testing\ndescription: ${'x'.repeat(1100)}`); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /operator ceiling/u); + }); + + test('an invalid maturity value fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', 'name: unit-testing\ndescription: x\nmaturity: beta'); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /maturity/u); + }); + + test('alwaysApply: true fails (on-demand-only contract)', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'unit-testing', 'name: unit-testing\ndescription: x\nalwaysApply: true'); + const { code, output } = lint(root); + assert.equal(code, 1); + assert.match(output, /on-demand/u); + }); +}); diff --git a/tools/skill-schema.mjs b/tools/skill-schema.mjs new file mode 100644 index 0000000..800d0c5 --- /dev/null +++ b/tools/skill-schema.mjs @@ -0,0 +1,36 @@ +// Single source of truth for the skill contribution schema. +// +// Imported by the lint-skill-entry validator so that the documented schema and +// the enforced schema cannot drift apart. The installer's bundle-directory list +// in tools/install (Bash) mirrors BUNDLE_DIRS; keep the two in sync. + +export const REQUIRED_FRONTMATTER = ['name', 'description']; +export const OPTIONAL_FRONTMATTER = ['maturity', 'mandatory', 'scope']; +export const KNOWN_FRONTMATTER = [...REQUIRED_FRONTMATTER, ...OPTIONAL_FRONTMATTER]; + +export const MATURITY_VALUES = ['experimental', 'stable', 'deprecated']; + +// Directories the installer copies alongside skill.md (see tools/install). +export const BUNDLE_DIRS = ['references', 'scripts', 'assets', 'adapters']; + +// Directories allowed beside skill.md: the bundle dirs plus the repo-overlay +// dir. Anything else (notably knowledge/) is rejected, because the installer +// does not ship it and the reference would dangle post-install. +export const ALLOWED_SIBLING_DIRS = [...BUNDLE_DIRS, 'repos']; + +export const KNOWN_REPOS = ['metamask-extension', 'metamask-mobile', 'core']; + +// The description is always loaded into the operator's discovery surface, so it +// is the per-skill always-on cost. The ceiling is the per-operator minimum +// (OpenCode caps description at 1024), so a description that passes here is +// accepted by every target. +export const DESCRIPTION_MAX = 1024; + +export const RECOMMENDED_SECTIONS = ['When To Use', 'Workflow']; + +// kebab-case, matching the name regex Claude Code and OpenCode require. +export const NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/u; + +// The installer prepends this to generated output names; source names must not +// carry it. +export const INSTALLED_PREFIX = 'mms-';