code-review

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

1. Code Judo: Actively look for restructurings that delete complexity rather than rearranging it. Reframe the problem so branches, helpers, or layers disappear entirely.
2. Zero Spaghetti: Ban ad-hoc conditionals and scattered special cases in unrelated flows. Push logic into dedicated abstractions or state machines.
3. Direct & Boring: Prefer explicit, legible code over hacky, magical, or thin abstractions that add indirection without value.
4. Strict Boundaries: Enforce clean type contracts. Flag unnecessary optionality, any, unknown, or excessive casting. Banish as any unless explicitly permitted.
5. Canonical Home: Put logic in the correct layer and reuse existing utilities instead of building bespoke ones.

Workflow

1. 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.
2. 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.
3. Apply Checklists: For each file, evaluate changes against the checklists below.
4. 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.