Fix pinned Go quality tools on Go 1.26#689
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe Makefile centralizes versioned Go lint, deadcode, and vulnerability commands with module-derived toolchain selection. CI, agent guidance, and contributor documentation now invoke these targets. 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
73-80: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake the
baselinetarget cross-platform using native Make constructs.This recipe uses POSIX shell features (
if [ -z ... ]and$${VAR:-default}) which will fail on Windows when Make uses its defaultcmd.exe.You can make this fully shell-agnostic by leveraging Make's native conditional errors (
$(if ...)) and fallback expansion ($(or ...)). This shifts the validation out of the OS shell and into Make itself, ensuring it runs reliably across all platforms.🛠 Proposed refactor
baseline: build - `@if` [ -z "$(ZERO_BENCH_MODEL)" ]; then echo "Set ZERO_BENCH_MODEL (and optionally ZERO_BENCH_BINARY) before running 'make baseline'"; exit 2; fi - `@ZERO_BIN`="$${ZERO_BENCH_BINARY:-./zero}"; \ - go run ./cmd/zero-perf-bench turn \ - --suite internal/perfbench/manifests/baseline.json \ - --model $(ZERO_BENCH_MODEL) \ - --binary "$$ZERO_BIN" \ - --output internal/perfbench/reports/baseline.json + $(if $(ZERO_BENCH_MODEL),,$(error Set ZERO_BENCH_MODEL before running 'make baseline')) + go run ./cmd/zero-perf-bench turn \ + --suite internal/perfbench/manifests/baseline.json \ + --model $(ZERO_BENCH_MODEL) \ + --binary "$(or $(ZERO_BENCH_BINARY),./zero)" \ + --output internal/perfbench/reports/baseline.json🤖 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 73 - 80, Update the Makefile baseline target to remove POSIX shell validation and parameter-expansion syntax. Use Make-native $(if ...) validation for ZERO_BENCH_MODEL and $(or ...) to select ZERO_BENCH_BINARY with ./zero as the fallback, while preserving the existing benchmark command and arguments.
🤖 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`:
- Line 4: Replace the GNU Make-specific $(file <go.mod) expression assigned to
GO_MOD_WORDS with a portable $(shell cat go.mod) read, preserving the variable’s
existing purpose and downstream behavior.
---
Outside diff comments:
In `@Makefile`:
- Around line 73-80: Update the Makefile baseline target to remove POSIX shell
validation and parameter-expansion syntax. Use Make-native $(if ...) validation
for ZERO_BENCH_MODEL and $(or ...) to select ZERO_BENCH_BINARY with ./zero as
the fallback, while preserving the existing benchmark command and arguments.
🪄 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: 33e3fdf2-705a-4f7d-8570-e62bc3b380a4
📒 Files selected for processing (3)
.github/workflows/ci.ymlMakefileREADME.md
| # lint` before opening a PR" — these targets back those instructions. | ||
| .DEFAULT_GOAL := build | ||
| .PHONY: build build-all test test-race vet fmt fmt-check lint tidy clean baseline help | ||
| GO_MOD_WORDS := $(file <go.mod) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Test Make 3.81 compatibility.
cat << 'EOF' > test.mk
GO_MOD_WORDS := $(file <go.mod)
all:
`@echo` $(GO_MOD_WORDS)
EOF
touch go.mod
# macOS make is typically 3.81. We can simulate the failure with any make by checking syntax errors, or we can just point to the GNU Make manual for 4.2.
# Note: The sandbox might have Make 4.x, but we can verify that $(file <) is a Make 4.2+ feature.
echo "Make 4.2 release notes detail the addition of \$(file <...): https://lwn.net/Articles/687985/"Repository: Gitlawb/zero
Length of output: 243
Replace $(file <go.mod) with a portable read
$(file <...) needs GNU Make 4.2+, so the default macOS make will fail here. Use $(shell cat go.mod) or document a GNU Make requirement.
🤖 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` at line 4, Replace the GNU Make-specific $(file <go.mod) expression
assigned to GO_MOD_WORDS with a portable $(shell cat go.mod) read, preserving
the variable’s existing purpose and downstream behavior.
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 new Makefile parser compatible with stock macOS Make
Makefile:4
$(file <go.mod)is not supported by the system Make shipped with macOS (GNU Make 3.81), so it expands empty and the new parse-time validation immediately aborts every target. This makes the newly documentedmake lint-staticandmake vulncheckcommands—and existingmake fmt/make vet—unusable for normal macOS contributors before Go is invoked. The macOS CI leg only runs Go commands, so it does not exercise these targets. Use a parser compatible with the supported Make implementation (and tolerant of normalgo.modcomments), or explicitly provision and invoke a modern GNU Make on macOS.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Still requesting changes, but this is close. The rework since my last pass is the right shape — parsing go.mod directly kills both the stale-GOTOOLCHAIN probe problem and the GOWORK multi-module case, and the native cmd.exe Make steps on Windows CI plus the README cleanup close out the earlier asks. The blocker is the one jatmn flagged: the $(file <go.mod) read form needs GNU Make 4.2+, and macOS ships 3.81, where it expands empty and the new parse-time $(error) aborts every target — including make build and make fmt that work today. CI stays green only because no macOS job touches make, so this would land silently broken for macOS contributors. Whatever replaces it has to survive both stock macOS make and your new Windows cmd steps — $(shell cat go.mod) alone just trades one platform for the other, so something shell-neutral (or an explicit modern-Make requirement that CI actually exercises on macOS) is needed. The header comment still only mentions make lint — still not blocking, but worth folding into the next push.
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] Rebase and resolve the current
mainconflicts
AGENTS.md,README.md
GitHub currently reports this head asCONFLICTING/DIRTY, and merging it with currentmainproduces unresolved conflicts in these two files. The branch cannot merge as submitted and the conflict resolution must preserve the validation guidance added onmainalongside these new quality-tool instructions. -
[P2] Make the
go.modsearch independent of user Git grep settings
Makefile:4
git grephonors the caller'sgrep.patternType; with the validgrep.patternType=fixedconfiguration,^go[[:space:]]is treated literally and returns no version. The target-specific export therefore becomesGOTOOLCHAIN=go, so all three new quality targets fail before their tool runs. Force the intended regex mode (for example,git grep -G ...) or parse the directive without inheriting that user configuration.
…lity-tools # Conflicts: # AGENTS.md
|
Addressed the remaining blockers in merge commit Conflict resolution
Makefile robustness
Validation
Please re-review the conflict resolution and forced-regex Makefile fix. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head. Both my asks landed. The macOS Make 3.81 blocker is fixed at the root: the
I also looked at the one trap the git-grep approach could introduce, a user's grep.patternType overriding the version match, and the -G flag pins it to basic regex, so that's covered. Green across the board. 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
maketargets.lint-static,vulncheck, anddeadcodetargets aligned with the Go toolchain and pinned tool versions.Documentation
makecommands for formatting, vetting, linting, and security checks.