mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-03 05:10:16 +03:00
## The problem AI PRs write comments that restate the line below them, mark sections with box drawing, and narrate the diff. Nothing in the repo said not to, and nothing checked. `AGENTS.md` had one line about comments and it was buried in the Python section. Banners and `Step N:` narration have zero occurrences in the 15 months before Aug 2025, so this is new. ## The fix A written standard, plus a linter that enforces the mechanical part of it on added lines only. - [devGuide/CODE_COMMENTS.md](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/devGuide/CODE_COMMENTS.md) holds the reasoning and worked examples; a section in [AGENTS.md](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/AGENTS.md) holds the operative rules, kept short so they stay in an agent's context. The two are split by kind rather than duplicated, because the same prose in two places drifts. - Rules in [comment-rules.mjs](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs), shared by both engines. - Two engines. `.ts` / `.tsx` / `.mjs` go to an [oxlint JS plugin](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-lint-oxlint-plugin.mjs) so comments come from the parser rather than a line scan; `.java` / `.py` go to a [line scanner](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-lint.mjs). Neither reads the other's files, so they cannot disagree about one file. - Between them they read every comment form the repo writes: `//` and `/* */`, Javadoc and JSDoc, JSX comments, `#`, and Python docstrings. - Runs in `task pre-commit`, so the git hook and the `pre_commit.yml` CI job both get it, and as a Claude Code [`Stop` hook](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-lint-hook.mjs) so an agent fixes the comment inside the turn that wrote it. ## The rules The part worth arguing about. **Every rule blocks.** A rule that only warns is a rule nobody acts on, so a finding you believe is wrong is a bug in the rule: narrow it, or mark the line and say why. | | Fires on | | --- | --- | | [CMT001](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L71) | Every word in the comment already appears in the code below it. Max 6 words, skipped for prose punctuation and for a bare Arrange/Act/Assert marker | | [CMT002](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L92) | 4+ rule or box-drawing characters, or a bare section label from [a fixed list](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L84) (`Types`, `Helpers`, `State`, `Handlers`, ...) | | [CMT003](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L110) | `Step N:` with a separator, or `Then,` / `Next,` / `Finally,`. Suppressed in test files | | [CMT004](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L129) | A comment about the code's own past: `this used to`, `renamed from`, `was previously called`. Suppressed in test files | | [CMT005](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L154) | 3+ consecutive comment lines where 2/3 [parse as code](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L143) | | [CMT006](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L31) | A run of implementation comment over 12 lines, outside the first 5 lines of a file. Doc blocks are exempt, because the standard asks for thorough contracts | | [CMT007](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L180) | A parameter or return description that adds no word its name lacks. Reads Javadoc/JSDoc `@param`, Sphinx `:param name:` and Google `name: description` | | [CMT008](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L239) | An allow directive naming a rule that does not exist, or one that silenced nothing | | [CMT009](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L219) | A `TODO` / `FIXME` / `HACK` naming no issue or link. An owner is not accepted: a username goes stale, an issue outlives it | Each rule carries the readings it deliberately excludes, next to the rule. Those exclusions came from running the rules over this repo, not from taste: `CMT004` does not match a bare "no longer needed" because that is as often about runtime lifecycle as about history, and `CMT003` needs a separator after the number so a wrapped line beginning "step 2 unmounts + remounts the panel" reads as the prose it is. A comment sharing a line with code is judged by the rules that do not depend on the code below it, so a trailing `// TODO fix this` or `/* this used to run before the flush */` still reports, while `50L * 1024 * 1024 // 50 MB` does not. `CMT001` would have been wrong about six in seven trailing comments here, so it stays out of them. If a finding is wrong, `// comment-lint-allow: CMT002` on the line above. Rule-specific, [no blanket disable](https://github.com/Stirling-Tools/Stirling-PDF/blob/claude/ai-pr-comment-quality-dd970e/scripts/lint/comment-rules.mjs#L229). A directive naming a rule that does not exist, or silencing nothing, is itself a `CMT008` failure, so a typo cannot quietly disable a rule and a stale one gets deleted rather than accumulating. No native linter covers `CMT007`. `eslint-plugin-jsdoc`'s `require-param-description`, Checkstyle's `NonEmptyAtclauseDescription` and ruff's D-rules all check that a description exists, not whether it says anything. ## Scoping Added comment **text** only, not lines git calls new. Reindenting a file or moving a block makes git mark untouched comments as added; findings are matched against the comment text at the base, so only genuinely new content reports. The whole file is read and every comment in it evaluated. Only the *reporting* is filtered, so a rule still sees the code a comment introduces, the full run it belongs to, and the base version of the file. Existing tree is untouched. `task pre-commit:comment-lint:all` reports it and always exits 0: | | java | ts/js | py | | --- | --- | --- | --- | | findings | 1,218 | 741 | 204 | 2,163 across 542 files, mostly `CMT002` banners (1,482) and `CMT001` restatements (456). Clearing it is separate work, by directory. Not in this PR: an advisory LLM review layer for the things no pattern can judge. ## Verification Run against [#7494](https://github.com/Stirling-Tools/Stirling-PDF/pull/7494) as CI would, in a throwaway worktree: **two findings on a 78 file, +4,512 line change, both genuine banners, in 952ms**. A whole-file scan of those same files gives 11; the other 9 were withheld because that PR's author did not write them, and they are the `@param teamId the team ID` shape this standard exists to stop. Both scanners blank string and character literals before looking for comment markers, because a partial lex desynchronises everything after it: one apostrophe in a Java comment, or one Python template whose closing quotes start a line, is enough to read dozens of lines of code as a single comment. Two fixtures carry canaries that stop being reported if either engine ever desynchronises again. The [fixture corpus](https://github.com/Stirling-Tools/Stirling-PDF/tree/claude/ai-pr-comment-quality-dd970e/scripts/lint/fixtures) pins all 9 rules against both engines, and `--selftest` fails if the two disagree about the same file. ## Two things reviewers should know **The oxlint JS plugin API is alpha.** oxlint itself is stable and already this repo's frontend linter; the plugin API is the new dependency. Its documented failure mode ([oxc#25203](https://github.com/oxc-project/oxc/issues/25203)) is being skipped silently while oxlint still reports success. That affects the standalone release binary rather than the npm package this invokes, but the class of failure reads exactly like clean code, so the run asserts `number_of_rules >= 1` from oxlint's own report and a broken engine exits 2 rather than passing. If the API ever breaks, the fallback is folding these rules into the line scanner, which already implements all nine for Java and Python. **`.claude/settings.json` is now committed**, carrying the hook and nothing else: 19 lines, no `permissions`, nothing machine-specific. That partly reverts `c35546a212` ("Ignore claude dir"), which existed because this file had twice been committed by accident with a personal `permissions` allowlist, once with absolute machine paths. Personal config still belongs in `.claude/settings.local.json`, which the new pattern keeps ignored, and hook entries merge across the two so nobody's own hooks are lost. If you already hand-wrote a `.claude/settings.json`, copy it somewhere first: that path used to be git-ignored, and git overwrites an ignored file without warning when a commit starts tracking it. Across 19 local checkouts here, 13 have `settings.local.json` and none has a hand-written `settings.json`. To turn the hook off, `{ "env": { "COMMENT_LINT_HOOK": "0" } }` in local settings. Claude Code can only disable all hooks at once, hence the switch. The commit-time gate still applies. ## How to test ```bash task pre-commit:comment-lint:ci ``` The fixture corpus, then the diff. The corpus checks the rules themselves rather than the code under review, so it runs on CI and before a rule change, not on every local commit. ```bash task comment-lint:branch ``` `clean (34 files in scope)`. `task comment-lint` is the same thing scoped to uncommitted work, which is what the git hook and CI run. To watch it bite, add `// Is banner` above `export function isBanner` in `scripts/lint/comment-rules.mjs` and run `task comment-lint`: one `CMT001`, exit 1. The gate covers its own source, which is why these scripts have no section dividers. ```bash task pre-commit:comment-lint:all ``` The standing backlog, report-only. Verified on the pinned oxlint 1.77.0, not only the 1.79 the plugin was prototyped against.
744 lines
27 KiB
JavaScript
744 lines
27 KiB
JavaScript
#!/usr/bin/env node
|
|
|
|
// comment-lint - the comment-quality gate. Standard: devGuide/CODE_COMMENTS.md
|
|
//
|
|
// Owns .java and engine .py directly, and delegates .ts/.tsx to oxlint (see
|
|
// comment-lint-oxlint-plugin.mjs) so the frontend is judged against real comment
|
|
// tokens rather than lines. Both paths share the rules in comment-rules.mjs, so
|
|
// a finding means the same thing whichever engine produced it.
|
|
//
|
|
// node scripts/lint/comment-lint.mjs default: everything this
|
|
// working tree adds over HEAD,
|
|
// or over the target branch on CI
|
|
// node scripts/lint/comment-lint.mjs --since main findings on lines this branch added
|
|
// node scripts/lint/comment-lint.mjs --all whole tree, report only, never fails
|
|
// node scripts/lint/comment-lint.mjs <paths...> those files, every line
|
|
// node scripts/lint/comment-lint.mjs --selftest run the fixture corpus
|
|
// --quiet ...saying nothing unless it fails
|
|
// node scripts/lint/comment-lint.mjs --json machine-readable findings
|
|
//
|
|
// Exits non-zero for any finding on a line in scope: every rule blocks, because a
|
|
// warning is a finding nobody acts on. --all never fails, because the tree still
|
|
// has a backlog; it is the mode for working through it.
|
|
|
|
import { execFileSync } from "node:child_process";
|
|
import { existsSync, readFileSync, readdirSync, writeFileSync } from "node:fs";
|
|
import { dirname, isAbsolute, join, relative, resolve } from "node:path";
|
|
import { fileURLToPath } from "node:url";
|
|
|
|
import {
|
|
analyse,
|
|
commentBodiesOf,
|
|
normaliseComment,
|
|
isExcludedPath,
|
|
isGenerated,
|
|
isTestPath,
|
|
ruleLabel,
|
|
RULES,
|
|
} from "./comment-rules.mjs";
|
|
|
|
const HERE = dirname(fileURLToPath(import.meta.url));
|
|
const REPO = resolve(HERE, "..", "..");
|
|
const FRONTEND = join(REPO, "frontend");
|
|
|
|
// The Python this repo owns and formats: the engine service, plus the helper
|
|
// scripts that pre-commit already runs ruff over. Vendored and sample .py
|
|
// elsewhere in the tree is not ours to restyle.
|
|
const JAVA = /\.java$/;
|
|
const PYTHON = /^(engine|scripts|\.github\/scripts)\/.*\.py$/;
|
|
|
|
// The oxlint engine parses plain JS as happily as TS, so the lint scripts and
|
|
// build tooling are held to the same rules as the app.
|
|
const TYPESCRIPT = /\.(tsx?|mts|cts|mjs|cjs|jsx?)$/;
|
|
|
|
const FIXTURES_REL = "scripts/lint/fixtures/";
|
|
|
|
// oxlint rejects any path containing "..", so it is always run from the repo
|
|
// root and given repo-relative paths. Its config still lives under frontend/,
|
|
// which is what makes `oxlint` and the plugin resolvable from there.
|
|
const OXLINT_BIN = "frontend/node_modules/oxlint/bin/oxlint";
|
|
const OXLINT_CONFIG = "frontend/oxlint.comments.config.ts";
|
|
|
|
// Windows caps a command line near 32k characters, which a whole-tree file list
|
|
// exceeds by a wide margin. Unbatched it dies with ENAMETOOLONG, and silently:
|
|
// oxlint exits non-zero normally, so the error reads as "no findings".
|
|
const ARGV_BUDGET = 24_000;
|
|
|
|
// Every module constant lives in this block. The top-level run below starts
|
|
// before any function body is reached, so a `const` declared further down is
|
|
// still in its temporal dead zone when the first call touches it.
|
|
const baseComments = new Map();
|
|
|
|
// A Java char literal, which is the only thing a single quote can legitimately
|
|
// open: one character, or one escape. An apostrophe in prose never matches, so
|
|
// `/** The approver's team ... */` keeps its closing delimiter. Without this the
|
|
// apostrophe opened a literal that never closed, the `*/` was blanked away, and
|
|
// the scanner read the next 47 lines of code as one comment.
|
|
const CHAR_LITERAL = /^'(\\[btnfr'"\\0]|\\u[0-9a-fA-F]{4}|[^'\\])'/;
|
|
|
|
// A docstring opens the line, optionally behind a string prefix. Anything with
|
|
// code in front of the quotes is a value, not documentation.
|
|
const DOCSTRING_OPEN = /^[rbuf]{0,2}("""|''')/;
|
|
|
|
const argv = process.argv.slice(2);
|
|
const flags = new Set(argv.filter((a) => a.startsWith("--")));
|
|
const positional = argv.filter((a) => !a.startsWith("--") && !isFlagValue(a));
|
|
|
|
if (flags.has("--selftest")) process.exit(runSelfTest());
|
|
if (flags.has("--help")) {
|
|
process.stdout.write(
|
|
readFileSync(fileURLToPath(import.meta.url), "utf8")
|
|
.split("\n")
|
|
.slice(1, 22)
|
|
.join("\n")
|
|
.replace(/^\/\/ ?/gm, "") + "\n",
|
|
);
|
|
process.exit(0);
|
|
}
|
|
|
|
const scope = resolveScope();
|
|
const findings = collect(scope);
|
|
process.exit(publish(findings, scope));
|
|
|
|
// A "scope" is the set of files to look at plus, when the run is diff-based, the
|
|
// set of lines that are new. Reporting a legacy finding in a file someone merely
|
|
// touched is how a gate like this gets switched off, so diff runs filter by line.
|
|
|
|
function resolveScope() {
|
|
if (flags.has("--all")) return { mode: "all", files: trackedFiles(), added: null };
|
|
|
|
const paths = positional.map(toRepoPath);
|
|
|
|
// 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")) {
|
|
const ref = mergeBase(flagValue("--since"));
|
|
return narrow(diffScope(["diff", "--unified=0", "--no-color", ref, "--", ...paths], ref, paths), paths);
|
|
}
|
|
if (paths.length > 0) return { mode: "paths", files: paths, added: null };
|
|
|
|
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;
|
|
const ref = mergeBase(base ? `origin/${base}` : "HEAD");
|
|
return diffScope(["diff", "--unified=0", "--no-color", ref], ref);
|
|
}
|
|
|
|
function mergeBase(ref) {
|
|
try {
|
|
return git(["merge-base", "HEAD", ref]).trim();
|
|
} catch {
|
|
// A shallow clone or a missing remote ref: compare against the ref itself.
|
|
return ref;
|
|
}
|
|
}
|
|
|
|
function diffScope(args, base, paths = null) {
|
|
let diff;
|
|
try {
|
|
diff = git(args);
|
|
} catch (error) {
|
|
// A shallow clone, a detached CI checkout, or a base branch that was never
|
|
// fetched. Degrading to report-only beats failing a build over plumbing.
|
|
warn(`could not resolve a diff (${firstLine(error.stderr ?? error.message)}), so nothing was checked.`);
|
|
return { mode: "all", files: [], added: null };
|
|
}
|
|
|
|
const added = new Map();
|
|
let file = null;
|
|
for (const line of diff.split("\n")) {
|
|
if (line.startsWith("+++ ")) {
|
|
const path = line.slice(4).replace(/^b\//, "").trim();
|
|
file = path === "/dev/null" ? null : path;
|
|
if (file) added.set(file, new Set());
|
|
continue;
|
|
}
|
|
if (!file || !line.startsWith("@@")) continue;
|
|
const hunk = /\+(\d+)(?:,(\d+))?/.exec(line);
|
|
if (!hunk) continue;
|
|
const start = Number(hunk[1]);
|
|
const count = hunk[2] === undefined ? 1 : Number(hunk[2]);
|
|
for (let i = 0; i < count; i++) added.get(file).add(start + i);
|
|
}
|
|
|
|
// git diff never mentions an untracked file, so a brand new one would be waved
|
|
// through entirely. Every line of one is new. Listing every untracked file in
|
|
// the repo costs about as much as the diff, so when the caller already named the
|
|
// paths, only those are asked about.
|
|
for (const file of untrackedFiles(paths)) {
|
|
if (added.has(file)) continue;
|
|
added.set(file, allLinesOf(file));
|
|
}
|
|
|
|
return { mode: "diff", files: [...added.keys()], added, base };
|
|
}
|
|
|
|
function untrackedFiles(paths = null) {
|
|
const args = ["ls-files", "--others", "--exclude-standard"];
|
|
if (paths) args.push("--", ...paths);
|
|
return git(args).split("\n").filter(Boolean);
|
|
}
|
|
|
|
function allLinesOf(file) {
|
|
const path = insideRepo(file);
|
|
if (!path || !existsSync(path)) return new Set();
|
|
const total = readFileSync(path, "utf8").split(/\r?\n/).length;
|
|
return new Set(Array.from({ length: total }, (_, i) => i + 1));
|
|
}
|
|
|
|
function narrow(scope, paths) {
|
|
// A diff that could not be resolved already returned a degraded scope with no
|
|
// added map. Pass it straight through: an empty map here would read as a clean
|
|
// pass rather than as a run that checked nothing.
|
|
if (!scope.added) return scope;
|
|
|
|
const wanted = new Set(paths);
|
|
const added = new Map([...scope.added].filter(([file]) => wanted.has(file)));
|
|
return { mode: "diff", files: [...added.keys()], added, base: scope.base };
|
|
}
|
|
|
|
function trackedFiles() {
|
|
return git(["ls-files"]).split("\n").filter(Boolean);
|
|
}
|
|
|
|
function toRepoPath(path) {
|
|
return relative(REPO, resolve(process.cwd(), path)).replace(/\\/g, "/");
|
|
}
|
|
|
|
function collect(scope) {
|
|
const selected = scope.files.map((f) => f.replace(/\\/g, "/")).filter(isLintable);
|
|
|
|
// A path named on the command line and then dropped has to be said out loud.
|
|
// Reporting "clean" for a file this never opened is the failure mode the rest
|
|
// of this script works to avoid.
|
|
if (scope.mode === "paths") {
|
|
for (const file of scope.files) {
|
|
if (!selected.includes(file.replace(/\\/g, "/"))) warn(`skipped ${file}: not a lintable file inside the repo.`);
|
|
}
|
|
}
|
|
const results = [];
|
|
|
|
for (const file of selected.filter((f) => JAVA.test(f) || PYTHON.test(f))) {
|
|
results.push(...lintLineBased(file));
|
|
}
|
|
|
|
const ts = selected.filter((f) => TYPESCRIPT.test(f));
|
|
if (ts.length > 0) results.push(...lintTypeScript(ts));
|
|
|
|
if (scope.added) {
|
|
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 finding only counts if its comment text is not already
|
|
// in the file at the base.
|
|
//
|
|
// Cost is one `git show` per file, memoised. It gets one case wrong: adding a
|
|
// further copy of an already-duplicated comment reads 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(insideRepo(finding.file), "utf8").split(/\r?\n/)[finding.line - 1];
|
|
return line === undefined ? "" : normaliseComment(line);
|
|
} catch {
|
|
return "";
|
|
}
|
|
}
|
|
|
|
// Everything this tool reads is named by git or by a developer on the command
|
|
// line, so a path outside the repo is a mistake rather than an attack. Resolving
|
|
// through here keeps the contract true: git show and git diff cannot answer for a
|
|
// path outside the work tree, so escaping it only produces confusing output.
|
|
function insideRepo(file) {
|
|
const target = resolve(REPO, file);
|
|
const rel = relative(REPO, target);
|
|
if (rel.length === 0 || rel.startsWith("..") || isAbsolute(rel)) return "";
|
|
return target;
|
|
}
|
|
|
|
function isLintable(file) {
|
|
if (!insideRepo(file)) return false;
|
|
if (isExcludedPath(file)) return false;
|
|
|
|
// The corpus is deliberately full of findings. Only the selftest reads it,
|
|
// and it does so by path rather than through this filter.
|
|
if (file.startsWith(FIXTURES_REL)) return false;
|
|
if (!JAVA.test(file) && !PYTHON.test(file) && !TYPESCRIPT.test(file)) return false;
|
|
return existsSync(insideRepo(file));
|
|
}
|
|
|
|
function lintLineBased(file) {
|
|
const source = readFileSync(insideRepo(file), "utf8");
|
|
if (isGenerated(source)) return [];
|
|
const lines = source.split(/\r?\n/);
|
|
const runs = readRuns(lines, PYTHON.test(file) ? "py" : "java");
|
|
return analyse({ lines, runs, isTestFile: isTestPath(file) }).map((f) => ({ ...f, file }));
|
|
}
|
|
|
|
// Groups comment lines into runs, the same shape the oxlint plugin builds from
|
|
// parser tokens. String literals are blanked first so a `//` inside one is not
|
|
// mistaken for a comment; without that, every URL in a string became a finding.
|
|
function readRuns(lines, language) {
|
|
const runs = [];
|
|
let current = null;
|
|
let inBlock = false;
|
|
let docstring = null;
|
|
|
|
const push = (index, column, body, kind, trailing = false) => {
|
|
const line = index + 1;
|
|
if (!trailing && current && current.endLine === line - 1 && current.kind === kind && !current.trailing) {
|
|
current.lines.push({ line, column, body });
|
|
current.endLine = line;
|
|
return;
|
|
}
|
|
current = { startLine: line, endLine: line, kind, trailing, lines: [{ line, column, body }] };
|
|
runs.push(current);
|
|
if (trailing) current = null;
|
|
};
|
|
|
|
for (let i = 0; i < lines.length; i++) {
|
|
const raw = lines[i];
|
|
const text = blankStrings(raw, language);
|
|
const trimmed = text.trim();
|
|
const column = raw.length - raw.trimStart().length + 1;
|
|
|
|
if (inBlock) {
|
|
push(i, column, stripDocPrefix(raw), "doc");
|
|
if (trimmed.includes("*/")) inBlock = false;
|
|
continue;
|
|
}
|
|
if (trimmed.length === 0) {
|
|
current = null;
|
|
continue;
|
|
}
|
|
if (language === "py") {
|
|
// Every triple-quoted string is tracked, not just the documenting ones.
|
|
// A template assigned to a constant opens mid-line and so is not
|
|
// documentation, but its closing delimiter sits alone on a line and reads
|
|
// exactly like an opener. Ignoring those strings desynchronised the
|
|
// scanner for the rest of the file, and 35 lines of ordinary code were
|
|
// reported as commented-out.
|
|
if (docstring) {
|
|
if (docstring.isDoc) push(i, column, stripDocstringDelimiters(raw), "doc");
|
|
else current = null;
|
|
if (raw.includes(docstring.delimiter)) docstring = null;
|
|
continue;
|
|
}
|
|
if (trimmed.startsWith("#")) {
|
|
push(i, column, raw.trim().replace(/^#+/, ""), "line");
|
|
continue;
|
|
}
|
|
const hashAt = raw.indexOf("#");
|
|
if (hashAt > 0 && !/["']/.test(raw.slice(0, hashAt))) {
|
|
const body = raw.slice(hashAt + 1).trim();
|
|
if (body.length > 0) {
|
|
push(i, hashAt + 1, body, "line", true);
|
|
continue;
|
|
}
|
|
}
|
|
const quoted = tripleQuoted(trimmed);
|
|
if (quoted) {
|
|
if (quoted.isDoc) push(i, column, stripDocstringDelimiters(raw), "doc");
|
|
else current = null;
|
|
if (!quoted.closes) docstring = quoted;
|
|
continue;
|
|
}
|
|
current = null;
|
|
continue;
|
|
}
|
|
if (trimmed.startsWith("/*")) {
|
|
push(i, column, stripDocPrefix(raw), trimmed.startsWith("/**") ? "doc" : "block");
|
|
if (!trimmed.includes("*/")) inBlock = true;
|
|
continue;
|
|
}
|
|
if (trimmed.startsWith("//")) {
|
|
push(i, column, raw.trim().replace(/^\/\/+/, ""), "line");
|
|
continue;
|
|
}
|
|
|
|
// Code first, then a comment. blankStrings has already neutralised any `//`
|
|
// inside a string literal, so this index is a real comment marker. A block
|
|
// comment counts here only when it also closes on this line, matching the
|
|
// oxlint engine: one that runs on has its bulk on lines of its own.
|
|
const trailingLine = text.indexOf("//");
|
|
const trailingBlock = text.indexOf("/*");
|
|
const at =
|
|
trailingLine > 0 ? trailingLine : trailingBlock > 0 && text.includes("*/", trailingBlock) ? trailingBlock : -1;
|
|
if (at > 0) {
|
|
const body = raw
|
|
.slice(at + 2)
|
|
.replace(/\*\/.*$/, "")
|
|
.trim();
|
|
if (body.length > 0) {
|
|
push(i, at + 1, body, "line", true);
|
|
continue;
|
|
}
|
|
}
|
|
current = null;
|
|
}
|
|
|
|
return runs;
|
|
}
|
|
|
|
// The prose inside a docstring line, with the triple quotes and any string
|
|
// prefix taken off so the rules see what a reader sees.
|
|
// Where a triple-quoted string starts on this line, and whether it counts as
|
|
// documentation. It documents when the quotes open the line, allowing a string
|
|
// prefix; a template assigned to a constant opens mid-line and is data, and
|
|
// reading JSON as prose would judge its keys as comments. An odd number of
|
|
// delimiters means the string continues onto the next line.
|
|
function tripleQuoted(trimmed) {
|
|
const found = /("""|''')/.exec(trimmed);
|
|
if (!found) return null;
|
|
const delimiter = found[1];
|
|
const occurrences = trimmed.split(delimiter).length - 1;
|
|
return {
|
|
delimiter,
|
|
isDoc: DOCSTRING_OPEN.test(trimmed),
|
|
closes: occurrences % 2 === 0,
|
|
};
|
|
}
|
|
|
|
function stripDocstringDelimiters(raw) {
|
|
return raw
|
|
.trim()
|
|
.replace(/^[rbuf]{0,2}("""|''')/, "")
|
|
.replace(/("""|''')\s*$/, "")
|
|
.trim();
|
|
}
|
|
|
|
function stripDocPrefix(raw) {
|
|
return raw
|
|
.trim()
|
|
.replace(/^\/\*+/, "")
|
|
.replace(/\*+\/$/, "")
|
|
.replace(/^\*+/, "")
|
|
.trim();
|
|
}
|
|
|
|
// Replaces the contents of string and char literals with spaces, preserving
|
|
// length so columns stay correct. Escapes are honoured so "\"" does not end it.
|
|
function blankStrings(line, language) {
|
|
if (language === "py") return line;
|
|
let out = "";
|
|
let quote = null;
|
|
for (let i = 0; i < line.length; i++) {
|
|
const ch = line[i];
|
|
if (quote) {
|
|
if (ch === "\\") {
|
|
out += " ";
|
|
i++;
|
|
continue;
|
|
}
|
|
out += ch === quote ? ch : " ";
|
|
if (ch === quote) quote = null;
|
|
continue;
|
|
}
|
|
if (ch === '"') {
|
|
quote = ch;
|
|
out += ch;
|
|
continue;
|
|
}
|
|
if (ch === "'") {
|
|
const literal = CHAR_LITERAL.exec(line.slice(i));
|
|
if (!literal) {
|
|
// Prose, not a literal. Leave it alone.
|
|
out += ch;
|
|
continue;
|
|
}
|
|
out += `'${" ".repeat(literal[0].length - 2)}'`;
|
|
i += literal[0].length - 1;
|
|
continue;
|
|
}
|
|
if (ch === "/" && line[i + 1] === "/") return out + line.slice(i);
|
|
out += ch;
|
|
}
|
|
return out;
|
|
}
|
|
|
|
function lintTypeScript(files) {
|
|
if (!existsSync(join(REPO, OXLINT_BIN))) {
|
|
warn("frontend/node_modules/oxlint is missing, so TS/TSX was skipped. Run `task frontend:install`.");
|
|
return [];
|
|
}
|
|
return batch(files, ARGV_BUDGET).flatMap(runOxlint);
|
|
}
|
|
|
|
function batch(files, budget) {
|
|
const batches = [];
|
|
let current = [];
|
|
let size = 0;
|
|
for (const file of files) {
|
|
if (current.length > 0 && size + file.length + 1 > budget) {
|
|
batches.push(current);
|
|
current = [];
|
|
size = 0;
|
|
}
|
|
current.push(file);
|
|
size += file.length + 1;
|
|
}
|
|
if (current.length > 0) batches.push(current);
|
|
return batches;
|
|
}
|
|
|
|
function runOxlint(files) {
|
|
let stdout;
|
|
try {
|
|
// Invoked as `node <bin>` rather than through npx: spawning a .cmd shim on
|
|
// Windows fails with EINVAL unless a shell is used, and a shell would mean
|
|
// quoting every path. It must also be the npm package rather than the
|
|
// standalone release binary, which accepts a jsPlugins config, skips loading
|
|
// it, and still reports success (oxc-project/oxc#25203).
|
|
stdout = execFileSync(process.execPath, [OXLINT_BIN, "--config", OXLINT_CONFIG, "--format=json", ...files], {
|
|
cwd: REPO,
|
|
encoding: "utf8",
|
|
maxBuffer: 1 << 28,
|
|
stdio: ["ignore", "pipe", "pipe"],
|
|
});
|
|
} catch (error) {
|
|
// oxlint exits non-zero whenever it reports something, which is the normal case.
|
|
stdout = error.stdout ?? "";
|
|
if (!stdout.trim()) {
|
|
die(`oxlint failed on ${files.length} file(s): ${firstLine(error.stderr ?? error.message)}`);
|
|
}
|
|
}
|
|
|
|
return parseOxlint(stdout);
|
|
}
|
|
|
|
function firstLine(value) {
|
|
return value.toString().trim().split(/\r?\n/)[0];
|
|
}
|
|
|
|
function parseOxlint(stdout) {
|
|
const start = stdout.indexOf("{");
|
|
if (start < 0) die("oxlint produced no JSON report.");
|
|
let report;
|
|
try {
|
|
report = JSON.parse(stdout.slice(start));
|
|
} catch {
|
|
die("could not parse oxlint JSON output.");
|
|
}
|
|
|
|
// JS plugins are alpha, and their documented failure mode is being skipped
|
|
// silently while oxlint still reports success (oxc-project/oxc#25203).
|
|
// number_of_rules is the report saying whether the plugin's rule was actually
|
|
// registered. Without this check a dead plugin reads exactly like clean code.
|
|
if ((report.number_of_rules ?? 0) < 1) {
|
|
die("oxlint loaded no rules, so the comment plugin did not run. Refusing to report a pass.");
|
|
}
|
|
|
|
return (report.diagnostics ?? []).flatMap((d) => {
|
|
const parsed = /^(CMT\d{3})\s+(\S+):\s*(.*)$/.exec(d.message);
|
|
if (!parsed) return [];
|
|
const [, rule, , detail] = parsed;
|
|
const span = d.labels?.[0]?.span;
|
|
return [
|
|
{
|
|
file: d.filename.replace(/\\/g, "/"),
|
|
line: span?.line ?? 1,
|
|
column: span?.column ?? 1,
|
|
rule,
|
|
detail,
|
|
severity: RULES[rule].severity,
|
|
},
|
|
];
|
|
});
|
|
}
|
|
|
|
function publish(findings, scope) {
|
|
if (flags.has("--json")) {
|
|
process.stdout.write(`${JSON.stringify({ mode: scope.mode, findings }, null, 2)}\n`);
|
|
return 0;
|
|
}
|
|
|
|
if (findings.length === 0) {
|
|
process.stdout.write(`comment-lint: clean (${scope.files.length} file${scope.files.length === 1 ? "" : "s"} in scope)\n`);
|
|
return 0;
|
|
}
|
|
|
|
const byFile = new Map();
|
|
for (const f of findings) {
|
|
if (!byFile.has(f.file)) byFile.set(f.file, []);
|
|
byFile.get(f.file).push(f);
|
|
}
|
|
|
|
for (const [file, group] of [...byFile.entries()].sort()) {
|
|
process.stdout.write(`\n${file}\n`);
|
|
for (const f of group.sort((a, b) => a.line - b.line)) {
|
|
process.stdout.write(` ${String(f.line).padStart(5)} ${ruleLabel(f.rule)} ${f.detail}\n`);
|
|
}
|
|
}
|
|
|
|
process.stdout.write(
|
|
`\ncomment-lint: ${findings.length} finding${findings.length === 1 ? "" : "s"} across ${byFile.size} file${byFile.size === 1 ? "" : "s"}\n`,
|
|
);
|
|
|
|
if (scope.mode === "all") {
|
|
process.stdout.write("Report-only mode: --all never fails, so the standing backlog can be worked through in chunks.\n");
|
|
return 0;
|
|
}
|
|
|
|
process.stdout.write(
|
|
"\nThe standard is devGuide/CODE_COMMENTS.md. A comment must carry information the\n" +
|
|
"code cannot; if a reader could derive it from the code in front of them, delete it.\n" +
|
|
"If a finding is genuinely wrong, put `comment-lint-allow: CMT00X` on the line above.\n",
|
|
);
|
|
return 1;
|
|
}
|
|
|
|
function warn(message) {
|
|
process.stderr.write(`comment-lint: ${message}\n`);
|
|
}
|
|
|
|
// A gate that cannot run must not report a pass. Reserved for the engine being
|
|
// broken, as opposed to absent: a missing oxlint install is handled by skipping
|
|
// with a warning, so the hook stays usable before `task frontend:install`.
|
|
function die(message) {
|
|
process.stderr.write(`comment-lint: ${message}\n`);
|
|
process.exit(2);
|
|
}
|
|
|
|
// The fixture corpus is the contract between the two engines: the same rule set
|
|
// applied to .java/.py by the line scanner and to .ts/.tsx by oxlint, with
|
|
// fixtures/expected.json asserting what each file should produce.
|
|
//
|
|
// Expectations live outside the fixtures on purpose. An in-file marker would sit
|
|
// inside the very comment under test, changing its word count and its run
|
|
// length, so the fixture would stop being an example of the real thing.
|
|
//
|
|
// --selftest compare against expected.json
|
|
// --selftest --update rewrite expected.json from current behaviour
|
|
//
|
|
// A rule change is meant to show up as a reviewable diff in expected.json.
|
|
|
|
function runSelfTest() {
|
|
const dir = join(HERE, "fixtures");
|
|
const expectedPath = join(dir, "expected.json");
|
|
const files = readdirSync(dir)
|
|
.filter((f) => /\.(java|py|ts|tsx)$/.test(f))
|
|
.sort();
|
|
if (files.length === 0) {
|
|
warn("no fixtures found");
|
|
return 1;
|
|
}
|
|
|
|
const asRepoPath = (name) => relative(REPO, join(dir, name)).replace(/\\/g, "/");
|
|
const tsFixtures = files.filter((f) => TYPESCRIPT.test(f));
|
|
const tsFindings = tsFixtures.length > 0 ? lintTypeScript(tsFixtures.map(asRepoPath)) : [];
|
|
|
|
// A skipped engine looks exactly like a clean engine in the snapshot, so
|
|
// refuse to record or compare rather than baking in a false pass.
|
|
if (tsFixtures.length > 0 && tsFindings.length === 0) {
|
|
warn("the TS engine produced nothing, so it did not run. Install frontend deps first.");
|
|
return 1;
|
|
}
|
|
|
|
const actual = {};
|
|
for (const name of files) {
|
|
const found = TYPESCRIPT.test(name)
|
|
? tsFindings.filter((f) => f.file.endsWith(`/${name}`))
|
|
: lintLineBased(asRepoPath(name));
|
|
actual[name] = found
|
|
.map((f) => `${f.line}:${f.rule}:${f.severity}`)
|
|
.sort((a, b) => Number(a.split(":")[0]) - Number(b.split(":")[0]));
|
|
}
|
|
|
|
if (flags.has("--update")) {
|
|
writeFileSync(expectedPath, `${JSON.stringify(actual, null, 2)}\n`);
|
|
process.stdout.write(
|
|
`comment-lint selftest: recorded ${Object.keys(actual).length} fixtures to ${relative(REPO, expectedPath)}\n`,
|
|
);
|
|
return 0;
|
|
}
|
|
|
|
if (!existsSync(expectedPath)) {
|
|
warn("fixtures/expected.json is missing. Run --selftest --update to record it.");
|
|
return 1;
|
|
}
|
|
|
|
// --quiet says nothing unless something is wrong. It is how the lint tasks run
|
|
// the corpus first without burying their own output under eleven ok lines.
|
|
const quiet = flags.has("--quiet");
|
|
const expected = JSON.parse(readFileSync(expectedPath, "utf8"));
|
|
let failures = 0;
|
|
for (const name of files) {
|
|
const want = (expected[name] ?? []).join(" | ");
|
|
const got = actual[name].join(" | ");
|
|
if (want === got) {
|
|
if (!quiet) process.stdout.write(`ok ${name} (${actual[name].length})\n`);
|
|
continue;
|
|
}
|
|
failures++;
|
|
process.stdout.write(`FAIL ${name}\n expected: ${want || "(nothing)"}\n actual: ${got || "(nothing)"}\n`);
|
|
}
|
|
|
|
const stale = Object.keys(expected).filter((n) => !files.includes(n));
|
|
for (const name of stale) {
|
|
failures++;
|
|
process.stdout.write(`FAIL ${name} is in expected.json but the fixture is gone\n`);
|
|
}
|
|
|
|
if (failures > 0) {
|
|
process.stdout.write(
|
|
`\ncomment-lint selftest: ${failures} fixture(s) differ. If intended, rerun with --update and review the diff.\n`,
|
|
);
|
|
return 1;
|
|
}
|
|
if (!quiet) process.stdout.write("\ncomment-lint selftest: both engines match the corpus\n");
|
|
return 0;
|
|
}
|
|
|
|
function isFlagValue(arg) {
|
|
const index = argv.indexOf(arg);
|
|
return index > 0 && argv[index - 1] === "--since";
|
|
}
|
|
|
|
function flagValue(flag) {
|
|
const index = argv.indexOf(flag);
|
|
return argv[index + 1] ?? "origin/main";
|
|
}
|
|
|
|
function git(args) {
|
|
// stderr is captured rather than inherited so git's line-ending advice ("CRLF
|
|
// will be replaced by LF") does not print once per file on Windows. Real
|
|
// failures still surface: execFileSync throws, and the caller reads .stderr.
|
|
return execFileSync("git", args, {
|
|
cwd: REPO,
|
|
encoding: "utf8",
|
|
maxBuffer: 1 << 28,
|
|
stdio: ["ignore", "pipe", "pipe"],
|
|
});
|
|
}
|