diff --git a/docs/reviews/CODE-REVIEW-551-r1.md b/docs/reviews/CODE-REVIEW-551-r1.md new file mode 100644 index 00000000..f5b91d6e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-551-r1.md @@ -0,0 +1,175 @@ +# CODE-REVIEW-551-r1 + +## Скоуп + +Issue #551 (инфраструктурная, метка `infra`, ни одного файла класса A) — разнести +единственный монолитный job `review` в `.github/workflows/process.yml` на три +независимых job с собственными бюджетами: `prepare` (deterministic gates, 55 +мин), `model_review` (работа модели, 45 мин), `integrate` (публикация и +слияние, 55 мин). Материал ревью: коммит `31ef70cee617812b5bf32964269425f2aae78faa` +(единственный коммит на ветке `issue/551-review-stage-budgets` над `origin/dev`). + +Изменённые файлы: `.github/workflows/process.yml` (класс B), +`scripts/mutation-gate.mjs` (класс B), `scripts/wait-verdict.mjs` (класс B), +`test/review-doc-guard.test.mjs` (класс B), `AGENTS.md` и `PROCESS.md` +(класс C, документирование того же коммита). Класса A нет — трек +инфраструктурный, ТЗ и ревью ТЗ не требуются (§1 PROCESS.md), обязательно +только код-ревью. + +## Приёмка (из тела issue) — разбор по каждому пункту + +1. **«Долгий gate не съедает выделенный model-time и не запускает модель на + непригодном материале»** — выполнено. Ожидание `validate-gate.mjs` (до 45 + мин) осталось в `prepare` (шаг `gate`, `.github/workflows/process.yml:561-580`); + `model_review` не содержит ни одного вызова `validate-gate.mjs` (проверено + `awk` по границам job — совпадений 0) и получает материал строго после + зелёного/пропущенного гейта (`if: needs.prepare.outputs.proceed == 'true' && + needs.prepare.outputs.reuse != 'true'`, `process.yml:735`). +2. **«Смена SHA/tree/spec/review round между стадиями обнаруживается; старый + green не разрешает новый merge»** — выполнено. `prepare` собирает + `prepared.json` (schema, run_id, run_attempt, issue, stage, cycle, branch, + material_sha/tree/specs/issue_body, validate_result/url, rebase_note, + validated_note, spec_body_*) и публикует artifact с `manifest.sha256` + (`process.yml:676-720`). `model_review` проверяет `sha256sum -c` и **побитовое + совпадение всех полей** через `jq -e` плюс отдельно `git rev-parse HEAD` / + `HEAD^{tree}` против контракта (`process.yml:774-799`). `integrate` повторяет + ту же проверку над результатом модели (`process.yml:1173-1218`). Оба места + проверены мутационными свидетелями (см. таблицу ниже). +3. **«Timeout/cancel/failure каждой стадии оставляет понятное состояние; лимиты + review cycles не расходуются на невыполненный review»** — выполнено. + Ранние отказы (конфликт с dev, красный Validate) переводят задачу в + `S6-in-progress` уже внутри `prepare`, без вызова модели — цикл не + расходуется (это поведение не менялось, только физически осталось в + отдельном job). Провал `model_review` или `prepare` ловится в `integrate` + шагом «Проверить исходы предыдущих стадий» (`process.yml:1131-1152`) и не + трогает метку; финальный шаг «Позвать владельца, если стадия упала» называет + конкретную стадию (`process.yml:1541-1563`). +4. **«Неполная или подменённая передача evidence между jobs отвергается»** — + выполнено. `integrate` сверяет **точный набор имён файлов** artifact'а + (`manifest.sha256 prepared.json review-document.md verdict.json`, не более и + не менее), контрольные суммы и полное совпадение контракта, а вердикт + отдельно проверяется на структуру (`verdict ∈ {green,yellow,red}`, `high`/`medium` + — числа, `summary` — строка). +5. **«Есть измерение durations по стадиям до/после на сопоставимом сценарии»** + — **частично, и это ожидаемо, а не находка**. У каждой из трёх стадий есть + шаг `duration` (`if: always()`), значения сведены в таблицу + `## Бюджеты стадий (#551)` в `GITHUB_STEP_SUMMARY`. «До» — сопоставимый + прогон #550 (run 34750166665: 590 с суммарно, 170 с ожидание Validate, 385 с + модель, 3 с слияние), названный автором в хендоффе. «После» физически не + может быть измерено в рамках этого ревью: `process.yml` вступает в силу + только из ветки по умолчанию (§10.4 п.2), а эта ветка ещё не влилась — то + есть сам факт объективного измерения «после» откладывается до первого + реального прогона новой версии, что и заявлено автором явно + («Активация... потребует отдельной прямой команды владельца»). Это не + пробел реализации, а неизбежное следствие того, что задача меняет сам + конвейер ревью. + +Ограничение скоупа выдержано: модель, число циклов ревью (4), правила owner +arbitration не менялись — `job guard` (где считается `cycle`) не тронут ни +одной строкой (diff начинается ровно на переименовании `review:` → `prepare:`, +до этого — идентичный текст). Общий periodic reconciler очереди не затронут. + +## Как проверялось + +Диапазон материала: `git log --oneline origin/dev..HEAD` — один коммит +(`31ef70ce`); `git diff origin/dev...HEAD` — 6 файлов,見 выше. + +| Гейт | Команда | Результат | +|---|---|---| +| Дешёвые гейты (typecheck/test/build+сверка бандла) | — | **не гонял повторно**: Validate зелёный на этом точном SHA, https://github.com/Matysh/houseplan-card/actions/runs/34751210053 (см. врезку задачи, подтверждено ссылкой) | +| `check-docs.mjs` | — | не требуется: diff не трогает `src/**` (`git diff --stat` подтверждает) | +| Browser-смоки | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет… Browser-smoke этим диффом не выбираются» — прогон смоков не требуется | +| `golden:verify` | — | не требуется: diff не меняет визуальный результат (не тот класс файлов) | +| Инварианты модели / geometry | — | не требуется: геометрия/`layout`/`marker.space` не затронуты | +| `pytest tests_backend` | — | не требуется: Python не тронут | +| Prettier (синтаксис изменённого YAML) | `npx prettier@3.9.6 --check .github/workflows/process.yml` | exit 0, «All matched files use Prettier code style!» | +| YAML парсится | `python3 -c "import yaml; yaml.safe_load(open('.github/workflows/process.yml'))"` | OK, 4 job: guard/prepare/model_review/integrate | +| Целевой юнит-тест целиком | `node --test test/review-doc-guard.test.mjs` | 57/57 pass | +| Реестр мутантов (дешёвая часть) | `node --test test/mutation-gate.test.mjs` | 51/51 pass | +| Применимость патчей трёх новых мутантов | `node scripts/mutation-gate.mjs --check --id=` (×3) | `ok` на все три | +| Прогон трёх новых мутантов целиком (патч → тест) | `node scripts/mutation-gate.mjs --id=` (×3) | «поймано 1 из 1» на каждом — см. таблицу ниже | + +Не прогонял: `npx tsc --noEmit`, `npm test`, `npm run build` + сверка трёх копий +бандла — они покрыты зелёным Validate на этом же SHA (ссылка выше, правило +§8/#343). `no-new-any.mjs` не проверял отдельно: diff не трогает `src/**` / +`.ts`, гейт применяется к TS-исходникам, не к YAML/JS-скриптам, которые здесь +менялись, и никакого `any` new-line эти файлы не вводят по построению языка +(bash/YAML). + +## Защитные AC — таблица «чем краснеет» + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| Модель получает ровно материал `prepare`, а не подвижный `dev` | `test/review-doc-guard.test.mjs`, тест `#551: gates, модель и интеграция…`, `node --test --test-name-pattern="#551"` | Мутант `review-model-checks-out-moving-dev` (`ref: dev` вместо `needs.prepare.outputs.material_sha`) — прогнан, тест красит: `поймано 1 из 1` | +| Интеграция не принимает результат модели без сверки контрольной суммы | тот же тест | Мутант `review-integration-skips-evidence-checksum` (убирает `sha256sum -c`) — прогнан, `поймано 1 из 1` | +| Интеграция не доверяет failed/cancelled/skipped результату модели при `reuse≠true` | тот же тест | Мутант `review-integration-trusts-failed-model` (`if false; then` вместо проверки `MODEL_RESULT`) — прогнан, `поймано 1 из 1` | +| Три job сериализованы по issue (общая concurrency-группа) | `test/review-doc-guard.test.mjs`, тест «конвейер: посторонняя метка не входит в concurrency» (обновлён под 3 job) | не мутировал отдельно — чистая структурная проверка регэкспом по каждому из трёх job, без внешнего протекающего эффекта; отнесена к формату, не к защите (см. правило PROCESS.md §2.7 про AC без заявленной защиты) | + +Всем трём мутантам — правки самого `.github/workflows/process.yml`, guard — +`node --test --test-name-pattern="#551" test/review-doc-guard.test.mjs`; для +YAML-мутаций собирать бандл не требуется, поэтому мутация дешёвая и прогнана +целиком, а не только на применимость. + +## Прочитано кодом (без мутации, «проверено чтением») + +- Пути отказа `prepare` (конфликт с `dev`, красный Validate) переставляют метку + внутри самого `prepare`, до `model_review`/`integrate` — прочитано, логика не + изменилась относительно старого job, физически перенесена без правок + условий (`steps.branch`, `steps.material`, `steps.gate` остались локальными + outputs того же job). +- `integrate` с `if: always() && needs.guard.outputs.stage != ''` корректно + выполняется и при провале/пропуске `prepare`/`model_review`: шаг «Проверить + исходы предыдущих стадий» сам классифицирует три исхода (провал prepare, + ранний возврат без ревью, провал model_review) и не создаёт вердикт в + необорванных случаях — прочитано построчно, логика веток проверена вручную + для всех четырёх комбинаций (`PREPARE_RESULT`×`PROCEED`×`REUSE`×`MODEL_RESULT`). +- Имена artifact'ов (`review-prepared-…`, `review-result-…`) детерминированы + `issue`+`run_id`+`run_attempt`, совпадают между upload- и download-шагами + дословно — download-artifact@v4 не читает чужие run, что исключает подмену + между параллельными прогонами разных issue/раундов. +- `reuse=true` ветка (#499) не регрессировала: `model_review` целиком + пропускается по job-level `if`, `integrate` корректно трактует + `REUSE=true` как разрешение не дожидаться `model_review.result == success` + (короткое замыкание в бинарном `&&`), путь «зелёный без вызова модели» + сохранён дословно в шаге `decide`. +- Трейлеры коммита корректны: `Issue: #551`, `User-Visible: no` (правки не + меняют пользовательское поведение продукта — только служебный CI); класса A + нет, changelog не требуется. + +## Найдено + +Находок нет — ни High, ни Medium, ни Low. + +## Не проверялось (и почему) + +- Реальные post-факто длительности новых трёх job в проде — физически + недоступны до активации `process.yml` в ветке по умолчанию (см. AC5 выше); + это ожидание конвейера, а не пропуск проверки. +- `actionlint` — инструмент недоступен в окружении; синтаксис и структура job + проверены Prettier + `python3 yaml.safe_load` + ручным разбором ссылок + `needs.*`/`steps.*` по каждому job построчно. +- Полный ночной набор (`golden`, backend, performance, все смоки) — не + запускался: diff не задевает ни один из этих классов файлов, `smoke-select.mjs` + подтверждает пустой отбор. Соразмерность по PROCESS.md §8. + +## Вердикт + +Зелёный. Все пять пунктов приёмки выполнены (AC5 — с честной оговоркой о +природе задачи, не находка). Три протестированных мутационных свидетеля ловят +именно те регрессии, которые процесс требует доказать: подмену материала, +пропуск контрольной суммы, доверие недоверенному результату модели. Скоуп не +расширен, модель/лимит циклов/owner arbitration не тронуты. + +--- + + + +## Материал раунда + +- Ветка: `issue/551-review-stage-budgets`, коммит `31ef70cee617` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `56c26f1d8695228c45c53e439447fc4cfd7a955b` + ``` + git log --all --format='%H %T' | grep 56c26f1d8695 + ``` +- Тело issue: `824f489aab52c7d76ade6634cbdb819c9c36ff9261ae2982484d5199a4bc23d0` +- Вердикт конвейера: `green` · High 0