From aa85a95b32882555a46b9947cafffdf6d18fc4ec Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 11:38:37 +0000 Subject: [PATCH] docs: review document for #258 Issue: #258 User-Visible: no --- docs/reviews/CODE-REVIEW-258-r1.md | 204 +++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-258-r1.md diff --git a/docs/reviews/CODE-REVIEW-258-r1.md b/docs/reviews/CODE-REVIEW-258-r1.md new file mode 100644 index 00000000..46e9b55d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-258-r1.md @@ -0,0 +1,204 @@ +# CODE-REVIEW-258-r1 + +- Issue: [#258](https://github.com/Matysh/houseplan-card/issues/258) — «На 1.67.0-beta.4 после «Оптимизировать» появились белые клинья в местах схода стен» +- Ветка `issue/258-wall-key-storage-roundtrip`, коммит `28eaf86662819651b2c75e7423280f10820a757d` +- Этап: code (PROCESS.md §2.7) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- Вердикт: **жёлтый** + +## Скоуп + +ТЗ `docs/specs/258-wall-key-storage-roundtrip.md` описывает регресс: `wallKey()` +квантует середину стены через `Math.round`, и у стены нечётной длины в шагах +решётки середина попадает точно на границу округления. Точное узловое +представление вершины (`83/240`) и её девятизнаковое persisted-представление +(#224) дают два разных `key` для одного и того же ребра; `lookupWall()` +находит запись только по точному совпадению строки или терпимому запасу +ровно в полшага, а `thicknessCmAt()` — ещё и по точным `a/b` через +`exactCoveringWall()`. Реализация стабилизирует генерацию `key` near-grid +нормализацией endpoint'ов, добавляет строгий same-span lookup по точным `a/b` +между exact-key и legacy midpoint fallback, и пересобирает диагностику +`checkWallKeys` в `scripts/model-invariants.mjs`. + +**Важное уточнение хронологии, которое меняет оценку скоупа.** Между +утверждением ТЗ (spec-review, зелёный, `docs/reviews/SPEC-REVIEW-258-r1.md`) +и завершением реализации владелец сам проверил исходный диагноз исполнением +на своих реальных экспортах и +[отозвал его](https://github.com/Matysh/houseplan-card/issues/258#issuecomment-5385705030): +переписанные Optimize ключи не меняют `wallIntervals`/`wallEdgeBodies`/ +`buildMultiWallNodeMap` — тела стен и карта узлов побайтово идентичны +независимо от того, какой из двух ключей записан. Реальная причина белых +клиньев на скриншотах владельца осталась неизвестной; названы два других +следа (рост `wall-thickness.ts` на 760 строк в beta.4 от #249/#253, и +сплошное ребро без записи толщины, существовавшее уже на beta.3). Реализация +всё равно доведена по первоначальному контракту ТЗ — это осмысленное решение +(правит реальный, отдельно доказанный дефект класса «одно ребро — разные +ответы у потребителей»), но релиз-артефакты этой задачи заявляют, что именно +зарепорченный симптом устранён. Разбор ниже отдельно оценивает код (полностью +корректен по контракту ТЗ) и это продуктовое несоответствие. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` §2.7/§2.10/§7–§8/§12, + `docs/WALL-THICKNESS.md`, `docs/CONFIG-COMPATIBILITY.md`, + `docs/ARCHITECTURE.md` (участок Room Resize/wall key), тело issue #258 и + **все** комментарии, включая отзыв диагноза и хендофф от 2026-08-23, и + ТЗ/спек-ревью документ. +2. Первый заход код-ревью — разбор полный, без «дельты» (§2.10 неприменим). +3. `git diff origin/dev...HEAD --stat` — 25 файлов; продуктовый код — + только `src/wall-thickness.ts` (+40/-…). Остальное: тесты, `scripts/**` + (class B), `docs/**` (class C), `dist/**` + + `custom_components/houseplan/frontend/**` (class D, сгенерированы). +4. Прочитан весь diff `src/wall-thickness.ts`: `keyEpsilon()`, + `canonicalKeyCoordinate()`, изменённый `wallKey()`, изменённый + `lookupWall()` (новый same-span шаг между exact-key и legacy fallback). + Отдельно проверено, что `thicknessCmAt()` (:319) и `cmsForPoly()` (:1366) + оба вызывают именно этот `lookupWall()` — то есть AC2/AC3 «один resolved + wall entry для всех структурных потребителей» действительно следует из + единой точки входа, а не из совпадения двух отдельных формул. +5. Прочитан diff `scripts/model-invariants.mjs`, `scripts/mutation-gate.mjs`, + `scripts/smoke-links.mjs`, все новые/изменённые тесты + (`test/wall-thickness.test.mjs`, `test/plan-optimizer.test.mjs`, + `test/model-invariants.test.mjs`, `test/golden-matrix.test.mjs`, + `demo/golden/harness.mjs`, `demo/golden/matrix.mjs`, + `demo/smoke_wall_key_roundtrip.mjs`), документация + (`docs/WALL-THICKNESS.md`, `docs/ARCHITECTURE.md`, + `docs/CONFIG-COMPATIBILITY.md`, `docs/TESTING.md`, оба changelog). +6. Прогнаны гейты (таблица ниже) и три новых/изменённых mutation-анкера + отдельно, чтобы убедиться, что связанные с ними тесты умеют падать. + +### Гейты + +| Гейт | Команда | Результат | +|---|---|---| +| Typecheck | `npx tsc --noEmit` | зелёный | +| Unit | `npm test` | 1159 passed, 0 failed, 0 skipped | +| Build + bundle parity | `npm run build && npm run bundle:sync` + `cmp` трёх копий | `dist/houseplan-card.js` = `custom_components/houseplan/frontend/houseplan-card.js` = `demo/srv/assets/houseplan-card.js` | +| Docs fingerprint | `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 10 external links)» | +| Smoke selection | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 173 смоков в матрице; 2 «зарегистрированные связи»: `smoke_resize_wall_thickness.mjs` (← `closePoint`, `wallDir`), `smoke_wall_key_roundtrip.mjs` (← `canonicalKeyCoordinate`, `keyEpsilon`) — обе прогнаны | +| Целевые смоки | `node demo/smoke_wall_key_roundtrip.mjs`, `node demo/smoke_resize_wall_thickness.mjs` | оба `OK`, все под-проверки `true` | +| Model invariants | часть `npm test` (`test/model-invariants.test.mjs`, включает parity-тест копии `wallKey` и фикстур-sweep) | зелёный; отдельный `npm run invariants -- --config <файл>` не запускался — diff не даёт нового полного экспорта конфигурации сверх уже покрытых unit-фикстур, а `npm test` уже гоняет полную проверку по всем моделям проекта (демо-фикстуры + demo-стенд) | +| Mutation-анкеры (новые/изменённые) | `node scripts/mutation-gate.mjs --id=wall-key-storage-normalization-disabled` / `--id=wall-exact-span-fallback-disabled` / `--id=invariant-wall-key-storage-normalization-disabled` / `--id=invariant-keys-cry-wolf` / `--id=invariant-keys-hide-stale-observation` | все 5: «тест покраснел, как обязан» — тесты действительно умеют падать | +| Golden | часть `npm test` (`test/golden-matrix.test.mjs`); `npm run golden:verify` (браузерный рендер) не запускался | сценарий `wall-key-roundtrip-view-dark` зарегистрирован и покрыт unit-проверкой контракта; baseline не принимался (правильно — только через полный Linux CI artifact) | +| Backend | — | не запускался: diff не трогает `custom_components/**/*.py` | +| Performance | — | не запускался: AC не называет перф-эффект, спецификация явно фиксирует `O(1)`/`O(W)` без изменений | + +**Не прогнано и почему:** полный browser-`golden:verify` и полный набор из 173 +смоков — задача касается одного узкого T-стыка и явно выбранных потребителей; +`smoke-select` назвал ровно два релевантных смока, оба прогнаны, остальные 171 +не относятся к диффу и полный прогон — предрелизный гейт, не гейт ревью. +`pytest tests_backend` — diff не трогает Python. Отдельный `npm run invariants +-- --config <файл>` на внешнем экспорте — не требовался: diff не добавляет +новую геометрию, которую не покрыли бы уже прогнанные unit-фикстуры и полный +sweep внутри `npm test`. + +## Проверка AC + +| AC | Статус | Как доказано | +|---|---|---| +| AC1 (один key для exact/9-знака, odd/even, ±, reversed) | Выполнен | `test/wall-thickness.test.mjs` «issue 258 wallKey survives...» — 4 кейса + негативный тест на epsilon-границу; mutant `wall-key-storage-normalization-disabled` убивается | +| AC2 (строгий same-span lookup, без parent/child/neighbour/parallel) | Выполнен | `test/wall-thickness.test.mjs` «issue 258 exact-span lookup...» — оба варианта ключа находят 29 см, явные негативные кейсы (`broken-parent/child/neighbour/parallel`) не принимаются; render-space (`coordScale`) тоже проверен; mutant `wall-exact-span-fallback-disabled` убивается | +| AC3 (Optimize канонизирует key, preview не пишет, идемпотентность после roundtrip) | Выполнен | `test/plan-optimizer.test.mjs` «issue 258 Optimize canonicalizes...» — deepEqual на preview, `changed:true`/`canonicalized:1` на первом Apply, `changed:false` на in-memory и backend-echo повторе. Код в `plan-optimizer.ts` не менялся: `rekeyWallsAfterMove()`/canonicalизация уже вызывают исправленный `wallKey()` — прочитано и подтверждено, отдельная вторая формула не добавлена | +| AC4 (инвариант сообщает оба mismatch, парность с production `wallKey`) | Выполнен, но **изменена формулировка относительно ТЗ §8 — явно заявлено автором** | `checkWallKeys()` больше не возвращает `violations` (массив всегда пуст) — несовпадение репортится только как `notes`/observation. Автор прямо попросил ревьюера отдельно оценить это отступление. Технически обоснованно: после фикса `lookupWall()` резолвит любую запись с валидными `a/b` независимо от старого key через same-span шаг, поэтому «нарушение» никогда не воспроизводимо для записи с координатами — а `checkWallKeys` как «hard violation» немедленно покрасил бы `demo/fixtures/large-house.mjs`/`visual-matrix.mjs`, чьи ключи не по контракту по другой причине (заведено отдельно, #260). Даунгрейд подтверждён тестом «#258: старый и неразбираемый compatibility key — наблюдение» и двумя mutation-анкерами (`invariant-keys-cry-wolf`, `invariant-keys-hide-stale-observation`), оба убиваются. Принимаю как согласованное с текущим `dev` решение, а не как найденную находку | +| AC5 (T-node без клина во всех потребителях до/после Optimize/reload) | Выполнен для минимизированного T-fixture из issue | `demo/smoke_wall_key_roundtrip.mjs` — прогнан, `OK`, 24 под-проверки `true` (Plan/View/kiosk/Static/hidden-Iso/clean-floor/light-barrier × canonical/affected); unit `test/wall-thickness.test.mjs` «issue 258 repaired span reaches intervals, junction nodes and masonry» — реальная геометрическая проба `assertProbeInside` | +| AC6 (golden light/dark T-node) | Выполнен по прецеденту проекта | Сценарий `wall-key-roundtrip-view-dark` добавлен и покрыт `test/golden-matrix.test.mjs`; только dark-вариант — совпадает с прецедентом трёх похожих T-стыковых сценариев (#249/#253: `wall-junctions-view-dark`, `junction-patch-resilience-view-dark`, `multiwall-junction-bevel-view-dark`), которые тоже dark-only; baseline не принимался локально (правильно) | +| AC7 (регрессии: legacy key-only, parent inheritance, #253, #249, #248, openings, two-ray) | Выполнен | Полный `npm test` зелёный (1159/1159), включая все именованные регрессионные наборы; новых провалов не внесено | +| AC8 (implementation loop + provenance) | Выполнен | `typecheck`/`test`/`build`/parity/`check-docs` — все зелёные (см. таблицу гейтов); коммит несёт `Issue: #258` / `User-Visible: yes`; оба changelog правлены в том же коммите | + +## Находки + +### Medium (в скоупе задачи — правится в ней же) + +**Release-артефакты заявляют больше, чем доказано, и противоречат собственному +расследованию владельца в этом же issue.** + +- Файл: `docs/CHANGELOG.md` (и `docs/CHANGELOG.ru.md`) +- Итог: «Thick-wall T-junctions no longer develop white wedges after + “Optimize plans”» / «После «Оптимизировать планы» в T-образных стыках + толстых стен больше не появляются белые клинья» — безусловное утверждение, + что именно зарепорченный в #258 симптом устранён. +- Сценарий, в котором это ломается: владелец обновляется на бету с этим + коммитом, открывает свой реальный план (тот самый, со скриншотов), нажимает + «Оптимизировать» — и, по его же собственному измерению + ([комментарий](https://github.com/Matysh/houseplan-card/issues/258#issuecomment-5385705030)), + `wallEdgeBodies`/карта узлов для его двух реальных пространств не меняются + ни на бит между старым и новым кодом ключа. Если клинья на его скриншотах + вызваны не этим (а иным следом — код #249/#253 или ребро без толщины, + подробности в новом [#261](https://github.com/Matysh/houseplan-card/issues/261)), + changelog утверждает то, что не подтверждено, и создаёт основание закрыть + #258 без бета-регрессии, хотя визуальный дефект, из-за которого issue был + заведён, может остаться на месте. +- Почему это в скоупе и не High: код и все AC1–AC8 корректны и полностью + доказаны — правки не требуют переписывания реализации, только формулировки + release-артефактов (§13 ТЗ прямо включает их в DoD задачи). Достаточно + переформулировать записи `CHANGELOG.md`/`.ru.md` так, чтобы они описывали + фактически исправленный дефект («несогласованность потребителей толщины + стены при устаревшем/пересчитанном compatibility-key после Optimize»), а не + безусловно закрывали зарепорченный визуальный симптом. Тот же нюанс стоит + отразить и в финальном комментарии/закрытии #258 — новый + [#261](https://github.com/Matysh/houseplan-card/issues/261) уже открыт для + отдельного расследования реальной причины. + +### Low (снято с записью) + +- `scripts/model-invariants.mjs`, `titles` внутри `report()`: ключ `wall_key: + 'Записи толщины, которые не найдутся по ключу'` теперь мёртв — `wall_key` + как `kind` violation больше никогда не push'ится (см. AC4 выше, `violations` + для этого инварианта теперь всегда `[]`). Не влияет на поведение — вывод + просто никогда не использует эту строку. Не блокирую: тривиальная + косметика, безопасно оставить или почистить в следующей правке того же + файла. + +## Что проверено и корректно + +- Единая точка генерации key (`wallKey`) и единая точка резолва (`lookupWall`) + — оба структурных потребителя (`thicknessCmAt`, `cmsForPoly`) проходят + через один и тот же исправленный `lookupWall()`; вторая формула нигде не + завелась. +- Near-grid нормализация консервативна: epsilon строго на порядки меньше + половины шага решётки, легитимная off-grid геометрия не снапится + (проверено тестом и mutant'ом). +- Same-span резолв — действительно same-span, не containment: negative-кейсы + на parent/child/neighbour/parallel явно протестированы и не проходят. +- Optimize канонизирует ключ через уже существующий код (`rekeyWallsAfterMove` + импортирует и использует исправленный `wallKey`), отдельной второй формулы + оптимизатора не появилось; preview не мутирует, повторный вызов и + backend-echo — no-op. +- Копия формулы в `scripts/model-invariants.mjs` прикреплена parity-тестом к + продуктовой `wallKey` (как и предписывало ТЗ и практика #233/#234) — + расхождение будет обнаружено автоматически. +- Три новых и два переработанных mutation-анкера действительно убивают + соответствующие правки (проверено запуском каждого). +- Bundle parity (`dist` = `custom_components/.../frontend` = `demo/srv/assets`) + и docs-fingerprint — в порядке; трейлеры коммита корректны, оба changelog + правились в одном коммите с кодом. +- AC7 (регрессии #248/#249/#253/legacy key-only/two-ray/openings) не сломаны — + полный `npm test` зелёный без единого нового провала. + +## Чего не проверял + +- Полный `npm run golden:verify` (браузерный рендеринг golden) и полный набор + 173 смоков — не требуется объёмом задачи; ограничился двумя смоками, + названными `smoke-select`, плюс уже пройденной unit-частью golden-контракта. +- `python -m pytest tests_backend` — diff не касается `custom_components/**/*.py`. +- Perf-профили — AC не называет влияние на перф, а спецификация фиксирует + неизменную асимптотику; не проверял исполнением, принял по чтению кода + (линейный `lookupWall`, ни одного нового прохода по комнатам). +- Реальную причину белых клиньев на исходных скриншотах владельца — это вне + моей роли ревьюера кода этой задачи; зафиксировано находкой выше и новым + [#261](https://github.com/Matysh/houseplan-card/issues/261). + +## Итог + +Код и тесты для заявленного в ТЗ контракта (стабильный `wallKey`, строгий +same-span `lookupWall`, канонизация через Optimize, пересмотренная +диагностика инварианта) — корректны, полны и доказаны исполнением, включая +работающие mutation-анкеры. Единственная находка — Medium, в скоупе, +касается не кода, а точности формулировки release-артефактов, которые +безусловно заявляют устранение зарепорченного в #258 визуального симптома, +хотя собственное расследование владельца в этом же issue показывает, что +исправленный дефект не объясняет этот симптом на его реальных данных. Это +ровно случай «AC выполнены, но изменение не решает заявленный сценарий» — +жёлтый вердикт, возврат автору для правки формулировок в `docs/CHANGELOG.md` +и `docs/CHANGELOG.ru.md` (и, по желанию автора, уточнения в самом issue перед +закрытием).