diff --git a/.changeset/patch-propagate-mcp-allowed-filter.md b/.changeset/patch-propagate-mcp-allowed-filter.md new file mode 100644 index 00000000000..14c50fef451 --- /dev/null +++ b/.changeset/patch-propagate-mcp-allowed-filter.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fix MCP gateway `tools` output so `mcp-servers.allowed` filters apply for all engines instead of only Copilot. diff --git a/pkg/workflow/mcp_config_compilation_test.go b/pkg/workflow/mcp_config_compilation_test.go index d18a26cde11..abc6cd6c59e 100644 --- a/pkg/workflow/mcp_config_compilation_test.go +++ b/pkg/workflow/mcp_config_compilation_test.go @@ -187,6 +187,184 @@ func TestHasMCPConfigDetection(t *testing.T) { } } +// TestMCPServersAllowedToolFilterCompilation verifies that the allowed tool filter in +// mcp-servers section is properly compiled into the "tools" field in the output. +func TestMCPServersAllowedToolFilterCompilation(t *testing.T) { + tests := []struct { + name string + workflowContent string + serverName string + expectedContent []string + unexpectedInServer []string + }{ + { + name: "copilot - http mcp server with specific allowed tools", + workflowContent: `--- +on: + workflow_dispatch: +strict: false +permissions: + contents: read +engine: copilot +mcp-servers: + my-api: + type: http + url: https://api.example.com/mcp + allowed: + - get_data + - list_items +--- + +Test workflow. +`, + serverName: `"my-api"`, + expectedContent: []string{`"get_data"`, `"list_items"`}, + unexpectedInServer: []string{`"*"`}, + }, + { + name: "copilot - stdio mcp server with specific allowed tools", + workflowContent: `--- +on: + workflow_dispatch: +strict: false +permissions: + contents: read +engine: copilot +mcp-servers: + my-tool: + container: example/tool:latest + allowed: + - run_query + - fetch_results +--- + +Test workflow. +`, + serverName: `"my-tool"`, + expectedContent: []string{`"run_query"`, `"fetch_results"`}, + unexpectedInServer: []string{`"*"`}, + }, + { + name: "copilot - mcp server with no allowed field defaults to wildcard", + workflowContent: `--- +on: + workflow_dispatch: +strict: false +permissions: + contents: read +engine: copilot +mcp-servers: + my-api: + type: http + url: https://api.example.com/mcp +--- + +Test workflow. +`, + serverName: `"my-api"`, + expectedContent: []string{`"*"`}, + unexpectedInServer: []string{}, + }, + { + name: "claude - http mcp server with specific allowed tools passes through", + workflowContent: `--- +on: + workflow_dispatch: +strict: false +permissions: + contents: read +engine: claude +mcp-servers: + my-api: + type: http + url: https://api.example.com/mcp + allowed: + - get_data + - list_items +--- + +Test workflow. +`, + serverName: `"my-api"`, + expectedContent: []string{`"get_data"`, `"list_items"`}, + unexpectedInServer: []string{`"*"`}, + }, + { + name: "claude - http mcp server with no allowed field has no tools filter", + workflowContent: `--- +on: + workflow_dispatch: +strict: false +permissions: + contents: read +engine: claude +mcp-servers: + my-api: + type: http + url: https://api.example.com/mcp +--- + +Test workflow. +`, + serverName: `"my-api"`, + expectedContent: []string{`"url":`}, + unexpectedInServer: []string{`"tools":`}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpFile, err := os.CreateTemp("", "test-allowed-filter-*.md") + if err != nil { + t.Fatalf("Failed to create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + + if _, err := tmpFile.WriteString(tt.workflowContent); err != nil { + t.Fatalf("Failed to write to temp file: %v", err) + } + tmpFile.Close() + + compiler := NewCompiler() + compiler.SetSkipValidation(true) + + workflowData, err := compiler.ParseWorkflowFile(tmpFile.Name()) + if err != nil { + t.Fatalf("Failed to parse workflow file: %v", err) + } + + yamlContent, err := compiler.generateYAML(workflowData, tmpFile.Name()) + if err != nil { + t.Fatalf("Failed to generate YAML: %v", err) + } + + // Find the server-specific block in the YAML + serverIndex := strings.Index(yamlContent, tt.serverName) + if serverIndex == -1 { + t.Fatalf("Could not find server %s in generated YAML", tt.serverName) + } + + // Extract the server block (next 500 chars should be sufficient) + endIdx := min(serverIndex+500, len(yamlContent)) + serverBlock := yamlContent[serverIndex:endIdx] + + for _, content := range tt.expectedContent { + if !strings.Contains(serverBlock, content) { + t.Errorf("Expected %q in server block for %s, but not found.\nServer block:\n%s", + content, tt.serverName, serverBlock) + } + } + + for _, content := range tt.unexpectedInServer { + if strings.Contains(serverBlock, content) { + t.Errorf("Unexpected %q found in server block for %s.\nServer block:\n%s", + content, tt.serverName, serverBlock) + } + } + }) + } +} + // TestDevModeAgenticWorkflowsContainer verifies that the agentic-workflows MCP server // uses the locally built Docker image in dev mode instead of alpine:latest func TestDevModeAgenticWorkflowsContainer(t *testing.T) { diff --git a/pkg/workflow/mcp_config_custom.go b/pkg/workflow/mcp_config_custom.go index e7217b1e57b..8f557bba610 100644 --- a/pkg/workflow/mcp_config_custom.go +++ b/pkg/workflow/mcp_config_custom.go @@ -122,16 +122,12 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma // TOML format for HTTP MCP servers uses url and http_headers propertyOrder = []string{"url", "http_headers"} } else { - // JSON format - include copilot fields if required - if renderer.RequiresCopilotFields { - // For HTTP MCP with secrets in headers, env passthrough is needed - if len(headerSecrets) > 0 { - propertyOrder = []string{"type", "url", "headers", "tools", "env"} - } else { - propertyOrder = []string{"type", "url", "headers", "tools"} - } + // JSON format - include tools field for MCP gateway tool filtering (all engines) + // For HTTP MCP with secrets in headers, env passthrough is needed + if len(headerSecrets) > 0 { + propertyOrder = []string{"type", "url", "headers", "tools", "env"} } else { - propertyOrder = []string{"type", "url", "headers"} + propertyOrder = []string{"type", "url", "headers", "tools"} } } default: @@ -147,8 +143,11 @@ func renderSharedMCPConfig(yaml *strings.Builder, toolName string, toolConfig ma // Include type field only for engines that require copilot fields existingProperties = append(existingProperties, prop) case "tools": - // Include tools field only for engines that require copilot fields - if renderer.RequiresCopilotFields { + // Include tools field for JSON format when: + // - RequiresCopilotFields (Copilot always renders it; when Allowed is empty, the + // rendering code below defaults to the "*" wildcard) + // - OR allowed tools are explicitly specified (pass the filter to the MCP gateway) + if renderer.RequiresCopilotFields || len(mcpConfig.Allowed) > 0 { existingProperties = append(existingProperties, prop) } case "container":