code-review-expert

name	code-review-expert
description	Expert automated code review with SOLID, security, and quality checks. Supports --auto mode for CI/pre-push hooks. Use /code-review-expert or /code-review-expert --auto
license	MIT
tags	["code-review","security","solid","automation","quality"]

Code Review Expert

Overview

Perform a structured review of the current git changes with focus on SOLID, architecture, removal candidates, and security risks. Default to review-only output unless the user asks to implement changes.

Note: Lint, typecheck, format は lefthook (pre-commit/pre-push) と guard.sh (Hook 4) で自動実行されるため、本スキルでは省略しています。

Severity Levels

Level	Name	Description	Action
P0	Critical	Security vulnerability, data loss risk, correctness bug	Must block merge
P1	High	Logic error, significant SOLID violation, performance regression	Should fix before merge
P2	Medium	Code smell, maintainability concern, minor SOLID violation	Fix in this PR or create follow-up
P3	Low	Style, naming, minor suggestion	Optional improvement

Workflow

1) Preflight context

• Use git status -sb, git diff --stat, and git diff to scope changes.
• If needed, use rg or grep to find related modules, usages, and contracts.
• Identify entry points, ownership boundaries, and critical paths (auth, payments, data writes, network).

Edge cases:

• No changes: If git diff is empty, inform user and ask if they want to review staged changes or a specific commit range.
• Large diff (>500 lines): Summarize by file first, then review in batches by module/feature area.
• Mixed concerns: Group findings by logical feature, not just file order.

2) SOLID + architecture smells

Look for:

• SRP: Overloaded modules with unrelated responsibilities.
• OCP: Frequent edits to add behavior instead of extension points.
• LSP: Subclasses that break expectations or require type checks.
• ISP: Wide interfaces with unused methods.
• DIP: High-level logic tied to low-level implementations.

When you propose a refactor, explain why it improves cohesion/coupling and outline a minimal, safe split. If refactor is non-trivial, propose an incremental plan instead of a large rewrite.

SOLID Smell Prompts

SRP (Single Responsibility)

• File owns unrelated concerns (e.g., HTTP + DB + domain rules in one file)
• Large class/module with low cohesion or multiple reasons to change
• Functions that orchestrate many unrelated steps
• God objects that know too much about the system
• Ask: "What is the single reason this module would change?"

OCP (Open/Closed)

• Adding a new behavior requires editing many switch/if blocks
• Feature growth requires modifying core logic rather than extending
• No plugin/strategy/hook points for variation
• Ask: "Can I add a new variant without touching existing code?"

LSP (Liskov Substitution)

• Subclass checks for concrete type or throws for base method
• Overridden methods weaken preconditions or strengthen postconditions
• Subclass ignores or no-ops parent behavior
• Ask: "Can I substitute any subclass without the caller knowing?"

ISP (Interface Segregation)

• Interfaces with many methods, most unused by implementers
• Callers depend on broad interfaces for narrow needs
• Empty/stub implementations of interface methods
• Ask: "Do all implementers use all methods?"

DIP (Dependency Inversion)

• High-level logic depends on concrete IO, storage, or network types
• Hard-coded implementations instead of abstractions or injection
• Import chains that couple business logic to infrastructure
• Ask: "Can I swap the implementation without changing business logic?"

Common Code Smells (Beyond SOLID)

Smell	Signs
Long method	Function > 30 lines, multiple levels of nesting
Feature envy	Method uses more data from another class than its own
Data clumps	Same group of parameters passed together repeatedly
Primitive obsession	Using strings/numbers instead of domain types
Shotgun surgery	One change requires edits across many files
Divergent change	One file changes for many unrelated reasons
Dead code	Unreachable or never-called code
Speculative generality	Abstractions for hypothetical future needs
Magic numbers/strings	Hardcoded values without named constants

Refactor Heuristics

1. Split by responsibility, not by size - A small file can still violate SRP
2. Introduce abstraction only when needed - Wait for the second use case
3. Keep refactors incremental - Isolate behavior before moving
4. Preserve behavior first - Add tests before restructuring
5. Name things by intent - If naming is hard, the abstraction might be wrong
6. Prefer composition over inheritance - Inheritance creates tight coupling
7. Make illegal states unrepresentable - Use types to enforce invariants

3) Removal candidates + iteration plan

Identify code that is unused, redundant, or feature-flagged off. Distinguish safe delete now vs defer with plan. Provide a follow-up plan with concrete steps and checkpoints (tests/metrics).

