docs: review document for #228

Issue: #228
User-Visible: no

docs/reviews/CODE-REVIEW-228-r2.md

@@ -0,0 +1,194 @@
+# CODE-REVIEW-228-r2
+
+- Issue: [#228](https://github.com/Matysh/houseplan-card/issues/228) — надёжное рисование стен и операции с готовым контуром
+- ТЗ: `docs/specs/228-plan-drawing-problems.md` (ревью ТЗ зелёное, r1, #228)
+- Ветка: `issue/228-plan-drawing-problems`
+- Коммит реализации r1: `2aaef12` (`fix: make plan drawing fail closed`)
+- Коммит фикса r2: `172d5d7` (`fix: close plan repair review findings`, `Issue: #228`, `User-Visible: yes`)
+- Заход ревью: r2 · блокирующих циклов израсходовано 1 из 4 (r1 был жёлтым и списал единицу; настоящий заход разбирает дельту по §2.10 PROCESS.md / issue #214)
+- Ревьюер: Claude (код-ревью), сессия без контекста реализации и без контекста r1-ревью
+
+## 0. Предыдущий раунд и SHA
+
+Вердикт r1 найден в комментарии issue
+([issuecomment-5379067039](https://github.com/Matysh/houseplan-card/issues/228#issuecomment-5379067039)):
+жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 2 (M1, M2, обе в
+скоупе). Полный документ — `docs/reviews/CODE-REVIEW-228-r1.md` (в дереве на
+этом SHA, коммит `9540627`).
+
+**Замечание к процессу, не к коду:** сам текст вердикта в комментарии issue SHA
+не называет — только «Документ: …». SHA восстановлен из шапки документа
+(«Коммит реализации: `2aaef12`») и из хендоффа автора чуть выше
+(«Реализация: 2aaef12») и хендоффа фикса чуть ниже («предыдущий проверенный
+SHA 2aaef12 → фикс 172d5d7»), так что дельта для этого раунда однозначна:
+`git diff 2aaef12..172d5d7` (= `git diff 2aaef12..HEAD`, `HEAD` этой ветки —
+`172d5d7`). Это не блокирует разбор, но стоит перенести в шаблон вердикта:
+именование SHA явно, а не только через ссылку на документ.
+
+Ребейза не было: `git merge-base origin/dev HEAD` не изменился относительно
+диапазона задачи, `2aaef12` не достижим из `origin/dev` (задача просто ещё не
+слита) — код, проверенный в r1, и код на этом SHA продолжают одну и ту же
+историю без переписывания. Дельта локальна, полный разбор не требуется по
+§2.10/§7.2 AGENTS.md.
+
+## 1. Скоуп проверки — дельта r1→r2
+
+`git diff --stat 2aaef12..172d5d7`: 10 файлов, +420/−24.
+
+Продуктовый код — только `src/houseplan-card.ts` (+17/−9, две точки:
+`_offerExistingWallFace` и `_validateWallRepair`) и новый экспорт в
+`src/wall-face-repair.ts` (+15, чистая функция `repairMovesHostedPartition`).
+Тесты — `test/wall-face-repair.test.mjs` (+22/−2, новый unit-тест) и
+`demo/smoke_plan_drawing_repairs.mjs` (+29, два новых сценария). Остальное —
+три синхронные копии бандла, оба changelog (RU+EN, `User-Visible: yes` в том
+же коммите) и сам `docs/reviews/CODE-REVIEW-228-r1.md`, добавленный в дерево
+отдельным коммитом `9540627` между r1 и фиксом.
+
+Дельта задевает ровно AC6/AC7/AC8 (repair ≤2 см, негативная матрица, room
+dialog) и §8.6 ТЗ — те же строки, что и обе находки M1/M2 из r1. Ни одна
+другая поверхность (snap-overlay, strict-Shift, delete-room dialog,
+room-deletion transaction) не тронута — их AC (AC1–AC5, AC9–AC14) наследуются
+из r1 без повторной проверки, раздел 4.
+
+## 2. Как проверялось — гейты
+
+Дешёвые гейты прогнаны полностью (стоят минуты, код изменился):
+
+| Гейт | Команда | Результат |
+|---|---|---|
+| Typecheck | `npx tsc --noEmit` | зелёный, без вывода |
+| Unit | `npm test` | `# tests 1032 / # pass 1032 / # fail 0 / # skipped 0` (на 1 больше, чем в r1 — ровно новый тест `repairMovesHostedPartition`) |
+| Build | `npm run build` | зелёный, `dist/houseplan-card.js` собран за ~12s |
+| Синхронность бандла | `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` и `cmp … demo/srv/assets/houseplan-card.js` | обе копии побайтно идентичны, `git status` после сборки чист |
+| Named smoke (дельта) | `node demo/smoke_plan_drawing_repairs.mjs` | 16/16 `true`, `OK` |
+
+Дополнительно, специально для дисциплины «тест умеет падать» (§8 PROCESS.md,
+это условие честности сокращения гейтов на дельте): временно откатил
+`src/houseplan-card.ts` и `src/wall-face-repair.ts` к состоянию `2aaef12`,
+пересобрал, скопировал бандл в `demo/srv/assets/` и повторно прогнал тот же
+smoke — рабочее дерево затем восстановлено к `HEAD` и пересобрано, `git
+status` чист.
+
+- на коде до фикса `ambiguousLargeGapUsesWallsFlow` → **`false`** (упал), все
+  остальные 15 проверок остались `true`. Это прямое доказательство того, что
+  проверка M2 фальсифицируема и действительно закрывает найденный дефект, а не
+  переформулирует его условие.
+- `hostedOpeningBlocksRepair` на коде до фикса остался `true`: ожидаемо — сам
+  инлайновый guard (`if (proposal.sourceKey.startsWith('static:partition|'))
+  …`) в `2aaef12` был по чтению кода уже корректен, M1 была находкой про
+  **отсутствие доказательства**, а не про баг в логике (это прямо
+  сформулировано в M1 из r1: «Guard … по чтению кода верен … Но ни один тест
+  не воспроизводит этот случай»). Для этой находки доказательство падения даёт
+  прямой unit-тест на извлечённую функцию: `repairMovesHostedPartition` в
+  `2aaef12` не существовала вовсе (`grep` по старому файлу — 0 совпадений),
+  поэтому импорт этой функции в `test/wall-face-repair.test.mjs` на коде до
+  фикса завершился бы ошибкой сборки/импорта, а не тихим прохождением. Тест
+  падать умеет, просто не через тот же smoke-сценарий, а через прямой unit.
+
+**Чего не прогонял и почему:**
+
+- `smoke_plan_snap_overlay.mjs`, `smoke_unified_wall_tool.mjs` — прогнаны в r1
+  зелёными (34/34, 19/19), дельта r1→r2 не касается ни snap-overlay, ни
+  strict-Shift кода; наследуются без повторного прогона (раздел 4).
+- `npm run golden:verify`, полный smoke-набор (127 файлов), `pytest
+  tests_backend`, performance-профиль — как и в r1: diff не меняет ни один
+  существующий видимый кадр (правки чинят внутреннюю ветку без нового
+  рендера), ни один `.py`-файл не тронут, задача не задевает все поверхности.
+  Полные наборы — предрелизный гейт (§8 PROCESS.md), не гейт ревью; причины не
+  изменились с r1 и заново не переоцениваю их для дельты, которая эти
+  поверхности не трогает.
+
+## 3. Закрытие раунда r1
+
+| Находка r1 | Чем закрыта | Где это видно |
+|---|---|---|
+| **M1** — AC7 требует `unit + smoke` для «invalid hosted opening», guard в `_validateWallRepair` (houseplan-card.ts:12673, на SHA `2aaef12`) существовал, но не был воспроизведён ни одним тестом | Guard вынесен в именованную экспортируемую чистую функцию `repairMovesHostedPartition` (`src/wall-face-repair.ts:29-40`), покрытую прямым unit-тестом с тремя ветвями (совпадающий host, несовпадающий host/kind, не-partition `sourceKey`); плюс сквозной smoke-сценарий `hostedOpeningBlocksRepair`, который проводит `_offerExistingWallFace` → `_saveRoom()` через реальный hosted-opening на дистанции 1,2 см и проверяет, что `_curSpaceCfg` не изменился и показан именно `toast.wall_repair_changed` | `src/wall-face-repair.ts:29-40` (функция); `test/wall-face-repair.test.mjs:53-70` (unit, 3 ветви); `demo/smoke_plan_drawing_repairs.mjs:104-118` (smoke); вызов на Create-пути — `src/houseplan-card.ts:12674-12676` внутри `_validateWallRepair`, дёргается из `_applyWallFaceBatch` (`:12777`) |
+| **M2** — широкая (>2 см, screen-scale) диагностика в `_offerExistingWallFace` не различала `diagnostic.kind === 'repair'` и `'ambiguous'`; при ambiguous клик перехватывался и подсвечивался произвольный `proposals[0]` вместо обычного Walls flow (§8.6 ТЗ) | Условие сужено с `if (diagnostic.kind !== 'none')` до `if (diagnostic.kind === 'repair')`; при `'ambiguous'` код больше не входит в ветку, падает через к `if (!face …) return false;` — обычный Walls flow, без toast, без подсветки, без перехвата клика | `src/houseplan-card.ts:7684` (изменённое условие); regression-сценарий `ambiguousLargeGapUsesWallsFlow` в `demo/smoke_plan_drawing_repairs.mjs:94-102`, воспроизведён как падающий на коде до фикса (раздел 2 выше) |
+
+Обе находки закрыты фактическим кодом и тестом, который умеет падать (M2 —
+прямым воспроизведением регресса; M1 — прямым unit-тестом на извлечённую
+функцию), а не заявлением автора. Хендофф автора
+([issuecomment-5379081221](https://github.com/Matysh/houseplan-card/issues/228#issuecomment-5379081221))
+совпадает с тем, что видно в дельте.
+
+## 4. Унаследовано из r1
+
+Без повторной проверки в этом раунде, полностью на основании
+`docs/reviews/CODE-REVIEW-228-r1.md` (в дереве на SHA `9540627`,
+предмет разбора там — SHA `2aaef12`, тот же код в этих AC дельтой r2 не
+тронут):
+
+- **AC1–AC4** (активная ось/узел на текущем отрезке, fail-closed на
+  неоднозначных endpoint, strict-Shift через точное пересечение луча, цвет
+  подписи по точному вектору) — `src/plan-snap-overlay.ts`, `src/logic.ts`
+  (`isExact45Vector`), рендер `.active-axis`/`.active-vertex`. Дельта r1→r2 не
+  трогает ни один из этих файлов.
+- **AC5/AC8** (комната из готового контура, `Shift+click` bypass,
+  create-existing-face сохраняет партиции) — кроме уточнённой в M2 ветки
+  широкой диагностики, остальная логика `_offerExistingWallFace` и
+  `findWallFaceAtPoint`/`wall-face-graph.ts` не менялась.
+- **AC9** (диалог удаления через `hp-dialog`, не `confirm()`) — `src/houseplan-card.ts`
+  вне тронутого дельтой диапазона строк, `src/room-deletion.ts` не в diff.
+- **AC10–AC12** (Keep/Delete walls материализация и rehost, атомарность
+  Undo, fail-before-mutation порядок) — `src/room-deletion.ts` не в diff r2.
+- **AC13–AC16** (View не регрессирует, touch/gesture guard не тронут, perf-путь
+  не заходит в hover, backend/schema не тронуты) — ни один из названных в r1
+  файлов (`src/houseplan-card.ts:_svgPointerMove`, `custom_components/**/*.py`)
+  не в diff r2.
+- **AC17** (гейты/i18n/changelog/документация уровня r1) — новые i18n-ключи в
+  дельте не добавлялись (использован уже существующий `toast.wall_repair_changed`,
+  проверено `grep` — есть в обоих `src/i18n/*.json` и не создан в этом диффе);
+  changelog RU+EN обновлён именно в фикс-коммите `172d5d7` с точными формулировками
+  под M1/M2 (см. раздел 5).
+
+Основание для наследования: дельта `2aaef12..172d5d7` не задевает ни один файл
+и ни одну строку, от которых зависят перечисленные AC — проверено `git diff
+--stat` (раздел 1) и точечным чтением затронутых строк (раздел 3). Это не
+«не проверялось», а «проверено в r1 на коде, который здесь не менялся».
+
+## 5. Дополнительно проверено в этом раунде
+
+- Оба changelog (`docs/CHANGELOG.md`, `docs/CHANGELOG.ru.md`) правлены в том же
+  коммите `172d5d7`, что и код, с `User-Visible: yes` — трейлеры на коммите
+  корректны (`Issue: #228`, `User-Visible: yes`). Текст обеих правок точно
+  называет оба закрытых поведения: «ambiguous wider gaps remain ordinary
+  drawing» (M2) и «a wall carrying a hosted opening is never moved by repair»
+  (M1) — формулировки не обобщают и не расходятся с кодом.
+- `repairMovesHostedPartition` вызывается из `_validateWallRepair`, которая
+  используется на **обоих** путях подтверждения repair —
+  `_applyWallRepair` (одиночный, `:12685`) и `_applyWallFaceBatch` (через
+  `repairs[0]`, `:12777`) — оба реально достижимы из `_saveRoom`/`_offerExistingWallFace`
+  флоу, guard не обходится ни одним из них.
+- Оставление предложения (`_offerExistingWallFace`) hosted-репейра без
+  немедленного отказа и отказ только на Create — соответствует ТЗ §8.6
+  дословно («Room dialog не применяет repair заранее. Create повторно
+  проверяет … hosted openings»), это не новый пробел, а сознательный
+  двухфазный контракт, уже заложенный в ТЗ и подтверждённый в r1.
+- Тестовый счётчик: r1 зафиксировал `1031 pass`, здесь `1032 pass` — разница
+  ровно на 1 новый unit-тест M1, без скрытых удалений/пропусков (`0 skipped`,
+  расходится с хендоффом автора «1 skipped», как и в r1 — этот разброс не
+  влияет на вердикт, оба прогона зелёные).
+
+## 6. Чего не проверял
+
+- Всё, что унаследовано из r1 (раздел 4) — не переисполнялось повторно; список
+  причин не изменился и заново не переоценивался, кроме подтверждения, что
+  соответствующие файлы вне дельты.
+- `npm run golden:verify`, полный (127 файлов) smoke-набор,
+  `python -m pytest tests_backend`, performance-профиль — не запускал в этом
+  раунде; причины совпадают с r1 (раздел 2) и дельта их не меняет: новых
+  видимых кадров нет, `.py` не тронут, задача не задевает все поверхности.
+  Это предрелизный гейт (§8 PROCESS.md, §11.4 AGENTS.md), не гейт ревью.
+- Многопользовательский конкурентный сценарий и touch-специфичные жесты для
+  repair-пути — не переисполнялись; вывод r1 («чтением, не исполнением») не
+  затронут дельтой, которая не трогает `_markupClick`-обёртку или
+  multi-client код.
+
+## 7. Вердикт
+
+Обе Medium-находки r1 закрыты кодом и тестом, способным падать; никаких новых
+находок в дельте не обнаружено. High — 0, Medium — 0.
+
+Вердикт: зелёный · заход r2 · блокирующих циклов 1/4 · High: 0 · Medium: 0
+
+Готово к очереди на пре-релиз (`S8-merged`).