diff --git a/bestax-mcp/scripts/sync-skills.mjs b/bestax-mcp/scripts/sync-skills.mjs index cfdaaeccb..2b20d748e 100644 --- a/bestax-mcp/scripts/sync-skills.mjs +++ b/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); }); diff --git a/create-bestax/scripts/sync-skills.mjs b/create-bestax/scripts/sync-skills.mjs index 5dfe6c546..d4073f341 100644 --- a/create-bestax/scripts/sync-skills.mjs +++ b/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); }); diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index f9c471350..d1e61b2de 100644 --- a/scripts/check-conformance.mjs +++ b/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. diff --git a/scripts/gen-mcp-index.mjs b/scripts/gen-mcp-index.mjs index 992ca5704..89cf6be17 100644 --- a/scripts/gen-mcp-index.mjs +++ b/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)); } diff --git a/scripts/gen-mcp-index.test.mjs b/scripts/gen-mcp-index.test.mjs index 96ee7ddba..4e907a930 100644 --- a/scripts/gen-mcp-index.test.mjs +++ b/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); diff --git a/scripts/lib/api-page.mjs b/scripts/lib/api-page.mjs index 2b48df937..dfc9c5030 100644 --- a/scripts/lib/api-page.mjs +++ b/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: * diff --git a/scripts/lib/skills.mjs b/scripts/lib/skills.mjs index 74646c624..4caec7c93 100644 --- a/scripts/lib/skills.mjs +++ b/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.` ); diff --git a/scripts/lib/sync-skills.mjs b/scripts/lib/sync-skills.mjs index f7d1d0448..008476a2e 100644 --- a/scripts/lib/sync-skills.mjs +++ b/scripts/lib/sync-skills.mjs @@ -8,21 +8,37 @@ // `/skills` is the single source of truth, and every destination is generated // and gitignored. The roster is READ, not listed (#540): every directory // holding a SKILL.md is copied, through the shared predicate in skills.mjs. -import { cp, mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'; +import { + copyFile, + mkdir, + readFile, + rm, + stat, + writeFile, +} from 'node:fs/promises'; import { existsSync, rmSync } from 'node:fs'; import { createHash } from 'node:crypto'; import { setTimeout as sleep } from 'node:timers/promises'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { assertSkillsVetted, - isDsStore, + isSkillRefusal, readSkillNames, skillFiles, + skillRefusal, } from './skills.mjs'; const TAG = '[sync-skills]'; +/** + * An expected failure: tagged, and carrying the refusal code, so a caller + * printing failureText shows this message alone. + */ +function refusal(message, options) { + return skillRefusal(`${TAG} ${message}`, options); +} + // A sync is ~390 KB of file copies and takes tens of milliseconds. A lock held // longer than this is a crashed run, not a slow one. const LOCK = { staleMs: 60_000, timeoutMs: 120_000, pollMs: 50 }; @@ -31,8 +47,9 @@ const SIGNALS = ['SIGINT', 'SIGTERM', 'SIGHUP']; /** * Copy every skill under `src` into `dest`, after the checks every bundler - * makes, and log one line saying what happened. Throws an Error whose message - * starts with `[sync-skills]` on any expected failure, for the caller to print. + * makes, and log one line saying what happened. On any expected failure it + * throws an Error whose message starts with `[sync-skills]` and that carries + * the refusal code, so the caller prints it with failureText. * * - `label` is how the success line names `dest` (`templates/skills`). * - `stateDir`, when given, holds a lock and a fingerprint of the source. @@ -49,28 +66,17 @@ export async function syncSkills({ lock = {}, log = console.log, }) { - if (!existsSync(src)) throw new Error(`${TAG} source not found: ${src}`); + if (!existsSync(src)) throw refusal(`source not found: ${src}`); const names = await readSkillNames(src); // Checked BEFORE emptying the destination. Reading a roster off disk means // a wrong path or a renamed directory yields zero skills instead of an // error, and emptying first would ship an empty bundle rather than failing. - if (!names.length) throw new Error(`${TAG} no skills found in ${src}`); - - // The vetting gate the deleted allowlist used to be: discovery bundles - // whatever is on disk, and CI's skills-roster check only sees committed - // state, so an untracked scratch file would ship in a local build or a - // manual publish with no gate anywhere in the path. `git add` is the act - // of vetting, and a tree without git (an exported tarball) skips the gate. - try { - assertSkillsVetted(src, names, 'bundle'); - } catch (err) { - throw new Error(`${TAG} ${err.message}`, { cause: err }); - } + if (!names.length) throw refusal(`no skills found in ${src}`); if (!stateDir) { - await copySkills(src, dest, names); + await copySkills(dest, await vettedSkills(src, names)); log(`${TAG} copied ${names.length} skills -> ${label}`); return; } @@ -80,7 +86,6 @@ export async function syncSkills({ const held = { dir: join(stateDir, 'lock'), held: false }; await mkdir(stateDir, { recursive: true }); - const want = await fingerprint(src, names); // Release on interrupt. Ctrl-C during `pnpm all` is the realistic way a lock // gets abandoned, and node does not run `finally` blocks on a signal, so @@ -94,6 +99,11 @@ export async function syncSkills({ try { await acquireLock(held, timing); + // Vetted only once the lock is held. A run can wait on it for minutes, + // and a file or link added meanwhile is caught here, because the + // fingerprint and the copy both use this list and nothing else. + const skills = await vettedSkills(src, names); + const want = await fingerprint(skills); const have = await readFile(stampFile, 'utf8').catch(() => ''); // The freshness check is load-bearing, not an optimisation. Without it a @@ -105,7 +115,7 @@ export async function syncSkills({ if (have.trim() === want && existsSync(dest)) { log(`${TAG} up to date (${names.length} skills)`); } else { - await copySkills(src, dest, names); + await copySkills(dest, skills); // Stamped last: a run killed mid-copy leaves no stamp, so the next // caller redoes the work rather than trusting a half-populated tree. await writeFile(stampFile, `${want}\n`); @@ -117,17 +127,53 @@ export async function syncSkills({ } } -/** Empty `dest`, then copy each skill into it, leaving `.DS_Store` out. */ -async function copySkills(src, dest, names) { +/** + * Each skill in `names` with the files a copy ships, once every bundler's + * checks pass: `{ name, dir, files }`, where `files` is skillFiles' list for + * `dir`. The fingerprint and the copy both take this list, so what was + * checked is exactly what ships, and each skill is walked once. + * + * skillFiles leaves every `.DS_Store` out and refuses any other symbolic + * link in a skill, the skill directory included. The vetting gate is the + * one the deleted allowlist used to be: discovery bundles whatever is on + * disk, and CI's skills-roster check only sees committed state, so an + * untracked scratch file would ship in a local build or a manual publish + * with no gate anywhere in the path. `git add` is the act of vetting, and a + * tree without git (an exported tarball) skips the gate. It runs after the + * walk, so every listed file already existed when git was asked about it. + * + * Only those refusals get the tag. Anything else, such as a permission error, + * passes through with its own message and stack. + */ +async function vettedSkills(src, names) { + try { + const skills = []; + for (const name of names) { + const dir = join(src, name); + skills.push({ name, dir, files: await skillFiles(dir) }); + } + assertSkillsVetted(src, names, 'bundle'); + return skills; + } catch (err) { + if (!isSkillRefusal(err)) throw err; + throw refusal(err.message, { cause: err }); + } +} + +/** + * Empty `dest`, then copy exactly the files vettedSkills listed into it. A + * file that appeared on disk after the walk is not in the list, so it is not + * copied. Exported for its test. + */ +export async function copySkills(dest, skills) { await rm(dest, { recursive: true, force: true }); await mkdir(dest, { recursive: true }); - for (const name of names) { - await cp(join(src, name), join(dest, name), { - recursive: true, - // .DS_Store is the one path the vetting gate exempts. Keep it out of - // the bundle too so the exemption never becomes shipped content. - filter: path => !isDsStore(path), - }); + for (const { name, dir, files } of skills) { + for (const rel of files) { + const to = join(dest, name, rel); + await mkdir(dirname(to), { recursive: true }); + await copyFile(join(dir, rel), to); + } } } @@ -136,12 +182,12 @@ async function copySkills(src, dest, names) { // rewrites mtimes without changing what should be copied, and re-copying is // the cheap half. It is the destructive `rm` that has to be avoided while // another process is reading. -async function fingerprint(src, names) { +async function fingerprint(skills) { const hash = createHash('sha256'); - for (const name of names) { - for (const rel of await skillFiles(join(src, name))) { + for (const { name, dir, files } of skills) { + for (const rel of files) { hash.update(`${name}/${rel}`); - hash.update(await readFile(join(src, name, rel))); + hash.update(await readFile(join(dir, rel))); } } return hash.digest('hex'); @@ -203,8 +249,8 @@ async function acquireLock(lock, { staleMs, timeoutMs, pollMs }) { // Held far longer than a sync can legitimately take, so the holder is // gone. Report it now instead of burning the full timeout first. if (ageMs > staleMs) { - throw new Error( - `${TAG} the lock at ${lock.dir} has been held for ` + + throw refusal( + `the lock at ${lock.dir} has been held for ` + `${Math.round(ageMs / 1000)}s, which means an earlier run was killed ` + `before it could clean up. Remove that directory and re-run:\n` + ` rm -rf ${lock.dir}`, @@ -213,7 +259,7 @@ async function acquireLock(lock, { staleMs, timeoutMs, pollMs }) { } if (Date.now() > deadline) { - throw new Error(`${TAG} timed out waiting for lock: ${lock.dir}`, { + throw refusal(`timed out waiting for lock: ${lock.dir}`, { cause: err, }); } diff --git a/scripts/skills-roster.test.mjs b/scripts/skills-roster.test.mjs index b2e3e6fec..1d3dfef26 100644 --- a/scripts/skills-roster.test.mjs +++ b/scripts/skills-roster.test.mjs @@ -267,6 +267,47 @@ test('a hidden directory holding a SKILL.md is not skipped', () => { assert.match(v[0], /kebab-case/); }); +test('a symlinked skill directory is reported, as every bundler refuses it', async t => { + // The sync scripts and the MCP index refuse it through skillFiles. Reading + // it as an ordinary skill here would leave conformance green on a tree + // that cannot build. + const { mkdtempSync, mkdirSync, rmSync, symlinkSync, writeFileSync } = + await import('node:fs'); + const { tmpdir } = await import('node:os'); + const { join } = await import('node:path'); + + const root = mkdtempSync(join(tmpdir(), 'bestax-roster-link-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const skillsDir = join(root, 'skills'); + mkdirSync(join(skillsDir, 'bestax-form'), { recursive: true }); + writeFileSync(join(skillsDir, 'bestax-form', 'SKILL.md'), '# form\n'); + writeFileSync(join(skillsDir, 'CLAUDE.md'), '# notes\n'); + mkdirSync(join(root, 'elsewhere')); + writeFileSync(join(root, 'elsewhere', 'SKILL.md'), '# linked\n'); + symlinkSync(join(root, 'elsewhere'), join(skillsDir, 'bestax-linked')); + // A link to a file is not a skill directory, so it is no violation either. + symlinkSync(join(skillsDir, 'CLAUDE.md'), join(skillsDir, 'AGENTS.md')); + + const dirs = await readSkillDirs(skillsDir); + assert.deepEqual( + dirs.map(d => d.name), + ['bestax-form', 'bestax-linked'] + ); + // Still in the skill set, so every tool agrees on what the skills are. + assert.deepEqual(rosterSkillNames(dirs), ['bestax-form', 'bestax-linked']); + const v = skillDirViolations(dirs); + assert.equal(v.length, 1, v.join('\n')); + assert.match(v[0], /^skills\/bestax-linked\/: is a symbolic link/); + assert.match(v[0], /Commit the real directory/); + + // Reported even without a SKILL.md, and only once. + const bare = skillDirViolations([ + { name: 'bestax-bare', hasSkillFile: false, isSymlink: true }, + ]); + assert.equal(bare.length, 1, bare.join('\n')); + assert.match(bare[0], /symbolic link/); +}); + test('the section scope is line-anchored, not a substring search', () => { // `'## AI skills'` occurs inside `'### AI skills'`, so an indexOf-based scope // would lock onto a subheading and report an intact roster as wholly missing. diff --git a/scripts/sync-skills.test.mjs b/scripts/sync-skills.test.mjs index de31c4368..7809fef71 100644 --- a/scripts/sync-skills.test.mjs +++ b/scripts/sync-skills.test.mjs @@ -11,16 +11,27 @@ import { mkdirSync, mkdtempSync, readFileSync, + readdirSync, + chmodSync, rmSync, + symlinkSync, utimesSync, writeFileSync, } from 'node:fs'; import { execFileSync, spawn } from 'node:child_process'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { setTimeout as sleep } from 'node:timers/promises'; import { pathToFileURL } from 'node:url'; -import { syncSkills } from './lib/sync-skills.mjs'; +import { copySkills, syncSkills } from './lib/sync-skills.mjs'; +import { + SKILL_REFUSAL, + failureText, + readSkillDirs, + skillFiles, + skillRefusal, +} from './lib/skills.mjs'; /** A repo-shaped temp dir holding skills/ with two skills and some noise. */ function fixture(t) { @@ -53,6 +64,16 @@ function fixture(t) { /** The options create-bestax passes: no state dir, so no lock. */ const unlocked = f => ({ ...f, stateDir: undefined, label: 'out' }); +/** Every path under `dir`, relative and sorted, directories included. */ +const tree = dir => readdirSync(dir, { recursive: true }).sort(); + +/** What `run` rejected with, failing the test if it resolved. */ +const rejection = run => + run.then( + () => assert.fail('resolved'), + err => err + ); + test('copies every skill, leaves .DS_Store out, and replaces a stale copy', async t => { const f = fixture(t); mkdirSync(join(f.dest, 'bestax-gone'), { recursive: true }); @@ -74,10 +95,12 @@ test('copies every skill, leaves .DS_Store out, and replaces a stale copy', asyn test('fails before touching the destination', async t => { const f = fixture(t); - await assert.rejects( - syncSkills({ ...unlocked(f), src: join(f.root, 'nope') }), - /^Error: \[sync-skills\] source not found: / + const missing = await rejection( + syncSkills({ ...unlocked(f), src: join(f.root, 'nope') }) ); + assert.match(String(missing), /^Error: \[sync-skills\] source not found: /); + assert.equal(missing.code, SKILL_REFUSAL); + assert.equal(failureText(missing), missing.message); const empty = join(f.root, 'empty'); mkdirSync(empty); mkdirSync(join(f.dest, 'bestax-kept'), { recursive: true }); @@ -199,3 +222,197 @@ test('a signal while waiting exits 130 and leaves the lock it never held', async assert.equal(code, 130); assert.ok(existsSync(lockDir), 'a lock this run never held was removed'); }); + +test('a symbolic link in a skill is refused before anything is copied', async t => { + for (const locked of [false, true]) { + const f = fixture(t); + symlinkSync( + join(f.src, 'bestax-a', 'references', 'x.md'), + join(f.src, 'bestax-b', 'linked.md') + ); + const opts = locked ? f : unlocked(f); + const err = await rejection(syncSkills(opts)); + assert.match( + String(err), + /^Error: \[sync-skills\] .*bestax-b\/linked\.md is a symbolic link\. Commit the real file or directory\.$/ + ); + // Tagged, and still coded as a refusal, with the walk's own error kept. + assert.equal(err.code, SKILL_REFUSAL); + assert.equal(err.cause.code, SKILL_REFUSAL); + assert.equal(failureText(err), err.message); + assert.ok(!existsSync(f.dest), locked ? 'locked run copied' : 'copied'); + await assert.rejects(skillFiles(join(f.src, 'bestax-b')), /symbolic link/); + } +}); + +test('the failure text follows one signal, the refusal code', () => { + const refusal = skillRefusal('[sync-skills] no skills found in /x'); + assert.equal(refusal.code, 'ESKILLREFUSAL'); + assert.equal(failureText(refusal), refusal.message); + // The tag alone no longer counts. Only the code does. + const tagged = new Error('[sync-skills] looks like a refusal'); + assert.equal(failureText(tagged), tagged.stack); + const crash = new TypeError('boom'); + assert.equal(failureText(crash), crash.stack); + assert.match(failureText(crash), /TypeError: boom\n\s+at /); + assert.equal(failureText({ message: 'no stack' }), 'no stack'); + assert.equal(failureText('plain'), 'plain'); +}); + +test('a symlinked file at the top of skills/ is not listed as a skill', async t => { + const f = fixture(t); + symlinkSync(join(f.src, 'README.md'), join(f.src, 'AGENTS.md')); + symlinkSync(join(f.src, 'gone'), join(f.src, 'dangling')); + const names = (await readSkillDirs(f.src)).map(d => d.name); + assert.deepEqual(names, ['bestax-a', 'bestax-b', 'notes']); + // Nothing under a skill is a link, so the sync has nothing to refuse. + await syncSkills(unlocked(f)); + assert.deepEqual(f.lines, ['[sync-skills] copied 2 skills -> out']); +}); + +test('a link added while waiting on the lock is refused, not copied', async t => { + const f = fixture(t); + const lockDir = join(f.stateDir, 'lock'); + mkdirSync(lockDir, { recursive: true }); + const run = rejection( + syncSkills({ ...f, label: 'out', lock: { pollMs: 5, timeoutMs: 5000 } }) + ); + // The signal handlers go in just before the run starts waiting. + while (!process.listenerCount('SIGTERM')) await sleep(1); + symlinkSync( + join(f.src, 'bestax-a', 'SKILL.md'), + join(f.src, 'bestax-b', 'late.md') + ); + rmSync(lockDir, { recursive: true }); + const err = await run; + assert.match(err.message, /bestax-b\/late\.md is a symbolic link/); + assert.equal(err.code, SKILL_REFUSAL); + assert.ok(!existsSync(f.dest), 'the late link was copied'); + assert.ok(!existsSync(join(f.stateDir, 'fingerprint')), 'stamped anyway'); + assert.ok(!existsSync(lockDir), 'the lock was kept'); +}); + +test('the copy ships exactly the vetted list, not what is on disk by then', async t => { + const f = fixture(t); + // Both appear after the walk, so neither is in the list it produced. + writeFileSync(join(f.src, 'bestax-a', 'late.md'), 'late\n'); + symlinkSync( + join(f.src, 'bestax-a', 'SKILL.md'), + join(f.src, 'bestax-b', 'late-link.md') + ); + mkdirSync(join(f.dest, 'bestax-stale'), { recursive: true }); + await copySkills(f.dest, [ + { + name: 'bestax-a', + dir: join(f.src, 'bestax-a'), + files: ['SKILL.md', 'references/x.md'], + }, + { name: 'bestax-b', dir: join(f.src, 'bestax-b'), files: ['SKILL.md'] }, + ]); + assert.deepEqual(tree(f.dest), [ + 'bestax-a', + 'bestax-a/SKILL.md', + 'bestax-a/references', + 'bestax-a/references/x.md', + 'bestax-b', + 'bestax-b/SKILL.md', + ]); + assert.equal( + readFileSync(join(f.dest, 'bestax-a/references/x.md'), 'utf8'), + '# x\n' + ); +}); + +test('a symlinked skill directory is listed, then refused rather than skipped', async t => { + for (const locked of [false, true]) { + const f = fixture(t); + const outside = join(f.root, 'outside-skill'); + mkdirSync(outside); + writeFileSync(join(outside, 'SKILL.md'), '---\nname: bestax-c\n---\n'); + symlinkSync(outside, join(f.src, 'bestax-c')); + const dirs = await readSkillDirs(f.src); + assert.deepEqual( + dirs.find(d => d.name === 'bestax-c'), + { name: 'bestax-c', hasSkillFile: true, isSymlink: true }, + 'the linked skill is not silently dropped' + ); + assert.equal(dirs.find(d => d.name === 'bestax-a').isSymlink, false); + await assert.rejects( + syncSkills(locked ? f : unlocked(f)), + /^Error: \[sync-skills\] .*bestax-c is a symbolic link\. Commit the real directory\.$/ + ); + assert.ok(!existsSync(f.dest), 'copied anyway'); + } +}); + +test('a symlink named .DS_Store is skipped, and never copied', async t => { + for (const locked of [false, true]) { + const f = fixture(t); + // `ln -s /dev/null .DS_Store` is how people stop Finder writing one. + symlinkSync('/dev/null', join(f.src, 'bestax-b', '.DS_Store')); + symlinkSync( + join(f.src, 'bestax-a', 'SKILL.md'), + join(f.src, 'bestax-a', '.DS_Store') + ); + assert.deepEqual(await skillFiles(join(f.src, 'bestax-b')), ['SKILL.md']); + await syncSkills(locked ? { ...f, label: 'out' } : unlocked(f)); + assert.deepEqual(f.lines, ['[sync-skills] copied 2 skills -> out']); + assert.deepEqual(tree(f.dest), [ + 'bestax-a', + 'bestax-a/SKILL.md', + 'bestax-a/references', + 'bestax-a/references/x.md', + 'bestax-b', + 'bestax-b/SKILL.md', + ]); + } +}); + +test('an unexpected error is not tagged as a refusal and keeps its stack', async t => { + if (process.getuid?.() === 0) return t.skip('root ignores file modes'); + const f = fixture(t); + const locked = join(f.src, 'bestax-a', 'references'); + chmodSync(locked, 0o000); + // Restored here, not in t.after: the fixture's cleanup hook runs first and + // could not remove a directory it cannot read. + let err; + try { + err = await syncSkills(unlocked(f)).then( + () => assert.fail('resolved'), + e => e + ); + } finally { + chmodSync(locked, 0o755); + } + assert.equal(err.code, 'EACCES'); + assert.doesNotMatch(err.message, /^\[sync-skills\]/); + assert.equal(failureText(err), err.stack); +}); + +test('a committed symlink in a real repository is refused as a link', async t => { + const f = fixture(t); + const git = (...args) => + execFileSync( + 'git', + [ + '-C', + f.root, + '-c', + 'user.name=t', + '-c', + 'user.email=t@t', + '-c', + 'commit.gpgsign=false', + ...args, + ], + { stdio: 'pipe' } + ); + symlinkSync('SKILL.md', join(f.src, 'bestax-b', 'alias.md')); + git('init', '-q'); + git('add', '-A'); + git('commit', '-q', '-m', 'fixture'); + await assert.rejects( + syncSkills(unlocked(f)), + /^Error: \[sync-skills\] .*bestax-b\/alias\.md is a symbolic link\. Commit the real file or directory\.$/ + ); +});