chore: retro-review PR #85 — refactor(integration): typed runtime data via PriceHawkData (Phase 7 PR-1)

CHANGELOG.md

@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ## [Unreleased]
 
+### Changed
+
+- Internal: typed runtime data via `PriceHawkData` dataclass + `PriceHawkConfigEntry` alias; `entry.runtime_data` replaces `hass.data[DOMAIN][entry_id]` for coordinator storage. Service handlers (`analyze_csv`, `backfill_history`, `rank_alternatives`) re-resolve the coordinator on every invocation, eliminating a stale-closure bug latent across `OptionsFlowWithReload` cycles. Unload re-ordered: `async_unload_platforms` runs first, coordinator teardown only on success. Multi-entry service deregistration now sourced from `hass.config_entries.async_entries(DOMAIN)`. No user-facing change. (Phase 7 / PR-1)
+
 ### Documentation
 
 - Refreshed `README.md` for Phase 3: per-window dashboard tabs, ranked-alternatives table, CDR `Other (no API)` retailer path, partial-window mode, services FAQ.

DECISIONS.md

@@ -5,7 +5,33 @@
 
 <!-- Add new decisions at the top -->
 
-## 2026-05-14 — Phase 1 entry corrections
+## 2026-05-20 — Phase 7 Plan 01 (typed runtime data)
+
+### D-P7-1 — Adopt `PriceHawkConfigEntry = ConfigEntry[PriceHawkData]` typed-entry alias
+**Decision:** Introduce `custom_components/pricehawk/data.py` exporting `PriceHawkData` (`@dataclass(slots=True)`) and `PriceHawkConfigEntry: TypeAlias = ConfigEntry[PriceHawkData]`. All future `entry: PriceHawkConfigEntry` annotations use this alias. Coordinator storage moves from `hass.data[DOMAIN][entry_id]` to `entry.runtime_data`.
+**Rationale:** PR-1 from `PriceHawk v2 — Deep Research Round 2 (Scope-Corrected).md`, Wave 1 Foundation. Required prerequisite for Phase 8 Silver-compliance handlers (reauth, reconfigure, diagnostics) which all need a single typed object to reach into. HA core convention since 2024.
+**Alternatives:** Continue using `hass.data[DOMAIN]` — rejected as it blocks Silver compliance, leaks the multi-entry sentinel responsibility, and forces every consumer to know the entry-id-keyed indirection.
+**Consequences:** Every Phase 8 PR consumes this alias. Dataclass kept mutable (`slots=True`, NOT `frozen`) so additive fields can land in later PRs without re-creating the object.
+
+### D-P7-2 — Service handlers must re-resolve coordinator on every invocation
+**Decision:** The three registered service handlers (`analyze_csv`, `backfill_history`, `rank_alternatives`) read the coordinator via a `_resolve_coordinator()` helper that reads `entry.runtime_data` on every call. No closure capture of `coordinator` in setup scope.
+**Rationale:** Latent pre-existing bug surfaced (not introduced) by this PR: `OptionsFlowWithReload` (HA 2026.3+) triggers a setup→unload→setup cycle on options save. The entry object survives (same identity) but `entry.runtime_data.coordinator` is replaced with a fresh `PriceHawkCoordinator`. A handler that closed over `coordinator` from the original `async_setup_entry` scope would silently keep firing methods on the dead coordinator forever. The typed-runtime-data migration makes the failure mode more visible, so fixed in the same PR.
+**Alternatives:** Re-register the services on every `async_setup_entry` — rejected because the multi-entry sentinel only deregisters when the LAST entry unloads. Multiple registrations of the same service name in HA throw.
+**Consequences:** Sets the pattern for all future service handlers in this integration. `test_service_handlers_resolve_fresh_coordinator` enforces it.
+
+### D-P7-3 — `async_unload_entry` reordered: platform-unload first, coordinator teardown only on success
+**Decision:** `async_unload_platforms` runs FIRST in `async_unload_entry`. If it returns False, return False immediately with `entry.runtime_data` left intact so HA can retry the unload. Coordinator timer cancellation + state persistence happen ONLY after a successful platform unload.
+**Rationale:** The previous order (cancel timers → persist state → unload platforms) left the entry in a half-unloaded state on platform-unload failure — coordinator was already torn down with no recovery path. Audit Gap #4.
+**Alternatives:** Try/finally pattern with restore-on-failure — rejected as the simpler reorder produces equivalent correctness without restore complexity.
+**Consequences:** HA can safely retry `async_unload_entry` after a failure. Documented in `<verification>` MANUAL SMOKE step (multi-entry add/remove cycle).
+
+### D-P7-4 — Multi-entry singleton-service sentinel via `hass.config_entries.async_entries(DOMAIN)`
+**Decision:** Singleton-service deregistration (the three services unregistered when the last PriceHawk entry leaves) now reads the config-entries registry, not `hass.data`. Filters out the entry being unloaded explicitly via `entry_id` comparison (whether HA includes or excludes it from `async_entries(DOMAIN)` at unload time varies by HA version — explicit filter is version-safe).
+**Rationale:** PR removed `hass.data[DOMAIN]` entirely. Audit Gap #1: previous sentinel (`if not hass.data.get(DOMAIN)`) became unreachable garbage after the removal. Production-breaking for any HACS user with two PriceHawk entries (one per house) — either premature deregistration (services break) or services never unregistered (leak across HA restarts).
+**Alternatives:** Module-level counter — rejected because it diverges from HA's authoritative source of truth (config-entries registry).
+**Consequences:** `test_multi_entry_service_lifecycle` enforces the contract. Future entries (e.g. multi-NMI households) work correctly.
+
+
 
 ### D-P0-7 — Evaluator bug fixes (post-gate, during Phase 1 parity work)
 **Decision:** Two bugs corrected in `scripts/cdr_evaluator_proto.py`. Phase 0 gate result stands — bugs were masked by Plan C2's specifics + your hand-calc presumably caught the right semantics. Re-verify with `phase_0_verify.py --markdown`.

