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

SPEC-REVIEW-335-r1

Issue: #335 «Трейлы пылесосов: осиротевшие маркеры навсегда остаются в store, точки после рестарта не сохраняются» Этап: ТЗ на ревью (PROCESS.md §2.4), лёгкий трек (small) Заход: r1 · блокирующих циклов израсходовано 0/2 (лимит для лёгкого трека — 2, §2.4/§2.10) Материал: тело issue #335 на момент ревью (единственная правка — стартовый комментарий аналитика, без изменения ТЗ), репозиторий на dev @ 79aa0a90. Ветки issue/335-* не существует — код ещё не написан, ревью строго ТЗ-уровня.

Скоуп ревью

ТЗ живёт в теле issue (лёгкий трек, §5): проблема · аналитика и границы · контракт реализации · AC1–AC4 · откат · вне скоупа. Проверялось:

1. соответствие докладу о текущем поведении реальному коду (custom_components/houseplan/trails.py, websocket_api.py, src/devices.ts, src/houseplan-editor-runtime.ts);
2. согласованность с каноническим docs/VACUUM.md («Storage and lifecycle»);
3. однозначность и доказуемость каждого AC;
4. попадание задачи в docs/SCOPE.md (job J6 — «Keep the plan true as the home evolves»: трейлы — часть серверного состояния плана, задача чинит его корректность после редактирования — в скоупе);
5. отсутствие догадок, выданных за факт.

Как проверялось

Чтение исходников без исполнения (ревью ТЗ — код для задачи не существует):

• custom_components/houseplan/trails.py полностью (TrailBook, TrailRecorder.async_refresh/_sample/_on_state/async_delete);
• custom_components/houseplan/websocket_api.py: ws_config_set (создание/пропуск _refresh_trail_recorder, ветка no-op, ветка ошибки), _refresh_trail_recorder, ws_trail_delete, import/recovery cleanup (live_marker_ids + async_delete в цикле);
• custom_components/houseplan/validation.py (MARKER_SCHEMA, комментарий про tombstone), import_export.py (live_layout, разделение removed/live id);
• src/devices.ts (deletePlanMarkerRecords), src/houseplan-editor-runtime.ts (обработчик удаления маркера, вызов houseplan/trail/delete);
• tests_backend/test_trail_recorder.py (существующий тест test_refresh_never_tracks_a_removed_marker_even_with_stale_vacuum_fields);
• docs/VACUUM.md целиком, раздел «Storage and lifecycle» — построчно против текста issue.

Гейты не запускались — на этапе ТЗ кода нет, typecheck/test/build не относятся к предмету ревью.

Находки

High-1 — определение «маркер существует» игнорирует tombstone removed: true, из-за чего AC1 не закрывает сценарий, описанный в «Проблема» №1

Где: тело issue #335, раздел «Аналитика и границы», строка:

«Маркер существует» определяется так же, как в существующем import-пути: его id присутствует в успешно сохранённом config.markers. Остальные поля маркера не влияют на очистку.

и раздел «Критерии приёмки», AC1.

Что не так. Обычное удаление маркера через редактор НЕ убирает его id из config.markers. deletePlanMarkerRecords (src/devices.ts:914-934) для любого не-virtual маркера (все vacuum-маркеры такие — привязка device:/entity:vacuum.*) добавляет вместо удаления tombstone-запись {id, binding, removed: true, hidden: true}, которая остаётся в массиве навсегда — это осознанное поведение, задокументированное дважды:

• комментарий к функции: «Non-virtual deletion leaves a hidden tombstone so an older cached card degrades to a hidden marker instead of visibly resurrecting it»;
• custom_components/houseplan/validation.py:1673-1675: «A binding-level tombstone: not rendered or aggregated, but retained so automatic discovery does not put a deleted device straight back».

Таким образом, для штатного «удалить маркер в редакторе» id не исчезает из config.markers — он остаётся с removed: true. По формулировке AC1 («остальные поля маркера не влияют на очистку») такой tombstoned-маркер считается «существующим» вечно, и предложенная server-side очистка при config/set для него никогда не сработает — ни сразу, ни при любом последующем сохранении.

