Skip to content

docs(specs): correct the pr-review rubric's make check claim (gofmt -w mutates, and go mod tidy -diff never runs) #74

Description

@bketelsen

Problem

docs/specs/pr-review-rubric.md:21 ("Build gate green") tells a reviewer:

make check passes: gofmt -w leaves no diff, golangci-lint run
(.golangci.yml) reports no issues, go test -v ./... passes — the same
steps as the Lint, Unit Tests, and Verify jobs in
.github/workflows/ci.yml

Two things are wrong with that, and the first one bites.

1. It points reviewers at the mutating target. check is
fmt lint vet test test-coverage-check coverage-check (Makefile:110), and
fmt runs $(GOFMT) -w $(GOFILES) (Makefile:27-28) — it rewrites the
working tree
. A reviewer who runs make check to verify someone else's pull
request silently reformats their own checkout. The non-mutating gate the
rubric should name is make verify (Makefile:97-107), whose own docstring
says "Credential-free, non-mutating gate (what a read-only reviewer runs)" and
which uses gofmt-check (gofmt -l, Makefile:92-95) instead.

2. "the same steps as … the Verify job" is false. The Verify job's tidy
check is go mod tidy -diff, reached through the tidy-diff target
(Makefile:88-90). verify: runs it (Makefile:100); check: never does.
So make check can pass on a tree with an untidy go.mod that the Verify job
rejects — the exact local-green-≠-CI-green failure ADR-0038 exists to prevent.

Found 2026-08-28 while correcting the same family of claim for make ci in
open PR #73; that PR changes AGENTS.md, Makefile, and docs/org-adrs.md
and deliberately does not touch this file, so this is not a duplicate
(gh pr diff 73 --name-only confirms).

The change

Rewrite the "Build gate green" row of docs/specs/pr-review-rubric.md so it
is true of the targets as they are:

  1. Name make verify as the gate a reviewer runs, describing it
    accurately: go mod tidy -diff, gofmt -l (non-mutating), golangci-lint run at the pinned version, go vet, go test ./....
  2. Keep make check in the row only if it is described correctly — as the
    author-side target that rewrites files with gofmt -w and adds the
    coverage floor, and that does not run go mod tidy -diff.
  3. Drop the "same steps as the Lint, Unit Tests, and Verify jobs" equivalence,
    or replace it with the divergences that actually exist. Do not restate the
    job list in a way that will drift again — point at
    .github/workflows/ci.yml.
  4. Leave the Race Detection sentence alone; it is accurate.

Documentation only. Do not change Makefile, any target's behaviour, any
workflow, or any conformance alias (ADR-0001 — edit the canonical file, never
an alias). Do not renumber or restructure the rubric's other rows.

Acceptance criteria

  1. On the pull request head, every CI check is green, including the
    docs-integrity job (node scripts/check-docs.mjs) with
    docs_index_coverage, link_integrity, and symlink_resolution all
    1.000.
  2. grep -n 'gofmt -w' docs/specs/pr-review-rubric.md either returns nothing,
    or every remaining occurrence is on a line that also says the target
    rewrites files — no line presents gofmt -w as a check a reviewer runs.
  3. The rubric names make verify for the reviewer path, and no line claims
    make check runs the same steps as the Verify job.
  4. git diff origin/main --name-only lists only
    docs/specs/pr-review-rubric.md.
  5. make verify passes on the branch and git status --porcelain is empty
    afterwards, demonstrating in passing that verify is the non-mutating one.

Blast radius

Documentation only; no target, workflow, or protected boundary changes. The
claim being corrected is one a reviewer acts on, so getting it wrong in the
other direction (naming a non-mutating target that actually mutates) would be
worse than the current state — verify the targets against the Makefile rather
than against any other doc.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    snowcatQueued for the Snowcat fleet

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions