From a2787c948053cb4f402020d6c00c3241e74dc997 Mon Sep 17 00:00:00 2001 From: miga-heygen Date: Sun, 13 Sep 2026 03:25:10 +0000 Subject: [PATCH] fix(lint): fail skill lint on unresolved doc cross-references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/lint-skills.ts now checks every *.md under skills/, .claude/skills/ and .agents/skills/ for cross-references that do not resolve: inline links [text](target), reference definitions [id]: target, and backticked paths that are explicitly relative (./ or ../) and end in .md or .html. Targets resolve against the referencing file's directory and must exist; a #anchor on a Markdown target must match a GitHub-style heading slug of that file, and same-file #anchor links are checked against the file's own headings. Fenced code blocks, URI-scheme targets, absolute paths and placeholder targets ({ } * $ < >) are skipped. Bare backticked paths without ./ or ../ are a documented blind spot: on the current tree they are mostly skill-root shorthand, another skill's file, or a runtime artifact, and checking them produced ~200 hits that are not wrong. The former top-level main now runs only when the script is the entry point, so importing the exported checkers from the test file no longer exits the test runner when the tree is red. On the current tree the rule reports 34 violations across 16 files, all of the shape described in #3919 (../ one level too shallow for the referencing file, or a file that does not exist). The docs are corrected separately. Refs #3919 Co-Authored-By: Miguel Ángel --- scripts/lint-skills.test.mjs | 177 ++++++++++++++++++- scripts/lint-skills.ts | 317 +++++++++++++++++++++++++++++++---- 2 files changed, 453 insertions(+), 41 deletions(-) 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(); +}