fix: Correct seven defects found by review

custom_components/blebox_advanced/__init__.py

@@ -99,6 +99,13 @@ async def async_setup_entry(
         hass, entry, state=snapshot.actions if snapshot else None
     )
 
+    if snapshot is None:
+        # Nothing has ever been observed on this device - it is not answering
+        # now and it has never been remembered - so every polled platform below
+        # creates nothing, and with no entity listening the coordinator would
+        # stop polling and never notice the device coming back.
+        entry.async_on_unload(coordinator.async_keep_polling_without_entities())
+
     await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
     entry.async_on_unload(entry.add_update_listener(_async_options_updated))
     return True

custom_components/blebox_advanced/api.py

@@ -321,7 +321,7 @@ def _handle(
             ATTR_BLEBOX_ID: data.blebox_id,
             **hints,
         }
-        device_id = _async_ha_device_id(hass, data)
+        device_id = _async_ha_device_id(hass, entry_id, data)
         if device_id is not None:
             event_data[CONF_DEVICE_ID] = device_id
         hass.bus.async_fire(HA_EVENT, event_data)
@@ -363,20 +363,56 @@ def _parse_state_hints(query: Mapping[str, str]) -> dict[str, Any]:
 
 
 @callback
-def _async_ha_device_id(hass: HomeAssistant, data: BleBoxEventsData) -> str | None:
+def _async_ha_device_id(
+    hass: HomeAssistant, entry_id: str, data: BleBoxEventsData
+) -> str | None:
     """Resolve (and cache) the device registry id backing this entry.
 
     Looked up lazily because the device entry may not exist yet the first time
     a callback arrives, and its id never changes once it does.
+
+    Scoped to this config entry rather than looked up by identifier alone, and
+    that scoping is the whole point. We claim ``(blebox, <device id>)`` - the
+    *official* integration's domain, deliberately, so our entities land on the
+    same device as its relay and power entities. That makes a bare
+    ``async_get_device(identifiers=...)`` ambiguous the moment the official
+    integration is configured for the same device, which README documents as a
+    supported setup and which is the normal state mid-migration: the registry
+    narrows several matches by identifier-domain priority and keeps only rows
+    whose config entry domain equals the looked-up identifier's domain, so the
+    official row wins outright and ours is filtered out before the
+    "prefer the calling integration's own row" fallback is ever reached.
+    Device triggers are only ever offered on *our* row (see
+    :mod:`.device_trigger`, which skips config entries of any other domain), so
+    publishing the official row's id here meant the automation editor stored one
+    id and the bus carried another, and every device trigger built in the UI
+    silently never fired. The event entities kept working throughout because
+    they run off the dispatcher rather than the bus, which is what made it
+    quiet.
+
+    ``async_entries_for_config_entry`` rather than the newer
+    ``async_get_device_by_identifier``: identifiers are unique within a config
+    entry so the two mean the same thing here, but the module-level helper has
+    been in Home Assistant for years, and the receiver is the one module that
+    should not acquire a version floor of its own.
     """
-    if data.ha_device_id is not None:
-        return data.ha_device_id
-    device = dr.async_get(hass).async_get_device(
-        identifiers={(BLEBOX_DOMAIN, data.blebox_id)}
-    )
-    if device is not None:
-        data.ha_device_id = device.id
-    return data.ha_device_id
+    registry = dr.async_get(hass)
+    if (cached := data.ha_device_id) is not None:
+        # Only trust the cache while it still names a row this entry owns. A
+        # user can delete the device from the UI, and an id cached by a build
+        # from before the scoping fix names the official integration's row -
+        # neither may be allowed to outlive its truth for the rest of a session.
+        device = registry.async_get(cached)
+        if device is not None and entry_id in device.config_entries:
+            return cached
+        data.ha_device_id = None
+
+    identifier = (BLEBOX_DOMAIN, data.blebox_id)
+    for device in dr.async_entries_for_config_entry(registry, entry_id):
+        if identifier in device.identifiers:
+            data.ha_device_id = device.id
+            return device.id
+    return None
 
 
 @callback

custom_components/blebox_advanced/blebox_actions.py

@@ -368,12 +368,32 @@ def supports_http_action(self, trigger_type: int) -> bool:
         return ACTION_HTTP_GET in allowed if allowed else not self.field_preferences
 
     def free_slots(self) -> list[dict[str, Any]]:
-        """Unconfigured slots, in slot order."""
+        """Unconfigured slots, in slot order.
+
+        These three accessors partition the slot array: a slot is free, or ours,
+        or somebody else's, and never two of those at once. Provisioning adds
+        the pools up to decide whether a run fits, so an overlap between any two
+        of them would overcount the device's real capacity.
+        """
         return [a for a in self.actions if not is_configured(a)]
 
     def owned_actions(self) -> list[dict[str, Any]]:
