From bb4e2d9e60b8826cf8b7e2fa32c6b4b9e6ccf544 Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 15:36:24 +0200 Subject: [PATCH 01/11] add toolset default to make configuration easier --- README.md | 17 +++- cmd/github-mcp-server/main.go | 3 +- internal/ghmcp/server.go | 35 +++++++- internal/ghmcp/server_test.go | 156 ++++++++++++++++++++++++++++++++++ pkg/github/tools.go | 8 ++ 5 files changed, 216 insertions(+), 3 deletions(-) create mode 100644 internal/ghmcp/server_test.go diff --git a/README.md b/README.md index c0ac851a7..fc6c80d7d 100644 --- a/README.md +++ b/README.md @@ -359,7 +359,9 @@ docker run -i --rm \ ghcr.io/github/github-mcp-server ``` -### The "all" Toolset +### Special toolsets + +#### "all" toolset The special toolset `all` can be provided to enable all available toolsets regardless of any other configuration: @@ -373,6 +375,19 @@ Or using the environment variable: GITHUB_TOOLSETS="all" ./github-mcp-server ``` +#### "default" toolset + +The default toolset `default` is the configuration that gets passed to the server if no toolsets are specified. + +It consists of: +- ... + +To keep the default configuration and add additional toolsets, just specify "default, " + +```bash +GITHUB_TOOLSETS="default,stargazers" ./github-mcp-server +``` + ### Available Toolsets The following sets of tools are available (all are on by default): diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index a0e225293..e34044a89 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -45,8 +45,9 @@ var ( return fmt.Errorf("failed to unmarshal toolsets: %w", err) } + // No passed toolsets configuration means we enable the default toolset if len(enabledToolsets) == 0 { - enabledToolsets = github.GetDefaultToolsetIDs() + enabledToolsets = []string{github.ToolsetMetadataDefault.ID} } stdioServerConfig := ghmcp.StdioServerConfig{ diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index cb44dffa0..3a4274119 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -111,12 +111,14 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { // filter "all" from the enabled toolsets enabledToolsets = make([]string, 0, len(cfg.EnabledToolsets)) for _, toolset := range cfg.EnabledToolsets { - if toolset != "all" { + if toolset != github.ToolsetMetadataAll.ID { enabledToolsets = append(enabledToolsets, toolset) } } } + enabledToolsets = transformDefault(enabledToolsets) + // Generate instructions based on enabled toolsets instructions := github.GenerateInstructions(enabledToolsets) @@ -470,3 +472,34 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro req.Header.Set("Authorization", "Bearer "+t.token) return t.transport.RoundTrip(req) } + +// transformDefault replaces "default" in the enabled toolsets with the actual default toolset IDs. +// If "default" is present, it removes it and adds the default toolset IDs from GetDefaultToolsetIDs(). +// Duplicates are removed from the final result. +func transformDefault(enabledToolsets []string) []string { + hasDefault := false + result := make([]string, 0, len(enabledToolsets)) + seen := make(map[string]bool) + + // First pass: check if "default" exists and collect non-default toolsets + for _, toolset := range enabledToolsets { + if toolset == github.ToolsetMetadataDefault.ID { + hasDefault = true + } else if !seen[toolset] { + result = append(result, toolset) + seen[toolset] = true + } + } + + // If "default" was found, add the default toolset IDs + if hasDefault { + for _, defaultToolset := range github.GetDefaultToolsetIDs() { + if !seen[defaultToolset] { + result = append(result, defaultToolset) + seen[defaultToolset] = true + } + } + } + + return result +} diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go new file mode 100644 index 000000000..65c6fb100 --- /dev/null +++ b/internal/ghmcp/server_test.go @@ -0,0 +1,156 @@ +package ghmcp + +import ( + "testing" + + "github.com/github/github-mcp-server/pkg/github" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTransformDefault(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "empty slice", + input: []string{}, + expected: []string{}, + }, + { + name: "default only", + input: []string{"default"}, + expected: []string{ + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + { + name: "default with additional toolsets", + input: []string{"default", "actions", "gists"}, + expected: []string{ + "actions", + "gists", + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + { + name: "default with overlapping toolsets", + input: []string{"default", "issues", "actions"}, + expected: []string{ + "issues", + "actions", + "context", + "repos", + "pull_requests", + "users", + }, + }, + { + name: "no default present", + input: []string{"actions", "gists", "notifications"}, + expected: []string{"actions", "gists", "notifications"}, + }, + { + name: "duplicate toolsets without default", + input: []string{"actions", "gists", "actions"}, + expected: []string{"actions", "gists"}, + }, + { + name: "duplicate toolsets with default", + input: []string{"actions", "default", "actions", "issues"}, + expected: []string{ + "actions", + "issues", + "context", + "repos", + "pull_requests", + "users", + }, + }, + { + name: "multiple defaults (edge case)", + input: []string{"default", "actions", "default"}, + expected: []string{ + "actions", + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + { + name: "all default toolsets already present with default", + input: []string{"context", "repos", "issues", "pull_requests", "users", "default"}, + expected: []string{ + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := transformDefault(tt.input) + + // Check that the result has the correct length + require.Len(t, result, len(tt.expected), "result length should match expected length") + + // Create a map for easier comparison since order might vary + resultMap := make(map[string]bool) + for _, toolset := range result { + resultMap[toolset] = true + } + + expectedMap := make(map[string]bool) + for _, toolset := range tt.expected { + expectedMap[toolset] = true + } + + // Check that both maps contain the same toolsets + assert.Equal(t, expectedMap, resultMap, "result should contain all expected toolsets without duplicates") + + // Verify no duplicates in result + assert.Len(t, resultMap, len(result), "result should not contain duplicates") + + // Verify "default" is not in the result + assert.False(t, resultMap["default"], "result should not contain 'default'") + }) + } +} + +func TestTransformDefaultWithActualDefaults(t *testing.T) { + // This test verifies that the function uses the actual default toolsets from GetDefaultToolsetIDs() + input := []string{"default"} + result := transformDefault(input) + + defaultToolsets := github.GetDefaultToolsetIDs() + + // Check that result contains all default toolsets + require.Len(t, result, len(defaultToolsets), "result should contain all default toolsets") + + resultMap := make(map[string]bool) + for _, toolset := range result { + resultMap[toolset] = true + } + + for _, defaultToolset := range defaultToolsets { + assert.True(t, resultMap[defaultToolset], "result should contain default toolset: %s", defaultToolset) + } + + // Verify "default" is not in the result + assert.False(t, resultMap["default"], "result should not contain 'default'") +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index a982060de..e0a7a72b1 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -23,6 +23,14 @@ type ToolsetMetadata struct { } var ( + ToolsetMetadataAll = ToolsetMetadata{ + ID: "all", + Description: "Special toolset that enables all available toolsets", + } + ToolsetMetadataDefault = ToolsetMetadata{ + ID: "default", + Description: "Special toolset that enables the default toolset configuration. When no toolsets are specified, this is the set that is enabled", + } ToolsetMetadataContext = ToolsetMetadata{ ID: "context", Description: "Tools that provide context about the current user and GitHub context you are operating in", From 7cdf26f4b6509261a36d12f9b1952afd3a4fad89 Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 15:44:32 +0200 Subject: [PATCH 02/11] fix readme --- README.md | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index fc6c80d7d..94f2dbb48 100644 --- a/README.md +++ b/README.md @@ -87,17 +87,12 @@ Alternatively, to manually configure VS Code, choose the appropriate JSON block ### Configuration -#### Default toolset configuration - -The default configuration is: -- context -- repos -- issues -- pull_requests -- users +#### Toolset configuration See [Remote Server Documentation](docs/remote-server.md) for full details on remote server configuration, toolsets, headers, and advanced usage. This file provides comprehensive instructions and examples for connecting, customizing, and installing the remote GitHub MCP Server in VS Code and other MCP hosts. +When no toolsets are specified, [default toolsets](#default-toolset) are used. + #### Enterprise Cloud with data residency (ghe.com) GitHub Enterprise Cloud can also make use of the remote server. @@ -329,7 +324,7 @@ The GitHub MCP Server supports enabling or disabling specific groups of function _Toolsets are not limited to Tools. Relevant MCP Resources and Prompts are also included where applicable._ -The Local GitHub MCP Server follows the same [default toolset configuration](#default-toolset-configuration) as the remote version. +When no toolsets are specified, [default toolsets](#default-toolset) are used. #### Specifying Toolsets @@ -376,13 +371,16 @@ GITHUB_TOOLSETS="all" ./github-mcp-server ``` #### "default" toolset - The default toolset `default` is the configuration that gets passed to the server if no toolsets are specified. -It consists of: -- ... +The default configuration is: +- context +- repos +- issues +- pull_requests +- users -To keep the default configuration and add additional toolsets, just specify "default, " +To keep the default configuration and add additional toolsets: ```bash GITHUB_TOOLSETS="default,stargazers" ./github-mcp-server From 8d2995d0e3131d0ff48d85ee9b06f3090383f0c6 Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 15:56:37 +0200 Subject: [PATCH 03/11] adding transformer to cleanly handle special toolsets --- internal/ghmcp/server.go | 26 ++++++++++++++++---------- internal/ghmcp/server_test.go | 28 ++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 3a4274119..989da3cf0 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "os/signal" + "slices" "strings" "syscall" "time" @@ -117,7 +118,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { } } - enabledToolsets = transformDefault(enabledToolsets) + enabledToolsets = transformSpecialToolsets(enabledToolsets) // Generate instructions based on enabled toolsets instructions := github.GenerateInstructions(enabledToolsets) @@ -473,19 +474,24 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro return t.transport.RoundTrip(req) } -// transformDefault replaces "default" in the enabled toolsets with the actual default toolset IDs. -// If "default" is present, it removes it and adds the default toolset IDs from GetDefaultToolsetIDs(). +// transformSpecialToolsets handles special toolset keywords in the enabled toolsets list: +// - "all": Returns ["all"] immediately, ignoring all other toolsets +// - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() // Duplicates are removed from the final result. -func transformDefault(enabledToolsets []string) []string { - hasDefault := false - result := make([]string, 0, len(enabledToolsets)) +func transformSpecialToolsets(enabledToolsets []string) []string { + // Check if "all" is present - if so, return immediately + if slices.Contains(enabledToolsets, github.ToolsetMetadataAll.ID) { + return []string{github.ToolsetMetadataAll.ID} + } + + hasDefault := slices.Contains(enabledToolsets, github.ToolsetMetadataDefault.ID) + seen := make(map[string]bool) + result := make([]string, 0, len(enabledToolsets)) - // First pass: check if "default" exists and collect non-default toolsets + // Add non-default toolsets, removing duplicates for _, toolset := range enabledToolsets { - if toolset == github.ToolsetMetadataDefault.ID { - hasDefault = true - } else if !seen[toolset] { + if toolset != github.ToolsetMetadataDefault.ID && !seen[toolset] { result = append(result, toolset) seen[toolset] = true } diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 65c6fb100..3ab720261 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestTransformDefault(t *testing.T) { +func TestTransformSpecialToolsets(t *testing.T) { tests := []struct { name string input []string @@ -19,6 +19,26 @@ func TestTransformDefault(t *testing.T) { input: []string{}, expected: []string{}, }, + { + name: "all only", + input: []string{"all"}, + expected: []string{"all"}, + }, + { + name: "all with other toolsets", + input: []string{"all", "actions", "gists"}, + expected: []string{"all"}, + }, + { + name: "all at the end", + input: []string{"actions", "gists", "all"}, + expected: []string{"all"}, + }, + { + name: "all with default", + input: []string{"default", "all", "actions"}, + expected: []string{"all"}, + }, { name: "default only", input: []string{"default"}, @@ -104,7 +124,7 @@ func TestTransformDefault(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := transformDefault(tt.input) + result := transformSpecialToolsets(tt.input) // Check that the result has the correct length require.Len(t, result, len(tt.expected), "result length should match expected length") @@ -132,10 +152,10 @@ func TestTransformDefault(t *testing.T) { } } -func TestTransformDefaultWithActualDefaults(t *testing.T) { +func TestTransformSpecialToolsetsWithActualDefaults(t *testing.T) { // This test verifies that the function uses the actual default toolsets from GetDefaultToolsetIDs() input := []string{"default"} - result := transformDefault(input) + result := transformSpecialToolsets(input) defaultToolsets := github.GetDefaultToolsetIDs() From 69b1a3a3e28fe02865e94e55c021fe52642545cb Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 16:37:20 +0200 Subject: [PATCH 04/11] cleaning code --- internal/ghmcp/server.go | 46 +++++------ internal/ghmcp/server_test.go | 145 ++++++++++++++++++++++++---------- 2 files changed, 124 insertions(+), 67 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 989da3cf0..b4f6d5da2 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -10,7 +10,6 @@ import ( "net/url" "os" "os/signal" - "slices" "strings" "syscall" "time" @@ -107,18 +106,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { }, } - enabledToolsets := cfg.EnabledToolsets - if cfg.DynamicToolsets { - // filter "all" from the enabled toolsets - enabledToolsets = make([]string, 0, len(cfg.EnabledToolsets)) - for _, toolset := range cfg.EnabledToolsets { - if toolset != github.ToolsetMetadataAll.ID { - enabledToolsets = append(enabledToolsets, toolset) - } - } - } - - enabledToolsets = transformSpecialToolsets(enabledToolsets) + enabledToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets) // Generate instructions based on enabled toolsets instructions := github.GenerateInstructions(enabledToolsets) @@ -474,30 +462,34 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro return t.transport.RoundTrip(req) } -// transformSpecialToolsets handles special toolset keywords in the enabled toolsets list: -// - "all": Returns ["all"] immediately, ignoring all other toolsets +// cleanToolsets handles special toolset keywords in the enabled toolsets list: +// Duplicates are removed from the result. +// - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true) // - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() -// Duplicates are removed from the final result. -func transformSpecialToolsets(enabledToolsets []string) []string { - // Check if "all" is present - if so, return immediately - if slices.Contains(enabledToolsets, github.ToolsetMetadataAll.ID) { - return []string{github.ToolsetMetadataAll.ID} - } - - hasDefault := slices.Contains(enabledToolsets, github.ToolsetMetadataDefault.ID) - +// When dynamicToolsets is true, filters out "all" from the enabled toolsets. +func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string { seen := make(map[string]bool) result := make([]string, 0, len(enabledToolsets)) // Add non-default toolsets, removing duplicates for _, toolset := range enabledToolsets { - if toolset != github.ToolsetMetadataDefault.ID && !seen[toolset] { - result = append(result, toolset) + if !seen[toolset] { seen[toolset] = true + if toolset != github.ToolsetMetadataDefault.ID && toolset != github.ToolsetMetadataAll.ID { + result = append(result, toolset) + } } } - // If "default" was found, add the default toolset IDs + hasDefault := seen[github.ToolsetMetadataDefault.ID] + hasAll := seen[github.ToolsetMetadataAll.ID] + + // Handle "all" keyword - return early if not in dynamic mode + if hasAll && !dynamicToolsets { + return []string{github.ToolsetMetadataAll.ID} + } + + // Expand "default" keyword to actual default toolsets if hasDefault { for _, defaultToolset := range github.GetDefaultToolsetIDs() { if !seen[defaultToolset] { diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 3ab720261..3ab0c364c 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -10,38 +10,53 @@ import ( func TestTransformSpecialToolsets(t *testing.T) { tests := []struct { - name string - input []string - expected []string + name string + input []string + dynamicToolsets bool + expected []string }{ { - name: "empty slice", - input: []string{}, - expected: []string{}, + name: "empty slice", + input: []string{}, + dynamicToolsets: false, + expected: []string{}, }, { - name: "all only", - input: []string{"all"}, - expected: []string{"all"}, + name: "nil input slice", + input: nil, + dynamicToolsets: false, + expected: []string{}, }, + // all test cases { - name: "all with other toolsets", - input: []string{"all", "actions", "gists"}, - expected: []string{"all"}, + name: "all only", + input: []string{"all"}, + dynamicToolsets: false, + expected: []string{"all"}, }, { - name: "all at the end", - input: []string{"actions", "gists", "all"}, - expected: []string{"all"}, + name: "all appears multiple times", + input: []string{"all", "actions", "all"}, + dynamicToolsets: false, + expected: []string{"all"}, }, { - name: "all with default", - input: []string{"default", "all", "actions"}, - expected: []string{"all"}, + name: "all with other toolsets", + input: []string{"all", "actions", "gists"}, + dynamicToolsets: false, + expected: []string{"all"}, }, { - name: "default only", - input: []string{"default"}, + name: "all with default", + input: []string{"default", "all", "actions"}, + dynamicToolsets: false, + expected: []string{"all"}, + }, + // default test cases + { + name: "default only", + input: []string{"default"}, + dynamicToolsets: false, expected: []string{ "context", "repos", @@ -51,8 +66,9 @@ func TestTransformSpecialToolsets(t *testing.T) { }, }, { - name: "default with additional toolsets", - input: []string{"default", "actions", "gists"}, + name: "default with additional toolsets", + input: []string{"default", "actions", "gists"}, + dynamicToolsets: false, expected: []string{ "actions", "gists", @@ -64,44 +80,80 @@ func TestTransformSpecialToolsets(t *testing.T) { }, }, { - name: "default with overlapping toolsets", - input: []string{"default", "issues", "actions"}, + name: "no default present", + input: []string{"actions", "gists", "notifications"}, + dynamicToolsets: false, + expected: []string{"actions", "gists", "notifications"}, + }, + { + name: "duplicate toolsets without default", + input: []string{"actions", "gists", "actions"}, + dynamicToolsets: false, + expected: []string{"actions", "gists"}, + }, + { + name: "duplicate toolsets with default", + input: []string{"context", "repos", "issues", "pull_requests", "users", "default"}, + dynamicToolsets: false, expected: []string{ + "context", + "repos", "issues", + "pull_requests", + "users", + }, + }, + { + name: "default appears multiple times with different toolsets in between", + input: []string{"default", "actions", "default", "gists", "default"}, + dynamicToolsets: false, + expected: []string{ "actions", + "gists", "context", "repos", + "issues", "pull_requests", "users", }, }, + // Dynamic toolsets test cases { - name: "no default present", - input: []string{"actions", "gists", "notifications"}, - expected: []string{"actions", "gists", "notifications"}, + name: "dynamic toolsets - all only should be filtered", + input: []string{"all"}, + dynamicToolsets: true, + expected: []string{}, }, { - name: "duplicate toolsets without default", - input: []string{"actions", "gists", "actions"}, - expected: []string{"actions", "gists"}, + name: "dynamic toolsets - all with other toolsets", + input: []string{"all", "actions", "gists"}, + dynamicToolsets: true, + expected: []string{"actions", "gists"}, }, { - name: "duplicate toolsets with default", - input: []string{"actions", "default", "actions", "issues"}, + name: "dynamic toolsets - all with default", + input: []string{"all", "default", "actions"}, + dynamicToolsets: true, expected: []string{ "actions", - "issues", "context", "repos", + "issues", "pull_requests", "users", }, }, { - name: "multiple defaults (edge case)", - input: []string{"default", "actions", "default"}, + name: "dynamic toolsets - no all present", + input: []string{"actions", "gists"}, + dynamicToolsets: true, + expected: []string{"actions", "gists"}, + }, + { + name: "dynamic toolsets - default only", + input: []string{"default"}, + dynamicToolsets: true, expected: []string{ - "actions", "context", "repos", "issues", @@ -110,8 +162,9 @@ func TestTransformSpecialToolsets(t *testing.T) { }, }, { - name: "all default toolsets already present with default", - input: []string{"context", "repos", "issues", "pull_requests", "users", "default"}, + name: "only special keywords with dynamic mode", + input: []string{"all", "default"}, + dynamicToolsets: true, expected: []string{ "context", "repos", @@ -120,11 +173,23 @@ func TestTransformSpecialToolsets(t *testing.T) { "users", }, }, + { + name: "all with default and overlapping default toolsets in dynamic mode", + input: []string{"all", "default", "issues", "repos"}, + dynamicToolsets: true, + expected: []string{ + "issues", + "repos", + "context", + "pull_requests", + "users", + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := transformSpecialToolsets(tt.input) + result := cleanToolsets(tt.input, tt.dynamicToolsets) // Check that the result has the correct length require.Len(t, result, len(tt.expected), "result length should match expected length") @@ -155,7 +220,7 @@ func TestTransformSpecialToolsets(t *testing.T) { func TestTransformSpecialToolsetsWithActualDefaults(t *testing.T) { // This test verifies that the function uses the actual default toolsets from GetDefaultToolsetIDs() input := []string{"default"} - result := transformSpecialToolsets(input) + result := cleanToolsets(input, false) defaultToolsets := github.GetDefaultToolsetIDs() From 0c8980dce6955a467e4efab77fce4e41f6abbec1 Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 17:01:49 +0200 Subject: [PATCH 05/11] fixing cli message --- internal/ghmcp/server.go | 14 ++++++++------ internal/ghmcp/server_test.go | 27 +++++++++++++++++++++++++++ pkg/github/tools.go | 12 +++++++++--- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index b4f6d5da2..eb47d5eba 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -463,7 +463,8 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro } // cleanToolsets handles special toolset keywords in the enabled toolsets list: -// Duplicates are removed from the result. +// - Duplicates are removed from the result. +// - Removes whitespaces // - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true) // - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() // When dynamicToolsets is true, filters out "all" from the enabled toolsets. @@ -471,12 +472,13 @@ func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string { seen := make(map[string]bool) result := make([]string, 0, len(enabledToolsets)) - // Add non-default toolsets, removing duplicates + // Add non-default toolsets, removing duplicates and trimming whitespace for _, toolset := range enabledToolsets { - if !seen[toolset] { - seen[toolset] = true - if toolset != github.ToolsetMetadataDefault.ID && toolset != github.ToolsetMetadataAll.ID { - result = append(result, toolset) + trimmed := strings.TrimSpace(toolset) + if !seen[trimmed] { + seen[trimmed] = true + if trimmed != github.ToolsetMetadataDefault.ID && trimmed != github.ToolsetMetadataAll.ID { + result = append(result, trimmed) } } } diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 3ab0c364c..3813c207f 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -185,6 +185,33 @@ func TestTransformSpecialToolsets(t *testing.T) { "users", }, }, + // Whitespace test cases + { + name: "whitespace check- leading and trailing whitespace on regular toolsets", + input: []string{" actions ", " gists ", "notifications"}, + dynamicToolsets: false, + expected: []string{"actions", "gists", "notifications"}, + }, + { + name: "whitespace check - default toolset", + input: []string{" actions ", " default ", "notifications"}, + dynamicToolsets: false, + expected: []string{ + "actions", + "notifications", + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + { + name: "whitespace check - all toolset", + input: []string{" actions ", " gists ", "notifications", " all "}, + dynamicToolsets: false, + expected: []string{"all"}, + }, } for _, tt := range tests { diff --git a/pkg/github/tools.go b/pkg/github/tools.go index e0a7a72b1..85a4541bd 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -422,8 +422,14 @@ func GenerateToolsetsHelp() string { availableTools := strings.Join(availableToolsLines, ",\n\t ") toolsetsHelp := fmt.Sprintf("Comma-separated list of tool groups to enable (no spaces).\n"+ - "Default: %s\n"+ - "Available: %s\n", defaultTools, availableTools) + - "To enable all tools, use \"all\"." + "Available: %s\n", availableTools) + + "Special toolset keywords:\n" + + " - all: Enables all available toolsets\n" + + fmt.Sprintf(" - default: Enables the default toolset configuration of:\n\t %s\n", defaultTools) + + "Examples:\n" + + " - --toolsets=actions,gists,notifications\n" + + " - Default + additional: --toolsets=default,actions,gists\n" + + " - All tools: --toolsets=all" + return toolsetsHelp } From 5dfeacab0372b295a5094af37d513f038fefbee1 Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 17:03:29 +0200 Subject: [PATCH 06/11] remove duplicated test --- internal/ghmcp/server_test.go | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 3813c207f..3086ddf4b 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -3,12 +3,11 @@ package ghmcp import ( "testing" - "github.com/github/github-mcp-server/pkg/github" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestTransformSpecialToolsets(t *testing.T) { +func TestCleanToolsets(t *testing.T) { tests := []struct { name string input []string @@ -243,26 +242,3 @@ func TestTransformSpecialToolsets(t *testing.T) { }) } } - -func TestTransformSpecialToolsetsWithActualDefaults(t *testing.T) { - // This test verifies that the function uses the actual default toolsets from GetDefaultToolsetIDs() - input := []string{"default"} - result := cleanToolsets(input, false) - - defaultToolsets := github.GetDefaultToolsetIDs() - - // Check that result contains all default toolsets - require.Len(t, result, len(defaultToolsets), "result should contain all default toolsets") - - resultMap := make(map[string]bool) - for _, toolset := range result { - resultMap[toolset] = true - } - - for _, defaultToolset := range defaultToolsets { - assert.True(t, resultMap[defaultToolset], "result should contain default toolset: %s", defaultToolset) - } - - // Verify "default" is not in the result - assert.False(t, resultMap["default"], "result should not contain 'default'") -} From 6c9c201215fa9026bba57e6c21ce2be875075b9f Mon Sep 17 00:00:00 2001 From: Tony Truong Date: Wed, 15 Oct 2025 17:08:06 +0200 Subject: [PATCH 07/11] Update internal/ghmcp/server.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/ghmcp/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index eb47d5eba..e99864ce8 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -464,7 +464,7 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro // cleanToolsets handles special toolset keywords in the enabled toolsets list: // - Duplicates are removed from the result. -// - Removes whitespaces +// - Removes whitespace // - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true) // - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() // When dynamicToolsets is true, filters out "all" from the enabled toolsets. From d78630632e447a488309d9c75301a1d23f14342e Mon Sep 17 00:00:00 2001 From: Tony Truong Date: Wed, 15 Oct 2025 17:08:20 +0200 Subject: [PATCH 08/11] Update internal/ghmcp/server_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/ghmcp/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 3086ddf4b..c09945b03 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -186,7 +186,7 @@ func TestCleanToolsets(t *testing.T) { }, // Whitespace test cases { - name: "whitespace check- leading and trailing whitespace on regular toolsets", + name: "whitespace check - leading and trailing whitespace on regular toolsets", input: []string{" actions ", " gists ", "notifications"}, dynamicToolsets: false, expected: []string{"actions", "gists", "notifications"}, From 5390807838715d65f64a7a8e32c83edd057e240a Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 17:43:19 +0200 Subject: [PATCH 09/11] adding error message for invalid toolsets --- internal/ghmcp/server.go | 27 ++++++++++++++++---- internal/ghmcp/server_test.go | 46 ++++++++++++++++++++++++++++++----- pkg/github/tools.go | 12 +++++++++ 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index eb47d5eba..5ab0d27e5 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -106,7 +106,12 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { }, } - enabledToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets) + enabledToolsets, invalidToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets) + + // Log warning about invalid toolsets + if len(invalidToolsets) > 0 { + slog.Warn("invalid toolsets ignored", "toolsets", strings.Join(invalidToolsets, ", ")) + } // Generate instructions based on enabled toolsets instructions := github.GenerateInstructions(enabledToolsets) @@ -465,20 +470,32 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro // cleanToolsets handles special toolset keywords in the enabled toolsets list: // - Duplicates are removed from the result. // - Removes whitespaces +// - Validates toolset names and returns invalid ones separately // - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true) // - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() // When dynamicToolsets is true, filters out "all" from the enabled toolsets. -func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string { +// Returns: (validToolsets, invalidToolsets) +func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) ([]string, []string) { seen := make(map[string]bool) result := make([]string, 0, len(enabledToolsets)) + invalid := make([]string, 0) + validIDs := github.GetValidToolsetIDs() // Add non-default toolsets, removing duplicates and trimming whitespace for _, toolset := range enabledToolsets { trimmed := strings.TrimSpace(toolset) + if trimmed == "" { + continue + } if !seen[trimmed] { seen[trimmed] = true if trimmed != github.ToolsetMetadataDefault.ID && trimmed != github.ToolsetMetadataAll.ID { - result = append(result, trimmed) + // Validate the toolset name + if validIDs[trimmed] { + result = append(result, trimmed) + } else { + invalid = append(invalid, trimmed) + } } } } @@ -488,7 +505,7 @@ func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string { // Handle "all" keyword - return early if not in dynamic mode if hasAll && !dynamicToolsets { - return []string{github.ToolsetMetadataAll.ID} + return []string{github.ToolsetMetadataAll.ID}, invalid } // Expand "default" keyword to actual default toolsets @@ -501,5 +518,5 @@ func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) []string { } } - return result + return result, invalid } diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 3086ddf4b..54994a149 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -13,6 +13,7 @@ func TestCleanToolsets(t *testing.T) { input []string dynamicToolsets bool expected []string + expectedInvalid []string }{ { name: "empty slice", @@ -211,16 +212,41 @@ func TestCleanToolsets(t *testing.T) { dynamicToolsets: false, expected: []string{"all"}, }, + // Invalid toolset test cases + { + name: "mix of valid and invalid toolsets", + input: []string{"actions", "invalid_toolset", "gists", "typo_repo"}, + dynamicToolsets: false, + expected: []string{"actions", "gists"}, + expectedInvalid: []string{"invalid_toolset", "typo_repo"}, + }, + { + name: "invalid with whitespace", + input: []string{" invalid_tool ", " actions ", " typo_gist "}, + dynamicToolsets: false, + expected: []string{"actions"}, + expectedInvalid: []string{"invalid_tool", "typo_gist"}, + }, + { + name: "empty string in toolsets", + input: []string{"", "actions", " ", "gists"}, + dynamicToolsets: false, + expected: []string{"actions", "gists"}, + expectedInvalid: []string{}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := cleanToolsets(tt.input, tt.dynamicToolsets) + result, invalid := cleanToolsets(tt.input, tt.dynamicToolsets) - // Check that the result has the correct length require.Len(t, result, len(tt.expected), "result length should match expected length") - // Create a map for easier comparison since order might vary + if tt.expectedInvalid == nil { + tt.expectedInvalid = []string{} + } + require.Len(t, invalid, len(tt.expectedInvalid), "invalid length should match expected invalid length") + resultMap := make(map[string]bool) for _, toolset := range result { resultMap[toolset] = true @@ -231,13 +257,21 @@ func TestCleanToolsets(t *testing.T) { expectedMap[toolset] = true } - // Check that both maps contain the same toolsets + invalidMap := make(map[string]bool) + for _, toolset := range invalid { + invalidMap[toolset] = true + } + + expectedInvalidMap := make(map[string]bool) + for _, toolset := range tt.expectedInvalid { + expectedInvalidMap[toolset] = true + } + assert.Equal(t, expectedMap, resultMap, "result should contain all expected toolsets without duplicates") + assert.Equal(t, expectedInvalidMap, invalidMap, "invalid should contain all expected invalid toolsets") - // Verify no duplicates in result assert.Len(t, resultMap, len(result), "result should not contain duplicates") - // Verify "default" is not in the result assert.False(t, resultMap["default"], "result should not contain 'default'") }) } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 85a4541bd..a0b1690c9 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -133,6 +133,18 @@ func AvailableTools() []ToolsetMetadata { } } +// GetValidToolsetIDs returns a map of all valid toolset IDs for quick lookup +func GetValidToolsetIDs() map[string]bool { + validIDs := make(map[string]bool) + for _, tool := range AvailableTools() { + validIDs[tool.ID] = true + } + // Add special keywords + validIDs[ToolsetMetadataAll.ID] = true + validIDs[ToolsetMetadataDefault.ID] = true + return validIDs +} + func GetDefaultToolsetIDs() []string { return []string{ ToolsetMetadataContext.ID, From 40fbb6c386f8be77afed801692ba5edae4e4a73e Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 17:46:54 +0200 Subject: [PATCH 10/11] fix merge conflict --- internal/ghmcp/server.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 0bd009070..15969ca92 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -467,17 +467,13 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro return t.transport.RoundTrip(req) } -// cleanToolsets handles special toolset keywords in the enabled toolsets list: -// - Duplicates are removed from the result. -<<<<<<< HEAD +// cleanToolsets cleans and handles special toolset keywords: +// - Duplicates are removed from the result // - Removes whitespaces // - Validates toolset names and returns invalid ones separately -======= -// - Removes whitespace ->>>>>>> d78630632e447a488309d9c75301a1d23f14342e -// - "all": Returns ["all"] immediately, ignoring all other toolsets (unless dynamicToolsets is true) +// - "all": Returns ["all"] immediately, ignoring all other toolsets +// - when dynamicToolsets is true, filters out "all" from the enabled toolsets // - "default": Replaces with the actual default toolset IDs from GetDefaultToolsetIDs() -// When dynamicToolsets is true, filters out "all" from the enabled toolsets. // Returns: (validToolsets, invalidToolsets) func cleanToolsets(enabledToolsets []string, dynamicToolsets bool) ([]string, []string) { seen := make(map[string]bool) From f42f866adc038c4391bb2e1593afe8e8afaf9b1c Mon Sep 17 00:00:00 2001 From: tonytrg Date: Wed, 15 Oct 2025 17:53:09 +0200 Subject: [PATCH 11/11] add better formatting --- internal/ghmcp/server.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 15969ca92..5b4c5c158 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -108,9 +108,8 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { enabledToolsets, invalidToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets) - // Log warning about invalid toolsets if len(invalidToolsets) > 0 { - slog.Warn("invalid toolsets ignored", "toolsets", strings.Join(invalidToolsets, ", ")) + fmt.Fprintf(os.Stderr, "Invalid toolsets ignored: %s\n", strings.Join(invalidToolsets, ", ")) } // Generate instructions based on enabled toolsets