From ac8e6bbee811c28c7cd6056ca6ec4c82ccde1e58 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Sun, 25 Jan 2026 00:03:31 +0000 Subject: [PATCH 1/2] move resolveEnabledToolsets to builder --- internal/ghmcp/server.go | 32 +------ internal/ghmcp/server_test.go | 79 ++--------------- pkg/inventory/builder.go | 57 +++++++++++++ pkg/inventory/registry_test.go | 150 +++++++++++++++++++++++++++++++++ 4 files changed, 218 insertions(+), 100 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 4f68cfd49..05553256c 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -131,33 +131,6 @@ func createGitHubClients(cfg MCPServerConfig, apiHost apiHost) (*githubClients, }, nil } -// resolveEnabledToolsets determines which toolsets should be enabled based on config. -// Returns nil for "use defaults", empty slice for "none", or explicit list. -func resolveEnabledToolsets(cfg MCPServerConfig) []string { - enabledToolsets := cfg.EnabledToolsets - - // In dynamic mode, remove "all" and "default" since users enable toolsets on demand - if cfg.DynamicToolsets && enabledToolsets != nil { - enabledToolsets = github.RemoveToolset(enabledToolsets, string(github.ToolsetMetadataAll.ID)) - enabledToolsets = github.RemoveToolset(enabledToolsets, string(github.ToolsetMetadataDefault.ID)) - } - - if enabledToolsets != nil { - return enabledToolsets - } - if cfg.DynamicToolsets { - // Dynamic mode with no toolsets specified: start empty so users enable on demand - return []string{} - } - if len(cfg.EnabledTools) > 0 { - // When specific tools are requested but no toolsets, don't use default toolsets - // This matches the original behavior: --tools=X alone registers only X - return []string{} - } - // nil means "use defaults" in WithToolsets - return nil -} - func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { apiHost, err := parseAPIHost(cfg.Host) if err != nil { @@ -169,8 +142,6 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { return nil, fmt.Errorf("failed to create GitHub clients: %w", err) } - enabledToolsets := resolveEnabledToolsets(cfg) - // Create feature checker featureChecker := createFeatureChecker(cfg.EnabledFeatures) @@ -178,8 +149,9 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { inventoryBuilder := github.NewInventory(cfg.Translator). WithDeprecatedAliases(github.DeprecatedToolAliases). WithReadOnly(cfg.ReadOnly). - WithToolsets(enabledToolsets). + WithToolsets(cfg.EnabledToolsets). WithTools(cfg.EnabledTools). + WithDynamicMode(cfg.DynamicToolsets). WithFeatureChecker(featureChecker). WithServerInstructions() diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 7854cb77f..8e5ee1735 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/github/github-mcp-server/pkg/translations" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -42,72 +41,12 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { // is already tested in pkg/github/*_test.go. } -// TestResolveEnabledToolsets verifies the toolset resolution logic. -func TestResolveEnabledToolsets(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - cfg MCPServerConfig - expectedResult []string - }{ - { - name: "nil toolsets without dynamic mode and no tools - use defaults", - cfg: MCPServerConfig{ - EnabledToolsets: nil, - DynamicToolsets: false, - EnabledTools: nil, - }, - expectedResult: nil, // nil means "use defaults" - }, - { - name: "nil toolsets with dynamic mode - start empty", - cfg: MCPServerConfig{ - EnabledToolsets: nil, - DynamicToolsets: true, - EnabledTools: nil, - }, - expectedResult: []string{}, // empty slice means no toolsets - }, - { - name: "explicit toolsets", - cfg: MCPServerConfig{ - EnabledToolsets: []string{"repos", "issues"}, - DynamicToolsets: false, - }, - expectedResult: []string{"repos", "issues"}, - }, - { - name: "empty toolsets - disable all", - cfg: MCPServerConfig{ - EnabledToolsets: []string{}, - DynamicToolsets: false, - }, - expectedResult: []string{}, // empty slice means no toolsets - }, - { - name: "specific tools without toolsets - no default toolsets", - cfg: MCPServerConfig{ - EnabledToolsets: nil, - DynamicToolsets: false, - EnabledTools: []string{"get_me"}, - }, - expectedResult: []string{}, // empty slice when tools specified but no toolsets - }, - { - name: "dynamic mode with explicit toolsets removes all and default", - cfg: MCPServerConfig{ - EnabledToolsets: []string{"all", "repos"}, - DynamicToolsets: true, - }, - expectedResult: []string{"repos"}, // "all" is removed in dynamic mode - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - result := resolveEnabledToolsets(tc.cfg) - assert.Equal(t, tc.expectedResult, result) - }) - } -} +// Note: TestResolveEnabledToolsets was removed because the resolveEnabledToolsets +// function was moved to the Inventory Builder (pkg/inventory/builder.go). +// The same functionality is now tested in pkg/inventory/registry_test.go via: +// - TestWithDynamicMode_NilToolsets +// - TestWithDynamicMode_RemovesAllKeyword +// - TestWithDynamicMode_RemovesDefaultKeyword +// - TestWithDynamicMode_ExplicitToolsetsPreserved +// - TestWithDynamicMode_WithAdditionalTools +// - TestWithTools_NilToolsets_UsesEmptyToolsets diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index ff2d06d5d..697402a8c 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -41,6 +41,7 @@ type Builder struct { featureChecker FeatureFlagChecker filters []ToolFilter // filters to apply to all tools generateInstructions bool + dynamicMode bool // when true, "all" and "default" are stripped, empty toolsets is valid } // NewBuilder creates a new Builder. @@ -116,6 +117,17 @@ func (b *Builder) WithTools(toolNames []string) *Builder { return b } +// WithDynamicMode enables dynamic toolset mode. +// In this mode: +// - "all" and "default" keywords are removed from toolsetIDs +// - If no toolsets specified, starts with empty set (users enable on demand) +// +// Returns self for chaining. +func (b *Builder) WithDynamicMode(enabled bool) *Builder { + b.dynamicMode = enabled + return b +} + // WithFeatureChecker sets the feature flag checker function. // The checker receives a context (for actor extraction) and feature flag name, // returns (enabled, error). If error occurs, it will be logged and treated as false. @@ -135,6 +147,48 @@ func (b *Builder) WithFilter(filter ToolFilter) *Builder { return b } +// removeFromSlice removes all occurrences of value from slice. +func removeFromSlice(slice []string, value string) []string { + result := make([]string, 0, len(slice)) + for _, s := range slice { + if s != value { + result = append(result, s) + } + } + return result +} + +// applyDynamicModePreprocessing modifies toolsetIDs based on dynamicMode and additionalTools. +func (b *Builder) applyDynamicModePreprocessing() { + if b.dynamicMode && b.toolsetIDs != nil { + // In dynamic mode, remove "all" and "default" since users enable toolsets on demand + b.toolsetIDs = removeFromSlice(b.toolsetIDs, "all") + b.toolsetIDs = removeFromSlice(b.toolsetIDs, "default") + } + + if b.toolsetIDs != nil { + // Explicit toolsets were provided (possibly modified by dynamic mode) + return + } + + if b.dynamicMode { + // Dynamic mode with no toolsets specified: start empty so users enable on demand + b.toolsetIDs = []string{} + b.toolsetIDsIsNil = false + return + } + + if len(b.additionalTools) > 0 { + // When specific tools are requested but no toolsets, don't use default toolsets + // --tools=X alone registers only X + b.toolsetIDs = []string{} + b.toolsetIDsIsNil = false + return + } + + // Leave as nil (toolsetIDsIsNil = true) to use defaults in processToolsets() +} + // cleanTools trims whitespace and removes duplicates from tool names. // Empty strings after trimming are excluded. func cleanTools(tools []string) []string { @@ -162,6 +216,9 @@ func cleanTools(tools []string) []string { // (i.e., they don't exist in the tool set and are not deprecated aliases). // This ensures invalid tool configurations fail fast at build time. func (b *Builder) Build() (*Inventory, error) { + // Apply dynamic mode preprocessing before processing toolsets + b.applyDynamicModePreprocessing() + r := &Inventory{ tools: b.tools, resourceTemplates: b.resourceTemplates, diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index bb3337af0..5be33c569 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -1832,3 +1832,153 @@ func TestWithTools_DeprecatedAliasAndFeatureFlag(t *testing.T) { t.Errorf("Flag ON: Expected new_tool (via alias), got %s", availableOn[0].Tool.Name) } } + +// Tests for WithDynamicMode +func TestWithDynamicMode_NilToolsets(t *testing.T) { + tools := []ServerTool{ + mockToolWithDefault("tool1", "default_toolset", true, true), + mockToolWithDefault("tool2", "other_toolset", true, false), + } + + // Dynamic mode with nil toolsets should start with empty set + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithDynamicMode(true)) + + available := reg.AvailableTools(context.Background()) + if len(available) != 0 { + t.Errorf("Dynamic mode with nil toolsets should start empty, got %d tools", len(available)) + } +} + +func TestWithDynamicMode_RemovesAllKeyword(t *testing.T) { + tools := []ServerTool{ + mockTool("tool1", "toolset1", true), + mockTool("tool2", "toolset2", true), + } + + // Dynamic mode should remove "all" from toolsets + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithToolsets([]string{"all", "toolset1"}). + WithDynamicMode(true)) + + available := reg.AvailableTools(context.Background()) + if len(available) != 1 { + t.Errorf("Dynamic mode should have removed 'all', expected 1 tool, got %d", len(available)) + } + if available[0].Tool.Name != "tool1" { + t.Errorf("Expected tool1, got %s", available[0].Tool.Name) + } +} + +func TestWithDynamicMode_RemovesDefaultKeyword(t *testing.T) { + tools := []ServerTool{ + mockToolWithDefault("tool1", "default_toolset", true, true), + mockTool("tool2", "explicit_toolset", true), + } + + // Dynamic mode should remove "default" from toolsets + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithToolsets([]string{"default", "explicit_toolset"}). + WithDynamicMode(true)) + + available := reg.AvailableTools(context.Background()) + if len(available) != 1 { + t.Errorf("Dynamic mode should have removed 'default', expected 1 tool, got %d", len(available)) + } + if available[0].Tool.Name != "tool2" { + t.Errorf("Expected tool2, got %s", available[0].Tool.Name) + } +} + +func TestWithDynamicMode_ExplicitToolsetsPreserved(t *testing.T) { + tools := []ServerTool{ + mockTool("tool1", "toolset1", true), + mockTool("tool2", "toolset2", true), + mockTool("tool3", "toolset3", true), + } + + // Dynamic mode should preserve explicit toolsets (not "all" or "default") + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithToolsets([]string{"toolset1", "toolset3"}). + WithDynamicMode(true)) + + available := reg.AvailableTools(context.Background()) + if len(available) != 2 { + t.Fatalf("Expected 2 tools, got %d", len(available)) + } + + toolNames := make(map[string]bool) + for _, tool := range available { + toolNames[tool.Tool.Name] = true + } + if !toolNames["tool1"] || !toolNames["tool3"] { + t.Errorf("Expected tool1 and tool3, got %v", toolNames) + } +} + +func TestWithDynamicMode_False(t *testing.T) { + tools := []ServerTool{ + mockToolWithDefault("tool1", "default_toolset", true, true), + mockTool("tool2", "other_toolset", true), + } + + // Dynamic mode false should use default behavior (nil -> use defaults) + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithDynamicMode(false)) + + available := reg.AvailableTools(context.Background()) + // Should use defaults, which includes default_toolset + if len(available) != 1 { + t.Errorf("Expected 1 tool from default toolset, got %d", len(available)) + } + if available[0].Tool.Name != "tool1" { + t.Errorf("Expected tool1 from default toolset, got %s", available[0].Tool.Name) + } +} + +func TestWithDynamicMode_WithAdditionalTools(t *testing.T) { + tools := []ServerTool{ + mockTool("tool1", "toolset1", true), + mockTool("tool2", "toolset2", true), + } + + // Dynamic mode with additional tools should allow those tools + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithDynamicMode(true). + WithTools([]string{"tool2"})) + + available := reg.AvailableTools(context.Background()) + if len(available) != 1 { + t.Fatalf("Expected 1 additional tool, got %d", len(available)) + } + if available[0].Tool.Name != "tool2" { + t.Errorf("Expected tool2, got %s", available[0].Tool.Name) + } +} + +func TestWithTools_NilToolsets_UsesEmptyToolsets(t *testing.T) { + tools := []ServerTool{ + mockToolWithDefault("tool1", "default_toolset", true, true), + mockTool("tool2", "other_toolset", true), + } + + // When specific tools are requested but no toolsets, should not use defaults + reg := mustBuild(t, NewBuilder(). + SetTools(tools). + WithTools([]string{"tool2"})) + + available := reg.AvailableTools(context.Background()) + // Only the explicitly requested tool, not the default toolset + if len(available) != 1 { + t.Errorf("Expected 1 tool (only explicitly requested), got %d", len(available)) + } + if available[0].Tool.Name != "tool2" { + t.Errorf("Expected tool2, got %s", available[0].Tool.Name) + } +} From 3db9ddb5c2a417961d78537bb2a243ab2ba464c5 Mon Sep 17 00:00:00 2001 From: tommaso-moro Date: Sun, 25 Jan 2026 00:08:48 +0000 Subject: [PATCH 2/2] remove comment --- internal/ghmcp/server_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/internal/ghmcp/server_test.go b/internal/ghmcp/server_test.go index 8e5ee1735..7ee56665e 100644 --- a/internal/ghmcp/server_test.go +++ b/internal/ghmcp/server_test.go @@ -40,13 +40,3 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { // The actual middleware functionality and tool execution with ContextWithDeps // is already tested in pkg/github/*_test.go. } - -// Note: TestResolveEnabledToolsets was removed because the resolveEnabledToolsets -// function was moved to the Inventory Builder (pkg/inventory/builder.go). -// The same functionality is now tested in pkg/inventory/registry_test.go via: -// - TestWithDynamicMode_NilToolsets -// - TestWithDynamicMode_RemovesAllKeyword -// - TestWithDynamicMode_RemovesDefaultKeyword -// - TestWithDynamicMode_ExplicitToolsetsPreserved -// - TestWithDynamicMode_WithAdditionalTools -// - TestWithTools_NilToolsets_UsesEmptyToolsets