code-reviewing

What this skill does

# Code Reviewing

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:

```yaml
# Question 1: Review Depth (MCP: Google Engineering code review, OWASP)
question: "What type of code review is needed?"
header: "Review Type"
options:
  - label: "Quick Review (Recommended)"
    description: "Surface issues, style, obvious bugs (~15 min, ~3.5K tokens)"
  - label: "Thorough Review"
    description: "Multi-pass: logic, design, tests (~45 min, ~14K tokens)"
  - label: "Security Review"
    description: "Threat-focused: auth, validation, crypto (~30 min)"
  - label: "Architecture Review"
    description: "Design patterns, dependencies, coupling (~60 min)"

# Question 2: Risk Profile (MCP: CLI best practices - scope selection)
question: "What is the risk profile of this change?"
header: "Risk"
options:
  - label: "Low Risk (Recommended)"
    description: "Isolated fix, well-tested area, no user impact"
  - label: "Medium Risk"
    description: "Feature change, moderate scope, reversible"
  - label: "High Risk"
    description: "Critical path, security-sensitive, wide impact"
  - label: "Unknown"
    description: "I'm not sure - analyze and recommend"
```

Use these responses to select the appropriate review profile (quick, thorough, security, strict, performance).

## Review Workflow

```text
Code Review Progress:
- [ ] Step 0: RESEARCH PHASE (MANDATORY - Run MCP queries for technology stack)
- [ ] Step 0b: LOAD REPO CONFIG (Check .claude/code-review.md, fallback to CLAUDE.md)
- [ ] Step 0e: Git History Analysis (coupling, hot spots, author context - thorough/strict only)
- [ ] Step 1: Count files to review (accurate file counting)
- [ ] Step 1b: Detect generated content (scan for markers, identify generators)
- [ ] Step 2: Identify scope (files to review, excluding generated files)
- [ ] Step 3: Load context (repo standards + MCP research results + repo config)
- [ ] Step 4: Run Universal Checks (Layer 1) - informed by MCP research
- [ ] Step 5: Run Repo-Specific Checks (Layer 2, applying repo config rules)
- [ ] Step 5b: Run Claude Code Validation (Tier 4b, if CC files detected)
- [ ] Step 6: Report ALL findings with severity, rule source, and MCP validation status
- [ ] Step 7: Propose specific fixes with rationale and MCP-backed recommendations
```

### Step 0: RESEARCH PHASE (MANDATORY)

**CRITICAL: This step runs BEFORE any code analysis. It is NOT optional.**

Query MCP servers to understand current patterns and best practices BEFORE reviewing code. This prevents making claims based on stale training data.

**Load Reference:** [references/tier-0/research-phase.md](references/tier-0/research-phase.md)

**Quick Reference:**

1. **Detect Technology Stack** (from file extensions, imports, manifests)
2. **Query MCP Servers** (parallel queries based on detected technologies):
   - Microsoft tech (.NET, Azure, C#) → `microsoft-learn` + `perplexity` (ALWAYS dual-validate)
   - npm/PyPI packages → `context7` + `ref`
   - Security patterns → `perplexity` (OWASP)
   - Version claims → `perplexity` (ALWAYS validate)
3. **Build Current Truth Context** (store validated patterns for use during analysis)

**Core Rules:**

- **Research BEFORE analysis** - Know current best practices before making claims
- **Perplexity ALWAYS required** - Training data is stale; always cross-validate
- **Dual validation for Microsoft tech** - `microsoft-learn` can be stale, ALWAYS pair with `perplexity`
- **Every finding needs a source** - No finding without authoritative validation

**High-Risk Technologies (Extra Perplexity Validation Required):**

- .NET 10, .NET Aspire, Azure AI Foundry, HybridCache, Microsoft.Extensions.AI

### Step 0b: Load Repository Configuration

**Load repo-specific rules to customize the review.**

**Load Reference:** [references/tier-4/repo-config.md](references/tier-4/repo-config.md)

**Configuration Priority Chain:**

```text
1. .claude/code-review.md (PRIMARY)
   ├── Found: Parse config, apply rules, STOP
   └── Not found: Continue to fallback

1.5. .claude/rules/*.md (NATIVE - auto-loaded by Claude Code runtime)
     Always in context, supplements other config, path-scoped support

2. CLAUDE.md + @imports (FALLBACK)
   ├── Found: Read CLAUDE.md + follow @imports
   │   └── Extract rules from ## Critical Rules, ## Conventions sections
   └── Not found: Continue to fallback

3. No config found
   ├── Interactive mode: Use AskUserQuestion
   │   └── "No review configuration found. Review with default rules?"
   └── Non-interactive: Apply default rules only
```

**What Gets Loaded:**

| Source | What's Parsed |
| --- | --- |
| `.claude/code-review.md` | Tech Stack, Exclude Rules, Severity Overrides, Custom Checks |
| `.claude/rules/*.md` | Auto-injected by runtime; treat as additional review rules |
| CLAUDE.md + @imports | Text from ## Critical Rules, ## Conventions, ## Code Review sections |

**Config Effects:**

| Config Section | Effect on Review |
| --- | --- |
| Tech Stack | Override auto-detection, improve MCP query accuracy |
| Exclude Rules | Skip these rules entirely (no findings generated) |
| Severity Overrides | Change default severity for specific rules |
| Custom Checks | Add project-specific checks (file patterns, content rules) |

**Quick Reference:**

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

**Load Reference:** [references/tier-1/git-history-context.md](references/tier-1/git-history-context.md)

Extract git history context to inform review priorities and catch coupling issues:

1. **Read Configuration** - Check `.claude/code-review.md` for history analysis settings
2. **Coupling Analysis** - Find files that frequently change together
3. **Hot Spot Detection** - Identify high-churn files (configurable threshold, default: 10+ changes in 3 months)
4. **Author Context** - Extract ownership patterns (`strict` profile only)
5. **Recent Patterns** - Detect bug fix, security, or refactoring history

**Quick Reference Commands:**

```bash
# Coupling analysis - files that change together
git log --name-only --pretty=format: -- <files> | sort | uniq -c | sort -rn | head -20

# Hot spot detection - change frequency
git log --since="3 months ago" --name-only --pretty=format: | sort | uniq -c | sort -rn | head -20

# Author context - ownership
git shortlog -sn -- <files>

# Recent patterns - commit messages
gi