diff --git a/docs/reviews/CODE-REVIEW-723-r1.md b/docs/reviews/CODE-REVIEW-723-r1.md new file mode 100644 index 00000000..9aedab79 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-723-r1.md @@ -0,0 +1,183 @@ +# CODE-REVIEW-723-r1 + +Issue: #723 · Этап: code · Заход: r1 · Трек: show (PROCESS.md §5, #696) +Материал ревью: `b4e7eb86e6800ae4f7abdf0f9e52aa02855864d6` (ветка `issue/723-publish-push-refusal`, HEAD этой копии) +База сравнения: `origin/dev` = `7188db8187bee4fb601a0edea0acdfa0934b53f6` (dev впереди на 6 коммитов, слияние без конфликта — материал не приводился к dev, трек show) +Validate на материале: success, https://github.com/Matysh/houseplan-card/actions/runs/36784530059 (переиспользован, не перегонялся — #343) + +## Скоуп + +Диапазон `git log --oneline origin/dev..HEAD`: один коммит `b4e7eb86`. +Диф `git diff origin/dev...HEAD --stat`: + +``` +.github/workflows/_process.yml | 44 +++- +.github/workflows/release-review.yml | 35 +++- +PROCESS.md | 5 +- +scripts/merge-candidate.mjs | 44 +++- +test/publish-push-refusal.test.mjs | 395 +++++++++++++++++++++++++++++++++++ +test/release-review.test.mjs | 4 +- +6 files changed, 503 insertions(+), 24 deletions(-) +``` + +Ровно поверхность, названная в ТЗ (`release-review.yml`, `_process.yml`, +тесты) плюс переиспользуемый код разбора отказа в `scripts/merge-candidate.mjs` +(тот же модуль, что несёт `classifyPushRefusal`/`describePushRefusal` для +стража ребейза #705) и одно предложение в PROCESS.md. Выхода за скоуп нет. + +Задача: два шага публикации коммита (документ ревью релиза в `dev` — +`release-review.yml`; документ ревью в ветку задачи — `_process.yml`) +считали любой отказ `git push` сдвигом ветки и слепо повторяли/ребейзили. +Отказ самого GitHub (право на workflow, правило ветки, хук) под эту +диагностику не попадал и превращался в бесполезный повтор без объяснения +причины — тот же класс бага, что чинил #705 для стража ребейза. + +## AC · чем доказан · чем краснеет + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| AC1: оба шага разбирают stderr push через `merge-candidate.mjs --push-refusal`; устаревший lease → прежний повтор/ребейз; отказ GitHub → шаг останавливается без повторов, причина и ответ git без токена — в журнале и `$GITHUB_STEP_SUMMARY` | `test/publish-push-refusal.test.mjs` — оба шага исполняются настоящим bash и настоящим git во временных репозиториях (подменён только транспорт push); проверено чтением + исполнением | На старых (dev) версиях `release-review.yml`/`_process.yml` с тем же тестовым файлом падают 9 из 11 тестов (проверено мной лично, см. «Как проверялось») | +| AC2: `redactSecrets` вычищает токен, URL с учётными данными и `Authorization` из текста сводки/журнала | `noSecrets()`/`noisySecretsGone()` в каждом тесте отказа + отдельный юнит-тест с заголовком `Authorization: Bearer` и чужим токеном в stderr | Тест использует настоящий токен-паттерн (`ghs_…`, `ghp_…`) и заголовок `Authorization: Basic/Bearer …`; без вызова `redactSecrets`/без правки regex тест не пройдёт — проверено локальным запуском | +| AC3: в `run:` обоих шагов нет многострочного текста/heredoc; сводку пишет код (`refusalSummary`), не строка в workflow | Отдельный тест регэкспами проверяет тело `run:` на отсутствие `<<`-heredoc и наличие вызова `merge-candidate.mjs --push-refusal=… --summary=…`; `bash -n` и `python3 -c "yaml.safe_load(...)"` на обоих файлах — мной, см. ниже | Тест ищет буквальный heredoc-паттерн и упадёт, если кто-то вернёт `< "$push_err"`, + далее `kind=$(node …/merge-candidate.mjs --push-refusal=… --ref=… --stage=… + --summary=…) || kind=unknown`; `kind != stale` останавливает шаг с `exit 1` и + понятным `::error::`. Совпадает с существующим паттерном стража ребейза + (#705, `_process.yml:565-591`), включая то, что «прочий» отказ (в том числе + `unknown` — сеть/аутентификация) не лечится повтором — то же решение, что + там уже принято веткой `case … *) exit 1`. +- **`_process.yml` берёт `merge-candidate.mjs` из `dev`** (`git archive + origin/dev scripts | tar -x -C "$tools"`), а не из рабочей копии ветки + задачи — обоснованно: ветка задачи может быть старше и не нести новый + CLI-флаг `--summary`/`--stage`. Тот же приём уже применяет страж ребейза. +- **`refusalSummary`** (`scripts/merge-candidate.mjs:517-547`) собирает текст + кодом, не строкой в `run:`; `unfence()` защищает markdown-блок сводки от + тройных бэктиков в ответе GitHub; вызывается из CLI только когда `kind !== + stale`, то есть сводка появляется ровно тогда, когда шаг не повторяет. + Порядок аргументов (`ref`, `stage`) совпадает на обоих сайтах вызова. +- **AC2 (redactSecrets).** Секреты вычищаются до того, как текст попадает и в + `console.error` (журнал), и в файл `--summary`: оба берут уже + классифицированный `refusal.stderr`/`reason`, которые `classifyPushRefusal` + прогоняет через `redactSecrets` на входе. Тест специально кладёт «шумный» + stderr с заголовком `Authorization: Basic …` и чужим токеном + (`noisyRejected`) — оба вычищены. +- **AC3 (heredoc → построчная запись).** Оба места, ранее писавшие сообщение + коммита через `< "$msg"` + + `commit -q -F "$msg"`. Это не только соответствие форме (PROCESS.md §10.4 + п.4 описывает подрыв heredoc нулевым отступом), но и защита от более + тонкого случая: unquoted heredoc интерполирует переменные до сравнения с + терминатором, и значение, совпавшее с `MSG`/`EOF` на отдельной строке, + оборвало бы heredoc раньше времени. Построчный `echo "...$var..."` этому не + подвержен — переменная разворачивается один раз как аргумент, а не как + часть текста, заново читаемого шеллом на терминатор. + Остальные heredoc-и в репозитории (`_process.yml`, `ship-review.yml`, + `validate.yml`, `release.yml`) не тронуты и не должны быть: они либо несут + статичный текст, либо (`<<'NODE'`) явно закавычены и не интерполируют + переменные — риска, который правит именно этот AC, там нет. +- **PROCESS.md.** Одно предложение у стража ребейза, ссылается на #723 и + описывает то же различение (lease vs отказ GitHub) для обоих шагов + публикации — формулировка совпадает с реализацией. +- **`test/release-review.test.mjs`.** Точечная правка: старая проверка «текст + трейлеров присутствует одной строкой» заменена на проверку «трейлеры пишет + `echo`» — согласуется с тем, что точный текст обоих коммитов теперь проверяет + новый файл, а не этот тест. +- **Не выходит за трек show.** Мутанты по диффу не запрошены и не требуются + (#696); их отсутствие не находка. + +## Находки + +Нет. Ни одной High/Medium/Low находки — ни в скоупе, ни вне его. + +## Чего не проверял + +- **`actionlint`** — бинарь не установлен в этом окружении. Не перегонял; + заменил на `python3 yaml.safe_load` (валидность YAML) и `bash -n` на теле + каждого `run:`-шага (синтаксис bash) — оба чище. `actionlint` добавляет + поверх этого только schema-проверки полей самого workflow (не затронутых + диффом, кроме двух изменённых шагов) — риск низкий, но это не эквивалент + заявленного автором прогона, и я его не воспроизвёл. +- **`npx tsc --noEmit`, `npm test` (полный), `npm run build`** — не + перегонял: Validate уже зелёный на этом самом SHA + (run 36784530059, #343), сверка бандла входит в его состав. Прогнал только + точечно новый файл теста (`node --test test/publish-push-refusal.test.mjs`) + и ручную мутацию на соседнем worktree — этого было достаточно для AC этой + задачи. +- **`npm run invariants`, `npm run golden:verify`, `pytest tests_backend`** — + не применимы: диф не трогает геометрию, рендер карточки, метки `ci:golden` + или `custom_components/**/*.py`. +- **Браузерные смоки** — не прогонял; `smoke-select.mjs` подтвердил, что + выбирать нечего (диф не касается `src/**`), и ни один AC задачи смоук не + называет. +- **`mutation-gate --check`** — не перегонял; трек show не требует мутантов по + диффу (#696), автор привёл число как факультативную информацию, свериться с + ней вне скоупа ревью. +- **Ручное исполнение реальных GitHub Actions ранов** (сам прогон воркфлоу в + CI с намеренным отказом push) не делал — это выходит за рамки дешёвых гейтов + ревью; вместо этого исполнил тело `run:`-шагов как есть настоящим bash и + git во временном песочничном репозитории (то же, что делает + `test/publish-push-refusal.test.mjs`), включая обратную мутацию на старом + коде dev для проверки «тест умеет падать». + +## Вердикт + +Зелёный. Все три AC доказаны исполнением (не только чтением), тест +воспроизводимо красен на коде до правки (9/11, подтверждено независимым +прогоном на `origin/dev`), секреты вычищаются, heredoc уязвимость к +интерполяции устранена по существу, скоуп не нарушен, трейлеры на месте. + +--- + + + +## Материал раунда + +- Ветка: `issue/723-publish-push-refusal`, коммит `b4e7eb86e680` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `03963a1231ca2501b3bde09aa1b595db2b618ced` + ``` + git log --all --format='%H %T' | grep 03963a1231ca + ``` +- Тело issue: `2632a8969a8ffd8d9b62f5e9430ad9f8efc8fb301df3be0aba993cc6533802b3` +- Вердикт конвейера: `green` · High 0