Code Review Checklist

Check the PR's status checks tab — required builds, unit tests, lint, and type check. A reviewer who reviews against red CI is teaching the team to merge red. If a check is flaky, link the flake-tracking issue rather than re-running blindly.

Comment on the PR with the failing job name and assign back to the author. Don't continue the substantive review — feedback against a red branch fragments attention, and a regression often surfaces only once CI passes.

Compare the diff against the linked Jira or Linear ticket. Scope creep is the most common reason a 200-line PR turns into 1,200 lines mid-review — flag drift and ask the author to split rather than approving drift to avoid awkwardness.

Run the diff through eslint, rubocop, black, gofmt, or whatever the repo enforces. CI should catch this, but a local pass catches deltas between the author's editor and the project config. If formatting changes appear outside the substantive area, ask the author to split the formatting commit.

Confirm the diff follows the project's case convention — camelCase for JS/TS, snake_case for Python/Ruby, PascalCase for types and classes. Mixed conventions inside a single file are a smell that the author copied from another codebase without adapting.

Single-letter names are fine for tight loop indices (i, j) or established mathematical conventions; everywhere else they hide intent. Push back on names like data, obj, tmp, mgr — the engineer who has to grep for what mgr means six months from now will not thank you.

Every exported function, public class method, and HTTP handler should have a JSDoc or docstring block covering parameters, return shape, and error cases. Internal helpers usually don't need them — the test names are the documentation.

Reject comments that restate the code (// increment counter above counter++). Keep comments that name the constraint a reader can't see — the workaround for an upstream bug, the reason for a magic number, the known-bad alternative that was tried.

If the diff changes a public contract, a database schema, or a cross-service interface, the changelog and any relevant ADR must be updated in the same PR. ADR drift is how a new engineer six months in finds a 'documented' design that doesn't match the running system.

A class that loads config, talks to the DB, and formats output for the wire is three classes wearing one name. Suggest the split where it's mechanical; for ambiguous cases, leave a comment for the author to decide rather than blocking on architecture inside a feature PR.

Deep inheritance hierarchies are brittle — changing the base class ripples to every child. If the new class extends a base only to share two methods, suggest extracting those methods to a helper or a small interface and composing instead.

Dead code, commented-out blocks, and feature flags whose launch shipped last quarter all add diff noise. If the author added a flag, confirm there's a tracking ticket to clean it up; if there's an obsolete one nearby, ask them to delete it in the same PR.

An exception that says 'invalid input' tells the on-call engineer at 3am nothing. Errors should name what was invalid, what was expected, and ideally what the caller should do — 'expected non-null user_id, got null at handler/login.ts:42' is the bar.

Don't catch exceptions deep in business logic just to log and rethrow — that buries the stack trace and pollutes logs. Catch at the request boundary (HTTP handler, queue worker, CLI entry) where the error becomes a user-facing response; anything between throws bare.

Every log line in the request path should carry request_id and trace_id propagated from the inbound header (W3C traceparent or x-request-id). Without correlation, distributed tracing is a 30-minute manual grep. Confirm the new code uses the project's logger, not console.log or puts.

Open the test report and confirm new conditionals, error paths, and edge cases each have explicit assertions — not just the happy path. Coverage percentage alone is misleading; a single test asserting nothing can lift coverage to 100% without testing anything.

Even with gitleaks or GitGuardian in CI, reviewer eyes catch what scanners miss — hardcoded customer IDs, internal hostnames, .env files added to the diff. If anything sensitive is already in history, rotate the credential first; removing the commit doesn't remove it from forks and clones.

Dependabot and Renovate handle the routine bumps; new direct dependencies are the question. Capture whether this PR adds a new package to package.json, Gemfile, or requirements.txt — even one line of + "lodash.merge" matters for license and supply-chain reasons.

Run Snyk, GitHub Advanced Security, or npm audit against the new dep and confirm its license is compatible with the project — no GPL/AGPL pulled into a closed-source product, no Elastic/SSPL surprise. Pin to a specific version, not a floating range, and capture the SBOM impact if your team tracks it.

Record the decision and a short rationale. 'LGTM' on a 1,200-line diff is not a review; if you didn't read the diff, comment-only is the honest call. CODEOWNERS routes the formal approval — make sure the right reviewer signs off before merge.