mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
@@ -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). Возвращаю жёлтым для приведения
|
||||
спеки в соответствие с уже написанной верной докой подсистемы либо для
|
||||
явного решения владельца по трим-исключению.
|
||||
Reference in New Issue
Block a user