docs: review document for #314

Issue: #314
User-Visible: no

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

@@ -0,0 +1,205 @@
+# SPEC-REVIEW-314-r1
+
+Issue: [#314](https://github.com/Matysh/houseplan-card/issues/314) — Model v8:
+рисование комнат отклоняется, draft IDs теряются, возможны ложные перегородки.
+Этап: spec (PROCESS.md §2.4). Заход: r1. Блокирующих циклов израсходовано: 0/4.
+
+ТЗ: `docs/specs/314-v8-draft-write-regression.md`, ветка
+`issue/314-v8-draft-write-regression`, SHA материала ревью `772b43b6` (коммит
+«docs: specify atomic v8 draft writes»).
+
+Вердикт: **зелёный**.
+
+## Скоуп ревью
+
+Первый заход — полный разбор ТЗ, без раздела «Унаследовано» (§2.9/§2.10
+PROCESS.md не применяются, r1).
+
+Проверялось:
+
+1. соответствие обязательным разделам §7.1 (сценарий, что видит пользователь,
+   проблема/причины, скоуп/не-скоуп, контракт поведения, UX, модель данных и
+   миграция, i18n, AC1…ACn с доказательством, план автотестов, риски, откат,
+   release-артефакты);
+2. фактическая точность технических утверждений — построчная сверка каждой
+   заявленной причины дефекта с текущим кодом на `HEAD`, а не с пересказом
+   автора;
+3. однозначность и проверяемость каждого AC, наличие способа доказательства;
+4. отсутствие догадок, выданных за факт, вне блока «принято предположительно»;
+5. согласованность с `docs/SCOPE.md` (J6 — «keep the plan true as the home
+   evolves»), `docs/CONFIG-COMPATIBILITY.md` (текущий канон model v8) и
+   `docs/USER-GUIDE.ru.md`/`src/i18n/*.json` (текст тостов).
+
+## Как проверялось
+
+Полное чтение ТЗ (346 строк) и тела issue #314 с комментариями аналитики.
+Точечное чтение кода на упомянутых и смежных участках:
+
+- `src/houseplan-card.ts` — `_dropLegacySegments()` (сборка `room_drafts[].segments`
+  как `{ cm }`, потеря `id`), `_writeConfig()` (место вызова перед
+  `houseplan/config/set`), `_undoPoint()` (полная пересборка `segments` без id
+  при откате точки), `_saveConfigDebounced()` (catch-ветки
+  `wall_model_client_outdated`/`conflict`/generic — только toast, без rollback,
+  кроме `conflict`), `_commitPhysicalGeometry()` и `_pendingPhysicalWrites`
+  (подтверждено: `before` уже хранится как «earliest», `pending?.before ||
+  historyBefore`), `_finishWallChain()` (материализация активного draft в
+  `partitions`).
+- `custom_components/houseplan/validation.py` — `_legacy_wall_model_projection`,
+  `_legacy_wall_geometry_projection`, `_wall_catalog_projection`,
+  `_restore_wall_model_fields`, `validate_wall_model_transition()`, точное
+  место и текст `"v8 draft wall segments require ids"` (L1827-1828).
+- `docs/CONFIG-COMPATIBILITY.md` («Stable wall identity — model v8») — сверка,
+  что текущий канон действительно описывает грубую (не разделённую) проверку,
+  которую ТЗ предлагает уточнить, а не переписать заново.
+- `src/i18n/ru.json` / `en.json` — точный текст обоих тостов, упомянутых в
+  issue, совпадает дословно; ТЗ не предлагает менять и не изобретает
+  формулировки (§9: «Новых i18n-ключей нет»).
+- Существование и актуальность путей, названных в ТЗ:
+  `tests_backend/test_wall_segment_model.py`, `docs/TESTING.md` (уже содержит
+  раздел «Stable wall-segment identity (#282)»), `scripts/check-docs.mjs
+  --external`, `npm run invariants`, `npm run golden:verify` — все существуют
+  и соответствуют описанию.
+
+Не проверялось (не нужно на этапе spec): запуск гейтов/тестов — кода нет,
+проверять нечего; ревью spec-этапа не прогоняет typecheck/test/build.
+
+## Находки
+
+Ни одной High или Medium-находки, блокирующей выход в «Готово к разработке»,
+не обнаружено.
+
+### Low-1 (снята с записью). Сценарий не называет персону/job явно
+
+`docs/specs/314-v8-draft-write-regression.md` §1 описывает сценарий
+(«Пользователь рисует комнату в редакторе Плана») без прямой ссылки на
+`docs/SCOPE.md` (персона Home admin, job J6 — «Keep the plan true as the home
+evolves»). PROCESS.md §7.1 просит указывать персону и поверхность явно.
+
+**Почему не блокирует:** редактор Плана — по определению `docs/SCOPE.md`
+инструмент только для Home admin, второй персоны там физически нет (View/kiosk
+не пишут `room_drafts`). Двусмысленности, кто встретит этот дефект и на какой
+поверхности, не возникает; аналитический комментарий владельца уже явно
+называет J6. Снимаю без правки ТЗ.
+
+### Low-2 (снята с записью). Нет отдельного раздела «план автотестов»
+
+Список тестов есть, но распределён между §9 «Затронутые модули»,
+блоком «Обязательные регрессионные тесты» в теле issue и полем «Доказательство»
+каждого AC1…AC10, а не собран под одним заголовком «план автотестов»,
+формально ожидаемым PROCESS.md §7.1.
+
+**Почему не блокирует:** содержательно план полон и избыточно точен — для
+каждого нормативного пункта контракта (§4, §5, §6) есть привязанный AC с
+именованным способом доказательства (`frontend unit`, `backend schema
+fixture`, `parametrized backend tests`, `browser/fake-WS smoke`,
+`model-invariant unit + review кода`), а AC5 отдельно требует «тест обязан
+падать на `dev` до исправления». Организационный, не содержательный пробел.
+
+## Что проверено и корректно
+
+- **Root cause A (потеря ID при sanitation/undo) — подтверждён построчно.**
+  `_dropLegacySegments()` (`src/houseplan-card.ts`, ветка `room_drafts`)
+  пересобирает каждый `segment` как `{ cm }`; `id` действительно не
+  копируется. `_writeConfig()` действительно вызывает эту функцию
+  непосредственно перед `houseplan/config/set`. `_undoPoint()` действительно
+  пересобирает **весь** массив `segments` из `_draftSegmentCms` (не только
+  последний элемент) — при откате одной точки теряют id все N-1 уцелевших
+  сегментов, не только удалённый. ТЗ описывает это точно (§2.1) и корректно
+  ставит AC2 требованием сохранения id именно для «уцелевших N-1», а не только
+  «незатронутых».
+- **Backend-ошибка воспроизводится детерминированно.** `validation.py`
+  L1826-1828: `if any(not segment.get("id") for segment in
+  draft.get("segments", [])): raise vol.Invalid("v8 draft wall segments require
+  ids")` — точный текст сообщения из симптома пользователя совпадает.
+- **Root cause B (ложный «устаревший клиент») — подтверждён.**
+  `_legacy_wall_geometry_projection()` включает `rooms`, `walls`, `open_spans`,
+  **весь** `room_drafts` (без id, только `points`+`cm`) и **весь** `partitions`
+  as-is, плюс `openings` (id/type/x/y/angle/length, без host).
+  `validate_wall_model_transition()` при `old_model>=8, new_model>=8`
+  сравнивает эту проекцию целиком и при расхождении, но неизменном
+  `wall_segments`, кидает `wall_model_client_outdated` — независимо от того,
+  что именно изменилось. Значит правка одного draft-сегмента, добавление
+  partition или перемещение opening действительно триггерят ложный отказ уже
+  сегодня, если контурный каталог не менялся. Предложенное разделение на
+  catalog-coupled (§5.1: rooms id/poly/open_to, walls, open_spans) и
+  self-identifying (§5.2: room_drafts, partitions, wall_columns, openings)
+  проекции корректно ложится на реальный код: `wall_columns` в текущей
+  legacy-проекции вообще отсутствует, то есть уже не участвует в
+  false-positive — включение его в контракт лишь фиксирует существующее
+  верное поведение регрессионным тестом (AC3.4), не притворяется, что чинит
+  несуществующий баг.
+- **Механизм отката rejected-геометрии (§6) не изобретает новую
+  инфраструктуру, а достраивает существующую.** `_pendingPhysicalWrites`
+  действительно уже хранит «earliest» `before`
+  (`pending?.before || historyBefore` в `_commitPhysicalGeometry()`) —
+  §6.1 корректно описывает уже существующий факт, а не новое требование.
+  Не хватает только проводки catch-ветвей `wall_model_client_outdated` и
+  generic-ошибки до этого состояния (сегодня это делает только `conflict`) —
+  именно так и сформулирован AC6/AC7.
+- **Разбор продуктового риска «более новая команда теряется вместе с
+  отклонённой».** §6.3 и AC7 явно фиксируют: при отказе F1 откатываются
+  **обе** команды (F1 и более новая F2), а не только отклонённая. Это
+  потенциально видимая пользователю потеря работы, поэтому я целенаправленно
+  проверял, не должен ли это быть продуктовый вопрос владельцу, а не
+  «техническое предположение» (§16 п.3). Вывод: не должен — (а) это лишь
+  расширяет уже существующий прецедент в коде (`conflict`-ветка уже делает
+  `_cancelPath()` + полный `_reloadConfigOnly()`, то есть уже отбрасывает
+  локальный путь целиком), (б) после исправления root cause B легитимное
+  последовательное рисование стен вообще перестаёт получать
+  `wall_model_client_outdated`, так что путь отката — не основной сценарий, а
+  страховка от настоящих отказов (схема, сеть, реальный stale-writer),
+  (в) поведение явно протестировано отдельным AC7, а не спрятано. Решение
+  корректно оставлено «принято предположительно» технической категорией.
+- **Ни одна причина не выдана за факт без доказательства там, где доказательства
+  нет.** §2.3/§3 явно помечают связь «отклонённый draft → лишняя partition»
+  как «правдоподобный механизм», требующий отдельного доказывающего теста
+  (это и есть AC6/failure smoke) — не заявлено как решённое.
+- **Текст ошибок пользователю не меняется и не изобретается.** Оба
+  toast-текста, названные в issue, сверены дословно с
+  `src/i18n/ru.json`/`en.json`; ТЗ явно не добавляет новых i18n-ключей — верно.
+- **Совместимость с каноном.** `docs/CONFIG-COMPATIBILITY.md` («Stable wall
+  identity — model v8») сегодня описывает именно ту грубую проверку, которую
+  ТЗ сужает; ТЗ включает обновление этого файла в затронутые модули (§9) —
+  значит канон не разойдётся с кодом после реализации.
+- **Обязательные разделы §7.1 присутствуют по существу**: сценарий и
+  видимый результат (§1), проблема/причины (§2), скоуп/не-скоуп (§7/§8),
+  контракт поведения (§4-§6, весьма подробно), модель данных/миграция (§4 —
+  «Persisted schema и model_version не меняются»), i18n (§9), AC1…AC10 с
+  доказательством (§10), риски (§13, таблица риск/мера), откат (§14),
+  release-артефакты (§15). Отсутствие отдельных заголовков «проблема» и «UX» —
+  организационный вопрос, содержание есть (см. Low-2 по аналогии); UX явно
+  закрыт фразой «Новых кнопок, настроек и режимов нет» (§1) и §11.
+- **Не-скоуп корректно исключает соседние риски.** #306 (заблокирован),
+  автоматическая чистка существующих неоднозначных `partitions`, смена модели
+  толщины/визуала стен — всё явно выведено за рамки и не создаёт скрытого
+  расширения скоупа под видом фикса.
+- **Assumptions-блок (§16)** используется по назначению: все четыре пункта —
+  действительно технические решения (граница open_spans/#306, критерий
+  безопасности opening-only правки, стратегия «терять весь pending batch»,
+  «не удалять существующий мусор») с пометкой «поменять свободно», не
+  продуктовые вопросы, требующие ответа владельца.
+
+## Чего не проверял
+
+- Не запускал гейты (`typecheck`/`test`/`build`) — на этапе spec-review кода
+  нет, проверять нечего, это будет делом code-review.
+- Не проверял `_draftSegmentsForPath()` и полный код `commitWallSegmentModel()`
+  построчно — не требовалось для оценки ТЗ: контракт §4 описывает целевое
+  поведение этой функции, а не текущую реализацию, и её корректность —
+  предмет code-review, а не spec-review.
+- Не читал файлы владельца (`houseplan-space-convergence-test-...json`,
+  `2026-08-26_07-49-10.png`) — не приложены к issue/репозиторию для ревьюера
+  и явно помечены «публично не прикладывать»; ТЗ уже хеджирует любые выводы
+  из них как «не позволяет надёжно определить» / «требует отдельного теста»,
+  так что для оценки ТЗ они не нужны.
+
+## Итог
+
+ТЗ технически точно (все процитированные причины сверены построчно с кодом
+на HEAD и подтверждены), полно по требованиям §7.1, содержит десять
+однозначных, проверяемых AC с указанным способом доказательства, корректно
+разделяет продуктовые предположения (нет ни одного) от технических (явно
+помечены в §16), и не выдаёт недоказанные гипотезы за факт. Обе найденные
+Low-находки — организационные, не меняют содержание ТЗ и сняты без правки.
+
+Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0