feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP - #1495
feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP#1495cristim wants to merge 83 commits into
Conversation
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a local stdio MCP server exposing cross-cloud recommendation search, commitment purchase tools, safety-gated execution, action discovery, provider integrations, Azure billing-plan mapping, tests, and setup documentation. ChangesMCP commitment management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPServer
participant ToolHandler
participant ProviderClient
MCPClient->>MCPServer: Invoke search or purchase tool
MCPServer->>ToolHandler: Dispatch registered tool
ToolHandler->>ToolHandler: Validate arguments and safety flags
alt Dry-run or unconfirmed
ToolHandler-->>MCPClient: Return preview or refusal
else Confirmed real purchase
ToolHandler->>ProviderClient: Resolve credentials and client
ToolHandler->>ProviderClient: Purchase commitment with idempotency token
ProviderClient-->>ToolHandler: Return purchase result or error
ToolHandler-->>MCPClient: Return structured response
end
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
8b88ce3 to
6581b27
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
mcp/tools/azure_compute_ri.go (1)
135-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared dry_run/confirm resolution helper. Both files reimplement the identical
dryRun, confirm = true, false; if args.DryRun != nil {...}; if args.Confirm != nil {...}block for resolving optional*boolargs with defaults.
mcp/tools/azure_compute_ri.go#L135-L141: replace with a call to a shared helper, e.g.dryRun, confirm = resolveDryRunConfirm(args.DryRun, args.Confirm).mcp/tools/gcp_computeengine_cud.go#L142-L148: replace with the same shared helper call.♻️ Proposed shared helper
// in a shared tools file, e.g. registry.go func resolveDryRunConfirm(dryRun, confirm *bool) (bool, bool) { d, c := true, false if dryRun != nil { d = *dryRun } if confirm != nil { c = *confirm } return d, c }🤖 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 135 - 141, The dryRun/confirm default resolution is duplicated across both tool implementations. Add a shared resolveDryRunConfirm helper in the tools package that preserves defaults of true and false while handling optional pointers, then replace the inline blocks in mcp/tools/azure_compute_ri.go lines 135-141 and mcp/tools/gcp_computeengine_cud.go lines 142-148 with calls to it.mcp/tools/aws_ec2_ri.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDry_run/confirm default resolution is duplicated 5 times. Each purchase tool independently re-implements "dry_run defaults to true, confirm defaults to false, resolved from
*boolpointers" instead of sharing one generic helper — the root cause is thateffectiveDryRunConfirminmcp/tools/aws_ec2_ri.gois typed toec2RIPurchaseArgsand can't be reused elsewhere.
mcp/tools/aws_ec2_ri.go#L143-156: generalizeeffectiveDryRunConfirmto take(dryRun, confirm *bool) (bool, bool)and move it to a shared file (e.g.purchase.go) so all tools can call it.mcp/tools/aws_elasticache_ri.go#L145-151: replace the inline block with a call to the shared helper.mcp/tools/aws_rds_ri.go#L152-158: replace the inline block with a call to the shared helper.mcp/tools/aws_savingsplans.go#L162-168: replace the inline block with a call to the shared helper.mcp/tools/aws_simple_ri.go#L180-186: replace the inline block with a call to the 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, Generalize effectiveDryRunConfirm to accept dryRun and confirm *bool, returning resolved defaults of true and false, and move it into a shared purchase helper file. Replace the duplicated resolution blocks in the ElastiCache, RDS, Savings Plans, and Simple RI purchase tools with calls to this shared helper, while preserving their existing 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/README.md`:
- Around line 9-13: Update the build and launch instructions in the MCP README
to use go install ./cmd/cudly-mcp instead of go build -o cudly-mcp, then
instruct users to launch cudly-mcp from PATH; apply the same change to the
referenced duplicate section.
In `@mcp/tools/aws_savingsplans.go`:
- Around line 70-79: The Register method currently permits unsupported term and
payment combinations for SPTypeDatabase; add a Database-specific validation
branch before building the recommendation that accepts only TermOneYear and
PaymentOptionNoUpfront, rejecting 3-year or upfront payment options while
preserving existing validation for other savings plan types.
In `@mcp/tools/aws_simple_ri.go`:
- Around line 91-105: Add a human-readable display-name field to the RI tool
specification and use it in simpleAWSRIPurchaseTool.Descriptor when formatting
the purchase description, while retaining t.spec.product for machine-readable
metadata such as Product. Populate display names with the correctly cased AWS
product names.
In `@mcp/tools/azure_compute_ri.go`:
- Around line 15-27: Update the Azure compute RI purchase flow using
azureComputeRIPurchaseDescription so non-default payment_option values are
rejected when dry_run=false and confirm=true. Preserve those options for dry-run
validation, and retain the existing behavior for the default upfront option
until the purchase body supports billingPlanType.
In `@mcp/tools/search_recommendations.go`:
- Around line 25-38: Add IncludeRegions and ExcludeRegions fields to
searchRecommendationsArgs with appropriate JSON/schema metadata, then update the
search recommendations handler to forward both values into
common.RecommendationParams. Preserve the existing behavior when either filter
is omitted.
---
Nitpick comments:
In `@mcp/tools/aws_ec2_ri.go`:
- Line 1: Generalize effectiveDryRunConfirm to accept dryRun and confirm *bool,
returning resolved defaults of true and false, and move it into a shared
purchase helper file. Replace the duplicated resolution blocks in the
ElastiCache, RDS, Savings Plans, and Simple RI purchase tools with calls to this
shared helper, while preserving their existing behavior.
In `@mcp/tools/azure_compute_ri.go`:
- Around line 135-141: The dryRun/confirm default resolution is duplicated
across both tool implementations. Add a shared resolveDryRunConfirm helper in
the tools package that preserves defaults of true and false while handling
optional pointers, then replace the inline blocks in
mcp/tools/azure_compute_ri.go lines 135-141 and
mcp/tools/gcp_computeengine_cud.go lines 142-148 with calls to it.
🪄 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: 13bba9cf-49e2-462b-a91a-bc9f2d0dcc55
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
cmd/cudly-mcp/main.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.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 23 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
mcp/tools/aws_ec2_ri.go (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared dry_run/confirm defaulting logic instead of duplicating it five times. The same
dryRun, confirm = true, false; if args.DryRun != nil {...}; if args.Confirm != nil {...}pattern is re-implemented identically in every AWS purchase tool file. Since this logic gates real-money purchases, keeping it in one place reduces the risk of the copies drifting out of sync.
mcp/tools/aws_ec2_ri.go#L143-156: generalizeeffectiveDryRunConfirminto a shared helper (e.g.resolveDryRunConfirm(dryRun, confirm *bool) (bool, bool)) taking the two*boolpointers directly instead of theec2RIPurchaseArgsstruct, and call it from here.mcp/tools/aws_elasticache_ri.go#L145-152: replace the inline block with a call to the shared helper.mcp/tools/aws_rds_ri.go#L152-159: replace the inline block with a call to the shared helper.mcp/tools/aws_savingsplans.go#L180-187: replace the inline block with a call to the shared helper.mcp/tools/aws_simple_ri.go#L184-191: replace the inline block with a call to the 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, Extract the duplicated dryRun/confirm defaulting into a shared resolveDryRunConfirm(dryRun, confirm *bool) helper, preserving defaults of true and false and applying non-nil overrides. Refactor effectiveDryRunConfirm in the EC2 purchase flow to use this pointer-based helper, then replace the equivalent inline blocks in the ElastiCache RI, RDS RI, Savings Plans, and Simple RI purchase flows with calls to it.
🤖 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/README.md`:
- Around line 65-67: Update the MCP client registration guidance to state that
when GOBIN is unset, users must use $(go env GOPATH)/bin/cudly-mcp; remove the
misleading implication that $(go env GOBIN)/cudly-mcp is valid in that case.
Preserve the absolute-path guidance for configurations where GOBIN is set.
In `@mcp/server.go`:
- Around line 48-55: Update the registration flow around registrations(),
descriptors, and listTool so the cudly_list_commitment_actions descriptor is
included in its own catalog. Add the catalog tool’s descriptor to descriptors
before constructing listTool, while preserving the final registration list and
existing descriptor order.
In `@mcp/tools/aws_savingsplans.go`:
- Around line 133-139: Update the EC2Instance validation in the flow containing
validateDatabaseSPConstraints to require args.InstanceFamily in addition to
args.Region, returning a validation error when it is empty before proceeding
with purchase or constraint validation. Preserve existing validation behavior
for other savings plan types.
In `@mcp/tools/azure_compute_ri.go`:
- Around line 114-121: Reject whitespace-only required inputs at the validation
boundaries: update the Azure validation in mcp/tools/azure_compute_ri.go lines
114-121 and the GCP validation in mcp/tools/gcp_computeengine_cud.go lines
98-111 to trim-check region plus VMSize or machine_type while preserving
existing errors; add regression cases for whitespace-only values in
mcp/tools/azure_compute_ri_test.go lines 43-64 and
mcp/tools/gcp_computeengine_cud_test.go lines 46-69.
In `@mcp/tools/purchase.go`:
- Around line 76-87: Populate PurchaseResponse.TermYears in both the preview and
real-purchase branches of ExecutePurchase using the requested purchase term,
ensuring term_years is present in responses when applicable; otherwise remove
TermYears and its JSON contract if the value is not intended to be exposed.
---
Nitpick comments:
In `@mcp/tools/aws_ec2_ri.go`:
- Line 1: Extract the duplicated dryRun/confirm defaulting into a shared
resolveDryRunConfirm(dryRun, confirm *bool) helper, preserving defaults of true
and false and applying non-nil overrides. Refactor effectiveDryRunConfirm in the
EC2 purchase flow to use this pointer-based helper, then replace the equivalent
inline blocks in the ElastiCache RI, RDS RI, Savings Plans, and Simple RI
purchase flows with calls to it.
🪄 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: d6ca4bc7-4a19-41dc-849d-8311bfdb978d
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
README.mdcmd/cudly-mcp/main.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.go
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 34 minutes. |
Adversarial review follow-up (Opus pass)An independent adversarial review of this PR turned up two defects beyond what CodeRabbit's automated pass flagged. Both are fixed in the commits pushed just now. DEFECT 1 (HIGH, confirmed at runtime): provider factories were never registered for the MCP binary
Verified with Fix: added the same three blank imports to Regression test: added DEFECT 2 (MEDIUM): idempotency key collided across genuinely distinct repeat purchases
Fix: the idempotency key now folds in a discriminator that is either an explicit caller-supplied Added test coverage in Both fixes keep the rest of the money-path hygiene this PR already had intact (dry-run fail-safe, typed-enum validation, nullable cost fields, no PII in logs) -- nothing there needed to change.
|
|
🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
Real GCP CUD purchases are refused unless gcp_project_id is supplied, which
is correct, but the refusal read:
refusing real purchase: the target account could not be determined, so
pass gcp_project_id explicitly (or set ).
Two problems. It named an EMPTY environment variable, because GCP has no
ambient scope source for CredentialScope to fall back to. And "could not be
determined" implies an ambient source exists, sending a GCP caller looking
for a variable nothing reads.
Keeping the refusal strict, rather than inventing a GCP env fallback, is
deliberate:
- Nothing in providers/gcp reads GOOGLE_CLOUD_PROJECT or
CLOUDSDK_CORE_PROJECT, so adding one would invent a convention the
provider does not honor.
- It would also be unsafe. With no project configured the provider falls
back to getDefaultProject, which returns the FIRST ACTIVE PROJECT in the
caller's ListProjects response -- an artifact of IAM visibility and API
ordering, not a project anyone declared. An env-supplied scope could
name project A while the commitment landed in project B, making the
idempotency token assert an account it never touched. That is the
aliasing hazard requireCredentialScope exists to remove, in a worse form.
Providers now carry a credentialScopeSource describing their tool argument
and their ambient variable, if any. GCP has none, so it gets a plain
"gcp_project_id is required" message that explains why there is no fallback;
AWS and Azure keep naming AWS_PROFILE and AZURE_SUBSCRIPTION_ID so a caller
who set the variable is not told to pass an argument they do not need. The
doc comment records the reasoning so the missing fallback is not "fixed"
later. mcp/README.md states the contract alongside the existing scope rules.
TestRequireCredentialScopeGCPMessageIsActionable pins that GCP says
"is required" and never "could not be determined", and that AWS and Azure
still name their variables. Verified failing before this change.
The schema/description half of this finding, and the sibling effective_date
and test-assertion findings, are already fixed on this branch by f8ae701,
3a65136 and 85a86ac; this commit deliberately does not duplicate them.
Their rfc3339OrNil returns *string, which distinguishes "unknown" better than
the string version drafted here, so that one is kept and mine dropped.
|
All three CodeRabbit findings are addressed. Two were already fixed on the branch concurrently ( 1. GCP CUD purchases refused — decision: keep it strict, and it is now a stated contractI took the strict option, and after reading the provider I'd put it more strongly than "defensible": adding a When no project is configured, The schema/description half was fixed by Note the empty variable name — Providers now carry a AWS and Azure still name
2. Fabricated
|
Addressed the three actionables: two fixed, one dismissed with reasoningHead is now 1.
|
| Check | Result |
|---|---|
gofmt -l . |
clean (empty list) |
go vet ./... |
clean |
go build ./... |
ok |
go test ./mcp/... ./providers/aws/... ./cmd/cudly-mcp/... |
15/15 packages ok |
golangci-lint v2.10.1 (CI-pinned) |
0 issues. |
gocyclo -over 10 -ignore "_test\.go" . |
no findings |
The three purchase-path safety invariants were re-verified by reading the code after the rebase rather than assumed: the idempotency key still folds the credential scope in as its second component and reads no clock; requireCredentialScope still runs ahead of both the ResolveClient nil-check and the ResolveClient call, so a refusal resolves no credentials; and isRegionAgnostic still requires all three of CommitmentSavingsPlan, an empty effective region, and a recognised account-level plan type.
@coderabbitai full review
|
I’ll run a full review of 🐇🔍 ✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
mcp/tools/search_recommendations.go (2)
220-241: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSix sequential Cost Explorer calls on an interactive request path.
When both
term_yearsandpayment_optionare omitted for an AWS reservation search, this issues 6 serializedGetRecommendationscalls. Cost Explorer routinely takes seconds per call, so a fully-omitted search can run tens of seconds with no per-combo deadline — long enough for an MCP client to time out the tool call and lose the whole result set (fail-whole-search means there is no partial value to salvage). The combos are independent; running them under a boundederrgroupwould cut wall time to roughly one call.Not a blocker for merge — the fail-loud semantics documented above are the right call — but worth measuring before this lands in front of real accounts.
🤖 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 220 - 241, Update fetchSearchCombos to execute independent recClient.GetRecommendations calls concurrently using a bounded errgroup, limiting in-flight requests to avoid unbounded concurrency. Preserve the existing fail-whole-search behavior and error context, while safely aggregating results from successful combos and propagating the first error.
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
LookbackPeriodconstants instead of string literals.Every other override here derives from the typed enum;
lookback_periodalone hardcodes its values, so adding a window toenums.gowould updateValidateLookbackPeriodwhile leaving the advertised schema silently stale.♻️ Proposed fix
- "lookback_period": {Enum: []any{"7d", "30d", "60d"}}, + "lookback_period": {Enum: []any{ + string(LookbackPeriod7Days), + string(LookbackPeriod30Days), + string(LookbackPeriod60Days), + }},🤖 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 79 - 88, Update the lookback_period override in the BuildInputSchema configuration to derive its Enum values from the typed LookbackPeriod constants rather than hardcoded duration strings. Preserve the existing 7d, 30d, and 60d options while ensuring future enum additions stay reflected in the advertised schema.mcp/tools/enums_test.go (1)
207-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
ValidateLookbackPeriodtable test.Every other validator in
enums.gois pinned here, but the one with non-obvious semantics — empty is valid ("let the provider default"), while90d/7Dare not — has no coverage. That carve-out is exactly the behavior a future refactor is most likely to break.💚 Proposed test
func TestValidateLookbackPeriod(t *testing.T) { t.Parallel() cases := []struct { name string in string want LookbackPeriod wantErr bool }{ {"7d", "7d", LookbackPeriod7Days, false}, {"30d", "30d", LookbackPeriod30Days, false}, {"60d", "60d", LookbackPeriod60Days, false}, {"empty means provider default", "", "", false}, {"unsupported window", "90d", "", true}, {"case sensitive", "7D", "", true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { got, err := ValidateLookbackPeriod(tc.in) if tc.wantErr { assert.Error(t, err) return } assert.NoError(t, err) assert.Equal(t, tc.want, got) }) } }🤖 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_test.go` around lines 207 - 231, Add a table-driven TestValidateLookbackPeriod alongside TestValidateScope, covering valid 7d, 30d, and 60d values, valid empty input returning the zero LookbackPeriod, and errors for unsupported 90d and case-sensitive 7D inputs. Use the existing assertion pattern and run subtests in parallel.mcp/server_test.go (2)
44-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray doc comment attached to the wrong test.
Lines 44-49 document
TestRealPurchaseToolsDocumentMoneyImpactAndDryRun(defined at line 158) but sit immediately aboveTestEndToEndSearchThenDryRunPurchase, so godoc/readers get two concatenated, contradictory descriptions for one function. Move that block back onto the function it describes.🤖 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, Move the comment block describing TestRealPurchaseToolsDocumentMoneyImpactAndDryRun so it immediately precedes that function’s declaration. Leave the TestEndToEndSearchThenDryRunPurchase comment with only its end-to-end protocol description.
63-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCancel the server context so
server.Rungoroutines exit with the test.
ctx := context.Background()plusgo server.Run(ctx, serverTransport)leaves one server goroutine (and its transport) alive for the rest of the package's test binary; this repeats inTestListCommitmentActionsIncludesItself(Lines 121-129),TestAzureComputeRIPurchaseSchemaExcludesPartialUpfront(Lines 183-191) andTestSearchRecommendationsSchemaAdvertisesTermAndPaymentEnums(Lines 235-243).cmd/cudly-mcp/main_test.goalready uses a cancellable context for the same pattern.♻️ Suggested change (apply to each of the four tests)
- ctx := context.Background() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel()🤖 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 63 - 71, Use a cancellable context instead of context.Background in the four tests that start server.Run, including TestListCommitmentActionsIncludesItself, TestAzureComputeRIPurchaseSchemaExcludesPartialUpfront, and TestSearchRecommendationsSchemaAdvertisesTermAndPaymentEnums; defer cancel so each server goroutine and its transport exits when the test completes.mcp/tools/purchase.go (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile exceeds the 500-line limit (664 lines).
Splitting the audit-logging helpers (
logPurchaseAttempt/logPurchaseOutcome), the idempotency-key derivation (idempotencyKeyFor+*DetailsKey), and the Archera offer into their own files in this package would bring it back under the limit without touching behavior.As per coding guidelines, "keep files under 500 lines".
🤖 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/purchase.go` around lines 1 - 14, Split purchase.go into focused files within the tools package to keep each file under 500 lines: move the audit helpers logPurchaseAttempt and logPurchaseOutcome, idempotency-key logic including idempotencyKeyFor and the *DetailsKey types, and the Archera offer implementation into separate files. Preserve all existing behavior, symbols, and package-level access.Source: Coding guidelines
mcp/tools/purchase_test.go (1)
1-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile exceeds the 500-line limit (1188 lines). Consider splitting into focused test files (gate/authorization, idempotency-key, audit-logging, Archera offer).
As per coding guidelines, "keep files under 500 lines".
🤖 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/purchase_test.go` around lines 1 - 18, Split purchase_test.go into focused test files so each remains under 500 lines, grouping tests by gate/authorization, idempotency-key, audit-logging, and Archera offer behavior. Preserve the existing package, shared imports, fixtures, and test behavior while moving related tests together and removing imports that are no longer used in each resulting file.Source: Coding guidelines
providers/azure/services/synapse/client_test.go (1)
788-853: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffBoth test files now well exceed the repo's 500-line guideline.
providers/azure/services/synapse/client_test.go(958+ lines) andproviders/aws/service_client_test.go(583+ lines) are both already past the limit, and this PR's new test additions push them further over.
providers/azure/services/synapse/client_test.go#L788-L853: consider splitting the billing-plan/payment-option tests (and other purchase-commitment scenarios) into a separate_billingplan_test.go(or similar) file in the same package.providers/aws/service_client_test.go#L293-L583: consider extracting theapplyRecommendationFiltersregion-filtering tests into a dedicatedservice_client_region_filter_test.gofile.As per coding guidelines, "
**/*.{go,ts,tsx}: Follow Domain-Driven Design with bounded contexts, keep files under 500 lines, and use typed interfaces for public APIs."🤖 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/synapse/client_test.go` around lines 788 - 853, Split the Synapse billing-plan/payment-option tests around TestPurchaseCommitment_billingPlan into a same-package file such as client_billingplan_test.go, preserving their behavior and shared test helpers. Also move the applyRecommendationFilters region-filtering tests from providers/aws/service_client_test.go lines 293-583 into a dedicated same-package file such as service_client_region_filter_test.go; both original files should retain only unrelated tests.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/tools/aws_ec2_ri.go`:
- Around line 201-209: Trim credential arguments before constructing
ProviderConfig so provider authentication matches the normalized idempotency
scope. Update resolveClient in mcp/tools/aws_ec2_ri.go (201-209),
aws_elasticache_ri.go (152-160), aws_rds_ri.go (160-168), aws_simple_ri.go
(201-209), aws_savingsplans.go (299-307), azure_compute_ri.go (181-189), and
gcp_computeengine_cud.go (191-199) to pass trimmed AWSProfile,
AzureSubscriptionID, or GCPProjectID values respectively.
In `@mcp/tools/aws_savingsplans.go`:
- Around line 176-215: Canonicalize the region in the purchase flow for
account-level plan types (Compute, SageMaker, and Database) so caller-supplied
values cannot affect the purchase identity or idempotency token. Update the
region assignment around SPTypeEC2Instance and Recommendation construction to
always use savingsPlansAccountLevelRegion for non-EC2Instance plans, while
preserving the trimmed caller region for EC2Instance plans and their Details.
---
Nitpick comments:
In `@mcp/server_test.go`:
- Around line 44-61: Move the comment block describing
TestRealPurchaseToolsDocumentMoneyImpactAndDryRun so it immediately precedes
that function’s declaration. Leave the TestEndToEndSearchThenDryRunPurchase
comment with only its end-to-end protocol description.
- Around line 63-71: Use a cancellable context instead of context.Background in
the four tests that start server.Run, including
TestListCommitmentActionsIncludesItself,
TestAzureComputeRIPurchaseSchemaExcludesPartialUpfront, and
TestSearchRecommendationsSchemaAdvertisesTermAndPaymentEnums; defer cancel so
each server goroutine and its transport exits when the test completes.
In `@mcp/tools/enums_test.go`:
- Around line 207-231: Add a table-driven TestValidateLookbackPeriod alongside
TestValidateScope, covering valid 7d, 30d, and 60d values, valid empty input
returning the zero LookbackPeriod, and errors for unsupported 90d and
case-sensitive 7D inputs. Use the existing assertion pattern and run subtests in
parallel.
In `@mcp/tools/purchase_test.go`:
- Around line 1-18: Split purchase_test.go into focused test files so each
remains under 500 lines, grouping tests by gate/authorization, idempotency-key,
audit-logging, and Archera offer behavior. Preserve the existing package, shared
imports, fixtures, and test behavior while moving related tests together and
removing imports that are no longer used in each resulting file.
In `@mcp/tools/purchase.go`:
- Around line 1-14: Split purchase.go into focused files within the tools
package to keep each file under 500 lines: move the audit helpers
logPurchaseAttempt and logPurchaseOutcome, idempotency-key logic including
idempotencyKeyFor and the *DetailsKey types, and the Archera offer
implementation into separate files. Preserve all existing behavior, symbols, and
package-level access.
In `@mcp/tools/search_recommendations.go`:
- Around line 220-241: Update fetchSearchCombos to execute independent
recClient.GetRecommendations calls concurrently using a bounded errgroup,
limiting in-flight requests to avoid unbounded concurrency. Preserve the
existing fail-whole-search behavior and error context, while safely aggregating
results from successful combos and propagating the first error.
- Around line 79-88: Update the lookback_period override in the BuildInputSchema
configuration to derive its Enum values from the typed LookbackPeriod constants
rather than hardcoded duration strings. Preserve the existing 7d, 30d, and 60d
options while ensuring future enum additions stay reflected in the advertised
schema.
In `@providers/azure/services/synapse/client_test.go`:
- Around line 788-853: Split the Synapse billing-plan/payment-option tests
around TestPurchaseCommitment_billingPlan into a same-package file such as
client_billingplan_test.go, preserving their behavior and shared test helpers.
Also move the applyRecommendationFilters region-filtering tests from
providers/aws/service_client_test.go lines 293-583 into a dedicated same-package
file such as service_client_region_filter_test.go; both original files should
retain only unrelated tests.
🪄 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: 09cb8a92-fb6c-434b-b742-32ccba69ac16
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (59)
README.mdcmd/cudly-mcp/main.gocmd/cudly-mcp/main_test.gocmd/multi_service_stats.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_fanout_test.gomcp/tools/search_recommendations_test.gopkg/common/archera.gopkg/common/types.gopkg/common/types_test.goproviders/aws/recommendations/parser_ri.goproviders/aws/recommendations/parser_ri_test.goproviders/aws/recommendations/parser_sp.goproviders/aws/recommendations/parser_sp_test.goproviders/aws/service_client.goproviders/aws/service_client_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
…ase identity Addresses the two unresolved CodeRabbit threads on #1495. Both let an input that should not affect purchase identity change the idempotency token, and a forked token means ClientToken dedupe misses and the purchase runs twice. region is documented as "ignored for account-level plan types", but for Compute/SageMaker/Database it was still assigned to region and rec.Region and so reached idempotencyKeyFor. Buying a $10/hr Compute SP once without region and re-issuing the identical call with region="eu-west-1" (a self-correction, or a retry that fills the field in) derived two different tokens and bought a second plan. Pin the canonical account-level region for those plan types; only EC2Instance plans are genuinely region-scoped, and validateSavingsPlanArgs already requires a non-blank region for those. details.Region was already correctly gated on EC2Instance; this closes the same gap for rec.Region. Every resolveClient closure forwarded the raw credential argument to ProviderConfig while CredentialScope trimmed the same value for the token, so a padded " prod " named one account in the token and another (or none) in the config that actually authenticates the purchase. Route all seven sites through CredentialScope itself rather than an open-coded TrimSpace: it is the function the token already uses, so the two can no longer diverge. Passing no env-var fallback yields the trimmed explicit value, and an empty result correctly means "use ambient credentials", which is what the token recorded anyway. Both guards were confirmed to fail against the pre-fix code. The credential test is table-driven across all seven resolveClient implementations rather than one per provider: the bug was seven independent copies of the same expression, so a guard on a subset would let a future edit reintroduce it elsewhere. TestEC2InstanceSavingsPlanStillHonorsRegion pins the other side, so the canonicalization cannot silently buy an EC2Instance plan in the wrong region.
|
Pushed
Regression coverage. Both guards were confirmed to FAIL against the pre-fix code and pass after, rather than being written to match current behavior:
Local gates on Separately, still unaddressed from the earlier pass and not mine to land: @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/idempotency_scope_test.go`:
- Around line 61-77: Extend TestEC2InstanceSavingsPlanStillHonorsRegion to
generate idempotency tokens for equivalent EC2Instance savings-plan arguments in
different regions and assert the tokens differ. Keep the existing recommendation
region assertions, and use the idempotencyKeyFor path to verify region changes
are preserved rather than collapsed.
🪄 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: 42e9ea3c-a921-4f4f-b21a-bf5d2dd4aa0b
📒 Files selected for processing (8)
mcp/tools/aws_ec2_ri.gomcp/tools/aws_elasticache_ri.gomcp/tools/aws_rds_ri.gomcp/tools/aws_savingsplans.gomcp/tools/aws_simple_ri.gomcp/tools/azure_compute_ri.gomcp/tools/gcp_computeengine_cud.gomcp/tools/idempotency_scope_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- mcp/tools/azure_compute_ri.go
- mcp/tools/aws_elasticache_ri.go
- mcp/tools/gcp_computeengine_cud.go
- mcp/tools/aws_rds_ri.go
- mcp/tools/aws_ec2_ri.go
- mcp/tools/aws_savingsplans.go
- mcp/tools/aws_simple_ri.go
…kens Addresses the CodeRabbit finding on 44b6094. The test's own doc comment claimed region must still reach "the resolved region and the token" for EC2Instance plans, but it only asserted rec.Region and details.Region, leaving the token half of that claim untested. That gap matters in the opposite direction from the bug the previous commit fixed: a future change that canonicalized region for EC2Instance too would keep the field assertions green while collapsing two genuinely different purchases (m5 in eu-west-1 vs m5 in us-east-1) onto one token, at which point the second, legitimately distinct purchase would dedupe away and never happen. Derive tokens for the same plan in two regions and assert they differ. Confirmed the assertion fails when the region is pinned for every plan type, so it catches the over-pinning regression rather than restating what the field assertions already cover.
|
Pushed
Worth spelling out why this matters, since it guards the opposite direction from the bug the previous commit fixed. A future change that canonicalized region for EC2Instance too would keep the field assertions green while collapsing two genuinely different purchases (m5 in eu-west-1 vs m5 in us-east-1) onto one token, at which point the second, legitimately distinct purchase would dedupe away and silently never happen. The test now derives tokens for the same plan in two regions and asserts they differ. Confirmed it FAILS when the region is pinned for every plan type, so it catches the over-pinning regression rather than restating what the field assertions already covered. CI on Still open and not mine to land: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline mcp/tools/idempotency_scope_test.go --items all --type function --match 'TestAccountLevelSavingsPlanRegionCannotForkIdempotencyToken|TestEC2InstanceSavingsPlanStillHonorsRegion'
ast-grep outline mcp/tools/aws_savingsplans.go --items all --type function --match 'savingsPlanRecommendationFromArgs'
ast-grep outline mcp/tools/purchase.go --items all --type function --match 'idempotencyKeyFor|detailsKeyComponent|ExecutePurchase|rfc3339OrNil'
printf '\n--- idempotency test ---\n'
sed -n '1,260p' mcp/tools/idempotency_scope_test.go
printf '\n--- savings plans recommendation parsing ---\n'
rg -n -A150 -B20 'func savingsPlanRecommendationFromArgs' mcp/tools/aws_savingsplans.go
printf '\n--- token and timestamp shaping ---\n'
rg -n -A180 -B25 'func idempotencyKeyFor|func detailsKeyComponent|func rfc3339OrNil|EffectiveDate' mcp/tools/purchase.go
printf '\n--- current commit diff ---\n'
git show --format=fuller --stat --find-renames bdb43cd30
git show --format= --find-renames bdb43cd30 -- mcp/tools/idempotency_scope_test.goLength of output: 45839
Also, the stale No new actionable findings from this change. ✅ Action performedReview finished.
|
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 19 minutes. |
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
mcp/tools/purchase.go (1)
462-477: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOnly
ComputeDetailshas a value-type case; the other three silently fall through to"".If any tool ever passes
common.DatabaseDetails/CacheDetails/SavingsPlanDetailsby value (asComputeDetailsapparently already is somewhere), the type switch hitsdefaultand drops every service-specific dimension from the key — a silent token collision on a money path, exactly what this function exists to prevent. Adding the value cases makes the behavior uniform.♻️ Proposed change
case *common.DatabaseDetails: return databaseDetailsKey(d) + case common.DatabaseDetails: + return databaseDetailsKey(&d) case *common.CacheDetails: return cacheDetailsKey(d) + case common.CacheDetails: + return cacheDetailsKey(&d) case *common.SavingsPlanDetails: return savingsPlanDetailsKey(d) + case common.SavingsPlanDetails: + return savingsPlanDetailsKey(&d)🤖 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/purchase.go` around lines 462 - 477, Update detailsKeyComponent to handle value-type common.DatabaseDetails, common.CacheDetails, and common.SavingsPlanDetails cases alongside their pointer cases. Pass each value case by address to its existing key helper, preserving the current pointer behavior and preventing service-specific dimensions from being dropped.mcp/server_test.go (1)
44-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray doc comment: the
TestRealPurchaseToolsDocumentMoneyImpactAndDryRunblock is attached toTestEndToEndSearchThenDryRunPurchase.Lines 44-49 document a different test (defined at line 158, which now has no doc comment). Move that paragraph above its own function.
🤖 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, Move the documentation paragraph beginning with TestRealPurchaseToolsDocumentMoneyImpactAndDryRun above the TestRealPurchaseToolsDocumentMoneyImpactAndDryRun function, and leave the TestEndToEndSearchThenDryRunPurchase comment describing only the end-to-end test. Ensure the latter test’s existing documentation remains directly attached to its function.mcp/tools/aws_ec2_ri.go (1)
68-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
platformis missing the Enum/Default schema override thattenancy/scopeget.
platformhas the same shape astenancy/scope(small SDK-backed enum + documented default), but only those two are advertised viaFieldOverridein the schema. An MCP client can't discover the 4 validplatformvalues without a round trip that fails validation.♻️ Proposed fix
schema, err := BuildInputSchema[ec2RIPurchaseArgs](map[string]FieldOverride{ "term_years": {Enum: []any{int(TermOneYear), int(TermThreeYear)}}, "payment_option": {Enum: []any{string(PaymentOptionAllUpfront), string(PaymentOptionPartialUpfront), string(PaymentOptionNoUpfront)}}, + "platform": {Enum: []any{ + string(ec2types.RIProductDescriptionLinuxUnix), + string(ec2types.RIProductDescriptionLinuxUnixAmazonVpc), + string(ec2types.RIProductDescriptionWindows), + string(ec2types.RIProductDescriptionWindowsAmazonVpc), + }, Default: string(ec2types.RIProductDescriptionLinuxUnix)}, "scope": {Enum: []any{string(ScopeRegion), string(ScopeAvailabilityZone)}, Default: string(ScopeRegion)}, "tenancy": {Enum: []any{string(TenancyDefault), string(TenancyDedicated)}, Default: string(TenancyDefault)}, "dry_run": {Default: true}, "confirm": {Default: false}, })🤖 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 68 - 78, Update the BuildInputSchema overrides for ec2RIPurchaseArgs to include platform with the SDK’s four valid platform enum values and its documented default, matching the existing scope and tenancy overrides. Use the existing platform constants and preserve all other schema overrides unchanged.mcp/tools/gcp_computeengine_cud_test.go (1)
48-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the explicit-project requirement.
The PR objectives call out that GCP real purchases require an explicit project, but the invalid-args table never mutates
GCPProjectID. A blank/whitespacegcp_project_idcase would pin that requirement here.💚 Suggested additional cases
{"invalid term", func(a *gcpComputeEngineCUDPurchaseArgs) { a.TermYears = 2 }, "invalid term_years"}, + {"missing gcp_project_id", func(a *gcpComputeEngineCUDPurchaseArgs) { a.GCPProjectID = "" }, "gcp_project_id"}, + {"whitespace-only gcp_project_id", func(a *gcpComputeEngineCUDPurchaseArgs) { a.GCPProjectID = " " }, "gcp_project_id"},🤖 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_test.go` around lines 48 - 73, Add an invalid case to TestGCPComputeEngineRecommendationFromArgsInvalid that mutates GCPProjectID to blank or whitespace and expects the gcp_project_id-required validation error, preserving the existing table-driven test pattern.
🤖 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/search_recommendations.go`:
- Around line 412-419: Update trimSearchArgsIdentifiers to trim the credential
override fields aws_profile, azure_subscription_id, and gcp_project_id before
returning args, ensuring providerConfigFromArgs receives normalized values like
the existing region fields.
In `@providers/azure/services/internal/reservations/purchase.go`:
- Around line 105-121: Update the default branch of BillingPlanForPaymentOption
so the error message describes only the actual unsupported paymentOption value;
mention partial-upfront’s lack of an Azure equivalent only when that specific
value is supplied, without attributing it to unrelated inputs.
---
Nitpick comments:
In `@mcp/server_test.go`:
- Around line 44-61: Move the documentation paragraph beginning with
TestRealPurchaseToolsDocumentMoneyImpactAndDryRun above the
TestRealPurchaseToolsDocumentMoneyImpactAndDryRun function, and leave the
TestEndToEndSearchThenDryRunPurchase comment describing only the end-to-end
test. Ensure the latter test’s existing documentation remains directly attached
to its function.
In `@mcp/tools/aws_ec2_ri.go`:
- Around line 68-78: Update the BuildInputSchema overrides for ec2RIPurchaseArgs
to include platform with the SDK’s four valid platform enum values and its
documented default, matching the existing scope and tenancy overrides. Use the
existing platform constants and preserve all other schema overrides unchanged.
In `@mcp/tools/gcp_computeengine_cud_test.go`:
- Around line 48-73: Add an invalid case to
TestGCPComputeEngineRecommendationFromArgsInvalid that mutates GCPProjectID to
blank or whitespace and expects the gcp_project_id-required validation error,
preserving the existing table-driven test pattern.
In `@mcp/tools/purchase.go`:
- Around line 462-477: Update detailsKeyComponent to handle value-type
common.DatabaseDetails, common.CacheDetails, and common.SavingsPlanDetails cases
alongside their pointer cases. Pass each value case by address to its existing
key helper, preserving the current pointer behavior and preventing
service-specific dimensions from being dropped.
🪄 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: df0c93bb-dc6e-4ff4-bd3c-9ce6cc4bc7a0
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
README.mdcmd/cudly-mcp/main.gocmd/cudly-mcp/main_test.gocmd/multi_service_stats.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/idempotency_scope_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_fanout_test.gomcp/tools/search_recommendations_test.gopkg/common/archera.gopkg/common/types.gopkg/common/types_test.goproviders/aws/recommendations/parser_ri.goproviders/aws/recommendations/parser_ri_test.goproviders/aws/recommendations/parser_sp.goproviders/aws/recommendations/parser_sp_test.goproviders/aws/service_client.goproviders/aws/service_client_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
#1504) * fix(azure): coerce web partial-upfront to monthly, not upfront The web/API purchase path coerced Azure partial-upfront payment options to upfront before validation. Since PR #1495 wires Azure billingPlan directly from the normalized payment option, that coercion silently billed an all-upfront schedule the caller never chose. Coerce to monthly (no-upfront, CUDly's default Azure schedule) instead, so the rec still survives validation without flipping the billing schedule. Closes #1503 * docs(azure): correct payment-option mechanism comment (cost split, not billingPlan) The comment on NormalizePaymentOption/crossProviderPaymentAlias said the web/API path wires Azure's billingPlan directly from the normalized payment option. buildReservationBody in providers/azure/services/compute/client.go never emits a billingPlan field for VM reservations; the payment option actually drives the upfront-vs-monthly cost split in that file's GetOfferingDetails. No behavior change, comment accuracy only. * fix(api): warn on payment-option normalization coercion validatePurchaseRecommendation silently overwrote rec.Payment with the NormalizePaymentOption result, contradicting the doc comment that callers are expected to WARN when a raw payment option is coerced to its canonical form (e.g. azure partial-upfront -> monthly). Log the transition via pkg/logging, matching the pattern already used by scheduler.convertRecommendations, so the coercion is auditable on this money-affecting field. The coercion itself is unchanged. * docs(config): future-proof Azure payment-coercion comments for #1495 The coercion-rationale comments assert the normalized payment option is "not a billingPlan request field". That is accurate on main today (the reservation purchase body sends no billingPlan), but becomes false once the billingPlan wiring lands (#1495/#1502), and that wiring does not touch this file, so the comment would silently go stale on a money-path rationale. Rephrase to state the forward-compatible fact instead: the wiring maps only upfront/monthly tokens and hard-errors on partial-upfront, which is exactly why normalizing to monthly here keeps web-path Azure purchases valid. Comment-only change; no behavior difference. * feat(api): surface payment-option coercions in the purchase response The web execute path silently normalizes cross-provider payment tokens (most notably Azure partial-upfront -> monthly, #1503); until now only operators saw the WARN log while the caller's response gave no hint the billing schedule they requested was changed. Return the coercion to the caller: validatePurchaseRecommendation now also yields a PaymentAdjustment (rec index, provider, service, requested token, applied token, reason) whenever normalization changes the token, and executePurchase attaches the collected list to all three response bodies (approval-pending, duplicate-collapse, direct-execute) under payment_adjustments. The key is omitted entirely when every option was already canonical, so existing clients are unaffected on the common case, and the coercion policy itself (config.NormalizePaymentOption) plus the operator WARN log are unchanged. Tests: the guards table now cross-checks every coerced row surfaces a matching adjustment and every canonical row surfaces none; new field-level unit test for the Azure partial-upfront case; new handler-level tests assert the response body carries the adjustment for a mixed canonical+coerced batch (verified to fail with the attach stubbed out) and omits the key for a canonical-only batch, both with mock.AssertExpectations registered. frontend PurchaseResult gains the matching optional payment_adjustments field. * fix(frontend): coerce azure partial-upfront to monthly, not upfront The Go side of #1503 was fixed to map Azure partial-upfront onto "monthly", but normalizePaymentValue in the frontend still mapped it onto "upfront", so the two halves of the web path disagreed. That function decides which option the plan and purchase dropdowns pre-select (plans.ts:1166 on plan edit, plans.ts:1562 when prefilling from a selected commitment), so a rec carrying the legacy partial-upfront token pre-selected "Pay Upfront". Saving from that state submits an already-canonical "upfront", which the backend accepts verbatim, so the backend coercion never gets a chance to run and the user is committed to a full upfront charge they never chose. Azure offers exactly two billing plans and the total cost is identical either way, but the plan cannot be changed after purchase, so landing on monthly is the only direction that cannot surprise the user with an irreversible upfront charge. The jest case that pinned the old expectation is updated and now documents why monthly is required, mirroring the Go-side assertion in TestNormalizePaymentOption. Verified failing before this change and passing after. Refs #1503 * docs(config): correct azure payment-coercion rationale The doc comments justifying the partial-upfront to monthly coercion cited providers/azure/services/compute/client.go's GetOfferingDetails as the consumer that "drives the upfront-vs-monthly cost split". GetOfferingDetails has no non-test callers anywhere in the repo; it exists only to satisfy the ServiceClient interface, so the stated mechanism does not run. On a money path a wrong rationale is worse than none, since the next reader reasons from it. Replace it with what actually consumes the canonical token today (persisted on the execution, then copied into common.Recommendation.PaymentOption in internal/purchase/execution.go) and the billingPlan wiring arriving in #1495/#1502. Also record the two Microsoft-documented facts the decision rests on: upfront and monthly cost the same total, and the billing frequency cannot be changed after purchase. Note the documented exception that monthly is unavailable for SUSE Linux, Red Hat, Azure Red Hat OpenShift and pre-purchase plans, where the coerced token makes Azure reject the purchase; that loud rejection is the intended outcome. Adds the cross-reference to the frontend mirror of this mapping so the two stay in lockstep. Comments only, no behaviour change. Refs #1503 * fix(frontend): surface azure payment coercion to the user (#1503) The backend already returns `payment_adjustments` when it normalizes a requested payment option onto a different provider-canonical token (Azure has exactly two billing plans, Upfront and Monthly, so an inherited AWS-style `partial-upfront` has nowhere to land and is coerced to monthly). The field was declared in `frontend/src/api/types.ts` and read by nothing, so the coercion was disclosed to API clients and to the operator WARN log but never to the web user whose billing schedule actually changed. Coercing instead of rejecting is only defensible if the user is told, so this closes that gap: - Add `formatPaymentAdjustmentNotice` next to `normalizePaymentValue`, the mapping it discloses. Reuses the existing `getPaymentLabel` so the copy shows "Partial Upfront"/"Pay Monthly" rather than raw API tokens, and collapses a batch to its distinct requested -> applied pairs. - Render it as a separate non-expiring warning toast on both purchase submit paths (single and fan-out) so an irreversible billing-schedule change cannot scroll away inside a success message. The fan-out path collects from every fulfilled response, including buckets whose approval email failed, since those still created a pending execution carrying the coerced schedule. - Extract the inline `payment_adjustments` element type into a named `PaymentAdjustment` interface so the formatter is typed against the same shape the API returns. Regression coverage: the two disclosure tests in purchase-execution-toast.test.ts fail with the wiring removed and pass with it, exercising the real handler through to the rendered toast; a formatter-only unit test could not prove the notice reaches the user. * fix(api): disclose payment coercion only when the schedule changes The #1503 disclosure fired on every raw != canonical rewrite, including the cross-provider renames that leave the customer's cash flow untouched. Azure spells AWS's all-upfront "upfront" and AWS's no-upfront "monthly", so both of those rewrites are bookkeeping, not a billing change. That mattered on the ordinary path, not an edge case: the fan-out purchase modal builds its per-bucket Payment dropdown from paymentOptionsFor (frontend/src/lib/purchase-compatibility.ts), whose candidate list has no "upfront" entry. An Azure bucket the user chooses to pay upfront therefore ALWAYS submits "all-upfront", so every ordinary Azure upfront purchase raised a sticky "Billing schedule adjusted" warning claiming Azure "does not offer All Upfront" and that the purchase was applied as something else. Both claims are false, and the noise trains users to dismiss the one notice that is real. Classify a payment token by the cash flow it implies (config.PaymentScheduleFor) and surface a PaymentAdjustment only when the rewrite crosses schedules (config.PaymentCoercionChangesSchedule). Azure partial-upfront -> monthly and GCP upfront -> monthly still disclose; Azure all-upfront -> upfront and no-upfront -> monthly no longer do. The operator WARN keeps firing on every rewrite: a non-canonical token on the wire is an upstream input bug worth auditing even when it costs the customer nothing. Only the user-facing notice is narrowed. Adjustment construction moves into paymentAdjustmentFor so validatePurchaseRecommendation stays at gocyclo 10 (the pre-commit threshold), not 11. Regression coverage: the three rename cases assert nil adjustment plus a still-emitted WARN, and the GCP upfront-shaped tokens assert a surfaced adjustment. Both new tests fail against the previous raw != canonical condition and pass after (verified by reverting the condition).
…partial-upfront Addresses the two unresolved CodeRabbit threads on bdb43cd. trimSearchArgsIdentifiers normalized service, region, include_regions, exclude_regions, and account_filter but not the three credential overrides, so providerConfigFromArgs forwarded aws_profile / azure_subscription_id / gcp_project_id into ProviderConfig raw. A padded " my-profile " then failed credential resolution for a reason the resulting error never mentions, reading as "these credentials are broken" rather than "this name has a stray space". The purchase tools already normalize the same three fields through CredentialScope; this closes the gap on the search path. BillingPlanForPaymentOption's default branch appended "(partial-upfront has no azure equivalent)" to every unrecognized value, so a typo ("montly") or a value from another provider's vocabulary ("prepaid") was diagnosed as a partial-upfront problem the caller never had. Give partial-upfront its own case -- it is a real payment option elsewhere in CUDly, so the caller does need to know Azure has no equivalent -- and report anything else as simply not recognized. This extends the same split an earlier commit already made for the empty value, for the same reason: an error that names the wrong cause sends whoever reads it looking in the wrong place. A dedicated case rather than a conditional inside default, so the switch's already-lowercased and trimmed value decides it instead of re-normalizing. Both guards were confirmed to fail against the pre-fix code. The Azure test also pins that " Partial-Upfront " still reaches the partial-upfront case rather than falling through to the unrecognized branch.
|
Pushed
Regression coverage. Both guards were confirmed to FAIL against the pre-fix code and pass after. The Azure test also pins that Local gates on One note on the Azure submodule: Also confirming the @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit summary and changed hunks ---'
git show --format=fuller --stat --find-renames a1d269686
git show --format= --find-renames a1d269686 -- \
mcp/tools/search_recommendations.go \
mcp/tools/search_recommendations_test.go \
providers/azure/services/internal/reservations/purchase.go \
providers/azure/services/internal/reservations/purchase_test.go
printf '%s\n' '--- relevant source outlines ---'
ast-grep outline mcp/tools/search_recommendations.go --items all --type function --match 'trimSearchArgsIdentifiers|providerConfigFromArgs'
ast-grep outline providers/azure/services/internal/reservations/purchase.go --items all --type function --match 'BillingPlanForPaymentOption'
printf '%s\n' '--- search normalization and configuration ---'
rg -n -A90 -B15 'func trimSearchArgsIdentifiers|func providerConfigFromArgs' \
mcp/tools/search_recommendations.go
printf '%s\n' '--- Azure billing-plan mapping ---'
rg -n -A90 -B15 'func BillingPlanForPaymentOption' \
providers/azure/services/internal/reservations/purchase.go
printf '%s\n' '--- focused regression tests ---'
rg -n -C4 'AWSProfile|AzureSubscriptionID|GCPProjectID|Credential|Partial-Upfront|partial-upfront|recognized payment option' \
mcp/tools/search_recommendations_test.go \
providers/azure/services/internal/reservations/purchase_test.goLength of output: 40740 🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline mcp/tools/search_recommendations.go --items all --type method,function --match 'handle'
printf '%s\n' '--- handler call order ---'
rg -n -A150 -B20 'func \(t \*searchRecommendationsTool\) handle' \
mcp/tools/search_recommendations.go
printf '%s\n' '--- commit file list (sanity check) ---'
git diff-tree --no-commit-id --name-status -r a1d269686
printf '%s\n' '--- parent comparison for changed tests ---'
git diff --check a1d269686^ a1d269686Length of output: 9064
Both reported findings are addressed:
The diff is limited to the four stated implementation/test files, and the inspected diff has no whitespace errors. No new actionable findings. ✏️ Learnings added
✅ Action performedReview finished.
|
ARM subscription IDs are case-insensitive GUIDs, and nothing in
providers/azure canonicalizes them -- resolveAzureSubscriptionID forwards
whatever it is given straight into the request path. CredentialScope
trimmed the value but never folded its case, so the SAME subscription
spelled two ways derived two DIFFERENT idempotency tokens.
The double-spend that follows:
1. A purchase is issued with
azure_subscription_id="ABC12345-1234-1234-1234-1234567890AB", the
spelling the Azure portal hands you, and times out.
2. The model retries. This time the override is omitted, so the value
comes from AZURE_SUBSCRIPTION_ID, which conventionally holds the
lower-case "abc12345-...".
3. Both calls target the same subscription, but the tokens differ, so
reservations.FindReservationOrderByIdempotencyToken -- which matches
on the token tag across the TENANT-wide order list -- misses the
first order.
4. A SECOND reservation is purchased.
This is the defect family 44b6094 fixed for untrimmed input: purchase
identity forked by a difference the provider itself does not recognize.
Whitespace was normalized there; case was not.
Both Azure uses of the subscription ID -- the idempotency scope in handle
and the ProviderConfig in resolveClient -- now go through the single
azureCredentialScope helper, so they cannot normalize it differently.
Case folding is deliberately NOT pushed into CredentialScope: AWS named
profiles are case-SENSITIVE section names in ~/.aws/config, so
lower-casing one would point a real purchase at a profile that does not
exist or at a different one that does. GCP project IDs are lower-case by
grammar. The new guard covers both directions -- two cases of one Azure
subscription must derive ONE token, two different subscriptions must keep
two -- and asserts AWS profile case is still preserved end to end, so the
Azure fix cannot leak into the AWS path. It fails on the pre-fix code.
Also in this change:
- cudly_search_recommendations no longer claims it "spends no money".
Since d76aa7c, a reservation search that omits term_years and
payment_option fans out to six Cost Explorer requests, and AWS bills
those per request. The tool buys no commitment, but it is not free, and
a model told otherwise may loop on it. The description now says both.
- The account-level Savings Plan region guard covers Database alongside
Compute and SageMaker. It needs its own 1yr/no-upfront fixture because
validateDatabaseSavingsPlan permits nothing else. Database is
account-level by construction today (the branch tests for EC2Instance),
so the case exists to stop a refactor that enumerates the account-level
types explicitly from quietly dropping it.
Deferred to #1586: parseRecommendations logs and continues when one
recommendation detail fails to parse, so a combo whose best offer fails
quantity/cost parsing vanishes silently -- recreating within a combo the
"5 of 6 offers presented as all your options" defect the fan-out exists
to prevent. The parser is shared with the scheduler's discovery sweep,
which legitimately wants the tolerant behaviour, so the fix needs a
per-caller policy and does not belong on this branch.
|
Pushed Fixed1. Azure subscription-ID case could fork purchase identity (double-spend).
Same defect family as Both Azure uses of the subscription ID -- the idempotency scope in
2. 3. Account-level Savings Plan region guard now covers Deferred -- #1586
Gates
Pre-commit hooks ran clean on the commit (no @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit summary and file list ---'
git show --format=fuller --stat --find-renames 707cd14a0
git diff-tree --no-commit-id --name-status -r 707cd14a0
printf '%s\n' '--- changed hunks ---'
git show --format= --find-renames 707cd14a0
printf '%s\n' '--- symbol outlines ---'
for file in \
mcp/tools/azure_compute_ri.go \
mcp/tools/azure_compute_ri_test.go \
mcp/tools/aws_savingsplans_test.go \
mcp/tools/search_recommendations.go
do
if [ -f "$file" ]; then
echo "### $file"
ast-grep outline "$file" --items all --type function,method --view compact || true
fi
done
printf '%s\n' '--- identity and canonicalization references ---'
rg -n -C 4 \
'azureCredentialScope|CredentialScope\(|AzureSubscriptionID|idempotencyKeyFor|TestAzureSubscriptionCaseCannotForkIdempotencyToken|TestAWSProfileCaseIsPreserved|SPTypeDatabase|Database' \
mcp/tools README.md mcp/README.md 2>/dev/null || true
printf '%s\n' '--- deferred issue ---'
gh issue view 1586 --repo LeanerCloud/CUDly --json number,title,state,body,urlLength of output: 112131 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Azure provider subscription resolution ---'
ast-grep outline providers/azure/provider.go --items all --type function,method --match 'resolveAzureSubscriptionID|NewProvider' --view signatures
rg -n -A90 -B20 'func resolveAzureSubscriptionID|func NewProvider|AZURE_SUBSCRIPTION_ID|AzureSubscriptionID' \
providers/azure/provider.go
printf '%s\n' '--- Azure MCP purchase tool inventory and registration ---'
rg -n -C 3 \
'NewAzure|ProviderAzure|AzureSubscriptionID|azure_compute_ri_purchase|cudly_azure' \
mcp/tools mcp/server.go
printf '%s\n' '--- exact test execution path and environment use ---'
rg -n -A100 -B15 \
'func azurePurchaseIdentity|func TestAzureSubscriptionCaseCannotForkIdempotencyToken|func \(t \*azureComputeRIPurchaseTool\) handle|func \(t \*azureComputeRIPurchaseTool\) resolveClient' \
mcp/tools/idempotency_scope_test.go mcp/tools/azure_compute_ri.go
printf '%s\n' '--- diff integrity ---'
git diff --check 707cd14a0^ 707cd14a0Length of output: 51206 🤖 web_search executed:
Length of output: 6806
🐇✅ ✏️ Learnings added
✅ Action performedReview finished.
|
Summary
Implements the CUDly MCP server (#1488): a local MCP server exposing CUDly's RI/SP/CUD search and purchase surface across AWS, Azure, and GCP to any MCP client. Supersedes the design-only PR that previously occupied this slot; the design doc (
docs/design/mcp-server.md) has been removed and its findings folded into code comments and the newmcp/README.md.What landed (fully wired, real-purchase capable)
cudly_list_commitment_actions— catalog of every tool, generated from each tool's own descriptor so it can never drift from what's registered.cudly_search_recommendations— read-only wrapper overProvider.GetRecommendationsClient().GetRecommendations().cudly_aws_ec2_ri_purchasecudly_aws_opensearch_ri_purchase,cudly_aws_redshift_ri_purchase,cudly_aws_memorydb_ri_purchasecudly_aws_rds_ri_purchase,cudly_aws_elasticache_ri_purchasecudly_aws_savingsplans_purchasecudly_azure_compute_ri_purchasecudly_gcp_computeengine_cud_purchaseAll nine purchase tools share one safety gate (
mcp/tools/purchase.go):dry_rundefaultstrueand never contacts the cloud provider; a real purchase requiresdry_run=false AND confirm=true, otherwise a structured error is returned (never a silent no-op). Every real purchase is stamped with a newcommon.PurchaseSourceMCPenum value (never a caller-suppliable string) and a deterministic idempotency token, so a retried identical call dedupes at the provider instead of double-buying.Findings that changed scope from the design doc
PurchaseCommitmentsignature) was a stale local-tree artifact — verified againstorigin/main: every service client (AWS EC2/RDS/ElastiCache/Redshift/MemoryDB/OpenSearch/SavingsPlans, Azure compute, GCP computeengine) already has the correct signature. No PR-0 was needed.DoIdempotentPurchaseTwoStep+FindReservationOrderByIdempotencyToken(issue bug(providers/azure): DoPurchaseTwoStep dropped IdempotencyToken threading; reintroduces double-purchase risk (regression of #641; blocks #639) #721), and GCP computeengine derives both its commitment name and nativeRequestIdfrom the token (issue feat(purchases): make GCP Compute commitment creation idempotent (remaining slice of #641) #654). Re-verified against the committed client code before enabling real purchases for both, rather than shipping them dry-run-only as originally planned.Nothing deferred to dry-run-only
Every provider ended up real-purchase capable. Two pre-existing gaps were flagged (not fixed, out of scope) in the affected tool's description and
mcp/README.md:billingPlanType, so every Azure VM RI purchase uses Azure's default (upfront) billing plan regardless of thepayment_optionrequested — it only affects the displayed cost estimate.cudly_gcp_computeengine_cud_purchasetakesvcpu_count/memory_gbdirectly.Verification
go build ./...,go test ./...(6087+ tests across 41 packages),go vet ./...all green at every commit.gocyclo -over 10clean on every new file.golangci-lintv2.10.1 (matchingci.yml, not the newer local default) run explicitly: 0 issues, repo-wide.gosecclean on every commit via the pre-commit hook.mcp/server_test.go) drives the real MCP protocol over an in-memory transport: connects a real client to the real server, lists tools, and callscudly_aws_ec2_ri_purchasewithdry_runomitted, proving the default-true dry-run path returns structured cost JSON with zero AWS credentials configured and zero provider calls made.Closes #1488
Summary by CodeRabbit
cudly-mcp) with discoverable tools for recommendation search and preview-to-execute purchases across AWS, Azure, and GCP, plus a commitment-action catalog.