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

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

@@ -0,0 +1,152 @@
+---
+type: review
+work_item_id: ET-015
+verdict: APPROVED
+version: 1
+---
+
+# Review ET-015 — Healthcheck enduro-trails-app (python urllib one-liner)
+
+**Branch:** `feature/ET-015-healthcheck-enduro-trails-app-`
+**Base:** `main`
+**Reviewer:** agent:reviewer
+**Date:** 2026-06-05
+
+## Что проверял
+
+- TRZ: `docs/work-items/ET-015/02-trz.md` (особенно §3.1, §3.2, §3.3, R-1..R-5)
+- AC: `docs/work-items/ET-015/03-acceptance-criteria.md` (AC-01..AC-10)
+- ADR: `docs/work-items/ET-015/06-adr/ADR-020-healthcheck-via-python-urllib.md`
+- Глобальный ADR-индекс: `docs/architecture/adr/README.md`
+- PR diff (`git diff main..HEAD`): `docker-compose.yml`, `CHANGELOG.md`,
+  `docs/architecture/adr/README.md`, артефакты `docs/work-items/ET-015/**`,
+  `tests/static/test_healthcheck_compose.py`,
+  `tests/unit/test_healthcheck_oneliner.py`.
+- Запуск тестов: `pytest tests/static/test_healthcheck_compose.py
+  tests/unit/test_healthcheck_oneliner.py -v` → **16 passed**.
+
+## Соответствие ТЗ
+
+| Пункт TRZ | Ожидание | Факт в diff | Статус |
+|-----------|----------|-------------|--------|
+| §3.1 | YAML-массив `["CMD","python","-c", "<one-liner>"]` + `start_period: 20s`, `interval/timeout/retries` сохранены | `docker-compose.yml` lines 22–31 — байт-в-байт совпадает с §3.1 ADR-020 | ✓ |
+| §3.2 | Dockerfile НЕ меняется | `git diff main..HEAD -- Dockerfile` пуст | ✓ |
+| §3.3 | `src/api/main.py` НЕ меняется | `git diff main..HEAD -- src/api/main.py` пуст | ✓ |
+| R-1 | Затронут только `app.healthcheck`, прочие поля сервиса не тронуты | Подтверждено diff'ом — ports/volumes/environment не сдвинуты | ✓ |
+| R-2 | Изменение не требует `docker compose build` | Образ не меняется, команда исполняется существующим `python` интерпретатором | ✓ |
+| R-3 | Никаких ENV для пути healthcheck | URL зашит литералом | ✓ |
+| R-4 | ADR в `06-adr/` + запись в CHANGELOG | `ADR-020-healthcheck-via-python-urllib.md` + Unreleased/Fixed в `CHANGELOG.md` | ✓ |
+| R-5 | YAML валидный | `yaml.safe_load(open("docker-compose.yml"))` парсит без ошибок (проверено) | ✓ |
+
+## Соответствие Acceptance Criteria
+
+| AC | Тест | Результат |
+|----|------|-----------|
+| AC-03 «нет curl в healthcheck» | ST-01 (`test_st01_healthcheck_does_not_use_curl`) | PASS |
+| AC-04 «Dockerfile не ставит curl/wget» | ST-02 (`test_st02_dockerfile_does_not_apt_install_curl_or_wget`) + IT-04 (manual) | PASS (static) |
+| AC-05 «честно фиксирует unhealthy» | UT-02 (`test_ut02_returns_nonzero_when_port_unused`) + IT-03 (manual) | PASS (unit) |
+| AC-06 «stdlib python one-liner» | ST-03, UT-01, UT-03 (4 параметризации: 301/404/500/503) | PASS |
+| AC-07 «внутренний timeout < внешнего» | ST-04 (`test_st04_internal_timeout_less_than_external`) — `3 < 5` | PASS |
+| AC-08 «/api/health не сломан» | `git diff main..HEAD -- src/api/main.py` пуст; E2E-02 (manual) | PASS (static) |
+| AC-09 «CHANGELOG обновлён» | ST-06 (`test_st06_changelog_mentions_et015`) | PASS |
+| AC-10 «ADR зафиксирован» | ST-07 (`test_st07_adr_exists`) + ручная проверка содержимого ADR-020 | PASS |
+| AC-01, AC-02 «healthy после деплоя / стабилен 10 минут» | IT-01/IT-02/E2E-01 — оператор/deployer | Pending (вне review) |
+
+Замечание: AC-01/AC-02 закрываются только на live-среде (deployer/ops после
+`make deploy-test`); это явно зафиксировано в плане тестов (`done_when`).
+Review не блокирует — статические + unit-проверки полностью покрывают всё,
+что можно проверить из ветки.
+
+## Соответствие ADR-020
+
+- §«Решение» п.1 — YAML-блок 1:1 совпадает с фактическим `docker-compose.yml`.
+- §«Решение» п.2 — Dockerfile не тронут ✓.
+- §«Решение» п.3 — `main.py` не тронут ✓.
+- §«Решение» п.4 — `gps-collector` healthcheck не получает (в diff'е сервис
+  не меняется) ✓.
+- §«Решение» п.5 — `CHANGELOG.md` Unreleased/Fixed содержит ET-015 + строку
+  `fix(infra): use python urllib for container healthcheck (ET-015)` ✓.
+- Глобальный индекс ADR (`docs/architecture/adr/README.md`) пополнен строкой
+  ADR-020 ✓ (соблюдено процессное требование).
+- Альтернативы B/C/D/E явно отклонены и не «протекли» в реализацию (curl/wget
+  не появились, отдельный `scripts/healthcheck.py` не создан, `HEALTHCHECK`
+  директива в Dockerfile не добавлена) ✓.
+
+## Качество кода
+
+- **YAML.** Используется `CMD` (массив), а не `CMD-SHELL`. Корректно: Docker
+  выполняет `exec`-ом без shell-парсинга, экранирование не нужно.
+- **One-liner.** `import urllib.request, sys; sys.exit(0 if
+  urllib.request.urlopen(URL, timeout=3).status == 200 else 1)` —
+  компактно, без побочных эффектов, исключения корректно превращаются в
+  ненулевой exit code, что и нужно Docker'у.
+- **`start_period: 20s`** добавлен — оправдан в ADR/TRZ, защищает от ложных
+  фейлов в первые секунды старта uvicorn.
+- **Diff минимален и хирургичен.** Затронут ровно один логический блок —
+  это и есть «minor-change» по классификации ADR-020 §«Классификация».
+
+## Качество тестов
+
+- **`tests/static/test_healthcheck_compose.py`** (10 тестов):
+  - 6 первичных (ST-01..ST-04, ST-06, ST-07) с явной привязкой к AC и
+    источникам правды в docstring.
+  - 3 регрессивных: проверка локального URL, наличие `start_period`,
+    параметризованная проверка инвариантов `interval ≥ 30`, `retries ≥ 3`
+    (защита ADR-020 «инвариант: параметры не уменьшаются»).
+  - Чёрный список сторонних пакетов (`requests/httpx/aiohttp/urllib3`)
+    через `\b<pkg>\b` — корректный приём против ложных совпадений
+    подстроками.
+- **`tests/unit/test_healthcheck_oneliner.py`** (6 тестов):
+  - Ключевая фишка: код one-liner'а **читается из `docker-compose.yml`**
+    и URL подменяется через `_retarget()` — под тест уходит ровно та же
+    логика, что и в проде. Если в compose кто-то поменяет one-liner и
+    сломает контракт exit-code, эти тесты упадут.
+  - UT-01 проверяет `exit 0` на HTTP 200, UT-02 — `exit ≠ 0` при пустом
+    порту, UT-03 параметризован по 301/404/500/503 (защита от подмены
+    `== 200` на `< 400` или подобное).
+  - Мок-сервер на `http.server` — без внешних зависимостей, без флакки.
+- Тесты **запущены локально** (`pytest -v`): **16 passed** за 2.89s.
+
+## Findings
+
+### P0 (blocker)
+
+Нет.
+
+### P1 (must-fix)
+
+Нет.
+
+### P2 (should-fix)
+
+Нет.
+
+### P3 (nice-to-have)
+
+- **P3-1.** `CHANGELOG.md` исторически содержит **два** `## [Unreleased]`
+  заголовка (строки 6 и 151) — это унаследованная проблема репозитория,
+  PR ET-015 её не вносит и не усугубляет. Просто фиксирую — стоит когда-нибудь
+  устранить в отдельной задаче `docs:`. ST-06 на этом не ломается, потому что
+  ищет ET-015 в любой части файла, а не «строго в верхней Unreleased».
+- **P3-2.** В TRZ §1 формулировка цели говорит про «HTTP-код **2xx** как
+  healthy», но §3.1 (и реализация) проверяют именно `status == 200`. На
+  практике `/api/health` отдаёт 200, и UT-03 явно фиксирует поведение
+  для 301/404/500/503 → unhealthy, что согласуется с ADR-020. Это
+  ожидаемое сужение, зафиксированное в ADR-020 §«Решение»; стоит лишь
+  иметь в виду как документационную «шероховатость» в TRZ. Менять
+  поведение **не нужно**: иначе сломается часть UT-03 (301).
+- **P3-3.** `urllib.request.urlopen(...).status` — объект ответа не
+  закрывается (нет `with`). Для одноразового процесса healthcheck это
+  безопасно (process завершится через `sys.exit`), но из стилистических
+  соображений можно когда-нибудь обернуть в `with`. Не блокирует
+  и не входит в скоуп ET-015.
+
+## Вердикт
+
+**APPROVED.** Все P0/P1/P2 пусты. Реализация 1-в-1 соответствует TRZ §3.1
+и ADR-020 §«Решение»; не выходит за рамки BRD §6/§7; тесты адекватно
+покрывают статические инварианты и поведение exit-кода one-liner'а.
+Финальные AC-01/AC-02 закрываются на этапе deploy на mva154 — это
+ожидаемо и зафиксировано в `04-test-plan.yaml::done_when`.
+
+Можно передавать дальше (tester → deployer).