Removal and Iteration Plan Template

Priority Levels

• P0: Immediate removal needed (security risk, significant cost, blocking other work)
• P1: Remove in current sprint
• P2: Backlog / next iteration

Safe to Remove Now

Item: [Name/Description]

Field	Details
Location	path/to/file.ts:line
Rationale	Why this should be removed
Evidence	Unused (no references), dead feature flag, deprecated API
Impact	None / Low - no active consumers
Deletion steps	1. Remove code 2. Remove tests 3. Remove config
Verification	Run tests, check no runtime errors, monitor logs

Defer Removal (Plan Required)

Item: [Name/Description]

Field	Details
Location	path/to/file.ts:line
Why defer	Active consumers, needs migration, stakeholder sign-off
Preconditions	Feature flag off for 2 weeks, telemetry shows 0 usage
Breaking changes	List any API/contract changes
Migration plan	Steps for consumers to migrate
Timeline	Target date or sprint
Owner	Person/team responsible
Validation	Metrics to confirm safe removal (error rates, usage counts)
Rollback plan	How to restore if issues found

Checklist Before Removal

• Searched codebase for all references (rg, grep)
• Checked for dynamic/reflection-based usage
• Verified no external consumers (APIs, SDKs, docs)
• Feature flag telemetry reviewed (if applicable)
• Tests updated/removed
• Documentation updated
• Team notified (if shared code)

4) Security and reliability scan

Check for:

• XSS, injection (SQL/NoSQL/command), SSRF, path traversal
• AuthZ/AuthN gaps, missing tenancy checks
• Secret leakage or API keys in logs/env/files
• Rate limits, unbounded loops, CPU/memory hotspots
• Unsafe deserialization, weak crypto, insecure defaults
• Race conditions: concurrent access, check-then-act, TOCTOU, missing locks

Call out both exploitability and impact.

Security and Reliability Checklist

Input/Output Safety

• XSS: Unsafe HTML injection, dangerouslySetInnerHTML, unescaped templates, innerHTML assignments
• Injection: SQL/NoSQL/command/GraphQL injection via string concatenation or template literals
• SSRF: User-controlled URLs reaching internal services without allowlist validation
• Path traversal: User input in file paths without sanitization (../ attacks)
• Prototype pollution: Unsafe object merging in JavaScript (Object.assign, spread with user input)

AuthN/AuthZ

• Missing tenant or ownership checks for read/write operations
• New endpoints without auth guards or RBAC enforcement
• Trusting client-provided roles/flags/IDs
• Broken access control (IDOR - Insecure Direct Object Reference)
• Session fixation or weak session management

JWT & Token Security

• Algorithm confusion attacks (accepting none or HS256 when expecting RS256)
• Weak or hardcoded secrets
• Missing expiration (exp) or not validating it
• Sensitive data in JWT payload (tokens are base64, not encrypted)
• Not validating iss (issuer) or aud (audience)

Secrets and PII

• API keys, tokens, or credentials in code/config/logs
• Secrets in git history or environment variables exposed to client
• Excessive logging of PII or sensitive payloads
• Missing data masking in error messages

Supply Chain & Dependencies

• Unpinned dependencies allowing malicious updates
• Dependency confusion (private package name collision)
• Importing from untrusted sources or CDNs without integrity checks
• Outdated dependencies with known CVEs

CORS & Headers

• Overly permissive CORS (Access-Control-Allow-Origin: * with credentials)
• Missing security headers (CSP, X-Frame-Options, X-Content-Type-Options)
• Exposed internal headers or stack traces

Runtime Risks

• Unbounded loops, recursive calls, or large in-memory buffers
• Missing timeouts, retries, or rate limiting on external calls
• Blocking operations on request path (sync I/O in async context)
• Resource exhaustion (file handles, connections, memory)
• ReDoS (Regular Expression Denial of Service)

Cryptography

• Weak algorithms (MD5, SHA1 for security purposes)
• Hardcoded IVs or salts
• Using encryption without authentication (ECB mode, no HMAC)
• Insufficient key length

Race Conditions

Race conditions are subtle bugs that cause intermittent failures and security vulnerabilities. Pay special attention to:

Shared State Access

• Multiple threads/goroutines/async tasks accessing shared variables without synchronization
• Global state or singletons modified concurrently
• Lazy initialization without proper locking (double-checked locking issues)
• Non-thread-safe collections used in concurrent context

