feat: implement Phase B of TOP_01_PLAN — financial math verification

.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"}

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(<float literal>)` 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.

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`

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`.

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 |