move resolveEnabledToolsets to builder

Author: tommaso-moro

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,17 +142,16 @@ 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)
 
 	// Build and register the tool/resource/prompt inventory
 	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()
 

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

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,

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)
+	}
+}