diff --git a/.github/workflows/comments.yml b/.github/workflows/comments.yml new file mode 100644 index 0000000000..e6cd870f34 --- /dev/null +++ b/.github/workflows/comments.yml @@ -0,0 +1,40 @@ +name: Comments + +permissions: + contents: read + +# No secrets and a read-only token, so fork PRs run it too. Rules: CONTRIBUTING.md, "Comments". +on: + pull_request: + types: [opened, synchronize, reopened] + +concurrency: + group: comments-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + comments: + name: Comments + # A release PR carries commits that each passed this gate on their own PR. + if: ${{ !startsWith(github.head_ref, 'release/') }} + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + # The PR's own head, graded against its merge-base: not the merge preview, which would + # charge the branch for every comment main changed since it forked. + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 22 + # The commented-out-code rule asks the TypeScript parser, a root dependency. + - run: bash scripts/ci/install-workspace-dependencies.sh --ignore-scripts + - name: Test the checker + run: node --test scripts/check-comment-citations.test.mjs + - name: Check the comments this diff touched + env: + COMMENT_CHECK_BASE: ${{ github.event.pull_request.base.sha }} + run: node scripts/check-comment-citations.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8c63c8d085..82401f9a71 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -69,6 +69,19 @@ If you must add a cast, add a comment: const event = data as unknown as RuntimeEvent; ``` +#### Comments + +A comment says what the code cannot: the reason, the invariant, the non-obvious constraint. Names, types and tests carry the rest, and reasoning or history goes in the PR description, where it stays attached to the change. + +The `Comments` check (`scripts/check-comment-citations.mjs`) grades the comments a PR adds or edits: + +- **Citations must resolve.** A backticked path, a `path:line`, a backticked camelCase symbol, or "pinned by" / "covered by" / "see" plus a test file must point at something in the repo. A comment that names its source reads as evidence, so a stale one sends the next reader to a dead end. +- **No history.** "used to", "previously", "was removed", "before this change", "PR #123" describe the past, which git already records. State what is true now. +- **No commented-out code.** Delete it; git keeps it. +- **No block over 40 lines.** Cut it to the why and the invariant. A block that must stay whole (a licence, a diagram, a protocol table) starts with `comment-length: `. + +Only comment blocks holding a line your PR added can fail. Findings elsewhere in a file you touched are printed as warnings, and fixing one while you are there is welcome. To grade files by hand, pass their paths: `node scripts/check-comment-citations.mjs path/to/file.ts`. + ## Adding Registry Items (Blocks & Components) The registry at `registry/` contains reusable items installable via `hyperframes add `. Each item lives in its own directory under `registry/blocks/` or `registry/components/`. diff --git a/package.json b/package.json index 9b599711f1..a658844d94 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "player:perf": "bun run --filter @hyperframes/player perf", "format:check": "oxfmt --check .", "knip": "knip", - "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-registry-set-delta.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-pr-captures.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-media-use-copy-parity.test.mjs scripts/check-svg-sanitize-parity.test.mjs scripts/check-media-use-svg-sanitize-generated.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/pr-edit-concurrency.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts scripts/catalog-publication.test.mjs scripts/ci/resolve-workflow-pr.test.mjs scripts/check-catalog-source-pr.test.mjs scripts/generate-registry-items.test.ts scripts/catalog-drift.test.ts scripts/catalog-fetch-mirror.test.ts scripts/catalog-script-inlining.test.ts scripts/catalog-detail.test.ts scripts/generate-catalog-pages.test.ts scripts/verify-catalog-payloads.test.ts scripts/registry-skill-files.test.ts scripts/creator-editing-capabilities.test.mjs scripts/generate-catalog-previews.test.ts scripts/registry-primitive-payloads.test.ts registry/components/pan-stations/pan-stations.test.mjs && vitest run scripts/catalog/ scripts/contrastRatchet.test.ts scripts/generate-catalog-payloads.test.ts", + "test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-registry-set-delta.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-pr-captures.test.mjs scripts/check-comment-citations.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-media-use-copy-parity.test.mjs scripts/check-svg-sanitize-parity.test.mjs scripts/check-media-use-svg-sanitize-generated.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/pr-edit-concurrency.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts scripts/catalog-publication.test.mjs scripts/ci/resolve-workflow-pr.test.mjs scripts/check-catalog-source-pr.test.mjs scripts/generate-registry-items.test.ts scripts/catalog-drift.test.ts scripts/catalog-fetch-mirror.test.ts scripts/catalog-script-inlining.test.ts scripts/catalog-detail.test.ts scripts/generate-catalog-pages.test.ts scripts/verify-catalog-payloads.test.ts scripts/registry-skill-files.test.ts scripts/creator-editing-capabilities.test.mjs scripts/generate-catalog-previews.test.ts scripts/registry-primitive-payloads.test.ts registry/components/pan-stations/pan-stations.test.mjs && vitest run scripts/catalog/ scripts/contrastRatchet.test.ts scripts/generate-catalog-payloads.test.ts", "typecheck:scripts": "tsc --noEmit -p scripts/tsconfig.json", "test:skills": "node --test 'skills/**/*.test.mjs' 'packages/cli/src/media-use/**/*.test.mjs'", "generate:previews": "tsx scripts/generate-template-previews.ts", diff --git a/scripts/check-comment-citations.mjs b/scripts/check-comment-citations.mjs new file mode 100644 index 0000000000..6efa237e7a --- /dev/null +++ b/scripts/check-comment-citations.mjs @@ -0,0 +1,840 @@ +#!/usr/bin/env node +// fallow-ignore-file complexity +// Grades the comments of the files a PR changed; the rules and their reasons are in CONTRIBUTING.md. +// Citations (path, path:line, backticked camelCase symbol, "pinned by ") must resolve. +// Blocks the diff touched may not run past 40 lines, narrate history, or be commented-out code. +// Only lines the diff added can fail; the same finding elsewhere in the file is printed, not failed. +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import path from "node:path"; +import ts from "typescript"; + +const SCANNABLE = new Set([".ts", ".tsx", ".mjs", ".js", ".css", ".py", ".md"]); +// The lookahead keeps `ts` from matching inside ".tsx" and `js` inside ".json". +const PATH_EXT = "(?:ts|tsx|js|jsx|mjs|cjs|css|scss|py|md|json|ya?ml|sql|sh|html|txt)(?![\\w])"; +const PATHISH = new RegExp(`^[\\w@./-]*[\\w-]+\\.${PATH_EXT}$`); +const BARE_PATH_LINE = new RegExp(`\\b([\\w@./-]*[\\w-]+\\.${PATH_EXT}):(\\d+)\\b`, "g"); +const TEST_CLAIM = new RegExp( + `\\b(pinned by|asserted in|covered by|see)\\s+\`?([\\w@./-]*[\\w-]+\\.${PATH_EXT})\`?`, + "gi", +); + +// A name cited to say it is absent ("no `danger.js`") must not be resolved. Bound to one sentence, +// reaching back far enough to cover a wrapped three-item list. +const ASSERTS_ABSENCE = + /\b(no|not|never|without|neither|nor|nothing|absent|missing)\b[^.;]{0,120}$/i; + +// A sentence that names another repo or an installed package cites a file this checkout cannot see. +const NAMES_ANOTHER_REPO = /\bheygen-com\/(?!hyperframes\b)[\w.-]+/i; +const NAMES_A_DEPENDENCY = /@[\w.-]+\/[\w.-]+|\b[\w.-]+@\d+\.\d+|\b[\w.-]+\/dist\//; + +// The block rules read code only: a markdown plan describing what the code once did is a record. +const CODE_EXT = new Set([".ts", ".tsx", ".mjs", ".js", ".py"]); +const MAX_BLOCK_LINES = 40; + +// Bounded forms only: bare "was" also matches live conditions ("if the response was truncated"). +const HISTORY_NARRATION = + /\b(used to|previously|formerly|load[- ]bearing|before this (?:change|commit|PR|fix|refactor|rewrite|version|landed)|(?:before|after|in|per) #\d+|PRs? #\d+|was (?:removed|renamed|replaced|moved|split|deleted|introduced|reverted))\b/i; + +// Quoted words are someone else's, not this code's history. The single-quote form needs word +// boundaries and one line, or an apostrophe in "isn't" opens a span that blanks real findings. +const QUOTED_SPAN = /"[^"\n]*"|(?:`, or build output: none of these resolve in the tree. +function namesSomethingElse(cited) { + if (cited.startsWith("/") || cited.startsWith("~")) return true; + if (cited.includes(":")) return true; + return cited.split("/")[0] === "dist"; +} + +/** Of several files that could satisfy one citation, the one sharing the most leading path + * segments with the file doing the citing. */ +function nearestTo(from, candidates) { + if (candidates.length === 0) return null; + const home = from.split("/"); + const shared = (file) => { + const parts = file.split("/"); + let i = 0; + while (i < home.length && i < parts.length && home[i] === parts[i]) i++; + return i; + }; + return candidates.reduce((best, file) => (shared(file) > shared(best) ? file : best)); +} + +/** Blank out string literals so a `//` inside one is not read as a comment, and so a template + * literal's backticks are not read as a citation. Length is preserved to keep offsets honest. */ +function stripStrings(line) { + return line.replace(/(["'`])(?:\\.|(?!\1)[^\\])*\1/g, (m) => " ".repeat(m.length)); +} + +function scanCLike(lines) { + const out = []; + let inBlock = false; + let inTemplate = false; + for (let i = 0; i < lines.length; i++) { + let rest = lines[i]; + let text = ""; + while (rest.length > 0) { + if (inBlock) { + const end = rest.indexOf("*/"); + if (end === -1) { + text += rest; + break; + } + text += rest.slice(0, end); + inBlock = false; + rest = rest.slice(end + 2); + continue; + } + if (inTemplate) { + const end = rest.indexOf("`"); + if (end === -1) break; + inTemplate = false; + rest = rest.slice(end + 1); + continue; + } + // Complete literals are blanked, so a backtick left over opens a multi-line template. + const code = stripStrings(rest); + const marks = [ + [code.indexOf("//"), "line"], + [code.indexOf("/*"), "block"], + [code.indexOf("`"), "template"], + ] + .filter(([at]) => at !== -1) + .sort((a, b) => a[0] - b[0]); + if (marks.length === 0) break; + const [at, kind] = marks[0]; + if (kind === "template") { + inTemplate = true; + rest = rest.slice(at + 1); + continue; + } + if (kind === "block") { + inBlock = true; + rest = rest.slice(at + 2); + continue; + } + if (rest[at - 1] === ":") { + rest = rest.slice(at + 2); // a URL, not a comment + continue; + } + text += rest.slice(at + 2); + break; + } + const cleaned = text.replace(/^\s*\*+ ?/, ""); + if (cleaned.trim()) out.push({ line: i + 1, text: cleaned }); + } + return out; +} + +function scanPython(lines) { + const out = []; + let doc = null; + for (let i = 0; i < lines.length; i++) { + const raw = lines[i]; + if (doc !== null) { + const end = raw.indexOf(doc); + out.push({ line: i + 1, text: end === -1 ? raw : raw.slice(0, end) }); + if (end !== -1) doc = null; + continue; + } + const opener = raw.match(/("""|''')/); + if (opener) { + const rest = raw.slice(opener.index + 3); + const end = rest.indexOf(opener[1]); + out.push({ line: i + 1, text: end === -1 ? rest : rest.slice(0, end) }); + if (end === -1) doc = opener[1]; + continue; + } + const hash = stripStrings(raw).indexOf("#"); + if (hash !== -1) out.push({ line: i + 1, text: raw.slice(hash + 1) }); + } + return out; +} + +/** Markdown is prose all the way down, so every line outside a fence counts as a comment. */ +function scanMarkdown(lines) { + const out = []; + let fence = null; + for (let i = 0; i < lines.length; i++) { + const raw = lines[i]; + const marker = raw.match(/^\s*(```+|~~~+)/); + if (fence !== null) { + if (marker && raw.trim().startsWith(fence)) fence = null; + continue; + } + if (marker) { + fence = marker[1]; + continue; + } + if (raw.trim()) out.push({ line: i + 1, text: raw }); + } + return out; +} + +// A comment is a paragraph: the repo a name belongs to is often named a line or two above it. +function groupComments(comments) { + const blocks = []; + for (const comment of comments) { + const previous = blocks[blocks.length - 1]; + if (previous && comment.line === previous[previous.length - 1].line + 1) previous.push(comment); + else blocks.push([comment]); + } + return blocks; +} + +// Code blocks merge across blank continuation lines, so a symbol cited deep in a docblock +// excludes the whole docblock when its own occurrences are counted. +function withBlockContext(comments, lines, ext) { + const blocks = CODE_EXT.has(ext) + ? mergeAcrossBlankLines(groupComments(comments), lines) + : groupComments(comments); + return blocks.flatMap((block) => { + const context = block.map((comment) => comment.text).join(" "); + const blockStart = block[0].line; + const blockEnd = block.spanEnd ?? block[block.length - 1].line; + // `prefix` is what the paragraph said before this line, so a negation governs a wrapped name. + return block.map((comment, index) => ({ + ...comment, + context, + prefix: block + .slice(0, index) + .map((earlier) => earlier.text) + .join(" "), + blockStart, + blockEnd, + })); + }); +} + +// A blank " *" line inside a docblock is still inside it; the text scan drops it, so length +// measures the physical span by stepping over these. +const BLANK_CONTINUATION = /^\s*(?:\/\/+|\*|\/\*\*?)?\s*$/; + +function mergeAcrossBlankLines(blocks, lines) { + const merged = []; + for (const block of blocks) { + const previous = merged[merged.length - 1]; + const gapStart = previous ? previous[previous.length - 1].line : 0; + const gap = previous ? lines.slice(gapStart, block[0].line - 1) : null; + if (previous && gap.length > 0 && gap.every((line) => BLANK_CONTINUATION.test(line))) { + previous.push(...block); + previous.spanEnd = block[block.length - 1].line; + } else { + const copy = [...block]; + copy.spanEnd = block[block.length - 1].line; + merged.push(copy); + } + } + return merged; +} + +const blocksOf = (lines, ext) => mergeAcrossBlankLines(groupComments(scanFor(lines, ext)), lines); + +function scanFor(lines, ext) { + if (ext === ".md") return scanMarkdown(lines); + if (ext === ".py") return scanPython(lines); + return scanCLike(lines); +} + +// Asks the TypeScript parser, not a word list: prose puts two identifiers side by side, which is +// a syntax error, so clean diagnostics plus one binding or effectful statement means code. +function looksLikeCommentedOutCode(block) { + const texts = block.map((comment) => comment.text.trim()).filter(Boolean); + if (texts.length === 0 || texts.some((text) => PRAGMA.test(text))) return false; + const source = texts.join("\n"); + if (source.length < 6) return false; + // Parenthesised prose parses as a call; copied-out code keeps its formatter's terminator. + if (!/[;}]$/.test(source.trim())) return false; + const parsed = ts.createSourceFile( + "comment.tsx", + source, + ts.ScriptTarget.Latest, + false, + ts.ScriptKind.TSX, + ); + if (parsed.parseDiagnostics.length > 0) return false; + return parsed.statements.some((node) => { + if (REAL_STATEMENT.has(node.kind)) return true; + if (!ts.isExpressionStatement(node)) return false; + const expression = node.expression; + return ( + ts.isCallExpression(expression) || + ts.isAwaitExpression(expression) || + (ts.isBinaryExpression(expression) && + expression.operatorToken.kind === ts.SyntaxKind.EqualsToken) + ); + }); +} + +// Opener through closer line: a docblock's bare `/**` and `*/` carry no text but a reader +// still scrolls past them. +function physicalExtent(block, lines) { + let start = block[0].line; + let end = block.spanEnd ?? block[block.length - 1].line; + while (start > 1 && /^\s*\/\*+\s*$/.test(lines[start - 2])) start--; + while (end < lines.length && /^\s*\*+\/\s*$/.test(lines[end])) end++; + return { start, end }; +} + +// Every comment block in one source. `textLines` counts lines carrying prose, which a volume +// measure needs: a span grows when deleting code merges two blocks. +export function commentBlocks(source, ext) { + const lines = source.split("\n"); + const blocks = blocksOf(lines, ext); + return blocks.map((block) => ({ + ...physicalExtent(block, lines), + textLines: block.length, + marked: LENGTH_MARKER.test(block[0].text.trim()), + })); +} + +// Length, narration and commented-out code need no citation to be wrong. `touches(file, start, +// end)` limits them to blocks holding a line the diff added. +export function checkBlockRules(files, repo, touches = () => true) { + const failures = []; + for (const file of files) { + const ext = path.extname(file); + if (!CODE_EXT.has(ext) || !repo.has(file)) continue; + const lines = repo.source(file).split("\n"); + for (const block of blocksOf(lines, ext)) { + const extent = physicalExtent(block, lines); + if (!touches(file, extent.start, extent.end)) continue; + const where = `${file}:${block[0].line}`; + const first = block[0].text.trim(); + const joined = block.map((comment) => comment.text).join(" "); + + const span = extent.end - extent.start + 1; + if (span > MAX_BLOCK_LINES && !LENGTH_MARKER.test(first)) { + failures.push({ + where, + cite: `${span}-line comment block`, + why: `over ${MAX_BLOCK_LINES} lines. Cut it to the why and the invariant, or mark the first line "comment-length: " if it genuinely must stay (a licence, a diagram, a protocol table).`, + }); + } + + const history = joined.replace(QUOTED_SPAN, '""').match(HISTORY_NARRATION); + if (history) { + failures.push({ + where, + cite: joined.trim().slice(0, 100), + why: `narrates history ("${history[1]}"). Delete the sentence; git owns what this used to be. If it also states a live invariant, keep only that clause.`, + }); + } + + if (looksLikeCommentedOutCode(block)) { + failures.push({ + where, + cite: joined.trim().slice(0, 100), + why: "parses as code, so it is commented-out code. Delete it; git owns it.", + }); + } + } + } + return failures; +} + +function extractComments(source, ext) { + const lines = source.split("\n"); + return withBlockContext(scanFor(lines, ext), lines, ext); +} + +// camelCase only: a case shift separates a name from an English word, and PascalCase or +// snake_case names are mostly types and fields from outside the repo. +function looksLikeSymbol(token) { + if (!/^[a-z][A-Za-z0-9$]*$/.test(token)) return false; + if (token.length < 4 || ALLOW.has(token)) return false; + return /[a-z][A-Z]/.test(token); +} + +// Everything one comment line claims about the repo. `context` is the whole paragraph; a negation +// is read only from `prefix`, what the paragraph said before the citation. +function findCitations(text, context = text, prefix = "") { + const clean = text.replace(/\b[a-z][\w+.-]*:\/\/\S+/gi, " "); + if (NAMES_ANOTHER_REPO.test(context) || NAMES_A_DEPENDENCY.test(context)) { + return { paths: [], symbols: [], claims: [] }; + } + const paths = []; + const symbols = []; + let outsideTicks = clean; + for (const match of clean.matchAll(/`([^`\n]+)`/g)) { + const span = match[1].trim(); + const before = prefix + clean.slice(0, match.index); + outsideTicks = outsideTicks.replace(match[0], " ".repeat(match[0].length)); + if (ASSERTS_ABSENCE.test(before)) continue; + const withLine = span.match(new RegExp(`^(.+\\.${PATH_EXT}):(\\d+)$`)); + if (withLine) { + paths.push({ path: withLine[1], line: Number(withLine[2]), quoted: span }); + continue; + } + if (PATHISH.test(span)) { + paths.push({ path: span, line: null, quoted: span }); + continue; + } + if (looksLikeSymbol(span)) symbols.push(span); + } + for (const match of outsideTicks.matchAll(BARE_PATH_LINE)) { + if (ASSERTS_ABSENCE.test(prefix + clean.slice(0, match.index))) continue; + paths.push({ path: match[1], line: Number(match[2]), quoted: match[0] }); + } + const claims = [...clean.matchAll(TEST_CLAIM)].map((m) => ({ phrase: m[1], path: m[2] })); + return { paths, symbols, claims }; +} + +/** The repo as the checker needs to see it: which paths exist, how long they are, and where a + * word occurs. Backed by git, so it sees what is tracked and nothing else. */ +export function openRepo(root) { + const tracked = execFileSync("git", ["ls-files"], { + cwd: root, + encoding: "utf8", + maxBuffer: 256e6, + }) + .split("\n") + .filter(Boolean); + const files = new Set(tracked); + const byBasename = new Map(); + for (const file of tracked) { + const base = path.basename(file); + if (!byBasename.has(base)) byBasename.set(base, []); + byBasename.get(base).push(file); + } + const contents = new Map(); + + const read = (file) => { + if (!contents.has(file)) contents.set(file, readFileSync(path.join(root, file), "utf8")); + return contents.get(file); + }; + + return { + has: (file) => files.has(file), + source: read, + // Tried from the repo root, the citing file, its package, then as a path suffix, because + // people abbreviate a long path to the part that identifies it. + resolve(cited, from) { + const candidates = [ + path.normalize(cited), + path.normalize(path.join(path.dirname(from), cited)), + ]; + for (let dir = path.dirname(from); dir !== "." && dir !== "/"; dir = path.dirname(dir)) { + if (files.has(path.join(dir, "package.json"))) { + candidates.push(path.normalize(path.join(dir, cited))); + break; + } + } + for (const candidate of candidates) if (files.has(candidate)) return candidate; + const suffix = `/${path.normalize(cited)}`; + const loose = cited.includes("/") + ? tracked.filter((file) => file.endsWith(suffix)) + : (byBasename.get(cited) ?? []); + // Several packages share a basename, so the citing file's own package wins. + return nearestTo(from, loose); + }, + // A `packages//...` path whose package this checkout does not contain. + namesAbsentPackage(cited) { + const parts = path.normalize(cited).split("/"); + if (parts[0] !== "packages" || parts.length < 3) return false; + return !tracked.some((file) => file.startsWith(`packages/${parts[1]}/`)); + }, + lineCount(file) { + return read(file).split("\n").length; + }, + mentions(file, token) { + return read(file).includes(token); + }, + // Where each token occurs as a whole word, as "path:line". One git grep per batch of 400, + // since a spawn per token is minutes on a large diff. + occurrencesOf(tokens) { + const found = new Map(tokens.map((token) => [token, []])); + for (let i = 0; i < tokens.length; i += 400) { + const batch = tokens.slice(i, i + 400); + let output = ""; + try { + output = execFileSync("git", ["grep", "-nwF", ...batch.flatMap((t) => ["-e", t])], { + cwd: root, + encoding: "utf8", + maxBuffer: 512e6, + }); + } catch (error) { + // git grep exits 1 for "no matches at all". Any other status is a broken repo. + if (error.status !== 1) throw error; + } + for (const hit of output.split("\n")) { + if (!hit) continue; + const at = hit.split(":").slice(0, 2).join(":"); + const body = hit.slice(at.length + 1); + for (const token of batch) if (body.includes(token)) found.get(token).push(at); + } + } + return found; + }, + }; +} + +/** Every claim the comments in these files make about the rest of the repo, before any of it is + * resolved. Kept separate from resolution so the resolver can batch its one git grep. */ +function collectCitations(files, repo, inScope) { + const cited = []; + // The one place that decides whether a named file was opened; the OK line reports this count. + let examined = 0; + for (const file of files) { + if (!SCANNABLE.has(path.extname(file))) continue; + if (!repo.has(file)) continue; // deleted by this diff, or untracked + examined++; + for (const comment of extractComments(repo.source(file), path.extname(file))) { + const where = `${file}:${comment.line}`; + const touched = inScope(file, comment.line); + const { paths, symbols, claims } = findCitations( + comment.text, + comment.context, + comment.prefix, + ); + for (const cite of paths) cited.push({ kind: "path", where, file, touched, ...cite }); + for (const symbol of symbols) { + cited.push({ + kind: "symbol", + where, + file, + touched, + symbol, + blockStart: comment.blockStart, + blockEnd: comment.blockEnd, + }); + } + for (const claim of claims) + cited.push({ kind: "claim", where, file, touched, ...claim, symbols }); + } + } + return { cited, examined }; +} + +// `scope` is `{ kind: "all" }` for a sweep, or `{ kind: "lines", added, removed }` for a diff, +// where `removed` holds the names the diff's deleted code used. Only touched lines fail. +export function checkComments(root, files, scope = { kind: "all" }) { + const repo = openRepo(root); + const inScope = (file, line) => + scope.kind === "all" || (scope.added.get(file)?.has(line) ?? false); + const touches = (file, start, end) => { + const edited = scope.edited?.get(file); + for (let line = start; line <= end; line++) { + if (inScope(file, line) || edited?.has(line)) return true; + } + return false; + }; + const { cited, examined } = collectCitations(files, repo, inScope); + const warnings = []; + const blockFailures = checkBlockRules(files, repo, touches); + const symbols = [...new Set(cited.filter((c) => c.kind === "symbol").map((c) => c.symbol))]; + const occurrences = repo.occurrencesOf(symbols); + const missingPaths = new Set(); + const failures = []; + const report = (cite, why) => + (cite.touched ? failures : warnings).push({ + where: cite.where, + cite: cite.quoted ?? cite.display, + why, + }); + + for (const cite of cited) { + if (cite.kind === "path") { + if (namesSomethingElse(cite.path) || repo.namesAbsentPackage(cite.path)) continue; + const resolved = repo.resolve(cite.path, cite.file); + if (resolved === null) { + missingPaths.add(`${cite.where} ${cite.path}`); + report(cite, "no such file in the repo"); + continue; + } + const length = repo.lineCount(resolved); + if (cite.line !== null && cite.line > length) + report(cite, `${resolved} has only ${length} lines`); + continue; + } + + if (cite.kind === "symbol") { + // A hit inside the citing block proves nothing, so the whole block is excluded, not only + // the citing line: a renamed symbol can linger two lines up in the same docblock. + const elsewhere = occurrences.get(cite.symbol).filter((hit) => { + const match = hit.match(/^(.*):(\d+)$/); + if (!match) return true; + const [, hitFile, hitLine] = match; + return !( + hitFile === cite.file && + Number(hitLine) >= cite.blockStart && + Number(hitLine) <= cite.blockEnd + ); + }); + // A comment explaining a deletion may name what the diff's removed code used. Removed + // code, not removed lines: an edited comment would otherwise waive its own stale name. + if (elsewhere.length === 0 && !scope.removed?.has(cite.symbol)) { + report( + { ...cite, display: `\`${cite.symbol}\`` }, + "resolves nowhere in the repo (add it to ALLOW in this script if it names something outside it)", + ); + } + continue; + } + + if (namesSomethingElse(cite.path) || repo.namesAbsentPackage(cite.path)) continue; + const shown = { ...cite, display: `${cite.phrase} ${cite.path}` }; + const resolved = repo.resolve(cite.path, cite.file); + if (resolved === null) { + // The path pass owns "no such file"; report it here only if it did not already see this one. + if (!missingPaths.has(`${cite.where} ${cite.path}`)) + report(shown, "that file does not exist"); + continue; + } + if ( + cite.symbols.length > 0 && + !cite.symbols.some((symbol) => repo.mentions(resolved, symbol)) + ) { + report( + shown, + `${resolved} never mentions ${cite.symbols.map((sym) => `\`${sym}\``).join(" or ")}`, + ); + } + } + return { failures: [...failures, ...blockFailures], warnings, examined }; +} + +function mergeBase(root, ref) { + return execFileSync("git", ["merge-base", ref, "HEAD"], { cwd: root, encoding: "utf8" }).trim(); +} + +function changedFiles(root, base) { + return execFileSync("git", ["diff", "--name-only", `${base}...HEAD`], { + cwd: root, + encoding: "utf8", + maxBuffer: 64e6, + }) + .split("\n") + .filter(Boolean); +} + +// The code on one removed line, comment stripped. It errs toward dropping names: a lost name +// fails a citation loudly, a kept one would waive it silently. +function codeOnly(line) { + const closed = line.replace(/\/\*[\s\S]*?\*\//g, " "); + const opensBlock = closed.indexOf("/*"); + const body = opensBlock === -1 ? closed : closed.slice(0, opensBlock); + if (/^\s*\*/.test(body)) return ""; + const opensLine = body.indexOf("//"); + return opensLine === -1 ? body : body.slice(0, opensLine); +} + +const IDENTIFIER = /[A-Za-z_$][\w$]*/g; + +// One `git diff` answers three things: the lines each file gained, the lines a deleted comment line +// sat between (so the block it left still counts as edited), and the names removed code used. +// No pathspec: limiting it to the new paths stops git pairing a rename with its old path. +// `#` opens a comment only in Python; in TypeScript it opens a private field. +const COMMENT_LINE = /^\s*(?:\/\/|\/\*|\*)/; +const HASH_COMMENT_LINE = /^\s*#/; + +function diffScope(root, base) { + const added = new Map(); + const edited = new Map(); + const removed = new Set(); + const diff = execFileSync("git", ["diff", "--unified=0", "-M", `${base}...HEAD`], { + cwd: root, + encoding: "utf8", + maxBuffer: 256e6, + }); + let lines = null; + let near = null; + let gapAt = null; + let hashComments = false; + for (const line of diff.split("\n")) { + if (line.startsWith("+++ ")) { + lines = near = null; + if (line.startsWith("+++ b/")) { + hashComments = line.endsWith(".py"); + added.set(line.slice(6), (lines = new Set())); + edited.set(line.slice(6), (near = new Set())); + } + continue; + } + const hunk = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))?/); + if (hunk) { + const start = Number(hunk[1]); + const count = Number(hunk[2] ?? 1); + for (let i = 0; i < count; i++) lines?.add(start + i); + gapAt = count === 0 ? start : null; + continue; + } + // Skips the "--- a/path" header, and with it a removed line whose code starts with a dash. + if (line.startsWith("-") && !line.startsWith("---")) { + const body = line.slice(1); + const isComment = COMMENT_LINE.test(body) || (hashComments && HASH_COMMENT_LINE.test(body)); + if (near && gapAt !== null && isComment) near.add(gapAt).add(gapAt + 1); + for (const name of codeOnly(body).match(IDENTIFIER) ?? []) removed.add(name); + } + } + return { added, edited, removed }; +} + +// A file whose citations are dead by construction. Every run grades it first, resolved against +// this script's repo, so a checker that stopped resolving cannot print OK. +export const PROBE = "scripts/gate-probes/broken-citation.md"; +const PROBE_ROOT = path.resolve(import.meta.dirname, ".."); + +// Path and symbol citations are settled by separate resolvers; each must report on the probe. +const PROBE_MUST_REPORT = [ + ["a path that does not resolve", /no such file|does not exist/], + ["a symbol that resolves nowhere", /resolves nowhere/], +]; + +function proveGateCanFail() { + let failures = []; + try { + failures = checkComments(PROBE_ROOT, [PROBE], { kind: "all" }).failures; + } catch (error) { + console.error(`comments: the self-check could not run: ${error.message}`); + process.exit(2); + } + const silent = PROBE_MUST_REPORT.filter( + ([, saying]) => !failures.some((f) => saying.test(f.why)), + ); + if (silent.length === 0) return; + console.error(`comments: SELF-CHECK FAILED. ${PROBE} carries a citation of each kind below and`); + console.error(` this checker did not report ${silent.length} of them, so a clean verdict on`); + console.error(` anything else would mean nothing. Silent:`); + for (const [kind] of silent) console.error(` ${kind}`); + console.error(` Repair the checker, or the probe file if it was edited.`); + process.exit(2); +} + +if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) { + const root = process.cwd(); + proveGateCanFail(); + const explicit = process.argv.slice(2); + // Named files are a sweep of every line and need no git ref; no arguments grades the diff + // against the fork point with the base branch. + const base = + explicit.length > 0 ? null : mergeBase(root, process.env.COMMENT_CHECK_BASE ?? "origin/main"); + // The probe is graded by the self-check only, or every PR that edits it would go red. + const files = + base === null ? explicit : changedFiles(root, base).filter((file) => file !== PROBE); + const scope = base === null ? { kind: "all" } : { kind: "lines", ...diffScope(root, base) }; + const { failures, warnings, examined } = checkComments(root, files, scope); + + // A run that opened no file must not print OK. A sweep that examined nothing is a mistaken + // invocation (a git ref passed as a path); a diff with no scannable file is ordinary. + if (examined === 0) { + if (explicit.length > 0) { + console.error(`comments: examined 0 of the ${explicit.length} argument(s) given.`); + console.error(` This script takes FILE PATHS, not a git ref, and only these extensions:`); + console.error(` ${[...SCANNABLE].join(" ")}. Run it with no arguments to grade the diff.`); + process.exit(2); + } + console.log(`comments: nothing to check (${files.length} changed file(s), none scannable)`); + process.exit(0); + } + + for (const warning of warnings) { + console.log(`comments: ${warning.where} ${warning.cite}\n ${warning.why}`); + } + if (warnings.length > 0) { + console.log( + `\n ${warnings.length} of these. They are not on a line this diff touched, so they`, + ); + console.log(` do not fail the build. Fixing one while you are in the file is still free.\n`); + } + if (failures.length === 0) { + console.log(`comments: OK (${examined} file(s) examined of ${files.length} in scope)`); + process.exit(0); + } + console.error( + `comments FAILED. ${failures.length} problem(s) in the comments of the files this diff changed.\n`, + ); + for (const failure of failures) { + console.error(` - ${failure.where} ${failure.cite}`); + console.error(` ${failure.why}`); + } + console.error( + `\n Fix the citation or delete the comment. A comment nobody can trust is worse than`, + ); + console.error(` no comment: it reads as evidence and sends the next reader to a dead end.`); + process.exit(1); +} diff --git a/scripts/check-comment-citations.test.mjs b/scripts/check-comment-citations.test.mjs new file mode 100644 index 0000000000..76a00cd6fe --- /dev/null +++ b/scripts/check-comment-citations.test.mjs @@ -0,0 +1,812 @@ +// fallow-ignore-file complexity +// Each case builds a real git repo in a temp dir and runs the real checker against it, because +// the resolver is git: a stub for "does this path exist" would pass while the git calls were wrong. +import assert from "node:assert/strict"; +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { test } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { PROBE, checkComments } from "./check-comment-citations.mjs"; + +const CHECKER = fileURLToPath(new URL("./check-comment-citations.mjs", import.meta.url)); + +const BRAND = { + "src/brand.ts": ["export function brandKitId(kit) {", " return kit.id;", "}", ""].join("\n"), + "tests/brand.test.ts": [ + "import { brandKitId } from '../src/brand';", + "brandKitId({ id: 1 });", + "", + ].join("\n"), + "tests/unrelated.test.ts": ["import { other } from '../src/other';", "other();", ""].join("\n"), +}; + +function checkSubject(t, subject, extra = {}, scope = { kind: "all" }) { + const root = mkdtempSync(path.join(tmpdir(), "comment-citations-")); + t.after(() => rmSync(root, { recursive: true, force: true })); + const write = (entries) => { + for (const [rel, body] of Object.entries(entries)) { + mkdirSync(path.join(root, path.dirname(rel)), { recursive: true }); + writeFileSync(path.join(root, rel), body); + } + }; + execFileSync("git", ["init", "-q"], { cwd: root }); + write({ ...BRAND, ...extra, ...subject }); + execFileSync("git", ["add", "-A"], { cwd: root }); + const subjectPaths = Object.keys(subject); + const resolved = + scope.kind === "lines" + ? { + kind: "lines", + added: new Map(subjectPaths.map((file) => [file, scope.added])), + removed: scope.removed ?? new Set(), + } + : scope; + return checkComments(root, subjectPaths, resolved); +} + +const oneLiner = (comment) => [comment, "export const subject = 1;", ""].join("\n"); + +test("a comment whose path, line, symbol and test all resolve passes", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner( + "// The id comes from `brandKitId` in `src/brand.ts:1`, pinned by `tests/brand.test.ts`.", + ), + }); + assert.deepEqual(failures, []); +}); + +test("a cited file that does not exist fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// The shape lives in `src/gone.ts`."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /no such file/); + assert.equal(failures[0].where, "src/subject.ts:1"); +}); + +test("a line number past the end of a real file fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Defined at `src/brand.ts:99`."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /has only 4 lines/); +}); + +test("a backticked symbol that resolves nowhere fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Guarded by `missingHelperName`."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /resolves nowhere/); +}); + +// A symbol named two lines up in its own docblock is the citing block reading itself. +test("a symbol named only elsewhere in its own docblock still fails", (t) => { + const { failures } = checkSubject( + t, + { + "src/subject.ts": [ + "/**", + " * The comment renames a field to fooBar, matching the doc line below.", + " * The tests assert `fooBar` is set on write.", + " */", + "export const subject = 1;", + "", + ].join("\n"), + }, + {}, + { kind: "lines", added: new Set([3]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /resolves nowhere/); +}); + +test("a pinned-by claim on a test that never names the symbol fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// `brandKitId` is pinned by `tests/unrelated.test.ts`."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /never mentions `brandKitId`/); +}); + +test("a citation inside a fenced markdown block is not a citation", (t) => { + const { failures } = checkSubject(t, { + "docs/note.md": ["Prose.", "", "```ts", "// see `src/gone.ts`", "```", "", "Done.", ""].join( + "\n", + ), + }); + assert.deepEqual(failures, []); +}); + +test("the same markdown citation outside the fence does fail", (t) => { + const { failures } = checkSubject(t, { + "docs/note.md": ["Prose.", "", "see `src/gone.ts`", "", "Done.", ""].join("\n"), + }); + assert.equal(failures.length, 1); + assert.equal(failures[0].where, "docs/note.md:3"); +}); + +test("a slash-slash inside a multi-line template literal is code, not a comment", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": ["export const snippet = `", "// the shape lives in `,", " ;", ""].join( + "\n", + ), + }); + assert.deepEqual(failures, []); +}); + +test("history narration on a touched line fails the build", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// This used to be a map.") }, + {}, + { kind: "lines", added: new Set([1]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].cite, /used to be a map/); + assert.match(failures[0].why, /narrates history/); +}); + +test("a broken citation on a line the diff did not touch warns instead of failing", (t) => { + const { failures, warnings } = checkSubject( + t, + { "src/subject.ts": oneLiner("// The shape lives in `src/gone.ts`.") }, + {}, + { kind: "lines", added: new Set([2]) }, + ); + assert.deepEqual(failures, []); + assert.equal(warnings.length, 1); + assert.match(warnings[0].why, /no such file/); +}); + +test("a broken citation on a line the diff did touch fails", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// The shape lives in `src/gone.ts`.") }, + {}, + { kind: "lines", added: new Set([1]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /no such file/); +}); + +test("a bare filename resolves to the citing file's own package, not another's", (t) => { + const { failures } = checkSubject( + t, + { "packages/web/src/subject.ts": oneLiner("// Mirrors `helper.ts:3`.") }, + { + "packages/api/src/helper.ts": ["one", ""].join("\n"), + "packages/web/src/helper.ts": ["one", "two", "three", ""].join("\n"), + }, + ); + assert.deepEqual(failures, []); +}); + +test("a sentence asserting a name is absent is not a citation", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner( + "// There is no `src/gone.ts` and no package sets `strictNullChecks`.", + ), + }); + assert.deepEqual(failures, []); +}); + +test("the same names without the negation still fail", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner( + "// The shape is in `src/gone.ts`; every package sets `strictNullChecks`.", + ), + }); + assert.equal(failures.length, 2); +}); + +test("a sentence that names another repo is not this repo's to resolve", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Mirrors `publish.yml` in heygen-com/other-repo."), + }); + assert.deepEqual(failures, []); +}); + +test("a sentence that names this repo is still resolved", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Mirrors `publish.yml` in heygen-com/hyperframes."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /no such file/); +}); + +test("a sentence that names an installed dependency is not this repo's to resolve", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// `next/dist/compiled/cookies` supplies `parseTheCookie`."), + }); + assert.deepEqual(failures, []); +}); + +test("the repo a paragraph names covers a citation two lines later", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": [ + "// heygen-com/other-repo splits it the same way:", + "// its roster hooks pass a limit, and it fetches the pack separately", + "// through `getAvatarGroupLookList`.", + "export const subject = 1;", + "", + ].join("\n"), + }); + assert.deepEqual(failures, []); +}); + +test("a paragraph break ends that cover", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": [ + "// heygen-com/other-repo splits it the same way.", + "export const between = 1;", + "// It fetches the pack through `getAvatarGroupLookList`.", + "export const subject = 1;", + "", + ].join("\n"), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].cite, /getAvatarGroupLookList/); +}); + +test("a negation governs a name that wrapped onto the next line", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": [ + "// No package sets `strictNullChecks`, `exactOptionalPropertyTypes` or", + "// `noImplicitOverride`.", + "export const subject = 1;", + "", + ].join("\n"), + }); + assert.deepEqual(failures, []); +}); + +// No ".ts" twin of the fixture exists, so a capture truncated to ".ts" resolves nothing and reds. +test("a pinned-by claim reads the whole extension, not a prefix of it", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// `brandKitId`, see `tests/moment.test.tsx`.") }, + { "tests/moment.test.tsx": ["import { brandKitId } from '../src/brand';", ""].join("\n") }, + ); + assert.deepEqual(failures, []); +}); + +// --- Rule: a comment block longer than 40 lines --------------------------------------------- + +const longBlock = (lines, first = "// Why this exists.") => + [ + first, + ...Array.from({ length: lines - 1 }, (_, i) => `// line ${i + 2} of the explanation.`), + "export const subject = 1;", + "", + ].join("\n"); + +test("a comment block over 40 lines fails", (t) => { + const { failures } = checkSubject(t, { "src/subject.ts": longBlock(41) }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /over 40 lines/); +}); + +test("a comment block of exactly 40 lines passes", (t) => { + const { failures } = checkSubject(t, { "src/subject.ts": longBlock(40) }); + assert.deepEqual(failures, []); +}); + +test("an over-long block with a reasoned comment-length marker passes", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": longBlock( + 41, + "// comment-length: the wire protocol table, and it has to stay whole", + ), + }); + assert.deepEqual(failures, []); +}); + +test("a comment-length marker with no reason does not silence the rule", (t) => { + const { failures } = checkSubject(t, { "src/subject.ts": longBlock(41, "// comment-length:") }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /over 40 lines/); +}); + +// --- Scope: the block rules grade only blocks holding a line the diff added ----------------- + +test("an over-long block the diff did not touch is not graded", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": longBlock(41) }, + {}, + { kind: "lines", added: new Set([42]) }, + ); + assert.deepEqual(failures, []); +}); + +test("one touched line inside an over-long block grades the whole block", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": longBlock(41) }, + {}, + { kind: "lines", added: new Set([20]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /over 40 lines/); +}); + +test("history narration in a block the diff did not touch is not graded", (t) => { + const { failures, warnings } = checkSubject( + t, + { "src/subject.ts": oneLiner("// This was removed in the rewrite.") }, + {}, + { kind: "lines", added: new Set([2]) }, + ); + assert.deepEqual(failures, []); + assert.deepEqual(warnings, []); +}); + +test("history narration fails when the diff touched another line of its block", (t) => { + const { failures } = checkSubject( + t, + { + "src/subject.ts": [ + "// Both halves are load-bearing: the fallback must never commit.", + "// The caller's own guard does not cover it.", + "export const subject = 1;", + "", + ].join("\n"), + }, + {}, + { kind: "lines", added: new Set([2]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +test("commented-out code the diff did not touch is not graded", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// const cached = readCache(key);") }, + {}, + { kind: "lines", added: new Set([2]) }, + ); + assert.deepEqual(failures, []); +}); + +test("commented-out code on a touched line fails", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// const cached = readCache(key);") }, + {}, + { kind: "lines", added: new Set([1]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /commented-out code/); +}); + +// --- Rule: history narration ------------------------------------------------------------------ + +test("a comment that narrates history fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// This used to read from the cache."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +// Bare "was" is excluded on purpose: this sentence states a live condition. +test("ordinary past tense that states a live condition passes", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Retry only if the response was truncated mid-frame."), + }); + assert.deepEqual(failures, []); +}); + +test("a pull request number is history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Guards the race fixed in PR #4431."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +// --- Rule: commented-out code ----------------------------------------------------------------- + +test("commented-out code fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// const cached = readCache(key);"), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /commented-out code/); +}); + +// Both contain "return" and "cached"; only one is code, which is why the rule asks the parser. +test("prose describing what the code returns passes", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Return the cached row when the key is warm."), + }); + assert.deepEqual(failures, []); +}); + +test("a suppression pragma is not commented-out code", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// @ts-expect-error upstream types lag the runtime"), + }); + assert.deepEqual(failures, []); +}); + +test("a well-formed TODO that names a call is not commented-out code", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// TODO: drop this once readCache(key) is memoised"), + }); + assert.deepEqual(failures, []); +}); + +// --- Scope: the block rules are for code, not for the record ---------------------------------- + +test("a markdown file is exempt from all three block rules", (t) => { + const { failures } = checkSubject(t, { + "docs/plan.md": [ + "This used to read from the cache.", + "", + "```", + "const cached = readCache(key);", + "```", + "", + ].join("\n"), + }); + assert.deepEqual(failures, []); +}); + +test("before this, meaning earlier than this one, is not history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// How long the call gets before this route stops waiting on it."), + }); + assert.deepEqual(failures, []); +}); + +test("before this change, naming the change, is history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// Before this change the step cast every look the same way."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +// A quote reports someone else's words; rewording it would misquote them. +test("history narration inside a quotation is not this code's history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner('// The upstream error reads "that item is no longer in this kit".'), + }); + assert.deepEqual(failures, []); +}); + +test("history narration outside the quotation still fails", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner('// We used to show "that item is gone" here.'), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +// "was called" describes what the program did; only the structural verbs a commit performs fail. +test("a spy assertion using was called is not history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// A spy proves `brandKitId` was called, not that the row landed."), + }); + assert.deepEqual(failures, []); +}); + +test("a structural verb is still history", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// The helper was renamed when the rail landed."), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +test("a single-quoted quotation is skipped too", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// The upstream error reads 'that item is no longer in this kit'."), + }); + assert.deepEqual(failures, []); +}); + +test("no longer states a live fact and does not fail", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": oneLiner("// A tile cannot show something the catalog no longer contains."), + }); + assert.deepEqual(failures, []); +}); + +test("a file the diff does not touch is not graded", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// Nothing to see here.") }, + { + "src/elsewhere.ts": [ + "// This half is load-bearing: the fallback must never commit.", + "export const elsewhere = 1;", + "", + ].join("\n"), + }, + ); + assert.deepEqual(failures, []); +}); + +// --- Rule: a name outside this repo, and a name only the merge base holds ------------------- + +test("a React hook names the platform, not a broken citation", (t) => { + const { failures } = checkSubject( + t, + { + "src/subject.ts": oneLiner( + "// `useTransition` in the parent cannot carry the flag: that render suspends and never commits.", + ), + }, + {}, + { kind: "lines", added: new Set([1]) }, + ); + assert.deepEqual(failures, []); +}); + +// A sentence about a deletion has to name the deleted thing. +test("a symbol used by code this diff deletes passes", (t) => { + const { failures } = checkSubject( + t, + { + "src/subject.ts": oneLiner( + "// The kit stops carrying `needsReview`: the verdict map owns what a review row means.", + ), + }, + {}, + { kind: "lines", added: new Set([1]), removed: new Set(["needsReview"]) }, + ); + assert.deepEqual(failures, []); +}); + +test("a symbol this diff never deleted still fails", (t) => { + const { failures } = checkSubject( + t, + { "src/subject.ts": oneLiner("// Guarded by `missingHelperName`.") }, + {}, + { kind: "lines", added: new Set([1]), removed: new Set(["needsReview"]) }, + ); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /resolves nowhere/); +}); + +// A contraction and the trigger in one block: a naive quote pattern blanks from the apostrophe on. +test("a contraction does not blank the rest of the comment", (t) => { + const { failures } = checkSubject(t, { + "src/subject.ts": [ + "// It isn't obvious, but the retry is load-bearing: the second read is what settles the row.", + "// The caller's own guard does not cover it.", + "export const subject = 1;", + "", + ].join("\n"), + }); + assert.equal(failures.length, 1); + assert.match(failures[0].why, /narrates history/); +}); + +test("a react-dom export names the platform, not a broken citation", (t) => { + const { failures } = checkSubject( + t, + { + "src/subject.ts": oneLiner( + "// `createPortal` would escape the clip, and `flushSync` inside the resize would tear the row.", + ), + }, + {}, + { kind: "lines", added: new Set([1]) }, + ); + assert.deepEqual(failures, []); +}); + +// Drives the entry point against a real two-commit repo, so `mergeBase`, `changedFiles` and +// `diffScope` are tested too. A path in `base` and absent from `head` is a file the diff deletes; +// an `overrides` entry set to undefined drops that variable. +function runCli(t, base, head, args = [], overrides = {}) { + const root = mkdtempSync(path.join(tmpdir(), "comment-citations-cli-")); + t.after(() => rmSync(root, { recursive: true, force: true })); + const write = (entries) => { + for (const [rel, body] of Object.entries(entries)) { + mkdirSync(path.join(root, path.dirname(rel)), { recursive: true }); + writeFileSync(path.join(root, rel), body); + } + }; + const identity = ["-c", "user.email=t@example.com", "-c", "user.name=Test"]; + const run = (...argv) => execFileSync("git", argv, { cwd: root, encoding: "utf8" }); + run("init", "-q"); + write(base); + run("add", "-A"); + run(...identity, "commit", "-qm", "base", "--no-gpg-sign"); + const baseSha = run("rev-parse", "HEAD").trim(); + for (const rel of Object.keys(base)) if (!(rel in head)) rmSync(path.join(root, rel)); + write(head); + run("add", "-A"); + run(...identity, "commit", "-qm", "head", "--no-gpg-sign"); + + const env = { ...process.env, COMMENT_CHECK_BASE: baseSha, ...overrides }; + for (const [key, value] of Object.entries(env)) if (value === undefined) delete env[key]; + const cli = spawnSync(process.execPath, [CHECKER, ...args], { cwd: root, encoding: "utf8", env }); + return { status: cli.status, output: `${cli.stdout}${cli.stderr}` }; +} + +const KIT = { "src/kit.ts": ["export const needsReview = 1;", ""].join("\n") }; +const explains = (symbol) => + oneLiner( + `// The kit stops carrying \`${symbol}\`: the verdict map owns what a review row means.`, + ); + +test("the CLI passes a citation of a symbol this diff deletes", (t) => { + const { status, output } = runCli(t, KIT, { "src/subject.ts": explains("needsReview") }); + assert.equal(status, 0, output); + assert.match(output, /comments: OK/); +}); + +test("the CLI fails a citation of a symbol no commit ever held", (t) => { + const { status, output } = runCli(t, KIT, { "src/subject.ts": explains("missingHelperName") }); + assert.equal(status, 1, output); + assert.match(output, /missingHelperName/); + assert.match(output, /resolves nowhere in the repo/); +}); + +test("the CLI fails a commented-out block the diff adds and passes one it left alone", (t) => { + const dead = oneLiner("// const cached = readCache(key);"); + const added = runCli(t, KIT, { ...KIT, "src/subject.ts": dead }); + assert.equal(added.status, 1, added.output); + assert.match(added.output, /commented-out code/); + const base = { ...KIT, "src/subject.ts": dead }; + const untouched = runCli(t, base, { + ...base, + "src/subject.ts": `${dead}export const more = 2;\n`, + }); + assert.equal(untouched.status, 0, untouched.output); +}); + +test("a hand-run sweep grades a checkout that has no origin", (t) => { + const subject = oneLiner("// The id comes from `brandKitId` in `src/brand.ts:1`."); + const head = { ...BRAND, "src/subject.ts": subject }; + const { status, output } = runCli(t, KIT, head, ["src/subject.ts"], { + COMMENT_CHECK_BASE: undefined, + }); + assert.equal(status, 0, output); + assert.match(output, /comments: OK/); +}); + +test("a sweep with no origin still fails a citation that resolves nowhere", (t) => { + const head = { ...BRAND, "src/subject.ts": explains("missingHelperName") }; + const { status, output } = runCli(t, KIT, head, ["src/subject.ts"], { + COMMENT_CHECK_BASE: undefined, + }); + assert.equal(status, 1, output); + assert.match(output, /missingHelperName/); +}); + +// Editing a comment removes its old line, which carries the stale name. Waiving on removed lines +// would pass this; waiving on removed code fails it. +test("a citation already broken at the fork point fails when its line is edited", (t) => { + const rotted = (verb) => + oneLiner(`// The retry is settled by \`rottedName\` before the row is ${verb}.`); + const { status, output } = runCli( + t, + { ...KIT, "src/subject.ts": rotted("written") }, + { ...KIT, "src/subject.ts": rotted("committed") }, + ); + assert.equal(status, 1, output); + assert.match(output, /rottedName/); +}); + +// A `//` inside a string drops the rest of that removed line, so the name is not waived. +test("a name behind a slash-slash inside a string is not read as deleted code", (t) => { + const base = { + "src/kit.ts": ['export const docs = "https://example.com/needsReview";', ""].join("\n"), + }; + const { status, output } = runCli(t, base, { "src/subject.ts": explains("needsReview") }); + assert.equal(status, 1, output); + assert.match(output, /needsReview/); +}); + +// A removed line opening with `*` is block-comment text, so its names cannot waive anything. +test("a name inside a removed block comment is not read as deleted code", (t) => { + const base = { + "src/kit.ts": [ + "/**", + " * Settled by needsReview before the write.", + " */", + "export const kit = 1;", + "", + ].join("\n"), + }; + const head = { + "src/kit.ts": ["export const kit = 1;", ""].join("\n"), + "src/subject.ts": explains("needsReview"), + }; + const { status, output } = runCli(t, base, head); + assert.equal(status, 1, output); + assert.match(output, /needsReview/); +}); + +const wallFile = (first) => + [ + first, + ...Array.from({ length: 41 }, (_, i) => `// line ${i + 2} of the table.`), + "export const w = 1;", + "", + ].join("\n"); + +// A pathspec of only the new path stops git pairing the rename, and the whole file reads as added. +test("a pure rename does not grade the file's old comments", (t) => { + const old = { "src/a.ts": wallFile("// Why this exists.") }; + const { status, output } = runCli(t, old, { "src/b.ts": old["src/a.ts"] }); + assert.equal(status, 0, output); +}); + +test("deleting a block's comment-length marker grades the wall it leaves", (t) => { + const base = { + "src/a.ts": wallFile("// comment-length: a protocol table, it has to stay whole"), + }; + const head = { "src/a.ts": base["src/a.ts"].split("\n").slice(1).join("\n") }; + const { status, output } = runCli(t, base, head); + assert.equal(status, 1, output); + assert.match(output, /comment block/); +}); + +test("deleting a code line next to an old wall does not grade the wall", (t) => { + const base = { "src/a.ts": `export const gone = 0;\n${wallFile("// Why this exists.")}` }; + const { status, output } = runCli(t, base, { "src/a.ts": wallFile("// Why this exists.") }); + assert.equal(status, 0, output); +}); + +test("deleting a TypeScript private field next to an old wall does not grade the wall", (t) => { + const base = { "src/a.ts": ` #count = 0;\n${wallFile("// Why this exists.")}` }; + const { status, output } = runCli(t, base, { "src/a.ts": wallFile("// Why this exists.") }); + assert.equal(status, 0, output); +}); + +// --- A run that examined nothing must not report success -------------------------------------- + +test("a git ref handed in as an argument is refused, not counted as a file checked", (t) => { + const rot = oneLiner("// The shape lives in `src/gone.ts`."); + const { status, output } = runCli(t, KIT, { ...KIT, "src/subject.ts": rot }, ["origin/main"]); + assert.equal(status, 2, output); + assert.match(output, /examined 0 of the 1 argument\(s\) given/); + assert.doesNotMatch(output, /OK/); +}); + +test("a sweep of only unscannable files fails instead of reporting a pass", (t) => { + const head = { ...KIT, "docs/thing.json": "{}\n" }; + const { status, output } = runCli(t, KIT, head, ["docs/thing.json"]); + assert.equal(status, 2, output); + assert.match(output, /examined 0/); +}); + +test("the OK line reports how many files were examined, not how many were named", (t) => { + const clean = oneLiner("// The id comes from `brandKitId` in `src/brand.ts:1`."); + const head = { ...BRAND, ...KIT, "src/subject.ts": clean, "docs/thing.json": "{}\n" }; + const { status, output } = runCli(t, KIT, head, ["src/subject.ts", "docs/thing.json"]); + assert.equal(status, 0, output); + assert.match(output, /OK \(1 file\(s\) examined of 2 in scope\)/); +}); + +test("a diff of only unscannable files says so instead of reporting a pass", (t) => { + const { status, output } = runCli(t, KIT, { ...KIT, "docs/thing.json": "{}\n" }); + assert.equal(status, 0, output); + assert.match(output, /nothing to check \(1 changed file\(s\), none scannable\)/); + assert.doesNotMatch(output, /OK/); +}); + +// The probe's two citations use independent resolvers, so each must report on its own. +test("the built-in probe still carries a citation of each kind this checker refuses", () => { + const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); + const { failures } = checkComments(root, [PROBE], { kind: "all" }); + const why = failures.map((failure) => failure.why).join("\n"); + assert.match( + why, + /no such file|does not exist/, + "the path arm of the self-check reported nothing", + ); + assert.match(why, /resolves nowhere/, "the symbol arm of the self-check reported nothing"); +}); diff --git a/scripts/gate-probes/broken-citation.md b/scripts/gate-probes/broken-citation.md new file mode 100644 index 0000000000..962ff45914 --- /dev/null +++ b/scripts/gate-probes/broken-citation.md @@ -0,0 +1,9 @@ +# A citation that must never resolve + +This file is not documentation. `check-comment-citations.mjs` grades it before anything else, so a +checker that has stopped resolving citations fails instead of printing OK. + +Both citations below are dead on purpose and must stay dead. Do not create either name. + +The fence is pinned by `gate-probe-no-such-file.test.ts`, and the value comes from +`gateProbeNoSuchSymbol` at `gate-probe-no-such-file.ts:4242`.