refactor: separate ServerTool into own file with HandlerFunc pattern

- Extract ServerTool struct into pkg/toolsets/server_tool.go
- Add ToolDependencies struct for passing common dependencies to handlers
- HandlerFunc allows lazy handler generation from Tool definitions
- NewServerTool for new dependency-based tools
- NewServerToolLegacy for backward compatibility with existing handlers
- Update toolsets.go to store and pass dependencies
- Update all call sites to use NewServerToolLegacy

Co-authored-by: Adam Holt <4619+omgitsads@users.noreply.github.com>

Author: SamMorrowDrums

internal/ghmcp/server.go

@@ -18,6 +18,7 @@ import (
 	"github.com/github/github-mcp-server/pkg/lockdown"
 	mcplog "github.com/github/github-mcp-server/pkg/log"
 	"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"
 	"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -181,7 +182,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
 		enabledTools, _ = tsg.ResolveToolAliases(enabledTools)
 
 		// Register the specified tools (additive to any toolsets already enabled)
-		err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly)
+		err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly, toolsets.ToolDependencies{})
 		if err != nil {
 			return nil, fmt.Errorf("failed to register tools: %w", err)
 		}

pkg/github/dynamic_tools.go

@@ -61,9 +61,7 @@ func EnableToolset(s *mcp.Server, toolsetGroup *toolsets.ToolsetGroup, t transla
 			//
 			// Send notification to all initialized sessions
 			// s.sendNotificationToAllClients("notifications/tools/list_changed", nil)
-			for _, serverTool := range toolset.GetActiveTools() {
-				serverTool.RegisterFunc(s)
-			}
+			toolset.RegisterTools(s)
 
 			return utils.NewToolResultText(fmt.Sprintf("Toolset %s enabled", toolsetName)), nil, nil
 		})

pkg/toolsets/server_tool.go

