diff --git a/docs/reviews/SPEC-REVIEW-249-r1.md b/docs/reviews/SPEC-REVIEW-249-r1.md new file mode 100644 index 00000000..87c8e25c --- /dev/null +++ b/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**