diff --git a/docs/reviews/SPEC-REVIEW-229-r1.md b/docs/reviews/SPEC-REVIEW-229-r1.md new file mode 100644 index 00000000..4c2137f6 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-229-r1.md @@ -0,0 +1,246 @@ +# SPEC-REVIEW-229-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/229 +- **ТЗ:** `docs/specs/229-merge-collinear-partitions.md` +- **Ветка / SHA:** `issue/229-merge-collinear-partitions` @ `81210f7` +- **Этап:** spec (PROCESS.md §2.4) · трек: обычный (не `small`) +- **Заход:** r1 · блокирующих циклов израсходовано 0/4 до этого вердикта +- **Ревьюер:** Claude, роль «ревьюер ТЗ» (отдельная от роли аналитика/автора) + +## Скоуп + +Оценивалось только ТЗ issue #229 — сращивание коллинеарных независимых +перегородок одинаковой толщины при завершении цепочки стен, плюс аналогичное +слияние в «Оптимизировать планы» для уже нарисованных планов. Код не менялся и +не оценивался (правильный этап — S4, а не S7); объектом ревью является +исполнимость и проверяемость ТЗ, а не корректность отсутствующей реализации. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (полностью, включая §2.10, + §4, §7.1, §8) — это первый цикл (r1), правило «по дельте» не применяется. +2. Прочитано тело issue #229 и оба комментария (аналитика → `S3-spec`, автор ТЗ + → `S4-spec-review`), продуктовые решения владельца от 2026-08-21 (§4 ТЗ). +3. Прочитан сам файл ТЗ целиком (`docs/specs/229-merge-collinear-partitions.md`). +4. Проверены построчно все фактические ссылки на код, которые ТЗ приводит как + подтверждение причины и контракта: `houseplan-card.ts:6538` (`_finishWallChain`, + реальный номер строки метода — 6538, не 6558 как в теле issue; в самом файле + ТЗ строка не называется, см. находку L1), `wall-thickness.ts:1258` + (`normalizeWallIntervals`), `plan-optimizer.ts:402-530` (`wallsMerged`, + `spansMerged`, отчёт), `align-grid.ts:262` (только snap независимых + перегородок, без слияния), `partition-openings.ts:43-90` + (`resolvePartitionOpening`, `host.t`, `missing-partition`), + `wall-thickness.ts:743` и `:2256` (два разных допуска коллинеарности/смежности). + Все ссылки, кроме одной, точны. +5. Прочитан canonical `docs/WALL-THICKNESS.md` §9 (Independent partitions, + drafts and columns) и `docs/CONFIG-COMPATIBILITY.md` (Independent-wall opening + host, #132) — они определяют существующий контракт хостов проёмов, с которым + должно согласовываться §8.4 ТЗ. +6. Прочитан прецедентный код записи, который уже пересчитывает материализованную + проекцию проёма при геометрическом изменении хозяина: перетаскивание + перегородки (`houseplan-card.ts:7809-7826`) и прямое редактирование проёма + (`:11996-12007`) — оба вызывают `materializePartitionOpening` после + `resolvePartitionOpeningCompat`. +7. Проверена корректность строки `Touch editor: not exposed` по + `docs/TOUCH-SUPPORT.md` (документационное правило, строка 153) — значение из + допустимого набора, обоснование («не добавляет жестов») соответствует тому, что + вся фича живёт только на десктопном приёме цепочки в режиме «Стены». +8. Проверена согласованность с `docs/USER-GUIDE.ru.md` (раздел 8, «Комнаты и + стены») — существующее описание поведения («сегменты сохранятся обычными + независимыми стенами») не противоречит будущему («меньше независимых стен»), + план обновления документации (§15 ТЗ, одна строка) достаточен. +9. Код не запускался, гейты не гонялись — реализации нет, оценивать нечего; + на этапе spec-review это не требуется (PROCESS.md §2.4/§8 говорят о коде). + +## Находки + +### Medium-1 — момент слияния при завершении цепочки шире, чем решение владельца о старых планах + +**Файл:** `docs/specs/229-merge-collinear-partitions.md`, §17 «Принятые +предположения», п.4. + +**Формулировка ТЗ:** «Слияние при завершении цепочки применяется ко всему +пространству, а не только к новым записям: цепочка могла примкнуть к +нарисованному ранее отрезку, и шов на стыке — тот же самый случай.» + +**Проблема.** Владелец явно разделил два случая (§4 ТЗ, решения 2026-08-21): +(1) слияние новой цепочки — сразу; (2) слияние **уже нарисованных планов** — +только явным действием «Оптимизировать планы», с отчётом и отменой. Пункт 4 +раздела «Принятые предположения» помечен как техническое решение («менять +свободно»), но по факту расширяет действие правила 1 на данные, подпадающие под +правило 2: он не ограничивает слияние компонентой, связанной с только что +нарисованной цепочкой, а сканирует **всё пространство**. + +**Сценарий воспроизведения.** В пространстве уже лежат 9 перегородок из экспорта +владельца (#228, `1.json`), среди них коллинеарная пара `#0`/`#4` — то есть ровно +тот случай, который ТЗ само приводит как мотивирующий пример и который решение +владельца п.2 явно откладывает до «Оптимизировать планы». Администратор рисует +новую, никак не связанную стену в другом углу того же пространства и завершает +цепочку. По формулировке п.4 слияние пройдёт по всему пространству и сотрёт пару +`#0`/`#4` тоже — без отчёта и без выделенной отмены, которые п.2 обещает именно +для этого случая. Единственная защита — общая история Ctrl+Z, которая отменит +весь акт завершения цепочки (включая саму новую стену), а не только неожиданный +побочный эффект. + +**Почему это находка, а не варьируемая деталь реализации.** Оправдание в тексте +(«цепочка могла примкнуть к нарисованному ранее отрезку») описывает узкий случай +— новый сегмент касается старого и продолжает его. Формулировка реализации шире +оправдания: она не привязана к связности с новой цепочкой. Это меняет объём +видимых изменений (§7.1 PROCESS.md: «какой объём видимых изменений входит в этот +issue» — ровно тот класс, который решает владелец), а не техническую деталь типа +имени модуля или выбора выжившего `id`. + +**Что нужно.** Одно из двух: (a) сузить п.4 до компоненты связности, содержащей +концы только что завершённой цепочки (транзитивно через общие концы), тогда +поведение полностью укладывается в решение владельца п.1 и вопрос снимается без +эскалации; (b) если задуман действительно широкий охват (он и полезнее — план +чище), явно спросить владельца одним вопросом с предложенным дефолтом («слияние +при завершении цепочки задевает не только новый сегмент, а всё пространство — +ок?»), как требует §7.1. + +### Medium-2 — допуск «есть причина оставить узел» не определён для трёх из четырёх причин + +**Файл:** `docs/specs/229-merge-collinear-partitions.md`, §8.2 и §8.3. + +**Проблема.** §8.3 заявляет «один источник истины» для допусков, но называет +только два: `EPS_ANGLE` (коллинеарность) и `EPS_JOIN` (совпадение концов двух +*перегородок*). §8.2 перечисляет четыре причины оставить узел: третья +перегородка, **ребро комнаты**, **колонна**, **конец черновика**. Для первой +причины допуск неявно тот же `EPS_JOIN` (стык двух перегородок и есть предмет +§8.1.3). Для трёх остальных ТЗ не говорит, каким допуском и по какому +геометрическому критерию («точка на точке» vs «точка на отрезке» для ребра +комнаты) устанавливается совпадение. + +**Почему это блокирует однозначность AC.** AC2 требует четыре отдельных unit-теста +(«ребро комнаты», «колонна», «конец черновика», третья перегородка) и заявлен как +доказываемый `unit`, но без зафиксированного критерия автор теста выбирает допуск +по своему усмотрению — один из тех самых «плавающих» допусков, от которых §8.3 +явно старается защититься («иначе на границе слияние станет непредсказуемым»). +Конкретный риск: если для колонны или ребра комнаты возьмут допуск строже +`EPS_JOIN`, легитимный ULP-шум (риск, явно описанный в самом ТЗ со ссылкой на +#218/#223/#224) заставит узел остаться там, где физически есть только шум +координат, а не колонна; если допуск шире `EPS_JOIN`, слияние может пройти сквозь +примыкание, которое реально существует, но чуть-чуть не долетело до совпадения +концов — прямое нарушение риска №3 из §11. + +**Что нужно.** Одна фраза в §8.2 или §8.3: причины «ребро комнаты», «колонна», +«конец черновика» проверяются тем же `EPS_JOIN` (точка-в-допуске к общему концу +перегородки), либо явно называется другой, но такой же единый допуск с +обоснованием, почему он должен отличаться. + +### Medium-3 — §8.4 не упоминает материализованную legacy-проекцию проёма (`x/y/angle`) + +**Файл:** `docs/specs/229-merge-collinear-partitions.md`, §8.4, §9, AC3. + +**Проблема.** `docs/CONFIG-COMPATIBILITY.md` («Independent-wall opening host, +#132») документирует контракт: у хостованного проёма легacy-поля `x/y/angle` +— это **материализованная проекция** для старых фронтендов, которая должна +оставаться синхронной с `host.id`/`host.t`. Это не гипотеза, а действующий +прецедент в коде: перетаскивание перегородки (`houseplan-card.ts:7809-7826`) и +прямое редактирование проёма (`:11996-12007`) оба вызывают +`resolvePartitionOpeningCompat` → `materializePartitionOpening` сразу после +изменения геометрии хозяина, специально чтобы не оставить проекцию устаревшей. + +§8.4 ТЗ описывает пересчёт `host.t` и переписывание `host.id`, но ни словом не +упоминает пересчёт `x/y/angle`. §9 («Данные...») утверждает «формат перегородки +не меняется; меняется их количество и `host.id` части проёмов» — и тоже не +называет legacy-проекцию. AC3 проверяет «координаты центра проёма в единицах +плана» и `host.t ∈ [0,1]` — то есть **резолвленную**, а не материализованную +позицию; тест по AC3 в его текущей формулировке может стать зелёным при устаревших +`x/y/angle`. + +**Сценарий воспроизведения.** Слияние пересчитывает `host.t` двери на новую длину +хозяина, но материализованные `x/y/angle` остаются от старой перегородки. +Дверь физически на месте для текущего фронтенда (он всегда резолвит через +`host`), но откат на старый фронтенд (или любой код, который по прецеденту +CONFIG-COMPATIBILITY.md вправе читать `x/y/angle` напрямую) увидит дверь там, где +она была до слияния — ровно тот класс проблемы, для которого прецедентный код +уже существует и который #132 называет явно. + +**Что нужно.** Добавить в §8.4 шаг «для каждого пересчитанного проёма +материализовать `x/y/angle` тем же вызовом, что использует перетаскивание +перегородки», и явно упомянуть это в §9. Не обязательно расширять сам AC3 (там +всё ещё резолвленная позиция — правильный инвариант), но стоит добавить это +как отдельную строку либо в AC3, либо отдельным AC, иначе ревьюер кода не будет +знать, что искать в реализации. + +## Что проверено и корректно + +- Все требуемые по §7.1 разделы по существу присутствуют: сценарий и персона + (§1), видимое изменение (§2), причина/проблема (§3), скоуп/не-скоуп (§6–7), + контракт поведения (§8), данные/i18n/a11y (§9), performance (§10), риски + (§11), AC1…AC9 с доказательством (§12), план автотестов (§13), мутационный + гейт (§14), release-артефакты (§15), откат (§16), явный блок предположений + (§17). Заголовки не совпадают буквально с шаблоном, но это стандартная для + репозитория практика (см. `223-optimize-coordinate-canonicalization.md`, + `226-entity-parent-dedup.md`) — не находка. +- Продуктовая рамка (§1–2) отвечает на оба обязательных вопроса — какая персона + на какой поверхности и что человек видит без терминов реализации. +- Заявленная причина и асимметрия (§3) — не догадка: обе половины (наличие + `normalizeWallIntervals`/`wallsMerged` для стен комнат и его отсутствие для + независимых перегородок) подтверждены чтением `wall-thickness.ts`, + `plan-optimizer.ts`, `align-grid.ts`; номера строк точны, кроме одной (см. + выше — L1, не блокирует, разобрано отдельно как низкая находка). +- Три продуктовых решения владельца (§4) взяты из issue буквально, не + переинтерпретированы — кроме расширения решения №2 через техническое + предположение №4 (Medium-1 выше). +- AC1, AC4, AC5, AC6, AC7, AC8, AC9 однозначны, проверяемы и указывают способ + доказательства; AC3 явно требует «тест красный до реализации §8.4» — то самое + требование «тест должен уметь падать», зафиксированное заранее, а не постфактум. +- Допуски (§8.3) для собственно решения «сращивать/не сращивать» две + перегородки заданы через единые именованные константы, выраженные в долях шага + сетки (а не абсолютно) — правильная реакция на разброс `cell_cm` 1–25 (#230) и + на ULP-шум (#218/#223/#224). +- Границы применения (§8.5): транзитивное слияние до стабилизации и + независимость от порядка обхода — оба явно проверяются отдельными AC + (входят в AC1/AC6), а не декларируются без доказательства. +- Мутационный гейт (§14) покрывает по одной репрезентативной мутации на каждый + из четырёх названных рисков (§11) — стандартный для репозитория уровень + строгости, не исчерпывающий по всем под-случаям AC2, и это нормально (не + находка). +- `Touch editor: not exposed` — верно выбранное значение из + `docs/TOUCH-SUPPORT.md` §153 при отсутствии новых жестов. +- Не входит в задачу (§7): стены комнат, колонны, черновики контуров, + промахи примыкания (#228 п.2) — верно отнесены к соседним причинам/задачам, не + присвоены этой. +- Release-артефакты (§15) называют оба changelog, обновление + `docs/USER-GUIDE.ru.md` и явно отмечают отсутствие влияния на golden — сверено + с текущим содержанием гайда (раздел 8), противоречия нет. + +## Мелкая находка (Low, снимаю с записью, не блокирует) + +**L1.** Тело issue цитирует `_finishWallChain` как `houseplan-card.ts:6558`; в +файле метод объявлен на `6538`. Разница в 20 строк — комментарии/боевой код +сдвинулись между моментом анализа и моментом чтения. Не влияет на понимание или +проверяемость ТЗ (сам файл ТЗ конкретную строку не называет), исполнитель найдёт +метод по имени. Снимаю без действия. + +## Продуктовые вопросы владельцу + +Формально ни один из вопросов выше не обязан идти владельцу — оба фиксируемых +пункта (Medium-1, Medium-2) решаются техническим сужением формулировки в ТЗ, без +изменения того, что видит пользователь. Единственная точка, где владелец мог бы +захотеть высказаться, — Medium-1, если авторы предпочтут вариант (b) (широкий +охват всего пространства) вместо сужения (a); в этом случае вопрос будет +сформулирован автором ТЗ во втором заходе с предложенным дефолтом, как того +требует §7.1. Ревьюер вопросов владельцу сейчас не задаёт (согласно инструкции — +такие вопросы снимаются и решаются по существу в вердикте). + +## Чего не проверял + +- Реализацию — её не существует на этом этапе; код-ревью будет отдельной сессией + после `S5-ready` → `S6-in-progress` → `S7-code-review`. +- Содержимое `1.json` из #228 (конкретные координаты пары `#0`/`#4`) — принято на + веру как результат анализа предыдущего этапа (аналитика уже это подтвердила + чтением экспорта), пересчёт координат вручную не входит в задачу ревью ТЗ. +- `docs/specs/README.md` регистрацию — автор заявил, что запись добавлена; + не влияет на вердикт содержания ТЗ, отдельно не сверял. +- Существующие смоки (`smoke_subarea` и смоки рисования, упомянутые в §13) — + их код не читал; ТЗ верно указывает, что они должны быть прогнаны без правок + на этапе реализации, это забота код-ревью, не ТЗ-ревью. + +## Вердикт + +Медиум-находки все в скоупе задачи и чинятся правкой ТЗ, без блокирующих (High) +находок. Итог — жёлтый: возврат автору на правку ТЗ, фикс проходит повторный +цикл ревью (лимит для обычного трека — 4, израсходовано после этого раунда — 1).