diff --git a/docs/reviews/SPEC-REVIEW-611-r1.md b/docs/reviews/SPEC-REVIEW-611-r1.md new file mode 100644 index 00000000..13554dd4 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-611-r1.md @@ -0,0 +1,195 @@ +# SPEC-REVIEW-611-r1 + +**Issue:** [#611](https://github.com/Matysh/houseplan-card/issues/611) — «Импорт пространства не перепривязывает +`vacuum.map_routes[].space`: на чужой установке импорт падает, на своей маршрут робота молча уводит на старый этаж» +**Этап:** spec (PROCESS.md §2.4) +**Трек:** `small` (лёгкий) — лимит циклов ревью ТЗ 2 +**Заход:** r1 · блокирующих циклов израсходовано 0 из 2 +**Ревьюер:** независимая сессия, без контекста автора ТЗ +**Вердикт:** жёлтый + +--- + +## Скоуп + +ТЗ живёт в теле issue #611 (раздел `## ТЗ`), не в `docs/specs/` — задача заведена после 2026-09-10 (#517). +Предмет ревью — контракт, критерии приёмки AC1–AC3, откат и раздел «Принятые предположения» этого текста, +проверенные против фактического кода `custom_components/houseplan/import_export.py`, +`validation.py`, `vacuum_routes.py` на HEAD `61292c9e`. + +Заход первый: раздела «Закрытие предыдущего раунда» и «Унаследовано из r0» не требуются (§2.10 применяется +со второго цикла). + +## Как проверялось + +Ручного тестирования и исполнения кода на этапе ревью ТЗ не предусмотрено (артефакта для исполнения ещё нет — +код не написан). Проверка велась чтением: + +1. Прочитаны `docs/SCOPE.md` (задача закрывает J4/J6 — многоэтажный план, перенос пространства без потери + привязки устройств), `PROCESS.md` §2.3/§2.4/§7.1, `AGENTS.md`. +2. Прочитано тело issue #611 целиком и единственный комментарий (аналитика: ценность 8/10, сложность 3/10, + P2, трек `small`, обоснование трека — все критерии §5 выполняются). +3. Построчно сверены три утверждения ТЗ против кода: + - `custom_components/houseplan/import_export.py:1284-1660` (`build_space_merge`) — цикл по входящим маркерам + (`1415-1497`), где сейчас ремапится `marker.space` (1428), `marker.room_id` (1432-1438), `radar` (1439-1454) + и `vacuum.segment_map` (1455-1466), но не `vacuum.map_routes[].space`; + - `custom_components/houseplan/validation.py:910-951` (`validate_marker_vacuum_routes`) — вызывается на + `merged_config` уже после сборки (`import_export.py:1627`), т.е. до неё контракт п.1 успевает выполниться; + - `custom_components/houseplan/vacuum_routes.py:106-146` (`effective_routes`) — подтверждает п.3 ТЗ: + legacy-маршрут получает `space` из параметра `dock_space`, а не из отдельного сохранённого поля, значит + ремап `marker.space` действительно достаточен для legacy-случая. +4. Прочитаны действующие тесты `tests_backend/test_ha_import_export.py`: + - `test_issue_162_space_export_drops_map_routes_of_other_spaces`, + `test_issue_443_space_export_preserves_explicit_empty_routes`, + `test_issue_162_full_export_round_trips_every_map_route`, + `test_issue_162_space_export_keeps_legacy_calibration_untouched` (строки 2678–2755) — подтверждают, что + #162/#443 покрывают **экспорт**, а не перепривязку при импорте, как и заявляет «Механизм» issue; + - `test_space_remap_covers_marker_id_layout_and_vacuum_segment_map`, + `test_issue_244_space_import_repairs_existing_target_refs_with_exact_map`, + `test_issue_265_space_import_repairs_unique_previous_generation_lineage`, + `test_issue_244_space_import_does_not_repair_target_while_source_exists` (1491–1600+) — раскрыли путь, + не упомянутый в ТЗ (см. находку M1 ниже). +5. Проверена ссылка AC3 на `scripts/mutation-registry.mjs`: файл действительно регистрирует backend-мутанты + против `custom_components/houseplan/import_export.py` (например, окрестности строк 4938, 5596–5686, + 6869–6898, 9474) — ссылка не выдумана, а корректна. +6. Проверено, нет ли в `docs/CONFIG-COMPATIBILITY.md` (раздел «Vacuum map routes (#162)», строки 163–195) и + `docs/ARCHITECTURE.md:409-420` утверждений, которые фикс сделал бы ложными, и не описывают ли они уже + ремап при импорте пространства отдельно от `map_routes` (нет — оба документа молчат об идентификаторах при + копировании пространства одинаково для `marker.space`, `room_id` и `map_routes`; заявление ТЗ «документация + не требуется» самосогласовано с этим прецедентом, а не произвольно). +7. Проверена терминология: `docs/USER-GUIDE.ru.md` использует «пространство», «этаж», «робот-пылесос» — ТЗ не + вводит новых терминов интерфейса (фикс не user-facing поверхность, только сообщение об ошибке импорта и + changelog). + +Дешёвые гейты (typecheck/test/build) и Validate на этом этапе неприменимы: продуктовый код ещё не написан, +класс A не тронут. + +## Находки + +### Medium — M1: контракт не называет второй код-путь с идентичным дефектом (`_repair_target_space_refs`) + +**Файл/место:** тело issue #611, раздел «Контракт» п.1–2 и «Критерии приёмки» AC1–AC2 (текст ТЗ); +затрагиваемый код — `custom_components/houseplan/import_export.py:1052-1270` (`_repair_target_space_refs`), +вызывается из `build_space_merge` на `import_export.py:1564`, то есть до валидации на `:1627` — тем же +контрактом п.1, который говорит «в `build_space_merge`», без уточнения. + +**Формулировка:** контракт п.1 и AC1/AC2 описывают перепривязку `vacuum.map_routes[].space` только для +**входящих** маркеров документа (цикл `1415-1497`, `marker` из `incoming.get("markers")`). Но `build_space_merge` +вызывает и второй, отдельный проход — `_repair_target_space_refs` — который чинит **уже существующие в целевой +конфигурации** маркеры, чей `marker.space`/`room_id`/`vacuum.segment_map` ссылались на мёртвый (более не +`live`) ID, если этот ID лineage-разрешим к только что импортированному пространству (тесты +`test_issue_244_*`, `test_issue_265_*`). В этом проходе (`import_export.py:1152-1179`) `marker.space` (1154), +`marker.room_id` (1157-1165) и `vacuum.segment_map` (1166-1179) чинятся, а `vacuum.map_routes[].space` — нет: +для этого прохода в файле вообще нет строки, аналогичной 1455-1466 из первого цикла. Ни контракт, ни AC1-AC3, +ни «Принятые предположения» не упоминают этот путь — не как исключённый явно, а никак. + +**Почему это тот же дефект, а не гипотетический:** тест `test_issue_244_space_import_repairs_existing_target_refs_with_exact_map` +(строки 1507-1537) прогоняет ровно такой маркер — `"vacuum": {"segment_map": {"12": "living"}}` в целевой +конфигурации, чей `room_id`/`space` чинятся через лineage/exact-map. Если у такого же целевого маркера вместо +(или вместе с) `segment_map` стоит явный `map_routes` со `space`, указывающим на тот же старый ID, после починки +`marker.space` он укажет на новое пространство, а `vacuum.map_routes[0].space` — на старый, уже мёртвый ID. +Дальше — тот же исход, что описывает issue: либо `invalid_vacuum_map_route` (если старый ID действительно +исчез из `spaces`), либо (если по случайности ID ещё жив в другом контексте — на практике не бывает, `used` +исключает коллизии) молчаливо неверный этаж. Это тот же класс дефекта, что и заявленный в issue, просто +триггер — не сам факт копирования пространства, а последующий ре-импорт после того, как исходное пространство +уже когда-то было удалено (детач) и лineage-таблица его помнит. + +**Почему это не придирка к незаявленному сценарию:** контракт п.1 буквально говорит «в `build_space_merge`» +(имя функции, а не «в цикле по входящим маркерам»), и `_repair_target_space_refs` — код внутри той же функции, +вызванный до той же строки валидации (`:1627`), к которой апеллирует контракт. Формулировка «до запуска +`validate_marker_vacuum_routes`» относится к обоим проходам одинаково буквально. + +**Последствие для процесса:** это Medium, поскольку не блокирует реализуемость и проверяемость AC1-AC3 как +написано — они по-прежнему однозначны и доказуемы отдельно от этого пробела. Но раз пробел лежит в скоупе той +же задачи (тот же баг-класс, тот же файл, тот же вызов внутри `build_space_merge`, соседний код уже показывает +правильный паттерн на `segment_map`), это Medium **в скоупе**, а не повод заводить отдельный issue: чинится +здесь же — либо явным расширением контракта/AC (AC4 на путь `_repair_target_space_refs`), либо явной записью +в «Принятые предположения», почему этот путь сознательно не покрывается в #611 (например: «до отдельного +issue, потому что репродукция требует предварительного детача пространства — двухшаговый сценарий вне +аудита 22.09»). Молчание — не то же самое, что решение. + +**Не High:** ни один из AC1-AC3 не становится невыполнимым или недоказуемым без этого расширения; проблема — +в полноте контракта относительно собственной формулировки «в `build_space_merge`», а не в его противоречивости. + +--- + +## Проверено и корректно + +- **Механизм воспроизведён точно.** `old_space_id`/`new_space_id` (`import_export.py:1338,1342`), + `details["space_id"] = new_space_id` (`:1648`) — ТЗ ссылается на реальные имена и реальный порядок вызовов, + не выдуманные. +- **AC1/AC2 доказуемы уже сейчас**, без домыслов о будущем API: `create_export(kind="space")`, + `parse_document`, `build_space_merge(..., same_source=...)` — существующие публичные точки входа модуля, + тест-файл `tests_backend/test_ha_import_export.py` уже содержит соседние тесты той же формы (тестовый + паттерн `_document(tmp_path, "space")` + `build_space_merge(...)` используется многократно, включая + `test_issue_244_*`/`test_issue_265_*`). +- **П.3 контракта (legacy-маршрут вычисляется из `marker.space`) подтверждён кодом** — + `vacuum_routes.py:106-146`, `effective_routes()` берёт `dock_space` как параметр, отдельного сохраняемого + `route.space` для legacy нет. Это не домысел, а точное описание существующего инварианта. + Первое предложение п.3 («явный список не создаётся из legacy при импорте») тоже подтверждено — этот путь в + `build_space_merge` не создаёт `map_routes` там, где их не было. +- **П.4 (виртуализация теряет `vacuum` целиком) подтверждён** — `import_export.py:1489-1496`, `"vacuum"` в + списке полей, которые `pop`-аются при `duplicate_policy == "virtual"`. Ремап `map_routes`, если он случится + раньше проверки на 1467, для виртуализированного маркера станет мёртвым кодом, но не изменит наблюдаемый + результат — контракт корректно не создаёт здесь новой обязанности. +- **Ссылка на `scripts/mutation-registry.mjs` в AC3 реальна** — файл уже регистрирует backend-мутанты именно + против `import_export.py`, формат (`guard: 'python3 -m pytest tests_backend/...'`) соответствует остальному + реестру. +- **Раздел «Принятые предположения» — не выдача догадки за факт.** Утверждение об отказе перепривязывать + «похожие», а не точно совпадающие ID, явно помечено как сохранение существующего fail-closed поведения и + обосновано (повреждённые/вручную изменённые документы) — это ровно тот случай, когда решение принято и + явно записано, а не спрятано. +- **DoR-чеклист §2.5 закрыт по всем пунктам, кроме найденного M1:** i18n (нет новых ключей — верно, ошибка + использует существующий код `invalid_vacuum_map_route`), touch/performance (нет влияния — верно, чисто + backend-импорт), откат (revert одним коммитом — реалистично, схема хранения не меняется, `CONFIG_SCHEMA` + уже допускает `map_routes` через существующий `vol.Optional`), i18n en/ru changelog — план назван. +- **Трек `small` подтверждён.** Один backend-контракт (`build_space_merge`), никакой миграции конфигурации, + никакого нового UX-контракта, i18n/touch/perf не затронуты — критерии §5 действительно выполняются все + одновременно, а не выборочно. +- **Продуктовая рамка отвечает на оба обязательных вопроса §7.1.** Сценарий: администратор House Plan + переносит/копирует пространство многоэтажного дома, где настроен пылесос с явными маршрутами (J4/J6 из + `docs/SCOPE.md`). Что человек увидит: на чужой установке импорт пространства с настроенными маршрутами + вместо падения `invalid_vacuum_map_route` проходит; на своей — робот на новой копии не «наследует» этаж + старого оригинала молча. + +## Чего не проверял + +- **Исполнение кода не проводилось** — на этапе ревью ТЗ кода ещё нет; все проверки — чтением существующего + кода и тестов на HEAD `61292c9e`, не запуском. Это ожидаемо для этапа spec (артефакта для запуска нет), + отмечается явно по правилу «проверено чтением, не исполнением». +- **Не проверялась производительность** сборки `build_space_merge` на больших конфигурациях — ТЗ прямо + заявляет «без влияния», и это правдоподобно для точечной правки одного цикла по маркерам без изменения + сложности; отдельного перф-профиля для backend-импорта в проекте не предусмотрено. +- **Не проверялось**, существуют ли реальные, уже сохранённые у пользователей конфигурации, где сработал бы + путь M1 (`_repair_target_space_refs` + явный `map_routes` на целевом маркере) — это гипотетический, но + code-grounded сценарий; фактическая частота не оценивалась и не требуется на этапе ревью ТЗ. + +## Материал раунда + +- Issue: #611, тело проверено на состоянии, зафиксированном при выдаче задачи ревьюеру. +- `sha256` нормализованного (UTF-8, как получено через `gh issue view --json body`) тела issue: + `94e2253da2c31f34919377a64987560e85945a765281dac78e320a29231f1553` (4369 байт). +- Код-база: `61292c9ea05f0d7d3a98392c81f3be89f2706c7c` (рабочая копия соответствует HEAD). +- Комментарий аналитики учтён (ценность 8/10 · сложность 3/10 · P2 · тип bug · трек `small`). + +## Итог + +**Вердикт: жёлтый.** AC1-AC3 сформулированы однозначно и доказуемо, механизм и все технические ссылки в ТЗ +точны и проверены построчно против кода — это добротная работа. Единственная находка (M1, Medium, в скоупе) +не рушит написанные критерии, но оставляет незакрытым второй код-путь с тем же классом дефекта внутри той же +функции. Возврат автору: расширить контракт/AC на `_repair_target_space_refs`, либо явно и обоснованно +исключить этот путь в «Принятых предположениях». + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `61292c9ea05f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `008da06ab0a721f37308e9af3cf9fbfad2c659de` + ``` + git log --all --format='%H %T' | grep 008da06ab0a7 + ``` +- Тело issue: `e67628cb6a8a9b34c284381c57eb7ef1105361614b8bf1341c41e4af19b87f19` +- Вердикт конвейера: `yellow` · High 0