From deeff7ed1f51a80efee6729167ba845a4af2861b Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sun, 26 Jul 2026 23:42:36 +0300 Subject: [PATCH] fix(tradein/estimator): Tier S radius-fallback house-number boundary + distance_m MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit finding 1 (medium): the Tier S same-building radius-fallback matched `address ILIKE short_addr + '%'` with no boundary after the house number, so "ул. Ленина, 5%" collided with "..., 50/51/500/5а" (a different building). MAX_ANALOGS_PER_ADDRESS=5 + `len(tier_s) >= 3` meant one stray building could poison the same-building median. Fix reuses the already-vetted anchor Tier A machinery (_normalize_building_key + new shared _house_boundary_regex helper) instead of a bare string prefix, and replaces the hardcoded `0.0 AS distance_m` with a real ST_Distance so anomalously-far "same building" matches are visible in the data. _extract_short_addr is left in place (unused by _fetch_analogs now, own tests still pass) with a note explaining the supersession, rather than deleted, to keep this PR scoped to the boundary bug. Audit finding 2 (low): conflict_resolution.py's resolve_house_field/ resolve_listing_field priority system is confirmed dead in the production merge path (matching/houses.py + listings.py use a simpler COALESCE newest-wins pattern instead) per vault Decision_774_Matching_Architecture (2026-05-31, code-archaeology + live-DB verified). Documented this in the module docstring instead of deleting — removal + its ~30 dedicated tests is already scoped as an independent "Path 2 / Sub-3" cleanup in that decision, kept separate from this unrelated Tier-S bugfix. Not wired into the price-calc path per audit instructions. Tests: new tests/test_estimator_tier_s_house_boundary.py (boundary regex + SQL-rendering checks); test_same_building_match.py updated to assert the new boundary-regex SQL shape instead of the old bare ILIKE. Full suite: 2632 passed, 8 skipped, 1 deselected (same deselect as ci-tradein.yml). --- tradein-mvp/backend/app/services/estimator.py | 92 +++++++++----- .../services/matching/conflict_resolution.py | 19 +++ .../test_estimator_tier_s_house_boundary.py | 117 ++++++++++++++++++ .../backend/tests/test_same_building_match.py | 8 +- 4 files changed, 205 insertions(+), 31 deletions(-) create mode 100644 tradein-mvp/backend/tests/test_estimator_tier_s_house_boundary.py diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index c8bc30ab..d533bb6e 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -1681,6 +1681,19 @@ def _normalize_building_key( return street_core, base_no, letter +def _house_boundary_regex(base_no: int, letter: str | None) -> str: + """Numeric-boundary regex для «того же дома» (общий для Tier A anchor и Tier S radius). + + Дом 204 не матчит 2040/1204 (нет цифры-соседа слева/справа); литера при + наличии обязательна (204г ≠ 204д) — без неё сравниваем «голый» номер. + Корпус «/N» допускаем (тот же дом, слэш явно разрешён альтернативой в классе). + Применяется через SQL `~` к `lower(translate(address, 'ёЁ', 'ее'))`. + """ + if letter: + return rf"(^|[^0-9]){base_no}\s*{letter}([^а-яёa-z0-9/]|/|$)" + return rf"(^|[^0-9]){base_no}([^а-яёa-z0-9/]|/|$)" + + def _anchor_comp_from_row(r: Any) -> dict[str, Any]: """Строит comp-dict из строки SQL same-building/micro-radius (#694). @@ -1740,13 +1753,10 @@ def _fetch_anchor_comps( # ── Tier A: same building ──────────────────────────────────────────────── street, base_no, letter = _normalize_building_key(address) if street and base_no is not None: - # Numeric-boundary regex: дом 204 не матчит 2040/1204; литера при наличии - # обязательна (204г ≠ 204д). Корпус «/N» допускаем (тот же дом). ё→е в SQL - # для symmetry с нормализатором. psycopg v3: bind через :param, оператор ~. - if letter: - house_re = rf"(^|[^0-9]){base_no}\s*{letter}([^а-яёa-z0-9/]|/|$)" - else: - house_re = rf"(^|[^0-9]){base_no}([^а-яёa-z0-9/]|/|$)" + # ё→е в SQL для symmetry с нормализатором. psycopg v3: bind через :param, + # оператор ~. Boundary-regex вынесен в _house_boundary_regex (общий с + # Tier S radius-fallback ниже, см. _fetch_analogs). + house_re = _house_boundary_regex(base_no, letter) try: rows = ( db.execute( @@ -4295,6 +4305,14 @@ _TRAILING_NOISE_RE = re.compile( def _extract_short_addr(full_address: str | None) -> str | None: """Извлекает «улица + номер дома» из полного адреса для поиска в том же доме. + NOTE (audit fix): `_fetch_analogs` Tier S-fallback больше НЕ вызывает эту + функцию — заменена на `_normalize_building_key` + `_house_boundary_regex` + (numeric-boundary matching, тот же принцип что и anchor Tier A), т.к. голый + строковый prefix без границы номера дома схлопывал «Ленина, 5» с «Ленина, + 50/51/500». Оставлена как standalone-утилита (own tests в + test_extract_short_addr.py) — не удалена на случай будущего переиспользования + простого street+house извлечения без полной normalize_building_key семантики. + Примеры: "Свердловская область, г. Екатеринбург, ул. Заводская, д. 44-а" → "ул. Заводская, д. 44-а" "Россия, Екатеринбург, ул. Малышева, 1" → "ул. Малышева, 1" @@ -4582,7 +4600,9 @@ def _fetch_analogs( **Tier S (same building):** сначала канонический match по house_id_fk (если задан target_house_id) — детерминированно «тот же дом»; fallback — - address ILIKE prefix-match по short_addr. Если ≥3 результатов → tier='S'. + normalized street + numeric-boundary house-number regex (см. + _normalize_building_key + _house_boundary_regex, тот же принцип что и anchor + Tier A) + ST_DWithin(radius_m). Если ≥3 результатов → tier='S'. **Tier H (same class):** PostGIS + rooms + area + year ±15 + total_floors ±30%. Если ≥5 результатов → возвращаем; tier='H'. @@ -4686,26 +4706,36 @@ def _fetch_analogs( ) return _stratify_candidates(tier_sc), radius_m > DEFAULT_RADIUS_M, "S" - # ── Tier S (fallback): same building via address prefix ─────────────────── - # #oblast-D geo-bound fix: _extract_short_addr strips the city/admin prefix - # ("Нижний Тагил, улица Ленина, 100" → "улица Ленина, 100"), so an - # address-only ILIKE prefix match can collide with an IDENTICALLY-NAMED - # street+house-number in a COMPLETELY DIFFERENT city (e.g. "улица Ленина, - # 100" exists verbatim in Берёзовский — ~40 191 of ~40 200 active listings - # are EKB-region, so any such collision silently resolves to a distant EKB - # listing). A Нижний Тагил / Серов / Каменск subject then got "same - # building" comps from a listing ~100+ km away with distance_m hardcoded to - # 0.0 (masking the gap). Tier S's OWN semantics ("same building") already - # imply the match must be near the subject, so we now require ST_DWithin on - # the SAME radius_m the caller passed in (mirrors Tier H/W below) — this - # can only DROP false-positive cross-city matches, never legitimate - # same-building EKB hits (>99.9% of active listings carry lat/lon). - short_addr = _extract_short_addr(full_address) + # ── Tier S (fallback): same building via normalized street + house-number ── + # #oblast-D geo-bound fix: a bare address-prefix match can collide with an + # IDENTICALLY-NAMED street+house-number in a COMPLETELY DIFFERENT city (e.g. + # "улица Ленина, 100" exists verbatim in Берёзовский — ~40 191 of ~40 200 + # active listings are EKB-region, so any such collision silently resolves + # to a distant EKB listing). A Нижний Тагил / Серов / Каменск subject then + # got "same building" comps from a listing ~100+ km away. Tier S's OWN + # semantics ("same building") already imply the match must be near the + # subject, so we require ST_DWithin on the SAME radius_m the caller passed + # in (mirrors Tier H/W below) — this can only DROP false-positive + # cross-city matches, never legitimate same-building EKB hits (>99.9% of + # active listings carry lat/lon). + # + # Numeric-boundary fix (audit finding): the OLD implementation matched + # `address ILIKE short_addr + '%'` — a bare prefix with NO boundary after + # the house number, so "ул. Ленина, 5%" collided with "..., 50", "..., 51", + # "..., 500", "..., 5а" (a DIFFERENT building one street-address away). + # ONE such stray building could push len(tier_s) >= 3 and silently poison + # the same-building median. Reuse the anchor Tier A machinery instead + # (_normalize_building_key + _house_boundary_regex, estimator.py ~1741) — + # already proven/tested there — so the fallback requires the SAME + # numeric-boundary regex match, not just a naive string prefix. + street, base_no, letter = _normalize_building_key(full_address) - if short_addr: + if street and base_no is not None: + house_re = _house_boundary_regex(base_no, letter) tier_s_params = { **base_params, - "short_addr_prefix": short_addr + "%", + "street_like": "%" + street + "%", + "house_re": house_re, "lat": lat, "lon": lon, "radius": radius_m, @@ -4718,14 +4748,16 @@ def _fetch_analogs( WITH base AS ( SELECT {_ANALOG_SELECT_COLS}, - 0.0 AS distance_m, + ST_Distance(geom::geography, ST_MakePoint(:lon, :lat)::geography) + AS distance_m, 0.0 AS relevance_score, row_number() OVER ( PARTITION BY address ORDER BY scraped_at DESC ) AS rn_addr, {_RN_DUP_WINDOW} FROM listings - WHERE address ILIKE :short_addr_prefix + WHERE lower(translate(address, 'ёЁ', 'ее')) LIKE :street_like + AND lower(translate(address, 'ёЁ', 'ее')) ~ :house_re AND ST_DWithin(geom::geography, ST_MakePoint(:lon, :lat)::geography, :radius) {_COMMON_WHERE} ) @@ -4752,8 +4784,10 @@ def _fetch_analogs( tier_s = [dict(r) for r in tier_s_rows] if len(tier_s) >= 3: logger.info( - "analogs tier=S addr_prefix=%r → %d results", - short_addr, + "analogs tier=S street=%r base=%s letter=%s → %d results", + street, + base_no, + letter, len(tier_s), ) return _stratify_candidates(tier_s), radius_m > DEFAULT_RADIUS_M, "S" diff --git a/tradein-mvp/backend/app/services/matching/conflict_resolution.py b/tradein-mvp/backend/app/services/matching/conflict_resolution.py index cd0a83c1..712ece35 100644 --- a/tradein-mvp/backend/app/services/matching/conflict_resolution.py +++ b/tradein-mvp/backend/app/services/matching/conflict_resolution.py @@ -5,6 +5,25 @@ Source-of-truth dicts read by merge logic in match_or_create_house/listing. Sources covered: avito (serp/detail/houses_catalog/domoteka/imv), cian (serp/bti/detail/stats/valuation), yandex (serp/detail/realty_nb/valuation). + +STATUS (audit finding, confirmed against vault Decision_774_Matching_Architecture, +2026-05-31, code-archaeology + live-DB verified): `resolve_house_field` / +`resolve_listing_field` / `HOUSE_FIELD_PRIORITY` / `LISTING_FIELD_PRIORITY` are +NOT called anywhere in the production merge path — real house/listing upserts +in `matching/houses.py` / `matching/listings.py` use a simpler ad hoc +`COALESCE(EXCLUDED.x, table.x)` (newest-non-null-wins) pattern instead. +`update_canonical_fields` below is a deliberate Stage-8-v1 no-op stub ("Full +arbitration deferred to Stage 8.x" — see its own docstring); this is NOT an +accidentally-orphaned integration, it is an intentionally-staged one that never +got a Stage-8.x follow-up. Decision_774 already scoped removing this +(`resolve_*`/stub/`match_or_create_listing`) as an independent "Path 2 / Sub-3" +cleanup PR, deliberately kept separate from the accuracy-affecting Path 1a work +(house_id_fk anchor) — do NOT wire this into the live price-calc path without a +dedicated backtest+A/B (would change client-facing estimates). Left in place +(not deleted) here because ~30 existing tests in +`tests/matching/test_conflict_resolution.py` + `tests/test_matching.py` cover +it in detail; removing both belongs in that separate Sub-3 PR, not bundled with +an unrelated Tier-S bugfix. """ from __future__ import annotations diff --git a/tradein-mvp/backend/tests/test_estimator_tier_s_house_boundary.py b/tradein-mvp/backend/tests/test_estimator_tier_s_house_boundary.py new file mode 100644 index 00000000..36dbfbdc --- /dev/null +++ b/tradein-mvp/backend/tests/test_estimator_tier_s_house_boundary.py @@ -0,0 +1,117 @@ +"""Audit finding #1 — Tier S radius-fallback house-number boundary. + +Old bug: `_fetch_analogs` Tier S-fallback matched `address ILIKE short_addr + '%'` +with NO boundary after the house number, so "ул. Ленина, 5%" collided with +"..., 50" / "..., 51" / "..., 500" / "..., 5а" (a DIFFERENT building one street +address away). MAX_ANALOGS_PER_ADDRESS=5 + the `len(tier_s) >= 3` threshold meant +a single stray building could poison the same-building median. + +Fix reuses the anchor Tier A machinery (`_normalize_building_key` + +`_house_boundary_regex`, estimator.py ~1741) instead of a bare string prefix — +same numeric-boundary regex principle, now shared by both tiers. +""" + +from __future__ import annotations + +import os +import re +from typing import Any +from unittest.mock import MagicMock + +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +from app.services.estimator import _fetch_analogs, _house_boundary_regex + + +def _norm(addr: str) -> str: + return addr.lower().replace("ё", "е") + + +# ── _house_boundary_regex: pure boundary semantics ─────────────────────────── + + +def test_no_letter_matches_bare_number_and_corpus_slash() -> None: + pat = _house_boundary_regex(5, None) + assert re.search(pat, _norm("ул. Ленина, 5")) + assert re.search(pat, _norm("ул. Ленина, 5/2")) # корпус-слэш — тот же дом + + +def test_no_letter_rejects_longer_numbers_sharing_prefix() -> None: + """Ядро бага: «Ленина, 5» не должен матчить 50/51/500/15 (соседние дома).""" + pat = _house_boundary_regex(5, None) + for other in ("ул. Ленина, 50", "ул. Ленина, 51", "ул. Ленина, 500", "ул. Ленина, 15"): + assert not re.search(pat, _norm(other)), other + + +def test_no_letter_rejects_lettered_variant() -> None: + """Без литеры (5) НЕ матчит корпус-литеру (5а) — разные дома (симметрично + Tier A: «литера при наличии обязательна», 204г ≠ 204д ≠ 204).""" + pat = _house_boundary_regex(5, None) + assert not re.search(pat, _norm("ул. Ленина, 5а")) + + +def test_letter_requires_matching_letter() -> None: + pat = _house_boundary_regex(204, "г") + assert re.search(pat, _norm("8 Марта, 204г")) + assert not re.search(pat, _norm("8 Марта, 204д")) + assert not re.search(pat, _norm("8 Марта, 204")) + + +# ── _fetch_analogs Tier S-fallback: SQL uses boundary regex, not bare ILIKE ── + + +def _capture_calls(full_address: str | None) -> list[tuple[Any, ...]]: + db = MagicMock() + captured: list[tuple[Any, ...]] = [] + + def side_effect(*args: Any, **kwargs: Any) -> MagicMock: + captured.append(args) + result = MagicMock() + result.mappings.return_value.all.return_value = [] + return result + + db.execute.side_effect = side_effect + _fetch_analogs( + db, + lat=56.84, + lon=60.65, + rooms=2, + area=50.0, + radius_m=1000, + full_address=full_address, + target_house_id=None, + ) + return captured + + +def test_tier_s_fallback_uses_boundary_regex_not_bare_ilike() -> None: + calls = _capture_calls("Екатеринбург, ул. Ленина, 5") + assert calls, "Tier S-fallback должен выполнить хотя бы один запрос" + sql_text, params = str(calls[0][0]), calls[0][1] + assert "ILIKE" not in sql_text + assert ":house_re" in sql_text + assert ":street_like" in sql_text + assert params["house_re"] == _house_boundary_regex(5, None) + assert params["street_like"] == "%ленина%" + + +def test_tier_s_fallback_distance_m_no_longer_hardcoded_zero() -> None: + """Второй audit-пункт: distance_m больше не 0.0-хардкод, а реальный ST_Distance.""" + calls = _capture_calls("Екатеринбург, ул. Ленина, 5") + sql_text = str(calls[0][0]) + assert "0.0 AS distance_m" not in sql_text + assert "ST_Distance(geom::geography" in sql_text + + +def test_tier_s_fallback_no_house_number_skips_block() -> None: + """Адрес без номера дома → base_no=None → Tier S-fallback пропущен (не крашится, + падаем дальше по Tier H/W).""" + calls = _capture_calls("р-н Центр, улица Бориса Ельцина") + for args in calls: + assert ":house_re" not in str(args[0]) + + +def test_tier_s_fallback_none_address_skips_block() -> None: + calls = _capture_calls(None) + for args in calls: + assert ":house_re" not in str(args[0]) diff --git a/tradein-mvp/backend/tests/test_same_building_match.py b/tradein-mvp/backend/tests/test_same_building_match.py index 53020455..899d47ae 100644 --- a/tradein-mvp/backend/tests/test_same_building_match.py +++ b/tradein-mvp/backend/tests/test_same_building_match.py @@ -181,10 +181,14 @@ def test_falls_back_to_address_tier_s_without_house_id(): target_house_id=None, ) assert tier == "S" - # Без target_house_id канонический tier пропущен — первый запрос по address ILIKE + # Без target_house_id канонический tier пропущен — первый запрос по + # normalized street + numeric-boundary house regex (audit fix: raw ILIKE + # prefix replaced — see estimator.py _normalize_building_key/_house_boundary_regex). first_sql = _executed_sqls(db)[0] assert "house_id_fk" not in first_sql - assert "ILIKE" in first_sql + assert ":house_re" in first_sql + assert ":street_like" in first_sql + assert "ILIKE" not in first_sql def test_canonical_tier_s_too_few_falls_through():