lekss361/gendesign
1
0
Fork 0
Code Issues 57 Pull requests Projects Releases Packages Wiki Activity Actions

feat(parcels): best-layouts endpoint + service (#113 PR C) #196

Merged
lekss361 merged 3 commits from feat/113-best-layouts-endpoint into main 2026-05-16 09:10:07 +00:00
Conversation 3 Commits 3 Files changed 3 +986
lekss361 commented 2026-05-16 08:43:07 +00:00
Owner
Copy link

Summary

PR C (3 из 4) для Issue #113 Layout Analysis. Builds on PR #193 (schemas/helpers) + PR #194 (mv_layout_velocity).

Что в PR

  • POST /api/v1/parcels/{cad_num}/best-layouts — top layout patterns у конкурентов в радиусе
  • backend/app/services/site_finder/best_layouts.py (NEW, 563 LOC) — service layer
  • backend/app/api/v1/parcels.py (+24 LOC) — endpoint после /competitors
  • 10 pytest mock-based tests (404, empty, filter/exclude, threshold, scaling, etc)

Алгоритм (4 SQL queries)

  1. Centroid lookup (cad_parcels_geom → fallback cad_quarters_geom) — pattern из quarter_dump_lookup.py
  2. Competitors в radius через ST_DWithin + optional obj_class_filter
  3. Aggregate mv_layout_velocity per room_bucket (sum_deals, avg_area, avg_price)
  4. Supply batch query из domrf_kn_flats с CASE для (rb, ab) mapping — один query вместо N

Recommendation logic

  • Hamilton largest-remainder для normalize pct (invariant: sum=100)
  • target_total_flats (optional) → abs_units per bucket
  • confidence ∈ {high/medium/low} per coverage % (≥50/≥20/<20)
  • rationale_text — human-readable description

Phase 2.1 limitation

  • Без layout_type (евро/классика) и balcony_count — отсутствуют в domrf_kn_flats
  • Signature = room_bucket × area_bin = 30 классов (достаточно для unit-mix ТЗ)
  • Phase 2.2: B2B Объектив (#52) добавит layout_type — follow-up

Carryover bug flag (НЕ fixed в этом PR)

competitors.py:143 WHERE f.status='sold' всегда 0 (поле 99.8% NULL на проде). PR C обходит — использует objective_corpus_room_month (через MV). Follow-up issue для #112 рекомендуется.

Test plan

  • pytest tests/api/v1/test_parcel_best_layouts.py -v — 10/10 pass
  • pytest tests/api/v1/test_parcel_{competitors,connection_points}.py tests/test_layout_signature.py -q — 43 pass, no regression
  • ruff check + format clean
  • mypy strict clean
  • code-reviewer pre-push: ✅ APPROVE (4 minor non-blocking — coverage % denominator, snapshot subquery perf, empty filtered fallback rationale, hardcoded confidence thresholds — все cosmetic v1)
  • qa-tester post-deploy: curl POST /best-layouts на test cad с реальным radius → проверить shape + non-empty top_layouts на high-density cad'е

Known minor follow-ups (после merge)

  • Coverage % mixes pre/post-filter denominators
  • MAX(snapshot_date) subquery — pre-compute как bind param
  • confidence thresholds — config-driven

Closes part of #113 (full close after PR D PDF+UI).

## Summary PR C (3 из 4) для Issue #113 Layout Analysis. Builds on PR #193 (schemas/helpers) + PR #194 (mv_layout_velocity). ### Что в PR - **`POST /api/v1/parcels/{cad_num}/best-layouts`** — top layout patterns у конкурентов в радиусе - **`backend/app/services/site_finder/best_layouts.py`** (NEW, 563 LOC) — service layer - **`backend/app/api/v1/parcels.py`** (+24 LOC) — endpoint после `/competitors` - **10 pytest** mock-based tests (404, empty, filter/exclude, threshold, scaling, etc) ### Алгоритм (4 SQL queries) 1. Centroid lookup (`cad_parcels_geom` → fallback `cad_quarters_geom`) — pattern из quarter_dump_lookup.py 2. Competitors в radius через `ST_DWithin` + optional `obj_class_filter` 3. Aggregate `mv_layout_velocity` per `room_bucket` (sum_deals, avg_area, avg_price) 4. Supply batch query из `domrf_kn_flats` с CASE для `(rb, ab)` mapping — один query вместо N ### Recommendation logic - **Hamilton largest-remainder** для normalize pct (invariant: sum=100) - `target_total_flats` (optional) → `abs_units` per bucket - `confidence` ∈ {high/medium/low} per coverage % (≥50/≥20/<20) - `rationale_text` — human-readable description ### Phase 2.1 limitation - Без `layout_type` (евро/классика) и `balcony_count` — отсутствуют в `domrf_kn_flats` - Signature = `room_bucket × area_bin` = 30 классов (достаточно для unit-mix ТЗ) - Phase 2.2: B2B Объектив (#52) добавит `layout_type` — follow-up ### Carryover bug flag (НЕ fixed в этом PR) `competitors.py:143` `WHERE f.status='sold'` всегда 0 (поле 99.8% NULL на проде). PR C обходит — использует `objective_corpus_room_month` (через MV). Follow-up issue для #112 рекомендуется. ## Test plan - [x] `pytest tests/api/v1/test_parcel_best_layouts.py -v` — **10/10 pass** - [x] `pytest tests/api/v1/test_parcel_{competitors,connection_points}.py tests/test_layout_signature.py -q` — 43 pass, no regression - [x] `ruff check + format` clean - [x] `mypy strict` clean - [x] code-reviewer pre-push: ✅ APPROVE (4 minor non-blocking — coverage % denominator, snapshot subquery perf, empty filtered fallback rationale, hardcoded confidence thresholds — все cosmetic v1) - [ ] **qa-tester post-deploy:** `curl POST /best-layouts` на test cad с реальным radius → проверить shape + non-empty top_layouts на high-density cad'е ### Known minor follow-ups (после merge) - Coverage % mixes pre/post-filter denominators - `MAX(snapshot_date)` subquery — pre-compute как bind param - `confidence` thresholds — config-driven Closes part of #113 (full close after PR D PDF+UI).
lekss361 added 1 commit 2026-05-16 08:43:07 +00:00 
POST /api/v1/parcels/{cad_num}/best-layouts — top layout patterns
(room_bucket × area_bin) у конкурентов в радиусе с ranking by velocity
из mv_layout_velocity + unit-mix recommendation для архитектурного ТЗ.

Service (backend/app/services/site_finder/best_layouts.py, 563 LOC):
- centroid lookup с fallback (cad_parcels → cad_quarters)
- competitors-in-radius через ST_DWithin
- aggregate по mv_layout_velocity per room_bucket
- supply batch query (один SQL вместо N) для sold_pct_of_supply
- Hamilton largest-remainder normalizer (sum_pct=100 invariant)

Endpoint (backend/app/api/v1/parcels.py, +24 LOC) после /competitors.

Tests (backend/tests/api/v1/test_parcel_best_layouts.py, 10 mock tests):
404, empty competitors, MV empty, exclude/filter, min_velocity, time_window
scaling, obj_class_filter, target_total_flats, 3-bucket ranking.

Phase 2.1 (без layout_type / balcony_count — отсутствуют в БД,
ждут B2B Объектив #52). Reuses PR A schemas/helpers, PR B MV.

NOTE: competitors.py:143 status='sold' carryover bug НЕ fixed —
отдельный follow-up issue. PR C использует MV (через
objective_corpus_room_month) что работает корректно.
lekss361 commented 2026-05-16 08:48:03 +00:00
Author
Owner
Copy link

Deep Code Review — verdict

Summary

  • Status: 🔴 BLOCK
  • Files reviewed: 3 (P1: 2 — services/site_finder/best_layouts.py, api/v1/parcels.py; P3: 1 — test file)
  • Lines: +959 / -0
  • PR: #196 · head SHA fdc6456 · base 6aa22ef8 (mergeable=true, no conflicts, no prior reviews)
  • Counts: 🔴 1 · 🟠 1 · 🟡 4 · 🟢 2

🔴 Critical (BLOCK merge)

backend/app/services/site_finder/best_layouts.py:64,82,131 — column domrf_kn_objects.geom_3857 НЕ существует на проде → endpoint вернёт 500 на любой вызов

Verified via mcp__postgres-gendesign:

SELECT column_name FROM information_schema.columns
WHERE table_name='domrf_kn_objects' AND column_name ILIKE '%geom%';
-- → []  (только latitude/longitude NUMERIC, 4548/4548 заполнено)

competitors.py:106 (рабочий код в проде) корректно вычисляет геометрию on-the-fly:

ST_SetSRID(ST_MakePoint(o.longitude, o.latitude), 4326)::geography

Но best_layouts.py использует несуществующий column в трёх местах:

  • _COMPETITORS_IN_RADIUS_SQL (lines 64, 67) — WHERE geom_3857 IS NOT NULL AND ST_DWithin(geom_3857, ...)
  • _SUPPLY_BATCH_SQL (lines 105, 107) — JOIN domrf_kn_objects o ... WHERE o.geom_3857 IS NOT NULL AND ST_DWithin(o.geom_3857, ...)

Почему тесты прошли: все 10 тестов — mock-based, db.execute.side_effect = [...] возвращает MagicMock'и без выполнения SQL. DB-схема не проверяется → false-positive green.

Дополнительно: docstring класса утверждает Геометрия domrf_kn_objects.geom_3857 уже в EPSG:3857 (line 60) — это неверное утверждение про схему, не описка.

Fix (выровнять с competitors.py):

# В обоих SQL — заменить geom_3857 на on-the-fly construct:
_COMPETITORS_IN_RADIUS_SQL = text("""
    SELECT DISTINCT ON (obj_id) obj_id
    FROM domrf_kn_objects
    WHERE latitude IS NOT NULL AND longitude IS NOT NULL
      AND ST_DWithin(
          ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography,
          ST_SetSRID(
              ST_MakePoint(CAST(:center_lon AS float), CAST(:center_lat AS float)),
              4326
          )::geography,
          CAST(:radius_m AS float)
      )
      AND (CAST(:obj_class_filter AS text) IS NULL OR obj_class = CAST(:obj_class_filter AS text))
    ORDER BY obj_id, snapshot_date DESC NULLS LAST
""")

И в get_best_layouts(): вместо x_3857/y_3857 извлекать center_lon/center_lat из cad_*_geom (тем же паттерном что competitors.py использует в _PARCEL_CENTROID_SQL). После fix также нужен integration test (хотя бы один) который реально гоняет SQL — иначе схема-mismatch снова проскочит.

🟠 High (должен быть пофиксен до merge)

Integration test отсутствует, баг прошёл 10/10 unit tests

Все тесты — MagicMock на db.execute.side_effect. Это пропустило column-mismatch и пропустит любой будущий schema drift. Минимум: 1 smoke-тест через httpx.AsyncClient + test DB либо pytest-fixture с реальным connection к локальной prod-snapshot БД на одном cad_num с непустым результатом.

Reference из vault: fixes/Fix_Competitors_Schema_Mismatch_May26.md — точно такой же кейс с #112 (тоже 4 critical schema mismatch'а, пойманы reviewer'ом до merge, не тестами). Повторяющийся pattern → нужна доска "integration-test gate" для site_finder services.

🟡 Medium (желательно fix)

  1. best_layouts.py:182 — _round_to_5 dead code
    Функция определена, никем не вызывается (grep -r _round_to_5 backend/ → один файл). Либо удалить, либо использовать в _normalize_pct если задумано округление до 5%. Сейчас просто dead.

  2. best_layouts.py:411 — coverage % считает по post-exclude/filter denominator

    objects_with_data = len(obj_ids_with_data & set(all_obj_ids))
coverage_pct = objects_with_data / objects_total_in_radius

    objects_total_in_radius снимок ДО exclude/filter, obj_ids_with_data ограничен set(all_obj_ids) (после фильтрации). Если user вызвал с filter_competitor_obj_ids=[X], coverage будет искусственно занижен (1 obj с данными / 30 total = 3%, "low confidence"), хотя в filtered set может быть 100%. PR описание явно упоминает этот minor — flag для follow-up.

  3. best_layouts.py:135 — MAX(snapshot_date) subquery в WHERE без bind

    AND f.snapshot_date = (SELECT MAX(snapshot_date) FROM domrf_kn_flats)

    Subquery re-executes на каждый запрос. Лучше pre-fetch в Python (один SELECT на старте) + bind param. На domrf_kn_flats (407k rows, 96 days истории) MAX поднимет индекс idx_kn_flats_region_snap — не катастрофа, но лишние ~20-50ms на запрос.

  4. best_layouts.py:417-422 — hardcoded thresholds 50/20 для confidence
    Согласно PR описанию — config-driven follow-up. Для V1 OK, отметить как TODO с issue link.

🟢 Low / nits

  1. best_layouts.py:182-184 — _round_to_5 без unit-test — добавить или удалить
  2. best_layouts.py:25-29 — _VELOCITY_DIVISORS literal — last_year=2.0: 24mo / 12mo = 2 ОК, но если MV окно когда-нибудь поменяется (e.g. 36mo), эта const рассинхронится с 94_mv_layout_velocity.sql:51 INTERVAL '24 months'. Magic number — комментарий "matches NN_mv_layout_velocity.sql window" поможет

Cross-file impact analysis

  • Frontend: grep "best-layouts|best_layouts|BestLayouts" frontend/ → 0 matches. Нет client-side caller ещё → API contract change не сломает UI (это PR D будет добавлять).
  • Backend callers best_layouts: только новый endpoint parcels.py:2114. Чистый new code.
  • Schemas import: BestLayoutsRequest/Response, LayoutDataQuality, LayoutTzMixRow, LayoutTzRecommendation, TopLayoutRow — все определены в schemas/parcel.py:151-215 (PR A). Импорты ОК.
  • MV dependency: mv_layout_velocity определён в data/sql/94_mv_layout_velocity.sql (#194 merged). UNIQUE index (obj_id, room_bucket) + (obj_id) индекс есть — WHERE obj_id = ANY(:obj_ids) GROUP BY room_bucket будет index-only при <100 obj_id. ✅
  • competitors.py:143 carryover (status='sold'): подтверждено в проде — status 92% NULL (378578 / 407k rows). PR C правильно обходит — берёт total_deals_24mo из mv_layout_velocity (через objective_corpus_room_month, не kn_flats.status). ✅ workaround корректен, в PR описании flagged как known carryover bug.

Vault cross-check

  • ✅ code/modules/best_layouts.md — vault entry уже создан worker'ом, корректно описывает алгоритм и carryover
  • ⚠️ fixes/Fix_Competitors_Schema_Mismatch_May26.md — точно такой же класс bug'а (column не существует) уже был в #112. Сейчас повторяется в #113 PR C → паттерн: "site_finder services пишутся без проверки реальной схемы". Нужно либо integration-test gate, либо CI-проверка schema-references в SQL (grep + DESCRIBE).
  • ✅ domains/api/endpoints/Endpoint_Parcel_Competitors.md — описывает рабочий паттерн ST_MakePoint(longitude, latitude), 4326)::geography. Worker должен был свериться. После fix — обновить vault entry best_layouts.md (поменять "EPSG:3857 уже в БД" → "computed on-the-fly from lat/lng").

Performance findings

После fix column-mismatch:

  • _COMPETITORS_IN_RADIUS_SQL: ST_DWithin на geography → seq scan по domrf_kn_objects (4548 rows). Acceptable для 4k rows, но если БД вырастет — добавить GIST index на функциональный (ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)). Сейчас НЕ блокирующее.
  • _SUPPLY_BATCH_SQL: JOIN с MAX-subquery — на 407k flats × 4.5k objects → нужен EXPLAIN ANALYZE после fix. Pre-compute max_snap → bind как :max_snap снимет O(N) subquery overhead.
  • MV indexes: ✅ mv_layout_velocity_pk (obj_id, room_bucket) UNIQUE + mv_layout_velocity_obj_idx (obj_id) — оба покрывают WHERE obj_id = ANY(:obj_ids) GROUP BY room_bucket.

Positive observations

  • ✅ CAST trap соблюдён везде: CAST(:x AS float), CAST(:obj_class_filter AS text) (psycopg v3 + SQLAlchemy)
  • ✅ text(...) parametrized, никаких f-string в SQL
  • ✅ Hamilton largest-remainder корректен: floor + sort by frac → invariant sum=100 гарантирован
  • ✅ _normalize_pct обработка total<=0 edge case есть
  • ✅ Batch supply query (1 SQL вместо N) — правильная архитектура
  • ✅ try/except per-query с logger.exception — debug-friendly
  • ✅ 10 mock-tests покрывают edge cases (404, empty, exclude, filter, min_velocity, time_window scale, obj_class, target_total_flats, MV empty, 3-bucket ranking)
  • ✅ ruff line ≤100 соблюдён, нет :x::type (psycopg trap), нет print(), нет requests
  • ✅ async def handler, Depends(get_db) через Annotated — modern FastAPI pattern
  • ✅ Vault entry создан в code/modules/best_layouts.md синхронно с кодом (per CLAUDE.md rule #6)
  • ✅ Mergeable=true, single commit, не draft, ветка feat/..., нет --force/--no-verify в истории
  • ✅ Carryover bug competitors.py:143 status='sold' обоснованно flagged but not fixed — scope discipline

Recommended next steps

  1. Worker (backend-engineer): fix 🔴 — заменить geom_3857 на ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography в обоих SQL + переделать coord_row на lat/lng вместо x_3857/y_3857
  2. Удалить либо использовать _round_to_5
  3. Добавить хотя бы один integration smoke-test, который реально гоняет SQL на test/staging DB — иначе schema drift снова проскочит
  4. Обновить code/modules/best_layouts.md (vault) — убрать упоминание "EPSG:3857 в БД"
  5. После fix push → re-review (новый SHA → этот verdict устаревает)

Complexity / blast radius

  • Risk: Critical (endpoint полностью нерабочий в проде до fix)
  • Reversibility: Easy revert (single commit, no DB migration in this PR)
  • Recommended merge window: ❌ DO NOT MERGE until column-mismatch fixed

Auto-merge decision

Scope BLOCKED for auto-merge per .claude/rules/git-pr.md:

  • backend/app/api/v1/** ← touched
  • backend/app/services/** ← touched

Даже при APPROVE этот PR требует human merge. С текущим 🔴 — main session/user решит сначала пинговать worker для fixup, не мержить.

Action taken: posted verdict comment, НЕ мержил (scope blocked + 🔴 critical).

## Deep Code Review — verdict ### Summary - **Status**: 🔴 BLOCK - **Files reviewed**: 3 (P1: 2 — `services/site_finder/best_layouts.py`, `api/v1/parcels.py`; P3: 1 — test file) - **Lines**: +959 / -0 - **PR**: #196 · head SHA `fdc6456` · base `6aa22ef8` (mergeable=true, no conflicts, no prior reviews) - **Counts**: 🔴 1 · 🟠 1 · 🟡 4 · 🟢 2 --- ### 🔴 Critical (BLOCK merge) **`backend/app/services/site_finder/best_layouts.py:64,82,131` — column `domrf_kn_objects.geom_3857` НЕ существует на проде → endpoint вернёт 500 на любой вызов** Verified via `mcp__postgres-gendesign`: ``` SELECT column_name FROM information_schema.columns WHERE table_name='domrf_kn_objects' AND column_name ILIKE '%geom%'; -- → [] (только latitude/longitude NUMERIC, 4548/4548 заполнено) ``` `competitors.py:106` (рабочий код в проде) корректно вычисляет геометрию on-the-fly: ```sql ST_SetSRID(ST_MakePoint(o.longitude, o.latitude), 4326)::geography ``` Но `best_layouts.py` использует несуществующий column в трёх местах: - `_COMPETITORS_IN_RADIUS_SQL` (lines 64, 67) — `WHERE geom_3857 IS NOT NULL AND ST_DWithin(geom_3857, ...)` - `_SUPPLY_BATCH_SQL` (lines 105, 107) — `JOIN domrf_kn_objects o ... WHERE o.geom_3857 IS NOT NULL AND ST_DWithin(o.geom_3857, ...)` **Почему тесты прошли**: все 10 тестов — mock-based, `db.execute.side_effect = [...]` возвращает MagicMock'и без выполнения SQL. DB-схема не проверяется → false-positive green. **Дополнительно**: docstring класса утверждает `Геометрия domrf_kn_objects.geom_3857 уже в EPSG:3857` (line 60) — это неверное утверждение про схему, не описка. **Fix** (выровнять с `competitors.py`): ```python # В обоих SQL — заменить geom_3857 на on-the-fly construct: _COMPETITORS_IN_RADIUS_SQL = text(""" SELECT DISTINCT ON (obj_id) obj_id FROM domrf_kn_objects WHERE latitude IS NOT NULL AND longitude IS NOT NULL AND ST_DWithin( ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography, ST_SetSRID( ST_MakePoint(CAST(:center_lon AS float), CAST(:center_lat AS float)), 4326 )::geography, CAST(:radius_m AS float) ) AND (CAST(:obj_class_filter AS text) IS NULL OR obj_class = CAST(:obj_class_filter AS text)) ORDER BY obj_id, snapshot_date DESC NULLS LAST """) ``` И в `get_best_layouts()`: вместо `x_3857/y_3857` извлекать `center_lon/center_lat` из `cad_*_geom` (тем же паттерном что `competitors.py` использует в `_PARCEL_CENTROID_SQL`). После fix также нужен integration test (хотя бы один) который реально гоняет SQL — иначе схема-mismatch снова проскочит. --- ### 🟠 High (должен быть пофиксен до merge) **Integration test отсутствует, баг прошёл 10/10 unit tests** Все тесты — `MagicMock` на `db.execute.side_effect`. Это пропустило column-mismatch и пропустит любой будущий schema drift. Минимум: 1 smoke-тест через `httpx.AsyncClient + test DB` либо pytest-fixture с реальным connection к локальной prod-snapshot БД на одном cad_num с непустым результатом. Reference из vault: `fixes/Fix_Competitors_Schema_Mismatch_May26.md` — точно такой же кейс с #112 (тоже 4 critical schema mismatch'а, пойманы reviewer'ом до merge, не тестами). Повторяющийся pattern → нужна доска "integration-test gate" для site_finder services. --- ### 🟡 Medium (желательно fix) 1. **`best_layouts.py:182` — `_round_to_5` dead code** Функция определена, никем не вызывается (`grep -r _round_to_5 backend/` → один файл). Либо удалить, либо использовать в `_normalize_pct` если задумано округление до 5%. Сейчас просто dead. 2. **`best_layouts.py:411` — coverage % считает по post-exclude/filter denominator** ```python objects_with_data = len(obj_ids_with_data & set(all_obj_ids)) coverage_pct = objects_with_data / objects_total_in_radius ``` `objects_total_in_radius` снимок ДО `exclude/filter`, `obj_ids_with_data` ограничен `set(all_obj_ids)` (после фильтрации). Если user вызвал с `filter_competitor_obj_ids=[X]`, coverage будет искусственно занижен (1 obj с данными / 30 total = 3%, "low confidence"), хотя в filtered set может быть 100%. PR описание явно упоминает этот minor — flag для follow-up. 3. **`best_layouts.py:135` — `MAX(snapshot_date)` subquery в WHERE без bind** ```sql AND f.snapshot_date = (SELECT MAX(snapshot_date) FROM domrf_kn_flats) ``` Subquery re-executes на каждый запрос. Лучше pre-fetch в Python (один SELECT на старте) + bind param. На `domrf_kn_flats` (407k rows, 96 days истории) MAX поднимет индекс `idx_kn_flats_region_snap` — не катастрофа, но лишние ~20-50ms на запрос. 4. **`best_layouts.py:417-422` — hardcoded thresholds 50/20 для confidence** Согласно PR описанию — config-driven follow-up. Для V1 OK, отметить как TODO с issue link. --- ### 🟢 Low / nits 1. **`best_layouts.py:182-184` — `_round_to_5` без unit-test** — добавить или удалить 2. **`best_layouts.py:25-29` — `_VELOCITY_DIVISORS` literal — `last_year=2.0`**: 24mo / 12mo = 2 ОК, но если MV окно когда-нибудь поменяется (e.g. 36mo), эта const рассинхронится с `94_mv_layout_velocity.sql:51 INTERVAL '24 months'`. Magic number — комментарий "matches NN_mv_layout_velocity.sql window" поможет --- ### Cross-file impact analysis - **Frontend**: `grep "best-layouts|best_layouts|BestLayouts" frontend/` → 0 matches. Нет client-side caller ещё → API contract change не сломает UI (это PR D будет добавлять). - **Backend callers `best_layouts`**: только новый endpoint `parcels.py:2114`. Чистый new code. - **Schemas import**: `BestLayoutsRequest/Response`, `LayoutDataQuality`, `LayoutTzMixRow`, `LayoutTzRecommendation`, `TopLayoutRow` — все определены в `schemas/parcel.py:151-215` (PR A). Импорты ОК. - **MV dependency**: `mv_layout_velocity` определён в `data/sql/94_mv_layout_velocity.sql` (#194 merged). UNIQUE index `(obj_id, room_bucket)` + `(obj_id)` индекс есть — `WHERE obj_id = ANY(:obj_ids) GROUP BY room_bucket` будет index-only при <100 obj_id. ✅ - **`competitors.py:143` carryover (status='sold')**: подтверждено в проде — `status` 92% NULL (`378578 / 407k rows`). PR C правильно обходит — берёт `total_deals_24mo` из `mv_layout_velocity` (через `objective_corpus_room_month`, не `kn_flats.status`). ✅ workaround корректен, в PR описании flagged как known carryover bug. --- ### Vault cross-check - ✅ `code/modules/best_layouts.md` — vault entry уже создан worker'ом, корректно описывает алгоритм и carryover - ⚠️ `fixes/Fix_Competitors_Schema_Mismatch_May26.md` — точно такой же класс bug'а (column не существует) уже был в #112. Сейчас повторяется в #113 PR C → паттерн: "site_finder services пишутся без проверки реальной схемы". Нужно либо integration-test gate, либо CI-проверка schema-references в SQL (grep + DESCRIBE). - ✅ `domains/api/endpoints/Endpoint_Parcel_Competitors.md` — описывает рабочий паттерн `ST_MakePoint(longitude, latitude), 4326)::geography`. Worker должен был свериться. **После fix — обновить vault entry `best_layouts.md`** (поменять "EPSG:3857 уже в БД" → "computed on-the-fly from lat/lng"). --- ### Performance findings После fix column-mismatch: - `_COMPETITORS_IN_RADIUS_SQL`: `ST_DWithin` на `geography` → seq scan по `domrf_kn_objects` (4548 rows). Acceptable для 4k rows, но если БД вырастет — добавить GIST index на функциональный `(ST_SetSRID(ST_MakePoint(longitude, latitude), 4326))`. Сейчас НЕ блокирующее. - `_SUPPLY_BATCH_SQL`: JOIN с MAX-subquery — на 407k flats × 4.5k objects → нужен `EXPLAIN ANALYZE` после fix. Pre-compute `max_snap` → bind как `:max_snap` снимет O(N) subquery overhead. - MV indexes: ✅ `mv_layout_velocity_pk (obj_id, room_bucket) UNIQUE` + `mv_layout_velocity_obj_idx (obj_id)` — оба покрывают `WHERE obj_id = ANY(:obj_ids) GROUP BY room_bucket`. --- ### Positive observations - ✅ CAST trap соблюдён везде: `CAST(:x AS float)`, `CAST(:obj_class_filter AS text)` (psycopg v3 + SQLAlchemy) - ✅ `text(...)` parametrized, никаких f-string в SQL - ✅ Hamilton largest-remainder корректен: floor + sort by frac → invariant `sum=100` гарантирован - ✅ `_normalize_pct` обработка `total<=0` edge case есть - ✅ Batch supply query (1 SQL вместо N) — правильная архитектура - ✅ `try/except` per-query с `logger.exception` — debug-friendly - ✅ 10 mock-tests покрывают edge cases (404, empty, exclude, filter, min_velocity, time_window scale, obj_class, target_total_flats, MV empty, 3-bucket ranking) - ✅ ruff line ≤100 соблюдён, нет `:x::type` (psycopg trap), нет `print()`, нет `requests` - ✅ `async def` handler, `Depends(get_db)` через Annotated — modern FastAPI pattern - ✅ Vault entry создан в `code/modules/best_layouts.md` синхронно с кодом (per CLAUDE.md rule #6) - ✅ Mergeable=true, single commit, не draft, ветка `feat/...`, нет --force/--no-verify в истории - ✅ Carryover bug `competitors.py:143 status='sold'` обоснованно flagged but not fixed — scope discipline --- ### Recommended next steps 1. **Worker (backend-engineer)**: fix 🔴 — заменить `geom_3857` на `ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography` в обоих SQL + переделать coord_row на `lat/lng` вместо `x_3857/y_3857` 2. Удалить либо использовать `_round_to_5` 3. Добавить хотя бы один integration smoke-test, который реально гоняет SQL на test/staging DB — иначе schema drift снова проскочит 4. Обновить `code/modules/best_layouts.md` (vault) — убрать упоминание "EPSG:3857 в БД" 5. После fix push → re-review (новый SHA → этот verdict устаревает) --- ### Complexity / blast radius - **Risk**: **Critical** (endpoint полностью нерабочий в проде до fix) - **Reversibility**: Easy revert (single commit, no DB migration in this PR) - **Recommended merge window**: ❌ DO NOT MERGE until column-mismatch fixed --- ### Auto-merge decision Scope BLOCKED for auto-merge per `.claude/rules/git-pr.md`: - `backend/app/api/v1/**` ← touched - `backend/app/services/**` ← touched Даже при APPROVE этот PR требует human merge. С текущим 🔴 — main session/user решит сначала пинговать worker для fixup, **не мержить**. **Action taken**: posted verdict comment, НЕ мержил (scope blocked + 🔴 critical).
lekss361 added 1 commit 2026-05-16 08:52:13 +00:00 
domrf_kn_objects has no geom_3857 column (only latitude/longitude NUMERIC).
Switch _COMPETITORS_IN_RADIUS_SQL and _SUPPLY_BATCH_SQL to use
ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography — consistent
with competitors.py pattern. Rename centroid params x_3857/y_3857 →
center_lon/center_lat. Remove dead _round_to_5 function.

Fixes review-bot 🔴 BLOCK on fdc6456 PR #196.

Tests: 10/10 pass (mock keys updated к center_lon/center_lat), 43/43 regression OK.
lekss361 commented 2026-05-16 08:55:29 +00:00
Author
Owner
Copy link

Deep Code Review — re-review verdict (SHA a88917f)

Summary

  • Status: ⚠️ MINOR (was 🔴 BLOCK on fdc6456)
  • Files reviewed: 3 (P1: 2, P3: 1)
  • Lines: +961 / -0 (fixup: +26 / -24)
  • Previous SHA: fdc6456 → New SHA: a88917f
  • PR: #196 — state=open, mergeable=true

🔴 Critical

Все resolved.

🟠 High

Все resolved. (Integration test gap downgraded to 🟡 — schema bug fixed, runtime risk сильно снижен.)

🟡 Medium (carryover из предыдущего ревью — не блокирующее, можно follow-up issue)

  • best_layouts.py:382-388 — coverage % всё ещё mix pre/post-filter denominators:

    objects_with_data = len(obj_ids_with_data & set(all_obj_ids))  # post-filter
coverage_pct = round(objects_with_data / objects_total_in_radius * 100.0, 1)  # pre-filter

    При aggressive exclude_competitor_obj_ids coverage будет artificially low → confidence ниже чем должно. Fix: либо objects_total_in_radius = len(all_obj_ids) после фильтров, либо считать coverage % по pre-filter set целиком.

  • best_layouts.py:112 — MAX(snapshot_date) subquery в _SUPPLY_BATCH_SQL re-executes per scan:

    AND f.snapshot_date = (SELECT MAX(snapshot_date) FROM domrf_kn_flats)

    Pre-compute как bind param: latest_snap = db.scalar(text("SELECT MAX(snapshot_date) FROM domrf_kn_flats")) → pass via :latest_snap. Один extra round-trip vs повторение в spatial scan.

  • best_layouts.py:389-393 — hardcoded confidence thresholds (50.0/20.0). Move в settings.py (LAYOUT_CONFIDENCE_HIGH_PCT / LAYOUT_CONFIDENCE_MEDIUM_PCT) или в module-level constants для easier tuning.

  • best_layouts.py — no real-DB integration test. Mock tests passed на сломанной geom_3857 версии (paтterned after fixes/Fix_Competitors_Schema_Mismatch_May26.md). Severity downgraded (схема fixed, та же on-the-fly pattern что и working competitors.py:106), но добавить smoke test с реальной БД для регрессионного покрытия желательно — можно в PR D / отдельном test-only PR.

🟡 New observation

  • best_layouts.py:103-117 (_SUPPLY_BATCH_SQL) — ST_DWithin на on-the-fly geography без spatial index (поскольку нет geom_3857 column → нет GIST). Для prod radii 1-3 km в EKB likely OK (filter by spatial bbox через ST_DWithin cheap geography). Тот же trade-off что в competitors.py accepts — noted, not blocking.

🟢 Resolved since fdc6456

  • ✅ best_layouts.py:64,82,131 — geom_3857 references gone — заменены на ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography consistently в обоих SQL (_COMPETITORS_IN_RADIUS_SQL + _SUPPLY_BATCH_SQL). Schema verified через postgres MCP: domrf_kn_objects имеет latitude/longitude NUMERIC (нет geom_3857), domrf_kn_flats ditto.
  • ✅ Centroid params renamed x_3857/y_3857 → center_lon/center_lat (consistency с reference impl competitors.py).
  • ✅ _round_to_5 dead code removed.
  • ✅ Mock tests updated к новым param keys (per commit message — 10/10 pass).

Cross-file impact

  • best_layouts.py — следует established pattern из competitors.py:106 (geography on-the-fly). Other callers не затронуты.
  • parcels.py — endpoint только добавлен, существующие handlers не задеты.
  • mv_layout_velocity (PR B #194) — usage в _VELOCITY_BY_ROOM_SQL корректное, columns matches schema PR.

Vault cross-check

  • ✅ Не повторяет fixes/Fix_Competitors_Schema_Mismatch_May26.md pattern (та же проблема — column non-existent — здесь предотвращена fixup'ом).
  • Worth добавить entry fixes/Fix_BestLayouts_geom_3857_Phantom_May26.md documenting:
    • Bug: code references geom_3857 column на domrf_kn_objects, которого никогда не существовало
    • Root cause: assumed pattern из других таблиц без verify через \d domrf_kn_objects
    • Detection: deep-code-reviewer + postgres MCP schema check
    • Prevention: всегда verify column existence перед use в SQL, особенно для geography/PostGIS columns

Performance findings

  • _COMPETITORS_IN_RADIUS_SQL + _SUPPLY_BATCH_SQL: on-the-fly geography без spatial index. Acceptable для EKB radii. Worth EXPLAIN на проде через qa-tester post-deploy.

Positive observations

  • ✅ Focused fixup commit (4 минимальных правок без scope creep)
  • ✅ Test mocks updated в том же commit (no broken regression)
  • ✅ Clean fix follows reference impl pattern точно
  • ✅ Centroid lookup с fallback (parcels → quarters) — good defensive pattern
  • ✅ Hamilton largest-remainder для pct sum=100 — корректный алгоритм
  • ✅ Batch supply query (один SQL вместо N) — N+1 avoidance

Auto-merge decision

Scope BLOCKED для auto-merge — PR touches:

  • backend/app/api/v1/** (parcels.py)
  • backend/app/services/** (best_layouts.py)

Per Auto-merge policy в .claude/agents/deep-code-reviewer.md секция "Auto-merge BLOCKED" — даже при verdict без 🔴/🟠 reviewer не мержит сам. Main session / user принимают merge decision.

Recommended next steps

  1. Merge OK с точки зрения correctness — все critical/high fixed. 🟡 items — non-blocking, отдельный follow-up issue для cleanup (coverage denom + snapshot subquery + config thresholds + real-DB smoke).
  2. После merge: обязательный qa-tester post-deploy smoke — curl POST /best-layouts на high-density cad'е (e.g. 66:41:0303161:...) → verify shape + non-empty top_layouts + EXPLAIN на проде для confirm spatial scan не Seq Scan'ит всю domrf_kn_flats (~2M rows).
  3. Создать vault entry fixes/Fix_BestLayouts_geom_3857_Phantom_May26.md per CLAUDE.md rule #6.

Complexity / blast radius

  • Risk: Low (read-only endpoint, no DML, defensive try/except, fallback на empty response)
  • Reversibility: Easy revert (single endpoint, no DB migration)
  • Recommended merge window: anytime

Re-review by deep-code-reviewer subagent. Posted as lekss361. Scope blocked → main session / user merge decision.

## Deep Code Review — re-review verdict (SHA `a88917f`) ### Summary - **Status**: ⚠️ MINOR (was 🔴 BLOCK on `fdc6456`) - **Files reviewed**: 3 (P1: 2, P3: 1) - **Lines**: +961 / -0 (fixup: +26 / -24) - **Previous SHA**: `fdc6456` → **New SHA**: `a88917f` - **PR**: #196 — state=open, mergeable=true ### 🔴 Critical *Все resolved.* ### 🟠 High *Все resolved.* (Integration test gap downgraded to 🟡 — schema bug fixed, runtime risk сильно снижен.) ### 🟡 Medium (carryover из предыдущего ревью — не блокирующее, можно follow-up issue) - [ ] `best_layouts.py:382-388` — coverage % всё ещё mix pre/post-filter denominators: ```python objects_with_data = len(obj_ids_with_data & set(all_obj_ids)) # post-filter coverage_pct = round(objects_with_data / objects_total_in_radius * 100.0, 1) # pre-filter ``` При aggressive `exclude_competitor_obj_ids` coverage будет artificially low → confidence ниже чем должно. Fix: либо `objects_total_in_radius = len(all_obj_ids)` после фильтров, либо считать coverage % по pre-filter set целиком. - [ ] `best_layouts.py:112` — `MAX(snapshot_date)` subquery в `_SUPPLY_BATCH_SQL` re-executes per scan: ```sql AND f.snapshot_date = (SELECT MAX(snapshot_date) FROM domrf_kn_flats) ``` Pre-compute как bind param: `latest_snap = db.scalar(text("SELECT MAX(snapshot_date) FROM domrf_kn_flats"))` → pass via `:latest_snap`. Один extra round-trip vs повторение в spatial scan. - [ ] `best_layouts.py:389-393` — hardcoded confidence thresholds (`50.0`/`20.0`). Move в `settings.py` (`LAYOUT_CONFIDENCE_HIGH_PCT` / `LAYOUT_CONFIDENCE_MEDIUM_PCT`) или в module-level constants для easier tuning. - [ ] `best_layouts.py` — no real-DB integration test. Mock tests passed на сломанной `geom_3857` версии (paтterned after `fixes/Fix_Competitors_Schema_Mismatch_May26.md`). Severity downgraded (схема fixed, та же on-the-fly pattern что и working `competitors.py:106`), но добавить smoke test с реальной БД для регрессионного покрытия желательно — можно в PR D / отдельном test-only PR. ### 🟡 New observation - [ ] `best_layouts.py:103-117` (`_SUPPLY_BATCH_SQL`) — `ST_DWithin` на on-the-fly geography без spatial index (поскольку нет `geom_3857` column → нет GIST). Для prod radii 1-3 km в EKB likely OK (filter by spatial bbox через `ST_DWithin` cheap geography). Тот же trade-off что в `competitors.py` accepts — noted, not blocking. ### 🟢 Resolved since `fdc6456` - ✅ `best_layouts.py:64,82,131` — `geom_3857` references **gone** — заменены на `ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)::geography` consistently в обоих SQL (`_COMPETITORS_IN_RADIUS_SQL` + `_SUPPLY_BATCH_SQL`). Schema verified через postgres MCP: `domrf_kn_objects` имеет `latitude/longitude NUMERIC` (нет `geom_3857`), `domrf_kn_flats` ditto. - ✅ Centroid params renamed `x_3857/y_3857` → `center_lon/center_lat` (consistency с reference impl `competitors.py`). - ✅ `_round_to_5` dead code removed. - ✅ Mock tests updated к новым param keys (per commit message — 10/10 pass). ### Cross-file impact - `best_layouts.py` — следует established pattern из `competitors.py:106` (geography on-the-fly). Other callers не затронуты. - `parcels.py` — endpoint только добавлен, существующие handlers не задеты. - `mv_layout_velocity` (PR B #194) — usage в `_VELOCITY_BY_ROOM_SQL` корректное, columns matches schema PR. ### Vault cross-check - ✅ Не повторяет `fixes/Fix_Competitors_Schema_Mismatch_May26.md` pattern (та же проблема — column non-existent — здесь предотвращена fixup'ом). - Worth добавить entry `fixes/Fix_BestLayouts_geom_3857_Phantom_May26.md` documenting: - Bug: code references `geom_3857` column на `domrf_kn_objects`, которого никогда не существовало - Root cause: assumed pattern из других таблиц без verify через `\d domrf_kn_objects` - Detection: deep-code-reviewer + postgres MCP schema check - Prevention: всегда verify column existence перед use в SQL, особенно для geography/PostGIS columns ### Performance findings - `_COMPETITORS_IN_RADIUS_SQL` + `_SUPPLY_BATCH_SQL`: on-the-fly geography без spatial index. Acceptable для EKB radii. Worth EXPLAIN на проде через qa-tester post-deploy. ### Positive observations - ✅ Focused fixup commit (4 минимальных правок без scope creep) - ✅ Test mocks updated в том же commit (no broken regression) - ✅ Clean fix follows reference impl pattern точно - ✅ Centroid lookup с fallback (parcels → quarters) — good defensive pattern - ✅ Hamilton largest-remainder для pct sum=100 — корректный алгоритм - ✅ Batch supply query (один SQL вместо N) — N+1 avoidance ### Auto-merge decision **Scope BLOCKED** для auto-merge — PR touches: - `backend/app/api/v1/**` (parcels.py) - `backend/app/services/**` (best_layouts.py) Per Auto-merge policy в `.claude/agents/deep-code-reviewer.md` секция "Auto-merge BLOCKED" — даже при verdict без 🔴/🟠 reviewer не мержит сам. **Main session / user принимают merge decision.** ### Recommended next steps 1. **Merge OK** с точки зрения correctness — все critical/high fixed. 🟡 items — non-blocking, отдельный follow-up issue для cleanup (coverage denom + snapshot subquery + config thresholds + real-DB smoke). 2. После merge: обязательный `qa-tester` post-deploy smoke — `curl POST /best-layouts` на high-density cad'е (e.g. `66:41:0303161:...`) → verify shape + non-empty top_layouts + EXPLAIN на проде для confirm spatial scan не Seq Scan'ит всю `domrf_kn_flats` (~2M rows). 3. Создать vault entry `fixes/Fix_BestLayouts_geom_3857_Phantom_May26.md` per CLAUDE.md rule #6. ### Complexity / blast radius - **Risk**: Low (read-only endpoint, no DML, defensive try/except, fallback на empty response) - **Reversibility**: Easy revert (single endpoint, no DB migration) - **Recommended merge window**: anytime --- *Re-review by `deep-code-reviewer` subagent. Posted as `lekss361`. Scope blocked → main session / user merge decision.*
lekss361 added 1 commit 2026-05-16 09:01:14 +00:00 
3 carryover 🟡 items из re-review verdict на a88917f:

1. Coverage % denominator now post-filter (was mixing pre-filter count
   с post-filter intersection → artificially low confidence when
   user passed exclude/filter list)
2. MAX(snapshot_date) pre-computed via db.scalar() + bind :latest_snap
   instead of subquery in WHERE (avoid re-execution per spatial scan)
3. LAYOUT_CONFIDENCE_HIGH_PCT / LAYOUT_CONFIDENCE_MEDIUM_PCT as
   module-level constants instead of hardcoded literals

Integration test gap (#4 в re-review) — отдельный follow-up issue,
не в scope этого PR.

Tests: 10/10 best-layouts pass, 43/43 regression OK.
lekss361 commented 2026-05-16 09:09:17 +00:00
Author
Owner
Copy link

Deep Code Review — verdict (iteration 3, SHA 8488012f)

Summary

  • Status: ✅ APPROVE (scope blocked — needs human merge)
  • Files reviewed: 3 (P1: 2 — backend/app/api/v1/parcels.py, backend/app/services/site_finder/best_layouts.py; P3: 1 — backend/tests/api/v1/test_parcel_best_layouts.py)
  • Lines: +986 / −0 (delta vs a88917f: +46 / −21 across 2 files)
  • Previous verdicts: fdc6456 🔴 BLOCK · a88917f ⚠️ MINOR · 8488012f ✅ APPROVE
  • Counts: 🔴 0 / 🟠 0 / 🟡 1 / 🟢 2

Carryover items — status

# Item Previous Now Where
1 _round_to_5 dead code 🟡 ✅ removed (fixed in a88917f)
2 Coverage % pre/post-filter denominator inconsistency 🟡 ✅ FIXED best_layouts.py:389-395 — objects_total_after_filter = len(all_obj_ids), objects_with_data = len(obj_ids_with_data & set(all_obj_ids)) (intersection с post-filter)
3 MAX(snapshot_date) re-executed subquery 🟡 ✅ FIXED best_layouts.py:315 — pre-computed via db.scalar(...), bound as :latest_snap (CAST AS date) in _SUPPLY_BATCH_SQL
4 Hardcoded confidence thresholds 🟡 ✅ FIXED best_layouts.py:64-66 — LAYOUT_CONFIDENCE_HIGH_PCT = 50.0, LAYOUT_CONFIDENCE_MEDIUM_PCT = 20.0 module constants (not full settings.* config-driven, но acceptable v1 + есть комментарий "Tune via PR")
5 Integration test (живая БД) 🟠 (fixup-1) 🟡 (deferred) Worker explicitly defers to follow-up issue per commit message — 10/10 mock tests cover algorithmic invariants (rank sort, pct sum=100, time_window scaling, exclude/filter, target_total_flats). Acceptable v1 ship strategy.
6 _SUPPLY_BATCH_SQL on-the-fly geography (no GIST) 🟡 (fixup-1) 🟡 (carry) Unchanged. Consistent с competitors.py pattern. Schema fix geom_3857 → ST_MakePoint(longitude, latitude)::geography preserved from a88917f ✅. Optimization (generated column + GIST) — separate infra PR.

Schema verification (live DB via MCP)

  • ✅ domrf_kn_objects: latitude / longitude NUMERIC (no geom_3857) — matches PR usage
  • ✅ domrf_kn_flats: total_area NUMERIC, rooms INT, is_studio BOOL NULL, flat_type TEXT, snapshot_date DATE NOT NULL, obj_id BIGINT — все referenced колонки на месте; f.is_studio = TRUE корректно обрабатывает NULL (falls through на flat_type check)
  • ✅ mv_layout_velocity: PK (obj_id, room_bucket) → WHERE obj_id = ANY(:obj_ids) использует index; GROUP BY room_bucket fine
  • ✅ Schemas (backend/app/schemas/parcel.py): BestLayoutsRequest.time_window: Literal["last_month", "last_quarter", "last_year"] → _VELOCITY_DIVISORS[request.time_window] не может бросить KeyError на провалидированном input

New observations (+46 lines)

  • 🟢 db.scalar(text("SELECT MAX(snapshot_date) FROM domrf_kn_flats")) — full-table aggregate; на партиционированной таблице планировщик берёт MAX-per-partition (cheap), один-shot. Acceptable v1.
  • 🟢 LayoutDataQuality.objects_total_in_radius теперь семантически означает "post-filter count" (после exclude/filter), не "raw radius count". Поле новое — нет backward-compat issue, но рекомендуется уточнить в docstring schema. Cosmetic.
  • 🟢 Mock infra _make_db.db.scalar.return_value = latest_snap if latest_snap is not None else _TODAY — корректно покрывает новый db.scalar call.

Remaining 🟡 (non-blocking)

  • Integration test против live PostGIS — pytest -m integration с фикстурой geom + supply data. Worker открыл follow-up issue. Не блокирует merge — 10/10 mock tests + ручной qa-tester smoke post-deploy достаточно для ship.

Positive observations

  • ✅ Все 3 in-scope 🟡 carryover items fixed (#2 #3 #4)
  • ✅ Schema fix (geom_3857 → on-the-fly geography) preserved без regression
  • ✅ Bind params везде (CAST(:x AS type) per .claude/rules/backend.md), no SQL injection vector
  • ✅ psycopg v3 (через SQLAlchemy text + session.execute) — никакого psycopg2
  • ✅ logger.* не print(), httpx не requests
  • ✅ _VELOCITY_DIVISORS lookup type-safe (Literal-constrained input)
  • ✅ Hamilton largest-remainder invariant: sum=100 (tested via test_three_obj_ids_ranking_and_pct_sum_100)
  • ✅ Centroid fallback (cad_parcels_geom → cad_quarters_geom) per established quarter_dump_lookup.py pattern

Cross-file impact analysis

  • backend/app/schemas/parcel.py — schemas BestLayoutsRequest/Response, TopLayoutRow, LayoutTzMixRow/Recommendation, LayoutDataQuality импортируются только из этого PR (verified — no other consumers in this repo state)
  • backend/app/services/site_finder/layout_signature.py (из PR A #193) — area_bin(), layout_signature() reused correctly
  • mv_layout_velocity (PR B #194) — query pattern obj_id = ANY(:obj_ids) использует PK index, no perf concern
  • Frontend нет (TS types regen не нужен в рамках этого PR — UI в PR D)

Risk / merge

  • Risk: Low (новый endpoint, no breaking change на существующих route'ах)
  • Reversibility: Easy revert (один service file + один endpoint block, не трогает migrations / data)
  • Recommended merge window: anytime
  • CI: 10/10 best-layouts tests pass + 43/43 regression OK (per PR body)

Final action

  • Scope: backend/app/api/v1/** + backend/app/services/** → ❌ Auto-merge BLOCKED per .claude/rules/git-pr.md policy
  • Verdict: ✅ APPROVE — eligible for human merge (squash + delete branch)
  • Post-merge: spawn qa-tester для curl POST /api/v1/parcels/<test_cad>/best-layouts на high-density cad (ЕКБ центр) + проверка response shape + non-empty top_layouts
## Deep Code Review — verdict (iteration 3, SHA `8488012f`) ### Summary - **Status**: ✅ **APPROVE** (scope blocked — needs human merge) - **Files reviewed**: 3 (P1: 2 — `backend/app/api/v1/parcels.py`, `backend/app/services/site_finder/best_layouts.py`; P3: 1 — `backend/tests/api/v1/test_parcel_best_layouts.py`) - **Lines**: +986 / −0 (delta vs `a88917f`: +46 / −21 across 2 files) - **Previous verdicts**: `fdc6456` 🔴 BLOCK · `a88917f` ⚠️ MINOR · `8488012f` ✅ APPROVE - **Counts**: 🔴 0 / 🟠 0 / 🟡 1 / 🟢 2 ### Carryover items — status | # | Item | Previous | Now | Where | |---|---|---|---|---| | 1 | `_round_to_5` dead code | 🟡 | ✅ removed | (fixed in `a88917f`) | | 2 | Coverage % pre/post-filter denominator inconsistency | 🟡 | ✅ **FIXED** | `best_layouts.py:389-395` — `objects_total_after_filter = len(all_obj_ids)`, `objects_with_data = len(obj_ids_with_data & set(all_obj_ids))` (intersection с post-filter) | | 3 | `MAX(snapshot_date)` re-executed subquery | 🟡 | ✅ **FIXED** | `best_layouts.py:315` — pre-computed via `db.scalar(...)`, bound as `:latest_snap` (CAST AS date) in `_SUPPLY_BATCH_SQL` | | 4 | Hardcoded confidence thresholds | 🟡 | ✅ **FIXED** | `best_layouts.py:64-66` — `LAYOUT_CONFIDENCE_HIGH_PCT = 50.0`, `LAYOUT_CONFIDENCE_MEDIUM_PCT = 20.0` module constants (not full `settings.*` config-driven, но acceptable v1 + есть комментарий "Tune via PR") | | 5 | Integration test (живая БД) | 🟠 (fixup-1) | 🟡 (deferred) | Worker explicitly defers to follow-up issue per commit message — 10/10 mock tests cover algorithmic invariants (rank sort, pct sum=100, time_window scaling, exclude/filter, target_total_flats). Acceptable v1 ship strategy. | | 6 | `_SUPPLY_BATCH_SQL` on-the-fly geography (no GIST) | 🟡 (fixup-1) | 🟡 (carry) | Unchanged. Consistent с `competitors.py` pattern. Schema fix `geom_3857` → `ST_MakePoint(longitude, latitude)::geography` preserved from `a88917f` ✅. Optimization (generated column + GIST) — separate infra PR. | ### Schema verification (live DB via MCP) - ✅ `domrf_kn_objects`: `latitude` / `longitude` NUMERIC (no `geom_3857`) — matches PR usage - ✅ `domrf_kn_flats`: `total_area NUMERIC`, `rooms INT`, `is_studio BOOL NULL`, `flat_type TEXT`, `snapshot_date DATE NOT NULL`, `obj_id BIGINT` — все referenced колонки на месте; `f.is_studio = TRUE` корректно обрабатывает NULL (falls through на `flat_type` check) - ✅ `mv_layout_velocity`: PK `(obj_id, room_bucket)` → `WHERE obj_id = ANY(:obj_ids)` использует index; `GROUP BY room_bucket` fine - ✅ Schemas (`backend/app/schemas/parcel.py`): `BestLayoutsRequest.time_window: Literal["last_month", "last_quarter", "last_year"]` → `_VELOCITY_DIVISORS[request.time_window]` не может бросить KeyError на провалидированном input ### New observations (+46 lines) - 🟢 `db.scalar(text("SELECT MAX(snapshot_date) FROM domrf_kn_flats"))` — full-table aggregate; на партиционированной таблице планировщик берёт MAX-per-partition (cheap), один-shot. Acceptable v1. - 🟢 `LayoutDataQuality.objects_total_in_radius` теперь семантически означает "post-filter count" (после exclude/filter), не "raw radius count". Поле новое — нет backward-compat issue, но рекомендуется уточнить в docstring schema. Cosmetic. - 🟢 Mock infra `_make_db.db.scalar.return_value = latest_snap if latest_snap is not None else _TODAY` — корректно покрывает новый `db.scalar` call. ### Remaining 🟡 (non-blocking) - [ ] **Integration test против live PostGIS** — `pytest -m integration` с фикстурой geom + supply data. Worker открыл follow-up issue. Не блокирует merge — 10/10 mock tests + ручной qa-tester smoke post-deploy достаточно для ship. ### Positive observations - ✅ Все 3 in-scope 🟡 carryover items fixed (#2 #3 #4) - ✅ Schema fix (`geom_3857` → on-the-fly geography) preserved без regression - ✅ Bind params везде (`CAST(:x AS type)` per `.claude/rules/backend.md`), no SQL injection vector - ✅ `psycopg v3` (через SQLAlchemy text + session.execute) — никакого `psycopg2` - ✅ `logger.*` не `print()`, `httpx` не `requests` - ✅ `_VELOCITY_DIVISORS` lookup type-safe (Literal-constrained input) - ✅ Hamilton largest-remainder invariant: sum=100 (tested via `test_three_obj_ids_ranking_and_pct_sum_100`) - ✅ Centroid fallback (cad_parcels_geom → cad_quarters_geom) per established `quarter_dump_lookup.py` pattern ### Cross-file impact analysis - `backend/app/schemas/parcel.py` — schemas `BestLayoutsRequest/Response`, `TopLayoutRow`, `LayoutTzMixRow/Recommendation`, `LayoutDataQuality` импортируются только из этого PR (verified — no other consumers in this repo state) - `backend/app/services/site_finder/layout_signature.py` (из PR A #193) — `area_bin()`, `layout_signature()` reused correctly - `mv_layout_velocity` (PR B #194) — query pattern `obj_id = ANY(:obj_ids)` использует PK index, no perf concern - Frontend нет (TS types regen не нужен в рамках этого PR — UI в PR D) ### Risk / merge - **Risk**: Low (новый endpoint, no breaking change на существующих route'ах) - **Reversibility**: Easy revert (один service file + один endpoint block, не трогает migrations / data) - **Recommended merge window**: anytime - **CI**: 10/10 best-layouts tests pass + 43/43 regression OK (per PR body) ### Final action - **Scope**: `backend/app/api/v1/**` + `backend/app/services/**` → ❌ Auto-merge BLOCKED per `.claude/rules/git-pr.md` policy - **Verdict**: ✅ APPROVE — eligible for human merge (squash + delete branch) - **Post-merge**: spawn `qa-tester` для `curl POST /api/v1/parcels/<test_cad>/best-layouts` на high-density cad (ЕКБ центр) + проверка response shape + non-empty top_layouts
lekss361 merged commit ad16fc0e42 into main 2026-05-16 09:10:07 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
admin

Admin UI / internal tools / pipeline tracker

  
analytics

Аналитика данных / matching

  
auth

Authentication / authorization / multi-tenancy

  
automation

Автоматизация / cron / scheduler

  
bug

Something isn't working

  
business

Коммерческая воронка / пилоты / pipeline / реестр ПО

  
chore

Operational housekeeping

  
ci

Forgejo Actions / build pipeline

  
compliance

152-ФЗ / реестр ПО / legal / TOS

  
data

Data sources, parsers, ETL

  
data-moat

Уникальные данные — sales tracker, CRM aggregation

  
docs

README / runbooks / onboarding

  
duplicate

This issue or pull request already exists

  
dx

Developer experience / build speed

  
enhancement

New feature or request

  
Fable 5 ревью

Находки полного мульти-агентного ревью (Claude Fable 5, 2026-06-09)

  
feedback/max

Feedback from Max (Senior partner)

  
generative

Generative Design — concept generation engine

  
GG-форсайт

Новое ТЗ от Ген Геныча (июнь 2026): Site Finder v2 — ИИ-прогноз продуктовой стратегии девелопера. Актуальный приоритет.

  
needs-discussion

Pending team sync / decision

  
needs-human

Human review/decision required

  
observability

Monitoring / metrics / logs / Sentry / Uptime

  
pause-bots

Kill-switch — all autonomous bots pause

  
performance

Latency / throughput / DB query tuning

  
priority/p0

Critical / production-blocker

  
priority/p1

High priority

  
priority/p2

Normal priority

  
priority/p3

Низкий приоритет (follow-up, tech-debt)

  
scope/backend

Backend Python — FastAPI, Celery, scrapers

  
scope/db

DB migrations, indexes, SQL

  
scope/devops

docker-compose, Caddy, workflows

  
scope/frontend

Frontend — Next.js, React, TypeScript

  
scope/qa

Tests, smoke, Playwright

  
scrapers

Скрапперы (Avito/Cian/Yandex/Rosreestr)

  
security

Security audit / secrets / vulnerabilities

  
site-finder

Site Finder scoring quality & data layer

  
stage/1

Stage 1: Generative MVP (нед 2-7)

  
stage/2

Stage 2: Site Finder MVP (нед 8-12)

  
status/blocked

Blocked — needs human intervention

  
status/done

Closed, completed

  
status/needs-analysis

Raw/fuzzy task filed by human — auto-analyst подхватывает: code-archeology + декомпозиция в actionable scope/* + status/ready (или split на под-issues)

  
status/needs-fix

Reviewer/QA вернул worker на доработку — worker сам чинит свой PR (fixup), НЕ требует human (в отличие от status/blocked)

  
status/qa

PR merged, awaiting smoke test

  
status/ready

Ready for pickup by worker bot

  
status/review

PR open, awaiting code review

  
status/wip

Work in progress (worker claimed)

  
tech-debt

Технический долг

  
tradein

Trade-In MVP проект

  
ux

User experience / responsive / conversion

  
week ревью 1

Week review 1 — находки multi-agent аудита проекта (2026-06-14)

  
wontfix

This will not be worked on

  
вторичка

Trade-In Estimator — проект оценки вторичного жилья

  
ИРД

Исходно-разрешительная документация — слой ограничений участка для analyze-таблицы

No labels
admin
analytics
auth
automation
bug
business
chore
ci
compliance
data
data-moat
docs
duplicate
dx
enhancement
Fable 5 ревью
feedback/max
generative
GG-форсайт
needs-discussion
needs-human
observability
pause-bots
performance
priority/p0
priority/p1
priority/p2
priority/p3
scope/backend
scope/db
scope/devops
scope/frontend
scope/qa
scrapers
security
site-finder
stage/1
stage/2
status/blocked
status/done
status/needs-analysis
status/needs-fix
status/qa
status/ready
status/review
status/wip
tech-debt
tradein
ux
week ревью 1
wontfix
вторичка
ИРД
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: lekss361/gendesign#196
No description provided.
Delete branch "feat/113-best-layouts-endpoint"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?