Code Review Expert

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.

Severity Levels

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

• Load references/solid-checklist.md for specific prompts.
• 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.

3) Removal candidates + iteration plan

• Load references/removal-plan.md for template.
• 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).

4) Security and reliability scan

• Load references/security-checklist.md for coverage.
• 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.

5) Code quality scan

• Load references/code-quality-checklist.md for coverage.
• 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.

6) Output format

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
1. **[file:line]** Brief title
  - Description of issue
  - Suggested fix

### P2 - Medium
2. (continue numbering across sections)
  - ...

### 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 confirmation

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: Do NOT implement any changes until user explicitly confirms. This is a review-first workflow.

Resources

references/

Code Review Expert

A comprehensive code review skill for AI agents. Performs structured reviews with a senior engineer lens, covering architecture, security, performance, and code quality.

Installation

npx skills add sanyuan0704/sanyuan-skills --path skills/code-review-expert

Features

• SOLID Principles - Detect SRP, OCP, LSP, ISP, DIP violations
• Security Scan - XSS, injection, SSRF, race conditions, auth gaps, secrets leakage
• Performance - N+1 queries, CPU hotspots, missing cache, memory issues
• Error Handling - Swallowed exceptions, async errors, missing boundaries
• Boundary Conditions - Null handling, empty collections, off-by-one, numeric limits
• Removal Planning - Identify dead code with safe deletion plans

Usage

After installation, simply run:

/code-review-expert

The skill will automatically review your current git changes.

Workflow

1. Preflight - Scope changes via git diff 2. SOLID + Architecture - Check design principles 3. Removal Candidates - Find dead/unused code 4. Security Scan - Vulnerability detection 5. Code Quality - Error handling, performance, boundaries 6. Output - Findings by severity (P0-P3) 7. Confirmation - Ask user before implementing fixes

Severity Levels

Structure

code-review-expert/
├── SKILL.md                 # Main skill definition
├── agents/
│   └── agent.yaml           # Agent interface config
└── references/
    ├── solid-checklist.md   # SOLID smell prompts
    ├── security-checklist.md    # Security & reliability
    ├── code-quality-checklist.md # Error, perf, boundaries
    └── removal-plan.md      # Deletion planning template

References

Each checklist provides detailed prompts and anti-patterns:

• solid-checklist.md - SOLID violations + common code smells
• security-checklist.md - OWASP risks, race conditions, crypto, supply chain
• code-quality-checklist.md - Error handling, caching, N+1, null safety
• removal-plan.md - Safe vs deferred deletion with rollback plans

License

MIT

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)?"

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]

---

Defer Removal (Plan Required)

Item: [Name/Description]

---

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)

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

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)

---

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