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) } })