ci: lint job is root-only — 1291 golangci-lint findings across five unlinted workspace modules

[cristim]

The lint job in .github/workflows/ci.yml runs golangci-lint and go vet from the repository root. Under go.work that covers the root module only, so the other five workspace modules are unlinted — and they are red.

This is the same defect as LeanerCloud/cloud-commitments-cli#1751 (test jobs root-only), in the same file, one job over. LeanerCloud/cloud-commitments-cli#1751 fixes the test steps; this issue is deliberately kept separate so a lint-debt excavation does not bury a CI-gating fix, and vice versa.

Measured

golangci-lint run at the CI-pinned v2.10.1, per module, counts from golangci's own summary:

module	issues
. (root, the only one CI lints)	0
pkg	205
providers/aws	272
providers/azure	592
providers/gcp	222
tests/e2e	tool error, see below
total unlinted	1291

tests/e2e exits 5 with context loading failed: no go files to analyze — every file in it is behind //go:build e2e, so it needs --build-tags=e2e to be linted at all. Same root cause as the go test half of LeanerCloud/cloud-commitments-cli#1751.

Not all of it is cosmetic — but less of it is serious than first stated

Correction. An earlier revision of this issue called the 193 bodyclose findings "a resource-leak class on a provider that makes real API calls" and recommended triaging them first. That was wrong, and the correction inverts the recommendation. All 193 are in _test.go files — zero in production code — and they sit on okJSONResponse(...), a helper that hand-builds *http.Response fixtures for a fake HTTP client. There is no network resource behind them. Volume aside, they are the least urgent part of this backlog, not the most.

Split by production versus test code, which is the split that decides urgency:

linter	production	test
errcheck	18	0
unused	12	6
staticcheck	10	1
errorlint	8	0
gosec	1	0
bodyclose	0	193

So the set actually worth triaging is 49 production findings — errcheck 18 (unchecked errors, spread across all four modules), unused 12, staticcheck 10, errorlint 8 (providers/gcp), gosec 1 (providers/aws). Everything else is godot (502), misspell (201), gocritic (80) and the test-only bodyclose block.

Suggested approach

Mirror the per-module loop already used by govulncheck (ci.yml:312), the gosec SARIF scan (:333), and — once LeanerCloud/cloud-commitments-cli#1751 lands — the test steps. But do not turn the gate on and fix 1291 findings in one PR. Suggested split:

1. Triage the 49 production findings first (errcheck, unused, staticcheck, errorlint, gosec) — these are the ones that can represent real defects, and the set is small enough to review individually.
2. Handle the 193 test-only bodyclose findings separately — LeanerCloud/cloud-commitments-cli#1759. Volume, not risk.
3. Land the cosmetic sweep (godot, misspell, gocritic) separately, ideally with --fix, in its own PR. Note the standing caution that autofix must not rewrite SQL identifiers or test-asserted strings.
4. Only then add the modules to the lint loop, so the gate goes green on the same commit it goes live.

Reversing that order means either a long-red main or a suppressed gate, and suppressing is not on the table.

Verification

Same shape as LeanerCloud/cloud-commitments-cli#1751: after the loop is added, break a lint rule deliberately in providers/azure and confirm CI goes red. Before the change it will not.

Related

