diff --git a/docs/reviews/SPEC-REVIEW-278-r1.md b/docs/reviews/SPEC-REVIEW-278-r1.md new file mode 100644 index 00000000..d53e228b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-278-r1.md @@ -0,0 +1,165 @@ +# SPEC-REVIEW — Issue #278 · заход r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/278 +- **ТЗ:** `docs/specs/278-wall-union-isolation.md` (SHA `d906e5d8`, ветка + `issue/278-wall-union-isolation`) +- **Трек:** обычный (small/trivial явно исключены автором) — файл ТЗ обязателен, + что соблюдено. +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4. + +## Скоуп ревью + +Ревью ТЗ по PROCESS.md §2.4/§7.1: выполнимость и проверяемость AC1–AC15, +отсутствие догадок, выданных за факт, полнота обязательных разделов, согласие +с `docs/SCOPE.md` и каноническими документами подсистемы (`docs/WALL-THICKNESS.md`), +непротиворечивость связанным issue (#141, #197, #199, #276, #277). + +Продуктовый код не менялся (`git diff --stat origin/dev...HEAD` — только +`docs/specs/278-wall-union-isolation.md` и запись в `docs/specs/README.md`), +поэтому гейты `tsc`/`test`/`build`/`check-docs` к этому заходу не применяются: +на этапе spec трогается только документ, дерево кода идентично `dev`. + +## Как проверялось + +1. Прочитан текст issue #278 и оба комментария автора (аналитика 10/9/7/9, + P1, уточнение доказательной базы после повторного запуска preflight #199 + на `payload.config`). +2. Прочитан `docs/SCOPE.md` — задача закрывает J1/J4/J6 (не даёт локальному + дефекту физической геометрии визуально стирать этаж; план должен + "keep the plan true"). +3. Прочитан `docs/WALL-THICKNESS.md` §9 (Independent partitions, drafts and + columns) — сверка терминологии `extraBodies`, `physicalBodySet()`, + joined/raw body set, Glow/sun/clean-floor consumers. +4. Прочитан код `src/wall-thickness.ts` (`wallBodiesGeometry`, + `wallBodiesUnionPath`, циклы `roomRings`/`wallEdgeBodies`/`extraBodies`, + строки 2743–2880) — проверка центрального технического утверждения ТЗ. +5. Проверены issue #197 (закрыт) и #199 (закрыт) — согласованность границ + "не дубликат". +6. Проверены #276 и #277 — оба сейчас также в `S4-spec-review` (не смержены), + что важно для оценки принятого предположения о порядке merge (§18.4). +7. Проверено существование файлов, названных в плане тестов: + `test/wall-thickness.test.mjs`, `test/plan-geometry-preflight.test.mjs`, + `scripts/model-invariants.mjs` — существуют; `demo/smoke_wall_union_isolation.mjs` + — не существует (ожидаемо, создаётся в разработке). +8. Проверено использование `evenodd` fill-rule в `houseplan-card.ts`/ + `space-render.ts`/`physical-geometry.ts` — утверждение §5.7 о риске + схлопывания coincident-компонента подтверждено существующим кодом и + существующим комментарием в `physical-geometry.ts` про запрет + конкатенации противоположно ориентированных rings в один evenodd path. +9. Сверены со смежными принятыми ТЗ той же подсистемы — + `docs/specs/253-resize-wall-thickness.md` и + `docs/specs/275-multiwall-strip-containment.md` — на состав обязательных + разделов по PROCESS.md §7.1. + +## Что проверено и корректно + +- **Технический диагноз не догадка, а прочитанный код.** Утверждение §2/§12.4 + "цикл `extraBodies` в `wallBodiesGeometry()` не имеет per-piece try/catch, + в отличие от `roomRings` и `wallEdgeBodies`, и исключение улетает во внешний + catch, обнуляя весь результат" — подтверждено построчно + (`src/wall-thickness.ts:2815-2823` и `:2830-2840` оба оборачивают тело цикла + в `try/catch`, `:2872-2875` — нет). Это именно то различие, которое делает + ТЗ проверяемым, а не спекулятивным. +- **`roomGeom` действительно исключает extras** (`:2851` вычисляется до + `for (const extra of extraBodies)` на `:2872`) — соответствует AC5 + ("room area не включает extras"). +- Границы с #197/#199 объяснены и согласуются с фактическим текстом закрытых + issue: #197 — единичный virtual-junction patch, здесь другой узел отказа; + #199 — preflight Optimize, здесь — barrier для обычных commit-путей, которых + у #199 не было. Автор дополнительно подтвердил во втором комментарии issue, + что #199 не регрессировал (`wall-null` при повторном прогоне на точном + `payload.config`), и перенёс эту находку в границу задачи (§3.5) — хорошая + практика: уточнение доказательной базы отражено в ТЗ, а не осталось только + в комментарии. +- Типизированный контракт §4 (`ok`/`degraded-extra`/`failed-core`/ + `not-applicable`) и разделение strict/render-safe режимов дают однозначный, + проверяемый словарь для AC1–AC3, AC6. +- Canonical-consumers claim (§6) подтверждён кодом: `houseplan-card.ts`, + `space-render.ts` и `plan-geometry-preflight.ts` — все три реальных + потребителя `wallBodiesGeometry`/`wallBodiesUnionPath` в `src/`. +- Явно помечены принятые технические предположения (§18) с правом ревьюера их + оспорить — соответствует PROCESS.md §7.1: имена classes/reasons, бюджет + bounded-toast, отсутствие отдельного repair UI, порядок merge #276→#277→#278. + Открытых продуктовых вопросов действительно нет ни одного размытого места, + которое требовало бы решения владельца, а не инженерного суждения. +- AC1–AC15 в целом привязаны к конкретному способу доказательства (unit, + property/permutation matrix, production-bundle smoke, mutation, benchmark) — + ни один AC не оставлен без названного способа проверки. +- Non-scope (§11) корректно выносит #276 (устранение конкретного redundant + partition) и #277 (safe Resize) за периметр, не пытаясь тихо взять на себя их + скоуп. + +## Находки + +### Medium (в скоупе, блокирует зелёный вердикт) + +**M1 — отсутствует обязательный раздел «Риски».** +PROCESS.md §7.1 перечисляет обязательные разделы ТЗ: «…критерии приёмки +AC1…ACn с указанием доказательства · план автотестов · **риски** · откат · +release-артефакты». В `docs/specs/278-wall-union-isolation.md` слово «риск» +встречается один раз — как число в общей оценке заголовка +(«риск 9/10», строка 7) — и это всё; отдельного раздела с анализом рисков и +мер нет вообще (структура документа: §1 Сценарий → …→ §13 Производительность → +§14 Touch → §15 AC → §16 План тестов → §17 Release-артефакты и rollback → §18 +Принятые предположения — раздела «Риски» между планом тестов и rollback, где +он стоит в соседних ТЗ этой же подсистемы, нет). + +Это не формальность: два родственных ТЗ той же геометрической подсистемы, +написанные тем же автором, содержат именно такой раздел — +`docs/specs/253-resize-wall-thickness.md` (§14 «Риски и защита») и +`docs/specs/275-multiwall-strip-containment.md` (§11 «Риски и меры»). #278 — +не менее рискованная задача (та же авторская оценка «риск 9/10», плюс явная +кросс-issue зависимость), и именно здесь отсутствие анализа заметно: + +- §18.4 фиксирует предположение о порядке merge #276→#277→#278, но на момент + этого ревью **#276 и #277 оба ещё в `S4-spec-review`**, то есть все три + задачи идут параллельно, а не последовательно, как предполагает пункт. + Что произойдёт, если #278 будет реализован и смержен раньше #277 (временный + adapter, который #278 должен удалить, ещё не существует), или #276 не будет + готов к моменту, когда #278 нужен для AC6 (barrier зелёный «после valid #276 + reconciliation») — не разобрано ни как риск, ни как assumption с планом на + случай отклонения от предполагаемого порядка. +- Риск деградации UX для реальных легаси-планов (после этой задачи план с + повреждённым extra навсегда останется в `degraded-extra` и будет виден как + изолированный компонент до ручного #276-ремонта) заявлен как продуктовый + контракт (§18.5), но не оценён как риск с мерой (например: сколько таких + планов может существовать в проде, что видит пользователь до вмешательства). +- Риск того, что «reusable strict geometry transaction barrier» (§7) станет + тем самым единственным источником правды, но при интеграции с уже + реализуемым #277 останется временный дублирующий adapter дольше одного + цикла — упомянут как admission (§18.4 «удаляет временный adapter»), но не + как риск с последствием, если удаление не произойдёт до релиза. + +**Почему Medium, а не High:** ни один AC не становится невыполнимым или +непроверяемым из-за этого пропуска — сам контракт §3–§9 самодостаточен и +проверяем без раздела «Риски». Это дефект полноты документа, а не дефект +проверяемости критериев, поэтому не блокирует по критерию «ТЗ невыполнимо», +но прямо нарушает обязательный список разделов §7.1 → жёлтый вердикт, правка +остаётся в этом ТЗ (Medium в скоупе, решение владельца 2026-08-19 / #202: не +заводится отдельный issue). + +**Как закрыть:** добавить раздел «Риски» (после §13/§14 или перед §17, +по образцу 253/275), явно разобрав минимум три пункта выше — параллельность +#276/#277/#278, судьбу legacy `degraded-extra` планов без #276-ремонта и +жизненный цикл временного adapter #277. + +## Что не проверял + +- Реализуемость `union()`/`polyclip-ts` конкретно для сценария «сохранить + primary geometry при исключении на N-м extra и продолжить с N+1» — + прочитано только текущее поведение (общий catch), не пробовалась + экспериментальная реализация; на этапе spec это ожидаемо не требуется. +- Полный список файлов/модулей, которые будут затронуты в реализации — не + входит в обязательные разделы ТЗ §7.1 (это чек-лист DoR §2.5), поэтому не + оценивался как находка. +- Golden/perf/smoke наборы не запускались — код не менялся, гейты §8 к spec-этапу + неприменимы; таблица гейтов не прикладывается по той же причине. +- Не проверялось содержимое приложенного `33.json` (не публикуется по + политике приватности, согласно самому issue) — доверился описанию автора и + перепроверил через код, что описанный механизм отказа (`extraBodies` без + try/catch) реален и достаточен для воспроизведения заявленного симптома. + +## Вердикт + +Жёлтый. Единственная находка — M1, в скоупе задачи, не заводится отдельным +issue. После добавления раздела «Риски» документ готов к повторному циклу.