Files
houseplan-card/docs/reviews/SPEC-REVIEW-490-r1.md
2026-09-09 00:10:16 +03:00

12 KiB
Raw Permalink Blame History

SPEC-REVIEW-490-r1

  • Issue: #490 — «Атомарный recovery и live-состояние сводной панели»
  • Этап: ТЗ на ревью (S4-spec-review), PROCESS.md §2.4
  • Заход: r1 · блокирующих циклов израсходовано 0 из 4
  • Артефакт ТЗ: docs/specs/490-summary-recovery-live-state.md, коммит 95a34f730a572c06c732e1e029b2c8dc719428b9, ветка issue/490-summary-recovery-live-state
  • Трек: полный (не small) — обоснование в S2-комментарии проверено и корректно: больше одной поверхности (save-контракт панели + render dependency projection в houseplan-card.ts), multi-client concurrency/state contract, влияние на высокочастотный render scheduling

Скоуп

Issue заведён по прямой команде владельца из независимого аудита (2026-09-08, §11 п.1/12) по уже опубликованному в dev (но не зарелиженному) #437. Два подтверждённых дефекта:

  • F1 — saveDialog() в lost-ACK ветке принимает только rev из повторного config/get, но затем безусловно перезаписывает _serverCfg старым candidate, поэтому параллельная правка другого клиента к несвязанному полю документа теряется при следующей записи;
  • F2 — резолвер зависимостей рендера (_captureRenderDeviceSnapshot) не включает entity-источники summary_panel.blocks[].values[], поэтому summary-only датчик не обновляется на обычном HA-tick.

Это соответствует docs/SCOPE.md J1 (live-обзор), J2, J6 (multi-client sync, optimistic locking) — ТЗ не расширяет функциональность #437, а чинит регресс уже принятого контракта.

Как проверялось

Ревью читал ТЗ и сверял каждое заявленное поведение с:

  • телом issue #490 и обоими комментариями (аналитика S2, готовность ТЗ S3);
  • исходным ТЗ #437 (docs/specs/437-summary-panel.md, §7.1, §7.4, §9.1, §9.2) — контракт, который #490 исправляет, а не переопределяет;
  • фактическим кодом на SHA 95a34f73 (ветка задачи): src/summary-panel-runtime-loaded.ts (saveDialog()), src/houseplan-card.ts (_captureRenderDeviceSnapshot, _cfgEpoch/willUpdate, _setAreaLifecycleConfig), custom_components/houseplan/validation.py (SUMMARY_PANEL_SCHEMA — max=10 блоков × max=20 значений);
  • существующими тестами, которые ТЗ обещает расширить: test/summary-panel.test.mjs, test/render-device-snapshot.test.mjs, test/render-invalidation.test.mjs, demo/smoke_summary_panel.mjs — все существуют, план тестирования не ссылается на несуществующие файлы.

Продуктовый код не менялся и не оценивался на корректность реализации — это код-ревью следующего этапа. Проверялось только то, что ТЗ описывает проверяемый и технически достижимый контракт.

Проверка root cause (F1)

saveDialog() (src/summary-panel-runtime-loaded.ts, ветка catch (writeError)): при совпадении saved/draft принимает this.host._cfgRev = authoritative.rev, но this.host._serverCfg = candidate (старый локальный кандидат) стоит вне catch-блока и выполняется безусловно после любого исхода try. Соответствует описанию §3 п.1 ТЗ дословно.

Проверка root cause (F2)

entityIds в _captureRenderDeviceSnapshot собирается из room.settings.temp_source/hum_source, openingEntityReferences, decor-текстов и _vacFit.source — обхода settings.summary_panel нет нигде в файле (grep summary_panel src/houseplan-card.ts — 0 совпадений). Соответствует §3 п.2 ТЗ.

Проверка «200 values» (§14, принятое предположение №2)

Не голословно: backend-схема SUMMARY_PANEL_SCHEMA ограничивает blocks vol.Length(max=10), values внутри блока — vol.Length(max=20), итог 10×20=200 — число в ТЗ точное, не придуманное.

Проверка терминологии AC5 (identity/epoch)

