diff --git a/docs/reviews/CODE-REVIEW-636-r1.md b/docs/reviews/CODE-REVIEW-636-r1.md new file mode 100644 index 00000000..ffb0b367 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-636-r1.md @@ -0,0 +1,205 @@ +# CODE-REVIEW-636-r1 + +## Скоуп + +Issue #636 (инфраструктура, класс B, ускоренный вход по §1 — сразу `S7`, без +ТЗ/спека). Единственный коммит `351fef43d65ffbc6eeceda7e744bfe142db2236d` +поверх `dev`@`e29dfdeb`. Ни одного файла класса A — только `scripts/**`, +`test/**`, `.github/workflows/**`, `PROCESS.md`, `AGENTS.md`. + +Задача: стадия `prepare` конвейера ревью (`process.yml`) синхронно ждала +Validate с мутантами на материале до 45 минут (в среднем 28,1–28,7 мин на +раунд при 10–12 минутах работы модели), занимая раннер вхолостую. Правка +заменяет ожидание внутри job на событийное продолжение: `validate-gate.mjs +--no-wait` диспатчит прогон, убеждается, что тот встал на материал, и выходит +с кодом 2 (`pending`), не дожидаясь завершения; `process-resume.yml` +(`workflow_run: completed` на `Проверка (CI)`) переставляет `S7-code-review`, +когда раунд действительно ждал именно этот прогон; `process-reconcile.mjs` +служит страховкой на потерянное событие. + +AC из тела issue: +- AC1. Ни одна job конвейера не ждёт другой workflow дольше 3 мин (тест на + отсутствие цикла ожидания/`gh run watch` в `process.yml`). +- AC2. Время от `S7` до начала работы модели не растёт (замер на 5 задачах + до/после). +- AC3. Раунд, чей Validate завершился во время простоя reconcile, стартует не + позднее следующего тика reconcile. + +Заход r1 — первый цикл, разбор полный. + +## Как проверялось + +Материал — рабочая копия на `351fef43d65ffbc6eeceda7e744bfe142db2236d`, без +`git fetch`/`checkout`. Дешёвые гейты (`tsc`, `npm test`, `npm run build` со +сверкой бандла) не перегонялись: Validate на этом SHA зелёный — +https://github.com/Matysh/houseplan-card/actions/runs/35824683286 — и по +условиям задачи это покрывает и мутационный гейт (шесть mutant-jobs на +ревью). `check-docs.mjs` не требовался: diff не трогает `src/**`. Инварианты +модели, smoke, golden, `pytest tests_backend`, перф-профили — не требовались: +diff не трогает геометрию, рендер, Python или чувствительные к перфу пути. + +Дополнительно, сверх минимума, сам прогнал точечно: +- `node --test test/process-resume.test.mjs test/validate-gate.test.mjs + test/process-reconcile.test.mjs test/mutation-gate.test.mjs + test/review-doc-guard.test.mjs` → 158/158 pass — подтверждает, что новые и + задетые тесты зелёные на этом материале, а не только по слову автора. +- Вручную применил все 4 новых мутанта из `scripts/mutation-registry.mjs` + (`gate-no-wait-still-sleeps`, `resume-wakes-round-without-marker`, + `resume-ignores-active-run`, `reconcile-wakes-pending-while-validate-active`) + по одному и прогнал заявленный `guard`-тест на каждом патче: все четыре + красят соответствующий тест (1 упавший тест на патч), рабочая копия + восстановлена (`git status --porcelain` пуст после). Мутационный гейт не + просто существует — он действительно ловит снятую защиту. +- Прочитал полный текст `scripts/validate-gate.mjs`, `scripts/process-resume.mjs`, + `scripts/process-reconcile.mjs` (диффовые и смежные функции), новый + `.github/workflows/process-resume.yml`, изменённые куски `process.yml` и + `validate.yml` — целиком, не только по хankам диффа. + +## AC · чем доказан · чем краснеет + +| AC | Доказательство | Проверка ревьюера | +|---|---|---| +| AC1 | `test/validate-gate.test.mjs` — 4 новых теста `#636`: идущий dispatch → `pending` без единого `sleep` (`ops.now()===0`); прогон, который ещё не появился, — гейт диспатчит и ждёт появления (≤3 мин по `VALIDATE_APPEAR_MS`), не завершения; завершённый зелёный/красный — как раньше, сразу. `test/review-doc-guard.test.mjs` проверяет текст `process.yml`/`process-resume.yml` (условие `proceed == 'false'`, `--no-wait`, контракт маркера) | Прогнал тесты (158/158), убил мутант `gate-no-wait-still-sleeps` (1/1). Прочитал `validateGate`: ветка `if (!wait)` стоит строго после проверки `run.status === 'completed'`, поэтому завершённый прогон и с `--no-wait` возвращается как раньше — подтверждено тестом «зелёный/красный принимается сразу» | +| AC2 | Замер на 5 задачах после слияния — по конструкции задачи это возможно только постфактум, автор явно пометил это как «Чего не прогонял: живой конвейер (только на main после слияния)» | Не переисполнимо на этапе ревью — измерение требует живого прогона в проде после мержа и зеркалирования `process-resume.yml`/`process.yml` в `main`. **Проверено чтением, не исполнением**: событие `workflow_run` физически приходит в момент завершения Validate (тот же момент, когда раньше просыпался poll, ≤20 c реакции), поэтому по конструкции задержка не растёт. Формальное закрытие AC2 остаётся за первым живым раундом после мержа — это ожидаемо для AC такого типа, не находка | +| AC3 | `test/process-reconcile.test.mjs` — «#636 pending marker»: `pendingValidate==='active'` → `wait`; `completed`/`missing` → `retry` (следующий тик reconcile переставляет `S7` сам); без маркера — прежний `escalate` | Прогнал тест, убил мутант `reconcile-wakes-pending-while-validate-active` (1/1). Прочитал `decideReconciliation`: новая ветка стоит перед general `run.conclusion === 'success' → escalate`, поэтому успешный прогон с маркером не путается со старым «успех без вердикта» | + +## Находки + +Блокирующих (High) и находок Medium в скоупе — нет. + +Одно наблюдение, не блокирующее и не требующее отдельного issue: + +- **AC1 в общей формулировке шире, чем то, что фактически исправлено.** + `integrate`-job того же `process.yml` (через `scripts/merge-candidate.mjs`, + функция `waitValidate`) по-прежнему синхронно поллит `gh run list` до 45 + минут, когда за время ревью `dev` успел уйти вперёд и кандидата нужно + пересобрать и перепроверить. Это тоже «job конвейера, ждущая другой + workflow дольше 3 мин», но раздел «Факты» issue называет только стадию + `prepare` (28,1–28,7 мин на каждый раунд) и по аналогии — `nightly.yml` и + `release-gate.mjs`; про `merge-candidate.mjs`/`integrate` там не сказано ни + слова, и сам AC1 в скобках сужает предмет проверки до текста `process.yml` + на отсутствие цикла именно там, где раньше был. Не считаю это находкой в + скоупе: путь `integrate`-ожидания условный (срабатывает только когда `dev` + двигался во время ревью, а не на каждом раунде), для него уже заведён + отдельный бюджет 55 минут (#551) заранее, под него, и диффом не тронут — + то есть не регрессия. Если владелец сочтёт нужным закрыть и этот случай — + это отдельная задача, не расширение текущего скоупа. +- Аналогично не тронуты `nightly.yml` (`gh run watch`, 3–25 мин) и + `release-gate.mjs` (ожидание `performance.yml` до 60 мин), хотя раздел + «Правка» issue упоминает их как аналогичные случаи. AC1/AC2/AC3 их не + называют, тест AC1 явно ограничен `process.yml` — вне скоупа этой задачи. + +## Что проверено и корректно + +- `validateGate({ wait: false })`: ветвление корректно — сначала проверяется + `run.status === 'completed'` (зелёный/красный возвращаются немедленно + независимо от `wait`), и только для незавершённого прогона `wait: false` + выходит в `pending` без `sleep`. Появление dispatch (если прогона ещё нет) + по-прежнему ограничено `VALIDATE_APPEAR_MS` (3 мин, до двух попыток при + гонке ref/SHA, #539) — не новая ветка, поведение унаследовано. +- `process.yml`: `set +e / code=$? / set -e` корректно перехватывает код + выхода 2 до строгого режима; `case` покрывает все три исхода; шаг + «Validate идёт» и «Сохранить маркер ожидания» гейтятся именно на + `proceed == 'pending'`; шаг «Validate красный» теперь — точно на + `proceed == 'false'` (было `!= 'true'`, что раньше ошибочно включало бы и + `pending`). Все шаги, зависящие от зелёного гейта («Зелёные гейты на этом + SHA», сбор `prepared.json`, публикация артефакта модели), по-прежнему + условие `== 'true'`, поэтому `pending` их не задевает — не нужно было + трогать эти строки, и их не тронули. +- `integrate`-job: явная короткая ветка `PROCEED = "pending"` — печатает + причину, `proceed=false`, `exit 0`, не долетает до `failure()`-уведомления + владельца (это не авария, а штатный промежуточный статус). +- `process-resume.yml`: триггер `workflow_run` на точное имя workflow + (`"Проверка (CI)"`, сверено с `validate.yml`); job-level `if` фильтрует + `event == 'workflow_dispatch'` и `head_branch` `issue/*` до checkout — + нерелевантные прогоны (push, nightly на `dev`, PR) не тратят раннер вообще. + `concurrency: process-resume-` без `cancel-in-progress` сериализует + повторные события на одной ветке. Метка переставляется `HP_PROCESS_TOKEN`, + не `GITHUB_TOKEN` — иначе событие `labeled` не запустило бы `process.yml` + (тот же инвариант, что и у остального конвейера). Код берётся из `dev`, как + у `process-reconcile.yml`. +- `decideResume`: проверил все ветви руками — не-`workflow_dispatch`, + незавершённый прогон, чужой `headSha`, отсутствие `S7`, `blocked`/`review-4`, + активный процесс-прогон (в т.ч. НЕ только последний, а любой активный — это + на случай гонки со свежим раундом), отсутствие маркера, маркер на другом + SHA, устаревший/проигравший маркер (сортировка по `createdAt`+`id`, решает + только новейший прогон) — все корректно ведут к `noop`, и только полное + совпадение — к `resume`. Специально проверил сценарий «Validate дождался + сам merge-candidate во время `integrate`»: в этот момент процесс-прогон, + из которого запущен `integrate`, ещё `in_progress` → `mine.some(active)` → + `noop`; ложного резюме на постороннем dispatch того же branch не будет. +- `process-reconcile.mjs`: `pending`/`pendingValidate` гидратируются только + для `run.status === 'completed'` (не тратит вызовы API на активные прогоны); + новая ветка `run.conclusion === 'success' && run.pending` стоит раньше + общей `success → escalate`, поэтому не путает «ждём Validate» с «успех без + вердикта»; `validateStateOnMaterial` фильтрует только `workflow_dispatch` и + предпочитает `active` над `completed`, что верно (пока хоть один активен — + ждать). `loadSealedArtifact` — рефакторинг прежнего `loadPreparedArtifact` + без потери проверки чек-суммы; используется одинаково для `prepared.json` и + `pending.json`. +- Дедупликация меток: `retry` в reconcile для pending-случая проходит через + тот же `alreadyReported`/`reconciliationKey`, что и остальные ветки — + разные `reason` для `active`/`completed`/`missing` дают разные ключи, + повторный тик не долбит комментариями бесконечно на неизменном состоянии + (комментарий появляется один раз на пару issue+run+reason). +- `actions/upload-artifact@ea165f8d...# v4` — тот же закреплённый SHA, что уже + используется в трёх других местах `process.yml` и в + `process-reconcile.yml`; не новый непроверенный pin. +- `validate.yml` preflight обновлён третьим файлом (`process-resume.yml`) в + списке сверки `main`↔`dev` — без этого новый workflow-файл разошёлся бы + молча между ветками, как когда-то `process.yml`/`mutation-gate.yml` (#472). +- Трейлеры коммита: `Issue: #636`, `User-Visible: no` — верно, видимого + пользователю поведения нет; changelog не требуется и не тронут. +- `PROCESS.md`/`AGENTS.md` обновлены в том же коммите, описывают именно то + поведение, что в коде (сверил формулировки построчно с реализацией). + +## Чего не проверял + +- **AC2 не переисполним на этапе ревью** — требует замера на 5 живых задачах + после мержа и зеркалирования `process-resume.yml` в `main` (сам автор + назвал это в разделе «После слияния — владельцу»). Принято по чтению + конструкции (`workflow_run` срабатывает синхронно с завершением Validate), + формальная приёмка — задача первого живого раунда после мержа, не этого + ревью. +- Не гонял `npx tsc --noEmit` и полный `npm test`/`npm run build` отдельно — + Validate на `351fef43` зелёный, эти гейты и mutant-jobs в нём уже + исполнены; перегонять секунды не добавили бы уверенности сверх точечного + прогона (158/158) и ручной проверки 4 мутантов, который сделал сам. +- Не проверял `demo/smoke_*.mjs`, `golden:verify`, `pytest tests_backend`, + `model-invariants` — diff не трогает `src/**`, рендер, Python или + геометрию; неприменимо по составу diff. +- Не проверял живое поведение `gh run download`/`gh api` в реальном GitHub + Actions окружении (сетевые вызовы `realOps`/`processRuns`/обёртки в + `resume()`) — это тонкие обёртки без собственной логики, покрытые + fixture-тестами; живая проверка возможна только в реальном прогоне (тот же + пробел, что у AC2). + +## Материал раунда + +``` +material: 351fef43d65ffbc6eeceda7e744bfe142db2236d +``` + +## Вердикт + +**Зелёный.** Все три AC доказаны (AC1, AC3 — автотестом с проверенной +способностью падать; AC2 — чтением конструкции с честной пометкой +«формальная приёмка после мержа»). High и Medium-в-скоупе находок нет. +Единственное наблюдение (частичное покрытие общей формулировки AC1 — +`integrate`/`merge-candidate.mjs`, `nightly.yml`, `release-gate.mjs`) вне +скоупа этой задачи по тексту issue и не является регрессией — оставлено как +информация владельцу, отдельный issue не завожу. + +--- + + + +## Материал раунда + +- Ветка: `issue/636-review-wait-event`, коммит `351fef43d65f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `84cbf14af04c3438b6c5ff3e588fc64c3eb5593f` + ``` + git log --all --format='%H %T' | grep 84cbf14af04c + ``` +- Тело issue: `6b7ad1bfe1fff446e1ab2c3d089af99b61d2286903f313d2ec0f764dbb6d446e` +- Вердикт конвейера: `green` · High 0