From 4f759cf59a33041caa97ae9199b7b915f4d286da Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 15:27:46 +0000 Subject: [PATCH 1/2] Initial plan From 391c579adcf87234452d84d955efb53a9009fb7e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 26 Jan 2026 15:41:30 +0000 Subject: [PATCH 2/2] Refactor: Use compiler functions for MCP inspect instead of duplicate parsing - Replace manual frontmatter parsing in mcp_inspect.go with compiler.ParseWorkflowFile() - Remove duplicate applyImportsToFrontmatter function and mcp_inspect_imports.go - Update mcp_inspect_inspector.go to use compiler functions consistently - Use ParsedFrontmatter.ToMap() to reconstruct frontmatter for MCP extraction - Remove test for deleted applyImportsToFrontmatter function Benefits: - Eliminates ~100 lines of duplicate parsing/merging logic - Ensures consistent behavior between compilation and inspection - Automatically handles all import types (mcp-servers, tools, safe-inputs, etc.) - Leverages compiler's robust validation and error handling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_inspect.go | 99 +++++++++++------------------ pkg/cli/mcp_inspect_imports.go | 78 ----------------------- pkg/cli/mcp_inspect_inspector.go | 31 +++------ pkg/cli/mcp_inspect_test.go | 104 ------------------------------- 4 files changed, 46 insertions(+), 266 deletions(-) delete mode 100644 pkg/cli/mcp_inspect_imports.go diff --git a/pkg/cli/mcp_inspect.go b/pkg/cli/mcp_inspect.go index 49ed9e8357..54b8f4e38b 100644 --- a/pkg/cli/mcp_inspect.go +++ b/pkg/cli/mcp_inspect.go @@ -47,69 +47,32 @@ func InspectWorkflowMCP(workflowFile string, serverFilter string, toolFilter str fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Inspecting MCP servers in: %s", workflowPath))) } - // Parse the workflow file for MCP configurations - content, err := os.ReadFile(workflowPath) + // Use the compiler to parse the workflow file + // This automatically handles imports, merging, and validation + compiler := workflow.NewCompiler(verbose, "", "") + workflowData, err := compiler.ParseWorkflowFile(workflowPath) if err != nil { - errMsg := fmt.Sprintf("failed to read workflow file: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return fmt.Errorf("failed to read workflow file: %w", err) - } + // Handle shared workflow error separately (not a fatal error for inspection) + if _, isSharedWorkflow := err.(*workflow.SharedWorkflowError); isSharedWorkflow { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Cannot inspect shared/imported workflows directly - they must be imported by a main workflow")) + return nil + } - parsedData, err := parser.ExtractFrontmatterFromContent(string(content)) - if err != nil { errMsg := fmt.Sprintf("failed to parse workflow file: %v", err) fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) return fmt.Errorf("failed to parse workflow file: %w", err) } - // Validate frontmatter before analyzing MCPs - if err := parser.ValidateMainWorkflowFrontmatterWithSchemaAndLocation(parsedData.Frontmatter, workflowPath); err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Frontmatter validation failed: %v", err))) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Continuing with MCP inspection (validation errors may affect results)")) - } - // Don't return error - continue with inspection even if validation fails - } else if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Frontmatter validation passed")) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Workflow parsed successfully")) } - // Process imports from frontmatter to merge imported MCP servers - markdownDir := filepath.Dir(workflowPath) - importsResult, err := parser.ProcessImportsFromFrontmatterWithManifest(parsedData.Frontmatter, markdownDir, nil) - if err != nil { - errMsg := fmt.Sprintf("failed to process imports from frontmatter: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return fmt.Errorf("failed to process imports from frontmatter: %w", err) - } + // Build frontmatter map from WorkflowData for MCP extraction + // This includes all merged imports and tools + frontmatterForMCP := buildFrontmatterFromWorkflowData(workflowData) - // Apply imported MCP servers to frontmatter - frontmatterWithImports, err := applyImportsToFrontmatter(parsedData.Frontmatter, importsResult) - if err != nil { - errMsg := fmt.Sprintf("failed to apply imports: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return fmt.Errorf("failed to apply imports: %w", err) - } - - // Validate MCP configurations specifically using compiler validation - if toolsSection, hasTools := frontmatterWithImports["tools"]; hasTools { - if tools, ok := toolsSection.(map[string]any); ok { - if err := workflow.ValidateMCPConfigs(tools); err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("MCP configuration validation failed: %v", err))) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Continuing with MCP inspection (validation errors may affect results)")) - } else { - errMsg := fmt.Sprintf("MCP configuration validation failed: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return fmt.Errorf("MCP configuration validation failed: %w", err) - } - } else if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("MCP configuration validation passed")) - } - } - } - - // Extract MCP configurations from frontmatter with imports applied - mcpConfigs, err := parser.ExtractMCPConfigurations(frontmatterWithImports, serverFilter) + // Extract MCP configurations from the merged frontmatter + mcpConfigs, err := parser.ExtractMCPConfigurations(frontmatterForMCP, serverFilter) if err != nil { errMsg := fmt.Sprintf("failed to extract MCP configurations: %v", err) fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) @@ -119,16 +82,6 @@ func InspectWorkflowMCP(workflowFile string, serverFilter string, toolFilter str // Filter out safe-outputs MCP servers for inspection mcpConfigs = filterOutSafeOutputs(mcpConfigs) - // Check if safe-inputs are present in the workflow by parsing with the compiler - // (the compiler resolves imports and merges safe-inputs) - compiler := workflow.NewCompiler(verbose, "", "") - workflowData, err := compiler.ParseWorkflowFile(workflowPath) - if err != nil { - if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse workflow for safe-inputs: %v", err))) - } - } - // Start safe-inputs server if present var safeInputsServerCmd *exec.Cmd var safeInputsTmpDir string @@ -283,3 +236,23 @@ The command will: return cmd } + +// buildFrontmatterFromWorkflowData reconstructs a frontmatter map from WorkflowData +// This is used to extract MCP configurations after the compiler has processed imports and merging +func buildFrontmatterFromWorkflowData(workflowData *workflow.WorkflowData) map[string]any { + // Use the parsed frontmatter's ToMap() method if available + // This preserves the original frontmatter structure with imports already merged + if workflowData.ParsedFrontmatter != nil { + return workflowData.ParsedFrontmatter.ToMap() + } + + // Fallback to building manually (shouldn't happen in normal cases) + frontmatter := make(map[string]any) + + // Add tools section if present + if workflowData.Tools != nil && len(workflowData.Tools) > 0 { + frontmatter["tools"] = workflowData.Tools + } + + return frontmatter +} diff --git a/pkg/cli/mcp_inspect_imports.go b/pkg/cli/mcp_inspect_imports.go deleted file mode 100644 index 19344f18db..0000000000 --- a/pkg/cli/mcp_inspect_imports.go +++ /dev/null @@ -1,78 +0,0 @@ -package cli - -import ( - "fmt" - "os" - - "github.com/githubnext/gh-aw/pkg/console" - "github.com/githubnext/gh-aw/pkg/parser" - "github.com/githubnext/gh-aw/pkg/workflow" -) - -// applyImportsToFrontmatter merges imported MCP servers and tools into frontmatter -// Returns a new frontmatter map with imports applied -func applyImportsToFrontmatter(frontmatter map[string]any, importsResult *parser.ImportsResult) (map[string]any, error) { - mcpInspectLog.Print("Applying imports to frontmatter") - - // Create a copy of the frontmatter to avoid modifying the original - result := make(map[string]any) - for k, v := range frontmatter { - result[k] = v - } - - // If there are no imported MCP servers or tools, return as-is - if importsResult.MergedMCPServers == "" && importsResult.MergedTools == "" { - return result, nil - } - - // Get existing mcp-servers from frontmatter - var existingMCPServers map[string]any - if mcpServersSection, exists := result["mcp-servers"]; exists { - if mcpServers, ok := mcpServersSection.(map[string]any); ok { - existingMCPServers = mcpServers - } - } - if existingMCPServers == nil { - existingMCPServers = make(map[string]any) - } - - // Merge imported MCP servers using the workflow compiler's merge logic - compiler := workflow.NewCompiler(false, "", "") - mergedMCPServers, err := compiler.MergeMCPServers(existingMCPServers, importsResult.MergedMCPServers) - if err != nil { - errMsg := fmt.Sprintf("failed to merge imported MCP servers: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return nil, fmt.Errorf("failed to merge imported MCP servers: %w", err) - } - - // Update mcp-servers in the result - if len(mergedMCPServers) > 0 { - result["mcp-servers"] = mergedMCPServers - } - - // Get existing tools from frontmatter - var existingTools map[string]any - if toolsSection, exists := result["tools"]; exists { - if tools, ok := toolsSection.(map[string]any); ok { - existingTools = tools - } - } - if existingTools == nil { - existingTools = make(map[string]any) - } - - // Merge imported tools using the workflow compiler's merge logic - mergedTools, err := compiler.MergeTools(existingTools, importsResult.MergedTools) - if err != nil { - errMsg := fmt.Sprintf("failed to merge imported tools: %v", err) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) - return nil, fmt.Errorf("failed to merge imported tools: %w", err) - } - - // Update tools in the result - if len(mergedTools) > 0 { - result["tools"] = mergedTools - } - - return result, nil -} diff --git a/pkg/cli/mcp_inspect_inspector.go b/pkg/cli/mcp_inspect_inspector.go index 6fc31bbcbb..ccc230c3e1 100644 --- a/pkg/cli/mcp_inspect_inspector.go +++ b/pkg/cli/mcp_inspect_inspector.go @@ -11,6 +11,7 @@ import ( "github.com/githubnext/gh-aw/pkg/console" "github.com/githubnext/gh-aw/pkg/parser" + "github.com/githubnext/gh-aw/pkg/workflow" ) // spawnMCPInspector launches the official @modelcontextprotocol/inspector tool @@ -42,32 +43,20 @@ func spawnMCPInspector(workflowFile string, serverFilter string, verbose bool) e workflowPath = filepath.Join(cwd, workflowPath) } - // Parse the workflow file to extract MCP configurations - content, err := os.ReadFile(workflowPath) + // Use the compiler to parse the workflow file + // This automatically handles imports, merging, and validation + compiler := workflow.NewCompiler(verbose, "", "") + workflowData, err := compiler.ParseWorkflowFile(workflowPath) if err != nil { return err } - workflowData, err := parser.ExtractFrontmatterFromContent(string(content)) - if err != nil { - return err - } - - // Process imports from frontmatter to merge imported MCP servers - markdownDir := filepath.Dir(workflowPath) - importsResult, err := parser.ProcessImportsFromFrontmatterWithManifest(workflowData.Frontmatter, markdownDir, nil) - if err != nil { - return fmt.Errorf("failed to process imports from frontmatter: %w", err) - } - - // Apply imported MCP servers to frontmatter - frontmatterWithImports, err := applyImportsToFrontmatter(workflowData.Frontmatter, importsResult) - if err != nil { - return fmt.Errorf("failed to apply imports: %w", err) - } + // Build frontmatter map from WorkflowData for MCP extraction + // This includes all merged imports and tools + frontmatterForMCP := buildFrontmatterFromWorkflowData(workflowData) - // Extract MCP configurations from frontmatter with imports applied - mcpConfigs, err = parser.ExtractMCPConfigurations(frontmatterWithImports, serverFilter) + // Extract MCP configurations from the merged frontmatter + mcpConfigs, err = parser.ExtractMCPConfigurations(frontmatterForMCP, serverFilter) if err != nil { return err } diff --git a/pkg/cli/mcp_inspect_test.go b/pkg/cli/mcp_inspect_test.go index 0b8b49b03b..d19321345e 100644 --- a/pkg/cli/mcp_inspect_test.go +++ b/pkg/cli/mcp_inspect_test.go @@ -334,107 +334,3 @@ func TestFilterOutSafeOutputs(t *testing.T) { }) } } - -func TestApplyImportsToFrontmatter(t *testing.T) { - tests := []struct { - name string - frontmatter map[string]any - importsResult *parser.ImportsResult - expectError bool - validate func(t *testing.T, result map[string]any) - }{ - { - name: "merge imported MCP servers", - frontmatter: map[string]any{ - "on": "issues", - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []any{"create_issue"}, - }, - }, - }, - importsResult: &parser.ImportsResult{ - MergedMCPServers: `{"test-server":{"command":"node","args":["test.js"]}}`, - }, - expectError: false, - validate: func(t *testing.T, result map[string]any) { - mcpServers, ok := result["mcp-servers"].(map[string]any) - if !ok { - t.Fatal("Expected mcp-servers to be a map") - } - if _, exists := mcpServers["test-server"]; !exists { - t.Error("Expected test-server to be in mcp-servers") - } - }, - }, - { - name: "merge imported tools", - frontmatter: map[string]any{ - "on": "issues", - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []any{"create_issue"}, - }, - }, - }, - importsResult: &parser.ImportsResult{ - MergedTools: `{"bash":{"allowed":["git status"]}}`, - }, - expectError: false, - validate: func(t *testing.T, result map[string]any) { - tools, ok := result["tools"].(map[string]any) - if !ok { - t.Fatal("Expected tools to be a map") - } - if _, exists := tools["bash"]; !exists { - t.Error("Expected bash to be in tools") - } - if _, exists := tools["github"]; !exists { - t.Error("Expected github to be preserved in tools") - } - }, - }, - { - name: "no imports returns same frontmatter", - frontmatter: map[string]any{ - "on": "issues", - "tools": map[string]any{ - "github": map[string]any{ - "allowed": []any{"create_issue"}, - }, - }, - }, - importsResult: &parser.ImportsResult{}, - expectError: false, - validate: func(t *testing.T, result map[string]any) { - if _, exists := result["mcp-servers"]; exists { - t.Error("Expected no mcp-servers when none are imported") - } - tools, ok := result["tools"].(map[string]any) - if !ok { - t.Fatal("Expected tools to be preserved") - } - if _, exists := tools["github"]; !exists { - t.Error("Expected github to be preserved in tools") - } - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, err := applyImportsToFrontmatter(tt.frontmatter, tt.importsResult) - - if tt.expectError && err == nil { - t.Error("Expected error but got none") - } - if !tt.expectError && err != nil { - t.Errorf("Unexpected error: %v", err) - } - - if !tt.expectError && tt.validate != nil { - tt.validate(t, result) - } - }) - } -}