fix: add integration build verification to catch fuzz build failures on PRs#47398
Conversation
…ilures Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds CI verification for integration-tagged Go tests to catch compilation failures before fuzzing on main.
Changes:
- Adds integration build checks to the test and fuzz jobs.
- Compiles integration-tagged packages without selecting tests.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/cgo.yml |
Adds integration build verification steps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
.github/workflows/cgo.yml:1391
-run='^$'does not suppressTestMain; the integration-tagged CLI package runsmake buildbeforem.Run()(pkg/cli/compile_integration_test.go:33-87). Because this job has four matrix entries, that unintended setup is repeated four times. Use a no-op test executor to keep this pre-flight step compile-only.
go test -run='^$' -tags=integration ./...
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| # Regular unit tests use -tags '!integration', so build errors in | ||
| # integration-tagged files (e.g. unused imports) would otherwise | ||
| # go undetected until the fuzz job runs on main. | ||
| go test -run='^$' -tags=integration ./... |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot please run the Open unresolved review feedback (newest first):
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes on one issue that affects both new steps.
📋 Key Findings
Issue: go test -run='^$' triggers TestMain side-effects
Both new "Verify integration build" steps use go test -run='^$' -tags=integration ./.... While -run='^$' prevents test functions from running, it does not prevent TestMain from executing. pkg/cli/compile_integration_test.go has a TestMain that calls make build, so this step will unexpectedly build the binary on every CI run.
The fix is to use go build -tags=integration ./... instead, which is the correct tool for build verification without test execution side-effects.
Positive Highlights
- ✅ The root cause is well-understood and the PR description is clear
- ✅ Adding the guard in both the
testandfuzzjobs is the right scope
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 16.7 AIC · ⌖ 4.63 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
REQUEST_CHANGES — the integration build verification step uses the wrong tool and lacks a timeout.
### Key issues
go test -run='^$' is not a compile-only check — it executes TestMain in every package. In this repo, pkg/cli's TestMain runs make build, so both the test and fuzz job steps silently do a full binary build. The comments say "compile cleanly" but the command does substantially more. Use go test -c -tags=integration -o /dev/null ./... for a true compile-only check where TestMain is never invoked.
No step-level timeout — since TestMain runs make build, a hung network fetch or build step can block the runner indefinitely. Add timeout-minutes: 5 to both new steps.
The existing review comment (line 121) covers the test job variant; the fuzz job variant at line ~1391 has the same problem and needs the same fix.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 29.7 AIC · ⌖ 4.72 AIC · ⊞ 5.7K
Comment /review to run again
| # Fuzz tests such as FuzzSanitizeOutput run with -tags=integration; a build | ||
| # error in any integration-tagged file causes the entire fuzz run to fail | ||
| # with a misleading error message. | ||
| go test -run='^$' -tags=integration ./... |
There was a problem hiding this comment.
TestMain still executes — not a pure compile check: the fuzz job step runs test binaries in every package (including pkg/cli which invokes make build), silently doing more work than the comment claims and potentially masking real intent.
💡 Suggested fix
Use -c to compile without executing:
go test -c -tags=integration -o /dev/null ./...go test -c compiles each test binary and discards it; TestMain is never invoked. This gives a true compile-only check. The same fix applies to the test job equivalent step.
There was a problem hiding this comment.
Integration build verification — TestMain side-effect
The goal of catching unused-import build errors is correct and valuable. However, both new steps use go test -run='^$' -tags=integration ./... which, despite the empty run filter, still executes TestMain in pkg/cli/compile_integration_test.go. That TestMain calls make build to produce a binary (unless GH_AW_INTEGRATION_BINARY is set), turning a cheap compile check into a full binary build.
Recommended fix: Replace with go vet -tags=integration ./..., which type-checks all packages including integration-tagged test files without invoking TestMain:
- name: Verify integration build
run: go vet -tags=integration ./...See inline comments for both affected locations.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 21.4 AIC · ⌖ 4.61 AIC · ⊞ 5K
| # Fuzz tests such as FuzzSanitizeOutput run with -tags=integration; a build | ||
| # error in any integration-tagged file causes the entire fuzz run to fail | ||
| # with a misleading error message. | ||
| go test -run='^$' -tags=integration ./... |
There was a problem hiding this comment.
Same TestMain-executes-make build issue as line 121: go test -run='^$' -tags=integration ./... still invokes TestMain in pkg/cli/compile_integration_test.go, which calls make build unless GH_AW_INTEGRATION_BINARY is set.
Prefer go vet -tags=integration ./... which type-checks all packages (including integration-tagged test files) without executing TestMain or building the binary:
- name: Verify integration build
run: go vet -tags=integration ./...Alternatively, keep go test but supply a dummy binary path so TestMain skips the build:
- name: Verify integration build
env:
GH_AW_INTEGRATION_BINARY: /bin/true
run: go test -run='^$' -tags=integration ./...@copilot please address this.
|
@copilot Run pr-finisher skill |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 7f1e235. The integration verification steps now use |
|
🎉 This pull request is included in a new release. Release: |
Fuzz run #18583 on
mainfailed becauseFuzzSanitizeOutput(which builds with-tags=integration) couldn't compile due to an unusedassertimport inforbidden_fields_import_test.go. The root-cause import removal landed in94beb4a677; this PR adds a guard so the same class of failure is caught before reachingmain.Changes
testjob — addsgo test -run='^$' -tags=integration ./...after dependency verification. Compiles all integration-tagged test files on every push/PR without running any tests, exposing build errors (unused imports, missing symbols) that the regular-tags '!integration'step silently skips.fuzzjob — adds the same step as a pre-flight check before the fuzz matrix runs. Ensures integration-tagged files compile cleanly so a build failure surfaces with a clear error rather than a misleading fuzz-test failure.The underlying issue: regular CI excludes integration files; the fuzz job was the only thing that compiled them, and it ran only on
main.Run: https://github.com/github/gh-aw/actions/runs/29961275366