diff --git a/docs/reviews/SPEC-REVIEW-223-r1.md b/docs/reviews/SPEC-REVIEW-223-r1.md new file mode 100644 index 00000000..80212a6c --- /dev/null +++ b/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).