Comment-quality standard, and the gate that enforces it - #7663
Draft
ConnorYoh wants to merge 7 commits into
Draft
Conversation
Writes down what a comment is for, then enforces the mechanical part of it on the lines a branch adds. - devGuide/CODE_COMMENTS.md and an AGENTS.md section: the four jobs a comment can do, the two tests for keeping one, and what not to write. - scripts/lint/comment-rules.mjs: eight rules, shared by both engines. - Two engines, split by file ownership with no overlap. .ts/.tsx go to an oxlint JS plugin so comments come from the parser; .java/.py/.mjs go to a line scanner. scripts/lint/fixtures/ is the corpus that keeps them agreeing. - A PostToolUse hook so an agent sees findings on the file it just wrote, which needed .gitignore to stop excluding all of .claude/. - Wired into task pre-commit, so the git hook and CI both run it. Only CMT001 (restates the code), CMT002 (banners) and CMT005 (dead code) block. The other five advise, because measuring all eight against this repo found a legitimate form of each that no pattern can separate from the bad one.
github-actions
Bot
added
Documentation
Contributor
📦 Tauri Desktop Builds Ready!The desktop applications have been built and are ready for testing. Download Artifacts:🍎 macOS Universal: Download Stirling-PDF-macos-universal (.dmg) - 340.5 MB Built from commit bd2e9fb |
Without argument forwarding there was no way to ask what a whole branch adds: the default compares the working tree against HEAD, which reports nothing once the work is committed. CI was unaffected (it reads GITHUB_BASE_REF), but a local branch check was only reachable by calling node directly.
Two usability problems found while walking through the tests: Passing a base ref as `task comment-lint -- --since origin/main` is not portable. With the npm build of Task the launcher is a PowerShell script, and PowerShell strips the `--` before Task sees it, so Task printed its own usage instead. comment-lint:branch takes no arguments and defaults to origin/main. The lint tasks ran the fixture corpus verbosely before doing any linting, so their output was eleven ok lines followed by the one line anyone wanted, and was indistinguishable from the selftest task. --selftest --quiet says nothing unless a fixture differs. Also stops git's CRLF advice printing once per file on Windows.
--all passed every tracked TS/JS file in one argv: 2,867 paths, 188k of command line, against a Windows limit near 32k. It died with ENAMETOOLONG, and because the catch treats a non-zero exit as normal it returned no findings and said nothing. Every frontend result silently vanished, so the tree-wide count read 1,344 blocking when it is 2,018. A pull request touching enough TS files would have hit the same wall.
oxlint JS plugins are alpha, and their documented failure mode is being skipped silently while oxlint still reports success (oxc-project/oxc#25203). That bug is in the standalone release binary and not the npm package we invoke, but the class of failure is the one that matters for a gate: it reads exactly like clean code. - Assert number_of_rules >= 1 from the oxlint report, so a plugin that did not register is caught rather than treated as a pass. - A broken engine now exits 2 instead of warning and exiting 0. - The hook distinguishes the two: findings exit 2 and are shown to the agent, a broken engine exits 1 with a notice, so a dead plugin is not indistinguishable from clean. A missing oxlint install still just warns and skips, so the hook stays usable before task frontend:install.
Claude Code can only disable all hooks at once, so a developer who did not want this one had no way to keep the others. COMMENT_LINT_HOOK=0 turns it off, settable per developer in .claude/settings.local.json without touching the committed file. Also documents the settings split: personal config goes in settings.local.json, hook entries merge across the two rather than replacing each other, and anyone with a pre-existing hand-written settings.json should copy it first, because git overwrites an ignored file without warning once a commit starts tracking it.
Three changes from the review at 09e6b2b. A reindent or a code move no longer resurfaces comments nobody wrote. git marks a reformatted line as added, so line membership alone reported pre-existing findings: a whitespace-only reindent of PageImageLocator.java turned a banner at line 88 into a blocking error, and moving a block does the same. Findings are now matched against the comment text present at the base, so only genuinely new comment content reports. Costs one `git show` per changed file, memoised. Verified on both engines, and a genuinely new banner in the same file still blocks. Arrange/Act/Assert and Given/When/Then are exempt from CMT001. 85 of 537 findings were bare `// Assert` markers, all in test files. They restate the code by the rule's letter and carry real structure, and this is a blocking rule, so leaving it to be relitigated in every test PR was the wrong trade. Kept narrow: the marker first and at most four words, so `// Assert the cap is clamped ...` is still judged on its merits. CMT009 advises on a TODO that names no issue. 21 of the 25 in the tree name neither issue nor owner, which makes them the one comment category demonstrably rotting. Advisory, not blocking, because unlike CMT001/002/005 its false-positive rate here is unmeasured. Anchored at the start of the comment, after the unanchored version flagged the paragraph describing it. Doc changes: the contract bound now sits next to the instruction it bounds rather than 40 lines away in the per-language notes; references are split by durability with the supplementary-not-load-bearing rule demonstrated rather than only stated; a TODO section; and an explicit statement that this is not a push for fewer comments.
ConnorYoh
force-pushed
the
claude/ai-pr-comment-quality-dd970e
branch
from
August 25, 2026 11:36
72f3b0c to
8135fbb
Compare
Contributor
✅ Backend License Check PassedAll backend dependencies have valid and allowed licenses. The backend license report has been updated successfully. |
ConnorYoh
requested a deployment
to
ci-signing
August 25, 2026 11:40 — with 
GitHub Actions
In progress
GitHub Actions
In progress
ConnorYoh
requested a deployment
to
ci-signing
August 25, 2026 11:40 — with
GitHub Actions
In progress
GitHub Actions
In progress
Contributor
🚀 V2 Auto-Deployment Complete!🔗 Direct Test URL (non-SSL) http://54.175.155.236:7663 🧩 Admin portal included - try it at http://54.175.155.236:7663/portal. This deployment will be automatically cleaned up when the PR is closed. 🔄 Auto-deployed for approved V2 contributors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.mdhad 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.
.ts/.tsx/.mjsgo to an oxlint JS plugin so comments come from the parser rather than a line scan..java/.pygo to a line scanner. Neither reads the other's files..gitignoreto stop excluding all of.claude/, which also removes thegit add -fthe committed skills relied on.task pre-commit, so the git hook and thepre_commit.ymlCI job both get it.The rules
The part worth arguing about. Three block, five advise.
Types,Helpers,State,Handlers, ...)Step N:, orThen,/Next,/Finally,this used to,no longer needed,renamed from,was previously called@param blob - The blob, where the description adds no word the name lacksIMPORTANT:/CRITICAL:/WARNING:with no ticket, URL or CVE in the blockTODO/FIXME/HACKnaming no issue or linkWhich three block was decided by running all eight over this repo, not by how bad the smell sounds. The other five each hit something legitimate that no pattern separates from the bad version:
// Step 2: Wordinglabelling a real wizard step in AddWatermark.tsx, and on e2e specs whose comments mirror a written test procedure. Suppressed in test files entirely.Blocking those would train people to delete good comments to get a build green. CMT008 was narrowed the same way:
NOTE:used to fire and flagged 37 ordinary notes for nothing.If a finding is wrong,
// comment-lint-allow: CMT002on the line above. Rule-specific, no blanket disable.Scoping
Added comment text only, not just lines git calls new. Reindenting a file or moving a block makes git mark untouched comments as added, which reported findings nobody wrote: a whitespace-only reindent of
PageImageLocator.javaturned a pre-existing banner into a blocking error. Findings are matched against the comment text at the base, so only genuinely new content reports. Onegit showper changed file, memoised.Existing tree is untouched.
task pre-commit:comment-lint:allreports it and always exits 0:1,933 blocking and 516 advisory across 718 files. Java
CMT001is 85 lower than first measured: Arrange/Act/Assert and Given/When/Then markers are now exempt, since all 85 were bare// Assertin test files, where they carry test structure rather than restate the line. Clearing that is separate work, by directory.Not in this PR: an advisory LLM review layer for the things no pattern can judge.
On the committed .claude/settings.json
This partly reverts a deliberate decision, so it needs justifying.
.claude/was blanket-ignored by James in c35546a ("Ignore claude dir", 2026-04-07), and the reason is in the history either side of it:.claude/settings.jsonhad been committed by accident twice, both times carrying a personalpermissionsallowlist, once with absolute machine paths (//c/SourceCode/stirling/stirling-pdf-math-agent/...). It was removed in 6e56ac3 and again in 4f85a54, the latter titled "remove machine-specific .claude/settings.json from PR".The blanket ignore was the right call for that problem. What is committed here is a different file: hooks only, no
permissions, nothing machine-specific, 18 lines. Personal permission grants stay where Claude Code already puts them, insettings.local.json, which the new pattern keeps ignored..claude/settings.jsonis the file Claude Code intends to be checked in and shared. Personal config belongs in.claude/settings.local.json, which Claude Code writes to by default and keeps git-ignored. Hook entries merge across the two rather than replacing each other, so nobody's own hooks are lost.Two things worth knowing before this lands:
.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. No conflict, no stash. I reproduced it. In practice the collision set looks empty: across 19 local checkouts here, 13 havesettings.local.jsonand none has a hand-writtensettings.json.COMMENT_LINT_HOOK=0, settable per developer via{ "env": { "COMMENT_LINT_HOOK": "0" } }in local settings. The commit-time gate still applies, so opting out costs the early warning, not the check.On oxlint JS plugins
oxlint itself is stable and already this repo's frontend linter. Its JS plugin API is alpha, which is the actual new dependency here, so the failure mode is handled rather than assumed away:
number_of_rules >= 1from oxlint's own report, and a broken engine exits 2 rather than warning and passing.How to test
Both engines against eleven fixtures in scripts/lint/fixtures/, every line and severity asserted in expected.json. This is what catches a rule change that quietly widened.
clean (27 files in scope).task comment-lintis the same scoped to uncommitted work, which is what pre-commit and CI run.To watch it bite, add
// Is banneraboveexport function isBannerinscripts/lint/comment-rules.mjsand runtask comment-lint: one CMT001, exit 1. The gate covers its own source, which is why these scripts have no section dividers left.The backlog, report-only.
Verified on the pinned oxlint 1.77.0, not only the 1.79 the plugin was prototyped against.