Skip to content

fix(api): reject out-of-range assumptions where they enter, not two calls later (graam-harmony #4476) - #61

Open
selkordy wants to merge 1 commit into
mainfrom
fix/4416-assumption-range-validation
Open

fix(api): reject out-of-range assumptions where they enter, not two calls later (graam-harmony #4476)#61
selkordy wants to merge 1 commit into
mainfrom
fix/4416-assumption-range-validation

Conversation

@selkordy

Copy link
Copy Markdown
Contributor

Tracking issue: graam-harmony #4476 — full causal chain and the DelinqAdvPct finding are there.

The defect

A user typed 1000 into the "Prepays" (CPR) field on the deal Scenarios screen. What happened:

  1. POST /api/calccollateral accepted assumptions.cpr = 1000. There was not a single [Range], IValidatableObject or ModelState check anywhere in src/.
  2. CfCore.BuildAssumptionArray de-annualized it as 1 - Pow(1 - 1000/100, 1/12) = Pow(-9, 1/12) = NaN.
  3. Amortizer.cs "clamps" with Math.Clamp(smm, 0, 1) — a complete no-op for NaN, since both comparisons are false. if (balance < 1) break is NaN-blind too, so every period was emitted as NaN, not just one.
  4. Program.cs sets JsonNumberHandling.AllowNamedFloatingPointLiterals, so the endpoint returned 200 OK with NaN literals in the body.
  5. The client posted those cashflows to POST /api/waterfall, which finally 400'd with: Collateral cashflow at index 0 (2026-08-25) has a non-finite UnscheduledPrincipal value.

The error named the symptom, came from the wrong endpoint, two service calls downstream, and never mentioned the input the user actually got wrong.

This is verified, not inferred: reverting src/ on this branch and re-running the new tests reproduces it — the cpr = 1000 case returns OkObjectResult, not BadRequestObjectResult.

What happens now

assumptions.cpr = 1000 is out of range. CPR is an annual percentage between 0 and 100
(6 means 6% CPR). A value above 100 is usually a units mistake — a PSA speed, basis
points, or a fraction that was already multiplied by 100.

1. Boundary validation — src/GraamFlows.Api/Validation/AssumptionValidation.cs

Public static, returns string? (null = valid), mirroring the WaterfallController.ValidateRequest convention. Wired into CalcCollateralController.Calculate before the try. The response shape stays exactly { error = "<string>" } — a downstream Python client parses that key.

Every value must be finite and within [0, 100], across:

  • deal-level scalars cpr, cdr, severity, delinquency, advancing;
  • their vector forms — the message names the index (assumptions.cdrVector[3] = 4000 is out of range…);
  • the same fields on every assetAssumptions entry — the message names the asset key (assetAssumptions['LOAN-000042'].cdr = 900…), which over a loan-level tape of thousands of rows is the only thing that makes it findable.

Unit wording follows the declared type, because the same field means different things:

declared message says
prepaymentType: CPR "CPR is an annual percentage between 0 and 100 (6 means 6% CPR)."
prepaymentType: SMM "cpr is a MONTHLY prepayment hazard in percent … — not an annual CPR."
prepaymentType: ABS "cpr is a percentage of the ORIGINAL balance prepaying each month …"
defaultType: MDR "cdr is a MONTHLY default hazard in percent … — not an annual CDR."
defaultType: ORIGMDR "cdr is a MONTHLY default rate on the ORIGINAL balance …"

Non-finite gets its own wording: assumptions.cpr must be a finite number; got NaN.

Note on the ABS wording. The brief for this PR described ABS as an annual percentage of the original balance. BuildAbsAssumptionArray computes SMM = 100·ABS / (100 − ABS·(n−1)), which at n=1 gives SMM = ABS — i.e. a monthly percentage of the original balance, and harmony's own skill guidance agrees ("a fixed percentage of ORIGINAL balance each month"). Since these strings are read verbatim by users, the message says monthly. Flagging the deviation explicitly. (The ABS enum member's own trailing comment in PrepaymentTypeEnum.cs also says "annual" and is worth a follow-up.)

2. Unknown type strings are now rejected — census first

ParsePrepaymentType returns CPR for any unrecognized string, so "PSA" or "PercentCPR" was silently mis-modelled as CPR — a wrong answer with no error at all. Rejecting is a behaviour change, so it was gated on a census of both consumer repos:

repo finding
graam-web zero occurrences of prepaymentType / defaultType / calccollateral. The web app never sends either field; it goes through harmony.
graam-harmony every producer is closed over the safe set. sss_to_assumptions.py:68 is a {"CPR","ABS","SMM"} dict lookup defaulting to "CPR"; :115/:129 emit only ORIGMDR/MDR/CDR. cashflow_analyzer.py:196 and wal_validator.py:1116 are "CPR" if … else "ABS"; wal_validator.py:1485 is a literal "CPR". project.py:1964-65 sends "SMM"/"MDR". project.py:1471 and :1921 set the key only when the value is exactly "ABS", so the agent-supplied params.get("prepayment_type") at :1627/:2164 cannot leak an arbitrary string onto the wire. client.py:221-224 is a pass-through, but every in-repo caller feeding it is in the list above.

Every observed value is in {CPR, ABS, SMM} / {CDR, MDR, ORIGMDR} or the field is omitted, so the rejection was added. No live caller changes behaviour; case-insensitive matching and the omitted default are covered by tests.

Worth a follow-up, not fixed here: PrepaymentTypeEnum declares PercentCPR and PSA members that ParsePrepaymentType has no path to, so they are unreachable from the API today.

3. The NaN factory is gone — MathUtil.AnnualPercentToMonthlyHazard

One canonical de-annualization: saturates to 1.0 at/above 100 (matching today's exact-100 result, since Pow(0, 1/12) == 0) and to 0.0 at/below 0. NaN is deliberately not special-cased — the boundary validator rejects it, so a NaN reaching the engine is an engine bug and should stay loud rather than be silently zeroed into a plausible-looking cashflow. That choice is documented in the XML doc.

No tie-out number moves. The in-range expression is literally 1.0 - Math.Pow(1.0 - annualPercent / 100.0, 1.0 / 12.0); / 100.0 was not rewritten as * .01. A test asserts bit identity (BitConverter.DoubleToInt64Bits) against the original inline expression across 0, 0.5, 6, 25, 99.9, 100, so a future algebraic tidy-up fails loudly instead of silently repricing every deal. The only inputs whose behaviour changes are ones the old expression could not evaluate meaningfully: above 100 (was NaN) and below 0 (was a negative hazard the amortizer already clamped to 0).

CfCore now calls it. Note that BuildAssumptionArray and BuildReinvestAssumptionMatrices are not two copies of the expression — the reinvestment cohort path calls BuildAssumptionArray, so both de-annualization sites are fixed by the single edit.

ConvertToSmm was measured, and left alone. It uses cpr * .01 where the helper uses / 100.0. Over a 15,000,001-point sweep of [0, 100] (a 10M-point grid plus 5M random draws) 603,710 values disagreed, the widest by 115,223 ulps just below 100 where 1 - x/100 cancels hardest. So it is not bit-identical and deduping it would move numbers. Both functions now carry a comment explaining that they differ by rounding and that only AnnualPercentToMonthlyHazard is on the engine's assumption path. (A 12-point spread of round numbers showed zero disagreement — the wide sweep is what caught it.)

4. The downstream message points upstream

WaterfallController.ValidateRequest still fires as a backstop, but now adds: "Non-finite collateral almost always means an out-of-range assumption produced it upstream — check that CPR, CDR and severity are percentages between 0 and 100 (6, not 1000) and re-run the collateral projection."

Companion PRs

This fix spans three repos:

  • graam-harmony: the engine's sentence now reaches the reader instead of its JSON envelope, and an engine rejection maps to HTTP 400 (503 when the engine is unreachable) instead of 500. That is what makes message quality here load-bearing rather than decorative — the strings in AssumptionValidation are read verbatim by users on the deal screen.
  • graam-web: client-side field validation stops cpr=1000 before it is ever sent.

Tests

dotnet test: 217 passed / 0 failed before → 271 passed / 0 failed after (+54, no regressions).

Reverting src/ and re-running the new tests turns 23 of the 54 red, including the headline cpr = 1000 case, which fails with: "Expected type to be BadRequestObjectResult … but found OkObjectResult." The other 31 are the must-not-regress guards (boundary values 0 and 100 accepted, an ordinary request still 200s with non-empty cashflows, every recognized type string in any casing accepted, in-range bit identity) and pass either way by design.

  • Unit/Api/AssumptionRangeValidationTests.cs — pins that each message names the field, the value and the bound, not merely that it is non-empty; vector index; per-asset key; NaN/±Infinity; unit wording per declared type; unknown-type rejection.
  • Unit/AssetCashflowEngine/NonFiniteHazardTests.cs — engine-level regression: a small pool run straight through CfCore/Amortizer with rates just above 100 must emit only finite UnscheduledPrincipal / DefaultedPrincipal / Balance. This is the Math.Clamp(NaN) no-op guarded, plus a check that in-range rates are untouched.
  • Unit/Util/MonthlyHazardConversionTests.cs — the bit-identity tie-out guard and the saturation/NaN-propagation contract.

Separate finding — reported, deliberately NOT fixed here

delAdvIntTime / delAdvPrinTime are built with BuildAssumptionArray(..., convertToMonthly: false, divisor: 1.0, defaultValue: 100.0) — divisor 1.0, where sevTime and delTime next to them use 100.0. AssetAssumptions also defaults DelinqAdvPctInt = new ConstVector(100). So the value stays in percent while Amortizer.cs:407-411 consumes it as a fraction (1 - delAdvInt), giving 1 - 100 = -99.

Confirmed empirically on a $1,000,000 6% FRM, period 0:

delinquency advancing interest unadvancedInterest correct?
0% 100 5,000.00 0.00 masked — DQ=0 hides it entirely
10% 100 54,500.00 −49,500.00 no — 10.9× the $5,000 actually due
10% 0 4,500.00 500.00 yes — (1 − 0) = 1 is the only correct case
10% 50 29,500.00 −24,500.00 no

unadvInterest = interest × del × (1 − delAdvInt) = 5000 × 0.10 × (1 − 100) = −49,500, then interest -= unadvInterest inflates interest above what is owed. Negative unadvanced principal follows the same way. It is masked whenever DQ is 0 — which is what most harmony paths send (cashflow_analyzer, wal_validator both pass delinquency: 0.0), which is why it has not surfaced.

Not fixed in this PR: it changes numbers for every DQ>0 run and needs its own measurement against the reference oracle.

🤖 Generated with Claude Code

…alls later (graam-harmony #4476)

A user typed 1000 into the Prepays (CPR) field on the deal Scenarios screen.
/api/calccollateral accepted it with no validation of any kind;
CfCore.BuildAssumptionArray de-annualized it as Pow(-9, 1/12) = NaN; the
amortizer's Math.Clamp(smm, 0, 1) could not clamp it (every comparison against
NaN is false) and `if (balance < 1) break` never fired, so every period came
back NaN; and because Program.cs enables AllowNamedFloatingPointLiterals, the
endpoint returned 200 OK with NaN in the body. The mistake only surfaced one
service call later, as a non-finite-cashflow rejection from /api/waterfall that
named the symptom, came from the wrong endpoint, and never mentioned the input
the user actually got wrong.

- AssumptionValidation rejects non-finite and out-of-[0,100] values at the
  boundary, across deal-level scalars, their vector forms and every per-asset
  override, naming the field path, the value, the bound, the unit convention
  and the likely mistake. Vector messages name the index; per-asset messages
  name the asset key. Unit wording follows the declared prepaymentType /
  defaultType, because cpr under SMM is a monthly hazard, not an annual CPR.
- Unknown prepaymentType / defaultType strings are now rejected rather than
  silently modelled as CPR / CDR. A census of graam-harmony and graam-web found
  every value actually sent is CPR/ABS/SMM and CDR/MDR/ORIGMDR, or the field
  omitted, so no live caller changes behaviour.
- MathUtil.AnnualPercentToMonthlyHazard is the one canonical de-annualization
  and saturates at 0 and 1 instead of manufacturing NaN. The in-range
  expression is byte-for-byte unchanged and pinned by a bit-identity test, so
  no tie-out number moves. NaN is deliberately not special-cased: the boundary
  validator rejects it, so an internal NaN is an engine bug and stays loud.
- The /api/waterfall non-finite message stays as a backstop but now names the
  class of cause, so a reader who lands on it is pointed upstream.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@selkordy

Copy link
Copy Markdown
Contributor Author

End-to-end verification against two live engines

I built this branch and ran it on port 5201, alongside the unpatched production engine already running on 5200, and put the same request to both.

The fat-fingered request (assumptions.cpr = 1000, one $900k 6% FRM loan):

old engine (5200) this branch (5201)
status HTTP 200 HTTP 400
body 279 cashflow rows one sentence
row 0 unscheduledPrincipal NaN
row 0 balance NaN

The old engine's answer is the whole defect in one line: 200 OK, 279 rows, every one of them NaN, handed to the caller as if it were data. What this branch returns instead:

assumptions.cpr = 1000 is out of range. CPR is an annual percentage between 0 and 100
(6 means 6% CPR). A value above 100 is usually a units mistake — a PSA speed, basis
points, or a fraction that was already multiplied by 100.

The no-regression check that mattered most. Same request with cpr = 6.0, both engines, full field-by-field comparison of all 279 rows:

row counts: 279 279
BIT-IDENTICAL

Not "close enough" — every field of every row compares equal. That is the constraint about keeping / 100.0 and not rewriting it as * .01, confirmed on real cashflows rather than on a unit test of the helper alone.

The downstream backstop message, feeding the old engine's NaN rows to each engine's /api/waterfall:

  • old: Collateral cashflow at index 0 (2026-08-25) has a non-finite UnscheduledPrincipal value.
  • this branch: same, plus Non-finite collateral almost always means an out-of-range assumption produced it upstream — check that CPR, CDR and severity are percentages between 0 and 100 (6, not 1000) and re-run the collateral projection.

Through the harmony client, pointed at 5201, which is the path a real user's click takes:

str(exc)    : CalcCollateral HTTP 400: assumptions.cpr = 1000 is out of range. CPR is an
              annual percentage between 0 and 100 (6 means 6% CPR). ...
status_code : 400

That string is what reaches the deal screen once GraamOrg/graam-harmony#4477 lands, so the wording in this PR is what an end user literally reads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant