docs: review document for #126

Issue: #126
User-Visible: no

docs/reviews/SPEC-REVIEW-126-r1.md

@@ -0,0 +1,193 @@
+# SPEC-REVIEW-126-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/126
+- **ТЗ:** `docs/specs/126-ha-area-marker-relocation.md`
+- **Коммит материала:** `8bc8ba66` (ветка `issue/126-ha-area-marker-relocation`, поверх `origin/dev@6bf39ee9`)
+- **Этап:** ТЗ на ревью (PROCESS.md §2.4)
+- **Заход:** r1 (первый прогон ревью ТЗ по этому issue — прежних `SPEC-REVIEW-126-*` документов в истории репозитория нет, `git log --all` по этому пути пуст)
+- **Трек:** полный (владелец явно закрыл `small` в комментарии 2026-08-30)
+
+## Скоуп ревью
+
+Диапазон материала — только диф `docs/specs/126-ha-area-marker-relocation.md` +
+одна строка в `docs/specs/README.md` (`git diff origin/dev...HEAD --stat`).
+Продуктовый код не менялся — это подтверждено и текстом хендоффа автора, и
+самим дифом. Ревью оценивает: соответствие `docs/SCOPE.md` (J6), выполнимость
+и однозначность AC1–AC12, отсутствие догадок, выданных за факт, соответствие
+разделу §7.1 и полноту DoR-чеклиста §2.5 (это ревью — единственный гейт перед
+переводом в «Готово к разработке»).
+
+## Как проверялось
+
+| Гейт | Команда | Результат |
+|---|---|---|
+| Typecheck | `npx tsc --noEmit` | green, без вывода |
+| Unit-тесты | `npm test` | green: `tests 1668, pass 1667, fail 0, skipped 1` (пропуск — известный environment-sensitive кейс `smoke_opening_measure`, не относится к этому диффу) |
+| Build | `npm run build` | green, `dist` собран без ошибок |
+| `node scripts/check-docs.mjs` | не запускал | диф не трогает `src/**` — фингерпринт скриншотов не мог протухнуть от этого коммита |
+| `npm run invariants` / model invariants | не запускал | диф не трогает геометрию, `layout`, `marker.space`, `open_spans` — только текст ТЗ |
+| Browser smoke (`demo/smoke_*`) | не запускал | диф не содержит продуктового кода; смоки проверяют бандл, а бандл не менялся этим коммитом |
+| `golden:verify`, `performance_smoke`, `pytest tests_backend` | не запускал | не тронуты визуальные/perf/backend поверхности — диф docs-only |
+
+Зелёного Validate на SHA `8bc8ba66` не найдено (по условиям задачи), поэтому
+дешёвые гейты прогнаны вручную; все три зелёные. Тяжёлые гейты не применимы —
+объём гейта соразмерен объёму диффа (только ТЗ), как и предписано.
+
+Дополнительно я сверил ключевые технические утверждения ТЗ с реальным кодом на
+`dev@6bf39ee9` (агент `Explore`, плюс собственные `grep`/`Read`), поскольку ТЗ в
+разделе 4 заявляет несколько «выпущенных соседних контрактов» как факты — а
+такие заявления обязаны быть фактом, а не пересказом с чужих слов.
+
+## Находки
+
+### Medium — в скоупе задачи (возврат автору)
+
+**M1. §9 «Persisted provenance»: заявленный бюджет `known_devices` не существует в коде — факт, поданный как решённый, на деле является догадкой.**
+
+ТЗ утверждает: «максимум совпадает с действующим budget `known_devices` и общим
+config-size limit» (`docs/specs/126-ha-area-marker-relocation.md:198`). Проверка
+кода это не подтверждает:
+
+- `settings.known_devices` и `settings.new_device_ids` не объявлены в
+  `ServerConfig['settings']` (`src/types.ts` — там только `exclude_integrations`,
+  `group_lights`, `show_all`, `filter_seeded`, `icon_rules`); оба поля читаются
+  через `any`-каст в `src/houseplan-card.ts` (например, строки ~5071, ~5086);
+- явного per-field ограничения на длину/количество записей `known_devices` в
+  коде нет — массив растёт без отдельного капа;
+- единственный реальный числовой лимит — общий `MAX_CONFIG_BYTES = 2 * 1024 * 1024`
+  в `custom_components/houseplan/validation.py:1101`, применяемый ко всему
+  конфигу, а не к этому полю отдельно.
+
+Раздел «модель данных и миграция" — один из обязательных по §7.1, и именно в
+нём остаётся утверждение о несуществующем ограничении. Это ровно тот случай,
+про который прямо предупреждает этап ревью: «утверждение о поведении, которого
+нет ни в одном документе и которое не помечено как предположение — замечание».
+Практическое следствие: реализатор может решить, что отдельный бюджет для
+`marker_area_snapshot` уже где-то согласован и просто скопировать
+несуществующее число, вместо того чтобы принять его как самостоятельное
+техническое решение.
+
+**Как воспроизвести:** `grep -n "known_devices\|new_device_ids" src/types.ts` —
+пусто; `grep -n "MAX_CONFIG_BYTES" custom_components/houseplan/validation.py` —
+единственный найденный числовой лимит, общий для всего конфига.
+
+**Требуется:** переписать §9, убрав ссылку на несуществующий бюджет
+`known_devices`. Либо явно принять размер `marker_area_snapshot` как
+самостоятельное техническое решение (в раздел §23 «принято предположительно»,
+например «не больше активных зарегистрированных direct-биндингов, живёт по
+той же lifecycle-гигиене, что и `known_devices`»), либо прямо написать, что
+единственная защита — общий `MAX_CONFIG_BYTES`.
+
+### Low — снимаются с записью (реализатор решает сам, без возврата цикла)
+
+**L1. §8 «Eligibility»: нет формального поля, отличающего composite light group от direct entity marker.**
+
+ТЗ требует исключить «markerless auto `entity:` light group» из eligibility
+(строка 166), подразумевая чистое различение по типу привязки. По факту в
+`src/types.ts` есть только `bindingKind?: 'device' | 'entity' | 'virtual'`, и у
+light group это тоже `'entity'` (`src/devices.ts`, блок построения групп) —
+то есть на уровне типа обычная entity-маркер и light group неотличимы.
+Реальное различение в кодовой базе делается по побочным признакам (`id`
+с префиксом `lg_`, `icon === 'mdi:lightbulb-group'`, локализованная модель
+`device.light_group`).
+
+Это решаемо (признак уже используется в других местах того же файла), и это
+чисто техническая деталь — по §7.1 «всё, чего пользователь не наблюдает,
+агенты решают сами». Формально не блокирует ТЗ, но раздел §23 «принято
+предположительно» такую деталь не называет, а без неё AC5 («binding scope»)
+не самоочевиден для будущего код-ревью. **Снимается с записью:** реализация
+обязана явно задокументировать выбранный признак (например, в комментарии
+resolver-модуля или в §23 при следующей редакции), код-ревью проверит это по
+факту.
+
+**L2. §15/DoR: явного предложения «влияние на производительность — Х» нет отдельной фразой.**
+
+DoR (§2.5) требует явно называть влияние на производительность и бюджеты (или
+явное «нет»). Отдельного предложения об этом в ТЗ нет. При этом по существу
+пункт закрыт — AC11 требует bounded/no-loop поведение с performance-тестом, а
+resolver по тексту §10 запускается «после каждого authoritative registry
+rebuild», а не в рендер-цикле. Учитывая объём диффа (маленький pure-модуль) и
+явные гарантии AC11/§14 против дублирующей работы, риск для бюджета `bundle:budget`
+(256000 B gzip) и рендер-пути пренебрежимо мал. **Снимается с записью:**
+не блокирует ревью; рекомендую добавить одну явную строку в раздел рисков при
+следующей правке ТЗ ради дисциплины DoR-чеклиста, но отдельного цикла ради
+этого не открываю.
+
+## Что проверено и признано корректным
+
+- **Обязательные разделы §7.1 присутствуют полностью:** сценарий и персона
+  (§1, Home admin/View+Devices editor+hosted Static), «что человек увидит до
+  и после» одной фразой без терминов реализации (§1), проблема/диагноз (§2),
+  скоуп и не-скоуп (§5–6), контракт поведения (§7–14), UX/touch/a11y (§15),
+  модель данных и миграция (§16), i18n (§17, корректно «новых строк нет»),
+  AC1–AC12 с указанием способа доказательства (§18), план тестов (§19),
+  риски и откат (§21), release-артефакты (§22).
+- **J6 верно назван и обоснован** (`docs/SCOPE.md`): marker, оставшийся в
+  прежней комнате после переезда устройства в HA, — ровно случай «план
+  перестаёт быть правдой» из J6.
+- **Причина дефекта подтверждена кодом, а не заявлена на веру:**
+  `_livePos()` (`src/houseplan-card.ts:5174-5190`) действительно сравнивает
+  только `saved.s === d.space`, без комнаты/Area; тот же паттерн дублирован в
+  `markerPos()` (`src/space-geometry.ts:629-638`) — ТЗ верно указывает, что
+  дефект есть и в hosted Static. `diffNewDevices()` (`src/logic.ts:2092-2100`)
+  и `_syncNewDevices()` действительно сравнивают только множества id, без
+  учёта area — заявление о причине отсутствия метки подтверждено.
+- **Правило приоритета explicit override (раздел «#317») процитировано
+  кодом, а не придумано:** `src/devices.ts:1421-1424` —
+  `if (marker.area == null && marker.space && marker.room_id) return
+  roomClimateKey(...); return marker.area || null;` — это буквально то
+  трёхветочное правило, которое ТЗ формулирует в разделе 4. Отдельно
+  подтверждено `resolveExplicitMarkerPlacement()` (`src/devices.ts:1066-1086`,
+  `const area = marker.area || registryArea || ''`).
+- **Эндпойнт `houseplan/layout/delete`, на который опирается сценарий
+  переноса (§11), реален**, а не выдуман: backend
+  `custom_components/houseplan/websocket_api.py:1099-1106`, фронтенд уже
+  вызывает его в существующем коде удаления layout
+  (`src/houseplan-card.ts:5240-5243`).
+- **`DevItem.area`/`.space` действительно берутся из `device.area_id`/
+  `entity.area_id` реестра HA** в `buildDevices()`
+  (`src/devices.ts:1112-1113, 1157-1158, 1240-1241`) — заявление раздела 2 ТЗ
+  о механике перестройки корректно.
+- **Механизм очистки истории при авторитетном layout-событии другого
+  клиента, на который ссылается контракт «#74», реально существует**:
+  `_geometryHistory.clear(); this._devicePositionHistory.clear();` при
+  адопции структурных ответов и при `layoutChanged`
+  (`src/houseplan-card.ts:4215-4218`, `~5245`) — переиспользование, а не
+  придуманный факт.
+- **Механизм point-in-room для backfill (§12) опирается на существующий
+  `pointInPolygon()`** (`src/logic.ts:431`), уже используемый для аналогичных
+  проверок принадлежности точки комнате в нескольких модулях
+  (`houseplan-card.ts:7566`, `sun.ts:239,259`) — не изобретение несуществующей
+  возможности.
+- **Терминология согласована с `docs/USER-GUIDE.ru.md`:** красная метка
+  «Новое» и правило «поиск/Найти на плане её не снимают, открытие настроек —
+  снимает» (`docs/USER-GUIDE.ru.md:810-812`) совпадают с тем, что ТЗ
+  переиспользует в разделе «#29» и в §13 без выдумывания второго badge.
+- **Владельцу не задано ни одного нового вопроса** внутри ТЗ — все Q1–Q4
+  закрыты предыдущими комментариями, что и зафиксировано в разделе 3.
+- **`docs/specs/README.md` обновлён корректно** — строка со ссылкой на ТЗ
+  добавлена на своё алфавитное место.
+
+## Чего не проверял
+
+- Не проверял выполнимость самого резолвера — кода ещё нет, это спек-ревью,
+  а не код-ревью; технические имена по §23 прямо разрешено менять свободно.
+- Не прогонял golden/performance/browser smoke/backend pytest — диф их не
+  касается (см. таблицу гейтов и обоснование в каждой строке).
+- Не проверял по коду каждую строчку раздела «#29» (полный lifecycle
+  каталога устройств) — свойства hide/remove/HA-disabled ТЗ заявляет как «не
+  меняются», и это утверждение о неизменности, не о новом поведении;
+  выборочно свёл через `USER-GUIDE.ru.md`, глубокого код-аудита всего #29 не
+  делал, так как это чужой уже выпущенный контракт вне предмета этой задачи.
+- Не оценивал реалистичность оценки сложности/риска из комментариев owner
+  (7/10, 8/10) — это управленческая оценка, не предмет ревью ТЗ.
+
+## Вывод
+
+High-находок нет. Одна Medium-находка (M1) — в скоупе задачи, требует правки
+текста ТЗ (не архитектуры и не AC), поэтому отдельный issue не заводится.
+Две Low-находки сняты с записью и не блокируют. Раздел «Унаследовано из r0» не
+пишется — предыдущего раунда ревью ТЗ по этому issue не существует.
+
+**Вердикт: жёлтый.** Автору — поправить §9 (M1), при желании закрыть L1/L2 в
+той же правке, и вернуть на повторное ревью ТЗ.