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.

1. Build a change map.

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

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

1. Review against the checklist.

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

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