fix(coordinator): respect authoritative provider event labels - #765
fix(coordinator): respect authoritative provider event labels#765tykeal wants to merge 1 commit into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #765 +/- ##
===========================================
+ Coverage 84.14% 94.94% +10.79%
===========================================
Files 10 35 +25
Lines 801 5597 +4796
===========================================
+ Hits 674 5314 +4640
- Misses 127 283 +156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Let direct event providers declare when their callback labels describe the intended lock operation more reliably than the current entity state. This lets Zigbee2MQTT keypad unlock payloads route to the unlock path even when the same MQTT payload still exposes the pre-operation lock_state, while keeping state-first inference for Z-Wave JS fallback labels that may come from stale alarm/access-control sensors. Assisted-by: GitHub Copilot CLI 1.0.82 (Claude Opus 5, model claude-opus-5) Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org>
e0d09ff to
5c1d833
Compare
secondof9
left a comment
secondof9
left a comment
There was a problem hiding this comment.
📋 Review Summary
Note
Review Status: 🟢 APPROVED
Change Type: 🐛 Bug Fix
Review Effort: 🟢 Low
Core Impact: Completes Bug 3 from #699 by adding a narrow, provider-scoped lock_event_label_is_authoritative hook so operation-event-first providers (Zigbee2MQTT, ZHA, Akuvox) can let an unambiguous label like Unlocked via Keypad win over stale pre-operation entity state.
🚦 CI & Pipeline Health Summary
| Check / Workflow Name | Status | Impact on Review |
|---|---|---|
| Pytest (3.14) | ✅ PASSED | All 50 tests pass, incl. the two new regression tests (verified locally on CPython 3.14.3). |
| coverage | ✅ PASSED | Project coverage well within threshold. |
| HACS Validation | ✅ PASSED | Manifest/manifest hygiene clean. |
| Hassfest Validation | ✅ PASSED | Integration manifest compliant. |
| Prek | ✅ PASSED | Pre-commit/pre-merge gate clean. |
| HACS Validation (2nd job) | ✅ PASSED | |
| Autolabel PR | ⏩ SKIPPED | Builtin autolabel, non-blocking. |
Note
CI Pipeline Clear: All GitHub Actions workflows completed successfully. Ruff (ruff check custom_components/ tests/) also passes cleanly in the checked-out workspace.
The only non-CI comment is a Codecov coverage bot note reporting 4 uncovered lines. That is informational only and not a failing check — see the "Coverage" note below.
🔍 Architectural Walkthrough
Coordinator inference logic (custom_components/keymaster/coordinator.py)
_handle_provider_lock_eventnow computesprovider_label_is_authoritativefrom the new provider hook and checks it before thestate_changedbranch. The reordered decision tree is correct: authoritativeunlock/lock(excludingjam) labels take precedence, then the existing state-first fallback for Z-Wave JS and others remains intact.- The
"jam" not in label_lowerguard correctly excludesLock Jammedfrom being treated as a lock (the existingtest_handle_provider_lock_event_jam_label_falls_back_to_statestill covers this). - No new I/O, no blocking calls, no async-boundary violations — this is pure inference over already-held state. Fully HA-compliant.
Provider hook (custom_components/keymaster/providers/_base.py)
- Adds
lock_event_label_is_authoritativereturningFalseby default, with a clear docstring explaining the semantics. Defaulting toFalsepreserves the prior behavior for every provider that does not opt in — a safe, additive change. - Zigbee2MQTT, ZHA, and Akuvox override it to
True, each with a short justification comment. Z-Wave JS intentionally staysFalse, matching the PR's stated rationale about stale alarm/access-control sensor fallbacks.
Tests (tests/test_coordinator_events.py)
- Two new tests directly exercise the modified branch:
test_handle_provider_lock_event_zigbee2mqtt_label_overrides_changed_state— Z2M labelUnlocked via Keypadwins over aLOCKEDentity state.test_handle_provider_lock_event_authoritative_lock_label_overrides_changed_state— authoritativeLocked via Keypadwins over anUNLOCKEDentity state.
- Both were run locally on Python 3.14 and pass, and they also serve as the regression proof the PR body describes (reverting the production change makes
state_changedtrue and routes tonew_state, which these asserts reject).
Tip
On the Codecov coverage comment: the first Copilot review (on commit e0d09ff) flagged the new inference branch as lacking test coverage. That concern is now fully addressed by the two new tests, and the second Copilot review on HEAD already flipped to "Approval recommended." Coverage is not blocking CI here. No action required.
✅ Conclusion
Narrow, well-scoped, additive, and thoroughly tested. Completes #699 cleanly. Approving.
|
Thanks for the heads-up on that gap — it's now covered. The new test |
Summary
Fixes the remaining Bug 3 from #699 by making provider event/action inference explicitly provider-aware. Bugs 1, 2, 4, and 5 from #699 were already fixed by prior work, so this completes the issue.
Proposed change
Zigbee2MQTT delivers its keypad action and the pre-operation
lock_statein the same MQTT payload. That means the provider callback can reportUnlocked via Keypadwhile Home Assistant still exposeslocked, causing the coordinator's state-first inference to route the event to_lock_locked.This PR adds a narrow
BaseLockProvider.lock_event_label_is_authoritativehook. Direct operation-event providers (Zigbee2MQTT, ZHA, and Akuvox) opt in so unambiguous labels such asUnlocked via Keypadwin over stale entity state. Z-Wave JS keeps the defaultFalse, preserving the existing protection where its fallback path may derive labels from stale alarm/access-control sensors and a genuinely changed entity state should remain authoritative.Type of change
Additional information
Validation:
/home/tykeal/repos/personal/homeassistant/keymaster/.tox/py314/bin/pytesttox --workdir /home/tykeal/repos/personal/homeassistant/keymaster/.tox -e lintwith the ruff commands temporarily adjusted locally to use explicitcustom_components/ tests/pathstests/test_coordinator_events.py::test_handle_provider_lock_event_zigbee2mqtt_label_overrides_changed_statefails becausekmlock.lock_statebecomeslockedinstead of remainingunlocked.