Bug fix: invalid tool should error out

internal/ghmcp/server.go

@@ -221,7 +221,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
 		WithDeprecatedAliases(github.DeprecatedToolAliases).
 		WithReadOnly(cfg.ReadOnly).
 		WithToolsets(enabledToolsets).
-		WithTools(github.CleanTools(cfg.EnabledTools)).
+		WithTools(cfg.EnabledTools).
 		WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
 
 	// Apply token scope filtering if scopes are known (for PAT filtering)
@@ -235,6 +235,11 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
 		fmt.Fprintf(os.Stderr, "Warning: unrecognized toolsets ignored: %s\n", strings.Join(unrecognized, ", "))
 	}
 
+	// Check for unrecognized tools and error out (unlike toolsets which just warn)
+	if unrecognized := inventory.UnrecognizedTools(); len(unrecognized) > 0 {
+		return nil, fmt.Errorf("unrecognized tools: %s", strings.Join(unrecognized, ", "))
+	}
+
 	// Register GitHub tools/resources/prompts from the inventory.
 	// In dynamic mode with no explicit toolsets, this is a no-op since enabledToolsets
 	// is empty - users enable toolsets at runtime via the dynamic tools below (but can

pkg/inventory/builder.go

@@ -101,6 +101,7 @@ func (b *Builder) WithToolsets(toolsetIDs []string) *Builder {
 // WithTools specifies additional tools that bypass toolset filtering.
 // These tools are additive - they will be included even if their toolset is not enabled.
 // Read-only filtering still applies to these tools.
+// Input is cleaned (trimmed, deduplicated) during Build().
 // Deprecated tool aliases are automatically resolved to their canonical names during Build().
 // Returns self for chaining.
 func (b *Builder) WithTools(toolNames []string) *Builder {
@@ -127,6 +128,24 @@ func (b *Builder) WithFilter(filter ToolFilter) *Builder {
 	return b
 }
 
+// cleanTools trims whitespace and removes duplicates from tool names.
+// Empty strings after trimming are excluded.
+func cleanTools(tools []string) []string {
+	seen := make(map[string]bool)
+	var cleaned []string
+	for _, name := range tools {
+		trimmed := strings.TrimSpace(name)
+		if trimmed == "" {
+			continue
+		}
+		if !seen[trimmed] {
+			seen[trimmed] = true
+			cleaned = append(cleaned, trimmed)
+		}
+	}
+	return cleaned
+}
+
 // Build creates the final Inventory with all configuration applied.
 // This processes toolset filtering, tool name resolution, and sets up
 // the inventory for use. The returned Inventory is ready for use with
@@ -145,10 +164,19 @@ func (b *Builder) Build() *Inventory {
 	// Process toolsets and pre-compute metadata in a single pass
 	r.enabledToolsets, r.unrecognizedToolsets, r.toolsetIDs, r.toolsetIDSet, r.defaultToolsetIDs, r.toolsetDescriptions = b.processToolsets()
 
-	// Process additional tools (resolve aliases)
+	// Build set of valid tool names for validation
+	validToolNames := make(map[string]bool, len(b.tools))
+	for i := range b.tools {
+		validToolNames[b.tools[i].Tool.Name] = true
+	}
+
+	// Process additional tools (clean, resolve aliases, and track unrecognized)
 	if len(b.additionalTools) > 0 {
-		r.additionalTools = make(map[string]bool, len(b.additionalTools))
-		for _, name := range b.additionalTools {
+		cleanedTools := cleanTools(b.additionalTools)
+
+		r.additionalTools = make(map[string]bool, len(cleanedTools))
+		var unrecognizedTools []string
+		for _, name := range cleanedTools {
 			// Always include the original name - this handles the case where
 			// the tool exists but is controlled by a feature flag that's OFF.
 			r.additionalTools[name] = true
@@ -157,8 +185,12 @@ func (b *Builder) Build() *Inventory {
 			// the new consolidated tool is available.
 			if canonical, isAlias := b.deprecatedAliases[name]; isAlias {
 				r.additionalTools[canonical] = true
+			} else if !validToolNames[name] {
+				// Not a valid tool and not a deprecated alias - track as unrecognized
+				unrecognizedTools = append(unrecognizedTools, name)
 			}
 		}
+		r.unrecognizedTools = unrecognizedTools
 	}
 
 	return r

pkg/inventory/registry.go

@@ -58,6 +58,8 @@ type Inventory struct {
 	filters []ToolFilter
 	// unrecognizedToolsets holds toolset IDs that were requested but don't match any registered toolsets
 	unrecognizedToolsets []string
+	// unrecognizedTools holds tool names that were requested via WithTools but don't exist
+	unrecognizedTools []string
 }
 
 // UnrecognizedToolsets returns toolset IDs that were passed to WithToolsets but don't
@@ -66,6 +68,13 @@ func (r *Inventory) UnrecognizedToolsets() []string {
 	return r.unrecognizedToolsets
 }
 
+// UnrecognizedTools returns tool names that were passed to WithTools but don't
+// match any registered tools (and are not deprecated aliases). This is used to
+// error out when invalid tool names are specified.
+func (r *Inventory) UnrecognizedTools() []string {
+	return r.unrecognizedTools
+}
+
 // MCP method constants for use with ForMCPRequest.
 const (
 	MCPMethodInitialize             = "initialize"
@@ -112,6 +121,7 @@ func (r *Inventory) ForMCPRequest(method string, itemName string) *Inventory {
 		featureChecker:       r.featureChecker,
 		filters:              r.filters, // shared, not modified
 		unrecognizedToolsets: r.unrecognizedToolsets,
+		unrecognizedTools:    r.unrecognizedTools,
 	}
 
 	// Helper to clear all item types

pkg/inventory/registry_test.go

@@ -270,6 +270,107 @@ func TestUnrecognizedToolsets(t *testing.T) {
 	}
 }
 
+func TestUnrecognizedTools(t *testing.T) {
+	tools := []ServerTool{
+		mockTool("tool1", "toolset1", true),
+		mockTool("tool2", "toolset2", true),
+	}
+
+	deprecatedAliases := map[string]string{
+		"old_tool": "tool1",
+	}
+
+	tests := []struct {
+		name                 string
+		withTools            []string
+		expectedUnrecognized []string
+	}{
+		{
+			name:                 "all valid",
+			withTools:            []string{"tool1", "tool2"},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "one invalid",
+			withTools:            []string{"tool1", "blabla"},
+			expectedUnrecognized: []string{"blabla"},
+		},
+		{
+			name:                 "multiple invalid",
+			withTools:            []string{"invalid1", "tool1", "invalid2"},
+			expectedUnrecognized: []string{"invalid1", "invalid2"},
+		},
+		{
+			name:                 "deprecated alias is valid",
+			withTools:            []string{"old_tool"},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "mixed valid and deprecated alias",
+			withTools:            []string{"old_tool", "tool2"},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "empty input",
+			withTools:            []string{},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "whitespace trimmed from valid tool",
+			withTools:            []string{" tool1 ", "  tool2  "},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "whitespace trimmed from invalid tool",
+			withTools:            []string{" invalid_tool "},
+			expectedUnrecognized: []string{"invalid_tool"},
+		},
+		{
+			name:                 "duplicate tools deduplicated",
+			withTools:            []string{"tool1", "tool1"},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "duplicate invalid tools deduplicated",
+			withTools:            []string{"blabla", "blabla"},
+			expectedUnrecognized: []string{"blabla"},
+		},
+		{
+			name:                 "mixed whitespace and duplicates",
+			withTools:            []string{" tool1 ", "tool1", "  tool1  "},
+			expectedUnrecognized: nil,
+		},
+		{
+			name:                 "empty strings ignored",
+			withTools:            []string{"", "tool1", "  ", ""},
+			expectedUnrecognized: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			inv := NewBuilder().
+				SetTools(tools).
+				WithDeprecatedAliases(deprecatedAliases).
+				WithToolsets([]string{"all"}).
+				WithTools(tt.withTools).
+				Build()
+			unrecognized := inv.UnrecognizedTools()
+
+			if len(unrecognized) != len(tt.expectedUnrecognized) {
+				t.Fatalf("Expected %d unrecognized, got %d: %v",
+					len(tt.expectedUnrecognized), len(unrecognized), unrecognized)
+			}
+
+			for i, expected := range tt.expectedUnrecognized {
+				if unrecognized[i] != expected {
+					t.Errorf("Expected unrecognized[%d] = %q, got %q", i, expected, unrecognized[i])
+				}
+			}
+		})
+	}
+}
+
 func TestWithTools(t *testing.T) {
 	tools := []ServerTool{
 		mockTool("tool1", "toolset1", true),