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.

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.

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.

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.

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.

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.