From 783cc68b91aacb72fab1eae7064b9f2a8b01a86c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 05:54:11 +0000 Subject: [PATCH] docs: review document for #333 Issue: #333 User-Visible: no --- docs/reviews/CODE-REVIEW-333-r1.md | 222 +++++++++++++++++++++++++++++ 1 file changed, 222 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-333-r1.md diff --git a/docs/reviews/CODE-REVIEW-333-r1.md b/docs/reviews/CODE-REVIEW-333-r1.md new file mode 100644 index 00000000..98659969 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-333-r1.md @@ -0,0 +1,222 @@ +# CODE-REVIEW-333-r1 + +Issue: #333 · Заход: r1 · Блокирующих циклов израсходовано 0 из 2 +Материал: `git log --oneline origin/dev..HEAD` = один коммит `c29df29b` +(`fix: plan/optimize passes the junction gate; import stays free by design (#333)`), +родитель `73406225` (= `origin/dev` на момент проверки). + +## 1. Скоуп + +ТЗ (тело issue, small-трек, ревизия 2, оба раунда spec-ревью зелёные — +`docs/reviews/SPEC-REVIEW-333-r1.md`, `-r2.md`) требует: + +1. `ws_plan_optimize` вызывает `validate_junction_limits(candidate, stored, …)` + внутри существующей executor-функции `_validate_optimize_cpu`, ПОСЛЕ + миграции кандидата — наследование по правилу (легаси-нарушение проходит, + новое — отказ `junction_limit_`). +2. Успешный optimize обновляет `rt.junction_baseline` счётчиками кандидата. +3. Import/restore остаются вне гейта; докстринг `junction_limits.py` и + `docs/specs/329-junction-limits.md` §5 фиксируют периметр честно. + +Продуктовая рамка (`docs/SCOPE.md`): изменение защищает контракт «карточка +отказывает записи, ломающей П1–П5» (Core user jobs: геометрия остаётся +валидной) от обхода через второй командный путь — попадает в скоуп. +`User-Visible: no` в трейлере коммита корректен: поведение честного клиента +не меняется (AC10 #329 уже доказывает, что оптимизация не добавляет +нарушений), меняется только реакция на крафтовый payload. + +## 2. Как проверялось + +Дешёвые гейты, на которые ссылался бриф ревью («Validate на `c29df29b` +завершился success»), **не могут считаться подтверждёнными** — см. находку +H1 ниже. Прогнал сам: + +- `npx tsc --noEmit` — чисто. +- `npm test` — 1425 passed, 0 failed, 1 skipped (без изменений от базовой + линии). +- `npm run build` + `cmp dist/houseplan-card.js + custom_components/houseplan/frontend/houseplan-card.js` — идентичны. +- `node scripts/mutation-gate.mjs --check` — реестр консистентен, новый + мутант `junction-limit-optimize-unguarded` зарегистрирован корректно + (patch-строка совпадает с реальным кодом). +- Ручная проверка «мутант умеет убивать»: применил patch мутанта + (`return validate_junction_limits(...)` → `return {}, None`) к + `websocket_api.py`, прогнал + `pytest tests_backend/test_ha_websocket.py -k test_333_optimize_refuses_a_crafted_violation` — + тест красный (`assert not True`), рабочее дерево вернул к исходному + (`git status` чист). Тест умеет падать. +- `python -m pytest tests_backend -q` (полный бэкенд-набор, HA-харнес + реально поднимается в этом окружении, вопреки заметке автора о песочнице) — + **1 failed, 428 passed, 1 skipped, 1 error**. Разбор ниже (находка H1). +- `node scripts/smoke-select.mjs --base 73406225 --head c29df29b` — + «исполняемого frontend-диффа нет», браузерные смоки не выбираются + (diff не трогает `src/**`) — решение подтверждено инструментом. +- `check-docs` / golden / invariants — не гонял, diff не трогает `src/**` и + не меняет геометрию/ссылки на неё (только добавляет вызов уже + существующего валидатора на втором командном пути); обоснование ниже, §4. + +Не гонял и не буду: `python -m pytest tests_backend -q` уже покрывает +паритет П1–П4 (`test_junction_limits.py::test_parity_with_the_frontend_checks`) +без дополнительных прогонов. + +## 3. Находки + +### H1 (High, в скоупе). Optimize новым гейтом ломает существующий бэкенд-тест — подтверждённая регрессия + +`plan/optimize` на конфиге, который НИКОГДА не сохранялся (пустое +`config_data.get("config")`, `expected_config_rev: 0`), теперь отказывает на +`junction_limit_length`, если в кандидате есть хоть один сегмент короче +20 см — потому что для «предыдущего» состояния `old_counts` пуст, и +существующее правило («первая запись не может прийти уже сломанной», +`junction_limits.py:415`) считает любое нарушение новым. + +Ровно это происходит с существующим (не тронутым этим диффом) тестом +`test_plan_optimize_persists_exact_storage_roundtrip_target` +(`tests_backend/test_ha_websocket.py:599`, фикстура +`test/fixtures/optimize-storage-roundtrip.json`, тест — про #248, байт-точный +роундтрип storage, а не про геометрию): пространство `fine` содержит комнату +6×6/12 «условных» см (специально крошечную — фикстура проверяет округление на +разных `cell_cm`), после миграции в ней есть стена длиной 6 см — П3, минимум +20 см. + +Воспроизведение: + +``` +$ python -c " +from custom_components.houseplan.junction_limits import validate_junction_limits +import json +config = json.load(open('test/fixtures/optimize-storage-roundtrip.json'))['input']['config'] +validate_junction_limits(config, None) +" +JunctionLimitError: space=fine; rule=length; subject=wall-2p5v3xgwtvsbainkhyk4; actual=6; limit=20 +``` + +Доказано двумя прогонами полного `pytest tests_backend/test_ha_websocket.py -k +test_plan_optimize_persists_exact_storage_roundtrip_target`: + +- на родителе `73406225` (до фикса #333): **1 passed** (плюс тот же фоновый + teardown-`ERROR`, см. ниже — он не от этого коммита); +- на `c29df29b` (после фикса): **1 failed** (`assert response["success"]` → + `False`). + +Это не «недостающий тест», это диффом сломанный существующий зелёный тест — +полный набор `pytest tests_backend -q` на `c29df29b` красный +(`1 failed, 428 passed`). Мёртвого запуска этого набора на CI сейчас не +происходит (см. находку вне скоупа ниже), поэтому красный статус не был +замечен автоматически, но ревью обязано было его поймать — и поймало. + +Фикс — решение автора; правдоподобные варианты: (а) поправить фикстуру +`optimize-storage-roundtrip.json`, вложив в тест уже сохранённое предыдущее +состояние с тем же нарушением (echo-паттерн AC2), раз тест не должен +проверять геометрию; (б) явно обсудить в ТЗ, должен ли `plan/optimize` как +первая-когда-либо запись (`config_rev==0`) подчиняться тому же правилу, что +и `config/set` — если да, чинится тест; если нет, гейту нужно исключение, +и это уже требует правки контракта, а не только кода. + +**Блокирует.** Verdict: жёлтый. + +### Вне скоупа (заведено отдельным issue) + +Тестируя допущение брифа «Validate на `c29df29b` завершился success ⇒ tsc/ +npm test/npm build уже подтверждены», обнаружил, что job'ы `Фронтенд: типы, +юниты, мутанты, синхрон бандла` и `Бэкенд: pytest в Home Assistant` в этом +прогоне — `skipped`, а не `success`; общий «success» рана этого не показывает. +Причина — в классификации изменённых файлов (`.github/workflows/validate.yml`, +job `changes`): `BEFORE_SHA` этого push ссылается на коммит, вышедший из +истории веткой при force-push (аменд предыдущего пуша той же ветки, run +33145390451, `cancelled`), фолбэк на `merge-base origin/dev HEAD` разрешился +так, что диапазон включил только два doc-коммита ревью и потерял сам код-фикс +— `backend`/`frontend` вышли `false`, тяжёлые job тихо пропущены. + +Дефект соседний (инфраструктура CI, не продуктовый код этой задачи) и не +чинится в этой ветке. Заведён: **#347** — `ci: классификация файлов Validate +теряет диапазон после force-push, тяжёлые job тихо skip'аются` (labels: +`bug, infra, P1, S1-new`, ссылка на #333). + +## 4. Что проверено и корректно + +- **Контракт п.1 (вызов гейта в optimize).** `websocket_api.py:1609-1710`: + `validate_junction_limits(msg["config"], config_data.get("config"))` + вызывается ПОСЛЕ `CONFIG_SCHEMA`/миграции кандидата (`commit_wall_segment_model` + при необходимости) и после остальных семантических валидаторов, внутри + `_validate_optimize_cpu` — то же место в конвейере, что и в `config/set` + (websocket_api.py:1343). `JunctionLimitError` в except-списке (было мёртвым) + теперь действительно ловится и транслируется в `connection.send_error` + тем же стабильным кодом `junction_limit_`. +- **AC1 (крафтовый payload).** Тест + `test_333_optimize_refuses_a_crafted_violation_and_keeps_the_plan` + (`tests_backend/test_ha_websocket.py:2388`) прогнан — **PASSED**. Мутант + подтверждён ручным патчем (см. §2). Хранимые `config` и `rev` + побайтово не изменились после отказа — проверено ассертами теста. + Layout-неизменность из формулировки AC1 тестом не проверяется явно, но + гарантирована структурой кода: `JunctionLimitError` ловится ДО первого + `async_save_layout_state`/`async_save_config_state` в функции — физически + нечему меняться (см. Low-замечание ниже). +- **AC2 (наследование в optimize).** Тест + `test_333_optimize_inherits_stored_violations` — **PASSED**: эхо-оптимизация + пространства с унаследованным нарушением угла проходит. Согласуется с + #329 AC10 (легаси-оптимизация не создаёт новых нарушений). +- **AC3 (обновление кэша).** Тест + `test_333_optimize_refreshes_the_junction_baseline_cache` — **PASSED**: + через `recording`-обёртку над `validate_junction_limits` подтверждено, что + следующий `config/set` вызывается с `baseline_counts` из кэша + (`rt.junction_baseline`), не пересчитывая `previous`. + `websocket_api.py:1783` (`rt.junction_baseline = (int(new_config_rev), + optimize_counts or {})`) — симметрично строке 1396 в `config/set`. +- **AC4 (докстринг/спека).** Прочитал оба текста целиком: + `junction_limits.py:1-15` больше не обещает «hostile client cannot post» + без оговорок — явно называет периметр (`config/set` и `plan/optimize`) и + сознательно исключённый import/restore, с обоснованием (§3 #329: restore + не блокируется никогда). `docs/specs/329-junction-limits.md` §5 (новый + абзац, строки 127-134) — тот же периметр и тот же трейд-офф, ссылка на + решение владельца 2026-08-28. Текст соответствует коду, догадок, выданных + за решённое поведение, не нашёл. +- **Паритет П1–П4 и остальной бэкенд.** `test_parity_with_the_frontend_checks` + и все прочие 424 теста `tests_backend` (кроме H1) прошли без изменений. +- **Трейлеры коммита.** `Issue: #333`, `User-Visible: no` — оба на месте, + соответствуют характеру изменения (нет пользовательского поведения, + видимого честному клиенту). +- **Реестр мутаций.** `--check` зелёный, новый мутант описан честно + (`because` указывает на реальный риск — крафтовый optimize-payload + легализует нарушение для будущих `config/set`). + +## 5. Чего не проверял и почему + +- **check-docs, golden:verify, browser-смоки** — diff не трогает `src/**` + вообще (только `custom_components/**/*.py`, `docs/specs/*.md`, + `scripts/mutation-gate.mjs`, `tests_backend/*.py`); `smoke-select.mjs` + подтвердил инструментом «исполняемого frontend-диффа нет». Рендер и + геометрия не меняются — гейты не относятся к этому diff. +- **`npm run invariants`** — diff не меняет геометрию, рёбра комнат, записи + толщины, `layout`, `marker.space` или `open_spans`; добавляет только вызов + уже существующего Python-валидатора на втором командном пути. Инварианты + модели тут не про что проверять. +- **performance-профили** — не названы в AC; #330 §4.1 уже вынесла всю + дорогую цепочку (включая `validate_junction_limits`) в executor, это не + трогается. +- **Полный `node scripts/mutation-gate.mjs`** (все мутанты) — избыточен: + диффу соответствует ровно один новый мутант, он проверен точечно (ручной + патч + прогон целевого теста), это дешевле и доказательнее, чем гонять + весь реестр. +- **HA-харнес** — вопреки заметке автора («локальная песочница… не + поднимает»), в ЭТОЙ ревью-сессии `pytest-homeassistant-custom-component` + установился и харнес поднялся; воспользовался этим и прогнал реальные + прогоны, а не читал код вслепую. Один посторонний `ERROR` на teardown + каждого HA-теста (`assert isinstance(thread, threading._DummyThread) or + thread.name.startswith("waitpid-")`) — известная нестыковка версии + `pytest-homeassistant-custom-component` с Python 3.12 (первый поток теста + назван иначе), воспроизводится одинаково и на родителе `73406225`, и на + `c29df29b`, до и после патча мутанта — не связан с этим диффом, тестовые + тела он не затрагивает (различаю `FAILED`/`PASSED` в основном теле от + `ERROR` в teardown). + +## 6. Вердикт + +Контракт ТЗ реализован технически точно (AC1–AC4 каждый по отдельности +доказан тестом, который умеет падать), но диффом ломается несвязанный +существующий тест — реальная, воспроизведённая регрессия уровня High. +Правки ТЗ не требуется (если автор выберет чинить фикстуру/тест), либо +требуется ревизия контракта (если выберет явно исключить «первую запись» +из-под гейта optimize) — в обоих случаях нужен повторный код-ревью цикл. + +Вердикт: жёлтый · заход r1 · блокирующих циклов 0/2 · High: 1 · Medium: 1 → #347