From 7fd9ca0f6a23d7159d794a17d9b9a0fa46b3d046 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:48:56 +0000 Subject: [PATCH] docs: review document for #290 Issue: #290 User-Visible: no --- docs/reviews/SPEC-REVIEW-290-r1.md | 265 +++++++++++++++++++++++++++++ 1 file changed, 265 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-290-r1.md diff --git a/docs/reviews/SPEC-REVIEW-290-r1.md b/docs/reviews/SPEC-REVIEW-290-r1.md new file mode 100644 index 00000000..40cfc9d5 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-290-r1.md @@ -0,0 +1,265 @@ +# SPEC-REVIEW-290-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/290 +- **Этап:** spec (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +- **Ревьюируемый артефакт:** `docs/specs/290-near-axis-authoring-and-repair.md`, + ветка `issue/290-near-axis-authoring-repair`, коммит `dcdb7565` +- **Вердикт:** жёлтый + +## Скоуп ревью + +Issue не помечен `small`/`trivial` → обычный трек, ТЗ обязано жить файлом в +`docs/specs/`. Диапазон изменений от `origin/dev` — ровно один файл +(`docs/specs/290-near-axis-authoring-and-repair.md`, класс C), что и требуется +на этапе `S4-spec-review`: продуктовый код не тронут. + +Задача — часть цепочки #284 (канонизация координат записи) → #279 (устойчивый +рендер почти-ортогонального T-стыка) → #290 (не создавать такую геометрию +молча и дать явный способ её исправить). Продуктовая рамка по `docs/SCOPE.md` +— J6 («Keep the plan true as the home evolves»): администратор должен видеть и +контролировать изменение геометрии, Optimize обязан явно сообщать о +lossy-исправлениях. Это совпадает с собственной оценкой аналитика в issue. + +## Как проверялось + +1. Прочитаны `docs/SCOPE.md`, `PROCESS.md` (жизненный цикл, §7.1, §8), + `AGENTS.md`. +2. Прочитано тело issue #290 целиком и все три комментария (аналитика, + продуктовые вопросы с defaults, ссылка на готовое ТЗ). +3. Прочитан весь текст `docs/specs/290-near-axis-authoring-and-repair.md` + (313 строк). +4. Сверено с каноническими документами задетых подсистем: `docs/CANVAS.md` + (снап/Shift/hover-resolver, барьер канонизации координат записи, «Оптимизировать + планы»), `docs/RESIZE.md` (safe-resize contract, eligibility, pure pipeline, + Undo), `docs/WALL-THICKNESS.md` (модель `walls[]`, `wallKey`, rekey после + Resize/Optimize), `docs/USER-GUIDE.ru.md` (текущий текст диалога + «Оптимизировать планы» и его таблица операций). +5. Прочитаны связанные issue #279 (полная переписка причины/фикстуры), + #277 (safe Resize), #248 (идемпотентность Optimize), #284 (канонизация + записи) — все указаны в ТЗ как связанные. +6. Сверены существующие тестовые фикстуры: `test/fixtures/279-near-orthogonal-junction.json`, + `test/fixtures/real-plan-second-floor.json`, `test/fixtures/coordinate-canonicalization.json` + — на предмет соответствия тому, что AC4 называет «minimized fixture из #284». +7. Прочитан существующий продуктовый код на предмет уже введённой константы + `MULTI_WALL_NEAR_ORTHOGONAL_MAX_DEGREES = 0.25` (`src/wall-thickness.ts:84`), + чтобы проверить, не конфликтует ли предлагаемый общий `NEAR_AXIS_MAX_DEGREES` + с уже существующим порогом того же значения. +8. Для сравнения формата прочитаны полностью аналогичные недавние ТЗ той же + геометрической линии — `docs/specs/277-safe-resize.md` и + `docs/specs/279-near-orthogonal-junction.md` — как ориентир на «Риски», + «Rollback» и формат таблицы AC/Доказательство, принятый в этом репозитории. + +Гейты (`typecheck`/`test`/`build`) не запускались: на этапе `spec` продуктовый +код не менялся, диапазон diff состоит из одного файла документации, гоны не +применимы к этому этапу. + +## Находки + +### Medium (в скоупе задачи — чинится в этом же ТЗ) + +**M1. Отсутствуют обязательные разделы §7.1 «риски» и «откат».** + +- **Файл:** `docs/specs/290-near-axis-authoring-and-repair.md` +- **PROCESS.md §7.1:** «Обязательные разделы ТЗ: сценарий · что человек увидит + до и после · проблема · скоуп и не-скоуп · контракт поведения · UX · модель + данных и миграция · i18n · критерии приёмки AC1…ACn с указанием + доказательства · план автотестов · риски · откат · release-артефакты.» +- **Что не так:** в документе нет ни одного раздела «Риски» и ни одного + раздела/абзаца «Откат/Rollback». Единственное упоминание слова «риск» — в + шапке, это число самооценки аналитика (9/10), не анализ. Раздел 11 + («Release») описывает только трейлеры коммита и требование Linux-артефакта + для golden — про то, как вернуть изменение назад (revert commit? нужен ли + Labs-флаг? что произойдёт со старыми планами, если поведение окажется + ошибочным и придётся откатывать) не сказано ни слова. +- **Почему это не мелочь:** это не формальность, а действующая практика в этой + же геометрической линии задач. Оба непосредственных предшественника — + `docs/specs/277-safe-resize.md` (§12.1 «Риски и меры», таблица из 10 строк, и + §17 «Release-артефакты и rollback» с явным «Rollback — revert implementation + commit... После rollback вернётся широкий нестабильный UI, поэтому release + rollback должен быть полным и сопровождаться предупреждением») и + `docs/specs/279-near-orthogonal-junction.md` (§5 «Совместимость, риски и + performance») — оба содержат этот анализ. У #290 риск выше, чем у #277: здесь + вводится **автоматическое изменение геометрии при рисовании без обходного + модификатора** (§5.1 ТЗ, п. «Специального modifier bypass нет») — то есть + редактор будет менять то, что нарисовал пользователь, без явного согласия в + моменте. Ровно такой случай (переопределение поведения без spec на риски) + — то, ради чего DoR требует «влияние… названо (или явно «нет»)» и «риски + перечислены» (PROCESS.md §2.5). +- **Воспроизведение:** `grep -niE "риск|откат|rollback"` по файлу ТЗ (кроме + строки самооценки в шапке и одного упоминания «downgrade не требует + migration» в §9) не находит содержательного раздела. +- **Требуется:** добавить раздел «Риски» (минимум: ложные срабатывания + auto-straightening на длинных почти-диагональных стенах около границы + 0.25°; двойной подсчёт shared-стены между Optimize-кандидатами; конфликт с + параллельно идущими #276/#278/#281, которые тоже трогают Optimize-геометрию, + как это явно оговорено в #277 §12.1) и раздел «Откат» (что происходит, если + после релиза найден дефект: revert коммита, нет миграции схемы, старые планы + с уступом остаются как есть до следующего Optimize). + +**M2. AC исходного issue про `npm run invariants` на реальных данных и +видимое снижение числа почти-ортогональных узлов не перенесён в ТЗ.** + +- **Файл:** `docs/specs/290-near-axis-authoring-and-repair.md`, раздел 8 + (AC1–AC10), в сравнении с AC6 тела issue #290. +- **Что не так:** issue формулирует шестой критерий приёмки явно: «`npm run + invariants` на обоих реальных планах — код 0; после выпрямления уступа число + почти-ортогональных узлов уменьшается, и это видно в отчёте Optimize». В ТЗ + этому соответствует только AC5, но AC5 требует нулевых нарушений + `checkWallKeys`/`checkMixedRoleRecords`/preflight на **минимизированной + фикстуре**, а не на «обоих реальных планах», и не требует показать + уменьшение числа почти-ортогональных узлов ни в выводе + `scripts/model-invariants.mjs`, ни в `OptimizeReport`. Раздел 10 + («Ожидаемые файлы» → Tests/evidence) тоже не называет + `node scripts/model-invariants.mjs --config <...>` явно, хотя PROCESS.md §8 + делает эту команду обязательной ровно для диффов, трогающих геометрию и + ссылки на неё (edges, `wall thickness records`, `open_spans`) — а это ядро + задачи #290. +- **Почему это не формальность:** минимизированная фикстура доказывает, что + механизм работает на сконструированном случае; она не доказывает, что на + реальном плане (с сотнями узлов, где уже случались #253/#258/#259) число + почти-ортогональных узлов действительно падает, а не создаётся новый класс + случаев в другом месте плана. Именно необходимость такой проверки на + реальных данных и породила #279 (реальный экспорт `44.json`) и практику + «второго реального плана» в текущей серии коммитов ветки + (`test: add a second real plan to the masonry gate`, + `test: one thickness record must not describe two wall roles`) — то есть + инфраструктура для анонимизированной проверки на реальных/близких к реальным + данным в этом репозитории уже есть и используется соседними задачами. +- **Требуется:** либо явно перенести требование issue в AC (анонимизированная + фикстура, произведённая из реального плана, а не полностью синтетическая + «minimized»; факт снижения near-orthogonal-count через + `scripts/model-invariants.mjs` до/после Optimize), либо явно и обоснованно + сузить критерий с указанием, почему полный реальный план как источник + проверки не нужен — так же, как #277 §16 прямо объясняет использование + анонимизированной регрессионной фикстуры вместо сырого экспорта. Молчаливое + сужение критерия приёмки, сформулированного в issue, без объяснения — + ровно тот случай, который ревью обязано ловить. + +### Low (снимается либо правится, с записью) + +**L1. AC4 неточно называет источник fixture.** + +- **Файл:** `docs/specs/290-near-axis-authoring-and-repair.md`, AC4: «Minimized + fixture из #284 содержит duplicated shared physical edge `316×1`». +- **Наблюдение:** узел, который АС4 описывает (`316×1`, координаты + `(-1.670833333, 2.95)`/`(-0.354166667, 2.954166667)`), — это **буквально** + `test/fixtures/279-near-orthogonal-junction.json` (сверено байт в байт: тот + же `node`, те же координаты стен). Issue #284 — про барьер канонизации + координат записи (`snap = v => Math.round(v*240)/240`), отдельная задача про + другой механизм; она не производила эту fixture. Вероятная причина — + смешение «issue #284 как общее исследование, откуда взят пример» и «issue + #279 как задача, где эта fixture уже закоммичена». Это не блокирует + реализацию (fixture физически существует и полностью подходит под описание + AC4), но при написании автотеста первое, что сделает исполнитель — будет + искать/создавать fixture «из #284», где её нет. +- **Решение ревьюера:** Low, не эскалируется. Достаточно поправить ссылку на + `test/fixtures/279-near-orthogonal-junction.json` (или явно сказать «тот же + случай, что и в #279») при следующей правке файла; отдельного цикла ради + этого не требуется. + +## Что проверено и признано корректным + +- **Обязательные продуктовые разделы §7.1** («сценарий», «что человек + увидит») присутствуют и содержательны (разделы 1 и 3); ТЗ отвечает на оба + вопроса без терминов реализации в пользовательской части. +- **Продуктовые вопросы владельцу закрыты.** Ровно три вопроса + (выравнивать/предупреждать, поведение Optimize, единый допуск) заданы одним + комментарием с defaults, каждый — что человек видит/делает, не техническое + решение, выданное за продуктовое. Раздел 2 ТЗ фиксирует все три ответа + дословно, раздел «Открытых продуктовых вопросов нет» подтверждён — при + чтении issue и ТЗ вместе противоречий не найдено. +- **Единый допуск и его источник (раздел 4)** сформулирован проверяемо: + включительная граница `0.25°`, `316×1` классифицируется, `316×2` — нет, + zero-length исключён, инвариантность к endpoint order/winding/scale/theme + названа явно. AC1 требует единый source threshold и source-guard против + отдельных литералов `0.25` — реализуемо и falsifiable (мутант «порог ниже + `0.181315°`» и «strict `<`» в AC9 корректно бьют по обеим сторонам границы). + То, что предлагается использовать *то же числовое значение*, что уже занято + `MULTI_WALL_NEAR_ORTHOGONAL_MAX_DEGREES` в `src/wall-thickness.ts:84` для + другого геометрического сравнения (угол между двумя лучами, а не отклонение + одного луча от глобальной оси) — техническое решение, а не путаница: раздел + 4 явно оговаривает, что renderer может продолжать использовать + «эквивалентный dot/sine form for orthogonal pairs», то есть общий источник — + это значение допуска, а не буквально один и тот же exported symbol. Это + подпадает под «принято предположительно, поменять свободно» и не требует + отдельного продуктового решения. +- **Негативный контракт (диагонали не трогаются)** описан симметрично + положительному на всех трёх путях (Walls, Resize, Optimize) и покрыт + отдельным AC6 плюс мутантами AC9 «считать room copies как две стены» / + «repair only one owner shared wall», что закрывает риск двойного счёта + расшаренной стены — именно то, из-за чего первоначальный AC issue выделял + этот пункт отдельно. +- **Совместимость с существующими контрактами подсистем.** Раздел 5.2 + корректно наследует существующий контракт `docs/RESIZE.md` («только + numerically horizontal/vertical edge eligible», epsilon поглощает только + storage noise) и не пытается расширить Resize eligibility на near-axis + случаи — вместо этого явно направляет их через Optimize (раздел 12, пункт + 2), что не противоречит ни `RESIZE.md`, ни `WALL-THICKNESS.md`. Раздел 6.1 + корректно ссылается на существующий канонический механизм ключей толщины + (`wallKey`, exact-span lookup) из `WALL-THICKNESS.md` вместо изобретения + нового формата хранения — толщина не становится отдельным «candidate», + а reprojection'ится из исправленной геометрии существующим pipeline, что + соответствует модели `WALL-THICKNESS.md` («key is computed from + lattice-stable endpoints»). +- **Touch/производительность (раздел 9)** соответствует терминологии + `docs/TOUCH-SUPPORT.md` дословно («View/kiosk rendering fully supported» ↔ + «Fully supported» в таблице `TOUCH-SUPPORT.md`), не расширяет editor parity + на touch сверх «best effort», не блокирует pinch/pan/pointercancel — + соответствует установленному продуктовому решению («editors are + desktop-first»). Заявление «Authoring classifier `O(1)`» и «Optimize pass + линейный по edges» не противоречит существующим performance-бюджетам + `RESIZE.md` и не заявляет числ, которые нечем будет доказать: сохранена + ссылка на «full performance gate обязателен». +- **Никаких догадок, выданных за факт, не найдено** среди технических + утверждений о **существующем** поведении: каждая проверенная ссылка на + текущий контракт (Shift 45°, safe Resize eligibility, `wallKey`, + барьер канонизации записи #284) точно соответствует канону в `CANVAS.md` / + `RESIZE.md` / `WALL-THICKNESS.md`. Пункт раздела 12 («Принятые технические + предположения») корректно ограничен вопросами, которые пользователь не + наблюдает (какой endpoint двигается при равной безопасности кандидатов, + что unsafe-repair не обязан чинить любой ценой) — то есть именно тем + классом решений, которые PROCESS.md §7.1 разрешает решать автору/ревьюеру + без владельца. +- **i18n, DoR-чеклист.** Раздел 10 называет оба файла (`en.json`/`ru.json`); + конкретные ключи на этапе ТЗ не требуются — сверено с #277/#279, которые на + этом же этапе тоже ограничиваются общей фразой «RU/EN i18n» без перечисления + ключей. +- **Файл ТЗ лежит в правильном месте и правильно назван** — + `docs/specs/290-near-axis-authoring-and-repair.md`, номер совпадает с + номером issue, что соответствует PROCESS.md §2.3. + +## Чего не проверял + +- **Не проверял, что перечисленные в ТЗ файлы (`src/resize.ts`, + `src/align-grid.ts`/`src/plan-optimizer.ts`, `src/houseplan-card.ts`) + действительно являются правильными точками внедрения** — на этапе ТЗ кода + ещё нет, а раздел 10 явно помечен как «Ожидаемые файлы», то есть прогноз, а + не обязательство; проверка предметна для код-ревью. +- **Не оценивал реальную достижимость перфоманс-бюджетов** («Authoring + classifier O(1)», сохранение p95 из `RESIZE.md`) — это будет проверяться + performance-гейтом перед бетой, не на этапе спецификации. +- **Не проверял мутационный registry** (AC9) на предмет технической + реализуемости каждого конкретного мутанта в текущей кодовой базе — на этом + этапе mutation ids не существуют, это работа код-ревью. +- **Не запускал никаких гейтов** (`typecheck`/`test`/`build`) — diff этого + раунда состоит из одного файла документации класса C, гейты к этапу spec не + относятся (см. «Как проверялось»). +- **Не проверял docs/CONFIG-COMPATIBILITY.md registry на конкретные записи** — + ТЗ утверждает отсутствие изменений схемы (раздел 9), новых persisted-полей + нет, поэтому реестр совместимости не должен получать новых записей; это + утверждение принято на веру как техническое (не продуктовое) без + дополнительной проверки скрипта `config-field-registry.mjs`, так как задача + явно заявляет «schema не меняется». + +## Вывод + +Продуктовая часть ТЗ (сценарий, видимый результат, продуктовые решения +владельца, критерии приёмки как таковые, негативный контракт для диагоналей) +выполнена тщательно и без домыслов, выданных за факт. Причина жёлтого вердикта +— два содержательных пробела в обязательных разделах и трассируемости AC: +отсутствие анализа рисков и плана отката (§7.1), и молчаливое сужение AC6 +issue (проверка на реальных данных, видимое снижение числа +почти-ортогональных узлов) без объяснения. Оба пункта — Medium в скоупе этой +же задачи и чинятся правкой того же файла, без нового issue и без обращения к +владельцу.