fix(gcp): address remaining medium/low/nit findings from GCP broad audit (deferred from #1047) #6

Summary

Residual findings from the GCP broad-audit code review (docs/code-review/10-gcp-and-pkg.md), deferred from PR LeanerCloud/cloud-commitments-cli#1047 to keep that PR focused on the two Highs (H-1 rec-state filter, H-2 cache/storage fan-out wiring).

Findings to address

Medium

M-5 -- convertGCPRecommendation static PaymentOption only partially matches GetOfferingDetails' switch. Compute hardcodes "upfront"; cloudsql/memorystore/cloudstorage hardcode "monthly". Combined with scorer path, recs may reach GetOfferingDetails with the wrong payment option. Fix: derive PaymentOption from RecommendationParams.PaymentOption or leave it empty and let the offering step set it.

M-3 -- isResourceExhausted in computeengine classifies retryability by substring-matching the error string. The sibling permission_errors.go:IsPermissionError correctly uses errors.As/status.FromError. Fix: replace substring match with *googleapi.Error + codes.ResourceExhausted typed checks; promote a shared gcp.IsRetryable.

M-4 -- Hardcoded discount multipliers (0.63/0.45 for compute, 0.85/0.80 for SQL, etc.) presented as precise OfferingDetails when no commitment SKU exists. Fix: mark estimated pricing with an Estimated bool flag or treat absence as "pricing unavailable" rather than fabricating.

M-6 -- Struct-stored ctx context.Context field on all four GCP service clients and RecommendationsClientAdapter. The field is stored but never read; all methods receive ctx as a parameter. Remove the field and update the tests that assert on it.

M-7 -- GetAccounts synthesises a single-account list when ListProjects returns zero ACTIVE projects, hiding real credential/permission failures. Return the empty list or a descriptive error.

Low

L-3 -- cloudsql.GetExistingCommitments item-vs-page cap mislabel: legacy PricingPlan == "PACKAGE" is the deprecated per-instance billing mode, not a spend-based CUD. Return empty (aligned with the no-op purchase stance).

L-5 -- logging package: SetLevel/default logger mutate defaultLogger.level without synchronization. Under concurrent fan-out with go test -race, a concurrent SetLevel is a data race. Fix: use atomic.Int32.

L-6 -- MaskToken returns inputs of <=8 chars verbatim; add hard (redacted) for short inputs to prevent accidental secret logging.

Nit / Duplication

N-1 -- stringPtr/int64Ptr hand-rolled in computeengine; use common.Ptr[T] once LeanerCloud/cloud-commitments-cli#1035 lands.

D-1 -- The four GCP service clients are ~80% copy-paste (billing wrappers, SKU extraction, pricing helpers, term conversion, converter). Extract a gcp/services/shared package.

References

• Source: docs/code-review/10-gcp-and-pkg.md findings M-3 through M-7, L-3 through L-6, N-1, D-1
• Parent PR: fix(gcp): wire Count/pricing into GCP converters; refuse fabricated prices cloud-commitments-cli#1047 (addresses H-1 rec-state filter and H-2 cache/storage fan-out)

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A08b-016 (high)

M-4 asks for absent GCP pricing to be treated as unavailable rather than fabricated. There is a second path with the same outcome: when the catalog lookup itself fails, fillSQLPricing logs and returns without writing any cost field (providers/gcp/services/cloudsql/client.go:505-510, same at cloudstorage/client.go:497-502 and memorystore/client.go:491-496). convertGCPRecommendation has already set EstimatedSavings from the Recommender payload at line 558 before calling it at 566, so the emitted row carries a real savings number with CommitmentCost 0, OnDemandCost 0 and SavingsPercentage 0. Any ranking that divides by cost, or a savings-per-dollar-committed screen, reads that as a free commitment with positive savings and ranks it first. Audit finding A08b-016.

A08-021 (medium)

M-3 has a sibling in the same package, and fixing them together removes one shim entirely. providers/gcp/recommendations.go:402 declares a private isPermissionError while the package already exports IsPermissionError (permission_errors.go:29-42) using errors.As plus the gRPC codes.PermissionDenied surface. The local copy unwraps through isGoogleAPIError (recommendations.go:430-441), which recurses only over single Unwrap() error chains -- so it misses an error joined with errors.Join or a multi-%w wrap -- recognises no gRPC status at all, and falls back to a loose strings.Contains(msg, "403") && strings.Contains(msg, "permission"). The errorAs shim at :417 exists solely to support it and has no other caller. Calling the exported helper lets isPermissionError, errorAs and isGoogleAPIError all go. (audit finding A08-021)

A08b-037 (medium)

Another dead-surface item for this bucket, in the same clients as M-6/L-3. Since PurchaseCommitment became a not-supported no-op and GetExistingCommitments returns nil, cloudstorage's StorageService interface, its wrapper and SetStorageService are written and never read (field at client.go:64, setter at :81); the same holds for memorystore's redisService / CreateInstanceOperation / realRedisService (field :65, setter :82, CreateInstance wired at :104) and for SQLAdminService.InsertInstance / ListInstances in cloudsql (:26-27, :88-95, while the live path reaches the API only through ListTiers at :312). This keeps cloud.google.com/go/storage and a Redis create-instance capability linked into a client whose documented contract (LeanerCloud/cloud-commitments-cli#640) is that it must never create billable resources. Deleting the interfaces, wrappers, setters and mocks would remove that capability entirely. (audit finding A08b-037)