code-review-quality

When to Use

• PR code reviews
• Pair programming feedback
• Establishing team review standards
• Mentoring developers

Feedback Priority Levels

| Level | Icon | Meaning | Action |

|-------|------|---------|--------|

| Blocker | 🔴 | Bug/security/crash | Must fix before merge |

| Major | 🟡 | Logic issue/test gap | Should fix before merge |

| Minor | 🟢 | Style/naming | Nice to fix |

| Suggestion | 💡 | Alternative approach | Consider for future |

Review Scope Limits

| Lines Changed | Recommendation |

|---------------|----------------|

| < 200 | Single review session |

| 200-400 | Review in chunks |

| > 400 | Request PR split |

What to Focus On

| ✅ Review | ❌ Skip |

|-----------|---------|

| Logic correctness | Formatting (use linter) |

| Security risks | Naming preferences |

| Test coverage | Architecture debates |

| Performance issues | Style opinions |

| Error handling | Trivial changes |

---

Blocker (Must Fix)

```markdown

🔴 BLOCKER: SQL Injection Risk

This query is vulnerable to SQL injection:

```javascript

db.query(SELECT * FROM users WHERE id = ${userId})

```

Fix: Use parameterized queries:

```javascript

db.query('SELECT * FROM users WHERE id = ?', [userId])

```

Why: User input directly in SQL allows attackers to execute arbitrary queries.

```

Major (Should Fix)

```markdown

🟡 MAJOR: Missing Error Handling

What happens if fetchUser() throws? The error bubbles up unhandled.

Suggestion: Add try/catch with appropriate error response:

```javascript

try {

const user = await fetchUser(id);

return user;

} catch (error) {

logger.error('Failed to fetch user', { id, error });

throw new NotFoundError('User not found');

}

```

```

Minor (Nice to Fix)

```markdown

🟢 minor: Variable name could be clearer

d doesn't convey meaning. Consider daysSinceLastLogin.

```

Suggestion (Consider)

```markdown

💡 suggestion: Consider extracting this to a helper

This validation logic appears in 3 places. A validateEmail() helper would reduce duplication. Not blocking, but might be worth a follow-up PR.

```

---

Logic

• What happens when X is null/empty/negative?
• Is there a race condition here?
• What if the API call fails?

Security

• Is user input validated/sanitized?
• Are auth checks in place?
• Any secrets or PII exposed?

Testability

• How would you test this?
• Are dependencies injectable?
• Is there a test for the happy path? Edge cases?

Maintainability

• Will the next developer understand this?
• Is this doing too many things?
• Is there duplication we could reduce?

---

```typescript

// Comprehensive code review

await Task("Code Review", {

prNumber: 123,

checks: ['security', 'performance', 'testability', 'maintainability'],

feedbackLevels: ['blocker', 'major', 'minor'],

autoApprove: { maxBlockers: 0, maxMajor: 2 }

}, "qe-quality-analyzer");

// Security-focused review

await Task("Security Review", {

prFiles: changedFiles,

scanTypes: ['injection', 'auth', 'secrets', 'dependencies']

}, "qe-security-scanner");

// Test coverage review

await Task("Coverage Review", {

prNumber: 123,

requireNewTests: true,

minCoverageDelta: 0

}, "qe-coverage-analyzer");

```

---

Memory Namespace

```

aqe/code-review/

├── review-history/* - Past review decisions

├── patterns/* - Common issues by team/repo

├── feedback-templates/* - Reusable feedback

└── metrics/* - Review turnaround time

```

Fleet Coordination

```typescript

const reviewFleet = await FleetManager.coordinate({

strategy: 'code-review',

agents: [

'qe-quality-analyzer', // Logic, maintainability

'qe-security-scanner', // Security risks

'qe-performance-tester', // Performance issues

'qe-coverage-analyzer' // Test coverage

],

topology: 'parallel'

});

```

---

| ✅ Do | ❌ Don't |

|-------|---------|

| "Have you considered...?" | "This is wrong" |

| Explain why it matters | Just say "fix this" |

| Acknowledge good code | Only point out negatives |

| Suggest, don't demand | Be condescending |

| Review < 400 lines | Review 2000 lines at once |

---

• [agentic-quality-engineering](../agentic-quality-engineering/) - Agent coordination
• [security-testing](../security-testing/) - Security review depth
• [refactoring-patterns](../refactoring-patterns/) - Maintainability patterns

---

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.

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.