cleanup · diff
v1 to v1.2
34 added, 62 removed. Audit A to A.
---
name: cleanup
- description: "Review and clean up the given file(s)/folder(s)/module(s) in any language or framework: rate organization, find dead code, duplication, coupling, over-engineering, structural and architectural problems, then produce and execute a phased refactor plan. May apply architecture/pattern changes when they are a net simplification. Trigger on \"clean up X\", \"review and refactor X\", \"rate the code in X\", \"code quality review of X\"."
+ description: Review and clean up the given file(s)/folder(s)/module(s): rate organization, find dead code, duplication, coupling, over-engineering, deep nesting, structural issues, and bad comments, then produce and execute a phased refactor plan. Trigger on "clean up X", "review and refactor X", "rate the code in X", "code quality review of X", "fix the comments in X".
metadata:
- version: 1.0
+ version: "1.2"
---
# Code Cleanup
- Systematic review-then-refactor of a target path (file, folder, module, package, or feature) in **any language or framework**. Every judgment is calibrated against the host project's own conventions and the ecosystem's idioms, not generic taste. Behavior-preserving by default; architecture and pattern changes are **in scope** when they pass the net-simplification test (below). The argument is the target path(s); if none given, ask.
+ Review a target path (file, folder, module, or feature), then fix what you found. Judge everything against the project's own written rules (CLAUDE.md, rules files, lint config), not general taste. The argument is the target path(s). If none given, ask.
- ## Process
+ Limits: one review pass, one implementation pass, one question to the user. Do not add more.
- ### 1. Calibrate
+ ## Findings list
- - Detect the stack: languages, package manifests, build system, framework(s), and the project's own verification commands (typecheck/compile, lint, tests, formatter). Read them from project docs and manifests - never guess. These commands are the gate for every later phase.
- - Read the project's conventions FIRST - CLAUDE.md/AGENTS.md, rules files, lint/formatter config, editorconfig. Note the size ceiling, naming scheme, comment policy, and framework idioms (e.g. an auto-memoizing compiler makes manual memoization a finding, not a virtue; a DI container makes `new` in handlers a finding).
- - Inventory the target's files with line counts, largest first.
- - Assess the safety net: does the verification harness actually cover the target? If tests are thin, prefer low-risk phases, lean harder on adversarial verification, and consider adding characterization tests before risky structural work.
+ One file in the scratchpad, one line per finding: `id | category | file:line | claim | evidence | status`. Status is CONFIRMED, UNCERTAIN, or KEPT. For any "unused" or "dead" claim, the evidence is the search that proved it. Search for indirect uses too: keys built from strings, `obj[key]` lookups, translation keys assembled at runtime. A claim without a search stays UNCERTAIN. KEPT records what was checked and deliberately left alone.
- ### 2. Map the boundary
+ ## Process
- - Find every consumer outside the target (grep the target's path/package/symbol names). Record which symbols cross the boundary - the public surface that moves/renames must preserve. If the target is a published library, the surface includes consumers you cannot see: treat its exported API as frozen unless the user says otherwise.
- - **Invisible callers**: enumerate symbols invoked without an import - framework-registered routes/handlers/lifecycle hooks, DI/IoC registrations, reflection and dynamic dispatch (`getattr`, `Method.invoke`, message selectors), serialization/ORM field names, config- or convention-referenced classes, CLI entry points, FFI exports, template references, scheduled jobs, migrations. Grep alone cannot prove these dead.
- - Map the reverse direction (what the target reaches into) and flag misfiled code: anything inside the target consumed only by a different feature, judged by who calls it and whose data it touches.
- - Enumerate **adjacent plumbing** - out-of-tree files that wire the target in: routes/pages/handlers that mount it, DI wiring, build/config entries, data-access helpers and cache keys, localization files, docs/feature-map entries, CI steps. Plumbing is review scope, not just context: dead code hides there, and moves inside the target often require updating it.
+ ### 1. Map the target (you, no agents)
- ### 3. Hunt - parallel fan-out, every finding cited as `file:line`
+ - List the target's files with line counts, largest first.
+ - If the project has a dead-code tool (knip, ts-prune, depcheck, an unused-imports lint rule), run it on the target first and seed the findings list from its output. Manual searching then covers only what the tool cannot see: keys built at runtime, translation keys, response fields.
+ - Search once for files outside the target that import from it. The names they import are the public API that moves and renames must keep working.
+ - Note connected files: routes or pages that render the target, providers, query functions and keys, locale files, docs or feature index entries. Search them for references only. Do not review them in full.
+ - Write a 5-line summary of the project rules from the docs already in context: file size limit, naming, comment policy, framework habits (if the framework already memoizes, as React Compiler does, manual memoization counts as an issue), and the check commands (typecheck, lint, tests). Paste it into every agent prompt.
- Lenses, phrased stack-neutrally (translate each to the detected ecosystem):
+ ### 2. Review
- - **Dead code** - exports/functions with zero callers, parameters never used or always passed the same constant, state written but never read, unreachable branches, orphaned assets/locale keys/config entries, re-export indirection nobody imports through, commented-out code, feature flags whose losing branch shipped long ago.
- - **Duplication** - near-identical functions/types/templates, repeated data-massaging that reimplements an existing util or stdlib call (check the project's utils first), copy-pasted error/empty/loading shells, repeated inline constants (thresholds, colors, magic numbers, key lists).
- - **Coupling** - values threaded through layers unchanged (prop drilling, parameter plumbing, context objects passed everywhere), pass-through wrappers, N-argument bundles that should be one object, the same thing fetched/derived in many places, cross-feature reach-ins into another module's internals, circular imports, feature envy (a function that mostly manipulates another module's data).
- - **Over-engineering** - wrappers with one consumer and no behavior, generics/interfaces/traits with one instantiation, config indirection with a single reader, plugin points nobody plugs into, options nobody passes, speculative "future-proofing". Record what was evaluated and deliberately KEPT so the next pass doesn't re-litigate.
- - **Under-abstraction** - the inverse: god-files/classes past the project's ceiling, the same concept implemented twice, missing extraction where 3+ siblings repeat a pattern.
- - **Architecture & patterns** - the design itself is the wrong shape: layers that only forward calls, a pattern mismatched to the problem (inheritance where composition fits, singleton hiding dependencies, event indirection between two fixed parties, sync/async or push/pull mismatch), module boundaries that force shotgun surgery (one conceptual change = edits in many files), abstractions inverted from the dependency direction the domain wants, state owned in the wrong place. Propose the replacement shape, not just the complaint.
- - **Structure & naming** - loose root files, folder names that don't match contents, redundant filename prefixes (the directory is the namespace), inconsistent conventions within a folder, trivial re-export indirection, public surface exposing internals.
- - **Contract quality** - in typed languages: `any`/casts/non-null assertions where narrowing works, inline anonymous types in signatures, hand-written types duplicating inferred/generated ones. In dynamic languages: missing validation at trust boundaries, stringly-typed dispatch, dicts-as-structs where the ecosystem has a record idiom (dataclass, Struct, TypedDict).
- - **Data flow & performance** - N+1 calls (per-item queries/requests where a batch exists), the same data fetched or computed by multiple siblings, stored state where derivation works, work done per-render/per-request that belongs at a colder layer, unbounded collections without eviction/pagination, heavy imports for one function, sequential awaits on independent operations that should run concurrently.
- - **Error handling & resilience** - swallowed errors masking failure as a valid state, missing error/empty paths, inconsistent error strategy across siblings, missing cleanup of timers/handles/subscriptions/connections, race conditions from stale closures or unawaited sequencing, missing timeouts on external calls.
- - **Consistency** - two patterns solving the same problem within the target (mixed data-access idioms, mixed styling, mixed dialog/CLI-output/logging patterns): identify the project-dominant idiom and converge on it.
- - **Convention violations** - breaches of the project's own written rules (comments, naming, styling, framework idioms), judged strictly against the docs read in step 1.
- - **Micro-simplification** - line-level shrink: early-return/invert-if to kill nesting, redundant conditionals (`if (x) return true; return false`), boolean-flag parameters that should be two functions, switch/if-chains that should be lookup tables, loops reimplementing map/filter/stdlib, needless `else` after return, needless async/wrapping.
- - **Dependency hygiene** - unused dependencies in the manifest, two libraries doing the same job (two HTTP clients, two date libs), a heavy dependency used for one function the stdlib covers, vendored copies of what a dependency provides, polyfills/compat shims for environments no longer supported, deprecated APIs with a drop-in modern replacement.
- - **Test suite** - tests are code: permanently-skipped tests, duplicated setup that should be fixtures/helpers, tests pinning implementation details so refactors churn them, over-mocked tests that only exercise the mocks, dead test helpers, assertions that can't fail.
- - **Nesting & layering** - files housing multiple internal units that outgrew co-location (split when the file passes the size ceiling OR a unit gains a second consumer - otherwise co-location is good; don't split reflexively), helper closures that should be named units, call chains where each layer adds only forwarding.
- - **Adjacent plumbing** - run the dead-code, duplication, and consistency lenses over the plumbing from step 2: helpers with zero callers (callers went direct), unused cache/config keys, near-identical wiring files differing in a handful of values, mappings re-hardcoded per call site instead of using the target's own helpers, locale keys orphaned by UI changes, stale docs entries.
- - **Large & plumbed files** - rank the N largest files (target AND plumbing) and read them line-by-line even when under the ceiling: size correlates with responsibility accumulation. Separately ask of each pure-plumbing layer whether it earns its existence.
+ Read each target file once, fully. Record every finding as a line in the findings list. Skip a bullet only when it cannot apply.
- **Fan-out**: target ≤ ~10 files - hunt inline. Larger - dispatch parallel Explore/read-only subagents, each owning a **grouped bundle of lenses** over the whole target (e.g. ① dead code + over-engineering + dependency hygiene, ② duplication + consistency + convention violations, ③ coupling + data flow + nesting + micro-simplification, ④ architecture + under-abstraction + error handling + contract quality, ⑤ plumbing + large files + test suite). One agent per lens over-fragments the reading; one agent for everything loses the benefit of independent angles. Run the consumer/boundary map (step 2) as its own agent in the same batch. Each agent returns findings as `file:line - claim - evidence`, plus a keep-list of things it considered and cleared.
+ **Unused code**: exports nothing imports, props never used or always given the same value, state set but never read, unreachable branches, unused translation keys, assets, or style properties, commented-out code, wrappers with a single caller that add no behavior, generics only ever used with one type, a config layer read from one place, query functions or keys nobody imports, interfaces, base classes, or strategy patterns with a single implementation, tests for removed code, duplicated tests, unused mocks, outdated docs or feature index entries.
- ### 4. Rate
+ **Repeated code and mixed patterns**: near-identical functions, components, markup, or style blocks; data reshaping that redoes an existing util (check the project's utils first); copy-pasted loading, empty, and error blocks; the same constant written inline in several places; two ways of solving one problem inside the target (switch all to the one the project uses most); files past the size limit or doing too many jobs.
- Score each subfolder (or file, for small targets) /10 with a one-line justification. This makes the review scannable and directs refactor effort to the lowest scores.
+ **Control flow and function shape**: nested or chained ternaries, deep if/else that early returns would flatten, `else` after `return`, `if (x) return true; else return false`, negated conditions with swapped branches; boolean flag parameters that switch behavior (split the function), five or more parameters, different return shapes on different paths, one-line helpers called once (inline them); sequential awaits with no dependency between them, `.then` chains mixed with `await`, try/catch that only rethrows, `async` on functions that never await; `x ? x : y` where `??` fits, the same default applied at several layers, `null` and `undefined` both used for absence.
- ### 5. Verify adversarially
+ **Structure and data flow**: props passed through layers unchanged, many props that belong together in one object, the same data fetched or computed in several places, one request per item where a batch request exists, effects that copy data into state when it could be computed directly, lists that grow with no limit or pagination, a large library imported for one function, errors caught and ignored, missing error, loading, or empty states, timers or subscriptions never cleaned up, race conditions from outdated closures or missing awaits, `renderSomething()` helpers that should be components, markup nested four or more wrapper levels deep, importing another feature's internals, circular imports.
- Findings from a single reader are hypotheses, not facts.
+ **Comments, types, naming, text**: comments that describe the next line, restate the name, mention tasks or PRs, or mark removed code; divider comments like `// ---- Helpers ----`; long comments that should be one line saying why; comments explaining what confusing code does (rename or extract instead, then delete the comment); comments that contradict the code. Names that promise one thing while the code does another, booleans that don't read as yes/no questions, one concept under two names in the module, abbreviations nobody else uses. `any` or `unknown` casts, `!` assertions where a type check would do, anonymous object types written inline in signatures, hand-written types that duplicate what the code already infers or generates. Files sitting in the folder root that belong in a subfolder, filename prefixes that repeat the folder name, index files that only re-export. Hardcoded user-facing text, translation keys missing in some locales, missing alt or aria attributes. Anything that breaks the project rules summary.
- - Grep every "unused" claim yourself before scheduling a deletion; for anything on the invisible-callers list from step 2, demand positive evidence of deadness (e.g. the registration is itself dead), not absence of imports.
- - For risky findings - deletions, architecture changes, behavior-adjacent edits - spawn skeptic subagents prompted to **refute** the finding, not confirm it. A finding survives only if the skeptic fails to kill it. Batch skeptics in parallel; one skeptic can take several related findings.
- - A visual or structural double may be intentional (current-state vs next-state, A/B arms, per-tenant variants) - check semantics, not just similarity.
- - Mark anything unproven UNCERTAIN; it does not enter a phase until resolved or explicitly approved by the user.
+ **Packages and API endpoints** (only when the target owns a package manifest, endpoints, or config): packages with zero imports, two libraries doing one job, endpoints no client calls, response fields no consumer reads, feature flags that are always on or always off, environment variables nobody reads.
- ### 6. Plan in phases - each independently buildable and committable
+ Before a finding that deletes code becomes CONFIRMED, try to prove it wrong: a double render may be intentional, a "redundant" fetch may preload a cache on purpose. Before planning, re-run the search for a sample of the CONFIRMED deletions yourself.
- 1. **Delete dead code** (first, so later phases touch less).
- 2. **Pure moves/renames** via `git mv` - zero logic change, so history follows and review is trivial. Never mix moves with logic edits in one commit.
- 3. **Architecture & pattern changes** - reshape the design per the surviving architecture findings. Each must pass the **net-simplification test**: after the change there are fewer concepts, fewer layers, or fewer places to edit for a known kind of future change, and the diff's churn is proportionate to that win. A restructure that merely trades one shape for an equally complex one fails the test - drop it. Stage big reshapes as a sequence of small, individually-green commits (strangler-style: introduce the new seam, migrate callers, delete the old shape) rather than one big-bang diff.
- 4. **Dedupe extractions** - shared shells, helpers, hooks/mixins/traits.
- 5. **Smells & polish** - contracts/types, magic numbers, error handling, naming, consistency convergence.
+ ### 3. Rate and plan
- Order within each phase by blast radius, smallest first. If the safety net is thin (step 1), pull characterization tests forward as phase 0 for anything phase 3 will reshape.
+ Score each subfolder (or the whole target, if it has none) out of 10 with a one-line reason.
- ### 7. Execute - orchestrator + implementer subagents
+ Two phases, each with its own commit(s):
- - Small targets: implement inline, phase by phase.
- - Larger: delegate each phase to an implementer subagent with the exact finding list, the conventions from step 1, and the public-surface freeze list from step 2. Phases run **sequentially** (each builds on the last commit); within a phase, split across parallel subagents only when their file sets are disjoint.
- - After each phase the orchestrator - not the implementer - reviews the diff, runs the project's verification commands, and commits before dispatching the next phase. A red gate stops the line: fix or revert before proceeding, never stack a phase on a broken base.
+ 1. **Remove**: dead code and noise comments, then file moves and renames with `git mv` as a separate commit (never mix a move with a content edit).
+ 2. **Refactor**: structural changes, pulling repeated code into shared helpers, then polish (types, translations, unexplained numbers, error handling, comment rewrites).
- ### 8. Verify and report
+ Skip a phase with nothing in it. Gather everything that needs the user's decision into one question, asked now: new architectural pieces (providers, contexts, shared layers), deleting anything still UNCERTAIN, renaming or moving files that other code imports, changes to what the user sees. Do not stop again after this.
- After all phases: run the full gate once more, then exercise the affected flows end-to-end the way a user or caller would (run the app/CLI/tests-of-consumers, not just the compiler). Close with a short report: rating table (before scores), what changed per phase, net metrics (files, LOC, exports/public symbols before → after), findings deliberately kept, and anything left UNCERTAIN or out of scope.
+ ### 4. Execute and report
- ## Ask the user before
+ Run the project's check commands (typecheck, lint, tests, taken from the project docs) once per phase, not per file. Test screens or flows by hand only when a change affects what the user sees. Report: the rating table, line count before and after, and findings fixed, KEPT, and still UNCERTAIN.
- - Applying an architecture change whose net-simplification case is arguable, or that alters an API consumed outside the repo.
- - Deleting anything still marked UNCERTAIN.
- - Renaming/moving files consumed outside the target.
- - Any fix that changes externally observable behavior (error rendering, empty states, wire formats, CLI output, labels).
+ ## When to use agents
- Everything else - including net-simplifying pattern/architecture changes that preserve behavior and the public surface - proceeds without asking.
+ - **Under ~10 files**: do everything yourself, no agents.
+ - **Larger**: two reviewer agents, started at the same time. Reviewer 1 takes unused code and packages (mostly searching). Reviewer 2 takes repeated code, control flow, structure, and comments/types/naming/text (mostly reading). Each gets the rules summary, the file list, its bullets, and the line format. Each verifies its own claims and reports back only finding lines. Never one agent per bullet or per file.
+ - **Implementation**: one agent runs both phases in order, running the checks and committing after each. Review the full set of changes once at the end, not per phase.