diff --git a/BUGS.md b/BUGS.md index 3d2577c..ee0dfcf 100644 --- a/BUGS.md +++ b/BUGS.md @@ -12,7 +12,6 @@ Issues closed in this release. See `CHANGELOG.md` for the full feature/infra lis | Reference | Summary | |-----------|---------| -| Issue #80 | `find_recent_rf_data` no longer mis-correlates `message_stats.path`. When a `correlation_key` is supplied it returns `None` instead of falling back to an arbitrary recent sample. Correlation stays strictly observation-level: the returned sample's `snr`, `rssi`, `raw_hex`, and `routing_info` all belong to the same decoded wire copy (no cross-forward merging). The no-key fallback is constrained to a narrow `rf_fallback_window` (2s default). Regression tests in `tests/test_message_handler.py::TestFindRecentRfData`. | | Issue #83 | `KeyError('msg_hash')` asyncio parser spam fixed upstream by `meshcore >= 2.3.6` (new `meshcore_parser.py` guards with `'msg_hash' in l`). | | Issue #91 | Bot `!version` command added and version string shown in the web-viewer footer (`883b67d`, `fbf3995`). | | Issue #126 | `can't convert negative int to unsigned` on flood contacts fixed upstream by `meshcore >= 2.3.6` (out_path_len `-1` → `255` before packing). Defensive wire-field rebuild in `_ensure_contact_meshcore_path_encoding` (`ba52c3b`). | diff --git a/CHANGELOG.md b/CHANGELOG.md index f32ec3b..523c87a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,7 +73,6 @@ hardening fixes. single-message output (`46d3fab`). - CI log-injection regression check (`ce4fa8e`); lint gates for ruff, mypy, eslint, and shellcheck (`e1cf2eb` / `a12797f`). -- Configurable `rf_fallback_window` for RF correlation narrow fallback (issue #80). ### Changed @@ -104,14 +103,6 @@ hardening fixes. ### Fixed -- **#80**: `find_recent_rf_data` no longer falls back to an arbitrary recent RF - sample when a `correlation_key` is provided — it now returns `None` instead - of attaching `message_stats.path` to an unrelated flood packet. Correlation - stays strictly observation-level: the returned sample's `snr`, `rssi`, - `raw_hex`, and `routing_info` all belong to the same decoded wire copy. - The no-key fallback is also constrained to a narrow `rf_fallback_window` - (2 s default) to prevent DMs from picking up unrelated flood-traffic - routing info. - **#126** (negative `out_path_len`): fixed via `meshcore >= 2.3.6` dep bump plus defensive handling in `_ensure_contact_meshcore_path_encoding`. - **#83** (`KeyError('msg_hash')` asyncio spam): fixed via `meshcore >= 2.3.6` dep diff --git a/TODO.md b/TODO.md index 2c6dfe0..380384f 100644 --- a/TODO.md +++ b/TODO.md @@ -210,7 +210,6 @@ Dispositions recorded at v0.9.0 tagging. Revisit for v0.9.1. - Issue #160 — web viewer feed channel edit not persisting - Issue #137 — minimal-config install "not responding" (needs reporter repro with logs first) - TASK-00 — meshcore `IndexError` asyncio handler polish (BUG-022 base fix already shipped; revisit the paused refactor) -- **Path enrichment from same-`packet_hash` flood forwards (option C)** — follow-up to #80. When `find_recent_rf_data` returns the exact `packet_prefix` match, enrich its `routing_info` with the longest observed path from any cached sample sharing the same `packet_hash`, and simultaneously sync `message.hops` to the enriched `path_length` so `message_stats` rows stay internally consistent. Gives the `!path` command and mesh-graph updates the most complete route evidence without the hops/path skew that the naive upgrade would introduce. Touch points: `MessageHandler.find_recent_rf_data`, plus the two sites that copy `recent_rf_data['routing_info']` onto `MeshMessage` (lines ~426 and ~2081 in `modules/message_handler.py`). Regression tests should cover: (a) SNR/RSSI stay pinned to the matched observation, (b) `message.hops` == upgraded `path_length`, (c) no enrichment when `packet_hash` is missing. ### Stale PR triage diff --git a/modules/message_handler.py b/modules/message_handler.py index ced386b..59148a0 100644 --- a/modules/message_handler.py +++ b/modules/message_handler.py @@ -50,9 +50,6 @@ class MessageHandler: self.rf_data_timeout = float(bot.config.get('Bot', 'rf_data_timeout', fallback='15.0')) self.message_timeout = float(bot.config.get('Bot', 'message_correlation_timeout', fallback='10.0')) self.enhanced_correlation = bot.config.getboolean('Bot', 'enable_enhanced_correlation', fallback=True) - # Narrow window for the "most recent" fallback when no correlation_key is provided - # (issue #80: prevents attaching unrelated flood packets' routing info to DMs). - self.rf_fallback_window = float(bot.config.get('Bot', 'rf_fallback_window', fallback='2.0')) # Time-based cache for recent RF log data self.recent_rf_data: list[dict[str, Any]] = [] @@ -1178,26 +1175,12 @@ class MessageHandler: def find_recent_rf_data( self, correlation_key: str | None = None, max_age_seconds: float | None = None ) -> dict[str, Any] | None: - """Find recent RF data for SNR/RSSI and packet decoding. + """Find recent RF data for SNR/RSSI and packet decoding with improved correlation Args: correlation_key: Can be either: - packet_prefix (from raw_hex[:32]) for RF data correlation - pubkey_prefix (from message payload) for message correlation - When provided but no match is found, this returns ``None`` instead of - falling back to an arbitrary recent sample (issue #80). - max_age_seconds: Maximum age of RF samples to consider. Defaults to - ``rf_data_timeout``. - - When ``correlation_key`` is ``None`` (e.g. DM SNR/RSSI lookup), this returns - the most recent sample within a narrow ``rf_fallback_window`` so unrelated - flood-forwarded packets' routing info can't bleed into DMs. - - The returned sample is always a single wire observation — its ``snr``, - ``rssi``, ``raw_hex`` and ``routing_info`` all belong to the same decoded - copy. No cross-forward merging is performed (see TODO for option C, - which would enrich ``routing_info`` from longer-path forwards of the same - ``packet_hash`` while also syncing ``message.hops`` for consistency). """ import time current_time = time.time() @@ -1214,59 +1197,40 @@ class MessageHandler: self.logger.debug(f"No recent RF data found within {max_age_seconds}s window") return None + # Strategy 1: Try exact packet prefix match first (for RF data correlation) if correlation_key: - # Strategy 1: exact packet prefix match — observation-level identity. - # Both the message event and the RF-log event carry the same raw_hex for - # the specific wire copy that was decoded, so this is the authoritative - # match for SNR/RSSI/path attribution. for data in recent_data: rf_packet_prefix = data.get('packet_prefix', '') or '' if rf_packet_prefix == correlation_key: self.logger.debug(f"Found exact packet prefix match: {rf_packet_prefix}") return data - # Strategy 2: exact pubkey prefix match (for message correlation). + # Strategy 2: Try pubkey prefix match (for message correlation) + if correlation_key: for data in recent_data: rf_pubkey_prefix = data.get('pubkey_prefix', '') or '' if rf_pubkey_prefix == correlation_key: self.logger.debug(f"Found exact pubkey prefix match: {rf_pubkey_prefix}") return data - # Strategy 3: partial packet prefix match (≥ 16 chars). + # Strategy 3: Try partial packet prefix matches + if correlation_key: for data in recent_data: rf_packet_prefix = data.get('packet_prefix', '') or '' + # Check for partial match (at least 16 characters) min_length = min(len(rf_packet_prefix), len(correlation_key), 16) - if min_length >= 16 and rf_packet_prefix[:min_length] == correlation_key[:min_length]: - self.logger.debug( - f"Found partial packet prefix match: {rf_packet_prefix[:16]}... " - f"matches {correlation_key[:16]}..." - ) + if (rf_packet_prefix[:min_length] == correlation_key[:min_length] and min_length >= 16): + self.logger.debug(f"Found partial packet prefix match: {rf_packet_prefix[:16]}... matches {correlation_key[:16]}...") return data - # Issue #80: when a correlation_key was provided but nothing matched, do NOT - # fall back to an arbitrary recent sample — that's how message_stats.path ends - # up attached to unrelated flood packets. - self.logger.debug( - f"No RF match for correlation_key={correlation_key[:16]}...; returning None" - ) - return None + # Strategy 4: Use most recent data (fallback for timing issues) + if recent_data: + most_recent = max(recent_data, key=lambda x: x['timestamp']) + packet_prefix = most_recent.get('packet_prefix', 'unknown') + self.logger.debug(f"Using most recent RF data (fallback): {packet_prefix} at {most_recent['timestamp']}") + return most_recent - # No correlation_key provided (e.g. DM SNR/RSSI lookup). Use a narrow window to - # minimise the chance of grabbing unrelated flood traffic. - fallback_cutoff = current_time - self.rf_fallback_window - narrow = [d for d in recent_data if d['timestamp'] >= fallback_cutoff] - if not narrow: - self.logger.debug( - f"No RF data within {self.rf_fallback_window}s fallback window" - ) - return None - most_recent = max(narrow, key=lambda x: x['timestamp']) - packet_prefix = most_recent.get('packet_prefix', 'unknown') - self.logger.debug( - f"Using most recent RF data (narrow fallback): {packet_prefix} " - f"at {most_recent['timestamp']}" - ) - return most_recent + return None def store_message_for_correlation(self, message_id: str, message_data: dict[str, Any]) -> None: """Store a message temporarily to wait for RF data correlation""" diff --git a/tests/test_message_handler.py b/tests/test_message_handler.py index 1d3632e..7cb4652 100644 --- a/tests/test_message_handler.py +++ b/tests/test_message_handler.py @@ -341,24 +341,14 @@ class TestCleanupStaleCacheEntries: class TestFindRecentRfData: """Tests for MessageHandler.find_recent_rf_data().""" - def _rf_entry(self, age=0, packet_prefix="aabbccdd", pubkey_prefix="1122", - packet_hash=None, path_length=None, snr=5, rssi=-80, - path_hex=None): - entry = { + def _rf_entry(self, age=0, packet_prefix="aabbccdd", pubkey_prefix="1122"): + return { "timestamp": time.time() - age, - "snr": snr, - "rssi": rssi, + "snr": 5, + "rssi": -80, "packet_prefix": packet_prefix, "pubkey_prefix": pubkey_prefix, } - if packet_hash is not None: - entry["packet_hash"] = packet_hash - if path_length is not None: - routing: dict = {"path_length": path_length} - if path_hex is not None: - routing["path_hex"] = path_hex - entry["routing_info"] = routing - return entry def test_returns_none_when_empty(self, handler): handler.recent_rf_data = [] @@ -415,84 +405,9 @@ class TestFindRecentRfData: handler.recent_rf_data = [entry] # With max_age=5, entry is too old assert handler.find_recent_rf_data(max_age_seconds=5) is None - # With no correlation_key, the narrow fallback window applies even if - # max_age is large — so a 20s-old entry is still rejected. - handler.rf_fallback_window = 2.0 - assert handler.find_recent_rf_data(max_age_seconds=30) is None - # Expand the fallback window and the entry becomes visible again. - handler.rf_fallback_window = 30.0 + # With max_age=30, entry is visible assert handler.find_recent_rf_data(max_age_seconds=30) is entry - # --- Issue #80 regression tests ----------------------------------------- - - def test_unmatched_correlation_key_returns_none(self, handler): - """Issue #80: when correlation_key is provided but nothing matches, return - None instead of silently attaching an unrelated recent packet.""" - handler.rf_data_timeout = 30 - unrelated = self._rf_entry(age=1, packet_prefix="ffffffff", pubkey_prefix="ffff") - handler.recent_rf_data = [unrelated] - result = handler.find_recent_rf_data("deadbeefdeadbeef1234567890abcdef") - assert result is None - - def test_flood_duplicate_returns_exact_match_not_longer(self, handler): - """Issue #80 (option B): even when multiple RF samples share a - ``packet_hash`` via flood forwarding, ``find_recent_rf_data`` returns - the exact ``packet_prefix`` match verbatim — no cross-forward merging. - The matched sample's ``snr``/``rssi``/``routing_info`` all belong to the - same wire observation that was decoded.""" - handler.rf_data_timeout = 30 - matched = self._rf_entry( - age=2, packet_prefix="aabb" * 8, - packet_hash="9D272188AB364743", path_length=4, path_hex="f230df22", - snr=3, rssi=-90, - ) - longer = self._rf_entry( - age=1, packet_prefix="ccdd" * 8, - packet_hash="9D272188AB364743", path_length=5, path_hex="f230df2279", - snr=12, rssi=-60, - ) - handler.recent_rf_data = [matched, longer] - result = handler.find_recent_rf_data(matched["packet_prefix"]) - - assert result is matched - # Path and signal fields stay anchored to the matched observation — no - # enrichment from the longer forward. - assert result["routing_info"]["path_length"] == 4 - assert result["routing_info"]["path_hex"] == "f230df22" - assert result["snr"] == 3 - assert result["rssi"] == -90 - - def test_narrow_fallback_window_rejects_stale(self, handler): - """Issue #80: with no correlation_key, only very recent samples qualify - for the fallback — unrelated older flood traffic is ignored.""" - handler.rf_data_timeout = 30 - handler.rf_fallback_window = 2.0 - stale = self._rf_entry(age=10) - handler.recent_rf_data = [stale] - assert handler.find_recent_rf_data() is None - - def test_issue_80_sequence_does_not_mis_correlate(self, handler): - """Replay the #80 scenario: the decoded message's correlation_key does not - match any observed RF sample; a completely unrelated 1-hop packet arrived - most recently. We must NOT return that unrelated sample.""" - handler.rf_data_timeout = 30 - message_sample_short = self._rf_entry( - age=3, packet_prefix="aaaa" * 8, - packet_hash="9D272188AB364743", path_length=4, - ) - message_sample_long = self._rf_entry( - age=2, packet_prefix="bbbb" * 8, - packet_hash="9D272188AB364743", path_length=5, - ) - unrelated_recent = self._rf_entry( - age=1, packet_prefix="cccc" * 8, - packet_hash="60FA21CC81C77423", path_length=1, - ) - handler.recent_rf_data = [message_sample_short, message_sample_long, unrelated_recent] - # The message's correlation_key doesn't match anything in our cache. - result = handler.find_recent_rf_data("ffffffffffffffffffffffffffffffff") - assert result is None, "Unmatched correlation_key must not fall back to an unrelated packet" - # --------------------------------------------------------------------------- # handle_raw_data