feat(mcp): annotate every tool with MCP ToolAnnotations

docs/mcp-annotations-decisions.md

@@ -0,0 +1,138 @@
+# MCP tool annotations -- decisions
+
+This repo has no existing ADR/`docs/decisions/` convention (checked at
+implementation time: no `docs/adr/` or `docs/decisions/` directory), so
+this rationale lives here instead of in a numbered ADR.
+Context: `docs/plans/mcp/05-store.md` Phase A and `docs/plans/mcp/00-scope.md`
+§6 (not checked into this repo -- local planning notes for the MCP
+store-readiness workstream).
+
+## Why annotations at all
+
+Anthropic's and OpenAI's MCP directory review criteria both treat tool
+`title` and the `readOnlyHint`/`destructiveHint` annotations as a hard
+submission gate, and annotation quality is the #1 rejection cause at OpenAI.
+Independent of directory submission, the go-sdk's `mcp.ToolAnnotations` has a
+trap that makes annotating explicitly non-optional for this server
+specifically:
+
+> **Nil-defaults trap**: `DestructiveHint *bool` and `OpenWorldHint *bool`
+> default to `true` on the wire when left nil. A `nil` `Annotations` field
+> (or a struct that leaves either pointer unset) therefore publishes
+> "destructive, open-world, not read-only" for every tool that omits it --
+> including the read-only search and catalog tools. Omission is not a safe
+> or neutral default; it is a wrong answer for 2 of this server's 11 tools.
+
+`ReadOnlyHint bool` and `IdempotentHint bool` have no such trap (their zero
+value, `false`, is already the conservative reading), but every Descriptor
+still sets them explicitly via `readOnlyAnnotations`/`purchaseAnnotations`
+(`mcp/tools/registry.go`) so the intent is legible at every call site rather
+than resting on an implicit zero value.
+
+## D1 -- destructiveHint=true on every purchase tool
+
+Considered and rejected: treating a purchase as "additive" (it only creates
+a new reservation/commitment, never deletes or mutates existing state) and
+setting `destructiveHint=false`.
+
+Rejected because Anthropic's review criteria require `destructiveHint` on
+any tool that modifies state, not only ones that delete or overwrite it, and
+state explicitly that "destructive tools always prompt" in a client UI. A
+tool that commits the caller's cloud account to a multi-thousand-dollar,
+multi-year financial obligation is exactly the class of action a client
+should always confirm before executing, regardless of whether the technical
+effect is additive. All 9 purchase tools (`cudly_aws_ec2_ri_purchase`,
+`cudly_aws_rds_ri_purchase`, `cudly_aws_elasticache_ri_purchase`,
+`cudly_aws_opensearch_ri_purchase`, `cudly_aws_redshift_ri_purchase`,
+`cudly_aws_memorydb_ri_purchase`, `cudly_aws_savingsplans_purchase`,
+`cudly_azure_compute_ri_purchase`, `cudly_gcp_computeengine_cud_purchase`)
+therefore set `DestructiveHint: boolPtr(true)`.
+
+## A5 -- idempotentHint=false on every purchase tool, always
+
+Every purchase tool's `IdempotentHint` is `false`, unconditionally, even
+though `ExecutePurchase` (`mcp/tools/purchase.go`) already derives an
+idempotency key from the request and refuses/dedupes a byte-identical retry.
+
+Rejected alternative: `IdempotentHint=true`, on the theory that the
+server-side dedupe already makes a retry safe.
+
+Rejected because:
+
+- `idempotency_nonce` is caller-supplied and optional. Varying that one
+  string field turns an otherwise byte-identical request into a
+  deliberately distinct one, so "retrying with the same arguments has no
+  additional effect" is not actually true in general -- it only holds when
+  the caller leaves the nonce untouched, and the hint has no way to express
+  that condition.
+- Provider-side dedupe (AWS `ClientToken`, and the Azure/GCP equivalents) is
+  time- and scope-bounded, not a permanent guarantee independent of this
+  server's own idempotency key.
+- The failure asymmetry is one-sided: a wrongly-`true` hint risks an MCP
+  client auto-retrying a stalled/ambiguous call and buying a real
+  multi-thousand-dollar commitment twice; a wrongly-`false` hint costs
+  nothing worse than an extra confirmation round in a client UI that treats
+  non-idempotent tools more cautiously. Given that asymmetry, `false` is the
+  only defensible default across all 9 purchase tools.
+
+## Per-tool annotation table
+
+Built from `mcp/tools/registry.go`'s `readOnlyAnnotations`/
+`purchaseAnnotations` helpers and each tool's `Descriptor()`. Every tool sets
+`ReadOnlyHint`, `DestructiveHint`, `IdempotentHint`, and `OpenWorldHint`
+explicitly; none rely on the SDK's nil-defaults-to-true behavior.
+
+| Tool | ReadOnly | Destructive | Idempotent | OpenWorld | Title |
+|---|---|---|---|---|---|
+| `cudly_search_recommendations` | true | false | false | true | Search commitment recommendations |
+| `cudly_list_commitment_actions` | true | false | false | **false** | List available commitment actions |
+| `cudly_aws_ec2_ri_purchase` | false | true | false | true | Purchase AWS EC2 Reserved Instances |
+| `cudly_aws_rds_ri_purchase` | false | true | false | true | Purchase AWS RDS Reserved Instances |
+| `cudly_aws_elasticache_ri_purchase` | false | true | false | true | Purchase AWS ElastiCache Reserved Cache Nodes |
+| `cudly_aws_opensearch_ri_purchase` | false | true | false | true | Purchase AWS OpenSearch Reserved Instances |
+| `cudly_aws_redshift_ri_purchase` | false | true | false | true | Purchase AWS Redshift Reserved Instances |
+| `cudly_aws_memorydb_ri_purchase` | false | true | false | true | Purchase AWS MemoryDB Reserved Instances |
+| `cudly_aws_savingsplans_purchase` | false | true | false | true | Purchase an AWS Savings Plan |
+| `cudly_azure_compute_ri_purchase` | false | true | false | true | Purchase an Azure VM Reserved Instance |
+| `cudly_gcp_computeengine_cud_purchase` | false | true | false | true | Purchase a GCP Compute Engine Committed Use Discount |
+
+`cudly_list_commitment_actions` is the one `OpenWorldHint=false` tool in the
+server: it only ever reads the in-process `Descriptor` slice `NewServer`
+builds at startup, never a live cloud API, unlike
+`cudly_search_recommendations` (reaches Cost Explorer/Advisor/Recommender)
+and every purchase tool (reaches a live provider purchase API).
+
+## Enforcement
+
+`Descriptor.Annotations` is a plain, optional `*mcp.ToolAnnotations` field --
+nothing at the Go type level stops a new tool from leaving it nil or
+building one by hand instead of calling `readOnlyAnnotations`/
+`purchaseAnnotations`, so none of this is compiler-enforced. The guarantee
+is carried entirely by `mcp/annotations_test.go`, run in CI on every push:
+
+- `TestToolAnnotationsMatchDescriptor` is the drift test: it drives a real
+  `ListTools` call over the in-memory MCP transport and asserts, in both
+  directions, that the live `Annotations` on the wire equal each tool's
+  `Descriptor().Annotations` -- tool counts must match, every live tool
+  must have a matching `Descriptor`, and every `Descriptor` must have a
+  matching live tool. The bidirectional check matters because a tool
+  registered directly against `*gosdk.Server` outside the
+  `Descriptor`/`Register` contract would never appear in the descriptor
+  side of a one-directional comparison, and could ship unsafe or missing
+  annotations without this test noticing.
+- `TestToolAnnotationsValueAssertions` is a table-driven sweep, derived from
+  `Descriptor.Action` rather than a hardcoded tool-name list (so a future
+  12th purchase tool inherits the check automatically), asserting the
+  concrete required value of every hint per role, not just that a value is
+  present: `Annotations`/`DestructiveHint`/`OpenWorldHint` are never nil;
+  every tool whose `Action` contains `"purchase"` is `ReadOnlyHint=false`,
+  `DestructiveHint=true` (D1), `IdempotentHint=false` (A5), and
+  `OpenWorldHint=true`; the search tool (`Action == "search"`) is
+  `ReadOnlyHint=true`, `DestructiveHint=false`, `OpenWorldHint=true`; the
+  catalog tool (the one remaining role, `Action == ""`) is
+  `ReadOnlyHint=true`, `DestructiveHint=false`, `OpenWorldHint=false`; and
+  every `Title` is non-empty, differs from the tool's snake_case `Name`, and
+  contains no underscore. Asserting concrete values (not just "is set")
+  matters because `TestToolAnnotationsMatchDescriptor` alone would pass a
+  purchase tool that mistakenly set `IdempotentHint=true`, as long as
+  `Descriptor` and the live registration agreed on that same wrong value.

