ci: add lint-skill-entry structural validator

.github/scripts/lint-skill-entry.mjs

@@ -0,0 +1,210 @@
+#!/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, statSync } 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,
+  SCOPE_VALUES,
+  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']);
+const FALSY = new Set(['0', 'false', 'no', 'off']);
+
+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 budget`);
+  }
+
+  if (raw.maturity && !MATURITY_VALUES.includes(raw.maturity)) {
+    errors.push(`\`maturity\` "${raw.maturity}" must be one of: ${MATURITY_VALUES.join(', ')}`);
+  }
+
+  // `scope` and `mandatory` change installer behaviour, and a typo in either is a silent
+  // no-op today: the key is accepted, no enum runs, and the skill installs in a way the
+  // author did not intend. `scope: users` falls back to project scope; `mandatory: ture`
+  // is falsy. Warnings rather than errors — the blocking surface stays small.
+  if (raw.scope !== undefined && !SCOPE_VALUES.includes(raw.scope)) {
+    warnings.push(`\`scope\` "${raw.scope}" is not one of: ${SCOPE_VALUES.join(', ')} (installs as project scope)`);
+  }
+  if (raw.mandatory !== undefined && !TRUTHY.has(String(raw.mandatory).toLowerCase()) && !FALSY.has(String(raw.mandatory).toLowerCase())) {
+    warnings.push(`\`mandatory\` "${raw.mandatory}" is neither truthy nor falsy (treated as false)`);
+  }
+
+  // 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) {
+    // A trailing `\b` let `## When To Use Cases` satisfy `When To Use` — a different
+    // section. Anchoring to end-of-line fixes that but rejects `## Workflows` and
+    // `## Workflow (interactive)`, both of which are the section, and both of which exist
+    // in this corpus. So: optional plural, optional parenthetical qualifier, nothing else.
+    if (!new RegExp(`^#{1,4}\\s+${section}s?(?:\\s*\\([^)]*\\))?\\s*$`, '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}`)),
+  );
+}
+
+// A changed path under domains/ must live at domains/<domain>/skills/<name>/… and that
+// skill root must have a readable skill.md.
+//
+// This has to run BEFORE the collectSkills filter, not inside the per-skill loop.
+// collectSkills only returns directories that already match the expected layout and parse,
+// so anything malformed is invisible to it — a misplaced file, a SKILL.md casing error, or
+// a skill.md deleted in the same PR all produced "0 skill(s) checked, 0 error(s)" and a
+// green run. The shape has to be checked from the path side, where the malformed cases
+// actually exist.
+export const SKILL_PATH = /^domains\/([^/]+)\/skills\/([^/]+)\/(?:[^/]+\/)*[^/]+$/u;
+
+export function validatePathShape(file, root = ROOT) {
+  const normalized = file.split(path.sep).join('/');
+  if (!normalized.startsWith('domains/')) {
+    return null;
+  }
+  const match = SKILL_PATH.exec(normalized);
+  if (!match) {
+    return `path "${normalized}" is not under domains/<domain>/skills/<name>/`;
+  }
+  const [, domain, name] = match;
+  const skillRoot = path.join(root, 'domains', domain, 'skills', name);
+  try {
+    statSync(path.join(skillRoot, 'skill.md'));
+  } catch {
+    return `skill root "domains/${domain}/skills/${name}/" has no readable skill.md`;
+  }
+  return null;
+}
+
+function main() {
+  const paths = process.argv.slice(2).filter((arg) => !arg.startsWith('-'));
+  let errorCount = 0;
+  let warningCount = 0;
+
+  for (const file of paths) {
+    const problem = validatePathShape(file);
+    if (problem) {
+      console.log(`\nerror:   ${problem}`);
+      errorCount += 1;
+    }
+  }
+
+  // '*' rather than undefined: the linter never reads repoApplicable, but relying on that
+  // would break silently if `repo` became required.
+  const all = collectSkills([ROOT], '*');
+  const skills = paths.length > 0 ? skillsForPaths(all, paths) : all;
+
+  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();
+}

.github/workflows/lint-skill-entry.yml

