diff --git a/pkg/cli/codemod_mcp_mode_to_type.go b/pkg/cli/codemod_mcp_mode_to_type.go new file mode 100644 index 0000000000..82c7878c40 --- /dev/null +++ b/pkg/cli/codemod_mcp_mode_to_type.go @@ -0,0 +1,107 @@ +package cli + +import ( + "strings" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var mcpModeToTypeCodemodLog = logger.New("cli:codemod_mcp_mode_to_type") + +// getMCPModeToTypeCodemod creates a codemod for migrating 'mode' to 'type' in custom MCP server configurations +func getMCPModeToTypeCodemod() Codemod { + return Codemod{ + ID: "mcp-mode-to-type-migration", + Name: "Migrate MCP 'mode' to 'type'", + Description: "Renames 'mode' field to 'type' in custom MCP server configurations (mcp-servers section). Does not affect GitHub or Serena tool configurations.", + IntroducedIn: "0.7.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if mcp-servers section exists + mcpServersValue, hasMCPServers := frontmatter["mcp-servers"] + if !hasMCPServers { + return content, false, nil + } + + mcpServersMap, ok := mcpServersValue.(map[string]any) + if !ok { + return content, false, nil + } + + // Check if any MCP server has a 'mode' field + hasMode := false + for _, serverValue := range mcpServersMap { + serverConfig, ok := serverValue.(map[string]any) + if !ok { + continue + } + if _, hasModeField := serverConfig["mode"]; hasModeField { + hasMode = true + break + } + } + + if !hasMode { + return content, false, nil + } + + // Parse frontmatter to get raw lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Rename 'mode' to 'type' in all MCP servers + result, modified := renameModeToTypeInMCPServers(frontmatterLines) + if !modified { + return content, false, nil + } + + // Reconstruct the content + newContent := reconstructContent(result, markdown) + mcpModeToTypeCodemodLog.Print("Applied MCP mode-to-type migration") + return newContent, true, nil + }, + } +} + +// renameModeToTypeInMCPServers renames 'mode:' to 'type:' within mcp-servers blocks +func renameModeToTypeInMCPServers(lines []string) ([]string, bool) { + var result []string + var modified bool + var inMCPServers bool + var mcpServersIndent string + + for i, line := range lines { + trimmedLine := strings.TrimSpace(line) + + // Track if we're in mcp-servers block + if strings.HasPrefix(trimmedLine, "mcp-servers:") { + inMCPServers = true + mcpServersIndent = getIndentation(line) + result = append(result, line) + continue + } + + // Check if we've left mcp-servers block + if inMCPServers && len(trimmedLine) > 0 && !strings.HasPrefix(trimmedLine, "#") { + if hasExitedBlock(line, mcpServersIndent) { + inMCPServers = false + } + } + + // Rename 'mode:' to 'type:' if we're in mcp-servers block + if inMCPServers && strings.HasPrefix(trimmedLine, "mode:") { + newLine, replaced := findAndReplaceInLine(line, "mode", "type") + if replaced { + result = append(result, newLine) + modified = true + mcpModeToTypeCodemodLog.Printf("Renamed 'mode' to 'type' on line %d", i+1) + continue + } + } + + result = append(result, line) + } + + return result, modified +} diff --git a/pkg/cli/codemod_mcp_mode_to_type_test.go b/pkg/cli/codemod_mcp_mode_to_type_test.go new file mode 100644 index 0000000000..51afdfefcf --- /dev/null +++ b/pkg/cli/codemod_mcp_mode_to_type_test.go @@ -0,0 +1,209 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMCPModeToTypeCodemod(t *testing.T) { + codemod := getMCPModeToTypeCodemod() + + t.Run("renames mode to type in custom MCP servers", func(t *testing.T) { + content := `--- +engine: copilot +tools: + github: null +mcp-servers: + my-custom-server: + mode: stdio + command: npx + args: ["-y", "@my/server"] +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "tools": map[string]any{ + "github": nil, + }, + "mcp-servers": map[string]any{ + "my-custom-server": map[string]any{ + "mode": "stdio", + "command": "npx", + "args": []any{"-y", "@my/server"}, + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error when applying codemod") + assert.True(t, modified, "Should modify content") + assert.Contains(t, result, "type: stdio", "Should rename mode to type") + assert.NotContains(t, result, "mode: stdio", "Should not contain old mode field") + }) + + t.Run("does not modify workflows without mcp-servers", func(t *testing.T) { + content := `--- +engine: copilot +tools: + github: + mode: remote +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "tools": map[string]any{ + "github": map[string]any{ + "mode": "remote", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.False(t, modified, "Should not modify content without mcp-servers") + assert.Equal(t, content, result, "Content should remain unchanged") + }) + + t.Run("does not modify GitHub tool mode field", func(t *testing.T) { + content := `--- +engine: copilot +tools: + github: + mode: remote +mcp-servers: + my-server: + mode: stdio + command: node +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "tools": map[string]any{ + "github": map[string]any{ + "mode": "remote", + }, + }, + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "mode": "stdio", + "command": "node", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.True(t, modified, "Should modify mcp-servers") + assert.Contains(t, result, "mode: remote", "Should keep GitHub tool mode field") + assert.Contains(t, result, "type: stdio", "Should rename mode in mcp-servers to type") + assert.NotContains(t, result, "my-server:\n mode: stdio", "Should not contain mode in mcp-servers") + }) + + t.Run("handles multiple MCP servers with mode", func(t *testing.T) { + content := `--- +engine: copilot +mcp-servers: + server1: + mode: stdio + command: npm + server2: + mode: http + url: http://localhost:8080 +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "mcp-servers": map[string]any{ + "server1": map[string]any{ + "mode": "stdio", + "command": "npm", + }, + "server2": map[string]any{ + "mode": "http", + "url": "http://localhost:8080", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.True(t, modified, "Should modify content") + assert.Contains(t, result, "type: stdio", "Should rename first server mode") + assert.Contains(t, result, "type: http", "Should rename second server mode") + assert.NotContains(t, result, "mode: stdio", "Should not contain mode: stdio") + assert.NotContains(t, result, "mode: http", "Should not contain mode: http") + }) + + t.Run("does not modify when no mode field exists", func(t *testing.T) { + content := `--- +engine: copilot +mcp-servers: + my-server: + type: stdio + command: node +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "type": "stdio", + "command": "node", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.False(t, modified, "Should not modify content when no mode field") + assert.Equal(t, content, result, "Content should remain unchanged") + }) + + t.Run("preserves comments and formatting", func(t *testing.T) { + content := `--- +engine: copilot +mcp-servers: + my-server: + # MCP connection mode + mode: stdio # Use stdio transport + command: node +--- + +# Test Workflow +` + + frontmatter := map[string]any{ + "engine": "copilot", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "mode": "stdio", + "command": "node", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.True(t, modified, "Should modify content") + assert.Contains(t, result, "# MCP connection mode", "Should preserve comment") + assert.Contains(t, result, "type: stdio # Use stdio transport", "Should preserve inline comment and formatting") + assert.NotContains(t, result, "mode: stdio", "Should not contain old field") + }) +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 71218a20cd..c7e74becbc 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -32,5 +32,6 @@ func GetAllCodemods() []Codemod { getGrepToolRemovalCodemod(), getMCPNetworkMigrationCodemod(), getDiscussionFlagRemovalCodemod(), + getMCPModeToTypeCodemod(), } } diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 2c8301e0ff..ca9e7f5dbe 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -41,7 +41,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 14 + expectedCount := 15 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -116,6 +116,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "grep-tool-removal", "mcp-network-to-top-level-migration", "add-comment-discussion-removal", + "mcp-mode-to-type-migration", } require.Len(t, codemods, len(expectedOrder), "Should have expected number of codemods") diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 9aafd0d3ea..747a6b1f35 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -3425,15 +3425,10 @@ }, "description": "Environment variables" }, - "mode": { - "type": "string", - "enum": ["stdio", "http", "remote", "local"], - "description": "MCP server mode" - }, "type": { "type": "string", "enum": ["stdio", "http", "remote", "local"], - "description": "MCP server type" + "description": "MCP connection type. Use 'stdio' for command-based or container-based servers, 'http' for HTTP-based servers. 'local' is an alias for 'stdio' and is normalized during parsing." }, "version": { "type": ["string", "number"],