Files
houseplan-card/docs/reviews/SPEC-REVIEW-329-r1.md
2026-08-27 23:29:49 +03:00

18 KiB
Raw Permalink Blame History

SPEC-REVIEW-329-r1

Issue: #329 · Этап: spec (S4-spec-review) · Заход: r1 · Трек: обычный ТЗ: docs/specs/329-junction-limits.md, коммит 8dce8453975121a89263d44243e49059f7000da7 (докоммит: docs: spec #329 — wall junction limits and an honest sharp apex, только класс C — docs/specs/**, продуктовый код не тронут).

Скоуп ревью

Изменение — новый файл ТЗ на 151 строку, без сопутствующих правок кода. Задача не помечена small/trivial, поэтому полноценный файл ТЗ обязателен — формально верно. Ревью охватывает весь документ целиком (заход первый, дельта = документ полностью).

Продуктовая рамка (docs/SCOPE.md): задача — правка визуального дефекта масонри (некорректный рендер стыка стен) плюс новый барьер на запись геометрии. Это не новая функция, а восстановление корректности существующей геометрической модели (примыкает к J6 «keep the plan true» и общей целостности рендера стен, на которой держатся J1/J5/J7). Конфликта со SCOPE не вижу, владелец решение принял явно в чате issue.

Как проверялось

  • Прочитаны docs/SCOPE.md, PROCESS.md (весь, включая §2.4, §2.10, §7.1), AGENTS.md.
  • Прочитано тело issue #329 и оба комментария владельца (gh issue view 329 --comments) — сверены пять нормативных ограничений П1–П5 из ТЗ дословно с решением владельца в чате: совпадают без искажений.
  • Прочитан канонический docs/WALL-THICKNESS.md целиком, включая разделы про #309 (visual mitre limit / chamfer) и #310 (pair apex / butt-end trim), на которые ссылается §4 ТЗ — ссылки корректны, терминология («мitre», «биссектриса», «внутренние грани») не расходится с каноном.
  • Прочитан docs/USER-GUIDE.ru.md: раздел «Инструменты плана», таблица Resize, раздел про геометрическую проверку/диагностику при Optimize — искал установленную терминологию для UX отказа записи (см. находку M1).
  • Проверено git show --stat на коммит ТЗ: тронут только docs/specs/329-junction-limits.md, продуктовый код и гейты не задеты — прогон typecheck/test/build для чисто документационного коммита класса C не требуется (PROCESS.md §1 таблица классов, §8 — гейты код-ревью), не прогонял.
  • Grep по custom_components/houseplan/validation.py — подтверждён прецедент семантической дельта-валидации (invalid_partition_opening_jamb_margin), на который ссылается §5 ТЗ: ссылка не выдумана, паттерн существует.
  • Grep по src/grid-scale.ts, src/plan-optimizer.ts — подтверждено, что cell_cm — реальный настраиваемый параметр (диапазон 0.1–1000 см, по умолчанию 5 см), не константа — относится к находке L1.

Находки

M1 (Medium, в скоупе) — UX отказа записи заявлен как факт, а не как решение, и не согласован с уже существующим поведением Resize

docs/specs/329-junction-limits.md, §2 (нормативные ограничения, преамбула): «Нарушение = отказ всей записи, план не изменён, тост называет конкретное правило.» §3 распространяет это на все пять поверхностей записи: рисование и завершение контура, черновики, независимые стены/перегородки, Resize, инструмент «Толщина», merge/split.

Это утверждение о видимом поведении — то есть продуктовый факт, а не техническая деталь. Но docs/USER-GUIDE.ru.md уже фиксирует для Resize контракт отказа, который тостом не является:

  • строка 482: «Боковая стена перестала бы быть общей... → ручка объясняет: «Нельзя сдвинуть только часть общей стены»»;
  • строка 483: «Общая граница частичная... → ручка остаётся видимой, но приглушена; наведение, фокус или нажатие объясняет запрет»;
  • строка 492–493: «Неоднозначный результат отклоняется целиком...» — без упоминания тоста;
  • для пакетной геометрической проверки Optimize (строка 1438) отказ оформлен отдельным диалогом с «Скопировать диагностику», тоже не тостом.

Т.е. в продукте уже как минимум два разных established UI-паттерна отказа структурной записи (приглушённая ручка с текстом на фокус/hover — для Resize; диалог с диагностикой — для пакетной геометрической проверки), и ни один не является тостом. ТЗ утверждает единый тост для всех пяти поверхностей, не упоминая эти прецеденты и не помечая выбор как предположение в §9. AC7 (Resize/«Толщина») тоже не называет способ обратной связи — только «отказ fail-closed, план байт-неизменен», что расходится с общей формулировкой §2.

Это ровно тот класс дефекта, о котором предупреждает процесс: «утверждение о поведении, которого нет ни в одном документе и которое не помечено как предположение». Здесь оно к тому же противоречит документированному поведению Resize.

Чем это грозит. Реализация по букве §2 добавит тост поверх уже существующей ручки-объяснения на Resize — пользователь увидит два одновременных, возможно противоречащих, сообщения об одном и том же отказе, либо автор в процессе кодирования тихо перепишет UX Resize под тост, что является расширением скоупа (смена established UX-контракта отдельного инструмента) без отдельного продуктового решения.

Что нужно сделать. Явно расписать, какой канал обратной связи используется на КАЖДОЙ из пяти поверхностей (§3), сверяясь с уже задокументированным поведением каждого инструмента: тост уместен там, где сегодня нет собственного канала (рисование/завершение контура, черновики, merge/split), а для Resize — либо переиспользовать существующий приглушённая-ручка-и-объяснение канал (без тоста), либо явно зафиксировать как новое продуктовое решение и передать вопрос владельцу по правилам §7.1 («что человек видит», с предлагаемым вариантом по умолчанию).

M2 (Medium, в скоупе) — AC5 описывает два разных теста одной фразой и непроверяем в текущей формулировке

docs/specs/329-junction-limits.md, §8, AC5:

«Повторение «шпиля» из фикстуры issue рисованием — отказ по просвету (даже если каждый угол ≥ 15°, но толщины съедают комнату).»

Фикстура issue (houseplan-space-test2-2026-08-27_16-14-48.json, комната rmtbq3k5e-0) имеет вершину ≈9.9° — она нарушает П1 (угол < 15°) сама по себе. Отказ по этой фикстуре AC1 уже проверяет (угол). Формулировка AC5 одновременно (а) ссылается на эту же фикстуру и (б) оговаривает «даже если каждый угол ≥ 15°» — то есть описывает СОВЕРШЕННО ДРУГУЮ геометрию (все углы проходят П1, но толщины стен всё равно съедают просвет комнаты ниже 25 см²). Это два разных теста, слитых в один AC, и как единое проверяемое утверждение он не воспроизводим: не названо, каким конкретно углам/длинам/толщинам соответствует случай «угол ≥15°, но просвет < 25 см²» — то есть ровно то несогласованное место, где догадка выдаётся за факт, если разработчик сам подберёт числа.

Что нужно сделать. Разделить на AC5a (повтор фикстуры issue — отказ, допустимо по любому из П1/П5, тест уже частично покрыт AC1) и AC5b — отдельная синтетическая геометрия с конкретными числами (углы, длины, толщины), где все углы ≥15° проходят П1, но просвет всё равно < 25 см², и именно она доказывает независимость П5 от П1.

L1 (Low) — параллель «5 см = 1 клетка» вводит в заблуждение при нестандартном cell_cm

docs/specs/329-junction-limits.md, §2, П4 и П5: «не ближе 5 см (1 клетка)» и «≥ 25 см² (1 клетка 5×5)». cell_cm — реальный настраиваемый параметр пространства (0.1–1000 см, src/grid-scale.ts, src/plan-optimizer.ts), а не константа. Числа П4/П5 читаются как абсолютные сантиметровые пороги (не масштабируются с cell_cm) — это, видимо, и есть решение владельца (в его формулировке те же 5 см/25 см² даны как абсолютные величины, «клетка» — только пояснение при дефолтной сетке). Но параллель «(1 клетка)» рядом с абсолютным числом может подтолкнуть реализацию к ошибочному масштабированию порога на cell_cm пространства, отличном от 5 см. Снимается редакционно: убрать скобку «(1 клетка...)» из нормативного текста П4/П5 либо явно дописать «абсолютная величина, не зависит от cell_cm пространства». Не блокирует — правится или снимается решением ревьюера; фиксирую как Low, автор может снять с той же формулировкой.

