Add exhaustive switch vetting and enforce it across the tree#844
Conversation
Wire the nishanths/exhaustive analyzer in as a `go vet` vettool (new `make vet` target + go.mod tool directive) and make every enum switch satisfy it. Rather than blanket-suppress with `//exhaustive:ignore`, each switch now lists all enum members explicitly. Where the missing members should behave identically to the existing `default`, they're grouped into a case that `fallthrough`s into the default so runtime behavior is unchanged while a future enum addition forces a deliberate decision. The ignore directive is kept only on switches over large external/stdlib enums where enumeration is impractical, with the rationale inline: - syscall.Errno (~130 members) in httpingress - tea.KeyType (~90 members) in prompt/deploy_ui - reflect.Kind (~27 members) in entity reader and specs_match_test CI: run `make vet` as part of the lint job in test.yml.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request enables the exhaustive linter and makes switch statements explicit across the codebase. It adds explicit case branches, fallthroughs, and selective //exhaustive:ignore comments for many high-cardinality enums; updates disk/mount/volume reconciliation paths, sandbox lifecycle and health checks, watch/event conversion, authorization routing, saga terminal handling, value parsing/formatting, builder defaults, tests, and logging. No public APIs or behavioral changes were introduced; existing control flow and default behaviors are preserved. Comment |
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Yay, one step closer to Rust ;) We checked the few no-op cases (SetPortStatus x2, lsvd FlushNo) and they're all safe.
One ask before merging: can we wire it through golangci-lint (already in the lint job, bundles the same exhaustive analyzer) instead of a standalone go vet step? We'd get the shared single load, parallelism, and caching for free, and drop the vet target, CI step, and go.mod/go.sum churn. With golangci-lint already pinned, the bundled version is just as deterministic.
Only difference: golangci-lint still excludes a few dirs (lsvd/, disk/, etc.), so those won't be covered. Fine to punt, whether those should be linted at all is a separate cleanup TODO.
|
Sure, let me give it a look. |
golangci-lint bundles the exhaustive analyzer, so enable it there rather than maintaining a parallel `go vet` vettool. This drops the dedicated plumbing: - remove the `make vet` target - remove the `go vet (exhaustive)` step from the lint CI job - remove the nishanths/exhaustive tool directive from go.mod/go.sum The linter is configured with default-signifies-exhaustive: false to match the prior vettool behavior, and still honors `//exhaustive:ignore`. Tradeoff: golangci-lint's path exclusions are global, so the already lint-exempt paths (lsvd/, disk/, dataset/, pkg/cel, pkg/containerdx, *.gen.go) are no longer exhaustive-checked. The switches in those paths remain exhaustive; they're just not re-enforced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go.mod (1)
93-93: Go mod: gjson v1.18.0 looks safe; optionally bump for currency
github.com/tidwall/gjson v1.18.0(now a direct dependency) has no known security vulnerabilities reported specifically for that version. Latest upstream release is v1.19.1 (May 2026), so updating is recommended for staying current, not for security remediation.🤖 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 `@go.mod` at line 93, The go.mod currently pins github.com/tidwall/gjson at v1.18.0; update the dependency to a newer release (e.g., v1.19.1) by editing the module line for github.com/tidwall/gjson in go.mod, run go get github.com/tidwall/gjson@v1.19.1 (or go get -u github.com/tidwall/gjson) to update go.sum and tidy the module, and then run go mod tidy and go test to ensure nothing breaks; target symbols: the dependency entry "github.com/tidwall/gjson" in go.mod and the overall module's go.mod/go.sum maintenance commands.
🤖 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.
Nitpick comments:
In `@go.mod`:
- Line 93: The go.mod currently pins github.com/tidwall/gjson at v1.18.0; update
the dependency to a newer release (e.g., v1.19.1) by editing the module line for
github.com/tidwall/gjson in go.mod, run go get github.com/tidwall/gjson@v1.19.1
(or go get -u github.com/tidwall/gjson) to update go.sum and tidy the module,
and then run go mod tidy and go test to ensure nothing breaks; target symbols:
the dependency entry "github.com/tidwall/gjson" in go.mod and the overall
module's go.mod/go.sum maintenance commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 479c618a-a96d-4217-ae67-aea51a26863b
📒 Files selected for processing (2)
.golangci.ymlgo.mod
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
…ve-switch-vetting # Conflicts: # components/ipalloc/ipalloc.go
Merging the latest main surfaced enum switches that the now-enabled
exhaustive linter flags:
- controllers/sandbox/log.go: gjson.Type — handle Null/JSON (skipped)
- pkg/controller/controller.go: EntityOperation — Progress/Compacted
route to the existing no-op default
- pkg/entity/indexwatch/watcher.go: EntityOperation in two switches —
Progress/Compacted handled explicitly; Create/Update/Delete fall
through to eventFromOp
- components/ipalloc/ipalloc.go: indexwatch.EventType — handle
EventDeleted (resolved during the merge)
EntityOperation gained Progress and Compacted members on main, which is
exactly the kind of addition this linter is meant to catch.
…etting' into mir-1202-add-exhaustive-switch-vetting
Enabling the exhaustive analyzer in golangci-lint perturbs staticcheck's analyzer scheduling enough that it intermittently stops recognizing t.Fatal as terminating, raising a false-positive SA5011 (possible nil deref) on the guarded block.Bytes access. Add an explicit return after t.Fatal so the non-nil invariant holds structurally, independent of whether staticcheck models t.Fatal's control flow. No behavior change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@components/ipalloc/ipalloc.go`:
- Around line 189-190: The EventDeleted branch currently no-ops, which leaks
reservations in the allocations map; update the indexwatch.EventDeleted case to
release any reserved IPs for the deleted service by looking up the entry in
allocations (use the service key/ID used elsewhere in this file), call the
existing release function (e.g., releaseIP, ReleaseReservedIP or whatever the
file provides for freeing an allocation) or if none exists implement the reverse
of the allocation path to return the IP to the pool, and then remove the entry
from allocations so the subnet capacity is reclaimed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca0879d-2d5f-478e-b5c6-222295e60b33
📒 Files selected for processing (7)
components/ipalloc/ipalloc.gocontrollers/sandbox/log.gocontrollers/sandbox/sandbox.gopkg/controller/controller.gopkg/entity/indexwatch/watcher.goservers/httpingress/httpingress.goservers/runner/registration_test.go
✅ Files skipped from review due to trivial changes (4)
- servers/httpingress/httpingress.go
- controllers/sandbox/log.go
- pkg/controller/controller.go
- servers/runner/registration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/sandbox/sandbox.go
sandbox.go and volume.go gained explicit enum cases for the exhaustive linter (and sandbox.go also picked up changes from main). The parallel saga controller (saga_controller.go) carries the matching SandboxStatus case, so the two code paths stay in sync. Refresh the frozen hashes.
The ipalloc allocation table is append-only: IPs were recorded on snapshot/assign but never removed. A deleted service therefore held its addresses forever, slowly exhausting the subnet. Handle indexwatch.EventDeleted by pruning every allocation owned by the deleted service id, returning those IPs to the pool. Add a unit test covering the round-trip (release frees only the target service's IPs and they can be re-allocated). Fixes swt-e255.
…ve-switch-vetting # Conflicts: # controllers/sandbox/sandbox_frozen_test.go
2 participants
Summary
Enables the
exhaustiveanalyzer (bundled in golangci-lint) and makes every enum switch in the tree satisfy it, so adding a new enum member forces a compile-time decision at each relevant switch instead of silently slipping through adefault.What changed
exhaustivelinter in.golangci.ymlwithdefault-signifies-exhaustive: false(a baredefault:does not by itself make a switch exhaustive). It honors//exhaustive:ignoredirectives.default, they're grouped into a case thatfallthroughs into the default — so runtime behavior is unchanged, but the cases are now documented and checked. Each added case carries a short comment explaining why those members share that path (terminal / transient / unexpected / etc.).Intentional exceptions
//exhaustive:ignoreis kept only on switches over large external/stdlib enums where listing every arm is impractical, with the rationale inline:syscall.Errnoservers/httpingress/httpingress.gotea.KeyTypepkg/ui/prompt.go,cli/commands/deploy_ui.goreflect.Kindpkg/entity/reader.go,controllers/deployment/specs_match_test.goCoverage note
golangci-lint's path exclusions are global, so the already lint-exempt paths (
lsvd/,disk/,dataset/,pkg/cel,pkg/containerdx,*.gen.go) are not exhaustive-checked. The switches in those paths (3 inlsvd/) were still made exhaustive in this PR; they're just not re-enforced going forward.Verification
golangci-lint run ./...— no issues (exhaustive active; confirmed it flags a removed case)go build ./...and affected test packages compile