diff --git a/docs/reviews/SPEC-REVIEW-288-r1.md b/docs/reviews/SPEC-REVIEW-288-r1.md new file mode 100644 index 00000000..788d3637 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-288-r1.md @@ -0,0 +1,196 @@ +# SPEC-REVIEW-288-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/288 +- **Этап:** spec (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (правило #227 — + зелёный вердикт бюджет не тратит, но этот вердикт не зелёный) +- **Документ ТЗ:** `docs/specs/288-bounded-multiwall-corridor.md` +- **SHA на момент ревью:** `cb0f9e7351e919701666dfd05b53f0e218e672a5` + (коммит `docs: specify bounded multiwall corridor`) +- **Первый заход** — раздел «объём по дельте» (§2.10) не применяется, разбор + полный. + +## Скоуп ревью + +Проверено: тело issue #288 и оба комментария (аналитика + «ТЗ готово»), +`docs/specs/288-bounded-multiwall-corridor.md` целиком, `docs/SCOPE.md`, +`docs/WALL-THICKNESS.md` (канонический документ подсистемы), фрагменты +`src/wall-thickness.ts` (константы `MITRE_LIMIT`/`MULTI_WALL_JOIN_LIMIT`, +типы `MultiWallNodeRay`/`MultiWallNodeRaySupport`, функции +`multiWallBevelCutsAt`, `multiWallRayStripGeometry`, `bevelMultiWallBody`), +`demo/smoke_real_plan_masonry.mjs`, `test/fixtures/real-plan-second-floor.json` +(структура), `scripts/model-invariants.mjs` (наличие `--config`), и для +сравнения структуры — специфи́кации `docs/specs/275-multiwall-strip-containment.md` +и `docs/specs/278-wall-union-isolation.md` (та же подсистема, сопоставимый +риск/сложность — использованы как прецедент требуемых разделов). + +Как проверялось: ревью текстовое — сверка формулировок ТЗ с телом issue +(числа/причина совпадают буквально), с идентификаторами в реальном коде +(`MITRE_LIMIT`, `node.halfDepth`, `MultiWallNodeRay.supports` — все существуют +и используются именно так, как описывает контракт §3.1), с формулировками +`docs/WALL-THICKNESS.md` (ссылки на #249/#261/#271/#272/#275/#278/#279 в AC4 +и в тексте документа совпадают с уже задокументированным поведением, не +изобретены), и структурное — сверка присутствия обязательных разделов §7.1 +с прецедентными спеками той же подсистемы. Продуктового кода в диапазоне нет +(diff ограничен `docs/specs/**`, класс C) — гейты §8 к этапу spec не относятся +и не запускались; это гейты этапа code (§2.7). + +## Находки + +### Medium (в скоупе задачи — чинится в этом же ТЗ) + +**M1. Отсутствуют обязательные разделы «Риски» и «Откат» (§7.1, DoR §2.5).** + +`docs/specs/288-bounded-multiwall-corridor.md` не содержит ни одного раздела +с анализом рисков, ни раздела откат/rollback. Единственное упоминание слова +«риск» — оценочная цифра в шапке (`риск 8/10`), без содержания. Проверено +`grep -n -i "риск\|rollback\|откат"` по всему файлу — единственное совпадение +строка 7. + +PROCESS.md §7.1 перечисляет обязательные разделы ТЗ явно: «…план автотестов · +риски · откат · release-артефакты» — оба отсутствующих раздела в списке есть. +DoR §2.5 повторяет то же как отдельные обязательные пункты очереди: «влияние +на производительность… названо», «**откат**: как выключить или вернуть +назад», «риски перечислены». Пока эти пункты не выполнены, статус не может +стать «Готово к разработке» буквально по тексту §2.5: «Если хоть один пункт +не выполнен — статус не «Готово к разработке», как бы ни хотелось начать». + +Это не абстрактная формальность именно для данной задачи: сам автор оценил +риск как 8/10 и сложность 7/10 (P1, обычный трек — не `small`), а фикс трогает +общий для всех T/L/multi-wall стыков код (`bevelMultiWallBody`, +`multiWallBevelCutsAt`), от которого зависят уже закрытые контракты #249, +#261, #271, #272, #275, #278, #279 — то есть реальный риск регрессии высок и +уже отражён самим объёмом AC4, но нигде не назван явно как риск с мерой +(«риск: регрессия одного из семи прежних контрактов → мера: AC4 перегоняет +весь их набор»). Откат для правки без миграции и без флага — скорее всего +«просто revert коммита», но это должно быть сказано, а не подразумеваться: +раздел 6 («Совместимость…») описывает только *отсутствие* новых +compatibility-полей, а не механику отката самой правки. + +Прецедент: спеки той же подсистемы и сопоставимого риска — `#275` +(`docs/specs/275-multiwall-strip-containment.md`, разделы «11. Риски и меры» и +«12. Rollback») и `#278` (`docs/specs/278-wall-union-isolation.md`, разделы +«12.1. Риски и меры» и «17. Release-артефакты и rollback») — оба раздела +присутствуют. #288 того же класса дефекта и не легче ни по одной из этих +двух метрик, но раздела не имеет. + +**Чем закрывается:** добавить в ТЗ раздел «Риски и меры» (минимум: риск +регрессии multi-wall контрактов #249/#261/#271/#272/#275/#278/#279 → мера +AC4; риск undershooting/overshooting отсечения у соседней стены → мера AC3 + +AC7 mutation) и раздел «Откат» (одна фраза: чистый revert коммита, миграции и +флага не требуется, поскольку модель данных не меняется). Это не требует +пересмотра геометрического контракта — правка текстовая. + +### Low + +**L1. «Связано» в шапке не включает #279, хотя AC4 явно на него ссылается.** + +Шапка документа (строка 11) перечисляет «#249, #261, #271, #272, #275, #278, +#284–#286», но AC4 (строка 127) прямо требует «near-orthogonal #279 остаётся +защищённым». #279 — тот же класс контрактов, задействован в проверке +регрессии, и разумно ожидать его в списке связанных issue. Не блокирует +проверяемость ни одного AC и не влияет на выполнимость — чисто +трассируемость документа. Решение оставляю на автора: либо дополнить строку +11, либо снять как не влияющее (в этом случае — с записью в документе, а не +молчанием, по требованию §12 «Оставили в тексте ревью не считается +закрытием» — здесь это Low, а не Medium, поэтому достаточно осознанного +снятия). + +## Что проверено и корректно + +- **Раздел «Сценарий» и диагноз причины (п.1).** Числа `349/120/5` шагов, + толщины `30/30/30` см, соседняя стена `20` см, радиус + `MITRE_LIMIT × H = 4 × 15 = 60`, вырез `60 − 15 = 45` — воспроизводят тело + issue буквально, без искажений и без добавления недоказанных деталей. + Диагноз в issue помечен владельцем как «Подтверждённая причина» (совпадение + вычисленной величины с измеренной), так что повторение его в ТЗ как факта — + не догадка, выданная за решение, а корректное наследование уже + подтверждённого вывода. +- **Идентификаторы контракта существуют в коде.** `MITRE_LIMIT` (`src/wall-thickness.ts:78`), + `MultiWallNodeRay.supports`/`halfDepth` (там же, интерфейсы объявления и + использование в `multiWallRayStripGeometry`/`bevelMultiWallBody`) — контракт + §3.1 ссылается на реально существующие структуры, а не на вымышленные. +- **AC1/AC2 привязаны к существующему воспроизводимому инструменту.** + `demo/smoke_real_plan_masonry.mjs` уже в дереве (класс B, коммит + `c654e0ec`/`d5659478`), считает разрывы по каждому ребру каждой комнаты + (не только по четырём известным точкам), различает объявленный `open_span` + от дефекта. Требование AC1 «gapCount: 0, totalGapSteps: 0 плюс обновление + чисел `PLANS` в том же коммите» и AC2 «first-floor остаётся на нуле» — + однозначны и проверяются одной командой. +- **AC3 — table-driven unit, синтетическая, но осознанно упрощённая + конфигурация** (три луча `349/120/5` при `30/30/20` см вместо реальных + четырёх стен узла) корректна как проверка контракта §3.1 в отрыве от + конкретной топологии реального плана: контракт формулируется через + собственный half-depth соседнего ray, а не через воспроизведение точной + геометрии дома. AC1 остаётся источником истины для реального дефекта, + AC3 — независимая unit-гарантия того же правила на разных `cell_cm` и + порядках лучей. +- **AC4** — список регрессионных контрактов (#249 дискардед wedge, + #261 retained wedge, #271 finite rays, #272 enclosed holes, #275 protected + strips, #278 union failure isolation, #279 near-orthogonal) совпадает слово + в слово с формулировками §3 `docs/WALL-THICKNESS.md`, не выдуман. +- **AC7 (мутант)** — явно требует, чтобы порча корридора до зависимости + только от `MITRE_LIMIT × node.halfDepth` валила AC1 или AC3; это тест на + «умеет падать», а не декларация покрытия. +- **Раздел 4 «Scope»**, особенно «Не входит» (никакого переписывания + persisted-данных, никакого изменения глобальных констант, никакой новой + модели стен из ADR #282) корректно ограничивает скоуп и явно исключает + соседние задачи #289/#290, не позволяя дефекту раздуться в рефакторинг. +- **Раздел 8 «Порядок интеграции»** относительно ветки `#260` + (`issue/260-fixture-wall-keys`) корректно и без искажений повторяет + требование самого issue («влить до пересъёмки скриншотов») и явно + оговаривает, что #260 не входит в продуктовый скоуп #288 — не является + попыткой расширить скоуп чужой задачей. +- **«Принятые технические предположения» (п.9)** — все четыре пункта + действительно технические (верхняя граница ущерба, конкретный алгоритм + connector-а, объём фикстур, touch-политика), ни один не является + продуктовым вопросом, который следовало бы адресовать владельцу; блок + оформлен явно как предположение, что соответствует требованию §7.1 + («принято предположительно, поменять свободно»), а не выдаёт догадку за + факт. +- **Совместимость/touch/performance (п.6)** — «модель данных не меняется» + подтверждается отсутствием любых изменений схемы/`layout`/`Optimize` во + всём документе; touch-формулировка («View на touch и kiosk получает тот же + canonical result») соответствует терминологии `docs/TOUCH-SUPPORT.md` + (View — гарантированная поверхность, редактор — best effort), не изобретает + новый термин. +- **Продуктовых вопросов владельцу нет** — и это корректно: единственная + видимая пользователем перемена — «стены рисуются без разрыва», без нового + UX-контракта, без выбора между персонами. Открытых развилок, требующих + продуктового решения владельца, в тексте не найдено. + +## Чего не проверял + +- Не запускал `npx tsc --noEmit` / `npm test` / `npm run build` / + `node scripts/check-docs.mjs` — diff ограничен `docs/specs/288-*.md` + (класс C), продуктовый код и тесты не менялись; эти гейты относятся к + этапу code-review (§2.7) и будут обязательны там. +- Не выполнял `node demo/smoke_real_plan_masonry.mjs` живьём — на этапе spec + еще нет реализации; текущий (добавленный ранее, в другом issue) результат + `gapCount: 4, totalGapSteps: 181` уже зафиксирован в самом smoke-файле и + подтверждён текстом issue, повторный прогон здесь ничего нового не даёт. +- Не проверял, действительно ли предложенная в AC3 синтетическая + конфигурация (3 луча, включая «соседнюю» стену как один из лучей) технически + достаточна для воспроизведения именно того случая, где четвёртая + (не инцидентная узлу) стена теряет материал — если в реальности дефект + затрагивает стену, geometрически не входящую в `node.rays` того же узла, + синтетический repro AC3 может не поймать в точности этот путь кода. Это не + повод для находки: AC1 (реальный smoke) остаётся источником истины + независимо от того, что покажет AC3, и авторская реализация обязана + удовлетворить AC1 в любом случае. Точную топологию дефектного пути стоит + перепроверить на этапе code-review по фактическому диффу. +- Не проверял golden baseline и её текущее состояние (`artifacts/golden/`) — + AC6 описывает будущую работу (targeted golden для реального плана ещё не + существует), нечего сверять до реализации. + +## Вердикт + +High-находок нет. Одна Medium-находка (M1) — в скоупе задачи, не требует +отдельного issue (#202) и чинится добавлением двух коротких разделов в тот же +файл ТЗ. Одна Low-находка (L1) — на решение автора (дополнить или снять с +записью). + +``` +Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 → в задаче +Документ: docs/reviews/SPEC-REVIEW-288-r1.md +```