diff --git a/docs/reviews/CODE-REVIEW-585-r1.md b/docs/reviews/CODE-REVIEW-585-r1.md new file mode 100644 index 00000000..cfe0deb2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-585-r1.md @@ -0,0 +1,235 @@ +# CODE-REVIEW-585-r1 + +Материал: `8769905b4e0e872ef284570452924965a6b25c9a` (`issue/585-iso-overlay-boundary-search`), диапазон `origin/dev..HEAD` — 15 коммитов. +Заход r1, блокирующих циклов израсходовано 0 из 4. + +## Скоуп + +Задача #585 чинит регрессию #583: в скрытом 2.5D-виде (`hp_alpha`) грубая решётка +4 CSS px поиска свободного места для маркеров устройств/замков теряет разрешимую +щель, если она уже 4 px — в одной из 12 изометрических golden-сцен два маркера +устройства визуально сливаются. ТЗ требует заменить решётку/полный скан диска на +детерминированный поиск по границам запрещённых областей (O(периметра) вместо +O(площади)), сохранив все существующие точные проверки, и вернуть временно +расширенные performance-допуски двух изометрических профилей к исходным +150/60/75. + +Диапазон изменений строго соответствует заявленному скоупу: только +`src/iso-overlays.ts` (алгоритм) и `src/iso-scene-render.ts` (минимальное +подключение кеш-хинта, как разрешено §13 ТЗ), плюс тесты, golden одной сцены, +два performance-budget файла, канонические доки и сгенерированный `dist`/ +`custom_components/.../frontend`. i18n, конфиг, `CHANGELOG*` не тронуты — везде +`User-Visible: no`, что верно для скрытого экспериментального вида. + +## Как проверялось + +1. Прочитано полное ТЗ из тела issue #585 и оба предыдущих комментария + (аналитика, зелёный spec-review r1). +2. Прочитан весь `src/iso-overlays.ts` (1648 строк) и diff + `src/iso-scene-render.ts` целиком; прослежена последовательность + промежуточных коммитов (`6f226a5b` → `df836c81`), каждый из которых чинит + конкретный найденный на golden/perf прогонах дефект предыдущего шага + (кэш-хинт как точная граница, а не досрочный успех; сохранение + утверждённого tie-break; раскрытие границы комнаты только по свидетелю). +3. Прочитан весь diff `test/iso-overlays.test.mjs` (7 новых тестов) и сверен + с AC1/AC3/AC4. +4. Прочитан diff `test/performance-budget.test.mjs` и обоих + `demo/performance/budgets-*.json` — допуски 150/60/75 и неизменные + `hardMaxMs`/`maxRegressionRatio` (2200/600, 0.2) подтверждены построчно. +5. Прочитан diff `docs/ISOMETRIC.md`, `docs/ARCHITECTURE.md`, `docs/STATUS.md`. +6. Проверен единственный изменившийся golden (`demo/golden/baselines/ + baselines-index.json` — построчный diff показывает ровно одну изменившуюся + запись сцены, остальные 105 хэшей не тронуты) и сам PNG прочитан визуально: + в Room 2.5 видны два раздельных (хоть и близко стоящих) маркера вместо + одного слитого пятна — соответствует ожиданию AC2. +7. Независимо проверено выполнение AC6 (Linux CI, exact-SHA, полные профили): + `gh run list --workflow=performance.yml` нашёл прогон `35477092908` + (`workflow_dispatch`, `headSha=8769905b…`, `conclusion=success`); в логах + job `isometric` и `isometric-stage3` — таблицы `benchmark:compare` с + `candidate-sha=8769905b…`, все строки `✅`, лимиты посчитаны от + восстановленных 150/60/75 (например `panZoomMs` isometric: candidate 256.1 + ≤ limit 261.9). Это ни автор, ни spec-review не привязали ссылкой нигде — + см. находку ниже. +8. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — см. раздел + про смоки. +9. `git diff origin/dev...HEAD --stat` полностью просмотрен: подтверждено, что + не тронуты `custom_components/**/*.py`, i18n, `CHANGELOG*`, конфиг/схема. + +## Находки + +### Medium (в скоупе, чинится в этой же ветке) — устаревшая строка docs/STATUS.md и отсутствующий хендофф evidence + +`docs/STATUS.md` был обновлён один раз, коммитом `6f226a5b` (22:50, самое +начало работы над задачей) и с тех пор не трогался. Текущая строка снапшота +гласит: + +> «In implementation: #585 replaces the hidden 2.5D overlay's lossy 4 px +> lattice with iterative critical-boundary events, restores the dense Room 2.5 +> marker pair and returns both full isometric profiles to their ordinary +> performance allowances; **Linux golden/performance evidence is still +> required before merge**.» + +На момент этого код-ревью (материал `8769905b`) полный Linux performance-прогон +на точном SHA уже существует и зелёный (run `35477092908`, см. пункт 7 выше), +golden уже принят (`Baseline-Reviewed` в `86dffbc7`). Утверждение «evidence is +still required» больше не верно, а в самой задаче нет комментария с этими +ссылками — ни ссылки на `35477092908`, ни на прогон golden в финальном виде не +попали в хендофф issue (три существующих комментария — аналитика, зелёный +spec-review, «Взял: агент»). Это прямо расходится с ТЗ §12.5 («Запустить оба +полных изометрических performance-профиля на точном SHA кандидата и сохранить +ссылки/run ids в handoff») и AC8 («канонические документы... описывают... +снятие временного performance-исключения» — снятие исключения не отражено как +свершившийся факт). + +**Почему это Medium, а не Low:** AC7 требует «перед передачей на код-ревью +приложены требуемые targeted golden/performance evidence» — формально это не +выполнено, я нашёл и проверил прогон сам, а не по ссылке автора. AC8 явно +называет `docs/STATUS.md` файлом, который должен отражать актуальный статус; +сейчас он говорит будущему читателю, что доказательство ещё не получено, хотя +оно уже есть и зелёное. Это не блокирует функциональность (сам фикс работает и +доказан), но воспроизводит ровно тот сценарий, о котором предупреждает процесс +ревью: «Verified без названной команды и её результата доказательством не +является» — только тут наоборот, доказательство есть, но не названо нигде, +кроме как найдено ревьюером постфактум. + +**Что нужно поправить:** одним коммитом в этой же ветке — +1. обновить строку `docs/STATUS.md`, отразив, что фикс завершён и оба полных + профиля прошли на точном SHA (со ссылкой на run); +2. оставить в issue #585 комментарий с ссылками на `35477092908` (performance) + и на golden-прогон/baseline-review, как требует §12.5 ТЗ — это же служит + базой для «дисциплины хендоффа», на которую опирается сам процесс ревью. + +## Проверено и корректно + +- **AC1 (узкая щель, unit):** `group collision finds a legal one-pixel slit + between the coarse nodes` (`test/iso-overlays.test.mjs:139`) — геометрия, + где старая решётка 4 px не находила офсет 41 (только 40 «внутри» и 44 «уже + кладка»), теперь находится точно; `residualPairs` пуст, `status: 'ok'`. + Тест умеет падать: до фикса (коммит `6f226a5b`) это был red-witness, что + подтверждается сообщением коммита «fix(iso): находить узкие щели по + границам препятствий». +- **AC2 (проблемная сцена, golden + частично unit):** golden + `isometric-large-warm-remount-dark` принят с явным `Baseline-Reviewed` + (`86dffbc7`, run `35470468142`), изображение прочитано — маркеры Room 2.5 + визуально раздельны. Дополнительный unit-свидетель на синтетической, а не + буквальной геометрии Room 2.5 (см. замечание ниже, снято без записи как + Low) — по духу требования «golden не единственный оракул» выполнено: тот же + топологический дефект (щель между узлами 4 px решётки) воспроизведён и + проверяется без растровой сверки в `group collision finds a legal + one-pixel slit…` и в `group collision boundary events stay sparse…`. +- **AC3 (сохранение контрактов, unit):** порядок/перестановка + (`group collision separates a solvable dense set independently of input + order`), макс. радиус и деградация (`cap and ambiguous ownership fail safe…`, + `group collision reports a deterministic residual…`), room/hole/wall safety + (`nudge never crosses an island hole…`, `strict room ownership excludes + holes…`), исключение room labels (`isoOverlayPlane`/owner-тесты, строки + 49-55, 260-261), fit-mode skip (код `mode === 'live'` в + `iso-scene-render.ts` не тронут, кеш-хинт добавлен только внутри той же + ветки), деградация без бросков (`malformed collision input degrades…`) — всё + на месте, ничего не удалено. +- **AC4 (ограниченность поиска, unit + чтение кода):** + `buildIsoOverlayBoundaryCandidates` — публичный чистый генератор, покрыт + тестом на разреженность (`candidates.length < 7238/10`) и на отсутствие + ложных перекрёстных пересечений несвязанных прямоугольников (`boundary + events do not cross unrelated rectangle axes…`). Чтением кода подтверждено: + `addBoundaryCircle` — O(radius), не O(radius²); `localRefinementOffsets` + ограничена окном 9×9; расширение стен/оверлеев в главном цикле + `resolveIsoOverlayCollisions` — по мере обнаружения (`pendingWalls`, + `encounteredOverlayBoundaries`), а не заранее по всей сцене. Полного + однопиксельного скана диска в коде нет. +- **AC5 (визуальная локальность):** построчный diff + `baselines-index.json` показывает изменение ровно одного хэша сцены + (`isometric-large-warm-remount-dark`), остальные 105 записей и + `docs/images/screenshots.json`/`docs/images/*.png` — отдельная, явно + объяснённая история про уже смёрженный #598 (`61935250`, со своей + Linux-проверкой и просмотром трёх кадров), не про рендер этой задачи. +- **AC6 (performance, Linux CI):** оба файла budgets вернули точно 150/60/75 + при неизменных `hardMaxMs` (2200/600) и `maxRegressionRatio` (0.2) для всех + метрик — построчно сверено с ТЗ §10. Независимо найден и прочитан + зелёный прогон `performance.yml` run `35477092908` на точном SHA `8769905b`; + оба профиля (`isometric`, `isometric-stage3`) — `candidate-sha` совпадает, + все проверки `✅`. Формально не хендофнуто автором — см. находку выше. +- **AC7 (обычные гейты):** Validate на этом SHA зелёный (см. вводные к + ревью, run 35477091271) — `tsc`/`test:unit`/`build` не перегонялись повторно + по правилам сужения гейтов. `node scripts/check-docs.mjs` не запускался + отдельно: diff трогает `src/**`, но строгий docs-гейт уже входит в Validate + и он зелёный на этом SHA; отдельного повода сомневаться в его результате нет + (fingerprint обновлён последним коммитом `8769905b`, подтверждён Linux- + прогоном 35476979319 в тексте коммита). +- **AC8 (документация):** `docs/ISOMETRIC.md` и `docs/ARCHITECTURE.md` + корректно описывают именно тот алгоритм, что реализован (границы, а не + решётка/скан; порядок фильтров: приблизительные bounds → точные предикаты); + сверено построчно с кодом. `docs/STATUS.md` обновлён не полностью — см. + находку выше. +- Инварианты геометрии плана (`npm run invariants`) не требуются: задача + явно не трогает сохранённую геометрию/toolchain модели (§8 ТЗ), только + производную экранную геометрию текущего кадра. + +## Чего не проверял и почему + +- **`npx tsc --noEmit`, `npm test`, `npm run build`** — не перегонял. + Validate на точном материале `8769905b` зелёный (ссылка в вводных к + ревью, run 35477091271), включает все три. Дополнительных изменений после + этого прогона в дереве нет (материал — тот же коммит). +- **`npm run golden:verify` (полная матрица)** — не перегонял отдельно; + Validate на этом SHA уже гоняет полный golden-набор (в diff видно + «остальные 172 сцены Linux-прогона совпали с эталонами» в `86dffbc7`, и + финальный докс-фингерпринт подтверждён последующими зелёными + Linux-прогонами вплоть до самого HEAD). +- **`npm run invariants -- --config …`** — не требуется: diff не касается + рёбер комнат, записей толщины, `layout`, `marker.space`, `open_spans`; это + чисто экранная 2.5D-геометрия, не персистентная модель. +- **`python -m pytest tests_backend`** — не требуется, `custom_components/**/ + *.py` не тронут. +- **Browser-смоки `demo/smoke_*.mjs`** — не гонял. + `node scripts/smoke-select.mjs --base origin/dev --head HEAD` вернул + «прямое совпадение» на символе `pointInRing` для 4 смоков + (`smoke_multiwall_strip_containment`, `smoke_sun`, + `smoke_wall_thickness_transition`, `smoke_zero_divider_taper`). Проверено + чтением: `pointInRing` в `src/iso-overlays.ts` — приватная, неэкспортируемая + функция этого модуля; grep по `src/*.ts` не находит одноимённой функции ни в + wall-thickness/sun-геометрии, ни где-либо ещё, откуда эти смоки могли бы её + импортировать. Совпадение — по имени символа на изменённых строках, не по + графу вызовов; сами смоки проверяют плоский вид и толщину стен, которые ТЗ + прямо исключает из скоупа (§5) и которые физически не используют этот + приватный helper. Это ровно случай «слабая связь — повод посмотреть, а не + обязанность прогонять» из инструкции; посмотрел, прогонять не стал. +- **Golden PNG у изменившейся сцены** — не пересчитывал построением бандла; + прочитал уже принятый в коммите файл визуально (см. «Как проверялось», п.6) + и полагаюсь на канонический процесс `Baseline-Reviewed` (`86dffbc7`, + run `35470468142`), который для этого и существует. +- **Формальное построчное доказательство корректности итеративного + worklist-алгоритма** (`resolveIsoOverlayCollisions`, ~350 новых строк) — + не проводил как отдельную формальную процедуру: слишком дорого для code + review. Доверие построено на комбинации (а) 172+ golden-сцен, не изменившихся + побайтно, (б) целевых unit-тестов на конкретные топологии (щель, диагональная + комната + кэш-хинт, вогнутая стена вместо бампера её bbox), (в) видимой + дисциплины автора — каждый найденный на golden/perf прогоне контрпример чинился + отдельным коммитом с новым регрессионным тестом (`f5365f49`, `ca739743`, + `df836c81`). Остаточный риск: топология уже стен + уже собственных + оверлеев одновременно в очень плотной сцене вне покрытых сценариев — не + исключаю, но это тот же риск, что explicitly назван в ТЗ §14 и который + golden-матрица (172 сцены) частично снимает. + +## Вердикт + +Жёлтый. High: 0, Medium: 1 (в скоупе, возвращается автору в этой же ветке — +не заводится отдельным issue). Функциональность фикса подтверждена (AC1–AC6 +доказаны, включая независимо найденный зелёный exact-SHA performance-прогон); +единственная находка — процессная/документационная: `docs/STATUS.md` не +обновлён после получения evidence и сам handoff с run-ссылками не оставлен в +issue, как того требует ТЗ §12.5/AC7/AC8. + +--- + + + +## Материал раунда + +- Ветка: `issue/585-iso-overlay-boundary-search`, коммит `8769905b4e0e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `871720d6cd52ae82a178847032d4b7225b3c01d6` + ``` + git log --all --format='%H %T' | grep 871720d6cd52 + ``` +- Тело issue: `7e85ec65b07414bb734b5f19ffb10510ee60b3af4fdb52ed429df29d2593ebc8` +- Вердикт конвейера: `yellow` · High 0