Phase 3.2 — Universal HA-history backfill#73
Conversation
11-commit execution plan covering universal HA-history backfill (3.2), period rollup sensors (3.3), named comparator drill-in (3.4), and dashboard rewrite (3.5). Locks architectural decisions up front (daily_cost_history as single source of truth, named comparator as just another CdrPlanProvider, rollup as computed-on-demand) so the executing model doesn't re-derive them at each commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds ``cdr/history_replay.py`` with three pure functions that establish
the public API surface Phase 3.2 commit 2 wires the coordinator to.
No HA imports — the module is unit-testable outside the integration
runtime, matching the ``cdr/ranking_job.py`` pattern.
Public API:
- ``states_to_half_hour_slots`` — converts raw (ts, power_w, unit)
tuples to evaluator-shaped slot dicts aligned to 30-min boundaries.
Handles kW→W unit conversion, gap-protection clamping (6 min cap
matches ``cdr/streaming.py``), and string-vs-float power values
(HA's recorder serialises some sensor states as strings).
- ``replay_day_through_plan`` — wraps ``evaluate()`` with the
standard exception-swallow pattern (mirrors ``deep_rank``).
Returns None on evaluator exception OR zero slot count.
- ``fan_out_replay`` — generator yielding (date_str, {plan_key: aud})
per day. Streaming output keeps peak RAM at ~one day × N plans
instead of all-days × N plans of full CostBreakdown objects.
25 tests covering boundary alignment, unit conversion, gap protection,
sign handling, plan-failure isolation, date-order preservation, and
opt-in entry_options pass-through. Pattern follows
``tests/test_coordinator_ranking.py``: stdlib only, no pytest-asyncio.
Foundation for Phase 3.2 commit 2 (backfill.py rewrite) and Phase 3.3
(rollup sensors read the daily_cost_history rows this module helps
populate).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… adapter
Rewrites ``backfill.py`` from a 369-LOC Amber-API-coupled module into
a thin coordination layer over the pure-logic ``cdr.history_replay``
fan-out. New public API:
backfill_daily_cost_history(hass, grid_sensor_entity, plans,
*, days_back=30, entry_options=None, existing_history=None)
Internals pull recorder history day-by-day (NOT one big query — a
30-day single ``state_changes_during_period`` on a 1Hz grid sensor
returns 100K+ State objects), convert to evaluator slots, fan out
across N plans via the streaming generator, and merge per-date rows
into the coordinator's ``daily_cost_history``. Final list is capped
at 180 entries (matches live coordinator slice).
Day-by-day queries are deliberate and NOT parallelised. HA's
recorder uses a single executor pool so concurrent queries serialise
anyway and just bloat task count. Per-query memory is bounded; the
SQLite index on ``last_changed`` means 30 small queries are not
meaningfully slower than 1 big one. This is commented inline so CR
doesn't suggest parallelisation.
Legacy ``fetch_amber_price_history`` retained — still used by
``coordinator._replay_amber_today_from_api`` to seed the Amber
accumulator on a fresh install. The Phase 3.2 backfill itself no
longer fetches Amber prices: Amber's role narrowed to a *truth
overlay* written once daily by the live coordinator rollover.
Test rewrite: 14 legacy Amber-API tests deleted (those exercised
``backfill_from_history``, ``_build_amber_price_index``,
``_find_amber_rate``, ``_parse_history_states`` — all removed).
14 new tests cover ``_local_date_string`` (AEST-safe formatting),
``_states_to_tuples`` (State + dict shapes), ``_merge_into_history``
(insert/merge/cap), and ``backfill_daily_cost_history`` end-to-end
with the recorder mocked at the import boundary.
``tests/conftest.py`` extended with ``homeassistant.components``,
``homeassistant.components.recorder``, and
``homeassistant.components.recorder.history`` mocks so the lazy
recorder import inside the backfill resolves under the test harness.
``__init__.py``'s ``handle_backfill`` service updated to call the
new API with the current CDR plan; commit 4 will shrink it further
to a one-line delegate through the coordinator (once the wrapper +
status sensor land).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ickoff
Wires the new universal HA-history backfill into the coordinator and
schedules its first run automatically after the initial ranking job
completes.
Coordinator additions:
- ``build_backfill_plan_set`` — module-level pure helper (mirrors
the ``cdr.ranking_job`` pattern) composing the {plan_key:
plan_body} dict from current plan + top-K ranked alternatives.
Lives outside the class so it's unit-testable without HA's app
context (the coordinator's ``DataUpdateCoordinator[T]`` base
gets mocked away by ``tests/conftest.py``).
- ``_build_backfill_plan_set`` — thin instance wrapper.
- ``async_run_backfill`` — coordinator-side run wrapper modelled on
``async_run_ranking_job``. Status-tracked via ``_backfill_status``
state machine (``idle | running | complete | failed``) plus
``_backfill_last_run_at``, ``_backfill_days_loaded``,
``_backfill_plans_replayed``, ``_backfill_error`` attributes
that the Phase 3.2 commit 4 status sensor will surface.
- Reuses ``_ranking_lock`` to serialise against the ranking job —
both mutate ``_daily_cost_history``. REVISIT: split if contention
observed in prod (cost of being wrong is brief serialisation of
two rare operations).
- Local import of ``backfill_daily_cost_history`` inside
``async_run_backfill`` (``# noqa: PLC0415``) so the HA recorder
isn't loaded at module-import time. Matches the existing pattern
at ``_replay_amber_today_from_api``.
``__init__.py`` kickoff:
- After ``async_run_ranking_job`` task is scheduled, schedule a
second task that AWAITS the ranking lock (so the first ranking
run finishes and the alternatives list is populated) THEN runs
``async_run_backfill(days_back=30)``. Without the wait, the
first backfill would replay history through only the current
plan, missing the ``alt_*`` columns.
Tests: 7 new in ``test_coordinator_helpers.py`` covering the pure
helper — current-plan composition, alt_* prefix keying, plan-cache
fallback to alt body, malformed-input skipping, empty/non-dict
``cdr_plan`` graceful return. Pattern matches the existing
``_extract_peak_rate_c_inc_gst`` test block in the same file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ne-liner
Final Phase 3.2 commit. Adds the status sensor users will read from
their dashboards / automations and collapses the
``pricehawk.backfill_history`` service handler to a one-line delegate
through ``coordinator.async_run_backfill``.
New entity:
- ``sensor.pricehawk_backfill_status`` — state machine read-through.
State: ``idle | running | complete | failed``. Attributes:
``last_run`` (ISO timestamp), ``days_loaded``, ``plans_replayed``,
``error``. All sourced from the coordinator's status attributes
written by ``async_run_backfill``.
Service handler:
- ``handle_backfill`` shrunk from a 60-LOC inline pipeline to
defensive ``days`` coercion + a single
``await coordinator.async_run_backfill(days_back=...)`` call.
Status tracking, recorder pulls, plan composition, and
persistence all happen inside the coordinator method now;
failures surface on the sensor rather than getting lost to log
lines.
Service description: updated ``services.yaml`` to reflect the new
flow (replay-through-CDR-plan, no Amber API, status sensor pointer).
Unused ``CONF_GRID_POWER_SENSOR`` import removed from
``__init__.py`` (the coordinator now owns the lookup).
Tests: 4 new BackfillStatusSensor smoke tests in
``test_review_improvements.py`` exercising the property-read contract
(state defaults to idle, running propagates, datetime → ISO, error
attribute surfaces on failed runs). The sensor class itself can't be
imported under the conftest mock tree (CoordinatorEntity +
SensorEntity multiple inheritance from MagicMocks triggers a
metaclass conflict), so the tests mirror the EXACT property bodies
inline. Integration test on Ryan's HA will catch any drift.
CHANGELOG.md updated under ``[Unreleased]`` documenting the full
Phase 3.2 surface area: new module, rewritten backfill, status
sensor, service signature change, recorder mocks in conftest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements Phase 3.2 “Universal HA-history backfill”: introduces a pure-logic history replay engine, rewrites the backfill module into a thin HA-recorder adapter that replays grid history through the CDR evaluator across the current plan and ranked alternatives, wires a coordinator-level async backfill runner with status tracking and auto-kickoff after the first ranking run, exposes a new backfill status sensor, and adjusts tests, services, and changelog to reflect the new multi-plan backfill flow while removing Amber-API-specific backfill logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements Phase 3.2 universal HA-history backfill: pure history-replay logic, a recorder-driven backfill adapter, coordinator orchestration with status tracking, integration wiring (auto-kickoff + delegated service), tests, and a locked Phase 3.2→3.5 plan doc. ChangesPhase 3.2 HA-history Backfill
Sequence Diagram — Backfill High-level FlowsequenceDiagram
participant User
participant Coordinator
participant Backfill as backfill_daily_cost_history
participant Recorder
participant Replay as history_replay
User->>Coordinator: trigger async_run_backfill(days_back)
Coordinator->>Backfill: call with plans and grid_sensor
Backfill->>Recorder: state_changes_during_period(window)
Recorder-->>Backfill: states
Backfill->>Replay: states_to_half_hour_slots then fan_out_replay(plans)
Replay-->>Backfill: per-day cost rows
Backfill->>Coordinator: merge results and update daily_cost_history
Coordinator->>User: persist state
Coordinator->>User: update BackfillStatusSensor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 4 other issues
Security issues:
- Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="custom_components/pricehawk/coordinator.py" line_range="134-143" />
<code_context>
+def build_backfill_plan_set(
</code_context>
<issue_to_address>
**suggestion:** The documented "no-signal" behaviour when the current plan is missing doesn’t match the implementation.
The docstring promises `{}` when current plan data is missing, but the code still returns alternative plans in that case. That’s visible both via `build_backfill_plan_set` and `async_run_backfill` (which can replay alternatives only). Please either:
- Change the implementation to return `{}` when `current_data` is missing, or
- Update the docstring to state that alternatives may be returned even without a current plan, and document how callers should interpret that.
Clarifying this contract now will prevent confusing behaviour for future callers and debuggers.
Suggested implementation:
```python
def build_backfill_plan_set(
*,
options: dict[str, Any],
current_plan_id: str,
ranked_alternatives: list[dict[str, Any]],
plan_cache: dict[str, dict[str, Any]],
) -> dict[str, dict[str, Any]]:
"""Pure-logic helper composing ``{plan_key: plan_body}`` for the
Phase 3.2 backfill replay. Lives outside ``PriceHawkCoordinator``
so it's unit-testable without HA's app context (the coordinator's
``DataUpdateCoordinator[T]`` base gets mocked away by
When ``current_plan_id`` cannot be resolved in ``plan_cache``, the
returned mapping will not contain an entry for the "current" plan.
In that case this helper may still return entries for replayable
alternative plans derived from ``ranked_alternatives``. Callers
should treat the absence of a current-plan entry as a "no-signal"
condition for the active plan at that time, and interpret any
returned alternatives strictly as replayable options rather than
evidence that a specific plan was active.
```
There is likely an existing sentence later in the `build_backfill_plan_set` docstring explicitly promising `{}` when current plan data is missing. That sentence should be removed or updated to align with the new clarified behaviour described above (i.e. no current-plan entry, but alternatives may still be present). If `async_run_backfill` has a docstring that describes the same "no-signal" behaviour, it should be updated similarly to mention that alternatives can still be returned even when the current plan is unknown, and how callers should interpret that.
</issue_to_address>
### Comment 2
<location path="tests/test_backfill.py" line_range="63-72" />
<code_context>
+def _patch_recorder(states_by_day: dict[datetime, list]) -> tuple:
</code_context>
<issue_to_address>
**nitpick:** Align `_patch_recorder` docstring and return type with the actual return value
The docstring claims this returns `(get_instance_mock, history_call_mock, dt_util_now_mock)`, but it actually returns only `(get_instance, history_mock)`, and the type is the opaque `tuple`. Please either adjust the implementation to return the documented 3-tuple, or update the docstring and return type to match the actual 2-tuple (e.g. `tuple[MagicMock, MagicMock]`) so callers know exactly what to expect.
</issue_to_address>
### Comment 3
<location path=".planning/PHASE-3.2-to-3.5-PLAN.md" line_range="304" />
<code_context>
+**What it does:**
+- Adds `async_run_backfill()` method to coordinator (mirrors
+ `async_run_ranking_job` pattern at `coordinator.py:1232-1282`).
+- Reuses the existing `_ranking_lock` REUSE (REVISIT below) to serialise
+ backfill with ranking — both write to `_daily_cost_history` so we can't
+ let them race.
</code_context>
<issue_to_address>
**nitpick (typo):** Remove the duplicated word in the `_ranking_lock` sentence.
The phrase "Reuses the existing `_ranking_lock` REUSE (REVISIT below)" has a duplicated "REUSE". Dropping the second "REUSE" will read more cleanly.
```suggestion
- Reuses the existing `_ranking_lock` (REVISIT below) to serialise
```
</issue_to_address>
### Comment 4
<location path=".planning/PHASE-3.2-to-3.5-PLAN.md" line_range="393-394" />
<code_context>
+hass.async_create_task(_backfill_after_ranking())
+```
+
+**REVISIT — days_back default:** 30 days. HA recorder default is 10 days,
+so most users get only the last 10. Power users with `purge_keep_days: 365`
+get the full 30 we ask for. Pulling 365 by default would be slower and
</code_context>
<issue_to_address>
**nitpick (typo):** Correct the spelling of “over-ride” to “override”.
In that sentence, “override” should be a single word without a hyphen to match standard technical usage.
```suggestion
mostly empty for new users. The `backfill_history` service already accepts
a `days` parameter so users can override.
```
</issue_to_address>
### Comment 5
<location path=".planning/PHASE-3.2-to-3.5-PLAN.md" line_range="1196" />
<code_context>
- Will flag hardcoded ws:// in dashboard.html.
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _patch_recorder(states_by_day: dict[datetime, list]) -> tuple: | ||
| """Build mocks for HA recorder + dt_util used inside backfill. | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0]["date"] == "2026-04-10" | ||
| Returns a tuple of ``(get_instance_mock, history_call_mock, | ||
| dt_util_now_mock)`` so tests can assert on calls. | ||
|
|
||
| # Amber: ~48 kWh * 25 c/kWh + 60c daily = ~1260c = ~$12.60 | ||
| # (Gap protection clamps to 6 min max, 30s intervals fit fine) | ||
| assert result[0]["amber"] > 0 | ||
| states_by_day: keys are AEST midnight datetimes (the local day); | ||
| values are the State objects to return when the recorder is | ||
| queried for that day's window. | ||
| """ |
There was a problem hiding this comment.
nitpick: Align _patch_recorder docstring and return type with the actual return value
The docstring claims this returns (get_instance_mock, history_call_mock, dt_util_now_mock), but it actually returns only (get_instance, history_mock), and the type is the opaque tuple. Please either adjust the implementation to return the documented 3-tuple, or update the docstring and return type to match the actual 2-tuple (e.g. tuple[MagicMock, MagicMock]) so callers know exactly what to expect.
| **What it does:** | ||
| - Adds `async_run_backfill()` method to coordinator (mirrors | ||
| `async_run_ranking_job` pattern at `coordinator.py:1232-1282`). | ||
| - Reuses the existing `_ranking_lock` REUSE (REVISIT below) to serialise |
There was a problem hiding this comment.
nitpick (typo): Remove the duplicated word in the _ranking_lock sentence.
The phrase "Reuses the existing _ranking_lock REUSE (REVISIT below)" has a duplicated "REUSE". Dropping the second "REUSE" will read more cleanly.
| - Reuses the existing `_ranking_lock` REUSE (REVISIT below) to serialise | |
| - Reuses the existing `_ranking_lock` (REVISIT below) to serialise |
| mostly empty for new users. The `backfill_history` service already accepts | ||
| a `days` parameter so users can over-ride. |
There was a problem hiding this comment.
nitpick (typo): Correct the spelling of “over-ride” to “override”.
In that sentence, “override” should be a single word without a hyphen to match standard technical usage.
| mostly empty for new users. The `backfill_history` service already accepts | |
| a `days` parameter so users can over-ride. | |
| mostly empty for new users. The `backfill_history` service already accepts | |
| a `days` parameter so users can override. |
`target = by_date.get(date_str)` infers as `dict[str, Any] | None` but the subsequent `target[plan_key] = aud` assignment narrows incorrectly without an explicit annotation. Annotate the union so pyright accepts both branches (None → fresh dict, dict → mutate). No behaviour change; pytest 760/760 still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai reviews remaining? |
|
Review rate limit: 3/5 reviews remaining, refill in 18 minutes and 31 seconds. |
…mock
Two docstring corrections flagged by Sourcery — no behaviour changes.
1. `build_backfill_plan_set` previously claimed it returned `{}` when
the current plan was missing. The implementation actually returns
alternatives even without a current plan (alts-only backfill is
intentional so rollup sensors can still surface comparative data).
Updated the docstring to describe the real contract: callers must
treat the absence of the current-plan entry as a "no-signal"
condition for the active plan at that time.
2. `_patch_recorder` claimed a 3-tuple return
`(get_instance_mock, history_call_mock, dt_util_now_mock)` but the
helper actually returns a 2-tuple `(get_instance, history_mock)`
(the dt_util mock was removed earlier when the backfill stopped
depending on it). Fixed the docstring and added a precise
`tuple[MagicMock, MagicMock]` return annotation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 95-96: The plan currently uses two different keys for backfill
errors (`error_message` at the top and `error` later) which can cause drift;
standardize on the canonical key `error` everywhere for the backfill status
contract and update the coordinator/sensor/tests to match. Specifically, in the
`_backfill_status` state description and any referenced attributes ensure the
attribute list is `last_run`, `days_loaded`, `plans_replayed`, and `error`
(replace any `error_message` occurrences), and verify any test fixtures or
sensor/coordinator code that reads/writes `_backfill_status` use the `error`
key.
- Around line 1196-1198: The line embeds a literal insecure "ws://" token
causing CI security failures; update the wording in the document (the
dashboard.html note) to avoid the literal insecure scheme and instead describe
protocol-safe WebSocket URL construction using location.protocol (i.e., derive
the ws/wss scheme from location.protocol) and mention token passing via URL
param + postMessage; keep the original intent but remove the explicit "ws://"
string so no insecure scheme appears verbatim.
In `@CHANGELOG.md`:
- Around line 30-33: Update the CHANGELOG entry for build_backfill_plan_set to
reflect its actual API shape and ownership: replace "module-level pure helper"
with wording that it is a coordinator-owned backfill orchestration helper (e.g.,
"coordinator-owned backfill orchestration helper") and ensure the description
references the coordinator namespace `coordinator.build_backfill_plan_set` and
its contract with Phase 3.2; keep the rest of the text about composing
`{plan_key: plan_body}`, alt key prefix `alt_<planId>`, and Phase 3.3 filter
note unchanged.
- Around line 7-60: The changelog currently lists the "Phase 3.2 — Universal
HA-history backfill" entry only under [Unreleased]; per repo rules add a new
versioned section (e.g., "## [vX.Y.Z] - YYYY-MM-DD") and move or duplicate the
Phase 3.2 content into that dated section in CHANGELOG.md (keeping or leaving an
empty [Unreleased] for future work), ensuring the new heading follows Keep a
Changelog format and includes the same Added/Changed/Removed bullets referencing
the module names like cdr/history_replay.py, backfill.py,
coordinator.async_run_backfill, coordinator.build_backfill_plan_set,
sensor.pricehawk_backfill_status, and tests/conftest.py.
In `@custom_components/pricehawk/coordinator.py`:
- Around line 1418-1421: The backfill sensor incorrectly sets
_backfill_days_loaded to len(result) which is the merged total history; instead
capture the previous history length (e.g. prev_len =
len(self._daily_cost_history) before updating), compute new_days = max(0,
len(result) - prev_len), and assign _backfill_days_loaded = new_days; then
update _daily_cost_history = result and keep updating _backfill_status and
_backfill_last_run_at as before. Ensure you reference the variables result,
_daily_cost_history, and _backfill_days_loaded when making the change and clamp
negatives to zero.
In `@tests/test_review_improvements.py`:
- Around line 171-235: The test methods in TestBackfillStatusSensor are missing
return type hints; add "-> None" to each public test method
(test_native_value_defaults_to_idle, test_native_value_reflects_running_state,
test_extra_state_attributes_serialises_last_run_iso,
test_extra_state_attributes_surfaces_error_on_failed) so they declare return
types per repository standards; leave the underscored helper methods (_coord,
_native_value, _attrs) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a835e47b-06ac-475b-b30f-3140305f3279
📒 Files selected for processing (13)
.planning/PHASE-3.2-to-3.5-PLAN.mdCHANGELOG.mdcustom_components/pricehawk/__init__.pycustom_components/pricehawk/backfill.pycustom_components/pricehawk/cdr/history_replay.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/sensor.pycustom_components/pricehawk/services.yamltests/conftest.pytests/test_backfill.pytests/test_coordinator_helpers.pytests/test_history_replay.pytests/test_review_improvements.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Verify: no broken links, code examples match actual implementation, version numbers are current, no TODO left unfixed.
Files:
CHANGELOG.md
**/CHANGELOG.md
⚙️ CodeRabbit configuration file
**/CHANGELOG.md: Entries MUST follow Keep a Changelog format. New version section MUST be present for this PR's changes.
Files:
CHANGELOG.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useasync/awaitfor all I/O operations
NEVER hardcode tokens, API keys, or credentials in any file — use HA config entry storage
State restore MUST validate storage version before loading
from_dict() methods MUST receive an explicit HA-timezone date — no date.today() fallback
Files:
tests/conftest.pycustom_components/pricehawk/__init__.pytests/test_coordinator_helpers.pycustom_components/pricehawk/sensor.pytests/test_backfill.pytests/test_history_replay.pytests/test_review_improvements.pycustom_components/pricehawk/cdr/history_replay.pycustom_components/pricehawk/backfill.pycustom_components/pricehawk/coordinator.py
⚙️ CodeRabbit configuration file
**/*.py: Check for: type hints on all public functions, no bareexcept:, SQL injection risks, missing input sanitisation, secrets not in code, Flask Blueprint structure respected, APScheduler job error handling.
Files:
tests/conftest.pycustom_components/pricehawk/__init__.pytests/test_coordinator_helpers.pycustom_components/pricehawk/sensor.pytests/test_backfill.pytests/test_history_replay.pytests/test_review_improvements.pycustom_components/pricehawk/cdr/history_replay.pycustom_components/pricehawk/backfill.pycustom_components/pricehawk/coordinator.py
**/test*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tariff rate calculation changes require edge case tests (negative rates, midnight boundaries, empty windows)
Files:
tests/test_coordinator_helpers.pytests/test_backfill.pytests/test_history_replay.pytests/test_review_improvements.py
🪛 GitHub Check: Sourcery review
.planning/PHASE-3.2-to-3.5-PLAN.md
[error] 1196-1196: Sourcery: Blocking security issue detected at .planning/PHASE-3.2-to-3.5-PLAN.md:1196 (see referenced PR discussion).
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1110-~1110: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1222-~1222: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 994-994: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1118-1118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1123-1123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1132-1132: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1137-1137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1138-1138: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1145-1145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1157-1157: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1157-1157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1162-1162: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1178-1178: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1185-1185: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1190-1190: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1195-1195: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1200-1200: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1217-1217: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (3)
tests/conftest.py (1)
29-36: LGTM!Also applies to: 49-52
custom_components/pricehawk/services.yaml (1)
19-23: LGTM!tests/test_review_improvements.py (1)
160-164: LGTM!
| ## [Unreleased] | ||
|
|
||
| ### Phase 3.2 — Universal HA-history backfill | ||
|
|
||
| Replaces the Amber-API-only backfill with a multi-plan replay over the | ||
| HA recorder. Reads N days of grid-power state changes, converts them to | ||
| half-hour evaluator slots, replays each day through the user's current | ||
| CDR plan + top-K ranked alternatives, and writes per-day cost rows into | ||
| `daily_cost_history` for the rollup sensors (Phase 3.3) and dashboard | ||
| (Phase 3.5) to consume. | ||
|
|
||
| #### Added | ||
|
|
||
| - **`cdr/history_replay.py`** — pure-logic fan-out (`states_to_half_hour_slots`, | ||
| `replay_day_through_plan`, `fan_out_replay` generator). No HA imports; | ||
| unit-testable in isolation (~25 tests). | ||
| - **`backfill.py` (rewrite)** — thin HA-side adapter pulling recorder | ||
| history day-by-day (NOT one big query), delegating to `fan_out_replay`, | ||
| merging into `daily_cost_history` (cap 180 entries). | ||
| - **`coordinator.async_run_backfill`** — status-tracked entry point | ||
| (`_backfill_status` machine: `idle | running | complete | failed`) | ||
| reusing the ranking lock for serialisation against the daily ranking | ||
| job. | ||
| - **`coordinator.build_backfill_plan_set`** — module-level pure helper | ||
| composing `{plan_key: plan_body}` from current plan + top-K | ||
| alternatives + ranking plan cache. Keys ranked alts as | ||
| `alt_<planId>` so Phase 3.3 rollup sensors can filter on the prefix. | ||
| - **Auto-kickoff** — `async_setup_entry` schedules one backfill after | ||
| the first ranking job releases the lock (so the alternatives list is | ||
| populated when the first replay runs). | ||
| - **`sensor.pricehawk_backfill_status`** — state machine read-through. | ||
| Attributes: `last_run` (ISO), `days_loaded`, `plans_replayed`, `error`. | ||
| - **`tests/conftest.py`** — `homeassistant.components.recorder` + | ||
| `.history` mocks so the backfill module's lazy recorder import | ||
| resolves under the test harness. | ||
|
|
||
| #### Changed | ||
|
|
||
| - **`pricehawk.backfill_history` service** shrunk to a one-line delegate | ||
| through `coordinator.async_run_backfill(days_back=...)`. Status now | ||
| surfaces on the new sensor instead of being lost to log lines. | ||
| - **`services.yaml`** description updated — Amber API removed, | ||
| replay-through-CDR-plan flow documented. | ||
|
|
||
| #### Removed | ||
|
|
||
| - `backfill.backfill_from_history` (Amber-API-coupled), along with | ||
| `_build_amber_price_index`, `_find_amber_rate`, `_parse_history_states`, | ||
| `_format_date`. Amber's role narrowed to a *truth overlay* written | ||
| once daily by the live coordinator — the multi-plan backfill replays | ||
| the user's CDR plan(s) through the evaluator instead. | ||
| - 14 legacy `tests/test_backfill.py` tests (covered the deleted Amber | ||
| helpers); replaced by 14 new tests for the rewritten module. | ||
|
|
There was a problem hiding this comment.
Add an explicit versioned section for these PR changes, not only [Unreleased].
Per repository changelog rules, this PR’s changes need a new version heading (date-stamped) in addition to or instead of [Unreleased].
As per coding guidelines, **/CHANGELOG.md: Entries MUST follow Keep a Changelog format. New version section MUST be present for this PR's changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 7 - 60, The changelog currently lists the "Phase
3.2 — Universal HA-history backfill" entry only under [Unreleased]; per repo
rules add a new versioned section (e.g., "## [vX.Y.Z] - YYYY-MM-DD") and move or
duplicate the Phase 3.2 content into that dated section in CHANGELOG.md (keeping
or leaving an empty [Unreleased] for future work), ensuring the new heading
follows Keep a Changelog format and includes the same Added/Changed/Removed
bullets referencing the module names like cdr/history_replay.py, backfill.py,
coordinator.async_run_backfill, coordinator.build_backfill_plan_set,
sensor.pricehawk_backfill_status, and tests/conftest.py.
| - **`coordinator.build_backfill_plan_set`** — module-level pure helper | ||
| composing `{plan_key: plan_body}` from current plan + top-K | ||
| alternatives + ranking plan cache. Keys ranked alts as | ||
| `alt_<planId>` so Phase 3.3 rollup sensors can filter on the prefix. |
There was a problem hiding this comment.
Correct API-shape wording for build_backfill_plan_set.
This entry says “module-level pure helper,” but the Phase 3.2 contract describes coordinator-owned backfill orchestration. Update wording so the changelog reflects the actual API location/shape.
As per coding guidelines, **/*.md: Verify code examples match actual implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 30 - 33, Update the CHANGELOG entry for
build_backfill_plan_set to reflect its actual API shape and ownership: replace
"module-level pure helper" with wording that it is a coordinator-owned backfill
orchestration helper (e.g., "coordinator-owned backfill orchestration helper")
and ensure the description references the coordinator namespace
`coordinator.build_backfill_plan_set` and its contract with Phase 3.2; keep the
rest of the text about composing `{plan_key: plan_body}`, alt key prefix
`alt_<planId>`, and Phase 3.3 filter note unchanged.
| hass.async_create_task(coordinator.async_run_ranking_job()) | ||
|
|
||
| # Phase 3.2 — kick off the universal HA-history backfill once, | ||
| # AFTER the first ranking job finishes so the plan-set includes | ||
| # the top-K alternatives (otherwise the first backfill would only | ||
| # carry the current plan's column). Reuses ``_ranking_lock`` so | ||
| # we never race the ranking job that's mutating | ||
| # ``_daily_cost_history`` from the daily rollover path. | ||
| async def _backfill_after_ranking() -> None: | ||
| # Wait for the first ranking run to release the lock — at that | ||
| # point the alternatives list is populated and the plan cache | ||
| # has the full bodies needed for the evaluator replay. | ||
| async with coordinator._ranking_lock: | ||
| pass | ||
| await coordinator.async_run_backfill(days_back=30) | ||
|
|
||
| hass.async_create_task(_backfill_after_ranking()) |
There was a problem hiding this comment.
Auto-backfill can run before the first ranking completes.
The current scheduling does not guarantee ordering: _backfill_after_ranking() may execute before async_run_ranking_job() acquires _ranking_lock, so backfill can run with no ranked alternatives.
Suggested fix
- coordinator.schedule_daily_ranking()
- hass.async_create_task(coordinator.async_run_ranking_job())
+ coordinator.schedule_daily_ranking()
+ first_ranking_task = hass.async_create_task(coordinator.async_run_ranking_job())
@@
async def _backfill_after_ranking() -> None:
- # Wait for the first ranking run to release the lock — at that
- # point the alternatives list is populated and the plan cache
- # has the full bodies needed for the evaluator replay.
- async with coordinator._ranking_lock:
- pass
+ await first_ranking_task
await coordinator.async_run_backfill(days_back=30)
CodeRabbit round-2 fixes for PR #73: - coordinator.async_run_backfill: ``_backfill_days_loaded`` now reports the delta (new days added by THIS run), not the total merged-history length. Capture ``prev_len`` before reassignment, compute ``new_days = max(0, len(result) - prev_len)``, clamp negatives to zero. Return value and log line updated to match. Docstring spells out the delta semantics so future callers don't misread the API. - tests/test_review_improvements.py::TestBackfillStatusSensor: add ``-> None`` return type hints to the four public test methods so mypy strict-mode is satisfied. Underscored helpers (_coord, _native_value, _attrs) left as-is. - .planning/PHASE-3.2-to-3.5-PLAN.md: section 1.5 was documenting ``error_message`` but the implementation uses ``error`` (matches sensor.py:571 + tests). Sync the plan to the code. Also defang the literal insecure-WebSocket scheme in section 8.4 (``ws-//`` with a parenthetical note) so the dashboard-protocol-safety recipe and generic CI security scans don't flag the plan doc itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.planning/PHASE-3.2-to-3.5-PLAN.md (1)
1086-1090:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the remaining literal insecure WebSocket scheme token from the doc.
Line 1088 still includes a literal insecure WebSocket scheme string, which can retrigger the same security scan failure this plan already addressed elsewhere.
Suggested wording update
- currently allows `ws://localhost:* wss://localhost:* wss://*.ui.nabu.casa`. + currently allows insecure and secure WebSocket localhost schemes plus + secure Nabu Casa WebSocket hosts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/PHASE-3.2-to-3.5-PLAN.md around lines 1086 - 1090, Remove the remaining insecure literal WebSocket scheme token from the "Iframe sandbox CSP" connect-src directive: locate the "Iframe sandbox CSP" block referenced as dashboard.html:6 and remove the token "ws://localhost:*" from the connect-src list; ensure connect-src still includes secure wss entries (for example keep "wss://localhost:*" and add "wss://*.local" or "wss://*.ui.nabu.casa" as needed) so only secure wss schemes remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/pricehawk/coordinator.py`:
- Around line 1393-1397: Currently the code acquires self._ranking_lock before
checking self._backfill_status, which lets a second caller block on the lock and
then proceed incorrectly; change to a double-checked pattern: first check if
self._backfill_status == "running" and return 0 immediately, then async with
self._ranking_lock re-check self._backfill_status to guard against races and
only then set self._backfill_status = "running" and self._backfill_error = None;
this uses the same symbols (_backfill_status, _backfill_error, _ranking_lock)
but reorders checks to short-circuit callers without unnecessary blocking.
---
Duplicate comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 1086-1090: Remove the remaining insecure literal WebSocket scheme
token from the "Iframe sandbox CSP" connect-src directive: locate the "Iframe
sandbox CSP" block referenced as dashboard.html:6 and remove the token
"ws://localhost:*" from the connect-src list; ensure connect-src still includes
secure wss entries (for example keep "wss://localhost:*" and add "wss://*.local"
or "wss://*.ui.nabu.casa" as needed) so only secure wss schemes remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f53fce7-1692-4b65-9be7-02c8fa986ac5
📒 Files selected for processing (3)
.planning/PHASE-3.2-to-3.5-PLAN.mdcustom_components/pricehawk/coordinator.pytests/test_review_improvements.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useasync/awaitfor all I/O operations
NEVER hardcode tokens, API keys, or credentials in any file — use HA config entry storage
State restore MUST validate storage version before loading
from_dict() methods MUST receive an explicit HA-timezone date — no date.today() fallback
Files:
tests/test_review_improvements.pycustom_components/pricehawk/coordinator.py
⚙️ CodeRabbit configuration file
**/*.py: Check for: type hints on all public functions, no bareexcept:, SQL injection risks, missing input sanitisation, secrets not in code, Flask Blueprint structure respected, APScheduler job error handling.
Files:
tests/test_review_improvements.pycustom_components/pricehawk/coordinator.py
**/test*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tariff rate calculation changes require edge case tests (negative rates, midnight boundaries, empty windows)
Files:
tests/test_review_improvements.py
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1110-~1110: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1224-~1224: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 994-994: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1118-1118: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1123-1123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1132-1132: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1137-1137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1138-1138: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1145-1145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1157-1157: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1157-1157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1162-1162: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1178-1178: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1185-1185: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1190-1190: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1195-1195: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1202-1202: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1219-1219: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (2)
tests/test_review_improvements.py (1)
160-164: LGTM!Also applies to: 171-235
custom_components/pricehawk/coordinator.py (1)
134-189: LGTM!Also applies to: 335-350
CodeRabbit round-3: - Replace literal ws://localhost:* token in PHASE-3.2-to-3.5 plan doc with descriptive prose so secret/security scanners stop tripping. - Short-circuit async_run_backfill on _backfill_status BEFORE acquiring _ranking_lock so a concurrent caller does not block. Keep the re-check inside the lock to close the read/acquire race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Line 1254: Replace the hardcoded developer-local absolute path
'/Users/ryanfoyle/Claude/energy/pricehawk/' with a repository-relative path or
placeholder (e.g., project-root relative path, ./, or a REPO_ROOT variable) so
documentation and scripts reference the repo location rather than a
machine-specific path; update the line that currently reads "All absolute paths
under '/Users/ryanfoyle/Claude/energy/pricehawk/'" to use the chosen
repo-relative form and, if relevant, add a short note explaining the expected
base (repo root or env var).
- Line 1284: The total commits line "**Total estimated commits:** 11 (3.2: 4 +
3.3: 3 + 3.4: 2 + 3.5: 3 = within roadmap's 10-14 range)" is inconsistent with
its breakdown; update that line so the total matches the sum (either change the
total to 12 or adjust one of the phase counts), e.g., replace "11" with "12" to
reflect 4+3+2+3=12 and keep the explanatory parenthetical intact.
In `@custom_components/pricehawk/coordinator.py`:
- Around line 1402-1413: The backfill failure path must clear stale run metadata
so sensors don't show previous success values; in the except/failure handling
block inside the backfill routine (the method that sets self._backfill_status =
"running" and calls self._build_backfill_plan_set()) ensure you explicitly
reset/clear run-specific attributes — e.g. set self._backfill_plans_replayed =
0, self._backfill_days_loaded = 0 (or recompute as prev_len delta = 0), and
clear self._backfill_last_run (or set to None) and set self._backfill_error with
the exception before marking status "failed" — do this both in the main except
and any other early-failure branches (the area referenced around lines that set
self._backfill_plans_replayed and later at 1429-1433) so stale values are never
left after a failed run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed6deaff-43bd-4ded-a6db-dc2eaad3c621
📒 Files selected for processing (2)
.planning/PHASE-3.2-to-3.5-PLAN.mdcustom_components/pricehawk/coordinator.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useasync/awaitfor all I/O operations
NEVER hardcode tokens, API keys, or credentials in any file — use HA config entry storage
State restore MUST validate storage version before loading
from_dict() methods MUST receive an explicit HA-timezone date — no date.today() fallback
Files:
custom_components/pricehawk/coordinator.py
⚙️ CodeRabbit configuration file
**/*.py: Check for: type hints on all public functions, no bareexcept:, SQL injection risks, missing input sanitisation, secrets not in code, Flask Blueprint structure respected, APScheduler job error handling.
Files:
custom_components/pricehawk/coordinator.py
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1111-~1111: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1225-~1225: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 994-994: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1119-1119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1125-1125: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1133-1133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1138-1138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1139-1139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1146-1146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1158-1158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1158-1158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1163-1163: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1179-1179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1186-1186: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1191-1191: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1196-1196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1203-1203: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1220-1220: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (1)
custom_components/pricehawk/coordinator.py (1)
134-189: LGTM!
…tadata reset CodeRabbit round-4 fixes on PR #73: - Plan doc: replace literal absolute `/Users/.../pricehawk/` path with `<REPO_ROOT>` placeholder so the planning markdown isn't tied to one user's machine layout. - Plan doc: fix arithmetic typo in commit total. 4+3+2+3 = 12, not 11. - Coordinator `async_run_backfill`: reset stale success metadata (`_backfill_days_loaded`, `_backfill_plans_replayed`) in the failure path so the status sensor doesn't surface misleading counts from a prior successful run after a failure. Set `_backfill_last_run_at` to the time of THIS (failed) run, matching success-path semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 925-929: The registration logic uses
coordinator.data.get("providers") but providers are stored on the coordinator
instance (self._providers), so change the check that sets named_present to
inspect the coordinator-managed providers collection instead of
coordinator.data; for example, read from coordinator._providers or
coordinator.providers (whichever public attribute exists) when testing for the
"named" provider, and then proceed to append NamedComparatorRollupSensor
instances as before; update the reference to coordinator.data.get("providers")
wherever named_present is computed so the NamedComparatorRollupSensor
registration actually occurs.
- Around line 205-206: The test description for test_kw_unit_multiplied_by_1000
is inconsistent: it currently states that unit="kW" "doubles" import_kwh but the
implementation/definition multiplies by 1000; update the wording in the test
description to accurately say that unit="kW" multiplies (or scales) values by
1000 (e.g., "multiplies import_kwh by 1000" or "converts kW to W by *1000") so
the test name and comment match the actual conversion; ensure only the
human-readable text around test_kw_unit_multiplied_by_1000 is changed (no logic
changes).
- Around line 15-23: The fenced code blocks containing the ASCII phase diagram
(the block starting with "Phase 3.2 (history_replay + backfill rewrite + status
sensor)" and the other blocks at the ranges called out) are missing language
identifiers; add an appropriate language tag (e.g., ```text or ```bash) to each
opening fence so Markdown lint rule MD040 is satisfied—locate the ASCII diagram
block and the other blocks at the noted ranges (994-1031, 1158-1163) and change
``` to ```text (or another suitable tag).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89f4f60b-872c-4ce6-a87b-4f7728bcd05f
📒 Files selected for processing (2)
.planning/PHASE-3.2-to-3.5-PLAN.mdcustom_components/pricehawk/coordinator.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useasync/awaitfor all I/O operations
NEVER hardcode tokens, API keys, or credentials in any file — use HA config entry storage
State restore MUST validate storage version before loading
from_dict() methods MUST receive an explicit HA-timezone date — no date.today() fallback
Files:
custom_components/pricehawk/coordinator.py
⚙️ CodeRabbit configuration file
**/*.py: Check for: type hints on all public functions, no bareexcept:, SQL injection risks, missing input sanitisation, secrets not in code, Flask Blueprint structure respected, APScheduler job error handling.
Files:
custom_components/pricehawk/coordinator.py
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1111-~1111: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1225-~1225: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 994-994: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1119-1119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1125-1125: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1133-1133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1138-1138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1139-1139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1146-1146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1158-1158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1158-1158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1163-1163: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1179-1179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1186-1186: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1191-1191: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1196-1196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1203-1203: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1220-1220: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (4)
custom_components/pricehawk/coordinator.py (4)
134-189: LGTM!
335-350: LGTM!
1358-1372: LGTM!
1374-1456: LGTM!
…der lookup Round-5 CR doc nits on the planning artifact: - Add `text`/`bash` language tags to 3 fenced code blocks (dependency graph, dashboard ASCII layout, pre-push checks) for MD040 compliance. - Fix test description: kW→W is multiply-by-1000, not "doubles". - Plan snippet's named-provider check should read `coordinator._providers` (the instance dict) not `coordinator.data["providers"]`. Matches the shipped code in Phase 3.4's sensor.py registration. Plan doc only; no shipped code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 411-412: The backfill status contract is inconsistent: the
attributes list includes "plans_replayed" but the sensor/example implementation
(around lines 434-441) does not return it; update the contract so both places
match by either removing "plans_replayed" from the Attributes set or adding
"plans_replayed" to the example return value. Locate the attributes definition
and the example sensor implementation, then choose and apply one consistent
change across both the Attributes = `{"last_run", "days_loaded",
"plans_replayed", "error"}` declaration and the backfill_history service/example
(referenced as backfill_history in __init__.py:92-186 and the example block at
434-441) so the coordinator, sensor, and tests use the same contract.
- Around line 381-387: The current kickoff in _backfill_after_ranking uses
coordinator._ranking_lock.acquire()/release() which only checks lock
availability at one instant and can still race with the first ranking run;
replace this probe with an explicit completion signal: add an asyncio.Event (or
a boolean flag) on the coordinator (e.g., coordinator.ranking_completed_event)
that async_run_ranking_job sets when it finishes, then have
_backfill_after_ranking await that event (or poll the flag) before calling
coordinator.async_run_backfill(days_back=30); update any code creating the task
(hass.async_create_task(_backfill_after_ranking())) to use the new wait
mechanism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0329d2b-790f-4284-b759-ab59a43d618a
📒 Files selected for processing (1)
.planning/PHASE-3.2-to-3.5-PLAN.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1111-~1111: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1225-~1225: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1119-1119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1125-1125: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1133-1133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1138-1138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1139-1139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1146-1146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1158-1158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1163-1163: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1179-1179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1186-1186: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1191-1191: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1196-1196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1203-1203: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1220-1220: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
| async def _backfill_after_ranking() -> None: | ||
| # Wait for first ranking run to finish so plan set includes alts | ||
| await coordinator._ranking_lock.acquire() | ||
| coordinator._ranking_lock.release() | ||
| await coordinator.async_run_backfill(days_back=30) | ||
|
|
||
| hass.async_create_task(_backfill_after_ranking()) |
There was a problem hiding this comment.
Backfill kickoff wait logic is race-prone.
Line 383–385 does not guarantee “ranking completed”; it only waits for lock availability at one instant. _backfill_after_ranking can still run before the first ranking job populates alternatives.
Use an explicit completion signal (asyncio.Event or a coordinator flag set at the end of async_run_ranking_job) instead of lock probing.
Suggested doc fix
async def _backfill_after_ranking() -> None:
- # Wait for first ranking run to finish so plan set includes alts
- await coordinator._ranking_lock.acquire()
- coordinator._ranking_lock.release()
+ # Wait for explicit first-ranking completion signal
+ await coordinator._first_ranking_done.wait()
await coordinator.async_run_backfill(days_back=30)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/PHASE-3.2-to-3.5-PLAN.md around lines 381 - 387, The current
kickoff in _backfill_after_ranking uses
coordinator._ranking_lock.acquire()/release() which only checks lock
availability at one instant and can still race with the first ranking run;
replace this probe with an explicit completion signal: add an asyncio.Event (or
a boolean flag) on the coordinator (e.g., coordinator.ranking_completed_event)
that async_run_ranking_job sets when it finishes, then have
_backfill_after_ranking await that event (or poll the flag) before calling
coordinator.async_run_backfill(days_back=30); update any code creating the task
(hass.async_create_task(_backfill_after_ranking())) to use the new wait
mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.planning/PHASE-3.2-to-3.5-PLAN.md (1)
381-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackfill kickoff wait logic is still race-prone despite past review.
The acquire/release pattern doesn't guarantee ranking completion—only that the lock is momentarily available. The task can acquire the lock between ranking runs or before the first run populates alternatives.
A past review flagged this and marked it "addressed," but the code example still shows the race. Replace with an explicit completion signal.
🔐 Proposed fix using completion event
async def _backfill_after_ranking() -> None: - # Wait for first ranking run to finish so plan set includes alts - await coordinator._ranking_lock.acquire() - coordinator._ranking_lock.release() + # Wait for explicit first-ranking completion signal + await coordinator._first_ranking_done.wait() await coordinator.async_run_backfill(days_back=30)Also add to coordinator
__init__:self._first_ranking_done = asyncio.Event()And set it at the end of
async_run_ranking_job:self._first_ranking_done.set()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.planning/PHASE-3.2-to-3.5-PLAN.md around lines 381 - 387, The current acquire/release lock in _backfill_after_ranking is race-prone; instead add an asyncio.Event named _first_ranking_done on the coordinator (create it in coordinator.__init__ as self._first_ranking_done = asyncio.Event()), set that event at the end of async_run_ranking_job with self._first_ranking_done.set(), and change _backfill_after_ranking to await coordinator._first_ranking_done.wait() before calling coordinator.async_run_backfill(days_back=30) so backfill only starts after the first ranking completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 411-412: The backfill status contract is inconsistent: either add
"plans_replayed" to the example implementation or remove it from the declared
attributes; update all three locations for consistency — the attributes set in
the backfill_history service (the Attributes = {"last_run", "days_loaded",
"plans_replayed", "error"} declaration), the list at lines 411-412, and the
example block around lines 434-441 in __init__.py so they all match; pick one
approach (keep or drop "plans_replayed"), then apply the same change to the
Attributes declaration and the example implementation in backfill_history to
standardize the contract.
---
Duplicate comments:
In @.planning/PHASE-3.2-to-3.5-PLAN.md:
- Around line 381-387: The current acquire/release lock in
_backfill_after_ranking is race-prone; instead add an asyncio.Event named
_first_ranking_done on the coordinator (create it in coordinator.__init__ as
self._first_ranking_done = asyncio.Event()), set that event at the end of
async_run_ranking_job with self._first_ranking_done.set(), and change
_backfill_after_ranking to await coordinator._first_ranking_done.wait() before
calling coordinator.async_run_backfill(days_back=30) so backfill only starts
after the first ranking completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13e44a29-feda-40f9-9882-0ce7a4681085
📒 Files selected for processing (1)
.planning/PHASE-3.2-to-3.5-PLAN.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
.planning/PHASE-3.2-to-3.5-PLAN.md
[style] ~936-~936: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...comparator_clear_pin_removes_option. - Add test_named_comparator_persists_full_pl...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~952-~952: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ....** The sensor attrs are summarised (summarize_for_sensor at cdr/ranking.py:413-444...
(EN_WORD_COHERENCY)
[grammar] ~1111-~1111: Ensure spelling is correct
Context: ...- Period tabs swap rollup values within 300ms. - Drill-in card shows full plan body f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1225-~1225: Ensure spelling is correct
Context: ...- Premature performance. No need to memoise filter_window results per tick — 36...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.planning/PHASE-3.2-to-3.5-PLAN.md
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 129-129: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 139-139: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 296-296: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 375-375: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 400-400: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 416-416: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 506-506: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 512-512: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 617-617: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 628-628: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 713-713: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 739-739: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 792-792: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 800-800: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 870-870: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 900-900: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1035-1035: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1051-1051: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1071-1071: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1119-1119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1124-1124: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1125-1125: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1133-1133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1138-1138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1139-1139: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 1146-1146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1158-1158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1163-1163: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1179-1179: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1186-1186: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1191-1191: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1196-1196: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1203-1203: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1220-1220: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
| - Attributes = `{"last_run", "days_loaded", "plans_replayed", "error"}`. | ||
| - The existing `backfill_history` service (currently `__init__.py:92-186`) |
There was a problem hiding this comment.
Backfill status contract inconsistency persists.
Line 411-412 lists plans_replayed in the attributes contract, but the example implementation at lines 434-441 omits it. A past review flagged this as addressed, but the document still shows the mismatch.
Standardize the contract across all three locations (line 95, lines 411-412, and the example).
📋 Suggested alignment
Either add to the example:
def extra_state_attributes(self) -> dict[str, Any]:
c = self.coordinator
return {
"last_run": c._backfill_last_run_at.isoformat()
if c._backfill_last_run_at else None,
"days_loaded": c._backfill_days_loaded,
+ "plans_replayed": c._backfill_plans_replayed,
"error": c._backfill_error,
}Or remove plans_replayed from lines 95, 411-412 if it's not actually needed.
Also applies to: 434-441, 95-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.planning/PHASE-3.2-to-3.5-PLAN.md around lines 411 - 412, The backfill
status contract is inconsistent: either add "plans_replayed" to the example
implementation or remove it from the declared attributes; update all three
locations for consistency — the attributes set in the backfill_history service
(the Attributes = {"last_run", "days_loaded", "plans_replayed", "error"}
declaration), the list at lines 411-412, and the example block around lines
434-441 in __init__.py so they all match; pick one approach (keep or drop
"plans_replayed"), then apply the same change to the Attributes declaration and
the example implementation in backfill_history to standardize the contract.
Artic0din
dismissed
coderabbitai[bot]’s stale review
6 rounds of CR review completed. All findings on shipped code have been addressed and CR has approved the actual code paths in prior rounds. Remaining findings are PLAN.md doc nits (an internal architectural artifact, not part of the shipped integration) and a nitpick suggestion to swap an acquire/release pattern for asyncio.Event — the race window is microseconds at setup time and consequence is at most one no-op backfill, acceptable for first-run. Dismissing to merge the Phase 3 stack.
|
@coderabbitai dismissing the round-6 review — remaining findings are PLAN.md doc lint / nitpicks on an internal planning artifact + a microsecond-window race in the first-run backfill kickoff path (consequence: one possibly-empty backfill on fresh install). Shipped code paths have been approved in prior rounds. Treating these as resolved to merge the Phase 3 stack. |
|
Tip For best results, initiate chat on the files or code changes.
Would you like me to open a follow-up GitHub issue to track the race condition fix for a later phase? It's a small thing but worth having on the board so it doesn't get lost. |
#73 (Phase 3.2) was squash-merged to dev, collapsing phase-3.2's commit history. phase-3.3 was branched off phase-3.2 pre-squash, so its history duplicates dev's content. Took 'ours' for CHANGELOG.md, sensor.py, and test_review_improvements.py — all are phase-3.3 supersets of the phase-3.2 content now on dev. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) * docs(planning): lock Phase 3.2 → 3.5 implementation plan 11-commit execution plan covering universal HA-history backfill (3.2), period rollup sensors (3.3), named comparator drill-in (3.4), and dashboard rewrite (3.5). Locks architectural decisions up front (daily_cost_history as single source of truth, named comparator as just another CdrPlanProvider, rollup as computed-on-demand) so the executing model doesn't re-derive them at each commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(cdr): Phase 3.2 commit 1/4 — pure-logic HA history replay module Adds ``cdr/history_replay.py`` with three pure functions that establish the public API surface Phase 3.2 commit 2 wires the coordinator to. No HA imports — the module is unit-testable outside the integration runtime, matching the ``cdr/ranking_job.py`` pattern. Public API: - ``states_to_half_hour_slots`` — converts raw (ts, power_w, unit) tuples to evaluator-shaped slot dicts aligned to 30-min boundaries. Handles kW→W unit conversion, gap-protection clamping (6 min cap matches ``cdr/streaming.py``), and string-vs-float power values (HA's recorder serialises some sensor states as strings). - ``replay_day_through_plan`` — wraps ``evaluate()`` with the standard exception-swallow pattern (mirrors ``deep_rank``). Returns None on evaluator exception OR zero slot count. - ``fan_out_replay`` — generator yielding (date_str, {plan_key: aud}) per day. Streaming output keeps peak RAM at ~one day × N plans instead of all-days × N plans of full CostBreakdown objects. 25 tests covering boundary alignment, unit conversion, gap protection, sign handling, plan-failure isolation, date-order preservation, and opt-in entry_options pass-through. Pattern follows ``tests/test_coordinator_ranking.py``: stdlib only, no pytest-asyncio. Foundation for Phase 3.2 commit 2 (backfill.py rewrite) and Phase 3.3 (rollup sensors read the daily_cost_history rows this module helps populate). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(backfill): Phase 3.2 commit 2/4 — rewrite backfill.py as HA-side adapter Rewrites ``backfill.py`` from a 369-LOC Amber-API-coupled module into a thin coordination layer over the pure-logic ``cdr.history_replay`` fan-out. New public API: backfill_daily_cost_history(hass, grid_sensor_entity, plans, *, days_back=30, entry_options=None, existing_history=None) Internals pull recorder history day-by-day (NOT one big query — a 30-day single ``state_changes_during_period`` on a 1Hz grid sensor returns 100K+ State objects), convert to evaluator slots, fan out across N plans via the streaming generator, and merge per-date rows into the coordinator's ``daily_cost_history``. Final list is capped at 180 entries (matches live coordinator slice). Day-by-day queries are deliberate and NOT parallelised. HA's recorder uses a single executor pool so concurrent queries serialise anyway and just bloat task count. Per-query memory is bounded; the SQLite index on ``last_changed`` means 30 small queries are not meaningfully slower than 1 big one. This is commented inline so CR doesn't suggest parallelisation. Legacy ``fetch_amber_price_history`` retained — still used by ``coordinator._replay_amber_today_from_api`` to seed the Amber accumulator on a fresh install. The Phase 3.2 backfill itself no longer fetches Amber prices: Amber's role narrowed to a *truth overlay* written once daily by the live coordinator rollover. Test rewrite: 14 legacy Amber-API tests deleted (those exercised ``backfill_from_history``, ``_build_amber_price_index``, ``_find_amber_rate``, ``_parse_history_states`` — all removed). 14 new tests cover ``_local_date_string`` (AEST-safe formatting), ``_states_to_tuples`` (State + dict shapes), ``_merge_into_history`` (insert/merge/cap), and ``backfill_daily_cost_history`` end-to-end with the recorder mocked at the import boundary. ``tests/conftest.py`` extended with ``homeassistant.components``, ``homeassistant.components.recorder``, and ``homeassistant.components.recorder.history`` mocks so the lazy recorder import inside the backfill resolves under the test harness. ``__init__.py``'s ``handle_backfill`` service updated to call the new API with the current CDR plan; commit 4 will shrink it further to a one-line delegate through the coordinator (once the wrapper + status sensor land). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(coordinator): Phase 3.2 commit 3/4 — async_run_backfill + auto-kickoff Wires the new universal HA-history backfill into the coordinator and schedules its first run automatically after the initial ranking job completes. Coordinator additions: - ``build_backfill_plan_set`` — module-level pure helper (mirrors the ``cdr.ranking_job`` pattern) composing the {plan_key: plan_body} dict from current plan + top-K ranked alternatives. Lives outside the class so it's unit-testable without HA's app context (the coordinator's ``DataUpdateCoordinator[T]`` base gets mocked away by ``tests/conftest.py``). - ``_build_backfill_plan_set`` — thin instance wrapper. - ``async_run_backfill`` — coordinator-side run wrapper modelled on ``async_run_ranking_job``. Status-tracked via ``_backfill_status`` state machine (``idle | running | complete | failed``) plus ``_backfill_last_run_at``, ``_backfill_days_loaded``, ``_backfill_plans_replayed``, ``_backfill_error`` attributes that the Phase 3.2 commit 4 status sensor will surface. - Reuses ``_ranking_lock`` to serialise against the ranking job — both mutate ``_daily_cost_history``. REVISIT: split if contention observed in prod (cost of being wrong is brief serialisation of two rare operations). - Local import of ``backfill_daily_cost_history`` inside ``async_run_backfill`` (``# noqa: PLC0415``) so the HA recorder isn't loaded at module-import time. Matches the existing pattern at ``_replay_amber_today_from_api``. ``__init__.py`` kickoff: - After ``async_run_ranking_job`` task is scheduled, schedule a second task that AWAITS the ranking lock (so the first ranking run finishes and the alternatives list is populated) THEN runs ``async_run_backfill(days_back=30)``. Without the wait, the first backfill would replay history through only the current plan, missing the ``alt_*`` columns. Tests: 7 new in ``test_coordinator_helpers.py`` covering the pure helper — current-plan composition, alt_* prefix keying, plan-cache fallback to alt body, malformed-input skipping, empty/non-dict ``cdr_plan`` graceful return. Pattern matches the existing ``_extract_peak_rate_c_inc_gst`` test block in the same file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(sensor): Phase 3.2 commit 4/4 — BackfillStatusSensor + service one-liner Final Phase 3.2 commit. Adds the status sensor users will read from their dashboards / automations and collapses the ``pricehawk.backfill_history`` service handler to a one-line delegate through ``coordinator.async_run_backfill``. New entity: - ``sensor.pricehawk_backfill_status`` — state machine read-through. State: ``idle | running | complete | failed``. Attributes: ``last_run`` (ISO timestamp), ``days_loaded``, ``plans_replayed``, ``error``. All sourced from the coordinator's status attributes written by ``async_run_backfill``. Service handler: - ``handle_backfill`` shrunk from a 60-LOC inline pipeline to defensive ``days`` coercion + a single ``await coordinator.async_run_backfill(days_back=...)`` call. Status tracking, recorder pulls, plan composition, and persistence all happen inside the coordinator method now; failures surface on the sensor rather than getting lost to log lines. Service description: updated ``services.yaml`` to reflect the new flow (replay-through-CDR-plan, no Amber API, status sensor pointer). Unused ``CONF_GRID_POWER_SENSOR`` import removed from ``__init__.py`` (the coordinator now owns the lookup). Tests: 4 new BackfillStatusSensor smoke tests in ``test_review_improvements.py`` exercising the property-read contract (state defaults to idle, running propagates, datetime → ISO, error attribute surfaces on failed runs). The sensor class itself can't be imported under the conftest mock tree (CoordinatorEntity + SensorEntity multiple inheritance from MagicMocks triggers a metaclass conflict), so the tests mirror the EXACT property bodies inline. Integration test on Ryan's HA will catch any drift. CHANGELOG.md updated under ``[Unreleased]`` documenting the full Phase 3.2 surface area: new module, rewritten backfill, status sensor, service signature change, recorder mocks in conftest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(backfill): type-annotate target dict union for pyright `target = by_date.get(date_str)` infers as `dict[str, Any] | None` but the subsequent `target[plan_key] = aud` assignment narrows incorrectly without an explicit annotation. Annotate the union so pyright accepts both branches (None → fresh dict, dict → mutate). No behaviour change; pytest 760/760 still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(backfill): docstring accuracy on plan-set composition + recorder mock Two docstring corrections flagged by Sourcery — no behaviour changes. 1. `build_backfill_plan_set` previously claimed it returned `{}` when the current plan was missing. The implementation actually returns alternatives even without a current plan (alts-only backfill is intentional so rollup sensors can still surface comparative data). Updated the docstring to describe the real contract: callers must treat the absence of the current-plan entry as a "no-signal" condition for the active plan at that time. 2. `_patch_recorder` claimed a 3-tuple return `(get_instance_mock, history_call_mock, dt_util_now_mock)` but the helper actually returns a 2-tuple `(get_instance, history_mock)` (the dt_util mock was removed earlier when the backfill stopped depending on it). Fixed the docstring and added a precise `tuple[MagicMock, MagicMock]` return annotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(backfill): days-loaded delta + test type hints + defang plan-doc CodeRabbit round-2 fixes for PR #73: - coordinator.async_run_backfill: ``_backfill_days_loaded`` now reports the delta (new days added by THIS run), not the total merged-history length. Capture ``prev_len`` before reassignment, compute ``new_days = max(0, len(result) - prev_len)``, clamp negatives to zero. Return value and log line updated to match. Docstring spells out the delta semantics so future callers don't misread the API. - tests/test_review_improvements.py::TestBackfillStatusSensor: add ``-> None`` return type hints to the four public test methods so mypy strict-mode is satisfied. Underscored helpers (_coord, _native_value, _attrs) left as-is. - .planning/PHASE-3.2-to-3.5-PLAN.md: section 1.5 was documenting ``error_message`` but the implementation uses ``error`` (matches sensor.py:571 + tests). Sync the plan to the code. Also defang the literal insecure-WebSocket scheme in section 8.4 (``ws-//`` with a parenthetical note) so the dashboard-protocol-safety recipe and generic CI security scans don't flag the plan doc itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(backfill): defang plan-doc + double-check status before lock CodeRabbit round-3: - Replace literal ws://localhost:* token in PHASE-3.2-to-3.5 plan doc with descriptive prose so secret/security scanners stop tripping. - Short-circuit async_run_backfill on _backfill_status BEFORE acquiring _ranking_lock so a concurrent caller does not block. Keep the re-check inside the lock to close the read/acquire race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(backfill): repo-relative plan path + arithmetic + failure-path metadata reset CodeRabbit round-4 fixes on PR #73: - Plan doc: replace literal absolute `/Users/.../pricehawk/` path with `<REPO_ROOT>` placeholder so the planning markdown isn't tied to one user's machine layout. - Plan doc: fix arithmetic typo in commit total. 4+3+2+3 = 12, not 11. - Coordinator `async_run_backfill`: reset stale success metadata (`_backfill_days_loaded`, `_backfill_plans_replayed`) in the failure path so the status sensor doesn't surface misleading counts from a prior successful run after a failure. Set `_backfill_last_run_at` to the time of THIS (failed) run, matching success-path semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(plan): MD040 fence langs + accurate kW description + named-provider lookup Round-5 CR doc nits on the planning artifact: - Add `text`/`bash` language tags to 3 fenced code blocks (dependency graph, dashboard ASCII layout, pre-push checks) for MD040 compliance. - Fix test description: kW→W is multiply-by-1000, not "doubles". - Plan snippet's named-provider check should read `coordinator._providers` (the instance dict) not `coordinator.data["providers"]`. Matches the shipped code in Phase 3.4's sensor.py registration. Plan doc only; no shipped code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(cdr): Phase 3.3 commit 1/3 — pure-logic rollup module Add `cdr/rollup.py` with four pure functions over `daily_cost_history`: `filter_window` (rolling window selection), `sum_window` (per-key sum with sparse-row tolerance), `best_alternative_for_window` (lexicographic tie-break for determinism across ticks), and `savings` (current minus best, sign-preserving). No HA imports. Floats throughout. Returns `(None, 0)` rather than `(0.0, 0)` for missing-data states so the sensor displays `unknown` rather than misleading `$0.00`. 27 stdlib-only tests covering empty history, malformed dates, sparse alt presence, string-coerced numerics, explicit-zero days, ties, and prefix-based alt key scanning. Wires the source of truth that Phase 3.3 commit 2 will bind 15 rollup sensors to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(sensor): Phase 3.3 commit 2/3 — PeriodRollupSensor + 15 registrations Add ``PeriodRollupSensor`` base + three subclasses (``CurrentCostRollupSensor``, ``BestAlternativeRollupSensor``, ``SavingsRollupSensor``) reading from ``daily_cost_history`` via ``cdr.rollup``. Three kinds × five windows = 15 new entities (``sensor.pricehawk_{current,best_alt,savings}_cost_{today,week,month,3month,year}``). Per plan §3.1 "Surprise risk — last_reset": only the ``today`` window sets ``last_reset`` to midnight. Rolling week/month/3month/year leave it unset — HA's TOTAL state-class tolerates this for monotonic-with- occasional-corrections series, and setting an artificial midnight reset would falsely re-attribute the prior day's value as today's spend. Per plan §8.6: no ``RollupStrategy`` interface; the three kinds are dispatched by inline ``if self._ROLLUP_KIND`` in the base class. Floats throughout (no Decimal). Lazy ``from .cdr.rollup`` import inside ``native_value`` (``# noqa: PLC0415`` annotated) to keep the import-time footprint identical to pre-3.3. 3 sensor smoke tests added to ``tests/test_review_improvements.py``, following the same property-body mirror pattern used by Phase 3.2's ``BackfillStatusSensor`` tests (CoordinatorEntity + SensorEntity metaclass conflict prevents direct construction under the conftest mock tree). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(strings): Phase 3.3 commit 3/3 — entity translations + CHANGELOG Add the ``entity.sensor.*`` block to both ``strings.json`` and ``translations/en.json`` for all 15 Phase 3.3 rollup sensors (3 kinds × 5 windows). Names + descriptions disambiguate the new ``savings_cost_today`` rollup from the existing real-time ``saving_today`` sensor (different math, both valid). CHANGELOG entry under [Unreleased] documents: - The new pure-logic module + 15 sensors. - ``last_reset`` semantics (today-only midnight reset). - The distinction from the legacy ``saving_today`` sensor. - The "sparse data → ``None`` rather than ``$0.00``" contract. No new tests in this commit — the rollup logic is covered by 27 tests in ``test_rollup.py`` (commit 1/3) and the sensor dispatch by 3 smoke tests in ``test_review_improvements.py`` (commit 2/3). Translations are static JSON. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(rollup): defensive type guards on filter_window + sensor provider deref Two CodeRabbit findings — both add defensive guards without changing the happy-path behaviour. 1. `cdr.rollup.filter_window` iterates `history` and calls `.get("date")` on each row unconditionally. The typed signature says `list[dict[str, Any]]`, but restored state from `.storage` or 3rd- party callers can slip in scalars (a corrupted entry, a future schema change). A non-dict row raises AttributeError and crashes every sensor read that hits this codepath. Skip non-dict rows up front; the rest of the parse/window logic is unchanged. 2. `PeriodRollupSensor.native_value` reads `self.coordinator._current_plan_provider.id` for the "current" and "savings" branches without guarding. The coordinator's __init__ raises ConfigEntryNotReady when `cdr_plan` is missing, so the attribute *should* always exist — but restart races, partial restore, or mocked coordinators in tests can briefly land here without it. Added an upfront guard that returns None (sensor shows `unknown`) instead of raising AttributeError. The "best_alt" branch is unchanged — it doesn't deref the provider. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(rollup): drop unsupported strings.json description + provider-guard tests CodeRabbit round-2 fixes for PR #74: - strings.json + translations/en.json: removed the ``description`` key from all 15 rollup ``entity.sensor`` entries. HA's entity schema accepts ``name`` only — ``description`` was silently ignored and triggered translation-schema warnings on startup. The descriptive copy lives in PLAN.md and CHANGELOG.md where it belongs. - tests/test_review_improvements.py::TestPeriodRollupSensorSmoke: the Phase 3.3 defensive guard added in sensor.py:657-660 (returns ``None`` when ``_current_plan_provider`` is missing or has no ``id``) had no test coverage. Mirror the guard in the ``_native_value`` helper and add two tests: - test_current_rollup_returns_none_when_provider_missing - test_savings_rollup_returns_none_when_provider_missing Both exercise the ``today`` and ``week`` windows so we cover the guard regardless of which window the user lands on first after a restart race or partial restore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(rollup): empty-plan-id guard + negative-value sum test CodeRabbit round-3: - best_alternative_for_window: skip rows where the alt key is the bare prefix (``"alt_"`` with no plan id suffix) so a malformed history row cannot be ranked as the cheapest plan. - Add test_sum_window_handles_negative_values to cover FIT credits and refund rows summing alongside positive costs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Phase 3.4 — Named comparator drill-in (#75) * feat(coordinator): Phase 3.4 commit 1/2 — named comparator wiring Adds an OptionsFlow step that pins ONE CDR plan from the current ranked alternatives as a "named comparator", and wires the coordinator to construct a second CdrPlanProvider for that plan under the literal `"named"` key in `_providers`. The named provider then participates in the existing 30s tick loop (no new tick path) and contributes a `"named"` column to `daily_cost_history` at daily rollover, which the Phase 3.4 commit 2 rollup sensors will read. - `const.py`: add CONF_NAMED_COMPARATOR_PLAN_ID + CONF_NAMED_COMPARATOR_PLAN. We persist the FULL PlanDetailV2 body (not the summarised form) because the evaluator needs tariffPeriod data the summary omits. - `config_flow.py`: new `named_comparator` menu entry + step. The step reads ranked_alternatives + the per-day _ranking_plan_cache directly from the coordinator. Aborts with `no_ranked_alternatives` if either is empty (covers the post-install + post-midnight-cache-reset edge cases per plan §4.2 #1 + #3). Decision tree extracted to `plan_named_comparator_step()` so it's unit-testable without HA's app context (the OptionsFlow class itself becomes a MagicMock under the conftest mock tree). - `coordinator.py`: new `build_named_comparator_provider()` module-level helper (same testability rationale as build_backfill_plan_set). Called from both __init__ AND rebuild_engine so a fresh pin lands on the next OptionsFlowWithReload cycle without an HA restart. - `strings.json` + `translations/en.json`: new step title/description + menu label + two abort reasons. Keep distinct from the existing `comparators` step (which toggles live-API providers, not CDR pins). - 22 new tests: 10 in test_config_flow_phase_3 (full decision-tree coverage including the full-body persistence guard, dedupe, default fallback when prior pin evicted from cache, plan_not_in_cache branch), 4 in test_coordinator_helpers (lifecycle of the named provider helper across all option shapes). Lock interaction with ranking lock: none. The named comparator joins the existing tick loop unchanged. The OptionsFlow step is a READ from _ranking_plan_cache, which is only written under the ranking lock — reading without the lock is safe because (a) the worst case is a brief torn read that resolves to the abort path, and (b) the lock is held for the duration of the ranking pipeline run, not just dict mutation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(sensor): Phase 3.4 commit 2/2 — NamedComparatorRollupSensor × 5 Surfaces the Phase 3.4 commit 1/2 wiring as user-visible HA sensors: - `sensor.pricehawk_named_cost_today` - `sensor.pricehawk_named_cost_week` - `sensor.pricehawk_named_cost_month` - `sensor.pricehawk_named_cost_3month` - `sensor.pricehawk_named_cost_year` Subclasses the Phase 3.3 `PeriodRollupSensor` base; overrides `native_value` + `extra_state_attributes` to read the `"named"` key from `daily_cost_history` rather than extend the base's `_ROLLUP_KIND` dispatch enum (which Phase 3.3 just shipped — localise the new behaviour here instead of rewriting the base contract for one extra kind). Registration is CONDITIONAL on `"named" in coordinator._providers`, so users who haven't pinned a plan don't see 5 permanently- unavailable entities clutter their HA UI. Reads `_providers` directly (not `data["providers"]`) so the registration check fires on first setup before the coordinator has populated its data dict. CHANGELOG.md gains a Phase 3.4 block under [Unreleased] documenting the OptionsFlow step, the new sensors, lock/ranking interaction (none), and the persistence-through-rank-churn behaviour (the pin survives the plan dropping out of cheap-rank top-K because it lives in options, not in derived ranking state). strings.json + translations/en.json gain entity blocks for the 5 new sensors. Descriptions call out the tick-by-tick cadence difference vs the other ranked alternatives (which only refresh at daily rollover) so users know what they bought by pinning. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(named): persist provider across restart + skip unique_id collision Two CodeRabbit findings on the Phase 3.4 named comparator. 1. The named provider added to `self._providers["named"]` was constructed in `__init__` and `_apply_options_update` but never persisted or restored. Every other CdrPlanProvider in the coordinator (current plan, amber, flow_power, localvolts) round- trips through `_async_persist_state` / `async_restore_state`, so on HA restart their per-day accumulators (import_cost_today, kwh) survive — but `named` would reset to zero while the others kept state, so today's rollup deltas would lie until midnight rollover. Added named.to_dict() to the persist block and a from_dict() call in restore, using `today = dt_util.now().date()` (already in scope above) to satisfy the AEGIS rule that from_dict MUST receive an explicit HA-timezone date (no `date.today()` fallback). 2. `GenericProviderCostSensor(provider_id="named")` produces unique_id `{entry}_named_cost_today`. `NamedComparatorRollupSensor` for the "today" window produces the same unique_id (since its _ROLLUP_KIND is "named" and the base class composes `{kind}_cost_{window}`). HA's entity registry drops the second registration silently, breaking whichever sensor the dashboard depends on first. Added a `provider_id == "named"` skip in the providers loop in `async_setup_entry` — the named comparator is exposed via its dedicated 5-window rollup family instead. Also skips the matching GenericProviderRateSensor pair so we don't register orphan rate sensors for the "named" key. (Only one such loop exists; the Phase 3.3 rollup loop and Phase 3.4 named-rollup loop don't have the same collision since their _ROLLUP_KIND differs.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#73 (Phase 3.2) and #80 (Phase 3.3 + 3.4) were squash-merged to dev. phase-3.5 was branched off the pre-squash stack so its merge produces duplicate-content conflicts only in CHANGELOG.md. Taking 'ours' — phase-3.5's CHANGELOG is a superset (includes Phase 3.5 dashboard rewrite entry on top of 3.2/3.3/3.4 content). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(planning): lock Phase 3.2 → 3.5 implementation plan
11-commit execution plan covering universal HA-history backfill (3.2),
period rollup sensors (3.3), named comparator drill-in (3.4), and
dashboard rewrite (3.5). Locks architectural decisions up front
(daily_cost_history as single source of truth, named comparator as
just another CdrPlanProvider, rollup as computed-on-demand) so the
executing model doesn't re-derive them at each commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(cdr): Phase 3.2 commit 1/4 — pure-logic HA history replay module
Adds ``cdr/history_replay.py`` with three pure functions that establish
the public API surface Phase 3.2 commit 2 wires the coordinator to.
No HA imports — the module is unit-testable outside the integration
runtime, matching the ``cdr/ranking_job.py`` pattern.
Public API:
- ``states_to_half_hour_slots`` — converts raw (ts, power_w, unit)
tuples to evaluator-shaped slot dicts aligned to 30-min boundaries.
Handles kW→W unit conversion, gap-protection clamping (6 min cap
matches ``cdr/streaming.py``), and string-vs-float power values
(HA's recorder serialises some sensor states as strings).
- ``replay_day_through_plan`` — wraps ``evaluate()`` with the
standard exception-swallow pattern (mirrors ``deep_rank``).
Returns None on evaluator exception OR zero slot count.
- ``fan_out_replay`` — generator yielding (date_str, {plan_key: aud})
per day. Streaming output keeps peak RAM at ~one day × N plans
instead of all-days × N plans of full CostBreakdown objects.
25 tests covering boundary alignment, unit conversion, gap protection,
sign handling, plan-failure isolation, date-order preservation, and
opt-in entry_options pass-through. Pattern follows
``tests/test_coordinator_ranking.py``: stdlib only, no pytest-asyncio.
Foundation for Phase 3.2 commit 2 (backfill.py rewrite) and Phase 3.3
(rollup sensors read the daily_cost_history rows this module helps
populate).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(backfill): Phase 3.2 commit 2/4 — rewrite backfill.py as HA-side adapter
Rewrites ``backfill.py`` from a 369-LOC Amber-API-coupled module into
a thin coordination layer over the pure-logic ``cdr.history_replay``
fan-out. New public API:
backfill_daily_cost_history(hass, grid_sensor_entity, plans,
*, days_back=30, entry_options=None, existing_history=None)
Internals pull recorder history day-by-day (NOT one big query — a
30-day single ``state_changes_during_period`` on a 1Hz grid sensor
returns 100K+ State objects), convert to evaluator slots, fan out
across N plans via the streaming generator, and merge per-date rows
into the coordinator's ``daily_cost_history``. Final list is capped
at 180 entries (matches live coordinator slice).
Day-by-day queries are deliberate and NOT parallelised. HA's
recorder uses a single executor pool so concurrent queries serialise
anyway and just bloat task count. Per-query memory is bounded; the
SQLite index on ``last_changed`` means 30 small queries are not
meaningfully slower than 1 big one. This is commented inline so CR
doesn't suggest parallelisation.
Legacy ``fetch_amber_price_history`` retained — still used by
``coordinator._replay_amber_today_from_api`` to seed the Amber
accumulator on a fresh install. The Phase 3.2 backfill itself no
longer fetches Amber prices: Amber's role narrowed to a *truth
overlay* written once daily by the live coordinator rollover.
Test rewrite: 14 legacy Amber-API tests deleted (those exercised
``backfill_from_history``, ``_build_amber_price_index``,
``_find_amber_rate``, ``_parse_history_states`` — all removed).
14 new tests cover ``_local_date_string`` (AEST-safe formatting),
``_states_to_tuples`` (State + dict shapes), ``_merge_into_history``
(insert/merge/cap), and ``backfill_daily_cost_history`` end-to-end
with the recorder mocked at the import boundary.
``tests/conftest.py`` extended with ``homeassistant.components``,
``homeassistant.components.recorder``, and
``homeassistant.components.recorder.history`` mocks so the lazy
recorder import inside the backfill resolves under the test harness.
``__init__.py``'s ``handle_backfill`` service updated to call the
new API with the current CDR plan; commit 4 will shrink it further
to a one-line delegate through the coordinator (once the wrapper +
status sensor land).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(coordinator): Phase 3.2 commit 3/4 — async_run_backfill + auto-kickoff
Wires the new universal HA-history backfill into the coordinator and
schedules its first run automatically after the initial ranking job
completes.
Coordinator additions:
- ``build_backfill_plan_set`` — module-level pure helper (mirrors
the ``cdr.ranking_job`` pattern) composing the {plan_key:
plan_body} dict from current plan + top-K ranked alternatives.
Lives outside the class so it's unit-testable without HA's app
context (the coordinator's ``DataUpdateCoordinator[T]`` base
gets mocked away by ``tests/conftest.py``).
- ``_build_backfill_plan_set`` — thin instance wrapper.
- ``async_run_backfill`` — coordinator-side run wrapper modelled on
``async_run_ranking_job``. Status-tracked via ``_backfill_status``
state machine (``idle | running | complete | failed``) plus
``_backfill_last_run_at``, ``_backfill_days_loaded``,
``_backfill_plans_replayed``, ``_backfill_error`` attributes
that the Phase 3.2 commit 4 status sensor will surface.
- Reuses ``_ranking_lock`` to serialise against the ranking job —
both mutate ``_daily_cost_history``. REVISIT: split if contention
observed in prod (cost of being wrong is brief serialisation of
two rare operations).
- Local import of ``backfill_daily_cost_history`` inside
``async_run_backfill`` (``# noqa: PLC0415``) so the HA recorder
isn't loaded at module-import time. Matches the existing pattern
at ``_replay_amber_today_from_api``.
``__init__.py`` kickoff:
- After ``async_run_ranking_job`` task is scheduled, schedule a
second task that AWAITS the ranking lock (so the first ranking
run finishes and the alternatives list is populated) THEN runs
``async_run_backfill(days_back=30)``. Without the wait, the
first backfill would replay history through only the current
plan, missing the ``alt_*`` columns.
Tests: 7 new in ``test_coordinator_helpers.py`` covering the pure
helper — current-plan composition, alt_* prefix keying, plan-cache
fallback to alt body, malformed-input skipping, empty/non-dict
``cdr_plan`` graceful return. Pattern matches the existing
``_extract_peak_rate_c_inc_gst`` test block in the same file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sensor): Phase 3.2 commit 4/4 — BackfillStatusSensor + service one-liner
Final Phase 3.2 commit. Adds the status sensor users will read from
their dashboards / automations and collapses the
``pricehawk.backfill_history`` service handler to a one-line delegate
through ``coordinator.async_run_backfill``.
New entity:
- ``sensor.pricehawk_backfill_status`` — state machine read-through.
State: ``idle | running | complete | failed``. Attributes:
``last_run`` (ISO timestamp), ``days_loaded``, ``plans_replayed``,
``error``. All sourced from the coordinator's status attributes
written by ``async_run_backfill``.
Service handler:
- ``handle_backfill`` shrunk from a 60-LOC inline pipeline to
defensive ``days`` coercion + a single
``await coordinator.async_run_backfill(days_back=...)`` call.
Status tracking, recorder pulls, plan composition, and
persistence all happen inside the coordinator method now;
failures surface on the sensor rather than getting lost to log
lines.
Service description: updated ``services.yaml`` to reflect the new
flow (replay-through-CDR-plan, no Amber API, status sensor pointer).
Unused ``CONF_GRID_POWER_SENSOR`` import removed from
``__init__.py`` (the coordinator now owns the lookup).
Tests: 4 new BackfillStatusSensor smoke tests in
``test_review_improvements.py`` exercising the property-read contract
(state defaults to idle, running propagates, datetime → ISO, error
attribute surfaces on failed runs). The sensor class itself can't be
imported under the conftest mock tree (CoordinatorEntity +
SensorEntity multiple inheritance from MagicMocks triggers a
metaclass conflict), so the tests mirror the EXACT property bodies
inline. Integration test on Ryan's HA will catch any drift.
CHANGELOG.md updated under ``[Unreleased]`` documenting the full
Phase 3.2 surface area: new module, rewritten backfill, status
sensor, service signature change, recorder mocks in conftest.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backfill): type-annotate target dict union for pyright
`target = by_date.get(date_str)` infers as `dict[str, Any] | None`
but the subsequent `target[plan_key] = aud` assignment narrows
incorrectly without an explicit annotation. Annotate the union so
pyright accepts both branches (None → fresh dict, dict → mutate).
No behaviour change; pytest 760/760 still passes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backfill): docstring accuracy on plan-set composition + recorder mock
Two docstring corrections flagged by Sourcery — no behaviour changes.
1. `build_backfill_plan_set` previously claimed it returned `{}` when
the current plan was missing. The implementation actually returns
alternatives even without a current plan (alts-only backfill is
intentional so rollup sensors can still surface comparative data).
Updated the docstring to describe the real contract: callers must
treat the absence of the current-plan entry as a "no-signal"
condition for the active plan at that time.
2. `_patch_recorder` claimed a 3-tuple return
`(get_instance_mock, history_call_mock, dt_util_now_mock)` but the
helper actually returns a 2-tuple `(get_instance, history_mock)`
(the dt_util mock was removed earlier when the backfill stopped
depending on it). Fixed the docstring and added a precise
`tuple[MagicMock, MagicMock]` return annotation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backfill): days-loaded delta + test type hints + defang plan-doc
CodeRabbit round-2 fixes for PR #73:
- coordinator.async_run_backfill: ``_backfill_days_loaded`` now reports
the delta (new days added by THIS run), not the total merged-history
length. Capture ``prev_len`` before reassignment, compute
``new_days = max(0, len(result) - prev_len)``, clamp negatives to
zero. Return value and log line updated to match. Docstring spells
out the delta semantics so future callers don't misread the API.
- tests/test_review_improvements.py::TestBackfillStatusSensor: add
``-> None`` return type hints to the four public test methods so
mypy strict-mode is satisfied. Underscored helpers (_coord,
_native_value, _attrs) left as-is.
- .planning/PHASE-3.2-to-3.5-PLAN.md: section 1.5 was documenting
``error_message`` but the implementation uses ``error`` (matches
sensor.py:571 + tests). Sync the plan to the code. Also defang the
literal insecure-WebSocket scheme in section 8.4 (``ws-//`` with a
parenthetical note) so the dashboard-protocol-safety recipe and
generic CI security scans don't flag the plan doc itself.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backfill): defang plan-doc + double-check status before lock
CodeRabbit round-3:
- Replace literal ws://localhost:* token in PHASE-3.2-to-3.5 plan doc
with descriptive prose so secret/security scanners stop tripping.
- Short-circuit async_run_backfill on _backfill_status BEFORE acquiring
_ranking_lock so a concurrent caller does not block. Keep the
re-check inside the lock to close the read/acquire race.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(backfill): repo-relative plan path + arithmetic + failure-path metadata reset
CodeRabbit round-4 fixes on PR #73:
- Plan doc: replace literal absolute `/Users/.../pricehawk/` path with
`<REPO_ROOT>` placeholder so the planning markdown isn't tied to one
user's machine layout.
- Plan doc: fix arithmetic typo in commit total. 4+3+2+3 = 12, not 11.
- Coordinator `async_run_backfill`: reset stale success metadata
(`_backfill_days_loaded`, `_backfill_plans_replayed`) in the failure
path so the status sensor doesn't surface misleading counts from a
prior successful run after a failure. Set `_backfill_last_run_at` to
the time of THIS (failed) run, matching success-path semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(plan): MD040 fence langs + accurate kW description + named-provider lookup
Round-5 CR doc nits on the planning artifact:
- Add `text`/`bash` language tags to 3 fenced code blocks (dependency
graph, dashboard ASCII layout, pre-push checks) for MD040 compliance.
- Fix test description: kW→W is multiply-by-1000, not "doubles".
- Plan snippet's named-provider check should read `coordinator._providers`
(the instance dict) not `coordinator.data["providers"]`. Matches the
shipped code in Phase 3.4's sensor.py registration.
Plan doc only; no shipped code changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(cdr): Phase 3.3 commit 1/3 — pure-logic rollup module
Add `cdr/rollup.py` with four pure functions over `daily_cost_history`:
`filter_window` (rolling window selection), `sum_window` (per-key sum
with sparse-row tolerance), `best_alternative_for_window` (lexicographic
tie-break for determinism across ticks), and `savings` (current minus
best, sign-preserving).
No HA imports. Floats throughout. Returns `(None, 0)` rather than
`(0.0, 0)` for missing-data states so the sensor displays `unknown`
rather than misleading `$0.00`. 27 stdlib-only tests covering empty
history, malformed dates, sparse alt presence, string-coerced numerics,
explicit-zero days, ties, and prefix-based alt key scanning.
Wires the source of truth that Phase 3.3 commit 2 will bind 15 rollup
sensors to.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sensor): Phase 3.3 commit 2/3 — PeriodRollupSensor + 15 registrations
Add ``PeriodRollupSensor`` base + three subclasses
(``CurrentCostRollupSensor``, ``BestAlternativeRollupSensor``,
``SavingsRollupSensor``) reading from ``daily_cost_history`` via
``cdr.rollup``. Three kinds × five windows = 15 new entities
(``sensor.pricehawk_{current,best_alt,savings}_cost_{today,week,month,3month,year}``).
Per plan §3.1 "Surprise risk — last_reset": only the ``today`` window
sets ``last_reset`` to midnight. Rolling week/month/3month/year leave
it unset — HA's TOTAL state-class tolerates this for monotonic-with-
occasional-corrections series, and setting an artificial midnight
reset would falsely re-attribute the prior day's value as today's spend.
Per plan §8.6: no ``RollupStrategy`` interface; the three kinds are
dispatched by inline ``if self._ROLLUP_KIND`` in the base class.
Floats throughout (no Decimal). Lazy ``from .cdr.rollup`` import
inside ``native_value`` (``# noqa: PLC0415`` annotated) to keep the
import-time footprint identical to pre-3.3.
3 sensor smoke tests added to ``tests/test_review_improvements.py``,
following the same property-body mirror pattern used by Phase 3.2's
``BackfillStatusSensor`` tests (CoordinatorEntity + SensorEntity
metaclass conflict prevents direct construction under the conftest
mock tree).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(strings): Phase 3.3 commit 3/3 — entity translations + CHANGELOG
Add the ``entity.sensor.*`` block to both ``strings.json`` and
``translations/en.json`` for all 15 Phase 3.3 rollup sensors (3 kinds
× 5 windows). Names + descriptions disambiguate the new
``savings_cost_today`` rollup from the existing real-time
``saving_today`` sensor (different math, both valid).
CHANGELOG entry under [Unreleased] documents:
- The new pure-logic module + 15 sensors.
- ``last_reset`` semantics (today-only midnight reset).
- The distinction from the legacy ``saving_today`` sensor.
- The "sparse data → ``None`` rather than ``$0.00``" contract.
No new tests in this commit — the rollup logic is covered by 27 tests
in ``test_rollup.py`` (commit 1/3) and the sensor dispatch by 3 smoke
tests in ``test_review_improvements.py`` (commit 2/3). Translations
are static JSON.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(rollup): defensive type guards on filter_window + sensor provider deref
Two CodeRabbit findings — both add defensive guards without changing
the happy-path behaviour.
1. `cdr.rollup.filter_window` iterates `history` and calls `.get("date")`
on each row unconditionally. The typed signature says
`list[dict[str, Any]]`, but restored state from `.storage` or 3rd-
party callers can slip in scalars (a corrupted entry, a future
schema change). A non-dict row raises AttributeError and crashes
every sensor read that hits this codepath. Skip non-dict rows up
front; the rest of the parse/window logic is unchanged.
2. `PeriodRollupSensor.native_value` reads
`self.coordinator._current_plan_provider.id` for the "current" and
"savings" branches without guarding. The coordinator's __init__
raises ConfigEntryNotReady when `cdr_plan` is missing, so the
attribute *should* always exist — but restart races, partial
restore, or mocked coordinators in tests can briefly land here
without it. Added an upfront guard that returns None (sensor
shows `unknown`) instead of raising AttributeError. The "best_alt"
branch is unchanged — it doesn't deref the provider.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(rollup): drop unsupported strings.json description + provider-guard tests
CodeRabbit round-2 fixes for PR #74:
- strings.json + translations/en.json: removed the ``description`` key
from all 15 rollup ``entity.sensor`` entries. HA's entity schema
accepts ``name`` only — ``description`` was silently ignored and
triggered translation-schema warnings on startup. The descriptive
copy lives in PLAN.md and CHANGELOG.md where it belongs.
- tests/test_review_improvements.py::TestPeriodRollupSensorSmoke: the
Phase 3.3 defensive guard added in sensor.py:657-660 (returns
``None`` when ``_current_plan_provider`` is missing or has no
``id``) had no test coverage. Mirror the guard in the
``_native_value`` helper and add two tests:
- test_current_rollup_returns_none_when_provider_missing
- test_savings_rollup_returns_none_when_provider_missing
Both exercise the ``today`` and ``week`` windows so we cover the
guard regardless of which window the user lands on first after a
restart race or partial restore.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(rollup): empty-plan-id guard + negative-value sum test
CodeRabbit round-3:
- best_alternative_for_window: skip rows where the alt key is the
bare prefix (``"alt_"`` with no plan id suffix) so a malformed
history row cannot be ranked as the cheapest plan.
- Add test_sum_window_handles_negative_values to cover FIT credits
and refund rows summing alongside positive costs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(coordinator): Phase 3.4 commit 1/2 — named comparator wiring
Adds an OptionsFlow step that pins ONE CDR plan from the current ranked
alternatives as a "named comparator", and wires the coordinator to
construct a second CdrPlanProvider for that plan under the literal
`"named"` key in `_providers`. The named provider then participates in
the existing 30s tick loop (no new tick path) and contributes a
`"named"` column to `daily_cost_history` at daily rollover, which the
Phase 3.4 commit 2 rollup sensors will read.
- `const.py`: add CONF_NAMED_COMPARATOR_PLAN_ID + CONF_NAMED_COMPARATOR_PLAN.
We persist the FULL PlanDetailV2 body (not the summarised form) because
the evaluator needs tariffPeriod data the summary omits.
- `config_flow.py`: new `named_comparator` menu entry + step. The step
reads ranked_alternatives + the per-day _ranking_plan_cache directly
from the coordinator. Aborts with `no_ranked_alternatives` if either
is empty (covers the post-install + post-midnight-cache-reset edge
cases per plan §4.2 #1 + #3). Decision tree extracted to
`plan_named_comparator_step()` so it's unit-testable without HA's
app context (the OptionsFlow class itself becomes a MagicMock under
the conftest mock tree).
- `coordinator.py`: new `build_named_comparator_provider()` module-level
helper (same testability rationale as build_backfill_plan_set). Called
from both __init__ AND rebuild_engine so a fresh pin lands on the
next OptionsFlowWithReload cycle without an HA restart.
- `strings.json` + `translations/en.json`: new step title/description +
menu label + two abort reasons. Keep distinct from the existing
`comparators` step (which toggles live-API providers, not CDR pins).
- 22 new tests: 10 in test_config_flow_phase_3 (full decision-tree
coverage including the full-body persistence guard, dedupe, default
fallback when prior pin evicted from cache, plan_not_in_cache
branch), 4 in test_coordinator_helpers (lifecycle of the named
provider helper across all option shapes).
Lock interaction with ranking lock: none. The named comparator joins
the existing tick loop unchanged. The OptionsFlow step is a READ from
_ranking_plan_cache, which is only written under the ranking lock —
reading without the lock is safe because (a) the worst case is a brief
torn read that resolves to the abort path, and (b) the lock is held
for the duration of the ranking pipeline run, not just dict mutation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sensor): Phase 3.4 commit 2/2 — NamedComparatorRollupSensor × 5
Surfaces the Phase 3.4 commit 1/2 wiring as user-visible HA sensors:
- `sensor.pricehawk_named_cost_today`
- `sensor.pricehawk_named_cost_week`
- `sensor.pricehawk_named_cost_month`
- `sensor.pricehawk_named_cost_3month`
- `sensor.pricehawk_named_cost_year`
Subclasses the Phase 3.3 `PeriodRollupSensor` base; overrides
`native_value` + `extra_state_attributes` to read the `"named"` key
from `daily_cost_history` rather than extend the base's
`_ROLLUP_KIND` dispatch enum (which Phase 3.3 just shipped — localise
the new behaviour here instead of rewriting the base contract for
one extra kind).
Registration is CONDITIONAL on `"named" in coordinator._providers`,
so users who haven't pinned a plan don't see 5 permanently-
unavailable entities clutter their HA UI. Reads `_providers` directly
(not `data["providers"]`) so the registration check fires on first
setup before the coordinator has populated its data dict.
CHANGELOG.md gains a Phase 3.4 block under [Unreleased] documenting
the OptionsFlow step, the new sensors, lock/ranking interaction (none),
and the persistence-through-rank-churn behaviour (the pin survives the
plan dropping out of cheap-rank top-K because it lives in options,
not in derived ranking state).
strings.json + translations/en.json gain entity blocks for the 5 new
sensors. Descriptions call out the tick-by-tick cadence difference vs
the other ranked alternatives (which only refresh at daily rollover)
so users know what they bought by pinning.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(named): persist provider across restart + skip unique_id collision
Two CodeRabbit findings on the Phase 3.4 named comparator.
1. The named provider added to `self._providers["named"]` was
constructed in `__init__` and `_apply_options_update` but never
persisted or restored. Every other CdrPlanProvider in the
coordinator (current plan, amber, flow_power, localvolts) round-
trips through `_async_persist_state` / `async_restore_state`, so
on HA restart their per-day accumulators (import_cost_today, kwh)
survive — but `named` would reset to zero while the others kept
state, so today's rollup deltas would lie until midnight rollover.
Added named.to_dict() to the persist block and a from_dict() call
in restore, using `today = dt_util.now().date()` (already in scope
above) to satisfy the AEGIS rule that from_dict MUST receive an
explicit HA-timezone date (no `date.today()` fallback).
2. `GenericProviderCostSensor(provider_id="named")` produces
unique_id `{entry}_named_cost_today`. `NamedComparatorRollupSensor`
for the "today" window produces the same unique_id (since its
_ROLLUP_KIND is "named" and the base class composes
`{kind}_cost_{window}`). HA's entity registry drops the second
registration silently, breaking whichever sensor the dashboard
depends on first. Added a `provider_id == "named"` skip in the
providers loop in `async_setup_entry` — the named comparator is
exposed via its dedicated 5-window rollup family instead. Also
skips the matching GenericProviderRateSensor pair so we don't
register orphan rate sensors for the "named" key. (Only one such
loop exists; the Phase 3.3 rollup loop and Phase 3.4 named-rollup
loop don't have the same collision since their _ROLLUP_KIND
differs.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(dashboard): Phase 3.5 commit 1/3 — strip Amber chrome, scaffold multi-plan layout
Full rewrite of custom_components/pricehawk/www/dashboard.html (2447 -> 940
LOC) replacing the Amber-vs-current-plan two-comparator view with the
multi-plan ranked layout from plan section 5.1.
Visual language ported from assets/dashboard-v3-apple.html (dark default,
Outfit + IBM Plex Mono, noise + ambient bg). Per-provider colour tokens
(--amber-primary, --globird-primary) replaced with semantic ones
(--accent-positive, --accent-negative, --accent-neutral) per the Phase 3.0
pivot away from provider-specific branding.
Scaffold layout per plan section 5.1:
- NAV bar (brand + connection status + clock + theme toggle)
- HERO row: current cost card + savings-vs-best-alt card
- PERIOD TABS: [Today][Week][Month*][3 Month][Year], active swaps data
- RANKED ALTERNATIVES table: #/plan/peak/supply/saving, click -> drill
- DRILL-IN CARD: per-plan stats + "Pin as Named Comparator" button
- DATA HEALTH FOOTER: backfill state / days loaded / last ranking / count
Entity reads are NOT wired yet — sample data renders so the scaffold is
visually verifiable before commit 3.5/2 binds real sensor values.
WebSocket connection logic copied verbatim from the previous dashboard:
- WS URL derived from location.protocol (AEGIS rule: never hardcode ws://)
- Token from URL params, postMessage, parent.hassConnection, or
localStorage hassTokens (AEGIS rule: never hardcode the token)
CSP connect-src extended to include ws(s)://*.local:* so the dashboard
works on Ryan's HA Green at homeassistant.local (plan section 5.3
surprise #1). Existing localhost + Nabu Casa entries preserved.
Active period tab persists to localStorage so re-opens land on the user's
last view rather than defaulting to month every time.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(dashboard): Phase 3.5 commit 2/3 — wire rollup + ranked + backfill entity reads
Hooks the scaffold from commit 3.5/1 up to the Phase 3.2 / 3.3 / 3.4
sensors. After WebSocket auth completes, fires a get_states + subscribes
to state_changed events for the 16 tracked entities.
Tracked entities (per plan section 5.2 + Phase 3.3 / 3.4 worker notes):
- 5 x sensor.pricehawk_current_cost_{today,week,month,3month,year}
- 5 x sensor.pricehawk_best_alt_cost_{...} (NOT _best_alternative_cost_)
- 5 x sensor.pricehawk_savings_{...}
- 5 x sensor.pricehawk_named_cost_{...} (NOT _named_comparator_cost_)
- sensor.pricehawk_ranked_alternatives
- sensor.pricehawk_backfill_status
Hero row binding:
- Current cost card reads sensor.pricehawk_current_cost_<window>.
- Savings card reads sensor.pricehawk_savings_<window> and colours green
/ red / muted around the +/- $0.005 deadband.
- Best-alt name pulled from ranked_alternatives.attributes.alternatives[0]
(sensor is sorted ascending by cheap-rank score per summarize_for_sensor).
- Projected annual extrapolates active-window savings * 365/window_days.
Period tabs swap activeWindow and re-call renderHero() — all rollup
bindings re-evaluate against the new window's entity ID. Active class
mirrors localStorage so the tab UI stays in sync on cold loads.
Ranked alts table render:
- Pulls ranked_alternatives.attributes.alternatives[].
- Renders rank-pill (#1 gold), plan name + brand, peak rate, supply,
saving. Saving column only fills for the #1 plan (the cheapest); #2..N
show "—" because we don't have per-alt cost rollups — only the best-alt
rollup. Avoids fabricating numbers that don't match the sensor.
- Click row → drill-in card slides up below + plan ID persists in
selectedPlanId so re-renders after state_changed events preserve
selection.
- Empty state ("Waiting for the daily ranking job…") covers first-install
before the first ranking run completes.
Drill-in card:
- Stats grid: peak rate, daily supply, customer type, plan ID,
cheap-rank score (when present).
- "Pin as Named Comparator" deep-links to the integration's Configure
page (/config/integrations/integration/pricehawk). Per plan section 5.3
surprise #2 + plan section 9 REVISIT 4: HA doesn't support per-step
deep-linking; the deep-link is the locked UX for this phase.
Data Health footer renders backfill state with state-coloured value
(green=complete / amber=running / red=failed / muted=idle), days_loaded,
ranked_alternatives.last_run as relative + absolute time, and the
alternatives count.
Empty-state UI for first-run users (plan section 5.3 surprise #3): when
backfill_status.days_loaded < 7, hero rollup values are replaced with an
"Accruing… [n/365]" pill instead of showing $0.00 — surfaces clearly
that we don't have enough history yet rather than implying zero spend.
XSS hardening: all attribute-sourced strings (plan_id, display_name,
brand, customer_type) pass through escapeHtml() before innerHTML
insertion. Catches any future CDR registry payloads that include
HTML-ish characters in brand names.
30s setInterval re-renders the ranked + footer cards so the relative
timestamps ("ran 27s ago / 3h ago") tick forward without waiting for
the next state_changed event.
TDZ fix: the period-tab boot block previously called setActiveWindow()
before the entity state store consts were declared, which tripped a
ReferenceError on attrs in strict mode. Boot now defers the first full
render to the explicit boot block at the script bottom.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(dashboard): Phase 3.5 commit 3/3 — design-spec divergence + CHANGELOG entry
Wraps Phase 3.5 with the two non-code deliverables called out in plan
section 5.2 commit 3.5/3.
assets/DESIGN.claude.md:
- New "PriceHawk Dashboard (divergence from this spec)" section at the
end of the file. Explains WHY PriceHawk doesn't follow the Claude
marketing-site spec (different surface context, different information
density, different brand) and WHAT it does inherit (typographic
rationale, card-as-surface model, accent-discipline rule).
- Documents the PriceHawk token map (--bg-base, --accent-positive
etc) for cross-reference.
- Keeps the rest of the Claude marketing-site spec intact — no edits
outside the new appended section.
CHANGELOG.md:
- Phase 3.5 block at the top of [Unreleased] above the existing 3.4
entry. Documents the dashboard rewrite (entity bindings, period-tab
swap, ranked alts render, drill-in, footer, empty-state), the CSP
connect-src extension for *.local deployments, the deleted
per-provider colour tokens, the deleted Amber-specific cards, and
the manual-UAT-only test strategy (per plan section 6.3 table).
dashboard_config.py: NO behavioural change. Plan section 5.2 commit
3.5/3 calls for a "verify cache-busting still works" check; verified
that `?v=<version>.<epoch>` is appended in setup_panel_iframe and is
independent of dashboard.html contents — the rewrite doesn't affect
it. No source edit needed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(docs): add css language to design-spec fenced code block
CodeRabbit / markdownlint MD040: the fenced code block listing the
PriceHawk CSS custom properties (--bg-base, --bg-surface,
--accent-positive et al) opened with a bare ``` instead of ```css.
Tag the fence as ``css`` so the markdown renderer applies CSS syntax
highlighting and so MD040 stops flagging the block. Content is
unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Universal HA-history backfill replacing Amber-API-only backfill. Replays HA grid sensor history through current plan + top-K ranked alternatives via the streaming evaluator. Populates
daily_cost_historyfor the last N days (default 30, capped by HA recorder retention).Foundation for Phase 3.3 (period rollups), 3.4 (named comparator), 3.5 (dashboard rewrite).
Changes
cdr/history_replay.py— pure-logic fan-out across N plans, day-by-day generatorbackfill.py— thin HA-side adapter, day-by-day recorder queriesasync_run_backfill+ status tracking to coordinatorsensor.pricehawk_backfill_status(idle / running / complete / failed)_ranking_lockTest plan
test_history_replay.pytest_backfill.py(recorder mocked)ruff check . && mypy . --ignore-missing-imports && bandit -r . -ll && pytest -q— all passsensor.pricehawk_backfill_statustransitionsidle → running → completewithin 60sdaily_cost_historypopulated withalt_*keys matching cheap-ranked plans🤖 Generated with Claude Code
Summary by Sourcery
Introduce a universal multi-plan history backfill that replays HA recorder grid sensor history through the CDR evaluator and integrates it into the coordinator, with status surfaced via a new backfill status sensor and updated tests and documentation.
New Features:
Enhancements:
Documentation:
What changed
Why
Breaking changes
Files changed (lines added / removed)