docs: review document for #335

Issue: #335
User-Visible: no

docs/reviews/SPEC-REVIEW-335-r2.md

@@ -0,0 +1,58 @@
+# SPEC-REVIEW-335-r2
+
+Issue: #335 «Трейлы пылесосов: осиротевшие маркеры навсегда остаются в store, точки после рестарта не сохраняются»
+Этап: ТЗ на ревью (PROCESS.md §2.4), лёгкий трек (`small`)
+Заход: r2 · блокирующих циклов израсходовано 1/2 (лимит лёгкого трека — 2, §2.4/§2.10; зелёный вердикт бюджет не тратит, #227)
+Материал: тело issue #335 после правки автора (комментарий 5451473100, 2026-08-28T10:34:50Z) против версии, разобранной в SPEC-REVIEW-335-r1 (`docs/reviews/SPEC-REVIEW-335-r1.md`, коммит `ccf685e0`). Код не менялся: репозиторий `dev` на момент r1 был `79aa0a90`, сейчас `ccf685e0` — единственная разница между этими SHA есть публикация самого документа r1, ветки `issue/335-*` по-прежнему не существует. Предмет разбора — исключительно дельта текста issue.
+
+## Разбор по дельте (PROCESS.md §2.10)
+
+r1 закончился жёлтым вердиктом с одной блокирующей находкой (High-1) и одной находкой в скоупе (Medium-1). Дельта локальна: правка меняет два места в теле issue (определение «маркер существует» + связанные AC1/AC4, и раздел «Release-артефакты»), не трогает код (кода ещё нет), не меняет UX-контракт и не задевает новую подсистему. Условия «разбор остаётся полным» (ребейз, смена контракта, новая подсистема, объём дельты ≈ объёму задачи) не выполнены — разбор по дельте оправдан.
+
+Дельта проверена по трём источникам: тексту `docs/VACUUM.md` («Storage and lifecycle»), фактическому коду (`custom_components/houseplan/trails.py`, `websocket_api.py`, `src/devices.ts`) и тексту находок r1.
+
+### Закрытие раунда r1
+
+| Находка r1 | Чем закрыта | Где это видно |
+|---|---|---|
+| **High-1** — определение «маркер существует» игнорировало tombstone `removed: true`, из-за чего AC1 не покрывал штатное удаление vacuum-маркера в редакторе (единственный tombstone-путь, `deletePlanMarkerRecords`, `src/devices.ts:914-934`) | Раздел «Аналитика и границы» переопределяет живость маркера: «маркер жив, только если его `id` присутствует в успешно сохранённом `config.markers` и `removed is not True`» — то же правило, что уже использует `TrailRecorder.async_refresh()` (`trails.py:169-170`, `if m.get("removed") is True: continue`) и `import_export.live_layout`. AC1 теперь явно требует доказательства для штатного tombstone `removed: true` («удаляет существующий vacuum-marker штатным tombstone `removed: true` через обычный `config/set`») и отдельно для полного отсутствия `id`. AC4 добавляет требование «import/recovery также удаляют трейлы tombstone `removed: true`», закрывая явно предсказанный в r1 побочный эффект: сегодняшний import/undo-путь (`websocket_api.py:497-499`, `live_marker_ids = {str(marker.get("id")) for marker in target_config.get("markers") or []}`, аналогично на `websocket_api.py:1872-1874`) строит «живые» id без фильтра по `removed`, то есть тоже нуждается в правке под новое определение — контракт реализации (пункт 1) и AC4 это называют явно | Текущее тело issue, разделы «Аналитика и границы» (строка с `removed is not True`), AC1, AC4 |
+| **Medium-1** — не назван `User-Visible` и потребность в changelog | Раздел «Release-артефакты»: «`User-Visible: yes`: исправление меняет наблюдаемую долговечность и очистку истории перемещений пылесоса. В том же коммите обязательны короткие согласованные записи со ссылкой на #335 в `docs/CHANGELOG.md` и `docs/CHANGELOG.ru.md`» | Текущее тело issue, раздел «Release-артефакты» |
+
+Обе находки закрыты текстом ТЗ, а не заявлением автора: формулировки сверены построчно с кодом и с r1.
+
+### Проверка дельты по существу (не только «поменялось ли слово»)
+
+- **Согласованность с каноном.** `docs/VACUUM.md:143-146`: «Deleting a vacuum marker removes its layout and server trails, creates the normal removal tombstone…» — новое определение живости («`id` присутствует и `removed is not True`») это и есть формализация этого предложения канона; старое определение r1 ему прямо противоречило.
+- **AC1 после правки действительно покрывает Проблему №1.** Штатное удаление в редакторе создаёт только tombstone (`{id, binding, removed: true, hidden: true}`, `src/devices.ts:927-930`) — hard drop id из `config.markers` в продукте не происходит при обычном удалении. AC1 требует доказательства именно для tombstone-сценария первой строкой, а «полное отсутствие marker-id» назван вторым, независимым случаем (ближайший реальный аналог — смена id при ребиндинге, не единственный путь для основной проблемы) — принятая формулировка не подменяет основной сценарий более простым.
+- **AC2 не сломан правкой.** Текущий текст добавляет уточнение «`hidden`/`disabled` без `removed: true` не являются удалением» — это реальный отдельный путь в продукте: `hidden` используется и как самостоятельный флаг видимости маркера (не только внутри tombstone), что подтверждается использованием поля вне `deletePlanMarkerRecords` (`src/houseplan-editor-runtime.ts`, `src/device-presentation.ts`, `src/device-inbox.ts`). Уточнение верно закрывает потенциальную двусмысленность «hidden ⇒ removed» и не противоречит определению живости.
+- **Контракт реализации согласован с AC4.** Пункт 1 контракта требует «использовать общий helper с этим же правилом в существующих import/recovery путях» — фактический код (`websocket_api.py:497-499`, `1872-1874`) подтверждает, что сегодня эти пути определяют «живой» id без учёта `removed`, то есть общий helper при новом определении действительно изменит их поведение, как предсказывал r1; AC4 требует это как проверяемое регрессионное расширение, а не молчаливый побочный эффект.
+- **Новых договорных пробелов дельта не вносит.** «Вне скоупа», «Откат», «Контракт реализации» пп. 2–3, AC3 текстуально не изменились между r1 и текущей версией — их предмет дельта не задевает.
+
+## Унаследовано из r1
+
+Без повторной проверки принято по SPEC-REVIEW-335-r1.md (`docs/reviews/SPEC-REVIEW-335-r1.md`, зафиксирован коммитом `ccf685e0` в `dev`, разбор велся на SHA `79aa0a90`, текст которого дельта не коснулась):
+
+- **Bug #2 / AC3 (потеря первой точки после рестарта).** `TrailRecorder.async_refresh()` (`trails.py:190-191`) действительно вызывает `_sample()` и отбрасывает булев результат, тогда как `_on_state` (`trails.py:337-341`) на том же результате планирует сохранение и throttled-событие. Контракт («агрегировать результат стартовых `_sample()` и при фактическом изменении запускать тот же путь сохранения и уведомления») технически осуществим без искусственных допущений.
+- **AC2, ветка «no-op/ошибка не запускают очистку».** Поток `ws_config_set` возвращает результат до вызова `_refresh_trail_recorder` для семантического no-op и для всех ошибочных веток (`conflict`/`invalid_format`/`too_large`/`missing_plan`).
+- **Общий helper для import/recovery как разумный, проверяемый рефакторинг** (при условии исправления High-1, которое теперь внесено).
+- **Границы «вне скоупа»** согласованы с `docs/VACUUM.md`: формат/лимиты трейлов, debounce/throttle-интервалы, подписки контрактом не меняются.
+- **Откат** — единственный issue-коммит, обратной миграции нет, формат store не меняется.
+- **Попадание в `docs/SCOPE.md`, job J6** («Keep the plan true as the home evolves») — задача чинит корректность уже задокументированного в `docs/VACUUM.md` контракта, продуктовой рамки не расширяет.
+- **Выбор лёгкого трека (`small`)** — критерии §5 выполняются одновременно (риск/сложность ≤3, одна backend-поверхность жизненного цикла трейлов, без миграции конфига, без нового UX-контракта, без touch/perf влияния); расширение AC4 на import/recovery не создаёт вторую поверхность — это тот же backend lifecycle трейлов.
+
+## Что проверено и корректно (сверх унаследованного)
+
+- Определение живости маркера, AC1, AC2, AC4 и Release-артефакты — сверены построчно с текущим кодом (`trails.py`, `websocket_api.py`, `src/devices.ts`) и с `docs/VACUUM.md`; расхождений не найдено.
+- `User-Visible: yes` — обоснованное значение: оба бага меняют наблюдаемое поведение (трейлы больше не растут бессрочно осиротевшими записями; первая точка после рестарта не теряется), запись в оба changelog названа обязательной в том же коммите — соответствует DoR §2.5 и правилу 10 из PROCESS.md §3.
+
+## Чего не проверял
+
+- Реальный запуск тестов — на этапе ТЗ кода нет, `typecheck`/`test`/`build` к предмету ревью не относятся.
+- Многопоточная/многоклиентская гонка вокруг предложенной очистки при `config/set` — как и в r1, вне AC, оставлено на усмотрение реализации при условии, что существующие `write_lock`/`_refresh_lock` её накрывают.
+- Побочный эффект на `_source_health` — контракт реализации его не трогает; фактическое отсутствие регрессии подтвердит код-ревью.
+
+## Вердикт
+
+Обе находки r1 закрыты текстом ТЗ и подтверждены построчной сверкой с кодом и с `docs/VACUUM.md`, а не заявлением автора. Дельта нового High/Medium не вносит: расширенные AC1/AC4 корректно покрывают Проблему №1 (штатное удаление через tombstone), AC2 не сломан уточнением про `hidden`/`disabled`, `User-Visible`/changelog решены. Задача готова к «Готово к разработке».
+
+`Вердикт: зелёный · заход r2 · блокирующих циклов 1/2 · High: 0 · Medium: 0 → в задаче`