diff --git a/docs/reviews/SPEC-REVIEW-224-r1.md b/docs/reviews/SPEC-REVIEW-224-r1.md new file mode 100644 index 00000000..42ae1db1 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-224-r1.md @@ -0,0 +1,200 @@ +# SPEC-REVIEW-224-r1 + +- Issue: [#224](https://github.com/Matysh/houseplan-card/issues/224) — «Канонизировать координаты при записи конфига» +- Этап: spec (PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- Артефакт ТЗ: `docs/specs/224-config-coordinate-canonicalization.md` +- Ветка: `issue/224-config-coordinate-canonicalization`, SHA на момент ревью: `d5725c2fab2c93c007f9a81b7891fef3311ee1dc` +- Ревьюер: Claude, роль «ревьюер ТЗ» (не автор) + +## Скоуп ревью + +Первый заход — разбор полный, дельты предыдущего раунда нет. Диапазон: +тело issue #224 и все три комментария владельца (аналитика → ТЗ), файл +`docs/specs/224-config-coordinate-canonicalization.md` целиком, сверка с +фактическим кодом (`custom_components/houseplan/validation.py`, +`src/houseplan-card.ts`, `src/align-grid.ts`, `scripts/mutation-gate.mjs`), +с `docs/SCOPE.md`, `docs/CANVAS.md`, `docs/CONFIG-COMPATIBILITY.md`, +`docs/TESTING.md`. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #224 и все комментарии (аналитика, ТЗ, ссылка на файл). +3. Прочитан ТЗ-файл целиком на ветке задачи (`git show + origin/issue/224-config-coordinate-canonicalization:docs/specs/224-...md`). +4. Факты ТЗ сверены с реальным кодом, а не приняты на слово: + - `_COORD`/`_GEOM`/`POS_SCHEMA`/`LAYOUT_SCHEMA`/`CONFIG_SCHEMA` — + `custom_components/houseplan/validation.py:522-1130` — подтверждают + заявление §3 о том, что схема лишь проверяет диапазон/конечность и + возвращает исходный `float`. + - `validation.py:593` (`_dedupe_open_spans`, `round(a[0], 6)`) — + подтверждает ссылку issue на прецедент округления в схеме. + - `PLAN_SCALE_MIN/MAX`, `plan_angle` (`±360`), `CANVAS_LIMIT = 5000.0` — + подтверждают допущение §6.1 о том, что `abs(v)*1e9` не подходит + к границе `Number.MAX_SAFE_INTEGER`. + - `MARKER_SCHEMA` (`validation.py:1025-1124`) — подтверждает, что у + маркера в конфиге есть только `angle`, а не `x/y`; `x/y` живёт в + отдельном layout-сторе — таблица allowlist §6.2 разводит их верно. + - `_writeConfig`/`_persistLayout` в `src/houseplan-card.ts` существуют + и соответствуют описанному месту вставки конвергенции (§8). + - `async_save_config_state`/`async_save_layout_state` — подтверждены в + `websocket_api.py`, `store.py`, `__init__.py` (общий storage barrier + из §7.2 адресует реальные точки записи, а не придуманные). + - `snapN()` в `src/align-grid.ts` — подтверждает границу не-скоупа + («это не snap к решётке», §5) как отдельный, не переиспользуемый + механизм. + - `docs/TESTING.md:2093` — строка «server Undo restores the original + noisy bits» существует буквально; ТЗ §9 корректно определяет её как + подлежащую замене формулировку, а не выдумывает несуществующий текст. + - `scripts/mutation-gate.mjs` существует и поддерживает добавление + новых id — мутационные пункты §13 реализуемы. +5. `git diff origin/dev...origin/issue/224-config-coordinate-canonicalization` + — только `docs/specs/224-....md` (360 строк) и одна строка реестра в + `docs/specs/README.md`. Продуктовый код не тронут — ожидаемо для стадии + spec review, нарушений класса файлов нет. + +Гейты кода на этапе ревью ТЗ не запускались — работа состоит из одного +документа, продуктового кода в ветке нет, весь код-гейт неприменим на этой +стадии процесса (PROCESS.md §2.4 предметом ревью называет ТЗ, а не гейты). + +## Проверка обязательных разделов (PROCESS.md §7.1) + +Все обязательные разделы присутствуют и в правильном порядке — сценарий и +«что человек увидит» идут первыми: + +| Требование §7.1 | Раздел ТЗ | +|---|---| +| Сценарий (персона/поверхность/момент) | §1 | +| Что человек увидит до/после | §2 | +| Проблема | §3 | +| Скоуп и не-скоуп | §4, §5 | +| Контракт поведения | §6, §7, §8, §9 | +| UX | явно закрыт констатацией «нового UI/уведомления/настройки нет» в §2 и §11 (нет визуальных/интерактивных изменений — раздела по существу не требуется) | +| Модель данных и миграция | §10 | +| i18n | §11 (явное «новых строк нет») | +| AC1…ACn с доказательством | §12 | +| План автотестов | §13 | +| Риски | §14 | +| Откат | §14 (последний абзац) | +| Release-артефакты | §15 | + +Блок принятых технических предположений (§7.1, «размытое место... явным +блоком») присутствует — §16, 5 пунктов, каждый явно помечен «принято +предположительно, поменять свободно». + +## Проверка каждого AC на однозначность и способ доказательства + +AC1–AC13 (§12) — каждый сформулирован как проверяемое утверждение с +конкретным способом доказательства (fixture, unit, mutation id), не общими +словами вида «работает корректно». Ключевые: + +- AC1 (побитовое совпадение Python/TS) — доказывается общей fixture, + тест умеет упасть при расхождении округления/tie-правил. +- AC2/AC11 — allowlist и negative-contract разведены на два разных теста + (позитивный + мутант `quantization-hits-allowlist`), что не даёт + «зелёному тесту без падения» проскочить. +- AC4 — no-op-контракт явно завязан на CAS-проверку **до** сравнения + payload (§7.3 п.5), что не даёт no-op стать обходом optimistic locking — + формулировка исключает неоднозначность «а что если revision совпадает + случайно». +- AC7/AC9 — привязаны к реальному регрессу (#218) и к реальному мутанту + (`import-path-bypasses-schema`), не абстрактны. +- AC10 — граница «диагональные объекты не прилипают к сетке» имеет числовой + порог (максимальный сдвиг из §6.1), проверка не оставлена на усмотрение. + +Ни один AC не описывает недоказуемое субъективное состояние («выглядит +нормально», «работает быстро» без числа). + +## Проверка «не выдана ли догадка за решение» + +Утверждения о поведении, которые в ТЗ выглядят как факт, сверены с кодом +(см. «Как проверялось», п.4) — расхождений не найдено. Технические решения, +не наблюдаемые пользователем (точность 9 знаков, единая формула round-half- +away-from-zero, деление allowlist на «геометрию» и «presentation/calibration», +обязательность no-op, отказ от фоновой миграции) явно вынесены в §16 как +предположения, а не встроены в текст как свершившийся факт — граница +между «решено» и «предположено» в тексте видна. + +Продуктовых вопросов владельцу в ТЗ нет — и это корректно: единственная +персона (admin, единственный, кто пишет геометрию), единственный сценарий, +пользовательский результат явно нулевой («внешний вид и точность размещения +не меняются», §2). Открытых продуктовых развилок (какая персона важнее, +что считать приемлемой деградацией) в задаче нет — тип задачи это позволяет: +чисто внутренний storage-инвариант без нового UI. + +## Соответствие docs/SCOPE.md + +Задача закрывает J6 («Keep the plan true as the home evolves») — план не +должен статистически ломаться после обычных правок. Не пересекает out-of- +scope списки SCOPE.md. Явно не трогает vacuum calibration (SCOPE.md не +упоминает калибровку впрямую, но задача сама явно выносит её в negative +contract — корректная осторожность). + +## Согласованность с #218 и #223 + +- С #218: регрессия используется как эталонный AC (AC9), причём именно на + проблемном наборе координат из issue. +- С #223: ТЗ явно называет изменение формулировки Undo-контракта + («restores the original noisy bits» → «restores the original geometry in + canonical representation») и относит правку `docs/TESTING.md` в + release-артефакты (§15) — значит, задача не оставляет устаревшее + утверждение висеть в чужом документе, а берёт его обновление в свой DoD. + Это корректно по §12 запрещённого списка PROCESS.md («Оставили в тексте + ревью» не считается закрытием — здесь другой случай: правка чужого + упоминания через собственный release-артефакт, а не оставление находки + без действия). + +## Находки + +Не найдено. High — 0, Medium — 0, Low — 0. + +Рассмотренные, но не подтвердившиеся сомнения (для прозрачности разбора): + +- *Не размечены все 4 mutation id к конкретным AC-строкам §12 (только 2 из + 4 — `import-path-bypasses-schema` и `quantization-hits-allowlist` — + явно упомянуты в тексте AC).* Не является дефектом: `schema-quantization- + removed` и `frontend-writes-raw-coords` — мутанты уровня «сломать всю + канонизацию целиком», их естественно ловит позитивная проверка AC2/AC6 + вместе с обычным test suite; §13 явно называет добавление всех четырёх + мутаций отдельным пунктом плана реализации. Не блокирует ТЗ — это + плотность документа, не пробел в доказательстве. +- *Числовой предел безопасности (`abs(v)*factor < MAX_SAFE_INTEGER`) + сформулирован как факт, а не измерен на всех полях allowlist.* Проверено + выборочно по действующим `vol.Range` для `plan_scale`, `plan_angle`, + `_COORD`/`_GEOM` (±5000) — во всех случаях margin на порядки больше + необходимого. Не найдено поле allowlist, способное нарушить это + утверждение. + +## Что проверено и корректно + +- Полнота обязательных разделов §7.1. +- Однозначность и доказуемость каждого AC. +- Фактическая точность утверждений ТЗ о текущем коде (схемы, storage + barrier, frontend write path, snap-механизм, мутационный гейт). +- Явное разведение presentation/calibration и geometry в allowlist — + сверено с реальными схемами (`MARKER_SCHEMA`, `vacuum.calibration`, + `plan_aspect`/`view_box`). +- Отсутствие незаявленных догадок — все непродуктовые решения вынесены в + явный блок §16. +- Отсутствие изменений продуктового кода в ветке на этой стадии (класс A + не тронут — только класс C, `docs/specs/**`). +- Согласованность с #218 (регрессия) и #223 (обновление устаревшей + формулировки Undo в общем документе). + +## Чего не проверял + +- Реализацию — её ещё нет, это стадия spec review. +- Работоспособность будущей общей fixture Python/TypeScript — она ещё не + написана; корректность будет предметом код-ревью. +- Полный список числовых полей allowlist на предмет опечаток/пропусков + сверх выборочной проверки (markers, plan_*, walls, openings, decor, + room_drafts, partitions, wall_columns, open_spans) — сверены ключевые + представители каждой категории, не каждое поле построчно по схеме. +- Golden/smoke/performance — неприменимо на этой стадии, кода нет. + +## Вердикт + +Зелёный. ТЗ полное, однозначное, каждый AC проверяем и привязан к способу +доказательства, факты о коде подтверждены чтением исходников, догадки не +выданы за решения. Задача уходит в «Готово к разработке».