Repository navigation
fix(gcp): address remaining medium/low/nit findings from GCP broad audit (deferred from #1047) #6
Description
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)
Activity
fix(gcp): filter non-ACTIVE recs and wire cache/storage into fan-out
fix(gcp): wire Count/pricing into GCP converters; refuse fabricated p…
cristim commentedon Oct 17, 2026
Checked against main: L-5, L-6, M-3, M-4 and M-5 are fixed. The remaining items (A08-021, M-6, A08b-016, A08b-037, N-1, M-7) all sit in GCP files edited by open PRs #307, #433, #471 and #493. Blocked on those PRs.
cristim commentedon Oct 20, 2026
Claiming the GCP part: the isPermissionError shim (providers/gcp/recommendations.go:~331) and the duplicated stringPtr/int64Ptr helpers in computeengine. The dead-surface item is left to #471. [claude-cc-libs]
refactor(gcp): use the shared IsPermissionError and new(expr) instead…
refactor(gcp): thread ctx through ListSKUs, drop Provider.ctx
docs(changelog): note gcp stored-ctx breaking changes
refactor(gcp): remove unread stored ctx fields (#575)
fix(gcp): drop the unused context field from the CloudSQL, Storage an…
cristim commentedon Nov 12, 2026
Owner decision (2026-11-12): hold until #471 lands.
The staged change that deletes exported GCP interfaces (held change A08b-037, tree 3e7d2754) stays held until #471 lands. Nothing is pushed in the meantime. This issue stays open.
fix(gcp): classify RESOURCE_EXHAUSTED by error type only in computeen…
cristim commentedon Nov 13, 2026
Fixed the residual M-3 item:
isResourceExhaustedinproviders/gcp/services/computeengine/client.gono longer falls back to substring matching on the error text ("429", "RESOURCE_EXHAUSTED"). Only the typed googleapi 429 and gRPC ResourceExhausted checks remain, so unrelated errors that merely mention those strings are no longer retried.Commit: fb3c369
Test: TestIsResourceExhaustedClassifiesTypedErrorsOnly (fails before, passes after; mutation probe confirmed).
CI on that sha: pre-commit and CI - Build & Test succeeded.
Checked on main and already fixed: L-3, L-5, L-6, M-4, M-5, M-6, M-7, A08-021, N-1, A08b-016. Still open: D-1 and the held A08b-037 (untouched, waiting on #471). Not closing this issue.
cristim commentedon Nov 13, 2026
D-1 plan (planning only, no code yet)
Base:
origin/mainat7ff8c1a. No code, commit or PR. This issue stays open.1. Duplication found
Hand count from line ranges, excluding comments and blank lines. Estimate, expect about ±10%;
git diff --numstatsettles it per commit.getXPricingbody:skulist.Resolve,listSKUs,ExtractPricing,ScaleToTerm, struct copyclient.go:146-172client.go:161-188client.go:165-191GetOfferingDetails:TermYears,failed to get pricingwrap, offering ID,skumatch.OfferingDetailsclient.go:119-133client.go:134-148client.go:138-152XPricingstruct (same five fields)client.go:133-141client.go:150-157client.golistSKUsplus service-ID const (test seam)client.go:183-190client.go:225-233client.go:202-210extractXPricingFromSKUswrapper (test seam)client.go:175-181client.go:190-196client.go:194-200PurchaseCommitmentno-op (purchase path, not in D-1)client.go:106-117client.go:121-132client.go:124-136About 250 code lines follow one shape across the three pricing clients. Compute Engine has a parallel but different path (
computeengine/pricing.go:356-397), covered in risk 3.Not duplicated across the four clients, so not in D-1:
origin/main. N-1 is already closed per the comment of 2026-11-13.computeengine/client.go:570-669:isResourceExhausted,isAlreadyExists,adoptExistingCommitment,stripPermanentPrefix,unwrapNonSentinel): only computeengine calls them. No copy exists in the other clients. They stay.cloudstorage/client.go:198-225:extractStoragePriceFromSKU,skuMatchesStorageClass): unit and description rules that are specific to storage. They stay.GetRecommendationsandGetExistingCommitments(1-3 body lines each, per-service error text and rationale): sharing them adds indirection with no caller benefit. They stay.skumatch.ExtractPricing,ScaleToTerm,MatchesTier,InRegion,OfferingDetailsandskulist.Resolveandskulist.Cache. D-1 only removes the per-client glue around them.2. Boundary and commits
New package
providers/gcp/services/shared. It importsskulist,skumatchandcommononly, so there is no cycle. Exports:type Term struct { Currency, HourlyRate, CommitmentPrice, OnDemandPrice, SavingsPercentage }func Price(ctx, PriceInput) (*Term, error): resolves billing, lists the cached catalog, runsskumatch.ExtractPricingandScaleToTerm. Keeps thefailed to list SKUswrap.func OfferingDetails(ctx, rec, prefix, region string, price func(ctx, tier, region string, termYears int) (*Term, error)) (*common.OfferingDetails, error)Commits, each well under 400 added plus deleted lines:
test(gcp): pin offering-details error wrap and offering ID across cloudsql, cloudstorage and memorystore. Tests only, about 60 lines. Adds the one missing characterization test: thefailed to get pricingwrap is not pinned by any cloudsql, cloudstorage or memorystore test today. It proves the current behaviour before any move.refactor(gcp): share the catalog pricing pipeline in services/shared. Moves thegetXPricingbodies and theirlistSKUscalls intoshared.Price. Keeps each service'sgetXPricingandlistSKUsas one-line wrappers.SQLPricing,StoragePricingandRedisPricingbecome type aliases ofshared.Term. About 190 lines changed, about -65 net.refactor(gcp): share GetOfferingDetails assembly in services/shared. EachGetOfferingDetailsbecomes a one-line call toshared.OfferingDetails. About 50 lines changed, about -20 net.Total: 3 commits, about -85 net code lines, about 250 duplicated lines in play.
3. Tests that pin current behaviour (must pass unchanged)
Test files call these unexported names directly, so each wrapper stays with the same name and signature:
getSQLPricing,extractSQLPricingFromSKUs,listSKUs,getStoragePricing,extractStoragePricingFromSKUs,extractStoragePriceFromSKU,skuMatchesStorageClass,getRedisPricing,extractPricingFromSKUs. No test file is edited in C1 or C2.Pinned behaviour:
cloudsql/client_test.go:314,cloudstorage/client_test.go:460,memorystore/offering_pins_test.go:25.failed to list SKUsatcloudsql/client_test.go:386,cloudstorage/client_test.go:531,memorystore/client_test.go:469. Scale messages at*/scale_errors_test.go:21-22.cloudsql/pagination_test.go:50,cloudstorage/pagination_test.go:50,memorystore/service_id_test.go:29.cloudstorage/cache_test.go. C1 adds one new test insharedthat countsListSKUscalls.*/term_test.go,*/ambiguity_test.go,*/usage_type_test.go,*/tiered_rates_test.go,*/negative_price_test.go,*/pricing_currency_test.go.Verification per commit:
make build,make test-unit,make vet,make lint, and(cd providers/gcp && GOWORK=off go test -race -short ./...)if deps resolve, otherwise thego.workform.Mutation probes (each must make a named test fail, then revert):
failed to get pricingwrap. The new C0 test fails.listSKUs. The three pagination and service-ID tests fail. Remove thefailed to list SKUswrap. The three error-text tests fail. Change the label"Cloud SQL tier "inPriceinput. The scale tests fail. PasstermYearsas 1 inPrice. The term tests fail. Bypassskulist.Cache. The new shared count test fails. Change theGiBy.modivisor inextractStoragePriceFromSKU.tiered_rates_test.gofails.termYearsargument inshared.OfferingDetails. The offering and term tests fail.4. Risks
StorageServiceinterface andSetStorageService(cloudstorage/client.go:75), theredisServicefield andSetRedisService(memorystore/client.go:71), and the SQL admin interface and methods in cloudsql. D-1 never edits those field lists, setters or interfaces. C1 and C2 only touch thegetXPricing,listSKUs,GetOfferingDetailsand pricing struct lines. The held tree is based on an oldermain, so whichever lands second must rebase. The owner's hold condition was "until fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471 lands". fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471 is now MERGED, but the owner's decision still says hold. Ask the owner to confirm before anyone touches the held change. D-1 does not wait on that answer.SQLPricing,StoragePricingandRedisPricingbecome aliases with identical fields, so composite literals and field access still compile. Confirm withgrepthat no test builds those types with a literal of a different shape before C1.PurchaseCommitment(cloudsql:106,cloudstorage:121,memorystore:124),computeengine.PurchaseCommitment(client.go:671),buildInsertRequest(:768),adoptExistingCommitment(:601) andisResourceExhausted(:570) do not change in D-1.computeenginekeeps its own SKU cache (pricing.go:381-397,skuCache,skuMu) and its own billing interface that takesctx. Moving it ontoskulist.Cachechanges the exportedcomputeengine.BillingServiceand the test seams. It is out of D-1 and should be its own follow-up. Itscommon.SavingsPctalso differs fromskumatch.SavingsPercent. Flag both in the follow-up.Also decided:
services/sharedis a new public package, as the issue names it. If the owner prefers no new public API, move it underproviders/gcp/internal/with a one-line path change in each commit.Review passes (project rule: 3 consecutive passes)
*SQLPricingliterals, and an open question about public API. Fixed in this plan: line numbers re-cited fromorigin/main, the literal check added to risk 2, and the public-API choice written out.OfferingDetailscallback signature must matchgetXPricingexactly, and the C0 test gap. Fixed: signature stated, C0 added.Next step: owner confirms the held-change hold and the
services/sharedpath. Then C0, C1, C2 land in that order onmainundermulti-agent-commslocks, each after its own staged-diff review.test(gcp): pin the failed-to-get-pricing wrap in cloudsql, cloudstora…
refactor(gcp): share the catalog pricing and offering pipeline in ser…
cristim commentedon Nov 13, 2026
D-1 landed on main as 3375436 (test pinning the "failed to get pricing" wrap in cloudsql, cloudstorage and memorystore) and 11e5304 (new providers/gcp/services/shared with Price, OfferingDetails and Term; the three pricing types are now aliases of shared.Term; unexported seams kept, so no existing test changed). Net -135 +218 lines including a 83-line shared test. Two independent opus review passes; pass 2 found a fieldalignment lint failure on PriceInput, fixed before commit. Design choice: C1 and C2 from the plan were one commit (both under 400 lines, same package), simplest sensible split.
Known gap (pre-existing): the service name passed to skulist.Resolve is only visible when no billing service is injected, so no test observes it.
All other findings were already fixed on main (see the earlier comment). Remaining: A08b-037 (dead StorageService/RedisService/SQLAdminService surface). That is held by the owner's 2026-11-12 decision, so this issue stays open and I did not touch it. Blocker: owner to lift the hold.
fix(gcp): rename services/shared to services/catalogprice
cristim commentedon Nov 14, 2026
Hold lifted: #471 merged (2026-11-13), which the 2026-11-12 decision named as the condition for A08b-037. Remaining A08b-037 removal touches cloudsql, cloudstorage and memorystore client.go, which a stale go-6b worktree still has uncommitted changes in. Coordinate that worktree before editing. Commented by claude-go-cli-mcp, 2026-11-14.