diff --git a/docs/reviews/SPEC-REVIEW-529-r1.md b/docs/reviews/SPEC-REVIEW-529-r1.md new file mode 100644 index 00000000..7207fed8 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-529-r1.md @@ -0,0 +1,114 @@ +# SPEC-REVIEW-529-r1 + +**Issue:** #529 — «Тупик «conflicting wall identifiers»: план правится только после ручной чистки, а совет «Optimize plans» не помогает (по #527)» +**Этап:** spec (ревью ТЗ, PROCESS.md §2.4) +**Трек:** полный (заявленный автором критерий: «нет миграции конфига и новых compatibility-полей» — не пройден) +**Заход:** r1 · блокирующих циклов израсходовано 0 из 4 + +## Скоуп + +ТЗ живёт в теле issue #529 (единственный комментарий — аналитика S2, без отдельного текста ТЗ вне тела issue). Предмет: три дефекта, связанные с полем `room_drafts` в конфиге модели стен v10 — +1. `_migrate_room_drafts_to_partitions` (`custom_components/houseplan/wall_segment_model.py`) безусловно роняет коммит на пустом или непустом `room_drafts`, если это не первая миграция; +2. сторож устаревшего клиента `validate_wall_model_transition` (`custom_components/houseplan/validation.py:175-193`) не ловит случай, когда карточка эхом отправляет уже присланный ей `model_version=10`; +3. экспорт (`import_export._create_export`) падает тем же отказом, что и обычная запись. + +Контракт К1-К7 и AC1-AC8 переносят защиту #478 с безусловного `raise` на два места: миграцию (снимает пустой ключ молча, конвертирует непустой) и сторож (сравнивает присутствие `room_drafts` в заявке против сохранённого конфига, а не номера модели). + +## Как проверялось + +Чтением кода, без исполнения — на этапе ТЗ кода ещё нет, гейты не прогонялись (правка кода не входит в этот этап). + +- `docs/SCOPE.md`, `PROCESS.md` (§1-§10.3), `AGENTS.md` — контекст процесса и границы продукта. +- `docs/CONFIG-COMPATIBILITY.md`, особенно раздел «Ordinary wall chains — model v10 (#478)» — канонический текст защиты, которую ТЗ переписывает. +- `custom_components/houseplan/wall_segment_model.py:647-779` — `_migrate_room_drafts_to_partitions`, `commit_wall_segment_model`. +- `custom_components/houseplan/validation.py:142-220` — `validate_wall_model_transition`. +- `custom_components/houseplan/validation.py:1893-1960` — `_config_wall_segment_invariants` (часть `CONFIG_SCHEMA`). +- `custom_components/houseplan/websocket_api.py:1595-1700` (`ws_config_set`) и `:1990-2060` (`plan/optimize`) — реальная последовательность вызовов на сервере. +- `custom_components/houseplan/import_export.py:505-515, 670-690, 1815-1865` — порядок `commit_wall_segment_model` vs `CONFIG_SCHEMA` в экспорте/импорте. +- `src/wall-segment-model.ts:238-250, 700-800, 861-940` — TS-зеркало, `migrateLegacyRoomDraftsToPartitionsInPlace`, `commitWallSegmentModel`. +- `src/plan-optimizer.ts:33, 610-680` — клиентский оптимизатор, вызывающий тот же примитив. +- `src/houseplan-editor-runtime.ts:2000-2060` — где карточка ловит `WallSegmentModelError` и показывает тост до отправки запроса. +- `tests_backend/test_wall_segment_model.py:300-420`, `tests_backend/test_validation.py:1790-1860` — существующие свидетели, которые АС4/АС7 обещают сохранить зелёными. +- `src/i18n/en.json:743`, `src/i18n/ru.json:743` — подтверждение, что ключ `toast.wall_model_client_outdated` уже существует (i18n-раздел ТЗ корректен). +- `test/fixtures/282-wall-identity-parity.json`, `test/wall-segment-model.test.mjs:48` — паритетная фикстура, на которую опирается К6, существует. + +## Находки + +### Medium — К4/AC5 описывают механизм самолечения, который не соответствует реальному пути записи `config/set` + +**Файл:** тело issue #529, разделы «Контракт» (К4) и «AC» (AC5). + +**Сценарий отказа.** К4 гласит: «Если `room_drafts` есть и в заявке, и в сохранённом конфиге, заявка не отвергается: К1/К2 снимают ключ, и план перестаёт быть запертым». AC5 буквально формулирует свидетеля так: «`validate_wall_model_transition` не бросает; **последующий `commit_wall_segment_model` снимает ключ**». + +Это описывает последовательность вызовов, которой в продакшен-коде **не существует**. Реальный обработчик записи конфига — + +```python +# custom_components/houseplan/websocket_api.py:1663-1666 (ws_config_set._normalize) +def _normalize(candidate): + validate_wall_model_transition(candidate, data.get("config")) + return CONFIG_SCHEMA(candidate) +``` + +`commit_wall_segment_model` здесь **не вызывается вовсе** — ни напрямую, ни через `prepare_ordinary_summary_candidate` (`custom_components/houseplan/validation.py:2134-2149`, которая лишь оборачивает summary-panel-инварианты вокруг переданного `normalize`). После прохождения сторожа кандидат идёт прямо в `CONFIG_SCHEMA`, а внутри неё — в `_config_wall_segment_invariants`: + +```python +# custom_components/houseplan/validation.py:1906-1907 +if model >= 10 and "room_drafts" in space: + raise vol.Invalid("v10 config must not contain room_drafts") +``` + +Это **третья**, независимая точка защиты того же инварианта — отдельная от `_migrate_room_drafts_to_partitions` (К1/К2) и от `validate_wall_model_transition` (К3). Она реагирует на голое присутствие ключа безусловно, не спрашивая, первая ли это миграция и что лежит в сохранённом конфиге. Ни в теле ТЗ, ни в «Принятых технических предположениях», ни в списке «Затронутые файлы» эта функция не упомянута — при том что она находится в том же `validation.py`, который ТЗ уже трогает для К3, и охраняет ровно тот же инвариант («v10 config must not contain room_drafts»), что К1/К2 снимают. + +**Конкретное расхождение.** Пункт 2 «Принятых технических предположений» — «Порядок слоёв при записи сохраняется: валидация переходов идёт до миграции, поэтому К3 срабатывает раньше К2» — описывает несуществующий для `config/set` порядок: для этого писателя после `validate_wall_model_transition` идёт **не миграция (К2)**, а **`CONFIG_SCHEMA`**, которая содержит собственный, непереписываемый ТЗ запрет. + +**Почему это не голословно.** Проверено: `grep` `commit_wall_segment_model` по бэкенду показывает единственные вызовы — в `import_export.py` (экспорт/импорт, где порядок действительно «миграция → `CONFIG_SCHEMA`») и в `websocket_api.py:2050` (`plan/optimize`, и то условно — `if submitted_model < WALL_SEGMENT_MODEL_VERSION`). В `ws_config_set` вызова нет вообще (проверено чтением полного тела функции, `websocket_api.py:1595-1700`, и `prepare_ordinary_summary_candidate`, `validation.py:2134-2149`). + +**Смягчающее обстоятельство (почему не High).** Основной репортуемый симптом — блокировка при создании/переименовании комнаты — судя по всему устраняется на **клиентской** стороне: карточка запускает `commitWallSegmentModel` (TS-зеркало) локально ещё до формирования кандидата `config/set` (это подтверждает и S2-анализ: «запрос на сервер даже не уходит»). После правки К1/К2 в `src/wall-segment-model.ts` это локальное преобразование перестанет бросать исключение и снимет `room_drafts` **до** отправки на сервер — так что в реальности `config/set` от актуальной карточки, скорее всего, никогда не увидит `room_drafts` в кандидате, и сценарий К4/AC5 для `config/set` может быть попросту недостижим на практике. Но это — рассуждение ревьюера по коду, не то, что утверждает и проверяет ТЗ; АС5 как написан проверяет пару функций, которая никогда вместе не вызывается на сервере, и поэтому не является доказательством того, что «план перестаёт быть запертым» для реального обработчика `config/set`. + +**Что не покрыто ни одним AC.** Ни один из AC1-AC8 не бьёт по `_config_wall_segment_invariants` напрямую и ни один не прогоняет `validate_wall_model_transition` → `CONFIG_SCHEMA` в такой же последовательности, как это делает `ws_config_set` — то есть ровно тот путь, которым реально проходит запрос на переименование комнаты. Если реализация случайно оставит эту проверку нетронутой в сценарии, где она всё же достижима (например, будущий или уже существующий писатель, который — в отличие от `config/set` — передаёт `room_drafts` в `CONFIG_SCHEMA` до миграции), ни один свидетель этого не заметит. + +**Требуется от автора (правка ТЗ, тот же issue, без нового):** +1. Явно назвать `_config_wall_segment_invariants` (`validation.py:1906-1907`) в контракте — либо распространить К1/К4 на неё (снять безусловный запрет и там), либо явно объяснить и записать, почему её можно не трогать (например: «этот путь недостижим для реальных писателей, потому что X» — с указанием, для каких вызовов `CONFIG_SCHEMA` она всё ещё единственная линия обороны, если такие есть); +2. Поправить пункт 2 «Принятых предположений», чтобы он описывал реальный порядок вызовов `ws_config_set` (`validate_wall_model_transition` → `CONFIG_SCHEMA`, без миграции), а не гипотетический; +3. Либо добавить AC, доказывающий конкретно то, что реально происходит в `ws_config_set`/`prepare_ordinary_summary_candidate` для сценария К4, либо переформулировать АС5, чтобы он не утверждал вызов `commit_wall_segment_model` как механизм, работающий в этом писателе. + +Серьёзность — Medium, в скоупе задачи (это и есть тот самый compatibility-контракт, ради которого задача идёт полным треком): без High-находок это жёлтый вердикт, правка ТЗ и повторный (второй) заход. + +## Что проверено и корректно + +- **Продуктовая рамка (§7.1).** Раздел «Сценарий» называет персону (Home admin, редактирующий план — `docs/SCOPE.md`), поверхность (редактор плана) и момент («пытается создать комнату, переименовать её или выгрузить бэкап»). «До/После» сформулированы без терминов реализации. +- **К1/К2 фактически описывают существующий баг корректно.** Прочитан `_migrate_room_drafts_to_partitions` (`wall_segment_model.py:647-706`): проверка `"room_drafts" not in space` идёт первой, затем безусловный `raise WallSegmentMigrationError("duplicate-id", ...)` при `not initial_migration` — до какой-либо проверки длины `drafts`. Значит пустой список ловится тем же `raise`, что и непустой, ровно как заявлено в issue. +- **К3 корректно описывает текущий баг сторожа.** `validate_wall_model_transition` (`validation.py:183-193`): условие `old_model >= 10 and new_model < 10 and any("room_drafts" in space ...)` требует, чтобы присланный `model_version` был меньше 10 — а карточка эхом отправляет полученный от сервера `model_version=10`, что и позволяет устаревшему клиенту проскочить мимо этого сторожа, как и сказано в тексте. +- **AC4 совместим с существующим тестом.** `test_stale_v9_room_draft_write_over_v10_is_rejected_before_schema` (`tests_backend/test_validation.py:327-342`) отправляет `stale["model_version"] = 9` с `room_drafts`, а `previous` (сохранённый) без `room_drafts` — при переходе на условие «в заявке есть, в сохранённом нет» этот тест остаётся зелёным, ровно как обещает АС4. +- **АС6/К5 корректны для экспорта.** `import_export.py:505-515` (`_create_export`): `commit_wall_segment_model(raw_config)` вызывается **до** `CONFIG_SCHEMA(config)` — то есть к моменту схемной валидации `room_drafts` уже снят миграцией. После правки К1/К2 экспорт конфига с пустым или непустым `room_drafts` действительно перестанет падать; отдельная правка `import_export` не нужна, как и заявлено. +- **К6/АС3 — паритетная инфраструктура существует.** `test/fixtures/282-wall-identity-parity.json` и обращающийся к ней `test/wall-segment-model.test.mjs:48` присутствуют в дереве; TS-зеркало (`src/wall-segment-model.ts:744-748, 795`) содержит тот же баг (`hasOwnProperty` → безусловный `throw` при `!initialMigration`), что и Python-версия — правка К1/К2 в обоих зеркалах логически симметрична. +- **i18n раздел ТЗ точен.** `toast.wall_model_client_outdated` уже существует в `src/i18n/en.json:743` и `src/i18n/ru.json:743` — новых ключей действительно не требуется. +- **Клиентский оптимизатор использует тот же примитив.** `src/plan-optimizer.ts:33,648` вызывает `commitWallSegmentModelInPlace`/`migrateLegacyRoomDraftsToPartitionsInPlace` — то есть правка К1/К2 в TS-зеркале автоматически чинит и локальный расчёт «Optimize plans» на клиенте, без отдельной правки `plan-optimizer.ts`. Это подтверждает разумность К7 (шесть остальных веток `duplicate-id` и текст совета — вне скоупа): поведение Optimize для этой ветки меняется «бесплатно», как побочный эффект общей починки функции, а не нуждается в отдельном AC. +- **Откат, риски, миграция/совместимость, производительность, touch** — присутствуют, по существу, не содержат зазоров для конкретно описанного контракта (кроме находки выше). +- **Принятые технические предположения** сформулированы как «принято, можно оспорить» (пункты 1 и 3 корректны и не нуждаются в правке); зазор только в пункте 2, разобран в находке. + +## Чего не проверял + +- Не запускал `npx tsc --noEmit`, `npm test`, `npm run build`, backend pytest — этап ТЗ, продуктового кода ещё нет, оценивать нечего. +- Не проверял мутанты AC7 (`scripts/mutation-gate.mjs`, `scripts/backend-test-guard.mjs`) — до появления кода мутант не существует; это предмет код-ревью (PROCESS.md §2.7). +- Не проверял `_config_wall_segment_invariants` на предмет ДРУГИХ (не связанных с `room_drafts`) инвариантов — вне темы issue. +- Не трассировал полный клиентский путь `houseplan-editor-runtime.ts` для КАЖДОГО типа правки (только для создания/переименования комнаты, названных в issue) — достаточно для оценки контракта, но не является исчерпывающим аудитом фронтенда. +- Не проверял `_prepare_import_document` (`import_export.py:670-690`, выдача токена импорта) на предмет того, ловит ли она уже-конвертированный экспортированный файл — по коду он не будет содержать `room_drafts` после починки экспорта, так что сценарий, видимо, не реализуется, но это не входит в АС6 и явно не проверялось глубже. + +## Вердикт + +Жёлтый. Контракт в основном точен и проверяем (К1/К2/К3/К5/К6 подтверждены чтением кода, AC1-AC4, AC6-AC8 сформулированы однозначно и с указанным способом доказательства), но К4/AC5 описывают механизм самолечения через последовательность вызовов, которой не существует в реальном обработчике `config/set`, и не называют третью, отдельную точку защиты того же инваринта (`_config_wall_segment_invariants`, `validation.py:1906-1907`). Это ровно тот зазор, который §7.1 называет «догадкой, выданной за факт»: технически правдоподобно, но не проверено по коду автором и не покрыто ни одним AC. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `e408b6fb70a9` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0885d3e45b9242464d2fadc048fb64ce54c65a74` + ``` + git log --all --format='%H %T' | grep 0885d3e45b92 + ``` +- Тело issue: `097369f3a7fd490edce82469f02c715c2c084752b43ac793900719cd52f5c5b1` +- Вердикт конвейера: `yellow` · High 0