feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, GCP - #1495
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
docs/mcp-server-design
branch
from
July 23, 2026 00:01
8b88ce3 to
6581b27
Compare
cristim
changed the title
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ 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. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
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
Member
Author
|
@coderabbitai full review |
Contributor
✅ 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. |
Member
Author
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.
|
Contributor
|
🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
Contributor
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.
Member
Author
|
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 |
Contributor
🧩 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.
|
Member
Author
|
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 |
Contributor
|
✅ 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. |
Member
Author
|
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 |
Contributor
|
✅ 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. |
Member
Author
|
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 |
Contributor
|
✅ Action performedFull review finished. |
Contributor
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
cristim
added a commit
that referenced
this pull request
Jul 28, 2026
#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.
Member
Author
|
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 |
Contributor
🧩 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.
|
This was referenced Jul 28, 2026
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.
Member
Author
|
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 -- LeanerCloud/cloud-commitments-go#54
Gates
Pre-commit hooks ran clean on the commit (no @coderabbitai review |
Contributor
🧩 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.
|
Member
Author
|
Re-requesting review: the latest commit on this PR postdates your last review, so that review does not cover the current HEAD. Fixes landed since then were verified locally (build, tests, vet, gocyclo, and golangci-lint at the CI-pinned version), and CI on this PR is green across all workflows. Please review the current HEAD. @coderabbitai review |
Contributor
|
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 28, 2026
An Azure exchange mutates two sets of reservations: the ones handed back (sources) and the ones acquired (targets). Only the targets reached the Regions dimension of the execute:ri-exchange constraint check, so a caller permitted solely in eastus could POST an exchange whose source was their own subscription's westeurope Standard_M128 3-year reservation and whose target was a single eastus reservation. targetLocations returned ["eastus"], the region constraint passed, requireAzureSourceOwnership passed (the westeurope reservation IS billed to that subscription, and that gate keys on BillingScopeID rather than region), and the exchange committed: a westeurope commitment the caller was never authorized to touch was consumed and relocated to eastus, irreversibly. No other gate covered it -- ExchangeableReservation.Region was consulted by nothing. The AWS analog is safe only because AWS exchanges are same-region; cross-region is this feature's stated purpose. exchangeRegions now folds each source reservation's own region into the same normalized, de-duplicated set as the target locations, so every region the operation touches must be permitted. Source regions come from the tenant listing rather than the request body, which names only reservation ids; the listing is fetched once in authorizeAzureExchangeExecution and reused by requireAzureSourceOwnership so both source-side gates judge the same data. That listing now precedes the constraint check, since the Regions dimension cannot be assembled without it. A source whose Region Azure did not report (documented as possible for AppliedScopeType == Shared) or that is absent from the listing contributes an unknown-region sentinel instead of being dropped: dropping it would make "we do not know where this is" mean "unconstrained", the fail-open shape fixed in #1495. A permission with no Regions constraint is unaffected, matching unattributedAccountConstraint's posture on the AccountIDs dimension. Regression coverage asserts the security property rather than the constraint set as built: an eastus-only caller is denied a westeurope source, an unreported source region denies, and the sentinel does not block an unscoped caller. The first two fail against the pre-fix call site by reaching CalculateExchange.
cristim
added a commit
that referenced
this pull request
Jul 28, 2026
…ge (closes #596) (#1515) * feat(azure/compute): CalculateExchange and DoExchange client operations Adds CalculateExchange (price/preview + compatible offerings) and ExecuteExchange (commit) as thin wrappers over the armreservations CalculateExchange/Exchange LRO APIs, with an injected caller-func test seam so the async polling never has to run for real in tests. Fail-loud validation throughout: no quantity coercion, no default reservation term, and a nil-Properties/empty-SessionID response is an explicit error rather than a fabricated empty preview. Money fields (NetPayable, RefundsTotal, PurchasesTotal, BillingCurrencyTotal) are pointers so an absent amount is never read as free. Refs #473, closes #596 (client half; API handlers follow). * feat(api): Azure RI exchange compatible-offerings and execute endpoints Adds POST /api/ri-exchange/azure-instances/compatible-offerings (view:purchases) and POST /api/ri-exchange/azure-instances/exchange (execute:ri-exchange). The execute handler never trusts a client-supplied session: it re-runs CalculateExchange itself against the caller's sources/targets and executes only the fresh SessionID that call returns. Execution is refused on any policy error, a nil net payable, a currency mismatch, or a net payable that exceeds the caller's mandatory max_payment_due cap -- each guardrail is checked against the server's own re-quote, not anything the client sent. Constraint enforcement (execute:ri-exchange) carries AccountIDs from the resolved CloudAccount, Providers/Services fixed to azure/compute, every target region, and MaxPurchaseAmount from the cap, matching the AWS executeExchange precedent (SEC-01, issue #1141). Widens azureExchangeClient with CalculateExchange/ExecuteExchange; extends the existing stub to match. Closes #596 (API half; client operations landed separately). * test(api): Azure exchange handler coverage Table-driven coverage for the compatible-offerings and execute handlers: auth fail-closed (no session, missing execute:ri-exchange, constraint denial), every validation reject, and each execute money-path guardrail (cap exceeded, policy errors, currency mismatch, nil net payable) proven via a mock CalculateExchange result with ExecuteExchange asserted never called. The happy path asserts ExecuteExchange receives exactly the SessionID the CalculateExchange mock returned, proving the server-re-quote wiring end to end. Also folds in the golangci-lint (v2.10.1) findings this branch introduced: two err-shadow fixes in executeAzureExchange (match the existing executeExchange plain-assignment style instead of `if err := ...`), two godot comment-period fixes, and two gocritic nits in exchange_operations.go (index-based range to avoid a large struct copy; named results on extractPrice). Refs #596. * fix(api): reject currency-blind MaxPurchaseAmount cap on Azure execute MaxPurchaseAmount permission constraints are USD-denominated (matching the AWS execute:ri-exchange precedent), but the Azure execute handler was feeding the raw requested amount into the constraint check regardless of body.Currency. A non-USD request (e.g. 1000 KWD, worth far more than 1000 USD) could clear a cap meant to bound USD spend, since the comparison was a plain float check with no currency awareness. There is no FX conversion available here, so a non-USD amount can never be safely compared against the cap. Fail closed instead: a non-USD request is checked with an unmatchable sentinel amount, denying it whenever the granting permission carries any MaxPurchaseAmount constraint, while still allowing it through when the permission has no amount constraint at all. A second disambiguation call (amount neutralized) distinguishes "the cap blocked this" from "some other constraint dimension blocked this" so the error message stays accurate. Also adds a defensive nil check in checkAzureExchangeMoneyGuardrails so a future azureExchangeClient implementation returning a nil preview cannot panic the handler instead of erroring. Both guards verified to fail their regression test when removed (checked by hand, reverted before committing). * fix(api): drop dead action parameter from requirePermissionConstraints golangci-lint's full-tree run (CI's Lint Code job, no --new-from-rev) flagged `action` as always receiving "execute" across every call site: my currency- blind-cap fix added a second Azure call site alongside the existing AWS execute:ri-exchange and execute:purchases call sites, all three literal "execute". A parameter with only one real value across every caller is dead flexibility, so the parameter is removed and the action is hardcoded as requirePermissionConstraintsAction rather than threaded through four call sites for a value none of them vary. No behavior change: every call site already passed "execute". * fix(azure/compute): refuse exchange results with a failed terminal status CalculateExchange and ExecuteExchange only treated a non-nil result.Error as failure. Azure's contract documents Error as "required if status == failed or status == canceled", but a response that violates it (Failed/Cancelled with a nil Error) was reported as a success: the execute handler returned HTTP 200 with status "Failed" and logged "azure ri-exchange executed", giving the caller no signal that the exchange did not happen. On the quote side a Failed/Cancelled response carrying a SessionID was handed straight to ExecuteExchange, which commits whatever session the fresh quote returned. Assert the terminal status when Azure populates it: - CalculateExchange requires Succeeded. - ExecuteExchange accepts Succeeded, PendingPurchases and PendingRefunds (the swap is committed, one leg still settling) and refuses everything else, including statuses this SDK version does not know. An unrecognized post-commit status is genuinely ambiguous, so the error tells the operator to verify in the portal rather than retry into a possible double exchange. A nil status leaves the pre-existing guards (SessionID presence, non-nil Properties) in charge rather than inventing a failure Azure never reported. Regression tests cover both refused statuses and both accepted pending statuses; the refusal tests fail against the pre-fix code. * fix(api): derive azure exchange billing scope from the authorized subscription targets[].billing_scope_id was a required, caller-supplied field that went straight through to the purchase, while authorization was checked against the request's subscription_id. The execute:ri-exchange AccountIDs constraint is evaluated against the CloudAccount registered for subscription_id, so a caller could pass their own subscription_id to satisfy the constraint and then name a different subscription's billing scope in targets[], moving the charge outside the account whose permissions were actually verified. Derive the billing scope from subscription_id instead, matching every other Azure reservation purchase path in this repo (ComputeClient .buildReservationBody and the database / cache / search / cosmosdb / synapse / managedredis clients all build the scope from their own subscription ID). billing_scope_id becomes optional, and is rejected when supplied with anything other than the request subscription's own scope rather than silently ignored, so a caller who believed they were directing the charge elsewhere is told they were not. Covers both the offerings and execute endpoints, with the openapi schema updated to match. * test(api): split azure exchange tests out and fix a misplaced doc comment Addresses two CodeRabbit nitpicks on the Azure RI exchange tests. The doc comment describing TestCheckAzureExchangeMoneyGuardrails_NilPreviewRefused sat above TestCheckAzureExchangeMoneyGuardrails_CurrencyCaseInsensitive, so both comments stacked over the wrong test and the nil-preview rationale appeared to explain the currency rule. Move it to directly precede the test it documents; neither test body changes. Move the Azure compatible-offerings, execute, money-guardrail and subscription-scoping suites into handler_ri_exchange_azure_test.go in the same package. handler_ri_exchange_test.go had grown to 2788 lines against the project's 500-line guideline, and the Azure block was a self-contained ~1000-line section with its own mocks and helpers. Same package, so shared setup keeps working unchanged. No test was added, removed or altered: 86 test functions before, 86 after. * fix(api): check Azure exchange subscription scope before building client authorizeAzureExchangeExecution built the Azure client (buildAzureExchangeClient) before checking requireAzureSubscriptionScope, the reverse of the sibling getAzureCompatibleOfferings handler. For a scoped session this let an unregistered subscription_id short-circuit with buildAzureExchangeClient's distinguishable 404 ("no Azure account registered for subscription %q") before the scope check ever ran, while a registered-but-out-of-scope subscription reached the generic errNotFound instead -- an enumeration oracle letting a scoped caller learn which subscription IDs exist in the tenant. Building the client first also meant credentials for an out-of-scope account could be resolved before the request was denied. Reorders the check to run first, matching getAzureCompatibleOfferings exactly: scope check, then client build. Three new tests: an unregistered subscription and a registered-but-out-of- scope one now produce the identical generic errNotFound with no subscription-id echoed back, and a client_secret account with no stored secret proves no credential resolution is attempted before the scope denial (reverting the reorder makes all three fail: one on the distinguishable message, one on the credential-resolution side effect never getting to the scope check's own auth call, and one on the two error messages no longer matching). * fix(azure/compute): never drop a message-less exchange policy error extractExchangePreview only appended a policy violation when Azure populated its Message field. armreservations.ExchangePolicyError has two optional pointer fields, so Azure may report a violation as a bare Code. The execute handler gates solely on len(ExchangePreview.PolicyErrors) > 0, so a Code-only violation emptied the slice and let a policy-rejected exchange be committed for real money. Every entry now renders to a non-empty string: "Code: Message" when both are present, otherwise whichever is set, otherwise an explicit unspecified-violation marker. One reported violation always yields exactly one entry. * test(api): pin the azure exchange authz set and commit-path failures The execute suite matched the SEC-01 constraint argument with mock.Anything or an amount-only MatchedBy, so four of the five dimensions that confine an irreversible exchange were asserted nowhere: dropping Regions, or pointing AccountIDs at the wrong CloudAccount, left the whole suite green. targetLocations was entirely unexercised. Adds, each verified to fail against a deliberately mutated handler: - the full constraint set (AccountIDs from the request's own subscription_id, azure/compute, de-duplicated target Regions, cap) - lowercase "usd" through the handler, pinning the isUSD EqualFold that the direct-call guardrail test cannot reach - the disambiguation branch where another dimension is the real denial cause, which must not be reported as a currency problem - a failed re-quote aborting before ExecuteExchange is reached - 400-vs-500 classification on the commit call itself - cap boundary (exactly at cap, one cent over) and negative NetPayable, the refund side of a downgrade - case-variant billing_scope_id accepted, status codes on the missing guardrail validations * docs(api): document the azure exchange sources/targets item bounds The handler rejects empty arrays and more than maxAzureExchangeItems (50) sources or targets, but the spec advertised unbounded arrays, so clients only discovered the limit from a 400. Adds minItems/maxItems to both the compatible-offerings and execute request bodies. * test(api): guard the offerings endpoint against the enumeration oracle The execute endpoint has three tests pinning that requireAzureSubscriptionScope runs before buildAzureExchangeClient. The offerings endpoint had none: its only scope test injects an azureExchangeFactory, so the client build always succeeds and swapping the two calls would still yield errNotFound. Exercises the real buildAzureExchangeClient path for both an unregistered and a registered-but-out-of-scope subscription, and requires the two denials to be byte-identical, so a scoped caller cannot probe which subscription ids exist. Verified to fail against a handler with the two calls swapped. * docs(api): give the azure exchange sources/targets real schemas Both request bodies declared items as a bare `type: object`, so the contract exposed none of the fields the handler requires and generated clients could not construct or validate a payload. In particular billing_scope_id was described in prose but absent from the schema. Adds reusable AzureExchangeSource / AzureExchangeTarget component schemas referenced from both endpoints, with types, requiredness, minimums, and the caveat that billing_scope_id is not the scope that gets charged. The term enum is P1Y/P3Y/P5Y, matching what azureReservationTermFromString actually accepts (the SDK's own PossibleReservationTermValues), not the narrower set VM reservations are sold under -- Azure decides that per resource type and reports it as a policy error. Corrects an ExchangeTarget doc comment that claimed the SDK only had P1Y and P3Y. * docs(azure/compute): correct the documented reservation term set Both Term fields claimed the ISO 8601 term is "P1Y" or "P3Y". The values are stringified from armreservations.PossibleReservationTermValues(), which also includes P5Y in this SDK version, so Azure can return a compatible offering or an existing reservation carrying a term the comment says cannot occur. A consumer trusting it would treat a valid term as unsupported. Also states that the one/three-year pair is not the boundary: which terms Azure sells is per resource type and is Azure's call, surfaced as a policy error rather than filtered here. * fix(api): scope azure exchange sources to the authorized subscription Closes #1527. Every gate on these endpoints constrained the DESTINATION of an exchange: allowed_accounts scope, the execute:ri-exchange AccountIDs constraint, and the derived target billing scope all key off subscription_id. The sources were validated only for a non-empty reservation_id and quantity >= 1. Azure reservation orders are tenant-scoped, so ListExchangeableReservations enumerates the whole tenant by design. A caller authorized for subscription A could therefore name subscription B's reservation ids and hand B's commitments back, buying the replacement into A's billing scope, with Azure RBAC on the reservation order as the only backstop. Each source must now be billed to the authorized subscription. The check uses the reservation's own BillingScopeID, which Azure documents as the subscription charged for it and which is the scope an exchange refunds it to. That is the right discriminator even for AppliedScopeType Shared, which governs which subscriptions get the discount rather than which one paid; an AppliedScopes check would pass for nearly every reservation. Fails closed throughout: a source missing from the listing, one Azure reports without a billing scope, and a failed listing call are all refused rather than permitted. Denials are byte-identical whether the reservation belongs to someone else or does not exist, so the gate cannot be used to enumerate reservation ids elsewhere in the tenant. Applied to the pricing endpoint too, where the same gap leaks another subscription's commitment value. parseAzureExecuteRequest is extracted purely to keep executeAzureExchange within the gocyclo limit the new gate pushed it over; it makes no decisions of its own. * style(api): fix lint on the source-ownership gate golangci v2.10.1 on the root module flagged two issues in the new code: misspell on "theatre" in a doc comment, and gocritic rangeValCopy on the ownership map build (160 bytes copied per iteration). Index instead. * fix(auth/api): require every requested region to be permitted The Regions dimension of a permission constraint was matched with containsAny, the same any-overlap rule the other dimensions use. That rule is safe for AccountIDs, Providers and Services because a request names exactly one value on each of them, so any-match and all-match coincide. Regions is the one dimension a single request legitimately spans several values on: an Azure RI exchange submits every target's location at once via targetLocations. The result was an authorization bypass on a money path. A caller permitted only in eastus could attach a westus target to the exchange; containsAny found eastus in the permitted set, returned true, and the irreversible exchange executed for BOTH regions. "Regions limits to specific regions" cannot mean "limits to requests that mention at least one permitted region". matchAllRegionsConstraint now backs that dimension: every requested region must be permitted, comparison is case- and whitespace- insensitive, and the empty-list semantics are unchanged (an unconstrained permission, or a request naming no region, still matches). Every other dimension keeps containsAny. Single-region callers (the AWS reshape execute path, purchaseConstraintSets) are unaffected: for a one-element request all-match and any-match are the same test, and the case-insensitivity only widens what they match. Alongside it, targetLocations canonicalizes each location to trimmed lower case before de-duplicating, so the constraint set names each target region exactly once instead of demanding a permission for two spellings of one place ("EastUS", the casing the Azure portal shows, and "eastus", the casing its APIs return). Target validation now rejects a whitespace-only location as well, which previously survived the empty check and would reach the permission check as "", a region no permission can name. Regression tests cover the bypass through both matchConstraints and the real HasPermission entry point, the case and whitespace handling on both sides, and the targetLocations normalization both directly and through executeAzureExchange's submitted constraint set. Each fails against the pre-fix code. Refs #596 * fix(api/azure): bound RI exchange sources by the Regions constraint An Azure exchange mutates two sets of reservations: the ones handed back (sources) and the ones acquired (targets). Only the targets reached the Regions dimension of the execute:ri-exchange constraint check, so a caller permitted solely in eastus could POST an exchange whose source was their own subscription's westeurope Standard_M128 3-year reservation and whose target was a single eastus reservation. targetLocations returned ["eastus"], the region constraint passed, requireAzureSourceOwnership passed (the westeurope reservation IS billed to that subscription, and that gate keys on BillingScopeID rather than region), and the exchange committed: a westeurope commitment the caller was never authorized to touch was consumed and relocated to eastus, irreversibly. No other gate covered it -- ExchangeableReservation.Region was consulted by nothing. The AWS analog is safe only because AWS exchanges are same-region; cross-region is this feature's stated purpose. exchangeRegions now folds each source reservation's own region into the same normalized, de-duplicated set as the target locations, so every region the operation touches must be permitted. Source regions come from the tenant listing rather than the request body, which names only reservation ids; the listing is fetched once in authorizeAzureExchangeExecution and reused by requireAzureSourceOwnership so both source-side gates judge the same data. That listing now precedes the constraint check, since the Regions dimension cannot be assembled without it. A source whose Region Azure did not report (documented as possible for AppliedScopeType == Shared) or that is absent from the listing contributes an unknown-region sentinel instead of being dropped: dropping it would make "we do not know where this is" mean "unconstrained", the fail-open shape fixed in #1495. A permission with no Regions constraint is unaffected, matching unattributedAccountConstraint's posture on the AccountIDs dimension. Regression coverage asserts the security property rather than the constraint set as built: an eastus-only caller is denied a westeurope source, an unreported source region denies, and the sentinel does not block an unscoped caller. The first two fail against the pre-fix call site by reaching CalculateExchange. * fix(api/azure): check exchange source ownership before the constraints Folding source regions into the Regions dimension put the constraint check ahead of requireAzureSourceOwnership, and the two together formed an enumeration oracle. A caller scoped to subscription A and permitted only in eastus received two distinguishable 403s for a reservation id they do not own: an id that exists in eastus but is billed to subscription B cleared the Regions dimension and was refused by the ownership gate, while an id that does not exist -- or lives in an unpermitted region -- tripped the sentinel or the foreign region and was refused by the constraint check with a different message. The difference confirms "this reservation id exists, in one of my permitted regions, in a subscription I am not scoped to", which is exactly what requireAzureSourceOwnership's deliberately identical denials withhold. Ownership now runs immediately after the tenant listing and before the constraint check, so the whole path is as indistinguishable as the gate already was. The listing is in hand at that point, so there is no extra round trip, and executeAzureExchange no longer needs the listing handed back to it. It also sharpens the sentinel: every source reaching exchangeRegions is known-owned, so unknown-region means only "owned, but Azure reported no region" rather than doubling as "not yours" or "not real". The missing-source branch stays as a fail-closed default for any future caller without that guarantee. The existing TestRequireAzureSourceOwnership_DenialsAreIndistinguishable stays green with this bug present, because it calls the gate directly rather than driving the path. The new test drives the handler and asserts both probes return the same status and body; it fails against the previous ordering with the two messages diverging.
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
…, GCP (#1495) * docs(mcp): add architecture blueprint for CUDly MCP server 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 * docs(mcp): remove architecture blueprint doc Design work is complete; implementation follows in this same PR. * feat(common): add cudly-mcp purchase source enum 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. * feat(mcp): server skeleton + list-commitment-actions tool 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. * feat(mcp): search-recommendations tool 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. * feat(mcp): aws ec2 ri purchase tool 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. * feat(mcp): aws opensearch, redshift, memorydb ri purchase tools 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. * feat(mcp): aws rds and elasticache ri purchase tools 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). * feat(mcp): aws savings plans purchase tool 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. * feat(mcp): azure vm and gcp compute engine purchase tools 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. * docs(mcp): add mcp/README.md 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. * fix(mcp): resolve golangci-lint v2.10.1 findings 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. * test(mcp): add end-to-end search-then-dry-run-purchase test 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. * fix(mcp): derive idempotency key from every price-affecting dimension 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. * fix(mcp): omit unknown purchase cost/savings instead of reporting 0 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. * fix(mcp): reject Azure payment_option Azure will not honor 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. * fix(mcp): scope Azure payment_option gate to real purchases only 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. * feat(mcp): expose include_regions/exclude_regions on search_recommendations 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. * fix(mcp): validate Database Savings Plan term/payment constraints 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. * fix(mcp): properly case product names in simple RI tool descriptions 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. * docs(mcp): use go install instead of a root build artifact `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. * fix(mcp): capture decidePurchaseMode error in azure RI purchase gate 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. * docs: link to MCP server docs from root README Add a short pointer section so the root README surfaces the MCP server alongside the CLI reference instead of leaving it undiscovered. * fix(azure): wire payment option into reservation billing plan 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. * fix(mcp): accept and default to no-upfront on Azure RI purchase tool 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. * docs(mcp): update Azure billing-plan caveat to reflect no-upfront support 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. * fix(mcp): register provider factories for cudly-mcp binary 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). * fix(mcp): prevent idempotency-key collisions across time-separated purchases 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. * fix(mcp): require instance_family for EC2Instance Savings Plans 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. * fix(mcp): include cudly_list_commitment_actions in its own catalog 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. * fix(mcp): reject whitespace-only required string fields A bare `== ""` check on region/instance_type/vm_size/machine_type/etc lets a whitespace-only value like " " through to provider resolution on a confirmed real purchase, since it is neither empty nor a value the enum validators would catch. Add a requireNonBlank helper that trims before checking, and use it at every plain (non-enum) required-string check across the AWS EC2, RDS, ElastiCache, and simple RI tools, the AWS Savings Plans EC2Instance region/instance_family checks, and the Azure and GCP purchase tools. Enum-typed fields (payment_option, engine, term, etc.) already reject a whitespace-only value via their own allow-list check and are unaffected. Add whitespace-only regression cases to each tool's existing missing-required-field table tests. * fix(mcp): populate TermYears on preview and real purchase responses PurchaseResponse.TermYears was declared in the JSON contract but never set in either branch of ExecutePurchase, so it was always zero and omitted (omitempty) even though the term is known from the request: every *FromArgs constructor in this package writes it into Recommendation.Term in the "<N>yr" format via TermYears.RecommendationTerm(). Add termYearsFromRecommendationTerm to parse that format back into an int and populate TermYears in both the preview and real-purchase response construction. * docs(mcp): clarify GOBIN vs GOPATH/bin binary path guidance 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. * style(mcp): fix golangci-lint v2.10.1 findings 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). * fix(mcp): make purchase idempotency key fail-safe (drop auto time-bucket) Commit 2390f07c0 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. * fix(mcp): drop partial-upfront from Azure RI payment_option schema 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). * fix(mcp): trim whitespace-only region before Savings Plans account-level 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. * fix(mcp): scope Savings Plans Details.Region/InstanceFamily to EC2Instance 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. * fix(azure): validate payment option before Microsoft.Capacity registration 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. * fix(mcp): trim surrounding whitespace on required identifier fields 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. * fix(mcp): validate lookback_period in the search_recommendations handler 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. * fix(azure): validate term before capacity provider registration 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. * fix(mcp): trim region/instance_family before storing on SP recommendation 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. * fix(mcp): trim region/account identifiers in search_recommendations 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. * fix(azure): wire billingPlan into SQL Database reservation purchases Azure SQL Database reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/azure-sql/database/reservations-discount-overview). buildReservationBody never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502. * fix(azure): wire billingPlan into Cache for Redis reservation purchases Azure Cache for Redis reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/azure-cache-for-redis/cache-reserved-pricing). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502. * fix(azure): wire billingPlan into Cosmos DB reservation purchases Azure Cosmos DB reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/cosmos-db/reserved-capacity). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502. * fix(azure): wire billingPlan into Search reservation purchases The Microsoft.Capacity calculatePrice/purchase API uses the same properties struct for every reservedResourceType, and Azure's monthly- payments doc (learn.microsoft.com/azure/cost-management-billing/reservations/ prepare-buy-reservation) excludes only SUSE Linux, Red Hat plans, Azure Red Hat OpenShift, and pre-purchase plans from Monthly billing, so Search is covered the same as every other sibling service. PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. This is independent of the pre-existing "SearchService" reservedResourceType literal being unverified against the live catalog (issue #1189). Part of #1502. * fix(azure): wire billingPlan into Managed Redis reservation purchases Azure Managed Redis reservations support both Upfront and Monthly billing frequency (learn.microsoft.com/azure/redis/reserved-pricing). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502. * fix(azure): wire billingPlan into Synapse reservation purchases Azure Synapse Analytics Dedicated SQL pool (SQL DW) reserved capacity supports both Upfront and Monthly billing (learn.microsoft.com/azure/ cost-management-billing/reservations/prepay-sql-data-warehouse-charges). PurchaseCommitment never set properties.billingPlan, so a no-upfront recommendation was silently billed at Azure's Upfront default. Map rec.PaymentOption via the shared reservations.BillingPlanForPaymentOption helper (added in #1495 for compute) and fail loud on empty/unrecognized values instead of defaulting. Part of #1502. * fix(mcp): default Savings Plans search to 1yr no-upfront 30d 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". * fix(mcp): join all invalid search fields into one error 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. * docs(mcp): document Savings Plans search defaults 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. * fix(aws): filter recommendations by the region search param 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. * fix(aws): stop region filter from dropping all savings plans recs Savings Plans recommendations never populate the top-level Recommendation.Region: account-level plans (Compute/SageMaker/Database) carry no region at all, and EC2Instance plans carry their region in Details.Region instead. filterByIncludedRegions/filterByExcludedRegions matched only the top-level field, so any region or include_regions search silently dropped every Savings Plans recommendation, even when matching ones existed. Add effectiveRegion() to fall back to Details.Region for EC2Instance plans and treat a still-empty region as region-agnostic (always kept by an include filter, never dropped by an exclude filter) rather than "belongs to no region". Also normalize Details.Region at the parser (extractEC2SPFields) so it compares against the same canonical region codes the include/exclude sets use, matching the reservation parsers. * test(aws): cover savings plans recs in region filter tests TestApplyRecommendationFilters_Region only used fixtures with a populated top-level Region, so it could not catch a region filter silently dropping Savings Plans recommendations (which never populate that field). Add TestApplyRecommendationFilters_SavingsPlanRegion with an account-level SP rec (Region and Details.Region both empty) and an EC2Instance-style SP rec (Region empty, Details.Region set), asserting both include and exclude region filters behave correctly for each. * fix(mcp): scope purchase idempotency key to the target account The MCP idempotency key was derived only from product dimensions (provider, region, service, resource type, count, term, payment option, Details). Those describe WHAT is bought, never WHERE it lands, so two purchases identical in every product dimension but billed to different accounts derived the same token. On Azure that silently skips a real purchase rather than merely being imprecise. reservations.FindReservationOrderByIdempotencyToken lists reservation orders from the tenant-wide endpoint (ReservationOrdersListURL carries no subscription prefix, and its comment relies on the token being "globally unique per (execution, rec)" -- true for the CLI/web paths whose tokens come from a UUID execution ID, not for the MCP path whose token is purely a function of the request). Buying the same VM reservation for a second subscription in the same tenant therefore matched the first subscription's order by token, short-circuited, and reported success without buying anything for the second subscription. Add PurchaseRequest.CredentialScope, folded into idempotencyKeyFor, and populate it in all nine purchase tools via the new CredentialScope() helper: the caller-supplied override (aws_profile / azure_subscription_id / gcp_project_id) when present, otherwise the ambient environment variable the matching provider factory itself consults. AWS (per-account tag/ClientToken lookups) and GCP (per-project commitment names) scope their own dedupe, so this is defense in depth there and load-bearing on Azure. Regression tests cover the key-level distinction, the end-to-end threading into the token the provider actually dedupes on, and CredentialScope's override/environment/whitespace precedence. * fix(aws): stop region-less reservation recs bypassing the region filter The Savings Plans region fix earlier in this branch exempted a recommendation from both region filters whenever its effective region was empty, so that account-level Compute/SageMaker/Database plans (which carry no region at all) survive a region constraint. That predicate is too broad: an empty region is not by itself proof that a rec is region-agnostic. Every reservation parser in parser_services.go assigns rec.Region only under `if <svc>Details.Region != nil`, so an EC2/RDS/ElastiCache/OpenSearch/ Redshift/MemoryDB recommendation whose Cost Explorer payload omitted the region field reaches the filters with Region == "" while still being a single-region purchase. Exempting those let a recommendation of unknown region survive an explicit "us-east-1 only" filter and reach the purchase path, to be bought in whatever region the service client resolved. This filter is shared by the CLI, the web API, and the scheduler, so the blast radius was not limited to the new MCP search tool. Gate the exemption on CommitmentType == CommitmentSavingsPlan, which is what parser_sp.go sets and what "region-agnostic" actually means. A reservation rec with no region is once again dropped by an include filter (its region cannot be shown to match) and kept by an exclude filter (it cannot be shown to be excluded), the conservative direction each filter had before Savings Plans support was added. Also skip blank entries when building either filter's lookup set: a caller-supplied "" matches no real region code, and in the exclude filter it would otherwise drop every region-agnostic rec via a key that was never a region. Regression tests cover both, and the region-less-reservation cases fail against the empty-region-means-agnostic predicate. * fix(mcp): reject Savings Plans commitments finer than one cent providers/aws/services/savingsplans/client.go renders CreateSavingsPlanInput.Commitment with %.2f, so any hourly_commitment finer than a cent was silently rounded on the way to AWS: a requested $0.004/hour committed to "0.00" and a requested $10.005/hour committed to "10.01". The tool only checked > 0, so both were accepted and spent real money on a figure the caller never asked for -- exactly the silent coercion this money path must not do. Validate at the tool boundary that the caller's amount survives that same %.2f rendering unchanged, and fail loud naming the amount AWS would actually have been asked to commit. The comparison is against the rendered-then- reparsed value with a relative epsilon rather than an exact whole-cents test, because decimal literals like 0.07 are not exactly representable in float64 (0.07*100 is 7.000000000000001) and an exact test would reject most real commitments. This also keeps the idempotency key honest: idempotencyKeyFor folds in the full-precision HourlyCommitment, so two requests AWS would bill identically (10.001 and 10.004, both "10.00") previously derived different tokens and could purchase twice. * fix(mcp): reject blank filter entries and trim service in search Two input-hygiene gaps in cudly_search_recommendations, both of which returned a wrong answer instead of an error. A blank entry in include_regions/exclude_regions/account_filter cannot match any real region code or account ID, but a non-empty list still switches the corresponding filter on. include_regions=[" "] therefore activated region filtering with a set that matches nothing and returned zero recommendations, which a caller reads as "your account has nothing worth buying" rather than "your filter was malformed". Reject blank entries by name and index, joined with the other validation errors so a caller sees every problem at once. service was the one identifier field never trimmed, while region, include_regions, exclude_regions and account_filter all were. A padded " savingsplans-compute" both missed the AWS Savings Plans defaulting branch (isAWSSavingsPlansSearch matches on the raw value) and failed the supported-service check for a reason the error text did not explain. Trim it with the others, and move the whole trimming step ahead of validation so every check sees the normalized values the rest of the call actually uses. Extract the per-field validation into collectSearchFieldErrors to keep validateSearchArgs under the pre-commit gocyclo gate of 10. * fix(mcp): set the GCP CUD payment option instead of leaving it empty cudly_gcp_computeengine_cud_purchase built its Recommendation without a PaymentOption. GCP Compute Engine CUDs have exactly one billing schedule (monthly over the term, no upfront option), so the tool correctly exposes no payment_option argument, but an empty string is not neutral downstream: the offering-details switch in providers/gcp/services/computeengine/client.go has no case for "" and falls through to `default: upfrontCost = totalCost`, reporting the entire commitment as an upfront charge. Set it explicitly to "monthly", the sole value config.ValidPaymentOptionsByProvider["gcp"] recognises, and say why in the tool description so a caller does not go looking for the missing argument. * feat(mcp): log real purchases and document the missing approval gate A real MCP purchase left no record anywhere. The CLI path emits a common.AuditRecord per purchase (cmd/multi_service.go) and the web path persists a purchase_executions row that also carries the approval history, but the mcp/ tree contained no logging at all, so an operator asking "what did the assistant actually buy?" had nothing to read. Log an ATTEMPT line before the provider call and a matching OK/FAILED line after, recording provider, target account, region, resource, count, term, payment option, commitment ID and a masked idempotency token. Previews are deliberately not logged: they contact no provider and spend nothing, and logging every dry run would bury the real purchases. Output goes to the standard logger (stderr), never stdout, which the MCP stdio transport owns for JSON-RPC framing. Also document in mcp/README.md what this server does not provide, so the gaps are a deliberate choice rather than a surprise: no approval workflow (confirm=true is a guardrail against an accidental call, not an authorization control, and the model supplies it), no persisted audit record in CUDly's own purchase history, and credentials bounded only by what launched the process since the per-call account arguments are chosen by the model. * fix(azure): diagnose a missing payment option as missing BillingPlanForPaymentOption folded empty into the same error as an unrecognized value, so a purchase that failed because no payment option was supplied was told "partial-upfront has no azure equivalent". That is true and irrelevant, and it sends whoever reads the failure looking in the wrong place. Empty is a reachable state with its own remedy, not a typo: migration 000032 added recommendations.payment_option as TEXT NOT NULL defaulting to the empty string, so a row predating that migration reaches internal/purchase/execution.go with rec.Payment == "" and lands here. Give it a message that names the actual problem and says why a default is not applied for it (Azure's own default is upfront, which would charge the whole commitment immediately). Also correct ReservationOrdersListURL's doc comment. It asserted the idempotency token is "globally unique per (execution, rec)", stated as a property of the token itself; it is really a property the CALLER must provide, satisfied by the CLI/web paths only because they derive the token from a UUID execution ID. Spell out that a caller deriving a token from request parameters must fold in the target subscription, since this lookup is tenant-wide and would otherwise let two subscriptions in one tenant collide. * style(mcp): resolve golangci-lint v2.10.1 findings Three findings from the CI-pinned linter (v2.10.1, matching ci.yml, not the newer local default): a govet shadow on the named err return in validateSavingsPlanArgs, a misspell in the GCP payment-option comment, and a staticcheck S1021 split declaration in the new audit-logging test. The shadow fix uses a distinct variable name rather than `err =`, because plain reassignment trips gocritic's sloppyReassign in the same spot: with a named err return, only a differently-named local satisfies both linters. * test(aws): pin blank region skip and savings plans region normalization Addresses two CodeRabbit nitpicks. TestApplyRecommendationFilters_BlankRegionEntryIsNotAMatcher did not pin what it claimed. It used an account-level Savings Plan, which is region-agnostic, so filterByExcludedRegions short-circuits on isRegionAgnostic before the lookup map is ever consulted; the subtest passed with or without regionSet's blank skip. Switch the subject to a region-less reservation, which is NOT region-agnostic but still has an empty effective region, so it reaches the map lookup and is dropped by a "" key when the skip is absent. Verified the strengthened test fails with the skip removed. Add a regression test for extractEC2SPFields' region normalization. Cost Explorer sometimes returns a display name ("US East (N. Virginia)") rather than a region code, and downstream region filters compare against codes like "us-east-1", so an un-normalized value silently drops an EC2Instance Savings Plans recommendation from a search that explicitly asked for its region. * refactor(mcp): share one dry_run/confirm default resolver across tools Addresses a CodeRabbit nitpick. All seven purchase tools re-implemented the same block: default dry_run to true, default confirm to false, override each from its *bool when non-nil. EC2 had it as effectiveDryRunConfirm, typed to ec2RIPurchaseArgs so nothing else could reuse it; the other six inlined it. This is the gate that decides whether real money moves, so seven hand-copied versions were seven chances for one to drift. A copy that defaulted dryRun to false would silently turn an unconfirmed preview into a live purchase, and only review would catch it. Replace all seven with ResolveDryRunConfirm(dryRun, confirm *bool), taking the pointers directly so it is not tied to any one tool's args struct. Behavior is unchanged. The table-driven test covers the safety-critical row explicitly: confirm=true with dry_run omitted still resolves to a preview. * feat(mcp): offer Archera insurance after a purchase completes A completed MCP purchase now returns an `archera` block: the underutilization-insurance pitch, the signup link carrying CUDly attribution, the enrollment window in days, and both partnership disclosures. This mirrors what the CLI already prints after a real purchase (printArcheraPitch) and what the web UI shows in its post-approval modal, so a buyer going through MCP is offered the same coverage as one going through either other surface. Attached only when the purchase actually succeeded: never to a dry run and never to a failed purchase, since neither bought a commitment and neither started an enrollment window. Offering a 7-day window against a purchase that did not happen would be wrong on the facts. Both disclosures ship as first-class response fields rather than prose, because an MCP client renders this payload through a model: sending the signup link without the sponsorship and the works-fine-without-it facts alongside it would let a sponsored recommendation be presented as a neutral one. Tests assert the link never serializes without them. The link, window, disclosures and pitch move to pkg/common so the CLI and the MCP server read one definition instead of each carrying its own copy; cmd/multi_service_stats.go now sources its wording from there. The frontend keeps its TypeScript copy (it cannot import Go) and both sides point at each other. Note for follow-up, deliberately not changed here: internal/email/templates.go sends a different signup link (archera.ai/signup?mode=cudly) than the CLI and frontend (www.archera.ai/cudly). Reconciling them is a partnership/attribution question rather than a refactor, so the divergence is documented next to the constant instead of silently normalized. * fix(mcp): reject NaN and Inf hourly commitments in Savings Plans validation validateHourlyCommitment's `<= 0` check and %.2f-render-then-reparse trick both let NaN and +Inf slip through uncaught, contradicting the function's own comment that they were rejected. Add an explicit finite check so a non-finite hourly_commitment is refused with a clear error instead of reaching the AWS purchase call. * fix(mcp): log a provider-reported purchase failure as FAILED, not OK logPurchaseOutcome only checked the Go error, so PurchaseResult{Success: false, Error: nil} -- a provider that ran the call but reports it did not actually buy anything -- fell through to the "mcp purchase OK" audit line. Gate the OK line on result.Success explicitly so a failed purchase is never misrecorded as successful in the only audit trail this server keeps. * fix(mcp): add operator opt-in gate before executing real purchases The confirm flag on every purchase tool is model-supplied, not an authorization control: a prompt-injected or hallucinating model with ambient production credentials could execute a real purchase from a single tool call, with nothing on the operator's side to stop it. Add CUDLY_MCP_ENABLE_REAL_PURCHASES, checked at the single chokepoint in ExecutePurchase where a real (non-preview) purchase is about to resolve a provider client. Default off: only an exact "1" or "true" (case-insensitive) enables real purchases; anything else, including unset, refuses with an error naming the flag. Dry runs are unaffected either way. Document the gate in mcp/README.md. * fix(aws): route parser warnings to stderr, never stdout providers/aws/recommendations is linked into cmd/cudly-mcp, and the MCP stdio transport owns stdout for JSON-RPC framing. Two parser warnings used fmt.Printf, so they went to stdout: parser_ri.go parseRecommendations, per unparseable recommendation detail parser_sp.go the multi-plan-type path, per plan type whose CE call fails Either one fires during a cudly_search_recommendations call and injects a bare line of prose into the middle of the JSON-RPC stream, corrupting the protocol and breaking the client session. The SP path is not an exotic case: it triggers whenever one plan type fails while others succeed, e.g. Database SP being unavailable in an account. Both now use log.Printf, which writes to stderr like every other warning in these files. TestParseRecommendations_SkipsInvalidDetails already drove the RI path but only asserted the returned count, so it stayed green the whole time the bug was live. The new TestParseRecommendations_WarningsNeverGoToStdout asserts the property that matters by swapping os.Stdout for a pipe: verified failing before this change and passing after. * fix(aws): keep region-scoped EC2Instance SPs under region filters isRegionAgnostic exempted any CommitmentSavingsPlan with an empty effective region from both region filters. Only the account-level plan types (Compute, SageMaker, Database) belong to no region. An EC2Instance Savings Plan is region-scoped, and extractEC2SPFields yields Region == "" whenever Cost Explorer omitted SavingsPlansDetails or its Region field, since aws.ToString maps a nil pointer to "". So a region-scoped EC2Instance SP of unknown region survived an explicit "us-east-1 only" filter and could be purchased in whatever region the service client resolved. That is the same defect the CommitmentSavingsPlan gate just closed for reservations, one plan type over, and the function's own doc comment already described the intended rule as Compute/SageMaker/Database. The exemption now requires positive evidence that the plan is account-level: Details must be *SavingsPlanDetails and its PlanType must be recognised. Anything unknown (nil Details, a non-SavingsPlanDetails payload, an unrecognised plan type) stays region-scoped and is filtered conservatively, matching the direction the reservation case already took. Plan types are compared against the SDK's own sptypes.SavingsPlanType members rather than bare literals, so this vocabulary cannot drift from spPlanTypeDisplayString's. TestApplyRecommendationFilters_RegionlessEC2InstanceSPNotExempt covers the region-less EC2Instance SP under both filter shapes, the unrecognised and missing-Details cases, that account-level plans stay exempt, and that an EC2Instance SP carrying a real region is still matched on it. Verified failing on the CommitmentSavingsPlan-only gate and passing after. * fix(mcp): refuse a real purchase with no determinable account idempotencyKeyFor folds CredentialScope into the token, and CredentialScope returns "" when neither the explicit argument nor the ambient environment variable is set. So the SAME target account reached two ways derived two DIFFERENT tokens: omitting azure_subscription_id (ambient resolves to sub-X) gave "", while passing azure_subscription_id="sub-X" gave "sub-X". Every provider's dedupe is token-keyed, so the second call's lookup missed and bought a second commitment: Azure FindReservationOrderByIdempotencyToken GCP idempotentCommitmentName (token -> commitment name) AWS EC2/Redshift findRIByIdempotencyToken tag lookups, RDS/ElastiCache/OpenSearch/MemoryDB idempotencyGuard, Savings Plans CreateSavingsPlanInput.ClientToken Measured before the fix, one account, two call shapes: explicit sub-X -> token=ab1016c03e7b5928 purchases=1 ambient "" -> token=399b45fab938dd86 purchases=1 same token? false total purchases for one account: 2 Previewing without the account and then re-calling with it explicit is ordinary self-correcting model behavior, so this was a likely sequence rather than a corner case. The earlier review reasoned only about the opposite direction (one token spanning two accounts) and concluded an empty scope was safe because a purely-ambient provider is unambiguous for the process lifetime; that is true and irrelevant, since the hazard is two tokens for one account. A real purchase now requires a determinable account and is refused before any credential is resolved, naming the argument to pass. Previews are untouched, so pricing a purchase still needs no account. Explicit and ambient forms of a KNOWN account already converge, because CredentialScope falls back to the same environment variable the provider factory itself reads, so normal dedupe is preserved; only the both-absent case aliased and it is now unreachable. Deriving the effective account from the resolved client would be the stronger fix, but provider.ServiceClient exposes no account accessor, so that needs an interface change across every AWS/Azure/GCP service client. Failing closed removes the hazard by construction meanwhile: on an LLM-driven money path, making the operator name the target account before spending is the same posture as EnvEnableRealPurchases. The gates move into authorizeRealPurchase so ExecutePurchase stays under the gocyclo:10 pre-commit threshold (it hit 11) and so "what must be true before this server spends money" reads in one place. TestExecutePurchaseAmbientScopeCannotDoubleBuy reproduces the explicit-then-omitted sequence and asserts the second call never reaches the provider; verified failing before this change. Its companion TestExecutePurchaseExplicitAndAmbientAccountDedupe pins that the two ways of naming a known account still derive one token, so the fix removes the aliasing rather than relocating it. mcp/README.md documents the new requirement, and separately the already-landed Azure behavior change where an empty payment_option now fails loud instead of silently billing all-upfront (reachable from scheduled rows predating migration 000032). * fix(mcp): fan out reservation searches over every term and payment option cudly_search_recommendations documented "omitted means search all" for reservation searches, and mcp/README.md stated AWS defaults term/payment server-side when they are omitted. Neither was true. GetReservationPurchaseRecommendation takes exactly one TermInYears and one PaymentOption per request and returns results only for that cell. Omitting them sent Cost Explorer EMPTY enum values (convertTermInYears and convertPaymentOption both silently map unrecognized input to ""), AWS quietly applied its own 1yr/all-upfront default, and the caller received one of six purchasable offers while the tool claimed it had searched them all. Verified against a live account: the omitted-field search returned only 1yr/all-upfront at 40% savings, hiding 3yr/all-upfront at 63% on the identical t4g.nano. A model told "omitted searches all terms" would report the worse offer as the only option. Expand each omitted dimension to its full menu and issue one call per combination (6 when both are omitted, 2 or 3 when one is). Because each call now carries a concrete term and payment option, the AWS parser tags every returned recommendation with the offer that produced it, so the money figures are no longer blank and unattributable. A failing combination fails the whole search rather than being skipped: returning five of six offers is indistinguishable from "these are all your options" and would recreate the same defect. Results are always a non-nil slice, so a no-results search serializes as [] instead of sometimes [] and sometimes null. lookback_period is deliberately NOT fanned out; it is the usage evidence behind an offer rather than another offer to choose from, so AWS's server-side default stands. The schema text and README now say so instead of claiming it searches all lookback windows. TestSearchRecommendationsEC2NoDefaultsInjected asserted the incorrect premise in its own doc comment and is rewritten to guard what it was actually for: the Savings Plans defaults must not narrow a reservation search, and lookback_period must still reach AWS blank. * fix(mcp): advertise term/payment enums and split the search test file Addresses two CodeRabbit findings on the search fan-out commit (d76aa7c00). The tool schema constrained only provider and lookback_period, so an MCP client could not discover the valid payment_option/term_years values from the schema and had to learn them by sending a value the tool rejects. Add the enum overrides, mirroring ValidatePaymentOption / ValidateTermYears. Those validators remain the enforcing guard: a client may send anything regardless of what the schema declares, so this is discoverability, not a new gate. TestSearchRecommendationsSchemaAdvertisesTermAndPaymentEnums pins both value sets through the real MCP protocol (ListTools), following the existing Azure partial-upfront precedent in this file, so the schema cannot drift from the validators unnoticed. Confirmed to fail when either override is removed. search_recommendations_test.go had grown to 651 lines, past the 500-line guideline in CLAUDE.md. Split the AWS reservation fan-out tests into search_recommendations_fanout_test.go along the existing topical seam, leaving 473 and 190 lines. The shared fakes stay in the original file and remain visible to both: same package, different file. * fix(mcp): omit effective_date when the provider reported no timestamp common.PurchaseResult.Timestamp is a plain time.Time that not every provider client populates. PurchaseResponse.EffectiveDate was a plain string set from result.Timestamp.Format(time.RFC3339), and formatting the zero time.Time yields the literal "0001-01-01T00:00:00Z" rather than "", so the `omitempty` tag could never drop it. Every such response therefore advertised a real-looking commitment start date in the year 1: a fabricated value presented as real, on a field a caller may key billing or renewal reminders off. Make EffectiveDate a *string and populate it through rfc3339OrNil, which returns nil for the zero time. This is the time analog of the existing nonZeroCostPtr, applied for the same reason the cost fields already use pointers: keep "not known" distinguishable from a real value instead of fabricating one. Adds two regression tests. The guard asserts against the marshaled JSON, not just the Go field, because the JSON payload is what actually crosses the MCP boundary to the caller; it fails on the pre-fix code and passes after. The companion test pins that suppressing the zero value does not suppress a genuine timestamp. * test(mcp): guard content index and type assertion in the binary smoke test result.Content[0].(*gosdk.TextContent) panics on an empty Content slice or a non-text first block. A panic aborts the whole package's test run rather than failing this one assertion, so a regression in the tool's error shape would surface as an unrelated-looking crash instead of a readable failure naming the actual problem. Check the slice is non-empty and use the comma-ok form, reporting the concrete type when the assertion fails. * docs(mcp): explain why the GCP CUD tool has no ambient scope fallback Review flagged that gcp_computeengine_cud.go passes no environment fallback to CredentialScope, unlike the AWS ("AWS_PROFILE") and Azure ("AZURE_SUBSCRIPTION_ID") tools, so omitting gcp_project_id makes requireCredentialScope refuse every real GCP CUD purchase. The asymmetry is deliberate and must stay. CredentialScope's contract is that its fallback names the same variable the provider factory itself consults, so that naming an account explicitly and letting it resolve ambiently derive the same idempotency token. GCP has no such variable: resolveGCPProjectID reads only config.GCPProjectID and the deprecated config.Profile, and when both are empty NewProvider falls through to getDefaultProject, which picks the first ACTIVE project returned by a paginated ListProjects. Adding GOOGLE_CLOUD_PROJECT to the CredentialScope call alone would make the token lie: the scope would read that variable while the purchase still landed in whatever project getDefaultProject resolved, so the same target reached two ways would derive two different tokens and double-buy. That is precisely the hazard requireCredentialScope was added to close. Teaching the factory to read it too would fix the divergence but change project selection for every other consumer of providers/gcp (CLI, web, scheduler), and GOOGLE_CLOUD_PROJECT conventionally names the project a process runs in rather than the one it should buy for, so on a hosted runtime it would silently redirect purchases. Document the reasoning at the call site so it is not "fixed" later, and correct the gcp_project_id schema description, which still told the model the field was optional. It is optional for a dry_run preview and required for a real purchase, which is what made the refusal surprising in…
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
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.