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