| name | code-review |
| description | Expert code review providing thorough, constructive, and actionable feedback covering correctness, architecture, security, and project-specific invariants. |
| validate | scripts/validate.py |
| skill_type | universal |
| version | 1.0.0 |
Code Review
Thorough, constructive, and actionable code review that covers generic quality concerns, exception safety, correctness, and security invariants.
Review Feedback Format
For each issue found, provide:
- Severity: 🔴 Critical | 🟠 Important | 🟡 Suggestion | 💡 Nitpick
- Location: File and line number (or function name)
- Issue: Clear description of the problem
- Rule: The architectural rule or principle violated
- Fix: Concrete suggestion or code snippet
Review Tone
- Constructive, not critical — explain WHY something must change
- Acknowledge good patterns when you see them
- Ask clarifying questions when intent is unclear
- Focus on the code, not the person
Pass 1 — Context & Purpose
Before raising any issue:
- What is the purpose of this change? (Feature / Bug fix / Refactor / Performance)
- Does it solve the stated problem?
- Are there acceptance criteria or a linked spec?
Pass 2 — Architecture & Layer Violations 🔴
These are hard fails — merge must be blocked until resolved.
- Dependency Direction: Internal/domain layers must NEVER import from outer/infrastructure/presentation layers. Entities must be persistence-ignorant (no database ORM imports inside domain layers).
- Router/API Layer Discipline: Routers/handlers must only route. Flag any route handler that contains business logic (validation beyond schema, state computation) or direct database access.
- Unit of Work Contract: Services must depend on a Unit of Work or transaction manager interface—never raw database connections or sessions directly. Repositories must never commit transactions.
- DTO / Boundary: Service boundaries must use Data Transfer Objects (DTOs) or schemas, not raw database entity model instances.
Pass 3 — Exception Handling 🔴
Hard fail: Catching generic exceptions without an immediate raise or wrapping in a new exception.
- Catch the most specific exception possible. Swallowing errors masks real failures.
- Infrastructure failures must be re-wrapped as domain exceptions before propagating.
- Never use
pass inside an except/catch block without explicit documentation.
Pass 4 — Correctness & Logic 🟠
- Architectural technical debt check (AI hallucination risk): AI-generated code is frequently locally correct but globally inconsistent. Confirm:
- Does the new code follow the same patterns as adjacent code in the same layer?
- Does it introduce a new pattern that duplicates existing infrastructure?
- Is the naming consistent with the codebase conventions?
- API hallucination check (AI hallucination risk): AI models generate calls to methods, functions, and class attributes that do not exist. Verify:
- The method/function actually exists on the object/module being called.
- Internal methods being called exist with the exact signature used.
- Latent race condition check (AI hallucination risk): AI-generated code frequently introduces non-deterministic bugs in concurrent contexts. Flag any sequence where a value is read, a decision is made based on it, and a write occurs without an atomic lock or transaction boundary between read and write (check-then-act).
Pass 5 — Security 🔴
- Implicit security vulnerability check (AI hallucination risk): AI models embed known-vulnerable patterns. Check for:
- Unparameterised query fragments (f-string or
.format() used in database queries).
- Auth token handling: tokens passed in URLs, stored insecurely, or logged in plaintext.
- Unsafe deserialisation: unsafely loading user-supplied or external strings.
- JWT algorithm
none acceptance or missing signature verification.
- Least Privilege: Only grant the permissions necessary for the task.
- Secrets Protection: No secrets, API keys, or credentials in code, logs, or error messages.
Pass 6 — Performance 🟡
- N+1 queries: Loops calling database or repository methods per iteration without eager loading.
- Missing indexes: New database tables or query columns lacking proper index declarations.
- Unbounded queries: Ensure list endpoints paginate and restrict max result sizes.
Pass 7 — Testing 🟠
- TDD Law: New public functions and service methods must have at least one test.
- Test Integrity: Assert correct outcomes and state changes (e.g. transactions committed).
- Test Boundaries: Use unit tests for services (mocking external dependencies) and integration tests for end-to-end flows.
Pass 8 — Documentation & Hygiene 💡
- Public APIs: Document inputs, outputs, and exceptions raised.
- No commented-out code or stale debug print statements in production.
- No
# TODO without a linked issue number or tracking reference.
Quick Invariant Checklist
Before approving any PR, confirm:
