From bce0ae97b2f6d34e0b5aaa575a87997f84172be4 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 31 Aug 2026 05:37:37 +0000 Subject: [PATCH] docs: review document for #126 Issue: #126 User-Visible: no --- docs/reviews/SPEC-REVIEW-126-r1.md | 193 +++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-126-r1.md diff --git a/docs/reviews/SPEC-REVIEW-126-r1.md b/docs/reviews/SPEC-REVIEW-126-r1.md new file mode 100644 index 00000000..1ae7270f --- /dev/null +++ b/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 в +той же правке, и вернуть на повторное ревью ТЗ.