Engineering lens demos/eng-07-review-pack.html Fictional data ← all artifacts
Code review pack · pricing-service PR #482

Fix rounding defect in discount calculation: float arithmetic → Decimal, half-even

PR #482

ABC Corp is a fictional company. Every name, number and date is invented. This is a reference artifact generated with an LLM coding agent; the brief that produces it is at the bottom of this page.

Repo pricing-service Author M. Tanaka Reviewer J. Lindqvist Head a3f92c1 Ticket LH-1204 Reviewed 11 Jun 2026 Files 2 · +30 −15
Machine-readable header — for review tooling and the change record
review_pack:           pricing-service#482
ticket:                LH-1204
base:                  main
head_commit:           a3f92c1
author:                M. Tanaka
reviewer:              J. Lindqvist
decision:              approved_with_conditions
conditions_blocking:   [N1, N4, N5]
conditions_followup:   [N2, N6]
mergeable:             false   # blocking conditions open
change_record:         CHG-88412
reviewed_at:           2026-06-11
re_review_required_by: 2026-06-18   # if not merged within 5 working days
approval_expires:      on_merge | 2026-06-25

Review notes · 6 shown

pricing/discount.py +19 −9
1"""Discount calculation for pricing-service."""1"""Discount calculation for pricing-service."""
2from decimal import Decimal, ROUND_HALF_EVEN
2 3
4TWO_PLACES = Decimal("0.01")
3TIER_RATES = {"bronze": 0.05, "silver": 0.10, "gold": 0.15}5TIER_RATES = {"bronze": Decimal("0.05"), "silver": Decimal("0.10"),
6 "gold": Decimal("0.15")}
4MAX_RATE = 0.307MAX_RATE = Decimal("0.30")
5 8
6 9
7def apply_discount(amount, tier, promo_rate=0.0):10def apply_discount(amount, tier, promo_rate=Decimal("0")):
N2MINORJ. Lindqvist · pricing/discount.py · new L10

Please annotate the new contract: def apply_discount(amount: Decimal, tier: str, promo_rate: Decimal = Decimal("0")) -> Decimal. The whole point of this fix is the type discipline: make it visible to callers and to the linter.

8 """Apply tier and promo discount to an order amount."""11 """Apply tier and promo discount; round half-even to cents."""
12 amount = Decimal(str(amount))
N1BLOCKERJ. Lindqvist · pricing/discount.py · new L12

Decimal(str(amount)) silently launders float input, so callers can keep sending floats and we lose the audit trail of where they enter. Reject non-Decimal/non-str input here (raise TypeError) and convert at the API boundary instead; otherwise LH-1204 will recur one layer up. Condition of approval.

9 rate = TIER_RATES.get(tier, 0.0) + promo_rate13 rate = TIER_RATES.get(tier, Decimal("0")) + Decimal(str(promo_rate))
10 if rate > MAX_RATE:14 if rate > MAX_RATE:
11 rate = MAX_RATE15 rate = MAX_RATE
12 discounted = amount * (1 - rate)16 discounted = amount * (Decimal("1") - rate)
13 return round(discounted, 2)17 return discounted.quantize(TWO_PLACES, rounding=ROUND_HALF_EVEN)
N3INFOJ. Lindqvist · pricing/discount.py · new L17

Good choice. Half-even is exactly what the reconciliation report in LH-1204 expects, and quantising once, at the end, avoids the compounding drift we measured (€0.01 on 0.7% of orders). No change requested.

14 18
15 19
16def line_total(unit_price, qty, tier, promo_rate=0.0):20def line_total(unit_price, qty, tier, promo_rate=Decimal("0")):
17 """Total for one order line after discount."""21 """Total for one order line after discount."""
18 subtotal = unit_price * qty22 subtotal = Decimal(str(unit_price)) * qty
N4MAJORJ. Lindqvist · pricing/discount.py · new L22

qty is unvalidated. If a caller passes a float quantity (the bulk-order endpoint does), Decimal * float raises TypeError at runtime. Coerce with Decimal(qty) after asserting it is an int, or validate and reject. Needs a test either way.

