houseplan-card/docs/reviews/SPEC-REVIEW-669-r1.md

SPEC-REVIEW-669-r1

• Issue: https://github.com/Matysh/houseplan-card/issues/669
• Этап: S4-spec-review (ревью ТЗ, PROCESS.md §2.4)
• Трек: полный (метки bug, P1, S4-spec-review; ни small, ни trivial не выставлены — документ, а не только комментарий).
• Материал: тело issue #669, раздел ## ТЗ, + 3 комментария: (1) «Взял» автора с планом A/B-замера, (2) аналитика — таблица замеров, причина регрессии, оценка, решение «вопросов владельцу нет», ссылка на новый #675, (3) поправка к одной строке таблицы замеров.
• Заход: r1 · блокирующих циклов израсходовано 0 из 4
• Роль: ревьюер ТЗ (не автор)

Скоуп ревью

Регрессия производительности в cleanFloorForRoom (src/clean-floor.ts:47): каждый промах кэша чистого пола сразу вычитает все лестницы пространства (geometryAreaMinusStairs → geometryMinusStairs, src/stairs.ts:234-250) — хотя площадь читают только два потребителя (подсказка комнаты, PDF), а путь пола (нужный пяти... по факту четырём местам рендера) вычитания не требует. Задача переносит вычисление площади на момент первого чтения (лениво, мемоизировано в том же кэше) и добавляет отбор лестниц по пересечению bbox перед polyclip. Это разблокирует v1.78.0-beta.5, у которой красный performance-smoke на точном SHA 09873251.

SCOPE-проверка (docs/SCOPE.md): чисто внутренняя оптимизация без нового видимого поведения — под J1 («показать дом сейчас», включая скорость первого кадра) как выполнение уже принятого бюджета, а не новый job. Ничего из «никогда не строить» не задевается.

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

1. Прочитаны docs/SCOPE.md, docs/process/REVIEWER.md целиком; по ссылкам конспекта открыт PROCESS.md §2.3, §2.4, §2.5, §4, §7.1, §7.2 (§2.10 не применялся — это r1).
2. Прочитано тело issue #669 целиком и все 3 комментария (gh issue view 669 --json body,comments).
3. Сверены обязательные разделы §7.1: Сценарий, Что человек увидит до/после, Проблема, Скоуп/Не-скоуп, Контракт поведения, UX/модель данных и миграция/i18n/touch/производительность, Критерии приёмки AC1–AC4 со способом доказательства, План автотестов, Риски, Откат, Release-артефакты, Затронутые файлы, «Принято предположительно» — все на месте, первые два раздела продуктовые и без терминов реализации.
4. Прочитан текущий код src/clean-floor.ts и src/stairs.ts целиком, чтобы проверить, что описание проблемы соответствует коду, а предложенный скоуп реализуем без переписывания контракта:
   • подтверждено: path/geom в cleanFloorForRoom вычитают только физические тела (floorMinusBodies), лестницы в путь никогда не попадали — вычитание лестниц целиком живёт в поле area. Значит лениво откладывать нужно именно и только его; контракт «путь не меняется» (AC1, «Контракт поведения») уже выполняется today и задача его не трогает — только переносит момент вычисления area.
   • подтверждено количество мест, читающих только .path: grep -n "_cleanFloor(" даёт 4 попадания (src/houseplan-card.ts:9059, 9229, 10459, 11011), а не 5, как сказано в разделе «Проблема». Пятое место (:11873) читает весь объект ради .area (подсказка комнаты). Второй потребитель площади — resolveRoomArea (:10184) → та же ._cleanFloor(...).area. Отдельно в src/pdf/pdf-scene.ts:311 есть independent fallback-вызов geometryAreaMinusStairs в обход cleanFloorForRoom (когда resolveRoomArea не вернул число) — он не проходит через ленивый кэш и в ТЗ не упомянут, но и не входит в «Затронутые файлы»; поскольку это редкий fallback-путь, а не типовой рендер кадра, на бюджет AC3 он не давит — оставляю без находки, только для полноты цепочки.
   • подтверждено, что geometryMinusStairsSteps (сводная панель) реализован отдельно от geometryMinusStairs и не переиспользует его — фильтр по bbox внутри geometryMinusStairs (место, названное в «Принято предположительно») не заденет сводную панель автоматически, как и требует «Не-скоуп».
   • сверены оба файла бюджетов demo/performance/budgets-*-smoke.json: spaceSwitchMs — обязательная (hardMaxMs) метрика обоих профилей, а не информационная; лимиты (modelReadyMs 3000/2500, firstStableRenderMs 3500/3000, spaceSwitchMs 1800/1500) совпадают с числами из таблицы наблюдений в issue.
5. Сверены данные аналитики (таблица A/B) с самой формулировкой AC3 — см. находку M1 ниже.
6. Проверена логика ролбэка/release-артефактов на согласованность с политикой docs/STATUS.md (STABLE-тело исключает баги, которые «introduced and fixed strictly inside the beta line»): v1.78.0 ещё не имеет стабильного релиза (git tag -l 'v1.78.0*' — только беты 1–4), значит «changelog — нет» обоснованно, регрессия #663 нигде не выходила стабильным релизом.

Находки

Medium (в скоупе) — AC3 не гарантирует заявленную цель issue