-        """Slots created by this integration, in slot order."""
-        return [a for a in self.actions if is_owned(a)]
+        """Slots holding a live callback of this integration, in slot order.
+
+        ``is_configured`` is required as well as the ownership marker, and it is
+        not redundant with it: trigger type and action type are separate fields,
+        so a slot can carry our URL while its trigger reads as unconfigured.
+        Firmware that honours the ``triggerType: 0`` half of a clear and keeps
+        the ``actionType``/``param`` half leaves exactly that behind.
+
+        Such a slot used to appear in this list *and* in :meth:`free_slots`,
+        which let one physical slot be counted twice towards capacity and be
+        handed out twice in one run: the second callback silently destroyed the
+        first, and a slot taken from the free list while still sitting in the
+        reclaimable list was wiped by the clearing pass that followed. A slot
+        with no trigger never fires, so it is simply free.
+        """
+        return [a for a in self.actions if is_configured(a) and is_owned(a)]
 
     def foreign_actions(self) -> list[dict[str, Any]]:
         """Return configured slots owned by someone else; these are never touched."""
@@ -942,6 +962,19 @@ async def _async_sync_locked(self, desired: list[DesiredAction]) -> SyncResult:
             writes.append(build_action_payload(stale, template, **_clear_overrides()))
             result.cleared.append(_slot_id(stale))
 
+        # The whole plan is checked before the first request leaves, because a
+        # run that wrote two things into one slot would break the promise this
+        # method exists to keep in the quietest possible way: the second write
+        # destroys the first, and `SyncResult` still reports both. The pools are
+        # disjoint by construction, so this can only fire if that ever stops
+        # being true - in which case refusing the run leaves the device exactly
+        # as it was, which is the outcome design rule 2 asks for.
+        planned = [_slot_id(payload) for payload in writes]
+        if len(set(planned)) != len(planned):
+            raise BleBoxActionApiError(
+                f"Refusing to write the same action slot twice in one run: {planned}"
+            )
+
         for payload in writes:
             await self.async_save_action(payload)
 
@@ -956,10 +989,19 @@ async def async_remove_owned_actions(self) -> list[int]:
         async with self._action_lock:
             state = await self.async_get_actions_state()
             template = _field_template(state.actions)
+            # Every slot carrying our marker, rather than `owned_actions()`:
+            # removal has to erase our callback URL even from a slot whose
+            # trigger the firmware has already zeroed, or the entry goes away
+            # and the callback token stays readable in the wBox app.
+            #
             # Slot ids are resolved before the first write, so a device that
             # describes a slot without one fails before anything is touched
             # rather than half way through the clearing.
-            targets = [(_slot_id(action), action) for action in state.owned_actions()]
+            targets = [
+                (_slot_id(action), action)
+                for action in state.actions
+                if is_owned(action)
+            ]
             for _, action in targets:
                 await self.async_save_action(
                     build_action_payload(action, template, **_clear_overrides())

custom_components/blebox_advanced/coordinator.py

@@ -23,7 +23,7 @@
 from typing import Any
 
 from homeassistant.config_entries import ConfigEntry
-from homeassistant.core import HomeAssistant, callback
+from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback
 from homeassistant.helpers import issue_registry as ir
 from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed
 
@@ -191,6 +191,19 @@ def _measured_values(state: dict[str, Any]) -> tuple[str, ...]:
     return tuple(sorted(types))
 
 
+def _identity(info: DeviceInfo | None) -> tuple[str, ...] | None:
+    """Return the identity fields a device page shows, or None if it never said."""
+    if info is None:
+        return None
+    return (
+        info.device_type,
+        info.product,
+        info.firmware_version,
+        info.hardware_version,
+        info.api_level,
+    )
+
+
 def _timed_relays(state: dict[str, Any]) -> tuple[int, ...]:
     """Return the relays reporting a countdown, one sensor each."""
     relays = state.get("relays")
@@ -228,7 +241,13 @@ def capability_signature(snapshot: DeviceSnapshot) -> tuple[Any, ...]:
         _measured_values(state),
         _timed_relays(state),
         snapshot.uptime_s is not None,
-        snapshot.info is not None,
+        # The identity is compared by value, not merely by presence. It decides
+        # no entity, so it is not a capability, but it is what a device page
+        # shows; remembering only "an identity existed" left an offline start
+        # seeding the firmware version from before the last update, which is
+        # exactly the field someone checks to confirm an update worked. These
+        # move about as often as the firmware does, so they cost no writes.
+        _identity(snapshot.info),
     )
 
 
@@ -492,6 +511,59 @@ def async_request_full_refresh(self) -> None:
         """Ask for settings and actions on the next refresh, not just state."""
         self._force_full = True
 
