Files
houseplan-card/docs/reviews/SPEC-REVIEW-123-r1.md
2026-08-13 18:55:49 +00:00

14 KiB
Raw Permalink Blame History

SPEC-REVIEW-123-r1

  • Issue: https://github.com/Matysh/houseplan-card/issues/123
  • ТЗ под ревью: docs/specs/123-corner-split-wall.md (коммит ba56d4f)
  • Роль: ревьюер ТЗ (не автор), этап S4-spec-review
  • Трек: обычный (не small) — оценка сложности 7/10, больше одной поверхности, визуальная и световая геометрия; лёгкий трек корректно не применён
  • Цикл: r1/4

Скоуп ревью

Проверялось соответствие ТЗ:

  • docs/SCOPE.md — попадание в Core user jobs, отсутствие расширения скоупа;
  • PROCESS.md §2.4, §2.5 (DoR), §7.1 (обязательные разделы) и §12 (запреты);
  • AGENTS.md — классы файлов, ветка, легитимность приёма чужого issue в процесс;
  • каноническим документам затронутой подсистемы: docs/WALL-THICKNESS.md, docs/SUN.md, docs/LIGHT.md, docs/ISOMETRIC.md, docs/TOUCH-SUPPORT.md, docs/CONFIG-COMPATIBILITY.md;
  • docs/USER-GUIDE.ru.md — терминология «Split» / «Перегородка»;
  • фактическому состоянию кода (src/wall-thickness.ts, src/iso-walls.ts, src/space-render.ts, test/wall-thickness.test.mjs) — на предмет того, что технические утверждения ТЗ не являются непроверенной догадкой.

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

  1. Прочитан весь тред issue #123, включая решение владельца о приёме чужого issue в процесс (после правки конвейера, коммит 024cdc0, issue #114) и протокол аналитики с defaults Q1–Q3, принятыми владельцем 2026-08-13 (комментарий https://github.com/Matysh/houseplan-card/issues/123#issuecomment-5283843252).
  2. Сверены обязательные разделы ТЗ (§7.1 PROCESS.md) построчно — см. таблицу ниже.
  3. Прочитан код wallBodiesGeometry() (src/wall-thickness.ts:1361-1412): подтверждено, что тело стены строится как per-room outset(poly, half) − inset(poly, half), затем union по комнатам — именно механизм, который ТЗ называет причиной дефекта (диагональный Split из вершины вносит острые митры дочерних комнат в наружный union).
  4. Прочитан src/iso-walls.ts и docs/ISOMETRIC.md — подтверждено, что скрытая изометрия уже потребляет тот же wallBodiesGeometry() MultiPolygon, а не отдельную модель; утверждение ТЗ §6.4 о единой геометрии для Plan/View/ houseplan-space-card/изометрии не является новым архитектурным изобретением автора, а фиксирует уже существующий контракт.
  5. Прочитан docs/LIGHT.md («Opaque: the wall bodies exactly as the plan draws them (wallBodiesGeometry)») — подтверждает AC7 (Glow/солнце используют то же исправленное preграждение) технически достижимым без отдельной правки light-барьеров.
  6. Прочитан docs/TOUCH-SUPPORT.md — формулировка «safety floor» и «pointer cancellation» в ТЗ §9 дословно соответствует канону, а не придумана.
  7. Прочитан docs/USER-GUIDE.ru.md (таблица инструментов, разделы «Split» и «Перегородка») — терминология ТЗ совпадает с пользовательским словарём, различие Split/Перегородка воспроизведено верно и явно вынесено в не-скоуп (п.5.4).
  8. Прочитан test/wall-thickness.test.mjs — регрессионные сценарии, которые ТЗ в §11.1 п.7 требует не сломать (partial shared wall, virtual-T mitre, nested room, 45° wall, split materialisation), реально существуют в файле, то есть план автотестов не ссылается на несуществующее покрытие.
  9. Проверено, что docs/ARCHITECTURE.md, docs/STATUS.md, docs/CHANGELOG(.ru).md существуют — release-артефакты в §13 указывают на реальные файлы.
  10. Проверена запись в docs/specs/README.md — строка на #123 добавлена в том же коммите, ссылка issue ↔ ТЗ двусторонняя.

Обязательные разделы (§7.1 PROCESS.md)

Раздел Есть Комментарий
Сценарий (персона/поверхность/момент) ✅ §1
Что человек увидит до/после (без терминов реализации) ✅ §1, одна фраза
Проблема ✅ §2, с воспроизведёнными числами bbox на 948f284
Скоуп / не-скоуп ✅ §4 / §5
Контракт поведения ✅ §6
UX ✅ §9
Модель данных и миграция ✅ §8
i18n ✅ §9 (пусто, обосновано)
AC1…ACn с доказательством ✅ §10, 13 штук, каждый с типом
План автотестов ✅ §11
Риски ✅ §14
Откат ✅ §15
Release-артефакты ✅ §13

