Skip to content

Remove duplicate parsing logic from mcp inspect commands #11898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jan 26, 2026
Merged

Remove duplicate parsing logic from mcp inspect commands #11898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f759cf
Initial plan
Copilot Jan 26, 2026
391c579
Refactor: Use compiler functions for MCP inspect instead of duplicate…
Copilot Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 36 additions & 63 deletions pkg/cli/mcp_inspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,69 +47,32 @@
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))
Expand All @@ -119,16 +82,6 @@
// 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
Expand Down Expand Up @@ -283,3 +236,23 @@

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 {

Check failure on line 253 in pkg/cli/mcp_inspect.go

View workflow job for this annotation

GitHub Actions / lint-go 

S1009: should omit nil check; len() for nil maps is defined as zero (staticcheck)
frontmatter["tools"] = workflowData.Tools
}

return frontmatter
}
78 changes: 0 additions & 78 deletions pkg/cli/mcp_inspect_imports.go
View file
Open in desktop

This file was deleted.

31 changes: 10 additions & 21 deletions pkg/cli/mcp_inspect_inspector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
104 changes: 0 additions & 104 deletions pkg/cli/mcp_inspect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
Loading