Conduct context-driven code reviews focusing on quality, testability, and maintainability. Use when reviewing code, providing feedback, or establishing review practices.
Instrucciones de origen · Vista previa de solo lectura
name
code-review-quality
description
Conduct context-driven code reviews focusing on quality, testability, and maintainability. Use when reviewing code, providing feedback, or establishing review practices.
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.
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
