docs: review document for #291

Issue: #291
User-Visible: no

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

@@ -0,0 +1,262 @@
+# SPEC-REVIEW-291-r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/291
+- **Этап:** ТЗ на ревью (PROCESS.md §2.4)
+- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4
+- **Артефакт ТЗ:** `docs/specs/291-lattice-coordinate-write-barrier.md`
+- **Ветка:** `issue/291-lattice-coordinate-barrier`
+- **SHA на момент ревью:** `8b99df6e` (docs-only коммит, единственный впереди `origin/dev`)
+- **Трек:** обычный (не `small`/`trivial`) — верно: миграция-подобная операция,
+  frontend+backend boundary, сложность/риск 10/10 по собственной оценке автора
+
+## Скоуп ревью
+
+Это первая редакция ТЗ. Разбор полный: раздела «Унаследовано из r0» не
+существует, дельта-обзор (§2.10 PROCESS.md) относится только к r2+.
+
+Прочитано перед оценкой: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (полностью,
+включая §2.4, §2.5, §2.9, §2.10, §7.1, §7.2), тело issue #291 и оба комментария,
+`docs/adr/282-wall-geometry-representation.md`, `docs/CONFIG-COMPATIBILITY.md`
+(§«Canonical geometry on write #224»), `docs/WALL-THICKNESS.md` (модель `wallKey`),
+`docs/USER-GUIDE.ru.md` §19 «Обслуживание планов», действующий код
+`scripts/model-invariants.mjs` (`latticeProfile`, `latticeReport`, флаг
+`--lattice`), `test/model-invariants.test.mjs` (существующие пины шума на
+`real-plan-*.json`), i18n-ключи `gs.optimize_*` в `src/i18n/ru.json`, и структура
+десяти сопоставимых ТЗ в `docs/specs/` (090, 094, 150, 178, 179, 210, 211, 226,
+252, 279) для проверки конвенции разделов «Риски»/«Откат».
+
+## Как проверялось
+
+Ревью документное (этап spec, кода еще нет — единственный коммит в ветке
+касается только `docs/specs/291-*.md`). Гейты кода (`typecheck`/`test`/`build`)
+на этом этапе неприменимы: класса A/B изменений в диффе нет.
+
+Проверено по существу, а не на слух:
+
+1. Численный контракт (§3.1–3.2 ТЗ) — свёрен с реализованным
+   `scripts/model-invariants.mjs`: `GRID_N = 240`, `NOISE_STEPS = 1e-4`,
+   формула `latticeDeviation` совпадают буква в букву с §3.1 ТЗ. Значит цифры в
+   ТЗ не выдуманы, а взяты из уже принятого измерительного кода Stage 0 (#283).
+2. Существующая девяти-decimal граница #224 — прочитана в
+   `docs/CONFIG-COMPATIBILITY.md`, сверена с §3.2 ТЗ («canonicalizeScalar
+   сохраняет nine-decimal contract»): согласуется, ТЗ не переизобретает эту
+   часть.
+3. Модель `wallKey` и её текущая частичная защита от #258
+   (`max(pitch·10⁻⁶, 10⁻⁹)` при вычислении key) — прочитана в
+   `docs/WALL-THICKNESS.md`. ТЗ говорит о #258 в прошедшем времени
+   («перебрасывали») — не противоречит: локальный фикс в `wallKey` не убирает
+   источник шума как класс, ТЗ формулирует именно устранение класса.
+4. Существующие тесты `test/model-invariants.test.mjs:349-394` уже пинят
+   `noise >= 100` на обеих `real-plan-*.json` и `checkWallKeys(...).length === 0`
+   на них же — совпадает с AC3/AC6 ТЗ буквально, эти AC не гадание, а описание
+   уже наблюдаемого свойства существующих фикстур.
+5. `--lattice` флаг и JSON-вывод (`profile.noise`, `profile.byKind`, …) в
+   `scripts/model-invariants.mjs` уже существуют — AC3 не требует придумывать
+   новый CLI с нуля, инструмент есть.
+6. i18n: сверено текущее содержимое `gs.optimize_changes` в `src/i18n/ru.json`
+   (строка 817) с требованием AC7 «per-space breakdown `{spaceId,
+   canonicalized, far}`» — текущий ключ — это один общий счётчик
+   («устранён шум координат: {p}»), не таблица по пространствам. Значит
+   AC7 действительно вводит новый видимый текст, а не просто меняет число за
+   существующей строкой.
+7. Конвенция остальных ТЗ: `grep` по `docs/specs/*.md` на «Риски»/«Откат»/
+   «Rollback» — во всех проверяемых десяти документах (090, 094, 150, 178, 179,
+   210, 211, 226, 252, 279) есть отдельный раздел. В `291-*.md` — нет ни одного
+   упоминания risk-таблицы или отдельного «Откат».
+8. Термин «future layout owner» (§4, §10 ТЗ) — не изобретён на месте: тот же
+   термин («Future owner») использован как строка risk-таблицы в
+   `docs/specs/252-optimize-orphan-layout-report.md:327`. Признано корректным
+   переносом термина, не догадкой.
+
+Полные автотесты/смоки/perf-бенчмарки не прогонялись и не должны: на этапе
+spec-review нет кода для их прогона (диф — один файл `docs/specs/291-*.md`).
+
+## Находки
+
+### Medium (в скоупе задачи — чинится в текущем issue)
+
+**M1. Отсутствуют обе обязательные продуктовые секции §7.1, и новый видимый UI
+предъявлен как решённый факт, а не помечен предположением.**
+
+- **Файл:** `docs/specs/291-lattice-coordinate-write-barrier.md`, разделы 1 и 6
+  (и весь документ — ни разу не упомянуты персона, поверхность или момент
+  встречи).
+- **Что не так:** §7.1 PROCESS.md требует первыми двумя разделами: «какая
+  персона (`docs/SCOPE.md`), на какой поверхности, в какой момент это
+  встретит» и «что человек увидит, одной фразой, без терминов реализации», и
+  явно объясняет, почему это первые разделы: «ТЗ, которое не может ответить на
+  эти два вопроса, описывает работу, а не изменение продукта». Раздел 1 этого
+  ТЗ («Сценарий и измеренная причина») — целиком технический: процент шума,
+  формула, причина. Ни слова про Home admin, Plan editor или момент нажатия
+  «Оптимизировать планы».
+  Это не формальность: изменение реально видимо. `gs.optimize_changes`
+  (`src/i18n/ru.json:817`) сегодня — один общий счётчик «устранён шум
+  координат: {p}». AC7 требует per-space breakdown `{spaceId, canonicalized,
+  far}` и отдельную строку для layout без пространства — это новый текст в
+  диалоге Optimize, который увидит Home admin. Ни один owner-decision из #284
+  (процитированных в самом issue) не говорит о степени детализации отчёта —
+  только о том, что far-координаты не трогаются и перечисляются. Решение
+  «показать разбивку по пространствам, а не только total» — собственное
+  решение автора ТЗ, поданное как AC, а не как предположение, которое
+  ревьюер может оспорить (§7.1: «Догадка, записанная как факт, — худший вид
+  дефекта»).
+- **Сценарий, где это ломается:** реализация делает per-space таблицу; owner,
+  открыв бету, ожидал (по аналогии с текущим единственным counter'ом) простой
+  общий счётчик и считает новую таблицу лишней/шумной UI-детализацией admin-only
+  диалога — расхождение обнаруживается после код-ревью, а не на этапе ТЗ, где
+  чинить дешевле.
+- **Что сделать:** добавить два продуктовых раздела по шаблону §7.1 (персона —
+  Home admin; поверхность — Plan editor → «Общие настройки → Оптимизировать
+  планы»; момент — открытие предпросмотра Optimize; одна фраза «что видит
+  до/после» — например: «раньше отчёт показывал один общий счётчик очищенного
+  шума, теперь — разбивку по пространствам»); одним предложением явно связать
+  степень детализации отчёта с §12 «принято предположительно, поменять
+  свободно» либо получить продуктовое подтверждение у владельца одним вопросом
+  с default-вариантом (§7.1 формат вопроса).
+
+**M2. Нет разделов «Риски» и «Откат», обязательных по DoR (§2.5) и присутствующих
+в каждом сопоставимом ТЗ проекта.**
+
+- **Файл:** `docs/specs/291-lattice-coordinate-write-barrier.md` — весь документ.
+- **Что не так:** DoR-чеклист §2.5 PROCESS.md требует перед переводом в
+  «Готово к разработке»: «влияние на производительность... названо»
+  (выполнено, §AC11) — но также «риски перечислены» и «откат: как выключить
+  или вернуть назад» отдельным пунктом. В этом ТЗ ни одного из двух нет как
+  структурированного раздела. Сравнение: `docs/specs/252-optimize-orphan-layout-report.md`
+  §12 содержит таблицу «Риск | Мера» и явную строку «Rollback — revert
+  implementation-коммита. Persisted format не меняется...»;
+  `docs/specs/279-near-orthogonal-junction.md` §5 — «Главный риск... его
+  закрывают...»; `docs/specs/226-entity-parent-dedup.md` §18 — «Откат и риски»
+  как отдельный заголовок. Во всех 10 проверенных ТЗ раздел есть; в 291-м —
+  нет.
+- **Почему это не формальность именно здесь:** AC5 требует, чтобы barrier был
+  **непроходим** — ни один frontend outbound writer и ни один backend
+  `Store.async_save` config/layout путь не может обойти его. Это означает, что
+  ошибка в самой boundary-функции (например, из-за неверной granularity округления
+  или ложного срабатывания source/AST guard'а на легитимном новом writer'е)
+  блокирует **все** сохранения плана сразу на обоих стеках. Собственная оценка
+  риска в ТЗ и issue — 10/10. Для изменения такого класса не сказано ни
+  слова о том, как отличить misfire барьера от обычного дефекта в проде, и как
+  откатиться (просто ревертнуть коммит? отключить Labs-флагом? — не решено ни
+  так, ни так).
+- **Что сделать:** добавить раздел «Риски и откат»: минимум — таблица
+  «риск | мера» (ложный noise-match у легитимной near-node authored geometry;
+  guard блокирует легитимный новый writer; boundary даёт разные canonical
+  bits на fe/be при расхождении версий) и явное «откат — revert
+  implementation-коммита, `PLAN_MODEL_VERSION` не повышается, обратная
+  миграция не нужна» (или иной вариант, если авторское решение отличается).
+
+### Low (замечание, не блокирует; фиксируется или снимается с записью)
+
+**L1. AC3 иллюстрирует вывод CLI текстом, который не совпадает с реальным
+форматом инструмента.**
+
+- **Файл:** `docs/specs/291-lattice-coordinate-write-barrier.md`, AC3 (раздел 8).
+- ТЗ приводит:
+  ```
+  npm run invariants -- --config <candidate> --lattice
+  noise: 0 (0.00%)
+  ```
+  Фактический вывод `latticeReport()` (`scripts/model-invariants.mjs:520-541`,
+  уже реализован) — русский текст `"  шум у узла 0 (0.00%)"`, а не `noise: 0
+  (0.00%)`. С флагом `--json` было бы `profile.noise === 0`. Разночтение
+  косметическое и не меняет проверяемость критерия (число ноль — то же самое),
+  но при буквальном сравнении вывода на код-ревью может создать ложное
+  сомнение «инструмент не совпадает с ТЗ».
+- **Решение ревьюера:** не блокирует; правится либо заменой примера на
+  `profile.noise === 0` (JSON-режим), либо точной русской строкой, при
+  следующей правке документа по другому замечанию. Отдельного возврата ради
+  этого не требуется.
+
+**L2. Issue #291 называет себя «Стадия 1 из ADR #282», хотя реальная Stage 1
+ADR #282 — это «stored identity» (stable wall ids), которую сам этот же ТЗ
+явно выносит в «Не входит».**
+
+- Это находка по телу issue, не по файлу ТЗ (сам файл `docs/specs/291-*.md`
+  термин «Stage 1» не использует и корректно перечисляет `integer storage
+  schema, stable wall ids` в §7 «Не входит»). Расхождение — только в первой
+  фразе issue-body: «Выделено из исследования #284 ... Стадия 1 из ADR #282,
+  без смены схемы хранения». По ADR (`docs/adr/282-wall-geometry-representation.md`,
+  раздел «Stage 1 — stored identity»), Stage 1 — это именно stable id для
+  стен, а не lattice-canonicalization; последнее ближе к развитию Stage 0
+  (измерение → устранение измеренного явления), но не совпадает ни с одной
+  явно пронумерованной стадией ADR.
+- **Почему стоит внимания:** будущий читатель, ищущий прогресс по ADR #282 в
+  issue-трекере, увидит «Stage 1 сделан» по номеру #291 и решит, что stable
+  wall ids уже есть — хотя это не так и явно не входит в скоуп.
+- **Решение ревьюера:** не блокирует ТЗ (файл спеки не содержит ошибки), но
+  стоит поправить формулировку в issue (комментарием или правкой шапки) —
+  например, «расширение Stage 0 ADR #282» вместо «Стадия 1». Оставляю как
+  Low-замечание автору issue, не как находку к самому документу ТЗ.
+
+## Что проверено и признано корректным
+
+- Численные константы и формулы (`GRID_N=240`, `1e-4` порог, `Math.round`)
+  совпадают с уже реализованным и слитым кодом Stage 0 — не выдумка.
+- Разграничение `canonicalizeLatticeCoordinate` / `canonicalizeScalar` (§3.2)
+  корректно не пересекается с существующим #224 nine-decimal contract и не
+  вводит рекурсивную канонизацию произвольных чисел — прямое соответствие
+  требованию `docs/CONFIG-COMPATIBILITY.md` «No recursive "round every
+  number" migration is allowed».
+- Allow-list полей (§4) — по каждому пункту сверено с реально существующими
+  полями модели (`room.poly`, `walls[].a/b`, `partitions[].a/b`,
+  `wall_columns[].center`, `open_spans[].a/b`, layout `x/y`) — совпадает с
+  полями, которые уже обходит `modelCoordinates()` в
+  `scripts/model-invariants.mjs:316-358`. Все объекты allow-list реальны, не
+  придуманы.
+- AC1/AC2/AC10 (идемпотентность, shared fixture parity, мутанты) —
+  проверяемые, с точным диапазоном (4801 узлов, k=-2400..2400) и конкретным
+  списком обязательных мутантов; никаких «проверим на глаз».
+- AC5 (source/AST guard, непроходимый barrier) — не спекуляция: в репозитории
+  уже есть прецедент такого класса гейта (`scripts/backend-test-guard.mjs`),
+  подход реализуем.
+- AC6 — сформулирован как уже наблюдаемое свойство существующих фикстур
+  (`test/model-invariants.test.mjs:349-394` уже пинит `noise >= 100` и
+  `checkWallKeys === 0` на raw real-plan файлах) — ТЗ не требует нового
+  инварианта, а фиксирует то, что уже проверяется, плюс явное «raw
+  byte/hash-equivalent после теста».
+- §12 «Принятые технические предположения» — правильно оформлен как явный
+  блок допущений (место, где агенты решают несмотрящиеся пользователю детали
+  сами), и по содержанию все 5 пунктов действительно технические
+  (Store-механика, версия схемы, диагональные координаты, touch-контракт), а
+  не подмена продуктового решения.
+- Скоуп/не-скоуп (§7) корректно исключает дорогую часть ADR (integer schema,
+  stable ids, planar graph) и визуально несвязанные P1-баги (#288–290, #278
+  union algorithm) — совпадает с собственным анализом owner в issue-теле
+  («барьер не лечит то, что болит сейчас»).
+- Trailers/track: `tech-debt`/`P1`, обычный трек — корректно, задача не
+  подходит под `small` (миграция-подобная операция, две поверхности —
+  frontend+backend, широкая точка записи).
+
+## Чего не проверял
+
+- Собственно код: на этом этапе (S4-spec-review) продуктового кода в ветке
+  нет — единственный коммит `8b99df6e` касается только
+  `docs/specs/291-lattice-coordinate-write-barrier.md`. Гейты `typecheck`,
+  `npm test`, `npm run build`, `check-docs.mjs`, `invariants`, смоки,
+  perf-бенчмарк — неприменимы к этому диффу и не прогонялись; они станут
+  предметом код-ревью после реализации.
+  - Причина, а не пропуск: диапазон `git diff origin/dev...HEAD --stat` —
+    ровно один файл документации, класс C, без class A/B изменений.
+- Не проверял, действительно ли перечень «Ожидаемые файлы» (§10 ТЗ) полон —
+  список файлов на этапе спеки индикативен, а не контракт; это будет видно на
+  код-ревью по фактическому диффу.
+- Не связывался с владельцем и не задавал продуктовых вопросов напрямую:
+  вопрос о степени детализации Optimize-отчёта (M1) сформулирован как
+  находка с предложенным решением по умолчанию (следовать текущей
+  однострочной конвенции `gs.optimize_changes`, если автор явно не считает
+  таблицу по пространствам необходимой) — снимается автором в следующей
+  редакции ТЗ, а не эскалацией к владельцу, если умещается в пятиминутное
+  предположение с default-вариантом; при разногласии автор/ревьюер решают
+  вопрос вердиктом следующего цикла (§7.1), не владельцем.
+
+## Итог
+
+0 High, 2 Medium (в скоупе — обе чинятся в этом же issue текстовыми правками
+ТЗ, не переработкой технического контракта), 1 Low (не блокирует, правится
+заодно), плюс отдельное Low-замечание к формулировке issue-body (не к файлу
+ТЗ). Технический контракт (AC1–AC12, численные константы, allow-list,
+барьер, мутанты) хорошо обоснован и явно проверен по существующему коду —
+без High. Без High это жёлтый вердикт: документ возвращается автору на правку
+двух отсутствующих разделов, повторный заход re1 расходует один цикл из
+лимита 4.