diff --git a/docs/reviews/SPEC-REVIEW-229-r2.md b/docs/reviews/SPEC-REVIEW-229-r2.md new file mode 100644 index 00000000..c01fe825 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-229-r2.md @@ -0,0 +1,196 @@ +# SPEC-REVIEW-229-r2 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/229 +- **ТЗ:** `docs/specs/229-merge-collinear-partitions.md` +- **SHA на момент этого ревью:** `ecc3d6a88ba259d3446f77ab3a60170337bc068f` + (коммит `docs: close M1, M2 and M3 from the spec review of #229`) +- **Этап:** spec (PROCESS.md §2.4) · трек: обычный (не `small`) +- **Заход:** r2 · блокирующих циклов израсходовано 1/4 до этого вердикта +- **Предыдущий раунд:** `docs/reviews/SPEC-REVIEW-229-r1.md`, вердикт жёлтый на + SHA `81210f7` (ветка `issue/229-merge-collinear-partitions`) +- **Ревьюер:** Claude, роль «ревьюер ТЗ» + +## Скоуп этого раунда + +Дельта — один коммит `ecc3d6a`, который правит только +`docs/specs/229-merge-collinear-partitions.md` (62 добавленных / 15 удалённых +строк) в ответ на три Medium-находки r1 (M1, M2, M3). Правка локальна: не +ребейз, не смена контракта, не новая подсистема, объём дельты (одна секция +плюс точечные правки соседних) заметно меньше исходного ТЗ. По правилу §2.9/§2.10 +разбор идёт по дельте: полная повторная проверка — только для того, что дельта +задевает (AC2, AC3, AC8, §8.2, §8.4, §8.6, §14, §17.4); остальное наследуется из +r1 без повторного прогона. + +Тело issue #229 не менялось между раундами (сравнил текущее содержимое issue с +цитатами в r1 — совпадает буквально), поэтому продуктовая рамка (§1–§4 ТЗ) не +пересматривалась заново. + +## Как проверялось + +1. Прочитан вердикт r1 (`docs/reviews/SPEC-REVIEW-229-r1.md`) — три Medium + (M1, M2, M3) и одна снятая Low (L1). +2. `git diff 81210f7..HEAD -- docs/specs/229-merge-collinear-partitions.md` — + единственный файл дельты; построчно сверен с формулировками M1/M2/M3. +3. Для каждой из трёх находок проверено, чем именно она закрыта (не заявлением + коммита, а текстом ТЗ) — см. таблицу «Закрытие раунда r1». +4. Так как правка M2 вводит новый нормативный текст («ребро комнаты — ближайшая + вершина полигона»), он сверен с каноническим прецедентом продукта: + - `docs/specs/141-wall-junctions.md` §4.1: «Исправление охватывает точные + endpoint↔endpoint и endpoint↔line (**T**) соединения... между active/saved + draft, partition и **готовой комнатной стеной**» — T-стык партиции с + ЛИНИЕЙ комнатной стены (не только с её вершиной) прямо назван как канон + этого продукта, а не гипотеза; + - `docs/specs/141-wall-junctions.md` §13.1, п.5 — regression-тест явно требует + «T partition→partition, draft→partition **и partition→solid room wall**»; + - `src/plan-snap-overlay.ts:313` вызывает `distToSegment(...)` от + `roomEdges` (`src/logic.ts`) — существующий код-примитив для «точка близко + к стороне комнаты» уже оперирует расстоянием до **отрезка**, а не только до + вершины. +5. Проверены построчные ссылки на код, добавленные/исправленные дельтой: + `houseplan-card.ts:6538` (`_finishWallChain`, теперь верно и в §3, и в §6 — + было `:6558` в r1), `:7824` и `:12007` (`materializePartitionOpening`, + оба вызова подтверждены чтением файла). Все точны. +6. Заново прочитаны AC2, AC3, AC8, §14 (мутационный гейт) — они напрямую задеты + дельтой. AC1, AC4, AC5, AC6, AC7, AC9 дельта не касается — наследуются из r1. +7. Код не запускался, гейты не гонялись — на этапе spec-review реализации нет, + оценивать нечего (как и в r1). + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| **M1** — слияние при завершении цепочки шире решения владельца о старых планах | Добавлен §8.6 «Что именно сращивается при завершении цепочки»: скоуп сужен до компоненты связности по общим концам, содержащей хотя бы один новый сегмент цепочки; явно сказано «не всё пространство» с тем же примером `#0`/`#4` из #228, который в r1 был контрпримером | `docs/specs/229-merge-collinear-partitions.md` §8.6 (новая секция); AC8 дополнен строкой про перегородки вне компоненты связности; в мутационный гейт (§14) добавлена строка `chain-merge-sweeps-whole-space` → guard «юнит AC8»; §17.4 переписан и явно ссылается на «находку M1 ревью r1» | +| **M2** — допуск «есть причина оставить узел» не определён для трёх из четырёх причин | В §8.2 добавлен абзац: все четыре причины проверяются одним `EPS_JOIN`; для каждой названа точка стыка (третья перегородка — её конец; ребро комнаты — «ближайшая вершина полигона»; колонна — центр; черновик — конец сохранённого контура) | `docs/specs/229-merge-collinear-partitions.md` §8.2, абзац «Все четыре причины проверяются одним допуском `EPS_JOIN`». **Закрыта частично — см. Medium-1 ниже**: сам факт единого допуска и явного критерия для всех четырёх причин снимает исходную находку (было — критерий не назван вовсе, стало — назван), но выбранный критерий для «ребра комнаты» противоречит канону #141 | +| **M3** — §8.4 не упоминает материализованную legacy-проекцию `x/y/angle` | В §8.4 добавлен шаг 4 «пересчитывается материализованная проекция `x/y/angle`», сослан на `#132`/`CONFIG-COMPATIBILITY.md` и на тот же прецедентный код (`:7824`, `:12007`), что r1 приводил как образец. Инвариант и AC3 переписаны: явно требуют совпадения «и в резолвленном виде, и в материализованной проекции», AC3 теперь явно «остаётся красным, если пересчитан только `host.t`» | `docs/specs/229-merge-collinear-partitions.md` §8.4 п.4, §12 AC3, §14 добавлена строка `partition-merge-skips-materialization` → guard «юнит AC3» | +| **L1** (снята в r1 без действия) — расхождение номера строки `_finishWallChain` `:6558` vs `:6538` | Попутно исправлено и в §3, и в §6, хотя это не требовалось (была снята без действия) | `docs/specs/229-merge-collinear-partitions.md` §3, §6 | + +## Находки этого раунда + +### Medium-1 — критерий «ребро комнаты» для допуска `EPS_JOIN` (правка M2) сам по себе противоречит канону T-стыков продукта + +**Файл:** `docs/specs/229-merge-collinear-partitions.md`, §8.2 (новый абзац, +внесён дельтой r2). + +**Формулировка ТЗ (текущая):** «Точка стыка берётся у ребра комнаты — ближайшая +вершина полигона, у колонны — её центр, у черновика — конец сохранённого +контура.» + +**Проблема.** Эта фраза отвечает на вопрос, поднятый находкой M2 r1 («точка на +точке» vs «точка на отрезке» для ребра комнаты), но отвечает на него +геометрически неверно относительно уже принятого канона продукта. Слово +«ребро» здесь означает сторону комнатного полигона — отрезок, а не точку. ТЗ же +проверяет близость конца перегородки к **ближайшей вершине** этого полигона, то +есть только к его углам. Между тем `docs/specs/141-wall-junctions.md` (принятое +и не отменённое ТЗ этого же репозитория, канон стыков стен) §4.1 прямо называет +«endpoint↔line (T)» соединение партиции с **готовой комнатной стеной** одним из +двух типов стыков, которые продукт обязан поддерживать наравне с +endpoint↔endpoint, а §13.1 п.5 требует отдельного regression-теста именно на +«partition→solid room wall». T-стык по определению происходит **вдоль стороны** +комнаты, а не в её углу — иначе это было бы endpoint↔endpoint, а не +endpoint↔line, и отдельной категории в #141 не потребовалось бы. Существующий +код-примитив `distToSegment`/`roomEdges` (`src/plan-snap-overlay.ts:313`, +`src/logic.ts`), уже используемый в редакторе для похожей задачи «точка близко +к стороне комнаты», тоже оперирует расстоянием до отрезка, а не до вершины — +то есть естественный примитив для этой проверки в кодовой базе уже есть, и он не +«ближайшая вершина». + +**Почему это блокирует однозначность AC2, а не деталь реализации.** AC2 требует +unit-тест именно на случай «ребро комнаты» как причину не сращивать. По +буквальной формулировке текущего §8.2 автор теста обязан проверять близость к +вершине полигона, а не к стороне — то есть тест для типичного T-стыка +(перегородка примыкает к середине комнатной стены, что и есть основной мотив +всей категории «endpoint↔line» из #141) при точном следовании ТЗ **не найдёт +причину оставить узел**, и AC2 в этой части будет доказывать не то поведение, +которое нужно. + +**Сценарий воспроизведения.** Комнатная стена — длинная сторона (10 м), её углы +далеко. Администратор дорисовывает независимую перегородку, конец которой +касается этой стены точно посередине (T-стык, ровно случай из #141 §4.1/§13.1 +п.5), и на этом же конце сходится вторая независимая перегородка, коллинеарная +первой и той же толщины — классический кейс §8.1. По текущему §8.2 расстояние +от точки стыка до **ближайшей вершины** полигона (≈5 м) заведомо больше +`EPS_JOIN` (доли шага сетки), значит «причины оставить узел» формально нет, и +слияние по §8.1 проходит — стирая узел ровно там, где комнатная стена +T-образно примыкает к перегородке. Это прямое попадание в риск №3 из §11 +(«Потеря узла, который был нужен») — риск, который сама находка M2 в r1 и +призывала закрыть однозначным критерием, но выбранный критерий закрывает его +только для углового случая, оставляя открытым основной, T-образный. + +**Что нужно.** Заменить «ближайшая вершина полигона» на «ближайшая точка на +любой стороне (ребре) полигона» — то есть на point-to-segment расстояние по +всем сторонам комнаты (`roomEdges` + `distToSegment`, тот же примитив, что уже +используется в `plan-snap-overlay.ts`), а не point-to-vertex. Это одна фраза в +§8.2, не расширяет и не сужает продуктовое решение владельца — чисто техническое +уточнение критерия, которое сам ревьюер может закрыть по инструкции ревью (не +продуктовый вопрос: пользователь не видит разницы между «стык у угла» и «стык +посередине стены» — оба должны одинаково сохранять узел). + +## Унаследовано из r1 (без повторной проверки) + +Документ: `docs/reviews/SPEC-REVIEW-229-r1.md`, SHA `81210f7`. Принято на веру, +так как дельта r2 эти места не касается: + +- Продуктовая рамка §1–§4 (персона, сценарий, три решения владельца + 2026-08-21) — не менялась, тело issue не менялось. +- §7 «Не входит в задачу», §9 (данные/i18n/a11y), §10 (performance), §16 + (откат) — не менялись дельтой. +- AC1, AC4, AC5, AC6, AC7, AC9 — их формулировки и способ доказательства не + затронуты правками M1/M2/M3, признаны однозначными и проверяемыми в r1. +- Соответствие обязательным разделам §7.1 PROCESS.md, отсутствие непомеченных + догадок за пределами §8.2/§8.4/§8.6 (проверено в r1 постранично) и + корректность `Touch editor: not exposed` по `docs/TOUCH-SUPPORT.md` §153 — не + переоценивались, дельта этих разделов не касается. +- Строки кода `wall-thickness.ts:1258`, `plan-optimizer.ts:402-530`, + `align-grid.ts:262`, `partition-openings.ts:43-90`, `wall-thickness.ts:743`, + `:2256`, проверенные в r1, дельтой не менялись — не перечитывались повторно. +- Согласованность с `docs/USER-GUIDE.ru.md` §8 — не переоценивалась, план + документации (§15 ТЗ) дельтой не менялся. + +## Что проверено и корректно (дельта r2) + +- Все три Medium-находки r1 закрыты по существу текстом ТЗ, а не заявлением + коммита — см. таблицу выше. M1 и M3 закрыты полностью и корректно. +- M2 закрыта частично: сам факт объявления единого допуска и явного критерия + для всех четырёх причин — правильный ответ на форму находки M2 («допуск не + назван»); ошибка — в конкретном выборе критерия для одной из четырёх причин + (см. Medium-1 этого раунда). +- Новые строки мутационного гейта (`partition-merge-skips-materialization` → + AC3, `chain-merge-sweeps-whole-space` → AC8) корректно привязаны к тем AC, + которые проверяют соответствующее поведение. +- AC3 переписан так, что явно требует падения теста при пересчёте только + `host.t` без материализации проекции — сохраняет дисциплину «тест должен + уметь падать», зафиксированную в r1. +- AC8 корректно расширен пунктом про перегородки вне компоненты связности — + соответствует новому §8.6. +- §17.4 честно помечен как «Ревизия r2» со ссылкой на находку M1 r1 — не + выдаёт правку за исходное решение. +- Построчные ссылки на код, тронутые дельтой (`:6538`, `:7824`, `:12007`), + точны на текущем SHA. + +## Чего не проверял + +- Продуктовую рамку и решения владельца §1–§4 — не менялись, наследуются из r1 + (см. выше). +- Реализацию — её не существует на этом этапе. +- Существующие смоки и `docs/specs/README.md` — как и в r1, вне зоны этого + ревью и не затронуты дельтой. +- Значения `EPS_ANGLE`/`EPS_JOIN` как конкретные числа — ТЗ намеренно не + фиксирует их (принятое предположение §17.3), это не находка. +- Критерий «колонна — её центр»: не нашёл прямого прецедента (существующего + кода снаппинга партиции к поверхности колонны, а не к центру), но и прямого + противоречия канону — данные модели (`WallColumnCfg.center`) и то, что + `align-grid.ts` снаппит и центр колонны, и концы перегородок на одну и ту же + сетку, делают «конец партиции == центр колонны» правдоподобной рабочей + конвенцией, в отличие от «ребра комнаты», где есть прямое, явное + противоречие канону (#141) и существующему код-примитиву. Не эскалирую как + находку — это осталось предположением, которое ревьюер не смог ни + подтвердить, ни опровергнуть уверенно; если оно неверно, всплывёт на + код-ревью через AC2. + +## Вердикт + +Одна Medium-находка в скоупе задачи (Medium-1), без High. Она — прямое +следствие правки M2 этого же раунда, а не унаследованная проблема, и чинится +одной фразой в §8.2 без изменения продуктового решения владельца. Итог — +жёлтый: возврат автору на правку ТЗ, фикс проходит r3 (лимит для обычного +трека — 4, израсходовано после этого раунда — 2).