[WIP] Refactor duplicate MCP config loading logic (#2829)

* Initial plan

* Initial analysis of duplicate code issue

Co-authored-by: pelikhan <[email protected]>

* Extract shared helper for loading workflow MCP configs

- Created loadWorkflowMCPConfigs() helper in mcp_workflow_loader.go
- Updated mcp_list.go to use shared helper (removed 14 lines of duplicate code)
- Updated mcp_list_tools.go to use shared helper (removed 14 lines of duplicate code)
- Added comprehensive tests for the shared helper function
- All CLI tests pass
- Code formatted and linted successfully

Co-authored-by: pelikhan <[email protected]>

* Add changeset [skip-ci]

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Author: 3 people

.changeset/patch-refactor-mcp-config-loading.md

@@ -7,7 +7,6 @@ import (
 	"strings"
 
 	"github.com/githubnext/gh-aw/pkg/console"
-	"github.com/githubnext/gh-aw/pkg/parser"
 	"github.com/spf13/cobra"
 )
 
@@ -29,21 +28,10 @@ func ListWorkflowMCP(workflowFile string, verbose bool) error {
 		return listWorkflowsWithMCPServers(workflowsDir, verbose)
 	}
 
-	// Parse the specific workflow file
-	content, err := os.ReadFile(workflowPath)
+	// Parse the specific workflow file and extract MCP configurations
+	_, mcpConfigs, err := loadWorkflowMCPConfigs(workflowPath, "")
 	if err != nil {
-		return fmt.Errorf("failed to read workflow file: %w", err)
-	}
-
-	workflowData, err := parser.ExtractFrontmatterFromContent(string(content))
-	if err != nil {
-		return fmt.Errorf("failed to parse workflow file: %w", err)
-	}
-
-	// Extract MCP configurations (no server filter for listing)
-	mcpConfigs, err := parser.ExtractMCPConfigurations(workflowData.Frontmatter, "")
-	if err != nil {
-		return fmt.Errorf("failed to extract MCP configurations: %w", err)
+		return err
 	}
 
 	if len(mcpConfigs) == 0 {

pkg/cli/mcp_list.go

@@ -46,21 +46,10 @@ func ListToolsForMCP(workflowFile string, mcpServerName string, verbose bool) er
 		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Looking for MCP server '%s' in: %s", mcpServerName, workflowPath)))
 	}
 
-	// Parse the workflow file
-	content, err := os.ReadFile(workflowPath)
+	// Parse the workflow file and extract MCP configurations
+	_, mcpConfigs, err := loadWorkflowMCPConfigs(workflowPath, mcpServerName)
 	if err != nil {
-		return fmt.Errorf("failed to read workflow file: %w", err)
-	}
-
-	workflowData, err := parser.ExtractFrontmatterFromContent(string(content))
-	if err != nil {
-		return fmt.Errorf("failed to parse workflow file: %w", err)
-	}
-
-	// Extract MCP configurations, filtering by server name
-	mcpConfigs, err := parser.ExtractMCPConfigurations(workflowData.Frontmatter, mcpServerName)
-	if err != nil {
-		return fmt.Errorf("failed to extract MCP configurations: %w", err)
+		return err
 	}
 
 	// Find the specific MCP server

pkg/cli/mcp_list_tools.go

@@ -0,0 +1,41 @@
+package cli
+
+import (
+	"fmt"
+	"os"
+
+	"github.com/githubnext/gh-aw/pkg/parser"
+)
+
+// loadWorkflowMCPConfigs loads a workflow file and extracts MCP configurations.
+// This is a shared helper used by multiple MCP commands to avoid code duplication.
+//
+// Parameters:
+//   - workflowPath: absolute path to the workflow file
+//   - serverFilter: optional server name to filter by (empty string for no filter)
+//
+// Returns:
+//   - *parser.FrontmatterResult: parsed workflow data containing frontmatter and content
+//   - []parser.MCPServerConfig: list of MCP server configurations
+//   - error: any error that occurred during loading or parsing
+func loadWorkflowMCPConfigs(workflowPath string, serverFilter string) (*parser.FrontmatterResult, []parser.MCPServerConfig, error) {
+	// Read the workflow file
+	content, err := os.ReadFile(workflowPath)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to read workflow file: %w", err)
+	}
+
+	// Parse the frontmatter
+	workflowData, err := parser.ExtractFrontmatterFromContent(string(content))
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to parse workflow file: %w", err)
+	}
+
+	// Extract MCP configurations
+	mcpConfigs, err := parser.ExtractMCPConfigurations(workflowData.Frontmatter, serverFilter)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to extract MCP configurations: %w", err)
+	}
+
+	return workflowData, mcpConfigs, nil
+}

pkg/cli/mcp_workflow_loader.go

