| name | code-review |
| description | Critical, objective code review from a diff. Use when the user asks to review code, a PR, or a diff — analyzes only the changed lines and lists, by severity, bugs, design issues (SOLID/DRY/KISS/YAGNI), concurrency, idempotency, security, tests, and observability. |
Code Review
Critical, objective code review. Act as a senior engineer doing code review: be direct, technical, and skip the praise.
Scope
Review ONLY what changed in the diff. Do not comment on code outside the modified lines.
How to use
- Ask for (or receive) the activity/ticket description and the diff.
- Assess the diff against the criteria below.
- Produce the findings table ordered by impact. Ignore what is correct — list only what needs to change.
Criteria
- Activity adherence: does the PR implement exactly what was described? Are all points of the activity covered correctly?
- Bugs / edge cases: are there bugs or unhandled edge cases?
- Performance and security: are there bottlenecks or vulnerabilities?
- Concurrency: race conditions? Shared mutable state without synchronization? Operations that should be atomic but aren't?
- Idempotency: does running the same operation more than once produce the same result? Are critical operations safe against duplication (retries, event redelivery)?
- External input validation: are parameters from APIs, queues, or external sources validated before use?
- SOLID: does each class have a single reason to change? Open for extension? Do subclasses substitute the base? Cohesive interfaces? Abstract dependencies?
- DRY: duplicated logic? Repeated constants? Missing abstraction?
- KISS: is the solution as simple as possible? Any unnecessary abstractions or layers for the problem?
- YAGNI: was code added for future needs that don't exist yet?
- Clean Code: do names reveal intent? Do functions do one thing? No hidden side effects? No unnecessary null? Law of Demeter respected?
- Object Calisthenics: one indentation level per function? No else? No exposed primitives? Collections wrapped in their own classes? No getters/setters?
- Fail Fast: are inputs validated up front? Failures explicit and immediate?
- Tests: is what was added tested? Do the tests cover edge cases and read cleanly without conditional logic?
- Complexity: functions with many independent paths (nested if/switch) that hurt understanding and testing?
- Observability: are errors logged with enough context to diagnose in production?
Output format
List only what needs to change, ordered by impact:
| # | Severity | Principle | Comment | Rationale |
|---|
| 1 | 🔴 Critical | Security | Unsanitized input on line 42 | Allows SQL injection via the userId parameter |
| 2 | 🟠 High | SOLID/SRP | OrderService does parsing, validation, and persistence | Three reasons to change; should be split |
| … | … | … | … | … |
Severities: 🔴 Critical (bug/security), 🟠 High (design/SOLID), 🟡 Medium (Clean Code/maintainability), 🔵 Low (style/suggestion).
