diff --git a/docs/reviews/SPEC-REVIEW-226-r1.md b/docs/reviews/SPEC-REVIEW-226-r1.md new file mode 100644 index 00000000..55da7cf2 --- /dev/null +++ b/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» и таблица +закрытия предыдущего раунда не ведутся (§2.10 PROCESS.md действует со второго +цикла).