docs: review document for #223

Issue: #223
User-Visible: no

docs/reviews/SPEC-REVIEW-223-r1.md

@@ -0,0 +1,178 @@
+# SPEC-REVIEW-223-r1
+
+- Issue: [#223](https://github.com/Matysh/houseplan-card/issues/223) «Оптимизировать планы» должна
+  канонизировать координаты, а не консервировать floating-point шум
+- ТЗ: [`docs/specs/223-optimize-coordinate-canonicalization.md`](../specs/223-optimize-coordinate-canonicalization.md)
+  на коммите `3a81dd6223095465f5563cf9cd76cd3ca5355bdb`
+- Трек: обычный (не `small`) — оценка владельца в аналитике: сложность/риск 4/10, одна общая
+  функция координат + изменения в `AlignReport`/`OptimizeReport` + пользовательский текст + несколько
+  доказательных поверхностей
+- Раунд: r1/4
+- Вердикт: **жёлтый**
+
+## Скоуп ревью
+
+Первый цикл — разбор полный, по всему ТЗ и по коду затронутой поверхности
+(`src/align-grid.ts`, `src/plan-optimizer.ts`, i18n-строка `gs.optimize_changes`,
+`docs/CANVAS.md` §9.3/§9.5, `docs/USER-GUIDE.ru.md` §19). Задача не помечена `small`, ТЗ
+существует файлом, что соответствует критерию — трек выбран верно.
+
+## Как проверялось
+
+1. `docs/SCOPE.md` — задача лежит в J6 («Keep the plan true as the home evolves»,
+   явное обслуживание плана уже входит в этот пункт), не расширяет продуктовый скоуп.
+2. `AGENTS.md`/`PROCESS.md` §7.1 — обязательные разделы ТЗ проверены построчно (сценарий,
+   что видит человек, причина, scope/non-scope, контракт, UX, данные/миграция, AC,
+   план тестов, риски, откат, release-артефакты — все присутствуют).
+3. Тело issue #223 и все три комментария (аналитика владельца, занятие, хендофф автора).
+4. `docs/USER-GUIDE.ru.md` §19 «Обслуживание планов» — текущая формулировка таблицы
+   («Комнаты — вершины округляются к сетке») и текст диалога.
+5. `docs/CANVAS.md` §9.2–§9.5 целиком — контракт grid-bound/wall-bound, идемпотентность,
+   `optimizePlans`/`alignAllToGrid` как чистые функции.
+6. Код: `src/align-grid.ts` (`snapN`, `note()`, все вызовы `snapN` по типам элементов —
+   room poly/rect, room_drafts, partitions, wall_columns, decor, layout), `src/plan-optimizer.ts`
+   (`canonicalized`/`meaningfulChanged`/стамп `model_version`), `src/logic.ts` (`snapToWall`,
+   подтверждение, что проёмы не используют `snapN`), `src/space-geometry.ts` (`GRID_N`,
+   `GRID_STEP_N` — подтверждает арифметику из тела issue), `src/i18n/{en,ru}.json`
+   (существующая строка `gs.optimize_changes` и её текущие параметры `m,c,w,s`),
+   `src/houseplan-card.ts` (рендер диалога, откуда видно, что `c` — это уже существующий
+   `r.canonicalized`, а не новый показатель), `test/align-grid.test.mjs`,
+   `test/plan-optimizer.test.mjs` (существующие покрытия и структура фикстур).
+7. Мутация арифметики из тела issue (241 узел решётки, `GRID_N=240`) — подтверждена
+   константами в `src/space-geometry.ts`.
+
+Ревью кода/AC не проводилось — этап `spec`, ручного запуска гейтов не требовалось.
+
+## Находки
+
+### Medium (в скоупе задачи) — AC4 не покрывает партиции и колонны, где `snapN()` вызывается безусловно, а результат применяется только условно
+
+`src/align-grid.ts:239-253` — единственное место, где вычисленный `snapN()` может быть
+**отброшен**: для `partitions` координаты `a`/`b` считаются всегда, но записываются в
+объект (`p.a = a; p.b = b;`) только если `snappedLength > EPS && hostedFit`. Контракт
+§6 ТЗ прямо требует третьего условия для счётчика — «результат действительно записан в
+candidate» — именно ради таких случаев. Но §10 описывает реализацию как «локальную
+tracked-обёртку над чистым `snapN()`», а обёртка, считающая на **входе** в вызов
+(естественная первая реализация), увеличит `coordsCanonicalized` для отброшенной
+партиции, нарушив собственный контракт ТЗ незаметно для проверяющего.
+
+AC4 называет доказательную матрицу: «Units для room poly, rect/decor/layout и off-grid
+negative case» — `partitions` и `wall_columns` в этот список не входят, хотя оба
+являются grid-bound элементами по `docs/CANVAS.md` (раздел «Independent wall geometry»,
+`partitions[].a/b`, `wall_columns[].center`) и оба — реальные вызовы `snapN()` в этом же
+файле (`align-grid.ts:239-240` и `:262`). Код-ревью, сверяющее AC ровно по названному в
+ТЗ списку тестов, примет реализацию, у которой партиции считаются неверно, потому что
+эта ветка кода не названа ни в одном AC.
+
+**Воспроизведение (почему это не гипотетический краевой случай):** партиция с проёмом,
+чей `t`/`length` не помещается в границы после снапа стены (`hostedFit === false`) —
+обычная ситуация при явном Optimize старого плана, где стена сама сдвигается. `a`/`b`
+для такой партиции остаются шумными (что корректно — правка отклонена), но наивная
+tracked-обёртка всё равно засчитает эти два компонента в `coordsCanonicalized`, хотя
+candidate их не получил. Пользователь увидит положительный счётчик «канонизировано
+координат» для координат, которые физически не изменились в записи.
+
+**Фикс:** добавить в AC4 явное требование теста на партицию/колонну, для которой снап
+вычислен, но не применён (та же ветка `else d = 0`, что уже используется для `moved`) —
+счётчик должен остаться на прежнем значении для этого компонента. Технически это
+решает автор (порядок вызова обёртки vs. присвоение), но без явного AC эта ветка кода
+не гарантированно будет проверена ни тестом, ни рецензентом.
+
+### Medium (в скоупе задачи) — новый показатель делит слово «канонизировано» с уже существующим показателем другого смысла в той же строке диалога
+
+`src/i18n/ru.json:785` / `src/i18n/en.json:785` — действующая строка `gs.optimize_changes`
+уже содержит «канонизировано планов: {c}» / «plans canonicalized: {c}», где `c` —
+это `OptimizeReport.canonicalized` из `src/plan-optimizer.ts:404-500`: число
+**пространств**, у которых изменилось JSON-представление `open_spans`/`open_to`-связей/
+`walls` после rekey. Это уже существующий, отдельный от геометрии показатель.
+
+ТЗ §7 предлагает добавить в **ту же общую строку** («в общей строке всегда, включая
+ноль, так же как действующие счётчики миграций/планов/стен/виртуальных фрагментов»)
+третий показатель с тем же корнем — RU «канонизировано координат: {p}», EN
+«coordinates canonicalized: {p}». В итоге один и тот же диалог, в одном и том же
+предложении, дважды использует «канонизировано» для двух несвязанных единиц счёта
+(пространства vs. отдельные координатные компоненты). Это ровно тот отчёт, для
+которого `docs/CANVAS.md` §9.5 и `AUD-158B1-01` в шапке `align-grid.ts` требуют
+точности как условия доверия («A number that is merely typical... is worse than no
+number at all»): диалог — единственный гейт перед действием без undo per-объектно,
+и админ, сверяющий два числа с одинаковым словом, разумно может решить, что это
+одна и та же категория или что числа должны совпадать/суммироваться.
+
+ТЗ не упоминает этот уже существующий показатель `c` вовсе (ни в контракте §6, ни в
+рисках §11), хотя он находится в той же строке, которую §7 правит. Риск в §11
+("Счётчик вводит пользователя в заблуждение как число объектов") адресует другую
+путаницу (координаты vs объекты), но не эту.
+
+**Фикс (в скоупе, решает автор):** развести термины — например «канонизировано
+пространств: {c}» для существующего показателя и оставить «канонизировано координат:
+{p}» для нового, либо иначе разграничить формулировки так, чтобы два числа в одном
+предложении не делили корень для разных единиц. Строка `gs.optimize_changes` в любом
+случае правится этой задачей, так что это не выход за её границы.
+
+## Что проверено и корректно
+
+- **Причина и цифры воспроизведения (issue, раздел «Причина»)** — подтверждены
+  чтением `src/align-grid.ts:62-72`: `EPS = GRID_STEP_N * 1e-6 ≈ 4.17e-9`,
+  `snapN` действительно возвращает `v` при `|s-v| <= EPS`. Заявленный порядок
+  расхождения (`5.5e-17` против порога `~4.2e-9`, разница ~10⁸×) арифметически
+  верен.
+- **Контракт снапа (§6 ТЗ)** — `Math.round(v / GRID_STEP_N) * GRID_STEP_N`, `NaN`/
+  `Infinity` не трогаются — совпадает с текущим кодом `snapN()` построчно (меняется
+  только ветвление возврата, не формула узла).
+- **Идемпотентность после фикса рассуждением** — если первый прогон записал ровно
+  вычисленный узел `s = round(v/step)*step`, то повторный вызов той же формулы над
+  тем же `s` детерминированно вернёт `s` с разницей `0`, что по контракту §6 п.1
+  («результат... численно отличается от входа») не считается канонизацией —
+  `coordsCanonicalized: 0` на втором прогоне (AC5) логически следует из формулировки
+  контракта, а не является отдельным недоказанным утверждением.
+- **Проёмы верно исключены из нового счётчика** (§6: «Изменение угла проёма...
+  не входят в новый счётчик») — подтверждено чтением `snapToWall()` в `src/logic.ts:166-206`:
+  проекция на стену использует собственную квантизацию смещения вдоль стены, не
+  вызывает `snapN()` вовсе, что согласуется с WALL-BOUND правилом `docs/CANVAS.md` §9.3.
+- **`snapN()` не вызывается за пределами `align-grid.ts`** (кроме тестов) — grep
+  подтверждает, что расширение поведения не задевает live-снаппинг редактора
+  (`_snap`/`snapToGrid`/`snapR` в других файлах — отдельные функции), что согласуется
+  с Non-scope п.1 («без автоматической канонизации на Save/импорте/рендере»).
+  AC8 корректно выделяет это в отдельный негативный тест.
+- **Расчёт `moved`/`maxShift*` не растёт от канонизации** — `note()` (`align-grid.ts:169-175`)
+  фильтрует по `d > EPS`; после фикса разница `d` для «уже почти на узле» координаты
+  останется `<= EPS`, то есть `moved` не увеличится — контракт §6 п. «`maxShift`,
+  `maxShiftCm` и `maxSpace` не растут от замен в пределах EPS» реализуем без
+  дополнительных изменений `note()`.
+- **Формат AC** — все десять критериев проверяемы и у каждого указан способ
+  доказательства (unit/regression/idempotence/i18n+smoke/existing suite/docs+bundle/gates),
+  что удовлетворяет DoR §2.5.
+- **Отсутствие догадок, выданных за факт** — контракт §6 и границы §4/§5 совпадают
+  с уже задокументированным поведением `docs/CANVAS.md` §9.3/§9.5, а не изобретают
+  новое; единственные пять пунктов, отмеченных как решения автора, помечены явно
+  в §13 и являются техническими (не продуктовыми), что соответствует правилу
+  PROCESS.md §7.1.
+- **Продуктовых вопросов владельцу нет** — сценарий, видимый результат и границы
+  зафиксированы однозначно в аналитике владельца; согласуется с J6.
+- **Ссылка issue ↔ ТЗ** — `docs/specs/README.md` обновлён в том же коммите,
+  таблица содержит строку на #223.
+- **Release-артефакты (§12 ТЗ)** — список документов для правки (CHANGELOG ru/en,
+  USER-GUIDE.ru.md, CANVAS.md, TESTING.md, STATUS.md) покрывает все места, где
+  сейчас зафиксировано текущее (неверное) поведение; ни один канонический документ
+  не остаётся расходящимся с новым контрактом.
+
+## Чего не проверял
+
+- Не проверялся сам код реализации — он не написан на этапе `spec`; находки выше
+  относятся к пробелам в AC/контракте ТЗ, а не к дефектам существующего кода.
+- Не запускались тесты/гейты — не требуется на этапе ревью ТЗ.
+- Не проверялась точная фикстура «6 комнат владельца» из #218 — координаты не
+  воспроизводились из треда #218 построчно; AC3 сформулирован достаточно строго
+  (`changed:true`, `moved:0`, `coordsCanonicalized>0`, `union` проходит), чтобы быть
+  проверяемым независимо от того, откуда именно автор возьмёт числа.
+- Не оценивалось golden-влияние построением скриншота — принято на веру рассуждение
+  ТЗ «компоновка не меняется, добавляется текст в существующую строку»; риск
+  минимальный и явно закрыт пререлизным `golden:verify` по процессу.
+
+## Вердикт
+
+Вердикт: жёлтый · цикл r1/4 · High: 0 · Medium: 2 → в задаче
+
+Обе находки Medium и в скоупе текущего issue (правят ровно тот AC4 и ту же
+i18n-строку `gs.optimize_changes`, которые уже переписывает эта задача) — чинятся
+в ТЗ без отдельного issue (владелец, 2026-08-19, #202).