diff --git a/docs/reviews/SPEC-REVIEW-495-r1.md b/docs/reviews/SPEC-REVIEW-495-r1.md new file mode 100644 index 00000000..551c41be --- /dev/null +++ b/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`). + +--- + + + +## Материал раунда + +- Ветка: `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