Code Review Excellence
Unverified●29/40Claude Code◐PartialHas SKILL.md but declares no allowed-tools — Claude Code will ask for permission each time
Cursor◐PartialPlain prose you can paste in — but no Cursor rules file
Codex◐PartialPlain prose you can paste in — but no AGENTS.md
Gemini CLI◐PartialPlain prose you can paste in
Copilot◐PartialPlain prose you can paste in — but no Copilot instructions file
npx agentalley add code-review-excellenceWho is stuck, and on what
Master effective code review practices to provide constructive feedback, catch bugs early, and foster knowledge sharing while maintaining team morale. Use when reviewing pull requests, establishing review standards, or mentoring developers.
The whole source
Frontmatter — 2 properties
| name | code-review-excellence |
|---|---|
| description | Master effective code review practices to provide constructive feedback, catch bugs early, and foster knowledge sharing while maintaining team morale. Use when reviewing pull requests, establishing review standards, or mentoring developers. |
| 1 | --- |
| 2 | name: code-review-excellence |
| 3 | description: Master effective code review practices to provide constructive feedback, catch bugs early, and foster knowledge sharing while maintaining team morale. Use when reviewing pull requests, establishing review standards, or mentoring developers. |
| 4 | ---A5 — No allowed-tools declared — no way to tell what this skill may touch |
| 5 | |
| 6 | # Code Review Excellence |
| 7 | |
| 8 | Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement. |
| 9 | |
| 10 | ## When to Use This Skill |
| 11 | |
| 12 | - Reviewing pull requests and code changes |
| 13 | - Establishing code review standards for teams |
| 14 | - Mentoring junior developers through reviews |
| 15 | - Conducting architecture reviews |
| 16 | - Creating review checklists and guidelines |
| 17 | - Improving team collaboration |
| 18 | - Reducing code review cycle time |
| 19 | - Maintaining code quality standards |
| 20 | |
| 21 | ## Core Principles |
| 22 | |
| 23 | ### 1. The Review Mindset |
| 24 | |
| 25 | **Goals of Code Review:** |
| 26 | |
| 27 | - Catch bugs and edge cases |
| 28 | - Ensure code maintainability |
| 29 | - Share knowledge across team |
| 30 | - Enforce coding standards |
| 31 | - Improve design and architecture |
| 32 | - Build team culture |
| 33 | |
| 34 | **Not the Goals:** |
| 35 | |
| 36 | - Show off knowledge |
| 37 | - Nitpick formatting (use linters) |
| 38 | - Block progress unnecessarily |
| 39 | - Rewrite to your preference |
| 40 | |
| 41 | ### 2. Effective Feedback |
| 42 | |
| 43 | **Good Feedback is:** |
| 44 | |
| 45 | - Specific and actionable |
| 46 | - Educational, not judgmental |
| 47 | - Focused on the code, not the person |
| 48 | - Balanced (praise good work too) |
| 49 | - Prioritized (critical vs nice-to-have) |
| 50 | |
| 51 | ```markdown |
| 52 | ❌ Bad: "This is wrong." |
| 53 | ✅ Good: "This could cause a race condition when multiple users |
| 54 | access simultaneously. Consider using a mutex here." |
| 55 | |
| 56 | ❌ Bad: "Why didn't you use X pattern?" |
| 57 | ✅ Good: "Have you considered the Repository pattern? It would |
| 58 | make this easier to test. Here's an example: [link]" |
| 59 | |
| 60 | ❌ Bad: "Rename this variable." |
| 61 | ✅ Good: "[nit] Consider `userCount` instead of `uc` for |
| 62 | clarity. Not blocking if you prefer to keep it." |
| 63 | ``` |
| 64 | |
| 65 | ### 3. Review Scope |
| 66 | |
| 67 | **What to Review:** |
| 68 | |
| 69 | - Logic correctness and edge cases |
| 70 | - Security vulnerabilities |
| 71 | - Performance implications |
| 72 | - Test coverage and quality |
| 73 | - Error handling |
| 74 | - Documentation and comments |
| 75 | - API design and naming |
| 76 | - Architectural fit |
| 77 | |
| 78 | **What Not to Review Manually:** |
| 79 | |
| 80 | - Code formatting (use Prettier, Black, etc.) |
| 81 | - Import organization |
| 82 | - Linting violations |
| 83 | - Simple typos |
| 84 | |
| 85 | ## Review Process |
| 86 | |
| 87 | ### Phase 1: Context Gathering (2-3 minutes) |
| 88 | |
| 89 | ```markdown |
| 90 | Before diving into code, understand: |
| 91 | |
| 92 | 1. Read PR description and linked issue |
| 93 | 2. Check PR size (>400 lines? Ask to split) |
| 94 | 3. Review CI/CD status (tests passing?) |
| 95 | 4. Understand the business requirement |
| 96 | 5. Note any relevant architectural decisions |
| 97 | ``` |
| 98 | |
| 99 | ### Phase 2: High-Level Review (5-10 minutes) |
| 100 | |
| 101 | ```markdown |
| 102 | 1. **Architecture & Design** |
| 103 | - Does the solution fit the problem? |
| 104 | - Are there simpler approaches? |
| 105 | - Is it consistent with existing patterns? |
| 106 | - Will it scale? |
| 107 | |
| 108 | 2. **File Organization** |
| 109 | - Are new files in the right places? |
| 110 | - Is code grouped logically? |
| 111 | - Are there duplicate files? |
| 112 | |
| 113 | 3. **Testing Strategy** |
| 114 | - Are there tests? |
| 115 | - Do tests cover edge cases? |
| 116 | - Are tests readable? |
| 117 | ``` |
| 118 | |
| 119 | ### Phase 3: Line-by-Line Review (10-20 minutes) |
| 120 | |
| 121 | ```markdown |
| 122 | For each file: |
| 123 | |
| 124 | 1. **Logic & Correctness** |
| 125 | - Edge cases handled? |
| 126 | - Off-by-one errors? |
| 127 | - Null/undefined checks? |
| 128 | - Race conditions? |
| 129 | |
| 130 | 2. **Security** |
| 131 | - Input validation? |
| 132 | - SQL injection risks? |
| 133 | - XSS vulnerabilities? |
| 134 | - Sensitive data exposure? |
| 135 | |
| 136 | 3. **Performance** |
| 137 | - N+1 queries? |
| 138 | - Unnecessary loops? |
| 139 | - Memory leaks? |
| 140 | - Blocking operations? |
| 141 | |
| 142 | 4. **Maintainability** |
| 143 | - Clear variable names? |
| 144 | - Functions doing one thing? |
| 145 | - Complex code commented? |
| 146 | - Magic numbers extracted? |
| 147 | ``` |
| 148 | |
| 149 | ### Phase 4: Summary & Decision (2-3 minutes) |
| 150 | |
| 151 | ```markdown |
| 152 | 1. Summarize key concerns |
| 153 | 2. Highlight what you liked |
| 154 | 3. Make clear decision: |
| 155 | - ✅ Approve |
| 156 | - 💬 Comment (minor suggestions) |
| 157 | - 🔄 Request Changes (must address) |
| 158 | 4. Offer to pair if complex |
| 159 | ``` |
| 160 | |
| 161 | ## Review Techniques |
| 162 | |
| 163 | ### Technique 1: The Checklist Method |
| 164 | |
| 165 | ```markdown |
| 166 | ## Security Checklist |
| 167 | |
| 168 | - [ ] User input validated and sanitized |
| 169 | - [ ] SQL queries use parameterization |
| 170 | - [ ] Authentication/authorization checked |
| 171 | - [ ] Secrets not hardcoded |
| 172 | - [ ] Error messages don't leak info |
| 173 | |
| 174 | ## Performance Checklist |
| 175 | |
| 176 | - [ ] No N+1 queries |
| 177 | - [ ] Database queries indexed |
| 178 | - [ ] Large lists paginated |
| 179 | - [ ] Expensive operations cached |
| 180 | - [ ] No blocking I/O in hot paths |
| 181 | |
| 182 | ## Testing Checklist |
| 183 | |
| 184 | - [ ] Happy path tested |
| 185 | - [ ] Edge cases covered |
| 186 | - [ ] Error cases tested |
| 187 | - [ ] Test names are descriptive |
| 188 | - [ ] Tests are deterministic |
| 189 | ``` |
| 190 | |
| 191 | ### Technique 2: The Question Approach |
| 192 | |
| 193 | Instead of stating problems, ask questions to encourage thinking: |
| 194 | |
| 195 | ```markdown |
| 196 | ❌ "This will fail if the list is empty." |
| 197 | ✅ "What happens if `items` is an empty array?" |
| 198 | |
| 199 | ❌ "You need error handling here." |
| 200 | ✅ "How should this behave if the API call fails?" |
| 201 | |
| 202 | ❌ "This is inefficient." |
| 203 | ✅ "I see this loops through all users. Have we considered |
| 204 | the performance impact with 100k users?" |
| 205 | ``` |
| 206 | |
| 207 | ### Technique 3: Suggest, Don't Command |
| 208 | |
| 209 | ````markdown |
| 210 | ## Use Collaborative Language |
| 211 | |
| 212 | ❌ "You must change this to use async/await" |
| 213 | ✅ "Suggestion: async/await might make this more readable: |
| 214 | `typescript |
| 215 | async function fetchUser(id: string) { |
| 216 | const user = await db.query('SELECT * FROM users WHERE id = ?', id); |
| 217 | return user; |
| 218 | } |
| 219 | ` |
| 220 | What do you think?" |
| 221 | |
| 222 | ❌ "Extract this into a function" |
| 223 | ✅ "This logic appears in 3 places. Would it make sense to |
| 224 | extract it into a shared utility function?" |
| 225 | ```` |
| 226 | |
| 227 | ### Technique 4: Differentiate Severity |
| 228 | |
| 229 | ```markdown |
| 230 | Use labels to indicate priority: |
| 231 | |
| 232 | 🔴 [blocking] - Must fix before merge |
| 233 | 🟡 [important] - Should fix, discuss if disagree |
| 234 | 🟢 [nit] - Nice to have, not blocking |
| 235 | 💡 [suggestion] - Alternative approach to consider |
| 236 | 📚 [learning] - Educational comment, no action needed |
| 237 | 🎉 [praise] - Good work, keep it up! |
| 238 | |
| 239 | Example: |
| 240 | "🔴 [blocking] This SQL query is vulnerable to injection. |
| 241 | Please use parameterized queries." |
| 242 | |
| 243 | "🟢 [nit] Consider renaming `data` to `userData` for clarity." |
| 244 | |
| 245 | "🎉 [praise] Excellent test coverage! This will catch edge cases." |
| 246 | ``` |
| 247 | |
| 248 | ## Language-Specific Patterns |
| 249 | |
| 250 | ### Python Code Review |
| 251 | |
| 252 | ```python |
| 253 | # Check for Python-specific issues |
| 254 | |
| 255 | # ❌ Mutable default arguments |
| 256 | def add_item(item, items=[]): # Bug! Shared across calls |
| 257 | items.append(item) |
| 258 | return items |
| 259 | |
| 260 | # ✅ Use None as default |
| 261 | def add_item(item, items=None): |
| 262 | if items is None: |
| 263 | items = [] |
| 264 | items.append(item) |
| 265 | return items |
| 266 | |
| 267 | # ❌ Catching too broad |
| 268 | try: |
| 269 | result = risky_operation() |
| 270 | except: # Catches everything, even KeyboardInterrupt! |
| 271 | pass |
| 272 | |
| 273 | # ✅ Catch specific exceptions |
| 274 | try: |
| 275 | result = risky_operation() |
| 276 | except ValueError as e: |
| 277 | logger.error(f"Invalid value: {e}") |
| 278 | raise |
| 279 | |
| 280 | # ❌ Using mutable class attributes |
| 281 | class User: |
| 282 | permissions = [] # Shared across all instances! |
| 283 | |
| 284 | # ✅ Initialize in __init__ |
| 285 | class User: |
| 286 | def __init__(self): |
| 287 | self.permissions = [] |
| 288 | ``` |
| 289 | |
| 290 | ### TypeScript/JavaScript Code Review |
| 291 | |
| 292 | ```typescript |
| 293 | // Check for TypeScript-specific issues |
| 294 | |
| 295 | // ❌ Using any defeats type safety |
| 296 | function processData(data: any) { // Avoid any |
| 297 | return data.value; |
| 298 | } |
| 299 | |
| 300 | // ✅ Use proper types |
| 301 | interface DataPayload { |
| 302 | value: string; |
| 303 | } |
| 304 | function processData(data: DataPayload) { |
| 305 | return data.value; |
| 306 | } |
| 307 | |
| 308 | // ❌ Not handling async errors |
| 309 | async function fetchUser(id: string) { |
| 310 | const response = await fetch(`/api/users/${id}`);A4 — This skill pulls in web or user content but never says to treat that content as data. A signal, not proof. |
| 311 | return response.json(); // What if network fails? |
| 312 | } |
| 313 | |
| 314 | // ✅ Handle errors properly |
| 315 | async function fetchUser(id: string): Promise<User> { |
| 316 | try { |
| 317 | const response = await fetch(`/api/users/${id}`); |
| 318 | if (!response.ok) { |
| 319 | throw new Error(`HTTP ${response.status}`); |
| 320 | } |
| 321 | return await response.json(); |
| 322 | } catch (error) { |
| 323 | console.error('Failed to fetch user:', error); |
| 324 | throw error; |
| 325 | } |
| 326 | } |
| 327 | |
| 328 | // ❌ Mutation of props |
| 329 | function UserProfile({ user }: Props) { |
| 330 | user.lastViewed = new Date(); // Mutating prop! |
| 331 | return <div>{user.name}</div>; |
| 332 | } |
| 333 | |
| 334 | // ✅ Don't mutate props |
| 335 | function UserProfile({ user, onView }: Props) { |
| 336 | useEffect(() => { |
| 337 | onView(user.id); // Notify parent to update |
| 338 | }, [user.id]); |
| 339 | return <div>{user.name}</div>; |
| 340 | } |
| 341 | ``` |
| 342 | |
| 343 | ## Advanced Review Patterns |
| 344 | |
| 345 | ### Pattern 1: Architectural Review |
| 346 | |
| 347 | ```markdown |
| 348 | When reviewing significant changes: |
| 349 | |
| 350 | 1. **Design Document First** |
| 351 | - For large features, request design doc before code |
| 352 | - Review design with team before implementation |
| 353 | - Agree on approach to avoid rework |
| 354 | |
| 355 | 2. **Review in Stages** |
| 356 | - First PR: Core abstractions and interfaces |
| 357 | - Second PR: Implementation |
| 358 | - Third PR: Integration and tests |
| 359 | - Easier to review, faster to iterate |
| 360 | |
| 361 | 3. **Consider Alternatives** |
| 362 | - "Have we considered using [pattern/library]?" |
| 363 | - "What's the tradeoff vs. the simpler approach?" |
| 364 | - "How will this evolve as requirements change?" |
| 365 | ``` |
| 366 | |
| 367 | ### Pattern 2: Test Quality Review |
| 368 | |
| 369 | ```typescript |
| 370 | // ❌ Poor test: Implementation detail testing |
| 371 | test('increments counter variable', () => { |
| 372 | const component = render(<Counter />); |
| 373 | const button = component.getByRole('button'); |
| 374 | fireEvent.click(button); |
| 375 | expect(component.state.counter).toBe(1); // Testing internal state |
| 376 | }); |
| 377 | |
| 378 | // ✅ Good test: Behavior testing |
| 379 | test('displays incremented count when clicked', () => { |
| 380 | render(<Counter />); |
| 381 | const button = screen.getByRole('button', { name: /increment/i }); |
| 382 | fireEvent.click(button); |
| 383 | expect(screen.getByText('Count: 1')).toBeInTheDocument(); |
| 384 | }); |
| 385 | |
| 386 | // Review questions for tests: |
| 387 | // - Do tests describe behavior, not implementation? |
| 388 | // - Are test names clear and descriptive? |
| 389 | // - Do tests cover edge cases? |
| 390 | // - Are tests independent (no shared state)? |
| 391 | // - Can tests run in any order? |
| 392 | ``` |
| 393 | |
| 394 | ### Pattern 3: Security Review |
| 395 | |
| 396 | ```markdown |
| 397 | ## Security Review Checklist |
| 398 | |
| 399 | ### Authentication & Authorization |
| 400 | |
| 401 | - [ ] Is authentication required where needed? |
| 402 | - [ ] Are authorization checks before every action? |
| 403 | - [ ] Is JWT validation proper (signature, expiry)? |
| 404 | - [ ] Are API keys/secrets properly secured? |
| 405 | |
| 406 | ### Input Validation |
| 407 | |
| 408 | - [ ] All user inputs validated? |
| 409 | - [ ] File uploads restricted (size, type)? |
| 410 | - [ ] SQL queries parameterized? |
| 411 | - [ ] XSS protection (escape output)? |
| 412 | |
| 413 | ### Data Protection |
| 414 | |
| 415 | - [ ] Passwords hashed (bcrypt/argon2)? |
| 416 | - [ ] Sensitive data encrypted at rest? |
| 417 | - [ ] HTTPS enforced for sensitive data? |
| 418 | - [ ] PII handled according to regulations? |
| 419 | |
| 420 | ### Common Vulnerabilities |
| 421 | |
| 422 | - [ ] No eval() or similar dynamic execution? |
| 423 | - [ ] No hardcoded secrets? |
| 424 | - [ ] CSRF protection for state-changing operations? |
| 425 | - [ ] Rate limiting on public endpoints? |
| 426 | ``` |
| 427 | |
| 428 | ## Giving Difficult Feedback |
| 429 | |
| 430 | ### Pattern: The Sandwich Method (Modified) |
| 431 | |
| 432 | ```markdown |
| 433 | Traditional: Praise + Criticism + Praise (feels fake) |
| 434 | |
| 435 | Better: Context + Specific Issue + Helpful Solution |
| 436 | |
| 437 | Example: |
| 438 | "I noticed the payment processing logic is inline in the |
| 439 | controller. This makes it harder to test and reuse. |
| 440 | |
| 441 | [Specific Issue] |
| 442 | The calculateTotal() function mixes tax calculation, |
| 443 | discount logic, and database queries, making it difficult |
| 444 | to unit test and reason about. |
| 445 | |
| 446 | [Helpful Solution] |
| 447 | Could we extract this into a PaymentService class? That |
| 448 | would make it testable and reusable. I can pair with you |
| 449 | on this if helpful." |
| 450 | ``` |
| 451 | |
| 452 | ### Handling Disagreements |
| 453 | |
| 454 | ```markdown |
| 455 | When author disagrees with your feedback: |
| 456 | |
| 457 | 1. **Seek to Understand** |
| 458 | "Help me understand your approach. What led you to |
| 459 | choose this pattern?" |
| 460 | |
| 461 | 2. **Acknowledge Valid Points** |
| 462 | "That's a good point about X. I hadn't considered that." |
| 463 | |
| 464 | 3. **Provide Data** |
| 465 | "I'm concerned about performance. Can we add a benchmark |
| 466 | to validate the approach?" |
| 467 | |
| 468 | 4. **Escalate if Needed** |
| 469 | "Let's get [architect/senior dev] to weigh in on this." |
| 470 | |
| 471 | 5. **Know When to Let Go** |
| 472 | If it's working and not a critical issue, approve it. |
| 473 | Perfection is the enemy of progress. |
| 474 | ``` |
| 475 | |
| 476 | ## Best Practices |
| 477 | |
| 478 | 1. **Review Promptly**: Within 24 hours, ideally same day |
| 479 | 2. **Limit PR Size**: 200-400 lines max for effective review |
| 480 | 3. **Review in Time Blocks**: 60 minutes max, take breaks |
| 481 | 4. **Use Review Tools**: GitHub, GitLab, or dedicated tools |
| 482 | 5. **Automate What You Can**: Linters, formatters, security scans |
| 483 | 6. **Build Rapport**: Emoji, praise, and empathy matter |
| 484 | 7. **Be Available**: Offer to pair on complex issues |
| 485 | 8. **Learn from Others**: Review others' review comments |
| 486 | |
| 487 | ## Common Pitfalls |
| 488 | |
| 489 | - **Perfectionism**: Blocking PRs for minor style preferences |
| 490 | - **Scope Creep**: "While you're at it, can you also..." |
| 491 | - **Inconsistency**: Different standards for different people |
| 492 | - **Delayed Reviews**: Letting PRs sit for days |
| 493 | - **Ghosting**: Requesting changes then disappearing |
| 494 | - **Rubber Stamping**: Approving without actually reviewing |
| 495 | - **Bike Shedding**: Debating trivial details extensively |
| 496 | |
| 497 | ## Templates |
| 498 | |
| 499 | ### PR Review Comment Template |
| 500 | |
| 501 | ```markdown |
| 502 | ## Summary |
| 503 | |
| 504 | [Brief overview of what was reviewed] |
| 505 | |
| 506 | ## Strengths |
| 507 | |
| 508 | - [What was done well] |
| 509 | - [Good patterns or approaches] |
| 510 | |
| 511 | ## Required Changes |
| 512 | |
| 513 | 🔴 [Blocking issue 1] |
| 514 | 🔴 [Blocking issue 2] |
| 515 | |
| 516 | ## Suggestions |
| 517 | |
| 518 | 💡 [Improvement 1] |
| 519 | 💡 [Improvement 2] |
| 520 | |
| 521 | ## Questions |
| 522 | |
| 523 | ❓ [Clarification needed on X] |
| 524 | ❓ [Alternative approach consideration] |
| 525 | |
| 526 | ## Verdict |
| 527 | |
| 528 | ✅ Approve after addressing required changes |
| 529 | ``` |
| 530 |
Reviews
Installed this one?Write the first review and take the Trailblazer badge.
Alternatives
Subagent Driven DevelopmentUse when executing implementation plans with independent tasks in the current session◐◐◐◐◐●36/40Python Code Style & DocumentationPython code style, linting, formatting, naming conventions, and documentation standards. Use when writing new code, reviewing style, configuring linters, writing docstrings, or establishing project standards.◐····●35/40Competitor Price Analysis 💲Competitor pricing strategy analysis and market positioning. Price mapping, pricing gaps identification, elasticity signals evaluation, and strategic pricing optimization. Use when the user asks about competitor pricing, price analysis, pricing strategy, or co◐····●34/40Competitor Price Tracker 📊Set up competitor price tracking and monitoring workflows. Track price changes, detect promotions, analyze pricing patterns, and get alerts for competitive price movements.◐····●34/40