mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
Ревью #682 r1, Medium: перенос добавляет документу уровень вложенности (`docs/reviews/X.md` → `legacy/reviews/<тег>/X.md`, `docs/specs/` → `legacy/specs/`), а относительные ссылки внутри перенесённых документов и в соседях, ссылавшихся на них, никто не пересчитывал — на `97d19268` 53 битые ссылки в 46 файлах (заявление «все 26 резолвятся» в `7feb6177` было верно только до переноса документов ревью). Гейты архив не смотрят. `reviews-archive.mjs`: `repairLinks` пересчитывает ссылку, если она не резолвится от нового места, а цель находится от нового или старого места через карту переносов; битая и до переноса ссылка не трогается. `--apply` делает это само, `--repair-links=<rev>` — для всех переименований `<rev>..HEAD`, `--check-links` печатает битые. Этим коммитом `--repair-links=origin/dev` переписал ровно 53 ссылки в 46 файлах; остались две прежние «...»-заглушки в CODE-REVIEW-448-r2 (битые и на dev). Тесты: перенесённый документ, сосед со ссылкой в архив, ТЗ со ссылкой на позже перенесённое ревью, битая-до-переноса не трогается, в `legacy/` битых нет; мутант `reviews-archive-links-from-new-place-only`. PROCESS §2.10 и DEVELOPMENT › Release называют переписывание и `--check-links`. Issue: #682 User-Visible: no Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qZfe7YS4rqEMKoVeS3GKd
178 lines
15 KiB
Markdown
178 lines
15 KiB
Markdown
# 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**
|