mcpskills.net — Curated AI Skills, MCPs & Agents

mcpskills.net

Multi-dimensional code review with quality gates. Every change gets reviewed before merge.

When to Use

Before merging any PR or change
After completing a feature implementation
When another agent or model produced code you need to evaluate
When refactoring existing code
After any bug fix

The Five-Axis Review

Every review evaluates code across these dimensions:

1. Correctness

Does the code do what it claims to do?

Does it match the spec or task requirements?
Are edge cases handled (null, empty, boundary values)?
Are error paths handled (not just the happy path)?
Does it pass all tests?

2. Readability & Simplicity

Can another engineer understand this code without the author explaining it?

Are names descriptive and consistent with project conventions?
Is the control flow straightforward?
Is the code organized logically?
Could this be done in fewer lines?

3. Architecture

Does the change fit the system's design?

Does it follow existing patterns or introduce a new one?
Does it maintain clean module boundaries?
Are dependencies flowing in the right direction?

4. Security

Does the change introduce vulnerabilities?

Is user input validated and sanitized?
Are secrets kept out of code, logs, and version control?
Is authentication/authorization checked where needed?
Are SQL queries parameterized?

5. Performance

Does the change introduce performance problems?

Any N+1 query patterns?
Any unbounded loops or unconstrained data fetching?
Any unnecessary re-renders in UI components?
Any missing pagination on list endpoints?

Change Sizing

Target these sizes:

~100 lines changed   → Good. Reviewable in one sitting.
~300 lines changed   → Acceptable if it's a single logical change.
~1000 lines changed  → Too large. Split it.

Splitting strategies:

| Strategy | How | When | |----------|-----|------| | Stack | Submit a small change, start the next one based on it | Sequential dependencies | | By file group | Separate changes for groups needing different reviewers | Cross-cutting concerns | | Horizontal | Create shared code/stubs first, then consumers | Layered architecture | | Vertical | Break into smaller full-stack slices | Feature work |

Change Descriptions

Every change needs a description that stands alone in version control history.

First line: Short, imperative, standalone. "Delete the FizzBuzz RPC" not "Deleting the FizzBuzz RPC."

Body: What is changing and why. Include context, decisions, and reasoning not visible in the code itself.

Review Process

Step 1: Understand the Context

Before looking at code, understand the intent:

What is this change trying to accomplish?
What spec or task does it implement?
What is the expected behavior change?

Step 2: Review the Tests First

Tests reveal intent and coverage:

Do tests exist for the change?
Do they test behavior (not implementation details)?
Are edge cases covered?

Step 3: Review the Implementation

Walk through the code with the five axes in mind:

For each file changed:

Correctness: Does this code do what the test says it should?
Readability: Can I understand this without help?
Architecture: Does this fit the system?
Security: Any vulnerabilities?
Performance: Any bottlenecks?

Step 4: Categorize Findings

Label every comment with its severity:

| Prefix | Meaning | Author Action | |--------|---------|---------------| | (no prefix) | Required change | Must address before merge | | Critical: | Blocks merge | Security vulnerability, data loss, broken functionality | | Nit: | Minor, optional | Author may ignore — formatting, style preferences | | Optional: / Consider: | Suggestion | Worth considering but not required | | FYI | Informational only | No action needed |

Step 5: Verify the Verification

Check the author's verification story:

What tests were run?
Did the build pass?
Was the change tested manually?
Are there screenshots for UI changes?

The Review Checklist

## Review: [PR/Change title]

### Context
- [ ] I understand what this change does and why

### Correctness
- [ ] Change matches spec/task requirements
- [ ] Edge cases handled
- [ ] Error paths handled
- [ ] Tests cover the change adequately

### Readability
- [ ] Names are clear and consistent
- [ ] Logic is straightforward
- [ ] No unnecessary complexity

### Architecture
- [ ] Follows existing patterns
- [ ] No unnecessary coupling or dependencies
- [ ] Appropriate abstraction level

### Security
- [ ] No secrets in code
- [ ] Input validated at boundaries
- [ ] No injection vulnerabilities
- [ ] Auth checks in place

### Performance
- [ ] No N+1 patterns
- [ ] No unbounded operations
- [ ] Pagination on list endpoints

### Verification
- [ ] Tests pass
- [ ] Build succeeds
- [ ] Manual verification done (if applicable)

### Verdict
- [ ] **Approve** — Ready to merge
- [ ] **Request changes** — Issues must be addressed

Review Speed

Respond within one business day — this is the maximum, not the target
Ideal cadence: Respond shortly after a review request arrives
Prioritize fast individual responses over quick final approval
Large changes: Ask the author to split them

Handling Disagreements

When resolving review disputes, apply this hierarchy:

Technical facts and data override opinions and preferences
Style guides are the absolute authority on style matters
Software design must be evaluated on engineering principles
Codebase consistency is acceptable if it doesn't degrade overall health

Verification

After review is complete:

[ ] All Critical issues are resolved
[ ] All Required changes are resolved or explicitly deferred
[ ] Tests pass
[ ] Build succeeds
[ ] The verification story is documented