diff --git a/.agents/logs/2026-07-28.jsonl b/.agents/logs/2026-07-28.jsonl new file mode 100644 index 00000000..1f8f0957 --- /dev/null +++ b/.agents/logs/2026-07-28.jsonl @@ -0,0 +1 @@ +{"agent": "feature-flow", "session_id": "top01-phase-b-20260727", "triggered_by": "user-direct", "started_at": "2026-07-27T20:00:00Z", "timestamp": "2026-07-28T00:00:00Z", "duration_ms": null, "root_cause_category": "Financial Math Verification", "findings": [{"severity": "defect", "description": "prei/pipeline/handlers/underwriting.py was a second, fully float-based implementation of NOI/cap-rate/cash-on-cash living outside services/utils — AGENTS.md Never-Do #1 and #3 violation. Fixed by converting to Decimal and importing the canonical cap_rate/cash_on_cash from investor_app.finance.utils."}, {"severity": "defect", "description": "ref_one_percent_rule/ref_gross_rent_multiplier in tests/finance_reference.py silently returned a value on zero/negative input instead of raising ValueError like production, so those edge cases could diverge undetected between reference and production."}, {"severity": "defect", "description": "mypy's pre-commit hook (broader scope than the CI-only mypy core/ investor_app/finance/ command) flagged bare int/float literals passed to UnderwritingInput's now-Decimal fields in prei/pipeline/tests/test_underwriting.py and tests/test_underwriting_integration.py — fixed by wrapping literals in Decimal(...); pydantic coerces them fine at runtime but the pydantic mypy plugin isn't enabled in mypy.ini."}, {"severity": "note", "description": "Implemented Phase B of docs/TOP_01_PLAN.md: B-1 (IRR reference implementation, bisection-based, no numpy dependency), B-2 (expanded edge-case coverage to 50+ cases per function, 546 total), B-4 (mathematical derivation docstrings on noi/cap_rate/cash_on_cash/dscr/irr). B-3 (CI gate) was already wired. Bundled the user-approved underwriting.py float-to-Decimal fix. Recorded LIMIT-20 (bare-function vs calculate_* contract divergence, duplicate score_listing_v2) and LIMIT-21 (offer.py remains float currency) as deliberately out of scope. Archived Phase A's specification.md/design.md/tasks.json to features/top01-phase-a/ (merged but never archived) and regenerated root files for Phase B."}], "decision": "IMPLEMENTED", "blockers": [], "pr": "https://github.com/paruff/prei/pull/324"} diff --git a/design.md b/design.md index ac4f821d..68ecdf5c 100644 --- a/design.md +++ b/design.md @@ -1,46 +1,74 @@ -# Design: Phase A — CI/Test Quality Gaps - -### A-2: BDD pipeline suite over real HTTP -`tests_bdd/steps/pipeline_acceptance_steps.py` swaps `django.test.Client` for -an `httpx.Client` bound to pytest-django's `live_server.url`. `Given` steps -keep building fixtures via the ORM; `tests_bdd/conftest.py`'s `_reset_ctx` -fixture depends on `transactional_db` (not `db`) so rows committed by the test -process are visible to the live server's background-thread request handling. -POST steps fetch a CSRF token from the target form page first (`_csrf_token()` -helper) since httpx doesn't auto-handle Django CSRF the way the test client -does. - -### A-3: Acceptance suite runs pre-merge -`tests/acceptance/conftest.py`'s `base_url` fixture falls back to a -session-scoped `live_server` when `BASE_URL` is unset, lazily requested via -`request.getfixturevalue(...)` so `BASE_URL`-driven runs never touch Django's -DB fixtures. A separate autouse `_enable_db_for_live_server` fixture calls -`request.getfixturevalue("db")` per test function, since pytest-django blocks -DB access per-test regardless of a session-scoped fixture's own DB setup. -`ci-quality.yml`'s `acceptance-check` job drops `--collect-only` and runs the -suite for real, with `BASE_URL` intentionally unset. - -### A-4: Real build-time budget -`build-image`'s `job-start` step persists its epoch to `$GITHUB_ENV`. The -"Check build time" step computes the elapsed duration against that epoch and -fails (`::error::` + `exit 1`) past 600s. `timeout-minutes: 10` on the job -itself is a hard backstop independent of the soft check. - -### A-5: Response-shape validation -`schemas.py` gained two generic models — `LoginGateAssertion` -(`Literal[200, 302]`, for pages that redirect anonymous users to login) and -`NoCrashAssertion` (`status_code < 500`, for pages that must not error -regardless of auth state) — reused across the status-only files -(`test_brrrr.py`, `test_dashboard.py`, `test_pipeline.py`, `test_leasing.py`, -parts of `test_growth.py`/`test_property_pipeline.py`). Files with existing -purpose-built models (`LoginPageAssertion`, `DiscoveryPageAssertion`, -`StaticAssetAssertion` in `test_pages.py`; `GrowthAreasResponse` in -`test_growth.py`) now actually import and validate against them instead of -duplicating loose dict/status assertions. - -### Bugs surfaced by A-3 (fixed, not scope creep — this is what the new gate is for) -- `pipeline_list` view was missing `@login_required`, unlike sibling - `leasing_list`, causing a 500 instead of a redirect for anonymous access. -- `tests/acceptance/test_leasing.py` hardcoded a stale route (`/leasing/list/` - instead of `/leasing/`) that had never actually executed under - `--collect-only`. +# Design: Phase B — Financial Math + +### B-1: IRR reference implementation +`ref_irr(cashflows: list[Decimal]) -> Decimal` in `tests/finance_reference.py` +is independent of `numpy_financial` (unlike production's `irr()`, which wraps +it). It brackets a sign change in `NPV(r) = Σ cashflows[t] / (1+r)^t` over a +coarse grid (`r ∈ (-0.9999, 10)`, step `0.01`), then bisects within the +bracket to a `1e-7` tolerance. Returns `Decimal("0")` when no sign change is +found (no real root), mirroring production's existing NaN/Inf fallback. + +### B-2: Expanded edge-case coverage +`tests/test_finance_math.py`'s case lists (`NOI_CASES`, `CAP_RATE_CASES`, +`COC_CASES`, `DSCR_CASES`, `ONE_PCT_CASES`, `GRM_CASES`, new `IRR_CASES`) were +expanded to 50+ rows each, organized by category: normal/typical, zero in +each param position, negative in each param position, extreme magnitude, +currency sub-cent precision, boundary/threshold, and int-vs-Decimal coercion. +`ref_one_percent_rule`/`ref_gross_rent_multiplier` were updated to raise +`ValueError` under the same zero/negative conditions as production, so +zero/negative edge cases can't silently diverge between "production raises" +and "reference returns a value." + +### B-3: No workflow change +`ci-quality.yml`'s `finance-math` job already runs the whole of +`tests/test_finance_math.py`; B-1/B-2 adding IRR cases to that same file +extends the existing gate automatically. + +### B-4: Derivation docstrings +`noi`, `cap_rate`, `cash_on_cash`, `dscr`, `irr` in +`investor_app/finance/utils.py` gained full docstrings (formula + "Derivation:" +paragraph + Args/Returns), following the Args/Returns/Raises style already +used by `one_percent_rule`/`gross_rent_multiplier`. Those two also gained a +one-line derivation note for completeness. No function bodies changed — +docstrings only, verified via `ast.parse` + full test rerun. + +### Underwriting.py: float → Decimal, dedup +`UnderwritingInput`/`UnderwritingMetrics` (`prei/pipeline/handlers/underwriting.py`) +became `Decimal`-typed pydantic models — pydantic v2 coerces int/float/str into +`Decimal` fields natively, so existing bare-numeric call sites keep working +unchanged. The local duplicate `cap_rate()` was deleted; the module now +imports `cap_rate`/`cash_on_cash`/`to_decimal` from `investor_app.finance.utils` +directly. `cash_on_cash_yield()` keeps its distinct name and semantics +(all-cash acquisition yield: NOI over price+rehab, no debt service netted +out) but delegates its division through the canonical `cash_on_cash()` instead +of reimplementing `/` locally. The remaining composition helpers +(`gross_potential_rent`, `effective_gross_income`, `total_operating_expenses`, +`net_operating_income`, `max_allowable_offer`) converted their arithmetic to +`Decimal` via the reused `to_decimal()` helper; `solve_underwriting()` uses +`.quantize()` instead of `round()` for the final output. + +`orchestrator.py`'s `UnderwritingInput` construction boundary (`price * 0.012`/ +`price * 0.004` tax/insurance defaults) was wrapped in `Decimal("0.012")`/ +`Decimal("0.004")` arithmetic against a `to_decimal(canonical.price or 0.0)`- +coerced price, reusing `to_decimal()` rather than reinventing coercion. + +**Two non-obvious risks found during implementation:** +- `pytest.approx()` compared against a `Decimal` actual is + fragile, not uniformly broken — it silently short-circuits via exact + equality for representable values but raises `TypeError` on near-matches + (`abs(expected - actual)` can't mix `float` and `Decimal`). Every affected + assertion site was fixed by wrapping the Decimal actual in `float(...)` + rather than relying on which literals happen to match exactly. +- `Decimal * float` arithmetic (not just comparison) raises `TypeError` + unconditionally. Test-file call sites that computed derived values inline + (e.g. `uw.mao * 1.15`, `low.mao * 0.07 / 0.10`) needed `float(...)`-wrapping + of the Decimal operand before the float arithmetic. +`prei/pipeline/handlers/offer.py`'s `OfferInput.mao: float` field is untouched +by this — pydantic coerces a `Decimal` input to `float` automatically since no +arithmetic happens before construction at the remaining safe call sites. + +### Documentation-only additions +Two new `docs/KNOWN_LIMITATIONS.md` entries (LIMIT-20, LIMIT-21) record the +issues found but deliberately not fixed in this PR: the bare-function vs. +`calculate_*` contract divergence plus the duplicate `score_listing_v2` +functions, and `offer.py`'s remaining float-currency issue. diff --git a/docs/KNOWN_LIMITATIONS.md b/docs/KNOWN_LIMITATIONS.md index 98f2c033..3bba7212 100644 --- a/docs/KNOWN_LIMITATIONS.md +++ b/docs/KNOWN_LIMITATIONS.md @@ -212,6 +212,30 @@ This means a user who runs the API pre-`populate_growth_areas` gets empty result --- +### [LIMIT-20] 🟡 HIGH — Divergent bare-function vs. `calculate_*` contracts for the same formulas, plus a duplicate `score_listing_v2` + +**Location:** `investor_app/finance/utils.py` — `noi`/`cap_rate`/`cash_on_cash`/`dscr` vs. `calculate_noi`/`calculate_cap_rate`/`calculate_cash_on_cash`/`calculate_irr`; `investor_app/finance/utils.py:1830` vs. `core/services/scoring.py:110` (`score_listing_v2`). + +**Impact:** Two independent implementations exist for the same four KPI formulas — the bare functions return `Decimal("0")` on invalid input (e.g. zero purchase price, zero debt service), while the `calculate_*` variants raise `ValueError` under the same conditions. Callers that reach for the "wrong" variant get silently different failure behavior for identical bad input, and there is no single source of truth to point developers at. Separately, two functions both named `score_listing_v2` exist with different signatures — one in `investor_app/finance/utils.py`, one in `core/services/scoring.py` — an accident waiting to cause a wrong-function-imported bug. + +**Workaround:** None currently. Callers must know which variant (bare vs. `calculate_*`) they're calling and its error-handling contract; `score_listing_v2` callers must be careful to import from the intended module. + +**Fix tracked in:** Not yet filed. Found during Phase B (docs/TOP_01_PLAN.md) financial-math audit; out of scope for that PR since it requires an API-contract decision (which behavior is canonical) rather than a mechanical fix. + +--- + +### [LIMIT-21] 🟡 HIGH — `prei/pipeline/handlers/offer.py` remains float-based currency + +**Location:** `prei/pipeline/handlers/offer.py` — `OfferInput`, `OfferMetrics`, `solve_offer()` and its pricing-strategy multiplier/equity arithmetic. + +**Impact:** `OfferInput.mao`/`arv`/`rehab_budget` and `OfferMetrics.offer_price`/`estimated_equity` are `float`-typed, so all offer-price and equity math (strategy multipliers, premium calculations, equity clamping) is done in binary floating point rather than `Decimal`. This is the same class of currency-precision issue as the one fixed in `prei/pipeline/handlers/underwriting.py` during Phase B (docs/TOP_01_PLAN.md) — `AGENTS.md` "Never Do" item 3 ("Float persistence for currency") — but was deliberately left out of that PR's scope since the user approved only the underwriting.py fix, and converting `offer.py` also touches its downstream callers (`tests/test_offer_integration.py`, `tests/test_pipeline_e2e.py`, `tests_bdd/`) in ways that deserve their own reviewed change. + +**Workaround:** None — offer-price rounding/precision errors from float arithmetic are small in absolute terms (cents-level) for typical property values, so this is not currently causing observable defects, but the pattern should not be extended. + +**Fix tracked in:** Not yet filed. Recommended as a follow-up PR using the same `Decimal`/`to_decimal()` conversion approach applied to `underwriting.py`. + +--- + ## Resolved Limitations ### [LIMIT-R01] Docker container permissions — `app` user could not write `db.sqlite3` diff --git a/features/top01-phase-a/design.md b/features/top01-phase-a/design.md new file mode 100644 index 00000000..ac4f821d --- /dev/null +++ b/features/top01-phase-a/design.md @@ -0,0 +1,46 @@ +# Design: Phase A — CI/Test Quality Gaps + +### A-2: BDD pipeline suite over real HTTP +`tests_bdd/steps/pipeline_acceptance_steps.py` swaps `django.test.Client` for +an `httpx.Client` bound to pytest-django's `live_server.url`. `Given` steps +keep building fixtures via the ORM; `tests_bdd/conftest.py`'s `_reset_ctx` +fixture depends on `transactional_db` (not `db`) so rows committed by the test +process are visible to the live server's background-thread request handling. +POST steps fetch a CSRF token from the target form page first (`_csrf_token()` +helper) since httpx doesn't auto-handle Django CSRF the way the test client +does. + +### A-3: Acceptance suite runs pre-merge +`tests/acceptance/conftest.py`'s `base_url` fixture falls back to a +session-scoped `live_server` when `BASE_URL` is unset, lazily requested via +`request.getfixturevalue(...)` so `BASE_URL`-driven runs never touch Django's +DB fixtures. A separate autouse `_enable_db_for_live_server` fixture calls +`request.getfixturevalue("db")` per test function, since pytest-django blocks +DB access per-test regardless of a session-scoped fixture's own DB setup. +`ci-quality.yml`'s `acceptance-check` job drops `--collect-only` and runs the +suite for real, with `BASE_URL` intentionally unset. + +### A-4: Real build-time budget +`build-image`'s `job-start` step persists its epoch to `$GITHUB_ENV`. The +"Check build time" step computes the elapsed duration against that epoch and +fails (`::error::` + `exit 1`) past 600s. `timeout-minutes: 10` on the job +itself is a hard backstop independent of the soft check. + +### A-5: Response-shape validation +`schemas.py` gained two generic models — `LoginGateAssertion` +(`Literal[200, 302]`, for pages that redirect anonymous users to login) and +`NoCrashAssertion` (`status_code < 500`, for pages that must not error +regardless of auth state) — reused across the status-only files +(`test_brrrr.py`, `test_dashboard.py`, `test_pipeline.py`, `test_leasing.py`, +parts of `test_growth.py`/`test_property_pipeline.py`). Files with existing +purpose-built models (`LoginPageAssertion`, `DiscoveryPageAssertion`, +`StaticAssetAssertion` in `test_pages.py`; `GrowthAreasResponse` in +`test_growth.py`) now actually import and validate against them instead of +duplicating loose dict/status assertions. + +### Bugs surfaced by A-3 (fixed, not scope creep — this is what the new gate is for) +- `pipeline_list` view was missing `@login_required`, unlike sibling + `leasing_list`, causing a 500 instead of a redirect for anonymous access. +- `tests/acceptance/test_leasing.py` hardcoded a stale route (`/leasing/list/` + instead of `/leasing/`) that had never actually executed under + `--collect-only`. diff --git a/features/top01-phase-a/specification.md b/features/top01-phase-a/specification.md new file mode 100644 index 00000000..e6fcde94 --- /dev/null +++ b/features/top01-phase-a/specification.md @@ -0,0 +1,39 @@ +# Specification: Phase A — CI/Test Quality Gaps (docs/TOP_01_PLAN.md) +# Written: 2026-07-27 + +--- + +## 0. Problem + +`docs/TOP_01_PLAN.md` Phase A identifies 5 gaps between this repo's CI pipeline +and a genuinely trustworthy one: acceptance/BDD suites that don't exercise real +HTTP, a PR-gate acceptance job that only `--collect-only`s instead of running, +an unbounded Docker build step, and acceptance tests that only check status +codes instead of response shape. + +## 1. Requirements + +- A-1: `main-ci-guard.yml` blocks PR merges on Tier-2 (post-merge) failure. +- A-2: `tests_bdd/`'s pipeline acceptance suite drives real HTTP requests + (via pytest-django's `live_server`) instead of `django.test.Client`. +- A-3: `tests/acceptance/*.py` actually executes in the PR-gate tier + (`ci-quality.yml`), not just `--collect-only`, via a `live_server` fallback + when `BASE_URL` is unset. +- A-4: `docker-publish.yml`'s `build-image` job enforces a real 10-minute + build-time budget (soft check + hard `timeout-minutes` backstop). +- A-5: All `tests/acceptance/*.py` files validate response shape via + `schemas.py` Pydantic models, not just raw status codes. + +## 2. Acceptance Criteria + +| ID | Criterion | test_type | +|---|---|---| +| AC-A1-01 | `main-ci-guard.yml` fails the PR check when Tier 2 fails | ci | +| AC-A2-01 | `pytest tests_bdd/` passes using `live_server` + `httpx.Client` | unit | +| AC-A2-02 | POST-based BDD steps include a real CSRF token | unit | +| AC-A3-01 | `pytest tests/acceptance/` passes with no `BASE_URL` set (live_server fallback) | unit | +| AC-A3-02 | `ci-quality.yml`'s `acceptance-check` job runs tests for real, not `--collect-only` | ci | +| AC-A3-03 | `BASE_URL`-driven runs (`make test-acceptance`, `post-deployment.yml`) are unaffected | unit | +| AC-A4-01 | `build-image` job has `timeout-minutes: 10` | ci | +| AC-A4-02 | "Check build time" step fails the job if duration exceeds 600s | ci | +| AC-A5-01 | All 9 files in `tests/acceptance/` import and use `schemas.py` models | unit | diff --git a/features/top01-phase-a/tasks.json b/features/top01-phase-a/tasks.json new file mode 100644 index 00000000..3d8ac373 --- /dev/null +++ b/features/top01-phase-a/tasks.json @@ -0,0 +1,61 @@ +{ + "meta": { + "project": "prei", + "session": "top01-phase-a-20260727", + "date": "2026-07-27", + "feature": "Phase A — CI/Test Quality Gaps (docs/TOP_01_PLAN.md)", + "spec": "specification.md", + "design": "design.md" + }, + "tasks": [ + { + "id": "A-1", + "summary": "main-ci-guard blocks PR merge on Tier-2 failure", + "description": "Already implemented prior to this session; verified in .github/workflows/main-ci-guard.yml. No action taken.", + "depends_on": [], + "acceptance_criteria": [ + {"id": "AC-A1-01", "description": "main-ci-guard.yml fails the PR check when Tier 2 fails", "test_type": "ci"} + ] + }, + { + "id": "A-2", + "summary": "BDD pipeline suite drives real HTTP via live_server", + "description": "Rewrite tests_bdd/steps/pipeline_acceptance_steps.py to use an httpx.Client bound to pytest-django's live_server fixture instead of django.test.Client; switch tests_bdd/conftest.py's _reset_ctx to depend on transactional_db.", + "depends_on": [], + "acceptance_criteria": [ + {"id": "AC-A2-01", "description": "pytest tests_bdd/ passes using live_server + httpx.Client", "test_type": "unit"}, + {"id": "AC-A2-02", "description": "POST-based BDD steps include a real CSRF token", "test_type": "unit"} + ] + }, + { + "id": "A-3", + "summary": "Acceptance suite executes for real in the PR-gate tier", + "description": "Extend tests/acceptance/conftest.py's base_url fixture to fall back to a live_server when BASE_URL is unset; add an autouse fixture to unblock DB access per test; update ci-quality.yml's acceptance-check job to run pytest for real instead of --collect-only.", + "depends_on": [], + "acceptance_criteria": [ + {"id": "AC-A3-01", "description": "pytest tests/acceptance/ passes with no BASE_URL set (live_server fallback)", "test_type": "unit"}, + {"id": "AC-A3-02", "description": "ci-quality.yml's acceptance-check job runs tests for real, not --collect-only", "test_type": "ci"}, + {"id": "AC-A3-03", "description": "BASE_URL-driven runs (make test-acceptance, post-deployment.yml) are unaffected", "test_type": "unit"} + ] + }, + { + "id": "A-4", + "summary": "Real build-time budget on docker-publish.yml build-image job", + "description": "Persist job-start's epoch to $GITHUB_ENV; replace the no-op 'Check build time' stub with a real duration check that fails past 600s; add timeout-minutes: 10 to the job as a hard backstop.", + "depends_on": [], + "acceptance_criteria": [ + {"id": "AC-A4-01", "description": "build-image job has timeout-minutes: 10", "test_type": "ci"}, + {"id": "AC-A4-02", "description": "Check build time step fails the job if duration exceeds 600s", "test_type": "ci"} + ] + }, + { + "id": "A-5", + "summary": "Wire remaining acceptance test files to schemas.py", + "description": "Add LoginGateAssertion and NoCrashAssertion generic models to schemas.py; wire all 8 previously-unwired tests/acceptance/*.py files to use schemas.py models (existing purpose-built models where available, the new generic ones for status-only checks).", + "depends_on": [], + "acceptance_criteria": [ + {"id": "AC-A5-01", "description": "All 9 files in tests/acceptance/ import and use schemas.py models", "test_type": "unit"} + ] + } + ] +} diff --git a/investor_app/finance/utils.py b/investor_app/finance/utils.py index 70e65714..e12acfe5 100644 --- a/investor_app/finance/utils.py +++ b/investor_app/finance/utils.py @@ -19,30 +19,125 @@ def to_decimal(value: Decimal | float | int) -> Decimal: def noi(monthly_income: Decimal, monthly_expenses: Decimal) -> Decimal: + """Calculate annual Net Operating Income (NOI). + + NOI = (Monthly Income - Monthly Expenses) x 12 + + Derivation: NOI is the income-statement identity for property-level + operating income - gross income less operating expenses, before any + financing costs (debt service) or capital expenditures. Annualizing + the monthly figure gives the standard basis for cap rate, DSCR, and + other per-year KPIs. + + Args: + monthly_income: Gross monthly income (rent + other income). + monthly_expenses: Monthly operating expenses (excludes debt service). + + Returns: + Annual NOI as a Decimal. + """ return to_decimal(monthly_income) * Decimal(12) - to_decimal( monthly_expenses ) * Decimal(12) def cap_rate(annual_noi: Decimal, purchase_price: Decimal) -> Decimal: + """Calculate the Capitalization Rate (Cap Rate). + + Cap Rate = Annual NOI / Purchase Price + + Derivation: Cap rate is the property's unlevered yield - the return + an all-cash buyer would earn on the purchase price from operations + alone, independent of financing. It is the direct-capitalization + analogue of a bond's coupon yield: NOI divided by price, both taken + as of the same period. + + Args: + annual_noi: Annual Net Operating Income. + purchase_price: Total purchase price of the property. + + Returns: + Cap rate as a Decimal (e.g. 0.06 for 6%). Returns Decimal("0") + when purchase_price is zero. + """ if to_decimal(purchase_price) == 0: return Decimal("0") return to_decimal(annual_noi) / to_decimal(purchase_price) def cash_on_cash(annual_cash_flow: Decimal, total_cash_invested: Decimal) -> Decimal: + """Calculate Cash-on-Cash (CoC) Return. + + CoC = Annual Cash Flow / Total Cash Invested + + Derivation: Unlike cap rate, CoC is a levered return - it measures + the actual cash yield on the investor's own capital (down payment, + closing costs, rehab) after debt service, since annual_cash_flow is + NOI net of mortgage payments. It answers what the investor earns on + the cash actually put in, not what the property earns overall. + + Args: + annual_cash_flow: Annual cash flow after debt service. + total_cash_invested: Total cash invested by the investor (down + payment + closing costs + any rehab). + + Returns: + Cash-on-cash return as a Decimal (e.g. 0.12 for 12%). Returns + Decimal("0") when total_cash_invested is zero. + """ if to_decimal(total_cash_invested) == 0: return Decimal("0") return to_decimal(annual_cash_flow) / to_decimal(total_cash_invested) def dscr(annual_noi: Decimal, annual_debt_service: Decimal) -> Decimal: + """Calculate the Debt Service Coverage Ratio (DSCR). + + DSCR = Annual NOI / Annual Debt Service + + Derivation: DSCR is a lender solvency ratio, not an investor return + metric - it measures how many times over the property's NOI covers + its annual mortgage payments (principal + interest). A DSCR below + 1.0 means NOI alone cannot cover debt service; most lenders require + DSCR >= 1.20-1.25 as an underwriting minimum. + + Args: + annual_noi: Annual Net Operating Income. + annual_debt_service: Total annual mortgage payments (P&I). + + Returns: + DSCR as a Decimal (e.g. 1.25 means NOI covers debt service + 1.25x). Returns Decimal("0") when annual_debt_service is zero. + """ if to_decimal(annual_debt_service) == 0: return Decimal("0") return to_decimal(annual_noi) / to_decimal(annual_debt_service) def irr(cashflows: list[Decimal]) -> Decimal: + """Calculate the Internal Rate of Return (IRR) for a cashflow series. + + IRR is the discount rate r solving NPV(r) = 0, where + NPV(r) = sum(cashflows[t] / (1 + r)^t) over each period t. + + Derivation: IRR has no closed-form solution in general - it is + defined implicitly as the root of the NPV equation, found here via + numpy_financial's iterative solver. It answers what constant annual + return would make this series of cash in/outflows break even, which + lets cashflows of uneven timing/magnitude (purchase, several years + of rental income, then a sale) be compared on a single annualized + basis, unlike cap rate or CoC which are single-period snapshots. + + Args: + cashflows: Cashflow series; cashflows[0] is the initial outflow + (negative), subsequent entries are periodic net cashflows, + typically ending with a period that includes sale proceeds. + + Returns: + IRR as a Decimal (e.g. 0.15 for 15%). Returns Decimal("0") if + the solver fails to converge or returns a non-finite value (no + real root - e.g. all-same-sign cashflows). + """ cf = np.array([float(c) for c in cashflows], dtype=float) try: value = float(npf.irr(cf)) @@ -1666,6 +1761,11 @@ def one_percent_rule(monthly_rent: Decimal, purchase_price: Decimal) -> bool: The 1% Rule is a quick pass/fail filter: monthly rent should be at least 1% of the purchase price to indicate a potentially viable rental investment. + Derivation: it's a rule-of-thumb proxy for gross yield (monthly_rent x 12 + / purchase_price >= 0.12) rescaled to a monthly figure so it can be + screened without annualizing - a fast pre-filter, not a substitute for + cap rate or CoC. + Args: monthly_rent: Expected gross monthly rental income. purchase_price: Total purchase price of the property. @@ -1692,6 +1792,11 @@ def gross_rent_multiplier(purchase_price: Decimal, annual_rent: Decimal) -> Deci Lower GRM values indicate better value relative to rental income. Typical benchmarks: < 10 excellent, 10–15 good, 15–20 fair, > 20 poor. + Derivation: GRM is the reciprocal-scaled inverse of a rent yield + (price / rent, vs. cap rate's noi / price) - a valuation multiple + analogous to a price-to-revenue ratio, using gross rent rather than + NOI so it can be computed without first estimating operating expenses. + Args: purchase_price: Total purchase price of the property. annual_rent: Expected gross annual rental income. diff --git a/prei/pipeline/handlers/underwriting.py b/prei/pipeline/handlers/underwriting.py index da936b51..58ebe363 100644 --- a/prei/pipeline/handlers/underwriting.py +++ b/prei/pipeline/handlers/underwriting.py @@ -7,8 +7,11 @@ from __future__ import annotations +from decimal import Decimal + from pydantic import BaseModel +from investor_app.finance.utils import cap_rate, cash_on_cash, to_decimal # ── Data models ─────────────────────────────────────────────────────────────── @@ -16,115 +19,114 @@ class UnderwritingInput(BaseModel): """Input parameters for the underwriting solver. - All monetary values are in dollars (float). Rate fields are fractions. + All monetary values are Decimal dollars. Rate fields are fractions. """ - purchase_price: float - estimated_rent: float - vacancy_rate: float = 0.05 # Default 5% - rehab_budget: float = 0.0 - property_tax_annual: float - insurance_annual: float - maintenance_reserve_rate: float = 0.10 # 10% of gross rent - management_fee_rate: float = 0.08 # 8% of EGI - hoa_annual: float = 0.0 + purchase_price: Decimal + estimated_rent: Decimal + vacancy_rate: Decimal = Decimal("0.05") # Default 5% + rehab_budget: Decimal = Decimal("0") + property_tax_annual: Decimal + insurance_annual: Decimal + maintenance_reserve_rate: Decimal = Decimal("0.10") # 10% of gross rent + management_fee_rate: Decimal = Decimal("0.08") # 8% of EGI + hoa_annual: Decimal = Decimal("0") class UnderwritingMetrics(BaseModel): """Output metrics from the underwriting solver.""" - noi: float - cap_rate: float - cash_on_cash: float - mao: float + noi: Decimal + cap_rate: Decimal + cash_on_cash: Decimal + mao: Decimal # ── Pure arithmetic helpers ──────────────────────────────────────────────────── -def gross_potential_rent(estimated_rent: float) -> float: +def gross_potential_rent(estimated_rent: Decimal) -> Decimal: """Compute Gross Potential Rent (annual). - GPR = estimated_monthly_rent × 12 + GPR = estimated_monthly_rent x 12 """ - return estimated_rent * 12.0 + return to_decimal(estimated_rent) * Decimal(12) -def effective_gross_income(gpr: float, vacancy_rate: float) -> float: +def effective_gross_income(gpr: Decimal, vacancy_rate: Decimal) -> Decimal: """Compute Effective Gross Income. - EGI = GPR × (1 - vacancy_rate) + EGI = GPR x (1 - vacancy_rate) """ - return gpr * (1.0 - vacancy_rate) + return to_decimal(gpr) * (Decimal("1") - to_decimal(vacancy_rate)) def total_operating_expenses( - property_tax_annual: float, - insurance_annual: float, - gpr: float, - maintenance_reserve_rate: float, - egi: float, - management_fee_rate: float, - hoa_annual: float, -) -> float: + property_tax_annual: Decimal, + insurance_annual: Decimal, + gpr: Decimal, + maintenance_reserve_rate: Decimal, + egi: Decimal, + management_fee_rate: Decimal, + hoa_annual: Decimal, +) -> Decimal: """Compute total annual operating expenses. Operating Expenses = Property Taxes + Insurance + Maintenance Reserve + Property Management Fees + HOA - Maintenance Reserve = GPR × maintenance_reserve_rate - Management Fees = EGI × management_fee_rate + Maintenance Reserve = GPR x maintenance_reserve_rate + Management Fees = EGI x management_fee_rate """ - maintenance = gpr * maintenance_reserve_rate - management = egi * management_fee_rate + maintenance = to_decimal(gpr) * to_decimal(maintenance_reserve_rate) + management = to_decimal(egi) * to_decimal(management_fee_rate) return ( - property_tax_annual + insurance_annual + maintenance + management + hoa_annual + to_decimal(property_tax_annual) + + to_decimal(insurance_annual) + + maintenance + + management + + to_decimal(hoa_annual) ) -def net_operating_income(egi: float, opex: float) -> float: +def net_operating_income(egi: Decimal, opex: Decimal) -> Decimal: """Compute Net Operating Income. NOI = EGI - Operating Expenses """ - return egi - opex - - -def cap_rate(noi: float, purchase_price: float) -> float: - """Compute Capitalization Rate. - - Cap Rate = NOI / Purchase Price - - Returns 0.0 if purchase_price <= 0 to avoid division by zero. - """ - if purchase_price <= 0: - return 0.0 - return noi / purchase_price + return to_decimal(egi) - to_decimal(opex) -def cash_on_cash_yield(noi: float, purchase_price: float, rehab_budget: float) -> float: +def cash_on_cash_yield( + noi: Decimal, purchase_price: Decimal, rehab_budget: Decimal +) -> Decimal: """Compute Cash-on-Cash Yield (all-cash baseline). CoC = NOI / (Purchase Price + Rehab Budget) - Returns 0.0 if initial cash outlay <= 0. + Delegates the division to investor_app.finance.utils.cash_on_cash (which + already returns Decimal("0") when the denominator is zero) rather than + reimplementing it locally. This is deliberately *not* a call to that + module's true leveraged cash-on-cash metric conceptually - no debt + service is netted out of ``noi`` here, since this models an all-cash + acquisition yield (NOI over total cash outlay) rather than the return on + an investor's equity after financing. """ - initial_cash = purchase_price + rehab_budget - if initial_cash <= 0: - return 0.0 - return noi / initial_cash + initial_cash = to_decimal(purchase_price) + to_decimal(rehab_budget) + return cash_on_cash(to_decimal(noi), initial_cash) -def max_allowable_offer(noi: float, target_cap_rate: float) -> float: +def max_allowable_offer(noi: Decimal, target_cap_rate: Decimal | float) -> Decimal: """Solve for Max Allowable Offer given a target cap rate. MAO = NOI / Target Cap Rate - Returns 0.0 if target_cap_rate <= 0. + Returns Decimal("0") if target_cap_rate <= 0. """ - if target_cap_rate <= 0: - return 0.0 - return noi / target_cap_rate + rate = to_decimal(target_cap_rate) + if rate <= 0: + return Decimal("0") + return to_decimal(noi) / rate # ── Composition solver ──────────────────────────────────────────────────────── @@ -132,7 +134,7 @@ def max_allowable_offer(noi: float, target_cap_rate: float) -> float: def solve_underwriting( inputs: UnderwritingInput, - target_cap_rate: float, + target_cap_rate: Decimal | float, ) -> UnderwritingMetrics: """Compute all underwriting metrics and solve for MAO. @@ -178,8 +180,8 @@ def solve_underwriting( mao = max_allowable_offer(noi, target_cap_rate) return UnderwritingMetrics( - noi=round(noi, 2), - cap_rate=round(cap, 6), - cash_on_cash=round(coc, 6), - mao=round(mao, 2), + noi=noi.quantize(Decimal("0.01")), + cap_rate=cap.quantize(Decimal("0.000001")), + cash_on_cash=coc.quantize(Decimal("0.000001")), + mao=mao.quantize(Decimal("0.01")), ) diff --git a/prei/pipeline/orchestrator.py b/prei/pipeline/orchestrator.py index 47dd0e0c..fb371a64 100644 --- a/prei/pipeline/orchestrator.py +++ b/prei/pipeline/orchestrator.py @@ -7,8 +7,10 @@ from __future__ import annotations import logging +from decimal import Decimal from typing import Any, Dict, Optional, Set +from investor_app.finance.utils import to_decimal from prei.models.pipeline import PipelineStage, PropertyAsset from prei.pipeline.engine import ( AssetRepository, @@ -192,14 +194,18 @@ def run( # ── Stage 3: UNDERWRITING ──────────────────────────────────────── # Build input from canonical data (with sensible defaults) - price = canonical.price or 0.0 - rent = canonical.estimated_rent or 0.0 + price = to_decimal(canonical.price or 0.0) + rent = to_decimal(canonical.estimated_rent or 0.0) uw_input = UnderwritingInput( purchase_price=price, estimated_rent=rent, - property_tax_annual=raw_payload.get("property_tax_annual", price * 0.012), - insurance_annual=raw_payload.get("insurance_annual", price * 0.004), - hoa_annual=raw_payload.get("hoa_annual", 0.0), + property_tax_annual=raw_payload.get( + "property_tax_annual", price * Decimal("0.012") + ), + insurance_annual=raw_payload.get( + "insurance_annual", price * Decimal("0.004") + ), + hoa_annual=raw_payload.get("hoa_annual", Decimal("0")), ) uw_metrics = solve_underwriting(uw_input, self.target_cap_rate) diff --git a/prei/pipeline/tests/test_underwriting.py b/prei/pipeline/tests/test_underwriting.py index 979a307d..a830d666 100644 --- a/prei/pipeline/tests/test_underwriting.py +++ b/prei/pipeline/tests/test_underwriting.py @@ -1,5 +1,7 @@ """Tests for the underwriting solver engine.""" +from decimal import Decimal + import pytest from prei.pipeline.handlers.underwriting import ( @@ -18,15 +20,15 @@ # ── Sample input ────────────────────────────────────────────────────────────── BASE_INPUT = UnderwritingInput( - purchase_price=300_000.0, - estimated_rent=2500.0, - vacancy_rate=0.05, - rehab_budget=20_000.0, - property_tax_annual=3_600.0, - insurance_annual=1_200.0, - maintenance_reserve_rate=0.10, - management_fee_rate=0.08, - hoa_annual=600.0, + purchase_price=Decimal("300000.0"), + estimated_rent=Decimal("2500.0"), + vacancy_rate=Decimal("0.05"), + rehab_budget=Decimal("20000.0"), + property_tax_annual=Decimal("3600.0"), + insurance_annual=Decimal("1200.0"), + maintenance_reserve_rate=Decimal("0.10"), + management_fee_rate=Decimal("0.08"), + hoa_annual=Decimal("600.0"), ) # Expected intermediate values for BASE_INPUT: @@ -68,20 +70,22 @@ def test_total_operating_expenses(self): management_fee_rate=0.08, hoa_annual=600.0, ) - assert opex == pytest.approx(10_680.0) + assert float(opex) == pytest.approx(10_680.0) def test_net_operating_income(self): - assert net_operating_income(28_500, 10_680) == pytest.approx(17_820.0) + assert float(net_operating_income(28_500, 10_680)) == pytest.approx(17_820.0) assert net_operating_income(0, 0) == 0.0 - assert net_operating_income(10_000, 15_000) == pytest.approx(-5_000.0) + assert float(net_operating_income(10_000, 15_000)) == pytest.approx(-5_000.0) # ── Division by zero guards ────────────────────────────────────────────── def test_cap_rate_zero_price(self): - assert cap_rate(noi=10_000, purchase_price=0) == 0.0 + assert cap_rate(annual_noi=10_000, purchase_price=0) == 0.0 def test_cap_rate_negative_price(self): - assert cap_rate(noi=10_000, purchase_price=-100) == 0.0 + # Canonical investor_app.finance.utils.cap_rate has no special case + # for negative purchase_price - it performs real division. + assert cap_rate(annual_noi=10_000, purchase_price=-100) == -100.0 def test_cash_on_cash_zero_initial(self): assert cash_on_cash_yield(noi=10_000, purchase_price=0, rehab_budget=0) == 0.0 @@ -89,14 +93,14 @@ def test_cash_on_cash_zero_initial(self): def test_cash_on_cash_all_cash(self): """All-cash purchase: CoC = NOI / price.""" coc = cash_on_cash_yield(noi=17_820, purchase_price=300_000, rehab_budget=0) - assert coc == pytest.approx(0.0594, rel=1e-4) + assert float(coc) == pytest.approx(0.0594, rel=1e-4) def test_cash_on_cash_with_rehab(self): """With rehab budget: CoC = NOI / (price + rehab).""" coc = cash_on_cash_yield( noi=17_820, purchase_price=300_000, rehab_budget=20_000 ) - assert coc == pytest.approx(0.0556875, rel=1e-5) + assert float(coc) == pytest.approx(0.0556875, rel=1e-5) def test_max_allowable_offer_zero_target(self): assert max_allowable_offer(noi=10_000, target_cap_rate=0) == 0.0 @@ -107,7 +111,7 @@ def test_max_allowable_offer_negative_target(self): def test_max_allowable_offer_standard(self): """MAO = NOI / target_cap_rate.""" mao = max_allowable_offer(noi=17_820, target_cap_rate=0.08) - assert mao == pytest.approx(222_750.0) + assert float(mao) == pytest.approx(222_750.0) # ═══════════════════════════════════════════════════════════════════════════════ @@ -123,28 +127,30 @@ def test_base_case(self): result = solve_underwriting(BASE_INPUT, target_cap_rate=0.08) assert isinstance(result, UnderwritingMetrics) - assert result.noi == pytest.approx(17_820.0, rel=1e-4) - assert result.cap_rate == pytest.approx(0.0594, rel=1e-4) - assert result.cash_on_cash == pytest.approx(0.0556875, rel=1e-5) - assert result.mao == pytest.approx(222_750.0, rel=1e-4) + assert float(result.noi) == pytest.approx(17_820.0, rel=1e-4) + assert float(result.cap_rate) == pytest.approx(0.0594, rel=1e-4) + assert float(result.cash_on_cash) == pytest.approx(0.0556875, rel=1e-5) + assert float(result.mao) == pytest.approx(222_750.0, rel=1e-4) def test_high_target_cap_rate_lowers_mao(self): """Higher target cap rate → lower MAO.""" result_8 = solve_underwriting(BASE_INPUT, target_cap_rate=0.08) result_10 = solve_underwriting(BASE_INPUT, target_cap_rate=0.10) assert result_10.mao < result_8.mao - assert result_10.mao == pytest.approx(178_200.0, rel=1e-4) # 17820 / 0.10 + assert float(result_10.mao) == pytest.approx( + 178_200.0, rel=1e-4 + ) # 17820 / 0.10 def test_low_target_cap_rate_raises_mao(self): """Lower target cap rate → higher MAO.""" result_6 = solve_underwriting(BASE_INPUT, target_cap_rate=0.06) - assert result_6.mao == pytest.approx(297_000.0, rel=1e-4) # 17820 / 0.06 + assert float(result_6.mao) == pytest.approx(297_000.0, rel=1e-4) # 17820 / 0.06 def test_no_rehab(self): """Zero rehab budget → CoC = NOI / price.""" inp = BASE_INPUT.model_copy(update={"rehab_budget": 0}) result = solve_underwriting(inp, target_cap_rate=0.08) - assert result.cash_on_cash == pytest.approx(0.0594, rel=1e-4) + assert float(result.cash_on_cash) == pytest.approx(0.0594, rel=1e-4) def test_higher_vacancy_lowers_noi(self): """Higher vacancy rate → lower EGI → lower NOI.""" @@ -155,7 +161,7 @@ def test_higher_vacancy_lowers_noi(self): # Mgmt = 25500 * 0.08 = 2040 # OpEx = 3600 + 1200 + 3000 + 2040 + 600 = 10440 # NOI = 25500 - 10440 = 15060 - assert result.noi == pytest.approx(15_060.0, rel=1e-4) + assert float(result.noi) == pytest.approx(15_060.0, rel=1e-4) def test_zero_purchase_price(self): """Zero purchase price → cap_rate = 0, MAO still valid.""" @@ -164,15 +170,15 @@ def test_zero_purchase_price(self): assert result.cap_rate == 0.0 assert result.cash_on_cash == 0.0 # MAO should still be valid (based on NOI, not price) - assert result.mao == pytest.approx(222_750.0, rel=1e-4) + assert float(result.mao) == pytest.approx(222_750.0, rel=1e-4) def test_all_defaults(self): """Solver works with only required fields and defaults.""" inp = UnderwritingInput( - purchase_price=200_000.0, - estimated_rent=1800.0, - property_tax_annual=2_400.0, - insurance_annual=900.0, + purchase_price=Decimal("200000.0"), + estimated_rent=Decimal("1800.0"), + property_tax_annual=Decimal("2400.0"), + insurance_annual=Decimal("900.0"), ) result = solve_underwriting(inp, target_cap_rate=0.08) assert isinstance(result, UnderwritingMetrics) diff --git a/specification.md b/specification.md index e6fcde94..8ae66ac1 100644 --- a/specification.md +++ b/specification.md @@ -1,39 +1,65 @@ -# Specification: Phase A — CI/Test Quality Gaps (docs/TOP_01_PLAN.md) +# Specification: Phase B — Financial Math (docs/TOP_01_PLAN.md) # Written: 2026-07-27 --- ## 0. Problem -`docs/TOP_01_PLAN.md` Phase A identifies 5 gaps between this repo's CI pipeline -and a genuinely trustworthy one: acceptance/BDD suites that don't exercise real -HTTP, a PR-gate acceptance job that only `--collect-only`s instead of running, -an unbounded Docker build step, and acceptance tests that only check status -codes instead of response shape. +`docs/TOP_01_PLAN.md` Phase B requires the core financial-math functions in +`investor_app/finance/utils.py` to have independent reference implementations, +broad edge-case coverage gated in CI, and mathematical derivation docstrings. +Investigation found Phase B partially done already (commit `30fd355`): B-1 was +missing IRR's reference implementation, B-2 had only 5-9 cases per function +(well short of "50+"), B-3 was already wired, and B-4 had zero derivation +docstrings anywhere. The audit also surfaced a live `AGENTS.md` "Never Do" +violation adjacent to this work: `prei/pipeline/handlers/underwriting.py` was +a second, fully float-based implementation of NOI/cap-rate/cash-on-cash living +outside `services/utils` (Never-Do #1 and #3) — approved for fixing in this +same PR. ## 1. Requirements -- A-1: `main-ci-guard.yml` blocks PR merges on Tier-2 (post-merge) failure. -- A-2: `tests_bdd/`'s pipeline acceptance suite drives real HTTP requests - (via pytest-django's `live_server`) instead of `django.test.Client`. -- A-3: `tests/acceptance/*.py` actually executes in the PR-gate tier - (`ci-quality.yml`), not just `--collect-only`, via a `live_server` fallback - when `BASE_URL` is unset. -- A-4: `docker-publish.yml`'s `build-image` job enforces a real 10-minute - build-time budget (soft check + hard `timeout-minutes` backstop). -- A-5: All `tests/acceptance/*.py` files validate response shape via - `schemas.py` Pydantic models, not just raw status codes. +- B-1: `tests/finance_reference.py` has an independent, `numpy_financial`-free + reference implementation of every core KPI, including IRR. +- B-2: `tests/test_finance_math.py` covers 50+ parameterized edge cases per + function (normal, zero, negative, extreme magnitude, sub-cent precision, + boundary, int/Decimal coercion). +- B-3: `ci-quality.yml`'s `finance-math` job gates on the full expanded suite + (already wired; automatically covers new IRR cases once added). +- B-4: `noi`, `cap_rate`, `cash_on_cash`, `dscr`, `irr` in + `investor_app/finance/utils.py` have full derivation docstrings; `one_percent_rule`/ + `gross_rent_multiplier` get a derivation note added to their existing docstrings. +- UW-1: `prei/pipeline/handlers/underwriting.py` converts from `float` to + `Decimal` and stops duplicating `cap_rate`/`cash_on_cash` — it imports the + canonical implementations from `investor_app.finance.utils` instead. ## 2. Acceptance Criteria | ID | Criterion | test_type | |---|---|---| -| AC-A1-01 | `main-ci-guard.yml` fails the PR check when Tier 2 fails | ci | -| AC-A2-01 | `pytest tests_bdd/` passes using `live_server` + `httpx.Client` | unit | -| AC-A2-02 | POST-based BDD steps include a real CSRF token | unit | -| AC-A3-01 | `pytest tests/acceptance/` passes with no `BASE_URL` set (live_server fallback) | unit | -| AC-A3-02 | `ci-quality.yml`'s `acceptance-check` job runs tests for real, not `--collect-only` | ci | -| AC-A3-03 | `BASE_URL`-driven runs (`make test-acceptance`, `post-deployment.yml`) are unaffected | unit | -| AC-A4-01 | `build-image` job has `timeout-minutes: 10` | ci | -| AC-A4-02 | "Check build time" step fails the job if duration exceeds 600s | ci | -| AC-A5-01 | All 9 files in `tests/acceptance/` import and use `schemas.py` models | unit | +| AC-B1-01 | `ref_irr` exists in `tests/finance_reference.py`, no numpy dependency | unit | +| AC-B2-01 | `pytest tests/test_finance_math.py` passes with 50+ cases per function | unit | +| AC-B2-02 | `one_percent_rule`/`gross_rent_multiplier` reference functions raise `ValueError` matching production's contract | unit | +| AC-B3-01 | `ci-quality.yml`'s `finance-math` job runs `tests/test_finance_math.py` (already true) | ci | +| AC-B4-01 | `noi`/`cap_rate`/`cash_on_cash`/`dscr`/`irr` each have a "Derivation:" docstring paragraph | unit | +| AC-UW-01 | `UnderwritingInput`/`UnderwritingMetrics` fields are `Decimal`, not `float` | unit | +| AC-UW-02 | `underwriting.py` imports `cap_rate`/`cash_on_cash` from `investor_app.finance.utils`, no local duplicate | unit | +| AC-UW-03 | `pytest prei/pipeline/tests/test_underwriting.py tests/test_underwriting_integration.py tests/test_offer_integration.py` passes | unit | +| AC-UW-04 | `orchestrator.py`'s `price * 0.012`/`price * 0.004` boundary uses `Decimal` arithmetic | unit | + +## 3. Out of Scope + +- `prei/pipeline/handlers/offer.py`'s remaining float-based currency — tracked + as `docs/KNOWN_LIMITATIONS.md` LIMIT-21, not fixed here. +- Reconciling the bare-function vs. `calculate_*` contract divergence and the + duplicate `score_listing_v2` functions in `investor_app/finance/utils.py` — + tracked as LIMIT-20, requires an API-contract decision out of scope for this PR. + +## 4. Verification + +- `pytest tests/test_finance_math.py -v --tb=short` +- `pytest prei/pipeline/tests/test_underwriting.py tests/test_underwriting_integration.py tests/test_offer_integration.py prei/pipeline/tests/test_orchestrator.py -q -o addopts=""` +- `pytest tests_bdd/ core/tests/ prei/pipeline/tests/ -q` +- `mypy core/ investor_app/finance/` (existing CI command) +- Push branch, open PR, watch `ci-quality.yml` go green. PR stays open for + human review/merge — never merge or push to `main` directly. diff --git a/tasks.json b/tasks.json index 3d8ac373..1492457b 100644 --- a/tasks.json +++ b/tasks.json @@ -1,60 +1,69 @@ { "meta": { "project": "prei", - "session": "top01-phase-a-20260727", + "session": "top01-phase-b-20260727", "date": "2026-07-27", - "feature": "Phase A — CI/Test Quality Gaps (docs/TOP_01_PLAN.md)", + "feature": "Phase B — Financial Math (docs/TOP_01_PLAN.md)", "spec": "specification.md", "design": "design.md" }, "tasks": [ { - "id": "A-1", - "summary": "main-ci-guard blocks PR merge on Tier-2 failure", - "description": "Already implemented prior to this session; verified in .github/workflows/main-ci-guard.yml. No action taken.", + "id": "B-1", + "summary": "Add ref_irr to tests/finance_reference.py", + "description": "Independent, numpy_financial-free bisection-based IRR reference implementation, mirroring production's NaN/Inf -> Decimal(0) fallback for no-real-root cases.", "depends_on": [], "acceptance_criteria": [ - {"id": "AC-A1-01", "description": "main-ci-guard.yml fails the PR check when Tier 2 fails", "test_type": "ci"} + {"id": "AC-B1-01", "description": "ref_irr exists in tests/finance_reference.py, no numpy dependency", "test_type": "unit"} ] }, { - "id": "A-2", - "summary": "BDD pipeline suite drives real HTTP via live_server", - "description": "Rewrite tests_bdd/steps/pipeline_acceptance_steps.py to use an httpx.Client bound to pytest-django's live_server fixture instead of django.test.Client; switch tests_bdd/conftest.py's _reset_ctx to depend on transactional_db.", - "depends_on": [], + "id": "B-2", + "summary": "Expand edge-case coverage to 50+ per function", + "description": "Expand NOI/cap_rate/CoC/DSCR/one_percent_rule/GRM case lists to 50+ rows each and add a new IRR_CASES block; fix ref_one_percent_rule/ref_gross_rent_multiplier to raise ValueError matching production's contract.", + "depends_on": ["B-1"], "acceptance_criteria": [ - {"id": "AC-A2-01", "description": "pytest tests_bdd/ passes using live_server + httpx.Client", "test_type": "unit"}, - {"id": "AC-A2-02", "description": "POST-based BDD steps include a real CSRF token", "test_type": "unit"} + {"id": "AC-B2-01", "description": "pytest tests/test_finance_math.py passes with 50+ cases per function", "test_type": "unit"}, + {"id": "AC-B2-02", "description": "one_percent_rule/gross_rent_multiplier reference functions raise ValueError matching production's contract", "test_type": "unit"} ] }, { - "id": "A-3", - "summary": "Acceptance suite executes for real in the PR-gate tier", - "description": "Extend tests/acceptance/conftest.py's base_url fixture to fall back to a live_server when BASE_URL is unset; add an autouse fixture to unblock DB access per test; update ci-quality.yml's acceptance-check job to run pytest for real instead of --collect-only.", - "depends_on": [], + "id": "B-3", + "summary": "CI gate coverage for IRR (no workflow change needed)", + "description": "ci-quality.yml's finance-math job already runs the whole of tests/test_finance_math.py; B-1/B-2 automatically extend its coverage. Verified, no edit made.", + "depends_on": ["B-1", "B-2"], "acceptance_criteria": [ - {"id": "AC-A3-01", "description": "pytest tests/acceptance/ passes with no BASE_URL set (live_server fallback)", "test_type": "unit"}, - {"id": "AC-A3-02", "description": "ci-quality.yml's acceptance-check job runs tests for real, not --collect-only", "test_type": "ci"}, - {"id": "AC-A3-03", "description": "BASE_URL-driven runs (make test-acceptance, post-deployment.yml) are unaffected", "test_type": "unit"} + {"id": "AC-B3-01", "description": "ci-quality.yml's finance-math job runs tests/test_finance_math.py (already true)", "test_type": "ci"} ] }, { - "id": "A-4", - "summary": "Real build-time budget on docker-publish.yml build-image job", - "description": "Persist job-start's epoch to $GITHUB_ENV; replace the no-op 'Check build time' stub with a real duration check that fails past 600s; add timeout-minutes: 10 to the job as a hard backstop.", + "id": "B-4", + "summary": "Add mathematical derivation docstrings to utils.py core functions", + "description": "Add formula + Derivation: docstrings to noi, cap_rate, cash_on_cash, dscr, irr in investor_app/finance/utils.py; add a one-line derivation note to one_percent_rule/gross_rent_multiplier's existing docstrings.", "depends_on": [], "acceptance_criteria": [ - {"id": "AC-A4-01", "description": "build-image job has timeout-minutes: 10", "test_type": "ci"}, - {"id": "AC-A4-02", "description": "Check build time step fails the job if duration exceeds 600s", "test_type": "ci"} + {"id": "AC-B4-01", "description": "noi/cap_rate/cash_on_cash/dscr/irr each have a Derivation: docstring paragraph", "test_type": "unit"} ] }, { - "id": "A-5", - "summary": "Wire remaining acceptance test files to schemas.py", - "description": "Add LoginGateAssertion and NoCrashAssertion generic models to schemas.py; wire all 8 previously-unwired tests/acceptance/*.py files to use schemas.py models (existing purpose-built models where available, the new generic ones for status-only checks).", + "id": "UW-1", + "summary": "Fix underwriting.py float->Decimal + dedup cap_rate/cash_on_cash", + "description": "Convert UnderwritingInput/UnderwritingMetrics to Decimal; delete the local cap_rate() duplicate and import the canonical investor_app.finance.utils.cap_rate/cash_on_cash/to_decimal; convert composition helpers to Decimal arithmetic; fix orchestrator.py's price*0.012/price*0.004 boundary; fix downstream test-file Decimal/float interop breakage (test_underwriting.py, test_underwriting_integration.py, test_offer_integration.py, pipeline_steps.py).", "depends_on": [], "acceptance_criteria": [ - {"id": "AC-A5-01", "description": "All 9 files in tests/acceptance/ import and use schemas.py models", "test_type": "unit"} + {"id": "AC-UW-01", "description": "UnderwritingInput/UnderwritingMetrics fields are Decimal, not float", "test_type": "unit"}, + {"id": "AC-UW-02", "description": "underwriting.py imports cap_rate/cash_on_cash from investor_app.finance.utils, no local duplicate", "test_type": "unit"}, + {"id": "AC-UW-03", "description": "pytest prei/pipeline/tests/test_underwriting.py tests/test_underwriting_integration.py tests/test_offer_integration.py passes", "test_type": "unit"}, + {"id": "AC-UW-04", "description": "orchestrator.py's price * 0.012/price * 0.004 boundary uses Decimal arithmetic", "test_type": "unit"} + ] + }, + { + "id": "DOC-1", + "summary": "Document deliberately out-of-scope findings in KNOWN_LIMITATIONS.md", + "description": "Add LIMIT-20 (bare-function vs. calculate_* contract divergence + duplicate score_listing_v2) and LIMIT-21 (offer.py remains float-based currency).", + "depends_on": ["UW-1"], + "acceptance_criteria": [ + {"id": "AC-DOC-01", "description": "docs/KNOWN_LIMITATIONS.md has LIMIT-20 and LIMIT-21 entries in the file's existing format", "test_type": "manual"} ] } ] diff --git a/tests/finance_reference.py b/tests/finance_reference.py index 7dcbf031..4be8c28e 100644 --- a/tests/finance_reference.py +++ b/tests/finance_reference.py @@ -86,17 +86,27 @@ def ref_monthly_mortgage( # Pass if monthly_rent >= purchase_price × 0.01 +# purchase_price must be > 0 for the ratio to be meaningful — matches +# production's ValueError contract (investor_app/finance/utils.py:1663). def ref_one_percent_rule(monthly_rent: Decimal, purchase_price: Decimal) -> bool: - return monthly_rent >= purchase_price * Decimal("0.01") + if purchase_price <= Decimal("0"): + raise ValueError( + f"purchase_price must be greater than zero (received {purchase_price})" + ) + return monthly_rent / purchase_price >= Decimal("0.01") # ── Gross Rent Multiplier ────────────────────────────────────────────────── # GRM = Purchase Price ÷ Annual Rent +# annual_rent must be > 0 for the ratio to be meaningful — matches +# production's ValueError contract (investor_app/finance/utils.py:1687). def ref_gross_rent_multiplier(purchase_price: Decimal, annual_rent: Decimal) -> Decimal: - if annual_rent == Decimal("0"): - return Decimal("0") + if annual_rent <= Decimal("0"): + raise ValueError( + f"annual_rent must be greater than zero (received {annual_rent})" + ) return purchase_price / annual_rent @@ -106,3 +116,65 @@ def ref_gross_rent_multiplier(purchase_price: Decimal, annual_rent: Decimal) -> # Annual Depreciation = (Purchase Price - Land Value) ÷ 27.5 def ref_annual_depreciation(purchase_price: Decimal, land_value: Decimal) -> Decimal: return (purchase_price - land_value) / Decimal("27.5") + + +# ── IRR: Internal Rate of Return ─────────────────────────────────────────── + + +def _npv(rate: Decimal, cashflows: list[Decimal]) -> Decimal: + """NPV(r) = Σ cashflows[t] / (1+r)^t, t an integer period index.""" + base = Decimal("1") + rate + total = Decimal("0") + for t, cf in enumerate(cashflows): + total += cf / (base**t) + return total + + +# IRR is the rate r solving NPV(r) = 0. No closed form exists in general, so +# this brackets a sign change in NPV over a coarse grid and bisects within +# it. Returns Decimal("0") when no sign change is found in the scanned range +# (no real root — e.g. all-same-sign cashflows), mirroring production's +# existing NaN/Inf -> Decimal("0") fallback (investor_app/finance/utils.py:49-53). +def ref_irr(cashflows: list[Decimal]) -> Decimal: + if len(cashflows) < 2: + return Decimal("0") + + grid_lo = Decimal("-0.9999") + grid_hi = Decimal("10") + step = Decimal("0.01") + + r_prev = grid_lo + npv_prev = _npv(r_prev, cashflows) + if npv_prev == Decimal("0"): + return r_prev + + bracket = None + r = grid_lo + step + while r <= grid_hi: + npv_cur = _npv(r, cashflows) + if npv_cur == Decimal("0"): + return r + if (npv_prev < Decimal("0")) != (npv_cur < Decimal("0")): + bracket = (r_prev, r) + break + r_prev, npv_prev = r, npv_cur + r += step + + if bracket is None: + return Decimal("0") + + lo, hi = bracket + npv_lo = _npv(lo, cashflows) + tolerance = Decimal("0.0000001") + for _ in range(100): + if hi - lo < tolerance: + break + mid = (lo + hi) / Decimal("2") + npv_mid = _npv(mid, cashflows) + if npv_mid == Decimal("0"): + return mid + if (npv_lo < Decimal("0")) == (npv_mid < Decimal("0")): + lo, npv_lo = mid, npv_mid + else: + hi = mid + return (lo + hi) / Decimal("2") diff --git a/tests/test_finance_math.py b/tests/test_finance_math.py index 3c1b8ebd..5551d8e1 100644 --- a/tests/test_finance_math.py +++ b/tests/test_finance_math.py @@ -2,9 +2,11 @@ Each test compares the production KPI implementation against an independently-written reference implementation. A deviation greater -than ``Decimal(\"0.01\")`` is treated as a regression failure. +than the function's tolerance is treated as a regression failure. -60 edge cases across 8 core KPI functions. +300+ edge cases across 9 core KPI functions. NOI, cap rate, cash-on-cash, +DSCR, IRR, the 1% rule, and GRM each carry 50+ cases (docs/TOP_01_PLAN.md +Phase B, B-2); mortgage and depreciation retain their original coverage. """ from __future__ import annotations @@ -21,6 +23,7 @@ cash_on_cash, dscr, gross_rent_multiplier, + irr, noi, one_percent_rule, ) @@ -32,15 +35,51 @@ ref_cash_on_cash, ref_dscr, ref_gross_rent_multiplier, + ref_irr, ref_monthly_mortgage, ref_noi, ref_one_percent_rule, ) -# For tests that don't need Django: just verify reference self-consistency _D = Decimal +def _case(ref_fn, label: str, *args): + """Build a (label, *args, expected) case tuple, deriving ``expected`` + from the reference implementation itself. Used for bulk-generated edge + cases where the point is catching prod/ref divergence (regression + detection, the actual goal of this suite), not re-deriving the + arithmetic by hand for every row. + """ + return (label, *args, ref_fn(*args)) + + +# Shared value pools for generated edge cases. +_POS = [100, 500, 1000, 5000, 10000, 50000, 100000, 250000, 500000, 999999] +_NEG = [-100, -500, -1000, -5000, -10000, -50000, -100000, -250000, -500000, -999999] +_EXTREME_LARGE = [1_000_000, 5_000_000, 10_000_000, 100_000_000, 999_999_999] +_EXTREME_SMALL = [ + "0.0001", + "0.0005", + "0.001", + "0.005", + "0.01", + "0.05", + "0.1", + "0.5", + "0.9999", +] +_CURRENCY = [ + "1234.56", + "9999.99", + "50000.01", + "123456.78", + "654321.99", + "0.99", + "1000000.01", +] + + # ═══════════════════════════════════════════════════════════════════════════ # NOI — Net Operating Income # ═══════════════════════════════════════════════════════════════════════════ @@ -57,6 +96,25 @@ ("extreme_income", _D("50000"), _D("1000"), _D("588000")), ] +NOI_EXTRA_CASES = ( + [_case(ref_noi, f"zero_income_{e}", _D("0"), _D(str(e))) for e in _POS] + + [_case(ref_noi, f"zero_expenses_{i}", _D(str(i)), _D("0")) for i in _POS] + + [_case(ref_noi, f"negative_income_{i}", _D(str(i)), _D("800")) for i in _NEG] + + [_case(ref_noi, f"negative_expenses_{e}", _D("1500"), _D(str(e))) for e in _NEG] + + [ + _case(ref_noi, f"extreme_large_{i}", _D(str(i)), _D(str(i // 2))) + for i in _EXTREME_LARGE + ] + + [_case(ref_noi, f"extreme_small_{v}", _D(v), _D("0")) for v in _EXTREME_SMALL] + + [_case(ref_noi, f"currency_precision_{v}", _D(v), _D("1")) for v in _CURRENCY] + + [_case(ref_noi, f"boundary_equal_{v}", _D(str(v)), _D(str(v))) for v in _POS] + + [ + (f"int_coercion_{i}", i, i // 2, ref_noi(_D(str(i)), _D(str(i // 2)))) + for i in _POS[:5] + ] +) +NOI_CASES = NOI_CASES + NOI_EXTRA_CASES + @pytest.mark.parametrize("label,income,expenses,expected", NOI_CASES) def test_noi(label: str, income: Decimal, expenses: Decimal, expected: Decimal) -> None: @@ -83,11 +141,44 @@ def test_noi(label: str, income: Decimal, expenses: Decimal, expected: Decimal) ("low_cap", _D("1000"), _D("200000"), _D("0.005")), ] +CAP_RATE_EXTRA_CASES = ( + [_case(ref_cap_rate, f"zero_price_{n}", _D(str(n)), _D("0")) for n in _POS] + + [_case(ref_cap_rate, f"zero_noi_{p}", _D("0"), _D(str(p))) for p in _POS] + + [_case(ref_cap_rate, f"negative_noi_{n}", _D(str(n)), _D("200000")) for n in _NEG] + + [ + _case(ref_cap_rate, f"negative_price_{p}", _D("12000"), _D(str(p))) + for p in _NEG + ] + + [ + _case(ref_cap_rate, f"extreme_large_{n}", _D(str(n)), _D(str(n * 5))) + for n in _EXTREME_LARGE + ] + + [ + _case(ref_cap_rate, f"extreme_small_{v}", _D(v), _D("200000")) + for v in _EXTREME_SMALL + ] + + [ + _case(ref_cap_rate, f"currency_precision_{v}", _D(v), _D("200000.01")) + for v in _CURRENCY + ] + + [_case(ref_cap_rate, f"boundary_equal_{v}", _D(str(v)), _D(str(v))) for v in _POS] + + [ + (f"int_coercion_{n}", n, 200000, ref_cap_rate(_D(str(n)), _D("200000"))) + for n in _POS[:5] + ] +) +CAP_RATE_CASES = CAP_RATE_CASES + CAP_RATE_EXTRA_CASES + @pytest.mark.parametrize("label,noi,price,expected", CAP_RATE_CASES) def test_cap_rate(label: str, noi: Decimal, price: Decimal, expected: Decimal) -> None: prod = cap_rate(noi, price) - ref = ref_cap_rate(noi, price) + # ref_cap_rate only promises Decimal in/Decimal out (no to_decimal() of + # its own); int/int-coercion cases pass raw ints to exercise production's + # own to_decimal() boundary, so coerce here to keep ref's arithmetic in + # Decimal too (bare int/int division would otherwise silently produce a + # float and fail comparison against production's Decimal result). + ref = ref_cap_rate(_D(str(noi)), _D(str(price))) assert abs(prod - ref) < _D("0.0001"), f"Cap rate {label}: prod={prod} ref={ref}" @@ -106,13 +197,47 @@ def test_cap_rate(label: str, noi: Decimal, price: Decimal, expected: Decimal) - ("currency", _D("9999.99"), _D("100000.00"), _D("0.0999999")), ] +COC_EXTRA_CASES = ( + [_case(ref_cash_on_cash, f"zero_invested_{n}", _D(str(n)), _D("0")) for n in _POS] + + [_case(ref_cash_on_cash, f"zero_cashflow_{p}", _D("0"), _D(str(p))) for p in _POS] + + [ + _case(ref_cash_on_cash, f"negative_cashflow_{n}", _D(str(n)), _D("50000")) + for n in _NEG + ] + + [ + _case(ref_cash_on_cash, f"negative_invested_{p}", _D("6000"), _D(str(p))) + for p in _NEG + ] + + [ + _case(ref_cash_on_cash, f"extreme_large_{n}", _D(str(n)), _D(str(n * 2))) + for n in _EXTREME_LARGE + ] + + [ + _case(ref_cash_on_cash, f"extreme_small_{v}", _D(v), _D("50000")) + for v in _EXTREME_SMALL + ] + + [ + _case(ref_cash_on_cash, f"currency_precision_{v}", _D(v), _D("45678.90")) + for v in _CURRENCY + ] + + [ + _case(ref_cash_on_cash, f"boundary_equal_{v}", _D(str(v)), _D(str(v))) + for v in _POS + ] + + [ + (f"int_coercion_{n}", n, 50000, ref_cash_on_cash(_D(str(n)), _D("50000"))) + for n in _POS[:5] + ] +) +COC_CASES = COC_CASES + COC_EXTRA_CASES + @pytest.mark.parametrize("label,cf,invested,expected", COC_CASES) def test_cash_on_cash( label: str, cf: Decimal, invested: Decimal, expected: Decimal ) -> None: prod = cash_on_cash(cf, invested) - ref = ref_cash_on_cash(cf, invested) + ref = ref_cash_on_cash(_D(str(cf)), _D(str(invested))) assert abs(prod - ref) < _D("0.0001"), f"CoC {label}: prod={prod} ref={ref}" @@ -131,11 +256,36 @@ def test_cash_on_cash( ("currency", _D("99999.99"), _D("33333.33"), _D("3.0000009")), ] +DSCR_EXTRA_CASES = ( + [_case(ref_dscr, f"zero_debt_{n}", _D(str(n)), _D("0")) for n in _POS] + + [_case(ref_dscr, f"zero_noi_{d}", _D("0"), _D(str(d))) for d in _POS] + + [_case(ref_dscr, f"negative_noi_{n}", _D(str(n)), _D("12000")) for n in _NEG] + + [_case(ref_dscr, f"negative_debt_{d}", _D("15000"), _D(str(d))) for d in _NEG] + + [ + _case(ref_dscr, f"extreme_large_{n}", _D(str(n)), _D(str(n // 3))) + for n in _EXTREME_LARGE + ] + + [ + _case(ref_dscr, f"extreme_small_{v}", _D(v), _D("12000")) + for v in _EXTREME_SMALL + ] + + [ + _case(ref_dscr, f"currency_precision_{v}", _D(v), _D("9876.54")) + for v in _CURRENCY + ] + + [_case(ref_dscr, f"boundary_equal_{v}", _D(str(v)), _D(str(v))) for v in _POS] + + [ + (f"int_coercion_{n}", n, 12000, ref_dscr(_D(str(n)), _D("12000"))) + for n in _POS[:5] + ] +) +DSCR_CASES = DSCR_CASES + DSCR_EXTRA_CASES + @pytest.mark.parametrize("label,noi,debt,expected", DSCR_CASES) def test_dscr(label: str, noi: Decimal, debt: Decimal, expected: Decimal) -> None: prod = dscr(noi, debt) - ref = ref_dscr(noi, debt) + ref = ref_dscr(_D(str(noi)), _D(str(debt))) assert abs(prod - ref) < _D("0.0001"), f"DSCR {label}: prod={prod} ref={ref}" @@ -180,6 +330,45 @@ def test_mortgage( ("currency", _D("199.99"), _D("20000"), False), ] +ONE_PCT_EXTRA_CASES = ( + [_case(ref_one_percent_rule, f"zero_rent_{p}", _D("0"), _D(str(p))) for p in _POS] + + [ + _case(ref_one_percent_rule, f"negative_rent_{r}", _D(str(r)), _D("200000")) + for r in _NEG + ] + + [ + _case( + ref_one_percent_rule, + f"extreme_large_{p}", + _D(str(p // 50)), + _D(str(p)), + ) + for p in _EXTREME_LARGE + ] + + [ + _case(ref_one_percent_rule, f"extreme_small_price_{v}", _D("50"), _D(v)) + for v in _EXTREME_SMALL + ] + + [ + _case(ref_one_percent_rule, f"currency_precision_{v}", _D(v), _D("20000")) + for v in _CURRENCY + ] + + [ + _case( + ref_one_percent_rule, + f"boundary_exact_{v}", + _D(str(v)) * _D("0.01"), + _D(str(v)), + ) + for v in _POS + ] + + [ + (f"int_coercion_{p}", 2000, p, ref_one_percent_rule(_D("2000"), _D(str(p)))) + for p in _POS[:5] + ] +) +ONE_PCT_CASES = ONE_PCT_CASES + ONE_PCT_EXTRA_CASES + @pytest.mark.parametrize("label,rent,price,expected", ONE_PCT_CASES) def test_one_percent_rule( @@ -191,6 +380,23 @@ def test_one_percent_rule( assert ref == expected, f"1% Rule {label}: expected={expected} got={ref}" +# purchase_price <= 0 is invalid for the 1% Rule (production raises ValueError +# at investor_app/finance/utils.py:1663) — verify both production and the +# reference implementation enforce the same contract. +ONE_PCT_ERROR_CASES = [(f"zero_price_{r}", _D(str(r)), _D("0")) for r in _POS[:5]] + [ + (f"negative_price_{r}_{p}", _D(str(r)), _D(str(p))) + for r, p in zip(_POS[:5], _NEG[:5]) +] + + +@pytest.mark.parametrize("label,rent,price", ONE_PCT_ERROR_CASES) +def test_one_percent_rule_raises(label: str, rent: Decimal, price: Decimal) -> None: + with pytest.raises(ValueError): + one_percent_rule(rent, price) + with pytest.raises(ValueError): + ref_one_percent_rule(rent, price) + + # ═══════════════════════════════════════════════════════════════════════════ # Gross Rent Multiplier # ═══════════════════════════════════════════════════════════════════════════ @@ -204,14 +410,75 @@ def test_one_percent_rule( ("currency", _D("99999.99"), _D("12345.67"), _D("8.10001174")), ] +GRM_EXTRA_CASES = ( + [ + _case(ref_gross_rent_multiplier, f"zero_price_{r}", _D("0"), _D(str(r))) + for r in _POS + ] + + [ + _case(ref_gross_rent_multiplier, f"negative_price_{p}", _D(str(p)), _D("24000")) + for p in _NEG + ] + + [ + _case( + ref_gross_rent_multiplier, + f"extreme_large_{p}", + _D(str(p)), + _D(str(p // 10)), + ) + for p in _EXTREME_LARGE + ] + + [ + _case(ref_gross_rent_multiplier, f"extreme_small_rent_{v}", _D("50000"), _D(v)) + for v in _EXTREME_SMALL + ] + + [ + _case( + ref_gross_rent_multiplier, f"currency_precision_{v}", _D(v), _D("9876.54") + ) + for v in _CURRENCY + ] + + [ + _case(ref_gross_rent_multiplier, f"boundary_equal_{v}", _D(str(v)), _D(str(v))) + for v in _POS + ] + + [ + ( + f"int_coercion_{p}", + p, + 24000, + ref_gross_rent_multiplier(_D(str(p)), _D("24000")), + ) + for p in _POS[:5] + ] +) +GRM_CASES = GRM_CASES + GRM_EXTRA_CASES + @pytest.mark.parametrize("label,price,rent,expected", GRM_CASES) def test_grm(label: str, price: Decimal, rent: Decimal, expected: Decimal) -> None: prod = gross_rent_multiplier(price, rent) - ref = ref_gross_rent_multiplier(price, rent) + ref = ref_gross_rent_multiplier(_D(str(price)), _D(str(rent))) assert abs(prod - ref) < _D("0.0001"), f"GRM {label}: prod={prod} ref={ref}" +# annual_rent <= 0 is invalid for GRM (production raises ValueError at +# investor_app/finance/utils.py:1687) — verify both production and the +# reference implementation enforce the same contract. +GRM_ERROR_CASES = [(f"zero_rent_{p}", _D(str(p)), _D("0")) for p in _POS[:5]] + [ + (f"negative_rent_{p}_{r}", _D(str(p)), _D(str(r))) + for p, r in zip(_POS[:5], _NEG[:5]) +] + + +@pytest.mark.parametrize("label,price,rent", GRM_ERROR_CASES) +def test_grm_raises(label: str, price: Decimal, rent: Decimal) -> None: + with pytest.raises(ValueError): + gross_rent_multiplier(price, rent) + with pytest.raises(ValueError): + ref_gross_rent_multiplier(price, rent) + + # ═══════════════════════════════════════════════════════════════════════════ # Annual Depreciation # ═══════════════════════════════════════════════════════════════════════════ @@ -232,3 +499,138 @@ def test_annual_depreciation( prod = annual_depreciation(price, land) ref = ref_annual_depreciation(price, land) assert abs(prod - ref) < _D("0.01"), f"Depreciation {label}: prod={prod} ref={ref}" + + +# ═══════════════════════════════════════════════════════════════════════════ +# IRR — Internal Rate of Return +# ═══════════════════════════════════════════════════════════════════════════ + + +def _series(principal, annual_cf, years: int, exit_value=0) -> list[Decimal]: + """Initial outflow, (years - 1) equal inflows, final inflow + exit value.""" + if years <= 1: + return [Decimal(-principal), Decimal(annual_cf) + Decimal(exit_value)] + return ( + [Decimal(-principal)] + + [Decimal(annual_cf)] * (years - 1) + + [Decimal(annual_cf) + Decimal(exit_value)] + ) + + +_IRR_NORMAL = [ + (f"normal_p{p}_y{y}", _series(p, int(p * 0.08), y, int(p * 0.1))) + for p in [50000, 100000, 200000, 300000, 500000] + for y in [3, 5, 7, 10] +] # 20 cases + +_IRR_LONG_SERIES = [ + (f"long_series_y{y}", _series(150000, 15000, y, 30000)) for y in [12, 15, 18, 20] +] # 4 cases + +_IRR_EXTREME_LARGE = [ + (f"extreme_large_p{p}", _series(p, int(p * 0.05), 5, int(p * 0.2))) + for p in [10_000_000, 100_000_000, 999_999_999] +] # 3 cases + +_IRR_SMALL_MAGNITUDE = [ + ( + f"small_magnitude_p{p}", + _series(p, max(5, int(p * 0.08)), 4, max(5, int(p * 0.1))), + ) + for p in [500, 2000, 8000] +] # 3 cases + +_IRR_SINGLE_CASHFLOW = [ + (f"single_cashflow_{v}", [Decimal(-v)]) for v in [100, 100000, 1] +] # 3 cases — no real root possible with only one period + +_IRR_NO_SIGN_CHANGE = [ + ("all_positive_a", [Decimal(v) for v in [1000, 1000, 1000]]), + ("all_positive_b", [Decimal(v) for v in [500, 600, 700, 800]]), + ("all_positive_c", [Decimal(100)] * 10), + ("all_negative_a", [Decimal(v) for v in [-1000, -500, -200]]), + ("all_negative_b", [Decimal(-100)] * 5), +] # 5 cases — no sign change, no real root + +_IRR_VARIED = ( + [ + ( + f"declining_cf_p{p}", + [Decimal(-p)] + [Decimal(int(p * 0.1 * (1 - 0.05 * i))) for i in range(6)], + ) + for p in [80000, 150000, 250000] + ] + + [ + ( + f"growing_cf_p{p}", + [Decimal(-p)] + [Decimal(int(p * 0.05 * (1 + 0.1 * i))) for i in range(6)], + ) + for p in [80000, 150000, 250000] + ] + + [ + ( + f"currency_precision_p{p}", + _series(p, p * Decimal("0.075"), 5, p * Decimal("0.15")), + ) + for p in [Decimal("123456.78"), Decimal("99999.99")] + ] +) # 8 cases + +_IRR_INT_COERCION = [ + ("int_coercion_a", [-100000, 12000, 12000, 12000, 130000]), + ("int_coercion_b", [-50000, 6000, 6000, 60000]), + ("int_coercion_c", [-200000, 20000, 20000, 20000, 20000, 220000]), +] # 3 cases — raw ints, not Decimal, exercise to_decimal()/Decimal(str()) coercion + +IRR_CASES = ( + _IRR_NORMAL + + _IRR_LONG_SERIES + + _IRR_EXTREME_LARGE + + _IRR_SMALL_MAGNITUDE + + _IRR_SINGLE_CASHFLOW + + _IRR_NO_SIGN_CHANGE + + _IRR_VARIED + + _IRR_INT_COERCION +) # 49 cases + + +@pytest.mark.parametrize("label,cashflows", IRR_CASES) +def test_irr(label: str, cashflows: list) -> None: + prod = irr(cashflows) + ref = ref_irr([Decimal(str(c)) for c in cashflows]) + assert abs(prod - ref) < _D("0.0005"), f"IRR {label}: prod={prod} ref={ref}" + + +# Cashflow series with more than one sign change can have multiple +# mathematically valid real roots. numpy_financial.irr and our bisection +# search aren't guaranteed to converge on the *same* root in that case, so +# rather than asserting prod == ref, verify each independently satisfies +# NPV(rate) ≈ 0 — the actual definition of a valid IRR. +IRR_MULTIPLE_ROOT_CASES = [ + ( + "multi_sign_change_a", + [Decimal("-100000"), Decimal("300000"), Decimal("-220000")], + ), + ("multi_sign_change_b", [Decimal("-50000"), Decimal("120000"), Decimal("-71000")]), + ( + "multi_sign_change_c", + [Decimal("-200000"), Decimal("500000"), Decimal("-310000")], + ), +] # 3 cases + + +def _npv_at(rate: Decimal, cashflows: list[Decimal]) -> Decimal: + base = Decimal("1") + rate + return sum((cf / (base**t) for t, cf in enumerate(cashflows)), Decimal("0")) + + +@pytest.mark.parametrize("label,cashflows", IRR_MULTIPLE_ROOT_CASES) +def test_irr_multiple_roots(label: str, cashflows: list[Decimal]) -> None: + prod = irr(cashflows) + ref = ref_irr(cashflows) + assert abs(_npv_at(prod, cashflows)) < _D("1"), ( + f"IRR {label}: production rate {prod} does not zero NPV" + ) + assert abs(_npv_at(ref, cashflows)) < _D("1"), ( + f"IRR {label}: reference rate {ref} does not zero NPV" + ) diff --git a/tests/test_offer_integration.py b/tests/test_offer_integration.py index 7db5826d..7e25d06a 100644 --- a/tests/test_offer_integration.py +++ b/tests/test_offer_integration.py @@ -118,9 +118,9 @@ def test_offer_from_underwriting_mao(self): target_cap_rate=0.08, ) offer = solve_offer( - OfferInput(mao=uw.mao, arv=uw.mao * 1.15), OfferStrategy.TARGET + OfferInput(mao=uw.mao, arv=float(uw.mao) * 1.15), OfferStrategy.TARGET ) - assert offer.offer_price == pytest.approx(uw.mao, rel=1e-3) + assert offer.offer_price == pytest.approx(float(uw.mao), rel=1e-3) assert offer.estimated_equity is not None assert offer.estimated_equity > 0 @@ -164,7 +164,9 @@ def test_e2e_full_pipeline_with_offer(self): ) assert result.success mao = result.underwriting.mao - offer = solve_offer(OfferInput(mao=mao, arv=mao * 1.2), OfferStrategy.TARGET) + offer = solve_offer( + OfferInput(mao=mao, arv=float(mao) * 1.2), OfferStrategy.TARGET + ) assert offer.offer_price > 0 assert offer.estimated_equity is not None diff --git a/tests/test_underwriting_integration.py b/tests/test_underwriting_integration.py index df07b6f8..e5400dca 100644 --- a/tests/test_underwriting_integration.py +++ b/tests/test_underwriting_integration.py @@ -1,5 +1,7 @@ """Integration and E2E tests for the underwriting stage.""" +from decimal import Decimal + import pytest from prei.pipeline.handlers.underwriting import ( UnderwritingInput, @@ -7,10 +9,10 @@ ) BASE = UnderwritingInput( - purchase_price=300000, - estimated_rent=2500, - property_tax_annual=3600, - insurance_annual=1200, + purchase_price=Decimal("300000"), + estimated_rent=Decimal("2500"), + property_tax_annual=Decimal("3600"), + insurance_annual=Decimal("1200"), ) # ═══════════════════════════════════════════════════════════════════════════════ @@ -36,7 +38,7 @@ def test_mao_inversely_related_to_target_cap(self): low = solve_underwriting(BASE, 0.07) high = solve_underwriting(BASE, 0.10) assert high.mao < low.mao - assert high.mao == pytest.approx(low.mao * 0.07 / 0.10, rel=1e-4) + assert float(high.mao) == pytest.approx(float(low.mao) * 0.07 / 0.10, rel=1e-4) def test_rehab_budget_reduces_coc(self): """Adding rehab budget reduces cash-on-cash yield.""" diff --git a/tests_bdd/steps/pipeline_steps.py b/tests_bdd/steps/pipeline_steps.py index 536bfddc..ae6fd292 100644 --- a/tests_bdd/steps/pipeline_steps.py +++ b/tests_bdd/steps/pipeline_steps.py @@ -244,7 +244,7 @@ def then_noi(uw_result): @then("the cap rate should be approximately 5.94%") def then_cap(uw_result): - assert uw_result.cap_rate == pytest.approx(0.0594, rel=1e-3) + assert float(uw_result.cap_rate) == pytest.approx(0.0594, rel=1e-3) @then("the MAO should be approximately $222,750")