From eaab0e5dca2f98123714e56da633cd7a5739e4c2 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:08:44 -0700 Subject: [PATCH 1/9] fix(cli): honor the boolean config save contract in set commands ConfigManager.save_to_file returns a bare bool, but every mesh CLI set command (and the password command) still tuple-unpacked it, so each one wrote the YAML and then raised, skipping the live update and returning an error to the remote admin. Route every save through one helper that checks the bool, reports save failures honestly, and only live-applies after a successful write. The test fixture mocked the stale tuple shape, which is why the suite stayed green while the CLI was broken on the real manager. --- repeater/handler_helpers/mesh_cli.py | 112 ++++++++++++++----------- tests/test_handler_helpers_mesh_cli.py | 16 +++- 2 files changed, 76 insertions(+), 52 deletions(-) diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index cc0dff3..e301231 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -39,6 +39,22 @@ class MeshCLI: self.repeater_config = config.get("repeater", {}) self.mesh_config = config.setdefault("mesh", {}) + def _save_config_and_apply(self, sections=None) -> bool: + """Persist the config, then live-apply the changed sections. + + ``ConfigManager.save_to_file`` returns a bare bool; the tuple form is + tolerated for older manager doubles. Live update only runs after a + successful save so a failed write never half-applies a change. Pass + no sections to stage a change: saved to disk, applied on restart. + """ + result = self.config_manager.save_to_file() + saved = result[0] if isinstance(result, tuple) else bool(result) + if not saved: + return False + if sections: + self.config_manager.live_update_daemon(sections) + return True + def _get_node_name(self) -> str: """Return the configured node name, preferring the newer key when present.""" return self.repeater_config.get("node_name") or self.repeater_config.get("name", "Unknown") @@ -455,11 +471,9 @@ class MeshCLI: # Save config and live update try: - saved, err = self.config_manager.save_to_file() - if not saved: - logger.error(f"Failed to save password: {err}") - return f"Error: Failed to save config: {err}" - self.config_manager.live_update_daemon(["security"]) + if not self._save_config_and_apply(["security"]): + logger.error("Failed to save password: config save failed") + return "Error: Failed to save config" return f"password now: {new_password}" except Exception as e: logger.error(f"Failed to save password: {e}") @@ -615,32 +629,32 @@ class MeshCLI: try: if key == "af": self.repeater_config["airtime_factor"] = float(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "name": self._set_node_name(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "repeat": self.repeater_config["mode"] = "forward" if value.lower() == "on" else "monitor" - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return f"OK - repeat is now {'ON' if self.repeater_config['mode'] == 'forward' else 'OFF'}" elif key == "lat": self.repeater_config["latitude"] = float(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "lon": self.repeater_config["longitude"] = float(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "radio": @@ -656,46 +670,46 @@ class MeshCLI: self.config["radio"]["bandwidth"] = float(radio_parts[1]) self.config["radio"]["spreading_factor"] = int(radio_parts[2]) self.config["radio"]["coding_rate"] = int(radio_parts[3]) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["radio"]) + if not self._save_config_and_apply(["radio"]): + return "Error: Failed to save config" return "OK - restart repeater to apply" elif key == "freq": if "radio" not in self.config: self.config["radio"] = {} self.config["radio"]["frequency"] = float(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["radio"]) + if not self._save_config_and_apply(["radio"]): + return "Error: Failed to save config" return "OK - restart repeater to apply" elif key == "tx": if "radio" not in self.config: self.config["radio"] = {} self.config["radio"]["tx_power"] = int(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["radio"]) + if not self._save_config_and_apply(["radio"]): + return "Error: Failed to save config" return "OK" elif key == "guest.password": if "security" not in self.config: self.config["security"] = {} self.config["security"]["guest_password"] = value - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["security"]) + if not self._save_config_and_apply(["security"]): + return "Error: Failed to save config" return "OK" elif key == "owner.info": self.repeater_config["owner_info"] = value.replace("|", "\n") - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "allow.read.only": if "security" not in self.config: self.config["security"] = {} self.config["security"]["allow_read_only"] = value.lower() == "on" - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["security"]) + if not self._save_config_and_apply(["security"]): + return "Error: Failed to save config" return "OK" elif key == "advert.interval": @@ -703,8 +717,8 @@ class MeshCLI: if mins > 0 and (mins < 60 or mins > 240): return "Error: interval range is 60-240 minutes" self.repeater_config["advert_interval_minutes"] = mins - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "flood.advert.interval": @@ -712,8 +726,8 @@ class MeshCLI: if (hours > 0 and hours < 3) or hours > 168: return "Error: interval range is 3-168 hours" self.repeater_config["flood_advert_interval_hours"] = hours - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "flood.max": @@ -721,8 +735,8 @@ class MeshCLI: if max_val > 64: return "Error: max 64" self.repeater_config["max_flood_hops"] = max_val - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "path.hash.mode": @@ -730,8 +744,8 @@ class MeshCLI: if mode not in (0, 1, 2): return "Error: path.hash.mode must be 0, 1, or 2" self.mesh_config["path_hash_mode"] = mode - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["mesh"]) + if not self._save_config_and_apply(["mesh"]): + return "Error: Failed to save config" return "OK" elif key == "loop.detect": @@ -739,8 +753,8 @@ class MeshCLI: if mode not in ("off", "minimal", "moderate", "strict"): return "Error: loop.detect must be off, minimal, moderate, or strict" self.mesh_config["loop_detect"] = mode - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["mesh"]) + if not self._save_config_and_apply(["mesh"]): + return "Error: Failed to save config" return "OK" elif key == "rxdelay": @@ -748,8 +762,8 @@ class MeshCLI: if delay < 0: return "Error: cannot be negative" self.config.setdefault("delays", {})["rx_delay_base"] = delay - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater", "delays"]) + if not self._save_config_and_apply(["repeater", "delays"]): + return "Error: Failed to save config" return "OK" elif key == "txdelay": @@ -757,8 +771,8 @@ class MeshCLI: if delay < 0: return "Error: cannot be negative" self.config.setdefault("delays", {})["tx_delay_factor"] = delay - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater", "delays"]) + if not self._save_config_and_apply(["repeater", "delays"]): + return "Error: Failed to save config" return "OK" elif key == "direct.txdelay": @@ -766,20 +780,20 @@ class MeshCLI: if delay < 0: return "Error: cannot be negative" self.config.setdefault("delays", {})["direct_tx_delay_factor"] = delay - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater", "delays"]) + if not self._save_config_and_apply(["repeater", "delays"]): + return "Error: Failed to save config" return "OK" elif key == "multi.acks": self.repeater_config["multi_acks"] = int(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "int.thresh": self.repeater_config["interference_threshold"] = int(value) - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return "OK" elif key == "agc.reset.interval": @@ -787,8 +801,8 @@ class MeshCLI: # Round to nearest multiple of 4 rounded = (interval // 4) * 4 self.repeater_config["agc_reset_interval"] = rounded - saved, _ = self.config_manager.save_to_file() - self.config_manager.live_update_daemon(["repeater"]) + if not self._save_config_and_apply(["repeater"]): + return "Error: Failed to save config" return f"OK - interval rounded to {rounded}" else: diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 966415a..873f7a7 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -36,9 +36,9 @@ def _base_config(): } -def _cfg_mgr(save_ok=True, err=None): +def _cfg_mgr(save_ok=True): return SimpleNamespace( - save_to_file=MagicMock(return_value=(save_ok, err)), + save_to_file=MagicMock(return_value=save_ok), live_update_daemon=MagicMock(), ) @@ -97,9 +97,10 @@ def test_cmd_password_save_success_failure_and_exception(): assert cli_ok._cmd_password("password newpw") == "password now: newpw" ok_mgr.live_update_daemon.assert_called_once_with(["security"]) - bad_mgr = _cfg_mgr(save_ok=False, err="disk") + bad_mgr = _cfg_mgr(save_ok=False) cli_bad = MeshCLI("/tmp/cfg.yaml", _base_config(), bad_mgr) assert "Failed to save config" in cli_bad._cmd_password("password x") + bad_mgr.live_update_daemon.assert_not_called() ex_mgr = SimpleNamespace( save_to_file=MagicMock(side_effect=RuntimeError("boom")), @@ -442,3 +443,12 @@ def test_discovery_auto_add_skips_local_node_and_persists_remote(): ) assert remote_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) + + for command in ("af 2.0", "name x", "freq 915", "guest.password g", "flood.max 8"): + assert cli._cmd_set(command) == "Error: Failed to save config" + mgr.live_update_daemon.assert_not_called() From eae42d9e197da1515c0d8ae1a2f05a501ffc0551 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:09:42 -0700 Subject: [PATCH 2/9] fix(cli): store security settings where login authentication reads them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The password, guest.password, and allow.read.only commands wrote a top-level security section with a stale key name, while LoginHelper authenticates from repeater.security.admin_password/.guest_password/ .allow_read_only — so remote password changes never took effect. Point the set and get commands at the real subtree, and push repeater.security onto the live repeater ACL during a repeater-section live update: the ACL captures its passwords at registration, so without the refresh a saved change would still only apply after a restart. Room-server ACLs keep their per-identity settings passwords. --- repeater/config_manager.py | 8 ++++ repeater/handler_helpers/login.py | 33 +++++++++++++++ repeater/handler_helpers/mesh_cli.py | 34 +++++++-------- tests/test_handler_helpers_mesh_cli.py | 57 +++++++++++++++++++++++++- 4 files changed, 114 insertions(+), 18 deletions(-) diff --git a/repeater/config_manager.py b/repeater/config_manager.py index 4db5dc2..af4bcf0 100644 --- a/repeater/config_manager.py +++ b/repeater/config_manager.py @@ -275,6 +275,14 @@ class ConfigManager: self.daemon.repeater_handler.reload_runtime_config() logger.info("Reloaded RepeaterHandler runtime config") + # Re-apply login security when the repeater section changed; the + # repeater ACL captures repeater.security at registration, so a + # saved password change must be pushed to the live ACL explicitly. + if "repeater" in sections and self.daemon: + login_helper = getattr(self.daemon, "login_helper", None) + if login_helper is not None and hasattr(login_helper, "refresh_repeater_security"): + login_helper.refresh_repeater_security(daemon_config) + # Also reload advert_helper config if repeater section changed if self.daemon and hasattr(self.daemon, "advert_helper") and self.daemon.advert_helper: if "repeater" in sections: diff --git a/repeater/handler_helpers/login.py b/repeater/handler_helpers/login.py index 552f1d8..63e90e5 100644 --- a/repeater/handler_helpers/login.py +++ b/repeater/handler_helpers/login.py @@ -33,6 +33,9 @@ class LoginHelper: self.handlers = {} self.acls = {} # Per-identity ACLs keyed by hash_byte + # The repeater identity's ACL, kept so live config updates can re-apply + # repeater.security without re-registering the identity. + self._repeater_acl = None self._pending_tasks = set() # Shared across all identities so the node's total anon-reply rate is # bounded (mirrors firmware anon_limiter: ~4 requests / 2 min). @@ -116,6 +119,8 @@ class LoginHelper: ) self.acls[hash_byte] = identity_acl + if identity_type != "room_server": + self._repeater_acl = identity_acl logger.info(f"Created ACL for {identity_type} '{name}': hash=0x{hash_byte:02X}") # Create auth callback that uses this identity's ACL @@ -180,6 +185,34 @@ class LoginHelper: logger.info(f"Registered {identity_type} '{name}' login handler: hash=0x{hash_byte:02X}") + def refresh_repeater_security(self, config: dict = None) -> bool: + """Re-apply ``repeater.security`` from config to the live repeater ACL. + + The ACL captures its passwords at registration, so without this a saved + password change would only take effect after a restart. Room-server ACLs + keep their per-identity ``settings`` passwords and are not touched. + """ + acl = self._repeater_acl + if acl is None: + return False + + cfg = config if isinstance(config, dict) else self.config + security = (cfg or {}).get("repeater", {}).get("security", {}) or {} + + acl.admin_password = security.get("admin_password") or "" + acl.guest_password = security.get("guest_password") or "" + acl.allow_read_only = bool(security.get("allow_read_only", False)) + try: + acl.max_clients = int(security.get("max_clients", acl.max_clients)) + except (TypeError, ValueError): + logger.warning( + "Ignoring invalid repeater.security.max_clients=%r during security refresh", + security.get("max_clients"), + ) + + logger.info("Refreshed repeater ACL security settings from config") + return True + def _format_region_names(self) -> str: """Build the comma-separated region-names string for an anon regions reply. diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index e301231..4537676 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -55,6 +55,11 @@ class MeshCLI: self.config_manager.live_update_daemon(sections) return True + def _get_security_config(self): + """Return the repeater login security section (the one LoginHelper reads).""" + security = self.repeater_config.get("security") + return security if isinstance(security, dict) else {} + def _get_node_name(self) -> str: """Return the configured node name, preferring the newer key when present.""" return self.repeater_config.get("node_name") or self.repeater_config.get("name", "Unknown") @@ -463,15 +468,12 @@ class MeshCLI: if not new_password: return "Error: Password cannot be empty" - # Update security config - if "security" not in self.config: - self.config["security"] = {} - - self.config["security"]["password"] = new_password + # LoginHelper authenticates from repeater.security.admin_password. + self.repeater_config.setdefault("security", {})["admin_password"] = new_password # Save config and live update try: - if not self._save_config_and_apply(["security"]): + if not self._save_config_and_apply(["repeater"]): logger.error("Failed to save password: config save failed") return "Error: Failed to save config" return f"password now: {new_password}" @@ -552,7 +554,7 @@ class MeshCLI: return f"> {role}" elif param == "guest.password": - guest_pw = self.config.get("security", {}).get("guest_password", "") + guest_pw = self._get_security_config().get("guest_password") or "" return f"> {guest_pw}" elif param == "owner.info": @@ -560,7 +562,7 @@ class MeshCLI: return f"> {owner_info}" elif param == "allow.read.only": - allow = self.config.get("security", {}).get("allow_read_only", False) + allow = self._get_security_config().get("allow_read_only", False) return f"> {'on' if allow else 'off'}" elif param == "advert.interval": @@ -691,10 +693,9 @@ class MeshCLI: return "OK" elif key == "guest.password": - if "security" not in self.config: - self.config["security"] = {} - self.config["security"]["guest_password"] = value - if not self._save_config_and_apply(["security"]): + # LoginHelper authenticates from repeater.security.guest_password. + self.repeater_config.setdefault("security", {})["guest_password"] = value + if not self._save_config_and_apply(["repeater"]): return "Error: Failed to save config" return "OK" @@ -705,10 +706,11 @@ class MeshCLI: return "OK" elif key == "allow.read.only": - if "security" not in self.config: - self.config["security"] = {} - self.config["security"]["allow_read_only"] = value.lower() == "on" - if not self._save_config_and_apply(["security"]): + # LoginHelper reads repeater.security.allow_read_only. + self.repeater_config.setdefault("security", {})["allow_read_only"] = ( + value.lower() == "on" + ) + if not self._save_config_and_apply(["repeater"]): return "Error: Failed to save config" return "OK" diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 873f7a7..febebc1 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -23,6 +23,12 @@ def _base_config(): "multi_acks": 2, "interference_threshold": -115, "agc_reset_interval": 8, + "security": { + "admin_password": "adminpw", + "guest_password": "guest", + "allow_read_only": True, + "max_clients": 5, + }, }, "radio": { "frequency": 915000000, @@ -32,7 +38,8 @@ def _base_config(): "tx_power": 22, }, "mesh": {"path_hash_mode": 0, "loop_detect": "minimal"}, - "security": {"guest_password": "guest", "allow_read_only": True}, + # Decoy: the legacy top-level section the CLI must no longer touch. + "security": {"guest_password": "stale-top-level", "allow_read_only": False}, } @@ -95,7 +102,10 @@ def test_cmd_password_save_success_failure_and_exception(): assert cli_ok._cmd_password("password ") == "Error: Password cannot be empty" assert cli_ok._cmd_password("password newpw") == "password now: newpw" - ok_mgr.live_update_daemon.assert_called_once_with(["security"]) + # The password must land where LoginHelper reads it, not the legacy top-level section. + assert cfg["repeater"]["security"]["admin_password"] == "newpw" + assert "password" not in cfg["security"] + ok_mgr.live_update_daemon.assert_called_once_with(["repeater"]) bad_mgr = _cfg_mgr(save_ok=False) cli_bad = MeshCLI("/tmp/cfg.yaml", _base_config(), bad_mgr) @@ -185,9 +195,12 @@ def test_cmd_set_updates_and_validation_errors(): assert cli._cmd_set("freq 868000000").startswith("OK") assert cli._cmd_set("tx 17") == "OK" assert cli._cmd_set("guest.password g") == "OK" + assert cfg["repeater"]["security"]["guest_password"] == "g" + assert cfg["security"]["guest_password"] == "stale-top-level" assert cli._cmd_set("owner.info Alice|Ops") == "OK" assert cfg["repeater"]["owner_info"] == "Alice\nOps" assert cli._cmd_set("allow.read.only off") == "OK" + assert cfg["repeater"]["security"]["allow_read_only"] is False assert cli._cmd_set("path.hash.mode 2") == "OK" assert cfg["mesh"]["path_hash_mode"] == 2 assert cli._cmd_set("path.hash.mode 3") == "Error: path.hash.mode must be 0, 1, or 2" @@ -452,3 +465,43 @@ def test_cmd_set_save_failure_reports_error_and_skips_live_update(): for command in ("af 2.0", "name x", "freq 915", "guest.password g", "flood.max 8"): assert cli._cmd_set(command) == "Error: Failed to save config" mgr.live_update_daemon.assert_not_called() + + +def test_cmd_get_reads_repeater_security_section(): + cli = MeshCLI("/tmp/cfg.yaml", _base_config(), _cfg_mgr()) + + # Values come from repeater.security, not the legacy top-level decoy. + assert cli._cmd_get("guest.password") == "> guest" + assert cli._cmd_get("allow.read.only") == "> on" + + +def test_cli_password_change_applies_to_live_repeater_acl(tmp_path): + """A CLI password change must reach the running LoginHelper ACL, so the + next login attempt authenticates against the new password.""" + from repeater.config_manager import ConfigManager + from repeater.handler_helpers.login import LoginHelper + + cfg = _base_config() + login_helper = LoginHelper(identity_manager=None, config=cfg) + identity = SimpleNamespace(get_public_key=lambda: b"\x42" + b"\x00" * 31) + login_helper.register_identity( + name="repeater", identity=identity, identity_type="repeater", config=cfg + ) + acl = login_helper.get_acl_for_identity(0x42) + assert acl.admin_password == "adminpw" + assert acl.guest_password == "guest" + + daemon = SimpleNamespace(config=cfg, login_helper=login_helper) + manager = ConfigManager( + config_path=str(tmp_path / "config.yaml"), config=cfg, daemon_instance=daemon + ) + cli = MeshCLI(str(tmp_path / "config.yaml"), cfg, manager) + + assert cli._cmd_password("password fresh-secret") == "password now: fresh-secret" + assert acl.admin_password == "fresh-secret" + + assert cli._cmd_set("guest.password fresh-guest") == "OK" + assert acl.guest_password == "fresh-guest" + + assert cli._cmd_set("allow.read.only off") == "OK" + assert acl.allow_read_only is False From 101681fad493657d40600cfeab3508b834c924eb Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:10:20 -0700 Subject: [PATCH 3/9] fix(cli): accept radio parameters in MHz and stage them until restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set freq and set radio stored the CLI's MHz/kHz inputs directly into radio.frequency and radio.bandwidth, which the rest of the stack treats as Hz — a freq change tuned the radio to a few hundred hertz. Convert to Hz on write and validate set radio with the firmware gate (freq 150-2500, bw 7-500, sf 5-12, cr 5-8, same error string; set freq stays unvalidated like firmware). Radio changes are now saved without a live apply so the reply's restart-to-apply contract is real: a live retune would cut off the remote admin mid-session, and the all-or-nothing live radio path would also have dragged staged frequency changes along with a tx tweak. --- repeater/handler_helpers/mesh_cli.py | 45 +++++++++++++++----------- tests/test_handler_helpers_mesh_cli.py | 31 +++++++++++++++--- 2 files changed, 54 insertions(+), 22 deletions(-) diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index 4537676..5d289d2 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -45,7 +45,8 @@ class MeshCLI: ``ConfigManager.save_to_file`` returns a bare bool; the tuple form is tolerated for older manager doubles. Live update only runs after a successful save so a failed write never half-applies a change. Pass - no sections to stage a change: saved to disk, applied on restart. + no sections to stage a change: saved to disk, applied on restart + (radio parameters, matching firmware's reboot-to-apply). """ result = self.config_manager.save_to_file() saved = result[0] if isinstance(result, tuple) else bool(result) @@ -660,37 +661,45 @@ class MeshCLI: return "OK" elif key == "radio": - # Format: freq bw sf cr + # Format: freq(MHz) bw(kHz) sf cr — the config stores Hz. radio_parts = value.split() if len(radio_parts) != 4: return "Error: Expected freq bw sf cr" - if "radio" not in self.config: - self.config["radio"] = {} + freq_mhz = float(radio_parts[0]) + bw_khz = float(radio_parts[1]) + sf = int(radio_parts[2]) + cr = int(radio_parts[3]) + if not ( + 150.0 <= freq_mhz <= 2500.0 + and 7.0 <= bw_khz <= 500.0 + and 5 <= sf <= 12 + and 5 <= cr <= 8 + ): + return "Error, invalid radio params" - self.config["radio"]["frequency"] = float(radio_parts[0]) - self.config["radio"]["bandwidth"] = float(radio_parts[1]) - self.config["radio"]["spreading_factor"] = int(radio_parts[2]) - self.config["radio"]["coding_rate"] = int(radio_parts[3]) - if not self._save_config_and_apply(["radio"]): + radio_config = self.config.setdefault("radio", {}) + radio_config["frequency"] = int(round(freq_mhz * 1_000_000)) + radio_config["bandwidth"] = int(round(bw_khz * 1_000)) + radio_config["spreading_factor"] = sf + radio_config["coding_rate"] = cr + if not self._save_config_and_apply(): return "Error: Failed to save config" return "OK - restart repeater to apply" elif key == "freq": - if "radio" not in self.config: - self.config["radio"] = {} - self.config["radio"]["frequency"] = float(value) - if not self._save_config_and_apply(["radio"]): + # CLI input is MHz (firmware parity); the config stores Hz. + freq_mhz = float(value) + self.config.setdefault("radio", {})["frequency"] = int(round(freq_mhz * 1_000_000)) + if not self._save_config_and_apply(): return "Error: Failed to save config" return "OK - restart repeater to apply" elif key == "tx": - if "radio" not in self.config: - self.config["radio"] = {} - self.config["radio"]["tx_power"] = int(value) - if not self._save_config_and_apply(["radio"]): + self.config.setdefault("radio", {})["tx_power"] = int(value) + if not self._save_config_and_apply(): return "Error: Failed to save config" - return "OK" + return "OK - restart repeater to apply" elif key == "guest.password": # LoginHelper authenticates from repeater.security.guest_password. diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index febebc1..881a816 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -189,11 +189,19 @@ def test_cmd_set_updates_and_validation_errors(): assert cli._cmd_set("repeat off").endswith("OFF") assert cfg["repeater"]["mode"] == "monitor" - assert cli._cmd_set("radio 900000000 250000 9 6").startswith("OK") - assert cfg["radio"]["frequency"] == 900000000.0 + # CLI input is MHz/kHz (firmware parity); the config stores Hz. + assert cli._cmd_set("radio 900 250 9 6").startswith("OK") + assert cfg["radio"]["frequency"] == 900000000 + assert cfg["radio"]["bandwidth"] == 250000 + assert cfg["radio"]["spreading_factor"] == 9 + assert cfg["radio"]["coding_rate"] == 6 + assert cli._cmd_set("radio 100 250 9 6") == "Error, invalid radio params" + assert cli._cmd_set("radio 900 250 4 6") == "Error, invalid radio params" - assert cli._cmd_set("freq 868000000").startswith("OK") - assert cli._cmd_set("tx 17") == "OK" + assert cli._cmd_set("freq 868.5").startswith("OK") + assert cfg["radio"]["frequency"] == 868500000 + assert cli._cmd_set("tx 17") == "OK - restart repeater to apply" + assert cfg["radio"]["tx_power"] == 17 assert cli._cmd_set("guest.password g") == "OK" assert cfg["repeater"]["security"]["guest_password"] == "g" assert cfg["security"]["guest_password"] == "stale-top-level" @@ -505,3 +513,18 @@ def test_cli_password_change_applies_to_live_repeater_acl(tmp_path): assert cli._cmd_set("allow.read.only off") == "OK" assert acl.allow_read_only is False + + +def test_cmd_set_radio_commands_stage_without_live_apply(): + """Radio changes match firmware reboot-to-apply: saved to disk, never + live-applied (a live retune would cut off the admin mid-session).""" + cfg = _base_config() + mgr = _cfg_mgr() + cli = MeshCLI("/tmp/cfg.yaml", cfg, mgr) + + assert cli._cmd_set("radio 900 250 9 6") == "OK - restart repeater to apply" + assert cli._cmd_set("freq 868.5") == "OK - restart repeater to apply" + assert cli._cmd_set("tx 17") == "OK - restart repeater to apply" + + assert mgr.save_to_file.call_count == 3 + mgr.live_update_daemon.assert_not_called() From 6db8ac97f59e3baf5227c86a1ebb247a688227df Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:10:37 -0700 Subject: [PATCH 4/9] fix(cli): point the flood advert interval at the engine timer key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set flood.advert.interval wrote flood_advert_interval_hours, a key nothing reads, and get reported it with a default of 24 — the engine's flood-advert timer (and the web API) consume send_advert_interval_hours with a default of 10. Read and write the consumed key so the remote command actually reschedules the timer; the orphan key is left in place and ignored. --- repeater/handler_helpers/mesh_cli.py | 6 ++++-- tests/test_handler_helpers_mesh_cli.py | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index 5d289d2..f01bd32 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -571,7 +571,8 @@ class MeshCLI: return f"> {interval}" elif param == "flood.advert.interval": - interval = self.repeater_config.get("flood_advert_interval_hours", 24) + # The engine's flood-advert timer consumes send_advert_interval_hours. + interval = self.repeater_config.get("send_advert_interval_hours", 10) return f"> {interval}" elif param == "flood.max": @@ -736,7 +737,8 @@ class MeshCLI: hours = int(value) if (hours > 0 and hours < 3) or hours > 168: return "Error: interval range is 3-168 hours" - self.repeater_config["flood_advert_interval_hours"] = hours + # The engine's flood-advert timer consumes send_advert_interval_hours. + self.repeater_config["send_advert_interval_hours"] = hours if not self._save_config_and_apply(["repeater"]): return "Error: Failed to save config" return "OK" diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 881a816..9970af1 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -15,6 +15,7 @@ def _base_config(): "longitude": 3.4, "airtime_factor": 1.1, "advert_interval_minutes": 120, + # Stale orphan key from the old CLI; nothing must read it. "flood_advert_interval_hours": 24, "max_flood_hops": 20, "rx_delay_base": 0.2, @@ -221,6 +222,10 @@ def test_cmd_set_updates_and_validation_errors(): assert cli._cmd_set("advert.interval 59").startswith("Error: interval range") assert cli._cmd_set("flood.advert.interval 2").startswith("Error: interval range") + assert cli._cmd_set("flood.advert.interval 12") == "OK" + # The engine's timer key is updated; the stale orphan key is left alone. + assert cfg["repeater"]["send_advert_interval_hours"] == 12 + assert cfg["repeater"]["flood_advert_interval_hours"] == 24 assert cli._cmd_set("flood.max 100") == "Error: max 64" assert cli._cmd_set("rxdelay -1") == "Error: cannot be negative" assert cli._cmd_set("txdelay -1") == "Error: cannot be negative" @@ -528,3 +533,15 @@ def test_cmd_set_radio_commands_stage_without_live_apply(): assert mgr.save_to_file.call_count == 3 mgr.live_update_daemon.assert_not_called() + + +def test_cmd_get_flood_advert_interval_reads_engine_key(): + cfg = _base_config() + cli = MeshCLI("/tmp/cfg.yaml", cfg, _cfg_mgr()) + + # The orphan flood_advert_interval_hours (24) must be ignored; the engine + # key is absent so the engine default (10) is reported. + assert cli._cmd_get("flood.advert.interval") == "> 10" + + cfg["repeater"]["send_advert_interval_hours"] = 6 + assert cli._cmd_get("flood.advert.interval") == "> 6" From f19421ee6ce773a80a76ce7b5a835422e2f26df5 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:11:24 -0700 Subject: [PATCH 5/9] fix(cli): clamp delay factors to the firmware ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Firmware rejects rxdelay outside 0-20 and txdelay/direct.txdelay outside 0-2.0; the CLI only rejected negatives, so a remote admin could set delay factors far beyond what any firmware node would accept. Apply the firmware ranges with the firmware error strings and state the ranges in the help text. Existing configs with out-of-range values are untouched — only new CLI sets are gated. Also adds an end-to-end regression test running every set command against the real ConfigManager on a temp config file. --- repeater/handler_helpers/mesh_cli.py | 20 ++++---- tests/test_handler_helpers_mesh_cli.py | 67 ++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index f01bd32..de34220 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -56,7 +56,7 @@ class MeshCLI: self.config_manager.live_update_daemon(sections) return True - def _get_security_config(self): + def _get_security_config(self) -> Dict[str, Any]: """Return the repeater login security section (the one LoginHelper reads).""" security = self.repeater_config.get("security") return security if isinstance(security, dict) else {} @@ -357,9 +357,9 @@ class MeshCLI: " set flood.max Max flood hops (max 64)\n" " set path.hash.mode <0-2> Path hash mode (0=1B,1=2B,2=3B)\n" " set loop.detect Flood loop detection\n" - " set rxdelay RX delay base (>=0)\n" - " set txdelay TX delay factor (>=0)\n" - " set direct.txdelay Direct TX delay (>=0)\n" + " set rxdelay RX delay base (0-20)\n" + " set txdelay TX delay factor (0-2)\n" + " set direct.txdelay Direct TX delay (0-2)\n" " set multi.acks Multi-ack count\n" " set int.thresh Interference threshold\n" " set agc.reset.interval AGC reset (rounded to x4)" @@ -772,8 +772,8 @@ class MeshCLI: elif key == "rxdelay": delay = float(value) - if delay < 0: - return "Error: cannot be negative" + if delay < 0 or delay > 20.0: + return "Error, must be 0-20" self.config.setdefault("delays", {})["rx_delay_base"] = delay if not self._save_config_and_apply(["repeater", "delays"]): return "Error: Failed to save config" @@ -781,8 +781,8 @@ class MeshCLI: elif key == "txdelay": delay = float(value) - if delay < 0: - return "Error: cannot be negative" + if delay < 0 or delay > 2.0: + return "Error, must be 0-2" self.config.setdefault("delays", {})["tx_delay_factor"] = delay if not self._save_config_and_apply(["repeater", "delays"]): return "Error: Failed to save config" @@ -790,8 +790,8 @@ class MeshCLI: elif key == "direct.txdelay": delay = float(value) - if delay < 0: - return "Error: cannot be negative" + if delay < 0 or delay > 2.0: + return "Error, must be 0-2" self.config.setdefault("delays", {})["direct_tx_delay_factor"] = delay if not self._save_config_and_apply(["repeater", "delays"]): return "Error: Failed to save config" diff --git a/tests/test_handler_helpers_mesh_cli.py b/tests/test_handler_helpers_mesh_cli.py index 9970af1..6504176 100644 --- a/tests/test_handler_helpers_mesh_cli.py +++ b/tests/test_handler_helpers_mesh_cli.py @@ -227,9 +227,13 @@ def test_cmd_set_updates_and_validation_errors(): assert cfg["repeater"]["send_advert_interval_hours"] == 12 assert cfg["repeater"]["flood_advert_interval_hours"] == 24 assert cli._cmd_set("flood.max 100") == "Error: max 64" - assert cli._cmd_set("rxdelay -1") == "Error: cannot be negative" - assert cli._cmd_set("txdelay -1") == "Error: cannot be negative" - assert cli._cmd_set("direct.txdelay -1") == "Error: cannot be negative" + assert cli._cmd_set("rxdelay -1") == "Error, must be 0-20" + assert cli._cmd_set("rxdelay 20.5") == "Error, must be 0-20" + assert cli._cmd_set("rxdelay 20") == "OK" + assert cli._cmd_set("txdelay -1") == "Error, must be 0-2" + assert cli._cmd_set("txdelay 2.5") == "Error, must be 0-2" + assert cli._cmd_set("direct.txdelay -1") == "Error, must be 0-2" + assert cli._cmd_set("direct.txdelay 2.5") == "Error, must be 0-2" assert cli._cmd_set("agc.reset.interval 10") == "OK - interval rounded to 8" assert cli._cmd_set("bad") == "Error: Missing value" @@ -545,3 +549,60 @@ def test_cmd_get_flood_advert_interval_reads_engine_key(): cfg["repeater"]["send_advert_interval_hours"] = 6 assert cli._cmd_get("flood.advert.interval") == "> 6" + + +def test_cli_set_commands_persist_with_real_config_manager(tmp_path): + """Every CLI set command must work against the real ConfigManager bool save + contract: reply OK and leave the change on disk.""" + import yaml + + from repeater.config_manager import ConfigManager + + config_path = tmp_path / "config.yaml" + cfg = _base_config() + manager = ConfigManager(config_path=str(config_path), config=cfg, daemon_instance=None) + cli = MeshCLI(str(config_path), cfg, manager) + + commands = [ + "af 1.5", + "name node-b", + "repeat off", + "lat 10.5", + "lon -3.25", + "radio 869.618 250 9 6", + "freq 915.125", + "tx 17", + "guest.password gpw", + "owner.info Bob|Lab", + "allow.read.only on", + "advert.interval 90", + "flood.advert.interval 12", + "flood.max 8", + "path.hash.mode 1", + "loop.detect moderate", + "rxdelay 4.5", + "txdelay 1.5", + "direct.txdelay 0.25", + "multi.acks 1", + "int.thresh -110", + "agc.reset.interval 8", + ] + for command in commands: + reply = cli._cmd_set(command) + assert reply.startswith("OK"), f"set {command!r} replied {reply!r}" + + assert cli._cmd_password("password newadminpw") == "password now: newadminpw" + + saved = yaml.safe_load(config_path.read_text()) + assert saved["repeater"]["airtime_factor"] == 1.5 + assert saved["repeater"]["node_name"] == "node-b" + assert saved["repeater"]["mode"] == "monitor" + assert saved["radio"]["frequency"] == 915125000 + assert saved["radio"]["bandwidth"] == 250000 + assert saved["repeater"]["security"]["guest_password"] == "gpw" + assert saved["repeater"]["security"]["admin_password"] == "newadminpw" + assert saved["repeater"]["security"]["allow_read_only"] is True + assert saved["repeater"]["send_advert_interval_hours"] == 12 + assert saved["delays"]["rx_delay_base"] == 4.5 + assert saved["delays"]["tx_delay_factor"] == 1.5 + assert saved["delays"]["direct_tx_delay_factor"] == 0.25 From 1bbd2156a17440949123913db36d2cabcdea6c14 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:11:49 -0700 Subject: [PATCH 6/9] fix(trace): gate intermediate-hop trace relay on forward mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Firmware only relays a TRACE at an intermediate hop when allowPacketForward passes, so disabling forwarding stops trace relay. The trace helper forwarded via packet injection, which is gated only by the local-TX check — a repeater in monitor mode kept repeating traces while reporting repeat off. Consult the repeater mode before relaying; locally originated pings are injected directly and keep working in monitor mode, and ping-response matching still runs before the relay decision. --- repeater/handler_helpers/trace.py | 21 +++++- ...t_handler_helpers_trace_discovery_login.py | 64 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/repeater/handler_helpers/trace.py b/repeater/handler_helpers/trace.py index d90a4c4..c1acf40 100644 --- a/repeater/handler_helpers/trace.py +++ b/repeater/handler_helpers/trace.py @@ -135,11 +135,16 @@ class TraceHelper: logger.info(f"Path SNRs: [{', '.join(path_snrs)}], Hashes: [{', '.join(path_hashes)}]") should_forward = self._should_forward_trace(packet, trace_bytes, flags, hash_width) + mode_blocked = should_forward and not self._relay_allowed() + if mode_blocked: + should_forward = False + logger.info("Trace relay suppressed (repeat disabled)") if should_forward: await self._forward_trace_packet(packet, num_hops) else: - self._log_no_forward_reason(packet, trace_bytes, hash_width) + if not mode_blocked: + self._log_no_forward_reason(packet, trace_bytes, hash_width) if ( self.on_trace_complete and self._is_trace_complete(packet, trace_bytes, hash_width) @@ -271,6 +276,20 @@ class TraceHelper: return path_snrs, path_hashes + def _relay_allowed(self) -> bool: + """Firmware gates intermediate-hop TRACE relay on ``allowPacketForward`` + (Mesh.cpp), so a repeater that is not forwarding (monitor / no_tx mode) + must not relay traces. Locally originated pings are injected directly + and never pass through this gate. + """ + config = getattr(self.repeater_handler, "config", None) + mode = "forward" + if isinstance(config, dict): + mode = config.get("repeater", {}).get("mode", "forward") + if mode not in ("forward", "monitor", "no_tx"): + mode = "forward" + return mode == "forward" + def _should_forward_trace( self, packet, trace_bytes: bytes, flags: int, hash_width: int ) -> bool: diff --git a/tests/test_handler_helpers_trace_discovery_login.py b/tests/test_handler_helpers_trace_discovery_login.py index 9333f3b..ff9aa2d 100644 --- a/tests/test_handler_helpers_trace_discovery_login.py +++ b/tests/test_handler_helpers_trace_discovery_login.py @@ -201,6 +201,70 @@ async def test_trace_helper_forward_trace_packet_updates_recent_record_and_injec packet_injector.assert_awaited_once() +def _trace_relay_helper(mode, forward=True): + """TraceHelper wired so _should_forward_trace says yes; mode comes from config.""" + repeater_handler = MagicMock() + repeater_handler.is_duplicate.return_value = False + repeater_handler.calculate_packet_score.return_value = 0.9 + repeater_handler.config = {"repeater": {"mode": mode}} + helper = TraceHelper( + local_hash=0x42, + local_identity=FakeIdentity(0x42), + repeater_handler=repeater_handler, + ) + helper._forward_trace_packet = AsyncMock() + helper._extract_path_info = MagicMock(return_value=([], [])) + helper._should_forward_trace = MagicMock(return_value=forward) + helper.trace_handler._parse_trace_payload = MagicMock( + return_value={ + "valid": True, + "trace_path_bytes": b"\x42\x43", + "flags": 0, + "trace_hops": [b"\x42", b"\x43"], + "trace_path": [0x42, 0x43], + "tag": 1234, + } + ) + helper.trace_handler._format_trace_response = MagicMock(return_value="trace ok") + return helper + + +@pytest.mark.asyncio +async def test_trace_relay_suppressed_in_monitor_and_no_tx_mode(): + # Firmware gates TRACE relay on allowPacketForward, so a repeater with + # repeat off must not relay an intermediate-hop trace. + for mode in ("monitor", "no_tx"): + helper = _trace_relay_helper(mode) + packet = DummyPacket(path=b"\x01", payload=b"\xaa\xbb\xcc") + await helper.process_trace_packet(packet) + helper._forward_trace_packet.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_trace_relay_allowed_in_forward_and_unknown_mode(): + for mode in ("forward", "weird-mode"): + helper = _trace_relay_helper(mode) + packet = DummyPacket(path=b"\x01", payload=b"\xaa\xbb\xcc") + await helper.process_trace_packet(packet) + helper._forward_trace_packet.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_trace_ping_response_still_matches_in_monitor_mode(): + # Ping origination/response handling is not relay: a pending ping must + # resolve even when the repeater is not forwarding. + helper = _trace_relay_helper("monitor", forward=False) + tag = 555 + helper.trace_handler._parse_trace_payload.return_value["tag"] = tag + evt = helper.register_ping(tag, 0x42) + + packet = DummyPacket(path=b"\x01", payload=b"\xaa\xbb\xcc") + await helper.process_trace_packet(packet) + + assert evt.is_set() + helper._forward_trace_packet.assert_not_awaited() + + def test_trace_helper_cleanup_stale_pings(): helper = TraceHelper( local_hash=0x42, local_identity=FakeIdentity(0x42), repeater_handler=MagicMock() From bf72735233ad97733930aca330768a230721366e Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:12:10 -0700 Subject: [PATCH 7/9] fix(router): keep offering local candidates when a companion bridge raises _consume_via_local_candidates awaited the targeted companion bridge with no exception guard, so a raising bridge aborted the whole candidate loop: the hash-colliding room-server or repeater identity registered at the same one-byte dest hash was never offered the packet, and an unclaimed packet never fell through to the forwarding engine. Wrap the bridge call with the same log-and-continue handling the fan-out path already uses. --- repeater/packet_router.py | 12 +++++++--- tests/test_packet_router.py | 46 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/repeater/packet_router.py b/repeater/packet_router.py index d08b9eb..197975d 100644 --- a/repeater/packet_router.py +++ b/repeater/packet_router.py @@ -316,9 +316,15 @@ class PacketRouter: consumed = False if has_companion: - bridge_result = await companion_bridges[dest_hash].process_received_packet(packet) - if bridge_result.authenticated: - consumed = True + # A raising bridge must not abort the candidate loop: the colliding + # room-server / repeater identity below still gets offered the packet. + try: + bridge_result = await companion_bridges[dest_hash].process_received_packet(packet) + except Exception as e: + logger.debug("Companion bridge candidate error: %s", e) + else: + if bridge_result.authenticated: + consumed = True # Offer to the room-server / repeater identity when it shares the hash # (collision) or when no local companion claims it at all (normal # server-owned + remote-forward handling). diff --git a/tests/test_packet_router.py b/tests/test_packet_router.py index 0f0d7b7..4d2456f 100644 --- a/tests/test_packet_router.py +++ b/tests/test_packet_router.py @@ -1321,3 +1321,49 @@ class TestInjectedTxRawEcho(unittest.IsolatedAsyncioTestCase): ok = await router.inject_packet(_make_packet()) self.assertTrue(ok) + + +class TestCompanionDeliveryFailureHandling(unittest.IsolatedAsyncioTestCase): + """Dedupe marking and candidate-loop behaviour when companion bridges raise.""" + + async def test_login_candidate_bridge_error_still_offers_local_identity(self): + """A raising companion bridge must not abort the candidate loop: the + hash-colliding room-server/repeater identity still gets the packet.""" + daemon = _make_daemon() + bridge = _make_bridge() + bridge.process_received_packet = AsyncMock(side_effect=RuntimeError("bridge down")) + daemon.companion_bridges = {0xF5: bridge} + daemon.login_helper = MagicMock() + daemon.login_helper.handlers = {0xF5: MagicMock()} + daemon.login_helper.process_login_packet = AsyncMock(return_value=True) + daemon.repeater_handler = AsyncMock() + daemon.repeater_handler.storage = MagicMock() + daemon.repeater_handler.record_packet_only = MagicMock() + router = PacketRouter(daemon) + pkt = _make_packet(LoginServerHandler.payload_type()) + pkt.payload = bytes([0xF5, 0x01]) + + await router._route_packet(pkt) + + bridge.process_received_packet.assert_awaited_once() + daemon.login_helper.process_login_packet.assert_awaited_once() + # The colliding identity consumed it, so the engine must not re-forward. + daemon.repeater_handler.assert_not_awaited() + + async def test_text_candidate_bridge_error_leaves_packet_for_engine(self): + """Bridge raises and no local identity claims the text: the packet must + still reach the forwarding engine instead of dying with the exception.""" + daemon = _make_daemon() + bridge = _make_bridge() + bridge.process_received_packet = AsyncMock(side_effect=RuntimeError("bridge down")) + daemon.companion_bridges = {0xF5: bridge} + daemon.text_helper = MagicMock() + daemon.text_helper.handlers = {} + daemon.text_helper.process_text_packet = AsyncMock(return_value=False) + router = PacketRouter(daemon) + pkt = _make_packet(TextMessageHandler.payload_type()) + pkt.payload = bytes([0xF5, 0x01]) + + await router._route_packet(pkt) + + daemon.repeater_handler.assert_awaited_once() From 0a65e0566b29a288dc3eac798ca5ee9bee6be78b Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:12:24 -0700 Subject: [PATCH 8/9] fix(router): mark companion delivery only after a bridge receives the packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PATH and protocol-response paths recorded the companion dedupe unconditionally after the bridge fan-out, so a delivery where every bridge raised was still suppressed for the full dedupe TTL — the client lost the packet even though later mesh copies arrived. Have the fan-out report delivery (at least one bridge completed without raising) next to authentication, and mark the dedupe only on delivery, which is the behaviour the marking helper's contract already documented. One healthy bridge still counts as delivered, so duplicate suppression of repeated mesh copies is unchanged. --- repeater/packet_router.py | 47 ++++++++++++++++++------------ tests/test_packet_router.py | 57 +++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/repeater/packet_router.py b/repeater/packet_router.py index 197975d..122d379 100644 --- a/repeater/packet_router.py +++ b/repeater/packet_router.py @@ -273,14 +273,21 @@ class PacketRouter: return {} return companion_bridges - async def _fan_out_to_bridges(self, packet, bridges, *, context: str) -> bool: - """Offer packet to each bridge; True if any bridge authenticated it. + async def _fan_out_to_bridges(self, packet, bridges, *, context: str) -> tuple: + """Offer packet to each bridge; report ``(delivered, authenticated)``. Accepts a dict of bridges — pass a single-entry dict for targeted delivery to the bridge that owns ``dest_hash``. A bridge that raises is logged and skipped; ``result.authenticated`` is read directly (every bridge returns a HandlerResult) so a broken contract surfaces instead of being hidden. + + ``delivered`` is True when at least one bridge completed without raising — + the signal callers must use before ``_mark_delivered_to_companions`` so a + delivery where every bridge raised is retried on the next copy instead of + being suppressed for the dedupe TTL. ``authenticated`` is True when any + bridge authenticated the packet. """ + delivered = False authenticated = False for bridge in bridges.values(): try: @@ -288,9 +295,10 @@ class PacketRouter: except Exception as e: logger.debug("Companion bridge %s error: %s", context, e) continue + delivered = True if result.authenticated is True: authenticated = True - return authenticated + return delivered, authenticated async def _consume_via_local_candidates( self, packet, metadata: dict, dest_hash, helper, process_method_name: str @@ -638,21 +646,21 @@ class PacketRouter: companion_bridges = self._companion_bridges_for_packet(packet, metadata) if dest_hash is not None and dest_hash in companion_bridges: if not self._was_delivered_to_companions(packet): - consumed = ( - await self._fan_out_to_bridges( - packet, {dest_hash: companion_bridges[dest_hash]}, context="PATH" - ) - or consumed + delivered, authenticated = await self._fan_out_to_bridges( + packet, {dest_hash: companion_bridges[dest_hash]}, context="PATH" ) - self._mark_delivered_to_companions(packet) + consumed = authenticated or consumed + if delivered: + self._mark_delivered_to_companions(packet) elif companion_bridges and not self._was_delivered_to_companions(packet): # Dest not in bridges: path-return with ephemeral dest (e.g. multi-hop login). # Deliver to all bridges; each will try to decrypt and ignore if not relevant. - consumed = ( - await self._fan_out_to_bridges(packet, companion_bridges, context="PATH") - or consumed + delivered, authenticated = await self._fan_out_to_bridges( + packet, companion_bridges, context="PATH" ) - self._mark_delivered_to_companions(packet) + consumed = authenticated or consumed + if delivered: + self._mark_delivered_to_companions(packet) logger.debug( "PATH dest=0x%02x (anon) delivered to %d bridge(s) for matching", dest_hash or 0, @@ -674,13 +682,13 @@ class PacketRouter: companion_bridges = self._companion_bridges_for_packet(packet, metadata) local_hash = getattr(self.daemon, "local_hash", None) if dest_hash is not None and dest_hash in companion_bridges: - consumed = await self._fan_out_to_bridges( + _, consumed = await self._fan_out_to_bridges( packet, {dest_hash: companion_bridges[dest_hash]}, context="RESPONSE" ) logger.info("RESPONSE dest=0x%02x delivered to companion bridge", dest_hash) elif dest_hash == local_hash and companion_bridges: # Response addressed to this repeater (e.g. path-based reply to first hop) - consumed = await self._fan_out_to_bridges( + _, consumed = await self._fan_out_to_bridges( packet, companion_bridges, context="RESPONSE" ) logger.info( @@ -692,7 +700,7 @@ class PacketRouter: # Dest not in bridges and not local: likely ANON_REQ response (dest = ephemeral # sender hash). Deliver to all bridges; each will try to decrypt and ignore if # not relevant (firmware-like behavior, works with multiple companion bridges). - consumed = await self._fan_out_to_bridges( + _, consumed = await self._fan_out_to_bridges( packet, companion_bridges, context="RESPONSE" ) logger.debug( @@ -717,8 +725,11 @@ class PacketRouter: companion_bridges = self._companion_bridges_for_packet(packet, metadata) final_hop = _is_direct_final_hop(packet) if companion_bridges and (final_hop or not self._was_delivered_to_companions(packet)): - await self._fan_out_to_bridges(packet, companion_bridges, context="RESPONSE") - self._mark_delivered_to_companions(packet) + delivered, _ = await self._fan_out_to_bridges( + packet, companion_bridges, context="RESPONSE" + ) + if delivered: + self._mark_delivered_to_companions(packet) if companion_bridges and final_hop: # DIRECT with empty path: we're the final hop, so consume after delivery. processed_by_injection = True diff --git a/tests/test_packet_router.py b/tests/test_packet_router.py index 4d2456f..a1e46f5 100644 --- a/tests/test_packet_router.py +++ b/tests/test_packet_router.py @@ -1326,6 +1326,63 @@ class TestInjectedTxRawEcho(unittest.IsolatedAsyncioTestCase): class TestCompanionDeliveryFailureHandling(unittest.IsolatedAsyncioTestCase): """Dedupe marking and candidate-loop behaviour when companion bridges raise.""" + async def test_path_all_bridges_raising_is_retried_on_next_copy(self): + """A PATH delivery where every bridge raised must not be marked + delivered; the next mesh copy gets another delivery attempt.""" + daemon = _make_daemon() + bridge = _make_bridge() + bridge.process_received_packet = AsyncMock(side_effect=RuntimeError("bridge down")) + daemon.companion_bridges = {0x01: bridge} + router = PacketRouter(daemon) + pkt = _make_packet(PathHandler.payload_type()) + pkt.payload = bytes([0x01, 0xAA]) + + await router._route_packet(pkt) + self.assertEqual(bridge.process_received_packet.await_count, 1) + + # Bridge recovers; the second copy must be delivered, not TTL-suppressed. + recovered = AsyncMock(return_value=HandlerResult.not_for_us()) + bridge.process_received_packet = recovered + await router._route_packet(pkt) + recovered.assert_awaited_once() + + async def test_path_partial_bridge_failure_still_marks_delivered(self): + """One healthy bridge is a delivery: the duplicate copy stays suppressed.""" + daemon = _make_daemon() + raising = _make_bridge() + raising.process_received_packet = AsyncMock(side_effect=RuntimeError("boom")) + healthy = _make_bridge() + healthy.process_received_packet = AsyncMock(return_value=HandlerResult.not_for_us()) + daemon.companion_bridges = {0x01: raising, 0x02: healthy} + router = PacketRouter(daemon) + pkt = _make_packet(PathHandler.payload_type()) + # Dest not in bridges: anon path-return, delivered to all bridges. + pkt.payload = bytes([0xEE, 0xAA]) + + await router._route_packet(pkt) + await router._route_packet(pkt) + + healthy.process_received_packet.assert_awaited_once() + raising.process_received_packet.assert_awaited_once() + + async def test_protocol_response_all_bridges_raising_is_retried_on_next_copy(self): + daemon = _make_daemon() + bridge = _make_bridge() + bridge.process_received_packet = AsyncMock(side_effect=RuntimeError("bridge down")) + daemon.companion_bridges = {0x01: bridge} + router = PacketRouter(daemon) + pkt = _make_packet(ProtocolResponseHandler.payload_type()) + pkt.header = ROUTE_TYPE_FLOOD # not a final hop: dedupe decides delivery + + with patch("repeater.packet_router.PathHandler.payload_type", return_value=0x55): + await router._route_packet(pkt) + self.assertEqual(bridge.process_received_packet.await_count, 1) + + recovered = AsyncMock(return_value=HandlerResult.not_for_us()) + bridge.process_received_packet = recovered + await router._route_packet(pkt) + recovered.assert_awaited_once() + async def test_login_candidate_bridge_error_still_offers_local_identity(self): """A raising companion bridge must not abort the candidate loop: the hash-colliding room-server/repeater identity still gets the packet.""" From 8fcd7902c2ff8c822bfffbfa3e4e80af4145c49d Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 07:40:08 -0700 Subject: [PATCH 9/9] docs(cli): state the zero semantics of flood.max in the set help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenHop treats max_flood_hops 0 as unlimited while firmware's flood.max 0 forwards nothing — the inverse. Until that edge is decided alongside the policy-engine flood caps, say so in the help so a firmware-habituated admin does not set 0 expecting to disable forwarding (that is what 'set repeat off' is for) and silently get unlimited flooding instead. --- repeater/handler_helpers/mesh_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/repeater/handler_helpers/mesh_cli.py b/repeater/handler_helpers/mesh_cli.py index de34220..40b7ee2 100644 --- a/repeater/handler_helpers/mesh_cli.py +++ b/repeater/handler_helpers/mesh_cli.py @@ -354,7 +354,7 @@ class MeshCLI: " set allow.read.only on|off Read-only access\n" " set advert.interval 60-240 minutes\n" " set flood.advert.interval
3-168 hours\n" - " set flood.max Max flood hops (max 64)\n" + " set flood.max Max flood hops (max 64; 0 = unlimited, unlike firmware)\n" " set path.hash.mode <0-2> Path hash mode (0=1B,1=2B,2=3B)\n" " set loop.detect Flood loop detection\n" " set rxdelay RX delay base (0-20)\n"