Address review: pre-existing comments, AAA markers, TODOs

Three changes from the review at 09e6b2b.

A reindent or a code move no longer resurfaces comments nobody wrote.
git marks a reformatted line as added, so line membership alone reported
pre-existing findings: a whitespace-only reindent of PageImageLocator.java
turned a banner at line 88 into a blocking error, and moving a block does
the same. Findings are now matched against the comment text present at the
base, so only genuinely new comment content reports. Costs one `git show`
per changed file, memoised. Verified on both engines, and a genuinely new
banner in the same file still blocks.

Arrange/Act/Assert and Given/When/Then are exempt from CMT001. 85 of 537
findings were bare `// Assert` markers, all in test files. They restate the
code by the rule's letter and carry real structure, and this is a blocking
rule, so leaving it to be relitigated in every test PR was the wrong
trade. Kept narrow: the marker first and at most four words, so
`// Assert the cap is clamped ...` is still judged on its merits.

CMT009 advises on a TODO that names no issue. 21 of the 25 in the tree
name neither issue nor owner, which makes them the one comment category
demonstrably rotting. Advisory, not blocking, because unlike CMT001/002/005
its false-positive rate here is unmeasured. Anchored at the start of the
comment, after the unanchored version flagged the paragraph describing it.

Doc changes: the contract bound now sits next to the instruction it bounds
rather than 40 lines away in the per-language notes; references are split
by durability with the supplementary-not-load-bearing rule demonstrated
rather than only stated; a TODO section; and an explicit statement that
this is not a push for fewer comments.

Author: ConnorYoh

AGENTS.md

@@ -49,6 +49,12 @@ Two tests before keeping a comment:
 
 A comment block over ~12 lines outside a file or type header is either prose that belongs in `devGuide/`, or a sign the code needs restructuring.
 
+A TODO needs an issue, not an owner: `// TODO(#1234): re-enable the gate once account syncing lands`. A username goes stale and means nothing to an outside contributor. If it is not worth an issue, it is not worth a TODO.
+
+A reference is supplementary, never load-bearing: the comment must survive deleting it. `// See #1234` is a dead end; `// saving first loses every annotation (#6865)` is not. Specs (`RFC 3161`) and CVEs outlast tickets, so prefer them when either applies.
+
+This is not a push for fewer comments. The linter only ever asks you to delete, because deletion is the half it can judge. Contract docs on anything a caller outside the file can reach are the half it cannot, and the half this codebase is short of.
+
 `task comment-lint` enforces the mechanical part of this on the lines you add, and runs inside `task pre-commit`. Full guidance, worked examples, and the rule list: @devGuide/CODE_COMMENTS.md
 
 ## Common Development Commands

devGuide/CODE_COMMENTS.md

@@ -7,13 +7,26 @@ eventually contradict the code, and it dilutes the comments that matter.
 That single rule is the whole standard. The rest of this document is how to apply
 it, and what the linter checks.
 
+This is not a campaign to have fewer comments. Every rule the linter enforces is a
+deletion, because deletion is the half a pattern can judge; the half that matters
+more is the writing, and no linter will ask you for it. The contract docs below are
+the part this codebase is short of.
+
 ## The four jobs a comment can do
 
 **Contract.** What a caller must know that the signature cannot say:
 preconditions, invariants, units, ownership and lifetime, thread-safety, error
-semantics, side effects. Be generous here. This is the one category the codebase
-is short of, not long on. Goes on the type, method, or module as Javadoc, JSDoc,
-or a docstring.
+semantics, side effects. Goes on the type, method, or module as Javadoc, JSDoc, or
+a docstring.
+
+This is the one category the codebase is short of rather than long on, but "be
+generous" is not a checkable instruction, and taken alone it produces the padding
+`CMT007` flags. The bound is the surface, not the volume: **document the contract
+of everything a caller outside the file can reach, and nothing else.** Concretely,
+visible types and members in Java, the `@app/*` seams and exported hooks in
+TypeScript, modules and public functions in Python. Inside that surface, say
+whatever a caller genuinely needs. Outside it, a comment has to earn its place on
+the same terms as any other.
 
 ```java
 /**
@@ -31,8 +44,26 @@ Nothing there is recoverable from reading the method bodies. It states the
 ordering, the failure mode, and a deliberate gap in the threat model.
 
 **Why.** The non-obvious reason the code is shaped this way: the constraint it
-satisfies, the bug it avoids, the alternative that was rejected. Name the ticket,
-CVE, or spec when there is one.
+satisfies, the bug it avoids, the alternative that was rejected.
+
+Point at a source when there is one, and know how long each kind lasts. A **spec**
+is the best reference available and never moves: `RFC 3161`, `ISO 4217`, `RFC 9728
+section 3.1`. A **CVE or GHSA** is immutable too. A **ticket** is the weakest of
+the three, because it can be closed, moved or made private, so it must not be the
+only thing holding the comment up.
+
+That is the rule for all three, not just tickets: **a reference is supplementary,
+never load-bearing.** The comment has to survive deleting it. `// See #1234` is a
+dead end; the same fact with the reason first is not:
+
+```java
+// flatten() reads the annotation list that save() clears, so saving first loses
+// every annotation (#6865).
+document.flatten(annotations);
+```
+
+Delete the `(#6865)` and the comment still tells you everything you need. That is
+the test.
 
 ```java
 // whenComplete runs on the worker thread after the run finishes, so the
@@ -84,13 +115,32 @@ Two of these have a legitimate form worth knowing:
 A block longer than about 12 lines outside a file or type header is usually prose
 that belongs in `devGuide/`, or a sign the code needs restructuring.
 
+## TODOs
+
+A TODO needs something that will eventually close it, which means an issue:
+
+```java
+// TODO(#1234): re-enable the checkout gate once account syncing lands
+```
+
+An owner is not a substitute. A username goes stale the moment someone changes
+team and means nothing to an outside contributor, while an issue outlives both. If
+the work is not worth an issue, it is not worth a TODO, and the honest options are
+to do it now or leave the code as it is.
+
+This is the one comment category demonstrably rotting here today: of 25 TODO,
+FIXME and HACK comments in the tree, 21 name neither an issue nor an owner. Some
+are questions rather than tasks (`// TODO: why do this server side not client?`),
+which is a note to nobody. `CMT009` advises on new ones, at the point where
+opening the issue is cheapest.
+
 ## Per-language notes
 
