From 5b1f87d05071cf7cb88aeaf539c5a196f0f99389 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 15 Nov 2025 21:47:53 +0000 Subject: [PATCH 1/2] Pin Playwright to version 1.56.1 and add version validation - Add DefaultPlaywrightVersion constant set to 1.56.1 - Update mcp-config.go to use pinned version instead of 'latest' - Update mcp_renderer.go to use pinned version - Add validatePlaywrightVersion() function to check version consistency - Integrate Playwright version validation into runtime validation flow - Provide warnings when using 'latest' or mismatched versions Addresses issue #4099 --- pkg/constants/constants.go | 3 ++ pkg/workflow/mcp-config.go | 5 ++- pkg/workflow/mcp_renderer.go | 2 +- pkg/workflow/npm_validation.go | 60 ++++++++++++++++++++++++++++++ pkg/workflow/runtime_validation.go | 4 ++ 5 files changed, 71 insertions(+), 3 deletions(-) diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 2c5d4abd8b5..2b1fb642166 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -56,6 +56,9 @@ const DefaultHaskellVersion = "9.10" // DefaultDenoVersion is the default version of Deno for runtime setup const DefaultDenoVersion = "2.x" +// DefaultPlaywrightVersion is the default version of Playwright MCP server +const DefaultPlaywrightVersion = "1.56.1" + // DefaultAgenticWorkflowTimeoutMinutes is the default timeout for agentic workflow execution in minutes const DefaultAgenticWorkflowTimeoutMinutes = 20 diff --git a/pkg/workflow/mcp-config.go b/pkg/workflow/mcp-config.go index b588cff0233..617d5666835 100644 --- a/pkg/workflow/mcp-config.go +++ b/pkg/workflow/mcp-config.go @@ -36,7 +36,8 @@ func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightTool } // Determine version to use - respect version configuration if provided - playwrightPackage := "@playwright/mcp@latest" + // Default to the pinned version from constants + playwrightPackage := "@playwright/mcp@" + constants.DefaultPlaywrightVersion if includeCopilotFields && args.ImageVersion != "" && args.ImageVersion != "latest" { playwrightPackage = "@playwright/mcp@" + args.ImageVersion } @@ -203,7 +204,7 @@ func renderPlaywrightMCPConfigTOML(yaml *strings.Builder, playwrightTool any) { yaml.WriteString(" [mcp_servers.playwright]\n") yaml.WriteString(" command = \"npx\"\n") yaml.WriteString(" args = [\n") - yaml.WriteString(" \"@playwright/mcp@latest\",\n") + yaml.WriteString(fmt.Sprintf(" \"@playwright/mcp@%s\",\n", constants.DefaultPlaywrightVersion)) yaml.WriteString(" \"--output-dir\",\n") yaml.WriteString(" \"/tmp/gh-aw/mcp-logs/playwright\"") if len(args.AllowedDomains) > 0 { diff --git a/pkg/workflow/mcp_renderer.go b/pkg/workflow/mcp_renderer.go index c220d291ffb..5dcebcf4f3e 100644 --- a/pkg/workflow/mcp_renderer.go +++ b/pkg/workflow/mcp_renderer.go @@ -117,7 +117,7 @@ func (r *MCPConfigRendererUnified) renderPlaywrightTOML(yaml *strings.Builder, p yaml.WriteString(" [mcp_servers.playwright]\n") yaml.WriteString(" command = \"npx\"\n") yaml.WriteString(" args = [\n") - yaml.WriteString(" \"@playwright/mcp@latest\",\n") + yaml.WriteString(fmt.Sprintf(" \"@playwright/mcp@%s\",\n", constants.DefaultPlaywrightVersion)) yaml.WriteString(" \"--output-dir\",\n") yaml.WriteString(" \"/tmp/gh-aw/mcp-logs/playwright\"") if len(args.AllowedDomains) > 0 { diff --git a/pkg/workflow/npm_validation.go b/pkg/workflow/npm_validation.go index 86984bb520c..fcff14be1cd 100644 --- a/pkg/workflow/npm_validation.go +++ b/pkg/workflow/npm_validation.go @@ -37,6 +37,7 @@ import ( "strings" "github.com/githubnext/gh-aw/pkg/console" + "github.com/githubnext/gh-aw/pkg/constants" "github.com/githubnext/gh-aw/pkg/logger" ) @@ -86,3 +87,62 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { npmValidationLog.Print("All npx packages validated successfully") return nil } + +// validatePlaywrightVersion validates that Playwright package version matches the pinned constant +// This ensures consistent Playwright versions across all workflows +func (c *Compiler) validatePlaywrightVersion(workflowData *WorkflowData) error { + packages := extractNpxPackages(workflowData) + if len(packages) == 0 { + return nil + } + + npmValidationLog.Print("Checking Playwright package versions") + + var warnings []string + for _, pkg := range packages { + // Check if this is a Playwright package + if !strings.HasPrefix(pkg, "@playwright/mcp") { + continue + } + + // Parse the package version + parts := strings.Split(pkg, "@") + if len(parts) < 3 { + // No version specified - this is fine, will use default + continue + } + + version := parts[len(parts)-1] + + // Check if version matches pinned constant or is "latest" + if version != constants.DefaultPlaywrightVersion && version != "latest" { + npmValidationLog.Printf("Playwright version mismatch: %s (expected: %s)", pkg, constants.DefaultPlaywrightVersion) + warnings = append(warnings, fmt.Sprintf( + "Playwright package '%s' version does not match pinned version '%s'. Consider updating to '@playwright/mcp@%s' for consistency", + pkg, constants.DefaultPlaywrightVersion, constants.DefaultPlaywrightVersion, + )) + } else if version == "latest" { + npmValidationLog.Printf("Playwright using 'latest' tag, recommend pinning to: %s", constants.DefaultPlaywrightVersion) + if c.verbose { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf( + "⚠ Playwright package '%s' uses 'latest' tag. Consider pinning to version %s for reproducibility", + pkg, constants.DefaultPlaywrightVersion, + ))) + } + } else { + npmValidationLog.Printf("Playwright version validated: %s", pkg) + if c.verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("✓ Playwright version validated: %s", pkg))) + } + } + } + + if len(warnings) > 0 { + npmValidationLog.Printf("Playwright version validation completed with %d warnings", len(warnings)) + for _, warning := range warnings { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warning)) + } + } + + return nil +} diff --git a/pkg/workflow/runtime_validation.go b/pkg/workflow/runtime_validation.go index 907237f01af..03c927f37ae 100644 --- a/pkg/workflow/runtime_validation.go +++ b/pkg/workflow/runtime_validation.go @@ -156,6 +156,10 @@ func (c *Compiler) validateRuntimePackages(workflowData *WorkflowData) error { if err := c.validateNpxPackages(workflowData); err != nil { errors = append(errors, err.Error()) } + // Validate Playwright package versions for consistency + if err := c.validatePlaywrightVersion(workflowData); err != nil { + errors = append(errors, err.Error()) + } case "python": // Validate pip packages used in the workflow if err := c.validatePipPackages(workflowData); err != nil { From ad8b6783dcb30abc63ce38fd6f4b141f9137e196 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Nov 2025 14:41:37 -0800 Subject: [PATCH 2/2] Allow explicit "latest" version for Playwright while defaulting to pinned version (#4101) --- .github/workflows/blog-auditor.lock.yml | 2 +- .github/workflows/cloclo.lock.yml | 2 +- .../daily-multi-device-docs-tester.lock.yml | 2 +- .github/workflows/unbloat-docs.lock.yml | 2 +- pkg/workflow/custom_engine_test.go | 12 ++-- pkg/workflow/mcp-config.go | 13 ++++- pkg/workflow/mcp_config_refactor_test.go | 6 +- pkg/workflow/mcp_config_shared_test.go | 6 +- pkg/workflow/mcp_servers.go | 3 +- .../playwright_version_latest_test.go | 55 +++++++++++++++++++ pkg/workflow/version_field_test.go | 18 ++++-- 11 files changed, 99 insertions(+), 22 deletions(-) create mode 100644 pkg/workflow/playwright_version_latest_test.go diff --git a/.github/workflows/blog-auditor.lock.yml b/.github/workflows/blog-auditor.lock.yml index 05afb23d75a..e4c9f847885 100644 --- a/.github/workflows/blog-auditor.lock.yml +++ b/.github/workflows/blog-auditor.lock.yml @@ -1170,7 +1170,7 @@ jobs: "playwright": { "command": "npx", "args": [ - "@playwright/mcp@latest", + "@playwright/mcp@1.56.1", "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-origins", diff --git a/.github/workflows/cloclo.lock.yml b/.github/workflows/cloclo.lock.yml index 4088bbab622..b84bef521cb 100644 --- a/.github/workflows/cloclo.lock.yml +++ b/.github/workflows/cloclo.lock.yml @@ -2276,7 +2276,7 @@ jobs: "playwright": { "command": "npx", "args": [ - "@playwright/mcp@latest", + "@playwright/mcp@1.56.1", "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-origins", diff --git a/.github/workflows/daily-multi-device-docs-tester.lock.yml b/.github/workflows/daily-multi-device-docs-tester.lock.yml index 784a9778701..7d6a8458644 100644 --- a/.github/workflows/daily-multi-device-docs-tester.lock.yml +++ b/.github/workflows/daily-multi-device-docs-tester.lock.yml @@ -1180,7 +1180,7 @@ jobs: "playwright": { "command": "npx", "args": [ - "@playwright/mcp@latest", + "@playwright/mcp@v1.56.1", "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-origins", diff --git a/.github/workflows/unbloat-docs.lock.yml b/.github/workflows/unbloat-docs.lock.yml index b808ba2d0df..d098c28ad60 100644 --- a/.github/workflows/unbloat-docs.lock.yml +++ b/.github/workflows/unbloat-docs.lock.yml @@ -1988,7 +1988,7 @@ jobs: "playwright": { "command": "npx", "args": [ - "@playwright/mcp@latest", + "@playwright/mcp@1.56.1", "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-origins", diff --git a/pkg/workflow/custom_engine_test.go b/pkg/workflow/custom_engine_test.go index 0ff7477353c..548c608d07d 100644 --- a/pkg/workflow/custom_engine_test.go +++ b/pkg/workflow/custom_engine_test.go @@ -300,9 +300,9 @@ func TestCustomEngineRenderPlaywrightMCPConfigWithDomainConfiguration(t *testing t.Errorf("Expected Playwright configuration in output") } - // Check that it contains Playwright MCP npx configuration - if !strings.Contains(output, "@playwright/mcp@latest") { - t.Errorf("Expected Playwright MCP npx package in output") + // Check that it contains Playwright MCP npx configuration with the specified version + if !strings.Contains(output, "@playwright/mcp@v1.40.0") { + t.Errorf("Expected Playwright MCP npx package with v1.40.0 in output") } // Check that it contains --allowed-origins flag when domains are configured @@ -351,9 +351,9 @@ func TestCustomEngineRenderPlaywrightMCPConfigDefaultDomains(t *testing.T) { t.Errorf("Expected Playwright configuration in output") } - // Check that it contains Playwright MCP npx configuration - if !strings.Contains(output, "@playwright/mcp@latest") { - t.Errorf("Expected Playwright MCP npx package in output") + // Check that it contains Playwright MCP npx configuration with the specified version + if !strings.Contains(output, "@playwright/mcp@v1.40.0") { + t.Errorf("Expected Playwright MCP npx package with v1.40.0 in output") } // Check that it contains --allowed-origins flag for default domains diff --git a/pkg/workflow/mcp-config.go b/pkg/workflow/mcp-config.go index 617d5666835..09a234bb7ca 100644 --- a/pkg/workflow/mcp-config.go +++ b/pkg/workflow/mcp-config.go @@ -36,9 +36,9 @@ func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightTool } // Determine version to use - respect version configuration if provided - // Default to the pinned version from constants + // Default to the pinned version from constants when no version is specified playwrightPackage := "@playwright/mcp@" + constants.DefaultPlaywrightVersion - if includeCopilotFields && args.ImageVersion != "" && args.ImageVersion != "latest" { + if args.ImageVersion != "" { playwrightPackage = "@playwright/mcp@" + args.ImageVersion } @@ -200,11 +200,18 @@ func renderPlaywrightMCPConfigTOML(yaml *strings.Builder, playwrightTool any) { args := generatePlaywrightDockerArgs(playwrightTool) customArgs := getPlaywrightCustomArgs(playwrightTool) + // Determine version to use - respect version configuration if provided + // Default to the pinned version from constants when no version is specified + version := constants.DefaultPlaywrightVersion + if args.ImageVersion != "" { + version = args.ImageVersion + } + yaml.WriteString(" \n") yaml.WriteString(" [mcp_servers.playwright]\n") yaml.WriteString(" command = \"npx\"\n") yaml.WriteString(" args = [\n") - yaml.WriteString(fmt.Sprintf(" \"@playwright/mcp@%s\",\n", constants.DefaultPlaywrightVersion)) + yaml.WriteString(fmt.Sprintf(" \"@playwright/mcp@%s\",\n", version)) yaml.WriteString(" \"--output-dir\",\n") yaml.WriteString(" \"/tmp/gh-aw/mcp-logs/playwright\"") if len(args.AllowedDomains) > 0 { diff --git a/pkg/workflow/mcp_config_refactor_test.go b/pkg/workflow/mcp_config_refactor_test.go index 7f7c57d54c0..137e9b3a691 100644 --- a/pkg/workflow/mcp_config_refactor_test.go +++ b/pkg/workflow/mcp_config_refactor_test.go @@ -3,6 +3,8 @@ package workflow import ( "strings" "testing" + + "github.com/githubnext/gh-aw/pkg/constants" ) // TestRenderPlaywrightMCPConfigWithOptions verifies the shared Playwright config helper @@ -48,7 +50,7 @@ func TestRenderPlaywrightMCPConfigWithOptions(t *testing.T) { `"playwright": {`, `"command": "npx"`, `"args": [`, - `"@playwright/mcp@latest"`, + `"@playwright/mcp@` + constants.DefaultPlaywrightVersion + `"`, `"--output-dir"`, `"/tmp/gh-aw/mcp-logs/playwright"`, ` },`, @@ -269,7 +271,7 @@ func TestRenderPlaywrightMCPConfigTOML(t *testing.T) { `[mcp_servers.playwright]`, `command = "npx"`, `args = [`, - `"@playwright/mcp@latest"`, + `"@playwright/mcp@` + constants.DefaultPlaywrightVersion + `"`, `"--output-dir"`, `"/tmp/gh-aw/mcp-logs/playwright"`, }, diff --git a/pkg/workflow/mcp_config_shared_test.go b/pkg/workflow/mcp_config_shared_test.go index fe9ae9ff3d7..1a25c8740d2 100644 --- a/pkg/workflow/mcp_config_shared_test.go +++ b/pkg/workflow/mcp_config_shared_test.go @@ -3,6 +3,8 @@ package workflow import ( "strings" "testing" + + "github.com/githubnext/gh-aw/pkg/constants" ) // TestRenderPlaywrightMCPConfigShared tests the shared renderPlaywrightMCPConfig function @@ -23,7 +25,7 @@ func TestRenderPlaywrightMCPConfigShared(t *testing.T) { wantContains: []string{ `"playwright": {`, `"command": "npx"`, - `"@playwright/mcp@latest"`, + `"@playwright/mcp@` + constants.DefaultPlaywrightVersion + `"`, `"--output-dir"`, `"/tmp/gh-aw/mcp-logs/playwright"`, `"--allowed-origins"`, @@ -50,7 +52,7 @@ func TestRenderPlaywrightMCPConfigShared(t *testing.T) { wantContains: []string{ `"playwright": {`, `"command": "npx"`, - `"@playwright/mcp@latest"`, + `"@playwright/mcp@` + constants.DefaultPlaywrightVersion + `"`, }, wantEnding: "},\n", }, diff --git a/pkg/workflow/mcp_servers.go b/pkg/workflow/mcp_servers.go index 2334168064d..9de4c7a9b07 100644 --- a/pkg/workflow/mcp_servers.go +++ b/pkg/workflow/mcp_servers.go @@ -320,7 +320,8 @@ func getGitHubAllowedTools(githubTool any) []string { } func getPlaywrightDockerImageVersion(playwrightTool any) string { - playwrightDockerImageVersion := "latest" // Default Playwright Docker image version + // Default to empty string - caller will use pinned version when empty + playwrightDockerImageVersion := "" // Extract version setting from tool properties if toolConfig, ok := playwrightTool.(map[string]any); ok { if versionSetting, exists := toolConfig["version"]; exists { diff --git a/pkg/workflow/playwright_version_latest_test.go b/pkg/workflow/playwright_version_latest_test.go new file mode 100644 index 00000000000..f5fc5486a47 --- /dev/null +++ b/pkg/workflow/playwright_version_latest_test.go @@ -0,0 +1,55 @@ +package workflow + +import ( +"strings" +"testing" +) + +// TestPlaywrightExplicitLatestVersion verifies that when user explicitly sets version: "latest" +// it is respected and used in the rendered output +func TestPlaywrightExplicitLatestVersion(t *testing.T) { +t.Run("Explicit latest version for Copilot engine", func(t *testing.T) { +var yaml strings.Builder +playwrightTool := map[string]any{ +"version": "latest", +"allowed_domains": []string{"example.com"}, +} + +renderPlaywrightMCPConfigWithOptions(&yaml, playwrightTool, false, true, true) +output := yaml.String() + +if !strings.Contains(output, "@playwright/mcp@latest") { +t.Errorf("Expected @playwright/mcp@latest when user explicitly sets version: latest, got: %s", output) +} +}) + +t.Run("Explicit latest version for Claude engine", func(t *testing.T) { +var yaml strings.Builder +playwrightTool := map[string]any{ +"version": "latest", +"allowed_domains": []string{"example.com"}, +} + +renderPlaywrightMCPConfigWithOptions(&yaml, playwrightTool, false, false, false) +output := yaml.String() + +if !strings.Contains(output, "@playwright/mcp@latest") { +t.Errorf("Expected @playwright/mcp@latest when user explicitly sets version: latest, got: %s", output) +} +}) + +t.Run("Explicit latest version for Codex engine (TOML)", func(t *testing.T) { +var yaml strings.Builder +playwrightTool := map[string]any{ +"version": "latest", +"allowed_domains": []string{"example.com"}, +} + +renderPlaywrightMCPConfigTOML(&yaml, playwrightTool) +output := yaml.String() + +if !strings.Contains(output, "@playwright/mcp@latest") { +t.Errorf("Expected @playwright/mcp@latest when user explicitly sets version: latest, got: %s", output) +} +}) +} diff --git a/pkg/workflow/version_field_test.go b/pkg/workflow/version_field_test.go index 64962d7ee4e..ffd75f00920 100644 --- a/pkg/workflow/version_field_test.go +++ b/pkg/workflow/version_field_test.go @@ -33,7 +33,7 @@ func TestVersionField(t *testing.T) { // Test Playwright tool version extraction t.Run("Playwright version field extraction", func(t *testing.T) { - // Test "version" field + // Test "version" field with specific version playwrightTool := map[string]any{ "allowed_domains": []any{"example.com"}, "version": "v1.41.0", @@ -43,13 +43,23 @@ func TestVersionField(t *testing.T) { t.Errorf("Expected v1.41.0, got %s", result) } - // Test default value when version field is not present + // Test explicit "latest" version is respected + playwrightToolLatest := map[string]any{ + "allowed_domains": []any{"example.com"}, + "version": "latest", + } + result = getPlaywrightDockerImageVersion(playwrightToolLatest) + if result != "latest" { + t.Errorf("Expected latest (user explicitly set it), got %s", result) + } + + // Test default value when version field is not present (empty string, caller uses pinned version) playwrightToolDefault := map[string]any{ "allowed_domains": []any{"example.com"}, } result = getPlaywrightDockerImageVersion(playwrightToolDefault) - if result != "latest" { - t.Errorf("Expected default latest, got %s", result) + if result != "" { + t.Errorf("Expected default empty string, got %s", result) } })