From b7b28ee57923465ebbf2e538348f02691e36ff0d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:02:41 +0000 Subject: [PATCH] docs: review document for #107 Issue: #107 User-Visible: no --- docs/reviews/SPEC-REVIEW-107-r1.md | 294 +++++++++++++++++++++++++++++ 1 file changed, 294 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-107-r1.md diff --git a/docs/reviews/SPEC-REVIEW-107-r1.md b/docs/reviews/SPEC-REVIEW-107-r1.md new file mode 100644 index 00000000..f89ec7cc --- /dev/null +++ b/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:` с `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 +однозначны и типизированы, скоуп/не-скоуп точно повторяют явное решение +владельца «не обобщать». Обе находки сняты с записью в этом документе, на +усмотрение автора учесть при следующей правке.