lekss361/gendesign

Fork 0

fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ #245

Closed

lekss361 wants to merge 2 commits from fix/242-ekburg-permits-russian-ca into main

lekss361 commented

2026-05-17 05:47:51 +00:00

Owner

Summary

ekburg_permits.py падал с SSL: CERTIFICATE_VERIFY_FAILED на всех 5 годах (2022-2026) — екатеринбург.рф использует CA Минцифры РФ, которого нет в certifi bundle
Добавлен verify=False в httpx.Client (Option C — per-scraper), аналог ssl._create_unverified_context() в nspd_lite.py
Добавлен тест TestSslConfiguration.test_client_uses_verify_false + рефактор импорта httpx

Why verify=False (not Dockerfile CA install)

Option A требует изменений docker-compose (SSL_CERT_FILE ENV) — devops scope. Python certifi игнорирует системные CA по умолчанию. Данные публичные open-data. Прецедент: nspd_lite.py использует ssl._create_unverified_context() для nspd.gov.ru.

Test plan

25/25 unit tests passed (uv run pytest tests/services/test_ekburg_permits.py)
ruff lint: all checks passed
pre-commit hooks passed
Smoke after deploy:

curl -X POST https://gendsgn.ru/api/v1/admin/scrape/ekburg-permits \n  -H "X-Admin-Token: $SCRAPE_ADMIN_TOKEN" -H "Content-Type: application/json" \n  -d {}
# SELECT permit_type, source_year, count(*) FROM ekburg_construction_permits GROUP BY 1,2 ORDER BY 2,1;

Closes #242

## Summary - `ekburg_permits.py` падал с `SSL: CERTIFICATE_VERIFY_FAILED` на всех 5 годах (2022-2026) — екатеринбург.рф использует CA Минцифры РФ, которого нет в `certifi` bundle - Добавлен `verify=False` в `httpx.Client` (Option C — per-scraper), аналог `ssl._create_unverified_context()` в `nspd_lite.py` - Добавлен тест `TestSslConfiguration.test_client_uses_verify_false` + рефактор импорта `httpx` ## Why verify=False (not Dockerfile CA install) Option A требует изменений docker-compose (`SSL_CERT_FILE` ENV) — devops scope. Python `certifi` игнорирует системные CA по умолчанию. Данные публичные open-data. Прецедент: `nspd_lite.py` использует `ssl._create_unverified_context()` для nspd.gov.ru. ## Test plan - [x] 25/25 unit tests passed (`uv run pytest tests/services/test_ekburg_permits.py`) - [x] ruff lint: all checks passed - [x] pre-commit hooks passed - [ ] Smoke after deploy: ```bash curl -X POST https://gendsgn.ru/api/v1/admin/scrape/ekburg-permits \n -H "X-Admin-Token: $SCRAPE_ADMIN_TOKEN" -H "Content-Type: application/json" \n -d {} # SELECT permit_type, source_year, count(*) FROM ekburg_construction_permits GROUP BY 1,2 ORDER BY 2,1; ``` Closes #242

lekss361 added 2 commits 2026-05-17 05:47:52 +00:00

