code-review

name	code-review
description	Perform a general code review. Use it to inspect PRs, commits, diffs, or local changes for functional bugs, regressions, security issues, performance degradation, missing tests, and maintainability risks. Apply it when the user asks for a code review, PR review, risk check, or merge-readiness assessment.

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