mcp/annotations_test.go

@@ -0,0 +1,175 @@
+package mcp
+
+import (
+	"context"
+	"reflect"
+	"strings"
+	"testing"
+
+	gosdk "github.com/modelcontextprotocol/go-sdk/mcp"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/CUDly/mcp/tools"
+)
+
+// allDescriptors returns the Descriptor for every tool this server exposes,
+// including cudly_list_commitment_actions -- the same set NewServer builds,
+// so tests here cover every registered tool without hand-maintaining a
+// second list that could drift from the real registry.
+func allDescriptors() []tools.Descriptor {
+	regs := registrations()
+	descriptors := make([]tools.Descriptor, 0, len(regs)+1)
+	for _, r := range regs {
+		descriptors = append(descriptors, r.Descriptor())
+	}
+	descriptors = append(descriptors, tools.ListCommitmentActionsDescriptor())
+	return descriptors
+}
+
+// connectTestServer builds a real NewServer, connects a real gosdk.Client to
+// it over an in-memory transport, and returns the live session -- the same
+// end-to-end wiring TestEndToEndSearchThenDryRunPurchase already exercises,
+// factored out here so both annotation tests below can drive an actual
+// ListTools round trip instead of asserting against Descriptor alone.
+func connectTestServer(t *testing.T) *gosdk.ClientSession {
+	t.Helper()
+	ctx := context.Background()
+
+	server, err := NewServer("test")
+	require.NoError(t, err)
+
+	clientTransport, serverTransport := gosdk.NewInMemoryTransports()
+	go func() {
+		_ = server.Run(ctx, serverTransport)
+	}()
+
+	client := gosdk.NewClient(&gosdk.Implementation{Name: "test-client"}, nil)
+	session, err := client.Connect(ctx, clientTransport, nil)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = session.Close() })
+	return session
+}
+
+// TestToolAnnotationsMatchDescriptor is the A9 drift test: it proves the
+// Annotations the live MCP protocol advertises via ListTools are exactly
+// the Annotations each tool's Descriptor() reports, for every registered
+// tool -- in both directions. Descriptor.Annotations and each Register()'s
+// mcp.Tool.Annotations are two independent call sites (the same duplication
+// Description already has) -- nothing in the Go type system stops them from
+// diverging, so this is the regression guard. The tool-count and reverse
+// (live-to-descriptor) checks matter because the per-descriptor loop alone
+// only proves "every descriptor has a live match" -- a tool registered
+// directly against *gosdk.Server outside the Descriptor/Register contract
+// (so it never appears in allDescriptors()) could still ship with unsafe or
+// missing annotations and this test would stay green without them.
+func TestToolAnnotationsMatchDescriptor(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	session := connectTestServer(t)
+
+	toolsList, err := session.ListTools(ctx, nil)
+	require.NoError(t, err)
+
+	live := make(map[string]*gosdk.ToolAnnotations, len(toolsList.Tools))
+	for _, tl := range toolsList.Tools {
+		live[tl.Name] = tl.Annotations
+	}
+
+	descriptors := allDescriptors()
+	require.Lenf(t, toolsList.Tools, len(descriptors),
+		"ListTools returned %d tools but the descriptor registry has %d entries -- a tool registered "+
+			"outside Descriptor/Register would only surface here, not in the per-name checks below",
+		len(toolsList.Tools), len(descriptors))
+
+	descriptorNames := make(map[string]bool, len(descriptors))
+	for _, d := range descriptors {
+		descriptorNames[d.Name] = true
+	}
+	for name := range live {
+		assert.Truef(t, descriptorNames[name],
+			"tool %q is registered on the live server but has no matching Descriptor entry", name)
+	}
+
+	for _, d := range descriptors {
+		liveAnnotations, ok := live[d.Name]
+		require.Truef(t, ok, "tool %q from the descriptor registry was not returned by ListTools", d.Name)
+		assert.Truef(t, reflect.DeepEqual(d.Annotations, liveAnnotations),
+			"tool %q: ListTools annotations %+v != Descriptor annotations %+v", d.Name, liveAnnotations, d.Annotations)
+	}
+}
+
+// TestToolAnnotationsValueAssertions is the A10 value-assertion test: a
+// table-driven sweep over every tool's Descriptor (not a hardcoded tool-name
+// list, so a future 12th purchase tool cannot dodge it by omission) that
+// checks every hint's actual value, not just that it is set. A structural
+// nil-check alone would pass a purchase tool that mistakenly claims
+// IdempotentHint=true or OpenWorldHint=false, or a catalog tool that claims
+// OpenWorldHint=true, as long as Descriptor and the live registration agree
+// on the same wrong value -- TestToolAnnotationsMatchDescriptor only proves
+// the two never drift apart, not that either is correct.
+//
+// The three-way switch below covers every read-only/purchase role this
+// server has today: a tool whose Action contains "purchase" (9 tools), the
+// search tool (Action == "search", the only other role that sets Action),
+// and cudly_list_commitment_actions (Action left as its zero value ""), the
+// one in-process, closed-world catalog tool. Adding a role with a fourth
+// annotation profile (e.g. a future audit/server-info tool) will need a new
+// case here rather than falling through the default.
+func TestToolAnnotationsValueAssertions(t *testing.T) {
+	t.Parallel()
+
+	for _, d := range allDescriptors() {
+		d := d
+		t.Run(d.Name, func(t *testing.T) {
+			t.Parallel()
+
+			require.NotNilf(t, d.Annotations, "tool %q must set Annotations explicitly: a nil Annotations "+
+				"publishes destructive=true/openWorld=true/readOnly=false on the wire per the MCP spec", d.Name)
+			require.NotNilf(t, d.Annotations.DestructiveHint, "tool %q must set DestructiveHint explicitly "+
+				"(nil defaults to true on the wire)", d.Name)
+			require.NotNilf(t, d.Annotations.OpenWorldHint, "tool %q must set OpenWorldHint explicitly "+
+				"(nil defaults to true on the wire)", d.Name)
+
+			switch {
+			case strings.Contains(d.Action, "purchase"):
+				assert.Falsef(t, d.Annotations.ReadOnlyHint,
+					"tool %q has a purchase action (%q) so must be ReadOnlyHint=false", d.Name, d.Action)
+				assert.Truef(t, *d.Annotations.DestructiveHint,
+					"tool %q has a purchase action (%q) so must be DestructiveHint=true (D1)", d.Name, d.Action)
+				assert.Falsef(t, d.Annotations.IdempotentHint,
+					"tool %q has a purchase action (%q) so must be IdempotentHint=false (A5): "+
+						"idempotency_nonce is caller-supplied, so a claimed-idempotent purchase tool invites a "+
+						"client to \"safely\" retry into a duplicate real purchase", d.Name, d.Action)
+				assert.Truef(t, *d.Annotations.OpenWorldHint,
+					"tool %q has a purchase action (%q) so must be OpenWorldHint=true: every purchase reaches "+
+						"a live cloud provider API", d.Name, d.Action)
+			case d.Action == "search":
+				assert.Truef(t, d.Annotations.ReadOnlyHint, "tool %q is the search tool so must be ReadOnlyHint=true", d.Name)
+				assert.Falsef(t, *d.Annotations.DestructiveHint,
+					"tool %q is the search tool so must be DestructiveHint=false", d.Name)
+				assert.Falsef(t, d.Annotations.IdempotentHint,
+					"tool %q is the search tool so must be IdempotentHint=false: each search bills Cost Explorer "+
+						"per request, so claiming idempotency invites a free-retry billing loop", d.Name)
+				assert.Truef(t, *d.Annotations.OpenWorldHint,
+					"tool %q is the search tool so must be OpenWorldHint=true: it reaches a live Cost Explorer/"+
+						"Advisor/Recommender API", d.Name)
+			default:
+				// The one remaining role today is cudly_list_commitment_actions, the
+				// in-process catalog: closed-world by construction (it only ever reads
+				// the descriptors slice NewServer built at startup, never a live cloud API).
+				assert.Truef(t, d.Annotations.ReadOnlyHint, "tool %q must be ReadOnlyHint=true", d.Name)
+				assert.Falsef(t, *d.Annotations.DestructiveHint, "tool %q must be DestructiveHint=false", d.Name)
+				assert.Falsef(t, d.Annotations.IdempotentHint, "tool %q must be IdempotentHint=false", d.Name)
+				assert.Falsef(t, *d.Annotations.OpenWorldHint,
+					"tool %q is the in-process catalog tool so must be OpenWorldHint=false", d.Name)
+			}
+
+			assert.NotEmptyf(t, d.Annotations.Title, "tool %q must set a human-readable Title", d.Name)
+			assert.NotEqualf(t, d.Name, d.Annotations.Title,
+				"tool %q Title must differ from the snake_case tool name", d.Name)
+			assert.NotContainsf(t, d.Annotations.Title, "_",
+				"tool %q Title %q must be human-readable prose, not a snake_case identifier", d.Name, d.Annotations.Title)
+		})
+	}
+}

