From ce3e92b22b922aff052e894d0776f293ad1f4756 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:08:44 +0000 Subject: [PATCH] docs: review document for #419 Issue: #419 User-Visible: no --- docs/reviews/SPEC-REVIEW-419-r1.md | 243 +++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-419-r1.md diff --git a/docs/reviews/SPEC-REVIEW-419-r1.md b/docs/reviews/SPEC-REVIEW-419-r1.md new file mode 100644 index 00000000..9cbd0ee8 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-419-r1.md @@ -0,0 +1,243 @@ +# SPEC-REVIEW-419-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/419 +- Этап: ревью ТЗ (PROCESS.md §2.4) +- ТЗ: `docs/specs/419-area-snapshot-roster-guard.md` +- Материал: ветка `issue/419-area-snapshot-roster-guard`, SHA `153dc3f79401c55cb957f776566caaded85c9214`, + блоб ТЗ `eb5a7a352d742be601c7c695d27256f9af9bc0ad` +- Заход: r1 (первый; раздел «Унаследовано из r0» не применяется) +- Маршрут: standard (owner, комментарий 2026-09-02) — обоснованно: меняется + destructive lifecycle persisted metadata, риск выше лимита `small` + +## Вердикт + +**Зелёный.** High: 0, Medium: 0 (в скоупе и вне скоупа), Low: 2, обе сняты +ревьюером с записью (см. ниже) — не блокируют. + +## Скоуп ревью + +Полный разбор ТЗ (заход r1, дельты нет). Прочитано в порядке PROCESS.md: +`docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md`, тело issue #419 и оба комментария +владельца (аналитика с Q1–Q3 и defaults, публикация ТЗ), само ТЗ +`docs/specs/419-area-snapshot-roster-guard.md`, канонический раздел +«Marker Area provenance (#126)» в `docs/CONFIG-COMPATIBILITY.md`. + +## Как проверялось + +Ревью ТЗ на этапе spec не читает код на предмет корректности реализации — код +ещё не написан. Но каждое фактическое утверждение проблемы и контракта сверено +с текущим состоянием репозитория, чтобы отличить обоснованный анализ от +догадки, выданной за факт: + +- `src/device-area-relocation.ts` (полностью, 266 строк) — подтверждён ровно тот + дефект, который описывает issue: `resolveDeviceAreaRelocations()` (строки + 144–160) сравнивает `snapshot` только с `liveIds`/`liveBindings`, построенными + из `options.devices` — presentation-проекции, а не с реестром; +- `src/ha-binding-status.ts` (полностью) — подтверждено, что `authoritative` + (строка 141) гарантирует только успешный парсинг обоих WS-ответов + (`config/device_registry/list`, `config/entity_registry/list`) в массивы, но + не защищает от пустого/усечённого, но технически валидного кадра; подтверждено + существование `snapshot.devices`/`snapshot.entities` (полный реестр, не + отфильтрованный), `snapshot.revision` (инкрементируется на каждой успешной и + неуспешной перезагрузке) и готового механизма `refreshHaRegistries()` для + «одного контрольного refresh» из AC9 — эти источники данных существуют и + доступны, контракт §1–§3 ТЗ на них реализуем; +- `src/devices.ts:1118` (`buildDevices`) — подтверждена конкретная причина, по + которой живой device легитимно отсутствует в `devices[]`: `if (!area || + !areaToSpace[area]) continue;` — устройство без Area или с Area, не + сопоставленной ни одному space, отбрасывается на входе, до любых + показанных/скрытых фильтров; +- `src/space-card.ts:445-465` (`houseplan-space-card`, вторая поверхность из + «Сценария») — подтверждено, что Static-поверхность читает тот же + `resolveDeviceAreaRelocations()`, но использует только `.relocateIds` для + рендера и никогда не пишет `marker_area_snapshot` обратно. Дефект добирается + до неё не через собственный вызов резолвера, а через общий персистентный + снапшот, который портит write-путь `houseplan-card.ts`. Поэтому список + «Затрагиваемые файлы» ТЗ корректно не включает `space-card.ts` — это + потребитель уже испорченных данных, а не второй источник дефекта; +- `test/device-area-relocation.test.mjs` (полностью, 240 строк) и + `demo/smoke_area_relocation.mjs` (структура, включая перехват + `config/device_registry/list`/`config/entity_registry/list` на строках + 353–354) — подтверждена техническая возможность добавить параметризованную + матрицу и точный счётчик WS-вызовов реестра, как того требуют AC1–AC9 и план + автотестов; +- `docs/CONFIG-COMPATIBILITY.md`, раздел «Marker Area provenance (#126)» — + сверены заявления ТЗ про формат/лимит/поведение import-путей: ТЗ говорит + «Full import/export и space-only import сохраняют контракты #126 без + изменений» — это означает «поведение этих путей не меняется этой задачей», а + не «данные переживают cross-source import» (тот кейс и так по канону + выбрасывает карту) — формулировка не вводит в заблуждение при внимательном + чтении, но могла быть однозначнее (см. Low L1 ниже, не блокирует); +- git-история `#126`/`#403`/`#406` (`git log --grep`) подтверждает, что все три + контракта существуют как отдельные, ранее принятые задачи над тем же модулем, + что соответствует ссылкам ТЗ. + +## Проверка обязательных разделов (PROCESS.md §7.1) + +Все обязательные разделы присутствуют и в правильном порядке: сценарий · что +человек увидит до/после · проблема · скоуп/не-скоуп · контракт поведения · UX · +модель данных и миграция · i18n · критерии приёмки AC1–AC14 с доказательством · +план автотестов · риски · откат · release-артефакты. Плюс обязательный блок +«принято предположительно, поменять свободно» — присутствует и по существу +касается только нерешаемых пользователем технических деталей (сигнатура +резолвера, хранение runtime-кандидатов, раздельный empty-guard, переиспользование +существующего debounce), что соответствует PROCESS.md §7.1. + +**Продуктовые первые два раздела.** Персона названа (Home admin), поверхности — +обе («полная карточка и houseplan-space-card»), момент — во время +запуска/перезапуска/обновления HA. «Что человек увидит» — одной фразой без +терминов реализации, до и после. Соответствует шаблону. + +## Проверка однозначности и доказуемости AC + +14 критериев приёмки, каждый — проверяемое утверждение с названным способом +доказательства (`unit`, `browser smoke`, `parameterized unit`, +`mutation-gate.mjs`, «локальные гейты процесса», `check-docs`/CI/diff manifest). +Ни один не сформулирован как «работает корректно» без критерия. Технически +реализуемы все — см. «Как проверялось» выше: + +- AC1–AC4 (положительное доказательство жизни: полный Device/Entity Registry, + exact live state включая `unavailable`/`unknown`, живой сохранённый marker) — + каждый источник подтверждён существующим в коде; +- AC5–AC7 (два подтверждения, восстановление между проходами, повтор одной и + той же revision не считается вторым подтверждением) — однозначны: любая + отличающаяся успешная непустая authoritative revision закрывает второе + подтверждение, независимо от того, вызвана она контрольным refresh (AC9) или + сторонним событием реестра — оба AC не противоречат друг другу при точном + прочтении (см. Low L2); +- AC8 (limited/error и reload/remount не подтверждают и не наследуют) — + согласуется с §4 контракта и Q3-default владельца; +- AC9 (ровно один confirmation refresh, без reload-loop) — технически + реализуемо через существующий `refreshHaRegistries()`/`scheduleReload()` с + 80 мс debounce; +- AC10 (существующие пути #126/#403/#406 не задерживаются) — граница явно + описана в §5 контракта: явное удаление, rebind, explicit/ineligible, + normal relocation остаются однопроходными; +- AC11 (отказ записи не портит идемпотентность) — согласуется с уже + реализованным в коде паттерном restore-on-failure (`houseplan-card.ts:5267- + 5331`), контракт ТЗ его не переопределяет, только защищает точку принятия + решения раньше по конвейеру; +- AC12 (три целевых мутанта) — прямо соответствуют трём защищаемым инвариантам + контракта (§1 «presentation ≠ registry», §3 «одного отсутствия мало», §2 + «пустой namespace не источник»); +- AC13–AC14 — стандартные гейты процесса и синхронизация документации/changelog, + ничего специфичного к этой задаче не требуется сверх обычного. + +## Проверка «догадка вместо решения» + +Не найдено ни одного заявления о поведении, которое не подтверждается ни +существующим документом, ни исходным кодом, ни явно не помечено как +предположение технического уровня. Ключевые фактические утверждения проблемы +(«authoritative гарантирует только форму ответа, не его полноту»; «devices — +presentation-проекция, а не реестр»; «auto-device без сопоставленной Area +законно отсутствует в devices») — все верифицированы против кода в разделе «Как +проверялось» выше. Продуктовые defaults Q1–Q3 взяты дословно из ответа +владельца от 2026-09-02, а не изобретены автором. + +## Продуктовое рассуждение (SCOPE.md) + +Задача закрывает job **J6** «Keep the plan true as the home evolves» — +конкретно ветку «переезд area» из #126: без этого фикса временный сбой реестра +при рестарте HA способен молча стереть провенанс переезда, из-за чего плановая +карточка перестаёт отличать «устройство переехало» от «устройство обнаружено +впервые». UX-раздел ТЗ прямо и правильно формулирует, что исправление +намеренно тихое — новых элементов интерфейса, уведомлений, настроек нет, что +соответствует духу «never delete a user's file on an inference» +(`docs/SCOPE.md`): тот же принцип «wasted state дешевле, чем потерянные +данные», применённый здесь не к файлу, а к lifecycle-метаданным. Изменение не +ухудшает ни одну соседнюю персону/сценарий: Static-карточка (household members, +kiosk) получает тот же эффект косвенно и без нового кода в своей ветке (см. +выше), View/kiosk/touch не затронуты явно и по разделу «Touch, accessibility и +security» ТЗ. + +## Находки + +Находок, блокирующих цикл (High) или требующих правки в скоупе без блокировки +(Medium), нет. + +### Low (сняты ревьюером с записью, не блокируют) + +- **L1.** Формулировка «Full import/export и space-only import сохраняют + контракты #126 без изменений» неоднозначна при беглом чтении: можно понять + как «данные переживают import», хотя по канону (`CONFIG-COMPATIBILITY.md`, + «Marker Area provenance») cross-source full import и space-only import и + раньше не переносили эту карту. При внимательном чтении фраза корректна + («контракт», то есть поведение этих путей, не меняется этой задачей) и не + противоречит AC. Снимается без правки: реализация не может ошибиться иначе, + так как поведение уже зафиксировано существующим кодом/документом, который + эта задача не трогает (не-скоуп прямо это исключает). +- **L2.** Контракт «второе подтверждение» (§3 п.4, AC6) и «ровно один + confirmation refresh» (AC9) не объявляют явно одной строкой, что confirmation + может прийти НЕ от специально запрошенного refresh, а от любой следующей + отличающейся authoritative revision, случившейся по другой причине (внешнее + registry-событие). Оба текста по отдельности читаются однозначно и не + противоречат друг другу при точном прочтении (AC9 ограничивает число + *запрашиваемых* карточкой обновлений, а не число революций, которые вправе + засчитаться), но явная фраза избавила бы разработчика от лишнего + сомнения. Снимается без правки ТЗ: тесты AC5–AC7/AC9 достаточно точны, чтобы + реализация и код-ревью зафиксировали единственное согласованное поведение; + безопасность контракта (никогда не удалять по одному наблюдению) не зависит + от источника второй revision в любом случае. + +## Что проверено и корректно + +- Маршрут `standard` обоснован верно — задача меняет destructive lifecycle + persisted metadata, критерии лёгкого трека (`docs/PROCESS.md` §5) не + выполняются одновременно (минимум «нет нового UX-контракта» под вопросом + из-за меняющегося поведения записи, и уж точно объём контракта поведения не + укладывается в «сложность ≤3»); +- Q1–Q3 из аналитики перенесены в контракт ТЗ дословно (§2, §3, §4 соответствуют + принятым defaults), никаких расхождений с решением владельца от 2026-09-02; +- Скоуп и не-скоуп взаимно непротиворечивы и корректно исключают: смену формата + `marker_area_snapshot`, лимит 20 000, uborку `known_devices`/`new_device_ids`, + общий аудит HA Registry API — ничего из этого не требуется контрактом §1–§5; +- Все AC привязаны к реально существующим точкам данных (`hass.states`, полный + Device/Entity Registry через `haRegistrySnapshot`, сохранённые markers), а не + к гипотетическим API; +- План автотестов покрывает все 14 AC без дыр: каждая матрица unit-кейсов (1–9) + и каждый browser-smoke сценарий адресует конкретный AC; +- Откат — простой revert коммита, без флага; персистентная модель не меняется, + что подтверждено сверкой с текущим TS-типом `MarkerAreaSnapshot` в + `src/device-area-relocation.ts:1-9`; +- Release-артефакты названы полностью: оба changelog, нормативная документация, + обязательный Docs screenshots run (канонический источник фингерпринта), явно + «golden не меняется». + +## Чего не проверял (и почему не требовалось на этапе spec) + +- Код ещё не написан — код-ревью (PROCESS.md §2.7) на следующем этапе отвечает + на вопрос «работает ли реализация»; здесь оценивалась только выполнимость и + однозначность контракта; +- Не запускал `npx tsc --noEmit`/`npm test`/`npm run build` — на этапе ТЗ + продуктовый код не менялся (класс C, документация), гейты кода к этому этапу + не относятся; +- Не проверял `demo/smoke_area_relocation.mjs` исполнением — только структуру + файла для оценки технической реализуемости AC9 (счётчик WS-вызовов) и + существующего перехвата `config/device_registry/list`/`entity_registry/list`; +- Не сверял точный будущий сигнатурный контракт `resolveDeviceAreaRelocations()` + и совместимость с вызовом в `space-card.ts` — прочитано, что несовместимости + по данным нет (Static не пишет снапшот и не зависит от orphan-sweep решений), + но точная форма новых параметров прямо оставлена на усмотрение реализации + («принято предположительно, поменять свободно») и будет предметом код-ревью; +- Гейты `docs`/`check-docs`, golden, performance, backend — не относятся к + этапу ТЗ (документация/код ещё не менялись сверх самого файла ТЗ). + +## Унаследовано из r + +Не применяется: это первый заход (r1), предыдущего раунда нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/419-area-snapshot-roster-guard`, коммит `153dc3f79401` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `1ad31edcb0a80464e1e7b1a16839b69449c910ff` + ``` + git log --all --format='%H %T' | grep 1ad31edcb0a8 + ``` +- ТЗ `docs/specs/419-area-snapshot-roster-guard.md`, блоб `eb5a7a352d742be601c7c695d27256f9af9bc0ad` + ``` + git log --all --find-object=eb5a7a352d742be601c7c695d27256f9af9bc0ad -- docs/specs/419-area-snapshot-roster-guard.md + ```