| disable-model-invocation | true |
| name | code-review |
| description | Aggressive maintainability review focused on structural simplification, deleting complexity, type safety compiler soundness, resource leaks, accessibility, localization, and concurrency. Activate this skill ANY TIME the user asks for a code review or when you need to perform a senior-level, thorough analysis of code changes.
|
Code Review
Harsh, expert code review focused on implementation quality, aggressive simplification, and long-term maintainability. Be ambitious. Do not merely identify local cleanup opportunities; actively search for "code judo" moves that make the implementation dramatically simpler and more elegant.
Core Principles
- Code Judo: Actively look for restructurings that delete complexity rather than rearranging it. Reframe the problem so branches, helpers, or layers disappear entirely.
- Zero Spaghetti: Ban ad-hoc conditionals and scattered special cases in unrelated flows. Push logic into dedicated abstractions or state machines.
- Direct & Boring: Prefer explicit, legible code over hacky, magical, or thin abstractions that add indirection without value.
- Strict Boundaries: Enforce clean type contracts. Flag unnecessary optionality,
any, unknown, or excessive casting. Banish as any unless explicitly permitted.
- Canonical Home: Put logic in the correct layer and reuse existing utilities instead of building bespoke ones.
Workflow
- Context: Read the commit message (if any) and identify staged/unstaged changes. When diffing against a base branch, use
git diff <base>...HEAD (three-dot range) rather than git diff $(git merge-base <base> HEAD) HEAD — the three-dot form avoids a subshell so it matches the Bash permission allowlist without extra prompts.
- Scope: Review each changed file statically (do not build or run tests). Inspect the diff and read unchanged files (signatures, constants, types) to understand the full impact.
- Apply Checklists: For each file, evaluate changes against the checklists below.
- Report: Synthesize observations into a single cohesive report, grouped by impact (critical bugs, architectural, minor).
Review Checklist
Do not approve merely because the code works. Apply the following checks:
1. Code Smells Baseline (Fowler Refactoring, Ch. 3)
Review the diff against this checklist of common smells. Each smell is a judgment call, never a hard violation. A documented repository standard overrides this baseline.
- Mysterious Name — Function, variable, or type name doesn't reveal intent. → Suggest a rename.
- Primitive Obsession — Raw strings/primitives used for domain concepts. → Suggest enums, union types, or custom types.
- Data Clumps — Same groups of parameters/fields travel together. → Suggest bundling into an object or type.
- Duplicated Code — Same logic shape appears in multiple places. → Suggest extracting a shared helper.
- Shotgun Surgery — A single logical change forces edits across many files. → Suggest grouping them into one module.
- Divergent Change — A module is edited for multiple unrelated reasons. → Suggest splitting the module.
- Feature Envy — A method accesses another object's data more than its own. → Suggest moving the method onto the envied object.
- Speculative Generality — Hooks/parameters added for future needs. → Suggest deleting/inlining until a real need arises.
- Message Chains — Long navigation chains (e.g.
a.b().c()). → Suggest hiding the traversal behind a single delegate method.
- Middle Man — A class/function mostly delegates directly to another. → Suggest removing the middle man.
- Refused Bequest — Subclass ignores/overrides inherited features. → Suggest composition over inheritance.
- Repeated Switches — Same
switch/if cascade on a type is repeated. → Suggest polymorphism or a shared mapping.
2. Quality, Robustness & Maintainability
Reject the change if it introduces:
- Tangled Flow — Ad-hoc branching or complex conditional trees. → Suggest guard clauses or state machines.
- Scattered Logic — Feature-specific logic placed in shared/common modules. → Suggest moving it to the feature domain.
- Weak Abstraction — Indirection that doesn't simplify or reuse code. → Suggest inlining or reverting to direct calls.
- Redundant State — Duplicate state or cached values that can be derived. → Suggest deriving values dynamically.
- Parameter Sprawl — Function parameter list grows excessively. → Suggest bundling into a config object.
- Stringly-typed Code — Raw strings used instead of constants/unions. → Suggest converting to enums or string union types.
- Loose Typing — Unjustified usage of
any, unknown, or casting. → Suggest precise types or type guards.
- Type Boundary Drift — Deserialization/API boundaries are not validated. → Suggest schema validation (e.g., Zod) or type assertions at the boundary.
- Resource Leak — Timers, event listeners, or streams registered without cleanup. → Suggest adding cleanup in teardown hooks (e.g.,
ngOnDestroy).
- Connection Leak — DB/network handles left open in error paths. → Suggest closing handles in
finally blocks.
- Robustness Flaw — Unhandled boundary cases (null, empty, large payloads) or ungraceful crash on network/file failures. → Suggest guard clauses and graceful fallback handling.
- Unsanitized Input — User/external inputs not escaped. → Suggest escaping control characters (e.g.
*, _, ` in markdown; <, >, & in HTML).
- Doc Drift — Public API changes lack corresponding JSDoc updates. → Suggest updating documentation.
- Dependency Issues — Missing or unused dependencies in build configs. → Suggest resolving dependencies.
3. Code Reuse & Integration
Flag these opportunities:
- Duplicate Utility — Logic duplicates existing helper functions. → Suggest using the existing helper.
- Hand-rolled Utility — Inline code implements common utility tasks (path handling, environment/flag checks). → Suggest using standard codebase utilities.
- Search Rule — generic-looking code patterns are added without checking. → Actively search for existing helpers using
grep_search.
4. Efficiency & Performance
Reject the change if it introduces:
- Waste — Redundant computations, repeated file reads, or duplicate API calls. → Suggest caching or batching.
- Sequential Async — Independent async calls run sequentially. → Suggest combining via
Promise.all or Promise.allSettled.
- Hot-Path Bloat — Blocking tasks in startup, request handlers, or render loops. → Suggest async processing or lazy loading.
- TOCTOU Check — Pre-checking resource existence before operating (e.g. checking file existence). → Suggest operating directly and catching errors (e.g. ENOENT).
- Broad Scopes — Reading/loading entire files/datasets when filtering is possible. → Suggest scoped reads or query-level filtering.
5. Unit Test Quality (If tests changed)
Reject the change if it introduces:
- Mock Drift — Mocks that drift from real signatures or use generic
any structures. → Suggest type-safe mock frameworks.
- Bespoke Mocks — Manual component/DOM construction in tests. → Suggest reusing testing factories or helpers.
- Weak Assertions — Assertions that are trivial or verify properties unrelated to the behavior under test. → Suggest precise assertions verifying behavioral side-effects.
- Bloated Setup — Repeated boilerplate/mock setups across tests. → Suggest extracting to
beforeEach hooks or shared helpers.
- Inconsistent Style — Test structure diverges from surrounding tests. → Suggest aligning with local patterns.
- Missing Coverage — New complex logic or branches added without tests. → Reject change and request new unit tests.
