mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-02 21:03:34 +03:00
Comment-quality standard, and the gate that enforces it (#7663)
## 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.
This commit is contained in:
ConnorYoh
@@ -0,0 +1,119 @@
|
||||
#!/usr/bin/env node
|
||||
// Claude Code Stop hook: check the comments this turn wrote, before it ends.
|
||||
//
|
||||
// Wired up by .claude/settings.json. Exit 2 stops Claude from finishing and shows
|
||||
// stderr to it, so the comment is fixed inside the same turn and never reaches a
|
||||
// diff, a CI run, or a reviewer.
|
||||
//
|
||||
// Stop rather than PostToolUse, measured over 605 real turns: a run costs the same
|
||||
// whether it looks at one file or twenty-five, because node startup and one git
|
||||
// diff dominate and the TS engine is spawned once for the batch. Per write it was
|
||||
// 40 minutes of hook latency across those turns, and up to 35 seconds inside a
|
||||
// single heavy one; per turn it is under a second, flat. Half of all writes were
|
||||
// to a file already written that turn, so most of that work was repeated.
|
||||
//
|
||||
// To turn it off, set COMMENT_LINT_HOOK=0. Claude Code has no way to disable one
|
||||
// hook (only disableAllHooks, which turns off everyone's), so the opt-out lives
|
||||
// here instead. Per developer, in .claude/settings.local.json:
|
||||
//
|
||||
// { "env": { "COMMENT_LINT_HOOK": "0" } }
|
||||
//
|
||||
// The commit-time gate still applies either way, so opting out costs you the
|
||||
// early warning, not the check.
|
||||
|
||||
import { execFileSync } from "node:child_process";
|
||||
import { existsSync, readFileSync } from "node:fs";
|
||||
import { dirname, resolve } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const HERE = dirname(fileURLToPath(import.meta.url));
|
||||
const REPO = resolve(HERE, "..", "..");
|
||||
|
||||
// Invoked through Task so the taskfile stays the one place that defines how the
|
||||
// linter is called.
|
||||
const TASK_NAME = "pre-commit:comment-lint:hook";
|
||||
const OFF = new Set(["0", "off", "false", "no"]);
|
||||
|
||||
// The linter's own exit codes: 1 when it found something, 2 when its engine could
|
||||
// not run. The hook's codes mean different things, so they are mapped explicitly.
|
||||
const FOUND = 1;
|
||||
const ENGINE_BROKEN = 2;
|
||||
|
||||
if (OFF.has((process.env.COMMENT_LINT_HOOK ?? "").toLowerCase())) process.exit(0);
|
||||
|
||||
const payload = readStdin();
|
||||
|
||||
// Blocking the stop puts Claude back to work, which ends in another stop and
|
||||
// another chance to block. Claude Code sets this flag once a Stop hook has
|
||||
// already blocked in this turn, so a rule the agent cannot satisfy costs one
|
||||
// extra attempt rather than looping. The commit gate still catches whatever
|
||||
// survives.
|
||||
if (payload?.stop_hook_active) process.exit(0);
|
||||
|
||||
const result = run();
|
||||
if (result.status === 0) process.exit(0);
|
||||
|
||||
if (result.status === ENGINE_BROKEN) {
|
||||
process.stderr.write("comment-lint could not run, so comments in this turn were not checked.\n");
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
// Anything else is Task itself failing, which means the check did not happen.
|
||||
if (result.status !== FOUND) {
|
||||
process.stderr.write(`comment-lint did not run (task exit ${result.status}), so comments in this turn were not checked.\n`);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
// The linter's own report already names the file, line, rule and the standard, so
|
||||
// it is passed through rather than rewritten.
|
||||
process.stderr.write(`${result.output.trim()}\n\nFix these before finishing.\n`);
|
||||
process.exit(2);
|
||||
|
||||
function readStdin() {
|
||||
try {
|
||||
return JSON.parse(readFileSync(0, "utf8"));
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
// `task` on PATH is a shell wrapper that boots Node to launch the Go binary the
|
||||
// npm package already ships, which costs about 400ms. Prefer the binary; fall
|
||||
// back to the wrapper when the layout is not one of the ones probed, or when Task
|
||||
// came from somewhere else entirely.
|
||||
function taskCommand() {
|
||||
const nodeDir = dirname(process.execPath);
|
||||
const exe = process.platform === "win32" ? "task.exe" : "task";
|
||||
const candidates = [
|
||||
resolve(nodeDir, "node_modules/@go-task/cli/bin", exe),
|
||||
resolve(nodeDir, "../lib/node_modules/@go-task/cli/bin", exe),
|
||||
resolve(REPO, "node_modules/@go-task/cli/bin", exe),
|
||||
];
|
||||
for (const candidate of candidates) {
|
||||
if (existsSync(candidate)) return { command: candidate, shell: false };
|
||||
}
|
||||
return { command: process.platform === "win32" ? "task.cmd" : "task", shell: process.platform === "win32" };
|
||||
}
|
||||
|
||||
function run() {
|
||||
const { command, shell } = taskCommand();
|
||||
try {
|
||||
// --output=interleaved because the root Taskfile sets `output: prefixed`,
|
||||
// which would put the task name in front of every reported finding.
|
||||
// --exit-code because Task otherwise reports its own 201 for any failed task,
|
||||
// which hides whether the linter found something or could not run.
|
||||
// Task's stderr is captured rather than inherited: it announces its own
|
||||
// "Failed to run task" for any non-zero command, which would reach Claude
|
||||
// alongside the findings and read as a tooling error.
|
||||
const output = execFileSync(command, [TASK_NAME, "--silent", "--output=interleaved", "--exit-code"], {
|
||||
cwd: REPO,
|
||||
encoding: "utf8",
|
||||
maxBuffer: 1 << 26,
|
||||
shell,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
});
|
||||
return { status: 0, output };
|
||||
} catch (error) {
|
||||
return { status: error.status ?? -1, output: error.stdout ?? "" };
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,126 @@
|
||||
// oxlint JS plugin: the comment-quality rules for .ts and .tsx.
|
||||
//
|
||||
// This engine owns the frontend outright; comment-lint.mjs never scans TS. The
|
||||
// reason to run oxlint here rather than scan lines is that comment tokens come
|
||||
// from the parser, so a `//` inside a string or regex is not a comment, JSX
|
||||
// `{/* … */}` is, and positions are exact. Rule decisions themselves live in
|
||||
// comment-rules.mjs, shared with the Java/Python engine.
|
||||
//
|
||||
// Reported as one rule with the CMT id in the message, because oxlint config
|
||||
// severity is per rule name and every finding here shares one on/off switch.
|
||||
//
|
||||
// Enabled by frontend/oxlint.comments.config.ts. `context.report` needs
|
||||
// `node.range`; passing start/end throws.
|
||||
|
||||
import { analyse, isTestPath, isGenerated, isExcludedPath, ruleLabel } from "./comment-rules.mjs";
|
||||
|
||||
const comments = {
|
||||
create(context) {
|
||||
return {
|
||||
"Program:exit"() {
|
||||
const sourceCode = context.sourceCode;
|
||||
const filename = context.filename ?? context.getFilename?.() ?? "";
|
||||
if (isExcludedPath(filename)) return;
|
||||
|
||||
const text = sourceCode.text;
|
||||
if (isGenerated(text)) return;
|
||||
|
||||
const lines = sourceCode.getLines();
|
||||
const tokens = sourceCode.getAllComments();
|
||||
if (tokens.length === 0) return;
|
||||
|
||||
const runs = groupIntoRuns(tokens, lines);
|
||||
const findings = analyse({ lines, runs, isTestFile: isTestPath(filename) });
|
||||
|
||||
// Ranges come from the run entries rather than the enclosing token, so a
|
||||
// finding on the eighth line of a doc block points at that line instead
|
||||
// of at the opening `/**`.
|
||||
const ranges = new Map();
|
||||
for (const run of runs) {
|
||||
for (const entry of run.lines) ranges.set(entry.line, entry.range);
|
||||
}
|
||||
|
||||
for (const finding of findings) {
|
||||
context.report({
|
||||
message: `${ruleLabel(finding.rule)}: ${finding.detail}`,
|
||||
node: { type: "Line", range: ranges.get(finding.line) ?? [0, 1] },
|
||||
});
|
||||
}
|
||||
},
|
||||
};
|
||||
},
|
||||
};
|
||||
|
||||
// Adjacent comment lines with no code between them form one run, which is the
|
||||
// unit the block-length and dead-code rules judge. A comment sharing its line
|
||||
// with code is a trailing note, not part of any run.
|
||||
function groupIntoRuns(tokens, lines) {
|
||||
const runs = [];
|
||||
let current = null;
|
||||
|
||||
for (const token of tokens) {
|
||||
const entries = expand(token, lines);
|
||||
if (entries.length === 0) continue;
|
||||
|
||||
const kind = token.type === "Line" ? "line" : token.value.startsWith("*") ? "doc" : "block";
|
||||
const startLine = entries[0].line;
|
||||
const trailing = entries[0].trailing === true;
|
||||
const contiguous = !trailing && current && startLine === current.endLine + 1 && current.kind === kind && !current.trailing;
|
||||
|
||||
if (contiguous) {
|
||||
current.lines.push(...entries);
|
||||
current.endLine = entries[entries.length - 1].line;
|
||||
continue;
|
||||
}
|
||||
current = { startLine, endLine: entries[entries.length - 1].line, kind, trailing, lines: entries };
|
||||
runs.push(current);
|
||||
|
||||
// Code sits in front of a trailing comment, so nothing can continue it.
|
||||
if (trailing) current = null;
|
||||
}
|
||||
|
||||
return runs;
|
||||
}
|
||||
|
||||
// One entry per physical line, with the leading `*` of a doc block stripped so
|
||||
// the rules see the prose rather than the box drawing around it. Each entry
|
||||
// carries its own source range so findings can be reported where they are.
|
||||
function expand(token, lines) {
|
||||
const start = token.loc.start.line;
|
||||
const column = token.loc.start.column + 1;
|
||||
const before = (lines[start - 1] ?? "").slice(0, token.loc.start.column).trim();
|
||||
|
||||
// Code in front of the comment makes it a trailing note. Marked rather than
|
||||
// dropped, so CMT004 and CMT009 still see it: a TODO is a TODO wherever it
|
||||
// sits. The rules that compare a comment against the code below it stay out,
|
||||
// because a trailing comment usually decodes the line it sits on.
|
||||
//
|
||||
// A block comment counts as trailing only when it also closes on that line.
|
||||
// One that runs on has its bulk on lines of its own, so it is judged as the
|
||||
// block it is.
|
||||
const sameLine = token.loc.start.line === token.loc.end.line;
|
||||
const trailing = before.length > 0 && !before.startsWith("{") && (token.type === "Line" || sameLine);
|
||||
|
||||
if (token.type === "Line") {
|
||||
return [{ line: start, column, body: token.value, range: token.range, trailing }];
|
||||
}
|
||||
|
||||
// token.value is the text between the delimiters, so it begins two chars in.
|
||||
let offset = token.range[0] + 2;
|
||||
return token.value.split("\n").map((raw, index) => {
|
||||
const range = [offset, offset + Math.max(raw.length, 1)];
|
||||
offset += raw.length + 1;
|
||||
return {
|
||||
line: start + index,
|
||||
column: index === 0 ? column : 1,
|
||||
body: raw.replace(/^\s*\*+/, "").trim(),
|
||||
range,
|
||||
trailing,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
export default {
|
||||
meta: { name: "comments" },
|
||||
rules: { quality: comments },
|
||||
};
|
||||
@@ -0,0 +1,743 @@
|
||||
#!/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"],
|
||||
});
|
||||
}
|
||||
@@ -0,0 +1,500 @@
|
||||
// The comment-quality rule set, shared by both engines so a rule means the same
|
||||
// thing everywhere: the oxlint JS plugin (which owns .ts/.tsx, and has real
|
||||
// comment tokens and an AST) and comment-lint.mjs (which owns .java and .py, and
|
||||
// has only lines). Neither engine ever scans the other's files, so the two can
|
||||
// differ in precision without producing contradictory findings on one file.
|
||||
//
|
||||
// The standard these rules enforce is devGuide/CODE_COMMENTS.md. Changing a rule
|
||||
// here without changing that document leaves the repo with two answers.
|
||||
//
|
||||
// Between them the engines read every comment form the repo writes: // and /* */,
|
||||
// Javadoc and JSDoc, JSX comments, # and Python docstrings.
|
||||
|
||||
export const SEVERITY = { ERROR: "error", WARN: "warn" };
|
||||
|
||||
// Every rule blocks. A rule that only warns is a rule nobody acts on, so a
|
||||
// finding that turns out to be wrong is a bug in the rule: narrow it, or mark the
|
||||
// line with comment-lint-allow and say why. Each rule below carries the readings
|
||||
// it deliberately excludes, which is where to start when one misfires.
|
||||
export const RULES = {
|
||||
CMT001: { name: "restates-code", severity: SEVERITY.ERROR },
|
||||
CMT002: { name: "banner", severity: SEVERITY.ERROR },
|
||||
CMT003: { name: "step-narration", severity: SEVERITY.ERROR },
|
||||
CMT004: { name: "diff-narration", severity: SEVERITY.ERROR },
|
||||
CMT005: { name: "dead-code", severity: SEVERITY.ERROR },
|
||||
CMT006: { name: "block-too-long", severity: SEVERITY.ERROR },
|
||||
CMT007: { name: "doc-restates-signature", severity: SEVERITY.ERROR },
|
||||
CMT008: { name: "bad-allow", severity: SEVERITY.ERROR },
|
||||
CMT009: { name: "unowned-todo", severity: SEVERITY.ERROR },
|
||||
};
|
||||
|
||||
export const MAX_BLOCK_LINES = 12;
|
||||
|
||||
// CMT001 compares a comment against the code it introduces. Both sides are
|
||||
// reduced to the same shape first: lowercased, camel/snake/kebab split into
|
||||
// words, stop words and short words dropped. What survives is the information
|
||||
// each side actually carries, so "Handle drag start" and `handleDragStart` land
|
||||
// on the same set and the comment is shown to add nothing.
|
||||
|
||||
const STOP_WORDS = new Set(
|
||||
(
|
||||
"a an the and or but if then else for to of in on at by with from into is are be was were this that these those it its as we our you your do" +
|
||||
" does done use uses used using will would should can could may might not no yes new only also just so such via per each all any some more" +
|
||||
" most other another same when while where which what who how why here there now next finally first second third let const var function" +
|
||||
" return set get"
|
||||
).split(" "),
|
||||
);
|
||||
|
||||
const WORD_RE = /[a-z][a-z0-9]*/g;
|
||||
|
||||
export function contentWords(text) {
|
||||
return (text.toLowerCase().match(WORD_RE) ?? []).filter((w) => w.length > 2 && !STOP_WORDS.has(w));
|
||||
}
|
||||
|
||||
export function identWords(text) {
|
||||
const split = text.replace(/([a-z0-9])([A-Z])/g, "$1 $2").replace(/[_\-.]/g, " ");
|
||||
return contentWords(split);
|
||||
}
|
||||
|
||||
// Sentence punctuation marks prose, which is usually saying something the code
|
||||
// does not. A single trailing full stop does not count.
|
||||
const PROSE_PUNCT = /[.;:?!]/;
|
||||
const MAX_RESTATE_WORDS = 6;
|
||||
|
||||
// Arrange/Act/Assert and Given/When/Then label the shape of a test rather than
|
||||
// describe the line beneath. Exempt only as a bare marker, so
|
||||
// `// Assert the cap is clamped to the tier maximum` is prose and judged on its
|
||||
// merits.
|
||||
const TEST_STRUCTURE = /^(arrange|act|assert|given|when|then)\b/i;
|
||||
const MAX_MARKER_WORDS = 4;
|
||||
|
||||
export function restatesCode(body, codeText) {
|
||||
if (TEST_STRUCTURE.test(body.trim()) && body.trim().split(/\s+/).length <= MAX_MARKER_WORDS) return false;
|
||||
if (PROSE_PUNCT.test(body.replace(/\.$/, ""))) return false;
|
||||
const comment = contentWords(body);
|
||||
if (comment.length === 0 || comment.length > MAX_RESTATE_WORDS) return false;
|
||||
const code = identWords(codeText);
|
||||
if (code.length === 0) return false;
|
||||
|
||||
// Prefix matching either way, so "config" covers "configuration" and vice versa.
|
||||
return comment.every((w) => code.some((k) => k.startsWith(w) || w.startsWith(k)));
|
||||
}
|
||||
|
||||
const RULE_CHARS = /^[=~_*#+\-]{4,}|[─-╿]{4,}|[=~_*+]{4,}$/;
|
||||
const SECTION_LABEL = new RegExp(
|
||||
"^(imports?|exports?|types?|interfaces?|constants?|config|helpers?|utils?|utilities|state|handlers?|callbacks?|effects?" +
|
||||
"|render|rendering|styles?|props?|hooks?|setup|teardown|cleanup|main|public|private|internal|api|queries|mutations" +
|
||||
"|selectors?|actions?|reducers?|components?|fields?|getters?|setters?|lifecycle|boilerplate)" +
|
||||
"\\s*(section|area|block)?\\s*$",
|
||||
"i",
|
||||
);
|
||||
|
||||
export function isBanner(body) {
|
||||
if (RULE_CHARS.test(body.trim())) return true;
|
||||
|
||||
// A label wrapped in decoration is still a label: strip the decoration first.
|
||||
const bare = body
|
||||
.replace(/[=~_*#+\-─-╿]/g, " ")
|
||||
.replace(/\s+/g, " ")
|
||||
.trim();
|
||||
return bare.length > 0 && SECTION_LABEL.test(bare);
|
||||
}
|
||||
|
||||
// A bare "1." is not narration: numbered lists are how a doc block enumerates
|
||||
// conditions or alternatives, and matching them buries the rule in false
|
||||
// positives. Only the explicit step form and sequencing adverbs qualify, and the
|
||||
// number needs a separator after it, so a wrapped line beginning "step 2 unmounts
|
||||
// + remounts the panel" reads as the prose it is.
|
||||
const STEP = /^(step\s*\d+(\.\d+)?\s*[:.)\-]|(then|next|finally|afterwards|lastly)\s*[,:]\s+\S)/i;
|
||||
|
||||
export function isStepNarration(body) {
|
||||
return STEP.test(body.trim());
|
||||
}
|
||||
|
||||
// Only phrases that can be talking about the code's own past. Excluded because
|
||||
// each has an innocent reading that fires constantly:
|
||||
// "used to" alone - "Used to clamp the live line" means "is used to"
|
||||
// "previously" alone - "re-show even if previously dismissed" is runtime state
|
||||
// "was called" - collides with "verify getSession was called"
|
||||
// "left over from" - "no cards left over from the unfiltered grid"
|
||||
const DIFF_NARRATION = new RegExp(
|
||||
"\\b((this|it|we|they|that) used to|used to (be|live|sit)" +
|
||||
"|(this|it|that|which|the (class|method|field|code|file|module|palette|banner)) is no longer (needed|used)" +
|
||||
"|renamed from|was (previously|formerly) (called|named|known)" +
|
||||
"|instead of the old|has been (removed|replaced) )",
|
||||
"i",
|
||||
);
|
||||
const REMOVAL_SUFFIX = /(^|\s)[-(—]\s*(removed|deleted|dropped|no longer needed)\s*\)?\s*$/i;
|
||||
|
||||
export function isDiffNarration(body) {
|
||||
const t = body.trim();
|
||||
return DIFF_NARRATION.test(t) || REMOVAL_SUFFIX.test(t);
|
||||
}
|
||||
|
||||
const CODE_KEYWORD = new RegExp(
|
||||
"^(import|package|public|private|protected|static|final|abstract|class|interface|enum|record|extends|implements" +
|
||||
"|def|async|await|const|let|var|function|export|return|if|else|elif|for|while|do|try|catch|finally|switch|case" +
|
||||
"|throw|new|super|this|@[A-Za-z])\\b",
|
||||
);
|
||||
const STATEMENT_TAIL = /[;{}]\s*$/;
|
||||
const CALL_ONLY = /^[\w.$]+\s*\([^)]*\)\s*;?\s*$/;
|
||||
const ASSIGNMENT = /\S\s*=\s*\S/;
|
||||
|
||||
export function looksLikeCode(line) {
|
||||
const t = line.trim();
|
||||
if (t.length === 0) return false;
|
||||
if (CODE_KEYWORD.test(t)) return true;
|
||||
if (CALL_ONLY.test(t)) return true;
|
||||
return STATEMENT_TAIL.test(t) && ASSIGNMENT.test(t);
|
||||
}
|
||||
|
||||
export const MIN_DEAD_CODE_RUN = 3;
|
||||
const DEAD_CODE_SHARE = 2 / 3;
|
||||
|
||||
export function isDeadCodeRun(bodies) {
|
||||
if (bodies.length < MIN_DEAD_CODE_RUN) return false;
|
||||
const codeish = bodies.filter(looksLikeCode).length;
|
||||
return codeish / bodies.length >= DEAD_CODE_SHARE;
|
||||
}
|
||||
|
||||
// Every documented-parameter form this repo writes, so the rule is not quietly
|
||||
// Javadoc-only:
|
||||
// Javadoc / JSDoc @param blob The blob to download
|
||||
// Sphinx :param blob: The blob to download
|
||||
// Google docstring blob: The blob to download (under an Args: heading)
|
||||
// NumPy style is deliberately absent: it splits the name and the description
|
||||
// across two lines, and there is one instance of it in the tree.
|
||||
const PARAM_TAG = /^@param\s+(?:\{[^}]*\}\s+)?([\w$.]+)\s*-?\s*(.+)$/;
|
||||
const SPHINX_PARAM = /^:(?:param|arg|key)\s+(?:\S+\s+)?([\w.]+)\s*:\s*(.+)$/;
|
||||
const GOOGLE_PARAM = /^([a-z_][\w]*)\s*(?:\([^)]*\))?\s*:\s*(.+)$/;
|
||||
|
||||
const RETURN_TAG = /^@returns?\s+(.+)$/;
|
||||
const SPHINX_RETURN = /^:returns?\s*:\s*(.+)$/;
|
||||
|
||||
// Description adds nothing when every word in it already appears in the thing
|
||||
// being described. `@param blob - The blob to download` is the canonical case.
|
||||
//
|
||||
// No native linter covers this. 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.
|
||||
export function docRestatesSignature(body, ownerName = "") {
|
||||
const t = body.trim();
|
||||
|
||||
for (const pattern of [PARAM_TAG, SPHINX_PARAM, GOOGLE_PARAM]) {
|
||||
const match = pattern.exec(t);
|
||||
if (!match) continue;
|
||||
const [, name, description] = match;
|
||||
// Google form is just `name: description`, which also matches ordinary prose
|
||||
// containing a colon. Require the description to be short and unpunctuated so
|
||||
// "Note: the cap is clamped" is not read as a parameter called "note".
|
||||
if (pattern === GOOGLE_PARAM && /[.;,]/.test(description)) return false;
|
||||
return addsNothing(description, name, 5);
|
||||
}
|
||||
|
||||
const returns = RETURN_TAG.exec(t) ?? SPHINX_RETURN.exec(t);
|
||||
if (returns && ownerName) return addsNothing(returns[1], ownerName, 4);
|
||||
return false;
|
||||
}
|
||||
|
||||
function addsNothing(description, subject, limit) {
|
||||
const words = contentWords(description);
|
||||
if (words.length === 0 || words.length > limit) return false;
|
||||
const known = identWords(subject);
|
||||
return known.length > 0 && words.every((w) => known.some((k) => k.startsWith(w) || w.startsWith(k)));
|
||||
}
|
||||
|
||||
// A TODO with no reference has nothing that will ever close it. An owner is not
|
||||
// accepted in its place: a username goes stale when someone leaves and means
|
||||
// nothing to an outside contributor, while an issue outlives both.
|
||||
//
|
||||
// Anchored at the start, so this catches a comment that *is* a TODO rather than
|
||||
// prose that mentions the word.
|
||||
const TODO_MARKER = /^(TODO|FIXME|HACK|XXX)\b/;
|
||||
|
||||
// What counts as something that will close it: an issue, a link, or a security
|
||||
// advisory. Checked across the whole comment run, so the reference can sit on a
|
||||
// continuation line.
|
||||
const HAS_REFERENCE = /(#\d+|https?:\/\/|CVE-\d|GHSA-|[A-Z]{2,}-\d+)/;
|
||||
|
||||
export function isUnownedTodo(body, runText = body) {
|
||||
return TODO_MARKER.test(body) && !HAS_REFERENCE.test(runText);
|
||||
}
|
||||
|
||||
// A rule id is silenced by `comment-lint-allow: CMT002`, on the comment itself or
|
||||
// on the line above it. There is deliberately no form that disables every rule.
|
||||
//
|
||||
// The whole comment must be the directive. Matching it anywhere in the text meant
|
||||
// prose that merely mentions the syntax silenced a rule, which this file's own
|
||||
// paragraph above did.
|
||||
const DIRECTIVE = /^comment-lint-allow:\s*(.+?)\s*$/i;
|
||||
|
||||
export function isDirective(body) {
|
||||
return DIRECTIVE.test(body.trim());
|
||||
}
|
||||
|
||||
// A directive that names nothing real, or that suppresses nothing, is dead
|
||||
// configuration: it reads as a silenced rule while silencing nothing, and it
|
||||
// blinds the line for whoever inherits it. Reported for the same reason ESLint
|
||||
// has --report-unused-disable-directives and ruff has RUF100.
|
||||
class Allowance {
|
||||
constructor(directives) {
|
||||
this.entries = [];
|
||||
for (const directive of directives) {
|
||||
for (const token of directiveTokens(directive.body)) {
|
||||
this.entries.push({ token, directive, known: token in RULES, used: false });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Called only once a rule has decided it would report, so a directive counts
|
||||
// as used when it actually silenced something. Asking before the rule decided
|
||||
// marked every consulted directive as used, which hid the unused ones.
|
||||
suppresses(rule) {
|
||||
let allowed = false;
|
||||
for (const entry of this.entries) {
|
||||
if (entry.token !== rule) continue;
|
||||
entry.used = true;
|
||||
allowed = true;
|
||||
}
|
||||
return allowed;
|
||||
}
|
||||
|
||||
reportUnused(report) {
|
||||
for (const entry of this.entries) {
|
||||
if (entry.used) continue;
|
||||
const detail = entry.known ? `${entry.token} is allowed here but nothing reported it` : `${entry.token} is not a rule`;
|
||||
report("CMT008", entry.directive.line, entry.directive.column, detail, entry.directive.body);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Every token a directive names, valid or not, so an unknown one is reported
|
||||
// rather than quietly ignored. Matching only real ids would let `CMT999` through
|
||||
// as a silent no-op: it looks like a rule and silences nothing.
|
||||
export function directiveTokens(body) {
|
||||
const match = DIRECTIVE.exec(body.trim());
|
||||
if (!match) return [];
|
||||
return match[1]
|
||||
.split(",")
|
||||
.map((token) => token.trim().toUpperCase())
|
||||
.filter(Boolean);
|
||||
}
|
||||
|
||||
// Generated files carry whatever the generator emits, and editing them to
|
||||
// satisfy a lint rule would be undone on the next regeneration.
|
||||
const GENERATED_MARKER = /AUTO-?GENERATED|@generated|DO NOT EDIT|Code generated by/i;
|
||||
const GENERATED_HEADER_LINES = 10;
|
||||
|
||||
export function isGenerated(source) {
|
||||
return GENERATED_MARKER.test(source.split("\n", GENERATED_HEADER_LINES).join("\n"));
|
||||
}
|
||||
|
||||
export const EXCLUDED_PATHS = [
|
||||
/(^|\/)node_modules\//,
|
||||
/(^|\/)dist(-\w+)?\//,
|
||||
/(^|\/)build\//,
|
||||
/(^|\/)target\//,
|
||||
/(^|\/)vendor\//,
|
||||
/pdfjs/i,
|
||||
/thirdParty/i,
|
||||
/\.min\./,
|
||||
/src-tauri\/gen\//,
|
||||
/public\/locales\//,
|
||||
/\.d\.ts$/,
|
||||
/(^|\/)storybook-static\//,
|
||||
/(^|\/)playwright-report\//,
|
||||
/(^|\/)org\/apache\//,
|
||||
];
|
||||
|
||||
export function isExcludedPath(file) {
|
||||
const normalised = file.replace(/\\/g, "/");
|
||||
return EXCLUDED_PATHS.some((re) => re.test(normalised));
|
||||
}
|
||||
|
||||
// Comment text reduced to what a reader would call "the same comment": trimmed,
|
||||
// whitespace collapsed, comment markers and decoration stripped. Both sides of
|
||||
// the pre-existing check normalise through here so indentation and marker style
|
||||
// cannot make an unchanged comment look new.
|
||||
export function normaliseComment(text) {
|
||||
return String(text)
|
||||
.replace(/^[\s{]*(\/\/+|\/\*+|#+|\*+)/gm, " ")
|
||||
.replace(/\*+\/[\s}]*$/gm, " ")
|
||||
.replace(/\s+/g, " ")
|
||||
.trim()
|
||||
.toLowerCase();
|
||||
}
|
||||
|
||||
// Every comment in a source file, normalised. Deliberately permissive and
|
||||
// language-agnostic: it only ever decides whether a finding is pre-existing, so
|
||||
// over-matching suppresses a duplicate comment and under-matching just reports
|
||||
// something the author can look at.
|
||||
export function commentBodiesOf(source) {
|
||||
const bodies = new Set();
|
||||
for (const raw of source.split(/\r?\n/)) {
|
||||
const marker = /(\/\/+|\/\*+|^\s*\*+|#+)/.exec(raw);
|
||||
if (!marker) continue;
|
||||
const body = normaliseComment(raw.slice(marker.index));
|
||||
if (body.length > 0) bodies.add(body);
|
||||
}
|
||||
return bodies;
|
||||
}
|
||||
|
||||
export function ruleLabel(id) {
|
||||
return `${id} ${RULES[id].name}`;
|
||||
}
|
||||
|
||||
// Both engines funnel into this. They differ only in how they build `runs`: the
|
||||
// oxlint plugin reads real comment tokens, comment-lint.mjs scans lines. Keeping
|
||||
// the rule application here is what stops the two drifting apart.
|
||||
//
|
||||
// A "run" is a group of comment lines with no code between them, which is the
|
||||
// unit CMT005 and CMT006 judge. Shape:
|
||||
// { startLine, kind: "line" | "block" | "doc", lines: [{ line, column, body }] }
|
||||
// `line` is 1-based to match every editor and every diff.
|
||||
|
||||
export function analyse({ lines, runs, isTestFile = false }) {
|
||||
const findings = [];
|
||||
// `body` is the comment's own text, kept alongside the formatted detail so the
|
||||
// caller can ask whether this exact comment already existed before the change.
|
||||
// That is what stops a reindent or a code move reporting comments nobody wrote.
|
||||
const report = (rule, line, column, detail, body) => {
|
||||
if (isTestFile && SUPPRESSED_IN_TESTS.has(rule)) return;
|
||||
findings.push({ rule, line, column, detail, body: normaliseComment(body ?? detail), severity: RULES[rule].severity });
|
||||
};
|
||||
|
||||
for (const run of runs) {
|
||||
// A directive is scaffolding, not content. Leaving it in the run made it two
|
||||
// lines long, and CMT001 only judges a one-line run, so any directive
|
||||
// silenced CMT001 whatever rule it named.
|
||||
const directives = run.lines.filter((l) => isDirective(l.body));
|
||||
const content = run.lines.filter((l) => !isDirective(l.body));
|
||||
const allowed = new Allowance(directives);
|
||||
|
||||
if (content.length === 0) {
|
||||
allowed.reportUnused(report);
|
||||
continue;
|
||||
}
|
||||
|
||||
const bodies = content.map((l) => l.body);
|
||||
const runText = bodies.join("\n");
|
||||
const first = content[0];
|
||||
|
||||
// Only CMT004 and CMT009 judge a trailing comment. The others depend on the
|
||||
// comment introducing the code below it, and a trailing comment sits beside
|
||||
// it: `0x25 // "%PDF"` overlaps in words while adding the decoding, which is
|
||||
// the kind of lower-altitude fact the standard asks for.
|
||||
if (run.trailing) {
|
||||
for (const entry of content) {
|
||||
const body = entry.body.trim();
|
||||
if (body.length === 0) continue;
|
||||
if (isDiffNarration(body) && !allowed.suppresses("CMT004")) {
|
||||
report("CMT004", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
if (isUnownedTodo(body, runText) && !allowed.suppresses("CMT009")) {
|
||||
report("CMT009", entry.line, entry.column, truncate(body), body);
|
||||
}
|
||||
}
|
||||
allowed.reportUnused(report);
|
||||
continue;
|
||||
}
|
||||
|
||||
if (isDeadCodeRun(bodies) && !allowed.suppresses("CMT005")) {
|
||||
report("CMT005", run.startLine, first.column, `${bodies.length} commented-out lines`, runText);
|
||||
allowed.reportUnused(report);
|
||||
continue; // Every other rule would pile onto the same block of dead code.
|
||||
}
|
||||
|
||||
// A doc block is exempt: the standard asks for thorough contracts, so capping
|
||||
// their length would argue with itself. This judges runs of implementation
|
||||
// comment, where an essay means the code needs restructuring.
|
||||
const essay = run.kind !== "doc" && content.length > MAX_BLOCK_LINES && run.startLine > FILE_HEADER_LINES;
|
||||
if (essay && !allowed.suppresses("CMT006")) {
|
||||
report("CMT006", run.startLine, first.column, `${content.length} lines, limit ${MAX_BLOCK_LINES}`, runText);
|
||||
}
|
||||
|
||||
const owner = run.kind === "line" ? "" : nextCodeLine(lines, run);
|
||||
|
||||
for (const entry of content) {
|
||||
const body = entry.body.trim();
|
||||
if (body.length === 0) continue;
|
||||
|
||||
if (isBanner(body) && !allowed.suppresses("CMT002")) {
|
||||
report("CMT002", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
if (isStepNarration(body) && !allowed.suppresses("CMT003")) {
|
||||
report("CMT003", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
if (isDiffNarration(body) && !allowed.suppresses("CMT004")) {
|
||||
report("CMT004", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
if (docRestatesSignature(body, owner) && !allowed.suppresses("CMT007")) {
|
||||
report("CMT007", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
if (isUnownedTodo(body, runText) && !allowed.suppresses("CMT009")) {
|
||||
report("CMT009", entry.line, entry.column, truncate(body), body);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
// CMT001 judges a whole single-line run against the code it introduces, so
|
||||
// a two-line comment that happens to echo one identifier is left alone.
|
||||
// A one-line `/* … */` counts, which is how JSX `{/* Cap editor */}` above
|
||||
// `<CapEditor …>` is caught. A doc block does not: it is a contract, and
|
||||
// CMT007 is the rule that judges those.
|
||||
if (run.kind !== "doc" && content.length === 1) {
|
||||
const entry = first;
|
||||
const body = entry.body.trim();
|
||||
const code = nextCodeLine(lines, run);
|
||||
if (code && !isBanner(body) && restatesCode(body, code) && !allowed.suppresses("CMT001")) {
|
||||
report("CMT001", entry.line, entry.column, `${truncate(body)} -> ${truncate(code)}`, body);
|
||||
}
|
||||
}
|
||||
|
||||
allowed.reportUnused(report);
|
||||
}
|
||||
|
||||
return findings.sort((a, b) => a.line - b.line || a.column - b.column);
|
||||
}
|
||||
|
||||
// A file header is allowed to be as long as it needs to be.
|
||||
const FILE_HEADER_LINES = 5;
|
||||
const DETAIL_WIDTH = 58;
|
||||
|
||||
// Both of these say something real in a test and nothing anywhere else. A
|
||||
// regression test explains itself by describing the old behaviour, and the e2e
|
||||
// specs number their comments to match a written manual test procedure.
|
||||
const SUPPRESSED_IN_TESTS = new Set(["CMT003", "CMT004"]);
|
||||
|
||||
function precedingLine(lines, startLine) {
|
||||
return lines[startLine - 2] ?? "";
|
||||
}
|
||||
|
||||
function nextCodeLine(lines, run) {
|
||||
const commentLines = new Set(run.lines.map((l) => l.line));
|
||||
for (let i = run.startLine; i < lines.length; i++) {
|
||||
const lineNumber = i + 1;
|
||||
if (commentLines.has(lineNumber)) continue;
|
||||
const text = lines[i]?.trim() ?? "";
|
||||
if (text.length === 0) continue;
|
||||
if (text.startsWith("//") || text.startsWith("#") || text.startsWith("*") || text.startsWith("/*")) continue;
|
||||
if (text === "}" || text === "};" || text === ")" || text === ");") return "";
|
||||
return text;
|
||||
}
|
||||
return "";
|
||||
}
|
||||
|
||||
function truncate(text) {
|
||||
const flat = text.replace(/\s+/g, " ").trim();
|
||||
return flat.length > DETAIL_WIDTH ? `${flat.slice(0, DETAIL_WIDTH - 1)}…` : flat;
|
||||
}
|
||||
|
||||
export const TEST_FILE = /\.(test|spec)\.[jt]sx?$|(^|\/)src\/test\/|Test\.java$|Tests\.java$|(^|\/)test_[^/]+\.py$|_test\.py$/;
|
||||
|
||||
export function isTestPath(file) {
|
||||
return TEST_FILE.test(file.replace(/\\/g, "/"));
|
||||
}
|
||||
@@ -0,0 +1,19 @@
|
||||
package fixtures;
|
||||
|
||||
class AaaTest {
|
||||
|
||||
void clampsToTierMaximum() {
|
||||
// Arrange
|
||||
var wallet = walletAt(500);
|
||||
|
||||
// Act
|
||||
var result = clamp(wallet);
|
||||
|
||||
// Assert
|
||||
assertEquals(100, result.cap());
|
||||
|
||||
// Assert the cap is clamped rather than rejected, because the tier
|
||||
// downgrade path relies on it.
|
||||
assertTrue(result.clamped());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,17 @@
|
||||
# comment-lint fixtures
|
||||
|
||||
The contract between the two engines. Each file is a small, realistic example of
|
||||
what a rule fires on, or of something it must leave alone. `expected.json` records
|
||||
what every fixture should produce, down to the line and the severity.
|
||||
|
||||
Expectations live outside the fixtures deliberately: an in-file `EXPECT:` marker
|
||||
would sit inside the comment under test, changing its word count and its run
|
||||
length, so the fixture would stop being an example of the real thing.
|
||||
|
||||
```bash
|
||||
node scripts/lint/comment-lint.mjs --selftest # compare
|
||||
node scripts/lint/comment-lint.mjs --selftest --update # re-record, then review the diff
|
||||
```
|
||||
|
||||
Adding a rule means adding a fixture that fires it and a line in a `clean.*`
|
||||
fixture that must not. A rule with no fixture is a rule nobody can safely change.
|
||||
@@ -0,0 +1,32 @@
|
||||
package fixtures;
|
||||
|
||||
class Allow {
|
||||
|
||||
void keptOnPurpose() {
|
||||
// comment-lint-allow: CMT002
|
||||
// ---------- kept on purpose, this fixture proves the escape hatch ----------
|
||||
run();
|
||||
}
|
||||
|
||||
void unknownToken(Session session) {
|
||||
// comment-lint-allow: FAKE_RULE
|
||||
session.close();
|
||||
}
|
||||
|
||||
void looksLikeARuleButIsNot(Session session) {
|
||||
// comment-lint-allow: CMT999
|
||||
session.close();
|
||||
}
|
||||
|
||||
void allowedButNothingFires(Session session) {
|
||||
// comment-lint-allow: CMT001
|
||||
// Closed once the signing round trip has settled, not before.
|
||||
session.close();
|
||||
}
|
||||
|
||||
void directiveMustNotHideOtherRules(Registry registry) {
|
||||
// comment-lint-allow: CMT002
|
||||
// Clear the registry
|
||||
registry.clear();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,23 @@
|
||||
package fixtures;
|
||||
|
||||
class Apostrophes {
|
||||
|
||||
/** The approver's team, which must match the one this server already belongs to. */
|
||||
Long teamId;
|
||||
|
||||
// The caller's own retry budget applies here; this method does not retry.
|
||||
void submit() {}
|
||||
|
||||
void literals() {
|
||||
char quote = '\'';
|
||||
char newline = '\n';
|
||||
char slash = '/';
|
||||
String path = "a//b";
|
||||
}
|
||||
|
||||
/** Kept last: if the scanner desynchronises above, this stops being seen. */
|
||||
void canary() {
|
||||
// Build document
|
||||
document = build();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,26 @@
|
||||
package fixtures;
|
||||
|
||||
/**
|
||||
* Authority on which filesystem locations a policy may read or write. Fail-closed:
|
||||
* denied entirely under the saas profile, then Stirling's own config directory is
|
||||
* always rejected, then the path must resolve inside an allowed root.
|
||||
*
|
||||
* <p>Compared after normalisation so {@code ..} cannot escape a root. Symlink
|
||||
* escape is not defended: an operator who roots an allowlist on a symlink to a
|
||||
* sensitive location is trusted.
|
||||
*/
|
||||
class Clean {
|
||||
|
||||
/** Returns the normalised absolute path; throws if not permitted. */
|
||||
Path check(Path candidate) {
|
||||
// whenComplete runs on the worker thread after the run finishes, so the
|
||||
// terminal event never races the step events.
|
||||
return candidate.toAbsolutePath().normalize();
|
||||
}
|
||||
|
||||
void sizes() {
|
||||
// Bytes, not KiB: the API contract predates the unit change and callers
|
||||
// still send bytes.
|
||||
long limit = 5_242_880L;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,17 @@
|
||||
/**
|
||||
* Auth/session seam. saas keeps a Supabase web session; desktop keeps a JWT in
|
||||
* the Tauri secure store, and cloud code reads the token through here instead.
|
||||
* Default no-op; saas/ and desktop/ shadow it.
|
||||
*/
|
||||
export interface SessionSeam {
|
||||
/** Bearer access token for authenticated API calls, or null when signed out. */
|
||||
getAccessToken(): Promise<string | null>;
|
||||
}
|
||||
|
||||
export function createSeam(): SessionSeam {
|
||||
return {
|
||||
// Resolves null rather than throwing: a signed-out caller is an ordinary
|
||||
// state here, and every consumer already branches on null.
|
||||
getAccessToken: async () => null,
|
||||
};
|
||||
}
|
||||
@@ -0,0 +1,14 @@
|
||||
package fixtures;
|
||||
|
||||
class DeadCode {
|
||||
|
||||
// private void oldPath(PDDocument document) {
|
||||
// PDPage page = document.getPage(0);
|
||||
// page.setRotation(90);
|
||||
// document.save(target);
|
||||
// }
|
||||
|
||||
void currentPath(PDDocument document) {
|
||||
document.save(target);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,12 @@
|
||||
package fixtures;
|
||||
|
||||
class Docs {
|
||||
|
||||
/**
|
||||
* @param blob the blob
|
||||
* @param timeoutMs how long to wait before abandoning the read; the caller
|
||||
* owns retrying, because only it knows whether the operation is
|
||||
* idempotent
|
||||
*/
|
||||
void download(Blob blob, long timeoutMs) {}
|
||||
}
|
||||
@@ -0,0 +1,36 @@
|
||||
"""Module docstring, which the scanner must see as documentation."""
|
||||
|
||||
|
||||
def download(blob, timeout_ms):
|
||||
"""Fetch the blob.
|
||||
|
||||
Args:
|
||||
blob: The blob
|
||||
timeout_ms: How long to wait before abandoning the read; the caller owns
|
||||
retrying, because only it knows whether the operation is idempotent.
|
||||
|
||||
Returns:
|
||||
The fetched bytes.
|
||||
"""
|
||||
return read(blob)
|
||||
|
||||
|
||||
def sphinx(blob):
|
||||
"""Fetch the blob.
|
||||
|
||||
:param blob: The blob
|
||||
:returns: the sphinx result
|
||||
"""
|
||||
return read(blob)
|
||||
|
||||
|
||||
def payload():
|
||||
# Not documentation: a triple-quoted value, so its contents are data.
|
||||
body = """{"key": "value", "note": "Types"}"""
|
||||
return body
|
||||
|
||||
|
||||
def canary():
|
||||
# Build document
|
||||
document = build()
|
||||
return document
|
||||
@@ -0,0 +1,63 @@
|
||||
{
|
||||
"AaaTest.java": [],
|
||||
"allow.java": [
|
||||
"12:CMT008:error",
|
||||
"17:CMT008:error",
|
||||
"22:CMT008:error",
|
||||
"28:CMT008:error",
|
||||
"29:CMT001:error"
|
||||
],
|
||||
"apostrophes.java": [
|
||||
"20:CMT001:error"
|
||||
],
|
||||
"clean.java": [],
|
||||
"clean.ts": [],
|
||||
"deadcode.java": [
|
||||
"5:CMT005:error"
|
||||
],
|
||||
"docs.java": [
|
||||
"6:CMT007:error"
|
||||
],
|
||||
"docstrings.py": [
|
||||
"8:CMT007:error",
|
||||
"21:CMT007:error",
|
||||
"34:CMT001:error"
|
||||
],
|
||||
"narration.java": [
|
||||
"6:CMT003:error",
|
||||
"9:CMT003:error"
|
||||
],
|
||||
"narration.tsx": [
|
||||
"2:CMT003:error",
|
||||
"5:CMT004:error",
|
||||
"13:CMT001:error"
|
||||
],
|
||||
"restates.java": [
|
||||
"6:CMT001:error",
|
||||
"9:CMT001:error",
|
||||
"17:CMT002:error",
|
||||
"19:CMT002:error"
|
||||
],
|
||||
"restates.py": [
|
||||
"2:CMT001:error",
|
||||
"5:CMT002:error"
|
||||
],
|
||||
"restates.ts": [
|
||||
"2:CMT001:error",
|
||||
"14:CMT002:error",
|
||||
"18:CMT002:error"
|
||||
],
|
||||
"strings.java": [],
|
||||
"templates.py": [
|
||||
"22:CMT001:error"
|
||||
],
|
||||
"todos.java": [
|
||||
"6:CMT009:error"
|
||||
],
|
||||
"trailing.java": [
|
||||
"13:CMT009:error",
|
||||
"14:CMT004:error",
|
||||
"19:CMT009:error",
|
||||
"20:CMT004:error"
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
package fixtures;
|
||||
|
||||
class Narration {
|
||||
|
||||
void export(Document document) {
|
||||
// Step 1: collect the annotations
|
||||
var annotations = document.annotations();
|
||||
|
||||
// Then, flatten them onto the page
|
||||
document.flatten(annotations);
|
||||
|
||||
// IMPORTANT: do not reorder these
|
||||
document.save();
|
||||
|
||||
// No longer needed after the storage migration
|
||||
legacyCleanup();
|
||||
|
||||
// Ordering matters: flatten() reads the annotation list that save()
|
||||
// clears, so a save first loses every annotation. See #6865.
|
||||
document.close();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,17 @@
|
||||
export function Panel() {
|
||||
// Step 1: read the cap
|
||||
const cap = useCap();
|
||||
|
||||
// This used to be initialised by the footer, which mounted after the banner.
|
||||
useConsentBanner();
|
||||
|
||||
// CRITICAL: keep this above the early return
|
||||
useLayoutEffect(() => sync(cap), [cap]);
|
||||
|
||||
return (
|
||||
<div>
|
||||
{/* Cap editor */}
|
||||
<CapEditor cap={cap} />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,24 @@
|
||||
package fixtures;
|
||||
|
||||
class Restates {
|
||||
|
||||
void run(Registry registry, Job job) {
|
||||
// Clear the registry
|
||||
registry.clear();
|
||||
|
||||
// Sanitize filename
|
||||
String safeFilename = sanitizeFilename(job.originalFilename());
|
||||
|
||||
// Wait for the worker to drain before clearing, or an in-flight job
|
||||
// re-registers its temp file after the sweep.
|
||||
registry.awaitQuiescence();
|
||||
}
|
||||
|
||||
// ---------- Internal helpers ----------
|
||||
|
||||
// Types
|
||||
private enum Mode {
|
||||
FAST,
|
||||
SAFE
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,9 @@
|
||||
def build(request):
|
||||
# Get the current status
|
||||
status = request.current_status()
|
||||
|
||||
# ---- helpers ----
|
||||
|
||||
# Truncated to 200 chars because the audit column is varchar(200) and a
|
||||
# longer value fails the insert rather than being trimmed.
|
||||
return status[:200]
|
||||
@@ -0,0 +1,20 @@
|
||||
export function useGrid() {
|
||||
// Handle drag start
|
||||
const handleDragStart = (event: DragStartEvent) => event.active.id;
|
||||
|
||||
// Selection state
|
||||
const selection = new Set<string>();
|
||||
|
||||
// Debounced so a fast drag does not queue a layout pass per pointer move.
|
||||
const updateLayout = debounce(() => measure(), 16);
|
||||
|
||||
return { handleDragStart, selection, updateLayout };
|
||||
}
|
||||
|
||||
// ─── Types ────────────────────────────────────────────────────────────────
|
||||
|
||||
export type Gate = "OFFSITE_PROCESSING" | "AUTOMATION";
|
||||
|
||||
// Helpers
|
||||
|
||||
export function noop() {}
|
||||
@@ -0,0 +1,11 @@
|
||||
package fixtures;
|
||||
|
||||
class Strings {
|
||||
|
||||
// A `//` inside a literal is not a comment, and neither is an escaped quote.
|
||||
void urls() {
|
||||
String docs = "https://example.com/guide";
|
||||
String quoted = "a \" then // not a comment";
|
||||
char slash = '/';
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,24 @@
|
||||
"""Templates that open mid-line, whose closing delimiter starts a line."""
|
||||
|
||||
_TOOL_IO = '''
|
||||
TOOL_IO: dict[ToolEndpoint, ToolIOSpec] = {{
|
||||
{declarations}
|
||||
}}
|
||||
'''
|
||||
|
||||
_HEADER = """
|
||||
# Header emitted into the output file.
|
||||
# Types
|
||||
"""
|
||||
|
||||
|
||||
def resolve(schema):
|
||||
if "$ref" in schema:
|
||||
return lookup(schema["$ref"])
|
||||
return schema
|
||||
|
||||
|
||||
def canary():
|
||||
# Build document
|
||||
document = build()
|
||||
return document
|
||||
@@ -0,0 +1,25 @@
|
||||
package fixtures;
|
||||
|
||||
class Todos {
|
||||
|
||||
void bare() {
|
||||
// TODO: re-enable once account syncing lands
|
||||
skip();
|
||||
}
|
||||
|
||||
void referenced() {
|
||||
// TODO(#1234): re-enable once account syncing lands
|
||||
skip();
|
||||
}
|
||||
|
||||
void linked() {
|
||||
// FIXME: the upstream fix is tracked at https://example.com/issues/9
|
||||
workaround();
|
||||
}
|
||||
|
||||
void mentioned() {
|
||||
// Image placeholders are not scored: their body text is a TODO marker
|
||||
// rather than prose, so scoring it would reward the placeholder.
|
||||
score();
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,22 @@
|
||||
package fixtures;
|
||||
|
||||
class Trailing {
|
||||
|
||||
void decodings() {
|
||||
byte[] header = {0x25, 0x50, 0x44, 0x46}; // "%PDF"
|
||||
long maxSize = 50L * 1024 * 1024; // 50 MB
|
||||
double buffer = 0.10; // 10% headroom
|
||||
int mode = 2; // MB
|
||||
}
|
||||
|
||||
void stillJudged() {
|
||||
boolean supportsSign = false; // TODO make Sign work
|
||||
cleanup(); // this used to run before the flush
|
||||
}
|
||||
|
||||
void blockFormToo() {
|
||||
int mode = 2; /* MB */
|
||||
boolean ready = false; /* TODO wire this up */
|
||||
reset(); /* this used to run before the flush */
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user