From 0fa8290addb5011196b9d39942cbb2d267fff0ed Mon Sep 17 00:00:00 2001 From: Jean-Pierre Martineau Date: Sat, 29 Aug 2026 23:24:11 -0400 Subject: [PATCH] Contacts: a contact the device never held is still ours to remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list a screen removes from is the union of the device's contact table and the contacts MeshTerm remembers for it, so an entry can exist only on our side — or the firmware can drop one since we last read. Removing such a contact hit ERR_CODE_NOT_FOUND and reported "couldn't remove", leaving a row the reader had no way to get rid of. The device layer now raises ContactNotOnDeviceError for that code, the same class (and the same table lookup) the send path already raises. The single removal reports it as a warning naming what happened, forgets our half and ends the visit; the purge sweep counts it as purged rather than failed. The simulator refuses the same way, so the path is walkable under --mock. Co-Authored-By: Claude Opus 5 (1M context) --- meshterm/core/connection.py | 28 ++++++++++++++-- meshterm/ui/contacts_screen.py | 7 ++++ meshterm/ui/node_detail_screen.py | 20 ++++++++++- tests/test_device_config.py | 55 +++++++++++++++++++++++++++++++ tests/test_skeleton.py | 8 +++-- 5 files changed, 113 insertions(+), 5 deletions(-) diff --git a/meshterm/core/connection.py b/meshterm/core/connection.py index 3362559..d55f1d0 100644 --- a/meshterm/core/connection.py +++ b/meshterm/core/connection.py @@ -174,6 +174,11 @@ class ContactNotOnDeviceError(DeviceCommandError): for it (see :mod:`meshterm.core.contact_store`): a contact the firmware has since dropped still lists, and only the send finds out it is gone. + :meth:`Device.remove_contact` raises it for the same lookup against the same table, + where it means the opposite thing: nothing left to delete on the radio. A removal + therefore *reports* it and carries on dropping the contact from what MeshTerm + remembers, rather than failing and leaving a row the reader cannot get rid of. + Attributes: contact: The recipient the device could not find. """ @@ -535,8 +540,11 @@ class Device(ABC): node: The contact to remove. Raises: + ContactNotOnDeviceError: If the device holds no contact with that key — which + is not a failed removal but a removal with nothing left to do, so callers + drop it from what MeshTerm remembers and say so. DeviceCommandError: If the contact carries no public key to address it by, or - the device rejected the removal. + the device rejected the removal for any other reason. """ @abstractmethod @@ -1742,7 +1750,18 @@ class MeshCoreDevice(Device): async def remove_contact(self, node: Contact) -> None: # noqa: D102 - inherited docstring mc = self._require() pub = self._node_pubkey(node) # raises DeviceCommandError if it has no key - self._ok(await mc.commands.remove_contact(pub)) + result = await mc.commands.remove_contact(pub) + if result is not None and getattr(result, "is_error", lambda: False)(): + # A contact the firmware doesn't hold is the one rejection that isn't a + # failure: the list a screen removes from is the union of this table and the + # contacts MeshTerm remembers for the device, so the entry the reader is + # deleting may only ever have existed on our side. Same class the send path + # raises, for the same reason — the caller can finish the job. + if error_code(result) == _ERR_NOT_FOUND: + raise ContactNotOnDeviceError(node) + raise DeviceCommandError( + f"couldn't remove {node.name} from the device: {reject_reason(result)}" + ) async def get_tx_power(self) -> Optional[int]: # noqa: D102 - inherited docstring info = await self.get_self_info() @@ -2736,6 +2755,11 @@ class MockDevice(Device): async def remove_contact(self, node: Contact) -> None: # noqa: D102 - inherited docstring await asyncio.sleep(0) key = self._mock_key(node) + # Firmware can only delete a contact it holds, and answers ERR_CODE_NOT_FOUND for + # one it doesn't; the simulator refuses the same way so the "wasn't on the device, + # removed here anyway" path is walkable without a radio. + if key not in {self._mock_key(c) for c in self._contacts}: + raise ContactNotOnDeviceError(node) self._contacts = [c for c in self._contacts if self._mock_key(c) != key] async def get_tx_power(self) -> Optional[int]: # noqa: D102 - inherited docstring diff --git a/meshterm/ui/contacts_screen.py b/meshterm/ui/contacts_screen.py index 66966c5..5e5a20d 100644 --- a/meshterm/ui/contacts_screen.py +++ b/meshterm/ui/contacts_screen.py @@ -32,6 +32,7 @@ from typing import TYPE_CHECKING from rich.text import Text +from ..core.connection import ContactNotOnDeviceError from .contactlist import ContactListScreen, ContactRow from .menus import marked_label, menu_rows from .tui import Choice, Separator @@ -321,6 +322,12 @@ async def _purge_stale(ctx: "AppContext", self_key: str) -> int: for contact in victims: try: await device.remove_contact(contact) + except ContactNotOnDeviceError: + # The device has no such contact, so the sweep's work for it is already + # done: this one is listed only because MeshTerm remembers it for the + # device. It still counts as purged — forgetting it below is what makes + # the row go away — and there is nothing here for the reader to fix. + ctx.log.debug("contacts: %s was not on the device; purging ours", contact.name) except Exception as exc: # noqa: BLE001 - one bad removal shouldn't abort the sweep failed += 1 ctx.log.debug("contacts: purge failed for %s: %s", contact.name, exc) diff --git a/meshterm/ui/node_detail_screen.py b/meshterm/ui/node_detail_screen.py index 9f11371..dd69c5a 100644 --- a/meshterm/ui/node_detail_screen.py +++ b/meshterm/ui/node_detail_screen.py @@ -82,6 +82,7 @@ from typing import TYPE_CHECKING, Optional from rich.cells import cell_len from rich.text import Text +from ..core.connection import ContactNotOnDeviceError from ..core.geo import EARTH_RADIUS_KM, usable_fix from ..core.models import NODE_TYPE_LABELS, Contact, utcnow from ..platforms import Platform, on_platform @@ -1454,7 +1455,9 @@ async def _remove_contact( Returns: ``True`` if the contact was removed, ``False`` if the user cancelled or the device refused (the refusal is shown, never swallowed — a contact still on the radio must - not vanish from the list). + not vanish from the list). A device that has no such contact is not a refusal: the + removal finishes on our side and says so + (:class:`~meshterm.core.connection.ContactNotOnDeviceError`). """ from .surface import TuiUi @@ -1474,6 +1477,21 @@ async def _remove_contact( device = await ctx.device() try: await device.remove_contact(contact) + except ContactNotOnDeviceError: + # Nothing to delete on the radio — the entry only ever existed in what MeshTerm + # remembers for this device (or the firmware dropped it since we read the table). + # That is not a failed removal: the contact still goes, and the reader is told why + # the device had no part in it, because "removed" would otherwise be a claim about + # a radio that never held it. + ctx.log.debug("contacts: %s was not on the device; removing ours", contact.name) + await session.message_dialog( + Text( + f"⚠ {label} wasn't in this device's contacts — removed from the ones " + "MeshTerm remembers for it.", + style="warn", + ), + title="Remove contact", + ) except Exception as exc: # noqa: BLE001 - a refused removal is reported, not raised ctx.log.debug("contacts: remove failed for %s: %s", contact.name, exc) await session.message_dialog( diff --git a/tests/test_device_config.py b/tests/test_device_config.py index 8ed4c52..05f192a 100644 --- a/tests/test_device_config.py +++ b/tests/test_device_config.py @@ -557,6 +557,61 @@ async def test_a_refused_removal_is_shown_and_the_contact_stays() -> None: assert forgotten == [] # the device still holds it, so neither do we forget it +async def test_a_contact_the_device_never_held_is_still_removed_here() -> None: + """"Not on the device" is not a refusal — the removal finishes on our side, and says so. + + The list a screen removes from is the union of the device's table and the contacts + MeshTerm remembers for it, so the entry may only ever have existed on our side (or the + firmware dropped it since we read). Failing there left a row the reader could not get + rid of. Instead the reader is told the device had no part in it, our half is forgotten, + and the visit ends on the contact like any other removal. + """ + import asyncio + import logging + from types import SimpleNamespace + + from meshterm.core.connection import ContactNotOnDeviceError + from meshterm.core.models import Contact + from meshterm.ui.node_detail_screen import _remove_contact + from meshterm.ui.surface import TuiUi + from meshterm.ui.tui.session import TuiSession + + hub = Contact(name="Hub", public_key="3d" * 32, key_prefix="3d" * 6) + forgotten: list[tuple[str, str]] = [] + invalidated: list[bool] = [] + + class _Device: + async def remove_contact(self, contact) -> None: # noqa: ANN001 + await asyncio.sleep(0) + raise ContactNotOnDeviceError(contact) + + session = TuiSession() + ui = TuiUi(session) + + async def _accepted(*a, **k): # noqa: ANN001, ANN002, ANN003 + return True + + ui.dialog = _accepted # type: ignore[method-assign] + ctx = SimpleNamespace( + ui=ui, + log=logging.getLogger("test.remove"), + contact_store=SimpleNamespace( + forget=lambda dev, key: forgotten.append((dev, key)) + ), + devstate=SimpleNamespace(invalidate_contacts=lambda: invalidated.append(True)), + device=lambda: _device(_Device()), + ) + + task = asyncio.ensure_future(_remove_contact(ctx, hub, "cc" * 32, "Hub")) + told = await _step_until_screen(session, lambda s: "wasn't in this device" in _screen_text(s)) + text = _screen_text(told) + assert "Hub" in text and "⚠" in text # a warning, not the ✗ of a failed command + told.handle("escape") + assert await task is True + assert forgotten == [("cc" * 32, "3d" * 32)] + assert invalidated == [True] + + async def _true() -> bool: return True diff --git a/tests/test_skeleton.py b/tests/test_skeleton.py index f6c594d..08ac1ab 100644 --- a/tests/test_skeleton.py +++ b/tests/test_skeleton.py @@ -320,6 +320,8 @@ async def test_mock_device_honors_forced_path() -> None: async def test_mock_device_remove_contact_drops_it_from_the_table() -> None: """Removing a contact deletes it from the device's table (matched by public key).""" + from meshterm.core.connection import ContactNotOnDeviceError + device = MockDevice() await device.connect() before = await device.get_contacts() @@ -328,8 +330,10 @@ async def test_mock_device_remove_contact_drops_it_from_the_table() -> None: after = await device.get_contacts() assert "Alice" not in {c.name for c in after} assert len(after) == len(before) - 1 - # Removing one already gone is a no-op, not an error. - await device.remove_contact(alice) + # Removing one it no longer holds is refused the way firmware refuses it, so the + # screen's "wasn't on the device, removed here anyway" path is walkable on --mock. + with pytest.raises(ContactNotOnDeviceError): + await device.remove_contact(alice) assert len(await device.get_contacts()) == len(after)