Quality and Security

AI-Assisted Code Review: From a Diff to a Verified Decision

How a human or an AI reviewer reads a proposed change before merge, proves a defect with a reproducible finding, withdraws a suspicion that did not hold…

Chapter 50Quality and Security

Testing Fundamentals and Test Strategy covered how to choose the checks for a change. This unit looks at how to read the change itself before it is merged: what exactly a human or an AI reviewer should check, how to prove a defect they have found, and what to do when a suspicion turns out to be unfounded.

Code review is the examination of a proposed change in the context of its requirements and of the system. Its purpose is to reduce the risk of a defect and to make the code that has to be maintained easier to understand. A review is not a vote on personal style, and it is not measured by the number of comments. Google Engineering Practices states a similar criterion: approve a change as soon as it clearly improves the overall health of the system; a minor imperfection should not become a reason for delay. This is Google's guideline, not a universal rule for every risk or every team. The Standard of Code Review

A review should help answer practical questions: is the required behaviour implemented; has a contract that is already supported been broken; are the boundaries of access and data respected; do the tests check the behaviour that matters; is it clear what has to happen next? The depth of a review depends on the possible impact of the change: a short text diff and a change to how encryption keys are handled do not need the same route.

First, pin down exactly what you are reviewing

A review answers a question about a specific revision. Record the base revision and the current commit (the head), and then read the diff between exactly those two. If the author has already added a fix, an earlier conclusion may be out of date; if nobody knows which commit was reviewed, the conclusion is hard to reproduce.

Before the review, gather:

  • the task, the requirements and the acceptance criteria;
  • the description of the pull request or merge request (PR/MR) and the list of changes it claims to make;
  • the diff and its base revision;
  • related modules, types, application programming interface (API) contracts, the data schema and architecture decision records (ADRs), if the change touches them;
  • the relevant tests, the results of checks, and how to run them;
  • important constraints of the environment: for example, whether the agent can see the right branch, the dependencies, the logs or the test data.

The request description helps you understand the intent, but it does not replace primary evidence. Check the diff itself, and the contract or code path the conclusion relies on. A PR may describe the behaviour incompletely, and an agent's request may contain an outdated assumption. GitHub lets you leave comments both on the PR as a whole and on specific lines; choose the place for a discussion to match the scale of the question. Pull request reviews, Working with comments

If critical context is missing, say so. A review may end with the conclusion "insufficient evidence" rather than invented certainty. For example: "I cannot confirm migration compatibility: the available checkout has neither the previous version of the schema nor an upgrade test. I need these artefacts or a separate check result." That is more specific than silently assuming the migration is safe.

Follow the change's path to the user-facing result

Start from the requirement and follow the path to the result: the incoming request, the validation, the change of state, the dependency, the response or the side effect. Where useful, walk it backwards — from the place where the user will see the result to the code that produces it. Check not only the added lines but also the nearby code that determines what they actually mean.

Useful questions:

  • Exactly which scenario has changed for the user or for another system?
  • Is the new path called in every place it is needed, and are the old scenarios that must keep working preserved?
  • Are the arguments passed in the right order and of the right type? Are all callers of the updated function compatible?
  • Has the order of transactions, event publication, retries or resource clean-up changed?
  • Does the new behaviour match the API, the schema, the protocol and the consumers' expectations?
  • Which unchanged files or dependencies form the preconditions of this code?

A condition that is plausible locally can break a global invariant. For example, a function correctly checks the format of the owner, but the route calls it before a trusted identity has been established; a loop correctly updates one record, but a retry sends the event twice; a new argument is mandatory in the signature, but an internal call still uses the old number of arguments.

Check edge cases and error scenarios where they are relevant to the change: an empty value, a missing record, a wrong type, a duplicated request, an exhausted timeout, a retry, a concurrent request, a partially completed operation, a cancellation, or a failure of a dependency. There is no need to walk mechanically through a long list for every line. It is enough to formulate a real counterexample and check whether the code or a test rejects it.

