build: refuse symlinked skill files and keep sync crash traces

bestax-mcp/scripts/sync-skills.mjs

@@ -15,6 +15,7 @@
 // without a second committed copy of 390 KB of markdown bloating every diff.
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
+import { failureText } from '../../scripts/lib/skills.mjs';
 import { syncSkills } from '../../scripts/lib/sync-skills.mjs';
 
 const pkgRoot = path.resolve(
@@ -33,6 +34,6 @@ await syncSkills({
   // data/skills itself, and `files` negates it so it never ships.
   stateDir: path.join(pkgRoot, 'data', '.sync-skills'),
 }).catch(err => {
-  console.error(err.message);
+  console.error(failureText(err));
   process.exit(1);
 });

create-bestax/scripts/sync-skills.mjs

@@ -29,6 +29,7 @@
 // Pass one if a third caller is ever added, and keep it out of `files`.
 import { fileURLToPath } from 'node:url';
 import path from 'node:path';
+import { failureText } from '../../scripts/lib/skills.mjs';
 import { syncSkills } from '../../scripts/lib/sync-skills.mjs';
 
 const pkgRoot = path.resolve(
@@ -41,6 +42,6 @@ await syncSkills({
   dest: path.join(pkgRoot, 'templates', 'skills'),
   label: 'templates/skills',
 }).catch(err => {
-  console.error(err.message);
+  console.error(failureText(err));
   process.exit(1);
 });

scripts/check-conformance.mjs

@@ -3290,12 +3290,20 @@ function section(heading) {
 /**
  * What is wrong with the directories themselves, before any roster is read.
  * Pure, and separate from the walk, for the same reason rosterViolations is:
- * neither branch fires on the real tree, so only fixtures execute them.
+ * none of its branches fires on the real tree, so only fixtures execute them.
  */
 export function skillDirViolations(dirs) {
   const violations = [];
-  for (const { name, hasSkillFile } of dirs) {
-    if (!hasSkillFile) {
+  for (const { name, hasSkillFile, isSymlink } of dirs) {
+    if (isSymlink) {
+      // The sync scripts and the MCP index refuse it through skillFiles, so
+      // reporting it here keeps every tool on the same skill set.
+      violations.push(
+        `skills/${name}/: is a symbolic link, and the sync scripts and the ` +
+          `MCP index refuse a linked skill. Commit the real directory, or ` +
+          `remove the link.`
+      );
+    } else if (!hasSkillFile) {
       // A directory that looks like a skill and is not. The old hardcoded list
       // at least failed on a name it could not resolve; discovery just skips
       // it, which would be the same silent omission in a new place.

scripts/gen-mcp-index.mjs

@@ -52,6 +52,7 @@ import {
 import {
   assertSkillsVetted,
   byCodePoint,
+  failureText,
   readSkillNames,
   skillFiles,
   skillSlug,
@@ -312,6 +313,10 @@ export async function readSkills(skillsDir = SKILLS_DIR) {
   assertSkillsVetted(skillsDir, names, 'index');
   const out = [];
   for (const name of names) {
+    // One walk per skill, from its root, so a symbolic link anywhere in it is
+    // refused just as the sync scripts refuse it: the skill directory itself,
+    // SKILL.md, or any file below. The listings below are cut from this walk.
+    const shipped = await skillFiles(join(skillsDir, name));
     const skillFile = join(skillsDir, name, 'SKILL.md');
     const src = await readFile(skillFile, 'utf8');
     const fm = frontmatter(src);
@@ -335,14 +340,15 @@ export async function readSkills(skillsDir = SKILLS_DIR) {
     // one, and agents discover ids through `get_skill` rather than hardcoding
     // them, so the rename is accepted rather than aliased.
     const listing = async sub => {
-      const root = join(skillsDir, name, sub);
-      if (!existsSync(root)) return [];
+      const prefix = `${sub}/`;
       const files = [];
-      for (const rel of await skillFiles(root)) {
-        const body = await readFile(join(root, rel), 'utf8');
+      for (const file of shipped) {
+        if (!file.startsWith(prefix)) continue;
+        const rel = file.slice(prefix.length);
+        const body = await readFile(join(skillsDir, name, file), 'utf8');
         files.push({
           id: rel.replace(extname(rel), '').replace(/\//g, '-'),
-          file: `${sub}/${rel}`,
+          file,
           bytes: Buffer.byteLength(body),
         });
       }
@@ -672,9 +678,19 @@ export async function main() {
   );
 }
 
+/**
+ * The command line's failure handler: failureText, so a refusal prints its
+ * message and anything else its stack, then exit 1. `error` and `exit` are
+ * injectable so the tests can drive it.
+ */
+export function reportFailure(
+  err,
+  { error = console.error, exit = process.exit } = {}
+) {
+  error(failureText(err));
+  exit(1);
+}
+
 if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) {
-  main().catch(err => {
-    console.error(err.message ?? err);
-    process.exit(1);
-  });
+  main().catch(err => reportFailure(err));
 }

scripts/gen-mcp-index.test.mjs

@@ -22,8 +22,8 @@ import { readFile } from 'node:fs/promises';
 import { join, dirname } from 'node:path';
 import { fileURLToPath } from 'node:url';
 
-import { build, readSkills } from './gen-mcp-index.mjs';
-import { skillSlug } from './lib/skills.mjs';
+import { build, readSkills, reportFailure } from './gen-mcp-index.mjs';
+import { failureText, skillRefusal, skillSlug } from './lib/skills.mjs';
 
 const REPO = join(dirname(fileURLToPath(import.meta.url)), '..');
 
@@ -252,6 +252,79 @@ test('the skills manifest lists exactly what the sync scripts ship', async t =>
   await assert.rejects(readSkills(skillsDir), /debug\.log/);
 });
 
+test('the index refuses a symbolic link wherever the sync scripts do', async t => {
+  // The sync scripts refuse a link anywhere in a skill. The index used to
+  // walk only references/ and examples/, so it listed a linked skill
+  // directory, or a skill whose SKILL.md was a link, that no build can copy.
+  const {
+    mkdtempSync,
+    mkdirSync,
+    renameSync,
+    rmSync,
+    symlinkSync,
+    unlinkSync,
+    writeFileSync,
+  } = await import('node:fs');
+  const { tmpdir } = await import('node:os');
+
+  const root = mkdtempSync(join(tmpdir(), 'bestax-mcp-links-'));
+  t.after(() => rmSync(root, { recursive: true, force: true }));
+  const skillsDir = join(root, 'skills');
+  const skill = join(skillsDir, 'bestax-form');
+  mkdirSync(join(skill, 'references'), { recursive: true });
+  writeFileSync(
+    join(skill, 'SKILL.md'),
+    '---\nname: bestax-form\ndescription: Build forms.\n---\n'
+  );
+  writeFileSync(join(skill, 'references', 'api.md'), '# api\n');
+  const [form] = await readSkills(skillsDir);
+  assert.deepEqual(form.references, [
+    { id: 'api', file: 'references/api.md', bytes: 6 },
+  ]);
+  assert.deepEqual(form.examples, []);
+
+  const outside = join(root, 'outside');
+  mkdirSync(outside);
+  writeFileSync(
+    join(outside, 'SKILL.md'),
+    '---\nname: bestax-linked\ndescription: Linked.\n---\n'
+  );
+  symlinkSync(outside, join(skillsDir, 'bestax-linked'));
+  const linked = await readSkills(skillsDir).then(
+    () => assert.fail('a linked skill directory was indexed'),
+    err => err
+  );
+  assert.match(
+    linked.message,
+    /skills\/bestax-linked is a symbolic link\. Commit the real directory\.$/
+  );
+  assert.equal(failureText(linked), linked.message);
+  unlinkSync(join(skillsDir, 'bestax-linked'));
+
+  renameSync(join(skill, 'SKILL.md'), join(root, 'SKILL.md'));
+  symlinkSync(join(root, 'SKILL.md'), join(skill, 'SKILL.md'));
+  await assert.rejects(
+    readSkills(skillsDir),
+    /bestax-form\/SKILL\.md is a symbolic link\. Commit the real file or directory\./
+  );
+});
+
+test('the command line prints a refusal as its message, anything else as its stack', () => {
+  const report = err => {
+    const out = [];
+    reportFailure(err, {
+      error: text => out.push(text),
+      exit: code => out.push(code),
+    });
+    return out;
+  };
+  const refusal = skillRefusal('refusing to index untracked file(s)');
+  assert.deepEqual(report(refusal), [refusal.message, 1]);
+  const crash = new TypeError('boom');
+  assert.deepEqual(report(crash), [crash.stack, 1]);
+  assert.match(crash.stack, /^TypeError: boom\n\s+at /);
+});
+
 test('every skill reference id is unique within its skill', () => {
   for (const s of skills.skills) {
     const ids = [...s.references, ...s.examples].map(r => r.id);

scripts/lib/api-page.mjs

@@ -5,7 +5,7 @@
  * Everything here is FENCE-AWARE. `docs/docs/api/helpers/theme.md` contains a
  * `---` inside a code fence, and several pages show HTML comments inside `html`
  * fences — a naive line scan would treat those as a section break or a marker
- * and corrupt the page. Every scan therefore runs through `fenceMask()`.
+ * and corrupt the page.
  *
  * Region markers look like:
  *

scripts/lib/skills.mjs

@@ -6,7 +6,7 @@
 // scripts/lib/shell-words.mjs (#436) exists to prevent. All four import from
 // here now; a predicate change lands once or not at all.
 
-import { readdir, stat } from 'node:fs/promises';
+import { lstat, readdir, stat } from 'node:fs/promises';
 import { realpathSync } from 'node:fs';
 import { join, dirname, resolve } from 'node:path';
 import { execFileSync } from 'node:child_process';
@@ -38,11 +38,27 @@ export function byCodePoint(a, b) {
  * A dotted directory WITHOUT a SKILL.md is tooling and is ignored; one WITH a
  * SKILL.md is reported, because every consumer of this predicate really would
  * bundle it.
+ *
+ * `isSymlink` marks a symbolic link to a directory. It stays in the skill set
+ * so every tool sees the same skills, and every tool refuses it: the
+ * conformance check reports it, and skillFiles throws on it for the sync
+ * scripts and the MCP index.
  */
 export async function readSkillDirs(dir) {
   const found = [];
   for (const entry of await readdir(dir, { withFileTypes: true })) {
-    if (!entry.isDirectory()) continue;
+    // A symbolic link counts only when it points at a directory, so a linked
+    // file such as `AGENTS.md -> CLAUDE.md` is not mistaken for a skill. A
+    // linked directory is listed and marked rather than skipped, so every
+    // consumer refuses it instead of quietly leaving it out.
+    const isSymlink = entry.isSymbolicLink();
+    const isDir = isSymlink
+      ? await stat(join(dir, entry.name)).then(
+          s => s.isDirectory(),
+          () => false
+        )
+      : entry.isDirectory();
+    if (!isDir) continue;
 
     // A regular file specifically: a directory named SKILL.md would satisfy
     // a bare existence probe and then break every consumer that reads it.
@@ -55,7 +71,7 @@ export async function readSkillDirs(dir) {
 
     if (entry.name.startsWith('.') && !hasSkillFile) continue;
 
-    found.push({ name: entry.name, hasSkillFile });
+    found.push({ name: entry.name, hasSkillFile, isSymlink });
   }
   return found.sort((a, b) => byCodePoint(a.name, b.name));
 }
@@ -116,19 +132,66 @@ export function isDsStore(path) {
   return path.endsWith('.DS_Store');
 }
 
+/**
+ * The error code a refusal carries: a skill file that is untracked or a
+ * symbolic link, or a sync that cannot start. Its message says what to do.
+ * Callers tell it apart from an unexpected failure, such as a permission
+ * error, which should keep its own message and stack.
+ */
+export const SKILL_REFUSAL = 'ESKILLREFUSAL';
+
+/** An Error with `message` and `options` that carries SKILL_REFUSAL. */
+export function skillRefusal(message, options) {
+  return Object.assign(new Error(message, options), { code: SKILL_REFUSAL });
+}
+
+export function isSkillRefusal(err) {
+  return err?.code === SKILL_REFUSAL;
+}
+
+/**
+ * What a command line prints when it fails: the message alone for a refusal,
+ * which already says what to do, and the full stack for anything else, a
+ * filesystem or programming error someone has to debug. The sync scripts and
+ * gen-mcp-index all print through this, so they classify a failure the same
+ * way, by its code.
+ */
+export function failureText(err) {
+  if (isSkillRefusal(err)) return String(err.message);
+  return String(err?.stack ?? err?.message ?? err);
+}
+
 /**
  * The files under `dir` a bundler ships, as paths relative to it with
  * forward slashes: depth first, each directory's entries in code-point
  * order, and nothing isDsStore matches. What the MCP index lists, so it
- * cannot list a file the sync scripts leave out.
+ * cannot list a file the sync scripts leave out. Throws a refusal on a
+ * symbolic link, at `dir` itself or anywhere below it.
  */
 export async function skillFiles(dir) {
+  if ((await lstat(dir)).isSymbolicLink()) {
+    throw skillRefusal(`${dir} is a symbolic link. Commit the real directory.`);
+  }
   const out = [];
   const walk = async rel => {
     const entries = await readdir(join(dir, rel), { withFileTypes: true });
     for (const e of entries.sort((a, b) => byCodePoint(a.name, b.name))) {
       const next = rel ? `${rel}/${e.name}` : e.name;
+      // .DS_Store is skipped first, file or link alike, since
+      // `ln -s /dev/null .DS_Store` is a common way to stop Finder writing
+      // one. Skipping a link is safe because the bundlers copy only the
+      // files this walk lists, so nothing under that name can ship.
       if (isDsStore(next)) continue;
+      // Any other symbolic link is refused, not skipped. Copied as a link it
+      // would point at this checkout and dangle once the package leaves this
+      // machine. Followed, it would ship whatever it points at, which the
+      // vetting gate never saw.
+      if (e.isSymbolicLink()) {
+        throw skillRefusal(
+          `${join(dir, next)} is a symbolic link. ` +
+            'Commit the real file or directory.'
+        );
+      }
       if (e.isDirectory()) await walk(next);
       else if (e.isFile()) out.push(next);
     }
@@ -145,7 +208,7 @@ export async function skillFiles(dir) {
 export function assertSkillsVetted(skillsDir, names, verb) {
   const untracked = untrackedSkillPaths(skillsDir, names);
   if (untracked.length) {
-    throw new Error(
+    throw skillRefusal(
       `refusing to ${verb} untracked file(s) under skills/: ` +
         `${untracked.join(', ')}. \`git add\` them to vet them, or remove them.`
     );