diff --git a/docs/reviews/SPEC-REVIEW-123-r1.md b/docs/reviews/SPEC-REVIEW-123-r1.md new file mode 100644 index 00000000..c2b786a4 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-123-r1.md @@ -0,0 +1,165 @@ +# 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», а фиксированное решение ревьюера: можно поправить или +отклонить без нового цикла).