chore: retro-review PR #111 — fix(codex): P1-1 named_comparator + P1-2 service singleton + P1-4 singleRate + P#141
Closed
Artic0din wants to merge 4 commits into
Closed
Conversation
Codex P1-4 (2026-05-23): cheap_rank_score documented support for SINGLE_RATE plans, but _extract_peak_rate_cents() only walked tariffPeriod[].timeOfUseRates[]. Real CDR PlanDetailV2 bodies with rateBlockUType="singleRate" carry their rates in a singleRate.rates[] block — NOT in timeOfUseRates. Those plans scored None and got silently excluded from the ranked-alternatives list, biasing the output against flat/single-rate plans. Fix: read BOTH blocks and take the max unitPrice across them. The existing test fixture encodes single-rate plans inside timeOfUseRates with type="SINGLE_RATE" for fixture compat — that shape is still handled. Real CDR endpoints (which use the singleRate key) now work too. The demand-charges path (TODO-5 in TODOS.md) remains intentionally out of scope — that's a different cost model that needs evaluator work, not just rate extraction. 3 regression tests in TestCheapRankScore: - single-rate block extraction (real CDR shape) - stepped singleRate picks max - hybrid plan with both blocks picks max across them 1081 passing (was 1078), ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1-1 (2026-05-23): the named-comparator OptionsFlow step read
the coordinator from ``self.hass.data[DOMAIN][entry_id]``, but v3
stores the coordinator on ``entry.runtime_data`` (PriceHawkData
dataclass) — see data.py and the integration's async_setup_entry.
The legacy hass.data lookup always returned None on a v3 install, so
``alternatives`` stayed empty and the step aborted with
``no_ranked_alternatives`` even when the coordinator HAD a populated
ranked-alternatives list. The user couldn't pin any plan from the
ranked list as a Named Comparator — the whole feature was a no-op.
Fix: read ``self.config_entry.runtime_data.coordinator`` instead.
The Silver-checklist "no legacy hass.data" test (test_runtime_data
test_no_legacy_hass_data_reads) already grep-banned this pattern
package-wide; codex caught that this specific call site slipped
through because the test fixture matched only the literal string
``hass.data[DOMAIN]`` (with the brackets), and the broken line used
``hass.data.get(DOMAIN, {}).get(...)`` instead.
2 regression tests in tests/test_codex_p0_p1_fixes.py:
- named_comparator reads from runtime_data
- named_comparator does not use legacy access patterns
1083 passing (was 1081), ruff clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1-2 (2026-05-23): the three services (analyze_csv, backfill_history, rank_alternatives) were registered inside ``async_setup_entry`` with handlers that captured the current ``entry`` in a closure. HA's service registry stores one handler per (domain, name), so each entry's setup silently overwrote the previous registration. Multi-entry installs ended up routing every service call to whichever entry was set up LAST — calls to a service intended for entry-A could land on entry-B's coordinator. Fix: - Lift the three handlers + a shared ``_resolve_service_target_entry`` helper into a module-level ``_register_services_once`` function. - Guard registration with ``hass.services.has_service`` so the second entry's setup is a no-op for service registration. - Handlers resolve the target entry at call time via ``hass.config_entries.async_entries(DOMAIN)``, honoring an optional ``entry_id`` field in the service call data. Multiple loaded entries without an ``entry_id`` raise ServiceValidationError so the caller has to be explicit (silent routing is the bug we're fixing). - ``async_unload_entry`` already removes the singleton services on the LAST entry's unload (pre-existing pattern, lines 240-247) — pairs cleanly with this idempotent-register. Tests: - tests/test_runtime_data.py::test_service_handlers_resolve_fresh_coordinator updated: the hass mock now pre-populates ``config_entries.async_entries`` with the test entry so the new resolution path can find it. - tests/test_runtime_data.py::_make_hass: ``has_service`` default return_value=False so the idempotent guard doesn't skip registration in test runs. - tests/test_codex_p0_p1_fixes.py: 4 new tests pinning the singleton helper + has_service guard + call-time resolver + ambiguous-call rejection. 1087 passing (was 1083), ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1-3 (2026-05-23) flagged that `async_add_external_statistics` with `has_sum=True` is "monotonic", and our cumulative cost sum can decrease on export-heavy days. The fix codex proposed (split import + export streams) is a schema change that would break existing users' stat history, so verify before changing. Context7 query against HA developer docs confirms: - The monotonic rule lives on SENSOR-derived stats with `state_class=TOTAL_INCREASING`: "if the difference is negative, nothing is added to the sum." - For `state_class=TOTAL`: sum updates include negative diffs. - External statistics (via `async_add_external_statistics`) carry no `state_class` field on `StatisticMetaData`. The integration provides absolute `sum` values directly; HA stores them as given. - Energy Dashboard's cost source computes today's cost as `sum[end_of_day] - sum[start_of_day]`. A decreasing sum produces a negative today's-cost — correct for a net-export day where the user earned more than they spent. Codex may have been applying the sensor-stats monotonic rule to the external-stats path incorrectly. Our current single-net-cost stat is plausibly correct; the remaining concern is Energy Dashboard UX (do users see broken-looking negative bars on export-heavy days?). Updated TODO-CODEX-P1-3 to reflect: - Research complete - Code change deferred pending live-UAT eyeball test on a known export-heavy day (will happen naturally during solar season on the installed entry) - Decision criteria written: if negative-bar UX is acceptable, close as won't-fix; otherwise, split streams in a separate migration PR - Priority downgraded P1 → P2 (now blocked on UX call, not engineering) No code change in this commit — pure docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@claude review |
|
Claude encountered an error after 11m 52s —— View job Code Review — PR #111 Retro
|
|
Claude encountered an error after 12m 10s —— View job PR #111 Retro-Review
|
Owner
Author
|
Retro-review complete. Findings (if any) tracked in fix/retro-review-batch (#143) or noted in .planning/retro-review-findings.md. Closing synthetic PR + cleaning branches. |
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.
Synthetic retro-review of merged PR #111. Diff = original
408a8b0..d4792b8(same as original PR). Do NOT merge; close after @claude review posts.