feat(azure): wire billingPlan into 6 sibling reservation services (#1502) - #1505
Closed
cristim wants to merge 43 commits into
Closed
feat(azure): wire billingPlan into 6 sibling reservation services (#1502)#1505cristim wants to merge 43 commits into
cristim wants to merge 43 commits into
Conversation
Design pass for the MCP server that exposes CUDly's RI/SP/CUD purchase surface to Claude. Documents the CLI surface map, Go-SDK-direct approach (no shell-out), per-(provider,product,action) tool schemas, credential exposure, safety rails (dry-run default, confirm gate, source enum, idempotency), file layout, and a PR-0..PR-9 implementation sequence. Refs #1488
Design work is complete; implementation follows in this same PR.
Adds PurchaseSourceMCP so the upcoming MCP server can stamp purchases it makes with a server-controlled enum value, never a free-form string. NormalizeSource now accepts cudly-mcp alongside cudly-cli and cudly-web and reports it in the invalid-source error message.
Adds the CUDly MCP server foundation: a thin cmd/cudly-mcp/main.go entry point, mcp/server.go wiring, and a shared mcp/tools harness (typed enum validators, JSON-schema builder, and the dry_run/confirm purchase gate every purchase tool below will reuse). Registers the first tool, cudly_list_commitment_actions, which returns the live tool catalog built from each tool's own Descriptor() so the catalog can never drift from what is actually registered. SDK choice: github.com/modelcontextprotocol/go-sdk (v1.6.1, the current stable release; pre-release tags exist past it but were skipped). It is the spec owner's reference implementation and its generic AddTool infers JSON Schema from Go structs, which keeps every tool's schema typed rather than hand-built. mark3labs/mcp-go was the documented fallback if this SDK proved awkward; it did not, so no fallback was needed. The purchase gate (mcp/tools/purchase.go) is written and tested now, ahead of the AWS EC2 tool that will be its first real caller, so the safety-rail tests (confirm=false refuses execution; dry_run=true never resolves a provider client; same request derives the same idempotency token) land independently of any one provider's wiring.
Adds cudly_search_recommendations, a read-only wrapper over Provider.GetRecommendationsClient().GetRecommendations() -- the same call cmd/multi_service.go makes before purchasing. Provider name, payment option, term, and Savings Plans type filters are validated against the typed enums from the previous commit; the requested service is checked against the provider's own GetSupportedServices() so the tool can never drift from what each provider actually supports. No dry_run/confirm parameters: the tool never purchases anything, so there is nothing to gate.
Adds cudly_aws_ec2_ri_purchase, the first real-purchase tool, wired through the dry_run/confirm gate from mcp/tools/purchase.go. Term, payment option, and the EC2-specific platform/tenancy/scope dimensions (required by providers/aws/services/ec2/client.go's offering lookup) are validated against typed enums; platform/tenancy/scope default to the common case (Linux/UNIX, default tenancy, region scope) when omitted, documented in the schema rather than silently applied. Every real purchase stamps common.PurchaseSourceMCP and a deterministic idempotency token derived from the request's own identifying fields, so a retried call with identical arguments dedupes at the provider instead of double-purchasing.
Adds cudly_aws_opensearch_ri_purchase, cudly_aws_redshift_ri_purchase,
and cudly_aws_memorydb_ri_purchase via one generic
simpleAWSRIPurchaseTool: none of these three clients read
Recommendation.Details (providers/aws/services/{opensearch,redshift,
memorydb}/client.go), so they share an identical
region+resource_type+count+term+payment_option shape and the same
dry_run/confirm gate, differing only in service type and resource-type
description. A single test suite runs the shared safety-rail
assertions (confirm gate, dry_run gate, boundary validation, real
purchase wiring) once per product.
Adds cudly_aws_rds_ri_purchase and cudly_aws_elasticache_ri_purchase.
Both require a Recommendation.Details value their client's offering
lookup reads: RDS needs DatabaseDetails{Engine, AZConfig} (az_config
has no safe default -- providers/aws/services/rds/client.go refuses to
guess single-az vs multi-az since they have different prices and don't
cover each other's demand), and ElastiCache needs CacheDetails{Engine}
validated against the new CacheEngine enum (redis/memcached).
Adds cudly_aws_savingsplans_purchase, dollar-denominated rather than count-based (hourly_commitment, USD/hour). sp_type resolves to the precise per-plan-type ServiceType (e.g. ServiceSavingsPlansCompute) via the existing ServiceTypeForPlanType helper rather than the ServiceSavingsPlansAll umbrella sentinel, so the resolved client's own resolveSPPlanType cross-check rejects a mismatched Details.PlanType as defense in depth on top of the ValidateSPType boundary check. EC2Instance plans require region (they are region-scoped); Compute, SageMaker, and Database plans are account-level and default to the same us-east-1 single-query convention cmd/multi_service_helpers.go already uses for account-level Savings Plans recommendations.
Adds cudly_azure_compute_ri_purchase and cudly_gcp_computeengine_cud_purchase as full real-purchase tools, not dry-run-only. The design doc's retry-safety concern (RI/CUD identifiers derived from a timestamp instead of the idempotency token) turned out to already be fixed upstream: Azure compute dedupes via DoIdempotentPurchaseTwoStep + FindReservationOrderByIdempotencyToken (issue #721), and GCP computeengine derives both the commitment name and the native RequestId from the token (issue #654). Re-verified against the committed client code before enabling real purchases here. Two provider-specific quirks surfaced while wiring this: - Azure's purchase body never sends a billingPlanType, so every purchase uses Azure's default (upfront) billing plan regardless of payment_option -- flagged in the tool description as a pre-existing gap this PR does not fix, not silently routed around. - GCP's PurchaseCommitment reads Recommendation.Details as a value common.ComputeDetails (memoryMBFromDetails), not a pointer like every AWS Details assertion; the GCP tool sets memory_gb as a required field and matches that value-type shape exactly.
Documents install, per-provider credential setup (matching each provider's ambient-credential model, plus the per-call aws_profile/azure_subscription_id/gcp_project_id overrides), launch, ~/.claude/mcp.json registration, a worked search-then-preview-then- purchase example, the safety model (dry_run/confirm gate, typed enum validation, idempotency tokens), the Azure billing-plan and GCP vCPU/memory caveats flagged in the two previous commits, and troubleshooting.
Reproduced the CI-pinned golangci-lint version (v2.10.1, per ci.yml/feedback_golangci_exact_ci_version) locally and fixed everything it flagged across the mcp package: named result parameters on every *RecommendationFromArgs helper (gocritic unnamedResult), a direct Descriptor->ActionEntry struct conversion instead of a field-by-field literal (staticcheck S1016), American-English spelling in comments (misspell), and three govet shadow warnings from an inner `if err :=` reusing the outer named result's `err` identifier. 0 issues on a clean rerun.
Drives the real MCP protocol path (a real Client connected to the real NewServer over an in-memory transport) rather than a bare Go function call: connects, lists tools, then calls cudly_aws_ec2_ri_purchase with dry_run omitted (must default to true). Proves every tool's schema registers without error at connect time and that a dry-run purchase returns structured cost JSON through the full protocol stack with no AWS credentials configured in this test environment. Verified stable under -race across repeated runs.
idempotencyKeyFor only hashed provider/account/region/service/resource_type/ count/term/payment_option, ignoring rec.Details entirely. Two materially different purchases that only differ in a Details field (e.g. a $5/hr vs a $50/hr Compute Savings Plan, or a Linux vs Windows EC2 RI) collided on the same token, so the provider's idempotency dedupe would silently skip the second purchase instead of buying it. Fold every field of the Details type each purchase tool populates (ComputeDetails, DatabaseDetails, CacheDetails, SavingsPlanDetails) into the key via a deterministic per-type encoder, and drop rec.Account from the key since no *FromArgs constructor in this package ever sets it (an always-empty component gave no real discrimination and misled readers of the key format). Also corrects the idempotencyKeyFor docstring, which claimed the key already distinguished materially different requests. Added regression tests proving two Savings Plans requests differing only in hourly_commitment, and two EC2 RI requests differing only in platform, now derive different tokens. Both fail on the pre-fix code (same token) and pass after this change.
No purchase tool's *FromArgs constructor populates Recommendation's OnDemandCost/CommitmentCost/EstimatedSavings/SavingsPercentage (they build a fresh Recommendation from the caller's typed args, not a priced search result), and some provider clients (AWS EC2 RIs, Savings Plans) never populate PurchaseResult.Cost either. Because PurchaseResponse used plain float64 fields without omitempty, every dry-run preview and most real purchases reported cost/on_demand_cost/estimated_savings/savings_percentage as a literal 0, indistinguishable from a genuinely free purchase. Change those four fields to *float64 with omitempty, and add nonZeroCostPtr so a value is only surfaced when it is actually known (a real purchase's result.Cost still passes through when the provider populates it). Update mcp/README.md's worked example and safety-model section, which claimed a dry-run preview echoes back "the cost/savings figures already known from the recommendation" -- it validates parameters and reports pricing only when genuinely known. Split the purchase_test.go fixture into testRecommendation() (mirrors what real tools actually build: no cost fields) and testRecommendationWithCost() (used only to prove pass-through when a value is genuinely present); the old single fixture hand-set cost fields no real tool produces, which masked this finding. Added TestExecutePurchasePreviewOmitsUnknownCostFields, which asserts the four fields are nil and absent from the marshaled JSON. Both new and updated assertions fail to even compile against the pre-fix float64 fields (the type itself could not represent "unknown"), and pass after this change.
azure_compute_ri.go validated payment_option against the shared AWS/Azure/ GCP enum and then silently dropped it: Azure's purchase API (buildReservationBody) has no billing-plan parameter and always bills upfront, so a caller requesting no-upfront or partial-upfront got a real purchase billed upfront anyway, under a payment schedule they never chose. Reject any payment_option other than all-upfront with an explicit error instead of silently mismatching, per the project's fail-loud convention. Update the tool description, field schema comment, and mcp/README.md's caveat section to describe the new rejection behavior instead of the old "affects the cost estimate but not the invoice" framing. validAzureComputeArgs() now uses all-upfront, the one value Azure actually honors. Added TestAzureComputeRecommendationFromArgsRejectsUnhonoredPaymentOption (no-upfront and partial-upfront both rejected) and TestAzureComputeRecommendationFromArgsAcceptsAllUpfront. The rejection test fails on the pre-fix code (no error returned) and passes after this change.
Azure's purchase API has no billing-plan parameter and always bills upfront, so a payment_option other than all-upfront can never be honored for a real purchase. The previous check rejected any other value unconditionally, which also blocked a dry_run=true preview from validating those parameters even though a preview never spends money. Reuse decidePurchaseMode inside azureComputeRecommendationFromArgs so the all-upfront-only rejection applies only when the call would actually execute (dry_run=false, confirm=true); a preview now accepts any valid payment_option, and a confirm-missing call still surfaces the shared confirm=true error from ExecutePurchase.
…ations common.RecommendationParams already supports IncludeRegions and ExcludeRegions, but cudly_search_recommendations neither accepted nor forwarded them, so an MCP caller could not restrict (or exclude) regions the way the CLI's config does. Add include_regions/exclude_regions array params to the tool schema and forward them into RecommendationParams.
AWS's Database Savings Plans support only a one-year term billed no-upfront (confirmed against AWS's Database Savings Plans announcement, aws.amazon.com/about-aws/whats-new/2025/12/ database-savings-plans-savings) -- unlike Compute, EC2Instance, and SageMaker plans, there is no 3-year term and no all-upfront/ partial-upfront option. Add validateDatabaseSPConstraints to reject a mismatched term_years or payment_option for sp_type=Database before building the recommendation, instead of letting AWS reject it at purchase time. Split the growing validation chain out of savingsPlanRecommendationFromArgs into validateSavingsPlanArgs to keep cyclomatic complexity under the repo's gocyclo gate.
t.spec.product ("opensearch", "redshift", "memorydb") was interpolated
raw into the human-readable tool description, producing "AWS
opensearch Reserved Instances" instead of "AWS OpenSearch Reserved
Instances".
Add a displayName field to simpleAWSRIPurchaseSpec for the description
text only; the lowercase product identifier used for the Descriptor
and API calls is unchanged.
`go build -o cudly-mcp` created an untracked root-level binary, which the repo's guidelines don't allow. Switch the install/launch instructions to `go install ./cmd/cudly-mcp` and running `cudly-mcp` from PATH.
The all-upfront payment gate in azureComputeRecommendationFromArgs discarded decidePurchaseMode's error, tripping errcheck in CI. Capture it and require it to be nil before evaluating the gate, extracted into azureRealPurchaseRequiresAllUpfront to keep the function's cyclomatic complexity under the pre-commit gocyclo threshold.
Add a short pointer section so the root README surfaces the MCP server alongside the CLI reference instead of leaving it undiscovered.
buildReservationBody never set properties.billingPlan, so every Azure VM reservation purchase defaulted to Azure's Upfront billing regardless of the requested payment option. Add BillingPlanForPaymentOption, mapping all-upfront/upfront to armreservations.ReservationBillingPlanUpfront and no-upfront/monthly to ReservationBillingPlanMonthly (Azure's only two billing plans; no premium for spreading payments). Empty or unrecognized values, including partial-upfront which Azure cannot express, are a hard error rather than a silent default.
Now that buildReservationBody honors billingPlan, drop the all-upfront-only rejection on cudly_azure_compute_ri_purchase: payment_option defaults to no-upfront (matching the CLI's --payment default) when omitted, and both all-upfront and no-upfront flow through to a real purchase. partial-upfront is still rejected, unconditionally, since Azure has no equivalent billing plan at any layer.
…port The Azure caveat still described the old all-upfront-only limitation; update it to note the two supported billing plans, the no-upfront default, and that partial-upfront remains unsupported.
providers/aws, providers/azure, and providers/gcp register their factory via init() in the package root, and cmd/main.go already blank-imports all three so the CLI binary picks them up. cmd/cudly-mcp did not import them and nothing else in the mcp package's import graph pulled them in either, so provider.CreateProvider always returned "provider not registered" and every real purchase failed at the ResolveClient step. Only dry-run previews worked. Add the same three blank imports cmd/main.go already carries, and add a regression test in package main (mcp/... tests cannot observe this bug since that test binary never pulls in cmd/cudly-mcp's import graph).
…rchases idempotencyKeyFor derived the token purely from purchase parameters (provider/region/service/resource/count/term/payment/details), so two distinct, intentional purchases with identical parameters (e.g. "buy 3 m5.large RIs now" and "buy 3 more next week") hashed to the same token. findRIByIdempotencyToken then treated the second purchase as a retry of the first and silently skipped it. Fold a discriminator into the key: an explicit caller-supplied idempotency_nonce when provided, otherwise an automatic hourly time bucket. A rapid same-bucket retry after a network timeout still dedupes as before; purchases separated by more than a bucket no longer collide. A caller wanting strict long-lived dedup can pass the same nonce on both calls. Add idempotency_nonce as an optional argument on every purchase tool and update the README's safety-model and troubleshooting sections.
sp_type=EC2Instance already required region but not instance_family. Without it, DescribeSavingsPlansOfferings has no instanceFamily filter and can resolve across every family in the region instead of the one Cost Explorer actually recommended, risking a real purchase for the wrong workload. The provider client's lookupEC2OfferingIDStrict fails loud when the resulting offerings span more than one family, but that is defense in depth at the API boundary; require the field at the tool boundary too, mirroring how region is already required for this sp_type. instance_family stays optional and ignored for Compute, SageMaker, and Database plans, which are family-agnostic and account-level.
NewServer captured the descriptors slice before appending the list tool to regs, so the catalog cudly_list_commitment_actions returns at runtime never included its own entry, despite its description claiming to list every tool on the server. Export ListCommitmentActionsDescriptor so its static entry can be appended to the descriptors slice before the tool itself is constructed, and add an end-to-end test that calls the tool through the real NewServer wiring and asserts its own name appears in the result.
A bare `== ""` check on region/instance_type/vm_size/machine_type/etc lets a whitespace-only value like " " through to provider resolution on a confirmed real purchase, since it is neither empty nor a value the enum validators would catch. Add a requireNonBlank helper that trims before checking, and use it at every plain (non-enum) required-string check across the AWS EC2, RDS, ElastiCache, and simple RI tools, the AWS Savings Plans EC2Instance region/instance_family checks, and the Azure and GCP purchase tools. Enum-typed fields (payment_option, engine, term, etc.) already reject a whitespace-only value via their own allow-list check and are unaffected. Add whitespace-only regression cases to each tool's existing missing-required-field table tests.
PurchaseResponse.TermYears was declared in the JSON contract but never set in either branch of ExecutePurchase, so it was always zero and omitted (omitempty) even though the term is known from the request: every *FromArgs constructor in this package writes it into Recommendation.Term in the "<N>yr" format via TermYears.RecommendationTerm(). Add termYearsFromRecommendationTerm to parse that format back into an int and populate TermYears in both the preview and real-purchase response construction.
The register-with-client instructions showed both `$(go env GOBIN)/cudly-mcp` and `$(go env GOPATH)/bin/cudly-mcp` as bare alternatives without saying which applies when. `$(go env GOBIN)` expands to an empty string when GOBIN is unset, so presenting it as a standalone path invites a reader to use an invalid `/cudly-mcp` location. Tie each path explicitly to its GOBIN state, matching the Install section above it.
Rename the shadowed err in the requireNonBlank checks added to the EC2, simple-RI, and Azure tools to fieldErr (govet shadow flagged the inner err colliding with the named return), and preallocate the names slice in the new list-commitment-actions catalog test (prealloc).
…ket) Commit 2390f07 folded an automatic hourly time bucket into the MCP purchase idempotency key whenever the caller omitted an explicit nonce, to stop two genuinely separate purchases with identical parameters from colliding on the same token. That inverted the safety direction of a money path: a retry that happened to straddle an hour boundary (e.g. issued at 12:59:58, retried four seconds later at 13:00:02) derived a different key, so the provider could treat the retry as a brand new purchase and double-buy instead of deduping it. idempotencyKeyFor no longer reads any clock. With no nonce (the default), identical purchase dimensions always derive the same key regardless of elapsed time, so a retry always dedupes; the worst case is a skipped intentional repeat, never a double purchase. A caller who genuinely wants a second, otherwise-identical purchase authorizes it explicitly by passing a fresh idempotency_nonce, which every purchase tool already exposed; the same nonce on a retry of that call still dedupes. Removed the now-unused idempotencyBucket constant and idempotencyClock seam, updated the idempotencyKeyFor and PurchaseRequest.Nonce doc comments and the README's safety-model/troubleshooting sections to describe the fail-safe model, and replaced the obsolete bucket-boundary tests with a regression test proving identical no-nonce dimensions always derive the same key plus a test proving a nonce authorizes a distinct, still-dedupable purchase.
Azure has no partial-upfront billing plan; the MCP tool already rejected it at runtime but the schema enum still advertised it as a valid choice, inviting a call the tool could only ever refuse. Restrict the Azure compute RI tool's payment_option enum to all-upfront/no-upfront and keep the runtime rejection as defense in depth. AWS tools keep their unrestricted enum since AWS does support partial-upfront. Adds an end-to-end MCP protocol test asserting the advertised schema excludes partial-upfront (confirmed failing before the fix).
…vel default The account-level Savings Plans fallback only triggered on region == "", so a whitespace-only region (e.g. " ") skipped the savingsPlansAccountLevelRegion default and threaded the raw whitespace into resolveClient instead. Match the EC2Instance branch two lines up, which already trims before checking for blank. Adds a regression test with region=" " for an account-level sp_type (Compute), confirmed failing before the fix.
Azure SQL Database reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/azure-sql/database/reservations-discount-overview). buildReservationBody never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502.
Azure Cache for Redis reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/azure-cache-for-redis/cache-reserved-pricing). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502.
Azure Cosmos DB reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/cosmos-db/reserved-capacity). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502.
The Microsoft.Capacity calculatePrice/purchase API uses the same properties struct for every reservedResourceType, and Azure's monthly- payments doc (learn.microsoft.com/azure/cost-management-billing/reservations/ prepare-buy-reservation) excludes only SUSE Linux, Red Hat plans, Azure Red Hat OpenShift, and pre-purchase plans from Monthly billing, so Search is covered the same as every other sibling service. PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. This is independent of the pre-existing "SearchService" reservedResourceType literal being unverified against the live catalog (issue #1189). Part of #1502.
Azure Managed Redis reservations support both Upfront and Monthly billing frequency (learn.microsoft.com/azure/redis/reserved-pricing). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502.
Azure Synapse Analytics Dedicated SQL pool (SQL DW) reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/ cost-management-billing/reservations/prepay-sql-data-warehouse-charges). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a local stdio MCP server exposing recommendation search and commitment-purchase tools across AWS, Azure, and GCP. It introduces shared validation, dry-run/confirmation gates, idempotency handling, tool discovery, documentation, and Azure billing-plan request mapping. ChangesMCP server and shared tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant CudlyMCPServer
participant PurchaseTool
participant Provider
MCPClient->>CudlyMCPServer: List or call MCP tool
CudlyMCPServer->>PurchaseTool: Validate arguments and safety flags
PurchaseTool->>PurchaseTool: Apply dry-run or confirmation gate
PurchaseTool->>Provider: Resolve client and submit confirmed purchase
Provider-->>PurchaseTool: Return purchase result
PurchaseTool-->>MCPClient: Return structured response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
mcp/tools/enums.go (1)
80-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale CLI source path in this comment.
The reference points to
cmd/main.go:112, but the supplied stack context identifies the entry point ascmd/cudly-mcp/main.go. Correct the path so maintainers can locate--include-sp-types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/enums.go` around lines 80 - 82, Update the comment above SPType to replace the stale cmd/main.go:112 reference with cmd/cudly-mcp/main.go, preserving the existing description of --include-sp-types.mcp/tools/azure_compute_ri.go (1)
116-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting logic.
Lines 157-163 duplicate the same
*bool → boolresolution pattern already extracted aseffectiveDryRunConfirminaws_ec2_ri.go. See consolidated comment. The PartialUpfront rejection and payment-option defaulting logic above it is correct and well-documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/azure_compute_ri.go` around lines 116 - 177, Replace the local dryRun and confirm pointer-resolution block in azureComputeRecommendationFromArgs with the existing effectiveDryRunConfirm helper used by aws_ec2_ri.go. Preserve the current defaults of true for dryRun and false for confirm, and leave the payment-option validation and recommendation construction unchanged.mcp/tools/gcp_computeengine_cud.go (1)
100-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting logic.
Lines 144-150 duplicate the same
*bool → boolresolution pattern already extracted aseffectiveDryRunConfirminaws_ec2_ri.go. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/gcp_computeengine_cud.go` around lines 100 - 152, Update gcpComputeEngineRecommendationFromArgs to reuse the existing effectiveDryRunConfirm helper for resolving args.DryRun and args.Confirm, removing the duplicated local defaulting logic while preserving the current default values and returned booleans.mcp/tools/aws_elasticache_ri.go (1)
99-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dry_run/confirm defaulting logic.
Lines 147-153 reimplement the exact
*bool → booldefault-resolution thataws_ec2_ri.go'seffectiveDryRunConfirmalready extracts. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_elasticache_ri.go` around lines 99 - 155, Remove the duplicated dryRun and confirm default-resolution in elasticacheRecommendationFromArgs and reuse the existing effectiveDryRunConfirm helper from aws_ec2_ri.go. Preserve the current defaults of dryRun=true and confirm=false when the corresponding pointers are nil.mcp/tools/aws_rds_ri.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared dry-run/confirm defaulting and client-resolution boilerplate. All three AWS RI/Savings-Plans tool files reimplement the identical "default
dryRun/confirmfrom*boolargs" logic and a near-identical "buildProviderConfig→createProvider→GetServiceClient"resolveClientclosure, with no shared helper.
mcp/tools/aws_rds_ri.go#L154-173: extract a sharedresolveDryRunConfirm(dryRun, confirm *bool) (bool, bool)helper for lines 154-160, and a shared client-resolution helper parameterized by provider name/profile/region/service for lines 164-173.mcp/tools/aws_savingsplans.go#L193-237: replace the duplicated defaulting block (193-199) andresolveClient(228-237) with the same shared helpers.mcp/tools/aws_simple_ri.go#L186-205: replace the duplicated defaulting block (186-192) andresolveClient(196-205) with the same shared helpers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/tools/aws_rds_ri.go` at line 1, Extract shared helpers for dry-run/confirm defaulting and AWS client resolution from the duplicated logic in the RDS RI, Savings Plans, and Simple RI tools. Add resolveDryRunConfirm(dryRun, confirm *bool) and a parameterized helper that builds ProviderConfig, calls createProvider, and resolves GetServiceClient using provider name, profile, region, and service; update each tool’s existing blocks to call these helpers while preserving current defaults and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/tools/aws_savingsplans.go`:
- Line 43: Gate the Details.Region and Details.InstanceFamily assignments in the
savings plan request construction around the spType value, populating them only
when spType is EC2Instance and leaving them empty for Compute, SageMaker, and
Database plans. Update the related account-level coverage, including
TestSavingsPlanRecommendationFromArgsAccountLevelWhitespaceRegion, to verify
supplied region and instance-family values do not leak into Details.
In `@mcp/tools/enums.go`:
- Around line 13-24: Update requireNonBlank to reject values with leading or
trailing whitespace, or return the trimmed value and update every caller to
forward that normalized value. Preserve the existing required error for blank
inputs, and add a test covering a padded value such as " us-east-1 ".
In `@mcp/tools/search_recommendations.go`:
- Around line 73-76: Align searchRecommendationsArgs schema and
validateSearchArgs: enforce the advertised lookback_period values in the
handler, and add matching enum overrides for term_years and payment_option using
the same allowed values that runtime validation accepts. Ensure schema
validation and boundary validation reject and accept the same inputs.
In `@providers/azure/services/compute/client.go`:
- Around line 419-422: Update PurchaseCommitment to call
reservations.BillingPlanForPaymentOption and retain the parsed billing plan
before ensureCapacityProviderRegistered, returning validation errors before any
Azure registration. Pass that validated billing plan into the request-body
construction path, and remove the later duplicate parsing in the relevant
helper.
---
Nitpick comments:
In `@mcp/tools/aws_elasticache_ri.go`:
- Around line 99-155: Remove the duplicated dryRun and confirm
default-resolution in elasticacheRecommendationFromArgs and reuse the existing
effectiveDryRunConfirm helper from aws_ec2_ri.go. Preserve the current defaults
of dryRun=true and confirm=false when the corresponding pointers are nil.
In `@mcp/tools/aws_rds_ri.go`:
- Line 1: Extract shared helpers for dry-run/confirm defaulting and AWS client
resolution from the duplicated logic in the RDS RI, Savings Plans, and Simple RI
tools. Add resolveDryRunConfirm(dryRun, confirm *bool) and a parameterized
helper that builds ProviderConfig, calls createProvider, and resolves
GetServiceClient using provider name, profile, region, and service; update each
tool’s existing blocks to call these helpers while preserving current defaults
and error behavior.
In `@mcp/tools/azure_compute_ri.go`:
- Around line 116-177: Replace the local dryRun and confirm pointer-resolution
block in azureComputeRecommendationFromArgs with the existing
effectiveDryRunConfirm helper used by aws_ec2_ri.go. Preserve the current
defaults of true for dryRun and false for confirm, and leave the payment-option
validation and recommendation construction unchanged.
In `@mcp/tools/enums.go`:
- Around line 80-82: Update the comment above SPType to replace the stale
cmd/main.go:112 reference with cmd/cudly-mcp/main.go, preserving the existing
description of --include-sp-types.
In `@mcp/tools/gcp_computeengine_cud.go`:
- Around line 100-152: Update gcpComputeEngineRecommendationFromArgs to reuse
the existing effectiveDryRunConfirm helper for resolving args.DryRun and
args.Confirm, removing the duplicated local defaulting logic while preserving
the current default values and returned booleans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4975ad8e-00d4-4e64-990e-10a8ad486fff
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (50)
README.mdcmd/cudly-mcp/main.gocmd/cudly-mcp/main_test.gogo.modmcp/README.mdmcp/server.gomcp/server_test.gomcp/tools/aws_ec2_ri.gomcp/tools/aws_ec2_ri_test.gomcp/tools/aws_elasticache_ri.gomcp/tools/aws_elasticache_ri_test.gomcp/tools/aws_rds_ri.gomcp/tools/aws_rds_ri_test.gomcp/tools/aws_savingsplans.gomcp/tools/aws_savingsplans_test.gomcp/tools/aws_simple_ri.gomcp/tools/aws_simple_ri_test.gomcp/tools/azure_compute_ri.gomcp/tools/azure_compute_ri_test.gomcp/tools/enums.gomcp/tools/enums_test.gomcp/tools/gcp_computeengine_cud.gomcp/tools/gcp_computeengine_cud_test.gomcp/tools/list_commitment_actions.gomcp/tools/list_commitment_actions_test.gomcp/tools/purchase.gomcp/tools/purchase_test.gomcp/tools/registry.gomcp/tools/schema.gomcp/tools/schema_test.gomcp/tools/search_recommendations.gomcp/tools/search_recommendations_test.gopkg/common/types.gopkg/common/types_test.goproviders/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/internal/reservations/purchase.goproviders/azure/services/internal/reservations/purchase_test.goproviders/azure/services/managedredis/client.goproviders/azure/services/managedredis/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
| TermYears int `json:"term_years" jsonschema:"commitment length in years"` | ||
| PaymentOption string `json:"payment_option" jsonschema:"payment schedule"` | ||
| InstanceFamily string `json:"instance_family,omitempty" jsonschema:"EC2 instance family, e.g. m5; only meaningful for sp_type=EC2Instance"` | ||
| Region string `json:"region,omitempty" jsonschema:"AWS region; required for sp_type=EC2Instance, ignored for account-level plan types"` |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
SavingsPlanDetails.Region/InstanceFamily leak for non-EC2Instance plan types.
Region (line 43) is documented as "ignored for account-level plan types," and common.SavingsPlanDetails documents Region/InstanceFamily as "Only populated for EC2Instance" plans — but the Details struct at lines 185-190 is built from raw args.Region/args.InstanceFamily unconditionally, not gated by spType. If a caller supplies a region (or instance_family) for sp_type=Compute/SageMaker/Database, it leaks into Details.Region, which the purchase client uses as an offering-region filter — misdirecting DescribeSavingsPlansOfferings for what is supposed to be a region-agnostic, account-level purchase. No existing test (TestSavingsPlanRecommendationFromArgsAccountLevelWhitespaceRegion) asserts Details.Region stays empty for account-level types, so this gap is currently unguarded.
🐛 Proposed fix: gate Details fields by spType
- rec = common.Recommendation{
- Provider: common.ProviderAWS,
- Service: service,
- Region: region,
- CommitmentType: common.CommitmentSavingsPlan,
- Term: term.RecommendationTerm(),
- PaymentOption: string(paymentOption),
- Details: &common.SavingsPlanDetails{
- PlanType: string(spType),
- HourlyCommitment: args.HourlyCommitment,
- InstanceFamily: args.InstanceFamily,
- Region: args.Region,
- },
- }
+ details := &common.SavingsPlanDetails{
+ PlanType: string(spType),
+ HourlyCommitment: args.HourlyCommitment,
+ }
+ if spType == SPTypeEC2Instance {
+ details.InstanceFamily = args.InstanceFamily
+ details.Region = args.Region
+ }
+
+ rec = common.Recommendation{
+ Provider: common.ProviderAWS,
+ Service: service,
+ Region: region,
+ CommitmentType: common.CommitmentSavingsPlan,
+ Term: term.RecommendationTerm(),
+ PaymentOption: string(paymentOption),
+ Details: details,
+ }Also applies to: 178-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/tools/aws_savingsplans.go` at line 43, Gate the Details.Region and
Details.InstanceFamily assignments in the savings plan request construction
around the spType value, populating them only when spType is EC2Instance and
leaving them empty for Compute, SageMaker, and Database plans. Update the
related account-level coverage, including
TestSavingsPlanRecommendationFromArgsAccountLevelWhitespaceRegion, to verify
supplied region and instance-family values do not leak into Details.
Comment on lines
+13
to
+24
| // requireNonBlank returns an explicit "<field> is required" error when val is | ||
| // empty or contains only whitespace. A whitespace-only value (e.g. " ") | ||
| // passes a bare `== ""` check but carries no real region/instance-type/etc | ||
| // information, and on a real-purchase path could still reach provider | ||
| // resolution instead of being rejected at the MCP tool boundary like an | ||
| // actually-empty value already is. | ||
| func requireNonBlank(field, val string) error { | ||
| if strings.TrimSpace(val) == "" { | ||
| return fmt.Errorf("%s is required", field) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject or normalize surrounding whitespace at the MCP boundary.
requireNonBlank rejects only all-whitespace values. Inputs such as " us-east-1 " pass, but the helper returns no normalized value, so callers forwarding the original argument can send invalid provider identifiers. Either reject leading/trailing whitespace here or return the trimmed value and require callers to use it; add a padded-input test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/tools/enums.go` around lines 13 - 24, Update requireNonBlank to reject
values with leading or trailing whitespace, or return the trimmed value and
update every caller to forward that normalized value. Preserve the existing
required error for blank inputs, and add a test covering a padded value such as
" us-east-1 ".
Source: Coding guidelines
Comment on lines
+73
to
+76
| schema, err := BuildInputSchema[searchRecommendationsArgs](map[string]FieldOverride{ | ||
| "provider": {Enum: []any{string(common.ProviderAWS), string(common.ProviderAzure), string(common.ProviderGCP)}}, | ||
| "lookback_period": {Enum: []any{"7d", "30d", "60d"}}, | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep schema and handler validation aligned.
lookback_period is constrained only in the advertised schema; validateSearchArgs forwards arbitrary values when a caller bypasses schema validation. Conversely, term_years and payment_option are rejected at runtime but are not advertised as enums. Validate the lookback value in the handler and add matching overrides for the other constrained fields.
Suggested fix
schema, err := BuildInputSchema[searchRecommendationsArgs](map[string]FieldOverride{
- "provider": {Enum: []any{string(common.ProviderAWS), string(common.ProviderAzure), string(common.ProviderGCP)}},
- "lookback_period": {Enum: []any{"7d", "30d", "60d"}},
+ "provider": {Enum: []any{string(common.ProviderAWS), string(common.ProviderAzure), string(common.ProviderGCP)}},
+ "lookback_period": {Enum: []any{"7d", "30d", "60d"}},
+ "term_years": {Enum: []any{int(TermOneYear), int(TermThreeYear)}},
+ "payment_option": {Enum: []any{string(PaymentOptionAllUpfront), string(PaymentOptionPartialUpfront), string(PaymentOptionNoUpfront)}},
}) func validateSearchArgs(args searchRecommendationsArgs) (common.ProviderType, string, error) {
providerType, err := validateProviderName(args.Provider)
if err != nil {
return "", "", err
}
+ if args.LookbackPeriod != "" &&
+ args.LookbackPeriod != "7d" &&
+ args.LookbackPeriod != "30d" &&
+ args.LookbackPeriod != "60d" {
+ return "", "", fmt.Errorf("invalid lookback_period %q: must be one of 7d, 30d, 60d", args.LookbackPeriod)
+ }
+As per coding guidelines, “validate user input at system boundaries.”
Also applies to: 122-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/tools/search_recommendations.go` around lines 73 - 76, Align
searchRecommendationsArgs schema and validateSearchArgs: enforce the advertised
lookback_period values in the handler, and add matching enum overrides for
term_years and payment_option using the same allowed values that runtime
validation accepts. Ensure schema validation and boundary validation reject and
accept the same inputs.
Source: Coding guidelines
Comment on lines
+419
to
+422
| billingPlan, err := reservations.BillingPlanForPaymentOption(rec.PaymentOption) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the payment option before registering Microsoft.Capacity.
PurchaseCommitment calls ensureCapacityProviderRegistered before this helper runs, so an invalid or empty PaymentOption can still issue an Azure provider-registration operation before rejection. Validate and retain the parsed billing plan in PurchaseCommitment before that call, then pass it into body construction.
As per coding guidelines, “validate user input at system boundaries.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/azure/services/compute/client.go` around lines 419 - 422, Update
PurchaseCommitment to call reservations.BillingPlanForPaymentOption and retain
the parsed billing plan before ensureCapacityProviderRegistered, returning
validation errors before any Azure registration. Pass that validated billing
plan into the request-body construction path, and remove the later duplicate
parsing in the relevant helper.
Source: Coding guidelines
Member
Author
This was referenced Jul 25, 2026
Closed
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
Wires the
billingPlanproperty into the 6 Azure sibling reservation service clients that still omitted it:database(SQL Database),cache(Cache for Redis),cosmosdb(Cosmos DB),search(AI Search),managedredis(Managed Redis), andsynapse(SQL Data Warehouse). Mirrors what #1495 did forcompute(VM reservations): before this fix, ano-upfront/monthlyrecommendation for any of these 6 services was silently billed at Azure'sUpfrontdefault becauseproperties.billingPlanwas never set on the purchase body.STACKED ON #1495 — this branch is based on
docs/mcp-server-design(head6aabc822d) so the sharedreservations.BillingPlanForPaymentOptionhelper is available. Needs a rebase ontomainafter #1495 merges.Closes #1502.
Per-service billing-plan support
All 6 services go through the same generic
Microsoft.CapacitycalculatePrice/purchaseAPI (sameproperties.billingPlanschema for everyreservedResourceType). Azure's monthly-payments doc (learn.microsoft.com/azure/cost-management-billing/reservations/prepare-buy-reservation) gives an explicit exclusion list for Monthly billing — SUSE Linux, Red Hat plans, Azure Red Hat OpenShift Licenses, and pre-purchase plans — and none of the 6 sibling services are on it. Verified per-service against Microsoft Learn docs:* AI Search:
billingPlanitself is generic to the Microsoft.Capacity API schema (not resource-type-specific), so it is wired the same as the other 5. This is independent of the pre-existing"SearchService"reservedResourceTypestring literal already flagged as unverified against the live reservation catalog in #1189 — that is a different property and out of scope here.None of the 6 services needed a "reject monthly" fail-loud path; all support both
UpfrontandMonthly.Changes
providers/azure/services/internal/reservations/purchase.go'sBillingPlanForPaymentOption(added in feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP #1495) is reused as-is — no duplication.client.gofiles: computebillingPlanfromrec.PaymentOptionright after the existing term computation, and setproperties["billingPlan"] = string(billingPlan)in the purchase request body, matching compute's key ordering and shape exactly. An empty/unrecognized/partial-upfront payment option now fails loud with a clear error instead of silently defaulting to Azure'sUpfrontbehavior.client_test.gofiles: a newTestBillingPlan-style table test (per file's existing naming convention) asserting the captured request body carriesbillingPlan: Upfrontfor an all-upfront rec andbillingPlan: Monthlyfor a no-upfront rec, and that partial-upfront/empty payment options are rejected with a clear error before any HTTP call is made.PurchaseCommitmenttests in all 6 files to populatePaymentOptionon theirRecommendationfixtures (previously omitted since the field was unused) — mirrors what feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP #1495 already required forcompute's own pre-existing tests. Production recommendations always carryPaymentOption(set byExpandPaymentVariantsin the converter), so this only affects test fixtures, not runtime behavior.Verification
go build ./...— cleango vet ./...— cleangolangci-lintv2.10.1 (pinned to match CI'sci.yml, not the local-default v2.11.4) run at repo root — 0 issuesgocyclo -over 10 providers/azure/services/— cleango test ./...— full suite green (6143 tests / 41 packages)compute— all greenTest plan
mainafter feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP #1495 mergesSummary by CodeRabbit
New Features
cudly-mcpserver for MCP-compatible clients using standard input/output.Bug Fixes
Documentation