Repository navigation
Commit 887d51f
authored
feat(mcp): CUDly MCP server for RI/SP/CUD purchases across AWS, Azure, 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…1 parent 3e9660d commit 887d51f
62 files changed
Lines changed: 9683 additions & 94 deletions
File tree
- cmd
- cudly-mcp
- mcp
- tools
- pkg/common
- providers
- aws
- recommendations
- azure/services
- cache
- compute
- cosmosdb
- database
- internal/reservations
- managedredis
- search
- synapse
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
18 | 24 | | |
19 | 25 | | |
20 | 26 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
169 | 169 | | |
170 | 170 | | |
171 | 171 | | |
172 | | - | |
173 | | - | |
174 | | - | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
175 | 179 | | |
176 | 180 | | |
177 | 181 | | |
| |||
200 | 204 | | |
201 | 205 | | |
202 | 206 | | |
203 | | - | |
204 | | - | |
205 | | - | |
206 | | - | |
207 | | - | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
208 | 210 | | |
209 | 211 | | |
210 | 212 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
104 | 104 | | |
105 | 105 | | |
106 | 106 | | |
| 107 | + | |
107 | 108 | | |
108 | 109 | | |
109 | 110 | | |
| 111 | + | |
110 | 112 | | |
111 | 113 | | |
112 | 114 | | |
| |||
175 | 177 | | |
176 | 178 | | |
177 | 179 | | |
| 180 | + | |
| 181 | + | |
178 | 182 | | |
179 | 183 | | |
180 | 184 | | |
181 | 185 | | |
182 | 186 | | |
183 | 187 | | |
| 188 | + | |
184 | 189 | | |
185 | 190 | | |
186 | 191 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
222 | 222 | | |
223 | 223 | | |
224 | 224 | | |
| 225 | + | |
| 226 | + | |
225 | 227 | | |
226 | 228 | | |
227 | 229 | | |
| |||
296 | 298 | | |
297 | 299 | | |
298 | 300 | | |
| 301 | + | |
| 302 | + | |
299 | 303 | | |
300 | 304 | | |
301 | 305 | | |
| |||
318 | 322 | | |
319 | 323 | | |
320 | 324 | | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
321 | 329 | | |
322 | 330 | | |
323 | 331 | | |
| |||
345 | 353 | | |
346 | 354 | | |
347 | 355 | | |
| 356 | + | |
| 357 | + | |
348 | 358 | | |
349 | 359 | | |
350 | 360 | | |
| |||
387 | 397 | | |
388 | 398 | | |
389 | 399 | | |
| 400 | + | |
| 401 | + | |
390 | 402 | | |
391 | 403 | | |
392 | 404 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
489 | 489 | | |
490 | 490 | | |
491 | 491 | | |
| 492 | + | |
492 | 493 | | |
493 | 494 | | |
494 | 495 | | |
| |||
697 | 698 | | |
698 | 699 | | |
699 | 700 | | |
| 701 | + | |
700 | 702 | | |
701 | 703 | | |
702 | 704 | | |
| |||
0 commit comments