-**Java.** Javadoc on visible types and members, per the Google Java Style guide
-this repo already formats to. Its §7.3.1 exception applies: omit Javadoc on a
-self-explanatory member where there is genuinely nothing to add, but do not cite
-that to skip something a reader needs. Summary fragments are noun or verb phrases,
-not sentences beginning "This method returns".
+**Java.** Per the Google Java Style guide this repo already formats to. Its
+§7.3.1 exception applies: omit Javadoc on a self-explanatory member where there
+is genuinely nothing to add, but do not cite that to skip something a reader needs.
+Summary fragments are noun or verb phrases, not sentences beginning "This method
+returns".
 
 **TypeScript.** JSDoc where a caller needs the contract, particularly on the
 `@app/*` seams, exported hooks, and anything crossing a layer boundary. Do not
@@ -149,17 +199,25 @@ thing; `task pre-commit:comment-lint:selftest` checks it.
 | --- | --- | --- |
 | `CMT001` | A comment whose words are all already in the code below it | blocks |
 | `CMT002` | Section banners and position markers | blocks |
-| `CMT005` | Three or more commented-out lines of code | blocks |
+| `CMT005` | Three or more consecutive commented-out lines of code | blocks |
 | `CMT003` | `Step N:` and `Then,` narration | advises |
 | `CMT004` | Comments about the change rather than the code | advises |
 | `CMT006` | A comment block over 12 lines outside a header | advises |
 | `CMT007` | Doc tags that restate the signature | advises |
 | `CMT008` | `IMPORTANT:` / `CRITICAL:` with nothing to point at | advises |
+| `CMT009` | A `TODO` / `FIXME` / `HACK` naming no issue or link | advises |
+
+Only three rules block, and which three was decided by running them over this
+repo. The others each have a legitimate form that no pattern can distinguish from
+the bad one, and blocking those would teach people to delete good comments to get a
+build green.
+
+`CMT005` catches runs of three or more, so a single commented-out line passes. That
+is a deliberate trade against false positives on prose, and it means its low count
+is not evidence that the tree is free of dead code.
 
-Only three rules block, and which three was decided by running all eight over
-this repo. The other five each have a legitimate form that no pattern can
-distinguish from the bad one, and blocking those would teach people to delete good
-comments to get a build green.
+Findings are scoped to comment *text* that is new, not just to lines git calls new.
+Reindenting or moving code does not resurface comments you did not write.
 
 Advisory findings print and never fail anything. Fix them when they are right.
 

scripts/lint/comment-lint.mjs

@@ -26,7 +26,17 @@ import { existsSync, readFileSync, readdirSync, writeFileSync } from "node:fs";
 import { dirname, join, relative, resolve } from "node:path";
 import { fileURLToPath } from "node:url";
 
-import { analyse, isExcludedPath, isGenerated, isTestPath, ruleLabel, RULES, SEVERITY } from "./comment-rules.mjs";
+import {
+  analyse,
+  commentBodiesOf,
+  normaliseComment,
+  isExcludedPath,
+  isGenerated,
+  isTestPath,
+  ruleLabel,
+  RULES,
+  SEVERITY,
+} from "./comment-rules.mjs";
 
 const HERE = dirname(fileURLToPath(import.meta.url));
 const REPO = resolve(HERE, "..", "..");
@@ -56,6 +66,12 @@ const OXLINT_CONFIG = "frontend/oxlint.comments.config.ts";
 // vanished. Batching keeps each invocation well under the cap.
 const ARGV_BUDGET = 24_000;
 
