Code Review Quality

<default_to_action>

When reviewing code or establishing review practices:

• PRIORITIZE feedback: 🔴 Blocker (must fix) → 🟡 Major → 🟢 Minor → 💡 Suggestion

• FOCUS on: Bugs, security, testability, maintainability (not style preferences)

• ASK questions over commands: "Have you considered...?" > "Change this to..."

• PROVIDE context: Why this matters, not just what to change

• LIMIT scope: Review < 400 lines at a time for effectiveness

Quick Review Checklist:

• Logic: Does it work correctly? Edge cases handled?

• Security: Input validation? Auth checks? Injection risks?

• Testability: Can this be tested? Is it tested?

• Maintainability: Clear naming? Single responsibility? DRY?

• Performance: O(n²) loops? N+1 queries? Memory leaks?

Critical Success Factors:

• Review the code, not the person

• Catching bugs > nitpicking style

• Fast feedback (< 24h) > thorough feedback

</default_to_action>

Quick Reference Card

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

Feedback Templates

Blocker (Must Fix)

🔴 **BLOCKER: SQL Injection Risk**

This query is vulnerable to SQL injection:

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

**Fix:** Use parameterized queries:

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

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

Major (Should Fix)

🟡 **MAJOR: Missing Error Handling**

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

**Suggestion:** Add try/catch with appropriate error response:

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)

🟢 **minor:** Variable name could be clearer

`d` doesn't convey meaning. Consider `daysSinceLastLogin`.

Suggestion (Consider)

💡 **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.

Review Questions to Ask

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?

Minimum Findings Enforcement

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

Agent-Assisted Reviews

// 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");

Agent Coordination Hints

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

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'

});

Review Etiquette

✅ 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

Related Skills

• agentic-quality-engineering - Agent coordination

• security-testing - Security review depth

• refactoring-patterns - Maintainability patterns

Remember

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.

Skill Composition

• Security concerns → Compose with /security-testing for security-focused review

• Coverage check → Run /qe-coverage-analysis on changed files

• Ship decision → Feed review results into /qe-quality-assessment

Gotchas

• Agent reviews >400 lines at once and misses issues — chunk reviews to 200-400 lines maximum

• Nitpicking style while missing logic bugs is the #1 agent review failure — prioritize correctness over formatting

• Agent approves code that compiles but has subtle race conditions — always check shared state and async patterns

• Review comments without suggested fixes are unhelpful — always include a proposed alternative

• Agent doesn't check if the PR actually solves the linked issue — verify the stated problem is actually fixed