# Pull Request Review Checklist

From Modern Git Academy — https://moderngitacademy.com/

---

## Before you open one

- [ ] The diff does **one thing**. Two concerns = two pull requests.
- [ ] Under ~400 lines where possible. Review quality falls off a cliff past that.
- [ ] The title says what changed and why, not "fixes".
- [ ] The description explains the *reason*, which the diff cannot show.
- [ ] You have read your own diff, line by line.
- [ ] Debug statements, commented-out code and stray files are gone.
- [ ] Tests cover the change and actually fail without it.
- [ ] CI passes.

## As a reviewer — correctness

- [ ] Does it do what the description claims?
- [ ] Edge cases: empty, null, zero, one, very large, concurrent.
- [ ] Error paths — not just the happy path.
- [ ] Are failures handled, or silently swallowed?
- [ ] Any change in behaviour for existing callers?

## Security

- [ ] No credentials, tokens or keys in the diff — including in tests and fixtures.
- [ ] User input validated before use.
- [ ] No string-built SQL, shell commands or file paths from user input.
- [ ] New dependencies: are they necessary, maintained, and reputable?
- [ ] Permissions unchanged, or deliberately and minimally widened.

## Workflow changes — read these harder

- [ ] Actions pinned to a **commit SHA**, not a mutable tag.
- [ ] `permissions:` set explicitly and minimally.
- [ ] No untrusted code executed with access to secrets.
- [ ] `pull_request_target` — if present, understand exactly why.

A workflow file change is a change to something that runs with credentials. It deserves more
scrutiny than application code, and usually gets less.

## Maintainability

- [ ] Names say what things are.
- [ ] Comments explain *why*, not *what*.
- [ ] No copy-paste that should be shared, no premature abstraction.
- [ ] Consistent with surrounding code.

## How to comment

- **Distinguish blocking from optional.** Prefix non-blocking notes with "nit:".
- **Ask rather than assert** when you might be missing context.
- **Explain the reasoning**, not just the correction.
- **Approve when it is good enough**, not when it is what you would have written.

## Before merging

- [ ] Conversations resolved.
- [ ] Required checks green.
- [ ] Commit history sensible — squash if the branch has "fix typo" noise.
- [ ] Branch deletes on merge.

---

Full lessons: https://moderngitacademy.com/github/pull-requests/