A code review checklist for human and AI-assisted review

A checklist does not make a reviewer careful, but it does make them consistent. The items below are the questions that catch most production defects. Use them as a prompt for your own review, adapt them into your team’s rules, and decide which ones a machine should handle so that people can spend their attention where judgment is needed.

Each section ends with what MergeHarbor does for that area, and the summary table at the end shows the split at a glance. Where MergeHarbor has no dedicated check, this guide says so.

Before you start

Read the description and any linked ticket first. You cannot judge whether code is correct without knowing what it is meant to do.

• What problem does this change solve, and is the change the smallest one that solves it?
• Is the pull request one logical change? Large mixed changes hide defects.
• Which parts are risky (authentication, payments, data deletion, concurrency) and which are routine (renames, docs, tests)? Spend your time accordingly.

MergeHarbor: checks the change against the acceptance criteria of linked issues, marking each one Met, Not met or Unclear with file and line evidence. It ranks each part of the diff by risk before the model reads it, so the riskiest code gets the most attention.

Correctness

• Does the code do what the description says, for every input, not just the example in the test?
• Are boundary values handled: empty collections, zero, negative numbers, the maximum value, null or None?
• Were any guards, checks or validations removed? A deleted if is easy to miss in a diff.
• If a function’s behaviour changed, do all of its callers still work? Look beyond the files in the diff.
• Are comparisons, units and time zones consistent (cents versus dollars, seconds versus milliseconds, local time versus UTC)?

MergeHarbor: this is the main job of the codebase-aware review. The reviewer follows callers of changed functions, reads related code and history, and a second pass tries to disprove each finding before anything is posted.

Security

• Is all external input validated before use? Are queries and shell commands built with parameters, not string concatenation?
• Are authorization checks present on every new endpoint or code path, not only authentication?
• Are secrets loaded from a secret manager rather than committed?
• Is TLS verification left on? Is deserialization of untrusted data avoided?
• Does any log line or error message expose personal data, tokens or internal details?

MergeHarbor: built-in deterministic checks run on every pull request at no cost and flag committed secrets, SQL and shell injection, XSS sinks, eval, disabled TLS verification and unsafe deserialization. Missing authorization and data exposure need judgment, so they fall to the model review and to you. You can add your own rules, such as “never log card numbers, even masked”.

Concurrency

• Is shared state protected? Is every lock released on every path, including errors and early returns?
• Can two requests race on the same record? Is there a read-then-write that should be a single atomic operation?
• Are retries idempotent? A retried payment or email should not happen twice.
• Are goroutines, threads or async tasks bounded and cancelled when their caller goes away?

MergeHarbor: changes that touch concurrency are ranked as risky and reviewed first. There is no dedicated deterministic check for races; the model review investigates them, and rules such as “every charge takes an idempotency key” make your expectations explicit.

Error handling

• Is every error either handled or returned with enough context to debug it? Are any errors silently swallowed?
• In async code, is every promise awaited or deliberately left to run?
• Do failures leave the system in a consistent state, or can a half-finished operation leave bad data behind?
• Are timeouts set on network calls?

MergeHarbor: missing await and empty catch blocks that swallow errors are deterministic checks. Error handling is one of the signals used to rank risk, and the model review looks for swallowed errors and inconsistent state.

Data and migrations

• Will the migration lock a large table? Index builds, type changes and new constraints can block writes.
• Does it drop or rename a column the previous release still reads? During a rolling deploy, both versions run at once.
• Is a backfill done in the same transaction as the schema change?
• Can the migration be rolled back? Was an existing migration edited after it ran?
• Is there a DELETE or UPDATE without a WHERE?

Here is a migration that looks routine but blocks writes on PostgreSQL while the index builds:

-- Blocks writes to orders until the build finishes.
CREATE INDEX idx_orders_customer ON orders (customer_id);

-- Safer: build without holding a write lock (outside a transaction).
CREATE INDEX CONCURRENTLY idx_orders_customer ON orders (customer_id);

MergeHarbor: migration rules flag blocking index builds, NOT NULL columns added without a default, in-place type changes, dropped and renamed columns and tables, backfills inside schema transactions, unbounded deletes, missing down steps and edited migrations. Each comment explains the safer way for your framework.

API contracts

• Does the change remove or rename an endpoint, field, message or enum value that clients use?
• Did a field change type, or an optional input become required?
• Are other services or repositories consuming this contract? Do their owners know?
• Is the change versioned, or is there a deprecation period?

MergeHarbor: when a pull request edits an OpenAPI, protobuf or GraphQL contract, MergeHarbor compares it with the base version and lists every change that breaks an existing client. If the repository belongs to a cluster of related repositories, the finding names the ones whose code uses the contract.

Tests

• Is new logic covered by tests that would fail if the logic were wrong?
• Do the tests check behaviour rather than implementation details?
• Are edge cases tested, not only the happy path?
• Were tests deleted or weakened to make the change pass?

MergeHarbor: test gaps show how much of the changed code your tests run, from an uploaded coverage report or an estimate with no setup, and comment on changed functions with real logic and no test. It can write the missing tests. With proof runs, MergeHarbor writes tests that try to reproduce its own findings and runs them in an isolated sandbox, attaching the failing test as evidence.

Performance

• Is there a query inside a loop? An N+1 pattern is cheap in a test and slow in production.
• Are new queries backed by an index? Is any result set unbounded?
• Is large data loaded into memory when it could be streamed?
• Does a hot path now do extra work, such as a network call or a lock?

MergeHarbor: queries are one of the risk signals, and the model review comments on clear performance defects. There is no dedicated performance check or profiler. Use rules for your known hot spots, and keep load testing in your pipeline.

Observability

• When this code fails in production, will you know? Is there a log line, metric or trace span at the point of failure?
• Do log messages carry the identifiers needed to follow one request?
• Are new alerts actionable, and do dashboards reflect the change?

MergeHarbor: no dedicated check. If your team has conventions, such as “every external call records a latency metric”, write them as rules scoped to the folders they apply to, and the reviewer will apply them.

Readability and design

• Will the next person understand this code without the pull request description?
• Does the change follow existing patterns in the codebase, or introduce a new one without reason?
• Is the abstraction right for where the product is going?

MergeHarbor: notes on naming, style and documentation are held back unless you set review strictness to high, because they are rarely what a team wants from an automated reviewer. Design and product fit are for people.

What MergeHarbor checks and what it leaves to you

The built-in security, migration and contract checks run without a model and use no credits. Change risk scoring also uses no credits.

Turn the checklist into rules

The items your team cares about most should not live only in a document. Write them as plain-English rules next to the code they govern, so they apply on every pull request:

# services/payments/.mergeharbor/config.yml
tier: deep
instructions: Amounts are integers in cents.
rules:
  - Every charge takes an idempotency key.
  - rule: Use the ledger package for balance changes
    paths: ["**/*.go"]
    severity: major

A .mergeharbor/ folder applies to everything below it, and configuration is read from the merge base, so a pull request cannot change its own review. See repository configuration for every key.

Further reading

• What is AI code review?
• Free security checks and change risk
• Merge gate: combine findings, tests and approvals into one verdict