+    @callback
+    def async_keep_polling_without_entities(self) -> CALLBACK_TYPE:
+        """Poll on with nothing listening, and reload once the device answers.
+
+        A device that has never answered has nothing remembered, so every polled
+        platform creates nothing and no ``CoordinatorEntity`` is ever added.
+        ``DataUpdateCoordinator`` arms its interval only while something is
+        listening, so without this the entry stays inert for good: it never
+        polls, so automatic callbacks are never healed and the entities never
+        arrive however long the device has been back, until somebody reloads the
+        entry by hand. Setup deliberately succeeds while the device is down, so
+        Home Assistant will not retry it either.
+
+        Registering a listener is deliberately the whole mechanism. A timer of
+        our own would poll a device that *does* have entities a second time on
+        every interval; a listener shares the single interval the coordinator
+        already keeps, and it is only ever registered when there are no entities
+        to keep it.
+
+        Polling alone would still not produce the entities, because platform
+        setup has already run and nothing runs it again - so the first answer
+        reloads the entry. Setup refreshes before it forwards the platforms, so
+        by then they see a live snapshot and create everything the device turned
+        out to have. Stopping first makes this a one-shot: the reloaded entry
+        either has entities of its own or lands right back here.
+
+        Returns the callback that stops it, for the caller to hand to the config
+        entry so that an unload takes it down with everything else.
+        """
+        remove: CALLBACK_TYPE | None = None
+
+        @callback
+        def _async_stop() -> None:
+            """Stop watching, at most once: removing a listener twice raises."""
+            nonlocal remove
+            if remove is None:
+                return
+            unsubscribe, remove = remove, None
+            unsubscribe()
+
+        @callback
+        def _async_answered() -> None:
+            """Reload the entry as soon as there is something to build from."""
+            # Listeners are told about a *failed* refresh too, which is what
+            # keeps entities unavailable, so the snapshot is what to check.
+            if self.data is None:
+                return
+            _async_stop()
+            self.hass.config_entries.async_schedule_reload(self.config_entry.entry_id)
+
+        remove = self.async_add_listener(_async_answered)
+        return _async_stop
+
     @property
     def settings(self) -> dict[str, Any]:
         """The device's settings, preferring a value just written from here.
@@ -575,20 +647,31 @@ async def _async_update_data(self) -> DeviceSnapshot:
 
         # Best-effort: a device that answers its identity but not these still
         # delivers events, so a failure here must not fail the whole update.
-        settings: dict[str, Any] = {}
-        settings_read = True
+        #
+        # What a failure must not do either is publish an empty payload as live
+        # state. The update still succeeds, so every entity stays *available*
+        # and simply reports the wrong thing: the cloud tunnel, the backlight
+        # and the access point all read as off, the overload threshold and the
+        # restart select go unknown, and the access point blanks its SSID. Home
+        # Assistant records each of those as a genuine state change, and the
+        # relay-only polls in between carry it forward until the next slow
+        # cycle, so an automation watching the cloud tunnel fires on nothing at
+        # all. The payload the device last gave is still the best description of
+        # it, so that is what a failed read carries forward - only ever on
+        # failure, so a device that really did answer with an empty object is
+        # still believed.
+        settings: dict[str, Any]
         try:
             settings = await self.manager.async_get_settings()
         except BleBoxError as err:
-            settings_read = False
+            settings = previous.settings
             _LOGGER.debug("Settings unavailable on %s: %s", info.name, err)
 
-        network: dict[str, Any] = {}
-        network_read = True
+        network: dict[str, Any]
         try:
             network = await self.manager.async_get_network()
         except BleBoxError as err:
-            network_read = False
+            network = previous.network
             _LOGGER.debug("Network state unavailable on %s: %s", info.name, err)
 
         uptime = await self.manager.async_get_uptime()
@@ -612,13 +695,18 @@ async def _async_update_data(self) -> DeviceSnapshot:
             health=health,
         )
 
-        # A fetch that failed is not evidence that a capability went away: the
-        # best-effort reads above report an empty object either way, and
-        # remembering that would leave the next offline start without those
-        # entities. Uptime is best-effort inside the API layer itself, so a
-        # device that has reported one before has to keep reporting it to count.
-        uptime_read = uptime is not None or previous.uptime_s is None
-        if settings_read and network_read and uptime_read:
+        # A fetch that failed is not evidence that a capability went away, but
+        # it no longer has to block this either: the best-effort reads above
+        # carry the last payload forward, so the snapshot describes the same
+        # capabilities the last successful read did. Demanding that all of them
+        # succeed instead meant firmware without ``/api/device/network`` never
+        # had its shape remembered at all, and a device that has never been
+        # remembered comes up with no entities at all when it is unreachable at
+        # startup. Uptime is the one read still worth guarding: it is
+        # best-effort inside the API layer itself and answers ``None`` either
+        # way, so a device that has reported one before has to keep reporting it
+        # to count as having answered.
+        if uptime is not None or previous.uptime_s is None:
             self._async_remember_capabilities(snapshot)
         return snapshot