mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 20:29:00 +00:00
claude[bot]
@@ -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-хуком, ручная проверка мутанта на красный/зелёный), находок
|
||||
нет.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/705-push-refusal-kinds`, коммит `c17c05341bc5` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `43220b1aa1940537b1d101e7a3e377989273a6ad`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 43220b1aa194
|
||||
```
|
||||
- Тело issue: `4e6ce37d10900c768172f1d382518d679447b0d0207891176c66cd833c344ae7`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user