diff --git a/docs/reviews/SPEC-REVIEW-248-r1.md b/docs/reviews/SPEC-REVIEW-248-r1.md new file mode 100644 index 00000000..1100edde --- /dev/null +++ b/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**