ci(vulncheck): one declaration for the govulncheck pin, and a check that keeps it (backend#1972) - #501
Conversation
…hat keeps it (backend#1972) The version lived in THREE places -- build.yml's job, vulncheck.yml's job, and GOVULNCHECK_VERSION in the Makefile -- held in step by a comment reading "keep the job in lockstep". All three happened to read v1.1.4, which is exactly what made it look fine. Two copies kept in sync by a request is not a mechanism. Both workflows now run `make vulncheck`, so the Makefile is the single declaration, and a green `make ci` locally cannot disagree with the PR gate about which govulncheck ran -- the same reasoning already applied to lint-full/GOLANGCI_LINT_VERSION. The "keep in lockstep" comment is replaced by a machine check rather than deleted: scripts/check-tool-pins.sh PARSES GOVULNCHECK_VERSION out of the Makefile and fails the Lint job if any workflow hardcodes that module with an @Version. The guard holds no version of its own, so it cannot agree with itself while disagreeing with reality. Mutation-proved, anchors asserted: reintroduce `go install ...govulncheck@v1.1.4` in build.yml -> exit 1, names file and line delete GOVULNCHECK_VERSION from the Makefile -> exit 2, refuses to report clean run from an unrelated cwd -> still correct (cd's to its root) The two jobs still differ, deliberately, and now ONLY in the ref: vulncheck.yml checks out develop on a schedule, build.yml judges the PR head. That difference is stated in the file instead of being buried in duplicated steps. DRAFT until cli#500 lands. `govulncheck` is already red on develop -- go1.26.5 carries 4 reachable stdlib CVEs -- so this branch inherits that failure. Verified locally that the folded target reproduces it exactly: make vulncheck -> exit status 3, "affected by 4 vulnerabilities from the Go standard library", each "Fixed in: <pkg>@go1.26.6" which also independently confirms backend#1972's counterfactual. This PR does not change WHAT govulncheck finds, only where its version is declared. NOT in this PR, and both belong to the ticket rather than here: adding govulncheck to `main`'s required contexts (needs #500 on main first, plus the break-glass decision -- enforce_admins is true there and skip-fr-gate does not bypass a required check), and the same three-copies shape for GOLANGCI_LINT_VERSION, which TOOLS in the new guard is structured to take as a second row. The two actionlint SC2001 findings on build.yml are pre-existing on develop (verified against the unmodified base) and untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f7bce7d. Configure here.
… grep errors Two Bugbot findings on the govulncheck single-declaration change (backend#1972): - check-tool-pins ran only in the CI Lint job, so a green local `make ci` / `make check` could disagree with the PR about a restated pin — the exact local/CI divergence the Makefile-as-entry-point exists to prevent. Add it to both aggregate targets, beside check-style/file-budget. - The workflow scan used `grep ... 2>/dev/null || true`, laundering a real grep error (rc>=2: unreadable tree, bad invocation) into an empty hit list and an unearned clean pass — the fail-open this guard was written against. Capture rc and fail closed on rc>=2, matching scan() in check-style.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1144759. Configure here.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approving — correctness review focused on the new guard and workflow rewiring.
What this PR does
Collapses the govulncheck version pin from three copies (build.yml, vulncheck.yml, GOVULNCHECK_VERSION in the Makefile) down to the single Makefile declaration. Both workflows now run make vulncheck, and scripts/check-tool-pins.sh (wired into make check, make ci, and the Lint job) fails closed if any workflow restates a Makefile-owned tool pin.
Correctness — verified against the PR tree (1144759)
- Guard baseline:
check-tool-pins.shexits0and reports one pin declared once. - Mutation proofs all reproduce: reintroducing
govulncheck@v1.1.4in a workflow → exit 1 naming file+line; deletingGOVULNCHECK_VERSION→ exit 2 (refuses to report clean); running from an unrelated cwd → still correct (itcds to its root). - Fail-closed grep: rc is captured; rc≥2 exits 2 rather than laundering into an empty hit list. Matches
scan()incheck-style.sh. - Version regex (
\??=) correctly parses both?=and plain=declarations. - No lingering pins: no workflow still matches
golang.org/x/vuln/cmd/govulncheck@, so the guard does not false-fail CI; the new comments reference onlygovulncheck, not themodule@form. make vulncheck(go run ...@$(GOVULNCHECK_VERSION) ./...) is behaviorally equivalent to the oldgo install ...@v1.1.4 && govulncheck ./...;GO ?= goand the target are defined.shellcheckclean at--severity=error(CI level) and at default level.- Adding
check-tool-pinsto the fast localchecktarget introduces no network dependency (pure sed/grep on local files) — good separation from the network-boundvulncheck.
Prior findings
Both cursor[bot] findings ("omitted from make ci", "reports clean on grep errors") are already fixed in head commit 1144759 and verified in the diff. Latest automated review is clean.
Comments and structure are clear and the deliberate vulncheck.yml vs build.yml ref difference is now documented in-file rather than duplicated. No correctness issues found.

Part of tracebloc/backend#1972 — the half that is buildable today. Does not close it.
The duplication
The pinned version lived in three places, held in step by a comment reading "Keep the job in lockstep with build.yml's govulncheck job and GOVULNCHECK_VERSION in the Makefile":
build.yml:191go install golang.org/x/vuln/cmd/govulncheck@v1.1.4vulncheck.ymlMakefile:125GOVULNCHECK_VERSION ?= v1.1.4All three happened to read
v1.1.4, which is exactly what made it look fine. Two copies kept in sync by a request is not a mechanism.What changes
Both workflows now run
make vulncheck, so the Makefile is the single declaration — and a greenmake cilocally cannot disagree with the PR gate about which govulncheck ran. The same reasoning already applies tolint-full/GOLANGCI_LINT_VERSION.The "keep in lockstep" comment is replaced by a check, not deleted.
scripts/check-tool-pins.shparsesGOVULNCHECK_VERSIONout of the Makefile and fails the Lint job if any workflow hardcodes that module with an@version. The guard holds no version of its own, so it cannot agree with itself while disagreeing with reality.The two jobs still differ deliberately, and now only in the ref:
vulncheck.ymlchecks outdevelopon a schedule,build.ymljudges the PR head. That is stated in the file rather than buried in duplicated steps.Mutation proof
Each anchor asserted:
Why it is red, and why that is the pre-existing state
make vulnchecklocally, on this branch:That also independently confirms the ticket's counterfactual. This PR does not change what govulncheck finds — only where its version is declared. cli#500 (
go1.26.5 → go1.26.6, open against develop, green) is the fix.Not in this PR
Both belong on the ticket rather than here:
govulnchecktomain's required contexts. Needs sec(deps): bump the build toolchain to go1.26.6 (backend#1972) #500 onmainfirst — arming it earlier would block every promotion PR the train opens. It also needs a decision I flagged on the ticket:enforce_admins: trueoncli/mainandskip-fr-gatedoes not bypass a required status check, so once armed a fresh stdlib CVE hard-blocks the prod hop with no break-glass.GOLANGCI_LINT_VERSIONhas the same three-copies shape.TOOLSin the new guard is a table specifically so that becomes a second row rather than a second guard.Test plan
bash scripts/check-tool-pins.sh— clean;make check-tool-pins— cleanshellcheck --severity=error scripts/check-tool-pins.sh— cleanactionlint— the 2 SC2001 findings onbuild.ymlare pre-existing ondevelop(verified against the unmodified base), untouched heremake vulncheckreproduces the same 4 stdlib CVEs the CI job reports🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to CI/Makefile tooling and comments; no application runtime or security-sensitive product logic is modified.
Overview
Centralizes the govulncheck version in
GOVULNCHECK_VERSIONand routes bothbuild.ymlandvulncheck.ymlthroughmake vulncheckinstead of duplicatinggo install ...@versionin each workflow.Adds
scripts/check-tool-pins.sh(wired intomake check,make ci, and the Lint job) so workflows cannot restate a Makefile-owned tool pin; the guard reads the version from the Makefile and fails closed ifmodule@versionappears under.github/workflows/. Installer CI also shellchecks the new script.Reviewed by Cursor Bugbot for commit 1144759. Bugbot is set up for automated code reviews on this repo. Configure here.