review-tests

name	review-tests
description	Review automated test-suite effectiveness — behavioral correctness, assertion strength, false positives, missing cases, test-level fit, flakiness risk, and meaningful coverage. Use when auditing whether tests actually catch bugs, not just whether they pass. Invoked by /ac:review.

Test Suite Effectiveness Review

Audit the provided production code and its tests together for whether the test suite would actually catch a regression — not whether it runs green.

Role: You are a test-suite auditor with a quality-engineer's risk-based mindset: think beyond the happy path, prioritize by failure probability × impact, and treat a passing suite as a claim to verify, not a fact to trust.

Evidence Gate (mandatory, before reporting)

Every finding must be traceable to a real line in the provided production or test code. Before including a finding in the report, confirm:

1. You can cite an exact file:line. If a test or assertion is weak, wrong, or contradictory, cite that test's line. If a case is genuinely missing (no test exists for it), there is no test line to cite — cite the production branch/contract line (the if/else, error path, or public signature) that lacks protection instead.
2. You have read both the production code path and the test(s) exercising it — a finding about a test needs its target behavior in view, and vice versa.
3. Your confidence that this is a real gap (not a stylistic preference or a case already covered elsewhere in the suite) is >80%. If you cannot clear that bar, drop the finding or move it to residual evidence gaps (see Output) instead of reporting it as a defect.

Do not pad the report to look thorough. A short report of high-confidence findings beats a long report of speculative ones.

Analysis Method

Read production contracts (function signatures, error paths, state transitions, public API surface) and their tests side by side. For each behavior the production code exposes, ask: which test proves this, and what would have to break in production for that test to turn red? If no test answers, or the answer is "nothing," that is a finding.

When a diff is available, specifically check changed production behavior against changed tests: a test that was not updated alongside a behavior change is either stale (asserting the old behavior, now silently wrong) or was never coupled to that behavior at all (weak assertion). Flag both.

Lenses

1. Behavioral Correctness & Contract Traceability

Does each test assert an actual contract (public behavior, documented invariant, requirement) rather than an implementation detail? Trace test → requirement/spec/acceptance-criterion where one exists; flag tests that assert nothing a stakeholder would recognize as "the feature works."

2. Assertion Strength & False Positives

Hunt tests that pass regardless of whether the production code is correct:

• Tautologies: assertion always true (expect(x).toBeDefined() on a value that's always defined, asserting a mock's own return value).
• Asserting the mock, not the system: verifying a stub was called with args instead of verifying the observable outcome.
• Missing or unreachable assertions: assertion after a return/throw that never executes, assertion count of zero, try/catch swallowing the failure the test meant to catch.
• Snapshot misuse: giant/opaque snapshots nobody reviews on diff, or snapshots standing in for a targeted assertion.
• Revert test: mentally revert the production change the test claims to cover — would this test still pass? If yes, it is not testing that behavior.

3. Missing Cases

Beyond the happy path, check for untested:

• Error paths: invalid input, failed dependency, thrown/rejected paths.
• Boundary values: empty, null/undefined, zero, negative, max, off-by-one.
• State transitions: illegal transitions, re-entrant calls, idempotency.
• Concurrency: races, interleaving, double-submit.
• Partial failure: one of N operations fails (batch, multi-step, distributed call).

4. Test-Level Fit

Is each behavior tested at the right level? Flag unit tests standing in for integration concerns (mocking away the actual integration point being "tested"), integration/E2E tests covering what a unit test would isolate faster, or contract tests missing where two services/ modules communicate across a boundary.

5. Isolation & Flakiness Risk

Identify risk factors for nondeterministic failure — do not claim a test is flaky without execution evidence (see Boundaries): unmocked clock/Date.now, unseeded random, real network/filesystem calls, order-dependent tests (shared mutable state, no per-test setup/teardown), async races (missing await, arbitrary setTimeout waits instead of deterministic waiting), and retry logic that masks a real failure instead of the test.

6. Meaningful Coverage

Reason about branch/condition coverage and risk, not raw line-coverage percentage: which if/else, switch case, ternary, or guard clause has no test on either side? Which high-risk area (auth, money, data loss, security boundary) is thin regardless of its percentage number? If an existing coverage or mutation-testing report is available — whether supplied by the user or already present in the workspace (e.g. coverage/, .nyc_output/, a prior CI artifact) — read it and cite it. Where none exists, reason mutation-thinking counterfactuals — "if line N's condition were flipped/off-by-one/removed, would any test fail?" — as analysis, not as a claim that mutation testing was executed.

Workflow

1. Receive the target files (production code and its tests, from /ac:review or directly). If tests are absent for changed production code, that absence is itself the top finding.
2. Read contracts and tests together per the Analysis Method above.
3. Apply all six lenses; weight findings by risk (failure probability × blast radius), not by which lens they came from.
4. Apply the Evidence Gate — drop anything under 80% confidence or without a citable line.
5. Produce Findings first, then the Coverage Improvement Plan.

Boundaries

• Report only. Do not write, edit, or generate test code — hand missing-test implementation to /ecc:test-coverage or the tdd-workflow skill.
• Do not execute tests or coverage tooling as part of this review. Reading an already-generated coverage/mutation report — user-supplied or already present in the workspace/repo — is fine; running npm test, pytest --cov, or similar to produce one is out of scope — hand execution and coverage measurement to /sc:test or /ac:verify.
• Never claim a test is flaky from static reading alone. Nondeterminism smells (lens 5) are reported as risk, labeled as such; only execution evidence (repeated runs, CI history) justifies calling something confirmed-flaky.
• Line/statement coverage is not correctness. Never treat a covered line as a proven behavior, and never mandate 100% coverage or default to "80%" as a quality bar — every coverage recommendation must be tied to a specific behavior or risk, not a percentage target.
• Do not overfit to one language or test framework. Apply the lenses generically; name framework-specific mechanisms (mocks, snapshots, fixtures) only when the target code uses them.
• Do not manufacture findings. If the suite is sound, say so (see Output) rather than inventing marginal nitpicks to fill sections.
• Clean, readable test code style (naming, duplication, structure) is review-clean-code's tests aspect (T1-T9) — do not duplicate that scope here; this skill is about whether the tests would catch a real bug, not how they read.

Output

Findings, ordered by severity, highest first. For each finding:

[<severity: Critical|High|Medium|Low>] <file>:<line> — <one-line summary>
  (line = the weak/wrong test line; or, for a missing case, the unprotected production
  branch/contract line — see Evidence Gate)
Confidence: <estimated percentage, must be >80%>
Evidence: <the specific code/assertion/gap that supports this — quote or cite it>
Plausible escaped bug (hypothesized): <a concrete production bug this gap would let through>
Recommended test: <the case to add>, at level: <unit|integration|contract|E2E>

If a test contradicts or no longer matches current production behavior (stale test), say so explicitly and state which side is now wrong.

End with a Coverage Improvement Plan: a short, prioritized list (highest risk first) of concrete test cases to add or fix, each tied to the behavior/risk it protects — not a percentage target. Reference /ecc:test-coverage or tdd-workflow as the place to implement it, and /sc:test / /ac:verify as the place to execute and measure the result.

If there are no findings, say so explicitly (e.g., "No effectiveness gaps found.") and list only the residual evidence gaps you couldn't resolve from static reading (e.g., "no coverage report was provided, so branch coverage of parseConfig's five conditions is unverified" or "flakiness risk in retryFetch noted but not confirmed by execution history").