From 2d1fca1dfa969c74f808203d71dab0c46a8124b9 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 07:31:55 +0000 Subject: [PATCH] docs: review document for #256 Issue: #256 User-Visible: no --- docs/reviews/CODE-REVIEW-256-r1.md | 171 +++++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-256-r1.md diff --git a/docs/reviews/CODE-REVIEW-256-r1.md b/docs/reviews/CODE-REVIEW-256-r1.md new file mode 100644 index 00000000..e344edff --- /dev/null +++ b/docs/reviews/CODE-REVIEW-256-r1.md @@ -0,0 +1,171 @@ +# CODE-REVIEW — issue #256 — заход r1 + +**Скоуп задачи:** `houseplan/config/get` и `houseplan/layout/get` получают +необязательные параметры проекции (`space_id`, `fields`, `marker_fields` / +`space_id`), не меняя ответ по умолчанию. Лёгкий трек (`small`), ТЗ — в теле +issue. Заход первый: `git log --oneline origin/dev..HEAD` даёт один коммит +(`a952f5f`), это полный разбор, дельты по предыдущим раундам нет. + +Диапазон материала: `git diff origin/dev...HEAD` — 3 файла, +269/−5: +`custom_components/houseplan/projection.py` (новый, 95 строк), правки +`custom_components/houseplan/websocket_api.py` (+41/−5), `tests_backend/ +test_projection.py` (новый, 138 строк, 11 тестов). + +## Как проверялось + +Прочитаны в порядке из инструкции: `docs/SCOPE.md`, `AGENTS.md`, тело issue +#256 и оба комментария (спека и отчёт автора), `PROCESS.md` §2.7/§2.10/§4. +Изменение не трогает видимое поведение (`User-Visible: no`, подтверждено +диффом — нет правок в `docs/CHANGELOG*`), поэтому `docs/USER-GUIDE.ru.md` и +канонические документы подсистем не задействованы: меняется только +серверный WS-протокол для диагностических клиентов, UI не тронут. + +### Гейты — что прогнал сам и с каким результатом + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | чисто, exit 0 | +| JS-тесты | `npm test` | 1136/1136, 0 fail | +| Сборка + сверка бандла | `npm run build`, затем `diff dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | сборка ок; diff пустой (файлы идентичны) — ожидаемо, диффа в `src/**` нет | +| Backend pure-тесты (новые) | `python3 -m pytest tests_backend/test_projection.py -v` (после `pip3 install --user pytest pytest-asyncio`, песочница не имела pytest) | **11/11 passed** | +| Backend pure-подмножество целиком | `python3 -m pytest tests_backend -q --ignore=tests_backend/test_coordinate_canonicalization.py --ignore=tests_backend/test_validation.py` | **51 passed** — совпадает с числом, заявленным автором | +| Дисциплина «тест умеет падать» | вручную сломал `keep = {"id", *names}` → `{*names}` в копии `projection.py`, перезапустил `test_projection.py`: **4 теста упали** (потеря `id`), включая `test_marker_fields_keep_id_even_when_not_asked` и `test_markers_survive_unexpected_entries`. Отдельно занулил условие раннего возврата (`if False: return config`) — упали **2 теста инварианта идентичности** (`test_no_parameters_change_nothing`, `test_empty_or_malformed_lists_mean_no_projection`). Оба раза файл восстановлен из бэкапа, `git status` чист | тесты не ватные | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | — | «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут). Browser-smoke этим диффом не выбираются — выбирать нечего.» | + +Полный вывод `git status --short` после всех экспериментов — пусто, рабочее +дерево осталось чистым. + +### Что не прогонял и почему + +- **`node scripts/check-docs.mjs`** — не прогонял. Условие запуска — diff + трогает `src/**`; этот diff `src/**` не трогает (подтверждено и + `git diff --stat`, и выводом `smoke-select.mjs`). Отпечаток документации + считается по `src/**`, изменение в него не входит. +- **`npm run invariants -- --config <...>`** — не прогонял. Diff действительно + упоминает `layout` и `marker.space`, но только как *ключи фильтрации при + чтении* — ни один путь записи (`config/set`, `layout/set`, `plan/optimize`) + не тронут, хранимый документ проекция не мутирует (проверено чтением и + тестами `test_no_parameters_change_nothing`, `is CONFIG`/`is LAYOUT`). + Инварианты модели (#254) — про целостность *хранимых* данных; здесь модель + не меняется, поэтому гейт неприменим, а не пропущен. +- **`golden:verify`, browser-смоки** — не прогонял; рендер, геометрия и стили + не затронуты, `smoke-select.mjs` подтверждает: выбирать нечего. +- **HA-харнесс (`test_ha_websocket.py` и соседние)** — не прогонял. В + песочнице нет `homeassistant` (тот же пробел независимо воспроизведён на + `origin/dev` — `test_coordinate_canonicalization.py`/`test_validation.py` + падают с `ModuleNotFoundError` и там, это не регрессия задачи). Проводка + двух обработчиков (`ws_config_get`, `ws_layout_get`) разобрана **чтением, не + исполнением** — см. AC2/AC4 ниже; фактический прогон харнесса — за CI. +- **`hassfest`/HACS** — манифест не менялся, гейт неприменим. +- **Perf-профили** — не названы в AC, путь не относится к чувствительным к + перфу (проекция — плоская фильтрация словаря/списка на чтении, O(n) от + размера уже загруженного документа). +- **Одно число — один источник** — неприменимо: изменение не добавляет и не + меняет пользовательскую величину, это диагностический протокол, не UI. + +## AC — доказательства + +1. **`config/get` без параметров — ответ прежний.** Доказано тестом + `test_no_parameters_change_nothing` (проверяет тождество объекта `is + CONFIG`, не только равенство) и чтением кода: `ws_config_get` строит + `config` ровно как раньше, вызывает `project_config(config, space_id=None, + fields=None, marker_fields=None)`, а та при всех трёх `None` возвращает + входной объект без копии (`projection.py:56-58`). Тест умеет падать — + проверено (см. таблицу гейтов). +2. **`space_id` возвращает одно пространство, `markers`/`settings` не + урезаны.** Доказано `test_space_id_narrows_spaces_only` и + `test_unknown_space_returns_empty_list_not_an_error`. Плюс чтением: + `project_config` трогает только ключ `spaces`, остальные ключи копируются + как есть (`dict(config)`). +3. **`marker_fields: ["binding","space"]` оставляет `id`+два поля.** Доказано + `test_marker_fields_keep_id_even_when_not_asked` (сверка точного словаря) и + разрушающим экспериментом выше (тест ловит потерю `id`). +4. **`layout/get` с `space_id` — только позиции этого пространства.** + Доказано `test_layout_space_filter`. Проводка в `ws_layout_get` — + **проверено чтением, не исполнением**: `project_layout(data.get("layout", + {}), space_id=msg.get("space_id"))` подставлена на место прежнего + `data.get("layout", {})`, `rev`/`can_optimize_undo`/`undo_kind` считаются + из `config_data`/`data`, которые проекции не касаются — инвариант «флаги + считаются до проекции» не может быть задет этой правкой хотя бы потому, что + строки, их вычисляющие, физически не изменены (diff это подтверждает). + +Инвариант 2 из ТЗ («`rev`, `can_write`, `virtual_lights`, `can_optimize_undo`, +`undo_kind` не зависят от проекции») — проверено чтением: в `ws_config_get` +`virtual_lights` считается **до** вызова `project_config`, из непроецированного +`config`; `can_write`/`can_optimize_undo`/`undo_kind` используют `data`/ +`config_data`, не `config`. Порядок операций в диффе это гарантирует +структурно, не только по намерению. + +Инвариант 3 («проекция только для чтения») — проверено чтением: ни один из +трёх обработчиков записи (`config/set`, `layout/set`, `plan/optimize`) не +импортирует и не вызывает `projection.py` — `grep` по diff и по текущему +`websocket_api.py` подтверждает единственную точку импорта. + +Инвариант 4 (неизвестное имя поля — не ошибка; неизвестный `space_id` — пустой +список) — доказано `test_unknown_space_returns_empty_list_not_an_error`, +`test_unknown_marker_field_adds_nothing`. + +## Разобрано и корректно + +- Проекция не мутирует исходный документ: `project_config` делает `dict(config)` + (мелкое копирование) и никогда не пишет в `config[...]` до этого; `spaces`/ + `markers` заменяются новыми списками/словарями, а не редактируются на месте. + Подтверждено и тестами (`CONFIG == original` после вызова), и разрушающей + проверкой не потребовалось — код тривиально проверяем чтением. +- Порядок применения фильтров (`space_id` → `marker_fields` → `fields`) + корректен даже в комбинации «`fields` не включает `markers`, но + `marker_fields` задан»: `marker_fields` спроецирует `markers` до того, как + `fields` вырежет сам ключ `markers` — лишняя работа, не баг. +- Решение «пустой список = отсутствие проекции, а не проекция в ноль полей» + автор явно пометил как оспоримое. Он не наблюдаем ни одной персоной + (доступен только диагностическим клиентам, не описан в AC) — это техническое + решение в духе PROCESS.md §7 («решай и записывай»), а не догадка, выданная + за продуктовый факт. Принимаю: альтернатива (пустой список = «убрать все + поля») не более очевидна и нигде не запрошена. +- `id` всегда добавляется в `marker_fields` — соответствует AC3 буквально и + сопровождается тестом и комментарием, объясняющим «почему». +- Схемы `vol.Optional`/`vol.All(...vol.Length(...))` на `space_id` + (1–200 символов), `fields`/`marker_fields` (список строк 1–100 символов, + максимум 50 элементов) — разумные защитные пределы, симметричные между + `config/get` и `layout/get`. +- Загрузка модуля в тестах по пути (`importlib.util.spec_from_file_location`), + а не через пакет — повторяет приём из `test_virtual_lights.py` и обосновано + тем же #135; сам `projection.py` не импортирует ничего, кроме `typing`, что + и позволяет тесту работать без `homeassistant`. Подтверждено эмпирически: + `test_projection.py` собрался и прошёл в песочнице без HA/voluptuous, тогда + как соседние файлы, тянущие пакет, — нет. +- Трейлеры коммита `a952f5f`: `Issue: #256`, `User-Visible: no` — корректно, + изменение не даёт пользователю ничего нового, changelog обоснованно не + тронут. +- Продуктовая рамка не нарушена: `docs/SCOPE.md` не содержит прямого пункта + под диагностические WS-параметры, но задача явно заведена самим ревью + (аудит инфраструктуры 2026-08-23) и принята владельцем на уровне issue — + скоуп решён на входе, пересматривать на этапе кода нечего. + +## Находки + +Нет находок High или Medium. Ниже — то, что рассмотрено и сознательно не +поднято до находки: + +- **Low, не блокирует, не правлю:** порядок `space_id`→`marker_fields`→ + `fields` в `project_config` иногда строит `markers`-проекцию, которую тут же + выбрасывает `fields`-фильтр (см. выше). Чисто эффективность на маленьких + документах (максимум пара сотен маркеров), не корректность. Не стоит цикла + ревью. +- Рассмотрел как потенциальную Medium-находку: решение «пустой список = + нет проекции» могло быть продуктовым вопросом («хочу узнать только `rev`, + без конфигурации вообще»), а не техническим. Отклоняю как находку: ни один + реальный клиент (карточка, диагностика) не отправляет пустой список сегодня, + случай гипотетический, вариант автора не хуже альтернативы, и не наблюдаем + ни одной из трёх персон продукта — чисто внутренний контракт диагностического + API. Не эскалирую владельцу: это не вопрос «что видит пользователь». + +## Вердикт + +Все 4 AC доказаны автотестами, тесты проверены на способность падать. Оба +инварианта совместимости (байт-в-байт без параметров; `rev`/флаги считаются до +проекции) проверены и чтением, и тестами. Запись не затронута. Гейты, +соразмерные объёму задачи (typecheck, JS-тесты, build+bundle-сверка, pure +backend-тесты), зелёные; неприменимые гейты названы с обоснованием. + +**Зелёный.**