Merge branch 'main' into tonytrg/review-threads

Author: tonytrg

README.md

@@ -726,12 +726,12 @@ The following sets of tools are available:
 - **issue_read** - Get issue details
   - `issue_number`: The number of the issue (number, required)
   - `method`: The read operation to perform on a single issue.
-Options are:
-1. get - Get details of a specific issue.
-2. get_comments - Get issue comments.
-3. get_sub_issues - Get sub-issues of the issue.
-4. get_labels - Get labels assigned to the issue.
- (string, required)
+    Options are:
+    1. get - Get details of a specific issue.
+    2. get_comments - Get issue comments.
+    3. get_sub_issues - Get sub-issues of the issue.
+    4. get_labels - Get labels assigned to the issue.
+     (string, required)
   - `owner`: The owner of the repository (string, required)
   - `page`: Page number for pagination (min 1) (number, optional)
   - `perPage`: Results per page for pagination (min 1, max 100) (number, optional)
@@ -744,10 +744,10 @@ Options are:
   - `issue_number`: Issue number to update (number, optional)
   - `labels`: Labels to apply to this issue (string[], optional)
   - `method`: Write operation to perform on a single issue.
-Options are:
-- 'create' - creates a new issue.
-- 'update' - updates an existing issue.
- (string, required)
+    Options are:
+    - 'create' - creates a new issue.
+    - 'update' - updates an existing issue.
+     (string, required)
   - `milestone`: Milestone number (number, optional)
   - `owner`: Repository owner (string, required)
   - `repo`: Repository name (string, required)
@@ -784,11 +784,11 @@ Options are:
   - `before_id`: The ID of the sub-issue to be prioritized before (either after_id OR before_id should be specified) (number, optional)
   - `issue_number`: The number of the parent issue (number, required)
   - `method`: The action to perform on a single sub-issue
-Options are:
-- 'add' - add a sub-issue to a parent issue in a GitHub repository.
-- 'remove' - remove a sub-issue from a parent issue in a GitHub repository.
-- 'reprioritize' - change the order of sub-issues within a parent issue in a GitHub repository. Use either 'after_id' or 'before_id' to specify the new position.
-				 (string, required)
+    Options are:
+    - 'add' - add a sub-issue to a parent issue in a GitHub repository.
+    - 'remove' - remove a sub-issue from a parent issue in a GitHub repository.
+    - 'reprioritize' - change the order of sub-issues within a parent issue in a GitHub repository. Use either 'after_id' or 'before_id' to specify the new position.
+    				 (string, required)
   - `owner`: Repository owner (string, required)
   - `replace_parent`: When true, replaces the sub-issue's current parent issue. Use with 'add' method only. (boolean, optional)
   - `repo`: Repository name (string, required)

cmd/github-mcp-server/generate_docs.go

@@ -269,6 +269,9 @@ func generateToolDoc(tool mcp.Tool) string {
 
 			description = prop.Description
 
+			// Indent any continuation lines in the description to maintain markdown formatting
+			description = indentMultilineDescription(description, "    ")
+
 			paramLine := fmt.Sprintf("  - `%s`: %s (%s, %s)", propName, description, typeStr, requiredStr)
 			lines = append(lines, paramLine)
 		}
@@ -288,6 +291,19 @@ func contains(slice []string, item string) bool {
 	return false
 }
 
+// indentMultilineDescription adds the specified indent to all lines after the first line.
+// This ensures that multi-line descriptions maintain proper markdown list formatting.
+func indentMultilineDescription(description, indent string) string {
+	lines := strings.Split(description, "\n")
+	if len(lines) <= 1 {
+		return description
+	}
+	for i := 1; i < len(lines); i++ {
+		lines[i] = indent + lines[i]
+	}
+	return strings.Join(lines, "\n")
+}
+
 func replaceSection(content, startMarker, endMarker, newContent string) string {
 	startPattern := fmt.Sprintf(`<!-- %s -->`, regexp.QuoteMeta(startMarker))
 	endPattern := fmt.Sprintf(`<!-- %s -->`, regexp.QuoteMeta(endMarker))

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)

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

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
 }
 

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) {

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