reviewer(ET): auto-commit from reviewer run_id=67

docs/work-items/ET-011/12-review.md

@@ -0,0 +1,250 @@
+---
+type: review
+work_item_id: ET-011
+verdict: REQUEST_CHANGES
+version: 1
+---
+
+# Review ET-011 — GPX-download из popup публичного трека
+
+**Branch:** `feature/ET-011-popup-enduro-trails`
+**HEAD:** `7d8407a` (fix(ci): hoist imports to satisfy E402 + declare runtime/test deps in pyproject)
+**Build commit:** `eea6c84` (feat(gps-tracks): GPX download from public track popup)
+**Reviewer:** agent:reviewer
+**Дата:** 2026-06-03
+
+---
+
+## Сводка
+
+PR полностью реализует backend (`/api/gps-tracks/{track_id}/download`,
+`export.py`) и frontend (кнопка в popup, `_downloadPublicTrack`,
+обработчик ошибок), описанные в ADR-014 и ADR-015. Тесты UT-01..05,
+IT-01..08 написаны и проходят; ничего из старого функционала не
+сломано (89/89 API-тестов, 24/24 JS-тестов).
+
+Главное замечание — **отсутствует артефакт `tests/web/test_track_download.spec.ts`**,
+явно требуемый ADR-014 §4 и test-plan §E2E-01..04. Это оставляет
+без автоматизированного покрытия AC-1 / AC-2 / AC-13 (по
+`coverage_matrix` они покрываются исключительно E2E-тестами).
+
+---
+
+## Что проверено
+
+| Срез | Результат |
+|---|---|
+| `02-trz.md` REQ-F-01..F-07, REQ-NF-01..NF-07 | см. таблицу ниже |
+| `03-acceptance-criteria.md` AC-1..AC-15 | см. таблицу AC ниже |
+| `06-adr/ADR-014` / `ADR-015` | соответствует решениям A2/B1/C1/D1/E1/F/G/H/I/J и A2/B1/C1/D/F/G |
+| Линт (`ruff check`) | новые файлы — clean; pre-existing F401/E741 в `tests/unit/test_base_layer.py` и `tests/api/test_gps_tracks_sources_osm.py` — не относятся к ET-011 |
+| Тесты API (`pytest tests/api`) | **89/89 PASS** |
+| JS-тесты (`node --test tests/web/gps_tracks.test.js`) | **24/24 PASS** |
+| Документация архитектуры | обновлены `docs/architecture/README.md` и `docs/architecture/adr/README.md` (ADR-014/015 в индексе) |
+| Конфиг `config/gps_sources.yaml` | расширен `download_allowed` для всех 4 источников; OSM=true, остальные=false (соответствует ADR-015 §D) |
+
+---
+
+## Findings
+
+### P1 (must-fix)
+
+**P1-01. Отсутствует Playwright-спецификация `tests/web/test_track_download.spec.ts`**
+
+`docs/work-items/ET-011/04-test-plan.yaml` тесты E2E-01..E2E-04 явно
+размещают в файле `tests/web/test_track_download.spec.ts` (≈ 200 строк
+согласно ADR-014 §4 «Конвенция размещения нового кода»). Файл в PR
+отсутствует:
+
+```
+$ ls tests/web/test_track_download.spec.ts → MISSING
+```
+
+`coverage_matrix` test-plan показывает, что эти E2E-кейсы — единственное
+автоматизированное покрытие следующих AC:
+
+| AC | Покрытие по plan'у | Реализовано |
+|---|---|---|
+| AC-1 (кнопка в popup, REQ-F-01) | E2E-01, E2E-02 | **нет** |
+| AC-2 (клик → файл, REQ-F-02/F-03/F-05) | E2E-01 | частично (IT-01 покрывает только HTTP-сторону) |
+| AC-13 (mobile UX, REQ-NF-04) | E2E-02 | **нет** |
+| AC-7 (toast «не найден») | IT-02, **E2E-03** | только серверная часть (IT-02), UI-обработка `_handleDownloadError` без теста |
+
+UI-кейсы в `04b-ui-test-cases.md` — ручные/визуальные, не заменяют
+автоматизацию.
+
+→ Требуется создать `tests/web/test_track_download.spec.ts` минимум
+для E2E-01 (happy-path download) и E2E-02 (mobile viewport visible
+button). E2E-03 (mock 404 → toast) можно оформить через
+`page.route(...)`. E2E-04 опционален (gated `enabled_if`).
+
+Если установка Playwright в CI блокирована — допустимая альтернатива:
+тесты-юниты на JS (Node `--test`) на функции
+`_parseFilenameFromCD`, `_handleDownloadError` и проверка структуры
+HTML, возвращаемой `_renderTrackPopupHtml` (как кнопка добавлена,
+aria-label корректен). Это закроет AC-1 / AC-7 (UI-сторона), но
+AC-13 (mobile bbox) остаётся без покрытия — пометить как ручную
+проверку в `04b` (manual smoke на release).
+
+### P2 (should-fix)
+
+**P2-01. Контракт ответа 403 расходится с ADR-015 §G**
+
+ADR-015 §G документирует формат:
+```json
+{ "detail": "source_forbidden", "external_urls": [...] }
+```
+
+Фактически (`endpoint.py:384`):
+```python
+raise HTTPException(
+    status_code=403,
+    detail={"detail": "source_forbidden", "external_urls": external_urls},
+)
+```
+
+FastAPI оборачивает это в:
+```json
+{ "detail": { "detail": "source_forbidden", "external_urls": [...] } }
+```
+
+`tests/api/test_gps_tracks_download.py::test_it05_source_forbidden_403`
+явно делает `body.get("detail", body)` → `detail.get("external_urls")`,
+т.е. подтверждает удвоенную вложенность. На frontend
+`_handleDownloadError` тоже учитывает обе формы. По факту контракт
+работает, но **расходится с документированным в ADR-015 §G**.
+
+→ Либо обновить ADR-015 §G и AC-11 на реальный формат
+`{"detail":{"detail":"source_forbidden","external_urls":[...]}}`,
+либо использовать `JSONResponse(status_code=403, content={...})`
+напрямую, чтобы получить однослойную форму. Не блокер, но создаёт
+расхождение «doc vs runtime» для будущих интеграторов.
+
+**P2-02. `_handleDownloadError` для 400 не учтён в REQ-F-05**
+
+REQ-F-05.4 описывает обработку 403 / 404 / 5xx. Frontend дополнительно
+обрабатывает 400 («Неподдерживаемый формат файла»), что само по себе
+полезно. Но в UI-флоу 400 невозможен (кнопка всегда дёргает дефолтный
+`?format=gpx`), а в обычной popup-кнопке нет UI для выбора формата.
+→ Косметика; можно оставить как defensive.
+
+### P3 (nice-to-have)
+
+**P3-01.** Логирование через `logging.getLogger("uvicorn.access")`
+сольёт `track_download id=… size_bytes=…` в access-log формате (INFO).
+Для production-фильтрации удобнее отдельный logger
+(`logger = logging.getLogger("enduro_trails.gps_tracks")`); ADR-014 §J
+рекомендует именно access-logger, так что формальных претензий нет.
+
+**P3-02.** В `build_gpx` связь `external_urls[i] ↔ sources[i]` по
+индексу хрупкая. Если списки в БД разной длины (что возможно при
+dedup-merge), label для `<link><text>` будет вырожденно `sources[0]`.
+Документация (ADR-014 §G) этот edge-case не уточняет, тестов нет —
+явно не блокер, но стоит зафиксировать поведение тестом, чтобы при
+будущих рефакторингах не сломать молча.
+
+**P3-03.** `_renderTrackPopupHtml` использует строковую интерполяцию
+для `${name}`, `${user}`, `${url}` (источники). Это пре-existing
+ET-008 поведение, **не введено в ET-011** (новый блок `actionsHtml`
+содержит только числовой `trackId`, валидированный `Number.isFinite`).
+Out-of-scope, но отмечаю для будущей safety-итерации.
+
+---
+
+## REQ ↔ реализация
+
+| REQ | Реализация | Статус |
+|---|---|---|
+| REQ-F-01 (кнопка в popup) | `gps_tracks.js:498–509` — `actionsHtml`, aria-label «Скачать GPX», 32×32 CSS px из `app.css:1311–1338` | ✓ |
+| REQ-F-02 (endpoint, статусы) | `endpoint.py:332–435` — порядок 400 → 404 → 413 → 403 → 200 как в ADR-014 §H | ✓ |
+| REQ-F-03 (GPX 1.1) | `export.py::build_gpx` + UT-01..03 (XSD-валидация в `tests/fixtures/gpx-1.1/gpx.xsd`) | ✓ |
+| REQ-F-04 (имя файла) | `export.py::safe_filename` + UT-04 (10 кейсов) | ✓ |
+| REQ-F-05 (UX клика) | `_downloadPublicTrack`, `_parseFilenameFromCD`, `_handleDownloadError` | ✓ автоматического E2E-теста нет (см. P1-01) |
+| REQ-F-06 (license 403) | `endpoint.py:382–390` + `config.py::load_download_allowed_sources` + IT-05 | ✓ (контракт см. P2-01) |
+| REQ-F-07 (логирование) | `endpoint.py:419–424` через `uvicorn.access` | ✓ |
+| REQ-NF-01 (perf 300 ms p95) | не измерено в авто-тестах (AC-12 = manual perf check); IT-01 проходит за < 1 сек на 10 точек | ✓ (manual) |
+| REQ-NF-02 (cap 200k → 413) | `MAX_POINTS_FOR_DOWNLOAD = 200_000` + IT-04 | ✓ |
+| REQ-NF-03 (XSD валидация) | UT-03 + IT-07 (XSD 30 КБ закоммичен в fixtures) | ✓ |
+| REQ-NF-04 (mobile UX) | CSS 32×32, popup `maxWidth:'300px'`; **AC-13 без авто-проверки** (см. P1-01) | частично |
+| REQ-NF-05 (RFC 5987) | IT-06 проверяет `filename*=UTF-8''` и ASCII-fallback | ✓ |
+| REQ-NF-06 (наблюдаемость) | uvicorn access + logger.info | ✓ |
+| REQ-NF-07 (безопасность) | `Path(..., ge=1)` для int-id; `safe_filename` чистит ФС-символы; CORS не трогается | ✓ |
+
+---
+
+## AC ↔ покрытие
+
+| AC | Авто-тест | Статус |
+|---|---|---|
+| AC-1 | (по plan: E2E-01/02) | **gap — нет E2E** |
+| AC-2 | (по plan: E2E-01) — IT-01 покрывает только HTTP | **gap — нет UI-теста** |
+| AC-3 | IT-01 | ✓ |
+| AC-4 | UT-04 + IT-06 | ✓ |
+| AC-5 | UT-03 + IT-07 | ✓ |
+| AC-6 | manual smoke | вне scope авто |
+| AC-7 | IT-02 (✓), E2E-03 — нет | частично |
+| AC-8 | IT-03 | ✓ |
+| AC-9 | IT-04 | ✓ |
+| AC-10 | UT-01 + `test_ut01_osm_copyright_present` | ✓ |
+| AC-11 | IT-05 + `test_it05_dual_source_with_osm_passes` | ✓ (E2E-04 conditional) |
+| AC-12 | manual | вне scope |
+| AC-13 | (по plan: E2E-02) | **gap** |
+| AC-14 | UI-cases манульно | вне scope авто |
+| AC-15 | IT-08 + регрессионные API-тесты | ✓ (89/89) |
+
+---
+
+## ADR conformance
+
+**ADR-014 (GPX endpoint).**
+- §A решение A2 (fetch+Blob+a.download) — ✓ `_downloadPublicTrack`.
+- §B решение B1 (`xml.etree.ElementTree`) — ✓ `export.py:11`.
+- §C решение C1 (`Response(...)`, in-memory) — ✓ `endpoint.py:426–435`.
+- §D решение D1 (popup остаётся открытым) — ✓ (нет close-call в `_downloadPublicTrack`).
+- §E решение E1 (`export.py` модуль) — ✓.
+- §F sanitization — ✓ полностью (`_sanitize_for_filesystem`, `_truncate_utf8`, `_ascii_fallback`, см. UT-04).
+- §G GPX-структура — ✓ (порядок metadata-children name/desc/author/copyright/link/time, 6 знаков precision, OSM copyright, тест `test_trkpt_coordinate_precision_6_digits`).
+- §H порядок проверок — ✓ (format → SELECT → points_count → license → coords → build).
+- §I регистрация route — ✓ (после `/health`, конфликта префиксов нет).
+- §J logging — ✓.
+
+**ADR-015 (Source redistribution).**
+- §A решение A2 (поле в YAML, runtime-кэш) — ✓ `load_download_allowed_sources`.
+- §B решение B1 (ANY-правило) — ✓ `endpoint.py:383`, тест `test_it05_dual_source_with_osm_passes`.
+- §C решение C1 (default-deny) — ✓ `config.py:141`.
+- §D финальный whitelist — ✓ `config/gps_sources.yaml` (osm=true, остальные=false).
+- §F валидация — ✓ в route-handler, кэш в closure router'а.
+- §G ответ 403 — расходится с реализованным форматом, см. P2-01.
+
+---
+
+## Регрессия
+
+- `pytest tests/api` — **89/89 PASS** (включая `test_gps_tracks_endpoint.py` 20 кейсов для существующих маршрутов).
+- `node --test tests/web/gps_tracks.test.js` — **24/24 PASS**.
+- `tests/web/test_gps_tracks.py` (статика) — PASS (косвенно через pytest).
+
+Существующих маршрутов / структуры popup-полей / sheet-route::downloadGPX —
+ничего не сломано (IT-08 + ручная проверка отсутствия `closeOnClick`-регресса).
+
+---
+
+## Итог
+
+| Категория | Кол-во |
+|---|---|
+| P0 | 0 |
+| P1 | 1 (отсутствует `tests/web/test_track_download.spec.ts`) |
+| P2 | 2 (контракт 403, defensive 400-toast) |
+| P3 | 3 (logger выбор, edge-case link-text, pre-existing XSS-вектор) |
+
+**Verdict: REQUEST_CHANGES** — есть один P1 (отсутствие E2E-спеки,
+требуемой test-plan §E2E-01..04 и ADR-014 §4). Это оставляет AC-1,
+AC-2, AC-13, и UI-часть AC-7 без автоматизированного покрытия.
+
+После добавления E2E-теста (или эквивалентного JS-юнита для DOM-структуры
++ обработчиков, с явным фиксированием mobile-проверки как manual smoke)
+— PR готов к approve. P2-01 (контракт 403) стоит закрыть в той же
+итерации (правка ADR-015 §G **или** перейти на `JSONResponse`), потому
+что иначе frontend и backend связаны хрупким workaround'ом
+`detail.detail.external_urls`.