diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index f4e67f264..ec2253157 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -176,8 +176,8 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) { // Register specific tools if configured if len(cfg.EnabledTools) > 0 { - // Clean and validate tool names enabledTools := github.CleanTools(cfg.EnabledTools) + enabledTools, _ = tsg.ResolveToolAliases(enabledTools) // Register the specified tools (additive to any toolsets already enabled) err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly) diff --git a/pkg/github/deprecated_tool_aliases.go b/pkg/github/deprecated_tool_aliases.go new file mode 100644 index 000000000..4abdca14d --- /dev/null +++ b/pkg/github/deprecated_tool_aliases.go @@ -0,0 +1,14 @@ +// deprecated_tool_aliases.go +package github + +// DeprecatedToolAliases maps old tool names to their new canonical names. +// When tools are renamed, add an entry here to maintain backward compatibility. +// Users referencing the old name will receive the new tool with a deprecation warning. +// +// Example: +// +// "get_issue": "issue_read", +// "create_pr": "pull_request_create", +var DeprecatedToolAliases = map[string]string{ + // Add entries as tools are renamed +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index d37af98b8..f21a9ae5b 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -378,6 +378,8 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG tsg.AddToolset(stargazers) tsg.AddToolset(labels) + tsg.AddDeprecatedToolAliases(DeprecatedToolAliases) + return tsg } diff --git a/pkg/toolsets/toolsets.go b/pkg/toolsets/toolsets.go index d4964ee5f..d96b5fb50 100644 --- a/pkg/toolsets/toolsets.go +++ b/pkg/toolsets/toolsets.go @@ -192,16 +192,24 @@ func (t *Toolset) AddReadTools(tools ...ServerTool) *Toolset { } type ToolsetGroup struct { - Toolsets map[string]*Toolset - everythingOn bool - readOnly bool + Toolsets map[string]*Toolset + deprecatedAliases map[string]string + everythingOn bool + readOnly bool } func NewToolsetGroup(readOnly bool) *ToolsetGroup { return &ToolsetGroup{ - Toolsets: make(map[string]*Toolset), - everythingOn: false, - readOnly: readOnly, + Toolsets: make(map[string]*Toolset), + deprecatedAliases: make(map[string]string), + everythingOn: false, + readOnly: readOnly, + } +} + +func (tg *ToolsetGroup) AddDeprecatedToolAliases(aliases map[string]string) { + for oldName, newName := range aliases { + tg.deprecatedAliases[oldName] = newName } } @@ -307,6 +315,26 @@ func NewToolDoesNotExistError(name string) *ToolDoesNotExistError { return &ToolDoesNotExistError{Name: name} } +// ResolveToolAliases resolves deprecated tool aliases to their canonical names. +// It logs a warning to stderr for each deprecated alias that is resolved. +// Returns: +// - resolved: tool names with aliases replaced by canonical names +// - aliasesUsed: map of oldName → newName for each alias that was resolved +func (tg *ToolsetGroup) ResolveToolAliases(toolNames []string) (resolved []string, aliasesUsed map[string]string) { + resolved = make([]string, 0, len(toolNames)) + aliasesUsed = make(map[string]string) + for _, toolName := range toolNames { + if canonicalName, isAlias := tg.deprecatedAliases[toolName]; isAlias { + fmt.Fprintf(os.Stderr, "Warning: tool %q is deprecated, use %q instead\n", toolName, canonicalName) + aliasesUsed[toolName] = canonicalName + resolved = append(resolved, canonicalName) + } else { + resolved = append(resolved, toolName) + } + } + return resolved, aliasesUsed +} + // FindToolByName searches all toolsets (enabled or disabled) for a tool by name. // Returns the tool, its parent toolset name, and an error if not found. func (tg *ToolsetGroup) FindToolByName(toolName string) (*ServerTool, string, error) { diff --git a/pkg/toolsets/toolsets_test.go b/pkg/toolsets/toolsets_test.go index 3f4581f34..6362aad0e 100644 --- a/pkg/toolsets/toolsets_test.go +++ b/pkg/toolsets/toolsets_test.go @@ -3,8 +3,23 @@ package toolsets import ( "errors" "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" ) +// mockTool creates a minimal ServerTool for testing +func mockTool(name string, readOnly bool) ServerTool { + return ServerTool{ + Tool: mcp.Tool{ + Name: name, + Annotations: &mcp.ToolAnnotations{ + ReadOnlyHint: readOnly, + }, + }, + RegisterFunc: func(_ *mcp.Server) {}, + } +} + func TestNewToolsetGroupIsEmptyWithoutEverythingOn(t *testing.T) { tsg := NewToolsetGroup(false) if len(tsg.Toolsets) != 0 { @@ -262,3 +277,119 @@ func TestToolsetGroup_GetToolset(t *testing.T) { t.Errorf("expected error to be ToolsetDoesNotExistError, got %v", err) } } + +func TestAddDeprecatedToolAliases(t *testing.T) { + tsg := NewToolsetGroup(false) + + // Test adding aliases + tsg.AddDeprecatedToolAliases(map[string]string{ + "old_name": "new_name", + "get_issue": "issue_read", + "create_pr": "pull_request_create", + }) + + if len(tsg.deprecatedAliases) != 3 { + t.Errorf("expected 3 aliases, got %d", len(tsg.deprecatedAliases)) + } + if tsg.deprecatedAliases["old_name"] != "new_name" { + t.Errorf("expected alias 'old_name' -> 'new_name', got '%s'", tsg.deprecatedAliases["old_name"]) + } + if tsg.deprecatedAliases["get_issue"] != "issue_read" { + t.Errorf("expected alias 'get_issue' -> 'issue_read'") + } + if tsg.deprecatedAliases["create_pr"] != "pull_request_create" { + t.Errorf("expected alias 'create_pr' -> 'pull_request_create'") + } +} + +func TestResolveToolAliases(t *testing.T) { + tsg := NewToolsetGroup(false) + tsg.AddDeprecatedToolAliases(map[string]string{ + "get_issue": "issue_read", + "create_pr": "pull_request_create", + }) + + // Test resolving a mix of aliases and canonical names + input := []string{"get_issue", "some_tool", "create_pr"} + resolved, aliasesUsed := tsg.ResolveToolAliases(input) + + // Verify resolved names + if len(resolved) != 3 { + t.Fatalf("expected 3 resolved names, got %d", len(resolved)) + } + if resolved[0] != "issue_read" { + t.Errorf("expected 'issue_read', got '%s'", resolved[0]) + } + if resolved[1] != "some_tool" { + t.Errorf("expected 'some_tool' (unchanged), got '%s'", resolved[1]) + } + if resolved[2] != "pull_request_create" { + t.Errorf("expected 'pull_request_create', got '%s'", resolved[2]) + } + + // Verify aliasesUsed map + if len(aliasesUsed) != 2 { + t.Fatalf("expected 2 aliases used, got %d", len(aliasesUsed)) + } + if aliasesUsed["get_issue"] != "issue_read" { + t.Errorf("expected aliasesUsed['get_issue'] = 'issue_read', got '%s'", aliasesUsed["get_issue"]) + } + if aliasesUsed["create_pr"] != "pull_request_create" { + t.Errorf("expected aliasesUsed['create_pr'] = 'pull_request_create', got '%s'", aliasesUsed["create_pr"]) + } +} + +func TestFindToolByName(t *testing.T) { + tsg := NewToolsetGroup(false) + + // Create a toolset with a tool + toolset := NewToolset("test-toolset", "Test toolset") + toolset.readTools = append(toolset.readTools, mockTool("issue_read", true)) + tsg.AddToolset(toolset) + + // Find by canonical name + tool, toolsetName, err := tsg.FindToolByName("issue_read") + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if tool.Tool.Name != "issue_read" { + t.Errorf("expected tool name 'issue_read', got '%s'", tool.Tool.Name) + } + if toolsetName != "test-toolset" { + t.Errorf("expected toolset name 'test-toolset', got '%s'", toolsetName) + } + + // FindToolByName does NOT resolve aliases - it expects canonical names + _, _, err = tsg.FindToolByName("get_issue") + if err == nil { + t.Error("expected error when using alias directly with FindToolByName") + } +} + +func TestRegisterSpecificTools(t *testing.T) { + tsg := NewToolsetGroup(false) + + // Create a toolset with both read and write tools + toolset := NewToolset("test-toolset", "Test toolset") + toolset.readTools = append(toolset.readTools, mockTool("issue_read", true)) + toolset.writeTools = append(toolset.writeTools, mockTool("issue_write", false)) + tsg.AddToolset(toolset) + + // Test registering with canonical names + err := tsg.RegisterSpecificTools(nil, []string{"issue_read"}, false) + if err != nil { + t.Errorf("expected no error registering tool, got %v", err) + } + + // Test registering write tool in read-only mode (should skip but not error) + err = tsg.RegisterSpecificTools(nil, []string{"issue_write"}, true) + if err != nil { + t.Errorf("expected no error when skipping write tool in read-only mode, got %v", err) + } + + // Test registering non-existent tool (should error) + err = tsg.RegisterSpecificTools(nil, []string{"nonexistent"}, false) + if err == nil { + t.Error("expected error for non-existent tool") + } +}