code-review-and-quality

Name: code-review-and-quality
Author: addyosmani

byaddyosmani· 31 skills

Installs

20k

Stars

80k

Forks

8.7k

How to install code-review-and-quality?

addyosmani/agent-skills/code-review-and-quality

$npx -y skills add addyosmani/agent-skills --skill code-review-and-quality

Installs into the current project.

›Prefer a prompt? Paste this to your agent

Use this skill

Run `npx skills use "https://github.com/addyosmani/agent-skills" --skill "addyosmani/agent-skills/code-review-and-quality"` and follow the generated skill instructions now. Read its complete output, redirecting it to a temporary file first if necessary. Resolve relative paths from the supporting-files directory it provides.

Use the whole pack

Use the skills in "https://github.com/addyosmani/agent-skills" that are relevant to the current task. Run `npx skills add "https://github.com/addyosmani/agent-skills"` and select the relevant skills, then follow their instructions.

Files · 1

View on GitHub

SKILL.md

1	# Code Review and Quality
2
3	## Overview
4
5	Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance.
6
7	The approval standard: Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement. Don't block a change because it isn't exactly how you would have written it. If it improves the codebase and follows the project's conventions, approve it.
8
9	## When to Use
10
11	- Before merging any PR or change
12	- After completing a feature implementation
13	- When another agent or model produced code you need to evaluate
14	- When refactoring existing code
15	- After any bug fix (review both the fix and the regression test)
16
17	## The Five-Axis Review
18
19	Every review evaluates code across these dimensions:
20
21	### 1. Correctness
22
23	Does the code do what it claims to do?
24
25	- Does it match the spec or task requirements?
26	- Are edge cases handled (null, empty, boundary values)?
27	- Are error paths handled (not just the happy path)?
28	- Does it pass all tests? Are the tests actually testing the right things?
29	- Are there off-by-one errors, race conditions, or state inconsistencies?
30
31	### 2. Readability & Simplicity
32
33	Can another engineer (or agent) understand this code without the author explaining it?
34
35	- Are names descriptive and consistent with project conventions? (No `temp`, `data`, `result` without context)
36	- Is the control flow straightforward (avoid nested ternaries, deep callbacks)?
37	- Is the code organized logically (related code grouped, clear module boundaries)?
38	- Are there any "clever" tricks that should be simplified?
39	- Could this be done in fewer lines? (1000 lines where 100 suffice is a failure)
40	- Are abstractions earning their complexity? (Don't generalize until the third use case)
41	- Would comments help clarify non-obvious intent? (But don't comment obvious code.)
42	- Are there dead code artifacts: no-op variables (`_unused`), backwards-compat shims, or `// removed` comments?
43	- Is a new conditional bolted onto an unrelated flow? That's a design smell, not a nit — push the logic into its own helper, state, or policy instead of tangling an existing path.
44	- Do repeated conditionals on the same shape appear? They signal a missing model or dispatcher. A "temporary" branch is usually permanent debt.
45
46	### 3. Architecture
47
48	Does the change fit the system's design?
49
50	- Does it follow existing patterns or introduce a new one? If new, is it justified?
51	- Does it maintain clean module boundaries?
52	- Is there code duplication that should be shared?
53	- Are dependencies flowing in the right direction (no circular dependencies)?
54	- Is the abstraction level appropriate (not over-engineered, not too coupled)?
55	- Does this refactor reduce complexity or just relocate it? Count the concepts a reader must hold to follow the change. If a "cleaner" version leaves that count unchanged, it isn't cleaner — prefer the restructuring that makes whole branches, modes, or layers disappear over one that re-centralizes the same logic. Prefer deleting an abstraction to polishing it.
56	- Is feature-specific logic leaking into a shared or general-purpose module? Keep logic in its owning layer, reuse the existing canonical helper instead of a near-duplicate, and don't normalize architectural drift.
57	- Are type boundaries explicit? Question gratuitous `any`/`unknown`/optional/casts and silent fallbacks that paper over an unclear invariant — making the boundary explicit often makes the surrounding control flow simpler.
58
59	### 4. Security
60
61	For detailed security guidance, see `security-and-hardening`. Does the change introduce vulnerabilities?
62
63	- Is user input validated and sanitized?
64	- Are secrets kept out of code, logs, and version control?
65	- Is authentication/authorization checked where needed?
66	- Are SQL queries parameterized (no string concatenation)?
67	- Are outputs encoded to prevent XSS?
68	- Are dependencies from trusted sources with no known vulnerabilities?
69	- Is data from external sources (APIs, logs, user content, config files) treated as untrusted?
70	- Are external data flows validated at system boundaries before use in logic or rendering?
71
72	### 5. Performance
73
74	For detailed profiling and optimization, see `performance-optimization`. Does the change introduce performance problems?
75
76	- Any N+1 query patterns?
77	- Any unbounded loops or unconstrained data fetching?
78	- Any synchronous operations that should be async?
79	- Any unnecessary re-renders in UI components?
80	- Any missing pagination on list endpoints?
81	- Any large objects created in hot paths?
82
83	## Structural Remedies
84
85	When you flag a structural problem, propose the move — not just the problem. A review that only says "this is complex" leaves the author guessing. Reach for a named restructuring:
86
87	- Replace a chain of conditionals with a typed model or an explicit dispatcher.
88	- Collapse duplicate branches into a single clearer flow.
89	- Separate orchestration from business logic so each reads on its own.
90	- Move feature-specific logic out of a shared module into the package that owns the concept.
91	- Reuse the canonical helper instead of a bespoke near-duplicate.
92	- Make a type boundary explicit so downstream branching disappears.
93	- Delete a pass-through wrapper that adds indirection without clarifying the API.
94	- Extract a helper, or split a large file into focused modules.
95
96	Prefer the remedy that removes moving pieces over one that spreads the same complexity around.
97
98	## Change Sizing
99
100	Small, focused changes are easier to review, faster to merge, and safer to deploy. Target these sizes:
101
102	```
103	~100 lines changed → Good. Reviewable in one sitting.
104	~300 lines changed → Acceptable if it's a single logical change.
105	~1000 lines changed → Too large. Split it.
106	```
107
108	Watch file size, not just diff size. A small diff can still push a file past a healthy boundary — around 1000 total lines in a single file (distinct from the ~1000 changed-lines threshold above) is a common inspection signal, not a hard cap. When a change materially grows an already-large file, ask whether to extract helpers, subcomponents, or modules first, before piling more on. Decompose, then add.
109
110	What counts as "one change": A single self-contained modification that addresses one thing, includes related tests, and keeps the system functional after submission. One part of a feature — not the whole feature.
111
112	Splitting strategies when a change is too large:
113
114	\| Strategy \| How \| When \|
115	\|----------\|-----\|------\|
116	\| Stack \| Submit a small change, start the next one based on it \| Sequential dependencies \|
117	\| By file group \| Separate changes for groups needing different reviewers \| Cross-cutting concerns \|
118	\| Horizontal \| Create shared code/stubs first, then consumers \| Layered architecture \|
119	\| Vertical \| Break into smaller full-stack slices of the feature \| Feature work \|
120
121	When large changes are acceptable: Complete file deletions and automated refactoring where the reviewer only needs to verify intent, not every line.
122
123	Separate refactoring from feature work. A change that refactors existing code and adds new behavior is two changes — submit them separately. Small cleanups (variable renaming) can be included at reviewer discretion.
124
125	## Change Descriptions
126
127	Every change needs a description that stands alone in version control history.
128
129	First line: Short, imperative, standalone. "Delete the FizzBuzz RPC" not "Deleting the FizzBuzz RPC." Must be informative enough that someone searching history can understand the change without reading the diff.
130
131	Body: What is changing and why. Include context, decisions, and reasoning not visible in the code itself. Link to bug numbers, benchmark results, or design docs where relevant. Acknowledge approach shortcomings when they exist.
132
133	Anti-patterns: "Fix bug," "Fix build," "Add patch," "Moving code from A to B," "Phase 1," "Add convenience functions."
134
135	## Review Process
136
137	### Step 1: Understand the Context
138
139	Before looking at code, understand the intent:
140
141	```
142	- What is this change trying to accomplish?
143	- What spec or task does it implement?
144	- What is the expected behavior change?
145	```
146
147	### Step 2: Review the Tests First
148
149	Tests reveal intent and coverage:
150
151	```
152	- Do tests exist for the change?
153	- Do they test behavior (not implementation details)?
154	- Are edge cases covered?
155	- Do tests have descriptive names?
156	- Would the tests catch a regression if the code changed?
157	```
158
159	### Step 3: Review the Implementation
160
161	Walk through the code with the five axes in mind:
162
163	```
164	For each file changed:
165	1. Correctness: Does this code do what the test says it should?
166	2. Readability: Can I understand this without help?
167	3. Architecture: Does this fit the system?
168	4. Security: Any vulnerabilities?
169	5. Performance: Any bottlenecks?
170	```
171
172	### Step 4: Categorize Findings
173
174	Label every comment with its severity so the author knows what's required vs optional:
175
176	\| Prefix \| Meaning \| Author Action \|
177	\|--------\|---------\|---------------\|
178	\| (no prefix) \| Required change \| Must address before merge \|
179	\| Critical: \| Blocks merge \| Security vulnerability, data loss, broken functionality \|
180	\| Nit: \| Minor, optional \| Author may ignore — formatting, style preferences \|
181	\| Optional: / Consider: \| Suggestion \| Worth considering but not required \|
182	\| FYI \| Informational only \| No action needed — context for future reference \|
183
184	This prevents authors from treating all feedback as mandatory and wasting time on optional suggestions.
185
186	Lead with what matters. Order findings by leverage: correctness and security first, then structural regressions and missed simplifications, then everything else. Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem is the review.
187
188	### Step 5: Verify the Verification
189
190	Check the author's verification story:
191
192	```
193	- What tests were run?
194	- Did the build pass?
195	- Was the change tested manually?
196	- Are there screenshots for UI changes?
197	- Is there a before/after comparison?
198	```
199
200	## Multi-Model Review Pattern
201
202	Use different models for different review perspectives:
203
204	```
205	Model A writes the code
206	│
207	▼
208	Model B reviews for correctness and architecture
209	│
210	▼
211	Model A addresses the feedback
212	│
213	▼
214	Human makes the final call
215	```
216
217	This catches issues that a single model might miss — different models have different blind spots.
218
219	Example prompt for a review agent:
220	```
221	Review this code change for correctness, security, and adherence to
222	our project conventions. The spec says [X]. The change should [Y].
223	Flag any issues as Critical, Required, Optional, or Nit.
224	```
225
226	## Dead Code Hygiene
227
228	After any refactoring or implementation change, check for orphaned code:
229
230	1. Identify code that is now unreachable or unused
231	2. List it explicitly
232	3. Ask before deleting: "Should I remove these now-unused elements: [list]?"
233
234	Don't leave dead code lying around — it confuses future readers and agents. But don't silently delete things you're not sure about. When in doubt, ask.
235
236	```
237	DEAD CODE IDENTIFIED:
238	- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
239	- OldTaskCard component in src/components/ — replaced by TaskCard
240	- LEGACY_API_URL constant in src/config.ts — no remaining references
241	→ Safe to remove these?
242	```
243
244	## Review Speed
245
246	Slow reviews block entire teams. The cost of context-switching to review is less than the waiting cost imposed on others.
247
248	- Respond within one business day — this is the maximum, not the target
249	- Ideal cadence: Respond shortly after a review request arrives, unless deep in focused coding. A typical change should complete multiple review rounds in a single day
250	- Prioritize fast individual responses over quick final approval. Quick feedback reduces frustration even if multiple rounds are needed
251	- Large changes: Ask the author to split them rather than reviewing one massive changeset
252
253	## Handling Disagreements
254
255	When resolving review disputes, apply this hierarchy:
256
257	1. Technical facts and data override opinions and preferences
258	2. Style guides are the absolute authority on style matters
259	3. Software design must be evaluated on engineering principles, not personal preference
260	4. Codebase consistency is acceptable if it doesn't degrade overall health
261
262	Don't accept "I'll clean it up later." Experience shows deferred cleanup rarely happens. Require cleanup before submission unless it's a genuine emergency. If surrounding issues can't be addressed in this change, require filing a bug with self-assignment.
263
264	## Honesty in Review
265
266	When reviewing code — whether written by you, another agent, or a human:
267
268	- Don't rubber-stamp. "LGTM" without evidence of review helps no one.
269	- Don't soften real issues. "This might be a minor concern" when it's a bug that will hit production is dishonest.
270	- Quantify problems when possible. "This N+1 query will add ~50ms per item in the list" is better than "this could be slow."
271	- Push back on approaches with clear problems. Sycophancy is a failure mode in reviews. If the implementation has issues, say so directly and propose alternatives.
272	- Accept override gracefully. If the author has full context and disagrees, defer to their judgment. Comment on code, not people — reframe personal critiques to focus on the code itself.
273
274	## Dependency Discipline
275
276	Part of code review is dependency review:
277
278	Before adding any dependency:
279	1. Does the existing stack solve this? (Often it does.)
280	2. How large is the dependency? (Check bundle impact.)
281	3. Is it actively maintained? (Check last commit, open issues.)
282	4. Does it have known vulnerabilities? (`npm audit`)
283	5. What's the license? (Must be compatible with the project.)
284
285	Rule: Prefer standard library and existing utilities over new dependencies. Every dependency is a liability.
286
287	Upgrading an existing dependency is a code change like any other, and the riskiest upgrades are the ones merged in bulk with a message like "bump deps." Review them with the same discipline:
288
289	1. Read the changelog, not just the version number. Semver is a promise the maintainer may not have kept — a "patch" can carry a behavioral change. For a major bump, read the migration notes and find what breaks.
290	2. One dependency per change. Upgrade and merge them individually (or in small related groups). When a bulk bump breaks the build, you've lost which package did it; a single-package change makes the cause obvious and the revert clean.
291	3. Let the tests decide. The upgrade is verified by a green suite before and after, not by "it installed." If coverage around the dependency's behavior is thin, that gap is the real finding — add a test first.
292	4. Mind the transitive graph. Most installed packages are ones nobody chose directly. Review the lockfile diff, not just `package.json`; a single direct bump can pull in dozens of indirect changes.
293	5. Keep the lockfile honest. Commit it, review its diff, and never hand-edit it. The lockfile is the thing that actually pins what ships.
294
295	For triaging `npm audit` findings and supply-chain risk (typosquatting, compromised maintainers), follow the `security-and-hardening` skill — this section covers the upgrade workflow, that one covers the security verdict.
296
297	## The Review Checklist
298
299	```markdown
300	## Review: [PR/Change title]
301
302	### Context
303	- [ ] I understand what this change does and why
304
305	### Correctness
306	- [ ] Change matches spec/task requirements
307	- [ ] Edge cases handled
308	- [ ] Error paths handled
309	- [ ] Tests cover the change adequately
310
311	### Readability
312	- [ ] Names are clear and consistent
313	- [ ] Logic is straightforward
314	- [ ] No unnecessary complexity
315
316	### Architecture
317	- [ ] Follows existing patterns
318	- [ ] No unnecessary coupling or dependencies
319	- [ ] Appropriate abstraction level
320	- [ ] Refactors reduce complexity rather than relocate it
321	- [ ] No feature logic in shared modules; file stays within a healthy size
322
323	### Security
324	- [ ] No secrets in code
325	- [ ] Input validated at boundaries
326	- [ ] No injection vulnerabilities
327	- [ ] Auth checks in place
328	- [ ] External data sources treated as untrusted
329
330	### Performance
331	- [ ] No N+1 patterns
332	- [ ] No unbounded operations
333	- [ ] Pagination on list endpoints
334
335	### Verification
336	- [ ] Tests pass
337	- [ ] Build succeeds
338	- [ ] Manual verification done (if applicable)
339
340	### Verdict
341	- [ ] Approve — Ready to merge
342	- [ ] Request changes — Issues must be addressed
343	```
344	## See Also
345
346	- For detailed security review guidance, see `references/security-checklist.md`
347	- For performance review checks, see `references/performance-checklist.md`
348
349	## Common Rationalizations
350
351	\| Rationalization \| Reality \|
352	\|---\|---\|
353	\| "It works, that's good enough" \| Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds. \|
354	\| "I wrote it, so I know it's correct" \| Authors are blind to their own assumptions. Every change benefits from another set of eyes. \|
355	\| "We'll clean it up later" \| Later never comes. The review is the quality gate — use it. Require cleanup before merge, not after. \|
356	\| "AI-generated code is probably fine" \| AI code needs more scrutiny, not less. It's confident and plausible, even when wrong. \|
357	\| "The tests pass, so it's good" \| Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns. \|
358	\| "The refactor makes it cleaner" \| Relocating complexity isn't reducing it. If the reader still holds the same number of concepts, the structure didn't improve — look for the version where branches disappear. \|
359	\| "It's only a small addition to this file" \| Small diffs still push files past a healthy size and bolt branches onto unrelated flows. Judge the resulting structure, not the diff size. \|
360	\| "It's just a version bump" \| A bump is a behavior change you didn't write. Read the changelog; semver doesn't guarantee no breakage. \|
361	\| "I'll upgrade everything in one PR to save time" \| A bulk bump that breaks the build hides which package did it. One dependency per change keeps the cause and the revert clean. \|
362
363	## Red Flags
364
365	- PRs merged without any review
366	- Review that only checks if tests pass (ignoring other axes)
367	- "LGTM" without evidence of actual review
368	- Security-sensitive changes without security-focused review
369	- Large PRs that are "too big to review properly" (split them)
370	- No regression tests with bug fix PRs
371	- Review comments without severity labels — makes it unclear what's required vs optional
372	- Accepting "I'll fix it later" — it never happens
373	- A refactor that moves code around without reducing the number of concepts a reader must hold
374	- A change that grows an already-large file instead of decomposing it
375	- New conditionals scattered into unrelated code paths (a missing abstraction)
376	- A bespoke helper that duplicates an existing canonical one, or feature logic placed in a shared module
377	- A bulk "bump dependencies" PR with no changelog review and no per-package isolation
378	- A lockfile change that's hand-edited, uncommitted, or merged without reviewing its diff
379
380	## Verification
381
382	After review is complete:
383
384	- [ ] All Critical issues are resolved
385	- [ ] All Required (no-prefix) changes are resolved or explicitly deferred with justification
386	- [ ] Tests pass
387	- [ ] Build succeeds
388	- [ ] The verification story is documented (what changed, how it was verified)
389	- [ ] Dependency upgrades were reviewed against their changelog, isolated per package, and verified by a green suite wi

6 picks

Type

Run `npx skills use "https://github.com/addyosmani/agent-skills" --skill "addyosmani/agent-skills/code-review-and-quality"` and follow the generated skill instructions now. Read its complete output, redirecting it to a temporary file first if necessary. Resolve relative paths from the supporting-files directory it provides.

Use the skills in "https://github.com/addyosmani/agent-skills" that are relevant to the current task. Run `npx skills add "https://github.com/addyosmani/agent-skills"` and select the relevant skills, then follow their instructions.

code-review-and-quality

How to install code-review-and-quality?

Related

code-review-and-quality

How to install code-review-and-quality?

Related