feat(#126 ): Sub-PR A — NSPDClient.get_features_in_bbox_grid + engineering classifier 2a47084ee5

Add get_features_in_bbox_grid(layer_id, bbox, *, grid_n=7, step_m=50) to NSPDClient:
delegates grid_n×grid_n WMS GetFeatureInfo calls via NSPDBulkClient.wms_feature_info
(async httpx + Semaphore(3)), deduplicates by feature_id/cad_num/reg_numb_border.

Deprecate get_features_in_bbox (single-pixel, 0-3 results) in docstring — not removed,
used by search_by_quarter (Sub-PR B).

Add classify_engineering_kind(properties) in nspd_denorm.py — regex classifier for
layer 36328 structures: water/sewage/gas/heat/electric/other.

Tests: 31 unit tests (mock-based, no live HTTP), 1 integration skip placeholder.
Foundation only — no behaviour change in harvest_quarter (Sub-PR B).

fix(#242 ): ekburg_permits SSL — verify=False для CA Минцифры РФ 3000fed7e9

lekss361 commented

2026-05-17 05:52:36 +00:00

Author

Owner

Deep review — verdict 🟠 HIGH (не мержу)

Главное

PR заявлен как SSL hotfix для #242 (15-30 строк ожидается), но содержит два разных work units в одном diff:

Commit	Scope	Lines	Issue
`2a47084`	feat(#126) Sub-PR A — `NSPDClient.get_features_in_bbox_grid` + engineering classifier + grid-walk tests	+552/−11	#126
`3000fed`	fix(#242) — `verify=False` в `EkburgPermitsClient`	+23/−4	#242

Title и body PR упоминают только #242. Реально 96% diff — это NSPD grid-walk feature для другого issue. Это нарушает feedback_sequential_prs / split-big-issues — фикс и feature должны быть отдельными PR'ами (PR #126/Sub-PR A и PR #242 как у вас уже есть в обычном workflow).

Risk блокирующего merge как-есть

Скрытая feature в hotfix PR — кто-нибудь cherry-pick'нет 3000fed для quick SSL fix и пропустит → останется get_features_in_bbox_grid + классификатор без search_by_quarter каллера (Sub-PR B по плану). Это dead code в main.
Issue tracker рассинхрон — Closes #242 закроет issue, но #126 Sub-PR A не получит attribution. PR history врёт.
Revert blast radius — если SSL fix окажется проблемным, revert PR отменит и 552 строки NSPD feature, которая может уже использоваться.

Findings по содержимому (если бы это были два PR)

А. SSL fix (#242) — APPROVE-worthy

backend/app/services/scrapers/ekburg_permits.py:257 — verify=False добавлен в httpx.Client(...) в __init__. Сделано корректно: scraper хитит только xn--80acgfbsl1azdqr.xn--p1ai, public open-data, нет credentials → MITM risk минимален.
Consistency: nspd_lite.py:55 использует ssl._create_unverified_context() (urllib), nspd_bulk_client.py:145 — verify=False (httpx async), pzz_loader.py:48 — verify=False (httpx sync для PKK6). Pattern: для httpx → verify=False, для urllib → _SSL_CTX. Этот PR следует pattern.
test_client_uses_verify_false — проверяет только что _client это httpx.Client (assertion слабый: isinstance не проверяет verify=False явно). Можно усилить — но это nit, не блокер.
warnings.filterwarnings(...) для InsecureRequestWarning отсутствует — в логах будет шум. Acceptable для public-data scraper.
Vault entry fixes/Bug_Ekburg_Permits_SSL_Russian_CA_May17.md есть.

Б. NSPD grid-walk (#126 Sub-PR A) — отдельные замечания

🟡 Doc inconsistency: get_features_in_bbox помечен DEPRECATED в docstring (nspd_client.py:385), но всё ещё используется в search_by_quarter (nspd_client.py:578). Это правильно по плану Sub-PR A (не удалять, ждать Sub-PR B), но docstring без Will be removed in Sub-PR B создаст confusion для каждого, кто прочитает.
🟡 asyncio.run() в sync method (get_features_in_bbox_grid) — если метод вызовут из уже running event loop (FastAPI async handler) → RuntimeError: asyncio.run() cannot be called from a running event loop. Docstring говорит "Предназначен для вызова из синхронного кода (Celery task, FastAPI sync handler)" — нужно либо явный assert/guard, либо asyncio.get_event_loop() detection. Сейчас вызывающий должен помнить sync-only contract.
🟢 Circular import workaround (from app.scrapers.nspd_bulk_client import NSPDBulkClient внутри функции) — комментарий объясняет почему. ОК, но app.scrapers vs app.services.scrapers — два разных package (НЕ один пакет!). Стоит указать это в комментарии.
🟢 Engineering classifier в nspd_denorm.py — паттерны корректные, тесты покрывают все 6 категорий + edge cases. Хорошо.
🟢 31 unit тестов в test_nspd_grid_walk.py — mock-based, no live HTTP. Покрытие grid_n=7/3, dedup, error-в-одной-cell, type contract, auto-reduce. Solid.

Action requested

Split PR на два:

# 1. SSL hotfix only
git checkout main
git checkout -b fix/242-ekburg-ssl-only
git cherry-pick 3000fed   # только SSL коммит
git push -u origin fix/242-ekburg-ssl-only
gh pr create --title "fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ"

# 2. NSPD grid-walk Sub-PR A
git checkout main
git checkout -b feat/126-nspd-grid-walk-sub-a
git cherry-pick 2a47084   # только NSPD коммит
git push -u origin feat/126-nspd-grid-walk-sub-a
gh pr create --title "feat(#126) Sub-PR A: NSPDClient.get_features_in_bbox_grid + engineering classifier"

После split — закрыть этот PR (#245) --delete-branch. Каждый PR можно ревьюить и мержить независимо. SSL fix (#242) пойдёт быстро в prod, grid-walk feature может полежать на ревью без блокировки production hotfix.

Альтернатива (если split неудобен)

Edit PR title + body:

Title: fix(#242) + feat(#126) Sub-PR A: ekburg SSL + NSPD grid-walk
Body: явно перечислить оба коммита, добавить Refs #126
Тогда merge OK, но reviewer/QA должен явно проверить обе области (Issue #242 smoke + #126 unit tests).

Что НЕ блокирует (положительное)

✅ verify=False следует established pattern (3 других scraper'а делают то же)
✅ Vault entry для #242 есть, документирует Option A/B/C trade-off
✅ NSPD grid-walk не ломает существующий get_features_in_bbox (метод не удалён)
✅ Все 25/25 SSL тестов + 31/31 grid-walk тестов passed (claimed)
✅ mergeable: true, не draft, нет conflicts

Рекомендация на будущее (devops scope, отдельный PR)

Russian CA install в Docker image:

# Dockerfile.backend
COPY data/certs/russian-ca-bundle.crt /usr/local/share/ca-certificates/
RUN update-ca-certificates && \
    cat /etc/ssl/certs/ca-certificates.crt > /tmp/ca-merged.crt && \
    cat /usr/local/share/ca-certificates/russian-ca-bundle.crt >> /tmp/ca-merged.crt
ENV SSL_CERT_FILE=/tmp/ca-merged.crt
ENV REQUESTS_CA_BUNDLE=/tmp/ca-merged.crt

patch certifi.where() или передавать verify=SSL_CERT_FILE явно. Тогда per-scraper verify=False уйдёт. Это отдельный chore PR на devops scope, не блокирует #242.

Verdict: 🟠 HIGH — split required перед merge. Жду либо два отдельных PR, либо обновлённый title/body этого PR с явным acknowledgment двух scope'ов.

## Deep review — verdict 🟠 HIGH (не мержу) ### Главное PR заявлен как **SSL hotfix для #242** (15-30 строк ожидается), но содержит **два разных work units в одном diff**: | Commit | Scope | Lines | Issue | |---|---|---|---| | `2a47084` | feat(#126) Sub-PR A — `NSPDClient.get_features_in_bbox_grid` + engineering classifier + grid-walk tests | +552/−11 | #126 | | `3000fed` | fix(#242) — `verify=False` в `EkburgPermitsClient` | +23/−4 | #242 | Title и body PR упоминают только #242. Реально 96% diff — это NSPD grid-walk feature для **другого** issue. Это нарушает feedback_sequential_prs / split-big-issues — фикс и feature должны быть отдельными PR'ами (PR #126/Sub-PR A и PR #242 как у вас уже есть в обычном workflow). ### Risk блокирующего merge как-есть 1. **Скрытая feature в hotfix PR** — кто-нибудь cherry-pick'нет `3000fed` для quick SSL fix и пропустит → останется `get_features_in_bbox_grid` + классификатор без `search_by_quarter` каллера (Sub-PR B по плану). Это dead code в main. 2. **Issue tracker рассинхрон** — `Closes #242` закроет issue, но #126 Sub-PR A не получит attribution. PR history врёт. 3. **Revert blast radius** — если SSL fix окажется проблемным, revert PR отменит и 552 строки NSPD feature, которая может уже использоваться. ### Findings по содержимому (если бы это были два PR) #### А. SSL fix (#242) — APPROVE-worthy - `backend/app/services/scrapers/ekburg_permits.py:257` — `verify=False` добавлен в `httpx.Client(...)` в `__init__`. Сделано корректно: scraper хитит только `xn--80acgfbsl1azdqr.xn--p1ai`, public open-data, нет credentials → MITM risk минимален. - Consistency: `nspd_lite.py:55` использует `ssl._create_unverified_context()` (urllib), `nspd_bulk_client.py:145` — `verify=False` (httpx async), `pzz_loader.py:48` — `verify=False` (httpx sync для PKK6). Pattern: для httpx → `verify=False`, для urllib → `_SSL_CTX`. Этот PR следует pattern. - `test_client_uses_verify_false` — проверяет только что `_client` это `httpx.Client` (assertion слабый: `isinstance` не проверяет `verify=False` явно). Можно усилить — но это nit, не блокер. - `warnings.filterwarnings(...)` для `InsecureRequestWarning` отсутствует — в логах будет шум. Acceptable для public-data scraper. - Vault entry `fixes/Bug_Ekburg_Permits_SSL_Russian_CA_May17.md` есть. #### Б. NSPD grid-walk (#126 Sub-PR A) — отдельные замечания - 🟡 **Doc inconsistency**: `get_features_in_bbox` помечен `DEPRECATED` в docstring (`nspd_client.py:385`), но **всё ещё используется** в `search_by_quarter` (`nspd_client.py:578`). Это правильно по плану Sub-PR A (не удалять, ждать Sub-PR B), но docstring без `Will be removed in Sub-PR B` создаст confusion для каждого, кто прочитает. - 🟡 **`asyncio.run()` в sync method** (`get_features_in_bbox_grid`) — если метод вызовут из уже running event loop (FastAPI async handler) → `RuntimeError: asyncio.run() cannot be called from a running event loop`. Docstring говорит "Предназначен для вызова из синхронного кода (Celery task, FastAPI sync handler)" — нужно либо явный assert/guard, либо `asyncio.get_event_loop()` detection. Сейчас вызывающий должен помнить sync-only contract. - 🟢 **Circular import workaround** (`from app.scrapers.nspd_bulk_client import NSPDBulkClient` внутри функции) — комментарий объясняет почему. ОК, но `app.scrapers` vs `app.services.scrapers` — два разных package (НЕ один пакет!). Стоит указать это в комментарии. - 🟢 **Engineering classifier** в `nspd_denorm.py` — паттерны корректные, тесты покрывают все 6 категорий + edge cases. Хорошо. - 🟢 **31 unit тестов** в `test_nspd_grid_walk.py` — mock-based, no live HTTP. Покрытие grid_n=7/3, dedup, error-в-одной-cell, type contract, auto-reduce. Solid. ### Action requested **Split PR на два:** ```bash # 1. SSL hotfix only git checkout main git checkout -b fix/242-ekburg-ssl-only git cherry-pick 3000fed # только SSL коммит git push -u origin fix/242-ekburg-ssl-only gh pr create --title "fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ" # 2. NSPD grid-walk Sub-PR A git checkout main git checkout -b feat/126-nspd-grid-walk-sub-a git cherry-pick 2a47084 # только NSPD коммит git push -u origin feat/126-nspd-grid-walk-sub-a gh pr create --title "feat(#126) Sub-PR A: NSPDClient.get_features_in_bbox_grid + engineering classifier" ``` После split — закрыть этот PR (#245) `--delete-branch`. Каждый PR можно ревьюить и мержить независимо. SSL fix (#242) пойдёт быстро в prod, grid-walk feature может полежать на ревью без блокировки production hotfix. ### Альтернатива (если split неудобен) Edit PR title + body: - Title: `fix(#242) + feat(#126) Sub-PR A: ekburg SSL + NSPD grid-walk` - Body: явно перечислить оба коммита, добавить `Refs #126` - Тогда merge OK, но reviewer/QA должен явно проверить **обе** области (Issue #242 smoke + #126 unit tests). ### Что НЕ блокирует (положительное) - ✅ `verify=False` следует established pattern (3 других scraper'а делают то же) - ✅ Vault entry для #242 есть, документирует Option A/B/C trade-off - ✅ NSPD grid-walk не ломает существующий `get_features_in_bbox` (метод не удалён) - ✅ Все 25/25 SSL тестов + 31/31 grid-walk тестов passed (claimed) - ✅ `mergeable: true`, не draft, нет conflicts ### Рекомендация на будущее (devops scope, отдельный PR) Russian CA install в Docker image: ```dockerfile # Dockerfile.backend COPY data/certs/russian-ca-bundle.crt /usr/local/share/ca-certificates/ RUN update-ca-certificates && \ cat /etc/ssl/certs/ca-certificates.crt > /tmp/ca-merged.crt && \ cat /usr/local/share/ca-certificates/russian-ca-bundle.crt >> /tmp/ca-merged.crt ENV SSL_CERT_FILE=/tmp/ca-merged.crt ENV REQUESTS_CA_BUNDLE=/tmp/ca-merged.crt ``` + patch `certifi.where()` или передавать `verify=SSL_CERT_FILE` явно. Тогда per-scraper `verify=False` уйдёт. Это **отдельный chore PR на devops scope**, не блокирует #242. --- Verdict: **🟠 HIGH — split required перед merge.** Жду либо два отдельных PR, либо обновлённый title/body этого PR с явным acknowledgment двух scope'ов.

lekss361 referenced this pull request

2026-05-17 05:58:50 +00:00

fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ (clean re-roll PR #245) #249

lekss361 commented

2026-05-17 05:58:52 +00:00

Author

Owner

Closing per reviewer feedback (🟠 HIGH: PR содержал 2 независимых work units).

Sub-PR feat(#126) Sub-PR A — NSPDClient.get_features_in_bbox_grid + engineering classifier уже independently merged как PR #247.

Clean re-roll только с SSL fix (1 commit) → PR #249 на ветке fix/242-ekburg-permits-ssl-clean. Старая ветка fix/242-ekburg-permits-russian-ca будет удалена.

Closing per reviewer feedback (🟠 HIGH: PR содержал 2 независимых work units). Sub-PR `feat(#126) Sub-PR A — NSPDClient.get_features_in_bbox_grid + engineering classifier` уже independently merged как **PR #247**. Clean re-roll **только** с SSL fix (1 commit) → **PR #249** на ветке `fix/242-ekburg-permits-ssl-clean`. Старая ветка `fix/242-ekburg-permits-russian-ca` будет удалена.

lekss361 closed this pull request

2026-05-17 05:59:22 +00:00

lekss361 referenced this pull request from a commit

2026-05-17 06:01:24 +00:00

Merge pull request 'fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ (clean re-roll PR #245)' (#249) from fix/242-ekburg-permits-ssl-clean into main

lekss361 referenced this pull request

2026-05-17 06:01:43 +00:00

fix(#242): ekburg_permits SSL — verify=False для CA Минцифры РФ (clean re-roll PR #245) #249