Code Review Quality

1---2name: code-review-quality3description: "Conduct context-driven code reviews focusing on quality, testability, and maintainability. Use when reviewing code, providing feedback, or establishing review practices."4category: development-practices5priority: high6tokenEstimate: 9007agents: [qe-quality-analyzer, qe-security-scanner, qe-performance-tester, qe-coverage-analyzer]8implementation_status: optimized9optimization_version: 1.010last_optimized: 2025-12-0211dependencies: []12quick_reference_card: true13tags: [code-review, feedback, quality, testability, maintainability, pr-review]14trust_tier: 215validation:16  schema_path: schemas/output.json17  validator_path: scripts/validate-config.json18---19 20# Code Review Quality21 22<default_to_action>23When reviewing code or establishing review practices:241. PRIORITIZE feedback: 🔴 Blocker (must fix) → 🟡 Major → 🟢 Minor → 💡 Suggestion252. FOCUS on: Bugs, security, testability, maintainability (not style preferences)263. ASK questions over commands: "Have you considered...?" > "Change this to..."274. PROVIDE context: Why this matters, not just what to change285. LIMIT scope: Review < 400 lines at a time for effectiveness29 30**Quick Review Checklist:**31- Logic: Does it work correctly? Edge cases handled?32- Security: Input validation? Auth checks? Injection risks?33- Testability: Can this be tested? Is it tested?34- Maintainability: Clear naming? Single responsibility? DRY?35- Performance: O(n²) loops? N+1 queries? Memory leaks?36 37**Critical Success Factors:**38- Review the code, not the person39- Catching bugs > nitpicking style40- Fast feedback (< 24h) > thorough feedback41</default_to_action>42 43## Quick Reference Card44 45### When to Use46- PR code reviews47- Pair programming feedback48- Establishing team review standards49- Mentoring developers50 51### Feedback Priority Levels52| Level | Icon | Meaning | Action |53|-------|------|---------|--------|54| Blocker | 🔴 | Bug/security/crash | Must fix before merge |55| Major | 🟡 | Logic issue/test gap | Should fix before merge |56| Minor | 🟢 | Style/naming | Nice to fix |57| Suggestion | 💡 | Alternative approach | Consider for future |58 59### Review Scope Limits60| Lines Changed | Recommendation |61|---------------|----------------|62| < 200 | Single review session |63| 200-400 | Review in chunks |64| > 400 | Request PR split |65 66### What to Focus On67| ✅ Review | ❌ Skip |68|-----------|---------|69| Logic correctness | Formatting (use linter) |70| Security risks | Naming preferences |71| Test coverage | Architecture debates |72| Performance issues | Style opinions |73| Error handling | Trivial changes |74 75---76 77## Feedback Templates78 79### Blocker (Must Fix)80```markdown81🔴 **BLOCKER: SQL Injection Risk**82 83This query is vulnerable to SQL injection:84```javascript85db.query(`SELECT * FROM users WHERE id = ${userId}`)86```87 88**Fix:** Use parameterized queries:89```javascript90db.query('SELECT * FROM users WHERE id = ?', [userId])91```92 93**Why:** User input directly in SQL allows attackers to execute arbitrary queries.94```95 96### Major (Should Fix)97```markdown98🟡 **MAJOR: Missing Error Handling**99 100What happens if `fetchUser()` throws? The error bubbles up unhandled.101 102**Suggestion:** Add try/catch with appropriate error response:103```javascript104try {105  const user = await fetchUser(id);106  return user;107} catch (error) {108  logger.error('Failed to fetch user', { id, error });109  throw new NotFoundError('User not found');110}111```112```113 114### Minor (Nice to Fix)115```markdown116🟢 **minor:** Variable name could be clearer117 118`d` doesn't convey meaning. Consider `daysSinceLastLogin`.119```120 121### Suggestion (Consider)122```markdown123💡 **suggestion:** Consider extracting this to a helper124 125This validation logic appears in 3 places. A `validateEmail()` helper would reduce duplication. Not blocking, but might be worth a follow-up PR.126```127 128---129 130## Review Questions to Ask131 132### Logic133- What happens when X is null/empty/negative?134- Is there a race condition here?135- What if the API call fails?136 137### Security138- Is user input validated/sanitized?139- Are auth checks in place?140- Any secrets or PII exposed?141 142### Testability143- How would you test this?144- Are dependencies injectable?145- Is there a test for the happy path? Edge cases?146 147### Maintainability148- Will the next developer understand this?149- Is this doing too many things?150- Is there duplication we could reduce?151 152## Minimum Findings Enforcement153Reviews must meet a minimum weighted finding score of 3.0 (CRITICAL=3, HIGH=2, MEDIUM=1, LOW=0.5, INFORMATIONAL=0.25). If the initial review falls short, run the qe-devils-advocate agent as a meta-reviewer to find additional observations. Every review should have at least 3 actionable observations.154 155---156 157## Agent-Assisted Reviews158 159```typescript160// Comprehensive code review161await Task("Code Review", {162  prNumber: 123,163  checks: ['security', 'performance', 'testability', 'maintainability'],164  feedbackLevels: ['blocker', 'major', 'minor'],165  autoApprove: { maxBlockers: 0, maxMajor: 2 }166}, "qe-quality-analyzer");167 168// Security-focused review169await Task("Security Review", {170  prFiles: changedFiles,171  scanTypes: ['injection', 'auth', 'secrets', 'dependencies']172}, "qe-security-scanner");173 174// Test coverage review175await Task("Coverage Review", {176  prNumber: 123,177  requireNewTests: true,178  minCoverageDelta: 0179}, "qe-coverage-analyzer");180```181 182---183 184## Agent Coordination Hints185 186### Memory Namespace187```188aqe/code-review/189├── review-history/*     - Past review decisions190├── patterns/*           - Common issues by team/repo191├── feedback-templates/* - Reusable feedback192└── metrics/*            - Review turnaround time193```194 195### Fleet Coordination196```typescript197const reviewFleet = await FleetManager.coordinate({198  strategy: 'code-review',199  agents: [200    'qe-quality-analyzer',    // Logic, maintainability201    'qe-security-scanner',    // Security risks202    'qe-performance-tester',  // Performance issues203    'qe-coverage-analyzer'    // Test coverage204  ],205  topology: 'parallel'206});207```208 209---210 211## Review Etiquette212 213| ✅ Do | ❌ Don't |214|-------|---------|215| "Have you considered...?" | "This is wrong" |216| Explain why it matters | Just say "fix this" |217| Acknowledge good code | Only point out negatives |218| Suggest, don't demand | Be condescending |219| Review < 400 lines | Review 2000 lines at once |220 221---222 223## Related Skills224- [agentic-quality-engineering](../agentic-quality-engineering/) - Agent coordination225- [security-testing](../security-testing/) - Security review depth226- [refactoring-patterns](../refactoring-patterns/) - Maintainability patterns227 228---229 230## Remember231 232**Prioritize feedback:** 🔴 Blocker → 🟡 Major → 🟢 Minor → 💡 Suggestion. Focus on bugs and security, not style. Ask questions, don't command. Review < 400 lines at a time. Fast feedback (< 24h) beats thorough feedback.233 234**With Agents:** Agents automate security, performance, and coverage checks, freeing human reviewers to focus on logic and design. Use agents for consistent, fast initial review.235 236## Skill Composition237 238- **Security concerns** → Compose with `/security-testing` for security-focused review239- **Coverage check** → Run `/qe-coverage-analysis` on changed files240- **Ship decision** → Feed review results into `/qe-quality-assessment`241 242## Gotchas243 244- Agent reviews >400 lines at once and misses issues — chunk reviews to 200-400 lines maximum245- Nitpicking style while missing logic bugs is the #1 agent review failure — prioritize correctness over formatting246- Agent approves code that compiles but has subtle race conditions — always check shared state and async patterns247- Review comments without suggested fixes are unhelpful — always include a proposed alternative248- Agent doesn't check if the PR actually solves the linked issue — verify the stated problem is actually fixed