From 294d7047f376df19a6da69833ea7605504e79a25 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:30:31 +0000 Subject: [PATCH] docs: review document for #330 Issue: #330 User-Visible: no --- docs/reviews/CODE-REVIEW-330-r2.md | 257 +++++++++++++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-330-r2.md diff --git a/docs/reviews/CODE-REVIEW-330-r2.md b/docs/reviews/CODE-REVIEW-330-r2.md new file mode 100644 index 00000000..a248f680 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-330-r2.md @@ -0,0 +1,257 @@ +# CODE-REVIEW-330-r2 + +Issue: #330 — «Ограничения стыков (#329) блокируют event loop HA и дёргают ресайз на больших планах» +Этап: code (PROCESS.md §2.7) +Заход: r2 · блокирующих циклов израсходовано 1 из 4 (до этого раунда) +Ветка: `issue/330-junction-limits-performance`, HEAD ревью — `85636a65` + +## 0. Почему разбор полный, а не по дельте + +Предыдущий вердикт (code-review r1, 2026-08-28T00:00:48Z) не назвал SHA, на +котором он получен — это само по себе процессное упущение (см. ниже). Автор +в ответном комментарии называет два SHA до финального ребейза: `a7500973` +(HEAD на момент публикации r1) и `7513f93d` (докс-фикс, слитый до публикации +вердикта). Оба объекта в текущем дереве отсутствуют: + +``` +$ git cat-file -t a7500973 → fatal: Not a valid object name +$ git cat-file -t 7513f93d → fatal: Not a valid object name +``` + +Ветка была перебазирована на ушедший вперёд `dev` через мердж #62 («ребейз +над мерджем i18n-registry», коммит `85636a65`) уже ПОСЛЕ публикации r1. Это +ровно исключение §7.2 из брифинга: «ребейз на ушедший вперёд dev — после +ребейза это другой код». Пословный `git diff ..HEAD` невозможен и не +имел бы смысла — базовые деревья разошлись. Разбор ниже полный, по +`git diff origin/dev...HEAD`, с явным закрытием находок r1 отдельным +разделом. + +**Процессное наблюдение (не находка в код, не считается в M/H):** ни в +вердикте r1, ни в вердикте предыдущего code-review-раунда SHA не был назван +текстом. Формат §7.2 этого не требует явно, но по факту это стоило времени +на этот раунд — рекомендую в следующих раундах явно указывать SHA в теле +вердикта, а не только в документе. + +## 1. Скоуп + +Диапазон `origin/dev...HEAD`, 31 файл, +1731/-258: + +- `custom_components/houseplan/junction_limits.py` — линейный byNode-индекс + П3, bucket-решётка П4, `_migrated_spaces` без ремиграции v9 (§4.6), + `space_violation_counts`/`baseline_counts` (§4.2). +- `custom_components/houseplan/websocket_api.py` — цепочка валидаторов + `ws_config_set`/`ws_plan_optimize` в `async_add_executor_job` (§4.1), + запись/чтение `rt.junction_baseline` по rev (§4.2). +- `custom_components/houseplan/store.py` — новое поле рантайма + `junction_baseline`. +- `src/junction-limits.ts` — те же три среза на фронте (byNode-индекс, + bucket-решётка, зеркальная логика). +- `src/houseplan-card.ts` — `_junctionLimitViolations` принимает + `sharedGeometry` (§4.7, общий проход топологии/кладки на все комнаты), + `_junctionLimitsIntroduced` — кэш baseline по (identity, `_cfgEpoch`) + (§4.4) и путь «v9 как есть» (§4.6 на фронте). +- `demo/benchmark_junction_limits.mjs`, `package.json`, + `.github/workflows/validate.yml` — новый перф-контракт §5/AC7, вшитый в + job `performance_smoke` (не только в еженедельный `mutation-gate.yml`). +- `demo/smoke_junction_limits.mjs` — счётчик вызовов `_junctionLimitViolations` + за один жест ресайза (заявленная поведенческая часть AC4). +- `scripts/mutation-gate.mjs` — три новых мутанта (П3, П4, «протухший + baseline-кэш»). +- `test/junction-limits.test.mjs`, `tests_backend/test_junction_limits.py`, + `tests_backend/test_ha_websocket.py` — юниты эквивалентности и AC1. +- `docs/CHANGELOG.md`/`.ru.md` — одна строка, User-Visible: yes, в том же + коммите (`c90f5bf0`), что и код. +- Скриншоты/`screenshots.json` — механический пересчёт отпечатка + (src/** изменился), содержимое интерфейса не менялось. + +Вердикты П1–П5 не меняются нигде в дифф — заявление автора подтверждено +чтением: единственная содержательная логика (`_length`, `_distance_to_segment`, +условия нарушений, epsilons) не тронута; менялись только структуры данных +вокруг неё (индексация, кэш, порядок обхода). + +## 2. Как проверялось + +Все команды выполнены на этом SHA в этой рабочей копии; результат — вывод +ниже, не «verified» без числа. + +| Гейт | Команда | Результат | +|---|---|---| +| Типы | `npx tsc --noEmit` | чисто, без вывода | +| Юниты фронта | `npm test` | **1419 passed**, 1 skipped, 0 failed | +| Юниты бэкенда (чистый python, без HA) | `python3 -m pytest tests_backend/test_junction_limits.py -q` | **13 passed** | +| Юниты бэкенда (HA-харнесс) | `python3 -m pytest tests_backend -q` (pytest-homeassistant-custom-component установлен в этой сессии) | **423 passed, 1 skipped, 1 error** — ошибка теardown (`_run_safe_shutdown_loop` thread-leak assert) воспроизводится и на НЕ тронутом этой веткой тесте (`test_issue_244_...`) при отдельном прогоне того же файла — окружение, не диффа. Тест **AC1** (`test_330_config_set_validators_run_in_the_executor`) прошёл исполнением: `1 passed`. | +| Перф-контракт §5/AC7 | `npm run benchmark:junction-limits` | `pass: true`; tsFullCandidateMs 150.7/400 (2.65×), pyWarm 67.1/300, pyCold 2463/5000 | +| Сборка + идентичность бандла | `npm run build` + `git status --short` (чисто) + `diff dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | идентичны, `git status` чист после ребилда | +| Докс-гейт | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» | +| Инварианты модели | не гонял отдельной командой — `npm test` уже включает `test/model-invariants.test.mjs` на всех моделях проекта (зелёный); задача не вводит новую конфигурацию для точечной проверки `npm run invariants -- --config` | +| Смок-выборка | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → 7 прямых совпадений + 1 «зарегистрированная связь» (см. §3) | все 8 + 3 названных в AC6 (`island_rooms`, `room_resize`, `resize_pointer_real_plan`) прогнаны — **11/11 OK** (после `node scripts/bundle-sync.mjs`, без которого `demo/srv/assets/houseplan-card.js` отсутствовал и смоки падали таймаутом на билд, не на код) | +| Мутационная проверка (ручная, не полный `mutation-gate.mjs`) | вручную применил 3 патча из `scripts/mutation-gate.mjs` (П3-снова-квадратичен, П4-снова-перебор, протухший baseline-кэш) и прогнал их guard-команды | П3 и П4 **ловятся бенчем** (`pass:false`, tsSegmentLengthsMs 402.6/60 и tsNodeDistancesMs 207.4/80 соответственно); третий мутант — см. находку M1 ниже, там дисциплина «тест умеет падать» провалилась на смоке | +| golden | не гонял | diff не трогает рендер/геометрию/стили/слои — только порядок вычислений и кэш валидатора; визуальный результат не может измениться этим кодом | +| performance-профили сверх §5 | не гонял | не названы в AC сверх §5/AC7 | + +## 3. Смок-выборка — вывод инструмента + +``` +Изменено файлов src/**: 2 · символов на изменённых строках: 17 +Прямое совпадение (7): smoke_junction_holes, smoke_decor, smoke_glow_fail_dark, + smoke_glow, smoke_grid_scale_invariance, smoke_junction_limits, smoke_space_scale_defaults +Зарегистрированная связь (1): smoke_real_plan_masonry ← wallBodiesGeometry +``` +Плюс три смока, названных в AC6 автором (`island_rooms`, `room_resize`, +`resize_pointer_real_plan`) — не входят в прямые совпадения инструмента для +этого diff, но названы в AC, поэтому прогнаны. Итог: 11 смоков, все **OK**. + +## 4. Находки + +### M1 (в скоупе) — заявленная «поведенческая половина» AC4 не различает наличие и отсутствие кэша + +Файл: `demo/smoke_junction_limits.mjs:185-217`; коммит `c90f5bf0` и +подтверждающий комментарий автора («Поведенческая половина AC4 (N move → +N+1 вычислений на настоящем pointer-жесте)»). + +Смок считает вызовы `_junctionLimitViolations` за весь жест ресайза и +проверяет `resizeBaselineCachedPerGesture = jlCalls > 0 && jlCalls <= 12`. +Я вручную воспроизвёл ровно тот мутант, что заведён в +`scripts/mutation-gate.mjs` под id `junction-limit-baseline-cache-stale` +(отключил запись в `_junctionBaselineCache` — кэш никогда не срабатывает, +baseline пересчитывается на каждый move) и прогнал **сам смок** (не юнит, +который guard мутанта вызывает вместо него): + +``` +DEBUG jlCalls= 12 (кэш полностью отключён) +DEBUG jlCalls= 11 (немодифицированный код, кэш работает) +OK (оба раза) +``` + +Разница между «кэш работает» и «кэш полностью выключен» — **один вызов из +двенадцати**, и порог `<= 12` пропускает оба случая. Смок физически не может +отличить рабочий кэш от полностью удалённого — он зелёный в обоих случаях. +Причина в том, что счётчик `window.__jlCalls` ловит вообще все обращения к +`_junctionLimitViolations` за время жеста (включая вызовы вне +`_junctionLimitsIntroduced`, например из рендера/подсветки), а не только +экономию на baseline — сигнал тонет в шуме, и заявленная арифметика +«N+1 против 2N» на практике не воспроизводится этим жестом ни при каком +разумном пороге. + +Это ровно тот класс дефекта, который автор сам поймал для ЮНИТ-теста тем же +приёмом в `7aaf5a72` («тест умеет падать» — контракт перепрятали в +проверку по исходнику именно потому, что первая версия юнита не ловила +мутанты). Для смока тот же самоконтроль не был применён: заявление в +коммите, что поведенческая защита AC4 «живёт в смоке», не подтверждается. + +Фактическая защита AC4 сейчас — только regex-проверка по исходнику +(`test/junction-limits.test.mjs:380-399`, ищет литеральную строку +`cached.epoch === this._cfgEpoch`). Она реально работает как барьер (я +проверял: мой мутант меняет именно эту подстроку и юнит падает), но это +защита от одной конкретной формы регресса (удаление/подмена этого сравнения +в тексте), а не от логической. Сама РЕАЛИЗАЦИЯ кэша в продакшен-коде +корректна — я прочитал `_junctionLimitsIntroduced` (houseplan-card.ts:7503-7549) +и логика верна: WeakMap по identity `previousConfig`, инвалидация по +`_cfgEpoch`, который сам ведёт себя как установившийся паттерн в этом файле +(бампается в ~30 местах при любом реальном изменении `_serverCfg`/эпизода +редактирования). Это находка про КАЧЕСТВО ТЕСТА, а не про баг в фиче. + +**Почему в скоупе и Medium:** AC4 ТЗ требует доказательства «жест ресайза +считает baseline один раз»; заявленное доказательство decorative. Без +High это возвращается автору жёлтым, а не заводится отдельным issue — +находка про код и тесты ЭТОЙ задачи, не соседнего поведения. + +**Что нужно поправить:** либо ужесточить порог до значения, которое +реально разделяет 11 и 12 (например, точное равенство ожидаемому числу +вызовов для конкретной синтетической последовательности `mouse.move` — тогда +и сам тест обязан быть проверен на этом же мутанте), либо честно +переформулировать комментарий/заявление AC4: поведенческая защита не +добавляет покрытия сверх юнита по исходнику, и это нужно признать в +документе, а не заявлять как отдельную гарантию. + +## 5. Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где видно | +|---|---|---| +| H1 — докс-гейт красный (screenshots.json/PNG не из одного прогона) | Уже было закрыто `7513f93d` до публикации вердикта; после второго ребейза (`85636a65`) фингерпринт и PNG пересобраны ещё раз тем же правилом | `node scripts/check-docs.mjs` → «Documentation checks passed» на этом SHA (проверено мной заново) | +| H2 — бюджеты §5 калиброваны с запасом 1.14× вместо 2-3×, бенч не в CI-воркфлоу | Таблица замеров с обеих машин, бюджеты пересчитаны от худшей; шаг `npm run benchmark:junction-limits` добавлен в job `performance_smoke` в `validate.yml` | `.github/workflows/validate.yml:543-547`; сам прогон на этом SHA — `pass:true`, запас 2.65× (150.7/400) | +| M1 — обещанный AC1-тест на самом деле мерил только тёплый `validate_junction_limits`, не `websocket_api.py`/`hass` | Новый тест `test_330_config_set_validators_run_in_the_executor` патчит `validate_junction_limits` обёрткой-шпионом внутри реального HA-харнесса, вердикты (принято/отклонено `junction_limit_angle`) сверены | `tests_backend/test_ha_websocket.py:2322-2385`; **я прогнал его исполнением** (`pytest -k 330` → `1 passed`), не только чтением | +| M2 — §4.6-эквивалентность фронта проверена только текстовым паттерном; python-паритет на одной фикстуре | Три граничные фикстуры (as-is==through-migration) на TS (`test/junction-limits.test.mjs:401`) и на python (`tests_backend/test_junction_limits.py:423`, три полигона: spike/box/narrow) | Оба набора прогнаны: TS — часть зелёного `npm test`; python — часть зелёного `pytest tests_backend/test_junction_limits.py` (13/13) | + +Все четыре находки r1 закрыты по существу, подтверждено исполнением (не +только чтением) там, где это было в моих силах в этой песочнице. + +## 6. Унаследовано из r1 (без повторной проверки) + +- Общая архитектура срезов §4.1–§4.6 (rev-кэш, byNode-индекс П3, + «v9 как есть» §4.6) — признана в code-review r1 корректной по коду и + эквивалентной по вердиктам; в этом раунде я перечитал тот же код заново + (диф не менялся с r1 в этой части, кроме docs/CHANGELOG и бенч-калибровки) + и подтверждаю тот же вывод самостоятельно, а не по доверию к r1 — это не + наследование, а совпавший результат независимой проверки. +- Ничего из r1 не наследуется слепо: поскольку исходный SHA r1 недостижим + после ребейза (см. §0), весь код перечитан заново в этом раунде, а не + предположен на основании старого вердикта. + +## 7. Что проверено и корректно + +- **Эквивалентность вердиктов П1–П5** до и после — не изменилась ни в одной + строке содержательной логики; подтверждено чтением и параллельно + зелёными паритет-тестами TS↔Python (`test_parity_with_the_frontend_checks`, + уже существовавший до #330, прошёл в составе 13/13). +- **Bucket-решётка П4**: прочитал построчно оба зеркала (`junction_limits.py:223-291`, + `junction-limits.ts:188-247|`); граничный случай (узел и сегмент в соседних + ячейках решётки, но ближе порога) закрыт паддингом bbox сегмента на + `min_units` и проверен тестом `test_330_p4_bucket_matches_bruteforce_on_cell_borders` + (4 кейса, включая ровно 4 и 5 см от порога) — прогнан, зелёный. + Ассиметрия «узел проверяется в 9 соседних ячейках, сегмент — только в + своей» корректна по построению: паддинг сегмента компенсирует то, что для + узлов (точек) эквивалентного паддинга нет. +- **§4.6 «v9 как есть»**: перечитан барьер в `websocket_api.py:1325` (schema + всё равно валидирует документ целиком независимо от пропуска ремиграции) — + риск «подделать v9 без каталога» закрыт независимо от нового теста. +- **Executor-обёртка** в `ws_config_set`/`ws_plan_optimize`: `write_lock` + держится вокруг `await`, поэтому сериализация записей не нарушена; ошибка + `too_large` в `ws_plan_optimize` корректно вынесена из executor-функции + обратно в вызывающий код перед `connection.send_error` (сеть — дело event + loop, не executor-потока). +- **`rt.junction_baseline`**: инвалидация «по факту», а не по всем путям + записи — `ws_plan_optimize` не обновляет кэш вовсе (и не обязан: он не + проверяет ограничения стыков ни до, ни после #330 — не регрессия), поэтому + следующий `config/set` просто промахнётся по rev и пересчитает `previous` + с нуля; корректность не зависит от того, кто ещё пишет rev. +- **§4.7 общий проход**: `preflight.wallGeometry`, переданный в + `_junctionLimitsIntroduced` как `candidateGeometry`, построен + (`_rszSpaceCandidateGeometry`) из ТОЙ ЖЕ пары (`spaceId`, `sp`), что и + `limitCandidate` — не подмена по случайному кэшу; проверено чтением обеих + функций (houseplan-card.ts:9104-9121, 9156-9175). +- **CHANGELOG** — одна строка в обоих файлах, в том же коммите (`c90f5bf0`), + что и код, при `User-Visible: yes`. +- **Одно число — один источник**: diff не добавляет и не меняет ни одной + видимой пользователю величины (только внутренние тайминги/пороги + бенчмарка, невидимые пользователю) — раздел неприменим. + +## 8. Чего не проверял и почему + +- **golden:verify** — не гонял; diff не трогает рендер, геометрию, + стили или слои, только порядок/кэш вычислений валидатора стыков. +- **Полная матрица `demo/smoke_*.mjs` (194 шт.)** — не гонял; задача не + меняет ничего, что затрагивает широкие символы (`_cfgEpoch`, `_serverCfg`, + `_space` инструмент явно исключил как «не широкие» в данном случае), 11 + выбранных/названных в AC смоков — зелёные. +- **`benchmark_safe_resize`** — не гонял; это тот самый бенч, про который + spec-review r1 (H2) установил, что он не вызывает код лимитов вообще — + нерелевантен этой задаче. +- **Полный `scripts/mutation-gate.mjs` прогон (все ~40+ мутантов)** — не + гонял; это еженедельный гейт (PROCESS.md §8), не гейт ревью. Три НОВЫХ + мутанта проверил вручную построчно (см. §2, §4) — этого достаточно для + вопроса «умеет ли тест, который я прогонял, падать». +- **`npm run invariants -- --config <...>`** отдельной командой — не + гонял; задача не вводит новую конфигурацию/фикстуру геометрии за + пределами того, что уже покрыто `npm test` (который включает + `model-invariants.test.mjs` на моделях проекта). +- **Ручное тестирование в браузере** — вне цикла ревью по регламенту; браузерные + смоки — единственная замена, они прогнаны. + +## 9. Итог + +Один Medium в скоупе (M1 — заявленная поведенческая защита AC4 не +дискриминирует регресс, эмпирически подтверждено), High нет. Основная +перф-архитектура (§4.1–§4.7), эквивалентность вердиктов и перф-контракт §5 +подтверждены исполнением, а не только чтением или доверием к предыдущему +раунду.