From 5f3a3f4cf00967da8a007f132c5f74a1f39acab4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 29 Sep 2026 00:55:45 +0200 Subject: [PATCH] fix(purchase): preserve nullable cost with updated library pin Pin all four library modules to ce9513612901 and pass the provider's upfront cost pointer through unchanged. Preserve known zero values and omit unknown amounts in MCP output. Verify nil, zero, and positive costs through the MCP protocol with a fake provider. The old zero-dropping conversion fails the zero case. --- go.mod | 8 ++--- go.sum | 16 +++++----- tools/aws_ec2_ri_test.go | 65 ++++++++++++++++++++++++++++++++++++++++ tools/purchase.go | 21 +++---------- tools/purchase_test.go | 3 +- 5 files changed, 83 insertions(+), 30 deletions(-) diff --git a/go.mod b/go.mod index 5b7be77..5bed290 100644 --- a/go.mod +++ b/go.mod @@ -77,10 +77,10 @@ require ( ) require ( - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901 + 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 github.com/google/uuid v1.6.0 github.com/modelcontextprotocol/go-sdk v1.6.1 diff --git a/go.sum b/go.sum index 86f8960..0eff436 100644 --- a/go.sum +++ b/go.sum @@ -76,14 +76,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= 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-20260928205306-c5cf5c7fbad8 h1:/4MDV2fttTEnjt37Iq49v5SGMvYuBH68q99r9E+juOE= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8 h1:qUmZE0uvsWKU6Gtglo3h61jVEvqoihFjig6e1r/IvDk= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:tIUCLVX6utdBPiP6dAjY7GUfwIQngeSMK5A9aRqe6gA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8 h1:eyAXwyDwIvEI9vdHb2TqkNPRuhbMYWDXN4751LOltKA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:zgCL/ozOkcZUDbEC7a2UwW+6MPcLoBlEU2WUZ0TORUs= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8 h1:qs2P8PsqKcFl6eGMGvi7EfMXDuni3pqkc8vDRESppKA= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928205306-c5cf5c7fbad8/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901 h1:JWQwkwshjBoEuoIuFK1KHh70yZDYQTrm5GRloD6ojjU= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260928214714-ce9513612901/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901 h1:18MxQd7+ZQ+Z2uNbZfpIJQ00JWdSdTVSmVWBzg4R0DA= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20260928214714-ce9513612901/go.mod h1:tIUCLVX6utdBPiP6dAjY7GUfwIQngeSMK5A9aRqe6gA= +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= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY= github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o= github.com/aws/aws-sdk-go-v2/config v1.29.12 h1:Y/2a+jLPrPbHpFkpAAYkVEtJmxORlXoo5k2g1fa2sUo= diff --git a/tools/aws_ec2_ri_test.go b/tools/aws_ec2_ri_test.go index fdd15b6..f3d2374 100644 --- a/tools/aws_ec2_ri_test.go +++ b/tools/aws_ec2_ri_test.go @@ -2,8 +2,11 @@ package tools import ( "context" + "encoding/json" "testing" + "time" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -208,6 +211,68 @@ func TestAWSEC2RIPurchaseHandleRealPurchaseResolvesEC2Client(t *testing.T) { assert.Equal(t, common.PurchaseSourceMCP, fake.lastOpts.Source) } +func TestAWSEC2RIPurchaseCostJSON(t *testing.T) { + t.Parallel() + zero, positive := 0.0, 600.0 + cases := []struct { + name string + cost *float64 + }{ + {"unknown", nil}, + {"zero", &zero}, + {"positive", &positive}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fake := &fakeServiceClient{purchaseResult: common.PurchaseResult{Success: true, Cost: tc.cost}} + var gotService common.ServiceType + var gotRegion string + tool := &awsEC2RIPurchaseTool{ + createProvider: func(_ string, _ *provider.ProviderConfig) (provider.Provider, error) { + return &recordingProvider{ + fakeProvider: &fakeProvider{name: "aws"}, client: fake, + gotService: &gotService, gotRegion: &gotRegion, + }, nil + }, + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + server := mcp.NewServer(&mcp.Implementation{Name: "test-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: "test-client"}, nil) + session, err := client.Connect(ctx, clientTransport, nil) + require.NoError(t, err) + defer session.Close() + args := validEC2Args() + args.DryRun, args.Confirm = boolPtr(false), boolPtr(true) + result, err := session.CallTool(ctx, &mcp.CallToolParams{Name: awsEC2RIPurchaseName, Arguments: args}) + require.NoError(t, err) + require.False(t, result.IsError) + structured, err := json.Marshal(result.StructuredContent) + require.NoError(t, err) + var fields map[string]any + require.NoError(t, json.Unmarshal(structured, &fields)) + assert.Equal(t, true, fields["success"]) + assert.Equal(t, false, fields["dry_run"]) + assert.Equal(t, 1, fake.purchaseCalls) + if tc.cost == nil { + assert.NotContains(t, fields, "cost") + } else { + require.Contains(t, fields, "cost") + assert.Equal(t, *tc.cost, fields["cost"]) + } + assert.NotContains(t, fields, "on_demand_cost") + assert.NotContains(t, fields, "estimated_savings") + assert.NotContains(t, fields, "savings_percentage") + }) + } +} + // recordingProvider wraps fakeProvider to capture the service/region passed // to GetServiceClient and always return a fixed ServiceClient. type recordingProvider struct { diff --git a/tools/purchase.go b/tools/purchase.go index e4fea13..9dfcf51 100644 --- a/tools/purchase.go +++ b/tools/purchase.go @@ -287,16 +287,8 @@ func ResolveDryRunConfirm(dryRun, confirm *bool) (effectiveDryRun, effectiveConf // protocol-level error -- ExecutePurchase itself still returns a Go error // for gate refusals and provider-call failures. // -// Cost/OnDemandCost/EstimatedSavings/SavingsPercentage are pointers with -// omitempty: none of the *FromArgs constructors in this package populate -// Recommendation's cost fields (they build a fresh Recommendation from the -// caller's typed args, not from a priced search result), and some provider -// clients (e.g. AWS EC2 RIs, Savings Plans) never populate -// PurchaseResult.Cost either. A plain float64 could not distinguish "not -// known" from "genuinely $0", so every response reported 0 for money fields -// it never actually priced. A pointer that's nil (and omitted from the JSON -// payload entirely) when no real value exists lets a caller tell "unknown" -// apart from "confirmed zero" (feedback_nullable_not_zero). +// Executed Cost is the provider-reported total upfront cost: nil is omitted, +// while a known zero is retained. Unpriced recommendation fields are omitted. // // EffectiveDate is a pointer for the same reason, and it is not a // hypothetical concern: `omitempty` on a string only drops "", so a @@ -380,12 +372,7 @@ func termYearsFromRecommendationTerm(term string) int { return years } -// nonZeroCostPtr returns a pointer to v, or nil when v is exactly zero. Cost -// and savings fields on common.Recommendation and common.PurchaseResult are -// plain (unpointered) float64s that upstream code sometimes never populates -// (see the PurchaseResponse doc comment above); this treats an unpopulated -// zero as "unknown" rather than fabricating a real $0 figure the caller -// never priced. +// Recommendation's scalar cost and savings fields use zero for unpriced values. func nonZeroCostPtr(v float64) *float64 { if v == 0 { return nil @@ -670,7 +657,7 @@ func ExecutePurchase(ctx context.Context, req PurchaseRequest) (*PurchaseRespons Success: purchaseSucceeded(result), DryRun: result.DryRun, CommitmentID: result.CommitmentID, - Cost: nonZeroCostPtr(result.Cost), + Cost: result.Cost, OnDemandCost: nonZeroCostPtr(rec.OnDemandCost), EstimatedSavings: nonZeroCostPtr(rec.EstimatedSavings), SavingsPercentage: nonZeroCostPtr(rec.SavingsPercentage), diff --git a/tools/purchase_test.go b/tools/purchase_test.go index b2a5c44..f91856c 100644 --- a/tools/purchase_test.go +++ b/tools/purchase_test.go @@ -425,11 +425,12 @@ func TestExecutePurchaseRealPurchaseGate(t *testing.T) { // token -- never a caller-suppliable source string. func TestExecutePurchaseRealPurchaseCallsProviderWithMCPSource(t *testing.T) { t.Parallel() + cost := 600.0 fake := &fakeServiceClient{ purchaseResult: common.PurchaseResult{ Success: true, CommitmentID: "ri-12345", - Cost: 600, + Cost: &cost, }, } resolve := func(_ context.Context) (provider.ServiceClient, error) {