E2e Reviewer

E2E Test Scenario Quality Review

Systematic checklist for reviewing E2E spec files AND Page Object Model (POM) files. Covers Playwright and Cypress with full grep + LLM analysis. General principles (name-assertion alignment, missing Then, YAGNI) apply to any framework, but automated grep patterns are Playwright/Cypress-specific.

Reference:

• Playwright best practices: https://playwright.dev/docs/best-practices
• Cypress best practices: https://docs.cypress.io/app/core-concepts/best-practices

Phase 0: Framework Detection

Before running checks, determine the framework by grepping for actual import statements in .ts/.js files:

• @playwright/test → Playwright
• cypress (as a module import or cy. call) → Cypress

Do NOT use these as signals:

• nx.json "e2eTestRunner" field — a generator-default that routinely outlives the runner's actual removal; trust imports, not config
• package-lock.json cached transitive deps — Cypress can appear in lockfile long after removal
• .spec.ts filename alone — could be Jest/Vitest unit tests, not Playwright/Cypress E2E

When .spec.ts files exist without @playwright/test or cy. imports, inspect 1-2 of them: presence of TestBed/describe() + it() without page.goto/cy.visit indicates Jest unit tests → out of e2e-reviewer scope.

Skip framework-irrelevant checks: If Playwright, skip Cypress-specific greps (#9b cy.wait(ms), #3b Cypress uncaught:exception). If Cypress, skip Playwright-specific greps (#8a dangling page.locator, #10b describe.serial, #15 missing await on expect, #16 missing await on action, #17 direct page action API, #18 expect.soft overuse). This eliminates noise in Phase 1 output.

---

Phase 1: Mechanical Scan

Run the bundled scanner against the test directory:

bash /scripts/scan.sh 

` is the directory shown in the Skill tool's "Base directory" output (e.g., ~/.claude/skills/e2e-reviewer/). Auto-detect from project structure (common: e2e/, tests/, __tests__/, spec/, cypress/e2e/`).

The scanner internally uses, in priority order:

1. eslint-plugin-playwright / eslint-plugin-cypress — when locally installed in the target project (AST-based, most accurate, lowest FP rate)
2. ast-grep — Tree-sitter-backed for the FP-prone assertion patterns (#15 missing-await, #4c-4e one-shot state/text/count, #4f Locator-as-truthy)
3. ripgrep regex — universal fallback covering all remaining patterns

Output is grouped per pattern ID (#3, #4a, #15, etc.) with file:line:matched-line. See references/grep-patterns.md for the meaning of each ID.

Companion CI plugins (recommend when relevant). The mechanical always-pass class (#4f — expect(locator).toBeDefined() / .toBeTruthy() / .not.toBeNull()) is also shipped as standalone, autofixable ESLint rules: eslint-plugin-playwright-silent-pass and eslint-plugin-cypress-silent-pass. This review is agent-time (on-demand); those rules are commit/CI-time enforcement (eslint --fix). When a project shows #4f hits, recommend installing the matching plugin so that slice is caught deterministically on every commit — leaving this skill to focus on the semantic patterns no AST rule can decide.

Tier scoping note: Tier 2's sg-4f deliberately also matches RTL getBy*().toBeTruthy() in unit tests — that surface gets the jest-dom canonical fix from 4.1, not a P0 label. Severity classification of #4f stays with Phase 2 (Locator subject = P0; RTL = advisory). Tier 2 rules skip vendored/build artifacts via per-rule ignores.

Deterministic mode (cross-host convergence contract): different hosts (Claude Code, Codex, etc.) must produce comparable findings on the same repo. Tier 1/2 availability varies with the environment (local plugin installs, npx download policy, watchdog), which changes the raw hit set. For a comparable review, invoke the scanner in the canonical form and SAY SO in the report:

E2E_SMELL_NO_ESLINT_DOWNLOAD=1 E2E_SMELL_NO_AST_GREP_DOWNLOAD=1 bash /scripts/scan.sh 

(Tier 3 regex always runs and is the deterministic baseline; Tier 1/2 add precision when locally installed but never subtract findings — the exit-code gate guarantees a crashed tier cannot suppress Tier 3.) The report MUST state which tiers actually ran ("Tier coverage: 3 only" / "1+2+3").

E2E content scoping: for the FP-prone patterns (P0: #3, #4a, #4b, #4f, #4g, #15; P1: #9, #6, #5b, #19) the Tier 3 regex keeps a hit only when its file carries a real Playwright/Cypress marker (@playwright/test import, async ({ page fixture destructure, direct page. usage, or cy.(). This filters Vitest/Jest/RTL unit-test bleed-through at Phase 1 — the dominant false-positive source observed across a 110-repo OSS validation corpus.

Evidence rule: scanner hits are mechanical review signals. Report exact matches, then use Phase 2 where the rule requires intent or project context.

Suppression — // JUSTIFIED:: a hit is intentional and must be skipped when // JUSTIFIED: appears in any of these positions (exception: #7 Focused Test Leak has no exemption):

1. The line immediately preceding the hit
2. The line immediately preceding the enclosing call/block when the hit is inside a callback body — e.g., // JUSTIFIED: above page.evaluate(() => { … document.querySelector(…) … }) or page.waitForFunction(() => { … }) covers every qualifying pattern inside that callback
3. For chained calls split across lines (page.locator(…)\n .filter(…)\n .first()), the line immediately preceding the chain's starting expression covers .nth() / .first() / .last() further down the chain

Phase 2 also recognizes these as JUSTIFIED-equivalent (informal):

• // eslint-disable-next-line -- with concrete reason
• Author rationale comments above the hit (signals intentional vs accidental — see 4.2 band-aid awareness)
• Comments describing dual-mode UI handlers (e.g., // Single workspace mode — no workspace selection above if (await x.isVisible()) indicates intentional dual-mode, not a band-aid)

Comment / string-literal false positives (mostly handled by ast-grep and eslint when available; remaining ones for Phase 2 LLM):

• Trailing // comment on a code line — token in code triggers, comment is noise
• Block comment /* … { timeout: 0 } … */ containing the token
• String literal containing the token (e.g., "test.only('focused', ...)" in a meta-test for the rule itself)
• Same token in a different language API (e.g., Node fs.rm(path, { force: true }))

try/catch wrapping in spec files (#3 partial) requires LLM judgment (Phase 2) — too many legitimate uses to scan reliably.

---

Phase 2: LLM Review (Semantic And Context Checks Only)

Patterns already detected in Phase 1 (#3 partial, #4, #5, #6, #7, #8, #9, #10 partial, #14, #15, #16, #17, #18, #19, #3b) are skipped unless they need LLM confirmation. The LLM performs only these checks:

| # | Check | Reason | |---|-------|--------| | 1 | Name-Assertion Alignment | Requires semantic interpretation | | 2 | Missing Then | Requires logic flow analysis | | 3 | Error Swallowing — try/catch in specs | Too many legitimate non-test uses; requires reading context | | 4 | Always-Passing — .toBeTruthy() confirmation | Phase 1 flags all .toBeTruthy() hits; LLM confirms which ones have a Locator subject (P0) vs. a legitimate boolean variable (OK). Do NOT re-report other #4 sub-patterns already covered in Phase 1. | | 4c-4e | One-shot state — Locator-subject confirmation | Phase 1 flags expect(await x.isVisible()/isDisabled()/textContent()/inputValue()/...). LLM confirms x is a Playwright Locator/Page, NOT a custom service or helper method. False positive examples: expect(await myService.isEnabled()).toBe(true) (custom service), expect(await checkSessionValid(page)).toBe(true) (helper returning Promise). Flag P0 only when subject is a Locator/Page. | | 8 | Missing Assertion — Cypress dangling selectors | cy.get(...) standalone requires manual check | | 8a | Multi-line continuation skip | Phase 1 applies a previous-line continuation filter at scan time: a hit is dropped when the preceding non-blank line ends with ( or , (an argument inside a multi-line await expect(\n page.locator(...)\n)…, not a dangling statement). Semicolonless dangling locators are still detected. As a backstop, LLM SKIPS any residual hit with that same previous-line shape. | | 4b | toBeAttached() static-shell confirmation | Phase 1 flags positive toBeAttached(). P0 (vacuous) ONLY when the element is part of the static page shell that is always present. SKIP when the element is dynamically injected / conditionally rendered for the scenario under test (e.g. an expired-license banner, a just-registered block, a ` added at runtime) — then the assertion can genuinely fail and is meaningful. Scanner #4b hits arrive tagged [LLM-TRIAGE]: confirm a hit is a persistence assertion after a destructive action before reporting it P0 — generic render-gates on client-rendered elements are FPs (field data from one client-rendered-canvas OSS repo: 22/25 raw hits were FPs). | | 5a | Conditional gates action vs assertion | Phase 1 flags if (await x.isVisible()). SKIP when the if-body contains **no expect()** — it gates a setup/navigation action (open a menu, dismiss a drawer, dual-mode UI handler) and the test still has unconditional assertions afterward. Flag P0 only when an expect() lives inside the conditional, so the assertion runs zero times when the branch is false (silent pass). test.skip(reason) is always intentional — never flag. | | 10 | Flaky Test Patterns | For each grep hit that has // JUSTIFIED:, verify the rationale is concrete (e.g. "server returns in fixed order") rather than vague ("needed for now"); flag if the comment doesn't actually justify the position-coupling or serial dependency. Skip if no JUSTIFIED comment — Phase 1 already flagged. | | 11 | YAGNI in POM + Zombie Specs | Requires usage grep then judgment | | 12 | Missing Auth Setup | Spec navigates to protected routes (/dashboard, /settings, /admin, etc.) without preceding login, storageState, or auth beforeEach. Flag P0 — tests will hit login redirects. | | 13 | Inconsistent POM Usage | POM is imported but spec bypasses it with raw page.fill/page.click for operations the POM should encapsulate. Flag P1. | | 15 | Missing await on expect() confirmation | Phase 1 flags lines that start with expect( (no leading await). LLM confirms the subject is a Playwright Locator / Page — non-Locator expects like expect(count).toBe(3) don't need await. Flag P0 only when the subject is a Locator/Page. | | 16 | Missing await on action confirmation | Phase 1 flags lines that start with page.locator(...).action( or page.getBy...(...).action( (no leading await). LLM confirms the line lacks await and the action is a real Playwright action (not a synchronous chain). LLM also SKIPS the hit if the line is inside a Promise.all([ or Promise.race([ array — array elements don't need explicit await because the Promise.all awaits them. Flag P0 only for true standalone statements. | | 18 | expect.soft() overuse confirmation | Phase 1 flags all expect.soft() hits; LLM counts: if >50% of assertions in a single test are soft, flag P1 — soft assertions mask cascading failures. A few soft assertions among many hard ones is fine. | | 19 | Module-level mutable state confirmation | Phase 1 flags every ^let at column 0 in test code. LLM SKIPS the hit when it's a pure type declaration without an initializer (e.g., let page: Page; reassigned in beforeEach — idiomatic Playwright fixture). Flag P1 only when the let carries an initializer (let counter = 0;, let cache: Map = new Map();`) — that state survives across tests under parallel workers and retries. |

LLM-only write-path checks (#20–#23) — run on EVERY review; no grep signal exists. These four patterns never appear in Phase 1 output, so nothing mechanical drives them — execute each procedure here regardless of scanner hit counts (full contracts in references/pattern-reference.md):

| # | Check | Sev | Detection procedure | |---|-------|-----|---------------------| | 20 | Unmocked Real-Backend Writes | P1 | In each spec, list actions that submit forms or trigger mutation-shaped requests (signup/login/checkout/save/delete). For each, verify a route stub (page.route() / cy.intercept()) or mock fixture covers the endpoint — read helper and fixture files before flagging; the stub may live there. Client-side-only validation tests (no request fired) are not hits. Exemption: one clearly named real-backend smoke spec marked // JUSTIFIED: designated real-backend smoke. | | 21 | Manual Session-File Dependency | P2 | For each storageState: reference (spec, fixture, or playwright.config project), trace what writes that path. Flag when only a manual capture script — or nothing in-repo — produces it. A committed/manually captured file is acceptable only as a cache with a programmatic fallback (API-login helper or setup project). | | 22 | Optimistic UI Without Call Proof | P1 | For each test that clicks a write control (toggle/delete/save — read the component if unsure whether the handler issues a mutation), check the spec awaits request evidence: page.waitForRequest(), a route-handler hit flag, or mocked-request capture. Flag when the only assertions are DOM/UI state the component updates optimistically. Tests of pure client-side state (no request in the handler) are not hits. | | 23 | Fixture Ignores Render Guards | P2 | For each fixture consumed by a list/card component, open the component and collect conditions that suppress rendering (early return null, .filter(), .slice()). Cross-check fixture field values against them. Flag mismatches, and flag negative assertions (toHaveCount(0), empty-state checks) whose truth could come from a guard-suppressed render rather than the intended state. |

Zero-P0 floor (MANDATORY): Phase 1 reporting 0 P0 does NOT end the review. The LLM-only checks (#1 Name-Assertion, #2 Missing Then, #3 try/catch shapes, #12 Missing Auth, and the #20–#23 write-path checks above) run regardless of mechanical hit counts — multi-line shapes the regexes miss (e.g. blanket multi-line cy.on('uncaught:exception') suppressors) have carried a suite's entire P0 surface.

Bounded opening-token sweep (MANDATORY, exactly this list — no more, no less): for cross-host convergence the scanner-missed-shape sweep is a fixed checklist, not open-ended exploration. For each P0 family whose Phase 1 count is 0, grep the family's opening token and read the bodies of any matches:

| Family | Opening token grep | |--------|--------------------| | #3b | cy\.on\(\s*['"]uncaught:exception | | #3 | catch\s*[({] in spec files (bodies that swallow without rethrow/assert) | | #7 | \.only\( | | #8b | ^\s*await .*\.is[A-Z][a-zA-Z]*\( standalone statements | | #4h | expect\(\s*page\.url\(\) |

A zero on both the scanner AND its family token = genuinely clean; stop there.

Counting contract — Real P0 = N (MANDATORY definition): N is the number of DISTINCT flagged source lines (file:line) that survive Phase 2 false-positive elimination, after the consolidation rule (a line triggering multiple patterns counts ONCE). Do not count clusters, files, or pattern categories; do not count P1/P2 findings; do not count findings in framework self-test fixtures separately — include them in N but label them per 4.2-9. Two hosts reviewing the same commit must arrive at the same N.

Retry-wrapper skip (applies to #4c-4e, #4h, #15, #16): When a Phase 1 hit's enclosing function

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: voidmatcha
• Source: voidmatcha/e2e-skills
• License: Apache-2.0
• Homepage: https://www.skills.sh/voidmatcha/e2e-skills

Install and usage instructions live in the source repository linked above.