@@ -0,0 +1,66 @@
+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:
+      # The repo's own composite action, as every other workflow here uses. Removes the
+      # third-party dependency, and with it the unpinned-reference and credential-
+      # persistence findings that raw actions/checkout raised.
+      # Pinned to a hash per the blanket policy code scanning enforces. The other
+      # workflows here still float on @v3; they predate the policy check.
+      - uses: MetaMask/action-checkout-and-setup@0543b5929698c71e3ccc6ed24eac87825669b5de # v3.5.0
+        with:
+          is-high-risk-environment: false
+          node-version: 24.x
+          # Full history: the changed-file diff needs the base commit.
+          fetch-depth: 0
+
+      # Filenames come from PR contents, so they are untrusted input. They are read
+      # NUL-delimited, passed through the environment, and split on NUL into an array —
+      # never interpolated into a command line. A path containing a space, a quote, or a
+      # shell metacharacter is therefore just a filename.
+      - name: Collect changed skill files
+        id: changed
+        env:
+          BASE_SHA: ${{ github.event.pull_request.base.sha }}
+          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
+        run: |
+          git diff -z --name-only --diff-filter=d "$BASE_SHA" "$HEAD_SHA" -- 'domains/**' \
+            > changed-skill-files.bin
+          if [[ -s changed-skill-files.bin ]]; then
+            echo "any_changed=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "any_changed=false" >> "$GITHUB_OUTPUT"
+          fi
+
+          # A schema or installer change re-validates the whole catalogue. Without this the
+          # workflow triggers on tools/**, finds no changed skill files, skips the lint step,
+          # and reports green — so a tightened rule is never applied to what already exists.
+          if git diff --name-only "$BASE_SHA" "$HEAD_SHA" -- tools/ .github/scripts/ | grep -q .; then
+            echo "full_audit=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "full_audit=false" >> "$GITHUB_OUTPUT"
+          fi
+
+      - name: Audit every skill (schema or installer changed)
+        if: steps.changed.outputs.full_audit == 'true'
+        run: node .github/scripts/lint-skill-entry.mjs
+
+      - name: Lint changed skills
+        if: steps.changed.outputs.full_audit != 'true' && steps.changed.outputs.any_changed == 'true'
+        run: |
+          mapfile -d '' -t files < changed-skill-files.bin
+          node .github/scripts/lint-skill-entry.mjs "${files[@]}"

CONTRIBUTING.md

@@ -18,6 +18,47 @@ A well-crafted skill should:
 - **Declare maturity** — Use `experimental`, `stable`, or `deprecated` in frontmatter
 - **Be well-documented** — Clear instructions, examples, and expected outcomes
 
+### Is it a skill, or does it belong in an enforcement layer?
+
+Skills shape generation. Linters, fitness functions, and hooks enforce. The two do
+different work, and any pattern load-bearing enough to encode probably wants both.
+
+| Layer | Mechanism | Can an agent bypass it? | Timing |
+|-------|-----------|-------------------------|--------|
+| Skills (`domains/<area>/skills/<name>/skill.md`) | Generation-time guidance | Yes — suggestive only | Before output |
+| AI rules (`AGENTS.md`, `.cursor/rules/*.mdc`, `CLAUDE.md`) | Context injection | Yes — suggestive only | Before output |
+| Hooks (Claude Code `PreToolUse`, Cursor hooks) | Runtime interception | No | At tool call |
+| Linters, fitness functions, CI | Validation | No | After output |
+
+Three ways a proposed skill fails this test:
+
+- **A skill that substitutes for enforcement is unsafe.** An agent can ignore any context
+  it is given, so anything that must not be bypassed belongs in a hook or a lint rule.
+- **A skill that restates what a deterministic check already verifies is wasteful.** It
+  spends context on every invocation to duplicate ground truth that CI produces for free.
+- **A skill that teaches the upstream pattern, so the enforcement layer rarely has to fire,
+  is the right shape.** An existing lint rule is evidence the pattern matters enough to
+  encode at both layers — name the layer the skill pairs with.
+
+The question to answer in review is not *"is this redundant with the linter?"* but *"is this
+doing generation-time work the linter cannot?"*
+
+### Does it earn its context budget?
+
+Frontmatter for every installed skill is loaded at agent startup, as fixed overhead that
+grows linearly with the catalogue. A skill that is never selected still costs its
+`description` on every run.
+
+- Not a duplicate of a skill that already exists — check `metamask-skills list` first.
+- Actionable rather than aspirational: steps an agent can follow, not principles to admire.
+- Scoped so a reader can tell when it applies — neither one repo's quirk nor "good code".
+- `description` within the ceiling in [`tools/skill-schema.mjs`](tools/skill-schema.mjs). It is
+  the lowest limit across operators, so a description that passes is accepted by all of them.
+
+`yarn audit:skills` checks the deterministic properties — directory layout, name pattern,
+frontmatter keys, maturity values, description length — so review time goes to the two
+questions above, which no check can answer.
+
 ## How to Contribute
 
 ### Adding a New Skill

README.md

@@ -391,6 +391,11 @@ Extra metadata blocks (e.g. OpenClaw-style `metadata:` with emoji and
 homepage) are preserved through install — only `name`, `description`,
 `maturity`, `mandatory`, and `scope` are read by the CLI.
 
+The 1,536-character ceiling is a repo budget rather than an operator limit — the
+description is always-on context for every installed skill, so it is capped
+deliberately. It is enforced by `yarn audit:skills` from
+[`tools/skill-schema.mjs`](tools/skill-schema.mjs), which is the source of truth.
+
 ### Overlay frontmatter
 
 ```yaml

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",
+    "audit:skills": "node .github/scripts/lint-skill-entry.mjs"
   },
   "publishConfig": {
     "access": "public",