Merge #571: enforce the skill-anatomy layout rules

scripts/lib/skill-lint-test.js

@@ -5,10 +5,29 @@
 const assert = require('node:assert/strict');
 const { test } = require('node:test');
 
-const { lintSkillContent } = require('./skill-lint.js');
+const fs   = require('node:fs');
+const os   = require('node:os');
+const path = require('node:path');
+
+const { lintSkillContent, lintSkillLayout } = require('./skill-lint.js');
 
 const KNOWN = new Set(['alpha', 'beta']);
 
+/**
+ * Build a throwaway skill directory. `dirs` are created empty; `files` maps a
+ * path within the skill to its contents, creating parents as needed.
+ */
+function makeSkillDir({ dirs = [], files = {} } = {}) {
+  const root = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-layout-'));
+  for (const d of dirs) fs.mkdirSync(path.join(root, d), { recursive: true });
+  for (const [rel, body] of Object.entries(files)) {
+    const abs = path.join(root, rel);
+    fs.mkdirSync(path.dirname(abs), { recursive: true });
+    fs.writeFileSync(abs, body);
+  }
+  return root;
+}
+
 /** A SKILL.md body carrying every required section, so tests can isolate frontmatter. */
 function withAllSections(frontmatter) {
   return [
@@ -337,3 +356,82 @@ test('the error names the line so the fix is obvious', () => {
   );
   assert.match(yamlErrors(result)[0], /^Frontmatter line 4 /);
 });
+
+// ─── Context budget ──────────────────────────────────────────────────────────
+
+test('warns when SKILL.md exceeds the 500-line context budget', () => {
+  const padded = withAllSections(VALID_FRONTMATTER) + '\n'.repeat(600);
+
+  const { errors, warnings } = lintSkillContent('alpha', padded, KNOWN);
+
+  assert.equal(errors.length, 0, 'an over-budget skill must not block CI');
+  assert.equal(warnings.length, 1);
+  assert.match(warnings[0], /over the 500-line context budget/);
+});
+
+test('a SKILL.md at exactly the budget is not flagged', () => {
+  const base = withAllSections(VALID_FRONTMATTER);
+  const baseLines = (base.match(/\n/g) || []).length;
+  const atBudget = base + '\n'.repeat(500 - baseLines);
+
+  assert.equal((atBudget.match(/\n/g) || []).length, 500, 'fixture must sit exactly on the boundary');
+  const { warnings } = lintSkillContent('alpha', atBudget, KNOWN);
+
+  assert.equal(warnings.length, 0);
+});
+
+// ─── Layout ──────────────────────────────────────────────────────────────────
+
+test('reports an empty scripts/ directory', () => {
+  const dir = makeSkillDir({ dirs: ['scripts'] });
+
+  const errors = lintSkillLayout(dir);
+
+  assert.equal(errors.length, 1);
+  assert.match(errors[0], /Empty directory `scripts\/`/);
+});
+
+test('reports a directory that only nests more empty directories', () => {
+  const dir = makeSkillDir({ dirs: ['references', 'references/deep'] });
+
+  const errors = lintSkillLayout(dir);
+
+  assert.equal(errors.length, 1, 'the outermost empty directory is named once, not every level');
+  assert.match(errors[0], /Empty directory `references\/`/);
+});
+
+test('a directory holding a file is not empty', () => {
+  const dir = makeSkillDir({ files: { 'scripts/helper.sh': '#!/bin/bash\nset -e\n' } });
+
+  assert.deepEqual(lintSkillLayout(dir), []);
+});
+
+test('reports a supporting .md file that is not lowercase-hyphen-separated', () => {
+  const dir = makeSkillDir({ files: { 'Refinement_Criteria.md': 'x\n' } });
+
+  const errors = lintSkillLayout(dir);
+
+  assert.equal(errors.length, 1);
+  assert.match(errors[0], /Supporting file `Refinement_Criteria\.md` is not lowercase-hyphen-separated/);
+});
+
+test('names a badly named supporting file by its path within the skill', () => {
+  const dir = makeSkillDir({ files: { 'references/Floor_Guard.md': 'x\n' } });
+
+  const errors = lintSkillLayout(dir);
+
+  assert.equal(errors.length, 1);
+  assert.match(errors[0], /`references\/Floor_Guard\.md`/);
+});
+
+test('SKILL.md is exempt from the supporting-file naming rule', () => {
+  const dir = makeSkillDir({ files: { 'SKILL.md': 'x\n', 'examples.md': 'x\n' } });
+
+  assert.deepEqual(lintSkillLayout(dir), []);
+});
+
+test('non-markdown files are left to the Script Requirements conventions', () => {
+  const dir = makeSkillDir({ files: { 'scripts/Idea_Refine.sh': '#!/bin/bash\nset -e\n' } });
+
+  assert.deepEqual(lintSkillLayout(dir), []);
+});

scripts/lib/skill-lint.js

@@ -15,9 +15,12 @@
  *   - description does not exceed 1024 characters
  *   - description includes a 'when to use' trigger (skill-anatomy.md: Required)
  *   - required sections are present
+ *   - a skill's subdirectories are non-empty (skill-anatomy.md: Supporting Files)
+ *   - supporting .md filenames are lowercase-hyphen-separated
  *
  * Checks (warnings, do not block CI):
  *   - cross-skill references point to known skills
+ *   - SKILL.md stays under the 500-line context budget
  */
 
 const fs   = require('fs');
@@ -27,6 +30,12 @@ const path = require('path');
 
 const MAX_DESCRIPTION_LENGTH = 1024;
 
+// SKILL.md loads in full the moment an agent activates the skill, so its length
+// is paid for on every activation (docs/skill-anatomy.md -> Context Efficiency).
+// A warning, not an error: trimming a skill is an editorial call, and failing
+// CI on line count would block unrelated work on an over-budget skill.
+const MAX_SKILL_LINES = 500;
+
 // A skill directory name must be lowercase-hyphen-separated
 // (docs/skill-anatomy.md → Naming Conventions).
 const KEBAB_CASE = /^[a-z0-9]+(-[a-z0-9]+)*$/;
@@ -354,6 +363,17 @@ function lintSkillContent(dirName, content, knownSkills) {
     }
   }
 
