From 1241b9499c7b33d4a4b905d884f60ae6db74690c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 23 Sep 2026 01:59:49 +0000 Subject: [PATCH] docs: review document for #611 Issue: #611 User-Visible: no --- docs/reviews/CODE-REVIEW-611-r1.md | 183 +++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-611-r1.md diff --git a/docs/reviews/CODE-REVIEW-611-r1.md b/docs/reviews/CODE-REVIEW-611-r1.md new file mode 100644 index 00000000..65825261 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-611-r1.md @@ -0,0 +1,183 @@ +# CODE-REVIEW-611-r1 + +Issue: #611 · Этап: code · Заход: r1 · Материал: `9f82960c2c0a6f2e718d5a4422582a24be4c40e8` +(`origin/dev..HEAD` = один коммит, рабочая копия на нём проверена: `git rev-parse HEAD` == материал) + +## Скоуп + +Один коммит `9f82960c` поверх `dev`: + +- `custom_components/houseplan/import_export.py` — два блока: (1) в + `build_space_merge` перепривязка `vacuum.map_routes[].space` входящего + маркера при копировании пространства (AC1); (2) в + `_repair_target_space_refs` та же перепривязка для маршрутов уже + существующих целевых маркеров через `resolve_space`/`replace` (AC2). +- `tests_backend/test_ha_import_export.py` — 6 новых тестовых функций (часть + параметризована), покрывающих AC1-AC2 и негативные формы (`map_routes: []`, + legacy-калибровка). +- `scripts/mutation-registry.mjs` — два новых мутанта для AC3. +- `docs/CHANGELOG.md` / `docs/CHANGELOG.ru.md` — запись в `Unreleased`, в том + же коммите (`User-Visible: yes`, трейлеры `Issue: #611` / `User-Visible: yes` + на месте). + +ТЗ на входе — зелёное r2 (Medium M1 из r1 закрыт добавлением контракта п.2 и +AC2 под целевой ремонт). Задача помечена `small`, `bug`, `vacuum`. Соответствие +`docs/SCOPE.md`: правка держит J6 «Keep the plan true as the home evolves» +(копирование/импорт пространства не должно портить ссылки устройства) — +попадает в скоуп. + +## Как проверялось + +Диапазон материала — один коммит, полный разбор (не дельта: это первый заход +кода по этой задаче). + +| Гейт | Статус | Как | +|---|---|---| +| `npx tsc --noEmit` / `npm test` / `npm run build` (бандл x3) | не гонял | Validate на `9f82960c` зелёный (ссылка в задаче ревью), diff не трогает `src/**` — эти гейты не могли покраснеть от этого коммита | +| `python -m pytest tests_backend -q` | подтверждён | Validate/`Бэкенд: pytest в Home Assistant`, тот же SHA: `867 passed, 1 skipped` (job `107011332643`, лог сверен вручную) | +| `node scripts/check-docs.mjs` | не требуется | diff не трогает `src/**` | +| `npm run invariants -- --config …` | не требуется | diff не трогает рёбра/толщину/`layout`/`open_spans`; `marker.space` в диффе не редактируется — редактируется только `vacuum.map_routes[].space`, не геометрическая ссылка | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | прогнал | вывод: «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут). Browser-smoke этим диффом не выбираются — выбирать нечего» | +| `npm run golden:verify` | не требуется | diff не меняет рендер/геометрию/стили/слои (backend-only) | +| `python -m pytest tests_backend -q` — целевые новые тесты | прочитаны построчно | 6 функций в `test_ha_import_export.py`, see «Находки» и «Проверено и корректно» | +| Мутанты AC3 | подтверждены по логам CI на этом SHA | `space-import-keeps-incoming-vacuum-route-space` → «ok … покраснел на мутанте» (job `107011273739`); `space-import-keeps-target-vacuum-route-space` → «ok … покраснел на мутанте» (job `107011273859`) | +| Один источник чисел | н/п | diff не добавляет и не меняет отображаемую пользователю величину | + +Пришлось подтверждать `pytest`/мутанты через логи существующего зелёного +прогона CI на точном SHA (`https://github.com/Matysh/houseplan-card/actions/runs/35807412783`), +а не локальным повтором: локальная песочница ревью не имеет установленного +`pytest-homeassistant-custom-component` (`pip install -r +tests_backend/requirements.txt` не выполнялся намеренно — тяжёлая установка не +входит в дешёвые гейты, а нужный прогон уже зелёный на материале). Это не +самостоятельный прогон, а сверка с логом; там, где лог достаточно однозначен +(текст «ok … покраснел на мутанте», финальная строка `867 passed, 1 skipped`), +считаю доказательство состоявшимся. + +## Защитные AC — таблица «чем краснеет» + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| AC1 (входящий маршрут получает новый ID до валидации, обе установки) | `test_issue_611_incoming_route_follows_fresh_space[foreign]` / `[same-instance]` | мутант `space-import-keeps-incoming-vacuum-route-space` (оставляет `route["space"] = old_space_id`) → тест красный на CI (`9f82960c`, job 107011273739) | +| AC2, ветка exact/lineage (целевой маршрут ремонтируется) | `test_issue_611_target_route_follows_safe_space_repair[exact]` / `[lineage]` | мутант `space-import-keeps-target-vacuum-route-space` (`route["space"] = route.get("space")`, ремонт молча отбрасывается) → тест красный на CI (job 107011273859) | +| AC2, ветка live (живая ссылка не трогается) | `test_issue_611_live_target_route_is_not_rebound` | **не названо** — см. находку L1 | +| AC2, ветка ambiguous (неоднозначная сохраняется в `preservedUnresolved`) | `test_issue_611_ambiguous_target_route_is_preserved_and_reported` | **не названо** — см. находку L1 | +| AC3 (мутационный гард сам по себе) | запись в `scripts/mutation-registry.mjs`, два id | оба подтверждены зелёными (красными на мутанте) прогонами в CI на материале — см. выше | + +## Находки + +### Low L1 — ветки live/ambiguous AC2 не имеют собственного мутанта + +`_repair_target_space_refs` (`import_export.py:1180-1190`) вызывает для +маршрутов ту же пару `replace(...)` / `if mapped is not None: route["space"] += mapped`, что и существующий код `marker.space` (`:1148-1150`) и +`segment_map` (`:1170-1179`). Гипотетический мутант, убирающий охрану `if +mapped is not None` (например, безусловное присваивание `route["space"] = +mapped`, включая `None` для live/ambiguous случаев), сломал бы +`test_issue_611_live_target_route_is_not_rebound` и +`test_issue_611_ambiguous_target_route_is_preserved_and_reported` — но такого +мутанта в реестре нет. Backend pytest относится к «дорогим» гейтам, для +которых §2.7 требует именно мутанта, а не просто прогона со снятой защитой +вручную; формально пустая ячейка «чем краснеет» для этих двух под-критериев +AC2 — по правилу документа находка, а не примечание. + +Не блокирует и оцениваю как Low, а не Medium, по двум причинам: (1) это тот же +паттерн, что уже используется непокрытым мутантом кодом для `marker.space` и +`segment_map` в этой же функции (проверено чтением: ни для одного из них в +`scripts/mutation-registry.mjs` тоже нет мутанта на охрану `if mapped is not +None`) — новый код не создаёт новый класс риска, а наследует существующий, +уже принятый в проекте; (2) текст AC3 (сам прошедший зелёное ТЗ-ревью r2) +явно сужает обязательный мутант до сценария «перепривязка удалена/обойдена», +а не до полного покрытия всех пяти исходов резолвера — так что задача как +написана не требовала этого мутанта. Фиксирую как оставленный риск, а не +причину для жёлтого вердикта. + +## Проверено и корректно + +- **Порядок операций (AC1, п.1 контракта).** Перепривязка входящих маршрутов + (`import_export.py:1489`) выполняется до `validate_marker_vacuum_routes` + (`:1653`) — между ними только сборка `output_layout`, `CONFIG_SCHEMA` и + остальные валидаторы. Проверено чтением, порядок вызовов линейный, без + ветвлений, которые могли бы его переставить. +- **Отсутствие двойного учёта.** `_repair_target_space_refs` работает только + по `current_config["markers"]` (уже существующие в целевой конфигурации + маркеры), а входящие маркеры добавляются отдельно как `output_markers` + после вызова репара (`:1633`) — один и тот же маршрут не может быть учтён + и как `incoming`, и как `target`. Проверено чтением. +- **Ambiguous корректно детектируется на реальных ID.** Прогнал вручную + логику `canonical_import_root("space", …)` на входных данных теста + `test_issue_611_ambiguous_target_route_is_preserved_and_reported`: и живой + `space_ground_aaaaaaaa`, и кандидат-лайнидж `space_ground_bbbbbbbb` сводятся + к общему корню `ground`, из-за чего `live.get(root)` непусто и резолвер + обязан вернуть `"ambiguous"`, а не угадать. Тест это утверждает и это + математически верно для данного ввода — не совпадение параметров теста. +- **Не-route формы `vacuum` не трогаются молча (контракт п.4).** + `map_routes: []` и legacy `calibration`-блок без явного списка проверены + тестом `test_issue_611_non_route_vacuum_shapes_stay_unchanged` через + побайтовое сравнение `marker["vacuum"] == vacuum`; чтением подтверждено, что + оба новых блока кода защищены `isinstance(…, list)` и просто не находят, + что перебирать. +- **`reference_report` считает по контракту п.3.** `_report_remap`/ + `_report_reference` — плоские счётчики по `(bucket, category)` + (`import_export.py:978-1000`), проверено чтением; ожидания тестов + (`remapped.incoming/target` содержат ровно `marker.vacuum.map_routes.space: + 1`, `preservedUnresolved` — то же) соответствуют этой структуре без + дополнительных допущений. +- **`repaired_target_refs` включает ремонт маршрута (контракт п.3, «входит в + общий `repaired_target_refs`»).** `replace()` инкрементирует общий + `nonlocal repaired` независимо от категории (`:1106-1109`), поэтому тест + `test_issue_611_target_route_follows_safe_space_repair` вправе ожидать + `repaired_target_refs == 2` (marker.space + маршрут) — проверено чтением и + совпадает с прогоном (867 passed на CI). +- **Changelog/трейлеры.** `Issue: #611`, `User-Visible: yes` в теле коммита; + оба changelog обновлены в том же коммите, формулировки разделяют «чужая + установка» / «своя установка» так же, как ТЗ и AC1 — не более и не менее + того, что реально видно пользователю. +- **AC3 буквально.** Оба мутанта убирают именно «перепривязку» + (`route["space"] = new_space_id` → неизменное значение; ремонт → игнор + результата резолвера) — ровно то, что требует формулировка AC3, и оба + подтверждены красными на мутанте / зелёными как гард в CI на материале. + +## Чего не проверял + +- `npx tsc --noEmit`, `npm test` (frontend), `npm run build` — не перегонял + сам; полагаюсь на зелёный Validate на этом SHA и на то, что diff не + затрагивает `src/**`/frontend-тесты. +- `node scripts/check-docs.mjs`, `npm run golden:verify`, браузерные смоки — + не запускал: `smoke-select.mjs` подтвердил, что frontend-диффа нет, ни один + из этих гейтов не мог покраснеть от backend-only изменения. +- `pip install -r tests_backend/requirements.txt` / локальный + `python -m pytest tests_backend -q` — не выполнял в этой сессии (нет + `pytest-homeassistant-custom-component` в песочнице, а установка — тяжёлая + операция вне «дешёвых» гейтов); доказательство взято из логов зелёного + прогона `Бэкенд: pytest в Home Assistant` на точном материале ревью. +- Мутационный гейт целиком (`node scripts/mutation-gate.mjs`) — не гонял + целиком локально (тот же барьер: нет `pytest`); адресно подтвердил ровно + два новых мутанта по логам job'ов `107011273739` и `107011273859` того же + workflow run на материале. +- Ручного тестирования интеграции с реальным Home Assistant/пылесосом не + проводилось — вне периметра этого гейта; полагаюсь на pytest-харнес и + чтение кода. + +## Вердикт + +Зелёный. High: 0, Medium: 0. Одна находка Low (L1) — оставлена как +задокументированный, не блокирующий риск с обоснованием (наследует +непокрытый мутантами паттерн, уже существующий в той же функции для +`marker.space`/`segment_map`; AC3 как согласовано в ТЗ не требовал большего). +AC1-AC3 доказаны по коду и тестам, тесты умеют падать (подтверждено логами +CI на материале — оба новых мутанта красят соответствующие тесты), changelog +и трейлеры на месте. + +--- + + + +## Материал раунда + +- Ветка: `issue/611-vacuum-route-space-remap`, коммит `9f82960c2c0a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d96a15b45c6eac57fd41f642edf3ae6d278a9a01` + ``` + git log --all --format='%H %T' | grep d96a15b45c6e + ``` +- Тело issue: `bb14ef7839530fe63fc2aedfdcf59e5433cbb80fa20c2a3a84a0971f818fb78e` +- Вердикт конвейера: `green` · High 0