From 6983f2b08871705bbf82e027e49b6e5bace8c5a4 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:21:07 +0000 Subject: [PATCH] docs: review document for #290 Issue: #290 User-Visible: no --- docs/reviews/CODE-REVIEW-290-r1.md | 292 +++++++++++++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-290-r1.md diff --git a/docs/reviews/CODE-REVIEW-290-r1.md b/docs/reviews/CODE-REVIEW-290-r1.md new file mode 100644 index 00000000..2d6a7bf2 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-290-r1.md @@ -0,0 +1,292 @@ +# CODE-REVIEW-290-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/290 +- **Ветка:** `issue/290-near-axis-authoring-repair` +- **Диапазон:** `origin/dev` (`523190d8`) → HEAD (`5aae905a`) +- **ТЗ:** `docs/specs/290-near-axis-authoring-and-repair.md`, ревью ТЗ зелёное на r2 (`docs/reviews/SPEC-REVIEW-290-r2.md`) +- **Заход:** r1 (первый код-ревью раунд, дельта-раздел не нужен) +- **Вердикт:** красный · High: 1 · Medium: 2 + +## 1. Скоуп диффа + +41 файл, +1966/−340. Продукт: `src/near-axis.ts` (новый общий классификатор/repair), +`src/houseplan-card.ts` (authoring hover/click/closure, Optimize dialog/report), +`src/resize.ts` (exact-axis postcondition safe Resize), `src/plan-optimizer.ts` +(lossy Optimize pass, report, идемпотентность), `src/wall-thickness.ts` (алиас +на общий порог). Тесты: `test/near-axis.test.mjs` (новый), +`test/model-invariants.test.mjs`, `test/plan-optimizer.test.mjs`, +`demo/smoke_near_axis_optimize.mjs` (новый), `demo/smoke_plan_drawing_repairs.mjs`, +`scripts/mutation-gate.mjs`, `scripts/model-invariants.mjs`, `scripts/smoke-links.mjs`. +Документация: CANVAS/RESIZE/WALL-THICKNESS/ARCHITECTURE/CONFIG-COMPATIBILITY/ +USER-GUIDE(.ru)/TESTING/CHANGELOG(.ru) — все ссылаются на #290 и соответствуют +принятым в ТЗ формулировкам. `dist`/`custom_components/.../frontend` — класс D, +байт-в-байт совпадают с пересборкой (проверено). + +## 2. Как проверялось — гейты + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | зелёный | +| unit | `npm test` | 1226 passed, 1 skipped, 0 failed (совпадает с хендоффом автора) | +| build + bundle parity | `npm run build && cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` | идентичны | +| docs fingerprint | `node scripts/check-docs.mjs` (обязателен: диф трогает `src/**`) | «Documentation checks passed (7 files, 10 external links)» | +| smoke-выборка | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | см. §2.1 | +| invariants (обязателен: диф трогает геометрию) | `npm run invariants -- --config --near-axis` | см. §2.2; литерально `--config test/fixtures/real-plan-*.json` **не работает** без обёртки — фикстура хранит `{note, space}`, а не `{spaces:[...]}`, инвариант читается только через тест/скрипт, который её оборачивает | +| golden | не запускал | диф не добавляет и не меняет ни один golden-сценарий (`demo/golden/baselines/**` не тронут); автор утверждает, что полный Linux Validate с golden прошёл на точном SHA `5aae905` — не перепроверял CI-прогон отдельно, см. §5 | +| performance | не запускал | не назван в AC как влияющий количественно; спецификация ограничивается «Optimize pass линейный», что подтверждается чтением (`repairNearAxisRoomWalls` — один проход по кандидатам без вложенных полных пересчётов) | +| backend pytest | не запускал | диф не трогает `custom_components/**/*.py` | + +### 2.1 Смок-выборка + +`node scripts/smoke-select.mjs --base origin/dev --head HEAD`: + +- **Прямое совпадение:** `smoke_decor_layer_order.mjs`, `smoke_room_resize.mjs`, + `smoke_zero_divider_taper.mjs` (символы `roomPoly`/`samePoint`, попавшие на + изменённые строки нового `near-axis.ts`, — общие имена, но прогнал все три: + зелёные, без регрессий. +- **Зарегистрированная связь:** `smoke_near_axis_optimize.mjs` (новый, + `scripts/smoke-links.mjs`) — прогнан, зелёный (см. вывод в §3). +- **Слабая связь (19, общий символ `_path`):** `smoke_plan_drawing_repairs.mjs` + прогнан (он и есть целевой AC2-смок, изменён в диффе) — зелёный. Остальные 18 + — общий геометрический путь рисования, не про near-axis; прогнал дополнительно + `smoke_wall_junctions.mjs`, поскольку `wall-thickness.ts` меняет источник + константы `MULTI_WALL_NEAR_ORTHOGONAL_MAX_DEGREES` (ровно тот класс регресса, + что #234 поймал на «непрямом» имени) — зелёный. Остальные 17 не прогонял: + тема (табы, esc-диалоги, drag жесты) не пересекается с near-axis классификатором + ни по AC, ни по изменённым символам за пределами общего `_path`. + +### 2.2 Инварианты и AC10 — реальные планы + +`npm run invariants -- --config <файл> --near-axis` ожидает форму `{spaces:[...]}`; +обе `test/fixtures/real-plan-*.json` хранят `{note, space}` (как их использует +`test/model-invariants.test.mjs` и `demo/smoke_real_plan_masonry.mjs`). Обернул +вручную и прогнал программно (не через `npm run invariants` CLI буквально, +поскольку буквальный вызов из AC10 не аппликабелен к формату этой фикстуры): + +``` +real-plan-first-floor.json: near-axis before=0 after=0, wallsStraightened=0, changed(align)=true(шум координат), 2-й проход wallsStraightened=0 +real-plan-second-floor.json: near-axis before=1 after=0, wallsStraightened=1, maxStraightenShiftCm≈1, maxStraightenSpace='floor', 2-й проход wallsStraightened=0 +``` + +Подтверждает главный продуктовый факт: `real-plan-second-floor.json` — тот +самый план с дефектом из тела issue — после Confirm получает `near-axis=0`, +не задваивает счётчик, идемпотентен. Это ровно то, что AC10 требует по +существу. Но это моя ручная проверка, **не** автотест — см. Medium M2. + +## 3. Находки + +### High H1 — Resize отключается на всей комнате из-за чужой, нетронутой near-axis стены (AC3 нарушен шире заявленного) + +**Файл:** `src/resize.ts:809-811` + +```ts +for (let index = 0; index < next.length; index++) { + if (classifyNearAxisSegment(next[index], next[(index + 1) % next.length])) return false; +} +``` + +Цикл проверяет **все** рёбра результирующего полигона `next`, а не только +moving/side edges, как формулирует ТЗ §5.2 и AC3 («Outer и exact-shared safe +drags сохраняют **moving/side edges** exact-axis»). `next` совпадает с исходным +polygon везде, кроме двух передвигаемых вершин — значит любое чужое, +нетронутое near-axis ребро где-либо ещё в том же полигоне комнаты валит всю +проверку, и `resolveSafeResize` возвращает `invalid-geometry` для **любого** +handle этой комнаты, не только для handle, связанного с дефектом. + +**Воспроизведение** (пул `test-build`, без правок продукта): + +```js +import { resolveSafeResize } from './test-build/resize.js'; +const rooms = [/* north-west, south-west, east из test/fixtures/279-near-orthogonal-junction.json */]; +resolveSafeResize(rooms, [], 'north-west', 3, opts); +// → { enabled: false, reason: 'invalid-geometry' } +``` + +Ребро 3 — верхняя, точно осевая, никак не связанная с общей near-axis стеной +(она — ребро 1) сторона комнаты `north-west`. До #290 (без нового цикла) она +резалась бы штатно: остальные проверки `validateSafeResize` (topology, area +sign, clearance, `sideAxis`) её не касаются, потому что `prev`/`next` для +ребра 3 — рёбра 2 и 0, а не дефектное ребро 1. После #290 она заблокирована. + +Это точная геометрия из тела issue #290, подтверждённая ревью ТЗ r2 как +буквально присутствующая в tracked `test/fixtures/real-plan-second-floor.json`. +Значит на **реальном плане владельца** — том самом, что стал причиной задачи — +Resize окажется отключён на каждом ребре комнат `north-west`/`south-west`, а +не только на дефектной стене, до первого запуска Optimize. Ни ТЗ, ни AC не +формулируют и не согласовывали такое расширение блокировки, владелец не +принимал этого решения. + +**Почему не пойман тестами:** ни один файл в `test/resize*.test.mjs` не +импортирует `classifyNearAxisSegment`/`NEAR_AXIS_*`; ни один +`demo/smoke_*resize*.mjs` не использует near-axis геометрию (проверено grep по +всем трём). Новый путь в `validateSafeResize` не покрыт вообще ни одним тестом +— ни AC3, ни любой другой. + +**Серьёзность:** High. Реальный регресс на данных, которые сама задача +называет мотивирующим кейсом; не согласован с владельцем; не покрыт тестом; +масштаб — вся комната, а не заявленная в AC3 пара «moving/side edges». + +### Medium M1 — AC9: 3 из 6 обязательных мутантов отсутствуют + +**Файл:** `scripts/mutation-gate.mjs:918-951` + +ТЗ AC9 требует шесть категорий мутантов: порог ниже `0.181315°`, строгое `<` +вместо inclusive, bypass authoring snap, «чинить только одного владельца общей +стены», «считать копии комнат за две стены», «применять Optimize без +подтверждения». Зарегистрированы только первые три +(`near-axis-threshold-weakened`, `near-axis-inclusive-boundary-disabled`, +`near-axis-authoring-snap-bypassed`); `docs/TESTING.md:2828+` честно +перечисляет ровно эти три, то есть сокращение осознанное, но не согласованное +с принятым ТЗ. + +Проверил по существу (не только по названию): обычные unit-тесты +(`test/near-axis.test.mjs`, тест «repairs a duplicated 316x1 physical wall +once») фактически ловят обе оставшиеся геометрические мутации — `wallsStraightened` +жёстко равен `1` (не `2`), и `north.poly[1]`/`south.poly[0]` сравниваются +явно, так что «один владелец» и «две стены вместо одной» сломали бы +существующую assertion. Функциональность цела, но формального `mutation-gate` +доказательства, которого требует принятое AC9, нет. Мутант «Optimize без +подтверждения» аналогично не зарегистрирован как патч в `mutation-gate.mjs`, +хотя `previewDoesNotWrite`/`cancelDoesNotWrite` в `smoke_near_axis_optimize.mjs` +проверяют именно это поведение. + +**В скоупе задачи** — чинится добавлением трёх отсутствующих записей в +`scripts/mutation-gate.mjs` (или явным пересмотром AC9 с владельцем, если три +достаточны). + +### Medium M2 — AC10: нет автотеста на двух реальных фикстурах + +**Файлы:** `test/near-axis.test.mjs`, `test/model-invariants.test.mjs`, +`test/plan-optimizer.test.mjs` + +AC10 (введён в ТЗ r2 специально для закрытия M2 предыдущего ревью ТЗ) требует +доказательства на `real-plan-first-floor.json` и `real-plan-second-floor.json`: +`npm run invariants` до/после подтверждённого Optimize, строгое уменьшение +near-axis count, идемпотентность повторного прогона. В диффе нет ни одного +теста, вызывающего `repairNearAxisRoomWalls`/`optimizePlans`/`nearAxisProfile` +на этих двух фикстурах — только на минимизированной +`test/fixtures/279-near-orthogonal-junction.json` +(`test/near-axis.test.mjs`, `test/model-invariants.test.mjs`) и на синтетическом +inline-конфиге (`test/plan-optimizer.test.mjs`). + +Я проверил утверждение AC10 вручную выполнением (§2.2): на текущем коде оно +верно (`real-plan-second-floor.json`: near-axis 1→0, `wallsStraightened=1`, +`maxStraightenShiftCm≈1`, идемпотентно). Дефекта в поведении нет — дефект в +покрытии: без автотеста регресс на этой конкретной реальной геометрии не +поймает `npm test`, а именно ради этого доказательства AC10 и был добавлен в +ТЗ r2. Хендофф-комментарий автора («инварианты и near-axis профиль обоих +реальных планов») не называет ни точной команды, ни файла результата — то же +самое несоответствие, что и отсутствие теста. + +**В скоупе задачи** — чинится тестом вида `#287`-теста +(`checkMixedRoleRecords({ spaces: [space] })`) с оборачиванием фикстуры и +прогоном `optimizePlans`/`nearAxisProfile` до/после. + +### Low L1 — AC1 «source guard» не механизирован + +`AC1` требует «source guard запрещает отдельные литералы `0.25` в этих +classifiers». Проверил `grep -rn "0\.25" src/*.ts` — сейчас нарушений нет +(единственные источники — `near-axis.ts` и `wall-thickness.ts`, который +импортирует константу), и `test/near-axis.test.mjs` проверяет равенство +`MULTI_WALL_NEAR_ORTHOGONAL_MAX_DEGREES === NEAR_AXIS_MAX_DEGREES`. Но +формального грep-гейта (по образцу `test/single-source-numbers.test.mjs`), +который бы упал при появлении нового хардкода `0.25` в будущем, нет. Снимаю с +записью: сегодня нарушения нет, риск регресса невелик и не блокирует эту +задачу. + +## 4. Проверено и корректно + +- **AC1** (боundary matrix) — `test/near-axis.test.mjs`, тест «near-axis + boundary is shared, inclusive and bounded»: exact axis, `316×1`, `316×2`, + ровно на границе (`NEAR_AXIS_MAX_SLOPE`), чуть выше границы, 30°-диагональ, + mirrored/reversed. Тест умеет падать — прогнал мутанты + `near-axis-threshold-weakened` и `near-axis-inclusive-boundary-disabled` + вручную (guard-команды в `mutation-gate.mjs` их и запускают) и убедился, что + именно этот тест их убивает. +- **AC2** (Walls не сохраняет `316×1`) — `demo/smoke_plan_drawing_repairs.mjs`: + hover preview и committed click дают `316×0`, existing-node snap conflict не + ворует чужой anchor (`nearAxisRuleDoesNotClaimWrongEndpoint`). Прогнал — + зелёный. Мутант `near-axis-authoring-snap-bypassed` проверен по коду + (`_resolvePlanDrawPoint` вызывает `snapNearAxisEndpoint` только когда есть + `anchor`, `effectiveCandidate` корректно гасится при переносе точки) — + проверено чтением, не исполнением отдельно от guard-команды. +- **AC4/AC5/AC6/AC7 (кроме multi-space cell_cm)** — `test/near-axis.test.mjs` + + `test/plan-optimizer.test.mjs`: `wallsStraightened=1` на трекнутой фикстуре + #279, `maxStraightenShift` точно `1/240`, обе комнаты получают идентичный + endpoint, hosted-opening конфликт корректно уходит в `skipped`, true + diagonal (`316×2`, 30°) не входит ни в `wallsStraightened`, ни в + `wallsStraightenSkipped`. Multi-space/разный `cell_cm` (AC7, вторая часть) — + в тестах нет, но проверил вручную выполнением (§2.2, конструкция с + `cell_cm=1` и `cell_cm=100` на двух копиях фикстуры): `maxStraightenShiftCm` + и `maxStraightenSpace` корректно выбирают пространство с большим физическим + сдвигом, а не с большим сырым grid-сдвигом — код в + `src/plan-optimizer.ts:448-455` масштабирует каждое пространство своим + `cell_cm` до сравнения, ошибок не нашёл. +- **AC8** (UI/Undo/revision) — `demo/smoke_near_axis_optimize.mjs`: + `previewDoesNotWrite`, `cancelDoesNotWrite`, `applyUsesOneAtomicWrite`, + `reloadIsIdempotent`, `undoRestoresExactGeometry`, + `undoServerSnapshotIsByteExact`, `undoIsOneDeep` — все зелёные. Отдельного + golden-скрина light/dark для новой строки диалога нет (ни один + golden-сценарий не использует near-axis геометрию), но текст строки — + тема-независим (одна и та же Lit-разметка, разница только в CSS), поэтому + функциональной проверки `previewNamesLossyRepair` достаточно — проверено + чтением шаблона диалога (`src/houseplan-card.ts:16436-16441`). +- **«Одно число, один источник»** — `maxStraightenShiftCm` считается один раз + в `plan-optimizer.ts` (масштабирование через `cell_cm` конкретного + пространства) и передаётся в отчёт; UI (`straightenCm = + Math.ceil(r.maxStraightenShiftCm * 10) / 10`) только форматирует, не + пересчитывает. `wallsStraightened`, показанный в отдельной строке диалога, и + тот же счётчик, свёрнутый в агрегат `m` тоста `gs.align_done`, — это одна и + та же исходная величина, просуммированная с другими подобными счётчиками + (тот же паттерн, что у `openingsRehosted` и остальных), а не два независимых + вычисления одной и той же вещи — не является нарушением правила. + `test/single-source-numbers.test.mjs` прошёл (это механическая часть + правила; смысловую разобрал вручную выше). +- **`changed` не даёт ложных `wallsStraightened`** — `plan-optimizer.ts:645-646` + сравнивает итоговый JSON конфига/layout с исходным; `wallsStraightened`, + `wallsStraightenSkipped` и `maxStraightenShiftCm` зависят от `changed` + (строки 690-693), так что «исправлено N стен» не может появиться без + реального изменения записи. +- **Idempotency** — второй `optimizePlans` на уже выпрямленной геометрии даёт + `wallsStraightened=0`, `changed=false` — проверено и в unit-тесте, и вручную + на обоих реальных планах (§2.2). +- **Документация** — CANVAS/RESIZE/WALL-THICKNESS/ARCHITECTURE/CONFIG- + COMPATIBILITY/USER-GUIDE(.ru)/TESTING/CHANGELOG(.ru) читал полностью; + терминология («выпрямлено стен», «максимальное перемещение») совпадает + между кодом, i18n и `USER-GUIDE.ru.md`; трейлеры коммитов не проверял + отдельно (см. §5), но `User-Visible: yes` подтверждён правками в обоих + changelog в дифф-стате. +- **typecheck/build/bundle parity/check-docs** — зелёные (§2). + +## 5. Чего не проверял + +- **Полный Linux CI Validate на SHA `5aae905`** (golden, `docs`, `hacs`, + `hassfest`, `smoke`, `performance_smoke`, `backend`) — не переоткрывал сам + прогон через `gh`, доверился ссылке на зелёный run в хендоффе автора и + собственному локальному повтору дешёвых гейтов + целевых смоков. Если нужна + дополнительная гарантия — попросить точную ссылку на run id. +- **Полный набор `demo/smoke_*.mjs`** (182 файла) — прогнал только выбранные + скриптом `smoke-select.mjs` плюс `smoke_wall_junctions.mjs` по прецеденту + #234; полный прогон — предрелизный гейт (PROCESS.md §8), не гейт ревью. +- **`npm run golden:verify` и performance-профили** — не запускал; диф не + добавляет golden-сценариев и не называет performance-бюджет в AC явно. +- **`python -m pytest tests_backend`** — не запускал; диф не трогает Python. +- **Touch/pinch/pointercancel** — не гонял вручную в браузере; проверено + чтением: `_resolvePlanDrawPoint`/`snapNearAxisEndpoint` не завязаны на тип + указателя, а pinch/pan обрабатываются раньше и не доходят до этого пути + (ES5-уровня рефакторинг не менял pointer-разбор). +- **Коммит-трейлеры и провенанс** (`Issue:`/`User-Visible:` на каждом коммите + диапазона) — не проверял поштучно через `git log`; `pre-push`/`provenance` + CI job должны были бы поймать нарушение раньше ревью. + +## 6. Итог + +High H1 — реальный, воспроизведённый регресс Resize на геометрии, ради +которой заведена задача, вне заявленного в AC3 объёма и без покрытия тестом. +Medium M1/M2 — согласованные в ТЗ доказательства (AC9, AC10) реализованы не +полностью; функционально то, что можно проверить вручную, работает верно, но +автоматическое доказательство отсутствует. Возврат автору: сузить проверку в +`validateSafeResize` до moving/side edges (или явно вынести решение +«вся комната блокируется» на согласование с владельцем, если это осознанный +выбор), добавить три недостающих мутанта AC9 и тест AC10 на обеих реальных +фикстурах.