The phrase "there might be a race here" is not, on its own, a finding. Show the two operations, their sequence and the shared resource: "Two requests read the free slot before either writes. Each creates a booking, because the availability check and the insert are not atomic. Run both requests with a barrier before commit; the result must contain no more than one confirmation." A description like this gives a path to verification instead of leaving the author with an abstract worry.

Check conformance to requirements and the quality of the test evidence

A requirement has to be matched against behaviour, not merely against the presence of new code. For each acceptance criterion, find where it is enforced and how a deviation would be noticed. If the requirement forbids a user from reading someone else's invoice, a test of the owner reading their own invoice does not prove the requirement is met. You need a separate owner and an outside user, and the expected result must include a refusal and the absence of private data in the response.

A test is strong evidence when its expected result comes from the requirement, independently of the code under test. Make sure the test:

  • checks a result that matters, not just a call to a mock function or a number of covered lines;
  • will fail if the defect in question actually appears;
  • goes through the important system boundaries if those are what create the risk;
  • has specific preconditions, data and a way to reproduce it;
  • was run on the same revision that is being reviewed now.

A mock is useful for isolating a contract, but it does not confirm the behaviour of the real provider, database or queue unless it checks their integration. A test that copies a condition from the production code as its own expectation can repeat the same mistake. Check the independent source of the expected result: an explicit requirement, a documented contract, a recorded invariant or a verified data model. Google advises considering whether the test will fail when the code is broken, and whether the change really has the appropriate tests; the mere existence of a test file does not answer these questions. What to Look for in a Code Review

A green status for the build, the tests or static analysis is an important signal, but it proves only that those particular checks succeeded on a particular revision. Compare the commit and the list of required checks; if a job was skipped or a different commit was checked, write that down. GitHub's policy settings define which checks and approvals are required for a particular branch. Passing continuous integration (CI) does not cancel a confirmed defect, and a human approval does not turn incomplete CI into a pass. Status checks, About protected branches

Assess access, data and compatibility along a specific path

For a change that reads or modifies data, find out who can invoke the path and exactly which entity it touches. Check the owner of the record, the role, the organisation or tenant, delegation and the trust boundary — whichever of these dimensions exist in the product. A check that "the user is signed in" does not prove that "the user has the right to this object": the Open Worldwide Application Security Project (OWASP) calls the failure to check that right on an API object Broken Object Level Authorization (BOLA) and recommends checking permissions for the specific action on the specific object. Make sure an error response does not return fields that were supposed to be hidden. About OWASP, OWASP API Security Top 10, API1:2023

In changes to storage, logging, export or migration, look at where secrets and private data appear: in logs, exceptions, test dumps, telemetry, temporary files, backups or new permissions. For a migration, check the direction of the upgrade, the compatibility of old and new code, rerunning, partial failure and the possibility of recovery. An irreversible action should be considered with its actual constraints, not called safe merely because the test suite is green.

For APIs and dependencies, check more than whether the new branch compiles. Compatibility concerns clients that still use the old request shape or the old schema version; the order of events that consumers rely on; time limits; and the actual behaviour of the library in this configuration. If the risk sits at the boundary with an external system, state whether a contract test, an integration run, or only a local simulation was carried out.

This is not a reason to paste the full list of known attack categories into every review. Start from the specific change and its path to impact: which data, who controls it, where the check disappears, how it could be exploited or how the system could fail. If a full threat-modelling analysis is needed, it is a separate task with its own scope and evidence.

Write a finding that can be reproduced

A useful finding reports a specific risk, not merely disagreement with the shape of the code. The format below is a practical synthesis for this unit; neither Google Engineering Practices nor GitHub requires this particular template.

  1. Location and revision. The file, lines or path in the changed diff; the commit that was reviewed.
  2. Reproduction condition. Who has to do what; preconditions, data, order of events or type of error.
  3. Expected and actual. What should happen according to the requirement, and what happens instead.
  4. Impact. Who loses data, access or a function, and under what conditions; how serious the consequences are and how likely the scenario is.
  5. Evidence and confidence. A reference to the contract, stack trace, test, code path or reproduction; what remains an assumption.
  6. Smallest useful fix. The check, the change or the additional information that will close the identified risk.

