Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .agents/logs/2026-07-31.jsonl
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"agent": "build", "session_id": "build-20260731-finance-split-001", "triggered_by": "feature-flow", "started_at": "2026-07-31T16:00:00Z", "timestamp": "2026-07-31T17:45:00Z", "duration_ms": 6300000, "skills_loaded": ["code-generation", "refactoring", "template-application"], "findings": [{"id": "FIND-001", "description": "Moved copy of total_return_summary dropped the purchase_price key from its return dict, breaking test_dict_keys_present — restored the key to match the original contract", "actionable": true, "manual_review_needed": false, "severity": "defect"}, {"id": "FIND-002", "description": "after_tax_irr in taxes.py had a local import of irr from utils that was unused (function reimplements npf.irr inline) — removed per ruff F401", "actionable": true, "manual_review_needed": false, "severity": "defect"}, {"id": "FIND-003", "description": "tests/test_underwriting_score.py still tested the deleted pure score_listing_v2 (audit finding #2); kept one_percent_rule/gross_rent_multiplier primitive tests, deleted TestScoreListingV2 since production score lives only in core/services/scoring.py (covered by core/tests/test_scoring_v2.py)", "actionable": true, "manual_review_needed": false, "severity": "defect"}, {"id": "FIND-004", "description": "Service-layer duplicate calculate_noi in core/services/property_service.py was exported but imported by no production code — deleted function, export, and its test file", "actionable": true, "manual_review_needed": false, "severity": "defect"}, {"id": "FIND-005", "description": "Dual-pipeline investigation: Django (PipelineAsset/PipelineProperty + core/services/pipeline.py) is the load-bearing pipeline; the pydantic prei FastAPI router, CLI, and orchestrator are not mounted in any Django URLconf/INSTALLED_APPS/docker-compose; only core/views/__init__.py couples to prei (get_state_landlord_score + lazy DiscoveryProcessor/BatchScreeningProcessor/discover_from_all for the Growth Explorer bridge). Full removal of pydantic state machine requires PM sign-off", "actionable": false, "manual_review_needed": true, "severity": "note"}, {"id": "FIND-006", "description": "Re-export backfill from utils.py was unnecessary: after updating all 17 importers, no remaining importer pulls a moved name from investor_app.finance.utils; keeping the monolith aliases would defeat the split", "actionable": false, "manual_review_needed": false, "severity": "note"}, {"id": "FIND-007", "description": "Pre-existing mypy error in tests/acceptance/conftest.py:69 (no-any-return) unrelated to this change — file unmodified", "actionable": false, "manual_review_needed": false, "severity": "note"}], "decision": "implemented", "blockers": [], "pr": null}
54 changes: 54 additions & 0 deletions .agents/reports/build-report-2026-07-31-finance-split.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
## Build Report — Finance utils split + audit finding resolution (LIMIT-20)

**Status:** COMPLETE

---

### Tasks Completed

| Task | Title | Lines Changed | Status |
| -------- | ----- | ------------- | ------ |
| SPLIT-1 | Create `investor_app/finance/mortgage.py` (9 functions: mortgage, carrying costs, break-even rent, paydown, appreciation, ROI components) | ~330 | DONE |
| SPLIT-2 | Create `investor_app/finance/taxes.py` (11 functions: depreciation, tax benefits, after-tax IRR/CF, hold-period projections, sale proceeds, recapture) | ~640 | DONE |
| SPLIT-3 | Create `investor_app/finance/scoring.py` (5 primitives: 1% rule, GRM, price-to-rent, market normalization helpers) | ~150 | DONE |
| SPLIT-4 | Create `investor_app/finance/strategies.py` (8 functions: flip, rental, vacation, BRRRR calculators; `estimate_rehab_cost` decoupled from settings) | ~400 | DONE |
| SPLIT-5 | Rewrite `investor_app/finance/utils.py` to core math + Django-coupled analysis only; delete aliases (`calculate_noi`/`calculate_cap_rate`/`calculate_cash_on_cash`/`calculate_irr`), dead `score_listing_v1`, and deprecated `score_listing_v1_deprecated` chain | ~231 (net −2116) | DONE |
| IMPORTERS | Update 17 importers (services, api_views, views, tests) to canonical module locations | ~120 | DONE |
| AUDIT-1 | Remove dead service-layer duplicate `calculate_noi` from `core/services/property_service.py` (+ export in `__init__.py`, delete `tests/test_property_service.py`) | −45 | DONE |
| AUDIT-2 | Delete duplicate pure `score_listing_v2` tests (`TestScoreListingV2` in `tests/test_underwriting_score.py`); production version remains only in `core/services/scoring.py` | −161 | DONE |
| VERIFY | Fix behavior regression: `total_return_summary` must keep returning `purchase_price` key (moved copy dropped it) | +3 | DONE |
| DOCS | Mark LIMIT-20 resolved in `docs/KNOWN_LIMITATIONS.md` | +8 | DONE |

