Idea: a CI check for comment slop (a "handicap" that lets any contributor ship a clean diff)

[ianbmacdonald]

Update (2026-07-12): the shape below has changed. Following @superm1's suggestion in the
comments — a pre-commit rule, with CI flagging pre-commit — the proposal is now local-first:
the rule runs before anything is pushed (so there's no public scorecard and no barrier to entry),
CI only flags that pre-commit ran, and the signals stay deterministic (near-duplicate comment
blocks, over-long blocks, a comment matching the PR description, and the comments-only assertion).
No LLM rewriting diffs in CI — that would feed untrusted PR text to a model emitting code the
author then signs, and it can't know which of N duplicate explanations sits at the right seam.

The original proposal is left unedited below for context.

---

Following on from a Discord thread with @superm1 and @fl0rianr, who both independently
flagged the same thing on my recent PRs: too many comments, too extensive, the same idea
re-explained several times over.

They're right, and I don't think it's a me problem — I think it's a structural one that's
going to get more common, so it may be worth a mechanical answer rather than repeated
reviewer patience.

The problem

AGENTS.md already states the rule plainly: default to no comments; only explain a
non-obvious WHY; never write WHAT-comments. The rule isn't missing. It's just not
enforced, and in practice it gets read and then ignored — especially by AI-assisted
contributors, where verbosity is the default failure mode.

The cost lands on maintainers, who end up either reading past it or fixing it at merge.

What I found when I actually measured it

I cleaned up my three open PRs as a sample set. In every single case the problem was not
one over-written comment — it was one concept explained four or five times in different
places:

PR	added comment lines	comment:code
#2588	123 → 63	21% → 12%
#1706	26 → 10	25% → 12%
#1536	51 → 30	23% → 15%

In #2588, the rationale for one flag (--enforce-eager) was restated in five separate
places. In #1536, a 25-line block comment in the source turned out to be the PR description
pasted into the code.

The bit that makes this safe to automate

A comment cleanup changes no code, and that is mechanically provable, not a promise:

strip all comments + blank lines from both revisions -> assert byte-identical

I ran exactly that check on all three PRs before pushing. It's what let @fl0rianr say
"if you only changed comments you can merge" without re-reviewing the diff. Cheap,
deterministic, no judgement involved.

Proposal: a check, not a rewriter

I'd start with something non-destructive and deterministic — it never edits anyone's code:

1. Density warning — comment:code ratio on the added lines of a diff, warn past a
   threshold.
2. Repeated-explanation flag — the same concept explained at N+ sites (this was the
   actual root cause in all three of my PRs, and it's the one a human reviewer notices
   and resents).
3. Comments-only assertion — for a PR that claims to be a comment cleanup, prove it
   (the check above). Makes those trivially re-approvable.

Deliberately not proposing that CI rewrite anyone's diff. That would mean a
nondeterministic rewriter touching contributor code, and the commit would no longer be what
the author signed. The rewriting belongs author-side — a local hook the human reviews and
accepts, the same way I did this by hand.

The stripped prose isn't worthless, it's just misplaced: it belongs in the PR description or
a discussion thread, where depth is welcome and readers opt in.

Honest problems

• Judgement isn't mechanical. Telling a load-bearing WHY from a re-explanation needs
  whole-diff context ("this is already explained at a better seam"). A check can flag;
  it shouldn't decide.
• False positives are asymmetric. Deleting a genuine WHY is a silent loss — worse than
  the slop it removed. Which is another argument for warn-not-mutate.
• Thresholds are arbitrary and should probably start advisory (non-blocking) until
  they've been calibrated against real PRs.

Questions

• Is a non-blocking CI warning the right venue, or is this better as a pre-commit hook so it
  never reaches a reviewer at all?
• Would a warning-only check be welcome, or does anything new in CI need to earn its keep
  more than that?
• Is there appetite for the repeated-explanation signal specifically? It's the one that maps
  to what actually annoyed people, but it's also the hardest to compute well.

Happy to build the MVP if there's appetite — @superm1 said to go ahead if it's feasible, and
I think the warn-only version clearly is.

[superm1]

How about a pre-commit rule and then flagging pre-commit in CI? Would that work?

[ianbmacdonald]

yep.. was just drafting that .. resolves the injection risk

[ianbmacdonald]

Yes — and it's a better shape than what I posted. Two things it buys:

It drops the LLM, and with it the injection risk. Everything that actually caused the
mess turns out to be deterministic:

• near-duplicate comment blocks — this is the real root cause. In my PRs it was never one
  over-written comment, it was the same concept explained 4–5 times. The five
  --enforce-eager explanations were near-identical prose, so plain similarity detection
  finds them; no model needed.
• block comments over N lines
• a comment that substantially matches the PR description (in fix: resume downloads across HuggingFace commit hash changes #1536 the PR description had
  literally been pasted into the source as a 25-line block)
• the comments-only assertion — strip comments + blank lines, assert byte-identical — which
  makes a cleanup PR trivially re-reviewable

It flags; the human picks which of the duplicates to keep. That last part isn't
mechanizable — choosing the right seam needs judgement — but flagging the duplication doesn't.

And local-first fixes the barrier-to-entry problem that my version had. A scorecard on the
PR grades people in public; a pre-commit rule catches it before anything is pushed. Nothing
lands, nobody sees it, no red X — just "you explained this three times, pick one." That's much
closer to the handicap idea than what I originally wrote.

One thing worth flagging on the CI half, since it makes your suggestion pay for itself twice:
pre-commit doesn't currently run in CI at all — there's no pre-commit/black reference in
any of the workflows, though AGENTS.md says Black is "enforced in CI". Today 10 of the 60
tracked .py files on main don't pass the pinned Black 26.1.0 (including
.github/scripts/auto_label.py). So wiring pre-commit into CI closes an existing gap on its
own, before the comment rule exists — it'd just want those 10 normalised first, or the check
scoped to changed files.

