From 036a15aadefb96d3832d6099e4c5e224e1fac6ca Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 12 Sep 2026 20:07:39 +0000 Subject: [PATCH] docs: review document for #545 Issue: #545 User-Visible: no --- docs/reviews/SPEC-REVIEW-545-r1.md | 194 +++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-545-r1.md diff --git a/docs/reviews/SPEC-REVIEW-545-r1.md b/docs/reviews/SPEC-REVIEW-545-r1.md new file mode 100644 index 00000000..a6e6a79b --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-545-r1.md @@ -0,0 +1,194 @@ +# SPEC-REVIEW-545-r1 + +Issue: #545 «Радары: range/zone entity_id теряются из списка источников для +подписок и проверки прав» +Этап: ревью ТЗ (PROCESS.md §2.4) +Заход: r1 · трек: full (не `small`) · блокирующих циклов израсходовано 0 из 4 +Ревьюер: Claude (роль «ревьюер ТЗ», §6) +Материал: тело issue #545, раздел `## ТЗ`, на состоянии на момент разбора; +issue-комментарий «S2 — аналитика» (тот же материал, не решение по существу +вопроса). + +## Скоуп ревью + +Проверить, что ТЗ в теле issue #545 удовлетворяет §7.1 (обязательные разделы, +однозначность AC1…AC7 и указанный способ доказательства для каждого), что +продуктовые разделы («Сценарий», «Что человек увидит») отвечают на свои +вопросы, и что ни одно утверждение о существующем поведении не выдано за +факт без проверки. Это первый заход по этому ТЗ — версия для r2+ «разбор по +дельте» (§2.10) здесь неприменима. + +## Как проверялось + +Материал — исключительно текст: код смотрел не для того, чтобы утвердить +реализацию (её ещё нет), а чтобы отличить утверждение о факте от догадки, +которую §7.1 требует помечать явно. Прочитано и сверено построчно: + +- `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1, §2.3–2.5, §5, §7.1, §7.2); +- `docs/RADAR.md` — канонический документ подсистемы, раздел «Privacy, + permissions and storage» и таблица профилей; +- `custom_components/houseplan/radar_validation.py` — `_source_ids()`, + `radar_source_entity_ids()`, `_validate_sources()` для всех шести профилей; +- `custom_components/houseplan/radar.py` — `_resubscribe_sources()`, + `source_ids()`, `teardown()`, `_ensure_tick()`; +- `custom_components/houseplan/radar_websocket.py` — `_can_read()`, + `ws_radar_subscribe`, `ws_radar_setup_subscribe`, `source_restricted`/ + `health: restricted` пути; +- `tests_backend/test_ha_radar_websocket.py` — существующий `_Permissions` + fake (булев allow/deny, не per-entity) — сверка риска, названного в ТЗ; +- `scripts/mutation-gate.mjs`, `docs/TESTING.md` — подтверждение, что реестр + поддерживает Python-мутанты с `pytest`-гвардом (не только фронтенд), и что + формат идентификатора мутанта из ТЗ соответствует принятой конвенции; +- список файлов `tests_backend/test_radar_validation.py`, + `test_ha_radar.py`, `test_ha_radar_websocket.py` — существование + подтверждено (`ls`). + +Гейты не гонялись: на этапе ревью ТЗ кода ещё нет, гонять `typecheck`/`test`/ +`build` не над чем — это ожидаемо для S4-spec-review, а не пропуск. + +## Проверка утверждений ТЗ о текущем поведении (не догадка ли это) + +Раздел «Проблема» и «Контракт поведения» делают несколько фактических +заявлений о существующем коде. Каждое сверено с источником, а не принято на +слово: + +| Заявление ТЗ | Где проверено | Результат | +|---|---|---| +| `_source_ids()` собирает вложенные поля только по суффиксу `_entity` | `radar_validation.py:99-105` | Подтверждено: `if key.endswith("_entity")` | +| `range_v1`/`zones_v1` называют основной источник `entity_id`, а не `*_entity` | `radar_validation.py:244, 259` | Подтверждено: `unique_role(item.get("entity_id"), ...)` для ranges, `_entity(item.get("entity_id"), ...)` для zones. `"entity_id".endswith("_entity")` — ложно (сравнил посимвольно) | +| Coordinator реально читает `entity_id` при построении frame | `radar.py:502, 531` | Подтверждено: `self.hass.states.get(source.get("entity_id"))` | +| Подписки коллектора берутся из того же неполного `radar_source_entity_ids()` | `radar.py:189-193` | Подтверждено: `entity_ids = sorted({... for entity_id in radar_source_entity_ids(...)})`, передаётся в `async_track_state_report_event`/`_change_event` | +| `_can_read()`/ACL получает тот же неполный список | `radar_websocket.py:56, 128, 184, 224-225` | Подтверждено: все проверки идут через `coordinator.source_ids()` → `radar_source_entity_ids()` либо `validate_radar_draft()` → та же функция | +| Setup subscribe не имеет секундного tick-фолбэка, в отличие от live | `radar_websocket.py:196-251` vs `radar.py:258-268` (`_ensure_tick`) | Подтверждено: `_ensure_tick`/периодическая публикация существует только у `RadarCoordinator` (для активных `add_listener`), у `ws_radar_setup_subscribe` publish идёт исключительно по `source_reported` на пересчитанном `source_ids` | +| `restricted`/`source_restricted` — уже существующее fail-closed поведение, а не новое | `radar_websocket.py:57,132,185,226,239` | Подтверждено, задача переиспользует существующий путь, не изобретает новый | +| Существующий тест использует общий allow/deny boolean, а не per-entity fake (риск в разделе «Риски») | `tests_backend/test_ha_radar_websocket.py:23` (`_Permissions`, `connection.user.permissions.allowed = False/True`) | Подтверждено: это ровно тот недостаточный дизайн, который ТЗ требует не повторять в AC4 | +| `mutation-gate.mjs` умеет патчить backend `.py` и гонять `pytest`-гвард | `scripts/mutation-gate.mjs` (например записи с `guard: 'python3 -m pytest tests_backend/...'`, `file: 'custom_components/houseplan/*.py'`) | Подтверждено, AC5 технически осуществим на этом реестре | +| Перечисленные роли по профилям (общие optional + по группам `slots`/`ranges`/`zones`) | `radar_validation.py:198-266` | Построчно совпадает с контрактом ТЗ п.2–3 для всех шести профилей, включая отсутствие `presence_entity` у zones и обязательность `occupancy_entity` только для `presence_v1` | + +Ни одно из проверенных утверждений не оказалось догадкой, выданной за факт: +там, где ТЗ формулирует именно новое поведение (например, п.6–7 контракта — +«saved coordinator подписывается на оба HA event stream для полного +инвентаря»), это явно нормативное требование к реализации, а не описание +того, что уже так работает. + +## Обязательные разделы §7.1 + +Все присутствуют: сценарий · что человек увидит до/после · проблема · скоуп +и не-скоуп · контракт поведения · UX · модель данных/миграция/совместимость · +затронутые файлы · AC1…AC7 с доказательством · план автотестов · перф · +безопасность · риски · откат · release-артефакты · блок «принято +предположительно». Пункт DoR по i18n закрыт явным «frontend/i18n не +меняются» — это корректная форма пустого списка, а не пропуск раздела. + +## Проверка AC1…AC7 на однозначность и доказуемость + +- **AC1** (backend unit) — таблица для всех 6 профилей, включая точное имя + роли `entity_id` у range/zone и отрицательный случай произвольного + future-поля. Проверяемо механически, способ доказательства назван. +- **AC2** (backend HA) — саму развилку «что считается обходом» ТЗ снимает + прямым запретом полагаться на occupancy/count/presence в фикстуре; это + устраняет ровно тот тип ложного прохождения теста, что PROCESS.md требует + предотвращать (§2.7, «двух источников с разными списками»). +- **AC3** (backend HA) — три отдельных факта (subscribe range/zone id, + rebind заменяет набор целиком, unload снимает всё) — все три проверяемы по + отдельности, не слиты в одно расплывчатое утверждение. +- **AC4** (backend HA, security) — явно требует записывающий per-entity fake + permission (не общий boolean) и раздельно saved/draft путь — снимает + собственный риск, названный в разделе «Риски», а не просто декларирует его. +- **AC5** (mutation) — конкретный id мутанта, конкретная механика удаления + роли, обязательный `--check`. Соответствует формату существующего реестра. +- **AC6** (backend + ревью кода) — числовой инвариант («два listener») + сверяемый по факту: сейчас уже два (`state_report`+`state_change`) и на + коордиторе, и на setup — AC фиксирует неизменность этого числа при + расширении набора entity, а не декларирует что-то новое и непроверяемое. +- **AC7** (backend + docs review) — стандартный набор доказательств + совместимости и артефактов, ничего скрытого. + +Ни один AC не сформулирован как «неявное намерение», каждый называет способ +доказательства (`unit`/`backend HA`/`mutation`/`ревью кода`), как того +требует §2.5. + +## Продуктовые разделы и вопрос владельцу + +«Сценарий» называет персону/поверхность/момент (home admin, мастер настройки +или обычный план, изменение основного датчика диапазона/зоны), «Что человек +увидит» — одной фразой без терминов реализации. Автор аналитики (S2) +заявила, что открытых продуктовых вопросов нет, потому что ожидаемое +поведение уже зафиксировано существующим контрактом `docs/RADAR.md` +(«House Plan subscribes only to the exact configured entity IDs») и не +вводит нового UI/UX. Прочитав материал независимо, соглашаюсь: расхождение +между валидацией и inventory — техническая ошибка одной реализации, а не +неопределённость продукта, поэтому пачка вопросов владельцу здесь была бы +избыточной (§7.1 «вопрос, не блокирующий написание ТЗ, не задаётся вовсе»). + +## Скоуп и не-скоуп + +Не-скоуп корректно исключает Stage 2/3, историю, heatmap, изменения +`may_write`/прав HA и миграцию — всё это действительно посторонние +подсистемы. Скоуп ограничен одной точкой истины (profile → source roles) и +её тремя потребителями (validation, subscriptions, ACL), что соответствует +корню проблемы, а не симптомам по отдельности. + +## Откат + +Явно запрещён частичный откат «только ACL» или «только подписок» — важная +деталь: такой частичный откат воссоздал бы именно тот дефект, который +задача чинит (расхождение списков), и явный запрет предотвращает случайную +регрессию через невнимательный cherry-revert. + +## Что проверено и корректно + +- Все шесть заявленных факта о текущем коде подтверждены построчным чтением + (таблица выше), включая тонкий момент: `"entity_id".endswith("_entity")` + действительно ложно. +- Все файлы из «Затронутые файлы» существуют. +- Роли по профилям в контракте поведения дословно совпадают с + `_validate_sources()`. +- Мутационный гейт поддерживает заявленный тип мутанта на Python-файле. +- Откат, риски и не-скоуп внутренне непротиворечивы. + +## Чего не проверял + +- Не запускал автотесты и не собирал бандл — на этапе ревью ТЗ кода ещё нет, + проверять нечего; это не гейт этого этапа. +- Не оценивал качество будущей реализации — вне скоупа ревью ТЗ. +- Не проверял `docs/specs/485-radar-presence-stage1.md` построчно на полное + совпадение (упомянут в `docs/RADAR.md` как нормативный контракт Stage 1); + сверка ограничилась текущим кодом, что для проверки *этого* багфикса + достаточно. + +## Находки + +Нет. Ни одной High, ни одной Medium, ни одной Low. + +## Вердикт + +Зелёный. ТЗ полное по §7.1, каждый AC однозначен и снабжён способом +доказательства, продуктовые разделы отвечают на свои вопросы, ни одно +техническое утверждение не оказалось непроверенной догадкой — напротив, +каждое фактическое заявление о текущем коде подтвердилось при чтении +источника, включая нетривиальные детали (dead-code ветка zones в +`_resubscribe_sources`, недостаточность существующего permission-fake). +Открытых продуктовых вопросов владельцу нет и не требуется. + +--- + +## Материал раунда + +- SHA issue-материала: тело issue #545 на момент разбора (заход r1, первый + документ ревью для этой задачи — предыдущего раунда нет). +- Ветка/код: не применимо (S4-spec-review, кода ещё нет). + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `ce6650b1e05a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `8aa210d76f61c0edd53530a6f88f7d98567918dd` + ``` + git log --all --format='%H %T' | grep 8aa210d76f61 + ``` +- Тело issue: `8b063fe89abbf48179a894847dff1b5c223f394fa4f6e98ec284302624b71437` +- Вердикт конвейера: `green` · High 0