What automated review is good at, and what it isn't

Where the two bugs were

On a branch I finished recently, only two modules had no tests, and those two were also the only ones that shipped a regression. The tested part (a pure core of classification, accumulation and duplicate-detection logic, 184 tests) stayed silent from the first commit to the last.

It's a small sample, but the two untested modules had something specific in common, and it was the same thing that made them hard to test. Both hold mutable state across a lifecycle. A scan starts, emits events and finishes, and the module is responsible for what it's holding at each point in that sequence. The pure core has no lifecycle at all. You hand it a value and it hands one back.

Untested code has bugs, obviously, but I think there's more to it than that. The code that's awkward to test and the code where reading a diff doesn't help are the same code, for the same reason. The behaviour lives in a sequence and you can't see it in any one line.

What the process had already done

This is worth setting out properly, because I'm not saying review failed. The branch ran to 14 tasks. Each one went through a subagent code review (about 25 in total), followed by a whole-branch review and a performance spike against 100,000 files. Everything was green, and it had earned it, because those reviews found and fixed real things all the way through.

Then I found the two problems by hand, in about a minute of clicking.

In the first, every scan that ran to completion left its event listeners attached. Stale accumulators from earlier scans kept taking in later scans' events, so the file count climbed on each rescan (306, then 612, then 918), and files started being reported as duplicates of themselves. In the second, adding two scan roots where one contained the other, OneDrive and OneDrive\Documents, walked the shared subtree twice. A file with no twin anywhere on the disk was reported as "2 copies · 150 MB wasted".

Both told the user to delete files that had no duplicate. In a read-only analyser that's a wrong number on a screen. In the next phase, which acts on its own findings, it's data loss.

Why reading the diff couldn't catch them

Think about what a reviewer would have to hold in their head to catch the first one.

The registration code is correct. So is the teardown code, and so is the cancel path that calls it. The bug is that the completion path (the one where nothing went wrong) has no reason to call teardown, so it doesn't, and the listeners outlive the scan that owns them. No line is wrong. The defect is a missing edge in a state machine, and you'd only see it if you simulated a second scan starting while the first one's subscriptions were still live.

Reviewing a diff means reading a snapshot. You can't answer "what does this module hold after event A, then event B, then a remount" from a snapshot, whether you're a person or a model. Only a test that drives the sequence can answer it, and that was the one test that didn't exist.

The overlapping-roots problem is the same shape one level up. Every root is handled correctly, but nobody ever treated the set of roots as a thing with its own invariant.

What review did catch

This is the half that usually gets left out, and I think it's the more useful one.

The fix took four rounds, and each of the first two introduced a new problem. One left an add-folder button dead on Windows, and the other hung the UI on a scan fast enough to finish before the handler was wired up. Review caught every one of those, and tests caught none of them.

That's the split. Review is strong on what it can see, like a platform-specific API used wrongly, a handler that assumes it registers before the first event arrives, or an error path that returns without cleaning up. It reads unfamiliar code just as carefully at any hour, which people don't. What it can't do is run the program, and the two original problems only existed while the program was running.

So it wasn't a case of "the reviews were bad". I had one tool pointed at two different kinds of defect, and nothing pointed at the second kind.

What the fix looked like

Making the lifecycle testable meant making it explicit. The scan adapter now returns two operations instead of one opaque stop function.

export interface ScanHandle {
  // Genuine "stop this scan" — flips the backend's cancel flag AND releases
  // this scan's listeners. Only call this when a cancel is actually intended.
  cancel: () => void;
  // Releases this scan's listeners without touching the backend at all. Use
  // this once a scan is already over (onDone fired, or start_scan itself
  // failed) so its listeners can never receive another scan's events.
  unlisten: () => void;
}

Splitting them is what lets you express the completion path at all. A scan that's finished has nothing to cancel, and calling cancel there is worse than pointless, because the backend's cancel flag is replaced on every new scan, so a late cancel lands on whichever scan is running now.

The roots problem became a pure function, normalizeRoots, which collapses any root nested inside another, so it's testable in the same cheap way as the rest of the core. It's order-independent, matches on whole path segments so C:/A can't swallow C:/AB, and folds case on Windows and macOS but not Linux. Those are three rules I wouldn't have written down if the behaviour had stayed buried in the scan.

Then I wrote a fake event harness, about thirty lines, so the sequences that used to exist only as prose in a design note (rescan after success, done-before-start-resolves, cancel-then-restart, unmount mid-scan) are each a test now.

More generally, I think test effort belongs where state outlives a function call. Pure transformations are cheap to test, and they're also where a careful reader, or a model reading a diff, is most likely to spot the problem anyway, so tests there partly overlap with review. Anything holding a subscription, a handle, an accumulator or a flag across a lifecycle is the opposite on both counts. It's expensive to test and basically invisible to review.

So a high coverage number built from pure functions only tells you about those functions, and nothing about the sequences that actually broke here.