chore: retro-review PR #109 — fix(codex): P0-1 token-out-of-URL + P0-2 reset-all-providers + P1-6 cancel-on-un

custom_components/pricehawk/__init__.py

@@ -54,10 +54,17 @@ async def async_setup_entry(hass: HomeAssistant, entry: PriceHawkConfigEntry) ->
     # "Something is blocking Home Assistant from wrapping up the start
     # up phase" listing every other integration as collateral. Background
     # tasks are explicitly excluded from that wait.
-    hass.async_create_background_task(
+    #
+    # Codex P1-6 (2026-05-23) — retain the task handles and register
+    # cancellation on unload. Without this, reload/unload leaves these
+    # tasks running against an unloaded coordinator, which manifests
+    # as pytest "coroutine was never awaited" warnings AND as real
+    # data corruption when a reload races a still-running ranking pass.
+    ranking_task = hass.async_create_background_task(
         coordinator.async_run_ranking_job(),
         name=f"pricehawk_initial_ranking_{entry.entry_id}",
     )
+    entry.async_on_unload(ranking_task.cancel)
 
     # Phase 3.2 — kick off the universal HA-history backfill once,
     # AFTER the first ranking job finishes so the plan-set includes
@@ -73,10 +80,11 @@ async def _backfill_after_ranking() -> None:
             pass
         await coordinator.async_run_backfill(days_back=30)
 
-    hass.async_create_background_task(
+    backfill_task = hass.async_create_background_task(
         _backfill_after_ranking(),
         name=f"pricehawk_initial_backfill_{entry.entry_id}",
     )
+    entry.async_on_unload(backfill_task.cancel)
 
     # Copy www assets (icon + HTML) and register sidebar panel
     await copy_www_assets(hass)

custom_components/pricehawk/coordinator.py

@@ -1093,6 +1093,24 @@ async def _async_update_data(self) -> dict[str, Any]:
             )
             self._last_date = now_local.day
 
+            # Codex P0-2 (2026-05-23) — reset every registered provider's
+            # daily accumulators AFTER history capture but BEFORE the new
+            # day's tick. Without this, DWT, CdrPlan, FlowPower, and
+            # LocalVolts providers accumulate `today_cost` across days,
+            # corrupting Energy-Dashboard cost sensors + external
+            # statistics. Previously only `self._amber.reset_daily()` ran,
+            # and only inside the monthly-reset branch lower down.
+            for provider in self._providers.values():
+                try:
+                    provider.reset_daily()
+                except Exception:  # noqa: BLE001
+                    _LOGGER.warning(
+                        "reset_daily() raised for provider %s — daily "
+                        "counters may carry over into the next day",
+                        getattr(provider, "id", "<unknown>"),
+                        exc_info=True,
+                    )
+
             # Persist immediately after rollover to avoid data loss on crash
             await self.async_persist_state()
 

custom_components/pricehawk/dashboard_config.py

@@ -108,14 +108,26 @@ async def setup_panel_iframe(hass: HomeAssistant, entry: ConfigEntry) -> None:
     Uses async_register_built_in_panel with component_name="iframe" to create
     a sidebar entry that loads /local/pricehawk/dashboard.html.
 
-    A ``?v=<manifest-version>`` query param is appended so that browser and
+    A ``?v=<cache-buster>`` query param is appended so that browser and
     service-worker caches automatically invalidate on every HACS upgrade —
     without this, clients keep serving the previous dashboard.html.
 
-    NOTE: The HA long-lived access token is appended as a URL query parameter.
-    This is a security concern (tokens in URLs can appear in logs/referrer headers).
-    Future improvement: use a session-based auth approach instead.
+    Codex P0-1 (full-repo review 2026-05-23): the HA long-lived access
+    token is NO LONGER appended to the URL. Tokens in URLs leak via
+    browser history, referrer headers, screenshots, panel config dumps
+    and logs — redacting the log line did not stop the other paths.
+    The iframe page (``dashboard.html``) already has a four-method
+    auth-fallback chain (URL → parent frame ``hassConnection`` →
+    parent ``localStorage.hassTokens`` → local ``localStorage``); it
+    falls back cleanly to method 2/3/4 in HA's same-origin iframe
+    context, so removing method 1 from the Python side is safe.
+    The new ``setup_panel_custom_v2`` Lit panel is the long-term
+    replacement that bypasses iframes entirely — see PR #97.
+
+    ``entry`` is kept in the signature so callers don't change; the
+    parameter is currently unused.
     """
+    del entry  # No longer reads `ha_token` from the entry.
     from homeassistant.components.frontend import (
         async_register_built_in_panel,
         async_remove_panel,
@@ -135,13 +147,10 @@ async def setup_panel_iframe(hass: HomeAssistant, entry: ConfigEntry) -> None:
     # new iframe URL, defeating the 31-day max-age set by HA's /local/ static
     # handler — without it, browsers and the HA companion app can pin a stale
     # dashboard.html for weeks even after a HACS upgrade.
-    ha_token = entry.data.get("ha_token", "")
     cache_token = f"{version}.{int(time.time())}"
     dashboard_url = f"/local/pricehawk/dashboard.html?v={cache_token}"
-    if ha_token:
-        dashboard_url += f"&token={ha_token}"
 
-    # Remove existing panel first (token may have changed on re-setup)
+    # Remove existing panel first (cache-buster changes on re-setup)
     try:
         async_remove_panel(hass, PANEL_URL_PATH, warn_if_unknown=False)
         _LOGGER.debug("PriceHawk: removed existing panel before re-registering")
@@ -159,17 +168,14 @@ async def setup_panel_iframe(hass: HomeAssistant, entry: ConfigEntry) -> None:
             config={"url": dashboard_url},
             require_admin=False,
         )
-        # Phase 3.0g (CodeRabbit security): redact token from log output.
-        # The dashboard_url may contain `&token=<long-lived JWT>` and was
-        # previously written to the log in plain text — anyone with log
-        # access could lift the token and impersonate the integration.
-        log_url = dashboard_url
-        if "&token=" in log_url:
-            log_url = log_url.split("&token=")[0] + "&token=<REDACTED>"
+        # Codex P0-1: dashboard_url no longer contains a token; safe to
+        # log verbatim. Kept the log line for ops visibility on the
+        # cache-buster value so users can confirm a HACS upgrade rolled
+        # through.
         _LOGGER.info(
             "PriceHawk: sidebar panel registered at /%s -> %s",
             PANEL_URL_PATH,
-            log_url,
+            dashboard_url,
         )
     except Exception:
         _LOGGER.error(

tests/test_codex_p0_p1_fixes.py

@@ -0,0 +1,241 @@
+"""Regression tests for codex full-repo review findings (2026-05-23).
+
+Covers the subset addressed in PR #109:
+- P0-2: daily rollover MUST call ``reset_daily()`` on every registered
+  provider, not only Amber. Without this, DWT/CdrPlan/FlowPower/LocalVolts
+  providers accumulate ``today_cost`` across days, corrupting Energy
+  Dashboard cost sensors and external statistics.
+- P1-6: setup background tasks (ranking + backfill) MUST register
+  ``entry.async_on_unload(task.cancel)`` so HA cancels them cleanly on
+  reload/unload instead of leaving them to race an unloaded coordinator.
+
+Source-level + behaviour assertions, mirroring the existing test_reauth
++ test_reconfigure conventions (the EnergyCompareConfigFlow can't be
+instantiated under conftest HA stubs — see 07-02b D-1 deviation).
+"""
+
+from __future__ import annotations
+
+from pathlib import Path
+
+
+def _coordinator_source() -> str:
+    return (
+        Path(__file__).resolve().parents[1]
+        / "custom_components"
+        / "pricehawk"
+        / "coordinator.py"
+    ).read_text()
+
+
+def _init_source() -> str:
+    return (
+        Path(__file__).resolve().parents[1]
+        / "custom_components"
+        / "pricehawk"
+        / "__init__.py"
+    ).read_text()
+
+
+def _dashboard_config_source() -> str:
+    return (
+        Path(__file__).resolve().parents[1]
+        / "custom_components"
+        / "pricehawk"
+        / "dashboard_config.py"
+    ).read_text()
+
+
+# ----------------------------------------------------------------------
+# P0-2 — Daily rollover resets every provider, not just Amber
+# ----------------------------------------------------------------------
+
+
+class TestDailyRolloverResetsAllProviders:
+    """Codex P0-2: daily rollover persists yesterday's history but the
+    original implementation only reset the Amber provider's daily
+    accumulators (and only inside the monthly-reset branch lower down).
+    DWT, CdrPlan, FlowPower, and LocalVolts providers carried today's
+    counters across midnight, corrupting Energy Dashboard cost sensors
+    and external statistics.
+    """
+
+    def test_rollover_iterates_every_registered_provider(self):
+        src = _coordinator_source()
+        # Find the daily-rollover block — opens on `if now_local.day != self._last_date:`,
+        # closes on the next async def.
+        start = src.index("if now_local.day != self._last_date:")
+        end = src.index("# 5. Push current rates into providers", start)
+        block = src[start:end]
+        # The fix iterates self._providers (the registered set) and
+        # calls reset_daily on each. Both must appear in the block.
+        assert "for provider in self._providers.values():" in block, (
+            "Daily rollover must iterate every registered provider to "
+            "reset their daily accumulators."
+        )
+        assert "provider.reset_daily()" in block, (
+            "Daily rollover must call reset_daily() on each provider."
+        )
+
+    def test_rollover_guards_each_reset_with_try_except(self):
+        """A single provider raising must not skip the rest. The rollover
+        is shared infrastructure — one buggy provider can't take out the
+        whole day's reset.
+        """
+        src = _coordinator_source()
+        start = src.index("if now_local.day != self._last_date:")
+        end = src.index("# 5. Push current rates into providers", start)
+        block = src[start:end]
+        # The for-loop wraps each call in a try/except.
+        for_idx = block.index("for provider in self._providers.values():")
+        # Walk forward to find the corresponding try.
+        post_for = block[for_idx:]
+        assert "try:" in post_for[: post_for.index("provider.reset_daily()") + 50], (
+            "Each provider.reset_daily() call must be wrapped in try/"
+            "except so one buggy provider can't break the day's reset."
+        )
+        assert "reset_daily() raised for provider" in block, (
+            "On reset_daily failure the coordinator must log which "
+            "provider raised so operators can triage."
+        )
+
+    def test_rollover_reset_runs_after_history_capture(self):
+        """The history-append loop snapshots the PREVIOUS day's
+        net_daily_cost_aud per provider. Resetting BEFORE that snapshot
+        would zero history. The fix must run AFTER history capture +
+        BEFORE the new day's tick.
+        """
+        src = _coordinator_source()
+        start = src.index("if now_local.day != self._last_date:")
+        end = src.index("# 5. Push current rates into providers", start)
+        block = src[start:end]
+        history_idx = block.index("history_entry[pid]")
+        reset_idx = block.index("provider.reset_daily()")
+        assert reset_idx > history_idx, (
+            "reset_daily() must run AFTER history capture — otherwise "
+            "the recorded daily_cost_history rows are always zero."
+        )
+
+
+# ----------------------------------------------------------------------
+# P1-6 — Setup background tasks register cancellation on unload
+# ----------------------------------------------------------------------
+
+
+class TestBackgroundTaskCancellationOnUnload:
+    """Codex P1-6: PR #107 switched the initial ranking + backfill
+    schedulers to ``hass.async_create_background_task`` to fix the
+    bootstrap-blocking bug, but didn't retain the task handles. On
+    reload/unload the tasks survive against an unloaded coordinator —
+    visible as pytest "coroutine was never awaited" warnings and as
+    real data races in production.
+    """
+
+    def test_ranking_task_handle_captured_and_registered_for_cancel(self):
+        src = _init_source()
+        assert "ranking_task = hass.async_create_background_task(" in src, (
+            "The initial ranking job task must be retained in a "
+            "variable so we can hand its cancel to async_on_unload."
+        )
+        assert "entry.async_on_unload(ranking_task.cancel)" in src, (
+            "Ranking task must register ``task.cancel`` with "
+            "entry.async_on_unload so HA cancels it on reload/unload."
+        )
+
+    def test_backfill_task_handle_captured_and_registered_for_cancel(self):
+        src = _init_source()
+        assert "backfill_task = hass.async_create_background_task(" in src
+        assert "entry.async_on_unload(backfill_task.cancel)" in src, (
+            "Backfill task must register cancel-on-unload."
+        )
+
+    def test_no_bare_async_create_task_for_ranking_or_backfill(self):
+        """Regression guard against accidentally reverting the
+        bootstrap-blocking fix (PR #107) while patching task lifecycle.
+        Bare ``hass.async_create_task(coordinator.async_run_ranking_job())``
+        was the original bug — HA's bootstrap wait collected it and
+        logged "Something is blocking startup". The fix uses
+        ``async_create_background_task`` exclusively.
+        """
+        src = _init_source()
+        assert "hass.async_create_task(coordinator.async_run_ranking_job" not in src, (
+            "Ranking job must NOT be scheduled with async_create_task — "
+            "use async_create_background_task to keep it off HA's "
+            "bootstrap-wait list."
+        )
+        assert "hass.async_create_task(_backfill_after_ranking" not in src, (
+            "Backfill task must NOT use async_create_task."
+        )
+
+
+# ----------------------------------------------------------------------
+# P0-1 — Legacy iframe panel must NOT thread the HA token through URL
+# ----------------------------------------------------------------------
+
+
+class TestIframePanelNoTokenInUrl:
+    """Codex P0-1: ``setup_panel_iframe`` previously appended the HA
+    long-lived access token to the iframe URL as ``&token=<jwt>``.
+    Tokens in URLs leak via browser history, referrer headers,
+    screenshots, panel-config dumps and logs. Redacting the log line
+    did not stop the other leak paths.
+
+    The iframe page (`dashboard.html`) already has a four-method auth
+    fallback (URL → parent.hassConnection → parent.localStorage →
+    local.localStorage). Methods 2/3/4 work in HA's same-origin iframe
+    context, so removing method 1 from the Python side is safe.
+    """
+
+    def test_setup_panel_iframe_does_not_append_token_to_url(self):
+        src = _dashboard_config_source()
+        # Find the iframe-panel setup function and assert no `&token=`
+        # concatenation happens inside it.
+        start = src.index("async def setup_panel_iframe(")
+        end = src.index("\nasync def setup_panel_custom_v2(", start)
+        block = src[start:end]
+        assert '"&token="' not in block, (
+            "setup_panel_iframe must NOT append &token=... to the "
+            "dashboard URL — tokens in URLs leak via browser history, "
+            "referrer headers, and screenshots."
+        )
+        assert "'&token='" not in block
+        assert 'f"&token=' not in block
+        assert "f'&token=" not in block
+
+    def test_setup_panel_iframe_does_not_read_ha_token_from_entry(self):
+        """Belt-and-braces — even reading the token into a local var
+        risks logging or future re-introduction. Flag the actual
+        access patterns, not bare mentions of the string in comments
+        (the docstring documents the removal).
+        """
+        src = _dashboard_config_source()
+        start = src.index("async def setup_panel_iframe(")
+        end = src.index("\nasync def setup_panel_custom_v2(", start)
+        block = src[start:end]
+        for forbidden in (
+            'entry.data.get("ha_token"',
+            "entry.data.get('ha_token'",
+            'entry.data["ha_token"]',
+            "entry.data['ha_token']',",
+        ):
+            assert forbidden not in block, (
+                f"setup_panel_iframe must not read the token via {forbidden!r}"
+                " — the iframe page authenticates via HA's same-origin"
+                " session, not a URL-threaded token."
+            )
+        """Regression guard against accidentally reverting the
+        bootstrap-blocking fix (PR #107) while patching task lifecycle.
+        Bare ``hass.async_create_task(coordinator.async_run_ranking_job())``
+        was the original bug — HA's bootstrap wait collected it and
+        logged "Something is blocking startup". The fix uses
+        ``async_create_background_task`` exclusively.
+        """
+        src = _init_source()
+        assert "hass.async_create_task(coordinator.async_run_ranking_job" not in src, (
+            "Ranking job must NOT be scheduled with async_create_task — "
+            "use async_create_background_task to keep it off HA's "
+            "bootstrap-wait list."
+        )
+        assert "hass.async_create_task(_backfill_after_ranking" not in src, (
+            "Backfill task must NOT use async_create_task."
+        )