refactor: address PR review feedback for toolsets

- Rename AddDeprecatedToolAliases to WithDeprecatedToolAliases for
  immutable filter chain consistency (returns new ToolsetGroup)
- Remove unused mockGetRawClient from generate_docs.go (use nil instead)
- Remove legacy ServerTool functions (NewServerToolLegacy and
  NewServerToolFromHandlerLegacy) - no usages
- Add panic in Handler()/RegisterFunc() when HandlerFunc is nil
- Add HasHandler() method for checking if tool has a handler
- Add tests for HasHandler and nil handler panic behavior
- Update all tests to use new WithDeprecatedToolAliases pattern

Author: SamMorrowDrums

cmd/github-mcp-server/generate_docs.go

@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/github/github-mcp-server/pkg/github"
-	"github.com/github/github-mcp-server/pkg/raw"
 	"github.com/github/github-mcp-server/pkg/toolsets"
 	"github.com/github/github-mcp-server/pkg/translations"
 	gogithub "github.com/google/go-github/v79/github"
@@ -37,11 +36,6 @@ func mockGetClient(_ context.Context) (*gogithub.Client, error) {
 	return gogithub.NewClient(nil), nil
 }
 
-// mockGetRawClient returns a mock raw client for documentation generation
-func mockGetRawClient(_ context.Context) (*raw.Client, error) {
-	return nil, nil
-}
-
 func generateAllDocs() error {
 	if err := generateReadmeDocs("README.md"); err != nil {
 		return fmt.Errorf("failed to generate README docs: %w", err)
@@ -63,7 +57,7 @@ func generateReadmeDocs(readmePath string) error {
 	t, _ := translations.TranslationHelper()
 
 	// Create toolset group with mock clients (no deps needed for doc generation)
-	tsg := github.NewToolsetGroup(t, mockGetClient, mockGetRawClient)
+	tsg := github.NewToolsetGroup(t, mockGetClient, nil)
 
 	// Generate toolsets documentation
 	toolsetsDoc := generateToolsetsDoc(tsg)
@@ -306,7 +300,7 @@ func generateRemoteToolsetsDoc() string {
 	t, _ := translations.TranslationHelper()
 
 	// Create toolset group with mock clients
-	tsg := github.NewToolsetGroup(t, mockGetClient, mockGetRawClient)
+	tsg := github.NewToolsetGroup(t, mockGetClient, nil)
 
 	// Generate table header
 	buf.WriteString("| Name           | Description                                      | API URL                                               | 1-Click Install (VS Code)                                                                                                                                                                                                 | Read-only Link                                                                                                 | 1-Click Read-only Install (VS Code)                                                                                                                                                                                                 |\n")

internal/ghmcp/server.go

@@ -165,19 +165,17 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
 	// Create toolset group with all tools, resources, and prompts
 	tsg := github.NewToolsetGroup(cfg.Translator, getClient, getRawClient)
 
-	// Add deprecated tool aliases for backward compatibility
-	// See docs/deprecated-tool-aliases.md for the full list of renames
-	tsg.AddDeprecatedToolAliases(github.DeprecatedToolAliases)
-
 	// Clean tool names (WithTools will resolve any deprecated aliases)
 	enabledTools := github.CleanTools(cfg.EnabledTools)
 
 	// Apply filters based on configuration
+	// - WithDeprecatedToolAliases: adds backward compatibility aliases
 	// - WithReadOnly: filters out write tools when true
 	// - WithToolsets: nil=defaults, empty=none, handles "all"/"default" keywords
 	// - WithTools: additional tools that bypass toolset filtering (additive, resolves aliases)
 	// - WithFeatureChecker: filters based on feature flags
 	filteredTsg := tsg.
+		WithDeprecatedToolAliases(github.DeprecatedToolAliases).
 		WithReadOnly(cfg.ReadOnly).
 		WithToolsets(enabledToolsets).
 		WithTools(enabledTools).

pkg/github/tools_validation_test.go

@@ -185,6 +185,8 @@ func TestAllToolsHaveHandlerFunc(t *testing.T) {
 		t.Run(tool.Tool.Name, func(t *testing.T) {
 			assert.NotNil(t, tool.HandlerFunc,
 				"Tool %q must have a HandlerFunc", tool.Tool.Name)
+			assert.True(t, tool.HasHandler(),
+				"Tool %q HasHandler() should return true", tool.Tool.Name)
 		})
 	}
 }

pkg/toolsets/server_tool.go

@@ -57,17 +57,24 @@ func (st *ServerTool) IsReadOnly() bool {
 	return st.Tool.Annotations != nil && st.Tool.Annotations.ReadOnlyHint
 }
 
+// HasHandler returns true if this tool has a handler function.
+func (st *ServerTool) HasHandler() bool {
+	return st.HandlerFunc != nil
+}
+
 // Handler returns a tool handler by calling HandlerFunc with the given dependencies.
+// Panics if HandlerFunc is nil - all tools should have handlers.
 func (st *ServerTool) Handler(deps any) mcp.ToolHandler {
 	if st.HandlerFunc == nil {
-		return nil
+		panic("HandlerFunc is nil for tool: " + st.Tool.Name)
 	}
 	return st.HandlerFunc(deps)
 }
 
 // RegisterFunc registers the tool with the server using the provided dependencies.
+// Panics if the tool has no handler - all tools should have handlers.
 func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) {
-	handler := st.Handler(deps)
+	handler := st.Handler(deps) // This will panic if HandlerFunc is nil
 	s.AddTool(&st.Tool, handler)
 }
 
@@ -97,36 +104,3 @@ func NewServerTool[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, hand
 func NewServerToolFromHandler(tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandler) ServerTool {
 	return ServerTool{Tool: tool, Toolset: toolset, HandlerFunc: handlerFn}
 }
-
-// NewServerToolLegacy creates a ServerTool from a tool definition, toolset metadata, and an already-bound typed handler.
-// This is for backward compatibility during the refactor - the handler doesn't use dependencies.
-// Deprecated: Use NewServerTool instead for new code.
-func NewServerToolLegacy[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandlerFor[In, Out]) ServerTool {
-	return ServerTool{
-		Tool:    tool,
-		Toolset: toolset,
-		HandlerFunc: func(_ any) mcp.ToolHandler {
-			return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
-				var arguments In
-				if err := json.Unmarshal(req.Params.Arguments, &arguments); err != nil {
-					return nil, err
-				}
-				resp, _, err := handler(ctx, req, arguments)
-				return resp, err
-			}
-		},
-	}
-}
-
-// NewServerToolFromHandlerLegacy creates a ServerTool from a tool definition, toolset metadata, and an already-bound raw handler.
-// This is for backward compatibility during the refactor - the handler doesn't use dependencies.
-// Deprecated: Use NewServerToolFromHandler instead for new code.
-func NewServerToolFromHandlerLegacy(tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandler) ServerTool {
-	return ServerTool{
-		Tool:    tool,
-		Toolset: toolset,
-		HandlerFunc: func(_ any) mcp.ToolHandler {
-			return handler
-		},
-	}
-}

pkg/toolsets/toolsets.go

@@ -413,12 +413,19 @@ func (tg *ToolsetGroup) filterPromptsByName(name string) []ServerPrompt {
 	return []ServerPrompt{}
 }
 
-// AddDeprecatedToolAliases adds mappings from old tool names to new canonical names.
-func (tg *ToolsetGroup) AddDeprecatedToolAliases(aliases map[string]string) *ToolsetGroup {
+// WithDeprecatedToolAliases returns a new ToolsetGroup with the given deprecated aliases added.
+// Aliases map old tool names to new canonical names.
+func (tg *ToolsetGroup) WithDeprecatedToolAliases(aliases map[string]string) *ToolsetGroup {
+	newTG := tg.copy()
+	// Ensure we have a fresh map
+	newTG.deprecatedAliases = make(map[string]string, len(tg.deprecatedAliases)+len(aliases))
+	for k, v := range tg.deprecatedAliases {
+		newTG.deprecatedAliases[k] = v
+	}
 	for oldName, newName := range aliases {
-		tg.deprecatedAliases[oldName] = newName
+		newTG.deprecatedAliases[oldName] = newName
 	}
-	return tg
+	return newTG
 }
 
 // isToolsetEnabled checks if a toolset is enabled based on current filters.

pkg/toolsets/toolsets_test.go

@@ -252,22 +252,27 @@ func TestToolsForToolset(t *testing.T) {
 	}
 }
 
-func TestAddDeprecatedToolAliases(t *testing.T) {
+func TestWithDeprecatedToolAliases(t *testing.T) {
 	tools := []ServerTool{
 		mockTool("new_name", "toolset1", true),
 	}
 
 	tsg := NewToolsetGroup(tools, nil, nil)
-	tsg.AddDeprecatedToolAliases(map[string]string{
+	tsgWithAliases := tsg.WithDeprecatedToolAliases(map[string]string{
 		"old_name":  "new_name",
 		"get_issue": "issue_read",
 	})
 
-	if len(tsg.deprecatedAliases) != 2 {
-		t.Errorf("expected 2 aliases, got %d", len(tsg.deprecatedAliases))
+	// Original should be unchanged (immutable)
+	if len(tsg.deprecatedAliases) != 0 {
+		t.Errorf("original should have 0 aliases, got %d", len(tsg.deprecatedAliases))
 	}
-	if tsg.deprecatedAliases["old_name"] != "new_name" {
-		t.Errorf("expected alias 'old_name' -> 'new_name', got '%s'", tsg.deprecatedAliases["old_name"])
+
+	if len(tsgWithAliases.deprecatedAliases) != 2 {
+		t.Errorf("expected 2 aliases, got %d", len(tsgWithAliases.deprecatedAliases))
+	}
+	if tsgWithAliases.deprecatedAliases["old_name"] != "new_name" {
+		t.Errorf("expected alias 'old_name' -> 'new_name', got '%s'", tsgWithAliases.deprecatedAliases["old_name"])
 	}
 }
 
@@ -277,10 +282,10 @@ func TestResolveToolAliases(t *testing.T) {
 		mockTool("some_tool", "toolset1", true),
 	}
 
-	tsg := NewToolsetGroup(tools, nil, nil)
-	tsg.AddDeprecatedToolAliases(map[string]string{
-		"get_issue": "issue_read",
-	})
+	tsg := NewToolsetGroup(tools, nil, nil).
+		WithDeprecatedToolAliases(map[string]string{
+			"get_issue": "issue_read",
+		})
 
 	// Test resolving a mix of aliases and canonical names
 	input := []string{"get_issue", "some_tool"}
@@ -384,10 +389,10 @@ func TestWithToolsResolvesAliases(t *testing.T) {
 		mockTool("issue_read", "toolset1", true),
 	}
 
-	tsg := NewToolsetGroup(tools, nil, nil)
-	tsg.AddDeprecatedToolAliases(map[string]string{
-		"get_issue": "issue_read",
-	})
+	tsg := NewToolsetGroup(tools, nil, nil).
+		WithDeprecatedToolAliases(map[string]string{
+			"get_issue": "issue_read",
+		})
 
 	// Using deprecated alias should resolve to canonical name
 	filtered := tsg.WithToolsets([]string{}).WithTools([]string{"get_issue"})
@@ -593,10 +598,10 @@ func TestForMCPRequest_ToolsCall_DeprecatedAlias(t *testing.T) {
 		mockTool("list_commits", "repos", true),
 	}
 
-	tsg := NewToolsetGroup(tools, nil, nil)
-	tsg.AddDeprecatedToolAliases(map[string]string{
-		"old_get_me": "get_me",
-	})
+	tsg := NewToolsetGroup(tools, nil, nil).
+		WithDeprecatedToolAliases(map[string]string{
+			"old_get_me": "get_me",
+		})
 
 	// Request using the deprecated alias
 	filtered := tsg.ForMCPRequest(MCPMethodToolsCall, "old_get_me")
@@ -1033,3 +1038,35 @@ func TestFeatureFlagPrompts(t *testing.T) {
 		t.Errorf("Expected 2 prompts with checker, got %d", len(filtered.AvailablePrompts(context.Background())))
 	}
 }
+
+func TestServerToolHasHandler(t *testing.T) {
+	// Tool with handler
+	toolWithHandler := mockTool("has_handler", "toolset1", true)
+	if !toolWithHandler.HasHandler() {
+		t.Error("Expected HasHandler() to return true for tool with handler")
+	}
+
+	// Tool without handler
+	toolWithoutHandler := ServerTool{
+		Tool:    mcp.Tool{Name: "no_handler"},
+		Toolset: testToolsetMetadata("toolset1"),
+	}
+	if toolWithoutHandler.HasHandler() {
+		t.Error("Expected HasHandler() to return false for tool without handler")
+	}
+}
+
+func TestServerToolHandlerPanicOnNil(t *testing.T) {
+	tool := ServerTool{
+		Tool:    mcp.Tool{Name: "no_handler"},
+		Toolset: testToolsetMetadata("toolset1"),
+	}
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Error("Expected Handler() to panic when HandlerFunc is nil")
+		}
+	}()
+
+	tool.Handler(nil)
+}