Inspect the diff

What you will be able to do

Read a change line by line before you accept it. Look for what the tests do not show: unasked edits, changed promises, guesses about the input, and code that is hard to keep. A green test run is one piece of evidence, not the review.

Before you startWhat to know and to have readyAbout 45 minutes

What you need to know

• The task brief and its acceptance criteria, from Problem and acceptance criteria.
• How to read git diff and git diff --stat, from Read and review diffs in Git and Team Development Essentials.
• What a function promises, from Function contracts in Python for Practical AI Engineering.

What you need on your computer

• The project ~/projects/ticket-cleaner-assist from Module 2, on the branch feature/category-option, with its environment active. python -m pytest prints 26 passed, and git status says working tree clean.
• The labs folder ~/projects/nextia-learning-labs, with C15/candidates/.
• If the project is missing or broken, use the reset steps of Context and boundaries with the snapshot end-of-m02.
• No account, no key and no money. The changes that you review come from recorded sessions.

A new tap, before you pay

A plumber fits a new kitchen tap. When you come home, you turn it on, and water comes out. Do you pay?

Look closer first. The plumber also moved the light switch, “because it was in the way”. The new tap gives only cold water, because the plumber assumed that you wanted it so. A pipe under the sink now has a joint that nobody can reach. The water came out, and the job is still not right.

The running water is like a passing test. It shows that one thing works. It does not show what else changed, or what the plumber guessed.

The problem: “the tests pass, merge it”

In Problem and acceptance criteria, the course team gave an assistant Grace’s feature as one sentence. It was “Add a way to filter the report by category.” The change that came back is in the labs folder as candidate A. It is real, from the recorded session feature-vague, and nobody edited it.

In the recording, all of its tests passed: 29 passed. Tomás likes that:

“The tests pass, and the diff is small: 77 lines. Merge it.”

But you know from Module 2 that candidate A fails 4 of the 8 acceptance checks. It takes one category, it needs the exact spelling, and it adds a key to every report. The tests passed because the assistant wrote them for its own decisions. Before its edits, it even said what it would do:

What the assistant printed (part)

I’ll add an optional `--category` argument that matches a report category exactly and combines with the status filter. Without it, reports will still include all categories.

The second sentence sounds like “the report does not change”, but the code does change it. A summary of a change, from an assistant or a person, is a claim. The diff is the change.

The idea: code review, one line at a time

A code review is a careful reading of a change by someone who did not write it, before the change is accepted. You met it in the Git course, for a teammate’s pull request. With an assistant, you are always that someone, because nobody else read the code before you.

Read each added line, and ask four questions:

1. Is it an unrelated edit? The task did not ask for it: a renamed variable, a rewritten comment, a reformatted test.
2. Does it change a contract? A contract is what other code or people rely on: the report’s keys, the printed lines, the exit codes.
3. Does it hide an assumption? The code guesses something about the input: the spelling, one value instead of several, a terminal of a certain width.
4. Is it a maintainability problem? The next person will find the code hard to read or change: long lines, duplicated logic, a misleading name.

Then give each line one mark: needed (the task needs it, and it is right), unrelated (question 1), or risky (questions 2 to 4). A risky line is not always wrong. It is a line that you must check before you accept it.

A tiny example: four added lines

Here is the first part of candidate A, in ticket_cleaner/cli.py. The four added lines are lines 31 to 34 of the new file:

ticket_cleaner/cli.py (candidate A, part)

    help="which tickets to count (default: open)",
)
parser.add_argument(
    "--category",
    help="only count tickets in this category (exact match; default: all categories)",
)

• Lines 31, 32 and 34 are needed. The task needs an option, and --category is the name in the brief.
• Line 33 is risky. The help text says it out loud: “exact match”. So --category Billing finds nothing. It also says “this category”: one. The option has no action="append", so a second --category replaces the first, with no error.

Four lines hide two assumptions, and you can see both if you read the words. You did not need to run anything. You needed the brief next to the diff.

Review candidate A yourself

The explorer shows the code part of candidate A: 43 added lines in cli.py, report.py and tests/test_report.py. Mark the lines first. Then show the course team’s review and compare.

Explore

Mark each added line of candidate A

