docs: spec review document for #178

Issue: #178
User-Visible: no

docs/reviews/SPEC-REVIEW-178-r1.md

@@ -0,0 +1,227 @@
+# SPEC-REVIEW-178-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/178
+- **ТЗ:** [`docs/specs/178-toggle-entity.md`](../specs/178-toggle-entity.md)
+  (commit `e46ef6f55c44ec1f05268cdff4ceeb4dcc5af116`, ветка `issue/178-toggle-entity`)
+- **Ревьюер:** Claude (ревью ТЗ ≠ автор), этап `S4-spec-review`
+- **Цикл:** r1/4 (обычный трек — issue не `small`/`trivial`, подтверждено
+  комментарием аналитики и меткой `feature` без `small`)
+
+## Скоуп ревью
+
+Ревью ТЗ #178: новое optional поле `marker.toggle_entity`, дающее пользователю
+выбор конкретной собственной `light.*`/`switch.*`-сущности для действия
+«Переключить состояние» у составных устройств (сейчас цель выбирает эвристика
+`resolveOwnEntity()`/`ownRoleCandidates()`), плюс диалоговый селектор,
+stale-fallback, влияние на explicit controls-group, backend-валидацию и
+export/import.
+
+Не в скоупе ревью: код ещё не написан (issue в `S4-spec-review`,
+`git diff origin/dev...HEAD` для `src/**` и `custom_components/**/*.py` пуст —
+проверено), поэтому проверка реализации, тестов и гейтов — предмет будущего
+code-review (PROCESS.md §2.7).
+
+## Как проверялось
+
+Прочитано в заданном порядке:
+
+1. `docs/SCOPE.md` — цель задачи привязана к J3 («Let me act on the obvious
+   right from the plan»); лок-инвариант (§«The lock invariant») не затронут —
+   `toggle_entity` ограничен `light.*`/`switch.*`, что явно исключает
+   `lock.*`/`alarm_control_panel.*`.
+2. `AGENTS.md`, `PROCESS.md` целиком (включая §2.4, §2.5 DoR, §7.1, §5, §8).
+3. Issue #178 body и все 3 комментария: аналитика владельца (оценка, связанные
+   задачи, подтверждённые технические контракты), «взял в работу», «ТЗ готово».
+4. `docs/USER-GUIDE.ru.md` — строки 541, 571–590, 645–650, 780–781: термины
+   «Переключить состояние», «Ведущая сущность» (для `light_entity`, отдельное
+   поле) сверены с §2 и §9.2 ТЗ.
+5. `docs/CONFIG-COMPATIBILITY.md` (раздел про `marker.light_entity`,
+   строки 176–191) и `docs/TOUCH-SUPPORT.md` целиком — канонические документы,
+   которые ТЗ обязано соблюсти для поля-precedent и для editor-фичи.
+6. Само ТЗ `docs/specs/178-toggle-entity.md` целиком.
+7. Текущий код на той же ветке/коммите (продуктовый код не менялся) — построчно
+   сверены все фактические утверждения ТЗ о текущем поведении:
+   - `src/device-toggle.ts` — `ownRoleCandidates()` (370–392), `resolveOwnEntity()`
+     (401–423), `resolveControls()` (573–646), `toggleOriginOf()` (459–463);
+   - `src/devices.ts` — `ownControllableEntities()` (317–325),
+     `forcedLightEntityOf()` (329–336), `persistedExternalControls()` (288–300);
+   - `src/houseplan-card.ts` — диалоговый паттерн `light_entity`
+     (11924–11968, 12356–12358, 18398–18428: native `<select>`, Auto-опция,
+     `friendly name · entity_id`, stale-warning с `role="status"`,
+     `_announceToggleDraft`/`_markerDraft`, 17735–17913, 18014–18297);
+   - `custom_components/houseplan/validation.py` (`validate_marker_light_entities`,
+     189–220) и `websocket_api.py`/`import_export.py` — границы вызова валидатора;
+   - `custom_components/houseplan/import_export.py:895–897` — точный allowlist
+     полей, удаляемых при `duplicate_policy: virtual` (подтверждает claim §7.4).
+   Совпадение везде построчно точное — ни одна ссылка на код в ТЗ не оказалась
+   пересказом или догадкой.
+8. Прецедентные ТЗ этого же формата — `docs/specs/174-linked-virtual-light-controller.md`
+   и `docs/specs/164-washer-active-cycle.md` — чтобы установить, какие разделы
+   §7.1 в этом репозитории считаются обязательными на практике (обе содержат
+   отдельные «Риски/perf/security», «Откат», «Release-артефакты», «UX/i18n/touch»).
+
+Гейты (`typecheck`/`test`/`build`) не прогонялись — на этапе ревью ТЗ
+продуктового кода нет, прогон гейтов не относится к этому этапу (PROCESS.md
+§2.7/§8).
+
+## Находки
+
+### High-1 — отсутствуют обязательные разделы: риски, откат, влияние на touch/производительность
+
+**Файл:** `docs/specs/178-toggle-entity.md` (весь документ; список разделов —
+строки 15–426).
+
+PROCESS.md §7.1 называет обязательными разделами ТЗ, помимо прочих, **риски**,
+**откат** и (через DoR §2.5) явно названное **влияние на производительность**
+и **влияние на touch по `docs/TOUCH-SUPPORT.md`**. В документе нет ни одного
+из них — ни как отдельного раздела, ни растворённым в другом разделе. Полный
+список заголовков документа:
+
+```
+1. Сценарий и цель       6. Non-scope           11. Runtime group Toggle
+2. Что изменится...      7. Модель данных...    12. Backend и валидация
+3. Подтверждённое...     8. Кандидаты...        13. i18n и документация
+4. Решения владельца     9. Диалог устройства   14. Тестовый контракт
+5. Scope                 10. Runtime single...  15. Acceptance criteria
+                                                 16. Принятые предположения
+```
+
+Ни «риски», ни «откат», ни «touch», ни «производительность» не встречаются
+(проверено `grep -n -i` по всему файлу — совпадений нет, кроме одного числового
+«риск 4/10» в шапке-метаданных, унаследованного из issue, а не как раздел
+самого ТЗ).
+
+Это не формальная придирка: соседние ТЗ той же сложности и того же трека,
+принятые по этому же процессу, содержат эти разделы явно —
+`docs/specs/174-linked-virtual-light-controller.md` (§15 «Риски, performance и
+security», §16 «Откат», §17 «Release-артефакты», §12 «UX, i18n, accessibility
+и touch») и `docs/specs/164-washer-active-cycle.md` (§15/§16/§17/§18
+аналогично). `docs/TOUCH-SUPPORT.md:147–151` формулирует требование буквально:
+«New editor feature specifications and code reviews must state one of:
+`Touch editor: supported`; `Touch editor: best effort / intentionally
+degraded`; `Touch editor: not exposed`». #178 — ровно editor feature
+specification (новый `<select>` в диалоге устройства), и такой строки в
+документе нет вовсе.
+
+**Воспроизведение:** `grep -n -i "риск\|откат\|rollback\|touch\|производительн" docs/specs/178-toggle-entity.md`
+возвращает пустой список (кроме заголовка-метаданных с оценкой сложности).
+Автор код-ревью, дойдя до вопроса «какой откат у этой фичи, если баг
+проявится после релиза» или «это editor-фича, какой у неё touch-статус»,
+не найдёт ответа в каноническом документе — придётся выяснять по памяти или
+логике задним числом, что PROCESS.md §7.1 и прямо называет недопустимым
+(«ТЗ, которое не может ответить на эти два вопроса, описывает работу, а не
+изменение продукта» — тот же принцип применим к остальным обязательным
+разделам).
+
+Содержательно риск, скорее всего, невелик — поле optional, миграции нет
+(§7.2), диалог основан на native `<select>` (обычно touch-нейтрален), а
+основной риск совместимости (single vs group resolution) фактически разобран
+внутри §10–§11. Но раздел должен явно резюмировать это как «риск», а не
+рассеивать по контрактным пунктам, и явная touch-строка должна присутствовать
+буквально по требованию канона, а не подразумеваться.
+
+**Вердикт:** блокирует. Возврат в `S3-spec`: добавить разделы «Риски»,
+«Откат», «Touch editor: …» и явное «Производительность: …» (или «нет»).
+Содержание может быть коротким — судя по анализу выше, реальных
+неразрешённых рисков не открывается, — но раздел должен существовать и
+явно проговорить то, что сейчас можно только вывести по контексту.
+
+Больше High/Medium-находок нет.
+
+### Low-1 — способ доказательства не привязан к номеру AC явно
+
+**Файл:** `docs/specs/178-toggle-entity.md:406–424` (§15 Acceptance criteria).
+
+PROCESS.md §2.5/§7.1 требуют, чтобы у каждого AC было «указано, чем он
+доказывается». В §15 двенадцать пунктов AC не содержат инлайн-пометки
+`unit`/`smoke`/`golden`/«ревью кода» — способ доказательства восстанавливается
+только косвенно, сверкой с отдельным §14 (тестовый контракт), где нумерация
+тестов не совпадает 1:1 с нумерацией AC. Например AC1 («виден selector при
+двух кандидатах») доказывается smoke-тестом §14.2 п.1, а не одним из
+unit-тестов §14.1 — это можно восстановить логически, но не прочитать
+напрямую.
+
+**Воспроизведение неоднозначности:** код-ревьюер, сверяя «AC7 доказан?»,
+должен самостоятельно найти соответствие AC7 → unit-тесты §14.1 пп.7–9, а не
+прочитать это в самом §15.
+
+**Вердикт:** не блокирует. Содержательно способ доказательства
+восстанавливается однозначно по §14 для каждого AC — переноса на новый цикл
+не требует. Снимаю с запиской: на будущей ревизии (или в самом коде, в имени
+теста/комментарии) стоит явно указать `(doc: unit §14.1.N)` в каждом пункте
+§15, чтобы код-ревью не тратило цикл на восстановление соответствия.
+
+## Что проверено и корректно
+
+- **Технические утверждения о текущем коде верны построчно.** Каждая ссылка
+  на `resolveOwnEntity`, `ownRoleCandidates`, `ownControllableEntities`,
+  `forcedLightEntityOf`, `persistedExternalControls`, диалоговый паттерн
+  `light_entity` (native select, Auto-опция, warning, `_announceToggleDraft`)
+  и backend-границы валидации (`validate_marker_light_entities`,
+  `websocket_api.py`, `import_export.py`) сверена с реальным кодом и совпадает.
+  Ни одна догадка не выдана за факт.
+- **Продуктовые решения владельца перенесены точно.** Все 9 пунктов §4
+  («Решения владельца») буквально соответствуют формулировкам issue body и
+  комментария аналитики от 18.08.2026 (рабочее имя поля, видимость селектора,
+  бит-в-бит совместимость, stale-поведение, live-preview, буквальный
+  entity id, независимость от `light_entity`/`tap_target`, участие в группе,
+  исключение `marker:*`).
+- **Scope/Non-scope (§5/§6) согласованы** с прецедентом #88/#84 и явно
+  исключают домены за пределами `light.*`/`switch.*`, изменение confirmation
+  flow (#103) и `marker:*` (#107/#174) — ничего из этого владелец не просил.
+- **Compatibility без миграции (§7.2, §7.3) корректна и проверяема**:
+  отсутствие поля = прежняя цепочка бит-в-бит; stale-значение не стирается,
+  runtime не вызывает service по stale id — то же поведение, что уже
+  реализовано и задокументировано для `light_entity`
+  (`docs/CONFIG-COMPATIBILITY.md:176–182`).
+- **Влияние на explicit controls-group (§11) корректно разграничивает**
+  совместимый режим (без explicit поля группа остаётся external-only) и новый
+  режим (явно выбранная собственная сущность входит в группу без изменения
+  правила `any-on → turn_off all`) — соответствует решению владельца §4.8 и
+  не меняет уже принятую семантику группы.
+- **Backend-раздел (§12) корректно указывает те же точки вызова валидатора**,
+  что использует `light_entity` (`websocket_api.py:1256,1367`,
+  `import_export.py:980,1160-1162`), включая `validate_all=True` для полного
+  импорта — подтверждено чтением кода, а не пересказом.
+- **Export/import и virtualize-политика (§7.4) точно называют существующий
+  allowlist** (`import_export.py:895-897`) и корректно требуют добавить туда
+  новое поле — без этого явного шага поле осталось бы в virtual-маркере,
+  указывая на HA entity, которого больше нет.
+- **AC однозначны по формулировке** — ни один пункт §15 не читается двумя
+  взаимоисключающими способами (в отличие от найденного в прецеденте #174
+  Low-1); граничные случаи (одна кандидатка, stale, transient unavailable,
+  cover/virtual paths) описаны явно и без противоречий.
+- **Продуктовых вопросов владельцу не осталось.** Единственный источник
+  продуктовой неопределённости («что видит пользователь, до какого объёма») уже
+  закрыт issue body и комментарием аналитики; ТЗ не выдаёт ни одной технической
+  догадки за продуктовое решение — раздел §16 «Принятые технические
+  предположения» корректно отделяет 5 свободно изменяемых технических решений
+  (обобщение `ownControllableEntities()`, паттерн touched/write-fields, формат
+  `friendly name · entity_id`, `via` для diagnostics, отсутствие schema-версии)
+  от нормативных решений §4, и ни одно из них не маскирует продуктовый вопрос.
+- **Трассируемость issue ↔ ТЗ на месте**: `docs/specs/README.md:98` ссылается
+  на `178-toggle-entity.md`, документ ссылается на issue в шапке.
+
+## Чего не проверял
+
+- Реализацию — её нет: issue в `S4-spec-review`, `git diff origin/dev...HEAD`
+  для `src/**` и `custom_components/**/*.py` пуст (проверено).
+- Гейты `typecheck`/`test`/`build`/browser smoke — не относятся к этапу ревью
+  ТЗ; предмет будущего code-review (PROCESS.md §2.7/§8).
+- `python -m pytest tests_backend` — backend не менялся на этой ветке.
+- Golden/perf-эталоны — ТЗ описывает будущие сценарии (§14.3), реализации для
+  сверки нет.
+- Не проверял вручную UI/диалог в браузере — на этом этапе кода не существует;
+  сверка велась только с уже реализованным прецедентом `light_entity` по коду.
+
+## Вердикт
+
+Единственная блокирующая находка — отсутствие обязательных по PROCESS.md §7.1
+и `docs/TOUCH-SUPPORT.md` разделов (риски, откат, touch/производительность),
+которые в двух прецедентных ТЗ того же трека присутствуют явно. Остальное
+содержание ТЗ добротно: технические утверждения о коде проверены построчно и
+верны, продуктовые решения владельца перенесены точно, AC однозначны, догадок
+под видом фактов не найдено.
+
+**Вердикт: красный · цикл r1/4 · High: 1 · Medium: 0 → нет · Документ:
+docs/reviews/SPEC-REVIEW-178-r1.md**