From f39c118b54d8b313691c6ca0f0888a5d35e43864 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 3 Oct 2026 23:04:39 +0200 Subject: [PATCH] fix(recommendations): reject incomplete savings plans menus Pin the published AWS collection fix and exercise all Savings Plans selectors through the registered MCP transport and real AWS adapter. Refs LeanerCloud/cloud-commitments-go#170 --- go.mod | 2 +- go.sum | 4 +- ...earch_recommendations_completeness_test.go | 271 +++++++++++++----- 3 files changed, 195 insertions(+), 82 deletions(-) diff --git a/go.mod b/go.mod index 002fd8e..47b4de6 100644 --- a/go.mod +++ b/go.mod @@ -78,7 +78,7 @@ require ( require ( github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261002152209-006ef5c8d0a2 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 github.com/google/jsonschema-go v0.4.3 diff --git a/go.sum b/go.sum index 5ce6c86..e620060 100644 --- a/go.sum +++ b/go.sum @@ -78,8 +78,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80 h1:wVKlMokfaME/Lw525Qz3F155R3nY4VmuaIyh+6d4sCU= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261002152209-006ef5c8d0a2 h1:rYxXq0G0O2lkf8AB1ni9m4pVhIdb6Z0y52sTxS1RKIA= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261002152209-006ef5c8d0a2/go.mod h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 h1:DfNEBzFS7/MaBZljLGRRRUaarfnXXZyBE/6iozzp9/k= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695/go.mod h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 h1:iSdHYdmGUjcjtSmgGZxptBzDuLrJ9KR2mh1/x7FNvSY= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901/go.mod h1:zgCL/ozOkcZUDbEC7a2UwW+6MPcLoBlEU2WUZ0TORUs= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 h1:i6OwXLUheudN3GfwnYXdKuEq8vPdE9qkNJ+r71LRLKA= diff --git a/tools/search_recommendations_completeness_test.go b/tools/search_recommendations_completeness_test.go index d84afde..fbd0947 100644 --- a/tools/search_recommendations_completeness_test.go +++ b/tools/search_recommendations_completeness_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "strings" + "sync" "testing" "time" @@ -21,106 +22,180 @@ import ( func TestSearchRecommendationsAWSCompletenessProtocol(t *testing.T) { t.Parallel() - for _, selected := range []bool{false, true} { - for _, kind := range []string{"valid", "empty", "mixed", "all-invalid"} { - t.Run(fmt.Sprintf("selected=%t/%s", selected, kind), func(t *testing.T) { - t.Parallel() - fixture := &recommendationCompletenessHTTP{kind: kind} - adapter := awsprovider.NewRecommendationsClient(aws.Config{ - Region: "us-east-1", HTTPClient: fixture, - Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { - return aws.Credentials{AccessKeyID: "synthetic", SecretAccessKey: "synthetic"}, nil - }), - }) - tool := newTestSearchTool(&fakeProvider{name: "aws", services: []common.ServiceType{common.ServiceRDS}, recClient: adapter}) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - server := mcp.NewServer(&mcp.Implementation{Name: "completeness-server"}, nil) - require.NoError(t, tool.Register(server)) - clientTransport, serverTransport := mcp.NewInMemoryTransports() - serverSession, err := server.Connect(ctx, serverTransport, nil) - require.NoError(t, err) - defer serverSession.Close() - client := mcp.NewClient(&mcp.Implementation{Name: "completeness-client"}, nil) - session, err := client.Connect(ctx, clientTransport, nil) - require.NoError(t, err) - defer session.Close() - args := map[string]any{"provider": "aws", "service": "rds"} - wantRequests := []recommendationCompletenessRequest{ - {"ONE_YEAR", "ALL_UPFRONT"}, {"ONE_YEAR", "PARTIAL_UPFRONT"}, {"ONE_YEAR", "NO_UPFRONT"}, - {"THREE_YEARS", "ALL_UPFRONT"}, {"THREE_YEARS", "PARTIAL_UPFRONT"}, {"THREE_YEARS", "NO_UPFRONT"}, - } - if selected { - args["term_years"], args["payment_option"] = 3, "no-upfront" - wantRequests = wantRequests[5:] - } - result, err := session.CallTool(ctx, &mcp.CallToolParams{Name: searchRecommendationsName, Arguments: args}) - require.NoError(t, err) - require.Empty(t, fixture.unexpected) - if kind == "all-invalid" { - wantRequests = wantRequests[:1] - } - assert.Equal(t, wantRequests, fixture.requests) - if kind == "mixed" || kind == "all-invalid" { - require.True(t, result.IsError, "incomplete response produced a successful short menu: %+v", result.StructuredContent) - require.Nil(t, result.StructuredContent) - var diagnostic strings.Builder - for _, content := range result.Content { - part, ok := content.(*mcp.TextContent) - require.True(t, ok) - diagnostic.WriteString(part.Text) + for _, service := range []common.ServiceType{common.ServiceRDS, common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance, common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, common.ServiceSavingsPlansAll} { + for _, selected := range []bool{false, true} { + kinds := []string{"valid", "empty", "mixed", "all-invalid"} + if common.IsSavingsPlan(service) { + kinds = append(kinds, "partial-type-failure", "all-type-failure", "late-page-failure") + } + for _, kind := range kinds { + t.Run(fmt.Sprintf("%s/selected=%t/%s", service, selected, kind), func(t *testing.T) { + t.Parallel() + fixture := &recommendationCompletenessHTTP{kind: kind} + adapter := awsprovider.NewRecommendationsClient(aws.Config{ + Region: "us-east-1", HTTPClient: fixture, RetryMaxAttempts: 1, + Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "synthetic", SecretAccessKey: "synthetic"}, nil + }), + }) + tool := newTestSearchTool(&fakeProvider{name: "aws", services: []common.ServiceType{service}, recClient: adapter}) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + server := mcp.NewServer(&mcp.Implementation{Name: "completeness-server"}, nil) + require.NoError(t, tool.Register(server)) + clientTransport, serverTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(ctx, serverTransport, nil) + require.NoError(t, err) + defer serverSession.Close() + client := mcp.NewClient(&mcp.Implementation{Name: "completeness-client"}, nil) + session, err := client.Connect(ctx, clientTransport, nil) + require.NoError(t, err) + defer session.Close() + args := map[string]any{"provider": "aws", "service": string(service)} + wantRequests := []recommendationCompletenessRequest{ + {Term: "ONE_YEAR", Payment: "ALL_UPFRONT", Scope: "LINKED"}, {Term: "ONE_YEAR", Payment: "PARTIAL_UPFRONT", Scope: "LINKED"}, {Term: "ONE_YEAR", Payment: "NO_UPFRONT", Scope: "LINKED"}, + {Term: "THREE_YEARS", Payment: "ALL_UPFRONT", Scope: "LINKED"}, {Term: "THREE_YEARS", Payment: "PARTIAL_UPFRONT", Scope: "LINKED"}, {Term: "THREE_YEARS", Payment: "NO_UPFRONT", Scope: "LINKED"}, + } + if selected { + args["term_years"], args["payment_option"] = 3, "no-upfront" + wantRequests = wantRequests[5:] + } + if common.IsSavingsPlan(service) { + planTypes := map[common.ServiceType][]string{ + common.ServiceSavingsPlansCompute: {"COMPUTE_SP"}, common.ServiceSavingsPlansEC2Instance: {"EC2_INSTANCE_SP"}, + common.ServiceSavingsPlansSageMaker: {"SAGEMAKER_SP"}, common.ServiceSavingsPlansDatabase: {"DATABASE_SP"}, + common.ServiceSavingsPlansAll: {"COMPUTE_SP", "EC2_INSTANCE_SP", "SAGEMAKER_SP", "DATABASE_SP"}, + }[service] + term, payment, lookback := "ONE_YEAR", "NO_UPFRONT", "THIRTY_DAYS" + if selected { + term, payment, lookback = "THREE_YEARS", "PARTIAL_UPFRONT", "SIXTY_DAYS" + args["payment_option"], args["lookback_period"] = "partial-upfront", "60d" + } + wantRequests = make([]recommendationCompletenessRequest, 0, len(planTypes)+1) + for _, planType := range planTypes { + request := recommendationCompletenessRequest{Term: term, Payment: payment, PlanType: planType, Lookback: lookback, Scope: "LINKED"} + wantRequests = append(wantRequests, request) + if kind == "late-page-failure" && planType == planTypes[0] { + request.Token = "page-two" + wantRequests = append(wantRequests, request) + } + } + } + result, err := session.CallTool(ctx, &mcp.CallToolParams{Name: searchRecommendationsName, Arguments: args}) + require.NoError(t, err) + fixture.mu.Lock() + defer fixture.mu.Unlock() + require.Empty(t, fixture.unexpected) + if kind == "all-invalid" && service == common.ServiceRDS { + wantRequests = wantRequests[:1] + } + assert.Equal(t, wantRequests, fixture.requests) + if common.IsSavingsPlan(service) && kind != "valid" && kind != "empty" { + require.True(t, result.IsError, "incomplete SP collection produced a menu: %+v", result.StructuredContent) + require.Nil(t, result.StructuredContent) + var diagnostic strings.Builder + for _, content := range result.Content { + part, ok := content.(*mcp.TextContent) + require.True(t, ok) + diagnostic.WriteString(part.Text) + } + fatal := kind == "all-type-failure" || (kind == "partial-type-failure" && service != common.ServiceSavingsPlansAll) + if fatal { + assert.Contains(t, diagnostic.String(), "fixture API failure") + assert.NotContains(t, diagnostic.String(), "failed details") + } else { + details, scopes := 0, 1 + if kind == "mixed" || kind == "all-invalid" { + details, scopes = len(wantRequests), 0 + assert.Contains(t, diagnostic.String(), "page 0 detail") + assert.Contains(t, diagnostic.String(), "not-a-number") + } + assert.Regexp(t, fmt.Sprintf(`\b%d failed details\b`, details), diagnostic.String()) + assert.Regexp(t, fmt.Sprintf(`\b%d failed scopes\b`, scopes), diagnostic.String()) + if kind == "late-page-failure" { + assert.Contains(t, diagnostic.String(), "page 1") + } + } + if !fatal { + failedRequest := wantRequests[0] + assert.Contains(t, diagnostic.String(), failedRequest.PlanType) + term, payment := "1yr", "no-upfront" + if selected { + term, payment = "3yr", "partial-upfront" + } + assert.Contains(t, diagnostic.String(), "term "+term) + assert.Contains(t, diagnostic.String(), "payment "+payment) + } + return } - index, term, payment := 1, "3yr", "no-upfront" - if kind == "all-invalid" { - index = 0 + if kind == "mixed" || kind == "all-invalid" { + require.True(t, result.IsError, "incomplete response produced a successful short menu: %+v", result.StructuredContent) + require.Nil(t, result.StructuredContent) + var diagnostic strings.Builder + for _, content := range result.Content { + part, ok := content.(*mcp.TextContent) + require.True(t, ok) + diagnostic.WriteString(part.Text) + } + index, term, payment := 1, "3yr", "no-upfront" + if kind == "all-invalid" { + index = 0 + if !selected { + term, payment = "1yr", "all-upfront" + } + } + assert.Regexp(t, `\b1 failed details\b`, diagnostic.String()) + assert.Regexp(t, `\b0 failed scopes\b`, diagnostic.String()) + for _, want := range []string{fmt.Sprintf("block 0 detail %d", index), term, payment, "not-a-number"} { + assert.Contains(t, diagnostic.String(), want) + } if !selected { - term, payment = "1yr", "all-upfront" + assert.Contains(t, diagnostic.String(), "term="+term+", payment_option="+payment) } + return } - assert.Regexp(t, `\b1 failed details\b`, diagnostic.String()) - assert.Regexp(t, `\b0 failed scopes\b`, diagnostic.String()) - for _, want := range []string{fmt.Sprintf("block 0 detail %d", index), term, payment, "not-a-number"} { - assert.Contains(t, diagnostic.String(), want) + require.False(t, result.IsError) + encoded, err := json.Marshal(result.StructuredContent) + require.NoError(t, err) + var menu struct { + Count int `json:"count"` + Recommendations []json.RawMessage `json:"recommendations"` } - if !selected { - assert.Contains(t, diagnostic.String(), "term="+term+", payment_option="+payment) + require.NoError(t, json.Unmarshal(encoded, &menu)) + wantCount := len(wantRequests) + if kind == "empty" { + wantCount = 0 } - return - } - require.False(t, result.IsError) - encoded, err := json.Marshal(result.StructuredContent) - require.NoError(t, err) - var menu struct { - Count int `json:"count"` - Recommendations []json.RawMessage `json:"recommendations"` - } - require.NoError(t, json.Unmarshal(encoded, &menu)) - wantCount := len(wantRequests) - if kind == "empty" { - wantCount = 0 - } - assert.Equal(t, wantCount, menu.Count) - assert.Len(t, menu.Recommendations, wantCount) - assert.NotNil(t, menu.Recommendations) - }) + assert.Equal(t, wantCount, menu.Count) + assert.Len(t, menu.Recommendations, wantCount) + assert.NotNil(t, menu.Recommendations) + }) + } } } } type recommendationCompletenessRequest struct { - Term string `json:"TermInYears"` - Payment string `json:"PaymentOption"` + Term string `json:"TermInYears"` + Payment string `json:"PaymentOption"` + PlanType string `json:"SavingsPlansType,omitempty"` + Lookback string `json:"LookbackPeriodInDays,omitempty"` + Scope string `json:"AccountScope,omitempty"` + Token string `json:"NextPageToken,omitempty"` } type recommendationCompletenessHTTP struct { + mu sync.Mutex kind string requests []recommendationCompletenessRequest unexpected []string } func (f *recommendationCompletenessHTTP) Do(req *http.Request) (*http.Response, error) { + f.mu.Lock() + defer f.mu.Unlock() operation := req.Header.Get("X-Amz-Target") - if operation != "AWSInsightsIndexService.GetReservationPurchaseRecommendation" { + if req.URL.Host != "ce.us-east-1.amazonaws.com" || (operation != "AWSInsightsIndexService.GetReservationPurchaseRecommendation" && operation != "AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation") { f.unexpected = append(f.unexpected, operation) return nil, fmt.Errorf("unexpected SDK operation: %s", operation) } @@ -129,6 +204,44 @@ func (f *recommendationCompletenessHTTP) Do(req *http.Request) (*http.Response, return nil, err } f.requests = append(f.requests, params) + if operation == "AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation" { + status := http.StatusOK + detail := map[string]any{"AccountId": "survivor", "HourlyCommitmentToPurchase": "2", "EstimatedMonthlySavingsAmount": "10", "UpfrontCost": "3", "CurrentAverageHourlyOnDemandSpend": "4"} + invalid := map[string]any{"HourlyCommitmentToPurchase": "not-a-number"} + details := make([]map[string]any, 1, 2) + details[0] = detail + token := "" + switch f.kind { + case "empty": + details = details[:0] + case "mixed": + details = append(details, invalid) + case "all-invalid": + details = []map[string]any{invalid} + case "late-page-failure": + if params.PlanType == f.requests[0].PlanType { + token = "page-two" + if params.Token != "" { + status = http.StatusBadRequest + } + } + case "partial-type-failure": + if params.PlanType == f.requests[0].PlanType { + status = http.StatusBadRequest + } + case "all-type-failure": + status = http.StatusBadRequest + } + body := map[string]any{"NextPageToken": token, "SavingsPlansPurchaseRecommendation": map[string]any{"SavingsPlansPurchaseRecommendationDetails": details}} + if status != http.StatusOK { + body = map[string]any{"__type": "InvalidParameterValueException", "Message": "fixture API failure"} + } + encoded, err := json.Marshal(body) + if err != nil { + return nil, err + } + return &http.Response{StatusCode: status, Header: http.Header{"Content-Type": {"application/x-amz-json-1.1"}}, Body: io.NopCloser(strings.NewReader(string(encoded))), Request: req}, nil + } valid := map[string]any{ "RecommendedNumberOfInstancesToPurchase": "2", "EstimatedMonthlySavingsAmount": "10", "EstimatedMonthlyOnDemandCost": "30",