diff --git a/docs/reviews/CODE-REVIEW-229-r2.md b/docs/reviews/CODE-REVIEW-229-r2.md new file mode 100644 index 00000000..51301053 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-229-r2.md @@ -0,0 +1,232 @@ +# CODE-REVIEW-229-r2 + +Issue: [#229](https://github.com/Matysh/houseplan-card/issues/229) — «прямая стена, нарисованная в несколько кликов, остаётся одной записью, а не рядом швов». +Заход: r2. Ревью ТЗ прошло тремя заходами (r1 M×3 → r2 M×1 → r3 зелёный, SHA `1ecd267`). +Код-ревью: r1 — красный (High×2, SHA `50a00e4`), r2 — этот документ, SHA `ed13b4a`. +Блокирующих циклов до этого раунда: 1/4. + +## Скоуп раунда (§2.10) + +Предыдущий заход (r1) получен на диапазоне `origin/dev...HEAD` при `HEAD = 50a00e4` +(коммиты `e6be43b` feat + `50a00e4` docs-скриншоты). SHA не был назван в самом +тексте вердикта — фиксирую его здесь по факту: комментарий-вердикт содержит только +имена коммитов, точку HEAD восстановил из истории веток (`git log --oneline +origin/dev..HEAD`, коммит непосредственно перед фикс-коммитом `ed13b4a`). + +Дельта раунда — `git diff 50a00e4..HEAD` (коммит `ed13b4a`, один коммит): + +``` +custom_components/houseplan/frontend/houseplan-card.js | 6 +- (сгенерировано) +demo/srv/assets/houseplan-card.js | 6 +- (сгенерировано) +dist/houseplan-card.js | 6 +- (сгенерировано) +docs/images/screenshots.json | 22 +- (отпечаток источника, ожидаемо) +docs/reviews/CODE-REVIEW-229-r1.md | 282 ++ (публикация документа r1, не код) +demo/smoke_wall_chain_merge.mjs | 22 +- +scripts/mutation-gate.mjs | 25 ++ +src/houseplan-card.ts | 9 +- +src/plan-optimizer.ts | 7 +- +test/plan-optimizer.test.mjs | 20 ++ +``` + +Дельта локальна: один коммит, пять содержательных файлов, оба High из r1 и +только они. Продуктовая рамка не менялась, новая подсистема не задета, ребейза +не было (`50a00e4` — предок `HEAD` напрямую). Полный повторный разбор не требуется +по правилу §2.10 — разбор ограничен дельтой плюс тем, до чего дельта дотягивается +(AC2 и §8.6/AC8, единственные AC, чьё доказательство эта правка меняет). + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **High-1**: `_mergeSpacePartitions` (houseplan-card.ts) и `optimizePlans` (plan-optimizer.ts) делили уже сырой полигон комнаты (`roomPoly`) на `NORM_W` второй раз — комната «улетала» в область ~0.0001, T-стык к её стороне не находился | Обе обвязки перестали делить `roomPolygons` на `NORM_W`: `src/houseplan-card.ts:6557-6560`, `src/plan-optimizer.ts:507-510`. Комментарий на месте объясняет, что `roomPoly` уже возвращает координаты в масштабе перегородок | Прочитано и перепроверено: `roomPoly` (`src/logic.ts:103-108`) отдаёт `r.x/r.y/r.w/r.h` без масштабирования; `sp.partitions[i].a` в `_finishWallChain` — тоже сырые координаты (`segment.a[0] / NORM_W`, `houseplan-card.ts:6609`). Т.е. деление было лишним объективно, не только по утверждению автора. Дополнительно воспроизведено исполнением: новый юнит `test/plan-optimizer.test.mjs:497-516` («a node on the side of a room survives the sweep») зелёный на текущем коде, красный при ручном возврате деления (см. «Как проверялось») | +| **High-2**: `_finishWallChain` вызывает слияние (`houseplan-card.ts:6612` тогда) раньше, чем убирает завершаемый черновик из `sp.room_drafts`, поэтому собственные концы черновика ложно считались чужим примыканием | `draftEnds` теперь исключает запись с `id === this._activeDraftId` (`src/houseplan-card.ts:6566`), тем же паттерном, что `plan-snap-overlay.ts` | Прочитано: исключение стоит до `this._activeDraftId = null` (строка 6626), то есть значение ещё актуально в момент вызова. Воспроизведено исполнением: `demo/smoke_wall_chain_merge.mjs` теперь рисует продолжение реальными кликами (`click()`, строки 53-62) вместо присвоения `_path`, и это зелёное; при ручном откате исключения (снятие условия `if (draft?.id === this._activeDraftId) return [];`) смок красный — подтверждено запуском мутанта `chain-merge-sees-own-draft` через `node scripts/mutation-gate.mjs --id=chain-merge-sees-own-draft` (см. ниже) | + +Обе High-находки закрыты по существу, не заплаткой: в обоих случаях автор +объяснил причину (общая система координат `roomPoly`/`partitions`; порядок +операций в `_finishWallChain`) и добился падения теста на прежнем коде, а не +подобрал совпадающие числа постфактум — я это проверил самостоятельно (см. ниже). + +## Унаследовано из r1 + +Из документа `docs/reviews/CODE-REVIEW-229-r1.md` (SHA `50a00e4`) принято без +повторной проверки — дельта не касается их доказательной поверхности: + +- **AC1** (прямая цепочка — одна запись) — не тронут; `demo/smoke_wall_chain_merge.mjs` + проверки `straightRunIsOneRecord`/`straightRunSpansTheChain`/`straightRunKeepsThickness` + не менялись. +- **AC3** (проём не двигается, оба представления — `host.t` и legacy `x/y/angle`) — + `wall-merge.ts` и его юниты не менялись в этой дельте. +- **AC4** (разная толщина не сращивается), **AC5** (допуски `EPS_JOIN`), + **AC6** (детерминизм по порядку) — та же пара, `wall-merge.ts`/`wall-merge.test.mjs` + вне дельты. +- **AC7** (оптимизатор сращивает накопленное, идемпотентность) — логика сведения + накопленных швов в `plan-optimizer.ts` не менялась, изменилась только область + видимости комнат внутри той же функции. +- **AC9** (release-артефакты) — не применим к этому коммиту: `User-Visible: no`, + changelog не требуется; сам факт трейлера перепроверен (см. «Гейты»). +- Общая корректность структуры ТЗ, i18n, touch-контракт (`Touch editor: not exposed`) — + вне дельты кода, r1 их не пересматривал. + +## Что проверялось и как + +**Всегда (быстрые гейты, прогнаны заново на `ed13b4a`):** + +| Гейт | Команда | Результат | +|---|---|---| +| Типы | `npx tsc --noEmit` | чисто | +| Юниты | `npm test` | 1004/1004, 0 fail | +| Сборка + сверка бандла | `npm run build` + `cmp` трёх копий (`dist`, `custom_components/houseplan/frontend`, `demo/srv/assets`) | идентичны байт-в-байт, дерево чистое после копирования | + +**По необходимости, определённой дельтой:** + +- `node demo/smoke_wall_chain_merge.mjs` — единственный смок, прямо изменённый + дельтой (H2) и покрывающий сценарий High-1/High-2 на живом клик-пути. Зелёный, + 9/9 проверок. +- Соседние смоки того же семейства (тронутый `_finishWallChain`/`_mergeSpacePartitions` + и оптимизатор): `optimize_coordinate_canonicalization`, `wall_junctions`, + `partition_openings`, `free_walls` — зелёные, регрессий не внесено. +- `node scripts/check-docs.mjs` — прошёл (7 файлов, отпечаток `docs/images/screenshots.json` + синхронен с пересобранным бандлом — сам факт изменения этого файла в дельте ожидаем, + не дефект). +- `node scripts/process-gate.mjs` — пройден, 0 предупреждений; трейлеры `Issue: #229` / + `User-Visible: no` на `ed13b4a` корректны, changelog не требовался и не менялся. +- **Дисциплина «тест умеет падать» — проверена исполнением, не на словах,** для обеих + High-находок: + - вручную вернул деление на `NORM_W` в `src/plan-optimizer.ts`, пересобрал + `test-build` (`npx tsc -p tsconfig.test.json && node scripts/fix-test-build.mjs`) + и прогнал `node --test --test-name-pattern="issue 229" test/plan-optimizer.test.mjs`: + новый тест «a node on the side of a room survives the sweep» красный + (`1 !== 0`, ровно симптом из r1 — `partitionsMerged` вместо ожидаемых 0); + восстановил файл — тест снова зелёный, дерево чистое; + - прогнал мутант `node scripts/mutation-gate.mjs --id=chain-merge-sees-own-draft` + через штатный харнесс (git worktree + пересборка бандла): чистый прогон зелёный, + мутант красный, «поймано 1 из 1» — H2 подтверждён штатным гейтом. + +**Не прогонялось и почему:** полный набор из 127 смоков, `golden:verify`, +`pytest tests_backend` — дельта не трогает Python, рендер, слои и любую другую +поверхность вне рисования перегородок и оптимизатора; полные наборы — предрелизный +гейт (§8), не гейт ревью. Performance-профили не запускал — в AC не названы, +дельта не касается путей, отмеченных как чувствительные к перфу. + +## Находки + +### Medium-1 (в скоупе задачи): регрессионное покрытие High-1 закрывает только один из двух исправленных вызовов + +`src/houseplan-card.ts` (`_mergeSpacePartitions`, живой путь рисования) и +`src/plan-optimizer.ts` (`optimizePlans`, «Оптимизировать планы») содержали +**одинаковый** дефект лишнего деления `roomPolygons` на `NORM_W`, и оба +исправлены в этом коммите идентичным образом. Но новый регрессионный тест +(`test/plan-optimizer.test.mjs:497-516`) и сторожащий его мутант +`partition-merge-rescales-rooms` (`scripts/mutation-gate.mjs:718-730`) проверяют +**только** путь `plan-optimizer.ts`. Путь `houseplan-card.ts` — тот самый, для +которого r1 привёл второе, самостоятельное воспроизведение («в браузере через +реальные клики, `space.partitions.length === 1` вместо 2») — не получил ни +юнита, ни смок-сценария, ни мутанта. + +**Воспроизведение (исполнением):** вручную вернул то же лишнее деление +**только** в `src/houseplan-card.ts` (оставив `plan-optimizer.ts` в исправленном +виде), пересобрал бандл и `test-build`, прогнал весь набор: + +- `npm test` — 1004/1004 pass, **ни один тест не покраснел**; +- `node demo/smoke_wall_chain_merge.mjs` — все 9 проверок зелёные, дефект не + затронут ни одной из них (в этом смоке ни разу не участвует комната — только + голые перегородки). + +Восстановил файл — поведение снова корректно (проверено чтением: `roomPoly` +и координаты перегородок в `_finishWallChain`/`_mergeSpacePartitions` — один и +тот же масштаб, деление объективно лишнее в обоих местах). Сам текущий код +корректен; дефект — в отсутствии автоматической защиты именно этого вызова: +следующая правка `_mergeSpacePartitions` может тихо вернуть тот же баг на живом +пути рисования, и ни `npm test`, ни штатный набор смоков этого не заметят. + +**Почему это Medium, а не High:** правка, которая лежит в этом коммите, сама по +себе корректна и доказана чтением плюс ручным воспроизведением (я его провёл +сам, а не поверил заявлению автора) — AC2 для этого вызова не «предположительно +доказан», а доказан наблюдением. Дефект — в будущей защите, а не в текущем +поведении, поэтому не блокирует, но должен быть закрыт в этой же задаче: она и +её мутационный гейт — единственное место, где это естественно сделать, и Medium +в скоупе чинится в текущем issue (§2.7, #202). + +**Предложение:** добавить в `demo/smoke_wall_chain_merge.mjs` сценарий с +комнатой, к середине стороны которой рисуется примыкающая перегородка через +настоящие клики (по образцу нового юнита в `plan-optimizer.test.mjs`, но через +живой `_finishWallChain`), и зарегистрировать для `src/houseplan-card.ts` +второй мутант-близнец `partition-merge-rescales-rooms`, гвардом на этот смок. + +### Low (снято без действия): точный SHA предыдущего вердикта не был назван явно + +Комментарий-вердикт r1 называет коммиты (`e6be43b`, `50a00e4`), но не отдельно +итоговый SHA HEAD на момент разбора — пришлось восстанавливать по истории +(см. «Скоуп раунда»). Не влияет на проверяемость: коммиты названы однозначно, +диапазон восстанавливается детерминированно. Снимаю без действия, отмечаю для +будущих раундов этой же задачи. + +## Находка вне скоупа — заводится отдельным issue + +При проверке «тест умеет падать» для High-1 через штатный харнесс +(`node scripts/mutation-gate.mjs --id=partition-merge-rescales-rooms`) прогон +завершился на этапе чистого прогона: + +``` +FAIL чистый прогон: node --test --test-name-pattern="issue 229" test/plan-optimizer.test.mjs +красный без мутанта +Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../test-build/plan-optimizer.js' +``` + +Причина — не в мутанте и не в тесте: `makeWorktree()`/`buildBundle()` +(`scripts/mutation-gate.mjs:964-990`) создают чистый git-worktree и пересобирают +только `rollup` (бандл), но никогда не выполняют `npx tsc -p tsconfig.test.json +&& node scripts/fix-test-build.mjs`. Любой мутант, чей `guard` — короткая форма +`node --test --test-name-pattern="…" test/<файл>.test.mjs`, где `<файл>.test.mjs` +импортирует `../test-build/*.js` (а не читает `src/*` текстом), в свежем +worktree не находит `test-build/` вообще и падает с `ERR_MODULE_NOT_FOUND` — то +есть «гейт словил поломку» на самом деле означает «гейт не может исполниться», +а `runCleanGuards()` (базовый прогон без мутанта) тоже красный по той же причине, +поэтому полный прогон встаёт на первом же таком мутанте с кодом выхода 2. + +Это не специфично для #229: тот же паттерн гварда уже используют мутанты issue +#220 (`chain-merge-sweeps-...` — нет, конкретно `stale-space-position-...` и три +мутанта у `test/space-order.test.mjs`, `scripts/mutation-gate.mjs:848-894`, +внесены коммитом `a8aeecc`, который старше `#229` целиком) и старые мутанты +самого #229 из `e6be43b` (`test/wall-merge.test.mjs`, шесть мутантов, +`scripts/mutation-gate.mjs:757-820`) — я прогнал один из них +(`partition-merge-ignores-thickness`) и получил ту же ошибку. Значит недостаток +живёт в общей инфраструктуре `scripts/mutation-gate.mjs`, предшествует этой +задаче и не создан её диффом (дифф только добавил ещё один мутант того же, +уже дефектного, вида). Правка чужого скоупа с этой ветки запрещена (#202) — +заведён отдельный issue: + +**Новый issue: [#235](https://github.com/Matysh/houseplan-card/issues/235)** +«`mutation-gate.mjs` не пересобирает `test-build` в worktree-мутантах — гварды, +читающие `../test-build/*.js`, всегда красны на чистом прогоне» → метки +`tech-debt`, `P2`, `S1-new`, ссылка на #229 и #220 как источники затронутых +мутантов. + +Само поведение фиксов High-1/High-2 это не отменяет: я подтвердил обе находки +без штатного харнесса — прямым воспроизведением в основном рабочем дереве +(ручной откат патча → пересборка `test-build` в реальном, не временном, +дереве → тест красный → восстановление) и, для High-2, штатным прогоном +мутанта, чей guard не зависит от `test-build` (браузерный смок). + +## Итог по AC + +| AC | Статус | Доказательство | +|---|---|---| +| AC1 | не тронут дельтой | унаследовано из r1 | +| **AC2** | подтверждён повторно | юнит `plan-optimizer.test.mjs` (комната) + смок (перегородка↔перегородка) — оба зелёные и оба доказано умеют падать; путь `houseplan-card.ts` доказан чтением кода, автотеста нет (Medium-1) | +| AC3 | не тронут дельтой | унаследовано из r1 | +| AC4-AC6 | не тронуты дельтой | унаследовано из r1 | +| AC7 | не тронут дельтой | унаследовано из r1 | +| **AC8 (§8.6)** | подтверждён повторно | смок `chain_wall_merge`, мутант `chain-merge-sees-own-draft` через штатный харнесс — красный на откате, зелёный на текущем коде | +| AC9 | не применим к этому коммиту | `User-Visible: no`, трейлеры проверены `process-gate.mjs` | + +## Вердикт + +Обе High-находки r1 закрыты по существу и перепроверены исполнением, а не по +заявлению автора. Найден один новый Medium **в скоупе** задачи (регрессионное +покрытие High-1 однобокое — защищён только путь оптимизатора, путь живого +рисования нет) и один Medium **вне скоупа** (инфраструктурный дефект +`mutation-gate.mjs`, предшествующий этой задаче, заведён отдельным issue). +High нет — вердикт жёлтый. + +Вердикт: жёлтый · заход r2 · блокирующих циклов 2/4 · High: 0 · Medium: 1 → в задаче