mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 04:38:55 +00:00
claude[bot]
@@ -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`, либо явно и обоснованно
|
||||
исключить этот путь в «Принятых предположениях».
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `dev`, коммит `61292c9ea05f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `008da06ab0a721f37308e9af3cf9fbfad2c659de`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 008da06ab0a7
|
||||
```
|
||||
- Тело issue: `e67628cb6a8a9b34c284381c57eb7ef1105361614b8bf1341c41e4af19b87f19`
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user