Code Review Criteria

Code Review Criteria and Quality

Overview

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.

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.

When to Use

• The reviewer subagent of the review-code flow loads this skill to perform the review of a code change.
• To review code, always go through the review-code flow — never load this skill directly for that. This is the criteria reference, not the flow.

Core Principles

These principles underpin every axis. When in doubt, default to them.

• DRY (Don't Repeat Yourself): Every piece of knowledge has one authoritative representation. If the same logic appears in two places, extract it into a shared helper, model, or type. Reviewers: flag duplicated logic as a required change — it's not "just similar," it's drift that will diverge.
• KISS (Keep It Simple, Stupid): The simplest solution that works is the best solution. Complexity must earn its place. Reviewers: if you need more than one sentence to explain what a piece of code does, it's too complex — push for simplification before merge.
• YAGNI (You Aren't Gonna Need It): Don't add abstractions, hooks, or generalizations for hypothetical future use cases. Generalize on the third occurrence, not the first. Reviewers: delete speculative generality.
• Don't invent problems: Do not manufacture issues to produce more feedback. Every finding must be a real risk, a real readability barrier, or a real architectural concern — not a hypothetical or a stylistic preference disguised as a problem.

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? Are the tests actually testing the right things?
• Are there off-by-one errors, race conditions, or state inconsistencies?

2. Readability & Simplicity

Can another engineer (or agent) understand this code without the author explaining it?

• Are names descriptive and consistent with project conventions? (No temp, data, result without context)
• Is the control flow straightforward (avoid nested ternaries, deep callbacks)?
• Are there any "clever" tricks that should be simplified?
• KISS check: Is this the simplest approach that solves the problem? A 20-line straightforward function beats a 5-line clever one that requires a comment to explain.
• Could this be done in fewer lines? (1000 lines where 100 suffice is a failure)
• Are abstractions earning their complexity? (Don't generalize until the third use case)
• Is a new conditional bolted onto an unrelated flow? Push the logic into its own helper, state, or policy.
• Do repeated conditionals on the same shape appear? They signal a missing model or dispatcher.
• Are there dead code artifacts: no-op variables, backwards-compat shims, or // removed comments?

3. Architecture

Does the change fit the system's design?

• Does it follow existing patterns or introduce a new one? If new, is it justified?
• Does it maintain clean module boundaries?
• DRY check: Is there existing code that does the same thing? Reuse the canonical helper instead of writing a near-duplicate. If two branches do nearly the same thing, collapse them.
• Are dependencies flowing in the right direction (no circular dependencies)?
• Is the abstraction level appropriate (not over-engineered, not too coupled)?
• Does this refactor reduce complexity or just relocate it? Count the concepts a reader must hold. Prefer the restructuring that makes whole branches disappear over one that re-centralizes the same logic. Prefer deleting an abstraction to polishing it.
• Is feature-specific logic leaking into a shared or general-purpose module?
• Are type boundaries explicit? Question gratuitous any/unknown/optional/casts and silent fallbacks.
• Structural remedies: When you flag a problem, propose the move — not just the problem. Replace conditionals with dispatchers, collapse duplicate branches, separate orchestration from business logic, extract helpers, split large files. Prefer the remedy that removes moving pieces over one that spreads the same complexity around.

4. Security

For detailed security guidance, see security-and-hardening.

• 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 (no string concatenation)?
• Are outputs encoded to prevent XSS?
• Are dependencies from trusted sources with no known vulnerabilities?
• Is data from external sources (APIs, logs, user content, config files) treated as untrusted?

5. Performance

• Any N+1 query patterns?
• Any unbounded loops or unconstrained data fetching?
• Any synchronous operations that should be async?
• Any unnecessary re-renders in UI components?
• Any missing pagination on list endpoints?
• Any large objects created in hot paths?

Review Process

1. Understand the intent — What is this change trying to accomplish? What spec or task does it implement?
2. Review tests first — Tests reveal intent and coverage. Do they test behavior, not implementation details? Are edge cases covered?
3. Review the implementation — Walk through each file with the five axes in mind.
4. Categorize findings — Label every comment with its severity:

Unique finding IDs. Assign every finding a stable identifier: F1, F2, F3, … numbered in order of severity (Critical first, then High, Medium, Low, Suggestion). Use the ID everywhere the finding is mentioned — in section headers, in the verdict, in follow-up discussion. Never renumber within a review. Example: **F3 (High)** — app/validate.cljs:42 — duplicate branch logic….

For each finding, describe the circumstances under which it could fail: specific inputs, load conditions, timing, or user actions that trigger the problem. "This crashes when input is null" is actionable; "this might crash" is not.

Lead with what matters: correctness and security first, then structural issues, then everything else. A few high-conviction comments beat a long list.

5. Verify the verification — What tests were run? Did the build pass? Was the change tested manually? Screenshots for UI changes?

Review Output

Structure every review using this format:

Summary

Briefly explain what the code does and give an overall assessment.

Critical and High-Priority Issues

List problems that could cause security incidents, data loss, crashes, incorrect behavior, or major performance degradation. Each finding gets its unique ID (F1, F2, …). For each: state the severity, identify the file/function/code section, explain why it's a problem, describe failure circumstances, and provide a concrete improvement with corrected code when useful.

Other Findings

List medium- and low-priority issues, including maintainability and design concerns. Continue the ID sequence started above (F3, F4, …).

Suggested Refactoring

Provide focused code changes or revised snippets. Preserve existing behavior unless a behavior change is explicitly justified.

Testing Recommendations

Identify missing tests and describe specific test cases, including edge cases and failure scenarios.