custom_components/pricehawk/__init__.py

@@ -2,7 +2,6 @@
 
 import logging
 
-from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant
 
 from .const import (
@@ -16,22 +15,22 @@
     remove_panel,
     setup_panel_iframe,
 )
+from .data import PriceHawkConfigEntry, PriceHawkData
 
 _LOGGER = logging.getLogger(__name__)
 
 PLATFORMS = ["sensor"]
 
 
-async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
+async def async_setup_entry(hass: HomeAssistant, entry: PriceHawkConfigEntry) -> bool:
     """Set up PriceHawk from a config entry."""
     _LOGGER.info("Setting up PriceHawk integration (entry=%s)", entry.entry_id)
-    hass.data.setdefault(DOMAIN, {})
 
     coordinator = PriceHawkCoordinator(hass, entry)
     await coordinator.async_restore_state()
     await coordinator.async_config_entry_first_refresh()
 
-    hass.data[DOMAIN][entry.entry_id] = coordinator
+    entry.runtime_data = PriceHawkData(coordinator=coordinator)
 
     await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
 
@@ -67,13 +66,25 @@ async def _backfill_after_ranking() -> None:
     # OptionsFlowWithReload handles reloading automatically —
     # do NOT add an update_listener here (HA 2026.3+ forbids combining them).
 
+    # Service handlers re-resolve the coordinator from entry.runtime_data on
+    # every invocation. The entry object survives OptionsFlowWithReload, but
+    # the coordinator inside runtime_data is replaced — a captured closure
+    # reference would silently point at the dead instance after reload.
+    def _resolve_coordinator() -> PriceHawkCoordinator | None:
+        data: PriceHawkData | None = getattr(entry, "runtime_data", None)
+        return data.coordinator if data is not None else None
+
     # Register CSV analysis service
     async def handle_analyze_csv(call: object) -> None:
         """Handle the analyze_csv service call from dashboard.
 
         Accepts pre-parsed CSV rows from the dashboard JavaScript and runs
         them through the user's CONFIGURED tariff rates (not plan defaults).
         """
+        coord = _resolve_coordinator()
+        if coord is None:
+            _LOGGER.warning("analyze_csv: coordinator not available; entry unloaded?")
+            return
         rows = call.data.get("rows", [])  # type: ignore[attr-defined]
         if not rows:
             _LOGGER.error("No CSV rows provided to analyze_csv service")
@@ -90,8 +101,8 @@ async def handle_analyze_csv(call: object) -> None:
         )
 
         # Store in coordinator for dashboard access via entity attributes
-        coordinator.data["csv_comparison"] = result
-        coordinator.async_set_updated_data(coordinator.data)
+        coord.data["csv_comparison"] = result
+        coord.async_set_updated_data(coord.data)
 
         _LOGGER.info(
             "CSV analysis complete: %s saves $%.2f",
@@ -107,6 +118,10 @@ async def handle_analyze_csv(call: object) -> None:
     # coordinator method; status surfaces via
     # ``sensor.pricehawk_backfill_status``.
     async def handle_backfill(call: object) -> None:
+        coord = _resolve_coordinator()
+        if coord is None:
+            _LOGGER.warning("backfill_history: coordinator not available; entry unloaded?")
+            return
         raw_days = call.data.get("days", 30)  # type: ignore[attr-defined]
         try:
             days_back = max(1, min(int(raw_days), 90))
@@ -115,7 +130,7 @@ async def handle_backfill(call: object) -> None:
                 "backfill_history: invalid days=%r, using default 30", raw_days,
             )
             days_back = 30
-        await coordinator.async_run_backfill(days_back=days_back)
+        await coord.async_run_backfill(days_back=days_back)
 
     hass.services.async_register(DOMAIN, "backfill_history", handle_backfill)
 
