Fix pinned Go quality tools on Go 1.26#689
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe Makefile now centralizes versioned Go lint, deadcode, and vulnerability commands with module-derived toolchain selection. CI and ChangesGo tooling centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
6-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
README.mdto point to the centralized targets.The provided context snippet shows that
README.md(lines 356-369) still explicitly instructs users to execute the directgo run ...@v2.12.2andgovulncheck@v1.3.0commands. Please update the README to reference the newmake lint-staticandmake vulnchecktargets so the documentation does not drift from the actual intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 6 - 8, Update the README instructions for static linting and vulnerability checking to reference the centralized make lint-static and make vulncheck targets instead of direct go run commands. Preserve the surrounding documentation and ensure both commands match the Makefile target names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 10-15: Replace the OS-specific TOOLCHAIN_ENV block in the Makefile
with Make’s native export of GOTOOLCHAIN, then remove the $(TOOLCHAIN_ENV)
prefix from the lint-static, deadcode, and vulncheck recipes while preserving
their existing commands.
---
Nitpick comments:
In `@Makefile`:
- Around line 6-8: Update the README instructions for static linting and
vulnerability checking to reference the centralized make lint-static and make
vulncheck targets instead of direct go run commands. Preserve the surrounding
documentation and ensure both commands match the Makefile target names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb55dab-3d6b-4ec8-8400-09c89bbe4469
📒 Files selected for processing (3)
.github/workflows/ci.ymlAGENTS.mdMakefile
There was a problem hiding this comment.
Pull request overview
This PR introduces repository-managed make targets to run pinned Go quality/security tools (golangci-lint, govulncheck, deadcode) under the module’s required Go toolchain, and updates contributor documentation and CI to use those targets. This addresses the Go 1.26 module failing to load when tools invoked via go run package@version select an older toolchain.
Changes:
- Add
make lint-static,make vulncheck, andmake deadcodetargets with pinned tool versions and a derivedGOTOOLCHAIN. - Update
AGENTS.mdto instruct contributors/agents to use the newmaketargets. - Update CI security/code-health steps to call the new
maketargets instead of directgo run ...@versioncommands.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Makefile | Adds pinned tool targets and derives a Go toolchain value to run them under. |
| AGENTS.md | Switches required lint/security commands to the new make targets. |
| .github/workflows/ci.yml | Uses the new make targets for govulncheck, deadcode, and golangci-lint steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GO_VERSION := $(shell go list -m -f "{{.GoVersion}}") | ||
| GO_TOOLCHAIN := go$(GO_VERSION) | ||
| DEADCODE_VERSION := v0.46.0 | ||
| GOLANGCI_LINT_VERSION := v2.12.2 | ||
| GOVULNCHECK_VERSION := v1.3.0 | ||
|
|
||
| ifeq ($(OS),Windows_NT) | ||
| TOOLCHAIN_ENV := set "GOTOOLCHAIN=$(GO_TOOLCHAIN)" && | ||
| else | ||
| TOOLCHAIN_ENV := GOTOOLCHAIN=$(GO_TOOLCHAIN) | ||
| endif |
The Windows_NT branch used cmd.exe syntax (set "..." &&), but GNU Make runs recipes through sh even on Windows (MSYS/Git Bash), where that form fails and breaks make lint-static/deadcode/vulncheck. A plain GOTOOLCHAIN=... prefix works everywhere, so the OS check is gone. Also switch GO_VERSION/GO_TOOLCHAIN to recursive expansion so 'go list -m' only runs when a tool target actually needs it, instead of at parse time for every target. Addresses PR review from CodeRabbit and Copilot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Keep the pinned-tool targets usable with native Windows Make shells
Makefile:49
These recipes rely on POSIX temporary-assignment syntax (GOTOOLCHAIN=... go run). GNU Make on Windows uses the configured Windows shell, so under a normalcmd.exesetup that token is treated as the command to execute and all three targets fail before Go runs. The PR explicitly claims the targets work on Windows, but the workflow only runs them in the Ubuntu security job. Export the variable through Make (or use a shell-neutral wrapper) and exercise the targets on Windows rather than assuming an MSYS/Git-Bash shell. -
[P2] Update the public quality-check workflow to the toolchain-pinned commands
README.md:354
The README still tells contributors to run the exact version-suffixedgo runcommands that this PR identifies as selecting an older tool-module Go toolchain and failing to load this Go 1.26.5 module. Only CI andAGENTS.mduse the new Make targets, so a contributor following the documented workflow still reproduces #680. Point the lint and vulnerability commands atmake lint-staticandmake vulncheck, and reconcile the adjacent global-install alternative if it is meant to be runnable.
Address code review on PR Gitlawb#689: - lint-static/deadcode/vulncheck previously set GOTOOLCHAIN via a POSIX `VAR=value cmd` recipe prefix. GNU Make on Windows runs recipes through whatever SHELL is configured, and under a native cmd.exe setup that syntax is invalid — cmd.exe treats the whole "GOTOOLCHAIN=go1.26.5" token as the program to run and fails before go ever executes (reproduced locally: cmd.exe errors with "'MYVAR' is not recognized..." for the equivalent pattern). Switched to a target-specific `export`, which sets the variable in the child process environment via Make itself, before is invoked — works under sh, cmd.exe, and PowerShell alike (verified against all three targets with SHELL := cmd.exe). - README.md's Code Quality and Security Checks section still told contributors to run the raw `go run ...@version` commands this PR identifies as selecting the wrong (older) toolchain and failing to load this Go 1.26.5 module — only CI and AGENTS.md had been updated to the new make targets. Points steps 3-4 at `make lint-static` / `make vulncheck` instead, matching AGENTS.md. Verified end-to-end: `make vulncheck`, `make deadcode`, and `make lint-static` all run successfully and reach their tools (deadcode's advisory report and lint-static's 35 pre-existing findings match the PR's documented baseline). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Keep the derived toolchain to one module when a workspace is active
Makefile:11
go list -mlists every main module whenGOWORKselects a workspace. Its newline-separated Go versions are flattened by$(shell ...), so the exported value becomes something likeGOTOOLCHAIN="go1.26.5 1.25"; all three new targets then fail before their tool runs. Select Zero's module explicitly (or otherwise guarantee a single version) so these checks work from a multi-module workspace. -
[P2] Do not derive the recovery toolchain through the already-forced Go command
Makefile:11
Thego listused to computeGO_TOOLCHAINruns while Make expands the target-specific export, before that export reaches a recipe. Thus an inherited fixed olderGOTOOLCHAINmakes this initial command fail on Zero'sgo 1.26.5module, andmake lint-static,make deadcode, andmake vulnchecknever get thego1.26.5setting meant to repair the situation. Derive the version without depending on that forced command, or ensure the probe itself cannot inherit the stale override. -
[P2] Fix or remove the documented global-install alternative
README.md:361
Thesego install ...@versioncommands use the same versioned tool modules whose older toolchain selection is the root cause described immediately above. The installed golangci-lint and govulncheck binaries therefore still cannot analyze this Go 1.26.5 project, leaving contributors who follow the advertised alternative with a broken workflow. Document a Go-1.26-compatible installation path (and its invocation), or remove the alternative in favor of the Make targets.
There was a problem hiding this comment.
Re-ran the three targets' underlying commands under the GOTOOLCHAIN=go1.26.5 pin: vulncheck exits 0 with no vulnerabilities, deadcode exits 0, and golangci-lint loads the module fine (any findings are the pre-existing advisory ones, continue-on-error in CI). Moving the pin from a job-wide GITHUB_ENV awk step into Make target-specific exports scoped to just these three targets is a clean improvement, and the tool versions are unchanged from the old inline CI commands so there's no new trust surface. One trivial nit: the Makefile header comment still says "Run make lint before opening a PR" while AGENTS.md now points at make lint-static/make vulncheck not worth blocking. Approving.
Summary
Root cause
go run package@versionselected the tool module's older Go toolchain, which could not load this Go 1.26 module. CI attempted to export atoolchaindirective fromgo.mod, but the module only has agodirective, so the fallback remainedauto.The new Make targets derive
go1.26.5fromgo list -mand setGOTOOLCHAINwith platform-appropriate syntax before invoking each pinned tool.Validation
go fmt ./...completedgo vet ./...passedmake vulncheckpassed with no vulnerabilitiesmake deadcoderan successfully under Go 1.26.5 and produced its expected advisory reportmake lint-staticran successfully under Go 1.26.5 and reached the analyzers; it reports the 35 pre-existing advisory findings tracked by chore: fix golangci-lint findings (dead code, ineffassign, staticcheck) #527Fixes #680
Summary by CodeRabbit
Chores
Documentation