# Code Review Checklist: [Team or repository name]

> Code review checklist and review agreement for a team. It sets out
> roles, what reviewers check, how comments are labelled, how large a
> change should be and how fast reviews happen. Based on common industry
> practice for code review, with security items informed by OWASP
> guidance. Replace every `[bracketed]` value and adapt the lists to your
> stack.

<!--
Guidance: How to use this template
- Agree this document as a team, then link it from the pull request template.
  A checklist nobody agreed to becomes a source of arguments.
- Do not tick every box on every change. Reviewers scan the sections that
  apply; authors self-review against the same list before asking for review.
- Automate anything a machine can check (formatting, lint, types, secrets,
  dependency scanning) so humans spend review time on design and correctness.
- Revisit the checklist each quarter: remove items nobody uses, add items
  that would have caught recent incidents.
-->

| Field | Value |
|---|---|
| Document ID | [CRC-TEAM-001] |
| Applies to | [Repositories or services] |
| Version | [0.1] |
| Status | [Draft / Agreed / Superseded] |
| Owner | [@engineering-lead] |
| Agreed by | [Team name], YYYY-MM-DD |
| Next review | YYYY-MM-DD |
| Related documents | [Coding standards], [Pull request template], [Security standard], [Definition of done] |

## 1. How to use this checklist

<!-- Guidance: Explain the flow from author self-review to merge in a few steps. -->

1. **Author self-review**: before requesting review, the author reads the full diff and checks it against sections 3.1 to 3.10, then fills the pull request description (what, why, how tested, risks).
2. **Automated checks**: CI must be green (build, tests, lint, type check, security scans) before a human reviews.
3. **Reviewer pass**: the reviewer reads the description first, then the tests, then the code. Comments use the labels in section 4.
4. **Resolution**: the author answers every comment. Blocking comments must be resolved or explicitly waived by the reviewer before merge.
5. **Merge**: the author merges once approvals in section 2 are present and checks pass.

## 2. Reviewer roles

<!-- Guidance: Say who must approve what. Code owners and specialist reviewers prevent important changes slipping through with one casual approval. -->

| Role | When required | Responsibility |
|---|---|---|
| Peer reviewer | Every pull request | Correctness, design, readability, tests |
| Code owner | Changes to paths in `CODEOWNERS` | Fit with the module's design and conventions |
| Security reviewer | Authentication, authorisation, cryptography, input parsing, secrets, new external endpoints | Section 3.5 in depth |
| Accessibility reviewer | New or changed UI components and flows | Section 3.9 |
| Database reviewer | Schema migrations, queries on large tables | Migration safety, indexing, locking |
| Author | Always | Small focused changes, clear description, responding to comments |

Minimum approvals: [one peer reviewer for normal changes; two, including
a code owner, for changes to `[critical paths]`].

## 3. Checklist

<!-- Guidance: Ten areas. Keep items short and answerable with yes or no. Delete items your tools already enforce. -->

### 3.1 Correctness

- [ ] The change does what the linked ticket and description say, and nothing unrelated.
- [ ] Edge cases are handled: empty input, null or missing values, maximum sizes, zero, negative numbers, time zones, leap years.
- [ ] Error paths are handled; errors are not swallowed silently.
- [ ] Concurrency is safe: no race conditions on shared state; retries are idempotent.
- [ ] Backward compatibility is kept for APIs, events, stored data and configuration, or the break is planned and versioned.
- [ ] Database migrations are reversible or safe to roll forward, and do not lock large tables during peak hours.

### 3.2 Design

- [ ] The code sits in the right layer and module; dependencies point in the agreed direction.
- [ ] No duplication of existing utilities or services.
- [ ] Abstractions are justified by current needs, not speculative ones.
- [ ] Public interfaces are small and named for what they do.
- [ ] Significant design decisions are recorded in an ADR.

### 3.3 Readability and maintainability

- [ ] Names describe intent; no unexplained abbreviations.
- [ ] Functions are short enough to understand at a glance; deep nesting is avoided.
- [ ] Comments explain why, not what; outdated comments are removed.
- [ ] No dead code, commented-out code or debug output.
- [ ] Follows the team coding standard where tools do not enforce it.

### 3.4 Tests

- [ ] New behaviour has tests at the lowest useful level; bug fixes include a test that fails without the fix.
- [ ] Tests assert behaviour, not implementation details.
- [ ] Tests are deterministic: no reliance on real time, network, ordering or shared state.
- [ ] Test names describe the scenario and expected outcome.
- [ ] Coverage of changed lines meets the team target ([80%]) or the gap is explained.

### 3.5 Security (OWASP-informed)

