feat(gcp): add raw-SKU lookup parity with AWS (RC3-015, #35) - #97
Merged
Conversation
…ions (RC3-004, #31) Route CUR-style usage-type/SKU strings through processBOMItems (shared by estimate_bom and compare_bom_regions) via the same AWS SKU resolver get_price_by_sku already uses, so both tools accept a raw-SKU item alongside PricingSpec dicts. Scoped to raw-SKU support only; weighting and a providers filter are deferred.
Extends get_price_by_sku, get_prices_by_sku, and the raw-SKU branches of estimate_bom/compare_bom_regions to support provider="gcp", matching AWS's existing raw-SKU lookup. Hoists the shared lookup types/interface into a new internal/skulookup package so both providers implement one contract. GCP's region attribution follows a geoTaxonomy-first, serviceRegions-fallback rule (GLOBAL/REGIONAL/MULTI_REGIONAL), since restrictive serviceRegions lists on otherwise-global SKUs (observed in KMS) make plain serviceRegions matching unreliable on its own. Checked all 13 onboarded GCP services for MULTI_REGIONAL geoTaxonomy usage before generalizing Firestore's multi-region short-name parser — only Firestore needs it, so it stays Firestore-specific rather than being hoisted into a shared abstraction prematurely. Also fixes graduated tiered-rate billing math, region-name case sensitivity, a GCP catalog-fetch cache-stampede risk (singleflight coalescing), and removes dead code left behind by the provider-generic refactor, found via an 8-angle review pass over the initial implementation.
x7even
marked this pull request as ready for review
July 7, 2026 11:17
# Conflicts: # opencloudcosts-go/internal/server/server.go # opencloudcosts-go/internal/tools/bom.go # opencloudcosts-go/internal/tools/bom_test.go # opencloudcosts-go/internal/tools/compare_bom_regions.go # opencloudcosts-go/internal/tools/compare_bom_regions_test.go # opencloudcosts-go/internal/tools/sku_lookup.go # opencloudcosts-go/schemas/tools-snapshot.json
get_price_by_sku and get_prices_by_sku already emit tiered/all_tier_rates for GCP usage-volume-tiered SKUs and document it in their descriptions, but the OutputSchema for both tools never declared the fields. Found during merge-conflict review while reconciling PR #96 (raw-SKU BoM line items) with PR #97 (GCP SKU lookup parity).
…ation sku_lookup.go, bom.go, and aws_sku_lookup.go still described raw-SKU lookup as an AWS-only concept in doc comments and one user-facing error message, left over from before RC3-015 generalized it to also support provider="gcp". Comment/string-only changes, no logic touched.
Implements Azure as a third provider behind the shared skulookup.SKULookupProvider interface: fetch once by meterId (no per-region calls), bucket by ArmRegionName, disambiguate by IsPrimaryMeterRegion then type/spot-meterName hints, with a collision-safe tier resolver that reports Ambiguous rather than guessing when Reservation-type rows share identical (meterId, region, type, tierMinimumUnits) but price differently - confirmed against live API data where 86/115 sampled Reservation groups collided this way. Wires provider="azure" through get_price_by_sku, get_prices_by_sku, estimate_bom, and compare_bom_regions raw-SKU items.
Every tool declares an OutputSchema, but the shared jsonText response helper only ever set Content (unstructured text), never StructuredContent - a pre-existing gap since the original Go server rewrite. Strict MCP clients (the official Python SDK, used by the local test harness) reject every tool call outright when a schema is declared but structuredContent is absent, which surfaced only now under harness testing. Also defaults two nil []string fields (get_price_by_sku/get_prices_by_sku's no_mapping_in[].attempted_services on Azure's no-mapping path, and the upstream_failure branch's regions when the caller omits regions) to empty arrays before they reach the response map - both marshal to JSON null otherwise, which fails schema validation since both fields are declared as non-nullable arrays.
Covers the 7 gaps flagged in code review: AWS raw-SKU success, GCP/Azure no-mapping, ambiguous match, GCP/Azure tiered rates, batch mixed outcomes, raw-SKU BoM line items, and protocol edge cases (missing region, empty batch, unsupported provider, oversized batch, blank SKU). Adds rsku_manifest.json alongside TEST_PROMPTS since the harness has no per-test expected-outcome schema; the manifest carries expected_outcome per test ID for post-run grading.
resolveBOMSKUItem (bom.go) silently defaulted a missing provider to "aws", risking misrouted lookups since BoM calls routinely mix items from multiple providers in one request. Now returns a clear error instead. compare_bom_regions.go's independent item-partitioning loop gets the matching fix, using stringItemField so a non-string provider value is reported as a type error rather than folded into "missing", and a shared notSupportedEntry helper plus rawSKUProviderRequiredHint constant to keep the two call sites' messages from drifting apart. Also updates the stale field-level items schema description (server.go and tools-snapshot.json) that still said "provider aws or gcp" and omitted the required-provider behavior, and strengthens the regression test to reuse the existing makeComputePrice fixture helper.
go1.25.11's crypto/tls has an Encrypted Client Hello privacy leak (GO-2026-5856), flagged by CI's govulncheck step. Fixed upstream in go1.25.12.
This was referenced Jul 9, 2026
x7even
added a commit
that referenced
this pull request
Jul 9, 2026
go.mod requires go >= 1.25.12 (GO-2026-5856 toolchain bump), but the Docker builder stage was still pinned to 1.25.11, breaking the Publish Docker image workflow on every main push and tag since #97.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
provider="gcp"support toget_price_by_sku,get_prices_by_sku, and the raw-SKU branches ofestimate_bom/compare_bom_regions, matching AWS's existing raw-SKU lookup (closes RC3-015, RC3-015 (P2, SCOPED — GCP-first, needs live catalog access to implement): no GCP equivalent of get_price_by_sku exists #35).SKULookupResult,SKULookupRegionResult, error/hint-status constants) into a newinternal/skulookuppackage behind aSKULookupProviderinterface, so AWS and GCP implement one contract instead of AWS-only types being reused by convention.serviceRegionslist despite being globally priced — plainserviceRegionsmembership matching alone is unreliable.nam5/nam7/eur3), checked all 13 onboarded GCP services forMULTI_REGIONALgeoTaxonomy usage. Only Firestore needs it — every other service either uses plain literalserviceRegionsmatching or is pure GLOBAL — so it stays Firestore-specific rather than being hoisted into a shared abstraction prematurely. A live authenticated catalog check (not possible in this environment) is flagged as a follow-up to fully rule outMULTI_REGIONALon the untested services.singleflightcoalescing, dead code removal, several duplicated-logic consolidations). Two were reviewed and left as-is with reasoning in the commit.Test plan
go build ./...go vet ./...gofmt -l .(clean)go test ./... -count=1(all packages pass, including 9 newTestLookupSKUAcrossRegionsGeneric_*cases ininternal/providers/gcp)provider="gcp"end-to-end throughget_price_by_sku/get_prices_by_skuandcompare_bom_regions' raw-SKU partitioning — reachable with no schema-level blocks