docs: review document for #248

Issue: #248
User-Visible: no

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

@@ -0,0 +1,171 @@
+# SPEC-REVIEW-248-r1
+
+- Issue: [#248](https://github.com/Matysh/houseplan-card/issues/248) — «Оптимизировать» не идемпотентна после записи и reload
+- ТЗ: `docs/specs/248-optimize-idempotence.md`, ветка `issue/248-optimize-idempotence`, коммит `4d73d031a477c06c5b28366bd9e3883ebec421f1`
+- Этап: spec (PROCESS.md §2.4), заход r1, трек обычный (не `small`)
+- Вердикт: **зелёный**
+
+## Скоуп ревью
+
+Первый заход — разбор полный, разделов «Закрытие раунда» и «Унаследовано» нет
+(PROCESS.md §2.10 применяется со второго цикла).
+
+Проверено по порядку из инструкции:
+
+1. `docs/SCOPE.md` — сценарий закрывает J6 («Keep the plan true as the home
+   evolves»); действие administrative/desktop-first, View/kiosk не задеты.
+2. `AGENTS.md`, `PROCESS.md` §7.1, §2.4, §2.5 — обязательные разделы ТЗ,
+   классы файлов, критерии DoR.
+3. Тело issue #248 и оба комментария (аналитика владельца, хендофф автора ТЗ).
+4. `docs/USER-GUIDE.ru.md` — раздел «Что делает оптимизация» (строки
+   1360–1409), терминология «сдвинуто/устранён шум/повторный Optimize».
+5. Канонический документ подсистемы — `docs/CANVAS.md` §9.5, плюс
+   `docs/CONFIG-COMPATIBILITY.md` раздел «Canonical geometry on write (#224)».
+
+## Как проверялось
+
+Ревью состязательное: ТЗ читалось без устных пояснений автора, каждое
+техническое утверждение ТЗ (§3 «Подтверждённая причина», §6 «Контракт
+поведения», §13 «Принятые предположения») сверялось с актуальным кодом на
+`HEAD`, а не принималось на слово.
+
+Прочитан код:
+
+- `src/coordinate-canonicalization.ts` (полностью) — allowlist и формула
+  `canonicalizeNumber` (floor(|v|·10⁹+0.5)/10⁹, `COORDINATE_DECIMALS = 9`).
+- `src/plan-optimizer.ts` (полностью) — `optimizePlans()`: `changed`
+  вычисляется как `JSON.stringify(config) !== original`, где `original =
+  JSON.stringify(configIn)`, и **нигде** не пропускается через
+  `canonicalizeConfigGeometry`/`canonicalizeLayoutGeometry`. Это подтверждает
+  корневую причину ТЗ буквально: pure-результат `snapN()` (шаг `1/240`,
+  двоичное число) и то, что вернёт `config/get` после девятизнакового
+  round-trip storage-контракта #224, — два разных JSON-представления одного
+  и того же логического значения.
+- `src/align-grid.ts` (основная часть) — `snapN()`, `GRID_STEP_N`, `EPS`,
+  перечень grid-bound/wall-bound элементов (room poly/rect, room_drafts,
+  partitions, wall_columns, decor, openings) — совпадает с матрицей AC2.
+- `custom_components/houseplan/websocket_api.py:1554-1666` (`ws_plan_optimize`)
+  — `pending["config"]`/`pending["layout"]` канонизируются явно (`canonicalize_
+  config_geometry(msg["config"])`), а фактическая запись идёт через
+  `async_save_config_state()` / `async_save_layout_state()`.
+- `custom_components/houseplan/store.py:150-229` — оба метода канонизируют
+  геометрию **внутри себя** (`layout_store_payload` → `canonicalize_layout_
+  geometry`, `async_save_config_state` → `canonicalize_config_geometry`)
+  независимо от того, что передал вызывающий код.
+- `custom_components/houseplan/__init__.py:184` (startup finisher,
+  `optimize_pending`) — путь recovery, на который ссылается AC3, существует.
+- `test/plan-optimizer.test.mjs`, `test/align-grid.test.mjs`,
+  `test/fixtures/coordinate-canonicalization.json` +
+  `test/coordinate-canonicalization.test.mjs` +
+  `tests_backend/test_coordinate_canonicalization.py` — подтверждают, что
+  паттерн «одна JSON-фикстура, читаемая Node и Python независимо, без
+  вызова одного рантайма из другого» (§13.2 ТЗ) уже используется в проекте
+  для ровно той же канонизации, а не изобретается заново.
+
+Вывод по существу: **исходная гипотеза issue («сырая запись против
+канонизированного pending») в актуальном `dev` не подтверждается — ровно как
+и написал автор в аналитике и в ТЗ §3.** Обе фактические записи
+(`async_save_config_state`, `async_save_layout_state`) канонизируют геометрию
+сами, независимо от вызывающего кода. Настоящая причина —
+несовместимость двух корректных по отдельности числовых контрактов (шаг
+сетки `1/240` не представим точной десятичной дробью, storage округляет её
+до 9 знаков), и именно она воспроизводится по коду. ТЗ не выдаёт догадку за
+факт: раздел §13 явно маркирует технические решения как «принято
+предположительно», а корневая причина подтверждена чтением, а не заявлена.
+
+## Разделы §7.1 — проверка полноты
+
+Все обязательные разделы присутствуют и не пусты: сценарий (§1) · что человек
+увидит до/после (§2) · проблема/причина (§3) · scope/non-scope (§4–5) ·
+контракт поведения (§6) · UX/i18n/touch (§7) · модель данных и совместимость
+(§8) · AC1…AC6 с доказательством (§9) · план автотестов (§10) · риски/perf/
+security/rollback (§11) · release-артефакты (§12) · принятые технические
+предположения (§13). Продуктовых вопросов владельцу нет — по инструкции
+такое допустимо, когда ожидаемое поведение уже зафиксировано (здесь —
+`docs/CANVAS.md` §9.5 и текущий текст `USER-GUIDE.ru.md`, см. ниже).
+
+## AC — однозначность и способ доказательства
+
+| AC | Однозначен? | Доказательство названо и выполнимо? |
+|---|---|---|
+| AC1 | Да — «changed:false, нулевые счётчики, deep-equal» проверяемо программно | unit + mutation guard на «убрать финальную канонизацию» — файл `test/plan-optimizer.test.mjs` существует, фикстуры названы |
+| AC2 | Да — конкретная матрица geometry-типов и `cell_cm` | unit parameterized + существующий `test/align-grid.test.mjs` + mutant «вернуть сырой 1/240» |
+| AC3 | Да — exact-equal pending/final/recovery pair | backend pytest, startup finisher (`__init__.py:184`) реально существует |
+| AC4 | Да, включая явную оговорку «без Node subprocess из pytest» (§13.2) | общая fixture по прецеденту `coordinate-canonicalization.json`; парность фикстур — новый, но понятный guard |
+| AC5 | Частично составной (browser smoke + чтение кода Undo lifecycle), но каждая половина названа отдельно | targeted smoke с mocked WS + explicit «проверено чтением» для Undo — соответствует §18 PROCESS.md |
+| AC6 | Да, стандартная формулировка | typecheck/unit/build + bundle parity; golden/smoke/perf — предрелizный гейт по правилу «полные наборы — не гейт ревью» |
+
+Ни один AC не содержит скрытого домысла о поведении, которого нет в
+канонических документах: контракт §6.1–6.4 — прямое следствие уже
+существующего `docs/CANVAS.md` §9.5 («every pass is idempotent») и
+`docs/CONFIG-COMPATIBILITY.md` («A repeated canonical Save … is a no-op»),
+только явно распространённое на write/reload boundary, которого раньше не
+было в явном виде.
+
+## Находки
+
+Блокирующих (High) находок нет. Находок Medium в скоупе или вне скоупа нет.
+
+### Low — не блокирует, оставлено на усмотрение автора
+
+1. **§12 ТЗ формулирует обновление `docs/USER-GUIDE.ru.md` так, будто факт
+   идемпотентности сейчас не документирован**, а строка 1380 текущего
+   `USER-GUIDE.ru.md` уже утверждает «Повторный Optimize над результатом
+   ничего не предлагает» — то есть документ уже обещает то поведение, которое
+   чинит эта задача, только без явного упоминания границы reload/server-event.
+   Не искажает контракт и не создаёт риска для AC — при реализации это,
+   вероятно, точечное уточнение одной фразы, а не новый раздел. Снимается
+   автором по факту правки; фиксирую, чтобы это не создало неверного
+   впечатления при код-ревью, что документация была неверна и её было нужно
+   переписывать заново.
+
+## Что проверено и корректно
+
+- Корневая причина (§3 ТЗ) подтверждена чтением actual `dev`, а не
+  унаследована из первоначальной (опровергнутой) гипотезы issue.
+- Контракт §6 не противоречит `docs/CANVAS.md` §9.5 и
+  `docs/CONFIG-COMPATIBILITY.md` — расширяет их на write/reload boundary.
+- Non-scope (§5) корректно исключает изменение шага сетки, точности
+  канонизации #224, миграций и визуальных изменений — задача не расширяет
+  скоуп бага в рефакторинг.
+- Тестовый план (§10, AC1–AC4) ссылается на реально существующие файлы
+  (`test/plan-optimizer.test.mjs`, `test/align-grid.test.mjs`,
+  `test/fixtures/coordinate-canonicalization.json`) и на реально существующий
+  прецедент общей Node/Python-фикстуры, а не на гипотетический паттерн.
+- Backend-путь (`ws_plan_optimize`, `async_save_config_state`,
+  `async_save_layout_state`, startup finisher) существует ровно там, где ТЗ
+  на него ссылается, с теми же именами функций.
+- Release-артефакты (§12), rollback (§11) и i18n/touch (§7) — не создают
+  новых миграций, флагов или UI-контрактов; согласуется с оценкой сложности
+  4/10 и `User-Visible: yes` для будущего implementation-коммита.
+- Роли и трейлеры: коммит `4d73d03` несёт `Issue: #248` и
+  `User-Visible: no` (корректно — это документация ТЗ, не поведение); автор
+  ТЗ и ревьюер — разные модели (Codex/Claude), правило PROCESS.md §6
+  соблюдено.
+
+## Чего не проверял
+
+- Не запускал никаких гейтов (`typecheck`/`test`/`build`) — на этапе spec-review
+  предмет проверки текст ТЗ и его соответствие коду/документам, а не
+  implementation; кода поведения ещё нет (класс A файлов в диффе этого
+  коммита нет — только `docs/specs/**`).
+- Не проверял `custom_components/houseplan/validation.py:600`
+  (`_dedupe_open_spans`), упомянутый в исходном issue как альтернативная
+  гипотеза (нормализация проёмов на записи) — ТЗ явно отводит эту гипотезу
+  (§3: «предложенная в исходном описании причина... не подтверждается») и не
+  включает её в scope; сама эта альтернатива не стала предметом контракта,
+  поэтому её код не требовался для проверки полноты ТЗ.
+- Не оценивал производительность реализации (нет кода) — риск-таблица §11
+  ТЗ содержит явную оценку (`O(n)` copy-on-write), достаточную для DoR.
+- Не проверял i18n-файлы построчно — ТЗ утверждает «новых строк нет», что
+  проверяемо тривиально на этапе код-ревью через diff `src/i18n/*.json`.
+
+## Итог
+
+ТЗ выполнимо, каждый AC проверяем и снабжён способом доказательства,
+корневая причина подтверждена по коду, а не по доверию к автору или к
+первоначальной (опровергнутой) гипотезе issue. Скоуп не расширяется и не
+сужается относительно реального дефекта. Единственная находка — Low,
+не блокирует переход в «Готово к разработке».
+
+**Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0**