Code Review Skill

Perform a structured, multi-dimensional code review. The review covers five dimensions in order: Security → Quality → Performance → Architecture → Testing.

Workflow

1. Determine Scope

If $0 (the first argument) is provided and refers to a specific file, review only that file. Otherwise review the current working tree — inspect git diff (uncommitted changes) and/or git diff HEAD~1 (last commit). If neither produces meaningful diff context, ask the user what to review.

2. Gather Context

• Read the files to be reviewed in full.
• For each file, note its language, framework, and role in the project.
• Search for related configuration files (package.json, Cargo.toml, go.mod, tsconfig.json, etc.) if they inform the review.

3. Review Dimensions

Review each dimension in order. Within each dimension, list findings. Skip a dimension only if it is clearly not applicable (e.g., a CSS change needs no security review — state the skip and a one-sentence reason).

Dimension Detail: Security

Check for common vulnerability classes relevant to the code:
- Injection (SQL, NoSQL, OS command, template, XSS)
- Broken Access Control — missing auth checks, IDOR, privilege escalation
- Secrets — hardcoded tokens, keys, passwords, internal URLs shipped to client
- Input Validation — missing or incorrect sanitization, type coercion issues
- Unsafe Deserialization — eval, pickle, JSON.parse on untrusted input
- Data Exposure — PII, internal IPs/structure in error messages or responses
- Dependencies — use of known-vulnerable library versions or deprecated APIs

Label each security finding with severity: CRITICAL (exploitable in production), HIGH (exploitable with preconditions), MEDIUM (defense-in-depth), LOW (best practice).

Dimension Detail: Quality

Check for:
- Logic bugs — off-by-one, incorrect comparison, wrong operator, missing early return
- Error handling — swallowed errors, overly broad catches, missing cleanup (finally / defer / using)
- Null/undefined safety — unchecked nullable returns or parameters
- Code smells — long functions (>40 lines), deeply nested conditionals (>3 levels), magic numbers, mutating inputs
- Duplication — copy-pasted logic that should be extracted
- Readability — misleading names, unnecessary complexity, missing abstractions
- Edge cases — empty collections, zero values, boundary inputs, concurrent access

Dimension Detail: Performance

Check for:
- N+1 queries in database access patterns
- Unnecessary allocation in hot paths — temporary objects, repeated computations
- Blocking calls in async/event-loop context
- Large payloads — fetching more data than needed, missing pagination
- Caching missed — repeated identical I/O or computation
- Inefficient data structures — O(n²) loops that could be O(n log n) or better

Skip this dimension for trivial or non-performance-sensitive changes (config files, docs, UI layout only), stating the reason.

Dimension Detail: Architecture

Check for:
- Layer violation — business logic in controllers, data access in views, etc.
- Coupling — tight coupling to concrete implementations, static singletons, global state
- Cohesion — modules doing too many unrelated things
- Pattern consistency — does the change follow the project's established patterns? If it breaks them, is the break justified?
- API design — backward compatibility, sensible defaults, consistent naming
- Boundaries — proper separation between public API and internal implementation

Dimension Detail: Testing

Check for:
- Coverage gap — is the changed code exercised by existing or new tests?
- Test correctness — do tests actually assert the right behavior? Are they flaky?
- Test quality — does the test describe intent? Are test data and setup clear?
- Testability — does the change make testing harder (static methods, hidden dependencies, global state)?

4. Produce Report

Output a structured markdown report containing:

## Review Summary

**Scope**: <files or diff range reviewed>
**Overall**: <PASS / PASS_WITH_CONCERNS / FAIL>  (one-line rationale)

### Findings

#### 🔴 Security (N findings)
| Severity | File | Line | Finding |
|----------|------|------|---------|
| CRITICAL | src/x.ts | 42 | ... |
| HIGH | ... | ... | ... |

- **Finding title** [severity] — explanation, impact, and fix suggestion.

#### 🟠 Quality (N findings)
...

#### 🟡 Performance (N findings)
...

#### 🔵 Architecture (N findings)
...

#### 🟢 Testing (N findings)
...

### Recommendations

1. **Must fix** — blocking issues (critical security, correctness bugs)
2. **Should fix** — high-severity issues that should be addressed before merge
3. **Consider** — improvements for future iterations

5. Closing

• If the review found blocking issues, clearly state that the code should NOT be merged as-is.
• If all findings are advisory, state that the code is acceptable with optional improvements.
• Offer to write fixes or tests if the user requests it.

Rules

1. Be specific — always include file paths and line numbers. Vague feedback is not useful.
2. Be constructive — every finding must include a concrete suggestion for improvement.
3. Prioritize — not all findings are equal. Focus on what matters.
4. Respect project conventions — if the project has an established pattern that differs from general best practice, note it but do not flag it as an error unless it causes actual problems.
5. Don't bikeshed — skip style nitpicks unless they affect correctness or readability.
6. False positives — if you are unsure about a finding, state the uncertainty. Flag it with a question rather than an assertion.

Output Format

Always produce the Review Summary section at the top so the reader immediately understands the verdict. Detailed findings follow.

When reviewing a PR or branch, include a brief assessment of whether the change is safe to merge.