Jaw 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.

dev is canonical: dev §0.2 Rule Classes, §3 Verification Gate, and §5 Safety Rules apply to all work governed by this skill. 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

dev-testing owns test adequacy and QA execution. dev-debugging owns RCA when review discovers a runtime failure. dev-architecture owns coupling and boundary placement.

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

This section owns the mandatory review pre-scan; dev-security owns security policy and deep analysis. Use this 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.

AI Slop Cleanup Checklist (REVIEW-SLOP-01)

When targeting a post-implementation cleanup pass ("remove slop", "clean AI code", "deslop"), or >=3 slop items are caught during normal review, apply this checklist. Safety invariant: lock behavior with green tests BEFORE removing any code.

Stylistic: (1) Obvious comments (restating code, trivial docstrings, commented-out code; KEEP: why-comments, ticket links, regex explanations). (2) Over-defensive code (null checks for guaranteed values, broad except Exception/empty catch {}; KEEP: boundary validation). (3) Excessive complexity (deep nesting >3, nested ternaries, if/elif for type discrimination -> match/case, object annotation -> Protocol/TypeVar).

Structural: (4) Needless abstraction (pass-through wrappers, single-use helpers, speculative indirection). (5) Boundary violations (wrong-layer imports, hidden coupling; delegate to dev-architecture). (6) Oversized modules (>250 pure LOC is a slop-cleanup smell, not a split mandate; dev-architecture owns >400L canonical split).

Hidden cost: (7) Performance equivalences (O(n^2) where O(n) exists, repeated computation). (8) Scope leaks (mutable global state, scattered env reads).

Coverage: (9) Missing behavior tests for changed paths.

Changed-File Coverage Ledger (REVIEW-COVERAGE-01, DEFAULT)

Account for every changed file as reviewed, skipped (reason), or out-of-scope (reason) before verdict. Generated, lock, vendored, binary, and outside-domain files may be skipped only with an explicit reason. Any unaccounted file makes the verdict incomplete.

Finding Falsification (REVIEW-FALSIFY-01, DEFAULT)

Before reporting a finding:

1. State it as a testable claim.
2. Search tests, guards, caller context, and docs for contradictory evidence.
3. Downgrade or retract the claim when contradictory evidence disproves it.
4. Retain the claim when it survives the falsification attempt.

Every finding includes verification: verified|unverified. Use unverified when the falsification attempt could not be completed or evidence is incomplete.

Interdiff Re-Review (REVIEW-INTERDIFF-01, DEFAULT)

Anchor the re-review to both the previous reviewed commit/range and the new head; record those anchors in the review. Review only the interdiff, preserve unresolved findings, verify each claimed fix, and process new findings normally. Revisit unchanged code when a cross-file dependency changed. If either anchor is missing, history was rewritten ambiguously, or the interdiff cannot be trusted, fall back to a full review of the current base-to-head diff.

Deletion and worktree rules

Three DEFAULT rules live in references/deletion-and-worktree.md:

• REVIEW-GUARD-REMOVAL-01 — deleting a trust-boundary guard needs a regression test that exercises the deleted path. The expected result inverts by deletion kind: a relocating deletion must stay GREEN, a non-replacing one goes RED.
• REVIEW-REMOVED-BACKEND-01 — a document listing available capabilities gets checked for retired ones reappearing as available.
• REVIEW-WORKTREE-01 — never check out another review ref in the worktree you were handed; the condition is observable state, not ownership.