Happy to put up the pre-commit rule as a PR and let you look at the flagging heuristics.

[superm1]

Go for it

[ianbmacdonald]

Put it up as #2689.

Two things came out of building it that are worth flagging here rather than on the PR:

• The heuristics are the part worth attacking. Two independent model reviews caught a false positive I'd have shipped: # was read as a comment rather than a preprocessor directive, so three files each adding an alphabetised #include block got flagged as slop. That's a false flag on the most routine C++ diff there is — the exact barrier-to-entry this is supposed to avoid. Fixed, with a regression test, but it's the class of thing to keep poking at.
• The pre-commit-in-CI half stands on its own, before any comment rule exists. It's scoped to changed files, so it doesn't trip on the pre-existing Black drift on main.

[ianbmacdonald]

Splitting this into two pieces, with an off-ramp for the second.

Phase 1 — the flagger (#2689). The deterministic slop check + pre-commit hook + CI job, the shape you proposed, @superm1. It fires on every commit and every PR. A wrong flag costs a contributor a few minutes; nothing merges on its say-so. Worth landing now.

Phase 2 — --assert-comments-only (follow-up). Strips comments and blank lines from both revisions and asserts the remaining code is byte-identical. It can't be a pre-commit rule — a normal PR changes code, so it would fail on all of them. It only works as an opt-in check you control: apply a comments-only label, and CI runs the assertion on base..HEAD in a clean checkout. The contributor runs nothing and can't paste a fake "Verified".

The gap it closes is already open here. There's a PR on this repo right now whose description says it removes a WHAT-comment, while its diff is +6/−0: it removes no comment and adds an executable guard. Honest branch drift, caught because someone read the diff. But "the description and the diff disagree" is exactly what nobody re-reads a diff to check, and it's one command to verify mechanically.

Why they're separate: adversarial review has found fifteen distinct ways a code change could slip past the assertion as "comments only" — an indentation shift; a shebang flipped to python3 -O, disabling every assert; a */ formed by line-splicing, which closes a block comment for the compiler but not for the checker. Four were introduced while fixing a previous one. The flagger, over the same reviews, had zero. Different risk; coupling them is a mistake.

The off-ramp. A checker that can be fooled may be worse than none, because it manufactures false confidence. That isn't hypothetical: the first version of this proof stripped // anywhere in a line, so a code change hidden after a URL compared equal — it would have certified a diff that wasn't comments-only. @fl0rianr's review of #2689 found more of the same class. Making it airtight means modelling every language's lexer well enough to never be wrong. If that argument lands, close #2704.

It's up now, while still being hardened, and rounds are still running. I'm not asking anyone to merge it until an independent round comes back empty.

The upside is the handicap framing I opened with. The label is for contributors who haven't earned your trust yet — a newcomer, or an AI-assisted one — whose "just a comment cleanup" claim you'd otherwise verify by reading the whole diff. Apply the label and CI verifies it in one pass: O(1) instead of O(diff). And they graduate out of it: a contributor whose cleanups you merge on sight, you just merge.

Phase 1 stands on its own if you'd rather stop there.