Reviewing Pull Requests for Correctness

Establish intent and scope first

Read the pull request description, requirement, acceptance criteria, linked incident, and rollout plan before the diff. State what users or callers should observe and what must remain unchanged. Inspect the file list, generated changes, dependency updates, configuration, migrations, and tests. A small line count can still alter authentication, shared parsing, caching, or a public contract used throughout the product.

Follow the owning code rather than reviewing each line in isolation. Identify entry points, data stores, external calls, and outputs. Check nearby callers when a function signature or meaning changes. Search for assumptions the patch may violate, such as nullable fields, ordering, retries, tenant scope, timezone, or previous serialized values. Ask for missing context when the intended behavior cannot be determined from available product evidence.

Trace the consequential paths

Walk at least the normal path, expected rejection, dependency failure, and important boundary. For a write, follow validation, authorization, transaction behavior, side effects, response, and retry. For a read, inspect access scope, filters, ordering, absence, caching, and stale data. For asynchronous work, consider duplicate delivery, partial completion, cancellation, and whether retries can repeat an external effect.

Check invariants at the layer that can enforce them. Application checks alone may race before a database write, while a uniqueness constraint without a handled conflict may produce an unexpected server error. Inspect trust transitions carefully: client fields, headers, stored ownership, and server-generated identifiers have different authority. Ensure errors do not expose secrets and logs do not copy credentials or sensitive payloads.

Evaluate evidence, not test quantity

Map tests to the likely failures. Unit tests can prove rules and boundaries, integration tests can prove schemas and transactions, and end-to-end tests can prove critical wiring and browser behavior. Read assertions to confirm they would fail for the suspected defect. A new test that executes a branch but asserts only a status may miss incorrect data, cross-tenant access, or an unintended side effect.

Run the focused checks when practical and inspect failures rather than assuming the pipeline will explain them. Consider static types, linting, migration validation, contract checks, and production observability as additional evidence. Generated snapshots and fixtures deserve review like implementation code. If a risk cannot be tested cheaply, ask for a clear manual verification record or explain why a later environment supplies the required evidence.

Communicate findings precisely

A useful blocking comment describes the condition, current behavior, consequence, and required property. Include a minimal example when it removes ambiguity. Mark questions and optional improvements accordingly so the author can distinguish correctness work from style preference. Prefer established team conventions and automated formatting over repeated personal taste debates. Review the change, not the author's ability or motivation.

After revisions, verify that the actual risk is resolved and that the correction did not create another path. Resolve stale threads transparently and summarize any remaining rollout or follow-up concern. Approval means the reviewer found reasonable evidence for the stated scope, not that defects are impossible. High-risk changes may still need another owner, staged rollout, monitoring, or independent operational approval.

Review a project invitation endpoint

A pull request lets workspace administrators invite a person by email and sends a notification after inserting a record.

1. Confirm the intended permissions, privacy behavior, duplicate rule, expiration, notification requirement, and tenant boundary.
2. Trace normalized email input through account lookup, authority check, invitation insert, transaction, message request, and response.
3. Test concurrent duplicate requests and discover that both pass the preliminary lookup before either insert completes.
4. Request a database uniqueness rule plus handled conflict behavior, then check whether notification remains single and retry-safe.
5. Run the focused integration case and verify cross-tenant, unknown-email, expired-session, and provider-failure outcomes remain safe.

Correctness review worksheet

Use this sequence to concentrate review time on behavior and evidence.

• Intent: user outcome, acceptance criteria, preserved behavior, owner, deployment plan, and rollback constraints.
• Surface: entry points, callers, stored data, contracts, configuration, dependencies, generated files, and affected tenants.
• Paths: success, rejection, absence, boundary, dependency failure, retry, concurrency, cancellation, and partial completion.
• Trust and data: authentication, authorization, validation, invariant, transaction, privacy, logs, and compatibility.
• Evidence and response: tests, manual proof, monitoring, blocker, suggestion, question, revision check, and follow-up.

Common mistakes

• Reviewing only changed lines and missing a caller whose assumption the new return value violates.
• Approving because tests are green without checking whether their assertions cover the risky outcome.
• Blocking on personal naming or formatting preferences while leaving a concrete failure scenario unexplained.

Try one

Sources

• GitHub pull request review guidanceGitHub's documentation for review states, comments, and protected review workflows.
• GitHub required status checksGitHub's documentation for protected branches, required checks, and review controls.
• GitHub Actions documentationGitHub's primary guide to automated workflows, jobs, runners, and events.