fix: harden setup, mqtt subscribe and CI actions - #49
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideThis PR hardens integration setup, MQTT subscription handling, entity data access, sensor time generation, and CI workflows to be more tolerant of legacy/corrupt data and external changes while pinning GitHub Actions to specific commits for reproducibility and security. Sequence diagram for hardened async_setup_entry email/username migrationsequenceDiagram
participant HomeAssistant
participant ConfigEntry
participant async_setup_entry
participant HonClient
HomeAssistant->>async_setup_entry: async_setup_entry(hass, entry)
async_setup_entry->>ConfigEntry: entry.data.get(email)
async_setup_entry->>ConfigEntry: entry.data.get(username)
alt email_missing_and_username_present
async_setup_entry->>HomeAssistant: hass.config_entries.async_update_entry(entry, data)
end
async_setup_entry->>HonClient: HonClient(email, password, refresh_token)
HonClient-->>async_setup_entry: client_created
async_setup_entry-->>HomeAssistant: True/False
Sequence diagram for MQTT _subscribe_topic fast-path handlingsequenceDiagram
participant MQTTClient
participant MQTTSocket
participant Future
MQTTClient->>MQTTSocket: send_packet(mqtt5.SubscribePacket)
MQTTSocket-->>Future: set_result()
MQTTClient->>MQTTClient: _subscribe_topic(topic)
alt future_done
MQTTClient->>Future: future.result()
else future_not_done
MQTTClient->>Future: asyncio.wrap_future(future)
MQTTClient->>Future: asyncio.wait_for(..., _SUBSCRIBE_TIMEOUT)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR pins two GitHub Actions in the CI workflow to fixed commit SHAs, and applies defensive fixes to the addhOn integration: config entry email/username migration fallback, coordinator data type guarding, an MQTT subscribe future fast-path, and a sensor timestamp fallback. ChangesRobustness and CI Fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="custom_components/addhon/sensor.py" line_range="1115-1122" />
<code_context>
def _now():
# Lazy dt import: keeps the test stubs (which import this module but never
# drive a coordinator update) free of a homeassistant.util.dt stub.
+ from datetime import UTC
+
from homeassistant.util import dt as dt_util
+ now = getattr(dt_util, "now", None)
+ if callable(now):
+ return now(UTC)
+ # Test stubs and older HA shims may only provide utcnow().
return dt_util.utcnow()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle dt_util.now implementations that don’t accept a timezone argument
There’s a corner case where `dt_util.now` exists but doesn’t accept a `tz` argument: `callable(now)` is true, but `now(UTC)` would raise `TypeError`, never reaching the utc fallback. To keep this robust across dt utility variants, wrap `now(UTC)` in `try`/`except TypeError` and fall back to `dt_util.utcnow()` on error.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| from datetime import UTC | ||
|
|
||
| from homeassistant.util import dt as dt_util | ||
|
|
||
| now = getattr(dt_util, "now", None) | ||
| if callable(now): | ||
| return now(UTC) | ||
| # Test stubs and older HA shims may only provide utcnow(). |
There was a problem hiding this comment.
suggestion (bug_risk): Handle dt_util.now implementations that don’t accept a timezone argument
There’s a corner case where dt_util.now exists but doesn’t accept a tz argument: callable(now) is true, but now(UTC) would raise TypeError, never reaching the utc fallback. To keep this robust across dt utility variants, wrap now(UTC) in try/except TypeError and fall back to dt_util.utcnow() on error.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| now = getattr(dt_util, "now", None) | ||
| if callable(now): | ||
| return now(UTC) | ||
| # Test stubs and older HA shims may only provide utcnow(). | ||
| return dt_util.utcnow() |
There was a problem hiding this comment.
callable(now) only verifies that the attribute exists and is callable — it does not verify the function's signature. A test stub that defines now as a zero-argument function (e.g. def now(): ...) would pass the callable check and then raise TypeError when called as now(UTC), bypassing the utcnow() fallback entirely. Wrapping the call in a try/except TypeError keeps the fallback reachable in all stub configurations.
| now = getattr(dt_util, "now", None) | |
| if callable(now): | |
| return now(UTC) | |
| # Test stubs and older HA shims may only provide utcnow(). | |
| return dt_util.utcnow() | |
| now = getattr(dt_util, "now", None) | |
| if callable(now): | |
| try: | |
| return now(UTC) | |
| except TypeError: | |
| pass | |
| # Test stubs and older HA shims may only provide utcnow(). | |
| return dt_util.utcnow() |
| if "email" not in entry.data and entry.data.get("username"): | ||
| hass.config_entries.async_update_entry( | ||
| entry, data={**entry.data, "email": email} | ||
| ) |
There was a problem hiding this comment.
"email" into the config entry but leaves the old "username" key in place. That stale key is harmless today, but any code that iterates entry.data keys or inspects them for diagnostics will see an unexpected key. Dropping "username" in the same update keeps the entry data clean.
| if "email" not in entry.data and entry.data.get("username"): | |
| hass.config_entries.async_update_entry( | |
| entry, data={**entry.data, "email": email} | |
| ) | |
| if "email" not in entry.data and entry.data.get("username"): | |
| migrated = {k: v for k, v in entry.data.items() if k != "username"} | |
| migrated["email"] = email | |
| hass.config_entries.async_update_entry(entry, data=migrated) |
Drop the getattr/callable fallback dance from the PR's _now(): production HA always exposes dt_util.now(), so a plain dt_util.now(UTC) is enough (still a tz-aware UTC datetime, as the TIMESTAMP device class requires). The only stub that drives HonLastRefreshSensor (test_debug_panel lifecycle tests) now provides dt.now instead of dt.utcnow, keeping the compatibility shim in the test rather than in production.
The username->email migration merged {**entry.data, "email": email}, which
left the stale "username" key in the config entry data. Rebuild the dict
without "username" in the same update so migrated entries carry only "email"
(no unexpected key for diagnostics/iteration). Addresses Greptile P2 on PR #49.
# Conflicts: # custom_components/addhon/__init__.py
2 participants
Summary by Sourcery
Harden setup, MQTT subscription handling, entity data access, and CI validation actions for the addhon integration.
Bug Fixes:
CI:
Summary by CodeRabbit
Greptile Summary
This PR applies targeted hardening fixes across setup, MQTT subscription, entity data access, and sensor timestamps, plus pins CI action SHAs to specific commits for reproducibility.
__init__.py: Recovers legacy config entries that stored the account identifier under"username"instead of"email", migrating the key in-place on first successful setup.base_entity.py: Guards_appliance_dataagainst non-dict coordinator payloads that can appear before the first successful refresh.mqtt.py: Fast-paths already-resolvedconcurrent.futures.Futureobjects to avoid a race wherewait_fortimes out before the event-loop callback can copy the result.sensor.py/tests/: Replaces the deprecateddt_util.utcnow()withdt_util.now(UTC)to return a timezone-aware datetime, which theTIMESTAMPdevice class requires; the test stub is updated in lock-step.Confidence Score: 5/5
All changes are well-scoped defensive fixes with no regressions introduced; safe to merge.
Each change addresses a concrete, narrow failure mode: legacy key migration, coordinator payload guard, MQTT future timing, and deprecated datetime API. The logic is straightforward, the test stub is updated in lock-step, and no new code paths introduce unexpected side effects. Previously flagged observations are minor and do not affect correctness of the happy path.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[async_setup_entry] --> B{entry.data has 'email'?} B -- Yes --> D[use email directly] B -- No --> C{entry.data has 'username'?} C -- Yes --> E[email = username value] C -- No --> F[email = None → return False] E --> G[migrate: add 'email' key to entry.data] G --> D D --> H[HonClient setup] H --> I[NativeMqttClient._subscribe_topic] I --> J{future.done?} J -- Yes --> K[future.result — raise if error] J -- No --> L[asyncio.wait_for wrap_future] L -- timeout --> M[raise HonCodedError] L -- success --> N[subscribed] K --> N H --> O[HonBaseEntity._appliance_data] O --> P{coordinator.data is dict?} P -- No --> Q[return empty dict] P -- Yes --> R{entry is dict?} R -- No --> Q R -- Yes --> S[return entry] H --> T[HonLastRefreshSensor._now] T --> U[dt_util.now UTC → tz-aware datetime]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[async_setup_entry] --> B{entry.data has 'email'?} B -- Yes --> D[use email directly] B -- No --> C{entry.data has 'username'?} C -- Yes --> E[email = username value] C -- No --> F[email = None → return False] E --> G[migrate: add 'email' key to entry.data] G --> D D --> H[HonClient setup] H --> I[NativeMqttClient._subscribe_topic] I --> J{future.done?} J -- Yes --> K[future.result — raise if error] J -- No --> L[asyncio.wait_for wrap_future] L -- timeout --> M[raise HonCodedError] L -- success --> N[subscribed] K --> N H --> O[HonBaseEntity._appliance_data] O --> P{coordinator.data is dict?} P -- No --> Q[return empty dict] P -- Yes --> R{entry is dict?} R -- No --> Q R -- Yes --> S[return entry] H --> T[HonLastRefreshSensor._now] T --> U[dt_util.now UTC → tz-aware datetime]Comments Outside Diff (1)
custom_components/addhon/client/transport/mqtt.py, line 457-464 (link)HonCodedErrorwrapping on the fast pathWhen
future.done()isTrueand the future completed with an error (e.g. an awscrt transport failure),future.result()re-raises that exception directly. Because onlyasyncio.TimeoutErroris caught in thistryblock, the raw awscrt exception propagates to_subscribe_missing's genericexcept Exceptionhandler and is logged as a warning — which is the right outcome, but the exception is no longer wrapped inHonCodedErroras it would be through the slow path. If callers or logging consumers depend onHonCodedErrorfor error attribution (e.g. metrics or structured error codes), the fast-path bypasses that contract.Reviews (2): Last reviewed commit: "fix(sensor): simplify _now() to dt_util...." | Re-trigger Greptile