What a useful written pull request review contains, and what it cannot tell you

Start with the standard you are applying

A review needs a standard, or it turns into a list of the reviewer's tastes. Google's published engineering practices give a workable one: favour approving a change once it definitely improves the overall health of the code, even if it is not perfect, because there is no perfect code, only better code. The same guidance says technical facts and data overrule personal preference, and that a minor or educational point can be marked as a nit, which tells the author they may skip it. Decide before you start which kind of reader the review serves: a team deciding whether to merge, or a buyer deciding whether to accept outside work. The findings are written differently for each.

• State the standard at the top of the review so the reader knows what a finding means.
• Mark optional points as nits and keep them out of the list that must be fixed.

What to look at in a change

Google's list is a good checklist. Look at the design and whether the change belongs where it is. Check functionality, including edge cases and concurrency. Look at complexity, tests, naming, comments, style, consistency with the surrounding code and whether the documentation needs to change. Read every line a person wrote, and look at the change in the context of the whole file and system. Tests deserve their own look, since tests do not test themselves: check that they would fail if the behaviour were wrong, not just that they exist. A change that affects how people build, test, use or release the software should update the documentation in the same change.

• Ask what the change is supposed to do and compare it with what it does.
• Ask what input would break it, and check whether any test would notice.

A finding someone can act on

A finding is useful when the reader can find it, understand it and check it without asking the reviewer what was meant. State the file and line, what is wrong or risky, why it matters, how serious it is and how to verify it. Where you can show a defect with a failing test or a traced input, show it. Where you cannot, say the finding is suspected, so nobody mistakes a hunch for a fact. Order findings by severity, not by the order of the files. Record what was good as well, because the same guidance says to give encouragement for good practice, and because it shows the author what to keep.

• Give every finding five parts: place, problem, reason, severity and verification.
• Label each as shown or suspected.
• Finish with a list of what you did not examine.

What a review cannot tell you, and how the paid job is accepted

GitHub offers three kinds of review on a pull request: Comment, Approve, which signals that the changes are ready to merge, and Request changes. A written review from outside the team informs a decision and is not an approval. It says what was found in the areas examined; it cannot say that the change is safe to run in production, and it is not a security audit. The written-findings job accepts a review on four tests: each finding has the five parts, defects are shown or labelled suspected, a not-examined list exists for each change, and your own reviewer can follow three findings of their choice using only what is written. Nothing in the review approves a merge. Send the number of pull requests, their rough size and what each should achieve; do not send code.