docs: review document for #249

Issue: #249
User-Visible: no

docs/reviews/SPEC-REVIEW-249-r1.md

@@ -0,0 +1,261 @@
+# SPEC-REVIEW-249-r1
+
+- Issue: [#249](https://github.com/Matysh/houseplan-card/issues/249) — «Стык трёх стен разной толщины рисуется шипом наружу вместо соединённого узла»
+- ТЗ: `docs/specs/249-multiwall-junction-bevel.md`, ветка `issue/249-multiwall-junction-bevel`, коммит `8d2e00bbbfbb9151ca2079eaf6e51f916cb59eff`
+- Этап: spec (PROCESS.md §2.4), заход r1, трек обычный (не `small`)
+- Вердикт: **жёлтый**
+
+## Скоуп ревью
+
+Первый заход — разбор полный. Разделов «Закрытие раунда» и «Унаследовано»
+нет: PROCESS.md §2.10 применяется начиная со второго цикла, а вердикт
+предыдущего раунда по этой задаче отсутствует (в комментариях issue есть
+только этапы аналитики и продуктовых Q&A, ревью ТЗ ещё не проводилось).
+
+Проверено по порядку из инструкции:
+
+1. `docs/SCOPE.md` — сценарий закрывает J1 («Show the whole home... live
+   spatial overview»): дефект постоянно виден в View/kiosk, где View — сам
+   продукт для двух из трёх персон. Не найдено конфликтов с `TOUCH-SUPPORT.md`
+   — правка не создаёт и не меняет интеракций.
+2. `AGENTS.md`, `PROCESS.md` §2.4, §2.5, §7.1, §4, §12 — обязательные разделы
+   ТЗ, критерии DoR, формат вердикта, лимит циклов.
+3. Тело issue #249 и все пять комментариев: воспроизведение и корневая
+   причина автором ТЗ, продуктовые вопросы Q1/Q2 с предложенными default,
+   решения владельца, объявление о готовом ТЗ.
+4. `docs/USER-GUIDE.ru.md` — термины «стык», «mitre/bevel» (строка 487)
+   уже существуют и согласуются со словарём ТЗ («митра», «фаска»); новой
+   пользовательской терминологии эта задача не вводит (чистый bugfix
+   геометрии, не новая UX-возможность).
+5. Канонический документ подсистемы — `docs/WALL-THICKNESS.md` целиком
+   (277 строк): модель `walls`, рост ±½, единый structural pass, `MITRE_LIMIT`,
+   virtual-junction patches и их fail-isolation (#197), тестовый раздел §8.
+
+## Как проверялось
+
+Ревью состязательное: каждое техническое утверждение ТЗ сверялось с кодом на
+`HEAD`, а не принималось на слово автора.
+
+- `src/wall-thickness.ts` — подтверждены все процитированные в issue и ТЗ
+  места: `export const MITRE_LIMIT = 4` (строка 46); `insetContour()` со
+  сравнением `dist <= MITRE_LIMIT * maxO` (строка 1030, функция начинается на
+  967); `outsetContour()` с идентичным сравнением (строка 2238, функция
+  начинается на 2183); митра-лимит для independent-junction patches (строка
+  1853); лимит на строке 701. Все совпадают с номерами строк, названными в
+  теле issue и ТЗ §2.
+- Строка 626: `same.halfDepth = Math.max(same.halfDepth, halfDepth)` —
+  подтверждает, что «совпадающие сонаправленные фрагменты берут максимальную
+  полутолщину» (ТЗ §6, `ray`) не изобретённое правило, а уже существующий в
+  коде паттерн дедупликации интервалов.
+- `src/wall-merge.ts` — `junctionAt()` (строка 101) и `spaceMergeGeometry()`
+  (строка 278) существуют и, как верно указано в ТЗ §2/§6, работают с
+  independent partitions, а не с чисто комнатными узлами — что и объясняет,
+  почему баг не ловится существующим junction-кодом.
+- Пересчитаны числа из ТЗ §2 самостоятельно: `cell_cm=30` даёт множитель
+  `wallCmToUnits`, при котором 50 см → 3.4722, 70 см → 4.8611 render units;
+  измеренная join-вершина `8.7312 / 4.8611 = 1.7958 ≈ 1.80×H`, что совпадает
+  с текстом ТЗ и подтверждением владельца в третьем комментарии issue. Числа
+  внутренне непротиворечивы и не выданы за факт без проверки.
+- `docs/WALL-THICKNESS.md` §3 «Body render»: «Mitre joins; bevel when the
+  mitre spike exceeds `MITRE_LIMIT × thickness`» и §8 (тестовый охват) —
+  подтверждают, что описанный в ТЗ переход mitre→bevel является
+  существующим контрактом для степени 2, который ТЗ §7.3 явно сохраняет
+  без изменений.
+- Сопоставлены заголовки шести последних полноформатных ТЗ (`248`, `244`,
+  `239`, `229`, `197`, `198`) — все имеют отдельные разделы «UX, i18n,
+  accessibility и touch», «План [реализации и] автотестов» и «Риски,
+  производительность и rollback/security». См. находки ниже.
+
+## Разделы §7.1 — проверка полноты
+
+| Требуемый раздел | В ТЗ #249 | Комментарий |
+|---|---|---|
+| Сценарий | §1 | ок |
+| Что человек увидит до/после | §3 | ок, размещён после «проблемы» — порядок не регламентирован |
+| Проблема | §2 | ок (воспроизведение + корневая причина) |
+| Скоуп и не-скоуп | §5 | ок |
+| Контракт поведения | §7 | ок по объёму, но см. Medium-находку M2 (внутреннее противоречие §7.2.1/7.2.2) |
+| UX | §8 (одна строка: «Нет новых i18n, настроек, действий, жестов либо различий mouse/touch») | контента по существу достаточно (правка не создаёт новых интеракций), но отдельного раздела нет — см. Low ниже |
+| Модель данных и миграция | §8 (первые пункты) | ок |
+| i18n | §8 (та же строка) | ок, новых строк нет |
+| AC1…ACn с доказательством | §9 | ок, см. таблицу ниже |
+| План автотестов | распределён по «Доказательство:» в каждом AC | по содержанию покрыто, но не собран отдельным разделом — см. Low |
+| **Риски** | **отсутствует** | **см. Medium-находку M1** |
+| Откат | §11 | ок |
+| Release-артефакты | §10 | ок |
+
+## AC — однозначность и способ доказательства
+
+| AC | Однозначен? | Доказательство названо и выполнимо? |
+|---|---|---|
+| AC1 | Да — конкретный узел, конкретные числа `H=4.8611`, `R=1.25×H`, инварианты связности | unit + новый fixture `test/fixtures/249-multiwall-junction.json` — файл ещё не существует (ожидаемо для ТЗ), путь и формат согласованы с существующим паттерном фикстур в `test/` |
+| AC2 | Да по формуле, но см. M2 — сама граничная формула (§7.2.1 vs §7.2.2) внутренне противоречива в полосе `(R, R+epsilon]`, что делает пограничный случай матрицы недоказуемым однозначно | table-driven unit, численный bound + сравнение нормализованной геометрии — метод выполним для всех точек вне спорной полосы |
+| AC3 | Да — «те же точки/paths», «без обновления двухлучевых expected values» — сильная, проверяемая гарантия регрессии | unit regression на существующих `insetContour()`/`outsetContour()` тестах |
+| AC4 | Да — все поверхности через один smoke с path/geometry assertions и cache parity | `demo/smoke_multiwall_junction.mjs` — новый файл, обязателен к запуску локально перед `S7-code-review` (`AGENTS.md` «Гейты») |
+| AC5 | Да — golden с semantic assertions на число лучей и габарит, что явно исключает пустой/неверно кадрированный PNG (защита от регресса #234) | `demo/golden/matrix.mjs` + `test/golden-matrix.test.mjs`, приёмка эталона только по `npm run golden:accept -- --reviewed` на полном Linux CI |
+| AC6 | Да, стандартная формулировка deep-equality | unit + существующий зелёный набор без изменений |
+| AC7 | Да, состав гейтов реализации соразмерен задаче (§8 PROCESS.md): typecheck/test/build/targeted smoke здесь, golden/полный smoke/perf — предрелизно | команды названы точно |
+
+Ни один AC не содержит домысла о поведении, которого нет ни в одном
+документе: контракт §7 — прямое продолжение уже существующего
+`MITRE_LIMIT`/bevel-механизма из `docs/WALL-THICKNESS.md` §3, распространённое
+на узлы степени 3+ по явному решению владельца (Q1/Q2 в комментариях issue),
+а не изобретённое автором. Открытых продуктовых вопросов в тексте не
+осталось — оба вопроса, заданные владельцу, получили ответ и дословно перенесены в §4 ТЗ.
+
+## Находки
+
+Блокирующих (High) находок нет.
+
+### Medium — в скоупе задачи, чинится в этом же ТЗ
+
+**M1. Обязательный раздел «Риски» отсутствует полностью.**
+
+`PROCESS.md` §7.1 перечисляет «риски» как отдельный обязательный раздел ТЗ, а
+DoR-чеклист §2.5 требует «риски перечислены» как блокирующий пункт готовности
+к разработке. В ТЗ #249 такого раздела нет вовсе (проверено `grep -ni
+"риск"` по всему файлу — единственное упоминание это унаследованная из
+аналитики метка «Сложность/риск: 5/10 и 6/10» в шапке документа, без единого
+слова разбора). Для сравнения — все шесть последних полноформатных ТЗ (`239`,
+`244`, `248`, `229`, `197`, `198`) содержат отдельный раздел с конкретными
+пунктами и мерами снижения.
+
+Здесь эта пустота предметна, а не формальна: сама задача в issue названа
+владельцем «риск 6/10» именно потому, что правка меняет каноническую
+геометрию узла для **любого** узла из 3+ разных физических лучей — а такие
+узлы (обычные T-стыки внутренних перегородок с наружной стеной) есть почти в
+каждом сохранённом плане, не только в приложенном экспорте. Правка сужает
+допустимый габарит join-вершины с `4×H` (старый `MITRE_LIMIT`) до `1.25×H`
+для всех узлов степени 3+ — то есть потенциально меняет вид существующих,
+сегодня визуально корректных T-стыков, чья текущая митра лежит в интервале
+`(1.25×H, 4×H]`. AC2 проверяет только новые синтетические сценарии, AC6 явно
+исключает лишь independent-partition junctions и двухлучевые углы — ни один
+AC не требует прогона существующего golden-набора ради проверки именно этого
+побочного эффекта на реальных (не связанных с #249) планах. Требование это
+частично закрывается штатным пре-релизным гейтом (`golden:verify` на полном
+наборе перед бетой, §8 PROCESS.md), но ТЗ обязано назвать этот риск явно:
+«неизменность T-стыков неявно полагается на пре-релизный гейт» — это не то
+же самое, что «названо в разделе рисков».
+
+**Почему это делает ТЗ невыполнимым без правки:** без явного раздела рисков
+DoR-чеклист (§2.5) формально не проходит («риски перечислены» — пункт
+блокирующий), а автор реализации не получит явного указания, что перед
+переводом в `S8-merged` нужно специально просмотреть diff полного golden-
+прогона на предмет затронутых T-стыков за пределами фикстуры #249, а не
+просто принять новый эталон.
+
+**Предлагаемое закрытие:** добавить раздел «Риски», перенести и раскрыть уже
+названные в аналитике риски («меняется каноническая геометрия узла; ошибка
+способна дать щель, лишнюю кладку либо изменить обычные двухлучевые углы»),
+явно указать risk «широкий blast radius: любой существующий T-стык степени 3+
+может визуально сдвинуться» и назвать митигацию (полный `golden:verify` перед
+бетой обязателен, а не опционален, именно из-за этого риска).
+
+**M2. Внутреннее противоречие критерия mitre/bevel в §7.2, пункты 1 и 2.**
+
+Текст:
+
+> 1. существующее пересечение смещённых граней остаётся митрой, если его
+>    расстояние от node `<= R + epsilon`;
+> 2. если пересечения нет, оно не конечное либо дальше `R`, join содержит две
+>    штатные offset-точки соседних граней — прямую фаску;
+
+Для расстояния `d`, такого что `R < d <= R + epsilon`, оба условия истинны
+одновременно: по правилу 1 вершина «остаётся митрой» (`d <= R + epsilon`), по
+правилу 2 она же должна стать фаской (`дальше R`). Это не гипотетическая
+придирка к формулировке — это прямое противоречие в самом контракте
+поведения (§7 — обязательный раздел, самый важный для реализуемости),
+из-за которого ни автор кода, ни автор unit-теста не могут детерминированно
+решить, какой веткой идти для точки в этой полосе шириной `epsilon`. AC1 и
+AC2 наследуют ту же двусмысленность, поскольку оба формулируют допуск как
+`1.25 × H + epsilon`, то есть используют границу правила 1, но не оговаривают,
+что при пересечении именно в этой полосе достаточно **любого** из двух
+результатов (что было бы законным способом снять противоречие, если это
+осознанный допуск на точность).
+
+Практическое влияние ограничено полосой шириной `epsilon` (масштаб
+машинной точности геометрии, `openEps(pitch, coordScale)`), то есть вряд ли
+проявится на конкретном экспорте владельца — поэтому не блокирует, но делает
+геометрический контракт не полностью однозначным, что прямо противоречит
+задаче ревьюера «найти, где ТЗ не выполнимо или не проверяемо».
+
+**Предлагаемое закрытие:** унифицировать порог — либо оба правила используют
+`R` (эпсилон остаётся только допуском сравнения чисел с плавающей точкой, а
+не расширением зоны «остаётся митрой»), либо явно указать, что при `d` в
+полосе `(R, R + epsilon]` оба исхода (митра или фаска) считаются корректными
+и не различаются ни одним AC/тестом.
+
+### Low — не блокирует, снимается ревьюером с записью
+
+1. **Раздел «UX, i18n, accessibility и touch» и «План автотестов» не
+   выделены отдельными заголовками**, в отличие от шести последних
+   полноформатных ТЗ (`239`, `244`, `248`, `229`, `197`, `198`), где это
+   устойчивый, повторяющийся шаблон. Контент по существу присутствует
+   (распределён по §8 и по полю «Доказательство:» каждого AC) и ничего не
+   пропускает по содержанию — в отличие от M1, здесь нет содержательной
+   пустоты, только структурное расхождение с шаблоном соседних задач.
+   Снимаю без требования правки: не создаёт риска для DoR и не влияет на
+   проверяемость AC. Автору стоит учесть при следующем ТЗ для единообразия
+   документов в `docs/specs/`.
+
+## Что проверено и корректно
+
+- Корневая причина (§2 ТЗ) подтверждена цитированием реального кода на
+  `HEAD`: `MITRE_LIMIT = 4`, независимые mitre-вычисления двух комнат в
+  `insetContour()`/`outsetContour()`, отсутствие покрытия со стороны
+  `junctionAt()`/`spaceMergeGeometry()` — не заявлена на веру.
+- Численные значения (`H = 4.8611`, `R = 6.076...`, измеренный спайк `8.7312`,
+  `1.80×H`) внутренне согласованы и совпадают с независимым пересчётом.
+- Продуктовые решения владельца (Q1 fallback = прямая фаска без округления;
+  Q2 лимит `1.25 × max half-depth` для узлов 3+ лучей, двухлучевые узлы без
+  изменений) перенесены в §4 ТЗ дословно, без искажений и без добавления
+  новых, не согласованных с владельцем решений.
+- Scope/не-scope (§5) корректно исключает миграцию, изменение толщины стен,
+  редакторские инструменты, округлённые joins и не трогает
+  `linearWallJoinPatches()` — не расширяет и не сужает баг-скоуп в
+  рефакторинг.
+- AC1–AC7 в целом именуют реальный, а не гипотетический тестовый
+  инструментарий: `test/wall-thickness.test.mjs`, `demo/golden/matrix.mjs`,
+  `test/golden-matrix.test.mjs` — все три файла существуют в текущем дереве
+  проекта и являются подходящим местом для новых кейсов.
+- AC5 защищён от пустого/неверно кадрированного PNG semantic-assertions —
+  прямая мера против того же класса дефекта, что уже стоил регресса на
+  #234 (`docs`-джоб покраснел из-за пропущенной пересъёмки, здесь риск иного
+  рода — «зелёный, но бессодержательный» golden — предупреждён явно).
+- Термины §6 ТЗ («ray», «node», «join vertex», дедупликация shared-интервала
+  между двумя комнатами) обоснованы существующим кодом (строка 626 —
+  `Math.max` при слиянии дублей), а не изобретены заново.
+- i18n/touch/данные (§8): корректно константируют отсутствие изменений —
+  правка не создаёт новых строк, настроек, миграций или touch-специфичного
+  поведения; согласуется с характером задачи (bugfix геометрии рендера).
+
+## Чего не проверял
+
+- Не запускал никаких гейтов (`typecheck`/`test`/`build`) — на этапе
+  spec-review предмет проверки текст ТЗ, а класс A файлов в этом коммите не
+  менялся (дифф — только `docs/specs/249-multiwall-junction-bevel.md`).
+- Не исполнял и не мог исполнить AC1–AC5 — упомянутые в них новые файлы
+  (`test/fixtures/249-multiwall-junction.json`,
+  `demo/smoke_multiwall_junction.mjs`, golden-сценарий узла) ещё не написаны;
+  это ожидаемо для стадии ТЗ и будет предметом код-ревью.
+- Не проверял степень визуального совпадения нового «1.25×H» лимита с уже
+  существующими golden-эталонами, где встречаются T-стыки степени 3 —
+  именно это отсутствие и составляет находку M1: полный `golden:verify`
+  относится к предрелизному гейту, а не к этапу ТЗ, но должен быть назван
+  как явный риск в тексте самого ТЗ.
+- Не пересчитывал вручную геометрию для матрицы AC2 (15/50/70 см, 4 луча) —
+  формула `R = 1.25 × max(halfDepth)` детерминирована и проверяема
+  тривиально на этапе код-ревью через unit-тест.
+
+## Итог
+
+ТЗ описывает реальный, подтверждённый по коду дефект и переносит явные
+продуктовые решения владельца без искажений и без домыслов. Блокирующих
+находок нет. Две находки Medium в скоупе задачи: полностью отсутствующий
+обязательный раздел «Риски» (M1) и внутреннее противоречие порога
+mitre/bevel в самом важном разделе — контракте поведения (M2). Обе чинятся
+без переписывания ТЗ — точечным дополнением одного раздела и унификацией
+одного порога, поэтому исправление не требует нового цикла продуктовых
+вопросов владельцу.
+
+**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2**