feat(integration): Phase 3.1 commit 5 — rank_alternatives service + lifecycle#69
Closed
Artic0din wants to merge 2 commits into
Closed
feat(integration): Phase 3.1 commit 5 — rank_alternatives service + lifecycle#69Artic0din wants to merge 2 commits into
Artic0din wants to merge 2 commits into
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideWires the daily ranking job into the integration lifecycle and exposes a new Home Assistant service to trigger ranking on demand, plus matching teardown/unregister logic. 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 service handler
handle_rank_alternativescurrently does a blindint(call.data.get("top_k", 20)); consider validating/coercing the value more defensively (e.g., handleValueErrorand non-numeric input) so a malformed call doesn’t raise unexpectedly. - You type
callasobjectin the new service handler; using the concretehomeassistant.core.ServiceCalltype here (as done elsewhere in HA code) would make the interface clearer and help static analysis.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The service handler `handle_rank_alternatives` currently does a blind `int(call.data.get("top_k", 20))`; consider validating/coercing the value more defensively (e.g., handle `ValueError` and non-numeric input) so a malformed call doesn’t raise unexpectedly.
- You type `call` as `object` in the new service handler; using the concrete `homeassistant.core.ServiceCall` type here (as done elsewhere in HA code) would make the interface clearer and help static analysis.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: 1
🤖 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/__init__.py`:
- Around line 193-195: handle_rank_alternatives currently casts
call.data.get("top_k", 20) to int directly which can raise ValueError/TypeError
for malformed payloads; validate and sanitize the raw value from call.data
before casting (e.g., check for None, numeric string, or int), perform a safe
int conversion within a try/except that catches ValueError and TypeError, and on
error fall back to the default (20) or return a clear service error; ensure the
bounded logic (max/min) still uses the sanitized integer and reference the
handle_rank_alternatives function, the top_k variable and call.data when making
the fix.
🪄 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: 38ab215f-fb32-4dca-a376-57f3a174a08d
📒 Files selected for processing (2)
custom_components/pricehawk/__init__.pycustom_components/pricehawk/services.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useasync/awaitfor all I/O operations
NEVER hardcode tokens, API keys, or credentials in any file — use HA config entry storage
State restore MUST validate storage version before loading
from_dict() methods MUST receive an explicit HA-timezone date — no date.today() fallback
Files:
custom_components/pricehawk/__init__.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/__init__.py
🔇 Additional comments (2)
custom_components/pricehawk/services.yaml (1)
34-53: LGTM!custom_components/pricehawk/__init__.py (1)
44-49: LGTM!Also applies to: 218-227
5 tasks
Artic0din
added a commit
that referenced
this pull request
May 16, 2026
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>
Artic0din
force-pushed
the
phase-3-1-rank-service
branch
from
1aff3eb to
6f1f923
Compare
…+ 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>
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>
Artic0din
force-pushed
the
phase-3-1-rank-service
branch
from
6f1f923 to
d022187
Compare
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 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 5 of Phase 3.1. Stacks on #68 (coordinator hook). Wires the daily ranking job into integration setup/teardown and adds a manual-trigger HA service.
What was broken
Commit 4 (#68) added
coordinator.schedule_daily_ranking()andasync_run_ranking_job()but nothing called them. The methods existed; the lifecycle didn't invoke them. Equally, there was no way for a user to force-run the ranking job after a plan switch — they'd have to wait until the next 00:30 fire.What this fixes
async_setup_entrynow invokesschedule_daily_ranking()+ a one-shotasync_create_task(async_run_ranking_job())so the alternatives data populates immediately on fresh install rather than being empty until midnight.async_unload_entrycallscancel_ranking()mirroring the existingcancel_persist()pattern.pricehawk.rank_alternativeslets users force-run the pipeline from Developer Tools → Services. Singletop_kfield (1-100, default 20) clamped server-side.Test plan
ruff check— cleanpython -c "import yaml; yaml.safe_load(...)"services.yaml validatespytest -q— 713/713 full suite still passes (no regression)_LOGGERshows "rank_alternatives service: ran successfully, N result(s)" — deferred to user runtime00:30callback fires (will be visible in log) — deferredChanges
custom_components/pricehawk/__init__.pycustom_components/pricehawk/services.yamlService shape
Lifecycle additions
Why
No tests for the service handler itself: it's a closure inside
async_setup_entry(tightly coupled to HA'shass.services.async_registerplumbing — can't unit-test without HA itself). The underlyingasync_run_ranking_joband its pure-logic backing (ranking_job.run_ranking_job) are already covered by 17 tests in #68. The handler is just arg coercion + delegation.Breaking Changes
None. Additive — no existing service changed, no existing lifecycle method touched.
Files Changed
custom_components/pricehawk/__init__.py(+25)custom_components/pricehawk/services.yaml(+22)Stacked on
Depends on #68 — must merge that PR first or this PR's diff will look messy after rebase. Open this PR against
phase-3-1-coordinator-hook(#68's branch) so the diff stays clean. Will retarget todevafter #68 merges.🤖 Generated with Claude Code
Summary by Sourcery
Integrate the daily alternatives ranking job into the PriceHawk integration lifecycle and expose a manual trigger service in Home Assistant.
New Features:
pricehawk.rank_alternativesto manually trigger the alternatives ranking pipeline with a configurabletop_kparameter.Enhancements:
rank_alternativesservice is unregistered when the integration is unloaded to align with existing lifecycle cleanup.Summary
Changes
Integrated daily ranking job into lifecycle:
async_setup_entrynow callscoordinator.schedule_daily_ranking()and immediately triggers a ranking run viahass.async_create_task(coordinator.async_run_ranking_job())to populate alternatives data on fresh install.Added cleanup on unload:
async_unload_entrynow callscoordinator.cancel_ranking()to mirror existing cancel_persist() cleanup pattern.Exposed manual ranking service: New
rank_alternativesHome Assistant service allows triggering the ranking job manually. The service acceptstop_kparameter (integer, range 1–100, default 20) which is clamped server-side and passed tocoordinator.async_run_ranking_job(top_k=...).Service lifecycle management: The
rank_alternativesservice is automatically unregistered during last-entry teardown alongside existing services.Why
This change completes the integration of the alternatives ranking pipeline into the Home Assistant lifecycle, ensuring:
Breaking Changes
None. All changes are additive with no modifications to existing APIs or function signatures.
Files Changed