@@ -125,6 +140,10 @@ async def handle_backfill(call: object) -> None:
     # switching plans (so the alternatives ranking reflects the new
     # distributor / postcode immediately).
     async def handle_rank_alternatives(call: object) -> None:
+        coord = _resolve_coordinator()
+        if coord is None:
+            _LOGGER.warning("rank_alternatives: coordinator not available; entry unloaded?")
+            return
         # CR-fix: malformed service payload (e.g. ``top_k: "abc"`` from
         # a typo in a YAML automation) would raise ValueError/TypeError
         # and fail the call. Coerce defensively + fall back to default.
@@ -137,7 +156,7 @@ async def handle_rank_alternatives(call: object) -> None:
             )
             top_k = 20
         top_k = max(1, min(top_k, 100))
-        result = await coordinator.async_run_ranking_job(top_k=top_k)
+        result = await coord.async_run_ranking_job(top_k=top_k)
         _LOGGER.info(
             "rank_alternatives service: ran successfully, %d result(s)",
             len(result),
@@ -151,23 +170,40 @@ async def handle_rank_alternatives(call: object) -> None:
     return True
 
 
-async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
-    """Unload a config entry."""
+async def async_unload_entry(hass: HomeAssistant, entry: PriceHawkConfigEntry) -> bool:
+    """Unload a config entry.
+
+    Order matters: platform-unload runs FIRST. If it fails, the coordinator
+    and runtime_data are left intact so HA can retry. Only on success do we
+    cancel timers, persist state, and (if this was the last entry) tear down
+    the singleton services.
+    """
     _LOGGER.info("Unloading PriceHawk integration (entry=%s)", entry.entry_id)
-    coordinator: PriceHawkCoordinator | None = hass.data[DOMAIN].pop(
-        entry.entry_id, None
-    )
-    if coordinator:
-        coordinator.cancel_persist()
-        coordinator.cancel_ranking()
-        await coordinator.async_persist_state()
+
+    unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS)
+    if not unload_ok:
+        return False
+
+    data: PriceHawkData | None = getattr(entry, "runtime_data", None)
+    if data is not None:
+        data.coordinator.cancel_persist()
+        data.coordinator.cancel_ranking()
+        await data.coordinator.async_persist_state()
 
     await remove_panel(hass)
 
-    # Unregister services if no more entries remain
-    if not hass.data.get(DOMAIN):
+    # Multi-entry sentinel: only unregister the singleton services when THIS
+    # is the last remaining entry. Uses the config-entries registry — NOT
+    # hass.data, which is no longer maintained. The entry being unloaded may
+    # or may not still appear in async_entries(DOMAIN) depending on HA
+    # version, so filter it out explicitly.
+    remaining = [
+        e for e in hass.config_entries.async_entries(DOMAIN)
+        if e.entry_id != entry.entry_id
+    ]
+    if not remaining:
         hass.services.async_remove(DOMAIN, "analyze_csv")
         hass.services.async_remove(DOMAIN, "backfill_history")
         hass.services.async_remove(DOMAIN, "rank_alternatives")
 
-    return await hass.config_entries.async_unload_platforms(entry, PLATFORMS)
+    return True

custom_components/pricehawk/data.py

@@ -0,0 +1,21 @@
+"""Typed runtime data for the PriceHawk integration (Phase 7 / PR-1)."""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import TYPE_CHECKING, TypeAlias
+
+from homeassistant.config_entries import ConfigEntry
+
+if TYPE_CHECKING:
+    from .coordinator import PriceHawkCoordinator
+
+
+@dataclass(slots=True)
+class PriceHawkData:
+    """Runtime data attached to a PriceHawk ConfigEntry via entry.runtime_data."""
+
+    coordinator: "PriceHawkCoordinator"
+
+
+PriceHawkConfigEntry: TypeAlias = ConfigEntry[PriceHawkData]

custom_components/pricehawk/sensor.py

@@ -20,6 +20,7 @@
 from homeassistant.util import dt as dt_util
 
 from .const import DOMAIN
+from .data import PriceHawkConfigEntry
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -804,11 +805,20 @@ def extra_state_attributes(self) -> dict[str, Any]:
 
 async def async_setup_entry(
     hass: HomeAssistant,
-    entry: ConfigEntry,
+    entry: PriceHawkConfigEntry,
     async_add_entities: AddEntitiesCallback,
 ) -> None:
     """Set up PriceHawk sensors from a config entry."""
-    coordinator = hass.data[DOMAIN][entry.entry_id]
+    # HA's platform-setup lifecycle guarantees this runs after async_setup_entry
+    # in __init__.py has populated entry.runtime_data. The assert narrows the
+    # Optional[PriceHawkData] for mypy and loud-fails any test fixture that
+    # violates the lifecycle, instead of producing an AttributeError on a
+    # downstream .coordinator access.
+    data = entry.runtime_data
+    assert data is not None, (
+        "entry.runtime_data missing — async_setup_entry in __init__.py must run first"
+    )
+    coordinator = data.coordinator
 
     entities: list[SensorEntity] = []