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

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 снимает ключ».

Это описывает последовательность вызовов, которой в продакшен-коде не существует. Реальный обработчик записи конфига —

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

# 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