[yellowbean]

BugFix:

• feat: fix pool-level DefaultByAmt allocation across multiple assets #321

New Feature:

• * New CPR prepayment calculation logic

[yellowbean]

Total review — PR #322 (Sep26): per-commit, risks, potential bugs, API impact, UT coverage

Scope: 29 commits, 51 files, +1804/−521 (base master → head b627079). No route, endpoint, or API-type changes across the whole PR (EngineAPI / type API unchanged — verified by diff). The externally visible deltas are error-path behavior, one additive assumptions variant, and the config schema. Test additions are substantial (+800/−70) — suite currently 520/520 green.

Note: the branch tip also carries one local, unpushed commit 67c7d64 (Fix#332: NonEmpty sumTsCF/firstDate/CashFlow zero rows) which is not yet part of this PR.

---

Per-commit review

commit	area	verdict / notes
b00df76	.gitignore	fine
391afe9	pool DefaultByAmt pro-rata allocation	correct core (largest-remainder via prorataFactors); mortgage-only — catch-all still applies the full total per asset for other asset types (see M3); tests added (PoolTest)
9d87a96	fee double-count fix (calcDueFee: drop + fd)	superseded
639b85f	Revert of 9d87a96 (incl. its tests)	net-zero; undocumented revert — if the double-count is real it remains open (see M2)
0c3bfcd	integer-cents largest-remainder prorataFactors; getCurrentBal ScheduleMortgageFlow	good (no Rational rounding drift; zero-total safe; residual-tie by index documented); removed unused Lens imports; tests added
023a2af	DefaultByAmt > Σ balances → descriptive Left	good; small edge: negative balances unguarded (L3)
52b58bf	PrepaymentABS (monthly-only) + monthsBetween	HIGH severity finding H1 (division by zero / negative SMM beyond 1/r months); periodsBetween removed from Lib exports → library-API break, not in CHANGELOG (M4); basic tests added
225b152 / aa5ec33	version bumps 0.52.6/0.52.7	fine
1742d65	Docker/CI arm64	HIGH — change does not produce working arm64 images (H2)
8d1209c	#327 priceBonds unknown bond → Left	good; UT added (DealTest +20)
7a7888e	#331 mflow* accessors total for non-mortgage rows	good; UT (testMflowAccessors)
e1b8634	#325 Weekly/BiWeekly/Daily period handling	good; UT added
cb73353	head/tail hardening (Util/DateUtil/Stmt/Analytics/Asset/Liability/…)	good direction; wide blast radius but all via defaults with UTs; many -Wx-partial sites remain repo-wide (tracked)
6f573a0	#333 lookupM/lookupVs + pre-run name validation	good; behavior change = previously-crashing inputs now descriptive; UT added
d85cd94	#335 seeded foldl for agg rules	good; UT added
cf050b2	#346 boundedParMap + worker cap	good; UT added; see H3 interplay (boundary forcing)
1ee1ff9	#348 version from Paths_Hastructure	good; committed swagger.json must be regenerated on bumps (L4)
f046cb6 / a6554a8	#352 delete Validation.hs; cabal dedupe	fine
f8b5966	"UT for #4321"	tests fine; message references a non-existent issue (L1)
ea87daa	bump 0.52.8	fine
d0f2674 (empty msg)	#353 NonPerfAssumption defaults + -Werror=missing-fields + fixtures	good; UT (nonPerfAssumptionTest); empty message (L1)
bc85499 (empty msg)	#346 follow-up (resolveWorkerCap/env)	superseded by 6ea703b; empty message (L1)
59623c2 (empty msg)	#328 strict-canonical TxnComment codec + roundtrip tests	good; wire-visible canonicalization of 4 tags + strict rejection of legacy forms — documented in CHANGELOG; empty message (L1)
6ea703b (empty msg)	#345 ServerConfig + config.yml schema + per-endpoint caps + worker precedence	good, well-tested pure logic; config.yml is a deployment interface change (M5); empty message (L1)
a48a6c6	#344 Warp JSON error boundary	good first install, but covers app-phase only — value-level escapes still abort mid-stream; no automated UT; CHANGELOG overstates (H3)
b627079	#329 handlers total over request types; -Werror=incomplete-patterns on exe; 400s for map endpoints	good and compile-enforced; but 400 bodies are plain text while all other errors are JSON (M1); no transport-level UTs for the new paths
(local) 67c7d64	#332 totalize cashflow aggregation	not pushed yet — include before merge

---

High severity

H1. PrepaymentABS divides by zero / produces negative SMM for normal inputs

src/Asset.hs:191-201: smm m = r / (1 - r * fromIntegral (pred m)) with Rate = Rational (division by zero = runtime exception, not a nice error).

• The formula is the correct conversion if r is a monthly rate applied to the original balance — but then the outstanding balance implied by 1 - r*(m-1) hits zero at month 1 + 1/r:
  • r = 2% → month 51: denominator exactly 0 → crash (Ratio has zero denominator).
  • r = 6% → month 18: denominator -0.02 → SMM = -3 (silently corrupt projection!).
  • r = 10% → month 11 crash; r = 1.0 (allowed by the guard) → month 2 crash.
• Current UTs only exercise months 1–4 (absDs = Jul–Oct 2021) — the failing region is untested.

Recommend: validate r * (maxMonthAge - 1) < 1 at buildPrepayRates time (return descriptive Left), and/or clamp SMM to (0,1]; document r semantics; add tests crossing 1/r months (e.g. 2% with a 60-month schedule).

H2. Docker/CI arm64 path does not produce working arm64 images

.github/workflows/docker-image-dev-by-tag.yml (single ubuntu-latest = amd64 runner, platforms linux/amd64,linux/arm64) + Dockerfile:

1. The workflow passes build-args: CFLAGS=…, CPPFLAGS=… but the Dockerfile never declares ARG CFLAGS/CPPFLAGS — the args are ignored.
2. Even if declared, the values are set via export inside a RUN case … — shell exports do not persist to the later RUN cabal install layer.
3. FROM --platform=$BUILDPLATFORM haskell:… AS build pins the build stage to the build platform (amd64 on ubuntu-latest) → GHC produces an amd64 binary for both targets, and the per-target ubuntu final stage copies that amd64 binary into the arm64 image → exec format error at runtime.
   Recommend: either drop the --platform=$BUILDPLATFORM pin (build under QEMU per target) or split into a native per-arch matrix (ubuntu-24.04-arm) with a manifest merge; declare ARG/ENV CFLAGS CPPFLAGS properly; add a CI smoke step (docker run the arm64 image and hit /version).

H3. #344 boundary covers the app-phase only; the dominant engine-error class still aborts mid-stream

Verified against warp-3.4.9 sources: istatus flips before sendResponse. Exceptions while forcing a lazily-built response body (which is where wrapRun/runDeal Eithers are evaluated — Aeson forces them during body writing) cannot be re-rendered — the connection aborts, only the log records it. Consequences:

• The header/CHANGELOG claim ("exceptions escaping the request pipeline now answer 500…") is broader than what is achieved; No exception boundary at the server: pure errors escape as generic HTTP 500 #344 was closed with this gap open.
• The proposed completion (force responses to WHNF inside the handler action — return $! wrapResult / forceRespMap for the boundedParMap maps) is not part of this PR; a self-review addendum is posted on the issue.
• No automated UT for the boundary itself.
  Recommend: implement the WHNF-forcing pass (small, ~11 sites), correct the CHANGELOG wording, add a unit/golden test for errorBoundary (e.g. via responseToStream), or reopen No exception boundary at the server: pure errors escape as generic HTTP 500 #344 until done.

---

Medium

• M1. Plain-text 400 bodies. enforceRunCap and badRequest use err400 { errBody = cs msg } — Servant renders that as a plain body with no JSON envelope, while every other error path is JSON ({"Left":…} in-band, and the No exception boundary at the server: pure errors escape as generic HTTP 500 #344 500 envelope). Clients now see three error encodings. Recommend a shared helper that JSON-encodes the message ({"Left":…} / errHeaders=[json]).
• M2. Reverted fee double-count fix. 9d87a96 (drop + fd in the two calcDueFee FeeFlow branches) was fully reverted with its tests. If the double count is real, it is unfixed and untested; if the diagnosis was wrong, the revert should say so. Please open/close an issue either way.
• M3. Pool DefaultByAmt fix is mortgage-only. allocateDefaultByAmt's catch-all replicate (length balances) assumps still applies the full total per asset for Loan/Installment/Lease/Receivable/FixedAsset/ProjectedCashFlow pools — the over-allocation the commit fixes for mortgages persists there (if those types accept DefaultByAmt). Extend or document.
• M4. periodsBetween removed from Lib exports (replaced by monthsBetween). Source-breaking for library consumers; CHANGELOG doesn't mention it (and the CPPR/ABS semantics of monthsBetween — clipped partial months — deserve a line too).
• M5. config.yml schema change is a deployment interface change. maxScenario/maxDealNum are gone (silently ignored now via generic aeson), and invalid configs now abort startup instead of falling back to 8081 (intended, changelogged as BEHAVIOR CHANGE ✓). Consider logging ignored legacy keys; document that defaults are now per-endpoint caps of 1000.

---

Low / hygiene

• L1. Four empty commit messages (d0f2674, bc85499e, 59623c2, 6ea703b7) and one referencing a non-existent issue (f8b5966 "UT for #4321"). Squash/describe before merge.
• L2. UT.LedgerTest is imported by test/MainTest.hs but missing from the cabal other-modules — -Wmissing-home-modules on every test build; add it.
• L3. prorataFactors with negative individual balances produces negative allocations (only total ≤ 0 is guarded). Edge case; decide semantics.
• L4. swagger.json is a committed, runtime-generated artifact: version bumps require regeneration (currently 0.52.10 ✓). A CI check (git diff --exit-code swagger.json after a build) would prevent drift.

---

Servant API — what absbox needs to adapt

• No route/schema breaks: paths, request/response types and JSON shapes for existing inputs are unchanged. Happy-path clients need no changes.
• New behaviors to handle (optional client work):
  1. HTTP 400 responses (plain text today) for: per-request run caps (config.yml maxScenario/maxDealNum are ignored; scenario/deal combos are unbounded #345) and misrouted payloads (app/Main.hs scenario/multi-deal handlers are non-exhaustive over RunDealReq #329).
  2. HTTP 500 with JSON {"Left":"engine error: …"} + CORS for app-phase exceptions (No exception boundary at the server: pure errors escape as generic HTTP 500 #344) — plus the documented mid-stream abort residual (H3).
  3. New PrepaymentABS variant accepted in prepayment assumptions (monthly pools only).
  4. Deployment: new config.yml keys (maxRunNum.<endpoint>, workerNum); old keys ignored; invalid config aborts startup.
  5. Worker cap precedence: HASTRUCTURE_WORKERS > workerNum > -N.

UT coverage — assessment

Well covered: prorata (LibTest), pool DefaultByAmt incl. exceeds-balance (PoolTest +155), PrepaymentABS basic vectors/validation (AssetTest), mflow* accessors, NonPerfAssumption defaults, resolveWorkerNum/checkRunNumLimit/run-limits (UtilTest), #327/#325/#331/#333/#335/#346/#328 (roundtrip over 38 witnesses) and #332 (8 cases) — 520/520 green.

Missing / edge cases to add:

1. PrepaymentABS beyond 1/r months (the crash/negative-SMM region) — H1; highest-value missing test.
2. No exception boundary at the server: pure errors escape as generic HTTP 500 #344 boundary: no automated test (golden test of errorBoundary body/status/CORS, or an integration harness); plus a value-level (lazy body) test to pin the H3 gap after forcing is implemented.
3. HTTP-transport tests for the 400 paths (enforceRunCap, badRequest) — pure logic is tested, the response rendering is not; also assert the content-type/envelope consistency from M1.
4. Docker arm64 smoke test — at minimum run the built image and hit /version per platform.
5. Fee double-count — whatever the resolution of M2 (fix or wontfix), it currently has zero coverage in the merged state.
6. Non-mortgage pool DefaultByAmt allocation behavior (M3) — either a test proving the replicate path is intended, or extend the fix.
7. Negative-balance prorata edge (L3).

Recommended before merge

1. H1 fix + tests (blocking — silent corruption/crash in the new feature).
2. H2 Docker fix + per-platform smoke (blocking for the arm64 deliverable).
3. H3: implement handler-level forcing or correct the changelog/issue state explicitly.
4. M1 decision (JSON-envelope 400s) and M2/M3/M4 follow-up issues or fixes.
5. Hygiene: describe the empty-message commits; add UT.LedgerTest to cabal; push 67c7d64 (sumTs/sumTsCF/firstDate crash on empty input #332) into the PR.

[yellowbean]

Re-review — after the review round (33 commits, all pushed)

Branch state: base master → head 0d16b9e; the five review-round commits are now in the PR (remote head matches local). Suite 529/529 green; lib + exe build clean. No route/endpoint/API-type changes in the new commits.

1. Status of the previous review's findings

finding	status	evidence
H1 PrepaymentABS zero-denominator / negative SMM	✅ fixed (67a3e69)	smm returns 0 when 1 - r*(m-1) <= 0 (asset month age m); 3 new UTs incl. the exact exhaustion month (2% → m=51), the negative region (6% → m=18) and the 100% edge. Residual SMM > 1 before exhaustion tracked in #358
H2 Docker arm64 image carried an amd64 binary	✅ fixed (77ce119)	native per-arch matrix (ubuntu-latest + ubuntu-24.04-arm), push-by-digest + manifest merge, per-platform smoke test (/version), dead CFLAGS block and unused build-args removed; YAML validated. CI-only — cannot be exercised locally (registry unreachable); first a* tag validates. Runner-access caveat documented in the workflow header
H3 #344 boundary covered app-phase only	✅ fixed (019208c)	handlers force results ($!; multi-run maps via new Util.forceMapValues); live proof: value-level error gave http 000 / empty reply before, 500 {"Left":"engine error: boom"} + CORS after; 3 UTs incl. exception-propagation
M1 plain-text 400 bodies	✅ fixed (0d16b9e)	badRequestJSON (in lib, testable) renders {"Left":"…"} + application/json for both enforceRunCap and badRequest; 3 UTs. One error envelope now across 200/400/500
M3 pool DefaultByAmt fix is mortgage-only	❌ open	allocateDefaultByAmt catch-all (Pool.hs:335) still replicates the full total per asset for non-mortgage pools
M4 periodsBetween removed from Lib exports	❌ open	not in CHANGELOG yet
M5 config schema/startup behavior	✅ documented	BEHAVIOR CHANGE entries present
L1 4 empty commit messages + #4321	❌ in history	squash/rename before merge if desired
L2 UT.LedgerTest missing from cabal	❌ open	-Wmissing-home-modules still printed on every test build
L3 negative-balance prorata	❌ open	noted
L4 swagger.json drift	✅ consistent today	cabal 0.52.10 = swagger 0.52.10; a CI drift check would keep it so

2. Per-commit review — the five new commits

commit	area	verdict
67c7d64	#332 totalize cashflow aggregation	good — dead sumTs deleted, sumTsCF → NonEmpty (source break documented), firstDate empty → begin date + DummyCF → epocDate, unreachable error clause removed, CashFlow zero clauses added; in-tree behavior unchanged (callers were guarded); 8 UTs
67a3e69	#323 PrepaymentABS exhaustion	good — crash/negative-SMM region now 0; pre-exhaustion values bit-identical; residual over-reporting (SMM > 1) tracked in #358; 3 UTs
77ce119	H2 Docker/CI arm64	good — see above; no Haskell impact
019208c	H3 response forcing	good — see above; multi-run TTFB now waits for all scenarios (total time unchanged)
0d16b9e	M1 JSON 400s	good — see above; also removed the now-unused cs import

3. Fresh health metrics (clean -Wall build today)

• -Wx-partial 69 (steady) — partial-function family still tracked.
• -Wmissing-methods 24 (steady) — Typeclass instances leave methods unimplemented (runtime no instance method errors) #330 open.
• -Wincomplete-record-updates 15 (steady) — Incomplete record updates on sum-typed records (15 sites) #354 open.
• -Wincomplete-patterns 218 under -Wall — dominated by hygiene noise (-Wunused-matches 1477, -Wunused-imports 469, -Wname-shadowing 216, -Worphans 177) across Cashflow.hs/DealAction.hs/Mortgage.hs — pre-existing, not introduced by this PR.
• One small latent found under -Wall: app/Main.hs:152 (patchCumulativeToPoolRun) lambda-matches CF.CashFlowFrame _ txns, but CashFlowFrame also has DummyCF — unreachable today (no DummyCF construction sites) and, if it ever fired, the H3 forcing would now envelop it. Same family as Incomplete record updates on sum-typed records (15 sites) #354; noted for the record.

4. Client impact (absbox) — updated

• No route/type/schema changes anywhere in the PR; success responses unchanged.
• Error contract is now uniform: {"Left": "…"} with application/json on 200 (in-band), 400 (rejections), 500 (boundary + CORS). The Python wrapper needs no mandatory change; an optional error_message(resp) helper can parse the envelope uniformly.

5. UT coverage — updated

• Added this round: sumTs/sumTsCF/firstDate crash on empty input #332 (8 cases), PrepaymentABS exhaustion/edge (3), forceMapValues (3), badRequestJSON (3) → suite 529.
• Still missing (same list as before, narrowed):
  1. fee double-count (M2) — zero coverage in the merged state until decided;
  2. HTTP-transport level tests for 400/500 paths (helpers are UT'd; end-to-end needs a deal/pool JSON fixture — none in repo);
  3. M3 non-mortgage DefaultByAmt behavior;
  4. Incomplete record updates on sum-typed records (15 sites) #354 record-update crash paths (open issue; would be enveloped by the H3 forcing if hit, but should be totalized);
  5. Docker smoke runs only in CI (per-platform, in-workflow) ✓ — plus a manifest inspect in the merge job.

6. Recommended before merge

1. M2 — open/close an issue for the fee double-count (fix or wontfix); it currently reverts silently.
2. M3 — extend allocateDefaultByAmt beyond MortgageAssump or document the restriction (small).
3. M4 — one CHANGELOG line: periodsBetween → monthsBetween (library-API break, clipped partial months).
4. L2 — add UT.LedgerTest to the test other-modules (one line, silences a per-build warning).
5. L1 — squash/rename the four empty-message commits and the #4321 reference (history hygiene; optional if the merge is squash-merge anyway).
6. Optional: fold L3 into PrepaymentABS: SMM can exceed 1 before exhaustion, over-reporting prepayment vs outstanding balance #358 or a small issue.

Tracked-but-open work that does not block this merge: #354 (record updates), #330 (missing methods), the x-partial family, #358 (PrepaymentABS over-reporting residual).

[yellowbean]

M4 / L1 / L2 / L3 fixed

• M4 — CHANGELOG line added under 0.52.6 documenting the library-API removal of Lib.periodsBetween in favor of Lib.monthsBetween (clipped partial months).
• L2 — UT.LedgerTest added to the test-suite other-modules; the -Wmissing-home-modules warning is gone from every test build (its bookTest was already registered and running).
• L3 — prorataFactors now clamps negative balances to zero weight instead of producing negative allocations; sums remain exact; 2 new UTs (single negative, all-negative) → suite 531/531.
• L1 — the four empty-message commits and the #4321 reference have been reworded (content-identical rewrite; tree unchanged). Note this rewrote branch history, so all commit SHAs changed:

old	new
d0f2674	5193636 (Fix#353)
bc85499	2ee3864 (Fix#346 worker-cap env)
59623c2	4093f29 (Fix#328 codec)
6ea703b	5ce395b (Fix#345 ServerConfig)
f8b5966	3bb176a (collected-CF safety UTs)
67c7d64	(Fix#332 — message kept, new SHA: e33a7d9)
67a3e69	(Fix#323 — message kept, new SHA: ec21f10)
77ce119	(H2 CI — new SHA: d7b1565)
019208c	(H3 forcing — new SHA: c929825)
0d16b9e	(M1 — new SHA: 9be2aee)
0026349 (the L2/L3/M4 fix)	e24c500

Previous review comments referencing the old SHAs map through the table above.

[yellowbean]

M2 resolved — not a bug

The fee "double-count" fix (9d87a96, dropping + fd in the two calcDueFee FeeFlow branches) and its full revert (639b85f) were intentional: the + fd term is the correct semantics for FeeFlowByPoolPeriod / FeeFlowByBondPeriod (the originally-accrued due belongs on top of the period-computed amount), so the revert stands as the correct behavior — no tracking issue needed.

M2 is closed as not a bug; the only open items on this PR remain the tracked backlog (#354 record updates, #330 missing methods, x-partial family, #358 PrepaymentABS over-reporting residual), none of which block merge.