Check-Then-Act (TOCTOU)

• if (exists) then use patterns without atomic operations
• if (authorized) then perform where authorization can change
• File existence check followed by file operation
• Balance check followed by deduction (financial operations)
• Inventory check followed by order placement

Database Concurrency

• Missing optimistic locking (version column, updated_at checks)
• Missing pessimistic locking (SELECT FOR UPDATE)
• Read-modify-write without transaction isolation
• Counter increments without atomic operations (UPDATE SET count = count + 1)
• Unique constraint violations in concurrent inserts

Distributed Systems

• Missing distributed locks for shared resources
• Leader election race conditions
• Cache invalidation races (stale reads after writes)
• Event ordering dependencies without proper sequencing
• Split-brain scenarios in cluster operations

Common Patterns to Flag

# Dangerous patterns:
if not exists(key):       # TOCTOU
    create(key)

value = get(key)          # Read-modify-write
value += 1
set(key, value)

if user.balance >= amount:  # Check-then-act
    user.balance -= amount

Questions to Ask

• "What happens if two requests hit this code simultaneously?"
• "Is this operation atomic or can it be interrupted?"
• "What shared state does this code access?"
• "How does this behave under high concurrency?"

Data Integrity

• Missing transactions, partial writes, or inconsistent state updates
• Weak validation before persistence (type coercion issues)
• Missing idempotency for retryable operations
• Lost updates due to concurrent modifications

5) Code quality scan

Check for:

• Error handling: swallowed exceptions, overly broad catch, missing error handling, async errors
• Performance: N+1 queries, CPU-intensive ops in hot paths, missing cache, unbounded memory
• Boundary conditions: null/undefined handling, empty collections, numeric boundaries, off-by-one

Flag issues that may cause silent failures or production incidents.

Code Quality Checklist

Error Handling

Anti-patterns to Flag

• Swallowed exceptions: Empty catch blocks or catch with only logging

  try { ... } catch (e) { }  // Silent failure
  try { ... } catch (e) { console.log(e) }  // Log and forget
  

• Overly broad catch: Catching Exception/Error base class instead of specific types
• Error information leakage: Stack traces or internal details exposed to users
• Missing error handling: No try-catch around fallible operations (I/O, network, parsing)
• Async error handling: Unhandled promise rejections, missing .catch(), no error boundary

Best Practices to Check

• Errors are caught at appropriate boundaries
• Error messages are user-friendly (no internal details exposed)
• Errors are logged with sufficient context for debugging
• Async errors are properly propagated or handled
• Fallback behavior is defined for recoverable errors
• Critical errors trigger alerts/monitoring

Questions to Ask

• "What happens when this operation fails?"
• "Will the caller know something went wrong?"
• "Is there enough context to debug this error?"

Performance & Caching

CPU-Intensive Operations

• Expensive operations in hot paths: Regex compilation, JSON parsing, crypto in loops
• Blocking main thread: Sync I/O, heavy computation without worker/async
• Unnecessary recomputation: Same calculation done multiple times
• Missing memoization: Pure functions called repeatedly with same inputs

Database & I/O

• N+1 queries: Loop that makes a query per item instead of batch

  // Bad: N+1
  for (const id of ids) {
    const user = await db.query(`SELECT * FROM users WHERE id = ?`, id)
  }
  // Good: Batch
  const users = await db.query(`SELECT * FROM users WHERE id IN (?)`, ids)
  

• Missing indexes: Queries on unindexed columns
• Over-fetching: SELECT * when only few columns needed
• No pagination: Loading entire dataset into memory

Caching Issues

• Missing cache for expensive operations: Repeated API calls, DB queries, computations
• Cache without TTL: Stale data served indefinitely
• Cache without invalidation strategy: Data updated but cache not cleared
• Cache key collisions: Insufficient key uniqueness
• Caching user-specific data globally: Security/privacy issue

Memory

• Unbounded collections: Arrays/maps that grow without limit
• Large object retention: Holding references preventing GC
• String concatenation in loops: Use StringBuilder/join instead
• Loading large files entirely: Use streaming instead

Questions to Ask

• "What's the time complexity of this operation?"
• "How does this behave with 10x/100x data?"
• "Is this result cacheable? Should it be?"
• "Can this be batched instead of one-by-one?"

Boundary Conditions

Null/Undefined Handling

