diff --git a/docs/reviews/CODE-REVIEW-727-r1.md b/docs/reviews/CODE-REVIEW-727-r1.md new file mode 100644 index 00000000..91ef0c60 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-727-r1.md @@ -0,0 +1,196 @@ +# CODE-REVIEW-727-r1 + +Issue: #727 · этап code · трек `ask` · заход r1 · блокирующих циклов 0/4 +Материал: `git log --oneline origin/dev..HEAD` / `git diff origin/dev...HEAD`, +ровно `93be52ebe8c3f7860eeb955c5dda2a6d50bdbabe` (рабочая копия уже на нём) + +## Скоуп + +Один коммит поверх `dev` (`52dc08a0`): ночной режим пакетного ревью `ship` и +переиспользование его результата гейтом беты по патч-набору (К1–К9, AC1–AC10 +из ТЗ в теле #727). Классы изменений — B (`scripts/**`, `.github/workflows/**`, +`test/**`) и C (`PROCESS.md`, `docs/process/REVIEWER.md`); файлов класса A нет. +`User-Visible: no` — изменений в обоих CHANGELOG не требуется и нет +(проверено: `git diff --stat` по `docs/CHANGELOG*.md` пуст). + +Файлы: `scripts/ship-review.mjs` (+428/-…), `scripts/reviews-archive.mjs`, +`scripts/reviews-index.mjs`, `.github/workflows/_nightly.yml`, +`.github/workflows/_ship-review.yml`, `PROCESS.md` (§10.4, §11.7), +`docs/process/REVIEWER.md`, `test/ship-review.test.mjs`, +`test/nightly-workflow.test.mjs`, `test/process-digests.test.mjs`. + +Спец-ревью пройдено за r1→r2 (единственная Medium-находка r1 по К7/АС7 — +нереализуемость правила архивации для стабильной базы без примера в АС — +закрыта в r2, вердикт зелёный, `docs/reviews/SPEC-REVIEW-727-r2.md`). Трек +`ask` подтверждён верно: сложность/риск >3 (четыре поверхности) и публичный +контракт гейта беты меняется. + +## Как проверялось + +Прочитано построчно: `scripts/ship-review.mjs` целиком (591 строка, не только +дифф), `scripts/reviews-archive.mjs`/`reviews-index.mjs` диффы с контекстом +вызывающих функций (`compareStable`, `stableTagsThrough`, `ordered`), +`.github/workflows/_nightly.yml` и `_ship-review.yml` целиком, `PROCESS.md` +§10.4/§11.7 и `docs/process/REVIEWER.md` диффы, весь новый текст +`test/ship-review.test.mjs` (580 новых строк) и `test/nightly-workflow.test.mjs` +(137 новых строк). + +Каждый AC1–AC9 сверен построчно с кодом (оракул AC → конкретная функция/шаг → +тест, который его проверяет). AC10 — гейты, ниже. + +**Исполнено, не только прочитано:** +- `node --test test/ship-review.test.mjs test/nightly-workflow.test.mjs + test/process-digests.test.mjs test/default-branch-workflows.test.mjs` — + 29 + … тестов, все зелёные (включая тесты на настоящем bash/git во временных + репозиториях — `shipSandbox`, `tempRepo`, `runStep`). +- `node scripts/mutation-gate.mjs --check` — exit 0, 3 предупреждения (те же + три имени, что называет автор как «как на dev»: `corpus-loses-its-short-edge`, + `optimize-reports-work-it-did-not-do`, `nightly-reuse-accepts-stale-marker` — + все три про геометрический корпус/#650, не про этот дифф). Якорь + `ship-review-ignores-merge-marker` найден и зелёный, новые мутанты + `ship-review-accepts-partial-coverage`/`ship-review-accepts-high` в реестре + есть и зелёные. +- `node scripts/entry-cost.mjs --check` — зелёный. +- `node scripts/reviews-index.mjs --check` — зелёный («INDEX.md свеж»). +- `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — «исполняемого + frontend-диффа нет, browser-smoke не выбираются»: `src/**/*.ts` не тронут, + смоки объективно нечего выбирать, а не решение их пропустить. +- **Мутационная проверка вручную** (дисциплина «тест умеет падать») на двух + участках с наибольшим риском тихой порчи: + - `samePatchSet` → `return true` (покрытие никогда не станет `stale`): + упал AC3, AC4, AC5 и интеграционный AC2/AC5-тест — 4 из 19 тестов файла. + - `archivePlan`: `ordered.find(...)` → `[...ordered].reverse().find(...)` + (брать не ближайшую линию новее базы, а самую дальнюю — тестовый случай + АС7 (в), `[v1.78.0, v1.78.1, v1.79.0]` должен дать `v1.78.1`): упал ровно + тест АС7. Файл восстановлен `cp` из бэкапа после обеих проверок, + `git diff` после — пуст. + +**Не прогонял и почему:** `actionlint` — бинарь недоступен в этой песочнице; +автор заявляет «чистый по изменённым файлам», не перепроверено независимо — +риск низкий: YAML `run:`-блоки этого диффа уже проверяются `bash -n` в +исполняемых тестах (`test/ship-review.test.mjs`, +`test/nightly-workflow.test.mjs`), а структурные инварианты (поля job, порядок +шагов, `needs`/`if`/`permissions`) — текстовыми assert'ами на реальном файле. +`npx tsc --noEmit` / `npm test` (полный) / `npm run build` — не перегонял: +Validate зелёный на этом SHA (ссылка в промпте ревью), дешёвые гейты закрыты +им. Python/`pytest tests_backend` — не трогается, нет правки +`custom_components/**/*.py`. `npm run golden:verify` и `invariants` — нет +`ci:golden`, нет правки геометрии/рендера. Performance — не назван в AC. + +## Находки + +Нет High. Нет Medium в скоупе задачи. + +**Low (наблюдение, не блокирует, не правлю и не снимаю — требует решения +владельца о сроке).** ТЗ (раздел «Разбиение») инструктирует: «Новый issue E +(завести в `S1-new`, ссылка на #727): «Красная ночь: комментарий в задачи, +слитые после последней зелёной»». Поиском по заголовку/метке `S1-new` такой +issue в репозитории не найден — похоже, не заведён. Это явно вне скоупа +самого #727 (раздел «Не-скоуп»: «Красная ночь — issue E»), поэтому не влияет +ни на один AC1–AC10 и не блокирует это ревью; но без заведённой задачи +инструкция ТЗ технически не выполнена полностью, и решение, которое владелец +принял при развилке #707, рискует потеряться. Не завожу отдельным issue сам: +это не найденный попутный дефект кода (критерий §12 «вне скоупа»), а +административный шаг, порядок исполнения которого (сейчас / при следующей +правке конвейера) решает не ревьюер. + +**Low (наблюдение).** `readRangeDocs` — новая, нетривиальная функция (сортировка +по глубине коммита через `rev-list --count`, дедупликация документа с одним +именем между `candidate` и `devRef` в пользу `devRef`, отсечение чужой базы +для ночных имён по regex). Прямого unit-теста на саму функцию нет: она +проверяется только через интеграционный `shipSandbox`-тест, где сценарий +«один и тот же документ с разным содержимым одновременно в дереве кандидата и +в `origin/dev»` не встречается (там документ либо ещё не существует, либо уже +запушен в оба места с одним и тем же содержимым). Логика — простая перезапись +`Map` по порядку обхода `[candidate, devRef]`, поведение прочитано и совпадает +с докстрингом; риск низкий, повторной сессии не прошу. + +## Проверка критериев приёмки + +| AC | Что | Доказательство в коде | Тест | Вердикт | +|---|---|---|---|---| +| AC1 К1 | Патч-набор: `git patch-id --stable`, без `Release:`, без коммитов только `docs/reviews/**`, порядок не важен, cherry-pick даёт тот же id | `hasReleaseTrailer`, `countsForPatchSet`, `commitPatchIds` (явные опции диффа — `PATCH_DIFF`), `issuePatchSets` | `#727 AC1` — временный git-репозиторий, реальный `git patch-id` | доказано исполнением | +| AC2 К2 | `tag=nightly` требует `candidate`; имя документа по базе+SHA12; `prepare` отдаёт только `none`/`stale`; блок несёт `mode`/`patches` в хвосте | `shipReviewMode`, `nightlyDocPath`, `shipDocPath`, `anchorBlock`/`parseAnchorBlock` | `#727 AC2` (unit) + `#727 AC2/AC5` и `#727 AC2 _ship-review.yml` (реальный bash, реальный workflow-файл через `stepRun`) | доказано исполнением | +| AC3 К3 | Четыре статуса покрытия; «последний документ главнее»; чужая база не в счёт; документ без `patches` — по номеру | `shipCoverage` | `#727 AC3` — все переходы статусов, включая «high отменяет ранний clean» и наоборот | доказано исполнением; мутация `samePatchSet→true` ловится тестом | +| AC4 К4 | Гейт: всё `clean` без документа тега проходит; `none`/`stale`/`high` — отказ с командой (force только когда нужен) | `shipReviewProblems` | `#727 AC4` | доказано исполнением | +| AC5 К5 | Бета читает дельту; бриф называет прочитанное ночью; `force=true` — всё; пустая дельта — без модели | `planShipReview`, `renderShipBrief` | `#727 AC5` (unit) + `#727 AC2/AC5` (сквозной сценарий на реальном workflow) | доказано исполнением | +| AC6 К6 | Новая job после Validate, `if: always()`, не красит ночь, ждёт только появления прогона (18×10с) | `.github/workflows/_nightly.yml` (`ship_review` job) | `#727 AC6` (структура YAML) + два теста на реальном bash (`runStep`) — вывод SHA до ожидания, красный Validate красит именно job ожидания, dispatch/appearance таймауты — предупреждение, не красная ночь | доказано исполнением | +| AC7 К7 | `parseDocName` узнаёт ночное имя; `archivePlan`: база-бета → своя линия, стабильная база → ближайшая новее, без линии новее — `kept` | `parseDocName` (`SHIP_DOC_NAME` с опциональной `-dev-sha12` группой), `archivePlan` (новая ветка `doc.nightly && STABLE_TAG_RE`), `renderIndex` (сортировка ночь/небета, подпись «ночь после») | `#727 AC7` — все четыре примера (а)-(г) из АС дословно | доказано исполнением; мутация `find→reverse().find` ловится тестом (в) | +| AC8 К8 | Строка о High только ночью, с меткой документа, без повтора на тот же документ | `highCommentBody`, `highCommentTargets`, шаг «High ночью» в `_ship-review.yml` (`if: needs.prepare.outputs.mode == 'nightly'`) | `#727 AC8` (unit) + `#727 AC8 _ship-review.yml` (реальный bash: beta не пишет, nightly без High не пишет Medium/Low, nightly с High — только в задачу без метки) | доказано исполнением | +| AC9 К9 | Канон — PROCESS.md §11.7/§10.4, REVIEWER.md, ключевое правило в `process-digests` | диффы PROCESS.md/REVIEWER.md прочитаны целиком; `test/process-digests.test.mjs` | `node --test test/process-digests.test.mjs` зелёный; `entry-cost --check` зелёный | доказано исполнением | +| AC10 | Гейты | — | `gate:small` (заявлен автором, зелёный Validate подтверждает дешёвую часть), `mutation-gate --check`, `reviews-index --check` — перепрогнаны мной, зелёные; якорь найден | доказано исполнением (частично — моим прогоном) | + +## Что проверено и корректно + +- Патч-набор исключает ровно то, что нужно: `Release:`-коммит кандидата беты + (несущий `Issue:` всей линии) и чисто-доковые коммиты — иначе ночной набор + никогда бы не совпал с набором на кандидате беты (тест это явно + демонстрирует: один и тот же набор до и после `Release:`-коммита). +- `shipReviewProblems` корректно различает «документа тега нет вовсе» (нет + force в команде) от «документ тега есть, но не всё покрывает» (force + обязателен) — оба случая сведены к конкретным regex-проверкам в тестах. +- `archivePlan`: ветвление «база-бета» (точное совпадение, прежняя логика) и + «стабильная база» (поиск ближайшей линии строго новее, новая логика через + уже существующий `compareStable`) не пересекаются и не регрессируют старый + `deepEqual`-тест `#696` (проверено прогоном). +- `_nightly.yml`: SHA прогона Validate выводится шагом `id: validate` + **до** шага ожидания (`gh run watch --exit-status`), поэтому красный + Validate не теряет кандидата для ночного ship-ревью — ключевое свойство + АС6, прочитано и подтверждено порядком шагов и `outputs:` на уровне job. + `ship_review` job зависит только от `dispatch` (не от его успеха — `if: + always()`) и несёт `continue-on-error: true`, так что сбой диспетчеризации + ship-ревью не красит ночь — ровно то поведение, которое требует АС6 + («предупреждение, не красная ночь»). +- `_ship-review.yml`: `mode`/`patches` публикуемого блока берутся из выходов + `prepare` (детерминированный скрипт), а не из JSON-результата модели — + единственный источник числа patch-id не раздваивается между моделью и + шагом публикации (§8, «одно число — один источник»); явно проверено тестом + с «подложным» `patches` в результате модели, который публикация + игнорирует. +- Трейлеры коммита корректны: `Issue: #727`, `User-Visible: no`, + `Co-Authored-By`/`Claude-Session` — на месте. + +## Унаследовано из спец-ревью (без повторной проверки) + +Продуктовых вопросов владельцу по ТЗ не было (спец-ревью r2, зелёный, +`docs/reviews/SPEC-REVIEW-727-r2.md`, материал `a49f7095`). Код-ревью принимает +формулировки К1–К9 и границы скоупа/не-скоупа как данность и проверяет только +соответствие кода этим формулировкам — что и сделано выше. + +## Чего не проверял + +- `actionlint` — бинарь недоступен в песочнице ревью; заявление автора не + перепроверено независимо (риск низкий, см. «Как проверялось»). +- Полные `npx tsc --noEmit` / `npm test` / `npm run build` — не перегонял: + зелёный Validate на `93be52eb` (ссылка в промпте) закрывает дешёвые гейты + для этого SHA. +- Живой ночной прогон (`_nightly.yml` → `ship-review.yml -f tag=nightly` на + реальном GitHub Actions, включая `git describe` с настоящими тегами в + checkout публикации) — автор сам называет это риском и предлагает + понаблюдать первую живую ночь; контрактные тесты на реальном bash/git это + закрывают настолько, насколько возможно до прод-прогона, но не заменяют его. +- Browser-smoke, golden, performance, HA-pytest — объективно не применимы к + этому диффу (обоснование — «Как проверялось»). + +## Вердикт + +Все AC1–AC10 доказаны исполнением (не только чтением), два defensive-правила +(stale-покрытие, выбор ближайшей архивной линии) проверены мутацией вручную и +тест их ловит. High и Medium в скоупе нет. Одно Low-наблюдение (issue E из +ТЗ не заведён) — вне скоупа #727, не блокирует. + +**Зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/727-nightly-ship-review`, коммит `93be52ebe8c3` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `990dd76b955a9fc9d630dd09709eaa5d4f052e05` + ``` + git log --all --format='%H %T' | grep 990dd76b955a + ``` +- Тело issue: `8ce2942ca915bc938c8c5b4a720bb71d5a06d18c13db0692ee802f6a55662b7a` +- Вердикт конвейера: `green` · High 0