mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-10-30 09:16:55 +00:00
ci: grade the comments each PR touches (#4444)
This commit is contained in:
Miguel Ángel
@@ -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
|
||||
@@ -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: <reason>`.
|
||||
|
||||
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 <name>`. Each item lives in its own directory under `registry/blocks/` or `registry/components/`.
|
||||
|
||||
+1
-1
@@ -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",
|
||||
|
||||
@@ -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 <file>") 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]*"|(?<![A-Za-z0-9])'[^'\n]*'(?![A-Za-z0-9])/g;
|
||||
|
||||
// Exempts a block from the length rule, only with a stated reason.
|
||||
const LENGTH_MARKER = /comment-length:\s*(\S.*?)\s*$/;
|
||||
|
||||
// Pragmas and TODOs parse as code and are not code (the ruff ERA001 false-positive classes).
|
||||
const PRAGMA =
|
||||
/^\s*(?:@ts-|eslint-|prettier-|biome-|oxlint-|istanbul |c8 |v8 |type:|noqa|pylint:|mypy:|TODO|FIXME|NOTE|XXX|HACK|ponytail:|comment-length:)/i;
|
||||
|
||||
// A bare identifier or string literal parses clean and means nothing; these bind or have an effect.
|
||||
const REAL_STATEMENT = new Set([
|
||||
ts.SyntaxKind.VariableStatement,
|
||||
ts.SyntaxKind.FunctionDeclaration,
|
||||
ts.SyntaxKind.ClassDeclaration,
|
||||
ts.SyntaxKind.IfStatement,
|
||||
ts.SyntaxKind.ForStatement,
|
||||
ts.SyntaxKind.ForOfStatement,
|
||||
ts.SyntaxKind.ForInStatement,
|
||||
ts.SyntaxKind.WhileStatement,
|
||||
ts.SyntaxKind.ReturnStatement,
|
||||
ts.SyntaxKind.ImportDeclaration,
|
||||
ts.SyntaxKind.ExportDeclaration,
|
||||
ts.SyntaxKind.ExportAssignment,
|
||||
ts.SyntaxKind.SwitchStatement,
|
||||
ts.SyntaxKind.TryStatement,
|
||||
ts.SyntaxKind.ThrowStatement,
|
||||
ts.SyntaxKind.InterfaceDeclaration,
|
||||
ts.SyntaxKind.TypeAliasDeclaration,
|
||||
]);
|
||||
|
||||
// Platform names git grep cannot find in this repo. Listed, not read from node_modules, so the
|
||||
// verdict does not depend on an install. Taken at react and react-dom 19.2.4.
|
||||
const REACT_EXPORTS = [
|
||||
"cacheSignal",
|
||||
"captureOwnerStack",
|
||||
"cloneElement",
|
||||
"createContext",
|
||||
"createElement",
|
||||
"createRef",
|
||||
"forwardRef",
|
||||
"isValidElement",
|
||||
"startTransition",
|
||||
"useActionState",
|
||||
"useCallback",
|
||||
"useContext",
|
||||
"useDebugValue",
|
||||
"useDeferredValue",
|
||||
"useEffect",
|
||||
"useEffectEvent",
|
||||
"useId",
|
||||
"useImperativeHandle",
|
||||
"useInsertionEffect",
|
||||
"useLayoutEffect",
|
||||
"useMemo",
|
||||
"useOptimistic",
|
||||
"useReducer",
|
||||
"useRef",
|
||||
"useState",
|
||||
"useSyncExternalStore",
|
||||
"useTransition",
|
||||
];
|
||||
const REACT_DOM_EXPORTS = [
|
||||
"createPortal",
|
||||
"createRoot",
|
||||
"flushSync",
|
||||
"hydrateRoot",
|
||||
"prefetchDNS",
|
||||
"preinitModule",
|
||||
"preloadModule",
|
||||
"requestFormReset",
|
||||
"useFormState",
|
||||
"useFormStatus",
|
||||
];
|
||||
|
||||
// Grow this when the gate flags a real name that lives outside the repo.
|
||||
const ALLOW = new Set([
|
||||
"canParse",
|
||||
"showModal",
|
||||
"requestIdleCallback",
|
||||
"webkitAnimationEnd",
|
||||
"targetOrigin",
|
||||
...REACT_EXPORTS,
|
||||
...REACT_DOM_EXPORTS,
|
||||
]);
|
||||
|
||||
// A URL, an absolute path, a `<sha>:<path>`, 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: <reason>" 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/<name>/...` 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);
|
||||
}
|
||||
@@ -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");
|
||||
});
|
||||
@@ -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`.
|
||||
Reference in New Issue
Block a user