Skip to content

Commit a03b3bf

Browse files
SamMorrowDrumsSamMorrowDrums
committed
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
1 parent 689a040 commit a03b3bf

File tree

6 files changed

+81
-69
lines changed

6 files changed

+81
-69
lines changed

‎cmd/github-mcp-server/generate_docs.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"strings"
1111

1212
"github.com/github/github-mcp-server/pkg/github"
13-
"github.com/github/github-mcp-server/pkg/raw"
1413
"github.com/github/github-mcp-server/pkg/toolsets"
1514
"github.com/github/github-mcp-server/pkg/translations"
1615
gogithub "github.com/google/go-github/v79/github"
@@ -37,11 +36,6 @@ func mockGetClient(_ context.Context) (*gogithub.Client, error) {
3736
return gogithub.NewClient(nil), nil
3837
}
3938

40-
// mockGetRawClient returns a mock raw client for documentation generation
41-
func mockGetRawClient(_ context.Context) (*raw.Client, error) {
42-
return nil, nil
43-
}
44-
4539
func generateAllDocs() error {
4640
if err := generateReadmeDocs("README.md"); err != nil {
4741
return fmt.Errorf("failed to generate README docs: %w", err)
@@ -63,7 +57,7 @@ func generateReadmeDocs(readmePath string) error {
6357
t, _ := translations.TranslationHelper()
6458

6559
// Create toolset group with mock clients (no deps needed for doc generation)
66-
tsg := github.NewToolsetGroup(t, mockGetClient, mockGetRawClient)
60+
tsg := github.NewToolsetGroup(t, mockGetClient, nil)
6761

6862
// Generate toolsets documentation
6963
toolsetsDoc := generateToolsetsDoc(tsg)
@@ -306,7 +300,7 @@ func generateRemoteToolsetsDoc() string {
306300
t, _ := translations.TranslationHelper()
307301

308302
// Create toolset group with mock clients
309-
tsg := github.NewToolsetGroup(t, mockGetClient, mockGetRawClient)
303+
tsg := github.NewToolsetGroup(t, mockGetClient, nil)
310304

311305
// Generate table header
312306
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‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,19 +165,17 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
165165
// Create toolset group with all tools, resources, and prompts
166166
tsg := github.NewToolsetGroup(cfg.Translator, getClient, getRawClient)
167167

168-
// Add deprecated tool aliases for backward compatibility
169-
// See docs/deprecated-tool-aliases.md for the full list of renames
170-
tsg.AddDeprecatedToolAliases(github.DeprecatedToolAliases)
171-
172168
// Clean tool names (WithTools will resolve any deprecated aliases)
173169
enabledTools := github.CleanTools(cfg.EnabledTools)
174170

175171
// Apply filters based on configuration
172+
// - WithDeprecatedToolAliases: adds backward compatibility aliases
176173
// - WithReadOnly: filters out write tools when true
177174
// - WithToolsets: nil=defaults, empty=none, handles "all"/"default" keywords
178175
// - WithTools: additional tools that bypass toolset filtering (additive, resolves aliases)
179176
// - WithFeatureChecker: filters based on feature flags
180177
filteredTsg := tsg.
178+
WithDeprecatedToolAliases(github.DeprecatedToolAliases).
181179
WithReadOnly(cfg.ReadOnly).
182180
WithToolsets(enabledToolsets).
183181
WithTools(enabledTools).

‎pkg/github/tools_validation_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,8 @@ func TestAllToolsHaveHandlerFunc(t *testing.T) {
185185
t.Run(tool.Tool.Name, func(t *testing.T) {
186186
assert.NotNil(t, tool.HandlerFunc,
187187
"Tool %q must have a HandlerFunc", tool.Tool.Name)
188+
assert.True(t, tool.HasHandler(),
189+
"Tool %q HasHandler() should return true", tool.Tool.Name)
188190
})
189191
}
190192
}

‎pkg/toolsets/server_tool.go‎

Lines changed: 9 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,24 @@ func (st *ServerTool) IsReadOnly() bool {
5757
return st.Tool.Annotations != nil && st.Tool.Annotations.ReadOnlyHint
5858
}
5959

60+
// HasHandler returns true if this tool has a handler function.
61+
func (st *ServerTool) HasHandler() bool {
62+
return st.HandlerFunc != nil
63+
}
64+
6065
// Handler returns a tool handler by calling HandlerFunc with the given dependencies.
66+
// Panics if HandlerFunc is nil - all tools should have handlers.
6167
func (st *ServerTool) Handler(deps any) mcp.ToolHandler {
6268
if st.HandlerFunc == nil {
63-
return nil
69+
panic("HandlerFunc is nil for tool: " + st.Tool.Name)
6470
}
6571
return st.HandlerFunc(deps)
6672
}
6773

6874
// RegisterFunc registers the tool with the server using the provided dependencies.
75+
// Panics if the tool has no handler - all tools should have handlers.
6976
func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) {
70-
handler := st.Handler(deps)
77+
handler := st.Handler(deps) // This will panic if HandlerFunc is nil
7178
s.AddTool(&st.Tool, handler)
7279
}
7380

@@ -97,36 +104,3 @@ func NewServerTool[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, hand
97104
func NewServerToolFromHandler(tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandler) ServerTool {
98105
return ServerTool{Tool: tool, Toolset: toolset, HandlerFunc: handlerFn}
99106
}
100-
101-
// NewServerToolLegacy creates a ServerTool from a tool definition, toolset metadata, and an already-bound typed handler.
102-
// This is for backward compatibility during the refactor - the handler doesn't use dependencies.
103-
// Deprecated: Use NewServerTool instead for new code.
104-
func NewServerToolLegacy[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandlerFor[In, Out]) ServerTool {
105-
return ServerTool{
106-
Tool: tool,
107-
Toolset: toolset,
108-
HandlerFunc: func(_ any) mcp.ToolHandler {
109-
return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) {
110-
var arguments In
111-
if err := json.Unmarshal(req.Params.Arguments, &arguments); err != nil {
112-
return nil, err
113-
}
114-
resp, _, err := handler(ctx, req, arguments)
115-
return resp, err
116-
}
117-
},
118-
}
119-
}
120-
121-
// NewServerToolFromHandlerLegacy creates a ServerTool from a tool definition, toolset metadata, and an already-bound raw handler.
122-
// This is for backward compatibility during the refactor - the handler doesn't use dependencies.
123-
// Deprecated: Use NewServerToolFromHandler instead for new code.
124-
func NewServerToolFromHandlerLegacy(tool mcp.Tool, toolset ToolsetMetadata, handler mcp.ToolHandler) ServerTool {
125-
return ServerTool{
126-
Tool: tool,
127-
Toolset: toolset,
128-
HandlerFunc: func(_ any) mcp.ToolHandler {
129-
return handler
130-
},
131-
}
132-
}

‎pkg/toolsets/toolsets.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -413,12 +413,19 @@ func (tg *ToolsetGroup) filterPromptsByName(name string) []ServerPrompt {
413413
return []ServerPrompt{}
414414
}
415415

416-
// AddDeprecatedToolAliases adds mappings from old tool names to new canonical names.
417-
func (tg *ToolsetGroup) AddDeprecatedToolAliases(aliases map[string]string) *ToolsetGroup {
416+
// WithDeprecatedToolAliases returns a new ToolsetGroup with the given deprecated aliases added.
417+
// Aliases map old tool names to new canonical names.
418+
func (tg *ToolsetGroup) WithDeprecatedToolAliases(aliases map[string]string) *ToolsetGroup {
419+
newTG := tg.copy()
420+
// Ensure we have a fresh map
421+
newTG.deprecatedAliases = make(map[string]string, len(tg.deprecatedAliases)+len(aliases))
422+
for k, v := range tg.deprecatedAliases {
423+
newTG.deprecatedAliases[k] = v
424+
}
418425
for oldName, newName := range aliases {
419-
tg.deprecatedAliases[oldName] = newName
426+
newTG.deprecatedAliases[oldName] = newName
420427
}
421-
return tg
428+
return newTG
422429
}
423430

424431
// isToolsetEnabled checks if a toolset is enabled based on current filters.

‎pkg/toolsets/toolsets_test.go‎

Lines changed: 55 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -252,22 +252,27 @@ func TestToolsForToolset(t *testing.T) {
252252
}
253253
}
254254

255-
func TestAddDeprecatedToolAliases(t *testing.T) {
255+
func TestWithDeprecatedToolAliases(t *testing.T) {
256256
tools := []ServerTool{
257257
mockTool("new_name", "toolset1", true),
258258
}
259259

260260
tsg := NewToolsetGroup(tools, nil, nil)
261-
tsg.AddDeprecatedToolAliases(map[string]string{
261+
tsgWithAliases := tsg.WithDeprecatedToolAliases(map[string]string{
262262
"old_name": "new_name",
263263
"get_issue": "issue_read",
264264
})
265265

266-
if len(tsg.deprecatedAliases) != 2 {
267-
t.Errorf("expected 2 aliases, got %d", len(tsg.deprecatedAliases))
266+
// Original should be unchanged (immutable)
267+
if len(tsg.deprecatedAliases) != 0 {
268+
t.Errorf("original should have 0 aliases, got %d", len(tsg.deprecatedAliases))
268269
}
269-
if tsg.deprecatedAliases["old_name"] != "new_name" {
270-
t.Errorf("expected alias 'old_name' -> 'new_name', got '%s'", tsg.deprecatedAliases["old_name"])
270+
271+
if len(tsgWithAliases.deprecatedAliases) != 2 {
272+
t.Errorf("expected 2 aliases, got %d", len(tsgWithAliases.deprecatedAliases))
273+
}
274+
if tsgWithAliases.deprecatedAliases["old_name"] != "new_name" {
275+
t.Errorf("expected alias 'old_name' -> 'new_name', got '%s'", tsgWithAliases.deprecatedAliases["old_name"])
271276
}
272277
}
273278

@@ -277,10 +282,10 @@ func TestResolveToolAliases(t *testing.T) {
277282
mockTool("some_tool", "toolset1", true),
278283
}
279284

280-
tsg := NewToolsetGroup(tools, nil, nil)
281-
tsg.AddDeprecatedToolAliases(map[string]string{
282-
"get_issue": "issue_read",
283-
})
285+
tsg := NewToolsetGroup(tools, nil, nil).
286+
WithDeprecatedToolAliases(map[string]string{
287+
"get_issue": "issue_read",
288+
})
284289

285290
// Test resolving a mix of aliases and canonical names
286291
input := []string{"get_issue", "some_tool"}
@@ -384,10 +389,10 @@ func TestWithToolsResolvesAliases(t *testing.T) {
384389
mockTool("issue_read", "toolset1", true),
385390
}
386391

387-
tsg := NewToolsetGroup(tools, nil, nil)
388-
tsg.AddDeprecatedToolAliases(map[string]string{
389-
"get_issue": "issue_read",
390-
})
392+
tsg := NewToolsetGroup(tools, nil, nil).
393+
WithDeprecatedToolAliases(map[string]string{
394+
"get_issue": "issue_read",
395+
})
391396

392397
// Using deprecated alias should resolve to canonical name
393398
filtered := tsg.WithToolsets([]string{}).WithTools([]string{"get_issue"})
@@ -593,10 +598,10 @@ func TestForMCPRequest_ToolsCall_DeprecatedAlias(t *testing.T) {
593598
mockTool("list_commits", "repos", true),
594599
}
595600

596-
tsg := NewToolsetGroup(tools, nil, nil)
597-
tsg.AddDeprecatedToolAliases(map[string]string{
598-
"old_get_me": "get_me",
599-
})
601+
tsg := NewToolsetGroup(tools, nil, nil).
602+
WithDeprecatedToolAliases(map[string]string{
603+
"old_get_me": "get_me",
604+
})
600605

601606
// Request using the deprecated alias
602607
filtered := tsg.ForMCPRequest(MCPMethodToolsCall, "old_get_me")
@@ -1033,3 +1038,35 @@ func TestFeatureFlagPrompts(t *testing.T) {
10331038
t.Errorf("Expected 2 prompts with checker, got %d", len(filtered.AvailablePrompts(context.Background())))
10341039
}
10351040
}
1041+
1042+
func TestServerToolHasHandler(t *testing.T) {
1043+
// Tool with handler
1044+
toolWithHandler := mockTool("has_handler", "toolset1", true)
1045+
if !toolWithHandler.HasHandler() {
1046+
t.Error("Expected HasHandler() to return true for tool with handler")
1047+
}
1048+
1049+
// Tool without handler
1050+
toolWithoutHandler := ServerTool{
1051+
Tool: mcp.Tool{Name: "no_handler"},
1052+
Toolset: testToolsetMetadata("toolset1"),
1053+
}
1054+
if toolWithoutHandler.HasHandler() {
1055+
t.Error("Expected HasHandler() to return false for tool without handler")
1056+
}
1057+
}
1058+
1059+
func TestServerToolHandlerPanicOnNil(t *testing.T) {
1060+
tool := ServerTool{
1061+
Tool: mcp.Tool{Name: "no_handler"},
1062+
Toolset: testToolsetMetadata("toolset1"),
1063+
}
1064+
1065+
defer func() {
1066+
if r := recover(); r == nil {
1067+
t.Error("Expected Handler() to panic when HandlerFunc is nil")
1068+
}
1069+
}()
1070+
1071+
tool.Handler(nil)
1072+
}

0 commit comments

Comments
 (0)