diff --git a/.github/workflows/smoke-codex.lock.yml b/.github/workflows/smoke-codex.lock.yml index f4b863f078..81e3fe56ad 100644 --- a/.github/workflows/smoke-codex.lock.yml +++ b/.github/workflows/smoke-codex.lock.yml @@ -645,9 +645,9 @@ jobs: "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com", + "localhost;localhost:*;127.0.0.1;127.0.0.1:*", "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com" + "localhost;localhost:*;127.0.0.1;127.0.0.1:*" ] mounts = ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] diff --git a/pkg/workflow/args.go b/pkg/workflow/args.go index 0d8d21f1e2..38712d8783 100644 --- a/pkg/workflow/args.go +++ b/pkg/workflow/args.go @@ -44,9 +44,9 @@ func getGitHubCustomArgs(githubTool any) []string { } // getPlaywrightCustomArgs extracts custom args from Playwright tool configuration -func getPlaywrightCustomArgs(playwrightTool any) []string { - if toolConfig, ok := playwrightTool.(map[string]any); ok { - return extractCustomArgs(toolConfig) +func getPlaywrightCustomArgs(playwrightConfig *PlaywrightToolConfig) []string { + if playwrightConfig != nil && len(playwrightConfig.Args) > 0 { + return playwrightConfig.Args } return nil } diff --git a/pkg/workflow/args_field_test.go b/pkg/workflow/args_field_test.go index 712f661ae6..2d44294a49 100644 --- a/pkg/workflow/args_field_test.go +++ b/pkg/workflow/args_field_test.go @@ -53,7 +53,8 @@ func TestArgsField(t *testing.T) { "allowed_domains": []any{"example.com"}, "args": []any{"--browser", "firefox"}, } - result := getPlaywrightCustomArgs(playwrightTool) + playwrightConfig := parsePlaywrightTool(playwrightTool) + result := getPlaywrightCustomArgs(playwrightConfig) if len(result) != 2 { t.Errorf("Expected 2 args, got %d", len(result)) } @@ -66,7 +67,8 @@ func TestArgsField(t *testing.T) { "allowed_domains": []any{"example.com"}, "args": []string{"--headless"}, } - result = getPlaywrightCustomArgs(playwrightToolString) + playwrightConfigString := parsePlaywrightTool(playwrightToolString) + result = getPlaywrightCustomArgs(playwrightConfigString) if len(result) != 1 { t.Errorf("Expected 1 arg, got %d", len(result)) } @@ -78,7 +80,8 @@ func TestArgsField(t *testing.T) { playwrightToolDefault := map[string]any{ "allowed_domains": []any{"example.com"}, } - result = getPlaywrightCustomArgs(playwrightToolDefault) + playwrightConfigDefault := parsePlaywrightTool(playwrightToolDefault) + result = getPlaywrightCustomArgs(playwrightConfigDefault) if result != nil { t.Errorf("Expected nil args, got %v", result) } diff --git a/pkg/workflow/mcp-config-playwright.go b/pkg/workflow/mcp-config-playwright.go index 77f3c07424..460c0779c5 100644 --- a/pkg/workflow/mcp-config-playwright.go +++ b/pkg/workflow/mcp-config-playwright.go @@ -11,17 +11,17 @@ var mcpPlaywrightLog = logger.New("workflow:mcp-config-playwright") // renderPlaywrightMCPConfig generates the Playwright MCP server configuration // Uses Docker container to launch Playwright MCP for consistent browser environment // This is a shared function used by both Claude and Custom engines -func renderPlaywrightMCPConfig(yaml *strings.Builder, playwrightTool any, isLast bool) { +func renderPlaywrightMCPConfig(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig, isLast bool) { mcpPlaywrightLog.Print("Rendering Playwright MCP configuration") - renderPlaywrightMCPConfigWithOptions(yaml, playwrightTool, isLast, false, false) + renderPlaywrightMCPConfigWithOptions(yaml, playwrightConfig, isLast, false, false) } // renderPlaywrightMCPConfigWithOptions generates the Playwright MCP server configuration with engine-specific options // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. -func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightTool any, isLast bool, includeCopilotFields bool, inlineArgs bool) { - args := generatePlaywrightDockerArgs(playwrightTool) - customArgs := getPlaywrightCustomArgs(playwrightTool) +func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig, isLast bool, includeCopilotFields bool, inlineArgs bool) { + args := generatePlaywrightDockerArgs(playwrightConfig) + customArgs := getPlaywrightCustomArgs(playwrightConfig) // Extract all expressions from playwright arguments and replace them with env var references expressions := extractExpressionsFromPlaywrightArgs(args.AllowedDomains, customArgs) @@ -119,9 +119,9 @@ func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightTool // renderPlaywrightMCPConfigTOML generates the Playwright MCP server configuration in TOML format for Codex // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. -func renderPlaywrightMCPConfigTOML(yaml *strings.Builder, playwrightTool any) { - args := generatePlaywrightDockerArgs(playwrightTool) - customArgs := getPlaywrightCustomArgs(playwrightTool) +func renderPlaywrightMCPConfigTOML(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig) { + args := generatePlaywrightDockerArgs(playwrightConfig) + customArgs := getPlaywrightCustomArgs(playwrightConfig) // Use official Playwright MCP Docker image (no version tag - only one image) playwrightImage := "mcr.microsoft.com/playwright/mcp" diff --git a/pkg/workflow/mcp_benchmark_test.go b/pkg/workflow/mcp_benchmark_test.go index 359c3501ce..f9843d3e61 100644 --- a/pkg/workflow/mcp_benchmark_test.go +++ b/pkg/workflow/mcp_benchmark_test.go @@ -11,11 +11,12 @@ func BenchmarkRenderPlaywrightMCPConfig(b *testing.B) { "container": "mcr.microsoft.com/playwright:v1.41.0", "allowed-domains": []any{"github.com", "*.github.io"}, } + playwrightConfig := parsePlaywrightTool(playwrightTool) b.ResetTimer() for i := 0; i < b.N; i++ { var yaml strings.Builder - renderPlaywrightMCPConfig(&yaml, playwrightTool, true) + renderPlaywrightMCPConfig(&yaml, playwrightConfig, true) } } @@ -30,10 +31,11 @@ func BenchmarkGeneratePlaywrightDockerArgs(b *testing.B) { "*.googleapis.com", }, } + playwrightConfig := parsePlaywrightTool(playwrightTool) b.ResetTimer() for i := 0; i < b.N; i++ { - _ = generatePlaywrightDockerArgs(playwrightTool) + _ = generatePlaywrightDockerArgs(playwrightConfig) } } @@ -49,11 +51,12 @@ func BenchmarkRenderPlaywrightMCPConfig_Complex(b *testing.B) { }, "args": []any{"--debug", "--timeout", "30000"}, } + playwrightConfig := parsePlaywrightTool(playwrightTool) b.ResetTimer() for i := 0; i < b.N; i++ { var yaml strings.Builder - renderPlaywrightMCPConfig(&yaml, playwrightTool, true) + renderPlaywrightMCPConfig(&yaml, playwrightConfig, true) } } diff --git a/pkg/workflow/mcp_config_refactor_test.go b/pkg/workflow/mcp_config_refactor_test.go index b99ae632f8..3e3b916e8b 100644 --- a/pkg/workflow/mcp_config_refactor_test.go +++ b/pkg/workflow/mcp_config_refactor_test.go @@ -91,7 +91,8 @@ func TestRenderPlaywrightMCPConfigWithOptions(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var output strings.Builder - renderPlaywrightMCPConfigWithOptions(&output, tt.playwrightTool, tt.isLast, tt.includeCopilotFields, tt.inlineArgs) + playwrightConfig := parsePlaywrightTool(tt.playwrightTool) + renderPlaywrightMCPConfigWithOptions(&output, playwrightConfig, tt.isLast, tt.includeCopilotFields, tt.inlineArgs) result := output.String() @@ -321,7 +322,8 @@ func TestRenderPlaywrightMCPConfigTOML(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var output strings.Builder - renderPlaywrightMCPConfigTOML(&output, tt.playwrightTool) + playwrightConfig := parsePlaywrightTool(tt.playwrightTool) + renderPlaywrightMCPConfigTOML(&output, playwrightConfig) result := output.String() diff --git a/pkg/workflow/mcp_config_shared_test.go b/pkg/workflow/mcp_config_shared_test.go index 20e7025a3c..5575c5733b 100644 --- a/pkg/workflow/mcp_config_shared_test.go +++ b/pkg/workflow/mcp_config_shared_test.go @@ -66,7 +66,8 @@ func TestRenderPlaywrightMCPConfigShared(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var yaml strings.Builder - renderPlaywrightMCPConfig(&yaml, tt.playwrightTool, tt.isLast) + playwrightConfig := parsePlaywrightTool(tt.playwrightTool) + renderPlaywrightMCPConfig(&yaml, playwrightConfig, tt.isLast) result := yaml.String() diff --git a/pkg/workflow/mcp_renderer.go b/pkg/workflow/mcp_renderer.go index d05c125f65..24466caddf 100644 --- a/pkg/workflow/mcp_renderer.go +++ b/pkg/workflow/mcp_renderer.go @@ -122,21 +122,24 @@ func (r *MCPConfigRendererUnified) RenderGitHubMCP(yaml *strings.Builder, github func (r *MCPConfigRendererUnified) RenderPlaywrightMCP(yaml *strings.Builder, playwrightTool any) { mcpRendererLog.Printf("Rendering Playwright MCP: format=%s, inline_args=%t", r.options.Format, r.options.InlineArgs) + // Parse playwright tool configuration to strongly-typed struct + playwrightConfig := parsePlaywrightTool(playwrightTool) + if r.options.Format == "toml" { - r.renderPlaywrightTOML(yaml, playwrightTool) + r.renderPlaywrightTOML(yaml, playwrightConfig) return } // JSON format - renderPlaywrightMCPConfigWithOptions(yaml, playwrightTool, r.options.IsLast, r.options.IncludeCopilotFields, r.options.InlineArgs) + renderPlaywrightMCPConfigWithOptions(yaml, playwrightConfig, r.options.IsLast, r.options.IncludeCopilotFields, r.options.InlineArgs) } // renderPlaywrightTOML generates Playwright MCP configuration in TOML format // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. -func (r *MCPConfigRendererUnified) renderPlaywrightTOML(yaml *strings.Builder, playwrightTool any) { - args := generatePlaywrightDockerArgs(playwrightTool) - customArgs := getPlaywrightCustomArgs(playwrightTool) +func (r *MCPConfigRendererUnified) renderPlaywrightTOML(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig) { + args := generatePlaywrightDockerArgs(playwrightConfig) + customArgs := getPlaywrightCustomArgs(playwrightConfig) // Use official Playwright MCP Docker image (no version tag - only one image) playwrightImage := "mcr.microsoft.com/playwright/mcp" diff --git a/pkg/workflow/mcp_servers.go b/pkg/workflow/mcp_servers.go index ea50f1b0ba..9be11e1966 100644 --- a/pkg/workflow/mcp_servers.go +++ b/pkg/workflow/mcp_servers.go @@ -131,8 +131,9 @@ func collectMCPEnvironmentVariables(tools map[string]any, mcpTools []string, wor if hasPlaywright { // Extract all expressions from playwright arguments using ExpressionExtractor if playwrightTool, ok := tools["playwright"]; ok { - allowedDomains := generatePlaywrightAllowedDomains(playwrightTool) - customArgs := getPlaywrightCustomArgs(playwrightTool) + playwrightConfig := parsePlaywrightTool(playwrightTool) + allowedDomains := generatePlaywrightAllowedDomains(playwrightConfig) + customArgs := getPlaywrightCustomArgs(playwrightConfig) playwrightAllowedDomainsSecrets := extractExpressionsFromPlaywrightArgs(allowedDomains, customArgs) for envVarName, originalExpr := range playwrightAllowedDomainsSecrets { envVars[envVarName] = originalExpr @@ -967,70 +968,40 @@ func getGitHubAllowedTools(githubTool any) []string { return nil } -func getPlaywrightDockerImageVersion(playwrightTool any) string { +func getPlaywrightDockerImageVersion(playwrightConfig *PlaywrightToolConfig) string { playwrightDockerImageVersion := string(constants.DefaultPlaywrightBrowserVersion) // Default Playwright browser Docker image version // Extract version setting from tool properties - if toolConfig, ok := playwrightTool.(map[string]any); ok { - if versionSetting, exists := toolConfig["version"]; exists { - switch v := versionSetting.(type) { - case string: - playwrightDockerImageVersion = v - case int: - playwrightDockerImageVersion = fmt.Sprintf("%d", v) - case int64: - playwrightDockerImageVersion = fmt.Sprintf("%d", v) - case uint64: - playwrightDockerImageVersion = fmt.Sprintf("%d", v) - case float64: - playwrightDockerImageVersion = fmt.Sprintf("%g", v) - } - } + if playwrightConfig != nil && playwrightConfig.Version != "" { + playwrightDockerImageVersion = playwrightConfig.Version } return playwrightDockerImageVersion } // getPlaywrightMCPPackageVersion extracts version setting for the @playwright/mcp NPM package // This is separate from the Docker image version because they follow different versioning schemes -func getPlaywrightMCPPackageVersion(playwrightTool any) string { +func getPlaywrightMCPPackageVersion(playwrightConfig *PlaywrightToolConfig) string { // Always use the default @playwright/mcp package version. return string(constants.DefaultPlaywrightMCPVersion) } // generatePlaywrightAllowedDomains extracts domain list from Playwright tool configuration with bundle resolution // Uses the same domain bundle resolution as top-level network configuration, defaulting to localhost only -func generatePlaywrightAllowedDomains(playwrightTool any) []string { +func generatePlaywrightAllowedDomains(playwrightConfig *PlaywrightToolConfig) []string { // Default to localhost with all port variations (same as Copilot agent default) allowedDomains := constants.DefaultAllowedDomains // Extract allowed_domains from Playwright tool configuration - if toolConfig, ok := playwrightTool.(map[string]any); ok { - if domainsConfig, exists := toolConfig["allowed_domains"]; exists { - // Create a mock NetworkPermissions structure to use the same domain resolution logic - playwrightNetwork := &NetworkPermissions{} - - switch domains := domainsConfig.(type) { - case []string: - playwrightNetwork.Allowed = domains - case []any: - // Convert []any to []string - allowedDomainsSlice := make([]string, len(domains)) - for i, domain := range domains { - if domainStr, ok := domain.(string); ok { - allowedDomainsSlice[i] = domainStr - } - } - playwrightNetwork.Allowed = allowedDomainsSlice - case string: - // Single domain as string - playwrightNetwork.Allowed = []string{domains} - } + if playwrightConfig != nil && len(playwrightConfig.AllowedDomains) > 0 { + // Create a mock NetworkPermissions structure to use the same domain resolution logic + playwrightNetwork := &NetworkPermissions{ + Allowed: playwrightConfig.AllowedDomains.ToStringSlice(), + } - // Use the same domain bundle resolution as the top-level network configuration - resolvedDomains := GetAllowedDomains(playwrightNetwork) + // Use the same domain bundle resolution as the top-level network configuration + resolvedDomains := GetAllowedDomains(playwrightNetwork) - // Ensure localhost domains are always included - allowedDomains = parser.EnsureLocalhostDomains(resolvedDomains) - } + // Ensure localhost domains are always included + allowedDomains = parser.EnsureLocalhostDomains(resolvedDomains) } return allowedDomains @@ -1044,11 +1015,11 @@ type PlaywrightDockerArgs struct { } // generatePlaywrightDockerArgs creates the common Docker arguments for Playwright MCP server -func generatePlaywrightDockerArgs(playwrightTool any) PlaywrightDockerArgs { +func generatePlaywrightDockerArgs(playwrightConfig *PlaywrightToolConfig) PlaywrightDockerArgs { return PlaywrightDockerArgs{ - ImageVersion: getPlaywrightDockerImageVersion(playwrightTool), - MCPPackageVersion: getPlaywrightMCPPackageVersion(playwrightTool), - AllowedDomains: generatePlaywrightAllowedDomains(playwrightTool), + ImageVersion: getPlaywrightDockerImageVersion(playwrightConfig), + MCPPackageVersion: getPlaywrightMCPPackageVersion(playwrightConfig), + AllowedDomains: generatePlaywrightAllowedDomains(playwrightConfig), } } diff --git a/pkg/workflow/tools_parser.go b/pkg/workflow/tools_parser.go index d3aaf799e2..39c76c7f59 100644 --- a/pkg/workflow/tools_parser.go +++ b/pkg/workflow/tools_parser.go @@ -264,30 +264,42 @@ func parsePlaywrightTool(val any) *PlaywrightToolConfig { if configMap, ok := val.(map[string]any); ok { config := &PlaywrightToolConfig{} + // Handle version field - can be string or number if version, ok := configMap["version"].(string); ok { config.Version = version + } else if versionNum, ok := configMap["version"].(int); ok { + config.Version = fmt.Sprintf("%d", versionNum) + } else if versionNum, ok := configMap["version"].(int64); ok { + config.Version = fmt.Sprintf("%d", versionNum) + } else if versionNum, ok := configMap["version"].(float64); ok { + config.Version = fmt.Sprintf("%g", versionNum) } // Handle allowed_domains - can be string or array if allowedDomains, ok := configMap["allowed_domains"]; ok { if str, ok := allowedDomains.(string); ok { - config.AllowedDomains = []string{str} + config.AllowedDomains = PlaywrightAllowedDomains{PlaywrightDomain(str)} } else if arr, ok := allowedDomains.([]any); ok { - config.AllowedDomains = make([]string, 0, len(arr)) + config.AllowedDomains = make(PlaywrightAllowedDomains, 0, len(arr)) for _, item := range arr { if str, ok := item.(string); ok { - config.AllowedDomains = append(config.AllowedDomains, str) + config.AllowedDomains = append(config.AllowedDomains, PlaywrightDomain(str)) } } } } - if args, ok := configMap["args"].([]any); ok { - config.Args = make([]string, 0, len(args)) - for _, item := range args { - if str, ok := item.(string); ok { - config.Args = append(config.Args, str) + // Handle args field - can be []any or []string + if argsValue, ok := configMap["args"]; ok { + if arr, ok := argsValue.([]any); ok { + config.Args = make([]string, 0, len(arr)) + for _, item := range arr { + if str, ok := item.(string); ok { + config.Args = append(config.Args, str) + } } + } else if arr, ok := argsValue.([]string); ok { + config.Args = arr } } diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index a6d3eb65d6..21a5d90986 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -260,11 +260,26 @@ type GitHubToolConfig struct { Lockdown bool `yaml:"lockdown,omitempty"` } +// PlaywrightDomain represents a domain name allowed for Playwright browser automation +type PlaywrightDomain string + +// PlaywrightAllowedDomains is a slice of allowed domain names for Playwright +type PlaywrightAllowedDomains []PlaywrightDomain + +// ToStringSlice converts PlaywrightAllowedDomains to []string +func (p PlaywrightAllowedDomains) ToStringSlice() []string { + result := make([]string, len(p)) + for i, domain := range p { + result[i] = string(domain) + } + return result +} + // PlaywrightToolConfig represents the configuration for the Playwright tool type PlaywrightToolConfig struct { - Version string `yaml:"version,omitempty"` - AllowedDomains []string `yaml:"allowed_domains,omitempty"` - Args []string `yaml:"args,omitempty"` + Version string `yaml:"version,omitempty"` + AllowedDomains PlaywrightAllowedDomains `yaml:"allowed_domains,omitempty"` + Args []string `yaml:"args,omitempty"` } // SerenaToolConfig represents the configuration for the Serena MCP tool diff --git a/pkg/workflow/version_field_test.go b/pkg/workflow/version_field_test.go index c645fd8e86..2b9e7a55f4 100644 --- a/pkg/workflow/version_field_test.go +++ b/pkg/workflow/version_field_test.go @@ -65,7 +65,8 @@ func TestVersionField(t *testing.T) { "allowed_domains": []any{"example.com"}, "version": "v1.41.0", } - result := getPlaywrightDockerImageVersion(playwrightTool) + playwrightConfig := parsePlaywrightTool(playwrightTool) + result := getPlaywrightDockerImageVersion(playwrightConfig) if result != "v1.41.0" { t.Errorf("Expected v1.41.0, got %s", result) } @@ -74,7 +75,8 @@ func TestVersionField(t *testing.T) { playwrightToolDefault := map[string]any{ "allowed_domains": []any{"example.com"}, } - result = getPlaywrightDockerImageVersion(playwrightToolDefault) + playwrightConfigDefault := parsePlaywrightTool(playwrightToolDefault) + result = getPlaywrightDockerImageVersion(playwrightConfigDefault) if result != string(constants.DefaultPlaywrightBrowserVersion) { t.Errorf("Expected default %s, got %s", string(constants.DefaultPlaywrightBrowserVersion), result) } @@ -84,7 +86,8 @@ func TestVersionField(t *testing.T) { "allowed_domains": []any{"example.com"}, "version": 20, } - result = getPlaywrightDockerImageVersion(playwrightToolInt) + playwrightConfigInt := parsePlaywrightTool(playwrightToolInt) + result = getPlaywrightDockerImageVersion(playwrightConfigInt) if result != "20" { t.Errorf("Expected 20, got %s", result) } @@ -94,7 +97,8 @@ func TestVersionField(t *testing.T) { "allowed_domains": []any{"example.com"}, "version": 1.41, } - result = getPlaywrightDockerImageVersion(playwrightToolFloat) + playwrightConfigFloat := parsePlaywrightTool(playwrightToolFloat) + result = getPlaywrightDockerImageVersion(playwrightConfigFloat) if result != "1.41" { t.Errorf("Expected 1.41, got %s", result) } @@ -104,7 +108,8 @@ func TestVersionField(t *testing.T) { "allowed_domains": []any{"example.com"}, "version": int64(142), } - result = getPlaywrightDockerImageVersion(playwrightToolInt64) + playwrightConfigInt64 := parsePlaywrightTool(playwrightToolInt64) + result = getPlaywrightDockerImageVersion(playwrightConfigInt64) if result != "142" { t.Errorf("Expected 142, got %s", result) }