@@ -0,0 +1,121 @@
+package toolsets
+
+import (
+	"context"
+	"encoding/json"
+
+	"github.com/modelcontextprotocol/go-sdk/mcp"
+)
+
+// HandlerFunc is a function that takes dependencies and returns an MCP tool handler.
+// This allows tools to be defined statically while their handlers are generated
+// on-demand with the appropriate dependencies.
+type HandlerFunc func(deps ToolDependencies) mcp.ToolHandler
+
+// ToolDependencies contains all dependencies that tool handlers might need.
+// Fields are pointers/interfaces so they can be nil when not needed by a specific tool.
+type ToolDependencies struct {
+	// GetClient returns a GitHub REST API client
+	GetClient any // func(context.Context) (*github.Client, error)
+
+	// GetGQLClient returns a GitHub GraphQL client
+	GetGQLClient any // func(context.Context) (*githubv4.Client, error)
+
+	// GetRawClient returns a raw HTTP client for GitHub
+	GetRawClient any // raw.GetRawClientFn
+
+	// RepoAccessCache is the lockdown mode repo access cache
+	RepoAccessCache any // *lockdown.RepoAccessCache
+
+	// T is the translation helper function
+	T any // translations.TranslationHelperFunc
+
+	// Flags are feature flags
+	Flags any // FeatureFlags
+
+	// ContentWindowSize is the size of the content window for log truncation
+	ContentWindowSize int
+}
+
+// ServerTool represents an MCP tool with a handler generator function.
+// The tool definition is static, while the handler is generated on-demand
+// when the tool is registered with a server.
+type ServerTool struct {
+	// Tool is the MCP tool definition containing name, description, schema, etc.
+	Tool mcp.Tool
+
+	// HandlerFunc generates the handler when given dependencies.
+	// This allows tools to be passed around without handlers being set up,
+	// and handlers are only created when needed.
+	HandlerFunc HandlerFunc
+}
+
+// Handler returns a tool handler by calling HandlerFunc with the given dependencies.
+func (st *ServerTool) Handler(deps ToolDependencies) mcp.ToolHandler {
+	if st.HandlerFunc == nil {
+		return nil
+	}
+	return st.HandlerFunc(deps)
+}
+
+// RegisterFunc registers the tool with the server using the provided dependencies.
+func (st *ServerTool) RegisterFunc(s *mcp.Server, deps ToolDependencies) {
+	handler := st.Handler(deps)
+	s.AddTool(&st.Tool, handler)
+}
+
+// NewServerTool creates a ServerTool from a tool definition and a typed handler function.
+// The handler function takes dependencies and returns a typed handler.
+func NewServerTool[In any, Out any](tool mcp.Tool, handlerFn func(deps ToolDependencies) mcp.ToolHandlerFor[In, Out]) ServerTool {
+	return ServerTool{
+		Tool: tool,
+		HandlerFunc: func(deps ToolDependencies) mcp.ToolHandler {
+			typedHandler := handlerFn(deps)
+			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 := typedHandler(ctx, req, arguments)
+				return resp, err
+			}
+		},
+	}
+}
+
+// NewServerToolFromHandler creates a ServerTool from a tool definition and a raw handler function.
+// Use this when you have a handler that already conforms to mcp.ToolHandler.
+func NewServerToolFromHandler(tool mcp.Tool, handlerFn func(deps ToolDependencies) mcp.ToolHandler) ServerTool {
+	return ServerTool{Tool: tool, HandlerFunc: handlerFn}
+}
+
+// NewServerToolLegacy creates a ServerTool from a tool definition and an already-bound typed handler.
+// This is for backward compatibility during the refactor - the handler doesn't use ToolDependencies.
+// Deprecated: Use NewServerTool instead for new code.
+func NewServerToolLegacy[In any, Out any](tool mcp.Tool, handler mcp.ToolHandlerFor[In, Out]) ServerTool {
+	return ServerTool{
+		Tool: tool,
+		HandlerFunc: func(_ ToolDependencies) 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 and an already-bound raw handler.
+// This is for backward compatibility during the refactor - the handler doesn't use ToolDependencies.
+// Deprecated: Use NewServerToolFromHandler instead for new code.
+func NewServerToolFromHandlerLegacy(tool mcp.Tool, handler mcp.ToolHandler) ServerTool {
+	return ServerTool{
+		Tool: tool,
+		HandlerFunc: func(_ ToolDependencies) mcp.ToolHandler {
+			return handler
+		},
+	}
+}

pkg/toolsets/toolsets.go

@@ -1,8 +1,6 @@
 package toolsets
 
 import (
-	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"strings"
@@ -32,27 +30,7 @@ func NewToolsetDoesNotExistError(name string) *ToolsetDoesNotExistError {
 	return &ToolsetDoesNotExistError{Name: name}
 }
 
-type ServerTool struct {
-	Tool         mcp.Tool
-	RegisterFunc func(s *mcp.Server)
-}
-
-func NewServerTool[In any, Out any](tool mcp.Tool, handler mcp.ToolHandlerFor[In, Out]) ServerTool {
-	return ServerTool{Tool: tool, RegisterFunc: func(s *mcp.Server) {
-		th := 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
-		}
-
-		s.AddTool(&tool, th)
-	}}
-}
+// ServerTool is defined in server_tool.go
 
 type ServerResourceTemplate struct {
 	Template mcp.ResourceTemplate
@@ -86,6 +64,8 @@ type Toolset struct {
 	readOnly    bool
 	writeTools  []ServerTool
 	readTools   []ServerTool
+	// deps holds the dependencies for tool handlers
+	deps ToolDependencies
 	// resources are not tools, but the community seems to be moving towards namespaces as a broader concept
 	// and in order to have multiple servers running concurrently, we want to avoid overlapping resources too.
 	resourceTemplates []ServerResourceTemplate
@@ -114,16 +94,22 @@ func (t *Toolset) RegisterTools(s *mcp.Server) {
 	if !t.Enabled {
 		return
 	}
-	for _, tool := range t.readTools {
-		tool.RegisterFunc(s)
+	for i := range t.readTools {
+		t.readTools[i].RegisterFunc(s, t.deps)
 	}
 	if !t.readOnly {
-		for _, tool := range t.writeTools {
-			tool.RegisterFunc(s)
+		for i := range t.writeTools {
+			t.writeTools[i].RegisterFunc(s, t.deps)
 		}
 	}
 }
 
+// SetDependencies sets the dependencies for this toolset's tool handlers.
+func (t *Toolset) SetDependencies(deps ToolDependencies) *Toolset {
+	t.deps = deps
+	return t
+}
+
 func (t *Toolset) AddResourceTemplates(templates ...ServerResourceTemplate) *Toolset {
 	t.resourceTemplates = append(t.resourceTemplates, templates...)
 	return t
@@ -358,7 +344,7 @@ func (tg *ToolsetGroup) FindToolByName(toolName string) (*ServerTool, string, er
 // RegisterSpecificTools registers only the specified tools.
 // Respects read-only mode (skips write tools if readOnly=true).
 // Returns error if any tool is not found.
-func (tg *ToolsetGroup) RegisterSpecificTools(s *mcp.Server, toolNames []string, readOnly bool) error {
+func (tg *ToolsetGroup) RegisterSpecificTools(s *mcp.Server, toolNames []string, readOnly bool, deps ToolDependencies) error {
 	var skippedTools []string
 	for _, toolName := range toolNames {
 		tool, _, err := tg.FindToolByName(toolName)
@@ -373,7 +359,7 @@ func (tg *ToolsetGroup) RegisterSpecificTools(s *mcp.Server, toolNames []string,
 		}
 
 		// Register the tool
-		tool.RegisterFunc(s)
+		tool.RegisterFunc(s, deps)
 	}
 
 	// Log skipped write tools if any

pkg/toolsets/toolsets_test.go

@@ -1,6 +1,8 @@
 package toolsets
 
 import (
+	"context"
+	"encoding/json"
 	"errors"
 	"testing"
 
@@ -9,15 +11,20 @@ import (
 
 // mockTool creates a minimal ServerTool for testing
 func mockTool(name string, readOnly bool) ServerTool {
-	return ServerTool{
-		Tool: mcp.Tool{
+	return NewServerToolFromHandler(
+		mcp.Tool{
 			Name: name,
 			Annotations: &mcp.ToolAnnotations{
 				ReadOnlyHint: readOnly,
 			},
+			InputSchema: json.RawMessage(`{"type":"object","properties":{}}`),
 		},
-		RegisterFunc: func(_ *mcp.Server) {},
-	}
+		func(_ ToolDependencies) mcp.ToolHandler {
+			return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+				return nil, nil
+			}
+		},
+	)
 }
 
 func TestNewToolsetGroupIsEmptyWithoutEverythingOn(t *testing.T) {
@@ -375,20 +382,25 @@ func TestRegisterSpecificTools(t *testing.T) {
 	toolset.writeTools = append(toolset.writeTools, mockTool("issue_write", false))
 	tsg.AddToolset(toolset)
 
+	deps := ToolDependencies{}
+
+	// Create a real server for testing
+	server := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "1.0.0"}, nil)
+
 	// Test registering with canonical names
-	err := tsg.RegisterSpecificTools(nil, []string{"issue_read"}, false)
+	err := tsg.RegisterSpecificTools(server, []string{"issue_read"}, false, deps)
 	if err != nil {
 		t.Errorf("expected no error registering tool, got %v", err)
 	}
 
 	// Test registering write tool in read-only mode (should skip but not error)
-	err = tsg.RegisterSpecificTools(nil, []string{"issue_write"}, true)
+	err = tsg.RegisterSpecificTools(server, []string{"issue_write"}, true, deps)
 	if err != nil {
 		t.Errorf("expected no error when skipping write tool in read-only mode, got %v", err)
 	}
 
 	// Test registering non-existent tool (should error)
-	err = tsg.RegisterSpecificTools(nil, []string{"nonexistent"}, false)
+	err = tsg.RegisterSpecificTools(server, []string{"nonexistent"}, false, deps)
 	if err == nil {
 		t.Error("expected error for non-existent tool")
 	}