diff --git a/docs/reviews/CODE-REVIEW-697-r1.md b/docs/reviews/CODE-REVIEW-697-r1.md new file mode 100644 index 00000000..a481c19d --- /dev/null +++ b/docs/reviews/CODE-REVIEW-697-r1.md @@ -0,0 +1,210 @@ +# CODE-REVIEW-697-r1 + +Issue: [#697](https://github.com/Matysh/houseplan-card/issues/697) · этап: code · заход: r1 · +блокирующих циклов израсходовано 0 из 4 · трек: `ask` (поднят автором с причиной, +задача меняет два общих механизма конвейера) · маршрут: инфраструктурная задача, +`S2`/`S3` пропущены (§1) · материал: **`2a62ad5b95e02b19f339ef71e67dbf07265adcce`** +(единственный коммит ветки `issue/697-derived-on-dev` поверх `dev@c716bb0f`). + +## Скоуп + +Задача переносит два производных артефакта — отпечаток скриншотов документации +(`docs/images/screenshots.json` + кадры) и эталоны golden +(`demo/golden/baselines/**`) — с веток задач на `dev`, принимая их один раз на +бету одним коммитом бота (`beta-derived.yml`), вместо того чтобы каждая задача с +`src/**`-диффом их коммитила и разрешала конфликты. Второе следствие: трейлер +`Release:` на ветке задачи больше не включает тяжёлый набор Validate (смоки, +golden, perf) — его теперь включают только метки `ci:full`/`ci:golden`, и гейт +материала ревью (`validate-gate.mjs`) сам диспатчит `full=true`, не принимая +лёгкий push-прогон как доказательство. + +Файлов класса A (`src/**`, backend Python, манифесты, i18n) в диффе нет — +подтверждено `git diff --stat origin/dev...HEAD` (15 файлов: `.github/workflows/**`, +`scripts/**`, `test/**`, `PROCESS.md`, `docs/process/**`, `CONTRIBUTING.md`). +Это ровно определение инфраструктурной задачи из §1, поэтому пропуск `S2-analysis` +и `S3-spec` корректен — предложение и решение владельца зафиксированы в теле +issue и его первом комментарии, а не в отдельном ТЗ. + +Какую строку `docs/SCOPE.md` это обслуживает: задача не продуктовая, это +`PROCESS.md`/пайплайн, явно разрешённый класс правок (§1, класс B) — сам +`docs/SCOPE.md` о ней не говорит, и это ожидаемо. + +## Как проверялось + +Проверялась ровно дельта: единственный коммит целиком, других раундов не было +(r1), поэтому раздел «Закрытие раунда r0» не пишется. + +Прочитано построчно: +- `PROCESS.md` — весь дифф (§3 п.13, §5.1, §8, §11.4); +- `docs/process/AUTHOR.md`, `docs/process/REVIEWER.md`, `CONTRIBUTING.md` — весь дифф; +- `.github/workflows/beta-derived.yml` — целиком (новый файл, 215 строк); +- `.github/workflows/_process.yml`, `.github/workflows/validate.yml` — весь дифф; +- `scripts/classify-changes.mjs`, `scripts/process-track.mjs`, + `scripts/validate-gate.mjs`, `scripts/mutation-registry.mjs` — весь дифф плюс + окружающий контекст (`ci-proof.mjs::evaluateCiProof`, `requiredCheckIds`, + `merge-candidate.mjs`) — не изменены этим диффом, но нужны, чтобы понять, + дотягивается ли `full=true` до кандидата слияния; +- `test/beta-derived.test.mjs`, `test/classify-changes.test.mjs`, + `test/process-track.test.mjs`, `test/validate-gate.test.mjs` — весь дифф. + +Независимо от заявлений автора в issue я перепроверил Validate-прогон +[36480865713](https://github.com/Matysh/houseplan-card/actions/runs/36480865713) +через `gh run view`/`gh api`: +- прогон — `workflow_dispatch` на `issue/697-derived-on-dev`@`2a62ad5b`, `conclusion: success`; +- лог job «Классификация изменённых файлов» печатает `heavy=false`, + `mutants_requested=true` — то есть на материале был запрошен мутационный, но + не полный набор (метки `ci:full`/`ci:golden` на issue нет, диффа по `src/**` + тоже нет — ожидаемо); +- все шесть шардов `Мутанты по диффу (N/6)` — `success`; смоки/golden/perf — + `skipped` (согласовано с `heavy=false`); +- логи шардов подтверждают убийство ИМЕННО четырёх новых мутантов #697: + `bot-golden-commit-without-provenance`, `ci-golden-label-does-not-order-full-set`, + `review-gate-accepts-light-proof-for-full`, `task-branch-release-trailer-heavy-again` + — каждый напечатал `ok : заявленный тест покраснел на мутанте`. + +Это и есть таблица «AC · чем доказан · чем краснеет» для защитных AC этой +задачи: + +| Защитный AC | Чем доказан | Чем краснеет (мутант · результат) | +|---|---|---| +| Ветка задачи с `Release:` не включает тяжёлый набор повторно | `test/classify-changes.test.mjs` (`#697: на ветке задачи…`), исполнено | `task-branch-release-trailer-heavy-again` — `success` на прогоне 36480865713 | +| `ci:golden` без `ci:full` тоже заказывает полный набор на материале | `test/process-track.test.mjs` (`#697: полный набор…`), исполнено | `ci-golden-label-does-not-order-full-set` — `success` | +| Лёгкий push-прогон не засчитывается доказательством при `full=true` | `test/validate-gate.test.mjs` (`#697: ci:full/ci:golden…`), исполнено; плюс `evaluateCiProof`: `policy?.full && !proof.request?.full → stale` (`scripts/ci-proof.mjs:376`, не менялся этим диффом, переиспользован) | `review-gate-accepts-light-proof-for-full` — `success` | +| Коммит бота с изменёнными эталонами обязан нести `Release:`/`Baseline-Reviewed:`, иначе провенанс отклонит | `test/beta-derived.test.mjs` (`#697: сообщение коммита…`), исполнено через `validateCommitMessage` | `bot-golden-commit-without-provenance` — `success` | + +Пустых третьих столбцов нет — все четыре мутанта реально прогнаны на материале, +не только заявлены. + +Дополнительно прогнано вручную в этом ревью (дёшево, воспроизводимо): +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` → + «Исполняемого frontend-диффа нет… Browser-smoke этим диффом не выбираются». + Решение: смоки не нужны — диффа по `src/**` нет, выбирать нечего, а не + «пропустить проверку». +- `actionlint` (бинарь `v1.7.7`, установлен вручную в `/tmp`, в окружении не был) + на все три изменённых workflow: единственные находки — 8 preexisting + shellcheck-предупреждений (стиль/info) в `_process.yml`/`validate.yml` на + строках, которые этот дифф не трогает (проверено построчным сравнением с + `origin/dev`); `beta-derived.yml` — чисто. +- `python3 -c 'yaml.safe_load(...)'` на все три workflow — валидный YAML. +- Прочитаны (не исполнены) `scripts/ci-proof.mjs::evaluateCiProof`, + `requiredCheckIds`, `scripts/merge-candidate.mjs` — чтобы ответить, доходит + ли `ci:full`/`ci:golden` до кандидата слияния (см. находки — нет, и это не + дефект, см. ниже). + +## Что проверено и корректно + +- **Дешёвые гейты подтверждены зелёным Validate на точном SHA материала** + (ссылка выше, `conclusion: success`) — `typecheck`, `npm test`, `npm run build` + + `bundle-policy --verify` перегонять не нужно. +- **Мутанты по диффу** — обязательны на треке `ask`, прогнаны на материале + (dispatch, не push), все шесть шардов зелёные, четыре новых мутанта явно + проверены построчно в логах (см. таблицу выше). +- **Трейлеры коммита**: `Issue: #697`, `User-Visible: no` — верно: изменение не + видимо пользователю карточки, changelog не нужен. Один коммит, ветка + `issue/697-derived-on-dev`, без PR — соответствует правилу репозитория. +- **`isTaskBranch`/`heavyGatesRequested`**: порядок проверок в + `scripts/classify-changes.mjs` корректен — `pull_request`/`workflow_dispatch`/ + `schedule` решают исход до проверки ветки, `isTaskBranch` перехватывает только + обычный `push` на `issue/*`, и только там глушит `Release:`. На `dev` и вне + ветки задачи (`refName` не передан) поведение не изменилось — тест это + явно закрывает (`'без ветки — прежнее правило'`). Оба вызова + `classify-changes.mjs` в `validate.yml` (`--heavy` и `--screenshots-mode`) + получают `REF_NAME` — проверено регэкспом в тесте и вручную (`grep -c`). +- **`validate-gate.mjs`/`ci-proof.mjs` совместность**: `policy.full` — не новое + поле «для галочки»: `evaluateCiProof` действительно отклоняет прокси с + `stale`, если `proof.request.full` не совпадает с ожиданием политики + (`scripts/ci-proof.mjs:376`), и `requiredCheckIds` при `request.full` + требует зелёных `smoke`/`golden`/`performance_smoke` (строка 249, не + изменена этим диффом, но именно на неё опирается новый `--full`). Это + честное переиспользование существующего механизма (было введено под + `release`-политику), а не декоративный флаг. +- **`merge-candidate.mjs` не тронут и не обязан быть тронут этим диффом.** + Я проверил, не открывает ли это дыру для `ci:golden`-задач: слияние + сверяет `patch-id` дифф-ветки после ребейза с диффом, проверенным на ревью + (`scripts/merge-candidate.mjs:274, 282`), и при расхождении **отказывает** + и возвращает issue в `S7-code-review`, а не сливает молча. Значит любое + изменение golden-файлов, прошедшее полный Validate на материале ревью, + либо доедет до `dev` байт-в-байт (patch-id равен), либо слияние + переоткроет ревью — полный набор на кандидате слияния отдельно не нужен. + Это не новая находка, а подтверждение того, что задача корректно не + расширяла свой скоуп на файл, где менять было нечего. +- **Документация синхронна с кодом**: `PROCESS.md` §3 п.13/§5.1/§8/§11.4, + `docs/process/AUTHOR.md`, `docs/process/REVIEWER.md`, `CONTRIBUTING.md` + правлены в одном коммите с кодом и друг другу не противоречат; + `publish-prerelease.yml` (не в диффе) по-прежнему держит + `check-docs --screenshots=strict` на кандидате — заявленный в PROCESS.md + «предохранитель» существует и не сломан этим диффом. +- **`beta-derived.yml`**: без прав `contents: write` у job, пишет в `dev` + единственным `git push` без `--force` через `HP_PROCESS_TOKEN` (существующий + секрет, уже используемый в остальном конвейере); коммит golden несёт + `Release:`/`Baseline-Reviewed:` только когда golden реально изменился; + подпись коммита (`docs: accept derived artifacts on dev for …`) намеренно не + похожа на кандидата — проверено `isCandidateSubject(...) === false` + (`bundle-policy.mjs`, не менялся). Приёмка golden требует завершённого + `Validate` именно на `dev` (`path`/`branch`/`status` проверяются перед + `gh run download`) — чужой прогон или прогон на ветке задачи доказательством + не станет. +- **Одно число — один источник**: изменение не трогает ни одну пользователю + видимую величину (нет диффа в `src/**`, нет правок пользовательских текстов), + вопрос неприменим. + +## Находки + +Блокирующих (High) находок нет. Находок Medium в скоупе или вне скоупа нет. + +Low, снятые без правки (запись, не находка на вердикт): +- `beta-derived.yml` ни разу не запускался «вживую» (пишет в `dev`, а это + публичная запись, согласованная только на бету) — открыто заявлено автором + в разделе «НЕ сделано» issue-комментария. Первый реальный прогон пойдёт под + контролем релиз-менеджера перед ближайшей бетой с ручной проверкой диффа + `docs/images`/`demo/golden/baselines` до принятия — то есть у механизма есть + человеческий контроль на первом реальном использовании, а не слепое доверие + синтетическому тесту текста workflow. Отмечаю как остаточный риск first-run, + не как дефект кода. +- Опциональный пункт «привязать приёмку к хешу входов рендера, а не к + commit/tree» из тела issue не реализован — но он явно помечен + «Опционально» и решения владельца по нему не запрашивалось (только два + вопроса из «Нужно решение владельца» с ответами «да»/«бот»). Не пропуск AC. + +## Чего не проверял + +- Не исполнял `beta-derived.yml` (ни живьём, ни через `act`) — GitHub Actions + workflow с реальным `gh api`/push в `dev`; проверка построчная, чтением, не + исполнением (`step()`-парсинг совпадает с тем, что делают + `test/beta-derived.test.mjs`). +- Не прогонял `npm run golden:verify`, `python -m pytest tests_backend`, + `npm run invariants` — не применимы: диффа по визуалу/Python/геометрии нет + (подтверждено `smoke-select`, `git diff --stat`), и на issue нет меток + `ci:golden`/правок `custom_components/**/*.py`. +- Не повторял `npx tsc --noEmit`/`npm test`/`npm run build` локально — покрыты + зелёным Validate 36480865713 на точном SHA материала, что честно + засчитывается по правилам этого прогона (#343). +- Не проверял `scripts/golden-accept.mjs`/`scripts/docs-accept.mjs`/ + `scripts/validate-commit-provenance.mjs` изнутри — они не изменены этим + диффом (`git diff --stat` подтверждает), это переиспользуемые механизмы + более ранних задач (#246, #573, #657); их корректность — не предмет этого + ревью. +- Не проверял `ship-review.yml`/`release-review.yml` на предмет использования + `HP_PROCESS_TOKEN` или новых меток — они этим диффом не тронуты. + +## Вердикт + +Зелёный. Задача сузила проверяемый периметр правильно (никакого файла класса A, +инфраструктурный маршрут обоснован), тесты — исполняемые, а не только текстовые +грепы поверх прозы, и все четыре заявленных мутанта я перепроверил по логам +реального CI-прогона на точном SHA материала, а не поверил заявлению автора. +Документация синхронна с кодом в одном коммите. Блокирующих находок нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/697-derived-on-dev`, коммит `2a62ad5b95e0` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b3ce3332568fabce6d9204ffc8f0fa27cbd31e35` + ``` + git log --all --format='%H %T' | grep b3ce3332568f + ``` +- Тело issue: `8a1b08521497d715ba4b7870ef91556ec417feec89151bec0daf9733401188c2` +- Вердикт конвейера: `green` · High 0