diff --git a/docs/reviews/CODE-REVIEW-702-r1.md b/docs/reviews/CODE-REVIEW-702-r1.md new file mode 100644 index 00000000..044929bf --- /dev/null +++ b/docs/reviews/CODE-REVIEW-702-r1.md @@ -0,0 +1,197 @@ +# CODE-REVIEW-702-r1 + +Issue: #702 · Трек: show · Заход: r1 · Блокирующих циклов израсходовано: 0 из 2 +Материал ревью: `3a681e24e49038f202fc74cc3f5482e93e43977e` (ветка `issue/702-delete-merged-branches`, один коммит поверх `dev@c716bb0f`) + +## Скоуп + +Задача — гигиена веток конвейера (issue #702): 368 влитых `issue/*` веток +висели на `origin`, список веток переставал что-то значить, агент, искавший +ветку по номеру, мог взять устаревшую. Предложение из тела issue: + +1. После успешного слияния `merge-candidate.mjs` удаляет ветку задачи; + удаление — только если вершина ветки равна влитому кандидату. +2. Разовая чистка уже влитых веток — список владельцу, удаление только после + явного согласия. +3. Не трогать `codex/*`, релизные и невлитые ветки. + +Диапазон правки (`git diff origin/dev...HEAD`): + +| Файл | Класс | Что | +|---|---|---| +| `scripts/merge-candidate.mjs` | B (tooling) | `deleteBranch()` в `realOps`; вызов из `finish()` с `branchTip`, проброшенным из обеих веток слияния (fast-forward и push после Validate); текст комментария в `commentFor()` | +| `PROCESS.md` §10.4 | C (docs) | пункт в списке шагов слияния | +| `test/merge-candidate.test.mjs` | B | 4 новых теста (`fakeOps` получил `deleteBranch`/`deleteOk`) | +| `scripts/mutation-registry.mjs` | B | мутант `merged-task-branch-kept` | + +Файлов класса A нет — продуктовый код карточки не тронут, отсюда +`User-Visible: no` в трейлере коммита (корректно). Пункт 2 (разовая чистка) +в диффе не реализован кодом — по комментарию автора список подготовлен +(`branches-to-delete-702.txt`), удаление не выполнялось, ждёт согласия +владельца. Это соответствует AC: чистка — ручное действие владельца, не +автоматизация этой задачи. + +Ветка к `dev` не приводилась (трек show, #696): сливается с открытыми +задачами #697–#700 без конфликта (`git merge-tree`, со слов автора; не +перепроверялось — не относится к AC этой задачи). + +## Как проверялось + +Ревью читало код и намеренно воспроизвело граничный случай, который юнит-тест +подменяет фейком: сам механизм `git push --force-with-lease` на удаление ветки. + +- Прочитан `scripts/merge-candidate.mjs` целиком: путь `finish()`, оба места, + где `branchTip` попадает в `extra` (строка ~287 — fast-forward, ~332 — + после зелёного Validate), и путь `commentFor()`. +- Прочитан весь `test/merge-candidate.test.mjs`: `fakeOps`, все четыре новых + теста и их согласованность с `mergeCandidate()`. +- Выполнен независимый эксперимент на временном bare-репозитории (два + прогона): `git push --force-with-lease=refs/heads/X:<ожидание> origin + :refs/heads/X` — (а) при совпадении ожидания с реальной вершиной ветка + удаляется (`exit 0`); (б) при устаревшем ожидании push отклоняется с + `! [rejected] (delete) -> feature (stale info)`, ветка остаётся. Это + подтверждает, что регэксп `/stale info|rejected|fetch first|lease/i` в + `deleteBranch()` ловит именно ту ошибку, которую реально отдаёт git, а не + придуманную автором строку. +- Прогнаны узкие автотесты и гейты, относящиеся к диффу (список — в таблице + «Гейты» ниже). +- Проверены трейлеры коммита, сверка чисел (нет числа, видимого дважды — + единственное число диффа, SHA/статусы, генерируется кодом, не + дублируется руками). +- Проверено, что job `integrate` в `_process.yml`, вызывающая + `merge-candidate.mjs`, использует не GITHUB_TOKEN (у которого в этом job + только `contents: read`), а секрет `HP_PROCESS_TOKEN` через явный URL с + токеном — то есть право на push/delete не зависит от `permissions:` блока + job и не сужено этой задачей. +- Проверено, что ни один другой скрипт (`task-packet.mjs`, + `process-gate.mjs`, `process-metrics.mjs`, `wait-verdict.mjs`) не + предполагает существование ветки задачи после `S8-merged` — удаление + ничего не ломает по цепочке. + +## AC · чем доказан · чем краснеет + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| Ветка удаляется после успешного слияния (fast-forward и push) | `test/merge-candidate.test.mjs` (2 теста), исполнено (26/26) | Мутант `merged-task-branch-kept` (`if (false && merged && …)`) — гард `node --test --test-name-pattern="#702" test/merge-candidate.test.mjs`; прогнан лично: `node scripts/mutation-gate.mjs --id=merged-task-branch-kept` → «merged-task-branch-kept: заявленный тест покраснел на мутанте», «поймано 1 из 1» | +| Удаление — только если вершина ветки на origin равна вершине, которую видело слияние (lease) | Юнит-тест «сдвинутая вершина — ветка остаётся» (`deleteOk: false`); плюс независимый ручной эксперимент на настоящем git (см. «Как проверялось») — подтверждает, что `--force-with-lease` на удаление действительно защищает от гонки, а не только в фейке | Ручной эксперимент — стерев `--force-with-lease` до простого `git push origin :refs/heads/X`, оба ручных прогона поменяли бы поведение (случай (б) удалил бы ветку вместо отказа); отдельного мутанта на этот случай нет, но реальный git-эксперимент закрывает то же самое доказательство подлинным исполнением, а не фейком | +| Красный/незавершённый merge не удаляет ветку (red Validate, conflict, reject-stale, rereview, give-up) | Юнит-тест: красный Validate → `calls.filter(delete) == []`; устаревшая ветка (#312) → `calls.filter(delete) == []` | Проверено чтением: во всех этих исходах `finish()` вызывается без `branchTip` в `extra`, а `merged` вычисляется из `decision.action`, который в этих ветках никогда не `push`/`fast-forward` — структурно недостижимо, что делает отдельный мутант избыточным для этой части | +| Разовая чистка 368 влитых веток — не автоматически, только после согласия владельца | Чтение диффа: код чистки в этом коммите отсутствует; issue-комментарий автора подтверждает, что ничего не удалено | Не мутант — это отсутствие функциональности, доказывается отсутствием кода, а не тестом | + +## Находки + +Нет High. Нет Medium в скоупе. Нет Medium вне скоупа. + +**Low (снята с записью, не блокирует):** если `deleteBranch()` бросает +исключение, НЕ совпадающее с lease-регэкспом (например, реальная ошибка прав +или блокировка ветки правилом защиты) — оно ловится в `finish()` +(`catch (error) { ops.log(...) }`), и `branchDeleted` остаётся `null`. +Комментарий в issue в этом случае вообще не упоминает попытку удаления — +ни «удалена», ни «оставлена: вершина сдвинулась». Различить в +issue-комментарии «ветка осталась, потому что автор допушил коммит» +(ожидаемо, задокументировано) от «ветка осталась из-за сбоя git/прав» +(неожиданно) можно только по логу job в Actions, который не хранится +вечно и не виден из issue. Не блокирует: инвариант «сбой удаления не +отменяет слияние» соблюдён, риска для данных нет — в худшем случае +лишняя ветка молча остаётся висеть, то есть today's статус-кво. Снимаю +без правки: цена читаемости лога ниже цены нового условного пути в +и так плотной функции `finish()`. + +## Что проверено и корректно + +- `deleteBranch(ref, expected)` в `realOps` использует тот же + `pushUrl` (с токеном), что и `pushWithLease`, и тот же паттерн + `--force-with-lease=refs/heads/${ref}:${expected}` — синтаксис реально + поддерживается git для удаления (подтверждено экспериментом), не + придуман. +- Оба места вызова `finish(..., { branchTip })` передают корректную + «последнюю вершину, которую видело слияние»: `tip` (= `actual`) в + fast-forward-пути, где ветка задачи вообще не пушится, и `candidate` + (уже реально запушенный в `branch` строкой раньше, `pushWithLease(candidate, + branch, tip)`) в пути с ребейзом — совпадает с ожиданием юнит-тестов + (`cand-mat-on-dev1` и т. п.). +- Все исходы, где `merged !== true` (`conflict`, `rereview`, + `validation-red`, `validation-missing`, `give-up`, `reject-stale`), + структурно не передают `branchTip` и не вызывают удаление — проверено и + чтением, и тестами. +- `commentFor()` добавляет к тексту «слито» одну из двух опциональных + фраз строго по `ctx.branchDeleted === true/false`; при `null` — исходный + текст без изменений, обратной совместимости с существующими тестами + комментариев не сломано (`assert.match(commentFor('push', ctx), /… слито$/)` + по-прежнему проходит, т.к. `ctx.branchDeleted` не задан → `undefined`). +- Мутант `merged-task-branch-kept` синтаксически корректен (`find` совпадает + один-в-один со строкой файла), гард называет реальный тест, реестр цел + (`node --test test/mutation-gate.test.mjs` — 69/69, структурные проверки + реестра проходят). +- Трейлеры коммита: `Issue: #702`, `User-Visible: no` — верно, продуктовый + код (класс A) не менялся. +- PROCESS.md §10.4 обновлён текстом, согласованным с кодом (обе ветки: + «после push» покрывает и fast-forward, и обычный push). +- Ни один другой скрипт конвейера не читает ветку задачи после + `S8-merged` — удаление ничего дальше по цепочке не ломает. +- Число, которое дифф делает видимым (368/370 влитых веток), — не + дублируется в коде: это только текст комментария issue автора, не число + в PROCESS.md или тесте; в самом диффе такого числа нет вовсе — нечему + расходиться. + +## Чего не проверял + +- `npx tsc --noEmit`, `npm test` (полный), `npm run build` + + сверка трёх копий бандла — не перегонял: зелёный Validate на этом же SHA + (`3a681e24`) уже подтверждён: + https://github.com/Matysh/houseplan-card/actions/runs/36482861441 +- Браузерные смоки — не прогонял: `node scripts/smoke-select.mjs --base + origin/dev --head HEAD` сам ответил «исполняемого frontend-диффа нет + (`src/**/*.ts` не тронут) — смоки этим диффом не выбираются, выбирать + нечего». Явных смоков в AC issue не названо. +- `npm run golden:verify` — не прогонял, нет изменений рендера/UI. +- `python -m pytest tests_backend -q` — не прогонял, нет правок + `custom_components/**/*.py`. +- `npm run invariants` — не прогонял, диффа геометрии/модели нет. +- Performance-профили — не названы в AC, не прогонял. +- Реальное поведение `merge-candidate.mjs` целиком на живом GitHub Actions + (реальный `gh`/`GH_TOKEN`, реальные права `HP_PROCESS_TOKEN` на удаление + ветки в этом конкретном репозитории) — не проверял; это уже + ответственность реального прогона `integrate` при следующем слиянии, не + предмет ревью кода. Логика вокруг него (аргументы, условия, регэксп + ошибки) проверена чтением и одним независимым git-экспериментом на + временном репозитории (см. «Как проверялось»). +- Разовая чистка 368 веток (пункт 2 issue) — не в этом коммите, поэтому не + предмет код-ревью; когда владелец даст согласие и появится код/действие, + это отдельный материал. + +## Гейты — сводка + +| Гейт | Прогнан | Результат | +|---|---|---| +| typecheck / npm test / build+bundle-policy | нет — Validate на SHA материала | зелёный: run 36482861441 | +| `node --test test/merge-candidate.test.mjs` | да | 26/26 | +| `node --test test/process-digests.test.mjs` | да | 5/5 | +| `node --test test/mutation-gate.test.mjs` | да (целостность реестра) | 69/69 | +| `node scripts/mutation-gate.mjs --id=merged-task-branch-kept` | да | мутант убит, 1/1 | +| `node scripts/process-gate.mjs --range origin/dev..HEAD` | да | 0 предупреждений | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | да | нет frontend-диффа, смоки не выбираются | +| `golden:verify` | нет | не применимо (нет диффа рендера) | +| `pytest tests_backend` | нет | не применимо (нет правок Python) | +| `invariants` | нет | не применимо (нет правок геометрии) | +| ручной git-эксперимент force-with-lease на удаление ref | да | подтверждает регэксп ошибки в `deleteBranch()` соответствует реальному git | + +## Вердикт + +Зелёный. AC выполнены и доказаны (автотестом + мутантом + независимым +ручным экспериментом с реальным git на защитную часть); находок, +блокирующих или требующих правки в скоупе, нет; единственная Low-находка +снята с записью. + +--- + + + +## Материал раунда + +- Ветка: `issue/702-delete-merged-branches`, коммит `3a681e24e490` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `387871be0d5bb95001506344a933150fb5ca5d16` + ``` + git log --all --format='%H %T' | grep 387871be0d5b + ``` +- Тело issue: `34ce91f776aff8956b77aea955e69a919ee4f52be89c8e94beb90f1e9beb5e85` +- Вердикт конвейера: `green` · High 0