Stop suggesting redundant Playwright CLI mode - #58041
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories (found 19).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this is a small, well-scoped fix that removes redundant mode: cli guidance now that Playwright is CLI-only, and updates docs/error messages/tests consistently.
📋 Notes
- ✅ New test
TestPiEngineAcceptsPlaywrightWithImplicitCLIModedirectly covers the changed behavior (Pi + omitted mode + CLI proxy), following red-green-refactor spirit. - ✅ Error message and doc updates are consistent across
playwright_validation.go,docs/reference/playwright.md, anddocs/engines/pi.md— no lingeringmode: clireferences left behind in the diff. - No actionable issues found; nothing to block on.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 16 AIC · ⌖ 17.1 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
Remaining contradictory guidance and insufficient Pi integration coverage prevent the stated migration update from being complete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates Playwright guidance to treat CLI as the sole built-in mode and removes redundant mode: cli examples.
Changes:
- Revises migration validation and documentation.
- Adds Pi implicit-CLI coverage.
- Updates workflow guidance and release notes.
File summaries
| File | Description |
|---|---|
pkg/workflow/playwright_validation.go |
Updates migration errors. |
pkg/workflow/playwright_validation_test.go |
Tests guidance and Pi behavior. |
docs/src/content/docs/reference/playwright.md |
Removes redundant mode examples. |
docs/src/content/docs/engines/pi.md |
Documents Pi Playwright support. |
.github/skills/agentic-workflows/SKILL.md |
Adds compatibility guidance routing. |
.github/aw/syntax-tools-imports.md |
Simplifies Playwright configuration. |
.changeset/patch-playwright-cli-only-guidance.md |
Records the patch change. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| Remove `mode: mcp`. The built-in integration is CLI-only, so no replacement | ||
| `mode` field is needed. The compiler now reports `mode: mcp` as an error. |
| func TestPiEngineAcceptsPlaywrightWithImplicitCLIMode(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| tools := map[string]any{ | ||
| "github": map[string]any{"mode": "gh-proxy"}, | ||
| "cli-proxy": true, | ||
| "playwright": map[string]any{}, | ||
| } | ||
|
|
||
| require.NoError(t, compiler.validatePiEngineRequirements(NewTools(tools), NewPiEngine())) | ||
| require.NoError(t, compiler.validatePlaywrightMode(&WorkflowData{Tools: tools})) | ||
| assert.True(t, isPlaywrightCLIMode(tools)) | ||
| } |
| - `.github/aw/charts-trending.md` | ||
| - `.github/aw/charts.md` | ||
| - `.github/aw/cli-commands.md` | ||
| - `.github/aw/compat.md` |
There was a problem hiding this comment.
This PR is mostly lean; the only simplification I’d still make is collapsing the one-off Pi-specific Playwright test into the existing validation coverage. net: -1 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58041 · codex · mai10 · 5.01 AIC · ⌖ 8.09 AIC · ⊞ 14K
Comment /ponytail to run again
| assert.Contains(t, err.Error(), "mcp-servers") | ||
| } | ||
|
|
||
| func TestPiEngineAcceptsPlaywrightWithImplicitCLIMode(t *testing.T) { |
There was a problem hiding this comment.
pkg/workflow/playwright_validation_test.go:106: yagni: one-off Pi-engine test for implicit CLI mode. Fold it into the existing TestValidatePiEngineRequirements/TestValidatePlaywrightMode cases to keep coverage local and avoid extra test surface.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review verdict
The docs cleanup is moving in the right direction, but the validator text now overstates the contract and contradicts behavior the compiler still accepts.
Blocking theme
The built-in Playwright integration is clearly CLI-only now, but this PR changes user-facing validation guidance from "mode: cli or omit mode" to "remove mode" without making the parser/validator reject explicit mode: cli everywhere. That leaves users with two incompatible stories depending on which file or error path they hit.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 25.4 AIC · ⌖ 7.39 AIC · ⊞ 23.5K
Comment /review to run again
| mode, | ||
| "mode must be a literal value; expressions are not allowed", | ||
| "Set mode to cli, or omit mode because CLI is the default", | ||
| "Remove mode because the built-in Playwright integration is CLI-only", |
There was a problem hiding this comment.
This guidance is now stricter than the implementation: validatePlaywrightMode still accepts explicit mode: cli, and the repo still contains examples/tests that rely on that accepted form, so telling users to always remove mode is misleading and will create contradictory migration advice.
💡 Align the message with the actual compatibility contract
Right now only mode: mcp and expression-valued mode are rejected. Explicit mode: cli remains valid, which means the validator should either:
- keep documenting both accepted forms (
mode: clior omitted), or - start rejecting
mode: clieverywhere and update the rest of the tree in the same change.
A message like "Omit mode because CLI is the default, though explicit mode: cli is still accepted for compatibility" would avoid sending users into a dead-end contradiction.
There was a problem hiding this comment.
Clean, focused change. Removes redundant mode: cli guidance consistently across docs, compiler validation messages, and the migration path, with a new test (TestPiEngineAcceptsPlaywrightWithImplicitCLIMode) covering the implicit-CLI-mode case. No correctness or security issues found in the changed lines.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 20.8 AIC · ⌖ 13.2 AIC · ⊞ 8.3K
|
@copilot Please address the current blocking review feedback, refresh the branch if needed, and run the
Please summarize what changed and what remains for maintainers once the follow-up is pushed.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please reconcile the remaining review feedback on this Playwright guidance PR before handing back to maintainers.
Please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Updated the compatibility story to match the implementation: omit |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed the failing sync check by restoring the missing |
|
🎉 This pull request is included in a new release. Release: |
Pi already supports the built-in CLI-only Playwright integration, but migration guidance suggested adding the redundant
mode: clisetting.Migration guidance
mode: mcprather than replace it.Pi coverage
modeis omitted.Documentation
mode: cliexamples.