Что проверено и корректно

  • Пять численных ограничений П1–П5 в ТЗ дословно совпадают с решением владельца в чате issue — искажений или добавленных «от себя» цифр нет.
  • Граница применения (§3): проверки только на записи, изменяющей геометрию; легаси/импорт/восстановление/миграция не блокируются; Optimize не обязан чинить легаси, но не должен создавать новых нарушений — сформулировано как явный пост-условный AC (AC10), проверяемо.
  • §4 (рендер легаси-острия) корректно опирается на существующий канон: #309 (visual mitre limit / chamfer) и #310 (pair apex, «нода из ровно двух лучей») в docs/WALL-THICKNESS.md действительно описывают именно тот механизм, который вырождается в «трезубец» при полном перекрытии тел — ссылки точны, не выдуманы.
  • Геометрический контракт фаски §4 явно и корректно продублирован в §9 как «принято предположительно, поменять свободно» — не выдаётся за окончательное решение, ревьюер вправе оспорить: не оспариваю, реализация разумна.
  • §5 (бэкенд): ссылка на существующий паттерн семантической дельта-валидации (invalid_partition_opening_jamb_margin в custom_components/houseplan/validation.py) подтверждена чтением кода — прецедент реален, а не придуман для ТЗ.
  • Обязательные разделы §7.1 присутствуют по содержанию (сценарий, что видит пользователь, проблема, скоуп/не-скоуп — слиты в §1/§3, но содержательно покрыты, контракт поведения §2/§4, модель данных/миграция §3/§6, i18n §6, AC1–AC10 с указанием способа доказательства, план автотестов §6, риски §10, откат §11, release-артефакты §6). Формальных пропусков разделов нет.
  • Новых полей конфига нет, config-field-registry не трогается — заявлено явно (§6), согласуется с docs/CONFIG-COMPATIBILITY.md (миграция/импорт легаси не блокируются, что и требует этот документ).
  • AC1–AC4, AC6–AC10 однозначны, у каждого названы конкретные граничные числа и способ доказательства (unit/smoke/golden/backend), тест умеет различать pass/fail на границе — реализуемо без дополнительных догадок.
  • Открытых догадок, выданных за факт технических решений, не найдено за пределами M1 (продуктовый вопрос UX) — все технические допущения явно оформлены в §9 «принято предположительно».
  • Артефакт корректного класса: файл docs/specs/329-junction-limits.md, имя соответствует <NN>-<slug>.md, задача не small, полноценный файл обязателен и присутствует.

Чего не проверял

  • Код не менялся в этом коммите (только docs/specs/**, класс C) — гейты typecheck/test/build/check-docs/инварианты модели не прогонял: они не относятся к чисто документационной правке на этапе spec-review и прогоняются на этапе код-ревью.
  • Не проверял, как именно физтела считаются в кэше для П5 (реализация ещё не существует) — это заявлено как assumed в §9, оценка технической реализации относится к код-ревью.
  • Не проверял golden-матрицу на предмет «нет вершин острее 15°» — ТЗ само требует подтвердить это прогоном на этапе реализации (§6), это не задача spec-ревью.
  • Не запрашивал у владельца дополнительных продуктовых уточнений — вопрос M1 возвращается автору ТЗ как находка, а не выносится владельцу напрямую (по регламенту дуб. технических/продуктовых вопросов на этапе ревью решает ревьюер вплоть до исчерпания лимита циклов).

Вердикт

Жёлтый. Два Medium в скоупе задачи (M1, M2), High-находок нет. Оба чинятся в тексте того же ТЗ без смены его архитектуры; отдельный issue не заводится (#202). Low (L1) — по усмотрению автора, править или снять записью.