mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
claude[bot]
@@ -0,0 +1,165 @@
|
||||
# SPEC-REVIEW-491-r1
|
||||
|
||||
Issue: [#491](https://github.com/Matysh/houseplan-card/issues/491) — «Optimize/Undo: не терять незавершённую парную транзакцию при следующей записи».
|
||||
Этап: ревью ТЗ (PROCESS.md §2.4), полный трек (P1/bug, лёгкий трек не проходит по
|
||||
сложности/риску и числу затронутых backend-эндпоинтов — названо аналитиком).
|
||||
Заход: r1 · блокирующих циклов израсходовано 0 из 4.
|
||||
|
||||
Материал: `docs/specs/491-optimize-undo-pair-recovery.md` на коммите
|
||||
`286f11b24efa4b95b26c16619f768ca1cd2c5b75` (рабочая копия уже на нём), тело
|
||||
issue #491 и оба комментария (аналитика, передача на ревью).
|
||||
|
||||
## Скоуп
|
||||
|
||||
ТЗ описывает защиту SCOPE J6: незавершённая (crash/error) парная запись
|
||||
config+layout, оставленная `plan/optimize` или `plan/optimize_undo`, не должна
|
||||
уничтожаться следующим обычным writer'ом (`config/set`, `layout/set`,
|
||||
`layout/update`, `layout/delete`, `geometry/repair`) до того, как эта пара
|
||||
доведена до целевого состояния или отката. Предлагается обобщить уже
|
||||
существующий retry→rollback протокол `_commit_import_pair` (Import,
|
||||
space-delete) на Optimize/Undo и добавить write fence перед каждым ordinary/
|
||||
paired writer.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Ревью ТЗ на этом этапе кода не существует (задача ещё не реализована), поэтому
|
||||
предмет проверки — не гейты, а полное чтение текущего backend-кода, на который
|
||||
ссылается ТЗ, чтобы отличить точный технический разбор от догадки, выданной за
|
||||
факт (риск, прямо названный в промпте ревью).
|
||||
|
||||
Прочитано целиком и построчно сверено с текстом ТЗ:
|
||||
|
||||
- `custom_components/houseplan/websocket_api.py` — `ws_plan_optimize` (строки
|
||||
1890–2063), `ws_plan_optimize_undo` (2074–2148), `ws_config_set` (1516–1687,
|
||||
включая `_discard_optimizer_snapshot` на 1657), `ws_layout_set` (678–729),
|
||||
`ws_layout_update` (739–796), `ws_layout_delete` (1307–1328),
|
||||
`ws_geometry_repair` (810–902), `ws_space_delete` (1789–1877, использует
|
||||
`_commit_import_pair`), `ws_import_apply` (491–599, тот же helper),
|
||||
`_persist_pair_intent` / `_converge_pair` / `_commit_import_pair` (311–382),
|
||||
`_discard_optimizer_snapshot` (180–191), `_layout_metadata` (303–308);
|
||||
- `custom_components/houseplan/__init__.py` целиком — startup-резолвер
|
||||
`optimize_pending` (160–222) и compatibility-fallback для pending без
|
||||
`final_metadata` (189–194);
|
||||
- `docs/ARCHITECTURE.md` (окрестности 1190–1230) — существующий
|
||||
задокументированный контракт `optimize_pending`/`optimize_backup` для
|
||||
Import, который ТЗ предлагает распространить на Optimize/Undo;
|
||||
- `docs/CONFIG-COMPATIBILITY.md`, `docs/USER-GUIDE.ru.md` — подтверждение, что
|
||||
ТЗ не описывает несуществующее поведение (существующий обобщённый сценарий
|
||||
ошибки сохранения на месте, новых терминов нет);
|
||||
- `tests_backend/test_ha_websocket.py` — существование теста
|
||||
`test_plan_optimize_pair_and_one_deep_undo_survives_geometry_repair`,
|
||||
названного в AC8, подтверждено (`grep`, файл найден).
|
||||
|
||||
Инструментальные гейты (typecheck/test/build) на этом этапе неприменимы: ТЗ не
|
||||
меняет код. Вопрос уместности полного трека и учёта дельты (§2.9/§2.10) не
|
||||
возникает — это первый заход.
|
||||
|
||||
## Находки
|
||||
|
||||
Ни одной High или Medium находки. Технические расхождения между ТЗ и
|
||||
фактическим кодом не обнаружены — каждое утверждение раздела «Проблема и
|
||||
подтверждённая причина» и раздела «Термины и инварианты» проверено чтением
|
||||
конкретной строки:
|
||||
|
||||
- «`layout/set`, `layout/update` и `layout/delete` намеренно удаляют
|
||||
`optimize_backup` и `optimize_pending`» — подтверждено, все три вызывают
|
||||
`async_save_layout_state(..., remove=(_OPTIMIZE_BACKUP, _OPTIMIZE_PENDING))`
|
||||
безусловно при фактической записи (строки 725, 793, 1324);
|
||||
- «`config/set` после durable config write вызывает
|
||||
`_discard_optimizer_snapshot`» — подтверждено, вызов на строке 1657, после
|
||||
`async_save_config_state` на 1647, best-effort (`except Exception`);
|
||||
- «Optimize/Undo не используют существующий retry → rollback протокол... уже
|
||||
защищены Import и удаление пространства» — подтверждено: `ws_plan_optimize`
|
||||
и `ws_plan_optimize_undo` пишут intent→config→layout напрямую, без обёртки
|
||||
в `_commit_import_pair`, которую `ws_import_apply` и `ws_space_delete`
|
||||
используют дословно;
|
||||
- «Setup умеет завершить пару... старые pending без `final_metadata`
|
||||
продолжают обрабатываться по compatibility fallback» — подтверждено кодом
|
||||
`async_setup_entry` (`__init__.py:189-194`): при отсутствии `final_metadata`
|
||||
используется старое поведение (сохранить/очистить `optimize_backup` по
|
||||
`clear_backup`).
|
||||
|
||||
Догадок, выданных за решённый факт, не найдено: единственное потенциально
|
||||
продуктовое следствие — «неразрешимый pending блокирует последующие обычные
|
||||
writer'ы до восстановления или перезапуска» (AC6, §7 п.5) — не изобретение
|
||||
автора ТЗ, а дословно названная владельцем граница в теле issue («либо явное
|
||||
блокирование обычных writers до разрешения»). Раздел 18 «Принятые технические
|
||||
предположения» корректно ограничен нефункциональными решениями (место
|
||||
resolver'а, переиспользование `_commit_import_pair`, код ошибки,
|
||||
read-repair-исключения, порядок permission-check/fence) и явно помечен как
|
||||
предположения, которые ревьюер вправе оспорить — оспаривать нечего, все пять
|
||||
следуют из существующего кода Import.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Обязательные разделы §7.1 присутствуют все: сценарий, что человек увидит,
|
||||
проблема, скоуп/не-скоуп, контракт (§§6–9), UX/i18n, модель данных и
|
||||
совместимость, AC1–AC10 с доказательством, план автотестов, риски, откат,
|
||||
release-артефакты;
|
||||
- первые два раздела продуктовые и отвечают на персону/поверхность/момент
|
||||
(администратор дома, десктоп-редактор, окно между Optimize/Undo и
|
||||
следующей правкой) и «что человек увидит» одной фразой без терминов
|
||||
реализации;
|
||||
- каждый AC1–AC10 указывает способ доказательства (`backend`/HA-harness,
|
||||
мутационный свидетель либо явное «чтение кода» для AC5/AC9/AC10) —
|
||||
соответствует требованию §2.5 DoR;
|
||||
- обоснование полного трека именует нарушенный критерий `small` (сложность/
|
||||
риск выше 3, несколько backend-эндпоинтов) — не голословное «обычный
|
||||
трек»;
|
||||
- продуктовых вопросов владельцу нет, и оснований считать это неполным не
|
||||
нашлось: единственное поведенческое следствие (блокировка writer'ов до
|
||||
разрешения) прямо взято из границ, заданных владельцем в теле issue;
|
||||
- i18n (нет новых ключей), touch/View/Kiosk (без нового взаимодействия),
|
||||
производительность (одно дополнительное чтение metadata под уже
|
||||
существующим `write_lock`), откат (без миграции данных) — все названы
|
||||
явно, ни один пункт DoR не обойдён молчанием;
|
||||
- модель данных: новых persisted-полей нет, что подтверждено чтением —
|
||||
`final_metadata`/`kind`/`clear_backup` уже присутствуют в структуре pending,
|
||||
используемой Import (строки 568-594, 1837-1857 websocket_api.py), ТЗ лишь
|
||||
обобщает их на Optimize/Undo, не меняя формат;
|
||||
- список затронутых файлов и модулей присутствует (в комментарии аналитика,
|
||||
на который ссылается ТЗ) и совпадает с фактическим расположением кода,
|
||||
прочитанным при этом ревью.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Не прогонялись `typecheck`/`test`/`build` — на этапе ревью ТЗ продуктовый
|
||||
код не менялся, дешёвые гейты неприменимы;
|
||||
- не проверялась реализация write fence, retry/rollback-обобщения или
|
||||
HA-harness fault-injection тестов — их ещё не существует, это предмет
|
||||
будущего код-ревью (S6→S7), включая обязательную для защитных AC таблицу
|
||||
«чем краснеет» (PROCESS §2.7);
|
||||
- не оценивалась производительность на реальном профиле — ТЗ прямо говорит
|
||||
«нет» влияния и обоснование (одно доп. чтение под уже держащимся локом)
|
||||
достаточно для стадии ТЗ;
|
||||
- не проверялись открытые issue на дубликаты сверх названных автором (#87,
|
||||
#466) — беглый просмотр их описаний в тексте ТЗ/issue не выявил
|
||||
противоречий, глубокий поиск дубликатов не проводился.
|
||||
|
||||
## Вывод
|
||||
|
||||
ТЗ технически точное (каждое утверждение о текущем коде проверено чтением и
|
||||
подтвердилось), полное по составу разделов, каждый AC проверяем и несёт
|
||||
способ доказательства, продуктовая рамка (сценарий/персона/до-после) на
|
||||
месте, единственное поведенческое следствие для пользователя прямо
|
||||
санкционировано владельцем в теле issue, догадок под видом решений не
|
||||
обнаружено.
|
||||
|
||||
**Вердикт: зелёный.**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/491-optimize-undo-pair-recovery`, коммит `286f11b24efa` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `3ec8105ab515d6fe60692dda82b4345bfc8d411a`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 3ec8105ab515
|
||||
```
|
||||
- ТЗ `docs/specs/491-optimize-undo-pair-recovery.md`, блоб `1a496fcff52cef3cd9ae35d5f967375278b8cb03`
|
||||
```
|
||||
git log --all --find-object=1a496fcff52cef3cd9ae35d5f967375278b8cb03 -- docs/specs/491-optimize-undo-pair-recovery.md
|
||||
```
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user