diff --git a/docs/reviews/SPEC-REVIEW-481-r1.md b/docs/reviews/SPEC-REVIEW-481-r1.md new file mode 100644 index 00000000..d58565a2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-481-r1.md @@ -0,0 +1,166 @@ +# SPEC-REVIEW-481-r1 + +Issue: #481 «changed_mutants: журнал пойманных свидетелей вместо суда по диапазону» +Этап: ТЗ на ревью (PROCESS.md §2.4). Трек: `small` — ТЗ в теле issue, файл `docs/specs/` не создаётся. +Заход: r1. Комментариев в issue на момент разбора нет — предыдущих раундов не было. + +## Скоуп + +Предмет ревью — тело issue #481 целиком: раздел «Проблема», ТЗ («Решение»), таблица +критериев приёмки AC1–AC7, «Затронутые файлы», «Оценка». Материал не менялся между +заходами (r1), дельта не применяется. + +## Как проверялось + +- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` целиком (§1–§10.4), включая + таблицу классов файлов, шаблон лёгкого трека (§5), таксономию доказательств AC (§2.5), + правило 18. +- Сверены все issue-ссылки из текста: `gh issue view` на #475, #480, #472, #388 — все + существуют, закрыты, содержание (guardFiles из #475, timeout-хотфикс #480, база + диапазона #388, контракт полного прогона #472) соответствует тому, как их описывает + автор. +- Проверено существование технических фактов, на которые опирается ТЗ, чтением текущего + кода на `dev`: `guardFiles()` экспортирована в `scripts/mutation-gate.mjs:7160`; + `visualFingerprint` и нормализация версии — в `scripts/source-fingerprint.mjs:156`; + структура мутанта (`id`/`guard`/`patches[].file`) соответствует действующему + `MUTANT_DEFINITIONS`. Ни одно утверждение о существующем поведении не оказалось + домыслом. +- Проверено, что `docs/specs/481-*.md` не создан — верно для лёгкого трека. +- Проверена ветка `issue/481-mutation-ledger` (`git diff origin/dev...origin/issue/481-mutation-ledger`): + 3 коммита, только файлы класса B/C/D (`scripts/mutation-gate.mjs`, + `scripts/source-fingerprint.mjs`, `.github/workflows/validate.yml`, + `test/mutation-gate.test.mjs`, `test/validate-workflow.test.mjs`, `docs/TESTING.md`, + `docs/images/screenshots.json`), ни одного файла класса A. Использована как + независимая перекрёстная проверка реализуемости и полноты AC — не как предмет этого + этапа (этап — spec, не code; код по существу не оценивался). +- Сопоставлен предложенный `actions/cache/save … if: always()` с существующим соседним + паттерном success-only (`performance_smoke`, `.github/workflows/validate.yml:905-924`, + «маркер пишется последним шагом… кэш сохраняется post-шагом, то есть тоже лишь при + успехе job») — расхождение осознанное и обосновано в тексте issue. +- Проверен прецедент текстовых контрактных тестов над YAML воркфлоу + (`test/validate-workflow.test.mjs`) — подтверждает реализуемость AC5 предложенным + способом. +- Гейты (typecheck/test/build) не гонялись: на этапе spec-review материалом является + текст ТЗ, а не диапазон кода; из уже существующей реализации на ветке гейты не + запускались намеренно — их прогон и оценка принадлежат этапу код-ревью (§2.7), а не + этому. + +## Находки + +### Medium — в скоупе задачи, чинится в этом же issue + +**1. Отсутствует обязательный раздел «откат».** + +Шаблон лёгкого трека (PROCESS.md §5) требует ровно четыре части: «проблема · контракт · +AC1…ACn с доказательством · откат». В issue есть первые три, раздела об откате нет вовсе +— ни отдельным пунктом, ни фразой внутри другого раздела. + +Это не формальность: журнал — новый постоянный артефакт (кэш GitHub Actions, +переживающий прогоны), и должно быть явно сказано, что откат дёшев. Реализация на ветке +`issue/481-mutation-ledger` действительно ведёт себя безопасно (`readLedger` трактует +отсутствующий файл, битый JSON и несовпавшую `schema` одинаково — как пустой журнал, +т.е. «гонять всё», а не отказ), но это обстоятельство должно утверждать само ТЗ, а не +ревьюер, нашедший его чтением чужой реализации. + +Исправление — один абзац: «Откат: ревёрт коммита убирает флаг `--ledger` и шаги кэша из +`validate.yml`; сам журнал не требует миграции или очистки — отсутствующий, битый или +несовпавшей схемы файл читается как пустой (прежнее поведение, гонять всё)». + +**2. AC6 доказывается способом вне разрешённой таксономии и способом, запрещённым +правилом 18.** + +DoR (§2.5) ограничивает доказательство AC пятью категориями: `unit` / `backend` / +`smoke` / `golden` / «ревью кода». AC6 называет доказательством «замер при реализации, +числа в issue» — это не автотест и не «проверено чтением», а неформализованное ручное +измерение, вписываемое постфактум в комментарий. Rule 18 прямо запрещает именно эту +форму: «Фразы «проверил локально, всё работает» в процессе не существует… либо тест, +который умеет падать, либо честное «проверено чтением, не исполнением»». + +Проверено на практике, а не в теории: в ветке `issue/481-mutation-ledger` уже есть полная +реализация с юнитами на AC1–AC4 (`test/mutation-gate.test.mjs`, блоки `#481 AC1`…`#481 +AC4`) и контрактным тестом на AC5 (`test/validate-workflow.test.mjs`, `#481 AC5`). Для +AC6 нет теста вообще — ни `AC6`, ни исторические SHA `052549fa`/`f2e38e48` не встречаются +в диффе нигде, кроме текста issue. Формулировка критерия («единицы, не 131») к тому же +нефальсифицируема — не назван порог, отделяющий пройденный критерий от непройденного. + +Исправление — один из двух путей: + - (a) снять AC6 как отдельный критерий: то, что он утверждает, фактически следует из + AC1+AC2 и проверяется тем же unit-механизмом на синтетическом журнале/диапазоне без + привязки к живым SHA; сценарий #480 достаточно оставить иллюстрацией в разделе + «Проблема», где он уже есть; + - (b) переклассифицировать доказательство AC6 в «ревью кода» и явно поручить + код-ревьюеру самому прогнать реальный диапазон один раз, зафиксировав команду и + результат в документе код-ревью. + +### Low — снимаю с записью, не блокирует + +**3.** Влияние на производительность/touch не названо явно словом «нет» (DoR §2.5 +формально требует явную запись). Снимаю без правки: очевидно из природы изменения — все +затронутые файлы класса B/C, ни одного файла класса A, следовательно изменение не может +задеть ни рантайм карты, ни touch-контракт. Дописывать строку ради строки не нужно. + +## Что проверено и корректно + +- Все issue-ссылки точны и корроборируют друг друга (#475/#480/#472/#388 — сверено + `gh issue view`, содержание совпадает с описанием автора). +- Технические факты не являются домыслом: экспорт `guardFiles()`, `visualFingerprint`, + структура `MUTANT_DEFINITIONS` — всё подтверждено чтением действующего кода `dev`. +- AC1–AC5 и AC7 однозначны, проверяемы и попадают в разрешённую таксономию доказательств + (unit / контрактный тест воркфлоу / отрицательные прогоны свидетелей). Независимо + подтверждено: на ветке задачи для них уже есть реальные автотесты, умеющие различать + правильное и сломанное поведение (например, `#481 AC2` явно проверяет и совпадение, и + несовпадение, и отсутствие отпечатка в журнале). +- Раздел «Что не меняется» корректно ограничивает скоуп: полный прогон, `--check` и + `--id` журнал не читают, что исключает риск незаметного ослабления еженедельного + контракта (#472). +- `actions/cache/save … if: always()` — осознанное и обоснованное отступление от + соседнего паттерна success-only (`performance_smoke`): в тексте явно объяснено, зачем + здесь нужен именно этот вариант (сохранить частичный прогресс отменённого или + протаймаутившего шага), решение не списано бездумно с соседа. +- Трек `small` обоснован: все критерии §5 выполняются одновременно (сложность/риск ≤3, + одна поверхность — гейт мутационного тестирования, нет миграции конфига, нет нового + UX-контракта, нет влияния на perf/touch); файла `docs/specs/481-*.md` нет, как и + требуется на лёгком треке. +- Классовая принадлежность «Затронутых файлов» указана верно — всё класса B/C, ни одного + класса A; факт подтверждён содержимым уже существующих коммитов ветки задачи. + +## Чего не проверял + +- Автотесты не запускались, реализация по существу не оценивалась — это предмет + код-ревью (§2.7), не этого этапа. Чтение кода ветки `issue/481-mutation-ledger` + использовано только как перекрёстная проверка реализуемости ТЗ и достоверности его + технических утверждений (в частности — для находки 2), не как код-ревью. +- Не нашёл в репозитории файл `AUDIT-2026-09-06-v1730beta2.md`, упомянутый в разделе + «Проблема» — вероятно, локальный/owner-side артефакт вне дерева (как и ревью до + релиза 1.62). Факты, которые он подтверждает (пять отменённых прогонов подряд, + диапазон 71 файл/131 мутант), независимо корроборируются содержимым закрытых issue + #480/#388 и текущим состоянием `validate.yml` — расхождений не нашёл. +- Не оценивал реальное поведение GitHub Actions cache под гонкой `cancel-in-progress` vs + `if: always()` — это эксплуатационный риск уровня код-ревью/пост-мержа, не + спецификации. + +## Наблюдение вне находок (не блокирует, не входит в подсчёт) + +Задача не трогает ни одного файла класса A — по механическому признаку AGENTS.md формально +подошла бы под описание «инфраструктурная задача, вне флоу». Но этот абзац AGENTS.md +описывает работу, закреплённую за Claude («CI, scripts, labels, demo stands, лендинг, +дистрибуция — только Claude»), а не за Codex как автором продуктовых/тестовых задач. +Практика репозитория (#475 — тоже gate-инфраструктура, тоже `small`, прошло полный +S1…S8) подтверждает, что задачи такого рода штатно идут по обычному лёгкому треку. +Маршрутизация issue #481 через `S3-spec` → `S4-spec-review` корректна, действий не +требует; привожу это наблюдение только для прозрачности рассуждения. + +## Вердикт + +Жёлтый. High: 0, Medium: 2 (обе в скоупе задачи — отдельный issue не заводится, правятся +в этом же issue). Обе находки дёшевы по цене исправления: один абзац «откат» и +переформулировка доказательства одного AC. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Якоря снять не удалось: ветки задачи нет, материал читался по `dev`.