diff --git a/docs/reviews/CODE-REVIEW-313-r1.md b/docs/reviews/CODE-REVIEW-313-r1.md new file mode 100644 index 00000000..26ee14b4 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-313-r1.md @@ -0,0 +1,293 @@ +# CODE-REVIEW-313-r1 + +Issue: #313 · этап: код (code) · трек: `small` (лёгкий, лимит циклов код-ревью — 2) · +заход: r1 · блокирующих циклов израсходовано: 0 из 2 · SHA материала ревью: `5e5dad277cbdc238a5af899ff6727aed60d604b1` + +## Скоуп + +Ветка `issue/313-wallthick-standalone`, один коммит `5e5dad27` поверх `dev`. +Конвейер привёл ветку к `dev` до ревью (§7.2/#257): поверх лёг 1 коммит `dev` +(`docs: review document for #313`, `ffe0f0c7` — сам SPEC-REVIEW-313-r1, уже +принятый и слитый). Это не смысловой ребейз продуктового кода — единственный +родительский коммит между старым и новым состоянием документ-only, диапазон +`origin/dev...HEAD` по-прежнему ровно один коммит `5e5dad27`. Разбор в любом +случае полный: это первый заход код-ревью (`r1`), раздел «Унаследовано из +r(N-1)» неприменим — предыдущего раунда код-ревью не было. + +Реализует ТЗ из тела issue #313 (light-трек, SPEC-REVIEW-313-r1 зелёный): +инструмент «Толщина» резолвит/пишет также перегородки (`space.partitions`) и +сегменты сохранённых черновиков (`space.room_drafts[]`), с приоритетом +независимой кладки при точном наложении, без кнопки «на всю комнату» для неё, +с отказом нуля/пусто для независимой кладки. + +Диф: `src/houseplan-card.ts` (+109/-…), новый смок +`demo/smoke_wallthick_standalone.mjs`, второй патч мутанта +`wall-thickness-writer-bypasses-common-barrier` в `scripts/mutation-gate.mjs`, +оба CHANGELOG, `docs/USER-GUIDE.ru.md`, `docs/images/screenshots.json` +(только fingerprint), три копии бандла. + +## Как проверялось + +Читал: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (§1–§12), тело issue #313 и +все 3 комментария (аналитика с тремя решениями владельца, зелёный вердикт +SPEC-REVIEW, хендофф автора), `docs/WALL-THICKNESS.md` целиком (§1, §6, §9 +особенно), срез `docs/USER-GUIDE.ru.md` по «Толщине», `SPEC-REVIEW-313-r1.md`. + +Код читался на `HEAD @ 5e5dad27`, полный `git diff origin/dev...HEAD`. Ключевые +утверждения не приняты на слово, а **исполнены**: + +### Гейты — прогнал + +| Гейт | Команда | Результат | +|---|---|---| +| typecheck | `npx tsc --noEmit` | зелёный, без вывода | +| unit | `npm test` | 1335 pass / 1 skip / 0 fail (1336 total) — совпадает с хендоффом | +| build + bundle sync | `npm run build && npm run bundle:sync` | `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` → совпадают; `cmp dist/houseplan-card.js demo/srv/assets/houseplan-card.js` → совпадают | +| docs fingerprint | `node scripts/check-docs.mjs` (diff трогает `src/**`, гейт обязателен) | «Documentation checks passed (7 files, 10 external links)» | +| выборка смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | 25 «прямое совпадение»; из них прогнал 5 по существу диффа (ниже) | +| `demo/smoke_wallthick_standalone.mjs` (новый, AC1–AC4) | `node demo/smoke_wallthick_standalone.mjs` | OK — **и подтверждено, что тест умеет падать**: собрал бандл с того же файла на `origin/dev` (`ffe0f0c7`, чистый worktree, свой `node_modules` симлинком) — без правки `_wallThickHit`/`_wallThickApply`/`_wallDialog.source` смок бросает необработанное исключение (`TypeError` в `_wallThickApply`), а не тихо проходит | +| `demo/smoke_wall_thickness.mjs` (AC4, комнатные стены) | `node demo/smoke_wall_thickness.mjs` | OK, все под-проверки true | +| `demo/smoke_wallthick_hover_width.mjs` (AC4) | `node demo/smoke_wallthick_hover_width.mjs` | OK | +| `demo/smoke_resize_wall_thickness.mjs` (AC4) | `node demo/smoke_resize_wall_thickness.mjs` | OK | +| `demo/smoke_optimize_coincident_partition.mjs` (тематически смежный с AC3 — #308-сценарий, два прямых совпадения символов `_wallDialog`/`_wallThickApply`, не «слабая связь») | `node demo/smoke_optimize_coincident_partition.mjs` | OK, все под-проверки true, включая `thicknessToolSelectsCanonicalWall`/`thicknessChangesSingleBody` | +| мутация (точечно, не полный набор) | `node scripts/mutation-gate.mjs --id=wall-thickness-writer-bypasses-common-barrier` | «поймано 1 из 1» — но см. находку Medium ниже: результат обманчив | +| инварианты модели | `npm run invariants -- --config test/fixtures/276-coincident-partition.json` | 1 нарушение — **предсуществующее и ожидаемое** (та же fixture #276/#308: перегородка поверх ребра комнаты; не следствие этого диффа — `plan-geometry-preflight.ts`/`near-axis.ts`/`coordinate-canonicalization.ts`, которые видит скрипт, диффом не тронуты). Не запускал отдельный export с реальной записью толщины независимой кладки: диф не меняет `wall_segments[]`/ключи/`open_spans`, пишет только `partition.cm`/`draft.segments[i].cm` тем же способом, что уже существующий `_savePhysicalDialog` (houseplan-card.ts:8287-8300, select-инструмент) — не новая форма мутации модели | + +Остальные 20 «прямых совпадений» от smoke-select — не прогонял: связь через +широко переиспользуемые символы (`_activeDraftId`, `_geometrySnapshot`, +`_showToast`, `wallIntervals`) без содержательного пересечения с изменённым +кодом (`_wallThickHit`/`_wallThickApply`/`_wallDialog`/шаблон диалога); полный +прогон — предрелизная обязанность, не гейт этого ревью. +`golden`/`pytest tests_backend`/perf — не запускал: диф не меняет визуальный +результат (нет новых компонентов рендера — существующая полоса hover и диалог +переиспользуются как есть) и не трогает `custom_components/**/*.py` или +performance-чувствительные пути; в AC они не названы. + +### AC — как доказаны + +- **AC1** (перегородка: hit → диалог → запись `partition.cm` → тело + перерисовывается → Undo). Доказано смоком `smoke_wallthick_standalone` + (hit/диалог/запись) плюс моей отдельной проверкой исполнением — Undo смок + **не тестирует** (комментарий в файле сам это признаёт: «server history is + async-free here» без единого вызова undo). Я воспроизвёл отдельно: + `_wallThickClick` → правка на 35 → `_wallThickApply` → `Control+z` → значение + вернулось к исходным 20, `_geometryHistory.size === 1` (один Undo, как того + требует контракт). AC1 закрыт. +- **AC2** (сегмент драфта: диалог с cm сегмента, запись `draft.segments[i].cm`, + активный драфт не виден). Смоком подтверждена happy-path запись (12 → 18) и + исключение активного драфта — по коду (`draft.id === this._activeDraftId + continue`, houseplan-card.ts:11971-11973). **Не подтверждена находкой ниже** — + см. High: для `segments[i].cm === 0` диалог показывает не cm сегмента, а + выдуманное значение. Формально AC2 просит «диалог с cm сегмента» — для этого + конкретного, документами же допускаемого состояния контракт не выполнен. +- **AC3** (приоритет наложения; ноль/пусто отказ). Смок подтверждает обе части + на #308-фикстуре (перегородка `cm:30` поверх ребра комнаты `left`) и отказ + нуля с сохранением значения и открытым диалогом. Дополнительно прогнал + `smoke_optimize_coincident_partition` — тот же топологический класс + («перегородка поверх ребра») с независимой (не игрушечной) фикстурой, + `thicknessToolSelectsCanonicalWall`/`thicknessChangesSingleBody` зелёные. + AC3 закрыт. +- **AC4** (комнатные стены не регрессируют). Три названных смока + + `npm test` (1335/1335, включая ~200 юнитов #278/#282 wall-model-barrier) — + все зелёные, golden не переприёмывался (диф не меняет рендер-геометрию комнат + — только резолвер инструмента и рендер диалога). Код `wallIntervals(...)` в + комнатной ветке резолвера байтово не менялся (только обёрнут в `offer(..., + false)` — при отсутствии независимых кандидатов `offer` воспроизводит старое + сравнение `d < best.d` строго, тай-брейк по независимости не участвует). AC4 + закрыт. + +## Находки + +### High — 1 + +**Смешение «сегмент отсутствует» и «cm сегмента равен нулю» в резолвере +драфтов; следствие — тихая порча данных при клике «Применить» без правки +поля.** + +`src/houseplan-card.ts:11976`: +```ts +open: false, cm: Number(draft.segments[i]?.cm) || 15, +``` +`RoomDraftCfg.segments[i].cm` — обязательное `number` (`src/types.ts:60`), 0 — +легитимное для проекта значение того же самого поля: `docs/WALL-THICKNESS.md` +§6 прямо документирует «Empty / 0 leaves the new wall thin» для сегментов +черновика, и соседняя функция `validCm` (`src/houseplan-card.ts:13447-13448`, +`value >= 0 ? value : null`) в этом же файле трактует 0 как валидное **текущее** +значение того же поля при трассировке уже сохранённой независимой кладки — +разработчик уже один раз учёл это же поле как «0 — не отсутствие», просто в +другом методе. `||` в новом коде не различает «сегмента с таким индексом нет» +(законная защита от рассинхрона `points`/`segments`) и «сегмент есть, cm = 0» +— оба варианта схлопываются в `15`. + +**Воспроизведение (исполнением, не догадкой)** — синтетический конфиг, +`page.evaluate` на собранном бандле HEAD (`demo/serve.mjs`): +```js +room_drafts: [{ id: 'thin-draft', points: [[0.6,0.5],[0.9,0.5]], segments: [{ cm: 0 }] }] +// tool = 'wallthick' +_wallThickHit([0.75*NORM_W, 0.5*NORM_W]).cm // → 15 (истинное значение — 0) +_wallThickClick(...) +_wallDialog.value // → "15" (должно быть "" — cmToField(0,…) === '') +_wallThickApply(false) // пользователь просто закрывает диалог, ничего не меняя +serverCfg.spaces[0].room_drafts[0].segments[0].cm // → 15 (было 0!) +``` +Результат — ровно то, что напечатал скрипт: +`{"hitCm":15,"trueStoredCm":0,"dialogValue":"15","cmAfterApplyWithoutEdit":15}`. + +**Сценарий отказа.** Черновик, нарисованный «тонким» (без толщины) — состояние, +которое канон подсистемы называет существующим («Empty / 0 leaves the new wall +thin»), не рисуется в редакторе как обычная стена по толщине, но геометрически +существует. Владелец открывает «Толщину», наводит на этот сегмент, видит +полосу подсветки шириной 15 см (реального 0 не видно — тоже следствие того же +`hit.cm`), кликает, видит в поле «15», нажимает Enter/Apply, думая, что просто +закрывает диалог или подтверждает увиденное — сегмент необратимо (без второго +явного намерения) становится физической стеной 15 см. Это ровно класс «одно +число — один источник» (PROCESS.md §8): подсветка инструмента, поле диалога и +итоговая запись показывают одно и то же неверное число, полученное из одного +неверного источника (`_wallThickHit`), а не три независимых. + +Партиционная ветка той же строки (`Number(partition.cm) || 0`, houseplan-card.ts:11963) +не страдает: 0 — единственное значение, при котором фолбэк вообще участвует, и +он совпадает с истинным (для `PartitionCfg` нет задокументированного «тонкого» +состояния и нет пути создания `cm: 0`, в отличие от драфтов). + +**В скоупе ли находка.** Да: это тот же файл, тот же метод, что ТЗ описывает +(«Диалог: для перегородки/драфта — cm сегмента»), правка — одна строка. + +### Medium — 1, в скоупе + +**Второй патч мутанта `wall-thickness-writer-bypasses-common-barrier` не +проверяет то, что заявлено в коммите; заявление «краснота обоих проверена +исполнением» не подтверждается.** + +Коммит `5e5dad27` добавляет второй `patches[]`-элемент в +`scripts/mutation-gate.mjs:1221-1227`, целящий именно новую точку коммита +(`_wallThickApply`, независимая кладка) — и заявляет в теле коммита: «Мутант +расширен вторым патчем на новую точку коммита (#278-гвард), краснота обоих +патчей проверена исполнением». Проверил раздельно, применяя патчи мутанта +по одному к `src/houseplan-card.ts` (с восстановлением файла после каждой +проверки, `git status` чист): + +- только 1-й патч (легаси-точка, `history.wall_thickness` в комнатной ветке) + применён → `node --test --test-name-pattern="production source routes + physical writers" test/wall-union-isolation.test.mjs` **красный** (как + ожидается); +- только 2-й патч (новая точка, независимая кладка) применён, легаси-точка + цела → тот же тест **зелёный** — не ловит. + +Причина: гвард-тест (`test/wall-union-isolation.test.mjs:107`) — это +регэксп-поиск текста `_commitPhysicalGeometry\(this\._t\('history\.wall_thickness'` +по всему исходнику одной строкой (без `s`/multiline-режима на `.`), а не +проверка конкретного места. Он совпадает **только** с легаси-вызовом +(написан в одну строку), потому что новый вызов +(houseplan-card.ts:12064-12066) обёрнут переносом строки сразу после +`_commitPhysicalGeometry(` — до `this._t(...)` в исходнике перевод строки, +через который движковый `.` без флага `s` не проходит. Поэтому при +`--id=wall-thickness-writer-bypasses-common-barrier` (оба патча вместе, +как в проверил и в реальном CI-запуске мутанта) тест краснеет **целиком за +счёт первого патча**; второй патч можно откатить, ничего не изменится в +результате — «поймано 1 из 1» в отчёте `mutation-gate.mjs` не отличает эти +случаи. + +Проверил и поведенчески — не только по регэкспу, но и по факту: применил +только 2-й патч, пересобрал бандл, прогнал сам новый +`demo/smoke_wallthick_standalone.mjs` — **тоже зелёный**. Мутация +(`_recordGeometry` вместо `_commitPhysicalGeometry`) не даёт `_saveConfig()` +(houseplan-card.ts:7402, единственное место записи на бэкенд в +`_commitPhysicalGeometry`) — смок читает мутированное поле прямо из +`_serverCfg` (объект уже изменён присваиванием `partition.cm = cmRaw` до +вызова коммита), поэтому не видит разницы между «записано с проверкой и +уходом на сервер» и «переживено локально, история есть, на бэкенд ничего не +ушло». Практическое следствие обхода барьера в этой ветке — не гипотетическое: +`_commitPhysicalGeometry` пропускаетfail-closed валидацию геометрии +(`_checkSpacePhysicalGeometry`) и обязательный `_saveConfig()`; будущий +рефакторинг, случайно подставивший «облегчённый» путь записи именно в этой +новой ветке, ни один из двух существующих гейтов не поймает. + +**В скоупе ли находка.** Да: и файл (`scripts/mutation-gate.mjs`, класс B, но +через issue этой задачи — AGENTS.md таблица классов, «может использовать issue +того изменения, которое покрывает»), и утверждение (текст коммита #313) — +целиком продукт этой задачи. + +## Что проверено и корректно + +- Приоритет независимой кладки при точном наложении (`offer(...)`, + houseplan-card.ts:11940-11946): тай-брейк `independent && !best.independent + && d <= best.d + 1e-9` — корректно предпочитает независимую кладку только + при равном/лучшем расстоянии, не переопределяет явно более далёкий + кандидат. Порядок перебора (комнаты → перегородки → драфты) не создаёт + систематического смещения: тай-брейк явный, не полагается на порядок + вставки для двух разных источников. +- `sp` (`_curSpaceCfg`, сырой конфиг) и `space` (`_spaceModel()`, отмасштабированная + проекция) — разные объекты, но `id` партиций/драфтов сохраняются 1:1 через + проекцию (`src/space-geometry.ts:167-180`); поиск по id в `_wallThickApply` + корректен, тот же паттерн, что уже использует `_wallThickClick`→`_wallDialog` + для комнатных стен и `_savePhysicalDialog` для партиций/драфтов + (houseplan-card.ts:8287-8300) — не новая, а переиспользованная связка. +- Диалоговая кнопка «на всю комнату» скрывается корректно по + `d.source.kind === 'room'` (houseplan-card.ts:12636), без нового UX-контракта + — остальной диалог тот же компонент. +- Валидация диапазона (1–100, отказ 0/пусто для независимой кладки) идёт + ДО ветвления по источнику и применяется к обеим веткам одинаково; после + ветвления для независимой кладки — отдельный явный отказ на 0/пусто, + соответствует решению владельца №2. Тост переиспользован + (`toast.physical_range`), новый ключ `toast.wallthick_set` уже существовал в + обоих `src/i18n/en.json`/`ru.json:535` до этого коммита (проверил — не + добавлен этим диффом, i18n не тронут, как и заявлено в аналитике «новых + ключей не требуется»). +- Оба CHANGELOG (`docs/CHANGELOG.md`/`.ru.md`) правлены в том же коммите, + трейлеры `Issue: #313`/`User-Visible: yes` на месте — соответствует + AGENTS.md. +- `docs/USER-GUIDE.ru.md` дополнен двумя строками таблицы («Значение», «Какие + стены») терминологией, уже принятой в этом документе (не изобретает новых + слов), корректно описывает и отказ нуля, и приоритет наложения. +- `docs/images/screenshots.json`: изменился только `sourceFingerprint`/ + `sourceSha256` (совпадает с текущим `src/**`, проверено `check-docs.mjs`), + `imageSha256` не тронуты — ни один из 10 задокументированных сценариев не + показывает Plan-редактор с инструментом «Толщина», пересъёмка не требовалась; + фингерпринт обязан был устареть при любой правке `src/**` и был обновлён + правильно. +- Мутант `wall-thickness-writer-bypasses-common-barrier` при обоих патчах + вместе (как запускает CI) действительно ловит регресс — см. Medium выше, + почему это не то же самое, что «оба патча по отдельности эффективны». + +## Чего не проверял + +- Golden/скриншоты, `pytest tests_backend`, performance — не запускал: диф не + меняет визуальный рендер и не трогает Python/бэкенд; не названы в AC. Если + ошибаюсь и где-то в full/static рендере тело диалога/полосы отличается от + существующего компонента — это откроет полный прогон перед бетой. +- Полный `smoke-select` список (остальные 20 из 25 «прямых совпадений») — + осознанно не прогонял, см. таблицу выше; риск считаю низким, но это решение, + а не факт. +- Столбы (`space.wall_columns`) инструментом «Толщина» — вне ТЗ и вне этого + диффа, как и зафиксировал ревьюер ТЗ; не проверял, потому что не должен быть + затронут (не проверял чтением, что резолвер их не подхватывает — это уже не + добавлено кодом, а не «пропущенная проверка»). +- Не проверял поведение при коллинеарной перегородке РЯДОМ (не точно на + ребре) с контуром комнаты, вне #308-фикстуры — тот же пробел, что явно + оставил открытым ревьюер ТЗ («это забота код-ревью... или находка для + отдельного цикла»); не нашёл в этом диффе признаков, что общий случай + (не точное наложение, а близкое соседство) обрабатывается иначе, чем раньше + — резолвер использует то же `distToSegment`/`pull`-радиус, что и для + комнатных стен исторически, поведение не новое для этого диффа. Оставляю + как открытый вопрос для будущей задачи, не завожу отдельный issue: это не + находка ПРОТИВ #313, а недоказанное свойство, унаследованное из старого + кода. +- Не проверял `#306` (замена 0 на виртуальную стену) — вне скоупа, ТЗ прямо + говорит, что этой задаче нужен только шов (switch по источнику), а не + реализация; шов на месте (houseplan-card.ts:12052-12056, единственная точка + ветвления по `d.source.kind`). + +## Вердикт + +Красный. Единственная High-находка — конкретный, воспроизведённый исполнением +дефект в коде, который сама задача добавляет (`_wallThickHit`, драфт-ветка): +диалог «Толщины» лжёт о текущей толщине сегмента черновика, когда она равна 0 +(легитимное, задокументированное в `docs/WALL-THICKNESS.md` §6 состояние), и +безusловный клик «Применить» без правки поля молча превращает такой сегмент из +0 в 15 см. Это прямое нарушение AC2 («диалог с cm сегмента») для конкретного +достижимого состояния модели, а не гипотетическое. Medium-находка (обманчивая +мутационная защита новой точки коммита) в скоупе той же задачи, чинится там же. + +Обе находки локальны (одна строка кода на High; тестовый патч + при +необходимости сам тест на Medium) — переписывания ТЗ/скоупа не требуют.