From 8b775247a705cbb6e2319122d04dac129bc83e81 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 27 Sep 2026 06:28:44 +0000 Subject: [PATCH] docs: review document for #669 Issue: #669 User-Visible: no --- docs/reviews/INDEX.md | 3 +- docs/reviews/SPEC-REVIEW-669-r1.md | 193 +++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 docs/reviews/SPEC-REVIEW-669-r1.md diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index ae39ce38..7c4c9dfb 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,10 +1,11 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1093, issue: 388. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1094, issue: 389. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| | #671 | [CODE-REVIEW-671-r1.md](CODE-REVIEW-671-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | +| #669 | [SPEC-REVIEW-669-r1.md](SPEC-REVIEW-669-r1.md) | spec · r1 | 🟡 жёлтый | 0 | 1 | AC3 не гарантирует заявленную цель issue; фактическая неточность в «Проблема» (не блокирует) | — | | #667 | [CODE-REVIEW-667-r1.md](CODE-REVIEW-667-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #666 | [CODE-REVIEW-666-r1.md](CODE-REVIEW-666-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #665 | [SPEC-REVIEW-665-r1.md](SPEC-REVIEW-665-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | заявленная правка docs/ISOMETRIC.md | `docs/ISOMETRIC.md` `docs/adr/160-isometric-stage3-overlays.md` `check-docs.mjs` `ISOMETRIC.md` | diff --git a/docs/reviews/SPEC-REVIEW-669-r1.md b/docs/reviews/SPEC-REVIEW-669-r1.md new file mode 100644 index 00000000..a9db723f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-669-r1.md @@ -0,0 +1,193 @@ +# 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