Publish Sign in

Code Review and Quality

Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human…

You say
Buy it · $45 Read it before you buy $45 Written by addyosmani · unverified publisher
Context cost
5.2k tokensestimated from the bundle, loaded when it triggers
Bundle
1 file · 20.6 kBtext throughout, nothing executable
Licence
MITpaid listing
Last change
—no release on file
Servers it uses
Noneruns standalone

What it does

Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human. Use when you need to assess code quality across multiple dimensions before it enters the main branch.

Installed, it changes the agent in these ways.

What this skill changes about the agent is not written down here yet. The listing was collected from its source, and the description is in its own SKILL.md.

Guardrail

Constrains what the agent is allowed to do.

collaborationcode reviewagent
Filed under

Communication & Collaboration

The skill itself

This is the whole product. A skill is instructions the model reads, so there is nothing behind the listing you cannot see first — the front matter loads with every session, and the body below it loads when the skill triggers.

SKILL.md20.6 kB · 397 lines
--- name: code-review-and-quality description: Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human. Use when you need to assess code quality across multiple dimensions before it enters the main branch. ---
6# Code Review and Quality
7
8## Overview
9
10Multi-dimensional code review with quality gates. Every change gets reviewed before merge — no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance.
11
12**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.
13
14## When to Use
15
16- Before merging any PR or change
17- After completing a feature implementation
18- When another agent or model produced code you need to evaluate
19- When refactoring existing code
20- After any bug fix (review both the fix and the regression test)
21
22## The Five-Axis Review
23
24Every review evaluates code across these dimensions:
25
26### 1. Correctness
27
28Does the code do what it claims to do?
29
30- Does it match the spec or task requirements?
31- Are edge cases handled (null, empty, boundary values)?
32- Are error paths handled (not just the happy path)?
33- Does it pass all tests? Are the tests actually testing the right things?
34- Are there off-by-one errors, race conditions, or state inconsistencies?
35
36### 2. Readability & Simplicity
37
38Can another engineer (or agent) understand this code without the author explaining it?
39
40- Are names descriptive and consistent with project conventions? (No temp, data, result without context)
41- Is the control flow straightforward (avoid nested ternaries, deep callbacks)?
42- Is the code organized logically (related code grouped, clear module boundaries)?
43- Are there any "clever" tricks that should be simplified?
44- **Could this be done in fewer lines?** (1000 lines where 100 suffice is a failure)
45- **Are abstractions earning their complexity?** (Don't generalize until the third use case)
46- Would comments help clarify non-obvious intent? (But don't comment obvious code.)
47- Are there dead code artifacts: no-op variables (_unused), backwards-compat shims, or // removed comments?
48- **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.
49- **Do repeated conditionals on the same shape appear?** They signal a missing model or dispatcher. A "temporary" branch is usually permanent debt.
50
51### 3. Architecture
52
53Does the change fit the system's design?
54
55- Does it follow existing patterns or introduce a new one? If new, is it justified?
56- Does it maintain clean module boundaries?
57- Is there code duplication that should be shared?
58- Are dependencies flowing in the right direction (no circular dependencies)?
59- Is the abstraction level appropriate (not over-engineered, not too coupled)?
60- **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.
61- **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.
62- **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.
63
64### 4. Security
65
66For detailed security guidance, see security-and-hardening. Does the change introduce vulnerabilities?
67
68- Is user input validated and sanitized?
69- Are secrets kept out of code, logs, and version control?
70- Is authentication/authorization checked where needed?
71- Are SQL queries parameterized (no string concatenation)?
72- Are outputs encoded to prevent XSS?
73- Are dependencies from trusted sources with no known vulnerabilities?
74- Is data from external sources (APIs, logs, user content, config files) treated as untrusted?
75- Are external data flows validated at system boundaries before use in logic or rendering?
76
77### 5. Performance
78
79For detailed profiling and optimization, see performance-optimization. Does the change introduce performance problems?
80
81- Any N+1 query patterns?
82- Any unbounded loops or unconstrained data fetching?
83- Any synchronous operations that should be async?
84- Any unnecessary re-renders in UI components?
85- Any missing pagination on list endpoints?
86- Any large objects created in hot paths?
87
88## Structural Remedies
89
90When 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:
91
92- **Replace a chain of conditionals** with a typed model or an explicit dispatcher.
93- **Collapse duplicate branches** into a single clearer flow.
94- **Separate orchestration from business logic** so each reads on its own.
95- **Move feature-specific logic** out of a shared module into the package that owns the concept.
96- **Reuse the canonical helper** instead of a bespoke near-duplicate.
97- **Make a type boundary explicit** so downstream branching disappears.
98- **Delete a pass-through wrapper** that adds indirection without clarifying the API.
99- **Extract a helper, or split a large file** into focused modules.
100
101Prefer the remedy that removes moving pieces over one that spreads the same complexity around.
102
103## Change Sizing
104
105Small, focused changes are easier to review, faster to merge, and safer to deploy. Target these sizes:
106
107```
108~100 lines changed → Good. Reviewable in one sitting.
109~300 lines changed → Acceptable if it's a single logical change.
110~1000 lines changed → Too large. Split it.
111```
112
113**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.
114
115**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.
116
117**Splitting strategies when a change is too large:**
118
119| Strategy | How | When |
120|----------|-----|------|
121| **Stack** | Submit a small change, start the next one based on it | Sequential dependencies |
122| **By file group** | Separate changes for groups needing different reviewers | Cross-cutting concerns |
123| **Horizontal** | Create shared code/stubs first, then consumers | Layered architecture |
124| **Vertical** | Break into smaller full-stack slices of the feature | Feature work |
125
126**When large changes are acceptable:** Complete file deletions and automated refactoring where the reviewer only needs to verify intent, not every line.
127
128**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.
129
130## Change Descriptions
131
132Every change needs a description that stands alone in version control history.
133
134**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.
135
136**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.
137
138**Anti-patterns:** "Fix bug," "Fix build," "Add patch," "Moving code from A to B," "Phase 1," "Add convenience functions."
139
140## Review Process
141
142### Step 1: Understand the Context
143
144Before looking at code, understand the intent:
145
146```
147- What is this change trying to accomplish?
148- What spec or task does it implement?
149- What is the expected behavior change?
150```
151
152### Step 2: Review the Tests First
153
154Tests reveal intent and coverage:
155
156```
157- Do tests exist for the change?
158- Do they test behavior (not implementation details)?
159- Are edge cases covered?
160- Do tests have descriptive names?
161- Would the tests catch a regression if the code changed?
162```
163
164### Step 3: Review the Implementation
165
166Walk through the code with the five axes in mind:
167
168```
169For each file changed:
1701. Correctness: Does this code do what the test says it should?
1712. Readability: Can I understand this without help?
1723. Architecture: Does this fit the system?
1734. Security: Any vulnerabilities?
1745. Performance: Any bottlenecks?
175```
176
177### Step 4: Categorize Findings
178
179Label every comment with its severity so the author knows what's required vs optional:
180
181| Prefix | Meaning | Author Action |
182|--------|---------|---------------|
183| *(no prefix)* | Required change | Must address before merge |
184| **Critical:** | Blocks merge | Security vulnerability, data loss, broken functionality |
185| **Nit:** | Minor, optional | Author may ignore — formatting, style preferences |
186| **Optional:** / **Consider:** | Suggestion | Worth considering but not required |
187| **FYI** | Informational only | No action needed — context for future reference |
188
189This prevents authors from treating all feedback as mandatory and wasting time on optional suggestions.
190
191**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.
192
193### Step 5: Verify the Verification
194
195Check the author's verification story:
196
197```
198- What tests were run?
199- Did the build pass?
200- Was the change tested manually?
201- Are there screenshots for UI changes?
202- Is there a before/after comparison?
203```
204
205## Multi-Model Review Pattern
206
207Use different models for different review perspectives:
208
209```
210Model A writes the code
211 │
212 ▼
213Model B reviews for correctness and architecture
214 │
215 ▼
216Model A addresses the feedback
217 │
218 ▼
219Human makes the final call
220```
221
222This catches issues that a single model might miss — different models have different blind spots.
223
224**Example prompt for a review agent:**
225```
226Review this code change for correctness, security, and adherence to
227our project conventions. The spec says [X]. The change should [Y].
228Flag any issues as Critical, Required, Optional, or Nit.
229```
230
231## Dead Code Hygiene
232
233After any refactoring or implementation change, check for orphaned code:
234
2351. Identify code that is now unreachable or unused
2362. List it explicitly
2373. **Ask before deleting:** "Should I remove these now-unused elements: [list]?"
238
239Don'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.
240
241```
242DEAD CODE IDENTIFIED:
243- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
244- OldTaskCard component in src/components/ — replaced by TaskCard
245- LEGACY_API_URL constant in src/config.ts — no remaining references
246→ Safe to remove these?
247```
248
249## Review Speed
250
251Slow reviews block entire teams. The cost of context-switching to review is less than the waiting cost imposed on others.
252
253- **Respond within one business day** — this is the maximum, not the target
254- **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
255- **Prioritize fast individual responses** over quick final approval. Quick feedback reduces frustration even if multiple rounds are needed
256- **Large changes:** Ask the author to split them rather than reviewing one massive changeset
257
258## Handling Disagreements
259
260When resolving review disputes, apply this hierarchy:
261
2621. **Technical facts and data** override opinions and preferences
2632. **Style guides** are the absolute authority on style matters
2643. **Software design** must be evaluated on engineering principles, not personal preference
2654. **Codebase consistency** is acceptable if it doesn't degrade overall health
266
267**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.
268
269## Honesty in Review
270
271When reviewing code — whether written by you, another agent, or a human:
272
273- **Don't rubber-stamp.** "LGTM" without evidence of review helps no one.
274- **Don't soften real issues.** "This might be a minor concern" when it's a bug that will hit production is dishonest.
275- **Quantify problems when possible.** "This N+1 query will add ~50ms per item in the list" is better than "this could be slow."
276- **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.
277- **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.
278
279## Dependency Discipline
280
281Part of code review is dependency review:
282
283**Before adding any dependency:**
2841. Does the existing stack solve this? (Often it does.)
2852. How large is the dependency? (Check bundle impact.)
2863. Is it actively maintained? (Check last commit, open issues.)
2874. Does it have known vulnerabilities? (npm audit)
2885. What's the license? (Must be compatible with the project.)
289
290**Rule:** Prefer standard library and existing utilities over new dependencies. Every dependency is a liability.
291
292**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:
293
2941. **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.
2952. **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.
2963. **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.
2974. **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.
2985. **Keep the lockfile honest.** Commit it, review its diff, and never hand-edit it. The lockfile is the thing that actually pins what ships.
299
300For 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.
301
302## The Review Checklist
303
304```markdown
305## Review: [PR/Change title]
306
307### Context
308- [ ] I understand what this change does and why
309
310### Correctness
311- [ ] Change matches spec/task requirements
312- [ ] Edge cases handled
313- [ ] Error paths handled
314- [ ] Tests cover the change adequately
315
316### Readability
317- [ ] Names are clear and consistent
318- [ ] Logic is straightforward
319- [ ] No unnecessary complexity
320
321### Architecture
322- [ ] Follows existing patterns
323- [ ] No unnecessary coupling or dependencies
324- [ ] Appropriate abstraction level
325- [ ] Refactors reduce complexity rather than relocate it
326- [ ] No feature logic in shared modules; file stays within a healthy size
327
328### Security
329- [ ] No secrets in code
330- [ ] Input validated at boundaries
331- [ ] No injection vulnerabilities
332- [ ] Auth checks in place
333- [ ] External data sources treated as untrusted
334
335### Performance
336- [ ] No N+1 patterns
337- [ ] No unbounded operations
338- [ ] Pagination on list endpoints
339
340### Verification
341- [ ] Tests pass
342- [ ] Build succeeds
343- [ ] Manual verification done (if applicable)
344
345### Verdict
346- [ ] **Approve** — Ready to merge
347- [ ] **Request changes** — Issues must be addressed
348```
349## See Also
350
351- For detailed security review guidance, see ../../references/security-checklist.md
352- For performance review checks, see ../../references/performance-checklist.md
353
354## Common Rationalizations
355
356| Rationalization | Reality |
357|---|---|
358| "It works, that's good enough" | Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds. |
359| "I wrote it, so I know it's correct" | Authors are blind to their own assumptions. Every change benefits from another set of eyes. |
360| "We'll clean it up later" | Later never comes. The review is the quality gate — use it. Require cleanup before merge, not after. |
361| "AI-generated code is probably fine" | AI code needs more scrutiny, not less. It's confident and plausible, even when wrong. |
362| "The tests pass, so it's good" | Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns. |
363| "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. |
364| "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. |
365| "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. |
366| "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. |
367
368## Red Flags
369
370- PRs merged without any review
371- Review that only checks if tests pass (ignoring other axes)
372- "LGTM" without evidence of actual review
373- Security-sensitive changes without security-focused review
374- Large PRs that are "too big to review properly" (split them)
375- No regression tests with bug fix PRs
376- Review comments without severity labels — makes it unclear what's required vs optional
377- Accepting "I'll fix it later" — it never happens
378- A refactor that moves code around without reducing the number of concepts a reader must hold
379- A change that grows an already-large file instead of decomposing it
380- New conditionals scattered into unrelated code paths (a missing abstraction)
381- A bespoke helper that duplicates an existing canonical one, or feature logic placed in a shared module
382- A bulk "bump dependencies" PR with no changelog review and no per-package isolation
383- A lockfile change that's hand-edited, uncommitted, or merged without reviewing its diff
384
385## Verification
386
387After review is complete:
388
389- [ ] All Critical issues are resolved
390- [ ] All Required (no-prefix) changes are resolved or explicitly deferred with justification
391- [ ] Tests pass
392- [ ] Build succeeds
393- [ ] The verification story is documented (what changed, how it was verified)
394- [ ] Dependency upgrades were reviewed against their changelog, isolated per package, and verified by a green suite with the lockfile diff reviewed
395
396**Presumptive blockers:** surface and propose the simpler design for each of these; escalate to Required only when the change actively makes structure worse: a refactor that relocates complexity instead of reducing it; a change that pushes a file past the size boundary with no decomposition; feature logic added to a shared module; a near-duplicate of an existing canonical helper; a silent fallback that hides an unclear invariant.
397
In the file
SKILL.md3,262 words
Files1
LicenceMIT
Why you can read it

Nothing in a skill executes. The client loads the text and the model follows it, so a skill can be audited the way a runbook is — by reading it.

What it costs in context

Skills are not billed by the call. They are paid for in context: every token the instructions occupy is a token your code, your diff and your conversation cannot use. Here is what this one takes and when it takes it.

≈70
always loaded
The name and description, so the model knows the skill exists and when to reach for it.
5,080
on trigger
The instruction body, read only when the skill fires.
2.6%
of a 200k window
Ten skills this size would take about 26% of the window before you open a file.
050k100k150k200k context window

5.2k tokens, estimated from the bundle at four bytes to the token, held for the rest of the session once it triggers. Heavy. Teams tend to install this one per project rather than globally, and load it only when the job comes up.

Servers bill, skills cost

A server charges by the month. A skill charges once per session, in context, and then keeps charging it for as long as the session lives.

Before and after

The same question, put to the same model twice: once as it comes, and once with these instructions loaded.

No worked example has been published for this skill yet.

Adoption
Installsnone yet
Ratingno reviews yet

The procedure it runs

The procedure has not been published here. It is in the skill’s own SKILL.md, which its author has not sent to the marketplace yet.

Prose, not code

These steps are written for a model to follow, not executed by a runtime. It can still be told to skip one, and it will say so when it does.

Servers it uses

None. This skill calls no MCP servers at all.

Everything it needs is in the instructions, so it works in a project with nothing connected — the model reads the file and changes how it works with what it can already reach.

It writes no files and reaches no network. All it changes is how the model reasons and writes.

What it asks for
Writes filesno
Network accessno

Read from the allowed-tools line of this skill’s own SKILL.md. A skill grants no permissions of its own — it can only ask for tools your client already has.

What it will not do

Every skill is narrow, and the useful ones say where they stop. These are the jobs this one is the wrong tool for.

What this skill is not for has not been published here. Nothing is implied by that: it is a section the author has not filled in.

What is in the bundle

1 file, 20.6 kB on disk. A bundle is text throughout: the instructions the model reads, plus the templates it fills in.

  • SKILL.md20.6 kB
What is not in it

No dependencies and nothing executable: a skill is text the agent reads, so the bundle is 1 file you can review in full before installing. The MIT licence covers the templates and examples as well as the instructions.

Install

Installing copies the bundle into your project. Nothing runs at install time — the files sit on disk until the model reads them.

$45 once
Code Review and Quality · MIT · addyosmani
one-time
Price$45 once
LicenceMIT — the author’s, unchanged by this purchase
Paid throughStripe, once, on the card you add at the checkout
Keeps workingfor good — the files are yours once they are on disk
Updatesevery update its author ships, delivered through this account

You can read the whole bundle before paying — the SKILL.md above is the product, not a preview of it. What the money buys is the delivery: the folder packaged and handed to your machine by key, every update its author ships, and our support if it does not do what this listing says. The terms of use are MIT, set by the author and unchanged by buying it here.

Sign in to buyCreate an account

Payment runs through Stripe, on a page like this one rather than a redirect. Once there is an account it joins the same mcprush invoice as everything else you run, so there is never a second card to enter.

Which clients pick it up on their own

A skill is a folder of text. A client with a skills folder reads it without being told; everywhere else the same text works, it is just handed to the model rather than found.

Claude Code.claude/skills/
Claude Desktop
ChatGPT
Cursor.cursor/skills/
VS Code.github/skills/
Codex CLI.agents/skills/
Gemini CLI.gemini/skills/
Grok.grok/skills/
Zed.agents/skills/
Windsurf.windsurf/skills/
Agent SDK.claude/skills/
HTTP / API
This release
Versionnot versioned
Publishedno release date on file
Price$45
Referenceaddyosmani/code-review-and-quality

Versions

Its author publishes no version number, so there is nothing here to pin to: what you install is the folder as it stands today. Instructions change more often than APIs do — a skill can be rewritten entirely without anything it depends on moving.

v
  • No earlier releases have been published to the marketplace.
Pinning

Nothing to pin to: this skill carries no version number of its own. What you install is what the folder holds on the day you install it.

Reviews

—no reviews yet · no installs yet

Nobody has reviewed this skill yet. The rating is the mean of the reviews written here, so there is none until somebody writes the first.

Who can post

Only accounts that have had the skill installed for fourteen days, so a review is written after living with it rather than after reading it. Publishers may reply once.

Who wrote it

AD
addyosmani

Publishes on mcprush.

0 servers listed13 skills listednot claimed
Profile
Publisher
Servers0