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 | File | When to Read | What It Covers | |------|-------------|----------------| | `references/tech-debt.md` | Tech debt inventory or paydown | Debt quadrant, inventory template, review integration, paydown budget | | `references/ai-assisted-review.md` | Using AI review tools in PR workflow | AI review workflow, severity classification, re-review policy, exclusions, metrics | ## 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. | Tool | Catches | Misses | Key Rules | |------|---------|--------|-----------| | ESLint/Ruff | Style, simple bugs, import issues | Architecture, business logic | `import/no-cycle`, `no-unused-vars`, `no-floating-promises`, `complexity` | | tsc/mypy | Type errors, null safety | Runtime behavior, performance | `strict`, `noImplicitAny`, `strictNullChecks` | | Semgrep | Injection, auth bypass, SSRF | Complex multi-step vulnerabilities | `javascript.lang.security.audit.sqli` | | 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. 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: | Issue | Threshold | Severity | |-------|-----------|----------| | Long function | >50 lines | Medium | | Large file | >400 lines | Medium; apply `dev-architecture` §1 canonical split rule | | 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 Canonical rule imported from `dev-architecture` §1: **>400 LOC -> split (DEFAULT)**. | Range | Interpretation | |-------|---------------| | 200-400 lines | Healthy — easy to navigate and review | | 400-500 lines | Should split unless the author states a concrete reason | | >500 lines | Blocking review finding unless already being split in this diff | ### 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 | 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 | 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 | ### Dead Code | Pattern | Detection | Fix | |---------|-----------|-----| | Unreachable code after return/throw | `no-unreachable`, compiler warnings | Delete the dead branch | | Unused imports / variables | `no-unused-vars`, `@typescript-eslint/no-unused-vars` | Remove | | Commented-out code blocks | Manual review | Delete — use version control history | | Unused exports | `ts-prune`, `knip`, grep for import sites | Remove export; delete if no internal use | | Stale feature-flagged code | Check flag status in flag service | Remove dead branch and the flag check | Dead code is a maintenance tax — remove rather than comment out. ### 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 | Delegate placement to `dev-architecture` §4; schema/content depth to `dev-security` | ### 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 | 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