docs: review document for #226

Issue: #226
User-Visible: no

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

@@ -0,0 +1,218 @@
+# SPEC-REVIEW — issue #226, цикл r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/226
+- **ТЗ:** `docs/specs/226-entity-parent-dedup.md`, коммит `5feabed` (ветка
+  `issue/226-entity-parent-dedup`)
+- **Трек:** обычный полный (не `small`), лимит циклов ревью ТЗ — 4
+- **Ревьюер:** Claude (роль «Ревьюер ТЗ», отдельная сессия от автора)
+- **Вердикт:** жёлтый · цикл r1/4 · High: 0 · Medium: 1 → в задаче
+
+## Скоуп проверки
+
+Issue #226 — баг: явно размещённый `entity:X` не вычитается из авто-обнаруженного
+родительского `device:D`, поэтому один физический прибор задваивается на плане
+(классический вход — HA-хелпер «Switch as X»). ТЗ вводит частичное ownership
+между entity-markers и родительским устройством плюс отдельные правила для
+`hidden_by`, tombstone, marker.hidden и явной пары `device:D + entity:X`.
+
+Это первый цикл — раздел «объём по дельте» (§2.10 PROCESS.md) не применяется,
+разбор выполнен полностью:
+
+1. соответствие ТЗ обязательным разделам §7.1 PROCESS.md;
+2. однозначность и доказуемость AC1…AC10;
+3. отсутствие догадок, выданных за факт без пометки «предположение»;
+4. соответствие ТЗ реальному коду `src/devices.ts`, `src/ha-binding-status.ts`
+   и канону (`docs/SCOPE.md`, `docs/FILTERING.md`, `docs/TOUCH-SUPPORT.md`);
+5. что вопросы, заданные и не заданные владельцу, действительно продуктовые
+   либо действительно технические.
+
+## Как проверялось
+
+- Прочитаны `docs/SCOPE.md` (J1/J3/J4/J6 — единый правдивый объект, однозначное
+  действие, предсказуемая настройка, план остаётся верным при появлении
+  HA-сущностей), `AGENTS.md`, `PROCESS.md` целиком (§2.4, §2.10, §7.1, §7.2, §12).
+- Прочитано тело issue #226 и все пять комментариев: аналитика, пакет вопросов
+  Q1-Q3, решения владельца по каждому вопросу, занятие, публикация ТЗ. Метка на
+  момент ревью — `S4-spec-review`.
+- Прочитан `docs/specs/226-entity-parent-dedup.md` целиком (19 разделов).
+- Прочитан текущий код `src/devices.ts` построчно в проблемной области:
+  `entitiesByDevice()` (:42-51, не фильтрует `hidden`, только
+  `isRegistryEntryEnabled`), `claimed`-цикл (:1032, ровно тот баг, что описан в
+  issue — `claimed.add(m.binding)` кладёт точный `entity:X`), авто-устройства
+  (:1032-1042, `claimed.has('device:' + dev.id)` — не видит entity-привязку),
+  `seedHiddenBindings()` (:956-990), `primaryEntity`/`resolvedDeviceStateEntities`/
+  `visibleFirst` (:99-177, уже читают `reg.hidden` как существующий паттерн —
+  ТЗ его не изобретает, а переиспользует).
+- Прочитан `src/ha-binding-status.ts`: `isRegistryEntryEnabled` (`disabled_by ==
+  null`, не трогает hidden), `activeRegistryHass`/`fullRegistryHass` (проекции по
+  `disabled_by`, hidden не фильтруют), `resolveHaBindingStatus` (`ha_disabled` —
+  тоже по `disabled_by`, не по `hidden_by`) — подтверждает заявление ТЗ §5, что
+  `hidden_by` сейчас не участвует ни в одной из этих проекций.
+- Прочитан `docs/FILTERING.md` (строки 190-217) — контракт #94 (Aqara Roller
+  shade E1, `cover.*` скрыт интеграцией, видимый `switch.*_reverse_direction`,
+  cover-first resolver) подтверждён дословно тем же текстом, что цитирует ТЗ §8.
+- Прочитан ранее реализованный `docs/superpowers/specs/2026-08-08-ha-disabled-devices-design.md`
+  (инвариант 5: «`hidden_by` сущности HA не равен `disabled_by`: скрытая в
+  интерфейсе HA сущность остаётся рабочей для House Plan») — ТЗ #226 этому канону
+  не противоречит: не превращает `hidden_by` в глобальный фильтр, использует его
+  только как узкий критерий «не поддерживает остаток» (§3.2-3.3 ТЗ).
+- Прочитан `docs/TOUCH-SUPPORT.md` (строки 9-40) — формулировка ТЗ §17 «View и
+  kiosk release-blocking» совпадает с каноном дословно.
+- Проверено docs/CONFIG-COMPATIBILITY.md на предмет требований к чисто
+  runtime-правкам без изменения схемы — заявление ТЗ §9 «схема, backend,
+  storage version и wire protocol не меняются» противоречий не имеет.
+- Гейты кода в этом цикле не запускались: предмет ревью — ТЗ, реализации ещё
+  нет (стадия `S4-spec-review`, изменён только класс C — `docs/specs/**`).
+
+## Находки
+
+### Medium — в скоупе задачи
+
+**M1. Комбинация «явный marker на видимую сущность + единственный оставшийся
+сиблинг скрыт HA и функционально важен (сценарий #94)» не имеет ни AC, ни теста,
+хотя решённые в ТЗ правила делают её исход неочевидным и потенциально
+регрессионным именно для дважды уже чинившегося контракта штор.**
+
+Разбор по коду и ТЗ:
+
+- §3.3 ТЗ: «Hidden sibling не удерживает остаток» — HA-hidden сущность не
+  считается основанием для остаточного auto-marker.
+- §6 шаг 5: «Если `visibleResidual(D)` пуст, auto-marker не добавляется. Наличие
+  только hidden siblings не считается остатком.»
+- §8: «Только **остаточный** auto-marker, возникший после явного entity-marker,
+  применяет правило «hidden siblings не удерживают остаток»» — то есть защита
+  #94 (тест-кейс 8) прямо ограничена **нетронутым** устройством.
+
+Возьмём буквально сценарий #94: устройство `D` = штора, `cover.curtain` скрыт
+интеграцией, `switch.reverse_direction` виден. Пользователь размещает
+`entity:switch.reverse_direction` (ровно тот класс действия, который #226
+описывает как «выбирает то, что видит» — здесь это доступный видимый service
+switch). По правилам ТЗ: `switch.reverse_direction` уходит под точный
+entity-marker; единственный оставшийся сиблинг `cover.curtain` — скрыт HA и по
+§3.3/§6.5 не формирует остаток → `visibleResidual(D)` пуст → auto-marker `D`
+целиком исчезает с плана.
+
+До фикса #226 при том же действии пользователя `D` продолжал бы строиться как
+полный auto-device (баг #226 создаёт **дубль**: и entity-marker, и полноценный
+auto-device с cover-first резолвером). После фикса, по буквальному прочтению
+принятого правила, дубль устраняется ценой **потери самого объекта**: шторы
+целиком нет на плане, хотя до задачи #226 функциональное представление шторы
+(пусть и задвоенное) на плане было. Это худший, а не нейтральный исход именно
+для контракта, который #94 чинил дважды (`docs/FILTERING.md` — «Why the cover is
+FIRST and not third», «шторы никогда не жёлтые» уже дважды пробивалось
+соседними правилами).
+
+Формально это не противоречит букве принятого владельцем default по Q2 (owner:
+«При расчёте остатка после отдельного entity-marker не считать скрытые siblings
+основанием для второго auto-marker»), и потому не новый неотвеченный
+продуктовый вопрос по существу правила. Но конкретное следствие — «устройство
+может исчезнуть с плана целиком, если пользователь разместит маркер на видимую
+вспомогательную сущность шторы» — нигде в ТЗ не названо явно и не имеет
+собственного теста в матрице §12 (кейс 7 испытывает HA-hidden **саму** `X`,
+кейс 8 испытывает **нетронутое** устройство; комбинации «видимая размещённая
+сущность + единственный оставшийся сиблинг скрыт и функционально первичен» нет
+ни в одном из 14 кейсов).
+
+**Почему это Medium, а не Low.** Дефект не в правиле как таковом (оно — решение
+владельца), а в отсутствии доказательства и явной фиксации именно этого
+пограничного исхода в зоне, которая уже дважды была источником регрессии (#94
+и последующий DEV-1DA1-01/DEV-2C947-01, оба упомянуты в `docs/FILTERING.md`).
+Без явного теста разработчик может реализовать правило иначе (например, счесть,
+что «функционально протected» сиблинг вроде cover-first-резолвера обязан
+удерживать остаток), и оба прочтения пройдут существующую матрицу тестов ТЗ
+одинаково зелёным — расхождение проявится только на живом Aqara-конфиге
+пользователя.
+
+**Фикс, ожидаемый в этом же цикле:** добавить в §12 (матрица тестов) явный
+пятнадцатый кейс — размещённая видимая `entity:switch.reverse_direction`,
+единственный оставшийся сиблинг `cover.curtain` скрыт HA — и явно зафиксировать
+в §8/§3.3 ожидаемый результат (auto-marker `D` исчезает — если это осознанно
+принимается; либо cover-first защита #94 переживает и это состояние, если нет).
+Достаточно одного предложения в ТЗ плюс одной строки в матрице; выбор из двух
+исходов — техническое решение автора по формулировке правила, разногласие
+с ревьюером решается вердиктом, а не владельцем (`PROCESS.md` §7.1).
+
+### Low
+
+Не найдено находок, которые стоило бы фиксировать отдельно и не чинить.
+
+## Что проверено и корректно
+
+- Обязательные разделы §7.1 присутствуют по существу: сценарий и персона (§1,
+  включает и постановку проблемы), что человек увидит до/после (§2, без
+  терминов реализации), скоуп/не-скоуп (§4), контракт поведения (§3/§6-8),
+  модель данных и совместимость (§9), i18n (§10 — не требуется, обоснованно:
+  нет новых строк UI), критерии приёмки AC1…AC10 с доказательством (§13), план
+  автотестов (§12/§14), риски (§18), откат (§18), release-артефакты (§16).
+  Отдельного заголовка «Проблема» нет — текст интегрирован в §1 без потери
+  содержания, не блокирующее замечание.
+- Все три продуктовых вопроса (Q1 частичное ownership, Q2 `hidden_by` без
+  регрессии #94, Q3 асимметрия `device:D`/`entity:X`) заданы владельцу пачкой
+  с default и получили явные ответы **до** написания ТЗ — процесс §7.1
+  соблюдён, ТЗ переносит принятые решения, а не изобретает их.
+- Причина дефекта, описанная в §3 ТЗ и в самом issue, точно соответствует коду:
+  `claimed.add(m.binding)` (:1032 `src/devices.ts`) кладёт ровно
+  `entity:light.liustra`, цикл авто-устройств (:1039) проверяет только
+  `claimed.has('device:' + dev.id)` — связь entity→device в `claimed` не
+  участвует. Смежная находка про `hidden_by`/`entitiesByDevice()` тоже
+  подтверждена чтением: функция (:42-51) фильтрует только `disabled_by`.
+- Все продуктовые решения §3 (1-6) прослежены до конкретных AC и тест-кейсов
+  без пропусков: partial ownership → AC1/AC2; `hidden_by` не глобальный фильтр
+  → AC4/кейс8; hidden sibling не держит остаток → AC4/кейс7; явная асимметрия
+  `device:D`/`entity:X` → AC3/кейс4; tombstone не владеет → AC3/кейс6; marker
+  hidden сохраняет ownership → AC4/кейс5.
+- Алгоритм (§6) явно требует линейную сложность `O(markers + entities +
+  devices)` и запрещает вложенный поиск — соответствует требованию
+  производительности §17 и не противоречит текущей структуре `buildDevices()`
+  (уже строит `claimed`/`entsBy` за один проход).
+- Защита #94 сформулирована через прямую цитату существующего канона
+  (`docs/FILTERING.md:190-217`) и получила отдельный обязательный регрессионный
+  тест (кейс 8) для **нетронутого** устройства — совпадает с текстом канона
+  дословно, не пересказ на слово.
+- Раздел «Принятые предположения» (§19) — реальные технические допущения
+  (семантика «видимой» entity как отсутствие `reg.hidden`, отсутствие новых
+  настроек/переводов), не подмена продуктового решения: все пять пунктов
+  либо прямо повторяют ответы владельца, либо являются нейтральными
+  техническими деталями (инертность layout key уже была решена в самом issue
+  автором аналитики).
+- Не-скоуп (§4) корректно исключает автослияние/удаление двух явных markers,
+  смену выбора primary у полного device-marker, глобальное исключение
+  `hidden_by`, миграцию и новый config — то есть ТЗ не расширяет задачу дальше
+  системного правила ownership, заявленного в issue.
+- Touch/kiosk (§17) корректно унаследован как release-blocking для View и
+  kiosk по действующему `docs/TOUCH-SUPPORT.md`, без нового touch-контракта.
+- Compatibility (§9) корректен: изменение чисто runtime-проекции без записи
+  конфига не требует миграции, схема/backend/wire protocol не меняются —
+  подтверждено отсутствием противоречий в `docs/CONFIG-COMPATIBILITY.md`.
+- Track (обычный, не `small`) выбран верно и обоснован в issue самим автором
+  аналитики: несколько потребителей (`buildDevices`, seeder, light/Glow, LQI,
+  два редактора), конфликт с уже принятым поведением штор — критерий «одна
+  поверхность» для лёгкого трека не выполняется.
+- Mutation-gate (§14 ТЗ) сузил предложенный в issue список с трёх до двух id
+  (`entity-marker-kept-in-parent-device`, `entity-marker-parent-seeded`) без
+  отдельного мутанта на `hidden_by`-регрессию — но это техническое решение
+  автора о стратегии тестов (`PROCESS.md` §7.1: «стратегия тестов… агенты
+  решают сами»), не продуктовая догадка; AC4 всё равно доказывается unit-кейсами
+  5/7/8/9 без мутационного гейта на каждый.
+
+## Чего не проверял
+
+- Код `buildDevices()`/`seedHiddenBindings()` реализации ещё не существует
+  (стадия `S4-spec-review`, изменён только класс C) — `npx tsc --noEmit`,
+  `npm test`, `npm run build` не запускались: собирать пока нечего.
+- Golden/browser smoke/performance/mutation-gate не запускались по той же
+  причине — предмет код-ревью (§2.7), не ревью ТЗ (§2.4).
+- Не проверялась построчно вся `docs/USER-GUIDE.ru.md`/`docs/USER-GUIDE.md` на
+  предмет других мест, описывающих текущее (дублирующее) поведение auto-device —
+  целевой grep по «auto-device», «родительск», «Switch as X» ограничен разделом
+  §16 ТЗ, где эти файлы названы release-артефактом; полнота их будущей правки —
+  предмет код-ревью, не ревью ТЗ.
+- Не проверялась историческая полнота предыдущих SPEC/CODE-REVIEW документов
+  по #170 (связанная находка) на предмет собственных незакрытых находок — вне
+  предмета этого ревью.
+
+## Раздел «Унаследовано» — не применяется
+
+Это первый цикл ревью ТЗ (r1); раздел «Унаследовано из r<N-1>» и таблица
+закрытия предыдущего раунда не ведутся (§2.10 PROCESS.md действует со второго
+цикла).