code-review-and-quality

Code Review and Quality

Overview

Multi-dimensional code review with quality gates. Every change must be reviewed before merging, no exceptions. Review covers five axes: correctness, readability, architecture, security, and performance.

Approval criteria: Approve a change when it clearly improves overall code health, even if it is not perfect. Perfect code does not exist; the goal is continuous improvement. Do not block it just because it is not exactly the same as how you would write it. If it improves the codebase and follows project conventions, approve it.

When to use

• Before merging any PR or change
• After completing feature implementation
• When another agent or model has produced code you need to evaluate
• When refactoring existing code
• After fixing any bug (review both the fix and the regression test)

Five-axis review

Evaluate code from these dimensions in every review:

1. Correctness

Does the code do what it claims to do?

• Does it meet the spec or task requirements?
• Does it handle edge cases (null, empty, boundary values)?
• Does it handle error paths (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 and simplicity

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

• Are names descriptive and consistent with project conventions? (no lack of context temp、data、result）
• Is control flow direct and clear (avoid nested ternaries, deep callbacks)?
• Is the code organization logical (related code kept together, clear module boundaries)?
• Are there "clever" tricks that should be simplified?
• Can it be done in fewer lines?(1000 lines when 100 would suffice is a failure)
• Does the abstraction justify the complexity it introduces?(Do not generalize before the third use case)
• Do comments help clarify non-obvious intent? (But do not comment obvious code.)
• Are there traces of dead code: no-op variables (_unused), backward-compatibility shims, or // removed comments?
• Is a newly added conditional branch being hard-wired into an unrelated flow? This is a design smell, not a minor issue (nit); put this logic into its own helper, state, or policy, and do not entangle existing paths.
• Is there repeated conditional checking on the same shape? This indicates a missing model or dispatcher. "Temporary" branches are usually permanent debt.

3. Architecture

Does this change fit the system design?

• Does it follow existing patterns, or introduce a new pattern? If it is a new pattern, is there sufficient justification?
• Does it maintain clear module boundaries?
• Is there code duplication that should be shared?
• Is the dependency direction correct (no circular dependencies)?
• Is the level of abstraction appropriate (neither over-engineered nor over-coupled)?
• Does this refactor reduce complexity, or does it just move it somewhere else? Count how many concepts a reader must hold in their head at once to follow this change. If the so-called "cleaner" version does not reduce that number, it is not clean: prefer refactors that make an entire branch, pattern, or layer disappear, rather than re-concentrating the same logic elsewhere. Rather than polishing an abstraction, delete it.
• Has feature-specific logic leaked into shared or generic modules? Keep logic in the layer it belongs to, reuse existing canonical helpers instead of creating a near-duplicate, and do not tolerate architectural drift.
• Are type boundaries explicit? Question those casually written any/unknown/optional types/type assertions, as well as silent fallbacks that mask unclear invariants. Making boundaries explicit often makes the surrounding control flow simpler.

4. Security

For detailed security guidance, see security-and-hardening. Does this change introduce vulnerabilities?

• Is user input validated and sanitized?
• Are secrets kept out of code, logs, and version control?
• Are checks performed where authentication/authorization is required?
• Are SQL queries parameterized (no string concatenation)?
• Is output encoded to prevent XSS?
• Do dependencies come from trusted sources and have no known vulnerabilities?
• Is data from external sources (APIs, logs, user content, configuration files) treated as untrusted?
• Are external data flows validated at system boundaries before being used for logic or rendering?

5. Performance

For detailed profiling and optimization guidance, see performance-optimization. Does this change introduce performance issues?

• Are there any N+1 query patterns?
• Are there any unbounded loops or unconstrained data fetching?
• Are there any synchronous operations that should be asynchronous?
• Are there any unnecessary re-renders in UI components?
• Is pagination missing from list endpoints?
• Are large objects being created in hot paths?

Structured Remediation

When pointing out structural problems, provide a fix, not just the problem itself. A review that only says "this is too complex" leaves the author at a loss. Propose named refactoring options:

• Replace a long chain of conditionals with a typed model or an explicit dispatcher.
• Merge duplicate branches into a single clearer flow.
• Separate orchestration from business logic, so that each is readable on its own.
• Move feature-specific logic out of shared modulesand into the package that truly owns this concept.
• Reuse canonical helpers, rather than writing your own near-duplicate custom version.
• Make type boundaries explicit, so that downstream branches disappear along with it.
• Delete pass-through wrappersthat add a layer of indirection without making the API clearer.
• Extract helpers, or split large files intomodules with focused responsibilities.

Prefer remediation options that reduce moving parts over options that spread the same complexity somewhere else.

Change Size

Small, focused changes are easier to review, faster to merge, and safer to deploy. The target size is as follows:

~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.

Watch the total file size, not just the diff size. A small diff can still push a file past a healthy boundary: about 1000 totallines per file is a common scrutiny signal (distinct from the threshold of about 1000 changedlines above), but it is not a hard limit either. When a change significantly grows a file that is already large, first ask whether to extract a helper, subcomponent, or module before piling on more code. Decompose first, then add.

What counts as "one change": A self-contained modification that solves only one thing, comes with relevant tests, and leaves the system usable after commit. It is one component of a feature, not the entire feature.

Splitting strategies when a change is too large:

Strategy	Approach	When to Use
Stack	Commit a small change first, then base the next one on it	Sequential dependencies exist
Group by file	Open a separate change for each group of files that needs a different reviewer	Cross-cutting concerns
Horizontal split	Write shared code/stubs first, then write consumers	Layered architecture
Vertical split	Break the feature into smaller full-stack slices	Feature development

When large changes are acceptable: Complete file deletions, and automated refactorings where the reviewer only needs to confirm intent and does not need to verify line by line.

Keep refactoring and feature development separate. A change that both modifies existing code and adds new behavior is really two changes, so commit them separately. Small cleanups (such as variable renames) may be allowed in at the reviewer's discretion.

Change Description

Every change needs a description that can be understood on its own in version control history.

First line: Short, imperative mood, understandable on its own. Use "Delete the FizzBuzz RPC", not "Deleting the FizzBuzz RPC." It should carry enough information that someone searching the history can understand the change without reading the diff.

Body: What was changed and why. Include context, decisions, and reasoning that are not visible in the code itself. Link bug numbers, benchmark results, or design documents where relevant. Also be candid when the approach has flaws.

Anti-patterns: “Fix bug”、“Fix build”、“Add patch”、“Moving code from A to B”、“Phase 1”、“Add convenience functions”。

Review Process

Step 1: Understand the context

Before looking at the 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?
- Do tests have descriptive names?
- Would the tests catch a regression if the code changed?

Step 3: Review the implementation

Walk through the code along the five axes:

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: Grade the findings

Every comment should be labeled with a severity level so the author can distinguish what must be changed from what is optional:

Prefix	Meaning	How the author handles it
(no prefix)	Must fix	Must be resolved before merging
Critical:	Blocks merge	Security vulnerabilities, data loss, broken functionality
Nit:	Minor, optional	The author may ignore: formatting, style preferences
Optional: / Consider:	Suggestions	Worth considering, but not required
FYI	For reference only	No action needed: context to keep for later reference

This prevents the author from treating all feedback as mandatory and wasting time on optional suggestions.

Put what truly matters first. Order findings by impact: correctness and security first, then structural regressions and missed simplification opportunities, then everything else. Don't bury real problems under a pile of cosmetic nits: a few high-confidence comments are better than a long checklist. If you find one structural problem and ten nits, that structural problemisthe core of this review.

Step 5: Check the verification status

Check the author's verification narrative:

- What tests were run?
- Did the build pass?
- Was the change tested manually?
- Are there screenshots for UI changes?
- Is there a before/after comparison?

Multi-model review mode

Use different models to take on different review perspectives:

Model A writes the code
    │
    ▼
Model B reviews for correctness and architecture
    │
    ▼
Model A addresses the feedback
    │
    ▼
Human makes the final call

This can catch issues a single model might miss, because different models have different blind spots.

Example prompt for a review agent:

Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
Flag any issues as Critical, Required, Optional, or Nit.

Dead code hygiene

After any refactor or implementation change, check for orphaned code:

1. Find code that is now unreachable or no longer used
2. List it explicitly
3. Before deleting, ask: "These elements are no longer used now, should they be deleted: [list]?"

Don't leave dead code in place; it confuses later readers and agents. But also don't quietly delete things you're not sure about. If unsure, ask.

DEAD CODE IDENTIFIED:
- formatLegacyDate() in src/utils/date.ts — replaced by formatDate()
- OldTaskCard component in src/components/ — replaced by TaskCard
- LEGACY_API_URL constant in src/config.ts — no remaining references
→ Safe to remove these?

Review speed

Slow reviews block the whole team. The context-switching cost of switching to a review is lower than the cost of making others wait.

• Respond within one business day: this is the upper limit, not the goal
• Ideal pace: respond as soon as possible after receiving a review request, unless you are in deep focused coding. A typical change should go through multiple rounds of review within one day
• Prioritize fast responses each round, rather than quickly giving final approval. Fast feedback reduces frustration even if it takes multiple rounds
• Large changes: ask the author to split them up, rather than forcing a review of one huge changeset

Handling disagreements

When resolving review disputes, follow this hierarchy:

1. Technical facts and datatake precedence over opinions and preferences
2. Style guidesare the highest authority on style issues
3. Software designmust be evaluated based on engineering principles, not personal preference
4. Codebase consistencyis acceptable as long as it does not harm overall health

Don't accept "I'll clean it up later." Experience proves that deferred cleanup almost never happens. Unless it is truly an emergency, require cleanup to be completed before submission. If surrounding issues really cannot be resolved in this change, ask for a bug to be filed and assigned to yourself.

Honesty in review

When reviewing code, whether the code comes from you, another agent, or a human:

• Don't just rubber-stamp it. An "LGTM" without review evidence benefits no one.
• Don't downplay real problems. Calling a bug that will blow up production "a minor concern" is dishonest.
• Quantify problems as much as possible. "This N+1 query adds about 50ms to each item in the list" is far better than "this might be a bit slow."
• Push back on approaches that are genuinely problematic. Sycophancy is a failure mode in review. If the implementation has problems, say so directly and offer an alternative.
• Accept being overruled gracefully. If the author has the full context and insists, respect their judgment. Comments target the code, not the person: rewrite personal criticism as an opinion about the code itself.

Dependency Discipline

Part of code review is dependency review:

Before adding any dependency:

1. Can the existing stack solve this? (Usually it can.)
2. How big is this dependency? (Check the impact on the bundle.)
3. Is it still actively maintained? (Look at recent commits and open issues.)
4. Does it have known vulnerabilities? (npm audit）
5. What is the license? (It must be compatible with the project.)

Rule: Prefer the standard library and existing tools over new dependencies. Every dependency is a liability.

Upgrading existing dependenciesis a code change like any other code change, and the riskiest upgrades are exactly the kind that get waved through as "bump deps" and merged in bulk. Review them with the same discipline:

1. Read the changelog, not just the version number. semver is just a promise maintainers may not keep: a "patch" can also carry behavior changes. For major version upgrades, read the migration notes and find out what will break.
2. Upgrade only one dependency per change. Upgrade and merge one at a time (or in small related groups). When a bulk bump breaks the build, you no longer know which package did it; single-package changes make the cause obvious and the rollback clean.
3. Let the tests speak. The basis for validating an upgrade is a test suite that is all green before and after the upgrade, not "it installed." If coverage around the dependency's behavior is thin, that gap is the real finding: add tests first.
4. Watch the transitive dependency graph. Most packages that get installed were not directly chosen by anyone. Review the lockfile diff, not just package.json: a bump of one direct dependency can pull in dozens of indirect changes.
5. Keep the lockfile honest. Commit it, review its diff, never hand-edit it. The lockfile is what actually pins down what gets released.

For npm audit results and supply chain risks (typosquatting, compromised maintainers), follow the security-and-hardening skill: this section covers the upgradeworkflow, and that section gives the security conclusions.

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
- [ ] Refactors reduce complexity rather than relocate it
- [ ] No feature logic in shared modules; file stays within a healthy size

### Security
- [ ] No secrets in code
- [ ] Input validated at boundaries
- [ ] No injection vulnerabilities
- [ ] Auth checks in place
- [ ] External data sources treated as untrusted

### 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

See Also

• For detailed security review guidance, see ../../references/security-checklist.md
• For performance review checklists, see ../../references/performance-checklist.md

Common Self-Justifications

Self-Justification	Reality
"It works, that's enough"	Code that works but is unreadable, insecure, or architecturally wrong accumulates compounding technical debt.
"I wrote it, so I'm sure it's fine"	Authors are blind to their own assumptions. Every change needs a second pair of eyes.
"I'll clean it up later"	"Later" never comes. Review is the quality gate; use it. Require cleanup before merge, not after.
"AI-generated code should be fine"	AI code needs more scrutiny, not less. It sounds confident and credible even when it's wrong.
"It's tested, so it's good"	Tests are necessary but not sufficient. They don't catch architectural problems, security risks, or readability issues.
"The refactor made it cleaner"	Moving complexity around is not the same as reducing it. If the reader still has to hold the same number of concepts in their head at once, the structure hasn't improved: go find the version that makes the branches disappear.
"It's just a small addition to this file"	A small diff can still push a file past a healthy size or bolt a branch onto an unrelated flow. Judge the final structure, not the diff size.
"It's just a version bump"	An upgrade is a behavior change you didn't write. Read the changelog: semver doesn't guarantee no breakage.
"Upgrading everything in one PR saves time"	A batch bump that breaks the build hides the offending package. One change per dependency keeps both the cause and the rollback clean.

Red flags

• Merging a PR without any review
• A review that only checks whether tests pass (ignoring the other axes)
• An "LGTM" with no actual evidence of review
• Security-related changes without a security-oriented review
• A PR "too big to review properly" (split it)
• A bug-fix PR without an accompanying regression test
• Review comments without severity labels: can't tell what's mandatory from what's optional
• Accepting "I'll fix it later": that won't happen
• A refactor that only moves code around without reducing the number of concepts the reader must hold in their head
• A change that lets an already-large file keep growing instead of decomposing it first
• Scattering new conditional branches across unrelated code paths (a sign of missing abstraction)
• Reinventing the wheel with a custom helper that nearly duplicates an existing canonical helper, or putting feature logic into a shared module
• A batch "bump dependencies" PR: no changelog read, no per-package isolation
• Lockfile changes that were hand-edited, uncommitted, or merged without a diff review

Verification

After review is complete:

• All Critical issues resolved
• All must-fix (no-prefix) items resolved, or explicitly deferred with a clear reason
• Tests pass
• Build succeeds
• Verification narrative recorded (what changed, how it was verified)
• Dependency upgrades reviewed against the changelog, isolated per package, and verified by a test suite that was green before and after the upgrade, with the lockfile diff reviewed too

Presumptive blockers: For each of the following, call it out and give a simpler design; upgrade to Required only when the change genuinely worsens the structure: refactorings that merely move complexity around rather than reducing it; changes that push a file past a size boundary without decomposition; adding feature logic into a shared module; near-duplication of an existing canonical helper; silent fallbacks that mask an unclear invariant.