fix(lint): pass build tags to the custom analyzers via GOFLAGS - #1325
fix(lint): pass build tags to the custom analyzers via GOFLAGS#1325morgo wants to merge 1 commit into
Conversation
singlechecker's own -tags flag is a documented no-op, and `go run -tags=X ./cmd/checker` applies the tag when building the checker binary rather than to the packages its loader analyzes. Both closeandlog and severityglyphs relied on one of those two forms, so neither had ever inspected an integration- or e2e-tagged file. Route the tags through GOFLAGS so they reach packages.Load, add the missing e2e leg, and give the pre-commit hook the integration/ and e2e/ directory legs the golangci-lint matrix above it already has. `go list` keeps its own -tags flag, where it was always correct. Turning the check on surfaced 93 pre-existing discarded closes, now converted to utils.CloseAndLog. The "redundant closer must discard" exception did not apply to them: sql.DB.Close() is idempotent by design and returns nil on every close after the first, so a redundant close on a pool an owner already closed logs nothing. AGENTS.md is corrected to split on idempotency, since *sql.Conn does return ErrConnDone on a second close — the one such site now skips the guard when the owner has closed the handle, keeping the error checked. Wire check-closeandlog into the lint workflow; it had no CI job, so nothing kept the analyzer's findings at zero.
There was a problem hiding this comment.
🟡 Changes recommended
The pre-commit script and Makefile currently clobber existing user GOFLAGS (and the script clears GOFLAGS on the default leg), which can unexpectedly change go behavior during lint runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes how custom Go analyzers (closeandlog + severityglyphs) receive build tags by routing tags through GOFLAGS so they reach packages.Load, adds an e2e tag leg, and updates CI/pre-commit coverage accordingly. It also applies the newly-effective closeandlog analyzer findings by converting many discarded Close() calls to utils.CloseAndLog.
Changes:
- Pass
integration/e2ebuild tags to custom analyzers viaGOFLAGS(instead ofgo run -tags/ singlechecker-tags), and add the missinge2eleg. - Expand the pre-commit hook to analyze
integration/ande2e/directories, and wirecheck-closeandloginto the GitHub Actions lint workflow. - Replace many discarded close calls (
_ = x.Close()) withutils.CloseAndLog(...), and update redundant-closer guidance inAGENTS.md.
File summaries
| File | Description |
|---|---|
| scripts/lint-fix.sh | Routes analyzer build tags via GOFLAGS; expands analyzer coverage to integration/ and e2e/. |
| Makefile | Updates check-closeandlog to run with integration/e2e tags via GOFLAGS. |
| .github/workflows/lint.yaml | Adds CI execution of make check-closeandlog in the lint workflow. |
| AGENTS.md | Updates repository guidance on redundant closers based on handle idempotency (*sql.DB vs *sql.Conn). |
| pkg/webhook/webhook_misc_integration_test.go | Replaces discarded DB close with utils.CloseAndLog. |
| pkg/webhook/webhook_integration_test.go | Replaces multiple discarded closes in integration tests with utils.CloseAndLog. |
| pkg/webhook/terminal_apply_head_publish_test.go | Replaces discarded DB close in test cleanup with utils.CloseAndLog. |
| pkg/webhook/rollback_integration_test.go | Replaces discarded DB closes with utils.CloseAndLog. |
| pkg/webhook/plan_integration_test.go | Replaces discarded closes across plan integration scenarios with utils.CloseAndLog. |
| pkg/webhook/plan_drift_integration_test.go | Replaces discarded closes in drift test setup/cleanup with utils.CloseAndLog. |
| pkg/webhook/plan_comment_retire_integration_test.go | Updates redundant-close guard logic and uses utils.CloseAndLog. |
| pkg/webhook/plan_change_ownership_integration_test.go | Replaces discarded closes in ownership tests with utils.CloseAndLog. |
| pkg/webhook/fanout_two_deployment_integration_test.go | Replaces discarded DB close with utils.CloseAndLog. |
| pkg/webhook/direct_gate_integration_test.go | Replaces discarded closes for DB/rows with utils.CloseAndLog. |
| pkg/webhook/check_records_stopped_test.go | Replaces discarded DB close in test cleanup with utils.CloseAndLog. |
| pkg/webhook/check_records_rollback_test.go | Replaces discarded DB close in test cleanup with utils.CloseAndLog. |
| pkg/webhook/check_records_refused_plan_test.go | Replaces discarded DB close in test cleanup with utils.CloseAndLog. |
| pkg/webhook/blocked_gate_integration_test.go | Replaces discarded DB close in test setup with utils.CloseAndLog. |
| pkg/webhook/auto_plan_integration_test.go | Replaces discarded close(s) with utils.CloseAndLog. |
| pkg/webhook/apply_comment_integration_test.go | Updates multiple test fixtures/cleanups to use utils.CloseAndLog. |
| pkg/tern/local_client_integration_test.go | Replaces discarded closes in integration tests with utils.CloseAndLog. |
| pkg/storage/internal/sqlstore/postgres_integration_test.go | Replaces discarded closes on postgres DB handles with utils.CloseAndLog. |
| pkg/storage/internal/sqlstore/mysql_test.go | Replaces discarded close on test DB with utils.CloseAndLog. |
| pkg/namedlock/namedlock_integration_test.go | Fixes redundant-closer behavior for *sql.Conn using an atomic termination guard + utils.CloseAndLog. |
| pkg/engine/spirit/direct_integration_test.go | Replaces discarded rows.Close() with utils.CloseAndLog. |
| pkg/auth/dex_integration_test.go | Replaces discarded listener close with utils.CloseAndLog. |
| integration/workflow_test.go | Replaces discarded closes for server/service/clients with utils.CloseAndLog. |
| integration/setup_test.go | Replaces discarded closes in integration test harness setup/teardown with utils.CloseAndLog. |
| integration/hybrid_mode_test.go | Replaces discarded gRPC conn close with utils.CloseAndLog. |
| integration/grpc_integration_test.go | Replaces discarded closes in GRPC integration test setup with utils.CloseAndLog. |
| integration/cli_test.go | Replaces discarded close(s) in CLI integration tests with utils.CloseAndLog. |
| e2e/local/local_test.go | Replaces discarded closes in e2e cleanup with utils.CloseAndLog. |
| e2e/local/helpers_test.go | Replaces discarded rows.Close() with utils.CloseAndLog in e2e helper polling. |
| e2e/grpc/helpers_test.go | Replaces discarded DB close with utils.CloseAndLog in grpc e2e helper. |
| e2e/grpc/grpc_test.go | Replaces discarded closes in grpc e2e cleanup with utils.CloseAndLog. |
Review details
Suppressed comments (1)
scripts/lint-fix.sh:246
- Same GOFLAGS issue as run_closeandlog: when build_tags is empty this forces GOFLAGS="" and clears any existing user GOFLAGS for the default analyzer run, which can change go command behavior unexpectedly.
- Files reviewed: 35/35 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @GOFLAGS=-tags=integration go run ./cmd/closeandlog-check ./... | ||
| @GOFLAGS=-tags=e2e go run ./cmd/closeandlog-check ./... |
| fi | ||
|
|
||
| if ! go run $tag_flag ./cmd/closeandlog-check "${packages[@]}" 2>&1; then | ||
| if ! GOFLAGS=${build_tags:+-tags=$build_tags} go run ./cmd/closeandlog-check "${packages[@]}" 2>&1; then |
singlechecker's own-tagsflag is a documented no-op, andgo run -tags=X ./cmd/checkerapplies the tag when building the checker binary, not to the packages its loader analyzes.check-closeandlogand the pre-commit hook'srun_closeandlog/run_severityglyphseach relied on one of those two forms, so neither analyzer had ever inspected an integration- or e2e-tagged file.Changes
GOFLAGSat every call site so they reachpackages.Load, and add the missinge2eleg.go listkeeps its own-tagsflag, where it was always correct.integration/ande2e/directory legs the golangci-lint matrix above it already has, and drop its stale claim that e2e packages cannot be analyzed. Verified by injecting a violation into an e2e file: the hook now fails on it, where before it passed.check-closeandloginto the lint workflow. It had no CI job at all, so nothing kept its findings at zero.utils.CloseAndLog.On the redundant-closer exception
Most surfaced sites looked like the sanctioned "redundant closer must discard its error" case, but that rule's premise does not hold for
*sql.DB:Close()is idempotent by design and returnsnilon every close after the first, so a redundant close on a pool an owner already closed logs nothing. Thesql: database is already closederror the rule cites comes from querying a closed pool, not from closing it twice. No suppression mechanism was needed.The exception is real for
*sql.Conn, which returnsErrConnDoneon a second close. The one such site — the terminable lock connection inpkg/namedlock— now skips the guard once the owner has closed the handle, so the close error stays checked rather than discarded. AGENTS.md is corrected to split on idempotency, and five in-code comments asserting the false "guaranteed already-closed error" are updated.Notes for review
pkg/localscalehas three failing subtests on this branch. They fail identically onmainate8a8a4c8and are unrelated to this change.This PR was prepared by Claude Code (Opus 5).