docs: review document for #510

Issue: #510
User-Visible: no

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-<ref>` для 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 <id> --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`
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `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