fix: refine MCP tool annotations

Author: freeznet

PLAN.md

@@ -4,6 +4,39 @@
 
 Prepare StreamNative MCP Server for Claude connector submission and review.
 
+## Current review follow-up: annotation helper granularity
+
+Reviewer feedback is valid: `toolannotations.ReadOnly` and `toolannotations.Destructive` currently call `mcp.WithToolAnnotation(...)`, replacing the whole annotation struct and dropping mcp-go defaults for `idempotentHint` and `openWorldHint`. The helper also collapses all side effects into `Destructive`, so local session mutations and reversible lifecycle operations look equivalent to delete/apply operations.
+
+Implementation status: implemented in current work.
+
+1. Replaced whole-annotation assignment helpers with composed field setters so unspecified hints keep mcp-go defaults unless explicitly changed.
+2. Exposed helper categories:
+   - `ReadOnly(title)` for non-mutating external reads: read-only true, destructive false, idempotent true, open-world true.
+   - `ExternalRead(title)` as explicit alias for external read tools when caller intent should be obvious.
+   - `LocalSessionMutation(title)` for session/context state changes: read-only false, destructive false, idempotent false by default, open-world false.
+   - `Mutating(title, destructive, idempotent)` for external writes/side effects: read-only false, caller-controlled destructive/idempotent, open-world true.
+3. Kept `Destructive(title)` as compatibility wrapper over `Mutating(title, true, false)` while migrating call sites to precise helpers.
+4. Updated context tools to use `LocalSessionMutation`; updated operation-mode helper to derive destructive/idempotent from `OperationSpec` where available instead of treating all writes as destructive.
+5. Extended annotation tests to assert `idempotentHint` and `openWorldHint` for static/context/builder tools, not only read/destructive.
+
+Risks:
+
+- Tool annotation surface changes are runtime-visible to MCP clients and Claude review.
+- Incorrect idempotency classification can mislead clients; delete/remove/reset may be idempotent only depending backend behavior.
+- Broad migration across all builders risks noisy diff; recommended first patch helper + high-signal call sites, then operation registry cleanup separately.
+
+Recommended validation:
+
+```bash
+go test -race ./pkg/mcp/... ./pkg/mcp/builders/... ./pkg/mcp/pftools/...
+go test -race ./...
+go fmt ./...
+go mod tidy
+golangci-lint run --timeout=3m
+make build
+```
+
 ## Current review follow-up: operation spec registry
 
 Reviewer feedback is valid: current branch split read/write tool names, but duplicated `toolMode` helpers and per-builder write-operation maps still create scatter-shot maintenance. Adding one operation can require enum updates, write map updates, handler switch updates, docs, and compliance-test classification. `validateModeOperation` also classifies any operation missing from the write map as read, so an unclassified future write can pass read-mode validation until the handler switch rejects or mishandles it.

pkg/mcp/builders/kafka/annotation_compliance_test.go

@@ -54,17 +54,23 @@ func TestKafkaToolAnnotationCompliance(t *testing.T) {
 			require.NotEmpty(t, tool.Annotations.Title, tool.Name)
 			require.NotNil(t, tool.Annotations.ReadOnlyHint, tool.Name)
 			require.NotNil(t, tool.Annotations.DestructiveHint, tool.Name)
+			require.NotNil(t, tool.Annotations.IdempotentHint, tool.Name)
+			require.NotNil(t, tool.Annotations.OpenWorldHint, tool.Name)
 			require.LessOrEqual(t, len(tool.Name), 64, tool.Name)
 
 			isRead := strings.HasSuffix(tool.Name, "_read")
 			isWrite := strings.HasSuffix(tool.Name, "_write") || strings.Contains(tool.Name, "produce") || strings.Contains(tool.Name, "consume")
 			if isRead {
 				require.True(t, *tool.Annotations.ReadOnlyHint, tool.Name)
 				require.False(t, *tool.Annotations.DestructiveHint, tool.Name)
+				require.True(t, *tool.Annotations.IdempotentHint, tool.Name)
+				require.True(t, *tool.Annotations.OpenWorldHint, tool.Name)
 			}
 			if isWrite {
 				require.False(t, *tool.Annotations.ReadOnlyHint, tool.Name)
 				require.True(t, *tool.Annotations.DestructiveHint, tool.Name)
+				require.False(t, *tool.Annotations.IdempotentHint, tool.Name)
+				require.True(t, *tool.Annotations.OpenWorldHint, tool.Name)
 			}
 			assertOperationEnumMode(t, tool.Name, tool.InputSchema.Properties["operation"])
 		}

pkg/mcp/builders/kafka/connect.go

@@ -113,7 +113,7 @@ func (b *KafkaConnectToolBuilder) buildKafkaConnectTool(mode toolMode) mcp.Tool
 		"- get: Retrieve detailed information about a Kafka Connect cluster or specific connector."
 	operationEnum := kafkaConnectOperationSpecs.NamesForMode(mode)
 	toolName := "kafka_admin_connect_read"