19 return apply_discount(subtotal, tier, promo_rate)23 return apply_discount(subtotal, tier, promo_rate)
tests/test_discount.py +11 −6
1import pytest1import pytest
2from decimal import Decimal
2 3
3from pricing.discount import apply_discount, line_total4from pricing.discount import apply_discount, line_total
4 5
5 6
6def test_gold_tier_discount():7def test_gold_tier_discount():
7 assert apply_discount(100.00, "gold") == 85.008 assert apply_discount("100.00", "gold") == Decimal("85.00")
8 9
9 10
10def test_promo_stacks_with_tier():11def test_promo_stacks_with_tier():
11 assert apply_discount(200.00, "silver", 0.05) == 170.0012 assert apply_discount("200.00", "silver", "0.05") == Decimal("170.00")
12 13
13 14
14def test_cap_at_thirty_percent():15def test_cap_at_thirty_percent():
15 assert apply_discount(100.00, "gold", 0.40) == 70.0016 assert apply_discount("100.00", "gold", "0.40") == Decimal("70.00")
17
18
19def test_half_even_rounds_to_nearest_even_cent():
20 # 0.25 * (1 - 0.10) = 0.225 → ties go to the even digit
21 assert apply_discount("0.25", "silver") == Decimal("0.22")
N5MAJORJ. Lindqvist · tests/test_discount.py · new L21

Only one tie case, and it only proves rounding down to even. Add the mirror case ("0.35", "silver" → 0.315 → Decimal("0.32"), rounding up to even) plus a parametrized sweep of x.xx5 boundaries: this test is the regression guard for the whole PR.

16 22
17 23
18def test_line_total():24def test_line_total():
19 assert line_total(19.99, 3, "bronze") == 56.9725 assert line_total("19.99", 3, "bronze") == Decimal("56.97")
N6MINORJ. Lindqvist · tests/test_discount.py · new L25

Magic expected value. Derive it in the test (Decimal("19.99") * 3 * Decimal("0.95") quantized) or add a comment showing the arithmetic, so the next reader can tell an intentional change from a regression.

Review summary

NoteSeverityLocationFindingResolution required
N1BLOCKERdiscount.py:12Float input silently converted at calculation layer.Before merge
N4MAJORdiscount.py:22qty unvalidated; float qty raises TypeError.Before merge
N5MAJORtest_discount.py:21Tie-rounding coverage one-sided.Before merge
N2MINORdiscount.py:10Missing type hints on new signature.Follow-up PR acceptable
N6MINORtest_discount.py:25Magic expected value in test.Follow-up PR acceptable
N3INFOdiscount.py:17Half-even + single quantize confirmed correct for LH-1204.None

Four-eyes sign-off

Reviewer
J. Lindqvist (senior engineer, pricing-service)
Author
M. Tanaka
Decision
APPROVED WITH CONDITIONS
Conditions
  • N1 resolved before merge: reject float input in apply_discount; convert at the API boundary only.
  • N4 resolved before merge: validate or coerce qty, with a covering test.
  • N5 resolved before merge: two-sided tie cases added to the half-even test.
  • N2 and N6 may land in a follow-up PR within the same sprint.
Reviewed at
Head commit a3f92c1 · ticket LH-1204
Date
11 Jun 2026

Attach this file to change record CHG-88412 as review evidence.

How this was made: the brief, how to reproduce it, and an honesty note

The brief

Render the diff of PR #482 as a single self-contained HTML review pack:
side-by-side old/new code with line numbers, my review comments as margin
notes tagged BLOCKER/MAJOR/MINOR/INFO with a severity filter, a jump-link
index, a summary table, and a four-eyes sign-off section I can archive
with the change ticket. No external requests; it will be stored as
review evidence.

How to reproduce

Paste the brief into any capable LLM: GPT, Claude, Gemini, Grok, DeepSeek, or the assistant your company provides. Iterate a few rounds on layout and content until it reads well. Save the final answer as a .html file and open it in any browser. Expect similar output, not identical: every model has its own taste, and that is fine.

Honesty note

This reference artifact was built with Claude Code, an LLM coding agent, over several iterations. Treat it as the bar to aim for, not as a guaranteed first answer. All data on this page is fictional.

Next artifact Architecture Decision Record →
vishalshah.app