### Artifacts Produced

- [x] Source code files — `investor_app/finance/{mortgage,taxes,scoring,strategies}.py`
- [x] Source code files — rewritten `investor_app/finance/utils.py`
- [ ] Manifests in `manifests/` — N/A (no K8s surface touched)
- [ ] Pipeline in `pipeline-spec.yaml` — N/A (no CI pipeline change)
- [ ] Overlays in `overlays/` — N/A (no GitOps change)

### Validation Results

| Check | Status |
| --------- | ------ |
| Lint (ruff) | PASS |
| Typecheck (mypy, touched files) | PASS |
| Tests (full suite) | PASS — 1803 passed, 1 skipped, 261 deselected |
| Policy | PASS — no governance violations; `postgres`, `migration-safety`, `gitops` untouched |

Pre-existing mypy error in `tests/acceptance/conftest.py:69` (no-any-return) is unrelated to this change — file unmodified.

### Blockers

None.

### Dual-pipeline (pydantic `prei` vs Django) findings

Per the user's directive to resolve the dual-pipeline question "using Django," investigated how much production code depends on the pydantic `prei` side:

- **Django is the load-bearing pipeline**: `core/models/pipeline.py` (`PipelineAsset`, `PipelineProperty`), `core/services/pipeline.py`, screening, leasing, notifications, and ~20 prod/test files. This is the source of truth.
- **`prei` pydantic side is a standalone FastAPI microservice, not mounted**: `prei/api/pipeline_routes.py` (FastAPI router) and `prei/cli.py` are not referenced by any Django URLconf, `INSTALLED_APPS`, docker-compose service, or CI deploy. `prei/pipeline/orchestrator.py` is imported only by tests.
- **One production coupling**: `core/views/__init__.py` imports `prei.integrations.landlord_data.get_state_landlord_score` (top-level) and lazily imports `DiscoveryProcessor`/`BatchScreeningProcessor`/`ScreeningThresholds`/`PipelineEngine`/`InMemoryAssetRepository`/`discover_from_all` for the Growth Explorer bridge (P0 fix from `docs/assessments/AUDIT_GA_PIPELINE.md`).
- **Recommendation**: keep the pydantic discovery/screening *processors* (they are the only working bridge from Growth Explorer into pipeline screening, and they are Decimal-based after Phase B) but do not build new state on pydantic models (`PropertyAsset`/`StageLog`); persist pipeline state via Django `PipelineProperty`/`PipelineAsset`. Full removal of the pydantic state machine, FastAPI routes, and CLI is a separate reviewed change requiring PM sign-off — it touches the orchestrator, handlers, 11 test files, and the Growth Explorer bridge. Filed as a follow-up recommendation, not executed here.

The finance-utils split itself is independent of that decision: `prei/pipeline/{orchestrator,handlers/underwriting}.py` still import only `to_decimal`/`cap_rate`/`cash_on_cash` from `investor_app.finance.utils`, all of which remain in place.
8 changes: 6 additions & 2 deletions core/api_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,17 @@
from .models import VrmProperty
from .serializers import VrmPropertySerializer

