Merge pull request #1661 from Hestia-Homes/fix/persist-rafter-insulation-thickness

Persist the measured at-rafters roof insulation thickness

docs/migrations/epc-building-part-fabric-overrides.md

@@ -0,0 +1,133 @@
+# Migration note — `epc_building_part` RdSAP construction inputs
+
+**For the team to action alongside the model-side change (Hestia-Homes/Model#1656).**
+The Python side is done and tested against the SQLModel-built test DB; the
+**production Drizzle schema needs one matching column** that this repo does not
+own.
+
+## The bug this closes
+
+`SapBuildingPart.rafter_insulation_thickness` is read by the SAP calculator
+(`heat_transmission.py:922`) but had no DB column, so it was mapped correctly
+from the gov-EPC API and then **silently discarded on save** (*persist != score*).
+
+RdSAP 10 §5.11.2: a roof insulated **at rafters** lodges its insulation thickness
+in this dedicated field, **not** in `roof_insulation_thickness` (which stays null
+for rafter roofs, since rafters aren't loft joists). With the thickness the roof
+bills on the **measured** Table 16 column (2) row; without it, it falls back to
+the Table 18 column (2) unknown-thickness age-band default — `2.30` for bands
+A–D, i.e. the spec's *uninsulated* value.
+
+**Measured impact.** Property 749719 (portfolio 824, End-terrace bungalow, band
+D, RdSAP SAP 10.2, inspected 2026-01-20) lodges `"250mm"` → Table 16 col (2)
+U **0.23**. Dropped, the roof bills at U **2.30** — a 10x error:
+
+| quantity | lodged | dropped | with the field |
+|---|---:|---:|---:|
+| SAP | 73 (band C) | 63.22 (band D) | 72.65 |
+| primary energy kWh/m²/yr | 212 | 327.9 | 212.3 |
+| CO2 t/yr | 1.5 | 2.40 | 1.54 |
+
+Confirmed on a second property, 750189 (lodged 74 → 68.08 dropped, 74.11 with
+250 mm). Across portfolios 796 + 824, **0 of 1,449 certs that score within ±0.5
+of lodged carry `roof_insulation_location == 1`** — the at-rafters path never
+passes today. The defect is real but **narrow**: 4 properties in those two
+portfolios, and the field is populated on 17 of 1,000 (1.7%) certs in the
+committed RdSAP-21.0.1 corpus.
+
+Secondary harm: 749719 draws a **loft-insulation recommendation worth 9.63 SAP**
+on a roof an accredited assessor rated 4/5.
+
+## Required Drizzle migration — `epc_building_part`
+
+Additive, nullable, **no backfill possible** (the value was never stored):
+
+| Column | Type | Null | Notes |
+|---|---|---|---|
+| `rafter_insulation_thickness` | `jsonb` | yes | `Union[str, int]` — `"250mm"` from the API, int mm from Site Notes. **JSONB**, matching the existing `roof_insulation_thickness` / `flat_roof_insulation_thickness` / `wall_insulation_thickness` columns, so `int` vs `str` survives the round-trip. |
+| `wall_is_basement` | `boolean` | **yes — load-bearing** | Must be nullable, **not** `NOT NULL DEFAULT false`. `null` = "not stated, fall back to the gov-API code-6 heuristic"; `false` = "explicitly system-built, do NOT apply the heuristic". Collapsing `false` to `null` inverts the result. |
+
+Generate with `npm run migration:generate`; run it **before** deploying the
+backend code.
+
+## Backfill / re-ingest
+
+The column never existed, so **existing rows have nothing to restore**. Every
+affected property must be **re-ingested from the gov-EPC API** before its score
+moves; the migration alone changes no existing baseline. Re-run **Baseline
+(stage 2) before Modelling (stage 3)** so both stages score on one calculator
+version.
+
+## The second column — `wall_is_basement`
+
+RdSAP 10 §5.17 / Table 23. It is **not** a lodged U-value: it is a
+disambiguation flag selecting *which* RdSAP default applies, so it belongs with
+`rafter_insulation_thickness`, not with the U-values below.
+
+RdSAP code 6 is canonically **system-built**, and the gov-API path infers
+"basement" from a code-6 heuristic. The Elmhurst site-notes mapper sets
+`wall_is_basement=False` precisely to defeat that heuristic. Dropping the field
+does not merely lose data — it **inverts** the answer:
+
+```
+wall_construction = 6 (system-built)
+  Elmhurst sets wall_is_basement=False  -> main_wall_is_basement = False
+  after DB round-trip (dropped -> None) -> main_wall_is_basement = True   <- flipped
+```
+
+A system-built wall then bills as a basement wall, and via `has_basement` drags
+the whole ground floor onto the Table 23 basement-floor U-value.
+
+The field is declared on **no** RdSAP schema, so it never arrives via the gov
+API — but the Elmhurst site-notes path sets it, and site-notes EPCs **are**
+persisted (`scripts/wchg_elmhurst_ingest.py`). A corpus scan of gov-API certs
+shows 0 occurrences and is therefore the wrong population to measure it on.
+
+Rare but real: `main_wall_is_basement` is documented at 54 of 67k parts in the
+2026 sweep.
+
+## Deliberately NOT persisted
+
+The same audit found three sibling `SapBuildingPart` fields with the same drop —
+`wall_u_value`, `roof_u_value`, `floor_u_value`. They are
+**intentionally left unpersisted**: they are full-SAP artefacts, and we re-model
+every dwelling *as* an RdSAP assessment, which derives U-values from the
+construction-default cascade (RdSAP 10 §5.6/§5.7/§5.11) rather than honouring an
+assessor's measured value. Carrying them would make the re-model a hybrid.
+
+Population on the committed RdSAP-21.0.1 corpus (1,000 certs) confirms it:
+`roof_u_value` **0**, `floor_u_value` **0**, `wall_u_value` **4 (0.4%)**.
+
+They stay on the `_UNPERSISTED_ALLOWLIST` in
+`tests/repositories/epc/test_epc_persistence_field_coverage.py`, but with a
+**corrected** justification — see below.
+
+## Why the structural guard missed it
+
+The ADR-0036 guard exists precisely to catch this class of silent drop. All five
+fields were allow-listed with the justification **"dormant — not read by the
+calculator; no FE column"**. That was true when written; it expired silently when
+the §5.1 / §5.11.2 override code landed in `heat_transmission.py`, and nothing
+re-checked the premise. A stale allow-list entry is worse than no entry, because
+it reads as a deliberate, reviewed decision.
+
+The four remaining entries now say *"deliberate — full-SAP only; RdSAP re-model
+uses default U-values (#1656)"*, which is both true and the actual reason.
+
+**Worth considering:** make the justification machine-checkable — fail the guard
+if a field claiming "not read by the calculator" appears in an attribute read
+under `domain/sap10_calculator/`.
+
+## Model-side change (already done)
+
+- `infrastructure/postgres/epc_property_table.py` — `EpcBuildingPartModel` gains
+  `rafter_insulation_thickness`; `from_domain` writes it.
+- `repositories/epc/epc_postgres_repository.py` — `_to_building_part`
+  reconstructs it.
+- `tests/repositories/epc/test_epc_round_trip.py` —
+  `test_rafter_insulation_thickness_round_trips` pins it deep-equal;
+  `test_wall_is_basement_round_trips` pins the code-6 inversion case.
+- `tests/repositories/epc/test_epc_persistence_field_coverage.py` — the three
+  U-value entries re-justified; the `rafter_insulation_thickness` and
+  `wall_is_basement` entries removed so the guard now *enforces* their
+  reconstruction.

docs/migrations/epc-property-round-trip-fidelity.md

@@ -105,11 +105,19 @@ guard now *enforces* their reconstruction.
 | `cylinder_volume_measured_l` | `SapHeating` | `epc_property.heating_cylinder_volume_measured_l` |
 | `air_permeability_ap50_m3_h_m2` | `SapVentilation` | `epc_property.ventilation_air_permeability_ap50_m3_h_m2` |
 
-(Still allow-listed as dormant — not calculator-read, no column: `SapBuildingPart.{floor_u_value,
-roof_u_value, wall_u_value, wall_is_basement, rafter_insulation_thickness}`,
+(Still allow-listed as dormant — not calculator-read, no column:
 `SapAlternativeWall.is_basement`, `SapHeating.cylinder_heat_loss`. And `SapAlternativeWall.{u_value,
 wall_thickness_mm}` remain FE-column-pending.)
 
+> **Superseded (#1656).** `SapBuildingPart.{floor_u_value, roof_u_value, wall_u_value,
+> wall_is_basement, rafter_insulation_thickness}` were previously allow-listed here as
+> "dormant — not calculator-read". **That justification was false**: all five are read by
+> `heat_transmission.py` as authoritative overrides of the U-value cascade (§5.1 lodged
+> U-values, §5.11.2 at-rafters thickness, §5.17 basement wall). Dropping them cost
+> **−9.8 SAP** on property 749719 (a whole band, C → D). They are now persisted and their
+> allow-list entries removed — see
+> [`epc-building-part-fabric-overrides.md`](epc-building-part-fabric-overrides.md).
+
 ---
 
 ## 1. Type fidelity — convert `Union[int, str]` code columns to JSONB

infrastructure/postgres/epc_property_table.py

@@ -650,6 +650,24 @@ class EpcBuildingPartModel(SQLModel, table=True):
     roof_insulation_thickness: Optional[Union[str, int]] = Field(
         default=None, sa_column=Column(JSONB, nullable=True)
     )
+    # RdSAP 10 §5.11.2 — a roof insulated AT RAFTERS lodges its thickness here,
+    # NOT in `roof_insulation_thickness` (which stays null for rafter roofs).
+    # `heat_transmission.py:922` prefers it when `roof_insulation_location == 1`
+    # so the measured Table 16 column (2) row applies; without it the roof falls
+    # to the Table 18 column (2) unknown-thickness default (2.30 for bands A-D),
+    # a 10x U-value error. Union[str, int] ("250mm" from the API) → JSONB, like
+    # the `roof_insulation_thickness` sibling.
+    rafter_insulation_thickness: Optional[Union[str, int]] = Field(
+        default=None, sa_column=Column(JSONB, nullable=True)
+    )
+    # RdSAP 10 §5.17 / Table 23 — selects the basement-wall U-value column for
+    # the part's primary wall (`heat_transmission.py:955` via
+    # `SapBuildingPart.main_wall_is_basement`). NOT a lodged U-value: it picks
+    # WHICH RdSAP default applies, so unlike wall/roof/floor_u_value it must be
+    # persisted. Nullable is load-bearing — `None` means "not stated, fall back
+    # to the gov-API code-6 heuristic", `False` means "explicitly system-built,
+    # do NOT apply the heuristic". Collapsing False to None inverts the answer.
+    wall_is_basement: Optional[bool] = Field(default=None)
     room_in_roof_floor_area: Optional[float] = Field(default=None)
     room_in_roof_construction_age_band: Optional[str] = Field(default=None)
     alt_wall_1_area: Optional[float] = Field(default=None)
@@ -699,6 +717,8 @@ class EpcBuildingPartModel(SQLModel, table=True):
             curtain_wall_age=part.curtain_wall_age,
             roof_insulation_location=part.roof_insulation_location,
             roof_insulation_thickness=part.roof_insulation_thickness,
+            rafter_insulation_thickness=part.rafter_insulation_thickness,
+            wall_is_basement=part.wall_is_basement,
             room_in_roof_floor_area=float(rir.floor_area) if rir else None,
             room_in_roof_construction_age_band=(
                 rir.construction_age_band if rir else None

repositories/epc/epc_postgres_repository.py

@@ -1003,6 +1003,8 @@ class EpcPostgresRepository(EpcRepository):
             curtain_wall_age=bp.curtain_wall_age,
             roof_insulation_location=bp.roof_insulation_location,
             roof_insulation_thickness=bp.roof_insulation_thickness,
+            rafter_insulation_thickness=bp.rafter_insulation_thickness,
+            wall_is_basement=bp.wall_is_basement,
             sap_room_in_roof=(
                 SapRoomInRoof(
                     floor_area=bp.room_in_roof_floor_area,

tests/repositories/epc/test_epc_persistence_field_coverage.py

@@ -60,12 +60,26 @@ _UNPERSISTED_ALLOWLIST: dict[str, str] = {
     "SapRoomInRoofSurface": "FE child table pending — room-in-roof surface detail",
     # --- Nested gaps the calculator does NOT read (dormant); no FE column.
     "SapAlternativeWall.is_basement": "dormant — not read by the calculator; no FE column",
-    "SapBuildingPart.floor_u_value": "dormant — not read by the calculator (column exists, unread)",
-    "SapBuildingPart.rafter_insulation_thickness": "dormant — not read by the calculator; no FE column",
-    "SapBuildingPart.roof_u_value": "dormant — not read by the calculator; no FE column",
-    "SapBuildingPart.wall_is_basement": "dormant — not read by the calculator; no FE column",
-    "SapBuildingPart.wall_u_value": "dormant — not read by the calculator; no FE column",
     "SapHeating.cylinder_heat_loss": "dormant — not read by the calculator; no FE column",
+    # --- DELIBERATELY not persisted (#1656). These four ARE read by the
+    # calculator (`heat_transmission.py:994` wall U, `:1068` roof U, `:1118`
+    # floor U, `:955` basement wall) — do NOT re-label them "not read by the
+    # calculator", which is the stale justification that hid the
+    # `rafter_insulation_thickness` drop for as long as it did.
+    #
+    # They are full-SAP artefacts. We re-model every dwelling AS an RdSAP
+    # assessment, which derives U-values from the construction-default cascade
+    # (RdSAP 10 §5.6/§5.7/§5.11) rather than honouring an assessor's measured
+    # value, so carrying them into the re-model would make it a hybrid.
+    # Measured on the committed RdSAP-21.0.1 corpus (1000 certs): roof_u_value
+    # 0, floor_u_value 0, wall_u_value 4 (0.4%).
+    #
+    # `wall_is_basement` is deliberately NOT in this list — it is a
+    # disambiguation flag selecting which RdSAP default applies (§5.17/Table 23),
+    # not a measured U-value, and it IS persisted.
+    "SapBuildingPart.wall_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)",
+    "SapBuildingPart.roof_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)",
+    "SapBuildingPart.floor_u_value": "deliberate — full-SAP only; RdSAP re-model uses default U-values (#1656)",
 }
 
 

tests/repositories/epc/test_epc_round_trip.py

@@ -240,3 +240,92 @@ def test_building_part_wall_insulation_thickness_preserves_int(
     value = reloaded.sap_building_parts[0].wall_insulation_thickness
     assert value == 100
     assert isinstance(value, int)
+
+
+def test_rafter_insulation_thickness_round_trips(db_engine: Engine) -> None:
+    # RdSAP 10 §5.11.2 — a roof insulated AT RAFTERS lodges its insulation
+    # thickness in `rafter_insulation_thickness`, NOT `roof_insulation_thickness`
+    # (which stays null for rafter roofs, since rafters aren't loft joists).
+    # `heat_transmission.py:922` prefers it when `roof_insulation_location == 1`
+    # so the MEASURED Table 16 column (2) row applies. It had no DB column, so it
+    # was silently dropped on save and every at-rafters roof fell back to the
+    # Table 18 column (2) unknown-thickness default (persist != score).
+    #
+    # Real impact: property 749719 lodges "250mm" -> Table 16 col (2) U 0.23.
+    # Dropped, the roof bills at U 2.30 — a 10x error worth -9.8 SAP (lodged 73,
+    # calculated 63.2) that moved the dwelling a whole band, C -> D.
+    #
+    # Note this is an RdSAP INPUT (an insulation thickness feeding the standard
+    # cascade), not a lodged U-value. The sibling `wall/roof/floor_u_value` and
+    # `wall_is_basement` fields are full-SAP artefacts and are deliberately NOT
+    # persisted — see the `_UNPERSISTED_ALLOWLIST` note in
+    # test_epc_persistence_field_coverage.py.
+    from dataclasses import replace
+
+    # Arrange — a green fixture carrying a measured at-rafters thickness.
+    original = _load_epc("RdSAP-Schema-21.0.1")
+    assert original.sap_building_parts, "fixture must have a building part"
+    bp0 = replace(
+        original.sap_building_parts[0],
+        rafter_insulation_thickness="250mm",
+    )
+    original = replace(
+        original, sap_building_parts=[bp0, *original.sap_building_parts[1:]]
+    )
+
+    # Act
+    with Session(db_engine) as session:
+        epc_property_id = EpcPostgresRepository(session).save(original)
+        session.commit()
+    with Session(db_engine) as session:
+        reloaded = EpcPostgresRepository(session).get(epc_property_id)
+
+    # Assert — the measured rafter thickness survives, deep-equal.
+    assert reloaded == original
+
+
+def test_wall_is_basement_round_trips(db_engine: Engine) -> None:
+    # RdSAP 10 §5.17 / Table 23 — `wall_is_basement` decides whether a part's
+    # primary wall bills on the basement-wall column (and, via `has_basement`,
+    # drags the whole ground floor onto the Table 23 basement-floor U-value).
+    #
+    # It is NOT a lodged U-value: it is a disambiguation flag that selects WHICH
+    # RdSAP default applies, so unlike wall/roof/floor_u_value it must survive
+    # the round-trip. The gov API never lodges it (declared on no RdSAP schema);
+    # the Elmhurst site-notes mapper sets it, and site-notes EPCs ARE persisted
+    # (scripts/wchg_elmhurst_ingest.py).
+    #
+    # Dropping it is worse than lossy, it INVERTS: RdSAP code 6 is canonically
+    # SYSTEM-BUILT, and the API path infers "basement" from a code-6 heuristic.
+    # Elmhurst sets `wall_is_basement=False` precisely to defeat that heuristic.
+    # Round-tripping False -> None silently re-enables it, so a system-built wall
+    # comes back as a basement wall.
+    from dataclasses import replace
+
+    from datatypes.epc.domain.epc_property_data import (
+        BASEMENT_WALL_CONSTRUCTION_CODE,
+    )
+
+    # Arrange — a system-built (code 6) main wall explicitly flagged NOT basement.
+    original = _load_epc("RdSAP-Schema-21.0.1")
+    assert original.sap_building_parts, "fixture must have a building part"
+    bp0 = replace(
+        original.sap_building_parts[0],
+        wall_construction=BASEMENT_WALL_CONSTRUCTION_CODE,
+        wall_is_basement=False,
+    )
+    original = replace(
+        original, sap_building_parts=[bp0, *original.sap_building_parts[1:]]
+    )
+    assert original.sap_building_parts[0].main_wall_is_basement is False
+
+    # Act
+    with Session(db_engine) as session:
+        epc_property_id = EpcPostgresRepository(session).save(original)
+        session.commit()
+    with Session(db_engine) as session:
+        reloaded = EpcPostgresRepository(session).get(epc_property_id)
+
+    # Assert — the wall is still system-built, not silently re-read as basement.
+    assert reloaded is not None
+    assert reloaded.sap_building_parts[0].main_wall_is_basement is False