Это прямо противоречит собственному коду проекта: TrailRecorder.async_refresh (trails.py:169-171) уже трактует removed: True как «маркера нет» для целей живого трекинга (if m.get("removed") is True: continue), и это закреплено существующим тестом test_refresh_never_tracks_a_removed_marker_even_with_stale_vacuum_fields (tests_backend/test_trail_recorder.py:530-550). Тот же водораздел removed/не-removed — устоявшийся в кодовой базе способ отличать «живой» маркер от «удалённого»: import_export.py:172-183 (live_layout) строит removed = {id : removed is True} и live = {id : removed is not True} именно для этой цели.

Как это расходится с реальным поведением, которое чинит issue. Сегодня штатное удаление маркера в редакторе УЖЕ дёргает отдельный best-effort вызов houseplan/trail/delete сразу после config/set (src/houseplan-editor-runtime.ts:7990, обёрнут в .catch(() => undefined), без повтора) — это ровно то, что документирует docs/VACUUM.md: «Deleting a vacuum marker removes its layout and server trails». Формулировка «Проблема» №1 в issue («Удаление маркера через config/set сохраняет новый конфиг, но не удаляет его записи… Очистка сейчас выполняется только для полного импорта») верна только в узком смысле «один config/set сам по себе не чистит», но не описывает систему целиком: реальный риск не в том, что очистки нет вовсе, а в том, что единственная сегодняшняя очистка — это ничем не подстрахованный клиентский вызов, который тихо проглатывает свою ошибку и не повторяется (закрытая вкладка, обрыв сети, переход со страницы между config/set и trail/delete). Именно этот сценарий и должен закрывать серверный self-heal — но при определении «id присутствует, поле removed не важно» он не закрывается, потому что маркер так и остаётся в массиве как tombstone.

Чем это грозит реализации. Автор технически МОЖЕТ реализовать AC1 буквально — тест соберёт config.markers, из которого маркер целиком исключён (не тombstone, а честный «hard drop»), убедится, что после такого config/set трейл вычищен, и AC1 позеленеет. Но это докажет очистку для сценария, который в продукте почти не встречается штатно (ближайший реальный аналог — смена id при ребиндинге маркера на другое устройство, houseplan-editor-runtime.ts:7866-7877, либо полный импорт/восстановление), а не для «удалил маркер пылесоса в редакторе», то есть не для того случая, который явно назван как Проблема №1.

Предлагаемая правка (на усмотрение автора, ревьюер технические решения не диктует, но альтернатива нужна для развязки): определить «маркер существует» как «id присутствует в config.markers и removed is not True» — то есть тем же правилом, что уже использует async_refresh для pairs, и тем же, что import_export.live_layout использует для «live». При таком определении:

• AC1 естественно закрывает реальный сценарий (маркер тombstoned → на этом же или следующем config/set его трейл вычищается сервером, независимо от того, успел ли отработать клиентский trail/delete);
• AC2 не меняется по сути (маркеры без tombstone по-прежнему сохраняют трейл);
• общий helper (пункт 1 «Контракта реализации») при переносе на import-путь физически меняет его текущее поведение (сегодня import-очистка тоже игнорирует removed) — это нужно явно назвать в AC4 как ожидаемое расширение регрессионного покрытия, а не молчаливый побочный эффект.

Серьёзность: High, блокирует. Без ответа на этот вопрос AC1 недоказуем как критерий приёмки той проблемы, которую issue заявляет; принять ТЗ в таком виде — значит зафиксировать критерий, который не отличает «починили» от «не починили» в основном сценарии.

Medium-1 (в скоупе) — release-артефакты / User-Visible не названы

Где: раздел «Откат» — покрывает миграцию и обратимость, но нигде в ТЗ не указано, User-Visible: yes или no, и нужен ли docs/CHANGELOG.md/.ru.md. DoR (§2.5) требует явного решения по этому пункту, лёгкий трек его не отменяет.

Оба бага меняют наблюдаемое поведение трейлов (меньше осиротевших раздутых записей; после рестарта HA трейл не теряет первую точку) — это не чисто внутренний рефакторинг. Разумное значение по умолчанию — User-Visible: yes с короткой записью в оба changelog (бажный фикс, доступный пользователю), но это должен подтвердить автор при переходе в S5-ready, а не додумывать на этапе реализации.