Your marks

-

The review

5 risky, 1 unrelated, 37 needed

Found

-

The review of each added line. With JavaScript, you mark the lines yourself first.

ticket_cleaner/cli.py: 5 added lines

Review	Line	Code
		@@ -28,6 +28,10 @@ def make_parser() -> argparse.ArgumentParser:
	28	choices=sorted(STATUSES),
	29	help="which tickets to count (default: open)",
	30	)
needed	31	+ parser.add_argument(
needed	32	+ "--category",
risky	33	+ help="only count tickets in this category (exact match; default: all categories)",
Says 'exact match' out loud: one category only, and the spelling must be exact.
needed	34	+ )
	35	parser.add_argument(
	36	"--output",
	37	type=Path,
		@@ -62,7 +66,7 @@ def main(argv: list[str] | None = None) -> int:
	66	for record in rejected:
	67	logger.warning("row %d rejected: %s", record.row, "; ".join(record.problems))
	68
		- summary = build_summary(tickets, rejected, args.status)
needed	69	+ summary = build_summary(tickets, rejected, args.status, category=args.category)
	70	write_json(summary, args.output)
	71	logger.info("wrote %s", args.output)
	72	print(f"{len(rows)} rows: {len(tickets)} valid, {len(rejected)} rejected.")

ticket_cleaner/report.py: 15 added lines

Review	Line	Code
		@@ -6,6 +6,11 @@ def filter_by_status(tickets: list[Ticket], status: str = "open") -> list[Ticket
	6	return [ticket for ticket in tickets if ticket.status == status]
	7
	8
needed	9	+def filter_by_category(tickets: list[Ticket], category: str) -> list[Ticket]:
needed	10	+ """Return the tickets that have the given category."""
risky	11	+ return [ticket for ticket in tickets if ticket.category == category]
Exact match: --category Billing or ' billing' finds nothing. The tickets' categories are in small letters.
needed	12	+
needed	13	+
	14	def count_by_category(tickets: list[Ticket]) -> dict[str, int]:
	15	"""Return the number of tickets in each category, sorted by category."""
	16	counts = {}
		@@ -21,11 +26,19 @@ def average_priority(tickets: list[Ticket]) -> float | None:
	26	return round(sum(ticket.priority for ticket in tickets) / len(tickets), 1)
	27
	28
		-def build_summary(tickets: list[Ticket], rejected: list[Rejected], status: str) -> dict:
		- """Return the report as a dictionary that can be saved as JSON."""
needed	29	+def build_summary(
needed	30	+ tickets: list[Ticket],
needed	31	+ rejected: list[Rejected],
needed	32	+ status: str,
needed	33	+ category: str | None = None,
needed	34	+) -> dict:
unrelated	35	+ """Return a report filtered by status and, optionally, category."""
Rewrites the docstring of build_summary; the old one was still true.
	36	selected = filter_by_status(tickets, status)
needed	37	+ if category is not None:
needed	38	+ selected = filter_by_category(selected, category)
	39	return {
	40	"status": status,
risky	41	+ "category": category,
Changes the contract: every report gets a new key, also without --category. Grace's spreadsheet reads the report.
	42	"valid_records": len(tickets),
	43	"selected": len(selected),
	44	"by_category": count_by_category(selected),

tests/test_report.py: 23 added lines

Review	Line	Code
		@@ -25,6 +25,29 @@ def test_categories_are_in_alphabetical_order(tickets):
	25	assert list(count_by_category(tickets)) == ["billing", "login"]
	26
	27
needed	28	+def test_summary_without_category_includes_all_categories(tickets):
needed	29	+ summary = build_summary(tickets, [], status="open")
risky	30	+ assert summary["category"] is None
The test checks the new key, so the test repeats the contract change instead of catching it.
needed	31	+ assert summary["selected"] == 2
needed	32	+ assert summary["by_category"] == {"billing": 1, "login": 1}
needed	33	+
needed	34	+
needed	35	+def test_summary_filters_by_both_status_and_category(tickets):
needed	36	+ summary = build_summary(tickets, [], status="closed", category="billing")
risky	37	+ assert summary["category"] == "billing"
The test checks the new key, so the test repeats the contract change instead of catching it.
needed	38	+ assert summary["valid_records"] == 3
needed	39	+ assert summary["selected"] == 1
needed	40	+ assert summary["by_category"] == {"billing": 1}
needed	41	+ assert summary["average_priority"] == 2.0
needed	42	+
needed	43	+
needed	44	+def test_summary_for_a_category_with_no_tickets(tickets):
needed	45	+ summary = build_summary(tickets, [], status="open", category="unknown")
needed	46	+ assert summary["selected"] == 0
needed	47	+ assert summary["by_category"] == {}
needed	48	+ assert summary["average_priority"] is None
needed	49	+
needed	50	+
	51	def test_summary_for_a_status_with_no_tickets(tickets):
	52	summary = build_summary(tickets, [], status="pending")
	53	assert summary["selected"] == 0

A real generated diff: the recorded answer to a one-sentence request. The labels are the course team's review: 5 risky, 1 unrelated, 37 needed.

Text description of the explorer

The explorer shows the real diff of candidate A, one table per file. The files are ticket_cleaner/cli.py (5 added lines), ticket_cleaner/report.py (15) and tests/test_report.py (23). Each added line has a mark: needed, unrelated or risky. Every line starts as needed. The review shows the course team’s label of each line, your mark, and a reason for each line that is not needed:

• cli.py line 33, risky: the help text says “exact match”: one category, exact spelling.
• report.py line 11, risky: ticket.category == category is an exact match, so --category Billing finds nothing.
• report.py line 35, unrelated: the docstring of build_summary is rewritten, but the old one was still true.
• report.py line 41, risky: "category": category, adds a key to every report, also without the option.
• tests/test_report.py lines 30 and 37, risky: the tests check the new key, so they repeat the contract change.

The other 37 added lines are needed.

Did you mark the two test lines? They are easy to miss, because a test looks like evidence. But a test that checks the new key turns the change into a promise. If the key is a mistake, the test now protects the mistake.

Larkfield: a checklist for every change

Amira does not trust her memory for this. She writes a short checklist and keeps it in the project, as docs/review-checklist.md. It has five parts:

• Scope: only the files of the task, no unrelated edits, no deleted or changed data file.
• Behaviour and contracts: each criterion has code and a test. The report and the exit codes change only if the task says so. Edge cases work, and the tests would fail without the change.
• Dependencies: no new package, or one that passed the checks of the next lesson.
• Security and data: input checked before it is used as a path or a command; no secret printed or logged; nothing deleted without asking. The lesson after next is about this part.
• Checks that I ran myself: the tests, ruff, and the commands of the task.

The last part matters most with an assistant. A tool can print “all tests pass” in its chat. Only your own run, in your own terminal, is evidence.

Here is the checklist applied to candidate A. Each finding has evidence that a colleague could check:

The change-review checklist, applied to candidate A. Four of the seven rows fail, and a command or a line shows each failure.

Text description of the diagram

One row per section of the checklist, with the finding and its evidence. Red rows fail, green rows pass, and amber is a warning.

• Scope (warning): all 4 files are allowed. But the docstring of build_summary() is rewritten, and README.md has no row for the option (criterion 7).
• Behaviour and contracts (fails): every report gets a new key, "category": null, also without the option. Evidence: tests/test_baseline.py fails with Left contains 1 more item: {'category': None}.
• Edge cases (fails): exact match, one category only. --category " Billing " gives selected 0; --category billing --category shipping gives {"shipping": 1}.
• Tests (fails): assert summary["category"] is None: the new tests repeat the change.
• Dependencies (passes): no new package; the requirements files do not change.
• Security and data (passes): --category is only compared with the names: no path, no command, no secret.
• Checks that I ran myself (fails): 1 failed, 30 passed; ruff format --check . says 1 file would be reformatted.

Decision: reject candidate A, and send the findings back with the task brief.

Notice the baseline test. In the recorded session, the project had no tests/test_baseline.py, so all 29 tests passed. Your project has it, from Repository preparation. So in your project, the same diff fails one test. That test exists for exactly this moment.

Review candidate A in your project

1. Check the start state. git status says working tree clean, and python -m pytest prints 26 passed. A clean start means that git diff shows only the candidate.

2. Apply candidate A. git apply changes the files as the patch says, and it makes no commit:

   git apply ../nextia-learning-labs/C15/candidates/A-vague-request.patch

   No output means that the patch applied.

3. Look at the scope first:

   git diff --stat

   Output

    tests/test_cli.py        | 34 ++++++++++++++++++++++++++++++++++
    tests/test_report.py     | 23 +++++++++++++++++++++++
    ticket_cleaner/cli.py    |  6 +++++-
    ticket_cleaner/report.py | 17 +++++++++++++++--
    4 files changed, 77 insertions(+), 3 deletions(-)

   Compare it with the brief’s constraints: cli.py, report.py, README.md and the tests. Every file is allowed. One allowed file is missing: README.md.

4. Read the code part of the diff, with the brief open next to it:

   git diff ticket_cleaner/

   Find the lines of the explorer: the help text, the == in filter_by_category(), the new docstring and the "category": category, key.

5. Run the tests yourself:

   python -m pytest

   Output (the end)

   FAILED tests/test_baseline.py::test_report_of_the_sample_file_is_unchanged - ...
   ========================= 1 failed, 30 passed in 0.05s =========================

   Higher up, the output shows why:

   Output (part)

   E         Left contains 1 more item:
   E         {'category': None}

6. Run two commands of the brief. Each prints 15 rows: 9 valid, 6 rejected. and writes reports/summary.json. Open the report after each one, and compare it with the brief:

   python -m ticket_cleaner data/tickets.csv --category " Billing "
   python -m ticket_cleaner data/tickets.csv --category billing --category shipping

   The first report has "selected": 0 and "by_category": {}, but the brief expects 2 billing tickets. The second report has "category": "shipping" and "by_category": {"shipping": 1}. The billing tickets are gone, and nothing says so.

7. Run the format check:

   ruff format --check .

   Output (the end)

   1 file would be reformatted, 19 files already formatted

8. Reject the candidate. git restore . puts back every changed file from the last commit:

   git restore .
   git status --short
   python -m pytest

   git status --short prints nothing, and the tests print 26 passed.

Write your findings down, as in the diagram, before you restore. They go back with the brief to whoever asks for the next attempt.

The accepted change, after review

Amira did not ask again with one sentence. In Module 2, the course team gave the assistant the task brief (session feature-bounded, real). That change passed all 8 acceptance checks. Its review still found three things:

• ruff format --check . failed, because two lines were too long.
• The docstring of build_summary() did not state the important rule: the categories key appears only with the option.
• Its tests compared the whole report in one large parametrized test. They were correct, but hard to read next to the project’s other tests.

Amira kept the design: the filter after the status filter, and the categories key only with the option. She fixed the three findings herself. The result is in the snapshot end-of-m03. You copy it, and then you review it as if it were new, because it is new to you.

1. Add the checklist to the project, and commit it on its own:

   macOS and Linux

   mkdir -p docs
   cp ../nextia-learning-labs/C15/snapshots/end-of-m03/docs/review-checklist.md docs/
   git add docs/review-checklist.md
   git commit -m "Add a checklist for reviewing a generated change"

   Windows

   New-Item -ItemType Directory -Force docs
   Copy-Item ..\nextia-learning-labs\C15\snapshots\end-of-m03\docs\review-checklist.md docs\
   git add docs/review-checklist.md
   git commit -m "Add a checklist for reviewing a generated change"

   Output (the commit)

   [feature/category-option 8281ea5] Add a checklist for reviewing a generated change
    1 file changed, 41 insertions(+)
    create mode 100644 docs/review-checklist.md

   Your commit hash is different.

2. Copy the accepted change: two code files, two test files and the README.

   macOS and Linux

   S=../nextia-learning-labs/C15/snapshots/end-of-m03
   cp $S/ticket_cleaner/cli.py $S/ticket_cleaner/report.py ticket_cleaner/
   cp $S/tests/test_cli.py $S/tests/test_report.py tests/
   cp $S/README.md .

   Windows

   $S = "..\nextia-learning-labs\C15\snapshots\end-of-m03"
   Copy-Item "$S\ticket_cleaner\cli.py", "$S\ticket_cleaner\report.py" ticket_cleaner\
   Copy-Item "$S\tests\test_cli.py", "$S\tests\test_report.py" tests\
   Copy-Item "$S\README.md" .

3. Review the scope:

   git diff --stat

   Output

    README.md                |  9 +++++++++
    tests/test_cli.py        | 19 +++++++++++++++++++
    tests/test_report.py     | 39 +++++++++++++++++++++++++++++++++++++++
    ticket_cleaner/cli.py    |  8 +++++++-
    ticket_cleaner/report.py | 22 +++++++++++++++++++---
    5 files changed, 93 insertions(+), 4 deletions(-)

4. Read git diff ticket_cleaner/ with the checklist. The option has action="append" and metavar="NAME", so it can repeat. build_summary() cleans each name with name.strip().lower(), filters after the status filter, and adds "categories" only when categories is not None. valid_records still counts all the tickets. The filter compares the names with ticket.category as it is stored, so it does not work around Grace’s bug. On her export, --category login selects 2 login tickets, not 4. The fix comes in Module 4.

5. Run the checks yourself:

   python -m pytest
   ruff check .
   ruff format --check .

   Output (the end of each command)

   ============================== 34 passed in 0.04s ==============================
   All checks passed!
   21 files already formatted

   The baseline test passes: without --category, the report has not changed.

6. Run a command of the brief, and check the report against your values from Module 2:

   python -m ticket_cleaner data/tickets.csv --category billing --category shipping

   The report has "selected": 3, "by_category": {"billing": 2, "shipping": 1}, "valid_records": 9 and "categories": ["billing", "shipping"]. In the same way, --category " Billing " gives "selected": 2, and --category garden gives "selected": 0 with exit code 0.

7. Commit the change:

   git add -A
   git commit -m "Add the --category option, after review"

   Output (part)

    5 files changed, 93 insertions(+), 4 deletions(-)

Look at the size. The accepted change is +93 −4 in five files, and candidate A was +77 −3 in four. The better change is the larger one. Size never decides a review: the criteria and the line-by-line reading do.

When to review, and what it costs

Review every change before you accept it: from an assistant, from a colleague, from yourself last week. The review is the moment when you take ownership of the code. After it, “the assistant wrote it” is no longer an answer to any question.

A careful review takes time, and the time grows with the size of the diff. That is the strongest reason for small tasks and small steps, as in Break work into steps. If a diff is too large to read in one sitting, do not skim it. Ask for a smaller change.

A review has limits. It finds what a careful reader can see, and it misses what nobody thought to check. So it works with the tests and the brief’s commands, not instead of them. Add a line to the checklist each time that a review misses something.

Common mistake

git diff shows nothing, but the files changed

Symptom: an agent says that it changed three files. You run git diff --stat, and it prints nothing. You conclude that there is nothing to review.

Cause: some assistants commit their own changes. Then the change is in a commit, and git diff compares your files with that commit: no difference. The course recorded its sessions with commits turned off, but the default of a tool can differ.

Diagnose: look at the history:

git log --oneline -3
git show --stat HEAD

Output (the course team's test, with candidate A committed)

ac7ea70 Add a way to filter the report by category
951e017 Start

ac7ea70 Add a way to filter the report by category
 tests/test_cli.py        | 34 ++++++++++++++++++++++++++++++++++
 tests/test_report.py     | 23 +++++++++++++++++++++++
 ticket_cleaner/cli.py    |  6 +++++-
 ticket_cleaner/report.py | 17 +++++++++++++++--
 4 files changed, 77 insertions(+), 3 deletions(-)

Fix: review the commit with git show HEAD, or the whole branch with git diff main...feature/category-option, the three dots of the Git course. To reject the commit, Amira runs git reset --hard HEAD~1 herself, after she checks that nothing else is in it. Then she turns off automatic commits in the tool’s settings, as her scope file says: she runs git commit herself.

Your turn: review candidate B

Candidate B is real. The course team gave the assistant the same task brief and added one sentence: “While you are in these files, improve the code wherever you can.” It passed all 8 acceptance checks, with +171 −10 in five files.

Open nextia-learning-labs/C15/candidates/B-improve-while-there.patch in your editor. It is a text file, so you do not need to apply it. Use the checklist, and find:

1. Two edits that the task did not ask for.
2. A test that depends on something outside the code.
3. One decision about Grace’s bug that the brief did not make.

Show the answer

1. Unrelated edits. In report.py, counts = {} becomes counts: dict[str, int] = {} in count_by_category(), a function that the task does not touch. In tests/test_cli.py, the existing test test_report_for_pending_tickets is joined into one long line. Neither edit changes behaviour, but both make the diff larger and the review slower. The long line also fails the format check: ruff format --check . points to tests/test_cli.py:10.
2. A test that depends on the terminal. test_help_describes_category_option checks that the help text contains "repeat for multiple categories". argparse wraps the help text to the width of the terminal. The course team measured it: 1 failed, 35 passed in an 80-column terminal, and 36 passed in a 160-column one. The test passes on a wide screen and fails on a colleague’s narrow one. That is a hidden assumption.
3. A decision about Grace’s bug. B compares ticket.category.strip().lower(), so --category login also counts the tickets stored as LOGIN. That quietly works around Grace’s bug for this option only, while by_category still shows LOGIN and login apart. The plan of Module 2 decided the opposite: the bug is a separate task, with its own fix and test.

“Improve wherever you can” asked for decisions that nobody reviewed. A review finds them, and each one goes back to the owner of the task.

See it end to endFix a bug in an unfamiliar codebaseCase study

The case study reviews the assistant’s four-line fix with this checklist, then runs the inputs that the assistant’s own review named.

See it end to endUpgrade a dependency safelyCase study

The case study reviews an assistant’s upgrade one change at a time, against the real changelog of the package.

Knowledge check

Check your understanding

Name the four questions that you ask about each added line, with one line of candidate A for each question.

Show the answer

• Unrelated edit? """Return a report filtered by status and, optionally, category.""": the old docstring of build_summary() was still true.
• Changed contract? "category": category,: every report gets a new key, also without the option.
• Hidden assumption? return [ticket for ticket in tickets if ticket.category == category]: an exact match, so Billing finds nothing. The help text “this category” also means one category only.
• Maintainability problem? The new test test_report_for_a_category_with_no_tickets in tests/test_cli.py fails ruff format --check, so the project’s format is not kept.

Check your understanding

In the recording, candidate A gave 29 passed. In your project, the same patch gave 1 failed, 30 passed. Explain the difference, and what it says about baseline tests.

Show the answer

The recording ran on the snapshot start, which has no tests/test_baseline.py. Your project has that test from Module 1. It pins the report of data/tickets.csv as it was before any change. Candidate A adds "category": null to every report, so the pinned report no longer matches: Left contains 1 more item: {'category': None}. The assistant’s own tests could not catch this, because the assistant wrote them for its own decision. A test that you wrote before the change, for the behaviour that must not change, can catch it.

Check your understanding

Tomás says: 'The accepted change is bigger than candidate A: +93 against +77. We asked for something simple, so A was the better result.' Answer him.

Show the answer

Size does not decide a review. Candidate A is smaller because it does less: one category, an exact match, and no README row. It also changes the report for everybody. The accepted change does what the brief asks, and every criterion has a test. Its extra lines are the repeatable option, the cleaning of the names, the categories key only with the option, the README and the tests. Each one traces back to a criterion. Ask of each line “does the task need it, and is it right?”, not “how many lines are there?”.

Recap

• A passing test run is one piece of evidence. A code review reads every added line before you accept the change.
• Ask four questions of each line: unrelated edit, changed contract, hidden assumption, maintainability problem. Mark it needed, unrelated or risky.
• Candidate A passed its own tests and failed the review. It added a key to every report, matched exactly, took one category, and its tests repeat the change. In your project, the baseline test caught the new key.
• docs/review-checklist.md makes the review the same every time. Its last part, “checks that I ran myself”, is the part that an assistant cannot do for you.
• Your project has the accepted change: 34 passed, and two new commits on feature/category-option.

Next, a change arrives that adds a package to the project. Before anything is installed, you check the package.

Next: Verify dependencies