diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 165886606..b1bad9fa4 100644 --- a/internal/ghmcp/server.go +++ b/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 diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index 0400c2a24..9d135218e 100644 --- a/pkg/inventory/builder.go +++ b/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 diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index f3691e38a..7313d58b3 100644 --- a/pkg/inventory/registry.go +++ b/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 diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index 2c3262873..2bb0c7d65 100644 --- a/pkg/inventory/registry_test.go +++ b/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),