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