From 1fcbb0de6dbf7cf128767efdc08303e7caf7d25e Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:49:51 +0000 Subject: [PATCH] docs: review document for #306 Issue: #306 User-Visible: no --- docs/reviews/SPEC-REVIEW-306-r1.md | 291 +++++++++++++++++++++++++++++ 1 file changed, 291 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-306-r1.md diff --git a/docs/reviews/SPEC-REVIEW-306-r1.md b/docs/reviews/SPEC-REVIEW-306-r1.md new file mode 100644 index 00000000..7752b46d --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-306-r1.md @@ -0,0 +1,291 @@ +# SPEC-REVIEW-306-r1 + +- **Issue:** [#306](https://github.com/Matysh/houseplan-card/issues/306) — Редактор плана: заменить виртуальные стены обычными стенами толщиной 0 +- **Этап:** ревью ТЗ (PROCESS.md §2.4) +- **Заход:** r1 · блокирующих циклов израсходовано 0 из 4 (до этого раунда) +- **ТЗ:** `docs/specs/306-zero-thickness-walls.md`, зафиксировано коммитом `904c47e44c7d08ec3e029678226a304fa3a15cc8` («docs: specify zero-thickness wall migration»), это же HEAD ветки на момент ревью +- **Метка issue:** `S4-spec-review` (не `small`) — полный трек, ТЗ обязано жить в `docs/specs/`, что выполнено + +## Скоуп ревью + +Первый заход, полный разбор (§2.10 неприменим — это не повторный раунд). +Проверено: тело issue #306 и все 3 комментария (аналитика автора, дополнение +владельца о `zero_wall_style`, хендофф автора ТЗ); файл +`docs/specs/306-zero-thickness-walls.md` целиком (654 строки, 19 разделов, +18 AC); соответствие `docs/SCOPE.md` (J4/J6), `AGENTS.md`, `PROCESS.md` §7.1 и +§2.5 (DoR-чеклист); сверка технических утверждений ТЗ с текущим кодом +(`custom_components/houseplan/validation.py`, наличие модулей из §14) и с +каноническими документами `docs/LIGHT.md`, `docs/WALL-THICKNESS.md`, +`docs/TOUCH-SUPPORT.md`, `docs/CONFIG-COMPATIBILITY.md`, +`docs/USER-GUIDE.ru.md`; сверка структуры документа с прецедентами полного +трека — `docs/specs/299-mixed-role-wall-records.md` и +`docs/specs/224-config-coordinate-canonicalization.md` (оба приняты, оба того +же автора/жанра: геометрия стен/схема данных). + +## Как проверялось + +- `git show --stat 904c47e4` — подтверждён коммит и его состав (только два + файла: сам ТЗ и `docs/specs/README.md`). +- Построчное чтение ТЗ против обязательных разделов §7.1 и DoR §2.5. +- Точечная верификация фактических утверждений о текущей системе, а не + доверие тексту ТЗ на слово: + - `custom_components/houseplan/validation.py:1236,1264,1286` — `cm` для + `walls[]`/`room_drafts[].segments[]`/`partitions[]` действительно + `Range(min=1, max=100)`; `:1309` — `wall_columns[].cm` действительно + `Range(min=1, max=150)`. Оба факта, на которых стоит модель диапазонов + §4.1 и §2 п.2, подтверждены кодом, а не только словами автора. + - `custom_components/houseplan/const.py:54` — `PLAN_MODEL_VERSION = 7`, + совпадает с заявленным переходом «7 → 8» (§4.1, §8). + - `custom_components/houseplan/validation.py:911,913,917,921` — + `MAX_OPENINGS/MAX_WALLS/MAX_WALL_COLUMNS/MAX_OPEN_SPANS = 500`, совпадает с + лимитом 500 в §8 п.9, §11 и рисках §18. + - Существование всех перечисленных в §14 существующих модулей + (`wall-thickness.ts`, `physical-geometry.ts`, `wall-face-graph.ts`, + `wall-merge.ts`, `open-spans.ts`, `houseplan-card.ts`, + `light-visibility.ts`, `sun.ts`, `iso-walls.ts`, `plan-optimizer.ts`, + `plan-geometry-preflight.ts`, `coordinate-canonicalization.ts`, + `coordinate_canonicalization.py`) — все на месте; `zero-walls.ts` заявлен + как новый, и ТЗ прямо говорит, что имя можно менять — не выдаётся за + существующий факт. + - `docs/LIGHT.md` — правило «bare outline of any room edge with no + thickness is opaque» и «virtual (open) boundaries are transparent» + совпадает с AC3 (отсутствующая запись = легаси физическая стена, + непрозрачна) и с fallback `dashed` (прозрачна) — ТЗ не противоречит + принятой световой модели. + - `docs/TOUCH-SUPPORT.md:9,38,95,114,136,166` — «View/kiosk touch-first, + editors best effort» совпадает с §12 ТЗ. + - `docs/CONFIG-COMPATIBILITY.md` (раздел «Status meanings», строки 37-46) — + сверка предложенных в ТЗ статусов реестра с действующим перечнем. См. + находку M3. + - `docs/USER-GUIDE.ru.md` — полнотекстовый грep по «Граница/виртуальн» + (строки 58, 432, 527-548, 1439, 1473, 1496-1499, 1641, 1649, 1682) — + подтверждено, что RU-документ подробно описывает удаляемый инструмент. + См. находку M2. + - Структурная сверка с `docs/specs/299-*.md` и `docs/specs/224-*.md` + (`grep '^## '`) — оба начинаются разделами «Сценарий» и «Что человек + увидит до и после», оба содержат отдельный раздел «План автотестов» и + «Принятые технические предположения». См. находку M1. +- `gh issue view 306 --json ... --comments` — прочитаны все три комментария + целиком, включая уточнение владельца про световой режим (второй комментарий, + 15:10:53Z), которое ТЗ полностью учло (таблица §2 п.4 совпадает буква в + букву). + +## Находки + +### M1 (Medium, в скоупе) — отсутствуют два обязательных первых раздела ТЗ + +**Файл:** `docs/specs/306-zero-thickness-walls.md`, строки 1–24 (§1 «Цель» +идёт первым разделом). + +PROCESS.md §7.1 называет обязательный список разделов ТЗ и отдельно +подчёркивает: «Два первых раздела — продуктовые, и они идут первыми не +случайно. **Сценарий:** какая персона (`docs/SCOPE.md`), на какой +поверхности, в какой момент это встретит. **Что человек увидит:** одной +фразой, без терминов реализации. ТЗ, которое не может ответить на эти два +вопроса, описывает работу, а не изменение продукта.» + +В ТЗ #306 таких разделов нет вообще — ни как отдельных заголовков, ни как +абзаца внутри «Цели». Документ начинается прямо с технической формулировки +(«Убрать из продукта отдельный инструмент «Граница» и канонические сущности +`space.open_spans`/`rooms[].open_to`»). Персона (администратор дома, +`docs/SCOPE.md` J4/J6), поверхность (Plan editor, десктоп) и момент +(рисование/правка стены, настройка пространства) угадываются по контексту, но +не зафиксированы явно, и однофразового «до/после» без терминов реализации в +документе нет вообще — есть только раздел issue «Результат для пользователя» +(не часть ТЗ-файла) с пятишаговым технически описанным сценарием. + +Это не единичное упущение: два предыдущих принятых ТЗ той же линейки — +`docs/specs/299-mixed-role-wall-records.md` (§1 «Сценарий», §2 «Что человек +увидит до и после») и `docs/specs/224-config-coordinate-canonicalization.md` +(§1 «Сценарий, персона и момент», §2 «Что человек увидит до и после») — +оба следуют этому требованию буквально с теми же заголовками. #306 из этого +ряда выпадает. + +**Сценарий отказа:** без явного «до/после» ревьюер и будущий читатель ТЗ +проверяют соответствие продукту (`docs/SCOPE.md`) по разрозненным техническим +формулировкам §1–§2, а не по одному предложению. Для задачи этого размера +(654 строки, смена модели данных, 18 AC) риск разъехаться с тем, что владелец +на самом деле утвердил, — не гипотетический: второй комментарий владельца сам +поправил первую аналитику автора («стиль не влияет на геометрический смысл» +→ отменено), то есть продуктовая рамка здесь уже была неоднозначной один раз. + +Не High: ни один из 18 AC не становится из-за этого неоднозначным или +недоказуемым — сам контракт поведения (§2, §5–§8) детален и проверяем. +Дефект структурный, чинится в этом же ТЗ без переоценки AC. + +**Почему это Medium, а не High:** реализация не может пойти по неверному пути +из-за отсутствия этих разделов — все AC самодостаточны; речь о структурной +полноте документа, а не о содержательной дыре в контракте. + +### M2 (Medium, в скоупе) — `docs/USER-GUIDE.ru.md` не входит в список затрагиваемых/release-документов + +**Файл:** `docs/specs/306-zero-thickness-walls.md`, строка 424 (§14 +«Затронутые модули») и строка 613 (§17 «Release-артефакты»). + +Оба места называют `docs/USER-GUIDE.md`, но не `docs/USER-GUIDE.ru.md`. +Проект ведёт русский и английский user-guide отдельными файлами (как +changelog: `docs/CHANGELOG.md` + `docs/CHANGELOG.ru.md`, оба явно +упомянуты в этом же ТЗ). AGENTS.md прямо требует: «для работы, которая меняет +видимое поведение, также читай `docs/USER-GUIDE.ru.md` — терминология +интерфейса берётся оттуда […] или UI начинает говорить на языке разработчика». + +`docs/USER-GUIDE.ru.md` сейчас содержит развёрнутое описание удаляемого +инструмента «Граница»/термина «виртуальная стена»: таблица инструментов +(строка 432 — «Граница | Виртуальный участок общей стены | …»), отдельный +раздел «Виртуальные стены» (строки 529–549, с таблицей ситуаций: создание, +закрытие, объединение соседних участков, поведение проёмов, толщины, света, +просмотра, редакторов, «внешнюю стену сделать виртуальной нельзя», +«ответвление из середины не поддерживается»), упоминания в разделах Optimize +(строка 1439), удаления комнаты (строка 1447), совпадающей partition (строки +1496–1499), инвариантов (строка 1641) и лимитов хранилища (строка 1649: «500 +записей стен и 500 виртуальных участков»). После реализации #306 весь этот +материал станет неверным (инструмента не будет, термин исчезает, лимит +`MAX_OPEN_SPANS` перестаёт быть отдельным лимитом пользовательского объекта). + +**Сценарий отказа:** реализатор берёт §14/§17 как чек-лист файлов для правки +в том же коммите (правило AGENTS.md «документация — в том же коммите, что +поведение», правило 11 PROCESS.md §3). Раз файл не назван — обновление RU +user-guide не гарантировано формально, и продукт получает ту же болезнь, что +уже отмечена как известный пробел в `docs/SCOPE.md` («README predates the +two-editor redesign») — только на этот раз с описанием несуществующей более +функции, а не отставанием скриншотов. + +**Почему Medium, не High:** не блокирует ни один AC (AC18 требует +согласованности RU/EN user docs в общем виде и формально не провалится, если +проверяющий не заметит отсутствие явного файла в списке) и легко чинится +добавлением одной строки в §14 и §17. + +### M3 (Medium, в скоупе) — статусы compatibility-реестра в §4.2 не совпадают с действующим перечнем `docs/CONFIG-COMPATIBILITY.md` + +**Файл:** `docs/specs/306-zero-thickness-walls.md`, строки 135–136 (§4.2): +«добавляются в `docs/CONFIG-COMPATIBILITY.md` как `deprecated-read / +project-in-memory / migrate-on-structural-write`». + +Действующий раздел «Status meanings» в `docs/CONFIG-COMPATIBILITY.md` (строки +37–46) определяет ровно пять статусов: `decision-required`, `deprecated-read`, +`migrate-on-write`, `migrate-on-settings-save`, `drop-on-validation`. Ни +`project-in-memory`, ни `migrate-on-structural-write` в этом перечне не +существует. Документ там же называет `scripts/config-field-registry.mjs` +машиночитаемым источником истины для этих статусов — то есть это не +просто описательный текст, а контролируемый словарь, который потребляет +тулинг (`npm run audit:config`). + +Показательно, что первый комментарий автора к этому же issue (аналитика, +раздел «Рекомендуемый целевой контракт», пункт про #33) корректно ссылался на +существующую пару `deprecated-read/migrate-on-write` — а финальный текст ТЗ +разошёлся с собственной же более ранней и верной формулировкой. + +**Сценарий отказа:** реализатор либо придумывает новый статус в реестре без +отдельного решения (реестр — «машиночитаемый источник истины», расширение +словаря — не тривиальная правка одного файла документации, а изменение +контракта тулинга), либо использует несуществующий статус как текстовую +метку не в реестре, и `docs/CONFIG-COMPATIBILITY.md` расходится с +`scripts/config-field-registry.mjs` — то есть ровно то рассогласование, которое +этот документ существует, чтобы предотвратить. AC15 («`docs/ +CONFIG-COMPATIBILITY.md` описывает оба deprecated поля и downgrade») не +называет, какой из двух наборов статусов правильный, и в текущем виде +недоказуем однозначно. + +**Предлагаемая правка (техническое решение, не продуктовое, автор решает +сам):** либо переиспользовать `deprecated-read` (для чтения) и +`migrate-on-write` (для записи) — оба уже существуют и по смыслу подходят, — +либо явно завести новый статус в самом реестре `config-field-registry.mjs` и +задокументировать его добавление как часть этой задачи, а не просто +упомянуть в prose. + +**Почему Medium, не High:** это техническое решение (не продуктовое), решается +автором/ревьюером без владельца (§7.1), не меняет ни один пользовательский AC +и не выглядит как факт, выданный за поведение продукта — просто +несогласованная терминология, дешёвая в правке. + +### L1 (Low, снимается с записью) — нет консолидированного блока «принято предположительно» + +§7.1 требует явный блок в конце ТЗ для нерешённых технических деталей +(«принято предположительно, поменять свободно»), и оба предыдущих ТЗ той же +линейки (#299 §15, #224 §16) его содержат отдельным разделом. В #306 такого +раздела нет, хотя технические допущения по факту делаются: имя модуля +`src/zero-walls.ts` («имя можно уточнить без изменения контракта», §3.3), +конкретные пороги производительности 10%/5% (§11), выбор статусов реестра +совместимости (см. M3). Часть из них снабжена инлайн-пометкой о свободе +изменения (§3.3), но не собрана в один блок, который ревьюер мог бы +предметно оспорить целиком. + +**Решение ревьюера:** не блокирует и не требует отдельной правки — все +затронутые технические решения либо помечены инлайн как свободные к +изменению, либо уже пойманы отдельными находками (M3). Снимается с этой +записью; при следующем ТЗ той же линейки стоит вернуться к практике отдельного +раздела. + +## Что проверено и признано корректным + +- **Структура AC.** Все 18 AC пронумерованы, у каждого есть однозначный + критерий и явный способ доказательства (unit/backend/smoke/golden), как + требует DoR §2.5. Ни в одном AC не найдено недоказуемой или расплывчатой + формулировки. +- **Продуктовые решения зафиксированы, открытых вопросов нет.** §2 п.10: + «Открытых продуктовых вопросов нет» — подтверждается: оба комментария + владельца (включая уточнение про световой режим как настройку пространства, + а не свойство стены) полностью учтены текстом §2 п.4 и §7.1 светового + раздела; расхождений между тем, что попросил владелец, и тем, что описывает + ТЗ, не найдено. +- **Инвариант «отсутствие записи ≠ нулевая стена» (AC3)** — центральный + инвариант всей миграции — сформулирован явно и многократно (§1, §3.2, §8 + п.5, риски §18) и подтверждён действующей моделью света (`docs/LIGHT.md`: + «a wall is still a wall when it is drawn as a line» — оpaque). + Ложного автоматического преобразования легаси оси в открытую стену + документ не допускает ни в одном месте. +- **Защита проёмов от потери (AC10)** — контракт «никогда не удалять + автоматически» повторяется консистентно в §6.2, §8 п.6, §18 и первом + комментарии автора; нет расхождений между разделами. +- **Диапазоны толщины (0..100 для стен/перегородок, 1..150 для колонн)** + проверены по факту в `validation.py` — текущие серверные ограничения + совпадают с тем, что автор заявляет как «сейчас», so предлагаемое изменение + диапазона корректно описывает дельту, а не выдумывает её. + и учитывает существующий предел колонн, не трогая его — согласовано с + `docs/WALL-THICKNESS.md`. +- **Touch/View floor (§12, AC16)** согласован с `docs/TOUCH-SUPPORT.md`: + View/kiosk — блокирующий гарантированный уровень, Plan editor — best effort, + ровно так, как определяет каноника. + Ссылки на существующие модули (§14) — все перечисленные существующие файлы + на месте; `zero-walls.ts` заявлен как новый, что не является ложным фактом. +- **Совместимость и откат (§10)** реалистичны: явно назван unsupported + downgrade после записи `cm:0`, явно исключён labs-флаг с обоснованием (две + одновременно пишущие модели опаснее фиче-флага) — техническое решение + автора, не спрятанное как факт. +- **Зависимости и вне скоупа (§19)** соответствуют комментарию-аналитике: + #282 верно не блокирует эту задачу, но и не дублируется второй новой + сущностью; #148/#173 получают superseded-заметку, а не переписывание задним + числом (§14) — проверено, что #173 не содержит прямо противоречащего + контракта «Границы» (только общеанглийское слово «boundary» = край комнаты), + так что заявленное действие соразмерно фактическому конфликту. + +## Чего не проверял + +- **Реализуемость световой модели «line barrier без площади» в текущем + ray-sweep (`splitAtIntersections`/`visibilityPolygon`)** — не запускал + никакой код и не писал прототип; сверил только то, что автор ТЗ явно + называет эпсилон-риск и ограничивает его границами numeric predicate (§7.1, + §11), то есть заранее не выдаёт непроверенное решение за факт. Технический + риск реален (это по сути новый тип барьера, а не полигон), но он назван + как риск в самом ТЗ (§18: «Glow и sun разойдутся» / «единый resolver»), а + не спрятан — на этапе ТЗ этого достаточно, полную проверку сделает + код-ревью и `test/light-visibility.test.mjs`/`test/sun.test.mjs`. +- **Полный список `docs/specs/README.md`** на прочие несоответствия вне + #306 — не аудировал. +- **Гейты/тесты не запускал** — на этапе ревью ТЗ нет кода для типографии/ + сборки; авторская проверка (`node --test test/docs-accept.test.mjs + test/process-gate.test.mjs` — 37 passed, 1 skipped) не перепроверялась + повторным запуском, так как класс изменений — только `docs/**` (класс C) и + утверждение автора относится к формальным doc-гейтам, не к продукту. +- **`ADR #282` и его точная граница с #306** — прочитан только через ссылки + из issue-комментария и §19 ТЗ, сам ADR-файл не читался целиком. + +## Вердикт + +Три находки Medium, все в скоупе задачи, все дешёво чинятся правкой текста +ТЗ без пересмотра AC или продуктовых решений. High-находок нет: ни одна не +делает какой-либо AC недоказуемым, неоднозначным или основанным на выданной +за факт догадке — предложенный контракт поведения детален, самосогласован и +там, где я мог сверить его с кодом/канон-документами, подтверждён. + +`Вердикт: жёлтый · заход r1 · блокирующих циклов 1/4 · High: 0 · Medium: 3 → в задаче`