diff --git a/docs/reviews/SPEC-REVIEW-473-r1.md b/docs/reviews/SPEC-REVIEW-473-r1.md new file mode 100644 index 00000000..c9a2f2e8 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-473-r1.md @@ -0,0 +1,236 @@ +# SPEC-REVIEW-473-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/473 +- Этап: spec (ревью ТЗ, PROCESS.md §2.4) +- Заход: r1 · блокирующих циклов израсходовано 0 из 4 +- ТЗ: `docs/specs/473-iso-perf-witnesses-and-smoke.md`, SHA `2cedcc6221e40ea6ead2f48ff7fb37ddc96a51ca` +- Вердикт: **жёлтый** + +## Скоуп ревью + +Материал раунда — весь диапазон HEAD..HEAD (первый заход, дельты по +предыдущему раунду нет): два файла, `docs/specs/473-iso-perf-witnesses-and-smoke.md` +(новый, 125 строк) и одна строка в `docs/specs/README.md`. Продуктового и +тестового кода нет — задача ещё в статусе ТЗ. + +Задача — процессная/инфраструктурная (постфактум-ревью perf-дельты #160 плюс +диффозависимый перф-смок в Validate), класс файлов будущей реализации — только +B (`test/**`, `scripts/mutation-gate.mjs`, `.github/workflows/validate.yml`, +`demo/performance/*.json`, `PROCESS.md`). `docs/SCOPE.md` прямо не применяется: +задача не меняет ни один Core user job, а укрепляет процесс, которым продукт +разрабатывается — это соответствует прецеденту #404/#399/#422 (те же +инфраструктурные ТЗ полного трека). + +## Как проверялось + +Гейты (`typecheck`/`test`/`build`) на этом раунде неприменимы: дифф — два +документных файла, продуктового и тестового кода ещё нет. Прогонять их +незачем, и ссылка в задании на зелёный Validate (`2cedcc62`) сюда не +адресуется — там просто нечего было ломать. + +Разбор шёл построчной сверкой каждого технического утверждения ТЗ с текущим +деревом (`2cedcc62`, тот же SHA что HEAD): + +- построчные ссылки §4 (`iso-scene-render.ts:783,786,805,821`, + `iso-overlays.ts:342-345`) — прочитаны и сверены с исходником; +- существование и сигнатуры скриптов, упомянутых в §5 (`demo/benchmark_large_house.mjs`, + `demo/benchmark_glow.mjs`, `demo/performance/compare.mjs`, `evaluate.mjs`) — + прочитаны целиком в интересующих местах (`valueArg`, `--variants`, + `--absolute-only`, `minimumSamples`); +- существующие бюджеты (`budgets-glow-smoke.json`, `budgets-large-house-isometric.json` + и др.) — сверены на предмет прецедента «smoke-бюджет как урезанное + подмножество полного» и совпадения `hardMaxMs` с цифрами регрессии из + Проблемы; +- `.github/workflows/validate.yml` — job `changes`, `reuse`, `performance_smoke` + прочитаны целиком, чтобы понять, как диффозависимый триггер (§5) сочетается + с механизмом переиспользования по содержимому (`scripts/gate-reuse.mjs`) и + есть ли ручной способ форсировать путь (`workflow_dispatch` — проверено по + всем workflow, в `validate.yml` его нет); +- `test/validate-workflow.test.mjs`, `test/performance-budget.test.mjs` — + существуют, стиль контрактных тестов (regex по тексту YAML) подтверждает + реализуемость AC3/AC4/AC5 как статических проверок; +- `de215578` — коммит существует в истории, воспроизведение AC6 технически + доступно; +- `docs/specs/README.md` — формат строки таблицы сверен с соседними записями. + +## Находки + +Все три — Medium, в скоупе задачи: без High это жёлтый вердикт, правки вносятся +в тот же файл ТЗ и проходят повторный цикл (PROCESS.md §2.4, #202). Отдельный +issue не заводится. + +### Medium-1 — AC7 недоказуем предписанным способом + +**Файл:** `docs/specs/473-iso-perf-witnesses-and-smoke.md:103` + +> AC7 | На текущем `dev` оба новых профиля укладываются в потолки за 3 образца | прогон в CI этой ветки + +§5 (той же строкой 68-71) делает триггер профилей диффозависимым от +ПРОДУКТОВЫХ путей: `perf_iso` — если дифф задел `src/iso-*`, `perf_interaction` +— если дифф задел `src/live-*`, `src/render-*`, +`src/houseplan-render-lifecycle.ts`, `src/houseplan-card.ts`. Это выходы job +`changes`, которая судит **дифф ветки против последнего доказанно зелёного +предка** (`validate.yml:270-284`), а не содержимое дерева. + +Сама реализация этой задачи, согласно §6, «продуктовый код не меняет» — +дифф ветки состоит из `test/**`, `scripts/mutation-gate.mjs`, +`.github/workflows/validate.yml`, `demo/performance/*.json`. Ни один из этих +путей не матчит шаблоны `src/iso-*` / `src/live-*` / `src/render-*` / +`houseplan-card.ts`. Значит на пуше этой самой ветки `perf_iso` и +`perf_interaction` резолвятся в `false`, новые шаги смока не выполняются +вовсе, и «прогон в CI этой ветки» не производит того свидетельства, которое +AC7 требует. + +**Проверено, что обхода через ручной триггер нет**: `validate.yml` не имеет +`workflow_dispatch` (в отличие от `performance.yml`, `mutation-gate.yml`, +`docs-screenshots.yml`, у которых он есть) — форсировать классификацию +нечем. + +Способ существует (ровно как для AC6 — разовый замер `npm run +benchmark:large-house-isometric -- --samples=3 --warmups=1` + +`benchmark:compare --absolute-only --budgets=...`, команда и результат +записываются в issue), но ТЗ называет другой, автоматический и, как показано +выше, недостижимый на этой ветке. Либо AC7 меняет способ доказательства на +ручной (аналогично AC6), либо явно описывает временный форс-коммит в +продуктовый файл для проверки с последующим откатом перед мержем — сейчас в +тексте нет ни того, ни другого. + +### Medium-2 — `--variants=60` не поддерживается скриптом, на который распространён контракт + +**Файл:** `docs/specs/473-iso-perf-witnesses-and-smoke.md:74-79` + +> Профили идут с `--variants=60 --samples=3 --warmups=1 --absolute-only` и +> своими smoke-бюджетами… + +Флаг `--variants` относится к сценариям, где он читается: `demo/benchmark_glow.mjs:27` +(`requestedVariants = valueArg('variants')?.split(',')...`) — там это число +одновременных обновлений состояния (`[1, 10, 30, 60]`). Оба новых профиля, +`large-house-isometric-v1` и `large-house-interaction-v1`, идут через +`demo/benchmark_large_house.mjs` (`package.json:22,25`), который вообще не +читает `--variants` — там только `samples`, `warmups`, `target-root`, +`profile`, `output` (`demo/benchmark_large_house.mjs:17-22`). Формулировка §5 +одной строкой распространяет флаг glow-скрипта на все три профиля — фактическая +ошибка в записанном как готовый контракт наборе CLI-аргументов. + +Эффект сегодня безвреден (неизвестный `--variants=` молча игнорируется +`valueArg`), но это ровно тот класс утверждения, который ТЗ обязано не +допускать: техническая деталь подана как проверенный факт, не будучи +сверенной с кодом инструмента. Правка — убрать `--variants=60` из вызовов +`benchmark:large-house-isometric`/`-interaction`, оставить только у +`benchmark:glow`. + +### Medium-3 — четыре обязательных раздела §7.1 отсутствуют без пометки «не применимо» + +**Файл:** `docs/specs/473-iso-perf-witnesses-and-smoke.md` (весь документ, разделы 1-10) + +PROCESS.md §7.1 перечисляет обязательные разделы ТЗ: сценарий · что человек +увидит до и после · проблема · скоуп и не-скоуп · контракт поведения · UX · +модель данных и миграция · i18n · критерии приёмки · план автотестов · +риски · откат · release-артефакты. В документе есть Проблема (§1), +Скоуп/Не-скоуп (§2-3), контракт (§4-5), откат (внутри §6), критерии приёмки +(§7), release-артефакты (§8) — но полностью отсутствуют как разделы: +**Сценарий**, **Что человек увидит до и после**, **UX/модель +данных/i18n**, **Риски**. Не встречается даже фраза «не применимо» — +разделы просто не написаны. + +Прямой прецедент — `docs/specs/404-smoke-exception-guard.md` (та же +инфраструктурная категория, тот же полный трек, тот же автор): там есть +`## Сценарий` (кто и когда столкнётся — разработчик и CI, не пользователь), +`## Что человек увидит до и после` (явно «видимого поведения продукта +задача не меняет»), `## UX, модель данных, i18n` (одной строкой «не +применимо»), и отдельный `## Риски` с тремя названными рисками и +смягчениями. Здесь этот минимум не выполнен. + +Содержательно часть материала для «Рисков» в тексте есть, но рассыпана по +другим разделам без общего заголовка: рост времени job (§10, как +предположение с планом отката, а не как риск), достижимость мутации AABB +(§10). Не названы вовсе: шумность абсолютного порога на 3 образцах на +разделяемом раннере (ложный красный на чужой, легитимной правке — именно то, +против чего заведена вся задача #473, доведённая до абсурда: диффозависимый +смок может начать шуметь на легитимных изменениях и стать тем самым «гейтом +не там, где нужно», о котором вторая половина Проблемы), и хрупкость четырёх +новых мутантов к будущим рефакторингам подписи кэша. Раздел «Риски» обязателен +не как формальность, а чтобы этот перечень стал видимым до, а не после +инцидента. + +## Что проверено и корректно + +- Числа и ссылка на прогон в §1 совпадают дословно с телом issue #473. +- Все построчные ссылки §4 на `iso-scene-render.ts` (783, 786, 805, 821) и + `iso-overlays.ts` (342-345) точны на текущем дереве. +- Мутация AABB (`boundsNear` без `gap` и без равенства) описывает + реальный код: `boundsNear` использует нестрогие `<=`/`>=` с допуском `gap` + — мутация в строгие операторы без допуска валидна и достижима. +- Предметы кэширования (WeakMap по `input.wallSilhouettes`, подпись + `shapeSignature`/`signature`, гард `!previous.nearWallBefore || …`) — + описаны верно и совпадают с кодом дословно, вплоть до имён полей. +- `budgets-glow-smoke.json` / `budgets-space-glow-smoke.json` — уже + существующий прецедент «smoke-бюджет как урезанное подмножество полного», + подтверждает реализуемость §5 без нового механизма. +- `hardMaxMs: 3500` в `budgets-large-house-isometric.json` + (`firstStableRenderMs`) против измеренных 9870 мс в Проблеме — AC6 + действительно ловится с одним образцом, как заявлено. +- `de215578` существует в истории — AC6 воспроизводимо. +- `test/validate-workflow.test.mjs`, `test/performance-budget.test.mjs` + существуют и уже проверяют YAML/JSON текстовыми утверждениями — AC3-AC5 + реализуемы заявленным способом (статический контракт, не запуск CI). +- `scripts/gate-reuse.mjs`: ключ переиспользования — хеш содержимого + (`sourceFingerprint` + оснастка job), не зависит от диапазона диффа; + добавление признака набора профилей в ключ (AC5) технически сводится к + добавлению строки в `key:` кэша в `validate.yml`, без изменения самого + скрипта — реализуемо, хотя способ (какая именно строка) оставлен автору + как техническая деталь (правомерно, не продуктовый вопрос). +- Пути `src/live-*`, `src/render-*`, `houseplan-render-lifecycle.ts`, + `houseplan-card.ts`, `src/iso-*` — все существуют, шаблоны `has()` из + Проблемы issue корректны. +- Обоснование полного трека (не `small`): две поверхности (тесты + продуктового кода + workflow), перф — часть контракта задачи — корректно + дисквалифицирует лёгкий трек по критерию §5 PROCESS.md. +- `docs/specs/README.md`: строка добавлена в верном месте таблицы, формат + совпадает с соседними записями. +- Не-скоуп (§3) дословно соответствует «Не входит» issue: пороги полных + профилей и §11.4 не переоткрываются. + +## Чего не проверял + +- Не прогонялись `typecheck`/`test`/`build`/`bundle:sync` — дифф раунда не + содержит ни продуктового, ни тестового кода, гонять нечего; это станет + предметом код-ревью. +- Не проверялась фактическая укладываемость трёх новых профилей в + 20-минутный лимит `performance_smoke` (§10, «Принятые предположения») — + это требует реального прогона CI, которого на этапе ТЗ ещё нет; ТЗ само + помечает это как предположение с планом отхода («лимит поднимается»), что + корректно по формату §7.1. +- Не проверялась фактическая шумность абсолютных порогов на 3 образцах на + разделяемых GitHub-раннерах (см. Medium-3) — это открытый риск, а не + проверяемый факт на этой стадии. +- Не читались все 122+ существующих мутанта в `scripts/mutation-gate.mjs` + построчно — только формат записи и подтверждение, что регистрация новых + идёт тем же путём. +- Не проверялась точная фикстура «три комнаты, одна стена, плита у стены и + плита вдали» на предмет буквального совпадения с какой-то одной + существующей — это техническая деталь реализации, не продуктовый вопрос, + и ревьюер вправе спросить о ней при коде, если фикстура не даст нужных + условий. + +## Материал раунда + +- SHA ветки: `2cedcc6221e40ea6ead2f48ff7fb37ddc96a51ca` (= HEAD на момент ревью) +- Дерево ТЗ: `docs/specs/473-iso-perf-witnesses-and-smoke.md` на этом SHA +- `git show 2cedcc62 --stat`: `docs/specs/473-iso-perf-witnesses-and-smoke.md` (new, 125 lines), `docs/specs/README.md` (+1 line) + +--- + + + +## Материал раунда + +- Ветка: `issue/473-iso-perf-witnesses`, коммит `2cedcc6221e4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `41b642d61cff58dea0718d19efa6209b897a5a33` + ``` + git log --all --format='%H %T' | grep 41b642d61cff + ``` +- ТЗ `docs/specs/473-iso-perf-witnesses-and-smoke.md`, блоб `03c7d24cb503bea7ac083ed9a1ffc12a77b3c0e0` + ``` + git log --all --find-object=03c7d24cb503bea7ac083ed9a1ffc12a77b3c0e0 -- docs/specs/473-iso-perf-witnesses-and-smoke.md + ```