Code Review Checklist

Open the linked Jira/Linear ticket and confirm the PR scope matches what was agreed during refinement. PRs that drift from the ticket — added refactors, opportunistic cleanups, scope creep — are the most common source of review friction. Flag scope mismatches in a top-level review comment before going line-by-line.

Don't review only in the GitHub/GitLab diff view for non-trivial PRs. Pull the branch, run it, exercise the changed code path. The diff hides cross-file context; running it locally surfaces issues like broken local dev setup, missing env vars, or unrunnable tests.

Don't review a red build. If a check is failing, ask the author to fix or explicitly flag it as a known flake before continuing. "Just rerun it" becomes a habit that masks real regressions.

Review quality drops sharply on PRs over ~400 lines of net diff. Categorize before you start reading; if it's oversized, request a split rather than skim-approving with "LGTM".

Suggest a split along natural seams: refactor before behavior change, infra/config before app code, schema migration before consuming code. Pause review until the split lands; reviewing a 1,500-line PR reliably means rubber-stamping it.

If your toolchain has ESLint, Prettier, RuboCop, gofmt, ruff, or equivalent — these should already run in CI. Spot-check that the configured rules actually ran on the changed files (no skipped files via .eslintignore or //eslint-disable). Style nits are for the linter, not the reviewer.

Flag ambiguous variable names (data, tmp, handler2), unused imports, commented-out code blocks, and stray console.log / puts / print debug statements. These are cheap to fix now and expensive to clean up later.

Comments should explain why, not what — the code shows the what. If a regex, recursive call, or non-obvious algorithm has no comment, ask for one. Bonus: a TODO/FIXME with a ticket link is acceptable; a TODO with no owner is not.

For each new branch in the code, look for a corresponding test case. Coverage percentage alone is misleading — a 90% line-coverage PR can still skip the actual conditional logic. Read the test names and assertions, not just the coverage delta.

If the PR changes an HTTP endpoint, gRPC method, or message contract, an integration test should exercise the request/response shape end-to-end. Unit tests with mocked clients miss serialization bugs and middleware ordering issues.

Common gaps: empty input, null/None, max-length input, unicode/emoji, concurrent calls, network timeout, partial failure, retry behavior. Ask: what happens if the downstream call returns 500? Is the error logged with enough context for an on-call to triage at 3am?

Does this change belong in this service? PRs that introduce cross-service coupling (a billing concept leaking into the auth service, a UI module reaching into a backend repo) are architecture smells. If unsure, loop in the service owner via CODEOWNERS or a tech lead.

Grep the codebase for the new utility's name, signature, or core logic before approving. Reinvented date parsers, retry helpers, and HTTP client wrappers proliferate when reviewers don't ask. If a similar helper exists, push back unless the author has a specific reason not to use it.

Watch for N+1 queries on list endpoints, unbounded loops over user input, in-memory aggregations that will OOM at 10x scale, and god-classes that violate single responsibility. "It works for our current data size" is not a passing answer.

Look for files under db/migrate, migrations/, Alembic, Flyway, golang-migrate, or schema.rb/schema.prisma changes. A schema change deserves its own review pass — these are the highest-blast-radius changes in the diff.

On Postgres, ADD COLUMN with a non-null default rewrites the table under exclusive lock — fine on 10K rows, catastrophic on 50M. Require CREATE INDEX CONCURRENTLY, batched backfills, and a documented rollback. Confirm the migration is reversible or that an explicit forward-only decision is captured in the PR description.

API keys, database URLs, JWT secrets, AWS access keys, internal hostnames in test fixtures. GitHub secret scanning + gitleaks/trufflehog in pre-commit catch most cases, but a human pass on the diff is the last line of defense. If a secret lands on main, rotation alone doesn't fix it — the value lives in git history forever without filter-repo or BFG.

Every new endpoint needs an explicit authorization check, not just authentication. Confirm the user can only access resources they own (IDOR protection), and that input is validated before hitting the database (SQL injection, command injection, OWASP Top 10). "The frontend prevents this" is not an authorization model.

Check package.json / Gemfile / go.mod / pyproject.toml diff for new entries. Each new dep adds maintenance surface, license obligations, and supply-chain risk. Worth scrutiny even if it's a one-line install.

Run Snyk, npm audit, bundler-audit, or your SCA tool against the new dep tree. Verify the license is compatible with the product (no AGPL surprises in a closed-source SaaS, no SSPL gotchas). A package with 200 weekly downloads, no recent commits, and a single maintainer is a supply-chain liability — push back.

Use the platform's review action — Approve, Request changes, or Comment. Don't approve with unresolved blocking comments; either resolve them or use "Request changes" so the PR can't be merged. Reviewer notes capture context (what you focused on, what you skipped) for the audit trail and for the next reviewer if approval is delegated.

Confirm squash vs. rebase vs. merge-commit matches the repo's branch protection rules. Confirm CODEOWNERS approvals are satisfied. If the commit message will become the squash message, make sure it's not "fix typo (#1234)" — clean it up before merge so the main-branch history stays readable.

When the decision is "Request changes", capture the blocking items as PR comments and any non-blocking items as tickets in Jira/Linear linked from the PR. Avoid the pattern where deferred feedback lives only in PR threads — those become unsearchable once the PR is closed.