code Review/bugs/Logic and State Mutation Bugs: Finding Correctness Defects in a Pull Request

bugs

Logic and State Mutation Bugs: Finding Correctness Defects in a Pull Request

Off-by-one bounds, floating-point money, leaked internal collections, and mutable defaults — how to read a diff for the correctness defects that reach production most often.

Reviewer Detection Checklist

0 of 12 verified (0%)

Defect Patterns & Fixes

Currency held in a floating-point accumulator

java

Scenario: An order service totals line items and applies a percentage discount before returning the amount to charge.

public double processOrder(Order order) {
    double totalAmount = 0.0;
    for (OrderItem item : order.getItems()) {
        double itemTotal = item.getPrice() * item.getQuantity();
        if (order.getDiscountCode().equals("SUMMER20")) {
            itemTotal = itemTotal * 0.80;
        }
        totalAmount += itemTotal;
    }
    return totalAmount;
}

Why this is defective: Two defects in six lines. double cannot represent most decimal fractions exactly, so each multiply-and-add introduces error; across a large basket or a nightly billing run the total drifts from the sum finance computes, and no amount of display rounding fixes a stored value that is already wrong. Separately, order.getDiscountCode().equals(...) throws NullPointerException for every order without a discount code — the common case. Calling equals on the constant instead removes the null branch entirely rather than adding a check.

How to Spot It in Reviews:

•double or float anywhere near price, amount, total, balance, rate, or tax.
•A getter called directly on a nullable field, then dereferenced — invert to constant-first comparison.
•No explicit rounding mode or scale on a monetary result means the rounding is whatever the last operation happened to do.
•Check the return type as well as the arithmetic: fixing the accumulator but returning double re-introduces the loss at the boundary.

Accessor leaks internal mutable state

csharp

Scenario: A cart aggregate exposes its line items so callers can render them.

public class Cart
{
    private readonly List<LineItem> _items = new();

    public List<LineItem> Items => _items;

    public void Add(LineItem item)
    {
        if (_items.Count >= MaxItems) throw new CartFullException();
        _items.Add(item);
    }
}

// Elsewhere, far from any invariant:
cart.Items.Add(new LineItem(...));   // bypasses MaxItems entirely
cart.Items.Clear();                  // empties a cart nobody asked to empty

Why this is defective: readonly on the field protects the reference, not the contents. Exposing the List<T> means every invariant the class enforces — item limits, price recalculation, audit events — can be bypassed from anywhere in the codebase, and the resulting corruption surfaces far from the mutation. The bug is not that someone will do it maliciously; it is that a reasonable caller will do it by accident and the type system will agree with them.

How to Spot It in Reviews:

•An expression-bodied property or getter that returns a field of a mutable collection type.
•A class that enforces an invariant in one method while exposing the state that invariant protects.
•Public arrays are always mutable — IReadOnlyList on an array-backed property still hands out a copyable reference if the array itself is returned.
•Ask 'what happens if a caller mutates this?' — if the answer is 'the invariant breaks', it is a defect, not a style preference.

Mutable default argument shared across every call

python

Scenario: A helper builds an audit trail for a request and appends the current step.

def record_step(step: str, trail: list[str] = []) -> list[str]:
    trail.append(step)
    return trail


# Request 1
record_step("validated")          # -> ["validated"]
# Request 2, minutes later, different user
record_step("validated")          # -> ["validated", "validated"]

Why this is defective: Default arguments are evaluated once, when the function is defined, so every call that omits trail shares the same list object for the lifetime of the process. In a long-running server this accumulates state across requests and across users, which is not merely a wrong result — an audit trail or error list that carries one tenant's data into another tenant's response is a data-leak incident. The same trap applies to dict, set, and any mutable object constructed in a default, including datetime.now().

How to Spot It in Reviews:

•Grep the diff for =[], ={}, =set(), and =dict() in parameter lists.
•Any default value that is not a literal number, string, None, True, or False deserves a second look.
•The symptom is 'results grow over time' or 'works on the first request' — treat those bug reports as a default-argument search.
•The corrected version also copies the caller's list; mutating an argument in place is a separate defect hiding behind the same signature.

Off-by-one in a pagination bound

Scenario: A cursor endpoint slices a result page and reports whether more rows exist.

func Page(rows []Row, offset, limit int) ([]Row, bool) {
    end := offset + limit
    if end > len(rows) {
        end = len(rows)
    }
    page := rows[offset:end]
    hasMore := end < len(rows)-1     // off by one
    return page, hasMore
}

Why this is defective: end < len(rows)-1 reports hasMore = false while exactly one row remains, so the last record of every result set is unreachable through the API — invisible in tests that use ten rows and a page size of three, obvious to the customer who cannot find their most recent order. The corrected version also guards offset beyond the end (a panic, not a wrong answer) and copies the slice, because a returned sub-slice shares the caller's backing array and a later append can overwrite data the caller still holds.

How to Spot It in Reviews:

•Any -1 or +1 adjacent to a length or size is worth stating out loud: 'when exactly one row is left, this returns...'.
•Check the empty case and the exactly-full case, not the middle of the range.
•In Go specifically, a returned sub-slice is an alias — look for return s[a:b] from an exported function.
•Boundary defects need boundary tests: a test with len(rows) == offset+limit is the one that fails.

Why plain logic defects still dominate real incidents

Reviewers over-index on exotic categories. In practice the defect that takes production down is usually a boring one: an inverted condition, a variable compared against the wrong bound, money held in a double, or a method that hands a caller a reference to its own internal list. None of these need load, timing, or an attacker. They are wrong the first time they run, for inputs that occur every day, and they are the easiest class to catch by reading carefully — which is exactly why missing one is expensive.

