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

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p2Backlog-worthyBacklog-worthy

   severity/mediumModerate harmModerate harm

   urgency/this-sprintWithin the current sprintWithin the current sprint

   impact/internalTeam-internal onlyTeam-internal only

   effort/lWeeksWeeks

   type/choreMaintenance / non-user-visibleMaintenance / non-user-visible

   on Aug 8, 2026
2. cristim commented on Aug 8, 2026

   

   cristim

   on Aug 8, 2026

   MemberAuthor

   More actions

   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.
3. cristim

   transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026