• ci: unit and integration test jobs only run the root module — providers/*, pkg and tests/e2e are never tested cloud-commitments-cli#1751 — test jobs root-only. Same file, same cause, fixed separately.
• fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore cloud-commitments-cli#1740 / PR fix(test): repair 30 unfailable mock assertions and guard the class cloud-commitments-cli#1750 — where the multi-module blind spot first surfaced: four unfailable money-path mock assertions sat in providers/aws, a module CI neither tested nor linted.
• PR fix(test): repair 30 unfailable mock assertions and guard the class cloud-commitments-cli#1750 verified that its own change adds zero new providers/aws findings — 272 with and without it, identical per-linter breakdown. All 272 predate it.

[cristim]

Triage of the 49 production findings

Requested: classify by reachability, size it, and answer specifically — which errcheck/errorlint sites are on live provider call paths, and does any swallow an error on a money operation?

Measured against bce0c2364 (main + the LeanerCloud/cloud-commitments-cli#1751 workflow change; no Go source differences). Nothing fixed.

Direct answer

No finding swallows an error on a money operation. One is a genuine control-flow defect on the purchase path and is worth fixing on its own; the other 48 are deliberate, already-documented, or fail-loud.

The headline number is misleading in the same way the bodyclose one was. All 18 errcheck findings are explicit blank discards (v, _ := or _ = f()), not unchecked calls — .golangci.yml sets errcheck.check-blank: true and check-type-assertions: true, so deliberate discards are reported by design. "18 unchecked errors" is not 18 latent bugs.

---

Worth fixing: 1

providers/azure/services/internal/reservations/purchase.go:475 — in doPurchase, on the Azure reservation purchase path.

body, _ := io.ReadAll(resp.Body)
...
return fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body))

That error string is then consumed by:

func IsSessionTimeout(err error) bool { return strings.Contains(err.Error(), sessionTimeoutFragment) }

and the retry loop branches on it:

if IsSessionTimeout(purchaseErr) && attempt < purchaseMaxAttempts { ... continue }
return "", purchaseErr

So the discarded io.ReadAll error decides retry versus abort. If the read fails or truncates before the session-timeout fragment on a 400, IsSessionTimeout returns false and the purchase is abandoned rather than retried — a condition the code is explicitly designed to retry.

Direction: fails closed. It abandons a purchase, it does not double-buy. But it converts a designed-retryable failure into a permanent one silently, and the operator sees status 400: with an empty body. This is the only site among the 49 where a discarded error feeds control flow rather than a message.

Fix: check the read error and surface it, or classify session timeout from a source that does not depend on a successful body read. effort/xs.

Documented and correct: 17 of the 18 errcheck

site(s)	why it is not a defect
azure/recommendations.go:220, gcp/recommendations.go:161,322 — _ = g.Wait()	Deliberate, with a comment: every goroutine returns nil, per-service errors are captured separately, and ctx.Err() is checked on the very next line so cancellation is not swallowed. Verified in all three.
pkg/concurrency/concurrency.go:61	SharedSemaphore documents "or nil if none"; Acquire/Release nil-guard explicitly. The , _ is the contract.
aws/recommendations/converters.go:40,82,110	convertPaymentOption is already marked Deprecated with the silent-empty behaviour spelled out, fails closed at Cost Explorer (which rejects ""), retained only for a legacy path, tracked to LeanerCloud/cloud-commitments-cli#865/#1075.
pkg/config/load.go:345,349,353,357	cobra flags.GetBool/GetString error only when the flag is undefined — a build-time mistake, not runtime. --yes defaulting to false is the safe direction.
azure/internal/pricing/retail_prices.go:110, azure/services/compute/client.go:372,406	body feeds a JSON decode that fails on truncation, or an error message. Fail-closed.
azure/mocks/azure_mocks.go:134	test-support package; every importer of providers/azure/mocks is a _test.go file (verified).
purchase.go:287 (doCalculatePrice), :390 (fetchReservationOrdersPage)	Truncation makes json.Unmarshal fail, so both return errors. :390 backs the idempotency lookup, and its caller DoIdempotentPurchaseTwoStep explicitly refuses to purchase on a lookup error to avoid a double-buy — there is no fall-through.

errorlint: 8, none reachable as a defect

• 6× err == iterator.Done (computeengine ×3, cloudsql, cloudstorage, memorystore) plus gcp/provider.go:430. Google's iterator contract returns iterator.Done unwrapped. Were it ever wrapped, the following if err != nil returns an error — the loop fails loudly rather than truncating silently, which is the direction that matters.
• gcp/recommendations.go:431 isGoogleAPIError — errorlint sees err.(*googleapi.Error) but the function already hand-rolls recursive unwrapping below it, so the stated premise is false for Unwrap() error chains. One narrow real gap: it does not handle Unwrap() []error (errors.Join), which errors.As would. It feeds isPermissionError, which decides Warn-vs-ERROR logging (fix(frontend): the approval modal falls back to a no-amount body and still offers Approve #247) — observability, not money.

staticcheck 10, gosec 1, unused 12

• SA4004 ×3 (elasticache, memorydb, rds clients) — I checked rather than assuming, because "loop unconditionally terminated" can mean pagination that only ever reads page 1 on an offering-selection path. It does not: scanRDSOfferingPage intentionally takes the first offering from an already-payment-option-filtered page, and returns an error if that first one does not match the requested type. Style finding on deliberate take-first logic that fails closed.
• SA1019 config.Profile is deprecated ×3 (one per provider) — the repo's own deprecation, with typed replacements named in the message. A real migration, self-tracking.
• gosec G117 on ec2/client.go:982 ClientToken — matches a secret-name pattern. ClientToken is the EC2 idempotency key, not a credential.
• unused 12 — dead real*Service / real*BucketHandle types in gcp/cloudstorage and gcp/memorystore (10 of the 12), plus an unused const and field. Straight deletions, but they are the production implementations behind test doubles, so confirm nothing wires them up dynamically before removing.

---

Sizing

bucket	count	effort
purchase.go:475 retry-decision fix	1	xs
errorlint → errors.Is/errors.As (mechanical, low risk, mild robustness gain)	8	xs
unused deletions	12	s
SA1019 config.Profile migration to typed fields	3	s, needs care
Deliberate discards — //nolint:errcheck with the existing rationale, or leave and exclude	17	xs–s
SA4004, ST1005, QF1008, G117	7	xs

Recommendation: fix purchase.go:475 on its own, now. It is the only one with a behavioural consequence, it is xs, and it is on the purchase path. Everything else can ride with the lint-gate work and does not need to precede it.

The 17 deliberate discards deserve a decision rather than edits: they are correct code being reported because the config is strict. Annotating each with //nolint:errcheck plus the reason already in the surrounding comment makes that visible; the alternative is relaxing check-blank, which would also hide genuinely unchecked calls and should not be done.

Method notes

• Prod-vs-test split by file path; providers/azure/mocks additionally verified to have no non-test importer.
• Every claim above was read in source, not inferred from the linter name — the correction earlier in this issue came from doing exactly that inference.
• Any before/after comparison when this is fixed must be a comm set diff on normalized findings, not totals; and golangci-lint must show exit 0 and a non-empty findings block before being called clean.