docs: review document for #107

Issue: #107
User-Visible: no

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

@@ -0,0 +1,294 @@
+# SPEC-REVIEW-107-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/107
+- **ТЗ под ревью:** `docs/specs/107-virtual-light-toggle.md` (коммит `af851cd`,
+  ветка `issue/107-virtual-light-toggle`)
+- **Роль:** ревьюер ТЗ (не автор), этап `S4-spec-review`
+- **Трек:** обычный (не `small`/`trivial`) — автор корректно не поставил `small`:
+  задача вводит новый backend WS-контракт, отдельное operational-хранилище и
+  новый UX-контракт target preview, что прямо нарушает критерии §5 PROCESS.md
+  («нет нового UX-контракта», «нет влияния на touch»)
+- **Цикл:** r1/4
+
+## Скоуп ревью
+
+Проверялось соответствие ТЗ:
+
+- `docs/SCOPE.md` — попадание в Core user jobs (J1/J3), сохранение
+  замороженного статуса virtual devices, lock-инвариант;
+- `PROCESS.md` §2.4, §2.5 (DoR), §7.1 (обязательные разделы), §12 (запреты);
+- `AGENTS.md` — классы файлов, ветка `issue/107-virtual-light-toggle`, трейлеры;
+- `docs/LIGHT.md` и `docs/DEVICE-LIGHT-SETTINGS-MATRIX.ru.md` — канон модели
+  источника света, passive `Always`, OR-контракт controllers (#84/#88);
+- `docs/CONFIG-COMPATIBILITY.md` — образец, каким должен быть раздел
+  compatibility (old/new matrix, `decision-required`/`deprecated-read` и т.п.);
+- `docs/TOUCH-SUPPORT.md` и `docs/UX-MODES.md` — блокирующий View/kiosk-контракт,
+  разрешённые tap-действия;
+- `docs/USER-GUIDE.ru.md` — терминология «Переключить состояние», роль
+  «Всегда», «Виртуальное устройство»;
+- фактическому состоянию кода (`src/devices.ts`, `src/device-toggle.ts`,
+  `src/types.ts`) — на предмет того, что технический диагноз ТЗ (§3) не
+  является непроверенной догадкой, а описывает существующий код.
+
+## Как проверялось
+
+1. Прочитан весь тред issue #107: тело issue (черновик AC1–AC4, явное решение
+   владельца «обрабатывать как частный случай, не обобщать»), комментарий
+   аналитики Q1–Q3 с default-ответами, комментарий владельца от 14.08.2026,
+   фиксирующий Q1–Q3 без изменений (`#107#issuecomment-5287719167`), и
+   комментарий «ТЗ готово».
+2. Прочитан `docs/SCOPE.md` целиком: Core user jobs J1/J3, «Excess-functionality
+   audit» (virtual devices — «keep, frozen (no growth)»), lock-инвариант,
+   правило «никогда не удалять файл пользователя по догадке» (не касается этой
+   задачи — файлов не удаляет).
+3. Построчно сверены обязательные разделы ТЗ (§7.1 `PROCESS.md`) — таблица ниже.
+4. Прочитан `src/device-toggle.ts` целиком, в частности `resolveToggleIntent()`
+   (:590–621) и `resolveOwnEntity`/`ownRoleCandidates` (:355–419): подтверждено
+   дословно то, что описывает §3 ТЗ — виртуальный marker с `tap_action=toggle`
+   не имеет кандидатов в `ownRoleCandidates` (нет `bindingRef`/`entities`), и
+   `resolveOwnEntity` возвращает `null`, что даёт `emptyIntent(origin,
+   'no-actionable-entity')` через ветку `device.virtual || device.bindingKind
+   === 'virtual'` (:616–619) — **независимо от `is_light`**. Это важно: ТЗ
+   обязано менять поведение только для точной тройки, а не для любого
+   virtual+toggle, и код подтверждает, что сегодня Auto/Never virtual с
+   toggle-действием получают тот же `no-actionable-entity` тем же путём — AC1/
+   AC3 корректно требуют, чтобы для них ничего не менялось.
+5. Прочитан `resolvedLightSources()` (`src/devices.ts`:450–565), включая
+   создание passive-источника `marker:<id>` с `passive: !candidate.eid` и
+   `on: candidate.eid ? … : true` (:487–493) и OR-контракт контроллеров
+   (:525–532: `source.on = !control?.linked || […].some(eid => … === 'on')`).
+   Совпадает дословно с §3 п.1–2 и §6.3 ТЗ.
+6. Прочитан `lightGraphFingerprint`/`lightStateFingerprint`
+   (`src/devices.ts`:331–354) — оба фингерпринта сегодня не содержат никакого
+   поля мануального override для virtual-источника. Это подтверждает
+   техническую необходимость AC14 (новая runtime-revision обязана войти в ключ
+   кэша) — без неё один tap не вызвал бы инвалидацию `resolvedLightSources()`,
+   и Glow остался бы старым до следующего HA state tick, чего в системе для
+   virtual-маркера никогда не происходит. Идея «мутантного теста» в §15.1.5
+   (удаление revision из ключа кэша должно ронять тест) — конкретный и
+   проверяемый способ закрыть именно этот риск.
+7. Прочитан `docs/LIGHT.md` (раздел «Source, state and service identity») —
+   тройная идентичность `key`/`stateEids`/`serviceEids`, три-стейт `is_light`,
+   passive forced source, OR нескольких controllers — всё описанное в §3/§6
+   ТЗ дословно совпадает с каноном, ничего не придумано заново.
+8. Прочитан `docs/DEVICE-LIGHT-SETTINGS-MATRIX.ru.md` — матрица про
+   Live/Ручн./R (цвет, яркость, радиус) не про on/off-агрегацию; задача явно
+   не трогает geometry/color/brightness/radius (§13 «Не входит»), поэтому
+   отсутствие правки этого файла в release-артефактах (§17 ТЗ) корректно —
+   изменения принадлежат `docs/LIGHT.md`, что там и указано.
+9. Прочитан `docs/CONFIG-COMPATIBILITY.md` целиком как образец формата
+   compatibility-записи. §12 ТЗ («отдельный optional operational Store version
+   1», «отсутствие = пустой off-set/on», «operational state не входит в
+   export») по форме и содержанию соответствует принятому в проекте стилю
+   (сравнимо с разделами «Per-marker light role», «marker.controls[]»).
+10. Прочитан `docs/TOUCH-SUPPORT.md` («View is fully supported… must be
+    convenient and reliable», «Kiosk — primary supported environment») и
+    `docs/UX-MODES.md` («device tap (info / more-info / toggle per settings)» —
+    разрешённое View-взаимодействие). §10 ТЗ формулирует touch/kiosk как
+    блокирующие поверхности этими же словами, не эскалируя и не изобретая
+    более строгий контракт, чем канон.
+11. Проверена терминология `docs/USER-GUIDE.ru.md` (строки 467–470, 553–562):
+    «Переключить состояние», «Является источником света: Авто/Всегда/Никогда»,
+    «виртуальный маркер» — ТЗ использует ровно эти термины.
+12. Проверены существующие i18n-ключи `marker.toggle_none_*` и
+    `marker.toggle_effect_turn_on/off` (`src/i18n/ru.json`) — новые ключи,
+    которые требует §9 ТЗ, логически продолжают уже принятую схему именования,
+    а не вводят параллельную.
+13. Проверена запись в `docs/specs/README.md:81` — строка на #107 присутствует
+    в том же коммите, ссылка issue ↔ ТЗ двусторонняя.
+14. Прочитан `docs/types.ts` — поле `tap_action`/`tapAction` существует ровно
+    в том виде, на который ссылается §5 ТЗ («effective tap action равен
+    `toggle`»).
+15. Проверено количество и релевантность 16 AC (§14) — каждый несёт явный тип
+    доказательства (`unit`/`backend`/`smoke`/«ревью кода»/`build`) из
+    допустимого по DoR перечня; ни один AC не двусмысленен относительно того,
+    какое конкретно поведение проверяется.
+
+## Обязательные разделы (§7.1 PROCESS.md)
+
+| Раздел | Есть | Комментарий |
+|---|---|---|
+| Сценарий (персона/поверхность/момент) | ✅ | §1 |
+| Что человек увидит до/после | ✅ | §2, без терминов реализации |
+| Проблема | ⚠️ частично | Явно как отдельный заголовок отсутствует; фактически покрыта §2 («до») и §3 («подтверждённая причина текущего поведения») — см. Low-1 |
+| Скоуп / не-скоуп | ✅ | §5 (точное условие) / §13 |
+| Контракт поведения | ✅ | §5–§8 |
+| UX / i18n / accessibility / touch | ✅ | §9, §10 |
+| Модель данных и миграция | ✅ | §12 |
+| AC1…ACn с доказательством | ✅ | §14, 16 штук, каждый типизирован |
+| План автотестов | ✅ | §15 |
+| Риски | ❌ отсутствует как раздел | Риск-релевантный материал есть (fail-safe §7.5, security-граница §16, сложность 7/10 в шапке), но не сведён в один раздел с явной привязкой риск → закрывающий AC, как это сделано, например, в `SPEC-REVIEW-131-r1` — см. Low-2 |
+| Откат | ✅ | §18 |
+| Release-артефакты | ✅ | §17 |
+
+Десять из двенадцати обязательных разделов присутствуют и содержательны;
+два («Проблема», «Риски») по факту не выделены отдельным заголовком, хотя
+материал по существу распределён по документу. Дополнительно есть корректно
+обособленный §19 «Принятые технические предположения» — граница между
+продуктовыми решениями (принятыми владельцем в Q1–Q3) и свободно
+пересматриваемой техникой проведена явно, ни одна догадка не выдана за факт
+без пометки.
+
+## Находки
+
+Находок уровня **High** нет.
+
+### Low-1 — раздел «Проблема» не выделен отдельно
+
+**Файл:** `docs/specs/107-virtual-light-toggle.md` (между §2 и §3)
+
+Формально PROCESS.md §7.1 перечисляет «проблема» отдельным пунктом в списке
+обязательных разделов. В документе нет заголовка с этим названием — «до»
+(§2) и технический диагноз (§3) вместе дают эквивалентное содержание, но
+читатель, ищущий формулировку «в чём проблема» одним куском, должен собрать
+её из двух разделов и самого issue.
+
+**Почему не блокирует:** содержание есть и оно точное (см. «Как проверялось»
+п.4) — диагноз в §3 построчно совпадает с реальным кодом, не является
+догадкой. Это вопрос оформления, а не отсутствия решения.
+
+**Решение ревьюера:** Low, не блокирует, снимается с записью. На усмотрение
+автора — при следующей правке можно дать §2/§3 общий подзаголовок «Проблема»,
+либо оставить как есть.
+
+### Low-2 — нет консолидированного раздела «Риски»
+
+**Файл:** `docs/specs/107-virtual-light-toggle.md`
+
+Сложность/риск задачи оценены автором в 7/10 (шапка документа), но в отличие
+от, например, `docs/specs/131-readonly-cold-start.md` (§14 «Риски», 5 пунктов,
+каждый со ссылкой на закрывающий AC), здесь риск-релевантные утверждения
+рассеяны по документу без единого места: fail-safe при revision gap (§7.5),
+границы trust boundary (§16), решение о хранении вместо конфигурации (§4 п.1).
+Отсутствует явное перечисление, например: «риск: два отдельных HA `Store`
+могут разойтись при crash между save — закрыт §7.5 fail-safe reset + AC9»,
+«риск: canonical-кэш не инвалидируется без HA tick — закрыт AC14 + мутантный
+тест §15.1.5», «риск: manual state перекрывает controller и наоборот путает
+пользователя — закрыт §6.3 + AC5/AC12».
+
+**Почему не блокирует:** каждый реальный риск, который я смог определить при
+чтении кода и канона, оказался закрыт конкретным AC или конкретным разделом
+контракта (перечислено выше) — отсутствует сам факт непокрытого риска, не
+хватает только сведения их в один раздел для читаемости и трассируемости.
+
+**Решение ревьюера:** Low, не блокирует, снимается с записью. Рекомендация
+автору — на следующей правке (не обязательно в этом цикле) собрать
+существующие риск-утверждения в один раздел «Риски» с явной ссылкой
+риск → AC, по образцу `SPEC-REVIEW-131-r1`.
+
+## Что проверено и корректно
+
+- **Соответствие `docs/SCOPE.md`.** Задача закрывает J1 (Glow/room fill/room
+  stats — одно пространственное состояние) и J3 (очевидное безопасное
+  действие прямо с плана). Она не расширяет «замороженный» статус virtual
+  devices в общий state engine — владелец явно одобрил именно узкое
+  исключение в теле issue («не превращать в системный механизм»), и §1/§5 ТЗ
+  формулируют условие исключения как точную тройку, без обобщения на другие
+  роли/действия. Lock-инвариант не затронут: virtual target никогда не
+  резолвится в secure entity или HA service (§8.1, §16) — соответствует
+  правилу SCOPE.md «любой новый путь актуации либо отказывает locks, либо
+  добавляется явным абзацем в SCOPE.md» (здесь второе не требуется, так как
+  путь и так отказывает).
+- **Продуктовые вопросы закрыты владельцем, не додуманы автором.** Q1
+  (жизненный цикл/синхронизация), Q2 (что именно переключается — canonical
+  state, а не только Glow) и Q3 (приоритет manual state над controller links)
+  — все три явно продуктовые («что видит/делает пользователь», «что считается
+  этим же issue»), заданы одним комментарием с default-вариантами и приняты
+  владельцем дословно 14.08.2026. Открытых продуктовых вопросов в финальной
+  редакции нет — и это корректно, а не подозрительно: они были заданы и
+  закрыты на этапе аналитики, а не пропущены.
+- **Раздел §19 корректно отделяет технику от продукта.** Все 8 пунктов
+  (имя Store/класса, механизм доставки initial state, форма typed intent,
+  раскладка подписок, поведение local cache, консервативный reset вместо
+  угадывания истории, hidden vs tombstone, поведение импорта) — техническая
+  реализация уже принятых продуктовых решений Q1–Q3, ни один пункт не прячет
+  продуктовое решение под видом «технического предположения».
+- **Техническая точность диагноза (§3) подтверждена чтением кода**, не
+  является голословным утверждением автора: `resolveToggleIntent()`,
+  `resolvedLightSources()`, `lightGraphFingerprint`/`lightStateFingerprint` —
+  все три технических утверждения совпадают построчно с текущим `src/devices.ts`
+  и `src/device-toggle.ts` (см. «Как проверялось» п.4–6).
+- **Точность границы исключения.** Код подтверждает, что сегодня *любой*
+  virtual marker с `tap_action=toggle` (Auto/Never/Always) получает
+  `no-actionable-entity` одним и тем же путём (`resolveOwnEntity` → `null`);
+  §5 ТЗ и AC1/AC3 корректно требуют не менять этот путь для Auto/Never, а не
+  просто «для не-Always», что было бы более рискованной (и не запрошенной)
+  формулировкой.
+- **Canonical single-consumer contract (§6.2, AC5/AC6) методологически верный
+  ответ на риск дублирования источника истины.** Проверено, что
+  `resolvedLightSources()` — уже сегодня единственный вход для Glow, room fill,
+  room stats, preview и `houseplan-space-card» (docs/LIGHT.md, «Source, state
+  and service identity»); требование ТЗ не создавать отдельную ветку в
+  рендерере, room card, preview или static card прямо предотвращает
+  повторение уже случившегося в проекте расхождения слоёв (см. историю
+  «layered model» в docs/LIGHT.md).
+- **Кэш-инвалидация (AC14) — не декоративное требование.** Подтверждено, что
+  без явного добавления runtime-revision в `lightStateFingerprint`
+  манипуляция чисто виртуальным состоянием (без единой реальной HA entity)
+  физически не имеет другого триггера инвалидации кэша — HA state tick для
+  virtual marker никогда не придёт. План теста §15.1.5 («мутантный» тест,
+  который обязан упасть при удалении revision из ключа кэша) — конкретное и
+  проверяемое требование именно к этому риску, соответствует принятой в
+  проекте дисциплине «тест должен уметь падать».
+- **Touch/View/kiosk-контракт (§10) сформулирован дословно по канону**
+  (`docs/TOUCH-SUPPORT.md`, `docs/UX-MODES.md`), не эскалирован и не ослаблен.
+- **Терминология UX/i18n (§9) взята из `docs/USER-GUIDE.ru.md`**, не
+  изобретена; новые ключи логически продолжают существующую схему
+  `marker.toggle_none_*`/`marker.toggle_effect_*`.
+- **Compatibility (§12) не вводит миграцию config/marker/export**, что
+  соответствует и явному решению владельца (Q1: «operational data, не входит
+  в config/export») и общему принципу `docs/CONFIG-COMPATIBILITY.md` —
+  отдельный versioned Store, а не новое поле в `Marker`.
+- **Не-скоуп (§13) корректно отсекает обобщение**, прямо запрещённое
+  владельцем в теле issue: общий state engine для virtual devices, действие
+  для других ролей/actions, создание `input_boolean`/synthetic entity,
+  управление цветом/яркостью/радиусом через tap.
+- **Release-артефакты (§17) перечисляют реальные существующие файлы**
+  (`docs/CHANGELOG.md`/`.ru.md`, `docs/USER-GUIDE.ru.md`, `README.md`,
+  `docs/LIGHT.md`, `docs/ARCHITECTURE.md`, `docs/CONFIG-COMPATIBILITY.md`, три
+  bundle snapshot) — ни один не выдуман.
+- **Реестр `docs/specs/README.md`** обновлён тем же коммитом, ссылка issue ↔
+  ТЗ двусторонняя.
+- **Трейлеры коммита `af851cd`** (`Issue: #107`, `User-Visible: no` —
+  ожидаемо, так как коммит правит только ТЗ) корректны для документационного
+  коммита; продуктовый код действительно не тронут (подтверждено и текстом
+  ТЗ, и комментарием «ТЗ готово»: «продуктовый код не менялся»).
+
+## Чего не проверял
+
+- Не проверял, реализуем ли предложенный backend design (§7: общий
+  load-modify-save lock между config и operational store, атомарный WS
+  toggle) без побочных эффектов на существующие писатели конфигурации — по
+  §19 п.1/п.6 ТЗ это свободно изменяемое техническое предположение автора
+  кода и предмет код-ревью, а не ревью ТЗ.
+- Не запускал никаких автотестов, не собирал бандл и не проверял backend на
+  Python — на этапе `spec` это не требуется; все технические утверждения,
+  которые проверялись, проверены чтением существующего TypeScript-кода, не
+  исполнением.
+- Не проверял golden/скриншоты — ТЗ §15.4 явно и обоснованно откладывает их
+  до pre-beta gate (переиспользуется существующая flat-сцена, новый
+  художественный baseline не проектируется).
+- Не проверял детали Python-реализации `houseplan.virtual_lights` (имя класса,
+  формат хранения) — §19 п.1 прямо помечает точные имена как свободно
+  изменяемые технические детали.
+- Не проверял полноту 127 browser-smoke сценариев и не запускал ни одного —
+  на этапе ревью ТЗ это не требуется и не относится к гейтам код-ревью §8
+  PROCESS.md; целевой smoke-сценарий (§15.3 ТЗ) описан достаточно конкретно
+  (одна eligible лампа, один контроллер, один независимый источник, две full
+  card и одна static card) для последующей проверки на этапе код-ревью.
+- Не проверял, действительно ли предложенный «typed operational target»
+  (§8.1) может быть добавлен в `ResolvedToggleIntent` без расширения его типа
+  несовместимым образом — вопрос реализации, накрытый AC6 и предметом
+  код-ревью.
+
+## Вердикт
+
+Зелёный. High: 0, Medium: 0. Две находки Low — (1) раздел «Проблема» не
+выделен отдельным заголовком, содержание фактически распределено по §2/§3;
+(2) риск-релевантный материал не сведён в консолидированный раздел «Риски»
+со ссылками риск → AC. Обе не блокируют: содержание по существу присутствует
+и подтверждено построчной сверкой с реальным кодом и каноном подсистемы,
+открытых продуктовых вопросов нет (Q1–Q3 закрыты владельцем 14.08.2026), AC
+однозначны и типизированы, скоуп/не-скоуп точно повторяют явное решение
+владельца «не обобщать». Обе находки сняты с записью в этом документе, на
+усмотрение автора учесть при следующей правке.