The Semantics of a Code Review Approval

A review tool records approvals as booleans attached to a change. That representation is much poorer than the statement an approver actually makes, and the gap causes a specific class of failure: a change is merged on the strength of an approval that no longer applies to what is being merged. Understanding what an approval can and cannot mean requires separating the claims it bundles, because those claims have different lifetimes and different evidence requirements.

An approval bundles at least four claims

When a reviewer clicks approve, the recorded fact is one bit. The intended meaning is a conjunction, and the parts are worth separating because each fails independently.

The revision claim. The approver read a specific version of the change. This is a claim about an object, and objects change. If the change is amended, rebased, or has commits appended, the revision the approver read is no longer the revision being merged.

The comprehension claim. The approver understood what the change does. This is the weakest link in practice, because it is unverifiable from outside and because the failure mode is silent: an approver who misread the intent still approves, and their approval is indistinguishable in the record from one given with full understanding.

The defect claim. The approver found no defect of the classes they are capable of detecting. Note the qualifier. This is not “there are no defects”; it is “this reader, with this context, did not find one”, which depends on the reader and the diff rather than on the code.

The adequacy claim. The approver believes the change is appropriate to merge: it does what the project wants, in the way the project wants it, at the time it is being merged. This is a judgement about direction, not about correctness, and it is the claim most likely to be made by a different person than the one making the defect claim.

A single bit cannot represent a conjunction of four claims with different lifetimes. Review systems that try to do so end up encoding the revision claim implicitly, since an approval attaches to a diff position and positions are computed against a base revision, while silently discarding the other three. The result is a system that can answer “was this approved” and cannot answer “approved by whom, for what, and does that still hold”.

Revision pinning and the staleness problem

The mechanical fix for the revision claim is to record approvals against a content hash rather than a position. A patch set has an identity; an approval names that identity; when the identity changes, the approval is marked stale. This is what makes a force-push visible in the review record: the new patch set is a different object, and the approvals attached to the old one do not carry over.

The interesting question is what should happen to a stale approval. Three policies are in use, and they trade review cost against review safety.

Discarding all approvals on any new patch set is the strictest. It is also the most wasteful, because the common case is a patch set that fixes one review comment and touches nothing else, and re-review of the whole change to validate a two-line fix is not a productive use of the second reviewer. In practice strict discard pushes authors toward not amending, appending fixup commits instead, which produces a history of small noisy commits and defers the cleanup to merge time, where it is often skipped.

Carrying approvals forward unconditionally is the loosest, and it is unsafe in a specific way: the diff between patch sets can contain anything, including changes the approver would have objected to. A system that carries approvals forward without showing the approver what changed since their review has converted their approval into a claim about a revision they never saw.

The middle policy, and the one that matches how reviewers actually work, is to carry approval forward only when the new patch set is a descendant of the approved one and to surface the incremental diff to the approver. The approver then reviews only the delta, which is exactly the amount of new material. This works because the delta is usually small, and it fails when the delta is a rebase that rewrote every hunk. In that case the incremental diff is not smaller than the whole change, and the tooling should say so rather than pretend the review is incremental.

Rebase is the hard case

A rebase against a moved base can change the diff without changing the author’s intent. If the base gained a function with the same name the change introduces, the rebase produces a conflict; resolving it produces new code that no one has reviewed. If the base changed a function signature the change calls, the rebase produces a call site that no longer matches, and whether the build catches it depends on the language and on whether the call is typed.

The review-relevant fact is that a rebase is an edit performed by a merge tool or by the author under time pressure, and its output is unreviewed. Approvals carried across a rebase therefore vouch for code that was written during the rebase, which is precisely the code most likely to be wrong, because conflict resolution is done quickly and without the surrounding reasoning that produced the original change.

Two practices follow. Resolving a conflict is a change to the change and deserves a review comment or at minimum a note in the change description saying which hunks were resolved by hand. And the pre-merge check should verify that the merged tree is what was reviewed, which means verifying the merge result rather than the branch tip, a distinction that matters whenever the base has moved, because the branch tip was tested against a base that no longer exists.

Approval as a claim about the merge result

This is the deepest structural problem in review sign-off. A reviewer approves a diff computed against base $B$. The merge lands the change on base $B’$. The merged tree is not the tree the reviewer saw, and the difference is not only the base’s own changes — it is the interaction between the change and those changes.

The interaction can be a semantic conflict with no textual conflict. Two changes each add a case to a switch statement in different places; the merge combines both cleanly and produces a switch that falls through where neither intended. Two changes each register a handler with the same name; the merge compiles and one silently shadows the other. Text-level merge algorithms operate on lines and cannot see either problem.

The consequence for approval semantics is that the strongest statement a reviewer can make is about a diff, and the statement the merge actually requires is about a tree. Closing that gap requires evaluating the change against the base it will land on, which is the function of a merge queue rather than of a reviewer. The relationship is described with the queue mechanics: the queue exists because approval is about a diff and merging is about a tree.

Review state as a finite automaton

Review tools typically model the review as a small state machine: draft, in review, approved, merged. The states are coarse and the transitions are per-person, and the coarse model is where several process pathologies originate.

One pathology is approval accumulation. If a policy requires two approvals, and approvals attach to revisions, then the count is only meaningful if both approvals are current. A system that displays “2 of 2 approvals” without indicating which revisions they name permits a merge on the strength of one current approval and one that predates a substantial rewrite.

Another is the dismissible review. A reviewer who requests changes blocks the change until the request is resolved or dismissed. If dismissal is available to the author, the block is advisory, and the review record shows a dismissed request rather than a resolved one. Whether that is acceptable is a policy question, but it should be a policy question. A default that allows self-dismissal makes the blocking state meaningless without anyone deciding it should be.

A third is the difference between “no objection” and “endorsement”. An approval given after a cursory read and an approval given after a careful one are the same bit. Some systems add a distinct state for “reviewed but not endorsed”, which is useful precisely because it lets a reviewer record the comprehension claim without the adequacy claim, and lets the merge policy require endorsement from someone specific while accepting comprehension from anyone.

What approval cannot be

Approval cannot be a warranty. The defect claim is bounded by the reviewer’s detection ability, and the classes outside that bound, such as reachability, concurrency, cost, and data flow, are not covered by any amount of reading. Treating approval as assurance that the change is defect-free misallocates the responsibility for those classes onto a mechanism that cannot carry it.

Approval also cannot be delegated without losing its content. A reviewer who approves because a colleague said the change is fine is recording the colleague’s claim under their own name, and the record now attributes a judgement to someone who did not make it. The comprehension claim in particular cannot be transferred, because it is a fact about what a particular reader understood.

What approval can be is a documented, attributable, revision-scoped statement that a specific reader examined a specific artifact and formed a specific judgement. Every part of that sentence has to be represented in the tooling for the approval to mean anything, and the parts most often dropped are the revision and the specificity of the judgement.