ci: catch cross-platform build breaks before release#19
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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. ✨ 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 review |
✅ Action performedReview finished.
|
CI builds one platform (linux/amd64) while release.yml ships six. The repo has platform-gated source - shell_windows.go, procattr_windows.go and their _unix twins - that CI never compiles, so a signature change to shellCommand or configureProcessGroup compiles green on main and breaks at tag time, after the tag is public. Add a cross-compile matrix job covering the five targets the test job does not, mirroring the shape release.yml:11-29 already runs on this same runner pool. fail-fast: false so a failure names the target. The job trusts the workspace the same way the test job does. The container runs as root over a checkout owned by another uid, so git reports dubious ownership and Go's VCS stamping - which runs when building main packages - fails the build with "error obtaining VCS status: exit status 128". Marking the checkout safe keeps stamping on and consistent with the test job and the release builds, rather than papering over it with -buildvcs=false. Also add two cheap gates the repo lacked: gofmt -s drift (make fmt rewrites and can never fail CI) and go mod tidy drift (make check mutates, so it cannot gate either). Both print the remedy. All six targets build clean today - this is a missing guard, not a fix.
1dd227e to
dbd878c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Description
CI builds one platform.
ci.yml:52isgo build -o /dev/null ./cmd/opencodereviewinsidecontainer: golang:1.26.5, i.e. linux/amd64.release.yml:15-29cross-compiles six, andonly fires on
push: tags: ['v*'].That gap has teeth here, because the repo carries platform-gated source CI never compiles:
procattr_unix.gousessyscall.SysProcAttr{Setpgid: true}andsyscall.Kill; the Windowstwins are, as far as CI is concerned, dead code. A signature change to
shellCommandorconfigureProcessGroupcompiles green onmainand breaks at tag time — after the tag isalready public.
All six targets build clean today. This is a missing guard, not a live bug.
I proved the gap rather than asserting it
Appended
var _ = thisSymbolDoesNotExisttoshell_windows.goand ran every gate CI has todayagainst every gate this PR adds:
go build -o /dev/null ./cmd/opencodereview— today'sBuildstepgo build -o /dev/null ./...— linux/amd64, whole treego vet ./...GOOS=windows GOARCH=amd64 go build ./...— new matrix legcmd/opencodereview/shell_windows.go:14:9: undefined: thisSymbolDoesNotExistGOOS=windows GOARCH=arm64 go build ./...— new matrix legGOOS=darwin GOARCH=arm64 go build ./...— new matrix legThe first three rows are the argument: a break that ships broken binaries is invisible to
every check this repo runs today. The last row shows
fail-fast: falseisolating the failureto the targets that actually broke.
Cost — answered by production, not by my estimate
The obvious objection is runner time, and I'd rather not answer it with a number from my laptop.
release.yml:11-29already runs this exact shape on this exact runner pool:runs-on: self-hosted,container: golang:1.26.5, a 6-way matrix over the same GOOS/GOARCH pairs,CGO_ENABLED: '0'.Measured from the last three releases (
gh run view <id> --json jobs):33–38 seconds per leg, including checkout, container start and artifact upload — and the legs
run in parallel. So the capacity question is already answered by your own release pipeline.
Two honest deltas: this job builds
./...whererelease.ymlbuilds./cmd/opencodereviewwith
-ldflags(locally that is ~+8%), and it skips artifact upload. Neither changes the orderof magnitude.
Why a separate job rather than a step in
testAppending five sequential cross-builds to
testwould put them inside itstimeout-minutes: 15budget, alongside
go vet, agovulncheckinstall and scan, andgo test -raceover 23packages. A hygiene PR that makes CI time out is the worst possible outcome. As a sibling job
the legs run concurrently,
test's runtime is unchanged, andfail-fast: falsenames thetarget that broke instead of surfacing one opaque loop failure.
linux/amd64is deliberately excluded —testalready covers it. To be sure that leaves nohole, I checked that the linux/amd64 whole tree is covered too: injecting a break into a
non-
cmdpackage (internal/llm/providers.go) makesgo vet ./...exit 1. Sovetandtest -racealready compile all 23 packages on linux/amd64; the matrix only needs the other five.Two riders, same "CI should have caught this" theme
Both are gates the repo currently cannot enforce, because the Makefile targets mutate rather
than fail:
gofmtdrift. CI runsgo vetonly (ci.yml:29);make fmtrewrites files and neverfails, so formatting drift can land on
main. Peer:ahmetb/kubectxci.yml:41.go mod tidydrift. No check exists.Makefile:67runs tidy but mutates, so it can nevergate CI. Peer:
jesseduffield/lazygitci.yml:146—go mod tidy && git diff --exit-code || (echo "go.mod file is not clean..." && exit 1).Both gates print their own remedy via
::error::, which renders as a job annotation in the PRUI so the contributor sees the fix without opening the log. A bare
test -z "$(gofmt -s -l .)"prints nothing on failure — a red X with an empty log.
I verified both gates actually fire, since a gate that only passes on a clean tree is
satisfied by a step that does nothing:
gofmt -s -w ..github.com/atotto/clipboard, currently an indirectdependency, forcing
tidyto promote it → exit 1 with+ github.com/atotto/clipboard v0.1.4/- github.com/atotto/clipboard v0.1.4 // indirect.gofmt -s -l .is currently empty on this tree, so-sadds no churn.Peer evidence
avivsinai/agent-message-queueci.yml:37—Cross-platform buildstep withGOOS=windows go build ./.../GOOS=freebsd go build ./...alexjbarnes/vault-sync.github/workflows/cross-compile.yml:14—Cross-compile checkdocker/model-runnerci.yml:12— the lint job itself ismatrix: goos: [linux, darwin, windows]Limitations
go mod tidycontactsproxy.golang.org(noGOFLAGS/GOPROXY/GOPRIVATEis set anywhere in this repo, so defaults apply), so a proxyoutage would turn a green PR red.
ci.yml:33'sgo install …@latestalready has thatproperty, so this adds no new class of failure — but it is worth stating rather than
discovering.
cross-compilejob onself-hostedrunners. My fork has noself-hosted runners, so
ci.ymlcannot execute there at all. The job's behaviour is provenby the local mutation test above and its cost by your release runs, but the first real
execution will be the
cross-compilecheck on this PR.go.modif it passes.Nothing downstream in
testdepends on that, but it is a real side effect.timeout-minutes: 20on the new job is a guess rather than a measurement, since the job hasnever run. Given 33–38 s legs in production it is ~30× headroom.
Type of Change
How Has This Been Tested?
stays green while both Windows legs fail and name the file and line
fail-fast: falseisolates — darwin/arm64 unaffected in the same rungo vet ./...exits 1 on a non-cmd break)GOCACHE— all clean todayactionlintv1.7.12 — exit 0yaml.safe_load; job/leg/step-ordering assertions via a Python schema checkChecklist
mutation test and gate falsification above.
One question for maintainers
ci.ymlhas no caching at all — noactions/setup-go, noactions/cache, nocontainer.volumes— soGOCACHE/GOMODCACHElive in the ephemeral container FS and every runrecompiles the stdlib and every dependency. That already taxes the existing
testjob, and it isthe honest answer to "why does any of this cost anything". The self-hosted-native fix is small:
I deliberately left it out of this PR because it needs host-side directories only you control.
Happy to open it separately if that's useful.
AI assistance: Claude Code helped research and verify this change. I reviewed the full diff
and take responsibility for it.