--- name: audit-tests description: The repo's test-suite health expert. Reviews the test suite the way a senior staff engineer who cares about test quality would — coverage gaps on critical paths, inappropriate or excessive mocking (mocking what you own, mocks that never assert, asserting on mock internals over behavior), weak or absent assertions, brittle test strategy (testing private implementation, over-pinned arg matchers, golden-snapshot noise), flaky patterns (unfrozen time/randomness, real network, sleeps, order dependence), fixture smells, test duplication that wants parametrization/table-driven cases, rotting skip/xfail, and drift from the repo's own documented test conventions. FIX-FIRST: open one focused PR per mechanically-safe fix; file an issue only for design-heavy test-strategy changes. Scoped to TEST quality only — source-architecture findings are deferred to the `audit-architecture` skill. SILENT ON CLEAN: a run that finds nothing (even nitpicks) produces no PR, no issue, and no report. Built to run unattended several times a day, so dedup is strict and per-run caps are low. Reads the repo's test conventions from `config.guidelines.testing`. Use when the user asks to "audit the tests", "check test quality", "find test smells", "review the test suite", or when invoked by a scheduled remote agent. Has working-tree side effects (branches + PRs) and GitHub side effects (issues, labels). --- # Audit the test suite and fix what's safe to fix This skill is a recurring test-health sweep. It reads the **test suite** the way a senior staff engineer who is opinionated about testing would — finding the quality problems that don't fail CI (the suite is green) but quietly erode the suite's value: tests that pass without proving anything, mocks that hide the bug they should catch, coverage that looks fine in aggregate but misses the error path that actually breaks in prod. It **fixes what is mechanically safe** (one focused PR per finding) and **files an issue** only for changes that need design discussion. Two properties make this skill safe to schedule several times a day: - **Silent on clean.** A run that surfaces no findings — including nitpicks — opens no PR, files no issue, and prints no report. Absence is the signal. Most runs should be silent. - **Strict dedup, low caps.** Because it runs unattended and often, it never re-opens a fix that already has an open PR/issue, never re-files something a human closed `wontfix`, and caps itself hard per run so it can't flood review. ## Load the repo config Before anything else, load the repo config (see [`../../references/config-schema.md`](../../references/config-schema.md)): 1. Read `.claude/maintainerd.json` from the repo root. 2. If it does not exist, **STOP** and tell the user: > This repo has no `.claude/maintainerd.json`. Run `/bootstrap` to generate it, then re-run me. Do not guess values or hardcode another repo's settings. 3. Read the keys this skill needs: - `config.repo` — GitHub `owner/name`, passed to every `gh ... --repo`. - `config.defaultBranch` — the branch audits check out and PRs target. - `config.language` — `python` | `typescript`; selects the language-specific mechanics below. - `config.paths.tests` — root of the test tree (everything in scope lives under here). - `config.paths.source` — root of the source the suite covers. - `config.commands.test` — the suite that must be green before and after a fix. - `config.commands.coverage` — coverage command. **If `null`, skip the coverage-gap category entirely** (don't invent one). - `config.commands.format` / `config.commands.lint` — pre-flight gates (skip any that are `null`). - `config.labels.testQuality` / `config.labels.automated` — labels applied to every PR/issue this skill opens. - `config.audits.testPrCap` / `config.audits.testIssueCap` — per-run caps (default **2 / 2** if absent; note the fallback in your report). - `config.audits.promoteThreshold` / `config.audits.promoteLookbackDays` — pattern-promotion knobs (default **3** within **90** days if absent). See step 7. - `config.guidelines.testing` — **the repo's own test conventions.** Read this file in full; it holds the repo-specific rules (the DB/fixture pattern, what may and may not be mocked, isolation rules, assertion expectations, redundant framework boilerplate, naming) that this skill checks the suite against. If it's missing or still full of `TODO` markers, say so in your report and proceed with the language-generic checks only. Treat a `null` command as **"this repo has no such step — skip it, don't invent one."** ## Scope: tests only This skill audits **test quality**. It is the sibling of `audit-architecture`, which owns the **source** side (oversized modules, source DRY, source typing, dead exports, source invariants). To avoid two schedulers opening competing PRs: - **In scope:** anything under `config.paths.tests` — the tests themselves, the fixtures, the conftest/setup files, the fakes and test doubles, test helpers, and the *coverage* of `config.paths.source` as measured by the suite. - **Out of scope (defer to `audit-architecture`):** refactoring `config.paths.source` for its own sake. The one overlap — "this source module has **zero** tests" — belongs to `audit-architecture`'s missing-tests category; don't duplicate it here. This skill instead improves the *quality* of tests that already exist and closes *coverage gaps within already-tested modules* (the uncovered error path, the unexercised branch). - If a test smell can only be fixed by changing source (e.g. code is untestable because a dependency is hard-wired instead of injected), **file an issue** describing the testability problem; don't refactor the source here. The repo's formatter/linter and the test hook already gate the obvious. Don't re-file what they catch. This skill targets what they can't see: whether a passing test actually tests anything. ## What it looks for Each category has a default detection method and a default routing decision (PR vs. issue). Routing is a default — apply judgment. The detection commands here are described language-agnostically; the exact greps and tool flags for `config.language` are in [Language-specific mechanics](#language-specific-mechanics). | Category | Detection | Default routing | | --- | --- | --- | | **Coverage gaps on tested modules** | Run `config.commands.coverage`, then read its machine-readable report for files with covered lines but uncovered **branches** or error/exception paths. Focus on modules that *have* tests but leave a meaningful branch uncovered — the `catch`/`except`, the early-return guard, the cap-fallback. **Skip this whole category if `config.commands.coverage` is `null`.** | **PR** if the missing case is a small, obvious test to add (≤ ~60 lines, one behavior); **issue** if covering it needs new fixtures or non-trivial setup | | **Mocking what you own** | Read tests that mock/patch an internal collaborator that could be exercised for real. The cardinal repo-specific case — e.g. mocking the DB layer instead of using the real test-DB fixture — **lives in `config.guidelines.testing`**; read that file and flag tests that violate it. Catch every mock constructor, not just one (see the language section's greps), then verify each hit is a legitimate placeholder (the unit under test doesn't touch the real collaborator) vs. a mock standing in for real behavior the test should assert against. | **PR** if swapping to the real fixture is local and the assertions still hold; **issue** if it reshapes the test | | **Mocks that never assert** | A mock created and passed in but never checked (no call/await assertion) and whose return value isn't asserted on either — the mock is decorative, the test proves nothing about the interaction. Grep for mock construction, then confirm a corresponding assertion exists. | **PR** — add the missing behavioral assertion, or simplify away the mock | | **Asserting on mock internals over behavior** | Tests that assert a mock was called with some incidental positional/argument shape instead of the observable outcome — brittle, breaks on harmless refactors. Prefer asserting the effect (DB row, returned value, emitted event). | **Issue** if rewriting the assertion changes what's tested; **PR** if it's a clean tighten | | **Weak / absent assertions** | Two reliable greps: tests whose body has **no assertion** at all (exercise-only), and over-broad "expects any error" assertions where a specific type/message is meant. The "truthiness where a value check belongs" smell (a test whose **only** assertion is `assert x` / `x is not None` / `expect(x).toBeTruthy()`) is *not* a useful grep — those guards legitimately precede real assertions, so a bare grep over-matches by 10×. Find it by reading short test bodies, not sweeping. | **PR** — strengthen to assert the actual value / specific error | | **Flaky patterns** | Grep for sleeps; unfrozen current-time / `random` in a test that asserts on the result; real network to a live host; tests that depend on execution order or shared module-global mutable state. | **PR** if the fix is freeze-the-clock / inject-the-value / use fake timers; **issue** if it needs a fixture redesign | | **Redundant framework boilerplate** | Per `config.guidelines.testing`: a decorator/marker the repo's test config has made a no-op (e.g. a per-test async marker that auto-mode makes redundant, a leftover `.only`/`.skip` that disables the rest of the file). Read the convention there; don't assume one. | **PR** — mechanical removal (roll all hits in one file, or a few files, into one PR) | | **Rotting skip / xfail** | Grep for the skip/expected-fail markers; flag any without a stated reason, and any expected-fail that now passes. | **PR** if the test now passes (un-skip it) or the skip is clearly obsolete; **issue** if the skip hides a real open bug | | **Test duplication → parametrize** | Manual reading: 3+ near-identical test bodies differing only in input/expected — the textbook parametrized / table-driven case (`@pytest.mark.parametrize`, `it.each`/`test.each`). Also copy-pasted setup blocks across tests in a file that want a shared fixture/helper. | **PR** if the collapse is mechanical and the cases stay readable; **issue** if it's a big restructure | | **Fixture smells** | Unused fixtures (defined, never requested); fixtures with hidden side effects; suite/session-scoped fixtures holding **mutable** state that can leak across tests; setup duplicated across files that belongs in a shared conftest/setup. | **PR** for unused-fixture deletion; **issue** for scope/leak redesign | | **Slow tests** | Run the suite with the runner's slowest-tests report; flag individual tests far above the median (e.g. a unit test taking seconds because it sleeps, hits the network, or rebuilds heavy state per-case). | **Issue** — speeding up usually means restructuring setup | | **Repo test-invariant drift** | Read the conventions in `config.guidelines.testing` and **check the suite against each one.** These are the repo's load-bearing test rules CI doesn't enforce — the DB/fixture isolation pattern, fixtures that must mirror a source change (e.g. a new app-state attribute mirrored in the test client fixture), user-facing rendering rules (timezone/locale), teardown that must rely on the rollback rather than be hand-rolled. Flag each violation against the rule as written. | **Issue** — these are load-bearing and the fix often spans setup + tests | | **Untestable source (testability smell)** | Surfaced while writing/reading a test: source that can't be tested without mocking because a dependency is constructed inline instead of injected (e.g. a client built inside a function instead of passed in). | **Issue** — the fix is a source change; describe it, don't make it (that's `audit-architecture`/a human's call) | The table isn't exhaustive, but every addition must be an **objective, nameable test-quality smell** that obeys the same routing rule and the "Scope: tests only" / "What it does NOT look for" sections — not source review, not subjective style. Concretely, also capture any of: - A non-descriptive test name (`test_it_works`, `test_1`, `it('works')`) that doesn't state the behavior under test. - A single test asserting several **unrelated** behaviors (should be split into focused tests). - A test double / fake that has drifted from the real interface it stands in for (asserts pass against a contract production no longer has). - A fixture or test helper that swallows errors or hides failures (bare `except` / empty `catch`, unconditional `return`). - A tautological assertion (`assert x == x`, `expect(x).toBe(x)`, `assert mock.return_value == mock.return_value`). - A test coupled to private implementation (asserts on an underscore-prefixed / non-exported attribute or method that isn't part of the contract). - A golden-snapshot test whose snapshot is so large or volatile it's noise — it re-blesses on every harmless change and catches nothing. Route each the same way: mechanical and behavior-preserving → PR, judgment-heavy or reshaping → issue. Anything about *source* goes to `audit-architecture`; anything that's a matter of taste (naming preference, comment density) is out of scope — don't open work for it. ## Language-specific mechanics The greps and tool flags for each language live in [`references/language-mechanics.md`](references/language-mechanics.md) — read the block matching `config.language`. What counts as a finding stays in the table above. ## What it does NOT look for - **Source architecture.** Oversized source modules, source DRY, source typing, dead source exports, source invariants → `audit-architecture`. A source module with **zero** tests is also that skill's call, not this one. - **Formatting / lint.** The repo's formatter/linter owns it (hook + CI). Don't re-file what `config.commands.format` / `config.commands.lint` catch. - **Test pass/fail.** The suite is green or this skill shouldn't be running fixes on top of red. If `config.commands.test` is red on a clean `config.defaultBranch`, stop and report that — don't audit on top of a broken suite. - **Raising the coverage *number* for its own sake.** Chasing a percentage produces assertion-free tests that exercise lines without proving behavior — the exact anti-pattern this skill exists to remove. Only add coverage where a **real untested behavior/branch** exists and the test would *catch a real regression*. - **Performance of the code under test.** Slow *tests* are in scope; slow *production code* is not. - **Generated/vendored fixtures.** Recorded fixture data is data, not test logic — don't flag its shape. ## Workflow Use a todo list to track findings as you process them — the sweep spans many files. ### 1. Pre-flight: start clean and green ```bash git status --short # working tree must be clean git checkout git pull # suite must be green before auditing ``` - **Dirty tree** → a human is mid-work. Stop, stay silent (or one line if invoked interactively). Don't stash. (Exception: untracked files under `config.paths.skillsDir` are scaffolding — benign; stage specific paths only.) - **Red suite on clean `config.defaultBranch`** → don't audit on top of breakage. Report the failing tests and stop. Do not open any test-fix PRs while the suite is red. The `config.labels.testQuality` label this skill applies must exist. If a fresh clone or remote runner reports "label not found" on `gh pr edit`/`gh issue create`, create it once and continue: ```bash gh label create --repo \ --description "Automated audit-tests findings (test-suite health)" --color BFD4F2 ``` ### 2. Sweep the categories (fast greps first, coverage last) **Collect findings into a list — open nothing until the sweep is done.** This lets you de-dup across categories and pick the highest-value fixes within the cap. Fast pass (cheap greps from the [language section](#language-specific-mechanics); each hit is a *candidate*, not yet a finding — read the surrounding test before believing it): 1. Redundant framework boilerplate (per `config.guidelines.testing`). 2. Skip/xfail/only rot. 3. Flaky patterns — sleeps; unfrozen current-time/`random` in result-asserting tests. 4. Weak assertions / broad error expectations; assertion-free test bodies. (Don't grep for truthiness guards — they over-match; catch truthiness-only tests by reading.) 5. Mocked collaborators — construct-then-verify against `config.guidelines.testing` (placeholder vs. real-behavior-mock). Reading pass (judgment — don't force findings if nothing obvious surfaces): 6. Mocks that never assert; assertions on mock internals over behavior. 7. Test duplication that wants parametrize/table-driven cases; fixture smells. 8. Repo test-invariant drift — read each rule in `config.guidelines.testing` and check the suite against it. Slow pass (run once, near the end): 9. Slow tests — the runner's slowest-tests report. 10. **Coverage gaps** — run `config.commands.coverage` (skip if `null`), then read its report: for modules that already have tests, find uncovered **branches** / exception arms that represent real untested behavior. (Don't chase whole untested modules — that's `audit-architecture`.) ### 3. De-duplicate — strict (this runs several times a day) For each candidate finding, skip it if any of these is true: ```bash # This skill's own open PRs/issues gh pr list --repo --state open --json number,title,headRefName --limit 50 gh issue list --repo --state open --label --json number,title --limit 100 # Don't collide with audit-architecture mid-fix on the same file gh pr list --repo --state open --search "head:arch-" --json number,headRefName,files --limit 50 # Don't re-file what a human closed wontfix gh issue list --repo --state closed --label \ --search "is:closed reason:not-planned" --json number,title --limit 50 ``` Skip when: - An open PR with an `audit-tests-` branch already addresses this file+category (it may be from a run an hour ago — **the most important dedup, because this skill runs often**). - An open `arch-*` PR touches the same test/source file (let it land first; auditing a file mid-flight causes conflicts). - An open `config.labels.testQuality` issue already describes it. - A human closed the same finding `wontfix` — that's their standing answer; don't refile. ### 4. Route each surviving finding - **One PR** if all: fix is < ~120 lines of diff, touches ≤ 4 files, behavior-preserving for the code under test (you're improving the *test*, not changing what production code does), and a reviewer would say "yes, obviously better." Prefer PRs — the mandate is *fix it*. - **One issue** otherwise (test-strategy reshapes, fixture-scope redesigns, testability smells that need a source change, slow-test restructures). - Unsure → **issue**. **Umbrella issues** when one category yields many similar findings (e.g. 8 files with the same fixture-leak smell): one prioritized umbrella, not 8 thin issues. The umbrella lists every covered finding grouped by priority with explicit out-of-scope notes. ### 5. Open PRs (cap: `config.audits.testPrCap`, default 2 per run) The cap is low **on purpose** — this runs several times a day; a handful of focused, obviously-correct test PRs per run is a sustainable trickle, a flood of them trains reviewers to rubber-stamp. For each PR-routed finding, highest-value first: ```bash SLUG="audit-tests--" # e.g. audit-tests-asyncio-marker-cleanup git checkout && git checkout -b "$SLUG" ``` Make the fix. Keep it laser-focused — **only** the test change you described. Do not "while I'm here" adjacent tests, do not reformat, do not touch source unless the finding is explicitly a source change you've decided is mechanical and safe (rare; default is to route source changes to an issue). **Pre-flight before pushing.** If the `create-pr` skill is installed, delegate the pre-flight + push + PR-open to it (it runs every gate the repo declares and enforces the PR template). Otherwise run the gates inline — all of these must pass, **skipping any whose command is `null`**: ```bash # skip if null # skip if null ``` If any fails on something you didn't touch, the failure is unrelated → abandon this finding (`git checkout && git branch -D "$SLUG"`; nothing was pushed), note it in the report, move on. **No `--no-verify`.** A test-quality skill that ships a red PR has no credibility. Then open the PR (or let `create-pr` do it): ```bash git push -u origin "$SLUG" gh pr create --repo --base \ --title "test: " \ --body "