A short example: "[P1, blocking] booking.py:84, commit abc123. When two users book the last place before the first request commits, both pass the free-slot check and both receive a confirmation. This violates the requirement of one booking per place. Add a concurrent test with two requests that start before the first commit, and enforce an atomic constraint on the write. Verified along the transactional path; reproducing with two parallel requests produces two rows."

Keep a claim, a hypothesis and an idea apart. "The server returns someone else's phone number on the GET /users/{id} path without checking the owner" is a claim, and it should come with a reproduction. "The logger might be writing the password" is a hypothesis until the logging level and the shape of the exception have been checked. "The variable could be renamed" is an optional idea, if the name does not harm understanding or the contract. Do not promote a hypothesis to a critical finding just because the consequence could be severe; a serious possible impact is a reason to check the path quickly and to explain the uncertainty clearly.

Choose a priority without exaggerating

Use the repository's own convention first: P0–P3 prefixes, risk levels, or the words blocking, non-blocking, nit. If a team needs a compact scheme, one convenient option is this:

Label Meaning for this review
P0 / critical A confirmed risk to security, data integrity or availability with high impact; requires an immediate response according to the team's process.
P1 / high A confirmed defect in the main scenario, access rights, compatibility or reliability; fix before merge.
P2 / moderate A real problem, but its impact or conditions are limited; whether it blocks depends on the consequences and on local policy.
P3 / suggestion An improvement to readability or future maintenance without a confirmed defect; do not disguise it as a mandatory fix.

This is an example of a local scale, not a GitHub standard and not Google's scale. Not every team needs four numbers. What matters is saying what is mandatory before merge, what can be discussed afterwards, and what is optional advice. Google uses labels such as Nit, Optional and FYI to distinguish a required action from a suggestion, but does not define this P0–P3 scheme. How to Write Code Review Comments

Decide the outcome of the review, then check the new revision

After reviewing, state one of three decisions:

  • Approve. The reviewed revision meets the important requirements and contains no confirmed blocking risks. Remaining suggestions are marked as optional.
  • Request changes. There are one or more confirmed findings that, under the agreed rule, must be resolved before merge.
  • Insufficient evidence. No well-founded conclusion is possible because of missing context, an incomplete run or an unknown contract. Name the evidence that is needed and who can provide it.

When a suspicion has not been confirmed, withdraw it explicitly: "I checked the transaction and the unique constraint on slot_id. Two concurrent inserts cannot both succeed; my comment about a duplicate booking was wrong. Withdrawing it." Deleting the comment without explanation leaves the reader guessing whether the code changed or the finding simply dropped out of sight.

After fixes, reread the changed lines and the related paths on the new head. The new code may have solved the original problem but introduced a regression, changed the behaviour of an adjacent branch, or been left without the test it needs. Record which revision was reviewed, and close the discussion with a link to the fix or the evidence. An approval of an old revision may become void after a substantial new commit — depending on branch protection, whether dismissal of stale approvals is configured, and the rule about the most recent push. Check the repository's rules rather than assuming the same behaviour everywhere. About protected branches, Resolving reviews

No findings means that the reviewer has no confirmed problem within the reviewed revision, the context and the checks available. It is not proof that there are no errors. If a path was not run or data was missing, narrow the wording: "On commit abc123 I found no errors against the requirements I checked; I did not run concurrent load."

How to use an AI reviewer

AI is convenient when it has been given a clear scope for the review and the necessary context is available to it. It can quickly cross-reference large areas of a diff, find callers of a changed interface, walk through edge states or propose a counterexample. Treat this as a prompt for verification, not as proof of infallibility or complete coverage.

