--- name: test-refactor description: "Audit one test suite at a time for dead weight, duplication, parallel-unsafety and runtime cost, or characterize behavior before a risky refactor — with production code frozen and good coverage kept, proven per module rather than assumed. Triggers: test refactor, characterize before refactor, characterization tests, clean the tests, refactor the tests, test suite audit, DRY the tests, parallelize the test suite, speed up the tests." user-invocable: true --- # test-refactor ## Host execution **Claude Code:** retain the agent-dispatch and concurrency behavior defined below. **Codex:** use only the inline behavior stated here. When loaded by Codex, run every audit slice, scanner pass, ticket step, and drain step inline and sequentially. Never spawn, request, or claim agents or subagents; replace parallel read-only fan-out with ordered passes over the same disjoint areas, preserving the full evidence and coverage contract. ## Purpose The sibling of `deep-refactor`, with the contract inverted. There, the test suite is the frozen proof that a refactor preserved behavior. Here the tests **are** the object of change — so the frozen side is **production code** (a test refactor that "needs" a production edit is out of scope; a real defect a test uncovers is reported and ticketed separately, never smuggled into a test PR), and the proof that rigor survived is **measured**: per-module coverage parity plus a demonstration that every touched test can still fail. **The goal is a suite that is clean, DRY, easy to understand, worth trusting, cheap to run — and that keeps good coverage.** A test that cannot fail, or that fails without naming what broke, is worse than no test: it spends CI time buying false confidence. The suite's wall-clock and memory footprint are deliverables in their own right: every second and megabyte it costs is paid on every push, by every contributor, forever. And coverage is a goal, never a casualty: this skill removes tests that cover *nothing* and duplication that covers things *twice* — deletions and merges hold per-module coverage at or above the baseline, and the depth phase pushes it up where the platform is critical. A cleanup that ends with less real coverage than it started with has failed, whatever else it improved. **One suite at a time.** The user names the scope (e.g. "backend unit tests", "frontend component tests", "integration"). Everything outside it is untouchable this run; cross-suite findings go in the report as follow-ups. ### `characterize ` — establish a frozen contract first When invoked with `characterize`, do not run a deletion campaign. Production code stays frozen and the sole goal is to add the smallest owner-boundary tests that expose the behavior a later `deep-refactor` ticket must preserve. Start with a named risky flow, public contract, failure mode, or compatibility path; never characterize internal call shape merely because it is easy to assert. Each new test answers: (1) what observable behavior or invariant it protects, (2) what credible regression makes it fail, (3) why existing coverage misses that regression, and (4) whether it demands a production seam no production caller needs. If the fourth answer is yes, move the test to the real boundary instead. For a reported bug, prove the test fails on the pre-fix behavior when feasible. One owner-boundary regression covers one defect; do not replay it at every crossed layer. Report the established contract, its owning test, targeted validation, and the precise `deep-refactor` candidate it unblocks. Then return to the normal audit → report → ticket → drain flow; characterization is a ticket in that flow, not a new approval gate. Four phases: **audit → report → ticket → drain**. ## Phase 1 — Audit On Claude Code, fan out parallel read-only agents over disjoint slices of the suite, plus one pass over the harness itself (fixtures, conftest/setup files, CI test job, coverage config). On Codex, cover those slices and the harness in order inline. **Loop until dry**: after acting on a pass, run another with fresh eyes; the exit condition is a pass that comes back empty. Classify every test — no test is skipped because it looks fine: 1. **Tests nothing — delete.** Tests of the language, the standard library, or an external library called directly (a test of *your* function that happens to call the library is fine — that tests your integration); tautologies (asserting a value equals the value just constructed); mock echoes (asserting a mock returned what it was told to return); assertion-free tests that pass by not crashing when crashing was never the risk. Each deletion ships with its category and the **evidence it covered nothing**: show the assertion cannot fail, or mutate the subject and show the test stays green. 2. **Duplicate coverage — merge.** Same subject, same behavior, different copies: fold into one parametrized test. **Parametrization is not duplication** — cases that differ in inputs are distinct coverage; add missing cases while you're there. But never fuse tests of *different behaviors* into one mega-test: a failure must name its scenario, so one behavior = one test (or one parametrized family with self-describing ids). 3. **Load-bearing but odd-looking — verify, then leave.** Regression locks, source-text guards, non-vacuity meta-tests, coverage link-imports, seams kept for patching: check history and tickets before touching, and record *why it stays* so the next audit doesn't relitigate it. A test you can't explain is a research item, not a deletion candidate. **One shape here is a defect even while green: a source-text guard that scans RAW text.** It disarms itself the day someone documents the rule beside the code it polices — a comment explaining "we deliberately never call `X` here" contains `X`, so the guard scanning for `X` now passes on prose rather than on code. The same trap fires when the scanned output carries the guard's own identifiers (a collected-test listing contains the guard's own name). Fix it by parsing — walk the AST, or strip comments and docstrings before scanning — never by loosening the assertion. 4. **Shallow on critical paths — deepen.** Identify the platform-critical modules; a happy-path-only test on one is a finding. Add edge cases, failure paths, boundary values, property-based tests where they pay. Depth on what matters outranks breadth on what doesn't. 5. **Unreadable or WET — refactor.** Copy-paste setup becomes shared fixtures, factories, builders. But **locality beats indirection**: a reader must see what a test asserts without chasing five fixtures, and a test must contain **no logic** — a loop or conditional that computes the expected value re-implements the subject and inherits its bugs. Explicit expected values, self-describing names, one assertion story per test. 6. **Expensive — profile, then cheapen.** Profile the suite: the slowest tests, the costliest fixtures, and peak memory per worker — measured, not guessed. The usual culprits: real sleeps and timeouts where a condition wait or fake clock belongs; expensive state rebuilt per test that one wider-scoped, read-only fixture could serve (widen scope **only** for state no test mutates — a shared mutable fixture trades speed for coupling and parallel-unsafety); oversized fixture data where a minimal case proves the same thing; unmocked network/disk on paths the test doesn't assert; subprocess or container spawns per test that can be pooled per worker; giant parametrize grids where a boundary-value subset has identical failure-detection power (prove it: the dropped cases catch no mutation the kept ones miss). A speedup must never be bought with coverage — that's what the parity gate below is for. **Retention bar before deletion.** An odd-looking test stays unless the audit records its exact name/location, the regression it can detect, non-test callers of any production or support seam it covers, stronger surviving owner-boundary proof (or why none is needed), the reason/history it exists where available, the deletion it unlocks, risk, and a focused validation command. Source inspection can be an independent guard only when it protects an externally meaningful key, byte, path, or architecture contract and survives an identifier-only refactor. A test that merely resembles implementation is suspect, not disposable. **Hermeticity is part of the audit.** A unit test that opens a real network connection is broken even while green: on a dev machine a local service may silently answer (the test then has side effects on a live system), and on CI nothing answers — each swallowed best-effort call burns a connect-retry budget, and in the wrong network mode hangs the suite outright. The diagnosis signature is CI durations quantized at identical values (±0.2 s) across unrelated tests: that is a timeout constant, never compute. Audit for unpatched I/O seams (publishes, queue sends, best-effort telemetry) and treat local wall-clock as inadmissible evidence about CI — a suite that is fast locally can be 10× slower on CI for reasons only CI can show you. **Pins run the other way here.** Enumerate what depends on the tests before moving them: coverage detectors keyed on static test imports, CI selection globs and naming conventions, per-file coverage-omit rules, meta-tests that scan test source, docs referencing test names. A rename or move ships with that inventory or it doesn't ship. **Measure, don't infer.** "Duplicate" comes from diffing assertions, not titles; "covers nothing" from a mutation the test survived; "flaky" from repeated runs, not reputation; "slow" and "heavy" from the profiler and the process's own peak-memory accounting, not from watching the run; coverage claims from per-module before/after reports, never the global percentage alone (a global number hides a module dropping to zero). ## Phase 2 — Report One synthesized document: deletions (each with category + evidence), merges, harness findings, parallel-unsafety inventory, critical-path depth gaps, the load-bearing list, and any production defects the audit uncovered (reported, not fixed). Three baselines attached — per-module coverage, suite wall-clock (same parallelism as CI), and peak memory per worker — **each recorded with the command and the commit that produced it**. Publish where the team can act on it. ### Cleanup train ledger The report carries one durable row per candidate: `ID`, protected behavior, evidence, test owner, production/test pins, frozen side, validation command, dependency, and status. Status is exactly `Proposed`, `Approved`, `In progress`, `Blocked`, `Superseded`, `Done`, or `Rejected` (with a one-line reason). Work that cannot be judged because the behavior lacks a trustworthy owner-boundary test is `Blocked — characterization required`; add that proof with production frozen before any code refactor depends on it. A test that is odd-looking but independently protects a real contract is `Deliberate — do not fix`, not cleanup fuel. **The Phase-2 baselines are a starting measurement, not a fixed yardstick.** This train's whole purpose is to change the suite those numbers describe, so by car 4 the Phase-2 figures describe a tree that no longer exists — test counts, file counts and wall-clock have all moved, and moved *because the earlier cars worked*. Each car therefore re-derives its own baseline at its branch point and reports its delta against that; a car measured against the Phase-2 snapshot is reporting its predecessors' work as its own, or failing a gate for their changes. Likewise every count in a ticket — call sites, spec files, collected tests — is re-derived before planning it, never carried over from the report — see Phase 4. ## Phase 3 — Ticket One ticket = one PR, sequenced: 1. **Deletions and merges** — every removed or fused test enumerated in the ticket in advance. Coverage parity proven per module in the PR, not asserted. 2. **Harness and fixture consolidation** — shared fixtures, factories, dead fixtures removed, setup dedup. Behavior of every surviving test unchanged. 3. **Parallel-isolation fixes** — see the invariant below; one ticket owns making the whole suite safe in a single parallel run. 4. **Runtime-cost fixes** — the profiled speed and memory wins, each PR reporting measured before/after against the Phase-2 baselines. 5. **Depth additions last** — new tests for critical-path gaps, so they land on the cleaned suite (and are written cheap from the start). Every ticket lists its own out-of-scope items. Production-code diff in every PR is **empty**, verified mechanically (diff the production paths — zero lines), except a separately-ticketed defect fix that is its own PR. Every ticket is executable with no audit-session context. State protected behavior, exact in-scope and tempting-but-out-of-scope paths, the retained test owner, frozen production boundary, pins, repository-native commands with expected results, and specific STOP conditions (drift, a new pin, a required production edit, or a failed characterization). End with a **prevention decision**: `Guard added`, `Ownership recorded`, or `No guard justified`. A guard must be the cheapest independent proof, never a brittle implementation grep or a permanent instruction added for ceremony. ## Phase 4 — Drain - One branch per ticket off current main; isolated worktrees when parallel. - **Re-verify at the merged state, not at authoring time.** Each merged car changes the suite the next car's ticket describes: locate every target by content rather than by line number, and re-derive every count and baseline against the branch's own tree rather than trusting the report. A claim that no longer holds is a finding to report before implementing, never a silent skip or a blind apply. - **Reconcile the ledger before every car.** Re-check every ready candidate against current main. Mark independently fixed work `Superseded`; refresh drifted evidence and scope before it can run; retain a reintroduced resolved problem as `Possible regression`, not a duplicate; and leave an evidence-backed `Rejected` or `Deliberate — do not fix` row visible so the next audit does not relitigate it. Drain all remaining `Approved` work without pausing for a checkpoint. - **Do not hand-roll the drain.** Each ticket goes through the project's normal implement → review → merge pipeline (`agile-10-implement` / `agile-11-merge-train` where installed), so every car carries the same validation, phase markers, review receipts and post-merge postmortem as any other ticket. An audit train is a *source of tickets*, never a parallel process with weaker evidence: a car that merges with no marker trail leaves the board unable to say how the change was reviewed, and that gap is invisible precisely because the code shipped fine. - **The single-parallel-run invariant.** The whole suite runs in **one parallel invocation in the CI test job**. Never solve a parallelism conflict by splitting the run, serializing a subset, quarantining a file, or adding retries — fix the test's isolation instead: unique temp dirs, ports, database schemas/transactions per worker; no shared mutable globals; no fixed resource names; condition-based waits, never sleeps. Prove order-independence by running with randomized order and full parallelism locally before pushing. A test that only passes serially, in a fixed order, or on retry is a defect with a diagnosis, not an inconvenience with a workaround. - **Make hermeticity structural, not aspirational.** Block real network connects at the test-harness level (a socket-level guard that raises instantly, allowing only what the runner itself needs) and patch every seam it exposes. One trap decides the design: a best-effort seam that swallows exceptions swallows the guard's error too and stays green — so pair the blocker with an opt-in trace audit that logs every blocked connect per test, and assert zero residue. Lock the guard itself with tests that prove it raises instantly against an unroutable address. - **A wall-clock assertion in a unit test is flaky decoration.** "Completes in under N ms" pays cold-start costs (imports, caches, calendar builds) that say nothing about the property under test and everything about the runner's mood. Assert the *behaviour* that makes it fast — the early return taken, the expensive collaborator never constructed — and delete the stopwatch. - **Non-vacuity on everything you touched.** A merged, parametrized, or rewritten test is proven able to fail: inject a defect in its subject, watch it go red, revert. A cleanup PR whose tests all still pass proves nothing by itself — green-after-deletion is exactly what a botched deletion looks like. - **A planned mutation that reddens ZERO tests is a finding, not a pass.** It is the only direct evidence that the guard you were about to trust is decorative — the suite cannot see the very defect the test claims to catch. Report it, then either rewrite the guard until the mutation reddens it or re-plan the item; never record it as a completed non-vacuity proof, and never move on because "the tests are green". Expect this to fire on the mutations you were most confident about: a framework that de-duplicates or batches (an in-flight request cache, a scheduler) can absorb the injected defect and leave the count blind, which is itself the finding. - **Re-derive before you gate.** Coverage, wall-clock and memory gates are read against the car's own re-derived baseline (Phase 2), not the report's snapshot. - Coverage gate per PR: per-module coverage at or above that baseline. A drop is a blocker, not a footnote. - Cost gate per PR: wall-clock and peak memory at or below the baseline (measured the same way, same parallelism). A cleanup that makes the suite slower or heavier explains itself in the PR or doesn't merge; a perf win is stated with its numbers, not adjectives. **A measured win far below the ticket's estimate is a finding worth stating plainly** — say so in the PR rather than quoting the estimate; the audit's projection was a hypothesis and the benchmark is the result. **Never quote a wall-clock number measured under concurrent load**: a machine also building sibling cars produces a spread wider than the effect, and any figure drawn from it is noise wearing a decimal point. - Merge only on a green CI run you verified yourself; sequential merges; rebase the next branch when file sets intersect. Two identical CI failures are a diagnosis, not a rerun. ## Work discovered mid-phase — do it, or ticket it properly Every phase discovers work its ticket did not plan for. Two decisions, in order, and neither of them is "leave it in a comment": **1. Do it now, or file it?** - **Trivial and inside the current scope** → do it here. A one-line correction or a stale comment beside code you are already editing does not need its own ticket; filing one costs more than the fix. - **Anything else** → a follow-up ticket: non-trivial, carrying risk, needing its own review, or reaching into files this work does not own. Never silently widen the diff to absorb it, and never let it survive only as prose in a PR body. **2. Which backlog does it enter?** - **The current sprint** — it blocks the sprint goal, it is a must-have, or a human asked for it. - **The product backlog** — everything else, and this is the default. Pulling work into a running sprint is a scope change, not a convenience. **Point it at creation.** A ticket minted mid-phase never passes back through the refinement skill, so if it is not sized here it is never sized at all, and the sprint's velocity figure silently stops describing the work delivered. Use the project's normal estimation scale; if it truly cannot be sized yet, label it `unsized` with a one-line reason rather than leaving the field empty by default. ## Definition of done Suite green in one parallel CI run; per-module coverage ≥ the baseline; wall-clock and peak memory ≤ the baseline (better where the audit found waste); every deletion enumerated with evidence; zero production-code changes in the train; the report updated; every new lesson (a pin class you hadn't met, a flake signature, an isolation trick, a fixture-cost surprise) written down where the next audit will find it.