@@ -0,0 +1,169 @@
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/githubnext/gh-aw/pkg/constants"
+)
+
+func TestLoadWorkflowMCPConfigs(t *testing.T) {
+	// Create a temporary directory for test workflows
+	tmpDir := t.TempDir()
+	workflowsDir := filepath.Join(tmpDir, constants.GetWorkflowDir())
+	err := os.MkdirAll(workflowsDir, 0755)
+	if err != nil {
+		t.Fatalf("Failed to create test directory: %v", err)
+	}
+
+	// Create a test workflow with MCP servers
+	testWorkflowContent := `---
+on:
+  workflow_dispatch:
+
+permissions: read-all
+
+safe-outputs:
+  create-issue:
+    title-prefix: "[Test] "
+
+tools:
+  github:
+    mcp:
+      type: stdio
+      command: "npx"
+      args: ["@github/github-mcp-server"]
+      allowed: ["create_issue"]
+
+mcp-servers:
+  test-server:
+    type: stdio
+    command: "node"
+    args: ["test-server.js"]
+    allowed: ["test_tool_1", "test_tool_2"]
+
+---
+
+# Test Workflow
+This is a test workflow.`
+
+	testWorkflowPath := filepath.Join(workflowsDir, "test-workflow.md")
+	err = os.WriteFile(testWorkflowPath, []byte(testWorkflowContent), 0644)
+	if err != nil {
+		t.Fatalf("Failed to create test workflow file: %v", err)
+	}
+
+	t.Run("load_workflow_with_no_filter", func(t *testing.T) {
+		workflowData, mcpConfigs, err := loadWorkflowMCPConfigs(testWorkflowPath, "")
+		if err != nil {
+			t.Errorf("loadWorkflowMCPConfigs() error = %v", err)
+			return
+		}
+
+		if workflowData == nil {
+			t.Error("Expected workflowData to be non-nil")
+			return
+		}
+
+		if workflowData.Frontmatter == nil {
+			t.Error("Expected workflowData.Frontmatter to be non-nil")
+			return
+		}
+
+		// Should find multiple MCP servers (github, test-server, safe-outputs)
+		if len(mcpConfigs) < 2 {
+			t.Errorf("Expected at least 2 MCP servers, got %d", len(mcpConfigs))
+		}
+	})
+
+	t.Run("load_workflow_with_server_filter", func(t *testing.T) {
+		workflowData, mcpConfigs, err := loadWorkflowMCPConfigs(testWorkflowPath, "github")
+		if err != nil {
+			t.Errorf("loadWorkflowMCPConfigs() error = %v", err)
+			return
+		}
+
+		if workflowData == nil {
+			t.Error("Expected workflowData to be non-nil")
+			return
+		}
+
+		// Should find only the github MCP server
+		found := false
+		for _, config := range mcpConfigs {
+			if strings.EqualFold(config.Name, "github") {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			t.Error("Expected to find 'github' MCP server with filter")
+		}
+	})
+
+	t.Run("load_nonexistent_workflow", func(t *testing.T) {
+		nonexistentPath := filepath.Join(workflowsDir, "nonexistent.md")
+		_, _, err := loadWorkflowMCPConfigs(nonexistentPath, "")
+		if err == nil {
+			t.Error("Expected error for nonexistent workflow, got nil")
+		}
+		if !strings.Contains(err.Error(), "failed to read workflow file") {
+			t.Errorf("Expected 'failed to read workflow file' error, got: %v", err)
+		}
+	})
+
+	t.Run("load_workflow_with_invalid_frontmatter", func(t *testing.T) {
+		invalidWorkflowContent := `---
+invalid: yaml: content: [
+---
+# Invalid Workflow`
+
+		invalidWorkflowPath := filepath.Join(workflowsDir, "invalid-workflow.md")
+		err = os.WriteFile(invalidWorkflowPath, []byte(invalidWorkflowContent), 0644)
+		if err != nil {
+			t.Fatalf("Failed to create invalid workflow file: %v", err)
+		}
+
+		_, _, err = loadWorkflowMCPConfigs(invalidWorkflowPath, "")
+		if err == nil {
+			t.Error("Expected error for invalid frontmatter, got nil")
+		}
+		if !strings.Contains(err.Error(), "failed to parse workflow file") {
+			t.Errorf("Expected 'failed to parse workflow file' error, got: %v", err)
+		}
+	})
+
+	t.Run("load_workflow_without_mcp_servers", func(t *testing.T) {
+		noMCPWorkflowContent := `---
+on:
+  push:
+permissions: read-all
+---
+# No MCP Workflow`
+
+		noMCPWorkflowPath := filepath.Join(workflowsDir, "no-mcp-workflow.md")
+		err = os.WriteFile(noMCPWorkflowPath, []byte(noMCPWorkflowContent), 0644)
+		if err != nil {
+			t.Fatalf("Failed to create no-MCP workflow file: %v", err)
+		}
+
+		workflowData, mcpConfigs, err := loadWorkflowMCPConfigs(noMCPWorkflowPath, "")
+		if err != nil {
+			t.Errorf("loadWorkflowMCPConfigs() error = %v", err)
+			return
+		}
+
+		if workflowData == nil {
+			t.Error("Expected workflowData to be non-nil")
+			return
+		}
+
+		// Should return empty list, not error
+		if len(mcpConfigs) != 0 {
+			t.Errorf("Expected 0 MCP servers, got %d", len(mcpConfigs))
+		}
+	})
+}