#+TITLE: Code Review Workflow #+AUTHOR: Craig Jennings & Claude #+DATE: 2026-01-23 * Overview This workflow guides code review with a focus on what matters most. It's based on the Code Review Pyramid (Gunnar Morling) which prioritizes review effort by cost of change - spend the most time on things that are hardest to fix later. #+begin_example ┌─────────────────┐ │ CODE STYLE │ ← Automate ┌───┴─────────────────┴───┐ │ TESTS │ ← Automate ┌───┴─────────────────────────┴───┐ │ DOCUMENTATION │ ┌───┴─────────────────────────────────┴───┐ │ IMPLEMENTATION SEMANTICS │ ← Focus ┌───┴─────────────────────────────────────────┴───┐ │ API SEMANTICS │ ← Most Focus └─────────────────────────────────────────────────┘ ↑ HARDER TO CHANGE LATER = MORE HUMAN REVIEW TIME #+end_example *Key Insight:* A formatting issue is a 2-minute fix. A breaking API change after release is a multi-sprint disaster. Prioritize accordingly. * When to Use This Workflow Use this workflow when: - Reviewing pull requests / merge requests - Doing self-review before submitting code - Pair reviewing with another developer * Effort Scaling Not all changes need the same scrutiny: | Change Type | Review Depth | Time | |-------------+--------------+------| | Typo/docs fix | Quick scan for correctness | 2-5 min | | Bug fix | Verify fix + no regressions | 10-15 min | | New feature | Full checklist, deep dive on logic | 30-60 min | | Architectural change | Bring in senior reviewers, verify design | 60+ min | * The Core Checklist Use this for every review. If you answer "no" or "maybe" to any item, dig deeper using the relevant Deep Dive section. ** 1. API Semantics (Hardest to Change - Focus Most Here) These questions matter most because changes here break things for other people. - [ ] *Simple?* Is the API as small as possible but as large as needed? - [ ] *Predictable?* Does it follow existing patterns and the principle of least surprise? - [ ] *Clean boundaries?* Is there clear separation between public API and internal implementation? - [ ] *Generally useful?* Is the API helpful for common cases, not just one specific scenario? - [ ] *Backwards compatible?* Are there any breaking changes to things users already rely on? ** 2. Implementation Semantics (Focus Here) Once the API looks good, check how the code actually works. - [ ] *Correct?* Does it actually do what the requirements/ticket says? - [ ] *Logically sound?* Are there edge cases, off-by-one errors, or logic bugs? - [ ] *Simple?* Could this be simpler? Is there unnecessary complexity or over-engineering? - [ ] *Robust?* Does it handle errors gracefully? Concurrency issues? - [ ] *Secure?* Is user input validated? No hardcoded secrets? (See Security Deep Dive) - [ ] *Performant?* Any expensive operations, N+1 queries, or scaling concerns? (See Performance Deep Dive) - [ ] *Observable?* Is there appropriate logging, metrics, or tracing? - [ ] *Dependencies OK?* Do new libraries pull their weight? License acceptable? ** 3. Documentation Good code still needs good explanations. - [ ] *New features documented?* README, API docs, or user guides updated? - [ ] *Code comments explain "why"?* Complex logic or non-obvious decisions explained? - [ ] *Readable?* Documentation is clear, accurate, no major errors? ** 4. Tests (Automate the "Pass/Fail" Check) Don't just check if tests pass - check if they're good tests. - [ ] *Tests exist?* Are new features and bug fixes covered by tests? - [ ] *Test the right things?* Do tests verify behavior, not implementation details? - [ ] *Edge cases covered?* Are boundary conditions and error paths tested? - [ ] *Right test type?* Unit tests where possible, integration tests where necessary? - [ ] *Would catch regressions?* If someone breaks this later, will a test fail? ** 5. Code Style (Automate This) Let linters and formatters handle these. Only check manually if tooling isn't set up. - [ ] *Linter passes?* No style violations flagged by automated tools? - [ ] *Readable?* Methods reasonable length? Clear naming? - [ ] *DRY?* No unnecessary duplication? ** 6. Process & Collaboration Code review is a conversation, not an inspection. - [ ] *PR has context?* Does the description explain WHY, not just what? - [ ] *Reasonable size?* Is this reviewable in one sitting? (<400 lines ideal) - [ ] *Self-reviewed?* Has the author clearly reviewed their own code first? - [ ] *No debug artifacts?* Console.logs, commented code, TODOs without tickets removed? * Deep Dives Use these when the Core Checklist flags a concern. Each section provides detailed checks for specific areas. ** Security Deep Dive When "Is it secure?" raises concerns, check these: *** Input Validation - [ ] All user input validated and sanitized - [ ] SQL queries parameterized (no string concatenation) - [ ] Output encoded to prevent XSS - [ ] File uploads validated (type, size, content) - [ ] Path traversal prevented *** Authentication & Authorization - [ ] Auth checks at every access point - [ ] Session management secure - [ ] CSRF protections for state-changing operations - [ ] Rate limiting where appropriate *** Secrets & Data - [ ] No hardcoded credentials (use env vars/secrets manager) - [ ] Sensitive data encrypted at rest and in transit - [ ] Error messages don't leak system information - [ ] Logs exclude sensitive data *** Common Vulnerabilities (OWASP) - [ ] No SQL injection vectors - [ ] No command injection vectors - [ ] No unsafe deserialization - [ ] Proper HTTP security headers ** Performance Deep Dive When "Is it performant?" raises concerns, check these: *** Database - [ ] No N+1 query problems - [ ] Appropriate indexes exist - [ ] No SELECT * - explicit columns only - [ ] Pagination for large result sets - [ ] Connection pooling used *** Code Efficiency - [ ] No unnecessary loops or iterations - [ ] Appropriate data structures (HashMap vs List lookups) - [ ] Expensive operations cached where beneficial - [ ] Large collections processed efficiently (streaming vs loading all) - [ ] Resources properly closed (connections, file handles) *** Scaling - [ ] Will this work with 10x/100x the data? - [ ] Async operations used where beneficial - [ ] No blocking operations in hot paths ** Concurrency Deep Dive When reviewing multi-threaded or async code: - [ ] Shared mutable state properly synchronized - [ ] No race conditions - [ ] Lazy initialization is thread-safe - [ ] Getters don't return mutable internal objects - [ ] Thread-safe collections used where needed - [ ] Consistent lock ordering (no deadlocks) - [ ] Loop variables passed into closures/lambdas (not captured) - [ ] Proper timeouts on blocking operations - [ ] Tests run with race detector where available ** Error Handling Deep Dive When reviewing error handling and logging: - [ ] Exceptions caught at appropriate levels - [ ] System "fails safe" (deny on error, not grant) - [ ] Transactions rolled back on failure - [ ] Error messages help debugging but don't leak info - [ ] Correlation IDs for tracing across services - [ ] Log levels appropriate (DEBUG/INFO/WARN/ERROR) - [ ] Critical failures trigger alerts ** API Compatibility Deep Dive When reviewing API changes: *** Non-Breaking (Safe) - Adding optional fields - Adding new endpoints - Making required fields optional - Adding new enum values (if clients handle unknown) *** Breaking (Dangerous) - Removing fields or endpoints - Changing field types - Making optional fields required - Changing semantic meaning of fields - Changing response status codes *** If Breaking Changes Required - [ ] Version bumped appropriately - [ ] Migration path documented - [ ] Deprecation warnings added - [ ] Transition period planned ** Dependencies Deep Dive When new dependencies are added: - [ ] Actually necessary (not just convenience) - [ ] From trusted, reputable source - [ ] Actively maintained (recent commits, responsive maintainers) - [ ] No known vulnerabilities (check Dependabot/Snyk) - [ ] License compatible with project - [ ] Doesn't bloat dependency tree excessively - [ ] Version pinned in lock file * How to Give Feedback ** Frame as Questions Bad: "This is wrong." Good: "I noticed we used X here - would Y be more efficient for this case?" ** Distinguish Severity - *Blocker:* Must fix before merge (security issue, breaking bug) - *Should fix:* Important but could be follow-up PR - *Suggestion:* Nice to have, author's discretion - *Nit:* Minor style preference, totally optional ** Call Out Good Stuff Reviews are for learning too. If you see a clever solution or clean pattern, say so. ** Be Timely Don't let PRs sit. If you're stuck, ask someone to pair-review with you. * Anti-Patterns to Avoid ** The Empty LGTM Never rubber-stamp with "Looks Good To Me" without actually reviewing. Even noting one thing you liked shows you read it. ** Nitpicking Don't comment on spaces, tabs, or semicolons. Let automated tools handle formatting. Focus on logic and design. ** The Stall Don't let PRs languish for days. Review within 24 hours or communicate delays. ** Bikeshedding Don't spend 30 minutes debating variable names while ignoring architectural concerns. ** Review by Line Count 400 lines of careful refactoring may need less scrutiny than 40 lines of new auth logic. Adjust effort to risk, not size. * Validation Checklist Before approving, verify: - [ ] Core checklist complete (all 5 pyramid levels) - [ ] Any "maybe" answers investigated with Deep Dives - [ ] Feedback framed constructively - [ ] Blockers clearly marked vs suggestions - [ ] You could explain this code to someone else * Sources - [[https://www.morling.dev/blog/the-code-review-pyramid/][The Code Review Pyramid]] - Gunnar Morling - [[https://google.github.io/eng-practices/review/][Google Engineering Practices]] - Code Review Guidelines - [[https://owasp.org/www-project-code-review-guide/][OWASP Code Review Guide]] - Security Checklist - [[https://github.com/code-review-checklists/java-concurrency][Java Concurrency Checklist]] - Thread Safety - [[https://go.dev/wiki/CodeReviewConcurrency][Go Concurrency Wiki]] - Race Conditions