engineering

Code Review Checklist

Systematic code review checklist covering correctness, security, performance, maintainability, and testing — with severity-based prioritization and actionable feedback templates.

code-reviewchecklistsecurityqualitypull-requests

Works well with agents

API Developer Agent Frontend Engineer Agent

Works well with skills

Bug Report Writing Open Source Contributing Guide Ticket Writing

$ npx skills add The-AI-Directory-Company/(…) --skill code-review-checklist

code-review-checklist/

SKILL.md

Markdown

1
2	# Code Review Checklist
3
4	## Before you start
5
6	Gather the following from the reviewer or PR author. If anything is missing, ask before proceeding:
7
8	1. What does the PR do? — The specific change, not "fixes stuff"
9	2. Why is it needed? — Link to ticket, bug report, or requirement
10	3. What is the blast radius? — Which systems, APIs, or users are affected
11	4. Are there tests? — New, modified, or explicitly skipped (with justification)
12	5. Is there a deploy plan? — Feature flag, migration, rollback strategy
13
14	## Review template
15
16	### 1. First Pass: Understand Intent
17
18	Read the PR description and linked ticket before touching code. Answer:
19
20	- What is the expected behavior change?
21	- What should NOT change?
22	- Are there edge cases called out by the author?
23
24	If the PR description is empty or vague, stop and request one. Reviewing code without context produces shallow feedback.
25
26	### 2. Correctness
27
28	Check each item:
29
30	- [ ] Logic matches the stated requirement — not just "runs without errors"
31	- [ ] Edge cases are handled: empty inputs, null values, boundary conditions, concurrent access
32	- [ ] Error paths return meaningful messages, not silent failures or generic 500s
33	- [ ] State mutations are atomic where required — no partial updates on failure
34	- [ ] Data types are correct: string vs number, nullable vs required, timezone-aware dates
35
36	### 3. Security
37
38	- [ ] User input is validated and sanitized before use
39	- [ ] Authentication and authorization checks are present on new endpoints
40	- [ ] Secrets are not hardcoded or logged — check for API keys, tokens, passwords
41	- [ ] SQL queries use parameterized statements, not string concatenation
42	- [ ] File uploads validate type, size, and content — not just extension
43	- [ ] CORS, CSP, and rate limiting are configured for new routes
44
45	### 4. Performance
46
47	- [ ] Database queries use indexes — check for full table scans on large tables
48	- [ ] N+1 queries are eliminated — look for loops that issue individual queries
49	- [ ] Large datasets are paginated, not loaded entirely into memory
50	- [ ] Expensive computations are cached or debounced where appropriate
51	- [ ] New API endpoints have timeout and retry policies defined
52
53	### 5. Maintainability
54
55	- [ ] Names describe intent: `calculateTotalPrice` not `doStuff` or `handleData`
56	- [ ] Functions do one thing — if a function has "and" in its description, split it
57	- [ ] Magic numbers and strings are extracted to named constants
58	- [ ] Complex logic has comments explaining WHY, not WHAT
59	- [ ] Dead code, commented-out blocks, and TODO-without-tickets are removed
60
61	### 6. Testing
62
63	- [ ] Happy path is covered with assertions on expected output
64	- [ ] Failure modes are tested: invalid input, network errors, timeout, auth failures
65	- [ ] Tests are deterministic — no reliance on wall clock, random data, or external services
66	- [ ] Test names describe the scenario and expected outcome
67	- [ ] Mocks are minimal — over-mocking hides integration bugs
68
69	### 7. Compatibility and Deployment
70
71	- [ ] Database migrations are backward-compatible (no column drops without migration plan)
72	- [ ] API changes are versioned or backward-compatible
73	- [ ] Feature flags gate incomplete or risky functionality
74	- [ ] Environment-specific configuration is externalized, not hardcoded
75
76	## Severity classification for findings
77
78	Classify every finding. Mixed-severity comments without labels waste the author's time.
79
80	\| Severity \| Meaning \| Action required \|
81	\|----------\|---------\|----------------\|
82	\| Blocker \| Bug, security flaw, or data loss risk \| Must fix before merge \|
83	\| Major \| Performance issue, missing validation, poor error handling \| Should fix before merge \|
84	\| Minor \| Style, naming, minor readability improvement \| Fix or acknowledge \|
85	\| Nit \| Preference, optional suggestion \| Author decides \|
86
87	## Feedback templates
88
89	Use these formats to keep comments actionable:
90
91	Blocker: "This query has no auth check. An unauthenticated user can access other users' data via `GET /api/users/:id`. Add the `requireAuth` middleware before this handler."
92
93	Major: "This loop issues one query per item (N+1). For 500 items, that is 500 queries. Consider using `WHERE id IN (...)` with a single batch query."
94
95	Minor: "`processData` does not describe what this function does. Consider `validateAndNormalizeAddress` to match the actual behavior."
96
97	Nit: "Personal preference: I would extract this ternary into a named variable for readability. Up to you."
98
99	## Quality checklist
100
101	Before submitting your review, verify:
102
103	- [ ] Every comment has a severity label (Blocker, Major, Minor, Nit)
104	- [ ] Blockers include a specific fix suggestion, not just "this is wrong"
105	- [ ] You reviewed the test changes, not just the implementation
106	- [ ] You checked for what is MISSING, not just what is present
107	- [ ] You verified the PR does not introduce regressions in existing behavior
108	- [ ] Your feedback is directed at the code, not the person
109
110	## Common mistakes
111
112	- Reviewing style instead of substance. Nitpicking formatting while missing a SQL injection is a failed review. Check correctness and security first, style last.
113	- Approving without understanding. If you cannot explain what the PR does, you cannot verify it is correct. Ask questions instead of rubber-stamping.
114	- Giving vague feedback. "This could be better" is not actionable. State what is wrong, why it matters, and how to fix it.
115	- Ignoring the test diff. Tests that do not assert meaningful behavior give false confidence. Verify test quality, not just test existence.
116	- Blocking on nits. Holding up a PR for variable naming while a production bug waits is poor prioritization. Approve with nits when blockers are absent.
117	- Skipping the "what is missing" check. The most dangerous bugs are in the code that was never written: missing validation, missing error handling, missing auth checks.
118