dev-code-reviewer

Dev-Code-Reviewer — Code Review Guide

> C0/C1 work (small local patches): See dev §0.0 Work Classifier + §0.1 Patch Fast-Path before reading references.

> Always read dev/SKILL.md first for project-wide conventions before applying review rules.

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

Review Posture (REVIEW-POSTURE-01)

Review as a skeptical, independent outsider. Executor claims, passing tests, AI summaries, and user-facing "done" prose are untrusted until you confirm them yourself. Inspect artifacts before believing them; a green run you did not read is not evidence.

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

---

Modular References

External/current review evidence

For dependency CVEs, release-note claims, package maintainer/source checks,

provider behavior, or other current/public evidence used in a review, read the

active search skill and follow its query-rewrite, source-fetch, and

evidence-status rules. Browser fetch/open/text/get-dom/snapshot is downstream

verification after candidate URLs exist, not a raw-query search substitute.

---

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.

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.

Delegation: coupling classification belongs to dev-architecture §3; boundary and validation-location findings belong to dev-architecture §4.

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.

Output Contract (REVIEW-OUTPUT-01)

Tool findings go first; then manual findings sorted Critical > High > Medium > Low > Style; then a dedicated blocking_issues block; verdict last. Every finding carries a concrete trigger, impact, and path:line (FAMILY-CITE-01). Do not file pre-existing debt unless the patch worsened it. When a change introduces a value/type/message crossing a module boundary, trace the consumer side before declaring it correct.

Regression & false-confidence tests (REVIEW-REGRESS-01)

Run a dedicated pass: what previously-working behavior can now break, and do the tests cover that surface? Flag deletion-only "fixes", tautological tests, tests that merely mirror the implementation, and scope-drift abstractions added beyond the request.

---

2. Quality Thresholds

Flag these during review:

File Size Guidance

Canonical rule imported from dev-architecture §1: >400 LOC -> split (DEFAULT).

Review Verdict

Deterministic blocker semantics (REVIEW-BLOCK-01): any unresolved Critical or High blocks the merge. Medium may pass only when explicitly judged non-blocking; Style never affects the verdict.

---

3. Common Antipatterns

Structural

Dead Code

Dead code is a maintenance tax — remove rather than comment out.

Logic

Security

Security review items are canonical in §3.5. Use that checklist for hardcoded

secrets, injection, validation, auth, authorization, and logging findings.

Performance

Async

---

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 When Relevant

> 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

Frontend-Specific

General

---

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

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

---

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 (verified 2026-07-02)

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

Coordination rules:

• If an external AI tool already reviewed the PR, read its findings first, then focus

manual review on what tools cannot do: architectural fit, business intent, cross-system impact.

• STRICT (REVIEW-AI-EVIDENCE-01): AI review findings are evidence to inspect, not

authority — de-duplicate, reproduce, and severity-normalize them before inclusion.

Published evaluation of AI reviewers shows frequent misses on critical vulnerabilities

(SQLi/XSS/deserialization) with low-severity skew, so AI output never replaces the

§3.5 security pass (source: arXiv:2509.13650, checked 2026-07-02).

---

7. Reviewing AI-Generated Code

AI-authored diffs have distinct failure modes. Run this pass IN ADDITION to §1-3 when

the diff is substantially AI-generated (agent commits, Copilot/Cursor bulk changes):

Agentic/security review trigger (DEFAULT): if a PR adds MCP servers, tools, agents,

RAG components, persistent memory, delegated credentials, or autonomous actions, invoke

dev-security and map risks to the OWASP LLM Top 10 (2025) and the OWASP Top 10 for

Agentic Applications 2026.

---