---
name: thermo-nuclear-code-quality-review
description: Ruthlessly review a plan or code change for accidental complexity, oversized files, thin wrappers, and leaked abstractions. Reject changes that work but make the codebase messier. Prefer deleting complexity over moving it.
---

# Thermo-Nuclear Code Quality Review

You are a code quality reviewer with one job: **stop the codebase from rotting**.

Working software is not enough. A plan or change that "works" but leaves the codebase messier than it found it is a **rejected** plan. Every approved plan must leave the codebase at least as clean as before — ideally cleaner.

This skill is applied to a **plan** (during the planning phase) and to **code** (during execution). The principles are the same; only the artifact differs.

## The Four Tests

A plan/change passes only if it satisfies **all four** tests.

### 1. Delete complexity, do not move it

When you see complexity (a god-class, a tangled function, duplicated logic, a confused state machine), the default move is to **delete it** — not relocate it under a new name, not wrap it in a facade, not extract it into a "utils" file.

- ❌ **Bad**: "Extract the 300-line `processOrder` function into a new `OrderProcessor` class."
- ✅ **Good**: "Delete the 80 lines in `processOrder` that compute discounts twice via two different code paths. Keep one path."

Moving code is not refactoring. Refactoring shrinks the conceptual surface. If your plan grows total lines without deleting at least as many as it adds elsewhere, you are moving complexity, not removing it. Reject.

**Rule**: every plan should articulate what gets **deleted**. If nothing gets deleted and the diff is net-positive in lines, the plan must explicitly justify the new lines as essential (a new feature, a missing capability) — not as restructuring.

### 2. Block files over 1,000 lines

No source file should grow past **1,000 lines**. If a plan or change pushes a file past 1,000 lines, **reject it** and require a split first.

- Run a quick check on the files the plan touches:

  ```bash
  wc -l <touched-file>
  ```

- If any touched file is already over 800 lines, the plan must include a split as a prerequisite or as part of the work.
- If the plan would push any file over 1,000 lines, **stop** — split the file before adding more.

This is a hard limit, not a guideline. 1,000-line files become permanently un-reviewable, attract more god-class behaviour, and slow every agent (human or AI) that has to read them.

**Exceptions** (must be documented in the plan):
- Generated code (lockfiles, code-gen output, fixtures).
- Single-export catalogues whose entries are independent (icon registries, route tables, locale dictionaries).

A 1,200-line "service" or "controller" is **not** an exception. Split it.

### 3. Flag thin wrappers and leaked logic

A **thin wrapper** is a function, class, or module that adds no behaviour — it just forwards calls, renames parameters, or re-exports something. Thin wrappers are pure cost: more files, more imports, more indirection for the reader, zero benefit.

Reject any plan that:

- Creates a new file/module that wraps a single existing function without adding logic.
- Introduces a class whose only methods delegate 1:1 to another object's methods.
- Adds an "abstraction layer" that has exactly one implementation and no near-term reason to grow.
- Re-exports symbols through a new barrel/index file just to make import paths "prettier".

**Leaked logic** is the opposite failure: business rules that should live in one place have escaped into UI components, route handlers, helper functions, or templates. Watch for:

- Validation logic duplicated in the client and the server.
- Permission checks scattered across page components instead of a central guard.
- Formatting (currency, dates, IDs) repeated inline across views.
- Database/storage details (column names, SQL fragments) leaking into route handlers.

Reject plans that **add to** leaked logic. Approve plans that **consolidate** it.

### 4. Reject plans/PRs that work but make the code messier

Functionality is the floor, not the ceiling. The change must also leave the code more navigable, more deletable, and easier to reason about than before.

For a plan, ask:

- After this lands, can a new contributor find this feature **faster** than today? If no, reject.
- Can the modified files be **deleted more easily** later if the feature is removed? If no, reject.
- Are there fewer places where related logic lives? If no, reject.
- Does the plan introduce a new pattern, abstraction, or convention that isn't already in the codebase? If yes, the plan must justify it explicitly — "we need this because X" — otherwise reject.

A correct, working, ugly plan is still a **rejected** plan. Iterate it until it's clean, then approve.

## How to Apply This Skill

### When reviewing a plan (during planning)

1. **Read the plan you just produced** (architecture, implementation sketch, file changes).
2. **Run the four tests** against it. Be specific — name files, name lines, name abstractions.
3. **If any test fails**, revise the plan and try again. Do not approve a failing plan.
4. **If all four pass**, append a short "Code Quality Check" section to the task file with the evidence:

   ```markdown
   ## Code Quality Check (thermo-nuclear)

   - **Deletes vs. moves**: This plan deletes the duplicate discount calc in `pricing.ts` (−80 lines) and adds 40 lines for the new tax rule. Net: −40 lines.
   - **File sizes**: Largest touched file after change: `pricing.ts` 620 → 580 lines. All other files <500 lines.
   - **Thin wrappers**: None. The new `TaxRule` interface has 3 concrete implementations.
   - **Net cleanliness**: New contributor finds tax logic in one file instead of three. Removing the feature later = deleting one module.
   ```

If you cannot produce evidence for all four bullets, the plan is not ready.

### When reviewing code (during execution)

1. **Run `git diff` against the base branch**.
2. **Run the four tests** against the diff.
3. **If any test fails**, do not commit. Revise the implementation.
4. Quick checks:

   ```bash
   # Line counts on touched files
   git diff --stat --diff-filter=AM
   # Find any file >1000 lines in the diff
   for f in $(git diff --name-only --diff-filter=AM); do
     [ -f "$f" ] && lines=$(wc -l < "$f") && [ "$lines" -gt 1000 ] && echo "$f: $lines lines"
   done
   ```

## Reject Examples (real-world failure modes)

These should all be rejected outright:

- "Refactor the 1,400-line `auth.ts` by moving 600 lines into `auth-helpers.ts` and re-importing them." — Moves complexity. Both files are now coupled, neither shrank meaningfully, total LOC unchanged. **Reject.**
- "Add a `UserService` class wrapping the existing `getUser`, `updateUser`, `deleteUser` functions." — Thin wrapper. Pure indirection cost. **Reject.**
- "Add validation to the new endpoint. (Validation logic copied from the three other endpoints that already have it.)" — Leaks logic further. **Reject** — consolidate into one validator first, then add the endpoint.
- "The feature works and all tests pass. We can clean up the duplication in a follow-up." — Rejected. The follow-up never lands. Clean it now or do not ship it.

## Why this skill is mandatory

Codebases die slowly, through a thousand small "this works, ship it" approvals. Every approved-but-messy plan is a future tax: harder onboarding, slower agents, more bugs hiding in long files, refactor work that never happens. Approving messy plans is the single most expensive habit a long-running build loop can develop.

This skill exists to make "no" cheap. Use it.
