From 66bbf5b4737f8693ed2067cf20867823b2ca5630 Mon Sep 17 00:00:00 2001 From: Logan Cusano Date: Sun, 20 Sep 2026 20:22:42 -0400 Subject: [PATCH] intelligence: don't reject a geocode just because it is far from the node (#159) _geocode_location's node-distance fallback (geocode_max_km, 40km) is a proxy for "is this plausible" that only makes sense when the node's own position is the best guess we have at the area. It doesn't hold for a citywide/patched feed: node-002 sits in Westchester but relays "New York City - NYPD Citywide 2 Patch", ~56km from the addresses on it. Every real, correctly-geocoded address on that talkgroup was rejected by this check, every time -- location_coords stayed permanently null for the whole system, which killed the location_proximity correlation signal and let duplicate incidents form for the same event reported at two nearby addresses two minutes apart ("Shots Fired at Jackson Avenue" / "Shots Fired at 1108 Jackson Avenue", 2026-09-20 ~23:00 UTC, merged by hand via the Archive page's attach/detach while this fix went in). trust_named_region skips the node-distance rejection exactly when the query already carries a place name that isn't the node's own position -- operator-set area_context, or a municipality parsed from the talkgroup's own name. The anchor path is untouched; an anchor's own radius is always authoritative when one has been resolved. Also fixes a compounding defect found while verifying: the query was grafting the node's own COUNTY onto an already-self-named region ("...New York City..., Westchester, New York"), which is self-contradictory and could degrade the geocode independent of the distance check. The node's county is now used only when nothing else names the place; state stays in both branches since it's coarse enough to be correct either way. Extracted the query-assembly logic into a pure, unit-tested helper (_location_query_parts) rather than testing it only through the full extraction pipeline. Filed #160 as a follow-up: place_verifier.py's verify() has the identical no-anchor gap for transcript place-name correction, not fixed here. Verified: 422 pass, 0 fail (9 new tests). Co-Authored-By: Claude Sonnet 5 --- drb-c2-core/app/internal/intelligence.py | 96 +++++++-- .../tests/test_geocode_named_region.py | 185 ++++++++++++++++++ 2 files changed, 268 insertions(+), 13 deletions(-) create mode 100644 drb-c2-core/tests/test_geocode_named_region.py diff --git a/drb-c2-core/app/internal/intelligence.py b/drb-c2-core/app/internal/intelligence.py index a673421..d263454 100644 --- a/drb-c2-core/app/internal/intelligence.py +++ b/drb-c2-core/app/internal/intelligence.py @@ -368,18 +368,20 @@ async def extract_scenes( # the country. location_coords: Optional[dict] = None if location: - parts = [location] - if tg_area.get("municipality") or tg_area.get("county") or tg_area.get("state"): - parts += [tg_area[f] for f in area_context.PLACE_FIELDS if tg_area.get(f)] - elif node_lat is not None and node_lon is not None: - muni = _municipality_from_tg(talkgroup_name) - state = await _get_node_state(node_id or "", node_lat, node_lon) if node_id else "" - county = _node_county_cache.get(node_id or "") if node_id else "" - parts += [p for p in (muni, county, state) if p] + node_state, node_county = "", "" + if not area_context.has_place(tg_area) and node_id and node_lat is not None and node_lon is not None: + # Only worth the (cached-after-first-call) reverse-geocode + # when nothing better already describes this talkgroup. + node_state = await _get_node_state(node_id, node_lat, node_lon) + node_county = _node_county_cache.get(node_id) or "" + parts, tg_named_region = _location_query_parts( + location, tg_area, talkgroup_name, node_state, node_county, + ) query = ", ".join(parts) if tg_anchor or (node_lat is not None and node_lon is not None): location_coords = await _geocode_location( - query, node_lat, node_lon, anchor=tg_anchor + query, node_lat, node_lon, anchor=tg_anchor, + trust_named_region=tg_named_region, ) # Embed this scene's content @@ -514,6 +516,7 @@ async def _geocode_location( node_lat: Optional[float] = None, node_lon: Optional[float] = None, anchor: Optional[dict] = None, + trust_named_region: bool = False, ) -> Optional[dict]: """ Geocode using Google Maps Geocoding API, biased toward the channel's area. @@ -529,6 +532,26 @@ async def _geocode_location( talkgroup has a resolved anchor, that is the reference and its own radius is the bound. Distance-from-node stays only as the fallback for a system nobody has described yet — it was always a stand-in for this. + + server-26#159: "a system nobody has described yet" turned out to include + systems that describe themselves — "New York City - NYPD Citywide 2 Patch" + names its own coverage area right in the talkgroup name, parsed into the + query by `_municipality_from_tg`, but a large aggregated/patched feed like + this is routinely received 40-70km from an antenna that happens to sit + wherever the node owner lives. Real, correctly-geocoded addresses on that + feed were being rejected by the node-distance check every single time — + location_coords stayed permanently null for the whole system, which killed + the location_proximity correlation signal and let duplicate incidents form + for the same event reported at two nearby addresses two minutes apart. + + `trust_named_region` is True exactly when the query already carries a place + name that isn't the node's own position — operator-set area_context, or a + municipality parsed from the talkgroup's own name. In that case a distant + node is not evidence of a bad geocode, so the node-distance check is + skipped and precision is judged by `location_type` alone (still required + to be ROOFTOP/RANGE_INTERPOLATED/GEOMETRIC_CENTER, below). This does not + touch the anchor path at all — an anchor's own radius is always authoritative + when one has been resolved. """ import httpx from app.config import settings @@ -594,11 +617,21 @@ async def _geocode_location( lat, lng = float(loc["lat"]), float(loc["lng"]) dist_km = _geo_dist_km(ref_lat, ref_lon, lat, lng) if dist_km > max_km: - logger.warning( - f"Geocoding rejected '{location_str}' → ({lat:.4f}, {lng:.4f}) " - f"— {dist_km:.1f}km from {ref_label} exceeds {max_km:.1f}km" + # server-26#159: the node-distance bound is a proxy for "is + # this plausible" that only makes sense when the node's own + # position is our best guess at the area — never when the + # query already names a different region on its own terms. + if not (ref_label == "node" and trust_named_region): + logger.warning( + f"Geocoding rejected '{location_str}' → ({lat:.4f}, {lng:.4f}) " + f"— {dist_km:.1f}km from {ref_label} exceeds {max_km:.1f}km" + ) + return None + logger.info( + f"Geocoding '{location_str}' → ({lat:.4f}, {lng:.4f}) is " + f"{dist_km:.1f}km from the receiving node, past {max_km:.1f}km — " + f"accepted anyway: the query names its own region, not the node's" ) - return None coords = {"lat": lat, "lng": lng} logger.info( f"Geocoded '{location_str}' → {coords} " @@ -624,6 +657,43 @@ def _municipality_from_tg(tg_name: Optional[str]) -> Optional[str]: return cleaned +def _location_query_parts( + location: str, + tg_area: dict, + talkgroup_name: Optional[str], + node_state: str, + node_county: str, +) -> tuple[list[str], bool]: + """ + Build the geocode query parts for `location`, plus whether the query names + a region the *talkgroup itself* covers (operator-set area_context, or a + municipality parsed from the talkgroup's own name) rather than one guessed + from wherever the receiving node happens to sit (server-26#159). + + That distinction matters downstream: `_geocode_location`'s node-distance + sanity check is only a valid proxy for "is this plausible" when the node's + own position is the best guess we have at the area. A citywide/patched + feed ("New York City - NYPD Citywide 2 Patch") names its own coverage area + right in the talkgroup name — grafting the node's own county onto that + (Ossining-style: valid when the feed genuinely is local to the node, + actively wrong when it names a distant region of its own) would make the + query self-contradictory, so the node's COUNTY is used only when nothing + better names the place. The node's STATE is coarse enough to still be + correct either way and is kept in both branches. + """ + parts = [location] + if area_context.has_place(tg_area): + parts += [tg_area[f] for f in area_context.PLACE_FIELDS if tg_area.get(f)] + return parts, True + + muni = _municipality_from_tg(talkgroup_name) + if muni: + parts += [p for p in (muni, node_state) if p] + else: + parts += [p for p in (node_county, node_state) if p] + return parts, muni is not None + + def _build_transcript_block(transcript: str, segments: Optional[list[dict]]) -> str: """Format transcript as numbered transmissions if segments are available.""" if segments and len(segments) > 1: diff --git a/drb-c2-core/tests/test_geocode_named_region.py b/drb-c2-core/tests/test_geocode_named_region.py new file mode 100644 index 0000000..76f3802 --- /dev/null +++ b/drb-c2-core/tests/test_geocode_named_region.py @@ -0,0 +1,185 @@ +""" +server-26#159: a citywide/patched feed can be received far from its own +coverage area — "New York City - NYPD Citywide 2 Patch" was ~56km from the +receiving node, well past geocode_max_km (40km). Real, correctly-geocoded +addresses on that talkgroup were rejected by intelligence._geocode_location's +node-distance sanity check every time, so location_coords never populated for +the whole system: location_proximity correlation was permanently dead there, +and the same real event reported at two nearby addresses two minutes apart +became two separate incidents instead of one. + +`trust_named_region` fixes this narrowly: the node-distance check is a proxy +for "is this plausible" that only makes sense when the node's own position is +the best guess we have at the area. It must not apply when the query already +names a different region on its own terms (operator-set area_context, or a +municipality parsed straight from the talkgroup's own name) — and it must +never touch the anchor path, whose own radius is always authoritative. +""" +from unittest.mock import patch + +import pytest + +import app.internal.intelligence as intel +from app.config import settings + + +def _maps_result(lat: float, lng: float, location_type: str = "ROOFTOP"): + payload = { + "status": "OK", + "results": [{ + "geometry": { + "location": {"lat": lat, "lng": lng}, + "location_type": location_type, + }, + }], + } + + class _Resp: + def raise_for_status(self): pass + def json(self): return payload + + class _Client: + async def __aenter__(self): return self + async def __aexit__(self, *a): return False + async def get(self, *a, **k): return _Resp() + + return patch("httpx.AsyncClient", lambda *a, **k: _Client()) + + +@pytest.fixture(autouse=True) +def _api_key(): + # intelligence.py imports settings locally per-function (`from app.config + # import settings`), which binds the same cached singleton — patching the + # module-level object here reaches it, but `intel.settings` itself does + # not exist as an attribute. + with patch.object(settings, "google_maps_api_key", "test-key"): + yield + + +# Node at (0, 0); result at (1, 0) is ~111km away — well past the 40km default. +NODE_LAT, NODE_LON = 0.0, 0.0 +FAR_LAT, FAR_LNG = 1.0, 0.0 + + +@pytest.mark.asyncio +async def test_named_region_geocode_accepted_beyond_node_distance(): + with _maps_result(FAR_LAT, FAR_LNG): + coords = await intel._geocode_location( + "1108 Jackson Avenue, New York City - NYPD Citywide 2 Patch", + node_lat=NODE_LAT, node_lon=NODE_LON, + trust_named_region=True, + ) + assert coords == {"lat": FAR_LAT, "lng": FAR_LNG} + + +@pytest.mark.asyncio +async def test_local_geocode_still_rejected_beyond_node_distance_without_named_region(): + """Regression guard: a bare street name with no named region still uses + the node as its only plausibility check, exactly as before this fix.""" + with _maps_result(FAR_LAT, FAR_LNG): + coords = await intel._geocode_location( + "Main Street", + node_lat=NODE_LAT, node_lon=NODE_LON, + trust_named_region=False, + ) + assert coords is None + + +@pytest.mark.asyncio +async def test_anchor_path_ignores_trust_named_region(): + """The anchor's own radius is always authoritative — trust_named_region + is only a statement about the node fallback, never a way to widen an + anchor that was itself deliberately sized to discriminate.""" + anchor = {"lat": NODE_LAT, "lng": NODE_LON, "radius_km": 10.0} + with _maps_result(FAR_LAT, FAR_LNG): + coords = await intel._geocode_location( + "1108 Jackson Avenue, New York City - NYPD Citywide 2 Patch", + node_lat=NODE_LAT, node_lon=NODE_LON, + anchor=anchor, trust_named_region=True, + ) + assert coords is None + + +@pytest.mark.asyncio +async def test_named_region_geocode_within_node_distance_is_unaffected(): + """A close result is accepted the same way regardless of the flag.""" + near_lat, near_lng = 0.05, 0.0 # ~5.5km from the node + with _maps_result(near_lat, near_lng): + coords = await intel._geocode_location( + "Main Street, Ossining, New York", + node_lat=NODE_LAT, node_lon=NODE_LON, + trust_named_region=True, + ) + assert coords == {"lat": near_lat, "lng": near_lng} + + +@pytest.mark.asyncio +async def test_imprecise_result_still_rejected_regardless_of_trust(): + """trust_named_region relaxes the distance check only — the location_type + precision filter (server-26#37) still applies unconditionally.""" + with _maps_result(FAR_LAT, FAR_LNG, location_type="APPROXIMATE"): + coords = await intel._geocode_location( + "1108 Jackson Avenue, New York City - NYPD Citywide 2 Patch", + node_lat=NODE_LAT, node_lon=NODE_LON, + trust_named_region=True, + ) + assert coords is None + + +# --------------------------------------------------------------------------- +# _location_query_parts — pure query assembly, no HTTP involved +# --------------------------------------------------------------------------- + +def test_operator_configured_area_wins_and_is_named_region(): + parts, named = intel._location_query_parts( + "High Street", {"municipality": "Yorktown", "state": "New York"}, + "Tac 1", node_state="New York", node_county="Westchester", + ) + assert parts == ["High Street", "Yorktown", "New York"] + assert named is True + + +def test_local_talkgroup_name_gets_node_state_but_not_node_county(): + """ + "Ossining PD" is genuinely local to the node, so appending the node's own + state is correct. Its COUNTY is dropped even here — server-26#159's fix + applies uniformly once a municipality is derived, since there is no way + to tell "local" and "distant-but-node-adjacent" apart from the string + alone, and the county was never necessary for a bare municipality name + that already disambiguates via the state. + """ + parts, named = intel._location_query_parts( + "High Street", {}, "Ossining PD", + node_state="New York", node_county="Westchester", + ) + assert parts == ["High Street", "Ossining", "New York"] + assert named is True + + +def test_citywide_patched_feed_does_not_get_the_nodes_county_grafted_on(): + """ + server-26#159's actual production case: the talkgroup names its own + (distant) region, so the node's county (Westchester, ~56km away) must not + be appended — it would make the query self-contradictory ("...New York + City..., Westchester, New York") and risks degrading the geocode result's + precision independently of the distance check this issue also fixes. + """ + parts, named = intel._location_query_parts( + "1108 Jackson Avenue", {}, "New York City - NYPD Citywide 2 Patch", + node_state="New York", node_county="Westchester", + ) + assert "Westchester" not in parts + assert parts == ["1108 Jackson Avenue", "New York City - NYPD Citywide 2 Patch", "New York"] + assert named is True + + +def test_uninformative_talkgroup_name_falls_back_to_node_county_and_state(): + """A tactical channel or bare code gives _municipality_from_tg nothing — + the only remaining evidence really is where the node sits, so the + original node-county-and-state fallback is preserved for this case.""" + parts, named = intel._location_query_parts( + "High Street", {}, "Tac 1", + node_state="New York", node_county="Westchester", + ) + assert parts == ["High Street", "Westchester", "New York"] + assert named is False