Phase 3.4 — Named comparator drill-in#75
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a Phase 3.4 named-comparator option to pin a ranked CDR plan, builds an optional "named" provider in the coordinator, exposes five rolling-window named-cost sensors, and includes localization and tests. ChangesNamed Comparator Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant HA
participant OptionsFlow
participant PlanCache
participant Coordinator
participant Sensors
User->>HA: Open options -> select "Pin a Named Comparator"
HA->>OptionsFlow: Render dropdown using ranked_alternatives
OptionsFlow->>PlanCache: plan_named_comparator_step validates cached plans
User->>OptionsFlow: Select plan ID or (clear pin)
alt Plan exists in cache
OptionsFlow->>Coordinator: Persist full plan body + plan ID
Coordinator->>Coordinator: build_named_comparator_provider -> register "named"
Coordinator->>Sensors: Evaluate pinned plan on tick (≈30s)
Sensors-->>HA: Publish named rollup sensor states
else Plan evicted from cache
OptionsFlow-->>User: Abort plan_not_in_cache
else No alternatives
OptionsFlow-->>User: Abort no_ranked_alternatives
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements Phase 3.4 'named comparator' feature: adds an options flow step to pin a single CDR plan as a tick-level comparator, wires a dedicated CdrPlanProvider into the coordinator under a stable 'named' key, and exposes 5 new rollup sensors that aggregate the pinned plan’s costs over standard windows, with supporting constants, translations, and unit tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 left some high level feedback:
- The provider key "named" is hard-coded in multiple places (coordinator, sensors, rollup, tests); consider centralizing it as a constant to avoid typos and keep future refactors simpler.
- The "(clear pin)" label for the named comparator selector is currently a hard-coded English string in
plan_named_comparator_step; it would be more consistent with the rest of the flow to source this from translations/strings rather than embedding it directly. - The
NamedComparatorRollupSensorduplicates the window-filtering and sum logic already used byPeriodRollupSensor; you might want to refactor common rollup behavior into a shared helper to reduce duplication and keep future rollup changes in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The provider key "named" is hard-coded in multiple places (coordinator, sensors, rollup, tests); consider centralizing it as a constant to avoid typos and keep future refactors simpler.
- The "(clear pin)" label for the named comparator selector is currently a hard-coded English string in `plan_named_comparator_step`; it would be more consistent with the rest of the flow to source this from translations/strings rather than embedding it directly.
- The `NamedComparatorRollupSensor` duplicates the window-filtering and sum logic already used by `PeriodRollupSensor`; you might want to refactor common rollup behavior into a shared helper to reduce duplication and keep future rollup changes in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
5 tasks
Owner
Author
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/pricehawk/sensor.py (1)
855-873:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unique_id collision between generic named cost and named rollup
today.When
"named"is active,GenericProviderCostSensorandNamedComparatorRollupSensor(window="today")both resolve tonamed_cost_today(same unique_id key). One sensor gets dropped.💡 Suggested fix
@@ for provider_id, snap in providers_block.items(): @@ - entities.append( - GenericProviderCostSensor( - coordinator, entry, provider_id, provider_name - ) - ) + # Avoid collision with NamedComparatorRollupSensor unique_id: + # "named_cost_today". + if provider_id != "named": + entities.append( + GenericProviderCostSensor( + coordinator, entry, provider_id, provider_name + ) + )Also applies to: 904-915
🤖 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 `@custom_components/pricehawk/sensor.py` around lines 855 - 873, The two sensors collide on unique_id (e.g., both resolving to named_cost_today) — update the unique_id generation for the sensors to make them unique: modify GenericProviderCostSensor.unique_id (and/or NamedComparatorRollupSensor.unique_id) to include a distinguishing token such as the sensor type ("cost" vs "rollup") and the window value (e.g., "_cost_today" vs "_rollup_today") and ensure provider_id/provider_name is part of the key; change the unique_id construction inside the GenericProviderCostSensor and NamedComparatorRollupSensor classes so they produce distinct identifiers (also apply the same fix for the other occurrence referenced around the 904-915 block).
🤖 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 274-295: The named comparator provider created by
build_named_comparator_provider and registered as self._providers["named"] is
not persisted/restored, so add the same persist/restore logic used for other
CdrPlanProvider instances to save its accumulator/daily totals on shutdown and
to call the provider's from_dict restore method on startup; ensure you pass the
explicit Home Assistant timezone date (do not allow a date.today() fallback)
into from_dict when restoring the named provider so its totals align with peers
after restart.
---
Outside diff comments:
In `@custom_components/pricehawk/sensor.py`:
- Around line 855-873: The two sensors collide on unique_id (e.g., both
resolving to named_cost_today) — update the unique_id generation for the sensors
to make them unique: modify GenericProviderCostSensor.unique_id (and/or
NamedComparatorRollupSensor.unique_id) to include a distinguishing token such as
the sensor type ("cost" vs "rollup") and the window value (e.g., "_cost_today"
vs "_rollup_today") and ensure provider_id/provider_name is part of the key;
change the unique_id construction inside the GenericProviderCostSensor and
NamedComparatorRollupSensor classes so they produce distinct identifiers (also
apply the same fix for the other occurrence referenced around the 904-915
block).
🪄 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: bb1a99db-2fa2-4da4-bc80-dd2a99febbf6
📒 Files selected for processing (9)
CHANGELOG.mdcustom_components/pricehawk/config_flow.pycustom_components/pricehawk/const.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/sensor.pycustom_components/pricehawk/strings.jsoncustom_components/pricehawk/translations/en.jsontests/test_config_flow_phase_3.pytests/test_coordinator_helpers.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/sensor.pycustom_components/pricehawk/const.pytests/test_coordinator_helpers.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/config_flow.pytests/test_config_flow_phase_3.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/sensor.pycustom_components/pricehawk/const.pytests/test_coordinator_helpers.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/config_flow.pytests/test_config_flow_phase_3.py
**/*.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
**/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_config_flow_phase_3.py
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~28-~28: Do not mix variants of the same word (‘summarize’ and ‘summarise’) within a single text.
Context: ...2body (not the summarised form fromcdr.ranking.summarize_for_sensor`) because the evaluator ne...
(EN_WORD_COHERENCY)
🔇 Additional comments (14)
custom_components/pricehawk/strings.json (4)
239-239: LGTM!
260-266: LGTM!
429-432: LGTM!
496-515: LGTM!custom_components/pricehawk/translations/en.json (4)
239-239: LGTM!
260-266: LGTM!
429-432: LGTM!
496-515: LGTM!CHANGELOG.md (1)
9-66: LGTM!custom_components/pricehawk/const.py (1)
74-90: LGTM!custom_components/pricehawk/coordinator.py (1)
186-207: LGTM!custom_components/pricehawk/config_flow.py (1)
70-71: LGTM!Also applies to: 99-194, 1979-2111
tests/test_config_flow_phase_3.py (1)
9-17: LGTM!Also applies to: 64-298
tests/test_coordinator_helpers.py (1)
12-12: LGTM!Also applies to: 247-317
Artic0din
force-pushed
the
phase-3.3-period-rollups
branch
from
d0fb00c to
ecd50cc
Compare
Owner
Author
|
@coderabbitai review |
Artic0din
force-pushed
the
phase-3.4-named-comparator
branch
from
c010f01 to
c06117a
Compare
✅ Actions performedReview triggered.
|
Artic0din
force-pushed
the
phase-3.3-period-rollups
branch
from
ecd50cc to
b5a1e95
Compare
Artic0din
force-pushed
the
phase-3.4-named-comparator
branch
from
c06117a to
cb979c4
Compare
Artic0din
force-pushed
the
phase-3.3-period-rollups
branch
from
b5a1e95 to
1ee02ac
Compare
Artic0din
force-pushed
the
phase-3.4-named-comparator
branch
from
cb979c4 to
5449eed
Compare
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>
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>
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>
Artic0din
force-pushed
the
phase-3.3-period-rollups
branch
from
1ee02ac to
99ff449
Compare
Artic0din
force-pushed
the
phase-3.4-named-comparator
branch
from
5449eed to
507db8b
Compare
3 tasks
Artic0din
added a commit
that referenced
this pull request
May 18, 2026
) * 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>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User can pin ONE CDR plan from the ranked alternatives as a "named comparator" that runs tick-by-tick (every coordinator update) instead of only daily. Surfaces as 5 new rollup sensors:
sensor.pricehawk_named_cost_{today,week,month,3month,year}.Stacked on PR #74 (Phase 3.3 — Period rollup sensors) which is stacked on PR #73 (Phase 3.2 — Universal HA-history backfill). Merge #73 → #74 → #75 in order.
Changes
named_comparator— dropdown of current ranked alternatives + "clear pin" optionCONF_NAMED_COMPARATOR_PLAN_ID+CONF_NAMED_COMPARATOR_PLANconstantsCdrPlanProviderkeyed"named"when plan pinned — joins existing tick loopNamedComparatorRollupSensor(subclass ofPeriodRollupSensorfrom 3.3) × 5 windowsTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Introduce a configurable named comparator CDR plan and corresponding rolling cost sensors driven by a dedicated provider and options flow step.
New Features:
Enhancements:
Documentation:
Tests:
What changed
Why
Breaking changes
Files changed (lines added/removed)