diff --git a/docs/reviews/SPEC-REVIEW-274-r1.md b/docs/reviews/SPEC-REVIEW-274-r1.md new file mode 100644 index 00000000..2332185f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-274-r1.md @@ -0,0 +1,102 @@ +# SPEC-REVIEW-274-r1 + +- Issue: [#274](https://github.com/Matysh/houseplan-card/issues/274) — «Беспроводной выключатель на плане приглушён, хотя превью того же маркера обещает нейтральную подложку» +- Этап: spec (PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- ТЗ: `docs/specs/274-wireless-controller-presentation-parity.md` +- Коммит: `9b76427` (ветка `issue/274-wireless-controller-parity`, совпадает с текущим `HEAD`) +- Класс изменения этого коммита: C (документация) — `docs/specs/274-...md` + `docs/specs/README.md`, продуктовый код не тронут. + +## Скоуп ревью + +Первый заход, разбор полный (иное не требуется — предыдущего вердикта нет). Проверено: + +1. Соответствие `docs/SCOPE.md` — какую строку Core user jobs закрывает задача. +2. Обязательные разделы ТЗ по PROCESS.md §7.1 и чек-лист DoR §2.5. +3. Однозначность и проверяемость AC1…AC8, способ доказательства у каждого. +4. Достоверность технического диагноза — сверка построчных ссылок на код, приведённых в ТЗ и в issue, с фактическим `src/device-presentation.ts` и `src/houseplan-card.ts`. +5. Отсутствие догадки, выданной за факт, и корректность обращения с продуктовыми вопросами владельца (issue поднимал два вопроса; проверено, закрыты ли они уже принятым контрактом #251, а не додуманы). +6. Согласованность с каноническим предшествующим ТЗ `docs/specs/251-controller-target-availability.md`, на контракте которого строится #274. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +- Прочитано тело issue #274 и оба комментария (аналитика владельца, хендофф ТЗ на ревью). +- Прочитан сам файл ТЗ целиком (317 строк) и связанный `docs/specs/251-controller-target-availability.md` целиком. +- `git show 9b76427 --stat` и `git diff d83b0c5..9b76427 --stat` — подтверждено, что коммит трогает только `docs/specs/**`, класс A/B не задет. +- Построчная сверка диагноза: `src/device-presentation.ts:559-235,574-597,600-635`, `src/houseplan-card.ts` (`_renderDeviceSnapshot`, вызовы preview-резолва вокруг строки 20279), `src/hp-device-preview.ts:171`, `src/i18n/en.json` (`marker.preview.reason.*`). +- Проверено существование файлов, на которые ссылается план тестов: `test/devices.test.mjs`, `test/device-presentation.test.mjs`, `test/device-toggle.test.mjs`, а также уже существующий `demo/smoke_device_preview_parity.mjs` (готовая инфраструктура именно для сравнения plan/preview одного и того же маркера). + +## Гейты + +Диапазон коммита — только `docs/specs/**`, класс C, продуктовый код не менялся. `typecheck`/`test`/`build`/`check-docs`/смоки/инварианты в этом раунде **не гоняются**: ни один их предмет (src, тесты, конфигурация геометрии) в диффе не участвует, а сам этап — ревью ТЗ, а не кода. Это решение, а не пропуск: перечисляю его явно. + +## Проверка диагноза по коду (не на слово автору) + +ТЗ и issue называют точные строки текущего кода как основание диагноза. Все они подтвердились при чтении: + +- `src/device-presentation.ts:561` — `if (visual.availability === 'unavailable') return 'unavailable';` (причина для preview) — совпадает дословно. +- `src/device-presentation.ts:583` — `else if (visual.availability === 'unavailable') classes.push('unavail');` (класс для плана) — совпадает дословно. +- `src/device-presentation.ts:218-235` — комментарий и тело `controllerAvailability()` — совпадает дословно, включая формулировку «Controls are not controller evidence…». +- `resolveDevicePresentation()` (строка ~600-635) действительно вызывает `controllerAvailability(hass, d)` только для `sourceKind === 'controls'`, то есть один и тот же чистый резолвер используется и планом, и preview — значит расхождение объясняется входом (`hass`/`d.entities`), а не веткой логики. Это ключевое утверждение ТЗ (§3), и оно подтверждается. +- `src/houseplan-card.ts` — план читает `this._renderDeviceSnapshot?.presentations.get(...)` (предвычисленный, иммутабельный снэпшот), тогда как ветка preview (около строки 20279) вызывает `resolveDevicePresentation(this._planHass, previewDevice, {...})` заново, где `previewDevice` — результат `_markerPreviewDevice(d)`. Второй верхнеуровневый путь подтверждён так же, как заявляет ТЗ. + +Диагноз ТЗ не является догадкой, выданной за факт: там, где точная причина ещё не доказана (какое именно поколение registry/roster вызывает расхождение), текст прямо говорит «без живого runtime dump не доказан» и запрещает реализации выбирать ветку по догадке (§3, §14 п.1 промаркирован как предположение). Это ровно то поведение, которого требует процесс. + +## Продуктовые вопросы владельца — закрыты корректно, не додуманы + +Issue поднимал два вопроса владельцу («что видеть для кнопки без своего состояния» и «считается ли `unknown` у `event.*` признаком недоступности»). ТЗ (§4) не додумывает ответ, а показывает, что оба уже даны решением по #251: правило 5 контракта #251 («доступность по любому живому own entity state, диагностика участвует, controls — нет») отвечает на оба вопроса дословно, и это решение уже принято владельцем (не техническое, не спорное). Проверено, что #251 действительно формулирует именно эти пункты (см. `docs/specs/251-controller-target-availability.md` §4 п.1,5 и §6.1). Трактовка ТЗ верна: #274 — это регрессия/рассинхронизация уже принятого контракта, а не новое продуктовое решение, поэтому пачка вопросов владельцу здесь не нужна. + +## Проверка AC1…AC8 + +Все восемь пронумерованы, для каждого указан способ доказательства (`unit`/`smoke`/`golden`/`mutation`/`review`), как того требует DoR §2.5: + +- **AC1** — точная фикстура (event=unknown, battery=100, LQI=164, update=off, group=off) и точный ожидаемый результат (`available`, `neutral`, без `unavail`, `lqiText=164`). Однозначно, проверяемо unit-тестом. +- **AC2** — сравнение DOM плана и preview для того же сохранённого маркера без правок; для этого уже существует инфраструктура — `demo/smoke_device_preview_parity.mjs` (сравнивает `.dev` на плане и в `hp-device-preview` по классам/иконке/значению). AC не называет файл по имени, но выбор файла — техническое решение автора (§7.1), а не пробел ТЗ. +- **AC3** — матрица переходов `off → on → unavailable → off`, однозначные ожидания для каждого шага. +- **AC4** — перечислена конкретная минимальная матрица поколений (registry arrival, state tick, save/rebind, continuity hold/candidate/commit, reconnect), критерий «одно поколение — одно лицо» сформулирован проверяемо. +- **AC5** — явно требует не ослаблять негативный контракт #251 (event-only binding, all-unknown case), ссылается на существующую table-driven matrix для расширения, а не переписывания. +- **AC6** — parity View/kiosk/Static/preview в light/dark; корректно относит semantic golden к предрелизному циклу («локальный baseline не принимается»), что соответствует общему правилу PROCESS §8/§13. +- **AC7** — два конкретных, содержательных мутанта (не source-regex) с указанием, какие AC обязаны покраснеть при каждом. Это именно то, что требует «дисциплина падающего теста». +- **AC8** — гейты реализации, корректно разделяет локальный цикл и предрелизный цикл. + +Ни один AC не сформулирован расплывчато; у каждого есть точка отказа, по которой можно судить о провале. + +## Обязательные разделы (PROCESS.md §7.1) и DoR (§2.5) + +Все обязательные разделы §7.1 присутствуют по существу: сценарий/персона (§1), что человек увидит до/после (§2), проблема и подтверждённые факты (§3), контракт поведения (§4, §6), scope/не-scope (§5), UX/touch/compatibility (§7), критерии приёмки с доказательством (§8), план автотестов (§9), риски (§12), откат (§13), release-артефакты (§11). Продуктовых открытых вопросов нет, что явно зафиксировано (§4). + +Один пункт чек-листа DoR §2.5 — «перечислены затронутые файлы и модули» — не оформлен отдельным списком. Функции и файлы фактически названы по тексту (`buildDevices()`, `controllerAvailability()`, `RenderDeviceSnapshot`, `test/devices.test.mjs`, `test/device-presentation.test.mjs`, документация в §11), но сведены не в один список. Не считаю это блокирующим: сама граница диагноза (§3) сознательно не зафиксирована («без runtime dump не доказан»), и заранее заявленный список конкретных production-файлов был бы ровно той вставленной вперёд догадкой, которую процесс запрещает. Снимаю как Low с этой записью, а не превращаю в Medium — фиксировать список файлов раньше, чем найден точный источник рассинхронизации, значило бы гадать. + +## Находки + +### Low — «До»-описание в §2 использует внутренний термин вместо простой фразы (снято) + +`docs/specs/274-wireless-controller-presentation-parity.md:35` — фраза «план приглушает выключатель классом `unavail`» вставляет имя CSS-класса в раздел, который по формату должен быть «одной фразой без терминов реализации» (PROCESS.md §7.1). Не блокирует: это репродукция симптома вплотную к тексту issue, вторая часть того же предложения («показывает зелёный LQI 164, а preview обещает нейтральную подложку») уже передаёт смысл на пользовательском языке, и точное имя класса скорее помогает второй-ревьюер/автору однозначно опознать баг. Снимаю как Low с этой записью, править не обязательно. + +### Low — DoR-пункт «затронутые файлы и модули» не оформлен отдельным списком (снято) + +Разобран выше — снят с записью «список файлов сознательно не даётся до red production-path теста, это соответствует принципу „граница диагноза не додумывается“». + +High/Medium находок нет. + +## Что проверено и корректно + +- Диагноз построен на подтверждаемых номерах строк, все сверены с фактическим кодом на `HEAD` — не пересказ автора. +- Контракт #274 не переопределяет и не ослабляет уже принятый контракт #251 (явная негативная AC5 плюс явный список «не входит»). +- Продуктовые вопросы владельца из тела issue закрыты ссылкой на уже принятое решение #251, а не выданы за новую догадку. +- AC1…AC8 пронумерованы, у каждого — однозначный ожидаемый результат и способ доказательства; мутанты (AC7) содержательны. +- Компатибилити/миграция/i18n/touch/perf/security разделы присутствуют и корректно говорят «без изменений» там, где это действительно так (persisted config, backend, touch-жесты, i18n-ключи). +- Release-артефакты (§11) перечисляют оба changelog, ARCHITECTURE.md, USER-GUIDE.{md,ru.md}, TESTING.md — соответствует правилу «документация в том же коммите». +- Трассируемость issue ↔ ТЗ ↔ индекс (`docs/specs/README.md`) на месте в обе стороны. +- Коммит ТЗ несёт корректные трейлеры (`Issue: #274`, `User-Visible: no`) для чисто документационного класса C изменения. + +## Чего не проверял + +- Не гонял `typecheck`/`test`/`build`/`check-docs`/смоки/инварианты — диапазон коммита не содержит класса A/B (только `docs/specs/**`), поэтому эти гейты не относятся к предмету этого раунда ревью ТЗ. +- Не проверял глубину архитектуры `RenderDeviceSnapshot`/continuity commit barrier построчно — это территория код-ревью после того, как красный production-path тест по AC1/AC2 будет написан; на этапе ТЗ важно, что граница явно не додумана, а не то, где именно лежит фикс. +- Не связывался с владельцем — продуктовых вопросов, требующих его решения, не осталось. + +## Вердикт + +Зелёный. ТЗ технически обосновано, ссылки на код проверены и совпадают, AC однозначны и проверяемы, продуктовые вопросы закрыты ссылкой на уже принятое решение, а не догадкой. Обе Low-находки сняты с записью в этом документе.