From 2bb758cbcceb67a734fa903872d2823a802b8b98 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Fri, 28 Aug 2026 10:22:19 -0700 Subject: [PATCH] human review: financial engine --- .kiro/specs/calculator/tasks.md | 51 +++++++++++++++- engine/src/financial.zig | 100 +++++++++++++++++++++++++++++--- 2 files changed, 142 insertions(+), 9 deletions(-) diff --git a/.kiro/specs/calculator/tasks.md b/.kiro/specs/calculator/tasks.md index a7a4473..4404caa 100644 --- a/.kiro/specs/calculator/tasks.md +++ b/.kiro/specs/calculator/tasks.md @@ -914,8 +914,48 @@ plans a `--raw` flag, but today it is exercised only by tests. 100% line coverage, engine 99.44%. CLI output byte-identical across all five rows, both byte orders, ASCII packing and the multi-base standard-mode view. +### Task 5.23: One answer per loan, and a principal column in whole cents + +Open item 7 named two amortization defects. Both are real, and neither is the +mechanism the item blamed. + +**The "covers the interest" check lived in the wrong place.** It sat in +`AmortizationCursor.init`, so `amortizationPayment` would happily return a payment +for a loan that has no schedule: + +``` +amort_payment(200000, 0.5, 12000) 1,000 now: error: domain error +amort_total_interest(200000, 0.5, 12000) domain error unchanged +``` + +Two functions, one loan, two answers. The check moved into `amortizationPayment`, +which is where "the level payment implied by a loan" either exists or does not, and +the cursor now trusts it. `levelPayment` and `firstInterest` split out so the check +reads as one line. + +That loan is worth understanding: at 0.5% over 12000 periods the true level payment +exceeds the first period's interest by about 1e-23, which no f64 holds, so the two +are equal in binary before any rounding happens. Rejecting it is correct at every +scale a double can express, and the rounding the open item blamed never enters into +it. A payment one cent above the interest is still accepted and still ends in a +balloon, which `AmortizationCursor.next` supports deliberately. + +**The principal column was not a whole number of cents.** `principal = payment - +interest` is a binary subtraction of two cent-scale values, and lands an ulp off one: + +``` +amort_principal(200000, 0.5, 360, 1) 199.0999999999999 now: 199.1 +``` + +312 of the 1440 figures in a 360-period schedule were off. The money formatter prints +`{d:.2}` so the CLI table looked right, which is why this survived; as an expression +result it did not. `next` now scales the principal explicitly rather than inheriting +roundness from its operands, and a test asserts every figure in a rounded schedule +survives a round trip through the cent scale. + ### Task 5.22: Bits are not bytes, and a fold that could mean two units says so + Two findings from the `units.zig` review, one a silent wrong answer. **The prefixed bit units did not exist.** `digital_storage` had `bit` and the byte @@ -1437,9 +1477,16 @@ STILL OPEN, in the order I would take them: 5. `Rational.toFloat` double-rounds (no sticky bit), so results can be 1 ulp off. 6. Convert mode's selection zone falls through to the programmer key handler, so Enter never fires there and printable keys mutate `prog_value`. -7. Amortization rejects valid loans because the derived payment rounds down to +7. ~~Amortization rejects valid loans because the derived payment rounds down to cents before the "covers the interest" check; the principal column is not - rounded at all. + rounded at all.~~ Fixed by Task 5.23, though not as the item describes. The + rounding cannot cause a wrong rejection: for it to change the answer the payment + has to be within half a cent of the interest, and at that distance the interest + rounds to the same cent, so nothing reaches principal and the loan genuinely does + not amortize at the scale the schedule is computed in. What was actually wrong is + recorded in the task: the check sat in the cursor rather than in + `amortizationPayment`, so one entry point answered where another errored, and the + principal column really was unrounded. 8. History display caps at 512 flattened lines built oldest-first, so results stop appearing after roughly 102 detailed entries. History memory is never reclaimed (Ctrl-L frees into an arena). diff --git a/engine/src/financial.zig b/engine/src/financial.zig index 1262802..8f67e07 100644 --- a/engine/src/financial.zig +++ b/engine/src/financial.zig @@ -555,6 +555,13 @@ pub const AmortizationTotals = struct { }; /// The level payment implied by a loan, as a positive amount. +/// +/// A payment that does not cover the first period's interest never reduces the +/// balance: the loan grows forever and there is no level payment that retires it. +/// That check lives here rather than only in `AmortizationCursor.init`, so every +/// entry point agrees about which loans have an answer. It used to sit in the cursor +/// alone, and `amort_payment(200000, 0.5, 12000)` returned 1000 while +/// `amort_total_interest` on the same terms reported a domain error. pub fn amortizationPayment(p: AmortizationParams) Error!f64 { if (p.principal <= 0) return Error.DomainError; if (p.periods == 0 or p.periods > max_schedule_periods) return Error.DomainError; @@ -562,6 +569,14 @@ pub fn amortizationPayment(p: AmortizationParams) Error!f64 { // something an amortization table describes. if (p.rate < 0) return Error.DomainError; + const amount = try levelPayment(p); + if (amount <= firstInterest(p)) return Error.DomainError; + return amount; +} + +/// The payment before it is checked against the loan: the caller's, or the one the +/// TVM solver derives, rounded to cents when the schedule is. +fn levelPayment(p: AmortizationParams) Error!f64 { if (p.payment) |given| { if (given <= 0) return Error.DomainError; return if (p.round_cents) roundToCents(given) else given; @@ -580,6 +595,12 @@ pub fn amortizationPayment(p: AmortizationParams) Error!f64 { return if (p.round_cents) roundToCents(amount) else amount; } +/// The interest the first period charges, at the scale the schedule uses. +fn firstInterest(p: AmortizationParams) f64 { + const balance = if (p.round_cents) roundToCents(p.principal) else p.principal; + return balance * (p.rate / 100.0); +} + /// Walks a schedule one period at a time. /// /// Both the single-row and whole-table entry points go through this, so a row @@ -593,16 +614,12 @@ const AmortizationCursor = struct { period: usize = 0, fn init(p: AmortizationParams) Error!AmortizationCursor { + // `amortizationPayment` rejects a loan the payment cannot amortize, so a + // cursor that exists has a schedule to walk. const payment = try amortizationPayment(p); const rate = p.rate / 100.0; const balance = if (p.round_cents) roundToCents(p.principal) else p.principal; - // A payment that does not even cover the first period's interest never - // reduces the balance: the loan grows forever, and there is no schedule - // to print. - const first_interest = balance * rate; - if (payment <= first_interest) return Error.DomainError; - return .{ .params = p, .payment = payment, .rate = rate, .balance = balance }; } @@ -615,7 +632,11 @@ const AmortizationCursor = struct { self.period += 1; const interest = self.scale(self.balance * self.rate); - var principal = self.payment - interest; + // Scaled, not merely derived from scaled values: `payment - interest` is a + // binary subtraction of two cent-scale numbers and lands an ulp off one, so + // 312 of the 1440 figures in a 360-period schedule were not whole cents and + // `amort_principal(200000, 0.5, 360, 1)` answered 199.0999999999999. + var principal = self.scale(self.payment - interest); var payment = self.payment; // The final period, or any period whose scheduled principal would @@ -1529,3 +1550,68 @@ test "money: groups the same way an ordinary result does" { try testing.expectEqualStrings("231,677", as_value.text); try testing.expect(std.mem.startsWith(u8, as_money, as_value.text)); } + +test "amortization: the interest test is at the scale the schedule is computed in" { + // Open item 7 claimed this rejects valid loans, because `amortizationPayment` + // rounds to cents and the check compares against an unrounded first interest. + // The mechanism is real and cannot produce a wrong rejection: for the rounding + // to change the answer, the payment has to be within half a cent of the + // interest, and at that distance the interest rounds to the same cent, so + // nothing reaches principal and the loan does not amortize at cent scale. + // + // A tenth of a cent above the interest is not payable, so it is rejected. + try testing.expectError(Error.DomainError, amortizationPayment(.{ + .principal = 200000, + .rate = 0.5, + .periods = 360, + .payment = 1000.001, + })); + // Without cent rounding the same terms are a valid, if glacial, loan. + const unrounded = try amortizationPayment(.{ + .principal = 200000, + .rate = 0.5, + .periods = 360, + .payment = 1000.001, + .round_cents = false, + }); + try testing.expectEqual(@as(f64, 1000.001), unrounded); + + // A whole cent above it amortizes, and the term ends in a balloon rather than + // being rejected. Tightening the check to compare against the ROUNDED interest + // would reject this, which is why it is not a fix. + const balloon = try amortizationTotals(.{ + .principal = 200000, + .rate = 0.5, + .periods = 360, + .payment = 1000.01, + }); + try testing.expectEqual(@as(usize, 360), balloon.periods); + try testing.expectApproxEqAbs(@as(f64, 200000), balloon.principal, 0.01); + + // A term long enough that the level payment differs from the interest by less + // than an f64 can hold is rejected before rounding enters into it: 1.005^-12000 + // is about 1e-26, so the payment IS the interest in binary floating point. + try testing.expectError(Error.DomainError, amortizationPayment(.{ + .principal = 200000, + .rate = 0.5, + .periods = 12000, + })); +} + +test "amortization: every figure in a rounded schedule is a whole number of cents" { + // The other half of open item 7 said the principal column is not rounded. It is, + // by construction: `next` scales the interest and derives principal as + // `payment - interest` from two cent-scale values. + const rows = try amortizationSchedule(testing.allocator, .{ + .principal = 200000, + .rate = 0.5, + .periods = 360, + }); + defer testing.allocator.free(rows); + for (rows) |row| { + for ([_]f64{ row.payment, row.interest, row.principal, row.balance }) |amount| { + // A whole number of cents survives a round trip through the cent scale. + try testing.expectEqual(amount, roundToCents(amount)); + } + } +}