Серьёзность: Medium, в скоупе задачи — чинится добавлением одной строки в ТЗ (или в AC4/«Откат»), отдельного цикла не требует сама по себе, но по правилу §2.4 при отсутствии High это уже жёлтый вердикт, а не самостоятельное основание для другого циклa.

Что проверено и корректно

• Bug #2 (первая точка после рестарта не сохраняется) — подтверждено чтением: TrailRecorder.async_refresh (trails.py:190-191) вызывает self._sample(src, time.time()) и отбрасывает булев результат; сохранение (_schedule_save) и событие houseplan_trail_updated в этой ветке никогда не запускаются, в отличие от _on_state (строки 347-351), где тот же результат используется. Контракт AC3 («агрегировать результат стартовых _sample() и при фактическом изменении запускать тот же путь сохранения и уведомления») точно описывает нужную правку и технически осуществим без искусственных допущений.
• AC2 (no-op / ошибка не запускают очистку) — подтверждено чтением ws_config_set: семантический no-op (msg["config"] == data.get("config")) возвращает результат до строки, где вызывается _refresh_trail_recorder; ветки conflict/invalid_format/too_large/missing_plan возвращают ошибку тем же путём, до _refresh_trail_recorder. Значит требование AC2 «неуспешный/no-op write не запускает очистку» соответствует уже существующему потоку управления и не требует новых допущений — только не сломать его при добавлении шага очистки.
• Общий helper для import/recovery (пункт 1 контракта) — реальный прецедент найден и процитирован верно: websocket_api.py (полный импорт) строит live_marker_ids из target_config.get("markers") и вызывает recorder.async_delete(marker_id) для каждого отсутствующего id; вынесение этой логики в общий helper — разумный, проверяемый рефакторинг (при условии исправления High-1).
• Границы «вне скоупа» согласуются с docs/VACUUM.md: формат/лимиты трейлов, debounce/throttle интервалы и подписки не описаны как изменяемые нигде в контракте реализации — согласовано.
• Откат — единственный issue-коммит, обратной миграции нет, формат store не меняется — корректно и достаточно для этой задачи.
• Job/скоуп по SCOPE.md — задача не создаёт нового поведения, а чинит уже задокументированный docs/VACUUM.md контракт («деление маркера удаляет трейлы», «трейл переживает рестарт») — попадает в J6, вопросов к продуктовой рамке нет.
• Аналитика лёгкого трека — критерии §5 (сложность/риск ≤3, одна поверхность — backend lifecycle трейлов, без миграции конфига, без нового UX-контракта, без влияния на touch/perf) действительно выполняются одновременно; выбор small не оспаривается.

Чего не проверял

• Тексты AC не проверялись на предмет запуска реального pytest — кода ещё нет, оценка чисто по тексту ТЗ и текущему состоянию dev.
• Не проверялся более широкий побочный эффект правки на _source_health (пункт «Подписки, source-health… не меняются» принят на слово — код _refresh_source_health действительно не тронут контрактом реализации, но фактическое отсутствие регрессии подтвердит только код-ревью).
• Не оценивалась многопоточная/многоклиентская гонка config/set вокруг предложенной очистки — вне AC, оставлено на усмотрение реализации при условии, что write_lock/_refresh_lock (уже существующие) её накрывают.

Вердикт

Единственная блокирующая находка (High-1) — определение «существования» маркера для очистки не покрывает основной сценарий, который issue называет проблемой №1 (обычное удаление vacuum-маркера в редакторе), потому что такое удаление оставляет removed: true tombstone, а не убирает id из config.markers. Пока это не исправлено (или явно не оспорено автором с указанием, почему raw-presence — правильный выбор), AC1 не доказывает то, что заявлено в «Проблема».

Medium-1 (release-артефакты/User-Visible) — в скоупе, чинится одной строкой, самостоятельного цикла не образует.

Вердикт: жёлтый · заход r1 · блокирующих циклов 0/2 · High: 1 · Medium: 1 → в задаче