ci: get the lint job running again on golangci-lint v2 - #2
Merged
Conversation
The lint job has never passed. golangci-lint-action@v6 only knows golangci-lint v1, so `version: latest` resolved to v1.64.8, which is built with Go 1.24 and refuses to start against a go.mod targeting 1.25.0. It failed before reaching any code. Move to golangci-lint-action@v9 with golangci-lint v2.13.1, pinned rather than `latest` — floating is what silently picked a version incompatible with the toolchain. .golangci.yml is migrated to the v2 format (errcheck, govet, ineffassign, staticcheck and unused are v2 defaults; gofmt and goimports move to `formatters`), keeping the same effective linter set. With the linter actually running, it found 16 issues. Two were real: - api/firmware.go: upgradeInProgress was dead code. Both callers hold s.mu and use runningUpgradeLocked directly. Removed, and its note on the one-AP-at-a-time rule moved to the helper that survives. - driver/wax/pin_test.go: the path-stability check compared two identical calls inline, which staticcheck reads as a tautology. Same check, via variables, so the intent is legible. The rest are false positives against deliberate choices, each marked with the reason at the site: the sanitizer's 0644 fixtures, the detached upgrade goroutine, and the auth cookies, whose Secure flag is conditional so that login still works over plain HTTP on a LAN. gosec also flags the pinned-TLS setup for a resumption bypass that cannot happen here — Go only resumes when ClientSessionCache is set, and neither net/http nor we set one, so the pin is checked on every handshake. That is now written down next to the code. gosec's two taint-analysis rules are switched off. They are not deterministic in v2.13.1: on identical source with a cold cache, G703 reported three findings in api/history.go on roughly one run in three and none on the others. What they check is already enforced by core.ValidateDeviceName and covered by tests. Ten consecutive cold runs are now clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
lintjob has never passed on this repo. It was not failing on the code — it never reached the code.Why it was broken
golangci/golangci-lint-action@v6only supports golangci-lint v1, soversion: latestresolved to v1.64.8, which is built with Go 1.24 and refuses to start against ago.modtargeting 1.25.0:What changed
latest(floating is exactly what picked an incompatible version without anyone noticing)..golangci.ymlmigrated to the v2 format viagolangci-lint migrate, comments restored by hand. Same effective linter set: errcheck, govet, ineffassign, staticcheck and unused are v2 defaults, and gofmt/goimports move to the newformatterssection.What the linter found once it ran
16 issues. Two were real:
internal/api/firmware.go—upgradeInProgresswas dead code; both real callers holds.muand callrunningUpgradeLockeddirectly. Removed, with its note about the fleet-wide one-at-a-time rule moved onto the helper that survives.internal/driver/wax/pin_test.go— the path-stability check compared two identical calls inline, which staticcheck reads as a tautology (SA4000). Same check, held in variables, so what it is asserting is legible.The rest are false positives against deliberate choices, each annotated at the site: the sanitizer's
0644fixtures (committed, no secrets by construction), the detached upgrade goroutine (it must outlive the request), and the auth cookies — which already setHttpOnlyandSameSite, withSecuredeliberately conditional so login still works over plain HTTP on a LAN.The TLS one worth reading
gosec flagged the certificate-pinning setup (G123):
VerifyPeerCertificateis skipped on resumed TLS sessions, so a resumed connection could dodge the pin check. It cannot happen here — Go only resumes whentls.Config.ClientSessionCacheis set,net/httpnever sets one (checked the 1.25 stdlib), and neither do we, so every connection is a full handshake. That reasoning is now written next to the code, along with what to do if a session cache is ever added: move the check toVerifyConnection, which does run on resumed handshakes.Taint analysis is off, deliberately
G703andG706are disabled. They are not deterministic in v2.13.1 — on identical source with a cold cache, G703 reported three findings ininternal/api/history.goon roughly one run in three and none on the others. A check that fails at random is worse than no check. What they look for is already enforced bycore.ValidateDeviceName(a single safe path element, no separators, no..) and covered by the traversal cases ininternal/core/config_test.go.Verification
On the build host, in containers:
golangci-lint runinvocations: 0 issues each. (Before the exclusion, the same tree flipped between 0 and 3.)go build ./...,go vet ./...,gofmt -l .clean,go test ./...all packages pass.No access point was contacted.