diff --git a/custom_components/lock_code_manager/providers/matter.py b/custom_components/lock_code_manager/providers/matter.py index 4efb7e036..ddf1659bb 100644 --- a/custom_components/lock_code_manager/providers/matter.py +++ b/custom_components/lock_code_manager/providers/matter.py @@ -381,21 +381,46 @@ async def async_set_user(self, user: User) -> SetUserResult: # Matter SDK raises ``matter_server.common.errors.MatterError`` # subclasses (e.g. ``UnknownError`` carrying # ``InteractionModelError: InvalidCommand (0x85)``); those are - # NOT ``HomeAssistantError`` subclasses, so they slipped past - # the original catch. Some Matter lock firmwares reject - # rename-on-existing-user with InvalidCommand even though the - # user record itself is valid -- the user still exists at the - # known index, the only thing lost is the name update. The - # subsequent credential write proceeds against the resolved - # user_id. - LOGGER.warning( - "Lock %s: failed to update user name on slot %s " - "(user_index=%s); continuing without name update: %s", - self.lock.entity_id, - slot, - existing_user_index, - err, - ) + # NOT ``HomeAssistantError`` subclasses. Some Matter lock + # firmwares reject rename-on-existing-user when the new + # name contains non-alphanumeric characters (the colons in + # the canonical ``lcm::`` prefix). Retry with the + # slot-only fallback (``str(slot)``), which the tolerant + # parser still recognizes as the slot binding. If the + # second attempt also fails, fall through to the + # historical "tolerate name-set failures" contract -- the + # user record itself remains valid at ``existing_user_index`` + # so the credential write can still proceed. + slot_only_name = str(slot) + try: + await set_lock_user( + client, + node, + user_index=existing_user_index, + user_name=slot_only_name, + ) + except (HomeAssistantError, MatterError) as fallback_err: + LOGGER.warning( + "Lock %s: failed to update user name on slot %s " + "(user_index=%s); continuing without name update. " + "Canonical attempt: %s. Slot-only fallback: %s", + self.lock.entity_id, + slot, + existing_user_index, + err, + fallback_err, + ) + else: + LOGGER.info( + "Lock %s: lock rejected canonical tag %r for slot %s " + "(%s); renamed to slot-only %r so the slot binding " + "survives the read", + self.lock.entity_id, + user.name, + slot, + err, + slot_only_name, + ) return SetUserResult(user_id=existing_user_index, created=False) # CREATE: no LCM-tagged user exists for this slot yet — let Matter @@ -416,14 +441,41 @@ async def async_set_user(self, user: User) -> SetUserResult: f"Matter set_lock_user failed for {self.lock.entity_id}: {err}" ) from err except MatterError as err: - # Matter SDK error on CREATE -- unlike UPDATE we can't tolerate - # this because without an allocated user_index the credential - # write has no target. Surface as LockOperationFailed so the - # seam routes it through retry rather than the catchall - # _suspend_slot path (which would create a spurious repair). - raise LockOperationFailed( - f"Matter set_lock_user failed for {self.lock.entity_id}: {err}" - ) from err + # Matter SDK error on CREATE with the canonical tagged name. + # Some lock firmwares restrict ``userName`` to alphanumeric + # characters and reject the colons in ``lcm::``. Retry + # once with the slot-only fallback (``str(slot)``) -- the + # tolerant parser recognizes digit-only names as the slot + # binding, so the find-or-create-by-tag lookup still works on + # subsequent operations. + slot_only_name = str(slot) + try: + result = await set_lock_user( + client, + node, + user_index=None, + user_name=slot_only_name, + ) + except MatterError as fallback_err: + # Second attempt also failed -- the lock is rejecting more + # than just the charset. Surface as LockOperationFailed so + # the seam routes through retry rather than the catchall + # suspend path (which would create a spurious repair issue). + raise LockOperationFailed( + f"Matter set_lock_user failed for {self.lock.entity_id} " + f"on both canonical and slot-only fallback names: " + f"canonical={err}, slot-only={fallback_err}" + ) from fallback_err + LOGGER.info( + "Lock %s: lock rejected canonical tag %r for slot %s (%s); " + "created with slot-only %r so the slot binding survives the " + "read", + self.lock.entity_id, + user.name, + slot, + err, + slot_only_name, + ) return SetUserResult(user_id=result["user_index"], created=True) def _slot_from_seam_user(self, user: User) -> int: diff --git a/tests/providers/matter/test_provider.py b/tests/providers/matter/test_provider.py index 108f3646d..478df9dc9 100644 --- a/tests/providers/matter/test_provider.py +++ b/tests/providers/matter/test_provider.py @@ -2297,6 +2297,10 @@ async def test_set_user_update_tolerates_name_set_failure( and a transient 500 or a rejected name should not block the subsequent credential write. """ + # AsyncMock(side_effect=Error) raises the same error on every call. + # The new fallback path attempts canonical then slot-only; both raise + # here, so the test exercises the "both fail -> tolerate" branch + # while keeping the historical HomeAssistantError contract. mock_set_user = AsyncMock(side_effect=HomeAssistantError("500")) user = User(user_id=2, name="lcm:2:Updated Name") with ( @@ -2320,7 +2324,7 @@ async def test_set_user_update_tolerates_name_set_failure( result = await matter_lock_simple.async_set_user(user) assert result == SetUserResult(user_id=7, created=False) - mock_set_user.assert_called_once() + assert mock_set_user.call_count == 2 assert "failed to update user name" in caplog.text async def test_set_user_update_tolerates_matter_sdk_error( @@ -2337,6 +2341,11 @@ async def test_set_user_update_tolerates_matter_sdk_error( and let MatterError bubble past, causing the seam's catchall to spuriously suspend the slot and create a repair issue. """ + # Both canonical and slot-only attempts raise the same error here; + # the test exercises the "both attempts fail -> tolerate" branch + # for a MatterError specifically. A dedicated test + # (test_set_user_update_falls_back_to_slot_only_on_charset_rejection) + # covers the success-on-fallback case. mock_set_user = AsyncMock( side_effect=UnknownError("InteractionModelError: InvalidCommand (0x85)") ) @@ -2365,23 +2374,67 @@ async def test_set_user_update_tolerates_matter_sdk_error( # subsequent credential write to the right user; only the name # update was dropped. assert result == SetUserResult(user_id=7, created=False) - mock_set_user.assert_called_once() + assert mock_set_user.call_count == 2 assert "failed to update user name" in caplog.text assert "InvalidCommand (0x85)" in caplog.text - async def test_set_user_create_raises_operation_failed_on_matter_sdk_error( + async def test_set_user_create_falls_back_to_slot_only_on_charset_rejection( + self, hass: HomeAssistant, matter_lock_simple: MatterLock, caplog + ) -> None: + """CREATE retries with the slot-only fallback when canonical is rejected. + + Some Matter lock firmwares restrict ``userName`` to alphanumeric + and reject the colons in ``lcm::``. The provider retries + once with the slot-only fallback (``str(slot)``); the tolerant + parser recognizes digit-only names as the slot binding, so the + slot identity survives subsequent reads. + """ + # First call fails with InvalidCommand; second succeeds. + mock_set_user = AsyncMock( + side_effect=[ + UnknownError("InteractionModelError: InvalidCommand (0x85)"), + {"user_index": 4}, + ] + ) + user = User(user_id=1, name="lcm:1:Alice") + with ( + patch.object( + matter_lock_simple, "_get_matter_client", return_value=MagicMock() + ), + patch.object( + matter_lock_simple, "_get_matter_node", return_value=MagicMock() + ), + self._patch_users([]), + patch(f"{_PROVIDER_MODULE}.set_lock_user", mock_set_user), + ): + result = await matter_lock_simple.async_set_user(user) + + assert result == SetUserResult(user_id=4, created=True) + assert mock_set_user.call_count == 2 + first_call_kwargs = mock_set_user.call_args_list[0].kwargs + second_call_kwargs = mock_set_user.call_args_list[1].kwargs + assert first_call_kwargs["user_name"] == "lcm:1:Alice" + assert second_call_kwargs["user_name"] == "1" + assert "rejected canonical tag" in caplog.text + assert "InvalidCommand (0x85)" in caplog.text + + async def test_set_user_create_raises_when_both_attempts_fail( self, hass: HomeAssistant, matter_lock_simple: MatterLock ) -> None: - """CREATE maps ``MatterError`` to ``LockOperationFailed``. + """CREATE surfaces ``LockOperationFailed`` only after BOTH names fail. - Unlike UPDATE, a CREATE failure cannot be tolerated -- without an - allocated user_index the subsequent credential write has no - target. Routing to ``LockOperationFailed`` ensures the seam - retries through the normal backoff path rather than dropping - into the catchall suspend (which would create a spurious repair - issue without giving the lock a chance to recover). + If the lock rejects the canonical AND the slot-only fallback, + the failure is more than just a charset issue. Surfacing as + ``LockOperationFailed`` lets the seam route through the normal + retry path rather than the catchall suspend (which would create + a spurious repair issue). """ - mock_set_user = AsyncMock(side_effect=UnknownError("transient failure")) + mock_set_user = AsyncMock( + side_effect=[ + UnknownError("InvalidCommand (0x85)"), + UnknownError("InvalidCommand again"), + ] + ) user = User(user_id=1, name="lcm:1:Alice") with ( patch.object( @@ -2392,10 +2445,99 @@ async def test_set_user_create_raises_operation_failed_on_matter_sdk_error( ), self._patch_users([]), patch(f"{_PROVIDER_MODULE}.set_lock_user", mock_set_user), - pytest.raises(LockOperationFailed, match="transient failure"), + pytest.raises(LockOperationFailed, match="both canonical and slot-only"), ): await matter_lock_simple.async_set_user(user) + assert mock_set_user.call_count == 2 + + async def test_set_user_update_falls_back_to_slot_only_on_charset_rejection( + self, hass: HomeAssistant, matter_lock_simple: MatterLock, caplog + ) -> None: + """UPDATE retries with the slot-only fallback so the slot tag survives. + + UPDATE's contract is "tolerate name-set failures," but a + successful slot-only rename preserves the slot binding across + reads (the digit-only name parses to the same slot). Strictly + better than letting the name drift to whatever pre-LCM string + the lock had. + """ + mock_set_user = AsyncMock( + side_effect=[ + UnknownError("InvalidCommand (0x85)"), + None, + ] + ) + user = User(user_id=2, name="lcm:2:Updated Name") + with ( + patch.object( + matter_lock_simple, "_get_matter_client", return_value=MagicMock() + ), + patch.object( + matter_lock_simple, "_get_matter_node", return_value=MagicMock() + ), + self._patch_users( + [ + { + "user_index": 7, + "user_name": "lcm:2:Original", + "credentials": [{"type": "pin", "index": 3}], + }, + ] + ), + patch(f"{_PROVIDER_MODULE}.set_lock_user", mock_set_user), + ): + result = await matter_lock_simple.async_set_user(user) + + assert result == SetUserResult(user_id=7, created=False) + assert mock_set_user.call_count == 2 + assert mock_set_user.call_args_list[1].kwargs["user_name"] == "2" + assert "rejected canonical tag" in caplog.text + assert "renamed to slot-only" in caplog.text + + async def test_set_user_update_tolerates_when_both_attempts_fail( + self, hass: HomeAssistant, matter_lock_simple: MatterLock, caplog + ) -> None: + """UPDATE still tolerates the failure when both attempts fail. + + The user record at ``existing_user_index`` is still valid; only + the rename was lost. The subsequent credential write can still + proceed, so we return the resolved user_id and log a single + warning carrying both error messages for diagnosis. + """ + mock_set_user = AsyncMock( + side_effect=[ + UnknownError("canonical rejected"), + UnknownError("slot-only rejected too"), + ] + ) + user = User(user_id=2, name="lcm:2:Bob") + with ( + patch.object( + matter_lock_simple, "_get_matter_client", return_value=MagicMock() + ), + patch.object( + matter_lock_simple, "_get_matter_node", return_value=MagicMock() + ), + self._patch_users( + [ + { + "user_index": 9, + "user_name": "lcm:2:Original", + "credentials": [{"type": "pin", "index": 1}], + }, + ] + ), + patch(f"{_PROVIDER_MODULE}.set_lock_user", mock_set_user), + ): + result = await matter_lock_simple.async_set_user(user) + + assert result == SetUserResult(user_id=9, created=False) + assert mock_set_user.call_count == 2 + assert "failed to update user name" in caplog.text + assert "canonical rejected" in caplog.text + assert "slot-only rejected too" in caplog.text + # ============================================================================= # async_delete_user tests