diff --git a/docs/work-items/ET-001/12-review.md b/docs/work-items/ET-001/12-review.md new file mode 100644 index 0000000..d084261 --- /dev/null +++ b/docs/work-items/ET-001/12-review.md @@ -0,0 +1,144 @@ +--- +type: code-review +work_item_id: ET-001 +version: 1 +status: approved +reviewer: "agent:reviewer" +date: 2026-05-15 +commit_reviewed: e263f84 +--- + +# Code Review — ET-001 + +## Verdict: **APPROVED** + +Реализация соответствует ТЗ, ADR-001 и acceptance criteria. Все автопроверки +проходят, тесты зелёные. Замечания только P3 (nice-to-have) — не блокируют +мерж. + +## Проверенные файлы + +| Файл | Назначение | Статус | +|---|---|---| +| `infra/osrm/enduro.lua` | OSRM-профиль с блокировкой шлагбаумов и исключением тротуаров | OK | +| `scripts/rebuild-osrm.sh` | Скрипт пересборки графа (extract→partition→customize→restart) | OK | +| `tests/integration/test_routing_barriers.py` | 3 статических + 4 интеграционных теста | OK | + +Изменения за пределы scope не обнаружены — diff чистый, только заявленные +файлы и сопутствующие work-item артефакты. + +## Автопроверки + +- `python3 -m ruff check src/ tests/integration/test_routing_barriers.py` → **All checks passed!** (AC-5) +- `bash -n scripts/rebuild-osrm.sh` → синтаксис ок, файл исполняемый. +- Lua: `luac` в окружении отсутствует, поэтому test_lua_syntax деградировал + до структурных проверок (наличие `process_node`/`process_way`/`process_turn`/ + `setup` и финального `return`). Структура корректна. По коду профиля + очевидных синтаксических проблем нет: таблицы закрыты, `function`/`end` + сбалансированы, `api_version = 4` соответствует OSRM ≥ 5.20. (AC-5 — частично, + полная проверка `luac -p` будет в CI с установленным lua-runtime.) +- `pytest tests/integration/test_routing_barriers.py` → **7 passed in 0.28s** + (TC-001..TC-005 + 2 статических AC-теста). OSRM-сервер при прогоне был доступен, + интеграционные тесты реально выполнились, а не зачислились по `skipif`. (AC-4) + +## Соответствие AC (чеклист) + +### AC-1: Шлагбаумы заблокированы — **PASS** +- [x] `blocked_barriers` в `enduro.lua` (стр. 68–77) содержит ровно 8 типов из ТЗ: + `gate`, `bollard`, `lift_gate`, `chain`, `cycle_barrier`, + `motorcycle_barrier`, `border_control`, `block`. +- [x] `process_node` (стр. 103–111) выставляет + `forward_mode = mode.inaccessible` и `backward_mode = mode.inaccessible` — + ровно как требует ADR-001 (Альтернатива A). +- [x] `cattle_grid` и `ford` в списке отсутствуют (явно проверено в + `test_blocked_barriers_match_trz`). + +### AC-2: Тротуары исключены — **PASS** +- [x] `excluded_highways` (стр. 80–85) содержит `footway`, `pedestrian`, `steps`, + `corridor`. +- [x] `process_way` (стр. 117–118) делает ранний `return` для этих типов. +- [x] В `highway_rate` (стр. 16–34) этих ключей нет — проверено + `test_excluded_highways_match_trz`. + +### AC-3: Скрипт пересборки — **PASS** +- [x] `scripts/rebuild-osrm.sh` рабочий, `set -euo pipefail`, валидирует наличие + каталога / pbf / lua до запуска docker. +- [x] Содержит все четыре шага: `osrm-extract` → `osrm-partition` → + `osrm-customize` → `docker restart`. +- [x] Параметризован через env-переменные (`OSRM_DIR`, `OSRM_PBF`, + `OSRM_PROFILE`, `OSRM_IMAGE`, `OSRM_CONTAINER`) с разумными default'ами, + совпадающими с ТЗ §2. +- [x] Корректная обработка отсутствующего контейнера (WARNING вместо падения). + +### AC-4: Тесты — **PASS** +- [x] Минимум 3 integration теста (`test_route_avoids_barrier`, + `test_route_no_footway`, `test_route_allows_cattle_grid`, + `test_existing_route_works`) — фактически 4. Покрыты TC-001, TC-002, + TC-003, TC-005 из `04-test-plan.yaml`. +- [x] Дополнительно покрыт TC-004 (`test_lua_syntax`) и два AC-теста на состав + таблиц — статические, гоняются всегда. +- [x] `osrm_required` корректно skip'ает интеграционные тесты при отсутствии + OSRM — CI без инфры не падает. +- [x] Все 7 тестов проходят локально. + +### AC-5: Lint — **PASS** (с оговоркой) +- [x] `ruff check` — 0 ошибок. +- [x] Lua структурно корректен; полная `luac -p` будет в CI. + +### AC-6: Обратная совместимость — **PASS** +- [x] TC-005 (`test_existing_route_works`) — регрессия на обычный маршрут + без шлагбаумов/тротуаров. Прошёл. +- [x] API `/api/route` не трогался — изменения только в lua-профиле OSRM. +- [x] Логика `path`/`cycleway` в городской застройке, веса `highway_rate`, + `tracktype_multiplier`, `process_turn` сохранены без изменений + (соответствует ограничению ТЗ §5: «НЕ менять веса существующих дорог»). + +## Замечания + +### P3 (nice-to-have, не блокируют) + +1. **`tests/integration/test_routing_barriers.py:47–50`** — `BARRIER_NODE` + собирается как `(float(os.environ.get(..., "0")) or None, ...)`. Если + переменная задана легитимным значением `"0"`, она превратится в `None` + из-за `0.0 or None`. На практике координата `(0,0)` бессмысленна для ЦФО, + и ниже есть явная проверка `if node_lon is None or node_lat is None`, так + что функционально безопасно. Косметически чище было бы `None` по умолчанию + и явный `float()` после проверки на наличие переменной. + +2. **`tests/integration/test_routing_barriers.py:294–298`** — проверка + «footway/тротуар в name шага» — слабая эвристика (OSM редко вписывает + "footway" в `name`). Это покрытие TC-002 по факту тонкое. Для усиления + можно дополнительно проверять `step.mode` (если OSRM его отдаёт) или + аннотации. Сейчас принимаем — ТЗ не требует жёсткой проверки тегов + сегментов, а на уровне графа footway уже выкинут (AC-2 закрыт статически). + +3. **`infra/osrm/enduro.lua:9`** — `api_version = 4` объявлен глобально без + `local`. Это норма для OSRM lua API (osrm-extract читает именно глобал), + но стоит оставить комментарий «глобал — требование OSRM API», чтобы + будущий читатель не подумал, что забыли `local`. Чистая косметика. + +### P0/P1/P2 + +Нет. + +## Соответствие ADR-001 + +- [x] Решение применено в коде ровно как в разделе «Решение» ADR-001: + `mode.inaccessible` на обе стороны, тег `access` игнорируется. +- [x] Альтернатива B (penalty) и Альтернатива C (учитывать access) не + использованы — корректно. + +## Соответствие ТЗ §5 (ограничения) + +- [x] Веса существующих дорог не изменены (highway_rate не трогали — только + убрали оттуда footway/pedestrian/steps, которые и в исходнике могли + отсутствовать, но AC-2 явно требует). +- [x] scenic/link/recon логика не задета (в текущем профиле её не было — diff + это подтверждает). +- [x] `cattle_grid` и `ford` не блокируются. +- [x] Пересборка графа — ручной шаг (`scripts/rebuild-osrm.sh`), не в CI. + +## Итог + +Готово к мержу. После мержа — выполнить ручной шаг пересборки графа на +mva154 согласно `07-infra-requirements.md`.