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

SPEC-REVIEW-314-r1

Issue: #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