Before running it, record:

  • which branch, commit or uncommitted change should be reviewed;
  • the requirement, the task boundaries and the contracts that must not be changed silently;
  • the files and environments the agent actually has access to;
  • whether it is allowed to run commands and tests, and whether the network and integration dependencies are available.

Ask the agent, for each finding, to show the path from condition to consequence, the exact lines and commit, the evidence or reproduction, its level of confidence, and an option for the smallest check. Separately, ask it not to report runs that did not happen and not to draw conclusions from files it does not have. If it cannot read CI, private documentation or the schema, that limitation must remain visible.

A second AI reviewer, or parallel agents, can add an independent angle, but they do not prove independence, correctness or completeness. Models can share the same mistaken assumption or repeat the same pattern. Check the code path, the official contract and the actual test result. In the current Codex documentation, /review works on a selected diff and returns prioritised comments; whether information about a GitHub PR is visible depends on access, the active branch and a configured gh. Anthropic's help article, as of 2 September 2026, describes Claude Code Review as a research preview for Team and Enterprise plans, unavailable to organisations with zero data retention (ZDR); the review leaves comments but does not approve or block the PR. Product availability, permissions and capabilities change, so check the current documentation and settings at the time you do the work. Codex code review, Set up Code Review for Claude Code

In its blog post launching Code Review for Claude Code, Anthropic gave its own internal figures for the share of PRs that received substantive comments and for comments marked "incorrect". This is a vendor's report on its own product and process, not an independent assessment of precision, recall or results for another team. Do not carry these numbers over as a forecast for your own repository. Bringing Code Review to Claude Code

Case study of an open PR: when review finds the fix's own regression

In July 2026, the author of the open PR openai/openai-python #3510 proposed that, after the [DONE] event, the client should read the remainder of the Server-Sent Events (SSE) stream so that the Hypertext Transfer Protocol (HTTP) connection would be released correctly. The initial explanation set out a logical path to the benefit: the server had already signalled completion, but the buffer might still contain the terminator of the chunked response.

The first Codex review found that the draining code sat in a finally block, so it ran even when the user deliberately stopped iterating before [DONE]. For a long or stalled stream, this turned closing into waiting for the rest of the network body. The author restricted the drain to the path on which [DONE] had already been seen, and applied the change to both the synchronous and asynchronous iterators. PR #3510, the first Codex comment and commit c789ceb

The next review found a different defect in the fix itself: if a transport error occurred after a correct [DONE], an exception from the finally block could turn a completed result into an error and never reach response.close(). The author called this a real bug in their own change, restricted the handling to the transport exception, ensured the connection was closed, added synchronous and asynchronous regression tests, and described the before/after difference. Later it turned out that cancellation while waiting for the remaining bytes could also skip closing; commit 04fdd7d wrapped the drain so that the connection would be closed on that path too. This cycle shows well why a review checks not only the original diff but also the fixes. PR #3510, the author's reply and commit f24d09a, commit 04fdd7d

A further review noticed that the limit on the number of parsed SSE events does not stop the draining if, after [DONE], the server keeps the connection open and sends only comment lines. These do not become events, although the decoder keeps consuming raw bytes. The author capped the drain at 64 KiB of raw data and added a test with comment-only chunks: checking the byte stream specifically is necessary because comments never hand control back to the event loop. The limit is checked after a chunk has been consumed, so the last chunk may slightly exceed the nominal 64 KiB. The author also noted that a single read that hangs with no new bytes is, as before, bounded by the configured HTTPX read timeout. PR #3510, discussion of the drain limit, commit c8e39cbc4a

After that, the author did not accept a separate suggestion about an idle socket, explaining how the HTTPX read timeout affects the configuration. In September a maintainer closed the PR without merging it: the team preferred to solve connection reuse at the transport layer, where there is less risk of tying clean-up to a timeout or a cancellation. So this story is not a simple "AI found a bug, the PR was merged". It contains several reproduced regressions and tests, a reasoned disagreement about the boundary of the fix, and a final decision not to accept this patch in this form. PR #3510, the author's reply about the timeout and the maintainer's decision of 23 September 2026

