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