diff --git a/docs/reviews/CODE-REVIEW-705-r1.md b/docs/reviews/CODE-REVIEW-705-r1.md new file mode 100644 index 00000000..ba5bf67c --- /dev/null +++ b/docs/reviews/CODE-REVIEW-705-r1.md @@ -0,0 +1,192 @@ +# CODE-REVIEW-705-r1 + +## Материал раунда + +- Issue: #705 · трек `show` (инфраструктура, вход сразу в `S7-code-review`) +- SHA материала: `c17c05341bc503f1cf2aa85684a23c0f6afeaeeb` (рабочая копия уже на нём) +- `git log --oneline origin/dev..HEAD`: один коммит — + `c17c0534 fix(process): a GitHub push refusal is not a stale lease (#705)` +- Ветка к `dev` не приводилась (трек `show`, #696): `dev` впереди на 10 + коммитов, слияние чистое — это подтверждено конвейером и не находка. +- Validate на этом SHA зелёный: https://github.com/Matysh/houseplan-card/actions/runs/36771211279 + (`npx tsc --noEmit`, `npm test`, `npm run build` + сверка копий бандла на + этом прогоне не перегонялись — приняты по ссылке). + +## Скоуп + +Задача чинит инцидент #700: `pushWithLease`/`deleteBranch` в +`scripts/merge-candidate.mjs` считали устаревшим lease любой push-отказ, +содержащий слово `rejected`, включая `! [remote rejected]` — отказ самого +GitHub (например, кандидату, меняющему `.github/workflows/`, от токена без +права `workflow`). Диффу соответствуют три файла кода +(`scripts/merge-candidate.mjs`, `.github/workflows/_process.yml`, +`scripts/mutation-registry.mjs`) и два тестовых (`test/merge-candidate.test.mjs`, +`test/rebase-generated.test.mjs`), плюс абзац в `PROCESS.md` §4.2. Работа +инфраструктурная (класс B), под J-строку `docs/SCOPE.md` не подпадает — +верно, инфраструктура конвейера ревью в скоуп продукта не входит и +рецензируется по `AGENTS.md`/`PROCESS.md`, а не по `docs/SCOPE.md`. + +## Как проверялось + +Прочитан целиком дифф `git diff origin/dev...HEAD` (все 6 файлов), тело +issue #705 и единственный комментарий автора («Взял и сделал»). Код разобран +построчно: `classifyPushRefusal`, `redactSecrets`, `PushRefusal`, +`decideMerge` с новым полем `s.refused`, обёртка `mergeCandidate` → +`mergeAttempts`, изменения в `realOps.pushWithLease`/`deleteBranch`, новый +CLI-режим `--push-refusal=` (`describePushRefusal`/`pushRefusalMain`), правки +`.github/workflows/_process.yml` (шаг «Привести ветку к dev» и новый шаг +возврата автору), запись в `scripts/mutation-registry.mjs`, абзац +`PROCESS.md` §4.2. + +Гейты, прогнанные лично в этом раунде: + +| Гейт | Команда | Результат | +|---|---|---| +| Целевые unit-тесты диффа | `node --test test/merge-candidate.test.mjs test/rebase-generated.test.mjs` | 52/52 зелёные | +| Регресс канона ревью | `node --test test/process-digests.test.mjs` | 5/5 зелёные | +| Реестр мутантов синтаксически цел | `node scripts/mutation-registry-check.mjs` | exit 0, без вывода | +| Отбор браузерных смоков | `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет … смоки не выбираются» — не гейт этой задачи | +| Защитный AC3 — мутант `push-refusal-kinds-glued` | вручную применил патч мутанта из реестра к `scripts/merge-candidate.mjs`, прогнал `node --test --test-name-pattern="#705 AC1" test/merge-candidate.test.mjs`, затем восстановил файл из `git diff`/резервной копии | **красный** — 2 из 3 тестов паттерна `#705 AC1` падают (`git push dev отклонён — stale:` вместо `stale (stale info):`), после восстановления файла — снова 52/52 зелёные | + +Не прогонялось и почему: `npx tsc --noEmit`, `npm test` (полный), `npm run build` ++ сверка бандла — уже зелёные на этом SHA в Validate (ссылка выше), диффу они +не нужны повторно (#343). `golden:verify` — нет метки `ci:golden` и диффа в +`src/**`/`demo/**`. `pytest tests_backend` — Python не тронут. Инварианты +модели (`npm run invariants`) — геометрия не тронута. Performance-профили — в +AC не названы. Полный `mutation-gate --check` (весь реестр) — не гейт ревью, +мутанты в разработке не гоняются (#709), ночной прогон отдельно; проверен +только точечно мутант этой задачи, как того требует защитный AC. + +## §8: число, видимое пользователю + +Диффу не принадлежит ни одна продуктовая/пользовательская величина — правка +инфраструктурная, `User-Visible: no` (см. ниже). В коде есть два совпадающих +магических литерала `1500`: новый `PUSH_STDERR_LIMIT` (обрезка stderr для +классификации отказа) и уже существовавшая до #705 `.slice(0, 1500)` в +generic-обработчике `#492` (сбой шага слияния) — они относятся к разным +путям и разным текстам, случайное совпадение значения, не общий источник +факта. Это не дублирование пользовательского числа в смысле §8 (не то же +самое значение, показанное дважды из двух вычислений), поэтому не отмечаю +находкой — но называю прямо, раз диффом введён второй такой литерал. + +## Находки + +Нет. High: 0, Medium: 0, Low: 0. + +## Что проверено и корректно + +- **AC1.** `classifyPushRefusal` (scripts/merge-candidate.mjs:102-122) + различает три исхода по тексту stderr: + - устаревший lease — `! [rejected] … (stale info|fetch first|non-fast-forward)` + и гонка lease на сервере (`cannot lock ref … but expected`, + `incorrect old value provided`) — трактуется как тот же stale; + - отказ по праву на workflow — регэксп `WORKFLOW_REFUSAL` покрывает classic + PAT, fine-grained PAT, OAuth App, GitHub App/`GITHUB_TOKEN`, прежние формы + `bot`/`integration` — проверено тестом на каждую формулировку + (`test/merge-candidate.test.mjs`, блок `WORKFLOW_REFUSALS`); + - прочий `[remote rejected]` — pre-receive/protected-branch хуки и подобное. + `redactSecrets` вырезает userinfo в URL, `ghp_/gho_/ghu_/ghs_/ghr_/github_pat_`, + заголовок Authorization и явно переданные секреты (`split/join` до regex — + защита от новых форматов токенов) — проверено чтением и тестом с + комбинацией всех типов секретов в одном тексте, включая частичное + совпадение внутри URL. stderr печатается в журнал (`log()` внутри + `refusedPush`) — раньше не печатался вовсе (сам баг #700). +- **AC2.** `decideMerge` получил `s.refused` и отображает его в + `push-refused-workflow`/`push-refused` **до** проверки `conflict` — + корректно: отказ GitHub — не «ветка изменилась» (#312) и не конфликт + ребейза. `mergeCandidate` — новая тонкая обёртка над переименованным + `mergeAttempts`: ловит `PushRefusal`, брошенный из `realOps.pushWithLease` + на **любом** из трёх мест, где он вызывается (fast-forward в `dev`, публикация + кандидата в ветку задачи, финальный push кандидата в `dev`), и вместо + падения шага комментирует issue и возвращает `S6-in-progress`, не выполняя + `deleteBranch` — веткa не помечается влитой, потому что не влита. Прочертил + все три места вызова `pushWithLease` в `mergeAttempts` (строки ~440, ~456, + ~483) — исключение из любого перехватывается одним и тем же catch. + Комментарий `push-refused-workflow` содержит дословную фразу ТЗ («кандидат + меняет workflow-файл, токен конвейера не может его опубликовать: ребейз и + push делает автор, либо владелец выдаёт право»), имя файла и ответ GitHub — + подтверждено тестами AC2 построчным `assert.match`. + Страж ребейза в `.github/workflows/_process.yml` (шаг «Привести ветку к + dev») разбирает stderr push **тем же кодом** — + `node scripts/merge-candidate.mjs --push-refusal=…` — а не своим regex; + новый шаг «Push ребейза отклонён по праву на workflow» переводит задачу в + `S6-in-progress` с комментарием и не расходует цикл ревью (код не читался). + Шаги «Зафиксировать SHA материала», «Зелёный вердикт применим без ревью» и + «Validate на материале» корректно пропускаются доп. условием + `steps.rebase.outputs.refused == ''` — проверено и юнит-тестом + (`test/rebase-generated.test.mjs`, разбор текста workflow), и + bash-исполнением реального фрагмента шага (`runStepPush`) с фейковым `git`, + отвечающим нужным stderr — воспроизводит фактическое поведение раннера, а + не только текст скрипта. +- **AC3.** Мутант `push-refusal-kinds-glued` (реестр, + `scripts/mutation-registry.mjs`) возвращает старую склеенную регулярку + `/stale info|rejected|fetch first|lease/i` **перед** веткой workflow — я + применил этот патч руками к рабочей копии, прогнал guard-тест + (`--test-name-pattern="#705 AC1"`) и получил красный результат (2 из 3 + тестов падают, ассерты видят `kind=stale` там, где ожидался `workflow`), + затем восстановил файл. Мутация действительно ловится названным guard'ом — + «умеет падать» подтверждено исполнением, не заявлением автора. +- Регресс на существующий разбор: удаление влитой ветки (`deleteBranch`) + по-прежнему возвращает `false` только на устаревшем lease; отказ GitHub при + удалении ветки (`PUSH_REFUSAL.remote`/`workflow`) теперь бросает ошибку с + текстом причины вместо молчаливого `false` — проверено тестом + (`assert.throws(...).../remote-rejected/`). +- Тест на настоящем git с `pre-receive`-хуком (без сети, `bare`-репозиторий в + tmp) подтверждает, что классификация работает не только на сконструированных + строках stderr, а на реальном ответе git/сервера, и что `dev` не трогается + при отказе (`ls-remote` после отказа = `base`). +- Трейлеры коммита: `Issue: #705`, `User-Visible: no` — верно, правка + внутренняя, оба CHANGELOG не нужны. +- `test/process-digests.test.mjs` (сверка `REVIEWER.md` с `PROCESS.md`) + зелёный — новый абзац `PROCESS.md` §4.2 не ломает сверку выжимки. + +## Риски, названные автором и подтверждённые как честные + +Автор прямо пишет: «точный текст отказа именно от `HP_PROCESS_TOKEN` не +виден — логи #700 закрыты. Тексты взяты из публичных отчётов GitHub». +Согласен, что это не устранимо в рамках этой задачи: реальный текст отказа +для конкретно этого приложения/токена нельзя воспроизвести без доступа к уже +закрытым логам. Деградация безопасна: нераспознанный текст отказа падает в +`unknown` (сбой шага, как раньше, ничего не скрывается) или в generic +`remote-rejected` (комментарий с причиной, без ложного «ветка изменилась»). +Не нахожу это блокирующим — риск честно раскрыт, а поведение при ошибке +классификации не хуже прежнего. + +Отдельно: сама эта задача меняет `.github/workflows/_process.yml`, то есть +её собственное слияние в `dev` — ровно тот случай, который она чинит. Если +владелец ещё не выдал `HP_PROCESS_TOKEN` право на workflow (пункт 1 +предложения в ТЗ, явно вынесенный за скоуп кода), слияние этого кандидата +само продемонстрирует новый путь `push-refused-workflow` вместо ложного +#312 — это ожидаемое поведение, а не дефект, и не входит в код-ревью. + +## Чего не проверял + +- Полный `npm run gate:small`/CI-раннер заново — принят по зелёному Validate + на этом SHA (ссылка выше, #343). +- Реальный, боевой отказ GitHub с токеном `HP_PROCESS_TOKEN` — недоступен + (логи #700 закрыты); классификация проверена на текстах из публичной + документации GitHub, а не на воспроизведённом инциденте. +- Полный ночной прогон реестра мутантов — не гейт ревью на `show` (#709), + прогнан точечно только мутант этой задачи. +- Golden/perf/HA pytest/invariants — диффу не соответствуют (обоснование в + таблице гейтов выше). + +## Вердикт + +Зелёный. AC1–AC3 выполнены и доказаны исполнением (unit-тесты, реальный git +с pre-receive-хуком, ручная проверка мутанта на красный/зелёный), находок +нет. + +--- + + + +## Материал раунда + +- Ветка: `issue/705-push-refusal-kinds`, коммит `c17c05341bc5` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `43220b1aa1940537b1d101e7a3e377989273a6ad` + ``` + git log --all --format='%H %T' | grep 43220b1aa194 + ``` +- Тело issue: `4e6ce37d10900c768172f1d382518d679447b0d0207891176c66cd833c344ae7` +- Вердикт конвейера: `green` · High 0