diff --git a/docs/reviews/CODE-REVIEW-309-r1.md b/docs/reviews/CODE-REVIEW-309-r1.md new file mode 100644 index 00000000..88069707 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-309-r1.md @@ -0,0 +1,242 @@ +# CODE-REVIEW-309-r1 + +Issue: https://github.com/Matysh/houseplan-card/issues/309 +Ветка: `issue/309-junction-visual-limit` +Ревьюер: Claude (код-ревью), сессия отдельна от автора (Codex) +Диапазон: `origin/dev...HEAD`, HEAD = `e85bae70f0e13a70f0395e8199a2bdcc3d775a26` +Заход: r1 (первый заход этапа код-ревью; этап ТЗ уже прошёл 3 захода и закрыт +зелёным вердиктом SPEC-REVIEW-309-r3, бюджет циклов код-ревью отдельный — 0/4). + +## Скоуп проверки + +Три коммита класса A/B поверх ТЗ: + +1. `101cf709` `fix: cap junction mitres at the visual limit and drop + foreign-sector pair patches (#309)` — `User-Visible: yes`, changelog + RU+EN в этом же коммите. +2. `e85bae70` `test: accept the #309 junction-teeth baselines` — класс D, + `Release:`/`Baseline-Reviewed:` присутствуют. + +Единственный тронутый продуктовый файл — `src/wall-thickness.ts` (4 хунка): +константа `VISUAL_MITRE_LIMIT` + функция `chamferApex`; ветка парных патчей +`linearWallJoinPatches` (порог фаски + скип узлов ≥3 лучей через локальный +`buildMultiWallNodeMap`); ветка вееров `junctionNodeGeometry` (та же фаска). +Диффом подтверждено (не заявлением автора): `MULTI_WALL_JOIN_LIMIT`, +`multiWallBevelCutsAt`, mitre контуров комнат (:1475/:3875) вне диффа — +AC8 закрыт чтением `git diff origin/dev...HEAD -- src/` (единственный файл — +`wall-thickness.ts`, только 4 названных хунка). + +## Как проверялось — таблица гейтов + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | зелёный | +| unit | `npm test` | **1 из 1310 красный** (см. Finding H1/M1) — не относится к продуктовому коду #309 | +| build + копии бандла | `npm run build && cmp dist/... custom_components/.../houseplan-card.js` | зелёный, байты совпадают | +| bundle:sync (3-я копия для смоков) | `npm run bundle:sync` | зелёный | +| docs fingerprint | `node scripts/check-docs.mjs` | зелёный (7 файлов, 10 ссылок) | +| выбор смоков по диффу | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | см. ниже, вывод приложен | +| смок (прямое совпадение) | `node demo/smoke_junction_holes.mjs` | OK | +| смок (прямое совпадение) | `node demo/smoke_real_plan_masonry.mjs` | OK (лично прогнан ревьюером — не назван в хендоффе автора) | +| смок (зарегистрированная связь) | `node demo/smoke_multiwall_junction.mjs` | OK | +| смок (зарегистрированная связь) | `node demo/smoke_junction_patch_resilience.mjs` | OK | +| golden | `npm run golden:verify` | **129/129 зелёный**, включая 3 новые `junction-309-{step,spike,hump}-dark` | +| perf (риск §3.10.4 спеки) | `node demo/smoke_render_perf.mjs` | OK | +| мутанты #309 (AC7 a/b/c/d) | `node scripts/mutation-gate.mjs --id=<4 id>` | **все 4 поймали регрессию** (см. ниже) | +| мутант #302, адаптированный диффом | `node scripts/mutation-gate.mjs --id=junction-fans-disabled` | поймал | +| model-invariants | не прогонялся | diff не создаёт/не переносит записи толщины, не меняет `walls`/`layout`/`marker.space`/`open_spans` — чистая рендер-геометрия узлов, критерий §8 не наступает | +| backend pytest | не прогонялся | диффом не тронут `custom_components/**/*.py` | + +Вывод `smoke-select.mjs` (приложен полностью): + +``` +Изменено файлов src/**: 1 · символов проекта на изменённых строках: 7 +Матрица: 190 смоков · порог «широкого» символа: больше 38 смоков + +Прямое совпадение (2): + demo/smoke_junction_holes.mjs ← MITRE_LIMIT, buildMultiWallNodeMap, junctionNodeGeometry + demo/smoke_real_plan_masonry.mjs ← MITRE_LIMIT + +Зарегистрированная связь (3): + demo/smoke_junction_patch_resilience.mjs ← buildMultiWallNodeMap + demo/smoke_multiwall_junction.mjs ← buildMultiWallNodeMap + demo/smoke_multiwall_strip_containment.mjs ← buildMultiWallNodeMap +``` + +Решение по каждой строке: оба прямых совпадения прогнаны лично (второе — +`smoke_real_plan_masonry.mjs` — автор его в хендоффе не называл, прогнал сам, +результат OK). Из трёх «зарегистрированных связей» прогнаны две +(`junction_patch_resilience`, `multiwall_junction`); `multiwall_strip_containment` +не прогонялась лично, но автор заявляет её прогон OK в хендоффе, а смысловая +связь (только `buildMultiWallNodeMap`, без AC на плотных T-стыках #275) — +не первоочередная для риска этой задачи (плотная выборка защищённых полос +#275/#279, не тронутых диффом). Остальные смоки из хендоффа автора +(`wall_junctions`, `free_walls`, `wall_thickness×3`, `active_chain_ink`, +`hatch_density`) не перепроверялись — не входят ни в прямые, ни в +зарегистрированные совпадения инструмента, доверие заявлению автора. + +Мутационный гейт AC7, вывод для каждого: + +``` +visual-mitre-limit-back-to-4: поймано 1 из 1 +chamfer-disabled-full-mitre: поймано 1 из 1 +pair-patches-at-multiwall-nodes: поймано 1 из 1 +chamfer-chord-instead-of-perpendicular: поймано 1 из 1 +junction-fans-disabled (адаптирован #302): поймано 1 из 1 +``` + +## Находки + +### Medium (в скоупе) — M1: `npm test` красный на проверяемом SHA + +**Файл:** `test/golden-matrix.test.mjs:370` + +**Воспроизведение:** `npm test` → `not ok 378 - sun-ray golden requires +browser-painted light from a state-only sun entity` → +`AssertionError: 46 !== 45` на строке `assert.equal(GOLDEN_MATRIX_VERSION, 45)`. +Детерминированно (перепроверено дважды, включая изолированный прогон +`node --test test/golden-matrix.test.mjs`). + +**Причина:** коммит `101cf709` корректно поднимает +`GOLDEN_MATRIX_VERSION` 45 → 46 в `demo/golden/matrix.mjs` (добавлены 3 новые +сцены `junction-309-*`, что по правилу `demo/golden/README.md` требует +бампа) — но не обновляет соседний, не тронутый диффом +`test/golden-matrix.test.mjs`, который хардкодит ожидаемую версию `45` в +отдельном тесте. Файл `test/golden-matrix.test.mjs` в диффе не фигурирует +вовсе (не входит в список изменённых файлов коммита). + +**Внешнее подтверждение:** CI на точном SHA `e85bae70` (run +[32886114649](https://github.com/Matysh/houseplan-card/actions/runs/32886114649)) +— job `frontend` **failure**, из-за чего `golden`/`smoke`/`performance_smoke` +в этом прогоне пропущены (`skipped`) каскадом. Это значит, что ни один из +golden/смок-результатов, заявленных в хендоффе автора, не подтверждён CI на +этом SHA — только моим локальным прогоном после `npm run bundle:sync`. + +**Важный контекст:** на удалённой ветке уже существует более новый коммит +`6c1534f1` (`test: pin the golden matrix guard to version 46 (#309)`, +запушен в 21:55, через 5 минут после `e85bae70`), который правит именно эту +строку 45→46 и заявляет «Полный npm test — 1309/0». Он **не входит в +диапазон `origin/dev...HEAD` этого ревью** (HEAD зафиксирован на `e85bae70` +на момент старта ревью) и потому формально не проверялся — CI на его SHA +(run 32886626656) зелёный, но это вне scope этого захода. Фикс уже есть, +но не в проверяемом дереве — по букве процесса это правка, ожидающая +собственного цикла ревью, а не «уже закрыто». + +**Не является дефектом фичи #309**: все 6 юнитов «issue 309», все 4 новых +мутанта, `junctionContractHoles` и 129/129 golden (включая три новые сцены) +зелёные и на этом SHA, и после `bundle:sync`. Ломается только не относящаяся +к геометрии проверка версии матрицы в другом файле. + +**Почему Medium, а не High:** дефект не про продукт (пользователь его не +видит, AC #309 не задет), это красный обязательный гейт `npm test`, +входящий в «всегда»-набор §8 PROCESS.md — так что не блокирующий сам факт +не в счёт, а исправление обязательно в этой же задаче. + +### Low (снята с записью) — AC5 доказана не буквально как записано в ТЗ + +**Файл:** `docs/specs/309-junction-visual-limit.md:100`, AC5. + +AC5 требует: «`junctionContractHoles` пуст на всех сценах сета и на **полном +экспорте отчёта (13 комнат/24 перегородки)**». Фикстура, добавленная этой +задачей (`test/fixtures/309-junction-teeth.json`), — не полный экспорт, а +минимальный вырез трёх узлов (0 комнат, 9 перегородок, ровно как записано в +плане тестов §6: «минимальный вырез трёх узлов»). Существующий смок-детектор +`smoke_junction_holes.mjs`, который AC5 называет вторым доказательством, +использует фикстуру `302-junction-artifacts.json` (3 комнаты / 7 стен) — тоже +не «13/24». + +Ни в диффе, ни в репозитории нет фикстуры с 13 комнатами/24 перегородками; +полный экспорт владельца, судя по всему, не коммитился (аналогично практике +`scripts/wall-strip-containment.mjs` — внешние бэкапы не копируются в Git), +но ТЗ не пометило это как «принято предположительно», и хендофф не назвал +расхождение явно. + +**Снимаю без правки** (решение ревьюера, PROCESS §2.7): риск дыр на большом +реальном плане закрыт равноценным, хоть и не тем же самым тестом — лично +прогнанным `demo/smoke_real_plan_masonry.mjs`, который сэмплирует контур +кладки на двух реальных многокомнатных фикстурах (`real-plan-first-floor`, +`real-plan-second-floor`) после этого диффа: `gapCount: 0` на обоих этажах +(45757 и 8048 сэмплов). Это не то же самое, что владельческий экспорт из +issue, но по порядку величины (реальный многокомнатный план, а не +изолированный узел) закрывает тот же риск, который AC5 называет явно. Автору +на будущее — либо приложить факт эквивалентности прямо в ТЗ/хендофф, либо +всё же закоммитить полный экспорт как ещё один `test/fixtures/309-*.json`. + +## Проверка AC + +| AC | Доказательство | Вердикт | +|---|---|---| +| AC1 Шип 10+20 | юнит `issue 309 the acute 10/20 pair keeps its tail within the visual limit` (падает при мутантах a/b) + golden `junction-309-spike-dark` | доказан | +| AC2 Горб 3×50 | юнит `issue 309 the 3×50 node fans stay within the visual limit` + golden `junction-309-hump-dark` | доказан | +| AC3 Ступенька 15/15/30/30 | юнит `issue 309 the mixed-thickness cross has no step in a foreign quadrant` (точечный пробник в чужом квадранте) + golden `junction-309-step-dark` | доказан | +| AC4 Прямые углы неизменны | юнит `issue 309 a square corner of equal depths keeps its byte-identical mitre` + математически: 1.41h < 1.5h ⇒ та же ветка кода, что и раньше (1.41h < 4h) — проверено чтением | доказан | +| AC5 Без дыр | юнит на фикстуре-вырезе + смок; **не на «полном экспорте 13/24»** — см. Low выше, снята с эквивалентной заменой | доказан частично, снято с записью | +| AC6 Golden весь сет зелёный | `npm run golden:verify` → 129/129, 3 новые сцены + 16 старых `junction-*` + `junction-owner-repro-dark` байтово прежние (подтверждено прогоном, не по заявлению) | доказан | +| AC7 Мутанты a/b/c/d | 4 прогона `mutation-gate.mjs --id=...`, все поймали регрессию (см. таблицу гейтов) | доказан | +| AC8 (не-скоуп #249) | `git diff origin/dev...HEAD -- src/` — единственный файл `wall-thickness.ts`, 4 хунка, ни один не касается `MULTI_WALL_JOIN_LIMIT`/`multiWallBevelCutsAt`/room-contour mitre; существующие тесты/сцены paper envelope не в диффе → зелёные по построению | доказан диффом | + +## Разобрано по коду (сверх AC) + +- Математическое доказательство, что `chamferApex` никогда не проваливается + (не возвращает `null` из-за вырождения) при вызове из обоих сайтов: и в + `linearWallJoinPatches`, и в `junctionNodeGeometry` точки `pA`/`pB`(`EA`/`EB`) + всегда находятся на расстоянии ровно `halfDepth` от узла по построению + (`node + n·halfDepth`), значит их проекция на ось «узел→вершина» строго + меньше `visual = 1.5·max(hA,hB)`, а вызывающая сторона уже гарантирует + `d > visual` — отсюда `t ∈ (0,1)` строго для обеих граней. Разрыв + «фаска не считается → сырой mitre остаётся» (который был бы дефектом в + ветке `junctionNodeGeometry`, где фолбэк — именно неусечённый `mitre`, а + не безопасный бевел) математически недостижим для текущих вызывающих + конструкций. Проверено чтением, не только тестами. +- Скип узлов ≥3 лучей (`coveredByFans`) корректно не затрагивает 2-лучевые + узлы: `buildMultiWallNodeMap` регистрирует только узлы с достаточным + числом лучей, что подтверждено и напрямую тестом на остром 10/20-стыке + (patches.length ≥ 1 при 2 сегментах). +- `out.fans` в `junctionNodeGeometry` содержит все три случая (mitre, + reflex-хорда, бевel) — значит, полный скип пар в `linearWallJoinPatches` + для узлов ≥3 лучей не оставляет непокрытых секторов по построению функции, + не только по факту прохождения детектора дыр. +- Отклонение от буквы ТЗ §3.2 («фаска перпендикулярна биссектрисе сектора»): + код режет перпендикулярно направлению «узел→вершина mitre», а не + геометрической биссектрисе лучей A/B. Для равных полутолщин это одно и то + же (симметрия), для разных (случай AC1, 10 vs 20 см) — строго говоря, + разные направления. Технический выбор не оспариваю: canonical-документ + (`WALL-THICKNESS.md:206-219`) описывает его именно так («perpendicular to + the apex direction»), реализация и документация согласованы, юниты + проверяют радиальное ограничение (AC1/AC2 формулируют именно его — «вылет + ≤ 1.5h»), а не направление разреза, и golden-скрин зафиксировал + фактическую форму. Технические споры автор/ревьюер решает вердикт, а не + владелец — вердикт: не возражаю, это законная детализация «как сделано». + +## Не проверялось + +- `python -m pytest tests_backend` — диффом не тронут `custom_components/**/*.py`. +- `node scripts/model-invariants.mjs --config ...` — диффом не создаются и не + переносятся записи толщины/`walls`/`layout`/`marker.space`/`open_spans`; + критерий §8 «трогает геометрию или ссылки на неё» для сохраняемой модели не + наступает (это рендер-геометрия существующих узлов, не новая структура + данных). +- Смоки `smoke_wall_junctions.mjs`, `smoke_free_walls.mjs`, + `smoke_wall_thickness*.mjs`, `smoke_active_chain_ink.mjs`, + `smoke_hatch_density.mjs`, названные автором в хендоффе, — не + переисполнялись лично; смок-селектор их не выделил ни прямым, ни + зарегистрированным совпадением, доверие заявлению автора без дублирования. +- `demo/smoke_multiwall_strip_containment.mjs` — не переисполнялся лично (см. + раздел «Как проверялось»), доверие заявлению автора. +- Полный владельческий экспорт (13 комнат/24 перегородки) — не существует в + репозитории как фикстура; см. Low-находку. + +## Вывод + +Геометрия исправления корректна и хорошо защищена: 6 юнитов + 4 новых +мутанта (все ловят регрессию) + 129/129 golden + прямые/зарегистрированные +смоки зелёные, включая лично прогнанный `smoke_real_plan_masonry.mjs` на двух +реальных многокомнатных планах. AC1–AC4, AC6–AC8 доказаны без оговорок; AC5 +доказана частично и снята с запиской (Low). Единственная блокирующая для +зелёного вердикта находка — M1: обязательный гейт `npm test` красный на +проверяемом SHA `e85bae70` из-за забытого обновления хардкода +`GOLDEN_MATRIX_VERSION` в стороннем тестовом файле; подтверждено и локально, +и упавшим job `frontend` в CI на этом же SHA. Фикс уже есть на ветке +(`6c1534f1`), но вне диапазона этого захода — по процессу его нужно внести +(или подтвердить) в рамках возврата на правки и пройти код-ревью заново на +приведённом дереве.