- [ ] All input from users, files, headers and other services is validated on the server side.
- [ ] Output is encoded for its context (HTML, attribute, URL, JavaScript, SQL, shell); no string-built queries or commands.
- [ ] Every endpoint checks authorisation for the specific object requested, not only that the user is logged in.
- [ ] Authentication and session changes follow the security standard (password storage, token lifetime, cookie flags).
- [ ] No secrets, keys or tokens in code, tests, logs or error messages.
- [ ] Sensitive data is not logged and is encrypted in transit and at rest where required.
- [ ] Server-side requests to user-supplied URLs are restricted (no unrestricted fetches to internal addresses).
- [ ] New dependencies follow the dependency policy and have no known critical vulnerabilities.
- [ ] Security-relevant events (login, permission change, failed authorisation) are logged for audit.

### 3.6 Performance

- [ ] No queries inside loops (N+1); queries use indexes on large tables.
- [ ] Unbounded lists are paginated; large payloads are streamed.
- [ ] Expensive work is cached or moved off the request path where appropriate.
- [ ] Front-end changes stay within the performance budget (bundle size, render time).

### 3.7 Observability

- [ ] Important operations emit structured logs with correlation IDs.
- [ ] New endpoints or jobs have metrics (rate, errors, duration) and, if critical, alerts.
- [ ] Error messages are actionable for the on-call engineer.
- [ ] Feature flags have an owner and a removal date.

### 3.8 Documentation

- [ ] README, API reference or runbook updated when behaviour, configuration or operations change.
- [ ] Changelog or release notes entry added for user-visible changes.
- [ ] Public functions and API endpoints have accurate doc comments or schema descriptions.

### 3.9 Accessibility

- [ ] Interactive elements are reachable and operable by keyboard, with visible focus.
- [ ] Controls have accessible names; images have text alternatives or are marked decorative.
- [ ] Colour contrast meets the target level (WCAG 2.2 AA) and information is not conveyed by colour alone.
- [ ] Form errors are shown in text and announced to assistive technology.
- [ ] Automated accessibility checks pass for changed components.

### 3.10 Internationalisation

- [ ] No hard-coded user-facing strings; all text goes through the translation system.
- [ ] Dates, numbers and currencies are formatted for the user's locale.
- [ ] Layouts tolerate longer translations and, if supported, right-to-left scripts.
- [ ] Text input handles IME composition and non-Latin characters.

## 4. Comment labels and severity

<!-- Guidance: Labels tell the author what must change and what is optional. Using them consistently removes most review friction. -->

| Label | Meaning | Blocks merge? | Example |
|---|---|---|---|
| `blocker:` | Must change before merge: bug, security issue, data loss risk, broken contract | Yes | `blocker: this endpoint returns other users' invoices; check ownership before loading.` |
| `issue:` | Should change; reviewer expects a fix or a convincing reason | Yes, until answered | `issue: this retry is not idempotent and may charge twice.` |
| `suggestion:` | Improvement the author may accept or decline | No | `suggestion: extracting this into a helper would make the test simpler.` |
| `question:` | Reviewer needs to understand before deciding | No, unless it reveals an issue | `question: why is the timeout 30 s here and 5 s elsewhere?` |
| `nit:` | Minor style or naming preference | No | `nit: "cnt" could be "retryCount".` |
| `praise:` | Something done well, worth repeating | No | `praise: clear migration plan with rollback.` |

Comment etiquette: comment on the code, not the person; explain the
reason; offer an alternative where possible; resolve disagreements in a
short call rather than long threads, then record the outcome in the pull request.

## 5. Pull request size guidance

<!-- Guidance: Small pull requests get faster and better reviews. Set limits and explain how to split larger work. -->

| Size | Changed lines (excluding generated files and lockfiles) | Expectation |
|---|---|---|
| Small | Up to 200 | Preferred; review in one sitting |
| Medium | 201 to 400 | Acceptable; description must explain structure |
| Large | Over 400 | Split, unless mechanical (renames, generated code); agree with reviewer in advance |

Ways to split: separate refactoring from behaviour changes; land
interfaces before implementations; hide incomplete features behind flags;
split migrations from the code that uses them.

## 6. Review service level

<!-- Guidance: Agree how fast reviews happen. Slow reviews push authors into large batched changes, which makes review worse. -->

| Item | Target |
|---|---|
| First response to a review request | Within [4 working hours] |
| Follow-up after author changes | Within [1 working day] |
| Urgent fixes (production incident) | Within [30 minutes]; reviewer on call |
| Maximum age of an open pull request | [5 working days] before it is discussed at stand-up |

Review load is shared: each engineer reviews at least [as many pull
requests as they open] per week. Metrics tracked monthly: median time to
first review, median time to merge, and share of pull requests over 400 lines.

## 7. Revision of this checklist

<!-- Guidance: Keep a short record so the team sees why items were added or removed. -->

| Version | Date | Change | Reason |
|---|---|---|---|
| 0.1 | YYYY-MM-DD | Initial checklist | Team agreement |
| [0.2] | YYYY-MM-DD | *Example:* Added server-side request item to 3.5 | Finding from penetration test |

## Standard and licence note

Based on common industry practice for code review. Security items are
informed by OWASP guidance such as the OWASP Top 10 and the OWASP Code
Review Guide, written here in original wording; consult OWASP for the
full material. Template licence: MIT.