diff --git a/docs/reviews/SPEC-REVIEW-510-r1.md b/docs/reviews/SPEC-REVIEW-510-r1.md new file mode 100644 index 00000000..42887a88 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-510-r1.md @@ -0,0 +1,239 @@ +# SPEC-REVIEW-510-r1 + +- **Issue:** https://github.com/Matysh/houseplan-card/issues/510 +- **ТЗ:** `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md` +- **Этап:** spec (S4-spec-review) · заход r1 · блокирующих циклов израсходовано 0/4 до этого раунда +- **Материал:** тело ТЗ на HEAD рабочей копии (`34bf81a1`), тело issue #510 и комментарий S2 (Codex, 09.09) + +## Скоуп ревью + +ТЗ описывает три поверхности протокола CI (`validate.yml`, `process.yml`, +`merge-candidate.mjs`) плюс правила хендоффа в `PROCESS.md`/`AGENTS.md`. Трек — +полный (сам автор в шапке ТЗ называет критерий, которому задача не +соответствует: «§5 "одна поверхность" не проходит» — это ровно то, что +требует AGENTS.md при выборе трека, замечаний к выбору трека нет). + +Проверял: полноту обязательных разделов §7.1, однозначность и доказуемость +AC1…AC6, и — поскольку весь предмет ТЗ это низкоуровневый контракт CI — +техническую состоятельность описанных механизмов **против фактического кода +репозитория**, а не только внутреннюю логичность текста. Продуктовых вопросов +владельцу нет: задача инфраструктурная, ни один пункт не задевает то, что +видит пользователь карточки (AC6 прямо это фиксирует и проверяем — `src/**` +действительно нигде не упомянут в §11). + +## Как проверялось + +Читал ТЗ построчно и сверял каждое фактическое утверждение о текущем +состоянии кода с самим кодом: + +- `.github/workflows/validate.yml` — вход `workflow_dispatch.inputs.full`, + группа `concurrency` (`validate-dispatch-` для dispatch, отдельная от + push), job `changed_mutants` и её текущее условие; +- `.github/workflows/process.yml` — шаги `material` (id, строка 414), `reuse` + (id, строка 429-440, условие `rebase.outputs.conflict != 'true' && + guard.outputs.stage == 'code'`), «Конфликт с dev» (445), «Зелёные гейты на + этом SHA» / `id: validated` (484-510, ровно тот текст, что дан в задании как + «Зелёного Validate на этом SHA нет»), последующие шаги с условием + `rebase.outputs.conflict != 'true' && reuse.outputs.reuse != 'true'`; +- `.github/workflows/nightly.yml` — токен и `permissions: actions: write`, + которыми там уже пользуется `gh workflow run`; +- `scripts/classify-changes.mjs` — `heavyGatesRequested`, `hasReleaseTrailer`, + `CHECK_OF_OUTPUT`, чтобы сверить предложенную `mutantsRequested` с уже + существующим паттерном для `heavy`; +- `scripts/merge-candidate.mjs` — `realOps`, `waitValidate` (текущая сигнатура + без фильтра по событию, JSON-поля `databaseId,status,conclusion,url` без + `event`), константы `VALIDATE_APPEAR_MS`/`VALIDATE_TOTAL_MS`/`MAX_ATTEMPTS`; +- `scripts/mutation-gate.mjs` — реестр уже содержит мутанты на + `.github/workflows/*.yml`, `scripts/merge-candidate.mjs`, + `scripts/review-doc-guard.mjs`, т.е. AC4 (протокольные мутанты) технически + достижим тем же механизмом, а не новой инфраструктурой; +- `test/review-doc-guard.test.mjs` — способ проверки порядка шагов в текущих + тестах: `indexOf` по подстроке имени шага в сыром YAML, ровно то, что ТЗ + обещает для нового шага `gate`; +- `docs/specs/README.md` — обязательный раздел «release-артефакты» и его + требование для задач без пользовательского поведения. + +Не проверял (не относится к этапу spec): реальный `scripts/validate-gate.mjs` +не существует, код не пишется на этом этапе — оценивался только контракт, +который ТЗ для него описывает. + +## Находки + +### Medium (в скоупе) — 1: место шага `gate` относительно `reuse` не зафиксировано и рискует сломать fast-path #499 + +**Файл:** `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md`, §5.2, пункт 1 и 3. + +ТЗ формулирует условие нового шага `gate`: + +``` +needs.guard.outputs.stage == 'code' && steps.rebase.outputs.conflict != 'true' && steps.reuse.outputs.reuse != 'true' +``` + +и одновременно говорит, что шаг идёт **«после `material`»** — а `material` +(строка 414 `process.yml`) исполняется **до** `reuse` (строка 429-440). +Шаги GitHub Actions читают вывод друг друга по фактическому порядку +объявления в файле: если `gate` буквально вставить сразу после `material` (то +есть перед `reuse`), выражение `steps.reuse.outputs.reuse` в этот момент ещё +не определено, поэтому `!= 'true'` вычисляется в `true` **независимо от +того, что реально решил `reuse`** — условие перестаёт что-либо фильтровать. + +Практическое следствие — ровно тот сценарий, который #499 был написан +устранять: возврат ревью на дереве, где вне `docs/reviews/**` ничего не +менялось (например, ребейз-переприменение зелёного вердикта после конфликта +слияния). `reuse` в этом случае обязан коротко замкнуть путь без вызова +модели — но если `gate` не видит его результата и стоит раньше, конвейер всё +равно уйдёт в 10–45-минутный dispatch Validate с мутантами на practически +той же дельте кода, которую только что признали неизменной. Это тот же класс +потерь, который стоил #437 r4 12 минут (только дороже: там был holостой +проход ревьюера, здесь — ещё и полный прогон CI). + +Описанный в §8 тест (`review-doc-guard.test.mjs`: «шаг gate стоит после +material и до установки зависимостей») эту ошибку не ловит: обе трактовки +порядка («сразу после material» и «после reuse») одинаково проходят проверку +«между material и install deps», потому что `reuse` тоже лежит в этом +диапазоне. + +**Чем закрывается в этом же ТЗ:** одно явное предложение о порядке — «`gate` +стоит после `reuse`» (а не «после `material`»), и уточнение теста: +`indexOf('- name: ...reuse...') < indexOf('- id: gate')` наравне с уже +названной проверкой. Правки самого текста хватает, отдельный issue не +заводится (Medium в скоупе, PROCESS.md §2.4). + +### Medium (в скоупе) — 2: критерий «подходящий прогон» в `validate-gate.mjs`/`merge-candidate.mjs` не проверяет, что мутанты реально запрошены и выполнены + +**Файл:** тот же ТЗ, §5.1 пункт 2 и §6. + +Алгоритм называет «подходящим» любой прогон с `event == 'workflow_dispatch'` +на нужном SHA и читает его **общий** `conclusion`. Но с этим же ТЗ вход +`mutants` получает `default: false`, то есть не любой `workflow_dispatch` на +эту ветку означает «мутанты были запрошены и `changed_mutants` реально +выполнилась»: `workflow_dispatch` с явным `full=false, mutants=false` (ручной +запуск для другой цели, например только чтобы проверить классификацию) на +том же SHA — и общий `conclusion` вполне может быть `success`, потому что +`changed_mutants` в нём просто `skipped`, а не `failed`. Алгоритм примет это +как доказательство и откроет ревью без единого запущенного мутанта — именно +то, от чего в этом же проекте написан целый раздел PROCESS.md §2.7 про +«третий столбец — чем краснеет» и разбор #423/#430, где зелёный тест без +реальной защиты дважды стоил дня. + +Симметричная проблема в `merge-candidate.mjs` §6: `waitValidate(sha, { event: +'workflow_dispatch' })` описан так же — фильтр по типу события, не по факту +выполнения `changed_mutants` с успехом. + +**Чем закрывается в этом же ТЗ:** уточнить критерий одним из двух способов — +(а) `validate-gate.mjs`/`waitValidate` дополнительно запрашивают +`gh run view --json jobs` и требуют, чтобы job `changed_mutants` в этом +прогоне была `conclusion == success` (не `skipped`), либо (б) читают входы +самого dispatch-прогона (`gh run view --json ... ` содержит `event` payload с +`inputs`, либо `workflow_dispatch` API отдаёт `display_title`/`name` — +технический выбор автора) и проверяют `full=='true' || mutants=='true'` +явно, а не по одному факту события. Оба варианта дешёвы и укладываются в уже +описанный `ops`-интерфейс; фиксируется одним уточнением текста и одной +дополнительной строкой в описанных тестах `validate-gate.test.mjs` / +`merge-candidate.test.mjs` («дispatch без мутантов на нужном SHA → не +считается»). + +### Low — 1: нет явной декларации «пользовательское поведение не меняется» и N/A по i18n/UX/модели данных + +**Файл:** тот же ТЗ, разделы отсутствуют. + +`docs/specs/README.md` требует прямо: «для чистого refactoring ТЗ должно +прямо зафиксировать отсутствие пользовательских изменений и перечислить +технические доказательства безопасного поведения», если раздел +release-артефактов не заполняется. В ТЗ #510 такой явной строки нет — AC6 +(«Перф/touch/UX не затронуты: `src/**` без изменений») закрывает это по +смыслу, но не формально: нет ни отдельной строки i18n (§7.1 требует раздел +даже когда ответ «не применимо»), ни явного `User-Visible: no`. + +Правится одной строкой в §12 или новым мини-разделом («Release-артефакты: +нет — `src/**` не меняется, `User-Visible: no`; i18n/UX/модель данных/миграция +— не применимо, инфраструктурная задача»). Не блокирует: по существу ответ +уже виден из AC6 и §11, найдено ревьюером, а не унесено молча — снимаю +находку с этой записью, если автор согласится с тем же текстом при правке; +иначе поднимаю до Medium в следующем заходе. + +## Что проверено и корректно + +- Обязательные продуктовые разделы §7.1 присутствуют и на своём месте: + сценарий (§1.1) и «что человек увидит» (§1.2) стоят первыми, отвечают на + оба вопроса именно для того персонажа, который тут есть — автора/ревьюера + как оператора CI (задача инфраструктурная, это ожидаемо и не является + подменой продуктовой рамки). +- Скоуп/не-скоуп (§2/§3) разделены чётко, не-скоуп явно исключает состав + мутантов, `heavy`-гейты и ретраи «до зелёного» — ни один пункт не пытается + расшириться на #479/#480/#499, которые ТЗ прямо называет смежными. + чтобы не путать с новым. +- Каждый AC1…AC6 указывает способ доказательства (конкретный тестовый файл + или «штатный раннер» мутантов) — ни один не оставлен без свидетеля. + «Догадка, выданная за факт» не встретилась: там, где ТЗ описывает + поведение существующего кода, я перепроверил и утверждение подтвердилось + (см. «Как проверялось») — включая менее очевидные детали вроде уже + существующей отдельной concurrency-группы для dispatch и точных имён/строк + шагов в `process.yml`. +- Откат (§9) и риски (§10.1) названы предметно, а не общими словами; риск про + токен (`HP_PROCESS_TOKEN` для нового вызова `gh workflow run` в + `process.yml`, впервые из PAT, а не `GITHUB_TOKEN`, как в `nightly.yml`) + подтверждён чтением `nightly.yml` — это не выдумка, это реальное различие + между двумя workflow, и план «проверить на первом прогоне» для чисто + инфраструктурного риска такого рода приемлем. +- AC4 (протокольные мутанты) реалистичен: реестр `scripts/mutation-gate.mjs` + уже содержит мутанты на `.github/workflows/*.yml` и `scripts/merge- + candidate.mjs`, механизм не изобретается заново. +- Совместимость (§9) честно называет цену перехода (одна лишняя мутантная + проверка на первый `S7` после внедрения) вместо того, чтобы её прятать. + +## Чего не проверял + +- Реальный код `scripts/validate-gate.mjs` — его нет, этап spec его не + предполагает; оценивался только контракт, который ТЗ ему назначает. + Соответственно не запускал `npm run typecheck`/`npm test`/`npm run build`: + на этом этапе нет диффа продукта или тестов, который они могли бы поймать + (класс B/C файлов ещё не существует). + `node scripts/check-docs.mjs` не запускал — `src/**` в ТЗ не упомянут и + не будет затронут по AC6, гейт неприменим до этапа кода. + Не проверял golden/смоки/perf/backend/инварианты модели — задача не + трогает геометрию, рендер, состояние комнат или бэкенд ни на йоту. +- Не проверял область `docs/specs/README.md` дальше цитированного правила о + release-артефактах (сама таблица со ссылкой на ТЗ уже верно добавлена в + P1 — это видно из чтения файла, отдельно не тестировал). +- Не оценивал права токена (`HP_PROCESS_TOKEN` scope `workflow`/`actions`) — + это внешняя конфигурация GitHub, не код репозитория; риск назван в ТЗ и + помечен «проверить на первом прогоне», это адекватно для находки такого + рода на этапе spec. + +## Вердикт + +High: 0 · Medium (в скоупе): 2 · Low: 1. + +Обе Medium-находки — про сам механизм, который ТЗ вводит (порядок шага +`gate` относительно `reuse`, и точность критерия «мутанты действительно +выполнены») — конкретны, воспроизводимы по тексту ТЗ и фактическому коду, и +закрываются правкой текста ТЗ в этом же документе без изменения скоупа +задачи. High нет: ни один AC не сломан целиком, оба дефекта — недосказанность +в контракте, а не ошибочное решение. + +**Вердикт: жёлтый.** Возврат в «ТЗ в работе» для уточнения §5.1/§5.2/§6 и, +по желанию автора, Low-находки; далее — повторный цикл ревью по дельте +(PROCESS.md §2.10). + +## Материал раунда + +- SHA ТЗ: `34bf81a1ed51f041fb3b5b967de84961ce566720` +- Файл: `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md` + +--- + + + +## Материал раунда + +- Ветка: `issue/510-mutants-on-candidate-and-review-gate`, коммит `34bf81a1ed51` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0ca8ddf79d45debb76c69e832504a826d78a8e7f` + ``` + git log --all --format='%H %T' | grep 0ca8ddf79d45 + ``` +- ТЗ `docs/specs/510-mutants-on-candidate-and-review-waits-validate.md`, блоб `d6e47a72256e888541f3a2bcdb085bfad81ff52d` + ``` + git log --all --find-object=d6e47a72256e888541f3a2bcdb085bfad81ff52d -- docs/specs/510-mutants-on-candidate-and-review-waits-validate.md + ``` +- Вердикт конвейера: `yellow` · High 0