--- name: code-review-expert description: Perform senior-grade code reviews covering SOLID principles, security vulnerabilities, performance hotspots, error handling, boundary conditions, and safe dead code removal. Use when asked to "review my code", "/code-review-expert", "run code review", "check for bugs", or "audit architecture". license: MIT metadata: author: sanyuan0704 repo: sanyuan0704/sanyuan-skills version: "1.0" ---

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

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

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

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

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

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

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

• 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)

• 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

• 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)

• 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

• 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

• 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

• 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)

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

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

• 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

• 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

• 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

• 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

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

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

• 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

• Swallowed exceptions: Empty catch blocks or catch with only logging
• ```javascript
• 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

• 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

• 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

• N+1 queries: Loop that makes a query per item instead of batch
• ```javascript
• // 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

• 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

• 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

• 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 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

• 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

• 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

// 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 ```