Все обязательные разделы присутствуют и содержательны, не формальные заглушки.

Находки

Находок уровня High и Medium нет.

Low-1 — тип доказательства AC11 не входит буквально в перечень §2.5

Файл: docs/specs/123-corner-split-wall.md:293-295

AC11 помечен (performance + ревью кода). DoR (PROCESS.md §2.5) перечисляет допустимые типы доказательства как unit / backend / smoke / golden / «ревью кода»; литерала performance в этом перечне нет. По существу критерий всё равно доказуем: в ТЗ явно указано «ревью кода» вторым типом, а performance_smoke/large-house benchmark — существующие release-blocking гейты (§11.4 этого же ТЗ, PROCESS.md §8), а не новый вид проверки. Блокирующим не является, но для чистоты трассируемости стоит переформулировать доказательство AC11 как «ревью кода» с явной ссылкой на существующий performance_smoke/ large-house benchmark, не вводя пятый тип доказательства.

Решение ревьюера: Low, не блокирует. Можно поправить формулировку при следующей правке ТЗ или снять с этой записью — оставляю на усмотрение автора, т.к. критерий по сути проверяем и не создаёт риска для DoR.

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

  • Легитимность приёма issue в процесс (чужой автор, но явно допущен владельцем после правки конвейера #114) — не относится к дефектам ТЗ, отдельно зафиксировано в треде issue самим владельцем.
  • Соответствие docs/SCOPE.md: задача закрывает J6 («Keep the plan true as the home evolves») и частично J4 (встроенный редактор без искажений архитектуры), обе строки в статусе «Closed» — это регрессионный баг внутри уже принятой функциональности, а не новая фича и не расширение скоупа.
  • Владелец лично принял defaults Q1–Q3 и приоритет P2 (комментарии 2026-08-13T16:52 и 17:00) — открытых продуктовых вопросов в финальной редакции ТЗ нет, и это корректно: вопросы были заданы и закрыты на этапе аналитики, а не додуманы автором.
  • Технический диагноз причины (per-room outset−inset union, острые митры дочерних комнат Split входят в наружный силуэт) подтверждён чтением src/wall-thickness.ts — не голословное утверждение автора.
  • Раздел 16 «Принятые технические предположения» корректно отделяет свободно изменяемые технические решения (имена helper'ов, конкретная boolean- декомпозиция, имя golden/smoke сценария) от решений владельца Q1–Q3, которые пересмотру не подлежат — ни одна догадка не выдана за факт без пометки.
  • Не найдено ни одного утверждения о поведении, которое не следует ни из канонических документов, ни из принятых владельцем defaults, ни из чтения существующего кода, и при этом не помечено как предположение.
  • AC1–AC13 однозначны, у каждого указан тип доказательства и он входит (кроме Low-1) в допустимый по DoR список; план автотестов (§11) даёт конкретный, проверяемый маршрут для каждого, включая явное требование «тест из п.3 обязан краснеть на 948f284» — критерий, защищающий от неспособного падать теста.
  • Не-скоуп (§5) корректно отсекает смежные соблазны (не превращать Split в Перегородку, не трогать инструмент «Перегородка», не менять модель данных, не вводить новый UX для cap/join) — типичные места, где скоуп мог бы незаметно расшириться.
  • Release-артефакты (§13) перечисляют реальные файлы, включая docs/WALL-THICKNESS.md (exterior/shared junction invariant) и docs/USER-GUIDE.ru.md — корректная точка правки терминологии для пользователя.
  • Реестр docs/specs/README.md обновлён тем же коммитом, ссылка issue ↔ ТЗ двусторонняя (PROCESS.md §7.1).

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

  • Не проверял, что предложенная в §7 архитектурная декомпозиция (exterior envelope vs shared divider body) реализуема без регрессии в polyclip-ts-based boolean операциях — это по правилам ТЗ (§16 п.2) свободно изменяемое техническое предположение автора кода, не предмет ревью ТЗ.
  • Не проверял производительность реального large-house benchmark — AC11 предполагает существующий гейт, а не новый, и это станет предметом ревью кода/пре-релизного гейта, не ревью ТЗ.
  • Не запускал никаких автотестов — на этапе spec это не требуется; проверка существования регрессионных сценариев (см. «Как проверялось», п.8) сделана чтением файла, не исполнением.
  • Не проверял корректность конкретных числовых bbox-диагностик из §2 — доверяю записи владельца/автора в треде issue как источнику числа, поскольку оно уже независимо зафиксировано в комментарии аналитики до написания ТЗ.

Вердикт

Зелёный. High: 0, Medium: 0. Одна находка Low (AC11 формулировка типа доказательства) — не блокирует, оставлена автору на усмотрение с записью в этом документе (не «TODO», а фиксированное решение ревьюера: можно поправить или отклонить без нового цикла).