Что не так. «Ожидаемое поведение» (преамбула issue) и AC3 обещают: «После исправления полный Validate точного release-candidate SHA проходит с первого воспроизводимого запуска» / «Полный Validate кандидата слияния и кандидата беты: performance_smoke обоих профилей … проходит modelReadyMs и firstStableRenderMs с первого прогона при неизменных бюджетах». Формулировка называет только эти две метрики, но performance_smoke — единый job с несколькими обязательными (hardMaxMs) метриками в том же прогоне, включая spaceSwitchMs. По собственным замерам аналитики в этом же issue spaceSwitchMs уже проваливается на изометрическом профиле точного SHA 09873251 (1821.2 мс и 1813.5 мс против лимита 1800 мс — два из трёх зафиксированных провальных прогонов), и признаётся «не относящейся к лестницам» — не-скоуп, вынесено в отдельный #675. Ни ТЗ, ни преамбула не говорят, что делать с этим фактом: если #675 не сольётся раньше или вместе с кандидатом beta.5, тот же самый exact-SHA performance_smoke может остаться красным на spaceSwitchMs даже после идеального выполнения AC1–AC4 этой задачи — то есть заявленная цель issue («кандидат стабильно проходит действующие performance-бюджеты… полный Validate… проходит с первого воспроизводимого запуска») не гарантируется тем, что реально проверяет AC3.

Почему это находка, а не придирка к тексту. Разработчик и последующий код-ревьюер будут закрывать именно AC3 как написано — двумя названными метриками. Формально «AC3 выполнен» при этом возможен ровно в момент, когда release по-прежнему заблокирован тем же job'ом на другой метрике; никто не обязан заметить разрыв между «AC выполнен» и «issue решает свою же преамбулу», а этот разрыв прямо запрещён REVIEWER.md («жёлтый вердикт допустим и при выполненных AC, если изменение не решает заявленный сценарий»).

Чем закрывается в скоупе задачи (без нового инженерного объёма). Переформулировать AC3 и/или преамбулу «Ожидаемое поведение», выбрав одно из: (a) явно ограничить обещание двумя названными метриками и добавить в «Не-скоуп»/«Влияние» прямую фразу «для полностью зелёного performance_smoke кандидата требуется также #675»; либо (b) если у автора/владельца есть основания считать spaceSwitchMs эпизодически проходящим независимо от данной задачи (шум, а не устойчивый провал) — явно это утверждать со ссылкой на замер, а не оставлять предположение читателю. Технический характер этого уточнения (не продуктовый вопрос) означает, что owner не привлекается — решение принимает автор ТЗ, я его снимаю как открытый вопрос и фиксирую сюда.

Low — фактическая неточность в «Проблема» (не блокирует)

Раздел «Проблема» утверждает: «пять мест рендера запрашивают только путь пола». Чтением кода (grep -n "_cleanFloor(" src/houseplan-card.ts) подтверждаю четыре таких места (:9059, :9229, :10459, :11011); пятый вызов (:11873) читает .area, а не только .path, и относится к другой категории потребителей (подсказка комнаты), уже перечисленной отдельно. Утверждение не входит ни в один AC и не влияет на реализуемость — рекомендую исправить число при следующей правке ТЗ, не считаю это блокером и снимаю сам.

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

• Обязательные разделы §7.1 присутствуют, в правильном порядке; первые два — продуктовые, без терминов реализации.
• AC1 и AC2 однозначны, имеют явную пару «доказательство / чем краснеет», тестовый план реализуем без изменения контракта (geom/path уже не зависят от лестниц; ленивость нужна только для area).
• AC4 и контракт поведения («в пределах относительной погрешности 1e-9») корректно фиксируют неизменность видимого числа площади.
• «Не-скоуп» корректно ограждает geometryMinusStairsSteps (независимая реализация — трогать geometryMinusStairs её не заденет) и 2.5D-рендер лестниц, с явным условием возврата в ТЗ, если остаточных издержек окажется достаточно, чтобы провалить AC3 на изометрии — хорошая гигиена против молчаливого расширения скоупа.
• UX/модель данных/i18n/touch — «нет изменений», согласовано с кодом (никаких новых полей конфигурации, никакого нового текста).
• Release-артефакты («changelog — нет») обоснованы политикой docs/STATUS.md о STABLE-теле: v1.78.0 ещё ни разу не выходил стабильным релизом (только beta.1–4), регрессия #663 нигде не публиковалась стабильно.
• Риск «устаревшая площадь при копировании объекта» реалистично описан и сейчас не имеет ни одного нарушителя (grep на spread/Object.assign результата _cleanFloor/cleanFloorForRoom — пусто), т.е. риск корректно назван, а не выдуман.
• «Принято предположительно» корректно помечает две технические развилки (геттер vs areaOf, место фильтра bbox) как свободно изменяемые — не требует ответа владельца, соответствует §7.1 (не продуктовый вопрос).
• Открытых продуктовых вопросов к владельцу нет — согласен с автором: видимое поведение не меняется, различие в AC3 (находка Medium выше) — техническая формулировка, а не продуктовый выбор.

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

• Не запускал npm test/typecheck/build — на этапе spec кода ещё нет, реализация не начата (ветка issue/669-* не создана).
• Не запускал perf-смок и не проверял поведение реального polyclip-ts на 250 лестницах — AC3 по определению доказывается на кандидате в код-ревью, не на этапе ТЗ.
• Не проверял #675 по существу (отдельная задача); использовал только факт её существования и формулировку не-скоупа для находки Medium выше.

Вердикт

Вердикт: жёлтый · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 1 → в задаче

---

Материал раунда

• Ветка: dev, коммит c9cd6cbf1f5b — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
• Дерево материала: 2c7a4c124868cf7499d7b5c725ee028d3810eb62

  git log --all --format='%H %T' | grep 2c7a4c124868
  

• Тело issue: 1500ed85bc2e116a29201cc0f77d438e9e6381195fffc70053acc9f250e1dff6
• Вердикт конвейера: yellow · High 0