docs: review document for #186

Issue: #186
User-Visible: no

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

@@ -0,0 +1,358 @@
+# SPEC-REVIEW-186-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/186
+- **ТЗ под ревью:** [`docs/specs/186-partition-opening-jamb-margin.md`](https://github.com/Matysh/houseplan-card/blob/issue/186-partition-jamb-margin/docs/specs/186-partition-opening-jamb-margin.md)
+  (коммит `7d4d3f0`), обычный трек — не `small`/`trivial`
+- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
+- **Трек:** обычный, лимит циклов ревью ТЗ — 4 (§4 PROCESS.md)
+- **Цикл:** r1/4
+
+## Скоуп ревью
+
+ТЗ #186 — follow-up к #132 (найдено на код-ревью `CODE-REVIEW-132-r1.md`,
+Medium-1): вводит содержательный jamb safety margin (половина фактической
+толщины independent-стены у каждого торца) для door/window/gate/passage,
+размещённых в `partition`, вместо нынешнего `jambMargin=0` в
+`resolvePartitionOpening()` и голого `1e-9`-эпсилона в backend. Затрагивает
+placement/drag/dialog в Plan editor, frontend commit validation, backend
+semantic delta validation (`config/set`, optimize, import) и compatibility
+уже сохранённых near-end проёмов.
+
+Не в скоупе ревью: продуктовый код. Ветка `issue/186-partition-jamb-margin`
+содержит один коммит (`7d4d3f0`, `Issue: #186 · User-Visible: no`) — только
+спецификация и обновление `docs/specs/README.md`, реализации нет.
+Гейты (`typecheck`/`test`/`build`) не прогонялись: на этапе ревью ТЗ
+продуктового кода не существует, прогон к этому этапу не относится
+(PROCESS.md §2.4/§8).
+
+## Как проверялось
+
+1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (действующая
+   редакция, включая §1, §2.2–§2.4, §5, §7.1, §8, §10.2).
+2. Прочитано тело issue #186 и все три комментария: аналитика владельца
+   (оценка 5/10 · 6/10 · 5/10 · P2 · `bug`, обычный трек с явным
+   обоснованием, три вопроса с default), решение владельца (все три default
+   приняты дословно) и хендофф «ТЗ готово к ревью».
+3. Построчно сверены технические утверждения §4 ТЗ с кодом на этой ветке:
+   - `resolvePartitionOpening()` (`src/partition-openings.ts:35-61`) —
+     параметр `jambMargin = 0` подтверждён; проверка `does-not-fit`
+     (`along - length/2 < jambMargin - 1e-9 || along + length/2 >
+     axisLength - jambMargin + 1e-9`) действительно единственное место, где
+     `jambMargin` используется.
+   - Продакшн call sites (`src/houseplan-card.ts:7455, 7920, 7953, 11300,
+     11604, 11613, 17569`, `src/space-render.ts:214`) действительно не
+     передают шестой аргумент — ТЗ не завышает диагноз issue.
+   - Формула §4: `wallDepth = wallCmToUnits(cm, cellCm, gridPitch)`,
+     `jambMargin = wallDepth/2` — проверено по `wallCmToUnits()`
+     (`src/wall-thickness.ts:70-73`, `= (cm/cellCm) * gridPitch`) и по
+     фактическому вызову на `houseplan-card.ts:7455-7457`, где
+     `lengthScale=NORM_W` и `gridPitch=this._gridPitch` (геттер возвращает
+     `GRID_PITCH`, `src/houseplan-card.ts:5771`, `GRID_PITCH = NORM_W /
+     GRID_N`, `GRID_N = 240`, `src/space-geometry.ts:197-201`). В render-
+     единицах (`NORM_W`-масштаб) формула самосогласована с `axisLength` и
+     `length`, которые в этом же call site тоже в render-единицах.
+   - Backend-эквивалент §4: `partition.cm / cell_cm / 240 / 2` в
+     normalized units (масштаб хранения, 1.0 = ширина канвы) — пересчитан
+     независимо: `wallCmToUnits` в render-единицах даёт `(cm/cellCm) *
+     (1000/240)`; деление на `NORM_W=1000` (переход в normalized) даёт
+     `(cm/cellCm)/240`, и `/2` для margin — **формула ТЗ подтверждена
+     арифметически**, это не придуманная константа, а имеющийся `GRID_N=240`
+     (`src/space-geometry.ts:197`, тот же литерал уже упомянут комментарием в
+     `custom_components/houseplan/validation.py:445`, «240 cells across the
+     unit width»).
+   - `_space_geometry_invariants()` (`custom_components/houseplan/
+     validation.py:813-852`, встроена в `SPACE_SCHEMA` через `vol.All`) —
+     подтверждён zero-margin fit-check с `1e-9` (строка 845), встроенный в
+     структурную схему, а не в semantic-слой — совпадает с диагнозом issue и
+     §8 ТЗ («Структурная SPACE_SCHEMA сохраняет прежний zero-margin fit
+     check»).
+   - `validate_partition_opening_hosts()` (`validation.py:55-79`) — реально
+     существующий semantic delta validator с сигнатурой `(config,
+     previous=None)`, уже используется в `config/set`
+     (`websocket_api.py:1260`) и в optimize (`websocket_api.py:1372`, тот же
+     блок вызывает `validate_marker_controls`/`validate_opening_passages`/
+     `validate_partition_opening_hosts` с явным комментарием «Optimization is
+     a normal configuration write with an additional layout transaction. It
+     must enforce the same marker-link semantics as config/set»). Это
+     подтверждает AC6 («config/set и optimize применяют один semantic
+     validator») как реальный, не гипотетический паттерн.
+4. Прочитан `docs/CONFIG-COMPATIBILITY.md` целиком, включая уже принятый
+   аналогичный прецедент «Independent-wall opening host (#132)» и «Open-
+   passage opening type (#157)» — оба используют тот же приём (new/changed
+   строго, existing/unrelated толерантно), что и §8 ТЗ #186 предлагает для
+   jamb margin; терминология («legacy», «read-compatibility», «drop-on-
+   validation») не изобретается заново.
+5. Прочитан `docs/USER-GUIDE.ru.md` §9 («Двери, окна, открытые проёмы, ворота
+   и замки», строки 428-479) и §20 («Хранение, совместная работа и
+   резервные копии», строки 1207-1244) — терминология диалога («независимая
+   стена», «край проёма», «торец») совпадает с уже принятым текстом;
+   отдельно проверено описание **полного** экспорта/импорта (см. находку
+   High-1 ниже).
+6. Прочитан `docs/WALL-THICKNESS.md` — термин «jamb» (jamb return, строки
+   94-134) уже канонический для геометрии откоса; «jamb safety margin» ТЗ не
+   вводит новую непонятную лексику, а расширяет существующее семейство.
+7. Проверен полный путь **full import** (кнопка «Полная копия» из §20
+   USER-GUIDE), которым оперирует §8 ТЗ:
+   - `prepare_apply()` (`custom_components/houseplan/import_export.py:1304-
+     1344`): при `document["kind"] == "full"` (строка 1330) вызывается
+     **только** `CONFIG_SCHEMA(config)` (строка 1344) — ни
+     `validate_marker_controls`, ни `validate_opening_passages`, ни
+     `validate_partition_opening_hosts` не вызываются вовсе для этого пути.
+     Сравнить с `document["kind"] != "full"` (строка 1339,
+     `build_space_merge`), который **вызывает** semantic-валидаторы, но
+     всегда с новым `space_id` (реассайн id, USER-GUIDE строка 1218), из-за
+     чего delta-сравнение по id всё равно никогда не находит «старую» запись.
+   - `ws_import_apply()` (`websocket_api.py:365-444`) на `kind == "full"`
+     (строка 426) сохраняет **текущий** `config_data.get("config")` в
+     `_OPTIMIZE_BACKUP` метаданных ради одной отмены (`can_undo`, строка 499)
+     — то есть «предыдущая» версия конфига физически загружена и доступна в
+     этот момент, просто не передаётся ни в один semantic validator.
+   - Итог: на сегодня **full import вообще не проверяет partition-opening-
+     host инварианты семантически** — ни downgrade guard, ни (будущий) jamb
+     margin; единственный барьер — структурная `SPACE_SCHEMA` с её
+     zero-margin fit-check. §8 ТЗ вводит для этого пути принципиально новую,
+     а не расширяемую проверку. См. находку High-1.
+8. Проверено соответствие `docs/SCOPE.md`: задача — геометрическая
+   достоверность плана (J4 «zero to a working plan... room polygons bound to
+   areas», J6 «keep the plan true as the home evolves»), персона home admin,
+   поверхность — desktop Plan editor + backend validation; View/kiosk не
+   затрагиваются. Полное резервное копирование/восстановление — тоже прямо
+   часть J6 («keep the plan true») и задокументировано в USER-GUIDE §20 —
+   поэтому конфликт из находки High-1 лежит внутри того же core job, который
+   ТЗ заявляет закрывать, а не в смежной, посторонней функции.
+9. Проверено, что issue не помечен `small`/`trivial`: аналитика владельца
+   явно называет причины (новый видимый граничный контракт,
+   compatibility-решение) — критерии §5 PROCESS.md («нет нового UX-
+   контракта», «нет compatibility-полей») не выполняются, обычный трек
+   обоснован корректно, файл ТЗ в `docs/specs/` создан как требуется.
+
+Гейты (`typecheck`/`test`/`build`, browser smoke, golden, backend pytest) не
+прогонялись — продуктового кода нет, что и ожидается на этапе ревью ТЗ.
+
+## Обязательные разделы (§7.1 PROCESS.md)
+
+| Раздел | Есть | Комментарий |
+|---|---|---|
+| Сценарий (персона/поверхность/момент) | ✅ | §1: home admin, Plan editor, размещение/перемещение проёма в independent-стене |
+| Что человек увидит до/после | ✅ | §2, одна фраза до/после без терминов реализации |
+| Проблема | ⚠️ | Нет отдельного заголовка «Проблема»; содержание фактически есть в §2 «До» и подтверждено issue. Формальный, не содержательный пробел — см. Low-1 |
+| Скоуп / не-скоуп | ✅ | §5/§6, конкретны, «доп. зазор между проёмами» и «resize partition» явно исключены |
+| Контракт поведения | ✅ | §7 (placement, drag/dialog, перемещение host) + §8 (backend) |
+| UX | ✅ | §9, таблица состояний, 8 строк |
+| Модель данных и миграция | ✅ | §10: «модель данных и schema полей не меняются» — явно и обоснованно |
+| i18n | ✅ | §10, RU/EN строка для banner приведена дословно |
+| AC1…ACn с доказательством | ✅ | §11, 6 штук, у каждого назван способ доказательства (unit/backend/smoke) |
+| План автотестов | ✅ | §12, разделён на implementation-loop и pre-review гейты |
+| Риски | ✅ | §13, 7 строк риск/мера — но не покрывает риск найденного противоречия (High-1) |
+| Откат | ✅ | §14, без миграции, возврат к zero-margin |
+| Release-артефакты | ✅ | §15, оба changelog, USER-GUIDE, CONFIG-COMPATIBILITY, TESTING |
+
+Присутствует также обязательный блок «Принятые технические предположения»
+(§16, 4 пункта) — все технические (rigid translation, `type`-only edit,
+имена helper/reason/i18n key, malformed `cm`/`cell_cm` fallback), ни один не
+маскирует продуктовый вопрос. Однако именно то, что должно было попасть в
+этот блок или в пачку вопросов владельцу — трактовка full import как
+«без trusted previous» — вместо этого записано как решённый факт в §5/§8/§11
+(AC4). См. находку High-1.
+
+## Находки
+
+### High-1 — «строгая» проверка full import противоречит уже принятому владельцем round-trip-инварианту и ломает документированный backup/restore
+
+**Файл:** `docs/specs/186-partition-opening-jamb-margin.md`, §8 (строки
+163-166), §5 (строка 68), AC4 (строки 223-231).
+
+**Что заявлено.** §8: «Полный import/restore без trusted previous
+рассматривает все hosted openings как новые и проверяет strict.» AC4 явно
+фиксирует это в тесте: «Direct geometry edit и full import того же нарушения
+отклоняются» (строка 228), с доказательством «backend delta/import tests с
+previous и без него» (строка 230).
+
+**Почему это противоречие, а не деталь реализации.**
+
+1. Owner-decision #3 (принята дословно в комментарии от 2026-08-19 08:02):
+   «Уже сохранённые проёмы, нарушающие новый предел, остаются видимыми и
+   рабочими, не сдвигаются автоматически и допускают неизменённый round-trip
+   при посторонних правках... Новый предел применяется при создании,
+   перемещении, изменении длины или перепривязке.» Ни исходный default
+   аналитика, ни решение владельца ни разу не называют «полный
+   экспорт/импорт» как один из триггеров нового предела — речь только о
+   прямых geometry-действиях над самим проёмом/host.
+2. «Полная копия» — не гипотетическая, а прямо задокументированная в
+   `docs/USER-GUIDE.ru.md` §20 (строки 1207-1244) функция **этого же** J6
+   («keep the plan true as the home evolves», `docs/SCOPE.md`): «Экспорт
+   предлагает... Полная копия — ...её импорт заменяет текущую модель
+   целиком; до следующего изменения плана доступна одна отмена.» Наличие
+   «одной отмены» — прямое доказательство (`websocket_api.py:426-444`,
+   `_OPTIMIZE_BACKUP`), что это рутинная операция на **той же** живой
+   инсталляции (восстановление вчерашней резервной копии после неудачных
+   правок), а не только перенос на чистый новый инстанс.
+3. Полный импорт **не переприсваивает id** (`import_export.py:1330-1344`,
+   ветка `kind == "full"`: `config = imported_config` без ремаппинга, в
+   отличие от `space`-merge, который явно генерирует новые id — USER-GUIDE
+   строка 1218). Значит id пространств/проёмов в восстановленном файле
+   буквально совпадают с id, под которыми они хранились в момент экспорта.
+4. Итог сценария: администратор на v1.65.0-beta.2 создал проём впритык к
+   торцу independent-стены (валидно на тот момент). После обновления до
+   версии с #186 он делает «Полная копия» (рутинный совет из
+   `docs/USER-GUIDE.ru.md` перед крупной правкой, строка 1358) — экспорт
+   содержит этот проём без изменений, потому что по decision #3 он остаётся
+   «видимым и рабочим». Позже (после неудачного эксперимента, или после
+   переустановки HA, или просто ради восстановления) он импортирует **тот
+   же самый, ничем не изменённый файл** обратно. По §8 ТЗ и AC4 этот импорт
+   **обязан быть отклонён** ровно из-за геометрии, которая мгновение назад
+   была признана допустимой для чтения/рендера/экспорта. Это прямое
+   нарушение decision #3 «допускают неизменённый round-trip» и очень
+   вероятный сценарий (единственное действие пользователя — сделать и
+   восстановить резервную копию, ничего не редактируя).
+5. Технически ничего не мешало сохранить round-trip хотя бы для случая
+   «restore на ту же инсталляцию»: `ws_import_apply()` уже загружает текущий
+   `config_data.get("config")` в этот самый момент (используется как backup
+   для отмены, строка 429) — тот же объект мог бы быть передан в semantic
+   validator как `previous`, как это уже делается для `config/set`
+   (`websocket_api.py:1260`) и optimize (`websocket_api.py:1372`). Но даже
+   это не спасает случай disaster-recovery на **пустой** инсталляции
+   (переустановка HA, `config_data.get("config")` пуст) — там по любому
+   id-based delta-подходу «предыдущего» совпадения не найдётся, и restore
+   старого валидного бэкапа всё равно будет отклонён. Это структурное
+   ограничение самой архитектуры full-import (нет модели «that record is the
+   same one, unchanged»), а не деталь одной реализации — то есть вопрос
+   принципиально не имеет тихого технического решения и должен решаться
+   владельцем.
+6. Формулировка §8 предъявлена как принятое решение, а не как assumption:
+   она не попала ни в блок §16 «Принято предположительно», ни в пачку
+   вопросов владельцу, хотя по своей природе — это ровно тот тип вопроса,
+   который согласно PROCESS.md §7.1 обязан идти владельцу: «поведение в
+   пограничном случае» и «что считать приемлемой деградацией». Оценка
+   аналитика (Touch/Performance были явно взвешены) не рассматривала этот
+   write-path вовсе.
+
+**Последствие для процесса.** Утверждение о поведении (full import —
+strict), которого нет ни в одном согласованном документе и которое
+противоречит уже зафиксированному владельцем decision #3, подано как факт.
+Это ровно тот класс дефекта, который PROCESS.md называет «худшим видом» —
+предположение, записанное как решение, потому что оно read как решение и
+прошло бы имплементацию буквально по AC4.
+
+**Требуемое действие.** Автор обязан вынести это отдельным продуктовым
+вопросом владельцу (в формате §7.1: что неясно · что изменится от ответа ·
+default), например:
+
+> Full import/restore того же полного бэкапа, содержащего legacy near-end
+> proём — тоже вариант «посторонней правки» с сохранением round-trip
+> (decision #3), или отдельный, более строгий путь, где такой бэкап нужно
+> будет сначала починить? Default: round-trip сохраняется всегда, когда id
+> пространства/проёма совпадает с уже хранимой записью (сравнение с текущим
+> `config_data.get("config")`, который и так загружается для undo-бэкапа);
+> при restore на пустую/чужую инсталляцию (нет совпадения по id) — new/strict,
+> как сейчас предложено.
+
+Это High: находка блокирует переход в `S5-ready`, поскольку задевает уже
+принятое владельцем решение и заложена в AC4 буквально — реализовать ТЗ как
+написано значит намеренно сломать документированный сценарий backup/restore
+для любого дома с legacy near-end проёмом.
+
+### Low-1 — нет отдельного раздела «Проблема»
+
+**Файл:** `docs/specs/186-partition-opening-jamb-margin.md`, между §1 и §3;
+ближайшее место — §2 «До / после» (строки 13-22).
+
+PROCESS.md §7.1 перечисляет «проблема» отдельным обязательным разделом.
+Содержательно проблема изложена — в §2 «До» и в теле issue («Практическое
+следствие: дверь/окно/ворота можно поставить в перегородку впритык к самому
+её концу, без зазора на откос»), но отдельного заголовка/абзаца «Проблема» в
+файле ТЗ нет. Ни один AC от этого не становится неоднозначным.
+
+**Решение ревьюера:** Low, не блокирует. Снимаю без возврата ТЗ на правку;
+рекомендую добавить короткий явный абзац «Проблема» при следующей правке
+файла (тот же цикл, что закроет High-1), а не отдельным циклом ревью.
+
+## Что проверено и корректно
+
+- **Физическая формула margin (§4 ТЗ) арифметически верна и не выдумана** —
+  проверена независимо через `wallCmToUnits()`, `GRID_PITCH`/`GRID_N=240`
+  (`src/space-geometry.ts:197-201`) на фронтенде и пересчитана в normalized-
+  масштаб для backend; итоговая backend-формула `cm/cell_cm/240/2` совпадает
+  с ТЗ буквально, включая уже присутствующий в кодовой базе литерал `240`
+  (комментарий `validation.py:445`).
+- **Диагноз issue подтверждён кодом**: `jambMargin=0` по умолчанию,
+  production call sites его не передают (`partition-openings.ts:41`,
+  8 реальных call sites), backend fit-check использует только `1e-9`
+  (`validation.py:845`) — ТЗ не завышает и не занижает масштаб проблемы.
+- **AC6 («config/set и optimize — один semantic validator») подтверждён
+  реальным существующим паттерном**: `validate_partition_opening_hosts()`
+  уже вызывается из обоих путей с явным комментарием о необходимости
+  одинаковой семантики (`websocket_api.py:1260, 1372`).
+- **§8 корректно описывает существующее устройство структурной схемы**:
+  `_space_geometry_invariants()` действительно встроена в `SPACE_SCHEMA`
+  через `vol.All` и продолжит принимать zero-margin legacy-геометрию, если
+  её саму не менять — ТЗ явно требует именно это («Структурная SPACE_SCHEMA
+  сохраняет прежний zero-margin fit check»).
+- **Терминология соответствует канону**: «независимая стена», «торец»,
+  «край проёма» — из `docs/USER-GUIDE.ru.md`; «jamb» как основа термина —
+  из уже принятого `docs/WALL-THICKNESS.md`. Диалоговое сообщение (RU/EN)
+  не изобретает новую лексику для пользователя (само слово «jamb»
+  наружу не выходит).
+- **`docs/CONFIG-COMPATIBILITY.md`-паттерн выбран корректно** для описанных
+  путей (config/set, optimize): «new/changed strict, unrelated tolerant» —
+  тот же приём, что уже принят для #132 (host) и #157 (passage) в этом же
+  документе; никакой новой персистентной модели/поля не вводится, что
+  соответствует §10 ТЗ и Rule №1 AGENTS.md (класс A без новых
+  compatibility-полей не требует отдельного решения о миграции).
+- **AC1, AC2, AC5 однозначны и снабжены допустимым способом доказательства**
+  (pure unit matrix, backend parametrized tests, browser smoke), включая
+  точную границу (equality valid) и матрицу `cm=1/15/100` — тестируемо и не
+  расплывчато.
+- **Touch-декларация присутствует буквально**: «Touch editor: best effort /
+  intentionally degraded» (§10) — соответствует `docs/TOUCH-SUPPORT.md`
+  Documentation rule, без нового жеста и с явным safety floor (tap/pinch/
+  pointercancel).
+- **Откат (§14) реалистичен**: без persisted-полей нет обратной миграции;
+  просто удаление strict-политики и семантического валидатора.
+- **§16 «Принятые предположения» не маскирует продуктовый вопрос** — все
+  четыре пункта (rigid translation, `type`-only edit, публичность имён,
+  malformed cm/cell_cm) действительно технические и не требуют решения
+  владельца сами по себе.
+
+## Чего не проверял
+
+- Реализацию — её нет: ветка содержит один документный коммит (`7d4d3f0`),
+  продуктовый код (`src/**`, `custom_components/houseplan/**/*.py`) не
+  менялся — проверено чтением diff коммита и содержимого текущих файлов
+  (`resolvePartitionOpening`, `validate_partition_opening_hosts`,
+  `_space_geometry_invariants`, `prepare_apply` не тронуты этой веткой).
+- Гейты `typecheck`/`test`/`build`/browser smoke/golden/`pytest
+  tests_backend` — не относятся к этапу ревью ТЗ; предмет будущего
+  код-ревью (PROCESS.md §2.4/§8).
+- Все восемь текущих production call sites `resolvePartitionOpening()`
+  построчно — проверены три (`houseplan-card.ts:7455`, и по номерам строк
+  для остальных сверено только наличие вызова через `grep`, не полный
+  контекст каждого) и одно место в `space-render.ts:214`; для целей ревью
+  ТЗ этого достаточно (диагноз issue про default-параметр не зависит от
+  конкретного call site), но при код-ревью потребуется пройти все.
+- Численную точность оценок аналитики (5/10 · 6/10 · 5/10 · P2) по существу
+  — поле владельца (PROCESS.md §2.2), уже принятое явным решением до
+  написания ТЗ.
+- Совместимость found-High-1 сценария с другими фичами полного импорта
+  (missing-plan/attachment проверки, `model_version` restore) — не относится
+  к предмету находки, эти проверки независимы от partition-opening host
+  semantics.
+
+## Вердикт
+
+Красный. High: 1 (§8/AC4 «full import — всегда strict» противоречит уже
+принятому владельцем round-trip-инварианту decision #3 и ломает
+задокументированный в `docs/USER-GUIDE.ru.md` §20 сценарий backup/restore
+для любого дома с legacy near-end проёмом; не помечено как assumption,
+подано как решённый факт). Medium: 0. Low: 1 (отсутствует отдельный
+заголовок «Проблема» — не блокирует, снимается с рекомендацией). Остальная
+часть ТЗ технически точна: физическая формула margin проверена независимым
+пересчётом и совпадает с реальными константами кода (`GRID_N=240`), AC1/AC2/
+AC5/AC6 однозначны и снабжены допустимым доказательством, compatibility-
+подход для config/set и optimize соответствует уже принятому в проекте
+паттерну. Возврат в `S3-spec`: автору нужно вынести found High-1 отдельным
+батчем вопросов владельцу (по шаблону §7.1 — что неясно · что изменится ·
+default) прежде чем ТЗ сможет получить зелёное ревью.
+
+**Вердикт: красный · цикл r1/4 · High: 1 · Medium: 0 → нет · Документ:
+docs/reviews/SPEC-REVIEW-186-r1.md**