Bug: Workflow with invalid MCP configuration silently proceeds

[eaftan]

This workflow file has an invalid configuration for the Playwright MCP (playwright: should be at a higher level than mcp:; this is written in the old style). When I run gh aw compile I don't get a warning or anything, I just get a .lock.yml file that silently doesn't include the Playwright MCP.

The compiler should probably error on an invalid MCP configuration block.

[github-actions]

🎯 Agentic Issue Triage

This is a legitimate bug in the workflow compiler where invalid MCP configurations are silently ignored by default, leading to poor developer experience when users expect their MCP servers to be included but they are missing from the compiled output.

Root Cause Analysis

Technical Analysis

The issue stems from the default behavior of gh aw compile:

1. Validation is opt-in, not enabled by default: In pkg/cli/commands.go:528, validation is skipped unless the --validate flag is explicitly passed:

   compiler.SetSkipValidation(!validate)

2. MCP validation exists but is bypassed: The codebase has comprehensive MCP validation in pkg/workflow/mcp-config.go via ValidateMCPConfigs() function, which includes:

   • JSON schema validation for MCP configurations
   • Type checking (stdio vs http)
   • Required field validation
   • Container vs command conflict detection

3. Silent failure mode: When validation is skipped, invalid MCP configurations are simply ignored rather than producing warnings, leading to compiled .lock.yml files that silently omit the intended MCP servers.

Impact Assessment

• Developer Experience: Poor - Users expect their MCP configurations to work but get silent failures
• Debugging Difficulty: High - No indication that the configuration is invalid
• Data Loss Risk: Medium - Workflow functionality is silently reduced

Suggested Solutions

Implementation Options

Option 1: Enable validation by default (Recommended)

• Change the default behavior to enable MCP validation
• Add a --skip-validation flag for backwards compatibility if needed
• Files to modify: pkg/cli/commands.go:528

Option 2: Add MCP-specific warnings

• Keep validation off by default but add specific warnings for common MCP configuration issues
• Check for old-style configurations and warn users
• Files to modify: pkg/workflow/compiler.go (around line 578)

Option 3: Hybrid approach

• Enable validation by default
• Make warnings less strict (warnings vs errors) for backwards compatibility
• Allow users to opt out with --no-validate

Development Resources

Code Locations and Testing

Key Files to Examine:

• Validation logic: pkg/workflow/mcp-config.go:330 (ValidateMCPConfigs)
• Compiler integration: pkg/workflow/compiler.go:578 (where validation is called)
• CLI command: pkg/cli/commands.go:523 (CompileWorkflows function)
• Test coverage: pkg/workflow/mcp_json_test.go:150 (TestValidateMCPConfigs)

Testing Strategy:

1. Create a test case with old-style MCP configuration (like the Playwright example)
2. Verify current behavior (silent ignore)
3. Implement fix and verify proper error/warning is shown
4. Test with --validate flag to ensure existing functionality works

Related Issues:

