feat(bom): raw-SKU line items for estimate_bom and compare_bom_regions (RC3-004, #31) - #96
Merged
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.
x7even
marked this pull request as ready for review
July 7, 2026 00:02
x7even
added a commit
that referenced
this pull request
Jul 7, 2026
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).
x7even
added a commit
that referenced
this pull request
Jul 9, 2026
* feat(bom): add raw-SKU line items to estimate_bom and compare_bom_regions (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. * feat(gcp): add raw-SKU lookup parity with AWS (RC3-015, #35) 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. * fix(server): declare tiered/all_tier_rates in SKU-lookup output schemas 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). * docs(sku-lookup): fix stale AWS-only doc comments after GCP generalization 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. * feat(azure): add raw-SKU lookup parity with AWS/GCP (RC3-015 Azure) 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. * fix(server): populate MCP structuredContent on every tool response 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. * test(harness): add 71 raw-SKU-lookup coverage tests across AWS/GCP/Azure 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. * fix(bom): require explicit provider for raw-SKU BoM items 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. * chore(build): bump go toolchain to 1.25.12 to fix GO-2026-5856 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.
3 tasks
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
Progresses #31 (RC3-004). Scoped to raw-SKU line items only (v1: AWS-only, synchronous per-region) — weighting and a
providers[]filter are deferred to a follow-up issue, per the original issue's own DACI-listed fallback scope ("(a) Build v1 AWS-only, synchronous per-line calls").processBOMItems(shared byestimate_bomandcompare_bom_regions) now accepts a raw-SKU dict alongside PricingSpec dicts, resolving it via the same AWS SKU resolverget_price_by_skualready uses (LookupSKUAcrossRegions).compare_bom_regions's partition loop routes raw-SKU items through the same AWS-only gate as PricingSpec items, reporting a non-AWS raw-SKU item once innot_supportedrather than once per compared region.BOMAdvisories(egress/LB/NAT/backup rows) by mapping the AWS Pricing API servicecode to the short-form tokenBOMAdvisoriesexpects.provider/service/operation/product_family) now distinguishes "absent" from "wrong type," surfacing a clear error instead of silently defaulting.rawBOMSKU,stringItemField,classifySKURegionResult,resolveAWSSKUProviderFromMap) so this logic isn't duplicated betweenbom.goandsku_lookup.go.This also gives #33 (RC3-014, blocked on RC3-004) its proposed unblock mechanism: DynamoDB and other CUR-native services that fail
estimate_bom's PricingSpecSupports()check can now be passed as raw-SKU items instead.Ran through
/code-review(high effort, 8 finder angles + verification) against the implementation; this PR includes fixes for all 9 confirmed/plausible findings from that review (duplicate not_supported errors, broken advisory matching, untrimmed SKU whitespace, silent type-coercion on hint fields, duplicated SKU-result discrimination logic, dropped error codes, stale tool description).Test plan
go build ./...go vet ./...go test ./...(all packages, includingTestDescriptionParityWithSnapshot/TestOutputSchemaParityWithSnapshot)gofmt -lon all touched files (clean)