mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 12:18:51 +00:00
claude[bot]
@@ -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<N-1>
|
||||
|
||||
Не применяется: это первый заход (r1), предыдущего раунда нет.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `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
|
||||
```
|
||||
Reference in New Issue
Block a user