code-review-quality

SKILL.md

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:
```javascript
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)
```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)

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