2026-09-08 · 7 min read

Off-by-one errors in code review: five shapes, twelve PRs, and why round test data hides all of them

We have a couple of hundred pull requests in the library with a planted bug, and when I went through the ones tagged as boundary mistakes I noticed something slightly embarrassing: nearly every one of them drops the last record, and nearly every one of them was "verified" by its author on a dataset whose size happened to divide evenly. That happens to be the shape of the bug in the wild too, which is why the authors kept landing on it. Off-by-one is the error that lives exactly where nobody's test data goes, and once you've seen the five shapes it takes, you start finding it in diffs you would have approved in ten seconds.

Why this one bug family survives every test suite

You already know the mechanics. Someone counts from zero where the other side counts from one, or uses a strict comparison where the spec says "or more", or floors a division that had a remainder. What makes it worth a whole page is the epidemiology rather than the arithmetic: the bug only fires on a boundary value, and boundary values are precisely the ones a developer doesn't reach for when they write a quick check. You pick 20 members for the staging directory, 100 rows for the CSV, 1000 users for the export, and all of those are multiples of the page size, so the floored page count is right, the loop that stops one short looks fine because the last item was an empty line anyway, and the PR description honestly says "checked against sample data." The author is not lying. The sample data was just round.

And AI-written PRs make this worse in a specific way, because a model will happily generate a test fixture and a test that uses it, and it will pick 20 or 100 or 1000 for exactly the same reason a human does: they're the numbers that come to mind first. So you get a green suite whose fixtures never touch the remainder case, and a reviewer who sees tests and relaxes.

Shape one: the floored page count

The most common one in our library by a wide margin. Somebody needs to know how many pages, batches, or slides a list needs, and they write the division without the ceiling.

const pageCount = Math.floor(members.length / PAGE_SIZE);
// 43 members, page size 10: pageCount is 4, members 41 to 43 are unreachable

With 40 members it's perfect. With 43 the Next button disables itself on page four and three people quietly don't exist in the directory. The same line shows up as total // PAGE_SIZE in a Python nightly export (the last 250 of 1250 users never get fetched) and as a slide count in an Angular carousel where the last two products land on a slide the loop never builds. The review comment that catches all three is the same: "this floors, so any remainder is dropped; should be Math.ceil, and can we add a fixture whose length isn't a multiple of the page size?" That second half of the comment is the part that actually protects the codebase, because the next person to touch the file will run into the test before they run into the bug.

Practice on it: the React directory, the Python export, the Angular carousel.

Shape two: the loop that stops one short

This one has two disguises. The obvious one is a hand-written bound with a stray - 1:

for (let i = 0; i < expenses.length - 1; i++) {
  rows.push(toCsvRow(expenses[i]));
}

The last expense never gets pushed, and with exactly one expense the loop body never runs at all, so the CSV is a header and nothing else. The less obvious disguise is a batching loop whose condition asks whether a whole batch still fits:

for (let start = 0; start + BATCH_SIZE <= records.length; start += BATCH_SIZE) {
  await bulkIndex(records.slice(start, start + BATCH_SIZE));
}

That reads as careful. It looks like someone thought about not overrunning the array. What it actually does is refuse to send the trailing partial batch, so a sync of 1,050 records indexes 1,000 and reports success. COBOL has its own version, because PERFORM VARYING ... UNTIL WS-IX = WS-REGION-COUNT tests the condition before each pass and so never runs the pass where the index equals the count; the last region's summary line simply isn't written, and it has probably been not-written in a few production feeds for decades.

The tell for this shape is any loop condition that isn't the plain, boring i < length. The moment the bound has arithmetic in it, ask what happens on the last element, and then ask what happens when there's exactly one.

Practice on it: the CSV export, the batched sync, the COBOL region summary.

Shape three: two counting systems meet at an API boundary

Zero-based on one side, one-based on the other, no adapter in between. This is the off-by-one that hides best, because both halves are internally consistent and the bug only exists on the wire between them.

// component counts pages from 0; the API documents page=1 as the first page
this.service.fetchPage(this.pageIndex()).subscribe(...)

And here's the cruel part: most APIs clamp page=0 up to page one instead of returning an error, because that's friendlier. So the first page renders twice, the last page never renders, and no request ever fails. Nothing in any log says anything is wrong. In the audit-log exporter version of this, for (let page = 0; page < meta.totalPages; page++) walks pages zero through N minus one against a one-based API, and the legacy clamp turns what should have been a 400 into a duplicate of page one plus a missing page N.

The review comment here is a question, not an assertion, and it's the single most useful question in this whole family: "what's the base of the page parameter on the server side, and where in this diff do we convert?" If the answer is "we don't", you've found it.

Practice on it: the Angular shipments list, the audit-log exporter, the React catalog grid.

Shape four: inclusive in the spec, exclusive in the code

"Orders of $50.00 or more ship free." The diff says subtotalCents > 5000. A $50.00 cart pays $5.99 shipping, and it will do that for months, because who tests exactly fifty dollars.

// before the refactor: if (qty >= t.minQty)
if (qty > t.minQty) return t.unitCents;

The wholesale-tier version is nastier because it arrived inside a refactor: the old loop used >=, the rewrite scans the other direction and uses >, and the PR description says "no behaviour change." A customer ordering exactly the minimum quantity for a tier gets the tier below. This is the shape where you have to read the requirement and the operator side by side, out loud if necessary, because your eyes will slide over a single missing equals sign every time.

Practice on it: the free-shipping badge, the wholesale tier.

Shape five: the boundary value that hits no branch at all

The last one is rarer and worse. Instead of dropping a record, it hangs.

while (days > 365) {
  if (isLeapYear(year)) {
    if (days > 366) { days -= 366; year += 1; }
  } else {
    days -= 365; year += 1;
  }
}

Day 366 of a leap year satisfies the outer while, enters the leap branch, fails the inner if, and there's no else. Nothing decrements, nothing advances, the loop spins forever. This is the mechanism that froze every 30GB Zune on 31 December 2008, and the reason it's in this list is that it's the same review instinct as the other four: find the boundary value (the last day, the exact threshold, the remainder) and trace it through by hand. Here the trace takes about fifteen seconds and ends in an infinite loop.

Practice on it: the device clock.

What to actually write in the review

Across all five shapes the comment that scores, both with our grader and with a real colleague, has the same three parts. Name the boundary value that breaks ("with 43 members", "on day 366", "a cart of exactly $50.00"). State what happens to it (dropped, duplicated, hangs). Ask for the fixture that would have caught it, which almost always means a size that is not a multiple of anything. The third part is what turns a caught bug into a caught bug class, and it's the part reviewers skip most, because by the time you've found the bug you feel done.

If you want to calibrate yourself, the twelve PRs linked above are all live and free. Some of them you'll catch in seconds. At least one of them, statistically, you'll approve, and the canonical review will tell you which boundary you walked past.