The practical lesson: require a counterexample for any substantial comment; after a fix, walk the related path again and check for the fix's own side effects; when a recommendation is not accepted, record the conditions and consequences of that choice. A test should prove the required invariant, not merely mirror the shape of the proposed code.

Teaching example: a user changes someone else's booking

A PR adds the ability to change a booking's date through PATCH /api/bookings/{booking_id}. The acceptance criterion says that a client may change only their own bookings within their own organisation. The form hides the edit button for someone else's booking, and a positive test checks that the owner can change the date.

The reviewer follows the request from the route to the database write and sees that the server looks up the booking by booking_id alone; there is an authentication check, but the link to principal.user_id and principal.tenant_id is not checked. The interface is not an access boundary: an outside user can call the API directly.

A complete comment might look like this:

[P1, blocking] bookings.py:84, commit abc123. Any authenticated user who knows or guesses a booking_id can send PATCH /api/bookings/{booking_id} and change the date of another client's booking. The route checks that the user is signed in, but loads the record by id alone and does not check the owner or the organisation. This could wreck someone else's appointment. Add a server-side access check in the query or in a policy. A negative API test should sign in as user B, try to change user A's booking, receive a refusal according to the policy, leave the date unchanged and not see the record's contents. Keep the owner test as a positive control.

Before posting such a comment, the reviewer checks the actual service call and the source of principal, rather than assuming the behaviour from the name update_booking. They also look for a check in middleware, in an object-relational mapping (ORM) filter or in the database query: it may live outside the changed line. If the invariant applies at this boundary and there is no server-side check, the decision is request changes, because this is a confirmed authorisation bug on the changed route. If the reviewer does find a check, the test should exercise the real access boundary with two different users and show the refusal for the outside record.

This is a synthetic teaching PR; the path, lines and commit are illustrative, and no code was run here. It shows how to formulate a specific condition, a path to impact, a negative test and a decision. The fact that a user is signed in to the system does not, by itself, prove their right to a specific object.

Teaching example: a suspicion refuted by a database constraint

In a PR, the booking service checks whether a slot is free and then inserts a record. The reviewer sees two parallel requests and writes: "Both can pass the check before inserting; users will get two bookings for one place. Blocks merge."

This is a plausible suspicion, but the conclusion is not yet confirmed. Another reviewer checks the transaction, the migration and the UNIQUE(slot_id) constraint on the table of active bookings. The database enforces uniqueness atomically; one of the two concurrent writes ends in a conflict, which is turned into a "slot already taken" response. The condition "two successful writes for one slot" is impossible in the stated schema.

The reviewer tries the negative scenario in a database-level test, or checks an existing concurrent test with two clients. If the test really does confirm one success and one controlled conflict, the reviewer withdraws the blocking finding: "I had not taken the unique constraint on slot_id into account; the inserts are serialised at the database level, and no duplicate active record arises. Withdrawing the comment. The test test_concurrent_booking_conflicts confirms the expected result." If there is no such test, the appropriate new suggestion is to add one, not to claim a confirmed duplicate.

The constraint proves the invariant only in the schema and conditions where it applies. If a booking is archived before the check, if another record type has no such constraint, or if the migration has not yet been deployed to every environment, the original suspicion may remain valid for a different path. Check the actual index definition, the transaction boundary and the conflict handling. Do not accept a function name like create_unique_booking as proof of uniqueness.

This is a synthetic teaching example; it does not describe a real PR or a test run here. Its aim is to show how to tell a possible defect from a confirmed one, and to withdraw a wrong comment as explicitly as an accepted one.

Teaching example: do not let a fix grow without a decision

In a PR fixing the ownership check on invoices, the author changes the access condition and adds a negative test: user B cannot read user A's invoice. The review confirmed the narrow path, and the author was asked to fix only that.