+  // ── Context budget ───────────────────────────────────────────────────────
+  // Counted as newlines so the number matches what `wc -l` reports, which is
+  // how a contributor would check this by hand.
+  const lineCount = (content.match(/\n/g) || []).length;
+  if (lineCount > MAX_SKILL_LINES) {
+    warnings.push(
+      `SKILL.md is ${lineCount} lines, over the ${MAX_SKILL_LINES}-line context budget ` +
+      `(skill-anatomy.md: Context Efficiency) — move detail into a supporting file`,
+    );
+  }
+
   // ── Cross-skill references ───────────────────────────────────────────────
   const refs = extractSkillReferences(content);
   for (const ref of refs) {
@@ -365,13 +385,73 @@ function lintSkillContent(dirName, content, knownSkills) {
   return { errors, warnings, exempt };
 }
 
+/** True when dir contains no regular file at any depth. */
+function isEffectivelyEmpty(dir) {
+  for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+    if (entry.isDirectory()) {
+      if (!isEffectivelyEmpty(path.join(dir, entry.name))) return false;
+    } else {
+      return false;
+    }
+  }
+  return true;
+}
+
+/**
+ * Lint the files that travel with a skill, as opposed to the contents of
+ * SKILL.md. These rules need the filesystem, so they live here rather than in
+ * the pure content linter. Returns an array of error strings.
+ *
+ * Enforced (docs/skill-anatomy.md):
+ *   - Supporting Files: "do not create an empty `scripts/` directory just to
+ *     mirror other skills". Generalized to any subdirectory — an empty
+ *     `references/` is noise for the same reason.
+ *   - Naming Conventions: "Supporting files: lowercase-hyphen-separated.md".
+ *     Scoped to .md, which is what that line covers; `scripts/` helpers carry
+ *     their own conventions under Script Requirements.
+ */
+function lintSkillLayout(skillDir) {
+  const errors = [];
+
+  const walk = (dir, relBase) => {
+    for (const entry of fs.readdirSync(dir, { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name))) {
+      const rel = relBase ? `${relBase}/${entry.name}` : entry.name;
+      const abs = path.join(dir, entry.name);
+
+      if (entry.isDirectory()) {
+        if (isEffectivelyEmpty(abs)) {
+          errors.push(
+            `Empty directory \`${rel}/\` — remove it rather than mirroring another skill's layout ` +
+            `(skill-anatomy.md: Supporting Files)`,
+          );
+        } else {
+          walk(abs, rel);
+        }
+        continue;
+      }
+
+      if (rel === 'SKILL.md' || !entry.name.endsWith('.md')) continue;
+      if (!KEBAB_CASE.test(entry.name.slice(0, -'.md'.length))) {
+        errors.push(
+          `Supporting file \`${rel}\` is not lowercase-hyphen-separated ` +
+          `(skill-anatomy.md: Naming Conventions)`,
+        );
+      }
+    }
+  };
+
+  walk(skillDir, '');
+  return errors;
+}
+
 /**
  * Lint a skill by directory name: reads its SKILL.md, then delegates to
  * lintSkillContent. This is the thin filesystem wrapper the CLI uses.
  * Returns { errors, warnings, exempt }.
  */
 function lintSkill(dirName, skillsDir, knownSkills) {
-  const skillPath = path.join(skillsDir, dirName, 'SKILL.md');
+  const skillDir  = path.join(skillsDir, dirName);
+  const skillPath = path.join(skillDir, 'SKILL.md');
 
   if (!fs.existsSync(skillPath)) {
     return { errors: ['Missing SKILL.md'], warnings: [], exempt: false };
@@ -384,7 +464,9 @@ function lintSkill(dirName, skillsDir, knownSkills) {
     return { errors: [`Unreadable SKILL.md: ${err.message}`], warnings: [], exempt: false };
   }
 
-  return lintSkillContent(dirName, content, knownSkills);
+  const result = lintSkillContent(dirName, content, knownSkills);
+  result.errors.push(...lintSkillLayout(skillDir));
+  return result;
 }
 
 // Export only functions. The policy collections (REQUIRED_SECTIONS,
@@ -397,5 +479,6 @@ module.exports = {
   frontmatterYamlErrors,
   extractSkillReferences,
   lintSkillContent,
+  lintSkillLayout,
   lintSkill,
 };