refactor(coordinator): extract _apply_options_to_state#170
Conversation
…+ add DWT equivalence cases Linus PR #170 audit fixed two real defects in _apply_options_to_state: 1. Orphan-state bug. The projector cleared self._dwt_provider = None BEFORE the strict-mode guard decided whether to bail. A non-strict rebuild with neither cdr_plan nor a DWT enable flag would null _dwt_provider, then early-return, leaving _current_plan_provider pointing at the stale DWT instance — a half-rebuilt coordinator. Fix moves the reset INSIDE the CDR branch, past the early-return, so a bad rebuild keeps every provider slot intact. P13 no-regression: every prior call site that depended on the CDR branch nulling _dwt_provider still sees that behaviour, because the reset still happens — just not in the orphan window. 2. DWT branch untested. _EQUIVALENCE_CASES were all CDR — the DWT vs CDR branch of _build_dwt_provider had no init-vs-rebuild parity assertion. Added dwt_oe_enabled_options_only (OpenElectricity path, every setting in options) and dwt_aemo_enabled_data_fallback (AEMO Direct, every setting in data so the opt() fallback inside _build_dwt_provider is what makes the assertion pass). 3. New regression test test_nonstrict_rebuild_preserves_dwt_provider_on_bad_options pre-seeds a DWT provider, fires a bad rebuild, asserts all three slots survive. Confirmed to fail pre-fix. Minor (Linus also flagged): FlowPowerProvider(dict(options)) and LocalVoltsProvider(dict(options)) verified safe — both constructors accept dict[str, Any] and dict(...) coerces any Mapping to a plain dict without behaviour change. Validation: - uv run ruff check on changed files: passes (pre-existing scripts/ failures unrelated) - uv run pyright custom_components/pricehawk/coordinator.py: same 2 pre-existing errors / 12 warnings — no new typing issues - uv run pytest tests/test_coordinator.py tests/test_reconfigure.py tests/test_runtime_data.py: 54 passed P13 no-regression + P17 tests are part of the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd01c2a017
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t DWT options (codex follow-up) Codex P2 follow-up on PR #170. ``_apply_options_to_state(strict=False)`` unconditionally called ``_build_dwt_provider``, which raises ``ConfigEntryNotReady`` (AC-10c) when an entry's ``current_provider`` marker says DWT but the matching enable/API-key fields are missing. Pre-refactor ``rebuild_engine`` never built a DWT provider from scratch, so the same inconsistent options update would log and keep the existing providers — the systemic-fix refactor regressed that behaviour and now tears the integration down mid-edit. Fix wraps the builder call in ``try/except ConfigEntryNotReady`` that only swallows the raise on the non-strict (rebuild) path — strict mode (``__init__``) still re-raises so HA surfaces the failure at initial setup. On the rebuild path we log and bail BEFORE touching ``_dwt_provider`` / ``_current_plan_provider`` / ``_providers``, matching the existing missing-``cdr_plan`` early-return contract (P13 no-regression). Tests: ``tests/test_reconfigure.py::TestRebuildGracefulDegradeOnInconsistentDwt`` pins the regression — non-strict rebuild does not raise, keeps existing providers, AND strict init still raises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # so it is safe to clear the DWT slot before reassigning the | ||
| # current-plan provider. | ||
| self._dwt_provider = None | ||
| self._current_plan_provider = CdrPlanProvider( |
There was a problem hiding this comment.
🚫 [mypy] reported by reviewdog 🐶
Incompatible types in assignment (expression has type "CdrPlanProvider", variable has type "Provider") [assignment]
| # so it is safe to clear the DWT slot before reassigning the | ||
| # current-plan provider. | ||
| self._dwt_provider = None | ||
| self._current_plan_provider = CdrPlanProvider( |
There was a problem hiding this comment.
📝 [mypy] reported by reviewdog 🐶
Protocol member Provider.id expected settable variable, got read-only attribute
| # so it is safe to clear the DWT slot before reassigning the | ||
| # current-plan provider. | ||
| self._dwt_provider = None | ||
| self._current_plan_provider = CdrPlanProvider( |
There was a problem hiding this comment.
📝 [mypy] reported by reviewdog 🐶
Protocol member Provider.name expected settable variable, got read-only attribute
| dict(options) | ||
| ) | ||
| if self._named_comparator is not None: | ||
| self._providers["named"] = self._named_comparator |
There was a problem hiding this comment.
🚫 [mypy] reported by reviewdog 🐶
Incompatible types in assignment (expression has type "CdrPlanProvider", target has type "Provider") [assignment]
| dict(options) | ||
| ) | ||
| if self._named_comparator is not None: | ||
| self._providers["named"] = self._named_comparator |
There was a problem hiding this comment.
📝 [mypy] reported by reviewdog 🐶
Protocol member Provider.id expected settable variable, got read-only attribute
| dict(options) | ||
| ) | ||
| if self._named_comparator is not None: | ||
| self._providers["named"] = self._named_comparator |
There was a problem hiding this comment.
📝 [mypy] reported by reviewdog 🐶
Protocol member Provider.name expected settable variable, got read-only attribute
| new_options.get(CONF_GRID_POWER_SENSOR) | ||
| or self.config_entry.data.get(CONF_GRID_POWER_SENSOR, "") | ||
| self._apply_options_to_state( | ||
| new_options, self.config_entry.data, strict=False, |
There was a problem hiding this comment.
🚫 [mypy] reported by reviewdog 🐶
Item "None" of "ConfigEntry[Any] | None" has no attribute "data" [union-attr]
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Both `__init__` (initial setup) and `rebuild_engine` (options-flow reload) duplicated grid-sensor resolution, the DWT vs CDR current-plan slot, every comparator's pricing-mode logic, and the named comparator. That duplication had already drifted twice in production (#150 grid sensor; #153 grid-sensor double-assign). Constitution P14 — "If the same issue appears in multiple places, fix the underlying abstraction." Both call sites now flow through one projector. Strict mode (init-time) raises ConfigEntryNotReady on missing required config; non-strict mode (rebuild-time) degrades gracefully — preserves existing semantics on both paths. `_build_dwt_provider` now accepts `(options, data)` mappings instead of a `ConfigEntry` so the same builder serves both paths. Tests: parametrised `TestApplyOptionsToStateEquivalence` asserts the init-time and rebuild-time paths produce identical observable state across four scenario fixtures plus strict/non-strict gate semantics and the #150 grid-sensor regression case. `tests/conftest.py` now stubs `DataUpdateCoordinator` with a real subclassable class so `PriceHawkCoordinator` resolves as an actual type for unit testing (was a `_MockModule` MagicMock). Validation: ruff clean on touched files; pyright net -2 errors (2 pre-existing CdrPlanProvider protocol mismatches remain, was 4 duplicates before); pytest 1108/1108 passing including 8 new equivalence tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ add DWT equivalence cases Linus PR #170 audit fixed two real defects in _apply_options_to_state: 1. Orphan-state bug. The projector cleared self._dwt_provider = None BEFORE the strict-mode guard decided whether to bail. A non-strict rebuild with neither cdr_plan nor a DWT enable flag would null _dwt_provider, then early-return, leaving _current_plan_provider pointing at the stale DWT instance — a half-rebuilt coordinator. Fix moves the reset INSIDE the CDR branch, past the early-return, so a bad rebuild keeps every provider slot intact. P13 no-regression: every prior call site that depended on the CDR branch nulling _dwt_provider still sees that behaviour, because the reset still happens — just not in the orphan window. 2. DWT branch untested. _EQUIVALENCE_CASES were all CDR — the DWT vs CDR branch of _build_dwt_provider had no init-vs-rebuild parity assertion. Added dwt_oe_enabled_options_only (OpenElectricity path, every setting in options) and dwt_aemo_enabled_data_fallback (AEMO Direct, every setting in data so the opt() fallback inside _build_dwt_provider is what makes the assertion pass). 3. New regression test test_nonstrict_rebuild_preserves_dwt_provider_on_bad_options pre-seeds a DWT provider, fires a bad rebuild, asserts all three slots survive. Confirmed to fail pre-fix. Minor (Linus also flagged): FlowPowerProvider(dict(options)) and LocalVoltsProvider(dict(options)) verified safe — both constructors accept dict[str, Any] and dict(...) coerces any Mapping to a plain dict without behaviour change. Validation: - uv run ruff check on changed files: passes (pre-existing scripts/ failures unrelated) - uv run pyright custom_components/pricehawk/coordinator.py: same 2 pre-existing errors / 12 warnings — no new typing issues - uv run pytest tests/test_coordinator.py tests/test_reconfigure.py tests/test_runtime_data.py: 54 passed P13 no-regression + P17 tests are part of the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t DWT options (codex follow-up) Codex P2 follow-up on PR #170. ``_apply_options_to_state(strict=False)`` unconditionally called ``_build_dwt_provider``, which raises ``ConfigEntryNotReady`` (AC-10c) when an entry's ``current_provider`` marker says DWT but the matching enable/API-key fields are missing. Pre-refactor ``rebuild_engine`` never built a DWT provider from scratch, so the same inconsistent options update would log and keep the existing providers — the systemic-fix refactor regressed that behaviour and now tears the integration down mid-edit. Fix wraps the builder call in ``try/except ConfigEntryNotReady`` that only swallows the raise on the non-strict (rebuild) path — strict mode (``__init__``) still re-raises so HA surfaces the failure at initial setup. On the rebuild path we log and bail BEFORE touching ``_dwt_provider`` / ``_current_plan_provider`` / ``_providers``, matching the existing missing-``cdr_plan`` early-return contract (P13 no-regression). Tests: ``tests/test_reconfigure.py::TestRebuildGracefulDegradeOnInconsistentDwt`` pins the regression — non-strict rebuild does not raise, keeps existing providers, AND strict init still raises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Artic0din
force-pushed
the
refactor/constitution-02-rebuild-engine-systemic
branch
from
e2164b1 to
da5de4c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da5de4c1c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # same projector so grid-sensor resolution, current-plan slot | ||
| # (DWT/CDR), and comparator pricing-mode logic cannot drift. | ||
| self._dwt_provider: DynamicWholesaleTariffProvider | None = None | ||
| dwt_provider = self._build_dwt_provider(entry) |
There was a problem hiding this comment.
This helper was changed to require options and data, but the init path still calls it with the old single ConfigEntry argument. Every PriceHawkCoordinator(hass, entry) construction reaches this line before the new projector call, so integration setup now raises TypeError: _build_dwt_provider() missing 1 required positional argument: 'data' before any provider state can be initialized.
Useful? React with 👍 / 👎.
| oe_enabled = bool(_resolve(entry, CONF_DWT_OE_ENABLED)) | ||
| aemo_enabled = bool(_resolve(entry, CONF_DWT_AEMO_ENABLED)) | ||
| current_provider = _resolve(entry, CONF_CURRENT_PROVIDER) |
There was a problem hiding this comment.
After the signature change, _build_dwt_provider no longer has an entry variable in scope, but these remaining _resolve(entry, ...) reads run before the new opt() fallback values are used. Any rebuild path that calls _apply_options_to_state() reaches this helper and raises NameError before it can decide whether the entry is DWT or CDR, so options-flow rebuilds are broken even for normal CDR entries.
Useful? React with 👍 / 👎.
| self._apply_options_to_state( | ||
| new_options, self.config_entry.data, strict=False, | ||
| ) |
There was a problem hiding this comment.
Appending the new projector at the end means it is skipped by the old early-return path above: for a DWT entry whose DWT markers live in entry.data rather than new_options (the new test fixture explicitly covers this data-fallback case), lines 2587-2635 see no DWT flag and no cdr_plan and return before this call. In that options-flow scenario, edits such as changing the grid sensor or DWT region are silently ignored instead of being handled by _apply_options_to_state.
Useful? React with 👍 / 👎.
| self._grid_power_entity = ( | ||
| options.get(CONF_GRID_POWER_SENSOR) | ||
| or data.get(CONF_GRID_POWER_SENSOR, "") | ||
| ) |
There was a problem hiding this comment.
The new projector uses truthiness instead of the repository's documented options-over-data lookup, so if the options flow supplies CONF_GRID_POWER_SENSOR as an empty value (for example after the user clears the selector or the entity disappears), this falls back to the setup-time entry.data sensor and keeps reading the old entity. The helper comments above _resolve explicitly treat empty option values as intentional clears; this branch should match that absent-key-only fallback semantics.
Useful? React with 👍 / 👎.
* fix(coordinator): repair PR #170 auto-resolve breakage PR #170's rebase auto-merge concatenated both sides of the `_apply_options_to_state` refactor, leaving: - 3 duplicate `oe_enabled = bool(opt(...))` lines before `def opt(...)` declared (`F821 Undefined name 'opt'`) - 1 stale `opt(CONF_API_KEY)` call in `__init__` scope where `opt` is not defined (should remain `_resolve(entry, CONF_API_KEY)`) Delete the 3 duplicate lines; restore L650 to `_resolve(entry, ...)`. ruff clean + AST parses. * fix(tests): drop duplicate MagicMock import in test_coordinator.py * fix(coordinator): align _build_dwt_provider call site with new signature
The #170 auto-merge cascade left full inline copies of the _apply_options_to_state projector in BOTH __init__ and rebuild_engine, each running alongside the actual projector call — double application plus 5 no-redef errors (attributes assigned twice with annotations) and a latent bug where _api_key was read options→data via _resolve_str then immediately overwritten by a data-only read. - __init__: remove the dead inline DWT/CDR + providers + amber_mode + named_comparator block (~75 lines); keep typed placeholders + the single projector call. Fixes 5 no-redef. - rebuild_engine: remove the dead inline provider-rebuild block (~145 lines); keep only the strict=False projector call it already made. Fixes 5 Mapping-vs-dict arg-type errors. - _api_key: single options→data read via _resolve_str (drops the data-only override that silently ignored options-flow key edits). - rebuild_per_tick_explanation: param retyped dict[str,dict[str,Any]] → ProviderBlock to match _build_providers_block + build_explanation. Fixes 2 arg-type errors. explanation.py: - _won_bullets helpers: replace `.get(key, {})` (which poisons the ProviderSnapshot TypedDict into a union) with None-guarded access. Fixes call-overload + var-annotated. Net: 220 fewer lines, projector is now the single source of truth as its docstring always claimed, 12 of 14 mypy errors fixed by real code repair (not suppression).
* ci(mypy): restore mypy with HA follow_imports=skip boundary Root cause of the earlier mypy hell: pytest-homeassistant-custom-component pins HA to 2023.7.3, whose stubs lag the APIs the code targets (OptionsFlowWithReload, current SelectorOptionsType). mypy analysing those stale internals produced 30+ false positives. Fix per constitution §14 (systemic over local) + §19 (platform conventions): treat `homeassistant.*` and `openelectricity.*` as Any via `follow_imports = skip` in mypy.ini — the correct boundary for a HACS integration whose HA API correctness is validated at runtime, not by mypy. Our own logic stays fully type-checked. Restores the Mypy CI step. Expect only real, non-HA type errors to remain (arg-type, no-redef, var-annotated, call-overload) — fixed in follow-up commits on this branch. * fix(coordinator): de-duplicate projector logic + fix 12 type errors The #170 auto-merge cascade left full inline copies of the _apply_options_to_state projector in BOTH __init__ and rebuild_engine, each running alongside the actual projector call — double application plus 5 no-redef errors (attributes assigned twice with annotations) and a latent bug where _api_key was read options→data via _resolve_str then immediately overwritten by a data-only read. - __init__: remove the dead inline DWT/CDR + providers + amber_mode + named_comparator block (~75 lines); keep typed placeholders + the single projector call. Fixes 5 no-redef. - rebuild_engine: remove the dead inline provider-rebuild block (~145 lines); keep only the strict=False projector call it already made. Fixes 5 Mapping-vs-dict arg-type errors. - _api_key: single options→data read via _resolve_str (drops the data-only override that silently ignored options-flow key edits). - rebuild_per_tick_explanation: param retyped dict[str,dict[str,Any]] → ProviderBlock to match _build_providers_block + build_explanation. Fixes 2 arg-type errors. explanation.py: - _won_bullets helpers: replace `.get(key, {})` (which poisons the ProviderSnapshot TypedDict into a union) with None-guarded access. Fixes call-overload + var-annotated. Net: 220 fewer lines, projector is now the single source of truth as its docstring always claimed, 12 of 14 mypy errors fixed by real code repair (not suppression).
1 participant
Summary
Constitution P14 systemic fix —
__init__andrebuild_engineno longer duplicate provider/mode setup.The two paths previously hand-rolled the same logic across ~175 lines each:
grid-sensor resolution (options→data fallback), the DWT vs CDR current-plan slot,
every comparator's three-state pricing-mode resolution (Amber / Flow Power / LocalVolts),
the named-comparator wiring, and the providers-dict reset.
That duplication had already drifted twice in production
(retro-review #150 surfaced the grid-sensor read; #153 caught a duplicate assignment
that would have silently negated the data-fallback).
Per Constitution P14:
Both call sites now flow through a single
_apply_options_to_state(options, data, *, strict)projector.strict=True(init-time) raisesConfigEntryNotReadyon missing required config(cdr_plan, static-plan envelopes for non-off static_prd modes, inconsistent DWT markers per AC-10c).
strict=False(rebuild-time) degrades gracefully — logs and keeps the existing provideron missing cdr_plan; drops a comparator to
offon missing static plan.Both semantic behaviours match the pre-refactor implementation exactly.
_build_dwt_providernow accepts(options, data)mappings instead of aConfigEntry,so the same builder serves both paths.
Constitution alignment
Test plan
uv run ruff check custom_components/pricehawk/coordinator.py tests/test_coordinator.py tests/test_review_improvements.py tests/conftest.py— clean on all touched files (5 pre-existing F541s inscripts/are unrelated, baseline confirmed).uv run pyright custom_components/pricehawk/coordinator.py— 2 errors (down from 4; remaining errors are the pre-existing CdrPlanProvider/Provider protocol mismatch, not introduced by this refactor).uv run pytest --tb=short tests/test_coordinator.py tests/test_runtime_data.py tests/test_reconfigure.py— 51/51 passing (43 baseline + 8 new equivalence tests).uv run pytest --tb=short --ignore=tests/test_cdr_evaluator.py— 1108/1108 full suite passing.static_prdtolive_apion a comparator must rebuild that provider without tearing down the integration.Deviations
tests/conftest.pygained a real_StubDataUpdateCoordinatorbase class soPriceHawkCoordinatorresolves as an actual type for unit tests. Before,DataUpdateCoordinatorwas a_MockModuleMagicMock andclass PriceHawkCoordinator(DataUpdateCoordinator[...])ended up as a MagicMock too — meaning the existingtest_review_improvements.py::TestCoordinatorReset::test_monthly_reset_handles_all_providerswas never actually exercising real coordinator behaviour. That test was updated to add a minimalcdr_planenvelope so the real-class construction path succeeds.coordinator.py(lines 481, 599) — pre-existingCdrPlanProvider(read-only@property id) vsProviderprotocol (writableid: strattribute) variance issue. Out of scope for this refactor; would require either making the Protocol attributes read-only (broad refactor across all providers) or makingCdrPlanProvider.ida writable attribute. Baseline pyright confirms these 2 errors existed before the refactor — the refactor reduced the count from 4 (because the duplicate logic inrebuild_engineraised the same errors twice).🤖 Generated with Claude Code