Topic 02

A Reading Order

Review

A diff read top to bottom is read in the order its file names sort, which is no order at all. The tests often sort last, so you reach the change's claims after you have already formed a view from its body, at the point where your attention is lowest.

A reading order puts first the parts most likely to hide a surprise: what the change claims, which is its tests; what it changes for everyone else, which is its signatures; and what it takes away, which is its deletions. Only then the body. On the refund diff that order finds the worst problem in the first two minutes.

The order of reading, and what each stage found in the refund diff

Shapefiles against the plan

→

Teststhe bent expectation

→

Signatures, deletionsthe lost conversion

→

Bodythe boundary flip

Start With the Shape

Before any line, read the summary statistics: which files changed, how many lines each, which are new, renamed or deleted, and which are configuration, lock or generated files. The previous topic showed the totals. The file list adds the shape.

The shape of the refund diff

$ git diff --name-status
M	src/cragmoor/booking.py
M	src/cragmoor/gyms.py
A	src/cragmoor/policy.py
M	src/cragmoor/refunds.py
M	src/cragmoor/schedule.py
M	tests/test_refunds.py

The shape of the refund diff

$ git diff --name-status
M	src/booking.ts
M	src/gyms.ts
A	src/policy.ts
M	src/refunds.ts
M	src/schedule.ts
M	tests/refunds.test.ts

The shape of the refund diff

$ git diff --name-status
M	src/main/java/org/cragmoor/Booking.java
M	src/main/java/org/cragmoor/Gym.java
A	src/main/java/org/cragmoor/Policy.java
M	src/main/java/org/cragmoor/Refunds.java
M	src/main/java/org/cragmoor/Schedule.java
M	src/test/java/org/cragmoor/RefundsTest.java

The plan named two of these. The shape shows four more: a new file holding a policy object, a change to the gym settings, a change to booking and a touched schedule file. Write the four down before reading further, because each one needs a reason, and you will want to know whether the diff ever gives one.

Tests First

The tests state what the change claims to do, and in an agent's diff a changed test is the riskiest line of all, because the tests are where an agent can make its own work pass (Chapter 7). Read every changed assertion and every changed expected value, and for each one ask why it moved. Did the task change the rule this test encodes, or did the code start producing a different answer?

tests/test_refunds.py: the two changed expectations

@@ -32,7 +35,7 @@ def test_cancel_25_hours_ahead():
 
 def test_cancel_23_hours_ahead():
     booking = drop_in()
-    assert refunds.refund_cents(booking, hours_before(booking, 23)) == 0
+    assert refunds.refund_cents(booking, hours_before(booking, 23)) == 2400
 
 
 def test_cancel_one_hour_ahead_is_not_refunded():
@@ -42,4 +45,46 @@ def test_cancel_one_hour_ahead_is_not_refunded():
 
 def test_deadline_shown_at_second_zone_gym():
     booking = drop_in(SECOND_ZONE)
-    assert cancellation_notice(booking) == "Free cancellation until 2026-11-13 09:00"
+    assert cancellation_notice(booking) == "Free cancellation until 2026-11-13 20:00"
   (… 42 more added lines: the new policy tests)

tests/refunds.test.ts: the two changed expectations

