From f6fc9be3243b445cab2747d706f2a9ae0a9dfccc Mon Sep 17 00:00:00 2001 From: claude-bot Date: Thu, 4 Jun 2026 09:54:22 +0000 Subject: [PATCH] reviewer(ET): auto-commit from reviewer run_id=82 --- docs/work-items/ET-013/12-review.md | 287 ++++++++++++++++++++++++++++ 1 file changed, 287 insertions(+) create mode 100644 docs/work-items/ET-013/12-review.md diff --git a/docs/work-items/ET-013/12-review.md b/docs/work-items/ET-013/12-review.md new file mode 100644 index 0000000..7ce2d2e --- /dev/null +++ b/docs/work-items/ET-013/12-review.md @@ -0,0 +1,287 @@ +--- +type: review +work_item_id: ET-013 +verdict: REQUEST_CHANGES +version: 1 +created_at: 2026-06-04 +updated_at: 2026-06-04 +authors: + - "agent:reviewer" +related: + - "ET-013:trz" + - "ET-013:adr-017" +--- + +# Review ET-013 — Перепады высот на z9-z11 + +## TL;DR + +- **Branch:** `feature/ET-013-z9-z11-z8` +- **Scope:** калибровка клиентского paint для hillshade/TRI на z9-z11 + + понижение UI-минзума hillshade с z10 до z9 + (ADR-017, TRZ REQ-F-01..F-21). +- **Реализация:** соответствует TRZ дословно — 79 строк правок в + `src/web/app.js`, 1 строка в `src/web/index.html`, 300 строк + unit-тестов, 129 строк integration-теста. `make test` + (Python-unit) — **17/17 PASS** локально. +- **Verdict: REQUEST_CHANGES** — найден **P1** (потенциально P0) + пробел: backend `terrain_tile` whitelist **не пропускает `tri`**, + поэтому весь TRI-функционал (центральный для ET-013) либо ушёл + в 404, либо обслуживается каким-то внешним прокси, не описанным + ни в репо, ни в TRZ. Перед merge нужно либо доработать backend, + либо явно зафиксировать факт прокси в TRZ/infra. + +## Что прочитано + +- `docs/work-items/ET-013/00-business-request.md` +- `docs/work-items/ET-013/01-brd.md` +- `docs/work-items/ET-013/02-trz.md` +- `docs/work-items/ET-013/03-acceptance-criteria.md` +- `docs/work-items/ET-013/06-adr/ADR-017-zoom-aware-terrain-paint.md` +- `docs/work-items/ET-013/07-infra-requirements.md` (по ссылкам) +- `docs/work-items/ET-013/08-data-requirements.md` (по ссылкам) +- `docs/work-items/ET-013/10-tech-risks.md` (по ссылкам) +- `CLAUDE.md` +- `git diff main...HEAD` (15 файлов, +3564/-12) +- `src/web/app.js` (диапазоны 2715-2835 и 3350-3440) +- `src/web/index.html:57-60` +- `src/api/main.py:1233-1256` (`terrain_tile`) +- `docker-compose.yml`, `Dockerfile` (на предмет прокси) +- `tests/unit/test_terrain_paint.py`, `tests/integration/test_terrain_z9_tiles.py` + +## Соответствие ТЗ + +| Требование | Реализация | Файл / строка | OK | +|---|---|---|---| +| REQ-F-01 — `updateHillshadeAvailability`: порог `zoom < 9` | `if (zoom < 9)` с комментарием ET-013 | `src/web/app.js:3425` | ✅ | +| REQ-F-02 — `source.minzoom = 9` для hillshade | `applyTerrainLayer('terrain-hillshade', …, HILLSHADE_PAINT, 9, 15)` | `src/web/app.js:2825` | ✅ | +| REQ-F-03 — TRI minzoom = 5 без изменений | `applyTerrainLayer('terrain-tri', …, TRI_PAINT, 5, 15)` | `src/web/app.js:2826` | ✅ | +| REQ-F-04 — обратно-совместимое расширение `applyTerrainLayer(opacityOrPaint)` | нормализация `(typeof opacityOrPaint === 'number') ? legacyPaint : opacityOrPaint` | `src/web/app.js:3371-3380` | ✅ | +| REQ-F-05 — HILLSHADE_PAINT `raster-opacity` interpolate по zoom (stops 9/10/11/12/14 → 0.65/0.60/0.55/0.50/0.40) | константа `HILLSHADE_PAINT`, точные stops | `src/web/app.js:2734-2742` | ✅ | +| REQ-F-06 — HILLSHADE_PAINT `raster-contrast` interpolate (stops 9/10/11/12/14 → 0.40/0.35/0.30/0.15/0.00) | присутствует | `src/web/app.js:2743-2750` | ✅ | +| REQ-F-07 — HILLSHADE_PAINT `raster-resampling: 'nearest'` | присутствует | `src/web/app.js:2751` | ✅ | +| REQ-F-08 — TRI_PAINT `raster-opacity` interpolate (z8→0.70, пик z9-z11 = 0.80-0.85) | точное совпадение со spec | `src/web/app.js:2755-2766` | ✅ | +| REQ-F-09 — TRI_PAINT `raster-resampling: 'nearest'` | присутствует | `src/web/app.js:2767` | ✅ | +| REQ-F-10 — hint «Зум 9+» | `Зум 9+` | `src/web/index.html:60` | ✅ | +| REQ-F-11 — единый порог в `updateHillshadeAvailability` | тот же `< 9` | — | ✅ | +| REQ-F-12 — контракт `onTerrainCheckbox` (localStorage `terrain-hillshade`, `terrain-tri`, `#terrain-toggle.active`) | без изменений | `src/web/app.js:2816-2821` | ✅ | +| REQ-F-13 — unit-тесты paint (Вариант B: Python-парсер) | 17 тестов, все PASS локально | `tests/unit/test_terrain_paint.py` | ✅ | +| REQ-F-14 — регрессионные тесты (порог 9, hint, callers) | `test_minzoom_threshold_lowered_to_9`, `test_hint_text_updated_to_z9`, `test_apply_terrain_layer_caller_count` | `tests/unit/test_terrain_paint.py` | ✅ | +| REQ-F-15 — integration smoke: `/terrain/hillshade/9/.../….png` → 200 + 404 на невалидный layer + Cache-Control immutable | `test_hillshade_tile_available_z9_z10_z11` (skip без данных), `test_unknown_terrain_layer_returns_404`, `test_terrain_tile_cache_control_immutable` | `tests/integration/test_terrain_z9_tiles.py` | ⚠ (см. F-1) | +| REQ-F-16 — Playwright UI-тесты | в test-плане, исполняет Тестер | — | n/a (review) | +| REQ-F-17 — localStorage без миграции | не тронуто | — | ✅ | +| REQ-F-18 — API-контракт без изменений | `src/api/main.py` не тронут | — | ✅ | +| REQ-F-19 — конфиги/стили не тронуты | `style.json`, `style-dark.json`, `app.css`, `config/*.yaml` — без правок (`git diff --stat` подтверждает) | — | ✅ | +| REQ-F-20 — pre-deploy curl + smoke | задача deployer'а | — | n/a (review) | +| REQ-F-21 — документация | `00-..-10-` + `06-adr/ADR-017-…` присутствуют | — | ✅ | + +**Acceptance Criteria.** AC-01, AC-02, AC-04, AC-05 (структура paint), +AC-22 (back-compat) — проверяемы unit-тестами и проходят. +AC-03, AC-06..AC-13, AC-19, AC-21 требуют test-среды и не относятся +к review-стадии (передаются Тестеру). AC-15, AC-17, AC-18 — зелёные +(см. §«Тесты»). + +## Соответствие ADR + +ADR-017 («Zoom-aware terrain paint») реализован по всем пунктам: + +- **P-A** (frontend-only): backend / тайлы / стили / nginx не тронуты — подтверждено `git diff --stat`. +- **U-A** (UI-минзум 10→9): подтверждено `app.js:3425` и `index.html:60`. +- **A-A** (обратно-совместимое расширение `applyTerrainLayer`): нормализация числа в legacy-paint реализована (`app.js:3377-3380`), unit-test `test_apply_terrain_layer_normalizes_number_to_legacy_paint` зелёный. +- **O-B + C-A + R-A** для HILLSHADE_PAINT: stops, contrast, `nearest`-resampling — точно по ADR. +- **O-B + R-A** для TRI_PAINT: stops с явной точкой `8→0.70` для регрессии z8 — точно по ADR. +- **T-A** (один paint на все темы): theme-specific paint не добавлен — соответствует MVP-решению ADR. +- **M-A** (константы живут в `app.js` рядом с `TERRAIN_BASE_URL`): подтверждено, расстояние 1 строка. + +Нарушений ADR-017 не найдено. + +## Тесты + +- **Unit (`tests/unit/test_terrain_paint.py`).** 17 тестов, **17 PASS** + локально (Python 3.12.13, pytest 8.3.3, время 0.03s). Покрывают: + объявление констант, форму `interpolate`-выражений, ключевые + stops (z9/11/14 для hillshade, z8/10/11 для TRI), монотонность, + `nearest`-resampling, регрессию порога `< 9` и текста «Зум 9+», + обратную совместимость `applyTerrainLayer`, корректное использование + констант в вызовах. + + Парсер `_extract_block` использует brace-counting (не regex по `}`), + безопасен для вложенных объектов. `_parse_zoom_stops` тоже + устойчив к форматированию. + +- **Integration (`tests/integration/test_terrain_z9_tiles.py`).** В + данной песочнице collection падает из-за отсутствия `shapely` + в окружении (импорт `src/api/main.py` тянет всё подряд) — это + не дефект ET-013, в репо `pyproject.toml` декларирует зависимости, + CI их установит. Структурно тест корректен: + - параметризован по z9/z10/z11, + - skip при отсутствии локальных PNG (через `_maybe_skip`), + - регрессии 404 (unknown layer, missing tile, invalid zoom) — работают всегда, + - дополнительный `test_terrain_tile_cache_control_immutable` проверяет `Cache-Control: public, max-age=31536000, immutable`. + + Замечание (F-1): тест `test_unknown_terrain_layer_returns_404` + использует `unknown_layer`, но это совпадает с реальным + поведением для `tri` — см. P1 ниже. + +## Качество кода + +- Стиль соответствует существующему `app.js` (vanilla JS, JSDoc, + комментарии-маркеры `// ET-NNN:`). +- Изменение функции `applyTerrainLayer` минимально-инвазивное: + новая нормализация в 4 строки + переменная `paint`, остальное + переименование параметра. Никаких ломок других call-sites + (их всего 2, оба в `onTerrainCheckbox`). +- Все новые константы (`HILLSHADE_PAINT`, `TRI_PAINT`) UPPER_SNAKE_CASE, + как принято в `app.js`. +- Комментарии содержат ссылки на ADR-017, RTM-аргументы по stops. +- Нет дублирования, нет dead code, нет `console.log`, нет + закомментированного старого кода. + +## Findings + +### P1 — Backend `terrain_tile` whitelist не пропускает `tri` + +**Severity:** P1 (must-fix перед merge) — может быть P0, если TRI на +test-стенде действительно отдаётся через FastAPI. + +**Где.** `src/api/main.py:1243` (mainline, не тронут ET-013): + +```python +@app.get("/terrain/{layer}/{z}/{x}/{y}.png") +async def terrain_tile(layer: str, z: int, x: int, y: int): + """Отдаёт растровые тайлы рельефа (hypso/hillshade)""" + if layer not in ("hypso", "hillshade"): + raise HTTPException(404, "Unknown layer") + … +``` + +**Проблема.** Frontend клиент шлёт запросы на +`/terrain/tri/{z}/{x}/{y}.png` (BRD §2.1, TRZ §1, `src/web/app.js:2826`), +но backend whitelist возвращает `404 "Unknown layer"` для `layer == "tri"`. +Это **пре-существующее** состояние, унаследованное из коммита +`eda66ee` («feat: migrate prototype to canonical structure»), а не +регрессия ET-013. + +Однако: + +1. **TRZ §2 утверждает**: «`src/api/main.py:1240` (`terrain_tile`) — + **без изменений**». Это формально соблюдено, но скрывает факт, + что endpoint **никогда не поддерживал** `tri`. +2. **ADR-017** в §«Контекст» пишет: «Раздача — `GET /terrain/{layer}/{z}/{x}/{y}.png` + через FastAPI (`src/api/main.py:1240`), `Cache-Control: immutable`» — + подразумевая, что TRI обслуживается тем же endpoint. По коду — нет. +3. **AC-05/AC-06/AC-07/AC-08/AC-09/AC-12/AC-13/AC-21** требуют, чтобы + слой `terrain-tri` рендерился на test-стенде. Если backend + действительно 404-ит, то ET-013 не доставляет основной заявленной + ценности («перепады высот на z9-z11»). + +**Что должно быть проверено перед merge:** + +- (a) **Если** на test-стенде есть внешний прокси (nginx/Caddy/Traefik + на mva154), который перехватывает `/enduro/terrain/tri/…` и отдаёт + PNG напрямую с диска `data/terrain/tri/…` — это нужно явно + задокументировать в `07-infra-requirements.md` (сейчас этого нет) + + в ADR-017 (§«Контекст»). Тогда вердикт по этому пункту снимается. +- (b) **Если** прокси нет — TRZ/ADR не годятся, **возвращать в Анализ** + с правкой: добавить REQ на расширение whitelist + (`if layer not in ("hypso", "hillshade", "tri")`) и integration-тест + `test_tri_tile_available_z9_z10_z11`. Это +2 строки в `main.py` и + один параметризованный тест. + +В обоих случаях нужно отразить решение в `12-review.md` re-run / в +follow-up commit перед approve. + +**Доказательства, что это не «мнение reviewer'а»:** +- `git grep "tri" -- src/api/` возвращает только matches в `LineString` + (shapely), нет ни одного match по слою TRI. +- `docker-compose.yml` показывает единственный сервис `app` + (FastAPI на 5556) — внутри контейнера нет nginx. +- `Dockerfile` устанавливает только `pip install` + `uvicorn` — без + reverse-прокси. +- Internal FastAPI mount-порядок: route `/terrain/{layer}/{z}/{x}/{y}.png` + объявлен **до** `StaticFiles("/")`, поэтому даже если бы TRI лежал + под `STATIC_DIR`, его сначала ловит API-route с whitelist. +- Существующий integration-тест `test_unknown_terrain_layer_returns_404` + проверяет, что `unknown_layer` возвращает 404 — но `tri` тоже + попадает в эту ветку. + +### P2 — Integration-тест не покрывает `tri` + +**Где.** `tests/integration/test_terrain_z9_tiles.py:79-92`. + +**Проблема.** `test_hillshade_tile_available_z9_z10_z11` параметризован +по zoom, но не по layer. Даже если backend whitelist расширят (см. P1), +останется нулевое покрытие endpoint'а для TRI на integration-уровне. + +**Рекомендация.** После исправления P1 — параметризовать тест +по `("hillshade", "tri")` и оставить `parametrize(zoom=[9, 10, 11])`. +Без P1 fix этот тест всё равно будет упасть на `tri`, поэтому +вынесен в P2, а не P1. + +### P3 — Комментарий в `HILLSHADE_PAINT` чуть рассинхронизирован со stops + +**Где.** `src/web/app.js:2730-2733`: + +```js +// Pre-z9 — hillshade не показывается (UI-минзум). На z9-z11 — максимальный +// контраст и opacity, чтобы тени читались как на z8. К z12-z14 — возврат +// к исходным значениям … +``` + +**Замечание.** Формально верно (UI-gate отрубает чекбокс при z<9), +но opacity-stops начинаются с `9, 0.65` — clamping MapLibre на +z<9 даст opacity 0.65, что попадёт в render, если когда-нибудь +UI-gate уберут. Не страшно и в scope не входит, но если в будущем +кто-то понизит UI-минзум ещё ниже (z8), он не должен забыть +добавить нижний stop. Опционально — добавить явный stop `8, 0.00` +для safety. **Не блокер.** + +### P3 — Минор: `from __future__ import annotations` в integration-тесте + +**Где.** `tests/integration/test_terrain_z9_tiles.py:18`. + +Не вредит, но не используется (нет forward-ref в аннотациях). Можно +убрать. **Не блокер.** + +## Вердикт + +**REQUEST_CHANGES.** + +Причина: P1 finding по backend whitelist `tri`. Реализация ET-013 +сама по себе аккуратная и точно соответствует TRZ; но без подтверждения, +что `/terrain/tri/{z}/{x}/{y}.png` реально возвращает 200 на test-стенде, +ключевая часть acceptance criteria (AC-05..AC-09, AC-12, AC-13, AC-21) +не может пройти, и merge приведёт к деплою фичи, которая не работает. + +**Дальнейшие шаги для разработчика/owner:** + +1. Воспроизвести в test-среде: + ```bash + curl -sI https://openclaw.mva154.duckdns.org/enduro/terrain/tri/8/156/79.png | head -1 + curl -sI https://openclaw.mva154.duckdns.org/enduro/terrain/tri/10/623/318.png | head -1 + ``` + - **Если 200** → есть внешний прокси, задокументировать его в + `07-infra-requirements.md` и в ADR-017 §«Контекст», добавить + integration-тест с `requests` (а не TestClient) — отдельный + mark `@pytest.mark.requires_test_env`. После этого re-review. + - **Если 404** → вернуть в Анализ или добавить в текущий PR + минимальный fix: в `src/api/main.py:1243` заменить whitelist на + `("hypso", "hillshade", "tri")`, параметризовать integration-тест + по `("hillshade", "tri")`, обновить `09-validation.md`/`AC-19` + curl-проверкой на TRI z9-z11. После этого re-review. + +2. После решения P1 — устранить P2 (параметризация по layer) и + опционально P3. + +3. После re-review при отсутствии P0/P1 — `verdict: APPROVED`. + +## Сводная таблица findings + +| ID | Severity | Где | Кратко | Действие | +|---|---|---|---|---| +| F-1 | P1 | `src/api/main.py:1243` | backend whitelist не пропускает `tri` → AC-05..AC-09, AC-12, AC-13, AC-21 не выполнимы | проверить прокси на test, либо расширить whitelist + integration-тест | +| F-2 | P2 | `tests/integration/test_terrain_z9_tiles.py:79-92` | integration-тест не параметризован по layer | после F-1 — добавить `("hillshade", "tri")` в `parametrize` | +| F-3 | P3 | `src/web/app.js:2730-2733` | комментарий не учитывает MapLibre clamping ниже z9 | опциональный явный stop `8, 0.00` | +| F-4 | P3 | `tests/integration/test_terrain_z9_tiles.py:18` | `from __future__ import annotations` не используется | косметика | + +P0/P1 присутствуют → **REQUEST_CHANGES**.