diff --git a/docs/reviews/CODE-REVIEW-249-r1.md b/docs/reviews/CODE-REVIEW-249-r1.md new file mode 100644 index 00000000..90cd54e6 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-249-r1.md @@ -0,0 +1,257 @@ +# CODE-REVIEW-249-r1 + +- Issue: [#249](https://github.com/Matysh/houseplan-card/issues/249) — ограниченная геометрия узла из трёх и более стен +- Этап: code (PROCESS.md §2.7) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 (этот заход не расходует бюджет: вердикт красный) +- Коммит на ревью: `062a98a1d841a5de633392357dcf0d4264ed3620` ("fix: bound multi-wall junction bevels") +- ТЗ: `docs/specs/249-multiwall-junction-bevel.md`, редакция r2 (SPEC-REVIEW-249-r2, зелёный) +- Материал: `git diff origin/dev...HEAD`, полный (не delta-review — это первый заход этапа code) + +## Скоуп проверки + +Прочитаны: `docs/SCOPE.md` (J1), `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, ТЗ +`docs/specs/249-multiwall-junction-bevel.md` (все 13 разделов, включая §9 AC1–AC7), +тело issue #249 и все комментарии (аналитика, продуктовые Q&A, ТЗ, оба раунда +spec-ревью, отчёт автора о реализации). Просмотрен весь diff +`src/wall-thickness.ts` (416 добавленных/изменённых строк), новый fixture, +`test/wall-thickness.test.mjs` (переписанные и новые тесты), +`demo/smoke_multiwall_junction.mjs`, изменения golden matrix/harness/test, +обновления `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, `docs/TESTING.md`, +`docs/CHANGELOG.md`/`.ru.md`. + +## Как проверялось + +Гейты реализации (соразмерны диапазону diff — трогает `src/**`, требует +check-docs; смоки выбраны по AC4 и по инструменту `smoke-select.mjs`, а не +прогнаны полным набором): + +| Гейт | Результат | Прогнал | +|---|---|---| +| `npx tsc --noEmit` | зелёный | да | +| `npm test` | 1117 passed / 0 failed / 0 skipped (автор заявлял 1116+1 skip — итог тестов совпадает, расхождение по skip не расследовано, не блокирует) | да | +| `npm run build` | зелёный | да | +| SHA-256 трёх копий бандла | совпадают: `9e2c89fd...eefe05ec7` | да, сверил вручную | +| `node scripts/check-docs.mjs` | зелёный (7 файлов, 10 внешних ссылок) | да, обязателен — diff трогает `src/**` | +| `node demo/smoke_multiwall_junction.mjs` | зелёный, 15/15 | да — прямой AC4 гейт | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | нашёл 1 прямое совпадение: `smoke_decor_layer_order.mjs` (символ `roomPoly`) | да | +| `node demo/smoke_decor_layer_order.mjs` | зелёный, 26/26 | да | +| `node demo/smoke_split_corner_wall.mjs` | зелёный, 14/14 | да — тема пересекается с новым junction-кодом | +| `node demo/smoke_junction_patch_resilience.mjs` | зелёный, 14/14 | да | +| `node demo/smoke_wall_junctions.mjs` | зелёный, 12/12 | да | + +**Не прогонялось** (предрелизные гейты по PROCESS.md §8, не гейт ревью): +`npm run golden:verify` (полный), `npm run golden:accept`, performance-профили, +полный `smokes:select`/167 смоков, backend/HA-harness (Python не тронут). +Golden baseline не принимался — соответствует отчёту автора. + +Все заявленные автором числа (SHA-256 бандла, счётчики smoke JSON, состав +`smoke-select`) подтверждены самостоятельным прогоном, а не переписаны со слов +автора. + +Дополнительно я самостоятельно воспроизвёл геометрию (см. находку H1) через +compiled `test-build/wall-thickness.js`, вызывая экспортированные +`roomWallProfile`/`insetContour`/`outsetContour`/`buildMultiWallNodeMap` +напрямую, и сравнил результат с тем же вызовом на `origin/dev` (через +временный `git worktree`) — оба прогона выполнены, артефакты и worktree +удалены до завершения ревью, в репозитории не оставлено файлов. + +## Находки + +### H1 (High, блокирует). `innerContourForRoom` (clean floor) получает +геометрически неверную bevel-точку на узлах с несимметричной толщиной +сходящихся стен — floor выходит за пределы контура здания + +**Точка отказа:** `src/wall-thickness.ts:1066-1069` (fallback-ветка +`insetContour`/`outsetContour`, идентичный код в обеих функциях) — +существующий (не новый) код, который для чрезмерной митры кладёт две точки +`poly[i] + normal·offset` независимо для каждого ребра, без проверки, что +результат остаётся внутри многоугольника. Раньше эта ветка почти не +срабатывала для узлов 3+ (порог был `MITRE_LIMIT×maxO`, то есть до `4×` +толщины), поэтому дефект был латентным. #249 делает порог `1.25×H` — то есть +именно тем механизмом, который теперь регулярно уводит обычные T/L-стыки +перегородки с внешней стеной в эту ветку, — и подставляет туда несимметричные +толщины (перегородка много толще внешней стены), при которых точка одного +ребра уходит по нормали на всю величину offset этого ребра, что при остром угле +между рёбрами пробивает противоположную сторону многоугольника насквword. + +**Воспроизведение (числами, не «на глаз»):** тот же `cornerSplitFixture`, что +уже используется в тестах `test/wall-thickness.test.mjs` (`outerCm: 15`, +`dividerCm: 100`, path по умолчанию — то есть комбинация, реально +прогоняемая гейтом «corner Split preserves facade... every positive-thickness +3-ray matrix», строка `outerCm=15, dividerCm=100`): + +``` +node — узел (100, 100), угол между наружным ребром и перегородкой ≈ 26.6° +H = 41.6667 (полутолщина перегородки), R = 1.25×H = 52.0833 + +roomWallProfile('source').poly = [[100,100],[900,100],[900,500]] +roomWallProfile('source').offsets = [6.25, 6.25, 41.6667] + +insetContour(poly, offsets, multiWallNodes) → + [118.63389981249824, 62.7322003750035], ← точка floor ВНЕ здания + [100, 106.25], + [893.75, 106.25], + [893.75, 450.2902504687544] +``` + +Комната "source" — часть прямоугольника `y ∈ [100,700]`. Точка +`[118.63, 62.73]` имеет `y = 62.73 < 100`, то есть лежит **выше верхнего +края здания**, вне контура вообще (не просто вне комнаты — вне здания +целиком). Расстояние точки от узла — ровно `41.6667` (халф-толщина +перегородки), то есть формально `≤ R` (52.08), поэтому проверка §7.2.4 +(«join-вершина не дальше `R + epsilon`») этот дефект не ловит: контракт +ограничивает только *расстояние*, а не сторону многоугольника, а фактическая +точка получена смещением от узла по нормали чужого (перегородочного) ребра на +всю его полутолщину — без проверки, что это не пробивает соседнее ребро. + +Union-уровневый `multiWallBevelTriangles`/`bevelMultiWallBody` (новый код +#249, добавляющий вырезание лишнего клина из финального wall body) эту же +несимметрию на этом узле не ломает так явно, потому что `wallBodiesGeometry` +дополнительно пересекает итоговое тело с `exterior.centre` +(`src/wall-thickness.ts:2364-ish`, `if (body && exterior) body = +intersection(body, exterior.centre)`), что подрезает вылезшую геометрию +обратно к контуру здания. У `innerContourForRoom` (clean floor) такой +защитной пересечки нет вовсе — функция отдаёт результат `insetContour` +напрямую (после диффа: `src/wall-thickness.ts` в районе +`innerContourForRoom`), поэтому дефект долетает до потребителя без всякой +подрезки. + +**Проверка «было/стало»:** тот же вызов на `origin/dev` (до #249, через +временный `git worktree`, тот же `cornerSplitFixture`, идентичный код helper'ов) +даёт `insetContour` без всплеска — расхождение строгого инварианта +`floor_union == original.poly − wall_body` составляет `≈ 6.5e-11` (шум +плавающей точки, тест на dev проходит с допуском `1e-7`). На коммите #249 то +же расхождение — `≈ 1686.24` (на 10 порядков больше допуска), причём вся +дельта — это именно тот вырвавшийся за пределы здания клин. + +**Почему это не поймано авторскими гейтами:** этот же fixture +(`outerCm=15`, `dividerCm=100`) используется в тесте +`test/wall-thickness.test.mjs` — было +`'corner Split clean floors are exactly the room union minus canonical +walls'` со строгим `closeTo(geometryDifferenceArea(actual, expected), 0, +1e-7)` в обоих направлениях. Автор **заменил** этот тест на +`'corner Split clean-floor contours use the same bounded bevel endpoints'` +(новое имя), где вместо точного геометрического равенства теперь проверяется +только «каждая bevel-точка присутствует где-то среди вершин floor-контура» +(`floors.some((floor) => floor.some((point) => distance < 1e-7))`) — эта +проверка тривиально проходит, даже когда floor-контур неверно вышел за +пределы здания, поскольку сама вырвавшаяся точка — это ровно та «bevel-точка», +которую тест ищет. Ни `demo/smoke_multiwall_junction.mjs` +(`cleanFloorConsumerIsPresent` проверяет только наличие path, не его форму), +ни новый golden-сценарий (`fill_mode: 'none'` — floor не рисуется вовсе) +дефект не покрывают. AC4 ТЗ прямо называет floor/room fills/room hover +каноническим потребителем той же исправленной геометрии (§7.4) — это +нарушено. + +**Почему это Blocking, а не косметика:** это ровно тот же класс дефекта, для +которого написано ТЗ (клин, вылезающий за пределы стены), только +переехавший с внешнего контура кладки на внутренний контур пола, причём для +совершенно реалистичной конфигурации — перегородка заметно толще наружной +стены, встречающая её под острым/умеренным углом. Инструмент, которым это +создаётся («Split» комнаты по диагонали с последующей установкой толщины +перегородки), — штатный редакторский путь, не синтетика теста. + +**Что нужно автору:** переоценить fallback-ветку `insetContour`/ +`outsetContour` (строки 1066–1069) для случаев, когда единичное смещение по +нормали одного ребра на всю величину его offset выходит за пределы, +образуемые соседним ребром — либо ограничивать длину смещения проекцией на +соседнее ребро/дистанцией до узла, либо (как минимум) вернуть +`innerContourForRoom` защитное пересечение с исходным полигоном комнаты, +аналогичное тому, что `wallBodiesGeometry` уже делает для wall body. Отдельно +нужно вернуть строгую (не ослабленную) проверку инварианта +`floor_union == room_union − canonical_wall_body`, которая существовала до +этого коммита — она и обнаружила бы дефект сама. + +--- + +### M1 (Medium, в скоупе — чинится в этом же цикле). AC2 не покрыт буквально +заявленным кейсом «три стены 15/50/70 см» (три РАЗНЫЕ толщины у трёх лучей) + +Спецификация (§9, AC2) требует минимум четыре сценария, включая «трёх стен +15/50/70 см» — то есть узел, где ВСЕ ТРИ луча имеют разную толщину (без +повторов). Матрица в `test/wall-thickness.test.mjs` +(`'issue #249 node classification is order, direction and scale +independent'`) покрывает: 3 равных луча, 3 луча с halves `[7,5,5]` +(**только два разных значения** — 7 и 5,5), 4 равных луча, 4 луча +`[2,5,3,7]` (все разные). Ни один 3-лучевой случай с тремя различными +попарными halfDepth не тестируется явно; сам fixture AC1 тоже +50/50/70 см (две толщины совпадают). Кейс `[7,5,5]` покрывает оба +направления одной несимметричной пары через обход по кругу, но не покрывает +тройку из трёх взаимно разных величин (нет пары «средний-большой» без +повторов), которую владелец в issue привёл как конкретный числовой пример +(«например 15/50/70»). + +Учитывая находку H1 (именно несимметрия толщин на многолучевом узле — источник +дефекта), это не формальная придирка: тест с тремя различными halfDepth, +особенно при остром угле, с высокой вероятностью тоже поймал бы H1 или его +аналог. Чинится точечно — один дополнительный кейс в существующем +`cases`-массиве с тремя различными halves, без новых продуктовых вопросов. + +## Что проверено и корректно + +- **AC1** (экспортный fixture #249, узел 50/50/70): unit + `'issue #249 bounds the exported three-wall junction with straight + bevels'` — воспроизведён и зелёный; `H = 4.8611`, все join-вершины `≤ + 1.25×H + epsilon`, один связный компонент, повторный расчёт даёт тот же + результат, вход не мутируется (deep-equal до/после). Для ЭТОГО конкретного + узла (толщины 50/50/70, угол не острый) floor-консьюмер H1 не проявляет — + проверил отдельно (`innerContourForRoom` на этом fixture даёт точки на + разумном расстоянии 3.5–4.9 от узла, без выхода за пределы), дефект + специфичен для более несимметричных/острых конфигураций. +- **AC3** (двухлучевые узлы не меняются): существующий 2-лучевой набор + тестов не тронут (кроме одного, №197-фикстура, у которого расширенный + fixture содержит собственный узел степени 3+ — это отдельный, задокументированный + в тесте, легитимный сдвиг площади, не 2-лучевой регресс). Отдельный + тест `twoRay` подтверждает `insetContour`/`outsetContour` с пустой + multi-wall картой идентичны вызову без карты. +- **AC4** (общий body для всех поверхностей): `demo/smoke_multiwall_junction.mjs` + зелёный по всем 15 полям — Plan/View/kiosk/Static/hidden-Iso совпадают + байт-в-байт по пути, light barriers используют ту же masonry, HA-tick и + смена темы не перестраивают topology (кэш переиспользуется). +- **AC5** (golden фиксирует видимый результат): новый сценарий + `multiwall-junction-bevel-view-dark` с semantic assertions (узел заполнен, + отброшенный клин пуст, 3 луча) — не может пройти на пустом/неверном кадре; + версия матрицы поднята (38→39); `test/golden-matrix.test.mjs` проверяет + форму сценария. +- **AC6** (данные не мутируются): подтверждено deep-equality до/после в новом + unit-тесте и в `'corner Split rendering does not materialize or mutate + saved geometry'`. +- **AC7** (гейты реализации): все четыре зелёные, включая build/typecheck — + перепроверено самостоятельно, не только со слов автора. +- **Документация**: `docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, + `docs/TESTING.md` обновлены по существу (не формально) и соответствуют + фактическому коду; `docs/CHANGELOG.md`/`.ru.md` оба обновлены в этом же + коммите при `User-Visible: yes`; трейлеры `Issue: #249` и + `User-Visible: yes` на месте. +- **Union-уровневая коррекция** (`multiWallBevelTriangles`/ + `bevelMultiWallBody`) корректна для симметричных/сбалансированных узлов + (проверено чтением кода и подтверждено зелёными таргетированными unit- и + smoke-тестами) — проблема локализована именно в старой fallback-ветке + `insetContour`/`outsetContour`, не в новой union-логике. + +## Чего не проверял + +- Полный `npm run golden:verify`/`golden:accept`, полный набор из 167 смоков, + performance-профили — предрелизные гейты по PROCESS.md §8, не обязаны быть + прогнаны на этом заходе; риск R1 самой спеки («широкий blast radius для + T-стыков») явно требует именно полного golden-прогона перед бетой, и находка + H1 показывает, что этот прогон обязателен и по причине, шире описанной в + спеке — не только визуальный дрейф, но и потенциальный явный дефект. + Golden-сценарий #249 не поймает H1 — там `fill_mode: 'none'`. + Не устанавливал/не запускал этот набор сам. +- Не искал системно другие узлы (существующие golden fixtures, реальные + сохранённые планы) с похожей несимметрией толщин/острым углом, где H1 мог + бы проявиться иначе (например, в `openingWallIndex`/tunnel-геометрии рядом с + таким узлом) — за рамками того, что доказывает конкретный repro. +- Backend/Python — не тронут диффом, HA-harness не требовался и не запускал. + +## Резюме + +Реализация в целом добросовестно покрывает контракт ТЗ (единая node map, +union-уровневая коррекция клина, документация, golden/smoke на заявленном +экспортном сценарии) — но именно в том месте, где спецификация явно +предупреждала о риске (§11 R2: «фаска может создать щель или ложную +кладку… визуально малый дефект»), обнаружен подтверждённый воспроизведением +дефект (H1), который автор не поймал, потому что ослабил единственный тест, +способный его обнаружить. Это блокирующая находка — заход не может быть +зелёным.