From 8887c93d92455b48ddef20d4545c8d5016ffaff0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 10:21:19 +0000 Subject: [PATCH 1/5] Initial plan From 5baf02b47ea11e077efe08c176781d8245b88f5b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 10:30:39 +0000 Subject: [PATCH 2/5] Migrate dynamic toolset to modelcontextprotocol/go-sdk This commit migrates the dynamic toolset (enable_toolset, list_available_toolsets, get_toolset_tools) from mark3labs/mcp-go to modelcontextprotocol/go-sdk. Changes: - Removed //go:build ignore tag - Updated imports to use modelcontextprotocol/go-sdk - Migrated all tool functions to use new SDK patterns - Updated ToolsetEnum helper to return []any instead of mcp.PropertyOption - Converted DSL-based schema definitions to jsonschema.Schema structures - Updated handler signatures to use map[string]any args - Replaced old result helpers with utils package equivalents - Fixed EnableToolset to use RegisterFunc instead of AddTools - Created comprehensive test suite for all three tools - Generated toolsnaps for the new tools Related to #1428 Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com> --- pkg/github/__toolsnaps__/enable_toolset.snap | 23 ++ .../__toolsnaps__/get_toolset_tools.snap | 23 ++ .../list_available_toolsets.snap | 11 + pkg/github/dynamic_tools.go | 243 ++++++++++-------- pkg/github/dynamic_tools_test.go | 241 +++++++++++++++++ 5 files changed, 434 insertions(+), 107 deletions(-) create mode 100644 pkg/github/__toolsnaps__/enable_toolset.snap create mode 100644 pkg/github/__toolsnaps__/get_toolset_tools.snap create mode 100644 pkg/github/__toolsnaps__/list_available_toolsets.snap create mode 100644 pkg/github/dynamic_tools_test.go diff --git a/pkg/github/__toolsnaps__/enable_toolset.snap b/pkg/github/__toolsnaps__/enable_toolset.snap new file mode 100644 index 000000000..7259c56f6 --- /dev/null +++ b/pkg/github/__toolsnaps__/enable_toolset.snap @@ -0,0 +1,23 @@ +{ + "annotations": { + "readOnlyHint": true, + "title": "Enable a toolset" + }, + "description": "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable", + "inputSchema": { + "type": "object", + "required": [ + "toolset" + ], + "properties": { + "toolset": { + "type": "string", + "description": "The name of the toolset to enable", + "enum": [ + "mock-toolset" + ] + } + } + }, + "name": "enable_toolset" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/get_toolset_tools.snap b/pkg/github/__toolsnaps__/get_toolset_tools.snap new file mode 100644 index 000000000..ace797d6c --- /dev/null +++ b/pkg/github/__toolsnaps__/get_toolset_tools.snap @@ -0,0 +1,23 @@ +{ + "annotations": { + "readOnlyHint": true, + "title": "List all tools in a toolset" + }, + "description": "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task", + "inputSchema": { + "type": "object", + "required": [ + "toolset" + ], + "properties": { + "toolset": { + "type": "string", + "description": "The name of the toolset you want to get the tools for", + "enum": [ + "mock-toolset" + ] + } + } + }, + "name": "get_toolset_tools" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_available_toolsets.snap b/pkg/github/__toolsnaps__/list_available_toolsets.snap new file mode 100644 index 000000000..bcdf0537d --- /dev/null +++ b/pkg/github/__toolsnaps__/list_available_toolsets.snap @@ -0,0 +1,11 @@ +{ + "annotations": { + "readOnlyHint": true, + "title": "List available toolsets" + }, + "description": "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call", + "inputSchema": { + "type": "object" + }, + "name": "list_available_toolsets" +} \ No newline at end of file diff --git a/pkg/github/dynamic_tools.go b/pkg/github/dynamic_tools.go index 45a481576..b51889dd7 100644 --- a/pkg/github/dynamic_tools.go +++ b/pkg/github/dynamic_tools.go @@ -1,5 +1,3 @@ -//go:build ignore - package github import ( @@ -9,132 +7,163 @@ import ( "github.com/github/github-mcp-server/pkg/toolsets" "github.com/github/github-mcp-server/pkg/translations" - "github.com/mark3labs/mcp-go/mcp" - "github.com/mark3labs/mcp-go/server" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" ) -func ToolsetEnum(toolsetGroup *toolsets.ToolsetGroup) mcp.PropertyOption { - toolsetNames := make([]string, 0, len(toolsetGroup.Toolsets)) +func ToolsetEnum(toolsetGroup *toolsets.ToolsetGroup) []any { + toolsetNames := make([]any, 0, len(toolsetGroup.Toolsets)) for name := range toolsetGroup.Toolsets { toolsetNames = append(toolsetNames, name) } - return mcp.Enum(toolsetNames...) + return toolsetNames } -func EnableToolset(s *server.MCPServer, toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { - return mcp.NewTool("enable_toolset", - mcp.WithDescription(t("TOOL_ENABLE_TOOLSET_DESCRIPTION", "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable")), - mcp.WithToolAnnotation(mcp.ToolAnnotation{ - Title: t("TOOL_ENABLE_TOOLSET_USER_TITLE", "Enable a toolset"), - // Not modifying GitHub data so no need to show a warning - ReadOnlyHint: ToBoolPtr(true), - }), - mcp.WithString("toolset", - mcp.Required(), - mcp.Description("The name of the toolset to enable"), - ToolsetEnum(toolsetGroup), - ), - ), - func(_ context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // We need to convert the toolsets back to a map for JSON serialization - toolsetName, err := RequiredParam[string](request, "toolset") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - toolset := toolsetGroup.Toolsets[toolsetName] - if toolset == nil { - return mcp.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil - } - if toolset.Enabled { - return mcp.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil - } +func EnableToolset(s *mcp.Server, toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { + tool := mcp.Tool{ + Name: "enable_toolset", + Description: t("TOOL_ENABLE_TOOLSET_DESCRIPTION", "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ENABLE_TOOLSET_USER_TITLE", "Enable a toolset"), + // Not modifying GitHub data so no need to show a warning + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "toolset": { + Type: "string", + Description: "The name of the toolset to enable", + Enum: ToolsetEnum(toolsetGroup), + }, + }, + Required: []string{"toolset"}, + }, + } - toolset.Enabled = true + handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsets back to a map for JSON serialization + toolsetName, err := RequiredParam[string](args, "toolset") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + toolset := toolsetGroup.Toolsets[toolsetName] + if toolset == nil { + return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil + } + if toolset.Enabled { + return utils.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil, nil + } - // caution: this currently affects the global tools and notifies all clients: - // - // Send notification to all initialized sessions - // s.sendNotificationToAllClients("notifications/tools/list_changed", nil) - s.AddTools(toolset.GetActiveTools()...) + toolset.Enabled = true - return mcp.NewToolResultText(fmt.Sprintf("Toolset %s enabled", toolsetName)), nil + // caution: this currently affects the global tools and notifies all clients: + // + // Send notification to all initialized sessions + // s.sendNotificationToAllClients("notifications/tools/list_changed", nil) + for _, serverTool := range toolset.GetActiveTools() { + serverTool.RegisterFunc(s) } + + return utils.NewToolResultText(fmt.Sprintf("Toolset %s enabled", toolsetName)), nil, nil + }) + + return tool, handler } -func ListAvailableToolsets(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { - return mcp.NewTool("list_available_toolsets", - mcp.WithDescription(t("TOOL_LIST_AVAILABLE_TOOLSETS_DESCRIPTION", "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call")), - mcp.WithToolAnnotation(mcp.ToolAnnotation{ - Title: t("TOOL_LIST_AVAILABLE_TOOLSETS_USER_TITLE", "List available toolsets"), - ReadOnlyHint: ToBoolPtr(true), - }), - ), - func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // We need to convert the toolsetGroup back to a map for JSON serialization - - payload := []map[string]string{} - - for name, ts := range toolsetGroup.Toolsets { - { - t := map[string]string{ - "name": name, - "description": ts.Description, - "can_enable": "true", - "currently_enabled": fmt.Sprintf("%t", ts.Enabled), - } - payload = append(payload, t) - } - } +func ListAvailableToolsets(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { + tool := mcp.Tool{ + Name: "list_available_toolsets", + Description: t("TOOL_LIST_AVAILABLE_TOOLSETS_DESCRIPTION", "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_AVAILABLE_TOOLSETS_USER_TITLE", "List available toolsets"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{}, + }, + } + + handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsetGroup back to a map for JSON serialization + + payload := []map[string]string{} - r, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("failed to marshal features: %w", err) + for name, ts := range toolsetGroup.Toolsets { + { + t := map[string]string{ + "name": name, + "description": ts.Description, + "can_enable": "true", + "currently_enabled": fmt.Sprintf("%t", ts.Enabled), + } + payload = append(payload, t) } + } - return mcp.NewToolResultText(string(r)), nil + r, err := json.Marshal(payload) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal features: %w", err) } + + return utils.NewToolResultText(string(r)), nil, nil + }) + + return tool, handler } -func GetToolsetsTools(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { - return mcp.NewTool("get_toolset_tools", - mcp.WithDescription(t("TOOL_GET_TOOLSET_TOOLS_DESCRIPTION", "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task")), - mcp.WithToolAnnotation(mcp.ToolAnnotation{ - Title: t("TOOL_GET_TOOLSET_TOOLS_USER_TITLE", "List all tools in a toolset"), - ReadOnlyHint: ToBoolPtr(true), - }), - mcp.WithString("toolset", - mcp.Required(), - mcp.Description("The name of the toolset you want to get the tools for"), - ToolsetEnum(toolsetGroup), - ), - ), - func(_ context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // We need to convert the toolsetGroup back to a map for JSON serialization - toolsetName, err := RequiredParam[string](request, "toolset") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - toolset := toolsetGroup.Toolsets[toolsetName] - if toolset == nil { - return mcp.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil - } - payload := []map[string]string{} - - for _, st := range toolset.GetAvailableTools() { - tool := map[string]string{ - "name": st.Tool.Name, - "description": st.Tool.Description, - "can_enable": "true", - "toolset": toolsetName, - } - payload = append(payload, tool) - } +func GetToolsetsTools(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { + tool := mcp.Tool{ + Name: "get_toolset_tools", + Description: t("TOOL_GET_TOOLSET_TOOLS_DESCRIPTION", "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_GET_TOOLSET_TOOLS_USER_TITLE", "List all tools in a toolset"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "toolset": { + Type: "string", + Description: "The name of the toolset you want to get the tools for", + Enum: ToolsetEnum(toolsetGroup), + }, + }, + Required: []string{"toolset"}, + }, + } - r, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("failed to marshal features: %w", err) + handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsetGroup back to a map for JSON serialization + toolsetName, err := RequiredParam[string](args, "toolset") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + toolset := toolsetGroup.Toolsets[toolsetName] + if toolset == nil { + return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil + } + payload := []map[string]string{} + + for _, st := range toolset.GetAvailableTools() { + tool := map[string]string{ + "name": st.Tool.Name, + "description": st.Tool.Description, + "can_enable": "true", + "toolset": toolsetName, } + payload = append(payload, tool) + } - return mcp.NewToolResultText(string(r)), nil + r, err := json.Marshal(payload) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal features: %w", err) } + + return utils.NewToolResultText(string(r)), nil, nil + }) + + return tool, handler } diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go new file mode 100644 index 000000000..269667d1f --- /dev/null +++ b/pkg/github/dynamic_tools_test.go @@ -0,0 +1,241 @@ +package github + +import ( + "context" + "encoding/json" + "testing" + + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/toolsets" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_EnableToolset(t *testing.T) { + // Create a mock toolset group + tsg := toolsets.NewToolsetGroup(false) + mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") + tsg.AddToolset(mockToolset) + + // Create mock server + s := NewServer("test", &mcp.ServerOptions{}) + + // Verify tool definition + tool, _ := EnableToolset(s, tsg, translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + // Validate tool schema + assert.Equal(t, "enable_toolset", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.True(t, tool.Annotations.ReadOnlyHint, "enable_toolset tool should be read-only") + + tests := []struct { + name string + requestArgs map[string]interface{} + expectError bool + expectedErrMsg string + }{ + { + name: "successfully enable toolset", + requestArgs: map[string]interface{}{ + "toolset": "mock-toolset", + }, + expectError: false, + }, + { + name: "toolset not found", + requestArgs: map[string]interface{}{ + "toolset": "non-existent", + }, + expectError: true, + expectedErrMsg: "Toolset non-existent not found", + }, + { + name: "missing toolset parameter", + requestArgs: map[string]interface{}{ + // No toolset parameter + }, + expectError: true, + expectedErrMsg: "missing required parameter: toolset", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Reset toolset state + mockToolset.Enabled = false + + // Create new instances for each test + s := NewServer("test", &mcp.ServerOptions{}) + tsg := toolsets.NewToolsetGroup(false) + mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") + tsg.AddToolset(mockToolset) + + _, handler := EnableToolset(s, tsg, translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, _, err := handler(context.Background(), &request, tc.requestArgs) + + // Verify results + require.NoError(t, err) + if tc.expectError { + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.False(t, result.IsError) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "enabled") + }) + } +} + +func Test_ListAvailableToolsets(t *testing.T) { + // Create a mock toolset group + tsg := toolsets.NewToolsetGroup(false) + mockToolset1 := toolsets.NewToolset("toolset1", "First toolset") + mockToolset2 := toolsets.NewToolset("toolset2", "Second toolset") + tsg.AddToolset(mockToolset1) + tsg.AddToolset(mockToolset2) + + // Verify tool definition + tool, _ := ListAvailableToolsets(tsg, translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + // Validate tool schema + assert.Equal(t, "list_available_toolsets", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.True(t, tool.Annotations.ReadOnlyHint, "list_available_toolsets tool should be read-only") + + t.Run("successfully list toolsets", func(t *testing.T) { + _, handler := ListAvailableToolsets(tsg, translations.NullTranslationHelper) + + // Create call request with empty args + request := createMCPRequest(map[string]interface{}{}) + + // Call handler + result, _, err := handler(context.Background(), &request, map[string]interface{}{}) + + // Verify results + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + + // Parse JSON response + var toolsets []map[string]string + err = json.Unmarshal([]byte(textContent.Text), &toolsets) + require.NoError(t, err) + + // Verify we have the expected toolsets + assert.Len(t, toolsets, 2) + foundToolset1 := false + foundToolset2 := false + for _, ts := range toolsets { + if ts["name"] == "toolset1" { + foundToolset1 = true + assert.Equal(t, "First toolset", ts["description"]) + assert.Equal(t, "true", ts["can_enable"]) + } + if ts["name"] == "toolset2" { + foundToolset2 = true + assert.Equal(t, "Second toolset", ts["description"]) + assert.Equal(t, "true", ts["can_enable"]) + } + } + assert.True(t, foundToolset1, "Expected to find toolset1") + assert.True(t, foundToolset2, "Expected to find toolset2") + }) +} + +func Test_GetToolsetsTools(t *testing.T) { + // Create a mock toolset group + tsg := toolsets.NewToolsetGroup(false) + mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") + + // Add a mock tool to the toolset + mockTool, mockHandler := GetDependabotAlert(stubGetClientFn(nil), translations.NullTranslationHelper) + mockToolset.AddReadTools(toolsets.NewServerTool(mockTool, mockHandler)) + tsg.AddToolset(mockToolset) + + // Verify tool definition + tool, _ := GetToolsetsTools(tsg, translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + // Validate tool schema + assert.Equal(t, "get_toolset_tools", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.True(t, tool.Annotations.ReadOnlyHint, "get_toolset_tools tool should be read-only") + + tests := []struct { + name string + requestArgs map[string]interface{} + expectError bool + expectedErrMsg string + }{ + { + name: "successfully get toolset tools", + requestArgs: map[string]interface{}{ + "toolset": "mock-toolset", + }, + expectError: false, + }, + { + name: "toolset not found", + requestArgs: map[string]interface{}{ + "toolset": "non-existent", + }, + expectError: true, + expectedErrMsg: "Toolset non-existent not found", + }, + { + name: "missing toolset parameter", + requestArgs: map[string]interface{}{ + // No toolset parameter + }, + expectError: true, + expectedErrMsg: "missing required parameter: toolset", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, handler := GetToolsetsTools(tsg, translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, _, err := handler(context.Background(), &request, tc.requestArgs) + + // Verify results + require.NoError(t, err) + if tc.expectError { + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.False(t, result.IsError) + textContent := getTextResult(t, result) + + // Parse JSON response + var tools []map[string]string + err = json.Unmarshal([]byte(textContent.Text), &tools) + require.NoError(t, err) + + // Verify we have the expected tool + assert.Len(t, tools, 1) + assert.Equal(t, "get_dependabot_alert", tools[0]["name"]) + assert.Equal(t, "mock-toolset", tools[0]["toolset"]) + }) + } +} From 0807bfc7b2c6ff8c2c03797b843fd3c31092919e Mon Sep 17 00:00:00 2001 From: Adam Holt Date: Thu, 20 Nov 2025 17:13:11 +0100 Subject: [PATCH 3/5] Enable dynamic tools --- pkg/github/tools.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 11d148695..cb7b47505 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -390,9 +390,9 @@ func InitDynamicToolset(s *mcp.Server, tsg *toolsets.ToolsetGroup, t translation // Need to add the dynamic toolset last so it can be used to enable other toolsets dynamicToolSelection := toolsets.NewToolset(ToolsetMetadataDynamic.ID, ToolsetMetadataDynamic.Description). AddReadTools( - // toolsets.NewServerTool(ListAvailableToolsets(tsg, t)), - // toolsets.NewServerTool(GetToolsetsTools(tsg, t)), - // toolsets.NewServerTool(EnableToolset(s, tsg, t)), + toolsets.NewServerTool(ListAvailableToolsets(tsg, t)), + toolsets.NewServerTool(GetToolsetsTools(tsg, t)), + toolsets.NewServerTool(EnableToolset(s, tsg, t)), ) dynamicToolSelection.Enabled = true From 2e5f0927422af65dc28b8443113e14e641de8fd0 Mon Sep 17 00:00:00 2001 From: Adam Holt Date: Thu, 20 Nov 2025 17:13:29 +0100 Subject: [PATCH 4/5] Remove new test and toolsnaps, we can follow up with this --- pkg/github/__toolsnaps__/enable_toolset.snap | 23 -- .../__toolsnaps__/get_toolset_tools.snap | 23 -- .../list_available_toolsets.snap | 11 - pkg/github/dynamic_tools_test.go | 241 ------------------ 4 files changed, 298 deletions(-) delete mode 100644 pkg/github/__toolsnaps__/enable_toolset.snap delete mode 100644 pkg/github/__toolsnaps__/get_toolset_tools.snap delete mode 100644 pkg/github/__toolsnaps__/list_available_toolsets.snap delete mode 100644 pkg/github/dynamic_tools_test.go diff --git a/pkg/github/__toolsnaps__/enable_toolset.snap b/pkg/github/__toolsnaps__/enable_toolset.snap deleted file mode 100644 index 7259c56f6..000000000 --- a/pkg/github/__toolsnaps__/enable_toolset.snap +++ /dev/null @@ -1,23 +0,0 @@ -{ - "annotations": { - "readOnlyHint": true, - "title": "Enable a toolset" - }, - "description": "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable", - "inputSchema": { - "type": "object", - "required": [ - "toolset" - ], - "properties": { - "toolset": { - "type": "string", - "description": "The name of the toolset to enable", - "enum": [ - "mock-toolset" - ] - } - } - }, - "name": "enable_toolset" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/get_toolset_tools.snap b/pkg/github/__toolsnaps__/get_toolset_tools.snap deleted file mode 100644 index ace797d6c..000000000 --- a/pkg/github/__toolsnaps__/get_toolset_tools.snap +++ /dev/null @@ -1,23 +0,0 @@ -{ - "annotations": { - "readOnlyHint": true, - "title": "List all tools in a toolset" - }, - "description": "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task", - "inputSchema": { - "type": "object", - "required": [ - "toolset" - ], - "properties": { - "toolset": { - "type": "string", - "description": "The name of the toolset you want to get the tools for", - "enum": [ - "mock-toolset" - ] - } - } - }, - "name": "get_toolset_tools" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_available_toolsets.snap b/pkg/github/__toolsnaps__/list_available_toolsets.snap deleted file mode 100644 index bcdf0537d..000000000 --- a/pkg/github/__toolsnaps__/list_available_toolsets.snap +++ /dev/null @@ -1,11 +0,0 @@ -{ - "annotations": { - "readOnlyHint": true, - "title": "List available toolsets" - }, - "description": "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call", - "inputSchema": { - "type": "object" - }, - "name": "list_available_toolsets" -} \ No newline at end of file diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go deleted file mode 100644 index 269667d1f..000000000 --- a/pkg/github/dynamic_tools_test.go +++ /dev/null @@ -1,241 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "testing" - - "github.com/github/github-mcp-server/internal/toolsnaps" - "github.com/github/github-mcp-server/pkg/toolsets" - "github.com/github/github-mcp-server/pkg/translations" - "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func Test_EnableToolset(t *testing.T) { - // Create a mock toolset group - tsg := toolsets.NewToolsetGroup(false) - mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") - tsg.AddToolset(mockToolset) - - // Create mock server - s := NewServer("test", &mcp.ServerOptions{}) - - // Verify tool definition - tool, _ := EnableToolset(s, tsg, translations.NullTranslationHelper) - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - // Validate tool schema - assert.Equal(t, "enable_toolset", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.True(t, tool.Annotations.ReadOnlyHint, "enable_toolset tool should be read-only") - - tests := []struct { - name string - requestArgs map[string]interface{} - expectError bool - expectedErrMsg string - }{ - { - name: "successfully enable toolset", - requestArgs: map[string]interface{}{ - "toolset": "mock-toolset", - }, - expectError: false, - }, - { - name: "toolset not found", - requestArgs: map[string]interface{}{ - "toolset": "non-existent", - }, - expectError: true, - expectedErrMsg: "Toolset non-existent not found", - }, - { - name: "missing toolset parameter", - requestArgs: map[string]interface{}{ - // No toolset parameter - }, - expectError: true, - expectedErrMsg: "missing required parameter: toolset", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - // Reset toolset state - mockToolset.Enabled = false - - // Create new instances for each test - s := NewServer("test", &mcp.ServerOptions{}) - tsg := toolsets.NewToolsetGroup(false) - mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") - tsg.AddToolset(mockToolset) - - _, handler := EnableToolset(s, tsg, translations.NullTranslationHelper) - - // Create call request - request := createMCPRequest(tc.requestArgs) - - // Call handler - result, _, err := handler(context.Background(), &request, tc.requestArgs) - - // Verify results - require.NoError(t, err) - if tc.expectError { - require.True(t, result.IsError) - errorContent := getErrorResult(t, result) - assert.Contains(t, errorContent.Text, tc.expectedErrMsg) - return - } - - require.False(t, result.IsError) - textContent := getTextResult(t, result) - assert.Contains(t, textContent.Text, "enabled") - }) - } -} - -func Test_ListAvailableToolsets(t *testing.T) { - // Create a mock toolset group - tsg := toolsets.NewToolsetGroup(false) - mockToolset1 := toolsets.NewToolset("toolset1", "First toolset") - mockToolset2 := toolsets.NewToolset("toolset2", "Second toolset") - tsg.AddToolset(mockToolset1) - tsg.AddToolset(mockToolset2) - - // Verify tool definition - tool, _ := ListAvailableToolsets(tsg, translations.NullTranslationHelper) - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - // Validate tool schema - assert.Equal(t, "list_available_toolsets", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.True(t, tool.Annotations.ReadOnlyHint, "list_available_toolsets tool should be read-only") - - t.Run("successfully list toolsets", func(t *testing.T) { - _, handler := ListAvailableToolsets(tsg, translations.NullTranslationHelper) - - // Create call request with empty args - request := createMCPRequest(map[string]interface{}{}) - - // Call handler - result, _, err := handler(context.Background(), &request, map[string]interface{}{}) - - // Verify results - require.NoError(t, err) - require.False(t, result.IsError) - - textContent := getTextResult(t, result) - - // Parse JSON response - var toolsets []map[string]string - err = json.Unmarshal([]byte(textContent.Text), &toolsets) - require.NoError(t, err) - - // Verify we have the expected toolsets - assert.Len(t, toolsets, 2) - foundToolset1 := false - foundToolset2 := false - for _, ts := range toolsets { - if ts["name"] == "toolset1" { - foundToolset1 = true - assert.Equal(t, "First toolset", ts["description"]) - assert.Equal(t, "true", ts["can_enable"]) - } - if ts["name"] == "toolset2" { - foundToolset2 = true - assert.Equal(t, "Second toolset", ts["description"]) - assert.Equal(t, "true", ts["can_enable"]) - } - } - assert.True(t, foundToolset1, "Expected to find toolset1") - assert.True(t, foundToolset2, "Expected to find toolset2") - }) -} - -func Test_GetToolsetsTools(t *testing.T) { - // Create a mock toolset group - tsg := toolsets.NewToolsetGroup(false) - mockToolset := toolsets.NewToolset("mock-toolset", "Mock toolset for testing") - - // Add a mock tool to the toolset - mockTool, mockHandler := GetDependabotAlert(stubGetClientFn(nil), translations.NullTranslationHelper) - mockToolset.AddReadTools(toolsets.NewServerTool(mockTool, mockHandler)) - tsg.AddToolset(mockToolset) - - // Verify tool definition - tool, _ := GetToolsetsTools(tsg, translations.NullTranslationHelper) - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - // Validate tool schema - assert.Equal(t, "get_toolset_tools", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.True(t, tool.Annotations.ReadOnlyHint, "get_toolset_tools tool should be read-only") - - tests := []struct { - name string - requestArgs map[string]interface{} - expectError bool - expectedErrMsg string - }{ - { - name: "successfully get toolset tools", - requestArgs: map[string]interface{}{ - "toolset": "mock-toolset", - }, - expectError: false, - }, - { - name: "toolset not found", - requestArgs: map[string]interface{}{ - "toolset": "non-existent", - }, - expectError: true, - expectedErrMsg: "Toolset non-existent not found", - }, - { - name: "missing toolset parameter", - requestArgs: map[string]interface{}{ - // No toolset parameter - }, - expectError: true, - expectedErrMsg: "missing required parameter: toolset", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - _, handler := GetToolsetsTools(tsg, translations.NullTranslationHelper) - - // Create call request - request := createMCPRequest(tc.requestArgs) - - // Call handler - result, _, err := handler(context.Background(), &request, tc.requestArgs) - - // Verify results - require.NoError(t, err) - if tc.expectError { - require.True(t, result.IsError) - errorContent := getErrorResult(t, result) - assert.Contains(t, errorContent.Text, tc.expectedErrMsg) - return - } - - require.False(t, result.IsError) - textContent := getTextResult(t, result) - - // Parse JSON response - var tools []map[string]string - err = json.Unmarshal([]byte(textContent.Text), &tools) - require.NoError(t, err) - - // Verify we have the expected tool - assert.Len(t, tools, 1) - assert.Equal(t, "get_dependabot_alert", tools[0]["name"]) - assert.Equal(t, "mock-toolset", tools[0]["toolset"]) - }) - } -} From f6cfda9565f0490ee1f5bc17138f5dacb6d9e5da Mon Sep 17 00:00:00 2001 From: Adam Holt Date: Thu, 20 Nov 2025 17:16:30 +0100 Subject: [PATCH 5/5] Just return the tool and handler directly instead of assigning to variables first. This stops copilot complaining in review that the variables are unused. --- pkg/github/dynamic_tools.go | 239 +++++++++++++++++------------------- 1 file changed, 115 insertions(+), 124 deletions(-) diff --git a/pkg/github/dynamic_tools.go b/pkg/github/dynamic_tools.go index b51889dd7..c65510246 100644 --- a/pkg/github/dynamic_tools.go +++ b/pkg/github/dynamic_tools.go @@ -21,149 +21,140 @@ func ToolsetEnum(toolsetGroup *toolsets.ToolsetGroup) []any { } func EnableToolset(s *mcp.Server, toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { - tool := mcp.Tool{ - Name: "enable_toolset", - Description: t("TOOL_ENABLE_TOOLSET_DESCRIPTION", "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable"), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_ENABLE_TOOLSET_USER_TITLE", "Enable a toolset"), - // Not modifying GitHub data so no need to show a warning - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "toolset": { - Type: "string", - Description: "The name of the toolset to enable", - Enum: ToolsetEnum(toolsetGroup), + return mcp.Tool{ + Name: "enable_toolset", + Description: t("TOOL_ENABLE_TOOLSET_DESCRIPTION", "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ENABLE_TOOLSET_USER_TITLE", "Enable a toolset"), + // Not modifying GitHub data so no need to show a warning + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "toolset": { + Type: "string", + Description: "The name of the toolset to enable", + Enum: ToolsetEnum(toolsetGroup), + }, }, + Required: []string{"toolset"}, }, - Required: []string{"toolset"}, }, - } + mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsets back to a map for JSON serialization + toolsetName, err := RequiredParam[string](args, "toolset") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + toolset := toolsetGroup.Toolsets[toolsetName] + if toolset == nil { + return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil + } + if toolset.Enabled { + return utils.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil, nil + } + + toolset.Enabled = true - handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - // We need to convert the toolsets back to a map for JSON serialization - toolsetName, err := RequiredParam[string](args, "toolset") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - toolset := toolsetGroup.Toolsets[toolsetName] - if toolset == nil { - return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil - } - if toolset.Enabled { - return utils.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil, nil - } - - toolset.Enabled = true - - // caution: this currently affects the global tools and notifies all clients: - // - // Send notification to all initialized sessions - // s.sendNotificationToAllClients("notifications/tools/list_changed", nil) - for _, serverTool := range toolset.GetActiveTools() { - serverTool.RegisterFunc(s) - } - - return utils.NewToolResultText(fmt.Sprintf("Toolset %s enabled", toolsetName)), nil, nil - }) - - return tool, handler + // caution: this currently affects the global tools and notifies all clients: + // + // Send notification to all initialized sessions + // s.sendNotificationToAllClients("notifications/tools/list_changed", nil) + for _, serverTool := range toolset.GetActiveTools() { + serverTool.RegisterFunc(s) + } + + return utils.NewToolResultText(fmt.Sprintf("Toolset %s enabled", toolsetName)), nil, nil + }) } func ListAvailableToolsets(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { - tool := mcp.Tool{ - Name: "list_available_toolsets", - Description: t("TOOL_LIST_AVAILABLE_TOOLSETS_DESCRIPTION", "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call"), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_LIST_AVAILABLE_TOOLSETS_USER_TITLE", "List available toolsets"), - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{}, + return mcp.Tool{ + Name: "list_available_toolsets", + Description: t("TOOL_LIST_AVAILABLE_TOOLSETS_DESCRIPTION", "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_LIST_AVAILABLE_TOOLSETS_USER_TITLE", "List available toolsets"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{}, + }, }, - } - - handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { - // We need to convert the toolsetGroup back to a map for JSON serialization - - payload := []map[string]string{} - - for name, ts := range toolsetGroup.Toolsets { - { - t := map[string]string{ - "name": name, - "description": ts.Description, - "can_enable": "true", - "currently_enabled": fmt.Sprintf("%t", ts.Enabled), + mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsetGroup back to a map for JSON serialization + + payload := []map[string]string{} + + for name, ts := range toolsetGroup.Toolsets { + { + t := map[string]string{ + "name": name, + "description": ts.Description, + "can_enable": "true", + "currently_enabled": fmt.Sprintf("%t", ts.Enabled), + } + payload = append(payload, t) } - payload = append(payload, t) } - } - r, err := json.Marshal(payload) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal features: %w", err) - } - - return utils.NewToolResultText(string(r)), nil, nil - }) + r, err := json.Marshal(payload) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal features: %w", err) + } - return tool, handler + return utils.NewToolResultText(string(r)), nil, nil + }) } func GetToolsetsTools(toolsetGroup *toolsets.ToolsetGroup, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) { - tool := mcp.Tool{ - Name: "get_toolset_tools", - Description: t("TOOL_GET_TOOLSET_TOOLS_DESCRIPTION", "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task"), - Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_GET_TOOLSET_TOOLS_USER_TITLE", "List all tools in a toolset"), - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "toolset": { - Type: "string", - Description: "The name of the toolset you want to get the tools for", - Enum: ToolsetEnum(toolsetGroup), + return mcp.Tool{ + Name: "get_toolset_tools", + Description: t("TOOL_GET_TOOLSET_TOOLS_DESCRIPTION", "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task"), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_GET_TOOLSET_TOOLS_USER_TITLE", "List all tools in a toolset"), + ReadOnlyHint: true, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "toolset": { + Type: "string", + Description: "The name of the toolset you want to get the tools for", + Enum: ToolsetEnum(toolsetGroup), + }, }, + Required: []string{"toolset"}, }, - Required: []string{"toolset"}, }, - } - - handler := mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - // We need to convert the toolsetGroup back to a map for JSON serialization - toolsetName, err := RequiredParam[string](args, "toolset") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - toolset := toolsetGroup.Toolsets[toolsetName] - if toolset == nil { - return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil - } - payload := []map[string]string{} - - for _, st := range toolset.GetAvailableTools() { - tool := map[string]string{ - "name": st.Tool.Name, - "description": st.Tool.Description, - "can_enable": "true", - "toolset": toolsetName, + mcp.ToolHandlerFor[map[string]any, any](func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + // We need to convert the toolsetGroup back to a map for JSON serialization + toolsetName, err := RequiredParam[string](args, "toolset") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + toolset := toolsetGroup.Toolsets[toolsetName] + if toolset == nil { + return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil + } + payload := []map[string]string{} + + for _, st := range toolset.GetAvailableTools() { + tool := map[string]string{ + "name": st.Tool.Name, + "description": st.Tool.Description, + "can_enable": "true", + "toolset": toolsetName, + } + payload = append(payload, tool) } - payload = append(payload, tool) - } - - r, err := json.Marshal(payload) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal features: %w", err) - } - return utils.NewToolResultText(string(r)), nil, nil - }) + r, err := json.Marshal(payload) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal features: %w", err) + } - return tool, handler + return utils.NewToolResultText(string(r)), nil, nil + }) }