-	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Connect", "Manage Kafka Connect")
+	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Connect", "Manage Kafka Connect", kafkaConnectOperationSpecs)
 	if isToolModeWrite(mode) {
 		resourceDesc = "Resource to operate on. Available resources:\n" +
 			"- connector: A single Kafka Connect connector instance that moves data between Kafka and external systems."

pkg/mcp/builders/kafka/groups.go

@@ -104,7 +104,7 @@ func (b *KafkaGroupsToolBuilder) buildKafkaGroupsTool(mode toolMode) mcp.Tool {
 		"- offsets: Get offsets for a specific consumer group"
 	operationEnum := kafkaGroupOperationSpecs.NamesForMode(mode)
 	toolName := "kafka_admin_groups_read"
-	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Consumer Groups", "Manage Kafka Consumer Groups")
+	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Consumer Groups", "Manage Kafka Consumer Groups", kafkaGroupOperationSpecs)
 	if isToolModeWrite(mode) {
 		operationDesc = "Operation to perform. Available operations:\n" +
 			"- remove-members: Remove specific members from a Consumer Group to force rebalancing or troubleshoot issues\n" +

pkg/mcp/builders/kafka/schema_registry.go

@@ -108,7 +108,7 @@ func (b *KafkaSchemaRegistryToolBuilder) buildKafkaSchemaRegistryTool(mode toolM
 		"- get: Get a subject's latest schema, a specific version, or compatibility level"
 	operationEnum := kafkaSchemaRegistryOperationSpecs.NamesForMode(mode)
 	toolName := "kafka_admin_sr_read"
-	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Schema Registry", "Manage Kafka Schema Registry")
+	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Schema Registry", "Manage Kafka Schema Registry", kafkaSchemaRegistryOperationSpecs)
 	if isToolModeWrite(mode) {
 		resourceDesc = "Resource to operate on. Available resources:\n" +
 			"- subject: A specific schema subject to register or delete\n" +

pkg/mcp/builders/kafka/topics.go

@@ -103,7 +103,7 @@ func (b *KafkaTopicsToolBuilder) buildKafkaTopicsTool(mode toolMode) mcp.Tool {
 		"- metadata: Get metadata for a specific topic\n"
 	operationEnum := kafkaTopicOperationSpecs.NamesForMode(mode)
 	toolName := "kafka_admin_topics_read"
-	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Topics", "Manage Kafka Topics")
+	annotation := builders.ToolAnnotationForMode(mode, "Read Kafka Topics", "Manage Kafka Topics", kafkaTopicOperationSpecs)
 	if isToolModeWrite(mode) {
 		operationDesc = "Operation to perform. Available operations:\n" +
 			"- create: Create a new topic with specified partitions, replication factor, and optional configs\n" +

pkg/mcp/builders/operation_annotations.go

@@ -21,10 +21,36 @@ import (
 
 // ToolAnnotationForMode selects Claude connector safety annotations from the
 // operation mode so schemas, validation, and annotations share the same mode
-// vocabulary.
-func ToolAnnotationForMode(mode OperationMode, readTitle, writeTitle string) mcp.ToolOption {
-	if mode == OperationModeWrite {
-		return toolannotations.Destructive(writeTitle)
+// vocabulary. If operation metadata is supplied, write-tool destructive and
+// idempotent hints are derived from the write specs instead of from mode alone.
+func ToolAnnotationForMode(mode OperationMode, readTitle, writeTitle string, registries ...OperationRegistry) mcp.ToolOption {
+	if mode != OperationModeWrite {
+		return toolannotations.ExternalRead(readTitle)
 	}
-	return toolannotations.ReadOnly(readTitle)
+
+	destructive, idempotent := writeAnnotationHints(registries...)
+	return toolannotations.Mutating(writeTitle, destructive, idempotent)
+}
+
+func writeAnnotationHints(registries ...OperationRegistry) (destructive bool, idempotent bool) {
+	if len(registries) == 0 {
+		return true, false
+	}
+
+	foundWriteSpec := false
+	allIdempotent := true
+	for _, registry := range registries {
+		for _, spec := range registry.SpecsForMode(OperationModeWrite) {
+			foundWriteSpec = true
+			destructive = destructive || spec.Destructive
+			allIdempotent = allIdempotent && spec.Idempotent
+		}
+	}
+	if !foundWriteSpec {
+		return true, false
+	}
+
+	// A multi-operation tool is idempotent only when every exposed write
+	// operation is idempotent.
+	return destructive, allIdempotent
 }

pkg/mcp/builders/operations.go

@@ -68,14 +68,24 @@ func (r OperationRegistry) Names() []string {
 	return names
 }
 
-// NamesForMode returns operation names for one mode in registry order.
-func (r OperationRegistry) NamesForMode(mode OperationMode) []string {
-	names := make([]string, 0, len(r))
+// SpecsForMode returns operation specs for one mode in registry order.
+func (r OperationRegistry) SpecsForMode(mode OperationMode) []OperationSpec {
+	specs := make([]OperationSpec, 0, len(r))
 	for _, spec := range r {
 		if spec.Mode == mode {
-			names = append(names, spec.Name)
+			specs = append(specs, spec)
 		}
 	}
+	return specs
+}
+
+// NamesForMode returns operation names for one mode in registry order.
+func (r OperationRegistry) NamesForMode(mode OperationMode) []string {
+	specs := r.SpecsForMode(mode)
+	names := make([]string, 0, len(specs))
+	for _, spec := range specs {
+		names = append(names, spec.Name)
+	}
 	return names
 }
 

pkg/mcp/builders/operations_test.go

@@ -37,21 +37,34 @@ func TestOperationRegistryModeEnumsAndValidation(t *testing.T) {
 }
 
 func TestToolAnnotationForMode(t *testing.T) {
-	read := ToolAnnotationForMode(OperationModeRead, "Read Things", "Manage Things")
-	write := ToolAnnotationForMode(OperationModeWrite, "Read Things", "Manage Things")
+	registry := OperationRegistry{
+		{Name: "list", Mode: OperationModeRead},
+		{Name: "update", Mode: OperationModeWrite, Destructive: false, Idempotent: true},
+	}
+
+	read := ToolAnnotationForMode(OperationModeRead, "Read Things", "Manage Things", registry)
+	write := ToolAnnotationForMode(OperationModeWrite, "Read Things", "Manage Things", registry)
 
 	readTool := mcp.NewTool("read", read)
 	writeTool := mcp.NewTool("write", write)
 
 	require.Equal(t, "Read Things", readTool.Annotations.Title)
 	require.NotNil(t, readTool.Annotations.ReadOnlyHint)
 	require.NotNil(t, readTool.Annotations.DestructiveHint)
+	require.NotNil(t, readTool.Annotations.IdempotentHint)
+	require.NotNil(t, readTool.Annotations.OpenWorldHint)
 	require.True(t, *readTool.Annotations.ReadOnlyHint)
 	require.False(t, *readTool.Annotations.DestructiveHint)
+	require.True(t, *readTool.Annotations.IdempotentHint)
+	require.True(t, *readTool.Annotations.OpenWorldHint)
 
 	require.Equal(t, "Manage Things", writeTool.Annotations.Title)
 	require.NotNil(t, writeTool.Annotations.ReadOnlyHint)
 	require.NotNil(t, writeTool.Annotations.DestructiveHint)
+	require.NotNil(t, writeTool.Annotations.IdempotentHint)
+	require.NotNil(t, writeTool.Annotations.OpenWorldHint)
 	require.False(t, *writeTool.Annotations.ReadOnlyHint)
-	require.True(t, *writeTool.Annotations.DestructiveHint)
+	require.False(t, *writeTool.Annotations.DestructiveHint)
+	require.True(t, *writeTool.Annotations.IdempotentHint)
+	require.True(t, *writeTool.Annotations.OpenWorldHint)
 }

pkg/mcp/builders/pulsar/annotation_compliance_test.go

@@ -46,17 +46,23 @@ func TestPulsarToolAnnotationCompliance(t *testing.T) {
 			require.NotEmpty(t, tool.Annotations.Title, tool.Name)
 			require.NotNil(t, tool.Annotations.ReadOnlyHint, tool.Name)
 			require.NotNil(t, tool.Annotations.DestructiveHint, tool.Name)
+			require.NotNil(t, tool.Annotations.IdempotentHint, tool.Name)
+			require.NotNil(t, tool.Annotations.OpenWorldHint, tool.Name)
 			require.LessOrEqual(t, len(tool.Name), 64, tool.Name)
 
 			isRead := strings.HasSuffix(tool.Name, "_read") || strings.HasPrefix(tool.Name, "pulsar_admin_namespace_policy_get") || tool.Name == "pulsar_admin_status" || tool.Name == "pulsar_admin_broker_stats" || tool.Name == "pulsar_admin_functions_worker"
 			isWrite := strings.HasSuffix(tool.Name, "_write") || strings.Contains(tool.Name, "_set") || strings.Contains(tool.Name, "_remove") || strings.Contains(tool.Name, "produce") || strings.Contains(tool.Name, "consume")
 			if isRead {
 				require.True(t, *tool.Annotations.ReadOnlyHint, tool.Name)
 				require.False(t, *tool.Annotations.DestructiveHint, tool.Name)
+				require.True(t, *tool.Annotations.IdempotentHint, tool.Name)
+				require.True(t, *tool.Annotations.OpenWorldHint, tool.Name)
 			}
 			if isWrite {
 				require.False(t, *tool.Annotations.ReadOnlyHint, tool.Name)
 				require.True(t, *tool.Annotations.DestructiveHint, tool.Name)
+				require.False(t, *tool.Annotations.IdempotentHint, tool.Name)
+				require.True(t, *tool.Annotations.OpenWorldHint, tool.Name)
 			}
 			assertOperationEnumMode(t, tool.Name, tool.InputSchema.Properties["operation"])
 		}