diff --git a/docs/reviews/SPEC-REVIEW-316-r1.md b/docs/reviews/SPEC-REVIEW-316-r1.md new file mode 100644 index 00000000..092feccb --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-316-r1.md @@ -0,0 +1,238 @@ +# SPEC-REVIEW-316-r1 + +Issue: #316 · ТЗ: `docs/specs/316-opening-host-auto-resolution.md` · +ветка `issue/316-migration-auto-resolve` · SHA материала ревью: `36f735ae` +(`docs: spec #316 — migration auto-resolves opening-host conflicts`). +Трек: обычный (метка `small` отсутствует) → требуется файл ТЗ и отдельный +документ ревью, что и сделано. + +## Скоуп + +Владелец решил в чате (комментарий S2→S4, 2026-08-26): конфликт +«проём ↔ нулевая стена» миграция v9 разрешает автоматически, рисование +нигде не блокируется. Настоящее ТЗ формализует правила авто-разрешения +(§3.1–3.4) и сужает контракт #306 §8.2 п.4. Отдельная задача #319 +(бэкенд-гард против stale-клиента) прямо исключена из скоупа — правильно, +это независимый инвариант. + +## Как проверялось + +Ревью документа без исполнения кода (стадия ТЗ, код ещё не написан — +commit `36f735ae` содержит только спецификацию). Проверено: + +1. Полное чтение `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (см. системный + промпт) — обязательные разделы §7.1 и чек-лист DoR §2.5 сверены построчно. +2. Тело issue #316 и оба комментария (аналитика с воспроизведением, + решение владельца S2→S4) — сверено, что ТЗ не расходится с зафиксированной + причиной и решением. +3. Чтение текущего (до фикса) `src/wall-segment-model.ts`: `buildAtoms`, + `resolveRoomOpeningHost`, `hostRoomOpenings`, `migrateSpace`, + `commitWallSegmentModel` — сверка каждого нормативного правила §3 ТЗ с + реальным кодом, который оно должно изменить. +4. Проверка ссылок ТЗ на прежние документы: `docs/specs/306-zero-thickness-walls.md` + §8.2 п.4 и п.8 (ровно то, что ТЗ #316 объявляет заменяемым/сужаемым — + подтверждено построчно), `docs/WALL-THICKNESS.md` §9 (независимая кладка, + `opening cannot punch a coincident independent wall`), issue #313 (полный + текст). +5. Проверка мест использования `resolveRoomOpeningHost` в `houseplan-card.ts` + (drag/placement путь, строки ~12680–12684 и ~12828–12834) — подтверждает + claim §3.3 «picker/placement уже умеет переставлять безхостовый проём». +6. Проверка `scripts/config-field-registry.mjs` (запись + `spaces[].openings[].host=wall`) и backend `validation.py` + (`vol.Optional("host")`) — сверка, меняется ли контракт совместимости, + который ТЗ должно менять/документировать. +7. Поиск i18n-ключей `toast.zero_wall_migration_blocked` / + `wall_model.reason.opening-host` в `src/i18n/{ru,en}.json` — проверка, + остаются ли они актуальны для пост-v9 fail-closed случая (AC5). +8. `ls demo/smoke_*.mjs` — проверка существующего покрытия по теме (нашёлся + `smoke_inert_openings.mjs`, но по не относящемуся термину — см. находку L1). + +Гейты `typecheck`/`test`/`build` не прогонялись: на этапе ТЗ продуктового +кода ещё нет, диапазон изменений — исключительно `docs/specs/316-*.md` +(класс C). Это не пропуск гейта — гейтов для класса C на этой стадии не +предусмотрено. + +## Находки + +### M1 (Medium, в скоупе) — ТЗ не закрывает несколько обязательных разделов §7.1 / чек-листа DoR §2.5 + +Отсутствуют полностью, не как «неявно покрыто», а буквально нет ни строки: + +- **i18n** — не сказано даже «изменений нет». Между тем это не тривиально: + AC5 подразумевает, что `wall_model.reason.opening-host` и + `toast.zero_wall_migration_blocked` остаются в строю для пост-v9 + fail-closed случая — я проверил, ключи в `ru.json`/`en.json` есть и не + тронуты, но ТЗ обязано сказать это явно, а не оставлять ревьюеру + реконструировать. +- **Release-артефакты** — changelog RU+EN, документация, golden/скриншоты + не упомянуты вовсе. Это не формальность: правило 3.1 меняет зонирование + `cm:0` относительно beta.3 (сам документ признаёт это в разделе «Риски»), + то есть меняет видимую геометрию стен в местах с легаси-проёмами — а + значит потенциально golden-эталоны, где такая геометрия зафиксирована. + ТЗ не говорит, требуется ли пересъёмка/сверка golden, и не резервирует + правку `docs/CHANGELOG.md`/`docs/CHANGELOG.ru.md`, хотя описанное + поведение прямо user-visible (пользователь больше не видит блокирующий + тост, комната рисуется). +- **Затронутые файлы/модули** — DoR §2.5 требует их перечисления + (`docs/specs/README.md` дальше по цепочке это использует); в ТЗ нет + списка (ожидаемо: `wall-segment-model.ts`, возможно + `scripts/config-field-registry.mjs`, `docs/specs/306-*.md` cross-ref). +- **Влияние на производительность и на touch** — ни разу не названо, даже + как «нет» с обоснованием. Оба пункта DoR §2.5 сформулированы как + обязательные к явному ответу («или явно "нет"»), не как опциональные. +- **Compatibility-поле по `docs/CONFIG-COMPATIBILITY.md`** — ТЗ не + упоминает этот документ вовсе, хотя правило 3.3 меняет реальный + контракт совместимости: `scripts/config-field-registry.mjs`, запись + `spaces[].openings[].host=wall` (строки 291–303), сейчас гласит + *«required for contour openings in model v8»* и *«materialize only when + exactly one carrier is proven»* — то есть текущий машиночитаемый канон + прямо противоречит новому §3.3 («без host» становится валидным + персистентным v9-состоянием при отсутствии кандидата). Правило + «документация — в том же коммите, что поведение» (PROCESS.md, правило + 11) требует, чтобы эта запись реестра тоже обновлялась; ТЗ должно было + назвать это явно как часть контракта, а не оставить открытие + ревьюеру. + +Отдельно: раздел «принято предположительно, поменять свободно» +(обязателен по PROCESS.md §7.1 для всего, что автор решает сам без +вопроса владельцу) в документе отсутствует полностью, хотя решений +уровня «непродуктовых, но не очевидных» несколько: порядок +tie-break в 3.2 (текущий host → расстояние → `cm` → id), и — что важнее — +семантика «инертного» проёма в 3.3 (проём остаётся в данных, но перестаёт +резать стену/создавать тоннель) — это уже граничит с «что считать +приемлемой деградацией», один из двух видов вопросов, которые процесс +резервирует за владельцем (PROCESS.md §7.1). Формально владелец уже дал +общую директиву «рисование нигде не блокируется», и 3.3 — разумное +техническое следствие, но фиксация этого решения явным блоком (а не +внутри нормативного правила) — то, что требует процесс, и то, что даёт +ревьюеру предмет для несогласия, а не факт. + +**Почему Medium, а не High:** ни один из этих пробелов не делает уже +написанные правила §3 невыполнимыми или непроверяемыми — рассуждение +корректно и обосновано кодом (проверено чтением `wall-segment-model.ts`, +см. «Как проверялось» п.3–6). Но по букве DoR §2.5 «если хоть один пункт +не выполнен — статус не "Готово к разработке"», и здесь не выполнено +сразу пять пунктов чек-листа. Чинится в этом же ТЗ без изменения +контракта §3 — ревизия 2 добавляет разделы, не переписывает правила. + +### M2 (Medium, в скоупе) — AC3 описывает фикстуру, не достижимую в названном ею механизме + +AC3: «Два коллинеарных совпадающих кандидата (**контурный + независимый**, +кейс #313)». Формулировка правила 3.2 сама ссылается на +`resolveRoomOpeningHost.eligible`, а функция (проверено чтением +`src/wall-segment-model.ts:588–615`) принимает единственный параметр +`segments: readonly WallSegmentEntry[]` — это `space.wall_segments`, +собранный `buildAtoms` **только из полигонов комнат** (контурные атомы). +Независимая кладка (`partitions`, `room_drafts`, `wall_columns`) в эту +коллекцию не попадает вообще, а `hostRoomOpenings` сама явно пропускает +`opening.host?.kind === 'partition'` (строка 620) — партиционные проёмы +резолвятся другим путём, не через это правило. Значит «контурный + +независимый» кандидат для `resolveRoomOpeningHost` в принципе не может +возникнуть так, как описано. + +Ссылка на «кейс #313» тоже не подтверждает сценарий: issue #313 — это +инструмент «Толщина» (`_wallThickHit`), не умеющий выбирать отдельно +стоящие стены/драфты в UI; он не о неоднозначности хоста проёма и не +про два коллинеарных кандидата в `wall_segments`. Единственная найденная +опора для «coincident»-сценария — `docs/WALL-THICKNESS.md` §9 +(«opening cannot punch a coincident independent wall», строки 480–509, +`smoke_optimize_coincident_partition.mjs`) — но это Optimize-конкретный +путь материализации, отдельный от `resolveRoomOpeningHost`, и там для +партиционных проёмов другой резолвер (`host:{kind:'partition'}`). + +Реальный источник неоднозначности внутри `resolveRoomOpeningHost` скорее +находится на стыке двух **соседних контурных атомов** (например, на +границе смены толщины — `smoke_wall_thickness_transition.mjs`), где +проём у самого стыка формально укладывается в допуск `EPS`/ +`GRID_STEP_N * 0.02` сразу для двух атомов. Это не то же самое, что +заявлено в AC3. + +**Что нужно от автора:** заменить фикстуру AC3 (и её обоснование) на +реально достижимую внутри `resolveRoomOpeningHost` — два коллинеарных +контурных атома на стыке (thickness-transition или аналогичный кейс), с +корректной ссылкой на источник, либо явно показать код-путь, которым +независимая кладка всё же попадает в `segments` (если такой существует и +я его не нашёл — тогда снимаю находку). + +**Почему Medium:** правило 3.2 (сам алгоритм tie-break) звучит разумно и +тестируемо — есть исполнимая, реально достижимая фикстура для той же +цели (стык двух контурных атомов). Дефект локален к описанию AC3, не к +нормативному правилу. + +### L1 (Low) — коллизия термина «инертный» + +§3.3 называет безхостовый проём «инертным» («проём... становится +инертным: не создаёт тоннель/вырез/тело, не участвует в физике»). Термин +уже занят: `docs/UX-MODES.md` (строки 18, 207) использует «inert» для +конкретного контракта — блокировки взаимодействия в View/kiosk по lock +guard, установленного `docs/SCOPE.md`. В коде есть отдельный смок +`demo/smoke_inert_openings.mjs`, который проверяет именно +View-инертность (pointer-events/cursor), а не отсутствие хоста — это +подтверждено чтением файла. Новое состояние (проём без host, при этом +по-прежнему интерактивный и перетаскиваемый в Plan mode — см. +`houseplan-card.ts:12680–12684`) описывает нечто иное, и повторное +использование того же русского слова создаёт риск, что будущий автор +кода/теста перепутает два разных инварианта по имени. + +**Предложение:** выбрать другой термин («безхостовый», «unhosted», «проём +без носителя» — как и названо в остальном тексте ТЗ) и не использовать +«инертный» применительно к этому состоянию. + +## Что проверено и корректно + +- Причинно-следственная цепочка (§1) точно совпадает с воспроизведением + из аналитического комментария владельца — не домысел, а прямая + трассировка к `commitWallSegmentModel`/`hostRoomOpenings` в текущем коде. +- Границы (§2): корректно и дословно сверено с #306 §8.2 — заменяется + ровно пункт 4 («legacy-конфликт... блокирует миграцию всего + пространства»), пункты 1–3 (picker не предлагает 0 как host, запрет + перевода host-атома в 0, отсутствие tunnel/fill у нулевой стены) и + пункт 7 (§8, строка 68: «новый проём на нулевой стене запрещён») + остаются в силе как пост-миграционные — согласуется с исходным текстом + #306. +- Правило 3.1 (проём удерживает стену) — физический смысл обоснован, + критерий (интервал проёма пересекает интервал атома, допуск + `GRID_STEP_N * 0.02`, тот же угловой критерий) идентичен уже + существующему `eligible()` в `resolveRoomOpeningHost` — заимствование + проверенного критерия, а не новое изобретение. Технически потребует + перестановки порядка вычислений в `buildAtoms` (сейчас cuts считаются + до того, как известны `cm`/host), но это вопрос реализации, не + корректности ТЗ. +- Правило 3.3 (проём без кандидатов → безхостовое состояние, рендер по + x/y) — подтверждено чтением `space-render.ts:285` и + `plan-geometry-preflight.ts:224/276`: обе точки уже трактуют + `!opening.host` как проходной случай («return [opening]»/«return + [fallback]»), то есть рендер по x/y для безхостового проёма — не + придуманное поведение, а уже существующий код-путь. Claim про + picker/placement также подтверждён (см. «Как проверялось» п.5). +- AC1, AC2, AC4–AC6 — сформулированы однозначно, с указанным способом + доказательства (`smoke`/`unit`/`backend`), фикстуры проверяемы по + описанному коду. +- Обратной совместимости данных, уже мигрировавших на beta.3, дано явное + и честное признание в разделе «Риски» — без попытки выдать частичный + охват за полный. +- Откат (§6) конкретен и проверяем: ревёрт ветки восстанавливает + прежнее поведение, уже мигрировавшие v9-документы остаются валидными. + +## Чего не проверял + +- Не проверял осуществимость реализации на уровне алгоритма (порядок + вычислений в `buildAtoms`/`hostRoomOpenings` для правил 3.1–3.3) — + это задача автора кода, не предмет ревью ТЗ; я лишь убедился, что + используемые в правилах критерии (допуски, угловой тест) уже есть в + кодовой базе и не придуманы с нуля. +- Не прогонял никаких гейтов (typecheck/test/build/смоки/golden) — на + этой стадии продуктового кода нет, класс изменений C (документация), + гейты для класса C не предусмотрены до кода. +- Не проверял #319 (бэкенд-гард) — прямо исключён из скоупа этим ТЗ и + корректно помечен как отдельная задача. +- Не оценивал точную реализуемость AC6 (байтовая идемпотентность) — + доверяю существующему паттерну (`commitWallSegmentModel` + уже задуман идемпотентным для Optimize, судя по комментариям в коде), + предметно не проверял иначе как чтением. + +## Вывод + +High-находок нет. M1 и M2 — в скоупе задачи, чинятся в этом же ТЗ без +пересмотра нормативных правил §3 (кроме фикстуры AC3). Вердикт — +жёлтый, документ возвращается автору на ревизию 2.