ci: check TODO owners, public URLs and source links in comments

.github/pull_request_template.md

@@ -21,3 +21,4 @@ How was this tested?
 - [ ] Unit tests added/updated
 - [ ] Manual testing performed
 - [ ] Documentation updated (if applicable)
+- [ ] Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug

CONTRIBUTING.md

@@ -71,18 +71,31 @@ 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.
+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. We follow the [Stack Overflow guidance on code comments](https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/):
 
-The `Comments` check (`scripts/check-comment-citations.mjs`) grades the comments a PR adds or edits:
+1. **Don't repeat the code.** `// Count label` above `const countLabel` tells the reader nothing new.
+2. **A comment doesn't make unclear code clear.** Rename the variable or extract the function instead of explaining it.
+3. **If you can't write a clear comment, the code may be the problem.** Simplify it first.
+4. **Clear up confusion; don't add to it.** A comment the reader has to decode costs more than none.
+5. **Explain code that looks wrong on purpose.** An unusual loop, a deliberate no-op or a workaround says why it has to be that way.
+6. **Link the source of copied or adapted code**, with a URL that will still work.
+7. **Link external references where they help**: a spec, an issue or a vendor document anyone can open.
+8. **Explain bug fixes by what the code must do and how to reproduce the bug**, not by the change's history or PR number.
+9. **Mark unfinished work with a TODO that has an owner or an issue**: `TODO(name):`, `TODO(area):` or `TODO(#1234):`.
+
+Reviewers judge rules 2 to 5 and 8. 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>`.
+- **A TODO, FIXME or XXX names an owner or an issue** in its parentheses, or links the issue (rule 9).
+- **A URL can be opened by anyone** (rule 7): well-formed, not on a private network or internal host, and not signed or carrying a token. `localhost` addresses that describe a dev server are fine.
+- **Warnings that never fail the build:** copied or adapted code without a source link (rule 6), and a short comment whose words mostly restate the next line of code (rule 1). Both are heuristics that measured too many false positives on this repo to fail a build.
 
 - **A package file's comment share may not rise** (`scripts/comment-ratchet.mjs`). For source under `packages/*/src` (tests excluded), a file you change may not end up with a higher share of comment lines than it had where your branch forked, unless you only deleted code. It may not gain a new comment block over 12 lines, and a new file may not start above its package's share. Move the explanation into a name, a type or the PR description.
 
-Only comment blocks holding a line your PR added can fail the citation and block rules. 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`.
+Only comment blocks holding a line your PR added can fail the citation and block rules, and the TODO and URL rules grade only the lines your PR added. Broken citations 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)
 

scripts/check-comment-citations.mjs

@@ -417,6 +417,135 @@ export function checkBlockRules(files, repo, touches = () => true) {
   return failures;
 }
 
+// `#` opens a comment only in Python; in TypeScript it opens a private field.
+const COMMENT_LINE = /^\s*(?:\/\/|\/\*|\*)/;
+const HASH_COMMENT_LINE = /^\s*#/;
+const isCommentLine = (line, ext) =>
+  COMMENT_LINE.test(line) || (ext === ".py" && HASH_COMMENT_LINE.test(line));
+
+// A TODO names who owns it or the issue that tracks it. Only a marker opening a comment line
+// counts; a sentence that mentions a TODO is prose.
+const TODO_MARK = /^\s*(?:TODO|FIXME|XXX)\s*[:(]/;
+// An owner is a person or an area (`jrs`, `player-perf`, `core follow-up`), or an issue.
+const OWNER = "(?:@?[A-Za-z][\\w .-]*|#\\d+)";
+const TODO_OWNED = new RegExp(`\\b(?:TODO|FIXME|XXX)\\s*\\(${OWNER}(?:,\\s*${OWNER})*\\)`);
+// Owner or issue go in the parentheses, `TODO(name):` or `TODO(#1234):`: a bare `#123456` could be a
+// colour. Otherwise the TODO's own line links the issue.
+const COPIED = /\b(?:copied|adapted|ported|borrowed) from\b/i;
+const URL_IN_TEXT = /\bhttps?:\/\/[^\s<>"'`)\]]*/g;
+const TRAILING_PUNCTUATION = /[.,;:!?]+$/;
+// The host is parsed, not matched: `evil.example/github.com/...` is no issue link.
+const ISSUE_PATH = /^\/[\w.-]+\/[\w.-]+\/issues\/\d+\/?$/;
+const linksIssue = (text) =>
+  [...text.matchAll(URL_IN_TEXT)].some(([raw]) => {
+    const url = URL.parse(raw.replace(TRAILING_PUNCTUATION, ""));
+    return url?.hostname === "github.com" && ISSUE_PATH.test(url.pathname);
+  });
+// Loopback is left alone: comments describe dev servers and CORS origins by it, not as links.
+const LOOPBACK = /^https?:\/\/(?:localhost|127\.0\.0\.1|\[::1\])(?:[:/]|$)/i;
+const PRIVATE_HOST =
+  /^(?:10(?:\.\d+){3}|192\.168(?:\.\d+){2}|172\.(?:1[6-9]|2\d|3[01])(?:\.\d+){2}|169\.254(?:\.\d+){2})$|\.(?:local|internal|lan|corp|intranet)$/;
+const SIGNED_QUERY =
+  /[?&](?:X-Amz-Signature|X-Amz-Credential|X-Goog-Signature|Signature|sig|token|access_token|Key-Pair-Id)=/i;
+// prettier-ignore
+const STOP_WORDS = new Set([
+  "a", "an", "the", "this", "that", "these", "those", "to", "of", "for", "in", "on", "at", "by", "with",
+  "and", "or", "is", "are", "be", "it", "its", "we", "our", "if", "then", "else", "as", "from",
+]);
+const words = (text) =>
+  text
+    .replace(/([a-z0-9])([A-Z])/g, "$1 $2")
+    .toLowerCase()
+    .match(/[a-z][a-z0-9]*/g) ?? [];
+
+// Why a URL in a comment cannot be followed by a reader, or null when it can. Placeholders
+// (`https://${host}`, `https://<host>`) are templates, not links.
+function urlProblem(raw) {
+  const cited = raw.replace(TRAILING_PUNCTUATION, "");
+  if (/^https?:\/\/$/.test(cited) || /[{}$*…]/.test(cited) || LOOPBACK.test(cited)) return null;
+  let url;
+  try {
+    url = new URL(cited);
+  } catch {
+    return "is not a well-formed URL";
+  }
+  if (PRIVATE_HOST.test(url.hostname.toLowerCase()))
+    return `points at a private host (${url.hostname})`;
+  if (SIGNED_QUERY.test(url.search)) return "carries a signature or token in its query";
+  return null;
+}
+
+const RESTATE_SHARE = 0.8;
+const DIVIDER = /[─━═]{2,}|-{4,}|={4,}/;
+
+// A standalone comment of one or two lines whose words are almost all names on the next code line.
+// Section dividers and JSDoc are left alone: they label and document, they do not narrate.
+function restatesCode(block, lines, extent, ext) {
+  if (block.length > 2 || !isCommentLine(lines[block[0].line - 1], ext)) return false;
+  if (/^\s*\/\*\*/.test(lines[extent.start - 1]) || block.some(({ text }) => DIVIDER.test(text))) {
+    return false;
+  }
+  const next = lines.slice(extent.end).find((line) => line.trim());
+  if (!next || isCommentLine(next, ext) || /^\s*}/.test(next)) return false;
+  const said = words(block.map((comment) => comment.text).join(" ")).filter(
+    (w) => !STOP_WORDS.has(w),
+  );
+  if (said.length < 2) return false;
+  const code = new Set(words(stripStrings(next)));
+  return said.filter((w) => code.has(w)).length / said.length >= RESTATE_SHARE;
+}
+
+// Findings for the checkable best-practice rules, each with the line range it rests on. Pure, so
+// the same function grades a PR and measures the whole tree.
+export function practiceFindings(source, ext) {
+  if (!CODE_EXT.has(ext)) return [];
+  const lines = source.split("\n");
+  const found = [];
+  for (const block of blocksOf(lines, ext)) {
+    const extent = physicalExtent(block, lines);
+    const joined = block.map((comment) => comment.text).join(" ");
+    const linked = /\bhttps?:\/\//.test(joined);
+    for (const { line, text } of block) {
+      const at = { from: line, to: line, cite: text.trim().slice(0, 100) };
+      if (TODO_MARK.test(text) && !TODO_OWNED.test(text) && !linksIssue(text)) {
+        found.push({
+          ...at,
+          rule: "todo",
+          why: 'a TODO names its owner, area or issue, "TODO(name):" or "TODO(#1234):", or links the issue.',
+        });
+      }
+      if (COPIED.test(text) && !linked) {
+        found.push({
+          ...at,
+          rule: "source",
+          warn: true,
+          why: "copied or adapted code links the original source.",
+        });
+      }
+      for (const [raw] of text.matchAll(URL_IN_TEXT)) {
+        const problem = urlProblem(raw);
+        if (problem)
+          found.push({
+            ...at,
+            rule: "url",
+            why: `${raw} ${problem}; link what every reader can open.`,
+          });
+      }
+    }
+    if (restatesCode(block, lines, extent, ext)) {
+      found.push({
+        from: extent.start,
+        to: extent.end,
+        cite: joined.trim().slice(0, 100),
+        rule: "restates",
+        warn: true,
+        why: "restates the next line of code. Say why, or delete it and let the names speak.",
+      });
+    }
+  }
+  return found;
+}
+
 function extractComments(source, ext) {
   const lines = source.split("\n");
   return withBlockContext(scanFor(lines, ext), lines, ext);
@@ -606,6 +735,18 @@ export function checkComments(root, files, scope = { kind: "all" }) {
   const { cited, examined } = collectCitations(files, repo, inScope);
   const warnings = [];
   const blockFailures = checkBlockRules(files, repo, touches);
+  // Line rules grade the lines the diff added; the restating rule grades the block it rests on.
+  for (const file of files.filter((name) => repo.has(name))) {
+    for (const finding of practiceFindings(repo.source(file), path.extname(file))) {
+      const graded =
+        finding.rule === "restates"
+          ? touches(file, finding.from, finding.to)
+          : inScope(file, finding.from);
+      if (!graded) continue;
+      const entry = { where: `${file}:${finding.from}`, cite: finding.cite, why: finding.why };
+      (finding.warn ? warnings : blockFailures).push(entry);
+    }
+  }
   const symbols = [...new Set(cited.filter((c) => c.kind === "symbol").map((c) => c.symbol))];
   const occurrences = repo.occurrencesOf(symbols);
   const missingPaths = new Set();
@@ -708,10 +849,6 @@ 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();
@@ -817,9 +954,9 @@ if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) {
   }
   if (warnings.length > 0) {
     console.log(
-      `\n  ${warnings.length} of these. They are not on a line this diff touched, so they`,
+      `\n  ${warnings.length} warning(s). They do not fail the build; fixing one while you are in`,
     );
-    console.log(`  do not fail the build. Fixing one while you are in the file is still free.\n`);
+    console.log(`  the file is still free.\n`);
   }
   if (failures.length === 0) {
     console.log(`comments: OK (${examined} file(s) examined of ${files.length} in scope)`);

scripts/check-comment-citations.test.mjs

@@ -437,7 +437,7 @@ test("a suppression pragma is not commented-out code", (t) => {
 
 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"),
+    "src/subject.ts": oneLiner("// TODO(jrs): drop this once readCache(key) is memoised"),
   });
   assert.deepEqual(failures, []);
 });
