Conversation
Added sections for two-factor authentication and multilingual integration. Removed outdated localization and how it works sections.
The "Added program select", "Added pause switch" and the two legacy entity-removal INFO logs emitted the appliance nickname / entity_id (whose object_id is the nickname slug) in clear. INFO is not gated by the debug toggles, so it always reaches home-assistant.log and gets attached to public GitHub issues. Log the redacted id (redact_id) instead. - select.py / switch.py: log redact_id(appliance_id) - __init__.py: import redact_id; both removal logs use redact_id(unique_id) - behavioral tests assert the nickname/entity_id never reach the INFO log (mutation-proven); relax the brittle source-substring import guard to a regex
hvac_mode/fan_mode coerced an unmapped or out-of-enum raw value into a guessed COOL/auto, which could fall outside the advertised _attr_hvac_modes / _attr_fan_modes; HA then logs "X is not a valid option" and ignores it. Return None (unknown) when the resolved value is unmapped or not in the advertised list. The full-list fallback keeps the common, healthy-device case unchanged. - climate.py: hvac_mode -> HVACMode | None, fan_mode None-guards on the advertised list - read-side tests for unmapped / mapped-but-not-advertised values (mutation-proven)
The static guard caught only bare Name/Attribute identity refs, so it missed
the v5.2.0/2026-06-25 leak forms: `data.get("name")` (an ast.Call) at INFO and
a bare `reg_entry.entity_id` in __init__.py (which was not even registered).
- register __init__.py in _FILES, forbidding bare .entity_id / .unique_id in the
legacy-cleanup logs
- new _identity_call_offender: flag `<x>.get("name"/nick_name/nickName/nickname)`
and `_get_name(...)` passed to a NON-gated logger call (info/warning/error/
exception/critical); the same at .debug stays allowed (debug is gated)
- add the camelCase `nickName` attribute to the forbidden entity attrs (_get_name
reads both nick_name and nickName via getattr)
- two meta-tests pin the new check and its level gate (non-vacuous)
… stale REST lastConnEvent An appliance was marked unavailable whenever the 60s REST poll's lastConnEvent.category was DISCONNECTED, even while it was streaming realtime MQTT appliancestatus updates. The Haier cloud can leave lastConnEvent frozen on an old DISCONNECTED for hours (observed: ~2.5h) while the appliance is online, so HA showed the device offline although the hOn app showed it connected (discussion #34, reproduced with a diagnostics dump + debug log). Treat realtime MQTT traffic as authoritative connectivity evidence, reconciled against the REST lastConnEvent by timestamp: - mark_realtime_seen(): an appliancestatus message marks the appliance connected (mirrored onto the raw `available` attribute the connectivity binary_sensor and the availability gate read, so it flips at once, not at the next poll) and records the cloud message time plus a local receipt time. - load_attributes reconciliation: a DISCONNECTED lastConnEvent downgrades to offline only when the disconnect event is NEWER than the last realtime message (cloud-vs-cloud ordering) AND that message is still within a wall-clock freshness window (_REALTIME_LIVENESS_TTL = 300s). The freshness bound prevents a once-seen realtime time from pinning a silently-dead appliance online forever while the cloud keeps returning a frozen-stale disconnect. Missing/unparseable timestamps fall back to honoring the DISCONNECTED. - mark_realtime_disconnected(): a device-scoped MQTT `disconnected` clears the liveness marks so a stale REST disconnect cannot resurrect the appliance. - Topic dispatch: AWS session-presence events ($aws/events/presence/...) are OUR client's session, not appliance connectivity, so they neither arm nor clear the protection (our client reconnecting/dropping must not pin a dead appliance online nor knock a live one offline). Only device-scoped events drive it. - parse_cloud_timestamp(): defensive, total parser for the two cloud time shapes (epoch-ms and ISO8601 with Z); never raises, including on huge ints. Type-agnostic (the original report was a dryer, the repro a washer). Adds unit tests for the reconciliation, the freshness/stuck-online guard, the explicit disconnect, session-presence discrimination, and parser totality.
…poll Adds a domain-wide service/action `addhon.refresh`, the automation-callable equivalent of the per-device "Refresh now" button (requested by a user in discussion #34: "an action, service or button to trigger such polling"). The button can only be pressed by hand from the UI; the service can be called from automations, scripts and Developer Tools. The handler reads hass.data live at call time, collects every loaded entry's coordinator and asks each for a debounced refresh (async_request_refresh, like the button), running them concurrently. Per-entry failures are isolated: each call is wrapped in a coroutine and gathered with return_exceptions=True, so a synchronous OR awaited raise from one account is logged at warning and never re-raised to the caller nor aborts the other accounts. The service is global to the domain: registered once (idempotent guard) and removed on the last unload, alongside the existing log-level services. No fields, no target. Adds services.yaml entry, en/it translations, and tests (registration, idempotency, refresh-all, per-entry isolation including a synchronous raise, live hass.data read, no-op when no entries). Narrows the pre-existing test_init_does_not_repoll_on_push source guard to the realtime push region so the service's legitimate use of async_request_refresh is not a false positive, while still asserting the push path never re-polls.
… cannot starve the rest (H1) _subscribe_appliances subscribed appliances sequentially and _subscribe raised HonCodedError(MQTT_SUBSCRIBE_TIMEOUT) on the first stalled topic, aborting the loop. Every appliance after the slow one was never subscribed, so a single slow appliance early in the list permanently starved the rest of realtime push (they fell back to the possibly-stale REST poll). The binary _subscribed health flag then stayed False on any single failure, eventually escalating to a full _start() rebuild that dropped ALL subscriptions and re-hit the same bad topic, periodically disrupting the healthy appliances too. Replace the binary _subscribed bool with a set of acked topic strings (_subscribed_topics_set), giving per-topic isolation and letting the watchdog distinguish a single bad appliance from a dead connection: - _subscribe_missing() subscribes only not-yet-subscribed topics and, on a per-topic timeout, logs a warning and continues instead of aborting; the failed topic is simply retried next watchdog tick while the healthy topics stay subscribed. _subscribe_topic() is the extracted one-topic primitive. - Watchdog health is now "connected AND no missing topics". In the connected-but-missing branch it re-subscribes the missing topics, commits the set only under the existing connection+generation guard, then: if any topic is subscribed (set non-empty) the connection is alive and remaining misses are appliance-specific -> reset the outage counters, do NOT escalate; only a total blackout (nothing subscribes at all) counts toward _MAX_RESUBSCRIBE_FAILURES and escalates to a rebuild, preserving the original "one tick past the cap" timing. - The set is rebound to an empty set (atomic) wherever the old flag was reset (_start, the disconnection/connection-failure lifecycle callbacks); create() and the watchdog commit it only when connection and generation still match. Constant values, the generation-guard pattern, awscrt-thread callback safety and the not-connected grace path are unchanged. Adds tests for per-topic isolation, chronic-bad-topic-does-not-rebuild, total-blackout-still-escalates, and disconnect-mid-subscribe clearing the set.
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? |
📝 WalkthroughWalkthroughAdds a refresh service, updates MQTT realtime/subscription handling and appliance liveness reconciliation, changes climate mode reporting to return ChangesProject docs and release metadata
Refresh service
Realtime MQTT and appliance liveness
Climate mode mapping
Log privacy and redaction
Sequence Diagram(s)sequenceDiagram
participant HomeAssistant
participant _async_register_services
participant _handle_refresh
participant coordinator
HomeAssistant->>_async_register_services: register SERVICE_REFRESH
HomeAssistant->>_handle_refresh: call addhon.refresh
_handle_refresh->>coordinator: async_request_refresh()
coordinator-->>_handle_refresh: result or exception
sequenceDiagram
participant MQTTBroker
participant NativeMqttClient
participant HonAppliance
MQTTBroker->>NativeMqttClient: $aws/events/presence/...
NativeMqttClient-->>MQTTBroker: ignore for liveness
MQTTBroker->>NativeMqttClient: appliancestatus payload
NativeMqttClient->>HonAppliance: mark_realtime_seen(timestamp)
MQTTBroker->>NativeMqttClient: disconnected payload
NativeMqttClient->>HonAppliance: mark_realtime_disconnected()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_ac_write_path.py (1)
507-540: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a missing-
machModeread-path case.This block covers unmapped and non-advertised values, but not the case where
settings.machModeis absent altogether. That is a separate branch here, and it's the one still capable of surfacing a guessedCOOLvalue instead ofNone.🤖 Prompt for 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. In `@tests/test_ac_write_path.py` around lines 507 - 540, Add a missing-read-path test for absent machMode in the climate entity logic. In the test class around _climate and hvac_mode, create a case where settings.onOffStatus is "1" but settings.machMode is not present at all, then assert that entity.hvac_mode returns None rather than a guessed HVACMode.COOL. This should cover the separate branch from the existing unmapped and non-advertised machMode tests.
🤖 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/addhon/client/transport/mqtt.py`:
- Around line 284-300: The MQTT presence handling in `client/transport/mqtt.py`
still falls through after the `$aws/events/presence/` debug log, so ignored
client-session events can still reach `self._hon.notify()` and the INFO payload
summary. Add an early return in that presence branch inside the MQTT message
handler so AWS session presence topics are fully ignored and do not trigger HA
updates or extra logging.
- Around line 468-474: The `_subscribe_missing()` loop currently only catches
`HonCodedError`, so other subscribe failures from `_subscribe_topic()` can stop
the remaining topics from being processed. Update the exception handling in
`_subscribe_missing()` (and the `_subscribe_topic()` call path if needed) so
non-timeout subscribe errors are also isolated, logged, and skipped, allowing
later topics in the same pass to continue subscribing.
In `@custom_components/addhon/climate.py`:
- Around line 199-211: The climate mode lookup in climate.py is still defaulting
a missing AC_ATTR_MODE/machMode to "1", which causes an absent mode to be
reported as HVACMode.COOL instead of None. Update the mode retrieval in the
climate entity logic around AC_MODE_MAP so a missing raw value is treated as
unknown rather than coerced, preserving the None path already used when the
mapping does not contain a value. Make sure the existing debug/return None
branch is reached for both unmapped and missing machMode values.
---
Nitpick comments:
In `@tests/test_ac_write_path.py`:
- Around line 507-540: Add a missing-read-path test for absent machMode in the
climate entity logic. In the test class around _climate and hvac_mode, create a
case where settings.onOffStatus is "1" but settings.machMode is not present at
all, then assert that entity.hvac_mode returns None rather than a guessed
HVACMode.COOL. This should cover the separate branch from the existing unmapped
and non-advertised machMode tests.
🪄 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: CHILL
Plan: Pro
Run ID: bd5dc095-0834-4841-8ccd-697cc9d7ff11
📒 Files selected for processing (24)
.github/FUNDING.ymlREADME.mdcustom_components/addhon/__init__.pycustom_components/addhon/client/engine/appliance.pycustom_components/addhon/client/helpers.pycustom_components/addhon/client/transport/mqtt.pycustom_components/addhon/climate.pycustom_components/addhon/const.pycustom_components/addhon/manifest.jsoncustom_components/addhon/select.pycustom_components/addhon/services.yamlcustom_components/addhon/switch.pycustom_components/addhon/translations/en.jsoncustom_components/addhon/translations/it.jsontests/test_ac_write_path.pytests/test_client_str_to_float.pytests/test_coordinator_config_entry.pytests/test_engine_appliance_root.pytests/test_hon_client_realtime.pytests/test_legacy_cleanup.pytests/test_log_identity_redaction.pytests/test_program_select.pytests/test_refresh_service.pytests/test_transport_mqtt.py
…ng too _subscribe_missing caught only HonCodedError (the per-topic timeout), so any other exception from client.subscribe()/the awscrt future (transport or awscrt errors) escaped the loop and aborted the pass, re-opening the H1 starvation it was meant to close: every later topic in that pass was skipped. Worse, such an escaped exception hit the watchdog's outer `except Exception`, which backs off but does NOT increment resubscribe_failures, so an all-topics-fail-non-timeout dead socket would never escalate to a token-refreshing rebuild. Broaden the per-topic handler to isolate EVERY non-cancellation failure (re-raise CancelledError so stop() is never deadlocked, then `except Exception` -> log + skip). A failed topic stays missing and is retried next tick; if nothing subscribes the watchdog's inline blackout escalation now fires correctly. Flagged by the v5.4.0 release review (CodeRabbit, Major). Adds tests: a non-timeout subscribe error is isolated (the other topics still subscribe), and a forward-protection guard that the handler keeps the CancelledError re-raise and does not switch to `except BaseException` (which would swallow cancellation and deadlock shutdown).
…-mode, monotonic TTL) Three small fixes from the v5.4.0 release review (CodeRabbit / Greptile): - mqtt.py: the $aws/events/presence/ branch now returns after the debug log instead of falling through to self._hon.notify() and the INFO payload log. Our client's own session presence changes no appliance state, so it must not push a spurious HA state update nor echo the session payload on every (re)connect. - climate.py: hvac_mode no longer defaults an absent machMode to "1" (which reported COOL). A powered-on unit with no settings.machMode now returns None (unknown), closing the gap in the unmapped/non-advertised None contract. (Note: fan_mode's "0" default is already safe because "0" is intentionally not in AC_FAN_MAP, so a missing windSpeed already yields None.) - appliance.py: the realtime freshness window (_REALTIME_LIVENESS_TTL) now uses time.monotonic() instead of a naive datetime.now(), so it is immune to wall-clock jumps (NTP steps, DST transitions). _last_realtime_ts (the cloud timestamp used for ordering vs the disconnect event) is unchanged. Tests: presence events now assert no coordinator notify; a missing-machMode read path returns None; the freshness-expiry test ages the monotonic mark.
Merged
Merged
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.
Automated release PR for
v5.4.0.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Greptile Summary
This release adds a domain-wide
addhon.refreshservice, fixes stale RESTDISCONNECTEDevents clobbering live MQTT-connected devices, upgrades the MQTT subscription tracker from a binary flag to a per-topic set (H1 isolation), and replaces guessed HVAC/fan modes with explicitNonereturns when a mode is unmapped or unadvertised.addhon.refreshservice: registers once per domain, iterates all loaded coordinators viaasyncio.gather(return_exceptions=True), isolates per-entry failures, and is cleaned up on last unload alongside the existing log-level services.appliance.py+helpers.py):mark_realtime_seen()records a cloud-stamped timestamp and a monotonic receipt time;load_attributescompares these against the RESTlastConnEventto prevent a frozen DISCONNECTED from knocking a live device offline, while still honoring a genuinely newer disconnect and expiring the protection after 300 s.mqtt.py): replaces_subscribed: boolwith_subscribed_topics_set: set[str];_subscribe_missing()skips already-subscribed topics, isolates per-topic failures, and the watchdog escalates to a full rebuild only on a total blackout (nothing subscribed at all), never on a single chronically-broken appliance topic.Confidence Score: 5/5
Safe to merge — all three core changes (stale-disconnect fix, per-topic MQTT set, new refresh service) are covered by dedicated tests and the previous monotonic/exception-handling concerns are resolved.
The stale-disconnect reconciliation uses
monotonic()for TTL (immune to DST/NTP jumps) and cloud-stamped timestamps for ordering (skew-free). The MQTT subscription redesign swallows per-topic exceptions inside_subscribe_missing()and only escalates to a rebuild on a total blackout, which is well-tested and intentional. The refresh service isolates per-entry failures withasyncio.gather(return_exceptions=True)and never re-raises to the automation caller. No issues were found that would cause incorrect behavior at runtime.No files require special attention.
Important Files Changed
addhon.refreshservice with per-entry failure isolation; updates the unload cleanup and early-return guard to include the new service; switches legacy-cleanup INFO logs to redacted IDs.mark_realtime_seen()/mark_realtime_disconnected()for cloud-vs-cloud timestamp ordering; TTL freshness usesmonotonic()(fixes the previous P2 aboutdatetime.now()DST sensitivity);load_attributesnow preserves liveness when realtime evidence is newer than a REST disconnect.parse_cloud_timestamp()helper: total (never raises), accepts epoch-ms or ISO8601 with variable fractional digits, always returns tz-aware UTC; guards against OverflowError on arbitrary-precision ints fromjson.loads._subscribed: boolwith_subscribed_topics_set: set[str]; adds_subscribe_missing()with per-topic exception isolation; watchdog escalates to rebuild only on total blackout;$aws/events/presence/topics are intercepted and ignored for appliance connectivity; addresses both previous P2 findings.hvac_modenow returns `HVACModeRealtimeReconciliationTestclass: 13 tests covering stale-disconnect protection, TTL expiry, explicit MQTT disconnect clearing liveness, ISO/epoch comparability, and out-of-order timestamp handling._subscribe_missinginstead of_subscribe_appliancesin test stubs; adds tests formark_realtime_seenforwarding, session-presence topic isolation, and the per-topic subscription set model.AcClimateReadPathTestclass: covers valid/unmapped/unadvertised hvac_mode and fan_mode cases, including missingmachModeand the OFF-wins-over-unmapped invariant.Comments Outside Diff (2)
custom_components/addhon/client/transport/mqtt.py, line 698-702 (link)_subscribe_missingonly catchesHonCodedError(which wrapsasyncio.TimeoutError). If the underlyingconcurrent.futures.Futurefromself.client.subscribe()fails with any other exception (e.g., a closed/bad-state client that raises directly), it escapes_subscribe_missingand is caught by the outerexcept Exceptionin the watchdog. That handler adds backoff but does not incrementresubscribe_failures, so the escalation to a full rebuild never fires via that counter — the watchdog would retry indefinitely with capped backoff rather than refreshing the AWS token.In the old design,
attempted_resubscribe = Trueensured any exception from the re-subscribe path incremented the counter. The new design relies entirely on theresubscribe_failuresinline path, which is only reached when_subscribe_missingreturns normally with an empty set — not when it raises. The watchdog comment "Only the REBUILD path … can land here now" is also inaccurate for this edge case.In practice, awscrt failures tend to trigger the disconnection lifecycle callbacks first, so
_connectionwould flip to False and a normal reconnect would occur. But the escalation guarantee is technically broken for the pathological case of a connected-but-subscribe-failing client without a disconnection callback.custom_components/addhon/client/engine/appliance.py, line 329-331 (link)datetime.now()for wall-clock TTL freshness check_last_realtime_localis stored and compared using naivedatetime.now()(local wall-clock time). The subtraction inload_attributes(datetime.now() - self._last_realtime_local) is a local-to-local delta, so it works correctly during normal operation. However, a DST "fall back" transition — where the local clock moves backwards by one hour — can cause the delta to appear larger than it actually is, potentially expiring the 300-second TTL prematurely and briefly allowing a stale RESTDISCONNECTEDto win.datetime.now(timezone.utc)avoids this entirely and has no extra cost. Most HA Docker deployments run in UTC so the risk is low, but the fix is trivial.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "fix: address v5.4.0 review findings (pre..." | Re-trigger Greptile