from investor_app.finance.utils import (
from investor_app.finance.mortgage import (
calculate_break_even_rent,
calculate_carrying_costs as calc_costs,
calculate_roi_components,
)
from investor_app.finance.strategies import (
calculate_flip_strategy,
calculate_rental_strategy,
calculate_roi_components,
calculate_vacation_rental_strategy,
)
from investor_app.finance.utils import (
cap_rate as calc_cap_rate,
cash_on_cash as calc_coc,
compute_analysis_for_property,
Expand Down
2 changes: 0 additions & 2 deletions core/services/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@

from core.services.portfolio import compute_portfolio_summary
from core.services.property_service import (
calculate_noi,
compute_noi,
compute_noi_for_user,
compute_noi_from_amounts,
)

__all__ = [
"calculate_noi",
"compute_noi",
"compute_noi_for_user",
"compute_noi_from_amounts",
Expand Down
8 changes: 3 additions & 5 deletions core/services/brrrr.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,15 @@
from django.conf import settings

from core.models import Listing
from investor_app.finance.utils import (
from investor_app.finance.mortgage import calculate_monthly_mortgage
from investor_app.finance.strategies import (
brrrr_coc_return,
calculate_monthly_mortgage,
cash_left_in_deal,
dscr,
estimate_arv,
estimate_rehab_cost,
max_refinance_loan,
noi,
to_decimal,
)
from investor_app.finance.utils import dscr, noi, to_decimal

logger = logging.getLogger(__name__)

Expand Down
9 changes: 2 additions & 7 deletions core/services/cma.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,8 @@
from django.conf import settings

from core.models import Listing, MarketSnapshot
from investor_app.finance.utils import (
calculate_monthly_mortgage,
cap_rate,
cash_on_cash,
dscr,
noi,
)
from investor_app.finance.mortgage import calculate_monthly_mortgage
from investor_app.finance.utils import cap_rate, cash_on_cash, dscr, noi

logger = logging.getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion core/services/market_scoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def _score_market_from_snapshot(snapshot) -> Decimal:
Returns a Decimal in [0, 100] computed as a weighted average of
normalised sub-scores for each available signal.
"""
from investor_app.finance.utils import (
from investor_app.finance.scoring import (
clamp_market_score,
normalize_market_growth_rate_score,
normalize_market_price_to_rent_score,
Expand Down
3 changes: 2 additions & 1 deletion core/services/portfolio.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,8 @@ def calculate_ytd_cashflow(

def _get_annual_debt_service(property_obj: Property) -> Decimal:
"""Calculate annual debt service for a property."""
from investor_app.finance.utils import calculate_monthly_mortgage, to_decimal
from investor_app.finance.mortgage import calculate_monthly_mortgage
from investor_app.finance.utils import to_decimal

loan_amount = to_decimal(property_obj.purchase_price) * (
Decimal("1") - to_decimal(property_obj.down_payment_pct)
Expand Down
6 changes: 3 additions & 3 deletions core/services/projections.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@
import numpy as np
import numpy_financial as npf

from investor_app.finance.utils import (
from investor_app.finance.mortgage import calculate_monthly_mortgage
from investor_app.finance.taxes import (
calculate_after_tax_cashflow,
calculate_annual_depreciation,
calculate_monthly_mortgage,
to_decimal,
)
from investor_app.finance.utils import to_decimal

if TYPE_CHECKING:
from core.models import Property
Expand Down
23 changes: 0 additions & 23 deletions core/services/property_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,26 +130,3 @@ def compute_noi_from_amounts(
Annual NOI as a ``Decimal``, quantized to two decimal places.
"""
return noi(monthly_income, monthly_expenses).quantize(Decimal("0.01"))


def calculate_noi(
gross_income: Decimal,
operating_expenses: Decimal,
) -> Decimal:
"""Calculate annual Net Operating Income (NOI).

NOI = Gross Income - Operating Expenses

This is a pure service-layer function that delegates to the finance
utility layer. It does not touch the database.

Args:
gross_income: Total annual gross income from the property.
operating_expenses: Total annual operating expenses (excluding debt service).

Returns:
Annual NOI as a ``Decimal``, quantized to two decimal places.
"""
return (to_decimal(gross_income) - to_decimal(operating_expenses)).quantize(
Decimal("0.01")
)
13 changes: 4 additions & 9 deletions core/services/scoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,11 @@ def score_listing_v2(property_obj, targets) -> UnderwritingScore:
Returns:
UnderwritingScore with all fields populated.
"""
from investor_app.finance.utils import (
build_cashflows,
from investor_app.finance.taxes import (
calculate_annual_depreciation,
calculate_after_tax_cashflow,
irr as calc_irr,
)
from investor_app.finance.utils import build_cashflows, irr as calc_irr
from core.models import UserProfile

pp = property_obj.purchase_price
Expand Down Expand Up @@ -167,12 +166,8 @@ def score_listing_v2(property_obj, targets) -> UnderwritingScore:
total_expenses = opex + mgmt_fee
annual_noi = effective_rent - total_expenses
# KPIs using utils functions
from investor_app.finance.utils import (
cap_rate as calc_cap_rate,
cash_on_cash,
dscr,
gross_rent_multiplier,
)
from investor_app.finance.scoring import gross_rent_multiplier
from investor_app.finance.utils import cap_rate as calc_cap_rate, cash_on_cash, dscr

cap = calc_cap_rate(annual_noi, pp)
grm = gross_rent_multiplier(pp, annual_rent) if annual_rent > 0 else Decimal("999")
Expand Down
14 changes: 6 additions & 8 deletions core/tests/test_deal_analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,16 @@

import pytest

from investor_app.finance.utils import (
from investor_app.finance.mortgage import (
calculate_break_even_rent,
calculate_flip_strategy,
calculate_monthly_mortgage,
calculate_rental_strategy,
calculate_roi_components,
cap_rate,
cash_on_cash,
dscr,
irr,
noi,
)
from investor_app.finance.strategies import (
calculate_flip_strategy,
calculate_rental_strategy,
)
from investor_app.finance.utils import cap_rate, cash_on_cash, dscr, irr, noi

# ---------------------------------------------------------------------------
# noi
Expand Down
12 changes: 7 additions & 5 deletions core/tests/test_finance_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@

import pytest

from investor_app.finance.utils import (
build_cashflows,
from investor_app.finance.mortgage import (
calculate_appreciation,
calculate_break_even_rent,
calculate_carrying_costs,
Expand All @@ -13,12 +12,15 @@
calculate_principal_paydown,
calculate_property_tax,
calculate_roi_components,
calculate_tax_benefits,
estimate_insurance,
)
from investor_app.finance.taxes import calculate_tax_benefits
from investor_app.finance.utils import (
build_cashflows,
cap_rate,
cash_on_cash,
dscr,
estimate_insurance,
compute_analysis_for_property,
dscr,
irr,
noi,
)
Expand Down
6 changes: 3 additions & 3 deletions docs/KNOWN_LIMITATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,15 @@ 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`
### [LIMIT-20] 🟡 HIGH — Divergent bare-function vs. `calculate_*` contracts for the same formulas, plus a duplicate `score_listing_v2` (resolved)

**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.
**Workaround:** Resolved by the finance-utils split. The `calculate_*` aliases (`calculate_noi`, `calculate_cap_rate`, `calculate_cash_on_cash`, `calculate_irr`), the dead `score_listing_v1` chain, and the duplicate pure `score_listing_v2` were deleted. The bare functions (`noi`, `cap_rate`, `cash_on_cash`, `dscr`, `irr`) are now the single source of truth in `investor_app/finance/utils.py`, with the production underwriting score living only in `core/services/scoring.py`. The service-layer duplicate `calculate_noi` in `core/services/property_service.py` was also removed.

**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.
**Fix tracked in:** Resolved in the finance-utils split PR (finance package reorganized into `mortgage`, `taxes`, `scoring`, `strategies` submodules).

---

Expand Down
Loading
Loading