@@ -810,3 +810,147 @@ test("the built-in probe still carries a citation of each kind this checker refu
   );
   assert.match(why, /resolves nowhere/, "the symbol arm of the self-check reported nothing");
 });
+
+// --- Best-practice rules: TODO owner, source links, public URLs, restating the code ------------
+
+const failsWith = (t, comment, pattern) => {
+  const { failures } = checkSubject(t, { "src/subject.ts": oneLiner(comment) });
+  assert.equal(failures.length, 1, JSON.stringify(failures));
+  assert.match(failures[0].why, pattern);
+};
+const passes = (t, comment) => {
+  const { failures, warnings } = checkSubject(t, { "src/subject.ts": oneLiner(comment) });
+  assert.deepEqual([...failures, ...warnings], []);
+};
+
+test("a TODO with no owner or issue fails", (t) => failsWith(t, "// TODO: cache this", /TODO/));
+test("a TODO naming its owner passes", (t) => passes(t, "// TODO(jrs): cache this"));
+test("a TODO naming its issue passes", (t) => passes(t, "// FIXME(#4012): cache this"));
+test("a TODO naming an owner and an issue passes", (t) =>
+  passes(t, "// TODO(jrs, #4012): cache this"));
+test("a TODO naming an area passes", (t) =>
+  passes(t, "// TODO(core follow-up): re-export the marker"));
+test("a TODO whose parentheses name nobody fails", (t) => {
+  for (const marker of ["TODO(#)", "TODO(-)", "TODO(.)"])
+    failsWith(t, `// ${marker}: cache this`, /TODO/);
+});
+test("a TODO linking its issue passes", (t) =>
+  passes(t, "// TODO: cache this, https://github.com/heygen-com/hyperframes/issues/4012"));
+test("a TODO's issue link ending a sentence passes", (t) =>
+  passes(t, "// TODO: cache this, see https://github.com/heygen-com/hyperframes/issues/4012."));
+test("an issue path on another host does not stand in for the issue", (t) => {
+  for (const url of [
+    "https://evil.example/github.com/heygen-com/hyperframes/issues/4012",
+    "https://evil.example/?next=github.com/heygen-com/hyperframes/issues/4012",
+    "https://github.life-white.uk.evil.example/heygen-com/hyperframes/issues/4012",
+  ])
+    failsWith(t, `// TODO: cache this, ${url}`, /TODO/);
+});
+test("a colour on the TODO's own line does not stand in for its issue", (t) =>
+  failsWith(t, "// TODO: fix the border colour #123456", /TODO/));
+test("a sentence that mentions a TODO is prose", (t) =>
+  passes(t, "// This resolves the TODO in the audit."));
+
+test("an unowned TODO on a line the diff did not touch is not graded", (t) => {
+  const { failures } = checkSubject(
+    t,
+    { "src/subject.ts": oneLiner("// TODO: cache this") },
+    {},
+    { kind: "lines", added: new Set([2]) },
+  );
+  assert.deepEqual(failures, []);
+});
+
+test("a URL on a private network fails", (t) =>
+  failsWith(t, "// Dashboard: http://10.0.4.2/grafana", /private host/));
+test("a URL on an internal host fails", (t) =>
+  failsWith(t, "// Runbook: https://wiki.corp/render", /private host/));
+test("a signed URL fails", (t) =>
+  failsWith(
+    t,
+    "// Sample: https://bucket.s3.amazonaws.com/a.mp4?X-Amz-Signature=abc",
+    /signature/,
+  ));
+test("a malformed URL fails", (t) =>
+  failsWith(t, "// Docs: https://exa%mple.com/x", /well-formed/));
+test("a public URL passes", (t) => passes(t, "// Spec: https://www.w3.org/TR/webcodecs/"));
+test("a loopback dev-server URL passes", (t) =>
+  passes(t, "// The dev server listens on http://localhost:5173."));
+test("a URL template passes", (t) => passes(t, "// Resolves to https://${host}/assets/<name>."));
+
+test("copied code without a source link warns and does not fail", (t) => {
+  const { failures, warnings } = checkSubject(t, {
+    "src/subject.ts": oneLiner("// Adapted from the upstream easing implementation."),
+  });
+  assert.deepEqual(failures, []);
+  assert.equal(warnings.length, 1);
+  assert.match(warnings[0].why, /original source/);
+});
+test("copied code with a source link passes", (t) =>
+  passes(t, "// Adapted from https://github.com/d3/d3-ease/blob/main/src/cubic.js."));
+
+const restating = ["// Count label", "const countLabel = 1;", ""].join("\n");
+test("a comment restating the next line warns and does not fail", (t) => {
+  const { failures, warnings } = checkSubject(t, { "src/subject.ts": restating });
+  assert.deepEqual(failures, []);
+  assert.equal(warnings.length, 1);
+  assert.match(warnings[0].why, /restates the next line/);
+});
+test("a comment saying why is not a restatement", (t) => {
+  const { failures, warnings } = checkSubject(t, {
+    "src/subject.ts": [
+      "// One word, never a segment: captions count words.",
+      "const countLabel = 1;",
+      "",
+    ].join("\n"),
+  });
+  assert.deepEqual([...failures, ...warnings], []);
+});
+test("a section divider is not a restatement", (t) => {
+  const { warnings } = checkSubject(t, {
+    "src/subject.ts": ["// ── Count label ──", "const countLabel = 1;", ""].join("\n"),
+  });
+  assert.deepEqual(warnings, []);
+});
+test("the CLI exits 0 on a restating comment and prints the warning", (t) => {
+  const { status, output } = runCli(t, KIT, { ...KIT, "src/subject.ts": restating });
+  assert.equal(status, 0, output);
+  assert.match(output, /restates the next line/);
+});
+test("the CLI fails an unowned TODO the diff adds", (t) => {
+  const { status, output } = runCli(t, KIT, {
+    ...KIT,
+    "src/subject.ts": oneLiner("// TODO: cache this"),
+  });
+  assert.equal(status, 1, output);
+  assert.match(output, /TODO/);
+});
+
+test("a colour elsewhere in the block does not stand in for a TODO's issue", (t) => {
+  const { failures } = checkSubject(t, {
+    "src/subject.ts": [
+      "// TODO: tune the ramp",
+      "// The base colour is #123456.",
+      "export const subject = 1;",
+      "",
+    ].join("\n"),
+  });
+  assert.equal(failures.length, 1);
+  assert.match(failures[0].why, /TODO/);
+});
+test("a public hostname that starts with a private-looking number passes", (t) =>
+  passes(t, "// Edge cache: https://10.cdn.example.com/assets/"));
+test("an ASCII section divider is not a restatement", (t) => {
+  const { warnings } = checkSubject(t, {
+    "src/subject.ts": ["// ---------- count label ----------", "const countLabel = 1;", ""].join(
+      "\n",
+    ),
+  });
+  assert.deepEqual(warnings, []);
+});
+test("a TypeScript private field is code, not a standalone comment", (t) => {
+  const { warnings } = checkSubject(t, {
+    "src/subject.ts": ["  #countLabel = 0; // count label", "  countLabel = 1;", ""].join("\n"),
+  });
+  assert.deepEqual(warnings, []);
+});