fix: collision-aware !path hop resolution

Bot_Usage.md

@@ -887,8 +887,14 @@ in one reply. Arguments:
   pin is still generated: `dist unavailable (1/total), <url>`.
 - Zero located hops → `dist/map unavailable (0/total)`; a direct (no-hop)
   message → `direct (no path)`.
-- Reliability note: 1-byte path hashes collide (many repeaters share a first
-  byte), so results are most reliable on 2- and 3-byte paths.
+- Reliability note: path hashes can collide (several repeaters share the same
+  leading byte(s)), so results are most reliable on 2- and 3-byte paths. Hops
+  resolve only against repeater/room contacts. When a hash matches more than
+  one repeater, the hop is placed only if a candidate is clearly the closest
+  fit to a known anchor (the bot's own location, the sender, or an
+  unambiguous neighbouring hop); otherwise it is left unlocated rather than
+  risk using a far repeater that merely shares the hash. Setting the bot's own
+  location on the radio improves this disambiguation.
 
 - 10s per-user cooldown; no auth; works in DM and any allowed channel.
 - Makes a da.gd shortener call per invocation (when ≥2 hops are located).

commands/path.py

@@ -50,18 +50,98 @@ def _haversine(lat1, lon1, lat2, lon2, unit):
     return 2 * r * math.asin(math.sqrt(a))
 
 
-async def _resolve_hop_named(ctx, hop_hex):
-    # "map a path-hop hex prefix to (lat, lon, name), or None when it
-    # doesn't resolve to exactly one contact carrying a location.
+def _num(v):
+    # device_info values are JSON-encoded; decode to float (None if missing).
+    try:
+        return float(json.loads(v))
+    except (TypeError, ValueError):
+        return None
+
+
+async def _bot_location(ctx):
+    # the bot's own advertised location (degrees), or None if unset (0/0).
     rows = await ctx.bot.db.fetchall(
-        "SELECT adv_lat, adv_lon, adv_name FROM contacts "
-        "WHERE substr(public_key,1,?)=? "
+        "SELECT key, value FROM device_info "
+        "WHERE key IN ('self_info.adv_lat','self_info.adv_lon')"
+    )
+    d = {r["key"]: r["value"] for r in rows}
+    lat = _num(d.get("self_info.adv_lat"))
+    lon = _num(d.get("self_info.adv_lon"))
+    if lat is None or lon is None or (lat == 0 and lon == 0):
+        return None
+    return (lat, lon)
+
+
+async def _sender_location(ctx):
+    # best-effort far-end anchor: the message sender's advertised location.
+    pk = ctx.sender_pubkey
+    if not pk:
+        return None
+    row = await ctx.bot.db.fetchone(
+        "SELECT adv_lat, adv_lon FROM contacts WHERE public_key=? "
         "AND adv_lat IS NOT NULL AND adv_lat!=0 AND adv_lon!=0",
-        (len(hop_hex), hop_hex),
+        (pk.lower(),),
     )
-    if len(rows) == 1:
-        return (rows[0]["adv_lat"], rows[0]["adv_lon"], rows[0]["adv_name"])
-    return None
+    return (row["adv_lat"], row["adv_lon"]) if row else None
+
+
+async def _resolve_hops(ctx, hops):
+    """Resolve each path hop to (lat, lon, name) or None.
+
+    Path hops are repeater/room hashes (the leading bytes of the repeater's
+    pubkey), which can collide across contacts. The old resolver matched any
+    contact type and trusted "exactly one located contact" — which silently
+    picked the WRONG repeater whenever only the wrong one (e.g. a far node
+    pulled in by tropospheric ducting) carried a location while the correct
+    local hop had none. Now:
+      - only repeater/room contacts are candidates (clients never repeat);
+      - a hop whose hash matches a single contact is trusted, even if distant
+        (a unique hash is unambiguous);
+      - a colliding hop is placed only when >= 2 of its candidates carry a
+        location and one is the clear nearest to a trusted anchor (the bot's
+        own location, the sender, or an already-unambiguous hop). A lone
+        located candidate among a collision is ambiguous and left unlocated,
+        so a wrong distance is never shown.
+    """
+    per_hop = []
+    for h in hops:
+        rows = await ctx.bot.db.fetchall(
+            "SELECT adv_lat, adv_lon, adv_name FROM contacts "
+            "WHERE type IN (2, 3) AND substr(public_key,1,?)=?",
+            (len(h), h),
+        )
+        located = [
+            (r["adv_lat"], r["adv_lon"], r["adv_name"])
+            for r in rows
+            if r["adv_lat"] not in (None, 0) and r["adv_lon"] not in (None, 0)
+        ]
+        per_hop.append((len(rows), located))
+
+    anchors = []
+    for loc in (await _bot_location(ctx), await _sender_location(ctx)):
+        if loc:
+            anchors.append(loc)
+
+    resolved = [None] * len(hops)
+    # pass 1: unambiguous hops (single matching contact) are trusted and seed
+    # the anchor set used to disambiguate any collisions.
+    for i, (n, located) in enumerate(per_hop):
+        if n == 1 and located:
+            resolved[i] = located[0]
+            anchors.append((located[0][0], located[0][1]))
+
+    # pass 2: collisions — choose the located candidate nearest a trusted
+    # anchor, but only when at least two candidates are located (otherwise the
+    # right one can't be told apart from the impostor → leave unlocated).
+    for i, (n, located) in enumerate(per_hop):
+        if n > 1 and len(located) >= 2 and anchors:
+            resolved[i] = min(
+                located,
+                key=lambda p: min(
+                    _haversine(p[0], p[1], a[0], a[1], "mi") for a in anchors
+                ),
+            )
+    return resolved
 
 
 def _pin_symbol(i):
