From a91a231e61815a97d14d2cd06047368e665bca80 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 11:00:46 +0000 Subject: [PATCH] docs: review document for #472 Issue: #472 User-Visible: no --- docs/reviews/SPEC-REVIEW-472-r1.md | 208 +++++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-472-r1.md diff --git a/docs/reviews/SPEC-REVIEW-472-r1.md b/docs/reviews/SPEC-REVIEW-472-r1.md new file mode 100644 index 00000000..785f2cb7 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-472-r1.md @@ -0,0 +1,208 @@ +# SPEC-REVIEW-472-r1 + +Этап: spec (PROCESS.md §2.4) · Трек: лёгкий (`small`) · Заход r1 · блокирующих циклов 0/2 +SHA на момент ревью: `615045cb1882fa36164a55d544dc6520f2f58618` (dev) +Ревьюер ≠ автор ТЗ. + +## Скоуп + +Issue #472: еженедельный полный мутационный прогон (`mutation-gate.yml`) может +падать/отменяться без адресата — нет прав на issue, нет шага на отказ, одна +concurrency-группа разрешает ручному прогону убить расписание. ТЗ в теле issue +(лёгкий трек), контракт из 6 пунктов, AC1–AC8, два мутанта на +`scripts/mutation-gate.mjs`. Продукт (карточка, фронтенд, бэкенд-интеграция) не +затронут — чистая CI/process-задача. + +## Как проверялось + +- Прочитаны: тело issue #472, оба комментария (`S2 — разбор`, `ТЗ готово`). +- Сверено с фактическим состоянием репозитория на `615045cb`: + `.github/workflows/mutation-gate.yml`, `.github/workflows/validate.yml` + (шаг `workflow_sync`), `.github/workflows/announce.yml` (образец + Telegram-шага), `scripts/mutation-gate.mjs` (реестр, `runMutant`, + `runCleanGuards`, `main()` — ветки `--shard`/`--check`/`--build-only`), + `test/mutation-gate.test.mjs` (существующее покрытие), стиль контрактных + тестов на YAML в `test/gate-reuse.test.mjs` (прецедент: строковые + проверки, без YAML-парсера — в `package.json` его и нет). +- Проверена модель прав GitHub Actions (`permissions:` на уровне job + заменяет, а не дополняет уровень workflow) против прецедентов в самом + репозитории (`validate.yml:38,40` — `actions: read` явно объявлен для job, + которой нужен список прогонов; job `smoke`/`frontend`, качающие артефакты + через `download-artifact`, не имеют собственного `permissions:` и потому + наследуют **неограниченный** дефолт `validate.yml`, где верхнего + `permissions:` нет вовсе — в отличие от `mutation-gate.yml`, где верхний + `permissions: contents: read` уже ограничивает всё, что не названо). +- docs/SCOPE.md, docs/USER-GUIDE.ru.md, канонические документы подсистем + (SUN/LIGHT/CANVAS/WALL-THICKNESS/UX-MODES/CONFIG-COMPATIBILITY/ + TOUCH-SUPPORT) — не применимы, задача не трогает продукт и видимое + пользователю поведение. Первым вопросом («какую строку Core user jobs + закрывает задача») можно пренебречь: это не продуктовая, а процессная/ + инфраструктурная задача, и §5 лёгкого трека такие явно допускает + («одна поверхность» — сам workflow и его репортёр). +- Файла в `docs/specs/` не создано — верно для `small` (§2.3). + +## Лёгкий трек: критерии §5 — все выполнены + +Подтверждаю разбор автора в S2: сложность/риск низкие (один workflow + один +новый чистый скрипт + контрактные тесты), одна поверхность, нет миграции +конфига, нет нового UX-контракта (пользователь продукта ничего не видит), нет +влияния на perf/touch. Отказа от лёгкого трека не требуется. + +## Находки + +### Medium (в скоупе задачи) — 2 + +**M1. AC3 не отличает «сбежавший мутант» от «чистый прогон уже красный» — +риск ложного/потерянного сигнала в самом отчёте, который эта задача обязана +делать надёжным.** + +- Файл: `scripts/mutation-gate.mjs`, функция `runMutant` (строка 6759) и + `runCleanGuards` (строки 6771–6793); контракт — тело issue #472, раздел + «Контракт», пункт 3, и AC3. +- Воспроизведение (по коду, не по прогону — «проверено чтением»): шаг + «Каждый тест ловит свою поломку» вызывает `node scripts/mutation-gate.mjs + --shard=N/4`. Внутри `main()` (строка 6897) **до** цикла по мутантам + всегда вызывается `runCleanGuards(selected)` — прогон каждого уникального + `guard` без мутации. Если хоть один guard уже красный без мутанта (флеки, + сломанное окружение, забытый мердж-конфликт в тесте), печатается + ``` + FAIL чистый прогон: красный без мутанта + ``` + и функция возвращает `false` раньше, чем напечатается хоть одна строка + `FAIL : тест остался зелёным на сломанном коде` — то есть эта форма + реально попадает в тот же лог, что улетит артефактом (AC2), и относится к + той же самой невыполненной задаче: «шард отказал, назови почему». + Контракт AC3 описывает парсинг как «отсортированный список id из + `FAIL`-строк всех шардов» — этого недостаточно, чтобы отличить + `FAIL чистый прогон: …` от `FAIL : …`: если репортёр наивно + берёт первый токен после `FAIL `, он получит фиктивный «id» вроде + `чистый` (или `чистый прогон`, в зависимости от реализации), которого нет + в реестре — а значит нет и `guard`-а для таблицы, и команда «что делать» + `node scripts/mutation-gate.mjs --id=чистый` в письме владельцу будет + синтаксически некорректной (реестр ответит «мутант «чистый» не объявлен»). + Если же вместо этого такую строку молча отбросить без специального + правила — реальная поломка (тест уже красный без всякого мутанта) исчезнет + из отчёта так же тихо, как исчезал изначальный сигнал, из-за которого + заведена вся задача. +- Все реальные id мутантов — строго `[a-z0-9-]+` (проверено: + `grep -c "id: '" scripts/mutation-gate.mjs` → 509 совпадений, ни один не + содержит кириллицу/пробел), так что различение технически дёшево — но ТЗ + не называет правило, и заявленные фикстуры юнита («три лога») не + гарантируют, что этот случай будет вообще замечен реализацией. +- Почему это не «додумать самому за автора»: правило разбора лога — решение + не продуктовое (агенты решают такие сами, §7.1), но оно прямо определяет, + выполняется ли AC3 и AC4 корректно, поэтому это находка ревью, а не + тихое согласие. +- Что чинит: одна фраза в контракте — например, «строки `FAIL чистый прогон: + …` не считаются сбежавшим мутантом; шард получает статус `failed` без + записи в `escaped`, само сообщение уходит в отдельное поле/строку отчёта + (broken guard)» — плюс лог-фикстура на этот случай в AC3. + +**M2. AC5 называет для job `report` только `permissions: issues: write` — +как буквально описано, job не сможет скачать артефакты шардов.** + +- Файл: тело issue #472, раздел «Контракт», пункт 4 («Job `report`»), и + таблица AC, строка AC5. +- В GitHub Actions `permissions:`, объявленный на уровне job, **заменяет** + права workflow целиком для этой job, а не дополняет их (документированное + поведение; в этом же репозитории есть подтверждающий прецедент: + `validate.yml:38-40` явно прописывает `actions: read` для job `changes`, + которой нужен список прогонов, тогда как job `smoke`/`frontend`, которые + тоже вызывают `actions/download-artifact`, никакого собственного + `permissions:` не имеют и получают безлимитный дефолт `validate.yml` — + в этом файле верхнего `permissions:` нет вовсе). У `mutation-gate.yml`, + наоборот, верхний `permissions: contents: read` уже есть (строки 31–32), + так что job `report` с одним `issues: write` получит **только** это право + и ничего сверх. +- Job `report` по контракту «скачивает артефакты всех шардов» — + `actions/download-artifact` обращается к Actions REST API, которая требует + `actions: read`; без него шаг вернёт 403 независимо от того, что репозиторий + публичный (это разграничение GitHub API, а не git-протокола). Ей также + нужен код репозитория, чтобы выполнить `node + scripts/mutation-gate-report.mjs` и прочитать реестр мутантов из + `scripts/mutation-gate.mjs` — то есть checkout, для которого обычно + указывают `contents: read` (на публичном репозитории он, возможно, + отработает и без явного права, но называть это гарантией не стоит). +- Последствие: реализация «по букве AC5» ломает job `report` на первом же + реальном отказе по расписанию — то есть именно тогда, когда его сигнал + нужен. Это новый вариант того самого «у отказа нет адресата», ради + которого заведена вся задача, просто на другом шаге. +- Что чинит: расширить AC5 — `permissions: { contents: read, actions: read, + issues: write }` (или явно объяснить, почему `contents` не нужен, если + автор проверит, что публичный checkout работает без него). + +### Low — 1 (не блокирует, на усмотрение автора) + +**L1. Неверный номер строки в разделе «Проблема».** Текст «checkout берёт +`ref: dev` (`mutation-gate.yml:33`)» — на `615045cb` эта строка (шаблон +`${{ github.event_name == 'workflow_dispatch' && inputs.ref || 'dev' }}`) +находится на строке 52, а не 33 (33–36 — блок `concurrency`). Утверждение по +существу верное (checkout действительно форсирует `dev` вне +`workflow_dispatch`), путаница только в ссылке — не влияет ни на один AC, +править по усмотрению автора. + +## Что проверено и корректно + +- Все три «конструктивные причины» из «Проблема» подтверждены построчно: + `permissions: contents: read` — строки 31–32; отсутствие шага на отказ — + в файле действительно нет `if: failure()`/уведомлений; одна + concurrency-группа `mutation-gate` с `cancel-in-progress: true` — строки + 34–36. +- AC1 (раздельные concurrency-группы по `github.event_name`) — реализуемо + контрактным тестом на строку `group:`, других триггеров у workflow нет + (только `workflow_dispatch` и `schedule`), двузначности нет. +- AC8 и пункт 5 контракта — сверено с `validate.yml`: шаг `workflow_sync` + (строки 61-77) сегодня действительно сверяет только `process.yml` через + `git show origin/main:… origin/dev:…`; расширение на второй файл тем же + diff'ом тривиально и не пересекается с M1/M2. +- AC7 (Telegram необязателен) — паттерн уже есть в `announce.yml` (тот же + `curl`, секреты через `env`, `set -euo pipefail`); контракт прямо + ссылается на этот файл, реализуемо без новых допущений. +- Формат `FAIL : тест остался зелёным на сломанном коде` в AC3/AC4 + дословно совпадает с реальной строкой в коде (`mutation-gate.mjs:6759`) — + сам факт этой конкретной строки не выдуман, выдумано (точнее, недосказано) + только то, что она не единственная форма `FAIL` в том же логе (см. M1). +- Два новых мутанта (`mutation-report-drops-missing-shard`, + `mutation-report-duplicates-escaped`) целятся именно в AC3 и укладываются + в существующий формат реестра `scripts/mutation-gate.mjs`. +- Откат — одним коммитом, без миграции данных; согласуется с «Что не + меняется» (реестр, шардирование, `--changed`, права `mutants`). +- Критерии лёгкого трека §5 — все пять выполнены одновременно, отказа от + трека не требуется (см. выше). +- Файл в `docs/specs/` не создан, соответствует правилу `small` (§2.3). + +## Чего не проверял + +- Не проверял, действительно ли `actions/checkout` на публичном репозитории + отработает без явного `contents: read` — сослался на общую практику GitHub + (github-token без прав всё ещё может анонимно клонировать публичный git, + но это не то же самое, что «гарантированно работает»), это дополнение к + M2, а не отдельная находка, автор может закрыть её явной проверкой в коде. +- Не прогонял `npx tsc --noEmit`/`npm test`/`npm run build` — Validate уже + зелёный на этом же SHA `615045cb` (упомянутый в контексте прогон), а + задача ещё в `S4-spec-review`: кода для этих гейтов пока нет, реализации + не существует. Это ревью ТЗ, а не код-ревью. +- Не запускал `node scripts/mutation-gate.mjs` живьём и не эмулировал + `--shard` с искусственно сломанным guard — вывод для M1 получен чтением + функций `runMutant`/`runCleanGuards`/`main`, а не исполнением; поведение + однозначно следует из порядка вызовов в коде (`runCleanGuards` выполняется + и может завершить `main` раньше цикла мутантов). +- Не проверял golden/смоки/инварианты модели/performance-профили — diff не + существует (ТЗ, не код), ни один из этих гейтов неприменим на этом этапе. + +## Вердикт + +Обе находки — Medium **в скоупе задачи** (AC3 и AC5 самого текущего +контракта), High нет. По правилу §2.4/§5 это жёлтый вердикт: автор правит +текст ТЗ (уточняет разбор `FAIL`-строк в контракте+AC3, расширяет +`permissions` в контракте+AC5), фикс проходит второй заход в пределах +лимита 2 циклов лёгкого трека. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Якоря снять не удалось: ветки задачи нет, материал читался по `dev`.