diff --git a/scripts/lint-skills.test.mjs b/scripts/lint-skills.test.mjs index e41b4e4e27..91c2b31d29 100644 --- a/scripts/lint-skills.test.mjs +++ b/scripts/lint-skills.test.mjs @@ -1,14 +1,17 @@ -// Positive / negative fixture tests for the SKILL.md frontmatter drift guard -// in scripts/lint-skills.ts. Runs the exported `lintFrontmatter` against known -// inputs and asserts the violation set matches expectation. +// Positive / negative fixture tests for the checkers exported by +// scripts/lint-skills.ts: SKILL.md frontmatter shape, registry-snapshot item +// refs, and doc cross-references. Each runs against known inputs and asserts +// the violation set matches expectation. // // Kept in .mjs (not .ts) so `node --test` can execute it via the same runner -// the rest of scripts/*.test.mjs use, without needing tsx. `bun scripts/…` -// runs .ts directly at lint-time; tests import the compiled export via tsx. +// the rest of scripts/*.test.mjs use; the .ts import is loaded through tsx. import test from "node:test"; import assert from "node:assert/strict"; -import { lintFrontmatter, lintRegistryItemRefs } from "./lint-skills.ts"; +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { headingSlugs, lintDocRefs, lintFrontmatter, lintRegistryItemRefs } from "./lint-skills.ts"; const wrap = (frontmatter) => `---\n${frontmatter}\n---\n\n# body\n`; @@ -180,3 +183,165 @@ test("registry refs: single-word ids are a KNOWN blind spot, not an accident", ( const doc = `${MARKER}\n\n\`glitch\` was renamed and this doc was not updated.\n`; assert.deepEqual(lintRegistryItemRefs(doc, KNOWN), []); }); + +// --------------------------------------------------------------------------- +// Cross-references between skill docs +// --------------------------------------------------------------------------- +// +// Fixtures are real files in a temp directory because the rule's whole job is +// to ask the filesystem whether a target exists. Layout: +// +// /skill/SKILL.md "# Setup", "## Providers", "## Providers" +// /skill/references/a.md <- the file under test (returned path) +// /skill/examples/demo.html + +function refFixture() { + const root = mkdtempSync(join(tmpdir(), "lint-skills-refs-")); + mkdirSync(join(root, "skill", "references"), { recursive: true }); + mkdirSync(join(root, "skill", "examples"), { recursive: true }); + writeFileSync( + join(root, "skill", "SKILL.md"), + "---\nname: s\ndescription: d\n---\n\n# Setup\n\n## Providers\n\n## Providers\n", + ); + writeFileSync(join(root, "skill", "examples", "demo.html"), "\n"); + return join(root, "skill", "references", "a.md"); +} + +test("doc refs: relative link to an existing file passes", () => { + const file = refFixture(); + assert.deepEqual( + lintDocRefs(file, "See [setup](../SKILL.md) and `../examples/demo.html`.\n"), + [], + ); +}); + +test("doc refs: link to a missing file is a violation naming the target", () => { + const file = refFixture(); + const violations = lintDocRefs(file, "# Doc\n\nRead [this](../references/missing.md).\n"); + assert.equal(violations.length, 1); + assert.equal(violations[0].line, 3); + assert.ok(violations[0].message.includes("../references/missing.md")); + assert.ok(violations[0].message.includes("does not exist")); +}); + +test("doc refs: path climbing above the skill root is a violation", () => { + const file = refFixture(); + // Target exists one level up; the doc climbs two. This is the shape most of + // the dead references on the tree had. + const violations = lintDocRefs(file, "Open `../../examples/demo.html` for the full build.\n"); + assert.equal(violations.length, 1); + assert.ok(violations[0].message.includes("../../examples/demo.html")); +}); + +test("doc refs: anchor must match a heading slug in the target", () => { + const file = refFixture(); + assert.deepEqual( + lintDocRefs(file, "[ok](../SKILL.md#setup) [dup](../SKILL.md#providers-1)\n"), + [], + ); + const violations = lintDocRefs(file, "See [preflight](../SKILL.md#preflight).\n"); + assert.equal(violations.length, 1); + assert.ok(violations[0].message.includes("#preflight")); + assert.ok(violations[0].message.includes("SKILL.md")); +}); + +test("doc refs: same-file anchor is checked against the file's own headings", () => { + const file = refFixture(); + const doc = "# Intro\n\n## Deep Dive: Part 2!\n\n[a](#deep-dive-part-2) [b](#nope)\n"; + const violations = lintDocRefs(file, doc); + assert.equal(violations.length, 1); + assert.ok(violations[0].message.includes("#nope")); +}); + +test("doc refs: links inside fenced code blocks are ignored", () => { + const file = refFixture(); + const doc = ["```md", "[x](./gone.md)", "`../gone.md`", "```", ""].join("\n"); + assert.deepEqual(lintDocRefs(file, doc), []); +}); + +test("doc refs: URLs, mailto, absolute paths and placeholders are ignored", () => { + const file = refFixture(); + const doc = [ + "[a](https://example.com/x.md) [b](http://example.com) [c](mailto:x@y.z)", + "[d](/etc/hosts) `../rules/.md` [e](../{slug}.md)", + "", + ].join("\n"); + assert.deepEqual(lintDocRefs(file, doc), []); +}); + +test("doc refs: backticked relative .md path to a missing file is a violation", () => { + const file = refFixture(); + const violations = lintDocRefs(file, "Follow `../references/cut-catalog.md` first.\n"); + assert.equal(violations.length, 1); + assert.ok(violations[0].message.includes("../references/cut-catalog.md")); +}); + +test("doc refs: bare backticked paths are a KNOWN blind spot, not an accident", () => { + // `references/foo.md` without a leading ./ or ../ is skill-root shorthand, + // another skill's file, or a runtime artifact more often than a file-relative + // path. Pinned so the tradeoff is visible in code. See lint-skills.ts header. + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "Read `references/does-not-exist.md`.\n"), []); +}); + +test("doc refs: reference-style definitions are checked, footnotes are not", () => { + const file = refFixture(); + assert.deepEqual( + lintDocRefs(file, "[setup]: ../SKILL.md#setup\n[^1]: a footnote, not a path\n"), + [], + ); + const violations = lintDocRefs(file, '[gone]: <../gone.md> "Title"\n'); + assert.equal(violations.length, 1); + assert.ok(violations[0].message.includes("../gone.md")); +}); + +test("doc refs: prose that merely starts with [Label]: is not a definition", () => { + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "[Label]: describes the thing, not a path\n"), []); +}); + +test("doc refs: a bare # links to the top of the target, not a heading", () => { + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "[top](#) [skill](../SKILL.md#)\n"), []); +}); + +test("doc refs: a target named twice on one line is reported once", () => { + const file = refFixture(); + assert.equal(lintDocRefs(file, "[`../gone.md`](../gone.md)\n").length, 1); +}); + +test("doc refs: a stray ](target) without link text is not a link", () => { + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "see the table ](../gone.md) above\n"), []); +}); + +test("doc refs: an existing directory target passes, with or without a fragment", () => { + const file = refFixture(); + assert.deepEqual( + lintDocRefs(file, "[ex](../examples) [ex2](../examples/) [ex3](../examples#x)\n"), + [], + ); +}); + +test("doc refs: a directory whose name ends in .md is not anchor-checked", () => { + const file = refFixture(); + mkdirSync(join(file, "..", "..", "notes.md")); + assert.deepEqual(lintDocRefs(file, "[n](../notes.md#anything)\n"), []); +}); + +test("doc refs: fragments on non-Markdown targets are not anchor-checked", () => { + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "[demo](../examples/demo.html#any-id)\n"), []); +}); + +test("doc refs: link syntax inside inline code is not a reference", () => { + const file = refFixture(); + assert.deepEqual(lintDocRefs(file, "Write links as `[text](../gone.md)` in prose.\n"), []); +}); + +test("heading slugs: GitHub-style lowercasing, punctuation strip, dedupe", () => { + const slugs = headingSlugs( + "# Hello, World!\n## `code` & Stuff\n## Hello, World!\n```\n# not a heading\n```\n", + ); + assert.deepEqual([...slugs], ["hello-world", "code--stuff", "hello-world-1"]); +}); diff --git a/scripts/lint-skills.ts b/scripts/lint-skills.ts index 10c8847f47..41ba8095b6 100644 --- a/scripts/lint-skills.ts +++ b/scripts/lint-skills.ts @@ -9,8 +9,9 @@ * Unsafe: `!` followed by `>` later in the same text block */ -import { readFileSync, readdirSync, statSync } from "node:fs"; -import { join, relative } from "node:path"; +import { readFileSync, readdirSync, realpathSync, statSync } from "node:fs"; +import { dirname, join, relative, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; import { parse as parseYaml, YAMLParseError } from "yaml"; import type { RegistryManifest } from "../packages/core/src/index.js"; @@ -288,6 +289,9 @@ function lintInlinePatterns(file: string, stripped: string): Violation[] { // ("add", "line", "name", "height", "text"). A 15:1 noise ratio is how a // check gets switched off, so the hyphen requirement stays. const REGISTRY_MARKER = //; +// Shared by matchAll (registry rule) and replace (doc-ref rule); both leave +// lastIndex at 0. Never call .exec/.test on it — that would poison matchAll. +const INLINE_CODE_SPAN = /`([^`\n]+)`/g; const REGISTRY_ITEM_ID = /^[a-z][a-z0-9]*(?:-[a-z0-9]+)+$/; function registryItemNames(): Set { @@ -311,7 +315,7 @@ export function lintRegistryItemRefs(content: string, known: Set): LineV if (!marker) return null; const allowed = new Set((marker[1] ?? "").split(",").filter(Boolean)); return content.split("\n").flatMap((line, index) => { - const dead = [...new Set([...line.matchAll(/`([^`\n]+)`/g)].map((m) => (m[1] ?? "").trim()))] + const dead = [...new Set([...line.matchAll(INLINE_CODE_SPAN)].map((m) => (m[1] ?? "").trim()))] .filter((token) => REGISTRY_ITEM_ID.test(token)) .filter((token) => !known.has(token) && !allowed.has(token)); return dead.map((token) => @@ -324,6 +328,214 @@ export function lintRegistryItemRefs(content: string, known: Set): LineV }); } +// --------------------------------------------------------------------------- +// Cross-references between skill docs +// --------------------------------------------------------------------------- +// +// Skill docs point at each other with relative paths: `[text](../foo.md)`, +// `[id]: ../foo.md`, or a backticked `../foo.md` in prose. Nothing used to fail +// when such a path was wrong — typically `../` climbing one level too far from +// a `references/` or `sub-agents/` subdirectory, or a file that moved — and an +// agent following the doc hit a read error and improvised. This check resolves +// every checked target against the referencing file's own directory and +// requires it to exist. When the target carries a `#anchor` and is a Markdown +// file, the anchor must match a GitHub-style heading slug in that file +// (lowercase, punctuation stripped, spaces → `-`, duplicates suffixed `-1`, +// `-2`, …). Same-file `#anchor` links are checked against the file itself. +// +// Skipped on purpose: URLs with a scheme (`https:`, `mailto:`, …), absolute +// paths, anything inside a fenced code block, and template-ish targets that +// carry `{`, `}`, `*`, `$`, `<`, or `>` (`rules/.md`). +// +// Markdown links and reference definitions have one resolution rule (relative +// to the file), so every relative target is checked. Backticked prose paths +// are checked only when they are explicitly relative (`./` or `../`) and point +// at a doc or example composition (`.md` / `.html`). A leading `./` or `../` +// is an unambiguous statement of "relative to this file", and a doc that uses +// it to mean "relative to the skill root" is exactly the defect this rule +// exists to catch. Bare backticked paths (`references/foo.md`) are NOT +// checked: measured on the current tree, most are skill-root shorthand, name a +// file in another skill, or describe a project artifact the skill writes at +// runtime (`assets/index.md`), and checking them would flag roughly 200 lines +// that are not wrong. That is a KNOWN false-negative blind spot, deliberately. + +interface DocRef { + line: number; + target: string; + text: string; +} + +const INLINE_LINK = /\[[^\]\n]*\]\(\s*(<[^>\n]*>|[^)\s]+)(?:\s+(?:"[^"]*"|'[^']*'))?\s*\)/g; +// CommonMark: `[id]: dest` optionally followed by a quoted title and nothing +// else, so a prose line like `[Label]: describes the thing` is not a definition. +const REFERENCE_DEFINITION = + /^ {0,3}\[([^\]^][^\]]*)\]:\s*(<[^>\n]*>|\S+)(?:\s+(?:"[^"]*"|'[^']*'|\([^)\n]*\)))?\s*$/; +const BACKTICK_RELATIVE_PATH = /`(\.\.?\/[^`\s]+\.(?:md|html)(?:#[^`\s]*)?)`/g; +const HAS_URI_SCHEME = /^[a-zA-Z][a-zA-Z0-9+.-]*:/; +const PLACEHOLDER_CHARS = /[{}*$<>]/; +const ATX_HEADING = /^ {0,3}#{1,6}\s+(.*?)(?:\s+#+)?\s*$/; + +function unwrapTarget(raw: string): string { + return raw.startsWith("<") && raw.endsWith(">") ? raw.slice(1, -1) : raw; +} + +function backtickTargets(line: string): string[] { + return [...line.matchAll(BACKTICK_RELATIVE_PATH)].map((m) => m[1] ?? ""); +} + +function proseTargets(prose: string): string[] { + const definition = REFERENCE_DEFINITION.exec(prose); + const links = [...prose.matchAll(INLINE_LINK)].map((m) => unwrapTarget(m[1] ?? "")); + return definition ? [unwrapTarget(definition[2] ?? ""), ...links] : links; +} + +function docRefsInLine(line: string, lineNumber: number): DocRef[] { + // Inline code is stripped before scanning for link syntax so a doc that + // *documents* `[text](path)` is not read as linking to `path`. + const prose = line.replace(INLINE_CODE_SPAN, ""); + // One violation per dead target per line: [`../x.md`](../x.md) names it twice. + const targets = new Set([...backtickTargets(line), ...proseTargets(prose)]); + const text = line.trim(); + return [...targets].map((target) => ({ line: lineNumber, target, text })); +} + +function isCheckableTarget(target: string): boolean { + return ( + target.length > 0 && + !HAS_URI_SCHEME.test(target) && + !target.startsWith("/") && + !PLACEHOLDER_CHARS.test(target) + ); +} + +function slugify(heading: string): string { + return heading + .trim() + .toLowerCase() + .replace(/[^\p{L}\p{N}\s_-]/gu, "") + .replace(/\s/g, "-"); +} + +function dedupedSlug(seen: Map, base: string): string { + const count = seen.get(base) ?? 0; + seen.set(base, count + 1); + return count === 0 ? base : `${base}-${count}`; +} + +/** GitHub-style anchor slugs for every ATX heading outside fenced blocks. */ +export function headingSlugs(content: string): Set { + const seen = new Map(); + const slugs = new Set(); + for (const line of stripFencedBlocks(content).split("\n")) { + const heading = ATX_HEADING.exec(line); + if (heading) slugs.add(dedupedSlug(seen, slugify(heading[1] ?? ""))); + } + return slugs; +} + +function decodeTarget(target: string): string { + try { + return decodeURIComponent(target); + } catch { + return target; + } +} + +function anchorViolation( + ref: DocRef, + anchor: string, + targetLabel: string, + targetContent: string, +): LineViolation | null { + if (headingSlugs(targetContent).has(decodeTarget(anchor).toLowerCase())) return null; + return violation( + ref.line, + `Anchor "#${anchor}" does not match any heading in ${targetLabel}.`, + ref.text, + ); +} + +function splitAnchor(target: string): { path: string; anchor: string | null } { + const hash = target.indexOf("#"); + // A bare trailing `#` links to the top of the target, not to a heading. + if (hash === -1 || hash === target.length - 1) { + return { path: target.replace(/#$/, ""), anchor: null }; + } + return { path: target.slice(0, hash), anchor: target.slice(hash + 1) }; +} + +type TargetRead = { kind: "missing" } | { kind: "directory" } | { kind: "file"; content: string }; + +const MISSING_TARGET_CODES = new Set(["ENOENT", "ENOTDIR"]); + +function classifyReadError(err: unknown): TargetRead { + const code = (err as NodeJS.ErrnoException).code ?? ""; + if (MISSING_TARGET_CODES.has(code)) return { kind: "missing" }; + if (code === "EISDIR") return { kind: "directory" }; + throw err; +} + +// Read-and-classify in one syscall rather than stat-then-read, so the answer +// cannot change between the check and the read. +function readTarget(resolved: string): TargetRead { + try { + return { kind: "file", content: readFileSync(resolved, "utf-8") }; + } catch (err) { + return classifyReadError(err); + } +} + +function missingViolation(ref: DocRef, label: string): LineViolation { + return violation( + ref.line, + `Cross-reference "${ref.target}" does not resolve: ${label} does not exist.`, + ref.text, + ); +} + +function anchoredTargetViolation( + ref: DocRef, + anchor: string, + resolved: string, + label: string, +): LineViolation | null { + const target = readTarget(resolved); + if (target.kind === "missing") return missingViolation(ref, label); + // A directory has no headings; existence is all that can be checked. + if (target.kind === "directory") return null; + return anchorViolation(ref, anchor, label, target.content); +} + +function checkTargetPath( + ref: DocRef, + resolved: string, + anchor: string | null, +): LineViolation | null { + const label = relative(REPO_ROOT, resolved); + // Only Markdown targets have heading slugs; an `.html#id` fragment is not checked. + if (anchor === null || !resolved.endsWith(".md")) { + return statSync(resolved, { throwIfNoEntry: false }) ? null : missingViolation(ref, label); + } + return anchoredTargetViolation(ref, anchor, resolved, label); +} + +function checkDocRef(ref: DocRef, filePath: string, content: string): LineViolation | null { + const { path, anchor } = splitAnchor(ref.target); + if (path.length === 0) { + return anchor === null ? null : anchorViolation(ref, anchor, "this file", content); + } + return checkTargetPath(ref, resolve(dirname(filePath), decodeTarget(path)), anchor); +} + +/** Violations for relative cross-references in `content` (located at `filePath`) that do not resolve. */ +export function lintDocRefs(filePath: string, content: string): LineViolation[] { + return stripFencedBlocks(content) + .split("\n") + .flatMap((line, index) => docRefsInLine(line, index + 1)) + .filter((ref) => isCheckableTarget(ref.target)) + .flatMap((ref) => checkDocRef(ref, filePath, content) ?? []); +} + function collectMarkdownFiles(dir: string): string[] { return readdirSync(dir, { withFileTypes: true, recursive: true }) .filter((entry) => entry.isFile() && entry.name.endsWith(".md")) @@ -343,47 +555,82 @@ function lintFile(filePath: string): Violation[] { // Main // --------------------------------------------------------------------------- -const files: string[] = []; -for (const dir of SKILLS_DIRS) { - if (!statSync(dir, { throwIfNoEntry: false })?.isDirectory()) continue; - files.push(...collectSkillFiles(dir)); -} -if (files.length === 0) { - console.log("No SKILL.md files found across skills/, .claude/skills/, .agents/skills/."); - process.exit(0); -} - -let totalViolations = 0; - -function report(file: string, violations: LineViolation[]): void { - for (const v of violations) { - console.error(`${file}:${v.line}: ${v.message}`); - console.error(` ${v.text}\n`); - totalViolations++; - } +interface Reporter { + report: (file: string, violations: LineViolation[]) => void; + total: () => number; } -for (const file of files) { - report(relative(process.cwd(), file), lintFile(file)); +function createReporter(): Reporter { + let total = 0; + return { + report(file, violations) { + for (const v of violations) { + console.error(`${file}:${v.line}: ${v.message}`); + console.error(` ${v.text}\n`); + total++; + } + }, + total: () => total, + }; } -const knownItems = registryItemNames(); -let snapshotsChecked = 0; -for (const dir of SKILLS_DIRS) { - if (!statSync(dir, { throwIfNoEntry: false })?.isDirectory()) continue; - for (const path of collectMarkdownFiles(dir)) { - const found = lintRegistryItemRefs(readFileSync(path, "utf-8"), knownItems); +/** Doc cross-references and registry snapshots for every markdown file; returns the snapshot count. */ +function lintMarkdownFiles(paths: string[], knownItems: Set, reporter: Reporter): number { + let snapshotsChecked = 0; + for (const path of paths) { + const content = readFileSync(path, "utf-8"); + const file = relative(process.cwd(), path); + reporter.report(file, lintDocRefs(path, content)); + const found = lintRegistryItemRefs(content, knownItems); if (found === null) continue; snapshotsChecked++; - report(relative(process.cwd(), path), found); + reporter.report(file, found); } + return snapshotsChecked; } -if (totalViolations > 0) { - console.error(`\n${totalViolations} skill lint error(s) found.`); - process.exit(1); -} else { +function main(): void { + const skillsDirs = SKILLS_DIRS.filter((dir) => + statSync(dir, { throwIfNoEntry: false })?.isDirectory(), + ); + const files = skillsDirs.flatMap(collectSkillFiles); + if (files.length === 0) { + console.log("No SKILL.md files found across skills/, .claude/skills/, .agents/skills/."); + process.exit(0); + } + + const reporter = createReporter(); + for (const file of files) { + reporter.report(relative(process.cwd(), file), lintFile(file)); + } + + const knownItems = registryItemNames(); + const markdownFiles = skillsDirs.flatMap(collectMarkdownFiles); + const snapshotsChecked = lintMarkdownFiles(markdownFiles, knownItems, reporter); + + if (reporter.total() > 0) { + console.error(`\n${reporter.total()} skill lint error(s) found.`); + process.exit(1); + } console.log( - `Checked ${files.length} skill file(s) and ${snapshotsChecked} registry snapshot(s) against ${knownItems.size} registry items — no issues found.`, + `Checked ${files.length} skill file(s), cross-references in ${markdownFiles.length} markdown file(s), and ${snapshotsChecked} registry snapshot(s) against ${knownItems.size} registry items — no issues found.`, ); } + +// Only run when executed directly (`tsx scripts/lint-skills.ts`). The test file +// imports the exported checkers, and a red tree must not exit the test runner. +// argv[1] is realpath'd because the ESM loader realpaths import.meta.url; without +// it a symlinked checkout (macOS /tmp) would skip main() and exit 0 silently. +function isEntryPoint(): boolean { + const entry = process.argv[1]; + if (!entry) return false; + try { + return realpathSync(resolve(entry)) === fileURLToPath(import.meta.url); + } catch { + return false; + } +} + +if (isEntryPoint()) { + main(); +}