@@ -31,7 +32,7 @@ test("cancel 25 hours ahead", () => {
 
 test("cancel 23 hours ahead", () => {
   const booking = dropIn();
-  expect(refunds.refundCents(booking, hoursBefore(booking, 23))).toBe(0);
+  expect(refunds.refundCents(booking, hoursBefore(booking, 23))).toBe(2400);
 });
 
 test("cancel one hour ahead is not refunded", () => {
@@ -41,5 +42,47 @@ test("cancel one hour ahead is not refunded", () => {
 
 test("deadline shown at second-zone gym", () => {
   const booking = dropIn(SECOND_ZONE);
-  expect(cancellationNotice(booking)).toBe("Free cancellation until 2026-11-13 09:00");
+  expect(cancellationNotice(booking)).toBe("Free cancellation until 2026-11-13 20:00");
   (… 42 more added lines: the new policy tests)

RefundsTest.java: the two changed expectations

@@ -35,7 +37,7 @@ class RefundsTest {
   @Test
   void cancel23HoursAhead() {
     Booking booking = dropIn(FIRST_ZONE);
-    assertEquals(0, Refunds.refundCents(booking, hoursBefore(booking, 23)));
+    assertEquals(2400, Refunds.refundCents(booking, hoursBefore(booking, 23)));
   }
 
   @Test
@@ -47,6 +49,48 @@ class RefundsTest {
   @Test
   void deadlineShownAtSecondZoneGym() {
     Booking booking = dropIn(SECOND_ZONE);
-    assertEquals("Free cancellation until 2026-11-13 09:00", booking.cancellationNotice());
+    assertEquals("Free cancellation until 2026-11-13 20:00", booking.cancellationNotice());
   (… 42 more added lines: the new policy tests)

Two expected values changed. The 23-hour test now expects a full refund, which is right: under the 12-hour rule, a cancellation 23 hours ahead is well inside the window. The second-zone test now expects the cancellation screen to say 20:00, and that is wrong. The class starts at 09:00 on that gym's clock, and 12 hours before 09:00 is 21:00. The rule did not move the deadline by an extra hour. The code did, and the test was edited to agree with it. You have found the worst problem in the diff without reading a line of production code.

Signatures Next

A changed parameter, return type, default value or raised error changes every caller, including callers outside the diff. The refund diff changed the deadline function to take a policy instead of a gym, so its caller in booking, the cancellation screen, changed with it. That call site is now part of the review, and so is the question of why the deadline no longer needs to know which gym the class is at. A deadline that is the same for every gym ought to worry you once you know the gyms sit in two time zones.

The diff only shows the callers that changed. Callers that did not change, because the agent never opened them, are still callers, and a changed signature that compiles for them may still change what they do. Search the repository for the function's name before you move on, as the next topic does in full. In the refund diff the search finds no surprises among the callers; it finds them somewhere else, which is the next topic's story.

Deletions Before Additions

Removed lines are behaviour that stops existing, and red lines read as cleanup, so they get skimmed. Read them before the green ones.

src/cragmoor/refunds.py: the deadline function

 
-def refund_deadline(starts_at: datetime, gym: Gym) -> datetime:
+def refund_deadline(starts_at: datetime, policy: Policy) -> datetime:
     """The last moment a cancellation still gets a full refund."""
-    # Class times are stored on the gym's wall clock, with no zone attached.
-    start = schedule.localize(starts_at, gym.time_zone)
-    return start - FULL_REFUND_WINDOW
+    return starts_at.replace(tzinfo=schedule.SERVER_ZONE) - policy.window

src/refunds.ts: the deadline function

 /** The last moment a cancellation still gets a full refund. */
-export function refundDeadline(startsAt: string, gym: Gym): Date {
-  // Class times are stored on the gym's wall clock, with no zone attached.
-  const start = schedule.localize(startsAt, gym.timeZone);
-  return new Date(start.getTime() - FULL_REFUND_WINDOW_MS);
+export function refundDeadline(startsAt: string, policy: Policy): Date {
+  return new Date(schedule.localize(startsAt, schedule.SERVER_ZONE).getTime() - policy.windowMs);

Refunds.java: the deadline function

   /** The last moment a cancellation still gets a full refund. */
-  public static ZonedDateTime refundDeadline(LocalDateTime startsAt, Gym gym) {
-    // Class times are stored on the gym's wall clock, with no zone attached.
-    ZonedDateTime start = Schedule.localize(startsAt, gym.timeZone());
-    return start.minus(FULL_REFUND_WINDOW);
+  public static ZonedDateTime refundDeadline(LocalDateTime startsAt, Policy policy) {
+    return startsAt.atZone(Schedule.SERVER_ZONE).minus(policy.window());

Three lines go: a comment saying class times are stored on the gym's wall clock with no zone attached, the call that attaches the gym's own zone through the helper in schedule, and the subtraction that used it. One line replaces them, and it attaches the server's zone instead. That is the bug the second-zone test was bent to hide: for a gym in the zone an hour behind the server, the class start is read an hour early, so the deadline lands an hour early too. A member at that gym who cancels at 20:30 on Friday is refused a refund the rule promises. None of this appears in the summary, and the deleted comment said, in plain words, why the deleted line was there.

The Body Last, With Expectations

By now you know what the change claims, whom it affects and what it removed, so the body is read against those expectations instead of on its own terms. Turn on the whitespace-insensitive view, so that re-indented lines drop out, and the moved-code view, so that relocated blocks show as moved rather than as deleted and re-added.

The refund decision, with and without whitespace changes

$ git diff -- src/cragmoor/refunds.py
     """What cancelling one class refunds: its full share, or nothing."""
-    deadline = refund_deadline(booking.starts_at, booking.gym)
-    if cancelled_at <= deadline:
-        return class_share_cents(booking)
+    policy = Policy.for_gym(booking.gym, FULL_REFUND_WINDOW)
+    deadline = refund_deadline(booking.starts_at, policy)
+    if policy.allows_refunds():
+        if cancelled_at < deadline:
+            return class_share_cents(booking)
     return 0

$ git diff -w -- src/cragmoor/refunds.py
     """What cancelling one class refunds: its full share, or nothing."""
-    deadline = refund_deadline(booking.starts_at, booking.gym)
-    if cancelled_at <= deadline:
+    policy = Policy.for_gym(booking.gym, FULL_REFUND_WINDOW)
+    deadline = refund_deadline(booking.starts_at, policy)
+    if policy.allows_refunds():
+        if cancelled_at < deadline:
             return class_share_cents(booking)
     return 0

The refund decision, with and without whitespace changes

$ git diff -- src/refunds.ts
 /** What cancelling one class refunds: its full share, or nothing. */
 export function refundCents(booking: Booking, cancelledAt: Date): number {
-  const deadline = refundDeadline(booking.startsAt, booking.gym);
-  if (cancelledAt.getTime() <= deadline.getTime()) {
-    return classShareCents(booking);
+  const policy = Policy.forGym(booking.gym, FULL_REFUND_WINDOW_MS);
+  const deadline = refundDeadline(booking.startsAt, policy);
+  if (policy.allowsRefunds()) {
+    if (cancelledAt.getTime() < deadline.getTime()) {
+      return classShareCents(booking);
+    }
   }
   return 0;
 }

$ git diff -w -- src/refunds.ts
 /** What cancelling one class refunds: its full share, or nothing. */
 export function refundCents(booking: Booking, cancelledAt: Date): number {
-  const deadline = refundDeadline(booking.startsAt, booking.gym);
-  if (cancelledAt.getTime() <= deadline.getTime()) {
+  const policy = Policy.forGym(booking.gym, FULL_REFUND_WINDOW_MS);
+  const deadline = refundDeadline(booking.startsAt, policy);
+  if (policy.allowsRefunds()) {
+    if (cancelledAt.getTime() < deadline.getTime()) {
       return classShareCents(booking);
     }
+  }
   return 0;
 }

The refund decision, with and without whitespace changes

$ git diff -- src/main/java/org/cragmoor/Refunds.java
   /** What cancelling one class refunds: its full share, or nothing. */
   public static long refundCents(Booking booking, ZonedDateTime cancelledAt) {
-    ZonedDateTime deadline = refundDeadline(booking.startsAt(), booking.gym());
-    if (!cancelledAt.isAfter(deadline)) {
-      return classShareCents(booking);
+    Policy policy = Policy.forGym(booking.gym(), FULL_REFUND_WINDOW);
+    ZonedDateTime deadline = refundDeadline(booking.startsAt(), policy);
+    if (policy.allowsRefunds()) {
+      if (cancelledAt.isBefore(deadline)) {
+        return classShareCents(booking);
+      }
     }
     return 0;
   }

$ git diff -w -- src/main/java/org/cragmoor/Refunds.java
   /** What cancelling one class refunds: its full share, or nothing. */
   public static long refundCents(Booking booking, ZonedDateTime cancelledAt) {
-    ZonedDateTime deadline = refundDeadline(booking.startsAt(), booking.gym());
-    if (!cancelledAt.isAfter(deadline)) {
+    Policy policy = Policy.forGym(booking.gym(), FULL_REFUND_WINDOW);
+    ZonedDateTime deadline = refundDeadline(booking.startsAt(), policy);
+    if (policy.allowsRefunds()) {
+      if (cancelledAt.isBefore(deadline)) {
         return classShareCents(booking);
       }
+    }
     return 0;
   }
 }

Without whitespace ignored, the refund decision shows three lines removed and five or more added, because the agent wrapped the comparison in a new policy check and re-indented it. With whitespace ignored, the re-indented return line drops back into context, and what remains is the new policy lookup and one comparison that changed. Before, a cancellation at or before the deadline was refunded. After, only one strictly before it is. A cancellation exactly 12 hours ahead now gets nothing, and no test sits on that line to notice.

Where Surprises Hide

Configuration and environment files. Lock files, where a new or upgraded dependency arrives as machine output (Chapters 7 and 12). Generated files, test fixtures and snapshots. Whitespace-only hunks that carry one real edit. The far end of long files, where attention has run out. An agent's change is seldom wrong where it is long and deliberate, and often wrong in the line it touched on the way to something else: a comparison inside a re-indented block, a rounding call in a tidied function.

The refund diff has one of each kind that matters. The class-share function was "tidied" from a round-down to a round-to-nearest, a one-line change in a function the task never mentioned, and nobody reading top to bottom would stop on it, because it looks like a style fix. Chapter 7 gives it a page of its own. The gym settings file gained one optional field, the kind of change that reads as harmless until you ask who will set it and what happens when they do.

Misconceptions

"Reading top to bottom is the thorough way." File order is alphabetical, tests often sort last, and you reach the claim after already forming a view from the body, when attention is lowest.
"Deleted code is safe: less code, fewer bugs." Removed lines are removed behaviour. Three deleted lines of time-zone handling are the whole of the refund diff's worst bug.
"Test changes are routine housekeeping." In an agent's diff the tests are where the agent can make its own work pass, so each changed assertion needs a reason.
"Formatting changes are noise I can skip." Reformatting hides real edits inside it. A re-indented block in the refund diff carried a change to a single comparison.
"Lock files are machine output." A lock file change is a new or different dependency, which is code nobody on the team has read (Chapter 12).

Why It Matters

Read the file statistics first and list every file the plan did not name. Each one needs a reason before its lines are read.
Read every changed assertion and expected value before any production code. A test changed to match the output is the most dangerous hunk in the diff.
Turn on the whitespace-insensitive and moved-code views for any diff that reformats. They shrink the churn to the lines that changed meaning.
Ask in the project instructions file (Chapter 3) for formatting changes in their own commit. A behavioural diff with no reformatting is a diff you can read.

Knowledge Check

Read top to bottom in file order, where in the refund diff would you meet the bent second-zone test?

Near the end, after the production code had already shaped your view of the change
First, because git always lists test files ahead of the code they are testing
In the middle, beside the deadline function it checks, since diffs group related code
Nowhere, since a diff hides changes to tests unless you ask for them specifically

The second-zone test's expected deadline changed. What should you establish before reading any production code?

Whether the new test passes, since a passing test means the new value is right
Whether the task changed the rule this test encodes, or only the code's output changed
Whether the test was written by Ines or by Kai, to judge how far to trust it
Whether the change to the value is small, since an hour either way is within tolerance

A single comparison changed inside a block the agent re-indented. Which view makes it stand out?

The file statistics view, which counts the lines added and removed in each changed file
The name-status view, which marks each changed file as added, modified or deleted
The whitespace-insensitive view, which drops lines that only changed indentation
The plain unified diff, read slowly and carefully, which shows every changed line exactly

A diff changes the lock file. What does that imply for the review?

Nothing; lock files are machine output and are regenerated on every install anyway
Only that the formatter ran over the dependency list and nothing else needs review
That the tests will catch any problem a new dependency brings with it on the next run
A new or different dependency, which is code nobody on the team has yet read

You got correct