diff --git a/.changeset/patch-refactor-mcp-config-loading.md b/.changeset/patch-refactor-mcp-config-loading.md new file mode 100644 index 00000000000..ce56c09590d --- /dev/null +++ b/.changeset/patch-refactor-mcp-config-loading.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Refactor duplicate MCP config loading logic into shared helper function diff --git a/.github/workflows/go-logger.lock.yml b/.github/workflows/go-logger.lock.yml index f412671046c..f64a2d2fd43 100644 --- a/.github/workflows/go-logger.lock.yml +++ b/.github/workflows/go-logger.lock.yml @@ -90,7 +90,7 @@ jobs: with: persist-credentials: false - name: Set up Node.js - uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: cache: npm cache-dependency-path: pkg/workflow/js/package-lock.json diff --git a/.github/workflows/technical-doc-writer.lock.yml b/.github/workflows/technical-doc-writer.lock.yml index 4d0e2910210..f0d8ec889d4 100644 --- a/.github/workflows/technical-doc-writer.lock.yml +++ b/.github/workflows/technical-doc-writer.lock.yml @@ -493,7 +493,7 @@ jobs: with: persist-credentials: false - name: Setup Node.js - uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: cache: npm cache-dependency-path: docs/package-lock.json diff --git a/.github/workflows/tidy.lock.yml b/.github/workflows/tidy.lock.yml index e5dff1b8d5a..ec16335d8d3 100644 --- a/.github/workflows/tidy.lock.yml +++ b/.github/workflows/tidy.lock.yml @@ -454,7 +454,7 @@ jobs: with: persist-credentials: false - name: Set up Node.js - uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: cache: npm cache-dependency-path: pkg/workflow/js/package-lock.json diff --git a/.github/workflows/unbloat-docs.lock.yml b/.github/workflows/unbloat-docs.lock.yml index 6f638f2124d..4ed225b63c4 100644 --- a/.github/workflows/unbloat-docs.lock.yml +++ b/.github/workflows/unbloat-docs.lock.yml @@ -846,7 +846,7 @@ jobs: - name: Checkout repository uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 - name: Setup Node.js - uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 with: cache: npm cache-dependency-path: docs/package-lock.json diff --git a/pkg/cli/mcp_list.go b/pkg/cli/mcp_list.go index 241898d987c..8ba302b4091 100644 --- a/pkg/cli/mcp_list.go +++ b/pkg/cli/mcp_list.go @@ -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 { diff --git a/pkg/cli/mcp_list_tools.go b/pkg/cli/mcp_list_tools.go index 83844540787..e1a3967abe6 100644 --- a/pkg/cli/mcp_list_tools.go +++ b/pkg/cli/mcp_list_tools.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 diff --git a/pkg/cli/mcp_workflow_loader.go b/pkg/cli/mcp_workflow_loader.go new file mode 100644 index 00000000000..c66d20af91d --- /dev/null +++ b/pkg/cli/mcp_workflow_loader.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 +} diff --git a/pkg/cli/mcp_workflow_loader_test.go b/pkg/cli/mcp_workflow_loader_test.go new file mode 100644 index 00000000000..71df4944211 --- /dev/null +++ b/pkg/cli/mcp_workflow_loader_test.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)) + } + }) +} diff --git a/pkg/workflow/schemas/github-workflow.json b/pkg/workflow/schemas/github-workflow.json index 160824c1de3..6b93ceff0b8 100644 --- a/pkg/workflow/schemas/github-workflow.json +++ b/pkg/workflow/schemas/github-workflow.json @@ -983,19 +983,9 @@ "$ref": "#/definitions/types", "items": { "type": "string", - "enum": [ - "created", - "rerequested", - "completed", - "requested_action" - ] + "enum": ["created", "rerequested", "completed", "requested_action"] }, - "default": [ - "created", - "rerequested", - "completed", - "requested_action" - ] + "default": ["created", "rerequested", "completed", "requested_action"] } } }, @@ -1207,13 +1197,7 @@ "type": "string", "enum": ["created", "closed", "opened", "edited", "deleted"] }, - "default": [ - "created", - "closed", - "opened", - "edited", - "deleted" - ] + "default": ["created", "closed", "opened", "edited", "deleted"] } } }, @@ -1231,23 +1215,9 @@ "$ref": "#/definitions/types", "items": { "type": "string", - "enum": [ - "created", - "updated", - "closed", - "reopened", - "edited", - "deleted" - ] + "enum": ["created", "updated", "closed", "reopened", "edited", "deleted"] }, - "default": [ - "created", - "updated", - "closed", - "reopened", - "edited", - "deleted" - ] + "default": ["created", "updated", "closed", "reopened", "edited", "deleted"] } } }, @@ -1260,21 +1230,9 @@ "$ref": "#/definitions/types", "items": { "type": "string", - "enum": [ - "created", - "moved", - "converted", - "edited", - "deleted" - ] + "enum": ["created", "moved", "converted", "edited", "deleted"] }, - "default": [ - "created", - "moved", - "converted", - "edited", - "deleted" - ] + "default": ["created", "moved", "converted", "edited", "deleted"] } } }, @@ -1558,25 +1516,9 @@ "$ref": "#/definitions/types", "items": { "type": "string", - "enum": [ - "published", - "unpublished", - "created", - "edited", - "deleted", - "prereleased", - "released" - ] + "enum": ["published", "unpublished", "created", "edited", "deleted", "prereleased", "released"] }, - "default": [ - "published", - "unpublished", - "created", - "edited", - "deleted", - "prereleased", - "released" - ] + "default": ["published", "unpublished", "created", "edited", "deleted", "prereleased", "released"] } } },