• This connects to broader MCP configuration handling improvements
• May relate to security initiatives around MCP validation (issues Proposal: Network permissions for MCP servers with Squid Proxy #78, Proposal: File system permissions for MCP servers running in Docker containers gh-aw-mcpg#275, githubnext/gh-aw-internal#719)

Reproduction Steps

# Create a workflow with old-style MCP config
cat > .github/workflows/test.md << 'EOF'
---
tools:
  playwright:
    mcp:
      playwright:
        type: stdio
        command: "npx"
        args: ["--yes", "@playwright/test"]
---

Test workflow content
EOF

# This currently succeeds silently without the playwright MCP
gh aw compile test.md

# This would show the validation error
gh aw compile --validate test.md

Next Steps

• Decide on solution approach (validation by default vs warnings)
• Implement the chosen solution
• Add test cases for old-style configuration detection
• Update documentation to clarify MCP configuration format
• Consider migration guide for users with old-style configurations

This issue significantly impacts developer experience and should be prioritized as it causes silent failures that are difficult to debug.

---

AI-generated content by Agentic Triage may contain mistakes.

[eaftan]

Also noticed that if the Claude config doesn't match the expected schema, there's no error or warning but the workflow does not contain the expected allowed tools.

Example workflow: https://github.com/githubnext/next-tf-infra-template/blob/12431a779c22e05e7870bcb50417b267989bc859/.github/workflows/daily-accessibility-review.md

• Note the missing allowed: in the Claude section in the frontmatter

Generated .lock.yml: https://github.com/githubnext/next-tf-infra-template/blob/12431a779c22e05e7870bcb50417b267989bc859/.github/workflows/agentic-accessibility-reviewer.lock.yml

[pelikhan]

I got bitten by this one too. I think we should remove the "mcp:" nested and just flatten the keys. It's too easy to forget that one.

@scout investigate how to strenghten the schema used to validate the frontmatter and the mcps.

[github-actions]

🎯 Scout Report

This issue highlights a critical UX problem where the gh aw compile command silently ignores invalid MCP configurations, leading to workflows being generated without expected functionality. The investigation reveals broader schema validation challenges beyond MCP configurations.

💥 **Core Problem Analysis**

Root Cause: The compiler has comprehensive MCP validation logic (ValidateMCPConfigs() in /pkg/workflow/mcp-config.go:330) that always runs, but there's a disconnect between what validation covers and what users expect.

Current Behavior:

• MCP structure validation works correctly (detects malformed JSON, missing required fields)
• BUT old-style configuration syntax is silently ignored during parsing, never reaching validation
• Result: .lock.yml files are generated without the intended MCP servers

Example Problem Pattern:

# OLD STYLE (silently ignored)
tools:
  playwright:
    mcp:              # ❌ This nesting level causes silent failure
      playwright:
        type: stdio
        command: "npx"
        args: ["--yes", "@playwright/test"]

# NEW STYLE (correct)
tools:
  playwright:         # ✅ Direct tool configuration
    type: stdio
    command: "npx"
    args: ["--yes", "@playwright/test"]

🔍 **Technical Investigation Findings**

Schema Processing Flow:

1. Frontmatter Parsing: pkg/parser/ extracts YAML structure
2. Tool Extraction: extractToolsFromFrontmatter() processes tools section
3. MCP Configuration: getMCPConfig() transforms tool configs
4. Validation: ValidateMCPConfigs() validates structure (when reached)
5. Generation: renderSharedMCPConfig() outputs to workflow

The Gap: Steps 2-3 silently drop incorrectly nested configurations before they reach validation.

Related Issues Context:

• Issue Proposal: Network permissions for MCP servers with Squid Proxy #78: Network permissions for MCP servers (security milestone)
• Issue Proposal: File system permissions for MCP servers running in Docker containers gh-aw-mcpg#275: File system permissions for Docker containers
• Issue githubnext/gh-aw-internal#719: Least privileges for MCP containers
• All part of v0.secure milestone showing comprehensive MCP security work

User Impact Evidence:

• @eaftan: "silently doesn't include the Playwright MCP"
• @pelikhan: "I got bitten by this one too" + request to flatten schema
• Also affects Claude configurations missing allowed: section (comment #3190388382)

🛠️ **Recommended Solutions & Implementation**

Solution Option 1: Enhanced Pre-Validation Detection (Recommended)

• Add detection for old-style patterns during extractToolsFromFrontmatter()
• Issue specific warnings for common migration patterns
• Files to modify: pkg/workflow/compiler.go around tool extraction logic

Solution Option 2: Schema Migration Assistance

• Detect old-style configurations and suggest corrections
• Provide clear migration guidance with examples
• Add gh aw validate command for configuration checking

Solution Option 3: Strengthen Schema Validation

• Expand ValidateMCPConfigs() to catch more configuration errors
• Add warnings for empty tool sections after processing
• Implement comprehensive configuration linting

Key Implementation Files:

• Core validation: pkg/workflow/mcp-config.go:330 (ValidateMCPConfigs)
• Tool processing: pkg/workflow/compiler.go:563 (extractToolsFromFrontmatter)
• Error messaging: Use pkg/console/console.go for styled warnings
• Testing: Expand pkg/workflow/mcp_json_test.go:150 test coverage

🧪 **Reproduction & Testing Strategy**

Reproduce Current Issue:

# Create test workflow with old-style MCP config
mkdir -p .github/workflows
cat > .github/workflows/test-mcp.md << 'EOF'
---
tools:
  playwright:
    mcp:
      playwright:
        type: stdio
        command: "npx"
        args: ["--yes", "@playwright/test"]
---
Test workflow content
EOF

# This succeeds but omits playwright MCP  
gh aw compile test-mcp.md

# Check generated file lacks MCP configuration
cat .github/workflows/test-mcp.lock.yml | grep -i playwright

Validation Test:

# This should catch the error (but currently doesn't for old-style configs)
gh aw compile --validate test-mcp.md

Success Criteria:

• Clear error/warning message for old-style configuration
• Suggested fix with correct syntax example
• No silent generation of incomplete workflows

📚 **Development Resources & Learning**

MCP Configuration Documentation:

• Claude MCP Server Specification
• GitHub Workflows for MCP Integration Patterns

Codebase Architecture Learning:

• Study pkg/workflow/mcp-config.go for validation patterns
• Review pkg/console/console.go for consistent CLI messaging
• Examine pkg/workflow/compiler_test.go for testing approaches

Security Context:

• This connects to broader MCP security initiatives (Hello from Claude #715, Proposal: File system permissions for MCP servers running in Docker containers gh-aw-mcpg#275, githubnext/gh-aw-internal#719)
• Part of v0.secure milestone emphasizing configuration validation
• Proper validation prevents security misconfigurations

Related CLI Tools:

# Examine validation behavior
gh aw compile --help | grep validate
gh aw compile --validate some-workflow.md

# Check current schema understanding  
gh aw inspect some-workflow.md

📋 **Next Steps Checklist**

Immediate Actions:

• Decide between warning vs. error for invalid configurations
• Determine if --validate flag should be default (breaking change assessment)
• Design clear error messages with migration guidance

Development Tasks:

• Implement old-style configuration detection in tool extraction phase
• Add specific error messages for common migration patterns
• Expand test coverage for configuration edge cases
• Update documentation with clear examples of correct syntax
• Consider gh aw migrate command for automated config updates

Quality Assurance:

• Test against existing workflow repositories for regression prevention
• Validate error messages are actionable and clear
• Ensure changes don't break valid configurations
• Document breaking changes if --validate becomes default

Priority Assessment: HIGH - This is a critical developer experience issue causing silent failures that are difficult to debug. The issue affects both new users learning MCP configuration and existing users migrating configurations.

Security Relevance: Part of the broader MCP security initiative. Proper validation prevents misconfigurations that could lead to unexpected workflow behavior.

AI-generated content by Scout The Researcher may contain mistakes.

[dsyme]

I think we should remove the "mcp:" nested and just flatten the keys. It's too easy to forget that one.

I don't see how we can do this and support "paste MCP as JSON" (which I was told is really necessary :))

Can we just improve the diagnostics enough?

[pelikhan]

Honestly not a fan of the inline JSON. MCP is complicated to configure and json -> yaml conversion is the least of your troubles

[dsyme]

Honestly not a fan of the inline JSON. MCP is complicated to configure and json -> yaml conversion is the least of your troubles

I'm sure you told me we had no choice but to allow this :)

I do think it's useful, the JSON --> YML is actually a problem for people - at least for me - I make mistakes.

[pelikhan]

we have a yaml compiler with json schema validation built, it flags the yaml annoying errors

There was an issue to get a better yaml validator that actually has source location for errors. I will bring this in.

It's important for agents (and humans) to get good errors messages.

[pelikhan]

Proposal:

• keep MCP as a string field to accept JSON payload
• support top level mcp fields
• merge both at compile time (error on clashes)

[dsyme]

Approved!