Repository navigation
ci: run the full prek hook suite as the gate - #58
Merged
Merged
Conversation
.pre-commit-config.yaml becomes the single source of what gets checked: `make check` is now `prek run --all-files`, and check-tidy/lint/test are hooks in there alongside prek's builtin hygiene checks and gitleaks. Hooks that would have run green without checking anything are pinned down: check-added-large-files gets --enforce-all and check-merge-conflict gets --assume-in-merge, since both otherwise consult git state (staged files, mid-merge) that CI's clean checkout never has. gitleaks keeps its --staged entry on purpose and is documented as a local commit-time guard, with GitHub push protection as the server-side net. The three local hooks carry always_run: true so a commit that only deletes files can't skip lint and test on an empty file list. A shared check-rev target makes `make lint` and `make fix` hard-fail on an unresolvable NEW_FROM_REV -- golangci-lint only warns and exits 0 there, which would let CI pass having linted nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
min0625
added a commit
that referenced
this pull request
Sep 1, 2026
Re-add gci. #59 dropped it as covered by gofumpt+goimports, but those two only sort within the groups a file already has: a third-party import stranded in the stdlib block is moved out into a group of its own rather than merged into the existing third-party block. gci is what collapses it back to the canonical two groups. Replace gomodguard_v2's local-replace-directives with the gomoddirectives linter. gomodguard only sees directly imported modules, so an indirect dep's replace slipped through; gomoddirectives forbids every replace by default -- checked against a resolvable non-local one, not just a `=> ../local`. Retire depguard. Its one rule (github.com/pkg/errors) moves into the gomodguard_v2 blocklist so the recommendation text sits next to the other deprecated modules. mitchellh/mapstructure and gopkg.in/yaml (prefix match, so both majors) join it. Enable bidichk, makezero and reassign. reassign gets patterns: ".*", which is upstream's own recommendation -- the default only guards EOF and Err*, leaving os.Args and http.DefaultClient open. Tests are excluded from it: they borrow os.Stdin/os.Args and restore them afterwards. Turn on errcheck.check-type-assertions and prealloc.for-loops (modernize rewrites 3-clause loops into the range form prealloc polices, so with the default the finding only surfaces after --fix), and turn off perfsprint.concat-loop, which modernize rewrites better by reusing the variable instead of inventing one. Exclude gosec's G104. It fires on bare call statements only, never on defer/go, and honors a whitelist -- a strict subset of errcheck, which also names the offending function; with uniq-by-line: false both would print on the same line. Narrowing errcheck (exclude-functions, std-error-handling) would reopen the gap. Correct the check-rev rationale in the Makefile and AGENTS.md. #58 documented golangci-lint as only warning and exiting 0 on an unresolvable --new-from-rev, which would let CI pass having linted nothing. It does not: it warns, reports every issue in the repo, and exits 1. The guard stays, but it buys a readable error message, not safety. `make fix` now runs tidy -> --fix -> tidy -> lint. --fix is not a fixpoint: its edits can trip a linter the fixing run never saw, and can add or remove imports. The new ordering was exercised on a clean tree, so it confirms the plumbing runs, not that the second pass caught a real fixpoint miss. Also document in AGENTS.md that .golangci.yaml is not self-contained -- whole-files: true is silently inert without one of the --new* modes, so the file only works alongside the Makefile's --new-from-rev harness -- and that formatter findings are the exception to the ratchet: apply `golangci-lint fmt ./...` repo-wide rather than letting a half-formatted tree contradict its own config. `golangci-lint run ./...` reports 0 issues repo-wide under this config, so the whole-files ratchet takes on no new debt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
min0625
added a commit
that referenced
this pull request
Sep 1, 2026
Re-add gci. #59 dropped it as covered by gofumpt+goimports, but those two only sort within the groups a file already has: a third-party import stranded in the stdlib block is moved out into a group of its own rather than merged into the existing third-party block. gci is what collapses it back to the canonical two groups. Replace gomodguard_v2's local-replace-directives with the gomoddirectives linter. gomodguard only sees directly imported modules, so an indirect dep's replace slipped through; gomoddirectives forbids every replace by default -- checked against a resolvable non-local one, not just a `=> ../local`. Retire depguard. Its one rule (github.com/pkg/errors) moves into the gomodguard_v2 blocklist so the recommendation text sits next to the other deprecated modules. mitchellh/mapstructure and gopkg.in/yaml (prefix match, so both majors) join it. Enable bidichk, makezero and reassign. reassign gets patterns: ".*", which is upstream's own recommendation -- the default only guards EOF and Err*, leaving os.Args and http.DefaultClient open. Tests are excluded from it: they borrow os.Stdin/os.Args and restore them afterwards. Turn on errcheck.check-type-assertions and prealloc.for-loops (modernize rewrites 3-clause loops into the range form prealloc polices, so with the default the finding only surfaces after --fix), and turn off perfsprint.concat-loop, which modernize rewrites better by reusing the variable instead of inventing one. Exclude gosec's G104. It fires on bare call statements only, never on defer/go, and honors a whitelist -- a strict subset of errcheck, which also names the offending function; with uniq-by-line: false both would print on the same line. Narrowing errcheck (exclude-functions, std-error-handling) would reopen the gap. Correct the check-rev rationale in the Makefile and AGENTS.md. #58 documented golangci-lint as only warning and exiting 0 on an unresolvable --new-from-rev, which would let CI pass having linted nothing. It does not: it warns, reports every issue in the repo, and exits 1. The guard stays, but it buys a readable error message, not safety. `make fix` now runs tidy -> --fix -> tidy -> lint. --fix is not a fixpoint: its edits can trip a linter the fixing run never saw, and can add or remove imports. The new ordering was exercised on a clean tree, so it confirms the plumbing runs, not that the second pass caught a real fixpoint miss. Also document in AGENTS.md that .golangci.yaml is not self-contained -- whole-files: true is silently inert without one of the --new* modes, so the file only works alongside the Makefile's --new-from-rev harness -- and that formatter findings are the exception to the ratchet: apply `golangci-lint fmt ./...` repo-wide rather than letting a half-formatted tree contradict its own config. `golangci-lint run ./...` reports 0 issues repo-wide under this config, so the whole-files ratchet takes on no new debt. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
.pre-commit-config.yamlbecomes the single source of truth for what gets checked.make checkis nowprek run --all-files --show-diff-on-failure, andcheck-tidy/lint/testlive in there as hooks alongside prek's builtin hygiene checks and gitleaks — sogit commit(staged) and CI (repo-wide) run the same suite.Hooks that would have passed green without checking anything
Verified empirically in a scratch repo under CI conditions (clean checkout, nothing staged, not mid-merge):
check-added-large-files--enforce-all→ caughtcheck-merge-conflict--assume-in-merge→ caughtBoth consult git state instead of the file list prek hands them, and CI's checkout has neither. The rationale is in a comment at the top of the config so the next hook added gets read before it gets trusted.
gitleakskeeps its hard-coded--stagedentry on purpose — it is a local commit-time guard, not a CI secret scan. GitHub secret scanning push protection (enabled on this repo) is the server-side net. Documented inline and in AGENTS.md rather than silently left as a green no-op.Other correctness details
always_run: trueon the three local hooks — without it prek skips a hook whose file list comes back empty, so a commit that only deletes files would skip lint and test entirely.check-revtarget:make lintandmake fixnow hard-fail on an unresolvableNEW_FROM_REV.golangci-lintonly warns and exits 0 there, which would let CI pass having linted nothing. CI passesorigin/${{ github.base_ref }}, and the guard was verified to propagate throughmake check→ prek → the nestedmake lint.repo: builtinuses prek's native Rust implementations — same checks, no repo clone or Python env built for code that never runs. Note this makes the config prek-only; upstreampre-commitrejects it withMissing required key: rev.~/.cache/prekcache step so the from-source gitleaks build stays off the 15-minute job timeout.Note on the
.serena/change.serena/memories/memory_maintenance.mdis unrelated to this change — it is the new repo-wide fixers' output on a pre-existing file (trailing whitespace, missing final newline). Kept deliberately: reverting it just moves the failure to the next unrelated PR that runsmake check.Verification
make checkpasses clean end to end, with no file rewritten.🤖 Generated with Claude Code