fix(mcp): default Savings Plans search to 1yr no-upfront 30d + region post-filter (#1506) - #1507
Closed
cristim wants to merge 48 commits into
Closed
fix(mcp): default Savings Plans search to 1yr no-upfront 30d + region post-filter (#1506)#1507cristim wants to merge 48 commits into
cristim wants to merge 48 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.
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.
…tance common.SavingsPlanDetails documents Region and InstanceFamily as only populated for EC2Instance plans, but savingsPlanRecommendationFromArgs set both unconditionally from caller args. A Compute/SageMaker/Database (account -level, family-agnostic) request with a caller-supplied region or instance_family leaked that value into Details, violating the documented invariant that other Details consumers rely on. Populate InstanceFamily and Region only when sp_type=EC2Instance, matching the validation that already requires both fields for that type.
…ation PurchaseCommitment called ensureCapacityProviderRegistered (a real ARM resource-provider GET, and a POST to register when unregistered) before buildReservationBody validated the payment option via BillingPlanForPaymentOption. An invalid or empty PaymentOption still triggered that side-effecting call before being rejected. Resolve the billing plan via BillingPlanForPaymentOption first and thread the parsed value into buildReservationBody, so an invalid payment option fails loud before any side effect and the plan is no longer parsed twice.
requireNonBlank rejected an all-whitespace value but let a value with surrounding whitespace (e.g. " us-east-1 ") pass through unchanged into rec.Region/rec.ResourceType/Details and, for region, into resolveClient's ProviderConfig.Region and GetServiceClient call. requireNonBlank now returns the trimmed form on success. Every purchase tool (EC2, RDS, ElastiCache, the OpenSearch/Redshift/MemoryDB group, Azure VM, GCP Compute Engine) stores and threads the trimmed value through rec.Region/rec.ResourceType/Details and, for region, returns it from the recommendation-building function so resolveClient never resolves the provider/service client against a raw, un-trimmed value.
lookback_period was constrained only by the tool's advertised jsonschema enum, not re-validated in the handler, so a direct MCP call bypassing schema enforcement could pass an unsupported value through to the provider. Add the typed LookbackPeriod enum and validate against it in validateSearchArgs, consistent with how payment_option and the other enumerable fields are already re-validated at the handler boundary.
buildReservationBody parses rec.Term via ParseTermYears after ensureCapacityProviderRegistered already ran, so an invalid term still triggered the Microsoft.Capacity provider-registration side effect before the purchase was rejected. Move body construction (which validates both payment option and term) ahead of registration in PurchaseCommitment.
…tion validateSavingsPlanArgs only trimmed args.Region and args.InstanceFamily for the blank-check; savingsPlanRecommendationFromArgs then stored the raw, untrimmed values into the resolved region, rec.Region, and Details.Region/InstanceFamily. Those flow into ProviderConfig, GetServiceClient, and the DescribeSavingsPlansOfferings lookup for a real EC2Instance Savings Plan purchase, so " us-east-1 " passed validation but could reach a real purchase with surrounding whitespace intact.
region, include_regions, exclude_regions, and account_filter were passed straight through to ProviderConfig.Region and RecommendationParams with no trim, unlike the purchase tools' requireNonBlank. A caller-supplied " us-east-1 " could resolve the wrong (or account-default) region and silently miss matching recommendations. region is optional here, so trim rather than reject surrounding whitespace.
AWS's GetSavingsPlansPurchaseRecommendation requires term, payment option, and lookback period on every call, unlike GetReservationPurchaseRecommendation (EC2/RDS/etc), which defaults them server-side when omitted. cudly_search_recommendations advertised these three fields as optional for every service, so an AWS Savings Plans search omitting them failed against Cost Explorer one field at a time. When the search targets an AWS Savings Plans service and the caller left a field blank, default payment_option to no-upfront, term_years to 1, and lookback_period to 30d before validating and building the provider call. A caller-supplied value is never overridden, and EC2/RDS/etc searches are unaffected. Update the jsonschema descriptions to state the per-service default/omit behavior instead of a blanket "omit to search all".
validateSearchArgs returned on the first invalid field it found, so a caller with several bad fields had to fix and resubmit repeatedly to discover the next one. Collect every payment_option/lookback_period/ term_years/sp_type validation failure and errors.Join them into a single returned error instead. Also add requireSavingsPlansSearchFields as a safety net: after the Savings Plans defaults from the previous commit run, an AWS SP search should never still be missing payment_option/term_years/ lookback_period. If it somehow is, name every missing field in one error rather than letting the request reach Cost Explorer and fail one field at a time.
Note in the known-gaps section that cudly_search_recommendations defaults term_years/payment_option/lookback_period to 1yr/no-upfront/ 30d for AWS Savings Plans searches, and why reservation searches (EC2/RDS/etc) are unaffected.
GetReservationPurchaseRecommendation and GetSavingsPlansPurchaseRecommendation are both account-level Cost Explorer calls with no region parameter -- AWS returns recommendations from every region the account has usage in regardless of what the caller asked for. applyRecommendationFilters already honored include_regions/exclude_regions, but a caller passing only region (as cudly_search_recommendations documents doing) got no region filtering at all, so a us-east-1 search could surface an eu-west-1 recommendation. Fold Region into the include-region set before filtering, matching the existing "in addition to (or instead of) region" semantics include_regions already advertises. No behavior change when no region constraint is supplied.
Contributor
📝 WalkthroughWalkthroughAdds a stdio MCP server exposing multi-cloud recommendation search, commitment purchase tools, catalog discovery, dry-run/confirmation safety gates, validation, idempotency, provider wiring, documentation, and integration tests. ChangesMCP server and commitment operations
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
mcp/tools/aws_ec2_ri.go (2)
145-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGeneralize
effectiveDryRunConfirmfor reuse across purchase tools.This helper is tied to
ec2RIPurchaseArgs, soazure_compute_ri.goandgcp_computeengine_cud.goreimplement the identical*bool→(bool, bool)defaulting logic inline instead of reusing it. Changing the signature to takedryRun, confirm *booldirectly would let all purchase tools share one implementation.🤖 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_ec2_ri.go` around lines 145 - 158, Generalize effectiveDryRunConfirm to accept dryRun and confirm *bool parameters instead of ec2RIPurchaseArgs, while preserving the true default for omitted dryRun and false default for omitted confirm. Update its EC2 caller and replace the equivalent inline defaulting logic in the Azure and GCP purchase tools with this shared helper.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared dry_run/confirm defaulting logic is duplicated across purchase tools. All three sites independently implement "dry_run defaults to true, confirm defaults to false when the caller omits the pointer" against different arg structs in the same
toolspackage; a single shared helper takingdryRun, confirm *boolwould remove the duplication.
mcp/tools/aws_ec2_ri.go#L145-158: generalizeeffectiveDryRunConfirmto acceptdryRun, confirm *booldirectly (instead of the wholeec2RIPurchaseArgs) so it can be reused package-wide.mcp/tools/azure_compute_ri.go#L159-165: replace the inline defaulting block with a call to the generalized shared helper.mcp/tools/gcp_computeengine_cud.go#L140-146: replace the inline defaulting block with a call to the generalized shared helper.🤖 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_ec2_ri.go` at line 1, The dry_run/confirm defaulting logic is duplicated across purchase tools. Generalize effectiveDryRunConfirm to accept dryRun and confirm *bool directly, then replace the inline defaulting blocks in the Azure and GCP purchase flows with calls to this shared helper, preserving defaults of dry_run=true and confirm=false.mcp/tools/gcp_computeengine_cud.go (1)
140-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate dry_run/confirm defaulting logic.
Same pattern flagged in
aws_ec2_ri.goandazure_compute_ri.go.🤖 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 140 - 146, Remove the duplicated dryRun and confirm defaulting pattern from the affected handler and reuse the existing shared/defaulting mechanism used by aws_ec2_ri.go and azure_compute_ri.go. Preserve the defaults of dryRun=true and confirm=false, while allowing non-nil args.DryRun and args.Confirm values to override them.mcp/tools/azure_compute_ri.go (1)
159-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate dry_run/confirm defaulting logic.
Same pattern as
effectiveDryRunConfirminaws_ec2_ri.go, reimplemented inline here instead of shared.🤖 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 159 - 165, Replace the inline dryRun and confirm defaulting in the Azure compute reserved-instance flow with the shared effectiveDryRunConfirm helper used by aws_ec2_ri.go. Reuse its returned values and remove the duplicated nil-check logic while preserving the current defaults and argument overrides.providers/azure/services/compute/client.go (1)
410-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit
providers/azure/services/compute/client.gointo smaller files; it’s 945 lines and exceeds the 500-line guideline.🤖 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 410 - 417, Split providers/azure/services/compute/client.go into cohesive smaller files, keeping ComputeClient methods and related reservation-building logic grouped by responsibility. Preserve all existing behavior, imports, package declarations, and symbol visibility while ensuring the original file is reduced below the 500-line guideline.Source: Coding guidelines
🤖 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/server_test.go`:
- Around line 44-61: Separate the two Go doc comments in mcp/server_test.go and
place the money-impact/dry-run paragraph immediately above
TestRealPurchaseToolsDocumentMoneyImpactAndDryRun. Keep the end-to-end protocol
description directly above TestEndToEndSearchThenDryRunPurchase, with a blank
line between the comments so each function receives only its intended
documentation.
In `@mcp/tools/search_recommendations_test.go`:
- Around line 146-162: The test around searchRecommendationsArgs must verify
that the singular Region is merged into IncludeRegions, not merely trimmed. Use
distinct Region and IncludeRegions values, then assert the resulting collection
contains both values without depending on ordering, while preserving the
existing trimming checks for the other fields.
---
Nitpick comments:
In `@mcp/tools/aws_ec2_ri.go`:
- Around line 145-158: Generalize effectiveDryRunConfirm to accept dryRun and
confirm *bool parameters instead of ec2RIPurchaseArgs, while preserving the true
default for omitted dryRun and false default for omitted confirm. Update its EC2
caller and replace the equivalent inline defaulting logic in the Azure and GCP
purchase tools with this shared helper.
- Line 1: The dry_run/confirm defaulting logic is duplicated across purchase
tools. Generalize effectiveDryRunConfirm to accept dryRun and confirm *bool
directly, then replace the inline defaulting blocks in the Azure and GCP
purchase flows with calls to this shared helper, preserving defaults of
dry_run=true and confirm=false.
In `@mcp/tools/azure_compute_ri.go`:
- Around line 159-165: Replace the inline dryRun and confirm defaulting in the
Azure compute reserved-instance flow with the shared effectiveDryRunConfirm
helper used by aws_ec2_ri.go. Reuse its returned values and remove the
duplicated nil-check logic while preserving the current defaults and argument
overrides.
In `@mcp/tools/gcp_computeengine_cud.go`:
- Around line 140-146: Remove the duplicated dryRun and confirm defaulting
pattern from the affected handler and reuse the existing shared/defaulting
mechanism used by aws_ec2_ri.go and azure_compute_ri.go. Preserve the defaults
of dryRun=true and confirm=false, while allowing non-nil args.DryRun and
args.Confirm values to override them.
In `@providers/azure/services/compute/client.go`:
- Around line 410-417: Split providers/azure/services/compute/client.go into
cohesive smaller files, keeping ComputeClient methods and related
reservation-building logic grouped by responsibility. Preserve all existing
behavior, imports, package declarations, and symbol visibility while ensuring
the original file is reduced below the 500-line guideline.
🪄 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: d0840207-0531-4b12-a718-c3a49ab2f43e
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
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/aws/service_client.goproviders/aws/service_client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/internal/reservations/purchase.goproviders/azure/services/internal/reservations/purchase_test.go
Comment on lines
+44
to
+61
| // TestRealPurchaseToolsDocumentMoneyImpactAndDryRun proves every tool that | ||
| // can execute a real purchase leads its description with a money-impact | ||
| // statement and a dry-run recommendation (design doc §3/§5) -- so any future | ||
| // purchase tool that forgets one fails this test rather than shipping a | ||
| // tool description that quietly omits the safety framing every other | ||
| // purchase tool carries. | ||
| // TestEndToEndSearchThenDryRunPurchase drives the real MCP protocol path | ||
| // end to end -- a real Client connected to the real NewServer over an | ||
| // in-memory transport, not a bare Go function call -- through the exact | ||
| // chain the README's worked example describes: list the catalog, then call | ||
| // cudly_aws_ec2_ri_purchase with dry_run=true. It proves the tool schema | ||
| // registered without error (AddTool's schema inference/validation runs at | ||
| // connect time) and that a dry-run purchase returns structured cost JSON | ||
| // without any AWS credentials configured in this test environment -- | ||
| // confirming dry_run=true never reaches the real purchase path even through | ||
| // the full protocol stack, not just the Go-level unit tests in | ||
| // mcp/tools/aws_ec2_ri_test.go. | ||
| func TestEndToEndSearchThenDryRunPurchase(t *testing.T) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment merged onto the wrong function.
Lines 44-49 (about TestRealPurchaseToolsDocumentMoneyImpactAndDryRun) have no blank line separating them from lines 50-60 (about TestEndToEndSearchThenDryRunPurchase), so the whole block attaches to TestEndToEndSearchThenDryRunPurchase at Line 61. TestRealPurchaseToolsDocumentMoneyImpactAndDryRun at Line 158 ends up with no doc comment at all.
📝 Proposed fix: move the misplaced paragraph to its function
-// TestRealPurchaseToolsDocumentMoneyImpactAndDryRun proves every tool that
-// can execute a real purchase leads its description with a money-impact
-// statement and a dry-run recommendation (design doc §3/§5) -- so any future
-// purchase tool that forgets one fails this test rather than shipping a
-// tool description that quietly omits the safety framing every other
-// purchase tool carries.
// TestEndToEndSearchThenDryRunPurchase drives the real MCP protocol path
// end to end -- a real Client connected to the real NewServer over an
// in-memory transport, not a bare Go function call -- through the exact
// chain the README's worked example describes: list the catalog, then call
// cudly_aws_ec2_ri_purchase with dry_run=true. It proves the tool schema
// registered without error (AddTool's schema inference/validation runs at
// connect time) and that a dry-run purchase returns structured cost JSON
// without any AWS credentials configured in this test environment --
// confirming dry_run=true never reaches the real purchase path even through
// the full protocol stack, not just the Go-level unit tests in
// mcp/tools/aws_ec2_ri_test.go.
func TestEndToEndSearchThenDryRunPurchase(t *testing.T) {+// TestRealPurchaseToolsDocumentMoneyImpactAndDryRun proves every tool that
+// can execute a real purchase leads its description with a money-impact
+// statement and a dry-run recommendation (design doc §3/§5) -- so any future
+// purchase tool that forgets one fails this test rather than shipping a
+// tool description that quietly omits the safety framing every other
+// purchase tool carries.
func TestRealPurchaseToolsDocumentMoneyImpactAndDryRun(t *testing.T) {Also applies to: 158-158
🤖 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/server_test.go` around lines 44 - 61, Separate the two Go doc comments in
mcp/server_test.go and place the money-impact/dry-run paragraph immediately
above TestRealPurchaseToolsDocumentMoneyImpactAndDryRun. Keep the end-to-end
protocol description directly above TestEndToEndSearchThenDryRunPurchase, with a
blank line between the comments so each function receives only its intended
documentation.
Comment on lines
+146
to
+162
| _, _, err := tool.handle(context.Background(), nil, searchRecommendationsArgs{ | ||
| Provider: "aws", | ||
| Service: "ec2", | ||
| Region: " us-east-1 ", | ||
| IncludeRegions: []string{" us-east-1 ", " us-west-2 "}, | ||
| ExcludeRegions: []string{" eu-west-1 "}, | ||
| AccountFilter: []string{" 123456789012 "}, | ||
| }) | ||
|
|
||
| require.NoError(t, err) | ||
| require.NotNil(t, gotCfg) | ||
| assert.Equal(t, "us-east-1", gotCfg.Region, "ProviderConfig.Region must be trimmed") | ||
| require.NotNil(t, client.lastParams) | ||
| assert.Equal(t, "us-east-1", client.lastParams.Region, "RecommendationParams.Region must be trimmed") | ||
| assert.Equal(t, []string{"us-east-1", "us-west-2"}, client.lastParams.IncludeRegions, "IncludeRegions entries must be trimmed") | ||
| assert.Equal(t, []string{"eu-west-1"}, client.lastParams.ExcludeRegions, "ExcludeRegions entries must be trimmed") | ||
| assert.Equal(t, []string{"123456789012"}, client.lastParams.AccountFilter, "AccountFilter entries must be trimmed") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover merging a singular region into include_regions.
Region is already present in IncludeRegions, so this passes even if the handler only trims fields and never performs the required merge. Use disjoint values and an order-independent assertion.
Proposed test adjustment
- IncludeRegions: []string{" us-east-1 ", " us-west-2 "},
+ IncludeRegions: []string{" us-west-2 "},
...
- assert.Equal(t, []string{"us-east-1", "us-west-2"}, client.lastParams.IncludeRegions, "IncludeRegions entries must be trimmed")
+ assert.ElementsMatch(t, []string{"us-east-1", "us-west-2"}, client.lastParams.IncludeRegions, "Region must be merged with trimmed IncludeRegions entries")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _, _, err := tool.handle(context.Background(), nil, searchRecommendationsArgs{ | |
| Provider: "aws", | |
| Service: "ec2", | |
| Region: " us-east-1 ", | |
| IncludeRegions: []string{" us-east-1 ", " us-west-2 "}, | |
| ExcludeRegions: []string{" eu-west-1 "}, | |
| AccountFilter: []string{" 123456789012 "}, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, gotCfg) | |
| assert.Equal(t, "us-east-1", gotCfg.Region, "ProviderConfig.Region must be trimmed") | |
| require.NotNil(t, client.lastParams) | |
| assert.Equal(t, "us-east-1", client.lastParams.Region, "RecommendationParams.Region must be trimmed") | |
| assert.Equal(t, []string{"us-east-1", "us-west-2"}, client.lastParams.IncludeRegions, "IncludeRegions entries must be trimmed") | |
| assert.Equal(t, []string{"eu-west-1"}, client.lastParams.ExcludeRegions, "ExcludeRegions entries must be trimmed") | |
| assert.Equal(t, []string{"123456789012"}, client.lastParams.AccountFilter, "AccountFilter entries must be trimmed") | |
| _, _, err := tool.handle(context.Background(), nil, searchRecommendationsArgs{ | |
| Provider: "aws", | |
| Service: "ec2", | |
| Region: " us-east-1 ", | |
| IncludeRegions: []string{" us-west-2 "}, | |
| ExcludeRegions: []string{" eu-west-1 "}, | |
| AccountFilter: []string{" 123456789012 "}, | |
| }) | |
| require.NoError(t, err) | |
| require.NotNil(t, gotCfg) | |
| assert.Equal(t, "us-east-1", gotCfg.Region, "ProviderConfig.Region must be trimmed") | |
| require.NotNil(t, client.lastParams) | |
| assert.Equal(t, "us-east-1", client.lastParams.Region, "RecommendationParams.Region must be trimmed") | |
| assert.ElementsMatch(t, []string{"us-east-1", "us-west-2"}, client.lastParams.IncludeRegions, "Region must be merged with trimmed IncludeRegions entries") | |
| assert.Equal(t, []string{"eu-west-1"}, client.lastParams.ExcludeRegions, "ExcludeRegions entries must be trimmed") | |
| assert.Equal(t, []string{"123456789012"}, client.lastParams.AccountFilter, "AccountFilter entries must be trimmed") |
🤖 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_test.go` around lines 146 - 162, The test
around searchRecommendationsArgs must verify that the singular Region is merged
into IncludeRegions, not merely trimmed. Use distinct Region and IncludeRegions
values, then assert the resulting collection contains both values without
depending on ordering, while preserving the existing trimming checks for the
other fields.
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
cudly_search_recommendationsadvertisedterm_years/payment_option/lookback_periodas optional for every service ("omit to search all"). That's true for AWS EC2/RDS reservation searches (GetReservationPurchaseRecommendationdefaults them server-side) but false for AWS Savings Plans:GetSavingsPlansPurchaseRecommendationrejects the call unless all three are set, so an SP search omitting them failed against Cost Explorer one field at a time.Three changes, one per commit:
cb4236a7e) -- when the search targets an AWS Savings Plans service and a field was omitted, defaultpayment_option=no-upfront,term_years=1(1yr),lookback_period=30dbefore validating. A caller-supplied value always wins. EC2/RDS/etc searches are unaffected. Updated the jsonschema descriptions to state the per-service default/omit behavior.db8f3f7eb) --validateSearchArgsused to return on the first bad field; now every invalid/missing field is collected anderrors.Joined into a single error. AddedrequireSavingsPlansSearchFieldsas a safety net so a future change to the defaulting logic fails loud with one combined error instead of the one-field-at-a-time AWS cascade this PR fixes.5e79f0fd1) -- investigated whetherregion/include_regions/exclude_regionsare honored. Foundinclude_regions/exclude_regionsalready are (viaapplyRecommendationFiltersinproviders/aws/service_client.go), but the singularregionfield was not:GetReservationPurchaseRecommendation/GetSavingsPlansPurchaseRecommendationare account-level Cost Explorer calls with no region parameter, so aregion="us-east-1"search alone got no filtering and could surface recommendations from any region the account has usage in. Foldedregioninto the include-region set so it filters the same wayinclude_regionsalready does (additive, matching the field's existing "in addition to (or instead of) region" doc text). No-op when no region constraint is supplied.Also updated
mcp/README.md's "Caveats and known gaps" section with both behaviors.Stacked PR note
This branch is based on
docs/mcp-server-design(PR #1495, MCP server foundation) rather thanmain, since the MCP tool code doesn't exist onmainyet. Needs a rebase ontomainonce #1495 merges.Test plan
go build ./...cleango vet ./...cleango test ./mcp/...andgo test ./providers/aws/...green (new tests added for all three changes; each confirmed to fail against pre-change code and pass post-change)gocyclo -over 10 mcp/and the touchedproviders/aws/service_client.gocleangolangci-lintv2.10.1 (CI-pinned version, not local v2.11.4): 0 issues, exit 0Closes #1506
Summary by CodeRabbit
New Features
Bug Fixes
Documentation