From bef87cc249680fdff5a526175b320e63efc629b0 Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Mon, 31 Aug 2026 13:38:53 -0400 Subject: [PATCH 1/4] ci: lint the issues a change introduces, not nothing at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `golangci-lint` pre-commit hook reports nothing on a CI runner. Its upstream entry is `--new-from-rev HEAD`, which compares the *working tree* against HEAD: exactly right locally, where it names what you are about to commit and `--fix` corrects it, and empty on a runner, where the checkout is clean. Measured on a tree carrying a real errcheck violation, the hook passed while `golangci-lint run` reported two issues on the same tree. `pass_filenames: false` is why diff-scoping the pre-commit *run* cannot help: pre-commit passes no filenames in any mode, so the entry's own base rev is the only thing that decides. The fix has to change the base rev. So the same upstream hook is declared a second time under an alias, with `--new-from-merge-base=origin/main` appended — args are appended to the entry and the later flag wins — and `stages: [manual]` to keep it out of every local commit, where the hook above already does the right thing. No `repo: local` hook, and no second lint authority: same config, same pin, same hook, asked a question it can answer on a clean checkout. The `rev-parse --verify` step is the point, not ceremony. A base ref that does not resolve makes golangci-lint print `0 issues` and exit 0 rather than erroring, so a shallow clone would turn this into a green check that linted nothing — the exact failure being fixed. `fetch-depth: 0` supplies the base branch history the merge base needs, and the assertion fails loudly if it ever goes missing. The comments claiming CI already lints the change against its base described something that was never true; they are corrected here. --- .github/workflows/ci.yml | 18 +++++++++++++++++- .pre-commit-config.yaml | 27 +++++++++++++++++++-------- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 92640d8..7ab8aa2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,6 +36,10 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + # `--new-from-merge-base` needs the base branch's history to find the + # common ancestor; a shallow clone has none of it. + fetch-depth: 0 - uses: actions/setup-go@v6 with: go-version-file: go.mod @@ -47,8 +51,20 @@ jobs: # this project's product, and the hook reporting nothing against a clean # checkout is beside the point — that it *builds and runs* is the thing # under test, and this is the one job that runs the action from the - # checkout. The Lint job still gates the change itself. + # checkout. The step below is what gates the change itself. uses: ./ + - name: Assert the lint base is present + # A base ref that does not resolve makes golangci-lint print `0 issues` + # and exit 0 — measured. Without this, losing `fetch-depth: 0` above + # would turn the lint below into a silent pass, which is the failure + # this whole change exists to end. + run: git rev-parse --verify origin/main >/dev/null + - name: Lint what this change introduces + # Same config, same pin, same hook — asked the narrower question: + # only what this change introduced, not the repository's backlog. + uses: ./ + with: + extra_args: --hook-stage manual golangci-lint-new --all-files # Gating happens inside the jobs, never with a workflow-level `paths:` # filter: a required check that never runs reports as *pending forever* diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0d62148..8c8ef27 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -41,18 +41,27 @@ repos: rev: v2.12.2 hooks: - id: golangci-lint-fmt - # The diff-scoped hook (`--new-from-rev HEAD`), deliberately: this runs on - # every commit and should report what you just wrote, not the - - # repository's whole backlog. CI lints the change against its base. - + # repository's whole backlog. # - # Keep `rev` in lockstep with go.mod's tool pin. + - id: golangci-lint + # The same upstream hook, told which base to diff against. + # `--new-from-merge-base` overrides the entry's `--new-from-rev HEAD` + # (measured), which is the whole reason this works on a clean checkout, + # where the tree and HEAD are identical so the entry finds nothing. + # + # `stages: [manual]` keeps it out of every local commit, where the hook + # above already reports what you are about to write and `--fix` corrects + # it. CI asks for this one by name. - id: golangci-lint + alias: golangci-lint-new + name: golangci-lint (issues this change introduces) + stages: [manual] + args: [--new-from-merge-base=origin/main] + # Secret scanning - repo: https://github.com/gitleaks/gitleaks rev: v8.30.1 @@ -60,5 +69,7 @@ repos: - id: gitleaks exclude: ^build/ -# Note: the whole-repo golangci-lint run stays in CI — the hook above only -# lints what changed since HEAD, which is fast enough for every commit. +# Note: CI runs the `golangci-lint-new` alias above at the manual stage, so a +# pull request is gated on the issues it introduces. Nothing runs the whole +# repository; whole-module linters like `unused` are reported only for code +# the change touched. From 2583c5466b0fce298cdd4aff1733366e8ead6eee Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Mon, 31 Aug 2026 14:42:37 -0400 Subject: [PATCH 2/4] ci: run pre-commit on the change only, and report rather than fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rules, applied everywhere: CI runs against the diff and never against every file, and CI reports rather than repairs. Both were broken here. `--all-files` is gone from CI. Every run is now scoped with `--from-ref`/`--to-ref` to the range the push or pull request actually introduced, resolved once in a step that falls back to the merge base with main when `before` arrives as all zeros (branch create, force-push) and then verifies every ref resolves. That verification is the point: an unresolvable ref makes both pre-commit and golangci-lint report nothing and exit 0, so without it a lost `fetch-depth: 0` becomes a green check over an unlinted change. The old default was `--all-files`, which replayed the whole repository's backlog at every contributor and reported it as if this change had caused it. `--fix=false` is appended to the CI hook's args. Upstream's entry carries `--fix`, and a linter that repairs its own findings has nothing left to report: measured on a committed misspelling, golangci-lint rewrote the runner's working copy, printed `0 issues` and exited 0. Green check, no output, and the committed code still misspelled — the same silent pass this work exists to end, one layer down. Fixing belongs to the local hook, which runs against the working tree where the repair is what you want; CI only reports. Verified both directions on the same content: the local hook corrected it in place and passed, the CI hook named both issues, exited 1 and left the file untouched. `--show-diff-on-failure` is added so the hooks that can only work by fixing (end-of-file-fixer and friends) surface as a readable diff instead of a bare red check. In pre-commit-hooks the Python parity smoke test is diff-scoped too. Parity is that job's whole purpose, so it has to ask both runners the same question; an `--all-files` run there compared a different one. --- .github/workflows/ci.yml | 43 +++++++++++++++++++++++++++++++--------- .pre-commit-config.yaml | 26 ++++++++++++++++-------- 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ab8aa2..72dfe31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,33 @@ jobs: go-version-file: go.mod check-latest: true cache: true # go-mod-tidy-repo resolves the whole graph; uncached it is slow and flaky + - name: Resolve the diff this change introduces + # Everything below runs against this range and nothing else — no + # `--all-files` anywhere in CI. A branch create or a force-push sends an + # all-zero `before`, and an unresolvable ref makes both pre-commit and + # golangci-lint report nothing while exiting 0, which is the silent pass + # this whole job exists to prevent. So: fall back to the merge base with + # main, then verify every ref actually resolves and fail loudly if not. + id: range + env: + BASE: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }} + HEAD_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} + run: | + set -euo pipefail + base="$BASE" + if [ -z "$base" ] || [ "$base" = "0000000000000000000000000000000000000000" ] \ + || ! git cat-file -e "${base}^{commit}" 2>/dev/null; then + echo "No usable base ref ('${base:-empty}') — falling back to the merge base with main" + base="$(git merge-base origin/main "$HEAD_SHA")" + fi + git rev-parse --verify "${base}^{commit}" >/dev/null + git rev-parse --verify "${HEAD_SHA}^{commit}" >/dev/null + # The manual hook below diffs against origin/main; if that is missing + # it reports `0 issues` and exits 0 rather than erroring. + git rev-parse --verify origin/main >/dev/null + echo "base=$base" >> "$GITHUB_OUTPUT" + echo "head=$HEAD_SHA" >> "$GITHUB_OUTPUT" + echo "Linting ${base}..${HEAD_SHA}" - name: Run pre-commit hooks # Nothing is skipped here, unlike every other repo, which skips the # HEAD-relative `golangci-lint`. Building a `language: golang` hook is @@ -53,18 +80,16 @@ jobs: # under test, and this is the one job that runs the action from the # checkout. The step below is what gates the change itself. uses: ./ - - name: Assert the lint base is present - # A base ref that does not resolve makes golangci-lint print `0 issues` - # and exit 0 — measured. Without this, losing `fetch-depth: 0` above - # would turn the lint below into a silent pass, which is the failure - # this whole change exists to end. - run: git rev-parse --verify origin/main >/dev/null + with: + extra_args: --from-ref ${{ steps.range.outputs.base }} --to-ref ${{ steps.range.outputs.head }} --show-diff-on-failure - name: Lint what this change introduces - # Same config, same pin, same hook — asked the narrower question: - # only what this change introduced, not the repository's backlog. + # Same config, same pin, same hook — asked the narrower question, and + # told not to fix (`--fix=false` in the hook's args): a linter that + # repairs its own findings exits 0 and reports nothing, which is a green + # check over unchanged, still-broken committed code. uses: ./ with: - extra_args: --hook-stage manual golangci-lint-new --all-files + extra_args: --from-ref ${{ steps.range.outputs.base }} --to-ref ${{ steps.range.outputs.head }} --hook-stage manual golangci-lint-new # Gating happens inside the jobs, never with a workflow-level `paths:` # filter: a required check that never runs reports as *pending forever* diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8c8ef27..2a87cf5 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -48,19 +48,29 @@ repos: # Keep `rev` in lockstep with go.mod's tool pin. - id: golangci-lint - # The same upstream hook, told which base to diff against. - # `--new-from-merge-base` overrides the entry's `--new-from-rev HEAD` - # (measured), which is the whole reason this works on a clean checkout, - # where the tree and HEAD are identical so the entry finds nothing. + # The same upstream hook, told which base to diff against and told not + # to fix. Both flags are appended to the entry, and both override it + # (measured): # - # `stages: [manual]` keeps it out of every local commit, where the hook - # above already reports what you are about to write and `--fix` corrects - # it. CI asks for this one by name. + # `--new-from-merge-base` beats the entry's `--new-from-rev HEAD`, + # which on a clean checkout compares the tree against an identical + # HEAD and so finds nothing. + # + # `--fix=false` beats the entry's `--fix`. This one is not a + # preference. Measured on a committed misspelling: with `--fix`, + # golangci-lint repaired the runner's working copy, printed + # `0 issues` and exited 0 — a green check, nothing reported, and the + # committed code still wrong. Fixing belongs to the hook above, which + # runs against your working tree where the repair is the point. CI + # only reports. + # + # `stages: [manual]` keeps it out of every local commit. CI asks for + # this one by name. - id: golangci-lint alias: golangci-lint-new name: golangci-lint (issues this change introduces) stages: [manual] - args: [--new-from-merge-base=origin/main] + args: [--new-from-merge-base=origin/main, --fix=false] # Secret scanning - repo: https://github.com/gitleaks/gitleaks From a646250cbce2dca242c8b0bd6650d2061428e68f Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Mon, 31 Aug 2026 14:47:34 -0400 Subject: [PATCH 3/4] ci: drop the redundant --show-diff-on-failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The action's own run line is already `pre-commit run --show-diff-on-failure --color=always ${{ inputs.extra_args }}`, so passing the flag through `extra_args` just repeated it on the command line. Harmless but misleading: it reads as though the diff output depended on this change. It does not — only the Python parity job in pre-commit-hooks, which invokes `pre-commit` directly rather than through the action, still needs to pass it itself. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 72dfe31..65e087c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -81,7 +81,7 @@ jobs: # checkout. The step below is what gates the change itself. uses: ./ with: - extra_args: --from-ref ${{ steps.range.outputs.base }} --to-ref ${{ steps.range.outputs.head }} --show-diff-on-failure + extra_args: --from-ref ${{ steps.range.outputs.base }} --to-ref ${{ steps.range.outputs.head }} - name: Lint what this change introduces # Same config, same pin, same hook — asked the narrower question, and # told not to fix (`--fix=false` in the hook's args): a linter that From 4bcc9c71a857c0925568643bdf26411bba345fc9 Mon Sep 17 00:00:00 2001 From: Blair Hamilton Date: Mon, 31 Aug 2026 14:50:45 -0400 Subject: [PATCH 4/4] docs: say why golangci-lint is the hook that needs --fix=false A fixing hook normally cannot hide in CI: pre-commit fails it with "files were modified by this hook", which is why end-of-file-fixer and friends stay honest without any extra flag. That check works from the filenames pre-commit handed the hook, and golangci-lint is `pass_filenames: false`, so there are none to compare against and the net does not catch it. Measured both ways on the same runner: end-of-file-fixer failed on a file it fixed, golangci-lint reported Passed on a file it fixed. Without that contrast the `--fix=false` line reads like belt-and-braces and is a natural thing for someone to delete later. --- .pre-commit-config.yaml | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 2a87cf5..8f78965 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -57,12 +57,21 @@ repos: # HEAD and so finds nothing. # # `--fix=false` beats the entry's `--fix`. This one is not a - # preference. Measured on a committed misspelling: with `--fix`, - # golangci-lint repaired the runner's working copy, printed - # `0 issues` and exited 0 — a green check, nothing reported, and the - # committed code still wrong. Fixing belongs to the hook above, which - # runs against your working tree where the repair is the point. CI - # only reports. + # preference. Measured on a committed misspelling: with `--fix`, the + # hook reported **Passed** while rewriting the runner's working copy, + # because a linter that repairs its own findings has nothing left to + # report. Green check, no output, committed code still wrong. + # + # And pre-commit's usual safety net does not cover it. A fixing hook + # normally fails with "files were modified by this hook" — that is why + # end-of-file-fixer and friends stay honest in CI. But that check works + # from the filenames pre-commit handed the hook, and this one is + # `pass_filenames: false`, so there are none to compare. Measured both + # ways: end-of-file-fixer failed on a file it fixed; golangci-lint + # passed on one it fixed. That is what makes this hook the exception. + # + # Fixing belongs to the hook above, which runs against your working + # tree where the repair is the point. CI only reports. # # `stages: [manual]` keeps it out of every local commit. CI asks for # this one by name.