The reason they survive to production is that they usually work for the value the author tested with. <= versus < is correct for every input except the boundary. 0.1 + 0.2 is close enough until you multiply by ten thousand transactions and reconcile the ledger.

What it costs in production

Wrong money. Floating-point arithmetic on currency drifts by fractions of a cent per operation. Aggregate that across a billing run and finance opens a ticket that engineering cannot close without a data migration.
Silent data corruption. A method returns its internal collection, a caller mutates it, and an object that was supposed to be immutable now disagrees with the database. The bug reports arrive weeks later and point everywhere except the accessor.
Off-by-one truncation. A pagination bound that drops the last row of every page. Nobody notices until a customer counts.
Sticky defaults. A mutable default argument in Python accumulates across calls, so request 500 sees data from request 1 — which looks exactly like a cross-tenant data leak, and has to be treated as one.
Inverted guard clauses. A permission check with or where it needed and fails open. That is a logic bug that files as a security incident.

How to spot it in review

Reading for logic defects is a different mode from reading for structure. Slow down at these five shapes.

1. Every comparison operator in a boundary expression. For each <, <=, >, >=, !=, ask what happens at exactly the boundary value and at zero. Loop bounds, slice ranges, retry counters, and page offsets are where off-by-one lives.

2. Every mutation of something you did not create in this scope. If a function mutates a parameter, a field of a parameter, or a value returned by a getter, the caller's state has changed. That may be intended; it usually is not documented.

3. Every accessor that returns a collection, array, map, or date. return this.items; hands out a live reference. In Java the fix is List.copyOf, in C# an IReadOnlyList plus a defensive copy, in Go a copied slice, in Python a tuple or a copy.

4. Every arithmetic operation on money, percentages, or durations. Decimal types for currency, integers for cents, explicit units on every variable. double totalAmount is a defect on sight in a financial path.

5. Every default value and every fallback. Mutable defaults, ??/|| collapsing a legitimate 0 or empty string to a fallback, and orElse building an expensive object on the happy path.

Language-specific tells:

Python: def f(items=[]), is used for value comparison, integer division / versus //, and truthiness checks that treat 0 and "" as missing.
Java: == on boxed types and strings, double for money, Optional.get() without a check, List returned straight from a field.
Go: an error shadowed by := in an inner scope, a slice aliasing a caller's backing array after append, a nil map written to.
C#: ?? on a value that can legitimately be zero, DateTime.Now in domain logic, struct copies mutated in place.
Rust: integer overflow that panics in debug and wraps in release, unwrap() on a value that is optional by design.

Tip

Read the diff twice. The first pass for what the code does, the second pass reading only the conditions and the arithmetic, out of context. Structural reading and boundary reading use different attention, and doing both at once is how the <= gets missed.

Fixing it without breaking something else

Logic fixes look tiny, which is what makes them dangerous — a one-character change is rarely reviewed as carefully as a new class.

Fixing an off-by-one shifts every downstream index. If callers, tests, or stored cursors were written against the buggy bound, they were compensating for it. Search for the compensation before you change the bound.
Switching double to BigDecimal/decimal changes serialisation, comparison, and equality. compareTo versus equals differ on scale (2.0 is not equal to 2.00), JSON output gains trailing zeros, and any database column, API contract, or downstream consumer must move with it.
Returning a defensive copy changes performance and identity. Callers that relied on mutating the returned list will silently stop working — a behaviour change that compiles cleanly. Find them.
Making a null-returning method throw instead flips the failure mode from a wrong result to an outage. Prefer the change that fails at the boundary where the value entered the system.
A data fix is usually needed too. If the defect has been live, corrupted rows already exist. A correct code fix that leaves bad data behind is half a fix.

How not to over-engineer the fix

The proportionate response to a logic defect is almost always change the expression, add the test.

Anti-patterns to push back on in review:

Wrapping the buggy call in a validation framework instead of correcting the operator.
Introducing a Money value object, a currency service, and a rounding policy engine when the change needed was BigDecimal and a scale.
Adding a defensive try/catch around a NullPointerException rather than fixing the field that is null.
Rewriting a loop as a stream/LINQ/iterator pipeline "while we are here" — now the reviewer of the fix cannot see the fix.
Adding a feature flag around a one-line correctness fix. Flags are for behaviour changes, not for arithmetic that was wrong.

The one place extra investment is justified: a regression test that fails before the fix and passes after. If the defect was reachable from a boundary value, the test must use that boundary value, not a comfortable one in the middle of the range.

Warning

Beware the fix that makes the symptom disappear without addressing the cause — clamping a negative result to zero, max(0, stock), or catching the exception the bad arithmetic throws. These convert a visible defect into an invisible one and are worth a blocking comment.

Comment templates that an agent can act on

"double totalAmount accumulates currency; use BigDecimal with RoundingMode.HALF_UP and change the return type."
"order.getDiscountCode().equals("SUMMER20") NPEs when the code is absent — flip to "SUMMER20".equals(order.getDiscountCode())."
"getItems() returns the live internal list; callers can mutate order state. Return List.copyOf(items)."
"Loop runs to i <= size, one past the end. Should be i < size."
"def add_tag(tag, tags=[]) shares one list across all calls. Default to None and create the list inside."

Previous Guide

Race Conditions and Thread Safety: Catching Concurrency Defects in Code Review

Next Guide

Null, Empty, and Boundary Edge Cases: The Reviewer's Systematic Checklist

Interactive Practice

Review Defective Code in the Workbench

Test your ability to spot this defect in our interactive Monaco-powered PR code editor.

Open Workbench