Skip to content

Fail the build on issue numbers and history in comments - #944

Merged
oryan-block merged 3 commits into
agent-friendlyfrom
agent-friendly-p4-comment-lint
Nov 4, 2026
Merged

oryan-block merged 3 commits into
agent-friendlyfrom
agent-friendly-p4-comment-lint

Conversation

@oryan-block

@oryan-block oryan-block commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Agents working in this repo keep repeating the same mistakes, and the repo should change so a change that looks right from one file is right for the whole repo. One of the most corrected ones is comments: issue numbers in code (before #552), and history narration like "keep those schemas building" or "as it did before this check". Review catches it a round late, so this makes the build catch it instead, with an error that says what to do.

A check that fails the build has to be precise though. A false positive teaches agents to delete good comments, so the phrase list only keeps forms that narrate a change.

Description

CommentConventionsTest reads every comment in src/main and src/test and fails on:

  • an issue number or issue/PR link, except the (#N) in // TODO <change> in X.0.0 (#N). Hex colors (#000000), URL fragments (/docs/#123) and ordinals (step #1) don't count
  • history phrases: "used to be", "anymore", "as before", "as it did", "before this check", "before #N", "keep those schemas building", "keeps building old schemas", "those schemas still build", "the old behavior" / "the previous behaviour", and "no longer" followed by a verb of change ("no longer needed", "no longer throws")
  • @since or a version-shaped "since 1.2.0" / "since v2". "since 2.5 seconds" is fine

Plain prose like "returns the list unchanged", "the previously resolved type", "used to build the type", "no longer than 80 characters", "is now cached" or "keeps working on an empty map" passes. Those cases are pinned in the test's own self-tests.

It matches a whole comment block at once, so a phrase split over two // lines is still caught. Strings, chars and backtick names are skipped. Kotlin block comments nest, Java ones don't. If it finds no sources (wrong working directory) it fails instead of passing. A failure looks like this:

1 comment convention violation(s) on 1 line(s):
src/main/kotlin/graphql/kickstart/tools/SchemaClassScanner.kt:127 [history] "as before" in: are still discovered in the same order as before. With includeUnusedTypes the unused types loop below already
    fix: say what the code does and why, not how it got here; move the reason into the PR description; for a false positive add `lint-allow history until YYYY-MM-DD by <name>: <reason>` to the line, at most 90 days out

lint-allow <rule> until YYYY-MM-DD by <name>: <reason> on the line skips that rule for that line, for at most 90 days. A bad marker fails and says why: expired, more than 90 days out, no reason, or an unknown rule. Text only counts as a marker in the full lint-allow <rule> until form, so a comment that just mentions lint-allow is fine. The review of the marker line is the approval.

Master had two comments that break the rules. I deleted both instead of rewording them. The scanner one explained the order of the interface field step, and SchemaClassScannerInterfaceFieldTest already pins that (moving the step first breaks 3 tests, dropping the includeUnusedTypes guard breaks 1). The linkDirective one just restated the name.

It doesn't try to catch comments that repeat the code. The prototype tried a heuristic for that and it caught none of the real ones, so that stays a review thing.

No release note, nothing changes for library users.

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Ran the test at the six revisions the prototype used. It fails at all of them. Flagged lines: 2 at master, 9 before the comment cleanup commit d0147a72, 6 and 8 before the rewords in fb402fa5 and 22418890, 2 each before 7448f2c6 and 76ba3a9b. Every comment block the first version flagged is still flagged, including the issue links, the #552 refs, before #552, "as before", "as it did before this check" and #823. The drop is extra lines inside blocks that are still flagged ("leave that path unchanged" one line under "as before") and the "keeps working with graphql-java 25" TODO, which was the one false positive. mvn clean verify on JDK 17 passes (444 tests), the check takes about 0.1s.

oryan-block and others added 3 commits November 4, 2026 13:56
CommentConventionsTest scans the comments in src/main and src/test and fails on an issue number outside a `// TODO <change> in X.0.0 (#N)`, on history narration such as "as before" or "no longer", and on "@SInCE" or "since X.Y". It matches each comment block as one text, so a phrase split across lines is caught too. Each failure names the file and line, the rule, the matched phrase and the fix.

A `lint-allow <rule> until YYYY-MM-DD by <name>: <reason>` marker on the line suppresses one rule for at most 90 days. An expired, reasonless or malformed marker fails.

Removes the two comments that broke the rules. SchemaClassScannerInterfaceFieldTest already pins the scan order the scanner comment explained.

Co-Authored-By: Claude <noreply@anthropic.com>
Ordinary prose failed the build: "returns the list unchanged", "the previously resolved type", "no longer than 80 characters", "is now cached", "keeps working", "step #1", a #000000 color, "since 2.5 seconds" and a comment that mentions lint-allow. The history list keeps only phrases that narrate a change, "no longer" needs a verb of change, the version rule needs a version-shaped token, and #N skips hex colors, URL fragments and ordinals like "step #1". Text is a marker only in the full `lint-allow <rule> until` form.

Every fix line names the lint-allow escape, and an invalid marker says why: expired, more than 90 days out, no reason, or an unknown rule. Java block comments no longer nest and Java strings have no templates. The test fails when it finds no sources instead of passing, and the header counts violations and lines.

Co-Authored-By: Claude <noreply@anthropic.com>
"keeps building old schemas" slipped through after the precision pass narrowed the keep-building phrase to one word order. Flag keep(s) building next to schemas in either order, and old or previous behavior. Self-tests pin both, plus "used to build the type" as purpose, not history.
@sonarqubecloud

sonarqubecloud Bot commented Nov 4, 2026

Copy link
Copy Markdown

@oryan-block
oryan-block merged commit af9ba4a into agent-friendly Nov 4, 2026
5 checks passed
@oryan-block
oryan-block deleted the agent-friendly-p4-comment-lint branch November 4, 2026 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant