docs: review document for #495

Issue: #495
User-Visible: no

docs/reviews/SPEC-REVIEW-495-r1.md

@@ -0,0 +1,265 @@
+# SPEC-REVIEW-495-r1 — #495: результат Import согласован с commit; удаление маршрутов робота доходит до Store
+
+- Issue: [#495](https://github.com/Matysh/houseplan-card/issues/495)
+- Этап: `spec` (PROCESS.md §2.4)
+- ТЗ под ревью: [`docs/specs/495-import-commit-and-route-runs-durability.md`](../specs/495-import-commit-and-route-runs-durability.md)
+- Материал: HEAD `70a394d2` (detached), содержит коммит ТЗ
+  `70a394d2 docs: spec for #495 — import result follows the commit, dropped route runs reach the store`
+- Заход: **r1**, блокирующих циклов израсходовано 0/4 (первый раунд — раздел
+  «Унаследовано из r0» не нужен)
+- Трек: полный (issue корректно не помечен `small`; в issue названо, какой
+  критерий §5 не проходит — «одна поверхность»: два независимых
+  durability-контракта, Apply-эндпоинт и рекордер следов)
+- Вердикт: **зелёный**
+
+## Скоуп ревью
+
+Диапазон `git diff abbca50c..70a394d2` (беспосредственно предыдущий коммит
+`dev`) содержит только:
+
+- `docs/specs/495-import-commit-and-route-runs-durability.md` (новый файл);
+- вход в issue #495 не менялся сверх комментария S2-аналитики.
+
+Продуктовый код (`custom_components/houseplan/**`), тесты, бандлы —
+не тронуты. Это ожидаемо: этап `spec` меняет только документацию.
+
+Прочитано до вердикта: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§9,
+§12), тело issue #495 и комментарий S2-аналитики, `docs/VACUUM.md`,
+`docs/ARCHITECTURE.md` (таблица WS-команд и раздел про crash-resumable
+паттерн), сам код: `custom_components/houseplan/websocket_api.py` (обработчик
+`ws_import_apply`, целиком блок `write_lock`…`send_result`),
+`custom_components/houseplan/import_export.py` (`get_candidate`,
+`create_preview`, лимиты вытеснения), `custom_components/houseplan/trails.py`
+(`TrailBook.drop_unknown_routes`, `TrailRecorder.async_purge_orphans`,
+`_async_delete_many`, `_refresh_lock`), три файла тестов (`test_trails.py`,
+`test_trail_recorder.py`, `test_ha_websocket.py`, `test_ha_import_export.py`)
+на предмет существования упомянутых в ТЗ тестов-образцов.
+
+## Как проверялось
+
+Ревью ТЗ, не код-ревью: гейты `npm test`/`npm run build`/`tsc`/`check-docs`
+не запускались — диапазон не содержит изменений `src/**` или
+`custom_components/**`, только новый файл в `docs/specs/`. Запускать их
+означало бы проверять пустое множество.
+
+Основная работа ревью — верификация каждого технического утверждения ТЗ
+против реального кода, поскольку у полного трека (в отличие от `small`) риск
+«описал не тот код» выше, а ТЗ делает много конкретных построчных утверждений
+о `websocket_api.py` и `trails.py`.
+
+| Проверка | Результат |
+|---|---|
+| Обязательные разделы ТЗ (PROCESS.md §7.1) | все присутствуют — таблица ниже |
+| AC1–AC7: однозначность + способ доказательства | у каждого назван тест/раунд мутантов — см. «AC» ниже |
+| Построчные утверждения о `websocket_api.py` / `import_export.py` / `trails.py` | сверено с кодом на `70a394d2` (эквивалентен `dev`) — см. «Верификация фактов» |
+| Ссылки на связанные issue (#491, #335, #162, #265) | тематика совпадает; #491 закрыт (S8), не дублируется |
+| Ссылки на существующие тесты-образцы | найдены все три — `test_import_apply_conflict_preserves_state_and_preview_token`, `test_success_events_are_emitted_only_after_both_target_writes`, `test_config_set_purges_tombstoned_and_absent_trails_durably` |
+| Признак «догадка выдана за решение» без пометки | целевой поиск — не найдено, см. ниже |
+| Продуктовые вопросы владельцу, не решённые самим ТЗ | не найдено — ТЗ решает всё внутри `§11` |
+
+### Обязательные разделы ТЗ (PROCESS.md §7.1)
+
+| Раздел | Есть | Где |
+|---|---|---|
+| Сценарий | да, две персоны/сценария (Import, Маршруты) | §1.1 |
+| Что человек увидит до/после | да, одной фразой на каждую половину | §1.2 |
+| Проблема | да, с построчными ссылками на код | §1 |
+| Скоуп и не-скоуп | да | §2, §3 |
+| Контракт поведения | да, раздельно по подсистемам | §4, §5 |
+| UX | да («не затрагивается», обоснованно) | §8.1 |
+| Модель данных и миграция | да («формат не меняется») | §7 |
+| i18n | да (не затрагивается) | §8.1 |
+| AC1…ACn с доказательством | да, 7 штук, у каждого назван тест | §8 |
+| План автотестов | да, подробный, включая мутанты | §6 |
+| Риски | да | §8.2 |
+| Откат | да | §7 |
+| Release-артефакты | да (changelog RU+EN) | §9 |
+
+Все разделы на месте — расхождений с §7.1, требующих находки, нет.
+
+### Верификация фактов
+
+ТЗ утверждает поведение конкретных строк кода на `dev`; каждое из этих
+утверждений проверено чтением того же кода на материале ревью, поскольку
+неверное построчное утверждение — тот дефект, который должен убить полный
+трек:
+
+- **B2 (Import).** `ws_import_apply` (`websocket_api.py:526–596`): кандидат
+  берётся `get_candidate(rt, msg["token"], owner)` (без `consume`) в начале
+  под `write_lock` — подтверждено, строка 538. После
+  `await _commit_pair(rt, pending, rollback)` идёт
+  `get_candidate(rt, msg["token"], _connection_user_id(connection), consume=True)`
+  — подтверждено, строка 634 (текущий код), это и есть точка, которую §4.1 ТЗ
+  предлагает заменить на `rt.import_previews.pop(msg["token"], None)`. Между
+  `_commit_pair` и этим вызовом никакого другого выражения, способного
+  бросить `ImportFailure`, нет — заявление ТЗ «путь commit→ошибка становится
+  недостижимым по построению» после замены верно.
+- **`get_candidate`** (`import_export.py:1999–2011`): при `consume=False` (на
+  входе) проверяет `expires`, `owner_id`, `digest`; при `consume=True` (после
+  commit, до правки) — те же проверки плюс безусловный `pop`. Подтверждает
+  §1 B2 буквально.
+- **Вытеснение** (`import_export.py:1956–1965`): `mine` — токены владельца в
+  порядке вставки (Python dict сохраняет порядок), эвикшн `mine.pop(0)` —
+  действительно «старейший первым». Применяемый токен создан раньше нового
+  preview того же владельца (preview → потом apply), значит он вытесняется
+  первым при достижении лимита — тестовый план AC2 (создать
+  `MAX_IMPORT_PREVIEWS_PER_USER` новых preview тем же владельцем, чтобы
+  вытеснить применяемый токен) технически корректен.
+- **PairCommitFailure / §4.2.** `_commit_pair` (websocket_api.py:384) бросает
+  `PairCommitFailure(recovery_pending=...)` при неудаче — до строки с `pop`,
+  которая находится уже после `await _commit_pair(...)`. Значит на пути
+  `PairCommitFailure` вызов `get_candidate(..., consume=True)`/новый `pop`
+  вообще не выполняется — заявление §4.2 «после `PairCommitFailure` токен
+  остаётся» верно без правки этого пути.
+- **B3 (маршруты).** `TrailRecorder.async_purge_orphans`
+  (`trails.py:314–341`): цикл по живым маркерам вызывает
+  `effective_routes(...)` → `self.book.drop_unknown_routes(marker_id, ids)`,
+  результат (`bool`) никуда не сохраняется; запись Store происходит только
+  внутри `_async_delete_many(orphan_ids)`, и только если `orphan_ids`
+  непусто (`if not orphan_ids: return 0` — строка 338). Подтверждено
+  построчно; ровно то, что описывает §1 B3.
+- **`asyncio.Lock` неренетерабелен**, и `_async_delete_many` уже сам берёт
+  `self._refresh_lock` (`trails.py:351`) — если бы `async_purge_orphans` тоже
+  взял этот лок на весь свой корпус (как требует §5.1) и затем вызвал
+  публичную `_async_delete_many` как есть, был бы deadlock. ТЗ (§5.1 п.4,
+  §8.2) корректно называет это и предписывает «внутреннюю версию без
+  повторного захвата lock» — риск замечен автором ТЗ, а не пропущен.
+- **`drop_unknown_routes` возвращает `bool`, не снятые прогоны**
+  (`trails.py:152–172`, `rec.pop(slot)` без возврата значения) — §5.1
+  описывает НОВУЮ обёртку `self._drop_unknown_routes(config)` (метод
+  рекордера, не путать с методом книги того же почти-имени), которая обязана
+  сама снять копию `current`/`previous` до вызова `book.drop_unknown_routes`,
+  чтобы получить `{marker_id: {slot: run}}` для отката. §5.2 явно говорит, что
+  сигнатура `TrailBook.drop_unknown_routes` (включая `bool`) не меняется —
+  тест `test_drop_unknown_routes_touches_only_runs_that_name_a_route`
+  (`test_trails.py:195`) подтверждён существующим и совместимым.
+- **Тесты-образцы существуют**: `test_import_apply_conflict_preserves_state_and_preview_token`
+  (`test_ha_import_export.py:2217`), `test_success_events_are_emitted_only_after_both_target_writes`
+  (`test_ha_import_export.py:2483`), `test_config_set_purges_tombstoned_and_absent_trails_durably`
+  (`test_ha_websocket.py:409`) — все три подтверждены существующими,
+  ссылки ТЗ корректны.
+- **«Существующие тесты Apply без правок»**: тест на строке 1208
+  `test_ha_import_export.py` (`runtime.import_previews[expiring]["expires"] = 0`
+  → `get_candidate(...)` бросает `preview_expired`) — это unit-тест на саму
+  функцию `get_candidate`, а не на обработчик `ws_import_apply`; правка
+  ТЗ не трогает `get_candidate`, только убирает её повторный вызов в
+  обработчике. Тест остаётся зелёным без правок, как и заявлено.
+- **docs/VACUUM.md / docs/ARCHITECTURE.md уже описывают целевое поведение**
+  (§9 ТЗ): `VACUUM.md:98` — «changing the target space is a NEW route
+  identity … recorded runs … are dropped»; `ARCHITECTURE.md:1178` —
+  `houseplan/import/apply` описан как «crash-resumable paired config/layout
+  commit». Оба подтверждены; заявление «фикс приводит код к тексту, документы
+  не меняются» корректно.
+- **Вызовы `_purge_trail_recorder`/`async_purge_orphans`** после каждого
+  config-commit — подтверждено 4 точками вызова
+  (`websocket_api.py:654,1757,2278`, `__init__.py:193`), согласуется с §1
+  «вызывается после каждого commit конфига».
+
+Не найдено ни одного построчного утверждения о коде, которое разошлось бы с
+материалом ревью.
+
+### Догадка, выданная за решение — целевой поиск
+
+Не найдено. Единственные более-менее «решательные» технические детали —
+имя новой приватной обёртки `_drop_unknown_routes` и точный протокол отката
+(что именно копируется для восстановления) — прямо не диктуют пользователю
+видимое поведение и относятся к классу «агенты решают между собой»
+(PROCESS.md §7.1); ТЗ не выдаёт их за нормативный факт о продукте, а
+описывает как техническую реализацию контракта §5.1, который сам
+сформулирован как обязательный результат («результат обязан дойти до
+Store»), а не как гадание о существующем коде.
+
+## Критерии приёмки (AC1–AC7)
+
+Каждый пронумерован, для каждого назван ровно один тест или группа тестов —
+проверяемость есть у всех:
+
+| AC | Доказывается | Проверка ревьюера |
+|---|---|---|
+| AC1 | `test_issue_495_apply_result_follows_the_commit_when_the_preview_expires_meanwhile` (backend) | сценарий (обёртка над `_commit_pair`, `expires=0`) технически реализуем на верифицированном коде §4.1 |
+| AC2 | `test_issue_495_apply_result_survives_eviction_by_a_newer_preview` (backend) | эвикшн-механизм подтверждён «старейший первым» — сценарий воспроизводим |
+| AC3 | `test_issue_495_dropped_route_runs_reach_the_store_without_orphans` (backend/stub) | воспроизводимо на подтверждённом `_async_delete_many`/`drop_unknown_routes` |
+| AC4 | `test_issue_495_dropped_route_runs_roll_back_when_the_store_write_fails` (backend/stub) | откат по образцу существующего отката в `_async_delete_many` (строки 373–383), симметрия описана в §5.1 п.4/5 |
+| AC5 | `test_issue_495_dropped_route_runs_share_the_orphan_transaction` (backend/stub) | комбинация путей п.4/п.5 §5.1, не противоречит текущей структуре `_async_delete_many` |
+| AC6 | новый тест `test_ha_websocket.py` по образцу `test_config_set_purges_tombstoned_and_absent_trails_durably` (backend/HA) | образец существует и подтверждён (строка 409) |
+| AC7 | мутанты §6.3, `scripts/mutation-gate.mjs` + `backend-test-guard.mjs` | пять мутантов, у каждого назван ровно один свидетель-тест; отрицательный прогон — требование самого ТЗ (§6.3, «до перевода в S7») |
+
+Ни один AC не оставляет открытого вопроса о способе доказательства.
+
+## Соответствие SCOPE.md
+
+Задача — про J6 («Keep the plan true as the home evolves»): расхождение
+ответа/памяти с диском — именно та ложь плана, которую J6 запрещает. Both
+half (Import-ответ и маршруты робота) относятся к этому job. Продуктового
+расширения скоупа нет: §3 (не-скоуп) явно исключает алгоритмы импорта,
+продление TTL, защиту токена до commit, #491-фехтование — все обоснованно, и
+все совпадают с тем, что реально не нужно трогать по прочитанному коду.
+
+## Что проверено и корректно
+
+- Обязательные разделы §7.1 все присутствуют.
+- Все 7 AC пронумерованы, однозначны, у каждого назван способ доказательства
+  и конкретный тест/группа тестов.
+- Каждое построчное техническое утверждение о `websocket_api.py`,
+  `import_export.py`, `trails.py` подтверждено чтением кода на материале
+  ревью (см. «Верификация фактов») — ни одного расхождения.
+- Технически рискованное место (расширение `_refresh_lock`,
+  неренетерабельность `asyncio.Lock`) не просто не упущено — ТЗ прямо
+  формулирует решение («внутренняя версия без повторного захвата lock») и
+  называет риск в §8.2.
+- «Принятые предположения» (§11) — три штуки, все нормативно нейтральны
+  (не меняют видимое поведение), явно помечены как предположения, а не факт.
+- Продуктовых вопросов владельцу, требующих ответа, не найдено — контракт
+  «результат определяется commit» и «удаление доходит до Store» не оставляет
+  места для персонального/UX-выбора; это баг-фикс с уже согласованным на
+  уровне документации (VACUUM.md, ARCHITECTURE.md) целевым поведением.
+- Ссылки на #491, #335, #162, #265 — не дублируются, использованы как
+  прецеденты паттернов (откат памяти, crash-resumable commit).
+- Откат (§7) полный: revert одного коммита, без миграции данных.
+- Release-артефакты (§9): changelog RU+EN назван; изменение документации
+  подсистем корректно признано ненужным (текст уже соответствует целевому
+  поведению).
+
+## Чего не проверял
+
+- Гейты `npm test`/`npm run build`/`tsc`/`check-docs`/`python -m pytest
+  tests_backend` не запускались — диапазон ревью не содержит изменений кода
+  или тестов, только новый файл в `docs/specs/`; прогон был бы проверкой
+  пустого множества для этого раунда.
+- Не проверялась реализуемость точного протокола отката §5.1 п.4/5
+  построчно как будущий diff (мутанты §6.3 и тесты §6 ещё не написаны) — это
+  по определению предмет `code`-этапа и будущего code-review-раунда этой же
+  задачи, а не ревью ТЗ.
+- Не проверялось поведение `_drop_unknown_routes`-обёртки на конкурентных
+  вызовах `async_refresh` во время purge — сценарий не упомянут ни в ТЗ, ни в
+  issue как открытый риск, и `_refresh_lock` по конструкции сериализует оба
+  пути; отдельного продуктового риска здесь не просматривается.
+- Мутанты и HA-харнесс AC6 не прогонялись (кода ещё нет) — асимметрично
+  этапу; будет предметом code-review.
+
+## Вердикт
+
+**Зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче.**
+
+ТЗ полное по §7.1, каждый AC однозначен и доказуем, все построчные
+технические утверждения о существующем коде подтверждены чтением того же
+кода на материале ревью, риск неренетерабельности lock замечен и решён
+самим автором ТЗ, продуктовых вопросов владельцу не осталось. Следующий
+статус — «Готово к разработке» (`S5-ready`).
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/495-import-commit-and-route-runs-durability`, коммит `70a394d22821` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `76d0e202bdc79141e49c499710b7ae826be6b8d6`
+  ```
+  git log --all --format='%H %T' | grep 76d0e202bdc7
+  ```
+- ТЗ `docs/specs/495-import-commit-and-route-runs-durability.md`, блоб `a6fb6bbe17da6cc4ca7579279397f40730c235fd`
+  ```
+  git log --all --find-object=a6fb6bbe17da6cc4ca7579279397f40730c235fd -- docs/specs/495-import-commit-and-route-runs-durability.md
+  ```
+- Вердикт конвейера: `green` · High 0