dev-code-reviewer

name	dev-code-reviewer
description	Code review guide for all orchestrated sub-agents. Review process, quality thresholds, antipattern detection, giving/receiving feedback. Available to any agent regardless of role — read this SKILL.md when performing or receiving code reviews.

Dev-Code-Reviewer — Code Review Guide

Systematic code review patterns for finding real issues, not bikeshedding.

When to Activate

• Reviewing code changes (own or others')
• Receiving code review feedback
• Assessing code quality before merge
• Evaluating pull requests or diffs
• Pre-refactoring quality baseline check

---

1. Code Review Process

Pre-Review Checklist

Before reviewing any code, verify:

• Build passes (no compile/type errors)
• Tests pass (all green)
• PR/diff description explains what changed and why
• Diff is reasonable size (<500 changed lines — split larger PRs)

Automated Pre-Scan (Run Before Manual Review)

Before reading a single line of code, run automated tools on changed files:

Run project-native linters, type checker, and tests before reviewing.

Pre-Scan Rules:

1. Critical/error findings → block review. Don't waste human review cycles on machine-detectable problems.
2. Warnings → note for review, don't block. Mention in review but don't make them blocking.
3. Tool findings go first in review output, before manual findings.
4. No tool available? Skip gracefully — pre-scan is additive, not a gate.

Tool	Catches	Misses
ESLint/Ruff	Style, simple bugs, import issues	Architecture, business logic
tsc/mypy	Type errors, null safety	Runtime behavior, performance
Semgrep	Injection, auth bypass, SSRF	Complex multi-step vulnerabilities
npm audit/pip-audit	Known CVEs in deps	Zero-day, license issues

Separation of concerns: Tools catch patterns; humans catch intent. Focus manual review on architecture, correctness, and business logic that tools cannot evaluate.

Review Order (by impact, not preference)

1. Architecture — Does the approach make sense? Right layer? Right abstraction? Is this the right place for this code?
2. Correctness — Logic errors, edge cases, off-by-one, null/undefined handling, error paths
3. Security — Input validation, injection risks, auth checks, secrets exposure
4. Performance — N+1 queries, unbounded collections, missing indexes, unnecessary computation
5. Maintainability — Naming, structure, complexity, test coverage, documentation
6. Style — Last priority. Don't bikeshed formatting when there are real issues.

Review Mindset

• Be specific. "This could fail" → "This throws if user is null on line 42"
• Suggest, don't demand. Unless it's a security or correctness issue.
• Explain why. Not just "change X to Y" but "X causes N+1 queries because..."
• Acknowledge good work. If a complex problem is solved elegantly, say so briefly.

---

2. Quality Thresholds

Flag these during review:

Issue	Threshold	Severity
Long function	>50 lines	Medium
Large file	>500 lines (target 200-400)	Medium
God class	>20 methods	High
Too many parameters	>5	Medium
Deep nesting	>4 levels	Medium
High cyclomatic complexity	>10 branches	High
Missing error handling	any unhandled async	High
Hardcoded secrets	API keys, passwords in source	Critical
SQL injection	string concatenation in queries	Critical
Debug statements	console.log, debugger left in	Low
TODO/FIXME	unresolved in production code	Low
TypeScript any	bypassing type safety	Medium

File Size Guidance

Range	Interpretation
200-400 lines	Healthy — easy to navigate and review
400-500 lines	Acceptable — consider splitting if complexity is high
500-800 lines	Review trigger — actively plan extraction
>800 lines	Split required — too large for effective review or AI context

Review Verdict

Indicator	Verdict	Action
No high/critical issues	✅ Approve	Merge
≤2 high issues, clearly fixable	🔧 Approve with suggestions	Fix before merge
Multiple high issues	⚠️ Request changes	Author must address
Any critical issue	🚫 Block	Cannot merge until resolved

---

3. Common Antipatterns

Structural

Pattern	Symptom	Fix
God class	One class does everything	Split by single responsibility
Long method	Function does 5+ distinct things	Extract named helper functions
Deep nesting	4+ levels of if/for/try	Guard clauses, early returns, extraction
Feature envy	Method uses another object's data more than its own	Move method to the data owner
Shotgun surgery	One change requires edits in 10+ files	Consolidate related logic

Logic

Pattern	Symptom	Fix
Boolean blindness	doThing(true, false, true)	Named options object or enum
Stringly typed	status === 'actve' (typo = silent bug)	Define enum or union type
Magic numbers	if (retries > 3)	Named constant: MAX_RETRIES = 3
Primitive obsession	Passing 5 related strings around	Create a data object/type
Direct mutation	user.name = 'x', arr.push(y)	Immutable: {...obj, name: 'x'}, [...arr, y]
Missing boundary validation	Business logic handles raw user input	Schema validation (Zod, Pydantic) at API entry point

Security

Pattern	Symptom	Fix
SQL injection	String concatenation in queries	Parameterized queries / prepared statements
Hardcoded secrets	apiKey = "sk-..." in source	Environment variables or secret manager
Missing validation	Raw user input passed to logic	Schema validation at API boundary
Overpermission	Broad access when narrow suffices	Principle of least privilege

Performance

Pattern	Symptom	Fix
N+1 queries	Loop → query per item	Batch fetch with WHERE IN (...)
Unbounded collections	.all() without LIMIT	Always paginate or set max
Missing index	Slow repeated lookups on same column	Add database index
Premature optimization	Complex caching for 10 rows	Profile first, optimize second

Async

Pattern	Symptom	Fix
Floating promise	doAsync() without await	Always await or handle rejection
Callback hell	4+ nested callbacks	Refactor to async/await
Missing timeout	External call can hang forever	Set timeout on all network calls

---

3.5 Security Review Quick-Check

For every review, scan for these OWASP-aligned red flags. Delegate to dev-security/SKILL.md for deep analysis.

Must-Check (Every PR)

Check	Red Flag	Severity
Hardcoded secrets	apiKey = "sk-...", DB URLs in source	Critical
SQL/NoSQL injection	String concatenation in queries	Critical
Missing input validation	User input passed to logic without schema check	High
Missing auth check	Endpoint accessible without authentication	High
BOLA (Broken Object Auth)	No ownership check on object access (/users/:id without verifying caller owns resource)	High
Secrets in logs	console.log(req.body) leaking tokens/passwords	High

Check When Relevant

Check	When	Red Flag
SSRF	External URL from user input	No URL allowlist, no domain validation
Path traversal	File path from user input	No path sanitization, ../ not blocked
Mass assignment	Object spread into DB model	Object.assign(model, req.body) without allowlist
Dep vulnerabilities	New dependencies added	No npm audit/pip-audit run
Lockfile changes	package-lock.json modified	Unexpected dependency resolution changes

Deep security analysis → invoke dev-security/SKILL.md. This checklist catches surface-level issues during code review; dev-security provides OWASP Top 10 depth, ASVS checklists, and static analysis integration.

---

3.6 Performance Review Quick-Check

Scan every PR for these common performance pitfalls:

Database & API

Check	Red Flag	Fix
N+1 queries	Loop containing DB call or API fetch	Batch with WHERE IN (...) or DataLoader
Missing pagination	.findAll() or SELECT * without LIMIT	Add cursor-based or offset pagination
Missing index	New WHERE/JOIN column without index	CREATE INDEX on filtered/joined columns
Unbounded query	No LIMIT on user-facing list endpoints	Always set max page size

Frontend-Specific

Check	Red Flag	Fix
Unnecessary re-renders	State updates in parent causing child re-render cascade	React.memo, useMemo, extract state down
Bundle size impact	New large dependency (>50KB gzipped)	Check bundlephobia.com, consider alternatives or lazy loading
Missing key prop	List rendering without stable keys	Use unique ID, never array index for dynamic lists
Unoptimized images	Large images without next/image, loading="lazy", or srcset	Use framework image optimization

General

Check	Red Flag	Fix
Missing timeout	External HTTP call without timeout	Set timeout on all network requests
Sync blocking	CPU-intensive work on main thread/event loop	Offload to worker/queue
Memory leak	Event listeners/subscriptions without cleanup	Add cleanup in useEffect return / finally block

---

4. Receiving Code Review

The Response Pattern

When receiving review feedback:

1. READ — Complete feedback without reacting immediately
2. UNDERSTAND — Restate the technical requirement in your own words
3. VERIFY — Check the suggestion against codebase reality (does it apply here?)
4. EVALUATE — Is it technically sound for THIS codebase, not just in theory?
5. RESPOND — Technical acknowledgment or reasoned pushback
6. IMPLEMENT — One item at a time, test each change

When to Push Back

Push back when:

• Suggestion breaks existing functionality (test it)
• Reviewer lacks full context of the current architecture
• Violates YAGNI — feature is unused (grep the codebase to verify)
• Technically incorrect for this technology stack
• Conflicts with established architectural decisions

How: Use technical reasoning. Reference working tests, existing code, or documented decisions. Never push back emotionally — always with evidence.

Implementation Order (multi-item feedback)

1. Clarify ALL unclear items FIRST — don't implement based on partial understanding
2. Blocking issues (security, data loss, broken functionality)
3. Simple fixes (typos, missing imports, naming)
4. Complex fixes (refactoring, logic changes)
5. Test EACH fix individually. Verify no regressions after each.

Acknowledging Feedback

✅ "Fixed. Changed X to use parameterized query."
✅ "Good catch — the null check was missing. Added guard on line 42."
✅ Just fix it and show the result in code.

❌ "You're absolutely right!"
❌ "Great point! Thanks for catching that!"
❌ Any performative agreement without verification

---

5. Requesting Code Review

When to Request

Situation	Priority
Before merge to main	Mandatory
After major feature completion	Mandatory
Before large refactoring	Mandatory
After complex bug fix	Recommended
When stuck on approach	Recommended
Small config/docs changes	Skip unless impactful

How to Request

1. Ensure build passes and all tests are green
2. Identify the diff range (base commit → head commit)
3. Provide a summary: what was implemented, what it should do, areas to focus on
4. Keep the diff <500 lines. Split larger changes into reviewable chunks.

Acting on Feedback

Severity	Action
Critical	Fix immediately, re-request review
High	Fix before proceeding to next task
Medium	Fix before merge, can continue other work
Low	Note for later, apply if trivial
Style	Apply if trivial, otherwise defer to team conventions

---

6. Sub-Agent Review Mode

Parallelize review only when domain breadth exceeds one reviewer's context (e.g., frontend + backend + infra in a single diff, or when the diff spans too many unrelated domains for a single pass). Each sub-agent receives its file subset, the review process from sections 1-5, and outputs structured findings. The orchestrator deduplicates, normalizes severity, and presents a unified review.

AI Tool Integration Awareness

When external AI review tools are available, coordinate — don't duplicate:

Tool	Strengths	Use When	Agent Focus Shifts To
GitHub Copilot Code Review	Full repo context, multi-model, auto-fix PRs	PR review on GitHub	Architecture, business logic, domain correctness
CodeRabbit	40+ linters, learnable preferences, low false-positive	Team with .coderabbit.yml configured	Cross-service impact, subtle logic errors
SonarQube	Enterprise SAST, tech debt tracking, security depth	Regulated environments, existing setup	Review findings, add context tools miss
Manual agent review	Full codebase understanding, intent verification	No external tools, offline, sensitive code	Everything — full §1-5 process

Coordination rule: If an external AI tool already reviewed the PR, read its findings first, then focus manual review on what the tool explicitly cannot do: architectural fit, business intent alignment, and cross-system impact.

---