docs: review document for #723

Issue: #723 User-Visible: no
2026-10-05 06:08:59 +00:00 · 2026-09-30 22:26:44 +00:00
parent 0d85807157
commit 3e7f7d3936
1 changed files with 183 additions and 0 deletions
@@ -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-паттерн и упадёт, если кто-то вернёт `<<EOF`; `bash -n` упал бы при обрыве блока `run:` |
+
+Все три AC — из категории «код должен делать одно, а не другое при отказе» (по
+сути защитный/разветвляющий сценарий), и для всех трёх столбец «чем краснеет»
+заполнен результатом реального прогона, а не словом «Verified».
+
+## Как проверялось
+
+1. Прочитан diff файл за файлом (`.github/workflows/release-review.yml`,
+   `.github/workflows/_process.yml`, `scripts/merge-candidate.mjs`,
+   `PROCESS.md`, `test/release-review.test.mjs`) — см. ниже по каждому.
+2. **Прогнан сам новый тест** `node --test test/publish-push-refusal.test.mjs`
+   на материале ревью: 11/11 зелёных.
+3. **Мутационная проверка вручную** (тест должен уметь падать, #723 сам
+   заявляет «на старых workflow красные 9 из 11» — заявление автора, не
+   доказательство): поднял `git worktree add /tmp/hp-dev-check origin/dev
+   --detach`, скопировал в него новый `test/publish-push-refusal.test.mjs` и
+   новый `scripts/merge-candidate.mjs` (источник `refusalSummary`/`--summary`),
+   но оставил старые `.github/workflows/*.yml` из `dev`. Результат:
+   `# pass 2 / # fail 9` — совпадает с заявлением автора дословно. Worktree
+   удалён (`git worktree remove --force`), рабочая копия не изменена.
+4. `bash -n` на извлечённом теле обоих шагов `run:` (тем же способом, что
+   тест вырезает блок — через первую строку с отступом меньше десяти) —
+   чисто на обоих.
+5. `python3 -c "yaml.safe_load(...)"` на обоих workflow-файлах — валидный YAML.
+6. `node scripts/smoke-select.mjs --base origin/dev --head HEAD` — «Исполняемого
+   frontend-диффа нет… Browser-smoke этим диффом не выбираются — это не
+   пропустить проверки, а выбирать нечего». Согласуется с диффом: ни один
+   изменённый файл не относится к `src/**`.
+7. Сверены трейлеры коммита `b4e7eb86`: `Issue: #723`, `User-Visible: no` —
+   есть оба; правка инфраструктурная (workflow-шаги публикации, не
+   пользовательское поведение карточки), classification `User-Visible: no`
+   корректна, changelog не требуется.
+8. Прослежен путь отказа `kind != stale` до места, где он проявится
+   пользователю процесса: в `_process.yml` job уже имеет шаг «Позвать
+   владельца, если стадия упала» (`if: failure()`), так что жёсткий `exit 1`
+   новой веткой не проваливается молча — сеть уведомления не порвана. Для
+   `release-review.yml` (manual dispatch) красный прогон виден тому, кто его
+   запустил — как и раньше, до этой правки, поведение при исчерпании трёх
+   попыток было тем же (`exit 1` в конце цикла).
+
+## Что проверено и корректно
+
+- **Разбор причины (AC1).** Оба шага: `push_err` пишется через `2> "$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`/`<<EOF` с интерполяцией переменных (`$TAG`, `$counts`,
+  `$NUM`) внутри heredoc, переведены на `{ echo …; echo …; } > "$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 уязвимость к
+интерполяции устранена по существу, скоуп не нарушен, трейлеры на месте.
+
+---
+
+<!-- material-anchors: сгенерировано конвейером (#414) -->
+
+## Материал раунда
+
+- Ветка: `issue/723-publish-push-refusal`, коммит `b4e7eb86e680` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
+- Дерево материала: `03963a1231ca2501b3bde09aa1b595db2b618ced`
+  ```
+  git log --all --format='%H %T' | grep 03963a1231ca
+  ```
+- Тело issue: `2632a8969a8ffd8d9b62f5e9430ad9f8efc8fb301df3be0aba993cc6533802b3`
+- Вердикт конвейера: `green` · High 0