mcp/tools/aws_ec2_ri.go

@@ -13,6 +13,10 @@ import (
 
 const awsEC2RIPurchaseName = "cudly_aws_ec2_ri_purchase"
 
+const awsEC2RIPurchaseTitle = "Purchase AWS EC2 Reserved Instances"
+
+var awsEC2RIPurchaseAnnotations = purchaseAnnotations(awsEC2RIPurchaseTitle)
+
 const awsEC2RIPurchaseDescription = "Purchase AWS EC2 Reserved Instances. THIS SPENDS REAL MONEY when " +
 	"dry_run=false and confirm=true. Always call with dry_run=true first (the default) to validate your " +
 	"parameters before committing; a dry_run response never contacts AWS and never spends money. Search first " +
@@ -56,6 +60,7 @@ func (t *awsEC2RIPurchaseTool) Descriptor() Descriptor {
 		Product:             "ec2",
 		Action:              "ri_purchase",
 		Description:         awsEC2RIPurchaseDescription,
+		Annotations:         awsEC2RIPurchaseAnnotations,
 		RealPurchaseEnabled: true,
 		ExamplePrompts: []string{
 			"Preview buying 3 m5.large 3-year no-upfront RIs in us-east-1",
@@ -79,6 +84,7 @@ func (t *awsEC2RIPurchaseTool) Register(s *mcp.Server) error {
 	mcp.AddTool(s, &mcp.Tool{
 		Name:        awsEC2RIPurchaseName,
 		Description: awsEC2RIPurchaseDescription,
+		Annotations: awsEC2RIPurchaseAnnotations,
 		InputSchema: schema,
 	}, t.handle)
 	return nil

mcp/tools/aws_ec2_ri_test.go

@@ -11,8 +11,6 @@ import (
 	"github.com/LeanerCloud/CUDly/pkg/provider"
 )
 
-func boolPtr(b bool) *bool { return &b }
-
 func validEC2Args() ec2RIPurchaseArgs {
 	return ec2RIPurchaseArgs{
 		AWSProfile:    "test-profile",

mcp/tools/aws_elasticache_ri.go

@@ -12,6 +12,10 @@ import (
 
 const awsElastiCacheRIPurchaseName = "cudly_aws_elasticache_ri_purchase"
 
+const awsElastiCacheRIPurchaseTitle = "Purchase AWS ElastiCache Reserved Cache Nodes"
+
+var awsElastiCacheRIPurchaseAnnotations = purchaseAnnotations(awsElastiCacheRIPurchaseTitle)
+
 const awsElastiCacheRIPurchaseDescription = "Purchase AWS ElastiCache Reserved Cache Nodes. THIS SPENDS REAL " +
 	"MONEY when dry_run=false and confirm=true. Always call with dry_run=true first (the default) to validate " +
 	"your parameters before committing; a dry_run response never contacts AWS and never spends money."
@@ -49,6 +53,7 @@ func (t *awsElastiCacheRIPurchaseTool) Descriptor() Descriptor {
 		Product:             "elasticache",
 		Action:              "ri_purchase",
 		Description:         awsElastiCacheRIPurchaseDescription,
+		Annotations:         awsElastiCacheRIPurchaseAnnotations,
 		RealPurchaseEnabled: true,
 		ExamplePrompts: []string{
 			"Preview buying 3 cache.r6g.large redis ElastiCache RIs in us-east-1 for 1 year",
@@ -71,6 +76,7 @@ func (t *awsElastiCacheRIPurchaseTool) Register(s *mcp.Server) error {
 	mcp.AddTool(s, &mcp.Tool{
 		Name:        awsElastiCacheRIPurchaseName,
 		Description: awsElastiCacheRIPurchaseDescription,
+		Annotations: awsElastiCacheRIPurchaseAnnotations,
 		InputSchema: schema,
 	}, t.handle)
 	return nil