diff --git a/docs/reviews/CODE-REVIEW-302-r2.md b/docs/reviews/CODE-REVIEW-302-r2.md new file mode 100644 index 00000000..c8971f18 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-302-r2.md @@ -0,0 +1,246 @@ +# CODE-REVIEW-302-r2 + +Issue: #302 · Этап: code · Заход: r2 · блокирующих циклов израсходовано 1/4 до +этого захода + +## Скоуп раунда + +r1 (SHA `fa1112e7`) закончился жёлтым: High 0, Medium 2 (M1 — доку +`docs/WALL-THICKNESS.md` не переписали под решение №5; M2 — гвард мутанта +`junction-fans-disabled` инфраструктурно сломан, «поймано» не было проверено +исполнением). Автор хендоффнул исправление на вершине `a5ea578` +(комментарий issue 2026-08-25T14:00:05Z), затем конвейер перед ревью +ребейзнул ветку на свежий `dev` — вершина стала `42e02396`. + +**Проверка ребейза (§7.2).** `git diff a5ea578..42e02396 --stat` даёт ровно +один файл: `docs/reviews/SPEC-REVIEW-304-r1.md` (+128, чужой issue). Дерево +кода/тестов/доки #302 идентично байт-в-байт. Это не «другой код» — единственный +привнесённый ребейзом файл не пересекается ни с одним путём этой задачи и не +проходит фильтр `frontend`-триггера CI (подтверждено логом job `changes`, +CI-прогон 32856428594). Разбор веду **по дельте** r1→r2: `git diff +fa1112e7..HEAD` — четыре файла по существу (`docs/WALL-THICKNESS.md`, +`scripts/mutation-gate.mjs`, `src/wall-thickness.ts`, плюс пересборка бандла и +`docs/images/screenshots.json`), plus два review-документа. Расширил разбор за +пределы формальной дельты в одном месте (см. находку M3) — при чтении +`src/wall-thickness.ts` полностью для оценки M2/Low вокруг него нашёл +расхождение AC6 с реализацией; код этого места не менялся между r1 и r2, но +находка целиком в скоупе задачи, а не в скоупе только этой правки, поэтому +привожу её отдельно, а не молчу. + +## Как проверялось + +Гейты «всегда» (исполнением, на HEAD `42e02396`): + +| гейт | команда | результат | +|---|---|---| +| типы | `npx tsc --noEmit` | чисто | +| юниты | `npm test` | 1303 pass / 1 skip / 0 fail | +| сборка | `npm run build` + `diff dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | rollup ok; `git status` после билда чист (свежий `dist` побайтно совпал с закоммиченным) — три копии (пересобранная, `dist/`, `custom_components/`) идентичны | +| доки | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» | +| инварианты | `npm run invariants -- --config test/fixtures/302-junction-artifacts.json` | «ссылки разрешимы, записи толщины находятся» | + +Мутанты, названные M2/Low (исполнением, штатным харнессом +`scripts/mutation-gate.mjs --id=...`): + +| id | результат | +|---|---| +| `junction-fans-disabled` | поймано 1 из 1 | +| `junction-fan-ignores-thick-length` | поймано 1 из 1 | +| `junction-reflex-outer-mitre-missing` | поймано 1 из 1 | +| `junction-fan-limit-back-to-249` | поймано 1 из 1 | +| `junction-detector-blind` | поймано 1 из 1 | +| `junction-pieces-unbounded` | поймано 1 из 1 | +| `partition-merge-ignores-junction` | поймано 1 из 1 | +| `junction-checks-room-vertices-only` | поймано 1 из 1 | + +`junction-supports-not-restored` и `multi-wall-paper-full-origin-cut` из +реестра удалены этим диффом — проверено, что их find-паттерны (обращения к +удалённым `paperWithNodeCorners`/`[...corners.supports, ...corners.fans]`) +больше не существуют в файле: мутанты стали неприменимы к текущему коду, снятие +не притворное. + +Смоки/golden: `smoke-select.mjs` для дельты `fa1112e7..HEAD` даёт прямые +совпадения `smoke_junction_holes`, `smoke_decor`, `smoke_grid_scale_invariance`, +`smoke_real_plan_masonry`, `smoke_space_scale_defaults` и пять +«зарегистрированных связей» (`junction_patch_resilience`, `multiwall_junction`, +`multiwall_strip_containment`, `resize_pointer_real_plan`, +`resize_wall_thickness`). Локально браузерные проверки (`golden:verify`, +`demo/smoke_*.mjs`) **не выполнились**: `page.route` не перехватывает +динамический `import()` в этом окружении — `Failed to fetch dynamically +imported module: http://demo.local/assets/houseplan-card.js`. Проверил, что +это ограничение песочницы, а не регрессия: та же ошибка воспроизводится +байт-в-байт на чистом `origin/dev` (worktree, символическая ссылка на +`node_modules`) с давно существующим `demo/smoke_align_guides.mjs`, который +дифф не касается. + +Вместо локального прогона поднял CI-прогоны по `gh run view`: +- `32855773018` (headSha `30b4bbf6`, кодово идентичен `a5ea578`/`42e02396` по + `src/**`/`test/**`/`scripts/**` — единственная разница дальше по цепочке это + фикс отпечатка скриншотов) — `frontend`, `golden`, все три шарда `smoke`, + `performance_smoke` зелёные; красным был только `docs` (протухший отпечаток + скриншотов — это и есть то, что чинит следующий коммит `42e02396`, дока + «refresh the screenshot fingerprint»); +- `32856428594` (headSha `a5ea578`) — `docs` зелёный (фингерпринт поправлен), + `frontend`/`golden`/`smoke` пропущены самим `changes`-джобом: между + `30b4bbf6` и `a5ea578` изменился только `docs/images/screenshots.json`, под + фронтенд-триггер не попадает; +- `32856829923` (headSha `42e02396`, текущий HEAD) — красный один + `process-gate` на несуществующем в раннере before-SHA (артефакт force-push + после ребейза на dev, автор описывает его же во всех трёх последних + хендоффах); `frontend`/`docs`/`provenance` зелёные, `golden`/`smoke` + пропущены — тем же основанием (после `a5ea578` изменился только чужой + `SPEC-REVIEW-304-r1.md`). + +Собранные вместе, эти три прогона покрывают полный набор гейтов на +кодово-идентичном дереве, включая golden и все смоки — считаю это equivalent +доказательству «оно работает», раз локальный браузерный прогон недоступен. + +## Закрытие раунда r1 + +| находка r1 | чем закрыта | где видно | +|---|---|---| +| **M1** — `docs/WALL-THICKNESS.md` §3 описывал отменённое решение «фаска #249 сохраняется» | Абзац переписан под решение №5 целиком: полный mitre в секторе, `bevelMultiWallBody` — только адресный трим для #271, `bevelMultiWallPaper` убрана | `docs/WALL-THICKNESS.md:185-206` (коммит `72a5992f`); прочитано и сверено построчно с текущим кодом `junctionNodeGeometry`/`bevelMultiWallBody` — соответствует | +| **M2** — гвард `junction-fans-disabled` был `node demo/smoke_junction_holes.mjs` без сборки `test-build`, падал `ERR_MODULE_NOT_FOUND` на чистом дереве | Guard переведён на `npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs && node --test --test-name-pattern="issue 302" ...`; дополнительно (второй виток, коммит `af33c49e`) сама проверка переведена с самоссылочного контракт-проб-детектора на независимый юнит формы `issue 302 a T node covers every sector with a fan or a mitre` (`test/wall-thickness.test.mjs:2930-2940`) | `scripts/mutation-gate.mjs:2174-2190`; исполнено лично — `node scripts/mutation-gate.mjs --id=junction-fans-disabled` → «поймано 1 из 1» | +| Low — `bevelMultiWallPaper` мёртвый код | Функция удалена целиком (`src/wall-thickness.ts`, было `:2955-2994`); следом измерением снята и re-union саппорт-квадов в теле (`31ba14e0`), плюс два осиротевших мутанта из реестра | `git diff fa1112e7..HEAD -- src/wall-thickness.ts` — оба блока отсутствуют; live-подтверждение — тест `issue 302 the owner repro is hole-free end to end` (`test/wall-thickness.test.mjs:3026-3054`) гоняет упрощённый пайплайн на реальной фикстуре репро и не находит дыр | + +## Находки + +### M3 (Medium, в скоупе задачи) — AC6 не выполнен: узловая механика не «безоперационна по difference», спека это не признаёт + +**Файл:** `src/wall-thickness.ts:3535-3555` (вызов), `src/wall-thickness.ts:2856-2953` +(тело `bevelMultiWallBody`); спека `docs/specs/302-junction-node-material.md:64-73` +(§4, решение №5), `:129-131` (§8.2), `:196-198` (AC6). + +**Что заявлено.** Решение владельца №5 (принято в день ребейза, зафиксировано +в спеке и в теле коммита `79e86b0a`): «фаска #249 демонтируется **целиком**... +слой `bevelMultiWallBody`/`bevelMultiWallPaper` **удаляется**». §8.2 повторяет +это как формальный контракт: «**В узловой механике нет ни одной операции +`difference`** (вычитающий слой фаски демонтирован): дыра между полосами +невозможна **по построению**». AC6 — то же самое как проверяемый критерий. + +**Что в коде.** `bevelMultiWallBody` (старая реализация фаски #249, с +`difference()` на строках 2889, 2898, 2922, 2924) не удалена: она вызывается +на подмножестве узлов, отфильтрованном `needsTrim` (короткий толстый саппорт, +#271) — `src/wall-thickness.ts:3542-3554`. Это не мёртвый код: комментарий на +`fixture #197` (`test/wall-thickness.test.mjs:2236-2240`) прямо говорит, что +тест полагается на этот трим («#271 removes only the area that the old +node-wide 8H rectangles invented»), и площадь фикстуры (`closeTo(..., +124535.20808099362, ...)`) посчитана с его участием. + +**Почему это не придирка к формулировке.** Весь смысл переработки #302 — +уйти от вычитающих ремонтных слоёв, потому что именно они были источником +рецидивирующих дыр (это буквально сюжет issue: 29 коммитов «point fixes», +которые не сходились). «Дыра невозможна по построению» — это утверждение +именно про отсутствие `difference`; для узлов, где реально срабатывает +`needsTrim`, инвариант «hole-free» держится не построением, а корректностью +унаследованного вычитающего кода — той же категории кода, что и раньше ломался. +Спека и коммит основного решения (`79e86b0a`) заявляют «удаляется целиком», но +имплементация — тот же коммит! — сохраняет узкий трим и честно объясняет +зачем в комментарии кода. То есть автор знал про исключение в момент +написания контрактной формулировки, но не отразил его ни в §4/§8.2/AC6 спеки. +`docs/WALL-THICKNESS.md` (живой документ подсистемы) это исключение описывает +верно — именно потому что M1 в этом самом раунде его туда вписал; спека +(контракт, по которому пишутся AC) осталась с абсолютной, невыполненной +формулировкой. + +Отдельно: ни один этап ревью до сих пор AC6 не подтверждал. Решение №5 +поменяло текст спеки уже ПОСЛЕ обоих раундов SPEC-REVIEW (они зелёные на +версии «фаска #249 сохраняется» — SHA `3b19111a`); AC6 в его текущей редакции +никогда не проходил ни одного ревью. В вердикте r1 (комментарий issue +2026-08-25T13:29:36Z) явно перечислены как проверенные AC1, AC2, AC4, AC5, +AC7, AC9 — AC6 в списке демонстративно нет, то есть r1 либо не проверял его, +либо проверял и не сообщил результат. + +**Воспроизведение.** `grep -n "difference(" src/wall-thickness.ts` в диапазоне +2856-2953 (`bevelMultiWallBody`) — 4 вызова; трассировка вызова от +`wallBodiesGeometry` (`:3542-3554`) до этой функции безусловна при +`trimNodes.length > 0`. `needsTrim` реально истинен минимум для узла фикстуры +#197 (по прямому указанию комментария теста, задача #271). + +**Серьёзность и что чинить.** Не блокирует релиз функционально — вся +визуальная/golden матрица подтверждает, что заявленный владельцем результат +(без вырезов/рожков, полный mitre) достигнут, и трим — узкий, адресный, +дисциплинированно закомментированный код, а не регресс. Но AC — формальный +критерий приёмки этой задачи, и он не выполнен в буквальном прочтении. Дёшево +чинится без правки кода: привести спеку в соответствие с уже написанной (в +этом же раунде) правдой `docs/WALL-THICKNESS.md` — явно назвать исключение +#271 в §4.5/§8.2 и переформулировать AC6 («узловая механика аддитивна для +всех узлов, кроме адресного #271-трима, унаследованного из #249 и суженного +до вырожденного случая»), либо, если владелец сочтёт исключение +неприемлемым, вернуть задачу на технический разбор устранения самого трима. +Это решение продуктовое/архитектурное, не моё — фиксирую находку, не +предписываю какой из двух путей выбрать. + +## Что проверено и корректно + +- M1, M2, Low из r1 закрыты по существу, не только по заявлению — см. таблицу + выше; для M2 лично прогнал мутант и получил «поймано 1 из 1», а не поверил + тексту коммита (текст коммита `72a5992f` сам признаёт, что автор один раз + уже ошибочно заявил «проверено исполнением», не проверив). +- Удаление `bevelMultiWallPaper`/`paperWithNodeCorners`/двух мутантов — + полное, без осиротевших ссылок; подтверждено `grep` по обеим удалённым + сигнатурам и живым тестом на реальном пайплайне (`the owner repro is + hole-free end to end`). +- `docs/WALL-THICKNESS.md` после M1 фактически точен для того путя, который + описывает M3 (адресный трим назван прямо) — то есть подсистемная дока не + расходится с кодом, расходится только сама спека issue. +- Трейлеры всех четырёх коммитов дельты (`72a5992f`, `af33c49e`, `31ba14e0`, + `42e02396`) содержат `Issue: #302` и `User-Visible: no` — корректно: эти + четыре коммита не меняют видимое поведение относительно того, что уже было + выпущено с `User-Visible: yes` в `79e86b0a`/`a5d30467` (там же в том же + коммите обновлены оба CHANGELOG — проверено `git show 79e86b0a -- + docs/CHANGELOG.md docs/CHANGELOG.ru.md`). +- tsc/test/build/check-docs/invariants — все чисто, гейты не помечены как + условно пройденные. +- Мутационное покрытие узловой механики (8 мутантов, включая переписанный + M2) — самопроверено исполнением, не только чтением реестра. + +## Чего не проверял и почему + +- **`npm run golden:verify` и `demo/smoke_*.mjs` — не выполнил локально.** + Окружение ревью не даёт Chromium перехватить динамический `import()` + (воспроизведено то же самое на чистом `origin/dev` с чужим смоком — + ограничение песочницы, не регрессия дельты). Компенсировано тремя + CI-прогонами на кодово-идентичном дереве (см. «Как проверялось»): golden + 126/126 и все три шарда смоков зелёные на `30b4bbf6`; `docs`-джоб зелёный на + `a5ea578`/`42e02396` после фикса отпечатка. +- **Полный golden-набор и весь `demo/smoke_*.mjs` (не только выбранные + smoke-select) не гонял вручную** — избыточно: `smoke-select.mjs` дал узкий + список, а CI уже прогнал полный набор (все три шарда — это весь + `demo/smoke_*.mjs`, не подмножество) на этом дереве. +- **Продуктовую визуальную приёмку 16 новых junction-сцен не пересматривал + глазами** — это решение владельца (комментарии issue 2026-08-25 11:40 и + 12:42), не предмет код-ревью; полагаюсь на golden pixel-diff. +- **AC3, AC8** (пиксельное совпадение непричастных сцен, перф-бюджет) — + унаследованы из r1/из CI (`performance_smoke` зелёный в `32855773018`), не + передельфрено отдельно: дельта r1→r2 не трогает рендер-путь и перф-профиль. + +## Унаследовано из r1 + +Документ r1: `docs/reviews/CODE-REVIEW-302-r1.md` (комментарий issue +2026-08-25T13:29:36Z), проверен на SHA `fa1112e7`. + +Принято без повторной проверки, так как дельта r1→r2 не касается этого кода: +- геометрия веера/mitre/bevel-хорды/рефлекса (`junctionNodeGeometry`) — + «прочитана построчно и соответствует контракту §8» (кроме уточнения по + AC6 выше — то новая находка этого раунда, не подтверждение r1); +- AC1, AC2, AC4, AC5, AC7, AC9 — сочтены выполненными и проверенными + исполнением в r1; код, который их обеспечивает, не менялся между `fa1112e7` + и `HEAD` (сверено `git diff fa1112e7..HEAD -- src/wall-thickness.ts` — + единственные правки вне M1/M2/Low перечислены в разделе «Закрытие раунда + r1» и не затрагивают mitre/bevel/reflex ветки); +- продуктовая рамка (SCOPE.md, J1/J2) и полнота DoR спеки — не пересматривал, + дельта не продуктовая; +- решение владельца №5 (полный mitre, отказ от фаски #249 как видимого + эффекта) как таковое не оспариваю — это принятое продуктовое решение; + находка M3 — про честность формальной формулировки AC6, а не про само + решение. + +## Вывод + +Мultimedia M1/M2/Low закрыты добросовестно и проверены исполнением, а не на +слово. Обнаружена одна новая находка в скоупе (M3): AC6 этой же задачи не +выполнен буквально, и это никогда не проходило ревью (решение №5 переписало +спеку уже после двух зелёных SPEC-REVIEW). Возвращаю жёлтым для приведения +спеки в соответствие с уже написанной верной докой подсистемы либо для +явного решения владельца по трим-исключению.