@@ -193,8 +273,9 @@ async def handle(ctx):
     path_str = ",".join(hops)
     nh = len(hops)
 
-    # resolve hops with names for the map markers
-    named = [await _resolve_hop_named(ctx, h) for h in hops]
+    # resolve hops with names for the map markers (collision-aware; see
+    # _resolve_hops — a far repeater sharing a path hash no longer wins)
+    named = await _resolve_hops(ctx, hops)
     loc = [p for p in named if p]
     n_located = len(loc)
 

tests/test_path_resolve.py

@@ -0,0 +1,170 @@
+#!/usr/bin/env python3
+"""Collision-aware path-hop resolution for !path.
+
+A path hop is the leading bytes of a repeater's pubkey and can collide across
+contacts. The resolver must never let a far repeater that merely shares the
+hash supply a bogus location (the tropospheric-ducting bug), while still
+trusting unambiguous hops and disambiguating multi-located collisions by
+proximity to the bot.
+
+Run: /home/steve/dev/meshcore/meshcore-bot/venv/bin/python tests/test_path_resolve.py
+"""
+
+import asyncio
+import importlib.util
+import json
+import sys
+from pathlib import Path
+from types import SimpleNamespace
+
+ROOT = Path(__file__).resolve().parent.parent
+sys.path.insert(0, str(ROOT))
+
+import mcbot  # noqa: E402
+
+# commands/ is not a package (the bot loads plugins dynamically), so load the
+# module straight from its file.
+_spec = importlib.util.spec_from_file_location(
+    "pathcmd", ROOT / "commands" / "path.py"
+)
+pathcmd = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(pathcmd)
+
+_failures = 0
+
+
+def check(cond, msg):
+    global _failures
+    print(f"  {'ok' if cond else 'FAIL'}: {msg}")
+    if not cond:
+        _failures += 1
+
+
+def make_bot():
+    cfg = mcbot.Config()
+    cfg.db_path = Path(":memory:")
+    log = mcbot.logging.getLogger("test-path")
+    log.addHandler(mcbot.logging.NullHandler())
+    log.propagate = False
+    return mcbot.MCBot(cfg, log)
+
+
+def pk(prefix):
+    # full 64-hex pubkey whose leading bytes are `prefix`
+    return prefix + "0" * (64 - len(prefix))
+
+
+async def add_contact(bot, public_key, *, type_=2, lat=None, lon=None, name=""):
+    await bot.db.execute(
+        "INSERT INTO contacts (public_key, adv_name, type, adv_lat, adv_lon) "
+        "VALUES (?,?,?,?,?)",
+        (public_key, name, type_, lat, lon),
+    )
+
+
+async def set_bot_location(bot, lat, lon):
+    # device_info values are JSON-encoded, mirroring _upsert_device_info
+    for k, v in (("self_info.adv_lat", lat), ("self_info.adv_lon", lon)):
+        await bot.db.execute(
+            "INSERT INTO device_info(key,value,last_updated) VALUES(?,?,0)",
+            (k, json.dumps(v)),
+        )
+
+
+def ctx_for(bot, sender_pubkey=None):
+    return SimpleNamespace(bot=bot, sender_pubkey=sender_pubkey)
+
+
+async def test_collision_only_far_has_geo_is_unlocated():
+    print("test_collision_only_far_has_geo_is_unlocated")
+    bot = make_bot()
+    await set_bot_location(bot, 32.0, -96.0)  # Texas
+    # the reported bug: local hop (correct) has no geo; a far repeater sharing
+    # the 2-byte hash has geo. The far one must NOT be used.
+    await add_contact(bot, pk("abcd0"), name="local", lat=None, lon=None)
+    await add_contact(bot, pk("abcd1"), name="louisiana", lat=30.0, lon=-92.0)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["abcd"])
+    check(res == [None], f"colliding hop with one geo'd impostor -> unlocated (got {res})")
+    bot.db.close()
+
+
+async def test_collision_two_geo_picks_nearest_to_bot():
+    print("test_collision_two_geo_picks_nearest_to_bot")
+    bot = make_bot()
+    await set_bot_location(bot, 32.0, -96.0)
+    await add_contact(bot, pk("abcd0"), name="near", lat=32.1, lon=-96.1)
+    await add_contact(bot, pk("abcd1"), name="far", lat=30.0, lon=-92.0)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["abcd"])
+    check(res[0] is not None and res[0][2] == "near",
+          f"nearest-to-bot candidate wins (got {res[0]})")
+    bot.db.close()
+
+
+async def test_unique_hop_trusted_even_if_far():
+    print("test_unique_hop_trusted_even_if_far")
+    bot = make_bot()
+    await set_bot_location(bot, 32.0, -96.0)
+    # a single repeater matches the hash -> unambiguous, trust it even if far
+    await add_contact(bot, pk("ef01"), name="solo", lat=12.3, lon=45.6)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["ef01"])
+    check(res[0] is not None and res[0][2] == "solo", "unique hop resolved")
+    bot.db.close()
+
+
+async def test_non_repeater_excluded():
+    print("test_non_repeater_excluded")
+    bot = make_bot()
+    await set_bot_location(bot, 32.0, -96.0)
+    # a companion (type 1) with geo must not satisfy a repeater hop
+    await add_contact(bot, pk("1234"), type_=1, name="client", lat=31.0, lon=-95.0)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["1234"])
+    check(res == [None], "companion-type contact not used as a hop")
+    # add a real repeater sharing the prefix -> now it (not the client) wins
+    await add_contact(bot, pk("12349"), type_=2, name="rep", lat=31.5, lon=-95.5)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["1234"])
+    check(res[0] is not None and res[0][2] == "rep", "repeater chosen over client")
+    bot.db.close()
+
+
+async def test_unique_geoless_hop_is_unlocated():
+    print("test_unique_geoless_hop_is_unlocated")
+    bot = make_bot()
+    await add_contact(bot, pk("5678"), name="noloc", lat=None, lon=None)
+    res = await pathcmd._resolve_hops(ctx_for(bot), ["5678"])
+    check(res == [None], "unique repeater with no geo -> unlocated")
+    bot.db.close()
+
+
+async def test_sender_anchor_used_when_no_bot_location():
+    print("test_sender_anchor_used_when_no_bot_location")
+    bot = make_bot()  # no bot location set
+    sender = pk("9999")
+    await add_contact(bot, sender, type_=1, name="sender", lat=30.0, lon=-92.0)
+    # collision with two geo'd candidates; only the sender anchors the choice
+    await add_contact(bot, pk("abcd0"), name="near-sender", lat=30.2, lon=-92.2)
+    await add_contact(bot, pk("abcd1"), name="far", lat=40.0, lon=-110.0)
+    res = await pathcmd._resolve_hops(ctx_for(bot, sender_pubkey=sender), ["abcd"])
+    check(res[0] is not None and res[0][2] == "near-sender",
+          f"sender anchors disambiguation when bot loc absent (got {res[0]})")
+    bot.db.close()
+
+
+async def main():
+    for t in (
+        test_collision_only_far_has_geo_is_unlocated,
+        test_collision_two_geo_picks_nearest_to_bot,
+        test_unique_hop_trusted_even_if_far,
+        test_non_repeater_excluded,
+        test_unique_geoless_hop_is_unlocated,
+        test_sender_anchor_used_when_no_bot_location,
+    ):
+        await t()
+    print()
+    if _failures:
+        print(f"FAILED: {_failures} check(s)")
+        sys.exit(1)
+    print("ALL TESTS PASSED")
+
+
+if __name__ == "__main__":
+    asyncio.run(main())