• Missing null checks: Accessing properties on potentially null objects
• Truthy/falsy confusion: if (value) when 0 or "" are valid
• Optional chaining overuse: a?.b?.c?.d hiding structural issues
• Null vs undefined inconsistency: Mixed usage without clear convention

Empty Collections

• Empty array not handled: Code assumes array has items
• Empty object edge case: for...in or Object.keys on empty object
• First/last element access: arr[0] or arr[arr.length-1] without length check

Numeric Boundaries

• Division by zero: Missing check before division
• Integer overflow: Large numbers exceeding safe integer range
• Floating point comparison: Using === instead of epsilon comparison
• Negative values: Index or count that shouldn't be negative
• Off-by-one errors: Loop bounds, array slicing, pagination

String Boundaries

• Empty string: Not handled as edge case
• Whitespace-only string: Passes truthy check but is effectively empty
• Very long strings: No length limits causing memory/display issues
• Unicode edge cases: Emoji, RTL text, combining characters

Common Patterns to Flag

// Dangerous: no null check
const name = user.profile.name

// Dangerous: array access without check
const first = items[0]

// Dangerous: division without check
const avg = total / count

// Dangerous: truthy check excludes valid values
if (value) { ... }  // fails for 0, "", false

Questions to Ask

• "What if this is null/undefined?"
• "What if this collection is empty?"
• "What's the valid range for this number?"
• "What happens at the boundaries (0, -1, MAX_INT)?"

6) Output format

Check --auto flag FIRST to decide output format.

Auto mode output (--auto flag present):

Use this compact format only. Do NOT use the full format below. Every token saved here is a token available for commit/push/PR steps that MUST follow.

Review: X files, Y lines
P0: 0 | P1: 0 | P2: N | P3: N
[If P0/P1 exist, list ONLY those — one line each: file:line — description]

That's it. No P2/P3 details, no removal plan, no suggestions, no markdown headers. Maximum 10 lines.

Interactive mode output (no --auto flag):

Structure your review as follows:

## Code Review Summary

**Files reviewed**: X files, Y lines changed
**Overall assessment**: [APPROVE / REQUEST_CHANGES / COMMENT]

---

## Findings

### P0 - Critical
(none or list)

### P1 - High
- **[file:line]** Brief title
  - Description of issue
  - Suggested fix

### P2 - Medium
...

### P3 - Low
...

---

## Removal/Iteration Plan
(if applicable)

## Additional Suggestions
(optional improvements, not blocking)

Inline comments: Use this format for file-specific findings:

::code-comment{file="path/to/file.ts" line="42" severity="P1"}
Description of the issue and suggested fix.
::

Clean review: If no issues found, explicitly state:

• What was checked
• Any areas not covered (e.g., "Did not verify database migrations")
• Residual risks or recommended follow-up tests

7) Next steps — AUTO vs INTERACTIVE

STEP 1: Check if --auto flag is present.

If --auto IS present (auto mode):

1. If P0 or P1 issues exist -> fix them immediately, no questions asked
2. If NO P0/P1 issues -> output the summary
3. MANDATORY: Write review marker file (both with and without P0/P1 fixes):

   git rev-parse HEAD > .code-review-done
   

   This enables the guard.sh Hook 6 pre-push check. Run this command immediately after confirming P0/P1 = 0.
4. MANDATORY RULES for auto mode:
   • Do NOT use AskUserQuestion under any circumstances
   • Do NOT display the "Next Steps" / "How would you like to proceed?" template below
   • Do NOT ask for confirmation, approval, or "go ahead"
   • Do NOT wait for user input
   • After outputting the summary (and fixing P0/P1 if any), the review phase is complete. Immediately continue with whatever remaining steps your current workflow requires (e.g., git add, commit, push, PR creation, report). Do NOT stop here.

If --auto is NOT present (interactive mode):

After presenting findings, ask user how to proceed:

---

## Next Steps

I found X issues (P0: _, P1: _, P2: _, P3: _).

**How would you like to proceed?**

1. **Fix all** - I'll implement all suggested fixes
2. **Fix P0/P1 only** - Address critical and high priority issues
3. **Fix specific items** - Tell me which issues to fix
4. **No changes** - Review complete, no implementation needed

Please choose an option or provide specific instructions.

Important (interactive mode only): Do NOT implement any changes until user explicitly confirms. This is a review-first workflow.

When the user confirms "no changes" or all fixes are complete (P0/P1 = 0), write the marker file:

git rev-parse HEAD > .code-review-done