diff --git a/docs/work-items/ET-013/12-review.md b/docs/work-items/ET-013/12-review.md index 5b5f625..708fc5a 100644 --- a/docs/work-items/ET-013/12-review.md +++ b/docs/work-items/ET-013/12-review.md @@ -2,9 +2,9 @@ type: review work_item_id: ET-013 verdict: APPROVED -version: 2 +version: 3 created_at: 2026-06-04 -updated_at: 2026-06-04 +updated_at: 2026-06-06 authors: - "agent:reviewer" related: @@ -12,203 +12,136 @@ related: - "ET-013:adr-017" --- -# Review ET-013 — Перепады высот на z9-z11 (re-run #2) +# Review ET-013 — Перепады высот на z9-z11 (re-run #3) ## TL;DR - **Branch:** `feature/ET-013-z9-z11-z8` -- **Scope:** калибровка клиентского paint для hillshade/TRI на z9-z11 - + понижение UI-минзума hillshade с z10 до z9 + расширение whitelist - backend-endpoint'а на `tri` (фикс по результатам review v1, F-1). -- **HEAD:** `099669d fix(terrain): расширить whitelist endpoint'а на 'tri' (ET-013 review F-1)` -- **Что изменилось со времени review v1:** - - `src/api/main.py:1252` whitelist расширен: - `("hypso", "hillshade") → ("hypso", "hillshade", "tri")` + docstring - с пояснением (см. F-1 v1). - - `tests/integration/test_terrain_z9_tiles.py` параметризован по - `layer = ["hillshade", "tri"]` для z9/z10/z11; добавлен явный - регрессионный тест `test_known_terrain_layer_accepted_by_whitelist` - по всем трём слоям (см. F-2 v1). -- **Тесты:** `pytest tests/unit/test_terrain_paint.py` — **17/17 PASS**, - `pytest tests/integration/test_terrain_z9_tiles.py` — **6 passed, 7 skipped** - (skip — отсутствие PNG-данных в sandbox, ожидаемо). -- **Verdict: APPROVED.** P0/P1 не найдено. Остались два опциональных - P3 из v1, оба косметика — не блокеры. +- **HEAD:** `316bb0d tester(ET): auto-commit from tester run_id=85` +- **Scope:** калибровка клиентского paint (hillshade/TRI) на z9-z11, + понижение UI-минзума hillshade z10→z9, обратно-совместимое + расширение `applyTerrainLayer`, расширение whitelist backend-endpoint'а + на `tri` (фикс по review v1, F-1). +- **Что изменилось со времени review v2:** **в коде и тестах — ничего.** + `git diff 099669d..HEAD --name-only` → только `docs/.../12-review.md` + (этот файл, v2) и `docs/.../13-test-report.md` (отчёт Тестера). + Реализация (`src/`, `tests/`) идентична той, что была одобрена в v2. +- **Тесты:** `pytest tests/unit/test_terrain_paint.py` — **17/17 PASS** + (локально, 0.03s). Integration-тесты в sandbox не собираются из-за + отсутствия `shapely` (ограничение окружения, не код) — Тестер + подтвердил 6 passed / 7 skipped в полном окружении (`13-test-report.md`). +- **Verdict: APPROVED.** P0/P1 не найдено. Остаются те же два + опциональных P3-косметических замечания из v1/v2. ## Что прочитано -- `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/12-review.md` v1 (предыдущий вердикт) +- `docs/work-items/ET-013/12-review.md` (v2) +- `docs/work-items/ET-013/13-test-report.md` - `CLAUDE.md` -- `git diff main...HEAD --stat` (18 файлов, +3911/-14) -- `git diff main...HEAD -- src/api/main.py src/web/app.js src/web/index.html` -- `src/api/main.py:1235-1264` (`terrain_tile` после фикса) -- `src/web/app.js` (диапазоны 2725-2835 и 3356-3430) +- `git diff 8da09e6..HEAD -- src/web/app.js src/web/index.html src/api/main.py` + (полный diff фичи) +- `git diff 099669d..HEAD --name-only` (что изменилось со времени v2) +- `src/api/main.py:1240-1264` (`terrain_tile`) +- `src/web/app.js` (2725-2767, 2821-2826, 3356-3430) - `src/web/index.html:57-65` -- `tests/unit/test_terrain_paint.py` -- `tests/integration/test_terrain_z9_tiles.py` +- `tests/unit/test_terrain_paint.py`, `tests/integration/test_terrain_z9_tiles.py` ## Соответствие ТЗ -| Требование | Реализация | Файл / строка | OK | +| Требование | Реализация | Файл | 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:3376-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/{layer}/9/.../….png` → 200 + 404 на невалидный layer + Cache-Control immutable | параметризован по `["hillshade", "tri"]` × `[9, 10, 11]`, регрессии 404, whitelist-тест по 3 слоям | `tests/integration/test_terrain_z9_tiles.py` | ✅ | -| REQ-F-16 — Playwright UI-тесты | в test-плане, исполняет Тестер | — | n/a (review) | +| REQ-F-01 / F-11 — порог `zoom < 9` в `updateHillshadeAvailability` | `if (zoom < 9)` + комментарий | `src/web/app.js:3425` | ✅ | +| REQ-F-02 — hillshade `minzoom=9` | `applyTerrainLayer('terrain-hillshade', …, HILLSHADE_PAINT, 9, 15)` | `app.js:2824` | ✅ | +| REQ-F-03 — TRI `minzoom=5` без изменений | `applyTerrainLayer('terrain-tri', …, TRI_PAINT, 5, 15)` | `app.js:2825` | ✅ | +| REQ-F-04 — обратно-совместимое `opacityOrPaint` | нормализация `typeof === 'number' ? legacy : obj` | `app.js:3376-3380` | ✅ | +| REQ-F-05/06/07 — HILLSHADE_PAINT (opacity/contrast interpolate + nearest) | stops 9/10/11/12/14 → 0.65/0.60/0.55/0.50/0.40; contrast 0.40→0.00; `nearest` | `app.js:2729-2751` | ✅ | +| REQ-F-08/09 — TRI_PAINT (opacity interpolate z8=0.70, пик 0.80-0.85 + nearest) | точное совпадение со spec | `app.js:2754-2767` | ✅ | +| REQ-F-10 — hint «Зум 9+» | `Зум 9+` | `index.html:60` | ✅ | +| REQ-F-12 — контракт `onTerrainCheckbox` (localStorage, `.active`) | без изменений | `app.js:2816-2826` | ✅ | +| REQ-F-13 — unit-тесты paint (Вариант B) | 17 тестов, все PASS | `tests/unit/test_terrain_paint.py` | ✅ | +| REQ-F-14 — регрессии (порог 9, hint, callers) | покрыты | `tests/unit/test_terrain_paint.py` | ✅ | +| REQ-F-15 — integration smoke `/terrain/{layer}/9.../`→200 + 404 + cache-header | параметризация `[hillshade,tri]×[9,10,11]`, whitelist по 3 слоям, 404-регрессии | `tests/integration/test_terrain_z9_tiles.py` | ✅ | +| REQ-F-16 — Playwright UI | исполнено Тестером (`13-test-report.md`) | — | n/a (review) | | REQ-F-17 — localStorage без миграции | не тронуто | — | ✅ | -| REQ-F-18 — API-контракт без изменений | сигнатура `GET /terrain/{layer}/{z}/{x}/{y}.png` сохранена; whitelist расширен (см. §«Изменения после v1») | `src/api/main.py:1240-1264` | ✅ | -| 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-…` присутствуют | — | ✅ | +| REQ-F-18 — API-контракт без изменений | сигнатура `GET /terrain/{layer}/{z}/{x}/{y}.png` сохранена; whitelist расширен на `tri` (см. ADR) | `main.py:1240-1264` | ✅ | +| REQ-F-19 — конфиги/стили не тронуты | `style.json`, `style-dark.json`, `app.css`, `config/*` — без правок | `git diff --stat` | ✅ | +| REQ-F-20 — pre-deploy curl/smoke | этап Деплоя | — | n/a (review) | +| REQ-F-21 — документация | `00`..`10` + `06-adr/ADR-017` присутствуют | — | ✅ | -**Acceptance Criteria.** -- AC-01, AC-02, AC-04, AC-05 (структура paint), AC-15, AC-17, AC-22 - (back-compat) — покрыты unit-тестами, **зелёные**. -- AC-16 — integration-тесты структурно корректны, в sandbox skip - из-за отсутствия PNG; whitelist-регрессия по `tri/hillshade/hypso` - работает без данных и зелёная. -- AC-03, AC-06..AC-13, AC-19, AC-21 — требуют test-среды и Playwright, - относятся к этапу Тестирования. +**Acceptance Criteria.** AC-01/02/04/05/15/17/22 — покрыты unit-тестами +(зелёные). AC-16 — integration структурно корректен (skip без PNG-данных +ожидаем; whitelist-регрессия работает без данных). Поведенческие +AC-03/06..13/19/21 — за этапом Тестирования (см. `13-test-report.md`). ## Соответствие ADR -ADR-017 («Zoom-aware terrain paint») реализован по всем пунктам: +ADR-017 реализован по всем решениям: P-A (frontend-калибровка), +U-A (UI-минзум 10→9), A-A (back-compat `applyTerrainLayer`), +O-B+C-A+R-A (HILLSHADE_PAINT), O-B+R-A (TRI_PAINT), T-A (один paint +на все темы), M-A (константы в `app.js`). -- **P-A** (frontend-only): backend-фикс whitelist'а `tri` — это - **корректная инфра-уточнение**, не выход за P-A. ADR-017 §«Контекст» - утверждал, что эндпоинт уже отдаёт `/terrain/{layer}/…` для TRI; - фактически до этого PR `tri` не был в whitelist'е в dev-режиме, и - фикс восстанавливает заявленное состояние (а не вводит новый - endpoint/source/слой). Документировано в docstring `terrain_tile`. -- **U-A** (UI-минзум 10→9): подтверждено `app.js:3425` и `index.html:60`. -- **A-A** (обратно-совместимое расширение `applyTerrainLayer`): - нормализация числа в legacy-paint реализована (`app.js:3376-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 строка. +**Замечание по разделу «Не меняются: `src/api/main.py`».** Фактически +`main.py:1252` изменён (whitelist `tri`). Это **не нарушение ADR**, а +корректное инфра-уточнение из review v1 (F-1): ADR-017 в «Контексте» +исходил из того, что endpoint уже отдаёт `/terrain/{layer}/…` для TRI; +до фикса `tri` не проходил whitelist в dev-режиме. Фикс восстанавливает +заявленное состояние, не вводит новый endpoint/source/слой/query/header, +сохраняет код ответа и `Cache-Control: immutable` (REQ-F-18 по сути +соблюдён). Документировано в docstring `terrain_tile` со ссылкой на F-1. +Изменение = 1 строка + docstring, низкий риск. -Нарушений ADR-017 не найдено. +## Изменения после review v2 -## Изменения после review v1 (что было исправлено) +Только документация: +- `12-review.md` — переписан до v2 (предыдущий вердикт APPROVED). +- `13-test-report.md` — добавлен Тестером (run_id=85). -| v1 finding | Severity | Статус | Что сделано | -|---|---|---|---| -| F-1 — backend whitelist не пропускает `tri` | P1 | **RESOLVED** | `src/api/main.py:1252`: `("hypso", "hillshade") → ("hypso", "hillshade", "tri")` + docstring с обоснованием (nginx на prod/test перехватывает, но dev-режим должен поддерживать нативно) | -| F-2 — integration-тест не параметризован по layer | P2 | **RESOLVED** | `test_terrain_tile_available_z9_z10_z11` параметризован по `["hillshade", "tri"]` × `[9, 10, 11]`; добавлен явный `test_known_terrain_layer_accepted_by_whitelist[hypso/hillshade/tri]` | -| F-3 — комментарий в `HILLSHADE_PAINT` не упоминает MapLibre clamping ниже z9 | P3 | OPEN (косметика) | Не блокер; см. ниже | -| F-4 — `from __future__ import annotations` неиспользован | P3 | N/A | В текущем integration-тесте `from __future__` отсутствует; в unit-тесте остался, но это микро-косметика | - -Все P0/P1 v1 закрыты. - -## Тесты - -- **Unit (`tests/unit/test_terrain_paint.py`).** 17 тестов, **17 PASS** - локально (Python 3.12.13, pytest 8.3.3, время 0.04s). Покрывают: - объявление констант, форму `interpolate`-выражений, ключевые stops - (z9/11/14 для hillshade, z8/10/11 для TRI), монотонность, - `nearest`-resampling, регрессию порога `< 9` и текста «Зум 9+», - обратную совместимость `applyTerrainLayer`, корректное использование - констант в вызовах. - -- **Integration (`tests/integration/test_terrain_z9_tiles.py`).** - 13 тестов: **6 passed, 7 skipped** в sandbox. - - Skipped: тесты, требующие реальных PNG-тайлов - (`test_terrain_tile_available_z9_z10_z11[*]`, - `test_terrain_tile_cache_control_immutable`) — корректное поведение - через `_maybe_skip`. - - Passed: whitelist-регрессия для всех трёх слоёв - (`hypso/hillshade/tri`), 404 на `unknown_layer`, 404 на - missing tile, 404 на невалидный zoom. Эти тесты доказывают, - что фикс F-1 работает (для `tri` теперь возвращается - `"Tile not found"`, а не `"Unknown layer"`). +Код и тесты (`src/`, `tests/`) **не менялись** с момента APPROVED-вердикта v2. +Ранее закрытые findings v1 (F-1 P1 backend whitelist, F-2 P2 integration +coverage) остаются закрытыми. ## Качество кода -- Стиль соответствует существующему `app.js` (vanilla JS, JSDoc, - комментарии-маркеры `// ET-NNN:`). -- Изменение функции `applyTerrainLayer` минимально-инвазивное: - новая нормализация в 4 строки + переменная `paint`, остальное — - переименование параметра. Никаких ломок других call-sites - (их всего 2, оба в `onTerrainCheckbox`). -- Backend-фикс whitelist'а — 1 строка кода + docstring; не меняет - сигнатуру endpoint'а и не вводит новых query/headers/code-path'ов - (REQ-F-18 формально сохранён). -- Все новые константы (`HILLSHADE_PAINT`, `TRI_PAINT`) UPPER_SNAKE_CASE, - как принято в `app.js`. -- Комментарии содержат ссылки на ADR-017, RTM-аргументы по stops, - ссылку на review F-1 в backend-docstring. -- Нет дублирования, нет dead code, нет `console.log`, нет - закомментированного старого кода. +- Стиль соответствует `app.js` (vanilla JS, JSDoc, маркеры `// ET-013:`). +- `applyTerrainLayer` расширён минимально-инвазивно (4 строки нормализации + + переменная `paint`); оба call-site обновлены, других нет. +- Константы `HILLSHADE_PAINT` / `TRI_PAINT` — UPPER_SNAKE_CASE, рядом с + `TERRAIN_BASE_URL`; комментарии ссылаются на ADR-017. +- Нет дублирования, dead code, `console.log`, закомментированного кода. +- Backend-фикс не вводит новых code-path'ов. -## Findings (текущая ревизия) +## Findings -### P3 — Комментарий в `HILLSHADE_PAINT` не упоминает MapLibre clamping ниже z9 +### F-3 — P3 (косметика) — комментарий HILLSHADE_PAINT не упоминает MapLibre clamping ниже z9 -**Где.** `src/web/app.js:2728-2733`. +**Где.** `src/web/app.js:2729-2733`. Stops opacity начинаются с `9, 0.65`; +при z<9 MapLibre заклампит на нижнем стопе. В текущем scope не проблема +(UI-gate отрубает чекбокс при z<9). Если порог когда-нибудь понизят — +добавить нижний stop `8, 0.00`. **Опционально, не блокер.** -**Замечание.** Stops opacity начинаются с `9, 0.65` — MapLibre сделает -clamping на нижнем стопе, поэтому при z<9 (если когда-нибудь UI-gate -уберут) opacity всё равно будет 0.65, что попадёт в render. -В текущем scope не проблема (UI-gate отрубает чекбокс при z<9), но -если в будущем порог понизят — нужно будет добавить нижний stop -`8, 0.00`. +### F-5 — P3 (косметика) — неиспользуемый `from __future__ import annotations` -**Действие.** Опционально. **Не блокер.** - -### P3 — `from __future__ import annotations` в unit-тесте - -**Где.** `tests/unit/test_terrain_paint.py:15`. - -**Замечание.** Не используется (нет forward-ref в аннотациях). Не вредит. - -**Действие.** Опционально. **Не блокер.** +**Где.** `tests/unit/test_terrain_paint.py:15`. Не вредит. **Опционально.** ## Вердикт **APPROVED.** - P0/P1 не найдено. -- P1 из review v1 (backend whitelist) и P2 (integration coverage) — - закрыты. -- Оставшиеся два P3 — косметика, не влияют на функциональность. - -Реализация ET-013 точно соответствует TRZ REQ-F-01..F-21 и ADR-017. -Тестовое покрытие достаточное: -- AC-01/02/04/05/15/17/22 — закрыты unit-тестами (зелёные). -- AC-16 — закрыт integration-тестами (структурно корректно, skip без данных, whitelist-регрессия зелёная). -- Поведенческие AC (AC-03, AC-06..AC-13, AC-19, AC-21) — корректно - переданы Тестеру для исполнения в test-среде. +- Код/тесты не менялись с момента APPROVED v2; повторная проверка + подтверждает соответствие TRZ REQ-F-01..F-21 и ADR-017. +- Unit-тесты зелёные (17/17). Integration структурно корректен, + подтверждён Тестером. +- Остаются два P3-косметических замечания — не блокеры. ## Сводная таблица findings | ID | Severity | Где | Кратко | Действие | |---|---|---|---|---| -| F-3 | P3 | `src/web/app.js:2728-2733` | комментарий не учитывает MapLibre clamping ниже z9 | опционально добавить явный stop `8, 0.00` | -| F-5 | P3 | `tests/unit/test_terrain_paint.py:15` | `from __future__ import annotations` неиспользован | косметика | +| F-3 | P3 | `src/web/app.js:2729-2733` | комментарий не учитывает MapLibre clamping ниже z9 | опц. добавить stop `8, 0.00` при будущем понижении порога | +| F-5 | P3 | `tests/unit/test_terrain_paint.py:15` | неиспользуемый `from __future__ import annotations` | косметика | P0/P1 отсутствуют → **APPROVED**.