_cfgEpoch — реальный существующий механизм (houseplan-card.ts, willUpdate, комментарий DEV-B701-01), уже используемый именно как сигнал «геометрию нужно пересчитать» отдельно от _cfgRev (concurrency-номер). Существует прецедент атомарной подмены _serverCfg без бампа эпохи (_setAreaLifecycleConfig) — доказывает, что контракт §6.2 «принять весь authoritative config через общий adoption seam» и §7.2 «relevant tick не меняет geometry epoch» технически реализуем в существующей архитектуре, а не выдуман.

Проверка словаря AC5 (relevant/irrelevant tick)

test/render-invalidation.test.mjs уже использует ровно эти термины (an unrelated HA state row does not invalidate..., a dependency row identity or presence change invalidates state...) — ТЗ говорит на языке существующего теста, план тестирования реалистичен.

Находки

Нет High. Нет Medium (ни в скоупе, ни вне скоупа). Нет Low.

Что проверено и корректно

  • Обязательные разделы §7.1 присутствуют по существу: сценарий (§1), до/после (§2), проблема как «подтверждённые причины» (§3), скоуп (§4), не-скоуп (§5), контракт поведения (§6–7), совместимость/данные/i18n (§8, явно «миграции нет»), AC1–AC6 с доказательством (§9), план автотестов (§10), риски (§11), откат (§12), release-артефакты (§13).
  • Каждый AC — проверяемое утверждение с указанным способом доказательства (unit/integration/smoke), не общая фраза; AC3 и негативная матрица явно отделяют «настоящий конфликт» от «потерянного ACK», что закрывает главный риск регрессии #102-типа (правка по одному AC не должна ломать соседний).
  • Не-скоуп (§5) корректно исключает #493 (picker/scale/mobile/lifecycle) и не переопределяет backend/API/схему — F1/F2 действительно локальны к save-контракту и render-dependency-проекции.
  • Раздел «Принятые предположения» (§14) размечен явно и корректно: все четыре пункта — технические решения (детектирование lost ACK, bounded dependency set, семантика unavailable/missing, отсутствие нового UX-контракта), ни один не маскирует продуктовый вопрос под факт. Проверка по существу (см. «200 values» выше) показала, что предположения не являются догадками, а выводимы из уже существующего кода/схемы.
  • Продуктовых вопросов владельцу нет и не должно быть: ожидаемое поведение уже зафиксировано контрактом #437 (§7.1, §7.4, §9.1) — задача восстанавливает, а не придумывает поведение. Никакого технического вопроса, ошибочно адресованного владельцу, не найдено.
  • Откат (§12) специфичен для этой задачи: явно запрещает частичный rollback одной из двух половин фикса, если тесты по-прежнему обещают общий контракт — не шаблонная фраза.
  • Release-артефакты (§13) корректно называют «golden не требуется» с обоснованием (нет визуальных изменений) вместо молчаливого пропуска.

Чего не проверял

  • Не проверялась реализация — код по этой задаче ещё не написан (issue в S4-spec-review, не S6/S7). Проверка «работает ли реализация» — предмет код-ревью следующего этапа.
  • Не запускались гейты (typecheck/test/build) — на этапе ТЗ они не относятся к предмету ревью (нет изменений в src/**/test/** этой задачи; коммит 95a34f73 — только docs/specs/**, класс C).
  • Не проверялась производительность существующего render-invalidation пути вне контекста этой задачи — вне скоупа.

Материал раунда

  • SHA: 95a34f730a572c06c732e1e029b2c8dc719428b9 (ветка issue/490-summary-recovery-live-state)
  • ТЗ: docs/specs/490-summary-recovery-live-state.md на этом SHA

Вердикт

Зелёный. ТЗ технически обоснованно, каждое заявленное расхождение с контрактом #437 подтверждено чтением кода на указанных строках, все AC проверяемы и снабжены способом доказательства, план тестов ссылается на существующие файлы, предположения размечены и не подменяют продуктовые решения. High: 0, Medium: 0.


Материал раунда

  • Ветка: issue/490-summary-recovery-live-state, коммит 95a34f730a57 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: 17238622e9af7346270d88a5100e8b6260789ca6
    git log --all --format='%H %T' | grep 17238622e9af
    
  • ТЗ docs/specs/490-summary-recovery-live-state.md, блоб d4bfd4ea63f1b2ce1cb255b2764fe14d25661df7
    git log --all --find-object=d4bfd4ea63f1b2ce1cb255b2764fe14d25661df7 -- docs/specs/490-summary-recovery-live-state.md