feat(coordinator): Phase 3.1 commit 4 — daily ranking job hook#68
Merged
Conversation
Wires the multi-plan ranking pipeline (from PR #64) into the coordinator on a daily 00:30 local schedule. Cheap-rank only for now; deep-rank (consumption replay) joins in Phase 3.2 when the universal HA-history backfill ships. Architecture ------------ Pure logic in new ``custom_components/pricehawk/cdr/ranking_job.py``: - ``get_user_geography(options)`` — extracts ``(state, postcode, distributor)`` from a config_entry's options. ``distributor`` comes from the user's current cdr_plan's ``geography.distributors[0]`` (the plan the user already accepted, so its distributor is by definition theirs). - ``get_competitor_retailers(session, options, *, competitor_fragments)`` — returns the retailer list scanned daily. Composition: user's CURRENT retailer (via ``cdr_plan.data.brand``) + hardcoded big-4 nationally-active competitors (AGL, Origin, EnergyAustralia, Red Energy). Dedup by brand_id so the user's retailer isn't double-scanned when it's also a big-4. Custom fragment tuple supported for tests + future user-configurable list. - ``run_ranking_job(session, options, *, top_k, plan_cache, competitor_fragments)`` — top-level orchestrator. Returns the cheap-ranked top-K plans. Coordinator-side wrappers ------------------------- - New attributes on PriceHawkCoordinator: - ``_cheap_ranked_alternatives: list[dict]`` - ``_ranking_last_run_at: datetime | None`` - ``_ranking_plan_cache: dict[str, dict]`` - ``_ranking_unsub: CALLBACK_TYPE | None`` - ``schedule_daily_ranking()`` registers ``async_track_time_change`` for ``hour=0, minute=30`` local. Safe to call twice (second call replaces first). - ``cancel_ranking()`` clears the handle. - ``async_run_ranking_job(*, top_k)`` is a thin delegate around ``ranking_job.run_ranking_job``. Owns HA-side concerns: session acquisition, exception swallowing across the daily boundary, state persistence, cache reset. Empty-result runs keep prior ranking (transient failure → don't zero yesterday's data). Why a separate module --------------------- PriceHawkCoordinator inherits from ``DataUpdateCoordinator[dict[str, Any]]`` which gets mocked away by ``tests/conftest.py``. The class is therefore unreachable in unit tests. Extracting the pure logic to ``ranking_job`` keeps it unit-testable while the class methods stay as thin delegates. Tests ----- 17 new tests in ``tests/test_coordinator_ranking.py``: - Module constants (competitor fragments, run-time). - ``get_user_geography``: 6 cases (happy path, missing fields, list ordering, empty options). - ``get_competitor_retailers``: 5 cases (current retailer first, dedup, missing brand, unmatchable fragment, custom override). - ``run_ranking_job``: 4 cases (empty retailers, geography forwarding, cache passthrough, top_k forwarding). Also added ``homeassistant.helpers.aiohttp_client`` to ``conftest.py`` mock list — coordinator.py imports it; without the mock, importing the coordinator silently fell back to a MagicMock class. Full suite: 713/713 pass (was 696 — +17 new). Ruff clean. Refs PHASE-3-ROADMAP.md §3.1. Next commit: HA service ``pricehawk.rank_alternatives`` in ``__init__.py``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideWires the previously standalone ranking foundation into the coordinator via a daily 00:30 scheduled job, implementing HA-independent orchestration logic in a new ranking_job module, adding coordinator state and wrappers for scheduling and execution, and covering the behavior with unit tests for geography, retailer selection, and ranking job orchestration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/test*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (3)
WalkthroughAdds Phase 3.1 daily multi-plan ranking: a new ranking_job module extracts user geography, builds prioritized/deduped retailer lists, and calls rank_alternatives. The coordinator schedules daily runs at 00:30, caches per-plan data with day rollover, persists top-K results, and handles failures safely. Tests validate parsing, composition, and orchestration. ChangesPhase 3.1 Daily Ranking
Sequence DiagramsequenceDiagram
participant Coordinator as PriceHawkCoordinator
participant RankingJob as custom_components.pricehawk.cdr.ranking_job
participant Registry as RetailerRegistry (get_registry)
participant RankService as rank_alternatives
Coordinator->>RankingJob: run_ranking_job(session, options, top_k, plan_cache)
RankingJob->>RankingJob: get_user_geography(options)
RankingJob->>Registry: get_registry(session)
Registry-->>RankingJob: retailer endpoints
RankingJob->>RankingJob: get_competitor_retailers(endpoints, fragments)
RankingJob->>RankService: rank_alternatives(state, postcode, distributor, top_k, cache)
RankService-->>RankingJob: ranked alternatives
RankingJob-->>Coordinator: results or []
alt Non-empty results
Coordinator->>Coordinator: persist alternatives, update last_run
else Empty or exception
Coordinator->>Coordinator: log, return prior cached alternatives
end
Coordinator->>Coordinator: clear plan_cache on day rollover
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 issue, and left some high level feedback:
- In the tests you repeatedly use
asyncio.run(...)to invoke async functions; consider marking the tests as async (e.g. viapytest.mark.asyncioor HA’s async test helpers) andawaiting directly to avoid potential event-loop nesting issues and to better match how the code runs under Home Assistant. - In
async_run_ranking_job,_ranking_last_run_atis only updated whenrankedis non-empty, so a run that successfully completes but yields an empty result will appear as if it never ran; if that’s not intentional, consider updating the timestamp whenever the pipeline finishes without raising, regardless of whether any alternatives were found.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the tests you repeatedly use `asyncio.run(...)` to invoke async functions; consider marking the tests as async (e.g. via `pytest.mark.asyncio` or HA’s async test helpers) and `await`ing directly to avoid potential event-loop nesting issues and to better match how the code runs under Home Assistant.
- In `async_run_ranking_job`, `_ranking_last_run_at` is only updated when `ranked` is non-empty, so a run that successfully completes but yields an empty result will appear as if it never ran; if that’s not intentional, consider updating the timestamp whenever the pipeline finishes without raising, regardless of whether any alternatives were found.
## Individual Comments
### Comment 1
<location path="custom_components/pricehawk/coordinator.py" line_range="1250-1252" />
<code_context>
+ _LOGGER.info(
+ "ranking: persisted %d alternative(s)", len(ranked),
+ )
+ # Reset the cache for tomorrow's run — plans may republish
+ # overnight and we want fresh data daily.
+ self._ranking_plan_cache.clear()
+ return ranked or self._cheap_ranked_alternatives
+
</code_context>
<issue_to_address>
**issue:** Cache clearing semantics conflict with the stated per-day TTL and reduce reuse for same-day manual runs.
Clearing `_ranking_plan_cache` after every successful run means same-day manual reruns won’t reuse cached plans; the effective TTL is per run, not per day. If the intent is same-day reuse (scheduled + manual), either keep the cache across runs and only clear it when the date changes, or update the docstring to describe a per-run cache so behavior and documentation match.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@custom_components/pricehawk/cdr/ranking_job.py`:
- Around line 54-56: The code assumes geo.get("distributors") is a list and does
distributors[0] unconditionally; validate the type first to avoid using a
string/dict as a distributor. Update the block around distributors =
geo.get("distributors") or [] so you only pick distributors[0] when
isinstance(distributors, list) and len(distributors) > 0, otherwise set
distributor = None (or handle a dict case explicitly if your domain requires
it). Ensure the change affects the return tuple (currently return None,
postcode, distributor) so malformed distributor values are not propagated into
ranking filters.
In `@custom_components/pricehawk/coordinator.py`:
- Around line 1250-1253: The code currently unconditionally calls
self._ranking_plan_cache.clear() at the end of the ranking run which prevents
subsequent async_run_ranking_job invocations from reusing cached plan details;
remove that unconditional clear and instead only clear the cache when the
calendar day rolls over (or via an explicit cache invalidation API). Concretely:
delete the self._ranking_plan_cache.clear() call in the function that returns
"ranked or self._cheap_ranked_alternatives", add a persistent marker (e.g.
self._last_ranking_date) updated to date.today() each successful run in
async_run_ranking_job, and only call self._ranking_plan_cache.clear() when
date.today() != self._last_ranking_date (or expose a clear_ranking_plan_cache()
method invoked by a daily scheduler). Update code to still return "ranked or
self._cheap_ranked_alternatives" as before.
🪄 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: 5b29680b-c8c2-4ec2-bad1-1e2335d766d0
📒 Files selected for processing (4)
custom_components/pricehawk/cdr/ranking_job.pycustom_components/pricehawk/coordinator.pytests/conftest.pytests/test_coordinator_ranking.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeRabbit Review Gate
🧰 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/conftest.pytests/test_coordinator_ranking.pycustom_components/pricehawk/cdr/ranking_job.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.pytests/test_coordinator_ranking.pycustom_components/pricehawk/cdr/ranking_job.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_ranking.py
🔇 Additional comments (4)
custom_components/pricehawk/cdr/ranking_job.py (1)
1-53: LGTM!Also applies to: 59-143
custom_components/pricehawk/coordinator.py (1)
16-72: LGTM!Also applies to: 256-269, 1183-1249
tests/conftest.py (1)
23-23: LGTM!Also applies to: 36-36
tests/test_coordinator_ranking.py (1)
1-257: LGTM!
This was referenced May 16, 2026
Two findings from CodeRabbit and Sourcery on PR #68 (coordinator hook). cache TTL (Sourcery + CR Major, same issue): - coordinator.py — replaced unconditional ``cache.clear()`` at end of ``async_run_ranking_job`` with a date-rollover check at the start. Same-day reruns (scheduled at 00:30, manual via rank_alternatives service) now reuse cached plans as the docstring promised. New local day → cache clears before the run so overnight republished plans get fresh data. - New ``_ranking_cache_date: date | None`` attribute tracks the cache's "as-of" day. distributors type guard (CR Minor): - ranking_job.py — ``get_user_geography`` now type-guards ``distributors`` with ``isinstance(..., list)`` before indexing. Malformed CDR payload could ship distributors as a string (e.g. ``"United Energy"``) and ``[0]`` would silently become ``"U"`` — skewing the ranking filter without raising. Tests: 18/18 ranking tests pass (was 17 + 1 new for type guard). Full suite 714/714. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@custom_components/pricehawk/cdr/ranking_job.py`:
- Around line 51-53: The code reads nested fields from options/cdr_plan
(variables cdr_plan, plan_data, geo and brand) without verifying types, which
can raise AttributeError for malformed payloads; fix by adding explicit type
guards (e.g., if isinstance(cdr_plan, dict): plan_data = cdr_plan.get("data") if
isinstance(cdr_plan.get("data"), dict) else {}; else: plan_data = {}; similarly
ensure geography is a dict before using .get and ensure brand is a str before
calling .strip(), falling back to safe defaults), update any public function
signatures in this module to include type hints for these inputs, and replace
any bare excepts around parsing with specific exceptions so malformed inputs are
handled gracefully and do not abort the ranking run.
In `@custom_components/pricehawk/coordinator.py`:
- Around line 1216-1263: async_run_ranking_job can run concurrently and corrupt
_ranking_plan_cache; fix by serializing runs with an asyncio.Lock: add a
persistent instance attribute (e.g. self._ranking_lock = asyncio.Lock()) during
coordinator init, then wrap the body of async_run_ranking_job (including the
today check, cache clear, calling run_ranking_job, and updating
_cheap_ranked_alternatives/_ranking_last_run_at) in an "async with
self._ranking_lock:" block so only one execution mutates _ranking_plan_cache at
a time and other invocations await completion.
In `@tests/test_coordinator_ranking.py`:
- Around line 101-109: Extend the test_non_list_distributors_safely_skipped test
to also assert that get_user_geography returns None for malformed nested shapes
by adding cases where cdr_plan.data is not a dict (e.g., string/int/list) and
where cdr_plan.data.geography is not a dict (e.g., string/int/list); for each
case construct opts like {"cdr_plan": <bad>} and {"cdr_plan": {"data": <bad>}}
respectively, call get_user_geography(opts) and assert the returned distributor
is None so the nested-shape guards around cdr_plan.data and geography are
covered.
🪄 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: 15c014ef-6b0f-4faf-8cc6-3fee7ed4246d
📒 Files selected for processing (3)
custom_components/pricehawk/cdr/ranking_job.pycustom_components/pricehawk/coordinator.pytests/test_coordinator_ranking.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_coordinator_ranking.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/cdr/ranking_job.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_coordinator_ranking.pycustom_components/pricehawk/coordinator.pycustom_components/pricehawk/cdr/ranking_job.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_ranking.py
3 tasks
Artic0din
added a commit
that referenced
this pull request
May 16, 2026
Three CodeRabbit findings on the round-1 fixes (f998d16): 1. ranking_job.py — extracted ``_safe_plan_data(options)`` helper that walks ``cdr_plan["data"]`` with isinstance gates at every level. Previously ``cdr_plan["data"]`` was assumed to be a dict; a malformed payload could ship it as a string (e.g. ``cdr_plan = {"data": "broken"}``) and trigger AttributeError on ``.get("geography")``. Same for ``brand`` — now ``isinstance(..., str)`` before ``.strip()``. ``geography`` non-dict also guarded in ``get_user_geography``. First-distributor-must-be-str check added to prevent dict / int from sneaking through as a distributor filter value. 2. coordinator.py — added ``self._ranking_lock = asyncio.Lock()`` to serialise concurrent runs. Scheduled 00:30 callback + manual service trigger could enter ``async_run_ranking_job`` simultaneously, interleaving ``_ranking_plan_cache`` mutations and duplicating every expensive CDR detail fetch. The lock means the second caller blocks briefly then returns the freshly-populated cache results. 3. tests/test_coordinator_ranking.py — added 4 new tests for non-dict ``cdr_plan``, non-dict ``data``, non-dict ``geography``, and non-string first distributor. Covers the AttributeError regression paths CR flagged. Tests: 22/22 ranking + 722/722 full suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three CodeRabbit findings on the round-1 fixes (f998d16): 1. ranking_job.py — extracted ``_safe_plan_data(options)`` helper that walks ``cdr_plan["data"]`` with isinstance gates at every level. Previously ``cdr_plan["data"]`` was assumed to be a dict; a malformed payload could ship it as a string (e.g. ``cdr_plan = {"data": "broken"}``) and trigger AttributeError on ``.get("geography")``. Same for ``brand`` — now ``isinstance(..., str)`` before ``.strip()``. ``geography`` non-dict also guarded in ``get_user_geography``. First-distributor-must-be-str check added to prevent dict / int from sneaking through as a distributor filter value. 2. coordinator.py — added ``self._ranking_lock = asyncio.Lock()`` to serialise concurrent runs. Scheduled 00:30 callback + manual service trigger could enter ``async_run_ranking_job`` simultaneously, interleaving ``_ranking_plan_cache`` mutations and duplicating every expensive CDR detail fetch. The lock means the second caller blocks briefly then returns the freshly-populated cache results. 3. tests/test_coordinator_ranking.py — added 4 new tests for non-dict ``cdr_plan``, non-dict ``data``, non-dict ``geography``, and non-string first distributor. Covers the AttributeError regression paths CR flagged. Tests: 22/22 ranking + 722/722 full suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Artic0din
added a commit
that referenced
this pull request
May 17, 2026
…ifecycle (#72) * feat(coordinator): Phase 3.1 commit 4 — daily ranking job hook Wires the multi-plan ranking pipeline (from PR #64) into the coordinator on a daily 00:30 local schedule. Cheap-rank only for now; deep-rank (consumption replay) joins in Phase 3.2 when the universal HA-history backfill ships. Architecture ------------ Pure logic in new ``custom_components/pricehawk/cdr/ranking_job.py``: - ``get_user_geography(options)`` — extracts ``(state, postcode, distributor)`` from a config_entry's options. ``distributor`` comes from the user's current cdr_plan's ``geography.distributors[0]`` (the plan the user already accepted, so its distributor is by definition theirs). - ``get_competitor_retailers(session, options, *, competitor_fragments)`` — returns the retailer list scanned daily. Composition: user's CURRENT retailer (via ``cdr_plan.data.brand``) + hardcoded big-4 nationally-active competitors (AGL, Origin, EnergyAustralia, Red Energy). Dedup by brand_id so the user's retailer isn't double-scanned when it's also a big-4. Custom fragment tuple supported for tests + future user-configurable list. - ``run_ranking_job(session, options, *, top_k, plan_cache, competitor_fragments)`` — top-level orchestrator. Returns the cheap-ranked top-K plans. Coordinator-side wrappers ------------------------- - New attributes on PriceHawkCoordinator: - ``_cheap_ranked_alternatives: list[dict]`` - ``_ranking_last_run_at: datetime | None`` - ``_ranking_plan_cache: dict[str, dict]`` - ``_ranking_unsub: CALLBACK_TYPE | None`` - ``schedule_daily_ranking()`` registers ``async_track_time_change`` for ``hour=0, minute=30`` local. Safe to call twice (second call replaces first). - ``cancel_ranking()`` clears the handle. - ``async_run_ranking_job(*, top_k)`` is a thin delegate around ``ranking_job.run_ranking_job``. Owns HA-side concerns: session acquisition, exception swallowing across the daily boundary, state persistence, cache reset. Empty-result runs keep prior ranking (transient failure → don't zero yesterday's data). Why a separate module --------------------- PriceHawkCoordinator inherits from ``DataUpdateCoordinator[dict[str, Any]]`` which gets mocked away by ``tests/conftest.py``. The class is therefore unreachable in unit tests. Extracting the pure logic to ``ranking_job`` keeps it unit-testable while the class methods stay as thin delegates. Tests ----- 17 new tests in ``tests/test_coordinator_ranking.py``: - Module constants (competitor fragments, run-time). - ``get_user_geography``: 6 cases (happy path, missing fields, list ordering, empty options). - ``get_competitor_retailers``: 5 cases (current retailer first, dedup, missing brand, unmatchable fragment, custom override). - ``run_ranking_job``: 4 cases (empty retailers, geography forwarding, cache passthrough, top_k forwarding). Also added ``homeassistant.helpers.aiohttp_client`` to ``conftest.py`` mock list — coordinator.py imports it; without the mock, importing the coordinator silently fell back to a MagicMock class. Full suite: 713/713 pass (was 696 — +17 new). Ruff clean. Refs PHASE-3-ROADMAP.md §3.1. Next commit: HA service ``pricehawk.rank_alternatives`` in ``__init__.py``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: PR #68 CR findings — cache TTL + distributors type guard Two findings from CodeRabbit and Sourcery on PR #68 (coordinator hook). cache TTL (Sourcery + CR Major, same issue): - coordinator.py — replaced unconditional ``cache.clear()`` at end of ``async_run_ranking_job`` with a date-rollover check at the start. Same-day reruns (scheduled at 00:30, manual via rank_alternatives service) now reuse cached plans as the docstring promised. New local day → cache clears before the run so overnight republished plans get fresh data. - New ``_ranking_cache_date: date | None`` attribute tracks the cache's "as-of" day. distributors type guard (CR Minor): - ranking_job.py — ``get_user_geography`` now type-guards ``distributors`` with ``isinstance(..., list)`` before indexing. Malformed CDR payload could ship distributors as a string (e.g. ``"United Energy"``) and ``[0]`` would silently become ``"U"`` — skewing the ranking filter without raising. Tests: 18/18 ranking tests pass (was 17 + 1 new for type guard). Full suite 714/714. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: PR #68 round-2 CR — nested shape guards + asyncio.Lock Three CodeRabbit findings on the round-1 fixes (f998d16): 1. ranking_job.py — extracted ``_safe_plan_data(options)`` helper that walks ``cdr_plan["data"]`` with isinstance gates at every level. Previously ``cdr_plan["data"]`` was assumed to be a dict; a malformed payload could ship it as a string (e.g. ``cdr_plan = {"data": "broken"}``) and trigger AttributeError on ``.get("geography")``. Same for ``brand`` — now ``isinstance(..., str)`` before ``.strip()``. ``geography`` non-dict also guarded in ``get_user_geography``. First-distributor-must-be-str check added to prevent dict / int from sneaking through as a distributor filter value. 2. coordinator.py — added ``self._ranking_lock = asyncio.Lock()`` to serialise concurrent runs. Scheduled 00:30 callback + manual service trigger could enter ``async_run_ranking_job`` simultaneously, interleaving ``_ranking_plan_cache`` mutations and duplicating every expensive CDR detail fetch. The lock means the second caller blocks briefly then returns the freshly-populated cache results. 3. tests/test_coordinator_ranking.py — added 4 new tests for non-dict ``cdr_plan``, non-dict ``data``, non-dict ``geography``, and non-string first distributor. Covers the AttributeError regression paths CR flagged. Tests: 22/22 ranking + 722/722 full suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(integration): Phase 3.1 commit 5 — rank_alternatives HA service + lifecycle Wires the ranking job into the integration's lifecycle and exposes a manual-trigger HA service. Completes Phase 3.1's user-visible plumbing (sensor + dashboard exposure is commit 6). Lifecycle wiring ---------------- - ``async_setup_entry`` now calls ``coordinator.schedule_daily_ranking()`` to register the 00:30 local-time callback, plus ``hass.async_create_task`` for an immediate first run so the alternatives data isn't empty until midnight on a fresh install. - ``async_unload_entry`` calls ``coordinator.cancel_ranking()`` mirroring the existing ``cancel_persist()`` pattern. HA service: pricehawk.rank_alternatives --------------------------------------- - ``services.yaml`` declares the service with a single ``top_k`` int field (1-100, default 20). - Handler clamps top_k to [1, 100] and delegates to ``coordinator.async_run_ranking_job(top_k=...)``. - Most useful right after a plan switch: lets the user force-run the ranking pipeline so the alternatives sensor reflects the new distributor / postcode without waiting for the next 00:30 schedule. - Unregistered alongside the other two services in ``async_unload_entry`` when the last config entry goes away. No tests -------- The service handler is a closure inside ``async_setup_entry`` (tightly coupled to HA's service registration plumbing — can't be unit-tested without HA itself). The underlying ``async_run_ranking_job`` is already covered by ``tests/test_coordinator_ranking.py`` (17 tests via the ``ranking_job`` module). Live smoke test will validate the wiring once the integration runs in HA. Full suite: 713/713 still passing. Ruff clean. services.yaml validates. Refs PHASE-3-ROADMAP.md §3.1. Next (optional commit 6): sensor for ranked alternatives + dashboard exposure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: PR #69 CR finding — handle invalid top_k payload defensively CR found that ``int(call.data.get("top_k", 20))`` raises ValueError/TypeError on malformed service data (e.g. a YAML typo sending ``top_k: "abc"``). The exception propagates back to the service caller and fails the run instead of degrading gracefully. Now wraps the coercion in try/except, logs a warning with the bad value, and falls back to the default 20. Clamp [1, 100] still applied after. Same kind of defence as elsewhere in the codebase (``safe_int``, ``safe_decimal`` for incentive parsers). Tests: 714/714 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
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
Commit 4 of Phase 3.1. Wires the ranking foundation (PR #64) into the coordinator on a daily 00:30 local schedule. Cheap-rank only; deep-rank deferred to Phase 3.2 (needs HA-history backfill).
What was broken
The ranking foundation (
cdr/ranking.py) was merged in #64 but had no consumer. Nothing scheduled it; nothing persisted results. Just inert library code.What this fixes
schedule_daily_ranking()registersasync_track_time_changeat 00:30 local. Daily job runs after midnight rollover so today's daily_cost_history is final.async_run_ranking_job()orchestratesget_competitor_retailers→rank_alternativesand persists top-K tocoordinator._cheap_ranked_alternatives.planIdso daily reruns skip unchanged plans.Still not consumer-visible — sensor wiring + lifecycle invocation come in commits 5-6.
Test plan
pytest tests/test_coordinator_ranking.py -v— 17/17 passpytest -q— 713/713 full suite pass (was 696 + 17 new)ruff check— cleanChanges
New files
custom_components/pricehawk/cdr/ranking_job.pytests/test_coordinator_ranking.pyModified
custom_components/pricehawk/coordinator.pytests/conftest.pyhomeassistant.helpers.aiohttp_clientCoordinator-side additions
Why
Per
.planning/PHASE-3-ROADMAP.md§3.1. The roadmap calls for both cheap-rank and deep-rank in Phase 3.1, but deep-rank needs HA-history backfill (Phase 3.2) to have meaningful consumption to rank against. Shipping cheap-rank-only now means users see ranked alternatives by daily supply + peak rate immediately; deep-rank slots in as a refinement once Phase 3.2 lands.Pure logic lives in
cdr/ranking_job.py(not coordinator.py) becausePriceHawkCoordinatorinherits fromDataUpdateCoordinator[dict[str, Any]]which gets mocked away by conftest. Class is unreachable in tests. Module functions stay testable; class methods are thin delegates.Breaking Changes
None. Pure additive on coordinator — no existing attribute or method touched. Schedule isn't wired to lifecycle yet (commit 5), so runtime behavior unchanged on
dev.Files Changed
custom_components/pricehawk/cdr/ranking_job.py(+152, new)custom_components/pricehawk/coordinator.py(+84, -1)tests/test_coordinator_ranking.py(+247, new)tests/conftest.py(+2)🤖 Generated with Claude Code
Summary by Sourcery
Integrate a daily multi-plan ranking job into the coordinator and introduce a testable orchestration layer for ranking competitors based on user geography and configuration.
New Features:
Enhancements:
Tests:
What changed
Why
Breaking changes
Files changed