A Code Review Should Test the Decision, Not the Author

One booking flow could trigger the same automatic wallet refill twice. The first refill happened while booking. A later debit event ran the ordinary refill check again.

The retained correction wrote a short-lived marker per account containing the pending amount. When a matching shipment debit arrived, it consumed the marker and skipped one refill. Focused tests covered the expected sequence.

That is enough to begin a review, not finish one. The decision is not “is this cache helper elegant?” It is:

Does a per-account, amount-matched, time-limited marker safely identify the one refill which this debit must not repeat?

That question exposes concurrency, identity, expiry, and recovery cases without saying anything about the person who wrote the change.

Restate the decision before reading the style

A useful review starts by translating the change into an observable claim:

given:
    a booking refill completed

when:
    the matching shipment debit is observed

then:
    record the debit
    do not dispatch another refill
    suppress no unrelated refill

This restatement does two things. It separates the required outcome from the chosen mechanism, and it gives every review comment a reason to exist.

Without it, review drifts toward local preferences:

• rename this method;
• extract this branch;
• use a different collection helper;
• move this class into another directory.

Those comments may improve readability. They do not establish whether a customer can be charged twice or whether a legitimate refill can be skipped.

Readability matters because it helps people verify behavior. It is not a substitute for verifying behavior.

Follow the irreversible effect

The highest-risk effect in the retained change was not the cache write. It was the card-to-wallet refill which the marker attempted to suppress.

Trace that effect in both directions:

eligibility decision
    -> refill dispatch
    -> payment attempt
    -> wallet transaction

booking refill
    -> marker
    -> later debit
    -> marker consumption
    -> refill suppressed

Then ask where execution may stop:

• The refill succeeds but the marker write fails.
• The marker exists but the debit arrives after expiry.
• Two matching debits read the marker concurrently.
• Another booking uses the same account and amount.
• The marker is consumed but downstream dispatch fails.
• The debit is delivered again after the marker is gone.

These are not accusations that each failure occurred. They are states implied by the mechanism. A review should distinguish reachable states from observed incidents and require stronger proof only where the consequence warrants it.

The retained tests prove the ordinary ordering: matching account, matching amount, shipment present, marker available, no second refill job, marker removed, debit dispatch preserved. They do not prove atomic consumption, durable correlation, concurrent same-amount behavior, expiry recovery, or redelivery.

That boundary is more useful than “tests look good.”

Identity decides whether suppression is safe

Idempotency depends on the identity of an operation, not merely similarity between two payloads.

The marker used two pieces of information:

storage key: account
stored value: pending amount

That may be a practical correlation heuristic. It is not a unique booking or refill identity. Two legitimate bookings for the same account and amount can be indistinguishable during the marker window.

Review the chosen identity with a small collision table:

If the first three columns do not determine the fourth, the key cannot provide exact idempotency by itself.

That does not automatically reject the change. A one-minute marker can be reasonable when the goal is best-effort duplicate suppression, events normally arrive in order, concurrency is bounded, mistakes are detectable, and recovery is cheap. The review must make those constraints explicit.

For an irreversible or expensive effect, prefer a durable operation identity:

booking attempt
    -> refill attempt identifier
    -> debit carries or resolves that identifier
    -> atomic terminal state

The identifier need not be globally clever. It needs to name the business attempt whose effect may be repeated.

Recovery is part of the decision

A happy-path assertion often checks only whether a job was dispatched. The harder question is what the system knows after an interruption.

Use states which describe knowledge rather than optimism:

A cache expiry is not a business state transition. It means the evidence disappeared. After that point, the system may again dispatch a refill even though the earlier one succeeded.

Likewise, reading and deleting a marker in two operations is not atomic consumption. Two workers can read the same value before either deletes it. Whether that creates a duplicate effect or suppresses a legitimate one depends on the surrounding flow, but the race belongs in the review.

The synthetic reproduction executes the expected sequential case, then shows two limits of the heuristic: an unrelated equal-amount operation matches the same marker, and expiry loses suppression. A second model uses distinct attempt identifiers and atomic consumption. It is a teaching comparison, not a claim that the retained system experienced either counterexample.

Tests should attack the claim

Once the decision is explicit, test names become review questions:

• repeats the same operation without repeating the effect;
• preserves a distinct operation with an equal amount;
• handles two consumers racing for one suppression record;
• keeps the debit event when refill suppression succeeds;
• recovers when downstream dispatch fails after consumption;
• defines behavior after correlation evidence expires; and
• exposes an unknown outcome instead of treating it as failure.

Not every change needs every test. Select cases from consequence and mechanism. An in-memory presentation preference does not need a durable idempotency ledger. A payment effect deserves more than one chronological happy path.

Mocks can also mislead here. An assertion that a dispatcher was called once proves the code’s interaction with the fake in that execution. It does not prove that two workers cannot pass a non-atomic guard or that a later delivery retains the same identity.

Use the narrowest level which can exercise the disputed contract: a pure collision table for key design, a real atomic store for claim behavior, and an integration test for effect ordering.

Comments should name risk and evidence

Compare two review comments:

I do not like this cache approach.

and:

The key identifies an account and amount, not a booking attempt. Two equal bookings inside the expiry window appear identical. Is best-effort suppression the intended contract, or do we need a durable attempt key?

The second comment is reviewable. It names the mechanism, counterexample, consequence, and decision. The author can answer with a constraint, add evidence, narrow the claim, or change the design.

The same structure works for approval:

For the declared one-minute, single-booking window, the focused test proves the matching debit is preserved while the second refill is skipped. Atomic redelivery remains outside this change and is recorded as a follow-up risk.

Approval is not an absence of questions. It is a bounded statement about the decision and proof.

Review ends at a declared boundary

Code review cannot prove every production interleaving. It should still leave the change with a truthful boundary:

• the outcome the code promises;
• the identity which makes repetition the same operation;
• the effects which must not repeat;
• the failure states the design handles;
• the evidence which exercises them; and
• the residual risks consciously accepted.

That standard applies equally to code from a new hire, a staff engineer, a founder, or an automated tool. Reputation may influence how much context a reviewer expects, but it cannot change the behavior the system will execute.

Review the decision until its contract, failure states, and proof agree. Leave the author out of it.