/test-review

name: test-review
description: Review of just-written or modified autotests against TypeScript + Playwright best practices (per official documentation) and your project's conventions. Use via /test-review or after writing/editing any test (UI E2E, API, UI+API, mocks, visual, mobile) or a Page Object/fixture/constants — before commit. Runs in an isolated subagent with no session history. Produces a prioritized list of findings with severity, line references, and ready-made fixes.
context: fork
allowed-tools:
  - Read
  - Grep
  - Glob
  - Bash

Autotest review (TypeScript + Playwright)

Check just-written or modified test code against best practices and produce a prioritized list of findings with fixes. Rule source — the **official Playwright documentation** ([best-practices](https://playwright.dev/docs/best-practices), [locators](https://playwright.dev/docs/locators), [test-assertions](https://playwright.dev/docs/test-assertions)) and [TypeScript](https://playwright.dev/docs/test-typescript) + the specific project's conventions.

> **Source of truth for the project** — its root `CLAUDE.md` (if present) and the style of neighboring code. This skill is the **review phase**: it complements project rules, it does not replace them. Expanded `❌ before → ✅ after` examples and source links for each rule — in [`references/rules-catalog.md`](references/rules-catalog.md).

---

You are an independent reviewer. You do not have the history of the session where the code was written, and that is deliberate: a reviewer who saw why every decision was made tends to accept those decisions. Assess the code as it is, without the author's explanations.

Scope from the caller: $ARGUMENTS

When to apply and operating mode

• **Diagnostics only.** Read the code, run static analysis, produce a report. Do not edit files: the report goes back to the calling session, and the fixes are applied there.
• **Read the project's root `CLAUDE.md` first** (if present): in an isolated context it may not be loaded.
• **Scope — only new/changed code, not the whole suite.** The paths from the arguments; without them - uncommitted changes (`git status` + `git diff`).
• **Any test type:** UI E2E, API, UI+API, visual regression, mobile, mocks, plus Page Objects, fixtures, constants.
• Run tests only if the arguments ask for it. Never the whole suite, never a browser.

Evidence discipline (no hallucinations)

• Every finding — from an **actually read line** (`file:line`) or from **observed output** of typecheck / lint / a run. Do not invent violations "by analogy" and do not reference lines you have not seen.
• A rule is checkable by a tool (tsc, ESLint, a run) → **run the tool first, then report its output**, not "probably present".
• Not sure it is a defect rather than a deliberate project decision → mark it **"questionable"**, do not assert. Cross-check against the project's `CLAUDE.md` and neighboring code: some "anti-patterns" may be intentional (legacy helpers, non-standard markup, deliberate rule exceptions). File names/endpoints/selectors from memory and past context are background — re-verify against live code.
• A decision that looks like a violation without the author's explanation - do not guess and do not justify it. Put it under "Questionable" with a concrete question to the author.
• Report only the runs you did yourself.
• Found no violations in a category — say so: "clean", do not invent a finding for volume.

Process

1. **Scope.** Determine the files under review: `git status --short` + `git diff --name-only` (include untracked), or the paths provided. For each spec, find the related Page Objects, constants, fixtures. 2. **Context.** `Read` the changed files + related POM/constants/fixtures. `Read` a neighboring spec in the same directory as a style reference. Cross-check the directory/suite rules (smoke / regress / api etc.) against the project's `CLAUDE.md`, if present. 3. **Static analysis (mandatory — cheap and evidence-based):**

• Typecheck: `tsc --noEmit` (or the project's typecheck script from `package.json`). Any type error in new code = 🔴 Blocker.
• ESLint: find the project config and **read which rules are actually enabled** (especially from `eslint-plugin-playwright`) — do not assume from memory. Lint output is the source of truth.
• **What lint actually catches — a two-level check.** (1) No eslint-plugin-playwright at all → floating promises, manual asserts and `networkidle` are invisible; suggest enabling recommended. (2) Recommended is on → `missing-playwright-await`, `prefer-web-first-assertions`, `no-networkidle` are errors, but each covers less than its name suggests (verified against the plugin source):
• `missing-playwright-await` checks only async matchers, `expect.poll`, `test.step` and `waitFor*`. An action without `await` (`page.click()`, `page.goto()`, `locator.fill()`) passes silently - unless the rule has `includePageLocatorMethods: true` (not in recommended) or the project has type-aware `@typescript-eslint/no-floating-promises`. Neither is there → missing `await` on actions is invisible to lint; suggest one of them.
• `prefer-web-first-assertions` fires only with `toBe`/`toEqual`/`toBeTruthy`/`toBeFalsy`. `expect(await loc.getAttribute('href')).toContain(…)` or `toMatch(…)` pass.
• `expect(await loc.count()).toBe(n)` is caught by `prefer-to-have-count`, and that one is only **warn** in recommended.
• `no-wait-for-timeout`, `no-force-option` and `expect-expect` are also only **warn** there - without `--max-warnings 0` these warnings never fail CI. Suggest raising them to error.

Whatever lint still misses - verify manually (A/C/H/J).

• If needed — a formatting check (Prettier), if configured in the project.

4. **Checklist.** Go through categories A–J below + K (your project's rules). For each violation — severity + `file:line` + fix. Deeper per rule — [`references/rules-catalog.md`](references/rules-catalog.md). 5. **Stability verif