+// Memoised base-version comment text, keyed by ref:path. Declared up here with
+// the other module constants because the top-level run starts before the
+// function bodies below it are reached, and a `const` further down would
+// still be in its temporal dead zone.
+const baseComments = new Map();
+
 const argv = process.argv.slice(2);
 const flags = new Set(argv.filter((a) => a.startsWith("--")));
 const positional = argv.filter((a) => !a.startsWith("--") && !isFlagValue(a));
@@ -88,18 +104,23 @@ function resolveScope() {
   // Paths plus --since is how the editor hook asks about one file: lint it, but
   // only the lines this session actually wrote.
   if (paths.length > 0 && flags.has("--since")) {
-    return narrow(diffScope(["diff", "--unified=0", "--no-color", mergeBase(flagValue("--since"))]), paths);
+    const ref = mergeBase(flagValue("--since"));
+    return narrow(diffScope(["diff", "--unified=0", "--no-color", ref], ref), paths);
   }
   if (paths.length > 0) return { mode: "paths", files: paths, added: null };
 
-  if (flags.has("--since")) return diffScope(["diff", "--unified=0", "--no-color", mergeBase(flagValue("--since"))]);
+  if (flags.has("--since")) {
+    const ref = mergeBase(flagValue("--since"));
+    return diffScope(["diff", "--unified=0", "--no-color", ref], ref);
+  }
 
   // Always a working-tree comparison, never `--cached`. Findings are read from
   // the file on disk, so diffing the index instead would pair index line numbers
   // with working-tree content and silently mismatch once the two differ.
   // CI knows the target branch; a developer running this before a commit does not.
   const base = process.env.GITHUB_BASE_REF;
-  return diffScope(["diff", "--unified=0", "--no-color", mergeBase(base ? `origin/${base}` : "HEAD")]);
+  const ref = mergeBase(base ? `origin/${base}` : "HEAD");
+  return diffScope(["diff", "--unified=0", "--no-color", ref], ref);
 }
 
 function mergeBase(ref) {
@@ -111,7 +132,7 @@ function mergeBase(ref) {
   }
 }
 
-function diffScope(args) {
+function diffScope(args, base) {
   let diff;
   try {
     diff = git(args);
@@ -146,7 +167,7 @@ function diffScope(args) {
     added.set(file, allLinesOf(file));
   }
 
-  return { mode: "diff", files: [...added.keys()], added };
+  return { mode: "diff", files: [...added.keys()], added, base };
 }
 
 function untrackedFiles() {
@@ -163,7 +184,7 @@ function allLinesOf(file) {
 function narrow(scope, paths) {
   const wanted = new Set(paths);
   const added = new Map([...scope.added].filter(([file]) => wanted.has(file)));
-  return { mode: "diff", files: [...added.keys()], added };
+  return { mode: "diff", files: [...added.keys()], added, base: scope.base };
 }
 
 function trackedFiles() {
@@ -186,11 +207,50 @@ function collect(scope) {
   if (ts.length > 0) results.push(...lintTypeScript(ts));
 
   if (scope.added) {
-    return results.filter((r) => scope.added.get(r.file)?.has(r.line));
+    const onAddedLine = results.filter((r) => scope.added.get(r.file)?.has(r.line));
+    return onAddedLine.filter((r) => !existedAtBase(r, scope.base));
   }
   return results;
 }
 
+// git marks a reindented or moved line as added, so line membership alone reports
+// comments nobody wrote: a whitespace-only reformat of PageImageLocator.java
+// turned a pre-existing banner into a blocking error. A finding only counts if
+// its comment text is not already in the file at the base.
+//
+// Cost is one `git show` per file, memoised. The one thing it gets wrong is
+// adding a further copy of an already-duplicated comment, which it treats as
+// pre-existing. That is the right way round for a blocking rule.
+
+function existedAtBase(finding, base) {
+  if (!base) return false;
+  // Findings from the oxlint plugin arrive without their comment text, because
+  // they cross a process boundary as a message string. Recover it from the file
+  // on disk at the reported line, which is the same text the rule judged.
+  const body = finding.body ?? currentLineBody(finding);
+  if (!body) return false;
+  const key = `${base}:${finding.file}`;
+  if (!baseComments.has(key)) {
+    let source = "";
+    try {
+      source = git(["show", key]);
+    } catch {
+      // Not in the base at all, so the whole file is new.
+    }
+    baseComments.set(key, commentBodiesOf(source));
+  }
+  return baseComments.get(key).has(body);
+}
+
+function currentLineBody(finding) {
+  try {
+    const line = readFileSync(join(REPO, finding.file), "utf8").split(/\r?\n/)[finding.line - 1];
+    return line === undefined ? "" : normaliseComment(line);
+  } catch {
+    return "";
+  }
+}
+
 function isLintable(file) {
   if (isExcludedPath(file)) return false;