ci: gate Go comment caps on changed blocks
Why
Production Go in this repo is 43% comment lines, 43% of the non-blank prod-Go lines added since June were comments, and 22% of prod-Go diff hunks touched only comments, which is where the review nits and merge conflicts live. The style guide already said "comment by exception", but its "keep, at any length" carve-out licensed the five-paragraph doc comments, and nothing mechanical enforced any of it.
- docs/dev/go-style.md adopts the Go Doc Comments model with hard per-site caps: package comments and exported top-level docs 3 lines, unexported top-level docs 1, everything else (including
_test.go) 2. scripts/ci/check-comment-caps.shenforces them as a pre-commit hook and thelint:comment-capsCI job, withtest:comment-caps-gateproving the fixtures fire (the jet-null-wrap four-piece pattern).
Ratchet only: the gate checks only comment blocks the diff touches. Existing oversized blocks stand until an edit touches them, and only that block must shrink. No retroactive cleanup rides here.
Over a cap means delete, not relocate. The surviving comment keeps the one load-bearing claim. Pages under docs/dev/ are written on purpose, so neither the gate's failure text nor the style guide sends overflow there.
What
Three choices a reviewer would not read off the cap table:
- A comment block that is the first non-blank content of a function body continues that function's doc comment and shares its cap. Without it a rejected doc just moves one line down and still reads as a doc. The body block keeps its own 2-line cap too, so nothing loosens on an exported func, and the ratchet counts each half separately so a body-head edit never drags an untouched grandfathered doc over the line.
- Bare
//separator lines no longer count, matching the directive exemption, so a//nolintrationale rides the directive line for free. --basediffs two-dot against the worktree fromgit merge-base "$base" HEAD, replacing"$base"...HEAD. Caps are read from disk, so on a dirty tree the HEAD-coordinate ranges flag blocks that merely slid. That was measured as three false flags after an uncommitted deletion on feat(datastore): dispatch the repository create... (!1618 - merged) • Hayley Swimelar • 19.4. The merge-base keeps the target's own commits out on a branch behind it, and on CI's clean checkout both forms agree.- The CI job picks its base by pipeline type: merged results and merge trains diff
HEAD^1, the merge commit's own record of the target tip plus any train cars ahead, and detached pipelines diffCI_MERGE_REQUEST_DIFF_BASE_SHA. A fork MR runs its detached pipeline in the fork, where origin is the fork and its main can trail this project's, measured at 1247 findings on a fork MR that changes five Go files. The fetched target tip is not a substitute forHEAD^1: a train car's diff from the tip carries the cars ahead of it, and a car whose pipeline config predates the gate has never run it, measured at 4 findings on Go files this MR does not touch when it rode the train behind one.
Size
2892 added and 30 removed reviewable LOC, over the 500 ceiling in docs/dev/development-model.md. The split is 1986 lines of test suite, 666 of gate, 145 of style guide, and 95 of CI, hook, skill, and AGENTS.md wiring. Splitting the gate from its suite is the only cut available and it makes both halves worse: the suite is the only evidence the gate fires, so landing it separately means merging an unproven gate or a suite with nothing to test. The suite is also where most of the growth went, from review finding behaviours it left unpinned.
Cost
Measured at these caps against this tree, with the job scoped to the MR:
- Repo-wide ceiling, if every block were touched: 24,491 findings across 1,632 of 1,658 non-generated Go files.
- Across the 15 most recent Go-touching MRs: median 76 findings, max 229, min 3.
- 93% of those sit on blocks the MR itself added. 7% are pre-existing blocks it merely touched.
- 65% are
test-fileblocks at cap 2.
Around 76 findings on a typical Go MR is the intended number, not a gate defect. The ratchet holds: the volume is overwhelmingly new prose rather than a retroactive scrub.
Test plan
bash scripts/ci/check-comment-caps_test.sh runs 191 assertions over twenty scenarios: every cap site in all three modes, the ratchet (including per-half on the body-head rule), directive and separator exemptions, every skip path, the generated-file skip on a file larger than a pipe buffer, a deletion-only hunk, --cached reading the index while the worktree differs, method receivers, grouped and plain declarations, a nested closer inside a group, a local group inside a function body, blank identifiers, both /* */ arms, the body-head rule across four body-opening shapes, a dirty tree not flagging a block that only slid, a branch behind its target not inheriting its blocks, a subdirectory run still seeing the repo root, and the argument guards. Scenario 12 covers the shapes review found unpinned: a struct field in and out of a group, an odd backtick in a line-leading block comment and in a mid-line one, a brace in mid-line block prose, code following */ on its line, a directive above and below its block, trading a block's last prose line for a directive, and a package comment detached by a blank line. Scenario 13 adds the positive direction of rules only a refute covered, which a mutation sweep found unpinned: the character walk against a backtick inside a double-quoted string, the blank-identifier class, a group opener, a bodyless declaration, a //line directive, and the _test.go row beating the exported one. Scenario 14 covers the diff-header parser in both directions, an added ++ content line and a git-quoted path, plus a group opener carrying a closed block comment and a blank line that does not detach a body head. This MR touches no .go files, so lint:comment-caps passes vacuously. test:comment-caps-gate is the real proof.
Scenario 16 pins the // --- banner carve-out: a divider above a documented declaration costs it nothing, a divider over its own 2-line row is still reported, a divider does not raise the cap of the doc below it, and a divider adjacent to the declaration is that declaration's doc rather than a banner. Scenario 17 pins the per-segment ratchet: an untouched segment is not charged to the segment beside it, an over-cap touched segment reports from its own first line, and a run the diff writes end to end still answers to one cap. Scenario 18 pins which directive position a gofmt'd tree can hold: above a doc comment gofmt moves an own-line directive to the bottom of the block, so editing it brushes the block, while above a body-head block the directive stays outside. Both of its fixtures are gofmt fixed points. Scenario 19 pins the decisions a mutation sweep left unpinned: a detached run does not become the doc comment of the func below it, a /* */ opener after the gap re-attaches the run exactly like a // line, and the body-head half ratchets per segment, so a deletion beside its trailing gap and an untouched body-head segment both go uncharged. A finding that sums segments from both sides of a gap names its span (file.go:3-7), because the block at the named line alone cannot show the count. Twelve of the assertions failed against the head before the tail-reach change: the two body-head ratchet behaviours, the span notation, and the tail-reach decisions (a bare // appended at a run's tail, a separator or directive after its final gap, and the gap blank itself belong to no segment, matching the declaration path, while a directive after a tail separator pulls it back inside the reach). Scenario 20 pins the first-parent property the CI job's base now relies on: from a fetched target tip a train car's diff carries the car ahead and fails the run, while from HEAD^1 a car without Go changes diffs to nothing and a Go-touching car still answers for its own blocks. Scenario 11 gains the unresolvable-base guard the arm leans on: a base with no merge base is a loud exit 2, never a silently green run. The checker itself is unchanged by the train-base change, so its new assertions all pass against the prior head. What moved is the base the CI job passes, plus a loud refusal when a non-detached pipeline's HEAD is not a merge commit.
Context for LLM agents
Non-goals:
- No cleanup of existing oversized comment blocks. Future-only by operator decision. Do not propose a repo-wide comment scrub in review.
- No relocation remedy. Overflow past a cap is deleted, never rehoused in a dev doc or a spec. Do not propose one in review.
_test.gocaps at 2 rather than 1 so the "200, not 201: the tag already existed" scenario-rationale idiom stays legal. The body-head rule applies there too, so one doc line plus a two-line body head is three and fails.
Design notes:
- Cap values are the
capfor()constants inscripts/ci/check-comment-caps.sh. Changing a cap is a one-line edit plus the matching go-style.md table row. - Block classification anchors on the raw next line: top-level decls sit at column 0, so an indented local
varorconstreads as an inline block (cap 2), not a top-level doc. The_test.shpins this. - Group state anchors at column 0 the same way: an indented
)closes a nested call rather than the group, and a local group inside a function never opens it, so local members keep the inline cap. Known edges, both without in-tree instances: the line closing a raw string has string content at column 0, so a)after the backtick still reads as a group closer. The brace-depth counter closes what used to be a second edge here, so a body comment inside a func-literal group member takes the inline cap rather than the group one. Repo-wide the anchoring adds one ratchet-gated finding (internal/datastore/npm_publish_committer.go) and reclassifies seven metrics-file members fromothercap 2 toexportedcap 3. - The body-head rule arms on a column-0
funcwhose code ends in{once a trailing comment is stripped, and on the) T {line closing a multi-line signature. Afuncinside a backtick raw string does not arm it: the character walk that strips strings and comments carries raw-string state across lines. head_flush()counts only the halves the diff touched, so a body-head edit cannot drag an untouched grandfathered doc over the shared cap. Scenario 7 pins both directions.- The generated-file skip tests the
head -5output inside a[[ ]]comparison rather than a pipeline's exit status:headexits early,printftakes SIGPIPE, andpipefailthen reported 141 on any file over a pipe buffer, so the skip had stopped working on exactly the large files it exists for. head_flush()emits at most one finding per site, preferring the merged doc-cap violation over the body block's own inline-cap violation.- The diff base depends on the pipeline type, and the two arms must not be collapsed. A merged-results or merge-train pipeline diffs from
HEAD^1: HEAD is a merge commit whose first parent is the target tip plus any train cars ahead, so the first parent scopes the diff to this MR. The MR diff base would pull in every commit the target gained since the branch diverged, and the target tip fetched fromoriginsweeps in the cars ahead, which have not run the gate when their pipeline config predates it. The arm refuses to run on a non-merge HEAD rather than under-check from the MR's own previous commit. A detached pipeline diffs fromCI_MERGE_REQUEST_DIFF_BASE_SHA, because a fork merge request runs its pipeline inside the fork, whereoriginis the fork, and its HEAD is the bare source tip with no merge commit. All five fork MRs open today run detached. - A
+++line is a file header only before that file's first@@. Inside a hunk the same text is an added content line whose own text starts with++, so the parser tracks hunk state rather than matching the pattern anywhere. Widening the match without that gate aborted the run on a raw string holding++ verbose. - Group state has two independent parts.
ingroupanchors at column 0 on the raw line for opener and closer alike, since gofmt puts a top-level group's parens there.gbracethen separates a member of the group from a field of a struct nested inside it, and floors at 0 so one mis-lexed brace cannot strand the rest of a file. - A
/* */block never reaches the character walk. One that begins a line is counted byinblk; one that opens after code raisesinmidfrom insidestrip()and is skipped without counting, since its prose is not source. Both paths matter: a brace in that prose used to stick the group depth, and a backtick used to blind the rest of the file. A block closing on its own line lexes to a space rather than the string placeholder, so a group opener carrying one still opens its group. - Where a directive sits decides whether the ratchet reaches it, and the asymmetry is deliberate. Above the block it is outside, so editing
//go:generatedoes not brush the prose under it. Below the blockbendcovers it, because trading the last prose line of an over-cap block for a directive has to keep reporting what survives. Excluding it from both sides reads tidier and lets exactly that edit through.