diff --git a/docs/reviews/SPEC-REVIEW-584-r1.md b/docs/reviews/SPEC-REVIEW-584-r1.md new file mode 100644 index 00000000..447ae855 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-584-r1.md @@ -0,0 +1,198 @@ +# SPEC-REVIEW-584-r1 + +**Issue:** #584 · **Этап:** ТЗ на ревью (S4-spec-review) · **Заход:** r1 · +**Трек:** полный (аналитика от 2026-09-15 назвала критерий: сложность/риск >3, +несколько модулей и поверхностей, меняется публичный контракт «см. в +свойствах = видимый габарит»). + +**Вердикт: жёлтый** + +--- + +## Скоуп проверки + +ТЗ живёт в теле issue #584 под заголовком `## ТЗ` (комментарий владельца от +2026-09-16T19:33:08Z объявляет переход в `S4-spec-review`). Материал ревью — +текст issue на момент чтения (последний комментарий владельца, тот же, что +поставил статус); отдельного файла в `docs/specs/` нет и не создаётся (архив +закрыт с #517). + +Прочитано перед разбором: `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком +(включая §2.4, §2.5, §2.7, §7.1, §12), тело issue #584 и все 6 комментариев +(аналитика → вопросы Q1/Q2 → ответ владельца → полное ТЗ дизайнеру → +присланный архив → приёмка пака дизайнером-исполнением). `docs/TOUCH-SUPPORT.md` +и канонические документы подсистем (SUN/LIGHT/CANVAS/WALL-THICKNESS/UX-MODES/ +CONFIG-COMPATIBILITY) просмотрены на предмет пересечения — задача их не +затрагивает (чистая геометрия SVG-артворка мебели, не стены/не свет/не солнце). + +## Как проверялось + +Поскольку задача целиком про фактическую геометрию, а не про рассказ, каждое +фактическое утверждение ТЗ сверено с текущим состоянием репозитория (SHA +`7c5fd32a`, `dev`), а не принято на слово: + +| Утверждение ТЗ | Проверка | Результат | +|---|---|---| +| 44 плановых SVG в `svg/plan`, 33 иконки в `svg/menu` | `ls assets/furniture/houseplan-0.3.0/svg/plan \| wc -l`, `svg/menu` | 44 и 33 — совпадает | +| `furnitureRenderTransform` существует | `grep -r furnitureRenderTransform src/` | найден в `houseplan-card.ts`, `houseplan-editor-runtime.ts`, `furniture.ts` | +| `parseSvgPath` — производственный парсер экспорта | `grep -r parseSvgPath src/` | единственное определение в `src/pdf/svg-path.ts` | +| `kitchen_floor` — дизайнерский символ 60×60, `svg/plan/kitchen_floor.svg` | `pack.json` строки 580-600 | `width_cm: 60, depth_cm: 60, file: svg/plan/kitchen_floor.svg` — совпадает | +| `dishwasher` — legacy-примитив 60×60, заполняющий unit box | `pack.json` (единственная запись — `svg/menu/dishwasher.svg`, без `width_cm`) + `src/furniture.ts:155` (`w: 60, h: 60, g: [box(), …]`) | подтверждено: `dishwasher` не входит в дизайнерский пак, это legacy-примитив из `RETAINED_IDS`, `box()` рисует полный `0..1×0..1` | +| `plant` — единственный legacy-символ с полями 2% | `src/furniture.ts:226-231` (`e 0.5,0.5,0.22,0.22` + лучи-линии до `0.02`/`0.98`) | внешний bbox линий действительно `0.02…0.98` — 2% с каждой стороны, совпадает | +| Генератор склеивает `path` простой конкатенацией `paths.join(' ')` | `scripts/generate-furniture-assets.mjs:97` | `return { d: paths.join(' '), … }` — подтверждён ровно описанный дефект | +| `furniture-plan-art.generated.ts`, `furniture-plan-catalog.generated.ts` существуют | `ls src/*.generated.ts` | оба файла на месте | +| `furniture:check` — существующий гейт | `package.json:47` | `"furniture:check": "node scripts/generate-furniture-assets.mjs --check"` | +| `test/furniture-*.test.mjs` — свидетели AC1–AC5 | прочитаны `furniture-stroke-contract.test.mjs`, `furniture-transform-contract.test.mjs` | оба существуют, но проверяют другие контракты (#361/#376/#383 — экранный stroke-resolver, resize/rotate editor-контракт), а не bounds SVG-геометрии; конфликта с новыми AC нет, но новые unit-тесты AC1–AC4, скорее всего, лягут в отдельный новый файл — это техническая деталь, не продуктовая, решать разработчику | + +Все фактические утверждения ТЗ подтвердились чтением кода — ни одной догадки, +выданной за факт, не найдено. Отдельно ценно, что владелец не просто принял +присланный дизайнером пак на слово, а перепроверил его **тем же +production-парсером**, которым будет проверяться AC1/AC2 (комментарий от +2026-09-16T19:33:08Z: 44 из 44, худшее отклонение `0.000000`) — то есть контракт +уже был воспроизведён на реальных файлах ещё до передачи в разработку. + +## §7.1 — обязательные разделы + +| Раздел §7.1 | Есть в ТЗ | Комментарий | +|---|---|---| +| Сценарий | ✅ «Пользовательский сценарий» | персона не названа явно (home admin), но контекст (свойства мебели, план) её однозначно определяет | +| Что человек увидит до/после | ✅ | числа названы (14% по стороне, 30% по площади), формулировка «это исправление, а не регрессия» — по делу | +| Проблема | ✅ | в теле issue выше `## ТЗ` («Что происходит») | +| Скоуп и не-скоуп | ⚠️ распределено | явного заголовка нет; не-скоуп фактически задан AC7 («редактор, hit-area, привязка к стенам, схема конфига и i18n не меняются») — содержание есть, форма нет. Low, не блокирует | +| Контракт поведения | ✅ «Контракт», пп. 1–8 | однозначен, проверен по коду (см. таблицу выше) | +| UX | ⚠️ отсутствует явно | по существу — «нет нового UX-контракта» (задача не трогает интеракции), это следует из AC7, но отдельного раздела нет. Low, не блокирует | +| Модель данных и миграция | ✅ «Совместимость и миграция» | миграции нет, обосновано | +| i18n | ✅ | явное «новых ключей нет» | +| AC1…ACn с доказательством | ✅ AC1–AC8 | у каждого назван способ (unit/golden/review/гейты) | +| План автотестов | ⚠️ распределено | не оформлен отдельным разделом, но каждый AC называет тип теста и файлы перечислены в «Затронутые файлы» — по существу план есть | +| Риски | ✅ «Риски» | 3 пункта, по делу (маскировка регрессии golden-пересъёмкой, склейка путей, рост ленивого чанка) | +| Откат | ✅ | «возврат коммита» — корректно для непереносимой миграции | +| Release-артефакты | ⚠️ частично | changelog RU+EN и golden названы (в «Затронутые файлы»), перф — отдельным разделом; **security явно не упомянут** (ни разу, даже «нет») | + +## Находки + +### Medium (в скоупе) — влияние на touch не названо + +**Файл:** тело issue #584, раздел `## ТЗ`. + +DoR §2.5 явно требует пункт «влияние на touch по `docs/TOUCH-SUPPORT.md` (View +и киоск — блокирующие)» как обязательный, отдельно от i18n и производительности, +которые в ТЗ присутствуют. В тексте ТЗ слова «touch», «тач», «киоск» не +встречаются вообще — раздел не просто краток, он отсутствует. + +**Почему это не тривиальная придирка:** задача меняет физический видимый +размер мебели **в View/kiosk тоже**, не только в редакторе (мебель рендерится +и там, и там одним и тем же `furnitureRenderTransform`). AC7 отдельно +фиксирует «hit-area… не меняются», что фактически закрывает главный +touch-риск (позиция resize/rotate-хэндлов на touch не сдвигается), но это +написано как граница скоупа редактора, а не как ответ на явно +предусмотренный процессом пункт. Реальный риск, скорее всего, нулевой — но +DoR требует **явного** «нет», а не выводимого из соседнего пункта. + +**Как закрыть:** одно предложение в ТЗ, например: «Touch: изменение +непосредственно не затрагивает — View/kiosk остаются readonly-поверхностью, +позиции hit-area и resize/rotate-хэндлов не пересчитываются (AC7); мебель в +редакторе визуально крупнее/меньше на ту же величину, что и в View, новых +touch-жестов не добавляется.» Правки кода не требует. + +**Серьёзность:** Medium, в скоупе — блокирует переход в DoR (§2.5 явно требует +пункт), но правится добавлением одного абзаца в тот же issue, без нового цикла +имплементации. Без High это жёлтый вердикт. + +### Low (снимается с записью, не блокирует) + +1. **Раздел «UX» и «скоуп/не-скоуп» не оформлены отдельными заголовками.** + Содержание по факту присутствует: AC7 однозначно перечисляет, что НЕ + меняется (редактор, hit-area, привязка к стенам, схема конфига, i18n), а + отсутствие нового UX-контракта следует из того же AC7 и из того, что задача + правит только артворк и генератор. Формального заголовка нет, но + переспрашивать автора ради двух заголовков дороже, чем цена неясности — + снимаю с записью. +2. **Security как release-артефакт не назван даже «нет».** Задача не + затрагивает ни одну поверхность из OWASP-класса рисков (правятся SVG-пути + мебели и генератор ассетов, без сетевых вызовов, парсинга пользовательского + ввода на новом пути или новых прав) — риска не вижу, но пункт DoR формально + пуст. Снимаю с записью, а не как Medium: в отличие от touch, здесь нет + спорной поверхности (View/kiosk), которую процесс называет блокирующей + явно. +3. **AC3 «допуск, пропорциональный масштабу» не даёт формулу.** Это разумно + оставить на усмотрение разработчика (технический, не продуктовый вопрос — + §7.1: «то, чего пользователь не наблюдает, агенты решают сами») при + условии, что реализация зафиксирует конкретную формулу и она останется + проверяемой (не «плавает» между прогонами). Ревьюер кода должен убедиться, + что тест «умеет падать» на конкретной, а не абстрактной величине допуска — + отмечаю на будущее, не как находку ТЗ. + +## Что проверено и корректно + +- **AC1–AC8 однозначны и снабжены способом доказательства** (unit / unit-generator + / golden / review / гейты) — ни один не пришлось домысливать. +- **AC2 конкретен**: именно пара `kitchen_floor`/`dishwasher`, оба подтверждены + в `pack.json`/`furniture.ts` как реальные `60×60` записи — тест будет + проверять то, что реально описано в баг-репорте, а не абстракцию. +- **AC4 и контракт п.6** точно описывают дефект склейки путей — сверено с + реальным кодом генератора (`paths.join(' ')`), включая упомянутые + `coffee_table_round`/`table_round`. +- **Ни одной догадки, выданной за факт**: все количественные утверждения + (44 символа, доля полей 2,7–3,9 у дизайнерских, 2% у `plant`, конкретные id + и размеры) подтверждаются либо кодом, либо независимым измерением владельца + тем же production-парсером, которым будет доказываться AC1. +- **Продуктовые вопросы закрыты владельцем предметно** (Q1 — видимый габарит + = осевая линия контура, обводка не считается; Q2 — без миграции, + центрируется в прежнем физическом боксе), открытых продуктовых вопросов в + тексте не осталось; технические решения (склейка путей, `plant`) вынесены в + явный блок «Принятые предположения», как требует §7.1. +- **Границы скоупа (AC7)** явно исключают редактор/hit-area/привязку к + стенам/схему конфига/i18n — правка не расползается на соседние подсистемы. +- **Откат и совместимость** описаны корректно для правки без миграции: `x/y`, + `w/h`, угол, зеркалирование сохраняются, откат — реверт коммита. +- **Риски названы предметно**, включая специфичный для этой задачи риск — + маскировку чужой регрессии golden-пересъёмкой. + +## Чего не проверял + +- Не проверял сам присланный дизайнером ZIP-архив (`fix-584-1`) — + на этапе ревью ТЗ кода/ассетов ещё нет, они появятся в ветке реализации; + их проверка (в том числе повторное измерение AC1/AC2 после факта) — + предмет код-ревью. +- Не запускал `furniture:check`, `npm test`, `golden:verify` и т.п. — на + этом этапе нет ни одной строки кода/ассета, гейты нечего гонять; это + предмет §8 на код-ревью, не ревью ТЗ. +- Не проверял `docs/CONFIG-COMPATIBILITY.md` построчно на предмет + дополнительных полей совместимости сверх того, что решает п.8 контракта — + задача явно не трогает схему конфига (AC7), риск счёл пренебрежимым для + ревью ТЗ. + +## Итог + +ТЗ содержательно сильное: контракт точен, AC проверяемы и уже частично +эмпирически подтверждены владельцем на реальном присланном паке тем же +инструментом, которым будет доказываться AC1. Единственная находка, +требующая правки текста, — отсутствующий явный пункт про touch/View/kiosk, +обязательный по DoR §2.5. High-находок нет, поэтому вердикт жёлтый: +одно предложение в ТЗ, без нового раунда имплементации. + +--- + +## Материал раунда + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/584 +- **Материал ТЗ:** тело issue #584, раздел `## ТЗ`, редакция комментария от + 2026-09-16T19:33:08Z (тот же комментарий ставит `S4-spec-review`). +- **Репозиторий на момент ревью:** `dev` @ `7c5fd32a08f07177d55905d65990180a354dd0ec` + (кода по задаче ещё нет — ревью ТЗ смотрит только текст issue и сверяет его + утверждения с текущим `dev`). + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `7c5fd32a08f0` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `2b57427cbed60f2f3b46aaf1b2c9a84147a8ed88` + ``` + git log --all --format='%H %T' | grep 2b57427cbed6 + ``` +- Тело issue: `a4ec41f212d36f3ec2d313b534161c25a38e58c118ed2f7b97ab437de1593eb3` +- Вердикт конвейера: `yellow` · High 0