Code Review
Both sides of the code review process: requesting and receiving feedback.
When to Request Review
Mandatory
| Situation |
Why |
| After completing major feature |
Catch issues before they spread |
| Before merge to main |
Gate quality |
| After each task in subagent workflow |
Fix issues before compounding |
Valuable
| Situation |
Why |
| When stuck |
Fresh perspective |
| Before refactoring |
Baseline check |
| After fixing complex bug |
Verify fix doesn't introduce new issues |
Requesting Review
What to Provide
| Element |
Purpose |
| What was implemented |
Context for reviewer |
| Requirements/plan reference |
What it should do |
| Base SHA |
Starting point |
| Head SHA |
Ending point |
| Brief summary |
Quick orientation |
Acting on Feedback
| Severity |
Action |
| Critical |
Fix immediately |
| Important |
Fix before proceeding |
| Minor |
Note for later |
| Disagree |
Push back with reasoning |
Receiving Review
Core Principle
Verify before implementing. Ask before assuming. Technical correctness over social comfort.
The Response Pattern
- Read - Complete feedback without reacting
- Understand - Restate requirement (or ask)
- Verify - Check against codebase reality
- Evaluate - Technically sound for THIS codebase?
- Respond - Technical acknowledgment or reasoned pushback
- Implement - One item at a time, test each
Handling Unclear Feedback
| Situation |
Action |
| Some items unclear |
STOP - ask before implementing any |
| Partially understood |
Don't implement partial - items may be related |
| Scope unclear |
Ask for clarification |
Key concept: Partial understanding leads to wrong implementation. Clarify everything first.
When to Push Back
| Situation |
Push Back |
| Suggestion breaks existing functionality |
Yes |
| Reviewer lacks full context |
Yes |
| Unused feature (YAGNI violation) |
Yes |
| Technically incorrect for this stack |
Yes |
| Legacy/compatibility reasons exist |
Yes |
| Conflicts with architectural decisions |
Yes |
How to Push Back
| Do |
Don't |
| Use technical reasoning |
Be defensive |
| Ask specific questions |
Argue emotionally |
| Reference working tests/code |
Ignore valid feedback |
| Show evidence |
Just say "no" |
Implementation Order
When fixing multiple items:
- Clarify anything unclear FIRST
- Then implement in order:
- Blocking issues (breaks, security)
- Simple fixes (typos, imports)
- Complex fixes (refactoring, logic)
- Test each fix individually
- Verify no regressions
Response Patterns
Forbidden (Performative)
| Don't Say |
Why |
| "You're absolutely right!" |
Performative, not technical |
| "Great point!" |
Empty agreement |
| "Let me implement that now" |
Before verification |
Correct Responses
| Situation |
Response |
| Feedback is correct |
"Fixed. [Brief description]" |
| Need clarification |
"Need clarification on X before proceeding" |
| Disagree |
"[Technical reasoning why current approach is better]" |
| Can't verify |
"Can't verify without [X]. Should I investigate?" |
Key concept: Actions speak. Just fix it. The code shows you heard the feedback.
Red Flags
| Never Do |
Why |
| Skip review because "it's simple" |
Simple changes cause bugs too |
| Ignore Critical issues |
They're critical for a reason |
| Proceed with unfixed Important issues |
They'll compound |
| Argue with valid technical feedback |
Ego over quality |
| Implement without understanding |
Wrong fixes waste time |
Source-Specific Handling
| Source |
Approach |
| User |
Trusted - implement after understanding, skip to action |
| External reviewer |
Evaluate technically before implementing |
| Automated tool |
Verify relevance to your context |