houseplan-card/docs/reviews/CODE-REVIEW-585-r1.md

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