Reading a Diff for the Change Nobody Asked For

By the end of this lesson you can review a change in an order that finds behaviour differences before style differences, and you can prove two versions of a function agree instead of hoping they do.

Reviewing a diff by reading it top to bottom is how small behaviour changes get approved. Attention is highest at the start and lowest at the end, the interesting change is rarely first, and a renamed variable in the middle absorbs the attention that an inverted comparison needed.

A generated diff makes this worse in a specific way: it is usually plausible everywhere. There are no obvious mistakes to catch and stop on, so a reader glides. The defence is a fixed order of attention and, wherever possible, running both versions rather than comparing them by eye.

This lesson is also the module's project increment. Direct one bounded change to the capstone, review it properly, and write down what changed and why.

Review in this order, every time. The order is chosen so the cheapest checks eliminate the largest categories of problem first.

1. The file list. Which files changed? Any file outside the stated boundary is a finding before you read a line of it.
2. Deletions. What was removed? A deleted test, a deleted guard, a deleted branch. Removals are the least visible part of a diff and the most consequential.
3. Conditions and comparisons. Every if, every and/or, every < versus <=. This is where behaviour hides.
4. Defaults and constants. A changed default alters every existing caller and shows up in no test that does not pin it.
5. Error handling. New broad catches, swallowed exceptions, new information in error responses.
6. New dependencies. Anything added to the requirements is a decision, not a detail.
7. Everything else, including formatting and renames. Last, deliberately.

Then check the evidence: do the tests actually pass, and were the existing ones changed? A green run over edited tests proves only that the tests agree with the code.

The strongest technique in this lesson is differential testing. When a change claims to preserve behaviour, keep both versions temporarily and run them over the same inputs:

for record in RECORDS:
    if before(record) != after(record):
        print("disagree:", record["notice_id"])

This finds the disagreement instead of asking you to see it, and it takes about a minute. Include boundary values in the inputs, because a boundary is where an operator change shows up and nowhere else.

Three findings that should always be raised rather than accepted quietly: a change to an existing test, a change outside the stated boundary, and an unrelated rename or reformat mixed into a functional change. None of them is necessarily wrong. All of them should be separated into their own review.

And the point of the whole exercise: you are accountable for what you merge. "The assistant wrote it" describes how a defect arrived, not who is responsible for it. If you cannot explain a line, it does not go in.

A task asked for one thing: make shortlist skip records with no published amount instead of raising. Here is the before and after.

Before:

DEFAULT_MINIMUM = 100000


def qualifies(record, minimum=DEFAULT_MINIMUM):
    return record["amount"] >= minimum and record.get("set_aside") == "Total Small Business"


def shortlist(records, minimum=DEFAULT_MINIMUM):
    return [r for r in records if qualifies(r, minimum)]

After:

DEFAULT_MINIMUM = 50000


def qualifies(record, minimum=DEFAULT_MINIMUM):
    amount = record.get("amount")
    if amount is None:
        return False
    return amount > minimum and record.get("set_aside") == "Total Small Business"


def shortlist(records, minimum=DEFAULT_MINIMUM):
    try:
        return [r for r in records if qualifies(r, minimum)]
    except Exception:
        return []

Reviewing in order: the file list is fine, one file. No deletions. Now conditions: >= became >, which was not requested and changes the boundary. Then defaults: DEFAULT_MINIMUM went from 100000 to 50000, which changes the result for every caller that relies on the default. Then error handling: a bare except Exception returning an empty list now hides any failure inside the comprehension, including the very KeyError the task was about, and reports "no matches".

The requested change, the amount is None check, is correct and is three lines of a diff containing four changes.

Given records = [{"amount": 100000, "set_aside": "Total Small Business"}, {"amount": 60000, "set_aside": "Total Small Business"}], predict len(shortlist(records)) under each version.

Restore DEFAULT_MINIMUM = 100000 in the after version but leave > in place. Predict the count, then predict it again for records = [{"amount": 310000, ...}, {"amount": 60000, ...}].

A task was framed properly, with a boundary and criteria. The response arrived with this summary:

Refactored the matching module for clarity, extracted helper functions, updated tests to match the new structure, and added type hints throughout. All 14 tests pass.

Two versions of one rule, and a claim that the rewrite changes nothing. Find the record where they disagree, then repair the rewrite.

Loading this exercise…

Optional, and the module's project increment. Direct one bounded change to your capstone: pick something small, write the task with a boundary and executable acceptance criteria, and have it implemented, by an assistant or by yourself following the specification exactly. Then review the diff in the seven-step order, run the criteria, and write two paragraphs in plain language: what changed, and why it is correct. Keep the task, the diff, and the explanation together. The capstone asks for exactly this record, and the explanation is the part that proves the review actually happened.

Sign in to track your progress on this exercise.

1. What are the first three things to look at in a diff, in order?
2. Why is reading a diff top to bottom a poor review strategy?
3. What does differential testing prove that reading cannot?
4. Why is "all tests pass" insufficient evidence when the tests were also changed?

Sign in to track your progress on this exercise.

Review by file list, deletions, conditions, defaults, error handling, dependencies, then everything else; run both versions over boundary inputs rather than comparing by eye; raise changed tests, out-of-boundary edits, and mixed-in renames as findings; and remember that whoever merges it owns it. Next: the risks that call for refusal rather than review.