From da10b3df3a0742983c3ef2ebfe71d66f44e6c0ec Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:01:37 +0000 Subject: [PATCH] docs: review document for #174 Issue: #174 User-Visible: no --- docs/reviews/SPEC-REVIEW-174-r1.md | 177 +++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-174-r1.md diff --git a/docs/reviews/SPEC-REVIEW-174-r1.md b/docs/reviews/SPEC-REVIEW-174-r1.md new file mode 100644 index 00000000..4d5ea9e1 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-174-r1.md @@ -0,0 +1,177 @@ +# SPEC-REVIEW-174-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/174 +- **ТЗ:** [`docs/specs/174-linked-virtual-light-controller.md`](../specs/174-linked-virtual-light-controller.md) + (commit `4a8958291d28fbd86d0233e6ac9208b068b24f76`, ветка `issue/174-linked-virtual-light`) +- **Ревьюер:** Claude (ревью ТЗ ≠ автор), этап `S4-spec-review` +- **Цикл:** r1/4 (обычный трек — issue не `small`/`trivial`, что подтверждено + комментарием аналитики) + +## Скоуп ревью + +Ревью ТЗ #174: изменение authority-правила пассивного forced-источника +«Всегда» + виртуальная лампа при наличии входящей связи от контроллера +(умного выключателя/реле), плюс переадресация клика по такой лампе на реальные +driver-сущности вместо operational `virtual_lights` toggle (#107). + +Не в скоупе ревью: код ещё не написан (issue в `S4-spec-review`), поэтому +проверка кода/тестов — предмет будущего code-review. + +## Как проверялось + +Прочитано в заданном порядке: + +1. `docs/SCOPE.md` — job'ы J1/J3, инвариант замка (не затронут: driver-сущности + ограничены `light.*`/`switch.*`). +2. `AGENTS.md`, `PROCESS.md` (полностью, включая §7.1 и §5). +3. Issue #174 body + все 3 комментария (владелец подтвердил семантику + 18.08.2026, аналитика оценила трек, автор сдал ТЗ). +4. `docs/USER-GUIDE.ru.md` — раздел «"Тупая" лампа с умным выключателем» + (строки 559–589), сверены термины «Всегда», «Переключить состояние», + «Управляет другими источниками света», Glow. +5. Канонические документы подсистемы: `docs/LIGHT.md` (раздел «Source, state + and service identity»), `docs/specs/084-passive-forced-light-sources.md`, + `docs/specs/107-virtual-light-toggle.md`, + `docs/DEVICE-LIGHT-SETTINGS-MATRIX.ru.md`. +6. Само ТЗ `docs/specs/174-linked-virtual-light-controller.md` целиком. +7. Текущий код на ветке `issue/174-linked-virtual-light` (тот же коммит, где + лежит ТЗ; продуктовый код не менялся) — `src/devices.ts` + (`resolvedLightSources`, строки 484–608, включая безусловный override на + 567–572), `src/device-toggle.ts` (`resolveToggleIntent`, 604–659; + `resolveControls`, 490–601; `sameToggleOperationTargets`, 686–698), + `src/virtual-light-state.ts` — чтобы убедиться, что цитаты кода и + утверждения о причине бага в ТЗ и в issue соответствуют реальному коду, а не + пересказу автора. Совпадение построчно точное (см. «Проверено и корректно»). +8. Существование упомянутых тестовых/смоук-файлов: + `test/devices.test.mjs`, `test/device-toggle.test.mjs`, + `test/device-presentation.test.mjs`, `demo/smoke_virtual_light_toggle.mjs`, + `demo/smoke_controls.mjs` — все существуют, ТЗ не ссылается на + несуществующие файлы кроме нового `demo/smoke_linked_virtual_light.mjs`, + что явно помечено как новый файл. + +Гейты (typecheck/test/build) не прогонялись — на этапе ревью ТЗ кода нет, +прогон гейтов здесь не предмет доказательства (это часть code-review, +PROCESS.md §2.7). + +## Проверка формальных требований §7.1 + +Все обязательные разделы присутствуют: сценарий и персона (§1), что человек +увидит до/после одной фразой без терминов реализации (§2 — использует «Glow» и +«HA state», но это установленный интерфейсный термин из +`docs/USER-GUIDE.ru.md`/`docs/LIGHT.md`, а не изобретённый жаргон), проблема с +подтверждённой причиной (§3, код процитирован точно), скоуп/не-скоуп (§6/§7), +контракт состояния и действия (§8/§9), единые визуальные consumers (§10), +модель данных и совместимость (§11), UX/i18n/touch (§12), AC1–AC14 с указанием +доказательства (§13), план автотестов (§14), риски/perf/security (§15), +откат (§16), release-артефакты (§17), явный блок принятых технических +предположений (§18). + +Каждый AC пронумерован, имеет ровно один заявленный способ доказательства +(`unit`/`smoke`/`ревью кода`/`build`) и формулировку, допускающую одну +интерпретацию — за одним отмеченным ниже исключением (Low-1). + +Продуктовых вопросов владельцу не осталось: единственное решение владельца, +требовавшееся для этой задачи (семантика «два умных устройства»), уже принято +в комментарии от 18.08.2026 и корректно перенесено в §4 ТЗ. Автор не подменил +догадкой ни одного продуктового пункта — там, где формат ответа мог требовать +нового текста (driver-group hint), ТЗ явно откладывает решение как «отдельная +продуктовая находка», а не придумывает текст (§12). + +## Находки + +### Low-1 — формулировка AC6 читается двусмысленно в отрыве от §9.1 + +**Файл:** `docs/specs/174-linked-virtual-light-controller.md:309-310` + +AC6 гласит: «клик по controller сохраняет #84/#94 group semantics и +переключает тот же driver projection, которым вычисляется linked source». +При нескольких контроллерах на одну лампу эта фраза, прочитанная изолированно, +может означать «клик по одному контроллеру должен переключить объединение +driver-сущностей ВСЕХ контроллеров лампы» — что было бы неверно и опасно +(один выключатель дистанционно переключил бы другой). По коду +(`resolveControls`, `devices.ts:490-601`) и по явной строке §9.1 «в простой +паре команда содержит реальный relay controller» видно, что имелось в виду +другое: клик по контроллеру продолжает переключать только *свою* сущность, +а утверждение AC6 — про переиспользование одного и того же вычислителя +driver-проекции (не про объединение по всем входящим связям цели). Строка +риска «Source click при нескольких controllers переключит только один relay» → +«Детерминированный union/dedupe и group tests» (§15) относится к клику по +**лампе** (AC4), а не по контроллеру, и путаницу создаёт именно совпадение +формулировок. + +**Воспроизведение неоднозначности:** контроллер A и контроллер B оба ссылаются +на одну и ту же passive-лампу L (валидная конфигурация по #84 §5.2, OR). +Буквальное прочтение AC6 «тот же driver projection, которым вычисляется +linked source» для L равно `{A.entity, B.entity}` — так что тест AC6 в +такой формулировке можно было бы написать так, что клик по A потребует +службы к B, чего продукт не хочет и чего не просит владелец. + +**Вердикт:** не блокирует — контекст §9.1 и таблица рисков снимают +неоднозначность, а код (`resolveControls`) уже реализует корректную, +per-controller семантику, которую AC6 обязан только сохранить. Снимаю без +правки ТЗ: автор код-ревью обязан читать AC6 вместе с §9.1 буквально «свой +relay, не объединение», и я фиксирую это здесь как явное толкование для +будущего код-ревью, а не как повод для нового цикла. + +Больше High/Medium-находок нет. + +## Что проверено и корректно + +- **Причина бага подтверждена исполнением, а не пересказом.** Цитаты кода из + issue и ТЗ (`source.on = !control?.linked || ...`; `isManualVirtualLightMarker` + override в `resolvedLightSources`, и ранняя проверка в `resolveToggleIntent`) + совпадают построчно с текущим `dev`-кодом на ветке. Ни одно техническое + утверждение о причине не является догадкой. +- **Продуктовое решение владельца корректно перенесено.** Формулировка §4 ТЗ + («два умных устройства», HA state — единственный источник истины, + lifecycle-граница create/remove link) слово в слово соответствует + комментарию владельца и последующей аналитике. +- **Контракт §8/§9 согласован с уже принятыми канонами #84/#107**, а не + придумывает новую модель: OR нескольких контроллеров, zero-driver → dormant + (не constant-on), lossless `controls`, запрет `marker:*` в `callService` — + всё это уже нормативно закреплено в #84 и просто применяется к + ранее заблокированному случаю. Новизна ТЗ ограничена ровно точкой конфликта + (#84 OR vs #107 manual override) и симметричной переадресацией клика лампы — + это соответствует заявленному скоуфу и не расширяет его. +- **Не входит в задачу (§7)** корректно исключает автосвязывание, новые + UI-поля, AND/NOT-логику, синхронизацию цвета/яркости и историю — ничего из + этого не запрашивал владелец. +- **Compatibility (§11)** — не меняются `Marker`/`ServerConfig`/backend schema, + что подтверждается кодом: единственная правка — в вычислении `source.on` и в + выборе operation, оба уже существующих pure-функциях без сериализации. +- **AC изложены с однозначным способом доказательства** и практически все — + с единственной трактовкой; мутационные требования присутствуют явно + (AC1, AC7) как и требует #85/PROCESS §2.7 «тест умеет падать». +- **Release-артефакты (§17)** включают оба changelog, `USER-GUIDE.ru.md`, + `LIGHT.md`, `DEVICE-LIGHT-SETTINGS-MATRIX.ru.md`, supersession-заметку в + `107-virtual-light-toggle.md` — то есть именно те документы, чьи текущие + формулировки (`USER-GUIDE.ru.md:583-589`, `LIGHT.md:149-159`) сейчас описывают + старое (заменяемое) поведение и стали бы враньём без обновления. +- **Откат (§16)** не требует миграции данных — корректно, так как формат + конфигурации не меняется. +- **Технические предположения (§18)** отделены от продуктовых решений и явно + помечены как «можно менять свободно», ревьюеру не выдано ни одной догадки + под видом факта. + +## Чего не проверял + +- Реализацию — её нет: issue находится в `S4-spec-review`, продуктовый код не + менялся с предыдущего merged-состояния (`git diff origin/dev...HEAD` для + `src/**` пуст). +- Гейты `typecheck`/`test`/`build`/browser smoke — не относятся к этапу + ревью ТЗ; будут предметом code-review (PROCESS.md §2.7/§8). +- `custom_components/houseplan/**/*.py` — ТЗ и код подтверждают отсутствие + backend-изменений, поэтому backend-тесты не запускались. +- Golden/perf-эталоны — ТЗ явно заявляет отсутствие нового visual baseline; + не перепроверял рендер вручную, так как речь о будущей реализации. + +## Вердикт + +Все обязательные разделы §7.1 присутствуют, каждый AC однозначен и имеет +названный способ доказательства, продуктовых открытых вопросов не осталось +(решение владельца зафиксировано и корректно перенесено), догадок под видом +факта не найдено. Единственная находка — Low, снята без цикла возврата, с +явной запиской для код-ревью. + +**Вердикт: зелёный · цикл r1/4 · High: 0 · Medium: 0 → нет · Документ: +docs/reviews/SPEC-REVIEW-174-r1.md**