diff --git a/repeater/handler_helpers/discovery.py b/repeater/handler_helpers/discovery.py index 42825c6..7ede2e8 100644 --- a/repeater/handler_helpers/discovery.py +++ b/repeater/handler_helpers/discovery.py @@ -36,6 +36,71 @@ NODE_TYPE_NAMES = { } +def build_discovery_advert_record(result: dict) -> Optional[dict]: + """Turn a discovery response into a zero-hop advert row, or None if unusable. + + A node-discover response is only ever answered by a node that heard our + zero-hop broadcast directly, so the row is recorded with ``zero_hop=True`` + and ``route_type=2`` — the same thing firmware ``putNeighbour()`` records + when a discovery response arrives. + """ + pubkey = str(result.get("pub_key") or "").strip().lower() + if pubkey.startswith("0x"): + pubkey = pubkey[2:] + if not pubkey: + return None + + node_type = int(result.get("node_type", 0) or 0) + rssi = result.get("rssi") + # response_snr is our RX of their reply; plain snr is the fallback shape. + snr = result.get("response_snr", result.get("snr")) + + return { + "timestamp": time.time(), + "pubkey": pubkey, + "node_name": result.get("node_name"), + "is_repeater": node_type == 2, + "route_type": 2, + "contact_type": NODE_TYPE_NAMES.get(node_type, "Unknown"), + "latitude": None, + "longitude": None, + "rssi": int(rssi) if rssi is not None else None, + "snr": float(snr) if snr is not None else None, + "is_new_neighbor": True, + "zero_hop": True, + } + + +def persist_discovery_result(storage, result: dict) -> bool: + """Store one discovery response, returning whether it was written. + + Accepts either storage object the callers actually hold: ``StorageCollector`` + exposes ``record_advert`` (store plus the MQTT/Glass advert publish), while + ``SQLiteHandler`` exposes only ``store_advert``. Preferring the first and + falling back to the second is what makes this work from the mesh CLI, which + is constructed with the SQLiteHandler — checking for ``record_advert`` alone + made ``discover.neighbors`` silently persist nothing. + """ + if storage is None: + return False + + record = build_discovery_advert_record(result) + if record is None: + return False + + writer = getattr(storage, "record_advert", None) or getattr(storage, "store_advert", None) + if not callable(writer): + logger.debug("Storage backend cannot persist discovery results") + return False + + try: + writer(record) + return True + except Exception as e: + logger.debug("Failed to persist discovery result for %s: %s", record["pubkey"][:8], e) + return False + + class DiscoveryHelper: """Helper class for processing discovery requests in the repeater.""" diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index 2d783d4..b14941c 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -129,41 +129,11 @@ class MeshCLI: if not self.storage_handler: return enriched - record_advert = getattr(self.storage_handler, "record_advert", None) - if not callable(record_advert): - return enriched + from repeater.handler_helpers.discovery import persist_discovery_result - try: - import time - - node_type = int(enriched.get("node_type", 0) or 0) - contact_type = { - 1: "Chat Node", - 2: "Repeater", - 3: "Room Server", - }.get(node_type, "Unknown") - - rssi = enriched.get("rssi") - snr = enriched.get("response_snr", enriched.get("snr")) - advert_record = { - "timestamp": time.time(), - "pubkey": pub_key, - "node_name": enriched.get("node_name"), - "is_repeater": node_type == 2, - "route_type": 2, - "contact_type": contact_type, - "latitude": None, - "longitude": None, - "rssi": int(rssi) if rssi is not None else None, - "snr": float(snr) if snr is not None else None, - "is_new_neighbor": True, - "zero_hop": True, - } - record_advert(advert_record) + if persist_discovery_result(self.storage_handler, enriched): enriched["known_neighbor"] = True enriched["auto_added"] = True - except Exception as exc: - logger.debug("Auto-add discovery result failed for %s: %s", pub_key, exc) return enriched diff --git a/repeater/neighbors_publisher.py b/repeater/neighbors_publisher.py index 4c6438b..77c1014 100644 --- a/repeater/neighbors_publisher.py +++ b/repeater/neighbors_publisher.py @@ -25,6 +25,7 @@ import time from datetime import datetime, timezone from typing import Any, Callable, Dict, List, Optional +from repeater.handler_helpers.discovery import persist_discovery_result from repeater.handler_helpers.neighbor_scopes import ( STATUS_RESPONDED, STATUS_TIMEOUT, @@ -431,36 +432,7 @@ class NeighborsPublisher: "snr": float(snr_raw) if snr_raw is not None else 0.0, } - storage = self._storage() - record_advert = getattr(storage, "record_advert", None) if storage else None - if not callable(record_advert): - return result - - node_type = int(result.get("node_type", 0) or 0) - rssi = result.get("rssi") - snr = result.get("response_snr", result.get("snr")) - try: - record_advert( - { - "timestamp": time.time(), - "pubkey": pubkey, - "node_name": result.get("node_name"), - "is_repeater": node_type == 2, - "route_type": 2, - "contact_type": {1: "Chat Node", 2: "Repeater", 3: "Room Server"}.get( - node_type, "Unknown" - ), - "latitude": None, - "longitude": None, - "rssi": int(rssi) if rssi is not None else None, - "snr": float(snr) if snr is not None else None, - "is_new_neighbor": True, - "zero_hop": True, - } - ) - except Exception as e: - logger.debug(f"Could not persist discovery result for {pubkey[:8]}: {e}") - + persist_discovery_result(self._storage(), result) return result def _snapshot_neighbors(self) -> List[NeighborSnapshot]: diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 6504176..854be6e 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -475,6 +475,60 @@ def test_discovery_auto_add_skips_local_node_and_persists_remote(): storage.record_advert.assert_called_once() +def test_discovery_auto_add_persists_through_the_storage_actually_wired_in(): + """MeshCLI is constructed with the SQLiteHandler, not the StorageCollector. + + See repeater/main.py:516 -> TextHelper(sqlite_handler=...storage.sqlite_handler) + -> MeshCLI(storage_handler=self.sqlite_handler). SQLiteHandler exposes + store_advert but no record_advert, so a persistence path that only looked for + record_advert made `discover.neighbors` silently record nothing in production + while passing tests that injected a hand-rolled double. Spec'ing the mock off + the real class is what keeps this honest. + """ + from repeater.data_acquisition.sqlite_handler import SQLiteHandler + + identity = SimpleNamespace(get_public_key=lambda: bytes.fromhex("11" * 32)) + storage = MagicMock(spec=SQLiteHandler) + cli = MeshCLI( + "/tmp/cfg.yaml", _base_config(), _cfg_mgr(), identity=identity, storage_handler=storage + ) + + result = cli._auto_add_discovery_result( + { + "pub_key": "22" * 32, + "node_name": "Remote Repeater", + "node_type": 2, + "rssi": -70, + "response_snr": 4.25, + } + ) + + assert result["auto_added"] is True + storage.store_advert.assert_called_once() + record = storage.store_advert.call_args.args[0] + assert record["pubkey"] == "22" * 32 + assert record["is_repeater"] is True + assert record["zero_hop"] is True + assert record["snr"] == 4.25 + assert record["rssi"] == -70 + + +def test_discovery_auto_add_prefers_record_advert_when_the_collector_is_wired_in(): + """StorageCollector.record_advert also publishes the advert; prefer it.""" + from repeater.data_acquisition.storage_collector import StorageCollector + + identity = SimpleNamespace(get_public_key=lambda: bytes.fromhex("11" * 32)) + storage = MagicMock(spec=StorageCollector) + cli = MeshCLI( + "/tmp/cfg.yaml", _base_config(), _cfg_mgr(), identity=identity, storage_handler=storage + ) + + result = cli._auto_add_discovery_result({"pub_key": "22" * 32, "node_type": 2}) + + assert result["auto_added"] is True + storage.record_advert.assert_called_once() + + def test_cmd_set_save_failure_reports_error_and_skips_live_update(): mgr = _cfg_mgr(save_ok=False) cli = MeshCLI("/tmp/cfg.yaml", _base_config(), mgr)