Skip to content

fix: Correct seven defects found by review - #17

Merged
sdebasek merged 1 commit into
mainfrom
fix/review-correctness
Aug 21, 2026
Merged

sdebasek merged 1 commit into
mainfrom
fix/review-correctness

Conversation

@sdebasek

@sdebasek sdebasek commented Aug 21, 2026

Copy link
Copy Markdown
Owner

First of three PRs from the multi-agent review. This one is everything that can actually break behaviour. 26 findings survived adversarial refutation; these seven are the correctness ones.

Every fix has a regression test verified to fail against the pre-fix source, checked in a scratch copy rather than assumed.

1. Device triggers stop firing once both integrations are configured (high)

api.py resolved the device row with identifiers={("blebox", blebox_id)}. That identifier is deliberately the official integration's, which is what makes our entities land on the same device. But DeviceRegistry._async_matching_devices narrows multiple matches by identifier-domain priority, so when the official integration is also set up, its row wins and ours is filtered out. Meanwhile device_trigger.async_get_triggers only offers triggers on a row carrying a blebox_advanced entry, i.e. ours.

Result: the automation editor stores our row id, hass.bus.async_fire publishes the official one, and every device automation built in the UI silently never fires. Event entities keep working, because they run off the dispatcher rather than the bus, which is exactly what hid this. README.md documents running both side by side.

Fixed by resolving scoped to this config entry. A cached id is now trusted only while it still names a row this entry owns, so a value cached by an older build cannot survive the session.

Pre-fix, the scratch copy ran 3 failed, 199 passed: every existing test green, only the new detectors failing. The old suite structurally could not catch this, because it never set up an official entry alongside ours.

2. A failed best-effort read published an empty payload as live state (medium)

async_get_settings and async_get_network are best effort, but their empty result went into the snapshot as though the device had answered with it, while last_update_success stayed True. So entities stayed available and wrong: cloud tunnel, backlight and access point all read off, overload and restart went unknown, the AP blanked its SSID, and the wrong value was carried forward by every fast poll for up to a minute. Home Assistant recorded those as genuine state changes.

A verifier found a further consequence: with settings blanked, the backlight's rgb_color is None, so turning it on during the window writes the default colour over the user's chosen one.

Now the previous value is carried forward, and only in the except branch, so a device that genuinely answers with an empty object is still believed.

3. A never-answered device left the coordinator permanently inert (medium)

Setup deliberately succeeds when the device is unreachable. Every polled platform then returns early, so no CoordinatorEntity is added, and DataUpdateCoordinator only arms its interval once something listens. The entry stopped polling for good: no recovery when the device returned, and no callback healing either. Only a manual reload escaped it.

One listener is now registered, only when coordinator.data is None after the setup refresh, so a healthy device gets nothing extra and there is no second poll cycle. It is one-shot: on the first refresh that produces a snapshot it removes itself and reloads the entry, so platforms rebuild against what the device turned out to have.

The cache-write guard also demanded settings, network and uptime, so firmware without /api/device/network never got a cache at all and fell into the trap on every unlucky restart. That guard is now only the uptime arm, which is safe precisely because of point 2.

4. A half-cleared action slot was double-booked (low, deeper than reported)

The review said the free and reclaimable pools were built independently. The real cause is sharper: free_slots() selects on trigger type, owned_actions() on action type plus the URL marker. Different fields, so not mutually exclusive. A slot whose triggerType is 0 while actionType/param still hold our callback lands in both, which is exactly what firmware leaves behind if it honours half a clear write. Capacity was overstated and a run could clear the callback it had just written, breaking the "fits entirely or changes nothing" rule in CONTRIBUTING.

The pools are now a partition. A plan that double books is refused before the first request. Note the trap this nearly created: making owned_actions() stricter would have silently broken async_remove_owned_actions, leaving the callback token behind in a dormant slot on removal. That now scans for ownership directly.

5. The device page froze at setup (low)

model, sw_version and hw_version came from entry.data, written once by the config flow. After a firmware update the device page kept the old version indefinitely, which is the one field a user checks to confirm an update worked. Identity now follows what the device reports, addressed by row id rather than by identifier, deliberately, since an identifier lookup hits the mechanism in point 1.

The capability signature recorded only whether an identity existed, so a firmware change never rewrote the cache and an offline start still seeded the pre-update version. It now compares the identity by value. Firmware moves rarely, so this costs no extra .storage writes.

6. Enabling events reloaded the entry twice (low)

Re-enabling an input's event entity writes to the entity registry, which schedules a reload of its own. The disable is now handed over and cleared in one step Home Assistant does not treat as a reason to reload. A disable made by the user is still never undone.

Testing

214 passing, up from 198. 98% coverage counting branches. ruff clean.

Each agent verified its own tests against pristine source, and one went further with per-fix attribution runs, confirming that no test passes by accident when only part of the fix is applied. Tests that pass both ways are flagged as behaviour-preservation guards rather than presented as regressions.

Not tested on hardware.

Device triggers stopped firing once the official BleBox integration was
also configured. The callback receiver resolved the device row by
identifier, and that identifier is deliberately the official
integration's, so Home Assistant's identifier-domain narrowing handed
back the official row while device triggers are only ever offered on
ours. The automation editor stored one row id and the bus event carried
the other, so every device automation built in the UI silently never
ran. Event entities kept working, which is what hid it. The row is now
resolved scoped to this config entry, and a cached id is trusted only
while it still names a row this entry owns.

A failed settings or network read published an empty object as live
state. Those reads are best effort, but the empty result went into the
snapshot as though the device had answered with it, while the entry
stayed available, so the cloud tunnel, backlight and access point all
read as off and the overload and restart controls went unknown for up to
a minute. Home Assistant recorded those as real state changes. A failed
read now carries the previous value forward, and a device that genuinely
answers with an empty object is still believed.

A device that had never answered left the coordinator with no listeners,
and the interval timer is only armed once something listens, so the
entry went permanently inert: no polling, no recovery when the device
came back, no callback healing. One listener is now registered in that
case only, and it reloads the entry as soon as a snapshot arrives.

Free and owned action slots were selected on different fields, trigger
type versus action type, so a slot the firmware had half cleared counted
in both pools. Capacity was overstated and a run could clear the
callback it had just written. The pools are now a partition, a plan that
double books is refused before the first write, and removal still erases
our URL from a slot whose trigger the firmware already zeroed.

Firmware, hardware and model on the device page were frozen at setup, so
an update never showed. They now follow what the device reports, and a
firmware change is remembered for the next offline start rather than
seeding the version from before the update.

Enabling events for an input that had none reloaded the entry twice,
because re-enabling its entity wrote to the registry. The disable is now
handed over and cleared in one step that Home Assistant does not treat
as a reason to reload.
@sdebasek
sdebasek merged commit c6f3d97 into main Aug 21, 2026
4 checks passed
@sdebasek
sdebasek deleted the fix/review-correctness branch August 21, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant