From e4f1ef231da75b24abf564d7e1e9552ed1f12e49 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:09:29 +0000 Subject: [PATCH 1/3] Initial plan From 8fdb3c88f2805230e991c4dae84e1989a9492937 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:18:00 +0000 Subject: [PATCH 2/3] Initial plan for MCP network configuration repair codemods Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/poem-bot.lock.yml | 35 +++++++++++++++++-- .github/workflows/sub-issue-closer.lock.yml | 35 +++++++++++++++++-- .github/workflows/workflow-generator.lock.yml | 35 +++++++++++++++++-- .../workflow-health-manager.lock.yml | 35 +++++++++++++++++-- 4 files changed, 132 insertions(+), 8 deletions(-) diff --git a/.github/workflows/poem-bot.lock.yml b/.github/workflows/poem-bot.lock.yml index eab3364ab55..b9177c59a3b 100644 --- a/.github/workflows/poem-bot.lock.yml +++ b/.github/workflows/poem-bot.lock.yml @@ -437,12 +437,19 @@ jobs: "name": "add_labels" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 2 issue(s) can be updated. Target: *.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -452,6 +459,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/sub-issue-closer.lock.yml b/.github/workflows/sub-issue-closer.lock.yml index 64eaa1fb68f..9146aa528c5 100644 --- a/.github/workflows/sub-issue-closer.lock.yml +++ b/.github/workflows/sub-issue-closer.lock.yml @@ -194,12 +194,19 @@ jobs: "name": "add_comment" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 20 issue(s) can be updated. Target: *.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 20 issue(s) can be updated. Target: *.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -209,6 +216,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/workflow-generator.lock.yml b/.github/workflows/workflow-generator.lock.yml index 70bed8c56fe..1b84af8606b 100644 --- a/.github/workflows/workflow-generator.lock.yml +++ b/.github/workflows/workflow-generator.lock.yml @@ -230,12 +230,19 @@ jobs: "name": "assign_to_agent" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 1 issue(s) can be updated. Target: ${{ github.event.issue.number }}.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 1 issue(s) can be updated. Target: ${{ github.event.issue.number }}.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -245,6 +252,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ diff --git a/.github/workflows/workflow-health-manager.lock.yml b/.github/workflows/workflow-health-manager.lock.yml index 2a2c1d809a0..21d56abaab8 100644 --- a/.github/workflows/workflow-health-manager.lock.yml +++ b/.github/workflows/workflow-health-manager.lock.yml @@ -251,12 +251,19 @@ jobs: "name": "add_comment" }, { - "description": "Update an existing GitHub issue's status, title, or body. Use this to modify issue properties after creation. Only the fields you specify will be updated; other fields remain unchanged. CONSTRAINTS: Maximum 5 issue(s) can be updated.", + "description": "Update an existing GitHub issue's status, title, labels, assignees, milestone, or body. Body updates support replacing, appending to, prepending content, or updating a per-run \"island\" section. CONSTRAINTS: Maximum 5 issue(s) can be updated.", "inputSchema": { "additionalProperties": false, "properties": { + "assignees": { + "description": "Replace the issue assignees with this list of GitHub usernames (e.g., ['octocat', 'mona']).", + "items": { + "type": "string" + }, + "type": "array" + }, "body": { - "description": "New issue body to replace the existing content. Use Markdown formatting.", + "description": "Issue body content in Markdown. For 'replace', this becomes the entire body. For 'append'/'prepend', this content is added with a separator and an attribution footer. For 'replace-island', only the run-specific section is updated.", "type": "string" }, "issue_number": { @@ -266,6 +273,30 @@ jobs: "string" ] }, + "labels": { + "description": "Replace the issue labels with this list (e.g., ['bug', 'campaign:foo']). Labels must exist in the repository.", + "items": { + "type": "string" + }, + "type": "array" + }, + "milestone": { + "description": "Milestone number to assign (e.g., 1). Use null to clear.", + "type": [ + "number", + "string" + ] + }, + "operation": { + "description": "How to update the issue body: 'append' (default - add to end with separator), 'prepend' (add to start with separator), 'replace' (overwrite entire body), or 'replace-island' (update a run-specific section).", + "enum": [ + "replace", + "append", + "prepend", + "replace-island" + ], + "type": "string" + }, "status": { "description": "New issue status: 'open' to reopen a closed issue, 'closed' to close an open issue.", "enum": [ From fac39bd93784afdb53d2f7da97f84afaf96dff9a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 21 Jan 2026 22:25:03 +0000 Subject: [PATCH 3/3] feat: add MCP network migration codemod - Add codemod to migrate MCP per-server network config to top-level - Handles single and multiple servers with network configurations - Merges duplicate domains across servers - Merges with existing top-level network configuration - Preserves all other MCP server fields - Add comprehensive test coverage with 14 test cases - Update codemod registry and test expectations Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_mcp_network.go | 433 ++++++++++++++++++++ pkg/cli/codemod_mcp_network_test.go | 604 ++++++++++++++++++++++++++++ pkg/cli/fix_codemods.go | 1 + pkg/cli/fix_codemods_test.go | 4 +- 4 files changed, 1041 insertions(+), 1 deletion(-) create mode 100644 pkg/cli/codemod_mcp_network.go create mode 100644 pkg/cli/codemod_mcp_network_test.go diff --git a/pkg/cli/codemod_mcp_network.go b/pkg/cli/codemod_mcp_network.go new file mode 100644 index 00000000000..b4281535de2 --- /dev/null +++ b/pkg/cli/codemod_mcp_network.go @@ -0,0 +1,433 @@ +package cli + +import ( + "fmt" + "strings" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var mcpNetworkCodemodLog = logger.New("cli:codemod_mcp_network") + +// getMCPNetworkMigrationCodemod creates a codemod for migrating per-server MCP network configuration to top-level network configuration +func getMCPNetworkMigrationCodemod() Codemod { + return Codemod{ + ID: "mcp-network-to-top-level-migration", + Name: "Migrate MCP network config to top-level", + Description: "Moves per-server MCP 'network.allowed' configuration to top-level workflow 'network.allowed'. Per-server network configuration is deprecated.", + IntroducedIn: "0.6.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 + } + + // Collect all network.allowed domains from MCP servers + var allAllowedDomains []string + serversWithNetwork := make(map[string]bool) + + for serverName, serverValue := range mcpServersMap { + serverConfig, ok := serverValue.(map[string]any) + if !ok { + continue + } + + // Check if this server has a network configuration + networkValue, hasNetwork := serverConfig["network"] + if !hasNetwork { + continue + } + + networkMap, ok := networkValue.(map[string]any) + if !ok { + continue + } + + // Extract allowed domains + allowedValue, hasAllowed := networkMap["allowed"] + if !hasAllowed { + continue + } + + // Convert allowed to []string + switch allowed := allowedValue.(type) { + case []any: + for _, domain := range allowed { + if domainStr, ok := domain.(string); ok { + allAllowedDomains = append(allAllowedDomains, domainStr) + } + } + // Only mark server as having network if it has domains + if len(allowed) > 0 { + serversWithNetwork[serverName] = true + } + case []string: + allAllowedDomains = append(allAllowedDomains, allowed...) + // Only mark server as having network if it has domains + if len(allowed) > 0 { + serversWithNetwork[serverName] = true + } + } + } + + // If no servers have network configuration, nothing to do + if len(serversWithNetwork) == 0 { + return content, false, nil + } + + // Remove duplicates from collected domains + allAllowedDomains = uniqueStrings(allAllowedDomains) + + // Parse frontmatter to get raw lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Remove network fields from all MCP servers + result := frontmatterLines + var modified bool + for serverName := range serversWithNetwork { + var serverModified bool + result, serverModified = removeFieldFromMCPServer(result, serverName, "network") + if serverModified { + modified = true + mcpNetworkCodemodLog.Printf("Removed network configuration from MCP server '%s'", serverName) + } + } + + if !modified { + return content, false, nil + } + + // Check if top-level network configuration already exists + existingNetworkValue, hasTopLevelNetwork := frontmatter["network"] + var existingAllowed []string + + if hasTopLevelNetwork { + if existingNetworkMap, ok := existingNetworkValue.(map[string]any); ok { + if existingAllowedValue, hasExistingAllowed := existingNetworkMap["allowed"]; hasExistingAllowed { + switch allowed := existingAllowedValue.(type) { + case []any: + for _, domain := range allowed { + if domainStr, ok := domain.(string); ok { + existingAllowed = append(existingAllowed, domainStr) + } + } + case []string: + existingAllowed = append(existingAllowed, allowed...) + } + } + } + } + + // Merge existing and new domains, remove duplicates + mergedDomains := append(existingAllowed, allAllowedDomains...) + mergedDomains = uniqueStrings(mergedDomains) + + // Add or update top-level network configuration + if hasTopLevelNetwork { + // Update existing network.allowed + result = updateNetworkAllowed(result, mergedDomains) + mcpNetworkCodemodLog.Printf("Updated top-level network.allowed with %d domains", len(mergedDomains)) + } else { + // Add new top-level network configuration + result = addTopLevelNetwork(result, mergedDomains) + mcpNetworkCodemodLog.Printf("Added top-level network.allowed with %d domains", len(mergedDomains)) + } + + // Reconstruct the content + newContent := reconstructContent(result, markdown) + mcpNetworkCodemodLog.Print("Applied MCP network migration to top-level") + return newContent, true, nil + }, + } +} + +// removeFieldFromMCPServer removes a field from a specific MCP server configuration +func removeFieldFromMCPServer(lines []string, serverName string, fieldName string) ([]string, bool) { + var result []string + var modified bool + var inMCPServers bool + var mcpServersIndent string + var inServerBlock bool + var serverIndent string + var inFieldBlock bool + var fieldIndent 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 + inServerBlock = false + } + } + + // Track if we're in the specific server block + if inMCPServers && strings.HasPrefix(trimmedLine, serverName+":") { + inServerBlock = true + serverIndent = getIndentation(line) + result = append(result, line) + continue + } + + // Check if we've left the server block + if inServerBlock && len(trimmedLine) > 0 && !strings.HasPrefix(trimmedLine, "#") { + currentIndent := getIndentation(line) + // Exit if we're back at mcp-servers level or less + if len(currentIndent) <= len(serverIndent) && strings.Contains(line, ":") { + inServerBlock = false + } + } + + // Remove field line if in server block + if inServerBlock && strings.HasPrefix(trimmedLine, fieldName+":") { + modified = true + inFieldBlock = true + fieldIndent = getIndentation(line) + mcpNetworkCodemodLog.Printf("Removed %s from mcp-server '%s' on line %d", fieldName, serverName, i+1) + continue + } + + // Skip nested properties under the field + if inFieldBlock { + // Empty lines within the field block should be removed + if len(trimmedLine) == 0 { + continue + } + + currentIndent := getIndentation(line) + + // Comments need to check indentation + if strings.HasPrefix(trimmedLine, "#") { + if len(currentIndent) > len(fieldIndent) { + // Comment is nested under field, remove it + continue + } + // Comment is at same or less indentation, exit field block and keep it + inFieldBlock = false + result = append(result, line) + continue + } + + // If this line has more indentation than field, it's a nested property + if len(currentIndent) > len(fieldIndent) { + continue + } + // We've exited the field block + inFieldBlock = false + } + + result = append(result, line) + } + + return result, modified +} + +// addTopLevelNetwork adds a new top-level network configuration +func addTopLevelNetwork(lines []string, domains []string) []string { + // Find a good place to insert (after on: field, or at the beginning) + insertIndex := 0 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "on:") { + // Insert after the on: block + insertIndex = i + 1 + // Skip any nested content under on: + if !strings.Contains(trimmed, "on: ") || strings.HasPrefix(trimmed, "on:") && len(trimmed) == 3 { + // on: is a block, find the end + onIndent := getIndentation(line) + for j := i + 1; j < len(lines); j++ { + nextLine := lines[j] + nextTrimmed := strings.TrimSpace(nextLine) + if len(nextTrimmed) == 0 { + continue + } + if hasExitedBlock(nextLine, onIndent) { + insertIndex = j + break + } + } + } + break + } + } + + // Build network configuration lines + var networkLines []string + networkLines = append(networkLines, "network:") + networkLines = append(networkLines, " allowed:") + for _, domain := range domains { + networkLines = append(networkLines, fmt.Sprintf(" - %s", domain)) + } + + // Insert at the determined position + result := make([]string, 0, len(lines)+len(networkLines)) + result = append(result, lines[:insertIndex]...) + result = append(result, networkLines...) + result = append(result, lines[insertIndex:]...) + + return result +} + +// updateNetworkAllowed updates the existing top-level network.allowed configuration +func updateNetworkAllowed(lines []string, domains []string) []string { + var result []string + var inNetworkBlock bool + var networkIndent string + var inAllowedBlock bool + var allowedIndent string + var replacedAllowed bool + + for _, line := range lines { + trimmedLine := strings.TrimSpace(line) + + // Track if we're in network block + if strings.HasPrefix(trimmedLine, "network:") { + inNetworkBlock = true + networkIndent = getIndentation(line) + result = append(result, line) + continue + } + + // Check if we've left network block + if inNetworkBlock && len(trimmedLine) > 0 && !strings.HasPrefix(trimmedLine, "#") { + if hasExitedBlock(line, networkIndent) { + inNetworkBlock = false + inAllowedBlock = false + } + } + + // Track if we're in allowed block within network + if inNetworkBlock && strings.HasPrefix(trimmedLine, "allowed:") { + inAllowedBlock = true + allowedIndent = getIndentation(line) + replacedAllowed = true + // Replace the allowed block + result = append(result, line) + for _, domain := range domains { + result = append(result, fmt.Sprintf("%s - %s", allowedIndent, domain)) + } + continue + } + + // Skip existing allowed array items + if inAllowedBlock { + currentIndent := getIndentation(line) + + // Empty lines - skip + if len(trimmedLine) == 0 { + continue + } + + // Comments at deeper indentation - skip + if strings.HasPrefix(trimmedLine, "#") && len(currentIndent) > len(allowedIndent) { + continue + } + + // Array items (lines starting with -) + if strings.HasPrefix(trimmedLine, "-") && len(currentIndent) > len(allowedIndent) { + continue + } + + // We've exited the allowed block + inAllowedBlock = false + } + + result = append(result, line) + } + + // If we didn't find an allowed block, add it to the network block + if !replacedAllowed { + // Find the end of the network block and insert allowed + result = addAllowedToNetwork(result, domains) + } + + return result +} + +// addAllowedToNetwork adds an allowed field to an existing network block +func addAllowedToNetwork(lines []string, domains []string) []string { + var result []string + var inNetworkBlock bool + var networkIndent string + var insertIndex = -1 + + for i, line := range lines { + trimmedLine := strings.TrimSpace(line) + + if strings.HasPrefix(trimmedLine, "network:") { + inNetworkBlock = true + networkIndent = getIndentation(line) + } + + if inNetworkBlock && len(trimmedLine) > 0 && !strings.HasPrefix(trimmedLine, "#") { + if hasExitedBlock(line, networkIndent) { + // Found the end of network block + insertIndex = i + break + } + } + + result = append(result, line) + } + + if insertIndex > 0 { + // Insert allowed before the next top-level block + allowedLines := []string{ + fmt.Sprintf("%s allowed:", networkIndent), + } + for _, domain := range domains { + allowedLines = append(allowedLines, fmt.Sprintf("%s - %s", networkIndent, domain)) + } + + result = append(result, allowedLines...) + result = append(result, lines[insertIndex:]...) + } else { + // Append at the end of network block + networkIndentStr := "" + for i := len(result) - 1; i >= 0; i-- { + trimmed := strings.TrimSpace(result[i]) + if strings.HasPrefix(trimmed, "network:") { + networkIndentStr = getIndentation(result[i]) + break + } + } + result = append(result, fmt.Sprintf("%s allowed:", networkIndentStr)) + for _, domain := range domains { + result = append(result, fmt.Sprintf("%s - %s", networkIndentStr, domain)) + } + } + + return result +} + +// uniqueStrings removes duplicates from a string slice while preserving order +func uniqueStrings(input []string) []string { + seen := make(map[string]bool) + var result []string + for _, item := range input { + if !seen[item] { + seen[item] = true + result = append(result, item) + } + } + return result +} diff --git a/pkg/cli/codemod_mcp_network_test.go b/pkg/cli/codemod_mcp_network_test.go new file mode 100644 index 00000000000..52a72a3bd8a --- /dev/null +++ b/pkg/cli/codemod_mcp_network_test.go @@ -0,0 +1,604 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetMCPNetworkMigrationCodemod(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + // Verify codemod metadata + assert.Equal(t, "mcp-network-to-top-level-migration", codemod.ID) + assert.Equal(t, "Migrate MCP network config to top-level", codemod.Name) + assert.NotEmpty(t, codemod.Description) + assert.Equal(t, "0.6.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) +} + +func TestMCPNetworkCodemod_NoMCPServers(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMCPNetworkCodemod_MCPServerWithoutNetwork(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + command: node server.js +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "command": "node server.js", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMCPNetworkCodemod_SingleServerWithNetwork(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + container: my-image + network: + allowed: + - example.com + - api.example.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "allowed": []any{"example.com", "api.example.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, " network:", "Should remove per-server network") + assert.Contains(t, result, "network:", "Should add top-level network") + assert.Contains(t, result, " allowed:", "Should add allowed field") + assert.Contains(t, result, " - example.com", "Should include domain") + assert.Contains(t, result, " - api.example.com", "Should include domain") + assert.Contains(t, result, "container: my-image", "Should preserve container field") +} + +func TestMCPNetworkCodemod_MultipleServersWithSameNetwork(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + server1: + container: image1 + network: + allowed: + - example.com + server2: + container: image2 + network: + allowed: + - example.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "server1": map[string]any{ + "container": "image1", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + "server2": map[string]any{ + "container": "image2", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, " allowed:", "Should remove per-server network") + assert.Contains(t, result, "network:", "Should add top-level network") + assert.Contains(t, result, " allowed:", "Should add allowed field") + // Should only have one instance of example.com (deduplication) + occurrences := 0 + for _, line := range splitLines(result) { + if line == " - example.com" { + occurrences++ + } + } + assert.Equal(t, 1, occurrences, "Should deduplicate domains") +} + +func TestMCPNetworkCodemod_MultipleServersWithDifferentNetworks(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + server1: + container: image1 + network: + allowed: + - example.com + server2: + container: image2 + network: + allowed: + - api.github.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "server1": map[string]any{ + "container": "image1", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + "server2": map[string]any{ + "container": "image2", + "network": map[string]any{ + "allowed": []any{"api.github.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "network:", "Should add top-level network") + assert.Contains(t, result, " - example.com", "Should merge domain from server1") + assert.Contains(t, result, " - api.github.com", "Should merge domain from server2") +} + +func TestMCPNetworkCodemod_MergeWithExistingTopLevelNetwork(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed: + - existing.com +mcp-servers: + my-server: + container: my-image + network: + allowed: + - new.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed": []any{"existing.com"}, + }, + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "allowed": []any{"new.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, " - existing.com", "Should keep existing domain") + assert.Contains(t, result, " - new.com", "Should add new domain") + assert.NotContains(t, result, " allowed:", "Should remove per-server network") +} + +func TestMCPNetworkCodemod_PreservesOtherMCPFields(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + type: stdio + container: my-image + env: + API_KEY: secret + network: + allowed: + - example.com + allowed: + - tool1 + - tool2 +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "type": "stdio", + "container": "my-image", + "env": map[string]any{ + "API_KEY": "secret", + }, + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + "allowed": []any{"tool1", "tool2"}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "type: stdio", "Should preserve type") + assert.Contains(t, result, "container: my-image", "Should preserve container") + assert.Contains(t, result, "env:", "Should preserve env") + assert.Contains(t, result, "API_KEY: secret", "Should preserve env value") + assert.Contains(t, result, " allowed:", "Should preserve tool allowed list") + assert.Contains(t, result, " - tool1", "Should preserve tool") + assert.NotContains(t, result, " network:", "Should remove network block") + assert.Contains(t, result, "network:", "Should add top-level network") + assert.Contains(t, result, " - example.com", "Should add domain to top-level") +} + +func TestMCPNetworkCodemod_MixedServersWithAndWithoutNetwork(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + server1: + container: image1 + network: + allowed: + - example.com + server2: + command: node server.js + server3: + container: image3 + network: + allowed: + - api.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "server1": map[string]any{ + "container": "image1", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + "server2": map[string]any{ + "command": "node server.js", + }, + "server3": map[string]any{ + "container": "image3", + "network": map[string]any{ + "allowed": []any{"api.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "command: node server.js", "Should preserve server2") + assert.Contains(t, result, "network:", "Should add top-level network") + assert.Contains(t, result, " - example.com", "Should include domain from server1") + assert.Contains(t, result, " - api.com", "Should include domain from server3") +} + +func TestMCPNetworkCodemod_PreservesMarkdown(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + container: my-image + network: + allowed: + - example.com +--- + +# Test Workflow + +This is a test workflow with: +- Multiple lines +- Markdown formatting + +` + "```yaml" + ` +key: value +` + "```" + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "# Test Workflow") + assert.Contains(t, result, "- Multiple lines") + assert.Contains(t, result, "```yaml") +} + +func TestMCPNetworkCodemod_PreservesComments(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +# MCP server configuration +mcp-servers: + my-server: + container: my-image + # Network configuration (deprecated) + network: + allowed: + - example.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "allowed": []any{"example.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "# MCP server configuration") + // Comments at the same level as the removed field are preserved (they're siblings, not children) + assert.Contains(t, result, "# Network configuration (deprecated)") +} + +func TestMCPNetworkCodemod_EmptyNetworkAllowed(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + container: my-image + network: + allowed: [] +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "allowed": []any{}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied, "Should not apply when network.allowed is empty") + _ = result // Unused but needed for test structure +} + +func TestMCPNetworkCodemod_NetworkWithoutAllowed(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +mcp-servers: + my-server: + container: my-image + network: + timeout: 30 +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "mcp-servers": map[string]any{ + "my-server": map[string]any{ + "container": "my-image", + "network": map[string]any{ + "timeout": 30, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied, "Should not apply when network doesn't have allowed field") + _ = result // Unused but needed for test structure +} + +func TestMCPNetworkCodemod_DeduplicatesAcrossServersAndTopLevel(t *testing.T) { + codemod := getMCPNetworkMigrationCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed: + - example.com + - api.com +mcp-servers: + server1: + container: image1 + network: + allowed: + - example.com + - new1.com + server2: + container: image2 + network: + allowed: + - api.com + - new2.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed": []any{"example.com", "api.com"}, + }, + "mcp-servers": map[string]any{ + "server1": map[string]any{ + "container": "image1", + "network": map[string]any{ + "allowed": []any{"example.com", "new1.com"}, + }, + }, + "server2": map[string]any{ + "container": "image2", + "network": map[string]any{ + "allowed": []any{"api.com", "new2.com"}, + }, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + + // Count occurrences of each domain + lines := splitLines(result) + domainCounts := make(map[string]int) + for _, line := range lines { + if line == " - example.com" { + domainCounts["example.com"]++ + } + if line == " - api.com" { + domainCounts["api.com"]++ + } + if line == " - new1.com" { + domainCounts["new1.com"]++ + } + if line == " - new2.com" { + domainCounts["new2.com"]++ + } + } + + assert.Equal(t, 1, domainCounts["example.com"], "Should have exactly one example.com") + assert.Equal(t, 1, domainCounts["api.com"], "Should have exactly one api.com") + assert.Equal(t, 1, domainCounts["new1.com"], "Should have exactly one new1.com") + assert.Equal(t, 1, domainCounts["new2.com"], "Should have exactly one new2.com") +} + +// Helper function to split content into lines +func splitLines(content string) []string { + lines := []string{} + current := "" + for _, char := range content { + if char == '\n' { + lines = append(lines, current) + current = "" + } else { + current += string(char) + } + } + if current != "" { + lines = append(lines, current) + } + return lines +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index e568c5759e6..2872fe80adb 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -30,5 +30,6 @@ func GetAllCodemods() []Codemod { getScheduleAtToAroundCodemod(), getDeleteSchemaFileCodemod(), getGrepToolRemovalCodemod(), + getMCPNetworkMigrationCodemod(), } } diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 4165fb84760..8ce5e0642ed 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 := 12 + expectedCount := 13 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -77,6 +77,7 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "schedule-at-to-around-migration", "delete-schema-file", "grep-tool-removal", + "mcp-network-to-top-level-migration", } for _, expectedID := range expectedIDs { @@ -113,6 +114,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "schedule-at-to-around-migration", "delete-schema-file", "grep-tool-removal", + "mcp-network-to-top-level-migration", } require.Len(t, codemods, len(expectedOrder), "Should have expected number of codemods")