Performs systematic code review with universal best practices and repo-specific standards. Auto-activates after significant code changes. Use when reviewing code, auditing files, checking PRs, examining staged changes, or when asked to "review", "check", "audit", or "examine" code. Enforces design principles (SOLID, DRY, KISS), security (OWASP), performance, concurrency safety, cross-platform compatibility, and codebase patterns. Use when this capability is needed.
Systematic code review skill based on industry best practices from Google Engineering, OWASP, and modern development standards. Designed to catch issues that casual review misses through structured, checklist-driven analysis.
Core Principle (Google): Approve code that improves overall code health, even if not perfect. Seek continuous improvement, not perfection. But NEVER approve code that degrades code health.
When to Use This Skill
Auto-activation triggers:
After completing significant implementation tasks
Before committing changes
When reviewing PRs or staged files
Explicit activation triggers:
User asks to "review", "check", "audit", or "examine" code
User mentions "code review", "PR review", "look at this code"
User asks about code quality or standards compliance
Interactive Review Scoping
Use AskUserQuestion to determine review depth and risk profile when not specified:
# Check if config exists
ls .claude/code-review.md 2>/dev/null || echo "No config, will use CLAUDE.md fallback"
# Preview what config will be loaded (via review command)
/code-quality:review --show-rules
Agent Applicability:
ALL agents loading the code-reviewing skill MUST execute Step 0b. This includes
code-reviewer, quality-reviewer, and security-reviewer. Each agent independently
loads and applies repo config within its own domain.
Security Exclusion Advisory:
When repo config excludes security-related rules, agents MUST emit an ADVISORY note:
"ADVISORY: Security rule '{rule}' excluded by repo config. Ensure intentional."
This is informational only -- not a finding, not blocking.
Step 0e: Git History Analysis
Triggered by:thorough or strict profile (partial for security/performance)
Before reviewing, count the files to ensure accurate reporting:
Review Scope
Counting Command
Staged changes
git diff --staged --name-only | wc -l
PR changes
git diff --name-only main...HEAD | wc -l
Specific paths
Use Glob tool and count results
Important:
Track this count accurately - report it in Files Reviewed output
Exclude binary files (images, compiled assets) from review count
Exclude deleted files from review count (nothing to review)
For renamed files: count as 1 file (the new name)
Large Changeset Warnings:
50+ files: Consider batched review or consensus mode
100+ files: Strongly recommend breaking into smaller chunks
Step 1b: Detect Generated Content
Scan changed files for generation markers to identify files that were created by scripts/tools. Review the generator (source of truth) instead of generated output.
Detection Markers (scan first 20 + last 10 lines):
File Type
Markers
Markdown
Generated: YYYY-MM-DD, <!-- Generated by ScriptName -->
When --baseline <branch> is specified, attribute each finding as NEW or PRE-EXISTING.
Why This Matters:
This is the #1 adoption blocker for code review tools. Teams with tech debt get overwhelmed by 100+ findings when their PR only introduces 3 new issues. They give up on the tool.
Baseline Mode Workflow:
Build attribution map from git diff:
git diff <baseline>...HEAD # Three-dot for symmetric comparison
Parse diff to identify changed line ranges per file
For each finding: Check if file:line is in changed ranges
Tag findings: NEW (line added/modified) or PRE-EXISTING (unchanged)
Separate output: New issues prominent, pre-existing collapsed
Finding Attribution Rules:
Line Status
Attribution
Output Section
Added (+ in diff)
NEW
"New Issues Introduced"
Modified (in hunk, changed)
NEW
"New Issues Introduced"
Context (in hunk, unchanged)
PRE-EXISTING
"Pre-existing Issues" (collapsed)
Not in any hunk
PRE-EXISTING
"Pre-existing Issues" (collapsed)
Deleted (- in diff)
SKIP
Not reviewed (doesn't exist)
Baseline Output Format:
## Review Summary (Baseline Mode)
**Baseline**: `main`
**New Issues (this changeset)**: 2 CRITICAL, 1 MAJOR ← FOCUS HERE
**Pre-existing Issues**: 47 (collapsed)
**Assessment**: Based on NEW issues only
## New Issues Introduced
[Detailed findings with Attribution: NEW]
<details>
<summary>Pre-existing Issues (47)</summary>
[Condensed findings]
</details>
Quality Gates (CI/CD):
--fail-on-new-critical: Exit 1 if ANY new CRITICAL
--fail-on-new-major: Exit 1 if ANY new MAJOR
Pre-existing issues do NOT fail the build
Edge Cases:
Baseline doesn't exist → Error with valid branch suggestions
File renamed → Attribute based on content changes
Merge commits → Use three-dot diff for correct comparison
Complexity Thresholds
Use these concrete thresholds when evaluating code complexity:
Metric
Warning
Error
Measurement
Cyclomatic Complexity
> 10
> 20
Count decision points (if, while, for, case, &&, ||, ?)
Cognitive Complexity
> 15
> 30
SonarQube metric (nesting adds more weight)
Function Lines
> 30
> 50
Count non-blank, non-comment lines
Nesting Depth
> 3
> 5
Count nested braces/indentation levels
Parameter Count
> 4
> 7
Count function/method parameters
File Lines
> 300
> 500
Count total lines in file
When thresholds exceeded:
Warning → MINOR severity finding
Error → MAJOR severity finding
Suggested Fix Template:
### Complexity Threshold Exceeded
**File**: `src/services/order.ts:processOrder()`
**Severity**: MAJOR
**Metric**: Cyclomatic Complexity = 23 (threshold: 15, error: 20)
**Problem**: Function has 23 independent paths through the code.
**Impact**: Difficult to test (need 23+ test cases), hard to maintain, high bug risk.
**Fix**: Extract methods for logical branches. Consider:
- Extract validation logic → `validateOrder()`
- Extract pricing logic → `calculatePrice()`
- Extract notification logic → `notifyCustomer()`
Progressive Loading (Token Optimization)
This skill uses tiered progressive disclosure to optimize token usage. Only load what's relevant to the files being reviewed.
Tier 0 (MANDATORY - Always First): Research Phase (~2,000 tokens)
ALWAYS load BEFORE analysis: references/tier-0/research-phase.md
Tier 4 (Repository-Specific): Load when repo config or CLAUDE.md exists
Context
Load Reference
.claude/code-review.md exists
references/tier-4/repo-config.md
CLAUDE.md exists (always)
references/tier-4/claude-md-core.md
*.md files
references/tier-4/documentation-rules.md
Duplication indicators
references/tier-4/anti-duplication-rules.md
Path patterns detected
references/tier-4/path-rules.md
*-{platform}.md files
references/tier-4/platform-rules.md
.claude/skills/**
references/tier-4/skill-rules.md
.claude/memory/**
references/tier-4/memory-rules.md
.claude/temp/**
references/tier-4/temp-file-rules.md
Profile thorough/strict
references/tier-4/pattern-compliance.md
Tier 4b (Claude Code Components): Delegate to specialized auditors when CC files detected
File Pattern
Component Type
Auditor Agent
.claude/agents/**/*.md, agents/*.md
Agent
claude-ecosystem:agent-auditor
.claude/skills/**, skills/*/SKILL.md
Skill
claude-ecosystem:skill-auditor
.claude/hooks/**, hooks.json
Hook
claude-ecosystem:hook-auditor
.claude/skills/**
Skill
claude-ecosystem:skill-auditor
CLAUDE.md, .claude/memory/**
Memory
claude-ecosystem:memory-component-auditor
output-styles/*.md
Output Style
claude-ecosystem:output-style-auditor
.mcp.json, mcp.json
MCP Config
claude-ecosystem:mcp-auditor
settings.json, .claude/settings.json
Settings
claude-ecosystem:settings-auditor
plugin.json, .claude-plugin/**
Plugin
claude-ecosystem:plugin-component-auditor
Status line scripts
Status Line
claude-ecosystem:statusline-auditor
Load the reference for complete detection and delegation patterns: references/tier-4/claude-code-components.md
Note: Tier 4b requires the claude-ecosystem plugin to be installed. If not installed, log a warning and skip CC-specific validation (continue with standard review).
Auditor Agent Verification: Agent names may change between plugin releases. Run /claude-ecosystem:list skills
to verify current auditor agent names if delegation fails. The table above reflects known agents as of this file's last update.
Tier 5 (Generated Content & MCP Validation Details): Load based on changeset characteristics
Trigger
Load Reference
Large changeset (50+ files)
references/tier-5/generated-content-detection.md
Generation markers found
references/tier-5/generated-content-detection.md
Need advanced MCP query patterns
references/tier-5/mcp-validation.md
Complex technology stack
references/tier-5/mcp-validation.md
Security patterns require OWASP validation
references/tier-5/mcp-validation.md
Note: Primary MCP validation happens in Tier 0 (Research Phase) which is MANDATORY. Tier 5 mcp-validation.md provides additional query templates and advanced patterns for complex scenarios.
Note: Token budgets increased by ~2,000 tokens due to MANDATORY Tier 0 Research Phase. This trade-off provides significantly more accurate reviews through MCP validation.
Layer 1: Universal Code Review Checklist
These checks apply to ANY codebase, ANY language.
1.1 Design and Architecture
Overall design makes sense - Interactions between components are logical
Belongs in this codebase - Not better suited for a library or different module
Integrates well - Fits with existing system architecture
Right time for this change - Not premature or addressing wrong problem
No over-engineering - Solves current problem, not speculative future needs (YAGNI)
SOLID Principles:
Single Responsibility (SRP) - Each class/function has ONE reason to change
Open-Closed (OCP) - Open for extension, closed for modification (no repeated if/else for types)
Liskov Substitution (LSP) - Derived classes substitutable for base (no explicit type casting)
Interface Segregation (ISP) - Clients depend only on methods they use
Dependency Inversion (DIP) - Depend on abstractions, not concrete implementations
1.2 Functionality and Logic
Does what it's supposed to do - Implements intended functionality
Good for users - Both end-users AND developers who'll use this code
Tests maintainable - Not overly complex, clear assertions
No flaky tests - Deterministic, not timing-dependent
Test Smell Detection:
Review test code for common test smells that reduce test quality and maintainability:
Smell
Detection
Severity
Why It Matters
Empty Test
Test method with no assertions
CRITICAL
False confidence - test always passes
Eager Test
> 5 assertions in one test
WARNING
When it fails, unclear which behavior broke
Mystery Guest
External file/network access without mock
MAJOR
Flaky, slow, environment-dependent
Assertion Roulette
Multiple asserts without descriptive messages
MINOR
Hard to identify which assertion failed
Sleep in Test
Thread.Sleep, await Task.Delay, setTimeout
MAJOR
Slow, flaky, hides timing bugs
Dead Test
Test that never fails (always passes)
WARNING
Usually tests nothing meaningful
Commented Test
Disabled test cases ([Ignore], skip, xtest)
WARNING
Technical debt, may hide real issues
Test Code Duplication
Same setup/teardown in multiple tests
MINOR
Maintenance burden, use fixtures
Example Findings:
### Test Smell: Eager Test
**File**: `tests/UserService.test.ts:45`
**Severity**: WARNING
**Confidence**: MEDIUM
**Problem**: Test "should handle user operations" has 12 assertions testing multiple behaviors.
**Impact**: When this test fails, you won't know which behavior broke without debugging.
**Suggested Fix**:
```typescript
// Before (Eager Test - 12 assertions, multiple behaviors)
test('should handle user operations', () => {
const user = createUser();
expect(user.name).toBe('John');
expect(user.email).toContain('@');
// ... 10 more assertions about different behaviors
});
// After (Focused Tests - one behavior each)
test('should create user with valid name', () => {
const user = createUser();
expect(user.name).toBe('John');
});
test('should validate email format', () => {
const user = createUser();
expect(user.email).toContain('@');
});
```markdown
### Test Smell: Sleep in Test
**File**: `tests/api.test.ts:78`
**Severity**: MAJOR
**Confidence**: HIGH
**Problem**: Test uses `await new Promise(r => setTimeout(r, 2000))` to wait for async operation.
**Impact**: Test is slow (2 seconds), flaky (timing varies), and hides real async handling bugs.
**Suggested Fix**:
```typescript
// Before (Sleep - slow and flaky)
await new Promise(r => setTimeout(r, 2000));
expect(result).toBeDefined();
// After (Proper async handling)
await waitFor(() => expect(result).toBeDefined());
// Or
await expect(asyncOperation()).resolves.toBeDefined();
### 1.8 Error Handling and Logging
- [ ] **Errors caught appropriately** - Right level of granularity
- [ ] **Error messages actionable** - Clear what went wrong and how to fix
- [ ] **Logging present** - For debugging and troubleshooting
- [ ] **No sensitive data in logs** - PII, passwords, keys excluded
- [ ] **Graceful degradation** - Partial failures don't crash entire system
### 1.9 Documentation
- [ ] **Code documented** - Public APIs, complex logic explained
- [ ] **README updated** - If behavior/setup changes
- [ ] **API docs updated** - If endpoints change
- [ ] **Inline comments where needed** - For non-obvious decisions
### 1.10 Cross-Platform Compatibility
- [ ] **No hardcoded platform paths**:
- `/mnt/c/Users/...` (WSL)
- `/c/Users/...` (Git Bash)
- `C:\Users\...` (Windows)
- `/home/username/...` (Linux)
- `/Users/username/...` (macOS)
- [ ] **Portable tool detection** - `command -v tool` not path hunting
- [ ] **Platform fallbacks** - Graceful handling when features unavailable
- [ ] **Scripts self-locate** - Use `Path(__file__).resolve()`, `$PSScriptRoot`, `${BASH_SOURCE[0]}`
### 1.11 Anti-Duplication
- [ ] **No duplicate content** - Same info in ONE place only
- [ ] **No identical files** - `diff` similar files to verify
- [ ] **Single source of truth** - Link instead of copy-paste
- [ ] **Config files distinct** - Each serves different purpose
### 1.12 Style and Consistency
- [ ] **Follows style guide** - Language/project conventions
- [ ] **Consistent with codebase** - Matches existing patterns
- [ ] **No style changes mixed with logic** - Separate formatting PRs
### 1.13 Accessibility (WCAG 2.1 AA)
- [ ] **Alt text present** - All images have descriptive alt text; decorative images use `alt=""`
- [ ] **Color contrast sufficient** - 4.5:1 for text, 3:1 for UI components
- [ ] **Keyboard navigable** - All interactive elements via Tab/Enter/Space; no keyboard traps
- [ ] **Focus visible** - Clear focus indicators on all interactive elements
- [ ] **Semantic HTML** - Proper heading hierarchy; buttons not divs; links not spans
- [ ] **ARIA correct** - Used only when semantic HTML insufficient; no conflicting roles
### 1.14 Internationalization (i18n)
- [ ] **No hardcoded strings** - All user-facing text externalized to resource files
- [ ] **Locale-aware formatting** - Dates, numbers, currency use locale APIs
- [ ] **RTL consideration** - Logical CSS properties where applicable
- [ ] **No string concatenation** - Use parameterized messages, not `"Hello " + name`
- [ ] **Pluralization handled** - Proper plural rules, not `count + " items"`
### 1.15 Observability
- [ ] **Structured logging** - JSON format with trace IDs, timestamps, context
- [ ] **Metrics present** - Latency, error rates, throughput for critical paths
- [ ] **Trace context propagated** - Distributed tracing across service boundaries
- [ ] **Health checks implemented** - Liveness/readiness probes with dependency checks
- [ ] **SLOs defined** - Measurable service level objectives for key operations
### 1.16 Data Privacy (GDPR/CCPA)
- [ ] **PII identified and protected** - Personal data encrypted, access controlled
- [ ] **Data retention enforced** - Clear policies, automated cleanup
- [ ] **Right to deletion** - Complete erasure across all systems possible
- [ ] **Consent tracked** - Explicit opt-in with audit trail
- [ ] **No PII in logs** - Redaction or hashing of personal identifiers
### 1.17 API Design
- [ ] **Versioning strategy** - Clear version in URL, header, or media type
- [ ] **Backward compatible** - New fields nullable, no removed fields
- [ ] **Deprecation documented** - Sunset dates, migration paths
- [ ] **Consistent naming** - Follows REST/GraphQL conventions
- [ ] **Error responses standardized** - Consistent error format across endpoints
### 1.18 Dependency Management
- [ ] **No known vulnerabilities** - CVE scanning in CI/CD
- [ ] **License compliance** - No GPL conflicts with proprietary code
- [ ] **Version pinned** - Lockfiles present and up-to-date
- [ ] **Transitive deps reviewed** - Indirect dependencies also secure
- [ ] **SBOM available** - Software Bill of Materials for audits
### 1.19 Database Patterns
- [ ] **N+1 queries avoided** - Eager loading or batch queries used
- [ ] **Indexes present** - For foreign keys, join columns, query patterns
- [ ] **Migrations backward compatible** - Incremental changes, no data loss
- [ ] **Schema properly normalized** - Or denormalized with clear rationale
- [ ] **Query optimization** - Explain plans reviewed for complex queries
### 1.20 Configuration Management
- [ ] **Secrets in vault** - Never hardcoded, use env vars or secrets manager
- [ ] **Feature flags used** - For gradual rollouts, A/B testing
- [ ] **12-factor compliant** - Config via environment, not files
- [ ] **Validation at startup** - Fail fast on missing/invalid config
- [ ] **Environment parity** - Same config structure across dev/staging/prod
### 1.21 Cloud/Infrastructure (12-Factor)
- [ ] **Stateless processes** - No local session storage, use external stores
- [ ] **Port binding** - Self-contained, exports HTTP via port
- [ ] **Disposability** - Fast startup, graceful SIGTERM shutdown
- [ ] **Dev/prod parity** - Minimal gap between environments
- [ ] **Container best practices** - Multi-stage builds, non-root user, resource limits
- [ ] **IaC used** - Terraform/CloudFormation for reproducibility
### 1.22 Frontend Patterns
- [ ] **Component design** - Small, reusable, composition over inheritance
- [ ] **State management** - Appropriate tool (local, context, Zustand, Redux)
- [ ] **Bundle size** - Code splitting, lazy loading, < 500KB main bundle
- [ ] **Memoization** - Strategic use of memo/useMemo/useCallback
- [ ] **Web Vitals** - LCP < 2.5s, FID < 100ms, CLS < 0.1
### 1.23 Mobile Patterns
- [ ] **Battery efficient** - WorkManager/JobScheduler, batched operations
- [ ] **Offline-first** - Local caching with sync, offline queue
- [ ] **Responsive layout** - Flexible dimensions (dp/sp), rotation handling
- [ ] **Memory efficient** - Image downsampling, lifecycle awareness
- [ ] **Network efficient** - Request batching, compression, exponential backoff
### 1.24 AI/ML Code Patterns
- [ ] **Model versioning** - MLflow/DVC for models and data
- [ ] **Reproducibility** - Random seeds, pinned dependencies, exact environments
- [ ] **Bias detection** - Fairness metrics across demographics
- [ ] **Data pipeline validated** - Schema validation, statistical tests
- [ ] **Model monitoring** - Drift detection for data and performance
### 1.25 Clean Code: Names (Robert C. Martin)
- [ ] **Intention-revealing names** - Name tells you why it exists, what it does, how it's used
- [ ] **No misleading names** - `accountList` should actually be a list; avoid false clues
- [ ] **Meaningful distinctions** - Not `data1`, `data2`, `dataInfo`, `theData`
- [ ] **Pronounceable names** - Can discuss code verbally without spelling variables
- [ ] **Searchable names** - Single-letter names only for small local scope
- [ ] **No encodings** - No Hungarian notation, no type prefixes (strName, intCount)
- [ ] **No mental mapping** - Reader shouldn't translate names to concepts they know
- [ ] **Class names are nouns** - Customer, Account, Parser (not verbs)
- [ ] **Method names are verbs** - postPayment, deletePage, save (not nouns)
### 1.26 Clean Code: Functions (Robert C. Martin)
- [ ] **Small** - 5-20 lines ideal; rarely exceed 30 lines
- [ ] **Do one thing** - Single level of abstraction; one reason to change
- [ ] **One abstraction level** - Don't mix getHtml() with .append("\n")
- [ ] **Descriptive names** - Long descriptive name better than short enigmatic one
- [ ] **Few arguments** - Zero ideal, one/two good, three questionable, never more than four
- [ ] **No flag arguments** - Split function into two instead of passing boolean
- [ ] **No side effects** - Don't modify unexpected state; function does what name says only
- [ ] **Command/Query separation** - Either do something OR answer something, never both
- [ ] **Prefer exceptions to error codes** - Don't return -
…(truncated)
1---2name: code-reviewing3description: Performs systematic code review with universal best practices and repo-specific standards. Auto-activates after significant code changes. Use when reviewing code, auditing files, checking PRs, examining staged changes, or when asked to "review", "check", "audit", or "examine" code. Enforces design principles (SOLID, DRY, KISS), security (OWASP), performance, concurrency safety, cross-platform compatibility, and codebase patterns. Use when this capability is needed.4---56# Code Reviewing78Systematic code review skill based on industry best practices from Google Engineering, OWASP, and modern development standards. Designed to catch issues that casual review misses through structured, checklist-driven analysis.910**Core Principle (Google):** Approve code that improves overall code health, even if not perfect. Seek continuous improvement, not perfection. But NEVER approve code that degrades code health.1112## When to Use This Skill1314**Auto-activation triggers:**1516- After completing significant implementation tasks17- Before committing changes18- When reviewing PRs or staged files1920**Explicit activation triggers:**2122- User asks to "review", "check", "audit", or "examine" code23- User mentions "code review", "PR review", "look at this code"24- User asks about code quality or standards compliance2526## Interactive Review Scoping2728Use AskUserQuestion to determine review depth and risk profile when not specified:2930```yaml31# Question 1: Review Depth (MCP: Google Engineering code review, OWASP)32question: "What type of code review is needed?"33header: "Review Type"34options:35 - label: "Quick Review (Recommended)"36 description: "Surface issues, style, obvious bugs (~15 min, ~3.5K tokens)"37 - label: "Thorough Review"38 description: "Multi-pass: logic, design, tests (~45 min, ~14K tokens)"39 - label: "Security Review"40 description: "Threat-focused: auth, validation, crypto (~30 min)"41 - label: "Architecture Review"42 description: "Design patterns, dependencies, coupling (~60 min)"4344# Question 2: Risk Profile (MCP: CLI best practices - scope selection)45question: "What is the risk profile of this change?"46header: "Risk"47options:48 - label: "Low Risk (Recommended)"49 description: "Isolated fix, well-tested area, no user impact"50 - label: "Medium Risk"51 description: "Feature change, moderate scope, reversible"52 - label: "High Risk"53 description: "Critical path, security-sensitive, wide impact"54 - label: "Unknown"55 description: "I'm not sure - analyze and recommend"56```5758Use these responses to select the appropriate review profile (quick, thorough, security, strict, performance).5960## Review Workflow6162```text63Code Review Progress:64- [ ] Step 0: RESEARCH PHASE (MANDATORY - Run MCP queries for technology stack)65- [ ] Step 0b: LOAD REPO CONFIG (Check .claude/code-review.md, fallback to CLAUDE.md)66- [ ] Step 0e: Git History Analysis (coupling, hot spots, author context - thorough/strict only)67- [ ] Step 1: Count files to review (accurate file counting)68- [ ] Step 1b: Detect generated content (scan for markers, identify generators)69- [ ] Step 2: Identify scope (files to review, excluding generated files)70- [ ] Step 3: Load context (repo standards + MCP research results + repo config)71- [ ] Step 4: Run Universal Checks (Layer 1) - informed by MCP research72- [ ] Step 5: Run Repo-Specific Checks (Layer 2, applying repo config rules)73- [ ] Step 5b: Run Claude Code Validation (Tier 4b, if CC files detected)74- [ ] Step 6: Report ALL findings with severity, rule source, and MCP validation status75- [ ] Step 7: Propose specific fixes with rationale and MCP-backed recommendations76```7778### Step 0: RESEARCH PHASE (MANDATORY)7980**CRITICAL: This step runs BEFORE any code analysis. It is NOT optional.**8182Query MCP servers to understand current patterns and best practices BEFORE reviewing code. This prevents making claims based on stale training data.8384**Load Reference:** [references/tier-0/research-phase.md](references/tier-0/research-phase.md)8586**Quick Reference:**87881. **Detect Technology Stack** (from file extensions, imports, manifests)892. **Query MCP Servers** (parallel queries based on detected technologies):90 - Microsoft tech (.NET, Azure, C#) → `microsoft-learn` + `perplexity` (ALWAYS dual-validate)91 - npm/PyPI packages → `context7` + `ref`92 - Security patterns → `perplexity` (OWASP)93 - Version claims → `perplexity` (ALWAYS validate)943. **Build Current Truth Context** (store validated patterns for use during analysis)9596**Core Rules:**9798- **Research BEFORE analysis** - Know current best practices before making claims99- **Perplexity ALWAYS required** - Training data is stale; always cross-validate100- **Dual validation for Microsoft tech** - `microsoft-learn` can be stale, ALWAYS pair with `perplexity`101- **Every finding needs a source** - No finding without authoritative validation102103**High-Risk Technologies (Extra Perplexity Validation Required):**104105- .NET 10, .NET Aspire, Azure AI Foundry, HybridCache, Microsoft.Extensions.AI106107### Step 0b: Load Repository Configuration108109**Load repo-specific rules to customize the review.**110111**Load Reference:** [references/tier-4/repo-config.md](references/tier-4/repo-config.md)112113**Configuration Priority Chain:**114115```text1161. .claude/code-review.md (PRIMARY)117 ├── Found: Parse config, apply rules, STOP118 └── Not found: Continue to fallback1191201.5. .claude/rules/*.md (NATIVE - auto-loaded by Claude Code runtime)121 Always in context, supplements other config, path-scoped support1221232. CLAUDE.md + @imports (FALLBACK)124 ├── Found: Read CLAUDE.md + follow @imports125 │ └── Extract rules from ## Critical Rules, ## Conventions sections126 └── Not found: Continue to fallback1271283. No config found129 ├── Interactive mode: Use AskUserQuestion130 │ └── "No review configuration found. Review with default rules?"131 └── Non-interactive: Apply default rules only132```133134**What Gets Loaded:**135136| Source | What's Parsed |137| --- | --- |138| `.claude/code-review.md` | Tech Stack, Exclude Rules, Severity Overrides, Custom Checks |139| `.claude/rules/*.md` | Auto-injected by runtime; treat as additional review rules |140| CLAUDE.md + @imports | Text from ## Critical Rules, ## Conventions, ## Code Review sections |141142**Config Effects:**143144| Config Section | Effect on Review |145| --- | --- |146| Tech Stack | Override auto-detection, improve MCP query accuracy |147| Exclude Rules | Skip these rules entirely (no findings generated) |148| Severity Overrides | Change default severity for specific rules |149| Custom Checks | Add project-specific checks (file patterns, content rules) |150151**Quick Reference:**152153```bash154# Check if config exists155ls .claude/code-review.md 2>/dev/null || echo "No config, will use CLAUDE.md fallback"156157# Preview what config will be loaded (via review command)158/code-quality:review --show-rules159```160161**Agent Applicability:**162163ALL agents loading the code-reviewing skill MUST execute Step 0b. This includes164code-reviewer, quality-reviewer, and security-reviewer. Each agent independently165loads and applies repo config within its own domain.166167**Security Exclusion Advisory:**168169When repo config excludes security-related rules, agents MUST emit an ADVISORY note:170"ADVISORY: Security rule '{rule}' excluded by repo config. Ensure intentional."171This is informational only -- not a finding, not blocking.172173### Step 0e: Git History Analysis174175**Triggered by:** `thorough` or `strict` profile (partial for `security`/`performance`)176177**Load Reference:** [references/tier-1/git-history-context.md](references/tier-1/git-history-context.md)178179Extract git history context to inform review priorities and catch coupling issues:1801811. **Read Configuration** - Check `.claude/code-review.md` for history analysis settings1822. **Coupling Analysis** - Find files that frequently change together1833. **Hot Spot Detection** - Identify high-churn files (configurable threshold, default: 10+ changes in 3 months)1844. **Author Context** - Extract ownership patterns (`strict` profile only)1855. **Recent Patterns** - Detect bug fix, security, or refactoring history186187**Quick Reference Commands:**188189```bash190# Coupling analysis - files that change together191git log --name-only --pretty=format: -- <files> | sort | uniq -c | sort -rn | head -20192193# Hot spot detection - change frequency194git log --since="3 months ago" --name-only --pretty=format: | sort | uniq -c | sort -rn | head -20195196# Author context - ownership197git shortlog -sn -- <files>198199# Recent patterns - commit messages200git log --oneline -10 -- <files>201```202203**Profile Behavior:**204205| Profile | Analysis Depth |206| --- | --- |207| quick | Skip entirely |208| security | Partial (security-related commits only) |209| thorough | Coupling + hot spots + recent patterns |210| strict | Full analysis including author context |211| performance | Partial (perf-related history only) |212213### Step 1: Count Files (Accurate File Counting)214215Before reviewing, count the files to ensure accurate reporting:216217| Review Scope | Counting Command |218| --- | --- |219| Staged changes | `git diff --staged --name-only \| wc -l` |220| PR changes | `git diff --name-only main...HEAD \| wc -l` |221| Specific paths | Use Glob tool and count results |222223**Important**:224225- Track this count accurately - report it in **Files Reviewed** output226- Exclude binary files (images, compiled assets) from review count227- Exclude deleted files from review count (nothing to review)228- For renamed files: count as 1 file (the new name)229230**Large Changeset Warnings**:231232- 50+ files: Consider batched review or consensus mode233- 100+ files: Strongly recommend breaking into smaller chunks234235### Step 1b: Detect Generated Content236237Scan changed files for generation markers to identify files that were created by scripts/tools. Review the generator (source of truth) instead of generated output.238239**Detection Markers** (scan first 20 + last 10 lines):240241| File Type | Markers |242| --- | --- |243| Markdown | `Generated: YYYY-MM-DD`, `<!-- Generated by ScriptName -->` |244| JSON | Root `"timestamp"` or `"generator"` fields |245| Scripts | `Auto-generated from`, `DO NOT EDIT`, `Generated by` |246| Code | `// <auto-generated>`, `@generated`, `*.g.cs`, `*.generated.ts` |247248**Workflow**:2492501. Scan changed files for markers2512. If marker found: extract generator name, search for script, add to review scope2523. Mark generated files for skip/light review2534. Warn if only generated files changed (may indicate stale regeneration)254255**Edge Cases**:256257- Generator also changed → Review generator only, skip generated258- Only generated changed → Warn: stale regeneration or manual edit259- Generator not found → Review generated file anyway260- Security-sensitive generated file → Still review (credentials, config)261262Load [references/tier-5/generated-content-detection.md](references/tier-5/generated-content-detection.md) for complete detection patterns and algorithm.263264### Step 2: Identify Scope265266- **Explicit request**: User-specified files or changes267- **Staged changes**: `git diff --staged` or `git status`268- **Recent work**: Files modified in current session269- **PR scope**: All files in a pull request270271### Step 3: Load Context272273Check for repo-specific standards. If found, load for Layer 2 checks.274275### Review Profiles276277When `--profile <name>` is specified, restrict tier loading to profile-specific subsets. This enables focused reviews for specific concerns.278279**Available Profiles:**280281| Profile | Tiers Loaded | Description | Use Case |282| --- | --- | --- | --- |283| `security` | 0, 1.3, 3-security | OWASP, secrets, auth, crypto | Fast security scan before merge |284| `quick` | 1.1-1.6, 1.12 | Design, logic, style basics | Pre-commit fast check |285| `thorough` | All tiers + 1.30-1.32 | Complete analysis + reference/pattern checks | PR review (default) |286| `strict` | All thorough + deep patterns | Full analysis + architectural pattern enforcement | Library releases, major changes |287| `performance` | 0, 1.5, 2-backend, 3-concurrency | N+1, complexity, memory, concurrency | Performance audit |288289**Profile Tier Mappings:**290291```text292security:293 - Tier 0: Research Phase (ALWAYS - detect auth/crypto libraries)294 - Tier 1.3: Security (OWASP-Based)295 - Tier 3: security-patterns.md (when auth/crypto patterns detected)296 - Skip: Design, readability, testing, documentation297298quick:299 - Tier 1.1: Design and Architecture (basic structure)300 - Tier 1.2: Functionality and Logic (obvious errors)301 - Tier 1.3: Security (CRITICAL only - hardcoded secrets)302 - Tier 1.4: Concurrency (obvious race conditions)303 - Tier 1.5: Performance (N+1, obvious inefficiencies)304 - Tier 1.6: Complexity (extreme violations)305 - Tier 1.12: Style (consistency)306 - Skip: Tier 0 MCP research, Tier 2+, Clean Code deep-dives307308thorough:309 - ALL tiers loaded based on file types/patterns310 - Complete analysis (default behavior)311 - Section 1.30: Reference Integrity (always when renames/deletions detected)312 - Section 1.31: Breaking Changes (always for public API changes)313 - Section 1.32: Pattern Compliance (basic pattern checks)314 - Sections 1.33-1.36: Git History Context (coupling, hot spots, recent patterns)315316strict:317 - ALL 'thorough' checks318 - Section 1.32 Pattern Compliance (full analysis):319 - Architectural pattern deviation (CQRS, mediator, repository)320 - DI registration consistency321 - File organization enforcement322 - Section 1.35: Author Context (ownership patterns, bus factor)323 - Import/export hygiene (module boundaries, barrel pollution)324 - Test correlation (orphaned tests detection)325 - Dependency safety (migration verification)326327performance:328 - Tier 0: Research Phase (performance best practices via MCP)329 - Tier 1.5: Performance and Efficiency330 - Tier 2: backend-checks.md (database, API performance)331 - Tier 3: concurrency-patterns.md (async, threading)332 - Skip: Style, documentation, clean code deep-dives333```334335**Token Savings by Profile:**336337| Profile | Est. Tokens | Savings vs Strict |338| --- | --- | --- |339| security | ~5,500 | 60%+ |340| quick | ~3,500 | 75%+ |341| thorough | ~14,000 | 15% |342| strict | ~17,000 | 0% (baseline) |343| performance | ~6,000 | 65%+ |344345### Baseline Mode (Differential Analysis)346347When `--baseline <branch>` is specified, attribute each finding as NEW or PRE-EXISTING.348349**Why This Matters:**350This is the #1 adoption blocker for code review tools. Teams with tech debt get overwhelmed by 100+ findings when their PR only introduces 3 new issues. They give up on the tool.351352**Baseline Mode Workflow:**3533541. **Build attribution map** from git diff:355356 ```bash357 git diff <baseline>...HEAD # Three-dot for symmetric comparison358 ```3593602. **Parse diff** to identify changed line ranges per file3613. **For each finding**: Check if `file:line` is in changed ranges3624. **Tag findings**: NEW (line added/modified) or PRE-EXISTING (unchanged)3635. **Separate output**: New issues prominent, pre-existing collapsed364365**Finding Attribution Rules:**366367| Line Status | Attribution | Output Section |368| --- | --- | --- |369| Added (`+` in diff) | NEW | "New Issues Introduced" |370| Modified (in hunk, changed) | NEW | "New Issues Introduced" |371| Context (in hunk, unchanged) | PRE-EXISTING | "Pre-existing Issues" (collapsed) |372| Not in any hunk | PRE-EXISTING | "Pre-existing Issues" (collapsed) |373| Deleted (`-` in diff) | SKIP | Not reviewed (doesn't exist) |374375**Baseline Output Format:**376377```markdown378## Review Summary (Baseline Mode)379380**Baseline**: `main`381**New Issues (this changeset)**: 2 CRITICAL, 1 MAJOR ← FOCUS HERE382**Pre-existing Issues**: 47 (collapsed)383**Assessment**: Based on NEW issues only384385## New Issues Introduced386[Detailed findings with Attribution: NEW]387388<details>389<summary>Pre-existing Issues (47)</summary>390[Condensed findings]391</details>392```393394**Quality Gates (CI/CD):**395396- `--fail-on-new-critical`: Exit 1 if ANY new CRITICAL397- `--fail-on-new-major`: Exit 1 if ANY new MAJOR398- Pre-existing issues do NOT fail the build399400**Edge Cases:**401402- Baseline doesn't exist → Error with valid branch suggestions403- File renamed → Attribute based on content changes404- Merge commits → Use three-dot diff for correct comparison405406### Complexity Thresholds407408Use these concrete thresholds when evaluating code complexity:409410| Metric | Warning | Error | Measurement |411| --- | --- | --- | --- |412| Cyclomatic Complexity | > 10 | > 20 | Count decision points (if, while, for, case, &&, \|\|, ?) |413| Cognitive Complexity | > 15 | > 30 | SonarQube metric (nesting adds more weight) |414| Function Lines | > 30 | > 50 | Count non-blank, non-comment lines |415| Nesting Depth | > 3 | > 5 | Count nested braces/indentation levels |416| Parameter Count | > 4 | > 7 | Count function/method parameters |417| File Lines | > 300 | > 500 | Count total lines in file |418419**When thresholds exceeded:**420421- Warning → MINOR severity finding422- Error → MAJOR severity finding423424**Suggested Fix Template:**425426```markdown427### Complexity Threshold Exceeded428429**File**: `src/services/order.ts:processOrder()`430**Severity**: MAJOR431**Metric**: Cyclomatic Complexity = 23 (threshold: 15, error: 20)432433**Problem**: Function has 23 independent paths through the code.434435**Impact**: Difficult to test (need 23+ test cases), hard to maintain, high bug risk.436437**Fix**: Extract methods for logical branches. Consider:438- Extract validation logic → `validateOrder()`439- Extract pricing logic → `calculatePrice()`440- Extract notification logic → `notifyCustomer()`441```442443### Progressive Loading (Token Optimization)444445This skill uses **tiered progressive disclosure** to optimize token usage. Only load what's relevant to the files being reviewed.446447**Tier 0 (MANDATORY - Always First):** Research Phase (~2,000 tokens)448449- **ALWAYS load BEFORE analysis**: [references/tier-0/research-phase.md](references/tier-0/research-phase.md)450- Contains: Technology detection matrix, MCP query templates, "Build Truth Context" workflow451- Purpose: Query MCP servers to understand current patterns BEFORE making any claims452- **This tier is NOT optional** - skip only if code is purely internal with no external dependencies453454**Tier 1 (Always Applied):** Universal checks in this file (~4,500 tokens)455456- Sections 1.1-1.12: Design, Logic, Security, Concurrency, Performance, Readability, Testing, Error Handling, Documentation, Cross-Platform, Anti-Duplication, Style457- Sections 1.25-1.29: Clean Code (Names, Functions, Comments, Conditionals, Code Smells)458- Section 1.30: Reference Integrity - [references/tier-1/reference-integrity.md](references/tier-1/reference-integrity.md) (when renames/deletions detected)459- Section 1.31: Breaking Changes - [references/tier-1/breaking-changes.md](references/tier-1/breaking-changes.md) (for public API changes)460- Sections 1.33-1.36: Git History Context - [references/tier-1/git-history-context.md](references/tier-1/git-history-context.md) (thorough/strict profiles, ~1,500 tokens)461462**Tier 2 (File-Type Triggered):** Load based on file extensions463464| Files | Load Reference |465| --- | --- |466| .tsx, .jsx, .vue, .svelte | [references/tier-2/frontend-checks.md](references/tier-2/frontend-checks.md) |467| .py, .java, .cs, .go, .rb | [references/tier-2/backend-checks.md](references/tier-2/backend-checks.md) |468| .swift, .kt, .dart | [references/tier-2/mobile-checks.md](references/tier-2/mobile-checks.md) |469| .sql, migrations/* | [references/tier-2/database-checks.md](references/tier-2/database-checks.md) |470| .yaml, .json, .env, .toml | [references/tier-2/config-checks.md](references/tier-2/config-checks.md) |471| Dockerfile, *.tf, k8s/* | [references/tier-2/infrastructure-checks.md](references/tier-2/infrastructure-checks.md) |472| .ipynb, model/*, ml/* | [references/tier-2/ai-ml-checks.md](references/tier-2/ai-ml-checks.md) |473474**Tier 3 (Content-Pattern Triggered):** Load when code contains specific patterns475476| Pattern Keywords | Load Reference |477| --- | --- |478| auth, crypto, password, secret, token, jwt | [references/tier-3/security-patterns.md](references/tier-3/security-patterns.md) |479| async, await, thread, lock, mutex, concurrent | [references/tier-3/concurrency-patterns.md](references/tier-3/concurrency-patterns.md) |480| route, endpoint, @app, @Get, @Post, api/ | [references/tier-3/api-patterns.md](references/tier-3/api-patterns.md) |481| PII, email, user, customer, gdpr, consent | [references/tier-3/privacy-patterns.md](references/tier-3/privacy-patterns.md) |482483**Clean Code Deep-Dives:** Load for detailed guidance484485- [references/clean-code/naming-functions.md](references/clean-code/naming-functions.md) - Full naming and function principles486- [references/clean-code/code-smells.md](references/clean-code/code-smells.md) - Complete code smell catalog487- [references/clean-code/refactoring-patterns.md](references/clean-code/refactoring-patterns.md) - Refactoring techniques488489**Tier 4 (Repository-Specific):** Load when repo config or CLAUDE.md exists490491| Context | Load Reference |492| --- | --- |493| .claude/code-review.md exists | [references/tier-4/repo-config.md](references/tier-4/repo-config.md) |494| CLAUDE.md exists (always) | [references/tier-4/claude-md-core.md](references/tier-4/claude-md-core.md) |495| *.md files | [references/tier-4/documentation-rules.md](references/tier-4/documentation-rules.md) |496| Duplication indicators | [references/tier-4/anti-duplication-rules.md](references/tier-4/anti-duplication-rules.md) |497| Path patterns detected | [references/tier-4/path-rules.md](references/tier-4/path-rules.md) |498| *-{platform}.md files | [references/tier-4/platform-rules.md](references/tier-4/platform-rules.md) |499| .claude/skills/** | [references/tier-4/skill-rules.md](references/tier-4/skill-rules.md) |500| .claude/memory/** | [references/tier-4/memory-rules.md](references/tier-4/memory-rules.md) |501| .claude/temp/** | [references/tier-4/temp-file-rules.md](references/tier-4/temp-file-rules.md) |502| Profile thorough/strict | [references/tier-4/pattern-compliance.md](references/tier-4/pattern-compliance.md) |503504**Tier 4b (Claude Code Components):** Delegate to specialized auditors when CC files detected505506| File Pattern | Component Type | Auditor Agent |507| --- | --- | --- |508| `.claude/agents/**/*.md`, `agents/*.md` | Agent | `claude-ecosystem:agent-auditor` |509| `.claude/skills/**`, `skills/*/SKILL.md` | Skill | `claude-ecosystem:skill-auditor` |510| `.claude/hooks/**`, `hooks.json` | Hook | `claude-ecosystem:hook-auditor` |511| `.claude/skills/**` | Skill | `claude-ecosystem:skill-auditor` |512| `CLAUDE.md`, `.claude/memory/**` | Memory | `claude-ecosystem:memory-component-auditor` |513| `output-styles/*.md` | Output Style | `claude-ecosystem:output-style-auditor` |514| `.mcp.json`, `mcp.json` | MCP Config | `claude-ecosystem:mcp-auditor` |515| `settings.json`, `.claude/settings.json` | Settings | `claude-ecosystem:settings-auditor` |516| `plugin.json`, `.claude-plugin/**` | Plugin | `claude-ecosystem:plugin-component-auditor` |517| Status line scripts | Status Line | `claude-ecosystem:statusline-auditor` |518519Load the reference for complete detection and delegation patterns: [references/tier-4/claude-code-components.md](references/tier-4/claude-code-components.md)520521**Note**: Tier 4b requires the `claude-ecosystem` plugin to be installed. If not installed, log a warning and skip CC-specific validation (continue with standard review).522523> **Auditor Agent Verification:** Agent names may change between plugin releases. Run `/claude-ecosystem:list skills`524> to verify current auditor agent names if delegation fails. The table above reflects known agents as of this file's last update.525526**Tier 5 (Generated Content & MCP Validation Details):** Load based on changeset characteristics527528| Trigger | Load Reference |529| --- | --- |530| Large changeset (50+ files) | [references/tier-5/generated-content-detection.md](references/tier-5/generated-content-detection.md) |531| Generation markers found | [references/tier-5/generated-content-detection.md](references/tier-5/generated-content-detection.md) |532| Need advanced MCP query patterns | [references/tier-5/mcp-validation.md](references/tier-5/mcp-validation.md) |533| Complex technology stack | [references/tier-5/mcp-validation.md](references/tier-5/mcp-validation.md) |534| Security patterns require OWASP validation | [references/tier-5/mcp-validation.md](references/tier-5/mcp-validation.md) |535536**Note:** Primary MCP validation happens in **Tier 0 (Research Phase)** which is MANDATORY. Tier 5 `mcp-validation.md` provides additional query templates and advanced patterns for complex scenarios.537538**Token Budget Estimates:**539540| Scenario | Tokens |541| --- | --- |542| Tier 0 Research Phase (MANDATORY) | ~2,000 (always included) |543| Simple Python file | ~8,500 (Tier 0 + Hub + backend) |544| React component | ~8,000 (Tier 0 + Hub + frontend) |545| Auth service | ~10,000 (Tier 0 + Hub + backend + security) |546| Full-stack PR | ~11,500 (Tier 0 + Hub + frontend + backend + API) |547| Documentation file (CLAUDE.md repo) | ~8,500 (Tier 0 + Hub + core-rules + documentation-rules) |548| Skill modification | ~8,000 (Tier 0 + Hub + core-rules + skill-rules) |549| Memory file update | ~7,500 (Tier 0 + Hub + core-rules + memory-rules) |550| With advanced MCP patterns | +~1,200 tokens (Tier 5 mcp-validation.md) |551| With CC auditor delegation | +~1,500 tokens (Tier 4b reference + auditor spawn overhead) |552| With generated content detection | +~1,200 tokens (Tier 5 generated-content-detection.md) |553| With git history context (thorough/strict) | +~1,500 tokens (Tier 1 git-history-context.md) |554555**Note:** Token budgets increased by ~2,000 tokens due to MANDATORY Tier 0 Research Phase. This trade-off provides significantly more accurate reviews through MCP validation.556557## Layer 1: Universal Code Review Checklist558559These checks apply to ANY codebase, ANY language.560561### 1.1 Design and Architecture562563- [ ] **Overall design makes sense** - Interactions between components are logical564- [ ] **Belongs in this codebase** - Not better suited for a library or different module565- [ ] **Integrates well** - Fits with existing system architecture566- [ ] **Right time for this change** - Not premature or addressing wrong problem567- [ ] **No over-engineering** - Solves current problem, not speculative future needs (YAGNI)568569**SOLID Principles:**570571- [ ] **Single Responsibility (SRP)** - Each class/function has ONE reason to change572- [ ] **Open-Closed (OCP)** - Open for extension, closed for modification (no repeated if/else for types)573- [ ] **Liskov Substitution (LSP)** - Derived classes substitutable for base (no explicit type casting)574- [ ] **Interface Segregation (ISP)** - Clients depend only on methods they use575- [ ] **Dependency Inversion (DIP)** - Depend on abstractions, not concrete implementations576577### 1.2 Functionality and Logic578579- [ ] **Does what it's supposed to do** - Implements intended functionality580- [ ] **Good for users** - Both end-users AND developers who'll use this code581- [ ] **Edge cases handled** - Boundary conditions, empty inputs, nulls582- [ ] **Error handling robust** - Failures handled gracefully with actionable messages583- [ ] **No logic errors** - Off-by-one, wrong operators, incorrect conditions584585### 1.3 Security (OWASP-Based)586587- [ ] **Input validation** - All inputs validated server-side (allowlist, not blocklist)588- [ ] **Output encoding** - Context-appropriate encoding (HTML, JS, SQL, URL)589- [ ] **No injection vulnerabilities** - SQL, command, XSS, path traversal590- [ ] **Authentication correct** - Login via POST, secure session handling, MFA where appropriate591- [ ] **Authorization enforced** - Role-based access, principle of least privilege592- [ ] **Secrets not hardcoded** - No API keys, passwords, tokens in code593- [ ] **Secrets not logged** - No sensitive data in logs, error messages, URLs594- [ ] **Cryptography modern** - bcrypt/Argon2 for passwords, AES-GCM for encryption, no MD5/SHA1595- [ ] **Dependencies secure** - No known vulnerabilities in third-party libraries596597### 1.4 Concurrency and Thread Safety598599- [ ] **Shared state protected** - Proper locks, mutexes, or atomics for shared data600- [ ] **No race conditions** - Concurrent access patterns analyzed601- [ ] **Consistent lock ordering** - Locks acquired in same order to prevent deadlocks602- [ ] **No circular dependencies** - Between resources protected by different locks603- [ ] **Async patterns correct** - Await used properly, exceptions propagated604- [ ] **Thread-safe collections** - Concurrent collections used where needed605- [ ] **No deadlock potential** - Timeout mechanisms, no indefinite waits while holding locks606607### 1.5 Performance and Efficiency608609- [ ] **No unnecessary operations** - Efficient algorithms, no redundant work610- [ ] **Appropriate data structures** - Right choice for access patterns611- [ ] **No N+1 queries** - Database queries optimized612- [ ] **Memory efficient** - No leaks, appropriate caching613- [ ] **I/O optimized** - Async for I/O-bound, batching where appropriate614- [ ] **No blocking in async** - Sync operations not blocking async contexts615616### 1.6 Complexity and Readability617618- [ ] **Not more complex than needed** - Can be understood quickly619- [ ] **Functions/classes reasonable size** - Single responsibility, not too long620- [ ] **No deep nesting** - Max 3-4 levels of indentation621- [ ] **Clear naming** - Names fully communicate purpose without being too long622- [ ] **Comments explain WHY** - Not what (code should be self-documenting)623- [ ] **No code duplication** - DRY principle followed624625### 1.7 Testing626627- [ ] **Tests included** - Unit/integration tests appropriate for change628- [ ] **Tests are correct** - Actually test what they claim to test629- [ ] **Tests are useful** - Will fail when code breaks630- [ ] **Edge cases tested** - Boundary conditions, error scenarios631- [ ] **Tests maintainable** - Not overly complex, clear assertions632- [ ] **No flaky tests** - Deterministic, not timing-dependent633634**Test Smell Detection:**635636Review test code for common test smells that reduce test quality and maintainability:637638| Smell | Detection | Severity | Why It Matters |639| --- | --- | --- | --- |640| **Empty Test** | Test method with no assertions | CRITICAL | False confidence - test always passes |641| **Eager Test** | > 5 assertions in one test | WARNING | When it fails, unclear which behavior broke |642| **Mystery Guest** | External file/network access without mock | MAJOR | Flaky, slow, environment-dependent |643| **Assertion Roulette** | Multiple asserts without descriptive messages | MINOR | Hard to identify which assertion failed |644| **Sleep in Test** | `Thread.Sleep`, `await Task.Delay`, `setTimeout` | MAJOR | Slow, flaky, hides timing bugs |645| **Dead Test** | Test that never fails (always passes) | WARNING | Usually tests nothing meaningful |646| **Commented Test** | Disabled test cases (`[Ignore]`, `skip`, `xtest`) | WARNING | Technical debt, may hide real issues |647| **Test Code Duplication** | Same setup/teardown in multiple tests | MINOR | Maintenance burden, use fixtures |648649**Example Findings:**650651```markdown652### Test Smell: Eager Test653654**File**: `tests/UserService.test.ts:45`655**Severity**: WARNING656**Confidence**: MEDIUM657658**Problem**: Test "should handle user operations" has 12 assertions testing multiple behaviors.659660**Impact**: When this test fails, you won't know which behavior broke without debugging.661662**Suggested Fix**:663```typescript664// Before (Eager Test - 12 assertions, multiple behaviors)665test('should handle user operations', () => {666 const user = createUser();667 expect(user.name).toBe('John');668 expect(user.email).toContain('@');669 // ... 10 more assertions about different behaviors670});671672// After (Focused Tests - one behavior each)673test('should create user with valid name', () => {674 const user = createUser();675 expect(user.name).toBe('John');676});677678test('should validate email format', () => {679 const user = createUser();680 expect(user.email).toContain('@');681});682```683684```text685686```markdown687### Test Smell: Sleep in Test688689**File**: `tests/api.test.ts:78`690**Severity**: MAJOR691**Confidence**: HIGH692693**Problem**: Test uses `await new Promise(r => setTimeout(r, 2000))` to wait for async operation.694695**Impact**: Test is slow (2 seconds), flaky (timing varies), and hides real async handling bugs.696697**Suggested Fix**:698```typescript699// Before (Sleep - slow and flaky)700await new Promise(r => setTimeout(r, 2000));701expect(result).toBeDefined();702703// After (Proper async handling)704await waitFor(() => expect(result).toBeDefined());705// Or706await expect(asyncOperation()).resolves.toBeDefined();707```708709```text710711### 1.8 Error Handling and Logging712713- [ ] **Errors caught appropriately** - Right level of granularity714- [ ] **Error messages actionable** - Clear what went wrong and how to fix715- [ ] **Logging present** - For debugging and troubleshooting716- [ ] **No sensitive data in logs** - PII, passwords, keys excluded717- [ ] **Graceful degradation** - Partial failures don't crash entire system718719### 1.9 Documentation720721- [ ] **Code documented** - Public APIs, complex logic explained722- [ ] **README updated** - If behavior/setup changes723- [ ] **API docs updated** - If endpoints change724- [ ] **Inline comments where needed** - For non-obvious decisions725726### 1.10 Cross-Platform Compatibility727728- [ ] **No hardcoded platform paths**:729 - `/mnt/c/Users/...` (WSL)730 - `/c/Users/...` (Git Bash)731 - `C:\Users\...` (Windows)732 - `/home/username/...` (Linux)733 - `/Users/username/...` (macOS)734- [ ] **Portable tool detection** - `command -v tool` not path hunting735- [ ] **Platform fallbacks** - Graceful handling when features unavailable736- [ ] **Scripts self-locate** - Use `Path(__file__).resolve()`, `$PSScriptRoot`, `${BASH_SOURCE[0]}`737738### 1.11 Anti-Duplication739740- [ ] **No duplicate content** - Same info in ONE place only741- [ ] **No identical files** - `diff` similar files to verify742- [ ] **Single source of truth** - Link instead of copy-paste743- [ ] **Config files distinct** - Each serves different purpose744745### 1.12 Style and Consistency746747- [ ] **Follows style guide** - Language/project conventions748- [ ] **Consistent with codebase** - Matches existing patterns749- [ ] **No style changes mixed with logic** - Separate formatting PRs750751### 1.13 Accessibility (WCAG 2.1 AA)752753- [ ] **Alt text present** - All images have descriptive alt text; decorative images use `alt=""`754- [ ] **Color contrast sufficient** - 4.5:1 for text, 3:1 for UI components755- [ ] **Keyboard navigable** - All interactive elements via Tab/Enter/Space; no keyboard traps756- [ ] **Focus visible** - Clear focus indicators on all interactive elements757- [ ] **Semantic HTML** - Proper heading hierarchy; buttons not divs; links not spans758- [ ] **ARIA correct** - Used only when semantic HTML insufficient; no conflicting roles759760### 1.14 Internationalization (i18n)761762- [ ] **No hardcoded strings** - All user-facing text externalized to resource files763- [ ] **Locale-aware formatting** - Dates, numbers, currency use locale APIs764- [ ] **RTL consideration** - Logical CSS properties where applicable765- [ ] **No string concatenation** - Use parameterized messages, not `"Hello " + name`766- [ ] **Pluralization handled** - Proper plural rules, not `count + " items"`767768### 1.15 Observability769770- [ ] **Structured logging** - JSON format with trace IDs, timestamps, context771- [ ] **Metrics present** - Latency, error rates, throughput for critical paths772- [ ] **Trace context propagated** - Distributed tracing across service boundaries773- [ ] **Health checks implemented** - Liveness/readiness probes with dependency checks774- [ ] **SLOs defined** - Measurable service level objectives for key operations775776### 1.16 Data Privacy (GDPR/CCPA)777778- [ ] **PII identified and protected** - Personal data encrypted, access controlled779- [ ] **Data retention enforced** - Clear policies, automated cleanup780- [ ] **Right to deletion** - Complete erasure across all systems possible781- [ ] **Consent tracked** - Explicit opt-in with audit trail782- [ ] **No PII in logs** - Redaction or hashing of personal identifiers783784### 1.17 API Design785786- [ ] **Versioning strategy** - Clear version in URL, header, or media type787- [ ] **Backward compatible** - New fields nullable, no removed fields788- [ ] **Deprecation documented** - Sunset dates, migration paths789- [ ] **Consistent naming** - Follows REST/GraphQL conventions790- [ ] **Error responses standardized** - Consistent error format across endpoints791792### 1.18 Dependency Management793794- [ ] **No known vulnerabilities** - CVE scanning in CI/CD795- [ ] **License compliance** - No GPL conflicts with proprietary code796- [ ] **Version pinned** - Lockfiles present and up-to-date797- [ ] **Transitive deps reviewed** - Indirect dependencies also secure798- [ ] **SBOM available** - Software Bill of Materials for audits799800### 1.19 Database Patterns801802- [ ] **N+1 queries avoided** - Eager loading or batch queries used803- [ ] **Indexes present** - For foreign keys, join columns, query patterns804- [ ] **Migrations backward compatible** - Incremental changes, no data loss805- [ ] **Schema properly normalized** - Or denormalized with clear rationale806- [ ] **Query optimization** - Explain plans reviewed for complex queries807808### 1.20 Configuration Management809810- [ ] **Secrets in vault** - Never hardcoded, use env vars or secrets manager811- [ ] **Feature flags used** - For gradual rollouts, A/B testing812- [ ] **12-factor compliant** - Config via environment, not files813- [ ] **Validation at startup** - Fail fast on missing/invalid config814- [ ] **Environment parity** - Same config structure across dev/staging/prod815816### 1.21 Cloud/Infrastructure (12-Factor)817818- [ ] **Stateless processes** - No local session storage, use external stores819- [ ] **Port binding** - Self-contained, exports HTTP via port820- [ ] **Disposability** - Fast startup, graceful SIGTERM shutdown821- [ ] **Dev/prod parity** - Minimal gap between environments822- [ ] **Container best practices** - Multi-stage builds, non-root user, resource limits823- [ ] **IaC used** - Terraform/CloudFormation for reproducibility824825### 1.22 Frontend Patterns826827- [ ] **Component design** - Small, reusable, composition over inheritance828- [ ] **State management** - Appropriate tool (local, context, Zustand, Redux)829- [ ] **Bundle size** - Code splitting, lazy loading, < 500KB main bundle830- [ ] **Memoization** - Strategic use of memo/useMemo/useCallback831- [ ] **Web Vitals** - LCP < 2.5s, FID < 100ms, CLS < 0.1832833### 1.23 Mobile Patterns834835- [ ] **Battery efficient** - WorkManager/JobScheduler, batched operations836- [ ] **Offline-first** - Local caching with sync, offline queue837- [ ] **Responsive layout** - Flexible dimensions (dp/sp), rotation handling838- [ ] **Memory efficient** - Image downsampling, lifecycle awareness839- [ ] **Network efficient** - Request batching, compression, exponential backoff840841### 1.24 AI/ML Code Patterns842843- [ ] **Model versioning** - MLflow/DVC for models and data844- [ ] **Reproducibility** - Random seeds, pinned dependencies, exact environments845- [ ] **Bias detection** - Fairness metrics across demographics846- [ ] **Data pipeline validated** - Schema validation, statistical tests847- [ ] **Model monitoring** - Drift detection for data and performance848849### 1.25 Clean Code: Names (Robert C. Martin)850851- [ ] **Intention-revealing names** - Name tells you why it exists, what it does, how it's used852- [ ] **No misleading names** - `accountList` should actually be a list; avoid false clues853- [ ] **Meaningful distinctions** - Not `data1`, `data2`, `dataInfo`, `theData`854- [ ] **Pronounceable names** - Can discuss code verbally without spelling variables855- [ ] **Searchable names** - Single-letter names only for small local scope856- [ ] **No encodings** - No Hungarian notation, no type prefixes (strName, intCount)857- [ ] **No mental mapping** - Reader shouldn't translate names to concepts they know858- [ ] **Class names are nouns** - Customer, Account, Parser (not verbs)859- [ ] **Method names are verbs** - postPayment, deletePage, save (not nouns)860861### 1.26 Clean Code: Functions (Robert C. Martin)862863- [ ] **Small** - 5-20 lines ideal; rarely exceed 30 lines864- [ ] **Do one thing** - Single level of abstraction; one reason to change865- [ ] **One abstraction level** - Don't mix getHtml() with .append("\n")866- [ ] **Descriptive names** - Long descriptive name better than short enigmatic one867- [ ] **Few arguments** - Zero ideal, one/two good, three questionable, never more than four868- [ ] **No flag arguments** - Split function into two instead of passing boolean869- [ ] **No side effects** - Don't modify unexpected state; function does what name says only870- [ ] **Command/Query separation** - Either do something OR answer something, never both871- [ ] **Prefer exceptions to error codes** - Don't return -872873…(truncated)
Run npx skillmds@latest add tomevault-io/code-reviewing in your terminal (requires Node.js), paste this page's agent-chat prompt into Claude, Cursor, or any MCP-connected agent, or download the SKILL.md file and copy it into your agent's skills directory.
Performs systematic code review with universal best practices and repo-specific standards. Auto-activates after significant code changes. Use when reviewing code, auditing files, checking PRs, examining staged changes, or when asked to "review", "check", "audit", or "examine" code. Enforces design principles (SOLID, DRY, KISS), security (OWASP), performance, concurrency safety, cross-platform compatibility, and codebase patterns. Use when this capability is needed. It is listed under Security on SkillMD.
This skill has not completed SkillMD's automated safety review yet. SkillMD never runs a skill's scripts for you; review the SKILL.md before installing.
This skill is tagged as working with Claude Code, Claude.ai, OpenAI Codex. SKILL.md is an open format, so most agents that read a skills directory can load it too.
Yes. Installing skills from SkillMD is free, and the skill stays under its author's original license.
tomevault-io (@tomevault-io) published this skill. Their other Agent Skills are listed on their SkillMD profile.