Add validation tests for tools, resources, and prompts metadata

This commit adds comprehensive validation tests to ensure all MCP items
have required metadata:

- TestAllToolsHaveRequiredMetadata: Validates Toolset.ID and Annotations
- TestAllToolsHaveValidToolsetID: Ensures toolsets are in AvailableToolsets()
- TestAllResourcesHaveRequiredMetadata: Validates resource metadata
- TestAllPromptsHaveRequiredMetadata: Validates prompt metadata
- TestToolReadOnlyHintConsistency: Validates IsReadOnly() matches annotation
- TestNoDuplicate*Names: Ensures unique names across tools/resources/prompts
- TestAllToolsHaveHandlerFunc: Ensures all tools have handlers
- TestDefaultToolsetsAreValid: Validates default toolset IDs
- TestToolsetMetadataConsistency: Ensures consistent descriptions per toolset

Also fixes a bug discovered by these tests: ToolsetMetadataGit was defined
but not added to AvailableToolsets(), causing get_repository_tree to have
an invalid toolset ID.

Author: SamMorrowDrums

pkg/github/tools.go

@@ -111,6 +111,7 @@ func AvailableToolsets() []toolsets.ToolsetMetadata {
 	return []toolsets.ToolsetMetadata{
 		ToolsetMetadataContext,
 		ToolsetMetadataRepos,
+		ToolsetMetadataGit,
 		ToolsetMetadataIssues,
 		ToolsetMetadataPullRequests,
 		ToolsetMetadataUsers,

pkg/github/tools_validation_test.go

@@ -0,0 +1,219 @@
+package github
+
+import (
+	"testing"
+
+	"github.com/github/github-mcp-server/pkg/toolsets"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// stubTranslation is a simple translation function for testing
+func stubTranslation(_, fallback string) string {
+	return fallback
+}
+
+// TestAllToolsHaveRequiredMetadata validates that all tools have mandatory metadata:
+// - Toolset must be set (non-empty ID)
+// - ReadOnlyHint annotation must be explicitly set (not nil)
+func TestAllToolsHaveRequiredMetadata(t *testing.T) {
+	tools := AllTools(stubTranslation)
+
+	require.NotEmpty(t, tools, "AllTools should return at least one tool")
+
+	for _, tool := range tools {
+		t.Run(tool.Tool.Name, func(t *testing.T) {
+			// Toolset ID must be set
+			assert.NotEmpty(t, tool.Toolset.ID,
+				"Tool %q must have a Toolset.ID", tool.Tool.Name)
+
+			// Toolset description should be set for documentation
+			assert.NotEmpty(t, tool.Toolset.Description,
+				"Tool %q should have a Toolset.Description", tool.Tool.Name)
+
+			// Annotations must exist and have ReadOnlyHint explicitly set
+			require.NotNil(t, tool.Tool.Annotations,
+				"Tool %q must have Annotations set (for ReadOnlyHint)", tool.Tool.Name)
+
+			// We can't distinguish between "not set" and "set to false" for a bool,
+			// but having Annotations non-nil confirms the developer thought about it.
+			// The ReadOnlyHint value itself is validated by ensuring Annotations exist.
+		})
+	}
+}
+
+// TestAllToolsHaveValidToolsetID validates that all tools belong to known toolsets
+func TestAllToolsHaveValidToolsetID(t *testing.T) {
+	tools := AllTools(stubTranslation)
+	validToolsetIDs := GetValidToolsetIDs()
+
+	for _, tool := range tools {
+		t.Run(tool.Tool.Name, func(t *testing.T) {
+			assert.True(t, validToolsetIDs[tool.Toolset.ID],
+				"Tool %q has invalid Toolset.ID %q - must be one of the defined toolsets",
+				tool.Tool.Name, tool.Toolset.ID)
+		})
+	}
+}
+
+// TestAllResourcesHaveRequiredMetadata validates that all resources have mandatory metadata
+func TestAllResourcesHaveRequiredMetadata(t *testing.T) {
+	// Resources need client functions, but we can pass nil for validation
+	// since we're not actually calling handlers
+	resources := AllResources(stubTranslation, nil, nil)
+
+	require.NotEmpty(t, resources, "AllResources should return at least one resource")
+
+	for _, res := range resources {
+		t.Run(res.Template.Name, func(t *testing.T) {
+			// Toolset ID must be set
+			assert.NotEmpty(t, res.Toolset.ID,
+				"Resource %q must have a Toolset.ID", res.Template.Name)
+
+			// Handler must be set
+			assert.NotNil(t, res.Handler,
+				"Resource %q must have a Handler", res.Template.Name)
+		})
+	}
+}
+
+// TestAllPromptsHaveRequiredMetadata validates that all prompts have mandatory metadata
+func TestAllPromptsHaveRequiredMetadata(t *testing.T) {
+	prompts := AllPrompts(stubTranslation)
+
+	require.NotEmpty(t, prompts, "AllPrompts should return at least one prompt")
+
+	for _, prompt := range prompts {
+		t.Run(prompt.Prompt.Name, func(t *testing.T) {
+			// Toolset ID must be set
+			assert.NotEmpty(t, prompt.Toolset.ID,
+				"Prompt %q must have a Toolset.ID", prompt.Prompt.Name)
+
+			// Handler must be set
+			assert.NotNil(t, prompt.Handler,
+				"Prompt %q must have a Handler", prompt.Prompt.Name)
+		})
+	}
+}
+
+// TestAllResourcesHaveValidToolsetID validates that all resources belong to known toolsets
+func TestAllResourcesHaveValidToolsetID(t *testing.T) {
+	resources := AllResources(stubTranslation, nil, nil)
+	validToolsetIDs := GetValidToolsetIDs()
+
+	for _, res := range resources {
+		t.Run(res.Template.Name, func(t *testing.T) {
+			assert.True(t, validToolsetIDs[res.Toolset.ID],
+				"Resource %q has invalid Toolset.ID %q", res.Template.Name, res.Toolset.ID)
+		})
+	}
+}
+
+// TestAllPromptsHaveValidToolsetID validates that all prompts belong to known toolsets
+func TestAllPromptsHaveValidToolsetID(t *testing.T) {
+	prompts := AllPrompts(stubTranslation)
+	validToolsetIDs := GetValidToolsetIDs()
+
+	for _, prompt := range prompts {
+		t.Run(prompt.Prompt.Name, func(t *testing.T) {
+			assert.True(t, validToolsetIDs[prompt.Toolset.ID],
+				"Prompt %q has invalid Toolset.ID %q", prompt.Prompt.Name, prompt.Toolset.ID)
+		})
+	}
+}
+
+// TestToolReadOnlyHintConsistency validates that read-only tools are correctly annotated
+func TestToolReadOnlyHintConsistency(t *testing.T) {
+	tools := AllTools(stubTranslation)
+
+	for _, tool := range tools {
+		t.Run(tool.Tool.Name, func(t *testing.T) {
+			require.NotNil(t, tool.Tool.Annotations,
+				"Tool %q must have Annotations", tool.Tool.Name)
+
+			// Verify IsReadOnly() method matches the annotation
+			assert.Equal(t, tool.Tool.Annotations.ReadOnlyHint, tool.IsReadOnly(),
+				"Tool %q: IsReadOnly() should match Annotations.ReadOnlyHint", tool.Tool.Name)
+		})
+	}
+}
+
+// TestNoDuplicateToolNames ensures all tools have unique names
+func TestNoDuplicateToolNames(t *testing.T) {
+	tools := AllTools(stubTranslation)
+	seen := make(map[string]bool)
+
+	for _, tool := range tools {
+		name := tool.Tool.Name
+		assert.False(t, seen[name],
+			"Duplicate tool name found: %q", name)
+		seen[name] = true
+	}
+}
+
+// TestNoDuplicateResourceNames ensures all resources have unique names
+func TestNoDuplicateResourceNames(t *testing.T) {
+	resources := AllResources(stubTranslation, nil, nil)
+	seen := make(map[string]bool)
+
+	for _, res := range resources {
+		name := res.Template.Name
+		assert.False(t, seen[name],
+			"Duplicate resource name found: %q", name)
+		seen[name] = true
+	}
+}
+
+// TestNoDuplicatePromptNames ensures all prompts have unique names
+func TestNoDuplicatePromptNames(t *testing.T) {
+	prompts := AllPrompts(stubTranslation)
+	seen := make(map[string]bool)
+
+	for _, prompt := range prompts {
+		name := prompt.Prompt.Name
+		assert.False(t, seen[name],
+			"Duplicate prompt name found: %q", name)
+		seen[name] = true
+	}
+}
+
+// TestAllToolsHaveHandlerFunc ensures all tools have a handler function
+func TestAllToolsHaveHandlerFunc(t *testing.T) {
+	tools := AllTools(stubTranslation)
+
+	for _, tool := range tools {
+		t.Run(tool.Tool.Name, func(t *testing.T) {
+			assert.NotNil(t, tool.HandlerFunc,
+				"Tool %q must have a HandlerFunc", tool.Tool.Name)
+		})
+	}
+}
+
+// TestDefaultToolsetsAreValid ensures default toolset IDs are all valid
+func TestDefaultToolsetsAreValid(t *testing.T) {
+	defaults := GetDefaultToolsetIDs()
+	valid := GetValidToolsetIDs()
+
+	for _, id := range defaults {
+		assert.True(t, valid[id],
+			"Default toolset ID %q is not in the valid toolset list", id)
+	}
+}
+
+// TestToolsetMetadataConsistency ensures tools in the same toolset have consistent descriptions
+func TestToolsetMetadataConsistency(t *testing.T) {
+	tools := AllTools(stubTranslation)
+	toolsetDescriptions := make(map[toolsets.ToolsetID]string)
+
+	for _, tool := range tools {
+		id := tool.Toolset.ID
+		desc := tool.Toolset.Description
+
+		if existing, ok := toolsetDescriptions[id]; ok {
+			assert.Equal(t, existing, desc,
+				"Toolset %q has inconsistent descriptions across tools", id)
+		} else {
+			toolsetDescriptions[id] = desc
+		}
+	}
+}