From ddc141b1befcb64992508e51b89f3ed8d67cfa4c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 22 Aug 2026 13:07:27 +0000 Subject: [PATCH] docs: review document for #199 Issue: #199 User-Visible: no --- docs/reviews/SPEC-REVIEW-199-r1.md | 210 +++++++++++++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-199-r1.md diff --git a/docs/reviews/SPEC-REVIEW-199-r1.md b/docs/reviews/SPEC-REVIEW-199-r1.md new file mode 100644 index 00000000..d4ffd110 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-199-r1.md @@ -0,0 +1,210 @@ +# SPEC-REVIEW-199-r1 — Optimize geometry preflight + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/199 +- **ТЗ:** `docs/specs/199-optimize-geometry-preflight.md`, коммит `0ac63b6` + на ветке `issue/199-optimize-geometry-preflight` + (родитель `c397b78` → `4dbdb44` → `dev@0d91c1e` на момент ревью). +- **Трек:** обычный (issue не помечен `small`), файл ТЗ обязателен и создан. +- **Заход:** r1 (первый цикл этого этапа для #199). +- **Вердикт:** см. итог ниже. + +## Скоуп ревью + +Разобрано ТЗ целиком: сценарий, продуктовые решения владельца (Q1–Q3 в +комментариях issue), контракт preflight (§7), UX-контракт (§8), backend/ +atomicity (§9), производительность (§10), AC1–AC14 (§11), план тестов (§12), +риски (§13), touch (§14), release-артефакты и rollback (§15), блок принятых +технических предположений (§16). + +Ревью строго по PROCESS.md §2.4: ищу, где ТЗ невыполнимо или непроверяемо, а +не соглашаюсь с автором. Устных пояснений автора не было — только issue, +комментарии и файл ТЗ. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком. +2. Прочитано тело issue #199 и все 6 комментариев (аналитика, продуктовые + вопросы Q1–Q3, решения владельца, хендофф автора ТЗ). +3. Прочитан файл ТЗ целиком (419 строк). +4. Каждая техническая ссылка ТЗ на существующий код проверена по `origin/dev` + (не по слухам автора): + - все перечисленные символы (`_openAlignDialog`, `optimizePlans`, + `wallBodiesGeometry`, `floorFootprintGeometry`, `_runAlignToGrid`, + `_renderAlignDialog`, `contentFingerprint`, `resolveOpenCuts`, + `wallIntervals`, `partitionOpeningHasCompositeRoomWall`, + `physicalBodyParts`, `spaceModels`, `NORM_W`, `GRID_STEP_N`, + `GRID_PITCH`, `gs.align_none`, `gs.align_done`) найдены `git grep` в + заявленных файлах; + - прочитан код `_openAlignDialog` / `_runAlignToGrid` / `_renderAlignDialog` + (`src/houseplan-card.ts:14903-15985`) — подтверждает описанный в ТЗ + текущий контракт диалога (`changed:false` → `gs.align_none` без Apply, + `changed:true` → отчёт + Apply); + - прочитаны оба реальных вызова `wallBodiesGeometry()` в продуктовом коде: + iso-путь (`_isoSource`, строка ~5206) **бросает** `Error` при `null` и + наличии стен/тел — это ровно тот "structural failure", который ТЗ + требует ловить; light-barrier-путь (~15652) деградирует мягко + (fallback на сырой контур). Плюс `_wallUnionGeometry` + (~11259) — путь, которым реально рисуется масонри в 2D-виде: при + `null`/пустом union он тоже молча откатывается на `paperRoomShapes`, + то есть именно так пользователь видел "исчезнувшую кладку" в #197; + - прочитан `wallBodiesUnionPath()` и `floorFootprintGeometry()` в + `src/wall-thickness.ts` — подтверждает различение ТЗ §7.3 между `null` + (structural failure) и «успешный пустой результат» (`united && !d` → + не ошибка, просто нет масонри); + - подтверждён large-house fixture (`demo/fixtures/large-house.mjs`): + `FLOOR_COUNT=3`, `ROOMS_PER_FLOOR=20` → 60 комнат, `OPENING_COUNT=100`, + `PARTITION_COUNT=60`, `COLUMN_COUNT=40` — числа в §10 ТЗ совпадают + буквально; + - проверены мутационные ID в `scripts/mutation-gate.mjs` — формат + (`kebab-case`, уже есть прецеденты вроде `union-failure-kills-space`, + `union-failure-silent`) совпадает с предложенными в ТЗ ID. +5. Прочитан `docs/CANVAS.md` §9.5 («Оптимизировать планы») — контракт диалога, + `optimizePlans`, one-deep snapshot, «отчёт — верхняя граница» совпадают с + тем, как ТЗ их описывает. +6. Прочитан `docs/TOUCH-SUPPORT.md` — safety floor («no data corruption or + silent loss of saved plan data») дословно то основание, на которое ссылается + ТЗ §1/§14. +7. Прочитан `docs/USER-GUIDE.ru.md` (разделы про Optimize/толщину стен) — + используемая в ТЗ терминология («Общие настройки → Оптимизировать планы», + «предпросмотр», «отмена») не изобретена, а взята оттуда. +8. Сверены два ближайших прецедента той же подсистемы для проверки + единообразия: `docs/specs/223-optimize-coordinate-canonicalization.md` и + реализация #224 (`git show 4a798e3 --stat`) — чтобы понять, что в этом + диалоге принято фиксировать в ТЗ дословно (RU+EN строки диалога) и что + действительно исторически остаётся вне DoD (английский `USER-GUIDE.md` + в #223/#224 не трогался — значит расхождение с ним не считается дефектом + и в ТЗ #199 тоже). +9. Проверен формат записи `docs/specs/README.md` (двухколоночная таблица + issue↔ТЗ, без колонки «Статус ТЗ») — соответствует актуальному состоянию + PROCESS.md §7.3 п.1. + +Гейты кода в этом раунде не запускались — на этапе ревью ТЗ гейты (typecheck/ +test/build) неприменимы, продукт не менялся. + +## Находки + +### Medium (в скоупе — чинится в этом же ТЗ) + +**M1. Английский текст сообщения об отказе и подсказки не зафиксирован +дословно — только «эквивалент», что делает AC6 непроверяемым для EN-локали.** + +- Файл: `docs/specs/199-optimize-geometry-preflight.md`, §8.1. +- Текущий текст: + ``` + - RU: «Не удалось безопасно проверить геометрию следующих пространств: + {spaces}{more}.» + - RU hint: «Планы не изменены. Обновите House Plan и повторите. Если ошибка + останется, приложите экспорт пространства к отчёту об ошибке.» + - EN: equivalent plain-language text without implementation terms. + ``` + Для двух главных предложений диалога EN-строка не приведена вообще — только + директива «эквивалентный текст». При этом в том же §8.1 для меньших + подстрок (fallback-имя `Space N`, суффикс `and N more`) EN дан дословно — + несогласованность внутри одного раздела. +- Почему это находка, а не техническая деталь: это пользовательский текст + нового диалогового состояния, то есть ровно то, что PROCESS.md §7.1 требует + фиксировать в ТЗ, а не отдавать на изобретение при реализации («Владельцу + задаются только продуктовые вопросы» не означает, что английскую копию + придумывает реализатор без утверждённого текста — второй язык тоже + наблюдаем пользователем). +- Прецедент того же диалога: `docs/specs/223-optimize-coordinate-canonicalization.md` + §7 добавляет новую строку в тот же Align-диалог и даёт пару целиком: + ``` + - RU: «обновлено пространств: {c}; устранён шум координат: {p}»; + - EN: «spaces updated: {c}; noisy coordinate values removed: {p}». + ``` + #199 — прямое продолжение той же серии задач (#197/#198/#223/#224/#229) над + тем же диалогом; стандарт "RU+EN дословно" в ней уже установлен и не должен + тихо ослабляться для более заметного по важности сообщения (блокирующий + отказ, а не информационная строка). +- Как воспроизвести проверку: открыть §8.1 ТЗ, попытаться написать unit-тест + на AC6 («i18n/UI unit») для английской локали — не из чего писать + assertion на конкретную строку, только на «что-то не техническое». +- Фикс: дописать в §8.1 дословный EN-эквивалент RU-сообщения и RU-hint (по + образцу уже данных в том же разделе пар "Space N"/"and N more"). +- Серьёзность: **Medium**, в скоупе задачи. High-находок нет, поэтому цикл + жёлтый, правка — в этом же ТЗ, без нового issue (#202). + +### Low (снимается без действия, для протокола) + +- Хендофф-комментарий автора ТЗ в issue содержит артефакт шаблона: «Коммит: + $sha.» — переменная не подставлена. Это дефект комментария, не ТЗ; на + содержание, полноту и проверяемость документа `docs/specs/199-*.md` не + влияет. Не блокирует, отмечаю для протокола; ревьюер не правит issue-комментарии. + +## Что проверено и корректно + +- **Продуктовая рамка (§1–§2).** Персона (домашний администратор), поверхность + («Общие настройки → Оптимизировать планы») и момент названы; предложение + «до/после» дано без терминов реализации. Явно назван J6 из SCOPE.md и + safety floor TOUCH-SUPPORT.md — оба подтверждены чтением исходников этих + документов, ссылка не декоративна. +- **Проблема подтверждена по коду, а не заявлена.** Пять шагов §3 (preview → + candidate → Apply → backend без polyclip-проверки) построчно совпадают с + реальным `_openAlignDialog`/`_runAlignToGrid` в `src/houseplan-card.ts`; в + частности, реальный fallback `_wallUnionGeometry` → `paperRoomShapes` при + `null`/пустом union — это ровно механизм "молча исчезнувшая кладка" из #197, + на который ссылается ТЗ. +- **Различение `null` (structural failure) и «успешный пустой результат» + (§7.3)** — не абстракция, а точное описание существующего поведения + `wallBodiesUnionPath()`/`floorFootprintGeometry()`: первое — жёсткая + ошибка (iso-путь её бросает исключением), второе — легитимный «пустая + комната без кладки» результат, который renderer рисует как обычные комнаты. + AC2/AC3 формулируют именно эту границу, и она проверяема. +- **Продуктовые решения владельца (Q1–Q3) перенесены в контракт без + искажения**: whole-plan block при любом failure (§7.1, AC4), блокировка + уже деградированного пространства без исключения «не стало хуже» (§4 п.2, + AC5), сообщение с именами пространств без технических деталей и без кнопки + Apply (§8.1, AC6) — всё совпадает с текстом решений владельца в issue. +- **Non-scope (§6)** явно исключает ремонт найденной геометрии, частичный + Apply, смену geometry engine, миграцию схемы, изменение времени жизни Undo — + снимает риск расползания скоупа, который прямо запрещён PROCESS.md §12. +- **Performance-бюджет (§10) — не оценка на глаз.** Числа fixture (3 + пространства/60 комнат/100 проёмов/60 partitions/40 columns) и базовое + измерение (median 155.9 ms / p95 162.56 ms / max 163.67 ms) сверены с + `demo/fixtures/large-house.mjs` и являются реальным замером автора, а не + придуманным ориентиром; относительный бюджет (20%+15ms поверх прямого + builder) корректно отделяет overhead preflight-обвязки от уже дорогого + polyclip-прохода. +- **Backend/atomicity (§9)** не добавляет новый параметр endpoint и не требует + второй реализации polyclip на Python — согласуется с Non-scope и с описанным + в CANVAS.md §9.5 контрактом одной атомарной транзакции. +- **AC1–AC14** — каждый пронумерован, однозначен и указывает способ + доказательства (unit/smoke/golden/mutation/benchmark/code review), что + требуется §2.5 DoR. +- **Блок §16 «Принятые технические предположения»** корректно отделяет + ненаблюдаемые пользователем решения (имена модулей, fingerprint-механизм, + отсутствие `preflight_passed` в backend) от продуктового контракта и явно + помечен как «ревьюер может оспорить» — соответствует PROCESS.md §7.1. +- **Формальная структура ТЗ** покрывает все обязательные разделы §7.1 + (сценарий · что видит человек · проблема · скоуп/не-скоуп · контракт · + UX · данные/миграция · i18n · AC с доказательством · план тестов · риски · + откат · release-артефакты); запись `docs/specs/README.md` обновлена в + формате, актуальном после чистки колонки «Статус ТЗ». +- **Rollback (§15)** — простой revert реализационного коммита, персистентные + данные не меняются новым preflight, что верно логически (preflight ничего + не пишет и не канонизирует сверх уже полученного Optimize-кандидата, §7.3). + +## Чего не проверял + +- Не проверялся сам будущий код реализации — его ещё нет, ревью только ТЗ. +- Не запускались `typecheck`/`test`/`build` — на этапе ревью ТЗ продукт не + меняется, гейты неприменимы. +- Не оценивалась реальная производительность на CI-машине (замер в ТЗ — + замер автора на его Windows-чекауте); согласно ТЗ §10 это будущая забота + code-review при нестабильности абсолютного бюджета в CI. +- Не проверялись мутационные тесты `scripts/mutation-gate.mjs` предметно + (их ещё нет для #199) — сверен только формат ID с существующими записями. +- Не исследовался `docs/CONFIG-COMPATIBILITY.md` построчно — ТЗ прямо + заявляет «без изменений и миграции», и этот пункт не оспаривается: он + логически следует из того, что preflight — чистая read-only проверка + существующего candidate, ничего не пишущая в schema. + +## Итог + +Единственная находка — Medium, в скоупе задачи, чинится правкой текста ТЗ +(добавить дословный EN-текст в §8.1), без нового issue (#202). High-находок +нет. Технические предположения корректны и проверены по коду `origin/dev` +предметно, а не приняты на слово автора. + +**Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 1 → в задаче**