review-tests

What this skill does

# Tests Review

Review tests in the specified path for quality issues.

> [!IMPORTANT]
> Consult [REFERENCE.md](REFERENCE.md) for the expected output format and level of detail.

## Scope

Determine the review scope before discovering files:

- If `$ARGUMENTS` is non-empty, treat it as a path (file or directory) and run:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh "$ARGUMENTS"
  ```
- If `$ARGUMENTS` is empty, scope to files added or modified on the current branch relative to the default branch:
  ```bash
  ${CLAUDE_PLUGIN_ROOT}/scripts/discover-files.sh
  ```

Handle the script's exit codes:
- **0 with output** — use the listed paths as input to the discovery step below.
- **0 with empty output** — branch has no diff vs the default branch. Tell the user and ask which path to review.
- **non-zero** — script prints a message to stderr (path not found, not a git repo, on the default branch with no path, detached HEAD, or default branch indeterminate). Relay the message and ask the user which path to review.

The script returns paths language-blind. The discovery step below filters to test files; if the filter matches nothing but the script's output was non-empty, the language may not be in the pattern list — apply judgment to identify test files in the output.

## Workflow

### Step 1 — Discover test files

From the script's output, filter to test files using language-appropriate patterns: `*_test.go`, `test_*.py`, `*_test.py`, `*.test.ts`, `*.test.js`, `*.spec.ts`, `*.spec.js`, `__tests__/**`, etc. Record the full file list and count.

### Step 2 — Choose execution strategy

- **1–2 files → Direct mode**: Read the files, evaluate against the Quality Criteria below, attach a short pattern label to each finding (same scheme as parallel mode — see Parallel Review Mode → Spawn subagents, item 7), then proceed to Pattern Collapsing.
- **3+ files → Parallel mode**: Batch files, spawn subagents, collect results, merge, then proceed to Pattern Collapsing.

### Parallel Review Mode

Use this mode when 3 or more test files are discovered.

#### Batching

Group files into batches based on total file count:

| Total files | Files per batch | ~Subagents |
|-------------|-----------------|------------|
| 3–10        | 1               | 3–10       |
| 11–20       | 2               | 6–10       |
| 21+         | 3               | 7–10       |

#### Spawn subagents

For each batch, use `Agent(subagent_type="general-purpose")`. **Spawn all subagents in a single message** so they run in parallel.

Each subagent prompt MUST include:

1. The file paths in its batch (instruct the subagent to read them)
2. The **Quality Criteria** section from this skill — copy it verbatim into the prompt
3. The **Severity** section from this skill — copy it verbatim into the prompt
4. The structured output format below
5. The explicit instruction: **"Do NOT use the Bash tool. Do NOT run any shell commands. Use only Read, Grep, and Glob tools. Return findings only."** — the review is static analysis of test files, so shell access adds latency and side-effect risk without enabling anything the read-only tools can't already do.
6. The explicit instruction: **"For every P2 and P3 finding, you MUST state a concrete falsifiability claim in the `explanation` field: 'if \<specific production change\> were made, this test would still incorrectly pass.' Omit findings that lack this claim. Two exceptions: P1 tautological tests (the claim is implicit — no production change can fail the test) and unclear-test-name findings (judged on readability, not falsifiability)."**
7. The explicit instruction: **"For the `pattern` field, use a short, reusable label that names the underlying anti-pattern (e.g., 'module-scope mutable mocks', 'tautological assertions'). If two findings in your batch stem from the same root cause, they MUST use the same pattern label."**

Instruct each subagent to return findings in this exact delimited format (one block per finding):

```
---FINDING---
priority: P<1|2|3>
location: <file:line>
title: <short title>
category: <Completeness|Usefulness|Coverage Gaps|Output Validation|Isolation|Readability|Integration Test Specifics>
pattern: <short label for the underlying anti-pattern, e.g. "module-scope mutable mocks" or "tautological assertions" — use the SAME label across findings that share the same root cause>
explanation: <what is wrong or missing and why it matters>
fix: <concrete prescription>
done_when: <verifiable criterion>
---END---
```

If the subagent finds no issues for its batch, it should return `---NO-FINDINGS---`.

#### Collect and merge

After all subagents return:

1. Parse each subagent's structured findings
2. Combine into a single list, sorted by priority (P1 first)
3. Deduplicate: if two findings share the same `location` (file:line) AND the same `category`, keep only the one with the highest priority
4. Group findings by `pattern` label — findings from different subagents that used the same (or very similar) pattern label share a root cause and will be collapsed in the Pattern Collapsing step

#### Error fallback

If a subagent fails or returns unparseable output, review those files directly (as in direct mode) and include a note in the report: `Note: Files [list] were reviewed directly due to subagent failure.`

### Pattern Collapsing

Both direct mode and parallel mode flow into this step before producing the final report.

After merging all findings, look for findings that share the **same root cause** — i.e., the same testing anti-pattern repeated across multiple test files. Examples:

- Multiple test files flagged for "module-level mutable mock leaks between tests" → one pattern: "test suite uses module-scope mocks instead of per-test setup"
- Multiple test files flagged for "globalThis.fetch replaced at module scope" → one pattern: "fetch mocking is done at import time instead of in beforeEach"
- Multiple test files flagged for "assertions only check error status, not return values" → one pattern: "tests validate calls were made but not results returned"

When you identify a shared root cause:

1. **Collapse** the N per-file findings into **one finding** that names the pattern, lists all affected files, and prescribes the codebase-wide fix
2. **Set severity** to the highest severity among the collapsed findings
3. **Keep separate** any findings that happen to share a category but have genuinely different root causes

This is critical: N findings for N instances of the same pattern creates noise. One finding that names the pattern and lists the affected locations is actionable.

## Quality Criteria

### Completeness
- Edge cases covered
- Error paths tested
- Boundary conditions checked
- Happy path and failure scenarios both present

### Usefulness
- Tests validate behavior, not implementation details
- Tests would fail if the code broke
- High coverage alone is not proof of quality — a tautological test covers lines without catching regressions

### Coverage Gaps
- Production code paths in the review scope should not ship with zero test coverage
- Use the asymmetry: **zero coverage is a strong signal of a real gap; high coverage is a weak signal of quality.** Gap findings carry a built-in falsifiability claim — "no test exercises this code, so a regression here would ship silently." Severity, however, follows export status (see the Severity section): exported/public zero-coverage is **P2**; internal/private zero-coverage is **P3**.
- Distinguish genuine gaps from indirect coverage. A function is not a gap if it is exercised transitively by a higher-level test (handler test that flows through a service). Prefer file- and module-level gaps ("this source file has no test that imports or exercises it") over symbol-level grep, which is too noisy in layered code.
- When a language-standard coverage tool has already been run and a report is available, trust it over static heuristics — it correctly recognizes indirect coverage.

### Output Vali