fix(lint): fail skill lint on unresolved doc cross-references (#3922)

* fix(lint): fail skill lint on unresolved doc cross-references 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 <miguel.sierra@heygen.com> * refactor(lint): trim the cross-reference rule's comments and inline its reporter * docs(lint): cite placeholders, not invented paths, in cross-reference comments * refactor(lint): keep the skill lint entry point under the complexity limit --------- Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com>
2026-10-30 09:16:55 +00:00 · 2026-10-02 08:20:12 -07:00
parent 9a4b2a2d74
commit e8ff5ca8c2
2 changed files with 412 additions and 46 deletions
@@ -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,157 @@ 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: real temp files, since the rule asks the filesystem. Layout: skill/SKILL.md (headings
+// "# Setup", "## Providers" twice), skill/references/a.md (the file under test), 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"), "<html></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/<id>.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", () => {
+  // A bare `references/<name>.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 the cross-reference note in lint-skills.ts.
+  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"]);
+});
@@ -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 = /<!--\s*registry-items:\s*(?:allow=([^\s]*))?\s*-->/;
+// 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<string> {
@@ -311,7 +315,7 @@ export function lintRegistryItemRefs(content: string, known: Set<string>): 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,187 @@ export function lintRegistryItemRefs(content: string, known: Set<string>): LineV
  });
 }

+// Cross-references between skill docs: relative link targets, and backticked `./` or `../` .md/.html paths, must
+// resolve from the referencing file; `#anchor`s into .md files must match a GitHub heading slug. Bare backticked
+// paths like `references/<name>.md` are skill-root shorthand and deliberately unchecked.
+
+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: a link whose text is the backticked target 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<string, number>, 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<string> {
+  const seen = new Map<string, number>();
+  const slugs = new Set<string>();
+  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 +528,71 @@ 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);
-}
+type Report = (file: string, violations: LineViolation[]) => void;

-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++;
-  }
-}
-
-for (const file of files) {
-  report(relative(process.cwd(), file), lintFile(file));
-}
-
-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<string>, report: Report): number {
+  let snapshotsChecked = 0;
+  for (const path of paths) {
+    const content = readFileSync(path, "utf-8");
+    const file = relative(process.cwd(), path);
+    report(file, lintDocRefs(path, content));
+    const found = lintRegistryItemRefs(content, knownItems);
    if (found === null) continue;
    snapshotsChecked++;
-    report(relative(process.cwd(), path), found);
+    report(file, found);
+  }
+  return snapshotsChecked;
+}
+
+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);
+  }
+
+  let totalViolations = 0;
+  const report: Report = (file, violations) => {
+    for (const v of violations) {
+      console.error(`${file}:${v.line}: ${v.message}`);
+      console.error(`  ${v.text}\n`);
+    }
+    totalViolations += violations.length;
+  };
+
+  for (const file of files) {
+    report(relative(process.cwd(), file), lintFile(file));
+  }
+
+  const knownItems = registryItemNames();
+  const markdownFiles = skillsDirs.flatMap(collectMarkdownFiles);
+  const snapshotsChecked = lintMarkdownFiles(markdownFiles, knownItems, report);
+
+  if (totalViolations > 0) {
+    console.error(`\n${totalViolations} skill lint error(s) found.`);
+    process.exit(1);
+  }
+  console.log(
+    `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.`,
+  );
+}
+
+// Tests import the checkers, so main() runs only as the entry point. argv[1] is realpath'd to match import.meta.url,
+// which the ESM loader realpaths; otherwise a symlinked checkout would skip main() and exit 0.
+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 (totalViolations > 0) {
-  console.error(`\n${totalViolations} skill lint error(s) found.`);
-  process.exit(1);
-} else {
-  console.log(
-    `Checked ${files.length} skill file(s) and ${snapshotsChecked} registry snapshot(s) against ${knownItems.size} registry items — no issues found.`,
-  );
+if (isEntryPoint()) {
+  main();
 }