In the new revision, the author has also rewritten the general-purpose database client, renamed parameters in eight APIs, reformatted neighbouring modules and changed the standard for handling timeouts. These changes may be useful, but they increase the scope without any connection to the access fix. The ownership test now does not prove that the contracts of other routes have been preserved or that all clients are compatible.

The reviewer should check the diff since the previous commit and name the concrete consequence of the scope: "After the access fix, the PR adds a change to the error format across all routes via the new ORM client. This changes the observable API and is not verified by the access criterion. Please move the refactoring into a separate PR, or add contracts and tests for all the changed responses; the existing approval applied to a different diff." This is a request to bring the review back to a verifiable boundary, not a claim that any adjacent refactoring is forbidden.

If the team decides to keep the expansion, the scope of the requirements must expand too: a list of consumers, contract tests, client migration and a justified rollout plan. If the broader task has no owner or evidence, it is better to split it off and record a link to a separate task. Once the choice is made, the reviewer reviews the new revision in full, within its new scope.

This scenario is also a teaching example. It shows that the review is responsible for the whole proposed diff: additional code becomes part of the solution even if the original request was narrow.

Tone and timing of feedback

Write about the code and its consequences, not about the author's abilities or intentions. Explain why the problem matters, propose a direct next step and mark whether it is mandatory. "You forgot about the second user" blames a person; "This test checks the owner, but does not reject a second user's access to the same booking_id" names a gap that can be verified. Google recommends specific, courteous comments that explain the reasoning and leave room for discussion when confidence is incomplete. How to Write Code Review Comments

A clarifying question is useful when the answer could change the conclusion: "Is the account_id from the token guaranteed to be the same owner as the URL parameter?" Questions without explanation can feel like riddles. If a comment only suggests a direction, label it Optional, Nit or with a local label. Do not add dozens of repeated minor comments if a single general comment and a linter deal with the same fact.

Fast feedback reduces the time spent waiting for a fix, but reviewing itself requires focus. Google advises responding promptly and gives one working day as its own maximum guideline, while also advising reviewers not to interrupt deep work in the middle of a focused task. This is an example of Google's practice, not a requirement for every team. Your own service level should take into account time zones, criticality, the size of the diff and the on-call schedule. Speed of Code Reviews

Accelerate treats fast feedback, small batches of change and approval processes as capabilities of the whole software-delivery organisation. In the capability model presented there, higher delivery performance was found both in teams with no separate change-approval process and in teams that used peer review. This is a result at the level of the organisation, not proof that an individual PR can be merged without review or that mandatory branch rules should be removed. For a team, it raises a question instead: does the queue reduce real uncertainty and risk, or does it only add unnecessary waiting? Measure the time to the first useful response and to the completion of the review, while keeping the level of control that the actual impact of the change requires. Accelerate, Forsgren, Humble & Kim, 2018, authorised excerpt, Appendix A

In the third edition of Peopleware, Tom DeMarco and Tim Lister discuss time for focused work, distractions and the telephone as part of how team work is organised. The book provides context for respecting colleagues' attention; it is a practical source, not a measurement of the cost of each code review and not a recipe for a calendar. For reviewer behaviour, rely on the team's specific agreement, not on a slogan about urgency. Peopleware: Productive Projects and Teams, 3rd ed., excerpt "Bring Back the Door"

The finished review result

A good review summary briefly gives the author and the team the grounds for a decision:

  • which revision was reviewed;
  • which requirements and parts of the system were covered;
  • which checks were run, on which commit, and with what result;
  • which findings are confirmed, with their priority and reproduction condition;
  • which comments were withdrawn or left as optional;
  • whether there are limitations or additional evidence is needed;
  • whether the change was approved, changes were requested, or the decision was deferred.

In this way, comments become traceable evidence rather than a list of impressions. A review is complete when the team can explain why this revision meets the requirements and how it dealt with the known risks — even if the decision was to split off part of the work or to stop a particular patch.

Sources and further reading

The sources were checked on 24 September 2026. GitHub documentation and product help pages change; before using a specific process, check the current repository settings and the version of the feature.