From eeb9c3482573a5a3e93e00bb8eff5c4e97922977 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:50:57 +0000 Subject: [PATCH] docs: review document for #172 Issue: #172 User-Visible: no --- docs/reviews/CODE-REVIEW-172-r1.md | 168 +++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-172-r1.md diff --git a/docs/reviews/CODE-REVIEW-172-r1.md b/docs/reviews/CODE-REVIEW-172-r1.md new file mode 100644 index 00000000..91c4954b --- /dev/null +++ b/docs/reviews/CODE-REVIEW-172-r1.md @@ -0,0 +1,168 @@ +# Code review #172 — r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/172 +- **Spec:** `docs/specs/172-zero-divider-taper.md`, зелёное `SPEC-REVIEW-172-r1.md` +- **Reviewed branch:** `issue/172-zero-divider-taper` +- **Reviewed range:** `origin/dev..HEAD` = `4582628` (spec) → `c2c9c8b` (spec review doc) + → `dfd56e8` (fix) +- **Base:** `origin/dev` at `c27185c` +- **Reviewer:** Claude, независимая сессия без контекста реализации + +## Вердикт + +**Зелёный · цикл r1/4 · High: 0 · Medium: 0.** + +Дефект из #172 устранён в общей geometry-функции, подтверждён исполняемыми +юнит- и browser-smoke тестами, которые я лично прогнал и проверил на +способность падать (временный откат фикса красит именно новые проверки). +Все 11 AC закрыты — либо автотестом, либо чтением кода с явной пометкой. Две +находки Low сняты в этом документе без блокировки. + +## Скоуп + +Единственный продуктовый файл — `src/wall-thickness.ts`: `insetContour()` и +`outsetContour()` получили симметричную ветку «локальный cap», которая +перехватывает переход `положительный offset ↔ ровно нулевой offset` до общей +mitre/bevel-логики и явно сохраняет обе точки (offset-точку толстой грани и +нетронутую вершину нулевой грани), не позволяя bevel-ветке отбрасывать вершину +нулевой грани и растягивать клин вдоль всего разделителя. + +Сопутствующие изменения: `test/wall-thickness.test.mjs` (два новых теста), +`demo/smoke_zero_divider_taper.mjs` (новый, реальный Split через UI), +`demo/golden/{harness,matrix}.mjs` + `test/golden-matrix.test.mjs` (новая +визуальная сцена, `GOLDEN_MATRIX_VERSION` 24→25, baseline сознательно не +принят), `docs/WALL-THICKNESS.md` §3 (задокументирован контракт cap), +`docs/CHANGELOG.md` / `docs/CHANGELOG.ru.md`, три синхронные копии бандла. +Ровно один продуктовый коммит `dfd56e8`, трейлеры `Issue: #172` / +`User-Visible: yes` на месте, оба changelog в том же коммите. Ветка +`issue/172-zero-divider-taper` соответствует правилу именования. + +## Как проверялось + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | pass | +| `npm test` | **830/830 pass** (`npm run inventory` подтверждает канонический счётчик) | +| `npm run build` + сверка трёх копий бандла | pass, все три `sha256` = `656c68df53108…34a181`, совпадает с закоммиченным | +| Откат двух новых веток в `insetContour`/`outsetContour` и повторный `npm test` | **2 новых теста красные** (`variable-offset contours keep a local cap…`, `near-collinear zero-depth Split divider never grows a masonry taper`), остальные 828 зелёные — дисциплина «тест умеет падать» подтверждена мной, а не только автором | +| `node demo/smoke_zero_divider_taper.mjs` (назван в AC6/AC7) | pass, все 13 полей `true`; повторно собрал бандл **без** фикса и прогнал тот же smoke — `planHasNoTaper` и `lightHasNoTaper` красные, с фиксом — зелёные | +| `node demo/smoke_split_corner_wall.mjs` (смежная поверхность: mitre/bevel для двух положительных толщин, AC3/AC9) | pass | +| `node demo/smoke_wall_thickness.mjs` (смежная поверхность: общий рендер стен/проёмов) | pass | +| `node demo/smoke_wall_junctions.mjs` (смежная поверхность: T/L-стыки через ту же join-логику) | pass | +| Ручная проверка AC4 (clean-floor invariant) на точном fixture AC1 через `innerContourForRoom` + `wallBodiesGeometry`, повторённая для `0°`, `0,477°`, `0,955°`, `-0,955°` | остаточная площадь ≈0,011% от общей (39 из ~340 527 единиц), одинаковая на точном `0°` и под углом — не фикс-специфичный дефект, а ранее существующий квантование-эпсилон atomic-геометрии | + +### Не прогонялось, и почему + +- **`npm run golden:verify`.** Диффа в `demo/golden/harness.mjs`/`matrix.mjs` + добавляет новую сцену `split-zero-divider-taper-dark` без baseline — + `verify` по контракту (`demo/golden/README.md`) обязан упасть на + отсутствующем эталоне независимо от корректности геометрии. AC8 сознательно + откладывает принятие baseline на предрелизный Linux-гейт + (`golden:accept -- --reviewed`), это прямо написано в ТЗ §12 и в хендоффе. + Локальный прогон дал бы только ожидаемый «missing baseline» без новой + информации; риск регрессии существующих сцен уже закрыт тремя целевыми + smoke-тестами и полным юнит-регрессом на той же общей geometry-функции. +- **`python -m pytest tests_backend`.** Ни один файл `custom_components/**/*.py` + не тронут. +- **Performance-профили.** Не названы в AC; спецификация явно фиксирует, что + прирост вершин ограничен одной точкой на переход и не меняет асимптотику; + диффа в hot-path выше единичного `if`-ветвления нет. +- **Полный набор из 136 browser-smoke.** Задача касается одной геометрической + функции с точечным изменением контракта; прогнаны названный в AC смок плюс + три смежных (corner-split, общая толщина стен, T/L-стыки) — поверхности, + которые используют ту же `insetContour`/`outsetContour`. Остальные 132 смока + не относятся к затронутой геометрии (проёмы без стен, Glow-специфика без + стен, UI-хром и т.д.). + +## Проверка AC1–AC11 + +| AC | Метод по ТЗ | Статус | Как закрыт | +|---|---|---|---| +| AC1 | unit | ✅ | Новый тест на точном fixture §3 (`0,477°`/`0,955°`); я подтвердил, что он красный на исходном (без фикса) коде | +| AC2 | unit | ✅ | Тот же тест — матрица `h=1/15/100`, углы по обе стороны, winding-перестановка; изолированный тест `insetContour`/`outsetContour` покрывает обе последовательности `h→0`/`0→h` напрямую на примитиве | +| AC3 | unit | ✅ | Полный регресс (830/830) не покраснел; точный коллинеарный шаг и пары `1↔15`, `15↔100` — существующие тесты остались зелёными; `smoke_split_corner_wall.mjs` подтверждает facade для 0/15/100 см | +| AC4 | unit | ✅ (низкая находка, см. ниже) | Отдельного теста именно для AC1-fixture нет; я исполнил тот же helper-pipeline, что и существующий тест clean-floor (строка 936), на fixture с углом — расхождение ≈0,011%, идентичное точному `0°`, то есть не связано с фиксом | +| AC5 | unit | ✅ (чтением) | `splitRoomPath()` не тронута этим диффом; тест «rendering does not materialize or mutate saved geometry» плюс smoke-поля `anglePreserved`/`renderDoesNotRewriteConfig` эмпирически подтверждают отсутствие snap и мутации конфигурации | +| AC6 | smoke | ✅ | `node demo/smoke_zero_divider_taper.mjs` — реальный Split из вогнутого угла под ~1°, `dividerStaysZero`, `planHasNoTaper` = true | +| AC7 | smoke | ✅ | Тот же smoke: `planUsesCanonicalBody`, `planViewParity`, `kioskParity`, `isoUsesCanonicalBody`, `staticParity`, `lightHasNoTaper`, `renderDoesNotRewriteConfig` — все true | +| AC8 | golden | ✅ (отложено по контракту) | Сцена добавлена в матрицу (v25), `test/golden-matrix.test.mjs` проверяет её состав; baseline не принят — так и требуется до предрелизного Linux-гейта | +| AC9 | unit+smoke | ✅ | 830/830 + три смежных smoke зелёные, независимые тела и проёмы не меняются | +| AC10 | код-ревью | ✅ | Прочитан диф: правка только в общей variable-offset геометрии `wall-thickness.ts`, ни одного renderer-specific ветвления, схема данных не тронута | +| AC11 | код-ревью | ✅ | Прочитан диф: новых DOM-узлов/событий/таймеров/сетевых вызовов/HA-сервисов нет | + +## Находки + +### Low-1 — AC4 не имеет отдельного исполняемого теста на fixture из АК1 + +Спецификация назначила AC4 методом `unit`, но фактический diff теста +(`test/wall-thickness.test.mjs`) не содержит проверки clean-floor invariant +именно для near-collinear нулевого разделителя — только для +`dividerCm: 100` (существующий тест до этой задачи). Я закрыл разрыв +самостоятельно: исполнил `innerContourForRoom` + `wallBodiesGeometry` по тому +же fixture, что и AC1 (`[600,400]→[900,405]` и соседние углы), и получил +устойчивое расхождение ≈39 единиц из ~340 527 (≈0,011%) — идентичное значению +на точном `0°`. Поскольку расхождение не зависит от угла и воспроизводится +даже без него, это ранее существующий квантование-артефакт atomic-геометрии, +а не то, что фикс должен был закрыть и не закрыл. + +**Вердикт:** снимается без правки. Инвариант AC4 подтверждён мной прямым +исполнением (не только чтением), отдельный тест на будущее не обязателен — +дальнейшее покрытие этой границы можно adресовать при следующей правке той же +области, если она понадобится. + +### Low-2 — неверная ссылка на процесс в хендоффе при пропуске named-smoke + +Хендофф реализации указывает, что `node demo/smoke_zero_divider_taper.mjs`, +golden и performance не запускались «по принятому implementation loop +(`PROCESS.md §11.4`)». §11.4 — это исключение для починки упавших +предрелизных гейтов после `S8-merged`, а не основание пропускать smoke, +названный в AC, перед выходом в `S7-code-review`. Правило, которое реально +требует прогона таких smoke локально, — это правка `AGENTS.md` (issue #151, +раздел «Гейты»): «перед переводом issue в `S7-code-review` — прогнать смоки, +названные в её AC, локально». Golden и performance действительно откладываются +на предрелиз (это верно и без §11.4), но smoke-тест из AC6/AC7 — нет. + +**Вердикт:** снимается без правки для этого цикла. Я прогнал +`demo/smoke_zero_divider_taper.mjs` сам (см. таблицу гейтов) — тест зелёный и +подтверждённо умеет падать без фикса, так что риска для этого issue нет. +Отмечаю только неточность цитаты в хендоффе на будущее. + +## Что проверено и корректно + +- Правка симметрична для `insetContour()`/`outsetContour()`, порядок точек + (offset-точка физической грани → нетронутая вершина нулевой грани) + детерминирован для inset и outset независимо. +- Для точного коллинеарного перехода `h↔0` новая ветка производит тот же + результат, что и старая `collinearJoint`-ветка (при `oB=0` смещение `nB*0` + вырождается в исходную вершину) — регресс на этот случай логически + исключён и эмпирически подтверждён (830/830, включая существующий тест на + точную ступень). +- Переход двух положительных offset'ов не затронут: новая ветка + активируется только при строгом `(oA>0) !== (oB>0)`, что подтверждено и + чтением кода, и тремя зелёными смежными smoke. +- Локальный cap ограничен физической half-depth и не растёт пропорционально + длине разделителя — подтверждено тестом с полосой сэмплирования, + масштабируемой от `halfDepth`, для `outerCm ∈ {1,15,100}`. +- Симметрия относительно порядка комнат и winding подтверждена перестановочным + тестом (reversed room order + reversed polygon winding, area-diff = 0). +- Один канонический источник геометрии для Plan/View/kiosk/static/hidden-Iso и + light-барьеров подтверждён smoke-полями `planViewParity`, `kioskParity`, + `staticParity`, `isoUsesCanonicalBody`, `lightHasNoTaper`. +- Рендер не мутирует `rooms`/`walls` (`renderDoesNotRewriteConfig: true`, + плюс существующий unit-тест). +- Трейлеры, changelog RU/EN, `docs/specs/README.md`, `docs/WALL-THICKNESS.md` + — все в одном продуктовом коммите, соответствуют правилам PROCESS.md §7.1 + и §10.1. +- Golden-матрица и её собственный тест (`test/golden-matrix.test.mjs`) + корректно описывают новую сцену без преждевременного принятия baseline. + +## Чего не проверял + +- Визуальный итог новой golden-сцены (baseline не существует по контракту до + предрелиза — см. «Не прогонялось» выше). +- Backend/HA harness — не затронут. +- Полный набор из 136 browser-smoke и `performance_smoke` — не относятся к + этому точечному изменению; остаются обязательными на предрелизном гейте. +- Мобильный/touch путь Split отдельно не тестировал: ТЗ фиксирует, что + сохранённая геометрия не зависит от типа указателя, а сам инструмент Split + desktop-first и не менялся этим диффом.