Code Review and Quality

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:

1. Correctness: Does this code do what the test says it should?
2. Readability: Can I understand this without help?
3. Architecture: Does this fit the system?
4. Security: Any vulnerabilities?
5. 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:

1. Technical facts and data override opinions and preferences
2. Style guides are the absolute authority on style matters
3. Software design must be evaluated on engineering principles
4. 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