mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-30 11:49:16 +00:00
@@ -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-находка
|
||||
снята с записью.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/702-delete-merged-branches`, коммит `3a681e24e490` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `387871be0d5bb95001506344a933150fb5ca5d16`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 387871be0d5b
|
||||
```
|
||||
- Тело issue: `34ce91f776aff8956b77aea955e69a919ee4f52be89c8e94beb90f1e9beb5e85`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user