diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 5b4c5c158..4b406f9ea 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -106,7 +106,24 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { }, } - enabledToolsets, invalidToolsets := cleanToolsets(cfg.EnabledToolsets, cfg.DynamicToolsets) + enabledToolsets := cfg.EnabledToolsets + + // If dynamic toolsets are enabled, remove "all" from the enabled toolsets + if cfg.DynamicToolsets { + enabledToolsets = github.RemoveToolset(enabledToolsets, github.ToolsetMetadataAll.ID) + } + + // Clean up the passed toolsets + enabledToolsets, invalidToolsets := github.CleanToolsets(enabledToolsets) + + // If "all" is present, override all other toolsets + if github.ContainsToolset(enabledToolsets, github.ToolsetMetadataAll.ID) { + enabledToolsets = []string{github.ToolsetMetadataAll.ID} + } + // If "default" is present, expand to real toolset IDs + if github.ContainsToolset(enabledToolsets, github.ToolsetMetadataDefault.ID) { + enabledToolsets = github.AddDefaultToolset(enabledToolsets) + } if len(invalidToolsets) > 0 { fmt.Fprintf(os.Stderr, "Invalid toolsets ignored: %s\n", strings.Join(invalidToolsets, ", ")) @@ -465,57 +482,3 @@ func (t *bearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro req.Header.Set("Authorization", "Bearer "+t.token) return t.transport.RoundTrip(req) } - -// cleanToolsets cleans and handles special toolset keywords: -// - Duplicates are removed from the result -// - Removes whitespaces -// - Validates toolset names and returns invalid ones separately -// - "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() -// 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 { - // Validate the toolset name - if validIDs[trimmed] { - result = append(result, trimmed) - } else { - invalid = append(invalid, trimmed) - } - } - } - } - - 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}, invalid - } - - // Expand "default" keyword to actual default toolsets - if hasDefault { - for _, defaultToolset := range github.GetDefaultToolsetIDs() { - if !seen[defaultToolset] { - result = append(result, defaultToolset) - seen[defaultToolset] = true - } - } - } - - return result, invalid -} diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go deleted file mode 100644 index c675306f6..000000000 --- a/internal/ghmcp/server_test.go +++ /dev/null @@ -1,278 +0,0 @@ -package ghmcp - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestCleanToolsets(t *testing.T) { - tests := []struct { - name string - input []string - dynamicToolsets bool - expected []string - expectedInvalid []string - }{ - { - name: "empty slice", - input: []string{}, - dynamicToolsets: false, - expected: []string{}, - }, - { - name: "nil input slice", - input: nil, - dynamicToolsets: false, - expected: []string{}, - }, - // all test cases - { - name: "all only", - input: []string{"all"}, - dynamicToolsets: false, - expected: []string{"all"}, - }, - { - name: "all appears multiple times", - input: []string{"all", "actions", "all"}, - dynamicToolsets: false, - expected: []string{"all"}, - }, - { - name: "all with other toolsets", - input: []string{"all", "actions", "gists"}, - dynamicToolsets: false, - expected: []string{"all"}, - }, - { - 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", - "issues", - "pull_requests", - "users", - }, - }, - { - name: "default with additional toolsets", - input: []string{"default", "actions", "gists"}, - dynamicToolsets: false, - expected: []string{ - "actions", - "gists", - "context", - "repos", - "issues", - "pull_requests", - "users", - }, - }, - { - 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: "dynamic toolsets - all only should be filtered", - input: []string{"all"}, - dynamicToolsets: true, - expected: []string{}, - }, - { - name: "dynamic toolsets - all with other toolsets", - input: []string{"all", "actions", "gists"}, - dynamicToolsets: true, - expected: []string{"actions", "gists"}, - }, - { - name: "dynamic toolsets - all with default", - input: []string{"all", "default", "actions"}, - dynamicToolsets: true, - expected: []string{ - "actions", - "context", - "repos", - "issues", - "pull_requests", - "users", - }, - }, - { - 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{ - "context", - "repos", - "issues", - "pull_requests", - "users", - }, - }, - { - name: "only special keywords with dynamic mode", - input: []string{"all", "default"}, - dynamicToolsets: true, - expected: []string{ - "context", - "repos", - "issues", - "pull_requests", - "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", - }, - }, - // 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"}, - }, - // 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, invalid := cleanToolsets(tt.input, tt.dynamicToolsets) - - require.Len(t, result, len(tt.expected), "result length should match expected length") - - 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 - } - - expectedMap := make(map[string]bool) - for _, toolset := range tt.expected { - expectedMap[toolset] = true - } - - 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") - - assert.Len(t, resultMap, len(result), "result should not contain duplicates") - - assert.False(t, resultMap["default"], "result should not contain 'default'") - }) - } -} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index a0b1690c9..31138258a 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -445,3 +445,77 @@ func GenerateToolsetsHelp() string { return toolsetsHelp } + +// AddDefaultToolset removes the default toolset and expands it to the actual default toolset IDs +func AddDefaultToolset(result []string) []string { + hasDefault := false + seen := make(map[string]bool) + for _, toolset := range result { + seen[toolset] = true + if toolset == ToolsetMetadataDefault.ID { + hasDefault = true + } + } + + // Only expand if "default" keyword was found + if !hasDefault { + return result + } + + result = RemoveToolset(result, ToolsetMetadataDefault.ID) + + for _, defaultToolset := range GetDefaultToolsetIDs() { + if !seen[defaultToolset] { + result = append(result, defaultToolset) + } + } + return result +} + +// cleanToolsets cleans and handles special toolset keywords: +// - Duplicates are removed from the result +// - Removes whitespaces +// - Validates toolset names and returns invalid ones separately - for warning reporting +// Returns: (toolsets, invalidToolsets) +func CleanToolsets(enabledToolsets []string) ([]string, []string) { + seen := make(map[string]bool) + result := make([]string, 0, len(enabledToolsets)) + invalid := make([]string, 0) + validIDs := 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 + result = append(result, trimmed) + if !validIDs[trimmed] { + invalid = append(invalid, trimmed) + } + } + } + + return result, invalid +} + +func RemoveToolset(tools []string, toRemove string) []string { + result := make([]string, 0, len(tools)) + for _, tool := range tools { + if tool != toRemove { + result = append(result, tool) + } + } + return result +} + +func ContainsToolset(tools []string, toCheck string) bool { + for _, tool := range tools { + if tool == toCheck { + return true + } + } + return false +} diff --git a/pkg/github/tools_test.go b/pkg/github/tools_test.go new file mode 100644 index 000000000..45c1e746f --- /dev/null +++ b/pkg/github/tools_test.go @@ -0,0 +1,282 @@ +package github + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCleanToolsets(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + expectedInvalid []string + }{ + { + name: "empty slice", + input: []string{}, + expected: []string{}, + }, + { + name: "nil input slice", + input: nil, + expected: []string{}, + }, + // CleanToolsets only cleans - it does NOT filter out special keywords + { + name: "default keyword preserved", + input: []string{"default"}, + expected: []string{"default"}, + }, + { + name: "default with additional toolsets", + input: []string{"default", "actions", "gists"}, + expected: []string{"default", "actions", "gists"}, + }, + { + name: "all keyword preserved", + input: []string{"all", "actions"}, + expected: []string{"all", "actions"}, + }, + { + name: "no special keywords", + input: []string{"actions", "gists", "notifications"}, + expected: []string{"actions", "gists", "notifications"}, + }, + { + name: "duplicate toolsets without special keywords", + input: []string{"actions", "gists", "actions"}, + expected: []string{"actions", "gists"}, + }, + { + name: "duplicate toolsets with default", + input: []string{"context", "repos", "issues", "pull_requests", "users", "default"}, + expected: []string{"context", "repos", "issues", "pull_requests", "users", "default"}, + }, + { + name: "default appears multiple times - duplicates removed", + input: []string{"default", "actions", "default", "gists", "default"}, + expected: []string{"default", "actions", "gists"}, + }, + // Whitespace test cases + { + name: "whitespace check - leading and trailing whitespace on regular toolsets", + input: []string{" actions ", " gists ", "notifications"}, + expected: []string{"actions", "gists", "notifications"}, + }, + { + name: "whitespace check - default toolset with whitespace", + input: []string{" actions ", " default ", "notifications"}, + expected: []string{"actions", "default", "notifications"}, + }, + { + name: "whitespace check - all toolset with whitespace", + input: []string{" all ", " actions "}, + expected: []string{"all", "actions"}, + }, + // Invalid toolset test cases + { + name: "mix of valid and invalid toolsets", + input: []string{"actions", "invalid_toolset", "gists", "typo_repo"}, + expected: []string{"actions", "invalid_toolset", "gists", "typo_repo"}, + expectedInvalid: []string{"invalid_toolset", "typo_repo"}, + }, + { + name: "invalid with whitespace", + input: []string{" invalid_tool ", " actions ", " typo_gist "}, + expected: []string{"invalid_tool", "actions", "typo_gist"}, + expectedInvalid: []string{"invalid_tool", "typo_gist"}, + }, + { + name: "empty string in toolsets", + input: []string{"", "actions", " ", "gists"}, + expected: []string{"actions", "gists"}, + expectedInvalid: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, invalid := CleanToolsets(tt.input) + + require.Len(t, result, len(tt.expected), "result length should match expected length") + + 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 + } + + expectedMap := make(map[string]bool) + for _, toolset := range tt.expected { + expectedMap[toolset] = true + } + + 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") + + assert.Len(t, resultMap, len(result), "result should not contain duplicates") + }) + } +} + +func TestAddDefaultToolset(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "no default keyword - return unchanged", + input: []string{"actions", "gists"}, + expected: []string{"actions", "gists"}, + }, + { + name: "default keyword present - expand and remove default", + 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 - should not duplicate", + input: []string{"default", "context", "repos"}, + expected: []string{ + "context", + "repos", + "issues", + "pull_requests", + "users", + }, + }, + { + name: "empty input", + input: []string{}, + expected: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := AddDefaultToolset(tt.input) + + require.Len(t, result, len(tt.expected), "result length should match expected length") + + 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 + } + + assert.Equal(t, expectedMap, resultMap, "result should contain all expected toolsets") + assert.False(t, resultMap["default"], "result should not contain 'default' keyword") + }) + } +} + +func TestRemoveToolset(t *testing.T) { + tests := []struct { + name string + tools []string + toRemove string + expected []string + }{ + { + name: "remove existing toolset", + tools: []string{"actions", "gists", "notifications"}, + toRemove: "gists", + expected: []string{"actions", "notifications"}, + }, + { + name: "remove from empty slice", + tools: []string{}, + toRemove: "actions", + expected: []string{}, + }, + { + name: "remove duplicate entries", + tools: []string{"actions", "gists", "actions", "notifications"}, + toRemove: "actions", + expected: []string{"gists", "notifications"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := RemoveToolset(tt.tools, tt.toRemove) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestContainsToolset(t *testing.T) { + tests := []struct { + name string + tools []string + toCheck string + expected bool + }{ + { + name: "toolset exists", + tools: []string{"actions", "gists", "notifications"}, + toCheck: "gists", + expected: true, + }, + { + name: "toolset does not exist", + tools: []string{"actions", "gists", "notifications"}, + toCheck: "repos", + expected: false, + }, + { + name: "empty slice", + tools: []string{}, + toCheck: "actions", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := ContainsToolset(tt.tools, tt.toCheck) + assert.Equal(t, tt.expected, result) + }) + } +}