Code Review

Code Review

Goals

• Validate the quality and stability of the change.
• Identify issues that should block merge, ordered by severity.
• Provide evidence-based feedback with a clear remediation direction.

Core Principles

• Report the highest-severity issues first.
• Attach file and line evidence to every issue.
• Prioritize behavior, security, data integrity, and regression risk over style preferences.
• Do not turn guesses into claims. Mark uncertainty as "needs confirmation."
• If no issues are found, state that there are no critical findings and note remaining risks.

Review Depth Modes

• Quick: focus only on critical or high-risk regressions.
• Standard: balance functionality, security, performance, and test coverage.
• Deep: expand into changed files, adjacent callers, and boundary contracts.

Use Standard by default unless the user asks otherwise.

Review Workflow

1. Understand the context.

• Read the PR description, issue, acceptance criteria, and change intent.
• Fix the scope: full PR, specific commit, or working tree.

2. Build a change map.

• Review the changed files and diff.
• Trace the impact path quickly: input -> logic -> storage, response, or render.

3. Inspect high-risk areas first.

• Authentication and authorization
• File or network I/O
• Delete, move, or recovery paths
• Transactions and concurrency
• Exception handling and rollback

4. Review against the checklist.

• Look for gaps across functionality, security, performance, code quality, tests, and docs or operations.

5. Write the result as findings.

• Order them Critical -> High -> Medium -> Low.
• Include the problem, evidence, impact, and fix in each item.

Severity Definitions

• Critical: immediate blockers such as auth bypass, privilege escalation, permanent data loss, or likely service outage.
• High: major functional errors, incorrect business outcomes, weakened security boundaries, or issues likely to cause incidents.
• Medium: conditional bugs, performance degradation, or growing maintainability risk. Fix before release when possible.
• Low: non-critical UX issues, minor exception handling gaps, or readability and consistency improvements.

Unified Checklist

Pre-Review

• Did you read the PR description, issue, and requirements to understand the goal?
• Did you separate in-scope from out-of-scope changes?
• Did you review CI results and failing logs?

Functionality

• Does it satisfy the requirements and acceptance criteria?
• Do both success and failure paths work?
• Are boundary values, empty values, nulls, and permission-denied cases handled?
• Are state transitions such as loading, success, failure, and retry consistent?

Security

• Is input validation performed at the boundary?
• Is the auth and authorization order correct?
• Is there no exposure risk for sensitive data such as tokens, keys, or personal information?
• Are there no hard-coded secrets?
• Is there no SQL injection, XSS, path traversal, or privilege bypass risk?

Performance

• Are there no unnecessary loops, queries, or N+1 patterns?
• Is there no excessive calculation or rendering for large data sets or long lists?
• Are cache and invalidation points appropriate?

Code Quality & Maintainability

• Do names explain intent?
• Are functions or components not taking on too much responsibility?
• Is duplicate logic not growing unnecessarily?
• Does the change preserve existing conventions and architecture boundaries?

Tests

• Were tests added for new or changed logic?
• Are failure, exception, and permission-denied cases covered?
• Are high-regression-risk paths protected?
• If the UI changed, is there at least one piece of visual validation evidence?

Docs & Ops

• Are behavior changes reflected in docs or changelogs?
• Are operational impacts such as env vars, migrations, and rollback paths documented?

Commonly Missed Traps

• Only the happy path works and failure paths are empty.
• Permission checks happen too late, after side effects already occurred.
• Subtle regressions appear in existing contracts such as API fields, status values, or event order.
• Missing cache invalidation causes UI and data mismatch.
• Tests pass, but real user input paths such as mobile, keyboard, or async races were never validated.

Review Comment Templates

Change Request

[Severity] Problem summary
- File: path/to/file.ts:line
- Evidence: current behavior or code evidence
- Impact: user, data, or operational impact
- Fix: minimum change and required tests

Clarifying Question

Question: This branch may need rollback on failure. What is the intended behavior?
Context: path/to/file.ts:line
Suggestion: Consider restoring state on failure or clarifying the transaction boundary.

Positive Feedback

Nice: Validating boundary inputs at the entry point keeps the downstream logic simpler.

Output Requirements

• Put findings first.
• Sort items by severity.
• Include the following four pieces in each item:
  • Problem
  • Evidence with file and line
  • Impact
  • Recommended fix

Recommended format:

1. [High] Authentication bypass risk
- File: src/auth/service.ts:88
- Evidence: admin-only function is called without a permission check
- Impact: a regular user may execute admin behavior
- Fix: add a role or permission guard before the call and add regression tests

Exit Rules

• If findings exist, keep the summary within three lines.
• If there are no findings, include all of the following:
  • "No critical issues found"
  • Remaining risks such as test gaps or areas needing manual validation
  • One to three recommended follow-up checks