diff --git a/docs/reviews/SPEC-REVIEW-473-r2.md b/docs/reviews/SPEC-REVIEW-473-r2.md new file mode 100644 index 00000000..2e245569 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-473-r2.md @@ -0,0 +1,196 @@ +# SPEC-REVIEW-473-r2 + +- Issue: https://github.com/Matysh/houseplan-card/issues/473 +- Этап: spec (ревью ТЗ, PROCESS.md §2.4) +- Заход: r2 · блокирующих циклов израсходовано 1 из 4 +- ТЗ: `docs/specs/473-iso-perf-witnesses-and-smoke.md`, SHA `ab0463de7881c64f8b42bed1f007c7ce4e434cf9` +- Вердикт: **зелёный** + +## Скоуп ревью (по дельте, PROCESS.md §2.10) + +Предыдущий раунд: `SPEC-REVIEW-473-r1.md`, вердикт жёлтый, материал — +`2cedcc6221e40ea6ead2f48ff7fb37ddc96a51ca` (= HEAD на момент r1). SHA +резолвится напрямую, ребейза не было — линейная история +`2cedcc62 → 7e4b0e69 (review doc) → ab0463de (fix)`. + +Дельта раунда: `git diff 2cedcc62..ab0463de -- docs/specs/473-iso-perf-witnesses-and-smoke.md` +— 41 добавленная / 5 удалённых строк, единственный файл. Полный диапазон +`git diff 2cedcc62..ab0463de --stat` дополнительно содержит только сам +документ r1 (`docs/reviews/SPEC-REVIEW-473-r1.md`, публикация предыдущего +раунда) — продуктового и тестового кода как не было, так и нет. + +Дельта локальна: правки только внутри ТЗ, автор не тронул код, не было +ребейза, подсистема не менялась, объём (41 строка) на порядок меньше объёма +исходного ТЗ (125 строк). Условия «разбор остаётся полным» (§2.10) не +выполнены — разбор ведётся по дельте: перепроверка трёх находок r1 плюс +верификация всех новых технических утверждений, которые дельта внесла. +AC, не задетые дельтой (AC1–AC6), наследуются без повторной проверки. + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium-1 — AC7 недоказуем предписанным способом (диффозависимый триггер не сработает на ветке самой задачи, `workflow_dispatch` у `validate.yml` нет) | AC7 переписан на ручное доказательство, как AC6: «вручную при реализации... команды и числа в issue», с явным объяснением почему автоматический путь недостижим на этой ветке. Добавлен AC8, доказывающий саму диффозависимость контрактным тестом на функцию классификации (без запуска реального Validate) | `docs/specs/473-iso-perf-witnesses-and-smoke.md:123-124` | +| Medium-2 — `--variants=60` не поддерживается `demo/benchmark_large_house.mjs` | Флаг убран из §5, формулировка заменена на факт: только `--samples=3 --warmups=1`, с явной сноской, что `--variants` принадлежит только `benchmark_glow.mjs`, и точной ссылкой на сигнатуру `demo/benchmark_large_house.mjs:17-18` | `docs/specs/473-iso-perf-witnesses-and-smoke.md:92-95` | +| Medium-3 — отсутствуют разделы §7.1 (Сценарий, Что человек увидит, UX/данные/i18n, Риски) без пометки «не применимо» | Добавлены `## 1.1. Сценарий`, `## 1.2. Что человек увидит до и после`, `## 7.1. UX, модель данных, i18n` («не применимо», с объяснением) и `## 7.2. Риски и меры` (таблица из четырёх рисков с мерами), по образцу `404-smoke-exception-guard.md` | `docs/specs/473-iso-perf-witnesses-and-smoke.md:29-44, 126-138` | + +Проверка не ограничилась фактом наличия текста — каждое техническое +утверждение новых строк сверено с деревом (см. ниже). + +## Как проверялось + +Гейты `typecheck`/`test`/`build` неприменимы и на этом раунде: дифф — один +документный файл, продуктового и тестового кода по-прежнему нет. Ссылка на +зелёный Validate (`ab0463de`, job `process-gate` и др.) в задании адресована +дешёвым гейтам будущей реализации, не этому диффу — здесь нечего было ломать. + +Построчная проверка каждого нового технического утверждения: + +- `demo/benchmark_large_house.mjs:17-22` прочитан целиком — принимает ровно + `samples`, `warmups`, `target-root`, `profile`, `output`; `--variants` там + действительно не читается. Подтверждает правку Medium-2 буквально. +- `demo/benchmark_glow.mjs:27` — `valueArg('variants')` существует только + здесь, подтверждает, что убранный флаг относился к другому скрипту. +- `package.json` — `benchmark:large-house-isometric` (`--profile=large-house-isometric-v1`) + и `benchmark:large-house-interaction` (`--profile=large-house-interaction-v1`) + существуют и совпадают с профилями AC7/§5. +- `.github/workflows/validate.yml:279-281` — job `changes` классифицирует + диапазон именно через inline-shell `has() { grep -qE "$1" ... }`, что + подтверждает предпосылку AC8 (классификация живёт в inline-shell, вынос в + отдельный скрипт или тестируемые regex-шаблоны — реальная, не выдуманная + работа) и отсутствие `workflow_dispatch` у `validate.yml`, на чём стоит + переформулированный AC7. +- `.github/workflows/validate.yml:492,720` — `timeout-minutes: 20` у + соответствующих job, подтверждает цифру «20-минутный лимit» в §7.2. +- `demo/performance/*.json` — уже существующая пара «smoke-бюджет как + урезанное подмножество полного» (`budgets-glow-smoke.json` рядом с + `budgets-large-house-glow-overlay.json`) — прецедент для новых + `budgets-isometric-smoke.json`/`budgets-interaction-smoke.json` не + выдуман, а назван по образцу. +- `test/validate-workflow.test.mjs`, `test/performance-budget.test.mjs` — + существуют (7140 и 19956 байт), формат уже содержит контрактные проверки + над текстом YAML/JSON — AC8 в этом стиле реализуем. +- Фраза «правило после #426» (§7.2, третья строка) — не находка в + строгом смысле, но проверена: собственно ревью #426 такого правила не + формулирует, однако тот же оборот с тем же смыслом («мутант на каждый + защитный контракт... для каждой новой проверки, которая обязана + краснеть, обязателен прогон отрицательного мутанта») уже используется тем + же автором в `docs/specs/162-vacuum-map-space-routing.md:126-129` — + внутренне согласованная, не изолированная ссылка, и совпадает с духом + PROCESS.md §2.7. Не блокирует. + +## Находки + +Одна остаточная, оценена как Low в скоупе Medium-3 из r1. + +### Low-1 — риск «хрупкость мутантов к рефакторингу подписи кэша» не вошёл в таблицу §7.2 + +**Файл:** `docs/specs/473-iso-perf-witnesses-and-smoke.md:131-138` + +Находка r1 (Medium-3) перечисляла два конкретных риска, отсутствовавших в +документе дословно: «шумность абсолютного порога на 3 образцах на +разделяемом раннере» и «хрупкость четырёх новых мутантов к будущим +рефакторингам подписи кэша». Новая таблица §7.2 закрывает первый (строка 2: +«Абсолютный smoke-потолок на 3 образцах шумит на общем раннере и красит +честные ветки»), но не содержит второго: ни в таблице, ни в §10 нет строки о +том, что новые свидетели §4 читают конкретные поля подписи кэша +(`shapeSignature`/`signature` — `anchor, plate, room, wallHeight, offset, +shadows, selected, unitsPerPixel`) и что переименование или реструктуризация +этих полей при будущем рефакторинге кэша способна тихо обнулить мутант, не +роняя его при этом (мутант перестаёт целиться в существующее поле, а не +перестаёт проходить). + +Ремонт дешёвый — одна строка в таблице §7.2, например: «Рефакторинг состава +подписи кэша (`shapeSignature`) переименует или уберёт поле, на которое +целится мутант, и мутант перестанет быть репрезентативным без единого +падения теста» → мера «имя поля упомянуто в тексте гарда/теста, ревью кода +любой правки `shapeSignature` обязано свериться со списком мутантов §4». + +Это Low, а не Medium: структурный пробел, из-за которого Medium-3 был +выставлен (полное отсутствие разделов), закрыт — раздел «Риски» есть, +таблица содержит по существу верные и проверенные пункты, а недостающая +строка не влияет на достижимость ни одного AC и не меняет контракт свидетелей +§4. Снимаю решением ревьюера с записью, а не возвращаю в новый цикл: цена +пятого гипотетического упоминания риска ниже цены ещё одного раунда ревью на +документ, который уже прошёл структурную проверку. Автор волен добавить +строку при реализации без нового цикла ревью ТЗ. + +## Что проверено и корректно (сверх унаследованного из r1) + +- AC7 и AC8 вместе закрывают ровно то, что не закрывал старый AC7: AC7 + доказывает актуальные потолки вручную (как AC6), AC8 доказывает сам + механизм диффозависимости без обращения к недостижимому CI-прогону этой + ветки — пробел в цепочке доказательств не остался. +- §1.1/§1.2 отвечают на оба продуктовых вопроса §7.1 PROCESS.md таким, + каким они должны быть для инфраструктурной задачи: персона — автор+ревьюер + задач отрисовки (не пользователь продукта — обосновано в r1 как + legitimate для этого класса задач, прецедент #404/#399/#422), «что видит» + сформулировано без терминов реализации («job краснеет с абсолютным + потолком... до того, как ревью началось»). +- §7.1 (UX/данные/i18n) корректно и минималистично: «не применимо» с + причиной, не пустая формальность. +- Новая нумерация разделов (1.1, 1.2, 7.1, 7.2) не создала коллизий с + существующими §2–§10 и таблицей АC — сверено построчно, разделы идут в + логическом порядке рядом с тем, что они дополняют. + +## Унаследовано из r1 (без повторной проверки) + +Документ: `docs/reviews/SPEC-REVIEW-473-r1.md`, материал: SHA +`2cedcc6221e40ea6ead2f48ff7fb37ddc96a51ca` (дерево +`41b642d61cff58dea0718d19efa6209b897a5a33`, блоб ТЗ +`03c7d24cb503bea7ac083ed9a1ffc12a77b3c0e0`). Принято без повторной сверки, +дельта не касается: + +- обоснование полного трека (две поверхности, сложность 3) — критерий §5 + PROCESS.md для лёгкого трека нарушен корректно и без изменений; +- точность построчных ссылок §4 на `iso-scene-render.ts` (783, 786, 805, + 821) и `iso-overlays.ts` (342-345), включая имена полей подписи кэша; +- реализуемость AC1 (мутанты) и AC2 (envelope-юнит) — не задеты дельтой; +- реализуемость AC3/AC5 как контрактных тестов над текстом + `validate.yml` (regex-стиль `test/validate-workflow.test.mjs`); +- реализуемость AC4 (smoke-бюджеты как урезанное подмножество полных + профилей) — прецедент `budgets-glow-smoke.json`; +- воспроизводимость AC6 на `de215578` (9870 мс против потолка 3500); +- корректность строки `docs/specs/README.md` (не задета дельтой r2); +- не-скоуп (§3) — не редактировался. + +## Чего не проверял + +- `typecheck`/`test`/`build` — дифф раунда не содержит ни продуктового, ни + тестового кода; станет предметом код-ревью. +- Фактическая укладываемость новых профилей в 20-минутный лимит + `performance_smoke` и фактическая шумность абсолютных порогов на общем + раннере — не проверяемо на этапе ТЗ, задача сама называет это + предположением/риском с планом отхода. +- Не читал 122+ существующих мутанта `scripts/mutation-gate.mjs` + построчно — не задето дельтой. +- Не проверял, действительно ли фикстура «плита у стены» в существующих + тестах даёт именно touching, а не overlap для мутанта AABB — ТЗ само + помечает это открытым риском (§7.2) с планом добавить фикстуру касания; + это станет предметом код-ревью через отрицательный прогон. + +## Материал раунда + +- SHA ветки: `ab0463de7881c64f8b42bed1f007c7ce4e434cf9` (= HEAD на момент + ревью, `git rev-parse HEAD` сверен непосредственно перед выводом) +- Дерево ТЗ: `docs/specs/473-iso-perf-witnesses-and-smoke.md` на этом SHA +- Базовый SHA дельты: `2cedcc6221e40ea6ead2f48ff7fb37ddc96a51ca` (материал r1) +- `git diff 2cedcc62..ab0463de --stat`: `docs/reviews/SPEC-REVIEW-473-r1.md` + (new, 236 lines — публикация r1), `docs/specs/473-iso-perf-witnesses-and-smoke.md` + (+41/−5) + +--- + + + +## Материал раунда + +- Ветка: `issue/473-iso-perf-witnesses`, коммит `ab0463de7881` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `d4bba359ffb6d71f9283be0c3144eba6cffa154e` + ``` + git log --all --format='%H %T' | grep d4bba359ffb6 + ``` +- ТЗ `docs/specs/473-iso-perf-witnesses-and-smoke.md`, блоб `5e2068d0dcab862fdf8964a3ec6966f1e73c2431` + ``` + git log --all --find-object=5e2068d0dcab862fdf8964a3ec6966f1e73c2431 -- docs/specs/473-iso-perf-witnesses-and-smoke.md + ```