diff --git a/docs/reviews/CODE-REVIEW-329-r1.md b/docs/reviews/CODE-REVIEW-329-r1.md new file mode 100644 index 00000000..880d704b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-329-r1.md @@ -0,0 +1,337 @@ +# CODE-REVIEW-329-r1 + +Issue: #329 · этап: code (PROCESS.md §2.7) · заход r1 · блокирующих циклов 0/4 (до этого вердикта) + +## Скоуп + +Диапазон: `origin/dev...HEAD` на ветке `issue/329-junction-limits`, HEAD = +`7017896e` (докоммит после ребейза на `dev`, который вобрал #264 — +извлечённый `ResizeController`). Ветка чисто ребейзится на `origin/dev` +(`git merge-base origin/dev HEAD` == tip `origin/dev`), конфликта нет — два +предыдущих запуска конвейера, отказавшихся ревьюить из-за конфликта, здесь не +актуальны, ветка перебазирована и запушена автором (комментарий +2026-08-27T20:00:24Z). + +Задача обычного трека (не `small`): ТЗ — `docs/specs/329-junction-limits.md`, +ревизия 6, спек-ревью прошло r1→r3 (зелёный на r3), затем автор внёс правки +владельца §4/П3 напрямую (ревизии 4–6) без дополнительного раунда +спек-ревью — это первый code-review заход для итогового кода. + +43 изменённых файла, из них продуктовый код: `src/junction-limits.ts` (новый), +`src/houseplan-card.ts`, `src/wall-thickness.ts`, `src/resize-controller.ts`, +`src/i18n/{en,ru}.json`, `custom_components/houseplan/junction_limits.py` +(новый), `custom_components/houseplan/websocket_api.py`. Остальное — тесты, +golden, документация, сгенерированный бандл. + +## Как проверялось + +Дешёвые гейты (обязательные всегда): + +- `npx tsc --noEmit` — чисто. +- `npm test` — 1388/1389 pass, 1 skip (питоновский parity-тест живёт в + `tests_backend`, не в node-раннере). Ни одного red. +- `npm run build` — собрался; `dist/houseplan-card.js` побайтово совпадает с + `custom_components/houseplan/frontend/houseplan-card.js` (сверено `diff -q` + после `npm run bundle:sync`). +- `node scripts/check-docs.mjs` — «Documentation checks passed (7 files, 10 + external links)», отпечаток скриншотов свежий (последний коммит ветки как + раз чинит это после ребейза). + +Геометрия/инварианты (диф трогает рёбра стен, `wall_segments`, толщину, +`layout`): + +- `npm run invariants -- --config test/fixtures/329-sharp-apex.json` — + «Инварианты выполнены: ссылки разрешимы, записи толщины находятся» (ключ + записи толщины и ключ решёточного ребра совпадают на самой фикстуре issue). +- `npm run golden:verify` — весь текущий набор (полная матрица, не только + новая сцена) прошёл, включая новую `sharp-apex-legacy-dark` (AC6) и уже + существующую `junction-owner-repro-dark`; ни одна старая сцена не + изменилась — риск §10 закрыт исполнением, а не заявлением. + +Бэкенд (диф трогает `custom_components/houseplan/*.py`): + +- `python -m pytest tests_backend/test_junction_limits.py -q` — 7 passed, + включая `test_parity_with_the_frontend_checks` (реально прогнал TS-функции + через `node --eval` и сравнил с питоновским вердиктом — не skip, `test-build/` + был собран `npm run invariants` до этого). +- Полный `python -m pytest tests_backend -q` в этой среде не поднимается: + `homeassistant` не установлен (это облачная CI-среда без + `.venv-backend`, установить полный HA недостаточно оправдано ради одного + ревью) — падает на импорте в трёх модулях, не в затронутых #329 файлах. + Ограничение окружения, не решение сузить проверку: описано в AGENTS.md + («Локально только pytest без HA — silently skips `test_ha_*.py`»). Здесь + импорт вообще падает раньше skip-логики `conftest.py`, поэтому я запустил + ИМЕННО целевой файл отдельно (см. выше) вместо того, чтобы полагаться на + «зелёный», который ничего не доказывает. +- Дополнительно: написал и выполнил собственный python-репро (см. находку + H1) — прямой вызов `validate_junction_limits` на сконструированных + fixtures, не полагаясь на чтение кода. + +Мутанты (диф добавляет/меняет 4 мутанта в `scripts/mutation-gate.mjs`) — +прогнаны точечно, не весь гейт (несоразмерно объёму ревью): + +- `junction-limit-angle-not-enforced` — поймано 1/1. +- `junction-limit-write-gate-removed` — поймано 1/1. +- `degenerate-apex-bevelled-again` — поймано 1/1. +- `resize-preview-reject-silent` (изменённый якорь) — поймано 1/1. + +Браузерные смоки — выбраны через `node scripts/smoke-select.mjs --base +origin/dev --head HEAD` (43 символа на изменённых строках `src/**` в 4 +файлах, порог широкого символа 38 смоков — под порог не попал ни один, +инструмент не потребовал расширять выборку до всей матрицы 193). Прогнаны: +оба смока, названных в AC/изменённых диффом (`smoke_junction_limits.mjs` — +AC1/AC3/AC5a/AC7a/AC7b/§3, `smoke_island_rooms.mjs` — П3 на комнатах-островах), +плюс все 13 «прямых совпадений» и 1 «зарегистрированная связь» +(`smoke_real_plan_masonry` — инструмент сам напомнил, что на реальном плане +кладка рвётся там, где синтетика цела, и восемь прошлых задач по стыкам уже +входили в бету с необнаруженным разрывом; учитывая, что диф правит +`insetContour`/`outsetContour`, счёл прогон обязательным, а не факультативным). +Итог — 15/15 OK, ни один не покраснел. + +## Находки + +### H1 (High, в скоупе). Бэкенд отказывает в легитимной правке легаси-плана — прямое нарушение §3 + +`custom_components/houseplan/junction_limits.py::validate_junction_limits` +сравнивает «before» и «after» по СЫРЫМ документам (`msg["config"]` и +`data.get("config")`), не пропуская ни один из них через +`commit_wall_segment_model`. Фронтенд ровно эту ошибку уже нашёл и +исправил в коммите `4758767e` («junction limits judge both sides after the +same migration») — коммит-сообщение прямым текстом объясняет механику: +«The baseline for inheritance was the raw previous document, which for a +legacy space carries no wall catalogue at all — so every inherited short +segment of a real plan looked new». На бэкенде аналогичная миграция +(`commit_wall_segment_model`) вызывается только в `import_export.py` и в +`ws_plan_optimize` (и то — условно, при `submitted_model < WALL_SEGMENT_MODEL_VERSION`), +но НЕ в `ws_config_set`, где живёт вызов `validate_junction_limits` +(websocket_api.py:1334). + +Следствие: хранимый (`previous`) документ пространства, которое ещё ни разу +не проходило структурную запись после введения каталога `wall_segments` +(`model_version < 8`), не содержит `wall_segments` вовсе — +`limit_segments()` читает только `wall_segments`/`partitions`/`room_drafts`, +поэтому `space_violations(old_space)` возвращает `[]` для такого +пространства НЕЗАВИСИМО от реальной геометрии. Кандидат же (`msg["config"]`) +современный клиент уже мигрировал на клиенте до отправки — там +`wall_segments` есть. Любое унаследованное нарушение (в т.ч. безобидное, +годами живущее в реальном плане пользователя — ровно повод, по которому +заведён #329) на первой же структурной правке после обновления карточки +читается как «новое» и запись ЦЕЛИКОМ отклоняется — даже если правка не +касалась нарушающего узла. Это прямое нарушение §3 ТЗ («миграция... +не блокируются никогда», «правка, не трогающая нарушающий элемент, +проходит как обычно») и AC8/AC9 по факту (тест паритета в +`test_junction_limits.py` не ловит это, потому что в его фикстурах ОБЕ +стороны уже содержат `wall_segments` — сценарий «previous без каталога» +не фигурирует нигде в тестах). + +**Воспроизведено исполнением** (не только чтением кода) — прямой вызов +модуля: + +```python +previous = {"spaces": [{ + "id": "s", "cell_cm": 5.0, "model_version": 0, "rooms": [], + "walls": [ # легаси-хранение, БЕЗ ключа "wall_segments" + {"key": "w0", "a": [0.0, 0.0], "b": [...], "cm": 15}, + {"key": "w1", "a": [0.0, 0.0], "b": [...], "cm": 15}, # угол 9° + ], +}]} +candidate = {"spaces": [{ + "id": "s", "cell_cm": 5.0, "model_version": 9, + "rooms": [{"id": "r1", "name": "Renamed"}], # НЕСВЯЗАННАЯ правка + "wall_segments": [ # тот же угол 9°, клиент уже мигрировал + {"id": "w0", ...}, {"id": "w1", ...}, + ], +}]} +jl.validate_junction_limits(candidate, previous) +# → JunctionLimitError: junction_limit_angle; actual=9; limit=15 +``` + +Результат: `REFUSED: junction_limit_angle` — легитимная переименование +комнаты на давно существующем плане с историческим острым углом отклоняется +целиком. Реальный масштаб поражения велик: сама мотивация #329 — план +РЕАЛЬНОГО пользователя с давним острым углом; после этой задачи такой +пользователь не сможет сохранить вообще никакую правку своего плана, пока +не прогонит Optimize (о котором он может не знать) — притом что §3 прямо +обещает обратное, и фронтенд этот же случай уже умеет обрабатывать +корректно (проверено смоком `§3: унаследованное нарушение не блокирует +несвязанную запись` внутри `smoke_junction_limits.mjs`, который проходит +именно потому, что бьёт только по фронтенд-пути). + +Чинится по образцу уже принятого решения: перед сравнением пропустить +`old_space` (и, для симметрии, `space`) через `commit_wall_segment_model` +— тем же приёмом, каким `_junctionLimitsIntroduced` в `houseplan-card.ts` +уже чинит идентичный дефект на фронтенде. `docs/CONFIG-COMPATIBILITY.md` +(этот же дифф) уже ОБЕЩАЕТ это поведение текстом «both have gone through +the same commitWallSegmentModel migration» — то есть документация описывает +контракт, которого бэкенд не реализует. + +### M1 (Medium, в скоупе). Мёртвый код в `wall-thickness.ts`, чьё удаление коммит уже задекларировал + +Коммит `002795f7` («fix: no jags on the edges of a degenerate apex») +убрал оба места, где вызывались `degenerateApexCaps()`/`clipPolygonOutsideCap()`, +и сообщение коммита прямо утверждает: «The clip helper and its cap plumbing +are gone». На деле в текущем HEAD остаются: + +- `apexCaps?: number[][][]` — поле интерфейса `MultiWallRoomRing` + (`src/wall-thickness.ts:2815`), никогда не устанавливается и не читается; +- `export function clipPolygonOutsideCap(...)` (строка 3637, ~30 строк); +- `export function degenerateApexCaps(...)` (строка 3696, ~60 строк). + +Ни один из них не вызывается из продуктового кода (`grep` по `src/` даёт +только сами определения) и не покрыт тестами (`grep` по `test/` — ноль +совпадений). Реальный переход на «острие вместо фаски» сделан иначе — +через `isDegenerateApexCorner` прямо в `insetContour`/`outsetContour` — эти +функции остались как нетронутый, непроверяемый и вводящий в заблуждение +довесок: следующий читающий код решит, что это часть работающего конвейера +клиппинга (интерфейс даже комментирует поле как «#329 §4: clip regions...»), +хотя оно мертво с коммита, заявившего обратное. + +### M2 (Medium, в скоупе). Дублированный раздел в `docs/USER-GUIDE.ru.md` + +Раздел «### Ограничения стыков стен» вставлен ДВАЖДЫ подряд, слово в слово +(строки 420–443 и 445–468 идентичны по всему тексту, включая таблицу и оба +абзаца после неё). Английская версия (`docs/USER-GUIDE.md`, «### Wall +junction limits») дублирования не имеет — это чисто копипаст-дефект русской +версии, попавший в тот же коммит, что и остальная документация задачи +(`User-Visible: yes` требует правки обоих changelog и, по духу того же +правила, аккуратной документации на обоих языках). + +### M3 (Medium, в скоупе). USER-GUIDE описывает канал Resize неполно — читатель не узнает про тост + +И `docs/USER-GUIDE.md`, и `docs/USER-GUIDE.ru.md` в новом разделе пишут: +«рисование и «Толщина» показывают тост с названием правила, а Resize +останавливает стену в последней разрешённой позиции» (RU, строки 441–443/ +466–468; аналогично EN). Формулировка явно противопоставляет тост (для +рисования/Толщины) и «просто останавливается» (для Resize) — то есть +читатель разумно поймёт, что при Resize тоста нет. + +По факту `src/houseplan-card.ts` (строки ~9181–9188) показывает тост и для +Resize: `resize.limit_stopped` + `_junctionLimitLabel(...)`, что подтверждено +исполнением — смок `smoke_junction_limits.mjs` проверяет +`resizeRefusalNamesRule` (тост содержит «5») и `resizeRefusalOnce === 1`, и +оба зелёные. Это ровно тот класс дефекта, который проверяет +`test/single-source-numbers.test.mjs` для чисел, но здесь смысловая +сторона: сам факт наличия канала обратной связи задокументирован неверно. +AC7a (ревизия 6 спека) корректно описывает тост-канал — расхождение +только в изданном для пользователя `USER-GUIDE`, который вправляли отдельно +и не сверили с финальным текстом AC7a. + +Смежное наблюдение (не отдельная находка, отметка для автора): §2 самого +ТЗ (нормативный текст) тоже не обновлён вслед за AC7a ревизии 6 и всё ещё +говорит «тост не используется» для Resize — но ТЗ на этапе code-review не +правится, отмечаю для полноты картины, поскольку именно расхождение §2 с +AC уже один раз обсуждалось на спек-ревью (r2-M1) и, похоже, вернулось в +другом месте документа. + +### M4 (Medium, в скоупе). AC10 (постусловие Optimize) не доказан ни тестом, ни явным разбором + +AC10: «Optimize на легаси-фикстуре не создаёт новых нарушений +(AC-постусловие §3)». В диффе нет ни юнита, ни смока, который прогоняет +`optimizePlans`/`ws_plan_optimize` на фикстуре с существующим нарушением и +сравнивает набор нарушений до/после. `grep` по «Optimize»/«optimize» во +всех тестовых файлах, тронутых этой задачей, ничего не находит. + +Это не тривиально верно «по построению»: `optimizePlans` вызывает +`alignAllToGrid` и `repairNearAxisRoomWalls`, которые двигают узлы к +решётке и выпрямляют почти-осевые стены — оба потенциально смещают +геометрию на доли сантиметра. Ни один из порогов П1–П5 не имеет запаса +шире шага решётки в общем случае (например, узел ровно на 5.0 см от +чужой стены после сдвига на привязку может уйти ниже порога). Формально +проверить это чтением я не могу (нужно было бы доказать, что снап +монотонно не уменьшает ни один из пяти инвариантов, что для П1/П4 не +очевидно), поэтому по правилу «либо тест, либо разбор с явной пометкой» +AC10 сейчас не доказан никак. + +## Что проверено и корректно + +- П1–П5 (`src/junction-limits.ts`) — юнит-покрытие точное, включая границы + (14°/15°/16°, 6/7 стен, 19/20/25 см, 4/5 см, пустой/непустой просвет), + T-стык как легальная инцидентность, компенсацию перепада толщин (АС3b) — + числа владельца (30/20 см → 5 см доборный атом) воспроизведены и на + фронте, и на бэке идентично. +- §4 (честная вершина легаси) — `isDegenerateApexCorner`, + `insetContour`/`outsetContour` дают ровно одну точку в вершине плана и + снаружи, и внутри; внешнее кольцо тела — треугольник без ступенек и + микровершин; golden-сцена `sharp-apex-legacy-dark` подтверждает это на + пиксельном уровне, вся остальная golden-матрица не сдвинулась. +- Наследование нарушений считается ПО ПРАВИЛУ (`increasedViolations`), а не + по subject id — комментарий и тест прямо объясняют, почему id нестабилен + через структурную запись (переатомизация); фронтенд-часть барьера + (`_junctionLimitsIntroduced`) верно мигрирует ОБЕ стороны через + `commitWallSegmentModel` перед сравнением — это именно то, чего не хватает + на бэкенде (H1). +- AC7a (Resize) — проверено настоящим pointer-жестом (не вызовом + внутренних методов): сетка 2 см, две комнаты в 10 см, тяга 6 см + останавливается на 6 см (не доезжает до нарушающих 4 см), ровно один тост + с «5». `_rszLimitViolation` сбрасывается в начале КАЖДОЙ проекции — + проверил код: без этого сброса тост от предыдущего шага мог бы объяснить + отказ другого рода; тест на это (`resize-preview-reject-silent` мутант) + ловит немоту, но не проверяет свежесть причины — сам код читаем и + корректен (обнуление на первой строке `_rszProjectPreview`). +- AC7b («Толщина») — значение не применяется, тост с названием правила, + план байт-неизменен (смок). +- AC1/AC3/AC5a — смоком, с проверкой, что тост называет конкретное число + (не просто безмолвный отказ). +- AC2/AC4 — юнитами на обеих сторонах (фронт+бэк), включая T-стык. +- AC6 — см. выше, golden + юниты на контурах и кольце. +- AC8 (миграция/импорт/restore не блокируются) — проверено ЧТЕНИЕМ, не + исполнением: `validate_junction_limits` вызывается только из + `ws_config_set`; ни `ws_import_apply`, ни `ws_files_migrate` его не + вызывают. На фронте `_junctionLimitsIntroduced` вызывается только из + `_commitPhysicalGeometry` и `_rszProjectPreview` — оба это операции + редактирования, не импорта/миграции. +- AC9 (паритет) — исполнено: `test_parity_with_the_frontend_checks` + реально прогоняет TS через `node --eval` и сравнивает с питоновским + результатом на 8 фикстурах, 7/7 pass. +- Backend exception plumbing в `ws_plan_optimize`: `JunctionLimitError` + добавлен в `except`-кортеж, хотя `validate_junction_limits` там не + вызывается (сознательно, по §5 — Optimize не должен упираться в новые + правила) — код от этого не ломается, исключение просто никогда не + возбуждается в этой ветке; не поднимаю до отдельной находки, так как + безвредно и не вводит в заблуждение так же сильно, как M1 (нет + комментария, утверждающего обратное). +- AC5b — формула П5 (`checkRoomClearance`) шейп-агностична (это чистая + формула площади по шнурку), поэтому квадратные фикстуры теста + математически эквивалентны треугольнику из спека; но буквально + «юнит с этими числами» (равносторонний 60/33 см, inradius 17.3/34.6 см) + отсутствует — фиксирую как Low, не поднимаю до Medium: код, который + доказывал бы обратное, был бы тем же самым. +- Трейлеры: `Issue: #329` на каждом коммите; `User-Visible: yes` только там, + где меняется видимое поведение (214355f4), и в том же коммите правки + обоих changelog — проверено `git show --stat`. +- Одно число — один источник: `_junctionLimitLabel` — единственное место, + формирующее текст отказа что для тоста рисования/Толщины, что для + Resize; `actual`/`limit` берутся из одного и того же объекта + `JunctionLimitViolation`, не пересчитываются отдельно для тоста и для + ручки — проверено чтением, не вижу второго источника чисел. + +## Чего не проверял и почему + +- Полный `python -m pytest tests_backend -q` (HA-harness) — недоступен в + этой среде (`homeassistant` не установлен, нет `.venv-backend`); прогнан + только целевой файл задачи (полностью, 7/7) плюс попытка полного набора + зафиксирована как падение импорта, не как «зелёный, но неполный». +- Полная браузерная матрица (193 смока) — не прогонял, диф не потребовал + этого по `smoke-select` (43 символа, порог 38 не превышен); прогнал все + прямые совпадения, зарегистрированную связь и оба AC-смока (15 итого). +- Полный `scripts/mutation-gate.mjs` (весь набор мутантов) — несоразмерно + ревью; прогнал точечно только 4 мутанта, которые диф добавил/изменил. +- `performance_smoke` — не прогонял: в AC не назван, диф не трогает горячий + путь рендера (П1–П4 — O(узлы+сегменты) на запись, П5 — на commit, не на + каждый кадр курсора, это подтверждается чтением: вызовы + `_junctionLimitViolations`/`_junctionLimitsIntroduced` находятся в + `_commitPhysicalGeometry` и в `_rszProjectPreview`, оба — не per-frame + рендер-путь). +- Ручное тестирование в браузере — не проводил (этап code-review, ручного + тестирования в цикле нет по регламенту; браузерные смоки — единственная + замена). +- AC5b буквально «с этими числами» — не поднимаю до отдельного прогона, + формула шейп-агностична (см. выше), различие признано как Low и не + требует отдельного гейта. + +## Итог + +Один High (H1) — реальный, воспроизведённый исполнением дефект, прямо +нарушающий §3 ТЗ и обесценивающий центральное обещание задачи («легаси не +блокируется») для бэкенд-пути записи. Плюс четыре Medium в скоупе (M1–M4), +все чинятся в этой же задаче без отдельного issue (владелец, #202).