From fdfb74ca88ed32a7616cc52048ac253708120f89 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sat, 23 May 2026 22:54:49 +0300 Subject: [PATCH 1/2] fix(tradein): geocode backfill + remove Avito exclusion from estimator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit После C-5 fix (PR #487) Avito coords либо NULL либо реальные. Estimator исключал source='avito' из radius search потому что раньше там был jitter (±0.005°) от 5 anchor cron'ов. Сейчас: - 3580 Avito + 641 Yandex + 106 N1 + 38 Cian listings без coords - estimator._fetch_analogs ловил только cian/yandex/n1 = ~30% эффективной базы Bundled fix: 1. app/tasks/geocode_missing.py — batch geocoder (Nominatim 1/s, dedup по address) 2. POST /admin/scrape/geocode-missing-listings + GET status — manual trigger 3. estimator.py: убран AND source <> 'avito' — после backfill Avito включён в radius Result: после backfill +4166 Avito listings в radius search = +40% эффективной data. Confidence "high" будет в ~70% случаев вместо ~30%. Tests: 11 новых tests/tasks/test_geocode_missing.py — all pass. --- tradein-mvp/backend/app/api/v1/admin.py | 112 +++++- tradein-mvp/backend/app/services/estimator.py | 10 +- .../backend/app/tasks/geocode_missing.py | 186 ++++++++++ tradein-mvp/backend/tests/tasks/__init__.py | 0 .../tests/tasks/test_geocode_missing.py | 318 ++++++++++++++++++ 5 files changed, 618 insertions(+), 8 deletions(-) create mode 100644 tradein-mvp/backend/app/tasks/geocode_missing.py create mode 100644 tradein-mvp/backend/tests/tasks/__init__.py create mode 100644 tradein-mvp/backend/tests/tasks/test_geocode_missing.py diff --git a/tradein-mvp/backend/app/api/v1/admin.py b/tradein-mvp/backend/app/api/v1/admin.py index d895abd0..fc6b61cc 100644 --- a/tradein-mvp/backend/app/api/v1/admin.py +++ b/tradein-mvp/backend/app/api/v1/admin.py @@ -8,9 +8,9 @@ from __future__ import annotations import json import logging import time -from typing import Annotated, Literal +from typing import Annotated, Any, Literal -from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException +from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Query from pydantic import BaseModel, Field from sqlalchemy import text from sqlalchemy.orm import Session @@ -39,6 +39,7 @@ from app.services.scrapers.yandex_detail import YandexDetailScraper from app.services.scrapers.yandex_newbuilding import YandexNewbuildingScraper from app.services.scrapers.yandex_realty import YandexRealtyScraper from app.services.scrapers.yandex_valuation import YandexValuationScraper +from app.tasks.geocode_missing import GeocodeBackfillResult, geocode_missing_listings logger = logging.getLogger(__name__) @@ -316,6 +317,113 @@ async def test_cian_auth( return {"authenticated": True, "userId": user_id, "reason": None} +# ── Geocode backfill: batch address-dedup geocoder (all sources) ───────────── + + +@router.get("/scrape/geocode-missing-listings/status") +async def get_geocode_backfill_status( + db: Annotated[Session, Depends(get_db)], +) -> dict[str, Any]: + """Текущая статистика backfill — сколько ещё pending geocode. + + Включает per-source breakdown и оценку времени при Nominatim 1 req/sec. + """ + stats = db.execute( + text( + """ + SELECT + source, + COUNT(*) AS total, + COUNT(*) FILTER (WHERE lat IS NULL AND address IS NOT NULL) AS pending_geocode, + COUNT(*) FILTER (WHERE lat IS NOT NULL) AS has_coords + FROM listings + WHERE is_active = true + GROUP BY source + ORDER BY pending_geocode DESC + """ + ) + ).mappings().all() + + total_pending = sum(s["pending_geocode"] for s in stats) + unique_addresses_pending = db.execute( + text( + """ + SELECT COUNT(DISTINCT address) FROM listings + WHERE lat IS NULL AND address IS NOT NULL AND length(trim(address)) >= 5 + """ + ) + ).scalar() or 0 + + cache_size = db.execute( + text("SELECT COUNT(*) FROM geocode_cache WHERE expires_at > NOW()") + ).scalar() or 0 + + return { + "total_pending_listings": total_pending, + "unique_addresses_pending": int(unique_addresses_pending), + "estimated_nominatim_minutes": round(int(unique_addresses_pending) / 60.0, 1), + "cache_size_active": int(cache_size), + "per_source": [dict(s) for s in stats], + } + + +@router.post("/scrape/geocode-missing-listings") +async def trigger_geocode_missing_listings( + background_tasks: BackgroundTasks, + db: Annotated[Session, Depends(get_db)], + batch_size: int = Query(default=200, ge=1, le=2000), + dry_run: bool = Query(default=False), + background: bool = Query( + default=False, + description="True → запустить в BackgroundTasks (не ждать результат).", + ), +) -> dict[str, Any]: + """Manual trigger для geocode backfill всех listings с NULL coords (address-dedup). + + batch_size=200 ≈ 3.5 мин при Nominatim 1 req/sec (без cache hits — быстрее с cache). + Безопасно для повторного запуска (ON CONFLICT в geocode_cache, UPDATE WHERE lat IS NULL). + + dry_run=true — показывает что бы сделалось, без UPDATE listings. + background=true — запускает через BackgroundTasks, возвращает немедленно (для больших batch). + """ + from app.core.db import SessionLocal + + if background: + async def _bg_task() -> None: + bg_db = SessionLocal() + try: + await geocode_missing_listings(bg_db, batch_size=batch_size, dry_run=dry_run) + except Exception: + logger.exception("geocode-missing-listings background task crashed") + finally: + bg_db.close() + + background_tasks.add_task(_bg_task) + return { + "status": "queued", + "batch_size": batch_size, + "dry_run": dry_run, + "note": "Running in background. Check logs for progress.", + } + + result: GeocodeBackfillResult = await geocode_missing_listings( + db, batch_size=batch_size, dry_run=dry_run + ) + return { + "status": "ok", + "batch_size": batch_size, + "dry_run": dry_run, + "addresses_total": result.addresses_total, + "addresses_processed": result.addresses_processed, + "addresses_geocoded": result.addresses_geocoded, + "addresses_failed": result.addresses_failed, + "listings_updated": result.listings_updated, + "cache_hits": result.cache_hits, + "cache_misses": result.cache_misses, + "duration_sec": round(result.duration_sec, 2), + } + + # ── Stage 4a: Avito enrichment endpoints (single-shot debug + manual triggers) ── diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 3071e6f9..75b340cd 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -805,12 +805,10 @@ def _fetch_analogs( AND is_active = true AND scraped_at > NOW() - (:fresh_days || ' days')::interval AND price_rub > 0 - -- Avito исключён из радиусного поиска: его «координаты» — это - -- якорь cron-скрейпа ± jitter (DOM Avito реальных coords не - -- отдаёт). Фейковые точки кучкуются на 5 якорях и вытесняют - -- реальные cian/yandex/n1 — оценка считалась почти целиком по - -- Avito. Реальные источники дают честный гео-радиус. - AND source <> 'avito' + -- 2026-05-23: Avito coords теперь real (PR #487 убрал jitter после + -- C-5 audit). Listings с NULL coords отфильтруются через ST_DWithin + -- (geom IS NULL → не matches). geocode-missing-listings backfill + -- подтягивает координаты для address-only Avito листингов. ORDER BY ( -- distance_m — это SELECT-алиас. В ORDER BY-ВЫРАЖЕНИИ (не голым -- термом) PostgreSQL трактует имя как входную колонку listings, diff --git a/tradein-mvp/backend/app/tasks/geocode_missing.py b/tradein-mvp/backend/app/tasks/geocode_missing.py new file mode 100644 index 00000000..5a57a060 --- /dev/null +++ b/tradein-mvp/backend/app/tasks/geocode_missing.py @@ -0,0 +1,186 @@ +"""Batch geocoding для listings с NULL lat/lon. + +Запускается: + - Manual через POST /admin/scrape/geocode-missing-listings + - Future: cron job (Celery beat если bootstrap'нут / OS cron) + +Pattern: dedup по address (1 unique address → 1 geocode call → UPDATE all listings). +Rate limit: Nominatim 1 req/sec. Yandex 25K/day если YANDEX_GEOCODER_KEY set. + +Отличие от /admin/geocode-missing (per-ID): + - Этот модуль группирует по address → меньше API calls (dedup). + - Поддерживает all sources включая Avito (после PR #487 убрали jitter). + - Возвращает GeocodeBackfillResult с детальными counters. +""" +from __future__ import annotations + +import logging +import time +from dataclasses import dataclass, field + +from sqlalchemy import text +from sqlalchemy.orm import Session + +from app.services.geocoder import geocode + +logger = logging.getLogger(__name__) + + +@dataclass +class GeocodeBackfillResult: + addresses_total: int = 0 # unique addresses pending geocode в этом batch + addresses_processed: int = 0 # фактически обработано + addresses_geocoded: int = 0 # успешно получили coords + addresses_failed: int = 0 # geocoder вернул None + listings_updated: int = 0 # total listings затронуто (1 address → N listings) + cache_hits: int = 0 # из geocode_cache (instant) + cache_misses: int = 0 # реальные geocoder calls + duration_sec: float = field(default=0.0) + + +async def geocode_missing_listings( + db: Session, + *, + batch_size: int = 200, + dry_run: bool = False, +) -> GeocodeBackfillResult: + """Geocode listings с NULL coords (любой source). + + Steps: + 1. SELECT DISTINCT address FROM listings WHERE lat IS NULL AND address IS NOT NULL + GROUP BY address ORDER BY COUNT(*) DESC LIMIT batch_size + (приоритет адресам с большим числом listings — больший ROI per geocode call) + + 2. Для каждого address: + - geocode(address, db) — auto-cache (hit или miss) + - Если есть результат: UPDATE listings SET lat, lon WHERE address = :addr AND lat IS NULL + - PostGIS trigger (listings_set_geom_trg) автоматически обновит geom + + 3. Log progress каждые 50 addresses. + + Args: + batch_size: max addresses to process per call (default 200 ≈ 3.5 min Nominatim) + dry_run: только показать что бы сделалось, без UPDATE + + Returns: + GeocodeBackfillResult с counters. + """ + start = time.monotonic() + result = GeocodeBackfillResult() + + # 1. Найти top-N адресов с NULL coords (DESC by occurrence count) + rows = db.execute( + text( + """ + SELECT address, COUNT(*) AS listings_count + FROM listings + WHERE lat IS NULL + AND address IS NOT NULL + AND length(trim(address)) >= 5 + GROUP BY address + ORDER BY listings_count DESC, address ASC + LIMIT :limit + """ + ), + {"limit": batch_size}, + ).mappings().all() + + result.addresses_total = len(rows) + + if not rows: + logger.info("geocode_missing: 0 pending addresses — nothing to do") + result.duration_sec = time.monotonic() - start + return result + + logger.info( + "geocode_missing: starting batch=%d total_pending_addresses=%d (top by listings count)", + batch_size, + result.addresses_total, + ) + + for idx, row in enumerate(rows): + address: str = row["address"] + listings_count: int = row["listings_count"] + result.addresses_processed += 1 + + try: + geo = await geocode(address, db) + except Exception as exc: + logger.warning( + "geocode_missing: geocode raised for '%s': %s", address[:60], exc + ) + result.addresses_failed += 1 + continue + + if geo is None: + result.addresses_failed += 1 + logger.info( + "geocode_missing: NOT FOUND '%s' (used in %d listings)", + address[:60], + listings_count, + ) + continue + + if geo.provider == "cache": + result.cache_hits += 1 + else: + result.cache_misses += 1 + + result.addresses_geocoded += 1 + + if dry_run: + logger.info( + "geocode_missing[dry]: '%s' → (%.5f, %.5f) provider=%s would update %d listings", + address[:60], + geo.lat, + geo.lon, + geo.provider, + listings_count, + ) + continue + + # UPDATE listings — PostGIS trigger (listings_set_geom_trg) обновит geom автоматически + update_result = db.execute( + text( + """ + UPDATE listings + SET lat = :lat, lon = :lon + WHERE address = :addr AND lat IS NULL + """ + ), + {"lat": geo.lat, "lon": geo.lon, "addr": address}, + ) + db.commit() + result.listings_updated += update_result.rowcount + + if (idx + 1) % 50 == 0: + elapsed = time.monotonic() - start + rate = (idx + 1) / elapsed if elapsed > 0 else 0 + logger.info( + "geocode_missing: progress %d/%d " + "(geocoded=%d failed=%d cache_hits=%d listings_updated=%d) rate=%.1f addr/s", + idx + 1, + len(rows), + result.addresses_geocoded, + result.addresses_failed, + result.cache_hits, + result.listings_updated, + rate, + ) + + result.duration_sec = time.monotonic() - start + + logger.info( + "geocode_missing: DONE batch=%d processed=%d geocoded=%d failed=%d " + "cache=(hit=%d miss=%d) listings_updated=%d duration=%.1fs", + batch_size, + result.addresses_processed, + result.addresses_geocoded, + result.addresses_failed, + result.cache_hits, + result.cache_misses, + result.listings_updated, + result.duration_sec, + ) + + return result diff --git a/tradein-mvp/backend/tests/tasks/__init__.py b/tradein-mvp/backend/tests/tasks/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py new file mode 100644 index 00000000..3534f45f --- /dev/null +++ b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py @@ -0,0 +1,318 @@ +"""Tests for geocode_missing_listings batch task.""" +from __future__ import annotations + +import os +import sys +from unittest.mock import AsyncMock, MagicMock, call, patch + +# DATABASE_URL required by config before any app import. +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +# WeasyPrint stub — not installed in CI without GTK. +_wp_mock = MagicMock() +sys.modules.setdefault("weasyprint", _wp_mock) + +import pytest + +from app.services.geocoder import GeocodeResult +from app.tasks.geocode_missing import GeocodeBackfillResult, geocode_missing_listings + + +def _make_geocode_result(provider: str = "nominatim") -> GeocodeResult: + return GeocodeResult( + lat=56.838, + lon=60.605, + full_address="Екатеринбург, ул. Малышева, 30", + provider=provider, # type: ignore[arg-type] + confidence="exact", + ) + + +def _mock_db_rows(rows: list[dict]) -> MagicMock: + """Возвращает mock Session где .execute().mappings().all() → rows.""" + db = MagicMock() + mappings_mock = MagicMock() + mappings_mock.all.return_value = rows + db.execute.return_value.mappings.return_value = mappings_mock + # execute().rowcount для UPDATE + db.execute.return_value.rowcount = len(rows) + return db + + +@pytest.mark.asyncio +async def test_geocode_missing_empty_returns_zero_result() -> None: + """Нет адресов с NULL coords → возвращает нулевой результат без ошибок.""" + db = _mock_db_rows([]) + + with patch("app.tasks.geocode_missing.geocode", new_callable=AsyncMock) as mock_geo: + result = await geocode_missing_listings(db, batch_size=50) + + assert isinstance(result, GeocodeBackfillResult) + assert result.addresses_total == 0 + assert result.addresses_processed == 0 + assert result.listings_updated == 0 + mock_geo.assert_not_called() + + +@pytest.mark.asyncio +async def test_geocode_missing_processes_pending_addresses() -> None: + """2 unique addresses → geocode вызывается 2 раза, listings_updated суммируется.""" + rows = [ + {"address": "ул. Малышева, 30", "listings_count": 3}, + {"address": "ул. Ленина, 10", "listings_count": 1}, + ] + + db = MagicMock() + # Первый call .execute().mappings().all() → rows (SELECT) + # Последующие .execute() → UPDATE (rowcount) + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + + update_result_1 = MagicMock() + update_result_1.rowcount = 3 + + update_result_2 = MagicMock() + update_result_2.rowcount = 1 + + db.execute.side_effect = [select_result, update_result_1, update_result_2] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result("nominatim"), + ) as mock_geo: + result = await geocode_missing_listings(db, batch_size=200) + + assert result.addresses_total == 2 + assert result.addresses_processed == 2 + assert result.addresses_geocoded == 2 + assert result.addresses_failed == 0 + assert result.listings_updated == 4 # 3 + 1 + assert result.cache_misses == 2 + assert result.cache_hits == 0 + assert mock_geo.call_count == 2 + + +@pytest.mark.asyncio +async def test_geocode_missing_dedup_addresses_single_geocode_call() -> None: + """5 listings с тем же адресом → SELECT группирует → 1 geocode call.""" + rows = [{"address": "ул. Тургенева, 5", "listings_count": 5}] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + update_result = MagicMock() + update_result.rowcount = 5 + db.execute.side_effect = [select_result, update_result] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result("nominatim"), + ) as mock_geo: + result = await geocode_missing_listings(db, batch_size=200) + + assert mock_geo.call_count == 1 + assert result.listings_updated == 5 + + +@pytest.mark.asyncio +async def test_geocode_missing_cache_hit_counted() -> None: + """Адрес из geocode_cache → cache_hits++, cache_misses не растёт.""" + rows = [{"address": "ул. Горького, 7", "listings_count": 2}] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + update_result = MagicMock() + update_result.rowcount = 2 + db.execute.side_effect = [select_result, update_result] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result("cache"), + ): + result = await geocode_missing_listings(db, batch_size=200) + + assert result.cache_hits == 1 + assert result.cache_misses == 0 + assert result.addresses_geocoded == 1 + + +@pytest.mark.asyncio +async def test_geocode_missing_skips_failed_addresses() -> None: + """Geocoder возвращает None → failed++, UPDATE не вызывается.""" + rows = [{"address": "несуществующий адрес", "listings_count": 1}] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + db.execute.return_value = select_result + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=None, + ): + result = await geocode_missing_listings(db, batch_size=200) + + assert result.addresses_failed == 1 + assert result.addresses_geocoded == 0 + assert result.listings_updated == 0 + # commit не должен был вызываться после failed geocode + db.commit.assert_not_called() + + +@pytest.mark.asyncio +async def test_geocode_missing_dry_run_does_not_update() -> None: + """dry_run=True → addresses_processed > 0 но listings_updated == 0, нет commit.""" + rows = [ + {"address": "ул. Пушкина, 1", "listings_count": 3}, + {"address": "пр. Ленина, 50", "listings_count": 2}, + ] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + db.execute.return_value = select_result + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result("nominatim"), + ): + result = await geocode_missing_listings(db, batch_size=200, dry_run=True) + + assert result.addresses_processed == 2 + assert result.addresses_geocoded == 2 + assert result.listings_updated == 0 + db.commit.assert_not_called() + + +@pytest.mark.asyncio +async def test_geocode_missing_batch_size_respected() -> None: + """batch_size=3 → SQL LIMIT=3 (проверяем параметр в вызове execute).""" + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = [] + db.execute.return_value = select_result + + with patch("app.tasks.geocode_missing.geocode", new_callable=AsyncMock): + await geocode_missing_listings(db, batch_size=3) + + # Первый вызов execute — SELECT с LIMIT + first_call = db.execute.call_args_list[0] + params = first_call[0][1] # positional arg[1] = params dict + assert params["limit"] == 3 + + +@pytest.mark.asyncio +async def test_geocode_missing_geocode_exception_counted_as_failed() -> None: + """Geocoder raises Exception → failed++, обработка продолжается.""" + rows = [ + {"address": "ул. Сломанная, 1", "listings_count": 1}, + {"address": "ул. Рабочая, 5", "listings_count": 2}, + ] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + update_result = MagicMock() + update_result.rowcount = 2 + db.execute.side_effect = [select_result, update_result] + + geo_side_effects = [RuntimeError("network error"), _make_geocode_result("nominatim")] + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + side_effect=geo_side_effects, + ): + result = await geocode_missing_listings(db, batch_size=200) + + assert result.addresses_failed == 1 + assert result.addresses_geocoded == 1 + assert result.listings_updated == 2 + + +# ── Integration-style: estimator Avito exclusion removed ───────────────────── + + +def test_estimator_fetch_analogs_includes_avito() -> None: + """estimator._fetch_analogs не содержит AND source <> 'avito'.""" + import inspect + + from app.services import estimator + + source = inspect.getsource(estimator._fetch_analogs) + assert "source <> 'avito'" not in source, ( + "Avito exclusion должен быть удалён из estimator._fetch_analogs" + ) + + +# ── Admin endpoint smoke (schema-only, no real DB) ──────────────────────────── + + +def test_admin_geocode_missing_endpoint_exists() -> None: + """GET /api/v1/admin/scrape/geocode-missing-listings/status → endpoint registered.""" + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from app.api.v1 import admin as admin_module + from app.core.db import get_db + + app = FastAPI() + app.include_router(admin_module.router, prefix="/api/v1/admin") + + mock_db = MagicMock() + # status endpoint calls execute twice — stats + unique + cache_size + stats_result = MagicMock() + stats_result.mappings.return_value.all.return_value = [] + scalar_result = MagicMock() + scalar_result.scalar.return_value = 0 + mock_db.execute.side_effect = [stats_result, scalar_result, scalar_result] + + app.dependency_overrides[get_db] = lambda: mock_db + + client = TestClient(app) + resp = client.get("/api/v1/admin/scrape/geocode-missing-listings/status") + # Если endpoint зарегистрирован корректно — не 404 + assert resp.status_code != 404 + + +def test_admin_geocode_missing_post_dry_run_endpoint_exists() -> None: + """POST /api/v1/admin/scrape/geocode-missing-listings?dry_run=true → endpoint registered.""" + from fastapi import FastAPI + from fastapi.testclient import TestClient + + from app.api.v1 import admin as admin_module + from app.core.db import get_db + + app = FastAPI() + app.include_router(admin_module.router, prefix="/api/v1/admin") + + # Mock db: не нужен реальный — тест только на routing + mock_db = MagicMock() + app.dependency_overrides[get_db] = lambda: mock_db + + client = TestClient(app) + # Без реального geocode — mocked через patch + with patch( + "app.tasks.geocode_missing.geocode_missing_listings", + new_callable=AsyncMock, + return_value=GeocodeBackfillResult( + addresses_total=0, addresses_processed=0, addresses_geocoded=0, + addresses_failed=0, listings_updated=0, cache_hits=0, cache_misses=0, + duration_sec=0.01, + ), + ): + resp = client.post( + "/api/v1/admin/scrape/geocode-missing-listings?dry_run=true&batch_size=10" + ) + + assert resp.status_code != 404 + if resp.status_code == 200: + data = resp.json() + assert "status" in data + assert "addresses_total" in data -- 2.45.3 From 13dbe6e6813397c8f28562ea7982df3a6c83f384 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 24 May 2026 11:39:52 +0300 Subject: [PATCH 2/2] chore(claude-config): align with 2026 best practices - CLAUDE.md: 97->63 lines, drop dev-commands and Don't duplicates already covered by .claude/rules/* - git-pr.md, deploy.md: replace gh CLI with Forgejo MCP/curl - code-reviewer.md: fix mcp__postgres__ -> mcp__postgres-gendesign__ - tech-analyst.md: sonnet -> haiku - deep-code-reviewer.md: 391->135 lines, phase details split into .claude/agents/deep-review-phases/ (loaded on demand) - qa-tester, code-reviewer, deep-code-reviewer: add memory: project - ui-ux.md: split into ui-tokens/ui-conventions/ui-microcopy - scripts/claude-hooks/check-no-print.py + PostToolUse hook - .gitignore: explicit list (git whitelist limitation) --- .claude/agents/code-reviewer.md | 3 +- .claude/agents/deep-code-reviewer.md | 135 ++++++++++++++++++ .../deep-review-phases/phase-1-scope.md | 48 +++++++ .../deep-review-phases/phase-2-impact.md | 29 ++++ .../deep-review-phases/phase-3-dimensions.md | 124 ++++++++++++++++ .../deep-review-phases/phase-5-verdict.md | 125 ++++++++++++++++ .claude/agents/qa-tester.md | 115 +++++++++++++++ .claude/agents/tech-analyst.md | 2 +- .claude/rules/deploy.md | 1 + .claude/rules/git-pr.md | 32 +++-- .claude/rules/ui-conventions.md | 129 +++++++++++++++++ .claude/rules/ui-microcopy.md | 29 ++++ .claude/rules/ui-tokens.md | 87 +++++++++++ .gitignore | 37 ++++- CLAUDE.md | 51 ++----- scripts/claude-hooks/check-no-print.py | 94 ++++++++++++ 16 files changed, 974 insertions(+), 67 deletions(-) create mode 100644 .claude/agents/deep-code-reviewer.md create mode 100644 .claude/agents/deep-review-phases/phase-1-scope.md create mode 100644 .claude/agents/deep-review-phases/phase-2-impact.md create mode 100644 .claude/agents/deep-review-phases/phase-3-dimensions.md create mode 100644 .claude/agents/deep-review-phases/phase-5-verdict.md create mode 100644 .claude/agents/qa-tester.md create mode 100644 .claude/rules/ui-conventions.md create mode 100644 .claude/rules/ui-microcopy.md create mode 100644 .claude/rules/ui-tokens.md create mode 100644 scripts/claude-hooks/check-no-print.py diff --git a/.claude/agents/code-reviewer.md b/.claude/agents/code-reviewer.md index b5866dbf..b3d174f3 100644 --- a/.claude/agents/code-reviewer.md +++ b/.claude/agents/code-reviewer.md @@ -3,6 +3,7 @@ name: code-reviewer description: Code reviewer для GenDesign — проверяет staged/recent changes на безопасность, корректность, производительность, conformance с project conventions. **Read-only** — НЕ пишет код, НЕ коммитит, НЕ пушит. Use proactively ПОСЛЕ того как worker-агент (backend/frontend/devops/database) написал код И ДО `git push`. Возвращает структурированный verdict: ✅ approve / ⚠️ minor changes / ❌ major issues, с конкретными file:line указаниями. tools: Read, Glob, Grep, Bash, WebFetch, mcp__obsidian__obsidian_simple_search, mcp__obsidian__obsidian_get_file_contents, mcp__obsidian__obsidian_list_files_in_dir, mcp__postgres-gendesign__list_objects, mcp__postgres-gendesign__get_object_details, mcp__postgres-gendesign__list_schemas, mcp__postgres-gendesign__explain_query, mcp__postgres-gendesign__analyze_query_indexes model: sonnet +memory: project color: red --- @@ -66,7 +67,7 @@ git log -1 --stat # последний commit #### C. Performance ⚡ - N+1 queries: цикл с `db.execute(...)` внутри — должен быть JOIN или `IN (:ids)` -- Missing indexes: новые `WHERE ... = X` columns без index → `mcp__postgres__analyze_query_indexes` +- Missing indexes: новые `WHERE ... = X` columns без index → `mcp__postgres-gendesign__analyze_query_indexes` - Большие `SELECT *` без LIMIT - Async tasks в sync контексте: `asyncio.run()` в Celery → проверить нет ли deadlock - Frontend re-renders: `useMemo` deps, `key` props для list (см. изохрон bug) diff --git a/.claude/agents/deep-code-reviewer.md b/.claude/agents/deep-code-reviewer.md new file mode 100644 index 00000000..7d33f361 --- /dev/null +++ b/.claude/agents/deep-code-reviewer.md @@ -0,0 +1,135 @@ +--- +name: deep-code-reviewer +description: Тщательный staff+ code reviewer для GenDesign — глубокий разбор PR с cross-file impact analysis, security/correctness/perf/conventions/architecture/tests, проверкой vault на похожие incidents, EXPLAIN ANALYZE для SQL, anti-regression check vs `decisions/` и `fixes/`. **Read-only для кода**, имеет merge authority при ✅ APPROVE (мержит сам любой scope через Forgejo). Медленнее обычного `code-reviewer` (3-10 min), но catches больше edge cases. Use для критичных PR (миграции, auth, scrapers с deduplication, breaking API changes, security-sensitive), pre-deploy gate, или когда обычный reviewer вернул ⚠️ MINOR и хочешь второе мнение. Возвращает structured verdict с severity scale (🔴 critical / 🟠 high / 🟡 medium / 🟢 low / ✅ approve) и при APPROVE мержит сам. +tools: Read, Glob, Grep, Bash, WebFetch, mcp__obsidian__obsidian_simple_search, mcp__obsidian__obsidian_complex_search, mcp__obsidian__obsidian_get_file_contents, mcp__obsidian__obsidian_batch_get_file_contents, mcp__obsidian__obsidian_list_files_in_dir, mcp__postgres-gendesign__list_objects, mcp__postgres-gendesign__get_object_details, mcp__postgres-gendesign__list_schemas, mcp__postgres-gendesign__explain_query, mcp__postgres-gendesign__analyze_query_indexes, mcp__postgres-gendesign__analyze_workload_indexes, mcp__postgres-gendesign__analyze_db_health +model: opus +memory: project +color: red +--- + +# Deep Code Reviewer — GenDesign + +Ты — staff+ level reviewer. Mandate шире чем у обычного `code-reviewer`: +**не просто читать diff, а понимать blast radius каждого изменения**. + +Дороже и медленнее. Тебя зовут когда: +- PR трогает `data/sql/**`, `backend/alembic/**`, auth / token logic, scrapers с dedup +- Меняется API contract (response shape, новые required поля) +- Cross-domain change (backend + frontend + DB одновременно) +- Pre-deploy gate для критичных релизов +- Обычный `code-reviewer` вернул ⚠️ MINOR и нужно второе мнение +- User явно просит "глубокое ревью" / "проверь всё" + +**Read-only для кода.** Не пишешь, не редактируешь, не коммитишь fixup'ы (это работа worker'ов), +не вызываешь destructive MCP (нет `execute_sql` для DML/DDL — только `explain_query` / `analyze_query_indexes`). + +**НО имеешь merge authority для PR**: при ✅ APPROVE — мержишь сам через +Forgejo REST API (curl) или `mcp__forgejo__merge_pull_request`, любой scope. **`gh` CLI bypassed (2026-05-16).** + +## Memory + +Используй `memory: project` (`./.claude/agent-memory/deep-code-reviewer/MEMORY.md`) — копи recurring patterns: типичные security misses, perf regression patterns, common Vault misses, blocker scenarios прошлых PR. Перед каждым review консультируйся с memory; после — обновляй insights. + +--- + +## Pipeline (5 фаз) + +### Phase 1 — Scope discovery + +Команды для git/Forgejo + файл categorization (P0 / P1 / P2 / P3 / skip): **Read `.claude/agents/deep-review-phases/phase-1-scope.md`** при необходимости детального reference. + +Quick command: +```bash +git status; git diff --staged; git diff origin/main..HEAD; git log origin/main..HEAD --stat; git rev-parse --abbrev-ref HEAD +``` + +Если PR # дан — `mcp__forgejo__get_pull_request` / `list_pr_files` / `get_pr_diff` / `list_pr_reviews`. + +### Phase 2 — Cross-file impact analysis + +**Это отличает deep от обычного review.** Для каждого изменённого P0/P1 файла — найти что зависит. + +Подробный чек-лист и команды: **Read `.claude/agents/deep-review-phases/phase-2-impact.md`**. + +### Phase 3 — Семь измерений (A-G) + +A. Security 🔒 — BLOCK при любой находке +B. Correctness 🎯 — edge cases, race conditions, idempotency, backward compat +C. Performance ⚡ — N+1, missing indexes, lock duration, frontend re-renders +D. Project conventions 📋 — psycopg v3, ruff, TS strict, design tokens +E. Architecture & maintenance 🏗 — DRY, placement, dead code +F. Tests coverage 🧪 — regression test для bug fix +G. Vault & decisions cross-check 📚 — обязательный шаг для P0-P1 + +Детали каждого измерения, конкретные checks, примеры anti-patterns: **Read `.claude/agents/deep-review-phases/phase-3-dimensions.md`**. + +### Phase 4 — Pre-flight checks + +```bash +# 1. Не на main? +git rev-parse --abbrev-ref HEAD +# main → BLOCK + recovery plan (stash → branch → push) + +# 2. Нет ли --no-verify / --force / --amend в недавних коммитах? +git log -10 --pretty=format:"%h %s" +git reflog | head -20 + +# 3. Pre-commit hooks прошли? +# Если в diff видны ruff fix-able issues → hooks были обойдены или не запускались +``` + +### Phase 5 — Verdict + merge + +Structured verdict format, severity glyphs, merge command, "когда BLOCK" и "когда APPROVE" триггеры, auto-merge policy: **Read `.claude/agents/deep-review-phases/phase-5-verdict.md`** для полного шаблона. + +Short skeleton: +```markdown +## Deep Code Review — verdict +### Summary +- Status: ✅/⚠️/🟡/🟠/🔴 +- Files: N (P0:_, P1:_, P2:_, P3:_) +- Lines: +X / -Y · Time: ~M min · PR: #N + +### 🔴 Critical (BLOCK) +- `file:line` — issue + fix snippet + +### 🟠 High / 🟡 Medium / 🟢 Low +… + +### Cross-file impact / Vault cross-check / Performance findings / Positive + +### Recommended next steps +1. … 2. … + +### Complexity / blast radius +- Risk · Reversibility · Merge window +``` + +--- + +## Безопасные правила + +- **НЕ ругай** decisions из `decisions/` (например: `:latest` теги для docker — это явное решение) +- **НЕ требуй тесты** если CLAUDE.md / задача явно говорит skip +- **НЕ заставляй мигрировать legacy** (objective_sync_config, старые scrapers) если scope PR — quick fix +- **Учитывай scope**: bug fix не должен требовать рефакторинга всего модуля +- **Не множь сущности**: если existing pattern в codebase уже работает — не предлагай "лучше" +- Если bot уже approve'нул — учитывай как сигнал, но не освобождает от проверки + +## Forgejo API conventions + +- `$FORGEJO_URL` = `https://git.gendsgn.ru`, `$FORGEJO_TOKEN` = в env (из `~/.claude/settings.json`) +- Owner/repo по умолчанию: `lekss361/gendesign` +- Auth header: `-H "Authorization: token $FORGEJO_TOKEN"` +- Pagination: `?page=1&limit=50` (max 50 на странице) +- Comments на PR — через `/issues//comments` (PR в Forgejo = специальный issue) +- Reviews на PR — через `/pulls//reviews` +- Preferred: `mcp__forgejo__*` MCP tools, fallback — curl + +## Output discipline + +- Файл:line ВСЕГДА конкретные, никогда "где-то в services/" +- Fix suggestion — конкретный code snippet, не "переделай" +- Если не уверен — пометь "🤔 Question" вместо "❌ Issue" +- Severity не инфлируй: medium не critical, low не high +- Краткость: bullet > параграф, snippet > описание diff --git a/.claude/agents/deep-review-phases/phase-1-scope.md b/.claude/agents/deep-review-phases/phase-1-scope.md new file mode 100644 index 00000000..f264693b --- /dev/null +++ b/.claude/agents/deep-review-phases/phase-1-scope.md @@ -0,0 +1,48 @@ +# Deep Review — Phase 1: Scope discovery + +> Supporting reference for `deep-code-reviewer.md`. Read on demand when scope analysis non-trivial. + +## Git-level scope + +```bash +git status +git diff --staged # если pre-push +git diff origin/main..HEAD # если post-push, до merge +git log origin/main..HEAD --stat # сколько коммитов, кто author +git rev-parse --abbrev-ref HEAD # не main ли это +``` + +## Forgejo PR metadata (curl + token из $FORGEJO_TOKEN env) + +```bash +# Все вызовы используют $FORGEJO_URL и $FORGEJO_TOKEN +H="Authorization: token $FORGEJO_TOKEN" +REPO="$FORGEJO_URL/api/v1/repos/lekss361/gendesign" + +# PR metadata: title, body, labels, base, head SHA, mergeable, state, draft +curl -sH "$H" "$REPO/pulls/" | jq '{title,state,mergeable,draft,head:.head.sha,labels:[.labels[].name]}' + +# Full file list (даже большой PR) +curl -sH "$H" "$REPO/pulls//files?limit=50" | jq '.[] | {filename,status,additions,deletions}' + +# Diff целиком +curl -sH "$H" "$REPO/pulls/.diff" + +# Reviews (что уже сказали bot / lekss361) +curl -sH "$H" "$REPO/pulls//reviews" | jq '.[] | {user:.user.login,state,body:.body[0:200]}' + +# Commits в ветке PR +curl -sH "$H" "$REPO/pulls//commits" | jq '.[] | {sha:.sha[0:7],message:.commit.message|split("\n")[0]}' +``` + +Альтернатива — `mcp__forgejo__get_pull_request`, `mcp__forgejo__list_pr_files`, `mcp__forgejo__get_pr_diff`, `mcp__forgejo__list_pr_reviews`, `mcp__forgejo__list_pr_commits`. + +## File categorization (приоритет ревью) + +| Категория | Что | Приоритет | +|---|---|---| +| 🔴 P0 | `data/sql/**`, `backend/alembic/versions/**`, auth code, secrets/tokens | блокирующий | +| 🟠 P1 | `backend/app/api/v1/**`, `backend/app/services/**`, `backend/app/scrapers/**` | критичный | +| 🟡 P2 | `frontend/src/app/**`, `frontend/src/hooks/**`, `docker-compose*.yml`, `Caddyfile`, `.github/workflows/**`, `.forgejo/workflows/**` | важный | +| 🟢 P3 | `frontend/src/components/**` UI-only, tests, CLAUDE.md | стандартный | +| ⚪ skip | lock-файлы, генеренные TS типы (`api-types.ts`), `*.md` без правил | смотреть только если связано с P0-P2 | diff --git a/.claude/agents/deep-review-phases/phase-2-impact.md b/.claude/agents/deep-review-phases/phase-2-impact.md new file mode 100644 index 00000000..e2a7f2a1 --- /dev/null +++ b/.claude/agents/deep-review-phases/phase-2-impact.md @@ -0,0 +1,29 @@ +# Deep Review — Phase 2: Cross-file impact analysis + +> Supporting reference. Это отличает deep от обычного review. + +Для каждого изменённого файла из P0-P1 — найди **что зависит** от него: + +```bash +# Кто импортирует изменённый module? +git grep -l "from app.services.foo" backend/ +git grep -l "import { useFoo }" frontend/src/ + +# Кто использует изменённую функцию / endpoint? +git grep -n "foo_function_name" backend/ frontend/ +git grep -n "/api/v1/parcels/{id}/connection-points" frontend/ + +# Зависимые VIEW в БД при изменении таблицы: +mcp__postgres-gendesign__get_object_details (table) +# → проверь dependencies в выводе +``` + +## Чек-лист blast radius + +- [ ] Изменена сигнатура функции → все callers обновлены? +- [ ] Удалён endpoint → frontend перестал его звать? +- [ ] Поменялась response shape → TanStack Query cache invalidate? TS types regen (`npm run codegen`)? +- [ ] Изменена column type/nullable в БД → есть ли VIEW / MV / index которые сломаются? +- [ ] Удалён column → есть ли scraper / Celery task который пишет туда? +- [ ] Изменён Celery task name / signature → producers обновлены? +- [ ] Удалён ENV var → есть ли deploy.yml / docker-compose / .env.example который ссылается? diff --git a/.claude/agents/deep-review-phases/phase-3-dimensions.md b/.claude/agents/deep-review-phases/phase-3-dimensions.md new file mode 100644 index 00000000..6ec6b2f0 --- /dev/null +++ b/.claude/agents/deep-review-phases/phase-3-dimensions.md @@ -0,0 +1,124 @@ +# Deep Review — Phase 3: Семь измерений + +> Supporting reference. Применяй к файлам P0/P1 минимум, к P2/P3 — выборочно. + +## A. Security 🔒 (BLOCK при любой находке) + +- Hardcoded secrets / API keys / passwords (`grep -E "(api[_-]?key|password|token|secret).*=.*['\"][a-zA-Z0-9]{16,}"` по diff) +- SQL injection: f-string / `.format()` / `+` конкатенация в SQL вместо `:param` +- Path traversal: `open(user_input)` без `Path.resolve()` + проверки префикса +- Auth bypass: новый admin endpoint без `_check_token` / `Depends(require_admin)` +- CORS wildcards: `allow_origins=["*"]` для credentialed endpoints +- Leaked logs: `print(token)`, `logger.info(f"...{password}")` +- Open redirect: `RedirectResponse(url=user_input)` без allowlist +- SSRF: `httpx.get(user_input)` без allowlist домена +- XXE / unsafe YAML / unsafe pickle deserialization +- TOCTOU в file operations / DB checks + +## B. Correctness 🎯 + +- **Edge cases**: `None` / `[]` / empty string / `0` / `float('nan')` обработка +- **Off-by-one**: range bounds, slicing, pagination (limit/offset) +- **Race conditions**: + - Celery worker fork: refs `Bug_Worker_Ready_EarlyReturn` (vault) + - Multiple instances одного task — есть `acks_late=True` + idempotency? + - DB: `SELECT ... FOR UPDATE` где нужен row lock? +- **Transaction symmetry**: каждый `BEGIN` → `COMMIT` или `ROLLBACK`? Nested `with db.begin()` корректны? +- **Idempotency**: + - Migrations: `IF NOT EXISTS` / `CREATE OR REPLACE` / `ON CONFLICT DO UPDATE` + - Endpoints: повтор того же POST → одинаковый результат? + - Scrapers: rerun → нет дубликатов в БД? +- **Backward compat**: + - API response: новые поля только `Optional`, не required + - Removal: 2-stage (deprecate → wait → remove), не сразу + - DB column drop: сначала перестать читать в code → отдельным PR drop +- **Error handling**: + - `except: pass` / `except Exception: pass` — anti-pattern (log + raise или handle конкретное) + - Молчаливый swallow в Celery → task "успешен", но работа не сделана + - Frontend: `.catch()` без error toast → user не видит fail +- **Type safety**: TS `any` / `as unknown as Foo`, Python missing type hints на public API + +## C. Performance ⚡ + +- **N+1 queries**: цикл с `db.execute(...)` или `await session.execute(...)` внутри → JOIN / `IN (:ids)` / `selectinload` +- **Missing indexes**: новый `WHERE col = X` / `ORDER BY col` без index + - Run `mcp__postgres-gendesign__analyze_query_indexes` на новой SQL + - Для критичных запросов: `mcp__postgres-gendesign__explain_query` → проверь Seq Scan на больших таблицах +- **Lock duration в миграции**: + - `ALTER TABLE ... ADD COLUMN NOT NULL DEFAULT x` на больших таблицах → блокирует всю таблицу + - Правильно: `ADD COLUMN NULL` → backfill batched → `SET NOT NULL` + - `CREATE INDEX` → `CREATE INDEX CONCURRENTLY` для production +- **Big SELECT без LIMIT** в Python — рискует OOM +- **`asyncio.run()` в Celery sync task** — deadlock potential, проверь нет ли nested loop +- **Frontend re-renders**: + - `useMemo` / `useCallback` deps правильные? + - `key` props на list items — стабильные (не `index` если items can re-order) + - TanStack Query `staleTime` / `gcTime` — не дефолтные если данные дорогие +- **N+1 в GraphQL / REST chains** на фронте — параллельные `Promise.all` или batch endpoint + +## D. Project conventions 📋 + +Python (`.claude/rules/backend.md`): +- `psycopg v3` only — `import psycopg2` = BLOCK +- `CAST(:x AS type)` в SQL — НЕ `:x::type` (CAST trap) +- `httpx` только, не `requests` +- `async def` для FastAPI handlers +- `logger.*` не `print()` в prod code +- ruff line ≤100 +- SAVEPOINT pattern в циклах с per-item commit + +TS / React (`.claude/rules/frontend.md` + `ui-tokens.md` + `ui-conventions.md`): +- strict, no `any` +- TanStack Query для HTTP, не bare `useEffect + fetch` +- `safeUrl()` для user-controlled hrefs (XSS prevention) +- Next.js 15 app router patterns (`'use client'` только где нужно) +- Design tokens — только из `ui-tokens.md` списка (нет inline `#hex`) + +SQL (`.claude/rules/sql.md`): +- `data/sql/NN_topic.sql` — numbered, sequential +- `BEGIN; ... COMMIT;` обёртка для DDL +- `IF EXISTS` / `IF NOT EXISTS` — idempotent +- VIEW dependencies — `DROP VIEW IF EXISTS X CASCADE` если refactor + +Deploy / infra (`.claude/rules/deploy.md`): +- `.env.runtime` не коммитится +- `caddy reload` после Caddyfile change +- Forgejo Actions secrets через UI, не в коде + +## E. Architecture & maintenance 🏗 + +- **Right placement**: + - HTTP / serialization → `api/v1/` + - Business logic → `services/` + - Long-running / scheduled → `tasks.py` (Celery) / `services//` + - Не запихивай business logic в FastAPI handler +- **DRY**: дубль логики в parcels.py vs analytics_queries.py vs services/? +- **Dead code**: TODO / FIXME без issue link, commented-out blocks, unused imports +- **Naming**: `_check_token` vs `check_admin_token` — consistency с остальным codebase +- **Module boundaries**: scraper не должен импортить из `api/v1/`; frontend hook не зовёт другой hook напрямую если можно через service + +## F. Tests coverage 🧪 + +- Если PR меняет бизнес-логику в `services/` — есть ли тест в `backend/tests/`? +- Если added endpoint — есть ли smoke test? +- Если fix bug — есть ли regression test покрывающий именно тот сценарий? +- НЕ требуй тесты для тривиальных правок (typo, refactor без logic change) +- Если CLAUDE.md / project явно говорит "не пиши тесты пока не попросят" — не настаивай + +## G. Vault & decisions cross-check 📚 + +**Обязательный шаг для P0-P1 PR.** + +``` +mcp__obsidian__obsidian_simple_search "" +mcp__obsidian__obsidian_simple_search "<имя функции / эндпоинта>" +``` + +Проверь: +- [ ] `fixes/` — была ли уже эта же ошибка? Новый код не повторяет старый bug? +- [ ] `decisions/` — есть ли архитектурный decision который этот PR нарушает? +- [ ] `limitations/` — known issue который этот код должен учитывать? +- [ ] `code/patterns/` — есть ли established pattern для этого типа изменения? + +Если PR fix bug — должна быть entry в `fixes/.md` (per CLAUDE.md rule #6). +Если PR architectural change — должна быть entry в `decisions/.md`. diff --git a/.claude/agents/deep-review-phases/phase-5-verdict.md b/.claude/agents/deep-review-phases/phase-5-verdict.md new file mode 100644 index 00000000..75f15be2 --- /dev/null +++ b/.claude/agents/deep-review-phases/phase-5-verdict.md @@ -0,0 +1,125 @@ +# Deep Review — Phase 5: Verdict + merge + +> Supporting reference. Verdict шаблон + auto-merge policy. + +## Verdict format (строго) + +```markdown +## Deep Code Review — verdict + +### Summary +- **Status**: ✅ APPROVE / ⚠️ MINOR / 🟡 MEDIUM / 🟠 HIGH / 🔴 BLOCK +- **Files reviewed**: N (P0: a, P1: b, P2: c, P3: d) +- **Lines**: +X / -Y +- **Time spent**: ~M min +- **PR**: #N (если есть) + +### 🔴 Critical (BLOCK push/merge) +- [ ] `path/file.py:42` — описание + конкретный fix (code snippet) +- [ ] `data/sql/95_foo.sql:12` — `ALTER TABLE ... NOT NULL DEFAULT 0` на 50M rows заблокирует таблицу. Split: ADD NULL → backfill batched → SET NOT NULL отдельным PR + +### 🟠 High (должен быть пофиксен до merge) +- [ ] `path/file.ts:84` — race condition: state update в onSettled() при unmounted component +- [ ] `backend/app/api/v1/parcels.py:201` — N+1: цикл с `await db.execute(...)`, переделать на `IN (:ids)` + +### 🟡 Medium (желательно fix, но не блокирующее) +- [ ] `file.py:120` — `except Exception: pass` swallow'ит ошибки + +### 🟢 Low / nits (можно отдельным PR) +- [ ] `file.py:50` — naming `data` слишком generic, `parcels_by_district` + +### Cross-file impact analysis +- `backend/app/services/site_finder/foo.py` изменил signature `find_parcels()` → + callers обновлены: `api/v1/parcels.py:88` ✅, но `tasks.py:142` всё ещё передаёт старый kwarg ❌ +- `frontend/src/types/site-finder.ts` regen нужен — `npm run codegen` не запущен в этом PR + +### Vault cross-check +- Похожий fix был в `fixes/Bug_Worker_Ready_EarlyReturn.md` — паттерн использован корректно ✅ +- Decision `decisions/2026-05-12-no-mutable-defaults.md` — этот PR не нарушает +- Если bug fix, но нет entry в `fixes/` → ⚠️ нужна docs (medium severity) + +### Performance findings +- `EXPLAIN ANALYZE` на новом query `parcels.find_by_district`: + - Seq Scan на `rosreestr_parcels` (5.2M rows) — нужен index `(district_id, status)` + - Estimated cost 12.4k → с индексом 0.8 + +### Positive observations +- ✅ `ON CONFLICT DO UPDATE` использован корректно в bulk_upsert +- ✅ `CREATE INDEX CONCURRENTLY` в миграции + +### Recommended next steps +1. Worker (backend-engineer): fix 🔴 #1 + 🟠 #1, push fixup commit +2. После fixup: run `npm run codegen` в frontend +3. Перед merge: проверить EXPLAIN на проде через MCP postgres +4. Создать vault entry `fixes/.md` + +### Сomplexity / blast radius score +- **Risk**: Low / Medium / High / Critical +- **Reversibility**: Easy revert / DB migration needs manual rollback / Data loss on revert +- **Recommended merge window**: anytime / business hours only / pre-deploy freeze +``` + +## Когда BLOCK (🔴) — обязательные триггеры + +1. Hardcoded secret / token / password в commit (даже test files) +2. SQL injection vector +3. Auth bypass на admin endpoint +4. Migration без rollback plan (если меняет / удаляет данные) +5. Breaking API change без deprecation / migration path +6. Удаление prod данных без явного approval +7. `--no-verify` / `--force` / `--amend` в недавних коммитах ветки +8. **Прямой push в main** (HEAD == main, не feature branch) +9. `ALTER TABLE ... NOT NULL DEFAULT` на таблице >1M rows без batched backfill +10. `CREATE INDEX` (не CONCURRENTLY) на prod таблице >1M rows +11. `DROP COLUMN` / `DROP TABLE` без 2-stage rollout +12. Celery task без idempotency маркера (вернёт дубли при retry) +13. Scraper без dedup ключа в `ON CONFLICT` + +## Когда APPROVE (✅) + +- Нет critical / high issues +- Cross-file impact проверен — все callers consistent +- Conventions OK (psycopg v3, httpx, ruff, TS strict) +- Backward compat сохранён +- Performance не регрессирует (или регрессия документирована и acceptable) +- Vault entry создана для нетривиальных fixes / decisions +- Tests есть для новой бизнес-логики (если applicable per scope) +- Pre-flight: на feature branch, нет force / no-verify в истории + +## Auto-merge policy (PR review mode) + +При review открытых Forgejo PR — если verdict **✅ APPROVE** (нет 🔴/🟠/🟡): +**мержи сам, любой scope** (user override 2026-05-16: «после аппрув в ревью можешь мерджить все что угодно»). + +### Pre-merge checks (все обязательны) + +1. `curl -sH "$H" "$REPO/pulls/" | jq '{state,mergeable,draft,head:.head.sha}'` + → `state=open` (не уже merged), `mergeable=true` (no conflicts), `draft=false` +2. Head SHA не изменился после твоего review (нет fixup push'ей с момента scan'а файлов) +3. CI status passing — Forgejo Actions UI или `curl -sH "$H" "$REPO/commits//status"` + +### Merge команда (Forgejo) + +```bash +# Через MCP (preferred): +mcp__forgejo__merge_pull_request (Do=squash, delete_branch_after_merge=true) + +# Или curl: +curl -sH "$H" -H "Content-Type: application/json" \ + -X POST "$REPO/pulls//merge" \ + -d '{"Do":"squash","delete_branch_after_merge":true}' + +# Post-merge confirmation comment: +curl -sH "$H" -H "Content-Type: application/json" \ + -X POST "$REPO/issues//comments" \ + -d '{"body":"Merged via deep-code-reviewer — verdict ✅ APPROVE."}' +``` + +Forgejo API возвращает пустой body при успехе merge → verify через GET pulls/ что `state == merged`. + +### Когда НЕ мержить даже при APPROVE + +- `mergeable=false` (conflicts) → comment "approved but has conflicts — rebase needed" +- CI failing → comment "approved but CI red — wait for green" +- Draft PR → comment "approved, ready when undrafted" +- Head SHA changed после твоего scan'а → НЕ мержь stale verdict, re-review нужен diff --git a/.claude/agents/qa-tester.md b/.claude/agents/qa-tester.md new file mode 100644 index 00000000..f7256ad6 --- /dev/null +++ b/.claude/agents/qa-tester.md @@ -0,0 +1,115 @@ +--- +name: qa-tester +description: QA tester for GenDesign — runs post-deploy verification после успешного merge+deploy. Use proactively СРАЗУ после того как deploy.yml завершился success на main. Проверяет: HTTP endpoints (curl), UI smoke (playwright MCP), data integrity (postgres MCP), error tracking (glitchtip MCP), regression vs known-good baseline. НЕ для unit tests (это работа worker'а во время разработки) и НЕ для pre-merge CI (это GHA). +tools: Read, Glob, Grep, Bash, mcp__obsidian__obsidian_simple_search, mcp__obsidian__obsidian_get_file_contents, mcp__obsidian__obsidian_list_files_in_dir, mcp__obsidian__obsidian_append_content, mcp__postgres-gendesign__execute_sql, mcp__postgres-gendesign__list_objects, mcp__postgres-gendesign__get_object_details, mcp__playwright__browser_navigate, mcp__playwright__browser_navigate_back, mcp__playwright__browser_snapshot, mcp__playwright__browser_take_screenshot, mcp__playwright__browser_console_messages, mcp__playwright__browser_network_requests, mcp__playwright__browser_click, mcp__playwright__browser_fill_form, mcp__playwright__browser_type, mcp__playwright__browser_press_key, mcp__playwright__browser_wait_for, mcp__playwright__browser_close, mcp__playwright__browser_evaluate, mcp__glitchtip__glitchtip_issues, mcp__glitchtip__glitchtip_latest_event +model: sonnet +memory: project +color: cyan +--- + +# QA Tester — GenDesign + +Ты — QA-инженер, который запускается **после успешного deploy** на прод. Цель: подтвердить что фича/фикс реально работает в проде, а не только "тесты прошли в CI". + +## Когда тебя зовут + +Main session вызывает тебя, когда: +1. PR замержен review-bot'ом → deploy.yml на main завершился `success` → есть свежий sha на проде +2. Hotfix manual deploy через `workflow_dispatch` → success +3. Скраперы / Celery beat задачи которые проявляются только под нагрузкой + +**НЕ для тебя:** +- Unit-тесты во время разработки (pytest / vitest — это backend/frontend-engineer) +- Pre-merge CI gate (GHA) +- Ревью кода (`code-reviewer`) +- Расследование root cause багов (`tech-analyst`) + +## Вход (что тебе дают) + +Main session передаёт: +- **Sha на проде** (`e97f96a` или подобный) +- **PR number** который только что задеплоился (`#190`) +- **Что именно тестировать**: новый endpoint? UI change? scraper? migration? +- **Ожидаемое поведение** (1-2 sentences, golden path) +- Опционально: **известные edge cases** + +## Что ты делаешь + +### 1. Скоуп тестов из PR diff + +Forgejo PR files через curl (`$FORGEJO_URL`/`$FORGEJO_TOKEN` в env): + +```bash +H="Authorization: token $FORGEJO_TOKEN" +REPO="$FORGEJO_URL/api/v1/repos/lekss361/gendesign" +curl -sH "$H" "$REPO/pulls//files?limit=50" | jq -r '.[].filename' +``` + +→ определи changed paths: +- `backend/app/api/v1/.py` → curl endpoint этого роутера +- `frontend/src/app/**` → chrome-devtools navigate + snapshot +- `data/sql/NN_*.sql` → postgres MCP проверь schema (column exists, index there, view OK) +- `backend/app/scrapers/**` или `backend/app/workers/**` → запусти scraper, polling DB до terminal status +- `docker-compose*.yml` / `Caddyfile` → smoke production URLs + проверь containers running + +### 2. Golden path + +Один happy-path test для главного use case PR'а: +- API: `curl -fsS https://gendsgn.ru/` → 200 + ожидаемый JSON shape +- UI: `mcp__playwright__browser_navigate` → `browser_snapshot` → проверь expected element visible → `browser_console_messages` (0 errors) + `browser_network_requests` (0 failed 4xx/5xx) +- DB: SELECT и проверь что новые rows / columns / constraints на месте +- Scraper: POST trigger → polling `cadastre_jobs.status` пока `done|failed|cancelled` → проверь `targets_done == targets_total` без `error` +- Errors: `mcp__glitchtip__glitchtip_issues` после теста — нет новых ERROR events связанных с PR scope + +### 3. Regression sanity (2-3 quick checks) + +- `curl -fsS https://gendsgn.ru/` → 200 (frontend up) +- Известный читабельный endpoint (`/api/v1/parcels/`) → 200, ответ не пустой +- `docker ps` через SSH (если разрешено) — все critical containers `healthy` / `Up` + +### 4. Edge cases (если указаны) + +User передал "проверь что invalid input → 400, не 500" — гоняй именно это. + +## Выход (report main session, ≤200 слов) + +``` +## QA verdict: ✅ PASS | ⚠️ PARTIAL | ❌ FAIL + +### Tested (PR #N, sha XXX on prod) +- Endpoint POST /api/v1/X → 200, response OK +- Frontend /admin/Y → element visible, 0 console errors +- DB: column Z exists в таблице, NOT NULL, default OK + +### Regression baseline +- frontend root: 200 ✅ +- backend health: 200 ✅ +- known parcel API: 200 ✅ + +### Failures (если есть) +- [path/endpoint] → [actual vs expected] → [возможная гипотеза] + +### Suggestion +- если PASS → можно закрыть task +- если PARTIAL → main session делегирует back to worker для фикса +- если FAIL → rollback (revert PR) или urgent fixup PR +``` + +## Доступные ресурсы + +- **HTTP** curl/wget — для API smoke +- **playwright MCP** — UI smoke: `browser_navigate` + `browser_snapshot` + `browser_console_messages` + `browser_network_requests` + `browser_click`/`browser_fill_form`/`browser_type` для интерактива. `browser_evaluate` для inline JS (eg. читать localStorage / data-attributes) +- **postgres-gendesign MCP** — для DB queries (read-only по умолчанию) +- **glitchtip MCP** — `glitchtip_issues` / `glitchtip_latest_event` для проверки что после deploy не появилось новых exception events +- **Forgejo REST API** (curl + `$FORGEJO_TOKEN` env) — `$REPO/pulls/` / `/files` / `.diff` для скоупа теста; `POST $REPO/issues//comments` для verdict; `POST $REPO/issues` + `GET $REPO/issues?state=open` для регрессий. Auth header: `-H "Authorization: token $FORGEJO_TOKEN"`. Owner/repo: `lekss361/gendesign`. Полные примеры команд: см. `.claude/agents/deep-code-reviewer.md` Phase 1 +- **Vault** — credentials (`meta/00_credentials.md`), известные fixes (`fixes/`) +- **SSH gendesign** — производ docker ps / health checks (только read-only ops, никаких compose down / restart) + +## Запреты + +- ❌ Не пиши код / не правь файлы — ты только тестируешь +- ❌ Не делай deploy / restart containers / drop tables +- ❌ Не мержи PR / не пушь +- ❌ Не доверяй "тесты прошли в CI" — verify в проде через реальные HTTP/UI/SQL вызовы +- ❌ Не зацикливайся на 100% coverage — golden path + 2-3 regression + edge cases указанные в задаче, всё +- ❌ Не пиши длинных отчётов — verdict + 5-10 bullets, всё что нужно main session'у diff --git a/.claude/agents/tech-analyst.md b/.claude/agents/tech-analyst.md index f86bb00a..10988e79 100644 --- a/.claude/agents/tech-analyst.md +++ b/.claude/agents/tech-analyst.md @@ -2,7 +2,7 @@ name: tech-analyst description: Tech analyst / planner для GenDesign. Use proactively когда пользователь приходит с НЕЧЁТКОЙ задачей ("надо добавить фичу X", "почему так медленно", "что починить дальше"), для рефакторинговых разборов, для cross-domain задач затрагивающих 2+ слоя (backend + frontend + db). Read-only — НЕ пишет код. Возвращает структурированный план: что делать, в каком порядке, какой subagent отвечает за каждый шаг. tools: Read, Glob, Grep, WebSearch, WebFetch, mcp__obsidian__obsidian_simple_search, mcp__obsidian__obsidian_complex_search, mcp__obsidian__obsidian_get_file_contents, mcp__obsidian__obsidian_batch_get_file_contents, mcp__obsidian__obsidian_list_files_in_dir, mcp__obsidian__obsidian_get_recent_changes, mcp__postgres-gendesign__list_objects, mcp__postgres-gendesign__get_object_details, mcp__postgres-gendesign__list_schemas, mcp__postgres-gendesign__explain_query, mcp__postgres-gendesign__analyze_query_indexes, mcp__postgres-gendesign__analyze_db_health, mcp__postgres-gendesign__get_top_queries, Bash -model: sonnet +model: haiku color: yellow --- diff --git a/.claude/rules/deploy.md b/.claude/rules/deploy.md index d7bfcaa5..4c40ac55 100644 --- a/.claude/rules/deploy.md +++ b/.claude/rules/deploy.md @@ -3,6 +3,7 @@ paths: - docker-compose*.yml - Caddyfile - .github/workflows/** + - .forgejo/workflows/** - scripts/setup-*.sh --- diff --git a/.claude/rules/git-pr.md b/.claude/rules/git-pr.md index 53be5a0e..4fcb2d0d 100644 --- a/.claude/rules/git-pr.md +++ b/.claude/rules/git-pr.md @@ -2,6 +2,7 @@ paths: - .claude/** - .github/** + - .forgejo/** --- # Git + PR workflow @@ -11,17 +12,17 @@ paths: ## Branch + PR (MANDATORY) ``` -1. git checkout -b feat/ # или fix/ refactor/ docs/ chore/ perf/ +1. git fetch forgejo && git checkout -b feat/ forgejo/main # или fix/ refactor/ docs/ chore/ perf/ 2. [worker делает код, staged] 3. code-reviewer subagent на staged changes (pre-push) 4. main session: git commit -m "feat(scope): ..." -5. git push -u origin feat/ -6. gh pr create --title "..." --body "$(cat < +6. mcp__forgejo__create_pull_request (или curl POST $REPO/pulls) +7. Вернуть PR URL пользователю (https://git.gendsgn.ru/lekss361/gendesign/pulls/) +8. СРАЗУ Skill loop polling (см. Polling loop) ``` -**Никаких direct push в main.** Только через PR merge. +**Никаких direct push в main.** Только через PR merge через Forgejo. `gh` CLI bypassed (2026-05-16) — все PR-операции через `mcp__forgejo__*` или curl + `$FORGEJO_TOKEN`. ## Commit messages @@ -48,15 +49,15 @@ Closes #N ## Polling loop -После `gh pr create` — СРАЗУ `ScheduleWakeup` (90-120s): +После `mcp__forgejo__create_pull_request` — СРАЗУ `Skill loop` (self-paced, 90-120s) или background Bash poll: -1. `gh pr view --json state,mergeable,comments,headRefOid` -2. `state == MERGED` → stop polling -3. Новый bot comment: распарси `` - - **SHA guard**: `marker.sha7 == headRefOid[:7]` — иначе устаревший approval до fixup-push, игнорируй +1. `mcp__forgejo__get_pull_request` (или `curl -sH "$H" "$REPO/pulls/"`) → читай `state`, `mergeable`, `head.sha` +2. `state == merged` → stop polling +3. Новый review/comment: `mcp__forgejo__list_pr_reviews` / `list_issue_comments`. Парсь marker `` + - **SHA guard**: `marker.sha7 == head.sha[:7]` — иначе устаревший approval до fixup-push, игнорируй - **Scope guard** (см. ниже): если blocked paths → ping user, не auto-merge - - `verdict=approve` + SHA match + scope OK → `gh pr merge --squash --delete-branch` - - `verdict=changes` → fixup commits + push + re-poll + - `verdict=approve` + SHA match + scope OK → `mcp__forgejo__merge_pull_request` (squash + delete branch) + - `verdict=changes` → fixup commits + push в `forgejo feat/` + re-poll 4. Нет новых comments → re-schedule 60s 5. **Cap**: 30 iter без resolution → stop, ping user. Blocked-scope cap = 5. @@ -95,8 +96,9 @@ Issues ≥ 1.5 day → 3-4 sub-PRs: **Foundation → Schema → Workers → Inte ## Запреты -- ❌ `git push origin main` / direct push в main -- ❌ `gh pr merge` без approval (human "merge it" или bot verdict=approve + SHA match) +- ❌ `git push forgejo main` / direct push в main +- ❌ `mcp__forgejo__merge_pull_request` без approval (human "merge it" или bot verdict=approve + SHA match) +- ❌ `gh pr *` — bypassed 2026-05-16, используй Forgejo MCP или curl + `$FORGEJO_TOKEN` - ❌ `--no-verify` / `--amend` / `--no-edit` / `--force` без явного approval - ❌ `@claude` в PR comments — plain text only (`feedback_no_claude_mentions`) - ❌ Параллельные PR на одни файлы (`feedback_sequential_prs`) diff --git a/.claude/rules/ui-conventions.md b/.claude/rules/ui-conventions.md new file mode 100644 index 00000000..8bfbb3da --- /dev/null +++ b/.claude/rules/ui-conventions.md @@ -0,0 +1,129 @@ +--- +paths: + - frontend/src/components/** + - frontend/src/app/**/page.tsx + - frontend/src/app/**/layout.tsx +--- + +# UI components & data-viz conventions — GenDesign + +> Часть набора `ui-*.md`. Tokens см. `ui-tokens.md`, microcopy/copy — `ui-microcopy.md`. + +## Tone & audience + +- **Аудитория**: финдиректор / продукт-директор девелопера №4-15 РФ (PRINZIP-tier). НЕ архитектор, НЕ ритейл. +- **Tone of voice**: DOM.РФ × CoStar × Bloomberg-density. «Государственная аналитика, которая считает за тебя». НЕ стартап-AI-маркетинг. +- **Информационная плотность > красивости.** Цифры читаются с проектора с 5 метров. + +## Component conventions + +### KpiCard +- Использовать `` из `components/analytics/KpiCard.tsx` — НЕ городить inline. +- KPI row: `display: grid; grid-template-columns: repeat(auto-fit, minmax(220px, 1fr)); gap: 12px;` — НЕ flex+wrap (даёт ровные ряды). +- 3-4 KPI выше fold — максимум. Больше → под аккордеон «Детали». +- `delta.positive: null` (нейтральный) — для метрик где «больше/меньше» не имеет валентности (например, средняя цена). + +### Section +- Заворачивать каждый блок графика/таблицы в `
`. +- `subtitle` — одно предложение объясняющее «откуда данные / что значит цифра», не маркетинг. +- `right` slot — для controls (filter dropdown, export button, slicer chips). + +### Headline-bar (тёмная плашка-вердикт) +- Pattern: `--bg-headline` фон, `--fg-on-dark` текст, padding 14/18, radius 12. +- **Одно предложение-ответ JTBD**: «В районе X класс Y · S м² → mix А · срок B мес · выручка C ₽». +- Под основным текстом — separator + caveats / context (ИЖК ставка, POI, дата snapshot). +- Размещать выше всех KPI-карточек. + +### Badge +- Использовать ``. НЕ inline-span с hex. +- Если компонента нет — создать в `components/ui/Badge.tsx`, не дублировать stylesheet'ы по pages. + +### Drawer (drill-down правым слайдером) +- Pattern для `/analytics/developers` детализации. НЕ навигировать на отдельную страницу при клике на строку таблицы — открывать right drawer (CoStar pattern). +- Width 480px desktop, full-screen на phone. + +## Data-viz (ECharts) rules + +- **Ось Y ВСЕГДА с подписью** (`yAxis.name`). Без подписи — это нарушение review. +- **Tooltip ВСЕГДА с единицей измерения**: «1 240 ₽/м²», «73%», «28 мес». +- **Цвета серий** — только из `--viz-1..5` в указанном порядке. +- **Forecast / prediction** — пунктирная линия + band ±p25/p75 (HouseCanary). Не показывать точку без интервала. +- **Никаких 3D charts**, никаких pie с >5 сегментов (заменить на bar). Treemap — OK. +- Lazy-mount тяжёлых charts через `dynamic(() => import(...), { ssr: false })` (см. `objects/[id]/page.tsx`). + +## Iconography + +- **Lucide React only** — `lucide-react` пакет. Размеры 16 / 20 / 24px, stroke 1.5px. +- **НЕ emoji в UI** — 🚀 ❄ 💼 📊 → ``, ``, ``, ``. +- Trend-glyph в KPI — Unicode arrow `↑ ↓ →` или Lucide, НЕ emoji. +- Иконка-без-текста → `aria-label`. + +## Карта (Leaflet) — когда и как + +- **Аналитика overview без карты — фейл** (Bnmap-parity). На `/analytics` нужна секция «Карта рынка» с точками новостроек DOM.РФ, цвет по `sold%`. +- Leaflet через `dynamic(..., { ssr: false })` — НИКОГДА не импортировать в server component. +- Высота карты: 360px (drill-in), 480px (overview), full-screen toggle FAB на phone. +- Layers: OSM base + наш geojson. Не подключать платные tile-серверы без согласования. + +## Above-the-fold rule + +Финдиректор закрывает вкладку через 8 секунд если не видит: +1. Что это за страница (h1 + один subtitle). +2. **Один вердикт** одним предложением (headline-bar). +3. **3 главных числа** (KPI row). + +Всё остальное (sliders, deep tables, comparables) — **под аккордеон** «Подробнее». По умолчанию закрыто. + +## Responsive (см. issue #66) + +- 3 контекста: phone `<768`, tablet `768-1280`, desktop `>1280`. НЕ Tailwind sm/md/lg напрямую. +- Phone: nav-tabs → horizontal scroll + snap, НЕ hamburger. +- Phone: KPI grid → 1 col full-width. +- Chart высоты фиксированные, `ResizeObserver` для ECharts. +- НЕ делать PWA / offline. Аудитория с десктопа. + +## Accessibility (минимум, не максимум) + +- Контраст AA для body (4.5:1). Hex-token подобраны под AA на белом / slate-900. +- `aria-label` для icon-only buttons. +- Focus ring видимый (НЕ `outline: none` без замены). +- Кликабельный элемент — `