code-review-cpsource

Overview

Structured code review skill with a mandatory verification phase and a 20-section C/C++ master checklist. Covers 11+ languages. Every suspected bug must be cross-referenced against tests and callers before being reported.

Instructions

Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.

When to Use This Skill

• Reviewing pull requests and code changes
• Establishing code review standards for teams
• Mentoring junior developers through reviews
• Conducting architecture reviews
• Creating review checklists and guidelines
• Improving team collaboration
• Reducing code review cycle time
• Maintaining code quality standards

Core Principles

1. The Review Mindset

Goals of Code Review:

• Catch bugs and edge cases
• Ensure code maintainability
• Share knowledge across team
• Enforce coding standards
• Improve design and architecture
• Build team culture

Not the Goals:

• Show off knowledge
• Nitpick formatting (use linters)
• Block progress unnecessarily
• Rewrite to your preference

2. Effective Feedback

Good Feedback is:

• Specific and actionable
• Educational, not judgmental
• Focused on the code, not the person
• Balanced (praise good work too)
• Prioritized (critical vs nice-to-have)

❌ Bad: "This is wrong."
✅ Good: "This could cause a race condition when multiple users
         access simultaneously. Consider using a mutex here."

❌ Bad: "Why didn't you use X pattern?"
✅ Good: "Have you considered the Repository pattern? It would
         make this easier to test. Here's an example: [link]"

❌ Bad: "Rename this variable."
✅ Good: "[nit] Consider `userCount` instead of `uc` for
         clarity. Not blocking if you prefer to keep it."

3. Review Scope

What to Review:

• Logic correctness and edge cases
• Security vulnerabilities
• Performance implications
• Test coverage and quality
• Error handling
• Documentation and comments
• API design and naming
• Architectural fit

What Not to Review Manually:

• Code formatting (use Prettier, Black, etc.)
• Import organization
• Linting violations
• Simple typos

Review Process

Phase 1: Context Gathering (2-3 minutes)

Before diving into code, understand:

1. Read PR description and linked issue
2. Check PR size (>400 lines? Ask to split)
3. Review CI/CD status (tests passing?)
4. Understand the business requirement
5. Note any relevant architectural decisions

Phase 2: High-Level Review (5-10 minutes)

1. Architecture & Design - Does the solution fit the problem?
   • For significant changes, consult Architecture Review Guide
   • Check: SOLID principles, coupling/cohesion, anti-patterns
2. Performance Assessment - Are there performance concerns?
   • For performance-critical code, consult Performance Review Guide
   • Check: Algorithm complexity, N+1 queries, memory usage
3. File Organization - Are new files in the right places?
4. Testing Strategy - Are there tests covering edge cases?

Phase 3: Line-by-Line Review (10-20 minutes)

For each file, check:

• Logic & Correctness - Edge cases, off-by-one, null checks, race conditions
• Security - Input validation, injection risks, XSS, sensitive data
• Performance - N+1 queries, unnecessary loops, memory leaks
• Maintainability - Clear names, single responsibility, comments

Phase 3.5: Verify Before Reporting (CRITICAL)

Every suspected bug MUST be verified before it is reported. Static reading of code is not sufficient — you must cross-reference callers, tests, and runtime behavior to confirm a finding is real. Unverified findings damage review credibility and waste the author's time.

Verification steps for each finding:

1. Check the test suite. Search for tests that exercise the suspected buggy code path. If tests exist and assert specific behavior that contradicts your assumption, the code may be correct and your mental model wrong.

   # Find tests for the function you think is buggy
   grep -rn "functionName" tests/
   

2. Check callers. Search for all call sites of the function. How do real callers use the API? Do they pass the arguments you think are problematic? A function that looks wrong in isolation may be correct given its actual usage contract.

   grep -rn "functionName(" src/ --include="*.c"
   

3. Trace through edge cases manually. For tricky logic (pointer arithmetic, loop bounds, state machines), pick concrete input values and trace execution step by step. Write down the variable states. Do not rely on intuition for off-by-one or signed/unsigned boundary conditions.

4. Distinguish "looks wrong" from "is wrong." Common false positives:

   • Intentional sentinel values (e.g., -1 meaning "end of list")
   • Deliberately asymmetric push/pop semantics
   • Legacy API contracts that differ from what the name suggests
   • Code that is correct but poorly documented

5. If you cannot verify, say so. Label unverified findings explicitly:

   [unverified] This appears to allow negative indices, but I could not
   find tests or callers that exercise this path. Needs manual confirmation.
   

6. Check whether the finding was already reported. Before writing up a finding, search the project's existing PRs and issues for prior reports or fixes of the same problem. A finding already covered upstream should be cited (with link) rather than re-reported as new. This avoids duplicate noise and surfaces prior discussion, proposed patches, or reasons the issue was deferred.

   # GitHub CLI — search open and closed PRs/issues
   gh pr list --state all --search "<keyword or function name>"
   gh issue list --state all --search "<keyword or function name>"
   # Also grep the local tree for TODO/FIXME/XXX referencing the same area
   grep -rn -E "TODO|FIXME|XXX" <path>
   

   If a prior PR or issue exists, note it in the finding:

   Previously reported in #1234 (open) / fixed in #1250 — verify the
   current branch includes that fix or is tracking the open discussion.
   

What NOT to do:

• Do not report a function as buggy based solely on reading its implementation without checking how it is actually called
• Do not assume a doc comment is the specification — the test suite is the ground truth for intended behavior
• Do not flag intentional design patterns (sentinel values, walk-to-end loops) as bugs without first checking whether tests confirm the behavior

Phase 4: Summary & Decision (2-3 minutes)

1. Summarize key concerns
2. Highlight what you liked
3. Make clear decision:
   • ✅ Approve
   • 💬 Comment (minor suggestions)
   • 🔄 Request Changes (must address)
4. Offer to pair if complex

Review Techniques

Technique 1: The Checklist Method

Use checklists for consistent reviews. See Security Review Guide for comprehensive security checklist.

Technique 2: The Question Approach

Instead of stating problems, ask questions:

❌ "This will fail if the list is empty."
✅ "What happens if `items` is an empty array?"

❌ "You need error handling here."
✅ "How should this behave if the API call fails?"

Technique 3: Suggest, Don't Command

Use collaborative language:

❌ "You must change this to use async/await"
✅ "Suggestion: async/await might make this more readable. What do you think?"

❌ "Extract this into a function"
✅ "This logic appears in 3 places. Would it make sense to extract it?"

Technique 4: Differentiate Severity

Report findings in four buckets:

1. Critical — Memory corruption, auth bypass, data race with real impact, undefined behavior reachable in production, data loss. Must fix before merge.
2. **M