diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 85d1206c..bb3039c4 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,6 +1,6 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1108, issue: 396. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1109, issue: 397. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| @@ -41,6 +41,7 @@ | #657 | [CODE-REVIEW-657-r1.md](CODE-REVIEW-657-r1.md) | code · r1 | 🔴 красный | 1 | 0 | после fast-forward слияния (dev не двигался за время ревью) docs/reviews/INDEX.md остаё… | `docs/reviews/INDEX.md` `test/reviews-index.test.mjs` `_process.yml` `merge-candidate.mjs` `test/merge-candidate.test.mjs` `scripts/reviews-index.mjs` | | #657 | [CODE-REVIEW-657-r2.md](CODE-REVIEW-657-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | из r1 — проверка закрытия | `scripts/merge-candidate.mjs` `PROCESS.md` `mutation-registry.mjs` `INDEX.md` | | #656 | [CODE-REVIEW-656-r1.md](CODE-REVIEW-656-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | +| #655 | [SPEC-REVIEW-655-r1.md](SPEC-REVIEW-655-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #654 | [SPEC-REVIEW-654-r1.md](SPEC-REVIEW-654-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | «Release-артефакты» не называют обновление docs/ISOMETRIC.md | `docs/ISOMETRIC.md` `docs/CHANGELOG.md` `docs/CHANGELOG.ru.md` `docs/reviews/INDEX.md` | | #654 | [SPEC-REVIEW-654-r2.md](SPEC-REVIEW-654-r2.md) | spec · r2 | 🟢 зелёный | 0 | 0 | — | — | | #654 | [CODE-REVIEW-654-r1.md](CODE-REVIEW-654-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-655-r1.md b/docs/reviews/SPEC-REVIEW-655-r1.md new file mode 100644 index 00000000..55f66d0f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-655-r1.md @@ -0,0 +1,216 @@ +# SPEC-REVIEW-655-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/655 +- **Этап:** `S4-spec-review` (ревью ТЗ, PROCESS.md §2.4) +- **Трек:** лёгкий (`small`), автор явно называет критерий: одна backend-поверхность + жизненного цикла, сложность 3/10, без миграции, нового UX-контракта, touch/perf + влияния. +- **Материал:** тело issue #655 целиком (разделы «Проблема», «Ожидается», «Не + скоуп», «Аналитика», `## ТЗ`) + два комментария владельца (аналитика/оценка, + передача на ревью) + два служебных комментария о падении автоматического + прогона `model review` (полезны только как контекст: метка не менялась, до + этого раунда задача реального ревью не проходила). +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 2 +- **Роль:** ревьюер ТЗ (не автор) + +## Скоуп ревью + +Backend-регрессия долговечности: `VirtualLightController` (0,5 с коалесценция) +и `TrailRecorder` (10 с debounce) гарантированно сбрасывают отложенную запись +при unload/reload интеграции, но не при штатной остановке/перезапуске HA +(`EVENT_HOMEASSISTANT_STOP`/`entry.async_shutdown`, не `async_unload_entry`). +Задача — добавить единый идемпотентный shutdown-путь для обоих хранилищ и +перевести фоновую задачу виртуальных ламп с голого `asyncio.create_task` на +HA-tracked API. Не в скоупе: троттлинг toggle, ACL `system-read-only`, +интервалы/формат store/трейлов, WebSocket API. + +**SCOPE-проверка (`docs/SCOPE.md`):** владелец относит задачу к J1 (живой +статус на плане должен отражать последнее принятое состояние) и J6 +(«keep the plan true as the home evolves» — состояние обязано пережить +штатное обслуживание HA). Обе фичи (virtual light toggle, trail book) уже +приняты в продукт раньше этой задачи; здесь чинится не расширение +функциональности, а её надёжность — новой scope-дыры это не открывает и +существующий периметр не расширяет. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `docs/process/REVIEWER.md` (по ссылкам — + PROCESS.md §2.4, §7.1, §7.2, §4; §2.10 не применялся — это r1), `AGENTS.md`. +2. Прочитано тело issue #655 целиком (`gh issue view 655 --json body,comments, + labels`) — все четыре комментария владельца/пайплайна, вопросов владельцу в + тексте нет, подтверждаю. +3. Сверены обязательные разделы §7.1: + - **сценарий + «что человек увидит»** — присутствуют одной фразой в разделе + «Ожидается» (это часть тела issue, не только `## ТЗ`; материал — тело + issue целиком): «домочадец переключил виртуальную лампу… администратор + перезапустил HA… после старта лампа в том состоянии, в каком её + оставили» — без терминов реализации, названы обе релевантные персоны + (`docs/SCOPE.md`: household members, home admin). + - **проблема** — описана дважды (верхний уровень + `### Проблема` внутри + `## ТЗ`), согласованно, с точными путями/строками кода. + - **скоуп/не-скоуп** — явно, дважды (верхний уровень кратко, `### Вне + скоупа` подробно), без противоречий между версиями. + - **контракт поведения** — 5 пронумерованных пунктов, каждый проверяем + независимо (см. п.4 ниже — перепроверены по коду). + - **UX** — отдельного заголовка нет, но это оправдано: правка не трогает + ни одного видимого интерактивного элемента, весь наблюдаемый эффект уже + покрыт сценарием «что человек увидит» (состояние после restart). Нет + видимой поверхности, которую полноценный раздел UX мог бы описать сверх + этого. + - **модель данных и миграция** — явное «нет»: контракт п.5 «Никакой + миграции данных или новой настройки нет», формат store/трейлов не + меняется (контракт п.4). + - **i18n** — не упомянут явно, но обоснованно: изменение не добавляет ни + одной пользовательской строки (только серверный `_LOGGER`), других + backend-задач в проекте i18n тоже не касается. + - **план автотестов** — распределён по AC1–AC5, каждый называет метод + доказательства (HA-harness/unit/mutant) — соответствует требованию + «указание способа доказательства» для каждого AC. + - **риски и откат** — `### Риски и откат`, названа единственная реальная + угроза (двойной cleanup / гонка timer-flush) и как она закрыта + (идемпотентность + AC3). + - **release-артефакты** — `User-Visible: yes`, оба changelog в том же + коммите, generated bundle не меняется — явно. +4. **Перепроверены фактические утверждения ТЗ по коду на SHA `9a7020ce` + (== origin/dev), а не приняты на слово:** + - `custom_components/houseplan/virtual_lights.py:128-130` (`_schedule_save`) + — буквально `asyncio.create_task(self._delayed_save())`, не HA-tracked + API. Подтверждено дословно как в issue. + - `custom_components/houseplan/__init__.py:258-275` (`async_unload_entry`) + — вызывает `rec.async_teardown()` и `virtual_lights.async_flush()`, но + других обработчиков `EVENT_HOMEASSISTANT_STOP` не регистрирует. + - `grep -rn "EVENT_HOMEASSISTANT_STOP" custom_components/houseplan/` — 0 + вхождений во всём пакете (только `radar.py:117` слушает + `EVENT_CORE_CONFIG_UPDATE`, к делу не относится). Заявление «слушателя + нет» подтверждено, не принято на веру. + - `custom_components/houseplan/trails.py:472-478` (`async_teardown`) — + под `self._refresh_lock` вызывает `_close_subscriptions()` и, если была + отложенная запись (`pending`), `await self.store.async_save(self.book.data)` + — ровно то поведение, которое ТЗ называет «уже безопасным при + reload/unload». + - `tests_backend/test_ha_virtual_lights.py:114` — + `test_unload_flushes_a_toggle_still_inside_the_debounce_window` существует + и называется как в issue; тест `hass.async_stop()`-сценария в проекте не + найден (`grep -rn "async_stop" tests_backend/` — 0 совпадений) — + подтверждает пробел в покрытии, который и закрывают AC1/AC2. + - `custom_components/houseplan/manifest.json:22` — `single_config_entry: + true`. Это устраняет риск, который я сам держал в уме при чтении + контракта п.1–2 (совместное состояние `TrailRecorder`, ключ + `hass.data[DOMAIN]["trail_recorder"]`, не per-entry): при ровно одном + возможном entry «один привязанный к entry обработчик» не может + коллизировать с другим экземпляром интеграции. + - `hass.async_create_task` / `entry.async_create_background_task` уже + используются в проекте (`radar.py:115`, + `frontend_registration.py:561`, `websocket_api.py` в нескольких местах) + — контракт п.3 не изобретает новый паттерн, а переиспользует + существующий, что снижает технический риск реализации. + - `VirtualLightController.__init__` (`virtual_lights.py:79-86`) не хранит + `hass`/`entry` — реализации потребуется протянуть ссылку на `hass` в + конструктор или передавать её в `_schedule_save`. Это чисто техническая + деталь (модуль/интерфейс класса — то, что ТЗ прямо отдаёт исполнителю), + не продуктовый вопрос и не блокер: конструктор вызывается один раз + (`store.py:136`) в контексте, где `hass` доступен. +5. Ветки/коммитов для #655 не существует (`git log --all --oneline | grep 655` + — пусто; `git diff origin/dev...HEAD` пуст, `HEAD == origin/dev == + 9a7020ce`). Гейты (`tsc`, `test`, `build`, смоки, `golden`, `pytest + tests_backend`, инварианты) неприменимы на этапе `spec` — продуктового + кода ещё нет, это штатное состояние, а не пропущенный гейт. +6. HA-рантайм-утверждение («на `EVENT_HOMEASSISTANT_STOP` HA зовёт + `entry.async_shutdown`, не `async_unload_entry`») сам автор помечает как + «по знанию кода HA, не по прогону» — я не смог перепроверить его + исполнением: `python3 -c "import homeassistant"` в песочнице отсутствует + (`ModuleNotFoundError`), как и предупреждает `AGENTS.md` о песочнице. + Формулировка честно ограничена, доказательство отложено на HA-harness + в каноническом Linux CI (AC5 это явно называет) — не считаю это + находкой, так как автор сам не выдаёт догадку за факт, а называет её + пределы. + +## Находки + +Нет. Ни одной находки High/Medium/Low после перепроверки кода — все +утверждения ТЗ подтвердились чтением исходников, а не приняты на слово. + +Одно наблюдение, не являющееся находкой (не блокирует, не требует +действия): AC1/AC2 называют тестовый приём «вызвать штатный +`hass.async_stop()`». Часть HA-интеграций в тестах вместо полной остановки +`hass` предпочитают `hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)` + +`await hass.async_block_till_done()` — легче для фикстуры, не завершает +цикл событий раньше, чем тест успеет прочитать файл store после. Это +стратегия теста, а не продуктовый контракт (§7.1: «стратегия тестов» — +то, что решает автор/ревьюер, не владелец), и ТЗ уже прямо резервирует +исполнителю свободу технических решений. Оставляю на усмотрение +реализации без возврата. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют по существу; сценарий и «что + человек увидит» — продуктовые, одной фразой, без терминов реализации. +- Оба факта дефекта (голая `asyncio.create_task` у виртуальных ламп, + отсутствие слушателя `EVENT_HOMEASSISTANT_STOP` во всём пакете) — + перепроверены построчным чтением кода, а не приняты со слов автора. +- Контракт из 5 пунктов однозначен, пункты не пересекаются и не + противоречат друг другу; «идемпотентно» и «не создаёт новую запись при + отсутствии pending-изменений» — прямо адресуют риск двойного cleanup, + который сам ТЗ называет в разделе «Риски». +- AC1–AC5 каждый называет способ доказательства (HA-harness с конкретными + шагами / unit-структурный свидетель / мутант) — критерий «указание + способа доказательства» выполнен по всем пяти. +- AC5 заранее формулирует негативное доказательство (мутанты, реестр + которых уже существует в проекте, `scripts/mutation-registry.mjs`) — это + снижает риск пустой графы «чем краснеет» на код-ревью. +- `single_config_entry: true` в манифесте снимает единственный + архитектурный риск, который я держал в уме при чтении контракта + (совместное состояние `TrailRecorder` при нескольких entries). +- Технический паттерн (`hass.async_create_task`/ + `entry.async_create_background_task`) уже используется в проекте для + аналогичных фоновых задач — реализуемость контракта п.3 подтверждена + прецедентом, а не гипотетична. +- Открытых продуктовых вопросов нет — подтверждаю по тексту: единственное + наблюдаемое поведение («последнее принятое состояние переживает + штатный stop/restart») уже полностью и однозначно определено сценарием + и контрактом, домысливать нечего. +- Release-артефакты, откат, отсутствие миграции — названы явно, без + зазора для догадки. + +## Чего не проверял + +- Гейты (`tsc --noEmit`, `npm test`, `npm run build`, `pytest + tests_backend`, инварианты, смоки, `golden:verify`) — не прогонял: этап + `spec`, кода для #655 нет, `git diff origin/dev...HEAD` пуст. Предмет + код-ревью после реализации. +- Не проверял исполнением HA-рантайм-утверждение про + `EVENT_HOMEASSISTANT_STOP` vs `entry.async_shutdown` — HA не + устанавливается в песочнице (`ModuleNotFoundError: homeassistant`); сам + автор ограничивает это утверждение как «по знанию кода, не по прогону» + и переносит фактическую проверку на AC5/HA-harness в каноническом Linux + CI — это его собственная явная граница, а не пропуск с моей стороны. +- Не оспаривал конкретный числовой сценарий («увеличить окно debounce») как + техническую деталь теста — оставляю технику на усмотрение реализации, + как и позволяет §7.1 («стратегия тестов» — не продуктовый вопрос). +- Не проверял, потребует ли реализация протянуть `hass`/`entry` в + `VirtualLightController.__init__` изменения сигнатуры конструктора — + архитектурная деталь модуля, прямо отданная исполнителю ТЗ. + +## Вердикт + +Обязательные разделы полны и по существу, оба факта дефекта и +архитектурные предпосылки контракта перепроверены чтением кода — расхождений +не найдено. AC1–AC5 однозначны, каждый называет способ доказательства, +негативное доказательство (AC5) спланировано заранее. Открытых продуктовых +вопросов нет. Готово к разработке. + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `9a7020ce428e` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `eec9247ab39136663b248226de905169bb3d18bc` + ``` + git log --all --format='%H %T' | grep eec9247ab391 + ``` +- Тело issue: `9592e83bda3f396fe43ee14eb6bc22b4860f8309d2c5e4e954dc1b90a293c348` +- Вердикт конвейера: `green` · High 0